On 1 February 2016 at 22:15, 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.
Have you filed it as an EDK2 bug, just out of interest?
> -#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 */
It's kind of odd to have something here scaled by get_ticks_per_sec(),
but then later add it to a pure nanoseconds value. (It works because
get_ticks_per_sec() always returns a value indicating 1 tick per ns.)
I think it would be cleaner to:
* have this #define be a nanosecond value, with no call to
get_ticks_per_sec()
(we have a NANOSECONDS_PER_SECOND constant if you want it)
* call timer_mod_ns() rather than timer_mod()
The ticks-per-sec stuff is legacy which we don't need for new code.
> /* Legacy initialization function for use by non-qdevified callers */
> @@ -1320,12 +1371,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. */
> + 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;
> }
Isn't (sd->ocr & OCR_POWER_UP) redundant in this check? If
(req.arg & ACMD41_ENQUIRY_MASK) is true then either:
(a) OCR_POWER_UP was set when we came in to the function
(b) OCR_POWER_UP wasn't set, but we went through the code path that
deletes the timer and calls sd_ocr_powerup(), which will set
OCR_POWER_UP
So the enquiry-mask bits being nonzero here implies OCR_POWER_UP must
be set.
Otherwise
Reviewed-by: Peter Maydell <peter.mayd...@linaro.org>
thanks
-- PMM
