Re: [PATCH 01/12] spi: dt-bindings: introduce the ``fifo-depth`` property

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

Hi Tudor,

On Fri, Feb 9, 2024 at 5:55 PM Tudor Ambarus <tudor.ambarus@xxxxxxxxxx> wrote:
> On 2/9/24 16:21, Conor Dooley wrote:
> > On Fri, Feb 09, 2024 at 01:56:56PM +0000, Tudor Ambarus wrote:
> >> On 2/8/24 18:24, Conor Dooley wrote:
> >>> On Thu, Feb 08, 2024 at 01:50:34PM +0000, Tudor Ambarus wrote:
> >>>> There are instances of the same IP that are configured by the integrator
> >>>> with different FIFO depths. Introduce the fifo-depth property to allow
> >>>> such nodes to specify their FIFO depth.
> >>>>
> >>>> We haven't seen SPI IPs with different FIFO depths for RX and TX, thus
> >>>> introduce a single property.
> >>>
> >>> Some citation attached to this would be nice. "We haven't seen" offers
> >>> no detail as to what IPs that allow this sort of configuration of FIFO
> >>> size that you have actually checked.
> >>>
> >>> I went and checked our IP that we use in FPGA fabric, which has a
> >>> configurable fifo depth. It only has a single knob for both RX and TX
> >>> FIFOs. The Xilinx xps spi core also has configurable FIFOs, but again RX
> >>> and TX sizes are tied there. At least that's a sample size of three.
> >>>
> >>> One of our guys is working on support for the IP I just mentioned and
> >>> would be defining a vendor property for this, so
> >>> Reviewed-by: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>
> >>>
> >>
> >> Thanks, Conor. I had in mind that SPI has a shift register and it's
> >> improbable to have different FIFO depths for RX and TX.
> >
> > IDK, but I've learned to expect the unexpectable, especially when it
> > comes to the IPs intended for use in FPGAs.
> >
> >> At least I don't
> >> see how it would work, I guess it will use the minimum depth between the
> >> two?
> >
> > I'm not really sure how it would work other than that in the general
> > case, but some use case specific configuration could work, but I do
> > agree that it is
> >
> >> I grepped by "fifo" in the spi bindings and I now see that renesas is
> >> using dedicated properties for RX and TX, but I think that there too the
> >> FIFOs have the same depths. Looking into drivers/spi/spi-sh-msiof.c I
> >> see that the of_device_id.data contains 64 bytes FIFOs for RX and TX,
> >> regardless of the compatible.
> >>
> >> Geert, any idea if the FIFO depths can differ for RX and TX in
> >> spi-sh-msiof.c?

See my other email
https://lore.kernel.org/all/CAMuHMdU_Hx9PLmHf2Xm1KKTy_OF-TeCv7SzmA5CZWz+PLkbAGA@xxxxxxxxxxxxxx

Note that at one point we did have 64/256 in the driver, but that was
changed in commit fe78d0b7691c0274 ("spi: sh-msiof: Fix FIFO size to
64 word from 256 word").  Diving into the discussion around that patch,
there seem to be two factors at play:
  1. Actual FIFO size,
  2. Maximum transfer size per block
     (up to 2 blocks on R-Car, up to 4 blocks on SH(-Mobile)).
As the driver supports only a single block, it should be limited to
64 on R-Car Gen2/3.  R-Car Gen4 claims to have widened the register
bit field for the maximum transfer size per block, so 256 might be
possible there...

> >> Anyway, even if there are such imbalanced architectures, I guess we can
> >> consider them when/if they appear? (add rx/tx-fifo-depth dt properties)
> >
> > I think so.
> >
> >> Documentation/devicetree/bindings/spi/renesas,sh-msiof.yaml:
> >> Override the default TX fifo size.  Unit is words.  Ignored if 0.
> >> Documentation/devicetree/bindings/spi/renesas,sh-msiof.yaml:
> >> renesas,rx-fifo-size:
> >> Documentation/devicetree/bindings/spi/renesas,sh-msiof.yaml:
> >> Override the default RX fifo size.  Unit is words.  Ignored if 0.
> >
> > These renesas ones seemed interesting at first glance due to these
> > comments, but what's missed by grep the is "deprecated" marking on
> > these. They seem to have been replaced by soc-specific compatibles.
>
> In the driver the renesas,{rx,tx}-fifo-size properties still have the
> highest priority:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git/tree/drivers/spi/spi-sh-msiof.c#n1350
>
> Maybe something for Geert, as I see he was the one marking these
> properties as deprecated. I guess he forgot to update the driver.
>
> Anyway, I think we shall be fine, even if we don't hear from Geert.

The renesas,{rx,tx}-fifo-size properties date back to the early days
of DT an ARM, when it was assumed that slightly different versions of
IP cores could be handled well using a single common compatible value,
and properties describing the (few) differences.  The pitfall here
is the "few differences": too many times people discovered later that
there were more differences, needing more properties, and complicating
backwards-compatibility.

Hence the handling of different FIFO sizes was moved to the driver based
on compatible values, and the renesas,{rx,tx}-fifo-size properties were
deprecated.  See commit beb74bb0875579c4 ("spi: sh-msiof: Add support
for R-Car H2 and M2"), which shows that there were more changes
needed than the anticipated FIFO sizes.  And more were added later,
see later additions to sh_msiof_chipdata.

So unless it is meant for a configurable synthesizable IP core, where
this is a documented parameter of the IP core, I advise against
specifying the FIFO size(s) in DT.

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