[U-Boot] [PATCH 06/10] Add MMC Framework
Andy Fleming
afleming at gmail.com
Tue Nov 4 20:40:26 CET 2008
On Tue, Nov 4, 2008 at 3:37 AM, Haavard Skinnemoen
<haavard.skinnemoen at atmel.com> wrote:
> Andy Fleming <afleming at freescale.com> wrote:
>> + mmc_init(mmc);
>> +
>> + n = mmc->block_dev.block_write(dev, blk, cnt, addr);
>> +
>> + printf("%d blocks written: %s\n",
>> + n, (n == cnt) ? "OK" : "ERROR");
>> + return (n == cnt) ? 0 : 1;
>> + } else {
>> + printf("Usage:\n%s\n", cmdtp->usage);
>> + rc = 1;
>> + }
>
> Is there any way to rescan the bus like the old "mmcinit" command did?
> This can be useful if you need to change the card.
I think this is being done. I designed it so that it acts like
eth_init(), being called every time you start a new transaction. It's
a bit clunky, but I liked it better than doing it manually. Is there
a command I missed, that doesn't invoke mmc_init() first?
>> +
>> +static struct mmc *mmc_devices;
>
> Maybe a struct list_head would be more appropriate here?
Yeah, it would. I copied this from eth.c, but that probably predates
list_head's addition to u-boot.
>> +static ulong mmc_bread(int dev_num, ulong start, lbaint_t blkcnt, void *dst)
>> +{
>> + int err;
>> + ulong src;
>> + struct mmc *mmc = find_mmc_device(dev_num);
>> +
>> + if (!mmc)
>> + return 0;
>> +
>> + src = start * mmc->read_bl_len;
>> +
>> + err = mmc_read(mmc, src, dst, blkcnt * mmc->read_bl_len);
>
> Hmm...here you multiply to get a byte offset, then mmc_read does a
> division to get back to the block numbers. Wouldn't it be better to do
> it the other way around and let mmc_read() call mmc_bread()? It would
> also avoid the bounce buffer allocation and unaligned block handling.
Yeah, I was thinking about that, but decided not to, for some reason.
I will reconsider again, as it fits better with my way of thinking
about it.
I think it may have had something to do with large reads.
>
> I also think this will overflow on SDHC since src is a 32-bit quantity
> on most platforms, and SDHC cards can be larger than 4 GiB.
Good catch.
>> +
>> + cmd.cmdidx = SD_CMD_APP_SEND_OP_COND;
>> + cmd.resp_type = MMC_CMD_R3;
>> + cmd.cmdarg = 0xff8000;
>
> Hmm...hardcoded voltages? Better let the board define that...
Oh. Ugh, yeah. I had a feeling I had some stuff like that lying around.
>> + cmd.cmdidx = MMC_CMD_SEND_OP_COND;
>> + cmd.resp_type = MMC_CMD_R3;
>> + cmd.cmdarg = 0x40ff8000;
>
> Same here. Would be nice if this could be defined in terms of a few
> constants too...it's hard to tell exactly what voltages you're
> announcing here...
Agreed.
>> +
>> + /* Only version 4 supports high-speed */
>> + if (mmc->version < MMC_VERSION_4)
>> + return 0;
>
> Hmm...but mmc_change_freq() doesn't sound like it has anything to do
> with high-speed...?
I could use a better comment there. And probably a better function name.
>
>> + mmc->caps |= MMC_MODE_4BIT;
>
> I though the host was supposed to set that. It may even depend on how
> the board is wired.
Ah, in this case, I was determining the card capabilities, but I do
need to match those against host capabilities.
>> + /* Set the card to use High Speed */
>> + cmd.cmdidx = MMC_CMD_SWITCH;
>> + cmd.resp_type = MMC_CMD_R1b;
>> + cmd.cmdarg = 0x1b90100;
>
> Some constant(s) would be nice...
Yeah, my bad. I was in a rush, and didn't bother to look up these constants.
>> + /* High Speed is set, there are two types: 52MHz and 26MHz */
>> + if (cardtype & MMC_HS_52MHZ)
>> + mmc->caps |= MMC_MODE_HS_52MHz | MMC_MODE_HS;
>> + else
>> + mmc->caps |= MMC_MODE_HS;
>
> How do you know the host supports high-speed? How does the host driver
> know when to switch into high-speed mode?
Yeah, that's a bit sketchy and untested. It would be better if I
added a way for the host to declare a max speed. I can steal from the
Linux code.
>> + if (mmc->scr[0] & SD_DATA_4BIT)
>> + mmc->caps |= MMC_MODE_4BIT;
>
> Again, we don't know if the host supports this.
Agreed.
>> + cmd.cmdidx = SD_CMD_SWITCH_FUNC;
>> + cmd.resp_type = MMC_CMD_R1;
>> + cmd.cmdarg = 0x80fffff1;
>
> Constants please.
*hangs head in shame*
>> +
>> + if ((switch_status[4] & 0x0f000000) == 0x01000000)
>> + mmc->caps |= MMC_MODE_HS;
>
> And we overrule the host again. Note that the host may support any
> combination of 1-bit, 4-bit, 8-bit busses and normal/high-speed
> signaling, and it needs to be told when to switch between them.
Will do.
>> + mmc->capacity = (csize + 1) << (cmult + 2);
>> + mmc->capacity *= mmc->read_bl_len;
>
> 64-bit multiply...hopefully all platforms are fine with that.
Are there platforms that can't do a 64-bit multiply in 32-bit chunks?
I tested this on an e500 core, which is only 32-bit, but the compiler
handles making it work. If there's a platform that doesn't support
it, they will have to come in and fix this code in whatever their
standard way for handling such things is.
>> + if (mmc->caps & MMC_MODE_HS)
>> + mmc_set_clock(mmc, 12000000);
>> + else
>> + mmc_set_clock(mmc, 12000000);
>
> Surely high-speed capable cards can do more than 12 MHz? And where are
> the host/board/card-specific frequency limits taken into account?
Ugh. This is an unintentional bug (rather than the earlier ones,
which were results of laziness). This was put in during testing of
our hardware, because there were issues with running at higher speeds.
I will fix that.
>> + if (mmc->caps & MMC_MODE_HS) {
>> + if (mmc->caps & MMC_MODE_HS_52MHz)
>> + mmc_set_clock(mmc, 52000000);
>> + else
>> + mmc_set_clock(mmc, 26000000);
>> + } else
>> + mmc_set_clock(mmc, 20000000);
>
> Same here. You calculated tran_speed above, why don't you use it?
Will do.
>> +int mmc_send_if_cond(struct mmc *mmc)
>> +{
>> + struct mmc_cmd cmd;
>> + int err;
>> +
>> + cmd.cmdidx = SD_CMD_SEND_IF_COND;
>> + cmd.cmdarg = 0x1aa;
>
> Constant(s) please. Should this be board-dependent?
Yes. The "aa" is universal, but the "1" is host-specific
(board-specific things will be left up to
the host driver, where possible)
>> + if (((uint *)(cmd.response))[0] != 0x1aa)
>
> Ditto.
Yeah.
>> +int mmc_register(struct mmc *mmc)
>> +{
>> + struct mmc *m;
>> +
>> + if (!mmc_devices)
>> + mmc_devices = mmc;
>> + else {
>> + for (m = mmc_devices; m->next != mmc_devices; m=m->next);
>> + m->next = mmc;
>> + }
>> +
>> + mmc->next = mmc_devices;
>
> A list_head would make this so much simpler:
>
> list_add_tail(&mmc->node, &mmc_devices);
Agreed.
>> -int mmc_legacy_init(int verbose);
>> -int mmc_read(ulong src, uchar *dst, int size);
>> -int mmc_write(uchar *src, ulong dst, int size);
>> +#define SD_HIGHSPEED_BUSY 0x00020000
>> +#define SD_HIGHSPEED_SUPPORTED 0x00020000
>
> Aren't those identical?
The appear to be...I'll figure out why they both exist.
>> +
>> +enum {
>> + MMC_CMD_RSP_NONE,
>> + MMC_CMD_R1 = 1,
>> + MMC_CMD_R1b,
>> + MMC_CMD_R2,
>> + MMC_CMD_R3,
>> + MMC_CMD_R4,
>> + MMC_CMD_R5,
>> + MMC_CMD_R5b,
>> + MMC_CMD_R6,
>> + MMC_CMD_R7
>
> Please define these in terms of sensible constants which make it easy to
> deduce the length of the reply, whether the CRC is expected to be
> valid, whether the card may signal busy, etc. The following flags seem
> to do the trick on Linux:
>
> #define MMC_RSP_PRESENT (1 << 0)
> #define MMC_RSP_136 (1 << 1) /* 136 bit response */
> #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 */
>
> which becomes
>
> #define MMC_RSP_NONE (0)
> #define MMC_RSP_R1 (MMC_RSP_PRESENT|MMC_RSP_CRC|MMC_RSP_OPCODE)
> #define MMC_RSP_R1B (MMC_RSP_PRESENT|MMC_RSP_CRC|MMC_RSP_OPCODE|MMC_RSP_BUSY)
> #define MMC_RSP_R2 (MMC_RSP_PRESENT|MMC_RSP_136|MMC_RSP_CRC)
> #define MMC_RSP_R3 (MMC_RSP_PRESENT)
> #define MMC_RSP_R4 (MMC_RSP_PRESENT)
> #define MMC_RSP_R5 (MMC_RSP_PRESENT|MMC_RSP_CRC|MMC_RSP_OPCODE)
> #define MMC_RSP_R6 (MMC_RSP_PRESENT|MMC_RSP_CRC|MMC_RSP_OPCODE)
> #define MMC_RSP_R7 (MMC_RSP_PRESENT|MMC_RSP_CRC|MMC_RSP_OPCODE)
...Yes, that makes much more sense.
>> +};
>> +
>> +struct mmc_cid {
>> + unsigned long psn;
>> + unsigned short oid;
>> + unsigned char mid;
>> + unsigned char prv;
>> + unsigned char mdt;
>> + char pnm[7];
>> +};
>> +
>> +struct mmc_csd
>> +{
>> + u8 csd_structure:2,
>> + spec_vers:4,
>> + rsvd1:2;
>> + u8 taac;
>> + u8 nsac;
>> + u8 tran_speed;
>> + u16 ccc:12,
>> + read_bl_len:4;
>> + u64 read_bl_partial:1,
>> + write_blk_misalign:1,
>> + read_blk_misalign:1,
>> + dsr_imp:1,
>> + rsvd2:2,
>> + c_size:12,
>> + vdd_r_curr_min:3,
>> + vdd_r_curr_max:3,
>> + vdd_w_curr_min:3,
>> + vdd_w_curr_max:3,
>> + c_size_mult:3,
>> + sector_size:5,
>> + erase_grp_size:5,
>> + wp_grp_size:5,
>> + wp_grp_enable:1,
>> + default_ecc:2,
>> + r2w_factor:3,
>> + write_bl_len:4,
>> + write_bl_partial:1,
>> + rsvd3:5;
>> + u8 file_format_grp:1,
>> + copy:1,
>> + perm_write_protect:1,
>> + tmp_write_protect:1,
>> + file_format:2,
>> + ecc:2;
>> + u8 crc:7;
>> + u8 one:1;
>> +};
>
> Are these the same in all versions of the SD and MMC specs?
Honestly, I didn't bother to check. None of this code uses it, I just
put it in there for later enhancements (and to allow the drivers which
were using it to continue doing so. I suspect that there are
endianness issues.
>> +struct mmc_cmd {
>> + ushort cmdidx;
>> + int resp_type;
>
> Should be unsigned.
Well...now. Before, it was a number between 0 and 9. But I will
change it when I change the constants.
>
>> + uint cmdarg;
>> + char response[18];
>
> Hmm...why 18 bytes? Linux only needs 4 32-bit words.
...Not sure.
>
>> + uint flags;
>> +};
>> +
>> +struct mmc_data {
>> + char *dest;
>> + const char *src; /* src buffers don't get written to */
>
> But you never use both at the same time. Why two pointers?
This is silly, but to get rid of a warning. Maybe my brain's just not
working, but I couldn't think of a way to allow the same pointer to
take assignments from bread and bwrite, as bwrite uses a constant
source, and bread has a writable destination. This was my
questionable hack.
>
>> + uint flags;
>> + int blocks;
>> + int blocksize;
>
> Do these have to be signed?
I don't think they do.
>> +};
>> +
>> +struct mmc {
>> + char name[32];
>> + void *priv;
>> + uint initialized;
>> + uint version;
>> + int high_capacity;
>
> There are a few 32-bit fields which contain 1 bit of information. Maybe
> they can be combined into some kind of "flags" field?
Sure.
>
>> + uint bus_width;
>> + uint clock;
>> + uint caps;
>
> Caps for whom? The host or the card?
Certainly I need to define that better, and add host capabilities
>
>> + uint ocr;
>> + uint scr[2];
>> + uint csd[4];
>> + char cid[16];
>> + ushort rca;
>> + uint tran_speed;
>
> It's not entirely clear how this is different from "clock". Ok, since I
> just went through the code, I happen to know the difference, but I'm
> sure others will be confused.
Will clarify that clock is the current operating frequency, while
tran_speed is the current card's maximum operating frequency.
>
>> + uint read_bl_len;
>> + uint write_bl_len;
>> + u64 capacity;
>> + block_dev_desc_t block_dev;
>> + int (*send_cmd)(struct mmc *mmc,
>> + struct mmc_cmd *cmd, struct mmc_data *data);
>> + void (*set_ios)(struct mmc *mmc);
>> + int (*init)(struct mmc *mmc);
>> + struct mmc *next;
>
> Some documentation would be nice. Maybe it would be a good idea to
> separate the host-specific bits from the card-specific bits too?
More documentation: will do. I'm not convinced on separation, though.
It's certainly easy to separate them visually, but I'm not sure
there's any technical advantage to be gained from putting the
different bits in different structures. It would involve a lot more
dereferencing, and I tend to think of u-boot drivers as a little less
concerned with strict encapsulation. The one thing I could think of
would be to add support for multiple, concurrent cards. To be honest,
that's a problem I haven't even thought about, and I'm not sure I want
to at this stage.
>
>> +};
>> +
>> +int mmc_register(struct mmc *mmc);
>> +int mmc_initialize(bd_t *bis);
>> +int mmc_init(struct mmc *mmc);
>> +int mmc_read(struct mmc *mmc, ulong src, uchar *dst, int size);
>> +struct mmc *find_mmc_device(int dev_num);
>> +void print_mmc_devices(char separator);
>
> These are not the only non-static functions you define:
>
> +int do_mmcinfo (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
> +int do_mmcops(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
> +int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data)
> +int mmc_set_blocklen(struct mmc *mmc, int len)
> +int mmc_read_block(struct mmc *mmc, void *dst, uint blocknum)
> +int mmc_go_idle(struct mmc* mmc)
> +int
> +sd_send_op_cond(struct mmc *mmc)
> +int mmc_send_op_cond(struct mmc *mmc)
> +int mmc_send_ext_csd(struct mmc *mmc, char *ext_csd)
> +int mmc_change_freq(struct mmc *mmc)
> +int sd_change_freq(struct mmc *mmc)
> +int fbase[] = {
> +int multipliers[] = {
> +void mmc_set_ios(struct mmc *mmc)
> +void mmc_set_clock(struct mmc *mmc, uint clock)
> +void mmc_set_bus_width(struct mmc *mmc, uint width)
> +int mmc_startup(struct mmc *mmc)
>
> and more. All of them are undocumented, so it's not clear how they're
> supposed to be used.
I will increase documentation, and reduce the scope of any identifiers
which aren't meant to be used outside of the file.
>> +#ifdef CONFIG_GENERIC_MMC
>> + WATCHDOG_RESET ();
>> + puts ("MMC: ");
>> + mmc_initialize (bd);
>> +#endif
>
> Why can't mmc_initialize() print the "MMC: " part too?
>
> Better yet, why not define mmc_initialize() as an empty function if
> CONFIG_GENERIC_MMC is not set?
I'll take a look. I copied this pretty closely from eth_initialize(),
so that's the why, but I don't see why I couldn't move the MMC: inside
mmc_initialize. Making it an empty function is fine with me, but
different from how the other functions do it.
>
> Ok, that's all for now. Thanks a lot for taking the effort to do this.
> A common mmc layer is much needed.
Thank you for taking the time to review it!
Andy
More information about the U-Boot
mailing list