diff mbox series

[1/4] clk: renesas: rzv2h-cpg: Add support for coupled clock

Message ID 20250303110433.76576-2-biju.das.jz@bp.renesas.com (mailing list archive)
State New
Delegated to: Geert Uytterhoeven
Headers show
Series Add RZ/G3E XSPI clocks | expand

Commit Message

Biju Das March 3, 2025, 11:04 a.m. UTC 
The spi and spix2 clk share same bit for clock gating. Add support
for coupled clock with checking the monitor bit for both the clocks.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/clk/renesas/rzv2h-cpg.c | 83 ++++++++++++++++++++++++++++++++-
 drivers/clk/renesas/rzv2h-cpg.h | 19 ++++++--
 2 files changed, 97 insertions(+), 5 deletions(-)

Comments

Stephen Boyd March 5, 2025, 11:16 p.m. UTC | #1 
Quoting Biju Das (2025-03-03 03:04:19)
> The spi and spix2 clk share same bit for clock gating. Add support
> for coupled clock with checking the monitor bit for both the clocks.

Could you add an intermediate parent clk of both spi and spix2 that only
handles the enable bit for clock gating? Then the enable count handling
would be in the core clk code.
Biju Das March 6, 2025, 10:10 a.m. UTC | #2 
Hi Stephen,

Thanks for the feedback.

> -----Original Message-----
> From: Stephen Boyd <sboyd@kernel.org>
> Sent: 05 March 2025 23:17
> Subject: Re: [PATCH 1/4] clk: renesas: rzv2h-cpg: Add support for coupled clock
> 
> Quoting Biju Das (2025-03-03 03:04:19)
> > The spi and spix2 clk share same bit for clock gating. Add support for
> > coupled clock with checking the monitor bit for both the clocks.
> 
> Could you add an intermediate parent clk of both spi and spix2 that only handles the enable bit for
> clock gating? Then the enable count handling would be in the core clk code.

The parent clock rate of spi and spix2 are different. If we use an intermediate parent clk,
What clk rate the parent will use??

The parent of spix2 and grand parent of spi are same. It is a mux.

Mux->spix2->clk gating
Mux->divider->spi->clk gating 

Cheers,
Biju
Stephen Boyd March 6, 2025, 10:36 p.m. UTC | #3 
Quoting Biju Das (2025-03-06 02:10:50)
> > From: Stephen Boyd <sboyd@kernel.org>
> > Quoting Biju Das (2025-03-03 03:04:19)
> > > The spi and spix2 clk share same bit for clock gating. Add support for
> > > coupled clock with checking the monitor bit for both the clocks.
> > 
> > Could you add an intermediate parent clk of both spi and spix2 that only handles the enable bit for
> > clock gating? Then the enable count handling would be in the core clk code.
> 
> The parent clock rate of spi and spix2 are different. If we use an intermediate parent clk,
> What clk rate the parent will use??

Alright, got it. Does the consumer care about the difference between the
two clks for the gating part? Presumably it's all the same SPI driver
here, so could it ignore the second clk and do something like
clk_bulk_enable()?

Put another way, why does the consumer care that there are two clks? The
hardware seems to want them to be the same thing for gating.

> 
> The parent of spix2 and grand parent of spi are same. It is a mux.
> 
> Mux->spix2->clk gating
> Mux->divider->spi->clk gating 

Is the divider fixed div-2? Are they supposed to be at some ratio with
respect to each other?
diff mbox series

Patch

diff --git a/drivers/clk/renesas/rzv2h-cpg.c b/drivers/clk/renesas/rzv2h-cpg.c
index 469d29549e8e..19fe225d48ed 100644
--- a/drivers/clk/renesas/rzv2h-cpg.c
+++ b/drivers/clk/renesas/rzv2h-cpg.c
@@ -111,6 +111,8 @@  struct pll_clk {
  * @on_bit: ON/MON bit
  * @mon_index: monitor register offset
  * @mon_bit: montor bit
+ * @enabled: soft state of the clock, if it is coupled with another clock
+ * @sibling: pointer to the other coupled clock
  */
 struct mod_clock {
 	struct rzv2h_cpg_priv *priv;
@@ -121,6 +123,8 @@  struct mod_clock {
 	u8 on_bit;
 	s8 mon_index;
 	u8 mon_bit;
+	bool enabled;
+	struct mod_clock *sibling;
 };
 
 #define to_mod_clock(_hw) container_of(_hw, struct mod_clock, hw)
@@ -573,11 +577,56 @@  static int rzv2h_mod_clock_endisable(struct clk_hw *hw, bool enable)
 
 static int rzv2h_mod_clock_enable(struct clk_hw *hw)
 {
-	return rzv2h_mod_clock_endisable(hw, true);
+	struct mod_clock *clock = to_mod_clock(hw);
+	int ret;
+
+	if (clock->sibling) {
+		struct rzv2h_cpg_priv *priv = clock->priv;
+		unsigned long flags;
+		bool enabled;
+
+		spin_lock_irqsave(&priv->rmw_lock, flags);
+		enabled = clock->sibling->enabled;
+		clock->enabled = true;
+		spin_unlock_irqrestore(&priv->rmw_lock, flags);
+		if (enabled) {
+			ret = rzv2h_mod_clock_is_enabled(&clock->hw);
+			if (!ret) {
+				dev_err(priv->dev, "Failed CLK_MON_ON 0x%x/%pC\n",
+					GET_CLK_MON_OFFSET(clock->mon_index), hw->clk);
+				ret = -ETIMEDOUT;
+			} else {
+				ret = 0;
+			}
+
+			return ret;
+		}
+	}
+
+	ret = rzv2h_mod_clock_endisable(hw, true);
+	if (ret)
+		clock->enabled = false;
+
+	return ret;
 }
 
 static void rzv2h_mod_clock_disable(struct clk_hw *hw)
 {
+	struct mod_clock *clock = to_mod_clock(hw);
+
+	if (clock->sibling) {
+		struct rzv2h_cpg_priv *priv = clock->priv;
+		unsigned long flags;
+		bool enabled;
+
+		spin_lock_irqsave(&priv->rmw_lock, flags);
+		enabled = clock->sibling->enabled;
+		clock->enabled = false;
+		spin_unlock_irqrestore(&priv->rmw_lock, flags);
+		if (enabled)
+			return;
+	}
+
 	rzv2h_mod_clock_endisable(hw, false);
 }
 
@@ -587,6 +636,28 @@  static const struct clk_ops rzv2h_mod_clock_ops = {
 	.is_enabled = rzv2h_mod_clock_is_enabled,
 };
 
+static struct mod_clock
+*rzv2h_mod_clock_get_sibling(struct mod_clock *clock,
+			     struct rzv2h_cpg_priv *priv)
+{
+	struct clk_hw *hw;
+	unsigned int i;
+
+	for (i = 0; i < priv->num_mod_clks; i++) {
+		struct mod_clock *clk;
+
+		if (priv->clks[priv->num_core_clks + i] == ERR_PTR(-ENOENT))
+			continue;
+
+		hw = __clk_get_hw(priv->clks[priv->num_core_clks + i]);
+		clk = to_mod_clock(hw);
+		if (clock->on_index == clk->on_index && clock->on_bit == clk->on_bit)
+			return clk;
+	}
+
+	return NULL;
+}
+
 static void __init
 rzv2h_cpg_register_mod_clk(const struct rzv2h_mod_clk *mod,
 			   struct rzv2h_cpg_priv *priv)
@@ -642,6 +713,16 @@  rzv2h_cpg_register_mod_clk(const struct rzv2h_mod_clk *mod,
 	}
 
 	priv->clks[id] = clock->hw.clk;
+	if (mod->is_coupled) {
+		struct mod_clock *sibling;
+
+		clock->enabled = rzv2h_mod_clock_is_enabled(&clock->hw);
+		sibling = rzv2h_mod_clock_get_sibling(clock, priv);
+		if (sibling) {
+			clock->sibling = sibling;
+			sibling->sibling = clock;
+		}
+	}
 
 	/*
 	 * Ensure the module clocks and MSTOP bits are synchronized when they are
diff --git a/drivers/clk/renesas/rzv2h-cpg.h b/drivers/clk/renesas/rzv2h-cpg.h
index b0e32e0c9ffd..4a568fef905d 100644
--- a/drivers/clk/renesas/rzv2h-cpg.h
+++ b/drivers/clk/renesas/rzv2h-cpg.h
@@ -162,6 +162,7 @@  enum clk_types {
  * @on_bit: ON bit
  * @mon_index: monitor register index
  * @mon_bit: monitor bit
+ * @is_coupled: flag to indicate coupled clock
  */
 struct rzv2h_mod_clk {
 	const char *name;
@@ -173,9 +174,11 @@  struct rzv2h_mod_clk {
 	u8 on_bit;
 	s8 mon_index;
 	u8 mon_bit;
+	bool is_coupled;
 };
 
-#define DEF_MOD_BASE(_name, _mstop, _parent, _critical, _no_pm, _onindex, _onbit, _monindex, _monbit) \
+#define DEF_MOD_BASE(_name, _mstop, _parent, _critical, _no_pm, _onindex, \
+		     _onbit, _monindex, _monbit, _iscoupled) \
 	{ \
 		.name = (_name), \
 		.mstop_data = (_mstop), \
@@ -186,16 +189,24 @@  struct rzv2h_mod_clk {
 		.on_bit = (_onbit), \
 		.mon_index = (_monindex), \
 		.mon_bit = (_monbit), \
+		.is_coupled = (_iscoupled), \
 	}
 
 #define DEF_MOD(_name, _parent, _onindex, _onbit, _monindex, _monbit, _mstop) \
-	DEF_MOD_BASE(_name, _mstop, _parent, false, false, _onindex, _onbit, _monindex, _monbit)
+	DEF_MOD_BASE(_name, _mstop, _parent, false, false, _onindex, _onbit, \
+		     _monindex, _monbit, false)
 
 #define DEF_MOD_CRITICAL(_name, _parent, _onindex, _onbit, _monindex, _monbit, _mstop) \
-	DEF_MOD_BASE(_name, _mstop, _parent, true, false, _onindex, _onbit, _monindex, _monbit)
+	DEF_MOD_BASE(_name, _mstop, _parent, true, false, _onindex, _onbit, \
+		     _monindex, _monbit, false)
 
 #define DEF_MOD_NO_PM(_name, _parent, _onindex, _onbit, _monindex, _monbit, _mstop) \
-	DEF_MOD_BASE(_name, _mstop, _parent, false, true, _onindex, _onbit, _monindex, _monbit)
+	DEF_MOD_BASE(_name, _mstop, _parent, false, true, _onindex, _onbit, \
+		     _monindex, _monbit, false)
+
+#define DEF_COUPLED(_name, _parent, _onindex, _onbit, _monindex, _monbit, _mstop) \
+	DEF_MOD_BASE(_name, _mstop, _parent, false, false, _onindex, _onbit, \
+		     _monindex, _monbit, true)
 
 /**
  * struct rzv2h_reset - Reset definitions