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(),
> + 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é <f4...@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);
>
