Re: [PATCH v2 05/11] spi: cadence-qspi: add FIFO depth detection quirk

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2371 bytes --]

On Fri, Apr 05, 2024 at 05:02:15PM +0200, Théo 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 = cqspi->ddata;
> +	struct device *dev = &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 = readl(cqspi->iobase + CQSPI_REG_SRAMPARTITION);
> +	fifo_depth = reg + 1;
> +
> +	if (ddata && ddata->quirks & CQSPI_DETECT_FIFO_DEPTH) {
> +		cqspi->fifo_depth = fifo_depth;
> +		dev_dbg(dev, "using FIFO depth of %u\n", fifo_depth);
> +	} else if (fifo_depth != 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?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]