[PATCH 0/2] spi: atmel-quadspi: Fix and add full CS delay support

[PATCH 0/2] spi: atmel-quadspi: Fix and add full CS delay support

[Alexander Dahl]

Hello everyone,

when testing on a SAM9X60 based board with an FPGA implementing a custom
SPI-MEM protocol, we found the need to fully control the delay settings
the atmel/microchip QSPI controller offers.

Greets
Alex

(series based on v6.11, tested on top of v6.6.44)

Cc: linux-spi@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org

Alexander Dahl (2):
  spi: atmel-quadspi: Avoid overwriting delay register settings
  spi: atmel-quadspi: Add cs_hold and cs_inactive setting support

 drivers/spi/atmel-quadspi.c | 50 +++++++++++++++++++++++++++++--------
 1 file changed, 40 insertions(+), 10 deletions(-)


base-commit: 98f7e32f20d28ec452afb208f9cffc08448a2652
-- 
2.39.5

[PATCH 1/2] spi: atmel-quadspi: Avoid overwriting delay register settings

[Alexander Dahl]

Previously the MR and SCR registers were just set with the supposedly
required values, from cached register values (cached reg content
initialized to zero).

All parts fixed here did not consider the current register (cache)
content, which would make future support of cs_setup, cs_hold, and
cs_inactive impossible.

Setting SCBR in atmel_qspi_setup() erases a possible DLYBS setting from
atmel_qspi_set_cs_timing().  The DLYBS setting is applied by ORing over
the current setting, without resetting the bits first.  All writes to MR
did not consider possible settings of DLYCS and DLYBCT.

Signed-off-by: Alexander Dahl <ada@thorsis.com>
Fixes: f732646d0ccd ("spi: atmel-quadspi: Add support for configuring CS timing")
---
 drivers/spi/atmel-quadspi.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c
index 5aaff3bee1b7..fcd57cf1f2cf 100644
--- a/drivers/spi/atmel-quadspi.c
+++ b/drivers/spi/atmel-quadspi.c
@@ -375,9 +375,9 @@ static int atmel_qspi_set_cfg(struct atmel_qspi *aq,
 	 * If the QSPI controller is set in regular SPI mode, set it in
 	 * Serial Memory Mode (SMM).
 	 */
-	if (aq->mr != QSPI_MR_SMM) {
-		atmel_qspi_write(QSPI_MR_SMM, aq, QSPI_MR);
-		aq->mr = QSPI_MR_SMM;
+	if (!(aq->mr & QSPI_MR_SMM)) {
+		aq->mr |= QSPI_MR_SMM;
+		atmel_qspi_write(aq->scr, aq, QSPI_MR);
 	}
 
 	/* Clear pending interrupts */
@@ -501,7 +501,8 @@ static int atmel_qspi_setup(struct spi_device *spi)
 	if (ret < 0)
 		return ret;
 
-	aq->scr = QSPI_SCR_SCBR(scbr);
+	aq->scr &= ~QSPI_SCR_SCBR_MASK;
+	aq->scr |= QSPI_SCR_SCBR(scbr);
 	atmel_qspi_write(aq->scr, aq, QSPI_SCR);
 
 	pm_runtime_mark_last_busy(ctrl->dev.parent);
@@ -534,6 +535,7 @@ static int atmel_qspi_set_cs_timing(struct spi_device *spi)
 	if (ret < 0)
 		return ret;
 
+	aq->scr &= ~QSPI_SCR_DLYBS_MASK;
 	aq->scr |= QSPI_SCR_DLYBS(cs_setup);
 	atmel_qspi_write(aq->scr, aq, QSPI_SCR);
 
@@ -549,8 +551,8 @@ static void atmel_qspi_init(struct atmel_qspi *aq)
 	atmel_qspi_write(QSPI_CR_SWRST, aq, QSPI_CR);
 
 	/* Set the QSPI controller by default in Serial Memory Mode */
-	atmel_qspi_write(QSPI_MR_SMM, aq, QSPI_MR);
-	aq->mr = QSPI_MR_SMM;
+	aq->mr |= QSPI_MR_SMM;
+	atmel_qspi_write(aq->mr, aq, QSPI_MR);
 
 	/* Enable the QSPI controller */
 	atmel_qspi_write(QSPI_CR_QSPIEN, aq, QSPI_CR);
-- 
2.39.5

Re: [PATCH 1/2] spi: atmel-quadspi: Avoid overwriting delay register settings

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 912 bytes --]

On Wed, Sep 18, 2024 at 10:27:43AM +0200, Alexander Dahl wrote:
> Previously the MR and SCR registers were just set with the supposedly
> required values, from cached register values (cached reg content
> initialized to zero).
> 
> All parts fixed here did not consider the current register (cache)
> content, which would make future support of cs_setup, cs_hold, and
> cs_inactive impossible.
> 
> Setting SCBR in atmel_qspi_setup() erases a possible DLYBS setting from
> atmel_qspi_set_cs_timing().  The DLYBS setting is applied by ORing over
> the current setting, without resetting the bits first.  All writes to MR
> did not consider possible settings of DLYCS and DLYBCT.
> 
> Signed-off-by: Alexander Dahl <ada@thorsis.com>
> Fixes: f732646d0ccd ("spi: atmel-quadspi: Add support for configuring CS timing")

This isn't actually a fix AFAICT since nothing yet sets any of these
fields?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] spi: atmel-quadspi: Avoid overwriting delay register settings

[Alexander Dahl]

Hello Mark,

Am Fri, Sep 20, 2024 at 10:22:19AM +0200 schrieb Mark Brown:
> On Wed, Sep 18, 2024 at 10:27:43AM +0200, Alexander Dahl wrote:
> > Previously the MR and SCR registers were just set with the supposedly
> > required values, from cached register values (cached reg content
> > initialized to zero).
> > 
> > All parts fixed here did not consider the current register (cache)
> > content, which would make future support of cs_setup, cs_hold, and
> > cs_inactive impossible.
> > 
> > Setting SCBR in atmel_qspi_setup() erases a possible DLYBS setting from
> > atmel_qspi_set_cs_timing().  The DLYBS setting is applied by ORing over
> > the current setting, without resetting the bits first.  All writes to MR
> > did not consider possible settings of DLYCS and DLYBCT.
> > 
> > Signed-off-by: Alexander Dahl <ada@thorsis.com>
> > Fixes: f732646d0ccd ("spi: atmel-quadspi: Add support for configuring CS timing")
> 
> This isn't actually a fix AFAICT since nothing yet sets any of these
> fields?

You're right if we just consider board dts files in mainline.  None of
those using the atmel-quadspi driver have a spi-cs-*-delay property
set in a SPI slave device node in current master.

The changes in this patch to MR writes do not change behaviour.

For changes to SCR however I see two possible bugs:

1. if atmel_qspi_set_cs_timing() is called before atmel_qspi_setup(),
   the second call just overwrites what the first call set.

2. if atmel_qspi_set_cs_timing() is called multiple times with
   different values, the values written to the register from the second
   call onwards are just wrong.

Maybe both are scenarios not happening in practice?

Long story short, I could just remove the Fixes line, and the rest is
fine?  Or should I split up with changes for MR and SCR going to
separate patches?

Greets
Alex

Re: [PATCH 1/2] spi: atmel-quadspi: Avoid overwriting delay register settings

[Alexander Dahl]

Hello everyone,

Am Wed, Sep 18, 2024 at 10:27:43AM +0200 schrieb Alexander Dahl:
> Previously the MR and SCR registers were just set with the supposedly
> required values, from cached register values (cached reg content
> initialized to zero).
> 
> All parts fixed here did not consider the current register (cache)
> content, which would make future support of cs_setup, cs_hold, and
> cs_inactive impossible.
> 
> Setting SCBR in atmel_qspi_setup() erases a possible DLYBS setting from
> atmel_qspi_set_cs_timing().  The DLYBS setting is applied by ORing over
> the current setting, without resetting the bits first.  All writes to MR
> did not consider possible settings of DLYCS and DLYBCT.
> 
> Signed-off-by: Alexander Dahl <ada@thorsis.com>
> Fixes: f732646d0ccd ("spi: atmel-quadspi: Add support for configuring CS timing")
> ---
>  drivers/spi/atmel-quadspi.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c
> index 5aaff3bee1b7..fcd57cf1f2cf 100644
> --- a/drivers/spi/atmel-quadspi.c
> +++ b/drivers/spi/atmel-quadspi.c
> @@ -375,9 +375,9 @@ static int atmel_qspi_set_cfg(struct atmel_qspi *aq,
>  	 * If the QSPI controller is set in regular SPI mode, set it in
>  	 * Serial Memory Mode (SMM).
>  	 */
> -	if (aq->mr != QSPI_MR_SMM) {
> -		atmel_qspi_write(QSPI_MR_SMM, aq, QSPI_MR);
> -		aq->mr = QSPI_MR_SMM;
> +	if (!(aq->mr & QSPI_MR_SMM)) {
> +		aq->mr |= QSPI_MR_SMM;
> +		atmel_qspi_write(aq->scr, aq, QSPI_MR);

On a second glance, this looks wrong, value for Mode Register (MR) is
written into Serial Clock Register (SCR), should be like this instead:

    atmel_qspi_write(aq->mr, aq, QSPI_MR);

This is somewhat embarrassing, because the patch was already applied
to master.  Should it be reverted, then I would send a v2 of the
series?  Or should I send a quick fixup?

Greets
Alex

>  	}
>  
>  	/* Clear pending interrupts */
> @@ -501,7 +501,8 @@ static int atmel_qspi_setup(struct spi_device *spi)
>  	if (ret < 0)
>  		return ret;
>  
> -	aq->scr = QSPI_SCR_SCBR(scbr);
> +	aq->scr &= ~QSPI_SCR_SCBR_MASK;
> +	aq->scr |= QSPI_SCR_SCBR(scbr);
>  	atmel_qspi_write(aq->scr, aq, QSPI_SCR);
>  
>  	pm_runtime_mark_last_busy(ctrl->dev.parent);
> @@ -534,6 +535,7 @@ static int atmel_qspi_set_cs_timing(struct spi_device *spi)
>  	if (ret < 0)
>  		return ret;
>  
> +	aq->scr &= ~QSPI_SCR_DLYBS_MASK;
>  	aq->scr |= QSPI_SCR_DLYBS(cs_setup);
>  	atmel_qspi_write(aq->scr, aq, QSPI_SCR);
>  
> @@ -549,8 +551,8 @@ static void atmel_qspi_init(struct atmel_qspi *aq)
>  	atmel_qspi_write(QSPI_CR_SWRST, aq, QSPI_CR);
>  
>  	/* Set the QSPI controller by default in Serial Memory Mode */
> -	atmel_qspi_write(QSPI_MR_SMM, aq, QSPI_MR);
> -	aq->mr = QSPI_MR_SMM;
> +	aq->mr |= QSPI_MR_SMM;
> +	atmel_qspi_write(aq->mr, aq, QSPI_MR);
>  
>  	/* Enable the QSPI controller */
>  	atmel_qspi_write(QSPI_CR_QSPIEN, aq, QSPI_CR);
> -- 
> 2.39.5
> 
> 

Re: [PATCH 1/2] spi: atmel-quadspi: Avoid overwriting delay register settings

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 272 bytes --]

On Thu, Sep 26, 2024 at 09:25:10AM +0200, Alexander Dahl wrote:

> This is somewhat embarrassing, because the patch was already applied
> to master.  Should it be reverted, then I would send a v2 of the
> series?  Or should I send a quick fixup?

A quick fixup is better.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 2/2] spi: atmel-quadspi: Add cs_hold and cs_inactive setting support

[Alexander Dahl]

spi-cs-inactive-delay-ns in dts is cs_inactive in spi core, and it maps
to DLYCS (Minimum Inactive QCS Delay) in QSPI Mode Register (QSPI_MR).

spi-cs-hold-delay-ns in dts is cs_hold in spi core, and it maps to
DLYBCT (Delay Between Consecutive Transfers) in QSPI_MR.  That one can
be set to other values than 0 only if the chip is not in Serial Memory
Mode (SMM), it must be written to '0' however when in SMM.

Tested on SAM9X60 based board with FPGA implementing custom SPI Memory
protocol.

Signed-off-by: Alexander Dahl <ada@thorsis.com>
---
 drivers/spi/atmel-quadspi.c | 36 ++++++++++++++++++++++++++++++++----
 1 file changed, 32 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c
index fcd57cf1f2cf..d46e2ca76330 100644
--- a/drivers/spi/atmel-quadspi.c
+++ b/drivers/spi/atmel-quadspi.c
@@ -516,21 +516,45 @@ static int atmel_qspi_set_cs_timing(struct spi_device *spi)
 	struct spi_controller *ctrl = spi->controller;
 	struct atmel_qspi *aq = spi_controller_get_devdata(ctrl);
 	unsigned long clk_rate;
+	u32 cs_inactive;
 	u32 cs_setup;
+	u32 cs_hold;
 	int delay;
 	int ret;
 
-	delay = spi_delay_to_ns(&spi->cs_setup, NULL);
-	if (delay <= 0)
-		return delay;
-
 	clk_rate = clk_get_rate(aq->pclk);
 	if (!clk_rate)
 		return -EINVAL;
 
+	/* hold */
+	delay = spi_delay_to_ns(&spi->cs_hold, NULL);
+	if (aq->mr & QSPI_MR_SMM) {
+		if (delay > 0)
+			dev_warn(&aq->pdev->dev,
+				 "Ignoring cs_hold, must be 0 in Serial Memory Mode.\n");
+		cs_hold = 0;
+	} else {
+		delay = spi_delay_to_ns(&spi->cs_hold, NULL);
+		if (delay < 0)
+			return delay;
+
+		cs_hold = DIV_ROUND_UP((delay * DIV_ROUND_UP(clk_rate, 1000000)), 32000);
+	}
+
+	/* setup */
+	delay = spi_delay_to_ns(&spi->cs_setup, NULL);
+	if (delay < 0)
+		return delay;
+
 	cs_setup = DIV_ROUND_UP((delay * DIV_ROUND_UP(clk_rate, 1000000)),
 				1000);
 
+	/* inactive */
+	delay = spi_delay_to_ns(&spi->cs_inactive, NULL);
+	if (delay < 0)
+		return delay;
+	cs_inactive = DIV_ROUND_UP((delay * DIV_ROUND_UP(clk_rate, 1000000)), 1000);
+
 	ret = pm_runtime_resume_and_get(ctrl->dev.parent);
 	if (ret < 0)
 		return ret;
@@ -539,6 +563,10 @@ static int atmel_qspi_set_cs_timing(struct spi_device *spi)
 	aq->scr |= QSPI_SCR_DLYBS(cs_setup);
 	atmel_qspi_write(aq->scr, aq, QSPI_SCR);
 
+	aq->mr &= ~(QSPI_MR_DLYBCT_MASK | QSPI_MR_DLYCS_MASK);
+	aq->mr |= QSPI_MR_DLYBCT(cs_hold) | QSPI_MR_DLYCS(cs_inactive);
+	atmel_qspi_write(aq->mr, aq, QSPI_MR);
+
 	pm_runtime_mark_last_busy(ctrl->dev.parent);
 	pm_runtime_put_autosuspend(ctrl->dev.parent);
 
-- 
2.39.5

Re: (subset) [PATCH 0/2] spi: atmel-quadspi: Fix and add full CS delay support

[Mark Brown]

On Wed, 18 Sep 2024 10:27:42 +0200, Alexander Dahl wrote:
> when testing on a SAM9X60 based board with an FPGA implementing a custom
> SPI-MEM protocol, we found the need to fully control the delay settings
> the atmel/microchip QSPI controller offers.
> 
> Greets
> Alex
> 
> [...]

Applied to

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

Thanks!

[1/2] spi: atmel-quadspi: Avoid overwriting delay register settings
      commit: 329ca3eed4a9a161515a8714be6ba182321385c7

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

Re: (subset) [PATCH 0/2] spi: atmel-quadspi: Fix and add full CS delay support

[Mark Brown]

On Wed, 18 Sep 2024 10:27:42 +0200, Alexander Dahl wrote:
> when testing on a SAM9X60 based board with an FPGA implementing a custom
> SPI-MEM protocol, we found the need to fully control the delay settings
> the atmel/microchip QSPI controller offers.
> 
> Greets
> Alex
> 
> [...]

Applied to

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

Thanks!

[2/2] spi: atmel-quadspi: Add cs_hold and cs_inactive setting support
      commit: 625de1881b5aee6a42a3130004e47dbd632429f8

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