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

[David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>]

On Wednesday 01 August 2007, Pierre Ossman wrote:
> On Thu, 26 Jul 2007 14:58:54 -0700
> David Brownell <david-b@pacbell.net> wrote:
> 
> > > 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,

Since I've not seen the MMC4 spec, I couldn't know.  :)

And the nearest approximation of MMC4 I've got (an early Samsung
spec) says CMD58 has argument "none" (== 0) ... while one of the
cards I've got rejected CMD58 before CMD1.  So "straightforward"
is not a word I could apply here...


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

Right, but that doesn't answer my question:  whether the card
is specified to lock up "forever", or whether (as I'd expect)
reinitialization (starting with reset) works the normal way.


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

My current setup uses a breadboard, so it can hook up to the
real SPI hardware ... I started out using bitbanged SPI to a
card which normally muxed to a "native" controller.  Maybe
one of those could work for you.


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

Doesn't the general disclaimer that MMC4 cards haven't been
tested already cover that part?  If you like, just tweak the
patch comments to clarify.


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

So, move the param and mmc_spi_set_crc() into core.c then, and
leave the calls where they are?

Can we do that as a separate patch?  You said below we can proceed,
which suggests to me that now's the time to convert to incrmentals.
I don't mind a few more update patches, but it'd be easier to do
them against say a git-mmc patch against 2.6.23-rc2 ...


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

I don't have either of those specs.  "Previous command" isn't
particularly clear, though maybe in context it could be ... it
might mean the immediately preceding command, or the one before
that, depending on usage.


> (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*)

Yeech!

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

I don't think you need to be nervous about that.  It clearly works,
and to those of us who've worked this driver and had to become one
with the docs we have, this hasn't raised flags.

It is however *much* better not to need to map the SPI status codes
into the "native" ones.  As you implicitly noted above, expecting
them to behave the same doesn't seem like a good idea!

- Dave


 
> 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