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

[Andrew Baumann]

> From: Peter Crosthwaite [mailto:address@hidden
> Sent: Monday, 4 January 2016 22:18
> On Mon, Jan 4, 2016 at 2:12 PM, Andrew Baumann
> <address@hidden> wrote:
> >> From: Peter Crosthwaite [mailto:address@hidden
> >> Sent: Thursday, 31 December 2015 21:38
> >> On Thu, Dec 31, 2015 at 1:40 PM, 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
> >> >
> >>
> >> This is a baffling symptom. Does prnsts card ejection state fully work
> >> with physical card ejections and insertions both before and after the
> >> subsequent controller reset?
> >
> > I just tested this, by polling prnsts and intsts in a tight loop at board 
> > startup.
> At power on with a card inserted, prnsts reads 1FFF0000. Subsequent
> removal of the card, re-insertion etc. does not change its value.
> 
> Does either the subsequent reset or the interrupt ack change it? I'm
> assuming it is stuck permanently at 1fff.

That's correct -- there's no change.

> >After enabling interrupts, I reliably see a card insert interrupt in intsts. 
> >If I
> then write zero to the interrupt enable register, the pending card insert
> interrupt remains, which seems to dispel the "mask on read" theory. Once
> acked or reset, the card insert interrupt never recurs. I never saw a card
> removal interrupt.
> >
> 
> So
> 
> * interrupt status is initially 0
> * writing one to enable triggers the ghost
> * it can only be cleared with a status ack
(or reset)
> * you can never get a second ghost

Correct.

[...]
> > but either way there is a reliable ghost of a card insertion interrupt that 
> > is
> signalled at power on, and remains pending until it is either acked or the
> controller reset, after which point it never recurs. And I'd really like to 
> model
> that somehow without making a mess of sdhci.c :) Any ideas?
> >
> 
> Ok, I think it can be explained as a bad top-level connection as
> follows. The pin is mis-connected in such a way that such that it sees
> one edge on the POR reset and never sees any action again. The
> controller considers this pin edge-triggered and has the penning quirk
> as well, that is it saves edge interrupt until they are enabled and
> then releases them singly to the status register.
> 
> This doesn't explain why the controller doesn't see the interrupt on
> the soft reset, but perhaps that is explained by the spec, as I don't
> see anywhere that says that the interrupt has to retrigger for a
> constantly inserted card over a controller reset. Might be
> implementation specific.
> 
> Looking at the set_cb stuff, I think the guard on your original quirk
> implementation may be missing for the sd_set_cb() in sdhci_initfn().
> If this guard were added that quirk would be more complete, as
> currently it probably is seeing action on changes of state.
> 
> I think the way to correct the original quirk is to:
> 
> * make both sd_set_cb()'s conditional
> * manually call insert_eject_cb() on the POR reset (call the CB
> instead of register it).
> 
> Note that sdhci has no device::reset callback. You could add this to
> implement your POR reset.
> 
> You then have the problem of the prnsts register, which I assume it
> getting blasted by the reset memset. That can be managed by
> specifically preserving those two bits of prnsts through the reset
> (with an accompanying comment that this is needed for your quirk).

Assuming the user doesn't eject/change the SD card at runtime then my original 
patch isn't necessary at all. (I'm happy with that outcome, which is why I 
submitted the revert patch.) Because the memset in reset clears norintstsen, 
the sdhci_insert_eject_cb will never signal an insert interrupt. If we wanted a 
quirk to disable insert/eject interrupts, then what you've proposed seems like 
the right thing to do, although I think we'd need to preserve more than two 
bits of prnsts; we'd also need the write protect bit, and it's probably safe to 
just keep the upper half of the register.

> Your patch as-is here doesn't seem to address the penning behaviour
> (where the interrupt status remains clear until it is enabled), maybe
> that can be added as a second quirk if needed later?

My second patch does handle this in a way that's good enough to boot UEFI: a 
card insert interrupt is pending at power on, and goes away on enable/ack or 
reset. However, it deviates from the hardware in that disabling an interrupt 
(intstsen) implicity masks it out from the intsts (and this is true for any 
interrupt, not just card insertion). I think the "right" thing to do would be 
to add a bool to the controller state to explicity track the pending card 
insert interrupt, and check it (under a quirk property) when the interrupt 
changes from disabled->enabled. Do you concur?

Andrew