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

claudiu beznea
2023-10-05T04:25:13Z

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