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@xxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxx>
> > > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
>
> > > --- 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
