Re: [Qemu-devel] [PATCH arm-devs v1 1/5] sd/sd.c: Fix "inquiry" ACMD41

[Peter Crosthwaite]

Hi Igor,

On Thu, May 23, 2013 at 8:31 PM, Igor Mitsyanko <address@hidden> wrote:
> On 05/23/2013 03:42 AM, Peter Crosthwaite wrote:
>> Hi Igor,
>>
>> On Wed, May 22, 2013 at 11:37 PM, Igor Mitsyanko <address@hidden> wrote:
>>>
>>> On 05/21/2013 10:50 AM, address@hidden wrote:
>>>
>>> From: Peter Crosthwaite <address@hidden>
>>>
>>> the SD command ACMD41 can be used in a read only mode to query device
>>> state without doing the SD card initialisation. This is valid even
>>> which the device is already initialised. Fix the command to be
>>> responsive when in the ready state accordingly.
>>>
>>> Signed-off-by: Peter Crosthwaite <address@hidden>
>>> ---
>>>
>>>   hw/sd/sd.c | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>>> index 2e0ef3e..89bfb7a 100644
>>> --- a/hw/sd/sd.c
>>> +++ b/hw/sd/sd.c
>>> @@ -1277,6 +1277,7 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
>>>           }
>>>           switch (sd->state) {
>>>           case sd_idle_state:
>>> +        case sd_ready_state:
>>>               /* We accept any voltage.  10000 V is nothing.  */
>>>               if (req.arg)
>>>                   sd->state = sd_ready_state;
>>>
>>>
>>> I couldn't find any info in SD specification that would confirm this change
>>> correctness, what about
>>> table "Table 4-29: Card State Transition Table" which states that ACMD41 is
>>> illegal in "ready" state?
>>>
>>
>> By the letter of the spec I think you are right. Although this patch
>> is needed to make my QEMU consistent with my real hardware. I'll dig
>> deeper.
>>
>
> Hello, Peter, after thinking some more about this, I assume that table
> 4-29 might be incorrect. It depends on when idle->ready state transition
> occurs, its not clear from specification.
>
> Controller issues first ACMD41 to start card's initialisation. Spec
> states that this process could take up to 1sec, and all this time
> controller should query card's busy state in a loop with ACMD41. After
> response to ACMD41 has busy flag deasserted, card is considered to be
> "ready". But this means that card was already in ready state when it
> received last ACMD41 command, right? Unless card transitions to ready
> state only after a response to last ACMD41 was sent.
>

This is exactly how it works. I did some experiments with a hacked up
linux driver:

--- a/drivers/mmc/core/sd_ops.c
+++ b/drivers/mmc/core/sd_ops.c
@@ -161,7 +161,9 @@ int mmc_send_app_op_cond(struct mmc_host *host,
u32 ocr, u32 *rocr)
        cmd.arg = ocr;
    cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R3 | MMC_CMD_BCR;

-   for (i = 100; i; i--) {
+    int busyness = 0;
+   for (i = 150; i; i--) {
+       mmc_delay(10);
        err = mmc_wait_for_app_cmd(host, NULL, &cmd, MMC_CMD_RETRIES);
        if (err)
            break;
@@ -175,13 +177,17 @@ int mmc_send_app_op_cond(struct mmc_host *host,
u32 ocr, u32 *rocr)
            if (!(cmd.resp[0] & R1_SPI_IDLE))
                break;
        } else {
-           if (cmd.resp[0] & MMC_CARD_BUSY)
-               break;
+           if (cmd.resp[0] & MMC_CARD_BUSY) {
+               busyness++;
+               printk(KERN_ALERT "busy returned\n");
+               if (busyness > 5) {
+                   break;
+               }
+           }
        }

        err = -ETIMEDOUT;

-       mmc_delay(10);
    }

Basically the patch will cause the driver to send 5 more ACMD41s even
after the (first) non-busy return. Real hardware (with a few different
SD card manufacturers) borks on these extra ACMD41s:

sdhci: Secure Digital Host Controller Interface driver
sdhci: Copyright(c) Pierre Ossman
sdhci-pltfm: SDHCI platform and OF driver helper
mmc0: Invalid maximum block size, assuming 512 bytes
mmc0: SDHCI controller on e0100000.ps7-sdio [e0100000.ps7-sdio] using ADMA
usbcore: registered new interface driver usbhid
usbhid: USB HID core driver
TCP: cubic registered
NET: Registered protocol family 10
sit: IPv6 over IPv4 tunneling driver
NET: Registered protocol family 17
NET: Registered protocol family 40
VFP support v0.3: implementor 41 architecture 3 part 30 variant 9 rev 4
Registering SWP/SWPB emulation handler
Freeing init memory: 6460K
INIT: version 2.88 booting
busy returned
mmc0: error -110 whilst initialising SD card
busy returned
mmc0: error -110 whilst initialising SD card
Starting Bootlog daemon: bootlogd.
Creating /dev/flash/* device nodes
busy returned
mmc0: error -110 whilst initialising SD card
busy returned
mmc0: error -110 whilst initialising SD card

QEMU before my patch is consistent with this behaviour (as expected).
QEMU after my patch loses the errors (which is bad):

sdhci: Secure Digital Host Controller Interface driver
sdhci: Copyright(c) Pierre Ossman
sdhci-pltfm: SDHCI platform and OF driver helper
mmc0: SDHCI controller on e0100000.ps7-sdio [e0100000.ps7-sdio] using ADMA
usbcore: registered new interface driver usbhid
usbhid: USB HID core driver
TCP: cubic registered
NET: Registered protocol family 10
sit: IPv6 over IPv4 tunneling driver
NET: Registered protocol family 17
NET: Registered protocol family 40
VFP support v0.3: implementor 41 architecture 3 part 30 variant 9 rev 0
Registering SWP/SWPB emulation handler
busy returned
busy returned
busy returned
busy returned
busy returned
busy returned
mmc0: SD Status: Invalid Allocation Unit size.
mmc0: new SD card at address 4567
Freeing init memory: 6460K
mmcblk0: mmc0:4567 QEMU! 256 MiB

Which only leaves your theory. The transition to ready state happens
on the successful poll of ACMD41 and not before. That and ACMD41 is
total illegal in ready state as documented.

> If that's how real SD card behaves in your tests, then I think this
> patch is OK, but it could benefit from a short comment explaining that
> this behaviour is not covered by specification.
>

So it turns out my error-throwing guest was using an inquiry ACMD41
with non-zero bits 31:24 in the arg. QEMU as is, misinterprets this as
a normal ("first") ACMD41 which is wrong. So my SD was getting
initialised ahead of time and QEMU was incorrectly putting my SD in
the ready state (rather than the read state being misbehaved as stated
by this patch). So the next version of the patch is very different and
fixes the ACMD41 inquiry vs first logic (but oddly the same subject
line). I've dropped the R.B. tags, as its fundamentally a different
patch. V2 on list.

Regards,
Peter

>
> Reviewed-by: Igor Mitsyanko <address@hidden>
>
>
>> Regards,
>> Peter
>>
>>> --
>>> Best wishes,
>>> Igor Mitsyanko
>>> email: address@hidden
>>
>>
>
>
> --
> Best wishes,
> Igor Mitsyanko
> email: address@hidden
>