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

[Peter Crosthwaite]

On Wed, Dec 9, 2015 at 10:17 AM, Andrew Baumann
<address@hidden> wrote:
>> 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?
>

It missed the boat for 2.5, but you could help by putting a tested-by
or reviewed-by to the patch.

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

Ideally, this would be managed on the board level but it is kinda hard
the way to code is organised. Currently SDHCI is managing the SD card
instantiation due to a lack of QOMification. So for the moment, this
would be valid as a boolean property which disables the card detect
logic completely. If we get the SD QOMification though, the power,
card detect and sd interface proper can then be more finely managed on
the board level for all these varying connectivity configurations.

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

Is this a runtime reset or an initial reset that is causing you grief?
You patch is also patching the controller reset so is it more a case
of the interrupt trigger on controller reset that is causing you
issues?

Regards,
Peter

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