Re: [patch 2.6.22-git5 3/4] MMC core learns about SPI

[Pierre Ossman <drzeus-mmc-p3sGCRWkH8CeZLLa646FqQ@public.gmane.org>]

On Thu, 26 Jul 2007 14:58:54 -0700
David Brownell <david-b@pacbell.net> wrote:

> 
> Yes we could ... but this way is IMO simpler, since it retains
> a simple invariant:  that the OCR parameter to attach() methods
> is always valid.  As one would expect!  Which gives another
> advantage:  no surprises.
> 

Fair enough. I'm not that attached to it.

> > 
> > This will fail to initialise a high capacity MMC card. From the MMC
> > spec:
> > 
> > "Without the CMD58 with bits [30:29] set as "10b" in prior to the
> > CMD1 a higher than 2GB of density of memory will remain in Idle
> > state forever." 
> 
> The first thing mmc_attach_mmc() does is reset the card...
> 
> Are you sure that "forever" means that reset will fail?
> That would be a very foolish thing to specify.  And while
> I've seen strange things in specs, that would be one of
> the worst that's not a vendor-specific part of a papering
> over errata ...
> 

Well, it's rather straight-forward if you think about it. As they've
changed the addressing scheme, but retain the original commands, a host
that does not understand the new scheme would utterly destroy the data.
So the card refuses to init unless the host can present some evidence
that it understands the new scheme.

> I'd be inclined to leave this alone unless someone gets
> such a card and notices that it doesn't work.  After all,
> lack of testing on those new (and-I-still-can't-find-one)
> cards is a general disclaimer.
> 

Too bad I have no SPI setup here. :(

Please add a comment that we know that MMC 4.2 is broken and can be
fixed once someone is able to test.

> > 
> > The *_ops.c only contain function wrappers of protocol commands, not
> > policy. So I think this is better placed where mmc_spi_set_crc() is
> > called. 
> 
> You mean, have separate module params for SD and for MMC?
> It seems cleaner this way...
> 

No, that would be silly. You'd probably have to stick it in core.c and
reference it from mmc.c and sd.c.

The reason I complain is that seeing mmc_spi_set_crc() will look like
it always activates CRC, as all other functions in *_ops.c are simple
wrappers around protocol ops.

> > 
> > "Illegal command" refers to the previous command sent (and failed).
> > So this can give false negatives. 
> 
> The immediately preceding command would be MMC_APP_CMD ... ?
> I didn't see any language in the specs which could let me
> interpret this as anything other than APP_CMD failure:
> 
>    When an error bit is detected in “R” mode the card will
>    report the error in the response to the command that raised
>    the exception. The command will not be executed and the
>    associated state transition will not take place. 
> 
> What this does is to quickly abort retries of SD card ops
> that were sent to MMC cards, and which always fail with
> ILLEGAL_COMMAND.
> 

I hate this part of the spec. The SD spec also claims:

"Clear condition B: Always related to the previous command"

But in the SPI section, they back out of that claim. So for SPI, things
seems somewhat sane.

(The MMC spec is even more hilarious. I lists clear condition A and B,
but never explains what they mean. And looking at some older card
specs, it seems that the MMC spec had the same conditions and
definitions as SD *sigh*)

We can proceed with your code, although I'm still a bit nervous about
sticking that parsing in there.

Rgds
-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/
_______________________________________________
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general