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:address@hidden
> Sent: Thursday, 25 February 2016 10:51 AM
> 
> On 24 February 2016 at 22:47, Andrew Baumann
> <address@hidden> 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 <address@hidden>
> > ---
> > 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