[PATCH v2 0/8] clk: renesas: rzg2l-cpg: Drop PM domain abstraction for MSTOP

[PATCH v2 0/8] clk: renesas: rzg2l-cpg: Drop PM domain abstraction for MSTOP

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Hi,

Series drops the PM domain abstraction for MSTOP to comply with the
requirements received from the hardware team regarding the configuration
sequence b/w the MSTOP and CLKON bits of individual modules.

The initial MSTOP support for RZ/G3S was proposed here:
https://lore.kernel.org/all/20231120070024.4079344-4-claudiu.beznea.uj@bp.renesas.com/

There are no DT users of this abstraction yet.

Please share your thoughts.

Thank you,
Claudiu Beznea

Changes in v2:
- updated the title and description for patches 1/8, 2/8 along
  with their content
- added patch 3/8
- collected tags
- drop duplicated mstop lists in patch 4/8
- detailed changelog for each patch can be found in the individual
  patch

Claudiu Beznea (8):
  clk: renesas: rzg2l-cpg: Postone updating priv->clks[]
  clk: renesas: rzg2l-cpg: Move pointers after hw member
  clk: renesas: rzg2l-cpg: Add macro to loop through module clocks
  clk: renesas: rzg2l-cpg: Add support for MSTOP in clock enable/disable
    API
  clk: renesas: r9a08g045: Drop power domain instantiation
  clk: renesas: rzg2l-cpg: Drop MSTOP based power domain support
  dt-bindings: clock: rzg2l-cpg: Drop power domain IDs
  Revert "dt-bindings: clock: renesas,rzg2l-cpg: Update
    #power-domain-cells = <1> for RZ/G3S"

 .../bindings/clock/renesas,rzg2l-cpg.yaml     |  18 +-
 drivers/clk/renesas/r9a07g043-cpg.c           | 132 ++---
 drivers/clk/renesas/r9a07g044-cpg.c           | 168 +++---
 drivers/clk/renesas/r9a08g045-cpg.c           | 227 ++++----
 drivers/clk/renesas/r9a09g011-cpg.c           | 116 ++---
 drivers/clk/renesas/rzg2l-cpg.c               | 493 ++++++++++--------
 drivers/clk/renesas/rzg2l-cpg.h               |  66 +--
 include/dt-bindings/clock/r9a07g043-cpg.h     |  53 --
 include/dt-bindings/clock/r9a07g044-cpg.h     |  58 ---
 include/dt-bindings/clock/r9a07g054-cpg.h     |  58 ---
 include/dt-bindings/clock/r9a08g045-cpg.h     |  71 ---
 11 files changed, 603 insertions(+), 857 deletions(-)

-- 
2.43.0

[PATCH v2 1/8] clk: renesas: rzg2l-cpg: Postone updating priv->clks[]

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Since the sibling data is filled after the priv->clks[] array entry is
populated, the first clock that is probed and has a sibling will
temporarily behave as its own sibling until its actual sibling is
populated. To avoid any issues, postpone updating priv->clks[] until after
the sibling is populated.

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

Changes in v2:
- postpone the update of priv->clks[id] as suggested in review process
- updated the patch title and description to reflect the new approach

 drivers/clk/renesas/rzg2l-cpg.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/renesas/rzg2l-cpg.c b/drivers/clk/renesas/rzg2l-cpg.c
index a8628f64a03b..c87ad5a972b7 100644
--- a/drivers/clk/renesas/rzg2l-cpg.c
+++ b/drivers/clk/renesas/rzg2l-cpg.c
@@ -1389,10 +1389,6 @@ rzg2l_cpg_register_mod_clk(const struct rzg2l_mod_clk *mod,
 		goto fail;
 	}
 
-	clk = clock->hw.clk;
-	dev_dbg(dev, "Module clock %pC at %lu Hz\n", clk, clk_get_rate(clk));
-	priv->clks[id] = clk;
-
 	if (mod->is_coupled) {
 		struct mstp_clock *sibling;
 
@@ -1404,6 +1400,10 @@ rzg2l_cpg_register_mod_clk(const struct rzg2l_mod_clk *mod,
 		}
 	}
 
+	clk = clock->hw.clk;
+	dev_dbg(dev, "Module clock %pC at %lu Hz\n", clk, clk_get_rate(clk));
+	priv->clks[id] = clk;
+
 	return;
 
 fail:
-- 
2.43.0

Re: [PATCH v2 1/8] clk: renesas: rzg2l-cpg: Postone updating priv->clks[]

[Geert Uytterhoeven]

On Wed, 14 May 2025 at 11:04, Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> Since the sibling data is filled after the priv->clks[] array entry is
> populated, the first clock that is probed and has a sibling will
> temporarily behave as its own sibling until its actual sibling is
> populated. To avoid any issues, postpone updating priv->clks[] until after
> the sibling is populated.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
>
> Changes in v2:
> - postpone the update of priv->clks[id] as suggested in review process
> - updated the patch title and description to reflect the new approach

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
i.e. will queue in renesas-clk for v6.17.

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

[PATCH v2 2/8] clk: renesas: rzg2l-cpg: Move pointers after hw member

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Reorder the pointer members in struct mstp_clock so they appear immediately
after the hw member. This helps avoid potential padding and eliminates the
need for any calculations in the to_mod_clock() macro. As struct clk_hw
currently contains only pointers, placing it first also avoids padding.

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v2:
- moved pointers after hw member
- updated the patch title and description to reflect the new approach
- collected tags

 drivers/clk/renesas/rzg2l-cpg.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/renesas/rzg2l-cpg.c b/drivers/clk/renesas/rzg2l-cpg.c
index c87ad5a972b7..767da288b0f7 100644
--- a/drivers/clk/renesas/rzg2l-cpg.c
+++ b/drivers/clk/renesas/rzg2l-cpg.c
@@ -1185,19 +1185,19 @@ rzg2l_cpg_register_core_clk(const struct cpg_core_clk *core,
  * struct mstp_clock - MSTP gating clock
  *
  * @hw: handle between common and hardware-specific interfaces
+ * @priv: CPG/MSTP private data
+ * @sibling: pointer to the other coupled clock
  * @off: register offset
  * @bit: ON/MON bit
  * @enabled: soft state of the clock, if it is coupled with another clock
- * @priv: CPG/MSTP private data
- * @sibling: pointer to the other coupled clock
  */
 struct mstp_clock {
 	struct clk_hw hw;
+	struct rzg2l_cpg_priv *priv;
+	struct mstp_clock *sibling;
 	u16 off;
 	u8 bit;
 	bool enabled;
-	struct rzg2l_cpg_priv *priv;
-	struct mstp_clock *sibling;
 };
 
 #define to_mod_clock(_hw) container_of(_hw, struct mstp_clock, hw)
-- 
2.43.0

Re: [PATCH v2 2/8] clk: renesas: rzg2l-cpg: Move pointers after hw member

[Geert Uytterhoeven]

On Wed, 14 May 2025 at 11:04, Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> Reorder the pointer members in struct mstp_clock so they appear immediately
> after the hw member. This helps avoid potential padding and eliminates the
> need for any calculations in the to_mod_clock() macro. As struct clk_hw
> currently contains only pointers, placing it first also avoids padding.
>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
>
> Changes in v2:
> - moved pointers after hw member
> - updated the patch title and description to reflect the new approach
> - collected tags

Thanks, will queue in renesas-clk for v6.17.

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

[PATCH v2 3/8] clk: renesas: rzg2l-cpg: Add macro to loop through module clocks

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Add a macro to iterate over the module clocks array. This will be useful
in the upcoming commits that move MSTOP support into the clock
enable/disable APIs.

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

Changes in v2:
- none; this patch is new

 drivers/clk/renesas/rzg2l-cpg.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/clk/renesas/rzg2l-cpg.c b/drivers/clk/renesas/rzg2l-cpg.c
index 767da288b0f7..c619b2da92b0 100644
--- a/drivers/clk/renesas/rzg2l-cpg.c
+++ b/drivers/clk/renesas/rzg2l-cpg.c
@@ -1202,6 +1202,13 @@ struct mstp_clock {
 
 #define to_mod_clock(_hw) container_of(_hw, struct mstp_clock, hw)
 
+#define for_each_mstp_clk(mstp_clk, hw, priv) \
+	for (unsigned int i = 0; (priv) && i < (priv)->num_mod_clks; i++) \
+		if ((priv)->clks[(priv)->num_core_clks + i] == ERR_PTR(-ENOENT)) \
+			continue; \
+		else if (((hw) = __clk_get_hw((priv)->clks[(priv)->num_core_clks + i])) && \
+			 ((mstp_clk) = to_mod_clock(hw)))
+
 static int rzg2l_mod_clock_endisable(struct clk_hw *hw, bool enable)
 {
 	struct mstp_clock *clock = to_mod_clock(hw);
@@ -1314,17 +1321,10 @@ static struct mstp_clock
 *rzg2l_mod_clock_get_sibling(struct mstp_clock *clock,
 			     struct rzg2l_cpg_priv *priv)
 {
+	struct mstp_clock *clk;
 	struct clk_hw *hw;
-	unsigned int i;
-
-	for (i = 0; i < priv->num_mod_clks; i++) {
-		struct mstp_clock *clk;
-
-		if (priv->clks[priv->num_core_clks + i] == ERR_PTR(-ENOENT))
-			continue;
 
-		hw = __clk_get_hw(priv->clks[priv->num_core_clks + i]);
-		clk = to_mod_clock(hw);
+	for_each_mstp_clk(clk, hw, priv) {
 		if (clock->off == clk->off && clock->bit == clk->bit)
 			return clk;
 	}
-- 
2.43.0

Re: [PATCH v2 3/8] clk: renesas: rzg2l-cpg: Add macro to loop through module clocks

[Geert Uytterhoeven]

Hi Claudiu,

On Wed, 14 May 2025 at 11:04, Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> Add a macro to iterate over the module clocks array. This will be useful
> in the upcoming commits that move MSTOP support into the clock
> enable/disable APIs.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Thanks for your patch!

> --- a/drivers/clk/renesas/rzg2l-cpg.c
> +++ b/drivers/clk/renesas/rzg2l-cpg.c
> @@ -1202,6 +1202,13 @@ struct mstp_clock {
>
>  #define to_mod_clock(_hw) container_of(_hw, struct mstp_clock, hw)
>
> +#define for_each_mstp_clk(mstp_clk, hw, priv) \
> +       for (unsigned int i = 0; (priv) && i < (priv)->num_mod_clks; i++) \
> +               if ((priv)->clks[(priv)->num_core_clks + i] == ERR_PTR(-ENOENT)) \
> +                       continue; \
> +               else if (((hw) = __clk_get_hw((priv)->clks[(priv)->num_core_clks + i])) && \
> +                        ((mstp_clk) = to_mod_clock(hw)))

s/mstp_clk/mstp_clock/ everywhere, to match the struct name.

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
i.e. will queue in renesas-clk for v6.17, with the above fixed.

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

[PATCH v2 4/8] clk: renesas: rzg2l-cpg: Add support for MSTOP in clock enable/disable API

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

The RZ/{G2L, V2L, G3S} CPG versions support a feature called MSTOP. Each
module has one or more MSTOP bits associated with it, and these bits need
to be configured along with the module clocks. Setting the MSTOP bits
switches the module between normal and standby states.

Previously, MSTOP support was abstracted through power domains
(struct generic_pm_domain::{power_on, power_off} APIs). With this
abstraction, the order of setting the MSTOP and CLKON bits was as follows:

Previous Order:
A/ Switching to Normal State (e.g., during probe):
1/ Clear module MSTOP bit
2/ Set module CLKON bit

B/ Switching to Standby State (e.g., during remove):
1/ Clear CLKON bit
2/ Set MSTOP bit

However, in some cases (when the clock is disabled through devres), the
order may have been (due to the issue described in link section):

1/ Set MSTOP bit
2/ Clear CLKON bit

Recently, the hardware team has suggested that the correct order to set
the MSTOP and CLKON bits is:

Updated Order:
A/ Switching to Normal State (e.g., during probe):
1/ Set CLKON bit
2/ Clear MSTOP bit

B/ Switching to Standby State (e.g., during remove):
1/ Set MSTOP bit
2/ Clear CLKON bit

To prevent future issues due to incorrect ordering, the MSTOP setup has
now been implemented in rzg2l_mod_clock_endisable(), ensuring compliance
with the sequence suggested in Figure 41.5: Module Standby Mode Procedure
from the RZ/G3S HW manual, Rev1.10.

Additionally, since multiple clocks of a single module may be mapped to a
single MSTOP bit, MSTOP setup is reference-counted.

Furthermore, as all modules start in the normal state after reset, if the
module clocks are disabled, the module state is switched to standby. This
prevents keeping the module in an invalid state, as recommended by the
hardware team.

Link: https://lore.kernel.org/all/20250215130849.227812-1-claudiu.beznea.uj@bp.renesas.com/
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v2:
- udpated patch description to avoid plural in the configuration
  sequence description b/w MSTOP and CLK_ON
- use atomic type to store the usage counter; s/refcnt/usecnt/g
- moved MSTOP_OFF(), MSTOP_MASK() macros to rzg2l-cpg.c
- dropped struct mstp_clock::critical and use clk_hw_get_flags()
  instead to get the clock flags
- used unsigned int iterators in for loops
- keep memory allocated for a single list for clocks sharing the
  same MSTOP by updating the rzg2l_mod_clock_add_shared_mstop_clk();
- s/rzg2l_cpg_mstop_show/rzg2l_mod_clock_mstop_show/g,
  s/rzg2l_cpg_mstop/rzg2l_mod_clock_mstop/g,
  s/rzg2l_cpg_update_shared_mstop_clocks/rzg2l_mod_clock_update_shared_mstop_clks/g
   to keep the same naming conventions for functions handling mod clock MSTOP
- use the newly added for_each_mstp_clk() macro all over the code

 drivers/clk/renesas/r9a07g043-cpg.c | 132 +++++++--------
 drivers/clk/renesas/r9a07g044-cpg.c | 168 +++++++++---------
 drivers/clk/renesas/r9a08g045-cpg.c | 105 ++++++------
 drivers/clk/renesas/r9a09g011-cpg.c | 116 ++++++-------
 drivers/clk/renesas/rzg2l-cpg.c     | 253 +++++++++++++++++++++++++++-
 drivers/clk/renesas/rzg2l-cpg.h     |  15 +-
 6 files changed, 523 insertions(+), 266 deletions(-)

diff --git a/drivers/clk/renesas/r9a07g043-cpg.c b/drivers/clk/renesas/r9a07g043-cpg.c
index fce2eecfa8c0..02dc5cecfd8d 100644
--- a/drivers/clk/renesas/r9a07g043-cpg.c
+++ b/drivers/clk/renesas/r9a07g043-cpg.c
@@ -164,143 +164,143 @@ static const struct cpg_core_clk r9a07g043_core_clks[] __initconst = {
 static const struct rzg2l_mod_clk r9a07g043_mod_clks[] = {
 #ifdef CONFIG_ARM64
 	DEF_MOD("gic",		R9A07G043_GIC600_GICCLK, R9A07G043_CLK_P1,
-				0x514, 0),
+				0x514, 0, 0),
 	DEF_MOD("ia55_pclk",	R9A07G043_IA55_PCLK, R9A07G043_CLK_P2,
-				0x518, 0),
+				0x518, 0, 0),
 	DEF_MOD("ia55_clk",	R9A07G043_IA55_CLK, R9A07G043_CLK_P1,
-				0x518, 1),
+				0x518, 1, 0),
 #endif
 #ifdef CONFIG_RISCV
 	DEF_MOD("iax45_pclk",	R9A07G043_IAX45_PCLK, R9A07G043_CLK_P2,
-				0x518, 0),
+				0x518, 0, 0),
 	DEF_MOD("iax45_clk",	R9A07G043_IAX45_CLK, R9A07G043_CLK_P1,
-				0x518, 1),
+				0x518, 1, 0),
 #endif
 	DEF_MOD("dmac_aclk",	R9A07G043_DMAC_ACLK, R9A07G043_CLK_P1,
-				0x52c, 0),
+				0x52c, 0, 0),
 	DEF_MOD("dmac_pclk",	R9A07G043_DMAC_PCLK, CLK_P1_DIV2,
-				0x52c, 1),
+				0x52c, 1, 0),
 	DEF_MOD("ostm0_pclk",	R9A07G043_OSTM0_PCLK, R9A07G043_CLK_P0,
-				0x534, 0),
+				0x534, 0, 0),
 	DEF_MOD("ostm1_pclk",	R9A07G043_OSTM1_PCLK, R9A07G043_CLK_P0,
-				0x534, 1),
+				0x534, 1, 0),
 	DEF_MOD("ostm2_pclk",	R9A07G043_OSTM2_PCLK, R9A07G043_CLK_P0,
-				0x534, 2),
+				0x534, 2, 0),
 	DEF_MOD("mtu_x_mck",	R9A07G043_MTU_X_MCK_MTU3, R9A07G043_CLK_P0,
-				0x538, 0),
+				0x538, 0, 0),
 	DEF_MOD("wdt0_pclk",	R9A07G043_WDT0_PCLK, R9A07G043_CLK_P0,
-				0x548, 0),
+				0x548, 0, 0),
 	DEF_MOD("wdt0_clk",	R9A07G043_WDT0_CLK, R9A07G043_OSCCLK,
-				0x548, 1),
+				0x548, 1, 0),
 	DEF_MOD("spi_clk2",	R9A07G043_SPI_CLK2, R9A07G043_CLK_SPI1,
-				0x550, 0),
+				0x550, 0, 0),
 	DEF_MOD("spi_clk",	R9A07G043_SPI_CLK, R9A07G043_CLK_SPI0,
-				0x550, 1),
+				0x550, 1, 0),
 	DEF_MOD("sdhi0_imclk",	R9A07G043_SDHI0_IMCLK, CLK_SD0_DIV4,
-				0x554, 0),
+				0x554, 0, 0),
 	DEF_MOD("sdhi0_imclk2",	R9A07G043_SDHI0_IMCLK2, CLK_SD0_DIV4,
-				0x554, 1),
+				0x554, 1, 0),
 	DEF_MOD("sdhi0_clk_hs",	R9A07G043_SDHI0_CLK_HS, R9A07G043_CLK_SD0,
-				0x554, 2),
+				0x554, 2, 0),
 	DEF_MOD("sdhi0_aclk",	R9A07G043_SDHI0_ACLK, R9A07G043_CLK_P1,
-				0x554, 3),
+				0x554, 3, 0),
 	DEF_MOD("sdhi1_imclk",	R9A07G043_SDHI1_IMCLK, CLK_SD1_DIV4,
-				0x554, 4),
+				0x554, 4, 0),
 	DEF_MOD("sdhi1_imclk2",	R9A07G043_SDHI1_IMCLK2, CLK_SD1_DIV4,
-				0x554, 5),
+				0x554, 5, 0),
 	DEF_MOD("sdhi1_clk_hs",	R9A07G043_SDHI1_CLK_HS, R9A07G043_CLK_SD1,
-				0x554, 6),
+				0x554, 6, 0),
 	DEF_MOD("sdhi1_aclk",	R9A07G043_SDHI1_ACLK, R9A07G043_CLK_P1,
-				0x554, 7),
+				0x554, 7, 0),
 #ifdef CONFIG_ARM64
 	DEF_MOD("cru_sysclk",   R9A07G043_CRU_SYSCLK, CLK_M2_DIV2,
-				0x564, 0),
+				0x564, 0, 0),
 	DEF_MOD("cru_vclk",     R9A07G043_CRU_VCLK, R9A07G043_CLK_M2,
-				0x564, 1),
+				0x564, 1, 0),
 	DEF_MOD("cru_pclk",     R9A07G043_CRU_PCLK, R9A07G043_CLK_ZT,
-				0x564, 2),
+				0x564, 2, 0),
 	DEF_MOD("cru_aclk",     R9A07G043_CRU_ACLK, R9A07G043_CLK_M0,
-				0x564, 3),
+				0x564, 3, 0),
 	DEF_COUPLED("lcdc_clk_a", R9A07G043_LCDC_CLK_A, R9A07G043_CLK_M0,
-				0x56c, 0),
+				0x56c, 0, 0),
 	DEF_COUPLED("lcdc_clk_p", R9A07G043_LCDC_CLK_P, R9A07G043_CLK_ZT,
-				0x56c, 0),
+				0x56c, 0, 0),
 	DEF_MOD("lcdc_clk_d",	R9A07G043_LCDC_CLK_D, R9A07G043_CLK_M3,
-				0x56c, 1),
+				0x56c, 1, 0),
 #endif
 	DEF_MOD("ssi0_pclk",	R9A07G043_SSI0_PCLK2, R9A07G043_CLK_P0,
-				0x570, 0),
+				0x570, 0, 0),
 	DEF_MOD("ssi0_sfr",	R9A07G043_SSI0_PCLK_SFR, R9A07G043_CLK_P0,
-				0x570, 1),
+				0x570, 1, 0),
 	DEF_MOD("ssi1_pclk",	R9A07G043_SSI1_PCLK2, R9A07G043_CLK_P0,
-				0x570, 2),
+				0x570, 2, 0),
 	DEF_MOD("ssi1_sfr",	R9A07G043_SSI1_PCLK_SFR, R9A07G043_CLK_P0,
-				0x570, 3),
+				0x570, 3, 0),
 	DEF_MOD("ssi2_pclk",	R9A07G043_SSI2_PCLK2, R9A07G043_CLK_P0,
-				0x570, 4),
+				0x570, 4, 0),
 	DEF_MOD("ssi2_sfr",	R9A07G043_SSI2_PCLK_SFR, R9A07G043_CLK_P0,
-				0x570, 5),
+				0x570, 5, 0),
 	DEF_MOD("ssi3_pclk",	R9A07G043_SSI3_PCLK2, R9A07G043_CLK_P0,
-				0x570, 6),
+				0x570, 6, 0),
 	DEF_MOD("ssi3_sfr",	R9A07G043_SSI3_PCLK_SFR, R9A07G043_CLK_P0,
-				0x570, 7),
+				0x570, 7, 0),
 	DEF_MOD("usb0_host",	R9A07G043_USB_U2H0_HCLK, R9A07G043_CLK_P1,
-				0x578, 0),
+				0x578, 0, 0),
 	DEF_MOD("usb1_host",	R9A07G043_USB_U2H1_HCLK, R9A07G043_CLK_P1,
-				0x578, 1),
+				0x578, 1, 0),
 	DEF_MOD("usb0_func",	R9A07G043_USB_U2P_EXR_CPUCLK, R9A07G043_CLK_P1,
-				0x578, 2),
+				0x578, 2, 0),
 	DEF_MOD("usb_pclk",	R9A07G043_USB_PCLK, R9A07G043_CLK_P1,
-				0x578, 3),
+				0x578, 3, 0),
 	DEF_COUPLED("eth0_axi",	R9A07G043_ETH0_CLK_AXI, R9A07G043_CLK_M0,
-				0x57c, 0),
+				0x57c, 0, 0),
 	DEF_COUPLED("eth0_chi",	R9A07G043_ETH0_CLK_CHI, R9A07G043_CLK_ZT,
-				0x57c, 0),
+				0x57c, 0, 0),
 	DEF_COUPLED("eth1_axi",	R9A07G043_ETH1_CLK_AXI, R9A07G043_CLK_M0,
-				0x57c, 1),
+				0x57c, 1, 0),
 	DEF_COUPLED("eth1_chi",	R9A07G043_ETH1_CLK_CHI, R9A07G043_CLK_ZT,
-				0x57c, 1),
+				0x57c, 1, 0),
 	DEF_MOD("i2c0",		R9A07G043_I2C0_PCLK, R9A07G043_CLK_P0,
-				0x580, 0),
+				0x580, 0, 0),
 	DEF_MOD("i2c1",		R9A07G043_I2C1_PCLK, R9A07G043_CLK_P0,
-				0x580, 1),
+				0x580, 1, 0),
 	DEF_MOD("i2c2",		R9A07G043_I2C2_PCLK, R9A07G043_CLK_P0,
-				0x580, 2),
+				0x580, 2, 0),
 	DEF_MOD("i2c3",		R9A07G043_I2C3_PCLK, R9A07G043_CLK_P0,
-				0x580, 3),
+				0x580, 3, 0),
 	DEF_MOD("scif0",	R9A07G043_SCIF0_CLK_PCK, R9A07G043_CLK_P0,
-				0x584, 0),
+				0x584, 0, 0),
 	DEF_MOD("scif1",	R9A07G043_SCIF1_CLK_PCK, R9A07G043_CLK_P0,
-				0x584, 1),
+				0x584, 1, 0),
 	DEF_MOD("scif2",	R9A07G043_SCIF2_CLK_PCK, R9A07G043_CLK_P0,
-				0x584, 2),
+				0x584, 2, 0),
 	DEF_MOD("scif3",	R9A07G043_SCIF3_CLK_PCK, R9A07G043_CLK_P0,
-				0x584, 3),
+				0x584, 3, 0),
 	DEF_MOD("scif4",	R9A07G043_SCIF4_CLK_PCK, R9A07G043_CLK_P0,
-				0x584, 4),
+				0x584, 4, 0),
 	DEF_MOD("sci0",		R9A07G043_SCI0_CLKP, R9A07G043_CLK_P0,
-				0x588, 0),
+				0x588, 0, 0),
 	DEF_MOD("sci1",		R9A07G043_SCI1_CLKP, R9A07G043_CLK_P0,
-				0x588, 1),
+				0x588, 1, 0),
 	DEF_MOD("rspi0",	R9A07G043_RSPI0_CLKB, R9A07G043_CLK_P0,
-				0x590, 0),
+				0x590, 0, 0),
 	DEF_MOD("rspi1",	R9A07G043_RSPI1_CLKB, R9A07G043_CLK_P0,
-				0x590, 1),
+				0x590, 1, 0),
 	DEF_MOD("rspi2",	R9A07G043_RSPI2_CLKB, R9A07G043_CLK_P0,
-				0x590, 2),
+				0x590, 2, 0),
 	DEF_MOD("canfd",	R9A07G043_CANFD_PCLK, R9A07G043_CLK_P0,
-				0x594, 0),
+				0x594, 0, 0),
 	DEF_MOD("gpio",		R9A07G043_GPIO_HCLK, R9A07G043_OSCCLK,
-				0x598, 0),
+				0x598, 0, 0),
 	DEF_MOD("adc_adclk",	R9A07G043_ADC_ADCLK, R9A07G043_CLK_TSU,
-				0x5a8, 0),
+				0x5a8, 0, 0),
 	DEF_MOD("adc_pclk",	R9A07G043_ADC_PCLK, R9A07G043_CLK_P0,
-				0x5a8, 1),
+				0x5a8, 1, 0),
 	DEF_MOD("tsu_pclk",	R9A07G043_TSU_PCLK, R9A07G043_CLK_TSU,
-				0x5ac, 0),
+				0x5ac, 0, 0),
 #ifdef CONFIG_RISCV
 	DEF_MOD("nceplic_aclk",	R9A07G043_NCEPLIC_ACLK, R9A07G043_CLK_P1,
-				0x608, 0),
+				0x608, 0, 0),
 #endif
 };
 
diff --git a/drivers/clk/renesas/r9a07g044-cpg.c b/drivers/clk/renesas/r9a07g044-cpg.c
index 77ca3a789568..c851d4eeebbe 100644
--- a/drivers/clk/renesas/r9a07g044-cpg.c
+++ b/drivers/clk/renesas/r9a07g044-cpg.c
@@ -242,176 +242,176 @@ static const struct {
 } mod_clks = {
 	.common = {
 		DEF_MOD("gic",		R9A07G044_GIC600_GICCLK, R9A07G044_CLK_P1,
-					0x514, 0),
+					0x514, 0, 0),
 		DEF_MOD("ia55_pclk",	R9A07G044_IA55_PCLK, R9A07G044_CLK_P2,
-					0x518, 0),
+					0x518, 0, 0),
 		DEF_MOD("ia55_clk",	R9A07G044_IA55_CLK, R9A07G044_CLK_P1,
-					0x518, 1),
+					0x518, 1, 0),
 		DEF_MOD("dmac_aclk",	R9A07G044_DMAC_ACLK, R9A07G044_CLK_P1,
-					0x52c, 0),
+					0x52c, 0, 0),
 		DEF_MOD("dmac_pclk",	R9A07G044_DMAC_PCLK, CLK_P1_DIV2,
-					0x52c, 1),
+					0x52c, 1, 0),
 		DEF_MOD("ostm0_pclk",	R9A07G044_OSTM0_PCLK, R9A07G044_CLK_P0,
-					0x534, 0),
+					0x534, 0, 0),
 		DEF_MOD("ostm1_pclk",	R9A07G044_OSTM1_PCLK, R9A07G044_CLK_P0,
-					0x534, 1),
+					0x534, 1, 0),
 		DEF_MOD("ostm2_pclk",	R9A07G044_OSTM2_PCLK, R9A07G044_CLK_P0,
-					0x534, 2),
+					0x534, 2, 0),
 		DEF_MOD("mtu_x_mck",	R9A07G044_MTU_X_MCK_MTU3, R9A07G044_CLK_P0,
-					0x538, 0),
+					0x538, 0, 0),
 		DEF_MOD("gpt_pclk",	R9A07G044_GPT_PCLK, R9A07G044_CLK_P0,
-					0x540, 0),
+					0x540, 0, 0),
 		DEF_MOD("poeg_a_clkp",	R9A07G044_POEG_A_CLKP, R9A07G044_CLK_P0,
-					0x544, 0),
+					0x544, 0, 0),
 		DEF_MOD("poeg_b_clkp",	R9A07G044_POEG_B_CLKP, R9A07G044_CLK_P0,
-					0x544, 1),
+					0x544, 1, 0),
 		DEF_MOD("poeg_c_clkp",	R9A07G044_POEG_C_CLKP, R9A07G044_CLK_P0,
-					0x544, 2),
+					0x544, 2, 0),
 		DEF_MOD("poeg_d_clkp",	R9A07G044_POEG_D_CLKP, R9A07G044_CLK_P0,
-					0x544, 3),
+					0x544, 3, 0),
 		DEF_MOD("wdt0_pclk",	R9A07G044_WDT0_PCLK, R9A07G044_CLK_P0,
-					0x548, 0),
+					0x548, 0, 0),
 		DEF_MOD("wdt0_clk",	R9A07G044_WDT0_CLK, R9A07G044_OSCCLK,
-					0x548, 1),
+					0x548, 1, 0),
 		DEF_MOD("wdt1_pclk",	R9A07G044_WDT1_PCLK, R9A07G044_CLK_P0,
-					0x548, 2),
+					0x548, 2, 0),
 		DEF_MOD("wdt1_clk",	R9A07G044_WDT1_CLK, R9A07G044_OSCCLK,
-					0x548, 3),
+					0x548, 3, 0),
 		DEF_MOD("spi_clk2",	R9A07G044_SPI_CLK2, R9A07G044_CLK_SPI1,
-					0x550, 0),
+					0x550, 0, 0),
 		DEF_MOD("spi_clk",	R9A07G044_SPI_CLK, R9A07G044_CLK_SPI0,
-					0x550, 1),
+					0x550, 1, 0),
 		DEF_MOD("sdhi0_imclk",	R9A07G044_SDHI0_IMCLK, CLK_SD0_DIV4,
-					0x554, 0),
+					0x554, 0, 0),
 		DEF_MOD("sdhi0_imclk2",	R9A07G044_SDHI0_IMCLK2, CLK_SD0_DIV4,
-					0x554, 1),
+					0x554, 1, 0),
 		DEF_MOD("sdhi0_clk_hs",	R9A07G044_SDHI0_CLK_HS, R9A07G044_CLK_SD0,
-					0x554, 2),
+					0x554, 2, 0),
 		DEF_MOD("sdhi0_aclk",	R9A07G044_SDHI0_ACLK, R9A07G044_CLK_P1,
-					0x554, 3),
+					0x554, 3, 0),
 		DEF_MOD("sdhi1_imclk",	R9A07G044_SDHI1_IMCLK, CLK_SD1_DIV4,
-					0x554, 4),
+					0x554, 4, 0),
 		DEF_MOD("sdhi1_imclk2",	R9A07G044_SDHI1_IMCLK2, CLK_SD1_DIV4,
-					0x554, 5),
+					0x554, 5, 0),
 		DEF_MOD("sdhi1_clk_hs",	R9A07G044_SDHI1_CLK_HS, R9A07G044_CLK_SD1,
-					0x554, 6),
+					0x554, 6, 0),
 		DEF_MOD("sdhi1_aclk",	R9A07G044_SDHI1_ACLK, R9A07G044_CLK_P1,
-					0x554, 7),
+					0x554, 7, 0),
 		DEF_MOD("gpu_clk",	R9A07G044_GPU_CLK, R9A07G044_CLK_G,
-					0x558, 0),
+					0x558, 0, 0),
 		DEF_MOD("gpu_axi_clk",	R9A07G044_GPU_AXI_CLK, R9A07G044_CLK_P1,
-					0x558, 1),
+					0x558, 1, 0),
 		DEF_MOD("gpu_ace_clk",	R9A07G044_GPU_ACE_CLK, R9A07G044_CLK_P1,
-					0x558, 2),
+					0x558, 2, 0),
 		DEF_MOD("cru_sysclk",   R9A07G044_CRU_SYSCLK, CLK_M2_DIV2,
-					0x564, 0),
+					0x564, 0, 0),
 		DEF_MOD("cru_vclk",     R9A07G044_CRU_VCLK, R9A07G044_CLK_M2,
-					0x564, 1),
+					0x564, 1, 0),
 		DEF_MOD("cru_pclk",     R9A07G044_CRU_PCLK, R9A07G044_CLK_ZT,
-					0x564, 2),
+					0x564, 2, 0),
 		DEF_MOD("cru_aclk",     R9A07G044_CRU_ACLK, R9A07G044_CLK_M0,
-					0x564, 3),
+					0x564, 3, 0),
 		DEF_MOD("dsi_pll_clk",	R9A07G044_MIPI_DSI_PLLCLK, R9A07G044_CLK_M1,
-					0x568, 0),
+					0x568, 0, 0),
 		DEF_MOD("dsi_sys_clk",	R9A07G044_MIPI_DSI_SYSCLK, CLK_M2_DIV2,
-					0x568, 1),
+					0x568, 1, 0),
 		DEF_MOD("dsi_aclk",	R9A07G044_MIPI_DSI_ACLK, R9A07G044_CLK_P1,
-					0x568, 2),
+					0x568, 2, 0),
 		DEF_MOD("dsi_pclk",	R9A07G044_MIPI_DSI_PCLK, R9A07G044_CLK_P2,
-					0x568, 3),
+					0x568, 3, 0),
 		DEF_MOD("dsi_vclk",	R9A07G044_MIPI_DSI_VCLK, R9A07G044_CLK_M3,
-					0x568, 4),
+					0x568, 4, 0),
 		DEF_MOD("dsi_lpclk",	R9A07G044_MIPI_DSI_LPCLK, R9A07G044_CLK_M4,
-					0x568, 5),
+					0x568, 5, 0),
 		DEF_COUPLED("lcdc_a",	R9A07G044_LCDC_CLK_A, R9A07G044_CLK_M0,
-					0x56c, 0),
+					0x56c, 0, 0),
 		DEF_COUPLED("lcdc_p",	R9A07G044_LCDC_CLK_P, R9A07G044_CLK_ZT,
-					0x56c, 0),
+					0x56c, 0, 0),
 		DEF_MOD("lcdc_clk_d",	R9A07G044_LCDC_CLK_D, R9A07G044_CLK_M3,
-					0x56c, 1),
+					0x56c, 1, 0),
 		DEF_MOD("ssi0_pclk",	R9A07G044_SSI0_PCLK2, R9A07G044_CLK_P0,
-					0x570, 0),
+					0x570, 0, 0),
 		DEF_MOD("ssi0_sfr",	R9A07G044_SSI0_PCLK_SFR, R9A07G044_CLK_P0,
-					0x570, 1),
+					0x570, 1, 0),
 		DEF_MOD("ssi1_pclk",	R9A07G044_SSI1_PCLK2, R9A07G044_CLK_P0,
-					0x570, 2),
+					0x570, 2, 0),
 		DEF_MOD("ssi1_sfr",	R9A07G044_SSI1_PCLK_SFR, R9A07G044_CLK_P0,
-					0x570, 3),
+					0x570, 3, 0),
 		DEF_MOD("ssi2_pclk",	R9A07G044_SSI2_PCLK2, R9A07G044_CLK_P0,
-					0x570, 4),
+					0x570, 4, 0),
 		DEF_MOD("ssi2_sfr",	R9A07G044_SSI2_PCLK_SFR, R9A07G044_CLK_P0,
-					0x570, 5),
+					0x570, 5, 0),
 		DEF_MOD("ssi3_pclk",	R9A07G044_SSI3_PCLK2, R9A07G044_CLK_P0,
-					0x570, 6),
+					0x570, 6, 0),
 		DEF_MOD("ssi3_sfr",	R9A07G044_SSI3_PCLK_SFR, R9A07G044_CLK_P0,
-					0x570, 7),
+					0x570, 7, 0),
 		DEF_MOD("usb0_host",	R9A07G044_USB_U2H0_HCLK, R9A07G044_CLK_P1,
-					0x578, 0),
+					0x578, 0, 0),
 		DEF_MOD("usb1_host",	R9A07G044_USB_U2H1_HCLK, R9A07G044_CLK_P1,
-					0x578, 1),
+					0x578, 1, 0),
 		DEF_MOD("usb0_func",	R9A07G044_USB_U2P_EXR_CPUCLK, R9A07G044_CLK_P1,
-					0x578, 2),
+					0x578, 2, 0),
 		DEF_MOD("usb_pclk",	R9A07G044_USB_PCLK, R9A07G044_CLK_P1,
-					0x578, 3),
+					0x578, 3, 0),
 		DEF_COUPLED("eth0_axi",	R9A07G044_ETH0_CLK_AXI, R9A07G044_CLK_M0,
-					0x57c, 0),
+					0x57c, 0, 0),
 		DEF_COUPLED("eth0_chi",	R9A07G044_ETH0_CLK_CHI, R9A07G044_CLK_ZT,
-					0x57c, 0),
+					0x57c, 0, 0),
 		DEF_COUPLED("eth1_axi",	R9A07G044_ETH1_CLK_AXI, R9A07G044_CLK_M0,
-					0x57c, 1),
+					0x57c, 1, 0),
 		DEF_COUPLED("eth1_chi",	R9A07G044_ETH1_CLK_CHI, R9A07G044_CLK_ZT,
-					0x57c, 1),
+					0x57c, 1, 0),
 		DEF_MOD("i2c0",		R9A07G044_I2C0_PCLK, R9A07G044_CLK_P0,
-					0x580, 0),
+					0x580, 0, 0),
 		DEF_MOD("i2c1",		R9A07G044_I2C1_PCLK, R9A07G044_CLK_P0,
-					0x580, 1),
+					0x580, 1, 0),
 		DEF_MOD("i2c2",		R9A07G044_I2C2_PCLK, R9A07G044_CLK_P0,
-					0x580, 2),
+					0x580, 2, 0),
 		DEF_MOD("i2c3",		R9A07G044_I2C3_PCLK, R9A07G044_CLK_P0,
-					0x580, 3),
+					0x580, 3, 0),
 		DEF_MOD("scif0",	R9A07G044_SCIF0_CLK_PCK, R9A07G044_CLK_P0,
-					0x584, 0),
+					0x584, 0, 0),
 		DEF_MOD("scif1",	R9A07G044_SCIF1_CLK_PCK, R9A07G044_CLK_P0,
-					0x584, 1),
+					0x584, 1, 0),
 		DEF_MOD("scif2",	R9A07G044_SCIF2_CLK_PCK, R9A07G044_CLK_P0,
-					0x584, 2),
+					0x584, 2, 0),
 		DEF_MOD("scif3",	R9A07G044_SCIF3_CLK_PCK, R9A07G044_CLK_P0,
-					0x584, 3),
+					0x584, 3, 0),
 		DEF_MOD("scif4",	R9A07G044_SCIF4_CLK_PCK, R9A07G044_CLK_P0,
-					0x584, 4),
+					0x584, 4, 0),
 		DEF_MOD("sci0",		R9A07G044_SCI0_CLKP, R9A07G044_CLK_P0,
-					0x588, 0),
+					0x588, 0, 0),
 		DEF_MOD("sci1",		R9A07G044_SCI1_CLKP, R9A07G044_CLK_P0,
-					0x588, 1),
+					0x588, 1, 0),
 		DEF_MOD("rspi0",	R9A07G044_RSPI0_CLKB, R9A07G044_CLK_P0,
-					0x590, 0),
+					0x590, 0, 0),
 		DEF_MOD("rspi1",	R9A07G044_RSPI1_CLKB, R9A07G044_CLK_P0,
-					0x590, 1),
+					0x590, 1, 0),
 		DEF_MOD("rspi2",	R9A07G044_RSPI2_CLKB, R9A07G044_CLK_P0,
-					0x590, 2),
+					0x590, 2, 0),
 		DEF_MOD("canfd",	R9A07G044_CANFD_PCLK, R9A07G044_CLK_P0,
-					0x594, 0),
+					0x594, 0, 0),
 		DEF_MOD("gpio",		R9A07G044_GPIO_HCLK, R9A07G044_OSCCLK,
-					0x598, 0),
+					0x598, 0, 0),
 		DEF_MOD("adc_adclk",	R9A07G044_ADC_ADCLK, R9A07G044_CLK_TSU,
-					0x5a8, 0),
+					0x5a8, 0, 0),
 		DEF_MOD("adc_pclk",	R9A07G044_ADC_PCLK, R9A07G044_CLK_P0,
-					0x5a8, 1),
+					0x5a8, 1, 0),
 		DEF_MOD("tsu_pclk",	R9A07G044_TSU_PCLK, R9A07G044_CLK_TSU,
-					0x5ac, 0),
+					0x5ac, 0, 0),
 	},
 #ifdef CONFIG_CLK_R9A07G054
 	.drp = {
 		DEF_MOD("stpai_initclk", R9A07G054_STPAI_INITCLK, R9A07G044_OSCCLK,
-					0x5e8, 0),
+					0x5e8, 0, 0),
 		DEF_MOD("stpai_aclk",	R9A07G054_STPAI_ACLK, R9A07G044_CLK_P1,
-					0x5e8, 1),
+					0x5e8, 1, 0),
 		DEF_MOD("stpai_mclk",	R9A07G054_STPAI_MCLK, R9A07G054_CLK_DRP_M,
-					0x5e8, 2),
+					0x5e8, 2, 0),
 		DEF_MOD("stpai_dclkin",	R9A07G054_STPAI_DCLKIN, R9A07G054_CLK_DRP_D,
-					0x5e8, 3),
+					0x5e8, 3, 0),
 		DEF_MOD("stpai_aclk_drp", R9A07G054_STPAI_ACLK_DRP, R9A07G054_CLK_DRP_A,
-					0x5e8, 4),
+					0x5e8, 4, 0),
 	},
 #endif
 };
diff --git a/drivers/clk/renesas/r9a08g045-cpg.c b/drivers/clk/renesas/r9a08g045-cpg.c
index 4035f3443598..ddc5d6cd572b 100644
--- a/drivers/clk/renesas/r9a08g045-cpg.c
+++ b/drivers/clk/renesas/r9a08g045-cpg.c
@@ -192,58 +192,59 @@ static const struct cpg_core_clk r9a08g045_core_clks[] __initconst = {
 };
 
 static const struct rzg2l_mod_clk r9a08g045_mod_clks[] = {
-	DEF_MOD("gic_gicclk",		R9A08G045_GIC600_GICCLK, R9A08G045_CLK_P1, 0x514, 0),
-	DEF_MOD("ia55_pclk",		R9A08G045_IA55_PCLK, R9A08G045_CLK_P2, 0x518, 0),
-	DEF_MOD("ia55_clk",		R9A08G045_IA55_CLK, R9A08G045_CLK_P1, 0x518, 1),
-	DEF_MOD("dmac_aclk",		R9A08G045_DMAC_ACLK, R9A08G045_CLK_P3, 0x52c, 0),
-	DEF_MOD("dmac_pclk",		R9A08G045_DMAC_PCLK, CLK_P3_DIV2, 0x52c, 1),
-	DEF_MOD("wdt0_pclk",		R9A08G045_WDT0_PCLK, R9A08G045_CLK_P0, 0x548, 0),
-	DEF_MOD("wdt0_clk",		R9A08G045_WDT0_CLK, R9A08G045_OSCCLK, 0x548, 1),
-	DEF_MOD("sdhi0_imclk",		R9A08G045_SDHI0_IMCLK, CLK_SD0_DIV4, 0x554, 0),
-	DEF_MOD("sdhi0_imclk2",		R9A08G045_SDHI0_IMCLK2, CLK_SD0_DIV4, 0x554, 1),
-	DEF_MOD("sdhi0_clk_hs",		R9A08G045_SDHI0_CLK_HS, R9A08G045_CLK_SD0, 0x554, 2),
-	DEF_MOD("sdhi0_aclk",		R9A08G045_SDHI0_ACLK, R9A08G045_CLK_P1, 0x554, 3),
-	DEF_MOD("sdhi1_imclk",		R9A08G045_SDHI1_IMCLK, CLK_SD1_DIV4, 0x554, 4),
-	DEF_MOD("sdhi1_imclk2",		R9A08G045_SDHI1_IMCLK2, CLK_SD1_DIV4, 0x554, 5),
-	DEF_MOD("sdhi1_clk_hs",		R9A08G045_SDHI1_CLK_HS, R9A08G045_CLK_SD1, 0x554, 6),
-	DEF_MOD("sdhi1_aclk",		R9A08G045_SDHI1_ACLK, R9A08G045_CLK_P1, 0x554, 7),
-	DEF_MOD("sdhi2_imclk",		R9A08G045_SDHI2_IMCLK, CLK_SD2_DIV4, 0x554, 8),
-	DEF_MOD("sdhi2_imclk2",		R9A08G045_SDHI2_IMCLK2, CLK_SD2_DIV4, 0x554, 9),
-	DEF_MOD("sdhi2_clk_hs",		R9A08G045_SDHI2_CLK_HS, R9A08G045_CLK_SD2, 0x554, 10),
-	DEF_MOD("sdhi2_aclk",		R9A08G045_SDHI2_ACLK, R9A08G045_CLK_P1, 0x554, 11),
-	DEF_MOD("ssi0_pclk2",		R9A08G045_SSI0_PCLK2, R9A08G045_CLK_P0, 0x570, 0),
-	DEF_MOD("ssi0_sfr",		R9A08G045_SSI0_PCLK_SFR, R9A08G045_CLK_P0, 0x570, 1),
-	DEF_MOD("ssi1_pclk2",		R9A08G045_SSI1_PCLK2, R9A08G045_CLK_P0, 0x570, 2),
-	DEF_MOD("ssi1_sfr",		R9A08G045_SSI1_PCLK_SFR, R9A08G045_CLK_P0, 0x570, 3),
-	DEF_MOD("ssi2_pclk2",		R9A08G045_SSI2_PCLK2, R9A08G045_CLK_P0, 0x570, 4),
-	DEF_MOD("ssi2_sfr",		R9A08G045_SSI2_PCLK_SFR, R9A08G045_CLK_P0, 0x570, 5),
-	DEF_MOD("ssi3_pclk2",		R9A08G045_SSI3_PCLK2, R9A08G045_CLK_P0, 0x570, 6),
-	DEF_MOD("ssi3_sfr",		R9A08G045_SSI3_PCLK_SFR, R9A08G045_CLK_P0, 0x570, 7),
-	DEF_MOD("usb0_host",		R9A08G045_USB_U2H0_HCLK, R9A08G045_CLK_P1, 0x578, 0),
-	DEF_MOD("usb1_host",		R9A08G045_USB_U2H1_HCLK, R9A08G045_CLK_P1, 0x578, 1),
-	DEF_MOD("usb0_func",		R9A08G045_USB_U2P_EXR_CPUCLK, R9A08G045_CLK_P1, 0x578, 2),
-	DEF_MOD("usb_pclk",		R9A08G045_USB_PCLK, R9A08G045_CLK_P1, 0x578, 3),
-	DEF_COUPLED("eth0_axi",		R9A08G045_ETH0_CLK_AXI, R9A08G045_CLK_M0, 0x57c, 0),
-	DEF_COUPLED("eth0_chi",		R9A08G045_ETH0_CLK_CHI, R9A08G045_CLK_ZT, 0x57c, 0),
-	DEF_MOD("eth0_refclk",		R9A08G045_ETH0_REFCLK, R9A08G045_CLK_HP, 0x57c, 8),
-	DEF_COUPLED("eth1_axi",		R9A08G045_ETH1_CLK_AXI, R9A08G045_CLK_M0, 0x57c, 1),
-	DEF_COUPLED("eth1_chi",		R9A08G045_ETH1_CLK_CHI, R9A08G045_CLK_ZT, 0x57c, 1),
-	DEF_MOD("eth1_refclk",		R9A08G045_ETH1_REFCLK, R9A08G045_CLK_HP, 0x57c, 9),
-	DEF_MOD("i2c0_pclk",		R9A08G045_I2C0_PCLK, R9A08G045_CLK_P0, 0x580, 0),
-	DEF_MOD("i2c1_pclk",		R9A08G045_I2C1_PCLK, R9A08G045_CLK_P0, 0x580, 1),
-	DEF_MOD("i2c2_pclk",		R9A08G045_I2C2_PCLK, R9A08G045_CLK_P0, 0x580, 2),
-	DEF_MOD("i2c3_pclk",		R9A08G045_I2C3_PCLK, R9A08G045_CLK_P0, 0x580, 3),
-	DEF_MOD("scif0_clk_pck",	R9A08G045_SCIF0_CLK_PCK, R9A08G045_CLK_P0, 0x584, 0),
-	DEF_MOD("scif1_clk_pck",	R9A08G045_SCIF1_CLK_PCK, R9A08G045_CLK_P0, 0x584, 1),
-	DEF_MOD("scif2_clk_pck",	R9A08G045_SCIF2_CLK_PCK, R9A08G045_CLK_P0, 0x584, 2),
-	DEF_MOD("scif3_clk_pck",	R9A08G045_SCIF3_CLK_PCK, R9A08G045_CLK_P0, 0x584, 3),
-	DEF_MOD("scif4_clk_pck",	R9A08G045_SCIF4_CLK_PCK, R9A08G045_CLK_P0, 0x584, 4),
-	DEF_MOD("scif5_clk_pck",	R9A08G045_SCIF5_CLK_PCK, R9A08G045_CLK_P0, 0x584, 5),
-	DEF_MOD("gpio_hclk",		R9A08G045_GPIO_HCLK, R9A08G045_OSCCLK, 0x598, 0),
-	DEF_MOD("adc_adclk",		R9A08G045_ADC_ADCLK, R9A08G045_CLK_TSU, 0x5a8, 0),
-	DEF_MOD("adc_pclk",		R9A08G045_ADC_PCLK, R9A08G045_CLK_TSU, 0x5a8, 1),
-	DEF_MOD("tsu_pclk",		R9A08G045_TSU_PCLK, R9A08G045_CLK_TSU, 0x5ac, 0),
-	DEF_MOD("vbat_bclk",		R9A08G045_VBAT_BCLK, R9A08G045_OSCCLK, 0x614, 0),
+	DEF_MOD("gic_gicclk",		R9A08G045_GIC600_GICCLK, R9A08G045_CLK_P1, 0x514, 0, 0),
+	DEF_MOD("ia55_pclk",		R9A08G045_IA55_PCLK, R9A08G045_CLK_P2, 0x518, 0, 0),
+	DEF_MOD("ia55_clk",		R9A08G045_IA55_CLK, R9A08G045_CLK_P1, 0x518, 1, 0),
+	DEF_MOD("dmac_aclk",		R9A08G045_DMAC_ACLK, R9A08G045_CLK_P3, 0x52c, 0, 0),
+	DEF_MOD("dmac_pclk",		R9A08G045_DMAC_PCLK, CLK_P3_DIV2, 0x52c, 1, 0),
+	DEF_MOD("wdt0_pclk",		R9A08G045_WDT0_PCLK, R9A08G045_CLK_P0, 0x548, 0, 0),
+	DEF_MOD("wdt0_clk",		R9A08G045_WDT0_CLK, R9A08G045_OSCCLK, 0x548, 1, 0),
+	DEF_MOD("sdhi0_imclk",		R9A08G045_SDHI0_IMCLK, CLK_SD0_DIV4, 0x554, 0, 0),
+	DEF_MOD("sdhi0_imclk2",		R9A08G045_SDHI0_IMCLK2, CLK_SD0_DIV4, 0x554, 1, 0),
+	DEF_MOD("sdhi0_clk_hs",		R9A08G045_SDHI0_CLK_HS, R9A08G045_CLK_SD0, 0x554, 2, 0),
+	DEF_MOD("sdhi0_aclk",		R9A08G045_SDHI0_ACLK, R9A08G045_CLK_P1, 0x554, 3, 0),
+	DEF_MOD("sdhi1_imclk",		R9A08G045_SDHI1_IMCLK, CLK_SD1_DIV4, 0x554, 4, 0),
+	DEF_MOD("sdhi1_imclk2",		R9A08G045_SDHI1_IMCLK2, CLK_SD1_DIV4, 0x554, 5, 0),
+	DEF_MOD("sdhi1_clk_hs",		R9A08G045_SDHI1_CLK_HS, R9A08G045_CLK_SD1, 0x554, 6, 0),
+	DEF_MOD("sdhi1_aclk",		R9A08G045_SDHI1_ACLK, R9A08G045_CLK_P1, 0x554, 7, 0),
+	DEF_MOD("sdhi2_imclk",		R9A08G045_SDHI2_IMCLK, CLK_SD2_DIV4, 0x554, 8, 0),
+	DEF_MOD("sdhi2_imclk2",		R9A08G045_SDHI2_IMCLK2, CLK_SD2_DIV4, 0x554, 9, 0),
+	DEF_MOD("sdhi2_clk_hs",		R9A08G045_SDHI2_CLK_HS, R9A08G045_CLK_SD2, 0x554, 10, 0),
+	DEF_MOD("sdhi2_aclk",		R9A08G045_SDHI2_ACLK, R9A08G045_CLK_P1, 0x554, 11, 0),
+	DEF_MOD("ssi0_pclk2",		R9A08G045_SSI0_PCLK2, R9A08G045_CLK_P0, 0x570, 0, 0),
+	DEF_MOD("ssi0_sfr",		R9A08G045_SSI0_PCLK_SFR, R9A08G045_CLK_P0, 0x570, 1, 0),
+	DEF_MOD("ssi1_pclk2",		R9A08G045_SSI1_PCLK2, R9A08G045_CLK_P0, 0x570, 2, 0),
+	DEF_MOD("ssi1_sfr",		R9A08G045_SSI1_PCLK_SFR, R9A08G045_CLK_P0, 0x570, 3, 0),
+	DEF_MOD("ssi2_pclk2",		R9A08G045_SSI2_PCLK2, R9A08G045_CLK_P0, 0x570, 4, 0),
+	DEF_MOD("ssi2_sfr",		R9A08G045_SSI2_PCLK_SFR, R9A08G045_CLK_P0, 0x570, 5, 0),
+	DEF_MOD("ssi3_pclk2",		R9A08G045_SSI3_PCLK2, R9A08G045_CLK_P0, 0x570, 6, 0),
+	DEF_MOD("ssi3_sfr",		R9A08G045_SSI3_PCLK_SFR, R9A08G045_CLK_P0, 0x570, 7, 0),
+	DEF_MOD("usb0_host",		R9A08G045_USB_U2H0_HCLK, R9A08G045_CLK_P1, 0x578, 0, 0),
+	DEF_MOD("usb1_host",		R9A08G045_USB_U2H1_HCLK, R9A08G045_CLK_P1, 0x578, 1, 0),
+	DEF_MOD("usb0_func",		R9A08G045_USB_U2P_EXR_CPUCLK, R9A08G045_CLK_P1, 0x578, 2,
+					0),
+	DEF_MOD("usb_pclk",		R9A08G045_USB_PCLK, R9A08G045_CLK_P1, 0x578, 3, 0),
+	DEF_COUPLED("eth0_axi",		R9A08G045_ETH0_CLK_AXI, R9A08G045_CLK_M0, 0x57c, 0, 0),
+	DEF_COUPLED("eth0_chi",		R9A08G045_ETH0_CLK_CHI, R9A08G045_CLK_ZT, 0x57c, 0, 0),
+	DEF_MOD("eth0_refclk",		R9A08G045_ETH0_REFCLK, R9A08G045_CLK_HP, 0x57c, 8, 0),
+	DEF_COUPLED("eth1_axi",		R9A08G045_ETH1_CLK_AXI, R9A08G045_CLK_M0, 0x57c, 1, 0),
+	DEF_COUPLED("eth1_chi",		R9A08G045_ETH1_CLK_CHI, R9A08G045_CLK_ZT, 0x57c, 1, 0),
+	DEF_MOD("eth1_refclk",		R9A08G045_ETH1_REFCLK, R9A08G045_CLK_HP, 0x57c, 9, 0),
+	DEF_MOD("i2c0_pclk",		R9A08G045_I2C0_PCLK, R9A08G045_CLK_P0, 0x580, 0, 0),
+	DEF_MOD("i2c1_pclk",		R9A08G045_I2C1_PCLK, R9A08G045_CLK_P0, 0x580, 1, 0),
+	DEF_MOD("i2c2_pclk",		R9A08G045_I2C2_PCLK, R9A08G045_CLK_P0, 0x580, 2, 0),
+	DEF_MOD("i2c3_pclk",		R9A08G045_I2C3_PCLK, R9A08G045_CLK_P0, 0x580, 3, 0),
+	DEF_MOD("scif0_clk_pck",	R9A08G045_SCIF0_CLK_PCK, R9A08G045_CLK_P0, 0x584, 0, 0),
+	DEF_MOD("scif1_clk_pck",	R9A08G045_SCIF1_CLK_PCK, R9A08G045_CLK_P0, 0x584, 1, 0),
+	DEF_MOD("scif2_clk_pck",	R9A08G045_SCIF2_CLK_PCK, R9A08G045_CLK_P0, 0x584, 2, 0),
+	DEF_MOD("scif3_clk_pck",	R9A08G045_SCIF3_CLK_PCK, R9A08G045_CLK_P0, 0x584, 3, 0),
+	DEF_MOD("scif4_clk_pck",	R9A08G045_SCIF4_CLK_PCK, R9A08G045_CLK_P0, 0x584, 4, 0),
+	DEF_MOD("scif5_clk_pck",	R9A08G045_SCIF5_CLK_PCK, R9A08G045_CLK_P0, 0x584, 5, 0),
+	DEF_MOD("gpio_hclk",		R9A08G045_GPIO_HCLK, R9A08G045_OSCCLK, 0x598, 0, 0),
+	DEF_MOD("adc_adclk",		R9A08G045_ADC_ADCLK, R9A08G045_CLK_TSU, 0x5a8, 0, 0),
+	DEF_MOD("adc_pclk",		R9A08G045_ADC_PCLK, R9A08G045_CLK_TSU, 0x5a8, 1, 0),
+	DEF_MOD("tsu_pclk",		R9A08G045_TSU_PCLK, R9A08G045_CLK_TSU, 0x5ac, 0, 0),
+	DEF_MOD("vbat_bclk",		R9A08G045_VBAT_BCLK, R9A08G045_OSCCLK, 0x614, 0, 0),
 };
 
 static const struct rzg2l_reset r9a08g045_resets[] = {
diff --git a/drivers/clk/renesas/r9a09g011-cpg.c b/drivers/clk/renesas/r9a09g011-cpg.c
index 22272279b104..ba25429c244d 100644
--- a/drivers/clk/renesas/r9a09g011-cpg.c
+++ b/drivers/clk/renesas/r9a09g011-cpg.c
@@ -151,64 +151,64 @@ static const struct cpg_core_clk r9a09g011_core_clks[] __initconst = {
 };
 
 static const struct rzg2l_mod_clk r9a09g011_mod_clks[] __initconst = {
-	DEF_MOD("pfc",		R9A09G011_PFC_PCLK,	 CLK_MAIN,     0x400, 2),
-	DEF_MOD("gic",		R9A09G011_GIC_CLK,	 CLK_SEL_B_D2, 0x400, 5),
-	DEF_MOD("sdi0_aclk",	R9A09G011_SDI0_ACLK,	 CLK_SEL_D,    0x408, 0),
-	DEF_MOD("sdi0_imclk",	R9A09G011_SDI0_IMCLK,	 CLK_SEL_SDI,  0x408, 1),
-	DEF_MOD("sdi0_imclk2",	R9A09G011_SDI0_IMCLK2,	 CLK_SEL_SDI,  0x408, 2),
-	DEF_MOD("sdi0_clk_hs",	R9A09G011_SDI0_CLK_HS,	 CLK_PLL2_800, 0x408, 3),
-	DEF_MOD("sdi1_aclk",	R9A09G011_SDI1_ACLK,	 CLK_SEL_D,    0x408, 4),
-	DEF_MOD("sdi1_imclk",	R9A09G011_SDI1_IMCLK,	 CLK_SEL_SDI,  0x408, 5),
-	DEF_MOD("sdi1_imclk2",	R9A09G011_SDI1_IMCLK2,	 CLK_SEL_SDI,  0x408, 6),
-	DEF_MOD("sdi1_clk_hs",	R9A09G011_SDI1_CLK_HS,	 CLK_PLL2_800, 0x408, 7),
-	DEF_MOD("emm_aclk",	R9A09G011_EMM_ACLK,	 CLK_SEL_D,    0x408, 8),
-	DEF_MOD("emm_imclk",	R9A09G011_EMM_IMCLK,	 CLK_SEL_SDI,  0x408, 9),
-	DEF_MOD("emm_imclk2",	R9A09G011_EMM_IMCLK2,	 CLK_SEL_SDI,  0x408, 10),
-	DEF_MOD("emm_clk_hs",	R9A09G011_EMM_CLK_HS,	 CLK_PLL2_800, 0x408, 11),
-	DEF_COUPLED("eth_axi",	R9A09G011_ETH0_CLK_AXI,	 CLK_PLL2_200, 0x40c, 8),
-	DEF_COUPLED("eth_chi",	R9A09G011_ETH0_CLK_CHI,	 CLK_PLL2_100, 0x40c, 8),
-	DEF_MOD("eth_clk_gptp",	R9A09G011_ETH0_GPTP_EXT, CLK_PLL2_100, 0x40c, 9),
-	DEF_MOD("usb_aclk_h",	R9A09G011_USB_ACLK_H,	 CLK_SEL_D,    0x40c, 4),
-	DEF_MOD("usb_aclk_p",	R9A09G011_USB_ACLK_P,	 CLK_SEL_D,    0x40c, 5),
-	DEF_MOD("usb_pclk",	R9A09G011_USB_PCLK,	 CLK_SEL_E,    0x40c, 6),
-	DEF_MOD("syc_cnt_clk",	R9A09G011_SYC_CNT_CLK,	 CLK_MAIN_24,  0x41c, 12),
-	DEF_MOD("iic_pclk0",	R9A09G011_IIC_PCLK0,	 CLK_SEL_E,    0x420, 12),
-	DEF_MOD("cperi_grpb",	R9A09G011_CPERI_GRPB_PCLK, CLK_SEL_E,  0x424, 0),
-	DEF_MOD("tim_clk_8",	R9A09G011_TIM8_CLK,	 CLK_MAIN_2,   0x424, 4),
-	DEF_MOD("tim_clk_9",	R9A09G011_TIM9_CLK,	 CLK_MAIN_2,   0x424, 5),
-	DEF_MOD("tim_clk_10",	R9A09G011_TIM10_CLK,	 CLK_MAIN_2,   0x424, 6),
-	DEF_MOD("tim_clk_11",	R9A09G011_TIM11_CLK,	 CLK_MAIN_2,   0x424, 7),
-	DEF_MOD("tim_clk_12",	R9A09G011_TIM12_CLK,	 CLK_MAIN_2,   0x424, 8),
-	DEF_MOD("tim_clk_13",	R9A09G011_TIM13_CLK,	 CLK_MAIN_2,   0x424, 9),
-	DEF_MOD("tim_clk_14",	R9A09G011_TIM14_CLK,	 CLK_MAIN_2,   0x424, 10),
-	DEF_MOD("tim_clk_15",	R9A09G011_TIM15_CLK,	 CLK_MAIN_2,   0x424, 11),
-	DEF_MOD("iic_pclk1",	R9A09G011_IIC_PCLK1,	 CLK_SEL_E,    0x424, 12),
-	DEF_MOD("cperi_grpc",	R9A09G011_CPERI_GRPC_PCLK, CLK_SEL_E,  0x428, 0),
-	DEF_MOD("tim_clk_16",	R9A09G011_TIM16_CLK,	 CLK_MAIN_2,   0x428, 4),
-	DEF_MOD("tim_clk_17",	R9A09G011_TIM17_CLK,	 CLK_MAIN_2,   0x428, 5),
-	DEF_MOD("tim_clk_18",	R9A09G011_TIM18_CLK,	 CLK_MAIN_2,   0x428, 6),
-	DEF_MOD("tim_clk_19",	R9A09G011_TIM19_CLK,	 CLK_MAIN_2,   0x428, 7),
-	DEF_MOD("tim_clk_20",	R9A09G011_TIM20_CLK,	 CLK_MAIN_2,   0x428, 8),
-	DEF_MOD("tim_clk_21",	R9A09G011_TIM21_CLK,	 CLK_MAIN_2,   0x428, 9),
-	DEF_MOD("tim_clk_22",	R9A09G011_TIM22_CLK,	 CLK_MAIN_2,   0x428, 10),
-	DEF_MOD("tim_clk_23",	R9A09G011_TIM23_CLK,	 CLK_MAIN_2,   0x428, 11),
-	DEF_MOD("wdt0_pclk",	R9A09G011_WDT0_PCLK,	 CLK_SEL_E,    0x428, 12),
-	DEF_MOD("wdt0_clk",	R9A09G011_WDT0_CLK,	 CLK_MAIN,     0x428, 13),
-	DEF_MOD("cperi_grpf",	R9A09G011_CPERI_GRPF_PCLK, CLK_SEL_E,  0x434, 0),
-	DEF_MOD("pwm8_clk",	R9A09G011_PWM8_CLK,	 CLK_MAIN,     0x434, 4),
-	DEF_MOD("pwm9_clk",	R9A09G011_PWM9_CLK,	 CLK_MAIN,     0x434, 5),
-	DEF_MOD("pwm10_clk",	R9A09G011_PWM10_CLK,	 CLK_MAIN,     0x434, 6),
-	DEF_MOD("pwm11_clk",	R9A09G011_PWM11_CLK,	 CLK_MAIN,     0x434, 7),
-	DEF_MOD("pwm12_clk",	R9A09G011_PWM12_CLK,	 CLK_MAIN,     0x434, 8),
-	DEF_MOD("pwm13_clk",	R9A09G011_PWM13_CLK,	 CLK_MAIN,     0x434, 9),
-	DEF_MOD("pwm14_clk",	R9A09G011_PWM14_CLK,	 CLK_MAIN,     0x434, 10),
-	DEF_MOD("cperi_grpg",	R9A09G011_CPERI_GRPG_PCLK, CLK_SEL_E,  0x438, 0),
-	DEF_MOD("cperi_grph",	R9A09G011_CPERI_GRPH_PCLK, CLK_SEL_E,  0x438, 1),
-	DEF_MOD("urt_pclk",	R9A09G011_URT_PCLK,	 CLK_SEL_E,    0x438, 4),
-	DEF_MOD("urt0_clk",	R9A09G011_URT0_CLK,	 CLK_SEL_W0,   0x438, 5),
-	DEF_MOD("csi0_clk",	R9A09G011_CSI0_CLK,	 CLK_SEL_CSI0, 0x438, 8),
-	DEF_MOD("csi4_clk",	R9A09G011_CSI4_CLK,	 CLK_SEL_CSI4, 0x438, 12),
-	DEF_MOD("ca53",		R9A09G011_CA53_CLK,	 CLK_DIV_A,    0x448, 0),
+	DEF_MOD("pfc",		R9A09G011_PFC_PCLK,	 CLK_MAIN,     0x400, 2, 0),
+	DEF_MOD("gic",		R9A09G011_GIC_CLK,	 CLK_SEL_B_D2, 0x400, 5, 0),
+	DEF_MOD("sdi0_aclk",	R9A09G011_SDI0_ACLK,	 CLK_SEL_D,    0x408, 0, 0),
+	DEF_MOD("sdi0_imclk",	R9A09G011_SDI0_IMCLK,	 CLK_SEL_SDI,  0x408, 1, 0),
+	DEF_MOD("sdi0_imclk2",	R9A09G011_SDI0_IMCLK2,	 CLK_SEL_SDI,  0x408, 2, 0),
+	DEF_MOD("sdi0_clk_hs",	R9A09G011_SDI0_CLK_HS,	 CLK_PLL2_800, 0x408, 3, 0),
+	DEF_MOD("sdi1_aclk",	R9A09G011_SDI1_ACLK,	 CLK_SEL_D,    0x408, 4, 0),
+	DEF_MOD("sdi1_imclk",	R9A09G011_SDI1_IMCLK,	 CLK_SEL_SDI,  0x408, 5, 0),
+	DEF_MOD("sdi1_imclk2",	R9A09G011_SDI1_IMCLK2,	 CLK_SEL_SDI,  0x408, 6, 0),
+	DEF_MOD("sdi1_clk_hs",	R9A09G011_SDI1_CLK_HS,	 CLK_PLL2_800, 0x408, 7, 0),
+	DEF_MOD("emm_aclk",	R9A09G011_EMM_ACLK,	 CLK_SEL_D,    0x408, 8, 0),
+	DEF_MOD("emm_imclk",	R9A09G011_EMM_IMCLK,	 CLK_SEL_SDI,  0x408, 9, 0),
+	DEF_MOD("emm_imclk2",	R9A09G011_EMM_IMCLK2,	 CLK_SEL_SDI,  0x408, 10, 0),
+	DEF_MOD("emm_clk_hs",	R9A09G011_EMM_CLK_HS,	 CLK_PLL2_800, 0x408, 11, 0),
+	DEF_COUPLED("eth_axi",	R9A09G011_ETH0_CLK_AXI,	 CLK_PLL2_200, 0x40c, 8, 0),
+	DEF_COUPLED("eth_chi",	R9A09G011_ETH0_CLK_CHI,	 CLK_PLL2_100, 0x40c, 8, 0),
+	DEF_MOD("eth_clk_gptp",	R9A09G011_ETH0_GPTP_EXT, CLK_PLL2_100, 0x40c, 9, 0),
+	DEF_MOD("usb_aclk_h",	R9A09G011_USB_ACLK_H,	 CLK_SEL_D,    0x40c, 4, 0),
+	DEF_MOD("usb_aclk_p",	R9A09G011_USB_ACLK_P,	 CLK_SEL_D,    0x40c, 5, 0),
+	DEF_MOD("usb_pclk",	R9A09G011_USB_PCLK,	 CLK_SEL_E,    0x40c, 6, 0),
+	DEF_MOD("syc_cnt_clk",	R9A09G011_SYC_CNT_CLK,	 CLK_MAIN_24,  0x41c, 12, 0),
+	DEF_MOD("iic_pclk0",	R9A09G011_IIC_PCLK0,	 CLK_SEL_E,    0x420, 12, 0),
+	DEF_MOD("cperi_grpb",	R9A09G011_CPERI_GRPB_PCLK, CLK_SEL_E,  0x424, 0, 0),
+	DEF_MOD("tim_clk_8",	R9A09G011_TIM8_CLK,	 CLK_MAIN_2,   0x424, 4, 0),
+	DEF_MOD("tim_clk_9",	R9A09G011_TIM9_CLK,	 CLK_MAIN_2,   0x424, 5, 0),
+	DEF_MOD("tim_clk_10",	R9A09G011_TIM10_CLK,	 CLK_MAIN_2,   0x424, 6, 0),
+	DEF_MOD("tim_clk_11",	R9A09G011_TIM11_CLK,	 CLK_MAIN_2,   0x424, 7, 0),
+	DEF_MOD("tim_clk_12",	R9A09G011_TIM12_CLK,	 CLK_MAIN_2,   0x424, 8, 0),
+	DEF_MOD("tim_clk_13",	R9A09G011_TIM13_CLK,	 CLK_MAIN_2,   0x424, 9, 0),
+	DEF_MOD("tim_clk_14",	R9A09G011_TIM14_CLK,	 CLK_MAIN_2,   0x424, 10, 0),
+	DEF_MOD("tim_clk_15",	R9A09G011_TIM15_CLK,	 CLK_MAIN_2,   0x424, 11, 0),
+	DEF_MOD("iic_pclk1",	R9A09G011_IIC_PCLK1,	 CLK_SEL_E,    0x424, 12, 0),
+	DEF_MOD("cperi_grpc",	R9A09G011_CPERI_GRPC_PCLK, CLK_SEL_E,  0x428, 0, 0),
+	DEF_MOD("tim_clk_16",	R9A09G011_TIM16_CLK,	 CLK_MAIN_2,   0x428, 4, 0),
+	DEF_MOD("tim_clk_17",	R9A09G011_TIM17_CLK,	 CLK_MAIN_2,   0x428, 5, 0),
+	DEF_MOD("tim_clk_18",	R9A09G011_TIM18_CLK,	 CLK_MAIN_2,   0x428, 6, 0),
+	DEF_MOD("tim_clk_19",	R9A09G011_TIM19_CLK,	 CLK_MAIN_2,   0x428, 7, 0),
+	DEF_MOD("tim_clk_20",	R9A09G011_TIM20_CLK,	 CLK_MAIN_2,   0x428, 8, 0),
+	DEF_MOD("tim_clk_21",	R9A09G011_TIM21_CLK,	 CLK_MAIN_2,   0x428, 9, 0),
+	DEF_MOD("tim_clk_22",	R9A09G011_TIM22_CLK,	 CLK_MAIN_2,   0x428, 10, 0),
+	DEF_MOD("tim_clk_23",	R9A09G011_TIM23_CLK,	 CLK_MAIN_2,   0x428, 11, 0),
+	DEF_MOD("wdt0_pclk",	R9A09G011_WDT0_PCLK,	 CLK_SEL_E,    0x428, 12, 0),
+	DEF_MOD("wdt0_clk",	R9A09G011_WDT0_CLK,	 CLK_MAIN,     0x428, 13, 0),
+	DEF_MOD("cperi_grpf",	R9A09G011_CPERI_GRPF_PCLK, CLK_SEL_E,  0x434, 0, 0),
+	DEF_MOD("pwm8_clk",	R9A09G011_PWM8_CLK,	 CLK_MAIN,     0x434, 4, 0),
+	DEF_MOD("pwm9_clk",	R9A09G011_PWM9_CLK,	 CLK_MAIN,     0x434, 5, 0),
+	DEF_MOD("pwm10_clk",	R9A09G011_PWM10_CLK,	 CLK_MAIN,     0x434, 6, 0),
+	DEF_MOD("pwm11_clk",	R9A09G011_PWM11_CLK,	 CLK_MAIN,     0x434, 7, 0),
+	DEF_MOD("pwm12_clk",	R9A09G011_PWM12_CLK,	 CLK_MAIN,     0x434, 8, 0),
+	DEF_MOD("pwm13_clk",	R9A09G011_PWM13_CLK,	 CLK_MAIN,     0x434, 9, 0),
+	DEF_MOD("pwm14_clk",	R9A09G011_PWM14_CLK,	 CLK_MAIN,     0x434, 10, 0),
+	DEF_MOD("cperi_grpg",	R9A09G011_CPERI_GRPG_PCLK, CLK_SEL_E,  0x438, 0, 0),
+	DEF_MOD("cperi_grph",	R9A09G011_CPERI_GRPH_PCLK, CLK_SEL_E,  0x438, 1, 0),
+	DEF_MOD("urt_pclk",	R9A09G011_URT_PCLK,	 CLK_SEL_E,    0x438, 4, 0),
+	DEF_MOD("urt0_clk",	R9A09G011_URT0_CLK,	 CLK_SEL_W0,   0x438, 5, 0),
+	DEF_MOD("csi0_clk",	R9A09G011_CSI0_CLK,	 CLK_SEL_CSI0, 0x438, 8, 0),
+	DEF_MOD("csi4_clk",	R9A09G011_CSI4_CLK,	 CLK_SEL_CSI4, 0x438, 12, 0),
+	DEF_MOD("ca53",		R9A09G011_CA53_CLK,	 CLK_DIV_A,    0x448, 0, 0),
 };
 
 static const struct rzg2l_reset r9a09g011_resets[] = {
diff --git a/drivers/clk/renesas/rzg2l-cpg.c b/drivers/clk/renesas/rzg2l-cpg.c
index c619b2da92b0..844a89b37026 100644
--- a/drivers/clk/renesas/rzg2l-cpg.c
+++ b/drivers/clk/renesas/rzg2l-cpg.c
@@ -11,10 +11,13 @@
  * Copyright (C) 2015 Renesas Electronics Corp.
  */
 
+#include <linux/atomic.h>
 #include <linux/bitfield.h>
+#include <linux/cleanup.h>
 #include <linux/clk.h>
 #include <linux/clk-provider.h>
 #include <linux/clk/renesas.h>
+#include <linux/debugfs.h>
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/init.h>
@@ -68,6 +71,9 @@
 
 #define MAX_VCLK_FREQ		(148500000)
 
+#define MSTOP_OFF(conf)		FIELD_GET(GENMASK(31, 16), (conf))
+#define MSTOP_MASK(conf)	FIELD_GET(GENMASK(15, 0), (conf))
+
 /**
  * struct clk_hw_data - clock hardware data
  * @hw: clock hw
@@ -1181,22 +1187,39 @@ rzg2l_cpg_register_core_clk(const struct cpg_core_clk *core,
 		core->name, PTR_ERR(clk));
 }
 
+/**
+ * struct mstop - MSTOP specific data structure
+ * @usecnt: Usage counter for MSTOP settings (when zero the settings
+ *          are applied to register)
+ * @conf: MSTOP configuration (register offset, setup bits)
+ */
+struct mstop {
+	atomic_t usecnt;
+	u32 conf;
+};
+
 /**
  * struct mstp_clock - MSTP gating clock
  *
  * @hw: handle between common and hardware-specific interfaces
  * @priv: CPG/MSTP private data
  * @sibling: pointer to the other coupled clock
+ * @mstop: MSTOP configuration
+ * @shared_mstop_clks: clocks sharing the MSTOP with this clock
  * @off: register offset
  * @bit: ON/MON bit
+ * @num_shared_mstop_clks: number of the clocks sharing MSTOP with this clock
  * @enabled: soft state of the clock, if it is coupled with another clock
  */
 struct mstp_clock {
 	struct clk_hw hw;
 	struct rzg2l_cpg_priv *priv;
 	struct mstp_clock *sibling;
+	struct mstop *mstop;
+	struct mstp_clock **shared_mstop_clks;
 	u16 off;
 	u8 bit;
+	u8 num_shared_mstop_clks;
 	bool enabled;
 };
 
@@ -1209,6 +1232,94 @@ struct mstp_clock {
 		else if (((hw) = __clk_get_hw((priv)->clks[(priv)->num_core_clks + i])) && \
 			 ((mstp_clk) = to_mod_clock(hw)))
 
+/* Need to be called with a lock held to avoid concurrent access to mstop->usecnt. */
+static void rzg2l_mod_clock_module_set_state(struct mstp_clock *clock,
+					     bool standby)
+{
+	struct rzg2l_cpg_priv *priv = clock->priv;
+	struct mstop *mstop = clock->mstop;
+	bool update = false;
+	u32 value;
+
+	if (!mstop)
+		return;
+
+	value = MSTOP_MASK(mstop->conf) << 16;
+
+	if (standby) {
+		unsigned int criticals = 0;
+
+		for (unsigned int i = 0; i < clock->num_shared_mstop_clks; i++) {
+			struct mstp_clock *clk = clock->shared_mstop_clks[i];
+
+			if (clk_hw_get_flags(&clk->hw) & CLK_IS_CRITICAL)
+				criticals++;
+		}
+
+		/*
+		 * If this is a shared MSTOP and it is shared with critical clocks,
+		 * and the system boots up with this clock enabled but no driver
+		 * uses it the CCF will disable it (as it is unused). As we don't
+		 * increment reference counter for it at registration (to avoid
+		 * messing with clocks enabled at probe but later used by drivers)
+		 * do not set the MSTOP here too if it is shared with critical
+		 * clocks and ref counted only by those critical clocks.
+		 */
+		if (criticals && criticals == atomic_read(&mstop->usecnt))
+			return;
+
+		value |= MSTOP_MASK(mstop->conf);
+
+		/* Allow updates on probe when usecnt = 0. */
+		if (!atomic_read(&mstop->usecnt))
+			update = true;
+		else
+			update = atomic_dec_and_test(&mstop->usecnt);
+	} else {
+		atomic_inc(&mstop->usecnt);
+		update = true;
+	}
+
+	if (update)
+		writel(value, priv->base + MSTOP_OFF(mstop->conf));
+}
+
+static int rzg2l_mod_clock_mstop_show(struct seq_file *s, void *what)
+{
+	struct rzg2l_cpg_priv *priv = s->private;
+	struct mstp_clock *clk;
+	struct clk_hw *hw;
+
+	seq_printf(s, "%-20s %-5s %-10s\n", "", "", "MSTOP");
+	seq_printf(s, "%-20s %-5s %-10s\n", "", "clk", "-------------------------");
+	seq_printf(s, "%-20s %-5s %-5s %-5s %-6s %-6s\n",
+		   "clk_name", "cnt", "cnt", "off", "val", "shared");
+	seq_printf(s, "%-20s %-5s %-5s %-5s %-6s %-6s\n",
+		   "--------", "-----", "-----", "-----", "------", "------");
+
+	for_each_mstp_clk(clk, hw, priv) {
+		u32 val;
+
+		if (!clk->mstop)
+			continue;
+
+		val = readl(priv->base + MSTOP_OFF(clk->mstop->conf)) &
+		      MSTOP_MASK(clk->mstop->conf);
+
+		seq_printf(s, "%-20s %-5d %-5d 0x%-3lx 0x%-4x", clk_hw_get_name(hw),
+			   __clk_get_enable_count(hw->clk), atomic_read(&clk->mstop->usecnt),
+			   MSTOP_OFF(clk->mstop->conf), val);
+
+		for (unsigned int i = 0; i < clk->num_shared_mstop_clks; i++)
+			seq_printf(s, " %pC", clk->shared_mstop_clks[i]->hw.clk);
+
+		seq_puts(s, "\n");
+	}
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(rzg2l_mod_clock_mstop);
+
 static int rzg2l_mod_clock_endisable(struct clk_hw *hw, bool enable)
 {
 	struct mstp_clock *clock = to_mod_clock(hw);
@@ -1231,7 +1342,15 @@ static int rzg2l_mod_clock_endisable(struct clk_hw *hw, bool enable)
 	if (enable)
 		value |= bitmask;
 
-	writel(value, priv->base + CLK_ON_R(reg));
+	scoped_guard(spinlock_irqsave, &priv->rmw_lock) {
+		if (enable) {
+			writel(value, priv->base + CLK_ON_R(reg));
+			rzg2l_mod_clock_module_set_state(clock, false);
+		} else {
+			rzg2l_mod_clock_module_set_state(clock, true);
+			writel(value, priv->base + CLK_ON_R(reg));
+		}
+	}
 
 	if (!enable)
 		return 0;
@@ -1332,6 +1451,94 @@ static struct mstp_clock
 	return NULL;
 }
 
+static struct mstop *rzg2l_mod_clock_get_mstop(struct rzg2l_cpg_priv *priv, u32 conf)
+{
+	struct mstp_clock *clk;
+	struct clk_hw *hw;
+
+	for_each_mstp_clk(clk, hw, priv) {
+		if (!clk->mstop)
+			continue;
+
+		if (clk->mstop->conf == conf)
+			return clk->mstop;
+	}
+
+	return NULL;
+}
+
+static void rzg2l_mod_clock_init_mstop(struct rzg2l_cpg_priv *priv)
+{
+	struct mstp_clock *clk;
+	struct clk_hw *hw;
+
+	for_each_mstp_clk(clk, hw, priv) {
+		if (!clk->mstop)
+			continue;
+
+		/*
+		 * Out of reset all modules are enabled. Set module state
+		 * in case associated clocks are disabled at probe. Otherwise
+		 * module is in invalid HW state.
+		 */
+		scoped_guard(spinlock_irqsave, &priv->rmw_lock) {
+			if (!rzg2l_mod_clock_is_enabled(&clk->hw))
+				rzg2l_mod_clock_module_set_state(clk, true);
+		}
+	}
+}
+
+static int rzg2l_mod_clock_update_shared_mstop_clks(struct rzg2l_cpg_priv *priv,
+						    struct mstp_clock *clock)
+{
+	struct mstp_clock *clk;
+	struct clk_hw *hw;
+
+	if (!clock->mstop)
+		return 0;
+
+	for_each_mstp_clk(clk, hw, priv) {
+		struct mstp_clock **new_clks;
+		int num_shared_mstop_clks;
+		bool found = false;
+
+		if (clk->mstop != clock->mstop)
+			continue;
+
+		num_shared_mstop_clks = clk->num_shared_mstop_clks;
+		for (unsigned int i = 0; i < num_shared_mstop_clks; i++) {
+			if (clk->shared_mstop_clks[i] == clock) {
+				found = true;
+				break;
+			}
+		}
+		if (found)
+			continue;
+
+		if (!num_shared_mstop_clks)
+			new_clks = devm_kmalloc_array(priv->dev, 2, sizeof(*new_clks), GFP_KERNEL);
+		else
+			new_clks = devm_krealloc(priv->dev, clk->shared_mstop_clks,
+						 (num_shared_mstop_clks + 1) * sizeof(*new_clks),
+						 GFP_KERNEL);
+
+		if (!new_clks)
+			return -ENOMEM;
+
+		if (!num_shared_mstop_clks)
+			new_clks[num_shared_mstop_clks++] = clk;
+		if (clk != clock)
+			new_clks[num_shared_mstop_clks++] = clock;
+
+		for (unsigned int i = 0; i < num_shared_mstop_clks; i++) {
+			new_clks[i]->shared_mstop_clks = new_clks;
+			new_clks[i]->num_shared_mstop_clks = num_shared_mstop_clks;
+		}
+	}
+
+	return 0;
+}
+
 static void __init
 rzg2l_cpg_register_mod_clk(const struct rzg2l_mod_clk *mod,
 			   const struct rzg2l_cpg_info *info,
@@ -1383,6 +1590,21 @@ rzg2l_cpg_register_mod_clk(const struct rzg2l_mod_clk *mod,
 	clock->priv = priv;
 	clock->hw.init = &init;
 
+	if (mod->mstop_conf) {
+		struct mstop *mstop = rzg2l_mod_clock_get_mstop(priv, mod->mstop_conf);
+
+		if (!mstop) {
+			mstop = devm_kzalloc(dev, sizeof(*mstop), GFP_KERNEL);
+			if (!mstop) {
+				clk = ERR_PTR(-ENOMEM);
+				goto fail;
+			}
+			mstop->conf = mod->mstop_conf;
+			atomic_set(&mstop->usecnt, 0);
+		}
+		clock->mstop = mstop;
+	}
+
 	ret = devm_clk_hw_register(dev, &clock->hw);
 	if (ret) {
 		clk = ERR_PTR(ret);
@@ -1404,6 +1626,13 @@ rzg2l_cpg_register_mod_clk(const struct rzg2l_mod_clk *mod,
 	dev_dbg(dev, "Module clock %pC at %lu Hz\n", clk, clk_get_rate(clk));
 	priv->clks[id] = clk;
 
+	/* Keep this after priv->clks[id] is updated. */
+	ret = rzg2l_mod_clock_update_shared_mstop_clks(priv, clock);
+	if (ret) {
+		clk = ERR_PTR(ret);
+		goto fail;
+	}
+
 	return;
 
 fail:
@@ -1875,6 +2104,13 @@ static int __init rzg2l_cpg_probe(struct platform_device *pdev)
 	for (i = 0; i < info->num_mod_clks; i++)
 		rzg2l_cpg_register_mod_clk(&info->mod_clks[i], info, priv);
 
+	/*
+	 * Initialize MSTOP after all the clocks were registered to avoid
+	 * invalid reference counting when multiple clocks (critical,
+	 * non-critical) share the same MSTOP.
+	 */
+	rzg2l_mod_clock_init_mstop(priv);
+
 	error = of_clk_add_provider(np, rzg2l_cpg_clk_src_twocell_get, priv);
 	if (error)
 		return error;
@@ -1891,9 +2127,23 @@ static int __init rzg2l_cpg_probe(struct platform_device *pdev)
 	if (error)
 		return error;
 
+	debugfs_create_file("mstop", 0444, NULL, priv, &rzg2l_mod_clock_mstop_fops);
+	return 0;
+}
+
+static int rzg2l_cpg_resume(struct device *dev)
+{
+	struct rzg2l_cpg_priv *priv = dev_get_drvdata(dev);
+
+	rzg2l_mod_clock_init_mstop(priv);
+
 	return 0;
 }
 
+static const struct dev_pm_ops rzg2l_cpg_pm_ops = {
+	NOIRQ_SYSTEM_SLEEP_PM_OPS(NULL, rzg2l_cpg_resume)
+};
+
 static const struct of_device_id rzg2l_cpg_match[] = {
 #ifdef CONFIG_CLK_R9A07G043
 	{
@@ -1932,6 +2182,7 @@ static struct platform_driver rzg2l_cpg_driver = {
 	.driver		= {
 		.name	= "rzg2l-cpg",
 		.of_match_table = rzg2l_cpg_match,
+		.pm	= pm_sleep_ptr(&rzg2l_cpg_pm_ops),
 	},
 };
 
diff --git a/drivers/clk/renesas/rzg2l-cpg.h b/drivers/clk/renesas/rzg2l-cpg.h
index b6eece5ffa20..50a5a23f2e6a 100644
--- a/drivers/clk/renesas/rzg2l-cpg.h
+++ b/drivers/clk/renesas/rzg2l-cpg.h
@@ -82,6 +82,8 @@
 #define SEL_PLL6_2	SEL_PLL_PACK(CPG_PL6_ETH_SSEL, 0, 1)
 #define SEL_GPU2	SEL_PLL_PACK(CPG_PL6_SSEL, 12, 1)
 
+#define MSTOP(name, bitmask)	((CPG_##name##_MSTOP) << 16 | (bitmask))
+
 #define EXTAL_FREQ_IN_MEGA_HZ	(24)
 
 /**
@@ -201,6 +203,7 @@ enum clk_types {
  * @name: handle between common and hardware-specific interfaces
  * @id: clock index in array containing all Core and Module Clocks
  * @parent: id of parent clock
+ * @mstop_conf: MSTOP configuration
  * @off: register offset
  * @bit: ON/MON bit
  * @is_coupled: flag to indicate coupled clock
@@ -209,26 +212,28 @@ struct rzg2l_mod_clk {
 	const char *name;
 	unsigned int id;
 	unsigned int parent;
+	u32 mstop_conf;
 	u16 off;
 	u8 bit;
 	bool is_coupled;
 };
 
-#define DEF_MOD_BASE(_name, _id, _parent, _off, _bit, _is_coupled)	\
+#define DEF_MOD_BASE(_name, _id, _parent, _off, _bit, _mstop_conf, _is_coupled)	\
 	{ \
 		.name = _name, \
 		.id = MOD_CLK_BASE + (_id), \
 		.parent = (_parent), \
+		.mstop_conf = (_mstop_conf), \
 		.off = (_off), \
 		.bit = (_bit), \
 		.is_coupled = (_is_coupled), \
 	}
 
-#define DEF_MOD(_name, _id, _parent, _off, _bit)	\
-	DEF_MOD_BASE(_name, _id, _parent, _off, _bit, false)
+#define DEF_MOD(_name, _id, _parent, _off, _bit, _mstop_conf)	\
+	DEF_MOD_BASE(_name, _id, _parent, _off, _bit, _mstop_conf, false)
 
-#define DEF_COUPLED(_name, _id, _parent, _off, _bit)	\
-	DEF_MOD_BASE(_name, _id, _parent, _off, _bit, true)
+#define DEF_COUPLED(_name, _id, _parent, _off, _bit, _mstop_conf)	\
+	DEF_MOD_BASE(_name, _id, _parent, _off, _bit, _mstop_conf, true)
 
 /**
  * struct rzg2l_reset - Reset definitions
-- 
2.43.0

Re: [PATCH v2 4/8] clk: renesas: rzg2l-cpg: Add support for MSTOP in clock enable/disable API

[Geert Uytterhoeven]

Hi Claudiu,

On Wed, 14 May 2025 at 11:04, Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> The RZ/{G2L, V2L, G3S} CPG versions support a feature called MSTOP. Each
> module has one or more MSTOP bits associated with it, and these bits need
> to be configured along with the module clocks. Setting the MSTOP bits
> switches the module between normal and standby states.
>
> Previously, MSTOP support was abstracted through power domains
> (struct generic_pm_domain::{power_on, power_off} APIs). With this
> abstraction, the order of setting the MSTOP and CLKON bits was as follows:
>
> Previous Order:
> A/ Switching to Normal State (e.g., during probe):
> 1/ Clear module MSTOP bit
> 2/ Set module CLKON bit
>
> B/ Switching to Standby State (e.g., during remove):
> 1/ Clear CLKON bit
> 2/ Set MSTOP bit
>
> However, in some cases (when the clock is disabled through devres), the
> order may have been (due to the issue described in link section):
>
> 1/ Set MSTOP bit
> 2/ Clear CLKON bit
>
> Recently, the hardware team has suggested that the correct order to set
> the MSTOP and CLKON bits is:
>
> Updated Order:
> A/ Switching to Normal State (e.g., during probe):
> 1/ Set CLKON bit
> 2/ Clear MSTOP bit
>
> B/ Switching to Standby State (e.g., during remove):
> 1/ Set MSTOP bit
> 2/ Clear CLKON bit
>
> To prevent future issues due to incorrect ordering, the MSTOP setup has
> now been implemented in rzg2l_mod_clock_endisable(), ensuring compliance
> with the sequence suggested in Figure 41.5: Module Standby Mode Procedure
> from the RZ/G3S HW manual, Rev1.10.
>
> Additionally, since multiple clocks of a single module may be mapped to a
> single MSTOP bit, MSTOP setup is reference-counted.
>
> Furthermore, as all modules start in the normal state after reset, if the
> module clocks are disabled, the module state is switched to standby. This
> prevents keeping the module in an invalid state, as recommended by the
> hardware team.
>
> Link: https://lore.kernel.org/all/20250215130849.227812-1-claudiu.beznea.uj@bp.renesas.com/
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
>
> Changes in v2:
> - udpated patch description to avoid plural in the configuration
>   sequence description b/w MSTOP and CLK_ON
> - use atomic type to store the usage counter; s/refcnt/usecnt/g
> - moved MSTOP_OFF(), MSTOP_MASK() macros to rzg2l-cpg.c
> - dropped struct mstp_clock::critical and use clk_hw_get_flags()
>   instead to get the clock flags
> - used unsigned int iterators in for loops
> - keep memory allocated for a single list for clocks sharing the
>   same MSTOP by updating the rzg2l_mod_clock_add_shared_mstop_clk();
> - s/rzg2l_cpg_mstop_show/rzg2l_mod_clock_mstop_show/g,
>   s/rzg2l_cpg_mstop/rzg2l_mod_clock_mstop/g,
>   s/rzg2l_cpg_update_shared_mstop_clocks/rzg2l_mod_clock_update_shared_mstop_clks/g
>    to keep the same naming conventions for functions handling mod clock MSTOP
> - use the newly added for_each_mstp_clk() macro all over the code

Thanks for the update!

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

> @@ -1209,6 +1232,94 @@ struct mstp_clock {
>                 else if (((hw) = __clk_get_hw((priv)->clks[(priv)->num_core_clks + i])) && \
>                          ((mstp_clk) = to_mod_clock(hw)))
>
> +/* Need to be called with a lock held to avoid concurrent access to mstop->usecnt. */
> +static void rzg2l_mod_clock_module_set_state(struct mstp_clock *clock,
> +                                            bool standby)
> +{
> +       struct rzg2l_cpg_priv *priv = clock->priv;
> +       struct mstop *mstop = clock->mstop;
> +       bool update = false;
> +       u32 value;
> +
> +       if (!mstop)
> +               return;
> +
> +       value = MSTOP_MASK(mstop->conf) << 16;
> +
> +       if (standby) {
> +               unsigned int criticals = 0;
> +
> +               for (unsigned int i = 0; i < clock->num_shared_mstop_clks; i++) {
> +                       struct mstp_clock *clk = clock->shared_mstop_clks[i];
> +
> +                       if (clk_hw_get_flags(&clk->hw) & CLK_IS_CRITICAL)
> +                               criticals++;
> +               }
> +
> +               /*
> +                * If this is a shared MSTOP and it is shared with critical clocks,
> +                * and the system boots up with this clock enabled but no driver
> +                * uses it the CCF will disable it (as it is unused). As we don't
> +                * increment reference counter for it at registration (to avoid
> +                * messing with clocks enabled at probe but later used by drivers)
> +                * do not set the MSTOP here too if it is shared with critical
> +                * clocks and ref counted only by those critical clocks.
> +                */
> +               if (criticals && criticals == atomic_read(&mstop->usecnt))
> +                       return;
> +
> +               value |= MSTOP_MASK(mstop->conf);
> +
> +               /* Allow updates on probe when usecnt = 0. */
> +               if (!atomic_read(&mstop->usecnt))
> +                       update = true;
> +               else
> +                       update = atomic_dec_and_test(&mstop->usecnt);
> +       } else {
> +               atomic_inc(&mstop->usecnt);
> +               update = true;

Shouldn't the update be conditional, i.e.:

    if (!atomic_read(&mstop->usecnt))
            update = true;
    atomic_inc(&mstop->usecnt);

?

> +       }
> +
> +       if (update)
> +               writel(value, priv->base + MSTOP_OFF(mstop->conf));
> +}

> +static int rzg2l_mod_clock_update_shared_mstop_clks(struct rzg2l_cpg_priv *priv,
> +                                                   struct mstp_clock *clock)
> +{
> +       struct mstp_clock *clk;
> +       struct clk_hw *hw;
> +
> +       if (!clock->mstop)
> +               return 0;
> +
> +       for_each_mstp_clk(clk, hw, priv) {
> +               struct mstp_clock **new_clks;
> +               int num_shared_mstop_clks;
> +               bool found = false;
> +
> +               if (clk->mstop != clock->mstop)
> +                       continue;
> +
> +               num_shared_mstop_clks = clk->num_shared_mstop_clks;
> +               for (unsigned int i = 0; i < num_shared_mstop_clks; i++) {
> +                       if (clk->shared_mstop_clks[i] == clock) {
> +                               found = true;
> +                               break;
> +                       }
> +               }
> +               if (found)
> +                       continue;

Can this happen? With your current code, the answer is yes.
But I think this loop and check can be removed...

> +
> +               if (!num_shared_mstop_clks)
> +                       new_clks = devm_kmalloc_array(priv->dev, 2, sizeof(*new_clks), GFP_KERNEL);
> +               else
> +                       new_clks = devm_krealloc(priv->dev, clk->shared_mstop_clks,
> +                                                (num_shared_mstop_clks + 1) * sizeof(*new_clks),
> +                                                GFP_KERNEL);
> +
> +               if (!new_clks)
> +                       return -ENOMEM;
> +
> +               if (!num_shared_mstop_clks)
> +                       new_clks[num_shared_mstop_clks++] = clk;
> +               if (clk != clock)

This check is always true

> +                       new_clks[num_shared_mstop_clks++] = clock;
> +
> +               for (unsigned int i = 0; i < num_shared_mstop_clks; i++) {
> +                       new_clks[i]->shared_mstop_clks = new_clks;
> +                       new_clks[i]->num_shared_mstop_clks = num_shared_mstop_clks;
> +               }

... by adding a "break" here.  The loop above has already updated the
shared_mstop_clks[] arrays for all clocks sharing the same mstop value.

> +       }
> +
> +       return 0;
> +}

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

Re: [PATCH v2 4/8] clk: renesas: rzg2l-cpg: Add support for MSTOP in clock enable/disable API

[Claudiu Beznea]

Hi, Geert,

On 22.05.2025 17:46, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> On Wed, 14 May 2025 at 11:04, Claudiu <claudiu.beznea@tuxon.dev> wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> The RZ/{G2L, V2L, G3S} CPG versions support a feature called MSTOP. Each
>> module has one or more MSTOP bits associated with it, and these bits need
>> to be configured along with the module clocks. Setting the MSTOP bits
>> switches the module between normal and standby states.
>>
>> Previously, MSTOP support was abstracted through power domains
>> (struct generic_pm_domain::{power_on, power_off} APIs). With this
>> abstraction, the order of setting the MSTOP and CLKON bits was as follows:
>>
>> Previous Order:
>> A/ Switching to Normal State (e.g., during probe):
>> 1/ Clear module MSTOP bit
>> 2/ Set module CLKON bit
>>
>> B/ Switching to Standby State (e.g., during remove):
>> 1/ Clear CLKON bit
>> 2/ Set MSTOP bit
>>
>> However, in some cases (when the clock is disabled through devres), the
>> order may have been (due to the issue described in link section):
>>
>> 1/ Set MSTOP bit
>> 2/ Clear CLKON bit
>>
>> Recently, the hardware team has suggested that the correct order to set
>> the MSTOP and CLKON bits is:
>>
>> Updated Order:
>> A/ Switching to Normal State (e.g., during probe):
>> 1/ Set CLKON bit
>> 2/ Clear MSTOP bit
>>
>> B/ Switching to Standby State (e.g., during remove):
>> 1/ Set MSTOP bit
>> 2/ Clear CLKON bit
>>
>> To prevent future issues due to incorrect ordering, the MSTOP setup has
>> now been implemented in rzg2l_mod_clock_endisable(), ensuring compliance
>> with the sequence suggested in Figure 41.5: Module Standby Mode Procedure
>> from the RZ/G3S HW manual, Rev1.10.
>>
>> Additionally, since multiple clocks of a single module may be mapped to a
>> single MSTOP bit, MSTOP setup is reference-counted.
>>
>> Furthermore, as all modules start in the normal state after reset, if the
>> module clocks are disabled, the module state is switched to standby. This
>> prevents keeping the module in an invalid state, as recommended by the
>> hardware team.
>>
>> Link: https://lore.kernel.org/all/20250215130849.227812-1-claudiu.beznea.uj@bp.renesas.com/
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> ---
>>
>> Changes in v2:
>> - udpated patch description to avoid plural in the configuration
>>   sequence description b/w MSTOP and CLK_ON
>> - use atomic type to store the usage counter; s/refcnt/usecnt/g
>> - moved MSTOP_OFF(), MSTOP_MASK() macros to rzg2l-cpg.c
>> - dropped struct mstp_clock::critical and use clk_hw_get_flags()
>>   instead to get the clock flags
>> - used unsigned int iterators in for loops
>> - keep memory allocated for a single list for clocks sharing the
>>   same MSTOP by updating the rzg2l_mod_clock_add_shared_mstop_clk();
>> - s/rzg2l_cpg_mstop_show/rzg2l_mod_clock_mstop_show/g,
>>   s/rzg2l_cpg_mstop/rzg2l_mod_clock_mstop/g,
>>   s/rzg2l_cpg_update_shared_mstop_clocks/rzg2l_mod_clock_update_shared_mstop_clks/g
>>    to keep the same naming conventions for functions handling mod clock MSTOP
>> - use the newly added for_each_mstp_clk() macro all over the code
> 
> Thanks for the update!
> 
>> --- a/drivers/clk/renesas/rzg2l-cpg.c
>> +++ b/drivers/clk/renesas/rzg2l-cpg.c
> 
>> @@ -1209,6 +1232,94 @@ struct mstp_clock {
>>                 else if (((hw) = __clk_get_hw((priv)->clks[(priv)->num_core_clks + i])) && \
>>                          ((mstp_clk) = to_mod_clock(hw)))
>>
>> +/* Need to be called with a lock held to avoid concurrent access to mstop->usecnt. */
>> +static void rzg2l_mod_clock_module_set_state(struct mstp_clock *clock,
>> +                                            bool standby)
>> +{
>> +       struct rzg2l_cpg_priv *priv = clock->priv;
>> +       struct mstop *mstop = clock->mstop;
>> +       bool update = false;
>> +       u32 value;
>> +
>> +       if (!mstop)
>> +               return;
>> +
>> +       value = MSTOP_MASK(mstop->conf) << 16;
>> +
>> +       if (standby) {
>> +               unsigned int criticals = 0;
>> +
>> +               for (unsigned int i = 0; i < clock->num_shared_mstop_clks; i++) {
>> +                       struct mstp_clock *clk = clock->shared_mstop_clks[i];
>> +
>> +                       if (clk_hw_get_flags(&clk->hw) & CLK_IS_CRITICAL)
>> +                               criticals++;
>> +               }
>> +
>> +               /*
>> +                * If this is a shared MSTOP and it is shared with critical clocks,
>> +                * and the system boots up with this clock enabled but no driver
>> +                * uses it the CCF will disable it (as it is unused). As we don't
>> +                * increment reference counter for it at registration (to avoid
>> +                * messing with clocks enabled at probe but later used by drivers)
>> +                * do not set the MSTOP here too if it is shared with critical
>> +                * clocks and ref counted only by those critical clocks.
>> +                */
>> +               if (criticals && criticals == atomic_read(&mstop->usecnt))
>> +                       return;
>> +
>> +               value |= MSTOP_MASK(mstop->conf);
>> +
>> +               /* Allow updates on probe when usecnt = 0. */
>> +               if (!atomic_read(&mstop->usecnt))
>> +                       update = true;
>> +               else
>> +                       update = atomic_dec_and_test(&mstop->usecnt);
>> +       } else {
>> +               atomic_inc(&mstop->usecnt);
>> +               update = true;
> 
> Shouldn't the update be conditional, i.e.:
> 
>     if (!atomic_read(&mstop->usecnt))
>             update = true;
>     atomic_inc(&mstop->usecnt);
> 
> ?

Indeed, it should be conditional as you suggested.

> 
>> +       }
>> +
>> +       if (update)
>> +               writel(value, priv->base + MSTOP_OFF(mstop->conf));
>> +}
> 
>> +static int rzg2l_mod_clock_update_shared_mstop_clks(struct rzg2l_cpg_priv *priv,
>> +                                                   struct mstp_clock *clock)
>> +{
>> +       struct mstp_clock *clk;
>> +       struct clk_hw *hw;
>> +
>> +       if (!clock->mstop)
>> +               return 0;
>> +
>> +       for_each_mstp_clk(clk, hw, priv) {
>> +               struct mstp_clock **new_clks;
>> +               int num_shared_mstop_clks;
>> +               bool found = false;
>> +
>> +               if (clk->mstop != clock->mstop)
>> +                       continue;
>> +
>> +               num_shared_mstop_clks = clk->num_shared_mstop_clks;
>> +               for (unsigned int i = 0; i < num_shared_mstop_clks; i++) {
>> +                       if (clk->shared_mstop_clks[i] == clock) {
>> +                               found = true;
>> +                               break;
>> +                       }
>> +               }
>> +               if (found)
>> +                       continue;
> 
> Can this happen? With your current code, the answer is yes.
> But I think this loop and check can be removed...
> 
>> +
>> +               if (!num_shared_mstop_clks)
>> +                       new_clks = devm_kmalloc_array(priv->dev, 2, sizeof(*new_clks), GFP_KERNEL);
>> +               else
>> +                       new_clks = devm_krealloc(priv->dev, clk->shared_mstop_clks,
>> +                                                (num_shared_mstop_clks + 1) * sizeof(*new_clks),
>> +                                                GFP_KERNEL);
>> +
>> +               if (!new_clks)
>> +                       return -ENOMEM;
>> +
>> +               if (!num_shared_mstop_clks)
>> +                       new_clks[num_shared_mstop_clks++] = clk;
>> +               if (clk != clock)
> 
> This check is always true

If I'm not wrong now, when adding the clock to it's own list, and the list
is empty (!num_shared_mstop_clks check above is true), if this condition is
missing the clock it will be added twice in its own list.

> 
>> +                       new_clks[num_shared_mstop_clks++] = clock;
>> +
>> +               for (unsigned int i = 0; i < num_shared_mstop_clks; i++) {
>> +                       new_clks[i]->shared_mstop_clks = new_clks;
>> +                       new_clks[i]->num_shared_mstop_clks = num_shared_mstop_clks;
>> +               }
> 
> ... by adding a "break" here.  The loop above has already updated the
> shared_mstop_clks[] arrays for all clocks sharing the same mstop value.

It may happen that the entries in the module clock array provided by the
SoC specific drivers to not be sorted by module clock ID. That's the case
with RZ/G2L IA55 clocks (from r9a07g044-cpg.c):

static const struct {
	struct rzg2l_mod_clk common[79];
#ifdef CONFIG_CLK_R9A07G054
	struct rzg2l_mod_clk drp[5];
#endif
} mod_clks = {
  .common = {
    // ...

    DEF_MOD("ia55_pclk", R9A07G044_IA55_PCLK, R9A07G044_CLK_P2,
			 0x518, 0, MSTOP(BUS_PERI_CPU, BIT(13))),
    DEF_MOD("ia55_clk",	R9A07G044_IA55_CLK, R9A07G044_CLK_P1,
                         0x518, 1, MSTOP(BUS_PERI_CPU, BIT(13))),

    // ...
};

Where IDs are defined as:

#define R9A07G044_IA55_CLK     8
#define R9A07G044_IA55_PCLK    9

These clocks share the same MSTOP bit.

Because the ia55_pclk is the 1st clock registered (index 9) it will be
added to priv->clks[base + 9].

Next registered clock will be for ia55_clk, with index 8, it will be added
to priv->clks[base + 8].

for_each_mstp_clk() loops on clocks from priv->clks[] array. If a break
will be done at the end of the for_each_mstp_clk() loop, at the end of the
registration each of these clocks will have on it's shared_mstop_clks[]
only references to itself.

Thank you for your review,
Claudiu

> 
>> +       }
>> +
>> +       return 0;
>> +}
> 
> 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

Re: [PATCH v2 4/8] clk: renesas: rzg2l-cpg: Add support for MSTOP in clock enable/disable API

[Geert Uytterhoeven]

Hi Claudiu,

On Fri, 23 May 2025 at 09:41, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> On 22.05.2025 17:46, Geert Uytterhoeven wrote:
> > On Wed, 14 May 2025 at 11:04, Claudiu <claudiu.beznea@tuxon.dev> wrote:
> >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>
> >> The RZ/{G2L, V2L, G3S} CPG versions support a feature called MSTOP. Each
> >> module has one or more MSTOP bits associated with it, and these bits need
> >> to be configured along with the module clocks. Setting the MSTOP bits
> >> switches the module between normal and standby states.
> >>
> >> Previously, MSTOP support was abstracted through power domains
> >> (struct generic_pm_domain::{power_on, power_off} APIs). With this
> >> abstraction, the order of setting the MSTOP and CLKON bits was as follows:
> >>
> >> Previous Order:
> >> A/ Switching to Normal State (e.g., during probe):
> >> 1/ Clear module MSTOP bit
> >> 2/ Set module CLKON bit
> >>
> >> B/ Switching to Standby State (e.g., during remove):
> >> 1/ Clear CLKON bit
> >> 2/ Set MSTOP bit
> >>
> >> However, in some cases (when the clock is disabled through devres), the
> >> order may have been (due to the issue described in link section):
> >>
> >> 1/ Set MSTOP bit
> >> 2/ Clear CLKON bit
> >>
> >> Recently, the hardware team has suggested that the correct order to set
> >> the MSTOP and CLKON bits is:
> >>
> >> Updated Order:
> >> A/ Switching to Normal State (e.g., during probe):
> >> 1/ Set CLKON bit
> >> 2/ Clear MSTOP bit
> >>
> >> B/ Switching to Standby State (e.g., during remove):
> >> 1/ Set MSTOP bit
> >> 2/ Clear CLKON bit
> >>
> >> To prevent future issues due to incorrect ordering, the MSTOP setup has
> >> now been implemented in rzg2l_mod_clock_endisable(), ensuring compliance
> >> with the sequence suggested in Figure 41.5: Module Standby Mode Procedure
> >> from the RZ/G3S HW manual, Rev1.10.
> >>
> >> Additionally, since multiple clocks of a single module may be mapped to a
> >> single MSTOP bit, MSTOP setup is reference-counted.
> >>
> >> Furthermore, as all modules start in the normal state after reset, if the
> >> module clocks are disabled, the module state is switched to standby. This
> >> prevents keeping the module in an invalid state, as recommended by the
> >> hardware team.
> >>
> >> Link: https://lore.kernel.org/all/20250215130849.227812-1-claudiu.beznea.uj@bp.renesas.com/
> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >> ---
> >>
> >> Changes in v2:
> >> - udpated patch description to avoid plural in the configuration
> >>   sequence description b/w MSTOP and CLK_ON
> >> - use atomic type to store the usage counter; s/refcnt/usecnt/g
> >> - moved MSTOP_OFF(), MSTOP_MASK() macros to rzg2l-cpg.c
> >> - dropped struct mstp_clock::critical and use clk_hw_get_flags()
> >>   instead to get the clock flags
> >> - used unsigned int iterators in for loops
> >> - keep memory allocated for a single list for clocks sharing the
> >>   same MSTOP by updating the rzg2l_mod_clock_add_shared_mstop_clk();
> >> - s/rzg2l_cpg_mstop_show/rzg2l_mod_clock_mstop_show/g,
> >>   s/rzg2l_cpg_mstop/rzg2l_mod_clock_mstop/g,
> >>   s/rzg2l_cpg_update_shared_mstop_clocks/rzg2l_mod_clock_update_shared_mstop_clks/g
> >>    to keep the same naming conventions for functions handling mod clock MSTOP
> >> - use the newly added for_each_mstp_clk() macro all over the code
> >
> > Thanks for the update!
> >
> >> --- a/drivers/clk/renesas/rzg2l-cpg.c
> >> +++ b/drivers/clk/renesas/rzg2l-cpg.c
> >
> >> @@ -1209,6 +1232,94 @@ struct mstp_clock {
> >>                 else if (((hw) = __clk_get_hw((priv)->clks[(priv)->num_core_clks + i])) && \
> >>                          ((mstp_clk) = to_mod_clock(hw)))
> >>
> >> +/* Need to be called with a lock held to avoid concurrent access to mstop->usecnt. */
> >> +static void rzg2l_mod_clock_module_set_state(struct mstp_clock *clock,
> >> +                                            bool standby)
> >> +{
> >> +       struct rzg2l_cpg_priv *priv = clock->priv;
> >> +       struct mstop *mstop = clock->mstop;
> >> +       bool update = false;
> >> +       u32 value;
> >> +
> >> +       if (!mstop)
> >> +               return;
> >> +
> >> +       value = MSTOP_MASK(mstop->conf) << 16;
> >> +
> >> +       if (standby) {
> >> +               unsigned int criticals = 0;
> >> +
> >> +               for (unsigned int i = 0; i < clock->num_shared_mstop_clks; i++) {
> >> +                       struct mstp_clock *clk = clock->shared_mstop_clks[i];
> >> +
> >> +                       if (clk_hw_get_flags(&clk->hw) & CLK_IS_CRITICAL)
> >> +                               criticals++;
> >> +               }
> >> +
> >> +               /*
> >> +                * If this is a shared MSTOP and it is shared with critical clocks,
> >> +                * and the system boots up with this clock enabled but no driver
> >> +                * uses it the CCF will disable it (as it is unused). As we don't
> >> +                * increment reference counter for it at registration (to avoid
> >> +                * messing with clocks enabled at probe but later used by drivers)
> >> +                * do not set the MSTOP here too if it is shared with critical
> >> +                * clocks and ref counted only by those critical clocks.
> >> +                */
> >> +               if (criticals && criticals == atomic_read(&mstop->usecnt))
> >> +                       return;
> >> +
> >> +               value |= MSTOP_MASK(mstop->conf);
> >> +
> >> +               /* Allow updates on probe when usecnt = 0. */
> >> +               if (!atomic_read(&mstop->usecnt))
> >> +                       update = true;
> >> +               else
> >> +                       update = atomic_dec_and_test(&mstop->usecnt);
> >> +       } else {
> >> +               atomic_inc(&mstop->usecnt);
> >> +               update = true;
> >
> > Shouldn't the update be conditional, i.e.:
> >
> >     if (!atomic_read(&mstop->usecnt))
> >             update = true;
> >     atomic_inc(&mstop->usecnt);
> >
> > ?
>
> Indeed, it should be conditional as you suggested.
>
> >
> >> +       }
> >> +
> >> +       if (update)
> >> +               writel(value, priv->base + MSTOP_OFF(mstop->conf));
> >> +}
> >
> >> +static int rzg2l_mod_clock_update_shared_mstop_clks(struct rzg2l_cpg_priv *priv,
> >> +                                                   struct mstp_clock *clock)
> >> +{
> >> +       struct mstp_clock *clk;
> >> +       struct clk_hw *hw;
> >> +
> >> +       if (!clock->mstop)
> >> +               return 0;
> >> +
> >> +       for_each_mstp_clk(clk, hw, priv) {
> >> +               struct mstp_clock **new_clks;
> >> +               int num_shared_mstop_clks;
> >> +               bool found = false;
> >> +
> >> +               if (clk->mstop != clock->mstop)
> >> +                       continue;
> >> +
> >> +               num_shared_mstop_clks = clk->num_shared_mstop_clks;
> >> +               for (unsigned int i = 0; i < num_shared_mstop_clks; i++) {
> >> +                       if (clk->shared_mstop_clks[i] == clock) {
> >> +                               found = true;
> >> +                               break;
> >> +                       }
> >> +               }
> >> +               if (found)
> >> +                       continue;
> >
> > Can this happen? With your current code, the answer is yes.
> > But I think this loop and check can be removed...
> >
> >> +
> >> +               if (!num_shared_mstop_clks)
> >> +                       new_clks = devm_kmalloc_array(priv->dev, 2, sizeof(*new_clks), GFP_KERNEL);
> >> +               else
> >> +                       new_clks = devm_krealloc(priv->dev, clk->shared_mstop_clks,
> >> +                                                (num_shared_mstop_clks + 1) * sizeof(*new_clks),
> >> +                                                GFP_KERNEL);
> >> +
> >> +               if (!new_clks)
> >> +                       return -ENOMEM;
> >> +
> >> +               if (!num_shared_mstop_clks)
> >> +                       new_clks[num_shared_mstop_clks++] = clk;
> >> +               if (clk != clock)
> >
> > This check is always true
>
> If I'm not wrong now, when adding the clock to it's own list, and the list
> is empty (!num_shared_mstop_clks check above is true), if this condition is
> missing the clock it will be added twice in its own list.

Sorry, I missed that this function is called _after_ the clock is
added to priv->clks[].  So one question and comment here:
  1. Do you need a one-entry array (actual allocation is two entries)
     for module clocks with an mstop entry that is not shared?
     Perhaps for critical clocks? That could be handled in
     rzg2l_mod_clock_module_set_state(), by explicitly checking
     the clock's own critical flag if num_shared_mstop_clks is zero.

  2. If rzg2l_mod_clock_update_shared_mstop_clks() would be called
     _before_ the clock is added to priv->clks[], the clk != clock
     check would not be needed.

> >> +                       new_clks[num_shared_mstop_clks++] = clock;
> >> +
> >> +               for (unsigned int i = 0; i < num_shared_mstop_clks; i++) {
> >> +                       new_clks[i]->shared_mstop_clks = new_clks;
> >> +                       new_clks[i]->num_shared_mstop_clks = num_shared_mstop_clks;
> >> +               }
> >
> > ... by adding a "break" here.  The loop above has already updated the
> > shared_mstop_clks[] arrays for all clocks sharing the same mstop value.
>
> It may happen that the entries in the module clock array provided by the
> SoC specific drivers to not be sorted by module clock ID. That's the case
> with RZ/G2L IA55 clocks (from r9a07g044-cpg.c):
>
> static const struct {
>         struct rzg2l_mod_clk common[79];
> #ifdef CONFIG_CLK_R9A07G054
>         struct rzg2l_mod_clk drp[5];
> #endif
> } mod_clks = {
>   .common = {
>     // ...
>
>     DEF_MOD("ia55_pclk", R9A07G044_IA55_PCLK, R9A07G044_CLK_P2,
>                          0x518, 0, MSTOP(BUS_PERI_CPU, BIT(13))),
>     DEF_MOD("ia55_clk", R9A07G044_IA55_CLK, R9A07G044_CLK_P1,
>                          0x518, 1, MSTOP(BUS_PERI_CPU, BIT(13))),
>
>     // ...
> };
>
> Where IDs are defined as:
>
> #define R9A07G044_IA55_CLK     8
> #define R9A07G044_IA55_PCLK    9
>
> These clocks share the same MSTOP bit.
>
> Because the ia55_pclk is the 1st clock registered (index 9) it will be
> added to priv->clks[base + 9].
>
> Next registered clock will be for ia55_clk, with index 8, it will be added
> to priv->clks[base + 8].
>
> for_each_mstp_clk() loops on clocks from priv->clks[] array. If a break
> will be done at the end of the for_each_mstp_clk() loop, at the end of the
> registration each of these clocks will have on it's shared_mstop_clks[]
> only references to itself.

If rzg2l_mod_clock_update_shared_mstop_clks() would be called _before_
the clock is added to priv->clks[], this issue could not happen, right?

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

Re: [PATCH v2 4/8] clk: renesas: rzg2l-cpg: Add support for MSTOP in clock enable/disable API

[Claudiu Beznea]

Hi, Geert,

On 26.05.2025 16:33, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> On Fri, 23 May 2025 at 09:41, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
>> On 22.05.2025 17:46, Geert Uytterhoeven wrote:
>>> On Wed, 14 May 2025 at 11:04, Claudiu <claudiu.beznea@tuxon.dev> wrote:
>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>
>>>> The RZ/{G2L, V2L, G3S} CPG versions support a feature called MSTOP. Each
>>>> module has one or more MSTOP bits associated with it, and these bits need
>>>> to be configured along with the module clocks. Setting the MSTOP bits
>>>> switches the module between normal and standby states.
>>>>
>>>> Previously, MSTOP support was abstracted through power domains
>>>> (struct generic_pm_domain::{power_on, power_off} APIs). With this
>>>> abstraction, the order of setting the MSTOP and CLKON bits was as follows:
>>>>
>>>> Previous Order:
>>>> A/ Switching to Normal State (e.g., during probe):
>>>> 1/ Clear module MSTOP bit
>>>> 2/ Set module CLKON bit
>>>>
>>>> B/ Switching to Standby State (e.g., during remove):
>>>> 1/ Clear CLKON bit
>>>> 2/ Set MSTOP bit
>>>>
>>>> However, in some cases (when the clock is disabled through devres), the
>>>> order may have been (due to the issue described in link section):
>>>>
>>>> 1/ Set MSTOP bit
>>>> 2/ Clear CLKON bit
>>>>
>>>> Recently, the hardware team has suggested that the correct order to set
>>>> the MSTOP and CLKON bits is:
>>>>
>>>> Updated Order:
>>>> A/ Switching to Normal State (e.g., during probe):
>>>> 1/ Set CLKON bit
>>>> 2/ Clear MSTOP bit
>>>>
>>>> B/ Switching to Standby State (e.g., during remove):
>>>> 1/ Set MSTOP bit
>>>> 2/ Clear CLKON bit
>>>>
>>>> To prevent future issues due to incorrect ordering, the MSTOP setup has
>>>> now been implemented in rzg2l_mod_clock_endisable(), ensuring compliance
>>>> with the sequence suggested in Figure 41.5: Module Standby Mode Procedure
>>>> from the RZ/G3S HW manual, Rev1.10.
>>>>
>>>> Additionally, since multiple clocks of a single module may be mapped to a
>>>> single MSTOP bit, MSTOP setup is reference-counted.
>>>>
>>>> Furthermore, as all modules start in the normal state after reset, if the
>>>> module clocks are disabled, the module state is switched to standby. This
>>>> prevents keeping the module in an invalid state, as recommended by the
>>>> hardware team.
>>>>
>>>> Link: https://lore.kernel.org/all/20250215130849.227812-1-claudiu.beznea.uj@bp.renesas.com/
>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>> ---
>>>>
>>>> Changes in v2:
>>>> - udpated patch description to avoid plural in the configuration
>>>>   sequence description b/w MSTOP and CLK_ON
>>>> - use atomic type to store the usage counter; s/refcnt/usecnt/g
>>>> - moved MSTOP_OFF(), MSTOP_MASK() macros to rzg2l-cpg.c
>>>> - dropped struct mstp_clock::critical and use clk_hw_get_flags()
>>>>   instead to get the clock flags
>>>> - used unsigned int iterators in for loops
>>>> - keep memory allocated for a single list for clocks sharing the
>>>>   same MSTOP by updating the rzg2l_mod_clock_add_shared_mstop_clk();
>>>> - s/rzg2l_cpg_mstop_show/rzg2l_mod_clock_mstop_show/g,
>>>>   s/rzg2l_cpg_mstop/rzg2l_mod_clock_mstop/g,
>>>>   s/rzg2l_cpg_update_shared_mstop_clocks/rzg2l_mod_clock_update_shared_mstop_clks/g
>>>>    to keep the same naming conventions for functions handling mod clock MSTOP
>>>> - use the newly added for_each_mstp_clk() macro all over the code
>>>
>>> Thanks for the update!
>>>
>>>> --- a/drivers/clk/renesas/rzg2l-cpg.c
>>>> +++ b/drivers/clk/renesas/rzg2l-cpg.c
>>>
>>>> @@ -1209,6 +1232,94 @@ struct mstp_clock {
>>>>                 else if (((hw) = __clk_get_hw((priv)->clks[(priv)->num_core_clks + i])) && \
>>>>                          ((mstp_clk) = to_mod_clock(hw)))
>>>>
>>>> +/* Need to be called with a lock held to avoid concurrent access to mstop->usecnt. */
>>>> +static void rzg2l_mod_clock_module_set_state(struct mstp_clock *clock,
>>>> +                                            bool standby)
>>>> +{
>>>> +       struct rzg2l_cpg_priv *priv = clock->priv;
>>>> +       struct mstop *mstop = clock->mstop;
>>>> +       bool update = false;
>>>> +       u32 value;
>>>> +
>>>> +       if (!mstop)
>>>> +               return;
>>>> +
>>>> +       value = MSTOP_MASK(mstop->conf) << 16;
>>>> +
>>>> +       if (standby) {
>>>> +               unsigned int criticals = 0;
>>>> +
>>>> +               for (unsigned int i = 0; i < clock->num_shared_mstop_clks; i++) {
>>>> +                       struct mstp_clock *clk = clock->shared_mstop_clks[i];
>>>> +
>>>> +                       if (clk_hw_get_flags(&clk->hw) & CLK_IS_CRITICAL)
>>>> +                               criticals++;
>>>> +               }
>>>> +
>>>> +               /*
>>>> +                * If this is a shared MSTOP and it is shared with critical clocks,
>>>> +                * and the system boots up with this clock enabled but no driver
>>>> +                * uses it the CCF will disable it (as it is unused). As we don't
>>>> +                * increment reference counter for it at registration (to avoid
>>>> +                * messing with clocks enabled at probe but later used by drivers)
>>>> +                * do not set the MSTOP here too if it is shared with critical
>>>> +                * clocks and ref counted only by those critical clocks.
>>>> +                */
>>>> +               if (criticals && criticals == atomic_read(&mstop->usecnt))
>>>> +                       return;
>>>> +
>>>> +               value |= MSTOP_MASK(mstop->conf);
>>>> +
>>>> +               /* Allow updates on probe when usecnt = 0. */
>>>> +               if (!atomic_read(&mstop->usecnt))
>>>> +                       update = true;
>>>> +               else
>>>> +                       update = atomic_dec_and_test(&mstop->usecnt);
>>>> +       } else {
>>>> +               atomic_inc(&mstop->usecnt);
>>>> +               update = true;
>>>
>>> Shouldn't the update be conditional, i.e.:
>>>
>>>     if (!atomic_read(&mstop->usecnt))
>>>             update = true;
>>>     atomic_inc(&mstop->usecnt);
>>>
>>> ?
>>
>> Indeed, it should be conditional as you suggested.
>>
>>>
>>>> +       }
>>>> +
>>>> +       if (update)
>>>> +               writel(value, priv->base + MSTOP_OFF(mstop->conf));
>>>> +}
>>>
>>>> +static int rzg2l_mod_clock_update_shared_mstop_clks(struct rzg2l_cpg_priv *priv,
>>>> +                                                   struct mstp_clock *clock)
>>>> +{
>>>> +       struct mstp_clock *clk;
>>>> +       struct clk_hw *hw;
>>>> +
>>>> +       if (!clock->mstop)
>>>> +               return 0;
>>>> +
>>>> +       for_each_mstp_clk(clk, hw, priv) {
>>>> +               struct mstp_clock **new_clks;
>>>> +               int num_shared_mstop_clks;
>>>> +               bool found = false;
>>>> +
>>>> +               if (clk->mstop != clock->mstop)
>>>> +                       continue;
>>>> +
>>>> +               num_shared_mstop_clks = clk->num_shared_mstop_clks;
>>>> +               for (unsigned int i = 0; i < num_shared_mstop_clks; i++) {
>>>> +                       if (clk->shared_mstop_clks[i] == clock) {
>>>> +                               found = true;
>>>> +                               break;
>>>> +                       }
>>>> +               }
>>>> +               if (found)
>>>> +                       continue;
>>>
>>> Can this happen? With your current code, the answer is yes.
>>> But I think this loop and check can be removed...
>>>
>>>> +
>>>> +               if (!num_shared_mstop_clks)
>>>> +                       new_clks = devm_kmalloc_array(priv->dev, 2, sizeof(*new_clks), GFP_KERNEL);
>>>> +               else
>>>> +                       new_clks = devm_krealloc(priv->dev, clk->shared_mstop_clks,
>>>> +                                                (num_shared_mstop_clks + 1) * sizeof(*new_clks),
>>>> +                                                GFP_KERNEL);
>>>> +
>>>> +               if (!new_clks)
>>>> +                       return -ENOMEM;
>>>> +
>>>> +               if (!num_shared_mstop_clks)
>>>> +                       new_clks[num_shared_mstop_clks++] = clk;
>>>> +               if (clk != clock)
>>>
>>> This check is always true
>>
>> If I'm not wrong now, when adding the clock to it's own list, and the list
>> is empty (!num_shared_mstop_clks check above is true), if this condition is
>> missing the clock it will be added twice in its own list.
> 
> Sorry, I missed that this function is called _after_ the clock is
> added to priv->clks[].  So one question and comment here:
>   1. Do you need a one-entry array (actual allocation is two entries)
>      for module clocks with an mstop entry that is not shared?

That extra entry should not be needed. It should not happen to have an
mstop clock in the priv->clks[] array w/o at least a clock in its shared
list. I was wrong in both the initial code and the reply I sent to your
initial comment. Appologies for that.

>      Perhaps for critical clocks? That could be handled in
>      rzg2l_mod_clock_module_set_state(), by explicitly checking
>      the clock's own critical flag if num_shared_mstop_clks is zero.
> 
>   2. If rzg2l_mod_clock_update_shared_mstop_clks() would be called
>      _before_ the clock is added to priv->clks[], the clk != clock
>      check would not be needed.

Yes, you're right. Running rzg2l_mod_clock_update_shard_mstop_clks() before
the priv->clks[] is updated simplifies the logic (see below).

> 
>>>> +                       new_clks[num_shared_mstop_clks++] = clock;
>>>> +
>>>> +               for (unsigned int i = 0; i < num_shared_mstop_clks; i++) {
>>>> +                       new_clks[i]->shared_mstop_clks = new_clks;
>>>> +                       new_clks[i]->num_shared_mstop_clks = num_shared_mstop_clks;
>>>> +               }
>>>
>>> ... by adding a "break" here.  The loop above has already updated the
>>> shared_mstop_clks[] arrays for all clocks sharing the same mstop value.
>>
>> It may happen that the entries in the module clock array provided by the
>> SoC specific drivers to not be sorted by module clock ID. That's the case
>> with RZ/G2L IA55 clocks (from r9a07g044-cpg.c):
>>
>> static const struct {
>>         struct rzg2l_mod_clk common[79];
>> #ifdef CONFIG_CLK_R9A07G054
>>         struct rzg2l_mod_clk drp[5];
>> #endif
>> } mod_clks = {
>>   .common = {
>>     // ...
>>
>>     DEF_MOD("ia55_pclk", R9A07G044_IA55_PCLK, R9A07G044_CLK_P2,
>>                          0x518, 0, MSTOP(BUS_PERI_CPU, BIT(13))),
>>     DEF_MOD("ia55_clk", R9A07G044_IA55_CLK, R9A07G044_CLK_P1,
>>                          0x518, 1, MSTOP(BUS_PERI_CPU, BIT(13))),
>>
>>     // ...
>> };
>>
>> Where IDs are defined as:
>>
>> #define R9A07G044_IA55_CLK     8
>> #define R9A07G044_IA55_PCLK    9
>>
>> These clocks share the same MSTOP bit.
>>
>> Because the ia55_pclk is the 1st clock registered (index 9) it will be
>> added to priv->clks[base + 9].
>>
>> Next registered clock will be for ia55_clk, with index 8, it will be added
>> to priv->clks[base + 8].
>>
>> for_each_mstp_clk() loops on clocks from priv->clks[] array. If a break
>> will be done at the end of the for_each_mstp_clk() loop, at the end of the
>> registration each of these clocks will have on it's shared_mstop_clks[]
>> only references to itself.
> 
> If rzg2l_mod_clock_update_shared_mstop_clks() would be called _before_
> the clock is added to priv->clks[], this issue could not happen, right?

That's true. With the above update this is not happen:

static int
rzg2l_mod_clock_update_shared_mstop_clks(struct rzg2l_cpg_priv *priv,
                                         struct mstp_clock *clock)
{
    struct mstp_clock *clk;
    struct clk_hw *hw;

    if (!clock->mstop)
        return 0;

    for_each_mstp_clk(clk, hw, priv) {
        struct mstp_clock **new_clks;
        int num_shared_mstop_clks;
        bool found = false;

        if (clk->mstop != clock->mstop)
            continue;

        num_shared_mstop_clks = clk->num_shared_mstop_clks;
        new_clks = devm_krealloc(priv->dev, clk->shared_mstop_clks,
                                 (num_shared_mstop_clks + 1) *
                                  sizeof(*new_clks),
                                 GFP_KERNEL);
        if (!new_clks)
            return -ENOMEM;

        new_clks[num_shared_mstop_clks++] = clock;

        for (unsigned int i = 0; i < num_shared_mstop_clks; i++) {
            new_clks[i]->shared_mstop_clks = new_clks;
            new_clks[i]->num_shared_mstop_clks = num_shared_mstop_clks;
        }
        break;
    }

    if (clock->num_shared_mstop_clks)
        return 0;

    clock->shared_mstop_clks = devm_kzalloc(priv->dev,
                                       sizeof(*clock->shared_mstop_clks),
                                       GFP_KERNEL);
    if (!clock->shared_mstop_clks)
        return -ENOMEM;

    clock->shared_mstop_clks[0] = clock;
    clock->num_shared_mstop_clks = 1;

    return 0;
}


Thank you for your review,
Claudiu

Re: [PATCH v2 4/8] clk: renesas: rzg2l-cpg: Add support for MSTOP in clock enable/disable API

[Geert Uytterhoeven]

Hi Claudiu,

On Mon, 26 May 2025 at 17:55, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> On 26.05.2025 16:33, Geert Uytterhoeven wrote:
> > On Fri, 23 May 2025 at 09:41, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> >> On 22.05.2025 17:46, Geert Uytterhoeven wrote:
> >>> On Wed, 14 May 2025 at 11:04, Claudiu <claudiu.beznea@tuxon.dev> wrote:
> >>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>>>
> >>>> The RZ/{G2L, V2L, G3S} CPG versions support a feature called MSTOP. Each
> >>>> module has one or more MSTOP bits associated with it, and these bits need
> >>>> to be configured along with the module clocks. Setting the MSTOP bits
> >>>> switches the module between normal and standby states.
> >>>>
> >>>> Previously, MSTOP support was abstracted through power domains
> >>>> (struct generic_pm_domain::{power_on, power_off} APIs). With this
> >>>> abstraction, the order of setting the MSTOP and CLKON bits was as follows:
> >>>>
> >>>> Previous Order:
> >>>> A/ Switching to Normal State (e.g., during probe):
> >>>> 1/ Clear module MSTOP bit
> >>>> 2/ Set module CLKON bit
> >>>>
> >>>> B/ Switching to Standby State (e.g., during remove):
> >>>> 1/ Clear CLKON bit
> >>>> 2/ Set MSTOP bit
> >>>>
> >>>> However, in some cases (when the clock is disabled through devres), the
> >>>> order may have been (due to the issue described in link section):
> >>>>
> >>>> 1/ Set MSTOP bit
> >>>> 2/ Clear CLKON bit
> >>>>
> >>>> Recently, the hardware team has suggested that the correct order to set
> >>>> the MSTOP and CLKON bits is:
> >>>>
> >>>> Updated Order:
> >>>> A/ Switching to Normal State (e.g., during probe):
> >>>> 1/ Set CLKON bit
> >>>> 2/ Clear MSTOP bit
> >>>>
> >>>> B/ Switching to Standby State (e.g., during remove):
> >>>> 1/ Set MSTOP bit
> >>>> 2/ Clear CLKON bit
> >>>>
> >>>> To prevent future issues due to incorrect ordering, the MSTOP setup has
> >>>> now been implemented in rzg2l_mod_clock_endisable(), ensuring compliance
> >>>> with the sequence suggested in Figure 41.5: Module Standby Mode Procedure
> >>>> from the RZ/G3S HW manual, Rev1.10.
> >>>>
> >>>> Additionally, since multiple clocks of a single module may be mapped to a
> >>>> single MSTOP bit, MSTOP setup is reference-counted.
> >>>>
> >>>> Furthermore, as all modules start in the normal state after reset, if the
> >>>> module clocks are disabled, the module state is switched to standby. This
> >>>> prevents keeping the module in an invalid state, as recommended by the
> >>>> hardware team.
> >>>>
> >>>> Link: https://lore.kernel.org/all/20250215130849.227812-1-claudiu.beznea.uj@bp.renesas.com/
> >>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>>> ---
> >>>>
> >>>> Changes in v2:
> >>>> - udpated patch description to avoid plural in the configuration
> >>>>   sequence description b/w MSTOP and CLK_ON
> >>>> - use atomic type to store the usage counter; s/refcnt/usecnt/g
> >>>> - moved MSTOP_OFF(), MSTOP_MASK() macros to rzg2l-cpg.c
> >>>> - dropped struct mstp_clock::critical and use clk_hw_get_flags()
> >>>>   instead to get the clock flags
> >>>> - used unsigned int iterators in for loops
> >>>> - keep memory allocated for a single list for clocks sharing the
> >>>>   same MSTOP by updating the rzg2l_mod_clock_add_shared_mstop_clk();
> >>>> - s/rzg2l_cpg_mstop_show/rzg2l_mod_clock_mstop_show/g,
> >>>>   s/rzg2l_cpg_mstop/rzg2l_mod_clock_mstop/g,
> >>>>   s/rzg2l_cpg_update_shared_mstop_clocks/rzg2l_mod_clock_update_shared_mstop_clks/g
> >>>>    to keep the same naming conventions for functions handling mod clock MSTOP
> >>>> - use the newly added for_each_mstp_clk() macro all over the code
> >>>
> >>> Thanks for the update!
> >>>
> >>>> --- a/drivers/clk/renesas/rzg2l-cpg.c
> >>>> +++ b/drivers/clk/renesas/rzg2l-cpg.c
> >>>
> >>>> @@ -1209,6 +1232,94 @@ struct mstp_clock {
> >>>>                 else if (((hw) = __clk_get_hw((priv)->clks[(priv)->num_core_clks + i])) && \
> >>>>                          ((mstp_clk) = to_mod_clock(hw)))
> >>>>
> >>>> +/* Need to be called with a lock held to avoid concurrent access to mstop->usecnt. */
> >>>> +static void rzg2l_mod_clock_module_set_state(struct mstp_clock *clock,
> >>>> +                                            bool standby)
> >>>> +{
> >>>> +       struct rzg2l_cpg_priv *priv = clock->priv;
> >>>> +       struct mstop *mstop = clock->mstop;
> >>>> +       bool update = false;
> >>>> +       u32 value;
> >>>> +
> >>>> +       if (!mstop)
> >>>> +               return;
> >>>> +
> >>>> +       value = MSTOP_MASK(mstop->conf) << 16;
> >>>> +
> >>>> +       if (standby) {
> >>>> +               unsigned int criticals = 0;
> >>>> +
> >>>> +               for (unsigned int i = 0; i < clock->num_shared_mstop_clks; i++) {
> >>>> +                       struct mstp_clock *clk = clock->shared_mstop_clks[i];
> >>>> +
> >>>> +                       if (clk_hw_get_flags(&clk->hw) & CLK_IS_CRITICAL)
> >>>> +                               criticals++;
> >>>> +               }
> >>>> +
> >>>> +               /*
> >>>> +                * If this is a shared MSTOP and it is shared with critical clocks,
> >>>> +                * and the system boots up with this clock enabled but no driver
> >>>> +                * uses it the CCF will disable it (as it is unused). As we don't
> >>>> +                * increment reference counter for it at registration (to avoid
> >>>> +                * messing with clocks enabled at probe but later used by drivers)
> >>>> +                * do not set the MSTOP here too if it is shared with critical
> >>>> +                * clocks and ref counted only by those critical clocks.
> >>>> +                */
> >>>> +               if (criticals && criticals == atomic_read(&mstop->usecnt))
> >>>> +                       return;
> >>>> +
> >>>> +               value |= MSTOP_MASK(mstop->conf);
> >>>> +
> >>>> +               /* Allow updates on probe when usecnt = 0. */
> >>>> +               if (!atomic_read(&mstop->usecnt))
> >>>> +                       update = true;
> >>>> +               else
> >>>> +                       update = atomic_dec_and_test(&mstop->usecnt);
> >>>> +       } else {
> >>>> +               atomic_inc(&mstop->usecnt);
> >>>> +               update = true;
> >>>
> >>> Shouldn't the update be conditional, i.e.:
> >>>
> >>>     if (!atomic_read(&mstop->usecnt))
> >>>             update = true;
> >>>     atomic_inc(&mstop->usecnt);
> >>>
> >>> ?
> >>
> >> Indeed, it should be conditional as you suggested.
> >>
> >>>
> >>>> +       }
> >>>> +
> >>>> +       if (update)
> >>>> +               writel(value, priv->base + MSTOP_OFF(mstop->conf));
> >>>> +}
> >>>
> >>>> +static int rzg2l_mod_clock_update_shared_mstop_clks(struct rzg2l_cpg_priv *priv,
> >>>> +                                                   struct mstp_clock *clock)
> >>>> +{
> >>>> +       struct mstp_clock *clk;
> >>>> +       struct clk_hw *hw;
> >>>> +
> >>>> +       if (!clock->mstop)
> >>>> +               return 0;
> >>>> +
> >>>> +       for_each_mstp_clk(clk, hw, priv) {
> >>>> +               struct mstp_clock **new_clks;
> >>>> +               int num_shared_mstop_clks;
> >>>> +               bool found = false;
> >>>> +
> >>>> +               if (clk->mstop != clock->mstop)
> >>>> +                       continue;
> >>>> +
> >>>> +               num_shared_mstop_clks = clk->num_shared_mstop_clks;
> >>>> +               for (unsigned int i = 0; i < num_shared_mstop_clks; i++) {
> >>>> +                       if (clk->shared_mstop_clks[i] == clock) {
> >>>> +                               found = true;
> >>>> +                               break;
> >>>> +                       }
> >>>> +               }
> >>>> +               if (found)
> >>>> +                       continue;
> >>>
> >>> Can this happen? With your current code, the answer is yes.
> >>> But I think this loop and check can be removed...
> >>>
> >>>> +
> >>>> +               if (!num_shared_mstop_clks)
> >>>> +                       new_clks = devm_kmalloc_array(priv->dev, 2, sizeof(*new_clks), GFP_KERNEL);
> >>>> +               else
> >>>> +                       new_clks = devm_krealloc(priv->dev, clk->shared_mstop_clks,
> >>>> +                                                (num_shared_mstop_clks + 1) * sizeof(*new_clks),
> >>>> +                                                GFP_KERNEL);
> >>>> +
> >>>> +               if (!new_clks)
> >>>> +                       return -ENOMEM;
> >>>> +
> >>>> +               if (!num_shared_mstop_clks)
> >>>> +                       new_clks[num_shared_mstop_clks++] = clk;
> >>>> +               if (clk != clock)
> >>>
> >>> This check is always true
> >>
> >> If I'm not wrong now, when adding the clock to it's own list, and the list
> >> is empty (!num_shared_mstop_clks check above is true), if this condition is
> >> missing the clock it will be added twice in its own list.
> >
> > Sorry, I missed that this function is called _after_ the clock is
> > added to priv->clks[].  So one question and comment here:
> >   1. Do you need a one-entry array (actual allocation is two entries)
> >      for module clocks with an mstop entry that is not shared?
>
> That extra entry should not be needed. It should not happen to have an
> mstop clock in the priv->clks[] array w/o at least a clock in its shared
> list. I was wrong in both the initial code and the reply I sent to your
> initial comment. Appologies for that.

So no single-entry arrays...

> >      Perhaps for critical clocks? That could be handled in
> >      rzg2l_mod_clock_module_set_state(), by explicitly checking
> >      the clock's own critical flag if num_shared_mstop_clks is zero.
> >
> >   2. If rzg2l_mod_clock_update_shared_mstop_clks() would be called
> >      _before_ the clock is added to priv->clks[], the clk != clock
> >      check would not be needed.
>
> Yes, you're right. Running rzg2l_mod_clock_update_shard_mstop_clks() before
> the priv->clks[] is updated simplifies the logic (see below).
>
> >
> >>>> +                       new_clks[num_shared_mstop_clks++] = clock;
> >>>> +
> >>>> +               for (unsigned int i = 0; i < num_shared_mstop_clks; i++) {
> >>>> +                       new_clks[i]->shared_mstop_clks = new_clks;
> >>>> +                       new_clks[i]->num_shared_mstop_clks = num_shared_mstop_clks;
> >>>> +               }
> >>>
> >>> ... by adding a "break" here.  The loop above has already updated the
> >>> shared_mstop_clks[] arrays for all clocks sharing the same mstop value.
> >>
> >> It may happen that the entries in the module clock array provided by the
> >> SoC specific drivers to not be sorted by module clock ID. That's the case
> >> with RZ/G2L IA55 clocks (from r9a07g044-cpg.c):
> >>
> >> static const struct {
> >>         struct rzg2l_mod_clk common[79];
> >> #ifdef CONFIG_CLK_R9A07G054
> >>         struct rzg2l_mod_clk drp[5];
> >> #endif
> >> } mod_clks = {
> >>   .common = {
> >>     // ...
> >>
> >>     DEF_MOD("ia55_pclk", R9A07G044_IA55_PCLK, R9A07G044_CLK_P2,
> >>                          0x518, 0, MSTOP(BUS_PERI_CPU, BIT(13))),
> >>     DEF_MOD("ia55_clk", R9A07G044_IA55_CLK, R9A07G044_CLK_P1,
> >>                          0x518, 1, MSTOP(BUS_PERI_CPU, BIT(13))),
> >>
> >>     // ...
> >> };
> >>
> >> Where IDs are defined as:
> >>
> >> #define R9A07G044_IA55_CLK     8
> >> #define R9A07G044_IA55_PCLK    9
> >>
> >> These clocks share the same MSTOP bit.
> >>
> >> Because the ia55_pclk is the 1st clock registered (index 9) it will be
> >> added to priv->clks[base + 9].
> >>
> >> Next registered clock will be for ia55_clk, with index 8, it will be added
> >> to priv->clks[base + 8].
> >>
> >> for_each_mstp_clk() loops on clocks from priv->clks[] array. If a break
> >> will be done at the end of the for_each_mstp_clk() loop, at the end of the
> >> registration each of these clocks will have on it's shared_mstop_clks[]
> >> only references to itself.
> >
> > If rzg2l_mod_clock_update_shared_mstop_clks() would be called _before_
> > the clock is added to priv->clks[], this issue could not happen, right?
>
> That's true. With the above update this is not happen:
>
> static int
> rzg2l_mod_clock_update_shared_mstop_clks(struct rzg2l_cpg_priv *priv,
>                                          struct mstp_clock *clock)
> {
>     struct mstp_clock *clk;
>     struct clk_hw *hw;
>
>     if (!clock->mstop)
>         return 0;
>
>     for_each_mstp_clk(clk, hw, priv) {
>         struct mstp_clock **new_clks;
>         int num_shared_mstop_clks;
>         bool found = false;
>
>         if (clk->mstop != clock->mstop)
>             continue;
>
>         num_shared_mstop_clks = clk->num_shared_mstop_clks;
>         new_clks = devm_krealloc(priv->dev, clk->shared_mstop_clks,
>                                  (num_shared_mstop_clks + 1) *
>                                   sizeof(*new_clks),
>                                  GFP_KERNEL);
>         if (!new_clks)
>             return -ENOMEM;
>
>         new_clks[num_shared_mstop_clks++] = clock;
>
>         for (unsigned int i = 0; i < num_shared_mstop_clks; i++) {
>             new_clks[i]->shared_mstop_clks = new_clks;
>             new_clks[i]->num_shared_mstop_clks = num_shared_mstop_clks;
>         }
>         break;
>     }
>
>     if (clock->num_shared_mstop_clks)
>         return 0;
>
>     clock->shared_mstop_clks = devm_kzalloc(priv->dev,
>                                        sizeof(*clock->shared_mstop_clks),
>                                        GFP_KERNEL);
>     if (!clock->shared_mstop_clks)
>         return -ENOMEM;
>
>     clock->shared_mstop_clks[0] = clock;
>     clock->num_shared_mstop_clks = 1;

... but here you still create a single-entry array?

>
>     return 0;
> }

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

Re: [PATCH v2 4/8] clk: renesas: rzg2l-cpg: Add support for MSTOP in clock enable/disable API

[Claudiu Beznea]

Hi, Geert,

On 26.05.2025 20:09, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> On Mon, 26 May 2025 at 17:55, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
>> On 26.05.2025 16:33, Geert Uytterhoeven wrote:
>>> On Fri, 23 May 2025 at 09:41, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
>>>> On 22.05.2025 17:46, Geert Uytterhoeven wrote:
>>>>> On Wed, 14 May 2025 at 11:04, Claudiu <claudiu.beznea@tuxon.dev> wrote:
>>>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>>>
>>>>>> The RZ/{G2L, V2L, G3S} CPG versions support a feature called MSTOP. Each
>>>>>> module has one or more MSTOP bits associated with it, and these bits need
>>>>>> to be configured along with the module clocks. Setting the MSTOP bits
>>>>>> switches the module between normal and standby states.
>>>>>>
>>>>>> Previously, MSTOP support was abstracted through power domains
>>>>>> (struct generic_pm_domain::{power_on, power_off} APIs). With this
>>>>>> abstraction, the order of setting the MSTOP and CLKON bits was as follows:
>>>>>>
>>>>>> Previous Order:
>>>>>> A/ Switching to Normal State (e.g., during probe):
>>>>>> 1/ Clear module MSTOP bit
>>>>>> 2/ Set module CLKON bit
>>>>>>
>>>>>> B/ Switching to Standby State (e.g., during remove):
>>>>>> 1/ Clear CLKON bit
>>>>>> 2/ Set MSTOP bit
>>>>>>
>>>>>> However, in some cases (when the clock is disabled through devres), the
>>>>>> order may have been (due to the issue described in link section):
>>>>>>
>>>>>> 1/ Set MSTOP bit
>>>>>> 2/ Clear CLKON bit
>>>>>>
>>>>>> Recently, the hardware team has suggested that the correct order to set
>>>>>> the MSTOP and CLKON bits is:
>>>>>>
>>>>>> Updated Order:
>>>>>> A/ Switching to Normal State (e.g., during probe):
>>>>>> 1/ Set CLKON bit
>>>>>> 2/ Clear MSTOP bit
>>>>>>
>>>>>> B/ Switching to Standby State (e.g., during remove):
>>>>>> 1/ Set MSTOP bit
>>>>>> 2/ Clear CLKON bit
>>>>>>
>>>>>> To prevent future issues due to incorrect ordering, the MSTOP setup has
>>>>>> now been implemented in rzg2l_mod_clock_endisable(), ensuring compliance
>>>>>> with the sequence suggested in Figure 41.5: Module Standby Mode Procedure
>>>>>> from the RZ/G3S HW manual, Rev1.10.
>>>>>>
>>>>>> Additionally, since multiple clocks of a single module may be mapped to a
>>>>>> single MSTOP bit, MSTOP setup is reference-counted.
>>>>>>
>>>>>> Furthermore, as all modules start in the normal state after reset, if the
>>>>>> module clocks are disabled, the module state is switched to standby. This
>>>>>> prevents keeping the module in an invalid state, as recommended by the
>>>>>> hardware team.
>>>>>>
>>>>>> Link: https://lore.kernel.org/all/20250215130849.227812-1-claudiu.beznea.uj@bp.renesas.com/
>>>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>>> ---
>>>>>>
>>>>>> Changes in v2:
>>>>>> - udpated patch description to avoid plural in the configuration
>>>>>>   sequence description b/w MSTOP and CLK_ON
>>>>>> - use atomic type to store the usage counter; s/refcnt/usecnt/g
>>>>>> - moved MSTOP_OFF(), MSTOP_MASK() macros to rzg2l-cpg.c
>>>>>> - dropped struct mstp_clock::critical and use clk_hw_get_flags()
>>>>>>   instead to get the clock flags
>>>>>> - used unsigned int iterators in for loops
>>>>>> - keep memory allocated for a single list for clocks sharing the
>>>>>>   same MSTOP by updating the rzg2l_mod_clock_add_shared_mstop_clk();
>>>>>> - s/rzg2l_cpg_mstop_show/rzg2l_mod_clock_mstop_show/g,
>>>>>>   s/rzg2l_cpg_mstop/rzg2l_mod_clock_mstop/g,
>>>>>>   s/rzg2l_cpg_update_shared_mstop_clocks/rzg2l_mod_clock_update_shared_mstop_clks/g
>>>>>>    to keep the same naming conventions for functions handling mod clock MSTOP
>>>>>> - use the newly added for_each_mstp_clk() macro all over the code
>>>>>
>>>>> Thanks for the update!
>>>>>
>>>>>> --- a/drivers/clk/renesas/rzg2l-cpg.c
>>>>>> +++ b/drivers/clk/renesas/rzg2l-cpg.c
>>>>>
>>>>>> @@ -1209,6 +1232,94 @@ struct mstp_clock {
>>>>>>                 else if (((hw) = __clk_get_hw((priv)->clks[(priv)->num_core_clks + i])) && \
>>>>>>                          ((mstp_clk) = to_mod_clock(hw)))
>>>>>>
>>>>>> +/* Need to be called with a lock held to avoid concurrent access to mstop->usecnt. */
>>>>>> +static void rzg2l_mod_clock_module_set_state(struct mstp_clock *clock,
>>>>>> +                                            bool standby)
>>>>>> +{
>>>>>> +       struct rzg2l_cpg_priv *priv = clock->priv;
>>>>>> +       struct mstop *mstop = clock->mstop;
>>>>>> +       bool update = false;
>>>>>> +       u32 value;
>>>>>> +
>>>>>> +       if (!mstop)
>>>>>> +               return;
>>>>>> +
>>>>>> +       value = MSTOP_MASK(mstop->conf) << 16;
>>>>>> +
>>>>>> +       if (standby) {
>>>>>> +               unsigned int criticals = 0;
>>>>>> +
>>>>>> +               for (unsigned int i = 0; i < clock->num_shared_mstop_clks; i++) {
>>>>>> +                       struct mstp_clock *clk = clock->shared_mstop_clks[i];
>>>>>> +
>>>>>> +                       if (clk_hw_get_flags(&clk->hw) & CLK_IS_CRITICAL)
>>>>>> +                               criticals++;
>>>>>> +               }
>>>>>> +
>>>>>> +               /*
>>>>>> +                * If this is a shared MSTOP and it is shared with critical clocks,
>>>>>> +                * and the system boots up with this clock enabled but no driver
>>>>>> +                * uses it the CCF will disable it (as it is unused). As we don't
>>>>>> +                * increment reference counter for it at registration (to avoid
>>>>>> +                * messing with clocks enabled at probe but later used by drivers)
>>>>>> +                * do not set the MSTOP here too if it is shared with critical
>>>>>> +                * clocks and ref counted only by those critical clocks.
>>>>>> +                */
>>>>>> +               if (criticals && criticals == atomic_read(&mstop->usecnt))
>>>>>> +                       return;
>>>>>> +
>>>>>> +               value |= MSTOP_MASK(mstop->conf);
>>>>>> +
>>>>>> +               /* Allow updates on probe when usecnt = 0. */
>>>>>> +               if (!atomic_read(&mstop->usecnt))
>>>>>> +                       update = true;
>>>>>> +               else
>>>>>> +                       update = atomic_dec_and_test(&mstop->usecnt);
>>>>>> +       } else {
>>>>>> +               atomic_inc(&mstop->usecnt);
>>>>>> +               update = true;
>>>>>
>>>>> Shouldn't the update be conditional, i.e.:
>>>>>
>>>>>     if (!atomic_read(&mstop->usecnt))
>>>>>             update = true;
>>>>>     atomic_inc(&mstop->usecnt);
>>>>>
>>>>> ?
>>>>
>>>> Indeed, it should be conditional as you suggested.
>>>>
>>>>>
>>>>>> +       }
>>>>>> +
>>>>>> +       if (update)
>>>>>> +               writel(value, priv->base + MSTOP_OFF(mstop->conf));
>>>>>> +}
>>>>>
>>>>>> +static int rzg2l_mod_clock_update_shared_mstop_clks(struct rzg2l_cpg_priv *priv,
>>>>>> +                                                   struct mstp_clock *clock)
>>>>>> +{
>>>>>> +       struct mstp_clock *clk;
>>>>>> +       struct clk_hw *hw;
>>>>>> +
>>>>>> +       if (!clock->mstop)
>>>>>> +               return 0;
>>>>>> +
>>>>>> +       for_each_mstp_clk(clk, hw, priv) {
>>>>>> +               struct mstp_clock **new_clks;
>>>>>> +               int num_shared_mstop_clks;
>>>>>> +               bool found = false;
>>>>>> +
>>>>>> +               if (clk->mstop != clock->mstop)
>>>>>> +                       continue;
>>>>>> +
>>>>>> +               num_shared_mstop_clks = clk->num_shared_mstop_clks;
>>>>>> +               for (unsigned int i = 0; i < num_shared_mstop_clks; i++) {
>>>>>> +                       if (clk->shared_mstop_clks[i] == clock) {
>>>>>> +                               found = true;
>>>>>> +                               break;
>>>>>> +                       }
>>>>>> +               }
>>>>>> +               if (found)
>>>>>> +                       continue;
>>>>>
>>>>> Can this happen? With your current code, the answer is yes.
>>>>> But I think this loop and check can be removed...
>>>>>
>>>>>> +
>>>>>> +               if (!num_shared_mstop_clks)
>>>>>> +                       new_clks = devm_kmalloc_array(priv->dev, 2, sizeof(*new_clks), GFP_KERNEL);
>>>>>> +               else
>>>>>> +                       new_clks = devm_krealloc(priv->dev, clk->shared_mstop_clks,
>>>>>> +                                                (num_shared_mstop_clks + 1) * sizeof(*new_clks),
>>>>>> +                                                GFP_KERNEL);
>>>>>> +
>>>>>> +               if (!new_clks)
>>>>>> +                       return -ENOMEM;
>>>>>> +
>>>>>> +               if (!num_shared_mstop_clks)
>>>>>> +                       new_clks[num_shared_mstop_clks++] = clk;
>>>>>> +               if (clk != clock)
>>>>>
>>>>> This check is always true
>>>>
>>>> If I'm not wrong now, when adding the clock to it's own list, and the list
>>>> is empty (!num_shared_mstop_clks check above is true), if this condition is
>>>> missing the clock it will be added twice in its own list.
>>>
>>> Sorry, I missed that this function is called _after_ the clock is
>>> added to priv->clks[].  So one question and comment here:
>>>   1. Do you need a one-entry array (actual allocation is two entries)
>>>      for module clocks with an mstop entry that is not shared?
>>
>> That extra entry should not be needed. It should not happen to have an
>> mstop clock in the priv->clks[] array w/o at least a clock in its shared
>> list. I was wrong in both the initial code and the reply I sent to your
>> initial comment. Appologies for that.
> 
> So no single-entry arrays...

Oh, I missread it yesterday everning. Sorry for confusion. Let me try again:

> 
>>>      Perhaps for critical clocks? That could be handled in

The clock is added to its own shared_mstop_clk[] array to avoid extra
conditions when all the clocks sharing the same mstop need to be checked
for an action. One example is code at [A] (for critical clocks) that was
available in v1.


>>>      rzg2l_mod_clock_module_set_state(), by explicitly checking
>>>      the clock's own critical flag if num_shared_mstop_clks is zero.

The clock was added to its own shared_mstop_clk[] array as a result of this
comment I got from you on v1 (with regards to checking the clock's critical
flag):

"If clock->shared_mstop_clks[] would include the current clock, then
(a) this test would not be needed, and
(b) all clocks sharing the same mstop could share a single
    clock->shared_mstop_clks[] array."

If I understood correctly, in [1] it has been proposed to have something
like what is proposed here to avoid extra conditional check (like [A]), in
rzg2l_mod_clock_module_set_state():

for (u8 i = 0; i < clock->num_shared_mstop_clks; i++) {
    unsigned int
    struct mstp_clock *clk = clock->shared_mstop_clks[i];

    if (clk->critical)
        criticals++;
}

/* Increment if clock is critical, too. */
if (clock->critical) // <<< [A]
    criticals++;

Please let me know if I misunderstood your initial request?

[1]
https://lore.kernel.org/all/CAMuHMdXFtBmjDu=1RS2MLNYzhZ0fmpT7+1QbA9p4LvoLHitOuw@mail.gmail.com/


>>>
>>>   2. If rzg2l_mod_clock_update_shared_mstop_clks() would be called
>>>      _before_ the clock is added to priv->clks[], the clk != clock
>>>      check would not be needed.
>>
>> Yes, you're right. Running rzg2l_mod_clock_update_shard_mstop_clks() before
>> the priv->clks[] is updated simplifies the logic (see below).
>>
>>>
>>>>>> +                       new_clks[num_shared_mstop_clks++] = clock;
>>>>>> +
>>>>>> +               for (unsigned int i = 0; i < num_shared_mstop_clks; i++) {
>>>>>> +                       new_clks[i]->shared_mstop_clks = new_clks;
>>>>>> +                       new_clks[i]->num_shared_mstop_clks = num_shared_mstop_clks;
>>>>>> +               }
>>>>>
>>>>> ... by adding a "break" here.  The loop above has already updated the
>>>>> shared_mstop_clks[] arrays for all clocks sharing the same mstop value.
>>>>
>>>> It may happen that the entries in the module clock array provided by the
>>>> SoC specific drivers to not be sorted by module clock ID. That's the case
>>>> with RZ/G2L IA55 clocks (from r9a07g044-cpg.c):
>>>>
>>>> static const struct {
>>>>         struct rzg2l_mod_clk common[79];
>>>> #ifdef CONFIG_CLK_R9A07G054
>>>>         struct rzg2l_mod_clk drp[5];
>>>> #endif
>>>> } mod_clks = {
>>>>   .common = {
>>>>     // ...
>>>>
>>>>     DEF_MOD("ia55_pclk", R9A07G044_IA55_PCLK, R9A07G044_CLK_P2,
>>>>                          0x518, 0, MSTOP(BUS_PERI_CPU, BIT(13))),
>>>>     DEF_MOD("ia55_clk", R9A07G044_IA55_CLK, R9A07G044_CLK_P1,
>>>>                          0x518, 1, MSTOP(BUS_PERI_CPU, BIT(13))),
>>>>
>>>>     // ...
>>>> };
>>>>
>>>> Where IDs are defined as:
>>>>
>>>> #define R9A07G044_IA55_CLK     8
>>>> #define R9A07G044_IA55_PCLK    9
>>>>
>>>> These clocks share the same MSTOP bit.
>>>>
>>>> Because the ia55_pclk is the 1st clock registered (index 9) it will be
>>>> added to priv->clks[base + 9].
>>>>
>>>> Next registered clock will be for ia55_clk, with index 8, it will be added
>>>> to priv->clks[base + 8].
>>>>
>>>> for_each_mstp_clk() loops on clocks from priv->clks[] array. If a break
>>>> will be done at the end of the for_each_mstp_clk() loop, at the end of the
>>>> registration each of these clocks will have on it's shared_mstop_clks[]
>>>> only references to itself.
>>>
>>> If rzg2l_mod_clock_update_shared_mstop_clks() would be called _before_
>>> the clock is added to priv->clks[], this issue could not happen, right?
>>
>> That's true. With the above update this is not happen:
>>
>> static int
>> rzg2l_mod_clock_update_shared_mstop_clks(struct rzg2l_cpg_priv *priv,
>>                                          struct mstp_clock *clock)
>> {
>>     struct mstp_clock *clk;
>>     struct clk_hw *hw;
>>
>>     if (!clock->mstop)
>>         return 0;
>>
>>     for_each_mstp_clk(clk, hw, priv) {
>>         struct mstp_clock **new_clks;
>>         int num_shared_mstop_clks;
>>         bool found = false;
>>
>>         if (clk->mstop != clock->mstop)
>>             continue;
>>
>>         num_shared_mstop_clks = clk->num_shared_mstop_clks;
>>         new_clks = devm_krealloc(priv->dev, clk->shared_mstop_clks,
>>                                  (num_shared_mstop_clks + 1) *
>>                                   sizeof(*new_clks),
>>                                  GFP_KERNEL);
>>         if (!new_clks)
>>             return -ENOMEM;
>>
>>         new_clks[num_shared_mstop_clks++] = clock;
>>
>>         for (unsigned int i = 0; i < num_shared_mstop_clks; i++) {
>>             new_clks[i]->shared_mstop_clks = new_clks;
>>             new_clks[i]->num_shared_mstop_clks = num_shared_mstop_clks;
>>         }
>>         break;
>>     }
>>
>>     if (clock->num_shared_mstop_clks)
>>         return 0;
>>
>>     clock->shared_mstop_clks = devm_kzalloc(priv->dev,
>>                                        sizeof(*clock->shared_mstop_clks),
>>                                        GFP_KERNEL);
>>     if (!clock->shared_mstop_clks)
>>         return -ENOMEM;
>>
>>     clock->shared_mstop_clks[0] = clock;
>>     clock->num_shared_mstop_clks = 1;
> 
> ... but here you still create a single-entry array?\

For the above mentioned reason.

Thank you,
Claudiu

Re: [PATCH v2 4/8] clk: renesas: rzg2l-cpg: Add support for MSTOP in clock enable/disable API

[Geert Uytterhoeven]

Hi Claudiu,

On Tue, 27 May 2025 at 10:58, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> On 26.05.2025 20:09, Geert Uytterhoeven wrote:
> > On Mon, 26 May 2025 at 17:55, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> >> On 26.05.2025 16:33, Geert Uytterhoeven wrote:
> >>> On Fri, 23 May 2025 at 09:41, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> >>>> On 22.05.2025 17:46, Geert Uytterhoeven wrote:
> >>>>> On Wed, 14 May 2025 at 11:04, Claudiu <claudiu.beznea@tuxon.dev> wrote:
> >>>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>>>>>
> >>>>>> The RZ/{G2L, V2L, G3S} CPG versions support a feature called MSTOP. Each
> >>>>>> module has one or more MSTOP bits associated with it, and these bits need
> >>>>>> to be configured along with the module clocks. Setting the MSTOP bits
> >>>>>> switches the module between normal and standby states.
> >>>>>>
> >>>>>> Previously, MSTOP support was abstracted through power domains
> >>>>>> (struct generic_pm_domain::{power_on, power_off} APIs). With this
> >>>>>> abstraction, the order of setting the MSTOP and CLKON bits was as follows:
> >>>>>>
> >>>>>> Previous Order:
> >>>>>> A/ Switching to Normal State (e.g., during probe):
> >>>>>> 1/ Clear module MSTOP bit
> >>>>>> 2/ Set module CLKON bit
> >>>>>>
> >>>>>> B/ Switching to Standby State (e.g., during remove):
> >>>>>> 1/ Clear CLKON bit
> >>>>>> 2/ Set MSTOP bit
> >>>>>>
> >>>>>> However, in some cases (when the clock is disabled through devres), the
> >>>>>> order may have been (due to the issue described in link section):
> >>>>>>
> >>>>>> 1/ Set MSTOP bit
> >>>>>> 2/ Clear CLKON bit
> >>>>>>
> >>>>>> Recently, the hardware team has suggested that the correct order to set
> >>>>>> the MSTOP and CLKON bits is:
> >>>>>>
> >>>>>> Updated Order:
> >>>>>> A/ Switching to Normal State (e.g., during probe):
> >>>>>> 1/ Set CLKON bit
> >>>>>> 2/ Clear MSTOP bit
> >>>>>>
> >>>>>> B/ Switching to Standby State (e.g., during remove):
> >>>>>> 1/ Set MSTOP bit
> >>>>>> 2/ Clear CLKON bit
> >>>>>>
> >>>>>> To prevent future issues due to incorrect ordering, the MSTOP setup has
> >>>>>> now been implemented in rzg2l_mod_clock_endisable(), ensuring compliance
> >>>>>> with the sequence suggested in Figure 41.5: Module Standby Mode Procedure
> >>>>>> from the RZ/G3S HW manual, Rev1.10.
> >>>>>>
> >>>>>> Additionally, since multiple clocks of a single module may be mapped to a
> >>>>>> single MSTOP bit, MSTOP setup is reference-counted.
> >>>>>>
> >>>>>> Furthermore, as all modules start in the normal state after reset, if the
> >>>>>> module clocks are disabled, the module state is switched to standby. This
> >>>>>> prevents keeping the module in an invalid state, as recommended by the
> >>>>>> hardware team.
> >>>>>>
> >>>>>> Link: https://lore.kernel.org/all/20250215130849.227812-1-claudiu.beznea.uj@bp.renesas.com/
> >>>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>>>>> ---
> >>>>>>
> >>>>>> Changes in v2:
> >>>>>> - udpated patch description to avoid plural in the configuration
> >>>>>>   sequence description b/w MSTOP and CLK_ON
> >>>>>> - use atomic type to store the usage counter; s/refcnt/usecnt/g
> >>>>>> - moved MSTOP_OFF(), MSTOP_MASK() macros to rzg2l-cpg.c
> >>>>>> - dropped struct mstp_clock::critical and use clk_hw_get_flags()
> >>>>>>   instead to get the clock flags
> >>>>>> - used unsigned int iterators in for loops
> >>>>>> - keep memory allocated for a single list for clocks sharing the
> >>>>>>   same MSTOP by updating the rzg2l_mod_clock_add_shared_mstop_clk();
> >>>>>> - s/rzg2l_cpg_mstop_show/rzg2l_mod_clock_mstop_show/g,
> >>>>>>   s/rzg2l_cpg_mstop/rzg2l_mod_clock_mstop/g,
> >>>>>>   s/rzg2l_cpg_update_shared_mstop_clocks/rzg2l_mod_clock_update_shared_mstop_clks/g
> >>>>>>    to keep the same naming conventions for functions handling mod clock MSTOP
> >>>>>> - use the newly added for_each_mstp_clk() macro all over the code
> >>>>>
> >>>>> Thanks for the update!
> >>>>>
> >>>>>> --- a/drivers/clk/renesas/rzg2l-cpg.c
> >>>>>> +++ b/drivers/clk/renesas/rzg2l-cpg.c
> >>>>>
> >>>>>> @@ -1209,6 +1232,94 @@ struct mstp_clock {
> >>>>>>                 else if (((hw) = __clk_get_hw((priv)->clks[(priv)->num_core_clks + i])) && \
> >>>>>>                          ((mstp_clk) = to_mod_clock(hw)))
> >>>>>>
> >>>>>> +/* Need to be called with a lock held to avoid concurrent access to mstop->usecnt. */
> >>>>>> +static void rzg2l_mod_clock_module_set_state(struct mstp_clock *clock,
> >>>>>> +                                            bool standby)
> >>>>>> +{
> >>>>>> +       struct rzg2l_cpg_priv *priv = clock->priv;
> >>>>>> +       struct mstop *mstop = clock->mstop;
> >>>>>> +       bool update = false;
> >>>>>> +       u32 value;
> >>>>>> +
> >>>>>> +       if (!mstop)
> >>>>>> +               return;
> >>>>>> +
> >>>>>> +       value = MSTOP_MASK(mstop->conf) << 16;
> >>>>>> +
> >>>>>> +       if (standby) {
> >>>>>> +               unsigned int criticals = 0;
> >>>>>> +
> >>>>>> +               for (unsigned int i = 0; i < clock->num_shared_mstop_clks; i++) {
> >>>>>> +                       struct mstp_clock *clk = clock->shared_mstop_clks[i];
> >>>>>> +
> >>>>>> +                       if (clk_hw_get_flags(&clk->hw) & CLK_IS_CRITICAL)
> >>>>>> +                               criticals++;
> >>>>>> +               }
> >>>>>> +
> >>>>>> +               /*
> >>>>>> +                * If this is a shared MSTOP and it is shared with critical clocks,
> >>>>>> +                * and the system boots up with this clock enabled but no driver
> >>>>>> +                * uses it the CCF will disable it (as it is unused). As we don't
> >>>>>> +                * increment reference counter for it at registration (to avoid
> >>>>>> +                * messing with clocks enabled at probe but later used by drivers)
> >>>>>> +                * do not set the MSTOP here too if it is shared with critical
> >>>>>> +                * clocks and ref counted only by those critical clocks.
> >>>>>> +                */
> >>>>>> +               if (criticals && criticals == atomic_read(&mstop->usecnt))
> >>>>>> +                       return;
> >>>>>> +
> >>>>>> +               value |= MSTOP_MASK(mstop->conf);
> >>>>>> +
> >>>>>> +               /* Allow updates on probe when usecnt = 0. */
> >>>>>> +               if (!atomic_read(&mstop->usecnt))
> >>>>>> +                       update = true;
> >>>>>> +               else
> >>>>>> +                       update = atomic_dec_and_test(&mstop->usecnt);
> >>>>>> +       } else {
> >>>>>> +               atomic_inc(&mstop->usecnt);
> >>>>>> +               update = true;
> >>>>>
> >>>>> Shouldn't the update be conditional, i.e.:
> >>>>>
> >>>>>     if (!atomic_read(&mstop->usecnt))
> >>>>>             update = true;
> >>>>>     atomic_inc(&mstop->usecnt);
> >>>>>
> >>>>> ?
> >>>>
> >>>> Indeed, it should be conditional as you suggested.
> >>>>
> >>>>>
> >>>>>> +       }
> >>>>>> +
> >>>>>> +       if (update)
> >>>>>> +               writel(value, priv->base + MSTOP_OFF(mstop->conf));
> >>>>>> +}
> >>>>>
> >>>>>> +static int rzg2l_mod_clock_update_shared_mstop_clks(struct rzg2l_cpg_priv *priv,
> >>>>>> +                                                   struct mstp_clock *clock)
> >>>>>> +{
> >>>>>> +       struct mstp_clock *clk;
> >>>>>> +       struct clk_hw *hw;
> >>>>>> +
> >>>>>> +       if (!clock->mstop)
> >>>>>> +               return 0;
> >>>>>> +
> >>>>>> +       for_each_mstp_clk(clk, hw, priv) {
> >>>>>> +               struct mstp_clock **new_clks;
> >>>>>> +               int num_shared_mstop_clks;
> >>>>>> +               bool found = false;
> >>>>>> +
> >>>>>> +               if (clk->mstop != clock->mstop)
> >>>>>> +                       continue;
> >>>>>> +
> >>>>>> +               num_shared_mstop_clks = clk->num_shared_mstop_clks;
> >>>>>> +               for (unsigned int i = 0; i < num_shared_mstop_clks; i++) {
> >>>>>> +                       if (clk->shared_mstop_clks[i] == clock) {
> >>>>>> +                               found = true;
> >>>>>> +                               break;
> >>>>>> +                       }
> >>>>>> +               }
> >>>>>> +               if (found)
> >>>>>> +                       continue;
> >>>>>
> >>>>> Can this happen? With your current code, the answer is yes.
> >>>>> But I think this loop and check can be removed...
> >>>>>
> >>>>>> +
> >>>>>> +               if (!num_shared_mstop_clks)
> >>>>>> +                       new_clks = devm_kmalloc_array(priv->dev, 2, sizeof(*new_clks), GFP_KERNEL);
> >>>>>> +               else
> >>>>>> +                       new_clks = devm_krealloc(priv->dev, clk->shared_mstop_clks,
> >>>>>> +                                                (num_shared_mstop_clks + 1) * sizeof(*new_clks),
> >>>>>> +                                                GFP_KERNEL);
> >>>>>> +
> >>>>>> +               if (!new_clks)
> >>>>>> +                       return -ENOMEM;
> >>>>>> +
> >>>>>> +               if (!num_shared_mstop_clks)
> >>>>>> +                       new_clks[num_shared_mstop_clks++] = clk;
> >>>>>> +               if (clk != clock)
> >>>>>
> >>>>> This check is always true
> >>>>
> >>>> If I'm not wrong now, when adding the clock to it's own list, and the list
> >>>> is empty (!num_shared_mstop_clks check above is true), if this condition is
> >>>> missing the clock it will be added twice in its own list.
> >>>
> >>> Sorry, I missed that this function is called _after_ the clock is
> >>> added to priv->clks[].  So one question and comment here:
> >>>   1. Do you need a one-entry array (actual allocation is two entries)
> >>>      for module clocks with an mstop entry that is not shared?
> >>
> >> That extra entry should not be needed. It should not happen to have an
> >> mstop clock in the priv->clks[] array w/o at least a clock in its shared
> >> list. I was wrong in both the initial code and the reply I sent to your
> >> initial comment. Appologies for that.
> >
> > So no single-entry arrays...
>
> Oh, I missread it yesterday everning. Sorry for confusion. Let me try again:
>
> >
> >>>      Perhaps for critical clocks? That could be handled in
>
> The clock is added to its own shared_mstop_clk[] array to avoid extra
> conditions when all the clocks sharing the same mstop need to be checked
> for an action. One example is code at [A] (for critical clocks) that was
> available in v1.
>
>
> >>>      rzg2l_mod_clock_module_set_state(), by explicitly checking
> >>>      the clock's own critical flag if num_shared_mstop_clks is zero.
>
> The clock was added to its own shared_mstop_clk[] array as a result of this
> comment I got from you on v1 (with regards to checking the clock's critical
> flag):
>
> "If clock->shared_mstop_clks[] would include the current clock, then
> (a) this test would not be needed, and
> (b) all clocks sharing the same mstop could share a single
>     clock->shared_mstop_clks[] array."
>
> If I understood correctly, in [1] it has been proposed to have something
> like what is proposed here to avoid extra conditional check (like [A]), in
> rzg2l_mod_clock_module_set_state():
>
> for (u8 i = 0; i < clock->num_shared_mstop_clks; i++) {
>     unsigned int
>     struct mstp_clock *clk = clock->shared_mstop_clks[i];
>
>     if (clk->critical)
>         criticals++;
> }
>
> /* Increment if clock is critical, too. */
> if (clock->critical) // <<< [A]
>     criticals++;
>
> Please let me know if I misunderstood your initial request?

You did not misunderstand my initial request.  Until recently, I just
hadn't realized that you still need to check for a critical clock if
the mstop is not shared, sorry for that.

However, I think it is better to just add an extra check

    if (!clock->num_shared_mstop_clks &&
        (clk_hw_get_flags(&clock->hw) & CLK_IS_CRITICAL))
            criticals++;

to rzg2l_mod_clock_module_set_state(), than to allocate single-entry
arrays for each and every clock that does not share the mstop.
Do you agree?

Thanks!

> [1]
> https://lore.kernel.org/all/CAMuHMdXFtBmjDu=1RS2MLNYzhZ0fmpT7+1QbA9p4LvoLHitOuw@mail.gmail.com/

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

Re: [PATCH v2 4/8] clk: renesas: rzg2l-cpg: Add support for MSTOP in clock enable/disable API

[Claudiu Beznea]

Hi, Geert,

On 27.05.2025 12:10, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> On Tue, 27 May 2025 at 10:58, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
>> On 26.05.2025 20:09, Geert Uytterhoeven wrote:
>>> On Mon, 26 May 2025 at 17:55, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
>>>> On 26.05.2025 16:33, Geert Uytterhoeven wrote:
>>>>> On Fri, 23 May 2025 at 09:41, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
>>>>>> On 22.05.2025 17:46, Geert Uytterhoeven wrote:
>>>>>>> On Wed, 14 May 2025 at 11:04, Claudiu <claudiu.beznea@tuxon.dev> wrote:
>>>>>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>>>>>
>>>>>>>> The RZ/{G2L, V2L, G3S} CPG versions support a feature called MSTOP. Each
>>>>>>>> module has one or more MSTOP bits associated with it, and these bits need
>>>>>>>> to be configured along with the module clocks. Setting the MSTOP bits
>>>>>>>> switches the module between normal and standby states.
>>>>>>>>
>>>>>>>> Previously, MSTOP support was abstracted through power domains
>>>>>>>> (struct generic_pm_domain::{power_on, power_off} APIs). With this
>>>>>>>> abstraction, the order of setting the MSTOP and CLKON bits was as follows:
>>>>>>>>
>>>>>>>> Previous Order:
>>>>>>>> A/ Switching to Normal State (e.g., during probe):
>>>>>>>> 1/ Clear module MSTOP bit
>>>>>>>> 2/ Set module CLKON bit
>>>>>>>>
>>>>>>>> B/ Switching to Standby State (e.g., during remove):
>>>>>>>> 1/ Clear CLKON bit
>>>>>>>> 2/ Set MSTOP bit
>>>>>>>>
>>>>>>>> However, in some cases (when the clock is disabled through devres), the
>>>>>>>> order may have been (due to the issue described in link section):
>>>>>>>>
>>>>>>>> 1/ Set MSTOP bit
>>>>>>>> 2/ Clear CLKON bit
>>>>>>>>
>>>>>>>> Recently, the hardware team has suggested that the correct order to set
>>>>>>>> the MSTOP and CLKON bits is:
>>>>>>>>
>>>>>>>> Updated Order:
>>>>>>>> A/ Switching to Normal State (e.g., during probe):
>>>>>>>> 1/ Set CLKON bit
>>>>>>>> 2/ Clear MSTOP bit
>>>>>>>>
>>>>>>>> B/ Switching to Standby State (e.g., during remove):
>>>>>>>> 1/ Set MSTOP bit
>>>>>>>> 2/ Clear CLKON bit
>>>>>>>>
>>>>>>>> To prevent future issues due to incorrect ordering, the MSTOP setup has
>>>>>>>> now been implemented in rzg2l_mod_clock_endisable(), ensuring compliance
>>>>>>>> with the sequence suggested in Figure 41.5: Module Standby Mode Procedure
>>>>>>>> from the RZ/G3S HW manual, Rev1.10.
>>>>>>>>
>>>>>>>> Additionally, since multiple clocks of a single module may be mapped to a
>>>>>>>> single MSTOP bit, MSTOP setup is reference-counted.
>>>>>>>>
>>>>>>>> Furthermore, as all modules start in the normal state after reset, if the
>>>>>>>> module clocks are disabled, the module state is switched to standby. This
>>>>>>>> prevents keeping the module in an invalid state, as recommended by the
>>>>>>>> hardware team.
>>>>>>>>
>>>>>>>> Link: https://lore.kernel.org/all/20250215130849.227812-1-claudiu.beznea.uj@bp.renesas.com/
>>>>>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> Changes in v2:
>>>>>>>> - udpated patch description to avoid plural in the configuration
>>>>>>>>   sequence description b/w MSTOP and CLK_ON
>>>>>>>> - use atomic type to store the usage counter; s/refcnt/usecnt/g
>>>>>>>> - moved MSTOP_OFF(), MSTOP_MASK() macros to rzg2l-cpg.c
>>>>>>>> - dropped struct mstp_clock::critical and use clk_hw_get_flags()
>>>>>>>>   instead to get the clock flags
>>>>>>>> - used unsigned int iterators in for loops
>>>>>>>> - keep memory allocated for a single list for clocks sharing the
>>>>>>>>   same MSTOP by updating the rzg2l_mod_clock_add_shared_mstop_clk();
>>>>>>>> - s/rzg2l_cpg_mstop_show/rzg2l_mod_clock_mstop_show/g,
>>>>>>>>   s/rzg2l_cpg_mstop/rzg2l_mod_clock_mstop/g,
>>>>>>>>   s/rzg2l_cpg_update_shared_mstop_clocks/rzg2l_mod_clock_update_shared_mstop_clks/g
>>>>>>>>    to keep the same naming conventions for functions handling mod clock MSTOP
>>>>>>>> - use the newly added for_each_mstp_clk() macro all over the code
>>>>>>>
>>>>>>> Thanks for the update!
>>>>>>>
>>>>>>>> --- a/drivers/clk/renesas/rzg2l-cpg.c
>>>>>>>> +++ b/drivers/clk/renesas/rzg2l-cpg.c
>>>>>>>
>>>>>>>> @@ -1209,6 +1232,94 @@ struct mstp_clock {
>>>>>>>>                 else if (((hw) = __clk_get_hw((priv)->clks[(priv)->num_core_clks + i])) && \
>>>>>>>>                          ((mstp_clk) = to_mod_clock(hw)))
>>>>>>>>
>>>>>>>> +/* Need to be called with a lock held to avoid concurrent access to mstop->usecnt. */
>>>>>>>> +static void rzg2l_mod_clock_module_set_state(struct mstp_clock *clock,
>>>>>>>> +                                            bool standby)
>>>>>>>> +{
>>>>>>>> +       struct rzg2l_cpg_priv *priv = clock->priv;
>>>>>>>> +       struct mstop *mstop = clock->mstop;
>>>>>>>> +       bool update = false;
>>>>>>>> +       u32 value;
>>>>>>>> +
>>>>>>>> +       if (!mstop)
>>>>>>>> +               return;
>>>>>>>> +
>>>>>>>> +       value = MSTOP_MASK(mstop->conf) << 16;
>>>>>>>> +
>>>>>>>> +       if (standby) {
>>>>>>>> +               unsigned int criticals = 0;
>>>>>>>> +
>>>>>>>> +               for (unsigned int i = 0; i < clock->num_shared_mstop_clks; i++) {
>>>>>>>> +                       struct mstp_clock *clk = clock->shared_mstop_clks[i];
>>>>>>>> +
>>>>>>>> +                       if (clk_hw_get_flags(&clk->hw) & CLK_IS_CRITICAL)
>>>>>>>> +                               criticals++;
>>>>>>>> +               }
>>>>>>>> +
>>>>>>>> +               /*
>>>>>>>> +                * If this is a shared MSTOP and it is shared with critical clocks,
>>>>>>>> +                * and the system boots up with this clock enabled but no driver
>>>>>>>> +                * uses it the CCF will disable it (as it is unused). As we don't
>>>>>>>> +                * increment reference counter for it at registration (to avoid
>>>>>>>> +                * messing with clocks enabled at probe but later used by drivers)
>>>>>>>> +                * do not set the MSTOP here too if it is shared with critical
>>>>>>>> +                * clocks and ref counted only by those critical clocks.
>>>>>>>> +                */
>>>>>>>> +               if (criticals && criticals == atomic_read(&mstop->usecnt))
>>>>>>>> +                       return;
>>>>>>>> +
>>>>>>>> +               value |= MSTOP_MASK(mstop->conf);
>>>>>>>> +
>>>>>>>> +               /* Allow updates on probe when usecnt = 0. */
>>>>>>>> +               if (!atomic_read(&mstop->usecnt))
>>>>>>>> +                       update = true;
>>>>>>>> +               else
>>>>>>>> +                       update = atomic_dec_and_test(&mstop->usecnt);
>>>>>>>> +       } else {
>>>>>>>> +               atomic_inc(&mstop->usecnt);
>>>>>>>> +               update = true;
>>>>>>>
>>>>>>> Shouldn't the update be conditional, i.e.:
>>>>>>>
>>>>>>>     if (!atomic_read(&mstop->usecnt))
>>>>>>>             update = true;
>>>>>>>     atomic_inc(&mstop->usecnt);
>>>>>>>
>>>>>>> ?
>>>>>>
>>>>>> Indeed, it should be conditional as you suggested.
>>>>>>
>>>>>>>
>>>>>>>> +       }
>>>>>>>> +
>>>>>>>> +       if (update)
>>>>>>>> +               writel(value, priv->base + MSTOP_OFF(mstop->conf));
>>>>>>>> +}
>>>>>>>
>>>>>>>> +static int rzg2l_mod_clock_update_shared_mstop_clks(struct rzg2l_cpg_priv *priv,
>>>>>>>> +                                                   struct mstp_clock *clock)
>>>>>>>> +{
>>>>>>>> +       struct mstp_clock *clk;
>>>>>>>> +       struct clk_hw *hw;
>>>>>>>> +
>>>>>>>> +       if (!clock->mstop)
>>>>>>>> +               return 0;
>>>>>>>> +
>>>>>>>> +       for_each_mstp_clk(clk, hw, priv) {
>>>>>>>> +               struct mstp_clock **new_clks;
>>>>>>>> +               int num_shared_mstop_clks;
>>>>>>>> +               bool found = false;
>>>>>>>> +
>>>>>>>> +               if (clk->mstop != clock->mstop)
>>>>>>>> +                       continue;
>>>>>>>> +
>>>>>>>> +               num_shared_mstop_clks = clk->num_shared_mstop_clks;
>>>>>>>> +               for (unsigned int i = 0; i < num_shared_mstop_clks; i++) {
>>>>>>>> +                       if (clk->shared_mstop_clks[i] == clock) {
>>>>>>>> +                               found = true;
>>>>>>>> +                               break;
>>>>>>>> +                       }
>>>>>>>> +               }
>>>>>>>> +               if (found)
>>>>>>>> +                       continue;
>>>>>>>
>>>>>>> Can this happen? With your current code, the answer is yes.
>>>>>>> But I think this loop and check can be removed...
>>>>>>>
>>>>>>>> +
>>>>>>>> +               if (!num_shared_mstop_clks)
>>>>>>>> +                       new_clks = devm_kmalloc_array(priv->dev, 2, sizeof(*new_clks), GFP_KERNEL);
>>>>>>>> +               else
>>>>>>>> +                       new_clks = devm_krealloc(priv->dev, clk->shared_mstop_clks,
>>>>>>>> +                                                (num_shared_mstop_clks + 1) * sizeof(*new_clks),
>>>>>>>> +                                                GFP_KERNEL);
>>>>>>>> +
>>>>>>>> +               if (!new_clks)
>>>>>>>> +                       return -ENOMEM;
>>>>>>>> +
>>>>>>>> +               if (!num_shared_mstop_clks)
>>>>>>>> +                       new_clks[num_shared_mstop_clks++] = clk;
>>>>>>>> +               if (clk != clock)
>>>>>>>
>>>>>>> This check is always true
>>>>>>
>>>>>> If I'm not wrong now, when adding the clock to it's own list, and the list
>>>>>> is empty (!num_shared_mstop_clks check above is true), if this condition is
>>>>>> missing the clock it will be added twice in its own list.
>>>>>
>>>>> Sorry, I missed that this function is called _after_ the clock is
>>>>> added to priv->clks[].  So one question and comment here:
>>>>>   1. Do you need a one-entry array (actual allocation is two entries)
>>>>>      for module clocks with an mstop entry that is not shared?
>>>>
>>>> That extra entry should not be needed. It should not happen to have an
>>>> mstop clock in the priv->clks[] array w/o at least a clock in its shared
>>>> list. I was wrong in both the initial code and the reply I sent to your
>>>> initial comment. Appologies for that.
>>>
>>> So no single-entry arrays...
>>
>> Oh, I missread it yesterday everning. Sorry for confusion. Let me try again:
>>
>>>
>>>>>      Perhaps for critical clocks? That could be handled in
>>
>> The clock is added to its own shared_mstop_clk[] array to avoid extra
>> conditions when all the clocks sharing the same mstop need to be checked
>> for an action. One example is code at [A] (for critical clocks) that was
>> available in v1.
>>
>>
>>>>>      rzg2l_mod_clock_module_set_state(), by explicitly checking
>>>>>      the clock's own critical flag if num_shared_mstop_clks is zero.
>>
>> The clock was added to its own shared_mstop_clk[] array as a result of this
>> comment I got from you on v1 (with regards to checking the clock's critical
>> flag):
>>
>> "If clock->shared_mstop_clks[] would include the current clock, then
>> (a) this test would not be needed, and
>> (b) all clocks sharing the same mstop could share a single
>>     clock->shared_mstop_clks[] array."
>>
>> If I understood correctly, in [1] it has been proposed to have something
>> like what is proposed here to avoid extra conditional check (like [A]), in
>> rzg2l_mod_clock_module_set_state():
>>
>> for (u8 i = 0; i < clock->num_shared_mstop_clks; i++) {
>>     unsigned int
>>     struct mstp_clock *clk = clock->shared_mstop_clks[i];
>>
>>     if (clk->critical)
>>         criticals++;
>> }
>>
>> /* Increment if clock is critical, too. */
>> if (clock->critical) // <<< [A]
>>     criticals++;
>>
>> Please let me know if I misunderstood your initial request?
> 
> You did not misunderstand my initial request.  Until recently, I just
> hadn't realized that you still need to check for a critical clock if
> the mstop is not shared, sorry for that.
> 
> However, I think it is better to just add an extra check
> 
>     if (!clock->num_shared_mstop_clks &&
>         (clk_hw_get_flags(&clock->hw) & CLK_IS_CRITICAL))
>             criticals++;
> 
> to rzg2l_mod_clock_module_set_state(), than to allocate single-entry
> arrays for each and every clock that does not share the mstop.
> Do you agree?

Yes!

Thank you for your review,
Claudiu

[PATCH v2 5/8] clk: renesas: r9a08g045: Drop power domain instantiation

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Since the configuration order between the individual MSTOP and CLKON bits
cannot be preserved with the power domain abstraction, drop the power
domain instantiations.

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

Changes in v2:
- added RTC MSTOP bit on the VBATB clock; the RTC and VBATTB clock
  is gated by the same bit (CPG_CLKON_VBAT, BIT 0) but they have
  different MSTOP bits (bit 8 for VBAT, bit 7 for RTC); this is
  not described in HW manual ATM.

 drivers/clk/renesas/r9a08g045-cpg.c | 216 ++++++++++++----------------
 1 file changed, 93 insertions(+), 123 deletions(-)

diff --git a/drivers/clk/renesas/r9a08g045-cpg.c b/drivers/clk/renesas/r9a08g045-cpg.c
index ddc5d6cd572b..405907925bb7 100644
--- a/drivers/clk/renesas/r9a08g045-cpg.c
+++ b/drivers/clk/renesas/r9a08g045-cpg.c
@@ -192,59 +192,105 @@ static const struct cpg_core_clk r9a08g045_core_clks[] __initconst = {
 };
 
 static const struct rzg2l_mod_clk r9a08g045_mod_clks[] = {
-	DEF_MOD("gic_gicclk",		R9A08G045_GIC600_GICCLK, R9A08G045_CLK_P1, 0x514, 0, 0),
-	DEF_MOD("ia55_pclk",		R9A08G045_IA55_PCLK, R9A08G045_CLK_P2, 0x518, 0, 0),
-	DEF_MOD("ia55_clk",		R9A08G045_IA55_CLK, R9A08G045_CLK_P1, 0x518, 1, 0),
-	DEF_MOD("dmac_aclk",		R9A08G045_DMAC_ACLK, R9A08G045_CLK_P3, 0x52c, 0, 0),
-	DEF_MOD("dmac_pclk",		R9A08G045_DMAC_PCLK, CLK_P3_DIV2, 0x52c, 1, 0),
-	DEF_MOD("wdt0_pclk",		R9A08G045_WDT0_PCLK, R9A08G045_CLK_P0, 0x548, 0, 0),
-	DEF_MOD("wdt0_clk",		R9A08G045_WDT0_CLK, R9A08G045_OSCCLK, 0x548, 1, 0),
-	DEF_MOD("sdhi0_imclk",		R9A08G045_SDHI0_IMCLK, CLK_SD0_DIV4, 0x554, 0, 0),
-	DEF_MOD("sdhi0_imclk2",		R9A08G045_SDHI0_IMCLK2, CLK_SD0_DIV4, 0x554, 1, 0),
-	DEF_MOD("sdhi0_clk_hs",		R9A08G045_SDHI0_CLK_HS, R9A08G045_CLK_SD0, 0x554, 2, 0),
-	DEF_MOD("sdhi0_aclk",		R9A08G045_SDHI0_ACLK, R9A08G045_CLK_P1, 0x554, 3, 0),
-	DEF_MOD("sdhi1_imclk",		R9A08G045_SDHI1_IMCLK, CLK_SD1_DIV4, 0x554, 4, 0),
-	DEF_MOD("sdhi1_imclk2",		R9A08G045_SDHI1_IMCLK2, CLK_SD1_DIV4, 0x554, 5, 0),
-	DEF_MOD("sdhi1_clk_hs",		R9A08G045_SDHI1_CLK_HS, R9A08G045_CLK_SD1, 0x554, 6, 0),
-	DEF_MOD("sdhi1_aclk",		R9A08G045_SDHI1_ACLK, R9A08G045_CLK_P1, 0x554, 7, 0),
-	DEF_MOD("sdhi2_imclk",		R9A08G045_SDHI2_IMCLK, CLK_SD2_DIV4, 0x554, 8, 0),
-	DEF_MOD("sdhi2_imclk2",		R9A08G045_SDHI2_IMCLK2, CLK_SD2_DIV4, 0x554, 9, 0),
-	DEF_MOD("sdhi2_clk_hs",		R9A08G045_SDHI2_CLK_HS, R9A08G045_CLK_SD2, 0x554, 10, 0),
-	DEF_MOD("sdhi2_aclk",		R9A08G045_SDHI2_ACLK, R9A08G045_CLK_P1, 0x554, 11, 0),
-	DEF_MOD("ssi0_pclk2",		R9A08G045_SSI0_PCLK2, R9A08G045_CLK_P0, 0x570, 0, 0),
-	DEF_MOD("ssi0_sfr",		R9A08G045_SSI0_PCLK_SFR, R9A08G045_CLK_P0, 0x570, 1, 0),
-	DEF_MOD("ssi1_pclk2",		R9A08G045_SSI1_PCLK2, R9A08G045_CLK_P0, 0x570, 2, 0),
-	DEF_MOD("ssi1_sfr",		R9A08G045_SSI1_PCLK_SFR, R9A08G045_CLK_P0, 0x570, 3, 0),
-	DEF_MOD("ssi2_pclk2",		R9A08G045_SSI2_PCLK2, R9A08G045_CLK_P0, 0x570, 4, 0),
-	DEF_MOD("ssi2_sfr",		R9A08G045_SSI2_PCLK_SFR, R9A08G045_CLK_P0, 0x570, 5, 0),
-	DEF_MOD("ssi3_pclk2",		R9A08G045_SSI3_PCLK2, R9A08G045_CLK_P0, 0x570, 6, 0),
-	DEF_MOD("ssi3_sfr",		R9A08G045_SSI3_PCLK_SFR, R9A08G045_CLK_P0, 0x570, 7, 0),
-	DEF_MOD("usb0_host",		R9A08G045_USB_U2H0_HCLK, R9A08G045_CLK_P1, 0x578, 0, 0),
-	DEF_MOD("usb1_host",		R9A08G045_USB_U2H1_HCLK, R9A08G045_CLK_P1, 0x578, 1, 0),
+	DEF_MOD("gic_gicclk",		R9A08G045_GIC600_GICCLK, R9A08G045_CLK_P1, 0x514, 0,
+					MSTOP(BUS_ACPU, BIT(3))),
+	DEF_MOD("ia55_pclk",		R9A08G045_IA55_PCLK, R9A08G045_CLK_P2, 0x518, 0,
+					MSTOP(BUS_PERI_CPU, BIT(13))),
+	DEF_MOD("ia55_clk",		R9A08G045_IA55_CLK, R9A08G045_CLK_P1, 0x518, 1,
+					MSTOP(BUS_PERI_CPU, BIT(13))),
+	DEF_MOD("dmac_aclk",		R9A08G045_DMAC_ACLK, R9A08G045_CLK_P3, 0x52c, 0,
+					MSTOP(BUS_REG1, BIT(2))),
+	DEF_MOD("dmac_pclk",		R9A08G045_DMAC_PCLK, CLK_P3_DIV2, 0x52c, 1,
+					MSTOP(BUS_REG1, BIT(3))),
+	DEF_MOD("wdt0_pclk",		R9A08G045_WDT0_PCLK, R9A08G045_CLK_P0, 0x548, 0,
+					MSTOP(BUS_REG0, BIT(0))),
+	DEF_MOD("wdt0_clk",		R9A08G045_WDT0_CLK, R9A08G045_OSCCLK, 0x548, 1,
+					MSTOP(BUS_REG0, BIT(0))),
+	DEF_MOD("sdhi0_imclk",		R9A08G045_SDHI0_IMCLK, CLK_SD0_DIV4, 0x554, 0,
+					MSTOP(BUS_PERI_COM, BIT(0))),
+	DEF_MOD("sdhi0_imclk2",		R9A08G045_SDHI0_IMCLK2, CLK_SD0_DIV4, 0x554, 1,
+					MSTOP(BUS_PERI_COM, BIT(0))),
+	DEF_MOD("sdhi0_clk_hs",		R9A08G045_SDHI0_CLK_HS, R9A08G045_CLK_SD0, 0x554, 2,
+					MSTOP(BUS_PERI_COM, BIT(0))),
+	DEF_MOD("sdhi0_aclk",		R9A08G045_SDHI0_ACLK, R9A08G045_CLK_P1, 0x554, 3,
+					MSTOP(BUS_PERI_COM, BIT(0))),
+	DEF_MOD("sdhi1_imclk",		R9A08G045_SDHI1_IMCLK, CLK_SD1_DIV4, 0x554, 4,
+					MSTOP(BUS_PERI_COM, BIT(1))),
+	DEF_MOD("sdhi1_imclk2",		R9A08G045_SDHI1_IMCLK2, CLK_SD1_DIV4, 0x554, 5,
+					MSTOP(BUS_PERI_COM, BIT(1))),
+	DEF_MOD("sdhi1_clk_hs",		R9A08G045_SDHI1_CLK_HS, R9A08G045_CLK_SD1, 0x554, 6,
+					MSTOP(BUS_PERI_COM, BIT(1))),
+	DEF_MOD("sdhi1_aclk",		R9A08G045_SDHI1_ACLK, R9A08G045_CLK_P1, 0x554, 7,
+					MSTOP(BUS_PERI_COM, BIT(1))),
+	DEF_MOD("sdhi2_imclk",		R9A08G045_SDHI2_IMCLK, CLK_SD2_DIV4, 0x554, 8,
+					MSTOP(BUS_PERI_COM, BIT(11))),
+	DEF_MOD("sdhi2_imclk2",		R9A08G045_SDHI2_IMCLK2, CLK_SD2_DIV4, 0x554, 9,
+					MSTOP(BUS_PERI_COM, BIT(11))),
+	DEF_MOD("sdhi2_clk_hs",		R9A08G045_SDHI2_CLK_HS, R9A08G045_CLK_SD2, 0x554, 10,
+					MSTOP(BUS_PERI_COM, BIT(11))),
+	DEF_MOD("sdhi2_aclk",		R9A08G045_SDHI2_ACLK, R9A08G045_CLK_P1, 0x554, 11,
+					MSTOP(BUS_PERI_COM, BIT(11))),
+	DEF_MOD("ssi0_pclk2",		R9A08G045_SSI0_PCLK2, R9A08G045_CLK_P0, 0x570, 0,
+					MSTOP(BUS_MCPU1, BIT(10))),
+	DEF_MOD("ssi0_sfr",		R9A08G045_SSI0_PCLK_SFR, R9A08G045_CLK_P0, 0x570, 1,
+					MSTOP(BUS_MCPU1, BIT(10))),
+	DEF_MOD("ssi1_pclk2",		R9A08G045_SSI1_PCLK2, R9A08G045_CLK_P0, 0x570, 2,
+					MSTOP(BUS_MCPU1, BIT(11))),
+	DEF_MOD("ssi1_sfr",		R9A08G045_SSI1_PCLK_SFR, R9A08G045_CLK_P0, 0x570, 3,
+					MSTOP(BUS_MCPU1, BIT(11))),
+	DEF_MOD("ssi2_pclk2",		R9A08G045_SSI2_PCLK2, R9A08G045_CLK_P0, 0x570, 4,
+					MSTOP(BUS_MCPU1, BIT(12))),
+	DEF_MOD("ssi2_sfr",		R9A08G045_SSI2_PCLK_SFR, R9A08G045_CLK_P0, 0x570, 5,
+					MSTOP(BUS_MCPU1, BIT(12))),
+	DEF_MOD("ssi3_pclk2",		R9A08G045_SSI3_PCLK2, R9A08G045_CLK_P0, 0x570, 6,
+					MSTOP(BUS_MCPU1, BIT(13))),
+	DEF_MOD("ssi3_sfr",		R9A08G045_SSI3_PCLK_SFR, R9A08G045_CLK_P0, 0x570, 7,
+					MSTOP(BUS_MCPU1, BIT(13))),
+	DEF_MOD("usb0_host",		R9A08G045_USB_U2H0_HCLK, R9A08G045_CLK_P1, 0x578, 0,
+					MSTOP(BUS_PERI_COM, BIT(5))),
+	DEF_MOD("usb1_host",		R9A08G045_USB_U2H1_HCLK, R9A08G045_CLK_P1, 0x578, 1,
+					MSTOP(BUS_PERI_COM, BIT(7))),
 	DEF_MOD("usb0_func",		R9A08G045_USB_U2P_EXR_CPUCLK, R9A08G045_CLK_P1, 0x578, 2,
-					0),
-	DEF_MOD("usb_pclk",		R9A08G045_USB_PCLK, R9A08G045_CLK_P1, 0x578, 3, 0),
-	DEF_COUPLED("eth0_axi",		R9A08G045_ETH0_CLK_AXI, R9A08G045_CLK_M0, 0x57c, 0, 0),
+					MSTOP(BUS_PERI_COM, BIT(6))),
+	DEF_MOD("usb_pclk",		R9A08G045_USB_PCLK, R9A08G045_CLK_P1, 0x578, 3,
+					MSTOP(BUS_PERI_COM, BIT(4))),
+	DEF_COUPLED("eth0_axi",		R9A08G045_ETH0_CLK_AXI, R9A08G045_CLK_M0, 0x57c, 0,
+					MSTOP(BUS_PERI_COM, BIT(2))),
 	DEF_COUPLED("eth0_chi",		R9A08G045_ETH0_CLK_CHI, R9A08G045_CLK_ZT, 0x57c, 0, 0),
 	DEF_MOD("eth0_refclk",		R9A08G045_ETH0_REFCLK, R9A08G045_CLK_HP, 0x57c, 8, 0),
-	DEF_COUPLED("eth1_axi",		R9A08G045_ETH1_CLK_AXI, R9A08G045_CLK_M0, 0x57c, 1, 0),
+	DEF_COUPLED("eth1_axi",		R9A08G045_ETH1_CLK_AXI, R9A08G045_CLK_M0, 0x57c, 1,
+					MSTOP(BUS_PERI_COM, BIT(3))),
 	DEF_COUPLED("eth1_chi",		R9A08G045_ETH1_CLK_CHI, R9A08G045_CLK_ZT, 0x57c, 1, 0),
 	DEF_MOD("eth1_refclk",		R9A08G045_ETH1_REFCLK, R9A08G045_CLK_HP, 0x57c, 9, 0),
-	DEF_MOD("i2c0_pclk",		R9A08G045_I2C0_PCLK, R9A08G045_CLK_P0, 0x580, 0, 0),
-	DEF_MOD("i2c1_pclk",		R9A08G045_I2C1_PCLK, R9A08G045_CLK_P0, 0x580, 1, 0),
-	DEF_MOD("i2c2_pclk",		R9A08G045_I2C2_PCLK, R9A08G045_CLK_P0, 0x580, 2, 0),
-	DEF_MOD("i2c3_pclk",		R9A08G045_I2C3_PCLK, R9A08G045_CLK_P0, 0x580, 3, 0),
-	DEF_MOD("scif0_clk_pck",	R9A08G045_SCIF0_CLK_PCK, R9A08G045_CLK_P0, 0x584, 0, 0),
-	DEF_MOD("scif1_clk_pck",	R9A08G045_SCIF1_CLK_PCK, R9A08G045_CLK_P0, 0x584, 1, 0),
-	DEF_MOD("scif2_clk_pck",	R9A08G045_SCIF2_CLK_PCK, R9A08G045_CLK_P0, 0x584, 2, 0),
-	DEF_MOD("scif3_clk_pck",	R9A08G045_SCIF3_CLK_PCK, R9A08G045_CLK_P0, 0x584, 3, 0),
-	DEF_MOD("scif4_clk_pck",	R9A08G045_SCIF4_CLK_PCK, R9A08G045_CLK_P0, 0x584, 4, 0),
-	DEF_MOD("scif5_clk_pck",	R9A08G045_SCIF5_CLK_PCK, R9A08G045_CLK_P0, 0x584, 5, 0),
+	DEF_MOD("i2c0_pclk",		R9A08G045_I2C0_PCLK, R9A08G045_CLK_P0, 0x580, 0,
+					MSTOP(BUS_MCPU2, BIT(10))),
+	DEF_MOD("i2c1_pclk",		R9A08G045_I2C1_PCLK, R9A08G045_CLK_P0, 0x580, 1,
+					MSTOP(BUS_MCPU2, BIT(11))),
+	DEF_MOD("i2c2_pclk",		R9A08G045_I2C2_PCLK, R9A08G045_CLK_P0, 0x580, 2,
+					MSTOP(BUS_MCPU2, BIT(12))),
+	DEF_MOD("i2c3_pclk",		R9A08G045_I2C3_PCLK, R9A08G045_CLK_P0, 0x580, 3,
+					MSTOP(BUS_MCPU2, BIT(13))),
+	DEF_MOD("scif0_clk_pck",	R9A08G045_SCIF0_CLK_PCK, R9A08G045_CLK_P0, 0x584, 0,
+					MSTOP(BUS_MCPU2, BIT(1))),
+	DEF_MOD("scif1_clk_pck",	R9A08G045_SCIF1_CLK_PCK, R9A08G045_CLK_P0, 0x584, 1,
+					MSTOP(BUS_MCPU2, BIT(2))),
+	DEF_MOD("scif2_clk_pck",	R9A08G045_SCIF2_CLK_PCK, R9A08G045_CLK_P0, 0x584, 2,
+					MSTOP(BUS_MCPU2, BIT(3))),
+	DEF_MOD("scif3_clk_pck",	R9A08G045_SCIF3_CLK_PCK, R9A08G045_CLK_P0, 0x584, 3,
+					MSTOP(BUS_MCPU2, BIT(4))),
+	DEF_MOD("scif4_clk_pck",	R9A08G045_SCIF4_CLK_PCK, R9A08G045_CLK_P0, 0x584, 4,
+					MSTOP(BUS_MCPU2, BIT(5))),
+	DEF_MOD("scif5_clk_pck",	R9A08G045_SCIF5_CLK_PCK, R9A08G045_CLK_P0, 0x584, 5,
+					MSTOP(BUS_MCPU3, BIT(4))),
 	DEF_MOD("gpio_hclk",		R9A08G045_GPIO_HCLK, R9A08G045_OSCCLK, 0x598, 0, 0),
-	DEF_MOD("adc_adclk",		R9A08G045_ADC_ADCLK, R9A08G045_CLK_TSU, 0x5a8, 0, 0),
-	DEF_MOD("adc_pclk",		R9A08G045_ADC_PCLK, R9A08G045_CLK_TSU, 0x5a8, 1, 0),
-	DEF_MOD("tsu_pclk",		R9A08G045_TSU_PCLK, R9A08G045_CLK_TSU, 0x5ac, 0, 0),
-	DEF_MOD("vbat_bclk",		R9A08G045_VBAT_BCLK, R9A08G045_OSCCLK, 0x614, 0, 0),
+	DEF_MOD("adc_adclk",		R9A08G045_ADC_ADCLK, R9A08G045_CLK_TSU, 0x5a8, 0,
+					MSTOP(BUS_MCPU2, BIT(14))),
+	DEF_MOD("adc_pclk",		R9A08G045_ADC_PCLK, R9A08G045_CLK_TSU, 0x5a8, 1,
+					MSTOP(BUS_MCPU2, BIT(14))),
+	DEF_MOD("tsu_pclk",		R9A08G045_TSU_PCLK, R9A08G045_CLK_TSU, 0x5ac, 0,
+					MSTOP(BUS_MCPU2, BIT(15))),
+	DEF_MOD("vbat_bclk",		R9A08G045_VBAT_BCLK, R9A08G045_OSCCLK, 0x614, 0,
+					MSTOP(BUS_MCPU3, GENMASK(8, 7))),
 };
 
 static const struct rzg2l_reset r9a08g045_resets[] = {
@@ -294,78 +340,6 @@ static const unsigned int r9a08g045_crit_mod_clks[] __initconst = {
 	MOD_CLK_BASE + R9A08G045_VBAT_BCLK,
 };
 
-static const struct rzg2l_cpg_pm_domain_init_data r9a08g045_pm_domains[] = {
-	/* Keep always-on domain on the first position for proper domains registration. */
-	DEF_PD("always-on",	R9A08G045_PD_ALWAYS_ON,
-				DEF_REG_CONF(0, 0),
-				GENPD_FLAG_ALWAYS_ON | GENPD_FLAG_IRQ_SAFE),
-	DEF_PD("gic",		R9A08G045_PD_GIC,
-				DEF_REG_CONF(CPG_BUS_ACPU_MSTOP, BIT(3)),
-				GENPD_FLAG_ALWAYS_ON),
-	DEF_PD("ia55",		R9A08G045_PD_IA55,
-				DEF_REG_CONF(CPG_BUS_PERI_CPU_MSTOP, BIT(13)),
-				GENPD_FLAG_ALWAYS_ON),
-	DEF_PD("dmac",		R9A08G045_PD_DMAC,
-				DEF_REG_CONF(CPG_BUS_REG1_MSTOP, GENMASK(3, 0)),
-				GENPD_FLAG_ALWAYS_ON),
-	DEF_PD("wdt0",		R9A08G045_PD_WDT0,
-				DEF_REG_CONF(CPG_BUS_REG0_MSTOP, BIT(0)),
-				GENPD_FLAG_IRQ_SAFE),
-	DEF_PD("sdhi0",		R9A08G045_PD_SDHI0,
-				DEF_REG_CONF(CPG_BUS_PERI_COM_MSTOP, BIT(0)), 0),
-	DEF_PD("sdhi1",		R9A08G045_PD_SDHI1,
-				DEF_REG_CONF(CPG_BUS_PERI_COM_MSTOP, BIT(1)), 0),
-	DEF_PD("sdhi2",		R9A08G045_PD_SDHI2,
-				DEF_REG_CONF(CPG_BUS_PERI_COM_MSTOP, BIT(11)), 0),
-	DEF_PD("ssi0",		R9A08G045_PD_SSI0,
-				DEF_REG_CONF(CPG_BUS_MCPU1_MSTOP, BIT(10)), 0),
-	DEF_PD("ssi1",		R9A08G045_PD_SSI1,
-				DEF_REG_CONF(CPG_BUS_MCPU1_MSTOP, BIT(11)), 0),
-	DEF_PD("ssi2",		R9A08G045_PD_SSI2,
-				DEF_REG_CONF(CPG_BUS_MCPU1_MSTOP, BIT(12)), 0),
-	DEF_PD("ssi3",		R9A08G045_PD_SSI3,
-				DEF_REG_CONF(CPG_BUS_MCPU1_MSTOP, BIT(13)), 0),
-	DEF_PD("usb0",		R9A08G045_PD_USB0,
-				DEF_REG_CONF(CPG_BUS_PERI_COM_MSTOP, GENMASK(6, 5)), 0),
-	DEF_PD("usb1",		R9A08G045_PD_USB1,
-				DEF_REG_CONF(CPG_BUS_PERI_COM_MSTOP, BIT(7)), 0),
-	DEF_PD("usb-phy",	R9A08G045_PD_USB_PHY,
-				DEF_REG_CONF(CPG_BUS_PERI_COM_MSTOP, BIT(4)), 0),
-	DEF_PD("eth0",		R9A08G045_PD_ETHER0,
-				DEF_REG_CONF(CPG_BUS_PERI_COM_MSTOP, BIT(2)), 0),
-	DEF_PD("eth1",		R9A08G045_PD_ETHER1,
-				DEF_REG_CONF(CPG_BUS_PERI_COM_MSTOP, BIT(3)), 0),
-	DEF_PD("i2c0",		R9A08G045_PD_I2C0,
-				DEF_REG_CONF(CPG_BUS_MCPU2_MSTOP, BIT(10)), 0),
-	DEF_PD("i2c1",		R9A08G045_PD_I2C1,
-				DEF_REG_CONF(CPG_BUS_MCPU2_MSTOP, BIT(11)), 0),
-	DEF_PD("i2c2",		R9A08G045_PD_I2C2,
-				DEF_REG_CONF(CPG_BUS_MCPU2_MSTOP, BIT(12)), 0),
-	DEF_PD("i2c3",		R9A08G045_PD_I2C3,
-				DEF_REG_CONF(CPG_BUS_MCPU2_MSTOP, BIT(13)), 0),
-	DEF_PD("scif0",		R9A08G045_PD_SCIF0,
-				DEF_REG_CONF(CPG_BUS_MCPU2_MSTOP, BIT(1)), 0),
-	DEF_PD("scif1",		R9A08G045_PD_SCIF1,
-				DEF_REG_CONF(CPG_BUS_MCPU2_MSTOP, BIT(2)), 0),
-	DEF_PD("scif2",		R9A08G045_PD_SCIF2,
-				DEF_REG_CONF(CPG_BUS_MCPU2_MSTOP, BIT(3)), 0),
-	DEF_PD("scif3",		R9A08G045_PD_SCIF3,
-				DEF_REG_CONF(CPG_BUS_MCPU2_MSTOP, BIT(4)), 0),
-	DEF_PD("scif4",		R9A08G045_PD_SCIF4,
-				DEF_REG_CONF(CPG_BUS_MCPU2_MSTOP, BIT(5)), 0),
-	DEF_PD("scif5",		R9A08G045_PD_SCIF5,
-				DEF_REG_CONF(CPG_BUS_MCPU3_MSTOP, BIT(4)), 0),
-	DEF_PD("adc",		R9A08G045_PD_ADC,
-				DEF_REG_CONF(CPG_BUS_MCPU2_MSTOP, BIT(14)), 0),
-	DEF_PD("tsu",		R9A08G045_PD_TSU,
-				DEF_REG_CONF(CPG_BUS_MCPU2_MSTOP, BIT(15)), 0),
-	DEF_PD("vbat",		R9A08G045_PD_VBAT,
-				DEF_REG_CONF(CPG_BUS_MCPU3_MSTOP, BIT(8)),
-				GENPD_FLAG_ALWAYS_ON),
-	DEF_PD("rtc",		R9A08G045_PD_RTC,
-				DEF_REG_CONF(CPG_BUS_MCPU3_MSTOP, BIT(7)), 0),
-};
-
 const struct rzg2l_cpg_info r9a08g045_cpg_info = {
 	/* Core Clocks */
 	.core_clks = r9a08g045_core_clks,
@@ -386,9 +360,5 @@ const struct rzg2l_cpg_info r9a08g045_cpg_info = {
 	.resets = r9a08g045_resets,
 	.num_resets = R9A08G045_VBAT_BRESETN + 1, /* Last reset ID + 1 */
 
-	/* Power domains */
-	.pm_domains = r9a08g045_pm_domains,
-	.num_pm_domains = ARRAY_SIZE(r9a08g045_pm_domains),
-
 	.has_clk_mon_regs = true,
 };
-- 
2.43.0

Re: [PATCH v2 5/8] clk: renesas: r9a08g045: Drop power domain instantiation

[Geert Uytterhoeven]

On Wed, 14 May 2025 at 11:04, Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> Since the configuration order between the individual MSTOP and CLKON bits
> cannot be preserved with the power domain abstraction, drop the power
> domain instantiations.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
>
> Changes in v2:
> - added RTC MSTOP bit on the VBATB clock; the RTC and VBATTB clock
>   is gated by the same bit (CPG_CLKON_VBAT, BIT 0) but they have
>   different MSTOP bits (bit 8 for VBAT, bit 7 for RTC); this is
>   not described in HW manual ATM.

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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

[PATCH v2 6/8] clk: renesas: rzg2l-cpg: Drop MSTOP based power domain support

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Since the configuration order between the individual MSTOP and CLKON bits
cannot be preserved with the power domain abstraction, drop the power
domain core code.

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v2:
- collected tags

 drivers/clk/renesas/rzg2l-cpg.c | 208 +++-----------------------------
 drivers/clk/renesas/rzg2l-cpg.h |  51 --------
 2 files changed, 17 insertions(+), 242 deletions(-)

diff --git a/drivers/clk/renesas/rzg2l-cpg.c b/drivers/clk/renesas/rzg2l-cpg.c
index 844a89b37026..be42eeee1814 100644
--- a/drivers/clk/renesas/rzg2l-cpg.c
+++ b/drivers/clk/renesas/rzg2l-cpg.c
@@ -148,6 +148,7 @@ struct rzg2l_pll5_mux_dsi_div_param {
  * @num_resets: Number of Module Resets in info->resets[]
  * @last_dt_core_clk: ID of the last Core Clock exported to DT
  * @info: Pointer to platform data
+ * @genpd: PM domain
  * @mux_dsi_div_params: pll5 mux and dsi div parameters
  */
 struct rzg2l_cpg_priv {
@@ -164,6 +165,8 @@ struct rzg2l_cpg_priv {
 
 	const struct rzg2l_cpg_info *info;
 
+	struct generic_pm_domain genpd;
+
 	struct rzg2l_pll5_mux_dsi_div_param mux_dsi_div_params;
 };
 
@@ -1769,39 +1772,14 @@ static int rzg2l_cpg_reset_controller_register(struct rzg2l_cpg_priv *priv)
 	return devm_reset_controller_register(priv->dev, &priv->rcdev);
 }
 
-/**
- * struct rzg2l_cpg_pm_domains - RZ/G2L PM domains data structure
- * @onecell_data: cell data
- * @domains: generic PM domains
- */
-struct rzg2l_cpg_pm_domains {
-	struct genpd_onecell_data onecell_data;
-	struct generic_pm_domain *domains[];
-};
-
-/**
- * struct rzg2l_cpg_pd - RZ/G2L power domain data structure
- * @genpd: generic PM domain
- * @priv: pointer to CPG private data structure
- * @conf: CPG PM domain configuration info
- * @id: RZ/G2L power domain ID
- */
-struct rzg2l_cpg_pd {
-	struct generic_pm_domain genpd;
-	struct rzg2l_cpg_priv *priv;
-	struct rzg2l_cpg_pm_domain_conf conf;
-	u16 id;
-};
-
-static bool rzg2l_cpg_is_pm_clk(struct rzg2l_cpg_pd *pd,
+static bool rzg2l_cpg_is_pm_clk(struct rzg2l_cpg_priv *priv,
 				const struct of_phandle_args *clkspec)
 {
-	if (clkspec->np != pd->genpd.dev.of_node || clkspec->args_count != 2)
+	if (clkspec->np != priv->genpd.dev.of_node || clkspec->args_count != 2)
 		return false;
 
 	switch (clkspec->args[0]) {
 	case CPG_MOD: {
-		struct rzg2l_cpg_priv *priv = pd->priv;
 		const struct rzg2l_cpg_info *info = priv->info;
 		unsigned int id = clkspec->args[1];
 
@@ -1826,7 +1804,7 @@ static bool rzg2l_cpg_is_pm_clk(struct rzg2l_cpg_pd *pd,
 
 static int rzg2l_cpg_attach_dev(struct generic_pm_domain *domain, struct device *dev)
 {
-	struct rzg2l_cpg_pd *pd = container_of(domain, struct rzg2l_cpg_pd, genpd);
+	struct rzg2l_cpg_priv *priv = container_of(domain, struct rzg2l_cpg_priv, genpd);
 	struct device_node *np = dev->of_node;
 	struct of_phandle_args clkspec;
 	bool once = true;
@@ -1835,7 +1813,7 @@ static int rzg2l_cpg_attach_dev(struct generic_pm_domain *domain, struct device
 	int error;
 
 	for (i = 0; !of_parse_phandle_with_args(np, "clocks", "#clock-cells", i, &clkspec); i++) {
-		if (!rzg2l_cpg_is_pm_clk(pd, &clkspec)) {
+		if (!rzg2l_cpg_is_pm_clk(priv, &clkspec)) {
 			of_node_put(clkspec.np);
 			continue;
 		}
@@ -1880,183 +1858,31 @@ static void rzg2l_cpg_detach_dev(struct generic_pm_domain *unused, struct device
 }
 
 static void rzg2l_cpg_genpd_remove(void *data)
-{
-	struct genpd_onecell_data *celldata = data;
-
-	for (unsigned int i = 0; i < celldata->num_domains; i++)
-		pm_genpd_remove(celldata->domains[i]);
-}
-
-static void rzg2l_cpg_genpd_remove_simple(void *data)
 {
 	pm_genpd_remove(data);
 }
 
-static int rzg2l_cpg_power_on(struct generic_pm_domain *domain)
-{
-	struct rzg2l_cpg_pd *pd = container_of(domain, struct rzg2l_cpg_pd, genpd);
-	struct rzg2l_cpg_reg_conf mstop = pd->conf.mstop;
-	struct rzg2l_cpg_priv *priv = pd->priv;
-
-	/* Set MSTOP. */
-	if (mstop.mask)
-		writel(mstop.mask << 16, priv->base + mstop.off);
-
-	return 0;
-}
-
-static int rzg2l_cpg_power_off(struct generic_pm_domain *domain)
-{
-	struct rzg2l_cpg_pd *pd = container_of(domain, struct rzg2l_cpg_pd, genpd);
-	struct rzg2l_cpg_reg_conf mstop = pd->conf.mstop;
-	struct rzg2l_cpg_priv *priv = pd->priv;
-
-	/* Set MSTOP. */
-	if (mstop.mask)
-		writel(mstop.mask | (mstop.mask << 16), priv->base + mstop.off);
-
-	return 0;
-}
-
-static int __init rzg2l_cpg_pd_setup(struct rzg2l_cpg_pd *pd)
-{
-	bool always_on = !!(pd->genpd.flags & GENPD_FLAG_ALWAYS_ON);
-	struct dev_power_governor *governor;
-	int ret;
-
-	if (always_on)
-		governor = &pm_domain_always_on_gov;
-	else
-		governor = &simple_qos_governor;
-
-	pd->genpd.flags |= GENPD_FLAG_PM_CLK | GENPD_FLAG_ACTIVE_WAKEUP;
-	pd->genpd.attach_dev = rzg2l_cpg_attach_dev;
-	pd->genpd.detach_dev = rzg2l_cpg_detach_dev;
-	pd->genpd.power_on = rzg2l_cpg_power_on;
-	pd->genpd.power_off = rzg2l_cpg_power_off;
-
-	ret = pm_genpd_init(&pd->genpd, governor, !always_on);
-	if (ret)
-		return ret;
-
-	if (always_on)
-		ret = rzg2l_cpg_power_on(&pd->genpd);
-
-	return ret;
-}
-
 static int __init rzg2l_cpg_add_clk_domain(struct rzg2l_cpg_priv *priv)
 {
 	struct device *dev = priv->dev;
 	struct device_node *np = dev->of_node;
-	struct rzg2l_cpg_pd *pd;
-	int ret;
-
-	pd = devm_kzalloc(dev, sizeof(*pd), GFP_KERNEL);
-	if (!pd)
-		return -ENOMEM;
-
-	pd->genpd.name = np->name;
-	pd->genpd.flags = GENPD_FLAG_ALWAYS_ON;
-	pd->priv = priv;
-	ret = rzg2l_cpg_pd_setup(pd);
-	if (ret)
-		return ret;
-
-	ret = devm_add_action_or_reset(dev, rzg2l_cpg_genpd_remove_simple, &pd->genpd);
-	if (ret)
-		return ret;
-
-	return of_genpd_add_provider_simple(np, &pd->genpd);
-}
-
-static struct generic_pm_domain *
-rzg2l_cpg_pm_domain_xlate(const struct of_phandle_args *spec, void *data)
-{
-	struct generic_pm_domain *domain = ERR_PTR(-ENOENT);
-	struct genpd_onecell_data *genpd = data;
-
-	if (spec->args_count != 1)
-		return ERR_PTR(-EINVAL);
-
-	for (unsigned int i = 0; i < genpd->num_domains; i++) {
-		struct rzg2l_cpg_pd *pd = container_of(genpd->domains[i], struct rzg2l_cpg_pd,
-						       genpd);
-
-		if (pd->id == spec->args[0]) {
-			domain = &pd->genpd;
-			break;
-		}
-	}
-
-	return domain;
-}
-
-static int __init rzg2l_cpg_add_pm_domains(struct rzg2l_cpg_priv *priv)
-{
-	const struct rzg2l_cpg_info *info = priv->info;
-	struct device *dev = priv->dev;
-	struct device_node *np = dev->of_node;
-	struct rzg2l_cpg_pm_domains *domains;
-	struct generic_pm_domain *parent;
-	u32 ncells;
+	struct generic_pm_domain *genpd = &priv->genpd;
 	int ret;
 
-	ret = of_property_read_u32(np, "#power-domain-cells", &ncells);
+	genpd->name = np->name;
+	genpd->flags = GENPD_FLAG_PM_CLK | GENPD_FLAG_ALWAYS_ON |
+		       GENPD_FLAG_ACTIVE_WAKEUP;
+	genpd->attach_dev = rzg2l_cpg_attach_dev;
+	genpd->detach_dev = rzg2l_cpg_detach_dev;
+	ret = pm_genpd_init(genpd, &pm_domain_always_on_gov, false);
 	if (ret)
 		return ret;
 
-	/* For backward compatibility. */
-	if (!ncells)
-		return rzg2l_cpg_add_clk_domain(priv);
-
-	domains = devm_kzalloc(dev, struct_size(domains, domains, info->num_pm_domains),
-			       GFP_KERNEL);
-	if (!domains)
-		return -ENOMEM;
-
-	domains->onecell_data.domains = domains->domains;
-	domains->onecell_data.num_domains = info->num_pm_domains;
-	domains->onecell_data.xlate = rzg2l_cpg_pm_domain_xlate;
-
-	ret = devm_add_action_or_reset(dev, rzg2l_cpg_genpd_remove, &domains->onecell_data);
+	ret = devm_add_action_or_reset(dev, rzg2l_cpg_genpd_remove, genpd);
 	if (ret)
 		return ret;
 
-	for (unsigned int i = 0; i < info->num_pm_domains; i++) {
-		struct rzg2l_cpg_pd *pd;
-
-		pd = devm_kzalloc(dev, sizeof(*pd), GFP_KERNEL);
-		if (!pd)
-			return -ENOMEM;
-
-		pd->genpd.name = info->pm_domains[i].name;
-		pd->genpd.flags = info->pm_domains[i].genpd_flags;
-		pd->conf = info->pm_domains[i].conf;
-		pd->id = info->pm_domains[i].id;
-		pd->priv = priv;
-
-		ret = rzg2l_cpg_pd_setup(pd);
-		if (ret)
-			return ret;
-
-		domains->domains[i] = &pd->genpd;
-		/* Parent should be on the very first entry of info->pm_domains[]. */
-		if (!i) {
-			parent = &pd->genpd;
-			continue;
-		}
-
-		ret = pm_genpd_add_subdomain(parent, &pd->genpd);
-		if (ret)
-			return ret;
-	}
-
-	ret = of_genpd_add_provider_onecell(np, &domains->onecell_data);
-	if (ret)
-		return ret;
-
-	return 0;
+	return of_genpd_add_provider_simple(np, genpd);
 }
 
 static int __init rzg2l_cpg_probe(struct platform_device *pdev)
@@ -2119,7 +1945,7 @@ static int __init rzg2l_cpg_probe(struct platform_device *pdev)
 	if (error)
 		return error;
 
-	error = rzg2l_cpg_add_pm_domains(priv);
+	error = rzg2l_cpg_add_clk_domain(priv);
 	if (error)
 		return error;
 
diff --git a/drivers/clk/renesas/rzg2l-cpg.h b/drivers/clk/renesas/rzg2l-cpg.h
index 50a5a23f2e6a..0a71c5ec24b6 100644
--- a/drivers/clk/renesas/rzg2l-cpg.h
+++ b/drivers/clk/renesas/rzg2l-cpg.h
@@ -257,51 +257,6 @@ struct rzg2l_reset {
 #define DEF_RST(_id, _off, _bit)	\
 	DEF_RST_MON(_id, _off, _bit, -1)
 
-/**
- * struct rzg2l_cpg_reg_conf - RZ/G2L register configuration data structure
- * @off: register offset
- * @mask: register mask
- */
-struct rzg2l_cpg_reg_conf {
-	u16 off;
-	u16 mask;
-};
-
-#define DEF_REG_CONF(_off, _mask) ((struct rzg2l_cpg_reg_conf) { .off = (_off), .mask = (_mask) })
-
-/**
- * struct rzg2l_cpg_pm_domain_conf - PM domain configuration data structure
- * @mstop: MSTOP register configuration
- */
-struct rzg2l_cpg_pm_domain_conf {
-	struct rzg2l_cpg_reg_conf mstop;
-};
-
-/**
- * struct rzg2l_cpg_pm_domain_init_data - PM domain init data
- * @name: PM domain name
- * @conf: PM domain configuration
- * @genpd_flags: genpd flags (see GENPD_FLAG_*)
- * @id: PM domain ID (similar to the ones defined in
- *      include/dt-bindings/clock/<soc-id>-cpg.h)
- */
-struct rzg2l_cpg_pm_domain_init_data {
-	const char * const name;
-	struct rzg2l_cpg_pm_domain_conf conf;
-	u32 genpd_flags;
-	u16 id;
-};
-
-#define DEF_PD(_name, _id, _mstop_conf, _flags) \
-	{ \
-		.name = (_name), \
-		.id = (_id), \
-		.conf = { \
-			.mstop = (_mstop_conf), \
-		}, \
-		.genpd_flags = (_flags), \
-	}
-
 /**
  * struct rzg2l_cpg_info - SoC-specific CPG Description
  *
@@ -320,8 +275,6 @@ struct rzg2l_cpg_pm_domain_init_data {
  * @crit_mod_clks: Array with Module Clock IDs of critical clocks that
  *                 should not be disabled without a knowledgeable driver
  * @num_crit_mod_clks: Number of entries in crit_mod_clks[]
- * @pm_domains: PM domains init data array
- * @num_pm_domains: Number of PM domains
  * @has_clk_mon_regs: Flag indicating whether the SoC has CLK_MON registers
  */
 struct rzg2l_cpg_info {
@@ -348,10 +301,6 @@ struct rzg2l_cpg_info {
 	const unsigned int *crit_mod_clks;
 	unsigned int num_crit_mod_clks;
 
-	/* Power domain. */
-	const struct rzg2l_cpg_pm_domain_init_data *pm_domains;
-	unsigned int num_pm_domains;
-
 	bool has_clk_mon_regs;
 };
 
-- 
2.43.0

[PATCH v2 7/8] dt-bindings: clock: rzg2l-cpg: Drop power domain IDs

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Since the configuration order between the individual MSTOP and CLKON bits
cannot be preserved with the power domain abstraction, drop the power
domain IDs. The corresponding code has also been removed. Currently, there
are no device tree users for these IDs.

Acked-by: Rob Herring (Arm) <robh@kernel.org>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v2:
- collected tags

 include/dt-bindings/clock/r9a07g043-cpg.h | 53 -----------------
 include/dt-bindings/clock/r9a07g044-cpg.h | 58 ------------------
 include/dt-bindings/clock/r9a07g054-cpg.h | 58 ------------------
 include/dt-bindings/clock/r9a08g045-cpg.h | 71 -----------------------
 4 files changed, 240 deletions(-)

diff --git a/include/dt-bindings/clock/r9a07g043-cpg.h b/include/dt-bindings/clock/r9a07g043-cpg.h
index 131993343777..e1f65f1928cf 100644
--- a/include/dt-bindings/clock/r9a07g043-cpg.h
+++ b/include/dt-bindings/clock/r9a07g043-cpg.h
@@ -200,57 +200,4 @@
 #define R9A07G043_AX45MP_CORE0_RESETN	78	/* RZ/Five Only */
 #define R9A07G043_IAX45_RESETN		79	/* RZ/Five Only */
 
-/* Power domain IDs. */
-#define R9A07G043_PD_ALWAYS_ON		0
-#define R9A07G043_PD_GIC		1	/* RZ/G2UL Only */
-#define R9A07G043_PD_IA55		2	/* RZ/G2UL Only */
-#define R9A07G043_PD_MHU		3	/* RZ/G2UL Only */
-#define R9A07G043_PD_CORESIGHT		4	/* RZ/G2UL Only */
-#define R9A07G043_PD_SYC		5	/* RZ/G2UL Only */
-#define R9A07G043_PD_DMAC		6
-#define R9A07G043_PD_GTM0		7
-#define R9A07G043_PD_GTM1		8
-#define R9A07G043_PD_GTM2		9
-#define R9A07G043_PD_MTU		10
-#define R9A07G043_PD_POE3		11
-#define R9A07G043_PD_WDT0		12
-#define R9A07G043_PD_SPI		13
-#define R9A07G043_PD_SDHI0		14
-#define R9A07G043_PD_SDHI1		15
-#define R9A07G043_PD_ISU		16	/* RZ/G2UL Only */
-#define R9A07G043_PD_CRU		17	/* RZ/G2UL Only */
-#define R9A07G043_PD_LCDC		18	/* RZ/G2UL Only */
-#define R9A07G043_PD_SSI0		19
-#define R9A07G043_PD_SSI1		20
-#define R9A07G043_PD_SSI2		21
-#define R9A07G043_PD_SSI3		22
-#define R9A07G043_PD_SRC		23
-#define R9A07G043_PD_USB0		24
-#define R9A07G043_PD_USB1		25
-#define R9A07G043_PD_USB_PHY		26
-#define R9A07G043_PD_ETHER0		27
-#define R9A07G043_PD_ETHER1		28
-#define R9A07G043_PD_I2C0		29
-#define R9A07G043_PD_I2C1		30
-#define R9A07G043_PD_I2C2		31
-#define R9A07G043_PD_I2C3		32
-#define R9A07G043_PD_SCIF0		33
-#define R9A07G043_PD_SCIF1		34
-#define R9A07G043_PD_SCIF2		35
-#define R9A07G043_PD_SCIF3		36
-#define R9A07G043_PD_SCIF4		37
-#define R9A07G043_PD_SCI0		38
-#define R9A07G043_PD_SCI1		39
-#define R9A07G043_PD_IRDA		40
-#define R9A07G043_PD_RSPI0		41
-#define R9A07G043_PD_RSPI1		42
-#define R9A07G043_PD_RSPI2		43
-#define R9A07G043_PD_CANFD		44
-#define R9A07G043_PD_ADC		45
-#define R9A07G043_PD_TSU		46
-#define R9A07G043_PD_PLIC		47	/* RZ/Five Only */
-#define R9A07G043_PD_IAX45		48	/* RZ/Five Only */
-#define R9A07G043_PD_NCEPLDM		49	/* RZ/Five Only */
-#define R9A07G043_PD_NCEPLMT		50	/* RZ/Five Only */
-
 #endif /* __DT_BINDINGS_CLOCK_R9A07G043_CPG_H__ */
diff --git a/include/dt-bindings/clock/r9a07g044-cpg.h b/include/dt-bindings/clock/r9a07g044-cpg.h
index e209f96f92b7..0bb17ff1a01a 100644
--- a/include/dt-bindings/clock/r9a07g044-cpg.h
+++ b/include/dt-bindings/clock/r9a07g044-cpg.h
@@ -217,62 +217,4 @@
 #define R9A07G044_ADC_ADRST_N		82
 #define R9A07G044_TSU_PRESETN		83
 
-/* Power domain IDs. */
-#define R9A07G044_PD_ALWAYS_ON		0
-#define R9A07G044_PD_GIC		1
-#define R9A07G044_PD_IA55		2
-#define R9A07G044_PD_MHU		3
-#define R9A07G044_PD_CORESIGHT		4
-#define R9A07G044_PD_SYC		5
-#define R9A07G044_PD_DMAC		6
-#define R9A07G044_PD_GTM0		7
-#define R9A07G044_PD_GTM1		8
-#define R9A07G044_PD_GTM2		9
-#define R9A07G044_PD_MTU		10
-#define R9A07G044_PD_POE3		11
-#define R9A07G044_PD_GPT		12
-#define R9A07G044_PD_POEGA		13
-#define R9A07G044_PD_POEGB		14
-#define R9A07G044_PD_POEGC		15
-#define R9A07G044_PD_POEGD		16
-#define R9A07G044_PD_WDT0		17
-#define R9A07G044_PD_WDT1		18
-#define R9A07G044_PD_SPI		19
-#define R9A07G044_PD_SDHI0		20
-#define R9A07G044_PD_SDHI1		21
-#define R9A07G044_PD_3DGE		22
-#define R9A07G044_PD_ISU		23
-#define R9A07G044_PD_VCPL4		24
-#define R9A07G044_PD_CRU		25
-#define R9A07G044_PD_MIPI_DSI		26
-#define R9A07G044_PD_LCDC		27
-#define R9A07G044_PD_SSI0		28
-#define R9A07G044_PD_SSI1		29
-#define R9A07G044_PD_SSI2		30
-#define R9A07G044_PD_SSI3		31
-#define R9A07G044_PD_SRC		32
-#define R9A07G044_PD_USB0		33
-#define R9A07G044_PD_USB1		34
-#define R9A07G044_PD_USB_PHY		35
-#define R9A07G044_PD_ETHER0		36
-#define R9A07G044_PD_ETHER1		37
-#define R9A07G044_PD_I2C0		38
-#define R9A07G044_PD_I2C1		39
-#define R9A07G044_PD_I2C2		40
-#define R9A07G044_PD_I2C3		41
-#define R9A07G044_PD_SCIF0		42
-#define R9A07G044_PD_SCIF1		43
-#define R9A07G044_PD_SCIF2		44
-#define R9A07G044_PD_SCIF3		45
-#define R9A07G044_PD_SCIF4		46
-#define R9A07G044_PD_SCI0		47
-#define R9A07G044_PD_SCI1		48
-#define R9A07G044_PD_IRDA		49
-#define R9A07G044_PD_RSPI0		50
-#define R9A07G044_PD_RSPI1		51
-#define R9A07G044_PD_RSPI2		52
-#define R9A07G044_PD_CANFD		53
-#define R9A07G044_PD_ADC		54
-#define R9A07G044_PD_TSU		55
-
 #endif /* __DT_BINDINGS_CLOCK_R9A07G044_CPG_H__ */
diff --git a/include/dt-bindings/clock/r9a07g054-cpg.h b/include/dt-bindings/clock/r9a07g054-cpg.h
index 2c99f89397c4..43f4dbda872c 100644
--- a/include/dt-bindings/clock/r9a07g054-cpg.h
+++ b/include/dt-bindings/clock/r9a07g054-cpg.h
@@ -226,62 +226,4 @@
 #define R9A07G054_TSU_PRESETN		83
 #define R9A07G054_STPAI_ARESETN		84
 
-/* Power domain IDs. */
-#define R9A07G054_PD_ALWAYS_ON		0
-#define R9A07G054_PD_GIC		1
-#define R9A07G054_PD_IA55		2
-#define R9A07G054_PD_MHU		3
-#define R9A07G054_PD_CORESIGHT		4
-#define R9A07G054_PD_SYC		5
-#define R9A07G054_PD_DMAC		6
-#define R9A07G054_PD_GTM0		7
-#define R9A07G054_PD_GTM1		8
-#define R9A07G054_PD_GTM2		9
-#define R9A07G054_PD_MTU		10
-#define R9A07G054_PD_POE3		11
-#define R9A07G054_PD_GPT		12
-#define R9A07G054_PD_POEGA		13
-#define R9A07G054_PD_POEGB		14
-#define R9A07G054_PD_POEGC		15
-#define R9A07G054_PD_POEGD		16
-#define R9A07G054_PD_WDT0		17
-#define R9A07G054_PD_WDT1		18
-#define R9A07G054_PD_SPI		19
-#define R9A07G054_PD_SDHI0		20
-#define R9A07G054_PD_SDHI1		21
-#define R9A07G054_PD_3DGE		22
-#define R9A07G054_PD_ISU		23
-#define R9A07G054_PD_VCPL4		24
-#define R9A07G054_PD_CRU		25
-#define R9A07G054_PD_MIPI_DSI		26
-#define R9A07G054_PD_LCDC		27
-#define R9A07G054_PD_SSI0		28
-#define R9A07G054_PD_SSI1		29
-#define R9A07G054_PD_SSI2		30
-#define R9A07G054_PD_SSI3		31
-#define R9A07G054_PD_SRC		32
-#define R9A07G054_PD_USB0		33
-#define R9A07G054_PD_USB1		34
-#define R9A07G054_PD_USB_PHY		35
-#define R9A07G054_PD_ETHER0		36
-#define R9A07G054_PD_ETHER1		37
-#define R9A07G054_PD_I2C0		38
-#define R9A07G054_PD_I2C1		39
-#define R9A07G054_PD_I2C2		40
-#define R9A07G054_PD_I2C3		41
-#define R9A07G054_PD_SCIF0		42
-#define R9A07G054_PD_SCIF1		43
-#define R9A07G054_PD_SCIF2		44
-#define R9A07G054_PD_SCIF3		45
-#define R9A07G054_PD_SCIF4		46
-#define R9A07G054_PD_SCI0		47
-#define R9A07G054_PD_SCI1		48
-#define R9A07G054_PD_IRDA		49
-#define R9A07G054_PD_RSPI0		50
-#define R9A07G054_PD_RSPI1		51
-#define R9A07G054_PD_RSPI2		52
-#define R9A07G054_PD_CANFD		53
-#define R9A07G054_PD_ADC		54
-#define R9A07G054_PD_TSU		55
-
 #endif /* __DT_BINDINGS_CLOCK_R9A07G054_CPG_H__ */
diff --git a/include/dt-bindings/clock/r9a08g045-cpg.h b/include/dt-bindings/clock/r9a08g045-cpg.h
index 311521fe4b59..410725b778a8 100644
--- a/include/dt-bindings/clock/r9a08g045-cpg.h
+++ b/include/dt-bindings/clock/r9a08g045-cpg.h
@@ -239,75 +239,4 @@
 #define R9A08G045_I3C_PRESETN		92
 #define R9A08G045_VBAT_BRESETN		93
 
-/* Power domain IDs. */
-#define R9A08G045_PD_ALWAYS_ON		0
-#define R9A08G045_PD_GIC		1
-#define R9A08G045_PD_IA55		2
-#define R9A08G045_PD_MHU		3
-#define R9A08G045_PD_CORESIGHT		4
-#define R9A08G045_PD_SYC		5
-#define R9A08G045_PD_DMAC		6
-#define R9A08G045_PD_GTM0		7
-#define R9A08G045_PD_GTM1		8
-#define R9A08G045_PD_GTM2		9
-#define R9A08G045_PD_GTM3		10
-#define R9A08G045_PD_GTM4		11
-#define R9A08G045_PD_GTM5		12
-#define R9A08G045_PD_GTM6		13
-#define R9A08G045_PD_GTM7		14
-#define R9A08G045_PD_MTU		15
-#define R9A08G045_PD_POE3		16
-#define R9A08G045_PD_GPT		17
-#define R9A08G045_PD_POEGA		18
-#define R9A08G045_PD_POEGB		19
-#define R9A08G045_PD_POEGC		20
-#define R9A08G045_PD_POEGD		21
-#define R9A08G045_PD_WDT0		22
-#define R9A08G045_PD_XSPI		23
-#define R9A08G045_PD_SDHI0		24
-#define R9A08G045_PD_SDHI1		25
-#define R9A08G045_PD_SDHI2		26
-#define R9A08G045_PD_SSI0		27
-#define R9A08G045_PD_SSI1		28
-#define R9A08G045_PD_SSI2		29
-#define R9A08G045_PD_SSI3		30
-#define R9A08G045_PD_SRC		31
-#define R9A08G045_PD_USB0		32
-#define R9A08G045_PD_USB1		33
-#define R9A08G045_PD_USB_PHY		34
-#define R9A08G045_PD_ETHER0		35
-#define R9A08G045_PD_ETHER1		36
-#define R9A08G045_PD_I2C0		37
-#define R9A08G045_PD_I2C1		38
-#define R9A08G045_PD_I2C2		39
-#define R9A08G045_PD_I2C3		40
-#define R9A08G045_PD_SCIF0		41
-#define R9A08G045_PD_SCIF1		42
-#define R9A08G045_PD_SCIF2		43
-#define R9A08G045_PD_SCIF3		44
-#define R9A08G045_PD_SCIF4		45
-#define R9A08G045_PD_SCIF5		46
-#define R9A08G045_PD_SCI0		47
-#define R9A08G045_PD_SCI1		48
-#define R9A08G045_PD_IRDA		49
-#define R9A08G045_PD_RSPI0		50
-#define R9A08G045_PD_RSPI1		51
-#define R9A08G045_PD_RSPI2		52
-#define R9A08G045_PD_RSPI3		53
-#define R9A08G045_PD_RSPI4		54
-#define R9A08G045_PD_CANFD		55
-#define R9A08G045_PD_ADC		56
-#define R9A08G045_PD_TSU		57
-#define R9A08G045_PD_OCTA		58
-#define R9A08G045_PD_PDM		59
-#define R9A08G045_PD_PCI		60
-#define R9A08G045_PD_SPDIF		61
-#define R9A08G045_PD_I3C		62
-#define R9A08G045_PD_VBAT		63
-
-#define R9A08G045_PD_DDR		64
-#define R9A08G045_PD_TZCDDR		65
-#define R9A08G045_PD_OTFDE_DDR		66
-#define R9A08G045_PD_RTC		67
-
 #endif /* __DT_BINDINGS_CLOCK_R9A08G045_CPG_H__ */
-- 
2.43.0

[PATCH v2 8/8] Revert "dt-bindings: clock: renesas,rzg2l-cpg: Update #power-domain-cells = <1> for RZ/G3S"

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

This reverts commit f33dca9ed6f41c8acf2c17c402738deddb7d7c28.
Since the configuration order between the individual MSTOP and CLKON bits
cannot be preserved with the power domain abstraction, drop the
Currently, there are no device tree users for #power-domain-cell = <1>.

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Acked-by: Rob Herring (Arm) <robh@kernel.org>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v2:
- collected tags

 .../bindings/clock/renesas,rzg2l-cpg.yaml      | 18 +-----------------
 1 file changed, 1 insertion(+), 17 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/renesas,rzg2l-cpg.yaml b/Documentation/devicetree/bindings/clock/renesas,rzg2l-cpg.yaml
index 0440f23da059..8c18616e5c4d 100644
--- a/Documentation/devicetree/bindings/clock/renesas,rzg2l-cpg.yaml
+++ b/Documentation/devicetree/bindings/clock/renesas,rzg2l-cpg.yaml
@@ -57,8 +57,7 @@ properties:
       can be power-managed through Module Standby should refer to the CPG device
       node in their "power-domains" property, as documented by the generic PM
       Domain bindings in Documentation/devicetree/bindings/power/power-domain.yaml.
-      The power domain specifiers defined in <dt-bindings/clock/r9a0*-cpg.h> could
-      be used to reference individual CPG power domains.
+    const: 0
 
   '#reset-cells':
     description:
@@ -77,21 +76,6 @@ required:
 
 additionalProperties: false
 
-allOf:
-  - if:
-      properties:
-        compatible:
-          contains:
-            const: renesas,r9a08g045-cpg
-    then:
-      properties:
-        '#power-domain-cells':
-          const: 1
-    else:
-      properties:
-        '#power-domain-cells':
-          const: 0
-
 examples:
   - |
     cpg: clock-controller@11010000 {
-- 
2.43.0