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

[Qemu-devel] [PATCH] 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 is relatively simple: if a card is present at power
on, remember that state as a "sticky" card insertion interrupt that
remains pending until enabled/cleared by the guest. We also need to
mask norintsts with norintstsen in sdhci_read, to avoid the interrupt
being visible before it is enabled.

Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
---
This is a little less savoury than the previous version. The real
ugliness here is that it can't be replaced by interposing on the card
insert interrupt between sd and sdhci (once they are refactored to
support it), because the interrupt needs to be "sticky" at the SDHCI
level.

I initially considered making all interrupts sticky in this way (i.e.,
unconditionally setting interrupts in the status register and masking
it with the enable register when read, but the SDHCI spec (section
1.8) is pretty clear that the interrupt status enable register acts as
a mask before the interrupt state is latched; i.e. if the interrupt
occurs but it is disabled, then it is forever lost.

One possible interpretation to explain what I'm seeing is that card
insertion is level-triggered, i.e. it is signalled as long as a card
remains inserted, but that would require a more extensive reworking of
the card insertion/removal interrupts logic, and would require yet
another quirk for item 3 above.

If anyone has suggestions for a cleaner fix, I'm all ears!

Thanks,
Andrew

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

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index dd83e89..6bce66a 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -892,7 +892,7 @@ static uint64_t sdhci_read(void *opaque, hwaddr offset, unsigned size)
         ret = s->clkcon | (s->timeoutcon << 16);
         break;
     case SDHC_NORINTSTS:
-        ret = s->norintsts | (s->errintsts << 16);
+        ret = (s->norintsts & s->norintstsen) | (s->errintsts << 16);
         break;
     case SDHC_NORINTSTSEN:
         ret = s->norintstsen | (s->errintstsen << 16);
@@ -1227,6 +1227,7 @@ 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("bcm2835-quirk", SDHCIState, bcm2835_quirk, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -1306,6 +1307,14 @@ static void sdhci_sysbus_realize(DeviceState *dev, Error ** errp)
     memory_region_init_io(&s->iomem, OBJECT(s), &sdhci_mmio_ops, s, "sdhci",
             SDHC_REGISTERS_MAP_SIZE);
     sysbus_init_mmio(sbd, &s->iomem);
+
+    /* Quirk for Raspberry Pi: set the card insert interrupt status.
+     * Needed to boot UEFI, which enables and then polls this bit at
+     * boot time before proceeding with card I/O.
+     */
+    if (s->bcm2835_quirk && (s->prnsts & SDHC_CARD_PRESENT)) {
+        s->norintsts |= SDHC_NIS_INSERT;
+    }
 }
 
 static void sdhci_sysbus_class_init(ObjectClass *klass, void *data)
diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index e78d938..76fdd9b 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -77,6 +77,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     bcm2835_quirk;/* Quirk for Raspberry Pi card insert status */
     /* 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] sdhci: add quirk property for card insert interrupt status on Raspberry Pi

[Peter Crosthwaite]

On Thu, Dec 31, 2015 at 1:40 PM, 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
>

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?

If prnsts works, then we can rule out all connectivity issues from the
IO pad to the controller.

> The implementation is relatively simple: if a card is present at power
> on, remember that state as a "sticky" card insertion interrupt that
> remains pending until enabled/cleared by the guest. We also need to
> mask norintsts with norintstsen in sdhci_read, to avoid the interrupt
> being visible before it is enabled.
>
> Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
> ---
> This is a little less savoury than the previous version. The real
> ugliness here is that it can't be replaced by interposing on the card
> insert interrupt between sd and sdhci (once they are refactored to
> support it), because the interrupt needs to be "sticky" at the SDHCI
> level.
>
> I initially considered making all interrupts sticky in this way (i.e.,
> unconditionally setting interrupts in the status register and masking
> it with the enable register when read, but the SDHCI spec (section
> 1.8) is pretty clear that the interrupt status enable register acts as
> a mask before the interrupt state is latched; i.e. if the interrupt
> occurs but it is disabled, then it is forever lost.
>

This mask-on-read theory is probably the most logical explanation.
Otherwise you need a new latch somewhere to explain the behaviour you
are seeing (assuming it is not level sensitive as you suggest below).
IIUC this still doesn't explain point 3 above though as this issue
would only create extra interrupts not cause misses. This has to be
two bugs right?

> One possible interpretation to explain what I'm seeing is that card
> insertion is level-triggered, i.e. it is signalled as long as a card
> remains inserted, but that would require a more extensive reworking of

That should be an easy test. If you just clear the interrupt w/o
disabling it does it insta-retrigger? It is really hard to explain the
one-shot symptom if this is the case however.

I can't see a single explanation for all your observations, so I'm
guessing that it is two bugs:

1: The mask-on-read behaviour (wrong by spec but you can live with it)
2: The edge detection logic on the card-detect pin is broken as a POR one-shot.

2 explains why bcm would defeature the interrupt in their docs.

Regards,
Peter

> the card insertion/removal interrupts logic, and would require yet
> another quirk for item 3 above.
>
> If anyone has suggestions for a cleaner fix, I'm all ears!
>
> Thanks,
> Andrew
>
>  hw/sd/sdhci.c         | 11 ++++++++++-
>  include/hw/sd/sdhci.h |  1 +
>  2 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index dd83e89..6bce66a 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -892,7 +892,7 @@ static uint64_t sdhci_read(void *opaque, hwaddr offset, unsigned size)
>          ret = s->clkcon | (s->timeoutcon << 16);
>          break;
>      case SDHC_NORINTSTS:
> -        ret = s->norintsts | (s->errintsts << 16);
> +        ret = (s->norintsts & s->norintstsen) | (s->errintsts << 16);
>          break;
>      case SDHC_NORINTSTSEN:
>          ret = s->norintstsen | (s->errintstsen << 16);
> @@ -1227,6 +1227,7 @@ 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("bcm2835-quirk", SDHCIState, bcm2835_quirk, false),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> @@ -1306,6 +1307,14 @@ static void sdhci_sysbus_realize(DeviceState *dev, Error ** errp)
>      memory_region_init_io(&s->iomem, OBJECT(s), &sdhci_mmio_ops, s, "sdhci",
>              SDHC_REGISTERS_MAP_SIZE);
>      sysbus_init_mmio(sbd, &s->iomem);
> +
> +    /* Quirk for Raspberry Pi: set the card insert interrupt status.
> +     * Needed to boot UEFI, which enables and then polls this bit at
> +     * boot time before proceeding with card I/O.
> +     */
> +    if (s->bcm2835_quirk && (s->prnsts & SDHC_CARD_PRESENT)) {
> +        s->norintsts |= SDHC_NIS_INSERT;
> +    }
>  }
>
>  static void sdhci_sysbus_class_init(ObjectClass *klass, void *data)
> diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
> index e78d938..76fdd9b 100644
> --- a/include/hw/sd/sdhci.h
> +++ b/include/hw/sd/sdhci.h
> @@ -77,6 +77,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     bcm2835_quirk;/* Quirk for Raspberry Pi card insert status */
>      /* 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] sdhci: add quirk property for card insert interrupt status on Raspberry Pi

[Andrew Baumann]

> From: Peter Crosthwaite [mailto:crosthwaitepeter@gmail.com]
> Sent: Thursday, 31 December 2015 21:38
> On Thu, Dec 31, 2015 at 1:40 PM, 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
> >
> 
> 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. 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.

I did once see a card interrupt (0x100, i.e. the one that comes from the card itself, not the controller) after re-inserting the card, but I think that's irrelevant.

It's impossible to boot the Pi without having a card inserted (well, maybe with a jtag debugger), but I did try inserting the card around 0.5s after applying power, and the results were the same.

So, without the prnsts bits, I can't confirm or deny your theory about debouncing logic, 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?

Andrew

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

[Peter Crosthwaite]

On Mon, Jan 4, 2016 at 2:12 PM, Andrew Baumann
<Andrew.Baumann@microsoft.com> wrote:
>> From: Peter Crosthwaite [mailto:crosthwaitepeter@gmail.com]
>> Sent: Thursday, 31 December 2015 21:38
>> On Thu, Dec 31, 2015 at 1:40 PM, 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
>> >
>>
>> 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.

>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
* you can never get a second ghost

This means you have two latches as there is no way it can be driven by
the raw pin, state, otherwise it would recur.

> I did once see a card interrupt (0x100, i.e. the one that comes from the card itself, not the controller) after re-inserting the card, but I think that's irrelevant.
>
> It's impossible to boot the Pi without having a card inserted (well, maybe with a jtag debugger), but I did try inserting the card around 0.5s after applying power, and the results were the same.
>
> So, without the prnsts bits, I can't confirm or deny your theory about debouncing logic,

It's disproven if you can never observe something other than 1FFF0000
for prnsts anyway.

> 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).

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?

Regards,
Peter

> Andrew

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

[Andrew Baumann]

> From: Peter Crosthwaite [mailto:crosthwaitepeter@gmail.com]
> Sent: Monday, 4 January 2016 22:18
> On Mon, Jan 4, 2016 at 2:12 PM, Andrew Baumann
> <Andrew.Baumann@microsoft.com> wrote:
> >> From: Peter Crosthwaite [mailto:crosthwaitepeter@gmail.com]
> >> Sent: Thursday, 31 December 2015 21:38
> >> On Thu, Dec 31, 2015 at 1:40 PM, 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
> >> >
> >>
> >> 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

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
<Andrew.Baumann@microsoft.com> wrote:
>> From: Peter Crosthwaite [mailto:crosthwaitepeter@gmail.com]
>> Sent: Monday, 4 January 2016 22:18
>> On Mon, Jan 4, 2016 at 2:12 PM, Andrew Baumann
>> <Andrew.Baumann@microsoft.com> wrote:
>> >> From: Peter Crosthwaite [mailto:crosthwaitepeter@gmail.com]
>> >> Sent: Thursday, 31 December 2015 21:38
>> >> On Thu, Dec 31, 2015 at 1:40 PM, 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
>> >> >
>> >>
>> >> 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
>

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

[Andrew Baumann]

> From: Peter Crosthwaite [mailto:crosthwaitepeter@gmail.com]
> Sent: Thursday, 7 January 2016 01:31
> On Tue, Jan 5, 2016 at 10:36 AM, Andrew Baumann
> <Andrew.Baumann@microsoft.com> wrote:
[...]
> Are there any issues with rPI WP bit?

It's documented as reserved and appears permanently set to 1, but there's no obvious way for me to test it since micro SD cards don't have R/O lock switches. I think the present behaviour is fine.

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

My guest (Windows/UEFI) can't handle this :( I will need to ensure that it is really once-only, and not re-issued on the controller reset.

I'll send a new patch for this in the next few days.

Thanks,
Andrew