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.
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
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
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
