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

[Tudor Ambarus <tudor.ambarus@xxxxxxxxxx>]

On 2/12/24 10:38, Geert Uytterhoeven wrote:
> Hi Tudor,

Hi, Geert!

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

I saw the response, thanks again!

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

Got it.

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

I guess I get it now. You marked those properties as deprecated so that
users stop using them and rely on the driver based compatible values,
but at the same time you allowed the devicetree properties to have a
higher priority than the driver based compatible values in case one
really wants/needs to use the dt properties. I don't have a preference
here, I guess it's fine.

Thanks for the explanations!
ta