diff mbox series

[v2,2/4] spi: spi-fsl-dspi: Fix delete the processing of undefined bitmask for rxdata

Message ID 20180930092535.24544-2-chuanhua.han@nxp.com (mailing list archive)
State New, archived
Headers show
Series [v2,1/4] spi: spi-mem: Add the spi_set_xfer_bpw function | expand

Commit Message

Chuanhua Han Sept. 30, 2018, 9:25 a.m. UTC
This patch fixes the problem of rxdata being equal to 0 during the XSPI
mode transfer of the dspi controller.
In XSPI mode, If it is not deleted, the value of rxdata will be equal
to 0, and the data received will not be received correctly, causing the
receiving transfer of the spi to fail.

Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com>
---
Changes in v2:
 -The original patch is divided into multiple patches(the original
patch theme is "spi: spi-fsl-dspi: Fix support for XSPI transport
mode"),one of which is segmented.

 drivers/spi/spi-fsl-dspi.c | 3 ---
 1 file changed, 3 deletions(-)

Comments

Boris Brezillon Sept. 30, 2018, 10:06 a.m. UTC | #1
On Sun, 30 Sep 2018 17:25:33 +0800
Chuanhua Han <chuanhua.han@nxp.com> wrote:

> This patch fixes the problem of rxdata being equal to 0 during the XSPI
> mode transfer of the dspi controller.
> In XSPI mode, If it is not deleted, the value of rxdata will be equal
> to 0, and the data received will not be received correctly, causing the
> receiving transfer of the spi to fail.
> 
> Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com>
> ---
> Changes in v2:
>  -The original patch is divided into multiple patches(the original
> patch theme is "spi: spi-fsl-dspi: Fix support for XSPI transport
> mode"),one of which is segmented.
> 
>  drivers/spi/spi-fsl-dspi.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
> index 3082e72e4f6c..4dc1064bf408 100644
> --- a/drivers/spi/spi-fsl-dspi.c
> +++ b/drivers/spi/spi-fsl-dspi.c
> @@ -243,9 +243,6 @@ static void dspi_push_rx(struct fsl_dspi *dspi, u32 rxdata)
>  	if (!dspi->rx)
>  		return;
>  
> -	/* Mask of undefined bits */
> -	rxdata &= (1 << dspi->bits_per_word) - 1;
> -

Why not

	if (dspi->bits_per_word)
		rxdata &= (1 << dspi->bits_per_word) - 1;

>  	if (dspi->bytes_per_word == 1)
>  		*(u8 *)dspi->rx = rxdata;
>  	else if (dspi->bytes_per_word == 2)
Chuanhua Han Sept. 30, 2018, 10:10 a.m. UTC | #2
> -----Original Message-----
> From: Boris Brezillon <boris.brezillon@bootlin.com>
> Sent: 2018年9月30日 18:07
> To: Chuanhua Han <chuanhua.han@nxp.com>
> Cc: broonie@kernel.org; linux-spi@vger.kernel.org;
> linux-kernel@vger.kernel.org; eha@deif.com
> Subject: Re: [PATCH v2 2/4] spi: spi-fsl-dspi: Fix delete the processing of
> undefined bitmask for rxdata
> 
> On Sun, 30 Sep 2018 17:25:33 +0800
> Chuanhua Han <chuanhua.han@nxp.com> wrote:
> 
> > This patch fixes the problem of rxdata being equal to 0 during the
> > XSPI mode transfer of the dspi controller.
> > In XSPI mode, If it is not deleted, the value of rxdata will be equal
> > to 0, and the data received will not be received correctly, causing
> > the receiving transfer of the spi to fail.
> >
> > Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com>
> > ---
> > Changes in v2:
> >  -The original patch is divided into multiple patches(the original
> > patch theme is "spi: spi-fsl-dspi: Fix support for XSPI transport
> > mode"),one of which is segmented.
> >
> >  drivers/spi/spi-fsl-dspi.c | 3 ---
> >  1 file changed, 3 deletions(-)
> >
> > diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
> > index 3082e72e4f6c..4dc1064bf408 100644
> > --- a/drivers/spi/spi-fsl-dspi.c
> > +++ b/drivers/spi/spi-fsl-dspi.c
> > @@ -243,9 +243,6 @@ static void dspi_push_rx(struct fsl_dspi *dspi, u32
> rxdata)
> >  	if (!dspi->rx)
> >  		return;
> >
> > -	/* Mask of undefined bits */
> > -	rxdata &= (1 << dspi->bits_per_word) - 1;
> > -
> 
> Why not
In xspi mode, the value of rxdata after the statement is processed is equal to 0 no matter what data is received.
> 
> 	if (dspi->bits_per_word)
> 		rxdata &= (1 << dspi->bits_per_word) - 1;
> 
> >  	if (dspi->bytes_per_word == 1)
> >  		*(u8 *)dspi->rx = rxdata;
> >  	else if (dspi->bytes_per_word == 2)
Boris Brezillon Sept. 30, 2018, 10:17 a.m. UTC | #3
On Sun, 30 Sep 2018 10:10:14 +0000
Chuanhua Han <chuanhua.han@nxp.com> wrote:

> > -----Original Message-----
> > From: Boris Brezillon <boris.brezillon@bootlin.com>
> > Sent: 2018年9月30日 18:07
> > To: Chuanhua Han <chuanhua.han@nxp.com>
> > Cc: broonie@kernel.org; linux-spi@vger.kernel.org;
> > linux-kernel@vger.kernel.org; eha@deif.com
> > Subject: Re: [PATCH v2 2/4] spi: spi-fsl-dspi: Fix delete the processing of
> > undefined bitmask for rxdata
> > 
> > On Sun, 30 Sep 2018 17:25:33 +0800
> > Chuanhua Han <chuanhua.han@nxp.com> wrote:
> >   
> > > This patch fixes the problem of rxdata being equal to 0 during the
> > > XSPI mode transfer of the dspi controller.
> > > In XSPI mode, If it is not deleted, the value of rxdata will be equal
> > > to 0, and the data received will not be received correctly, causing
> > > the receiving transfer of the spi to fail.
> > >
> > > Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com>
> > > ---
> > > Changes in v2:
> > >  -The original patch is divided into multiple patches(the original
> > > patch theme is "spi: spi-fsl-dspi: Fix support for XSPI transport
> > > mode"),one of which is segmented.
> > >
> > >  drivers/spi/spi-fsl-dspi.c | 3 ---
> > >  1 file changed, 3 deletions(-)
> > >
> > > diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
> > > index 3082e72e4f6c..4dc1064bf408 100644
> > > --- a/drivers/spi/spi-fsl-dspi.c
> > > +++ b/drivers/spi/spi-fsl-dspi.c
> > > @@ -243,9 +243,6 @@ static void dspi_push_rx(struct fsl_dspi *dspi, u32  
> > rxdata)  
> > >  	if (!dspi->rx)
> > >  		return;
> > >
> > > -	/* Mask of undefined bits */
> > > -	rxdata &= (1 << dspi->bits_per_word) - 1;
> > > -  
> > 
> > Why not  
> In xspi mode, the value of rxdata after the statement is processed is equal to 0 no matter what data is received.

Only if dspi->bits_per_word is 0.

Actually, I just had a look, and xfer->bits_per_word should never be 0
because spi_validate() makes sure it's initialized [1]. Don't know
where dpsi->bits_per_word comes from, but maybe you have a problem
there (dpsi->bits_per_word and xfer->bits_per_word not in sync).

[1]https://elixir.bootlin.com/linux/v4.19-rc5/source/drivers/spi/spi.c#L2869
Esben Haabendal Sept. 30, 2018, 10:29 a.m. UTC | #4
Chuanhua Han <chuanhua.han@nxp.com> writes:

> This patch fixes the problem of rxdata being equal to 0 during the XSPI
> mode transfer of the dspi controller.
> In XSPI mode, If it is not deleted, the value of rxdata will be equal
> to 0, and the data received will not be received correctly, causing the
> receiving transfer of the spi to fail.
>
> Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com>
> ---
> Changes in v2:
>  -The original patch is divided into multiple patches(the original
> patch theme is "spi: spi-fsl-dspi: Fix support for XSPI transport
> mode"),one of which is segmented.
>
>  drivers/spi/spi-fsl-dspi.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
> index 3082e72e4f6c..4dc1064bf408 100644
> --- a/drivers/spi/spi-fsl-dspi.c
> +++ b/drivers/spi/spi-fsl-dspi.c
> @@ -243,9 +243,6 @@ static void dspi_push_rx(struct fsl_dspi *dspi, u32 rxdata)
>  	if (!dspi->rx)
>  		return;
>  
> -	/* Mask of undefined bits */
> -	rxdata &= (1 << dspi->bits_per_word) - 1;

What is the dspi->bits_per_word value when your rxdata is set equal to
0?  Could this perhaps also be related to byte ordering problems?

>  	if (dspi->bytes_per_word == 1)
>  		*(u8 *)dspi->rx = rxdata;
>  	else if (dspi->bytes_per_word == 2)

/Esben
Chuanhua Han Sept. 30, 2018, 10:37 a.m. UTC | #5
> -----Original Message-----
> From: Boris Brezillon <boris.brezillon@bootlin.com>
> Sent: 2018年9月30日 18:17
> To: Chuanhua Han <chuanhua.han@nxp.com>
> Cc: broonie@kernel.org; linux-spi@vger.kernel.org;
> linux-kernel@vger.kernel.org; eha@deif.com
> Subject: Re: [PATCH v2 2/4] spi: spi-fsl-dspi: Fix delete the processing of
> undefined bitmask for rxdata
> 
> On Sun, 30 Sep 2018 10:10:14 +0000
> Chuanhua Han <chuanhua.han@nxp.com> wrote:
> 
> > > -----Original Message-----
> > > From: Boris Brezillon <boris.brezillon@bootlin.com>
> > > Sent: 2018年9月30日 18:07
> > > To: Chuanhua Han <chuanhua.han@nxp.com>
> > > Cc: broonie@kernel.org; linux-spi@vger.kernel.org;
> > > linux-kernel@vger.kernel.org; eha@deif.com
> > > Subject: Re: [PATCH v2 2/4] spi: spi-fsl-dspi: Fix delete the
> > > processing of undefined bitmask for rxdata
> > >
> > > On Sun, 30 Sep 2018 17:25:33 +0800
> > > Chuanhua Han <chuanhua.han@nxp.com> wrote:
> > >
> > > > This patch fixes the problem of rxdata being equal to 0 during the
> > > > XSPI mode transfer of the dspi controller.
> > > > In XSPI mode, If it is not deleted, the value of rxdata will be
> > > > equal to 0, and the data received will not be received correctly,
> > > > causing the receiving transfer of the spi to fail.
> > > >
> > > > Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com>
> > > > ---
> > > > Changes in v2:
> > > >  -The original patch is divided into multiple patches(the original
> > > > patch theme is "spi: spi-fsl-dspi: Fix support for XSPI transport
> > > > mode"),one of which is segmented.
> > > >
> > > >  drivers/spi/spi-fsl-dspi.c | 3 ---
> > > >  1 file changed, 3 deletions(-)
> > > >
> > > > diff --git a/drivers/spi/spi-fsl-dspi.c
> > > > b/drivers/spi/spi-fsl-dspi.c index 3082e72e4f6c..4dc1064bf408
> > > > 100644
> > > > --- a/drivers/spi/spi-fsl-dspi.c
> > > > +++ b/drivers/spi/spi-fsl-dspi.c
> > > > @@ -243,9 +243,6 @@ static void dspi_push_rx(struct fsl_dspi
> > > > *dspi, u32
> > > rxdata)
> > > >  	if (!dspi->rx)
> > > >  		return;
> > > >
> > > > -	/* Mask of undefined bits */
> > > > -	rxdata &= (1 << dspi->bits_per_word) - 1;
> > > > -
> > >
> > > Why not
> > In xspi mode, the value of rxdata after the statement is processed is equal to
> 0 no matter what data is received.
> 
> Only if dspi->bits_per_word is 0.
> 
> Actually, I just had a look, and xfer->bits_per_word should never be 0 because
> spi_validate() makes sure it's initialized [1]. Don't know where
> dpsi->bits_per_word comes from, but maybe you have a problem there
> (dpsi->bits_per_word and xfer->bits_per_word not in sync).
> 
> [1]https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Feli
> xir.bootlin.com%2Flinux%2Fv4.19-rc5%2Fsource%2Fdrivers%2Fspi%2Fspi.c%23
> L2869&amp;data=02%7C01%7Cchuanhua.han%40nxp.com%7Cd92a3b54ccf0
> 4d1c1f3208d626bde775%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C
> 0%7C636738994411369491&amp;sdata=piLwfBc0kzMhOnI5uubHYJ9tbe%2BR
> KENKbYiLrkY1c30%3D&amp;reserved=0
OK, Let me analyze it again,Thanks!
Esben Haabendal Sept. 30, 2018, 10:37 a.m. UTC | #6
Boris Brezillon <boris.brezillon@bootlin.com> writes:

> On Sun, 30 Sep 2018 10:10:14 +0000
> Chuanhua Han <chuanhua.han@nxp.com> wrote:
>
>> > -----Original Message-----
>> > From: Boris Brezillon <boris.brezillon@bootlin.com>
>> > Sent: 2018年9月30日 18:07
>> > To: Chuanhua Han <chuanhua.han@nxp.com>
>> > Cc: broonie@kernel.org; linux-spi@vger.kernel.org;
>> > linux-kernel@vger.kernel.org; eha@deif.com
>> > Subject: Re: [PATCH v2 2/4] spi: spi-fsl-dspi: Fix delete the processing of
>> > undefined bitmask for rxdata
>> > 
>> > On Sun, 30 Sep 2018 17:25:33 +0800
>> > Chuanhua Han <chuanhua.han@nxp.com> wrote:
>> >   
>> > > This patch fixes the problem of rxdata being equal to 0 during the
>> > > XSPI mode transfer of the dspi controller.
>> > > In XSPI mode, If it is not deleted, the value of rxdata will be equal
>> > > to 0, and the data received will not be received correctly, causing
>> > > the receiving transfer of the spi to fail.
>> > >
>> > > Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com>
>> > > ---
>> > > Changes in v2:
>> > >  -The original patch is divided into multiple patches(the original
>> > > patch theme is "spi: spi-fsl-dspi: Fix support for XSPI transport
>> > > mode"),one of which is segmented.
>> > >
>> > >  drivers/spi/spi-fsl-dspi.c | 3 ---
>> > >  1 file changed, 3 deletions(-)
>> > >
>> > > diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
>> > > index 3082e72e4f6c..4dc1064bf408 100644
>> > > --- a/drivers/spi/spi-fsl-dspi.c
>> > > +++ b/drivers/spi/spi-fsl-dspi.c
>> > > @@ -243,9 +243,6 @@ static void dspi_push_rx(struct fsl_dspi *dspi, u32  
>> > rxdata)  
>> > >  	if (!dspi->rx)
>> > >  		return;
>> > >
>> > > -	/* Mask of undefined bits */
>> > > -	rxdata &= (1 << dspi->bits_per_word) - 1;
>> > > -  
>> > 
>> > Why not  
>> In xspi mode, the value of rxdata after the statement is processed is equal
>> to 0 no matter what data is received.
>
> Only if dspi->bits_per_word is 0.
>
> Actually, I just had a look, and xfer->bits_per_word should never be 0
> because spi_validate() makes sure it's initialized [1]. Don't know
> where dpsi->bits_per_word comes from, but maybe you have a problem
> there (dpsi->bits_per_word and xfer->bits_per_word not in sync).
>
> [1]https://elixir.bootlin.com/linux/v4.19-rc5/source/drivers/spi/spi.c#L2869

dspi->bits_per_word = xfer->bits_per_word

https://elixir.bootlin.com/linux/v4.19-rc5/source/drivers/spi/spi-fsl-dspi.c#L697

So it should never be out of sync, and it should never be 0.

As I mentioned in another mail, I suspect what Han is observing is
caused by byte ordering, so that the mask masks the wrong data.
Maybe related to the byte-ordering fix patch.

/Esben
Boris Brezillon Sept. 30, 2018, 10:41 a.m. UTC | #7
On Sun, 30 Sep 2018 12:37:38 +0200
Esben Haabendal <esben.haabendal@gmail.com> wrote:

> Boris Brezillon <boris.brezillon@bootlin.com> writes:
> 
> > On Sun, 30 Sep 2018 10:10:14 +0000
> > Chuanhua Han <chuanhua.han@nxp.com> wrote:
> >  
> >> > -----Original Message-----
> >> > From: Boris Brezillon <boris.brezillon@bootlin.com>
> >> > Sent: 2018年9月30日 18:07
> >> > To: Chuanhua Han <chuanhua.han@nxp.com>
> >> > Cc: broonie@kernel.org; linux-spi@vger.kernel.org;
> >> > linux-kernel@vger.kernel.org; eha@deif.com
> >> > Subject: Re: [PATCH v2 2/4] spi: spi-fsl-dspi: Fix delete the processing of
> >> > undefined bitmask for rxdata
> >> > 
> >> > On Sun, 30 Sep 2018 17:25:33 +0800
> >> > Chuanhua Han <chuanhua.han@nxp.com> wrote:
> >> >     
> >> > > This patch fixes the problem of rxdata being equal to 0 during the
> >> > > XSPI mode transfer of the dspi controller.
> >> > > In XSPI mode, If it is not deleted, the value of rxdata will be equal
> >> > > to 0, and the data received will not be received correctly, causing
> >> > > the receiving transfer of the spi to fail.
> >> > >
> >> > > Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com>
> >> > > ---
> >> > > Changes in v2:
> >> > >  -The original patch is divided into multiple patches(the original
> >> > > patch theme is "spi: spi-fsl-dspi: Fix support for XSPI transport
> >> > > mode"),one of which is segmented.
> >> > >
> >> > >  drivers/spi/spi-fsl-dspi.c | 3 ---
> >> > >  1 file changed, 3 deletions(-)
> >> > >
> >> > > diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
> >> > > index 3082e72e4f6c..4dc1064bf408 100644
> >> > > --- a/drivers/spi/spi-fsl-dspi.c
> >> > > +++ b/drivers/spi/spi-fsl-dspi.c
> >> > > @@ -243,9 +243,6 @@ static void dspi_push_rx(struct fsl_dspi *dspi, u32    
> >> > rxdata)    
> >> > >  	if (!dspi->rx)
> >> > >  		return;
> >> > >
> >> > > -	/* Mask of undefined bits */
> >> > > -	rxdata &= (1 << dspi->bits_per_word) - 1;
> >> > > -    
> >> > 
> >> > Why not    
> >> In xspi mode, the value of rxdata after the statement is processed is equal
> >> to 0 no matter what data is received.  
> >
> > Only if dspi->bits_per_word is 0.
> >
> > Actually, I just had a look, and xfer->bits_per_word should never be 0
> > because spi_validate() makes sure it's initialized [1]. Don't know
> > where dpsi->bits_per_word comes from, but maybe you have a problem
> > there (dpsi->bits_per_word and xfer->bits_per_word not in sync).
> >
> > [1]https://elixir.bootlin.com/linux/v4.19-rc5/source/drivers/spi/spi.c#L2869  
> 
> dspi->bits_per_word = xfer->bits_per_word
> 
> https://elixir.bootlin.com/linux/v4.19-rc5/source/drivers/spi/spi-fsl-dspi.c#L697
> 
> So it should never be out of sync, and it should never be 0.
> 
> As I mentioned in another mail, I suspect what Han is observing is
> caused by byte ordering, so that the mask masks the wrong data.
> Maybe related to the byte-ordering fix patch.

Okay. Wait and see then.
diff mbox series

Patch

diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
index 3082e72e4f6c..4dc1064bf408 100644
--- a/drivers/spi/spi-fsl-dspi.c
+++ b/drivers/spi/spi-fsl-dspi.c
@@ -243,9 +243,6 @@  static void dspi_push_rx(struct fsl_dspi *dspi, u32 rxdata)
 	if (!dspi->rx)
 		return;
 
-	/* Mask of undefined bits */
-	rxdata &= (1 << dspi->bits_per_word) - 1;
-
 	if (dspi->bytes_per_word == 1)
 		*(u8 *)dspi->rx = rxdata;
 	else if (dspi->bytes_per_word == 2)