diff mbox series

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

Message ID 20210815103014.21208-4-biju.das.jz@bp.renesas.com (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series Add GbEthernet Clock support | expand

Commit Message

Biju Das Aug. 15, 2021, 10:30 a.m. UTC 
The AXI and CHI clocks use the same register bit for controlling clock
output. Add a new clock type for coupled clocks, which sets the
CPG_CLKON_ETH.CLK[01]_ON bit when at least one clock is enabled, and
clears the bit only when both clocks are disabled.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
v2->v3:
 * Reworked as per Geert's suggestion
 * Added enabled flag to track the status of clock, if it is coupled
   with another clock
 * Introduced siblings pointer which points to the other coupled
   clock
 * coupled clock linking is done during module clk register.
 * rzg2l_mod_clock_is_enabled function returns soft state of the
   module clocks, if it is coupled with another clock
 * Updated the commit header
v2:-
 * New patch
---
 drivers/clk/renesas/rzg2l-cpg.c | 62 +++++++++++++++++++++++++++++++++
 drivers/clk/renesas/rzg2l-cpg.h | 11 +++++-
 2 files changed, 72 insertions(+), 1 deletion(-)

Comments

Biju Das Aug. 16, 2021, 9:23 a.m. UTC | #1 
Hi All,


> Subject: [PATCH v3 3/4] clk: renesas: rzg2l: Add support to handle coupled
> clocks
> 
> The AXI and CHI clocks use the same register bit for controlling clock
> output. Add a new clock type for coupled clocks, which sets the
> CPG_CLKON_ETH.CLK[01]_ON bit when at least one clock is enabled, and
> clears the bit only when both clocks are disabled.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> v2->v3:
>  * Reworked as per Geert's suggestion
>  * Added enabled flag to track the status of clock, if it is coupled
>    with another clock
>  * Introduced siblings pointer which points to the other coupled
>    clock
>  * coupled clock linking is done during module clk register.
>  * rzg2l_mod_clock_is_enabled function returns soft state of the
>    module clocks, if it is coupled with another clock
>  * Updated the commit header
> v2:-
>  * New patch
> ---
>  drivers/clk/renesas/rzg2l-cpg.c | 62 +++++++++++++++++++++++++++++++++
> drivers/clk/renesas/rzg2l-cpg.h | 11 +++++-
>  2 files changed, 72 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/renesas/rzg2l-cpg.c b/drivers/clk/renesas/rzg2l-
> cpg.c index 597efc2504eb..ebcb57287bf6 100644
> --- a/drivers/clk/renesas/rzg2l-cpg.c
> +++ b/drivers/clk/renesas/rzg2l-cpg.c
> @@ -333,13 +333,17 @@ rzg2l_cpg_register_core_clk(const struct
> cpg_core_clk *core,
>   * @hw: handle between common and hardware-specific interfaces
>   * @off: register offset
>   * @bit: ON/MON bit
> + * @enabled: soft state of the clock, if it is coupled with another
> + clock
>   * @priv: CPG/MSTP private data
> + * @siblings: pointer to the other coupled clock
>   */
>  struct mstp_clock {
>  	struct clk_hw hw;
>  	u16 off;
>  	u8 bit;
> +	bool enabled;
>  	struct rzg2l_cpg_priv *priv;
> +	struct mstp_clock *siblings;
>  };
> 
>  #define to_mod_clock(_hw) container_of(_hw, struct mstp_clock, hw) @@ -
> 392,11 +396,35 @@ static int rzg2l_mod_clock_endisable(struct clk_hw *hw,
> bool enable)
> 
>  static int rzg2l_mod_clock_enable(struct clk_hw *hw)  {
> +	struct mstp_clock *clock = to_mod_clock(hw);
> +	struct mstp_clock *siblings = clock->siblings;
> +
> +	if (siblings) {
> +		if (siblings->enabled) {
> +			clock->enabled = true;
> +			return 0;
> +		}
> +
> +		clock->enabled = true;
> +	}
> +
>  	return rzg2l_mod_clock_endisable(hw, true);  }
> 
>  static void rzg2l_mod_clock_disable(struct clk_hw *hw)  {
> +	struct mstp_clock *clock = to_mod_clock(hw);
> +	struct mstp_clock *siblings = clock->siblings;
> +
> +	if (siblings) {
> +		if (siblings->enabled) {
> +			clock->enabled = false;
> +			return;
> +		}
> +
> +		clock->enabled = false;
> +	}
> +
>  	rzg2l_mod_clock_endisable(hw, false);
>  }
> 
> @@ -412,6 +440,9 @@ static int rzg2l_mod_clock_is_enabled(struct clk_hw
> *hw)
>  		return 1;
>  	}
> 
> +	if (clock->siblings)
> +		return clock->enabled;
> +
>  	value = readl(priv->base + CLK_MON_R(clock->off));
> 
>  	return !(value & bitmask);
> @@ -423,11 +454,33 @@ static const struct clk_ops rzg2l_mod_clock_ops = {
>  	.is_enabled = rzg2l_mod_clock_is_enabled,  };
> 
> +static struct mstp_clock
> +*rzg2l_cpg_get_sibling_clk(struct mstp_clock *clock,
> +			   struct rzg2l_cpg_priv *priv)
> +{
> +	struct mstp_clock *sibl_clk = NULL;
> +	struct clk_hw *hw;
> +	unsigned int i;
> +
> +	for (i = 0; i < priv->num_mod_clks; i++) {
> +		if (priv->clks[priv->num_core_clks + i] == ERR_PTR(-ENOENT))
> +			continue;
> +
> +		hw = __clk_get_hw(priv->clks[priv->num_core_clks + i]);
> +		sibl_clk = to_mod_clock(hw);
> +		if (clock->off == sibl_clk->off && clock->bit == sibl_clk-
> >bit)
> +			break;

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

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

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

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

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


Cheers,
Biju

> +	}
> +
> +	return sibl_clk;
> +}
> +
>  static void __init
>  rzg2l_cpg_register_mod_clk(const struct rzg2l_mod_clk *mod,
>  			   const struct rzg2l_cpg_info *info,
>  			   struct rzg2l_cpg_priv *priv)
>  {
> +	struct mstp_clock *sibling_clock = NULL;
>  	struct mstp_clock *clock = NULL;
>  	struct device *dev = priv->dev;
>  	unsigned int id = mod->id;
> @@ -484,6 +537,15 @@ rzg2l_cpg_register_mod_clk(const struct rzg2l_mod_clk
> *mod,
> 
>  	dev_dbg(dev, "Module clock %pC at %lu Hz\n", clk,
> clk_get_rate(clk));
>  	priv->clks[id] = clk;
> +
> +	if (mod->is_coupled) {
> +		sibling_clock = rzg2l_cpg_get_sibling_clk(clock, priv);
> +		if (sibling_clock) {
> +			clock->siblings = sibling_clock;
> +			sibling_clock->siblings = clock;
> +		}
> +	}
> +
>  	return;
> 
>  fail:
> diff --git a/drivers/clk/renesas/rzg2l-cpg.h b/drivers/clk/renesas/rzg2l-
> cpg.h index 5202c0512483..191c403aa52f 100644
> --- a/drivers/clk/renesas/rzg2l-cpg.h
> +++ b/drivers/clk/renesas/rzg2l-cpg.h
> @@ -93,6 +93,7 @@ enum clk_types {
>   * @parent: id of parent clock
>   * @off: register offset
>   * @bit: ON/MON bit
> + * @is_coupled: flag to indicate coupled clock
>   */
>  struct rzg2l_mod_clk {
>  	const char *name;
> @@ -100,17 +101,25 @@ struct rzg2l_mod_clk {
>  	unsigned int parent;
>  	u16 off;
>  	u8 bit;
> +	bool is_coupled;
>  };
> 
> -#define DEF_MOD(_name, _id, _parent, _off, _bit)	\
> +#define DEF_MOD_BASE(_name, _id, _parent, _off, _bit, _is_coupled)	\
>  	{ \
>  		.name = _name, \
>  		.id = MOD_CLK_BASE + (_id), \
>  		.parent = (_parent), \
>  		.off = (_off), \
>  		.bit = (_bit), \
> +		.is_coupled = (_is_coupled), \
>  	}
> 
> +#define DEF_MOD(_name, _id, _parent, _off, _bit)	\
> +	DEF_MOD_BASE(_name, _id, _parent, _off, _bit, false)
> +
> +#define DEF_COUPLED(_name, _id, _parent, _off, _bit)	\
> +	DEF_MOD_BASE(_name, _id, _parent, _off, _bit, true)
> +
>  /**
>   * struct rzg2l_reset - Reset definitions
>   *
> --
> 2.17.1
Geert Uytterhoeven Aug. 23, 2021, 10 a.m. UTC | #2 
Hi Biju,

On Sun, Aug 15, 2021 at 12:30 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> The AXI and CHI clocks use the same register bit for controlling clock
> output. Add a new clock type for coupled clocks, which sets the
> CPG_CLKON_ETH.CLK[01]_ON bit when at least one clock is enabled, and
> clears the bit only when both clocks are disabled.
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> v2->v3:
>  * Reworked as per Geert's suggestion
>  * Added enabled flag to track the status of clock, if it is coupled
>    with another clock
>  * Introduced siblings pointer which points to the other coupled
>    clock
>  * coupled clock linking is done during module clk register.
>  * rzg2l_mod_clock_is_enabled function returns soft state of the
>    module clocks, if it is coupled with another clock
>  * Updated the commit header

Thanks for the update!

> --- a/drivers/clk/renesas/rzg2l-cpg.c
> +++ b/drivers/clk/renesas/rzg2l-cpg.c
> @@ -392,11 +396,35 @@ static int rzg2l_mod_clock_endisable(struct clk_hw *hw, bool enable)
>
>  static int rzg2l_mod_clock_enable(struct clk_hw *hw)
>  {
> +       struct mstp_clock *clock = to_mod_clock(hw);
> +       struct mstp_clock *siblings = clock->siblings;
> +
> +       if (siblings) {
> +               if (siblings->enabled) {
> +                       clock->enabled = true;
> +                       return 0;
> +               }
> +
> +               clock->enabled = true;

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

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

> +       }
> +
>         return rzg2l_mod_clock_endisable(hw, true);
>  }
>
>  static void rzg2l_mod_clock_disable(struct clk_hw *hw)
>  {
> +       struct mstp_clock *clock = to_mod_clock(hw);
> +       struct mstp_clock *siblings = clock->siblings;
> +
> +       if (siblings) {
> +               if (siblings->enabled) {
> +                       clock->enabled = false;
> +                       return;
> +               }
> +
> +               clock->enabled = false;

Likewise.

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

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

> +       struct clk_hw *hw;
> +       unsigned int i;
> +
> +       for (i = 0; i < priv->num_mod_clks; i++) {
> +               if (priv->clks[priv->num_core_clks + i] == ERR_PTR(-ENOENT))
> +                       continue;
> +
> +               hw = __clk_get_hw(priv->clks[priv->num_core_clks + i]);
> +               sibl_clk = to_mod_clock(hw);
> +               if (clock->off == sibl_clk->off && clock->bit == sibl_clk->bit)
> +                       break;
> +       }
> +
> +       return sibl_clk;
> +}
> +
>  static void __init
>  rzg2l_cpg_register_mod_clk(const struct rzg2l_mod_clk *mod,
>                            const struct rzg2l_cpg_info *info,
>                            struct rzg2l_cpg_priv *priv)
>  {
> +       struct mstp_clock *sibling_clock = NULL;
>         struct mstp_clock *clock = NULL;
>         struct device *dev = priv->dev;
>         unsigned int id = mod->id;
> @@ -484,6 +537,15 @@ rzg2l_cpg_register_mod_clk(const struct rzg2l_mod_clk *mod,
>
>         dev_dbg(dev, "Module clock %pC at %lu Hz\n", clk, clk_get_rate(clk));
>         priv->clks[id] = clk;
> +
> +       if (mod->is_coupled) {
> +               sibling_clock = rzg2l_cpg_get_sibling_clk(clock, priv);
> +               if (sibling_clock) {
> +                       clock->siblings = sibling_clock;
> +                       sibling_clock->siblings = clock;
> +               }

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

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

Gr{oetje,eeting}s,

                        Geert
Geert Uytterhoeven Aug. 23, 2021, 10:08 a.m. UTC | #3 
Hi Biju,

On Mon, Aug 16, 2021 at 11:23 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Subject: [PATCH v3 3/4] clk: renesas: rzg2l: Add support to handle coupled
> > clocks
> >
> > The AXI and CHI clocks use the same register bit for controlling clock
> > output. Add a new clock type for coupled clocks, which sets the
> > CPG_CLKON_ETH.CLK[01]_ON bit when at least one clock is enabled, and
> > clears the bit only when both clocks are disabled.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

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

Indeed.

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

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

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

Gr{oetje,eeting}s,

                        Geert
Biju Das Aug. 23, 2021, 11:18 a.m. UTC | #4 
Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH v3 3/4] clk: renesas: rzg2l: Add support to handle
> coupled clocks
> 
> Hi Biju,
> 
> On Sun, Aug 15, 2021 at 12:30 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > The AXI and CHI clocks use the same register bit for controlling clock
> > output. Add a new clock type for coupled clocks, which sets the
> > CPG_CLKON_ETH.CLK[01]_ON bit when at least one clock is enabled, and
> > clears the bit only when both clocks are disabled.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> > v2->v3:
> >  * Reworked as per Geert's suggestion
> >  * Added enabled flag to track the status of clock, if it is coupled
> >    with another clock
> >  * Introduced siblings pointer which points to the other coupled
> >    clock
> >  * coupled clock linking is done during module clk register.
> >  * rzg2l_mod_clock_is_enabled function returns soft state of the
> >    module clocks, if it is coupled with another clock
> >  * Updated the commit header
> 
> Thanks for the update!
> 
> > --- a/drivers/clk/renesas/rzg2l-cpg.c
> > +++ b/drivers/clk/renesas/rzg2l-cpg.c
> > @@ -392,11 +396,35 @@ static int rzg2l_mod_clock_endisable(struct
> > clk_hw *hw, bool enable)
> >
> >  static int rzg2l_mod_clock_enable(struct clk_hw *hw)  {
> > +       struct mstp_clock *clock = to_mod_clock(hw);
> > +       struct mstp_clock *siblings = clock->siblings;
> > +
> > +       if (siblings) {
> > +               if (siblings->enabled) {
> > +                       clock->enabled = true;
> > +                       return 0;
> > +               }
> > +
> > +               clock->enabled = true;
> 
> clock->enabled is set to true regardless of the if condition.
> This also needs locking, in case both clocks are changed concurrently:

Agreed. It needs locking.

> 
>     spin_lock_irqsave(&priv->rmw_lock, flags);
>     enabled = siblings->enabled;
>     clock->enabled = true;
>     spin_unlock_irqrestore(&priv->rmw_lock, flags);
>     if (enabled)
>             return 0;
> 
> > +       }
> > +
> >         return rzg2l_mod_clock_endisable(hw, true);  }
> >
> >  static void rzg2l_mod_clock_disable(struct clk_hw *hw)  {
> > +       struct mstp_clock *clock = to_mod_clock(hw);
> > +       struct mstp_clock *siblings = clock->siblings;
> > +
> > +       if (siblings) {
> > +               if (siblings->enabled) {
> > +                       clock->enabled = false;
> > +                       return;
> > +               }
> > +
> > +               clock->enabled = false;
> 
> Likewise.
OK.

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

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

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

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

Cheers,
Biju

> 
> > +       }
> > +
> >         return;
> >
> >  fail:
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-
> m68k.org
> 
> In personal conversations with technical people, I call myself a hacker.
> But when I'm talking to journalists I just say "programmer" or something
> like that.
>                                 -- Linus Torvalds
Biju Das Aug. 23, 2021, 11:20 a.m. UTC | #5 
Hi Geert,

Thanks for the feedback.

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

OK.

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

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

Regards,
Biju
Biju Das Aug. 30, 2021, 8:36 a.m. UTC | #6 
Hi Geert,

> Subject: RE: [PATCH v3 3/4] clk: renesas: rzg2l: Add support to handle
> coupled clocks
> 
> Hi Geert,
> 
> Thanks for the feedback.
> 
> > Subject: Re: [PATCH v3 3/4] clk: renesas: rzg2l: Add support to handle
> > coupled clocks
> >
> > Hi Biju,
> >
> > On Sun, Aug 15, 2021 at 12:30 PM Biju Das <biju.das.jz@bp.renesas.com>
> > wrote:
> > > The AXI and CHI clocks use the same register bit for controlling
> > > clock output. Add a new clock type for coupled clocks, which sets
> > > the CPG_CLKON_ETH.CLK[01]_ON bit when at least one clock is enabled,
> > > and clears the bit only when both clocks are disabled.
> > >
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > ---
> > > v2->v3:
> > >  * Reworked as per Geert's suggestion
> > >  * Added enabled flag to track the status of clock, if it is coupled
> > >    with another clock
> > >  * Introduced siblings pointer which points to the other coupled
> > >    clock
> > >  * coupled clock linking is done during module clk register.
> > >  * rzg2l_mod_clock_is_enabled function returns soft state of the
> > >    module clocks, if it is coupled with another clock
> > >  * Updated the commit header
> >
> > Thanks for the update!
> >
> > > --- a/drivers/clk/renesas/rzg2l-cpg.c
> > > +++ b/drivers/clk/renesas/rzg2l-cpg.c
> > > @@ -392,11 +396,35 @@ static int rzg2l_mod_clock_endisable(struct
> > > clk_hw *hw, bool enable)
> > >
> > >  static int rzg2l_mod_clock_enable(struct clk_hw *hw)  {
> > > +       struct mstp_clock *clock = to_mod_clock(hw);
> > > +       struct mstp_clock *siblings = clock->siblings;
> > > +
> > > +       if (siblings) {
> > > +               if (siblings->enabled) {
> > > +                       clock->enabled = true;
> > > +                       return 0;
> > > +               }
> > > +
> > > +               clock->enabled = true;
> >
> > clock->enabled is set to true regardless of the if condition.
> > This also needs locking, in case both clocks are changed concurrently:
> 
> Agreed. It needs locking.
> 
> >
> >     spin_lock_irqsave(&priv->rmw_lock, flags);
> >     enabled = siblings->enabled;
> >     clock->enabled = true;
> >     spin_unlock_irqrestore(&priv->rmw_lock, flags);
> >     if (enabled)
> >             return 0;
> >
> > > +       }
> > > +
> > >         return rzg2l_mod_clock_endisable(hw, true);  }
> > >
> > >  static void rzg2l_mod_clock_disable(struct clk_hw *hw)  {
> > > +       struct mstp_clock *clock = to_mod_clock(hw);
> > > +       struct mstp_clock *siblings = clock->siblings;
> > > +
> > > +       if (siblings) {
> > > +               if (siblings->enabled) {
> > > +                       clock->enabled = false;
> > > +                       return;
> > > +               }
> > > +
> > > +               clock->enabled = false;
> >
> > Likewise.
> OK.
> 
> >
> > > +       }
> > > +
> > >         rzg2l_mod_clock_endisable(hw, false);  }
> > >
> > > @@ -412,6 +440,9 @@ static int rzg2l_mod_clock_is_enabled(struct
> > > clk_hw
> > *hw)
> > >                 return 1;
> > >         }
> > >
> > > +       if (clock->siblings)
> > > +               return clock->enabled;
> > > +
> > >         value = readl(priv->base + CLK_MON_R(clock->off));
> > >
> > >         return !(value & bitmask);
> > > @@ -423,11 +454,33 @@ static const struct clk_ops
> > > rzg2l_mod_clock_ops =
> > {
> > >         .is_enabled = rzg2l_mod_clock_is_enabled,  };
> > >
> > > +static struct mstp_clock
> > > +*rzg2l_cpg_get_sibling_clk(struct mstp_clock *clock,
> > > +                          struct rzg2l_cpg_priv *priv) {
> > > +       struct mstp_clock *sibl_clk = NULL;
> >
> > clk?
> > It's only a sibling when the condition inside the loop is true.
> 
> OK, will declare "clk" inside for loop to limit the scope.
> 
> >
> > > +       struct clk_hw *hw;
> > > +       unsigned int i;
> > > +
> > > +       for (i = 0; i < priv->num_mod_clks; i++) {
> > > +               if (priv->clks[priv->num_core_clks + i] == ERR_PTR(-
> > ENOENT))
> > > +                       continue;
> > > +
> > > +               hw = __clk_get_hw(priv->clks[priv->num_core_clks +
> i]);
> > > +               sibl_clk = to_mod_clock(hw);
> > > +               if (clock->off == sibl_clk->off && clock->bit ==
> > sibl_clk->bit)
> > > +                       break;
> > > +       }
> > > +
> > > +       return sibl_clk;
> > > +}
> > > +
> > >  static void __init
> > >  rzg2l_cpg_register_mod_clk(const struct rzg2l_mod_clk *mod,
> > >                            const struct rzg2l_cpg_info *info,
> > >                            struct rzg2l_cpg_priv *priv)  {
> > > +       struct mstp_clock *sibling_clock = NULL;
> > >         struct mstp_clock *clock = NULL;
> > >         struct device *dev = priv->dev;
> > >         unsigned int id = mod->id;
> > > @@ -484,6 +537,15 @@ rzg2l_cpg_register_mod_clk(const struct
> > > rzg2l_mod_clk *mod,
> > >
> > >         dev_dbg(dev, "Module clock %pC at %lu Hz\n", clk,
> > clk_get_rate(clk));
> > >         priv->clks[id] = clk;
> > > +
> > > +       if (mod->is_coupled) {
> > > +               sibling_clock = rzg2l_cpg_get_sibling_clk(clock,
> priv);
> > > +               if (sibling_clock) {
> > > +                       clock->siblings = sibling_clock;
> > > +                       sibling_clock->siblings = clock;
> > > +               }
> >
> > You forgot to initialize mstp_clock.enabled to match the current
> > hardware state.
> 
> OK. will initialize mstp_clock.enabled to match the current hardware
> state.

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

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

But the "rzg2l_mod_clock_is_enabled" function returns inverted value instead.

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

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

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

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

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

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

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

Please share your views on this investigation.

Regards
Biju
Geert Uytterhoeven Aug. 30, 2021, 1:21 p.m. UTC | #7 
Hi Biju,

On Mon, Aug 30, 2021 at 10:36 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Subject: RE: [PATCH v3 3/4] clk: renesas: rzg2l: Add support to handle
> > coupled clocks
> > > On Sun, Aug 15, 2021 at 12:30 PM Biju Das <biju.das.jz@bp.renesas.com>
> > > wrote:
> > > > The AXI and CHI clocks use the same register bit for controlling
> > > > clock output. Add a new clock type for coupled clocks, which sets
> > > > the CPG_CLKON_ETH.CLK[01]_ON bit when at least one clock is enabled,
> > > > and clears the bit only when both clocks are disabled.
> > > >
> > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > ---
> > > > v2->v3:
> > > >  * Reworked as per Geert's suggestion
> > > >  * Added enabled flag to track the status of clock, if it is coupled
> > > >    with another clock
> > > >  * Introduced siblings pointer which points to the other coupled
> > > >    clock
> > > >  * coupled clock linking is done during module clk register.
> > > >  * rzg2l_mod_clock_is_enabled function returns soft state of the
> > > >    module clocks, if it is coupled with another clock
> > > >  * Updated the commit header

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

Oops...

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

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

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

OK.

Gr{oetje,eeting}s,

                        Geert
diff mbox series

Patch

diff --git a/drivers/clk/renesas/rzg2l-cpg.c b/drivers/clk/renesas/rzg2l-cpg.c
index 597efc2504eb..ebcb57287bf6 100644
--- a/drivers/clk/renesas/rzg2l-cpg.c
+++ b/drivers/clk/renesas/rzg2l-cpg.c
@@ -333,13 +333,17 @@  rzg2l_cpg_register_core_clk(const struct cpg_core_clk *core,
  * @hw: handle between common and hardware-specific interfaces
  * @off: register offset
  * @bit: ON/MON bit
+ * @enabled: soft state of the clock, if it is coupled with another clock
  * @priv: CPG/MSTP private data
+ * @siblings: pointer to the other coupled clock
  */
 struct mstp_clock {
 	struct clk_hw hw;
 	u16 off;
 	u8 bit;
+	bool enabled;
 	struct rzg2l_cpg_priv *priv;
+	struct mstp_clock *siblings;
 };
 
 #define to_mod_clock(_hw) container_of(_hw, struct mstp_clock, hw)
@@ -392,11 +396,35 @@  static int rzg2l_mod_clock_endisable(struct clk_hw *hw, bool enable)
 
 static int rzg2l_mod_clock_enable(struct clk_hw *hw)
 {
+	struct mstp_clock *clock = to_mod_clock(hw);
+	struct mstp_clock *siblings = clock->siblings;
+
+	if (siblings) {
+		if (siblings->enabled) {
+			clock->enabled = true;
+			return 0;
+		}
+
+		clock->enabled = true;
+	}
+
 	return rzg2l_mod_clock_endisable(hw, true);
 }
 
 static void rzg2l_mod_clock_disable(struct clk_hw *hw)
 {
+	struct mstp_clock *clock = to_mod_clock(hw);
+	struct mstp_clock *siblings = clock->siblings;
+
+	if (siblings) {
+		if (siblings->enabled) {
+			clock->enabled = false;
+			return;
+		}
+
+		clock->enabled = false;
+	}
+
 	rzg2l_mod_clock_endisable(hw, false);
 }
 
@@ -412,6 +440,9 @@  static int rzg2l_mod_clock_is_enabled(struct clk_hw *hw)
 		return 1;
 	}
 
+	if (clock->siblings)
+		return clock->enabled;
+
 	value = readl(priv->base + CLK_MON_R(clock->off));
 
 	return !(value & bitmask);
@@ -423,11 +454,33 @@  static const struct clk_ops rzg2l_mod_clock_ops = {
 	.is_enabled = rzg2l_mod_clock_is_enabled,
 };
 
+static struct mstp_clock
+*rzg2l_cpg_get_sibling_clk(struct mstp_clock *clock,
+			   struct rzg2l_cpg_priv *priv)
+{
+	struct mstp_clock *sibl_clk = NULL;
+	struct clk_hw *hw;
+	unsigned int i;
+
+	for (i = 0; i < priv->num_mod_clks; i++) {
+		if (priv->clks[priv->num_core_clks + i] == ERR_PTR(-ENOENT))
+			continue;
+
+		hw = __clk_get_hw(priv->clks[priv->num_core_clks + i]);
+		sibl_clk = to_mod_clock(hw);
+		if (clock->off == sibl_clk->off && clock->bit == sibl_clk->bit)
+			break;
+	}
+
+	return sibl_clk;
+}
+
 static void __init
 rzg2l_cpg_register_mod_clk(const struct rzg2l_mod_clk *mod,
 			   const struct rzg2l_cpg_info *info,
 			   struct rzg2l_cpg_priv *priv)
 {
+	struct mstp_clock *sibling_clock = NULL;
 	struct mstp_clock *clock = NULL;
 	struct device *dev = priv->dev;
 	unsigned int id = mod->id;
@@ -484,6 +537,15 @@  rzg2l_cpg_register_mod_clk(const struct rzg2l_mod_clk *mod,
 
 	dev_dbg(dev, "Module clock %pC at %lu Hz\n", clk, clk_get_rate(clk));
 	priv->clks[id] = clk;
+
+	if (mod->is_coupled) {
+		sibling_clock = rzg2l_cpg_get_sibling_clk(clock, priv);
+		if (sibling_clock) {
+			clock->siblings = sibling_clock;
+			sibling_clock->siblings = clock;
+		}
+	}
+
 	return;
 
 fail:
diff --git a/drivers/clk/renesas/rzg2l-cpg.h b/drivers/clk/renesas/rzg2l-cpg.h
index 5202c0512483..191c403aa52f 100644
--- a/drivers/clk/renesas/rzg2l-cpg.h
+++ b/drivers/clk/renesas/rzg2l-cpg.h
@@ -93,6 +93,7 @@  enum clk_types {
  * @parent: id of parent clock
  * @off: register offset
  * @bit: ON/MON bit
+ * @is_coupled: flag to indicate coupled clock
  */
 struct rzg2l_mod_clk {
 	const char *name;
@@ -100,17 +101,25 @@  struct rzg2l_mod_clk {
 	unsigned int parent;
 	u16 off;
 	u8 bit;
+	bool is_coupled;
 };
 
-#define DEF_MOD(_name, _id, _parent, _off, _bit)	\
+#define DEF_MOD_BASE(_name, _id, _parent, _off, _bit, _is_coupled)	\
 	{ \
 		.name = _name, \
 		.id = MOD_CLK_BASE + (_id), \
 		.parent = (_parent), \
 		.off = (_off), \
 		.bit = (_bit), \
+		.is_coupled = (_is_coupled), \
 	}
 
+#define DEF_MOD(_name, _id, _parent, _off, _bit)	\
+	DEF_MOD_BASE(_name, _id, _parent, _off, _bit, false)
+
+#define DEF_COUPLED(_name, _id, _parent, _off, _bit)	\
+	DEF_MOD_BASE(_name, _id, _parent, _off, _bit, true)
+
 /**
  * struct rzg2l_reset - Reset definitions
  *