mmc: add generic mmc spi driver

This patch supports mmc/sd card with spi interface.
I have tested with sd and mmc cards. But there is still ocr issue with SDHC.

Quoted 4 lines · On Thursday 22 April 2010 22:53:34 Thomas Chou wrote:

This patch supports mmc/sd card with spi interface.
I have tested with sd and mmc cards. But there is still ocr issue with SDHC.

now the semi-obvious question ... how does this differ from the old spi_mmc.c driver ? i see this doesnt rely on the legacy mmc framework, but what about device/commandset support parity ?

it's too bad the new framework doesnt allow for dynamic probing like the spi layer. makes it a pain to work with a SPI/MMC card that can have the CS changed on the fly.
-mike

it's too bad the new framework doesnt allow for dynamic probing like the spi

Quoted 5 lines

layer. makes it a pain to work with a SPI/MMC card that can have the CS changed on the fly.
-mike

Quoted 3 lines · Thomas Chou

Quoted 3 lines

it's too bad the new framework doesnt allow for dynamic probing like the spi layer. makes it a pain to work with a SPI/MMC card that can have the CS changed on the fly.

Second thought.
With generic mmc framework, we can instance multiple mmc devices. Then we can probe one of them when we want.

Quoted 10 lines · On Friday 23 April 2010 01:55:11 Thomas Chou wrote:

Quoted 4 lines · Thomas Chou

Quoted 4 lines

it's too bad the new framework doesnt allow for dynamic probing like the spi layer. makes it a pain to work with a SPI/MMC card that can have the CS changed on the fly.

Second thought.
With generic mmc framework, we can instance multiple mmc devices. Then we can probe one of them when we want.

i dont think that's a scalable solution. what if you have multiple spi busses or 10's of GPIO CS's ? only way to scale is to have a new subcommand so people can do it themselves. a simple CONFIG_CMD_MMC_SPI command which forwards along calls to mmc_spi_init() on the fly would work. -mike

I have moved spi slave setup so that we can change the cs on the fly. We could add a subcommand, like,

mmc_spi [bus:]cs

Where should I place this command? in a new file in common dir or inside the mmc_spi driver?

Do you mean "parity" as crc? Crc is not supported, but can be added. I make some command set translation when spi doesn't support.

common/cmd_mmc_spi.c line 22

Quoted 4 lines · On Monday 26 April 2010 23:27:16 Thomas Chou wrote:

common/cmd_mmc_spi.c new file +1 -0

+	struct mmc *mmc = NULL;

useless assignment since mmc gets set shortly after

common/cmd_mmc_spi.c line 33

Quoted 13 lines

common/cmd_mmc_spi.c new file +11 -0

+	struct mmc_spi_priv *priv;
+
+	if (argc<  2)
+		goto usage;
+
+	dev_num = simple_strtoul(argv[1],&endp, 0);
+	if (*endp != 0)
+			goto usage;
+	mmc = find_mmc_device(dev_num);
+	if (!mmc || strcmp(mmc->name, "MMC_SPI"))
+			goto usage;

too many indents on the goto

also, this looks like it bails if no existing mmc spi device is found ? i thought the point of this command was to create new instances on the fly. this looks like it just reprograms the settings for an existing device.

Quoted 5 lines

common/cmd_mmc_spi.c new file +5 -0

+	if (bus != priv->bus || cs != priv->cs ||
+	    speed != priv->speed || mode != priv->speed) {
+		priv->bus = bus;
+		priv->cs = cs;
+		priv->speed = speed;

common/cmd_mmc_spi.c line 75

Quoted 6 lines

common/cmd_mmc_spi.c new file +5 -0

+		priv->mode = mode;
+		if (priv->slave) {
+			free(priv->slave);
+			priv->slave = NULL;
+		}

you shouldnt be poking the spi internals like this. use the common spi funcs to free/alloc the slave device.

This patch supports mmc/sd card with spi interface. It is based on the generic mmc framework. It works with SDHC and supports write.

The crc7 lib func is merged from linux and used to compute mmc command checksum.

There is a subcomamnd "mmc_spi" to setup spi bus and cs at run time.

Signed-off-by Thomas Chou <[email protected]>

(quoted diff) +17 -0

+
+static int do_mmc_spi(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
+{
+ ? ? ? int dev_num = -1;
+ ? ? ? uint bus;
+ ? ? ? uint cs;
+ ? ? ? uint speed;
+ ? ? ? uint mode;
+ ? ? ? char *endp;
+ ? ? ? struct mmc *mmc;
+ ? ? ? struct mmc_spi_priv *priv;
+
+ ? ? ? do {
+ ? ? ? ? ? ? ? mmc = find_mmc_device(++dev_num);
+ ? ? ? } while (mmc && strcmp(mmc->name, "MMC_SPI"));
+ ? ? ? if (!mmc) {
+ ? ? ? ? ? ? ? printf("Create MMC Device\n");

(quoted diff) +46 -0

+
+ ? ? ? priv = mmc->priv;
+ ? ? ? bus = priv->bus;
+ ? ? ? cs = priv->cs;
+ ? ? ? speed = priv->speed;
+ ? ? ? mode = priv->mode;
+
+ ? ? ? if (argc < 2)
+ ? ? ? ? ? ? ? goto info;
+ ? ? ? cs = simple_strtoul(argv[1], &endp, 0);
+ ? ? ? if (*argv[1] == 0 || (*endp != 0 && *endp != ':'))
+ ? ? ? ? ? ? ? goto usage;
+ ? ? ? if (*endp == ':') {
+ ? ? ? ? ? ? ? if (endp[1] == 0)
+ ? ? ? ? ? ? ? ? ? ? ? goto usage;
+ ? ? ? ? ? ? ? bus = cs;
+ ? ? ? ? ? ? ? cs = simple_strtoul(endp + 1, &endp, 0);
+ ? ? ? ? ? ? ? if (*endp != 0)
+ ? ? ? ? ? ? ? ? ? ? ? goto usage;
+ ? ? ? }
+ ? ? ? if (argc >= 3) {
+ ? ? ? ? ? ? ? speed = simple_strtoul(argv[2], &endp, 0);
+ ? ? ? ? ? ? ? if (*argv[2] == 0 || *endp != 0)
+ ? ? ? ? ? ? ? ? ? ? ? goto usage;
+ ? ? ? }
+ ? ? ? if (argc >= 4) {
+ ? ? ? ? ? ? ? mode = simple_strtoul(argv[3], &endp, 16);
+ ? ? ? ? ? ? ? if (*argv[3] == 0 || *endp != 0)
+ ? ? ? ? ? ? ? ? ? ? ? goto usage;
+ ? ? ? }
+ ? ? ? if (!spi_cs_is_valid(bus, cs)) {
+ ? ? ? ? ? ? ? printf("Invalid SPI bus %u cs %u\n", bus, cs);
+ ? ? ? ? ? ? ? return 1;
+ ? ? ? }
+
+ ? ? ? if (bus != priv->bus || cs != priv->cs ||
+ ? ? ? ? ? speed != priv->speed || mode != priv->mode) {
+ ? ? ? ? ? ? ? priv->bus = bus;
+ ? ? ? ? ? ? ? priv->cs = cs;
+ ? ? ? ? ? ? ? priv->speed = speed;
+ ? ? ? ? ? ? ? priv->mode = mode;
+ ? ? ? ? ? ? ? if (priv->slave) {
+ ? ? ? ? ? ? ? ? ? ? ? spi_free_slave(priv->slave);
+ ? ? ? ? ? ? ? ? ? ? ? priv->slave = NULL;
+ ? ? ? ? ? ? ? }
+ ? ? ? }

drivers/mmc/mmc_spi.c new file +2 -0

new file mode 100644
index 0000000..dedcef7
@@ -0,0 +1,318 @@
+/*
+ * generic mmc spi driver

+ ? ? ? cmdo[2] = cmdarg >> 24;
+ ? ? ? cmdo[3] = cmdarg >> 16;
+ ? ? ? cmdo[4] = cmdarg >> 8;
+ ? ? ? cmdo[5] = cmdarg;
+ ? ? ? cmdo[6] = (crc7(0, &cmdo[1], 5) << 1) | 0x01;
+ ? ? ? spi_xfer(priv->slave, 7 * 8, cmdo, NULL, SPI_XFER_BEGIN);
+ ? ? ? for (i = 0; i < CTOUT; i++) {
+ ? ? ? ? ? ? ? spi_xfer(priv->slave, 1 * 8, NULL, &r1, 0);
+ ? ? ? ? ? ? ? if ((r1 & 0x80) == 0)

+ ? ? ? ? ? ? ? ? ? ? ? break;
+ ? ? ? }
+ ? ? ? debug("%s:cmd%d resp%d %x\n", __func__, cmdidx, i, r1);
+ ? ? ? return r1;

drivers/mmc/mmc_spi.c new file +15 -0

+}
+
+static uint mmc_spi_readdata(struct mmc *mmc, char *buf,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? u32 bcnt, u32 bsize)
+{
+ ? ? ? struct mmc_spi_priv *priv = mmc->priv;
+ ? ? ? u8 r1;
+ ? ? ? u8 crc[2];
+ ? ? ? int i;
+ ? ? ? while (bcnt--) {
+ ? ? ? ? ? ? ? for (i = 0; i < RTOUT; i++) {
+ ? ? ? ? ? ? ? ? ? ? ? spi_xfer(priv->slave, 1 * 8, NULL, &r1, 0);
+ ? ? ? ? ? ? ? ? ? ? ? if (r1 != 0xff)
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? break;
+ ? ? ? ? ? ? ? }

drivers/mmc/mmc_spi.c new file +7 -0

+}
+
+static uint mmc_spi_writedata(struct mmc *mmc, const char *buf,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? u32 bcnt, u32 bsize)
+{
+ ? ? ? struct mmc_spi_priv *priv = mmc->priv;
+ ? ? ? u8 r1;

+ ? ? ? ? ? ? ? ? ? ? ? for (i = 0; i < WTOUT; i++) {
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? spi_xfer(priv->slave, 1 * 8, NULL, &r1, 0);
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? if (r1 == 0xff) {
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? r1 = 0;
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? break;
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? }
+ ? ? ? ? ? ? ? ? ? ? ? }
+ ? ? ? ? ? ? ? ? ? ? ? if (i == WTOUT) {
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? debug("%s:wtout %x\n", __func__, r1);
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? r1 = 0x04;

+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? break;
+ ? ? ? ? ? ? ? ? ? ? ? }
+ ? ? ? ? ? ? ? } else
+ ? ? ? ? ? ? ? ? ? ? ? break;
+ ? ? ? }
+ ? ? ? return r1;

+ ? ? ? u8 crc[2];
+ ? ? ? int i;
+ ? ? ? while (bcnt--) {
+ ? ? ? ? ? ? ? spi_xfer(priv->slave, 2 * 8, tok, NULL, 0);
+ ? ? ? ? ? ? ? spi_xfer(priv->slave, bsize * 8, buf, NULL, 0);
+ ? ? ? ? ? ? ? buf += bsize;
+ ? ? ? ? ? ? ? spi_xfer(priv->slave, 2 * 8, crc, NULL, 0);
+ ? ? ? ? ? ? ? spi_xfer(priv->slave, 1 * 8, NULL, &r1, 0);
+ ? ? ? ? ? ? ? if (r1 == 0x05) {
+ ? ? ? ? ? ? ? ? ? ? ? for (i = 0; i < WTOUT; i++) {
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? spi_xfer(priv->slave, 1 * 8, NULL, &r1, 0);
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? if (r1 == 0xff) {
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? r1 = 0;
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? break;
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? }
+ ? ? ? ? ? ? ? ? ? ? ? }
+ ? ? ? ? ? ? ? ? ? ? ? if (i == WTOUT) {
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? debug("%s:wtout %x\n", __func__, r1);
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? r1 = 0x04;
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? break;
+ ? ? ? ? ? ? ? ? ? ? ? }
+ ? ? ? ? ? ? ? }
+ ? ? ? }
+ ? ? ? if (bcnt == 0 && r1 == 0) {
+ ? ? ? ? ? ? ? spi_xfer(priv->slave, 2 * 8, stop, NULL, 0);
+ ? ? ? ? ? ? ? for (i = 0; i < WTOUT; i++) {
+ ? ? ? ? ? ? ? ? ? ? ? spi_xfer(priv->slave, 1 * 8, NULL, &r1, 0);
+ ? ? ? ? ? ? ? ? ? ? ? if (r1 == 0xff) {
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? r1 = 0;
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? break;
+ ? ? ? ? ? ? ? ? ? ? ? }
+ ? ? ? ? ? ? ? }
+ ? ? ? ? ? ? ? if (i == WTOUT) {
+ ? ? ? ? ? ? ? ? ? ? ? debug("%s:wtout %x\n", __func__, r1);
+ ? ? ? ? ? ? ? ? ? ? ? r1 = 0x04;
+ ? ? ? ? ? ? ? }
+ ? ? ? }
+ ? ? ? return r1;

drivers/mmc/mmc_spi.c new file +34 -0

+{
+ ? ? ? return (d[0] << 24) | (d[1] << 16) | (d[2] << 8) | d[3];
+}
+
+static inline void mmc_spi_deactivate(struct mmc *mmc)
+{
+ ? ? ? struct mmc_spi_priv *priv = mmc->priv;
+ ? ? ? spi_xfer(priv->slave, 1 * 8, NULL, NULL, SPI_XFER_END);
+ ? ? ? spi_xfer(priv->slave, 1 * 8, NULL, NULL, 0);
+}
+
+static int mmc_spi_request(struct mmc *mmc, struct mmc_cmd *cmd,
+ ? ? ? ? ? ? ? struct mmc_data *data)
+{
+ ? ? ? struct mmc_spi_priv *priv = mmc->priv;
+ ? ? ? u8 r1;
+ ? ? ? ushort cmdidx = cmd->cmdidx;
+ ? ? ? uint cmdarg = cmd->cmdarg;
+ ? ? ? u8 resp[16];
+ ? ? ? int i;
+ ? ? ? int ret = 0;
+ ? ? ? debug("%s:cmd%d %x %x %x\n", __func__,
+ ? ? ? ? ? ? cmd->cmdidx, cmd->resp_type, cmd->cmdarg, cmd->flags);
+ ? ? ? if (cmdidx == MMC_CMD_ALL_SEND_CID)
+ ? ? ? ? ? ? ? cmdidx = MMC_CMD_SEND_CID;
+ ? ? ? r1 = mmc_spi_sendcmd(mmc, cmdidx, cmdarg);
+ ? ? ? if (r1 == 0xff) {
+ ? ? ? ? ? ? ? ret = NO_CARD_ERR;
+ ? ? ? ? ? ? ? goto done;
+ ? ? ? } else if (cmd->resp_type == MMC_RSP_R2) {
+ ? ? ? ? ? ? ? r1 = mmc_spi_readdata(mmc, resp, 1, 16);
+ ? ? ? ? ? ? ? for (i = 0; i < 4; i++)
+ ? ? ? ? ? ? ? ? ? ? ? cmd->response[i] = rswab(resp + i * 4);
+ ? ? ? ? ? ? ? debug("r128 %x %x %x %x\n", cmd->response[0], cmd->response[1],

+ ? ? ? ? ? ? ? ? ? ? ? spi_xfer(priv->slave, 4 * 8,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?NULL, resp, 0);
+ ? ? ? ? ? ? ? ? ? ? ? priv->ocr58 = rswab(resp) & ~OCR_BUSY;
+ ? ? ? ? ? ? ? ? ? ? ? debug("ocr58 %x\n", priv->ocr58);
+ ? ? ? ? ? ? ? } else if (cmdidx == SD_CMD_SEND_IF_COND) {
+ ? ? ? ? ? ? ? ? ? ? ? if ((r1 & 0x7e) == 0)
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? priv->sd_if_cond = 1;
+ ? ? ? ? ? ? ? } else if (cmdidx == SD_CMD_APP_SEND_OP_COND ||
+ ? ? ? ? ? ? ? ? ? ? ? ? ?cmdidx == MMC_CMD_SEND_OP_COND) {
+ ? ? ? ? ? ? ? ? ? ? ? if (r1 & 0x04)
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ret = TIMEOUT;
+ ? ? ? ? ? ? ? ? ? ? ? else {
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? if (r1 & 0x01)
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? cmd->response[0] = priv->ocr58;
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? else {
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? mmc_spi_deactivate(mmc);
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? r1 = mmc_spi_sendcmd(mmc, 58, 0);
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? spi_xfer(priv->slave, 4 * 8,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?NULL, resp, 0);
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? priv->ocr58 = rswab(resp);
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? debug("ocr58 %x\n", priv->ocr58);
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? cmd->response[0] = priv->ocr58;
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? }
+ ? ? ? ? ? ? ? ? ? ? ? }
+ ? ? ? ? ? ? ? } else {
+ ? ? ? ? ? ? ? ? ? ? ? cmd->response[0] = 0;
+ ? ? ? ? ? ? ? ? ? ? ? if (r1 & 0x7e)
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? cmd->response[0] |= (1 << 19);
+ ? ? ? ? ? ? ? ? ? ? ? if (r1 & 0x40)
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? cmd->response[0] |= (1 << 31);
+ ? ? ? ? ? ? ? ? ? ? ? if (r1 & 0x20)
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? cmd->response[0] |= (1 << 30);
+ ? ? ? ? ? ? ? ? ? ? ? if (r1 & 0x10)

+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? cmd->response[0] |= (1 << 28);
+ ? ? ? ? ? ? ? ? ? ? ? if (r1 & 0x08)
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? cmd->response[0] |= (1 << 23);
+ ? ? ? ? ? ? ? ? ? ? ? if (r1 & 0x04)
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? cmd->response[0] |= (1 << 22);
+ ? ? ? ? ? ? ? ? ? ? ? if (r1 & 0x02)
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? cmd->response[0] |= (1 << 13);
+ ? ? ? ? ? ? ? }

+ ? ? ? }
+ ? ? ? if (data) {
+ ? ? ? ? ? ? ? debug("%s:data %x %x %x\n", __func__,
+ ? ? ? ? ? ? ? ? ? ? data->flags, data->blocks, data->blocksize);
+ ? ? ? ? ? ? ? if (data->flags == MMC_DATA_READ) {
+ ? ? ? ? ? ? ? ? ? ? ? r1 = mmc_spi_readdata(mmc, data->dest,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? data->blocks, data->blocksize);

+ ? ? ? ? ? ? ? } else if ?(data->flags == MMC_DATA_WRITE) {
+ ? ? ? ? ? ? ? ? ? ? ? if (cmdidx == MMC_CMD_WRITE_MULTIPLE_BLOCK)
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? r1 = mmc_spi_writeblock(mmc, data->src,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? data->blocks, data->blocksize);
+ ? ? ? ? ? ? ? ? ? ? ? else
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? r1 = mmc_spi_writedata(mmc, data->src,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? data->blocks, data->blocksize);
+ ? ? ? ? ? ? ? }

drivers/mmc/mmc_spi.c new file +15 -0

+struct mmc *mmc_spi_init(uint bus, uint cs, uint speed, uint mode)
+{
+ ? ? ? struct mmc *mmc;
+ ? ? ? struct mmc_spi_priv *priv;
+
+ ? ? ? mmc = malloc(sizeof(*mmc));
+ ? ? ? if (!mmc)
+ ? ? ? ? ? ? ? return NULL;
+ ? ? ? priv = malloc(sizeof(*priv));
+ ? ? ? if (!priv) {
+ ? ? ? ? ? ? ? free(mmc);
+ ? ? ? ? ? ? ? return NULL;
+ ? ? ? }
+ ? ? ? mmc->priv = priv;
+ ? ? ? priv->bus = bus;

+ ? ? ? mmc->f_max = speed;
+ ? ? ? mmc->f_min = mmc->f_max >> 9;

Quoted 4 lines

The crc7 lib func is merged from linux and used to compute mmc command checksum.

This should probably be a separate patch.

Quoted 8 lines

common/cmd_mmc_spi.c new file +6 -0

+
+       do {
+               mmc = find_mmc_device(++dev_num);
+       } while (mmc&&  strcmp(mmc->name, "MMC_SPI"));
+       if (!mmc) {
+               printf("Create MMC Device\n");

common/cmd_mmc_spi.c line 52

Quoted 10 lines

common/cmd_mmc_spi.c new file +10 -0

+               mmc = mmc_spi_init(CONFIG_MMC_SPI_BUS,
+                                  CONFIG_MMC_SPI_CS,
+                                  CONFIG_MMC_SPI_SPEED,
+                                  CONFIG_MMC_SPI_MODE);
+               if (!mmc) {
+                       printf("Failed to create MMC Device\n");
+                       return 1;
+               }
+               dev_num = mmc->block_dev.dev;
+       }

I'm not sure I understand the logic behind this code. The arguments to the command should be used to either find the already-existing bus, or to create a new one. Unless I'm misunderstanding, this searches for the first MMC_SPI bus, and if it finds it, uses that, otherwise it creates a new one with the values specified in the config file. Then it parses the command and overwrites the old parameters for the bus with new ones? Why? My instinct would be to create a separate instance of an MMC_SPI bus for each bus and chip select. My SPI is rusty, so maybe chip-select should be configurable on a use-by-use basis.

Certainly the current code will only use at most one MMC_SPI bus even if more are created, which seems wrong.

Quoted 3 lines

+ cmdo[1] = 0x40 + cmdidx;

Use a #define'd constant for 0x40. Maybe do this:

/* MMC SPI commands start with a start bit "0" and a transmit bit "1" */ #define MMC_SPI_CMD(x) (0x40 | (x& 0x3f))

cmdo[1] = MMC_SPI_CMD(cmdidx);

Why not check the CRC to make sure your data is correct?

Review

Quoted 3 lines

(quoted diff) +2 -0

+
+static inline uint rswab(u8 *d)

Did you mean "swap", here?

Hmmm.... I'm not entirely sure this is the way to go. If I'm reading this correctly, this function is converting between the standard SD protocol, and the SPI version of the protocol. It might be better to make mmc.c aware of which protocol it is using, so it a) doesn't issue commands that the SPI card doesn't understand, and b) can properly interpret responses.

Quoted 7 lines

drivers/mmc/mmc_spi.c new file +5 -0

+       }
+       if (data) {
+               debug("%s:data %x %x %x\n", __func__,
+                     data->flags, data->blocks, data->blocksize);
+               if (data->flags == MMC_DATA_READ) {

drivers/mmc/mmc_spi.c line 249

Quoted 10 lines

drivers/mmc/mmc_spi.c new file +10 -0

+                       r1 = mmc_spi_readdata(mmc, data->dest,
+                                       data->blocks, data->blocksize);
+               } else if  (data->flags == MMC_DATA_WRITE) {
+                       if (cmdidx == MMC_CMD_WRITE_MULTIPLE_BLOCK)
+                               r1 = mmc_spi_writeblock(mmc, data->src,
+                                       data->blocks, data->blocksize);
+                       else
+                               r1 = mmc_spi_writedata(mmc, data->src,
+                                       data->blocks, data->blocksize);
+               }

Why not check r1 to make sure your writes actually succeeded?

Review

Quoted 4 lines

(quoted diff) +2 -0

+
+       mmc->voltages = MMC_VDD_32_33 | MMC_VDD_33_34;

Is this part of the SPI spec? If so, this is fine, otherwise, we need to get the voltage information from the SPI bus, somehow.

Review

Quoted 4 lines

(quoted diff) +2 -0

+       mmc->f_max = speed;
+       mmc->f_min = mmc->f_max>>  9;

What's the logic behind f_min being f_max/512?

Quoted 18 lines

common/cmd_mmc_spi.c new file +16 -0

+static int do_mmc_spi(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
+{
+       int dev_num = -1;
+       uint bus;
+       uint cs;
+       uint speed;
+       uint mode;
+       char *endp;
+       struct mmc *mmc;
+       struct mmc_spi_priv *priv;
+
+       do {
+               mmc = find_mmc_device(++dev_num);
+       } while (mmc&&  strcmp(mmc->name, "MMC_SPI"));
+       if (!mmc) {
+               printf("Create MMC Device\n");

common/cmd_mmc_spi.c line 52

Quoted 10 lines

common/cmd_mmc_spi.c new file +10 -0

+               mmc = mmc_spi_init(CONFIG_MMC_SPI_BUS,
+                                  CONFIG_MMC_SPI_CS,
+                                  CONFIG_MMC_SPI_SPEED,
+                                  CONFIG_MMC_SPI_MODE);
+               if (!mmc) {
+                       printf("Failed to create MMC Device\n");
+                       return 1;
+               }
+               dev_num = mmc->block_dev.dev;
+       }

I'm not sure I understand the logic behind this code. The arguments to the command should be used to either find the already-existing bus, or to create a new one. Unless I'm misunderstanding, this searches for the first MMC_SPI bus, and if it finds it, uses that, otherwise it creates a new one with the values specified in the config file. Then it parses the command and overwrites the old parameters for the bus with new ones? Why? My instinct would be to create a separate instance of an MMC_SPI bus for each bus and chip select. My SPI is rusty, so maybe chip-select should be configurable on a use-by-use basis.

Certainly the current code will only use at most one MMC_SPI bus even if more are created, which seems wrong.

2. Andy suggested to create a new mmc_spi device for each bus and cs.

Quoted 2 lines · On Thursday 29 April 2010 10:51:03 Thomas Chou wrote:

2. Andy suggested to create a new mmc_spi device for each bus and cs.

this is what i was suggesting from the start ... sorry if i wasnt clear -mike

We need to query mmc devices in mmc_spi subcommand and don't want the "Device not found" message.

Signed-off-by Thomas Chou <[email protected]>

---
 drivers/mmc/mmc.c |   14 +++++++++++---
 include/mmc.h     |    1 +
 2 files changed, 12 insertions(+), 3 deletions(-)

drivers/mmc/mmc.c +1 -3

index cf4ea16..55ab13e 100644
@@ -60,7 +60,7 @@ int mmc_set_blocklen(struct mmc *mmc, int len)
 	return mmc_send_cmd(mmc, &cmd, NULL);
 }
 
-struct mmc *find_mmc_device(int dev_num)
+struct mmc *find_mmc_device_quiet(int dev_num)
 {
 	struct mmc *m;
 	struct list_head *entry;

@@ -72,11 +72,19 @@ struct mmc *find_mmc_device(int dev_num)
 			return m;
 	}
 
-	printf("MMC Device %d not found\n", dev_num);
-
 	return NULL;
 }
 

Dear Thomas Chou,

Quoted 3 lines · In message<[email protected]> you wrote:

We need to query mmc devices in mmc_spi subcommand and don't want the "Device not found" message.

Your subject line is way too long.

Instead of creating a new function please consider using an argument instead?

An argument, verbose, is added to enable/disable the "Device not found" message. Because we need to query mmc devices in mmc_spi subcommand and don't want the message.

common/cmd_mmc_spi.c line 69

Quoted 4 lines · On Wednesday, May 19, 2010 00:37:47 Thomas Chou wrote:

common/cmd_mmc_spi.c new file +2 -0

+	printf("%s: %d at %u:%u %u %u\n", mmc->name, mmc->block_dev.dev,
+	       bus, cs, speed, mode);

this is a bit terse. how about prefixing the hz output with like "hz:" and the mode with like "mode:" ?

common/cmd_mmc_spi.c line 82

Quoted 7 lines

common/cmd_mmc_spi.c new file +6 -0

+U_BOOT_CMD(
+	mmc_spi,	4,	0,	do_mmc_spi,
+	"mmc_spi setup",
+	"[bus:]cs [hz] [mode]	- setup mmc_spi device on given\n"
+	"				   SPI bus and chip select\n"
+);

there should be no newline at the end of the help string

drivers/mmc/mmc_spi.c line 291

Quoted 7 lines

drivers/mmc/mmc_spi.c new file +4 -0

+struct mmc *mmc_spi_init(uint bus, uint cs, uint speed, uint mode)
+{
+	struct mmc *mmc;
+	mmc->b_max = MMC_SPI_MAX_BLOCKS;

do you have some local modification ? i dont see b_max anywhere in include/mmc.h ...

unfortunately though, i tried this on my system and it doesnt seem to work. using a simple SPI<->MMC card, the old mmc_spi driver works on my board, but booting the new u-boot and running the same things shows:

mmc_spi_request:cmd1 1 40300000 0
mmc_spi_sendcmd:cmd1 resp4 1
<these last 2 lines repeat for a while>
Card did not respond to voltage select!

Quoted 10 lines · Mike Frysinger

drivers/mmc/mmc_spi.c line 291

Quoted 6 lines · On Wednesday, May 19, 2010 00:37:47 Thomas Chou wrote:

drivers/mmc/mmc_spi.c new file +4 -0

+struct mmc *mmc_spi_init(uint bus, uint cs, uint speed, uint mode)
+{
+	struct mmc *mmc;
+	mmc->b_max = MMC_SPI_MAX_BLOCKS;

do you have some local modification ? i dont see b_max anywhere in include/mmc.h ...

Please apply the multi-blocks patches (1,2,3/4) from Alagu on 05/12, as Andy said he had applied them.

Quoted 9 lines

unfortunately though, i tried this on my system and it doesnt seem to work. using a simple SPI<->MMC card, the old mmc_spi driver works on my board, but booting the new u-boot and running the same things shows:

mmc_spi_request:cmd1 1 40300000 0
mmc_spi_sendcmd:cmd1 resp4 1
<these last 2 lines repeat for a while>
Card did not respond to voltage select!

It seems the mmc card was not initialized and timed out. Please try remove the OCR_HCS in mmc_send_op_cond() of mmc.c temporarily.

	cmd.cmdarg = OCR_HCS | mmc->voltages;

---------------------^^^^^^^^^