diff mbox series

[v2] spi: imx: fix the burst length at DMA mode and CPU mode

Message ID 20240131101916.437398-1-carlos.song@nxp.com
State New
Headers show
Series [v2] spi: imx: fix the burst length at DMA mode and CPU mode | expand

Commit Message

Carlos Song Jan. 31, 2024, 10:19 a.m. UTC 
From: Carlos Song <carlos.song@nxp.com>

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 <carlos.song@nxp.com>
Reviewed-by: Clark Wang <xiaoning.wang@nxp.com>
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 V2:
- Removed BITS_PER_BYTE defination
---
 drivers/spi/spi-imx.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Comments

Francesco Dolcini Jan. 31, 2024, 7:42 p.m. UTC | #1 
On Wed, Jan 31, 2024 at 06:19:16PM +0800, carlos.song@nxp.com wrote:
> From: Carlos Song <carlos.song@nxp.com>
> 
> 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 <carlos.song@nxp.com>
> Reviewed-by: Clark Wang <xiaoning.wang@nxp.com>
> 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 V2:
> - Removed BITS_PER_BYTE defination
> ---
>  drivers/spi/spi-imx.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
> index 546cdce525fc..0436e7a161ef 100644
> --- a/drivers/spi/spi-imx.c
> +++ b/drivers/spi/spi-imx.c
> @@ -660,15 +660,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;
>  		}
>  	}

Add #include <linux/bits.h> given you are using BITS_PER_BYTE

Francesco
Carlos Song Feb. 1, 2024, 2:02 a.m. UTC | #2 
> -----Original Message-----
> From: Francesco Dolcini <francesco@dolcini.it>
> Sent: Thursday, February 1, 2024 3:42 AM
> To: Carlos Song <carlos.song@nxp.com>
> Cc: broonie@kernel.org; shawnguo@kernel.org; s.hauer@pengutronix.de;
> kernel@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>; benjamin@bigler.one;
> stefanmoring@gmail.com; linux-spi@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: [EXT] Re: [PATCH v2] 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 Wed, Jan 31, 2024 at 06:19:16PM +0800, carlos.song@nxp.com wrote:
> > From: Carlos Song <carlos.song@nxp.com>
> >
> > 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 <carlos.song@nxp.com>
> > Reviewed-by: Clark Wang <xiaoning.wang@nxp.com>
> > 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 V2:
> > - Removed BITS_PER_BYTE defination
> > ---
> >  drivers/spi/spi-imx.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c index
> > 546cdce525fc..0436e7a161ef 100644
> > --- a/drivers/spi/spi-imx.c
> > +++ b/drivers/spi/spi-imx.c
> > @@ -660,15 +660,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;
> >               }
> >       }
> 
> Add #include <linux/bits.h> given you are using BITS_PER_BYTE
> 
Hi, 

Thanks for your advice!
I found that <linux/property.h> has been included in spi-imx.c driver.
And <linux/bits.h> has been included in <linux/property.h>.
So I didn't add the "#include <linux/bits.h>" on purpose.

BR
Carlos
> Francesco
Francesco Dolcini Feb. 1, 2024, 8:52 a.m. UTC | #3 
On Thu, Feb 01, 2024 at 02:02:36AM +0000, Carlos Song wrote:
> > From: Francesco Dolcini <francesco@dolcini.it>
> > Add #include <linux/bits.h> given you are using BITS_PER_BYTE
> > 
> I found that <linux/property.h> has been included in spi-imx.c driver.
> And <linux/bits.h> has been included in <linux/property.h>.
> So I didn't add the "#include <linux/bits.h>" on purpose.

I think you should be explicit and include this header.
Code get refactored all the time and you can prevent issues this way.

Francesco
Carlos Song Feb. 1, 2024, 9:58 a.m. UTC | #4 
> -----Original Message-----
> From: Francesco Dolcini <francesco@dolcini.it>
> Sent: Thursday, February 1, 2024 4:52 PM
> To: Carlos Song <carlos.song@nxp.com>
> Cc: Francesco Dolcini <francesco@dolcini.it>; broonie@kernel.org;
> shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de;
> dl-linux-imx <linux-imx@nxp.com>; benjamin@bigler.one;
> stefanmoring@gmail.com; linux-spi@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: [EXT] Re: [PATCH v2] 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, Feb 01, 2024 at 02:02:36AM +0000, Carlos Song wrote:
> > > From: Francesco Dolcini <francesco@dolcini.it> Add #include
> > > <linux/bits.h> given you are using BITS_PER_BYTE
> > >
> > I found that <linux/property.h> has been included in spi-imx.c driver.
> > And <linux/bits.h> has been included in <linux/property.h>.
> > So I didn't add the "#include <linux/bits.h>" on purpose.
> 
> I think you should be explicit and include this header.
> Code get refactored all the time and you can prevent issues this way.
> 
Hi，
Thank you! This is acceptable, I will include <linux/bits.h> explicitly and send V3.

BR
Carlos

> Francesco
>
diff mbox series

Patch

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index 546cdce525fc..0436e7a161ef 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -660,15 +660,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;
 		}
 	}