Re: [PATCH v3 1/9] dt-bindings: memory: Document RZ/G3E support

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Prabhakar,

On Wed, 18 Jun 2025 at 15:41, Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> On Wed, Jun 18, 2025 at 1:59 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Wed, 18 Jun 2025 at 14:06, Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > > On Wed, Jun 18, 2025 at 8:03 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > On Tue, 17 Jun 2025 at 23:05, Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > > > > On Mon, Mar 31, 2025 at 7:25 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > > On Mon, 31 Mar 2025 at 17:33, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > > > > > > > On Mon, 31 Mar 2025 at 16:34, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > > > > From: Geert Uytterhoeven <geert@linux-m68k.org> On Mon, 31 Mar 2025
> > > > > > > > > > at 15:54, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > > > > > > From: Biju Das <biju.das.jz@bp.renesas.com> Document support for
> > > > > > > > > > > > the Expanded Serial Peripheral Interface (xSPI) Controller in
> > > > > > > > > > > > the Renesas RZ/G3E
> > > > > > > > > > > > (R9A09G047) SoC.
> > > > > > > > > > > >
> > > > > > > > > > > > Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> > > > > > > > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > > > > > >
> > > > > > > > > > > > --- /dev/null
> > > > > > > > > > > > +++ b/Documentation/devicetree/bindings/memory-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.
> > >
> > > > > Can we maybe use a unused ON index and ON bit for example 25, 0 (ie
> > > > > 0x190) and represent this is a module clock for example for the
> > > > > spi_clk_spix2 clock and use this in the DT and let the CPG core code
> > > > > handle such turning ON/OF the module clocks based on the enable count
> > > > > which will be handled internally in the driver?
> > > >
> > > > Please do not use "unused" module clock bits.  These do not describe
> > > > the hardware, and may actually exist in the hardware (try disabling
> > > > all undocumented module clocks, and observe what fails...).
> > > >
> > > Agreed, "unused" module clock bits were only used as a dummy. The
> > > read/write operations were only performed on the actual bits which are
> > > documented in the HW manual.
> > >
> > > > 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 ;-)

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?

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