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@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.
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
