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 <[email protected]>
> ---
> 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 <[email protected]>
> */
>
> +#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[];
> };
>
Hi Samuel,
On Thu, Mar 14, 2024 at 3:32 PM Samuel Holland
<[email protected]> 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 <[email protected]>
> > --- 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
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hi Geert,
On 2024-03-14 9:48 AM, Geert Uytterhoeven wrote:
> On Thu, Mar 14, 2024 at 3:32 PM Samuel Holland
> <[email protected]> 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 <[email protected]>
>
>>> --- 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
Hi Samuel,
On Thu, Mar 14, 2024 at 3:56 PM Samuel Holland
<[email protected]> wrote:
> On 2024-03-14 9:48 AM, Geert Uytterhoeven wrote:
> > On Thu, Mar 14, 2024 at 3:32 PM Samuel Holland
> > <[email protected]> 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 <[email protected]>
> >
> >>> --- 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
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds