Hi David,
I just tried out mmc_spi on Linux HEAD and it no longer works. :/
It seems to be caused by changes in the SPI core on how to handled bus
sharing. All I'm getting in dmesg is this:
[17179577.832000] mmc_spi spi1.0: can't share SPI bus
[17179577.836000] mmc_spi: probe of spi1.0 failed with error -31
Ideas? This platform worked fine with 2.6.23-rc*
Rgds
--
-- Pierre Ossman
Linux kernel, MMC maintainer http://www.kernel.org
PulseAudio, core developer http://pulseaudio.org
rdesktop, core developer http://www.rdesktop.org
> I just tried out mmc_spi on Linux HEAD and it no longer works. :/
I noticed that too. I've got a patch for one bug (appended), and
will be looking at a fix for the second.
> It seems to be caused by changes in the SPI core on how to handled bus
> sharing. All I'm getting in dmesg is this:
>
> [17179577.832000] mmc_spi spi1.0: can't share SPI bus
> [17179577.836000] mmc_spi: probe of spi1.0 failed with error -31
That issue is 49dce689ad4ef0fd1f970ef762168e4bd46f69a3, the
classdev-elimination patch from Tony Jones. It broke the
"does this bus have more than one device" test by relocating
the relevant sysfs nodes.
Quick workaround for that one is to disable the fault return
after that test.
- Dave
========== CUT HERE
When enumerating MMC cards using SPI, don't support the "just probe"
mechanism since it doesn't always work. Instead, always wait for the
reset to complete before issuing the next request.
This is a regression ... this SanDisk MMC card used to enumerate with
no trouble, despite this particular spec violation.
Signed-off-by: David Brownell <[email protected]>
--- a/drivers/mmc/core/mmc_ops.c 2007-10-13 17:44:43.000000000 -0700
+++ b/drivers/mmc/core/mmc_ops.c 2007-10-16 18:54:38.000000000 -0700
@@ -115,8 +115,11 @@ int mmc_send_op_cond(struct mmc_host *ho
if (err)
break;
- /* if we're just probing, do a single pass */
- if (ocr == 0)
+ /* if we're just probing, do a single pass ... except,
+ * accomodate cards which don't behave right until a
+ * SPI reset completes.
+ */
+ if (ocr == 0 && !mmc_host_is_spi(host))
break;
/* otherwise wait until reset completes */
On Wed, 17 Oct 2007 12:37:13 -0700
David Brownell <[email protected]> wrote:
>
> That issue is 49dce689ad4ef0fd1f970ef762168e4bd46f69a3, the
> classdev-elimination patch from Tony Jones. It broke the
> "does this bus have more than one device" test by relocating
> the relevant sysfs nodes.
>
> Quick workaround for that one is to disable the fault return
> after that test.
>
Annoying. Feel free to ping me when you've pushed fixes to Linus.
> When enumerating MMC cards using SPI, don't support the "just probe"
> mechanism since it doesn't always work. Instead, always wait for the
> reset to complete before issuing the next request.
>
> This is a regression ... this SanDisk MMC card used to enumerate with
> no trouble, despite this particular spec violation.
>
> Signed-off-by: David Brownell <[email protected]>
>
I think I'll have to NAK this as I believe it breaks MMC 4.2. I'll check
and get back 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
On Wed, 17 Oct 2007 12:37:13 -0700
David Brownell <[email protected]> wrote:
> When enumerating MMC cards using SPI, don't support the "just probe"
> mechanism since it doesn't always work. Instead, always wait for the
> reset to complete before issuing the next request.
>
> This is a regression ... this SanDisk MMC card used to enumerate with
> no trouble, despite this particular spec violation.
>
I've been testing a bit more here, and I can't get this particular bug. I have others though:
Out of my five MMC cards, only two work properly. One doesn't respond to SPI commands at all, and two hang on a second CMD0 and return ILLEGAL_COMMAND on a second run of CMD1. All of this is of course wildly out of spec. SPI seems to be a bonus, not a given. :/
As for your card, could you send me a dump as I'm unable to produce the issue here?
Rgds
--
-- Pierre Ossman
Linux kernel, MMC maintainer http://www.kernel.org
PulseAudio, core developer http://pulseaudio.org
rdesktop, core developer http://www.rdesktop.org
On Wednesday 17 October 2007, Pierre Ossman wrote:
>
> On Wed, 17 Oct 2007 12:37:13 -0700
> David Brownell <[email protected]> wrote:
> > That issue is 49dce689ad4ef0fd1f970ef762168e4bd46f69a3, the
> > classdev-elimination patch from Tony Jones. ?It broke the
> > "does this bus have more than one device" test by relocating
> > the relevant sysfs nodes.
> >
> > Quick workaround for that one is to disable the fault return
> > after that test.
> >
>
> Annoying. Feel free to ping me when you've pushed fixes to Linus.
This is another case where whacking around in sysfs seems
unavoidable, even when it breaks things. The patch below
works around that that change. It seems like something that
should go through you, not directly through Linus...
- Dave
================ CUT HERE
Fix mmc-over-spi regression
Patch 49dce689ad4ef0fd1f970ef762168e4bd46f69a3 changed the sysfs data
structures for SPI in a way which broke the MMC-over-SPI host driver.
This patch fixes that regression by changing the scheme used to keep
from knowingly trying to use a shared bus segment, and updates the
adjacent comments slightly to better explain the issue.
Signed-off-by: David Brownell <[email protected]>
--- a/drivers/mmc/host/mmc_spi.c 2007-10-23 21:24:41.000000000 -0700
+++ b/drivers/mmc/host/mmc_spi.c 2007-10-24 14:46:59.000000000 -0700
@@ -1165,6 +1165,23 @@ mmc_spi_detect_irq(int irq, void *mmc)
return IRQ_HANDLED;
}
+struct count_children {
+ unsigned n;
+ struct bus_type *bus;
+};
+
+static int maybe_count_child(struct device *dev, void *c)
+{
+ struct count_children *ccp = c;
+
+ if (dev->bus == ccp->bus) {
+ if (ccp->n)
+ return -EBUSY;
+ ccp->n++;
+ }
+ return 0;
+}
+
static int mmc_spi_probe(struct spi_device *spi)
{
void *ones;
@@ -1188,33 +1205,30 @@ static int mmc_spi_probe(struct spi_devi
return status;
}
- /* We can use the bus safely iff nobody else will interfere with
- * us. That is, either we have the experimental exclusive access
- * primitives ... or else there's nobody to share it with.
+ /* We can use the bus safely iff nobody else will interfere with us.
+ * Most commands consist of one SPI message to issue a command, then
+ * several more to collect its response, then possibly more for data
+ * transfer. Clocking access to other devices during that period will
+ * corrupt the command execution.
+ *
+ * Until we have software primitives which guarantee non-interference,
+ * we'll aim for a hardware-level guarantee.
+ *
+ * REVISIT we can't guarantee another device won't be added later...
*/
if (spi->master->num_chipselect > 1) {
- struct device *parent = spi->dev.parent;
+ struct count_children cc;
- /* If there are multiple devices on this bus, we
- * can't proceed.
- */
- spin_lock(&parent->klist_children.k_lock);
- if (parent->klist_children.k_list.next
- != parent->klist_children.k_list.prev)
- status = -EMLINK;
- else
- status = 0;
- spin_unlock(&parent->klist_children.k_lock);
+ cc.n = 0;
+ cc.bus = spi->dev.bus;
+ status = device_for_each_child(spi->dev.parent, &cc,
+ maybe_count_child);
if (status < 0) {
dev_err(&spi->dev, "can't share SPI bus\n");
return status;
}
- /* REVISIT we can't guarantee another device won't
- * be added later. It's uncommon though ... for now,
- * work as if this is safe.
- */
- dev_warn(&spi->dev, "ASSUMING unshared SPI bus!\n");
+ dev_warn(&spi->dev, "ASSUMING SPI bus stays unshared!\n");
}
/* We need a supply of ones to transmit. This is the only time
On Monday 22 October 2007, Pierre Ossman wrote:
> On Wed, 17 Oct 2007 12:37:13 -0700
> David Brownell <[email protected]> wrote:
>
> > When enumerating MMC cards using SPI, don't support the "just probe"
> > mechanism since it doesn't always work. Instead, always wait for the
> > reset to complete before issuing the next request.
> >
> > This is a regression ... this SanDisk MMC card used to enumerate with
> > no trouble, despite this particular spec violation.
> >
>
> I've been testing a bit more here, and I can't get this particular bug.
It's not just a bug, it's a regression ... a new bug which
was introduced by some patch. ;)
> I have others though:
>
> Out of my five MMC cards, only two work properly. One doesn't respond
> to SPI commands at all, and two hang on a second CMD0 and return
> ILLEGAL_COMMAND on a second run of CMD1. All of this is of course wildly
> out of spec. SPI seems to be a bonus, not a given. :/
SPI works fine on all the MMC and SD cards I've got here,
other than the minor glitch fixed by the "don't just probe"
patch I sent.
As you know, to be spec-conformant they *must* support SPI.
Agreed, there are too many vendors who don't appear to value
following specs. If those cards which are using the MMC or
SD trade marks, you might notify the relevant consortium and
asking them to fix those vendors. ;)
> As for your card, could you send me a dump as I'm unable to produce
> the issue here?
I'm not sure what you mean by "dump", but appended the sysfs attributes
produced after I disabled that new "just probe" mechanism.
- Dave
This is a dump of the /sys/class/mmc_host/mmc0/mmc0:0001 file attributes
(except the card serial number) for a SanDisk MMC card which doesn't like
SPI requests (like reading OCR) before the reset finishes.
This card _used_ to enumerate with the MMC-over-SPI stack, but
a recent change in the MMC core now prevents it from doing that...
=== cid
00000000 30 30 30 30 30 32 35 33 34 34 34 64 34 32 32 64 |00000253444d422d|
00000010 33 31 33 36 33 32 61 39 31 66 31 30 38 34 33 31 |313632a91f108431|
00000020 0a |.|
00000021
=== csd
00000000 34 34 32 36 30 30 32 61 31 66 66 39 38 33 64 33 |4426002a1ff983d3|
00000010 65 34 62 34 38 33 66 66 31 32 34 30 34 30 38 31 |e4b483ff12404081|
00000020 0a |.|
00000021
=== date
00000000 30 38 2f 32 30 30 31 0a |08/2001.|
00000008
=== fwrev
00000000 30 78 32 0a |0x2.|
00000004
=== hwrev
00000000 30 78 33 0a |0x3.|
00000004
=== manfid
00000000 30 78 30 30 30 30 30 32 0a |0x000002.|
00000009
=== name
00000000 53 44 4d 42 2d 31 36 0a |SDMB-16.|
00000008
=== oemid
00000000 30 78 30 30 30 30 0a |0x0000.|
00000007
=== type
00000000 4d 4d 43 0a |MMC.|
00000004
On Wed, 24 Oct 2007 15:37:10 -0700
David Brownell <[email protected]> wrote:
> On Monday 22 October 2007, Pierre Ossman wrote:
> >
> > I've been testing a bit more here, and I can't get this particular bug.
>
> It's not just a bug, it's a regression ... a new bug which
> was introduced by some patch. ;)
>
It's a bug in the card, which is what I was referring to. ;)
>
> SPI works fine on all the MMC and SD cards I've got here,
> other than the minor glitch fixed by the "don't just probe"
> patch I sent.
>
Lucky you. :)
> As you know, to be spec-conformant they *must* support SPI.
I never recall which of the different specs require SPI (not all do).
> Agreed, there are too many vendors who don't appear to value
> following specs. If those cards which are using the MMC or
> SD trade marks, you might notify the relevant consortium and
> asking them to fix those vendors. ;)
>
Hah! The SD and MMC boys seem to have no interest in people following the specs, considering all the shite that is out there.
>
> > As for your card, could you send me a dump as I'm unable to produce
> > the issue here?
>
> I'm not sure what you mean by "dump", but appended the sysfs attributes
> produced after I disabled that new "just probe" mechanism.
>
A dmesg with MMC_DEBUG so that I can see just how the card misbehaves.
Also, I see you're dying for my decodecid and decodecsd progs. ;)
I've included both.
Rgds
--
-- Pierre Ossman
Linux kernel, MMC maintainer http://www.kernel.org
PulseAudio, core developer http://pulseaudio.org
rdesktop, core developer http://www.rdesktop.org
On Wed, 24 Oct 2007 15:27:46 -0700
David Brownell <[email protected]> wrote:
>
> This is another case where whacking around in sysfs seems
> unavoidable, even when it breaks things. The patch below
> works around that that change. It seems like something that
> should go through you, not directly through Linus...
>
Indeed. Applied.
Rgds
--
-- Pierre Ossman
Linux kernel, MMC maintainer http://www.kernel.org
PulseAudio, core developer http://pulseaudio.org
rdesktop, core developer http://www.rdesktop.org