lkml.org 
[lkml]   [2006]   [Feb]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [patch 1/5] MMC OMAP driver
    * Russell King <rmk+lkml@arm.linux.org.uk> [060201 04:44]:
    > On Tue, Jan 31, 2006 at 09:34:08AM -0400, Anderson Briglia wrote:
    > > + /* Any data transfer means adtc type (but that information is not
    > > + * in command structure, so we flagged it into host struct.)
    > > + * However, telling bc, bcr and ac apart based on response is
    > > + * not foolproof:
    > > + * CMD0 = bc = resp0 CMD15 = ac = resp0
    > > + * CMD2 = bcr = resp2 CMD10 = ac = resp2
    > > + *
    > > + * Resolve to best guess with some exception testing:
    > > + * resp0 -> bc, except CMD15 = ac
    > > + * rest are ac, except if opendrain
    > > + */
    > > + if (host->data) {
    > > + cmdtype = OMAP_MMC_CMDTYPE_ADTC;
    > > + } else if (resptype == 0 && cmd->opcode != 15) {
    > > + cmdtype = OMAP_MMC_CMDTYPE_BC;
    > > + } else if (host->bus_mode == MMC_BUSMODE_OPENDRAIN) {
    > > + cmdtype = OMAP_MMC_CMDTYPE_BCR;
    > > + } else {
    > > + cmdtype = OMAP_MMC_CMDTYPE_AC;
    > > + }
    >
    > The 4 command types are decodable from the information provided.
    >
    > bc - broadcast commands without response
    > bcr - broadcast commands with response
    > ac - addressed command
    > adtc - addressed data transfer command
    >
    > From this, there are three bits of information required:
    >
    > 1. is the command addressed or broadcast (bc/bcr vs ac/adtc)?
    > 2. if broadcast, does it have a response (bc vs bcr)?
    > (mrq->cmd->flags & MMC_RSP_MASK) != MMC_RSP_NONE
    > satisfies this by definition.
    > 3. if addressed, does it have a data transfer (ac vs adtc)?
    > mrq->data != NULL
    > satisfies this by definition.
    >
    > Hence, to allow host drivers to decode the command type, we only need
    > to supply information on whether this was a broadcast or addressed
    > command. This could be a flag MMC_CMD_BROADCAST, which is passed
    > with the command.
    >
    > I'm thinking we want a couple of helper functions:
    >
    > mmc_resp_type(cmd)
    > mmc_cmd_type(cmd)
    >
    > which would allow the flags value to be tested for response and command
    > types - in which case we'd need to also have MMC_CMD_DATA as well. For
    > an example of what I'm proposing, see the end of this message.

    That sounds like a good solution.

    > Wouldn't this be a problem which isn't host specific? If so, why should
    > the fix be limited to just omap hosts? IOW: it's the wrong layer to fix
    > this problem.
    >
    > > +static inline int is_broken_card(struct mmc_card *card)
    > > +{
    > > + int i;
    > > + struct mmc_cid *c = &card->cid;
    > > + static const struct broken_card_cid {
    > > + unsigned int manfid;
    > > + char prod_name[8];
    > > + unsigned char hwrev;
    > > + unsigned char fwrev;
    > > + } broken_cards[] = {
    > > + { 0x00150000, "\x30\x30\x30\x30\x30\x30\x15\x00", 0x06, 0x03 },
    > > + };
    > > +
    > > + for (i = 0; i < sizeof(broken_cards)/sizeof(broken_cards[0]); i++) {
    > > + const struct broken_card_cid *b = broken_cards + i;
    > > +
    > > + if (b->manfid != c->manfid)
    > > + continue;
    > > + if (memcmp(b->prod_name, c->prod_name, sizeof(b->prod_name)) != 0)
    > > + continue;
    > > + if (b->hwrev != c->hwrev || b->fwrev != c->fwrev)
    > > + continue;
    > > + return 1;
    > > + }
    > > + return 0;
    > > +}
    >
    > I've already mentioned this to the OMAP folk... What problem is this
    > trying to work around? If it's a card problem, it's at the wrong
    > level. If it's a problem with the host not waiting the mandatory
    > 80 cycles before starting a command, that could be the upper layers
    > or a host problem.
    >
    > Either way, the right place to fix this is _not_ in the request
    > function but in the set_ios function. The request function does
    > not know if the card has just been powered up.

    Anderson, can you pull out the broken card check from omap.c, and put
    it into a separate patch? Let's fix the omap.c issues first, and have
    that integrated. Then we can start working on the additional patches
    and test them one at a time.

    > You've already told the MMC layer that your minimum clock is 400000
    > via mmc->f_min. Therefore, you won't be offered a clock rate lower
    > than this, so this test is superfluous.
    >
    > > + host->bus_mode = ios->bus_mode;
    > > + if (omap_has_menelaus()) {
    > > + if (host->bus_mode == MMC_BUSMODE_OPENDRAIN)
    > > + menelaus_mmc_opendrain(1);
    > > + else
    > > + menelaus_mmc_opendrain(0);
    > > + }
    > > + host->hw_bus_mode = host->bus_mode;
    >
    > Since this is the only place where the bus mode is changed, why do you
    > have similar code elsewhere in this driver?

    Hmm, the other bus mode toggle could be unnecessary. Needs to be
    verified on a board with a Menelaus chip.

    > > + if (cpu_is_omap24xx()) {
    > > + host->iclk = clk_get(&pdev->dev, "mmc_ick");
    > > + if (IS_ERR(host->iclk))
    > > + goto out;
    > > + clk_enable(host->iclk);
    > > + }
    > > +
    > > + if (!cpu_is_omap24xx())
    > > + host->fclk = clk_get(&pdev->dev,
    > > + (host->id == 1) ? "mmc1_ck" : "mmc2_ck");
    > > + else
    > > + host->fclk = clk_get(&pdev->dev, "mmc_fck");
    >
    > Wrong use of the clock API:
    >
    > * clk_get - lookup and obtain a reference to a clock producer.
    > * @dev: device for clock "consumer"
    > * @id: clock comsumer ID
    >
    > The ID string is supposed to be the consumer ID, not the producer ID.
    > If you have multiple devices of the same type, this string is supposed
    > to be a constant, and you're supposed to use the struct device to
    > work out which producer is required.

    I'll fix this in the omap clock framework, but probably won't get to it
    until next week at some point.

    Regards,

    Tony
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2006-02-01 20:50    [W:0.036 / U:61.912 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site