Received-SPF: pass (google.com: domain of linux-kernel+bounces-135483-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Mon, 8 Apr 2024 15:10:28 +0100
From: Mark Brown <broonie@kernel.org>
To: =?iso-8859-1?Q?Th=E9o?= Lebrun <theo.lebrun@bootlin.com>
Cc: Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Vaishnav Achath <vaishnav.a@ti.com>,
	Thomas Bogendoerfer <tsbogend@alpha.franken.de>,
	Rob Herring <robh@kernel.org>, linux-spi@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-mips@vger.kernel.org,
	Vladimir Kondratiev <vladimir.kondratiev@mobileye.com>,
	Gregory CLEMENT <gregory.clement@bootlin.com>,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	Tawfik Bayouk <tawfik.bayouk@mobileye.com>
Subject: Re: [PATCH v2 05/11] spi: cadence-qspi: add FIFO depth detection
 quirk
Message-ID: <551bea0a-1c9e-4e04-87db-c643fdaee85e@sirena.org.uk>
References: <20240405-cdns-qspi-mbly-v2-0-956679866d6d@bootlin.com>
 <20240405-cdns-qspi-mbly-v2-5-956679866d6d@bootlin.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="97Qcf0iyghKpwfU3"
Content-Disposition: inline
In-Reply-To: <20240405-cdns-qspi-mbly-v2-5-956679866d6d@bootlin.com>


--97Qcf0iyghKpwfU3
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Fri, Apr 05, 2024 at 05:02:15PM +0200, Th=E9o Lebrun wrote:

> Use hardware ability to read the FIFO depth thanks to
> CQSPI_REG_SRAMPARTITION that is partially read-only. Keep current
> behavior identical for existing compatibles.

The behaviour is not identical here - we now unconditionally probe the
FIFO depth on all hardware, the difference with the quirk is that we
will ignore any DT property specifying the depth.

> -	if (of_property_read_u32(np, "cdns,fifo-depth", &cqspi->fifo_depth)) {
> +	if (!(ddata && ddata->quirks & CQSPI_DETECT_FIFO_DEPTH) &&
> +	    of_property_read_u32(np, "cdns,fifo-depth", &cqspi->fifo_depth)) {
>  		dev_err(dev, "couldn't determine fifo-depth\n");

It's not obvious from just the code that we do handle having a FIFO
depth property and detection in the detection code, at least a comment
would be good.

> +static void cqspi_controller_detect_fifo_depth(struct cqspi_st *cqspi)
> +{
> +	const struct cqspi_driver_platdata *ddata =3D cqspi->ddata;
> +	struct device *dev =3D &cqspi->pdev->dev;
> +	u32 reg, fifo_depth;
> +
> +	/*
> +	 * Bits N-1:0 are writable while bits 31:N are read as zero, with 2^N
> +	 * the FIFO depth.
> +	 */
> +	writel(U32_MAX, cqspi->iobase + CQSPI_REG_SRAMPARTITION);
> +	reg =3D readl(cqspi->iobase + CQSPI_REG_SRAMPARTITION);
> +	fifo_depth =3D reg + 1;
> +
> +	if (ddata && ddata->quirks & CQSPI_DETECT_FIFO_DEPTH) {
> +		cqspi->fifo_depth =3D fifo_depth;
> +		dev_dbg(dev, "using FIFO depth of %u\n", fifo_depth);
> +	} else if (fifo_depth !=3D cqspi->fifo_depth) {
> +		dev_warn(dev, "detected FIFO depth (%u) different from config (%u)\n",
> +			 fifo_depth, cqspi->fifo_depth);
> +	}

It's not obvious to me that we should ignore an explicitly specified
property if the quirk is present - if anything I'd more expect to see
the new warning in that case, possibly with a higher severity if we're
saying that the quirk means we're more confident that the data reported
by the hardware is reliable.  I think what I'd expect is that we always
use an explicitly specified depth (hopefully the user was specifying it
for a reason?).

Pulling all the above together can we just drop the quirk and always do
the detection, or leave the quirk as just controlling the severity with
which we log any difference between detected and explicitly configured
depths?

--97Qcf0iyghKpwfU3
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAmYT+tMACgkQJNaLcl1U
h9BRWwf+Nn0TIUbAm2J8N2ADxHV5QvboVoCubMNpoS/qRWdNjo5kgnkbDgAqrTZU
Ms8Fp3rg41VV9aJw+eP1lypgpPLunatteZi6o4FzmEn5XL1s0K6+fkZ5MKkgBfhW
H03cNHSc9lhh4CJsYZy9naA+M/yaJC1c6Cjr7pW+h3EQfN2voDa2JG1cbEA1JB7q
PBKJTq7l7s5gpG/y49WdI4A7c+O4U0Zab36uOffPEypTE1drcZNPWZhJn8Yngv5b
lzcQzrF6vTaAqFwYG05pCLM8Fmh59SWLoNPseI7RmN+y0VGyHUdrfxVGVreApgYn
mZo0Eifdsts/a/IPS7iZHNlxo49wtg==
=xaan
-----END PGP SIGNATURE-----

--97Qcf0iyghKpwfU3--