diff mbox series

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

Message ID 20240405-cdns-qspi-mbly-v2-5-956679866d6d@bootlin.com (mailing list archive)
State Superseded
Headers show
Series spi: cadence-qspi: add Mobileye EyeQ5 support | expand

Commit Message

Théo Lebrun April 5, 2024, 3:02 p.m. UTC
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.

Hide feature behind a flag. If unset and detected value is different
from the devicetree-provided value, warn.

Move probe cqspi->ddata assignment prior to cqspi_of_get_pdata() call.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/spi/spi-cadence-quadspi.c | 38 ++++++++++++++++++++++++++++++++------
 1 file changed, 32 insertions(+), 6 deletions(-)

Comments

Mark Brown April 8, 2024, 2:10 p.m. UTC | #1
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?
Théo Lebrun April 8, 2024, 2:38 p.m. UTC | #2
Hello,

On Mon Apr 8, 2024 at 4:10 PM CEST, Mark Brown wrote:
> 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.

You are correct of course. Wording is incorrect. I wanted to highlight
that FIFO depth does not change for existing HW and still relies as
before on devicetree value.

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

I see. Will add comment or rework code to make more straight forward, or
both.

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

DT value isn't expected for compatibles with CQSPI_DETECT_FIFO_DEPTH
quirk, therefore we do not ignore a specified property. Bindings agree:
prop is false with EyeQ5 compatible.

> - 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?).

The goal was a simpler devicetree on Mobileye platform. This is why we
add this behavior flag. You prefer the property to be always present?
This is a only a nice-to-have, you tell me what you prefer.

I wasn't sure all HW behaved in the same way wrt read-only bits in
SRAMPARTITION, and I do not have access to other platforms exploiting
this driver. This is why I kept behavior reserved for EyeQ5-integrated
IP block.

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

If we do not simplify devicetree, then I'd vote for dropping this patch
entirely. Adding code for detecting such an edge-case doesn't sound
useful. Especially since this kind of error should only occur to people
adding new hardware support; those probably do not need a nice
user-facing error message. What do you think?

Regards,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Théo Lebrun April 8, 2024, 2:45 p.m. UTC | #3
Hello,

On Mon Apr 8, 2024 at 4:38 PM CEST, Théo Lebrun wrote:
> Hello,
>
> On Mon Apr 8, 2024 at 4:10 PM CEST, Mark Brown wrote:
> > 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.
>
> You are correct of course. Wording is incorrect. I wanted to highlight
> that FIFO depth does not change for existing HW and still relies as
> before on devicetree value.
>
> > > -	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.
>
> I see. Will add comment or rework code to make more straight forward, or
> both.
>
> > > +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
>
> DT value isn't expected for compatibles with CQSPI_DETECT_FIFO_DEPTH
> quirk, therefore we do not ignore a specified property. Bindings agree:
> prop is false with EyeQ5 compatible.
>
> > - 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?).
>
> The goal was a simpler devicetree on Mobileye platform. This is why we
> add this behavior flag. You prefer the property to be always present?
> This is a only a nice-to-have, you tell me what you prefer.
>
> I wasn't sure all HW behaved in the same way wrt read-only bits in
> SRAMPARTITION, and I do not have access to other platforms exploiting
> this driver. This is why I kept behavior reserved for EyeQ5-integrated
> IP block.
>
> > 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?
>
> If we do not simplify devicetree, then I'd vote for dropping this patch
> entirely. Adding code for detecting such an edge-case doesn't sound
> useful. Especially since this kind of error should only occur to people
> adding new hardware support; those probably do not need a nice
> user-facing error message. What do you think?

Option you hinted at on dt-bindings patch sounds nice to my ears:

 - Optional devicetree property;
 - If present, check HW value and warn if different;
 - If absent, use HW value.

This makes for a nice devicetree and simplifies driver code by removing
one quirk.

Sorry for delayed second thought.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Mark Brown April 8, 2024, 2:51 p.m. UTC | #4
On Mon, Apr 08, 2024 at 04:38:56PM +0200, Théo Lebrun wrote:
> On Mon Apr 8, 2024 at 4:10 PM CEST, Mark Brown wrote:
> > On Fri, Apr 05, 2024 at 05:02:15PM +0200, Théo Lebrun wrote:

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

> DT value isn't expected for compatibles with CQSPI_DETECT_FIFO_DEPTH
> quirk, therefore we do not ignore a specified property. Bindings agree:
> prop is false with EyeQ5 compatible.

Sure, but it's not obvious that that is the most helpful or constructive
way to handle things.

> > - 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?).

> The goal was a simpler devicetree on Mobileye platform. This is why we
> add this behavior flag. You prefer the property to be always present?
> This is a only a nice-to-have, you tell me what you prefer.

I would prefer that the property is always optional, or only required on
platforms where we know that the depth isn't probeable.

> I wasn't sure all HW behaved in the same way wrt read-only bits in
> SRAMPARTITION, and I do not have access to other platforms exploiting
> this driver. This is why I kept behavior reserved for EyeQ5-integrated
> IP block.

Well, if there's such little confidence that the depth is reported then
we shouldn't be logging an error.

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

> If we do not simplify devicetree, then I'd vote for dropping this patch
> entirely. Adding code for detecting such an edge-case doesn't sound
> useful. Especially since this kind of error should only occur to people
> adding new hardware support; those probably do not need a nice
> user-facing error message. What do you think?

I'm confused why you think dropping the patch is a good idea?
Théo Lebrun April 9, 2024, 10:07 a.m. UTC | #5
Hello,

On Mon Apr 8, 2024 at 4:51 PM CEST, Mark Brown wrote:
> On Mon, Apr 08, 2024 at 04:38:56PM +0200, Théo Lebrun wrote:
> > On Mon Apr 8, 2024 at 4:10 PM CEST, Mark Brown wrote:
> > > On Fri, Apr 05, 2024 at 05:02:15PM +0200, Théo Lebrun wrote:
>
> > > > +	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
>
> > DT value isn't expected for compatibles with CQSPI_DETECT_FIFO_DEPTH
> > quirk, therefore we do not ignore a specified property. Bindings agree:
> > prop is false with EyeQ5 compatible.
>
> Sure, but it's not obvious that that is the most helpful or constructive
> way to handle things.

Agreed, a simpler solution can be found.

> > > - 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?).
>
> > The goal was a simpler devicetree on Mobileye platform. This is why we
> > add this behavior flag. You prefer the property to be always present?
> > This is a only a nice-to-have, you tell me what you prefer.
>
> I would prefer that the property is always optional, or only required on
> platforms where we know that the depth isn't probeable.
>
> > I wasn't sure all HW behaved in the same way wrt read-only bits in
> > SRAMPARTITION, and I do not have access to other platforms exploiting
> > this driver. This is why I kept behavior reserved for EyeQ5-integrated
> > IP block.
>
> Well, if there's such little confidence that the depth is reported then
> we shouldn't be logging an error.
>
> > > 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?
>
> > If we do not simplify devicetree, then I'd vote for dropping this patch
> > entirely. Adding code for detecting such an edge-case doesn't sound
> > useful. Especially since this kind of error should only occur to people
> > adding new hardware support; those probably do not need a nice
> > user-facing error message. What do you think?
>
> I'm confused why you think dropping the patch is a good idea?

Sorry I was unclear. I'll recap here options I see possible.

 - (1) Require DT property for all compatibles. That would be my
   preferred option *if* you think we should keep the DT property
   mandatory. I do not think requiring property AND detecting at
   runtime is useful.

 - (2) Require DT property for all but EyeQ5 compatible. On this
   platform, runtime detection is done.
    - (2a) On others, warn if value is different from DT property.
    - (2b) On others, do not detect+warn.

 - (3) Make DT property optional for all compatibles.
    - (3a) If provided, warn if runtime detect value is different.
    - (3b) If provided, do not detect+warn.

My preference would go to (3a):
 - we avoid a new quirk,
 - we avoid dt-bindings conditionals based on compatible,
 - we add a warning for a potentially buggy behavior and,
 - we do not modify FIFO depth used for existing devicetrees.

To make a choice, it'd be useful to know other platform behaviors. I
have no reason to think this SRAMPARTITION behavior isn't reproducable
on other platforms but I cannot guarantee anything. I just tested on TI
J7200 EVM with the quad SPI-NOR instance (spi@47040000) and it works as
expected.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Mark Brown April 9, 2024, 3:51 p.m. UTC | #6
On Tue, Apr 09, 2024 at 12:07:56PM +0200, Théo Lebrun wrote:

>  - (3) Make DT property optional for all compatibles.
>     - (3a) If provided, warn if runtime detect value is different.
>     - (3b) If provided, do not detect+warn.

I think either of these is fine.
diff mbox series

Patch

diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
index abc1c35929cc..04a473fafe43 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -42,6 +42,7 @@  static_assert(CQSPI_MAX_CHIPSELECT <= SPI_CS_CNT_MAX);
 #define CQSPI_NO_SUPPORT_WR_COMPLETION	BIT(3)
 #define CQSPI_SLOW_SRAM		BIT(4)
 #define CQSPI_NEEDS_APB_AHB_HAZARD_WAR	BIT(5)
+#define CQSPI_DETECT_FIFO_DEPTH		BIT(6)
 
 /* Capabilities */
 #define CQSPI_SUPPORTS_OCTAL		BIT(0)
@@ -1500,13 +1501,15 @@  static int cqspi_of_get_flash_pdata(struct platform_device *pdev,
 
 static int cqspi_of_get_pdata(struct cqspi_st *cqspi)
 {
+	const struct cqspi_driver_platdata *ddata = cqspi->ddata;
 	struct device *dev = &cqspi->pdev->dev;
 	struct device_node *np = dev->of_node;
 	u32 id[2];
 
 	cqspi->is_decoded_cs = of_property_read_bool(np, "cdns,is-decoded-cs");
 
-	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");
 		return -ENXIO;
 	}
@@ -1538,8 +1541,6 @@  static void cqspi_controller_init(struct cqspi_st *cqspi)
 {
 	u32 reg;
 
-	cqspi_controller_enable(cqspi, 0);
-
 	/* Configure the remap address register, no remap */
 	writel(0, cqspi->iobase + CQSPI_REG_REMAP);
 
@@ -1573,8 +1574,29 @@  static void cqspi_controller_init(struct cqspi_st *cqspi)
 		reg |= CQSPI_REG_CONFIG_DMA_MASK;
 		writel(reg, cqspi->iobase + CQSPI_REG_CONFIG);
 	}
+}
 
-	cqspi_controller_enable(cqspi, 1);
+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);
+	}
 }
 
 static int cqspi_request_mmap_dma(struct cqspi_st *cqspi)
@@ -1727,6 +1749,7 @@  static int cqspi_probe(struct platform_device *pdev)
 	cqspi->pdev = pdev;
 	cqspi->host = host;
 	cqspi->is_jh7110 = false;
+	cqspi->ddata = ddata = of_device_get_match_data(dev);
 	platform_set_drvdata(pdev, cqspi);
 
 	/* Obtain configuration from OF. */
@@ -1818,8 +1841,6 @@  static int cqspi_probe(struct platform_device *pdev)
 	/* write completion is supported by default */
 	cqspi->wr_completion = true;
 
-	ddata = of_device_get_match_data(dev);
-	cqspi->ddata = ddata;
 	if (ddata) {
 		if (ddata->quirks & CQSPI_NEEDS_WR_DELAY)
 			cqspi->wr_delay = 50 * DIV_ROUND_UP(NSEC_PER_SEC,
@@ -1861,7 +1882,10 @@  static int cqspi_probe(struct platform_device *pdev)
 	}
 
 	cqspi_wait_idle(cqspi);
+	cqspi_controller_enable(cqspi, 0);
+	cqspi_controller_detect_fifo_depth(cqspi);
 	cqspi_controller_init(cqspi);
+	cqspi_controller_enable(cqspi, 1);
 	cqspi->current_cs = -1;
 	cqspi->sclk = 0;
 
@@ -1944,7 +1968,9 @@  static int cqspi_runtime_resume(struct device *dev)
 
 	clk_prepare_enable(cqspi->clk);
 	cqspi_wait_idle(cqspi);
+	cqspi_controller_enable(cqspi, 0);
 	cqspi_controller_init(cqspi);
+	cqspi_controller_enable(cqspi, 1);
 
 	cqspi->current_cs = -1;
 	cqspi->sclk = 0;