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

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 <address@hidden>
> ---
> 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
>