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

[Tudor Ambarus <tudor.ambarus@linaro.org>]

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

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