[patch 0/4] MMC-over-SPI for 2.6.22-rc4-mm

[patch 0/4] MMC-over-SPI for 2.6.22-rc4-mm

[David Brownell]

Here's the latest, which (modulo review issues) should be mergeable
against the latest git-mmc code (plus the crc7 patch, also in mm;
and the 4wire patch).

 - headers learn about SPI ... a minor change from the last version,
   removing that "R1D" message type hack

 - mmc_block learns about SPI ... no change

 - mmc core updates ... fixed merge conflicts with git-mmc, updated
   the CID/CSD handling, the new card unlock/code has been updated
   to know about SPI

 - mmc_spi host dirver ... cleanups from those CID/CSD updates,
   bugfixes for chipselect handling, started cleanup of DMA code

I ran into some failures triggered when CRCs were enabled.  The
trigger may be platform-specific (e.g. GCC bugs).  The flakey
non-recovery is a different issue altogether.

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

[patch 1/4] MMC-over-SPI header updates

[David Brownell]

Teach the MMC/SD/SDIO system headers that some hosts use SPI mode

 - New host capabilities bits
    * MMC_CAP_SPI, with mmc_host_is_spi() test
    * MMC_CAP_SPI_CRC, if it's ready to use CRCs
 
 - SPI-specific declarations:
    * Response types, MMC_RSP_SPI_R*
    * Two SPI-only commands 
    * Status bits used native to SPI:  R1_SPI_*, R2_SPI_*

 - Fix a few (unrelated) whitespace bugs in the headers.

None of these changes affects current code.

Signed-off-by: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>

---
 include/linux/mmc/core.h |   21 ++++++++++++++++++--
 include/linux/mmc/host.h |    4 +++
 include/linux/mmc/mmc.h  |   48 ++++++++++++++++++++++++++++++++++++++++-------
 3 files changed, 64 insertions(+), 9 deletions(-)

--- pxa.orig/include/linux/mmc/host.h	2007-06-06 16:48:44.000000000 -0700
+++ pxa/include/linux/mmc/host.h	2007-06-10 13:00:23.000000000 -0700
@@ -90,6 +90,8 @@ struct mmc_host {
 #define MMC_CAP_BYTEBLOCK	(1 << 2)	/* Can do non-log2 block sizes */
 #define MMC_CAP_MMC_HIGHSPEED	(1 << 3)	/* Can do MMC high-speed timing */
 #define MMC_CAP_SD_HIGHSPEED	(1 << 4)	/* Can do SD high-speed timing */
+#define MMC_CAP_SPI		(1 << 5)	/* Talks only SPI protocols */
+#define MMC_CAP_SPI_CRC		(1 << 6)	/* Handles CRC option in SPI */
 
 	/* host specific block data */
 	unsigned int		max_seg_size;	/* see blk_queue_max_segment_size */
@@ -137,6 +139,8 @@ static inline void *mmc_priv(struct mmc_
 	return (void *)host->private;
 }
 
+#define mmc_host_is_spi(host)	((host)->caps & MMC_CAP_SPI)
+
 #define mmc_dev(x)	((x)->parent)
 #define mmc_classdev(x)	(&(x)->class_dev)
 #define mmc_hostname(x)	((x)->class_dev.bus_id)
--- pxa.orig/include/linux/mmc/core.h	2007-06-06 16:48:44.000000000 -0700
+++ pxa/include/linux/mmc/core.h	2007-06-10 13:00:23.000000000 -0700
@@ -25,14 +25,19 @@ struct mmc_command {
 #define MMC_RSP_CRC	(1 << 2)		/* expect valid crc */
 #define MMC_RSP_BUSY	(1 << 3)		/* card may send busy */
 #define MMC_RSP_OPCODE	(1 << 4)		/* response contains opcode */
-#define MMC_CMD_MASK	(3 << 5)		/* command type */
+
+#define MMC_CMD_MASK	(3 << 5)		/* non-SPI command type */
 #define MMC_CMD_AC	(0 << 5)
 #define MMC_CMD_ADTC	(1 << 5)
 #define MMC_CMD_BC	(2 << 5)
 #define MMC_CMD_BCR	(3 << 5)
 
+#define MMC_RSP_SPI_S1	(1 << 7)		/* one status byte */
+#define MMC_RSP_SPI_S2	(1 << 8)		/* second status byte */
+#define MMC_RSP_SPI_OCR	(1 << 9)		/* OCR */
+
 /*
- * These are the response types, and correspond to valid bit
+ * These are the native response types, and correspond to valid bit
  * patterns of the above flags.  One additional valid pattern
  * is all zeros, which means we don't expect a response.
  */
@@ -47,6 +52,18 @@ struct mmc_command {
 #define mmc_resp_type(cmd)	((cmd)->flags & (MMC_RSP_PRESENT|MMC_RSP_136|MMC_RSP_CRC|MMC_RSP_BUSY|MMC_RSP_OPCODE))
 
 /*
+ * These are the SPI response types.  Commands return R1, with maybe
+ * more info.  Zero is an invalid type, meaning that the caller
+ * forgot to say which response type applies to this command.
+ */
+#define MMC_RSP_SPI_R1	(MMC_RSP_SPI_S1)
+#define MMC_RSP_SPI_R1B	(MMC_RSP_SPI_S1|MMC_RSP_BUSY)
+#define MMC_RSP_SPI_R2	(MMC_RSP_SPI_S1|MMC_RSP_SPI_S2)
+#define MMC_RSP_SPI_R3	(MMC_RSP_SPI_S1|MMC_RSP_SPI_OCR)
+
+#define mmc_spi_resp_type(cmd)	((cmd)->flags & (MMC_RSP_SPI_S1|MMC_RSP_BUSY|MMC_RSP_SPI_S2|MMC_RSP_SPI_OCR))
+
+/*
  * These are the command types.
  */
 #define mmc_cmd_type(cmd)	((cmd)->flags & MMC_CMD_MASK)
--- pxa.orig/include/linux/mmc/mmc.h	2007-06-10 13:00:21.000000000 -0700
+++ pxa/include/linux/mmc/mmc.h	2007-06-10 13:00:23.000000000 -0700
@@ -27,7 +27,7 @@
 
 /* Standard MMC commands (4.1)           type  argument     response */
    /* class 1 */
-#define	MMC_GO_IDLE_STATE         0   /* bc                          */
+#define MMC_GO_IDLE_STATE         0   /* bc                          */
 #define MMC_SEND_OP_COND          1   /* bcr  [31:0] OCR         R3  */
 #define MMC_ALL_SEND_CID          2   /* bcr                     R2  */
 #define MMC_SET_RELATIVE_ADDR     3   /* ac   [31:16] RCA        R1  */
@@ -39,8 +39,10 @@
 #define MMC_SEND_CID             10   /* ac   [31:16] RCA        R2  */
 #define MMC_READ_DAT_UNTIL_STOP  11   /* adtc [31:0] dadr        R1  */
 #define MMC_STOP_TRANSMISSION    12   /* ac                      R1b */
-#define MMC_SEND_STATUS	         13   /* ac   [31:16] RCA        R1  */
+#define MMC_SEND_STATUS          13   /* ac   [31:16] RCA        R1  */
 #define MMC_GO_INACTIVE_STATE    15   /* ac   [31:16] RCA            */
+#define MMC_SPI_READ_OCR         58   /* spi                  spi_R3 */
+#define MMC_SPI_CRC_ON_OFF       59   /* spi  [0:0] flag      spi_R1 */
 
   /* class 2 */
 #define MMC_SET_BLOCKLEN         16   /* ac   [31:0] block len   R1  */
@@ -90,15 +92,15 @@
  */
 
 /*
-  MMC status in R1
+  MMC status in R1, for native mode (SPI bits are different)
   Type
-  	e : error bit
+	e : error bit
 	s : status bit
 	r : detected and set for the actual command response
 	x : detected and set during command execution. the host must poll
             the card by sending status command in order to read these bits.
   Clear condition
-  	a : according to the card state
+	a : according to the card state
 	b : always related to the previous command. Reception of
             a valid command will clear it (with a delay of one command)
 	c : clear by read
@@ -124,10 +126,42 @@
 #define R1_CARD_ECC_DISABLED	(1 << 14)	/* sx, a */
 #define R1_ERASE_RESET		(1 << 13)	/* sr, c */
 #define R1_STATUS(x)            (x & 0xFFFFE000)
-#define R1_CURRENT_STATE(x)    	((x & 0x00001E00) >> 9)	/* sx, b (4 bits) */
+#define R1_CURRENT_STATE(x)	((x & 0x00001E00) >> 9)	/* sx, b (4 bits) */
 #define R1_READY_FOR_DATA	(1 << 8)	/* sx, a */
 #define R1_APP_CMD		(1 << 5)	/* sr, c */
 
+#define R1_STATE_IDLE		0		/* resetting; maybe opendrain */
+#define R1_STATE_READY		1		/* opendrain; ident soon */
+#define R1_STATE_IDENT		2		/* opendrain; setaddr soon */
+#define R1_STATE_STBY		3		/* addressed, ready to use */
+#define R1_STATE_TRAN		4
+#define R1_STATE_DATA		5
+#define R1_STATE_RCV		6
+#define R1_STATE_PRG		7
+#define R1_STATE_DIS		8
+#define R1_STATE(x)		(((x) & 0xf) << 9)
+
+/*
+ * MMC/SD in SPI mode reports R1 status always, and R2 for SEND_STATUS
+ */
+#define R1_SPI_IDLE		(1 << 0)
+#define R1_SPI_ERASE_RESET	(1 << 1)
+#define R1_SPI_ILLEGAL_COMMAND	(1 << 2)
+#define R1_SPI_COM_CRC		(1 << 3)
+#define R1_SPI_ERASE_SEQ	(1 << 4)
+#define R1_SPI_ADDRESS		(1 << 5)
+#define R1_SPI_PARAMETER	(1 << 6)
+
+#define R2_SPI_CARD_LOCKED	(1 << 0)
+#define R2_SPI_WP_ERASE_SKIP	(1 << 1)	/* or lock/unlock fail */
+#define R2_SPI_ERROR		(1 << 2)
+#define R2_SPI_CC_ERROR		(1 << 3)
+#define R2_SPI_CARD_ECC_ERROR	(1 << 4)
+#define R2_SPI_WP_VIOLATION	(1 << 5)
+#define R2_SPI_ERASE_PARAM	(1 << 6)
+#define R2_SPI_OUT_OF_RANGE	(1 << 7)	/* or CSD overwrite */
+
+
 /* These are unpacked versions of the actual responses */
 
 struct _mmc_csd {
@@ -181,7 +215,7 @@ struct _mmc_csd {
  * Card Command Classes (CCC)
  */
 #define CCC_BASIC		(1<<0)	/* (0) Basic protocol functions */
-					/* (CMD0,1,2,3,4,7,9,10,12,13,15) */
+					/* (CMD0,1,2,3,4,7,9,10,12,13,15,58,59) */
 #define CCC_STREAM_READ		(1<<1)	/* (1) Stream read commands */
 					/* (CMD11) */
 #define CCC_BLOCK_READ		(1<<2)	/* (2) Block read commands */

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [patch 1/4] MMC-over-SPI header updates

[Pierre Ossman]

David Brownell wrote:
> Teach the MMC/SD/SDIO system headers that some hosts use SPI mode
> 
>  - New host capabilities bits
>     * MMC_CAP_SPI, with mmc_host_is_spi() test
>     * MMC_CAP_SPI_CRC, if it's ready to use CRCs
>  
>  - SPI-specific declarations:
>     * Response types, MMC_RSP_SPI_R*
>     * Two SPI-only commands 
>     * Status bits used native to SPI:  R1_SPI_*, R2_SPI_*
> 
>  - Fix a few (unrelated) whitespace bugs in the headers.
> 
> None of these changes affects current code.
> 
> Signed-off-by: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> 
> ---
>  include/linux/mmc/core.h |   21 ++++++++++++++++++--
>  include/linux/mmc/host.h |    4 +++
>  include/linux/mmc/mmc.h  |   48 ++++++++++++++++++++++++++++++++++++++++-------
>  3 files changed, 64 insertions(+), 9 deletions(-)
> 
> --- pxa.orig/include/linux/mmc/host.h	2007-06-06 16:48:44.000000000 -0700
> +++ pxa/include/linux/mmc/host.h	2007-06-10 13:00:23.000000000 -0700
> @@ -90,6 +90,8 @@ struct mmc_host {
>  #define MMC_CAP_BYTEBLOCK	(1 << 2)	/* Can do non-log2 block sizes */
>  #define MMC_CAP_MMC_HIGHSPEED	(1 << 3)	/* Can do MMC high-speed timing */
>  #define MMC_CAP_SD_HIGHSPEED	(1 << 4)	/* Can do SD high-speed timing */
> +#define MMC_CAP_SPI		(1 << 5)	/* Talks only SPI protocols */
> +#define MMC_CAP_SPI_CRC		(1 << 6)	/* Handles CRC option in SPI */
>  
>  	/* host specific block data */
>  	unsigned int		max_seg_size;	/* see blk_queue_max_segment_size */

I still don't see the point of the SPI_CRC cap. It's not like we will have
multiple SPI host drivers, so either we support CRC or we don't. No need to have
this dynamic.

>  
> +#define MMC_RSP_SPI_S1	(1 << 7)		/* one status byte */
> +#define MMC_RSP_SPI_S2	(1 << 8)		/* second status byte */
> +#define MMC_RSP_SPI_OCR	(1 << 9)		/* OCR */
> +

You should describe form, not contents. The host driver shouldn't care what's in
the chunks, only how they need to be transferred. So size and things like CRC,
but not their meaning.

>  
> +#define R1_STATE_IDLE		0		/* resetting; maybe opendrain */
> +#define R1_STATE_READY		1		/* opendrain; ident soon */
> +#define R1_STATE_IDENT		2		/* opendrain; setaddr soon */
> +#define R1_STATE_STBY		3		/* addressed, ready to use */
> +#define R1_STATE_TRAN		4
> +#define R1_STATE_DATA		5
> +#define R1_STATE_RCV		6
> +#define R1_STATE_PRG		7
> +#define R1_STATE_DIS		8
> +#define R1_STATE(x)		(((x) & 0xf) << 9)
> +

This seems to be for the mapping voodoo in the spi host. And that is not
something we want, so this is not needed.

Rgds
-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [patch 1/4] MMC-over-SPI header updates

[David Brownell]

On Tuesday 12 June 2007, Pierre Ossman wrote:
> David Brownell wrote:
> > Teach the MMC/SD/SDIO system headers that some hosts use SPI mode
> > 
> > ...
> > 
> > --- pxa.orig/include/linux/mmc/host.h	2007-06-06 16:48:44.000000000 -0700
> > +++ pxa/include/linux/mmc/host.h	2007-06-10 13:00:23.000000000 -0700
> > @@ -90,6 +90,8 @@ struct mmc_host {
> >  #define MMC_CAP_BYTEBLOCK	(1 << 2)	/* Can do non-log2 block sizes */
> >  #define MMC_CAP_MMC_HIGHSPEED	(1 << 3)	/* Can do MMC high-speed timing */
> >  #define MMC_CAP_SD_HIGHSPEED	(1 << 4)	/* Can do SD high-speed timing */
> > +#define MMC_CAP_SPI		(1 << 5)	/* Talks only SPI protocols */
> > +#define MMC_CAP_SPI_CRC		(1 << 6)	/* Handles CRC option in SPI */
> >  
> >  	/* host specific block data */
> >  	unsigned int		max_seg_size;	/* see blk_queue_max_segment_size */
> 
> I still don't see the point of the SPI_CRC cap. It's not like we will have
> multiple SPI host drivers, so either we support CRC or we don't. No need to have
> this dynamic.

The point is that this way we can keep the host driver out of the business
of deciding whether or not to enable CRC mode.  The main alternative would
be to move that module parameter into the MMC core.


> > +#define MMC_RSP_SPI_S1	(1 << 7)		/* one status byte */
> > +#define MMC_RSP_SPI_S2	(1 << 8)		/* second status byte */
> > +#define MMC_RSP_SPI_OCR	(1 << 9)		/* OCR */
> > +
> 
> You should describe form, not contents. The host driver shouldn't care what's in
> the chunks, only how they need to be transferred. So size and things like CRC,
> but not their meaning.

This directly matches every MMC spec I've seen ... they define the response
format in those terms.  And I made this change per your request ... the host
driver checks for "R1", "R2", etc which are built up from those components.

I'll note that the "native" bits include meaning too ... opcode is most
certainly meaning, as is CRC.


> > +#define R1_STATE_IDLE		0		/* resetting; maybe opendrain */
> > +#define R1_STATE_READY		1		/* opendrain; ident soon */
> > +#define R1_STATE_IDENT		2		/* opendrain; setaddr soon */
> > +#define R1_STATE_STBY		3		/* addressed, ready to use */
> > +#define R1_STATE_TRAN		4
> > +#define R1_STATE_DATA		5
> > +#define R1_STATE_RCV		6
> > +#define R1_STATE_PRG		7
> > +#define R1_STATE_DIS		8
> > +#define R1_STATE(x)		(((x) & 0xf) << 9)
> > +
> 
> This seems to be for the mapping voodoo in the spi host. And that is not
> something we want, so this is not needed.

OK, I can remove that stuff now that the raw SPI status bytes are being
returned in resp[] ... I'll have to update the spi_mmc driver though,
and I'll hold off on that for a bit since I expect you'll send comments
specifically on that driver.

- Dave


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [patch 1/4] MMC-over-SPI header updates

[Pierre Ossman]

David Brownell wrote:
> 
> The point is that this way we can keep the host driver out of the business
> of deciding whether or not to enable CRC mode.  The main alternative would
> be to move that module parameter into the MMC core.
> 

Why would we want to turn it off?

> 
> This directly matches every MMC spec I've seen ... they define the response
> format in those terms.  And I made this change per your request ... the host
> driver checks for "R1", "R2", etc which are built up from those components.
> 

The components we defined for native mode cannot be found in the specs, we
derived them from the descriptions. So the same thing goes for SPI.

The point of this exercise is that we do not have to modify host drivers when
new response types show up (which it has when MMC got extended to SD, and SD to
SDIO).

So when R666 shows up, which happens to contain the anti-christ and be 5 bytes
long, we can describe this using existing defines and not have to modify the
host drivers. Having a name like "OCR" then does not describe properly what we
want the host to do.

> I'll note that the "native" bits include meaning too ... opcode is most
> certainly meaning, as is CRC.
> 

Granted, this is a bit fuzzy in some areas. These two are sometimes not reported
by hw, we can only tell it to check that they are correct. And there is no real
variation what we want to do with these values. Compare that to the payload
where it is difficult, if not impossible, for the host driver to determine what
we want to do with it.

Just trust me on this, we do _not_ want semantics in the host drivers. It has
caused problems with at least three host drivers so far, and omap is still
unable to use SD cards because it tries to parse responses by itself.

The rule of thumb is, data the core wants back should not be parsed by the host
driver. Things the core just wants validated is ok to parse in the host driver.

> 
> OK, I can remove that stuff now that the raw SPI status bytes are being
> returned in resp[] ... I'll have to update the spi_mmc driver though,
> and I'll hold off on that for a bit since I expect you'll send comments
> specifically on that driver.
> 

Great. I'll ignore the mapping parts in there for now then and review the rest
of the code.

Rgds
-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [patch 1/4] MMC-over-SPI header updates

[David Brownell]

On Wednesday 13 June 2007, Pierre Ossman wrote:
> David Brownell wrote:
> > 
> > The point is that this way we can keep the host driver out of the business
> > of deciding whether or not to enable CRC mode.  The main alternative would
> > be to move that module parameter into the MMC core.
> 
> Why would we want to turn it off?

It starts "off", and needs to be turned "on".  Reasons to leave it
off include performance (I measured over 30% slower with CRCs on while
writing a 120+ MB SD partition), and of course the fact that the CRC
stuff is the very newest feature in the code (thanks Jan!), and is not
yet as trustworthy (viz. the intermittent bug I noted).


> > This directly matches every MMC spec I've seen ... they define the response
> > format in those terms.  And I made this change per your request ... the host
> > driver checks for "R1", "R2", etc which are built up from those components.
> > 
> 
> The components we defined for native mode cannot be found in the specs, we
> derived them from the descriptions. So the same thing goes for SPI.

Umm ... no, not really; did you cross-check with the SPI specs?  All
the requests return one status byte.  One returns a second one.  One
returns the OCR in the place that second byte would be.  Some leave
the chip in a "busy" state.  Those are the components out of which
the SPI_R1, SPI_R1B, SPI_R2, and SPI_R3 are built.  (R3 is the one
which includes OCR.)


> The point of this exercise is that we do not have to modify host drivers when
> new response types show up (which it has when MMC got extended to SD, and SD to
> SDIO).

That seems like a fantasy to me.  We don't know in advance how those
types will be defined (or if they even will be!), so there's no way
we can know whether or not new types will need to be defined.

In fact it's almost certain that we *would* have to change that low
level protocol code to understand additional response types.  The
handling for them has to be hand coded in all cases...


> So when R666 shows up, which happens to contain the anti-christ and be 5 bytes
> long, we can describe this using existing defines and not have to modify the
> host drivers. Having a name like "OCR" then does not describe properly what we
> want the host to do.

Leaving the semantic question of where those 5 bytes would need to be
stored.  Clearly since it was defined as a new response type, the answer
used for R3 would not be appropriate... and R3 is the only 5 byte response
currently defined.


> > I'll note that the "native" bits include meaning too ... opcode is most
> > certainly meaning, as is CRC.
> > 
> 
> Granted, this is a bit fuzzy in some areas. These two are sometimes not reported
> by hw, we can only tell it to check that they are correct. And there is no real
> variation what we want to do with these values. Compare that to the payload
> where it is difficult, if not impossible, for the host driver to determine what
> we want to do with it.

You're missing the point that those values you're objecting to are fully
defined as part of the protocol, so we *DO* know exactly what to do ...

 
> Just trust me on this, we do _not_ want semantics in the host drivers. It has
> caused problems with at least three host drivers so far, and omap is still
> unable to use SD cards because it tries to parse responses by itself.

This isn't the OMAP driver though.  And I can't exactly make sense of the
guts of your objection (see next).

 
> The rule of thumb is, data the core wants back should not be parsed by the host
> driver. Things the core just wants validated is ok to parse in the host driver.

That's a fine policy.  (Though I've observed that most policies driving
interface design need to be broken sometimes.)

You've objected surprisingly vociferously ... given that so far as I
understand what you mean, that's the exact rule now followed in the
mmc_spi host driver!

 
> > 
> > OK, I can remove that stuff now that the raw SPI status bytes are being
> > returned in resp[] ... I'll have to update the spi_mmc driver though,
> > and I'll hold off on that for a bit since I expect you'll send comments
> > specifically on that driver.
> > 
> 
> Great. I'll ignore the mapping parts in there for now then and review the rest
> of the code.

Well, the R1_STATE bits of the mapping.

- Dave


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [patch 1/4] MMC-over-SPI header updates

[Pierre Ossman]

David Brownell wrote:
> 
> It starts "off", and needs to be turned "on".  Reasons to leave it
> off include performance (I measured over 30% slower with CRCs on while
> writing a 120+ MB SD partition), and of course the fact that the CRC
> stuff is the very newest feature in the code (thanks Jan!), and is not
> yet as trustworthy (viz. the intermittent bug I noted).
> 

Any CRC bugs should surface rather quickly, so that shouldn't be an isse.
Performance might be though, but do people really feel that those 30% is worth
losing CRC? I sure wouldn't feel comfortable with that.

> 
> Umm ... no, not really; did you cross-check with the SPI specs?  All
> the requests return one status byte.  One returns a second one.  One
> returns the OCR in the place that second byte would be.  Some leave
> the chip in a "busy" state.  Those are the components out of which
> the SPI_R1, SPI_R1B, SPI_R2, and SPI_R3 are built.  (R3 is the one
> which includes OCR.)
> 

But the controller will be fine just knowing "one byte", "two bytes" and "five
bytes" respectively.

> 
> That seems like a fantasy to me.  We don't know in advance how those
> types will be defined (or if they even will be!), so there's no way
> we can know whether or not new types will need to be defined.
> 
> In fact it's almost certain that we *would* have to change that low
> level protocol code to understand additional response types.  The
> handling for them has to be hand coded in all cases...
> 

>From experience, I'd say it's likely that formats are reused. Looking at native
mode, we originally had R1, R1b, R2 and R3. Yet we are now able to support R4
through R7 using the components already defined.

> 
> Leaving the semantic question of where those 5 bytes would need to be
> stored.  Clearly since it was defined as a new response type, the answer
> used for R3 would not be appropriate... and R3 is the only 5 byte response
> currently defined.
> 

Why not? It's not like we store R1 and R2 differently except for their size.

> 
> You're missing the point that those values you're objecting to are fully
> defined as part of the protocol, so we *DO* know exactly what to do ...
> 

My point is that we're spreading that knowledge into too many areas. Keep it in
the core and let the host driver deal with shuffling bits.

> 
> Well, the R1_STATE bits of the mapping.
> 

All mappings. The host driver shouldn't lie to the core.

-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [patch 1/4] MMC-over-SPI header updates

[David Brownell]

On Friday 15 June 2007, Pierre Ossman wrote:
> David Brownell wrote:
> > 
> > It starts "off", and needs to be turned "on".  Reasons to leave it
> > off include performance (I measured over 30% slower with CRCs on while
> > writing a 120+ MB SD partition), and of course the fact that the CRC
> > stuff is the very newest feature in the code (thanks Jan!), and is not
> > yet as trustworthy (viz. the intermittent bug I noted).
> > 
> 
> Any CRC bugs should surface rather quickly, so that shouldn't be an isse.

Surfaced != fixed.  The issue is that resolving them isn't all that
straightforward.  There's that one issue I surfaced, for example; it
turns up some nasty recovery issues wholly unrelated to CRCs.


> Performance might be though, but do people really feel that those 30% is worth
> losing CRC? I sure wouldn't feel comfortable with that.

If *you* want CRCs, then don't use the "use_crc=n" module option.
The stack is usable without them; most hardware isn't glitchey
enough to need them as more than a safety blanket.

Eventually, performance tuning should reduce that penalty, but
until then the option is certainly in the "must have" category
for at least developers.


> > That seems like a fantasy to me.  We don't know in advance how those
> > types will be defined (or if they even will be!), so there's no way
> > we can know whether or not new types will need to be defined.
> > ...
> 
> From experience, I'd say it's likely that formats are reused. Looking at native
> mode, we originally had R1, R1b, R2 and R3. Yet we are now able to support R4
> through R7 using the components already defined.

You had to define new types, and update the code to use them.
It was nice of the spec writers to make that easy for you, but
you can't rely on that happening every time.


It seems like the new SDHC stuff has only one significant change
affecting SPI:  there are now two requests which return a status
byte plus four data bytes.  One uses the previously defined R3
response type.  The other uses a newly defined R7 type.

So it seems like that particular issue can be addressed by morphing
the current MMC_RSP_SPI_OCR code into an MMC_RSP_SPI_4BYTE type,
along the lines of what you were talking about.


> > Well, the R1_STATE bits of the mapping.
> > 
> 
> All mappings. The host driver shouldn't lie to the core.

Mappings aren't lies though.  It's more like translation:  you
can say something in French or in English, and get across the
same core meaning.  To get certain details you'll want the
original language ... but that doesn't mean the translation
was a lie.

That said, I'd be glad to eventually see those R1 mappings
begone, when the core stops needing them.

- Dave


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [patch 1/4] MMC-over-SPI header updates

[Pierre Ossman]

David Brownell wrote:
> On Friday 15 June 2007, Pierre Ossman wrote:
>> Any CRC bugs should surface rather quickly, so that shouldn't be an isse.
> 
> Surfaced != fixed.  The issue is that resolving them isn't all that
> straightforward.  There's that one issue I surfaced, for example; it
> turns up some nasty recovery issues wholly unrelated to CRCs.
> 

I still think we should have the ambition that this can be made to work. We can
always remove it if that proves impossible (or not worth the effort).

> 
> If *you* want CRCs, then don't use the "use_crc=n" module option.
> The stack is usable without them; most hardware isn't glitchey
> enough to need them as more than a safety blanket.
> 

Silent data loss gives me the willies.

> Eventually, performance tuning should reduce that penalty, but
> until then the option is certainly in the "must have" category
> for at least developers.
> 

Fair enough.

>> From experience, I'd say it's likely that formats are reused. Looking at native
>> mode, we originally had R1, R1b, R2 and R3. Yet we are now able to support R4
>> through R7 using the components already defined.
> 
> You had to define new types, and update the code to use them.
> It was nice of the spec writers to make that easy for you, but
> you can't rely on that happening every time.
> 

You act like that happen by dumb luck. I'd say they looked at the existing spec
and designed the new features in a way that reused as much as possible. Sound
engineering really.

> 
> It seems like the new SDHC stuff has only one significant change
> affecting SPI:  there are now two requests which return a status
> byte plus four data bytes.  One uses the previously defined R3
> response type.  The other uses a newly defined R7 type.
> 
> So it seems like that particular issue can be addressed by morphing
> the current MMC_RSP_SPI_OCR code into an MMC_RSP_SPI_4BYTE type,
> along the lines of what you were talking about.
> 

My point exactly. I see no point in _OCR, but there is a possible gain with
_4BYTE (in the worst case, we get the same thing as if we'd done _OCR).

> 
> Mappings aren't lies though.  It's more like translation:  you
> can say something in French or in English, and get across the
> same core meaning.  To get certain details you'll want the
> original language ... but that doesn't mean the translation
> was a lie.
> 

You can't really compare computer protocols to human language. The level of
precision differs greatly.

Since you can't do a pure substitution, you have to simplify or try to guess
data. Both cases are accidents waiting to happen since the core is coded on the
assumption that the values reported are 100% correct.

> That said, I'd be glad to eventually see those R1 mappings
> begone, when the core stops needing them.
> 

So change the core.

Rgds
-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [patch 1/4] MMC-over-SPI header updates

[David Brownell]

On Wednesday 20 June 2007, Pierre Ossman wrote:
> David Brownell wrote:
> > On Friday 15 June 2007, Pierre Ossman wrote:
> >> Any CRC bugs should surface rather quickly, so that shouldn't be an isse.
> > 
> > Surfaced != fixed.  The issue is that resolving them isn't all that
> > straightforward.  There's that one issue I surfaced, for example; it
> > turns up some nasty recovery issues wholly unrelated to CRCs.
> 
> I still think we should have the ambition that this can be made to work. We can
> always remove it if that proves impossible (or not worth the effort).

And I never disagreed with that.


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

[patch 2/4] MMC-over-SPI card driver update

[David Brownell]

Teaching the MMC/SD block card driver about SPI.

 - Provide the SPI response type flags with each request issued.  The
   model is that if no such flags are provided, it will be rejected 
   by the MMC-over-SPI host.

 - Understand that multiblock SPI writes don't use STOP_TRANSMISSION.

Signed-off-by: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
---
 drivers/mmc/card/block.c |   20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

--- pxa.orig/drivers/mmc/card/block.c	2007-06-10 13:00:21.000000000 -0700
+++ pxa/drivers/mmc/card/block.c	2007-06-10 13:00:30.000000000 -0700
@@ -151,7 +151,7 @@ static u32 mmc_sd_num_wr_blocks(struct m
 
 	cmd.opcode = MMC_APP_CMD;
 	cmd.arg = card->rca << 16;
-	cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
+	cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_AC;
 
 	err = mmc_wait_for_cmd(card->host, &cmd, 0);
 	if ((err != MMC_ERR_NONE) || !(cmd.resp[0] & R1_APP_CMD))
@@ -161,7 +161,7 @@ static u32 mmc_sd_num_wr_blocks(struct m
 
 	cmd.opcode = SD_APP_SEND_NUM_WR_BLKS;
 	cmd.arg = 0;
-	cmd.flags = MMC_RSP_R1 | MMC_CMD_ADTC;
+	cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC;
 
 	memset(&data, 0, sizeof(struct mmc_data));
 
@@ -220,11 +220,11 @@ static int mmc_blk_issue_rq(struct mmc_q
 		brq.cmd.arg = req->sector;
 		if (!mmc_card_blockaddr(card))
 			brq.cmd.arg <<= 9;
-		brq.cmd.flags = MMC_RSP_R1 | MMC_CMD_ADTC;
+		brq.cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC;
 		brq.data.blksz = 1 << md->block_bits;
 		brq.stop.opcode = MMC_STOP_TRANSMISSION;
 		brq.stop.arg = 0;
-		brq.stop.flags = MMC_RSP_R1B | MMC_CMD_AC;
+		brq.stop.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
 		brq.data.blocks = req->nr_sectors >> (md->block_bits - 9);
 		if (brq.data.blocks > card->host->max_blk_count)
 			brq.data.blocks = card->host->max_blk_count;
@@ -244,7 +244,12 @@ static int mmc_blk_issue_rq(struct mmc_q
 
 		if (brq.data.blocks > 1) {
 			brq.data.flags |= MMC_DATA_MULTI;
-			brq.mrq.stop = &brq.stop;
+			/* SPI multiblock writes terminate using a special
+			 * token, not a STOP_TRANSMISSION request.
+			 */
+			if (!mmc_host_is_spi(card->host)
+					|| rq_data_dir(req) == READ)
+				brq.mrq.stop = &brq.stop;
 			readcmd = MMC_READ_MULTIPLE_BLOCK;
 			writecmd = MMC_WRITE_MULTIPLE_BLOCK;
 		} else {
@@ -308,7 +313,8 @@ static int mmc_blk_issue_rq(struct mmc_q
 
 				cmd.opcode = MMC_SEND_STATUS;
 				cmd.arg = card->rca << 16;
-				cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
+				cmd.flags = MMC_RSP_SPI_R2 | MMC_RSP_R1
+						| MMC_CMD_AC;
 				err = mmc_wait_for_cmd(card->host, &cmd, 5);
 				if (err) {
 					printk(KERN_ERR "%s: error %d requesting status\n",
@@ -511,7 +517,7 @@ mmc_blk_set_blksize(struct mmc_blk_data 
 	mmc_claim_host(card->host);
 	cmd.opcode = MMC_SET_BLOCKLEN;
 	cmd.arg = 1 << md->block_bits;
-	cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
+	cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_AC;
 	err = mmc_wait_for_cmd(card->host, &cmd, 5);
 	mmc_release_host(card->host);
 

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [patch 2/4] MMC-over-SPI card driver update

[Pierre Ossman]

David Brownell wrote:
> Teaching the MMC/SD block card driver about SPI.
> 
>  - Provide the SPI response type flags with each request issued.  The
>    model is that if no such flags are provided, it will be rejected 
>    by the MMC-over-SPI host.
> 
>  - Understand that multiblock SPI writes don't use STOP_TRANSMISSION.
> 
> Signed-off-by: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>

Looks ok.

-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

[patch 3/4] MMC-over-SPI core updates

[David Brownell]

Teach the MMC/SD/SDIO core about using SPI mode.

 - Use mmc_host_is_spi() so enumeration works through SPI protocols,
   not just the native (and sometimes parallel) ones

 - Provide the SPI response type flags with each request issued,
   including rquests from the new lock/unlock code.  The model
   is that if no such flags are provided, it will be rejected 
   by the MMC-over-SPI host.

 - Understand that some commands act a bit differently ... notably the
   OP_COND command doesn't return the OCR, and does status differently.

Those changes required some new and updated primitives:

 - Provide utilities to access two SPI-only requests, and one
   request that wasn't previously needed:
     * mmc_spi_read_ocr() ... SPI only
     * mmc_spi_set_crc() ... SPI only
     * mmc_spi_send_cid() ... works without broadcast mode

 - Updated internal routines:
     * previous mmc_send_csd() modified into mmc_send_cxd_native();
       it uses native "R2" responses, which include 16 bytes of data.
     * previous mmc_send_ext_csd() becomes new mmc_send_cxd_data()
       helper for command-and-data access

 - Modified mmc_send_ext_csd() now uses mmc_send_cxd_data() helper

 - Modified mmc_send_csd(), and new mmc_spi_send_cid(), routines use
   those helper routines based on whether they're native or SPI

The current assumption is that nobody will want a controller driver
to handle both SPI *and* the faster "native" MMC/SD protocols, even
on hardware which can do that.

The SPI support hasn't been tested on the new MMC4 cards (they're not
widely available) or with the new card lock/unlock mechanism.  Likewise
SD cards with 4GB and up may have surprises lurking.

Signed-off-by: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
---
 drivers/mmc/core/core.c    |   38 ++++++++-
 drivers/mmc/core/mmc.c     |   26 ++++--
 drivers/mmc/core/mmc_ops.c |  173 +++++++++++++++++++++++++++++++++++----------
 drivers/mmc/core/mmc_ops.h |    4 +
 drivers/mmc/core/sd.c      |   36 ++++++---
 drivers/mmc/core/sd_ops.c  |   17 +++-
 6 files changed, 226 insertions(+), 68 deletions(-)

--- pxa.orig/drivers/mmc/core/mmc_ops.h	2007-06-10 13:00:21.000000000 -0700
+++ pxa/drivers/mmc/core/mmc_ops.h	2007-06-10 13:00:34.000000000 -0700
@@ -26,5 +26,9 @@ int mmc_switch(struct mmc_card *card, u8
 int mmc_send_status(struct mmc_card *card, u32 *status);
 int mmc_lock_unlock(struct mmc_card *card, struct key *key, int mode);
 
+int mmc_spi_send_cid(struct mmc_host *host, u32 *cid);
+int mmc_spi_read_ocr(struct mmc_host *host, u32 *ocrp);
+int mmc_spi_set_crc(struct mmc_host *host);
+
 #endif
 
--- pxa.orig/drivers/mmc/core/core.c	2007-06-10 13:00:21.000000000 -0700
+++ pxa/drivers/mmc/core/core.c	2007-06-10 13:00:34.000000000 -0700
@@ -386,8 +386,13 @@ static void mmc_power_up(struct mmc_host
 	int bit = fls(host->ocr_avail) - 1;
 
 	host->ios.vdd = bit;
-	host->ios.bus_mode = MMC_BUSMODE_OPENDRAIN;
-	host->ios.chip_select = MMC_CS_DONTCARE;
+	if (mmc_host_is_spi(host)) {
+		host->ios.chip_select = MMC_CS_HIGH;
+		host->ios.bus_mode = MMC_BUSMODE_PUSHPULL;
+	} else {
+		host->ios.chip_select = MMC_CS_DONTCARE;
+		host->ios.bus_mode = MMC_BUSMODE_OPENDRAIN;
+	}
 	host->ios.power_mode = MMC_POWER_UP;
 	host->ios.bus_width = MMC_BUS_WIDTH_1;
 	host->ios.timing = MMC_TIMING_LEGACY;
@@ -406,8 +411,10 @@ static void mmc_power_off(struct mmc_hos
 {
 	host->ios.clock = 0;
 	host->ios.vdd = 0;
-	host->ios.bus_mode = MMC_BUSMODE_OPENDRAIN;
-	host->ios.chip_select = MMC_CS_DONTCARE;
+	if (!mmc_host_is_spi(host)) {
+		host->ios.bus_mode = MMC_BUSMODE_OPENDRAIN;
+		host->ios.chip_select = MMC_CS_DONTCARE;
+	}
 	host->ios.power_mode = MMC_POWER_OFF;
 	host->ios.bus_width = MMC_BUS_WIDTH_1;
 	host->ios.timing = MMC_TIMING_LEGACY;
@@ -498,6 +505,19 @@ void mmc_detect_change(struct mmc_host *
 EXPORT_SYMBOL(mmc_detect_change);
 
 
+static int mmc_spi_fixup(struct mmc_host *host, u32 *ocrp)
+{
+	int err;
+
+	if (!mmc_host_is_spi(host))
+		return MMC_ERR_NONE;
+
+	err = mmc_spi_read_ocr(host, ocrp);
+	if (err == MMC_ERR_NONE);
+		err = mmc_spi_set_crc(host);
+	return err;
+}
+
 void mmc_rescan(struct work_struct *work)
 {
 	struct mmc_host *host =
@@ -519,11 +539,13 @@ void mmc_rescan(struct work_struct *work
 		mmc_power_up(host);
 		mmc_go_idle(host);
 
-		mmc_send_if_cond(host, host->ocr_avail);
+		if (!mmc_host_is_spi(host))
+			mmc_send_if_cond(host, host->ocr_avail);
 
 		err = mmc_send_app_op_cond(host, 0, &ocr);
 		if (err == MMC_ERR_NONE) {
-			if (mmc_attach_sd(host, ocr))
+			err = mmc_spi_fixup(host, &ocr);
+			if (err != MMC_ERR_NONE || mmc_attach_sd(host, ocr))
 				mmc_power_off(host);
 		} else {
 			/*
@@ -532,7 +554,9 @@ void mmc_rescan(struct work_struct *work
 			 */
 			err = mmc_send_op_cond(host, 0, &ocr);
 			if (err == MMC_ERR_NONE) {
-				if (mmc_attach_mmc(host, ocr))
+				err = mmc_spi_fixup(host, &ocr);
+				if (err != MMC_ERR_NONE
+						|| mmc_attach_mmc(host, ocr))
 					mmc_power_off(host);
 			} else {
 				mmc_power_off(host);
--- pxa.orig/drivers/mmc/core/mmc_ops.c	2007-06-10 13:00:21.000000000 -0700
+++ pxa/drivers/mmc/core/mmc_ops.c	2007-06-10 13:00:34.000000000 -0700
@@ -31,6 +31,10 @@ static int _mmc_select_card(struct mmc_h
 
 	BUG_ON(!host);
 
+	/* SPI doesn't multiplex; "select" is meaningless */
+	if (mmc_host_is_spi(host))
+		return MMC_ERR_NONE;
+
 	memset(&cmd, 0, sizeof(struct mmc_command));
 
 	cmd.opcode = MMC_SELECT_CARD;
@@ -67,23 +71,30 @@ int mmc_go_idle(struct mmc_host *host)
 	int err;
 	struct mmc_command cmd;
 
-	mmc_set_chip_select(host, MMC_CS_HIGH);
-
-	mmc_delay(1);
+	/*
+	 * Non-SPI hosts need to prevent chipselect going active
+	 * during GO_IDLE; that would put chips into SPI mode.
+	 * SPI hosts necessarily ignore host->chip_select.
+	 */
+	if (!mmc_host_is_spi(host)) {
+		mmc_set_chip_select(host, MMC_CS_HIGH);
+		mmc_delay(1);
+	}
 
 	memset(&cmd, 0, sizeof(struct mmc_command));
 
 	cmd.opcode = MMC_GO_IDLE_STATE;
 	cmd.arg = 0;
-	cmd.flags = MMC_RSP_NONE | MMC_CMD_BC;
+	cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_NONE | MMC_CMD_BC;
 
 	err = mmc_wait_for_cmd(host, &cmd, 0);
 
 	mmc_delay(1);
 
-	mmc_set_chip_select(host, MMC_CS_DONTCARE);
-
-	mmc_delay(1);
+	if (!mmc_host_is_spi(host)) {
+		mmc_set_chip_select(host, MMC_CS_DONTCARE);
+		mmc_delay(1);
+	}
 
 	return err;
 }
@@ -99,14 +110,18 @@ int mmc_send_op_cond(struct mmc_host *ho
 
 	cmd.opcode = MMC_SEND_OP_COND;
 	cmd.arg = ocr;
-	cmd.flags = MMC_RSP_R3 | MMC_CMD_BCR;
+	cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R3 | MMC_CMD_BCR;
 
 	for (i = 100; i; i--) {
 		err = mmc_wait_for_cmd(host, &cmd, 0);
 		if (err != MMC_ERR_NONE)
 			break;
 
-		if (cmd.resp[0] & MMC_CARD_BUSY || ocr == 0)
+		if (mmc_host_is_spi(host)) {
+			/* wait until reset completes */
+			if (!(cmd.resp[2] & R1_SPI_IDLE))
+				break;
+		} else if (cmd.resp[0] & MMC_CARD_BUSY || ocr == 0)
 			break;
 
 		err = MMC_ERR_TIMEOUT;
@@ -114,7 +129,7 @@ int mmc_send_op_cond(struct mmc_host *ho
 		mmc_delay(10);
 	}
 
-	if (rocr)
+	if (rocr && !mmc_host_is_spi(host))
 		*rocr = cmd.resp[0];
 
 	return err;
@@ -164,41 +179,46 @@ int mmc_set_relative_addr(struct mmc_car
 	return MMC_ERR_NONE;
 }
 
-int mmc_send_csd(struct mmc_card *card, u32 *csd)
+static int
+mmc_send_cxd_native(struct mmc_host *host, u32 arg, u32 *cxd, int opcode)
 {
 	int err;
 	struct mmc_command cmd;
 
-	BUG_ON(!card);
-	BUG_ON(!card->host);
-	BUG_ON(!csd);
+	BUG_ON(!host);
+	BUG_ON(!cxd);
 
 	memset(&cmd, 0, sizeof(struct mmc_command));
 
-	cmd.opcode = MMC_SEND_CSD;
-	cmd.arg = card->rca << 16;
+	cmd.opcode = opcode;
+	cmd.arg = arg;
 	cmd.flags = MMC_RSP_R2 | MMC_CMD_AC;
 
-	err = mmc_wait_for_cmd(card->host, &cmd, MMC_CMD_RETRIES);
+	err = mmc_wait_for_cmd(host, &cmd, MMC_CMD_RETRIES);
 	if (err != MMC_ERR_NONE)
 		return err;
 
-	memcpy(csd, cmd.resp, sizeof(u32) * 4);
+	memcpy(cxd, cmd.resp, sizeof(u32) * 4);
 
 	return MMC_ERR_NONE;
 }
 
-int mmc_send_ext_csd(struct mmc_card *card, u8 *ext_csd)
+struct mmc_cxd {
+	struct mmc_card	*card;		/* optional */
+	void		*buf;
+	unsigned	len;
+	u32		opcode;
+	u32		arg;
+	unsigned	flags;
+};
+
+static int mmc_send_cxd_data(struct mmc_host *host, struct mmc_cxd *cxd)
 {
 	struct mmc_request mrq;
 	struct mmc_command cmd;
 	struct mmc_data data;
 	struct scatterlist sg;
 
-	BUG_ON(!card);
-	BUG_ON(!card->host);
-	BUG_ON(!ext_csd);
-
 	memset(&mrq, 0, sizeof(struct mmc_request));
 	memset(&cmd, 0, sizeof(struct mmc_command));
 	memset(&data, 0, sizeof(struct mmc_data));
@@ -206,21 +226,22 @@ int mmc_send_ext_csd(struct mmc_card *ca
 	mrq.cmd = &cmd;
 	mrq.data = &data;
 
-	cmd.opcode = MMC_SEND_EXT_CSD;
-	cmd.arg = 0;
-	cmd.flags = MMC_RSP_R1 | MMC_CMD_ADTC;
+	cmd.opcode = cxd->opcode;
+	cmd.arg = cxd->arg;
+	cmd.flags = cxd->flags;
 
-	data.blksz = 512;
+	data.blksz = cxd->len;
 	data.blocks = 1;
 	data.flags = MMC_DATA_READ;
 	data.sg = &sg;
 	data.sg_len = 1;
 
-	sg_init_one(&sg, ext_csd, 512);
+	sg_init_one(&sg, cxd->buf, cxd->len);
 
-	mmc_set_data_timeout(&data, card, 0);
+	if (cxd->card)
+		mmc_set_data_timeout(&data, cxd->card, 0);
 
-	mmc_wait_for_req(card->host, &mrq);
+	mmc_wait_for_req(host, &mrq);
 
 	if (cmd.error != MMC_ERR_NONE)
 		return cmd.error;
@@ -230,6 +251,85 @@ int mmc_send_ext_csd(struct mmc_card *ca
 	return MMC_ERR_NONE;
 }
 
+int mmc_spi_read_ocr(struct mmc_host *host, u32 *ocrp)
+{
+	struct mmc_command cmd;
+	int err;
+
+	memset(&cmd, 0, sizeof(struct mmc_command));
+
+	cmd.opcode = MMC_SPI_READ_OCR;
+	cmd.flags = MMC_RSP_SPI_R3;
+
+	err = mmc_wait_for_cmd(host, &cmd, 0);
+
+	*ocrp = cmd.resp[1];
+	return err;
+}
+
+int mmc_spi_set_crc(struct mmc_host *host)
+{
+	struct mmc_command cmd;
+
+	memset(&cmd, 0, sizeof(struct mmc_command));
+
+	cmd.opcode = MMC_SPI_CRC_ON_OFF;
+	cmd.flags = MMC_RSP_SPI_R1;
+	if (host->caps & MMC_CAP_SPI_CRC)
+		cmd.arg = 1;
+
+	return mmc_wait_for_cmd(host, &cmd, 0);
+}
+
+int mmc_send_csd(struct mmc_card *card, u32 *csd)
+{
+	struct mmc_cxd	cxd;
+
+	if (!mmc_host_is_spi(card->host))
+		return mmc_send_cxd_native(card->host, card->rca << 16,
+				csd, MMC_SEND_CSD);
+
+	cxd.card = card;
+	cxd.buf = csd;
+	cxd.len = 16;
+	cxd.opcode = MMC_SEND_CSD;
+	cxd.arg = 0;
+	cxd.flags = MMC_RSP_SPI_R1;
+
+	return mmc_send_cxd_data(card->host, &cxd);
+}
+
+int mmc_spi_send_cid(struct mmc_host *host, u32 *cid)
+{
+	struct mmc_cxd	cxd;
+
+	if (!mmc_host_is_spi(host))
+		return mmc_send_cxd_native(host, 0, cid, MMC_SEND_CID);
+
+	cxd.card = NULL;
+	cxd.buf = cid;
+	cxd.len = 16;
+	cxd.opcode = MMC_SEND_CID;
+	cxd.arg = 0;
+	cxd.flags = MMC_RSP_SPI_R1;
+
+	return mmc_send_cxd_data(host, &cxd);
+}
+
+int mmc_send_ext_csd(struct mmc_card *card, u8 *ext_csd)
+{
+	struct mmc_cxd	cxd;
+
+	cxd.card = card;
+	cxd.buf = ext_csd;
+	cxd.len = 512;
+	cxd.opcode = MMC_SEND_EXT_CSD;
+	cxd.arg = 0;
+	cxd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC;
+
+	return mmc_send_cxd_data(card->host, &cxd);
+}
+
 int mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value)
 {
 	int err;
@@ -245,7 +345,7 @@ int mmc_switch(struct mmc_card *card, u8
 		  (index << 16) |
 		  (value << 8) |
 		  set;
-	cmd.flags = MMC_RSP_R1B | MMC_CMD_AC;
+	cmd.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
 
 	err = mmc_wait_for_cmd(card->host, &cmd, MMC_CMD_RETRIES);
 	if (err != MMC_ERR_NONE)
@@ -265,8 +365,9 @@ int mmc_send_status(struct mmc_card *car
 	memset(&cmd, 0, sizeof(struct mmc_command));
 
 	cmd.opcode = MMC_SEND_STATUS;
-	cmd.arg = card->rca << 16;
-	cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
+	if (!mmc_host_is_spi(card->host))
+		cmd.arg = card->rca << 16;
+	cmd.flags = MMC_RSP_SPI_R2 | MMC_RSP_R1 | MMC_CMD_AC;
 
 	err = mmc_wait_for_cmd(card->host, &cmd, MMC_CMD_RETRIES);
 	if (err != MMC_ERR_NONE)
@@ -316,7 +417,7 @@ int mmc_lock_unlock(struct mmc_card *car
 
 	cmd.opcode = MMC_SET_BLOCKLEN;
 	cmd.arg = data_size;
-	cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
+	cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_AC;
 	err = mmc_wait_for_cmd(card->host, &cmd, MMC_CMD_RETRIES);
 	if (err != MMC_ERR_NONE)
 		goto out;
@@ -325,7 +426,7 @@ int mmc_lock_unlock(struct mmc_card *car
 
 	cmd.opcode = MMC_LOCK_UNLOCK;
 	cmd.arg = 0;
-	cmd.flags = MMC_RSP_R1B | MMC_CMD_ADTC;
+	cmd.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_ADTC;
 
 	memset(&data, 0, sizeof(struct mmc_data));
 
@@ -351,7 +452,7 @@ int mmc_lock_unlock(struct mmc_card *car
 
 	cmd.opcode = MMC_SEND_STATUS;
 	cmd.arg = card->rca << 16;
-	cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
+	cmd.flags = MMC_RSP_SPI_R2 | MMC_RSP_R1 | MMC_CMD_AC;
 
 	/* set timeout for forced erase operation to 3 min. (see MMC spec) */
 	erase_timeout = jiffies + 180 * HZ;
--- pxa.orig/drivers/mmc/core/mmc.c	2007-06-10 13:00:21.000000000 -0700
+++ pxa/drivers/mmc/core/mmc.c	2007-06-10 13:00:34.000000000 -0700
@@ -320,9 +320,15 @@ static int mmc_init_card(struct mmc_host
 	/*
 	 * Fetch CID from card.
 	 */
-	err = mmc_all_send_cid(host, cid);
-	if (err != MMC_ERR_NONE)
-		goto err;
+	if (mmc_host_is_spi(host)) {
+		err = mmc_spi_send_cid(host, cid);
+		if (err != MMC_ERR_NONE)
+			goto err;
+	} else {
+		err = mmc_all_send_cid(host, cid);
+		if (err != MMC_ERR_NONE)
+			goto err;
+	}
 
 	if (oldcard) {
 		if (memcmp(cid, oldcard->raw_cid, sizeof(cid)) != 0)
@@ -343,13 +349,15 @@ static int mmc_init_card(struct mmc_host
 	}
 
 	/*
-	 * Set card RCA.
+	 * For native busses:  set card RCA and leave open drain mode.
 	 */
-	err = mmc_set_relative_addr(card);
-	if (err != MMC_ERR_NONE)
-		goto free_card;
+	if (!mmc_host_is_spi(host)) {
+		err = mmc_set_relative_addr(card);
+		if (err != MMC_ERR_NONE)
+			goto free_card;
 
-	mmc_set_bus_mode(host, MMC_BUSMODE_PUSHPULL);
+		mmc_set_bus_mode(host, MMC_BUSMODE_PUSHPULL);
+	}
 
 	/*
 	 * Check if card is locked.
@@ -385,7 +393,7 @@ static int mmc_init_card(struct mmc_host
 
 	if (!oldcard) {
 		/*
-		 * Fetch and process extened CSD.
+		 * Fetch and process extended CSD.
 		 */
 		err = mmc_read_ext_csd(card);
 		if (err != MMC_ERR_NONE)
--- pxa.orig/drivers/mmc/core/sd.c	2007-06-10 13:00:21.000000000 -0700
+++ pxa/drivers/mmc/core/sd.c	2007-06-10 13:00:34.000000000 -0700
@@ -379,9 +379,19 @@ static int mmc_sd_init_card(struct mmc_h
 	/*
 	 * Fetch CID from card.
 	 */
-	err = mmc_all_send_cid(host, cid);
-	if (err != MMC_ERR_NONE)
-		goto err;
+	if (mmc_host_is_spi(host)) {
+		err = mmc_spi_set_crc(host);
+		if (err != MMC_ERR_NONE)
+			goto err;
+		err = mmc_spi_send_cid(host, cid);
+		if (err != MMC_ERR_NONE)
+			goto err;
+	} else {
+		err = mmc_all_send_cid(host, cid);
+		if (err != MMC_ERR_NONE)
+			goto err;
+	}
+
 
 	if (oldcard) {
 		if (memcmp(cid, oldcard->raw_cid, sizeof(cid)) != 0)
@@ -401,13 +411,15 @@ static int mmc_sd_init_card(struct mmc_h
 	}
 
 	/*
-	 * Set card RCA.
+	 * For native busses:  set card RCA and leave open drain mode.
 	 */
-	err = mmc_send_relative_addr(host, &card->rca);
-	if (err != MMC_ERR_NONE)
-		goto free_card;
+	if (!mmc_host_is_spi(host)) {
+		err = mmc_send_relative_addr(host, &card->rca);
+		if (err != MMC_ERR_NONE)
+			goto free_card;
 
-	mmc_set_bus_mode(host, MMC_BUSMODE_PUSHPULL);
+		mmc_set_bus_mode(host, MMC_BUSMODE_PUSHPULL);
+	}
 
 	/*
 	 * Check if card is locked.
@@ -455,9 +467,11 @@ static int mmc_sd_init_card(struct mmc_h
 		/*
 		 * Fetch switch information from card.
 		 */
-		err = mmc_read_switch(card);
-		if (err != MMC_ERR_NONE)
-			goto free_card;
+		if (!mmc_host_is_spi(host)) {
+			err = mmc_read_switch(card);
+			if (err != MMC_ERR_NONE)
+				goto free_card;
+		}
 	}
 
 	/*
--- pxa.orig/drivers/mmc/core/sd_ops.c	2007-06-06 16:47:58.000000000 -0700
+++ pxa/drivers/mmc/core/sd_ops.c	2007-06-10 13:00:34.000000000 -0700
@@ -89,10 +89,10 @@ int mmc_app_cmd(struct mmc_host *host, s
 
 	if (card) {
 		cmd.arg = card->rca << 16;
-		cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
+		cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_AC;
 	} else {
 		cmd.arg = 0;
-		cmd.flags = MMC_RSP_R1 | MMC_CMD_BCR;
+		cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_BCR;
 	}
 
 	err = mmc_wait_for_cmd(host, &cmd, 0);
@@ -148,14 +148,18 @@ int mmc_send_app_op_cond(struct mmc_host
 
 	cmd.opcode = SD_APP_OP_COND;
 	cmd.arg = ocr;
-	cmd.flags = MMC_RSP_R3 | MMC_CMD_BCR;
+	cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R3 | MMC_CMD_BCR;
 
 	for (i = 100; i; i--) {
 		err = mmc_wait_for_app_cmd(host, NULL, &cmd, MMC_CMD_RETRIES);
 		if (err != MMC_ERR_NONE)
 			break;
 
-		if (cmd.resp[0] & MMC_CARD_BUSY || ocr == 0)
+		if (mmc_host_is_spi(host)) {
+			/* wait until reset completes */
+			if (!(cmd.resp[2] & R1_SPI_IDLE))
+				break;
+		} else if (cmd.resp[0] & MMC_CARD_BUSY || ocr == 0)
 			break;
 
 		err = MMC_ERR_TIMEOUT;
@@ -175,6 +179,9 @@ int mmc_send_if_cond(struct mmc_host *ho
 	int err;
 	static const u8 test_pattern = 0xAA;
 
+	if (mmc_host_is_spi(host))
+		return MMC_ERR_FAILED;
+
 	/*
 	 * To support SD 2.0 cards, we must always invoke SD_SEND_IF_COND
 	 * before SD_APP_OP_COND. This command will harmlessly fail for
@@ -242,7 +249,7 @@ int mmc_app_send_scr(struct mmc_card *ca
 
 	cmd.opcode = SD_APP_SEND_SCR;
 	cmd.arg = 0;
-	cmd.flags = MMC_RSP_R1 | MMC_CMD_ADTC;
+	cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC;
 
 	data.blksz = 8;
 	data.blocks = 1;

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [patch 3/4] MMC-over-SPI core updates

[Pierre Ossman]

David Brownell wrote:
> @@ -498,6 +505,19 @@ void mmc_detect_change(struct mmc_host *
>  EXPORT_SYMBOL(mmc_detect_change);
>  
>  
> +static int mmc_spi_fixup(struct mmc_host *host, u32 *ocrp)
> +{
> +	int err;
> +
> +	if (!mmc_host_is_spi(host))
> +		return MMC_ERR_NONE;
> +
> +	err = mmc_spi_read_ocr(host, ocrp);
> +	if (err == MMC_ERR_NONE);
> +		err = mmc_spi_set_crc(host);
> +	return err;
> +}
> +
>  void mmc_rescan(struct work_struct *work)
>  {
>  	struct mmc_host *host =

I'd prefer if this is in mmc_rescan(). It's so little code that all this does is
hide the init sequence.

> @@ -519,11 +539,13 @@ void mmc_rescan(struct work_struct *work
>  		mmc_power_up(host);
>  		mmc_go_idle(host);
>  
> -		mmc_send_if_cond(host, host->ocr_avail);
> +		if (!mmc_host_is_spi(host))
> +			mmc_send_if_cond(host, host->ocr_avail);
>  

If you have a look at the SD physical spec (the public one), you'll see that
if_cond is needed on SPI aswell. I know you don't have cards for testing, but
let's code according to spec and try to find people who can test all parts.

>  		err = mmc_send_app_op_cond(host, 0, &ocr);
>  		if (err == MMC_ERR_NONE) {
> -			if (mmc_attach_sd(host, ocr))
> +			err = mmc_spi_fixup(host, &ocr);
> +			if (err != MMC_ERR_NONE || mmc_attach_sd(host, ocr))
>  				mmc_power_off(host);
>  		} else {
>  			/*

Looking at the example flow charts in the specs, this is not the proper way.
I've spent the morning staring at both specs in parallel, and here's how I think
it should be done:

[CMD8 for SD]
CMD58 [with HCS bit for MMC]
ACMD51, OK -> SD
CMD1, OK -> MMC

The SD spec states that both ACMD51 and CMD1 is legal, but nothing about why
both are supported. It also states that CMD1 has a bit indicating SDHC support
(as if CMD8 isn't enough), but ACMD51 doesn't. But the flow charts show ACMD51,
not CMD1. :/

The next step, in the bus handlers, will have to get the OCR on their own. SD
states that the OCR is valid in the initial CMD58 (before CMD1/ACMD51), but MMC
states that it is not. So for the sake of consistency, let both paths reissue a
CMD58 for the OCR.

> --- pxa.orig/drivers/mmc/core/mmc_ops.c	2007-06-10 13:00:21.000000000 -0700
> +++ pxa/drivers/mmc/core/mmc_ops.c	2007-06-10 13:00:34.000000000 -0700
> @@ -31,6 +31,10 @@ static int _mmc_select_card(struct mmc_h
>  
>  	BUG_ON(!host);
>  
> +	/* SPI doesn't multiplex; "select" is meaningless */
> +	if (mmc_host_is_spi(host))
> +		return MMC_ERR_NONE;
> +
>  	memset(&cmd, 0, sizeof(struct mmc_command));
>  
>  	cmd.opcode = MMC_SELECT_CARD;

I do not like this. Unsupported commands shouldn't be issued. We should not have
voodoo in here that makes them a no-op. That just obscures things. Feel free to
put a check in there if you feel the risk is too great.

>  
> -int mmc_send_ext_csd(struct mmc_card *card, u8 *ext_csd)
> +struct mmc_cxd {
> +	struct mmc_card	*card;		/* optional */
> +	void		*buf;
> +	unsigned	len;
> +	u32		opcode;
> +	u32		arg;
> +	unsigned	flags;
> +};
> +

This seems overly complex. You can do this with arguments as most of that struct
is superfluous:

 - card can be specified at all times if you reorg the init a bit, allowing you
to drop the host argument.

 - arg and flags are the same for all three commands, so those can be dropped.

> @@ -230,6 +251,85 @@ int mmc_send_ext_csd(struct mmc_card *ca
>  	return MMC_ERR_NONE;
>  }
>  
> +int mmc_spi_read_ocr(struct mmc_host *host, u32 *ocrp)
> +{

How about moving these further down so that the mmc_send_cxd_data() users are
right below it?

Also, the theme I've tried to have here is to take a card pointer whenever you
operate on a specific card, and a host pointer for bus wide things. It adds a
bit of readability.

> --- pxa.orig/drivers/mmc/core/mmc.c	2007-06-10 13:00:21.000000000 -0700
> +++ pxa/drivers/mmc/core/mmc.c	2007-06-10 13:00:34.000000000 -0700
> @@ -320,9 +320,15 @@ static int mmc_init_card(struct mmc_host
>  	/*
>  	 * Fetch CID from card.
>  	 */
> -	err = mmc_all_send_cid(host, cid);
> -	if (err != MMC_ERR_NONE)
> -		goto err;
> +	if (mmc_host_is_spi(host)) {
> +		err = mmc_spi_send_cid(host, cid);
> +		if (err != MMC_ERR_NONE)
> +			goto err;
> +	} else {
> +		err = mmc_all_send_cid(host, cid);
> +		if (err != MMC_ERR_NONE)
> +			goto err;
> +	}
>  
>  	if (oldcard) {
>  		if (memcmp(cid, oldcard->raw_cid, sizeof(cid)) != 0)

This is entirely subjective, but wouldn't this be cleaner:

	if (mmc_host_is_spi(host))
		err = mmc_spi_send_cid(host, cid);
	else
		err = mmc_all_send_cid(host, cid);

	if (err != MMC_ERR_NONE)
		goto err;

It's fewer lines at least, which should make Linus happy. ;)

> @@ -401,13 +411,15 @@ static int mmc_sd_init_card(struct mmc_h
>  	}
>  
>  	/*
> -	 * Set card RCA.
> +	 * For native busses:  set card RCA and leave open drain mode.
>  	 */

While your in the processes of fixing my crappy comments, this command actually
gets, not sends the RCA (SD is a bit backwards). :)

> @@ -455,9 +467,11 @@ static int mmc_sd_init_card(struct mmc_h
>  		/*
>  		 * Fetch switch information from card.
>  		 */
> -		err = mmc_read_switch(card);
> -		if (err != MMC_ERR_NONE)
> -			goto free_card;
> +		if (!mmc_host_is_spi(host)) {
> +			err = mmc_read_switch(card);
> +			if (err != MMC_ERR_NONE)
> +				goto free_card;
> +		}
>  	}
>  
>  	/*

Don't you want high-speed on your SPI bus? ;)

Add a FIXME here if you don't have the cards to test this. Or implement it
according to spec and wait for bug reports.

> --- pxa.orig/drivers/mmc/core/sd_ops.c	2007-06-06 16:47:58.000000000 -0700
> +++ pxa/drivers/mmc/core/sd_ops.c	2007-06-10 13:00:34.000000000 -0700
> @@ -89,10 +89,10 @@ int mmc_app_cmd(struct mmc_host *host, s
>  
>  	if (card) {
>  		cmd.arg = card->rca << 16;
> -		cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
> +		cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_AC;
>  	} else {
>  		cmd.arg = 0;
> -		cmd.flags = MMC_RSP_R1 | MMC_CMD_BCR;
> +		cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_BCR;
>  	}
>  
>  	err = mmc_wait_for_cmd(host, &cmd, 0);

You also need to fix the check further down as SPI doesn't have a bit indicating
if it toggled into ACMD mode.

> @@ -175,6 +179,9 @@ int mmc_send_if_cond(struct mmc_host *ho
>  	int err;
>  	static const u8 test_pattern = 0xAA;
>  
> +	if (mmc_host_is_spi(host))
> +		return MMC_ERR_FAILED;
> +
>  	/*
>  	 * To support SD 2.0 cards, we must always invoke SD_SEND_IF_COND
>  	 * before SD_APP_OP_COND. This command will harmlessly fail for

See above, we need this for SDHC.

Rgds
-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [patch 3/4] MMC-over-SPI core updates

[David Brownell]

On Wednesday 13 June 2007, Pierre Ossman wrote:
> David Brownell wrote:
> > @@ -498,6 +505,19 @@ void mmc_detect_change(struct mmc_host *
> >  EXPORT_SYMBOL(mmc_detect_change);
> >  
> >  
> > +static int mmc_spi_fixup(struct mmc_host *host, u32 *ocrp)
> > +{
> > +	int err;
> > +
> > +	if (!mmc_host_is_spi(host))
> > +		return MMC_ERR_NONE;
> > +
> > +	err = mmc_spi_read_ocr(host, ocrp);
> > +	if (err == MMC_ERR_NONE);
> > +		err = mmc_spi_set_crc(host);
> > +	return err;
> > +}
> > +
> >  void mmc_rescan(struct work_struct *work)
> >  {
> >  	struct mmc_host *host =
> 
> I'd prefer if this is in mmc_rescan(). It's so little code that all this does is
> hide the init sequence.

I can move it.  I tried expanding it inline there, and thought
the result was worse:  (a) duplication, in both MMC and SD code
paths; (b) it complicates otherwise-clean logic; (c) you had
asked that the core patch be small.

You're quite sure you want those drawbacks ... ?


> > @@ -519,11 +539,13 @@ void mmc_rescan(struct work_struct *work
> >  		mmc_power_up(host);
> >  		mmc_go_idle(host);
> >  
> > -		mmc_send_if_cond(host, host->ocr_avail);
> > +		if (!mmc_host_is_spi(host))
> > +			mmc_send_if_cond(host, host->ocr_avail);
> >  
>
> If you have a look at the SD physical spec (the public one), you'll see that
> if_cond is needed on SPI aswell. I know you don't have cards for testing, but
> let's code according to spec and try to find people who can test all parts.

I just grabbed a new version of  that spec; the original omitted
absolutely *everything* about SPI.


> >  		err = mmc_send_app_op_cond(host, 0, &ocr);
> >  		if (err == MMC_ERR_NONE) {
> > -			if (mmc_attach_sd(host, ocr))
> > +			err = mmc_spi_fixup(host, &ocr);
> > +			if (err != MMC_ERR_NONE || mmc_attach_sd(host, ocr))
> >  				mmc_power_off(host);
> >  		} else {
> >  			/*
> 
> Looking at the example flow charts in the specs, this is not the proper way.
> I've spent the morning staring at both specs in parallel, and here's how I think
> it should be done:
> 
> [CMD8 for SD]
> CMD58 [with HCS bit for MMC]
> ACMD51, OK -> SD
> CMD1, OK -> MMC

I'll have to get back to you on that after I read that new SD doc.

 
> The SD spec states that both ACMD51 and CMD1 is legal, but nothing about why
> both are supported. It also states that CMD1 has a bit indicating SDHC support
> (as if CMD8 isn't enough), but ACMD51 doesn't. But the flow charts show ACMD51,
> not CMD1. :/

I think it's the usual reason:  ACMD fails on MMC, so use that early to
sort out SD-vs-MMC.  The Linux stack might be marginally quicker if that
were the *ONLY* reason it used ACMD51.

 
> The next step, in the bus handlers, will have to get the OCR on their own. SD
> states that the OCR is valid in the initial CMD58 (before CMD1/ACMD51), but MMC
> states that it is not. So for the sake of consistency, let both paths reissue a
> CMD58 for the OCR.

That's done in the spi_fixup() routine.  ;)

I observed that some cards didn't like CMD58 (READ_OCR) until after
the reset was done, although all the specs I have say that it's
supposed to work while the reset is happening.

 
> > --- pxa.orig/drivers/mmc/core/mmc_ops.c	2007-06-10 13:00:21.000000000 -0700
> > +++ pxa/drivers/mmc/core/mmc_ops.c	2007-06-10 13:00:34.000000000 -0700
> > @@ -31,6 +31,10 @@ static int _mmc_select_card(struct mmc_h
> >  
> >  	BUG_ON(!host);
> >  
> > +	/* SPI doesn't multiplex; "select" is meaningless */
> > +	if (mmc_host_is_spi(host))
> > +		return MMC_ERR_NONE;
> > +
> >  	memset(&cmd, 0, sizeof(struct mmc_command));
> >  
> >  	cmd.opcode = MMC_SELECT_CARD;
> 
> I do not like this. Unsupported commands shouldn't be issued. We should not have
> voodoo in here that makes them a no-op. That just obscures things. Feel free to
> put a check in there if you feel the risk is too great.

Again, you had asked for the core patch to be small ... and
the patch is smaller that way, than when replicating it to
both call site.


> > -int mmc_send_ext_csd(struct mmc_card *card, u8 *ext_csd)
> > +struct mmc_cxd {
> > +	struct mmc_card	*card;		/* optional */
> > +	void		*buf;
> > +	unsigned	len;
> > +	u32		opcode;
> > +	u32		arg;
> > +	unsigned	flags;
> > +};
> > +
> 
> This seems overly complex.

The direct translation of the existing code would have needed them
all as function parameters; ungood to have so many params!


> You can do this with arguments as most of that struct 
> is superfluous:
> 
>  - card can be specified at all times if you reorg the init a bit, allowing you
> to drop the host argument.

Hmm, that may be right.  I was trying to touch that code as
little as possible ... less to break that way!

 
>  - arg and flags are the same for all three commands, so those can be dropped.

Arg is zero (except in the native send_csd), and two of
the three commands are not valid down that route in the
native code.  So I suppose it wouldn't hurt to pass an R1
response code there, knowing that it will never be used,
even though the correct code for those is R2.  That sort
of trick is a bit too ugly for my personal taste; I would
not have thought of always passing invalid parameters.  ;)


> > @@ -230,6 +251,85 @@ int mmc_send_ext_csd(struct mmc_card *ca
> >  	return MMC_ERR_NONE;
> >  }
> >  
> > +int mmc_spi_read_ocr(struct mmc_host *host, u32 *ocrp)
> > +{
> 
> How about moving these further down so that the mmc_send_cxd_data() users are
> right below it?

I'll move it so that and the crc routine are _after_ them ... to
keep all related code together.


> Also, the theme I've tried to have here is to take a card pointer whenever you
> operate on a specific card, and a host pointer for bus wide things. It adds a
> bit of readability.

There's a difference for SPI?!?  :)

Actually, the CRC and OCR stuff gets called before a card struct
is available.  That may be different in the rest of the code.

 
> > --- pxa.orig/drivers/mmc/core/mmc.c	2007-06-10 13:00:21.000000000 -0700
> > +++ pxa/drivers/mmc/core/mmc.c	2007-06-10 13:00:34.000000000 -0700
> > @@ -320,9 +320,15 @@ static int mmc_init_card(struct mmc_host
> >  	/*
> >  	 * Fetch CID from card.
> >  	 */
> > -	err = mmc_all_send_cid(host, cid);
> > -	if (err != MMC_ERR_NONE)
> > -		goto err;
> > +	if (mmc_host_is_spi(host)) {
> > +		err = mmc_spi_send_cid(host, cid);
> > +		if (err != MMC_ERR_NONE)
> > +			goto err;
> > +	} else {
> > +		err = mmc_all_send_cid(host, cid);
> > +		if (err != MMC_ERR_NONE)
> > +			goto err;
> > +	}
> >  
> >  	if (oldcard) {
> >  		if (memcmp(cid, oldcard->raw_cid, sizeof(cid)) != 0)
> 
> This is entirely subjective, but wouldn't this be cleaner:
> 
> 	if (mmc_host_is_spi(host))
> 		err = mmc_spi_send_cid(host, cid);
> 	else
> 		err = mmc_all_send_cid(host, cid);
> 
> 	if (err != MMC_ERR_NONE)
> 		goto err;
> 
> It's fewer lines at least, which should make Linus happy. ;)

Likewise on the SD side.  Though ... *both* need to re-enable
the CRC mode, currently only the SD path does so; fixed.


> > @@ -401,13 +411,15 @@ static int mmc_sd_init_card(struct mmc_h
> >  	}
> >  
> >  	/*
> > -	 * Set card RCA.
> > +	 * For native busses:  set card RCA and leave open drain mode.
> >  	 */
> 
> While your in the processes of fixing my crappy comments, this command actually
> gets, not sends the RCA (SD is a bit backwards). :)

Done (in sd.c only).


> > @@ -455,9 +467,11 @@ static int mmc_sd_init_card(struct mmc_h
> >  		/*
> >  		 * Fetch switch information from card.
> >  		 */
> > -		err = mmc_read_switch(card);
> > -		if (err != MMC_ERR_NONE)
> > -			goto free_card;
> > +		if (!mmc_host_is_spi(host)) {
> > +			err = mmc_read_switch(card);
> > +			if (err != MMC_ERR_NONE)
> > +				goto free_card;
> > +		}
> >  	}
> >  
> >  	/*
> 
> Don't you want high-speed on your SPI bus? ;)
> 
> Add a FIXME here if you don't have the cards to test this. Or implement it
> according to spec and wait for bug reports.

I'll add a REVISIT, unless I get inspired by this updated spec
that I just downloaded.

 
> > --- pxa.orig/drivers/mmc/core/sd_ops.c	2007-06-06 16:47:58.000000000 -0700
> > +++ pxa/drivers/mmc/core/sd_ops.c	2007-06-10 13:00:34.000000000 -0700
> > @@ -89,10 +89,10 @@ int mmc_app_cmd(struct mmc_host *host, s
> >  
> >  	if (card) {
> >  		cmd.arg = card->rca << 16;
> > -		cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
> > +		cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_AC;
> >  	} else {
> >  		cmd.arg = 0;
> > -		cmd.flags = MMC_RSP_R1 | MMC_CMD_BCR;
> > +		cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_BCR;
> >  	}
> >  
> >  	err = mmc_wait_for_cmd(host, &cmd, 0);
> 
> You also need to fix the check further down as SPI doesn't have a bit indicating
> if it toggled into ACMD mode.

Right now this is not needed, since the host records that and hands
it back in the R1 status mapping.

Other than that status mapping, this is useful to make sure the right
tracing statements are emitted:  report ACMDx not CMDx.  It makes
message tracing easier to read.  (This is something the MMC/SD core
might benefit from, too.  Did I mention I'm glad that it now issues
the CMDx messages in decimal, matching all docs, not hex?  :)


> > @@ -175,6 +179,9 @@ int mmc_send_if_cond(struct mmc_host *ho
> >  	int err;
> >  	static const u8 test_pattern = 0xAA;
> >  
> > +	if (mmc_host_is_spi(host))
> > +		return MMC_ERR_FAILED;
> > +
> >  	/*
> >  	 * To support SD 2.0 cards, we must always invoke SD_SEND_IF_COND
> >  	 * before SD_APP_OP_COND. This command will harmlessly fail for
> 
> See above, we need this for SDHC.

If the doc I just downloaded covers SDHC, I can code it to spec.

- Dave


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [patch 3/4] MMC-over-SPI core updates

[Pierre Ossman]

David Brownell wrote:
> 
> I can move it.  I tried expanding it inline there, and thought
> the result was worse:  (a) duplication, in both MMC and SD code
> paths; (b) it complicates otherwise-clean logic; (c) you had
> asked that the core patch be small.
> 
> You're quite sure you want those drawbacks ... ?
> 

Fairly. The idea when I did the rewrite was that the initial detection (that
identifies card type) should be fairly clear and shouldn't require you to dig
around in other areas.

As for the patches being small, I guess I wasn't clear. I'd like to see small
patches so that they are easily reviewed, but the total changes can be as large
as needs be. The end results is the most important part, so if you do not see a
way to split up patches then I'll just have to deal with reviewing large ones.

> 
> I just grabbed a new version of  that spec; the original omitted
> absolutely *everything* about SPI.
> 

How evil.

> 
> I think it's the usual reason:  ACMD fails on MMC, so use that early to
> sort out SD-vs-MMC.  The Linux stack might be marginally quicker if that
> were the *ONLY* reason it used ACMD51.
> 

The specs are unclear, especially when comparing old and new ones. We'll just
have to feel our way forward here.

> 
> I observed that some cards didn't like CMD58 (READ_OCR) until after
> the reset was done, although all the specs I have say that it's
> supposed to work while the reset is happening.
> 

That's not good, as high-capacity MMC needs an initial READ_OCR. :/

> 
> The direct translation of the existing code would have needed them
> all as function parameters; ungood to have so many params!
> 

I don't think a temporary structure makes it better. It just makes things less
readable and gives the impression that it's a persistent object.

> 
> Hmm, that may be right.  I was trying to touch that code as
> little as possible ... less to break that way!
> 

Where's your sense of advanture? ;)

> 
> Arg is zero (except in the native send_csd), and two of
> the three commands are not valid down that route in the
> native code.  So I suppose it wouldn't hurt to pass an R1
> response code there, knowing that it will never be used,
> even though the correct code for those is R2.  That sort
> of trick is a bit too ugly for my personal taste; I would
> not have thought of always passing invalid parameters.  ;)
> 

I won't tell anyone if you don't ;)

> 
> There's a difference for SPI?!?  :)
> 
> Actually, the CRC and OCR stuff gets called before a card struct
> is available.  That may be different in the rest of the code.
> 

Right, never mind then.

>> You also need to fix the check further down as SPI doesn't have a bit indicating
>> if it toggled into ACMD mode.
> 
> Right now this is not needed, since the host records that and hands
> it back in the R1 status mapping.
> 

In other words, lying to the core. Someone looking at that function will get the
impression that the system can verify that the card entered app command mode
even on spi, even though it can't. Don't try to get the round peg into the
square hole. Change the hole.

> Other than that status mapping, this is useful to make sure the right
> tracing statements are emitted:  report ACMDx not CMDx.  It makes
> message tracing easier to read.  (This is something the MMC/SD core
> might benefit from, too.  Did I mention I'm glad that it now issues
> the CMDx messages in decimal, matching all docs, not hex?  :)
> 

On my todo list. :)

-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [patch 3/4] MMC-over-SPI core updates

[David Brownell]

On Friday 15 June 2007, Pierre Ossman wrote:
> David Brownell wrote:
> > 
> > I can move it.  I tried expanding it inline there, and thought
> > the result was worse:  (a) duplication, in both MMC and SD code
> > paths; (b) it complicates otherwise-clean logic; (c) you had
> > asked that the core patch be small.
> > 
> > You're quite sure you want those drawbacks ... ?
> > 
> 
> Fairly. The idea when I did the rewrite was that the initial detection (that
> identifies card type) should be fairly clear and shouldn't require you to dig
> around in other areas.

I figured calling mmc_spi_fixup() would be very clear, and
since it's defined right before the routine using it then
digging would be a non-issue.

But if you want uglified code, I suppose I could do that.  ;)


> >> You also need to fix the check further down as SPI doesn't have a bit indicating
> >> if it toggled into ACMD mode.
> > 
> > Right now this is not needed, since the host records that and hands
> > it back in the R1 status mapping.
> > 
> 
> In other words, lying to the core.

I can't see it that way.  It's just a different way of managing
that status than you currently expect.


> Someone looking at that function will get the 
> impression that the system can verify that the card entered app command mode
> even on spi, even though it can't. Don't try to get the round peg into the
> square hole. Change the hole.

So, get rid of all R1_APP_CMD checks in the MMC stack?
That's what I'll do for now.

I count two of them, and they are both superfluous since
they follow successful completion of MMC_APP_CMD.  The
only way that can succeed is if the card supports APP_CMD
and enters that mode...

(The alternative would be to do the superfluous checks
only for non-SPI hosts, but that'd seem pointless.)

- Dave

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [patch 3/4] MMC-over-SPI core updates

[Pierre Ossman]

David Brownell wrote:
> 
> I can't see it that way.  It's just a different way of managing
> that status than you currently expect.
> 

You seem to be stuck in the idea that we want some voodoo translation layer
between native MMC and the SPI version. That is not desirable for a multitude of
reasons, so make sure the core knows SPI and the host driver is just a hardware
interface (like the other host drivers).

> 
> So, get rid of all R1_APP_CMD checks in the MMC stack?
> That's what I'll do for now.
> 
> I count two of them, and they are both superfluous since
> they follow successful completion of MMC_APP_CMD.  The
> only way that can succeed is if the card supports APP_CMD
> and enters that mode...
> 

The specs that was coded against suggested it was prudent to check the bit. It's
not like it's a measurable performance hit to do it.

> (The alternative would be to do the superfluous checks
> only for non-SPI hosts, but that'd seem pointless.)

I don't agree. Preferably we would always check it, but we can't with SPI so
we'll have to cross our fingers there.

Rgds
-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [patch 3/4] MMC-over-SPI core updates

[David Brownell]

On Wednesday 20 June 2007, Pierre Ossman wrote:
> You seem to be stuck in the idea that we want some voodoo translation layer
> between native MMC and the SPI version. That is not desirable for a multitude of
> reasons, so make sure the core knows SPI and the host driver is just a hardware
> interface (like the other host drivers).

I *HAVE* been moving SPI knowledge into the core, at your request.

Remember that the "how does it move" problems turn up issues.
You shouldn't take my comments as "being stuck" on some kind of
"voodoo" notion.  Any more than I take *your* comments that way.

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

[patch 4/4] MMC-over-SPI host driver

[David Brownell]

This is the latest version of the MMC-over-SPI support.  It works
on 2.6.22-rc4-mm1, along with the preceding patches which teach the
rest of the MMC stack about SPI so that this host driver can focus
on doing only lowlevel SPI-specific stuff.

It's been lightly tested on MMC and SD cards, reading and writing
ext3 filesystems (including fsck).  This includes CRC mode, which
is now enabled by default.

The main issue of note was observed on one SD card:  sometimes a
write triggers CRC errors, then the card seems to stop responding
until it's been power cycled.  Work around the former by passing
the "use_crc=n" module parameter, if it appears on your platform.

Signed-off-by: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
Cc: mikael.starvik-VrBV9hrLPhE@public.gmane.org,
Cc: Hans-Peter Nilsson <hp-VrBV9hrLPhE@public.gmane.org>
Cc: Jan Nikitenko <jan.nikitenko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Mike Lavender <mike-UTnDXsALFwNjMdQLN6DIHgC/G2K4zDHf@public.gmane.org>

---
 drivers/mmc/host/Kconfig    |   13 
 drivers/mmc/host/Makefile   |    1 
 drivers/mmc/host/mmc_spi.c  | 1526 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/spi/mmc_spi.h |   32 
 4 files changed, 1572 insertions(+)

--- pxa.orig/drivers/mmc/host/Kconfig	2007-06-06 16:47:58.000000000 -0700
+++ pxa/drivers/mmc/host/Kconfig	2007-06-10 13:00:41.000000000 -0700
@@ -100,3 +100,16 @@ config MMC_TIFM_SD
           To compile this driver as a module, choose M here: the
 	  module will be called tifm_sd.
 
+config MMC_SPI
+	tristate "MMC/SD over SPI"
+	depends on MMC && SPI_MASTER && EXPERIMENTAL
+	select CRC7
+	select CRC_ITU_T
+	help
+	  Some systems accss MMC/SD cards using the SPI protocol instead of
+	  using an MMC/SD controller.  The disadvantage of using SPI is that
+	  it's often not as fast; its compensating advantage is that SPI is
+	  available on many systems without native MMC/SD controllers.
+
+	  If unsure, or if your system has no SPI controller driver, say N.
+
--- pxa.orig/drivers/mmc/host/Makefile	2007-06-06 16:47:58.000000000 -0700
+++ pxa/drivers/mmc/host/Makefile	2007-06-10 13:00:41.000000000 -0700
@@ -15,4 +15,5 @@ obj-$(CONFIG_MMC_AU1X)		+= au1xmmc.o
 obj-$(CONFIG_MMC_OMAP)		+= omap.o
 obj-$(CONFIG_MMC_AT91)		+= at91_mci.o
 obj-$(CONFIG_MMC_TIFM_SD)	+= tifm_sd.o
+obj-$(CONFIG_MMC_SPI)		+= mmc_spi.o
 
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ pxa/include/linux/spi/mmc_spi.h	2007-06-10 13:00:41.000000000 -0700
@@ -0,0 +1,32 @@
+#ifndef __LINUX_SPI_MMC_SPI_H
+#define __LINUX_SPI_MMC_SPI_H
+
+struct device;
+struct mmc_host;
+
+/* Put this in platform_data of a device being used to manage an MMC/SD
+ * card slot.  (Modeled after PXA mmc glue; see that for usage examples.)
+ *
+ * REVISIT This is not a spi-specific notion.  Any card slot should be
+ * able to handle it.  If the MMC core doesn't adopt this kind of notion,
+ * switch the "struct device *" parameters over to "struct spi_device *".
+ */
+struct mmc_spi_platform_data {
+	/* driver activation and (optional) card detect irq hookup */
+	int (*init)(struct device *,
+		irqreturn_t (*)(int, void *),
+		void *);
+	void (*exit)(struct device *, void *);
+
+	/* how long to debounce card detect, in msecs */
+	unsigned detect_delay;
+
+	/* sense switch on sd cards */
+	int (*get_ro)(struct device *);
+
+	/* power management */
+	unsigned int ocr_mask;			/* available voltages */
+	void (*setpower)(struct device *, unsigned int maskval);
+};
+
+#endif /* __LINUX_SPI_MMC_SPI_H */
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ pxa/drivers/mmc/host/mmc_spi.c	2007-06-10 13:00:41.000000000 -0700
@@ -0,0 +1,1526 @@
+/*
+ * mmc_spi.c - Access SD/MMC cards through SPI master controllers
+ *
+ * (C) Copyright 2005, Intec Automation,
+ *		Mike Lavender (mike@steroidmicros)
+ * (C) Copyright 2006-2007, David Brownell
+ * (C) Copyright 2007, Axis Communications,
+ *		Hans-Peter Nilsson (hp-VrBV9hrLPhE@public.gmane.org)
+ * (C) Copyright 2007, ATRON electronic GmbH,
+ *		Jan Nikitenko <jan.nikitenko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
+ *
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+#include <linux/init.h>
+#include <linux/types.h>
+#include <linux/delay.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/device.h>
+#include <linux/blkdev.h>
+#include <linux/dma-mapping.h>
+
+#include <linux/mmc/host.h>
+#include <linux/mmc/mmc.h>
+
+#include <linux/spi/spi.h>
+#include <linux/spi/mmc_spi.h>
+#include <linux/crc7.h>
+#include <linux/crc-itu-t.h>
+
+
+/* NOTES:
+ *
+ * - For now, we won't try to interoperate with a real mmc/sd/sdio
+ *   controller, although some of them do have hardware support for
+ *   SPI protocol.  The main reason for such configs would be mmc-ish
+ *   cards like DataFlash, which don't support that "native" protocol.
+ *   SPI mode is a bit slower than non-parallel versions of MMC.
+ *
+ * - Likewise we don't try to detect DataFlash cards, which would
+ *   imply switching to a different driver (one which doesn't accept
+ *   MMC/SD commands).  Not many folk folk use both DataFlash cards
+ *   and MMC/SD cards, and Linux has no "MMC/SD card" level interface
+ *   distinct from "native" MMC protocols.
+ *
+ * - Protocol details, including timings, need to be audited
+ *
+ * - MMC depends on a different chipselect management policy than the
+ *   SPI interface currently supports:  it needs to issue multiple
+ *   spi_message requests without dropping the chipselect, using the
+ *   results of one to decide the next one to issue.  Pending updates
+ *   to the programming interface, this driver insists that it not
+ *   share the bus with other drivers, preventing conflicts.
+ *
+ * - We tell the controller to keep the chipselect active from the
+ *   beginning of an mmc_host_ops.request until the end.  So beware
+ *   of SPI controller drivers that don't handle the cs_change flag!
+ *   However, many cards seem OK with chipselect flapping up/down
+ *   during that time, if nobody else is talking on the bus.
+ *
+ * - We need <linux/mmc/mmc.h> for the response flag values and for
+ *   a handful of requests which need special handling.  There's no
+ *   silicon to embed that protocol knowledge for us.
+ */
+
+
+/*
+ * Local protocol constants ... ones that shouldn't ever need
+ * to be visible to upper layer code.
+ */
+
+#define SPI_MMC_COMMAND		0x40	/* mask into mmc command */
+
+/* response tokens used to ack each block written: */
+#define SPI_MMC_RESPONSE_CODE(x)	((x) & 0x1f)
+#define SPI_RESPONSE_ACCEPTED		((2 << 1)|1)
+#define SPI_RESPONSE_CRC_ERR		((5 << 1)|1)
+#define SPI_RESPONSE_WRITE_ERR		((6 << 1)|1)
+
+/* read and write blocks start with these tokens and end with crc;
+ * on error, read tokens act like a subset of R2_SPI_* values.
+ */
+#define SPI_TOKEN_SINGLE	0xfe	/* single block r/w, multiblock read */
+#define SPI_TOKEN_MULTI_WRITE	0xfc	/* multiblock write */
+#define SPI_TOKEN_STOP_TRAN	0xfd	/* terminate multiblock write */
+
+
+#define CRC_GO_IDLE_STATE	0x95	/* constant CRC for GO_IDLE */
+#define CRC_NO_CRC		0x01	/* placeholder for no-crc cmds */
+
+#define	MMC_POWERCYCLE_MSECS	20		/* board-specific? */
+
+
+/* The unit for these timeouts is milliseconds.  See mmc_spi_scanbyte.  */
+#define MINI_TIMEOUT		1
+#define READ_TIMEOUT		100
+#define WRITE_TIMEOUT		250
+
+
+#define ECRC	EBFONT
+
+#define	MMC_SPI_BLOCKSIZE	512
+
+/****************************************************************************/
+
+/*
+ * Local Data Structures
+ */
+
+union mmc_spi_command {
+	u8 buf[7];
+	struct {
+		u8 dummy;
+		u8 code;
+		u8 addr1;
+		u8 addr2;
+		u8 addr3;
+		u8 addr4;
+		u8 crc;
+	} command;
+};
+
+/* "scratch" is per-{command,block} data exchanged with the card */
+struct scratch {
+	u8			status[18];
+	u8			data_token;
+	__be16			crc_val;
+};
+
+struct mmc_spi_host {
+	struct mmc_host		*mmc;
+	struct spi_device	*spi;
+	u8			*rx_buf;
+	u8			*tx_buf;
+	u32			tx_idx;
+	u32			rx_idx;
+	u8			app_cmd;
+
+	struct mmc_spi_platform_data	*pdata;
+
+	/* for bulk data transfers */
+	struct spi_transfer	token, t, crc;
+	struct spi_message	m;
+	struct spi_transfer	early_status;
+
+	/* for status readback */
+	struct spi_transfer	status;
+	struct spi_message	readback;
+
+	/* underlying controller might support dma, but we can't
+	 * rely on it being used for any particular request
+	 */
+	struct device		*dma_dev;
+	dma_addr_t		dma;		/* of mmc_spi_host */
+
+	/* REVISIT get rid of the embedded buffers.  Do it all using
+	 * the scratch buffer; don't dma map this struct.
+	 */
+
+	/* pre-allocated dma-safe buffers */
+	union mmc_spi_command	command;
+	u8			status_byte;
+	u8			response[2];
+
+	struct scratch		*data;
+	dma_addr_t		data_dma;
+
+	/* Specs say to write ones most of the time, even when the card
+	 * has no need to read its input data; and many cards won't care.
+	 * This is our source of those ones.
+	 */
+	void			*ones;
+	dma_addr_t		ones_dma;
+};
+
+#ifdef	DEBUG
+static unsigned debug = 1;
+module_param(debug, uint, 0644);
+#else
+#define	debug	0
+#endif
+
+static int use_crc = 1;
+module_param(use_crc, bool, 0);
+
+
+/****************************************************************************/
+
+static inline int mmc_cs_off(struct mmc_spi_host *host)
+{
+	/* chipselect will always be inactive after setup() */
+	return spi_setup(host->spi);
+}
+
+static inline int mmc_spi_readbyte(struct mmc_spi_host *host)
+{
+	int status = spi_sync(host->spi, &host->readback);
+	if (status < 0)
+		return status;
+	return host->status_byte;
+}
+
+static int
+mmc_spi_readbytes(struct mmc_spi_host *host, void *bytes, unsigned len)
+{
+	int status;
+	int dma_mapped = host->readback.is_dma_mapped;
+	int dma = host->status.rx_dma;
+
+	host->status.rx_buf = bytes;
+	host->status.len = len;
+
+	host->readback.is_dma_mapped = 0;
+	status = spi_sync(host->spi, &host->readback);
+	host->readback.is_dma_mapped = dma_mapped;
+
+	host->status.rx_buf = &host->status_byte;
+	host->status.rx_dma = dma;
+	host->status.len = 1;
+	return status;
+}
+
+
+/* REVISIT:  is this fast enough?  these kinds of sync points could
+ * easily be offloaded to irq-ish code called by controller drivers,
+ * eliminating context switch costs.
+ *
+ * REVISIT:  after writes and erases, mmc_spi_busy() == true might be
+ * a fair hint to yield exclusive access to the card (so another driver
+ * can use the bus).  Busy-wait won't be an issue, since we already
+ * yield the CPU during all synchronous I/O calls.
+ */
+static int mmc_spi_busy(u8 byte)
+{
+	return byte == 0;
+}
+
+static int mmc_spi_delayed(u8 byte)
+{
+	return byte == 0xff;
+}
+
+static int
+mmc_spi_scanbyte(struct mmc_spi_host *host, int (*fail)(u8), unsigned delay)
+{
+	int		value = 0;
+	unsigned	wait;
+	unsigned	end_wait;
+
+	/*
+	 * Because we might (we will, for bitbanged SPI) be scheduled
+	 * out for extensive periods in this call, we'd get an
+	 * abundance of timeouts if we counted in jiffies on a system
+	 * with load, so instead we calculate it in the max number of
+	 * bytes we could theoretically scan before the timeout, if
+	 * everything else took zero time.
+	 *
+	 * REVISIT max_speed_hz may be a lot faster than our actual
+	 * transfer rate.  And byte I/O is *VERY* slow...
+	 */
+	end_wait = (delay * host->spi->max_speed_hz) / (1000 * 8);
+
+	for (wait = 0; wait < end_wait; wait++) {
+		value = mmc_spi_readbyte(host);
+		if (value < 0)
+			return value;
+		if (!fail(value)) {
+			if (debug > 1)
+				dev_dbg(&host->spi->dev,
+					"  mmc_spi: token %02x, wait %u\n",
+					value, wait);
+			return value;
+		}
+	}
+
+	dev_dbg(&host->spi->dev,
+			"  mmc_spi: scanbyte timeout, %02x (%u)\n",
+			value, end_wait);
+	return -ETIMEDOUT;
+}
+
+/*
+ * We provide raw command status as well as mapped status:
+ *	cmd->resp[0] - mapped, like 'native' mmc/sd
+ *	cmd->resp[1] - OCR (for READ_OCR only)
+ *	cmd->resp[2] - SPI R1
+ *	cmd->resp[3] - SPI R2 (for SEND_STATUS only)
+ *
+ * REVISIT presumably we can't escape providng mapped status,
+ * even though few callers actually check for it ...
+ */
+
+static inline void mmc_spi_map_r1(struct mmc_command *cmd, u8 r1)
+{
+	u32	mapped = 0;
+
+	cmd->resp[2] = r1;
+
+	/* spi mode doesn't expose the mmc/sd state machine, but
+	 * we can at least avoid lying about the IDLE state
+	 */
+	if (!(r1 & R1_SPI_IDLE))
+		mapped |= R1_STATE(R1_STATE_STBY);
+
+	/* type 'sr' (not an error) */
+	if (r1 & R1_SPI_ERASE_RESET)
+		mapped |= R1_ERASE_RESET;
+
+	/* types 'er' or 'erx', generic cmd->error code */
+	if (r1 & (R1_SPI_ERASE_SEQ
+			| R1_SPI_ADDRESS
+			| R1_SPI_PARAMETER)) {
+		cmd->error = MMC_ERR_FAILED;
+		if (r1 & R1_SPI_ERASE_SEQ)
+			mapped |= R1_ERASE_SEQ_ERROR;
+		/* erx */
+		if (r1 & R1_SPI_ADDRESS)
+			mapped |= R1_ADDRESS_ERROR;
+		/* REVISIT how to map R1_SPI_PARAMETER?
+		 * this collides with R2_SPI_OUT_OF_RANGE...
+		 */
+		if (r1 & R1_SPI_PARAMETER)
+			mapped |= R1_OUT_OF_RANGE;
+	}
+
+	/* type 'er' with special cmd->error codes */
+	if (r1 & R1_SPI_ILLEGAL_COMMAND) {
+		cmd->error = MMC_ERR_INVALID;
+		mapped |= R1_ILLEGAL_COMMAND;
+	}
+	if (r1 & R1_SPI_COM_CRC) {
+		cmd->error = MMC_ERR_BADCRC;
+		mapped |= R1_COM_CRC_ERROR;
+	}
+
+	cmd->resp[0] = mapped;
+}
+
+static void mmc_spi_map_r2(struct mmc_command *cmd, u8 r2)
+{
+	u32	mapped = 0;
+
+	cmd->resp[3] = r2;
+	if (!r2)
+		return;
+
+	/* type 'erx' */
+	if (r2 & R2_SPI_ERROR)
+		mapped |= R1_ERROR;
+	if (r2 & R2_SPI_CC_ERROR)
+		mapped |= R1_CC_ERROR;
+	if (r2 & R2_SPI_WP_VIOLATION)
+		mapped |= R1_WP_VIOLATION;
+	if (r2 & R2_SPI_OUT_OF_RANGE)
+		mapped |= R1_OUT_OF_RANGE;
+
+	/* type 'ex' */
+	if (r2 & R2_SPI_CARD_ECC_ERROR)
+		mapped |= R1_CARD_ECC_FAILED;
+	if (r2 & R2_SPI_ERASE_PARAM)
+		mapped |= R1_ERASE_PARAM;
+
+	/* NOTE:  we never set cmd->error, that would indicate that
+	 * the SEND_STATUS command failed ...
+	 */
+
+	/* type 'sx' */
+	if (r2 & R2_SPI_CARD_LOCKED)
+		mapped |= R1_CARD_IS_LOCKED;
+	if (r2 & R2_SPI_WP_ERASE_SKIP)
+		mapped |= R1_WP_ERASE_SKIP;
+
+	cmd->resp[0] |= mapped;
+}
+
+static void mmc_spi_map_data_err(struct mmc_command *cmd, u8 token)
+{
+	cmd->resp[0] = 0;
+	mmc_spi_map_r2(cmd, (token & 0x0f) << 2);
+	if (token & 0x10)
+		cmd->resp[0] |= R1_CARD_IS_LOCKED;
+}
+
+static char *maptype(struct mmc_command *cmd)
+{
+	switch (mmc_spi_resp_type(cmd)) {
+	case MMC_RSP_SPI_R1:	return "R1";
+	case MMC_RSP_SPI_R1B:	return "R1B";
+	case MMC_RSP_SPI_R2:	return "R2";
+	case MMC_RSP_SPI_R3:	return "R3";
+	default:		return "?";
+	}
+}
+
+static int mmc_spi_response_get(struct mmc_spi_host *host,
+		struct mmc_command *cmd, int cs_on)
+{
+	int value;
+	char tag[32];
+
+	snprintf(tag, sizeof tag, "  ... %sCMD%d response SPI_%s",
+		host->app_cmd ? "A" : "",
+		cmd->opcode, maptype(cmd));
+
+	if (cmd->opcode == MMC_STOP_TRANSMISSION) {
+		/*
+		 * We can't tell whether we read block data or the
+		 * command reply, so to cope with trash data during
+		 * the latency, we just read in 14 bytes (8 would be
+		 * enough according to the MMC spec; SD doesn't say)
+		 * after the command and fake a clean reply.  We could
+		 * avoid this if we saved what the card sent us while
+		 * we sent the command, and treat it like a normal
+		 * response if we didn't get a SPI_TOKEN_SINGLE.
+		 */
+		(void) mmc_spi_readbytes(host, host->command.buf,
+				sizeof host->command.buf);
+		(void) mmc_spi_readbytes(host, host->command.buf,
+				sizeof host->command.buf);
+		value = 0;
+	} else
+		value = mmc_spi_scanbyte(host, mmc_spi_delayed, MINI_TIMEOUT);
+
+	if (value < 0) {
+		dev_dbg(&host->spi->dev,
+			"%s: response error, %d\n", tag, value);
+		if (value == -ETIMEDOUT)
+			cmd->error = MMC_ERR_TIMEOUT;
+		else
+			cmd->error = MMC_ERR_FAILED;
+		goto fail;
+	}
+
+	if (value & 0x80) {
+		dev_dbg(&host->spi->dev, "%s: INVALID RESPONSE, %02x\n",
+					tag, host->response[0]);
+		cmd->error = MMC_ERR_FAILED;
+		value = -EBADR;
+		goto fail;
+	}
+
+	host->response[0] = value;
+	host->response[1] = 0;
+
+	cmd->error = MMC_ERR_NONE;
+	mmc_spi_map_r1(cmd, host->response[0]);
+
+	switch (mmc_spi_resp_type(cmd)) {
+
+	/* SPI R1B == R1 + busy; STOP_TRANSMISSION and less-common stuff */
+	case MMC_RSP_SPI_R1B:
+		(void) mmc_spi_scanbyte(host, mmc_spi_busy, WRITE_TIMEOUT);
+		break;
+
+	/* SPI R2 == R1 + second status byte; SEND_STATUS */
+	case MMC_RSP_SPI_R2:
+		host->response[1] = mmc_spi_readbyte(host);
+		mmc_spi_map_r2(cmd, host->response[1]);
+		cmd->resp[0] |= R1_READY_FOR_DATA;
+		/* we aren't reporting that SEND_STATUS failed... */
+		cmd->error = MMC_ERR_NONE;
+		break;
+
+	/* SPI R3 == R1 + OCR; used only by READ_OCR */
+	case MMC_RSP_SPI_R3:
+		(void) mmc_spi_readbytes(host, &cmd->resp[1], 4);
+		be32_to_cpus(&cmd->resp[1]);
+		break;
+
+	/* SPI R1 == just one status byte */
+	case MMC_RSP_SPI_R1:
+		break;
+
+	default:
+		dev_dbg(&host->spi->dev, "bad response type %04x\n",
+				mmc_spi_resp_type(cmd));
+		BUG();
+	}
+
+	if (!host->app_cmd
+			&& cmd->error == MMC_ERR_NONE
+			&& cmd->opcode == MMC_APP_CMD) {
+		host->app_cmd = 1;
+		cmd->resp[0] |= R1_APP_CMD;
+	}
+	if (debug > 1)
+		dev_dbg(&host->spi->dev, "%s: resp %02x.%02x\n",
+			tag,
+			host->response[1], host->response[0]);
+	if (cs_on)
+		return 0;
+	value = 0;
+
+	/* chipselect off on errors plus various success cases */
+fail:
+	mmc_cs_off(host);
+	return value;
+}
+
+/* Issue command and read its response.
+ * Returns zero on success, negative for error.
+ *
+ * On error, caller must cope with mmc core retry mechanism.  That
+ * means immediate low-level resubmit, which affects the bus lock...
+ */
+static int
+mmc_spi_command_send(struct mmc_spi_host *host,
+		struct mmc_request *mrq, u8 crc,
+		struct mmc_command *cmd, int cs_on)
+{
+	union mmc_spi_command	*tx = &host->command;
+	u32			arg = cmd->arg;
+	int			status;
+	struct spi_transfer	*t;
+	unsigned		opcode = cmd->opcode;
+
+	/* after 8 clock cycles the card is ready, and done previous cmd */
+	tx->command.dummy = 0xFF;
+
+	tx->command.code = opcode | SPI_MMC_COMMAND;
+	tx->command.addr1 = (u8)(arg >> 24);
+	tx->command.addr2 = (u8)(arg >> 16);
+	tx->command.addr3 = (u8)(arg >> 8);
+	tx->command.addr4 = (u8)arg;
+	if (use_crc)
+		tx->command.crc = (crc7(0, &tx->command.code, 5) << 1) | 0x01;
+	else
+		tx->command.crc = crc;
+
+	dev_dbg(&host->spi->dev, "  mmc_spi: %sCMD%d, MMC_SPI_%s\n",
+		host->app_cmd ? "A" : "", opcode,
+		maptype(cmd));
+
+	/* send command, leaving chipselect active */
+	spi_message_init(&host->m);
+	t = &host->t;
+	memset(t, 0, sizeof *t);
+	t->tx_buf = tx->buf;
+	t->len = sizeof(tx->buf);
+	t->cs_change = 1;
+	spi_message_add_tail(t, &host->m);
+
+	status = spi_sync(host->spi, &host->m);
+	if (status < 0) {
+		dev_dbg(&host->spi->dev, "  ... write returned %d\n", status);
+		cmd->error = MMC_ERR_FAILED;
+		return status;
+	}
+
+	/* after no-data commands and STOP_TRANSMISSION, chipselect off */
+	status = mmc_spi_response_get(host, cmd, cs_on);
+
+	/*
+	 * If this was part of a successful request with a stop-part,
+	 * our caller signals the request as done.
+	 */
+	if (status == 0 && mrq->stop == NULL)
+		mmc_request_done(host->mmc, mrq);
+	return status;
+}
+
+/* Build data message with up to four separate transfers.
+ *
+ * We always provide TX data, initialized to all ones if we're reading;
+ * thats's RX data and CRC, plus RX/TX status.  The MMC/SD protocol
+ * requires us to write ones; but Linux defaults to writing zeroes.
+ *
+ * We also handle DMA mapping, so the underlying SPI controller does
+ * not need to (re)do it for each message.
+ */
+static void
+mmc_spi_setup_data_message(
+	struct mmc_spi_host	*host,
+	int			multiple,
+	enum dma_data_direction	direction)
+{
+	struct spi_transfer	*t;
+	struct scratch		*scratch = host->data;
+	dma_addr_t		dma = host->data_dma;
+
+	spi_message_init(&host->m);
+	if (dma)
+		host->m.is_dma_mapped = 1;
+
+	/* for reads, we (manually) skip 0xff bytes before finding
+	 * the token; for writes, this transfer issues that token.
+	 */
+	if (direction == DMA_TO_DEVICE) {
+		t = &host->token;
+		memset(t, 0, sizeof *t);
+		t->len = 1;
+		if (multiple)
+			scratch->data_token = SPI_TOKEN_MULTI_WRITE;
+		else
+			scratch->data_token = SPI_TOKEN_SINGLE;
+		t->tx_buf = &scratch->data_token;
+		if (dma)
+			t->tx_dma = dma + offsetof(struct scratch, data_token);
+		spi_message_add_tail(t, &host->m);
+	}
+
+	/* Body of transfer is buffer, then CRC ...
+	 * either TX-only, or RX with TX-ones.
+	 */
+	t = &host->t;
+	memset(t, 0, sizeof *t);
+	t->tx_buf = host->ones;
+	t->tx_dma = host->ones_dma;
+	/* length and actual buffer info are written later */
+	spi_message_add_tail(t, &host->m);
+
+	t = &host->crc;
+	memset(t, 0, sizeof *t);
+	t->len = 2;
+	if (direction == DMA_TO_DEVICE) {
+		/* the actual CRC may get written later */
+		scratch->crc_val = CRC_NO_CRC;
+		t->tx_buf = &scratch->crc_val;
+		if (dma)
+			t->tx_dma = dma + offsetof(struct scratch, crc_val);
+	} else {
+		t->tx_buf = host->ones;
+		t->tx_dma = host->ones_dma;
+		t->rx_buf = &scratch->crc_val;
+		if (dma)
+			t->rx_dma = dma + offsetof(struct scratch, crc_val);
+	}
+	spi_message_add_tail(t, &host->m);
+
+	/*
+	 * Early status is always RX, with TX-ones ... and chipselect
+	 * always stays active when this completes.
+	 *
+	 * If this is a read, we need room for N(AC) (==1) all-ones bytes,
+	 * and the first potential token.
+	 *
+	 * For a write, the one-byte data response follows immediately, then
+	 * come zero or more busy bytes, then N(WR) (== one or more) all-ones
+	 * until we write the next token.  We can try to minimize I/O ops by
+	 * using a single read to collect end-of-busy.
+	 */
+	t = &host->early_status;
+	memset(t, 0, sizeof *t);
+	t->len = (direction == DMA_FROM_DEVICE) ? 2 : sizeof(scratch->status);
+	t->tx_buf = host->ones;
+	t->tx_dma = host->ones_dma;
+	t->rx_buf = scratch->status;
+	if (dma)
+		t->rx_dma = dma + offsetof(struct scratch, status);
+	t->cs_change = 1;
+	spi_message_add_tail(t, &host->m);
+}
+
+static inline int resp2status(u8 write_resp)
+{
+	switch (SPI_MMC_RESPONSE_CODE(write_resp)) {
+	case SPI_RESPONSE_ACCEPTED:
+		return 0;
+	case SPI_RESPONSE_CRC_ERR:
+		/* host shall then issue MMC_STOP_TRANSMISSION */
+		return -ECRC;
+	case SPI_RESPONSE_WRITE_ERR:
+		/* host shall then issue MMC_STOP_TRANSMISSION,
+		 * and should MMC_SEND_STATUS to sort it out
+		 */
+		return -EIO;
+	default:
+		return -EILSEQ;
+	}
+}
+
+/*
+ * Write one block:
+ *  - preceded by N(WR) [1+] all-ones bytes
+ *  - data block
+ *	+ token
+ *	+ data bytes
+ *	+ crc16
+ *  - an all-ones byte ... card writes a data-response byte
+ *  - followed by N(EC) [0+] all-ones bytes, card writes zero/'busy'
+ *
+ * Return negative errno, or 0xff.
+ */
+static inline int
+mmc_spi_writeblock(struct mmc_spi_host *host, struct spi_transfer *t)
+{
+	struct spi_device	*spi = host->spi;
+	int			status, i;
+	struct scratch		*scratch = host->data;
+
+	if (use_crc)
+		scratch->crc_val = cpu_to_be16(
+				crc_itu_t(0, t->tx_buf, t->len));
+	if (host->dma_dev)
+		dma_sync_single_for_device(host->dma_dev,
+				host->data_dma, sizeof *scratch,
+				DMA_BIDIRECTIONAL);
+
+	status = spi_sync(spi, &host->m);
+	if (status != 0) {
+		dev_dbg(&spi->dev, "write error (%d)\n", status);
+		return status;
+	}
+
+	if (host->dma_dev)
+		dma_sync_single_for_cpu(host->dma_dev,
+				host->data_dma, sizeof *scratch,
+				DMA_BIDIRECTIONAL);
+
+	/*
+	 * Get the transmission data-response reply.  It must follow
+	 * immediately after the data block we transferred.  This reply
+	 * doesn't necessarily tell whether the write operation succeeded;
+	 * it just says if the transmission was ok and whether *earlier*
+	 * writes succeeded; see the standard.
+	 */
+	status = resp2status(scratch->status[0]);
+	if (status != 0) {
+		dev_dbg(&spi->dev, "write error %02x (%d)\n",
+			scratch->status[0], status);
+		return status;
+	}
+	t->tx_buf += t->len;
+	if (host->dma_dev)
+		t->tx_dma += t->len;
+
+	/* Return when not busy.  Maybe we collected that status already;
+	 * in the worst case, we need more I/O.
+	 *
+	 * REVISIT that worst case isn't all that uncommon.  Avoid the
+	 * costly byte-at-a-time scanning in this case.
+	 */
+	for (i = 1; i < sizeof scratch->status; i++) {
+		if (scratch->status[i] != 0)
+			return 0;
+	}
+	return mmc_spi_scanbyte(host, mmc_spi_busy, WRITE_TIMEOUT);
+}
+
+/*
+ * Read one block:
+ *  - preceded by N(AC) [1+] all-ones bytes
+ *  - data block
+ *	+ token
+ *	+ data bytes
+ *	+ crc16
+ *  - followed by
+ *      + (single block) N(EC) [0+] all-ones bytes
+ *      + (multi block) more blocks till STOP_TRANSMISSION from host
+ */
+static inline int mmc_spi_readblock(struct mmc_spi_host *host,
+		struct mmc_command *cmd, struct spi_transfer *t)
+{
+	struct spi_device	*spi = host->spi;
+	int			status;
+	struct scratch		*scratch = host->data;
+
+	/* N(AC) is *at least* one byte; this "must not happen" */
+	if (scratch->status[0] != 0xff) {
+		dev_dbg(&spi->dev, "too-early card response %02x %02x\n",
+				scratch->status[0], scratch->status[1]);
+		return -EIO;
+	}
+
+	if (scratch->status[1] != 0xff)
+		status = scratch->status[1];
+	else
+		status = mmc_spi_scanbyte(host, mmc_spi_delayed, READ_TIMEOUT);
+
+	if (status == SPI_TOKEN_SINGLE) {
+		if (host->dma_dev)
+			dma_sync_single_for_device(host->dma_dev,
+					host->data_dma, sizeof *scratch,
+					DMA_BIDIRECTIONAL);
+
+		status = spi_sync(spi, &host->m);
+		if (host->dma_dev)
+			dma_sync_single_for_cpu(host->dma_dev,
+					host->data_dma, sizeof *scratch,
+					DMA_BIDIRECTIONAL);
+	} else {
+		/* we've read extra garbage, timed out, etc */
+		dev_dbg(&spi->dev, "read error %02x (%d)\n", status, status);
+		if (status < 0)
+			return status;
+		mmc_spi_map_data_err(cmd, status);
+		return -EIO;
+	}
+
+	if (use_crc) {
+		u16 crc = crc_itu_t(0, t->rx_buf, t->len);
+
+		be16_to_cpus(&scratch->crc_val);
+		if (scratch->crc_val != crc) {
+			dev_dbg(&spi->dev, "read - crc error: crc_val=0x%04x, "
+					"computed=0x%04x len=%d\n",
+					scratch->crc_val, crc, t->len);
+			return -ECRC;
+		}
+	}
+
+	t->rx_buf += t->len;
+	if (host->dma_dev)
+		t->rx_dma += t->len;
+
+	return 0;
+}
+
+/*
+ * An MMC/SD data stage includes one or more blocks, optional CRCs,
+ * and inline handshaking.  That handhaking makes it unlike most
+ * other SPI protocol stacks.
+ */
+static void
+mmc_spi_data_do(struct mmc_spi_host *host, struct mmc_command *cmd,
+		struct mmc_data *data, u32 blk_size)
+{
+	struct spi_device	*spi = host->spi;
+	struct device		*dma_dev = host->dma_dev;
+	struct spi_transfer	*t;
+	enum dma_data_direction	direction;
+	struct scatterlist	*sg;
+	unsigned		n_sg;
+	int			multiple;
+
+	if (data->flags & MMC_DATA_READ) {
+		direction = DMA_FROM_DEVICE;
+		multiple = (cmd->opcode == MMC_READ_MULTIPLE_BLOCK);
+
+		/*
+		 * We need to scan for the SPI_TOKEN_SINGLE token
+		 * *before* we issue the first (of multiple)
+		 * spi_messages reading the data plus two extra bytes,
+		 * (implying N\subscript{AC} and the *next* token), so
+		 * to avoid looking at garbage from an earlier
+		 * command, we reset the location where we'll read in
+		 * subsequent tokens.
+		 */
+		host->data->status[0] = 0xff;
+		host->data->status[1] = 0xff;
+	} else {
+		direction = DMA_TO_DEVICE;
+		multiple = (cmd->opcode == MMC_WRITE_MULTIPLE_BLOCK);
+	}
+	mmc_spi_setup_data_message(host, multiple, direction);
+	t = &host->t;
+
+	/* Handle scatterlist segments one at a time, with synch for
+	 * each 512-byte block
+	 */
+	for (sg = data->sg, n_sg = data->sg_len; n_sg; n_sg--, sg++) {
+		int			status = 0;
+		dma_addr_t		dma_addr = 0;
+		void			*kmap_addr;
+		unsigned		length = sg->length;
+
+		/* set up dma mapping for controller drivers that might
+		 * use DMA ... though they may fall back to PIO
+		 */
+		if (dma_dev) {
+			dma_addr = dma_map_page(dma_dev, sg->page, 0,
+						PAGE_SIZE, direction);
+			if (direction == DMA_TO_DEVICE)
+				t->tx_dma = dma_addr + sg->offset;
+			else
+				t->rx_dma = dma_addr + sg->offset;
+		}
+
+		/* allow pio too, with kmap handling any highmem */
+		kmap_addr = kmap(sg->page);
+		if (direction == DMA_TO_DEVICE)
+			t->tx_buf = kmap_addr + sg->offset;
+		else
+			t->rx_buf = kmap_addr + sg->offset;
+
+		/* transfer each block, and update request status */
+		while (length && status == 0) {
+			t->len = min(length, blk_size);
+
+			dev_dbg(&host->spi->dev,
+				"    mmc_spi: %s block, %d bytes\n",
+				(direction == DMA_TO_DEVICE)
+				? "write"
+				: "read",
+				t->len);
+
+			if (direction == DMA_TO_DEVICE)
+				status = mmc_spi_writeblock(host, t);
+			else
+				status = mmc_spi_readblock(host, cmd, t);
+			if (status < 0)
+				break;
+
+			data->bytes_xfered += t->len;
+			length -= t->len;
+
+			/* REVISIT can status ever become nonzero here? */
+			status = host->m.status;
+
+			if (!multiple)
+				break;
+		}
+
+		/* discard mappings */
+		if (direction == DMA_FROM_DEVICE)
+			flush_kernel_dcache_page(sg->page);
+		kunmap(sg->page);
+		if (dma_dev)
+			dma_unmap_page(dma_dev, dma_addr,
+					PAGE_SIZE, direction);
+
+		if (status < 0) {
+			dev_dbg(&spi->dev, "%s status %d\n",
+				(direction == DMA_TO_DEVICE)
+					? "write" : "read",
+				status);
+			if (status == -ECRC) {
+				cmd->error = MMC_ERR_BADCRC;
+				cmd->resp[0] = R1_COM_CRC_ERROR;
+			} else if (cmd->error == MMC_ERR_NONE)
+				cmd->error = MMC_ERR_FAILED;
+			break;
+		}
+	}
+
+	/* NOTE if caller issues SET_BLOCK_COUNT before a multiblock write,
+	 * this STOP_TRAN logic isn't needed except when we stop early for
+	 * errors.  Currently, mmc_block doesn't issue that request.
+	 */
+	if (direction == DMA_TO_DEVICE && multiple) {
+		struct scratch	*scratch = host->data;
+		int		tmp;
+		const unsigned	statlen = sizeof(scratch->status);
+
+		dev_dbg(&spi->dev, "    mmc_spi: STOP_TRAN\n");
+
+		/* Tweak the per-block message we set up earlier by morphing
+		 * it to hold single buffer with the token followed by some
+		 * all-ones bytes ... skip N(BR) (0..1), scan the rest for
+		 * "not busy any longer" status, and leave chip selected.
+		 */
+		INIT_LIST_HEAD(&host->m.transfers);
+		list_add(&host->early_status.transfer_list,
+				&host->m.transfers);
+
+		scratch->status[0] = SPI_TOKEN_STOP_TRAN;
+		memset(scratch->status + 1, 0xff, statlen - 1);
+		host->early_status.tx_buf = host->early_status.rx_buf;
+		host->early_status.tx_dma = host->early_status.rx_dma;
+		host->early_status.len = 1 + statlen;
+
+		if (host->dma_dev)
+			dma_sync_single_for_device(host->dma_dev,
+					host->data_dma, sizeof *scratch,
+					DMA_BIDIRECTIONAL);
+
+		tmp = spi_sync(spi, &host->m);
+
+		if (host->dma_dev)
+			dma_sync_single_for_cpu(host->dma_dev,
+					host->data_dma, sizeof *scratch,
+					DMA_BIDIRECTIONAL);
+
+		if (tmp < 0) {
+			if (cmd->error == MMC_ERR_NONE)
+				cmd->error = MMC_ERR_FAILED;
+			return;
+		}
+
+		/* ideally we collected "not busy" status with one I/O,
+		 * avoiding wasteful byte-at-a-time scanning...
+		 */
+		for (tmp = 2; tmp < statlen; tmp++) {
+			if (scratch->status[tmp] != 0)
+				return;
+		}
+
+		/* REVISIT we don't need to wait here.  It's OK to deselect
+		 * if we verify that it's not still busy before starting
+		 * the next command.
+		 */
+
+		/* Else, wait until the end of the busy period */
+		tmp = mmc_spi_scanbyte(host, mmc_spi_busy, WRITE_TIMEOUT);
+		if (tmp < 0 && cmd->error == MMC_ERR_NONE) {
+			if (tmp == -ETIMEDOUT)
+				cmd->error = MMC_ERR_TIMEOUT;
+			else
+				cmd->error = MMC_ERR_FAILED;
+		}
+	}
+}
+
+/* handle three MMC request stages:  commmand (required), data, and stop */
+static int
+mmc_spi_command_do(struct mmc_spi_host *host, struct mmc_request *mrq)
+{
+	int status;
+	int cs_on = mrq->data || mrq->stop;
+
+	if (mrq->data && (mrq->data->blksz > MMC_SPI_BLOCKSIZE)) {
+		mrq->cmd->error = MMC_ERR_INVALID;
+		return -EINVAL;
+	}
+
+	status = mmc_spi_command_send(host, mrq, CRC_NO_CRC,
+			mrq->cmd, cs_on);
+
+	if (status == 0 && mrq->data)
+		mmc_spi_data_do(host, mrq->cmd, mrq->data,
+				mrq->data->blksz);
+	if (mrq->stop) {
+		if (status == 0) {
+			cs_on = 0;
+			status = mmc_spi_command_send(host, mrq,
+					CRC_NO_CRC, mrq->stop, cs_on);
+			if (status != 0)
+				mrq->stop->error = MMC_ERR_FAILED;
+			mmc_request_done(host->mmc, mrq);
+		}
+	}
+
+	/* drop chipselect if it wasn't already done */
+	if (cs_on)
+		mmc_cs_off(host);
+
+	/*
+	 * No need to wait before the next command.  The minimum time
+	 * between commands is handled by the "dummy" byte in the command.
+	 */
+
+	return status;
+}
+
+/*
+ * RESET is started when cmd->opcode == MMC_GO_IDLE_STATE.  This can't
+ * be just a standard protocol operation.
+ *
+ * We expect the MMC core to be responsible for "very hard" resets like
+ * power cycling the card; there's no accessible reset signal.
+ */
+static int
+mmc_spi_initialize(struct mmc_spi_host *host, struct mmc_request *mrq)
+{
+	struct mmc_command	*cmd = mrq->cmd;
+	int			status;
+	int			could_invert_cs = 0;
+
+	dev_dbg(&host->spi->dev, "INITIALIZE...\n");
+
+	host->app_cmd = 0;
+
+	/* Try to be very sure any previous command has completed;
+	 * wait till not-busy, skip debris from any old commands.
+	 */
+	(void) mmc_spi_scanbyte(host, mmc_spi_busy, WRITE_TIMEOUT);
+	(void) mmc_spi_readbytes(host, host->command.buf,
+			sizeof host->command.buf);
+
+	/*
+	 * Do a burst with chipselect deactivated.  We need to do this
+	 * to meet the requirement of 74 clock cycles with chipselect
+	 * high before CMD0.  (Section 6.4.1, in "Simplified Physical
+	 * Layer Specification 2.0".)  Some cards are particularly
+	 * needy of this (e.g. Viking "SD256") while most others don't
+	 * seem to care.  Note that it's not enough to deactivate
+	 * chipselect without toggling the clock.  Beware of the hack:
+	 * we "know" that mmc_spi_readbytes uses the host->status
+	 * spi_transfer.
+	 *
+	 * Note that this is one of two places MMC/SD plays games with
+	 * the SPI protocol.  The other is that when chipselect is
+	 * released while the card returns BUSY status, the clock must
+	 * issue several cycles with chipselect high before the card
+	 * will stop driving its output.
+	 */
+	host->spi->mode |= SPI_CS_HIGH;
+	if (spi_setup(host->spi) != 0)
+		/* Just a warning; most cards work without it. */
+		dev_warn(&host->spi->dev,
+				"can't invert the active chip-select level\n");
+	else
+		could_invert_cs = 1;
+
+	(void) mmc_spi_readbytes(host, host->command.buf,
+			sizeof host->command.buf);
+	(void) mmc_spi_readbytes(host, host->command.buf,
+			sizeof host->command.buf);
+
+	host->spi->mode &= ~SPI_CS_HIGH;
+	if (spi_setup(host->spi) != 0) {
+		/* Wot, we can't get (back) the same setup we had before? */
+		dev_err(&host->spi->dev,
+				"failed restoring chip-select level\n");
+		return -EIO;
+	}
+
+	/* issue software reset */
+	cmd->arg = 0;
+	status = mmc_spi_command_send(host, mrq, CRC_GO_IDLE_STATE, cmd, 0);
+	if (status < 0) {
+		/* Maybe:
+		 *  - there's no card present
+		 *  - the card isn't seated correctly (bad contacts)
+		 *  - it didn't leave MMC/SD mode
+		 *  - there's other confusion in the card state
+		 *
+		 * Power cycling the card ought to help a lot.
+		 * At any rate, let's try again.
+		 */
+		status = mmc_spi_command_send(host, mrq,
+				CRC_GO_IDLE_STATE, cmd, 0);
+		if (status < 0)
+			dev_dbg(&host->spi->dev,
+				"can't initialize; no card%s?\n",
+				could_invert_cs
+					? ""
+					: " or chip-select error");
+	}
+	return status;
+}
+
+/****************************************************************************/
+
+/*
+ * MMC driver implementation -- the interface to the MMC stack
+ */
+
+static void mmc_spi_request(struct mmc_host *mmc, struct mmc_request *mrq)
+{
+	struct mmc_spi_host	*host = mmc_priv(mmc);
+	int			status = -EINVAL;
+	struct mmc_command	*cmd;
+
+	/* MMC core and layered drivers *MUST* issue SPI-aware commands */
+	cmd = mrq->stop;
+	if (cmd && !mmc_spi_resp_type(cmd)) {
+		dev_dbg(&host->spi->dev, "bogus STOP command\n");
+		dump_stack();
+		cmd->error = MMC_ERR_INVALID;
+		goto fail;
+	}
+
+	cmd = mrq->cmd;
+	if (!mmc_spi_resp_type(cmd)) {
+		dev_dbg(&host->spi->dev, "bogus command\n");
+		dump_stack();
+		cmd->error = MMC_ERR_INVALID;
+		goto fail;
+	}
+
+	if (!host->app_cmd) {
+		if (cmd->opcode == MMC_GO_IDLE_STATE)
+			status = mmc_spi_initialize(host, mrq);
+		else
+			status = mmc_spi_command_do(host, mrq);
+	} else {
+		status = mmc_spi_command_do(host, mrq);
+		host->app_cmd = 0;
+	}
+
+fail:
+	/*
+	 * If status was ok, the request would have been signalled done by
+	 * mmc_spi_command_do.
+	 */
+	if (status < 0)
+		mmc_request_done(host->mmc, mrq);
+}
+
+
+static void mmc_spi_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
+{
+	struct mmc_spi_host *host = mmc_priv(mmc);
+
+	if (host->pdata && host->pdata->setpower) {
+		dev_dbg(&host->spi->dev,
+			"mmc_spi:  power %08x\n", ios->vdd);
+		host->pdata->setpower(&host->spi->dev, ios->vdd);
+		if (ios->vdd)
+			msleep(MMC_POWERCYCLE_MSECS);
+		else {
+			int mres;
+
+			/*
+			 * We need to put all spi wires to low,
+			 * otherwise MMC card is powered from them
+			 * regardless it's power supply state!
+			 *
+			 * We leave chipselect active low,
+			 * switch mode to 0 to put clock to low.
+			 */
+			host->spi->mode &= ~(SPI_CPOL|SPI_CPHA);
+			mres = spi_setup(host->spi);
+			if (mres < 0)
+				dev_dbg(&host->spi->dev,
+					"switch to SPI mode 0 failed\n");
+
+			if (spi_w8r8(host->spi, 0x00) < 0)
+				dev_dbg(&host->spi->dev,
+					"put spi signals to low failed\n");
+
+			/*
+			 * Now clock should be low due to spi mode 0;
+			 * MOSI should be low because of written 0x00;
+			 * chipselect should be low (it is active low)
+			 * power supply is off, so now MMC is off too!
+			 *
+			 * FIXME no, chipselect can be high since the
+			 * device is inactive and SPI_CS_HIGH is clear...
+			 */
+			msleep(MMC_POWERCYCLE_MSECS);
+			if (mres == 0) {
+				host->spi->mode |= (SPI_CPOL|SPI_CPHA);
+				mres = spi_setup(host->spi);
+				if (mres < 0)
+					dev_dbg(&host->spi->dev,
+						"switch back to SPI mode 3"
+						" failed\n");
+			}
+		}
+	}
+
+	if (host->spi->max_speed_hz != ios->clock && ios->clock != 0) {
+		int		status;
+
+		host->spi->max_speed_hz = ios->clock;
+		status = spi_setup(host->spi);
+		dev_dbg(&host->spi->dev,
+			"mmc_spi:  clock to %d Hz, %d\n",
+			host->spi->max_speed_hz, status);
+	}
+}
+
+static int mmc_spi_get_ro(struct mmc_host *mmc)
+{
+	struct mmc_spi_host *host = mmc_priv(mmc);
+
+	if (host->pdata && host->pdata->get_ro)
+		return host->pdata->get_ro(mmc->parent);
+	/* board doesn't support read only detection; assume writeable */
+	return 0;
+}
+
+
+static const struct mmc_host_ops mmc_spi_ops = {
+	.request	= mmc_spi_request,
+	.set_ios	= mmc_spi_set_ios,
+	.get_ro		= mmc_spi_get_ro,
+};
+
+
+/****************************************************************************/
+
+/*
+ * SPI driver implementation
+ */
+
+static irqreturn_t
+mmc_spi_detect_irq(int irq, void *mmc)
+{
+	struct mmc_spi_host *host = mmc_priv(mmc);
+
+	mmc_detect_change(mmc, msecs_to_jiffies(host->pdata->detect_delay));
+	return IRQ_HANDLED;
+}
+
+static int mmc_spi_probe(struct spi_device *spi)
+{
+	void			*ones;
+	struct mmc_host		*mmc;
+	struct mmc_spi_host	*host;
+	int			status;
+
+	/* SanDisk and Hitachi MMC docs both show clock timing diagrams
+	 * with clock starting low (CPOL=0) and sampling on leading edge
+	 * (CPHA=0); clock is measured between rising edges.  Sandisk SD
+	 * docs show clock starting high (CPOL=1) and sampling on trailing
+	 * edge (CPHA=1), measuring between falling edges.
+	 *
+	 * Docs are very explicit that sampling is on the rising edge, so
+	 * the difference between SPI_MODE_0 and SPI_MODE_3 may not matter.
+	 */
+	spi->mode |= SPI_CPOL | SPI_CPHA;
+	spi->bits_per_word = 8;
+
+	status = spi_setup(spi);
+	if (status < 0) {
+		dev_dbg(&spi->dev, "needs SPI mode %02x, %d KHz; %d\n",
+				spi->mode, spi->max_speed_hz / 1000,
+				status);
+		return status;
+	}
+
+	/* We can use the bus safely if nobody else will interfere with
+	 * us.  That is, either we have the experimental exclusive access
+	 * primitives ... or else there's nobody to share it with.
+	 */
+	if (spi->master->num_chipselect > 1) {
+		struct device	*parent = spi->dev.parent;
+
+		/* If there are multiple devices on this bus, we
+		 * can't proceed.
+		 */
+		spin_lock(&parent->klist_children.k_lock);
+		if (parent->klist_children.k_list.next
+				!= parent->klist_children.k_list.prev)
+			status = -EMLINK;
+		else
+			status = 0;
+		spin_unlock(&parent->klist_children.k_lock);
+		if (status < 0) {
+			dev_err(&spi->dev, "can't share SPI bus\n");
+			return status;
+		}
+
+		/* REVISIT we can't guarantee another device won't
+		 * be added later.  It's uncommon though ... for now,
+		 * work as if this is safe.
+		 */
+		dev_warn(&spi->dev, "ASSUMING unshared SPI bus!\n");
+	}
+
+	/* We need a supply of ones to transmit.  This is the only time
+	 * the CPU touches these, so cache coherency isn't a concern.
+	 */
+	status = -ENOMEM;
+	ones = kmalloc(MMC_SPI_BLOCKSIZE, GFP_KERNEL);
+	if (!ones)
+		goto nomem;
+	memset(ones, 0xff, MMC_SPI_BLOCKSIZE);
+
+	mmc = mmc_alloc_host(sizeof *host, &spi->dev);
+	if (!mmc)
+		goto nomem;
+
+	mmc->ops = &mmc_spi_ops;
+
+	/* As long as we keep track of the number of successfully
+	 * transmitted blocks, we're good for multiwrite.
+	 */
+	mmc->caps = MMC_CAP_SPI | MMC_CAP_MULTIWRITE;
+	if (use_crc)
+		mmc->caps |= MMC_CAP_SPI_CRC;
+
+	/* SPI doesn't need the lowspeed device identification thing for
+	 * MMC or SD cards, since it never comes up in open drain mode.
+	 * That's good; some SPI masters can't handle very low speeds!
+	 *
+	 * However, low speed SDIO cards need not handle over 400 KHz;
+	 * that's the only reason not to use a few MHz for f_min (until
+	 * the upper layer reads the target frequency from the CSD).
+	 */
+	mmc->f_min = 400000;
+	mmc->f_max = spi->max_speed_hz;
+
+	host = mmc_priv(mmc);
+	host->mmc = mmc;
+	host->spi = spi;
+
+	host->ones = ones;
+
+	/* Platform data is used to hook up things like card sensing
+	 * and power switching gpios.
+	 */
+	host->pdata = spi->dev.platform_data;
+	if (host->pdata)
+		mmc->ocr_avail = host->pdata->ocr_mask;
+	if (!mmc->ocr_avail) {
+		dev_warn(&spi->dev, "ASSUMING 3.2-3.4 V slot power\n");
+		mmc->ocr_avail = MMC_VDD_32_33|MMC_VDD_33_34;
+	}
+
+	dev_set_drvdata(&spi->dev, mmc);
+
+	/* preallocate dma buffers */
+	host->data = kmalloc(sizeof *host->data, GFP_KERNEL);
+	if (!host->data)
+		goto fail_nobuf1;
+
+	/* setup message for status readback/write-ones */
+	spi_message_init(&host->readback);
+	spi_message_add_tail(&host->status, &host->readback);
+	host->status.tx_buf = host->ones;
+	host->status.rx_buf = &host->status_byte;
+	host->status.len = 1;
+	host->status.cs_change = 1;
+
+	if (spi->master->cdev.dev->dma_mask) {
+		struct device	*dev = spi->master->cdev.dev;
+
+		host->dma_dev = dev;
+		host->dma = dma_map_single(dev, host,
+				sizeof *host, DMA_BIDIRECTIONAL);
+		host->ones_dma = dma_map_single(dev, ones,
+				MMC_SPI_BLOCKSIZE, DMA_TO_DEVICE);
+		host->data_dma = dma_map_single(dev, host->data,
+				sizeof *host->data, DMA_BIDIRECTIONAL);
+
+		/* REVISIT in theory those map operations can fail... */
+
+		dma_sync_single_for_cpu(host->dma_dev,
+				host->data_dma, sizeof *host->data,
+				DMA_BIDIRECTIONAL);
+
+#ifdef	CONFIG_HIGHMEM
+		dev_dbg(&spi->dev, "highmem + dma-or-pio ...\n");
+#endif
+	}
+
+	/* register card detect irq */
+	if (host->pdata && host->pdata->init) {
+		status = host->pdata->init(&spi->dev, mmc_spi_detect_irq, mmc);
+		if (status != 0)
+			goto fail_glue_init;
+	}
+
+	status = mmc_add_host(mmc);
+	if (status != 0)
+		goto fail_add_host;
+
+	dev_info(&spi->dev, "SD/MMC host %s%s%s\n",
+			mmc->class_dev.bus_id,
+			use_crc ? ", use CRCs" : "",
+			(host->pdata && host->pdata->setpower)
+				? ", card poweron/off"
+				: "");
+	return 0;
+
+fail_add_host:
+	mmc_remove_host (mmc);
+	if (host->dma_dev)
+		dma_unmap_single(host->dma_dev, host->dma,
+				sizeof *host, DMA_BIDIRECTIONAL);
+fail_glue_init:
+	if (host->dma_dev)
+		dma_unmap_single(host->dma_dev, host->data_dma,
+				sizeof *host->data, DMA_BIDIRECTIONAL);
+	kfree(host->data);
+
+fail_nobuf1:
+	mmc_free_host(mmc);
+	dev_set_drvdata(&spi->dev, NULL);
+
+nomem:
+	kfree(ones);
+	return status;
+}
+
+
+static int __devexit mmc_spi_remove(struct spi_device *spi)
+{
+	struct mmc_host		*mmc = dev_get_drvdata(&spi->dev);
+	struct mmc_spi_host	*host;
+
+	if (mmc) {
+		host = mmc_priv(mmc);
+
+		/* prevent new mmc_detect_change() calls */
+		if (host->pdata && host->pdata->exit)
+			host->pdata->exit(&spi->dev, mmc);
+
+		mmc_remove_host(mmc);
+
+		if (host->dma_dev) {
+			dma_unmap_single(host->dma_dev, host->dma,
+				sizeof *host, DMA_BIDIRECTIONAL);
+			dma_unmap_single(host->dma_dev, host->ones_dma,
+				MMC_SPI_BLOCKSIZE, DMA_TO_DEVICE);
+			dma_unmap_single(host->dma_dev, host->data_dma,
+				sizeof *host->data, DMA_BIDIRECTIONAL);
+		}
+
+		kfree(host->data);
+		kfree(host->ones);
+
+		spi->max_speed_hz = mmc->f_max;
+		mmc_free_host(mmc);
+		dev_set_drvdata(&spi->dev, NULL);
+	}
+	return 0;
+}
+
+
+static struct spi_driver mmc_spi_driver = {
+	.driver = {
+		.name =		"mmc_spi",
+		.bus =		&spi_bus_type,
+		.owner =	THIS_MODULE,
+	},
+	.probe =	mmc_spi_probe,
+	.remove =	__devexit_p(mmc_spi_remove),
+};
+
+
+static int __init mmc_spi_init(void)
+{
+	return spi_register_driver(&mmc_spi_driver);
+}
+module_init(mmc_spi_init);
+
+
+static void __exit mmc_spi_exit(void)
+{
+	spi_unregister_driver(&mmc_spi_driver);
+}
+module_exit(mmc_spi_exit);
+
+
+MODULE_AUTHOR("Mike Lavender, David Brownell, "
+		"Hans-Peter Nilsson, Jan Nikitenko");
+MODULE_DESCRIPTION("SPI SD/MMC host driver");
+MODULE_LICENSE("GPL");

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [patch 4/4] MMC-over-SPI host driver

[Pierre Ossman]

David Brownell wrote:
> +#include <linux/device.h>
> +#include <linux/blkdev.h>
> +#include <linux/dma-mapping.h>
> +
> +#include <linux/mmc/host.h>
> +#include <linux/mmc/mmc.h>
> +

This header file is a warning sign. Hopefully it's just the mapping that needs
it, so it should go away in the next revision.

> +
> +
> +#define CRC_GO_IDLE_STATE	0x95	/* constant CRC for GO_IDLE */

Isn't this just optimisation now that we have CRC calculation? And if so, is the
speed gain worth the complexity?

> +
> +/* The unit for these timeouts is milliseconds.  See mmc_spi_scanbyte.  */
> +#define MINI_TIMEOUT		1
> +#define READ_TIMEOUT		100
> +#define WRITE_TIMEOUT		250
> +

Shouldn't these be set in the request by the core?

> +
> +	/* Specs say to write ones most of the time, even when the card
> +	 * has no need to read its input data; and many cards won't care.
> +	 * This is our source of those ones.
> +	 */
> +	void			*ones;

If this is just read, can't it be a global const?

> +
> +	if (cmd->opcode == MMC_STOP_TRANSMISSION) {
> +		/*
> +		 * We can't tell whether we read block data or the
> +		 * command reply, so to cope with trash data during
> +		 * the latency, we just read in 14 bytes (8 would be
> +		 * enough according to the MMC spec; SD doesn't say)
> +		 * after the command and fake a clean reply.  We could
> +		 * avoid this if we saved what the card sent us while
> +		 * we sent the command, and treat it like a normal
> +		 * response if we didn't get a SPI_TOKEN_SINGLE.
> +		 */
> +		(void) mmc_spi_readbytes(host, host->command.buf,
> +				sizeof host->command.buf);
> +		(void) mmc_spi_readbytes(host, host->command.buf,
> +				sizeof host->command.buf);
> +		value = 0;

Didn't you tweak the block driver to not send MMC_STOP_TRANSMISSION?

> +
> +	if (!host->app_cmd
> +			&& cmd->error == MMC_ERR_NONE
> +			&& cmd->opcode == MMC_APP_CMD) {
> +		host->app_cmd = 1;
> +		cmd->resp[0] |= R1_APP_CMD;
> +	}

Why does the host need to keep track of app command state?

> +
> +	/*
> +	 * If this was part of a successful request with a stop-part,
> +	 * our caller signals the request as done.
> +	 */
> +	if (status == 0 && mrq->stop == NULL)
> +		mmc_request_done(host->mmc, mrq);
> +	return status;
> +}

I assume the comment should have read "without".

> +
> +static inline int resp2status(u8 write_resp)
> +{
> +	switch (SPI_MMC_RESPONSE_CODE(write_resp)) {
> +	case SPI_RESPONSE_ACCEPTED:
> +		return 0;
> +	case SPI_RESPONSE_CRC_ERR:
> +		/* host shall then issue MMC_STOP_TRANSMISSION */
> +		return -ECRC;

What's with the made-up errno?

We should check what other parts of the kernel uses for CRC errors.

> +
> +	if (data->flags & MMC_DATA_READ) {
> +		direction = DMA_FROM_DEVICE;
> +		multiple = (cmd->opcode == MMC_READ_MULTIPLE_BLOCK);
> +

multiple = (cmd->data->blocks > 1)

> +	} else {
> +		direction = DMA_TO_DEVICE;
> +		multiple = (cmd->opcode == MMC_WRITE_MULTIPLE_BLOCK);
> +	}

same thing here

> +
> +		/* allow pio too, with kmap handling any highmem */
> +		kmap_addr = kmap(sg->page);
> +		if (direction == DMA_TO_DEVICE)
> +			t->tx_buf = kmap_addr + sg->offset;
> +		else
> +			t->rx_buf = kmap_addr + sg->offset;
> +

If you want highmem, you also need to be prepared for clustered sg entries. kmap
only maps a single page, and a clustered sg might be larger than that.

> +
> +	/* NOTE if caller issues SET_BLOCK_COUNT before a multiblock write,
> +	 * this STOP_TRAN logic isn't needed except when we stop early for
> +	 * errors.  Currently, mmc_block doesn't issue that request.
> +	 */

But will it hurt? Other commands might behave like that.

> +
> +	if (mrq->data && (mrq->data->blksz > MMC_SPI_BLOCKSIZE)) {
> +		mrq->cmd->error = MMC_ERR_INVALID;
> +		return -EINVAL;
> +	}
> +

If you've set your parameters correct, then this shouldn't be possible. So do a
BUG_ON() if you want to have a test.

> +
> +/*
> + * RESET is started when cmd->opcode == MMC_GO_IDLE_STATE.  This can't
> + * be just a standard protocol operation.
> + *
> + * We expect the MMC core to be responsible for "very hard" resets like
> + * power cycling the card; there's no accessible reset signal.
> + */

Why can't you do this on POWER_ON? I do not like it looking at opcodes, even if
this one isn't likely to change.

> +	/*
> +	 * Do a burst with chipselect deactivated.  We need to do this
> +	 * to meet the requirement of 74 clock cycles with chipselect
> +	 * high before CMD0.  (Section 6.4.1, in "Simplified Physical
> +	 * Layer Specification 2.0".)  Some cards are particularly
> +	 * needy of this (e.g. Viking "SD256") while most others don't
> +	 * seem to care.  Note that it's not enough to deactivate
> +	 * chipselect without toggling the clock.  Beware of the hack:
> +	 * we "know" that mmc_spi_readbytes uses the host->status
> +	 * spi_transfer.
> +	 *
> +	 * Note that this is one of two places MMC/SD plays games with
> +	 * the SPI protocol.  The other is that when chipselect is
> +	 * released while the card returns BUSY status, the clock must
> +	 * issue several cycles with chipselect high before the card
> +	 * will stop driving its output.
> +	 */

Sounds like this signaling can be done with the chip select ios already present
in the mmc layer. Then we won't need voodoo in the host driver.

> +
> +	/* issue software reset */
> +	cmd->arg = 0;
> +	status = mmc_spi_command_send(host, mrq, CRC_GO_IDLE_STATE, cmd, 0);
> +	if (status < 0) {
> +		/* Maybe:
> +		 *  - there's no card present
> +		 *  - the card isn't seated correctly (bad contacts)
> +		 *  - it didn't leave MMC/SD mode
> +		 *  - there's other confusion in the card state
> +		 *
> +		 * Power cycling the card ought to help a lot.
> +		 * At any rate, let's try again.
> +		 */
> +		status = mmc_spi_command_send(host, mrq,
> +				CRC_GO_IDLE_STATE, cmd, 0);
> +		if (status < 0)
> +			dev_dbg(&host->spi->dev,
> +				"can't initialize; no card%s?\n",
> +				could_invert_cs
> +					? ""
> +					: " or chip-select error");
> +	}

No, no, no... This is not the kind of behaviour you want to hide down in the
host driver. This is a decision the core should make. Don't put the layer cake
in the blender.

> +
> +	/* MMC core and layered drivers *MUST* issue SPI-aware commands */
> +	cmd = mrq->stop;
> +	if (cmd && !mmc_spi_resp_type(cmd)) {
> +		dev_dbg(&host->spi->dev, "bogus STOP command\n");
> +		dump_stack();
> +		cmd->error = MMC_ERR_INVALID;
> +		goto fail;
> +	}
> +

This seems like something that should just be in debug builds.

> +
> +static int mmc_spi_get_ro(struct mmc_host *mmc)
> +{
> +	struct mmc_spi_host *host = mmc_priv(mmc);
> +
> +	if (host->pdata && host->pdata->get_ro)
> +		return host->pdata->get_ro(mmc->parent);
> +	/* board doesn't support read only detection; assume writeable */
> +	return 0;
> +}
> +

A printk() might be nice so the user knows why the switch isn't being respected.

> +
> +	/* SanDisk and Hitachi MMC docs both show clock timing diagrams
> +	 * with clock starting low (CPOL=0) and sampling on leading edge
> +	 * (CPHA=0); clock is measured between rising edges.  Sandisk SD
> +	 * docs show clock starting high (CPOL=1) and sampling on trailing
> +	 * edge (CPHA=1), measuring between falling edges.
> +	 *
> +	 * Docs are very explicit that sampling is on the rising edge, so
> +	 * the difference between SPI_MODE_0 and SPI_MODE_3 may not matter.
> +	 */

Sure you haven't looked at high-speed versus legacy clocking?

> +
> +	if (spi->master->cdev.dev->dma_mask) {
> +		struct device	*dev = spi->master->cdev.dev;
> +
> +		host->dma_dev = dev;
> +		host->dma = dma_map_single(dev, host,
> +				sizeof *host, DMA_BIDIRECTIONAL);
> +		host->ones_dma = dma_map_single(dev, ones,
> +				MMC_SPI_BLOCKSIZE, DMA_TO_DEVICE);
> +		host->data_dma = dma_map_single(dev, host->data,
> +				sizeof *host->data, DMA_BIDIRECTIONAL);
> +

This might be wasteful if the system has an iommu.

Rgds
-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [patch 4/4] MMC-over-SPI host driver

[David Brownell]

On Wednesday 13 June 2007, Pierre Ossman wrote:
> David Brownell wrote:
> > +#include <linux/device.h>
> > +#include <linux/blkdev.h>
> > +#include <linux/dma-mapping.h>
> > +
> > +#include <linux/mmc/host.h>
> > +#include <linux/mmc/mmc.h>
> > +
> 
> This header file is a warning sign. Hopefully it's just the mapping that needs
> it, so it should go away in the next revision.

There were other cases, as I recall.  Maybe the core has changed
so much that some of them are no longer needed.  I removed as
many as seemed safe.


> > +
> > +
> > +#define CRC_GO_IDLE_STATE	0x95	/* constant CRC for GO_IDLE */
> 
> Isn't this just optimisation now that we have CRC calculation? And if so, is the
> speed gain worth the complexity?

Unless CRCs will always be on -- implying a significant performance
penalty on the kind of platform which *most* needs the MMC-over-SPI
support! -- ISTR that it's not optional; GO_IDLE always needs a CRC,
even if other requests don't use one.  That's why the specs list that
CRC explicitly.
 

> > +/* The unit for these timeouts is milliseconds.  See mmc_spi_scanbyte.  */
> > +#define MINI_TIMEOUT		1
> > +#define READ_TIMEOUT		100
> > +#define WRITE_TIMEOUT		250
> > +
> 
> Shouldn't these be set in the request by the core?

Not the MINI timeout, for sure.  Maybe some of the others.
(MINI should actually be 1..8 bytes, not N msec.)

The whole issue of timeouts needs investiation still ...
like, does the core even understand them right (for SPI).

 
> > +
> > +	/* Specs say to write ones most of the time, even when the card
> > +	 * has no need to read its input data; and many cards won't care.
> > +	 * This is our source of those ones.
> > +	 */
> > +	void			*ones;
> 
> If this is just read, can't it be a global const?

What do you mean by that?  It can't be "static", since such memory
is not generally DMA-safe.  Maybe the pointer could be static; but
each device would need its own DMA mapping; and there will usually
be more than one of these per system, so I can't see much need to
share that buffer between host/card instances.

 
> > +
> > +	if (cmd->opcode == MMC_STOP_TRANSMISSION) {
> > +		/*
> > +		 * We can't tell whether we read block data or the
> > +		 * command reply, so to cope with trash data during
> > +		 * the latency, we just read in 14 bytes (8 would be
> > +		 * enough according to the MMC spec; SD doesn't say)
> > +		 * after the command and fake a clean reply.  We could
> > +		 * avoid this if we saved what the card sent us while
> > +		 * we sent the command, and treat it like a normal
> > +		 * response if we didn't get a SPI_TOKEN_SINGLE.
> > +		 */
> > +		(void) mmc_spi_readbytes(host, host->command.buf,
> > +				sizeof host->command.buf);
> > +		(void) mmc_spi_readbytes(host, host->command.buf,
> > +				sizeof host->command.buf);
> > +		value = 0;
> 
> Didn't you tweak the block driver to not send MMC_STOP_TRANSMISSION?

Only for writes.  Reads still use this.  (Writes use a lowlevel
token replacing the normal next-block token.)

 
> > +
> > +	if (!host->app_cmd
> > +			&& cmd->error == MMC_ERR_NONE
> > +			&& cmd->opcode == MMC_APP_CMD) {
> > +		host->app_cmd = 1;
> > +		cmd->resp[0] |= R1_APP_CMD;
> > +	}
> 
> Why does the host need to keep track of app command state?

As noted in a previous reply (which you've not yet seen):
  (a) Because the MMC core relies on it to do so, and
  (b) Because otherwise diagnostics report CMDx instead of ACMDx.

You didn't comment on the mapping applied to cmd->resp[0] ...

 
> > +
> > +	/*
> > +	 * If this was part of a successful request with a stop-part,
> > +	 * our caller signals the request as done.
> > +	 */
> > +	if (status == 0 && mrq->stop == NULL)
> > +		mmc_request_done(host->mmc, mrq);
> > +	return status;
> > +}
> 
> I assume the comment should have read "without".

No; this handles the request_done() for success without
a stop part.  Caller does it otherwise...


> > +
> > +static inline int resp2status(u8 write_resp)
> > +{
> > +	switch (SPI_MMC_RESPONSE_CODE(write_resp)) {
> > +	case SPI_RESPONSE_ACCEPTED:
> > +		return 0;
> > +	case SPI_RESPONSE_CRC_ERR:
> > +		/* host shall then issue MMC_STOP_TRANSMISSION */
> > +		return -ECRC;
> 
> What's with the made-up errno?

Readability.

 
> We should check what other parts of the kernel uses for CRC errors.

If you can find some convention for CRC errors, this can change;
otherwise that won't matter, since only *this* code cares.

The MMC layer seems to only use the MMC-internal status codes.
(Which is a different issue ... that's generaly frowned upon.)

 
> > +
> > +	if (data->flags & MMC_DATA_READ) {
> > +		direction = DMA_FROM_DEVICE;
> > +		multiple = (cmd->opcode == MMC_READ_MULTIPLE_BLOCK);
> > +
> 
> multiple = (cmd->data->blocks > 1)

Can that now be relied upon?  The reason it was coded this way
(and likewise for writes) was that previously that was NOT true.

I recall trying that at some point, and watching lots of
stuff break...

 
> > +		/* allow pio too, with kmap handling any highmem */
> > +		kmap_addr = kmap(sg->page);
> > +		if (direction == DMA_TO_DEVICE)
> > +			t->tx_buf = kmap_addr + sg->offset;
> > +		else
> > +			t->rx_buf = kmap_addr + sg->offset;
> > +
> 
> If you want highmem, you also need to be prepared for clustered sg entries. kmap
> only maps a single page, and a clustered sg might be larger than that.

Actually it'd be best if we never saw highmem.  Fortunately that
conceptual abortion doesn't is rare (to the point of nonexistence)
on platforms which use this.  Maybe the driver should even depend
on !HIGHMEM, just to be sure.


> > +
> > +	/* NOTE if caller issues SET_BLOCK_COUNT before a multiblock write,
> > +	 * this STOP_TRAN logic isn't needed except when we stop early for
> > +	 * errors.  Currently, mmc_block doesn't issue that request.
> > +	 */
> 
> But will it hurt? Other commands might behave like that.

I think it's unspecified whether it would hurt.  STOP_TRAN is
however specified ONLY for those specific protocol messages.

I suspect you need to rethink how you're viewing this host
driver a bit ... it implements low level protocols, and as
such it can't avoid understanding relevant facts.  Offloading
high level protocol stuff to the core is fine.  By analogy,
link level drivers know about link level issues (STOP_TRAN,
PPPOE, etc), but not about the packets they encapsulate
(CMDx, ARP, etc).

 
> > +
> > +	if (mrq->data && (mrq->data->blksz > MMC_SPI_BLOCKSIZE)) {
> > +		mrq->cmd->error = MMC_ERR_INVALID;
> > +		return -EINVAL;
> > +	}
> > +
> 
> If you've set your parameters correct, then this shouldn't be possible. So do a
> BUG_ON() if you want to have a test.

Which parameters?

The standard kernel policy is to avoid BUG_ON like the plague
if there's any way to report an error to the upper level code
and have it proceed.  If the system can proceed, it's not any
kind of BUG().


> > +
> > +/*
> > + * RESET is started when cmd->opcode == MMC_GO_IDLE_STATE.  This can't
> > + * be just a standard protocol operation.
> > + *
> > + * We expect the MMC core to be responsible for "very hard" resets like
> > + * power cycling the card; there's no accessible reset signal.
> > + */
> 
> Why can't you do this on POWER_ON? I do not like it looking at opcodes, even if
> this one isn't likely to change.

Something to think about.  This is pretty much the way Jan's
patch delievered it.  I found that "74 clocks" stuff in the
powerup sequence, which is perhaps what you did.  That'd
mean the set_ios() stuff *really* needs to be refactored.
I don't really trust the reset logic anyway...

Jan, any comments?  You were the last person to really look
at this part.

 
> > +	/*
> > +	 * Do a burst with chipselect deactivated.  We need to do this
> > +	 * to meet the requirement of 74 clock cycles with chipselect
> > +	 * high before CMD0.  (Section 6.4.1, in "Simplified Physical
> > +	 * Layer Specification 2.0".)  Some cards are particularly
> > +	 * needy of this (e.g. Viking "SD256") while most others don't
> > +	 * seem to care.  Note that it's not enough to deactivate
> > +	 * chipselect without toggling the clock.  Beware of the hack:
> > +	 * we "know" that mmc_spi_readbytes uses the host->status
> > +	 * spi_transfer.
> > +	 *
> > +	 * Note that this is one of two places MMC/SD plays games with
> > +	 * the SPI protocol.  The other is that when chipselect is
> > +	 * released while the card returns BUSY status, the clock must
> > +	 * issue several cycles with chipselect high before the card
> > +	 * will stop driving its output.
> > +	 */
> 
> Sounds like this signaling can be done with the chip select ios already present
> in the mmc layer. Then we won't need voodoo in the host driver.

No.


> > +
> > +	/* issue software reset */
> > +	cmd->arg = 0;
> > +	status = mmc_spi_command_send(host, mrq, CRC_GO_IDLE_STATE, cmd, 0);
> > +	if (status < 0) {
> > +		/* Maybe:
> > +		 *  - there's no card present
> > +		 *  - the card isn't seated correctly (bad contacts)
> > +		 *  - it didn't leave MMC/SD mode
> > +		 *  - there's other confusion in the card state
> > +		 *
> > +		 * Power cycling the card ought to help a lot.
> > +		 * At any rate, let's try again.
> > +		 */
> > +		status = mmc_spi_command_send(host, mrq,
> > +				CRC_GO_IDLE_STATE, cmd, 0);
> > +		if (status < 0)
> > +			dev_dbg(&host->spi->dev,
> > +				"can't initialize; no card%s?\n",
> > +				could_invert_cs
> > +					? ""
> > +					: " or chip-select error");
> > +	}
> 
> No, no, no... This is not the kind of behaviour you want to hide down in the
> host driver. This is a decision the core should make. Don't put the layer cake
> in the blender.

Good point.  I think that came from frustration with some
cases of the "we can't really power cycle on this board"
logic not behaving.  I'll just return the first command_send().


> > +
> > +	/* MMC core and layered drivers *MUST* issue SPI-aware commands */
> > +	cmd = mrq->stop;
> > +	if (cmd && !mmc_spi_resp_type(cmd)) {
> > +		dev_dbg(&host->spi->dev, "bogus STOP command\n");
> > +		dump_stack();
> > +		cmd->error = MMC_ERR_INVALID;
> > +		goto fail;
> > +	}
> > +
> 
> This seems like something that should just be in debug builds.

Just this sanity check, not both?


> > +
> > +static int mmc_spi_get_ro(struct mmc_host *mmc)
> > +{
> > +	struct mmc_spi_host *host = mmc_priv(mmc);
> > +
> > +	if (host->pdata && host->pdata->get_ro)
> > +		return host->pdata->get_ro(mmc->parent);
> > +	/* board doesn't support read only detection; assume writeable */
> > +	return 0;
> > +}
> > +
> 
> A printk() might be nice so the user knows why the switch isn't being respected.

ISTR that not many of the other drivers do that, at least not there.
I can update the startup message to say if it has power on/off support.


> > +
> > +	/* SanDisk and Hitachi MMC docs both show clock timing diagrams
> > +	 * with clock starting low (CPOL=0) and sampling on leading edge
> > +	 * (CPHA=0); clock is measured between rising edges.  Sandisk SD
> > +	 * docs show clock starting high (CPOL=1) and sampling on trailing
> > +	 * edge (CPHA=1), measuring between falling edges.
> > +	 *
> > +	 * Docs are very explicit that sampling is on the rising edge, so
> > +	 * the difference between SPI_MODE_0 and SPI_MODE_3 may not matter.
> > +	 */
> 
> Sure you haven't looked at high-speed versus legacy clocking?

Nyet.


> > +
> > +	if (spi->master->cdev.dev->dma_mask) {
> > +		struct device	*dev = spi->master->cdev.dev;
> > +
> > +		host->dma_dev = dev;
> > +		host->dma = dma_map_single(dev, host,
> > +				sizeof *host, DMA_BIDIRECTIONAL);
> > +		host->ones_dma = dma_map_single(dev, ones,
> > +				MMC_SPI_BLOCKSIZE, DMA_TO_DEVICE);
> > +		host->data_dma = dma_map_single(dev, host->data,
> > +				sizeof *host->data, DMA_BIDIRECTIONAL);
> > +
> 
> This might be wasteful if the system has an iommu.

Systems without a "native" MMC/SD controller are unlikely to spend
silicon to put an IOMMU on the SPI controller.  I've seen them
on a few other controllers ... but that seems to have been a short
lived experiment, phased out in newer chips.

And in any case, that's like one page plus one cacheline.  If there
were an IOMMU, surely it can afford that much overhead.

- Dave




-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [patch 4/4] MMC-over-SPI host driver

[Pierre Ossman]

David Brownell wrote:
> 
> There were other cases, as I recall.  Maybe the core has changed
> so much that some of them are no longer needed.  I removed as
> many as seemed safe.
> 

Which are left? I doubt they are needed.

> 
> Unless CRCs will always be on -- implying a significant performance
> penalty on the kind of platform which *most* needs the MMC-over-SPI
> support! -- ISTR that it's not optional; GO_IDLE always needs a CRC,
> even if other requests don't use one.  That's why the specs list that
> CRC explicitly.
>  

Since it depends unconditionally on CRC7, you will always be able to calculate
that in runtime.

And in order for the host to know when to calculate it, I suggest another feel
to the ios structure. And drop the CAP_CRC, we'll consider CRC support mandatory
(even though you can have the option of turning it off in runtime).

> 
> Not the MINI timeout, for sure.  Maybe some of the others.
> (MINI should actually be 1..8 bytes, not N msec.)
> 
> The whole issue of timeouts needs investiation still ...
> like, does the core even understand them right (for SPI).
> 

Probably not. So add it. ;)

> 
> What do you mean by that?  It can't be "static", since such memory
> is not generally DMA-safe.  Maybe the pointer could be static; but
> each device would need its own DMA mapping; and there will usually
> be more than one of these per system, so I can't see much need to
> share that buffer between host/card instances.
> 

I didn't check how much data was in that thing, but it feels wrong not to share
memory when we know it's read-only.

>> Didn't you tweak the block driver to not send MMC_STOP_TRANSMISSION?
> 
> Only for writes.  Reads still use this.  (Writes use a lowlevel
> token replacing the normal next-block token.)
> 

You should trigger on it being data->stop, not on the opcode. Again, for forward
compatibility.

> 
> As noted in a previous reply (which you've not yet seen):
>   (a) Because the MMC core relies on it to do so, and
>   (b) Because otherwise diagnostics report CMDx instead of ACMDx.
> 

(a) should go away and (b) can be moved to the core.

> You didn't comment on the mapping applied to cmd->resp[0] ...

I thought you were going to remove all of them, but it seems I misunderstood.
The host driver shouldn't try to fake being a native controller as that will
come back to bite us in the ass eventually when we find some odd corner case.
Report the true SPI stuff up to the core and modify the core to understand it.

>>> +
>>> +	/*
>>> +	 * If this was part of a successful request with a stop-part,
>>> +	 * our caller signals the request as done.
>>> +	 */
>>> +	if (status == 0 && mrq->stop == NULL)
>>> +		mmc_request_done(host->mmc, mrq);
>>> +	return status;
>>> +}
>> I assume the comment should have read "without".
> 
> No; this handles the request_done() for success without
> a stop part.  Caller does it otherwise...
> 

Ah, so the comment doesn't describe why this is done, rather it describes why
mmc_request_done() isn't always called here?

Perhaps a rewording is in order as it has already confused at least one person. :)

> 
>>> +
>>> +static inline int resp2status(u8 write_resp)
>>> +{
>>> +	switch (SPI_MMC_RESPONSE_CODE(write_resp)) {
>>> +	case SPI_RESPONSE_ACCEPTED:
>>> +		return 0;
>>> +	case SPI_RESPONSE_CRC_ERR:
>>> +		/* host shall then issue MMC_STOP_TRANSMISSION */
>>> +		return -ECRC;
>> What's with the made-up errno?
> 
> Readability.
> 

Not sure it helps in that area. Many will get confused by an errno they do not
recognize.

>  
>> We should check what other parts of the kernel uses for CRC errors.
> 
> If you can find some convention for CRC errors, this can change;
> otherwise that won't matter, since only *this* code cares.
> 

I'm a neat freak. ;)

Won't EILSEQ do?

> The MMC layer seems to only use the MMC-internal status codes.
> (Which is a different issue ... that's generaly frowned upon.)
> 

I know, it's part of the baggage I got when I took over. It's on my todo list to
get rid of those codes. They've been annoying the hell out of me.

>> multiple = (cmd->data->blocks > 1)
> 
> Can that now be relied upon?  The reason it was coded this way
> (and likewise for writes) was that previously that was NOT true.
> 
> I recall trying that at some point, and watching lots of
> stuff break...
> 

I don't know your exact meaning of "multiple" here, but if it means "more than
one block", then checking the block count is the correct way.

> 
> Actually it'd be best if we never saw highmem.  Fortunately that
> conceptual abortion doesn't is rare (to the point of nonexistence)
> on platforms which use this.  Maybe the driver should even depend
> on !HIGHMEM, just to be sure.
> 

You can tell the MMC layer that by setting your DMA mask properly.

> 
> I think it's unspecified whether it would hurt.  STOP_TRAN is
> however specified ONLY for those specific protocol messages.
> 

I don't like sticking that information in here. We should code this into the
request somewhere and let the core determine which opcodes needs it.

> I suspect you need to rethink how you're viewing this host
> driver a bit ... it implements low level protocols, and as
> such it can't avoid understanding relevant facts.  Offloading
> high level protocol stuff to the core is fine.  By analogy,
> link level drivers know about link level issues (STOP_TRAN,
> PPPOE, etc), but not about the packets they encapsulate
> (CMDx, ARP, etc).
> 

I'd say your driver is more like the PHY (with some acceleration like computing
CRC), and the core contains the MAC.

>> If you've set your parameters correct, then this shouldn't be possible. So do a
>> BUG_ON() if you want to have a test.
> 
> Which parameters?

max_blk_size and friends in the host structure.

> 
> The standard kernel policy is to avoid BUG_ON like the plague
> if there's any way to report an error to the upper level code
> and have it proceed.  If the system can proceed, it's not any
> kind of BUG().
> 

WARN_ON() or something then. I'm not saying you should remove your if-clause
(not saying you should keep it either); but the fact is that this is an internal
kernel interface, so giving the wrong parameters is a bug that should be exposed.

>> Sounds like this signaling can be done with the chip select ios already present
>> in the mmc layer. Then we won't need voodoo in the host driver.
> 
> No.
> 

Why not? It would allow us to keep higher protocol semantics (which varying
delays depending on opcodes is after all) in the core.

>> This seems like something that should just be in debug builds.
> 
> Just this sanity check, not both?
> 

Both.

>> A printk() might be nice so the user knows why the switch isn't being respected.
> 
> ISTR that not many of the other drivers do that, at least not there.
> I can update the startup message to say if it has power on/off support.
> 

Just because the other boys are misbehaving doesn't mean you have to. ;)

> 
> Systems without a "native" MMC/SD controller are unlikely to spend
> silicon to put an IOMMU on the SPI controller.  I've seen them
> on a few other controllers ... but that seems to have been a short
> lived experiment, phased out in newer chips.
> 
> And in any case, that's like one page plus one cacheline.  If there
> were an IOMMU, surely it can afford that much overhead.
> 

Not if everyone has that attitude. ;)

It's not an issue I'm going to press, but I though I should point it out at least.

Rgds
-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [patch 4/4] MMC-over-SPI host driver

[David Brownell]

On Friday 15 June 2007, Pierre Ossman wrote:
> David Brownell wrote:
> > 
> > There were other cases, as I recall.  Maybe the core has changed
> > so much that some of them are no longer needed.  I removed as
> > many as seemed safe.
> 
> Which are left? I doubt they are needed.

If the MMC core and block code change more, many can go.  Remember
that I was making such core changes in small, safe doses!

Right now it looks like the main reason to keep including mmc.h is
to handle one aspect of mapping the data error token.  Four of its
bits are part of normal R2 response, suitable for shift-and-mask
handling, but the 0x10 bit seems to be vendor specific ... so for
now, if that's set I map it to R2_SPI_ERROR (generic "it broke")
and use that symbol to avoid excess magic numbers.


> 	 I suggest another feel
> to the ios structure. And drop the CAP_CRC, we'll consider CRC support mandatory
> (even though you can have the option of turning it off in runtime).

That's not a complete description.  :)
You're thinking of something like this?

 - always do the (dirt cheap) crc7 in the command;
 - move the module parameter into mmc core, default spi_crc=y;
 - add mmc_ios.spi_crc, initialized to match the module parameter;
 - mmc_spi_set_crc() uses that parameter; and
 - mmc_spi host will use that instead of its current use_crc, to
   tell whether it should init or verify datablock CRC16 values.

Except this doesn't seem like an ios-style thing to me... that's
on top of me disliking that confusing and error-prone "check N
parameters to see which ones should change this time" API style.

Having a simple flag in mmc_host would be much better.


> >>> +		return -ECRC;
> >> What's with the made-up errno?
> > 
> > Readability.
> > 
> 
> Not sure it helps in that area. Many will get confused by an errno they do not
> recognize.

I'll leave a #define pending a better solution though.
Maybe MMC_ERRNO_BADCRC not ECRC?


> >> We should check what other parts of the kernel uses for CRC errors.
> > 
> > If you can find some convention for CRC errors, this can change;
> > otherwise that won't matter, since only *this* code cares.
> > 
> 
> I'm a neat freak. ;)
> 
> Won't EILSEQ do?

After changing the existing usage of that code, and assuming
that code becomes reserved for that role within the MMC stack...


> > Actually it'd be best if we never saw highmem.  Fortunately that
> > conceptual abortion doesn't is rare (to the point of nonexistence)
> > on platforms which use this.  Maybe the driver should even depend
> > on !HIGHMEM, just to be sure.
> > 
> 
> You can tell the MMC layer that by setting your DMA mask properly.

That's at best an implementation quirk; those masks don't say
a thing about how kernel virtual memory space is managed (which
is why code needs to care about HIGHMEM).  For now I'll just
say the driver depends on !HIGHMEM.


> > I think it's unspecified whether it would hurt.  STOP_TRAN is
> > however specified ONLY for those specific protocol messages.
> 
> I don't like sticking that information in here. We should code this into the
> request somewhere and let the core determine which opcodes needs it.

It *IS* already encoded in the request:  only multiblock writes
need to use that token.  Likewise they use special start tokens
for each block.  And in the same way, other data blocks use a
third kind of start token; and may have data-dependent CRC values.
All such values are managed below the core.

This is just something that a person adding SET_BLOCK_COUNT
support (CMD23, for MMC only) would need to worry about.  And
that command seems to be "reserved" in more documents than not,
so it's unlikely to ever matter.  I updated that comment.


> >> If you've set your parameters correct, then this shouldn't be possible. So do a
> >> BUG_ON() if you want to have a test.
> > 
> > Which parameters?
> 
> max_blk_size and friends in the host structure.

OK, I'll just set that and get rid of the test.  I didn't much
like that test, but I did want to make sure that the limit was
not broken.

 
> >> Sounds like this signaling can be done with the chip select ios already present
> >> in the mmc layer. Then we won't need voodoo in the host driver.
> > 
> > No.
> > 
> 
> Why not? It would allow us to keep higher protocol semantics (which varying
> delays depending on opcodes is after all) in the core.

Well for starters, it's packaged in the mmc_ios which holds things that
don't change between requests, but SPI chipselect always goes active at
the beginning of every request and then goes inactive when it's done.

It seems the only reason to have it in mmc_ios is to help work around
some wbsd quirks during card enumeration.  Other controllers manage to
keep that signal pulled up until entering 4-wire mode.

- Dave


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [patch 4/4] MMC-over-SPI host driver

[Pierre Ossman]

David Brownell wrote:
> 
> If the MMC core and block code change more, many can go.  Remember
> that I was making such core changes in small, safe doses!
> 
> Right now it looks like the main reason to keep including mmc.h is
> to handle one aspect of mapping the data error token.  Four of its
> bits are part of normal R2 response, suitable for shift-and-mask
> handling, but the 0x10 bit seems to be vendor specific ... so for
> now, if that's set I map it to R2_SPI_ERROR (generic "it broke")
> and use that symbol to avoid excess magic numbers.
> 

Don't map at all. Just give the core what you got from the card.

> 
>> 	 I suggest another feel
>> to the ios structure. And drop the CAP_CRC, we'll consider CRC support mandatory
>> (even though you can have the option of turning it off in runtime).
> 
> That's not a complete description.  :)
> You're thinking of something like this?
> 
>  - always do the (dirt cheap) crc7 in the command;
>  - move the module parameter into mmc core, default spi_crc=y;
>  - add mmc_ios.spi_crc, initialized to match the module parameter;
>  - mmc_spi_set_crc() uses that parameter; and
>  - mmc_spi host will use that instead of its current use_crc, to
>    tell whether it should init or verify datablock CRC16 values.
> 

Yes. It more clearly separates mechanism (the host driver) from policy (the mmc
core).

> Except this doesn't seem like an ios-style thing to me... that's
> on top of me disliking that confusing and error-prone "check N
> parameters to see which ones should change this time" API style.
> 

I'm no fan of this ios thing either, but it's what we've got and it hasn't been
worth the effort to rewrite it yet.

> Having a simple flag in mmc_host would be much better.

Perhaps. But there's a lot of value in consistency.

> 
> I'll leave a #define pending a better solution though.
> Maybe MMC_ERRNO_BADCRC not ECRC?
> 

Well, since we're scrapping those custom error codes, then that's probably not a
good idea. We can hold off on this issue until that mess is sorted out.

> 
> After changing the existing usage of that code, and assuming
> that code becomes reserved for that role within the MMC stack...
> 

Overloading use of errno:s is rather common. Just look at ETIMEDOUT.

> 
> It *IS* already encoded in the request:  only multiblock writes
> need to use that token.  Likewise they use special start tokens
> for each block.  And in the same way, other data blocks use a
> third kind of start token; and may have data-dependent CRC values.
> All such values are managed below the core.
> 
> This is just something that a person adding SET_BLOCK_COUNT
> support (CMD23, for MMC only) would need to worry about.  And
> that command seems to be "reserved" in more documents than not,
> so it's unlikely to ever matter.  I updated that comment.
> 

Well, that person shouldn't have to worry about it as the host driver should
service all valid requests.

>>>> Sounds like this signaling can be done with the chip select ios already present
>>>> in the mmc layer. Then we won't need voodoo in the host driver.
>>> No.
>>>
>> Why not? It would allow us to keep higher protocol semantics (which varying
>> delays depending on opcodes is after all) in the core.
> 
> Well for starters, it's packaged in the mmc_ios which holds things that
> don't change between requests, but SPI chipselect always goes active at
> the beginning of every request and then goes inactive when it's done.
> 

Who said mmc_ios doesn't change between requests? And as your code states, it
isn't as simple as keeping it on during just the request.

> It seems the only reason to have it in mmc_ios is to help work around
> some wbsd quirks during card enumeration.  Other controllers manage to
> keep that signal pulled up until entering 4-wire mode.
> 

We try to think a bit more general than that, even though it was first
implemented because the wbsd driver needed it.

Rgds
-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [patch 4/4] MMC-over-SPI host driver

[David Brownell]

> > Right now it looks like the main reason to keep including mmc.h is
> > to handle one aspect of mapping the data error token. ...
>
> Don't map at all. Just give the core what you got from the card.

Tried that, it fails for the obvious (in retrospect) reason:  after each
command that's issued, something needs to ensure that the return code won't
be MMC_ERR_NONE after errors.  That means looking at those R1 bits which
report that hardware status.

And it's got to do that pretty much immediately, since when an error shows
up in the command, the data stage must not be performed.  It's impractical
to consider delegating such triage to the core.

The data token was a bit of a red herring, and after some re-factoring it
turned out that there's no current win to reporting those slightly more
detailed data stage errors.  (Or any obvious place to report them...)


> > You're thinking of something like this?
> > 
> >  - always do the (dirt cheap) crc7 in the command;
> >  - move the module parameter into mmc core, default spi_crc=y;
> >  - add mmc_ios.spi_crc, initialized to match the module parameter;
> >  - mmc_spi_set_crc() uses that parameter; and
> >  - mmc_spi host will use that instead of its current use_crc, to
> >    tell whether it should init or verify datablock CRC16 values.
> > 
>
> Yes. It more clearly separates mechanism (the host driver) from policy (the mmc
> core).

OK, I'll look down that route.


> > Except this doesn't seem like an ios-style thing to me... that's
> > on top of me disliking that confusing and error-prone "check N
> > parameters to see which ones should change this time" API style.
>
> I'm no fan of this ios thing either, but it's what we've got and it hasn't been
> worth the effort to rewrite it yet.
>
> > Having a simple flag in mmc_host would be much better.
>
> Perhaps. But there's a lot of value in consistency.

Unless you strongly object, I'll go the flag route though.  The thing is,
the IOS thing ("kitchen sink") will never vanish if it keeps growning at
every opportunity.

- Dave


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [patch 4/4] MMC-over-SPI host driver

[Pierre Ossman]

David Brownell wrote:
>>> Right now it looks like the main reason to keep including mmc.h is
>>> to handle one aspect of mapping the data error token. ...
>> Don't map at all. Just give the core what you got from the card.
> 
> Tried that, it fails for the obvious (in retrospect) reason:  after each
> command that's issued, something needs to ensure that the return code won't
> be MMC_ERR_NONE after errors.  That means looking at those R1 bits which
> report that hardware status.

The error codes were never supposed to report card status, only controller
status. So MMC_ERR_NONE is perfectly valid for a status response filled with
error bits.

> And it's got to do that pretty much immediately, since when an error shows
> up in the command, the data stage must not be performed.  It's impractical
> to consider delegating such triage to the core.
> 

This is an architectural limitation in the MMC layer that affects the native
protocol aswell. If you need it fixed, I'd say we have to do it in a proper,
general manner.

Why can't you do what native controllers do, just fling the data out there and
break when the card doesn't respond properly to it?

>>> Having a simple flag in mmc_host would be much better.
>> Perhaps. But there's a lot of value in consistency.
> 
> Unless you strongly object, I'll go the flag route though.  The thing is,
> the IOS thing ("kitchen sink") will never vanish if it keeps growning at
> every opportunity.
> 

But if we keep inventing new systems ad-hoc, we'll have a much bigger mess to
deal with.

Rgds
-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [patch 4/4] MMC-over-SPI host driver

[David Brownell]

On Wednesday 20 June 2007, Pierre Ossman wrote:
> David Brownell wrote:
> >>> Right now it looks like the main reason to keep including mmc.h is
> >>> to handle one aspect of mapping the data error token. ...
> >> Don't map at all. Just give the core what you got from the card.
> > 
> > Tried that, it fails for the obvious (in retrospect) reason:  after each
> > command that's issued, something needs to ensure that the return code won't
> > be MMC_ERR_NONE after errors.  That means looking at those R1 bits which
> > report that hardware status.
> 
> The error codes were never supposed to report card status, only controller
> status. So MMC_ERR_NONE is perfectly valid for a status response filled with
> error bits.

If so, then the MMC core is full of bugs, needing fixes
that are *FAR* beyond scope of my patches.

Essentially every error check compares against MMC_ERR_NONE.

This distinction you're making between "controller" and
"card" seems unnatural -- and counterproductive -- in
this context.  What the core code expects to see is fault
reports.  It doesn't actually understand layering to any
relevant degree; all it cares about is whether the operation
it requested has succeeded or not.


> > And it's got to do that pretty much immediately, since when an error shows
> > up in the command, the data stage must not be performed.  It's impractical
> > to consider delegating such triage to the core.
> > 
> 
> This is an architectural limitation in the MMC layer that affects the native
> protocol aswell. If you need it fixed, I'd say we have to do it in a proper,
> general manner.

So, I wait for patches from you which fix all those issues?


> Why can't you do what native controllers do, just fling the data out there and
> break when the card doesn't respond properly to it?

Because that would be a remarkably ... naive? ... way to
implement *any* protocol stack.  If I were grading college
students again, any such implementation would be seriously
downgraded.  It's a virtual certainty that throwing garbage
commands (like that "data") will make things break.  At
best, it enters a recoverable fault mode, only wasting the
time used to recover from having thrown known-to-be garbage
bytes at the card.  At worst ... devices become unrecoverable
with weaker provocations than that.

I sure hope you have heard that old chestnut, "Be liberal
in what you accept, and conservative in what you send."

That's an ancient principle for robust interoperation.  The
first RFC describing it was RFC 760, but the principle
predates that 1980 document by at least a few years.  The
way to apply that principle here is obvious:  don't throw
known garbage commands at a card.


> >>> Having a simple flag in mmc_host would be much better.
> >> Perhaps. But there's a lot of value in consistency.
> > 
> > Unless you strongly object, I'll go the flag route though.  The thing is,
> > the IOS thing ("kitchen sink") will never vanish if it keeps growning at
> > every opportunity.
> > 
> 
> But if we keep inventing new systems ad-hoc, we'll have a much bigger mess to
> deal with.

State flags predate the notion of this "ios" thingie;
they're far from being an "ad-hoc" notion.

- Dave

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [patch 4/4] MMC-over-SPI host driver

[Pierre Ossman]

Sorry for being unresponsive to your mails. I appreciate the hard work
you're doing, but I have too many other things on my table to answer
you in a timely manner. I hope you can put up with the delays.

On Fri, 22 Jun 2007 14:18:43 -0700
David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:

> 
> If so, then the MMC core is full of bugs, needing fixes
> that are *FAR* beyond scope of my patches.
> 
> Essentially every error check compares against MMC_ERR_NONE.
> 
> This distinction you're making between "controller" and
> "card" seems unnatural -- and counterproductive -- in
> this context.  What the core code expects to see is fault
> reports.  It doesn't actually understand layering to any
> relevant degree; all it cares about is whether the operation
> it requested has succeeded or not.
> 

Indeed. But the status bits in the responses are context dependent and
the same for all controllers. I fully agree that we pay too little
attention to them as it is. But fixing this should be done in the core,
not in each and every host driver.

> 
> > > And it's got to do that pretty much immediately, since when an
> > > error shows up in the command, the data stage must not be
> > > performed.  It's impractical to consider delegating such triage
> > > to the core.
> > > 
> > 
> > This is an architectural limitation in the MMC layer that affects
> > the native protocol aswell. If you need it fixed, I'd say we have
> > to do it in a proper, general manner.
> 
> So, I wait for patches from you which fix all those issues?
> 

This is not a problem in practice, so it is not something I will
prioritize.

The big problem fixing this is that it is very difficult,
if not impossible, to be able to apply some policy between the command
response and the data. Most hardware will simply trod along and start
shuffling data, no matter the response

> 
> Because that would be a remarkably ... naive? ... way to
> implement *any* protocol stack.  If I were grading college
> students again, any such implementation would be seriously
> downgraded.  It's a virtual certainty that throwing garbage
> commands (like that "data") will make things break.  At
> best, it enters a recoverable fault mode, only wasting the
> time used to recover from having thrown known-to-be garbage
> bytes at the card.  At worst ... devices become unrecoverable
> with weaker provocations than that.
> 

The system is crap from a theoretical point of view. But the system
differentiates between commands and data well enough for this to not be
a problem in practice. Solving this is complex and will be a no or
little gain.

> 
> State flags predate the notion of this "ios" thingie;
> they're far from being an "ad-hoc" notion.
> 

In other systems, yes. But it is inconsistent when it comes to the mmc
code.

Rgds
-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [patch 4/4] MMC-over-SPI host driver

[David Brownell]

On Wednesday 20 June 2007, Pierre Ossman wrote:
> David Brownell wrote:
> >>> Right now it looks like the main reason to keep including mmc.h is
> >>> to handle one aspect of mapping the data error token. ...
> >> Don't map at all. Just give the core what you got from the card.
> > 
> > Tried that, it fails for the obvious (in retrospect) reason:  after each
> > command that's issued, something needs to ensure that the return code won't
> > be MMC_ERR_NONE after errors.  That means looking at those R1 bits which
> > report that hardware status.
> 
> The error codes were never supposed to report card status, only controller
> status. So MMC_ERR_NONE is perfectly valid for a status response filled with
> error bits.

As I said earlier:  not with the current codebase, which disagrees
with your "only controller status" notion.  The original authors of
that code clearly had a "single level fault reporting" model in mind;
after all, that's pretty much standard practice everywhere.

I've worked with "multi level fault reporting" frameworks, and they
have uniformly been a pain in the posterior unless they can be used
in a "single level" mode.  Being able to drill down to lower level
details is an OK thing, although it's rarely used for anything except
debugging.  Being *forced* to do so is evil, especially when the
structure of those lower layers can be complex and nonuniform.

In this case, for example, you're assuming there *is* a "controller".
There might not be one; SPI (and for that matter the native protocol)
can just be bitbanged.  If there's hardware acceleration for various
protocol messages, that's fine but irrelevant to whether or not the
sequencing requirements of the protocol were followed.


> > And it's got to do that pretty much immediately, since when an error shows
> > up in the command, the data stage must not be performed.  It's impractical
> > to consider delegating such triage to the core.
> > 
> 
> This is an architectural limitation in the MMC layer that affects the native
> protocol aswell. If you need it fixed, I'd say we have to do it in a proper,
> general manner.

Disagree.  See above:  single layer fault reporting models are the
norm ... for very good reasons.  And even in those simpler models,
very little code ever cares about fault details when it comes to
recovery strategies.


> Why can't you do what native controllers do, just fling the data out there and
> break when the card doesn't respond properly to it?

One reason is that the "native" controllers split command and data
streams into different physical channels.  Cards can't ever mistake
data for commands ... which is a very real possibility with SPI.

- Dave


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [patch 4/4] MMC-over-SPI host driver

[David Brownell]

On Friday 15 June 2007, Pierre Ossman wrote:
> David Brownell wrote:
> > Not the MINI timeout, for sure.  Maybe some of the others.
> > (MINI should actually be 1..8 bytes, not N msec.)
> > 
> > The whole issue of timeouts needs investiation still ...
> > like, does the core even understand them right (for SPI).
> > 
> 
> Probably not. So add it. ;)

The latest specs say to use fixed timeouts, ignoring CSD values.
So the latest code (which I'll repost when git-mmc merges) got rid of
the bogus MINI timeout, and uses those timeouts unconditionally.
There seems to be no downside to using slightly longer timeouts than
a given older-spec card would tolerate.

- Dave

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/