[U-Boot] [PATCH v5] mmc: add generic mmc spi driver
Andy Fleming
afleming at gmail.com
Wed Apr 28 17:21:01 CEST 2010
On Wed, Apr 28, 2010 at 1:00 AM, Thomas Chou <thomas at wytron.com.tw> wrote:
> 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.
This should probably be a separate patch.
>
> There is a subcomamnd "mmc_spi" to setup spi bus and cs at run time.
>
> Signed-off-by: Thomas Chou <thomas at wytron.com.tw>
> +
> +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");
> + 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.
> +
> + 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;
> + }
> + }
> diff --git a/drivers/mmc/mmc_spi.c b/drivers/mmc/mmc_spi.c
> new file mode 100644
> index 0000000..dedcef7
> --- /dev/null
> +++ b/drivers/mmc/mmc_spi.c
> @@ -0,0 +1,318 @@
> +/*
> + * generic mmc spi driver
> + *
> +static uint mmc_spi_sendcmd(struct mmc *mmc, u8 cmdidx, u32 cmdarg)
> +{
> + struct mmc_spi_priv *priv = mmc->priv;
> + u8 cmdo[7];
> + u8 r1;
> + int i;
> + cmdo[0] = 0xff;
> + 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);
> + 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)
define a constant
> + break;
> + }
> + debug("%s:cmd%d resp%d %x\n", __func__, cmdidx, i, r1);
> + return r1;
> +}
> +
> +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;
> + }
> + debug("%s:tok%d %x\n", __func__, i, r1);
> + if (r1 == 0xfe) {
> + spi_xfer(priv->slave, bsize * 8, NULL, buf, 0);
> + buf += bsize;
> + spi_xfer(priv->slave, 2 * 8, NULL, crc, 0);
> + r1 = 0;
> + } else
> + break;
> + }
> + return r1;
Why not check the CRC to make sure your data is correct?
> +}
> +
> +static uint mmc_spi_writedata(struct mmc *mmc, const char *buf,
> + u32 bcnt, u32 bsize)
> +{
> + struct mmc_spi_priv *priv = mmc->priv;
> + u8 r1;
> + u8 tok[2] = { 0xff, 0xfe };
> + 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) {
Either define a named constant for 0x05, or put a comment describing
what's happening. Isn't this technically a data response, rather than
r1? Also,
> + 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;
A constant for this would be good, too.
> + break;
> + }
> + } else
> + break;
> + }
> + return r1;
> +}
> +
> +static uint mmc_spi_writeblock(struct mmc *mmc, const char *buf,
> + u32 bcnt, u32 bsize)
> +{
> + struct mmc_spi_priv *priv = mmc->priv;
> + u8 r1;
> + u8 tok[2] = { 0xff, 0xfc };
> + u8 stop[2] = { 0xff, 0xfd };
Constants for 0xfc and 0xfd, please.
> + 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;
> +}
> +
> +static inline uint rswab(u8 *d)
Did you mean "swap", here?
> +{
> + 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],
> + cmd->response[2], cmd->response[3]);
> + } else {
> + if (!data) {
> + spi_xfer(priv->slave, 4 * 8, NULL, resp, 0);
> + cmd->response[0] = rswab(resp);
> + debug("r32 %x\n", cmd->response[0]);
> + }
> + if (cmdidx == MMC_CMD_GO_IDLE_STATE) {
> + mmc_spi_deactivate(mmc);
> + r1 = mmc_spi_sendcmd(mmc, 58, 0);
Name "58", please.
> + 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);
> + }
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.
> + }
> + 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);
> + }
Why not check r1 to make sure your writes actually succeeded?
> +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;
> + priv->cs = cs;
> + priv->speed = speed;
> + priv->mode = mode;
> + sprintf(mmc->name, "MMC_SPI");
> + mmc->send_cmd = mmc_spi_request;
> + mmc->set_ios = mmc_spi_set_ios;
> + mmc->init = mmc_spi_init_p;
> + mmc->host_caps = 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.
> + mmc->f_max = speed;
> + mmc->f_min = mmc->f_max >> 9;
What's the logic behind f_min being f_max/512?
> + mmc->block_dev.part_type = PART_TYPE_DOS;
> +
> + mmc_register(mmc);
> +
> + return mmc;
> +}
Andy
More information about the U-Boot
mailing list