| Message ID | 20240208135045.3728927-2-tudor.ambarus@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [01/12] spi: dt-bindings: introduce the ``fifo-depth`` property | expand |
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
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
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:
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(+)