linux-renesas-soc.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/4] Add GbEthernet Clock support
@ 2021-07-27 14:17 Biju Das
  2021-07-27 14:17 ` [PATCH v2 1/4] drivers: clk: renesas: rzg2l-cpg: Add support to handle MUX clocks Biju Das
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Biju Das @ 2021-07-27 14:17 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd
  Cc: Biju Das, Geert Uytterhoeven, Sergei Shtylyov, Andrew Lunn,
	linux-renesas-soc, linux-clk, Chris Paterson, Biju Das,
	Prabhakar Mahadev Lad

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2 1/4] drivers: clk: renesas: rzg2l-cpg: Add support to handle MUX clocks
  2021-07-27 14:17 [PATCH v2 0/4] Add GbEthernet Clock support Biju Das
@ 2021-07-27 14:17 ` Biju Das
  2021-07-27 14:17 ` [PATCH v2 2/4] drivers: clk: renesas: r9a07g044-cpg: Add ethernet clock sources Biju Das
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 11+ messages in thread
From: Biju Das @ 2021-07-27 14:17 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd
  Cc: Biju Das, Geert Uytterhoeven, Sergei Shtylyov, linux-renesas-soc,
	linux-clk, Chris Paterson, Biju Das, Prabhakar Mahadev Lad

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2 2/4] drivers: clk: renesas: r9a07g044-cpg: Add ethernet clock sources
  2021-07-27 14:17 [PATCH v2 0/4] Add GbEthernet Clock support Biju Das
  2021-07-27 14:17 ` [PATCH v2 1/4] drivers: clk: renesas: rzg2l-cpg: Add support to handle MUX clocks Biju Das
@ 2021-07-27 14:17 ` Biju Das
  2021-07-27 14:17 ` [PATCH v2 3/4] drivers: clk: renesas: rzg2l-cpg: Add support to handle coupled clocks Biju Das
  2021-07-27 14:17 ` [PATCH v2 4/4] drivers: clk: renesas: r9a07g044-cpg: Add GbEthernet clock/reset Biju Das
  3 siblings, 0 replies; 11+ messages in thread
From: Biju Das @ 2021-07-27 14:17 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd
  Cc: Biju Das, Geert Uytterhoeven, linux-renesas-soc, linux-clk,
	Chris Paterson, Biju Das, Prabhakar Mahadev Lad

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2 3/4] drivers: clk: renesas: rzg2l-cpg: Add support to handle coupled clocks
  2021-07-27 14:17 [PATCH v2 0/4] Add GbEthernet Clock support Biju Das
  2021-07-27 14:17 ` [PATCH v2 1/4] drivers: clk: renesas: rzg2l-cpg: Add support to handle MUX clocks Biju Das
  2021-07-27 14:17 ` [PATCH v2 2/4] drivers: clk: renesas: r9a07g044-cpg: Add ethernet clock sources Biju Das
@ 2021-07-27 14:17 ` Biju Das
  2021-08-11 10:18   ` Geert Uytterhoeven
  2021-07-27 14:17 ` [PATCH v2 4/4] drivers: clk: renesas: r9a07g044-cpg: Add GbEthernet clock/reset Biju Das
  3 siblings, 1 reply; 11+ messages in thread
From: Biju Das @ 2021-07-27 14:17 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd
  Cc: Biju Das, Geert Uytterhoeven, linux-renesas-soc, linux-clk,
	Chris Paterson, Biju Das, Prabhakar Mahadev Lad

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2 4/4] drivers: clk: renesas: r9a07g044-cpg: Add GbEthernet clock/reset
  2021-07-27 14:17 [PATCH v2 0/4] Add GbEthernet Clock support Biju Das
                   ` (2 preceding siblings ...)
  2021-07-27 14:17 ` [PATCH v2 3/4] drivers: clk: renesas: rzg2l-cpg: Add support to handle coupled clocks Biju Das
@ 2021-07-27 14:17 ` Biju Das
  3 siblings, 0 replies; 11+ messages in thread
From: Biju Das @ 2021-07-27 14:17 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd
  Cc: Biju Das, Geert Uytterhoeven, linux-renesas-soc, linux-clk,
	Chris Paterson, Biju Das, Prabhakar Mahadev Lad

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 3/4] drivers: clk: renesas: rzg2l-cpg: Add support to handle coupled clocks
  2021-07-27 14:17 ` [PATCH v2 3/4] drivers: clk: renesas: rzg2l-cpg: Add support to handle coupled clocks Biju Das
@ 2021-08-11 10:18   ` Geert Uytterhoeven
  2021-08-12  6:59     ` Biju Das
  0 siblings, 1 reply; 11+ messages in thread
From: Geert Uytterhoeven @ 2021-08-11 10:18 UTC (permalink / raw)
  To: Biju Das
  Cc: Michael Turquette, Stephen Boyd, Linux-Renesas, linux-clk,
	Chris Paterson, Biju Das, Prabhakar Mahadev Lad

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH v2 3/4] drivers: clk: renesas: rzg2l-cpg: Add support to handle coupled clocks
  2021-08-11 10:18   ` Geert Uytterhoeven
@ 2021-08-12  6:59     ` Biju Das
  2021-08-12  7:54       ` Geert Uytterhoeven
  0 siblings, 1 reply; 11+ messages in thread
From: Biju Das @ 2021-08-12  6:59 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Michael Turquette, Stephen Boyd, Linux-Renesas, linux-clk,
	Chris Paterson, Biju Das, Prabhakar Mahadev Lad

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 3/4] drivers: clk: renesas: rzg2l-cpg: Add support to handle coupled clocks
  2021-08-12  6:59     ` Biju Das
@ 2021-08-12  7:54       ` Geert Uytterhoeven
  2021-08-13 12:16         ` Biju Das
  0 siblings, 1 reply; 11+ messages in thread
From: Geert Uytterhoeven @ 2021-08-12  7:54 UTC (permalink / raw)
  To: Biju Das
  Cc: Michael Turquette, Stephen Boyd, Linux-Renesas, linux-clk,
	Chris Paterson, Biju Das, Prabhakar Mahadev Lad

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH v2 3/4] drivers: clk: renesas: rzg2l-cpg: Add support to handle coupled clocks
  2021-08-12  7:54       ` Geert Uytterhoeven
@ 2021-08-13 12:16         ` Biju Das
  2021-08-13 12:46           ` Geert Uytterhoeven
  0 siblings, 1 reply; 11+ messages in thread
From: Biju Das @ 2021-08-13 12:16 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Michael Turquette, Stephen Boyd, Linux-Renesas, linux-clk,
	Chris Paterson, Biju Das, Prabhakar Mahadev Lad

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 



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 3/4] drivers: clk: renesas: rzg2l-cpg: Add support to handle coupled clocks
  2021-08-13 12:16         ` Biju Das
@ 2021-08-13 12:46           ` Geert Uytterhoeven
  2021-08-13 18:01             ` Biju Das
  0 siblings, 1 reply; 11+ messages in thread
From: Geert Uytterhoeven @ 2021-08-13 12:46 UTC (permalink / raw)
  To: Biju Das
  Cc: Michael Turquette, Stephen Boyd, Linux-Renesas, linux-clk,
	Chris Paterson, Biju Das, Prabhakar Mahadev Lad

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH v2 3/4] drivers: clk: renesas: rzg2l-cpg: Add support to handle coupled clocks
  2021-08-13 12:46           ` Geert Uytterhoeven
@ 2021-08-13 18:01             ` Biju Das
  0 siblings, 0 replies; 11+ messages in thread
From: Biju Das @ 2021-08-13 18:01 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Michael Turquette, Stephen Boyd, Linux-Renesas, linux-clk,
	Chris Paterson, Biju Das, Prabhakar Mahadev Lad

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2021-08-13 18:01 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-27 14:17 [PATCH v2 0/4] Add GbEthernet Clock support Biju Das
2021-07-27 14:17 ` [PATCH v2 1/4] drivers: clk: renesas: rzg2l-cpg: Add support to handle MUX clocks Biju Das
2021-07-27 14:17 ` [PATCH v2 2/4] drivers: clk: renesas: r9a07g044-cpg: Add ethernet clock sources Biju Das
2021-07-27 14:17 ` [PATCH v2 3/4] drivers: clk: renesas: rzg2l-cpg: Add support to handle coupled clocks Biju Das
2021-08-11 10:18   ` Geert Uytterhoeven
2021-08-12  6:59     ` Biju Das
2021-08-12  7:54       ` Geert Uytterhoeven
2021-08-13 12:16         ` Biju Das
2021-08-13 12:46           ` Geert Uytterhoeven
2021-08-13 18:01             ` Biju Das
2021-07-27 14:17 ` [PATCH v2 4/4] drivers: clk: renesas: r9a07g044-cpg: Add GbEthernet clock/reset Biju Das

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).