[PATCH 0/4] spi: cadence-quadspi: correct chip-select logic

[PATCH 0/4] spi: cadence-quadspi: correct chip-select logic

[Théo Lebrun]

Hi,

Here are three independent patches that relate to the handling of
chip-select and the number of those in the spi-cadence-quadspi.c
driver.

 - First one is about checking each flash node reg (ie CS) against the
   ->num_chipselect value instead of the hardcoded max constant. That
   means it checks against the num-cs DT prop if it existed. Previously
   num-cs==1 with 2 flash nodes would have lead to no error,
   a ->num_chipselect==1 and 2 flashes.

 - Second, we lower the max CS constant from 16 to 4. The hardware only
   supports 4 anyway, and that makes for less memory used. This got
   discovered on v6.8-rc2 when the SPI subsystem imposed a max CS of 4.
   The change got reverted later.

 - Lastly, we adjust the ->num_chipselect value reported to the actual
   number of chip-selects. Previously, it reported either the num-cs DT
   prop or the max value (if no num-cs was provided).

There is also a small fix to move to modern names and avoid using the
legacy compatibility layer (slave, etc).

Thanks,
Théo

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
Théo Lebrun (4):
      spi: cadence-qspi: assert each subnode flash CS is valid
      spi: cadence-qspi: set maximum chip-select to 4
      spi: cadence-qspi: report correct number of chip-select
      spi: cadence-qspi: switch from legacy names to modern ones

 drivers/spi/spi-cadence-quadspi.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)
---
base-commit: 19b50f80b3a4865bd477aa5c026dd234d39a50d2
change-id: 20240209-cdns-qspi-cs-621bfe7f327f

Best regards,
-- 
Théo Lebrun <theo.lebrun@bootlin.com>

[PATCH 1/4] spi: cadence-qspi: assert each subnode flash CS is valid

[Théo Lebrun]

Check each flash CS against the num-cs property from devicetree.
Fallback to the driver max supported value (CQSPI_MAX_CHIPSELECT) if
num-cs isn't present.

cqspi->num_chipselect is set in cqspi_of_get_pdata() to the num-cs
devicetree property, or to CQSPI_MAX_CHIPSELECT if num-cs is not set.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/spi/spi-cadence-quadspi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
index d44a0c501879..7ba4d5d16fd2 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -1635,7 +1635,7 @@ static int cqspi_setup_flash(struct cqspi_st *cqspi)
 			return ret;
 		}
 
-		if (cs >= CQSPI_MAX_CHIPSELECT) {
+		if (cs >= cqspi->num_chipselect) {
 			dev_err(dev, "Chip select %d out of range.\n", cs);
 			of_node_put(np);
 			return -EINVAL;

-- 
2.43.0

Re: [PATCH 1/4] spi: cadence-qspi: assert each subnode flash CS is valid

[Dhruva Gole]

Hi,

On Feb 09, 2024 at 14:45:30 +0100, Théo Lebrun wrote:
> Check each flash CS against the num-cs property from devicetree.
> Fallback to the driver max supported value (CQSPI_MAX_CHIPSELECT) if
> num-cs isn't present.
> 
> cqspi->num_chipselect is set in cqspi_of_get_pdata() to the num-cs
> devicetree property, or to CQSPI_MAX_CHIPSELECT if num-cs is not set.

Makes sense,

> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  drivers/spi/spi-cadence-quadspi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> index d44a0c501879..7ba4d5d16fd2 100644
> --- a/drivers/spi/spi-cadence-quadspi.c
> +++ b/drivers/spi/spi-cadence-quadspi.c
> @@ -1635,7 +1635,7 @@ static int cqspi_setup_flash(struct cqspi_st *cqspi)
>  			return ret;
>  		}
>  
> -		if (cs >= CQSPI_MAX_CHIPSELECT) {
> +		if (cs >= cqspi->num_chipselect) {

Reviewed-by: Dhruva Gole <d-gole@ti.com>


-- 
Best regards,
Dhruva Gole <d-gole@ti.com>

[PATCH 2/4] spi: cadence-qspi: set maximum chip-select to 4

[Théo Lebrun]

Change the maximum chip-select count in cadence-qspi to 4 instead of 16.
The value gets used as default ->num_chipselect when the num-cs DT
property isn't received from devicetree. It also determines the
cqspi->f_pdata array size.

Hardware only supports values up to 4; see cqspi_chipselect() that sets
CS using a one-bit-per-CS 4-bit register field.

Add a static_assert() call as a defensive measure to ensure we stay
under the SPI subsystem limit. It got set to 4 when introduced in
4d8ff6b0991d ("spi: Add multi-cs memories support in SPI core") and
later increased to 16 in 2f8c7c3715f2 ("spi: Raise limit on number of
chip selects").

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

diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
index 7ba4d5d16fd2..e9e3abd2142c 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -31,7 +31,9 @@
 #include <linux/timer.h>
 
 #define CQSPI_NAME			"cadence-qspi"
-#define CQSPI_MAX_CHIPSELECT		16
+#define CQSPI_MAX_CHIPSELECT		4
+
+static_assert(CQSPI_MAX_CHIPSELECT <= SPI_CS_CNT_MAX);
 
 /* Quirks */
 #define CQSPI_NEEDS_WR_DELAY		BIT(0)

-- 
2.43.0

[PATCH 3/4] spi: cadence-qspi: report correct number of chip-select

[Théo Lebrun]

Set the ->num_chipselect field in struct cqspi_st and struct
spi_controller to the current number of chip-select. The value is
dependent on declared flashes in devicetree.

Previously, the num-cs property from devicetree or the maximum value was
being reported.

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

diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
index e9e3abd2142c..895c950e7fd6 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -1621,6 +1621,7 @@ static const struct spi_controller_mem_caps cqspi_mem_caps = {
 
 static int cqspi_setup_flash(struct cqspi_st *cqspi)
 {
+	unsigned int max_cs = cqspi->num_chipselect - 1;
 	struct platform_device *pdev = cqspi->pdev;
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
@@ -1641,6 +1642,8 @@ static int cqspi_setup_flash(struct cqspi_st *cqspi)
 			dev_err(dev, "Chip select %d out of range.\n", cs);
 			of_node_put(np);
 			return -EINVAL;
+		} else if (cs < max_cs) {
+			max_cs = cs;
 		}
 
 		f_pdata = &cqspi->f_pdata[cs];
@@ -1654,6 +1657,7 @@ static int cqspi_setup_flash(struct cqspi_st *cqspi)
 		}
 	}
 
+	cqspi->num_chipselect = max_cs + 1;
 	return 0;
 }
 
@@ -1865,14 +1869,14 @@ static int cqspi_probe(struct platform_device *pdev)
 	cqspi->current_cs = -1;
 	cqspi->sclk = 0;
 
-	host->num_chipselect = cqspi->num_chipselect;
-
 	ret = cqspi_setup_flash(cqspi);
 	if (ret) {
 		dev_err(dev, "failed to setup flash parameters %d\n", ret);
 		goto probe_setup_failed;
 	}
 
+	host->num_chipselect = cqspi->num_chipselect;
+
 	if (cqspi->use_direct_mode) {
 		ret = cqspi_request_mmap_dma(cqspi);
 		if (ret == -EPROBE_DEFER)

-- 
2.43.0

[PATCH 4/4] spi: cadence-qspi: switch from legacy names to modern ones

[Théo Lebrun]

Both spi_master_get_devdata() and the ->master field in struct
spi_device are part of the compatibility layer provided by
<linux/spi/spi.h>. Switch away from them.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/spi/spi-cadence-quadspi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
index 895c950e7fd6..7ae3b2329089 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -1412,7 +1412,7 @@ static int cqspi_mem_process(struct spi_mem *mem, const struct spi_mem_op *op)
 static int cqspi_exec_mem_op(struct spi_mem *mem, const struct spi_mem_op *op)
 {
 	int ret;
-	struct cqspi_st *cqspi = spi_master_get_devdata(mem->spi->master);
+	struct cqspi_st *cqspi = spi_controller_get_devdata(mem->spi->controller);
 	struct device *dev = &cqspi->pdev->dev;
 
 	ret = pm_runtime_resume_and_get(dev);

-- 
2.43.0

Re: [PATCH 0/4] spi: cadence-quadspi: correct chip-select logic

[Mark Brown]

On Fri, 09 Feb 2024 14:45:29 +0100, Théo Lebrun wrote:
> Here are three independent patches that relate to the handling of
> chip-select and the number of those in the spi-cadence-quadspi.c
> driver.
> 
>  - First one is about checking each flash node reg (ie CS) against the
>    ->num_chipselect value instead of the hardcoded max constant. That
>    means it checks against the num-cs DT prop if it existed. Previously
>    num-cs==1 with 2 flash nodes would have lead to no error,
>    a ->num_chipselect==1 and 2 flashes.
> 
> [...]

Applied to

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

Thanks!

[1/4] spi: cadence-qspi: assert each subnode flash CS is valid
      commit: 0d62c64a8e48438545dcef7e5d2f4839ff5cfe4c
[2/4] spi: cadence-qspi: set maximum chip-select to 4
      commit: 7cc3522aedb5f4360c4502b2e89b279b7aa94ceb
[3/4] spi: cadence-qspi: report correct number of chip-select
      commit: 0f3841a5e1152eca1a58cfbd9ceb6d311aa7e647
[4/4] spi: cadence-qspi: switch from legacy names to modern ones
      (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