| Message ID | 1456354075-8424-3-git-send-email-Andrew.Baumann@microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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 */
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(-)