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

Hi, Geert,

On 04.10.2023 14:30, Geert Uytterhoeven wrote:

Hi Claudiu,

On Fri, Sep 29, 2023 at 7:39 AM Claudiu [off-list ref] wrote:

quoted

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!

quoted

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

quoted

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

quoted

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

quoted

+}

quoted

 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

quoted

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

quoted

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

quoted

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel