[U-Boot] [PATCH] SPI: Fix 32 bit transfers in mxc_spi.c
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; } }
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
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@denx.de
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
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
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/
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
participants (4)
-
Guennadi Liakhovetski -
Guennadi Liakhovetski -
Magnus Lilja -
Wolfgang Denk