diff mbox

[U-Boot] mx28: fix i.MX28 spi driver

Message ID 4F117435.7080807@esd.eu
State Accepted
Commit 2638b50b18ad78c8fc0433c05270d3c7d62a6e03
Delegated to: Stefano Babic
Headers show

Commit Message

Matthias Fuchs Jan. 14, 2012, 12:25 p.m. UTC
The generic spi flash driver (drivers/mtd/spi/spi_flash.c) uses the
spi low level driver's spi_xfer() function with len=0 to deassert the
SPI flash' chip select. But the i.MX28 spi driver rejects this call
due to len=0.

This patch implements an exception for len=0 with the SPI_XFER_END
flag set. This results in an extra read with the chip select being
deasserted afterwards. There seems to be no way to deassert the signal
by hand.

Signed-off-by: Matthias Fuchs <matthias.fuchs@esd.eu>
---
 drivers/spi/mxs_spi.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

Comments

Marek Vasut Jan. 14, 2012, 2:10 p.m. UTC | #1
> The generic spi flash driver (drivers/mtd/spi/spi_flash.c) uses the
> spi low level driver's spi_xfer() function with len=0 to deassert the
> SPI flash' chip select. But the i.MX28 spi driver rejects this call
> due to len=0.
> 
> This patch implements an exception for len=0 with the SPI_XFER_END
> flag set. This results in an extra read with the chip select being
> deasserted afterwards. There seems to be no way to deassert the signal
> by hand.

This seems good, but it doesn't look too correct either (is there really no 
other way to deassert CS than doing dummy read?). Do you see an issue with 
current implementation on some board please?

> 
> Signed-off-by: Matthias Fuchs <matthias.fuchs@esd.eu>
> ---
>  drivers/spi/mxs_spi.c |   12 +++++++++---
>  1 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/spi/mxs_spi.c b/drivers/spi/mxs_spi.c
> index 4c27fef..adb9ca8 100644
> --- a/drivers/spi/mxs_spi.c
> +++ b/drivers/spi/mxs_spi.c
> @@ -129,9 +129,15 @@ int spi_xfer(struct spi_slave *slave, unsigned int
> bitlen, int len = bitlen / 8;
>  	const char *tx = dout;
>  	char *rx = din;
> -
> -	if (bitlen == 0)
> -		return 0;
> +	char dummy;
> +
> +	if (bitlen == 0) {
> +		if (flags & SPI_XFER_END) {
> +			rx = &dummy;
> +			len = 1;
> +		} else
> +			return 0;
> +	}
> 
>  	if (!rx && !tx)
>  		return 0;
Matthias Fuchs Jan. 14, 2012, 6:37 p.m. UTC | #2
On 01/14/2012 03:10 PM, Marek Vasut wrote:
>> The generic spi flash driver (drivers/mtd/spi/spi_flash.c) uses the
>> spi low level driver's spi_xfer() function with len=0 to deassert the
>> SPI flash' chip select. But the i.MX28 spi driver rejects this call
>> due to len=0.
>>
>> This patch implements an exception for len=0 with the SPI_XFER_END
>> flag set. This results in an extra read with the chip select being
>> deasserted afterwards. There seems to be no way to deassert the signal
>> by hand.
> 
> This seems good, but it doesn't look too correct either (is there really no 
> other way to deassert CS than doing dummy read?). 
Not that I am aware of. Did you wrote that mxs_spi driver? How did you interpret the 
chips reference manual. I understood it this way: you need to tell the controller
to deassert chip select before the final transfer.

I could be possible to change the driver implementation and use GPIOs for 
chip select. But I think that's not the philisophy of this controller.

> Do you see an issue with 
> current implementation on some board please?
Yes, I installed an SST25VF032B SPI on the MX28EVK in preparation of our 
own board. The chip was detected correctly after power on, but erasing
(that's one place where the status register polling is used) more than one page
does not work and then after reset the device is not correctly detected.

I would expect problems on the M28EVK (Denx) also. You might want to turn 
SPI + spi_flash debugging to see it. Also I found a posting from
Alexander Keller on the u-boot list from 12/13/2011. Perhaps the can check if
this patch fixes his problems also.

Matthias
> 
>>
>> Signed-off-by: Matthias Fuchs <matthias.fuchs@esd.eu>
>> ---
>>  drivers/spi/mxs_spi.c |   12 +++++++++---
>>  1 files changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/spi/mxs_spi.c b/drivers/spi/mxs_spi.c
>> index 4c27fef..adb9ca8 100644
>> --- a/drivers/spi/mxs_spi.c
>> +++ b/drivers/spi/mxs_spi.c
>> @@ -129,9 +129,15 @@ int spi_xfer(struct spi_slave *slave, unsigned int
>> bitlen, int len = bitlen / 8;
>>  	const char *tx = dout;
>>  	char *rx = din;
>> -
>> -	if (bitlen == 0)
>> -		return 0;
>> +	char dummy;
>> +
>> +	if (bitlen == 0) {
>> +		if (flags & SPI_XFER_END) {
>> +			rx = &dummy;
>> +			len = 1;
>> +		} else
>> +			return 0;
>> +	}
>>
>>  	if (!rx && !tx)
>>  		return 0;
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
Alexander Keller Jan. 16, 2012, 11:33 p.m. UTC | #3
> On 01/14/2012 03:10 PM, Marek Vasut wrote:
> >> The generic spi flash driver (drivers/mtd/spi/spi_flash.c) uses the
> >> spi low level driver's spi_xfer() function with len=0 to deassert the
> >> SPI flash' chip select. But the i.MX28 spi driver rejects this call
> >> due to len=0.
> >>
> >> This patch implements an exception for len=0 with the SPI_XFER_END
> >> flag set. This results in an extra read with the chip select being
> >> deasserted afterwards. There seems to be no way to deassert the signal
> >> by hand.
> > 
> > This seems good, but it doesn't look too correct either (is there really
> no 
> > other way to deassert CS than doing dummy read?). 
> Not that I am aware of. Did you wrote that mxs_spi driver? How did you
> interpret the 
> chips reference manual. I understood it this way: you need to tell the
> controller
> to deassert chip select before the final transfer.
> 
> I could be possible to change the driver implementation and use GPIOs for 
> chip select. But I think that's not the philisophy of this controller.
> 
> > Do you see an issue with 
> > current implementation on some board please?
> Yes, I installed an SST25VF032B SPI on the MX28EVK in preparation of our 
> own board. The chip was detected correctly after power on, but erasing
> (that's one place where the status register polling is used) more than one
> page
> does not work and then after reset the device is not correctly detected.
> 
> I would expect problems on the M28EVK (Denx) also. You might want to turn 
> SPI + spi_flash debugging to see it. Also I found a posting from
> Alexander Keller on the u-boot list from 12/13/2011. Perhaps the can check

I just want to confirm and thanks to Matthias for this patch. I already stated my problems by writing the SPI flash ...

So, I use the IMX28EVK with a Winbond W25Q64CV SPI flash chip and it's working great now.

Thanks for the great work.

Alexander


> if
> this patch fixes his problems also.
> 
> Matthias
> > 
> >>
> >> Signed-off-by: Matthias Fuchs <matthias.fuchs@esd.eu>
> >> ---
> >>  drivers/spi/mxs_spi.c |   12 +++++++++---
> >>  1 files changed, 9 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/spi/mxs_spi.c b/drivers/spi/mxs_spi.c
> >> index 4c27fef..adb9ca8 100644
> >> --- a/drivers/spi/mxs_spi.c
> >> +++ b/drivers/spi/mxs_spi.c
> >> @@ -129,9 +129,15 @@ int spi_xfer(struct spi_slave *slave, unsigned int
> >> bitlen, int len = bitlen / 8;
> >>  	const char *tx = dout;
> >>  	char *rx = din;
> >> -
> >> -	if (bitlen == 0)
> >> -		return 0;
> >> +	char dummy;
> >> +
> >> +	if (bitlen == 0) {
> >> +		if (flags & SPI_XFER_END) {
> >> +			rx = &dummy;
> >> +			len = 1;
> >> +		} else
> >> +			return 0;
> >> +	}
> >>
> >>  	if (!rx && !tx)
> >>  		return 0;
> > _______________________________________________
> > U-Boot mailing list
> > U-Boot@lists.denx.de
> > http://lists.denx.de/mailman/listinfo/u-boot
> 
> 
> -- 
> -------------------------------------------------------------------------
> Dipl.-Ing. Matthias Fuchs
> Head of System Design
> 
> esd electronic system design gmbh
> Vahrenwalder Str. 207 - 30165 Hannover - GERMANY
> Phone: +49-511-37298-0 - Fax: +49-511-37298-68
> Please visit our homepage http://www.esd.eu
> Quality Products - Made in Germany
> -------------------------------------------------------------------------
> Geschäftsführer: Klaus Detering
> Amtsgericht Hannover HRB 51373 - VAT-ID DE 115672832
> -------------------------------------------------------------------------
Fabio Estevam Jan. 17, 2012, 7:34 p.m. UTC | #4
On 1/16/12, Alexander Keller <alexander.keller.85@gmx.de> wrote:

> I just want to confirm and thanks to Matthias for this patch. I already
> stated my problems by writing the SPI flash ...
>
> So, I use the IMX28EVK with a Winbond W25Q64CV SPI flash chip and it's
> working great now.
>
> Thanks for the great work.

Ok, great. Can you or Matthias post a patch adding SPI flash support to mx28evk?

I have just soldered one SPI flash on my board and would be glad to try it.

Regards,

Fabio Estevam
Fabio Estevam Jan. 23, 2012, 7:52 p.m. UTC | #5
On 1/14/12, Matthias Fuchs <matthias.fuchs@esd.eu> wrote:
> The generic spi flash driver (drivers/mtd/spi/spi_flash.c) uses the
> spi low level driver's spi_xfer() function with len=0 to deassert the
> SPI flash' chip select. But the i.MX28 spi driver rejects this call
> due to len=0.
>
> This patch implements an exception for len=0 with the SPI_XFER_END
> flag set. This results in an extra read with the chip select being
> deasserted afterwards. There seems to be no way to deassert the signal
> by hand.
>
> Signed-off-by: Matthias Fuchs <matthias.fuchs@esd.eu>

Tested-by: Fabio Estevam <fabio.estevam@freescale.com>

Soldered a SST25VF016B on a mx28evk (and also the SPI pullups) and
verified that the flash can be erased succesfully now.

I suggest that this patch gets applied as it fixes a real issue.

Thanks,

Fabio Estevam
Marek Vasut Jan. 23, 2012, 7:53 p.m. UTC | #6
> On 1/14/12, Matthias Fuchs <matthias.fuchs@esd.eu> wrote:
> > The generic spi flash driver (drivers/mtd/spi/spi_flash.c) uses the
> > spi low level driver's spi_xfer() function with len=0 to deassert the
> > SPI flash' chip select. But the i.MX28 spi driver rejects this call
> > due to len=0.
> > 
> > This patch implements an exception for len=0 with the SPI_XFER_END
> > flag set. This results in an extra read with the chip select being
> > deasserted afterwards. There seems to be no way to deassert the signal
> > by hand.
> > 
> > Signed-off-by: Matthias Fuchs <matthias.fuchs@esd.eu>
> 
> Tested-by: Fabio Estevam <fabio.estevam@freescale.com>
> 
> Soldered a SST25VF016B on a mx28evk (and also the SPI pullups) and
> verified that the flash can be erased succesfully now.
> 
> I suggest that this patch gets applied as it fixes a real issue.
> 
> Thanks,
> 
> Fabio Estevam

I'm all for it.

M
Stefano Babic Jan. 24, 2012, 8:40 a.m. UTC | #7
On 14/01/2012 13:25, Matthias Fuchs wrote:
> The generic spi flash driver (drivers/mtd/spi/spi_flash.c) uses the
> spi low level driver's spi_xfer() function with len=0 to deassert the
> SPI flash' chip select. But the i.MX28 spi driver rejects this call
> due to len=0.
> 
> This patch implements an exception for len=0 with the SPI_XFER_END
> flag set. This results in an extra read with the chip select being
> deasserted afterwards. There seems to be no way to deassert the signal
> by hand.
> 
> Signed-off-by: Matthias Fuchs <matthias.fuchs@esd.eu>
> ---

Applied to u-boot-imx, thanks.

Best regards,
Stefano Babic
Fabio Estevam Jan. 28, 2012, 10:08 p.m. UTC | #8
Hi Stefano,

On Tue, Jan 24, 2012 at 6:40 AM, Stefano Babic <sbabic@denx.de> wrote:

> Applied to u-boot-imx, thanks.

Which branch, please?

I am not able to find it.

Regards,

Fabio Estevam
Stefano Babic Jan. 29, 2012, 9:12 a.m. UTC | #9
On 28/01/2012 23:08, Fabio Estevam wrote:
> Hi Stefano,
> 
> On Tue, Jan 24, 2012 at 6:40 AM, Stefano Babic <sbabic@denx.de> wrote:
> 
>> Applied to u-boot-imx, thanks.
> 

Hi Fabio,

> Which branch, please?
> 
> I am not able to find it.

Sorry, I have not yet pushed to the server - done now.

Best regards,
Stefano Babic
diff mbox

Patch

diff --git a/drivers/spi/mxs_spi.c b/drivers/spi/mxs_spi.c
index 4c27fef..adb9ca8 100644
--- a/drivers/spi/mxs_spi.c
+++ b/drivers/spi/mxs_spi.c
@@ -129,9 +129,15 @@  int spi_xfer(struct spi_slave *slave, unsigned int bitlen,
 	int len = bitlen / 8;
 	const char *tx = dout;
 	char *rx = din;
-
-	if (bitlen == 0)
-		return 0;
+	char dummy;
+
+	if (bitlen == 0) {
+		if (flags & SPI_XFER_END) {
+			rx = &dummy;
+			len = 1;
+		} else
+			return 0;
+	}
 
 	if (!rx && !tx)
 		return 0;