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
