On Fri, Dec 4, 2015 at 1:16 PM, Andrew Baumann
<andrew.baum...@microsoft.com> wrote:
> The SD spec for ACMD41 says that a zero argument is an "inquiry"
> ACMD41, which does not start initialisation and is used only for
> retrieving the OCR. However, Tianocore EDK2 (UEFI) has a bug [1]: it
> first sends an inquiry (zero) ACMD41. If that first request returns an
> OCR value with the power up bit (0x80000000) set, it assumes the card
> is ready and continues, leaving the card in the wrong state. (My
> assumption is that this works on hardware, because no real card is
> immediately powered up upon reset.)
>
> This change implements a simple workaround: the first time an ACMD41
> is issued, the OCR is returned without the power up bit set. Upon
> subsequent reads, the OCR reports power up.
>
So mandating two ACMD41's to get a power-up signal would introduce a
bug in guests with the opposite race condition. I.e. you are
facilitating a guest that (incorrectly) assumes a lower bound on the
card power up, but that could break a guest (incorrectly) assuming an
upper bound. The other broken code would look something like this:
do_card_power_on();
sleep(1);
if (!get_acmd_41_power_up()) {
abort(); /* Card did not power up in one second */
}
granted, this is a bad guest, but I think it can be solved both ways
by a more realistic model of the behaviour. Instead of using the
ACMD41 as the trigger for the state change, use a QEMU timer and model
an actual timed power up delay.
Regards,
Peter
> [1]
> https://github.com/tianocore/edk2/blob/master/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c#L279
> (This is the loop starting with "We need to wait for the MMC or SD
> card is ready")
>
> Signed-off-by: Andrew Baumann <andrew.baum...@microsoft.com>
> ---
> Obviously this is a bug that should be fixed in EDK2. However, this
> initialisation appears to have been around for quite a while in EDK2
> (in various forms), and the fact that it has obviously worked with so
> many real SD/MMC cards makes me think that it would be pragmatic to
> have the workaround in QEMU as well.
>
> hw/sd/sd.c | 30 ++++++++++++++++++++++++++----
> 1 file changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 98af8bc..31a25bc 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -80,6 +80,7 @@ struct SDState {
> uint32_t mode; /* current card mode, one of SDCardModes */
> int32_t state; /* current card state, one of SDCardStates */
> uint32_t ocr;
> + bool ocr_initdelay;
> uint8_t scr[8];
> uint8_t cid[16];
> uint8_t csd[16];
> @@ -195,8 +196,21 @@ static uint16_t sd_crc16(void *message, size_t width)
>
> static void sd_set_ocr(SDState *sd)
> {
> - /* All voltages OK, card power-up OK, Standard Capacity SD Memory Card */
> - sd->ocr = 0x80ffff00;
> + /* All voltages OK, Standard Capacity SD Memory Card, not yet powered up
> */
> + sd->ocr = 0x00ffff00;
> + sd->ocr_initdelay = false;
> +}
> +
> +static bool sd_model_ocr_init_delay(SDState *sd)
> +{
> + if (!sd->ocr_initdelay) {
> + sd->ocr_initdelay = true;
> + return false;
> + } else {
> + /* Set powered up bit in OCR */
> + sd->ocr |= 0x80000000;
> + return true;
> + }
> }
>
> static void sd_set_scr(SDState *sd)
> @@ -451,6 +465,7 @@ static const VMStateDescription sd_vmstate = {
> .fields = (VMStateField[]) {
> VMSTATE_UINT32(mode, SDState),
> VMSTATE_INT32(state, SDState),
> + VMSTATE_BOOL(ocr_initdelay, SDState),
> VMSTATE_UINT8_ARRAY(cid, SDState, 16),
> VMSTATE_UINT8_ARRAY(csd, SDState, 16),
> VMSTATE_UINT16(rca, SDState),
> @@ -1300,10 +1315,17 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
> case sd_idle_state:
> /* We accept any voltage. 10000 V is nothing.
> *
> - * We don't model init delay so just advance straight to ready
> state
> + * We model an init "delay" of one polling cycle, as a
> + * workaround for around a bug in TianoCore EDK2 which
> + * sends an initial zero ACMD41, but nevertheless assumes
> + * that the card is in ready state as soon as it sees the
> + * power up bit set.
> + *
> + * Once we've delayed, we advance straight to ready state
> * unless it's an enquiry ACMD41 (bits 23:0 == 0).
> */
> - if (req.arg & ACMD41_ENQUIRY_MASK) {
> + if (sd_model_ocr_init_delay(sd)
> + && (req.arg & ACMD41_ENQUIRY_MASK)) {
> sd->state = sd_ready_state;
> }
>
> --
> 2.5.3
>
>
