[PATCH 1/2] clk: rockchip: add clock for the watchdog pclk on rk3328

[PATCH 1/2] clk: rockchip: add clock for the watchdog pclk on rk3328

[Leonidas P. Papadakos]

From: <itdaniher@gmail.com>

Following the discussion here:
https://github.com/rockchip-linux/kernel/issues/123

it can be seen that these are the changes needed to enable the use of the hardware watchdog in the rk3328 SoC.

This is in line with past changes for the rk3288:
http://lists.infradead.org/pipermail/linux-rockchip/2015-January/002314.html

Signed-off-by: Leonidas P. Papadakos <papadakospan@gmail.com>
---
 drivers/clk/rockchip/clk-rk3328.c      | 9 +++++++++
 include/dt-bindings/clock/rk3328-cru.h | 1 +
 2 files changed, 10 insertions(+)

diff --git a/drivers/clk/rockchip/clk-rk3328.c b/drivers/clk/rockchip/clk-rk3328.c
index 076b9777a..546ee0ab7 100644
--- a/drivers/clk/rockchip/clk-rk3328.c
+++ b/drivers/clk/rockchip/clk-rk3328.c
@@ -876,6 +876,8 @@ static const char *const rk3328_critical_clocks[] __initconst = {
 
 static void __init rk3328_clk_init(struct device_node *np)
 {
+	struct clk *clk;
+
 	struct rockchip_clk_provider *ctx;
 	void __iomem *reg_base;
 
@@ -892,6 +894,13 @@ static void __init rk3328_clk_init(struct device_node *np)
 		return;
 	}
 
+	clk = clk_register_fixed_factor(NULL, "pclk_wdt", "pclk_bus", 0, 1, 1);
+	if (IS_ERR(clk))
+		pr_warn("%s: could not register clock pclk_wdt: %ld\n",
+			__func__, PTR_ERR(clk));
+	else
+		rockchip_clk_add_lookup(ctx, clk, PCLK_WDT);
+
 	rockchip_clk_register_plls(ctx, rk3328_pll_clks,
 				   ARRAY_SIZE(rk3328_pll_clks),
 				   RK3328_GRF_SOC_STATUS0);
diff --git a/include/dt-bindings/clock/rk3328-cru.h b/include/dt-bindings/clock/rk3328-cru.h
index afb811340..555b4ff66 100644
--- a/include/dt-bindings/clock/rk3328-cru.h
+++ b/include/dt-bindings/clock/rk3328-cru.h
@@ -164,6 +164,7 @@
 #define PCLK_DCF		233
 #define PCLK_SARADC		234
 #define PCLK_ACODECPHY		235
+#define PCLK_WDT		236
 
 /* hclk gates */
 #define HCLK_PERI		308
-- 
2.21.0

[PATCH 2/2] arm64: dts: rockchip: enable rk3328 watchdog clock

[Leonidas P. Papadakos]

From: <itdaniher@gmail.com>

Add the missing clock property for the watchdog on rk3328.

This patch depends on:
clk: rockchip: add clock for the watchdog pclk on rk3328

Signed-off-by: Leonidas P. Papadakos <papadakospan@gmail.com>
---
 arch/arm64/boot/dts/rockchip/rk3328-roc-cc.dts | 4 ++++
 arch/arm64/boot/dts/rockchip/rk3328-rock64.dts | 4 ++++
 arch/arm64/boot/dts/rockchip/rk3328.dtsi       | 2 ++
 3 files changed, 10 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3328-roc-cc.dts b/arch/arm64/boot/dts/rockchip/rk3328-roc-cc.dts
index 5d499c908..49d51dd93 100644
--- a/arch/arm64/boot/dts/rockchip/rk3328-roc-cc.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3328-roc-cc.dts
@@ -349,3 +349,7 @@
 &vop_mmu {
 	status = "okay";
 };
+
+&wdt {
+	status = "okay";
+};
diff --git a/arch/arm64/boot/dts/rockchip/rk3328-rock64.dts b/arch/arm64/boot/dts/rockchip/rk3328-rock64.dts
index 7cfd5ca6c..18a3e0dfa 100644
--- a/arch/arm64/boot/dts/rockchip/rk3328-rock64.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3328-rock64.dts
@@ -405,3 +405,7 @@
 &vop_mmu {
 	status = "okay";
 };
+
+&wdt {
+	status = "okay";
+};
diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
index 994468671..e0a6279da 100644
--- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
@@ -407,6 +407,8 @@
 		compatible = "snps,dw-wdt";
 		reg = <0x0 0xff1a0000 0x0 0x100>;
 		interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&cru PCLK_WDT>;
+		status = "disabled";
 	};
 
 	pwm0: pwm@ff1b0000 {
-- 
2.21.0

Re: [PATCH 1/2] clk: rockchip: add clock for the watchdog pclk on rk3328

[Heiko Stuebner]

Hi,

Am Donnerstag, 6. Juni 2019, 01:57:13 CEST schrieb Leonidas P. Papadakos:
> From: <itdaniher@gmail.com>

Why is the From different from the Signed-off-by? Would also need a full name.
If the patch is from you, please just use the same From as for the Signed-off-by.

> 
> Following the discussion here:
> https://github.com/rockchip-linux/kernel/issues/123
> 
> it can be seen that these are the changes needed to enable the use of the hardware watchdog in the rk3328 SoC.
> 
> This is in line with past changes for the rk3288:
> http://lists.infradead.org/pipermail/linux-rockchip/2015-January/002314.html
> 
> Signed-off-by: Leonidas P. Papadakos <papadakospan@gmail.com>
> ---
>  drivers/clk/rockchip/clk-rk3328.c      | 9 +++++++++
>  include/dt-bindings/clock/rk3328-cru.h | 1 +
>  2 files changed, 10 insertions(+)
> 
> diff --git a/drivers/clk/rockchip/clk-rk3328.c b/drivers/clk/rockchip/clk-rk3328.c
> index 076b9777a..546ee0ab7 100644
> --- a/drivers/clk/rockchip/clk-rk3328.c
> +++ b/drivers/clk/rockchip/clk-rk3328.c
> @@ -876,6 +876,8 @@ static const char *const rk3328_critical_clocks[] __initconst = {
>  
>  static void __init rk3328_clk_init(struct device_node *np)
>  {
> +	struct clk *clk;
> +
>  	struct rockchip_clk_provider *ctx;
>  	void __iomem *reg_base;
>  
> @@ -892,6 +894,13 @@ static void __init rk3328_clk_init(struct device_node *np)
>  		return;
>  	}
>  
> +	clk = clk_register_fixed_factor(NULL, "pclk_wdt", "pclk_bus", 0, 1, 1);
> +	if (IS_ERR(clk))
> +		pr_warn("%s: could not register clock pclk_wdt: %ld\n",
> +			__func__, PTR_ERR(clk));
> +	else
> +		rockchip_clk_add_lookup(ctx, clk, PCLK_WDT);
> +

I've just Cc'ed you on 2 patches adding a SGRF_GATE clock-type. Please
use that as base for you rk3328-wdt-clock, so that we don't introduce more
boilderplate code.


>  	rockchip_clk_register_plls(ctx, rk3328_pll_clks,
>  				   ARRAY_SIZE(rk3328_pll_clks),
>  				   RK3328_GRF_SOC_STATUS0);

> diff --git a/include/dt-bindings/clock/rk3328-cru.h b/include/dt-bindings/clock/rk3328-cru.h
> index afb811340..555b4ff66 100644
> --- a/include/dt-bindings/clock/rk3328-cru.h
> +++ b/include/dt-bindings/clock/rk3328-cru.h
> @@ -164,6 +164,7 @@
>  #define PCLK_DCF		233
>  #define PCLK_SARADC		234
>  #define PCLK_ACODECPHY		235
> +#define PCLK_WDT		236
>  
>  /* hclk gates */
>  #define HCLK_PERI		308

please split the addition of the clock-id into a separate patch only adding
said id.


Thanks
Heiko

Re: [PATCH 1/2] clk: rockchip: add clock for the watchdog pclk on rk3328

[Leonidas P. Papadakos]

Hi,
> 
> Am Donnerstag, 6. Juni 2019, 01:57:13 CEST schrieb Leonidas P. 
> Papadakos:
>>  From: <itdaniher@gmail.com>
> 
> Why is the From different from the Signed-off-by? Would also need a 
> full name.
> If the patch is from you, please just use the same From as for the 
> Signed-off-by.
> 

I mistakenly though this was the way to credit someone for a patch, but 
it seems to be different.
I'll Cc: the author of this patch

>> 
>>  Following the discussion here:
>>  https://github.com/rockchip-linux/kernel/issues/123
>> 
>>  it can be seen that these are the changes needed to enable the use 
>> of the hardware watchdog in the rk3328 SoC.
>> 
>>  This is in line with past changes for the rk3288:
>>  
>> http://lists.infradead.org/pipermail/linux-rockchip/2015-January/002314.html
>> 
>>  Signed-off-by: Leonidas P. Papadakos <papadakospan@gmail.com>
>>  ---
>>   drivers/clk/rockchip/clk-rk3328.c      | 9 +++++++++
>>   include/dt-bindings/clock/rk3328-cru.h | 1 +
>>   2 files changed, 10 insertions(+)
>> 
>>  diff --git a/drivers/clk/rockchip/clk-rk3328.c 
>> b/drivers/clk/rockchip/clk-rk3328.c
>>  index 076b9777a..546ee0ab7 100644
>>  --- a/drivers/clk/rockchip/clk-rk3328.c
>>  +++ b/drivers/clk/rockchip/clk-rk3328.c
>>  @@ -876,6 +876,8 @@ static const char *const 
>> rk3328_critical_clocks[] __initconst = {
>> 
>>   static void __init rk3328_clk_init(struct device_node *np)
>>   {
>>  +	struct clk *clk;
>>  +
>>   	struct rockchip_clk_provider *ctx;
>>   	void __iomem *reg_base;
>> 
>>  @@ -892,6 +894,13 @@ static void __init rk3328_clk_init(struct 
>> device_node *np)
>>   		return;
>>   	}
>> 
>>  +	clk = clk_register_fixed_factor(NULL, "pclk_wdt", "pclk_bus", 0, 
>> 1, 1);
>>  +	if (IS_ERR(clk))
>>  +		pr_warn("%s: could not register clock pclk_wdt: %ld\n",
>>  +			__func__, PTR_ERR(clk));
>>  +	else
>>  +		rockchip_clk_add_lookup(ctx, clk, PCLK_WDT);
>>  +
> 
> I've just Cc'ed you on 2 patches adding a SGRF_GATE clock-type. Please
> use that as base for you rk3328-wdt-clock, so that we don't introduce 
> more
> boilderplate code.
> 
> 
>>   	rockchip_clk_register_plls(ctx, rk3328_pll_clks,
>>   				   ARRAY_SIZE(rk3328_pll_clks),
>>   				   RK3328_GRF_SOC_STATUS0);
> 
>>  diff --git a/include/dt-bindings/clock/rk3328-cru.h 
>> b/include/dt-bindings/clock/rk3328-cru.h
>>  index afb811340..555b4ff66 100644
>>  --- a/include/dt-bindings/clock/rk3328-cru.h
>>  +++ b/include/dt-bindings/clock/rk3328-cru.h
>>  @@ -164,6 +164,7 @@
>>   #define PCLK_DCF		233
>>   #define PCLK_SARADC		234
>>   #define PCLK_ACODECPHY		235
>>  +#define PCLK_WDT		236
>> 
>>   /* hclk gates */
>>   #define HCLK_PERI		308
> 
> please split the addition of the clock-id into a separate patch only 
> adding
> said id.
> 
> 
> Thanks
> Heiko
> 

I know less than him on this, but I want the feedback to be visible in 
the Cc:

Re: [PATCH 1/2] clk: rockchip: add clock for the watchdog pclk on rk3328

[Heiko Stuebner]

Hi,

Am Donnerstag, 6. Juni 2019, 13:42:20 CEST schrieb Leonidas P. Papadakos:
> 
> Hi,
> > 
> > Am Donnerstag, 6. Juni 2019, 01:57:13 CEST schrieb Leonidas P. 
> > Papadakos:
> >>  From: <itdaniher@gmail.com>
> > 
> > Why is the From different from the Signed-off-by? Would also need a 
> > full name.
> > If the patch is from you, please just use the same From as for the 
> > Signed-off-by.
> > 
> 
> I mistakenly though this was the way to credit someone for a patch, but 
> it seems to be different.
> I'll Cc: the author of this patch

Were you able yet to take a look at the clock-patches I Cc'ed you on
and look at reworking your patch accrodingly?


Thanks
Heiko

> 
> >> 
> >>  Following the discussion here:
> >>  https://github.com/rockchip-linux/kernel/issues/123
> >> 
> >>  it can be seen that these are the changes needed to enable the use 
> >> of the hardware watchdog in the rk3328 SoC.
> >> 
> >>  This is in line with past changes for the rk3288:
> >>  
> >> http://lists.infradead.org/pipermail/linux-rockchip/2015-January/002314.html
> >> 
> >>  Signed-off-by: Leonidas P. Papadakos <papadakospan@gmail.com>
> >>  ---
> >>   drivers/clk/rockchip/clk-rk3328.c      | 9 +++++++++
> >>   include/dt-bindings/clock/rk3328-cru.h | 1 +
> >>   2 files changed, 10 insertions(+)
> >> 
> >>  diff --git a/drivers/clk/rockchip/clk-rk3328.c 
> >> b/drivers/clk/rockchip/clk-rk3328.c
> >>  index 076b9777a..546ee0ab7 100644
> >>  --- a/drivers/clk/rockchip/clk-rk3328.c
> >>  +++ b/drivers/clk/rockchip/clk-rk3328.c
> >>  @@ -876,6 +876,8 @@ static const char *const 
> >> rk3328_critical_clocks[] __initconst = {
> >> 
> >>   static void __init rk3328_clk_init(struct device_node *np)
> >>   {
> >>  +	struct clk *clk;
> >>  +
> >>   	struct rockchip_clk_provider *ctx;
> >>   	void __iomem *reg_base;
> >> 
> >>  @@ -892,6 +894,13 @@ static void __init rk3328_clk_init(struct 
> >> device_node *np)
> >>   		return;
> >>   	}
> >> 
> >>  +	clk = clk_register_fixed_factor(NULL, "pclk_wdt", "pclk_bus", 0, 
> >> 1, 1);
> >>  +	if (IS_ERR(clk))
> >>  +		pr_warn("%s: could not register clock pclk_wdt: %ld\n",
> >>  +			__func__, PTR_ERR(clk));
> >>  +	else
> >>  +		rockchip_clk_add_lookup(ctx, clk, PCLK_WDT);
> >>  +
> > 
> > I've just Cc'ed you on 2 patches adding a SGRF_GATE clock-type. Please
> > use that as base for you rk3328-wdt-clock, so that we don't introduce 
> > more
> > boilderplate code.
> > 
> > 
> >>   	rockchip_clk_register_plls(ctx, rk3328_pll_clks,
> >>   				   ARRAY_SIZE(rk3328_pll_clks),
> >>   				   RK3328_GRF_SOC_STATUS0);
> > 
> >>  diff --git a/include/dt-bindings/clock/rk3328-cru.h 
> >> b/include/dt-bindings/clock/rk3328-cru.h
> >>  index afb811340..555b4ff66 100644
> >>  --- a/include/dt-bindings/clock/rk3328-cru.h
> >>  +++ b/include/dt-bindings/clock/rk3328-cru.h
> >>  @@ -164,6 +164,7 @@
> >>   #define PCLK_DCF		233
> >>   #define PCLK_SARADC		234
> >>   #define PCLK_ACODECPHY		235
> >>  +#define PCLK_WDT		236
> >> 
> >>   /* hclk gates */
> >>   #define HCLK_PERI		308
> > 
> > please split the addition of the clock-id into a separate patch only 
> > adding
> > said id.
> > 
> > 
> > Thanks
> > Heiko
> > 
> 
> I know less than him on this, but I want the feedback to be visible in 
> the Cc:
> 
> 
> 

Re: [PATCH 1/2] clk: rockchip: add clock for the watchdog pclk on rk3328

[Leonidas P. Papadakos]

> Were you able yet to take a look at the clock-patches I Cc'ed you on
> and look at reworking your patch accrodingly?
> 
> 
> Thanks
> Heiko
> 
My time is limited due to exams, and I have no knowledge on how the clk 
stuff works, but I'll read up when I have the time. The patched you 
CCed me on is certainly helpful in this regard

Re: [PATCH 1/2] clk: rockchip: add clock for the watchdog pclk on rk3328

[Heiko Stuebner]

Hi,

Am Freitag, 14. Juni 2019, 11:54:14 CEST schrieb Leonidas P. Papadakos:
> 
> > Were you able yet to take a look at the clock-patches I Cc'ed you on
> > and look at reworking your patch accrodingly?
> > 
> > 
> > Thanks
> > Heiko
> > 
> My time is limited due to exams, and I have no knowledge on how the clk 
> stuff works, but I'll read up when I have the time. The patched you 
> CCed me on is certainly helpful in this regard

ok. I have tested the conversion on the platforms I did convert there,
so I'll just apply the 2 patches later on.

Should I wait on you respinning the rk3328 watchdog patch, or just
add the rk3328 watchdog pclk myself?


Heiko

Re: [PATCH 1/2] clk: rockchip: add clock for the watchdog pclk on rk3328

[Leonidas P. Papadakos]

ok. I have tested the conversion on the platforms I did convert there,
> so I'll just apply the 2 patches later on.
> 
> Should I wait on you respinning the rk3328 watchdog patch, or just
> add the rk3328 watchdog pclk myself?
> 
> 
> Heiko
> 

Would be awesome if you added it yourself. You seem to understand this 
a lot better.

Also, I checked out the patch you CCed me on and I'm happy to see that 
the reused code has been replaced with a single unified macro. So much 
tidier.

As far as I understand then, it's a case of adding the pclk id, the 
sgrf thing and enable it in the dts. Cool!
Hoping to see it in 5.3