Re: [Qemu-devel] [PATCH v3 2/3] hw/sd: model a power-up delay, as a workaround for an EDK2 bug

[Andrew Baumann]

Hi Peter,

> From: Peter Maydell [mailto:address@hidden
> Sent: Thursday, 4 February 2016 1:22 AM
> 
> On 1 February 2016 at 22:15, 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/
> Mm
> > cDxe/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.
> 
> Have you filed it as an EDK2 bug, just out of interest?

No, I haven't. I didn't see an obvious path to do so; I'm also lazy :)

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

Makes sense. I was obviously copying something legacy. I will change this.

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

You're right. I think this was a hangover from the previous version. I'll clean 
it up.

> Otherwise
> Reviewed-by: Peter Maydell <address@hidden>

Thanks!
Andrew