diff mbox series

[06/19] hw/char/bcm2835_aux: Add trace events

Message ID 20190926173428.10713-7-f4bug@amsat.org
State New
Headers show
Series hw/arm/raspi: Improve Raspberry Pi 2/3 reliability | expand

Commit Message

Philippe Mathieu-Daudé Sept. 26, 2019, 5:34 p.m. UTC
The BCM2835 AUX UART is compatible with the 16650 model, when
the registers belong the the 16650 block, use its trace events,
else use bcm2835_aux_read/write.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/char/bcm2835_aux.c | 55 +++++++++++++++++++++++++++++++------------
 hw/char/trace-events  |  4 ++++
 2 files changed, 44 insertions(+), 15 deletions(-)

Comments

Alex Bennée Oct. 8, 2019, 11:22 a.m. UTC | #1
Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> The BCM2835 AUX UART is compatible with the 16650 model, when
> the registers belong the the 16650 block, use its trace events,
> else use bcm2835_aux_read/write.

My only concern here is how we surface that detail to the potential
user. It's not a major thing as I assume most users of the trace points
are developers so maybe...
> diff --git a/hw/char/trace-events b/hw/char/trace-events
> index 2ce7f2f998..a7d477ab1e 100644
> --- a/hw/char/trace-events
> +++ b/hw/char/trace-events
> @@ -1,5 +1,9 @@
>  # See docs/devel/tracing.txt for syntax documentation.
>
> +# bcm2835_aux.c

"(accesses to the 16650 areas are logged via the serial_ioport_write tracepoint)"?

> +bcm2835_aux_read(uint64_t addr, uint32_t value) "read addr 0x%"PRIx64" value 0x%x"
> +bcm2835_aux_write(uint64_t addr, uint32_t value) "read addr 0x%"PRIx64" value 0x%x"
> +
>  # parallel.c
>  parallel_ioport_read(const char *desc, uint16_t addr, uint8_t value) "read [%s] addr 0x%02x val 0x%02x"
>  parallel_ioport_write(const char *desc, uint16_t addr, uint8_t value) "write [%s] addr 0x%02x val 0x%02x"

Otherwise:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

--
Alex Bennée
Peter Maydell Oct. 14, 2019, 3:36 p.m. UTC | #2
On Thu, 26 Sep 2019 at 18:34, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> The BCM2835 AUX UART is compatible with the 16650 model, when
> the registers belong the the 16650 block, use its trace events,
> else use bcm2835_aux_read/write.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---

> +    if (is_16650(offset)) {
> +        trace_serial_ioport_read((offset & 0x1f) >> 2, res);
> +    } else {
> +        trace_bcm2835_aux_read(offset, res);
> +    }

I'm not really a fan of this. I would expect that if I turn
on the trace point for reads from the device that I see all
the reads, not just a subset of them. The device may be
minimally software-compatible with a 16650, but it isn't actually
a 16650, and there doesn't seem to be much point in sharing
the serial_ioport_read() tracepoint.

thanks
-- PMM
Philippe Mathieu-Daudé Oct. 14, 2019, 4:03 p.m. UTC | #3
On 10/14/19 5:36 PM, Peter Maydell wrote:
> On Thu, 26 Sep 2019 at 18:34, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> The BCM2835 AUX UART is compatible with the 16650 model, when
>> the registers belong the the 16650 block, use its trace events,
>> else use bcm2835_aux_read/write.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
> 
>> +    if (is_16650(offset)) {
>> +        trace_serial_ioport_read((offset & 0x1f) >> 2, res);
>> +    } else {
>> +        trace_bcm2835_aux_read(offset, res);
>> +    }
> 
> I'm not really a fan of this. I would expect that if I turn
> on the trace point for reads from the device that I see all
> the reads, not just a subset of them. The device may be
> minimally software-compatible with a 16650, but it isn't actually
> a 16650, and there doesn't seem to be much point in sharing
> the serial_ioport_read() tracepoint.

Yes, I posted a newer series for this device after review comments:
hw/arm/raspi: Split the UART block from the AUX block
https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg01498.html

I forgot to mention here this patch was obsolete, sorry :/
diff mbox series

Patch

diff --git a/hw/char/bcm2835_aux.c b/hw/char/bcm2835_aux.c
index a6fc1bf152..b26a255630 100644
--- a/hw/char/bcm2835_aux.c
+++ b/hw/char/bcm2835_aux.c
@@ -27,6 +27,7 @@ 
 #include "migration/vmstate.h"
 #include "qemu/log.h"
 #include "qemu/module.h"
+#include "trace.h"
 
 #define AUX_IRQ         0x0
 #define AUX_ENABLES     0x4
@@ -62,17 +63,24 @@  static void bcm2835_aux_update(BCM2835AuxState *s)
     qemu_set_irq(s->irq, s->iir != 0);
 }
 
+static bool is_16650(hwaddr offset)
+{
+    return offset >= AUX_MU_IO_REG && offset < AUX_MU_CNTL_REG;
+}
+
 static uint64_t bcm2835_aux_read(void *opaque, hwaddr offset, unsigned size)
 {
     BCM2835AuxState *s = opaque;
-    uint32_t c, res;
+    uint32_t c, res = 0;
 
     switch (offset) {
     case AUX_IRQ:
-        return s->iir != 0;
+        res = s->iir != 0;
+        break;
 
     case AUX_ENABLES:
-        return 1; /* mini UART permanently enabled */
+        res = 1; /* mini UART permanently enabled */
+        break;
 
     case AUX_MU_IO_REG:
         /* "DLAB bit set means access baudrate register" is NYI */
@@ -85,11 +93,13 @@  static uint64_t bcm2835_aux_read(void *opaque, hwaddr offset, unsigned size)
         }
         qemu_chr_fe_accept_input(&s->chr);
         bcm2835_aux_update(s);
-        return c;
+        res = c;
+        break;
 
     case AUX_MU_IER_REG:
         /* "DLAB bit set means access baudrate register" is NYI */
-        return 0xc0 | s->ier; /* FIFO enables always read 1 */
+        res = 0xc0 | s->ier; /* FIFO enables always read 1 */
+        break;
 
     case AUX_MU_IIR_REG:
         res = 0xc0; /* FIFO enables */
@@ -105,33 +115,34 @@  static uint64_t bcm2835_aux_read(void *opaque, hwaddr offset, unsigned size)
         if (s->iir == 0) {
             res |= 0x1;
         }
-        return res;
+        break;
 
     case AUX_MU_LCR_REG:
         qemu_log_mask(LOG_UNIMP, "%s: AUX_MU_LCR_REG unsupported\n", __func__);
-        return 0;
+        break;
 
     case AUX_MU_MCR_REG:
         qemu_log_mask(LOG_UNIMP, "%s: AUX_MU_MCR_REG unsupported\n", __func__);
-        return 0;
+        break;
 
     case AUX_MU_LSR_REG:
         res = 0x60; /* tx idle, empty */
         if (s->read_count != 0) {
             res |= 0x1;
         }
-        return res;
+        break;
 
     case AUX_MU_MSR_REG:
         qemu_log_mask(LOG_UNIMP, "%s: AUX_MU_MSR_REG unsupported\n", __func__);
-        return 0;
+        break;
 
     case AUX_MU_SCRATCH:
         qemu_log_mask(LOG_UNIMP, "%s: AUX_MU_SCRATCH unsupported\n", __func__);
-        return 0;
+        break;
 
     case AUX_MU_CNTL_REG:
-        return 0x3; /* tx, rx enabled */
+        res = 0x3; /* tx, rx enabled */
+        break;
 
     case AUX_MU_STAT_REG:
         res = 0x30e; /* space in the output buffer, empty tx fifo, idle tx/rx */
@@ -140,17 +151,25 @@  static uint64_t bcm2835_aux_read(void *opaque, hwaddr offset, unsigned size)
             assert(s->read_count < BCM2835_AUX_RX_FIFO_LEN);
             res |= ((uint32_t)s->read_count) << 16; /* rx fifo fill level */
         }
-        return res;
+        break;
 
     case AUX_MU_BAUD_REG:
         qemu_log_mask(LOG_UNIMP, "%s: AUX_MU_BAUD_REG unsupported\n", __func__);
-        return 0;
+        break;
 
     default:
         qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset %"HWADDR_PRIx"\n",
                       __func__, offset);
-        return 0;
+        break;
     }
+
+    if (is_16650(offset)) {
+        trace_serial_ioport_read((offset & 0x1f) >> 2, res);
+    } else {
+        trace_bcm2835_aux_read(offset, res);
+    }
+
+    return res;
 }
 
 static void bcm2835_aux_write(void *opaque, hwaddr offset, uint64_t value,
@@ -159,6 +178,12 @@  static void bcm2835_aux_write(void *opaque, hwaddr offset, uint64_t value,
     BCM2835AuxState *s = opaque;
     unsigned char ch;
 
+    if (is_16650(offset)) {
+        trace_serial_ioport_write((offset & 0x1f) >> 2, value);
+    } else {
+        trace_bcm2835_aux_write(offset, value);
+    }
+
     switch (offset) {
     case AUX_ENABLES:
         if (value != 1) {
diff --git a/hw/char/trace-events b/hw/char/trace-events
index 2ce7f2f998..a7d477ab1e 100644
--- a/hw/char/trace-events
+++ b/hw/char/trace-events
@@ -1,5 +1,9 @@ 
 # See docs/devel/tracing.txt for syntax documentation.
 
+# bcm2835_aux.c
+bcm2835_aux_read(uint64_t addr, uint32_t value) "read addr 0x%"PRIx64" value 0x%x"
+bcm2835_aux_write(uint64_t addr, uint32_t value) "read addr 0x%"PRIx64" value 0x%x"
+
 # parallel.c
 parallel_ioport_read(const char *desc, uint16_t addr, uint8_t value) "read [%s] addr 0x%02x val 0x%02x"
 parallel_ioport_write(const char *desc, uint16_t addr, uint8_t value) "write [%s] addr 0x%02x val 0x%02x"