> From: Peter Crosthwaite [mailto:crosthwaitepe...@gmail.com] > Sent: Tuesday, 8 December 2015 23:40 > On Tue, Dec 8, 2015 at 10:19 PM, Andrew Baumann > <andrew.baum...@microsoft.com> wrote: > >> From: Peter Crosthwaite [mailto:crosthwaitepe...@gmail.com] > >> 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 &&
