On Wed, Dec 16, 2015 at 11:02 AM, 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 models a delay of 0.5ms from the first ACMD41 to the power > being up. However, it also immediately sets the power on upon seeing a > non-zero (non-enquiry) ACMD41. This speeds up UEFI boot; it should > also account for guests that simply delay after card reset and then > issue an ACMD41 that they expect will succeed. > > [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. > > You might argue that the delay timer should start on sd_reset(), and > not the first ACMD41. However, that doesn't work reliably with UEFI, > because a large delay often elapses between the two (particularly in > debug builds that do lots of printing to the serial port). If the > timer fires too early, we'll still hit the bug, but we also don't want > to set a huge timeout value, because some guests may depend on it > expiring. > > hw/sd/sd.c | 45 ++++++++++++++++++++++++++++++++++++++++----- > 1 file changed, 40 insertions(+), 5 deletions(-) > > diff --git a/hw/sd/sd.c b/hw/sd/sd.c > index 1a24933..1011785 100644 > --- a/hw/sd/sd.c > +++ b/hw/sd/sd.c > @@ -33,6 +33,7 @@ > #include "sysemu/block-backend.h" > #include "hw/sd/sd.h" > #include "qemu/bitmap.h" > +#include "qemu/timer.h" > > //#define DEBUG_SD 1 > > @@ -43,7 +44,9 @@ do { fprintf(stderr, "SD: " fmt , ## __VA_ARGS__); } while > (0) > #define DPRINTF(fmt, ...) do {} while(0) > #endif > > -#define ACMD41_ENQUIRY_MASK 0x00ffffff > +#define ACMD41_ENQUIRY_MASK 0x00ffffff > +#define OCR_POWER_UP 0x80000000 > +#define OCR_POWER_DELAY (get_ticks_per_sec() / 2000) /* 0.5ms */ > > typedef enum { > sd_r0 = 0, /* no response */ > @@ -80,6 +83,7 @@ struct SDState { > uint32_t mode; /* current card mode, one of SDCardModes */ > int32_t state; /* current card state, one of SDCardStates */ > uint32_t ocr; > + QEMUTimer *ocr_power_timer; > uint8_t scr[8]; > uint8_t cid[16]; > uint8_t csd[16]; > @@ -194,8 +198,17 @@ 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; > +} > + > +static void sd_ocr_powerup(void *opaque) > +{ > + SDState *sd = opaque; > + > + /* Set powered up bit in OCR */ > + assert(!(sd->ocr & OCR_POWER_UP)); > + sd->ocr |= OCR_POWER_UP; > } > > static void sd_set_scr(SDState *sd) > @@ -449,6 +462,8 @@ static const VMStateDescription sd_vmstate = { > .fields = (VMStateField[]) { > VMSTATE_UINT32(mode, SDState), > VMSTATE_INT32(state, SDState), > + VMSTATE_UINT32(ocr, SDState), > + VMSTATE_TIMER_PTR(ocr_power_timer, SDState), > VMSTATE_UINT8_ARRAY(cid, SDState, 16), > VMSTATE_UINT8_ARRAY(csd, SDState, 16), > VMSTATE_UINT16(rca, SDState), > @@ -493,6 +508,7 @@ SDState *sd_init(BlockBackend *blk, bool is_spi) > sd->spi = is_spi; > sd->enable = true; > sd->blk = blk; > + sd->ocr_power_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sd_ocr_powerup, > sd); > sd_reset(sd); > if (sd->blk) { > /* Attach dev if not already attached. (This call ignores an > @@ -1295,12 +1311,31 @@ static sd_rsp_type_t sd_app_command(SDState *sd, > } > switch (sd->state) { > case sd_idle_state: > + /* If it's the first ACMD41 since reset, we need to decide > + * whether to power up. If this is not an enquiry ACMD41, > + * we immediately report power on and proceed below to the > + * ready state, but if it is, we set a timer to model a > + * delay for power up. This works around a bug in EDK2 > + * UEFI, which sends an initial enquiry ACMD41, but > + * assumes that the card is in ready state as soon as it > + * sees the power up bit set. */
Can this be done in a non-rPI specific way by just kicking off the timer on card reset? Regards, Peter > + if (!(sd->ocr & OCR_POWER_UP)) { > + if ((req.arg & ACMD41_ENQUIRY_MASK) != 0) { > + timer_del(sd->ocr_power_timer); > + sd_ocr_powerup(sd); > + } else if (!timer_pending(sd->ocr_power_timer)) { > + timer_mod(sd->ocr_power_timer, > + (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) > + + OCR_POWER_DELAY)); > + } > + } > + > /* We accept any voltage. 10000 V is nothing. > * > - * We don't model init delay so just advance straight to ready > state > + * Once we're powered up, we advance straight to ready state > * unless it's an enquiry ACMD41 (bits 23:0 == 0). > */ > - if (req.arg & ACMD41_ENQUIRY_MASK) { > + if ((sd->ocr & OCR_POWER_UP) && (req.arg & ACMD41_ENQUIRY_MASK)) > { > sd->state = sd_ready_state; > } > > -- > 2.5.3 >