convention for SPI dummy data?

U-Boot New Wolfgang Wegner 4 participants 8 messages Apr 23, 2010 – Jun 13, 2011 SPI

lore

Share & tools

lore

Wolfgang Wegner

author

2010-04-23 08:43 UTC #0 lore raw

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

Mike Frysinger

2010-04-23 15:08 UTC #1 lore raw

Quoted 6 lines · 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

Wolfgang Wegner

author

2010-04-23 15:18 UTC #2 lore raw

Hi Mike,

Quoted 9 lines · Mike Frysinger

Quoted 6 lines · 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.)

Quoted 1 line

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?

Quoted 1 line

-mike

Regards,
Wolfgang

Wolfgang Wegner

author

2010-04-23 15:34 UTC #3 lore raw

Quoted 2 lines · Wolfgang Wegner

(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

Mike Frysinger

2010-04-23 15:34 UTC #4 lore raw

In reply to Wolfgang Wegner

Quoted 6 lines · On Friday 23 April 2010 11:18:29 Wolfgang Wegner wrote:

Quoted 5 lines · Mike Frysinger
Quoted 4 lines · 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.

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
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

Quoted 1 line

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

thanks, wasnt aware

Quoted 7 lines

Quoted 1 line
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

Wolfgang Wegner

author

2010-04-23 15:49 UTC #5 lore raw

On Fri, Apr 23, 2010 at 11:34:53AM -0400, Mike Frysinger wrote:
[...]

Quoted 3 lines

Quoted 1 line
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.

Quoted 3 lines

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

Wolfgang Wegner

2010-04-23 15:43 UTC #6 lore raw

RFC PATCH 1/1 add CONFIG_SPI_IDLE_VAL for cf_spi.c to allow use of spi_mmc

In reply to Mike Frysinger

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 <[email protected]>

---
 drivers/spi/cf_spi.c |   14 +++++++++++---
 1 files changed, 11 insertions(+), 3 deletions(-)

drivers/spi/cf_spi.c +11 -3

index 8cc1d03..7b26f2a 100644
@@ -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();
 	}

Signature · 1 line

1.5.6.5

Terje B. Nilsen

2011-06-13 18:27 UTC #7 lore raw

RFC PATCH 1/1 add CONFIG_SPI_IDLE_VAL for cf_spi.c to allow use of spi_mmc

Quoted 8 lines · 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