public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/4] Miscellaneous fixes and clean-ups
@ 2025-09-04 13:31 Santhosh Kumar K
  2025-09-04 13:31 ` [PATCH 1/4] spi: cadence-quadspi: Flush posted register writes before INDAC access Santhosh Kumar K
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Santhosh Kumar K @ 2025-09-04 13:31 UTC (permalink / raw)
  To: miquel.raynal, broonie, vigneshr, marex, computersforpeace,
	grmoore, theo.lebrun
  Cc: linux-spi, linux-kernel, s-k6, praneeth, p-mantena, a-dutta,
	u-kumar1

This series introduces some small but important fixes and cleanups in
the Cadence QSPI Controller.

Tested on TI's AM62A SK and AM62P SK:
Logs: https://gist.github.com/santhosh21/0d25767b58d9a1d9624f2c502dd8f36b

Signed-off-by: Santhosh Kumar K s-k6@ti.com

Pratyush Yadav (2):
  spi: cadence-quadspi: Flush posted register writes before INDAC access
  spi: cadence-quadspi: Flush posted register writes before DAC access

Santhosh Kumar K (1):
  spi: cadence-quadspi: Fix cqspi_setup_flash()

Vignesh Raghavendra (1):
  spi: cadence-quadspi: Use BIT() macros where possible

 drivers/spi/spi-cadence-quadspi.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

-- 
2.34.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/4] spi: cadence-quadspi: Flush posted register writes before INDAC access
  2025-09-04 13:31 [PATCH 0/4] Miscellaneous fixes and clean-ups Santhosh Kumar K
@ 2025-09-04 13:31 ` Santhosh Kumar K
  2025-09-04 14:35   ` Pratyush Yadav
  2025-09-04 13:31 ` [PATCH 2/4] spi: cadence-quadspi: Flush posted register writes before DAC access Santhosh Kumar K
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 13+ messages in thread
From: Santhosh Kumar K @ 2025-09-04 13:31 UTC (permalink / raw)
  To: miquel.raynal, broonie, vigneshr, marex, computersforpeace,
	grmoore, theo.lebrun
  Cc: linux-spi, linux-kernel, s-k6, praneeth, p-mantena, a-dutta,
	u-kumar1, Pratyush Yadav, stable

From: Pratyush Yadav <pratyush@kernel.org>

cqspi_indirect_read_execute() and cqspi_indirect_write_execute() first
set the enable bit on APB region and then start reading/writing to the
AHB region. On TI K3 SoCs these regions lie on different endpoints. This
means that the order of the two operations is not guaranteed, and they
might be reordered at the interconnect level.

It is possible for the AHB write to be executed before the APB write to
enable the indirect controller, causing the transaction to be invalid
and the write erroring out. Read back the APB region write before
accessing the AHB region to make sure the write got flushed and the race
condition is eliminated.

Fixes: 140623410536 ("mtd: spi-nor: Add driver for Cadence Quad SPI Flash Controller")
CC: stable@vger.kernel.org
Signed-off-by: Pratyush Yadav <pratyush@kernel.org>
Signed-off-by: Santhosh Kumar K <s-k6@ti.com>
---
 drivers/spi/spi-cadence-quadspi.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
index 9bf823348cd3..eaf9a0f522d5 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -764,6 +764,7 @@ static int cqspi_indirect_read_execute(struct cqspi_flash_pdata *f_pdata,
 	reinit_completion(&cqspi->transfer_complete);
 	writel(CQSPI_REG_INDIRECTRD_START_MASK,
 	       reg_base + CQSPI_REG_INDIRECTRD);
+	readl(reg_base + CQSPI_REG_INDIRECTRD); /* Flush posted write. */
 
 	while (remaining > 0) {
 		if (use_irq &&
@@ -1090,6 +1091,8 @@ static int cqspi_indirect_write_execute(struct cqspi_flash_pdata *f_pdata,
 	reinit_completion(&cqspi->transfer_complete);
 	writel(CQSPI_REG_INDIRECTWR_START_MASK,
 	       reg_base + CQSPI_REG_INDIRECTWR);
+	readl(reg_base + CQSPI_REG_INDIRECTWR); /* Flush posted write. */
+
 	/*
 	 * As per 66AK2G02 TRM SPRUHY8F section 11.15.5.3 Indirect Access
 	 * Controller programming sequence, couple of cycles of
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/4] spi: cadence-quadspi: Flush posted register writes before DAC access
  2025-09-04 13:31 [PATCH 0/4] Miscellaneous fixes and clean-ups Santhosh Kumar K
  2025-09-04 13:31 ` [PATCH 1/4] spi: cadence-quadspi: Flush posted register writes before INDAC access Santhosh Kumar K
@ 2025-09-04 13:31 ` Santhosh Kumar K
  2025-09-04 14:36   ` Pratyush Yadav
  2025-09-04 13:31 ` [PATCH 3/4] spi: cadence-quadspi: Fix cqspi_setup_flash() Santhosh Kumar K
  2025-09-04 13:31 ` [PATCH 4/4] spi: cadence-quadspi: Use BIT() macros where possible Santhosh Kumar K
  3 siblings, 1 reply; 13+ messages in thread
From: Santhosh Kumar K @ 2025-09-04 13:31 UTC (permalink / raw)
  To: miquel.raynal, broonie, vigneshr, marex, computersforpeace,
	grmoore, theo.lebrun
  Cc: linux-spi, linux-kernel, s-k6, praneeth, p-mantena, a-dutta,
	u-kumar1, Pratyush Yadav, stable

From: Pratyush Yadav <pratyush@kernel.org>

cqspi_read_setup() and cqspi_write_setup() program the address width as
the last step in the setup. This is likely to be immediately followed by
a DAC region read/write. On TI K3 SoCs the DAC region is on a different
endpoint from the register region. This means that the order of the two
operations is not guaranteed, and they might be reordered at the
interconnect level. It is possible that the DAC read/write goes through
before the address width update goes through. In this situation if the
previous command used a different address width the OSPI command is sent
with the wrong number of address bytes, resulting in an invalid command
and undefined behavior.

Read back the size register to make sure the write gets flushed before
accessing the DAC region.

Fixes: 140623410536 ("mtd: spi-nor: Add driver for Cadence Quad SPI Flash Controller")
CC: stable@vger.kernel.org
Signed-off-by: Pratyush Yadav <pratyush@kernel.org>
Signed-off-by: Santhosh Kumar K <s-k6@ti.com>
---
 drivers/spi/spi-cadence-quadspi.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
index eaf9a0f522d5..447a32a08a93 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -719,6 +719,7 @@ static int cqspi_read_setup(struct cqspi_flash_pdata *f_pdata,
 	reg &= ~CQSPI_REG_SIZE_ADDRESS_MASK;
 	reg |= (op->addr.nbytes - 1);
 	writel(reg, reg_base + CQSPI_REG_SIZE);
+	readl(reg_base + CQSPI_REG_SIZE); /* Flush posted write. */
 	return 0;
 }
 
@@ -1063,6 +1064,7 @@ static int cqspi_write_setup(struct cqspi_flash_pdata *f_pdata,
 	reg &= ~CQSPI_REG_SIZE_ADDRESS_MASK;
 	reg |= (op->addr.nbytes - 1);
 	writel(reg, reg_base + CQSPI_REG_SIZE);
+	readl(reg_base + CQSPI_REG_SIZE); /* Flush posted write. */
 	return 0;
 }
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 3/4] spi: cadence-quadspi: Fix cqspi_setup_flash()
  2025-09-04 13:31 [PATCH 0/4] Miscellaneous fixes and clean-ups Santhosh Kumar K
  2025-09-04 13:31 ` [PATCH 1/4] spi: cadence-quadspi: Flush posted register writes before INDAC access Santhosh Kumar K
  2025-09-04 13:31 ` [PATCH 2/4] spi: cadence-quadspi: Flush posted register writes before DAC access Santhosh Kumar K
@ 2025-09-04 13:31 ` Santhosh Kumar K
  2025-09-04 14:41   ` Pratyush Yadav
  2025-09-04 15:32   ` Théo Lebrun
  2025-09-04 13:31 ` [PATCH 4/4] spi: cadence-quadspi: Use BIT() macros where possible Santhosh Kumar K
  3 siblings, 2 replies; 13+ messages in thread
From: Santhosh Kumar K @ 2025-09-04 13:31 UTC (permalink / raw)
  To: miquel.raynal, broonie, vigneshr, marex, computersforpeace,
	grmoore, theo.lebrun
  Cc: linux-spi, linux-kernel, s-k6, praneeth, p-mantena, a-dutta,
	u-kumar1

The 'max_cs' stores the largest chip select number. It should only
be updated when the current 'cs' is greater than existing 'max_cs'. So,
fix the condition accordingly.

Fixes: 0f3841a5e115 ("spi: cadence-qspi: report correct number of chip-select")
Signed-off-by: Santhosh Kumar K <s-k6@ti.com>
---
 drivers/spi/spi-cadence-quadspi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
index 447a32a08a93..da3ec15abb3e 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -1722,7 +1722,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;
+	unsigned int max_cs = 0;
 	struct platform_device *pdev = cqspi->pdev;
 	struct device *dev = &pdev->dev;
 	struct cqspi_flash_pdata *f_pdata;
@@ -1740,7 +1740,7 @@ static int cqspi_setup_flash(struct cqspi_st *cqspi)
 		if (cs >= cqspi->num_chipselect) {
 			dev_err(dev, "Chip select %d out of range.\n", cs);
 			return -EINVAL;
-		} else if (cs < max_cs) {
+		} else if (cs > max_cs) {
 			max_cs = cs;
 		}
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 4/4] spi: cadence-quadspi: Use BIT() macros where possible
  2025-09-04 13:31 [PATCH 0/4] Miscellaneous fixes and clean-ups Santhosh Kumar K
                   ` (2 preceding siblings ...)
  2025-09-04 13:31 ` [PATCH 3/4] spi: cadence-quadspi: Fix cqspi_setup_flash() Santhosh Kumar K
@ 2025-09-04 13:31 ` Santhosh Kumar K
  2025-09-04 14:49   ` Pratyush Yadav
  3 siblings, 1 reply; 13+ messages in thread
From: Santhosh Kumar K @ 2025-09-04 13:31 UTC (permalink / raw)
  To: miquel.raynal, broonie, vigneshr, marex, computersforpeace,
	grmoore, theo.lebrun
  Cc: linux-spi, linux-kernel, s-k6, praneeth, p-mantena, a-dutta,
	u-kumar1

From: Vignesh Raghavendra <vigneshr@ti.com>

Convert few open coded bit shifts to BIT() macro for better readability.
No functional changes intended.

Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
Signed-off-by: Santhosh Kumar K <s-k6@ti.com>
---
 drivers/spi/spi-cadence-quadspi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
index da3ec15abb3e..b18f095516f2 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -335,7 +335,7 @@ static bool cqspi_is_idle(struct cqspi_st *cqspi)
 {
 	u32 reg = readl(cqspi->iobase + CQSPI_REG_CONFIG);
 
-	return reg & (1UL << CQSPI_REG_CONFIG_IDLE_LSB);
+	return reg & BIT(CQSPI_REG_CONFIG_IDLE_LSB);
 }
 
 static u32 cqspi_get_rd_sram_level(struct cqspi_st *cqspi)
@@ -571,7 +571,7 @@ static int cqspi_command_read(struct cqspi_flash_pdata *f_pdata,
 		reg |= (dummy_clk & CQSPI_REG_CMDCTRL_DUMMY_MASK)
 		     << CQSPI_REG_CMDCTRL_DUMMY_LSB;
 
-	reg |= (0x1 << CQSPI_REG_CMDCTRL_RD_EN_LSB);
+	reg |= BIT(CQSPI_REG_CMDCTRL_RD_EN_LSB);
 
 	/* 0 means 1 byte. */
 	reg |= (((n_rx - 1) & CQSPI_REG_CMDCTRL_RD_BYTES_MASK)
@@ -1191,7 +1191,7 @@ static void cqspi_chipselect(struct cqspi_flash_pdata *f_pdata)
 		 * CS2 to 4b'1011
 		 * CS3 to 4b'0111
 		 */
-		chip_select = 0xF & ~(1 << chip_select);
+		chip_select = 0xF & ~BIT(chip_select);
 	}
 
 	reg &= ~(CQSPI_REG_CONFIG_CHIPSELECT_MASK
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/4] spi: cadence-quadspi: Flush posted register writes before INDAC access
  2025-09-04 13:31 ` [PATCH 1/4] spi: cadence-quadspi: Flush posted register writes before INDAC access Santhosh Kumar K
@ 2025-09-04 14:35   ` Pratyush Yadav
  0 siblings, 0 replies; 13+ messages in thread
From: Pratyush Yadav @ 2025-09-04 14:35 UTC (permalink / raw)
  To: Santhosh Kumar K
  Cc: miquel.raynal, broonie, vigneshr, marex, computersforpeace,
	grmoore, theo.lebrun, linux-spi, linux-kernel, praneeth,
	p-mantena, a-dutta, u-kumar1, Pratyush Yadav, stable

Hi,

On Thu, Sep 04 2025, Santhosh Kumar K wrote:

> From: Pratyush Yadav <pratyush@kernel.org>
>
> cqspi_indirect_read_execute() and cqspi_indirect_write_execute() first
> set the enable bit on APB region and then start reading/writing to the
> AHB region. On TI K3 SoCs these regions lie on different endpoints. This
> means that the order of the two operations is not guaranteed, and they
> might be reordered at the interconnect level.
>
> It is possible for the AHB write to be executed before the APB write to
> enable the indirect controller, causing the transaction to be invalid
> and the write erroring out. Read back the APB region write before
> accessing the AHB region to make sure the write got flushed and the race
> condition is eliminated.
>
> Fixes: 140623410536 ("mtd: spi-nor: Add driver for Cadence Quad SPI Flash Controller")
> CC: stable@vger.kernel.org
> Signed-off-by: Pratyush Yadav <pratyush@kernel.org>
> Signed-off-by: Santhosh Kumar K <s-k6@ti.com>

IIRC I wrote this patch a few years ago when I was still at TI. Nice to
see it being upstreamed! It feels strange to review my own patch, but
FWIW,

Reviewed-by: Pratyush Yadav <pratyush@kernel.org>

[...]

-- 
Regards,
Pratyush Yadav

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/4] spi: cadence-quadspi: Flush posted register writes before DAC access
  2025-09-04 13:31 ` [PATCH 2/4] spi: cadence-quadspi: Flush posted register writes before DAC access Santhosh Kumar K
@ 2025-09-04 14:36   ` Pratyush Yadav
  0 siblings, 0 replies; 13+ messages in thread
From: Pratyush Yadav @ 2025-09-04 14:36 UTC (permalink / raw)
  To: Santhosh Kumar K
  Cc: miquel.raynal, broonie, vigneshr, marex, computersforpeace,
	grmoore, theo.lebrun, linux-spi, linux-kernel, praneeth,
	p-mantena, a-dutta, u-kumar1, Pratyush Yadav, stable

On Thu, Sep 04 2025, Santhosh Kumar K wrote:

> From: Pratyush Yadav <pratyush@kernel.org>
>
> cqspi_read_setup() and cqspi_write_setup() program the address width as
> the last step in the setup. This is likely to be immediately followed by
> a DAC region read/write. On TI K3 SoCs the DAC region is on a different
> endpoint from the register region. This means that the order of the two
> operations is not guaranteed, and they might be reordered at the
> interconnect level. It is possible that the DAC read/write goes through
> before the address width update goes through. In this situation if the
> previous command used a different address width the OSPI command is sent
> with the wrong number of address bytes, resulting in an invalid command
> and undefined behavior.
>
> Read back the size register to make sure the write gets flushed before
> accessing the DAC region.
>
> Fixes: 140623410536 ("mtd: spi-nor: Add driver for Cadence Quad SPI Flash Controller")
> CC: stable@vger.kernel.org
> Signed-off-by: Pratyush Yadav <pratyush@kernel.org>
> Signed-off-by: Santhosh Kumar K <s-k6@ti.com>

Same as the previous,

Reviewed-by: Pratyush Yadav <pratyush@kernel.org>

[...]

-- 
Regards,
Pratyush Yadav

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/4] spi: cadence-quadspi: Fix cqspi_setup_flash()
  2025-09-04 13:31 ` [PATCH 3/4] spi: cadence-quadspi: Fix cqspi_setup_flash() Santhosh Kumar K
@ 2025-09-04 14:41   ` Pratyush Yadav
  2025-09-05 11:04     ` Santhosh Kumar K
  2025-09-04 15:32   ` Théo Lebrun
  1 sibling, 1 reply; 13+ messages in thread
From: Pratyush Yadav @ 2025-09-04 14:41 UTC (permalink / raw)
  To: Santhosh Kumar K
  Cc: miquel.raynal, broonie, vigneshr, marex, computersforpeace,
	grmoore, theo.lebrun, linux-spi, linux-kernel, praneeth,
	p-mantena, a-dutta, u-kumar1

On Thu, Sep 04 2025, Santhosh Kumar K wrote:

> The 'max_cs' stores the largest chip select number. It should only
> be updated when the current 'cs' is greater than existing 'max_cs'. So,
> fix the condition accordingly.
>
> Fixes: 0f3841a5e115 ("spi: cadence-qspi: report correct number of chip-select")
> Signed-off-by: Santhosh Kumar K <s-k6@ti.com>
> ---
>  drivers/spi/spi-cadence-quadspi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> index 447a32a08a93..da3ec15abb3e 100644
> --- a/drivers/spi/spi-cadence-quadspi.c
> +++ b/drivers/spi/spi-cadence-quadspi.c
> @@ -1722,7 +1722,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;
> +	unsigned int max_cs = 0;
>  	struct platform_device *pdev = cqspi->pdev;
>  	struct device *dev = &pdev->dev;
>  	struct cqspi_flash_pdata *f_pdata;
> @@ -1740,7 +1740,7 @@ static int cqspi_setup_flash(struct cqspi_st *cqspi)
>  		if (cs >= cqspi->num_chipselect) {
>  			dev_err(dev, "Chip select %d out of range.\n", cs);
>  			return -EINVAL;
> -		} else if (cs < max_cs) {
> +		} else if (cs > max_cs) {

Makes sense. Out of curiosity, are you using multiple CS in a real use
case or is this only theoretical?

Also nit: this could be simplified to:

		if (cs >= cqspi->num_chipselect) {
			dev_err(dev, "Chip select %d out of range.\n", cs);
			return -EINVAL;
		}

		max_cs = max_t(unsigned int, cs, max_cs);

but I think it is fine either way.

Reviewed-by: Pratyush Yadav <pratyush@kernel.org>

>  			max_cs = cs;
>  		}

-- 
Regards,
Pratyush Yadav

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/4] spi: cadence-quadspi: Use BIT() macros where possible
  2025-09-04 13:31 ` [PATCH 4/4] spi: cadence-quadspi: Use BIT() macros where possible Santhosh Kumar K
@ 2025-09-04 14:49   ` Pratyush Yadav
  2025-09-05 11:04     ` Santhosh Kumar K
  0 siblings, 1 reply; 13+ messages in thread
From: Pratyush Yadav @ 2025-09-04 14:49 UTC (permalink / raw)
  To: Santhosh Kumar K
  Cc: miquel.raynal, broonie, vigneshr, marex, computersforpeace,
	grmoore, theo.lebrun, linux-spi, linux-kernel, praneeth,
	p-mantena, a-dutta, u-kumar1

On Thu, Sep 04 2025, Santhosh Kumar K wrote:

> From: Vignesh Raghavendra <vigneshr@ti.com>
>
> Convert few open coded bit shifts to BIT() macro for better readability.
> No functional changes intended.
>
> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
> Signed-off-by: Santhosh Kumar K <s-k6@ti.com>

I see there are total 7 hits for the pattern "1 <<". Why not convert
them all while you're at it?

> ---
>  drivers/spi/spi-cadence-quadspi.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> index da3ec15abb3e..b18f095516f2 100644
> --- a/drivers/spi/spi-cadence-quadspi.c
> +++ b/drivers/spi/spi-cadence-quadspi.c
> @@ -335,7 +335,7 @@ static bool cqspi_is_idle(struct cqspi_st *cqspi)
>  {
>  	u32 reg = readl(cqspi->iobase + CQSPI_REG_CONFIG);
>  
> -	return reg & (1UL << CQSPI_REG_CONFIG_IDLE_LSB);
> +	return reg & BIT(CQSPI_REG_CONFIG_IDLE_LSB);
>  }
>  
>  static u32 cqspi_get_rd_sram_level(struct cqspi_st *cqspi)
> @@ -571,7 +571,7 @@ static int cqspi_command_read(struct cqspi_flash_pdata *f_pdata,
>  		reg |= (dummy_clk & CQSPI_REG_CMDCTRL_DUMMY_MASK)
>  		     << CQSPI_REG_CMDCTRL_DUMMY_LSB;
>  
> -	reg |= (0x1 << CQSPI_REG_CMDCTRL_RD_EN_LSB);
> +	reg |= BIT(CQSPI_REG_CMDCTRL_RD_EN_LSB);
>  
>  	/* 0 means 1 byte. */
>  	reg |= (((n_rx - 1) & CQSPI_REG_CMDCTRL_RD_BYTES_MASK)
> @@ -1191,7 +1191,7 @@ static void cqspi_chipselect(struct cqspi_flash_pdata *f_pdata)
>  		 * CS2 to 4b'1011
>  		 * CS3 to 4b'0111
>  		 */
> -		chip_select = 0xF & ~(1 << chip_select);
> +		chip_select = 0xF & ~BIT(chip_select);
>  	}
>  
>  	reg &= ~(CQSPI_REG_CONFIG_CHIPSELECT_MASK

-- 
Regards,
Pratyush Yadav

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/4] spi: cadence-quadspi: Fix cqspi_setup_flash()
  2025-09-04 13:31 ` [PATCH 3/4] spi: cadence-quadspi: Fix cqspi_setup_flash() Santhosh Kumar K
  2025-09-04 14:41   ` Pratyush Yadav
@ 2025-09-04 15:32   ` Théo Lebrun
  2025-09-05 11:04     ` Santhosh Kumar K
  1 sibling, 1 reply; 13+ messages in thread
From: Théo Lebrun @ 2025-09-04 15:32 UTC (permalink / raw)
  To: Santhosh Kumar K, miquel.raynal, broonie, vigneshr, marex,
	computersforpeace, grmoore
  Cc: linux-spi, linux-kernel, praneeth, p-mantena, a-dutta, u-kumar1

Hello Santhosh,

On Thu Sep 4, 2025 at 3:31 PM CEST, Santhosh Kumar K wrote:
> The 'max_cs' stores the largest chip select number. It should only
> be updated when the current 'cs' is greater than existing 'max_cs'. So,
> fix the condition accordingly.

Good catch. Current code can only work with one chip-select.

Reviewed-by: Théo Lebrun <theo.lebrun@bootlin.com>

Maybe we should error out if we don't enter the loop, ie if we have no
flash declared?
 - Before your patch, cqspi->num_chipselect was set to num-cs DT prop or
   CQSPI_MAX_CHIPSELECT as fallback.
 - After your patch, cqspi->num_chipselect is set to one.

In neither case do we get an error if no flash is defined in DT.

We could either return some error code or set cqspi->num_chipselect=0
which will lead to spi_register_controller() to fail [0].

[0]: https://elixir.bootlin.com/linux/v6.16.4/source/drivers/spi/spi.c#L3322-L3329

Thanks,

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/4] spi: cadence-quadspi: Fix cqspi_setup_flash()
  2025-09-04 14:41   ` Pratyush Yadav
@ 2025-09-05 11:04     ` Santhosh Kumar K
  0 siblings, 0 replies; 13+ messages in thread
From: Santhosh Kumar K @ 2025-09-05 11:04 UTC (permalink / raw)
  To: Pratyush Yadav
  Cc: miquel.raynal, broonie, vigneshr, marex, computersforpeace,
	grmoore, theo.lebrun, linux-spi, linux-kernel, praneeth,
	p-mantena, a-dutta, u-kumar1, s-k6

Hello,

On 04/09/25 20:11, Pratyush Yadav wrote:
> On Thu, Sep 04 2025, Santhosh Kumar K wrote:
> 
>> The 'max_cs' stores the largest chip select number. It should only
>> be updated when the current 'cs' is greater than existing 'max_cs'. So,
>> fix the condition accordingly.
>>
>> Fixes: 0f3841a5e115 ("spi: cadence-qspi: report correct number of chip-select")
>> Signed-off-by: Santhosh Kumar K <s-k6@ti.com>
>> ---
>>   drivers/spi/spi-cadence-quadspi.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
>> index 447a32a08a93..da3ec15abb3e 100644
>> --- a/drivers/spi/spi-cadence-quadspi.c
>> +++ b/drivers/spi/spi-cadence-quadspi.c
>> @@ -1722,7 +1722,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;
>> +	unsigned int max_cs = 0;
>>   	struct platform_device *pdev = cqspi->pdev;
>>   	struct device *dev = &pdev->dev;
>>   	struct cqspi_flash_pdata *f_pdata;
>> @@ -1740,7 +1740,7 @@ static int cqspi_setup_flash(struct cqspi_st *cqspi)
>>   		if (cs >= cqspi->num_chipselect) {
>>   			dev_err(dev, "Chip select %d out of range.\n", cs);
>>   			return -EINVAL;
>> -		} else if (cs < max_cs) {
>> +		} else if (cs > max_cs) {
> 
> Makes sense. Out of curiosity, are you using multiple CS in a real use
> case or is this only theoretical?

Real use case,  Pratyush - we have both OSPI NOR and QSPI NAND in our 
new AM62Lx EVM - CS0 and CS3 respectively.

> 
> Also nit: this could be simplified to:
> 
> 		if (cs >= cqspi->num_chipselect) {
> 			dev_err(dev, "Chip select %d out of range.\n", cs);
> 			return -EINVAL;
> 		}
> 
> 		max_cs = max_t(unsigned int, cs, max_cs);
> 
> but I think it is fine either way.

Yeah, this one's simpler, I'll go with this. Thanks!

Regards,
Santhosh.

> 
> Reviewed-by: Pratyush Yadav <pratyush@kernel.org>
> 
>>   			max_cs = cs;
>>   		}
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/4] spi: cadence-quadspi: Fix cqspi_setup_flash()
  2025-09-04 15:32   ` Théo Lebrun
@ 2025-09-05 11:04     ` Santhosh Kumar K
  0 siblings, 0 replies; 13+ messages in thread
From: Santhosh Kumar K @ 2025-09-05 11:04 UTC (permalink / raw)
  To: Théo Lebrun, miquel.raynal, broonie, vigneshr, marex,
	computersforpeace, grmoore
  Cc: linux-spi, linux-kernel, praneeth, p-mantena, a-dutta, u-kumar1,
	s-k6

Hello Theo,

On 04/09/25 21:02, Théo Lebrun wrote:
> Hello Santhosh,
> 
> On Thu Sep 4, 2025 at 3:31 PM CEST, Santhosh Kumar K wrote:
>> The 'max_cs' stores the largest chip select number. It should only
>> be updated when the current 'cs' is greater than existing 'max_cs'. So,
>> fix the condition accordingly.
> 
> Good catch. Current code can only work with one chip-select.
> 
> Reviewed-by: Théo Lebrun <theo.lebrun@bootlin.com>
> 
> Maybe we should error out if we don't enter the loop, ie if we have no
> flash declared?
>   - Before your patch, cqspi->num_chipselect was set to num-cs DT prop or
>     CQSPI_MAX_CHIPSELECT as fallback.
>   - After your patch, cqspi->num_chipselect is set to one.
> 
> In neither case do we get an error if no flash is defined in DT.
> 
> We could either return some error code or set cqspi->num_chipselect=0
> which will lead to spi_register_controller() to fail [0].

Yeah, we can use max_cs for this. Initiate max_cs with -1, check if it's 
still negative after the loop part - if yes, then it didn't enter the 
loop as there was no flash declared - return failure. I'll add this 
logic in v2. Thanks!

Regards,
Santhosh.

> 
> [0]: https://elixir.bootlin.com/linux/v6.16.4/source/drivers/spi/spi.c#L3322-L3329
> 
> Thanks,
> 
> --
> Théo Lebrun, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
> 
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/4] spi: cadence-quadspi: Use BIT() macros where possible
  2025-09-04 14:49   ` Pratyush Yadav
@ 2025-09-05 11:04     ` Santhosh Kumar K
  0 siblings, 0 replies; 13+ messages in thread
From: Santhosh Kumar K @ 2025-09-05 11:04 UTC (permalink / raw)
  To: Pratyush Yadav
  Cc: miquel.raynal, broonie, vigneshr, marex, computersforpeace,
	grmoore, theo.lebrun, linux-spi, linux-kernel, praneeth,
	p-mantena, a-dutta, u-kumar1, s-k6



On 04/09/25 20:19, Pratyush Yadav wrote:
> On Thu, Sep 04 2025, Santhosh Kumar K wrote:
> 
>> From: Vignesh Raghavendra <vigneshr@ti.com>
>>
>> Convert few open coded bit shifts to BIT() macro for better readability.
>> No functional changes intended.
>>
>> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
>> Signed-off-by: Santhosh Kumar K <s-k6@ti.com>
> 
> I see there are total 7 hits for the pattern "1 <<". Why not convert
> them all while you're at it?

Yeah, I just checked - I'll convert them as well.

Regards,
Santhosh.

> 
>> ---
>>   drivers/spi/spi-cadence-quadspi.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
>> index da3ec15abb3e..b18f095516f2 100644
>> --- a/drivers/spi/spi-cadence-quadspi.c
>> +++ b/drivers/spi/spi-cadence-quadspi.c
>> @@ -335,7 +335,7 @@ static bool cqspi_is_idle(struct cqspi_st *cqspi)
>>   {
>>   	u32 reg = readl(cqspi->iobase + CQSPI_REG_CONFIG);
>>   
>> -	return reg & (1UL << CQSPI_REG_CONFIG_IDLE_LSB);
>> +	return reg & BIT(CQSPI_REG_CONFIG_IDLE_LSB);
>>   }
>>   
>>   static u32 cqspi_get_rd_sram_level(struct cqspi_st *cqspi)
>> @@ -571,7 +571,7 @@ static int cqspi_command_read(struct cqspi_flash_pdata *f_pdata,
>>   		reg |= (dummy_clk & CQSPI_REG_CMDCTRL_DUMMY_MASK)
>>   		     << CQSPI_REG_CMDCTRL_DUMMY_LSB;
>>   
>> -	reg |= (0x1 << CQSPI_REG_CMDCTRL_RD_EN_LSB);
>> +	reg |= BIT(CQSPI_REG_CMDCTRL_RD_EN_LSB);
>>   
>>   	/* 0 means 1 byte. */
>>   	reg |= (((n_rx - 1) & CQSPI_REG_CMDCTRL_RD_BYTES_MASK)
>> @@ -1191,7 +1191,7 @@ static void cqspi_chipselect(struct cqspi_flash_pdata *f_pdata)
>>   		 * CS2 to 4b'1011
>>   		 * CS3 to 4b'0111
>>   		 */
>> -		chip_select = 0xF & ~(1 << chip_select);
>> +		chip_select = 0xF & ~BIT(chip_select);
>>   	}
>>   
>>   	reg &= ~(CQSPI_REG_CONFIG_CHIPSELECT_MASK
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2025-09-05 11:05 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-09-04 13:31 [PATCH 0/4] Miscellaneous fixes and clean-ups Santhosh Kumar K
2025-09-04 13:31 ` [PATCH 1/4] spi: cadence-quadspi: Flush posted register writes before INDAC access Santhosh Kumar K
2025-09-04 14:35   ` Pratyush Yadav
2025-09-04 13:31 ` [PATCH 2/4] spi: cadence-quadspi: Flush posted register writes before DAC access Santhosh Kumar K
2025-09-04 14:36   ` Pratyush Yadav
2025-09-04 13:31 ` [PATCH 3/4] spi: cadence-quadspi: Fix cqspi_setup_flash() Santhosh Kumar K
2025-09-04 14:41   ` Pratyush Yadav
2025-09-05 11:04     ` Santhosh Kumar K
2025-09-04 15:32   ` Théo Lebrun
2025-09-05 11:04     ` Santhosh Kumar K
2025-09-04 13:31 ` [PATCH 4/4] spi: cadence-quadspi: Use BIT() macros where possible Santhosh Kumar K
2025-09-04 14:49   ` Pratyush Yadav
2025-09-05 11:04     ` Santhosh Kumar K

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox