[Qemu-devel] [PATCH 0/3] sdhci patches to enable Raspberry Pi

[Qemu-devel] [PATCH 0/3] sdhci patches to enable Raspberry Pi

[Andrew Baumann]

This is a series of three tweaks needed to enable the generic sdhci
controller to emulate Raspberry Pi (bcm2835/2836), and boot Linux and
Windows.

There was some discussion of these changes in the following thread:
https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg01401.html

Cheers,
Andrew

Andrew Baumann (3):
  sd: sdhci: Delete over-zealous power check
  sdhci: don't raise a command index error for an unexpected response
  sdhci: add optional quirk property to disable card insertion/removal
    interrupts

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

-- 
2.5.3

[Qemu-devel] [PATCH 1/3] sd: sdhci: Delete over-zealous power check

[Andrew Baumann]

This check was conditionalising SD card operation on the card being
powered by the SDHCI host controller. It is however possible
(particularly in embedded systems) for the power control of the SD card
to be managed outside of SDHCI. This can be as trivial as hard-wiring
the SD slot VCC to a constant power-rail.

This means the guest SDHCI can validly opt-out of the SDHCI power
control feature while still using the card. So delete this check to
allow operation of the card with SDHCI power control.

This is needed for at least Xilinx Zynq and also makes Freescale i.MX25
work for me. The digilent Zybo board has a public schematic
which shows SD VCC hardwiring:

http://digilentinc.com/Data/Products/ZYBO/ZYBO_sch_VB.3.pdf
bottom of page 3.

Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
Reviewed-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
---

Notes:
    This is exactly equivalent to Peter's patch here:
    https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg06271.html
    ... which is why I've copied the signed-off-by lines (I assume that is
    legit; if not, my apologies!). It is also needed for Pi.

 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 8612760..bc39d48 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -831,7 +831,7 @@ static void sdhci_data_transfer(void *opaque)
 
 static bool sdhci_can_issue_command(SDHCIState *s)
 {
-    if (!SDHC_CLOCK_IS_ON(s->clkcon) || !(s->pwrcon & SDHC_POWER_ON) ||
+    if (!SDHC_CLOCK_IS_ON(s->clkcon) ||
         (((s->prnsts & SDHC_DATA_INHIBIT) || s->stopped_state) &&
         ((s->cmdreg & SDHC_CMD_DATA_PRESENT) ||
         ((s->cmdreg & SDHC_CMD_RESPONSE) == SDHC_CMD_RSP_WITH_BUSY &&
-- 
2.5.3

[Qemu-devel] [PATCH 2/3] sdhci: don't raise a command index error for an unexpected response

[Andrew Baumann]

This deletes a block of code that raised a command index error if a
command returned response data, but the guest did not set the
appropriate bits in the response register to handle such a response. I
cannot find any documentation that suggests the controller should
behave in this way, the error code doesn't make sense (command index
error is defined for the case where the index in a response does not
match that of the issued command), and in at least one case (CMD23
issued by UEFI on Raspberry Pi 2), actual hardware does not do this.

Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
---
 hw/sd/sdhci.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index bc39d48..dd83e89 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -243,9 +243,6 @@ static void sdhci_send_command(SDHCIState *s)
             (s->cmdreg & SDHC_CMD_RESPONSE) == SDHC_CMD_RSP_WITH_BUSY) {
             s->norintsts |= SDHC_NIS_TRSCMP;
         }
-    } else if (rlen != 0 && (s->errintstsen & SDHC_EISEN_CMDIDX)) {
-        s->errintsts |= SDHC_EIS_CMDIDX;
-        s->norintsts |= SDHC_NIS_ERR;
     }
 
     if (s->norintstsen & SDHC_NISEN_CMDCMP) {
-- 
2.5.3

[Qemu-devel] [PATCH 3/3] sdhci: add optional quirk property to disable card insertion/removal interrupts

[Andrew Baumann]

This is needed for a quirk of the Raspberry Pi (bcm2835/6) MMC
controller, where the card insert bit is documented as unimplemented
(always reads zero, doesn't generate interrupts) but is in fact
observed on hardware as set at power on, but is cleared (and remains
clear) on subsequent controller resets.

Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
---
 hw/sd/sdhci.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index dd83e89..61f919b 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -193,7 +193,9 @@ static void sdhci_reset(SDHCIState *s)
      * initialization */
     memset(&s->sdmasysad, 0, (uintptr_t)&s->capareg - (uintptr_t)&s->sdmasysad);
 
-    sd_set_cb(s->card, s->ro_cb, s->eject_cb);
+    if (!s->noeject_quirk) {
+        sd_set_cb(s->card, s->ro_cb, s->eject_cb);
+    }
     s->data_count = 0;
     s->stopped_state = sdhc_not_stopped;
 }
@@ -1208,6 +1210,7 @@ const VMStateDescription sdhci_vmstate = {
         VMSTATE_UINT16(data_count, SDHCIState),
         VMSTATE_UINT64(admasysaddr, SDHCIState),
         VMSTATE_UINT8(stopped_state, SDHCIState),
+        VMSTATE_BOOL(noeject_quirk, SDHCIState),
         VMSTATE_VBUFFER_UINT32(fifo_buffer, SDHCIState, 1, NULL, 0, buf_maxsz),
         VMSTATE_TIMER_PTR(insert_timer, SDHCIState),
         VMSTATE_TIMER_PTR(transfer_timer, SDHCIState),
@@ -1276,6 +1279,7 @@ static Property sdhci_sysbus_properties[] = {
     DEFINE_PROP_UINT32("capareg", SDHCIState, capareg,
             SDHC_CAPAB_REG_DEFAULT),
     DEFINE_PROP_UINT32("maxcurr", SDHCIState, maxcurr, 0),
+    DEFINE_PROP_BOOL("noeject-quirk", SDHCIState, noeject_quirk, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.5.3

Re: [Qemu-devel] [PATCH 1/3] sd: sdhci: Delete over-zealous power check

[Peter Crosthwaite]

On Wed, Dec 16, 2015 at 11:47 AM, Andrew Baumann
<Andrew.Baumann@microsoft.com> wrote:
> This check was conditionalising SD card operation on the card being
> powered by the SDHCI host controller. It is however possible
> (particularly in embedded systems) for the power control of the SD card
> to be managed outside of SDHCI. This can be as trivial as hard-wiring
> the SD slot VCC to a constant power-rail.
>
> This means the guest SDHCI can validly opt-out of the SDHCI power
> control feature while still using the card. So delete this check to
> allow operation of the card with SDHCI power control.
>
> This is needed for at least Xilinx Zynq and also makes Freescale i.MX25
> work for me. The digilent Zybo board has a public schematic
> which shows SD VCC hardwiring:
>

You should add a note that it is needed for rPI as well.

> http://digilentinc.com/Data/Products/ZYBO/ZYBO_sch_VB.3.pdf
> bottom of page 3.
>
> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> Reviewed-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>

You should add your tested-by as it is valuable as non-author. Pavan
probably should be CC'd singly (got can be configured to do this
automatically) but I would just CC him whole series as he has does
some SD work.

At this point here you add committable notes about what you have
changed since the original authorship. If you added the note about
fixing rPI, something like:

[ AB changes:
 * Add rPI to list of devices fixed in commit message
]

> Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
> ---
>
> Notes:
>     This is exactly equivalent to Peter's patch here:
>     https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg06271.html
>     ... which is why I've copied the signed-off-by lines (I assume that is
>     legit; if not, my apologies!). It is also needed for Pi.
>

The correct process it to apply the patch locally as originally,
authored then append your SoB and any notes/changes using git commit
--amend. Git am can apply the patch from an mbox, or the patches
utility can be used to download and apply patches off the list. You
could in this instance grab the original patch from my github tree:

https://github.com/pcrost/qemu/commit/db8e44ad1d31418fb4b5aaacc8552a39bb2d670f

Regards,
Peter

>  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 8612760..bc39d48 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -831,7 +831,7 @@ static void sdhci_data_transfer(void *opaque)
>
>  static bool sdhci_can_issue_command(SDHCIState *s)
>  {
> -    if (!SDHC_CLOCK_IS_ON(s->clkcon) || !(s->pwrcon & SDHC_POWER_ON) ||
> +    if (!SDHC_CLOCK_IS_ON(s->clkcon) ||
>          (((s->prnsts & SDHC_DATA_INHIBIT) || s->stopped_state) &&
>          ((s->cmdreg & SDHC_CMD_DATA_PRESENT) ||
>          ((s->cmdreg & SDHC_CMD_RESPONSE) == SDHC_CMD_RSP_WITH_BUSY &&
> --
> 2.5.3
>

Re: [Qemu-devel] [PATCH 1/3] sd: sdhci: Delete over-zealous power check

[Andrew Baumann]

> From: Peter Crosthwaite [mailto:crosthwaitepeter@gmail.com]
> Sent: Wednesday, 16 December 2015 12:00
> On Wed, Dec 16, 2015 at 11:47 AM, Andrew Baumann
> <Andrew.Baumann@microsoft.com> wrote:
> >     This is exactly equivalent to Peter's patch here:
> >     https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg06271.html
> >     ... which is why I've copied the signed-off-by lines (I assume that is
> >     legit; if not, my apologies!). It is also needed for Pi.
> >
> 
> The correct process it to apply the patch locally as originally,
> authored then append your SoB and any notes/changes using git commit
> --amend. Git am can apply the patch from an mbox, or the patches
> utility can be used to download and apply patches off the list.

Thanks Peter. I'll do this when I redo the patch series with any other feedback.

Andrew

Re: [Qemu-devel] [PATCH 2/3] sdhci: don't raise a command index error for an unexpected response

[Peter Crosthwaite]

On Wed, Dec 16, 2015 at 11:47 AM, Andrew Baumann
<Andrew.Baumann@microsoft.com> wrote:
> This deletes a block of code that raised a command index error if a
> command returned response data, but the guest did not set the
> appropriate bits in the response register to handle such a response. I
> cannot find any documentation that suggests the controller should
> behave in this way, the error code doesn't make sense (command index
> error is defined for the case where the index in a response does not
> match that of the issued command), and in at least one case (CMD23
> issued by UEFI on Raspberry Pi 2), actual hardware does not do this.
>
> Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com>

Reviewed-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>

All I can think of, is the original author is assuming that any
mismatch in response length can only occur on a mismatch of commands.
If we really need _CMDIDX it should be done directly as a check of the
command index in the response.

Regards,
Peter

> ---
>  hw/sd/sdhci.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index bc39d48..dd83e89 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -243,9 +243,6 @@ static void sdhci_send_command(SDHCIState *s)
>              (s->cmdreg & SDHC_CMD_RESPONSE) == SDHC_CMD_RSP_WITH_BUSY) {
>              s->norintsts |= SDHC_NIS_TRSCMP;
>          }
> -    } else if (rlen != 0 && (s->errintstsen & SDHC_EISEN_CMDIDX)) {
> -        s->errintsts |= SDHC_EIS_CMDIDX;
> -        s->norintsts |= SDHC_NIS_ERR;
>      }
>
>      if (s->norintstsen & SDHC_NISEN_CMDCMP) {
> --
> 2.5.3
>

Re: [Qemu-devel] [PATCH 3/3] sdhci: add optional quirk property to disable card insertion/removal interrupts

[Peter Crosthwaite]

On Wed, Dec 16, 2015 at 11:47 AM, Andrew Baumann
<Andrew.Baumann@microsoft.com> wrote:
> This is needed for a quirk of the Raspberry Pi (bcm2835/6) MMC
> controller, where the card insert bit is documented as unimplemented
> (always reads zero, doesn't generate interrupts) but is in fact
> observed on hardware as set at power on, but is cleared (and remains
> clear) on subsequent controller resets.
>

Yep. Thats what the doc is saying.

FWIW , that set-on-reset behaviour makes me very suspicious that this
is the SoC layer or pin mux tying off this pin incorrectly. A bug in
the pin-mux would cause exactly this symptom.

Unfortunately we (or at least I) don't have access to Arasan's doc to
tell if this this is SoC errata or SDHCI errata and the Broadcom doc
quite unhelpfully refers the reader to:

sd3.0_host_ahb_emmc4.4_usersguide_ver5.9_jan11_10.pdf

Which doesn't google.

> Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com>

Reviewed-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>

> ---
>  hw/sd/sdhci.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index dd83e89..61f919b 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -193,7 +193,9 @@ static void sdhci_reset(SDHCIState *s)
>       * initialization */
>      memset(&s->sdmasysad, 0, (uintptr_t)&s->capareg - (uintptr_t)&s->sdmasysad);
>
> -    sd_set_cb(s->card, s->ro_cb, s->eject_cb);
> +    if (!s->noeject_quirk) {
> +        sd_set_cb(s->card, s->ro_cb, s->eject_cb);
> +    }

This will conflict with PMMs SD busification work but the resolution
should be reasonably straightforward. The Busifed SDHCI can chose to
ignore the inserted cb from the bus instead of the card.

Regards,
Peter

>      s->data_count = 0;
>      s->stopped_state = sdhc_not_stopped;
>  }
> @@ -1208,6 +1210,7 @@ const VMStateDescription sdhci_vmstate = {
>          VMSTATE_UINT16(data_count, SDHCIState),
>          VMSTATE_UINT64(admasysaddr, SDHCIState),
>          VMSTATE_UINT8(stopped_state, SDHCIState),
> +        VMSTATE_BOOL(noeject_quirk, SDHCIState),
>          VMSTATE_VBUFFER_UINT32(fifo_buffer, SDHCIState, 1, NULL, 0, buf_maxsz),
>          VMSTATE_TIMER_PTR(insert_timer, SDHCIState),
>          VMSTATE_TIMER_PTR(transfer_timer, SDHCIState),
> @@ -1276,6 +1279,7 @@ static Property sdhci_sysbus_properties[] = {
>      DEFINE_PROP_UINT32("capareg", SDHCIState, capareg,
>              SDHC_CAPAB_REG_DEFAULT),
>      DEFINE_PROP_UINT32("maxcurr", SDHCIState, maxcurr, 0),
> +    DEFINE_PROP_BOOL("noeject-quirk", SDHCIState, noeject_quirk, false),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> --
> 2.5.3
>