[U-Boot] [PATCH] SPI: Fix 32 bit transfers in mxc_spi.c

[U-Boot] [PATCH] SPI: Fix 32 bit transfers in mxc_spi.c

[Magnus Lilja]

Commit f9b6a1575d9f1ca192e4cb60e547aa66f08baa3f,  "i.MX31: fix SPI
driver for shorter than 32 bit" broke 32 bit transfers. This patch
makes single 32 bit transfer work again.

Tested on i.MX31 Litekit and i.MX31 PDK using 32 bit transfers to
the MC13783/ATLAS chip (using the 'date' command).

Signed-off-by: Magnus Lilja <lilja.magnus@gmail.com>
Cc: Guennadi Liakhovetski <lg@denx.de>

---

I don't think transfers larger than 32 bits will work. It seems
like they worked in the original driver, but the above commit broke that.
This patch does not try to fix that problem.

 drivers/spi/mxc_spi.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/spi/mxc_spi.c b/drivers/spi/mxc_spi.c
index fad9840..8b5d4be 100644
--- a/drivers/spi/mxc_spi.c
+++ b/drivers/spi/mxc_spi.c
@@ -142,6 +142,8 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout,
 				*(u8 *)din = data;
 			else if (bitlen < 17)
 				*(u16 *)din = data;
+			else
+				*in_l = data;
 		}
 	}
 
-- 
1.5.6

[U-Boot] [PATCH] SPI: Fix 32 bit transfers in mxc_spi.c

[Wolfgang Denk]

Dear Guennadi,

In message <1257965907-5622-1-git-send-email-lilja.magnus@gmail.com> Magnus Lilja wrote:
> Commit f9b6a1575d9f1ca192e4cb60e547aa66f08baa3f,  "i.MX31: fix SPI
> driver for shorter than 32 bit" broke 32 bit transfers. This patch
> makes single 32 bit transfer work again.
> 
> Tested on i.MX31 Litekit and i.MX31 PDK using 32 bit transfers to
> the MC13783/ATLAS chip (using the 'date' command).
> 
> Signed-off-by: Magnus Lilja <lilja.magnus@gmail.com>
> Cc: Guennadi Liakhovetski <lg@denx.de>
> 
> ---
> 
> I don't think transfers larger than 32 bits will work. It seems
> like they worked in the original driver, but the above commit broke that.
> This patch does not try to fix that problem.
> 
>  drivers/spi/mxc_spi.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/spi/mxc_spi.c b/drivers/spi/mxc_spi.c
> index fad9840..8b5d4be 100644
> --- a/drivers/spi/mxc_spi.c
> +++ b/drivers/spi/mxc_spi.c
> @@ -142,6 +142,8 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout,
>  				*(u8 *)din = data;
>  			else if (bitlen < 17)
>  				*(u16 *)din = data;
> +			else
> +				*in_l = data;
>  		}
>  	}

Could you please comment ?

Thanks in advance.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Many aligators will be slain, but the swamp will remain.

[U-Boot] [PATCH] SPI: Fix 32 bit transfers in mxc_spi.c

[Guennadi Liakhovetski]

On Wed, 2 Dec 2009, Wolfgang Denk wrote:

> Dear Guennadi,
> 
> In message <1257965907-5622-1-git-send-email-lilja.magnus@gmail.com> Magnus Lilja wrote:
> > Commit f9b6a1575d9f1ca192e4cb60e547aa66f08baa3f,  "i.MX31: fix SPI
> > driver for shorter than 32 bit" broke 32 bit transfers. This patch
> > makes single 32 bit transfer work again.
> > 
> > Tested on i.MX31 Litekit and i.MX31 PDK using 32 bit transfers to
> > the MC13783/ATLAS chip (using the 'date' command).
> > 
> > Signed-off-by: Magnus Lilja <lilja.magnus@gmail.com>
> > Cc: Guennadi Liakhovetski <lg@denx.de>
> > 
> > ---
> > 
> > I don't think transfers larger than 32 bits will work. It seems
> > like they worked in the original driver, but the above commit broke that.
> > This patch does not try to fix that problem.
> > 
> >  drivers/spi/mxc_spi.c |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/spi/mxc_spi.c b/drivers/spi/mxc_spi.c
> > index fad9840..8b5d4be 100644
> > --- a/drivers/spi/mxc_spi.c
> > +++ b/drivers/spi/mxc_spi.c
> > @@ -142,6 +142,8 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout,
> >  				*(u8 *)din = data;
> >  			else if (bitlen < 17)
> >  				*(u16 *)din = data;
> > +			else
> > +				*in_l = data;
> >  		}
> >  	}
> 
> Could you please comment ?

Hm, I'm afraid, I broke more than just that. Now that I look at this loop, 
looks like I broke not only 32-bit transfers, but also all transfers with 
bitlen > 16, and this fix is then incomplete - it doesn't fix cases with 
bitlen > 32. Magnus, looks like you also only use single-block (bitlen=32) 
transfers? Do you have a chance to test > 32-bit transfers too?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de

[U-Boot] [PATCH] SPI: Fix 32 bit transfers in mxc_spi.c

[Magnus Lilja]

2009/12/3 Guennadi Liakhovetski <lg@denx.de>:
> On Wed, 2 Dec 2009, Wolfgang Denk wrote:
>
>> Dear Guennadi,
>>
>> In message <1257965907-5622-1-git-send-email-lilja.magnus@gmail.com> Magnus Lilja wrote:
>> > diff --git a/drivers/spi/mxc_spi.c b/drivers/spi/mxc_spi.c
>> > index fad9840..8b5d4be 100644
>> > --- a/drivers/spi/mxc_spi.c
>> > +++ b/drivers/spi/mxc_spi.c
>> > @@ -142,6 +142,8 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout,
>> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? *(u8 *)din = data;
>> > ? ? ? ? ? ? ? ? ? ? else if (bitlen < 17)
>> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? *(u16 *)din = data;
>> > + ? ? ? ? ? ? ? ? ? else
>> > + ? ? ? ? ? ? ? ? ? ? ? ? ? *in_l = data;
>> > ? ? ? ? ? ? }
>> > ? ? }
>>
>> Could you please comment ?
>
> Hm, I'm afraid, I broke more than just that. Now that I look at this loop,
> looks like I broke not only 32-bit transfers, but also all transfers with
> bitlen > 16, and this fix is then incomplete - it doesn't fix cases with
> bitlen > 32. Magnus, looks like you also only use single-block (bitlen=32)
> transfers? Do you have a chance to test > 32-bit transfers too?

No, I don't have anything suitable on the SPI bus that would allow me
to test > 32-bit transfer.


Regards, Magnus

[U-Boot] [PATCH] SPI: Fix 32 bit transfers in mxc_spi.c

[Magnus Lilja]

Hi

Magnus Lilja skrev:
> 2009/12/3 Guennadi Liakhovetski <lg@denx.de>:
>> On Wed, 2 Dec 2009, Wolfgang Denk wrote:
>>
>>> Dear Guennadi,
>>>
>>> In message <1257965907-5622-1-git-send-email-lilja.magnus@gmail.com> Magnus Lilja wrote:
>>>> diff --git a/drivers/spi/mxc_spi.c b/drivers/spi/mxc_spi.c
>>>> index fad9840..8b5d4be 100644
>>>> --- a/drivers/spi/mxc_spi.c
>>>> +++ b/drivers/spi/mxc_spi.c
>>>> @@ -142,6 +142,8 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout,
>>>>                             *(u8 *)din = data;
>>>>                     else if (bitlen < 17)
>>>>                             *(u16 *)din = data;
>>>> +                   else
>>>> +                           *in_l = data;
>>>>             }
>>>>     }
>>> Could you please comment ?
>> Hm, I'm afraid, I broke more than just that. Now that I look at this loop,
>> looks like I broke not only 32-bit transfers, but also all transfers with
>> bitlen > 16, and this fix is then incomplete - it doesn't fix cases with
>> bitlen > 32. Magnus, looks like you also only use single-block (bitlen=32)
>> transfers? Do you have a chance to test > 32-bit transfers too?
> 
> No, I don't have anything suitable on the SPI bus that would allow me
> to test > 32-bit transfer.


So, what was the verdict? I can only test SPI with the ATLAS (32 bit xfers).

Can the patch be accepted even though it doesn't fix all problems or does it have to a "fix-everything"-patch?

Regards, Magnus

[U-Boot] [PATCH] SPI: Fix 32 bit transfers in mxc_spi.c

[Guennadi Liakhovetski]

On Mon, 4 Jan 2010, Magnus Lilja wrote:

> Hi
> 
> Magnus Lilja skrev:
> > 2009/12/3 Guennadi Liakhovetski <lg@denx.de>:
> >> On Wed, 2 Dec 2009, Wolfgang Denk wrote:
> >>
> >>> Dear Guennadi,
> >>>
> >>> In message <1257965907-5622-1-git-send-email-lilja.magnus@gmail.com> Magnus Lilja wrote:
> >>>> diff --git a/drivers/spi/mxc_spi.c b/drivers/spi/mxc_spi.c
> >>>> index fad9840..8b5d4be 100644
> >>>> --- a/drivers/spi/mxc_spi.c
> >>>> +++ b/drivers/spi/mxc_spi.c
> >>>> @@ -142,6 +142,8 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout,
> >>>>                             *(u8 *)din = data;
> >>>>                     else if (bitlen < 17)
> >>>>                             *(u16 *)din = data;
> >>>> +                   else
> >>>> +                           *in_l = data;
> >>>>             }
> >>>>     }
> >>> Could you please comment ?
> >> Hm, I'm afraid, I broke more than just that. Now that I look at this loop,
> >> looks like I broke not only 32-bit transfers, but also all transfers with
> >> bitlen > 16, and this fix is then incomplete - it doesn't fix cases with
> >> bitlen > 32. Magnus, looks like you also only use single-block (bitlen=32)
> >> transfers? Do you have a chance to test > 32-bit transfers too?
> > 
> > No, I don't have anything suitable on the SPI bus that would allow me
> > to test > 32-bit transfer.
> 
> 
> So, what was the verdict? I can only test SPI with the ATLAS (32 bit xfers).
> 
> Can the patch be accepted even though it doesn't fix all problems or 
> does it have to a "fix-everything"-patch?

I would prefer a proper fix, or an explicit restriction on transfer 
length.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

[U-Boot] [PATCH] SPI: Fix 32 bit transfers in mxc_spi.c

[Magnus Lilja]

Hi

Guennadi Liakhovetski skrev:
>>>> Hm, I'm afraid, I broke more than just that. Now that I look at this loop,
>>>> looks like I broke not only 32-bit transfers, but also all transfers with
>>>> bitlen > 16, and this fix is then incomplete - it doesn't fix cases with
>>>> bitlen > 32. Magnus, looks like you also only use single-block (bitlen=32)
>>>> transfers? Do you have a chance to test > 32-bit transfers too?
>>> No, I don't have anything suitable on the SPI bus that would allow me
>>> to test > 32-bit transfer.
>>
>> So, what was the verdict? I can only test SPI with the ATLAS (32 bit xfers).
>>
>> Can the patch be accepted even though it doesn't fix all problems or 
>> does it have to a "fix-everything"-patch?
> 
> I would prefer a proper fix, or an explicit restriction on transfer 
> length.

In that case it will be the latter, explicit restriction on transfer length with a printf and returning an error code from spi_xfer.

Regards, Magnus