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

[claudiu beznea <claudiu.beznea@tuxon.dev>]

Hi, Geert,

On 04.10.2023 14:30, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> On Fri, Sep 29, 2023 at 7:39 AM Claudiu <claudiu.beznea@tuxon.dev> wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> 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@bp.renesas.com>
>> ---
>>
>> 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.

Indeed I missed these.

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

ok

> 
>> --- 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()?

ok

> 
>> +
>>  #endif
> 
> The rest LGTM.
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> 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