[U-Boot] [PATCH 06/10] Add MMC Framework

[Haavard Skinnemoen]

"Andy Fleming" <afleming at gmail.com> wrote:
> 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?

I don't think the block ops do. Nor do I think they should -- doing a
full enumeration on every block access sounds a bit excessive.

So, if you do

fatls mmc 0:1
<eject and re-insert card>
fatls mmc 0:1

I suspect the second command will fail.

> >> +
> >> +     /* 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_set_high_speed() or something?

> >
> >> +     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.

Right. How about renaming the existing "caps" field "card_caps" and
adding a new "host_caps" field?

> >> +     /* 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.

I think we need both a host flag indicating high-speed support and a
f_max field indicating the maximum frequency. Even if the host supports
high-speed signaling, there may be internal clock limitations that
prevents it from going all the way up to 52 MHz. Or board routing
issues.

> >> +     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.

Yeah, it's probably fine. I just felt like mentioning it.

> >> +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)

Sounds good.

> > 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.

There could be endianness issues too...but I remember someone pointing
out that the whole structure didn't correspond to some version of the
spec. So I think some of the fields are slightly different from one
spec revision to another.

But I guess the same argument that applied back then applies now -- if
no one uses those fields, no need to worry all that much. But it's
something we should keep in mind.

> 
> >> +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.

I'd prefer it to be an array of u32 so that I won't have to worry about
alignment when copying the response from the 32-bit hardware registers.
But I don't know what others prefer.

> >
> >> +     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.

Union?

> >> +     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.

Sounds good.

> >> +     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.

Well, I was mostly thinking about visual separation, perhaps create two
structs with card- and host-specific data respectively and embed them
into the mmc_device struct so no extra dereferencing will be necessary.
But just grouping them together and adding an empty line between them
would probably work just as well.

Multiple cards can probably wait, I agree.

> >> +#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.

Yeah, I know this is the way it's usually done, and it's been bothering
me for a while. It looks like most of the lib_<arch>/board.c files
contain more preprocessor directives than actual C code, so I just
wanted to suggest a way to reduce the cpp noise a little bit.

But I do understand if you want to stick to the usual way of doing
things.

Haavard