[U-Boot] convention for SPI dummy data?

[U-Boot] convention for SPI dummy data?

[Wolfgang Wegner]

Hi,

I have finally got MMC/SD working on my coldfire board and would like
to send some patches.

However, one of the things I had to change was the dummy data sent
out by SPI for read-only transactions. The original driver had all
zeros, for SD/MMC all ones (0xFF) is needed.

Is such a change acceptable, or is there any configuration option/flag
I could use?

Regards,
Wolfgang

[U-Boot] convention for SPI dummy data?

[Mike Frysinger]

On Friday 23 April 2010 04:43:07 Wolfgang Wegner wrote:
> However, one of the things I had to change was the dummy data sent
> out by SPI for read-only transactions. The original driver had all
> zeros, for SD/MMC all ones (0xFF) is needed.
> 
> Is such a change acceptable, or is there any configuration option/flag
> I could use?

if it isnt part of the SPI/MMC spec, use a config option named like IDLE_VAL.  
changing the default to 0xff is OK i think.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20100423/2d4d9e5f/attachment.pgp 

[U-Boot] convention for SPI dummy data?

[Wolfgang Wegner]

Hi Mike,

On Fri, Apr 23, 2010 at 11:08:10AM -0400, Mike Frysinger wrote:
> On Friday 23 April 2010 04:43:07 Wolfgang Wegner wrote:
> > However, one of the things I had to change was the dummy data sent
> > out by SPI for read-only transactions. The original driver had all
> > zeros, for SD/MMC all ones (0xFF) is needed.
> > 
> > Is such a change acceptable, or is there any configuration option/flag
> > I could use?
> 
> if it isnt part of the SPI/MMC spec, use a config option named like IDLE_VAL.  

the problem exists in the (coldfire) SPI driver, not in the MMC/SD code.
(For SD, the spec IMHO clearly states 0xFF for all idle transfers.)

> changing the default to 0xff is OK i think.

I could add CONFIG_SPI_IDLE_VAL and default it to 0x0 in the coldfire
SPI driver. In case one wants to use MMC/SD with this driver, one could
then add CONFIG_SPI_IDLE_VAL as 0xFF in the board to override it.
(Only pitfall is that the current 0x0 is used for 8- as well as 16-bit
transfers...)

ok?

> -mike

Regards,
Wolfgang

[U-Boot] convention for SPI dummy data?

[Wolfgang Wegner]

On Fri, Apr 23, 2010 at 05:18:29PM +0200, Wolfgang Wegner wrote:
> (Only pitfall is that the current 0x0 is used for 8- as well as 16-bit
> transfers...)

forget about that, the register is alway written as 16 bits.

Regards,
Wolfgang

[U-Boot] convention for SPI dummy data?

[Mike Frysinger]

On Friday 23 April 2010 11:18:29 Wolfgang Wegner wrote:
> On Fri, Apr 23, 2010 at 11:08:10AM -0400, Mike Frysinger wrote:
> > On Friday 23 April 2010 04:43:07 Wolfgang Wegner wrote:
> > > However, one of the things I had to change was the dummy data sent
> > > out by SPI for read-only transactions. The original driver had all
> > > zeros, for SD/MMC all ones (0xFF) is needed.
> > > 
> > > Is such a change acceptable, or is there any configuration option/flag
> > > I could use?
> > 
> > if it isnt part of the SPI/MMC spec, use a config option named like
> > IDLE_VAL.
> 
> the problem exists in the (coldfire) SPI driver, not in the MMC/SD code.

sorry, i thought you were proposing to fix it in the SPI/MMC driver

> (For SD, the spec IMHO clearly states 0xFF for all idle transfers.)

thanks, wasnt aware

> > changing the default to 0xff is OK i think.
> 
> I could add CONFIG_SPI_IDLE_VAL and default it to 0x0 in the coldfire
> SPI driver. In case one wants to use MMC/SD with this driver, one could
> then add CONFIG_SPI_IDLE_VAL as 0xFF in the board to override it.
> (Only pitfall is that the current 0x0 is used for 8- as well as 16-bit
> transfers...)

what i've seen so far is that the idle val doesnt matter to most spi devices.  
so on the Blackfin side, we've been defaulting to 0xff so that things do work 
"out of the box" for everyone (at least so far).
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20100423/4a83ff07/attachment.pgp 

[U-Boot] [PATCH RFC] add CONFIG_SPI_IDLE_VAL for cf_spi.c to allow use of spi_mmc

[Wolfgang Wegner]

This patch adds CONFIG_SPI_IDLE_VAL to cf_spi.c
The default setting is 0x0 to behave same as current version, in case
CONFIG_SPI_MMC is set, the value is set to 0xFFFF (all ones). In either
case, the value can be overwritten by board configuration.

Signed-off-by: Wolfgang Wegner <w.wegner@astro-kom.de>
---
 drivers/spi/cf_spi.c |   14 +++++++++++---
 1 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/cf_spi.c b/drivers/spi/cf_spi.c
index 8cc1d03..7b26f2a 100644
--- a/drivers/spi/cf_spi.c
+++ b/drivers/spi/cf_spi.c
@@ -49,6 +49,14 @@ extern void cfspi_release_bus(uint bus, uint cs);
 
 DECLARE_GLOBAL_DATA_PTR;
 
+#ifndef CONFIG_SPI_IDLE_VAL
+#if defined(CONFIG_SPI_MMC)
+#define CONFIG_SPI_IDLE_VAL	0xFFFF
+#else
+#define CONFIG_SPI_IDLE_VAL	0x0
+#endif
+#endif
+
 #if defined(CONFIG_CF_DSPI)
 /* DSPI specific mode */
 #define SPI_MODE_MOD	0x00200000
@@ -145,7 +153,7 @@ int cfspi_xfer(struct spi_slave *slave, uint bitlen, const void *dout,
 			}
 
 			if (din != NULL) {
-				cfspi_tx(ctrl, 0);
+				cfspi_tx(ctrl, CONFIG_SPI_IDLE_VAL);
 				if (cfslave->charbit == 16)
 					*spi_rd16++ = cfspi_rx();
 				else
@@ -169,7 +177,7 @@ int cfspi_xfer(struct spi_slave *slave, uint bitlen, const void *dout,
 		}
 
 		if (din != NULL) {
-			cfspi_tx(ctrl, 0);
+			cfspi_tx(ctrl, CONFIG_SPI_IDLE_VAL);
 			if (cfslave->charbit == 16)
 				*spi_rd16 = cfspi_rx();
 			else
@@ -177,7 +185,7 @@ int cfspi_xfer(struct spi_slave *slave, uint bitlen, const void *dout,
 		}
 	} else {
 		/* dummy read */
-		cfspi_tx(ctrl, 0);
+		cfspi_tx(ctrl, CONFIG_SPI_IDLE_VAL);
 		cfspi_rx();
 	}
 
-- 
1.5.6.5

[U-Boot] convention for SPI dummy data?

[Wolfgang Wegner]

On Fri, Apr 23, 2010 at 11:34:53AM -0400, Mike Frysinger wrote:
[...]
> > the problem exists in the (coldfire) SPI driver, not in the MMC/SD code.
> 
> sorry, i thought you were proposing to fix it in the SPI/MMC driver

no, as far as I can see it is always set to 0xFF there if set explicitly,
i.e. except for the read-only transfers.

> what i've seen so far is that the idle val doesnt matter to most spi devices.  
> so on the Blackfin side, we've been defaulting to 0xff so that things do work 
> "out of the box" for everyone (at least so far).

Sounds good - however I had just sent a patch implementing CONFIG_SPI_IDLE_VAL.

(I am quite hesitating to simply change such a basic default value because
I fear there might be other devices around relying on current behaviour.
On the other hand, my implementation adds 7 lines of preprocessor stuff,
which also does not look really good.)

Regards,
Wolfgang

[U-Boot] [PATCH RFC] add CONFIG_SPI_IDLE_VAL for cf_spi.c to allow use of spi_mmc

[Terje B. Nilsen]

Wolfgang Wegner <w.wegner <at> astro-kom.de> writes:

> 
> This patch adds CONFIG_SPI_IDLE_VAL to cf_spi.c
> The default setting is 0x0 to behave same as current version, in case
> CONFIG_SPI_MMC is set, the value is set to 0xFFFF (all ones). In either
> case, the value can be overwritten by board configuration.
> 
> Signed-off-by: Wolfgang Wegner <w.wegner <at> astro-kom.de>
> ---

Dear Wolfgang,

I am about to add support for SPI to SD/MMC to my Coldfire board (MCF5235 
based) both for das u-boot and for uclinux. Do you have any pointers and/or 
code you can assist with? Your words 'I have finally got MMC/SD working on my 
coldfire board' is exciting. 

Thanks for any help in advance.

Best regards,
Terje B. Nilsen