On 2/9/24 16:21, Conor Dooley wrote: > On Fri, Feb 09, 2024 at 01:56:56PM +0000, Tudor Ambarus wrote: >> >> + Geert >> >> 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? >> >> 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. Cheers, ta