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

[Peter Crosthwaite]

On Tue, Jan 5, 2016 at 10:36 AM, Andrew Baumann
<address@hidden> wrote:
>> 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.
>

Are there any issues with rPI WP bit?

>> 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?
>

Yes I think we need a separate new quirk for the bool for the penning.

The experimental results indicate that the device is completely
unresponsive to a run-time ejection or insertion event. This suggest
that we should follow through with more work on the no_eject quirk to
get the modelling right, but I guess the bool patch and the revert can
be a self contained first step and try again later with the no_eject
quirk.

When you add the penning bool, you will get a new penned insertion
event after the run-time reset. As long as your guest can handle this
(a generic driver should be able to as the card is actually inserted)
it should work fine.

Regards,
Peter

> Andrew
>