Received-SPF: pass (google.com: domain of linux-kernel+bounces-61334-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240208135045.3728927-1-tudor.ambarus@linaro.org>
 <20240208135045.3728927-2-tudor.ambarus@linaro.org> <20240208-grating-legwarmer-0a04cfb04d61@spud>
 <c2b08463-cb13-4e9b-8797-8ebcf1047f66@linaro.org> <20240209-chest-sleet-a119fc3d4243@spud>
 <0ac8d573-6486-458d-afb9-090b5f8d4a21@linaro.org>
In-Reply-To: <0ac8d573-6486-458d-afb9-090b5f8d4a21@linaro.org>
From: Geert Uytterhoeven <geert@linux-m68k.org>
Date: Mon, 12 Feb 2024 11:38:51 +0100
Message-ID: <CAMuHMdXEKecx-wQCSzqmRr6af2AUOnoFhfD2JLx28n8OYnvzGw@mail.gmail.com>
Subject: Re: [PATCH 01/12] spi: dt-bindings: introduce the ``fifo-depth`` property
To: Tudor Ambarus <tudor.ambarus@linaro.org>
Cc: Conor Dooley <conor@kernel.org>, broonie@kernel.org, robh@kernel.org, 
	andi.shyti@kernel.org, semen.protsenko@linaro.org, 
	krzysztof.kozlowski@linaro.org, alim.akhtar@samsung.com, 
	linux-spi@vger.kernel.org, linux-samsung-soc@vger.kernel.org, 
	linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, 
	andre.draszik@linaro.org, peter.griffin@linaro.org, kernel-team@android.com, 
	willmcvicker@google.com, conor+dt@kernel.org, devicetree@vger.kernel.org, 
	arnd@arndb.de
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Tudor,

On Fri, Feb 9, 2024 at 5:55=E2=80=AFPM 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 integr=
ator
> >>>> with different FIFO depths. Introduce the fifo-depth property to all=
ow
> >>>> such nodes to specify their FIFO depth.
> >>>>
> >>>> We haven't seen SPI IPs with different FIFO depths for RX and TX, th=
us
> >>>> introduce a single property.
> >>>
> >>> Some citation attached to this would be nice. "We haven't seen" offer=
s
> >>> no detail as to what IPs that allow this sort of configuration of FIF=
O
> >>> 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 t=
he
> >> 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 t=
he
> >> 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+PLk=
bAGA@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 ca=
n
> >> 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/driv=
ers/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-m68k=
org

In personal conversations with technical people, I call myself a hacker. Bu=
t
when I'm talking to journalists I just say "programmer" or something like t=
hat.
                                -- Linus Torvalds