Re: [Qemu-devel] [PATCH for-2.12 2/2] hw/sd/bcm2835_sdhost: Don't raise spurious interrupts

[Peter Maydell]

On 4 April 2018 at 12:50, Philippe Mathieu-Daudé <address@hidden> wrote:
> Hi Peter,
>
> On 03/19/2018 01:15 PM, Peter Maydell wrote:
>> The Linux bcm2835_sdhost driver doesn't work on QEMU, because our
>> model raises spurious data interrupts.  Our function
>> bcm2835_sdhost_fifo_run() will flag an interrupt any time it is
>> called with s->datacnt == 0, even if the host hasn't actually issued
>> a data read or write command yet.  This means that the driver gets a
>> spurious data interrupt as soon as it enables IRQs and then does
>> something else that causes us to call the fifo_run routine, like
>> writing to SDHCFG, and before it does the write to SDCMD to issue the
>> read.  The driver's IRQ handler then spins forever complaining that
>> there's no data and the SD controller isn't in a state where there's
>> going to be any data:
>>
>> [   41.040738] sdhost-bcm2835 3f202000.mmc: fsm 1, hsts 00000000
>> [   41.042059] sdhost-bcm2835 3f202000.mmc: fsm 1, hsts 00000000
>> (continues forever).
>>
>> Move the interrupt flag setting to more plausible places:
>>  * for BUSY, raise this as soon as a BUSYWAIT command has executed
>>  * for DATA, raise this when the FIFO has any space free (for a write)
>>    or any data in it (for a read)
>>  * for BLOCK, raise this when the data count is 0 and we've
>>    actually done some reading or writing
>>
>> This is pure guesswork since the documentation for this hardware is
>> not public, but it is sufficient to get the Linux bcm2835_sdhost
>> driver to work.
>>
>> Signed-off-by: Peter Maydell <address@hidden>
>> ---
>>  hw/sd/bcm2835_sdhost.c | 43 +++++++++++++++++++++++--------------------
>>  1 file changed, 23 insertions(+), 20 deletions(-)
>>
>> diff --git a/hw/sd/bcm2835_sdhost.c b/hw/sd/bcm2835_sdhost.c
>> index 79f3c5ceeb..0fd0853fa3 100644
>> --- a/hw/sd/bcm2835_sdhost.c
>> +++ b/hw/sd/bcm2835_sdhost.c
>> @@ -137,6 +137,9 @@ static void 
>> bcm2835_sdhost_send_command(BCM2835SDHostState *s)
>>          }
>>  #undef RWORD
>>      }
>> +    if ((s->cmd & SDCMD_BUSYWAIT) && (s->config & SDHCFG_BUSY_IRPT_EN)) {
>> +        s->status |= SDHSTS_BUSY_IRPT;
>> +    }
>>      return;
>>
>>  error:
>> @@ -187,18 +190,27 @@ static void bcm2835_sdhost_fifo_run(BCM2835SDHostState 
>> *s)
>>                  n++;
>>                  if (n == 4) {
>>                      bcm2835_sdhost_fifo_push(s, value);
>> +                    s->status |= SDHSTS_DATA_FLAG;
>
> ^ I'd move this line in bcm2835_sdhost_fifo_push(),

The bcm2835_sdhost_fifo_push() function is also used when
pushing data into the FIFO from the guest, though
(in the handling of writes to the SDDATA register), and
we don't want to set the DATA flag in that case I think.
So we need to set the flag only at the callsites where
it's the SD card pushing data into (or removing it from)
the FIFO.

>
>> +                    if (s->config & SDHCFG_DATA_IRPT_EN) {
>> +                        s->status |= SDHSTS_SDIO_IRPT;
>> +                    }
>>                      n = 0;
>>                      value = 0;
>>                  }
>>              }
>>              if (n != 0) {
>>                  bcm2835_sdhost_fifo_push(s, value);
>> +                s->status |= SDHSTS_DATA_FLAG;
>
> removing this one.
>
>>              }
>>          } else { /* write */
>>              n = 0;
>>              while (s->datacnt > 0 && (s->fifo_len > 0 || n > 0)) {
>>                  if (n == 0) {
>>                      value = bcm2835_sdhost_fifo_pop(s);
>> +                    s->status |= SDHSTS_DATA_FLAG;
>> +                    if (s->config & SDHCFG_DATA_IRPT_EN) {
>> +                        s->status |= SDHSTS_SDIO_IRPT;
>> +                    }
>>                      n = 4;
>>                  }
>>                  n--;
>> @@ -207,28 +219,19 @@ static void bcm2835_sdhost_fifo_run(BCM2835SDHostState 
>> *s)
>>                  value >>= 8;
>>              }
>>          }
>> +        if (s->datacnt == 0) {
>> +            s->edm &= ~0xf;
>
> while here, let's use SDEDM_FSM_MASK.

Sure.

thanks
-- PMM