From: Carlos Song <[email protected]>
For DMA mode, the bus width of the DMA is equal to the size of data
word, so burst length should be configured as bits per word.
For CPU mode, because of the spi transfer len is in byte, so burst
length should be configured as bits per byte * spi_imx->count.
Signed-off-by: Carlos Song <[email protected]>
Reviewed-by: Clark Wang <[email protected]>
Fixes: e9b220aeacf1 ("spi: spi-imx: correctly configure burst length when using dma")
Fixes: 5f66db08cbd3 ("spi: imx: Take in account bits per word instead of assuming 8-bits")
---
Changes for V3:
- include <linux/bits.h>
Changes for V4:
- keep the includes sorted alphabetically.
---
drivers/spi/spi-imx.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index 546cdce525fc..f7990ac2c654 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -2,6 +2,7 @@
// Copyright 2004-2007 Freescale Semiconductor, Inc. All Rights Reserved.
// Copyright (C) 2008 Juergen Beisert
+#include <linux/bits.h>
#include <linux/clk.h>
#include <linux/completion.h>
#include <linux/delay.h>
@@ -660,15 +661,14 @@ static int mx51_ecspi_prepare_transfer(struct spi_imx_data *spi_imx,
<< MX51_ECSPI_CTRL_BL_OFFSET;
else {
if (spi_imx->usedma) {
- ctrl |= (spi_imx->bits_per_word *
- spi_imx_bytes_per_word(spi_imx->bits_per_word) - 1)
+ 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 - 1)
+ ctrl |= (MX51_ECSPI_CTRL_MAX_BURST * BITS_PER_BYTE - 1)
<< MX51_ECSPI_CTRL_BL_OFFSET;
else
- ctrl |= (spi_imx->count * spi_imx->bits_per_word - 1)
+ ctrl |= (spi_imx->count * BITS_PER_BYTE - 1)
<< MX51_ECSPI_CTRL_BL_OFFSET;
}
}
--
2.34.1
On Thu, 2024-02-01 at 18:54 +0800, [email protected] wrote:
> From: Carlos Song <[email protected]>
>
> For DMA mode, the bus width of the DMA is equal to the size of data
> word, so burst length should be configured as bits per word.
>
> For CPU mode, because of the spi transfer len is in byte, so burst
> length should be configured as bits per byte * spi_imx->count.
>
> Signed-off-by: Carlos Song <[email protected]>
> Reviewed-by: Clark Wang <[email protected]>
> Fixes: e9b220aeacf1 ("spi: spi-imx: correctly configure burst length when using dma")
> Fixes: 5f66db08cbd3 ("spi: imx: Take in account bits per word instead of assuming 8-bits")
> ---
> Changes for V3:
> - include <linux/bits.h>
> Changes for V4:
> - keep the includes sorted alphabetically.
> ---
> drivers/spi/spi-imx.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
> index 546cdce525fc..f7990ac2c654 100644
> --- a/drivers/spi/spi-imx.c
> +++ b/drivers/spi/spi-imx.c
> @@ -2,6 +2,7 @@
> // Copyright 2004-2007 Freescale Semiconductor, Inc. All Rights Reserved.
> // Copyright (C) 2008 Juergen Beisert
>
> +#include <linux/bits.h>
> #include <linux/clk.h>
> #include <linux/completion.h>
> #include <linux/delay.h>
> @@ -660,15 +661,14 @@ static int mx51_ecspi_prepare_transfer(struct spi_imx_data *spi_imx,
> << MX51_ECSPI_CTRL_BL_OFFSET;
> else {
> if (spi_imx->usedma) {
> - ctrl |= (spi_imx->bits_per_word *
> - spi_imx_bytes_per_word(spi_imx->bits_per_word) - 1)
> + 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 - 1)
> + ctrl |= (MX51_ECSPI_CTRL_MAX_BURST * BITS_PER_BYTE - 1)
> << MX51_ECSPI_CTRL_BL_OFFSET;
> else
> - ctrl |= (spi_imx->count * spi_imx->bits_per_word - 1)
> + ctrl |= (spi_imx->count * BITS_PER_BYTE - 1)
I think that will not work for drivers which dont use bits_per_word=8.
https://lore.kernel.org/all/[email protected]/
> << MX51_ECSPI_CTRL_BL_OFFSET;
> }
> }
Best regards,
Benjamin Bigler
> -----Original Message-----
> From: Benjamin Bigler <[email protected]>
> Sent: Friday, February 2, 2024 4:15 AM
> To: Carlos Song <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected];
> dl-linux-imx <[email protected]>; [email protected]
> Cc: [email protected]; [email protected];
> [email protected]
> Subject: [EXT] Re: [PATCH v4] spi: imx: fix the burst length at DMA mode and
> CPU mode
>
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report this
> email' button
>
>
> On Thu, 2024-02-01 at 18:54 +0800, [email protected] wrote:
> > From: Carlos Song <[email protected]>
> >
> > For DMA mode, the bus width of the DMA is equal to the size of data
> > word, so burst length should be configured as bits per word.
> >
> > For CPU mode, because of the spi transfer len is in byte, so burst
> > length should be configured as bits per byte * spi_imx->count.
> >
> > Signed-off-by: Carlos Song <[email protected]>
> > Reviewed-by: Clark Wang <[email protected]>
> > Fixes: e9b220aeacf1 ("spi: spi-imx: correctly configure burst length
> > when using dma")
> > Fixes: 5f66db08cbd3 ("spi: imx: Take in account bits per word instead
> > of assuming 8-bits")
> > ---
> > Changes for V3:
> > - include <linux/bits.h>
> > Changes for V4:
> > - keep the includes sorted alphabetically.
> > ---
> > drivers/spi/spi-imx.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c index
> > 546cdce525fc..f7990ac2c654 100644
> > --- a/drivers/spi/spi-imx.c
> > +++ b/drivers/spi/spi-imx.c
> > @@ -2,6 +2,7 @@
> > // Copyright 2004-2007 Freescale Semiconductor, Inc. All Rights Reserved.
> > // Copyright (C) 2008 Juergen Beisert
> >
> > +#include <linux/bits.h>
> > #include <linux/clk.h>
> > #include <linux/completion.h>
> > #include <linux/delay.h>
> > @@ -660,15 +661,14 @@ static int mx51_ecspi_prepare_transfer(struct
> spi_imx_data *spi_imx,
> > << MX51_ECSPI_CTRL_BL_OFFSET;
> > else {
> > if (spi_imx->usedma) {
> > - ctrl |= (spi_imx->bits_per_word *
> > -
> spi_imx_bytes_per_word(spi_imx->bits_per_word) - 1)
> > + 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 -
> 1)
> > + ctrl |= (MX51_ECSPI_CTRL_MAX_BURST *
> > + BITS_PER_BYTE - 1)
> > <<
> MX51_ECSPI_CTRL_BL_OFFSET;
> > else
> > - ctrl |= (spi_imx->count *
> spi_imx->bits_per_word - 1)
> > + ctrl |= (spi_imx->count * BITS_PER_BYTE
> > + - 1)
Hi,
Thank you! You re right. I have get the patch history that spi->bits_per_word = 9 will
break the driver, this is a special case.
So burst length should be calculated by the number of words, instead of the number of bytes, otherwise the transmission of these non-byte-aligned word will break the driver.
the burst length can be set by:
spi_imx->count/DIV_ROUND_UP(spi_imx->bits_per_word, BITS_PER_BYTE) * spi_imx->bits_per_word.
I will set V5 later.
> I think that will not work for drivers which dont use bits_per_word=8.
> https://lore.ker/
> nel.org%2Fall%2F20230917164037.29284-1-stefanmoring%40gmail.com%2F&
> data=05%7C02%7Ccarlos.song%40nxp.com%7C6f52557285a34b97c6f508dc2
> 3626f51%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638424152
> 883113184%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoi
> V2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=2RerZ6
> 2kHdgiwqi5yxnDPLwWViplttHA2E05WFZmTuw%3D&reserved=0
> > <<
> MX51_ECSPI_CTRL_BL_OFFSET;
> > }
> > }
>
> Best regards,
> Benjamin Bigler