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