[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 2/3] hw/sd: model a power-up delay, as a work
From: |
Peter Crosthwaite |
Subject: |
Re: [Qemu-devel] [PATCH v2 2/3] hw/sd: model a power-up delay, as a workaround for an EDK2 bug |
Date: |
Sun, 20 Dec 2015 14:57:36 -0800 |
On Wed, Dec 16, 2015 at 11:02 AM, Andrew Baumann
<address@hidden> 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 <address@hidden>
> ---
> 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
>
- [Qemu-devel] [PATCH v2 0/3] SD emulation fixes for Pi2 Tianocore EDK2 UEFI, Andrew Baumann, 2015/12/16
- [Qemu-devel] [PATCH v2 3/3] hw/sd: use guest error logging rather than fprintf to stderr, Andrew Baumann, 2015/12/16
- [Qemu-devel] [PATCH v2 1/3] hw/sd: implement CMD23 (SET_BLOCK_COUNT) for MMC compatibility, Andrew Baumann, 2015/12/16
- [Qemu-devel] [PATCH v2 2/3] hw/sd: model a power-up delay, as a workaround for an EDK2 bug, Andrew Baumann, 2015/12/16
- Re: [Qemu-devel] [PATCH v2 2/3] hw/sd: model a power-up delay, as a workaround for an EDK2 bug,
Peter Crosthwaite <=
- Re: [Qemu-devel] [PATCH v2 2/3] hw/sd: model a power-up delay, as a workaround for an EDK2 bug, Peter Crosthwaite, 2015/12/21
- Re: [Qemu-devel] [PATCH v2 2/3] hw/sd: model a power-up delay, as a workaround for an EDK2 bug, Peter Maydell, 2015/12/23
- Re: [Qemu-devel] [PATCH v2 2/3] hw/sd: model a power-up delay, as a workaround for an EDK2 bug, Andrew Baumann, 2015/12/23