On 04/04/2018 08:57 AM, Peter Maydell wrote:
> On 4 April 2018 at 12:50, Philippe Mathieu-Daudé <f4...@amsat.org> 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 <peter.mayd...@linaro.org>
>>> ---
>>> 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.
Ok, thanks.
>
>>
>>> + 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
>
