diff mbox series

[v2,6/9] clk: renesas: rzv2h-cpg: Ignore monitoring CLK_MON bits for external clocks

Message ID 20250407165202.197570-7-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive)
State New
Delegated to: Geert Uytterhoeven
Headers show
Series clk: renesas: rzv2h: Add clock and reset entries for USB2 and GBETH | expand

Commit Message

Lad, Prabhakar April 7, 2025, 4:51 p.m. UTC 
From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Ignore CLK_MON bits when turning on/off module clocks that use an external
clock source.

Introduce the `DEF_MOD_EXTERNAL()` macro for defining module clocks that
may have an external clock source. Update `rzv2h_cpg_register_mod_clk()`
to update mon_index.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/clk/renesas/rzv2h-cpg.c | 24 ++++++++++++++++++++++++
 drivers/clk/renesas/rzv2h-cpg.h | 28 ++++++++++++++++++++++++----
 2 files changed, 48 insertions(+), 4 deletions(-)

Comments

Geert Uytterhoeven April 15, 2025, 2:36 p.m. UTC | #1 
Hi Prabhakar,

Thanks for your patch!

On Mon, 7 Apr 2025 at 18:52, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Ignore CLK_MON bits when turning on/off module clocks that use an external
> clock source.
>
> Introduce the `DEF_MOD_EXTERNAL()` macro for defining module clocks that
> may have an external clock source. Update `rzv2h_cpg_register_mod_clk()`
> to update mon_index.

So I guess you implemented this because the external clock was not
running, and you got into an infinite loop?

This looks rather fragile to me. How do you know when the clock
is actually running, and thus usable?

> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

> --- a/drivers/clk/renesas/rzv2h-cpg.c
> +++ b/drivers/clk/renesas/rzv2h-cpg.c
> @@ -569,6 +569,25 @@ static void rzv2h_mod_clock_mstop_disable(struct rzv2h_cpg_priv *priv,
>         spin_unlock_irqrestore(&priv->rmw_lock, flags);
>  }
>
> +static bool rzv2h_mod_clock_is_external(struct rzv2h_cpg_priv *priv,
> +                                       u16 ext_clk_offset,

unsigned int

> +                                       u8 ext_clk_bit,

unsigned int

> +                                       u8 ext_cond)

bool

> +{
> +       u32 value;
> +
> +       if (!ext_clk_offset)
> +               return false;
> +
> +       value = readl(priv->base + ext_clk_offset) & BIT(ext_clk_bit);
> +       value >>= ext_clk_bit;

No need to shift:

    return !!value == ext_cond;

> +
> +       if (value == ext_cond)
> +               return true;
> +
> +       return false;
> +}
> +
>  static int rzv2h_mod_clock_is_enabled(struct clk_hw *hw)
>  {
>         struct mod_clock *clock = to_mod_clock(hw);
> @@ -691,6 +710,11 @@ rzv2h_cpg_register_mod_clk(const struct rzv2h_mod_clk *mod,
>         clock->on_index = mod->on_index;
>         clock->on_bit = mod->on_bit;
>         clock->mon_index = mod->mon_index;
> +       /* If clock is coming from external source ignore the monitor bit for it */
> +       if (rzv2h_mod_clock_is_external(priv, mod->external_clk_offset,
> +                                       mod->external_clk_bit,
> +                                       mod->external_cond))

Perhaps just pass "mod" instead of three of its members, to fully
hide the logic inside the helper function?

> +               clock->mon_index = -1;
>         clock->mon_bit = mod->mon_bit;
>         clock->no_pm = mod->no_pm;
>         clock->priv = priv;

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
Geert Uytterhoeven April 15, 2025, 2:58 p.m. UTC | #2 
Hi Prabhakar,

On Mon, 7 Apr 2025 at 18:52, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Ignore CLK_MON bits when turning on/off module clocks that use an external
> clock source.
>
> Introduce the `DEF_MOD_EXTERNAL()` macro for defining module clocks that
> may have an external clock source. Update `rzv2h_cpg_register_mod_clk()`
> to update mon_index.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

> --- a/drivers/clk/renesas/rzv2h-cpg.c
> +++ b/drivers/clk/renesas/rzv2h-cpg.c
> @@ -569,6 +569,25 @@ static void rzv2h_mod_clock_mstop_disable(struct rzv2h_cpg_priv *priv,
>         spin_unlock_irqrestore(&priv->rmw_lock, flags);
>  }
>
> +static bool rzv2h_mod_clock_is_external(struct rzv2h_cpg_priv *priv,
> +                                       u16 ext_clk_offset,
> +                                       u8 ext_clk_bit,
> +                                       u8 ext_cond)
> +{
> +       u32 value;
> +
> +       if (!ext_clk_offset)
> +               return false;
> +
> +       value = readl(priv->base + ext_clk_offset) & BIT(ext_clk_bit);

As ext_clk_offset is actually the offset of the Static Mux Control
Registers (CPG_SSELm), this reads the current state of the mux.
However, can't the state be changed at runtime (despite it being named
a "static mux")?

> +       value >>= ext_clk_bit;
> +
> +       if (value == ext_cond)
> +               return true;
> +
> +       return false;
> +}
> +
>  static int rzv2h_mod_clock_is_enabled(struct clk_hw *hw)
>  {
>         struct mod_clock *clock = to_mod_clock(hw);

Gr{oetje,eeting}s,

                        Geert
Lad, Prabhakar April 15, 2025, 7:10 p.m. UTC | #3 
Hi Geert,

Thank you for the review.

On Tue, Apr 15, 2025 at 3:36 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> Thanks for your patch!
>
> On Mon, 7 Apr 2025 at 18:52, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Ignore CLK_MON bits when turning on/off module clocks that use an external
> > clock source.
> >
> > Introduce the `DEF_MOD_EXTERNAL()` macro for defining module clocks that
> > may have an external clock source. Update `rzv2h_cpg_register_mod_clk()`
> > to update mon_index.
>
> So I guess you implemented this because the external clock was not
> running, and you got into an infinite loop?
>
Yes, partially right but we didn't enter an infinite loop as we have a timeout.

For the CLK_MON, the HW manual for RZ/V2H section 4.4.4.8 CGC Control
Registers and 4.4.4.10 CGC Monitor Registers will be updated to below
in the next version.
 "The clock gating cells require source clocks to operate correctly.
If the source clocks are stopped, these registers cannot be used."

Currently without the series when we turn ON the clock the CLK_ON bit
gets set and to make sure it's turned ON the corresponding CLK_MON bit
is checked to ensure it's ON. When a request is made to turn ON the
clock first we check the CLK_MON bit and if it's being set we return
early as the clock was ON. This worked OK up until now where the
clocks used were internally generated.

In the case of RGMII interface where the Rx/Rx-180 clock was coming
from an PHY on an external pin the above didn't work as expected. When
we issued an unbind request on the glue driver all the clocks were
gated to OFF state i.e CLK_ON bits were set to '0'. Now when the bind
operation was requested  the clocks were requested to be turned ON, ie
when CLK_MON bits for RX/Rx-180 reported to be '1'  that is because
PHY was providing the clock and due to which the CLK_ON bit was unset
(and not gated to ON state)  due to which the DMA reset operation
failed in dwmac-core  driver.

Below is the thread,
[0] https://lore.kernel.org/all/CA+V-a8uWY1Av8eS1k9C6Td=RuB4PbCnQyXbNLzmhao0nr8Spbg@mail.gmail.com/

> This looks rather fragile to me. How do you know when the clock
> is actually running, and thus usable?
>
I was thinking the consumer driver would request the external device
to turn it ON/OFF.

> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> > --- a/drivers/clk/renesas/rzv2h-cpg.c
> > +++ b/drivers/clk/renesas/rzv2h-cpg.c
> > @@ -569,6 +569,25 @@ static void rzv2h_mod_clock_mstop_disable(struct rzv2h_cpg_priv *priv,
> >         spin_unlock_irqrestore(&priv->rmw_lock, flags);
> >  }
> >
> > +static bool rzv2h_mod_clock_is_external(struct rzv2h_cpg_priv *priv,
> > +                                       u16 ext_clk_offset,
>
> unsigned int
>
> > +                                       u8 ext_clk_bit,
>
> unsigned int
>
> > +                                       u8 ext_cond)
>
> bool
>
Agreed I 'll change to the above.

> > +{
> > +       u32 value;
> > +
> > +       if (!ext_clk_offset)
> > +               return false;
> > +
> > +       value = readl(priv->base + ext_clk_offset) & BIT(ext_clk_bit);
> > +       value >>= ext_clk_bit;
>
> No need to shift:
>
>     return !!value == ext_cond;
>
OK.

> > +
> > +       if (value == ext_cond)
> > +               return true;
> > +
> > +       return false;
> > +}
> > +
> >  static int rzv2h_mod_clock_is_enabled(struct clk_hw *hw)
> >  {
> >         struct mod_clock *clock = to_mod_clock(hw);
> > @@ -691,6 +710,11 @@ rzv2h_cpg_register_mod_clk(const struct rzv2h_mod_clk *mod,
> >         clock->on_index = mod->on_index;
> >         clock->on_bit = mod->on_bit;
> >         clock->mon_index = mod->mon_index;
> > +       /* If clock is coming from external source ignore the monitor bit for it */
> > +       if (rzv2h_mod_clock_is_external(priv, mod->external_clk_offset,
> > +                                       mod->external_clk_bit,
> > +                                       mod->external_cond))
>
> Perhaps just pass "mod" instead of three of its members, to fully
> hide the logic inside the helper function?
>
Agreed.

Cheers,
Prabhakar
Lad, Prabhakar April 15, 2025, 7:12 p.m. UTC | #4 
Hi Geert,

On Tue, Apr 15, 2025 at 4:01 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Mon, 7 Apr 2025 at 18:52, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Ignore CLK_MON bits when turning on/off module clocks that use an external
> > clock source.
> >
> > Introduce the `DEF_MOD_EXTERNAL()` macro for defining module clocks that
> > may have an external clock source. Update `rzv2h_cpg_register_mod_clk()`
> > to update mon_index.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> > --- a/drivers/clk/renesas/rzv2h-cpg.c
> > +++ b/drivers/clk/renesas/rzv2h-cpg.c
> > @@ -569,6 +569,25 @@ static void rzv2h_mod_clock_mstop_disable(struct rzv2h_cpg_priv *priv,
> >         spin_unlock_irqrestore(&priv->rmw_lock, flags);
> >  }
> >
> > +static bool rzv2h_mod_clock_is_external(struct rzv2h_cpg_priv *priv,
> > +                                       u16 ext_clk_offset,
> > +                                       u8 ext_clk_bit,
> > +                                       u8 ext_cond)
> > +{
> > +       u32 value;
> > +
> > +       if (!ext_clk_offset)
> > +               return false;
> > +
> > +       value = readl(priv->base + ext_clk_offset) & BIT(ext_clk_bit);
>
> As ext_clk_offset is actually the offset of the Static Mux Control
> Registers (CPG_SSELm), this reads the current state of the mux.
> However, can't the state be changed at runtime (despite it being named
> a "static mux")?
>
Agreed based on the HW manual this can be changed at runtime. So this
check needs to be done in the rzv2h_mod_clock_is_enabled().

Cheers,
Prabhakar
diff mbox series

Patch

diff --git a/drivers/clk/renesas/rzv2h-cpg.c b/drivers/clk/renesas/rzv2h-cpg.c
index c75ed6ed087b..dca0940b3df9 100644
--- a/drivers/clk/renesas/rzv2h-cpg.c
+++ b/drivers/clk/renesas/rzv2h-cpg.c
@@ -569,6 +569,25 @@  static void rzv2h_mod_clock_mstop_disable(struct rzv2h_cpg_priv *priv,
 	spin_unlock_irqrestore(&priv->rmw_lock, flags);
 }
 
+static bool rzv2h_mod_clock_is_external(struct rzv2h_cpg_priv *priv,
+					u16 ext_clk_offset,
+					u8 ext_clk_bit,
+					u8 ext_cond)
+{
+	u32 value;
+
+	if (!ext_clk_offset)
+		return false;
+
+	value = readl(priv->base + ext_clk_offset) & BIT(ext_clk_bit);
+	value >>= ext_clk_bit;
+
+	if (value == ext_cond)
+		return true;
+
+	return false;
+}
+
 static int rzv2h_mod_clock_is_enabled(struct clk_hw *hw)
 {
 	struct mod_clock *clock = to_mod_clock(hw);
@@ -691,6 +710,11 @@  rzv2h_cpg_register_mod_clk(const struct rzv2h_mod_clk *mod,
 	clock->on_index = mod->on_index;
 	clock->on_bit = mod->on_bit;
 	clock->mon_index = mod->mon_index;
+	/* If clock is coming from external source ignore the monitor bit for it */
+	if (rzv2h_mod_clock_is_external(priv, mod->external_clk_offset,
+					mod->external_clk_bit,
+					mod->external_cond))
+		clock->mon_index = -1;
 	clock->mon_bit = mod->mon_bit;
 	clock->no_pm = mod->no_pm;
 	clock->priv = priv;
diff --git a/drivers/clk/renesas/rzv2h-cpg.h b/drivers/clk/renesas/rzv2h-cpg.h
index 97054f207113..c64cfead6dc1 100644
--- a/drivers/clk/renesas/rzv2h-cpg.h
+++ b/drivers/clk/renesas/rzv2h-cpg.h
@@ -192,6 +192,10 @@  enum clk_types {
  * @on_bit: ON bit
  * @mon_index: monitor register index
  * @mon_bit: monitor bit
+ * @external_clk_offset: Offset to check to determine if the clock is external
+ * @external_clk_bit: Bit to check to determine if the clock is external
+ * @external_cond: Condition to determine whether a given clock source is external;
+ *                 it can be either 0 or 1.
  */
 struct rzv2h_mod_clk {
 	const char *name;
@@ -203,9 +207,14 @@  struct rzv2h_mod_clk {
 	u8 on_bit;
 	s8 mon_index;
 	u8 mon_bit;
+	u16 external_clk_offset:10;
+	u8 external_clk_bit:5;
+	u8 external_cond:1;
 };
 
-#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, _external_clk_offset, \
+		     _external_clk_bit, _external_cond) \
 	{ \
 		.name = (_name), \
 		.mstop_data = (_mstop), \
@@ -216,16 +225,27 @@  struct rzv2h_mod_clk {
 		.on_bit = (_onbit), \
 		.mon_index = (_monindex), \
 		.mon_bit = (_monbit), \
+		.external_clk_offset = (_external_clk_offset), \
+		.external_clk_bit = (_external_clk_bit), \
+		.external_cond = (_external_cond), \
 	}
 
 #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, \
+		     0, 0, 0)
 
 #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, \
+		     0, 0, 0)
 
 #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, \
+		     0, 0, 0)
+
+#define DEF_MOD_EXTERNAL(_name, _parent, _onindex, _onbit, _monindex, _monbit, _mstop, \
+			_external_clk_offset, _external_clk_bit, _external_cond) \
+	DEF_MOD_BASE(_name, _mstop, _parent, false, false, _onindex, _onbit, _monindex, _monbit, \
+		     _external_clk_offset, _external_clk_bit, _external_cond)
 
 /**
  * struct rzv2h_reset - Reset definitions