[Qemu-devel] [PATCH arm-devs v1 0/5] SD and SDHCI Fixes

[Qemu-devel] [PATCH arm-devs v1 0/5] SD and SDHCI Fixes

[peter.crosthwaite]

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

Fixes found in SD and SDHCI found doing some corner case testing.


Peter Crosthwaite (5):
  sd/sd.c: Fix "inquiry" ACMD41
  sd/sdhci.c: Only reset data_count on new commands
  sd/sdhci: Fix Buffer Write Ready interrupt
  sd/sdhci.c: Fix bdata_read DPRINT message
  sd/sdhci:ADMA: fix interrupt

 hw/sd/sd.c    |  1 +
 hw/sd/sdhci.c | 28 ++++++++++++++--------------
 2 files changed, 15 insertions(+), 14 deletions(-)

-- 
1.8.3.rc1.44.gb387c77.dirty

[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

[Qemu-devel] [PATCH arm-devs v1 2/5] sd/sdhci.c: Only reset data_count on new commands

[peter.crosthwaite]

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

The data_count variable was being reset on every transfer, including
DMA transfer resumptions. This is incorrect, it should only be set
on a new command.

Manifests as a bug when using ADMA and there is a timer delay between
ADMA frames where the fifo is left in a non empty state.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/sd/sdhci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 91dc9b0..0a84540 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -260,6 +260,7 @@ static void sdhci_send_command(SDHCIState *s)
     sdhci_update_irq(s);
 
     if (s->blksize && (s->cmdreg & SDHC_CMD_DATA_PRESENT)) {
+        s->data_count = 0;
         sdhci_do_data_transfer(s);
     }
 }
@@ -773,7 +774,6 @@ static void sdhci_do_adma(SDHCIState *s)
 static void sdhci_data_transfer(SDHCIState *s)
 {
     SDHCIClass *k = SDHCI_GET_CLASS(s);
-    s->data_count = 0;
 
     if (s->trnmod & SDHC_TRNS_DMA) {
         switch (SDHC_DMA_TYPE(s->hostctl)) {
-- 
1.8.3.rc1.44.gb387c77.dirty

[Qemu-devel] [PATCH arm-devs v1 3/5] sd/sdhci: Fix Buffer Write Ready interrupt

[peter.crosthwaite]

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

This interrupt is not risen after the last block is written to sd. It
is mutually exclusive with the end of transfer conditions. Fix.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/sd/sdhci.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 0a84540..ea510b5 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -405,15 +405,14 @@ static void sdhci_write_block_to_card(SDHCIState *s)
 
     /* Next data can be written through BUFFER DATORT register */
     s->prnsts |= SDHC_SPACE_AVAILABLE;
-    if (s->norintstsen & SDHC_NISEN_WBUFRDY) {
-        s->norintsts |= SDHC_NIS_WBUFRDY;
-    }
 
     /* Finish transfer if that was the last block of data */
     if ((s->trnmod & SDHC_TRNS_MULTI) == 0 ||
             ((s->trnmod & SDHC_TRNS_MULTI) &&
             (s->trnmod & SDHC_TRNS_BLK_CNT_EN) && (s->blkcnt == 0))) {
         SDHCI_GET_CLASS(s)->end_data_transfer(s);
+    } else if (s->norintstsen & SDHC_NISEN_WBUFRDY) {
+        s->norintsts |= SDHC_NIS_WBUFRDY;
     }
 
     /* Generate Block Gap Event if requested and if not the last block */
-- 
1.8.3.rc1.44.gb387c77.dirty

[Qemu-devel] [PATCH arm-devs v1 4/5] sd/sdhci.c: Fix bdata_read DPRINT message

[peter.crosthwaite]

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

This message was printing out the data in decimal only, which is not
very friendly to the debugging developer. Add hex variant in
parenthesis to make it consistent with other similar messages in this
module.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/sd/sdhci.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index ea510b5..15345dc 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -880,7 +880,8 @@ static uint32_t sdhci_read(SDHCIState *s, unsigned int offset, unsigned size)
     case  SDHC_BDATA:
         if (sdhci_buff_access_is_sequential(s, offset - SDHC_BDATA)) {
             ret = SDHCI_GET_CLASS(s)->bdata_read(s, size);
-            DPRINT_L2("read %ub: addr[0x%04x] -> %u\n", size, offset, ret);
+            DPRINT_L2("read %ub: addr[0x%04x] -> %u(0x%x)\n", size, offset,
+                      ret, ret);
             return ret;
         }
         break;
-- 
1.8.3.rc1.44.gb387c77.dirty

[Qemu-devel] [PATCH arm-devs v1 5/5] sd/sdhci:ADMA: fix interrupt

[peter.crosthwaite]

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

The end of transfer check was occurring and potentially returning before
the interrupt flag was checked. This means the interrupt will be missed
if it occurs on the last packet. Fix by checking for the interrupt
before checking for the end of transfer.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/sd/sdhci.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 15345dc..e64899c 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -730,6 +730,15 @@ static void sdhci_do_adma(SDHCIState *s)
             break;
         }
 
+        if (dscr.attr & SDHC_ADMA_ATTR_INT) {
+            DPRINT_L1("ADMA interrupt: admasysaddr=0x%lx\n", s->admasysaddr);
+            if (s->norintstsen & SDHC_NISEN_DMA) {
+                s->norintsts |= SDHC_NIS_DMA;
+            }
+
+            sdhci_update_irq(s);
+        }
+
         /* ADMA transfer terminates if blkcnt == 0 or by END attribute */
         if (((s->trnmod & SDHC_TRNS_BLK_CNT_EN) &&
                     (s->blkcnt == 0)) || (dscr.attr & SDHC_ADMA_ATTR_END)) {
@@ -752,15 +761,6 @@ static void sdhci_do_adma(SDHCIState *s)
             return;
         }
 
-        if (dscr.attr & SDHC_ADMA_ATTR_INT) {
-            DPRINT_L1("ADMA interrupt: admasysaddr=0x%lx\n", s->admasysaddr);
-            if (s->norintstsen & SDHC_NISEN_DMA) {
-                s->norintsts |= SDHC_NIS_DMA;
-            }
-
-            sdhci_update_irq(s);
-            return;
-        }
     }
 
     /* we have unfinished business - reschedule to continue ADMA */
-- 
1.8.3.rc1.44.gb387c77.dirty

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

Re: [Qemu-devel] [PATCH arm-devs v1 4/5] sd/sdhci.c: Fix bdata_read DPRINT message

[Peter Maydell]

On 21 May 2013 07:52,  <peter.crosthwaite@xilinx.com> wrote:
> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>
> This message was printing out the data in decimal only, which is not
> very friendly to the debugging developer. Add hex variant in
> parenthesis to make it consistent with other similar messages in this
> module.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

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

-- PMM

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

[-- 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 --]