diff mbox series

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

Message ID 20190605235714.22432-1-papadakospan@gmail.com
State Changes Requested, archived
Headers show
Series [1/2] clk: rockchip: add clock for the watchdog pclk on rk3328 | expand

Checks

Context Check Description
robh/checkpatch warning "total: 0 errors, 3 warnings, 28 lines checked"

Commit Message

Leonidas-Panagiotis Papadakos June 5, 2019, 11:57 p.m. UTC 
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(+)

Comments

Heiko Stuebner June 6, 2019, 10:02 a.m. UTC | #1 
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
Leonidas-Panagiotis Papadakos June 6, 2019, 11:42 a.m. UTC | #2 
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:
Heiko Stuebner June 14, 2019, 9:50 a.m. UTC | #3 
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:
> 
> 
>
Leonidas-Panagiotis Papadakos June 14, 2019, 9:54 a.m. UTC | #4 
> 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
Heiko Stuebner June 14, 2019, 10:31 a.m. UTC | #5 
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
Leonidas-Panagiotis Papadakos June 14, 2019, 7:47 p.m. UTC | #6 
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
diff mbox series

Patch

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