[Qemu-devel] [PATCH for-2.12 0/2] bcm2835_sdhost: fix interrupt handling

[Qemu-devel] [PATCH for-2.12 0/2] bcm2835_sdhost: fix interrupt handling

[Peter Maydell]

This patchset fixes the code in the bcm2835_sdhost device
so that it raises interrupts in more plausible places.

The Linux bcm2835_sdhost driver doesn't work on QEMU at the moment,
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.

If anybody has other OSes than Linux that use the sdhost SD
controller (not the 'Arasan' one, which is a different bit of
the SoC) testing with those would be appreciated. If anybody
has documentation for this hardware that would be even better.

With these patches plus the previous set, I can get the Debian
raspi3 image to boot (though there are a lot of warnings relating
to the pl011 UART for some reason).

thanks
-- PMM

Peter Maydell (2):
  hw/sd/bcm2835_sdhost: Add tracepoints
  hw/sd/bcm2835_sdhost: Don't raise spurious interrupts

 hw/sd/bcm2835_sdhost.c | 51 +++++++++++++++++++++++++++++++-------------------
 hw/sd/trace-events     |  6 ++++++
 2 files changed, 38 insertions(+), 19 deletions(-)

-- 
2.16.2

[Qemu-devel] [PATCH for-2.12 1/2] hw/sd/bcm2835_sdhost: Add tracepoints

[Peter Maydell]

Add some tracepoints to the bcm2835_sdhost driver, to assist
debugging.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/sd/bcm2835_sdhost.c | 10 ++++++++++
 hw/sd/trace-events     |  6 ++++++
 2 files changed, 16 insertions(+)

diff --git a/hw/sd/bcm2835_sdhost.c b/hw/sd/bcm2835_sdhost.c
index f7f4e656df..79f3c5ceeb 100644
--- a/hw/sd/bcm2835_sdhost.c
+++ b/hw/sd/bcm2835_sdhost.c
@@ -15,6 +15,7 @@
 #include "qemu/log.h"
 #include "sysemu/blockdev.h"
 #include "hw/sd/bcm2835_sdhost.h"
+#include "trace.h"
 
 #define TYPE_BCM2835_SDHOST_BUS "bcm2835-sdhost-bus"
 #define BCM2835_SDHOST_BUS(obj) \
@@ -99,6 +100,7 @@ static void bcm2835_sdhost_update_irq(BCM2835SDHostState *s)
 {
     uint32_t irq = s->status &
         (SDHSTS_BUSY_IRPT | SDHSTS_BLOCK_IRPT | SDHSTS_SDIO_IRPT);
+    trace_bcm2835_sdhost_update_irq(irq);
     qemu_set_irq(s->irq, !!irq);
 }
 
@@ -211,6 +213,7 @@ static void bcm2835_sdhost_fifo_run(BCM2835SDHostState *s)
 
         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;
@@ -229,6 +232,7 @@ static void bcm2835_sdhost_fifo_run(BCM2835SDHostState *s)
 
     s->edm &= ~(0x1f << 4);
     s->edm |= ((s->fifo_len & 0x1f) << 4);
+    trace_bcm2835_sdhost_edm_change("fifo run", s->edm);
 }
 
 static uint64_t bcm2835_sdhost_read(void *opaque, hwaddr offset,
@@ -280,6 +284,8 @@ static uint64_t bcm2835_sdhost_read(void *opaque, hwaddr offset,
         break;
     }
 
+    trace_bcm2835_sdhost_read(offset, res, size);
+
     return res;
 }
 
@@ -288,6 +294,8 @@ static void bcm2835_sdhost_write(void *opaque, hwaddr offset,
 {
     BCM2835SDHostState *s = (BCM2835SDHostState *)opaque;
 
+    trace_bcm2835_sdhost_write(offset, value, size);
+
     switch (offset) {
     case SDCMD:
         s->cmd = value;
@@ -314,6 +322,7 @@ static void bcm2835_sdhost_write(void *opaque, hwaddr offset,
             value &= ~0xf;
         }
         s->edm = value;
+        trace_bcm2835_sdhost_edm_change("guest register write", s->edm);
         break;
     case SDHCFG:
         s->config = value;
@@ -390,6 +399,7 @@ static void bcm2835_sdhost_reset(DeviceState *dev)
     s->cmd = 0;
     s->cmdarg = 0;
     s->edm = 0x0000c60f;
+    trace_bcm2835_sdhost_edm_change("device reset", s->edm);
     s->config = 0;
     s->hbct = 0;
     s->hblc = 0;
diff --git a/hw/sd/trace-events b/hw/sd/trace-events
index 2059ace61f..bfd1d62efc 100644
--- a/hw/sd/trace-events
+++ b/hw/sd/trace-events
@@ -1,5 +1,11 @@
 # See docs/devel/tracing.txt for syntax documentation.
 
+# hw/sd/bcm2835_sdhost.c
+bcm2835_sdhost_read(uint64_t offset, uint64_t data, unsigned size) "offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u"
+bcm2835_sdhost_write(uint64_t offset, uint64_t data, unsigned size) "offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u"
+bcm2835_sdhost_edm_change(const char *why, uint32_t edm) "(%s) EDM now 0x%x"
+bcm2835_sdhost_update_irq(uint32_t irq) "IRQ bits 0x%x\n"
+
 # hw/sd/core.c
 sdbus_command(const char *bus_name, uint8_t cmd, uint32_t arg, uint8_t crc) "@%s CMD%02d arg 0x%08x crc 0x%02x"
 sdbus_read(const char *bus_name, uint8_t value) "@%s value 0x%02x"
-- 
2.16.2

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

[Peter Maydell]

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;
+                    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);
-- 
2.16.2

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

[Peter Maydell]

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

Re: [Qemu-devel] [Qemu-arm] [PATCH for-2.12 0/2] bcm2835_sdhost: fix interrupt handling

[Peter Maydell]

Ping for code review, please? It would be nice if our 'raspi3'
model for 2.12 was one we could say actually booted a known
Linux image...

thanks
-- PMM

On 19 March 2018 at 16:15, Peter Maydell <peter.maydell@linaro.org> wrote:
> This patchset fixes the code in the bcm2835_sdhost device
> so that it raises interrupts in more plausible places.
>
> The Linux bcm2835_sdhost driver doesn't work on QEMU at the moment,
> 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.
>
> If anybody has other OSes than Linux that use the sdhost SD
> controller (not the 'Arasan' one, which is a different bit of
> the SoC) testing with those would be appreciated. If anybody
> has documentation for this hardware that would be even better.
>
> With these patches plus the previous set, I can get the Debian
> raspi3 image to boot (though there are a lot of warnings relating
> to the pl011 UART for some reason).
>
> thanks
> -- PMM
>
> Peter Maydell (2):
>   hw/sd/bcm2835_sdhost: Add tracepoints
>   hw/sd/bcm2835_sdhost: Don't raise spurious interrupts
>
>  hw/sd/bcm2835_sdhost.c | 51 +++++++++++++++++++++++++++++++-------------------
>  hw/sd/trace-events     |  6 ++++++
>  2 files changed, 38 insertions(+), 19 deletions(-)
>
> --
> 2.16.2

Re: [Qemu-devel] [PATCH for-2.12 1/2] hw/sd/bcm2835_sdhost: Add tracepoints

[Philippe Mathieu-Daudé]

Hi Peter,

On 03/19/2018 01:15 PM, Peter Maydell wrote:
> Add some tracepoints to the bcm2835_sdhost driver, to assist
> debugging.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/sd/bcm2835_sdhost.c | 10 ++++++++++
>  hw/sd/trace-events     |  6 ++++++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/hw/sd/bcm2835_sdhost.c b/hw/sd/bcm2835_sdhost.c
> index f7f4e656df..79f3c5ceeb 100644
> --- a/hw/sd/bcm2835_sdhost.c
> +++ b/hw/sd/bcm2835_sdhost.c
> @@ -15,6 +15,7 @@
>  #include "qemu/log.h"
>  #include "sysemu/blockdev.h"
>  #include "hw/sd/bcm2835_sdhost.h"
> +#include "trace.h"
>  
>  #define TYPE_BCM2835_SDHOST_BUS "bcm2835-sdhost-bus"
>  #define BCM2835_SDHOST_BUS(obj) \
> @@ -99,6 +100,7 @@ static void bcm2835_sdhost_update_irq(BCM2835SDHostState *s)
>  {
>      uint32_t irq = s->status &
>          (SDHSTS_BUSY_IRPT | SDHSTS_BLOCK_IRPT | SDHSTS_SDIO_IRPT);
> +    trace_bcm2835_sdhost_update_irq(irq);
>      qemu_set_irq(s->irq, !!irq);
>  }
>  
> @@ -211,6 +213,7 @@ static void bcm2835_sdhost_fifo_run(BCM2835SDHostState *s)
>  
>          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;
> @@ -229,6 +232,7 @@ static void bcm2835_sdhost_fifo_run(BCM2835SDHostState *s)
>  
>      s->edm &= ~(0x1f << 4);
>      s->edm |= ((s->fifo_len & 0x1f) << 4);
> +    trace_bcm2835_sdhost_edm_change("fifo run", s->edm);
>  }
>  
>  static uint64_t bcm2835_sdhost_read(void *opaque, hwaddr offset,
> @@ -280,6 +284,8 @@ static uint64_t bcm2835_sdhost_read(void *opaque, hwaddr offset,
>          break;
>      }
>  
> +    trace_bcm2835_sdhost_read(offset, res, size);
> +
>      return res;
>  }
>  
> @@ -288,6 +294,8 @@ static void bcm2835_sdhost_write(void *opaque, hwaddr offset,
>  {
>      BCM2835SDHostState *s = (BCM2835SDHostState *)opaque;
>  
> +    trace_bcm2835_sdhost_write(offset, value, size);
> +
>      switch (offset) {
>      case SDCMD:
>          s->cmd = value;
> @@ -314,6 +322,7 @@ static void bcm2835_sdhost_write(void *opaque, hwaddr offset,
>              value &= ~0xf;
>          }
>          s->edm = value;
> +        trace_bcm2835_sdhost_edm_change("guest register write", s->edm);
>          break;
>      case SDHCFG:
>          s->config = value;
> @@ -390,6 +399,7 @@ static void bcm2835_sdhost_reset(DeviceState *dev)
>      s->cmd = 0;
>      s->cmdarg = 0;
>      s->edm = 0x0000c60f;
> +    trace_bcm2835_sdhost_edm_change("device reset", s->edm);
>      s->config = 0;
>      s->hbct = 0;
>      s->hblc = 0;
> diff --git a/hw/sd/trace-events b/hw/sd/trace-events
> index 2059ace61f..bfd1d62efc 100644
> --- a/hw/sd/trace-events
> +++ b/hw/sd/trace-events
> @@ -1,5 +1,11 @@
>  # See docs/devel/tracing.txt for syntax documentation.
>  
> +# hw/sd/bcm2835_sdhost.c
> +bcm2835_sdhost_read(uint64_t offset, uint64_t data, unsigned size) "offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u"
> +bcm2835_sdhost_write(uint64_t offset, uint64_t data, unsigned size) "offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u"

Can you use the more explicit "size_t" and "%zu" please?

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> +bcm2835_sdhost_edm_change(const char *why, uint32_t edm) "(%s) EDM now 0x%x"
> +bcm2835_sdhost_update_irq(uint32_t irq) "IRQ bits 0x%x\n"
> +
>  # hw/sd/core.c
>  sdbus_command(const char *bus_name, uint8_t cmd, uint32_t arg, uint8_t crc) "@%s CMD%02d arg 0x%08x crc 0x%02x"
>  sdbus_read(const char *bus_name, uint8_t value) "@%s value 0x%02x"
> 

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

[Philippe Mathieu-Daudé]

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);
> 

Re: [Qemu-devel] [PATCH for-2.12 1/2] hw/sd/bcm2835_sdhost: Add tracepoints

[Peter Maydell]

On 4 April 2018 at 12:42, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Hi Peter,
>
> On 03/19/2018 01:15 PM, Peter Maydell wrote:
>> Add some tracepoints to the bcm2835_sdhost driver, to assist
>> debugging.

>> +# hw/sd/bcm2835_sdhost.c
>> +bcm2835_sdhost_read(uint64_t offset, uint64_t data, unsigned size) "offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u"
>> +bcm2835_sdhost_write(uint64_t offset, uint64_t data, unsigned size) "offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u"
>
> Can you use the more explicit "size_t" and "%zu" please?

The argument to the read/write functions which we're tracing
here isn't a size_t, though (and since it's only ever 1/2/4/8
it makes sense that it isn't a size_t). What would be the
point in casting it to an size_t here?

A quick grep through hw/*/trace-events suggests we don't
use size_t for tracing of mmio read/write functions
in other devices.

thanks
-- PMM

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é <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

Re: [Qemu-devel] [Qemu-arm] [PATCH for-2.12 1/2] hw/sd/bcm2835_sdhost: Add tracepoints

[Philippe Mathieu-Daudé]

On 04/04/2018 08:54 AM, Peter Maydell wrote:
> On 4 April 2018 at 12:42, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> Hi Peter,
>>
>> On 03/19/2018 01:15 PM, Peter Maydell wrote:
>>> Add some tracepoints to the bcm2835_sdhost driver, to assist
>>> debugging.
> 
>>> +# hw/sd/bcm2835_sdhost.c
>>> +bcm2835_sdhost_read(uint64_t offset, uint64_t data, unsigned size) "offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u"
>>> +bcm2835_sdhost_write(uint64_t offset, uint64_t data, unsigned size) "offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u"
>>
>> Can you use the more explicit "size_t" and "%zu" please?
> 
> The argument to the read/write functions which we're tracing
> here isn't a size_t, though (and since it's only ever 1/2/4/8
> it makes sense that it isn't a size_t). What would be the
> point in casting it to an size_t here?>
> A quick grep through hw/*/trace-events suggests we don't
> use size_t for tracing of mmio read/write functions
> in other devices.

Oh... I find using 'unsigned' confusing, but that's fine this way.

Regards,

Phil.

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

[Philippe Mathieu-Daudé]

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
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH for-2.12 0/2] bcm2835_sdhost: fix interrupt handling

[Gerd Hoffmann]

On Wed, Apr 04, 2018 at 11:18:23AM +0100, Peter Maydell wrote:
> Ping for code review, please? It would be nice if our 'raspi3'
> model for 2.12 was one we could say actually booted a known
> Linux image...

The Fedora 27 image kernel (aarch64, 4.13) boots and mounts the root
filesystem.  This is an improvement, so:

Tested-by: Gerd Hoffmann <kraxel@redhat.com>

It doesn't finish boot though, at some point the kernel starts to
complain about timeouts and xfs throws io errors, so things are not
perfect yet.

raspi2 doesn't boot the fedora 27 armhfp kernel.  Looks unrelated
though, it fails with "Division by zero in kernel" before even loading
the sdhost driver.  I suspect the kernel tries to access some hardware
not emulated by qemu ...

Trying to run u-boot as boot-loader (so I don't have to copy the kernel
from the image) doesn't work too.

cheers,
  Gerd

Re: [Qemu-devel] [Qemu-arm] [PATCH for-2.12 0/2] bcm2835_sdhost: fix interrupt handling

[Peter Maydell]

On 4 April 2018 at 15:25, Gerd Hoffmann <kraxel@redhat.com> wrote:
> On Wed, Apr 04, 2018 at 11:18:23AM +0100, Peter Maydell wrote:
>> Ping for code review, please? It would be nice if our 'raspi3'
>> model for 2.12 was one we could say actually booted a known
>> Linux image...
>
> The Fedora 27 image kernel (aarch64, 4.13) boots and mounts the root
> filesystem.  This is an improvement, so:
>
> Tested-by: Gerd Hoffmann <kraxel@redhat.com>
>
> It doesn't finish boot though, at some point the kernel starts to
> complain about timeouts and xfs throws io errors, so things are not
> perfect yet.
>
> raspi2 doesn't boot the fedora 27 armhfp kernel.  Looks unrelated
> though, it fails with "Division by zero in kernel" before even loading
> the sdhost driver.  I suspect the kernel tries to access some hardware
> not emulated by qemu ...
>
> Trying to run u-boot as boot-loader (so I don't have to copy the kernel
> from the image) doesn't work too.

Thanks for the testing. I've applied this series to target-arm.next
for 2.12 (with minor tweaks as per code review).

I have a blog post pending which describes how to boot a Debian
image on the raspi3 model, which I'll publish once this hits
master.

thanks
-- PMM