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

On Fri, Feb 09, 2024 at 01:56:56PM +0000, Tudor Ambarus wrote:

+ Geert

On 2/8/24 18:24, Conor Dooley wrote:

quoted

On Thu, Feb 08, 2024 at 01:50:34PM +0000, Tudor Ambarus wrote:

quoted

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@microchip.com>

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.