Hi Geert, Prabhakar,
> -----Original Message-----
> From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> Sent: 19 June 2025 09:17
> Subject: Re: [PATCH v3 1/9] dt-bindings: memory: Document RZ/G3E support
>
> Hi Prabhakar,
>
> On Wed, 18 Jun 2025 at 17:24, Lad, Prabhakar <prabhakar.csengg@xxxxxxxxx> wrote:
> > On Wed, Jun 18, 2025 at 3:03 PM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
> > > On Wed, 18 Jun 2025 at 15:41, Lad, Prabhakar <prabhakar.csengg@xxxxxxxxx> wrote:
> > > > On Wed, Jun 18, 2025 at 1:59 PM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
> > > > > On Wed, 18 Jun 2025 at 14:06, Lad, Prabhakar <prabhakar.csengg@xxxxxxxxx> wrote:
> > > > > > On Wed, Jun 18, 2025 at 8:03 AM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
> > > > > > > On Tue, 17 Jun 2025 at 23:05, Lad, Prabhakar <prabhakar.csengg@xxxxxxxxx> wrote:
> > > > > > > > On Mon, Mar 31, 2025 at 7:25 PM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
> > > > > > > > > On Mon, 31 Mar 2025 at 17:33, Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote:
> > > > > > > > > > > From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> On
> > > > > > > > > > > Mon, 31 Mar 2025 at 16:34, Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote:
> > > > > > > > > > > > > From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> > > > > > > > > > > > > On Mon, 31 Mar 2025 at 15:54, Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote:
> > > > > > > > > > > > > > > From: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > > > > > > > > > > > > > > Document support for the Expanded Serial
> > > > > > > > > > > > > > > Peripheral Interface (xSPI) Controller in
> > > > > > > > > > > > > > > the Renesas RZ/G3E
> > > > > > > > > > > > > > > (R9A09G047) SoC.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Reviewed-by: Rob Herring (Arm)
> > > > > > > > > > > > > > > <robh@xxxxxxxxxx>
> > > > > > > > > > > > > > > Signed-off-by: Biju Das
> > > > > > > > > > > > > > > <biju.das.jz@xxxxxxxxxxxxxx>
> > > > > > > > > > > > >
> > > > > > > > > > > > > > > --- /dev/null
> > > > > > > > > > > > > > > +++ b/Documentation/devicetree/bindings/memo
> > > > > > > > > > > > > > > +++ ry-controllers/renes
> > > > > > > > > > > > > > > +++ as,r
> > > > > > > > > > > > > > > +++ zg3e
> > > > > > > > > > > > > > > +++ -xspi.yaml
> > > > > > > > > > > > >
> > > > > > > > > > > > > > > + spi@11030000 {
> > > > > > > > > > > > > > > + compatible = "renesas,r9a09g047-xspi";
> > > > > > > > > > > > > > > + reg = <0x11030000 0x10000>, <0x20000000 0x10000000>;
> > > > > > > > > > > > > > > + reg-names = "regs", "dirmap";
> > > > > > > > > > > > > > > + interrupts = <GIC_SPI 228 IRQ_TYPE_EDGE_RISING>,
> > > > > > > > > > > > > > > + <GIC_SPI 229 IRQ_TYPE_EDGE_RISING>;
> > > > > > > > > > > > > > > + interrupt-names = "pulse", "err_pulse";
> > > > > > > > > > > > > > > + clocks = <&cpg CPG_MOD 0x9f>, <&cpg CPG_MOD 0xa0>,
> > > > > > > > > > > > > > > + <&cpg CPG_MOD 0xa1>, <&cpg
> > > > > > > > > > > > > > > + CPG_MOD 0xa1>;
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > On the next version I am going to update spix2
> > > > > > > > > > > > > > clk as <&cpg CPG_CORE R9A09G047_SPI_CLK_SPIX2>
> > > > > > > > >
> > > > > > > > > According to the RZ/G3E clock system diagram, (the
> > > > > > > > > parent of) clk_spi is derived from (the parent of) clk_spix2, not the other way
> around?
> > > > > > > > > So you can model clk_spi as a fixed divider clock with
> > > > > > > > > parent clk_spix2
> > > > > > >
> > > > > > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > > > [A]
> > > > >
> > > > > > > > > and factor two. I.e. provide a new core clock
> > > > > > > > > R9A09G047_SPI_CLK_SPI instead of your proposed R9A09G047_SPI_CLK_SPIX2?
> > > > > > > > >
> > > > > > > > With this approach when R9A09G047_SPI_CLK_SPI is used as a
> > > > > > > > core clock and XSPI node is disabled the clk_summary
> > > > > > > > reports the core clock is ON (while it's actually OFF).
> > > > > > >
> > > > > > > Is that a real problem, or is it purely cosmetic?
> > > > > > Just cosmetic tbh as despite being a MOD clock we have to
> > > > > > define it as a core clock in the DT.
>
> > > > > > > If spi_clk_spi really must show being disabled, you can
> > > > > > > change it from a fixed divider clock (which does not
> > > > > > > implement .{en,dis}able()) to a custom fixed divider clock
> > > > > > > that does implement .{en,dis}able() and keeps track
> > > > > > > internally of the fake state, or even looks at the state of spi_clk_spix2?
> > > > > > >
> > > > > > Good point. Maybe instead of implementing the dummy
> > > > > > .{en,dis}able() I will implement the is_enabled() +
> > > > > > (clk_fixed_factor_ops). The
> > > > > > is_enabled() will take care of reading from the MON bits and
> > > > > > report the actual state of the clock.
> > > > > >
> > > > > > > However, upon second look, spi_clk_spi is not implemented as
> > > > > > > a fixed divider clock with parent clk_spix2, as described above:
> > > > > > >
> > > > > > > .smux2_xspi_clk1 0 0 0 320000000 0 0 50000 Y
> > > > > > > .pllcm33_xspi 0 0 0 40000000 0 0 50000 Y
> > > > > > > spi_clk_spix2 0 0 0 40000000 0 0 50000 N
> > > > > > > spi_clk_spi 0 0 0 20000000 0 0 50000 Y
> > > > > > > spi_aclk 0 0 0 200000000 0 0 50000 N
> > > > > > > spi_hclk 0 0 0 200000000 0 0 50000 N
> > > > > > > .smux2_xspi_clk0 0 0 0 533333333 0 0 50000 Y
> > > > > > >
> > > > > > > Instead, they both use pllcm33_xspi as the parent clock.
> > > > > > > Apparently I missed that in the review of RZ/G3E XSPI clock support.
> > > > > > > The changelog for that patch does describe the correct topology?
> > > > > > >
> > > > > > The topology is correct for RZ/G3E, spi/spix2 are sourced from
> > > > > > pllcm33_xspi divider and there is a divider (/2) for spi.
> > > > >
> > > > > Both spi_clk_spix2 and spi_clk_spix have .pllcm33_xspi as
> > > > > immediate parent.
> > > > >
> > > > > [A] describes something different:
> > > > >
> > > > > .pllcm33_xspi 0 0 0 40000000 0 0 50000 Y
> > > > > spi_clk_spix2 0 0 0 40000000 0 0 50000 N
> > > > > spi_clk_spi 0 0 0 20000000 0 0 50000 Y
> > > > >
> > > > > I.e. if spi_clk_spix2() is disabled, spi_clk_spi() is disabled, too.
> > > > >
> > > > Okay, thanks - got it.
> > > >
> > > > To clarify, to implement spi_clk_spi core clock as a parent of
> > > > spi_clk_spix2 I will need to implement some sort of mechanism
> > > > which registers (late) core clks after core clks and module clks
> > > > are registered as spi_clk_spix2 is a module clock.
> > >
> > > Yes, I wondered about that as well, but wasn't too worried as you
> > > already added the smux with e.g. "et0_rxclk" as parent, which also
> > > doesn't exist at registration time ;-)
> > >
> > Good point.
> >
> > > But indeed, the smux uses clock names to find the parents, while
> > > fixed-factor clocks in zv2h_cpg_register_core_clk() expect clock IDs
> > > (which are converted to names), and don't handle non-core clocks yet.
> > > So either add support for late core clocks, or modify CLK_TYPE_FF to
> > > use cpg_core_clock.parent_names[] in case of a non-core parent?
> > >
> > I choose the late core registration of the clocks and with this the
> > core clk_spi still reports `Y` in the summary while the parent is OFF
> > (since its a FF clock).
>
> That leads to an interesting philosophical question: if a clock does not have an .is_enabled()
> callback, or cannot be disabled, should its enabled state match the enabled state of its parent?
> However, the same question can be asked for a clock that does have an
> .is_enabled() callback, and is currently enabled. What if its parent is disabled?
>
> > Code implementation for option#1
> > ------------------------------------------------
>
>
> > --- a/drivers/clk/renesas/rzv2h-cpg.c
> > +++ b/drivers/clk/renesas/rzv2h-cpg.c
>
> > @@ -727,8 +727,12 @@ rzv2h_cpg_register_core_clk(const struct
> > cpg_core_clk *core,
> > break;
> > case CLK_TYPE_FF:
> > WARN_DEBUG(core->parent >= priv->num_core_clks);
> > - parent = priv->clks[core->parent];
> > + if (late)
> > + parent = priv->clks[priv->num_core_clks + core->parent];
> > + else
> > + parent = priv->clks[core->parent];
>
> I'd rather keep the meaning of the parent ID the same in both cases, to avoid confusion.
>
> Perhaps keep the (modified) WARN_DEBUG():
>
> WARN_DEBUG(core->parent >=
> priv->num_core_clks + (late ? priv->num_mod_clks : 0));
>
> > if (IS_ERR(parent)) {
> > + pr_err("parent clk is NULL for %s parent:%d\n",
> > core->name, core->parent);
> > clk = parent;
> > goto fail;
> > }
>
> Thanks , this the simplest option of the two, but still shows the clock as enabled when it is not.
>
> > # Option#2
> > As mentioned in the previous thread I implemented FF clock with
> > is_enabled() with this I can see the status of core clk_spi reports
> > correct status.
>
> > Code implementation for option#2
> > ------------------------------------------------
>
> > --- a/drivers/clk/renesas/rzv2h-cpg.c
> > +++ b/drivers/clk/renesas/rzv2h-cpg.c
> > @@ -179,6 +179,28 @@ struct rzv2h_plldsi_div_clk { #define
> > to_plldsi_div_clk(_hw) \
> > container_of(_hw, struct rzv2h_plldsi_div_clk, hw)
> >
> > +/**
> > + * struct rzv2h_ff_mod_status_clk - Fixed Factor Module Status Clock
> > + *
> > + * @priv: CPG private data
> > + * @conf: fixed mod configuration
> > + * @hw: Fixed Factor Status Clock handle
> > + * @mult: multiplier value
> > + * @div: divider value
> > + * @flags: flags for the clock
> > + */
> > + struct rzv2h_ff_mod_status_clk {
> > + struct rzv2h_cpg_priv *priv;
> > + struct fixed_mod_conf conf;
> > + struct clk_hw hw;
> > + unsigned int mult;
> > + unsigned int div;
> > + unsigned int flags;
>
> You can replace the last four by embedding struct clk_fixed_factor (at the top of the struct!).
>
> > +};
> > +
> > +#define to_rzv2h_ff_mod_status_clk(_hw) \
> > + container_of(_hw, struct rzv2h_ff_mod_status_clk, hw)
> > +
> > static int rzv2h_cpg_pll_clk_is_enabled(struct clk_hw *hw) {
> > struct pll_clk *pll_clk = to_pll(hw); @@ -664,6 +686,114 @@
> > rzv2h_cpg_mux_clk_register(const struct cpg_core_clk *core,
> > return clk_hw->clk;
> > }
> >
> > +static unsigned long
> > +rzv2h_clk_ff_mod_status_recalc_rate(struct clk_hw *hw,
> > + unsigned long parent_rate)
>
> [...]
>
> > +static const struct clk_ops rzv2h_clk_ff_mod_status_ops = {
> > + .round_rate = rzv2h_clk_ff_mod_status_round_rate,
> > + .set_rate = rzv2h_clk_ff_mod_status_set_rate,
> > + .recalc_rate = rzv2h_clk_ff_mod_status_recalc_rate,
> > + .recalc_accuracy = rzv2h_clk_ff_mod_status_recalc_accuracy,
> > + .is_enabled = rzv2h_clk_ff_mod_status_is_enabled,
> > +};
>
> Lots of code copied from drivers/clk/clk-fixed-factor.c. Fortunately clk_fixed_factor_ops is public
> and exported, so you can just copy its callback pointers. instead of duplicating the code.
>
> > Please share your thoughts on this.
>
> Thanks , this is (slightly) more complex, and shows the correct clock state.
>
> Initially, I favored the first solution, due to its simplicity. But that was before I realized the
> second option could avoid duplicating code by reusing the callbacks from clk_fixed_factor_ops...
> What do other people think?
+1 for second solution.
Cheers,
Biju
