| 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 |
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?
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
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
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?
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
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 --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;
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(-)