Re: [Qemu-arm] [PATCH 4/8] bcm2835_emmc: add bcm2835 MMC/SD controller

[Andrew Baumann]

> From: Peter Crosthwaite [mailto:address@hidden
> Sent: Tuesday, 8 December 2015 23:40
> On Tue, Dec 8, 2015 at 10:19 PM, Andrew Baumann
> <address@hidden> wrote:
> >> From: Peter Crosthwaite [mailto:address@hidden
> >> Sent: Saturday, 5 December 2015 21:26
> >> Is this IP just SDHCI? We already model SDHCI in QEMU, see
> >> hw/sd/sdhci.c. If there are RPi specific features to the SDHCI
> >> implementation they should be added as optional extensions (probabably
> >> via subclassing) to the existing SDHCI model.
> >
> > So yes, it turns out this is fairly similar to SDHCI (-> lots of wasted 
> > work by
> Gregory and me, sigh), and indeed Linux boots with the existing sdhci
> emulation. However, there are some quirks, and UEFI/Windows depend on
> them. Namely:
> >  * The host control registers (offset 0x28 and above) seem to differ
> significantly. Maybe this is due to the SDHC version -- according to the
> BCM2835 peripherals spec, the controller implements "Version 3.0 Draft 1.0"
> of the SDHC spec, but of course I can't find that spec online anywhere. 
> Luckily
> nothing seems to depend on this, besides a few spurious warnings about
> invalid writes.
> 
> Looks reasonably consistent from a quick scan? 0x28 in shdci.c is only
> doing the ADMA stuff while there are other fields on the BCM model.

You're right, upon closer examination, it's not as bad as I thought (just 
reserved / unimplemented bits). The one significant difference that seems 
likely to cause a problem is the first register (offset 0x0-0x3) which is ARG2 
on Pi (used for auto CMD 23 support) but the SDMA system address on SDHCI (Pi 
doesn't support DMA in the controller). This will break if a guest wants to use 
auto cmd 23 -- I've yet to see one that does, but Gregory's model implements 
it, so perhaps he did.

> >  * Power is assumed to be always on -- the sdhci model requires the guest
> to turn it on by a write at offset 0x29 before issuing any commands, but on pi
> this bit is marked reserved, and commands are issued immediately after
> reset.
> 
> Does this help?:
> 
> https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg06271.html

Yes, that's the same change I made. Is it going to be applied?

> >  * The card inserted interrupt is rather broken on pi: it is set at the 
> > start of
> day, but a reset command clears it and it stays clear thereafter (and never
> generates interrupts).
> >
> 
> Is that more likely to be an IP connectivity problem (wierd input to
> the card-detect pin in the SoC)?

That might be it, but in any case I still need to model it somehow.

> > There's an inconsistency with response handling, too, although I'm not sure
> if it's a quirk of the Pi or a general bug in sdhci. Pi UEFI sends a CMD23
> without setting any of the response bits, but this command does in fact
> generate a 4-byte R1 response. The question is whether this should be
> treated as an error, or whether it simply means that the host wants to ignore
> the response. In sdhci, the following code path (around line 246) raises a
> "command index" error in the case that a non-zero response is returned but
> no response bits were set in the command register:
> >
> >     } else if (rlen != 0 && (s->errintstsen & SDHC_EISEN_CMDIDX)) {
> >         s->errintsts |= SDHC_EIS_CMDIDX;
> >         s->norintsts |= SDHC_NIS_ERR;
> >     }
> >
> > I do not observe this behaviour on the real Pi2 (and it breaks UEFI). The
> hardware semantics appear to be "if the command generates a response,
> but you didn't want to see it, we'll successfully complete the command and
> ignore the response", whereas the sdhci implementation raises an error for
> this as well as signalling completion. I have read the "SD Specifications 
> Part A2
> SD Host Controller Simplified Specification Version 2.00", but did not find
> anything describing this case, so it could be that this is open to 
> interpretation.
> (It could also be specified in SDHC v3.) The specific error also seems odd -- 
> my
> understanding is that a "command index" error means that the index in the
> response didn't match the index of the issued command, but that's hardly
> what is happening here.
> >
> 
> Starting to sound like a bug.

I think so, yes. It was written this way in the original commit -- I'm adding 
the original author (Igor Mitsyanko) to the thread, in case he has any insight.

> > Assuming this latter bug can be fixed generically, how do you propose
> handling the Pi quirks? I could add a bool property for "bcm2835-quirks" or
> similar and just special-case the relevant code (my preferred approach). I'm
> also open to subclassing, but no idea how that would work in practice, so
> would need some pointers.
> >
> 
> I think we need a more definitive list of the register level features
> that are different or added, I am not seeing what is BCM specific just
> yet.

The complete diff needed to boot Windows appears below. The first hunk avoids 
re-triggering the insert interrupt on card reset, the second fixes the bug 
described above, and the last hunk is equivalent to your patch linked above. I 
propose that we make the first conditional under a suitably-named bool property 
to enable the quirk, and apply the second two fixes directly.

Thoughts?

Andrew

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index d70d1a6..877dd51 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -193,7 +193,7 @@ static void sdhci_reset(SDHCIState *s)
      * initialization */
     memset(&s->sdmasysad, 0, (uintptr_t)&s->capareg - 
(uintptr_t)&s->sdmasysad);

-    sd_set_cb(s->card, s->ro_cb, s->eject_cb);
+    //sd_set_cb(s->card, s->ro_cb, s->eject_cb);
     s->data_count = 0;
     s->stopped_state = sdhc_not_stopped;
 }
@@ -243,9 +243,11 @@ static void sdhci_send_command(SDHCIState *s)
             (s->cmdreg & SDHC_CMD_RESPONSE) == SDHC_CMD_RSP_WITH_BUSY) {
             s->norintsts |= SDHC_NIS_TRSCMP;
         }
+#if 0
     } else if (rlen != 0 && (s->errintstsen & SDHC_EISEN_CMDIDX)) {
         s->errintsts |= SDHC_EIS_CMDIDX;
         s->norintsts |= SDHC_NIS_ERR;
+#endif
     }

     if (s->norintstsen & SDHC_NISEN_CMDCMP) {
@@ -831,7 +833,7 @@ static void sdhci_data_transfer(void *opaque)

 static bool sdhci_can_issue_command(SDHCIState *s)
 {
-    if (!SDHC_CLOCK_IS_ON(s->clkcon) || !(s->pwrcon & SDHC_POWER_ON) ||
+    if (!SDHC_CLOCK_IS_ON(s->clkcon) || /* !(s->pwrcon & SDHC_POWER_ON) || */
         (((s->prnsts & SDHC_DATA_INHIBIT) || s->stopped_state) &&
         ((s->cmdreg & SDHC_CMD_DATA_PRESENT) ||
         ((s->cmdreg & SDHC_CMD_RESPONSE) == SDHC_CMD_RSP_WITH_BUSY &&