[PATCH v2 0/4] Add GbEthernet Clock support

[PATCH v2 0/4] Add GbEthernet Clock support

[Biju Das]

This patch series aims to add GbEthernet clock support.
GbEthernet clock support involves handing mux clock support
for HP clock and coupled clock for axi/chi module clocks which
shares same bit for controlling the clock output.

This patch series is based on renesas-clk-for-v5.15.

v1->v2:
 * No change. Separated clock patches from driver patch series as per [1]
 [1]
  https://www.spinics.net/lists/linux-renesas-soc/msg59067.html
v1:-
 * New patch

Biju Das (4):
  drivers: clk: renesas: rzg2l-cpg: Add support to handle MUX clocks
  drivers: clk: renesas: r9a07g044-cpg: Add ethernet clock sources
  drivers: clk: renesas: rzg2l-cpg: Add support to handle coupled clocks
  drivers: clk: renesas: r9a07g044-cpg: Add GbEthernet clock/reset

 drivers/clk/renesas/r9a07g044-cpg.c | 31 ++++++++++++++++-
 drivers/clk/renesas/rzg2l-cpg.c     | 54 +++++++++++++++++++++++++++++
 drivers/clk/renesas/rzg2l-cpg.h     | 26 +++++++++++++-
 3 files changed, 109 insertions(+), 2 deletions(-)

-- 
2.17.1

[PATCH v2 1/4] drivers: clk: renesas: rzg2l-cpg: Add support to handle MUX clocks

[Biju Das]

Add support to handle mux clocks in order to select a clock source
from multiple sources.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v1->v2:
  * Moved SEL_PLL_PACK macro to here
  * Fixed the commit message and extra blank line as pointed by Sergei
  * Added Geert's Rb tag
v1:
  * New patch.
---
 drivers/clk/renesas/rzg2l-cpg.c | 23 +++++++++++++++++++++++
 drivers/clk/renesas/rzg2l-cpg.h | 12 ++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/drivers/clk/renesas/rzg2l-cpg.c b/drivers/clk/renesas/rzg2l-cpg.c
index 3b3b2c3347f3..597efc2504eb 100644
--- a/drivers/clk/renesas/rzg2l-cpg.c
+++ b/drivers/clk/renesas/rzg2l-cpg.c
@@ -130,6 +130,26 @@ rzg2l_cpg_div_clk_register(const struct cpg_core_clk *core,
 	return clk_hw->clk;
 }
 
+static struct clk * __init
+rzg2l_cpg_mux_clk_register(const struct cpg_core_clk *core,
+			   void __iomem *base,
+			   struct rzg2l_cpg_priv *priv)
+{
+	const struct clk_hw *clk_hw;
+
+	clk_hw = devm_clk_hw_register_mux(priv->dev, core->name,
+					  core->parent_names, core->num_parents,
+					  core->flag,
+					  base + GET_REG_OFFSET(core->conf),
+					  GET_SHIFT(core->conf),
+					  GET_WIDTH(core->conf),
+					  core->mux_flags, &priv->rmw_lock);
+	if (IS_ERR(clk_hw))
+		return ERR_CAST(clk_hw);
+
+	return clk_hw->clk;
+}
+
 struct pll_clk {
 	struct clk_hw hw;
 	unsigned int conf;
@@ -288,6 +308,9 @@ rzg2l_cpg_register_core_clk(const struct cpg_core_clk *core,
 		clk = rzg2l_cpg_div_clk_register(core, priv->clks,
 						 priv->base, priv);
 		break;
+	case CLK_TYPE_MUX:
+		clk = rzg2l_cpg_mux_clk_register(core, priv->base, priv);
+		break;
 	default:
 		goto fail;
 	}
diff --git a/drivers/clk/renesas/rzg2l-cpg.h b/drivers/clk/renesas/rzg2l-cpg.h
index 63695280ce8b..f538ffa3371c 100644
--- a/drivers/clk/renesas/rzg2l-cpg.h
+++ b/drivers/clk/renesas/rzg2l-cpg.h
@@ -24,6 +24,9 @@
 #define DIVPL3A		DDIV_PACK(CPG_PL3A_DDIV, 0, 3)
 #define DIVPL3B		DDIV_PACK(CPG_PL3A_DDIV, 4, 3)
 
+#define SEL_PLL_PACK(offset, bitpos, size) \
+		(((offset) << 20) | ((bitpos) << 12) | ((size) << 8))
+
 /**
  * Definitions of CPG Core Clocks
  *
@@ -43,6 +46,7 @@ struct cpg_core_clk {
 	const struct clk_div_table *dtable;
 	const char * const *parent_names;
 	int flag;
+	int mux_flags;
 	int num_parents;
 };
 
@@ -54,6 +58,9 @@ enum clk_types {
 
 	/* Clock with divider */
 	CLK_TYPE_DIV,
+
+	/* Clock with clock source selector */
+	CLK_TYPE_MUX,
 };
 
 #define DEF_TYPE(_name, _id, _type...) \
@@ -69,6 +76,11 @@ enum clk_types {
 #define DEF_DIV(_name, _id, _parent, _conf, _dtable, _flag) \
 	DEF_TYPE(_name, _id, CLK_TYPE_DIV, .conf = _conf, \
 		 .parent = _parent, .dtable = _dtable, .flag = _flag)
+#define DEF_MUX(_name, _id, _conf, _parent_names, _num_parents, _flag, \
+		_mux_flags) \
+	DEF_TYPE(_name, _id, CLK_TYPE_MUX, .conf = _conf, \
+		 .parent_names = _parent_names, .num_parents = _num_parents, \
+		 .flag = _flag, .mux_flags = _mux_flags)
 
 /**
  * struct rzg2l_mod_clk - Module Clocks definitions
-- 
2.17.1

[PATCH v2 2/4] drivers: clk: renesas: r9a07g044-cpg: Add ethernet clock sources

[Biju Das]

Ethernet reference clock can be sourced from PLL5_2 or PLL6_2. Add support
for ethernet source clock selection using SEL_PLL_6_2 mux.

This patch also renames the PLL5_DIV2 core clock to PLL5_2_DIV12 to match
with the register description as mentioned in RZ/G2L HW manual (Rev.0.50).

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
v1->v2:
 * Moved SEL_PLL_PACK macro to Mux handling support
 * Renamed PLL5_DIV2 core clock to PLL5_2_DIV12
v1:
 * New patch.
---
 drivers/clk/renesas/r9a07g044-cpg.c | 21 ++++++++++++++++++++-
 drivers/clk/renesas/rzg2l-cpg.h     |  3 +++
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/renesas/r9a07g044-cpg.c b/drivers/clk/renesas/r9a07g044-cpg.c
index 4c94b94c4125..acf19a6cde31 100644
--- a/drivers/clk/renesas/r9a07g044-cpg.c
+++ b/drivers/clk/renesas/r9a07g044-cpg.c
@@ -35,8 +35,11 @@ enum clk_ids {
 	CLK_PLL3_DIV4,
 	CLK_PLL4,
 	CLK_PLL5,
-	CLK_PLL5_DIV2,
+	CLK_PLL5_2,
+	CLK_PLL5_2_DIV12,
 	CLK_PLL6,
+	CLK_PLL6_2,
+	CLK_PLL6_2_DIV2,
 	CLK_P1_DIV2,
 
 	/* Module Clocks */
@@ -53,6 +56,9 @@ static const struct clk_div_table dtable_1_32[] = {
 	{0, 0},
 };
 
+/* Mux clock tables */
+static const char * const sel_pll6_2[]	= { ".pll6_2_div2", ".pll5_2_div12" };
+
 static const struct cpg_core_clk r9a07g044_core_clks[] __initconst = {
 	/* External Clock Inputs */
 	DEF_INPUT("extal", CLK_EXTAL),
@@ -64,6 +70,12 @@ static const struct cpg_core_clk r9a07g044_core_clks[] __initconst = {
 	DEF_FIXED(".pll2", CLK_PLL2, CLK_EXTAL, 133, 2),
 	DEF_FIXED(".pll3", CLK_PLL3, CLK_EXTAL, 133, 2),
 
+	DEF_FIXED(".pll5", CLK_PLL5, CLK_EXTAL, 125, 1),
+	DEF_FIXED(".pll5_2", CLK_PLL5_2, CLK_PLL5, 1, 6),
+
+	DEF_FIXED(".pll6", CLK_PLL6, CLK_EXTAL, 125, 6),
+	DEF_FIXED(".pll6_2", CLK_PLL6_2, CLK_PLL6, 1, 1),
+
 	DEF_FIXED(".pll2_div2", CLK_PLL2_DIV2, CLK_PLL2, 1, 2),
 	DEF_FIXED(".pll2_div16", CLK_PLL2_DIV16, CLK_PLL2, 1, 16),
 	DEF_FIXED(".pll2_div20", CLK_PLL2_DIV20, CLK_PLL2, 1, 20),
@@ -73,6 +85,9 @@ static const struct cpg_core_clk r9a07g044_core_clks[] __initconst = {
 	DEF_FIXED(".pll3_div2_4_2", CLK_PLL3_DIV2_4_2, CLK_PLL3_DIV2_4, 1, 2),
 	DEF_FIXED(".pll3_div4", CLK_PLL3_DIV4, CLK_PLL3, 1, 4),
 
+	DEF_FIXED(".pll5_2_div12", CLK_PLL5_2_DIV12, CLK_PLL5_2, 1, 2),
+	DEF_FIXED(".pll6_2_div2", CLK_PLL6_2_DIV2, CLK_PLL6_2, 1, 2),
+
 	/* Core output clk */
 	DEF_FIXED("I", R9A07G044_CLK_I, CLK_PLL1, 1, 1),
 	DEF_DIV("P0", R9A07G044_CLK_P0, CLK_PLL2_DIV16, DIVPL2A,
@@ -84,6 +99,10 @@ static const struct cpg_core_clk r9a07g044_core_clks[] __initconst = {
 	DEF_FIXED("P1_DIV2", CLK_P1_DIV2, R9A07G044_CLK_P1, 1, 2),
 	DEF_DIV("P2", R9A07G044_CLK_P2, CLK_PLL3_DIV2_4_2,
 		DIVPL3A, dtable_1_32, CLK_DIVIDER_HIWORD_MASK),
+	DEF_FIXED("M0", R9A07G044_CLK_M0, CLK_PLL3_DIV2_4, 1, 1),
+	DEF_FIXED("ZT", R9A07G044_CLK_ZT, CLK_PLL3_DIV2_4_2, 1, 1),
+	DEF_MUX("HP", R9A07G044_CLK_HP, SEL_PLL6_2,
+		sel_pll6_2, ARRAY_SIZE(sel_pll6_2), 0, CLK_MUX_HIWORD_MASK),
 };
 
 static struct rzg2l_mod_clk r9a07g044_mod_clks[] = {
diff --git a/drivers/clk/renesas/rzg2l-cpg.h b/drivers/clk/renesas/rzg2l-cpg.h
index f538ffa3371c..5202c0512483 100644
--- a/drivers/clk/renesas/rzg2l-cpg.h
+++ b/drivers/clk/renesas/rzg2l-cpg.h
@@ -11,6 +11,7 @@
 
 #define CPG_PL2_DDIV		(0x204)
 #define CPG_PL3A_DDIV		(0x208)
+#define CPG_PL6_ETH_SSEL	(0x418)
 
 /* n = 0/1/2 for PLL1/4/6 */
 #define CPG_SAMPLL_CLK1(n)	(0x04 + (16 * n))
@@ -27,6 +28,8 @@
 #define SEL_PLL_PACK(offset, bitpos, size) \
 		(((offset) << 20) | ((bitpos) << 12) | ((size) << 8))
 
+#define SEL_PLL6_2	SEL_PLL_PACK(CPG_PL6_ETH_SSEL, 0, 1)
+
 /**
  * Definitions of CPG Core Clocks
  *
-- 
2.17.1

[PATCH v2 3/4] drivers: clk: renesas: rzg2l-cpg: Add support to handle coupled clocks

[Biju Das]

The AXI and CHI clocks use the same register bit for controlling clock
output. Add a new clock type for coupled clocks, which sets the
CPG_CLKON_ETH.CLK[01]_ON bit when at least one clock is enabled, and
clears the bit only when both clocks are disabled.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
v2:-
 * New patch
---
 drivers/clk/renesas/rzg2l-cpg.c | 31 +++++++++++++++++++++++++++++++
 drivers/clk/renesas/rzg2l-cpg.h | 11 ++++++++++-
 2 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/renesas/rzg2l-cpg.c b/drivers/clk/renesas/rzg2l-cpg.c
index 597efc2504eb..4d2af113b54e 100644
--- a/drivers/clk/renesas/rzg2l-cpg.c
+++ b/drivers/clk/renesas/rzg2l-cpg.c
@@ -333,12 +333,16 @@ rzg2l_cpg_register_core_clk(const struct cpg_core_clk *core,
  * @hw: handle between common and hardware-specific interfaces
  * @off: register offset
  * @bit: ON/MON bit
+ * @is_coupled: flag to indicate coupled clock
+ * @on_cnt: ON count for coupled clocks
  * @priv: CPG/MSTP private data
  */
 struct mstp_clock {
 	struct clk_hw hw;
 	u16 off;
 	u8 bit;
+	bool is_coupled;
+	u8 on_cnt;
 	struct rzg2l_cpg_priv *priv;
 };
 
@@ -392,11 +396,37 @@ static int rzg2l_mod_clock_endisable(struct clk_hw *hw, bool enable)
 
 static int rzg2l_mod_clock_enable(struct clk_hw *hw)
 {
+	struct mstp_clock *clock = to_mod_clock(hw);
+	struct rzg2l_cpg_priv *priv = clock->priv;
+	unsigned long flags;
+
+	spin_lock_irqsave(&priv->rmw_lock, flags);
+	clock->on_cnt++;
+	if (clock->is_coupled && clock->on_cnt > 1) {
+		spin_unlock_irqrestore(&priv->rmw_lock, flags);
+		return 1;
+	}
+
+	spin_unlock_irqrestore(&priv->rmw_lock, flags);
+
 	return rzg2l_mod_clock_endisable(hw, true);
 }
 
 static void rzg2l_mod_clock_disable(struct clk_hw *hw)
 {
+	struct mstp_clock *clock = to_mod_clock(hw);
+	struct rzg2l_cpg_priv *priv = clock->priv;
+	unsigned long flags;
+
+	spin_lock_irqsave(&priv->rmw_lock, flags);
+	clock->on_cnt--;
+	if (clock->is_coupled && clock->on_cnt) {
+		spin_unlock_irqrestore(&priv->rmw_lock, flags);
+		return;
+	}
+
+	spin_unlock_irqrestore(&priv->rmw_lock, flags);
+
 	rzg2l_mod_clock_endisable(hw, false);
 }
 
@@ -475,6 +505,7 @@ rzg2l_cpg_register_mod_clk(const struct rzg2l_mod_clk *mod,
 
 	clock->off = mod->off;
 	clock->bit = mod->bit;
+	clock->is_coupled = mod->is_coupled;
 	clock->priv = priv;
 	clock->hw.init = &init;
 
diff --git a/drivers/clk/renesas/rzg2l-cpg.h b/drivers/clk/renesas/rzg2l-cpg.h
index 5202c0512483..191c403aa52f 100644
--- a/drivers/clk/renesas/rzg2l-cpg.h
+++ b/drivers/clk/renesas/rzg2l-cpg.h
@@ -93,6 +93,7 @@ enum clk_types {
  * @parent: id of parent clock
  * @off: register offset
  * @bit: ON/MON bit
+ * @is_coupled: flag to indicate coupled clock
  */
 struct rzg2l_mod_clk {
 	const char *name;
@@ -100,17 +101,25 @@ struct rzg2l_mod_clk {
 	unsigned int parent;
 	u16 off;
 	u8 bit;
+	bool is_coupled;
 };
 
-#define DEF_MOD(_name, _id, _parent, _off, _bit)	\
+#define DEF_MOD_BASE(_name, _id, _parent, _off, _bit, _is_coupled)	\
 	{ \
 		.name = _name, \
 		.id = MOD_CLK_BASE + (_id), \
 		.parent = (_parent), \
 		.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_COUPLED(_name, _id, _parent, _off, _bit)	\
+	DEF_MOD_BASE(_name, _id, _parent, _off, _bit, true)
+
 /**
  * struct rzg2l_reset - Reset definitions
  *
-- 
2.17.1

[PATCH v2 4/4] drivers: clk: renesas: r9a07g044-cpg: Add GbEthernet clock/reset

[Biju Das]

Add ETH{0,1} clock/reset entries to CPG driver.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
v1->v2:
 * Register axi/chi clock as coupled clocks
v1:-
 * New patch
---
 drivers/clk/renesas/r9a07g044-cpg.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/clk/renesas/r9a07g044-cpg.c b/drivers/clk/renesas/r9a07g044-cpg.c
index acf19a6cde31..1745e363e5a6 100644
--- a/drivers/clk/renesas/r9a07g044-cpg.c
+++ b/drivers/clk/renesas/r9a07g044-cpg.c
@@ -140,6 +140,14 @@ static struct rzg2l_mod_clk r9a07g044_mod_clks[] = {
 				0x578, 2),
 	DEF_MOD("usb_pclk",	R9A07G044_USB_PCLK, R9A07G044_CLK_P1,
 				0x578, 3),
+	DEF_COUPLED("eth0_axi",	R9A07G044_ETH0_CLK_AXI, R9A07G044_CLK_M0,
+				0x57c, 0),
+	DEF_COUPLED("eth0_chi",	R9A07G044_ETH0_CLK_CHI, R9A07G044_CLK_ZT,
+				0x57c, 0),
+	DEF_COUPLED("eth1_axi",	R9A07G044_ETH1_CLK_AXI, R9A07G044_CLK_M0,
+				0x57c, 1),
+	DEF_COUPLED("eth1_chi",	R9A07G044_ETH1_CLK_CHI, R9A07G044_CLK_ZT,
+				0x57c, 1),
 	DEF_MOD("i2c0",		R9A07G044_I2C0_PCLK, R9A07G044_CLK_P0,
 				0x580, 0),
 	DEF_MOD("i2c1",		R9A07G044_I2C1_PCLK, R9A07G044_CLK_P0,
@@ -184,6 +192,8 @@ static struct rzg2l_reset r9a07g044_resets[] = {
 	DEF_RST(R9A07G044_USB_U2H1_HRESETN, 0x878, 1),
 	DEF_RST(R9A07G044_USB_U2P_EXL_SYSRST, 0x878, 2),
 	DEF_RST(R9A07G044_USB_PRESETN, 0x878, 3),
+	DEF_RST(R9A07G044_ETH0_RST_HW_N, 0x87c, 0),
+	DEF_RST(R9A07G044_ETH1_RST_HW_N, 0x87c, 1),
 	DEF_RST(R9A07G044_I2C0_MRST, 0x880, 0),
 	DEF_RST(R9A07G044_I2C1_MRST, 0x880, 1),
 	DEF_RST(R9A07G044_I2C2_MRST, 0x880, 2),
-- 
2.17.1

Re: [PATCH v2 3/4] drivers: clk: renesas: rzg2l-cpg: Add support to handle coupled clocks

[Geert Uytterhoeven]

Hi Biju,

On Tue, Jul 27, 2021 at 4:18 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> The AXI and CHI clocks use the same register bit for controlling clock
> output. Add a new clock type for coupled clocks, which sets the
> CPG_CLKON_ETH.CLK[01]_ON bit when at least one clock is enabled, and
> clears the bit only when both clocks are disabled.
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Thanks for your patch!

> --- a/drivers/clk/renesas/rzg2l-cpg.c
> +++ b/drivers/clk/renesas/rzg2l-cpg.c
> @@ -333,12 +333,16 @@ rzg2l_cpg_register_core_clk(const struct cpg_core_clk *core,
>   * @hw: handle between common and hardware-specific interfaces
>   * @off: register offset
>   * @bit: ON/MON bit
> + * @is_coupled: flag to indicate coupled clock
> + * @on_cnt: ON count for coupled clocks
>   * @priv: CPG/MSTP private data
>   */
>  struct mstp_clock {
>         struct clk_hw hw;
>         u16 off;
>         u8 bit;
> +       bool is_coupled;
> +       u8 on_cnt;

While u8 is probably sufficient, you may want to use unsigned int,
as there will be a gap anyway due to alignment rules.

>         struct rzg2l_cpg_priv *priv;
>  };
>
> @@ -392,11 +396,37 @@ static int rzg2l_mod_clock_endisable(struct clk_hw *hw, bool enable)
>
>  static int rzg2l_mod_clock_enable(struct clk_hw *hw)
>  {
> +       struct mstp_clock *clock = to_mod_clock(hw);
> +       struct rzg2l_cpg_priv *priv = clock->priv;
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&priv->rmw_lock, flags);
> +       clock->on_cnt++;
> +       if (clock->is_coupled && clock->on_cnt > 1) {
> +               spin_unlock_irqrestore(&priv->rmw_lock, flags);
> +               return 1;
> +       }
> +
> +       spin_unlock_irqrestore(&priv->rmw_lock, flags);

I think you can avoid taking the spinlock and touching the counter
if the is_coupled flag is not set.

> +
>         return rzg2l_mod_clock_endisable(hw, true);
>  }

However, I'm wondering how this can work?

      DEF_COUPLED("eth0_axi", R9A07G044_ETH0_CLK_AXI, R9A07G044_CLK_M0,
                              0x57c, 0),
      DEF_COUPLED("eth0_chi", R9A07G044_ETH0_CLK_CHI, R9A07G044_CLK_ZT,
                              0x57c, 0),

This will create 2 independent clocks, each with their own mstp_clock
structure that has the is_coupled flag set.  Hence each clock has
its own counter. Shouldn't the counter be shared?
Am I missing something?

And what about rzg2l_mod_clock_is_enabled()?
Shouldn't it reflect the soft state instead of the shared hardware
state?

>  static void rzg2l_mod_clock_disable(struct clk_hw *hw)
>  {
> +       struct mstp_clock *clock = to_mod_clock(hw);
> +       struct rzg2l_cpg_priv *priv = clock->priv;
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&priv->rmw_lock, flags);
> +       clock->on_cnt--;
> +       if (clock->is_coupled && clock->on_cnt) {
> +               spin_unlock_irqrestore(&priv->rmw_lock, flags);
> +               return;
> +       }
> +
> +       spin_unlock_irqrestore(&priv->rmw_lock, flags);
> +
>         rzg2l_mod_clock_endisable(hw, false);
>  }
>
> @@ -475,6 +505,7 @@ rzg2l_cpg_register_mod_clk(const struct rzg2l_mod_clk *mod,
>
>         clock->off = mod->off;
>         clock->bit = mod->bit;
> +       clock->is_coupled = mod->is_coupled;
>         clock->priv = priv;
>         clock->hw.init = &init;
>
> diff --git a/drivers/clk/renesas/rzg2l-cpg.h b/drivers/clk/renesas/rzg2l-cpg.h
> index 5202c0512483..191c403aa52f 100644
> --- a/drivers/clk/renesas/rzg2l-cpg.h
> +++ b/drivers/clk/renesas/rzg2l-cpg.h
> @@ -93,6 +93,7 @@ enum clk_types {
>   * @parent: id of parent clock
>   * @off: register offset
>   * @bit: ON/MON bit
> + * @is_coupled: flag to indicate coupled clock
>   */
>  struct rzg2l_mod_clk {
>         const char *name;
> @@ -100,17 +101,25 @@ struct rzg2l_mod_clk {
>         unsigned int parent;
>         u16 off;
>         u8 bit;
> +       bool is_coupled;
>  };
>
> -#define DEF_MOD(_name, _id, _parent, _off, _bit)       \
> +#define DEF_MOD_BASE(_name, _id, _parent, _off, _bit, _is_coupled)     \
>         { \
>                 .name = _name, \
>                 .id = MOD_CLK_BASE + (_id), \
>                 .parent = (_parent), \
>                 .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_COUPLED(_name, _id, _parent, _off, _bit)   \
> +       DEF_MOD_BASE(_name, _id, _parent, _off, _bit, true)
> +
>  /**
>   * struct rzg2l_reset - Reset definitions
>   *

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 3/4] drivers: clk: renesas: rzg2l-cpg: Add support to handle coupled clocks

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH v2 3/4] drivers: clk: renesas: rzg2l-cpg: Add support
> to handle coupled clocks
> 
> Hi Biju,
> 
> On Tue, Jul 27, 2021 at 4:18 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > The AXI and CHI clocks use the same register bit for controlling clock
> > output. Add a new clock type for coupled clocks, which sets the
> > CPG_CLKON_ETH.CLK[01]_ON bit when at least one clock is enabled, and
> > clears the bit only when both clocks are disabled.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Thanks for your patch!
> 
> > --- a/drivers/clk/renesas/rzg2l-cpg.c
> > +++ b/drivers/clk/renesas/rzg2l-cpg.c
> > @@ -333,12 +333,16 @@ rzg2l_cpg_register_core_clk(const struct
> cpg_core_clk *core,
> >   * @hw: handle between common and hardware-specific interfaces
> >   * @off: register offset
> >   * @bit: ON/MON bit
> > + * @is_coupled: flag to indicate coupled clock
> > + * @on_cnt: ON count for coupled clocks
> >   * @priv: CPG/MSTP private data
> >   */
> >  struct mstp_clock {
> >         struct clk_hw hw;
> >         u16 off;
> >         u8 bit;
> > +       bool is_coupled;
> > +       u8 on_cnt;
> 
> While u8 is probably sufficient, you may want to use unsigned int, as
> there will be a gap anyway due to alignment rules.
> 
> >         struct rzg2l_cpg_priv *priv;
> >  };
> >
> > @@ -392,11 +396,37 @@ static int rzg2l_mod_clock_endisable(struct
> > clk_hw *hw, bool enable)
> >
> >  static int rzg2l_mod_clock_enable(struct clk_hw *hw)  {
> > +       struct mstp_clock *clock = to_mod_clock(hw);
> > +       struct rzg2l_cpg_priv *priv = clock->priv;
> > +       unsigned long flags;
> > +
> > +       spin_lock_irqsave(&priv->rmw_lock, flags);
> > +       clock->on_cnt++;
> > +       if (clock->is_coupled && clock->on_cnt > 1) {
> > +               spin_unlock_irqrestore(&priv->rmw_lock, flags);
> > +               return 1;
> > +       }
> > +
> > +       spin_unlock_irqrestore(&priv->rmw_lock, flags);
> 
> I think you can avoid taking the spinlock and touching the counter if the
> is_coupled flag is not set.

OK.

> 
> > +
> >         return rzg2l_mod_clock_endisable(hw, true);  }
> 
> However, I'm wondering how this can work?
> 
>       DEF_COUPLED("eth0_axi", R9A07G044_ETH0_CLK_AXI, R9A07G044_CLK_M0,
>                               0x57c, 0),
>       DEF_COUPLED("eth0_chi", R9A07G044_ETH0_CLK_CHI, R9A07G044_CLK_ZT,
>                               0x57c, 0),
> 
> This will create 2 independent clocks, each with their own mstp_clock
> structure that has the is_coupled flag set.  Hence each clock has its own
> counter. Shouldn't the counter be shared?
> Am I missing something?

Oops. You are correct. I need to add this counter to priv instead of mstp_clocks.

> 
> And what about rzg2l_mod_clock_is_enabled()?
> Shouldn't it reflect the soft state instead of the shared hardware state?

OK, will return Soft state for coupled clocks.

Cheers,
Biju

Re: [PATCH v2 3/4] drivers: clk: renesas: rzg2l-cpg: Add support to handle coupled clocks

[Geert Uytterhoeven]

Hi Biju,

On Thu, Aug 12, 2021 at 9:00 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Subject: Re: [PATCH v2 3/4] drivers: clk: renesas: rzg2l-cpg: Add support
> > to handle coupled clocks
> > On Tue, Jul 27, 2021 at 4:18 PM Biju Das <biju.das.jz@bp.renesas.com>
> > wrote:
> > > The AXI and CHI clocks use the same register bit for controlling clock
> > > output. Add a new clock type for coupled clocks, which sets the
> > > CPG_CLKON_ETH.CLK[01]_ON bit when at least one clock is enabled, and
> > > clears the bit only when both clocks are disabled.
> > >
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Thanks for your patch!
> >
> > > --- a/drivers/clk/renesas/rzg2l-cpg.c
> > > +++ b/drivers/clk/renesas/rzg2l-cpg.c
> > > @@ -333,12 +333,16 @@ rzg2l_cpg_register_core_clk(const struct
> > cpg_core_clk *core,
> > >   * @hw: handle between common and hardware-specific interfaces
> > >   * @off: register offset
> > >   * @bit: ON/MON bit
> > > + * @is_coupled: flag to indicate coupled clock
> > > + * @on_cnt: ON count for coupled clocks
> > >   * @priv: CPG/MSTP private data
> > >   */
> > >  struct mstp_clock {
> > >         struct clk_hw hw;
> > >         u16 off;
> > >         u8 bit;
> > > +       bool is_coupled;
> > > +       u8 on_cnt;
> >
> > While u8 is probably sufficient, you may want to use unsigned int, as
> > there will be a gap anyway due to alignment rules.
> >
> > >         struct rzg2l_cpg_priv *priv;
> > >  };
> > >
> > > @@ -392,11 +396,37 @@ static int rzg2l_mod_clock_endisable(struct
> > > clk_hw *hw, bool enable)
> > >
> > >  static int rzg2l_mod_clock_enable(struct clk_hw *hw)  {
> > > +       struct mstp_clock *clock = to_mod_clock(hw);
> > > +       struct rzg2l_cpg_priv *priv = clock->priv;
> > > +       unsigned long flags;
> > > +
> > > +       spin_lock_irqsave(&priv->rmw_lock, flags);
> > > +       clock->on_cnt++;
> > > +       if (clock->is_coupled && clock->on_cnt > 1) {
> > > +               spin_unlock_irqrestore(&priv->rmw_lock, flags);
> > > +               return 1;
> > > +       }
> > > +
> > > +       spin_unlock_irqrestore(&priv->rmw_lock, flags);
> >
> > I think you can avoid taking the spinlock and touching the counter if the
> > is_coupled flag is not set.
>
> OK.
>
> >
> > > +
> > >         return rzg2l_mod_clock_endisable(hw, true);  }
> >
> > However, I'm wondering how this can work?
> >
> >       DEF_COUPLED("eth0_axi", R9A07G044_ETH0_CLK_AXI, R9A07G044_CLK_M0,
> >                               0x57c, 0),
> >       DEF_COUPLED("eth0_chi", R9A07G044_ETH0_CLK_CHI, R9A07G044_CLK_ZT,
> >                               0x57c, 0),
> >
> > This will create 2 independent clocks, each with their own mstp_clock
> > structure that has the is_coupled flag set.  Hence each clock has its own
> > counter. Shouldn't the counter be shared?
> > Am I missing something?
>
> Oops. You are correct. I need to add this counter to priv instead of mstp_clocks.

On second thought, a counter is overkill. A simple flag should be
sufficient, as the clk core only calls .{en,dis}able() when the clock is
{dis,en}enabled.

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 3/4] drivers: clk: renesas: rzg2l-cpg: Add support to handle coupled clocks

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH v2 3/4] drivers: clk: renesas: rzg2l-cpg: Add support
> to handle coupled clocks
> 
> Hi Biju,
> 
> On Thu, Aug 12, 2021 at 9:00 AM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > > Subject: Re: [PATCH v2 3/4] drivers: clk: renesas: rzg2l-cpg: Add
> > > support to handle coupled clocks On Tue, Jul 27, 2021 at 4:18 PM
> > > Biju Das <biju.das.jz@bp.renesas.com>
> > > wrote:
> > > > The AXI and CHI clocks use the same register bit for controlling
> > > > clock output. Add a new clock type for coupled clocks, which sets
> > > > the CPG_CLKON_ETH.CLK[01]_ON bit when at least one clock is
> > > > enabled, and clears the bit only when both clocks are disabled.
> > > >
> > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > Reviewed-by: Lad Prabhakar
> > > > <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > >
> > > Thanks for your patch!
> > >
> > > > --- a/drivers/clk/renesas/rzg2l-cpg.c
> > > > +++ b/drivers/clk/renesas/rzg2l-cpg.c
> > > > @@ -333,12 +333,16 @@ rzg2l_cpg_register_core_clk(const struct
> > > cpg_core_clk *core,
> > > >   * @hw: handle between common and hardware-specific interfaces
> > > >   * @off: register offset
> > > >   * @bit: ON/MON bit
> > > > + * @is_coupled: flag to indicate coupled clock
> > > > + * @on_cnt: ON count for coupled clocks
> > > >   * @priv: CPG/MSTP private data
> > > >   */
> > > >  struct mstp_clock {
> > > >         struct clk_hw hw;
> > > >         u16 off;
> > > >         u8 bit;
> > > > +       bool is_coupled;
> > > > +       u8 on_cnt;
> > >
> > > While u8 is probably sufficient, you may want to use unsigned int,
> > > as there will be a gap anyway due to alignment rules.
> > >
> > > >         struct rzg2l_cpg_priv *priv;  };
> > > >
> > > > @@ -392,11 +396,37 @@ static int rzg2l_mod_clock_endisable(struct
> > > > clk_hw *hw, bool enable)
> > > >
> > > >  static int rzg2l_mod_clock_enable(struct clk_hw *hw)  {
> > > > +       struct mstp_clock *clock = to_mod_clock(hw);
> > > > +       struct rzg2l_cpg_priv *priv = clock->priv;
> > > > +       unsigned long flags;
> > > > +
> > > > +       spin_lock_irqsave(&priv->rmw_lock, flags);
> > > > +       clock->on_cnt++;
> > > > +       if (clock->is_coupled && clock->on_cnt > 1) {
> > > > +               spin_unlock_irqrestore(&priv->rmw_lock, flags);
> > > > +               return 1;
> > > > +       }
> > > > +
> > > > +       spin_unlock_irqrestore(&priv->rmw_lock, flags);
> > >
> > > I think you can avoid taking the spinlock and touching the counter
> > > if the is_coupled flag is not set.
> >
> > OK.
> >
> > >
> > > > +
> > > >         return rzg2l_mod_clock_endisable(hw, true);  }
> > >
> > > However, I'm wondering how this can work?
> > >
> > >       DEF_COUPLED("eth0_axi", R9A07G044_ETH0_CLK_AXI,
> R9A07G044_CLK_M0,
> > >                               0x57c, 0),
> > >       DEF_COUPLED("eth0_chi", R9A07G044_ETH0_CLK_CHI,
> R9A07G044_CLK_ZT,
> > >                               0x57c, 0),
> > >
> > > This will create 2 independent clocks, each with their own
> > > mstp_clock structure that has the is_coupled flag set.  Hence each
> > > clock has its own counter. Shouldn't the counter be shared?
> > > Am I missing something?
> >
> > Oops. You are correct. I need to add this counter to priv instead of
> mstp_clocks.
> 
> On second thought, a counter is overkill. A simple flag should be
> sufficient, as the clk core only calls .{en,dis}able() when the clock is
> {dis,en}enabled.

Just to clarify, simple flag, did you mean to use bit flag? (ie, 2 bits , since we have 2 module clocks)
when core clock calls enable, set a bit and reset the bit during disable.

Then based on the 2bits, either turn on/off clock or just return the status.

Please correct me, if my understanding wrong?

Cheers,
Biju 

Re: [PATCH v2 3/4] drivers: clk: renesas: rzg2l-cpg: Add support to handle coupled clocks

[Geert Uytterhoeven]

Hi Biju,

On Fri, Aug 13, 2021 at 2:17 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Subject: Re: [PATCH v2 3/4] drivers: clk: renesas: rzg2l-cpg: Add support
> > to handle coupled clocks
> > On Thu, Aug 12, 2021 at 9:00 AM Biju Das <biju.das.jz@bp.renesas.com>
> > wrote:
> > > > Subject: Re: [PATCH v2 3/4] drivers: clk: renesas: rzg2l-cpg: Add
> > > > support to handle coupled clocks On Tue, Jul 27, 2021 at 4:18 PM
> > > > Biju Das <biju.das.jz@bp.renesas.com>
> > > > wrote:
> > > > > The AXI and CHI clocks use the same register bit for controlling
> > > > > clock output. Add a new clock type for coupled clocks, which sets
> > > > > the CPG_CLKON_ETH.CLK[01]_ON bit when at least one clock is
> > > > > enabled, and clears the bit only when both clocks are disabled.
> > > > >
> > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > Reviewed-by: Lad Prabhakar
> > > > > <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > >
> > > > Thanks for your patch!
> > > >
> > > > > --- a/drivers/clk/renesas/rzg2l-cpg.c
> > > > > +++ b/drivers/clk/renesas/rzg2l-cpg.c
> > > > > @@ -333,12 +333,16 @@ rzg2l_cpg_register_core_clk(const struct
> > > > cpg_core_clk *core,
> > > > >   * @hw: handle between common and hardware-specific interfaces
> > > > >   * @off: register offset
> > > > >   * @bit: ON/MON bit
> > > > > + * @is_coupled: flag to indicate coupled clock
> > > > > + * @on_cnt: ON count for coupled clocks
> > > > >   * @priv: CPG/MSTP private data
> > > > >   */
> > > > >  struct mstp_clock {
> > > > >         struct clk_hw hw;
> > > > >         u16 off;
> > > > >         u8 bit;
> > > > > +       bool is_coupled;
> > > > > +       u8 on_cnt;
> > > >
> > > > While u8 is probably sufficient, you may want to use unsigned int,
> > > > as there will be a gap anyway due to alignment rules.
> > > >
> > > > >         struct rzg2l_cpg_priv *priv;  };
> > > > >
> > > > > @@ -392,11 +396,37 @@ static int rzg2l_mod_clock_endisable(struct
> > > > > clk_hw *hw, bool enable)
> > > > >
> > > > >  static int rzg2l_mod_clock_enable(struct clk_hw *hw)  {
> > > > > +       struct mstp_clock *clock = to_mod_clock(hw);
> > > > > +       struct rzg2l_cpg_priv *priv = clock->priv;
> > > > > +       unsigned long flags;
> > > > > +
> > > > > +       spin_lock_irqsave(&priv->rmw_lock, flags);
> > > > > +       clock->on_cnt++;
> > > > > +       if (clock->is_coupled && clock->on_cnt > 1) {
> > > > > +               spin_unlock_irqrestore(&priv->rmw_lock, flags);
> > > > > +               return 1;
> > > > > +       }
> > > > > +
> > > > > +       spin_unlock_irqrestore(&priv->rmw_lock, flags);
> > > >
> > > > I think you can avoid taking the spinlock and touching the counter
> > > > if the is_coupled flag is not set.
> > >
> > > OK.
> > >
> > > >
> > > > > +
> > > > >         return rzg2l_mod_clock_endisable(hw, true);  }
> > > >
> > > > However, I'm wondering how this can work?
> > > >
> > > >       DEF_COUPLED("eth0_axi", R9A07G044_ETH0_CLK_AXI,
> > R9A07G044_CLK_M0,
> > > >                               0x57c, 0),
> > > >       DEF_COUPLED("eth0_chi", R9A07G044_ETH0_CLK_CHI,
> > R9A07G044_CLK_ZT,
> > > >                               0x57c, 0),
> > > >
> > > > This will create 2 independent clocks, each with their own
> > > > mstp_clock structure that has the is_coupled flag set.  Hence each
> > > > clock has its own counter. Shouldn't the counter be shared?
> > > > Am I missing something?
> > >
> > > Oops. You are correct. I need to add this counter to priv instead of
> > mstp_clocks.
> >
> > On second thought, a counter is overkill. A simple flag should be
> > sufficient, as the clk core only calls .{en,dis}able() when the clock is
> > {dis,en}enabled.
>
> Just to clarify, simple flag, did you mean to use bit flag? (ie, 2 bits , since we have 2 module clocks)
> when core clock calls enable, set a bit and reset the bit during disable.
>
> Then based on the 2bits, either turn on/off clock or just return the status.
>
> Please correct me, if my understanding wrong?

Just one bool or bit in a bitfield, the second flag will be in the
other struct mstp_clock (can there be three coupled clocks?).

So I think something like below should work:

     struct mstp_clock {
            struct clk_hw hw;
            u16 off;
            u8 bit;
    +       bool enabled;
            struct rzg2l_cpg_priv *priv;
    +       struct mstp_clock *siblings;
     };

.enabled needs to track the soft state of the clock.
The actual coupling is handled through .siblings, which points to the
other coupled clock (or forms a circular list if you can have more than
two coupled clocks).  When registering a clock, if mod->is_coupled
is set, you walk all already registered module clocks to find one
with the same off and bit, and link them together.

In .{en,dis}able(), you only {en,dis}able the hardware clock if all
other clocks in the list are disabled.

if it turns out too costly to add a pointer to each clock (depends
on slab granularity), you can also use a different struct for coupled
clocks:

    struct mstp_coupled_clock {
            struct mstp_clock mstp;
            struct mstp_coupled_clock *siblings;
    };

but then you do need another flag in mstp_clock to indicate it is
a coupled clock, that can be converted to mstp_coupled_clock using
container_of().

Does that make sense?
Have a nice weekend!

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 3/4] drivers: clk: renesas: rzg2l-cpg: Add support to handle coupled clocks

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH v2 3/4] drivers: clk: renesas: rzg2l-cpg: Add support
> to handle coupled clocks
> 
> Hi Biju,
> 
> On Fri, Aug 13, 2021 at 2:17 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > > Subject: Re: [PATCH v2 3/4] drivers: clk: renesas: rzg2l-cpg: Add
> > > support to handle coupled clocks On Thu, Aug 12, 2021 at 9:00 AM
> > > Biju Das <biju.das.jz@bp.renesas.com>
> > > wrote:
> > > > > Subject: Re: [PATCH v2 3/4] drivers: clk: renesas: rzg2l-cpg:
> > > > > Add support to handle coupled clocks On Tue, Jul 27, 2021 at
> > > > > 4:18 PM Biju Das <biju.das.jz@bp.renesas.com>
> > > > > wrote:
> > > > > > The AXI and CHI clocks use the same register bit for
> > > > > > controlling clock output. Add a new clock type for coupled
> > > > > > clocks, which sets the CPG_CLKON_ETH.CLK[01]_ON bit when at
> > > > > > least one clock is enabled, and clears the bit only when both
> clocks are disabled.
> > > > > >
> > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > > Reviewed-by: Lad Prabhakar
> > > > > > <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > >
> > > > > Thanks for your patch!
> > > > >
> > > > > > --- a/drivers/clk/renesas/rzg2l-cpg.c
> > > > > > +++ b/drivers/clk/renesas/rzg2l-cpg.c
> > > > > > @@ -333,12 +333,16 @@ rzg2l_cpg_register_core_clk(const struct
> > > > > cpg_core_clk *core,
> > > > > >   * @hw: handle between common and hardware-specific interfaces
> > > > > >   * @off: register offset
> > > > > >   * @bit: ON/MON bit
> > > > > > + * @is_coupled: flag to indicate coupled clock
> > > > > > + * @on_cnt: ON count for coupled clocks
> > > > > >   * @priv: CPG/MSTP private data
> > > > > >   */
> > > > > >  struct mstp_clock {
> > > > > >         struct clk_hw hw;
> > > > > >         u16 off;
> > > > > >         u8 bit;
> > > > > > +       bool is_coupled;
> > > > > > +       u8 on_cnt;
> > > > >
> > > > > While u8 is probably sufficient, you may want to use unsigned
> > > > > int, as there will be a gap anyway due to alignment rules.
> > > > >
> > > > > >         struct rzg2l_cpg_priv *priv;  };
> > > > > >
> > > > > > @@ -392,11 +396,37 @@ static int
> > > > > > rzg2l_mod_clock_endisable(struct clk_hw *hw, bool enable)
> > > > > >
> > > > > >  static int rzg2l_mod_clock_enable(struct clk_hw *hw)  {
> > > > > > +       struct mstp_clock *clock = to_mod_clock(hw);
> > > > > > +       struct rzg2l_cpg_priv *priv = clock->priv;
> > > > > > +       unsigned long flags;
> > > > > > +
> > > > > > +       spin_lock_irqsave(&priv->rmw_lock, flags);
> > > > > > +       clock->on_cnt++;
> > > > > > +       if (clock->is_coupled && clock->on_cnt > 1) {
> > > > > > +               spin_unlock_irqrestore(&priv->rmw_lock, flags);
> > > > > > +               return 1;
> > > > > > +       }
> > > > > > +
> > > > > > +       spin_unlock_irqrestore(&priv->rmw_lock, flags);
> > > > >
> > > > > I think you can avoid taking the spinlock and touching the
> > > > > counter if the is_coupled flag is not set.
> > > >
> > > > OK.
> > > >
> > > > >
> > > > > > +
> > > > > >         return rzg2l_mod_clock_endisable(hw, true);  }
> > > > >
> > > > > However, I'm wondering how this can work?
> > > > >
> > > > >       DEF_COUPLED("eth0_axi", R9A07G044_ETH0_CLK_AXI,
> > > R9A07G044_CLK_M0,
> > > > >                               0x57c, 0),
> > > > >       DEF_COUPLED("eth0_chi", R9A07G044_ETH0_CLK_CHI,
> > > R9A07G044_CLK_ZT,
> > > > >                               0x57c, 0),
> > > > >
> > > > > This will create 2 independent clocks, each with their own
> > > > > mstp_clock structure that has the is_coupled flag set.  Hence
> > > > > each clock has its own counter. Shouldn't the counter be shared?
> > > > > Am I missing something?
> > > >
> > > > Oops. You are correct. I need to add this counter to priv instead
> > > > of
> > > mstp_clocks.
> > >
> > > On second thought, a counter is overkill. A simple flag should be
> > > sufficient, as the clk core only calls .{en,dis}able() when the
> > > clock is {dis,en}enabled.
> >
> > Just to clarify, simple flag, did you mean to use bit flag? (ie, 2
> > bits , since we have 2 module clocks) when core clock calls enable, set
> a bit and reset the bit during disable.
> >
> > Then based on the 2bits, either turn on/off clock or just return the
> status.
> >
> > Please correct me, if my understanding wrong?
> 
> Just one bool or bit in a bitfield, the second flag will be in the other
> struct mstp_clock (can there be three coupled clocks?).

RZ/G2L have maximum 2 coupled clocks.

> 
> So I think something like below should work:

> 
>      struct mstp_clock {
>             struct clk_hw hw;
>             u16 off;
>             u8 bit;
>     +       bool enabled;
>             struct rzg2l_cpg_priv *priv;
>     +       struct mstp_clock *siblings;
>      };
> 
> .enabled needs to track the soft state of the clock.
> The actual coupling is handled through .siblings, which points to the
> other coupled clock (or forms a circular list if you can have more than
> two coupled clocks).  When registering a clock, if mod->is_coupled is set,
> you walk all already registered module clocks to find one with the same
> off and bit, and link them together.

Thanks, Will prototype based on the above solution.

> 
> In .{en,dis}able(), you only {en,dis}able the hardware clock if all other
> clocks in the list are disabled.
> 
> if it turns out too costly to add a pointer to each clock (depends on slab
> granularity), you can also use a different struct for coupled
> clocks:
> 
>     struct mstp_coupled_clock {
>             struct mstp_clock mstp;
>             struct mstp_coupled_clock *siblings;
>     };
> 
> but then you do need another flag in mstp_clock to indicate it is a
> coupled clock, that can be converted to mstp_coupled_clock using
> container_of().
> 
> Does that make sense?

Yes. Thanks for pointers.

Regards,
Biju