Re: [PATCH v2 10/28] clk: renesas: rzg2l: refactor sd mux driver

[Geert Uytterhoeven]

Hi Claudiu,

On Fri, Sep 29, 2023 at 7:39 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 indices are also
> passed from platform specific initialization code.
>
> Also, mux flags are now passed to DEF_SD_MUX() as they will be later
> used by RZ/G3S.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> ---
>
> Changes in v2:
> - s/indexes/indices in commit description
> - mentioned in commit description that mux flags can now be passed to
>   driver though DEF_SD_MUX() macro
> - removed SoC specific names from macros' names
> - added spaces after { and before } when initializing arrays
> - preserved the order of .[gs]set_parent() API definitions for simpler
>   diff b/w versions
> - removed SD_MUX_NOTIF macro

Thanks for the update!

> --- a/drivers/clk/renesas/rzg2l-cpg.c
> +++ b/drivers/clk/renesas/rzg2l-cpg.c

> @@ -142,6 +146,77 @@ static void rzg2l_cpg_del_clk_provider(void *data)
>         of_clk_del_provider(data);
>  }
>
> +/* Must be called in atomic context. */
> +static int rzg2l_cpg_wait_clk_update_done(void __iomem *base, u32 conf)
> +{
> +       u32 bitmask = GENMASK(GET_WIDTH(conf) - 1, 0) << GET_SHIFT(conf);
> +       u32 off = GET_REG_OFFSET(conf);
> +       u32 val;
> +
> +       return readl_poll_timeout_atomic(base + off, val, !(val & bitmask), 10, 200);
> +}
> +
> +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;

include/linux/clk.h:

 * PRE_RATE_CHANGE - called immediately before the clk rate is changed,
 *     to indicate that the rate change will proceed.  Drivers must
 *     immediately terminate any operations that will be affected by the
 *     rate change.  Callbacks may either return NOTIFY_DONE, NOTIFY_OK,
 *     NOTIFY_STOP or NOTIFY_BAD.

> +
> +       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;

Likewise.

> +}

>
>  static const struct clk_ops rzg2l_cpg_sd_clk_mux_ops = {
>         .determine_rate = __clk_mux_determine_rate_closest,
> -       .set_parent     = rzg2l_cpg_sd_clk_mux_set_parent,
> -       .get_parent     = rzg2l_cpg_sd_clk_mux_get_parent,
> +       .set_parent     = rzg2l_cpg_sd_mux_clk_set_parent,
> +       .get_parent     = rzg2l_cpg_sd_mux_clk_get_parent,

Please keep the old names, for consistency with
__clk_mux_determine_rate_closest() and drivers/clk/clk-mux.c, and to
reduce the diff.

Any existing inconsistent use of "clk_mux" vs. "mux_clk" can be resolved
later with a separate patch, if anyone cares.

> --- a/drivers/clk/renesas/rzg2l-cpg.h
> +++ b/drivers/clk/renesas/rzg2l-cpg.h

> @@ -272,4 +276,6 @@ 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);

rzg2l_cpg_sd_clk_mux_notifier()?

> +
>  #endif

The rest LGTM.

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