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