diff mbox series

spi: spi-imx: imx51: revert burst length calculation back to bits_per_word

Message ID 20240618-spi-imx-fix-bustlength-v1-1-2053dd5fdf87@pengutronix.de (mailing list archive)
State Accepted
Commit df75470b317b46affbe1f5f8f006b34175be9789
Headers show
Series spi: spi-imx: imx51: revert burst length calculation back to bits_per_word | expand

Commit Message

Marc Kleine-Budde June 18, 2024, 5:34 p.m. UTC 
The patch 15a6af94a277 ("spi: Increase imx51 ecspi burst length based
on transfer length") increased the burst length calculation in
mx51_ecspi_prepare_transfer() to be based on the transfer length.

This breaks HW CS + SPI_CS_WORD support which was added in
6e95b23a5b2d ("spi: imx: Implement support for CS_WORD") and transfers
with bits-per-word != 8, 16, 32.

SPI_CS_WORD means the CS should be toggled after each word. The
implementation in the imx-spi driver relies on the fact that the HW CS
is toggled automatically by the controller after each burst length
number of bits. Setting the burst length to the number of bits of the
_whole_ message breaks this use case.

Further the patch 15a6af94a277 ("spi: Increase imx51 ecspi burst
length based on transfer length") claims to optimize the transfers.
But even without this patch, on modern spi-imx controllers with
"dynamic_burst = true" (imx51, imx6 and newer), the transfers are
already optimized, i.e. the burst length is dynamically adjusted in
spi_imx_push() to avoid the pause between the SPI bursts. This has
been confirmed by a scope measurement on an imx6d.

Subsequent Patches tried to fix these and other problems:

- 5f66db08cbd3 ("spi: imx: Take in account bits per word instead of assuming 8-bits")
- e9b220aeacf1 ("spi: spi-imx: correctly configure burst length when using dma")
- c712c05e46c8 ("spi: imx: fix the burst length at DMA mode and CPU mode")
- cf6d79a0f576 ("spi: spi-imx: fix off-by-one in mx51 CPU mode burst length")

but the HW CS + SPI_CS_WORD use case is still broken.

To fix the problems revert the burst size calculation in
mx51_ecspi_prepare_transfer() back to the original form, before
15a6af94a277 ("spi: Increase imx51 ecspi burst length based on
transfer length") was applied.

Cc: Stefan Moring <stefan.moring@technolution.nl>
Cc: Stefan Bigler <linux@bigler.io>
Cc: Clark Wang <xiaoning.wang@nxp.com>
Cc: Carlos Song <carlos.song@nxp.com>
Cc: Sebastian Reichel <sre@kernel.org>
Cc: Thorsten Scherer <T.Scherer@eckelmann.de>
Fixes: 15a6af94a277 ("spi: Increase imx51 ecspi burst length based on transfer length")
Fixes: 5f66db08cbd3 ("spi: imx: Take in account bits per word instead of assuming 8-bits")
Fixes: e9b220aeacf1 ("spi: spi-imx: correctly configure burst length when using dma")
Fixes: c712c05e46c8 ("spi: imx: fix the burst length at DMA mode and CPU mode")
Fixes: cf6d79a0f576 ("spi: spi-imx: fix off-by-one in mx51 CPU mode burst length")
Link: https://lore.kernel.org/all/20240618-oxpecker-of-ideal-mastery-db59f8-mkl@pengutronix.de
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/spi/spi-imx.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)


---
base-commit: 6ba59ff4227927d3a8530fc2973b80e94b54d58f
change-id: 20240618-spi-imx-fix-bustlength-9704b2fd9095

Best regards,

Comments

Thorsten Scherer June 19, 2024, 9:40 a.m. UTC | #1 
Hello,

On Tue, Jun 18, 2024 at 07:34:18PM +0200, Marc Kleine-Budde wrote:
> The patch 15a6af94a277 ("spi: Increase imx51 ecspi burst length based
> on transfer length") increased the burst length calculation in
> mx51_ecspi_prepare_transfer() to be based on the transfer length.
> 
> This breaks HW CS + SPI_CS_WORD support which was added in
> 6e95b23a5b2d ("spi: imx: Implement support for CS_WORD") and transfers
> with bits-per-word != 8, 16, 32.
> 
> SPI_CS_WORD means the CS should be toggled after each word. The
> implementation in the imx-spi driver relies on the fact that the HW CS
> is toggled automatically by the controller after each burst length
> number of bits. Setting the burst length to the number of bits of the
> _whole_ message breaks this use case.
> 
> Further the patch 15a6af94a277 ("spi: Increase imx51 ecspi burst
> length based on transfer length") claims to optimize the transfers.
> But even without this patch, on modern spi-imx controllers with
> "dynamic_burst = true" (imx51, imx6 and newer), the transfers are
> already optimized, i.e. the burst length is dynamically adjusted in
> spi_imx_push() to avoid the pause between the SPI bursts. This has
> been confirmed by a scope measurement on an imx6d.
> 
> Subsequent Patches tried to fix these and other problems:
> 
> - 5f66db08cbd3 ("spi: imx: Take in account bits per word instead of assuming 8-bits")
> - e9b220aeacf1 ("spi: spi-imx: correctly configure burst length when using dma")
> - c712c05e46c8 ("spi: imx: fix the burst length at DMA mode and CPU mode")
> - cf6d79a0f576 ("spi: spi-imx: fix off-by-one in mx51 CPU mode burst length")
> 
> but the HW CS + SPI_CS_WORD use case is still broken.
> 
> To fix the problems revert the burst size calculation in
> mx51_ecspi_prepare_transfer() back to the original form, before
> 15a6af94a277 ("spi: Increase imx51 ecspi burst length based on
> transfer length") was applied.
> 
> Cc: Stefan Moring <stefan.moring@technolution.nl>
> Cc: Stefan Bigler <linux@bigler.io>
> Cc: Clark Wang <xiaoning.wang@nxp.com>
> Cc: Carlos Song <carlos.song@nxp.com>
> Cc: Sebastian Reichel <sre@kernel.org>
> Cc: Thorsten Scherer <T.Scherer@eckelmann.de>
> Fixes: 15a6af94a277 ("spi: Increase imx51 ecspi burst length based on transfer length")
> Fixes: 5f66db08cbd3 ("spi: imx: Take in account bits per word instead of assuming 8-bits")
> Fixes: e9b220aeacf1 ("spi: spi-imx: correctly configure burst length when using dma")
> Fixes: c712c05e46c8 ("spi: imx: fix the burst length at DMA mode and CPU mode")
> Fixes: cf6d79a0f576 ("spi: spi-imx: fix off-by-one in mx51 CPU mode burst length")
> Link: https://lore.kernel.org/all/20240618-oxpecker-of-ideal-mastery-db59f8-mkl@pengutronix.de
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>

This patch fixes an issue on our i.MX8MP board (test based on v6.9 +
small-custom-patchstack + this-patch) where querying a ti,ads7953
(spi->mode |= SPI_CS_WORD) returned either 0 or 4095 .

Tested-by: Thorsten Scherer <t.scherer@eckelmann.de>

Thank you, Marc.

> ---
>  drivers/spi/spi-imx.c | 14 ++------------
>  1 file changed, 2 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
> index f4006c82f867..33164ebdb583 100644
> --- a/drivers/spi/spi-imx.c
> +++ b/drivers/spi/spi-imx.c
> @@ -660,18 +660,8 @@ static int mx51_ecspi_prepare_transfer(struct spi_imx_data *spi_imx,
>  		ctrl |= (spi_imx->target_burst * 8 - 1)
>  			<< MX51_ECSPI_CTRL_BL_OFFSET;
>  	else {
> -		if (spi_imx->usedma) {
> -			ctrl |= (spi_imx->bits_per_word - 1)
> -				<< MX51_ECSPI_CTRL_BL_OFFSET;
> -		} else {
> -			if (spi_imx->count >= MX51_ECSPI_CTRL_MAX_BURST)
> -				ctrl |= (MX51_ECSPI_CTRL_MAX_BURST * BITS_PER_BYTE - 1)
> -						<< MX51_ECSPI_CTRL_BL_OFFSET;
> -			else
> -				ctrl |= (spi_imx->count / DIV_ROUND_UP(spi_imx->bits_per_word,
> -						BITS_PER_BYTE) * spi_imx->bits_per_word - 1)
> -						<< MX51_ECSPI_CTRL_BL_OFFSET;
> -		}
> +		ctrl |= (spi_imx->bits_per_word - 1)
> +			<< MX51_ECSPI_CTRL_BL_OFFSET;
>  	}
>  
>  	/* set clock speed */
> 
> ---
> base-commit: 6ba59ff4227927d3a8530fc2973b80e94b54d58f
> change-id: 20240618-spi-imx-fix-bustlength-9704b2fd9095
> 
> Best regards,
> -- 
> Marc Kleine-Budde <mkl@pengutronix.de>
> 
> 

Best regards
Thorsten
Mark Brown June 20, 2024, 11:13 a.m. UTC | #2 
On Tue, 18 Jun 2024 19:34:18 +0200, Marc Kleine-Budde wrote:
> The patch 15a6af94a277 ("spi: Increase imx51 ecspi burst length based
> on transfer length") increased the burst length calculation in
> mx51_ecspi_prepare_transfer() to be based on the transfer length.
> 
> This breaks HW CS + SPI_CS_WORD support which was added in
> 6e95b23a5b2d ("spi: imx: Implement support for CS_WORD") and transfers
> with bits-per-word != 8, 16, 32.
> 
> [...]

Applied to

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

Thanks!

[1/1] spi: spi-imx: imx51: revert burst length calculation back to bits_per_word
      commit: df75470b317b46affbe1f5f8f006b34175be9789

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
Marc Kleine-Budde June 20, 2024, 11:43 a.m. UTC | #3 
On 20.06.2024 12:13:37, Mark Brown wrote:
> On Tue, 18 Jun 2024 19:34:18 +0200, Marc Kleine-Budde wrote:
> > The patch 15a6af94a277 ("spi: Increase imx51 ecspi burst length based
> > on transfer length") increased the burst length calculation in
> > mx51_ecspi_prepare_transfer() to be based on the transfer length.
> > 
> > This breaks HW CS + SPI_CS_WORD support which was added in
> > 6e95b23a5b2d ("spi: imx: Implement support for CS_WORD") and transfers
> > with bits-per-word != 8, 16, 32.
> > 
> > [...]
> 
> Applied to
> 
>    https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

As this is a fix of a regression, can you pick this for v6.10, too.

Marc
Mark Brown June 20, 2024, 11:47 a.m. UTC | #4 
On Thu, Jun 20, 2024 at 01:43:29PM +0200, Marc Kleine-Budde wrote:

> As this is a fix of a regression, can you pick this for v6.10, too.

It is applied for v6.10.
Marc Kleine-Budde June 20, 2024, 11:50 a.m. UTC | #5 
On 20.06.2024 12:47:14, Mark Brown wrote:
> On Thu, Jun 20, 2024 at 01:43:29PM +0200, Marc Kleine-Budde wrote:
> 
> > As this is a fix of a regression, can you pick this for v6.10, too.
> 
> It is applied for v6.10.

Thanks a lot,
Marc
diff mbox series

Patch

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index f4006c82f867..33164ebdb583 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -660,18 +660,8 @@  static int mx51_ecspi_prepare_transfer(struct spi_imx_data *spi_imx,
 		ctrl |= (spi_imx->target_burst * 8 - 1)
 			<< MX51_ECSPI_CTRL_BL_OFFSET;
 	else {
-		if (spi_imx->usedma) {
-			ctrl |= (spi_imx->bits_per_word - 1)
-				<< MX51_ECSPI_CTRL_BL_OFFSET;
-		} else {
-			if (spi_imx->count >= MX51_ECSPI_CTRL_MAX_BURST)
-				ctrl |= (MX51_ECSPI_CTRL_MAX_BURST * BITS_PER_BYTE - 1)
-						<< MX51_ECSPI_CTRL_BL_OFFSET;
-			else
-				ctrl |= (spi_imx->count / DIV_ROUND_UP(spi_imx->bits_per_word,
-						BITS_PER_BYTE) * spi_imx->bits_per_word - 1)
-						<< MX51_ECSPI_CTRL_BL_OFFSET;
-		}
+		ctrl |= (spi_imx->bits_per_word - 1)
+			<< MX51_ECSPI_CTRL_BL_OFFSET;
 	}
 
 	/* set clock speed */