Re: [PATCH 18/37] clk: renesas: rzg2l: refactor sd mux driver

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

Hi Claudiu,

On Fri, Sep 15, 2023 at 9:30 AM claudiu beznea <claudiu.beznea@xxxxxxxxx> wrote:
> On 14.09.2023 18:18, Geert Uytterhoeven wrote:
> > On Tue, Sep 12, 2023 at 6:52 AM Claudiu <claudiu.beznea@xxxxxxxxx> wrote:
> >> From: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> >>
> >> Refactor SD MUX driver to be able to reuse the same code on RZ/G3S.
> >> RZ/G2{L, UL} has a limitation with regards to switching the clock source
> >> for SD MUX (MUX clock source has to be switched to 266MHz before switching
> >> b/w 533MHz and 400MHz). This limitation has been introduced as a clock
> >> notifier that is registered on platform based initialization data thus the
> >> SD MUX code could be reused on RZ/G3S.
> >>
> >> As both RZ/G2{L, UL} and RZ/G3S has specific bits in specific registers
> >> to check if the clock switching has been done, this configuration (register
> >> offset, register bits and bits width) is now passed though
> >> struct cpg_core_clk::sconf (status configuration) from platform specific
> >> initialization code.
> >>
> >> Along with struct cpg_core_clk::sconf the mux table indexes is also
> >
> > indices are
> >
> >> passed from platform specific initialization code.
> >
> > Please also mention the passing of the mux flags, which is added so
> > you can pass CLK_SET_PARENT_GATE for G3S_SEL_PLL4 later.
>
> Ok.
>
> >
> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> >
> >> --- a/drivers/clk/renesas/r9a07g043-cpg.c
> >> +++ b/drivers/clk/renesas/r9a07g043-cpg.c
> >> @@ -21,6 +21,10 @@
> >>  #define G2UL_SEL_SDHI0         SEL_PLL_PACK(G2UL_CPG_PL2SDHI_DSEL, 0, 2)
> >>  #define G2UL_SEL_SDHI1         SEL_PLL_PACK(G2UL_CPG_PL2SDHI_DSEL, 4, 2)
> >>
> >> +/* Clock status configuration. */
> >> +#define G2UL_SEL_SDHI0_STS     SEL_PLL_PACK(CPG_CLKSTATUS, 28, 1)
> >> +#define G2UL_SEL_SDHI1_STS     SEL_PLL_PACK(CPG_CLKSTATUS, 29, 1)
> >
> > Just like in [PATCH 17/37], there is no real need for the "G2UL_"-prefix.
>
> Ok, I ususlly tend to guard everything with a proper namespace.

Sure, in many cases, that makes good sense.
In this case, not having the prefix makes it easier to compare clock tables:

    soc-dts-diff -b drivers/clk/renesas/r9a07g04[34]-cpg.c

(soc-dts-diff ignores the SoC part number, and can be found at
 https://github.com/geertu/linux-scripts)

> >> +int rzg2l_cpg_sd_mux_clk_notifier(struct notifier_block *nb, unsigned long event,
> >> +                                 void *data)
> >> +{
> >> +       struct clk_notifier_data *cnd = data;
> >> +       struct clk_hw *hw = __clk_get_hw(cnd->clk);
> >> +       struct clk_hw_data *clk_hw_data = to_clk_hw_data(hw);
> >> +       struct rzg2l_cpg_priv *priv = clk_hw_data->priv;
> >> +       u32 off = GET_REG_OFFSET(clk_hw_data->conf);
> >> +       u32 shift = GET_SHIFT(clk_hw_data->conf);
> >> +       const u32 clk_src_266 = 3;
> >> +       unsigned long flags;
> >> +       u32 bitmask;
> >> +       int ret;
> >> +
> >> +       if (event != PRE_RATE_CHANGE || (cnd->new_rate / MEGA == 266))
> >> +               return 0;
> >> +
> >> +       spin_lock_irqsave(&priv->rmw_lock, flags);
> >> +
> >> +       /*
> >> +        * As per the HW manual, we should not directly switch from 533 MHz to
> >> +        * 400 MHz and vice versa. To change the setting from 2’b01 (533 MHz)
> >> +        * to 2’b10 (400 MHz) or vice versa, Switch to 2’b11 (266 MHz) first,
> >> +        * and then switch to the target setting (2’b01 (533 MHz) or 2’b10
> >> +        * (400 MHz)).
> >> +        * Setting a value of '0' to the SEL_SDHI0_SET or SEL_SDHI1_SET clock
> >> +        * switching register is prohibited.
> >> +        * The clock mux has 3 input clocks(533 MHz, 400 MHz, and 266 MHz), and
> >> +        * the index to value mapping is done by adding 1 to the index.
> >> +        */
> >> +       bitmask = (GENMASK(GET_WIDTH(clk_hw_data->conf) - 1, 0) << shift) << 16;
> >> +       writel(bitmask | (clk_src_266 << shift), priv->base + off);
> >> +
> >> +       /* Wait for the update done. */
> >> +       ret = rzg2l_cpg_wait_clk_update_done(priv->base, clk_hw_data->sconf);
> >> +
> >> +       spin_unlock_irqrestore(&priv->rmw_lock, flags);
> >> +
> >> +       if (ret)
> >> +               dev_err(priv->dev, "failed to switch to safe clk source\n");
> >> +
> >> +       return ret;
> >> +}
> >> +
> >> +static int rzg2l_register_notifier(struct clk_hw *hw, const struct cpg_core_clk *core,
> >> +                                  struct rzg2l_cpg_priv *priv)
> >> +{
> >> +       struct notifier_block *nb;
> >> +
> >> +       if (!core->notifier)
> >> +               return 0;
> >> +
> >> +       nb = devm_kzalloc(priv->dev, sizeof(*nb), GFP_KERNEL);
> >> +       if (!nb)
> >> +               return -ENOMEM;
> >> +
> >> +       nb->notifier_call = core->notifier;
> >> +
> >> +       return clk_notifier_register(hw->clk, nb);
> >> +}
> >
> > I am not sure a notifier is the best solution.  Basically on RZ/G2L,
> > when changing the parent clock, you need to switch to a fixed
> > intermediate parent first.
> > What about just replacing the fixed clk_src_266 in the old
> > rzg2l_cpg_sd_mux_clk_set_parent() by a (signed) integer in
> > sd_mux_hw_data (specified in DEF_SD_MUX()), representing the index
> > of the intermediate clock?
> > -1 would mean an intermediate parent is not needed.
>
> That should work too but .set_rate() will be bulky for both mux and div.
>
> The idea was to have the .set_rate() common to the mux and the platform
> specificities implemented as notifiers and only the needed platforms to
> instantiate the notifier. And the same approach to be used by the divider
> (patch "[PATCH 19/37] clk: renesas: rzg2l: add a divider clock for RZ/G3S")
>
> With this it looked to me that the final code is more compact .set_rate
> being simple and platform specificities being implemented in notifier
> (valid for both MUX and DIV). The infrastructure is already there for
> notifier to be called before .set_rate().

TBH, I am not that familiar with clock notifiers, so I could use some
guidance from the clock maintainers.

Mike/Stephen: Are clock notifiers the right approach, here and in
              [PATCH 19.37]?

> >> --- a/drivers/clk/renesas/rzg2l-cpg.h
> >> +++ b/drivers/clk/renesas/rzg2l-cpg.h
> >> @@ -272,4 +278,9 @@ extern const struct rzg2l_cpg_info r9a07g044_cpg_info;
> >>  extern const struct rzg2l_cpg_info r9a07g054_cpg_info;
> >>  extern const struct rzg2l_cpg_info r9a09g011_cpg_info;
> >>
> >> +int rzg2l_cpg_sd_mux_clk_notifier(struct notifier_block *nb, unsigned long event, void *data);
> >> +
> >> +/* Macros to be used in platform specific initialization code. */
> >> +#define SD_MUX_NOTIF           (&rzg2l_cpg_sd_mux_clk_notifier)
> >
> > Any specific reason you are adding this macro?
>
> It looked to me like a better name to be used in platform specific drivers.
>
> > What is wrong with using &rzg2l_cpg_sd_mux_clk_notifier directly?
>
> Nothing, just that it is a longer than SD_MUX_NOTIF.

It adds another level of indirection for the casual reviewer, and needs
replacement when an SoC arrives that needs a different SD mux notifier.

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

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