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

  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.

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 Documentation/devicetree/bindings/spi/spi-controller.yaml | 5 +++++
 1 file changed, 5 insertions(+)
  

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.

> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---
>  Documentation/devicetree/bindings/spi/spi-controller.yaml | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/spi/spi-controller.yaml b/Documentation/devicetree/bindings/spi/spi-controller.yaml
> index 524f6fe8c27b..99272e6f115e 100644
> --- a/Documentation/devicetree/bindings/spi/spi-controller.yaml
> +++ b/Documentation/devicetree/bindings/spi/spi-controller.yaml
> @@ -69,6 +69,11 @@ properties:
>           Should be generally avoided and be replaced by
>           spi-cs-high + ACTIVE_HIGH.
>  
> +  fifo-depth:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Size of the data FIFO in bytes.
> +
>    num-cs:
>      $ref: /schemas/types.yaml#/definitions/uint32
>      description:
> -- 
> 2.43.0.687.g38aa6559b0-goog
>
  

+ 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. At least I don't
see how it would work, I guess it will use the minimum depth between the
two?

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)

Cheers,
ta


----
$ git grep fifo Documentation/devicetree/bindings/spi/
Documentation/devicetree/bindings/spi/atmel,at91rm9200-spi.yaml:
atmel,fifo-size:
Documentation/devicetree/bindings/spi/atmel,at91rm9200-spi.yaml:
atmel,fifo-size = <32>;
Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml:
cdns,fifo-depth:
Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml:
cdns,fifo-depth:
Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml:  cdns,fifo-depth:
Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml:  cdns,fifo-width:
Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml:  - cdns,fifo-depth
Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml:  - cdns,fifo-width
Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml:
cdns,fifo-depth = <128>;
Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml:
cdns,fifo-width = <4>;
Documentation/devicetree/bindings/spi/renesas,sh-msiof.yaml:
renesas,tx-fifo-size:
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.
Documentation/devicetree/bindings/spi/spi-sifive.yaml:  sifive,fifo-depth:
Documentation/devicetree/bindings/spi/spi-sifive.yaml:
sifive,fifo-depth = <8>;
  

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.
  

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
  

Hi Tudor,

On Thu, Feb 8, 2024 at 2:51 PM Tudor Ambarus <tudor.ambarus@linaro.org> 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.

Ha...

Current documentation for the Clock-Synchronized Serial Interface with
FIFO (MSIOF) on e.g. R-Car Gen2 and later states:

    FIFO capacity: 32 bits × 64 stages for transmission and 32 bits ×
256 stages for reception

Initially (many years ago), there was some doubt about the validity
of these values (older variants on SH supported 64/64), hence
drivers/spi/spi-sh-msiof.c still has

    .tx_fifo_size = 64,
    .rx_fifo_size = 64,

Probably we should test and revisit this...

> --- a/Documentation/devicetree/bindings/spi/spi-controller.yaml
> +++ b/Documentation/devicetree/bindings/spi/spi-controller.yaml
> @@ -69,6 +69,11 @@ properties:
>           Should be generally avoided and be replaced by
>           spi-cs-high + ACTIVE_HIGH.
>
> +  fifo-depth:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Size of the data FIFO in bytes.

I think it is prudent to consider the asymmetric case, too.
Whether that should be just two properties ("rx-fifo-depth" and
"tx-fifo-depth"), or also a third "fifo-depth", I defer to the DT
maintainers...

> +
>    num-cs:
>      $ref: /schemas/types.yaml#/definitions/uint32
>      description:

Gr{oetje,eeting}s,

                        Geert
  

On Fri, Feb 09, 2024 at 04:21:16PM +0000, Conor Dooley wrote:
> On Fri, Feb 09, 2024 at 01:56:56PM +0000, Tudor Ambarus wrote:

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

You do get devices that are single duplex only where the mismatched
sizes wouldn't be a pressing issue.
  

On 09/02/2024 18:13, Geert Uytterhoeven wrote:
> Hi Tudor,
> 
> On Thu, Feb 8, 2024 at 2:51 PM Tudor Ambarus <tudor.ambarus@linaro.org> 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.
> 
> Ha...
> 
> Current documentation for the Clock-Synchronized Serial Interface with
> FIFO (MSIOF) on e.g. R-Car Gen2 and later states:
> 
>     FIFO capacity: 32 bits × 64 stages for transmission and 32 bits ×
> 256 stages for reception
> 
> Initially (many years ago), there was some doubt about the validity
> of these values (older variants on SH supported 64/64), hence
> drivers/spi/spi-sh-msiof.c still has
> 
>     .tx_fifo_size = 64,
>     .rx_fifo_size = 64,
> 
> Probably we should test and revisit this...
> 
>> --- a/Documentation/devicetree/bindings/spi/spi-controller.yaml
>> +++ b/Documentation/devicetree/bindings/spi/spi-controller.yaml
>> @@ -69,6 +69,11 @@ properties:
>>           Should be generally avoided and be replaced by
>>           spi-cs-high + ACTIVE_HIGH.
>>
>> +  fifo-depth:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description:
>> +      Size of the data FIFO in bytes.
> 
> I think it is prudent to consider the asymmetric case, too.
> Whether that should be just two properties ("rx-fifo-depth" and
> "tx-fifo-depth"), or also a third "fifo-depth", I defer to the DT
> maintainers...

Since most of the cases FIFO depth tx=rx, we could go with three
properties and:

allOf:
 - not:
     required:
       - fifo-depth
       - tx-fifo-depth
 - not:
     required:
       - fifo-depth
       - rx-fifo-depth

and probably dependencies between rx and tx (see example-schema).

Best regards,
Krzysztof
  

Hi, Geert, Krzysztof,

On 2/11/24 13:49, Krzysztof Kozlowski wrote:> On 09/02/2024 18:13, Geert
Uytterhoeven wrote:
>> Hi Tudor,
>>
>> On Thu, Feb 8, 2024 at 2:51 PM Tudor Ambarus <tudor.ambarus@linaro.org> 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.
>>
>> Ha...
>>
>> Current documentation for the Clock-Synchronized Serial Interface with
>> FIFO (MSIOF) on e.g. R-Car Gen2 and later states:
>>
>>     FIFO capacity: 32 bits × 64 stages for transmission and 32 bits ×
>> 256 stages for reception
>>
>> Initially (many years ago), there was some doubt about the validity
>> of these values (older variants on SH supported 64/64), hence
>> drivers/spi/spi-sh-msiof.c still has
>>
>>     .tx_fifo_size = 64,
>>     .rx_fifo_size = 64,
>>
>> Probably we should test and revisit this...
>>
>>> --- a/Documentation/devicetree/bindings/spi/spi-controller.yaml
>>> +++ b/Documentation/devicetree/bindings/spi/spi-controller.yaml
>>> @@ -69,6 +69,11 @@ properties:
>>>           Should be generally avoided and be replaced by
>>>           spi-cs-high + ACTIVE_HIGH.
>>>
>>> +  fifo-depth:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    description:
>>> +      Size of the data FIFO in bytes.
>>
>> I think it is prudent to consider the asymmetric case, too.
>> Whether that should be just two properties ("rx-fifo-depth" and
>> "tx-fifo-depth"), or also a third "fifo-depth", I defer to the DT
>> maintainers...

Thanks, Geert for the insight!
> 
> Since most of the cases FIFO depth tx=rx, we could go with three
> properties and:
> 
> allOf:
>  - not:
>      required:
>        - fifo-depth
>        - tx-fifo-depth
>  - not:
>      required:
>        - fifo-depth
>        - rx-fifo-depth
> 
> and probably dependencies between rx and tx (see example-schema).
> 
Great. Thanks, Krzysztof! I'll give it a try.
Cheers,
ta
  

Hi Tudor,

On Fri, Feb 9, 2024 at 5:55 PM Tudor Ambarus <tudor.ambarus@linaro.org> 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@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?

See my other email
https://lore.kernel.org/all/CAMuHMdU_Hx9PLmHf2Xm1KKTy_OF-TeCv7SzmA5CZWz+PLkbAGA@mail.gmail.com

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@linux-m68korg

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
  

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@linaro.org> 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@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?
> 
> See my other email
> https://lore.kernel.org/all/CAMuHMdU_Hx9PLmHf2Xm1KKTy_OF-TeCv7SzmA5CZWz+PLkbAGA@mail.gmail.com
> 

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