Messages in this thread | ![/](/images/icornerl.gif) | | Date | Wed, 20 Mar 2024 08:31:47 -0500 | Subject | Re: [PATCH v2] clk: starfive: jh7100: Use clk_hw for external input clocks | From | Samuel Holland <> |
| |
On 2024-03-20 6:24 AM, Geert Uytterhoeven wrote: > The Starfive JH7100 clock driver does not use the DT "clocks" property > to find the external main input clock, but instead relies on the name of > the actual clock provider ("osc_sys"). This is fragile, and caused > breakage when sanitizing clock node names in DTS. > > Fix this by obtaining the external main input clock using > devm_clk_get(), and passing the returned clk_hw object to > devm_clk_hw_register_fixed_factor_parent_hw(). > > While name-based look-up of the other external input clocks works as-is, > convert them to a similar clk_hw-based scheme to increase uniformity, > and to decrease the number of name-based look-ups. > > 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. > > This is v2 of "[PATCH] clk: starfive: jh7100: Use provided clocks > instead of hardcoded names" > https://lore.kernel.org/r/898aa0925a9598d44721d00145015b215434cb3b.1710414195.git.geert@linux-m68k.org/ > > v2: > - Use devm_clk_hw_register_fixed_factor_parent_hw(), > - Drop no longer needed local osc_sys name. > --- > drivers/clk/starfive/clk-starfive-jh7100.c | 48 ++++++++++++++-------- > drivers/clk/starfive/clk-starfive-jh71x0.h | 1 + > 2 files changed, 32 insertions(+), 17 deletions(-) > > diff --git a/drivers/clk/starfive/clk-starfive-jh7100.c b/drivers/clk/starfive/clk-starfive-jh7100.c > index 0342db24c27e10df..ead5a7b14bab9045 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,11 @@ 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" };
This should be __initconst. Otherwise:
Reviewed-by: Samuel Holland <samuel.holland@sifive.com>
> struct jh71x0_clk_priv *priv; > 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 +310,21 @@ static int __init clk_starfive_jh7100_probe(struct platform_device *pdev) > if (IS_ERR(priv->base)) > return PTR_ERR(priv->base); > > - priv->pll[0] = devm_clk_hw_register_fixed_factor(priv->dev, "pll0_out", > - "osc_sys", 0, 40, 1); > + 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); > + } > + > + priv->pll[0] = devm_clk_hw_register_fixed_factor_parent_hw(priv->dev, > + "pll0_out", priv->ext[EXT_CLK_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); > + priv->pll[1] = devm_clk_hw_register_fixed_factor_parent_hw(priv->dev, > + "pll1_out", priv->ext[EXT_CLK_OSC_SYS], 0, 64, 1); > if (IS_ERR(priv->pll[1])) > return PTR_ERR(priv->pll[1]); > > @@ -331,16 +351,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[]; > }; >
| ![\](/images/icornerr.gif) |