* Russell King <[email protected]> [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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
