[PATCH 0/4] Add RZ/G3E XSPI clocks

[PATCH 0/4] Add RZ/G3E XSPI clocks

[Biju Das]

RZ/G3E XSPI has 4 clocks{ahb, axi, spi, spix2). spi and xpix share the
same CPG_ON bit, but they have different monitor bit. Added coupled clk to
handle this. The mux smux2_xspi_clk{0,1} used for selecting spi and spix2
clocks and pllcm33_xspi divider to select different clock rates.

Note:
 This patch series depend upon[1]

[1] https://lore.kernel.org/all/20250228202655.491035-1-prabhakar.mahadev-lad.rj@bp.renesas.com/

Biju Das (4):
  clk: renesas: rzv2h-cpg: Add support for coupled clock
  clk: renesas: rzv2h-cpg: Add support for static dividers
  clk: renesas: r9a09g047: Add support for xspi mux and divider
  clk: renesas: r9a09g047: Add XSPI clock/reset

 drivers/clk/renesas/r9a09g047-cpg.c |  40 ++++++++++
 drivers/clk/renesas/rzv2h-cpg.c     | 112 +++++++++++++++++++++++++++-
 drivers/clk/renesas/rzv2h-cpg.h     |  34 ++++++++-
 3 files changed, 181 insertions(+), 5 deletions(-)

-- 
2.43.0

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

[Biju Das]

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(-)

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
-- 
2.43.0

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

[Stephen Boyd]

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.

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

[Biju Das]

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

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

[Stephen Boyd]

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?

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

[Biju Das]

Hi Stephen,

> -----Original Message-----
> From: Biju Das
> Sent: 07 March 2025 07:02
> Subject: RE: [PATCH 1/4] clk: renesas: rzv2h-cpg: Add support for coupled clock
> 
> Hi Stephen,
> 
> > -----Original Message-----
> > From: Stephen Boyd <sboyd@kernel.org>
> > Sent: 06 March 2025 22:37
> > Subject: RE: [PATCH 1/4] clk: renesas: rzv2h-cpg: Add support for
> > coupled clock
> >
> > 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?
> 
> Although gating bit is same, for some reason their monitor bit is different. So, to confirm clk on
> status we need to check respective monitor bits. Parallelly, I will check with hardware team, does it
> need to monitor both these bits??

According to hardware team, the spix2 clock is twice the frequency of the spi clock, and the clock ON/OFF
period displayed for each bit in the monitor register varies slightly due to the difference in frequency.

So to check the status after changing the clock ON/OFF register setting,
please check the two monitor register bits together


Cheers,
Biju

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

[Stephen Boyd]

Quoting Biju Das (2025-03-14 00:18:33)
> Hi Stephen,
> 
> > -----Original Message-----
> > From: Biju Das
> > Sent: 07 March 2025 07:02
> > Subject: RE: [PATCH 1/4] clk: renesas: rzv2h-cpg: Add support for coupled clock
> > 
> > Hi Stephen,
> > 
> > > -----Original Message-----
> > > From: Stephen Boyd <sboyd@kernel.org>
> > > Sent: 06 March 2025 22:37
> > > Subject: RE: [PATCH 1/4] clk: renesas: rzv2h-cpg: Add support for
> > > coupled clock
> > >
> > > 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?
> > 
> > Although gating bit is same, for some reason their monitor bit is different. So, to confirm clk on
> > status we need to check respective monitor bits. Parallelly, I will check with hardware team, does it
> > need to monitor both these bits??
> 
> According to hardware team, the spix2 clock is twice the frequency of the spi clock, and the clock ON/OFF
> period displayed for each bit in the monitor register varies slightly due to the difference in frequency.
> 
> So to check the status after changing the clock ON/OFF register setting,
> please check the two monitor register bits together
> 

That answers the hardware side of the question. Why does software need
to care that they're two different things vs. one clk?

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

[Biju Das]

Hi Stephen,

> -----Original Message-----
> From: Stephen Boyd <sboyd@kernel.org>
> Sent: 20 March 2025 21:57
> Subject: RE: [PATCH 1/4] clk: renesas: rzv2h-cpg: Add support for coupled clock
> 
> Quoting Biju Das (2025-03-14 00:18:33)
> > Hi Stephen,
> >
> > > -----Original Message-----
> > > From: Biju Das
> > > Sent: 07 March 2025 07:02
> > > Subject: RE: [PATCH 1/4] clk: renesas: rzv2h-cpg: Add support for
> > > coupled clock
> > >
> > > Hi Stephen,
> > >
> > > > -----Original Message-----
> > > > From: Stephen Boyd <sboyd@kernel.org>
> > > > Sent: 06 March 2025 22:37
> > > > Subject: RE: [PATCH 1/4] clk: renesas: rzv2h-cpg: Add support for
> > > > coupled clock
> > > >
> > > > 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?
> > >
> > > Although gating bit is same, for some reason their monitor bit is
> > > different. So, to confirm clk on status we need to check respective
> > > monitor bits. Parallelly, I will check with hardware team, does it need to monitor both these
> bits??
> >
> > According to hardware team, the spix2 clock is twice the frequency of
> > the spi clock, and the clock ON/OFF period displayed for each bit in the monitor register varies
> slightly due to the difference in frequency.
> >
> > So to check the status after changing the clock ON/OFF register
> > setting, please check the two monitor register bits together
> >
> 
> That answers the hardware side of the question. Why does software need to care that they're two
> different things vs. one clk?

From software point, Consumer driver bother only about spi_clk.

So, treating as one clk(spi_clk) should be OK and we should drop
handling spi_x2 module clk in the clk driver instead treat this as an internal clock
(".spi_clk_x2")??

Then we should update the binding to have only 3 module clks instead of 4 by dropping
the spi_x2 module clk.

Geert, what is your opinion on this?

Example:
	DEF_SDIV(".spi_clk_x2", CLK_PLLCM33_XSPI, CLK_SMUX2_XSPI_CLK1, CSDIV0_DIVCTL3,
		 dtable_2_16),
	DEF_FIXED(".pllcm33_xspi_div2", CLK_PLLCM33_XSPI_DIV2, CLK_PLLCM33_XSPI, 1, 2),


	DEF_MOD("spi_clk_spi",		      CLK_PLLCM33_XSPI_DIV2, 10, 1, 5, 1,
						BUS_MSTOP(4, BIT(5))),

Note:
Currently I am facing an issue which is popped up using single clock,
If I use spi clock for rpm, then flash write is failing. If it is turned
on permanently then there is no issue.

Cheers,
Biju

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

[Stephen Boyd]

Quoting Biju Das (2025-03-21 07:21:24)
> > -----Original Message-----
> > From: Stephen Boyd <sboyd@kernel.org>
> > > > > >
> > > > > > 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?
> > > >
> > > > Although gating bit is same, for some reason their monitor bit is
> > > > different. So, to confirm clk on status we need to check respective
> > > > monitor bits. Parallelly, I will check with hardware team, does it need to monitor both these
> > bits??
> > >
> > > According to hardware team, the spix2 clock is twice the frequency of
> > > the spi clock, and the clock ON/OFF period displayed for each bit in the monitor register varies
> > slightly due to the difference in frequency.
> > >
> > > So to check the status after changing the clock ON/OFF register
> > > setting, please check the two monitor register bits together
> > >
> > 
> > That answers the hardware side of the question. Why does software need to care that they're two
> > different things vs. one clk?
> 
> From software point, Consumer driver bother only about spi_clk.
> 
> So, treating as one clk(spi_clk) should be OK and we should drop
> handling spi_x2 module clk in the clk driver instead treat this as an internal clock
> (".spi_clk_x2")??
> 
> Then we should update the binding to have only 3 module clks instead of 4 by dropping
> the spi_x2 module clk.

I don't see why the binding has to be updated. Can't we return a NULL
clk pointer when the driver calls clk_get() on the specifier for the
spi_x2 clk? Then nothing will happen for that clk. I guess we may need
to return the rate of the spi clk multiplied by 2 or something, but that
is far simpler to implement than arbitrating the hardware with custom
logic and meets the same result.

> 
> Geert, what is your opinion on this?
> 
> Example:
>         DEF_SDIV(".spi_clk_x2", CLK_PLLCM33_XSPI, CLK_SMUX2_XSPI_CLK1, CSDIV0_DIVCTL3,
>                  dtable_2_16),
>         DEF_FIXED(".pllcm33_xspi_div2", CLK_PLLCM33_XSPI_DIV2, CLK_PLLCM33_XSPI, 1, 2),
> 
> 
>         DEF_MOD("spi_clk_spi",                CLK_PLLCM33_XSPI_DIV2, 10, 1, 5, 1,
>                                                 BUS_MSTOP(4, BIT(5))),
> 
> Note:
> Currently I am facing an issue which is popped up using single clock,
> If I use spi clock for rpm, then flash write is failing. If it is turned
> on permanently then there is no issue.

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

[Biju Das]

Hi Stephen,

> -----Original Message-----
> From: Stephen Boyd <sboyd@kernel.org>
> Sent: 24 March 2025 23:48
> Subject: RE: [PATCH 1/4] clk: renesas: rzv2h-cpg: Add support for coupled clock
> 
> Quoting Biju Das (2025-03-21 07:21:24)
> > > -----Original Message-----
> > > From: Stephen Boyd <sboyd@kernel.org>
> > > > > > >
> > > > > > > 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?
> > > > >
> > > > > Although gating bit is same, for some reason their monitor bit
> > > > > is different. So, to confirm clk on status we need to check
> > > > > respective monitor bits. Parallelly, I will check with hardware
> > > > > team, does it need to monitor both these
> > > bits??
> > > >
> > > > According to hardware team, the spix2 clock is twice the frequency
> > > > of the spi clock, and the clock ON/OFF period displayed for each
> > > > bit in the monitor register varies
> > > slightly due to the difference in frequency.
> > > >
> > > > So to check the status after changing the clock ON/OFF register
> > > > setting, please check the two monitor register bits together
> > > >
> > >
> > > That answers the hardware side of the question. Why does software
> > > need to care that they're two different things vs. one clk?
> >
> > From software point, Consumer driver bother only about spi_clk.
> >
> > So, treating as one clk(spi_clk) should be OK and we should drop
> > handling spi_x2 module clk in the clk driver instead treat this as an
> > internal clock (".spi_clk_x2")??
> >
> > Then we should update the binding to have only 3 module clks instead
> > of 4 by dropping the spi_x2 module clk.
> 
> I don't see why the binding has to be updated. Can't we return a NULL clk pointer when the driver
> calls clk_get() on the specifier for the
> spi_x2 clk? 

Same specifier is used for both clocks. So, we cannot distinguish from DT entries.

> Then nothing will happen for that clk. I guess we may need to return the rate of the spi
> clk multiplied by 2 or something, but that is far simpler to implement than arbitrating the hardware
> with custom logic and meets the same result.


Maybe, during registering, treat spix2 as an ignore clock and store its pointer(eg: sibling).

During get(), return the stored pointer(by introducing new type CPG_MOD_COUPLED =2 in DT) for spix2.
This needs binding modification.

Now we have clk and clk->sibling for spi and spix2. With this we get proper rate. 

[    2.927484] rzv2h-cpg 10420000.clock-controller: clock (1, 161) is spi_clk_spi at 20000000 Hz
[    2.945706] rzv2h-cpg 10420000.clock-controller: clock (2, 161) is spi_clk_spix2 at 40000000 Hz

Enable/disable calls ignore for spix2. But it returns proper rate for both spi and spix2.

Is this approach, ok?

Cheers,
Biju

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

[Biju Das]

Hi Stephen,

> -----Original Message-----
> From: Biju Das
> Sent: 25 March 2025 12:23
> Subject: RE: [PATCH 1/4] clk: renesas: rzv2h-cpg: Add support for coupled clock
> 
> Hi Stephen,
> 
> > -----Original Message-----
> > From: Stephen Boyd <sboyd@kernel.org>
> > Sent: 24 March 2025 23:48
> > Subject: RE: [PATCH 1/4] clk: renesas: rzv2h-cpg: Add support for
> > coupled clock
> >
> > Quoting Biju Das (2025-03-21 07:21:24)
> > > > -----Original Message-----
> > > > From: Stephen Boyd <sboyd@kernel.org>
> > > > > > > >
> > > > > > > > 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?
> > > > > >
> > > > > > Although gating bit is same, for some reason their monitor bit
> > > > > > is different. So, to confirm clk on status we need to check
> > > > > > respective monitor bits. Parallelly, I will check with
> > > > > > hardware team, does it need to monitor both these
> > > > bits??
> > > > >
> > > > > According to hardware team, the spix2 clock is twice the
> > > > > frequency of the spi clock, and the clock ON/OFF period
> > > > > displayed for each bit in the monitor register varies
> > > > slightly due to the difference in frequency.
> > > > >
> > > > > So to check the status after changing the clock ON/OFF register
> > > > > setting, please check the two monitor register bits together
> > > > >
> > > >
> > > > That answers the hardware side of the question. Why does software
> > > > need to care that they're two different things vs. one clk?
> > >
> > > From software point, Consumer driver bother only about spi_clk.
> > >
> > > So, treating as one clk(spi_clk) should be OK and we should drop
> > > handling spi_x2 module clk in the clk driver instead treat this as
> > > an internal clock (".spi_clk_x2")??
> > >
> > > Then we should update the binding to have only 3 module clks instead
> > > of 4 by dropping the spi_x2 module clk.
> >
> > I don't see why the binding has to be updated. Can't we return a NULL
> > clk pointer when the driver calls clk_get() on the specifier for the
> > spi_x2 clk?
> 
> Same specifier is used for both clocks. So, we cannot distinguish from DT entries.
> 
> > Then nothing will happen for that clk. I guess we may need to return
> > the rate of the spi clk multiplied by 2 or something, but that is far
> > simpler to implement than arbitrating the hardware with custom logic and meets the same result.
> 
> 
> Maybe, during registering, treat spix2 as an ignore clock and store its pointer(eg: sibling).
> 
> During get(), return the stored pointer(by introducing new type CPG_MOD_COUPLED =2 in DT) for spix2.
> This needs binding modification.
> 
> Now we have clk and clk->sibling for spi and spix2. With this we get proper rate.
> 
> [    2.927484] rzv2h-cpg 10420000.clock-controller: clock (1, 161) is spi_clk_spi at 20000000 Hz
> [    2.945706] rzv2h-cpg 10420000.clock-controller: clock (2, 161) is spi_clk_spix2 at 40000000 Hz
> 
> Enable/disable calls ignore for spix2. But it returns proper rate for both spi and spix2.
> 
> Is this approach, ok?
> 

Another option is handle this as core clock, currently core clock does not gate in this driver.
With this, clock specifier can distinguish both the clocks.

                                 <&cpg CPG_MOD 0xa1>,
+                                <&cpg CPG_CORE R9A09G047_SPI_CLK_SPIX2>;


Cheers,
Biju

[PATCH 2/4] clk: renesas: rzv2h-cpg: Add support for static dividers

[Biju Das]

Add support for static dividers that does not need rmw operation.
This will avoid unnecessary memory allocation and using associated
legacy APIs.

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

diff --git a/drivers/clk/renesas/rzv2h-cpg.c b/drivers/clk/renesas/rzv2h-cpg.c
index 19fe225d48ed..42a517e11d42 100644
--- a/drivers/clk/renesas/rzv2h-cpg.c
+++ b/drivers/clk/renesas/rzv2h-cpg.c
@@ -349,6 +349,32 @@ rzv2h_cpg_ddiv_clk_register(const struct cpg_core_clk *core,
 	return div->hw.clk;
 }
 
+static struct clk * __init
+rzv2h_cpg_sdiv_clk_register(const struct cpg_core_clk *core, struct rzv2h_cpg_priv *priv)
+{
+	struct ddiv cfg_ddiv = core->cfg.ddiv;
+	const struct clk *parent;
+	const char *parent_name;
+	struct clk_hw *clk_hw;
+
+	parent = priv->clks[core->parent];
+	if (IS_ERR(parent))
+		return ERR_CAST(parent);
+
+	parent_name = __clk_get_name(parent);
+	clk_hw = clk_hw_register_divider_table(priv->dev, core->name,
+					       parent_name, 0,
+					       priv->base + cfg_ddiv.offset,
+					       cfg_ddiv.shift, cfg_ddiv.width,
+					       core->flag, core->dtable,
+					       &priv->rmw_lock);
+
+	if (IS_ERR(clk_hw))
+		return ERR_CAST(clk_hw);
+
+	return clk_hw->clk;
+}
+
 static struct clk * __init
 rzv2h_cpg_mux_clk_register(const struct cpg_core_clk *core,
 			   struct rzv2h_cpg_priv *priv)
@@ -451,6 +477,9 @@ rzv2h_cpg_register_core_clk(const struct cpg_core_clk *core,
 	case CLK_TYPE_DDIV:
 		clk = rzv2h_cpg_ddiv_clk_register(core, priv);
 		break;
+	case CLK_TYPE_SDIV:
+		clk = rzv2h_cpg_sdiv_clk_register(core, priv);
+		break;
 	case CLK_TYPE_SMUX:
 		clk = rzv2h_cpg_mux_clk_register(core, priv);
 		break;
diff --git a/drivers/clk/renesas/rzv2h-cpg.h b/drivers/clk/renesas/rzv2h-cpg.h
index 4a568fef905d..1905e3a4afad 100644
--- a/drivers/clk/renesas/rzv2h-cpg.h
+++ b/drivers/clk/renesas/rzv2h-cpg.h
@@ -115,6 +115,7 @@ enum clk_types {
 	CLK_TYPE_FF,		/* Fixed Factor Clock */
 	CLK_TYPE_PLL,
 	CLK_TYPE_DDIV,		/* Dynamic Switching Divider */
+	CLK_TYPE_SDIV,		/* Static Switching Divider */
 	CLK_TYPE_SMUX,		/* Static Mux */
 };
 
@@ -142,6 +143,12 @@ enum clk_types {
 		.flag = CLK_DIVIDER_HIWORD_MASK)
 #define DEF_CSDIV(_name, _id, _parent, _ddiv_packed, _dtable) \
 	DEF_DDIV(_name, _id, _parent, _ddiv_packed, _dtable)
+#define DEF_SDIV(_name, _id, _parent, _ddiv_packed, _dtable) \
+	DEF_TYPE(_name, _id, CLK_TYPE_SDIV, \
+		.cfg.ddiv = _ddiv_packed, \
+		.parent = _parent, \
+		.dtable = _dtable, \
+		.flag = CLK_DIVIDER_HIWORD_MASK)
 #define DEF_SMUX(_name, _id, _smux_packed, _parent_names) \
 	DEF_TYPE(_name, _id, CLK_TYPE_SMUX, \
 		 .cfg.smux = _smux_packed, \
-- 
2.43.0

[PATCH 3/4] clk: renesas: r9a09g047: Add support for xspi mux and divider

[Biju Das]

The mux smux2_xspi_clk{0,1} used for selecting spi and spix2 clocks and
pllcm33_xspi divider to select different clock rates. Add support for
both.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/clk/renesas/r9a09g047-cpg.c | 25 +++++++++++++++++++++++++
 drivers/clk/renesas/rzv2h-cpg.h     |  7 +++++++
 2 files changed, 32 insertions(+)

diff --git a/drivers/clk/renesas/r9a09g047-cpg.c b/drivers/clk/renesas/r9a09g047-cpg.c
index ff015b3b4d2f..05d8ccc81157 100644
--- a/drivers/clk/renesas/r9a09g047-cpg.c
+++ b/drivers/clk/renesas/r9a09g047-cpg.c
@@ -31,7 +31,13 @@ enum clk_ids {
 	CLK_PLLVDO,
 
 	/* Internal Core Clocks */
+	CLK_PLLCM33_DIV3,
+	CLK_PLLCM33_DIV4,
+	CLK_PLLCM33_DIV5,
 	CLK_PLLCM33_DIV16,
+	CLK_SMUX2_XSPI_CLK0,
+	CLK_SMUX2_XSPI_CLK1,
+	CLK_PLLCM33_XSPI,
 	CLK_PLLCLN_DIV2,
 	CLK_PLLCLN_DIV8,
 	CLK_PLLCLN_DIV16,
@@ -60,6 +66,14 @@ static const struct clk_div_table dtable_2_4[] = {
 	{0, 0},
 };
 
+static const struct clk_div_table dtable_2_16[] = {
+	{0, 2},
+	{1, 4},
+	{2, 8},
+	{3, 16},
+	{0, 0},
+};
+
 static const struct clk_div_table dtable_2_64[] = {
 	{0, 2},
 	{1, 4},
@@ -69,6 +83,10 @@ static const struct clk_div_table dtable_2_64[] = {
 	{0, 0},
 };
 
+/* Mux clock tables */
+static const char * const smux2_xspi_clk0[] = { ".pllcm33_div3", ".pllcm33_div4" };
+static const char * const smux2_xspi_clk1[] = { ".smux2_xspi_clk0", ".pllcm33_div5" };
+
 static const struct cpg_core_clk r9a09g047_core_clks[] __initconst = {
 	/* External Clock Inputs */
 	DEF_INPUT("audio_extal", CLK_AUDIO_EXTAL),
@@ -83,8 +101,15 @@ static const struct cpg_core_clk r9a09g047_core_clks[] __initconst = {
 	DEF_FIXED(".pllvdo", CLK_PLLVDO, CLK_QEXTAL, 105, 2),
 
 	/* Internal Core Clocks */
+	DEF_FIXED(".pllcm33_div3", CLK_PLLCM33_DIV3, CLK_PLLCM33, 1, 3),
+	DEF_FIXED(".pllcm33_div4", CLK_PLLCM33_DIV4, CLK_PLLCM33, 1, 4),
+	DEF_FIXED(".pllcm33_div5", CLK_PLLCM33_DIV5, CLK_PLLCM33, 1, 5),
 	DEF_FIXED(".pllcm33_div16", CLK_PLLCM33_DIV16, CLK_PLLCM33, 1, 16),
 
+	DEF_SMUX(".smux2_xspi_clk0", CLK_SMUX2_XSPI_CLK0, SSEL1_SELCTL2, smux2_xspi_clk0),
+	DEF_SMUX(".smux2_xspi_clk1", CLK_SMUX2_XSPI_CLK1, SSEL1_SELCTL3, smux2_xspi_clk1),
+	DEF_SDIV(".pllcm33_xspi", CLK_PLLCM33_XSPI, CLK_SMUX2_XSPI_CLK1, CSDIV0_DIVCTL3,
+		 dtable_2_16),
 	DEF_FIXED(".pllcln_div2", CLK_PLLCLN_DIV2, CLK_PLLCLN, 1, 2),
 	DEF_FIXED(".pllcln_div8", CLK_PLLCLN_DIV8, CLK_PLLCLN, 1, 8),
 	DEF_FIXED(".pllcln_div16", CLK_PLLCLN_DIV16, CLK_PLLCLN, 1, 16),
diff --git a/drivers/clk/renesas/rzv2h-cpg.h b/drivers/clk/renesas/rzv2h-cpg.h
index 1905e3a4afad..1f0e67f33cf9 100644
--- a/drivers/clk/renesas/rzv2h-cpg.h
+++ b/drivers/clk/renesas/rzv2h-cpg.h
@@ -61,10 +61,12 @@ struct smuxed {
 		.width = _width, \
 	})
 
+#define CPG_SSEL1		(0x304)
 #define CPG_CDDIV0		(0x400)
 #define CPG_CDDIV1		(0x404)
 #define CPG_CDDIV3		(0x40C)
 #define CPG_CDDIV4		(0x410)
+#define CPG_CSDIV0		(0x500)
 
 #define CDDIV0_DIVCTL2	DDIV_PACK(CPG_CDDIV0, 8, 3, 2)
 #define CDDIV1_DIVCTL0	DDIV_PACK(CPG_CDDIV1, 0, 2, 4)
@@ -76,6 +78,11 @@ struct smuxed {
 #define CDDIV4_DIVCTL1	DDIV_PACK(CPG_CDDIV4, 4, 1, 17)
 #define CDDIV4_DIVCTL2	DDIV_PACK(CPG_CDDIV4, 8, 1, 18)
 
+#define CSDIV0_DIVCTL3	DDIV_PACK(CPG_CSDIV0, 12, 2, CSDIV_NO_MON)
+
+#define SSEL1_SELCTL2	SMUX_PACK(CPG_SSEL1, 8, 1)
+#define SSEL1_SELCTL3	SMUX_PACK(CPG_SSEL1, 12, 1)
+
 #define BUS_MSTOP_IDX_MASK	GENMASK(31, 16)
 #define BUS_MSTOP_BITS_MASK	GENMASK(15, 0)
 #define BUS_MSTOP(idx, mask)	(FIELD_PREP_CONST(BUS_MSTOP_IDX_MASK, (idx)) | \
-- 
2.43.0

[PATCH 4/4] clk: renesas: r9a09g047: Add XSPI clock/reset

[Biju Das]

Add XSPI clock and reset entries.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/clk/renesas/r9a09g047-cpg.c | 15 +++++++++++++++
 drivers/clk/renesas/rzv2h-cpg.h     |  1 +
 2 files changed, 16 insertions(+)

diff --git a/drivers/clk/renesas/r9a09g047-cpg.c b/drivers/clk/renesas/r9a09g047-cpg.c
index 05d8ccc81157..438077b13198 100644
--- a/drivers/clk/renesas/r9a09g047-cpg.c
+++ b/drivers/clk/renesas/r9a09g047-cpg.c
@@ -35,9 +35,11 @@ enum clk_ids {
 	CLK_PLLCM33_DIV4,
 	CLK_PLLCM33_DIV5,
 	CLK_PLLCM33_DIV16,
+	CLK_PLLCM33_GEAR,
 	CLK_SMUX2_XSPI_CLK0,
 	CLK_SMUX2_XSPI_CLK1,
 	CLK_PLLCM33_XSPI,
+	CLK_PLLCM33_XSPI_DIV2,
 	CLK_PLLCLN_DIV2,
 	CLK_PLLCLN_DIV8,
 	CLK_PLLCLN_DIV16,
@@ -106,10 +108,13 @@ static const struct cpg_core_clk r9a09g047_core_clks[] __initconst = {
 	DEF_FIXED(".pllcm33_div5", CLK_PLLCM33_DIV5, CLK_PLLCM33, 1, 5),
 	DEF_FIXED(".pllcm33_div16", CLK_PLLCM33_DIV16, CLK_PLLCM33, 1, 16),
 
+	DEF_DDIV(".pllcm33_gear", CLK_PLLCM33_GEAR, CLK_PLLCM33_DIV4, CDDIV0_DIVCTL1, dtable_2_64),
+
 	DEF_SMUX(".smux2_xspi_clk0", CLK_SMUX2_XSPI_CLK0, SSEL1_SELCTL2, smux2_xspi_clk0),
 	DEF_SMUX(".smux2_xspi_clk1", CLK_SMUX2_XSPI_CLK1, SSEL1_SELCTL3, smux2_xspi_clk1),
 	DEF_SDIV(".pllcm33_xspi", CLK_PLLCM33_XSPI, CLK_SMUX2_XSPI_CLK1, CSDIV0_DIVCTL3,
 		 dtable_2_16),
+	DEF_FIXED(".pllcm33_xspi_div2", CLK_PLLCM33_XSPI_DIV2, CLK_PLLCM33_XSPI, 1, 2),
 	DEF_FIXED(".pllcln_div2", CLK_PLLCLN_DIV2, CLK_PLLCLN, 1, 2),
 	DEF_FIXED(".pllcln_div8", CLK_PLLCLN_DIV8, CLK_PLLCLN, 1, 8),
 	DEF_FIXED(".pllcln_div16", CLK_PLLCLN_DIV16, CLK_PLLCLN, 1, 16),
@@ -178,6 +183,14 @@ static const struct rzv2h_mod_clk r9a09g047_mod_clks[] __initconst = {
 						BUS_MSTOP(10, BIT(14))),
 	DEF_MOD("canfd_0_clkc",			CLK_PLLCLN_DIV20, 9, 14, 4, 30,
 						BUS_MSTOP(10, BIT(14))),
+	DEF_MOD("spi_hclk",			CLK_PLLCM33_GEAR, 9, 15, 4, 31,
+						BUS_MSTOP(4, BIT(5))),
+	DEF_MOD("spi_aclk",			CLK_PLLCM33_GEAR, 10, 0, 5, 0,
+						BUS_MSTOP(4, BIT(5))),
+	DEF_COUPLED("spi_clk_spi",		CLK_PLLCM33_XSPI_DIV2, 10, 1, 5, 1,
+						BUS_MSTOP(4, BIT(5))),
+	DEF_COUPLED("spi_clk_spix2",		CLK_PLLCM33_XSPI, 10, 1, 5, 2,
+						BUS_MSTOP(4, BIT(5))),
 	DEF_MOD("sdhi_0_imclk",			CLK_PLLCLN_DIV8, 10, 3, 5, 3,
 						BUS_MSTOP(8, BIT(2))),
 	DEF_MOD("sdhi_0_imclk2",		CLK_PLLCLN_DIV8, 10, 4, 5, 4,
@@ -230,6 +243,8 @@ static const struct rzv2h_reset r9a09g047_resets[] __initconst = {
 	DEF_RST(10, 0, 4, 17),		/* RIIC_8_MRST */
 	DEF_RST(10, 1, 4, 18),		/* CANFD_0_RSTP_N */
 	DEF_RST(10, 2, 4, 19),		/* CANFD_0_RSTC_N */
+	DEF_RST(10, 3, 4, 20),		/* SPI_HRESETN */
+	DEF_RST(10, 4, 4, 21),		/* SPI_ARESETN */
 	DEF_RST(10, 7, 4, 24),		/* SDHI_0_IXRST */
 	DEF_RST(10, 8, 4, 25),		/* SDHI_1_IXRST */
 	DEF_RST(10, 9, 4, 26),		/* SDHI_2_IXRST */
diff --git a/drivers/clk/renesas/rzv2h-cpg.h b/drivers/clk/renesas/rzv2h-cpg.h
index 1f0e67f33cf9..3e95236b3b63 100644
--- a/drivers/clk/renesas/rzv2h-cpg.h
+++ b/drivers/clk/renesas/rzv2h-cpg.h
@@ -68,6 +68,7 @@ struct smuxed {
 #define CPG_CDDIV4		(0x410)
 #define CPG_CSDIV0		(0x500)
 
+#define CDDIV0_DIVCTL1	DDIV_PACK(CPG_CDDIV0, 4, 3, 1)
 #define CDDIV0_DIVCTL2	DDIV_PACK(CPG_CDDIV0, 8, 3, 2)
 #define CDDIV1_DIVCTL0	DDIV_PACK(CPG_CDDIV1, 0, 2, 4)
 #define CDDIV1_DIVCTL1	DDIV_PACK(CPG_CDDIV1, 4, 2, 5)
-- 
2.43.0

Re: [PATCH 4/4] clk: renesas: r9a09g047: Add XSPI clock/reset

[Geert Uytterhoeven]

Hi Biju,

On Mon, 3 Mar 2025 at 12:04, Biju Das <biju.das.jz@bp.renesas.com> wrote:
>
> Add XSPI clock and reset entries.
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

Thanks for your patch!

> --- a/drivers/clk/renesas/rzv2h-cpg.h
> +++ b/drivers/clk/renesas/rzv2h-cpg.h
> @@ -68,6 +68,7 @@ struct smuxed {
>  #define CPG_CDDIV4             (0x410)
>  #define CPG_CSDIV0             (0x500)
>
> +#define CDDIV0_DIVCTL1 DDIV_PACK(CPG_CDDIV0, 4, 3, 1)
>  #define CDDIV0_DIVCTL2 DDIV_PACK(CPG_CDDIV0, 8, 3, 2)
>  #define CDDIV1_DIVCTL0 DDIV_PACK(CPG_CDDIV1, 0, 2, 4)
>  #define CDDIV1_DIVCTL1 DDIV_PACK(CPG_CDDIV1, 4, 2, 5)

This hunk is already queued as part of commit 4d6952981244d1e4
("clk: renesas: r9a09g057: Add entries for the DMACs") in
renesas-clk-for-v6.15.

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 4/4] clk: renesas: r9a09g047: Add XSPI clock/reset

[Biju Das]

Hi Geert,

Thanks for the feedback.

> -----Original Message-----
> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: 06 March 2025 14:44
> Subject: Re: [PATCH 4/4] clk: renesas: r9a09g047: Add XSPI clock/reset
> 
> Hi Biju,
> 
> On Mon, 3 Mar 2025 at 12:04, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> >
> > Add XSPI clock and reset entries.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> 
> Thanks for your patch!
> 
> > --- a/drivers/clk/renesas/rzv2h-cpg.h
> > +++ b/drivers/clk/renesas/rzv2h-cpg.h
> > @@ -68,6 +68,7 @@ struct smuxed {
> >  #define CPG_CDDIV4             (0x410)
> >  #define CPG_CSDIV0             (0x500)
> >
> > +#define CDDIV0_DIVCTL1 DDIV_PACK(CPG_CDDIV0, 4, 3, 1)
> >  #define CDDIV0_DIVCTL2 DDIV_PACK(CPG_CDDIV0, 8, 3, 2)  #define
> > CDDIV1_DIVCTL0 DDIV_PACK(CPG_CDDIV1, 0, 2, 4)  #define CDDIV1_DIVCTL1
> > DDIV_PACK(CPG_CDDIV1, 4, 2, 5)
> 
> This hunk is already queued as part of commit 4d6952981244d1e4
> ("clk: renesas: r9a09g057: Add entries for the DMACs") in renesas-clk-for-v6.15.

OK will drop it in next version.

Cheers,
Biju