2024-04-05 15:03:26

by Théo Lebrun

Subject: [PATCH v2 00/11] spi: cadence-qspi: add Mobileye EyeQ5 support

Hi all,

V2 of this series adding octal SPI-NOR support to Mobileye EyeQ5
platform. It has been tested on EyeQ5 hardware successfully.
V1 cover letter [5] contains a brief summary of what gets added.

There is no dependency except if you want zero errors in devicetree:
system-controller series [3] for <&clocks> phandle.

Have a nice day,
Théo

[0]: https://lore.kernel.org/lkml/[email protected]/
[1]: https://lore.kernel.org/linux-mips/[email protected]/
[2]: https://git.kernel.org/pub/scm/linux/kernel/git/mips/linux.git/log/
[3]: https://lore.kernel.org/lkml/[email protected]/
[4]: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git/log/
[5]: https://lore.kernel.org/lkml/[email protected]/

To: Mark Brown <[email protected]>
To: Rob Herring <[email protected]>
To: Krzysztof Kozlowski <[email protected]>
To: Conor Dooley <[email protected]>
To: Vaishnav Achath <[email protected]>
To: Thomas Bogendoerfer <[email protected]>
To: Rob Herring <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: Vladimir Kondratiev <[email protected]>
Cc: Gregory CLEMENT <[email protected]>
Cc: Thomas Petazzoni <[email protected]>
Cc: Tawfik Bayouk <[email protected]>
Signed-off-by: Théo Lebrun <[email protected]>

Changes in v2:
- Rebase upon v6.9-rc2.
- Fix dt-bindings commit subject tags.
- Take Reviewed-by: Krzysztof Kozlowski on dt-bindings commit.
- Add dt-bindings commit to order compatibles alphabetically.
adding EyeQ5 compatible can be taken alone easily.
- Drop patch taken upstream:
- Add To: Rob Herring, following get_maintainer.pl recommendation.
- Link to v1: https://lore.kernel.org/r/[email protected]

Krzysztof: unsure if you want this. It is second so that commit
spi: cadence-qspi: switch from legacy names to modern ones
---
Théo Lebrun (11):
spi: dt-bindings: cdns,qspi-nor: add mobileye,eyeq5-ospi compatible
spi: dt-bindings: cdns,qspi-nor: sort compatibles alphabetically
spi: cadence-qspi: allow building for MIPS
spi: cadence-qspi: store device data pointer in private struct
spi: cadence-qspi: add FIFO depth detection quirk
spi: cadence-qspi: minimise register accesses on each op if !DTR
spi: cadence-qspi: add no-IRQ mode to indirect reads
spi: cadence-qspi: add early busywait to cqspi_wait_for_bit()
spi: cadence-qspi: add mobileye,eyeq5-ospi compatible
MIPS: mobileye: eyeq5: Add SPI-NOR controller node
MIPS: mobileye: eyeq5: add octal flash node to eval board DTS

.../devicetree/bindings/spi/cdns,qspi-nor.yaml | 19 +++-
arch/mips/boot/dts/mobileye/eyeq5-epm5.dts | 15 +++
arch/mips/boot/dts/mobileye/eyeq5.dtsi | 15 +++
drivers/spi/Kconfig | 2 +-
drivers/spi/spi-cadence-quadspi.c | 114 ++++++++++++++++-----
5 files changed, 132 insertions(+), 33 deletions(-)
---
base-commit: afccf1991d034a11ce0a1c21d90feba510838e34
change-id: 20240209-cdns-qspi-mbly-de2205a44ab3

Best regards,
--
Théo Lebrun <[email protected]>

2024-04-05 15:03:32

by Théo Lebrun

Subject: [PATCH v2 03/11] spi: cadence-qspi: allow building for MIPS

The Cadence QSPI Controller driver is used on Mobileye EyeQ5 platform.
Allow building on MIPS.

Signed-off-by: Théo Lebrun <[email protected]>
---
drivers/spi/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index bc7021da2fe9..548af3d9e30d 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -246,7 +246,7 @@ config SPI_CADENCE

config SPI_CADENCE_QUADSPI
tristate "Cadence Quad SPI controller"
- depends on OF && (ARM || ARM64 || X86 || RISCV || COMPILE_TEST)
+ depends on OF && (ARM || ARM64 || X86 || RISCV || MIPS || COMPILE_TEST)
help
Enable support for the Cadence Quad SPI Flash controller.


--
2.44.0

2024-04-05 15:03:33

by Théo Lebrun

Subject: [PATCH v2 02/11] spi: dt-bindings: cdns,qspi-nor: sort compatibles alphabetically

Compatibles are ordered by date of addition.
Switch to (deterministic) alphabetical ordering.

Signed-off-by: Théo Lebrun <[email protected]>
---
Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
index 5509c126b1cf..e53d443c6f93 100644
--- a/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
+++ b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
@@ -79,13 +79,13 @@ properties:
- items:
- enum:
- amd,pensando-elba-qspi
- - mobileye,eyeq5-ospi
- - ti,k2g-qspi
- - ti,am654-ospi
- intel,lgm-qspi
- - xlnx,versal-ospi-1.0
- intel,socfpga-qspi
+ - mobileye,eyeq5-ospi
- starfive,jh7110-qspi
+ - ti,am654-ospi
+ - ti,k2g-qspi
+ - xlnx,versal-ospi-1.0
- const: cdns,qspi-nor
- const: cdns,qspi-nor


--
2.44.0

2024-04-05 15:03:43

by Théo Lebrun

Subject: [PATCH v2 06/11] spi: cadence-qspi: minimise register accesses on each op if !DTR

cqspi_enable_dtr() is called for each operation, commands or not, reads
or writes. It writes CQSPI_REG_CONFIG then waits for idle (three
successful reads). Skip that in the no-DTR case if DTR is already
disabled.

It cannot be skipped in the DTR case as cqspi_setup_opcode_ext() writes
to a register and we must wait for idle state.

According to ftrace, the average cqspi_exec_mem_op() call goes from
85.4µs to 83.6µs when reading 235M over UBIFS on an octal flash.

Signed-off-by: Théo Lebrun <[email protected]>
---
drivers/spi/spi-cadence-quadspi.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
index 04a473fafe43..55d20d565fe5 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -492,8 +492,11 @@ static int cqspi_enable_dtr(struct cqspi_flash_pdata *f_pdata,
if (ret)
return ret;
} else {
- reg &= ~CQSPI_REG_CONFIG_DTR_PROTO;
- reg &= ~CQSPI_REG_CONFIG_DUAL_OPCODE;
+ unsigned int mask = CQSPI_REG_CONFIG_DTR_PROTO | CQSPI_REG_CONFIG_DUAL_OPCODE;
+ /* Shortcut if DTR is already disabled. */
+ if ((reg & mask) == 0)
+ return 0;
+ reg &= ~mask;
}

writel(reg, reg_base + CQSPI_REG_CONFIG);

--
2.44.0

2024-04-05 15:03:47

by Théo Lebrun

Subject: [PATCH v2 04/11] spi: cadence-qspi: store device data pointer in private struct

Avoid of_device_get_match_data() call on each IRQ and each read
operation. Store pointer in `struct cqspi_st` device instance.

End-to-end performance measurements improve with this patch. On a given
octal flash, reading 235M over UBIFS is ~3.4% faster. During that read,
the average cqspi_exec_mem_op() call goes from 85.4µs to 80.7µs
according to ftrace. The worst case goes from 622.4µs to 615.2µs.

Signed-off-by: Théo Lebrun <[email protected]>
---
drivers/spi/spi-cadence-quadspi.c | 15 ++++++---------
1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
index 350b3dab3a05..abc1c35929cc 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -102,6 +102,8 @@ struct cqspi_st {
bool apb_ahb_hazard;

bool is_jh7110; /* Flag for StarFive JH7110 SoC */
+
+ const struct cqspi_driver_platdata *ddata;
};

struct cqspi_driver_platdata {
@@ -334,11 +336,8 @@ static u32 cqspi_get_versal_dma_status(struct cqspi_st *cqspi)
static irqreturn_t cqspi_irq_handler(int this_irq, void *dev)
{
struct cqspi_st *cqspi = dev;
+ const struct cqspi_driver_platdata *ddata = cqspi->ddata;
unsigned int irq_status;
- struct device *device = &cqspi->pdev->dev;
- const struct cqspi_driver_platdata *ddata;
-
- ddata = of_device_get_match_data(device);

/* Read interrupt status */
irq_status = readl(cqspi->iobase + CQSPI_REG_IRQSTATUS);
@@ -1358,16 +1357,13 @@ static ssize_t cqspi_read(struct cqspi_flash_pdata *f_pdata,
const struct spi_mem_op *op)
{
struct cqspi_st *cqspi = f_pdata->cqspi;
- struct device *dev = &cqspi->pdev->dev;
- const struct cqspi_driver_platdata *ddata;
+ const struct cqspi_driver_platdata *ddata = cqspi->ddata;
loff_t from = op->addr.val;
size_t len = op->data.nbytes;
u_char *buf = op->data.buf.in;
u64 dma_align = (u64)(uintptr_t)buf;
int ret;

- ddata = of_device_get_match_data(dev);
-
ret = cqspi_read_setup(f_pdata, op);
if (ret)
return ret;
@@ -1822,7 +1818,8 @@ 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);
+ 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,

--
2.44.0

2024-04-05 15:04:23

by Théo Lebrun

Subject: [PATCH v2 07/11] spi: cadence-qspi: add no-IRQ mode to indirect reads

Support reads through polling, without any IRQ. The main reason is
performance; profiling shows that the first IRQ comes quickly on our
specific hardware. Once this IRQ arrives, we poll until all data is
retrieved. Avoid initial sleep to reduce IRQ count.

Hide this behavior behind a quirk flag.

This is confirmed through micro-benchmarks, but also end-to-end
performance tests. Mobileye EyeQ5, octal flash, reading 235M on a UBIFS
filesystem:
- No optimizations, ~10.34s, ~22.7 MB/s, 199230 IRQs
- CQSPI_SLOW_SRAM, ~10.34s, ~22.7 MB/s, 70284 IRQs
- CQSPI_RD_NO_IRQ, ~9.37s, ~25.1 MB/s, 521 IRQs

Signed-off-by: Théo Lebrun <[email protected]>
---
drivers/spi/spi-cadence-quadspi.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
index 55d20d565fe5..ebb8c35f50fd 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -43,6 +43,7 @@ static_assert(CQSPI_MAX_CHIPSELECT <= SPI_CS_CNT_MAX);
#define CQSPI_SLOW_SRAM BIT(4)
#define CQSPI_NEEDS_APB_AHB_HAZARD_WAR BIT(5)
#define CQSPI_DETECT_FIFO_DEPTH BIT(6)
+#define CQSPI_RD_NO_IRQ BIT(7)

/* Capabilities */
#define CQSPI_SUPPORTS_OCTAL BIT(0)
@@ -703,6 +704,7 @@ static int cqspi_indirect_read_execute(struct cqspi_flash_pdata *f_pdata,
const size_t n_rx)
{
struct cqspi_st *cqspi = f_pdata->cqspi;
+ bool use_irq = !(cqspi->ddata && cqspi->ddata->quirks & CQSPI_RD_NO_IRQ);
struct device *dev = &cqspi->pdev->dev;
void __iomem *reg_base = cqspi->iobase;
void __iomem *ahb_base = cqspi->ahb_base;
@@ -726,17 +728,20 @@ static int cqspi_indirect_read_execute(struct cqspi_flash_pdata *f_pdata,
* all the read interrupts disabled for max performance.
*/

- if (!cqspi->slow_sram)
+ if (use_irq && cqspi->slow_sram)
+ writel(CQSPI_REG_IRQ_WATERMARK, reg_base + CQSPI_REG_IRQMASK);
+ else if (use_irq)
writel(CQSPI_IRQ_MASK_RD, reg_base + CQSPI_REG_IRQMASK);
else
- writel(CQSPI_REG_IRQ_WATERMARK, reg_base + CQSPI_REG_IRQMASK);
+ writel(0, reg_base + CQSPI_REG_IRQMASK);

reinit_completion(&cqspi->transfer_complete);
writel(CQSPI_REG_INDIRECTRD_START_MASK,
reg_base + CQSPI_REG_INDIRECTRD);

while (remaining > 0) {
- if (!wait_for_completion_timeout(&cqspi->transfer_complete,
+ if (use_irq &&
+ !wait_for_completion_timeout(&cqspi->transfer_complete,
msecs_to_jiffies(CQSPI_READ_TIMEOUT_MS)))
ret = -ETIMEDOUT;

@@ -778,7 +783,7 @@ static int cqspi_indirect_read_execute(struct cqspi_flash_pdata *f_pdata,
bytes_to_read = cqspi_get_rd_sram_level(cqspi);
}

- if (remaining > 0) {
+ if (use_irq && remaining > 0) {
reinit_completion(&cqspi->transfer_complete);
if (cqspi->slow_sram)
writel(CQSPI_REG_IRQ_WATERMARK, reg_base + CQSPI_REG_IRQMASK);

--
2.44.0

2024-04-05 15:04:23

by Théo Lebrun

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

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 <[email protected]>
---
drivers/spi/spi-cadence-quadspi.c | 38 ++++++++++++++++++++++++++++++++------
1 file changed, 32 insertions(+), 6 deletions(-)

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;

--
2.44.0

2024-04-05 15:05:37

by Théo Lebrun

Subject: [PATCH v2 08/11] spi: cadence-qspi: add early busywait to cqspi_wait_for_bit()

If the CQSPI_BUSYWAIT_EARLY quirk flag is on, call
readl_relaxed_poll_timeout() with no sleep at the start of
cqspi_wait_for_bit(). If its short timeout expires, a sleeping
readl_relaxed_poll_timeout() call takes the relay.

Behavior is hidden behind a quirk flag to keep the previous behavior the
same on all platforms.

The reason is to avoid hrtimer interrupts on the system. All read
operations take less than 100µs.

Signed-off-by: Théo Lebrun <[email protected]>
---
drivers/spi/spi-cadence-quadspi.c | 34 +++++++++++++++++++++++++---------
1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
index ebb8c35f50fd..230aad490e03 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -44,6 +44,7 @@ static_assert(CQSPI_MAX_CHIPSELECT <= SPI_CS_CNT_MAX);
#define CQSPI_NEEDS_APB_AHB_HAZARD_WAR BIT(5)
#define CQSPI_DETECT_FIFO_DEPTH BIT(6)
#define CQSPI_RD_NO_IRQ BIT(7)
+#define CQSPI_BUSYWAIT_EARLY BIT(8)

/* Capabilities */
#define CQSPI_SUPPORTS_OCTAL BIT(0)
@@ -110,7 +111,7 @@ struct cqspi_st {

struct cqspi_driver_platdata {
u32 hwcaps_mask;
- u8 quirks;
+ u16 quirks;
int (*indirect_read_dma)(struct cqspi_flash_pdata *f_pdata,
u_char *rxbuf, loff_t from_addr, size_t n_rx);
u32 (*get_dma_status)(struct cqspi_st *cqspi);
@@ -121,6 +122,7 @@ struct cqspi_driver_platdata {
/* Operation timeout value */
#define CQSPI_TIMEOUT_MS 500
#define CQSPI_READ_TIMEOUT_MS 10
+#define CQSPI_BUSYWAIT_TIMEOUT_US 500

/* Runtime_pm autosuspend delay */
#define CQSPI_AUTOSUSPEND_TIMEOUT 2000
@@ -299,13 +301,27 @@ struct cqspi_driver_platdata {

#define CQSPI_REG_VERSAL_DMA_VAL 0x602

-static int cqspi_wait_for_bit(void __iomem *reg, const u32 mask, bool clr)
+static int cqspi_wait_for_bit(const struct cqspi_driver_platdata *ddata,
+ void __iomem *reg, const u32 mask, bool clr,
+ bool busywait)
{
+ u64 timeout_us = CQSPI_TIMEOUT_MS * USEC_PER_MSEC;
u32 val;

+ if (busywait && ddata && ddata->quirks & CQSPI_BUSYWAIT_EARLY) {
+ int ret = readl_relaxed_poll_timeout(reg, val,
+ (((clr ? ~val : val) & mask) == mask),
+ 0, CQSPI_BUSYWAIT_TIMEOUT_US);
+
+ if (ret != -ETIMEDOUT)
+ return ret;
+
+ timeout_us -= CQSPI_BUSYWAIT_TIMEOUT_US;
+ }
+
return readl_relaxed_poll_timeout(reg, val,
(((clr ? ~val : val) & mask) == mask),
- 10, CQSPI_TIMEOUT_MS * 1000);
+ 10, timeout_us);
}

static bool cqspi_is_idle(struct cqspi_st *cqspi)
@@ -435,8 +451,8 @@ static int cqspi_exec_flash_cmd(struct cqspi_st *cqspi, unsigned int reg)
writel(reg, reg_base + CQSPI_REG_CMDCTRL);

/* Polling for completion. */
- ret = cqspi_wait_for_bit(reg_base + CQSPI_REG_CMDCTRL,
- CQSPI_REG_CMDCTRL_INPROGRESS_MASK, 1);
+ ret = cqspi_wait_for_bit(cqspi->ddata, reg_base + CQSPI_REG_CMDCTRL,
+ CQSPI_REG_CMDCTRL_INPROGRESS_MASK, 1, true);
if (ret) {
dev_err(&cqspi->pdev->dev,
"Flash command execution timed out.\n");
@@ -791,8 +807,8 @@ static int cqspi_indirect_read_execute(struct cqspi_flash_pdata *f_pdata,
}

/* Check indirect done status */
- ret = cqspi_wait_for_bit(reg_base + CQSPI_REG_INDIRECTRD,
- CQSPI_REG_INDIRECTRD_DONE_MASK, 0);
+ ret = cqspi_wait_for_bit(cqspi->ddata, reg_base + CQSPI_REG_INDIRECTRD,
+ CQSPI_REG_INDIRECTRD_DONE_MASK, 0, true);
if (ret) {
dev_err(dev, "Indirect read completion error (%i)\n", ret);
goto failrd;
@@ -1092,8 +1108,8 @@ static int cqspi_indirect_write_execute(struct cqspi_flash_pdata *f_pdata,
}

/* Check indirect done status */
- ret = cqspi_wait_for_bit(reg_base + CQSPI_REG_INDIRECTWR,
- CQSPI_REG_INDIRECTWR_DONE_MASK, 0);
+ ret = cqspi_wait_for_bit(cqspi->ddata, reg_base + CQSPI_REG_INDIRECTWR,
+ CQSPI_REG_INDIRECTWR_DONE_MASK, 0, false);
if (ret) {
dev_err(dev, "Indirect write completion error (%i)\n", ret);
goto failwr;

--
2.44.0

2024-04-05 15:05:58

by Théo Lebrun

Subject: [PATCH v2 09/11] spi: cadence-qspi: add mobileye,eyeq5-ospi compatible

Declare a new mobileye,eyeq5-ospi compatible. Exploit quirk flags:
detect FIFO depth through SRAMPARTITION register; avoid IRQs during
read operations.

Signed-off-by: Théo Lebrun <[email protected]>
---
drivers/spi/spi-cadence-quadspi.c | 11 +++++++++++
1 file changed, 11 insertions(+)

diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
index 230aad490e03..11f54f507787 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -2059,6 +2059,13 @@ static const struct cqspi_driver_platdata pensando_cdns_qspi = {
.quirks = CQSPI_NEEDS_APB_AHB_HAZARD_WAR | CQSPI_DISABLE_DAC_MODE,
};

+static const struct cqspi_driver_platdata mobileye_eyeq5_ospi = {
+ .hwcaps_mask = CQSPI_SUPPORTS_OCTAL,
+ .quirks = CQSPI_DISABLE_DAC_MODE | CQSPI_NO_SUPPORT_WR_COMPLETION |
+ CQSPI_DETECT_FIFO_DEPTH | CQSPI_RD_NO_IRQ |
+ CQSPI_BUSYWAIT_EARLY,
+};
+
static const struct of_device_id cqspi_dt_ids[] = {
{
.compatible = "cdns,qspi-nor",
@@ -2092,6 +2099,10 @@ static const struct of_device_id cqspi_dt_ids[] = {
.compatible = "amd,pensando-elba-qspi",
.data = &pensando_cdns_qspi,
},
+ {
+ .compatible = "mobileye,eyeq5-ospi",
+ .data = &mobileye_eyeq5_ospi,
+ },
{ /* end of table */ }
};


--
2.44.0

2024-04-05 15:06:15

by Théo Lebrun

Subject: [PATCH v2 11/11] MIPS: mobileye: eyeq5: add octal flash node to eval board DTS

Add SPI-NOR octal flash node to evaluation board devicetree.

Signed-off-by: Théo Lebrun <[email protected]>
---
arch/mips/boot/dts/mobileye/eyeq5-epm5.dts | 15 +++++++++++++++
1 file changed, 15 insertions(+)

diff --git a/arch/mips/boot/dts/mobileye/eyeq5-epm5.dts b/arch/mips/boot/dts/mobileye/eyeq5-epm5.dts
index 6898b2d8267d..0e5fee7b680c 100644
--- a/arch/mips/boot/dts/mobileye/eyeq5-epm5.dts
+++ b/arch/mips/boot/dts/mobileye/eyeq5-epm5.dts
@@ -21,3 +21,18 @@ memory@0 {
<0x8 0x02000000 0x0 0x7E000000>;
};
};
+
+&ospi {
+ flash0: flash@0 {
+ compatible = "jedec,spi-nor";
+ reg = <0>; /* chip select */
+
+ spi-max-frequency = <40000000>;
+ spi-rx-bus-width = <8>;
+ cdns,read-delay = <1>;
+ cdns,tshsl-ns = <400>;
+ cdns,tsd2d-ns = <400>;
+ cdns,tchsh-ns = <125>;
+ cdns,tslch-ns = <50>;
+ };
+};

--
2.44.0

2024-04-05 15:11:34

by Théo Lebrun

Subject: [PATCH v2 10/11] MIPS: mobileye: eyeq5: Add SPI-NOR controller node

Add Cadence Quad SPI controller node to EyeQ5 SoC devicetree.
Octal is supported.

Signed-off-by: Théo Lebrun <[email protected]>
---
arch/mips/boot/dts/mobileye/eyeq5.dtsi | 15 +++++++++++++++
1 file changed, 15 insertions(+)

diff --git a/arch/mips/boot/dts/mobileye/eyeq5.dtsi b/arch/mips/boot/dts/mobileye/eyeq5.dtsi
index 8d4f65ec912d..1543c2b9bcb6 100644
--- a/arch/mips/boot/dts/mobileye/eyeq5.dtsi
+++ b/arch/mips/boot/dts/mobileye/eyeq5.dtsi
@@ -158,6 +158,21 @@ timer {
clocks = <&core0_clk>;
};
};
+
+ ospi: spi@2100000 {
+ compatible = "mobileye,eyeq5-ospi", "cdns,qspi-nor";
+ reg = <0 0x2100000 0x0 0x1000>,
+ <0 0x10000000 0x0 0x8000000>;
+ interrupt-parent = <&gic>;
+ interrupts = <GIC_SHARED 20 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&clocks EQ5C_DIV_OSPI>;
+ assigned-clocks = <&clocks EQ5C_DIV_OSPI>;
+ assigned-clock-rates = <167000000>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ cdns,fifo-width = <4>;
+ cdns,trigger-address = <0x00000000>;
+ };
};
};


--
2.44.0

2024-04-05 15:44:36

by Théo Lebrun

Subject: Re: [PATCH v2 00/11] spi: cadence-qspi: add Mobileye EyeQ5 support

On Fri Apr 5, 2024 at 5:02 PM CEST, Théo Lebrun wrote:

[...]

> Changes in v2:
> - Rebase upon v6.9-rc2.
> - Fix dt-bindings commit subject tags.
> - Take Reviewed-by: Krzysztof Kozlowski on dt-bindings commit.
> - Add dt-bindings commit to order compatibles alphabetically.
> adding EyeQ5 compatible can be taken alone easily.
> - Drop patch taken upstream:
> - Add To: Rob Herring, following get_maintainer.pl recommendation.
> - Link to v1: https://lore.kernel.org/r/[email protected]
>
> Krzysztof: unsure if you want this. It is second so that commit
> spi: cadence-qspi: switch from legacy names to modern ones

Sorry for the weird formatting; b4 saw those two lines as trailers and
moved them last I guess. Proper formatting is:

Changes in v2:
- Rebase upon v6.9-rc2.
- Fix dt-bindings commit subject tags.
- Take Reviewed-by: Krzysztof Kozlowski on dt-bindings commit.
- Add dt-bindings commit to order compatibles alphabetically.
Krzysztof: unsure if you want this. It is second so that commit
adding EyeQ5 compatible can be taken alone easily.
- Drop patch taken upstream:
spi: cadence-qspi: switch from legacy names to modern ones
- Add To: Rob Herring, following get_maintainer.pl recommendation.
- Link to v1: https://lore.kernel.org/r/[email protected]

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

2024-04-06 11:38:39

by Krzysztof Kozlowski

Subject: Re: [PATCH v2 02/11] spi: dt-bindings: cdns,qspi-nor: sort compatibles alphabetically

On 05/04/2024 17:02, Théo Lebrun wrote:
> Compatibles are ordered by date of addition.
> Switch to (deterministic) alphabetical ordering.
>
> Signed-off-by: Théo Lebrun <[email protected]>
> ---
> Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Krzysztof Kozlowski <[email protected]>

Best regards,
Krzysztof

2024-04-08 14:10:45

by Mark Brown

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

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?

Attachments:

2024-04-08 14:14:44

by Mark Brown

Subject: Re: [PATCH v2 02/11] spi: dt-bindings: cdns,qspi-nor: sort compatibles alphabetically

On Fri, Apr 05, 2024 at 05:02:12PM +0200, Th?o Lebrun wrote:
> Compatibles are ordered by date of addition.
> Switch to (deterministic) alphabetical ordering.
>
> Signed-off-by: Th?o Lebrun <[email protected]>
> ---
> Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
> index 5509c126b1cf..e53d443c6f93 100644
> --- a/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
> +++ b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
> @@ -79,13 +79,13 @@ properties:
> - items:
> - enum:
> - amd,pensando-elba-qspi
> - - mobileye,eyeq5-ospi
> - - ti,k2g-qspi
> - - ti,am654-ospi
> - intel,lgm-qspi
> - - xlnx,versal-ospi-1.0
> - intel,socfpga-qspi
> + - mobileye,eyeq5-ospi
> - starfive,jh7110-qspi
> + - ti,am654-ospi
> + - ti,k2g-qspi
> + - xlnx,versal-ospi-1.0

In general it's better to sort trivial cleanup patches like this before
new functionality in order to avoid spurious dependencies.

Attachments:

2024-04-08 14:16:56

by Mark Brown

Subject: Re: [PATCH v2 08/11] spi: cadence-qspi: add early busywait to cqspi_wait_for_bit()

On Fri, Apr 05, 2024 at 05:02:18PM +0200, Th?o Lebrun wrote:

> If the CQSPI_BUSYWAIT_EARLY quirk flag is on, call
> readl_relaxed_poll_timeout() with no sleep at the start of
> cqspi_wait_for_bit(). If its short timeout expires, a sleeping
> readl_relaxed_poll_timeout() call takes the relay.
>
> Behavior is hidden behind a quirk flag to keep the previous behavior the
> same on all platforms.
>
> The reason is to avoid hrtimer interrupts on the system. All read
> operations take less than 100?s.

Why would this be platform specific, this seems like a very standard
optimisation technique?

Attachments:

2024-04-08 14:42:58

by Théo Lebrun

Subject: Re: [PATCH v2 08/11] spi: cadence-qspi: add early busywait to cqspi_wait_for_bit()

Hello,

On Mon Apr 8, 2024 at 4:16 PM CEST, Mark Brown wrote:
> On Fri, Apr 05, 2024 at 05:02:18PM +0200, Théo Lebrun wrote:
>
> > If the CQSPI_BUSYWAIT_EARLY quirk flag is on, call
> > readl_relaxed_poll_timeout() with no sleep at the start of
> > cqspi_wait_for_bit(). If its short timeout expires, a sleeping
> > readl_relaxed_poll_timeout() call takes the relay.
> >
> > Behavior is hidden behind a quirk flag to keep the previous behavior the
> > same on all platforms.
> >
> > The reason is to avoid hrtimer interrupts on the system. All read
> > operations take less than 100µs.
>
> Why would this be platform specific, this seems like a very standard
> optimisation technique?

It does not make sense if you know that all read operations take more
than 100µs. I preferred being conservative. If you confirm it makes
sense I'll remove the quirk.

Regards,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

2024-04-08 14:46:05

by Théo Lebrun

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

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

2024-04-08 15:11:02

by Théo Lebrun

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

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

2024-04-08 15:12:04

by Théo Lebrun

Subject: Re: [PATCH v2 02/11] spi: dt-bindings: cdns,qspi-nor: sort compatibles alphabetically

Hello,

On Mon Apr 8, 2024 at 4:14 PM CEST, Mark Brown wrote:
> On Fri, Apr 05, 2024 at 05:02:12PM +0200, Théo Lebrun wrote:
> > Compatibles are ordered by date of addition.
> > Switch to (deterministic) alphabetical ordering.
> >
> > Signed-off-by: Théo Lebrun <[email protected]>
> > ---
> > Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
> > index 5509c126b1cf..e53d443c6f93 100644
> > --- a/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
> > +++ b/Documentation/devicetree/bindings/spi/cdns,qspi-nor.yaml
> > @@ -79,13 +79,13 @@ properties:
> > - items:
> > - enum:
> > - amd,pensando-elba-qspi
> > - - mobileye,eyeq5-ospi
> > - - ti,k2g-qspi
> > - - ti,am654-ospi
> > - intel,lgm-qspi
> > - - xlnx,versal-ospi-1.0
> > - intel,socfpga-qspi
> > + - mobileye,eyeq5-ospi
> > - starfive,jh7110-qspi
> > + - ti,am654-ospi
> > + - ti,k2g-qspi
> > + - xlnx,versal-ospi-1.0
>
> In general it's better to sort trivial cleanup patches like this before
> new functionality in order to avoid spurious dependencies.

It wasn't clear to me if this patch was desired. I therefore put it
afterwards to avoid conflicts if "spi: dt-bindings: cdns,qspi-nor: add
mobileye,eyeq5-ospi compatible" was applied.

Now that I know it is desired, I'll move it first in the series.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

2024-04-08 15:22:01

by Mark Brown

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

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?

Attachments:

2024-04-08 17:15:17

by Mark Brown

Subject: Re: [PATCH v2 08/11] spi: cadence-qspi: add early busywait to cqspi_wait_for_bit()

On Mon, Apr 08, 2024 at 04:42:43PM +0200, Th?o Lebrun wrote:
> On Mon Apr 8, 2024 at 4:16 PM CEST, Mark Brown wrote:
> > On Fri, Apr 05, 2024 at 05:02:18PM +0200, Th?o Lebrun wrote:

> > > The reason is to avoid hrtimer interrupts on the system. All read
> > > operations take less than 100?s.

> > Why would this be platform specific, this seems like a very standard
> > optimisation technique?

> It does not make sense if you know that all read operations take more
> than 100?s. I preferred being conservative. If you confirm it makes
> sense I'll remove the quirk.

It does seem plausible at least, and the time could be made a tuneable
with quirks or otherwise if that's needed. I think I'd expect the MIPS
platform you're working with to be towards the lower end of performance
for systems that are new enough to have this hardware.

Attachments:

2024-04-08 18:33:48

by Mark Brown

Subject: Re: (subset) [PATCH v2 00/11] spi: cadence-qspi: add Mobileye EyeQ5 support

On Fri, 05 Apr 2024 17:02:10 +0200, Théo Lebrun wrote:
> V2 of this series adding octal SPI-NOR support to Mobileye EyeQ5
> platform. It has been tested on EyeQ5 hardware successfully.
> V1 cover letter [5] contains a brief summary of what gets added.
>
> There is no dependency except if you want zero errors in devicetree:
> system-controller series [3] for <&clocks> phandle.
>
> [...]

Applied to

https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[03/11] spi: cadence-qspi: allow building for MIPS
commit: 708eafeba9eec51c5bde8efef2a7c22d7113b771
[04/11] spi: cadence-qspi: store device data pointer in private struct
commit: dcc594aef1bf3a6a49b77ad2c0348d894b7cd956
[06/11] spi: cadence-qspi: minimise register accesses on each op if !DTR
commit: 563f8598cbc246a81d256e0e888dc085504caa90
[07/11] spi: cadence-qspi: add no-IRQ mode to indirect reads
(no commit info)

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

2024-04-09 10:08:45

by Théo Lebrun

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

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

2024-04-09 10:09:36

by Théo Lebrun

Subject: Re: [PATCH v2 08/11] spi: cadence-qspi: add early busywait to cqspi_wait_for_bit()

Hello,

On Mon Apr 8, 2024 at 6:40 PM CEST, Mark Brown wrote:
> On Mon, Apr 08, 2024 at 04:42:43PM +0200, Théo Lebrun wrote:
> > On Mon Apr 8, 2024 at 4:16 PM CEST, Mark Brown wrote:
> > > On Fri, Apr 05, 2024 at 05:02:18PM +0200, Théo Lebrun wrote:
>
> > > > The reason is to avoid hrtimer interrupts on the system. All read
> > > > operations take less than 100µs.
>
> > > Why would this be platform specific, this seems like a very standard
> > > optimisation technique?
>
> > It does not make sense if you know that all read operations take more
> > than 100µs. I preferred being conservative. If you confirm it makes
> > sense I'll remove the quirk.
>
> It does seem plausible at least, and the time could be made a tuneable
> with quirks or otherwise if that's needed. I think I'd expect the MIPS
> platform you're working with to be towards the lower end of performance
> for systems that are new enough to have this hardware.

Next revision will do the same busywait behavior unconditionally then.

Thanks!

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

2024-04-09 15:54:31

by Mark Brown

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

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.

Attachments: