| Message ID | 20180319161556.16446-3-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | bcm2835_sdhost: fix interrupt handling | expand |
On 19 March 2018 at 16:15, Peter Maydell <peter.maydell@linaro.org> 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.maydell@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: Oops, I had a comment here that should go above this if() that I failed to refresh into the patch before sending it: + /* We never really delay commands, so if this was a 'busywait' command + * then we've completed it now and can raise the interrupt. + */ thanks -- PMM
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.maydell@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(), > + 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. > + s->edm |= SDEDM_FSM_DATAMODE; > + trace_bcm2835_sdhost_edm_change("datacnt 0", s->edm); > + > + if ((s->cmd & SDCMD_WRITE_CMD) && > + (s->config & SDHCFG_BLOCK_IRPT_EN)) { > + s->status |= SDHSTS_BLOCK_IRPT; > + } > + } Your guesswork makes sens to me. Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > } > - if (s->datacnt == 0) { > - s->status |= SDHSTS_DATA_FLAG; > > - s->edm &= ~0xf; > - s->edm |= SDEDM_FSM_DATAMODE; > - trace_bcm2835_sdhost_edm_change("datacnt 0", s->edm); > - > - if (s->config & SDHCFG_DATA_IRPT_EN) { > - s->status |= SDHSTS_SDIO_IRPT; > - } > - > - if ((s->cmd & SDCMD_BUSYWAIT) && (s->config & SDHCFG_BUSY_IRPT_EN)) { > - s->status |= SDHSTS_BUSY_IRPT; > - } > - > - if ((s->cmd & SDCMD_WRITE_CMD) && (s->config & SDHCFG_BLOCK_IRPT_EN)) { > - s->status |= SDHSTS_BLOCK_IRPT; > - } > - > - bcm2835_sdhost_update_irq(s); > - } > + bcm2835_sdhost_update_irq(s); > > s->edm &= ~(0x1f << 4); > s->edm |= ((s->fifo_len & 0x1f) << 4); >
On 4 April 2018 at 12:50, Philippe Mathieu-Daudé <f4bug@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.maydell@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. > >> + 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
On 04/04/2018 08:57 AM, Peter Maydell wrote: > On 4 April 2018 at 12:50, Philippe Mathieu-Daudé <f4bug@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.maydell@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 >
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; + 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; } } 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; + s->edm |= SDEDM_FSM_DATAMODE; + trace_bcm2835_sdhost_edm_change("datacnt 0", s->edm); + + if ((s->cmd & SDCMD_WRITE_CMD) && + (s->config & SDHCFG_BLOCK_IRPT_EN)) { + s->status |= SDHSTS_BLOCK_IRPT; + } + } } - if (s->datacnt == 0) { - s->status |= SDHSTS_DATA_FLAG; - s->edm &= ~0xf; - s->edm |= SDEDM_FSM_DATAMODE; - trace_bcm2835_sdhost_edm_change("datacnt 0", s->edm); - - if (s->config & SDHCFG_DATA_IRPT_EN) { - s->status |= SDHSTS_SDIO_IRPT; - } - - if ((s->cmd & SDCMD_BUSYWAIT) && (s->config & SDHCFG_BUSY_IRPT_EN)) { - s->status |= SDHSTS_BUSY_IRPT; - } - - if ((s->cmd & SDCMD_WRITE_CMD) && (s->config & SDHCFG_BLOCK_IRPT_EN)) { - s->status |= SDHSTS_BLOCK_IRPT; - } - - bcm2835_sdhost_update_irq(s); - } + bcm2835_sdhost_update_irq(s); s->edm &= ~(0x1f << 4); s->edm |= ((s->fifo_len & 0x1f) << 4);
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.maydell@linaro.org> --- hw/sd/bcm2835_sdhost.c | 43 +++++++++++++++++++++++-------------------- 1 file changed, 23 insertions(+), 20 deletions(-) -- 2.16.2