[Qemu-devel] [PATCH v3 0/2] sdhci: pending insert quirk for raspberry pi

[Qemu-devel] [PATCH v3 0/2] sdhci: pending insert quirk for raspberry pi

[Andrew Baumann]

This is a small series to correctly implement a quirk modelling the
behaviour of the card insert interrupt bit on Raspberry Pi, and on
which Windows' UEFI loader sadly depends.

This should be the last change required to support Windows on Pi2,
outside of adding devices and enabling the quirk, which I hope to do
shortly in a new patch series.

Thanks,
Andrew

Andrew Baumann (2):
  sdhci: Revert "add optional quirk property to disable card
    insertion/removal interrupts"
  sdhci: add quirk property for card insert interrupt status on
    Raspberry Pi

 hw/sd/sdhci.c         | 42 +++++++++++++++++++++++++++++++++++-------
 include/hw/sd/sdhci.h |  2 +-
 2 files changed, 36 insertions(+), 8 deletions(-)

-- 
2.5.3

[Qemu-devel] [PATCH v3 1/2] sdhci: Revert "add optional quirk property to disable card insertion/removal interrupts"

[Andrew Baumann]

This reverts commit 723697551a7e926abe7d3c7f2966012b8075143d.

This change was poorly tested on my part. It squelched card insertion
interrupts on reset, but that was not necessary because sdhci_reset()
clears all the registers (via the call to memset), so the subsequent
sdhci_insert_eject_cb() call never sees the card insert interrupt
enabled. However, not calling the insert_eject_cb results in prnsts
remaining 0, when it actually needs to be updated to indicate card
presence and R/O status.

Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
---
 hw/sd/sdhci.c         | 9 +++------
 include/hw/sd/sdhci.h | 1 -
 2 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 73e7c87..f175b30 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -198,11 +198,9 @@ static void sdhci_reset(SDHCIState *s)
      * initialization */
     memset(&s->sdmasysad, 0, (uintptr_t)&s->capareg - (uintptr_t)&s->sdmasysad);
 
-    if (!s->noeject_quirk) {
-        /* Reset other state based on current card insertion/readonly status */
-        sdhci_set_inserted(dev, sdbus_get_inserted(&s->sdbus));
-        sdhci_set_readonly(dev, sdbus_get_readonly(&s->sdbus));
-    }
+    /* Reset other state based on current card insertion/readonly status */
+    sdhci_set_inserted(dev, sdbus_get_inserted(&s->sdbus));
+    sdhci_set_readonly(dev, sdbus_get_readonly(&s->sdbus));
 
     s->data_count = 0;
     s->stopped_state = sdhc_not_stopped;
@@ -1275,7 +1273,6 @@ 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(),
 };
 
diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index 607a83e..4816516 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -76,7 +76,6 @@ typedef struct SDHCIState {
     uint32_t buf_maxsz;
     uint16_t data_count;   /* current element in FIFO buffer */
     uint8_t  stopped_state;/* Current SDHC state */
-    bool     noeject_quirk;/* Quirk to disable card insert/remove interrupts */
     /* Buffer Data Port Register - virtual access point to R and W buffers */
     /* Software Reset Register - always reads as 0 */
     /* Force Event Auto CMD12 Error Interrupt Reg - write only */
-- 
2.5.3

[Qemu-devel] [PATCH v3 2/2] sdhci: add quirk property for card insert interrupt status on Raspberry Pi

[Andrew Baumann]

This quirk is a workaround for the following hardware behaviour, on
which UEFI (specifically, the bootloader for Windows on Pi2) depends:

1. at boot with an SD card present, the interrupt status/enable
   registers are initially zero
2. upon enabling it in the interrupt enable register, the card insert
   bit in the interrupt status register is immediately set
3. after a subsequent controller reset, the card insert interrupt does
   not fire, even if enabled in the interrupt enable register

The implementation uses a pending_insert bool, which can be set via a
property (enabling the quirk) and is cleared and remains clear once
the interrupt has been delivered.

Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
---
There's a fairly extensive discussion of the hardware behaviour that
this patch seeks to model in the thread at:
https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg00605.html

v3: changed to use subsection for vmstate, to preserve backward
compatibility

v2: changed implementation to use pending_insert bool rather than
masking norintsts at read time, since the older version diverges from
actual hardware behaviour when an interrupt is masked without being
acked

 hw/sd/sdhci.c         | 33 ++++++++++++++++++++++++++++++++-
 include/hw/sd/sdhci.h |  1 +
 2 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index f175b30..db34d78 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -204,6 +204,7 @@ static void sdhci_reset(SDHCIState *s)
 
     s->data_count = 0;
     s->stopped_state = sdhc_not_stopped;
+    s->pending_insert = false;
 }
 
 static void sdhci_data_transfer(void *opaque);
@@ -1095,6 +1096,12 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
         } else {
             s->norintsts &= ~SDHC_NIS_ERR;
         }
+        /* Quirk for Raspberry Pi: pending card insert interrupt
+         * appears when first enabled after power on */
+        if ((s->norintstsen & SDHC_NISEN_INSERT) && s->pending_insert) {
+            s->norintsts |= SDHC_NIS_INSERT;
+            s->pending_insert = false;
+        }
         sdhci_update_irq(s);
         break;
     case SDHC_NORINTSIGEN:
@@ -1181,6 +1188,24 @@ static void sdhci_uninitfn(SDHCIState *s)
     s->fifo_buffer = NULL;
 }
 
+static bool sdhci_pending_insert_vmstate_needed(void *opaque)
+{
+    SDHCIState *s = opaque;
+
+    return s->pending_insert;
+}
+
+static const VMStateDescription sdhci_pending_insert_vmstate = {
+    .name = "sdhci/pending-insert",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = sdhci_pending_insert_vmstate_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_BOOL(pending_insert, SDHCIState),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
 const VMStateDescription sdhci_vmstate = {
     .name = "sdhci",
     .version_id = 1,
@@ -1215,7 +1240,11 @@ const VMStateDescription sdhci_vmstate = {
         VMSTATE_TIMER_PTR(insert_timer, SDHCIState),
         VMSTATE_TIMER_PTR(transfer_timer, SDHCIState),
         VMSTATE_END_OF_LIST()
-    }
+    },
+    .subsections = (const VMStateDescription*[]) {
+        &sdhci_pending_insert_vmstate,
+        NULL
+    },
 };
 
 /* Capabilities registers provide information on supported features of this
@@ -1224,6 +1253,8 @@ static Property sdhci_pci_properties[] = {
     DEFINE_PROP_UINT32("capareg", SDHCIState, capareg,
             SDHC_CAPAB_REG_DEFAULT),
     DEFINE_PROP_UINT32("maxcurr", SDHCIState, maxcurr, 0),
+    DEFINE_PROP_BOOL("pending-insert-quirk", SDHCIState, pending_insert,
+                     false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index 4816516..aab7cf0 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -76,6 +76,7 @@ typedef struct SDHCIState {
     uint32_t buf_maxsz;
     uint16_t data_count;   /* current element in FIFO buffer */
     uint8_t  stopped_state;/* Current SDHC state */
+    bool     pending_insert;/* Quirk for Raspberry Pi card insert interrupt */
     /* Buffer Data Port Register - virtual access point to R and W buffers */
     /* Software Reset Register - always reads as 0 */
     /* Force Event Auto CMD12 Error Interrupt Reg - write only */
-- 
2.5.3

Re: [Qemu-devel] [PATCH v3 2/2] sdhci: add quirk property for card insert interrupt status on Raspberry Pi

[Peter Maydell]

On 24 February 2016 at 22:47, Andrew Baumann
<Andrew.Baumann@microsoft.com> wrote:
> This quirk is a workaround for the following hardware behaviour, on
> which UEFI (specifically, the bootloader for Windows on Pi2) depends:
>
> 1. at boot with an SD card present, the interrupt status/enable
>    registers are initially zero
> 2. upon enabling it in the interrupt enable register, the card insert
>    bit in the interrupt status register is immediately set
> 3. after a subsequent controller reset, the card insert interrupt does
>    not fire, even if enabled in the interrupt enable register
>
> The implementation uses a pending_insert bool, which can be set via a
> property (enabling the quirk) and is cleared and remains clear once
> the interrupt has been delivered.
>
> Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
> ---
> There's a fairly extensive discussion of the hardware behaviour that
> this patch seeks to model in the thread at:
> https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg00605.html
>
> v3: changed to use subsection for vmstate, to preserve backward
> compatibility
>
> v2: changed implementation to use pending_insert bool rather than
> masking norintsts at read time, since the older version diverges from
> actual hardware behaviour when an interrupt is masked without being
> acked
>
>  hw/sd/sdhci.c         | 33 ++++++++++++++++++++++++++++++++-
>  include/hw/sd/sdhci.h |  1 +
>  2 files changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index f175b30..db34d78 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -204,6 +204,7 @@ static void sdhci_reset(SDHCIState *s)
>
>      s->data_count = 0;
>      s->stopped_state = sdhc_not_stopped;
> +    s->pending_insert = false;

You seem to be trying to use this both to store the device
property value and as the current state of the device.
I don't think that will work, because if we do a system
reset then the device state should go back to what it was
when QEMU was first started (meaning it goes back to whatever
the property value was set to), and at the moment you have
no mechanism for that.

The patch seems OK otherwise (not having looked into the
depths of the hardware behaviour).

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 2/2] sdhci: add quirk property for card insert interrupt status on Raspberry Pi

[Andrew Baumann]

Hi Peter,

> From: Peter Maydell [mailto:peter.maydell@linaro.org]
> Sent: Thursday, 25 February 2016 10:51 AM
> 
> On 24 February 2016 at 22:47, Andrew Baumann
> <Andrew.Baumann@microsoft.com> wrote:
> > This quirk is a workaround for the following hardware behaviour, on
> > which UEFI (specifically, the bootloader for Windows on Pi2) depends:
> >
> > 1. at boot with an SD card present, the interrupt status/enable
> >    registers are initially zero
> > 2. upon enabling it in the interrupt enable register, the card insert
> >    bit in the interrupt status register is immediately set
> > 3. after a subsequent controller reset, the card insert interrupt does
> >    not fire, even if enabled in the interrupt enable register
> >
> > The implementation uses a pending_insert bool, which can be set via a
> > property (enabling the quirk) and is cleared and remains clear once
> > the interrupt has been delivered.
> >
> > Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
> > ---
> > There's a fairly extensive discussion of the hardware behaviour that
> > this patch seeks to model in the thread at:
> > https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg00605.html
> >
> > v3: changed to use subsection for vmstate, to preserve backward
> > compatibility
> >
> > v2: changed implementation to use pending_insert bool rather than
> > masking norintsts at read time, since the older version diverges from
> > actual hardware behaviour when an interrupt is masked without being
> > acked
> >
> >  hw/sd/sdhci.c         | 33 ++++++++++++++++++++++++++++++++-
> >  include/hw/sd/sdhci.h |  1 +
> >  2 files changed, 33 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> > index f175b30..db34d78 100644
> > --- a/hw/sd/sdhci.c
> > +++ b/hw/sd/sdhci.c
> > @@ -204,6 +204,7 @@ static void sdhci_reset(SDHCIState *s)
> >
> >      s->data_count = 0;
> >      s->stopped_state = sdhc_not_stopped;
> > +    s->pending_insert = false;
> 
> You seem to be trying to use this both to store the device
> property value and as the current state of the device.

That's right. It's set to true as a property value, and then cleared (and remains clear) once issued. 

> I don't think that will work, because if we do a system
> reset then the device state should go back to what it was
> when QEMU was first started (meaning it goes back to whatever
> the property value was set to), and at the moment you have
> no mechanism for that.

Hmm. This thought had occurred to me, but I could not find any system reset logic in sdhci.c -- sdhci_reset() is only referenced by the code path for a guest-initiated write to the reset register. There is no system reset handler logic anywhere that I can see, so the sdhci state would be the same after reset, unless something else completely tears down and re-inits the device, in which case my property trick should work. Is this a bug, or am I missing something?

> The patch seems OK otherwise (not having looked into the
> depths of the hardware behaviour).

Thanks. In any case, I am probably being a bit too "clever" and will rework it to use separate flags to track the pending interrupt and property value.

Andrew

Re: [Qemu-devel] [PATCH v3 2/2] sdhci: add quirk property for card insert interrupt status on Raspberry Pi

[Peter Maydell]

On 25 February 2016 at 19:00, Andrew Baumann
<Andrew.Baumann@microsoft.com> wrote:
> Hmm. This thought had occurred to me, but I could not find any
> system reset logic in sdhci.c -- sdhci_reset() is only referenced
> by the code path for a guest-initiated write to the reset register.
> There is no system reset handler logic anywhere that I can see, so
> the sdhci state would be the same after reset, unless something
> else completely tears down and re-inits the device, in which case
> my property trick should work. Is this a bug, or am I missing something?

Yes, that's a bug. The device should register a reset function
via dc->reset. This should generally have power-on-reset
semantics. (For PCI devices it also gets called on PCI bus
reset. Hopefully those two sets of reset semantics are the
same for this card...)

Missing reset implementation is a fairly common bug in QEMU
devices; it often goes unnoticed because of some combination of
"reset state is not well defined anyway" and "guest OS fully
programs the device and doesn't rely on whatever its reset state
happens to be" and "people don't often try to reset QEMU in the
middle of guest operation".

Unfortunately it's awkward to find devices that need fixing, because
it's hard to distinguish "this device is missing a reset function"
from "this device genuinely doesn't need a reset function because
it has no internal state of its own"...

thanks
-- PMM