diff mbox series

clk: starfive: jh7100: Use provided clocks instead of hardcoded names

Message ID 898aa0925a9598d44721d00145015b215434cb3b.1710414195.git.geert@linux-m68k.org (mailing list archive)
State Changes Requested, archived
Headers show
Series clk: starfive: jh7100: Use provided clocks instead of hardcoded names | expand

Commit Message

Geert Uytterhoeven March 14, 2024, 11:05 a.m. UTC
The Starfive JH7100 clock driver does not use the DT "clocks" property
to find its external input clocks, but instead relies on the names of
the actual external clock providers.  This is fragile, and caused
breakage when sanitizing clock names in DT.

Fix this by obtaining the external input clocks through the DT "clocks"
property, and using their clk_hw objects or corresponding name.

Fixes: f03606470886 ("riscv: dts: starfive: replace underscores in node names")
Fixes: 4210be668a09ee20 ("clk: starfive: Add JH7100 clock generator driver")
Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
After this is applied, the workaround in commit 7921e231f85a349d
("riscv: dts: starfive: jh7100: fix root clock names") can be reverted.
---
 drivers/clk/starfive/clk-starfive-jh7100.c | 47 +++++++++++++++-------
 drivers/clk/starfive/clk-starfive-jh71x0.h |  1 +
 2 files changed, 33 insertions(+), 15 deletions(-)

Comments

Samuel Holland March 14, 2024, 2:32 p.m. UTC | #1
Hi Geert,

On 2024-03-14 6:05 AM, Geert Uytterhoeven wrote:
> The Starfive JH7100 clock driver does not use the DT "clocks" property
> to find its external input clocks, but instead relies on the names of
> the actual external clock providers.  This is fragile, and caused
> breakage when sanitizing clock names in DT.
> 
> Fix this by obtaining the external input clocks through the DT "clocks"
> property, and using their clk_hw objects or corresponding name.
> 
> Fixes: f03606470886 ("riscv: dts: starfive: replace underscores in node names")
> Fixes: 4210be668a09ee20 ("clk: starfive: Add JH7100 clock generator driver")
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
> After this is applied, the workaround in commit 7921e231f85a349d
> ("riscv: dts: starfive: jh7100: fix root clock names") can be reverted.
> ---
>  drivers/clk/starfive/clk-starfive-jh7100.c | 47 +++++++++++++++-------
>  drivers/clk/starfive/clk-starfive-jh71x0.h |  1 +
>  2 files changed, 33 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/clk/starfive/clk-starfive-jh7100.c b/drivers/clk/starfive/clk-starfive-jh7100.c
> index 0342db24c27e10df..08e6f03f2cfc36c1 100644
> --- a/drivers/clk/starfive/clk-starfive-jh7100.c
> +++ b/drivers/clk/starfive/clk-starfive-jh7100.c
> @@ -7,6 +7,7 @@
>   * Copyright (C) 2021 Emil Renner Berthing <kernel@esmil.dk>
>   */
>  
> +#include <linux/clk.h>
>  #include <linux/clk-provider.h>
>  #include <linux/device.h>
>  #include <linux/init.h>
> @@ -18,10 +19,18 @@
>  #include "clk-starfive-jh71x0.h"
>  
>  /* external clocks */
> -#define JH7100_CLK_OSC_SYS		(JH7100_CLK_END + 0)
> -#define JH7100_CLK_OSC_AUD		(JH7100_CLK_END + 1)
> -#define JH7100_CLK_GMAC_RMII_REF	(JH7100_CLK_END + 2)
> -#define JH7100_CLK_GMAC_GR_MII_RX	(JH7100_CLK_END + 3)
> +enum {
> +	EXT_CLK_OSC_SYS,
> +	EXT_CLK_OSC_AUD,
> +	EXT_CLK_GMAC_RMII_REF,
> +	EXT_CLK_GMAC_GR_MII_RX,
> +	EXT_NUM_CLKS
> +};
> +
> +#define JH7100_CLK_OSC_SYS		(JH7100_CLK_END + EXT_CLK_OSC_SYS)
> +#define JH7100_CLK_OSC_AUD		(JH7100_CLK_END + EXT_CLK_OSC_AUD)
> +#define JH7100_CLK_GMAC_RMII_REF	(JH7100_CLK_END + EXT_CLK_GMAC_RMII_REF)
> +#define JH7100_CLK_GMAC_GR_MII_RX	(JH7100_CLK_END + EXT_CLK_GMAC_GR_MII_RX)
>  
>  static const struct jh71x0_clk_data jh7100_clk_data[] __initconst = {
>  	JH71X0__MUX(JH7100_CLK_CPUNDBUS_ROOT, "cpundbus_root", 0, 4,
> @@ -284,8 +293,12 @@ static struct clk_hw *jh7100_clk_get(struct of_phandle_args *clkspec, void *data
>  
>  static int __init clk_starfive_jh7100_probe(struct platform_device *pdev)
>  {
> +	static const char *jh7100_ext_clk[EXT_NUM_CLKS] =
> +		{ "osc_sys", "osc_aud", "gmac_rmii_ref", "gmac_gr_mii_rxclk" };
>  	struct jh71x0_clk_priv *priv;
> +	const char *osc_sys;
>  	unsigned int idx;
> +	struct clk *clk;
>  	int ret;
>  
>  	priv = devm_kzalloc(&pdev->dev, struct_size(priv, reg, JH7100_CLK_PLL0_OUT), GFP_KERNEL);
> @@ -298,13 +311,23 @@ static int __init clk_starfive_jh7100_probe(struct platform_device *pdev)
>  	if (IS_ERR(priv->base))
>  		return PTR_ERR(priv->base);
>  
> +	for (idx = 0; idx < EXT_NUM_CLKS; idx++) {
> +		clk = devm_clk_get(&pdev->dev, jh7100_ext_clk[idx]);
> +		if (IS_ERR(clk))
> +			return PTR_ERR(clk);
> +
> +		priv->ext[idx] = __clk_get_hw(clk);
> +	}
> +
> +	osc_sys = clk_hw_get_name(priv->ext[EXT_CLK_OSC_SYS]);
> +
>  	priv->pll[0] = devm_clk_hw_register_fixed_factor(priv->dev, "pll0_out",
> -							 "osc_sys", 0, 40, 1);
> +							 osc_sys, 0, 40, 1);
>  	if (IS_ERR(priv->pll[0]))
>  		return PTR_ERR(priv->pll[0]);
>  
>  	priv->pll[1] = devm_clk_hw_register_fixed_factor(priv->dev, "pll1_out",
> -							 "osc_sys", 0, 64, 1);
> +							 osc_sys, 0, 64, 1);

These should use devm_clk_hw_register_fixed_factor_parent_hw(). (Or you could
define a devm_clk_hw_register_fixed_factor_fw_name() and drop the other changes.)

Regards,
Samuel

>  	if (IS_ERR(priv->pll[1]))
>  		return PTR_ERR(priv->pll[1]);
>  
> @@ -331,16 +354,10 @@ static int __init clk_starfive_jh7100_probe(struct platform_device *pdev)
>  
>  			if (pidx < JH7100_CLK_PLL0_OUT)
>  				parents[i].hw = &priv->reg[pidx].hw;
> -			else if (pidx < JH7100_CLK_END)
> +			else if (pidx < JH7100_CLK_OSC_SYS)
>  				parents[i].hw = priv->pll[pidx - JH7100_CLK_PLL0_OUT];
> -			else if (pidx == JH7100_CLK_OSC_SYS)
> -				parents[i].fw_name = "osc_sys";
> -			else if (pidx == JH7100_CLK_OSC_AUD)
> -				parents[i].fw_name = "osc_aud";
> -			else if (pidx == JH7100_CLK_GMAC_RMII_REF)
> -				parents[i].fw_name = "gmac_rmii_ref";
> -			else if (pidx == JH7100_CLK_GMAC_GR_MII_RX)
> -				parents[i].fw_name = "gmac_gr_mii_rxclk";
> +			else if (pidx <= JH7100_CLK_GMAC_GR_MII_RX)
> +				parents[i].hw = priv->ext[pidx - JH7100_CLK_OSC_SYS];
>  		}
>  
>  		clk->hw.init = &init;
> diff --git a/drivers/clk/starfive/clk-starfive-jh71x0.h b/drivers/clk/starfive/clk-starfive-jh71x0.h
> index 23e052fc15495c41..4f46939179cd7418 100644
> --- a/drivers/clk/starfive/clk-starfive-jh71x0.h
> +++ b/drivers/clk/starfive/clk-starfive-jh71x0.h
> @@ -115,6 +115,7 @@ struct jh71x0_clk_priv {
>  	struct device *dev;
>  	void __iomem *base;
>  	struct clk_hw *pll[3];
> +	struct clk_hw *ext[4];
>  	struct jh71x0_clk reg[];
>  };
>
Geert Uytterhoeven March 14, 2024, 2:48 p.m. UTC | #2
Hi Samuel,

On Thu, Mar 14, 2024 at 3:32 PM Samuel Holland
<samuel.holland@sifive.com> wrote:
> On 2024-03-14 6:05 AM, Geert Uytterhoeven wrote:
> > The Starfive JH7100 clock driver does not use the DT "clocks" property
> > to find its external input clocks, but instead relies on the names of
> > the actual external clock providers.  This is fragile, and caused
> > breakage when sanitizing clock names in DT.
> >
> > Fix this by obtaining the external input clocks through the DT "clocks"
> > property, and using their clk_hw objects or corresponding name.
> >
> > Fixes: f03606470886 ("riscv: dts: starfive: replace underscores in node names")
> > Fixes: 4210be668a09ee20 ("clk: starfive: Add JH7100 clock generator driver")
> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>

> > --- a/drivers/clk/starfive/clk-starfive-jh7100.c
> > +++ b/drivers/clk/starfive/clk-starfive-jh7100.c

> > @@ -298,13 +311,23 @@ static int __init clk_starfive_jh7100_probe(struct platform_device *pdev)
> >       if (IS_ERR(priv->base))
> >               return PTR_ERR(priv->base);
> >
> > +     for (idx = 0; idx < EXT_NUM_CLKS; idx++) {
> > +             clk = devm_clk_get(&pdev->dev, jh7100_ext_clk[idx]);
> > +             if (IS_ERR(clk))
> > +                     return PTR_ERR(clk);
> > +
> > +             priv->ext[idx] = __clk_get_hw(clk);
> > +     }
> > +
> > +     osc_sys = clk_hw_get_name(priv->ext[EXT_CLK_OSC_SYS]);
> > +
> >       priv->pll[0] = devm_clk_hw_register_fixed_factor(priv->dev, "pll0_out",
> > -                                                      "osc_sys", 0, 40, 1);
> > +                                                      osc_sys, 0, 40, 1);
> >       if (IS_ERR(priv->pll[0]))
> >               return PTR_ERR(priv->pll[0]);
> >
> >       priv->pll[1] = devm_clk_hw_register_fixed_factor(priv->dev, "pll1_out",
> > -                                                      "osc_sys", 0, 64, 1);
> > +                                                      osc_sys, 0, 64, 1);
>
> These should use devm_clk_hw_register_fixed_factor_parent_hw(). (Or you could

Thanks, I didn't know about that function!

> define a devm_clk_hw_register_fixed_factor_fw_name() and drop the other changes.)

Sorry, I don't understand what you mean here?

Gr{oetje,eeting}s,

                        Geert
Samuel Holland March 14, 2024, 2:56 p.m. UTC | #3
Hi Geert,

On 2024-03-14 9:48 AM, Geert Uytterhoeven wrote:
> On Thu, Mar 14, 2024 at 3:32 PM Samuel Holland
> <samuel.holland@sifive.com> wrote:
>> On 2024-03-14 6:05 AM, Geert Uytterhoeven wrote:
>>> The Starfive JH7100 clock driver does not use the DT "clocks" property
>>> to find its external input clocks, but instead relies on the names of
>>> the actual external clock providers.  This is fragile, and caused
>>> breakage when sanitizing clock names in DT.
>>>
>>> Fix this by obtaining the external input clocks through the DT "clocks"
>>> property, and using their clk_hw objects or corresponding name.
>>>
>>> Fixes: f03606470886 ("riscv: dts: starfive: replace underscores in node names")
>>> Fixes: 4210be668a09ee20 ("clk: starfive: Add JH7100 clock generator driver")
>>> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> 
>>> --- a/drivers/clk/starfive/clk-starfive-jh7100.c
>>> +++ b/drivers/clk/starfive/clk-starfive-jh7100.c
> 
>>> @@ -298,13 +311,23 @@ static int __init clk_starfive_jh7100_probe(struct platform_device *pdev)
>>>       if (IS_ERR(priv->base))
>>>               return PTR_ERR(priv->base);
>>>
>>> +     for (idx = 0; idx < EXT_NUM_CLKS; idx++) {
>>> +             clk = devm_clk_get(&pdev->dev, jh7100_ext_clk[idx]);
>>> +             if (IS_ERR(clk))
>>> +                     return PTR_ERR(clk);
>>> +
>>> +             priv->ext[idx] = __clk_get_hw(clk);
>>> +     }
>>> +
>>> +     osc_sys = clk_hw_get_name(priv->ext[EXT_CLK_OSC_SYS]);
>>> +
>>>       priv->pll[0] = devm_clk_hw_register_fixed_factor(priv->dev, "pll0_out",
>>> -                                                      "osc_sys", 0, 40, 1);
>>> +                                                      osc_sys, 0, 40, 1);
>>>       if (IS_ERR(priv->pll[0]))
>>>               return PTR_ERR(priv->pll[0]);
>>>
>>>       priv->pll[1] = devm_clk_hw_register_fixed_factor(priv->dev, "pll1_out",
>>> -                                                      "osc_sys", 0, 64, 1);
>>> +                                                      osc_sys, 0, 64, 1);
>>
>> These should use devm_clk_hw_register_fixed_factor_parent_hw(). (Or you could
> 
> Thanks, I didn't know about that function!
> 
>> define a devm_clk_hw_register_fixed_factor_fw_name() and drop the other changes.)
> 
> Sorry, I don't understand what you mean here?

In the loop below, the parents are already referenced via .fw_name. That means
the string is the DT clock-names property value, not the Linux-internal clock
name (see clk_core_get()). These two function calls are the only ones that
depend on the internal clock name. If you change them to use .fw_name as well,
the clk_core_get() will do the right thing, and you don't need to manually call
devm_clk_get().

Regards,
Samuel
Geert Uytterhoeven March 14, 2024, 4:16 p.m. UTC | #4
Hi Samuel,

On Thu, Mar 14, 2024 at 3:56 PM Samuel Holland
<samuel.holland@sifive.com> wrote:
> On 2024-03-14 9:48 AM, Geert Uytterhoeven wrote:
> > On Thu, Mar 14, 2024 at 3:32 PM Samuel Holland
> > <samuel.holland@sifive.com> wrote:
> >> On 2024-03-14 6:05 AM, Geert Uytterhoeven wrote:
> >>> The Starfive JH7100 clock driver does not use the DT "clocks" property
> >>> to find its external input clocks, but instead relies on the names of
> >>> the actual external clock providers.  This is fragile, and caused
> >>> breakage when sanitizing clock names in DT.
> >>>
> >>> Fix this by obtaining the external input clocks through the DT "clocks"
> >>> property, and using their clk_hw objects or corresponding name.
> >>>
> >>> Fixes: f03606470886 ("riscv: dts: starfive: replace underscores in node names")
> >>> Fixes: 4210be668a09ee20 ("clk: starfive: Add JH7100 clock generator driver")
> >>> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> >
> >>> --- a/drivers/clk/starfive/clk-starfive-jh7100.c
> >>> +++ b/drivers/clk/starfive/clk-starfive-jh7100.c
> >
> >>> @@ -298,13 +311,23 @@ static int __init clk_starfive_jh7100_probe(struct platform_device *pdev)
> >>>       if (IS_ERR(priv->base))
> >>>               return PTR_ERR(priv->base);
> >>>
> >>> +     for (idx = 0; idx < EXT_NUM_CLKS; idx++) {
> >>> +             clk = devm_clk_get(&pdev->dev, jh7100_ext_clk[idx]);
> >>> +             if (IS_ERR(clk))
> >>> +                     return PTR_ERR(clk);
> >>> +
> >>> +             priv->ext[idx] = __clk_get_hw(clk);
> >>> +     }
> >>> +
> >>> +     osc_sys = clk_hw_get_name(priv->ext[EXT_CLK_OSC_SYS]);
> >>> +
> >>>       priv->pll[0] = devm_clk_hw_register_fixed_factor(priv->dev, "pll0_out",
> >>> -                                                      "osc_sys", 0, 40, 1);
> >>> +                                                      osc_sys, 0, 40, 1);
> >>>       if (IS_ERR(priv->pll[0]))
> >>>               return PTR_ERR(priv->pll[0]);
> >>>
> >>>       priv->pll[1] = devm_clk_hw_register_fixed_factor(priv->dev, "pll1_out",
> >>> -                                                      "osc_sys", 0, 64, 1);
> >>> +                                                      osc_sys, 0, 64, 1);
> >>
> >> These should use devm_clk_hw_register_fixed_factor_parent_hw(). (Or you could
> >
> > Thanks, I didn't know about that function!
> >
> >> define a devm_clk_hw_register_fixed_factor_fw_name() and drop the other changes.)
> >
> > Sorry, I don't understand what you mean here?
>
> In the loop below, the parents are already referenced via .fw_name. That means
> the string is the DT clock-names property value, not the Linux-internal clock
> name (see clk_core_get()). These two function calls are the only ones that
> depend on the internal clock name. If you change them to use .fw_name as well,
> the clk_core_get() will do the right thing, and you don't need to manually call
> devm_clk_get().

Thank you, I wasn't aware these fw_name names were actually doing the
right thing ;-)

Still, looking up by name is more expensive than just using the passed
clk_hw.  As there are multiple occurrences of the four external
clocks in the clocks table, thus involving multiple look-ups, I think
it's better to use clk_hw everywhere.

I will update for v2...

Gr{oetje,eeting}s,

                        Geert
diff mbox series

Patch

diff --git a/drivers/clk/starfive/clk-starfive-jh7100.c b/drivers/clk/starfive/clk-starfive-jh7100.c
index 0342db24c27e10df..08e6f03f2cfc36c1 100644
--- a/drivers/clk/starfive/clk-starfive-jh7100.c
+++ b/drivers/clk/starfive/clk-starfive-jh7100.c
@@ -7,6 +7,7 @@ 
  * Copyright (C) 2021 Emil Renner Berthing <kernel@esmil.dk>
  */
 
+#include <linux/clk.h>
 #include <linux/clk-provider.h>
 #include <linux/device.h>
 #include <linux/init.h>
@@ -18,10 +19,18 @@ 
 #include "clk-starfive-jh71x0.h"
 
 /* external clocks */
-#define JH7100_CLK_OSC_SYS		(JH7100_CLK_END + 0)
-#define JH7100_CLK_OSC_AUD		(JH7100_CLK_END + 1)
-#define JH7100_CLK_GMAC_RMII_REF	(JH7100_CLK_END + 2)
-#define JH7100_CLK_GMAC_GR_MII_RX	(JH7100_CLK_END + 3)
+enum {
+	EXT_CLK_OSC_SYS,
+	EXT_CLK_OSC_AUD,
+	EXT_CLK_GMAC_RMII_REF,
+	EXT_CLK_GMAC_GR_MII_RX,
+	EXT_NUM_CLKS
+};
+
+#define JH7100_CLK_OSC_SYS		(JH7100_CLK_END + EXT_CLK_OSC_SYS)
+#define JH7100_CLK_OSC_AUD		(JH7100_CLK_END + EXT_CLK_OSC_AUD)
+#define JH7100_CLK_GMAC_RMII_REF	(JH7100_CLK_END + EXT_CLK_GMAC_RMII_REF)
+#define JH7100_CLK_GMAC_GR_MII_RX	(JH7100_CLK_END + EXT_CLK_GMAC_GR_MII_RX)
 
 static const struct jh71x0_clk_data jh7100_clk_data[] __initconst = {
 	JH71X0__MUX(JH7100_CLK_CPUNDBUS_ROOT, "cpundbus_root", 0, 4,
@@ -284,8 +293,12 @@  static struct clk_hw *jh7100_clk_get(struct of_phandle_args *clkspec, void *data
 
 static int __init clk_starfive_jh7100_probe(struct platform_device *pdev)
 {
+	static const char *jh7100_ext_clk[EXT_NUM_CLKS] =
+		{ "osc_sys", "osc_aud", "gmac_rmii_ref", "gmac_gr_mii_rxclk" };
 	struct jh71x0_clk_priv *priv;
+	const char *osc_sys;
 	unsigned int idx;
+	struct clk *clk;
 	int ret;
 
 	priv = devm_kzalloc(&pdev->dev, struct_size(priv, reg, JH7100_CLK_PLL0_OUT), GFP_KERNEL);
@@ -298,13 +311,23 @@  static int __init clk_starfive_jh7100_probe(struct platform_device *pdev)
 	if (IS_ERR(priv->base))
 		return PTR_ERR(priv->base);
 
+	for (idx = 0; idx < EXT_NUM_CLKS; idx++) {
+		clk = devm_clk_get(&pdev->dev, jh7100_ext_clk[idx]);
+		if (IS_ERR(clk))
+			return PTR_ERR(clk);
+
+		priv->ext[idx] = __clk_get_hw(clk);
+	}
+
+	osc_sys = clk_hw_get_name(priv->ext[EXT_CLK_OSC_SYS]);
+
 	priv->pll[0] = devm_clk_hw_register_fixed_factor(priv->dev, "pll0_out",
-							 "osc_sys", 0, 40, 1);
+							 osc_sys, 0, 40, 1);
 	if (IS_ERR(priv->pll[0]))
 		return PTR_ERR(priv->pll[0]);
 
 	priv->pll[1] = devm_clk_hw_register_fixed_factor(priv->dev, "pll1_out",
-							 "osc_sys", 0, 64, 1);
+							 osc_sys, 0, 64, 1);
 	if (IS_ERR(priv->pll[1]))
 		return PTR_ERR(priv->pll[1]);
 
@@ -331,16 +354,10 @@  static int __init clk_starfive_jh7100_probe(struct platform_device *pdev)
 
 			if (pidx < JH7100_CLK_PLL0_OUT)
 				parents[i].hw = &priv->reg[pidx].hw;
-			else if (pidx < JH7100_CLK_END)
+			else if (pidx < JH7100_CLK_OSC_SYS)
 				parents[i].hw = priv->pll[pidx - JH7100_CLK_PLL0_OUT];
-			else if (pidx == JH7100_CLK_OSC_SYS)
-				parents[i].fw_name = "osc_sys";
-			else if (pidx == JH7100_CLK_OSC_AUD)
-				parents[i].fw_name = "osc_aud";
-			else if (pidx == JH7100_CLK_GMAC_RMII_REF)
-				parents[i].fw_name = "gmac_rmii_ref";
-			else if (pidx == JH7100_CLK_GMAC_GR_MII_RX)
-				parents[i].fw_name = "gmac_gr_mii_rxclk";
+			else if (pidx <= JH7100_CLK_GMAC_GR_MII_RX)
+				parents[i].hw = priv->ext[pidx - JH7100_CLK_OSC_SYS];
 		}
 
 		clk->hw.init = &init;
diff --git a/drivers/clk/starfive/clk-starfive-jh71x0.h b/drivers/clk/starfive/clk-starfive-jh71x0.h
index 23e052fc15495c41..4f46939179cd7418 100644
--- a/drivers/clk/starfive/clk-starfive-jh71x0.h
+++ b/drivers/clk/starfive/clk-starfive-jh71x0.h
@@ -115,6 +115,7 @@  struct jh71x0_clk_priv {
 	struct device *dev;
 	void __iomem *base;
 	struct clk_hw *pll[3];
+	struct clk_hw *ext[4];
 	struct jh71x0_clk reg[];
 };