Re: [PATCH v2 13/13] ARM: dts: r9a06g032: Describe the QSPI controller

[Geert Uytterhoeven]

Hi Miquel,

On Fri, 16 Jan 2026 at 10:49, Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:
> >> +               qspi0: spi@40005000 {
> >> +                       compatible = "renesas,r9a06g032-qspi", "renesas,rzn1-qspi", "cdns,qspi-nor";
> >> +                       reg = <0x40005000 0x1000>, <0x10000000 0x10000000>;
> >> +                       interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
> >> +                       clocks = <&sysctrl R9A06G032_CLK_QSPI0>, <&sysctrl R9A06G032_HCLK_QSPI0>,
> >> +                                <&sysctrl R9A06G032_HCLK_QSPI0>;
> >> +                       clock-names = "ref", "ahb", "apb";
> >> +                       #address-cells = <1>;
> >> +                       #size-cells = <0>;
> >> +                       cdns,fifo-width = <4>;
> >
> > <4> is the default, right?
>
> It is the default in the bindings indeed, however the driver does not
> imply that default and errors out if the property is missing. The
> property is also marked required in the bindings, which is kind of
> incorrect I guess. Also, all DTS explicitly set this value to 4.

OK.

>
> However looking into the RM I found "Transmit and receive FIFOs are 16
> bytes". I haven't tested that, I will.

Oh, that bullet is not present in the docs on the CD I looked at.
It is indeed documented in newer versions.

There's also cdns,fifo-depth, which thus should be 4?

>
> >> +                       cdns,trigger-address = <0>;
> >
> > Where in the RZ/N1 docs can I find if these two properties are
> > correct?
>
> This property is mandatory. Maybe I could just discard it for my
> compatible, because it is only relevant for indirect modes, which are
> unsupported.

OK.

> >> +                       status = "disabled";
> >> +               };
> >> +
> >>                 rtc0: rtc@40006000 {
> >>                         compatible = "renesas,r9a06g032-rtc", "renesas,rzn1-rtc";
> >>                         reg = <0x40006000 0x1000>;
> >
> > The rest LGTM, ignoring my comments on the bindings:
> > Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
>
> Thanks for the review, but I guess if I end up changing the DTS snippet
> I might drop it. Or would you like me to keep it anyway?

Please keep it as long as you don't change the (SoC integration)
things I typically focus on (address, interrupts, clocks), and don't make
too wild changes ;-)

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