LinuxLists.cc - [GIT PULL] MMC updates

[permalink] [raw]

Subject: Re: [GIT PULL] MMC updates

Pierre Ossman wrote:
> Linus, please pull from
>
> git://git.kernel.org/pub/scm/linux/kernel/git/drzeus/mmc.git for-linus
>

*ping*

--
-- Pierre Ossman

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

2007-05-05 04:46:03

by Linus Torvalds

[permalink] [raw]

Subject: Re: [GIT PULL] MMC updates

On Sat, 5 May 2007, Pierre Ossman wrote:

> Pierre Ossman wrote:
> > Linus, please pull from
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/drzeus/mmc.git for-linus
> >
>
> *ping*

*pong*.

Thanks for reminding me. I was away for a couple of days, missed some
emails, just pulled and pushed out.

Linus

2007-05-09 18:56:58

[permalink] [raw]

Subject: Re: [GIT PULL] MMC updates

On Tue, May 01, 2007 at 05:22:00PM +0200, Pierre Ossman wrote:
> Linus, please pull from
>
> git://git.kernel.org/pub/scm/linux/kernel/git/drzeus/mmc.git for-linus
>
> to receive the following updates:

Dug out from the ARM kautobuild...

drivers/mmc/host/pxamci.c: In function 'pxamci_cmd_done':
drivers/mmc/host/pxamci.c:236: error: 'MMC_ALL_SEND_CID' undeclared (first use in this function)
drivers/mmc/host/pxamci.c:236: error: (Each undeclared identifier is reported only once
drivers/mmc/host/pxamci.c:236: error: for each function it appears in.)
drivers/mmc/host/pxamci.c:237: error: 'MMC_SEND_CSD' undeclared (first use in this function)
drivers/mmc/host/pxamci.c:238: error: 'MMC_SEND_CID' undeclared (first use in this function)

--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:

2007-05-09 19:07:31

[permalink] [raw]

Subject: Re: [GIT PULL] MMC updates

Russell King wrote:
> Dug out from the ARM kautobuild...
>
> drivers/mmc/host/pxamci.c: In function 'pxamci_cmd_done':
> drivers/mmc/host/pxamci.c:236: error: 'MMC_ALL_SEND_CID' undeclared (first use in this function)
> drivers/mmc/host/pxamci.c:236: error: (Each undeclared identifier is reported only once
> drivers/mmc/host/pxamci.c:236: error: for each function it appears in.)
> drivers/mmc/host/pxamci.c:237: error: 'MMC_SEND_CSD' undeclared (first use in this function)
> drivers/mmc/host/pxamci.c:238: error: 'MMC_SEND_CID' undeclared (first use in this function)
>
>

What are opcode defines doing in the driver?

--
-- Pierre Ossman

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

2007-05-09 22:13:16

[permalink] [raw]

Subject: Re: [GIT PULL] MMC updates

On Wed, May 09, 2007 at 09:06:29PM +0200, Pierre Ossman wrote:
> Russell King wrote:
> > Dug out from the ARM kautobuild...
> >
> > drivers/mmc/host/pxamci.c: In function 'pxamci_cmd_done':
> > drivers/mmc/host/pxamci.c:236: error: 'MMC_ALL_SEND_CID' undeclared (first use in this function)
> > drivers/mmc/host/pxamci.c:236: error: (Each undeclared identifier is reported only once
> > drivers/mmc/host/pxamci.c:236: error: for each function it appears in.)
> > drivers/mmc/host/pxamci.c:237: error: 'MMC_SEND_CSD' undeclared (first use in this function)
> > drivers/mmc/host/pxamci.c:238: error: 'MMC_SEND_CID' undeclared (first use in this function)
> >
> >
>
> What are opcode defines doing in the driver?

See the comments immediately above and below its use.

Welcome to buggy hardware.

--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:

2007-05-10 05:45:00

[permalink] [raw]

Subject: Re: [GIT PULL] MMC updates

Russell King wrote:
> See the comments immediately above and below its use.
>
> Welcome to buggy hardware.
>
>

I've read through the erratum, and to me it seems like the bug affects
all long replies, not just these codes. So I think the code should be
fixed to look at the response flag, not the opcode.

Do you have hardware so that you can test such a change?

--
-- Pierre Ossman

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

2007-05-10 07:51:50

[permalink] [raw]

Subject: Re: [GIT PULL] MMC updates

On Thu, May 10, 2007 at 07:44:02AM +0200, Pierre Ossman wrote:
> Russell King wrote:
> > See the comments immediately above and below its use.
> >
> > Welcome to buggy hardware.
>
> I've read through the erratum, and to me it seems like the bug affects
> all long replies, not just these codes. So I think the code should be
> fixed to look at the response flag, not the opcode.
>
> Do you have hardware so that you can test such a change?

Nope. Suggest checking the git logs and contacting those who authored
the change.

--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:

2007-05-10 14:01:50

[permalink] [raw]

Subject: Re: [GIT PULL] MMC updates

Hi Nicolas,

You seem to be the source of this workaround, and also the maintainer of
PXA. So I guess this falls into your lap either way. Highlights from my
discussion with Russell:

Pierre Ossman wrote:
> Russell King wrote:
>
>> > Dug out from the ARM kautobuild...
>> >
>> > drivers/mmc/host/pxamci.c: In function 'pxamci_cmd_done':
>> > drivers/mmc/host/pxamci.c:236: error: 'MMC_ALL_SEND_CID' undeclared (first use in this function)
>> > drivers/mmc/host/pxamci.c:236: error: (Each undeclared identifier is reported only once
>> > drivers/mmc/host/pxamci.c:236: error: for each function it appears in.)
>> > drivers/mmc/host/pxamci.c:237: error: 'MMC_SEND_CSD' undeclared (first use in this function)
>> > drivers/mmc/host/pxamci.c:238: error: 'MMC_SEND_CID' undeclared (first use in this function)
>> >
>> >
>>
>
> What are opcode defines doing in the driver?

Pierre Ossman wrote:
> Russell King wrote:
>
>> See the comments immediately above and below its use.
>>
>> Welcome to buggy hardware.
>>
>>
>>
>
> I've read through the erratum, and to me it seems like the bug affects
> all long replies, not just these codes. So I think the code should be
> fixed to look at the response flag, not the opcode.
>
> Do you have hardware so that you can test such a change?
>
>

I guess the same question goes to you. :)

Rgds

--
-- Pierre Ossman

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

2007-05-10 14:52:54

[permalink] [raw]

Subject: Re: [GIT PULL] MMC updates

On Thu, 10 May 2007, Pierre Ossman wrote:

> You seem to be the source of this workaround, and also the maintainer of
> PXA.

Well... I used to.

But the only MMC capable PXA hardware in working conditions I have
access to at the moment is PXA255 based which doesn't suffer from this
erratum.

> Pierre Ossman wrote:
> > Russell King wrote:
> >
> >> See the comments immediately above and below its use.
> >>
> >> Welcome to buggy hardware.
> >>
> >>
> >>
> >
> > I've read through the erratum, and to me it seems like the bug affects
> > all long replies, not just these codes. So I think the code should be
> > fixed to look at the response flag, not the opcode.
> >
> > Do you have hardware so that you can test such a change?
> >
> >
>
> I guess the same question goes to you. :)

People in better position than I currently do to test a fix are most
likely to be found on lak.

Nicolas

2007-05-12 15:38:47

[permalink] [raw]

Subject: Re: [GIT PULL] MMC updates

On Thu, 10 May 2007, Nicolas Pitre wrote:

> On Thu, 10 May 2007, Pierre Ossman wrote:
>
> > You seem to be the source of this workaround, and also the maintainer of
> > PXA.
>
> Well... I used to.

Actually, I'm not the author of this workaround. And looking at it
closer, the current workaround is utterly buggy as it completely inhibit
CRC error reporting for everything but the listed commands when the MSB
of the response is a zero.

> But the only MMC capable PXA hardware in working conditions I have
> access to at the moment is PXA255 based which doesn't suffer from this
> erratum.
>
> > Pierre Ossman wrote:
> > > I've read through the erratum, and to me it seems like the bug affects
> > > all long replies, not just these codes. So I think the code should be
> > > fixed to look at the response flag, not the opcode.

Indeed.

Please apply the following patch. It is compile tested only as I don't
have PXA27x hardware with MMC at the moment, but it just cannot be worse
than the current code even when it was compiling.

----- >8
Subject: fix PXA27x MMC workaround for bad CRC on R2 response erratum

... and make it depend on the response flag rather than the command type.

Signed-off-by: Nicolas Pitre <[email protected]>
---
diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c
index d97d386..8240609 100644
--- a/drivers/mmc/host/pxamci.c
+++ b/drivers/mmc/host/pxamci.c
@@ -232,20 +232,15 @@ static int pxamci_cmd_done(struct pxamci_host *host, unsigned int stat)
/*
* workaround for erratum #42:
* Intel PXA27x Family Processor Specification Update Rev 001
+ * A bogus CRC error can appear if the msb of a R2 response
+ * is a one.
*/
- if (cmd->opcode == MMC_ALL_SEND_CID ||
- cmd->opcode == MMC_SEND_CSD ||
- cmd->opcode == MMC_SEND_CID) {
- /* a bogus CRC error can appear if the msb of
- the 15 byte response is a one */
- if ((cmd->resp[0] & 0x80000000) == 0)
- cmd->error = MMC_ERR_BADCRC;
- } else {
- pr_debug("ignoring CRC from command %d - *risky*\n",cmd->opcode);
- }
-#else
- cmd->error = MMC_ERR_BADCRC;
+ if (RSP_TYPE(mmc_resp_type(cmd)) == RSP_TYPE(MMC_RSP_R2) &&
+ (cmd->resp[0] & 0x80000000)) {
+ pr_debug("ignoring CRC from command %d - *risky*\n", cmd->opcode);
+ } else
#endif
+ cmd->error = MMC_ERR_BADCRC;
}

pxamci_disable_irq(host, END_CMD_RES);

2007-05-12 16:13:18

[permalink] [raw]

Subject: Re: [GIT PULL] MMC updates

Nicolas Pitre wrote:
> Actually, I'm not the author of this workaround. And looking at it
> closer, the current workaround is utterly buggy as it completely inhibit
> CRC error reporting for everything but the listed commands when the MSB
> of the response is a zero.
>
>

Your name popped up on the commit for this, but as that was during the
bk days the information was severely lacking.

> Please apply the following patch. It is compile tested only as I don't
> have PXA27x hardware with MMC at the moment, but it just cannot be worse
> than the current code even when it was compiling.
>
>

I would think that it would be better to look at just MMC_RSP_136 and
MMC_RSP_CRC in case we get future variations.

Rgds

--
-- Pierre Ossman

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

2007-05-12 16:23:11

[permalink] [raw]

Subject: Re: [GIT PULL] MMC updates

On Sat, May 12, 2007 at 06:12:27PM +0200, Pierre Ossman wrote:
> Nicolas Pitre wrote:
> > Actually, I'm not the author of this workaround. And looking at it
> > closer, the current workaround is utterly buggy as it completely inhibit
> > CRC error reporting for everything but the listed commands when the MSB
> > of the response is a zero.
> >
> >
>
> Your name popped up on the commit for this, but as that was during the
> bk days the information was severely lacking.

That's not correct. Just as today, it's entirely trackable. We've had
the sign-off thing for quite a long time, and we've also had the ARM
patch system for ages. That's what the "2271/3" number means.

http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=2271%2F1
http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=2271%2F3

First submitted on 22 November 2004 by Nicolas. Went through a couple
of revisions until 2271/3 which was committed on 27 November 2004. No
indication that the code was done by anyone other than Nicolas, though
maybe Nico didn't add appropriate creditation to the real authors.
Slap Wrist if we're missing the proper creditation!

--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:

2007-05-12 17:55:30

[permalink] [raw]

Subject: Re: [GIT PULL] MMC updates

On Sat, 12 May 2007, Russell King wrote:

> First submitted on 22 November 2004 by Nicolas. Went through a couple
> of revisions until 2271/3 which was committed on 27 November 2004. No
> indication that the code was done by anyone other than Nicolas, though
> maybe Nico didn't add appropriate creditation to the real authors.
> Slap Wrist if we're missing the proper creditation!

Whatever happened, I certainly knew nothing about MMC back then, and I
did a less than appropriate job at reviewing the patch.

I'm getting far more involved with MMC/SD/SDIO now so this issue will
get properly resolved.

Nicolas

2007-05-13 03:13:47

[permalink] [raw]

Subject: [PATCH] fix PXA27x MMC workaround for bad CRC with 136 bit response

... and make it depend on the response flag instead of the command type.

Signed-off-by: Nicolas Pitre <[email protected]>
---

On Sat, 12 May 2007, Pierre Ossman wrote:
> Nicolas Pitre wrote:
> > Please apply the following patch. It is compile tested only as I don't
> > have PXA27x hardware with MMC at the moment, but it just cannot be worse
> > than the current code even when it was compiling.
> >
> >
>
> I would think that it would be better to look at just MMC_RSP_136 and
> MMC_RSP_CRC in case we get future variations.

Makes sense.

diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c
index d97d386..f8985c5 100644
--- a/drivers/mmc/host/pxamci.c
+++ b/drivers/mmc/host/pxamci.c
@@ -232,20 +232,14 @@ static int pxamci_cmd_done(struct pxamci_host *host, unsigned int stat)
/*
* workaround for erratum #42:
* Intel PXA27x Family Processor Specification Update Rev 001
+ * A bogus CRC error can appear if the msb of a 136 bit
+ * response is a one.
*/
- if (cmd->opcode == MMC_ALL_SEND_CID ||
- cmd->opcode == MMC_SEND_CSD ||
- cmd->opcode == MMC_SEND_CID) {
- /* a bogus CRC error can appear if the msb of
- the 15 byte response is a one */
- if ((cmd->resp[0] & 0x80000000) == 0)
- cmd->error = MMC_ERR_BADCRC;
- } else {
- pr_debug("ignoring CRC from command %d - *risky*\n",cmd->opcode);
- }
-#else
- cmd->error = MMC_ERR_BADCRC;
+ if (cmd->flags & MMC_RSP_136 && cmd->resp[0] & 0x80000000) {
+ pr_debug("ignoring CRC from command %d - *risky*\n", cmd->opcode);
+ } else
#endif
+ cmd->error = MMC_ERR_BADCRC;
}

pxamci_disable_irq(host, END_CMD_RES);

2007-05-13 16:00:38