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

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

[Igor Mitsyanko]

[-- Attachment #1: Type: text/plain, Size: 1221 bytes --]

On 05/21/2013 10:50 AM, peter.crosthwaite@xilinx.com wrote:

From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
<peter.crosthwaite@xilinx.com>

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 <peter.crosthwaite@xilinx.com>
<peter.crosthwaite@xilinx.com>
---

 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?

-- 
Best wishes,
Igor Mitsyanko
email: i.mitsyanko@gmail.com

[-- Attachment #2: Type: text/html, Size: 1710 bytes --]

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

[Peter Crosthwaite]

Hi Igor,

On Wed, May 22, 2013 at 11:37 PM, Igor Mitsyanko <i.mitsyanko@gmail.com> wrote:
>
> On 05/21/2013 10:50 AM, peter.crosthwaite@xilinx.com wrote:
>
> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>
> 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 <peter.crosthwaite@xilinx.com>
> ---
>
>  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.

Regards,
Peter

> --
> Best wishes,
> Igor Mitsyanko
> email: i.mitsyanko@gmail.com

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

[Igor Mitsyanko]

On 05/23/2013 03:42 AM, Peter Crosthwaite wrote:
> Hi Igor,
>
> On Wed, May 22, 2013 at 11:37 PM, Igor Mitsyanko <i.mitsyanko@gmail.com> wrote:
>>
>> On 05/21/2013 10:50 AM, peter.crosthwaite@xilinx.com wrote:
>>
>> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>>
>> 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 <peter.crosthwaite@xilinx.com>
>> ---
>>
>>   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.

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.


Reviewed-by: Igor Mitsyanko <i.mitsyanko@gmail.com>


> Regards,
> Peter
>
>> --
>> Best wishes,
>> Igor Mitsyanko
>> email: i.mitsyanko@gmail.com
>
>


--
Best wishes,
Igor Mitsyanko
email: i.mitsyanko@gmail.com

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 <i.mitsyanko@gmail.com> wrote:
> On 05/23/2013 03:42 AM, Peter Crosthwaite wrote:
>> Hi Igor,
>>
>> On Wed, May 22, 2013 at 11:37 PM, Igor Mitsyanko <i.mitsyanko@gmail.com> wrote:
>>>
>>> On 05/21/2013 10:50 AM, peter.crosthwaite@xilinx.com wrote:
>>>
>>> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>>>
>>> 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 <peter.crosthwaite@xilinx.com>
>>> ---
>>>
>>>   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 <i.mitsyanko@gmail.com>
>
>
>> Regards,
>> Peter
>>
>>> --
>>> Best wishes,
>>> Igor Mitsyanko
>>> email: i.mitsyanko@gmail.com
>>
>>
>
>
> --
> Best wishes,
> Igor Mitsyanko
> email: i.mitsyanko@gmail.com
>

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

[Igor Mitsyanko]

On 05/27/2013 06:41 AM, peter.crosthwaite@xilinx.com wrote:
> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>
> QEMU models two (of the three) ACMD41 has two modes, "inquiry" and
> "first". The selection logic for which of the two is incorrect - it
> compares != 0 for the entire argument value rather than only bits 23:0
> as per the spec. Fix.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
> Changed since v2:
> Macroified magic number
> Added explanatory comment (PMM review)
> Changed since v1:
> Total rewrite
>
>   hw/sd/sd.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 2e0ef3e..a10313b 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -43,6 +43,8 @@ do { fprintf(stderr, "SD: " fmt , ## __VA_ARGS__); } while (0)
>   #define DPRINTF(fmt, ...) do {} while(0)
>   #endif
>
> +#define ACMD41_ENQUIRY_MASK 0x00ffffff
> +
>   typedef enum {
>       sd_r0 = 0,    /* no response */
>       sd_r1,        /* normal response command */
> @@ -1277,9 +1279,14 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
>           }
>           switch (sd->state) {
>           case sd_idle_state:
> -            /* We accept any voltage.  10000 V is nothing.  */
> -            if (req.arg)
> +            /* We accept any voltage.  10000 V is nothing.
> +             *
> +             * We don't model init delay so just advance straight to ready state
> +             * unless its an enquiry ACMD41 (bits 23:0 == 0).
> +             */
> +            if (req.arg & ACMD41_ENQUIRY_MASK) {
>                   sd->state = sd_ready_state;
> +            }
>
>               return sd_r3;
>
>

Reviewed-by: Igor Mitsyanko <i.mitsyanko@gmail.com>


--
Best wishes,
Igor Mitsyanko
email: i.mitsyanko@gmail.com

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

[Peter Maydell]

On 21 May 2013 07:50,  <peter.crosthwaite@xilinx.com> wrote:
> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>
> 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 <peter.crosthwaite@xilinx.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM

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

[peter.crosthwaite]

From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

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 <peter.crosthwaite@xilinx.com>
---

 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;
-- 
1.8.3.rc1.44.gb387c77.dirty