Hi Claudiu,

On Fri, Sep 15, 2023 at 9:30 AM claudiu beznea [off-list ref] wrote:

On 14.09.2023 18:18, Geert Uytterhoeven wrote:

quoted

On Tue, Sep 12, 2023 at 6:52 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 indexes is also

indices are

quoted

passed from platform specific initialization code.

Please also mention the passing of the mux flags, which is added so
you can pass CLK_SET_PARENT_GATE for G3S_SEL_PLL4 later.

Ok.

quoted

quoted

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

quoted

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

@@ -21,6 +21,10 @@
 #define G2UL_SEL_SDHI0         SEL_PLL_PACK(G2UL_CPG_PL2SDHI_DSEL, 0, 2)
 #define G2UL_SEL_SDHI1         SEL_PLL_PACK(G2UL_CPG_PL2SDHI_DSEL, 4, 2)

+/* Clock status configuration. */
+#define G2UL_SEL_SDHI0_STS     SEL_PLL_PACK(CPG_CLKSTATUS, 28, 1)
+#define G2UL_SEL_SDHI1_STS     SEL_PLL_PACK(CPG_CLKSTATUS, 29, 1)

Just like in [PATCH 17/37], there is no real need for the "G2UL_"-prefix.

Ok, I ususlly tend to guard everything with a proper namespace.

Sure, in many cases, that makes good sense.
In this case, not having the prefix makes it easier to compare clock tables:

    soc-dts-diff -b drivers/clk/renesas/r9a07g04[34]-cpg.c

(soc-dts-diff ignores the SoC part number, and can be found at
 https://github.com/geertu/linux-scripts)

quoted

quoted

+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;
+
+       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;
+}
+
+static int rzg2l_register_notifier(struct clk_hw *hw, const struct cpg_core_clk *core,
+                                  struct rzg2l_cpg_priv *priv)
+{
+       struct notifier_block *nb;
+
+       if (!core->notifier)
+               return 0;
+
+       nb = devm_kzalloc(priv->dev, sizeof(*nb), GFP_KERNEL);
+       if (!nb)
+               return -ENOMEM;
+
+       nb->notifier_call = core->notifier;
+
+       return clk_notifier_register(hw->clk, nb);
+}

I am not sure a notifier is the best solution.  Basically on RZ/G2L,
when changing the parent clock, you need to switch to a fixed
intermediate parent first.
What about just replacing the fixed clk_src_266 in the old
rzg2l_cpg_sd_mux_clk_set_parent() by a (signed) integer in
sd_mux_hw_data (specified in DEF_SD_MUX()), representing the index
of the intermediate clock?
-1 would mean an intermediate parent is not needed.

That should work too but .set_rate() will be bulky for both mux and div.

The idea was to have the .set_rate() common to the mux and the platform
specificities implemented as notifiers and only the needed platforms to
instantiate the notifier. And the same approach to be used by the divider
(patch "[PATCH 19/37] clk: renesas: rzg2l: add a divider clock for RZ/G3S")

With this it looked to me that the final code is more compact .set_rate
being simple and platform specificities being implemented in notifier
(valid for both MUX and DIV). The infrastructure is already there for
notifier to be called before .set_rate().

TBH, I am not that familiar with clock notifiers, so I could use some
guidance from the clock maintainers.

Mike/Stephen: Are clock notifiers the right approach, here and in
              [PATCH 19.37]?

quoted

quoted

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

@@ -272,4 +278,9 @@ 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);
+
+/* Macros to be used in platform specific initialization code. */
+#define SD_MUX_NOTIF           (&rzg2l_cpg_sd_mux_clk_notifier)

Any specific reason you are adding this macro?

It looked to me like a better name to be used in platform specific drivers.

quoted

What is wrong with using &rzg2l_cpg_sd_mux_clk_notifier directly?

Nothing, just that it is a longer than SD_MUX_NOTIF.

It adds another level of indirection for the casual reviewer, and needs
replacement when an SoC arrives that needs a different SD mux notifier.

Thanks!

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