[PATCH v3 0/4] Add GbEthernet Clock support

[PATCH v3 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.
v2->v3:
 * Rebased to latest renesas-clk
 * Updated commit header for all patches
 * Replaced CLK_PLL5_2 with PLL5_FOUT3
 * Removed CLK_PLL6_2 and pll6_2 as the clk is sourced from PLL6
 * Added enabled flag to track the status of clock, if it is coupled
   with another clock
 * Introduced siblings pointer which points to the other coupled
   clock
 * coupled clock linking is done during module clk register.
 * rzg2l_mod_clock_is_enabled function returns soft state of the
   module clocks, if it is coupled with another clock
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):
  clk: renesas: rzg2l: Add support to handle MUX clocks
  clk: renesas: r9a07g044: Add ethernet clock sources
  clk: renesas: rzg2l: Add support to handle coupled clocks
  clk: renesas: r9a07g044: Add GbEthernet clock/reset

 drivers/clk/renesas/r9a07g044-cpg.c | 29 +++++++++-
 drivers/clk/renesas/rzg2l-cpg.c     | 85 +++++++++++++++++++++++++++++
 drivers/clk/renesas/rzg2l-cpg.h     | 26 ++++++++-
 3 files changed, 138 insertions(+), 2 deletions(-)

-- 
2.17.1

[PATCH v3 1/4] clk: renesas: rzg2l: 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>
---
v2->v3:
 * Updated commit header drivers: clk: renesas: rzg2l-cpg with
   clk: renesas: rzg2l
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 v3 2/4] clk: renesas: r9a07g044: Add ethernet clock sources

[Biju Das]

Ethernet reference clock can be sourced from PLL5_FOUT3 or PLL6. 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>
---
v2->v3:
 * Replaced CLK_PLL5_2 with PLL5_FOUT3
 * Removed CLK_PLL6_2 and pll6_2 as the clk is sourced from PLL6
 * Updated commit header
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 | 19 ++++++++++++++++++-
 drivers/clk/renesas/rzg2l-cpg.h     |  3 +++
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/renesas/r9a07g044-cpg.c b/drivers/clk/renesas/r9a07g044-cpg.c
index 4c94b94c4125..90bbeb03308c 100644
--- a/drivers/clk/renesas/r9a07g044-cpg.c
+++ b/drivers/clk/renesas/r9a07g044-cpg.c
@@ -35,8 +35,10 @@ enum clk_ids {
 	CLK_PLL3_DIV4,
 	CLK_PLL4,
 	CLK_PLL5,
-	CLK_PLL5_DIV2,
+	CLK_PLL5_FOUT3,
+	CLK_PLL5_2_DIV12,
 	CLK_PLL6,
+	CLK_PLL6_2_DIV2,
 	CLK_P1_DIV2,
 
 	/* Module Clocks */
@@ -53,6 +55,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 +69,11 @@ 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_fout3", CLK_PLL5_FOUT3, CLK_PLL5, 1, 6),
+
+	DEF_FIXED(".pll6", CLK_PLL6, CLK_EXTAL, 125, 6),
+
 	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 +83,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_FOUT3, 1, 2),
+	DEF_FIXED(".pll6_2_div2", CLK_PLL6_2_DIV2, CLK_PLL6, 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 +97,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

Re: [PATCH v3 2/4] clk: renesas: r9a07g044: Add ethernet clock sources

[Geert Uytterhoeven]

On Sun, Aug 15, 2021 at 12:30 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> Ethernet reference clock can be sourced from PLL5_FOUT3 or PLL6. 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>

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 v3 3/4] clk: renesas: rzg2l: 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->v3:
 * Reworked as per Geert's suggestion
 * Added enabled flag to track the status of clock, if it is coupled
   with another clock
 * Introduced siblings pointer which points to the other coupled
   clock
 * coupled clock linking is done during module clk register.
 * rzg2l_mod_clock_is_enabled function returns soft state of the
   module clocks, if it is coupled with another clock
 * Updated the commit header
v2:-
 * New patch
---
 drivers/clk/renesas/rzg2l-cpg.c | 62 +++++++++++++++++++++++++++++++++
 drivers/clk/renesas/rzg2l-cpg.h | 11 +++++-
 2 files changed, 72 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/renesas/rzg2l-cpg.c b/drivers/clk/renesas/rzg2l-cpg.c
index 597efc2504eb..ebcb57287bf6 100644
--- a/drivers/clk/renesas/rzg2l-cpg.c
+++ b/drivers/clk/renesas/rzg2l-cpg.c
@@ -333,13 +333,17 @@ 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
+ * @enabled: soft state of the clock, if it is coupled with another clock
  * @priv: CPG/MSTP private data
+ * @siblings: pointer to the other coupled clock
  */
 struct mstp_clock {
 	struct clk_hw hw;
 	u16 off;
 	u8 bit;
+	bool enabled;
 	struct rzg2l_cpg_priv *priv;
+	struct mstp_clock *siblings;
 };
 
 #define to_mod_clock(_hw) container_of(_hw, struct mstp_clock, hw)
@@ -392,11 +396,35 @@ 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 mstp_clock *siblings = clock->siblings;
+
+	if (siblings) {
+		if (siblings->enabled) {
+			clock->enabled = true;
+			return 0;
+		}
+
+		clock->enabled = true;
+	}
+
 	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 mstp_clock *siblings = clock->siblings;
+
+	if (siblings) {
+		if (siblings->enabled) {
+			clock->enabled = false;
+			return;
+		}
+
+		clock->enabled = false;
+	}
+
 	rzg2l_mod_clock_endisable(hw, false);
 }
 
@@ -412,6 +440,9 @@ static int rzg2l_mod_clock_is_enabled(struct clk_hw *hw)
 		return 1;
 	}
 
+	if (clock->siblings)
+		return clock->enabled;
+
 	value = readl(priv->base + CLK_MON_R(clock->off));
 
 	return !(value & bitmask);
@@ -423,11 +454,33 @@ static const struct clk_ops rzg2l_mod_clock_ops = {
 	.is_enabled = rzg2l_mod_clock_is_enabled,
 };
 
+static struct mstp_clock
+*rzg2l_cpg_get_sibling_clk(struct mstp_clock *clock,
+			   struct rzg2l_cpg_priv *priv)
+{
+	struct mstp_clock *sibl_clk = NULL;
+	struct clk_hw *hw;
+	unsigned int i;
+
+	for (i = 0; i < priv->num_mod_clks; i++) {
+		if (priv->clks[priv->num_core_clks + i] == ERR_PTR(-ENOENT))
+			continue;
+
+		hw = __clk_get_hw(priv->clks[priv->num_core_clks + i]);
+		sibl_clk = to_mod_clock(hw);
+		if (clock->off == sibl_clk->off && clock->bit == sibl_clk->bit)
+			break;
+	}
+
+	return sibl_clk;
+}
+
 static void __init
 rzg2l_cpg_register_mod_clk(const struct rzg2l_mod_clk *mod,
 			   const struct rzg2l_cpg_info *info,
 			   struct rzg2l_cpg_priv *priv)
 {
+	struct mstp_clock *sibling_clock = NULL;
 	struct mstp_clock *clock = NULL;
 	struct device *dev = priv->dev;
 	unsigned int id = mod->id;
@@ -484,6 +537,15 @@ 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;
+
+	if (mod->is_coupled) {
+		sibling_clock = rzg2l_cpg_get_sibling_clk(clock, priv);
+		if (sibling_clock) {
+			clock->siblings = sibling_clock;
+			sibling_clock->siblings = clock;
+		}
+	}
+
 	return;
 
 fail:
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

RE: [PATCH v3 3/4] clk: renesas: rzg2l: Add support to handle coupled clocks

[Biju Das]

Hi All,


> Subject: [PATCH v3 3/4] clk: renesas: rzg2l: Add support to handle coupled
> clocks
> 
> 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->v3:
>  * Reworked as per Geert's suggestion
>  * Added enabled flag to track the status of clock, if it is coupled
>    with another clock
>  * Introduced siblings pointer which points to the other coupled
>    clock
>  * coupled clock linking is done during module clk register.
>  * rzg2l_mod_clock_is_enabled function returns soft state of the
>    module clocks, if it is coupled with another clock
>  * Updated the commit header
> v2:-
>  * New patch
> ---
>  drivers/clk/renesas/rzg2l-cpg.c | 62 +++++++++++++++++++++++++++++++++
> drivers/clk/renesas/rzg2l-cpg.h | 11 +++++-
>  2 files changed, 72 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/renesas/rzg2l-cpg.c b/drivers/clk/renesas/rzg2l-
> cpg.c index 597efc2504eb..ebcb57287bf6 100644
> --- a/drivers/clk/renesas/rzg2l-cpg.c
> +++ b/drivers/clk/renesas/rzg2l-cpg.c
> @@ -333,13 +333,17 @@ 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
> + * @enabled: soft state of the clock, if it is coupled with another
> + clock
>   * @priv: CPG/MSTP private data
> + * @siblings: pointer to the other coupled clock
>   */
>  struct mstp_clock {
>  	struct clk_hw hw;
>  	u16 off;
>  	u8 bit;
> +	bool enabled;
>  	struct rzg2l_cpg_priv *priv;
> +	struct mstp_clock *siblings;
>  };
> 
>  #define to_mod_clock(_hw) container_of(_hw, struct mstp_clock, hw) @@ -
> 392,11 +396,35 @@ 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 mstp_clock *siblings = clock->siblings;
> +
> +	if (siblings) {
> +		if (siblings->enabled) {
> +			clock->enabled = true;
> +			return 0;
> +		}
> +
> +		clock->enabled = true;
> +	}
> +
>  	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 mstp_clock *siblings = clock->siblings;
> +
> +	if (siblings) {
> +		if (siblings->enabled) {
> +			clock->enabled = false;
> +			return;
> +		}
> +
> +		clock->enabled = false;
> +	}
> +
>  	rzg2l_mod_clock_endisable(hw, false);
>  }
> 
> @@ -412,6 +440,9 @@ static int rzg2l_mod_clock_is_enabled(struct clk_hw
> *hw)
>  		return 1;
>  	}
> 
> +	if (clock->siblings)
> +		return clock->enabled;
> +
>  	value = readl(priv->base + CLK_MON_R(clock->off));
> 
>  	return !(value & bitmask);
> @@ -423,11 +454,33 @@ static const struct clk_ops rzg2l_mod_clock_ops = {
>  	.is_enabled = rzg2l_mod_clock_is_enabled,  };
> 
> +static struct mstp_clock
> +*rzg2l_cpg_get_sibling_clk(struct mstp_clock *clock,
> +			   struct rzg2l_cpg_priv *priv)
> +{
> +	struct mstp_clock *sibl_clk = NULL;
> +	struct clk_hw *hw;
> +	unsigned int i;
> +
> +	for (i = 0; i < priv->num_mod_clks; i++) {
> +		if (priv->clks[priv->num_core_clks + i] == ERR_PTR(-ENOENT))
> +			continue;
> +
> +		hw = __clk_get_hw(priv->clks[priv->num_core_clks + i]);
> +		sibl_clk = to_mod_clock(hw);
> +		if (clock->off == sibl_clk->off && clock->bit == sibl_clk-
> >bit)
> +			break;

Just found during testing, instead of "break" we should return sibl_clk; 
Otherwise for the first clock, it gets a wrong siblings,
Which will be overridden with correct siblings during
registration of other coupled clock.

Currently it gets into the loop 4 *97 = 388 times during registration and the extra memory is 97*sizeof(mstp*) bytes.
This solution simpler and neater. 

But not sure we should optimize this by adding all the coupled clocks
into priv structure (4 * sizeof(mstp*) bytes + 4* enabled flags + 4* link pointer) ? and 
at run time enable/disable will go through this list, find the clock and coupled clk and based
on coupled clk's enable status it will set clk's enabled status and set hardware clock

In that case rzg2l_mod_clock_is_enabled will also need to find the clock from that list and
return soft enabled status from priv structure.

But this solution has runtime overhead of finding clk and coupled clk from the priv structure


Cheers,
Biju

> +	}
> +
> +	return sibl_clk;
> +}
> +
>  static void __init
>  rzg2l_cpg_register_mod_clk(const struct rzg2l_mod_clk *mod,
>  			   const struct rzg2l_cpg_info *info,
>  			   struct rzg2l_cpg_priv *priv)
>  {
> +	struct mstp_clock *sibling_clock = NULL;
>  	struct mstp_clock *clock = NULL;
>  	struct device *dev = priv->dev;
>  	unsigned int id = mod->id;
> @@ -484,6 +537,15 @@ 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;
> +
> +	if (mod->is_coupled) {
> +		sibling_clock = rzg2l_cpg_get_sibling_clk(clock, priv);
> +		if (sibling_clock) {
> +			clock->siblings = sibling_clock;
> +			sibling_clock->siblings = clock;
> +		}
> +	}
> +
>  	return;
> 
>  fail:
> 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

Re: [PATCH v3 3/4] clk: renesas: rzg2l: Add support to handle coupled clocks

[Geert Uytterhoeven]

Hi Biju,

On Mon, Aug 16, 2021 at 11:23 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Subject: [PATCH v3 3/4] clk: renesas: rzg2l: Add support to handle coupled
> > clocks
> >
> > 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>

> > --- a/drivers/clk/renesas/rzg2l-cpg.c
> > +++ b/drivers/clk/renesas/rzg2l-cpg.c
> > @@ -423,11 +454,33 @@ static const struct clk_ops rzg2l_mod_clock_ops = {
> >       .is_enabled = rzg2l_mod_clock_is_enabled,  };
> >
> > +static struct mstp_clock
> > +*rzg2l_cpg_get_sibling_clk(struct mstp_clock *clock,
> > +                        struct rzg2l_cpg_priv *priv)
> > +{
> > +     struct mstp_clock *sibl_clk = NULL;
> > +     struct clk_hw *hw;
> > +     unsigned int i;
> > +
> > +     for (i = 0; i < priv->num_mod_clks; i++) {
> > +             if (priv->clks[priv->num_core_clks + i] == ERR_PTR(-ENOENT))
> > +                     continue;
> > +
> > +             hw = __clk_get_hw(priv->clks[priv->num_core_clks + i]);
> > +             sibl_clk = to_mod_clock(hw);
> > +             if (clock->off == sibl_clk->off && clock->bit == sibl_clk-
> > >bit)
> > +                     break;
>
> Just found during testing, instead of "break" we should return sibl_clk;
> Otherwise for the first clock, it gets a wrong siblings,
> Which will be overridden with correct siblings during
> registration of other coupled clock.

Indeed.

> Currently it gets into the loop 4 *97 = 388 times during registration and the extra memory is 97*sizeof(mstp*) bytes.
> This solution simpler and neater.
>
> But not sure we should optimize this by adding all the coupled clocks
> into priv structure (4 * sizeof(mstp*) bytes + 4* enabled flags + 4* link pointer) ? and
> at run time enable/disable will go through this list, find the clock and coupled clk and based
> on coupled clk's enable status it will set clk's enabled status and set hardware clock
>
> In that case rzg2l_mod_clock_is_enabled will also need to find the clock from that list and
> return soft enabled status from priv structure.
>
> But this solution has runtime overhead of finding clk and coupled clk from the priv structure

Yeah, that should be slower, due to the look-up overhead.

As an alternative to the sibling pointer, you could store a pointer
to a shared atomic_t counter, and use atomic_{inc,dec}_return().
That requires extra storage (one atomic_t per coupled clock), and
avoids taking the spinlock.  But you have to take the spinlock later
anyway, for changing the clock bits, and you still need to store the
per-clock "soft" enable flag.

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

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH v3 3/4] clk: renesas: rzg2l: Add support to handle
> coupled clocks
> 
> Hi Biju,
> 
> On Mon, Aug 16, 2021 at 11:23 AM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > > Subject: [PATCH v3 3/4] clk: renesas: rzg2l: Add support to handle
> > > coupled clocks
> > >
> > > 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>
> 
> > > --- a/drivers/clk/renesas/rzg2l-cpg.c
> > > +++ b/drivers/clk/renesas/rzg2l-cpg.c
> > > @@ -423,11 +454,33 @@ static const struct clk_ops rzg2l_mod_clock_ops
> = {
> > >       .is_enabled = rzg2l_mod_clock_is_enabled,  };
> > >
> > > +static struct mstp_clock
> > > +*rzg2l_cpg_get_sibling_clk(struct mstp_clock *clock,
> > > +                        struct rzg2l_cpg_priv *priv) {
> > > +     struct mstp_clock *sibl_clk = NULL;
> > > +     struct clk_hw *hw;
> > > +     unsigned int i;
> > > +
> > > +     for (i = 0; i < priv->num_mod_clks; i++) {
> > > +             if (priv->clks[priv->num_core_clks + i] == ERR_PTR(-
> ENOENT))
> > > +                     continue;
> > > +
> > > +             hw = __clk_get_hw(priv->clks[priv->num_core_clks + i]);
> > > +             sibl_clk = to_mod_clock(hw);
> > > +             if (clock->off == sibl_clk->off && clock->bit ==
> > > + sibl_clk-
> > > >bit)
> > > +                     break;
> >
> > Just found during testing, instead of "break" we should return
> > sibl_clk; Otherwise for the first clock, it gets a wrong siblings,
> > Which will be overridden with correct siblings during registration of
> > other coupled clock.
> 
> Indeed.
> 
> > Currently it gets into the loop 4 *97 = 388 times during registration
> and the extra memory is 97*sizeof(mstp*) bytes.
> > This solution simpler and neater.
> >
> > But not sure we should optimize this by adding all the coupled clocks
> > into priv structure (4 * sizeof(mstp*) bytes + 4* enabled flags + 4*
> > link pointer) ? and at run time enable/disable will go through this
> > list, find the clock and coupled clk and based on coupled clk's enable
> > status it will set clk's enabled status and set hardware clock
> >
> > In that case rzg2l_mod_clock_is_enabled will also need to find the
> > clock from that list and return soft enabled status from priv structure.
> >
> > But this solution has runtime overhead of finding clk and coupled clk
> > from the priv structure
> 
> Yeah, that should be slower, due to the look-up overhead.

OK.

> 
> As an alternative to the sibling pointer, you could store a pointer to a
> shared atomic_t counter, and use atomic_{inc,dec}_return().
> That requires extra storage (one atomic_t per coupled clock), and avoids
> taking the spinlock.  But you have to take the spinlock later anyway, for
> changing the clock bits, and you still need to store the per-clock "soft"
> enable flag.

Agreed. Looks the current solution is better. So I will stick with current one
And will post V4 based on previous comments.

Regards,
Biju

Re: [PATCH v3 3/4] clk: renesas: rzg2l: Add support to handle coupled clocks

[Geert Uytterhoeven]

Hi Biju,

On Sun, Aug 15, 2021 at 12:30 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>
> ---
> v2->v3:
>  * Reworked as per Geert's suggestion
>  * Added enabled flag to track the status of clock, if it is coupled
>    with another clock
>  * Introduced siblings pointer which points to the other coupled
>    clock
>  * coupled clock linking is done during module clk register.
>  * rzg2l_mod_clock_is_enabled function returns soft state of the
>    module clocks, if it is coupled with another clock
>  * Updated the commit header

Thanks for the update!

> --- a/drivers/clk/renesas/rzg2l-cpg.c
> +++ b/drivers/clk/renesas/rzg2l-cpg.c
> @@ -392,11 +396,35 @@ 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 mstp_clock *siblings = clock->siblings;
> +
> +       if (siblings) {
> +               if (siblings->enabled) {
> +                       clock->enabled = true;
> +                       return 0;
> +               }
> +
> +               clock->enabled = true;

clock->enabled is set to true regardless of the if condition.
This also needs locking, in case both clocks are changed concurrently:

    spin_lock_irqsave(&priv->rmw_lock, flags);
    enabled = siblings->enabled;
    clock->enabled = true;
    spin_unlock_irqrestore(&priv->rmw_lock, flags);
    if (enabled)
            return 0;

> +       }
> +
>         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 mstp_clock *siblings = clock->siblings;
> +
> +       if (siblings) {
> +               if (siblings->enabled) {
> +                       clock->enabled = false;
> +                       return;
> +               }
> +
> +               clock->enabled = false;

Likewise.

> +       }
> +
>         rzg2l_mod_clock_endisable(hw, false);
>  }
>
> @@ -412,6 +440,9 @@ static int rzg2l_mod_clock_is_enabled(struct clk_hw *hw)
>                 return 1;
>         }
>
> +       if (clock->siblings)
> +               return clock->enabled;
> +
>         value = readl(priv->base + CLK_MON_R(clock->off));
>
>         return !(value & bitmask);
> @@ -423,11 +454,33 @@ static const struct clk_ops rzg2l_mod_clock_ops = {
>         .is_enabled = rzg2l_mod_clock_is_enabled,
>  };
>
> +static struct mstp_clock
> +*rzg2l_cpg_get_sibling_clk(struct mstp_clock *clock,
> +                          struct rzg2l_cpg_priv *priv)
> +{
> +       struct mstp_clock *sibl_clk = NULL;

clk?
It's only a sibling when the condition inside the loop is true.

> +       struct clk_hw *hw;
> +       unsigned int i;
> +
> +       for (i = 0; i < priv->num_mod_clks; i++) {
> +               if (priv->clks[priv->num_core_clks + i] == ERR_PTR(-ENOENT))
> +                       continue;
> +
> +               hw = __clk_get_hw(priv->clks[priv->num_core_clks + i]);
> +               sibl_clk = to_mod_clock(hw);
> +               if (clock->off == sibl_clk->off && clock->bit == sibl_clk->bit)
> +                       break;
> +       }
> +
> +       return sibl_clk;
> +}
> +
>  static void __init
>  rzg2l_cpg_register_mod_clk(const struct rzg2l_mod_clk *mod,
>                            const struct rzg2l_cpg_info *info,
>                            struct rzg2l_cpg_priv *priv)
>  {
> +       struct mstp_clock *sibling_clock = NULL;
>         struct mstp_clock *clock = NULL;
>         struct device *dev = priv->dev;
>         unsigned int id = mod->id;
> @@ -484,6 +537,15 @@ 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;
> +
> +       if (mod->is_coupled) {
> +               sibling_clock = rzg2l_cpg_get_sibling_clk(clock, priv);
> +               if (sibling_clock) {
> +                       clock->siblings = sibling_clock;
> +                       sibling_clock->siblings = clock;
> +               }

You forgot to initialize mstp_clock.enabled to match the current
hardware state.

> +       }
> +
>         return;
>
>  fail:

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

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH v3 3/4] clk: renesas: rzg2l: Add support to handle
> coupled clocks
> 
> Hi Biju,
> 
> On Sun, Aug 15, 2021 at 12:30 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>
> > ---
> > v2->v3:
> >  * Reworked as per Geert's suggestion
> >  * Added enabled flag to track the status of clock, if it is coupled
> >    with another clock
> >  * Introduced siblings pointer which points to the other coupled
> >    clock
> >  * coupled clock linking is done during module clk register.
> >  * rzg2l_mod_clock_is_enabled function returns soft state of the
> >    module clocks, if it is coupled with another clock
> >  * Updated the commit header
> 
> Thanks for the update!
> 
> > --- a/drivers/clk/renesas/rzg2l-cpg.c
> > +++ b/drivers/clk/renesas/rzg2l-cpg.c
> > @@ -392,11 +396,35 @@ 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 mstp_clock *siblings = clock->siblings;
> > +
> > +       if (siblings) {
> > +               if (siblings->enabled) {
> > +                       clock->enabled = true;
> > +                       return 0;
> > +               }
> > +
> > +               clock->enabled = true;
> 
> clock->enabled is set to true regardless of the if condition.
> This also needs locking, in case both clocks are changed concurrently:

Agreed. It needs locking.

> 
>     spin_lock_irqsave(&priv->rmw_lock, flags);
>     enabled = siblings->enabled;
>     clock->enabled = true;
>     spin_unlock_irqrestore(&priv->rmw_lock, flags);
>     if (enabled)
>             return 0;
> 
> > +       }
> > +
> >         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 mstp_clock *siblings = clock->siblings;
> > +
> > +       if (siblings) {
> > +               if (siblings->enabled) {
> > +                       clock->enabled = false;
> > +                       return;
> > +               }
> > +
> > +               clock->enabled = false;
> 
> Likewise.
OK.

> 
> > +       }
> > +
> >         rzg2l_mod_clock_endisable(hw, false);  }
> >
> > @@ -412,6 +440,9 @@ static int rzg2l_mod_clock_is_enabled(struct clk_hw
> *hw)
> >                 return 1;
> >         }
> >
> > +       if (clock->siblings)
> > +               return clock->enabled;
> > +
> >         value = readl(priv->base + CLK_MON_R(clock->off));
> >
> >         return !(value & bitmask);
> > @@ -423,11 +454,33 @@ static const struct clk_ops rzg2l_mod_clock_ops =
> {
> >         .is_enabled = rzg2l_mod_clock_is_enabled,  };
> >
> > +static struct mstp_clock
> > +*rzg2l_cpg_get_sibling_clk(struct mstp_clock *clock,
> > +                          struct rzg2l_cpg_priv *priv) {
> > +       struct mstp_clock *sibl_clk = NULL;
> 
> clk?
> It's only a sibling when the condition inside the loop is true.

OK, will declare "clk" inside for loop to limit the scope.

> 
> > +       struct clk_hw *hw;
> > +       unsigned int i;
> > +
> > +       for (i = 0; i < priv->num_mod_clks; i++) {
> > +               if (priv->clks[priv->num_core_clks + i] == ERR_PTR(-
> ENOENT))
> > +                       continue;
> > +
> > +               hw = __clk_get_hw(priv->clks[priv->num_core_clks + i]);
> > +               sibl_clk = to_mod_clock(hw);
> > +               if (clock->off == sibl_clk->off && clock->bit ==
> sibl_clk->bit)
> > +                       break;
> > +       }
> > +
> > +       return sibl_clk;
> > +}
> > +
> >  static void __init
> >  rzg2l_cpg_register_mod_clk(const struct rzg2l_mod_clk *mod,
> >                            const struct rzg2l_cpg_info *info,
> >                            struct rzg2l_cpg_priv *priv)  {
> > +       struct mstp_clock *sibling_clock = NULL;
> >         struct mstp_clock *clock = NULL;
> >         struct device *dev = priv->dev;
> >         unsigned int id = mod->id;
> > @@ -484,6 +537,15 @@ 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;
> > +
> > +       if (mod->is_coupled) {
> > +               sibling_clock = rzg2l_cpg_get_sibling_clk(clock, priv);
> > +               if (sibling_clock) {
> > +                       clock->siblings = sibling_clock;
> > +                       sibling_clock->siblings = clock;
> > +               }
> 
> You forgot to initialize mstp_clock.enabled to match the current hardware
> state.

OK. will initialize mstp_clock.enabled to match the current hardware state.

Cheers,
Biju

> 
> > +       }
> > +
> >         return;
> >
> >  fail:
> 
> 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 v3 3/4] clk: renesas: rzg2l: Add support to handle coupled clocks

[Biju Das]

Hi Geert,

> Subject: RE: [PATCH v3 3/4] clk: renesas: rzg2l: Add support to handle
> coupled clocks
> 
> Hi Geert,
> 
> Thanks for the feedback.
> 
> > Subject: Re: [PATCH v3 3/4] clk: renesas: rzg2l: Add support to handle
> > coupled clocks
> >
> > Hi Biju,
> >
> > On Sun, Aug 15, 2021 at 12:30 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>
> > > ---
> > > v2->v3:
> > >  * Reworked as per Geert's suggestion
> > >  * Added enabled flag to track the status of clock, if it is coupled
> > >    with another clock
> > >  * Introduced siblings pointer which points to the other coupled
> > >    clock
> > >  * coupled clock linking is done during module clk register.
> > >  * rzg2l_mod_clock_is_enabled function returns soft state of the
> > >    module clocks, if it is coupled with another clock
> > >  * Updated the commit header
> >
> > Thanks for the update!
> >
> > > --- a/drivers/clk/renesas/rzg2l-cpg.c
> > > +++ b/drivers/clk/renesas/rzg2l-cpg.c
> > > @@ -392,11 +396,35 @@ 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 mstp_clock *siblings = clock->siblings;
> > > +
> > > +       if (siblings) {
> > > +               if (siblings->enabled) {
> > > +                       clock->enabled = true;
> > > +                       return 0;
> > > +               }
> > > +
> > > +               clock->enabled = true;
> >
> > clock->enabled is set to true regardless of the if condition.
> > This also needs locking, in case both clocks are changed concurrently:
> 
> Agreed. It needs locking.
> 
> >
> >     spin_lock_irqsave(&priv->rmw_lock, flags);
> >     enabled = siblings->enabled;
> >     clock->enabled = true;
> >     spin_unlock_irqrestore(&priv->rmw_lock, flags);
> >     if (enabled)
> >             return 0;
> >
> > > +       }
> > > +
> > >         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 mstp_clock *siblings = clock->siblings;
> > > +
> > > +       if (siblings) {
> > > +               if (siblings->enabled) {
> > > +                       clock->enabled = false;
> > > +                       return;
> > > +               }
> > > +
> > > +               clock->enabled = false;
> >
> > Likewise.
> OK.
> 
> >
> > > +       }
> > > +
> > >         rzg2l_mod_clock_endisable(hw, false);  }
> > >
> > > @@ -412,6 +440,9 @@ static int rzg2l_mod_clock_is_enabled(struct
> > > clk_hw
> > *hw)
> > >                 return 1;
> > >         }
> > >
> > > +       if (clock->siblings)
> > > +               return clock->enabled;
> > > +
> > >         value = readl(priv->base + CLK_MON_R(clock->off));
> > >
> > >         return !(value & bitmask);
> > > @@ -423,11 +454,33 @@ static const struct clk_ops
> > > rzg2l_mod_clock_ops =
> > {
> > >         .is_enabled = rzg2l_mod_clock_is_enabled,  };
> > >
> > > +static struct mstp_clock
> > > +*rzg2l_cpg_get_sibling_clk(struct mstp_clock *clock,
> > > +                          struct rzg2l_cpg_priv *priv) {
> > > +       struct mstp_clock *sibl_clk = NULL;
> >
> > clk?
> > It's only a sibling when the condition inside the loop is true.
> 
> OK, will declare "clk" inside for loop to limit the scope.
> 
> >
> > > +       struct clk_hw *hw;
> > > +       unsigned int i;
> > > +
> > > +       for (i = 0; i < priv->num_mod_clks; i++) {
> > > +               if (priv->clks[priv->num_core_clks + i] == ERR_PTR(-
> > ENOENT))
> > > +                       continue;
> > > +
> > > +               hw = __clk_get_hw(priv->clks[priv->num_core_clks +
> i]);
> > > +               sibl_clk = to_mod_clock(hw);
> > > +               if (clock->off == sibl_clk->off && clock->bit ==
> > sibl_clk->bit)
> > > +                       break;
> > > +       }
> > > +
> > > +       return sibl_clk;
> > > +}
> > > +
> > >  static void __init
> > >  rzg2l_cpg_register_mod_clk(const struct rzg2l_mod_clk *mod,
> > >                            const struct rzg2l_cpg_info *info,
> > >                            struct rzg2l_cpg_priv *priv)  {
> > > +       struct mstp_clock *sibling_clock = NULL;
> > >         struct mstp_clock *clock = NULL;
> > >         struct device *dev = priv->dev;
> > >         unsigned int id = mod->id;
> > > @@ -484,6 +537,15 @@ 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;
> > > +
> > > +       if (mod->is_coupled) {
> > > +               sibling_clock = rzg2l_cpg_get_sibling_clk(clock,
> priv);
> > > +               if (sibling_clock) {
> > > +                       clock->siblings = sibling_clock;
> > > +                       sibling_clock->siblings = clock;
> > > +               }
> >
> > You forgot to initialize mstp_clock.enabled to match the current
> > hardware state.
> 
> OK. will initialize mstp_clock.enabled to match the current hardware
> state.

While working on this, I found a bug in clk driver with patch
ef3c613ccd68 ("clk: renesas: Add CPG core wrapper for RZ/G2L SoC")

As per H/W manual(0.50), clock monitor status "1" means clock is supplied and
"0" means clock supply is stopped.

But the "rzg2l_mod_clock_is_enabled" function returns inverted value instead.

Due to this wrong status, The unused_clk_function never switch off the unused clocks,
before spawning before init.

After fixing "rzg2l_mod_clock_is_enabled" function and found that board is not booting.
Reason is, unused_clk_function turns off IA_55 and dmac clocks.

On further investigation, turning off IA55_CLK[1] and DMAC_ACLK[2]
leading GIC interrupts failure. 

I made IA55_CLK and DMAC_ACLK as critical clocks as even if, we disable the corresponding driver,
GIC interrupts should work and with that I am able to mount rootFS.

So I guess fixing "rzg2l_mod_clock_is_enabled" and Adding critical clocks "IA55_CLK" and "DMAC_ACLK"
Should be a single patch??

Then I tested DMA it was failing, as driver is not turning ON DMA_PCLK. So added PM
Routines to handle DMA clocks and with that DMA driver is working. This will be a separate
Patch for dmac driver.

[1] c3e67ad6f5a2 ("dt-bindings: clock: r9a07g044-cpg: Update clock/reset definitions")
[2] eb829e549ba6 ("clk: renesas: r9a07g044: Add DMAC clocks/resets")

Please share your views on this investigation.

Regards
Biju

Re: [PATCH v3 3/4] clk: renesas: rzg2l: Add support to handle coupled clocks

[Geert Uytterhoeven]

Hi Biju,

On Mon, Aug 30, 2021 at 10:36 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Subject: RE: [PATCH v3 3/4] clk: renesas: rzg2l: Add support to handle
> > coupled clocks
> > > On Sun, Aug 15, 2021 at 12:30 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>
> > > > ---
> > > > v2->v3:
> > > >  * Reworked as per Geert's suggestion
> > > >  * Added enabled flag to track the status of clock, if it is coupled
> > > >    with another clock
> > > >  * Introduced siblings pointer which points to the other coupled
> > > >    clock
> > > >  * coupled clock linking is done during module clk register.
> > > >  * rzg2l_mod_clock_is_enabled function returns soft state of the
> > > >    module clocks, if it is coupled with another clock
> > > >  * Updated the commit header

> > > You forgot to initialize mstp_clock.enabled to match the current
> > > hardware state.
> >
> > OK. will initialize mstp_clock.enabled to match the current hardware
> > state.
>
> While working on this, I found a bug in clk driver with patch
> ef3c613ccd68 ("clk: renesas: Add CPG core wrapper for RZ/G2L SoC")
>
> As per H/W manual(0.50), clock monitor status "1" means clock is supplied and
> "0" means clock supply is stopped.
>
> But the "rzg2l_mod_clock_is_enabled" function returns inverted value instead.

Oops...

> Due to this wrong status, The unused_clk_function never switch off the unused clocks,
> before spawning before init.
>
> After fixing "rzg2l_mod_clock_is_enabled" function and found that board is not booting.
> Reason is, unused_clk_function turns off IA_55 and dmac clocks.
>
> On further investigation, turning off IA55_CLK[1] and DMAC_ACLK[2]
> leading GIC interrupts failure.
>
> I made IA55_CLK and DMAC_ACLK as critical clocks as even if, we disable the corresponding driver,
> GIC interrupts should work and with that I am able to mount rootFS.
>
> So I guess fixing "rzg2l_mod_clock_is_enabled" and Adding critical clocks "IA55_CLK" and "DMAC_ACLK"
> Should be a single patch??

Depends on the order, if they are separate patches ;-)
I think it makes sense to have two separate patches, and thus add
the critical clocks first.

> Then I tested DMA it was failing, as driver is not turning ON DMA_PCLK. So added PM
> Routines to handle DMA clocks and with that DMA driver is working. This will be a separate
> Patch for dmac driver.

OK.

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 v3 4/4] clk: renesas: r9a07g044: 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>
---
v2->v3:
 * Updated the commit header
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 90bbeb03308c..8956828d7fca 100644
--- a/drivers/clk/renesas/r9a07g044-cpg.c
+++ b/drivers/clk/renesas/r9a07g044-cpg.c
@@ -138,6 +138,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,
@@ -182,6 +190,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 v3 4/4] clk: renesas: r9a07g044: Add GbEthernet clock/reset

[Geert Uytterhoeven]

On Sun, Aug 15, 2021 at 12:30 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> 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>

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