Hi Prabhakar,
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/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
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > 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?
> 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...).
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?
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?
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
