| Message ID | 20230912045157.177966-7-claudiu.beznea.uj@bp.renesas.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add new Renesas RZ/G3S SoC and RZ/G3S SMARC EVK | expand |
Hi Claudiu, On Tue, Sep 12, 2023 at 6:52 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: > From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Hardware user manual of RZ/G2L (r01uh0914ej0130-rzg2l-rzg2lc.pdf, > chapter 7.4.7 Procedure for Switching Clocks by the Dynamic Switching > Frequency Selectors) specifies that we need to check CPG_PL2SDHI_DSEL for > SD clock switching status. > > Fixes: eaff33646f4cb ("clk: renesas: rzg2l: Add SDHI clk mux support") > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> Thanks for your patch! > --- a/drivers/clk/renesas/rzg2l-cpg.c > +++ b/drivers/clk/renesas/rzg2l-cpg.c > @@ -188,7 +188,8 @@ static int rzg2l_cpg_sd_clk_mux_set_parent(struct clk_hw *hw, u8 index) > u32 off = GET_REG_OFFSET(hwdata->conf); > u32 shift = GET_SHIFT(hwdata->conf); > const u32 clk_src_266 = 2; > - u32 bitmask; > + u32 msk, val, bitmask; > + int ret; > > /* > * As per the HW manual, we should not directly switch from 533 MHz to > @@ -203,9 +204,6 @@ static int rzg2l_cpg_sd_clk_mux_set_parent(struct clk_hw *hw, u8 index) > */ > bitmask = (GENMASK(GET_WIDTH(hwdata->conf) - 1, 0) << shift) << 16; > if (index != clk_src_266) { > - u32 msk, val; > - int ret; > - > writel(bitmask | ((clk_src_266 + 1) << shift), priv->base + off); > > msk = off ? CPG_CLKSTATUS_SELSDHI1_STS : CPG_CLKSTATUS_SELSDHI0_STS; > @@ -221,7 +219,13 @@ static int rzg2l_cpg_sd_clk_mux_set_parent(struct clk_hw *hw, u8 index) > > writel(bitmask | ((index + 1) << shift), priv->base + off); > > - return 0; > + ret = readl_poll_timeout(priv->base + CPG_CLKSTATUS, val, > + !(val & msk), 100, "msk" may be uninitialized. > + CPG_SDHI_CLK_SWITCH_STATUS_TIMEOUT_US); > + if (ret) > + dev_err(priv->dev, "failed to switch clk source\n"); > + > + return ret; This is now (supposed to be) doing the same thing twice, once using clk_src_266, and then again with the wanted index, so why not introduce a small helper? That would have avoided the uninitialized variable, too. I know you're rewriting this code in "[PATCH 18/37] clk: renesas: rzg2l: refactor sd mux driver", but even after that, you always do a register write before calling rzg2l_cpg_wait_clk_update_done(), so it may still be a net win. > } > > static u8 rzg2l_cpg_sd_clk_mux_get_parent(struct clk_hw *hw) Gr{oetje,eeting}s, Geert
Hi, Geert, On 14.09.2023 14:42, Geert Uytterhoeven wrote: > Hi Claudiu, > > On Tue, Sep 12, 2023 at 6:52 AM Claudiu <claudiu.beznea@tuxon.dev> wrote: >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> >> Hardware user manual of RZ/G2L (r01uh0914ej0130-rzg2l-rzg2lc.pdf, >> chapter 7.4.7 Procedure for Switching Clocks by the Dynamic Switching >> Frequency Selectors) specifies that we need to check CPG_PL2SDHI_DSEL for >> SD clock switching status. >> >> Fixes: eaff33646f4cb ("clk: renesas: rzg2l: Add SDHI clk mux support") >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > Thanks for your patch! > >> --- a/drivers/clk/renesas/rzg2l-cpg.c >> +++ b/drivers/clk/renesas/rzg2l-cpg.c >> @@ -188,7 +188,8 @@ static int rzg2l_cpg_sd_clk_mux_set_parent(struct clk_hw *hw, u8 index) >> u32 off = GET_REG_OFFSET(hwdata->conf); >> u32 shift = GET_SHIFT(hwdata->conf); >> const u32 clk_src_266 = 2; >> - u32 bitmask; >> + u32 msk, val, bitmask; >> + int ret; >> >> /* >> * As per the HW manual, we should not directly switch from 533 MHz to >> @@ -203,9 +204,6 @@ static int rzg2l_cpg_sd_clk_mux_set_parent(struct clk_hw *hw, u8 index) >> */ >> bitmask = (GENMASK(GET_WIDTH(hwdata->conf) - 1, 0) << shift) << 16; >> if (index != clk_src_266) { >> - u32 msk, val; >> - int ret; >> - >> writel(bitmask | ((clk_src_266 + 1) << shift), priv->base + off); >> >> msk = off ? CPG_CLKSTATUS_SELSDHI1_STS : CPG_CLKSTATUS_SELSDHI0_STS; >> @@ -221,7 +219,13 @@ static int rzg2l_cpg_sd_clk_mux_set_parent(struct clk_hw *hw, u8 index) >> >> writel(bitmask | ((index + 1) << shift), priv->base + off); >> >> - return 0; >> + ret = readl_poll_timeout(priv->base + CPG_CLKSTATUS, val, >> + !(val & msk), 100, > > "msk" may be uninitialized. Indeed! I'll update it in next version. > >> + CPG_SDHI_CLK_SWITCH_STATUS_TIMEOUT_US); >> + if (ret) >> + dev_err(priv->dev, "failed to switch clk source\n"); >> + >> + return ret; > > This is now (supposed to be) doing the same thing twice, once using > clk_src_266, and then again with the wanted index, so why not introduce > a small helper? That would have avoided the uninitialized variable, too. Initially I thought about it but I found it too much for this stage as it is only about the readl_poll_timeout() and the debug message. I may keep the debug message in a local variable if you think worth it (but FMPOV it the code will look a bit... unusual). Moreover, as the code is rewritten in patch "[PATCH 18/37] clk: renesas: rzg2l: refactor sd mux driver" I thought it doesn't worth introducing a new helper in this patch. Thank you, Claudiu Beznea > > I know you're rewriting this code in "[PATCH 18/37] clk: renesas: > rzg2l: refactor sd mux driver", but even after that, you always do > a register write before calling rzg2l_cpg_wait_clk_update_done(), > so it may still be a net win. > >> } >> >> static u8 rzg2l_cpg_sd_clk_mux_get_parent(struct clk_hw *hw) > > Gr{oetje,eeting}s, > > Geert >
diff --git a/drivers/clk/renesas/rzg2l-cpg.c b/drivers/clk/renesas/rzg2l-cpg.c index 47f488387f33..70d1c28ba088 100644 --- a/drivers/clk/renesas/rzg2l-cpg.c +++ b/drivers/clk/renesas/rzg2l-cpg.c @@ -188,7 +188,8 @@ static int rzg2l_cpg_sd_clk_mux_set_parent(struct clk_hw *hw, u8 index) u32 off = GET_REG_OFFSET(hwdata->conf); u32 shift = GET_SHIFT(hwdata->conf); const u32 clk_src_266 = 2; - u32 bitmask; + u32 msk, val, bitmask; + int ret; /* * As per the HW manual, we should not directly switch from 533 MHz to @@ -203,9 +204,6 @@ static int rzg2l_cpg_sd_clk_mux_set_parent(struct clk_hw *hw, u8 index) */ bitmask = (GENMASK(GET_WIDTH(hwdata->conf) - 1, 0) << shift) << 16; if (index != clk_src_266) { - u32 msk, val; - int ret; - writel(bitmask | ((clk_src_266 + 1) << shift), priv->base + off); msk = off ? CPG_CLKSTATUS_SELSDHI1_STS : CPG_CLKSTATUS_SELSDHI0_STS; @@ -221,7 +219,13 @@ static int rzg2l_cpg_sd_clk_mux_set_parent(struct clk_hw *hw, u8 index) writel(bitmask | ((index + 1) << shift), priv->base + off); - return 0; + ret = readl_poll_timeout(priv->base + CPG_CLKSTATUS, val, + !(val & msk), 100, + CPG_SDHI_CLK_SWITCH_STATUS_TIMEOUT_US); + if (ret) + dev_err(priv->dev, "failed to switch clk source\n"); + + return ret; } static u8 rzg2l_cpg_sd_clk_mux_get_parent(struct clk_hw *hw)