[PATCH 0/5] hw/avr: Start using the Clock API

[PATCH 0/5] hw/avr: Start using the Clock API

[Philippe Mathieu-Daudé]

In this series we slowly start to use the recently added
Clock API in the AVR ATmega MCU.

As the Clock Control Unit is not yet modelled, we simply
connect the XTAL sink to the UART and Timer sources.

Philippe Mathieu-Daudé (5):
  hw/avr/atmega: Introduce the I/O clock
  hw/timer/avr_timer16: Use the Clock API
  hw/char/avr_usart: Restrict register definitions to source
  hw/char/avr_usart: Use the Clock API
  hw/char/avr_usart: Trace baudrate changes

 hw/avr/atmega.h                |  2 ++
 include/hw/char/avr_usart.h    | 32 ++---------------------
 include/hw/timer/avr_timer16.h |  3 ++-
 hw/avr/atmega.c                |  8 ++++--
 hw/char/avr_usart.c            | 46 ++++++++++++++++++++++++++++++++++
 hw/timer/avr_timer16.c         | 12 +++------
 hw/char/trace-events           |  3 +++
 7 files changed, 65 insertions(+), 41 deletions(-)

-- 
2.21.3

[PATCH 1/5] hw/avr/atmega: Introduce the I/O clock

[Philippe Mathieu-Daudé]

Use the Clock API to model the I/O clock. As we don't model
the Clock Control Unit, the XTAL is its unique clock source.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/avr/atmega.h | 2 ++
 hw/avr/atmega.c | 4 ++++
 2 files changed, 6 insertions(+)

diff --git a/hw/avr/atmega.h b/hw/avr/atmega.h
index 0928cb0ce6..c91317107f 100644
--- a/hw/avr/atmega.h
+++ b/hw/avr/atmega.h
@@ -14,6 +14,7 @@
 #include "hw/char/avr_usart.h"
 #include "hw/timer/avr_timer16.h"
 #include "hw/misc/avr_power.h"
+#include "hw/clock.h"
 #include "target/avr/cpu.h"
 
 #define TYPE_ATMEGA_MCU     "ATmega"
@@ -35,6 +36,7 @@ typedef struct AtmegaMcuState {
     /*< public >*/
 
     AVRCPU cpu;
+    Clock *ioclk;
     MemoryRegion flash;
     MemoryRegion eeprom;
     MemoryRegion sram;
diff --git a/hw/avr/atmega.c b/hw/avr/atmega.c
index 7131224431..9d814de499 100644
--- a/hw/avr/atmega.c
+++ b/hw/avr/atmega.c
@@ -15,6 +15,7 @@
 #include "exec/memory.h"
 #include "exec/address-spaces.h"
 #include "sysemu/sysemu.h"
+#include "hw/qdev-clock.h"
 #include "hw/qdev-properties.h"
 #include "hw/sysbus.h"
 #include "hw/boards.h" /* FIXME memory_region_allocate_system_memory for sram */
@@ -231,6 +232,9 @@ static void atmega_realize(DeviceState *dev, Error **errp)
         error_setg(errp, "\"xtal-frequency-hz\" property must be provided.");
         return;
     }
+    s->ioclk = qdev_init_clock_out(dev, "ioclk");
+    /* Clock Control Unit not implemented: directly distribute from xtal */
+    clock_set_hz(s->ioclk, s->xtal_freq_hz);
 
     /* CPU */
     object_initialize_child(OBJECT(dev), "cpu", &s->cpu, mc->cpu_type);
-- 
2.21.3

[PATCH 2/5] hw/timer/avr_timer16: Use the Clock API

[Philippe Mathieu-Daudé]

Expose the 'clkt' clock source. Connect the MCU I/O clock to it.
Drop the now unused 'cpu-frequency-hz' static property.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/timer/avr_timer16.h |  3 ++-
 hw/avr/atmega.c                |  3 +--
 hw/timer/avr_timer16.c         | 12 ++++--------
 3 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/include/hw/timer/avr_timer16.h b/include/hw/timer/avr_timer16.h
index 982019d242..fb1ef5d3be 100644
--- a/include/hw/timer/avr_timer16.h
+++ b/include/hw/timer/avr_timer16.h
@@ -31,6 +31,7 @@
 #include "hw/sysbus.h"
 #include "qemu/timer.h"
 #include "hw/hw.h"
+#include "hw/clock.h"
 
 enum NextInterrupt {
     OVERFLOW,
@@ -52,6 +53,7 @@ typedef struct AVRTimer16State {
     MemoryRegion iomem;
     MemoryRegion imsk_iomem;
     MemoryRegion ifr_iomem;
+    Clock *clkin;
     QEMUTimer *timer;
     qemu_irq capt_irq;
     qemu_irq compa_irq;
@@ -84,7 +86,6 @@ typedef struct AVRTimer16State {
     uint8_t ifr;
 
     uint8_t id;
-    uint64_t cpu_freq_hz;
     uint64_t freq_hz;
     uint64_t period_ns;
     uint64_t reset_time_ns;
diff --git a/hw/avr/atmega.c b/hw/avr/atmega.c
index 9d814de499..f14b558140 100644
--- a/hw/avr/atmega.c
+++ b/hw/avr/atmega.c
@@ -332,8 +332,7 @@ static void atmega_realize(DeviceState *dev, Error **errp)
         devname = g_strdup_printf("timer%zu", i);
         object_initialize_child(OBJECT(dev), devname, &s->timer[i],
                                 TYPE_AVR_TIMER16);
-        object_property_set_uint(OBJECT(&s->timer[i]), "cpu-frequency-hz",
-                                 s->xtal_freq_hz, &error_abort);
+        qdev_connect_clock_in(DEVICE(&s->timer[i]), "clkt", s->ioclk);
         sbd = SYS_BUS_DEVICE(&s->timer[i]);
         sysbus_realize(sbd, &error_abort);
         sysbus_mmio_map(sbd, 0, OFFSET_DATA + mc->dev[idx].addr);
diff --git a/hw/timer/avr_timer16.c b/hw/timer/avr_timer16.c
index c48555da52..7634fe6587 100644
--- a/hw/timer/avr_timer16.c
+++ b/hw/timer/avr_timer16.c
@@ -35,6 +35,7 @@
 #include "qapi/error.h"
 #include "qemu/log.h"
 #include "hw/irq.h"
+#include "hw/qdev-clock.h"
 #include "hw/qdev-properties.h"
 #include "hw/timer/avr_timer16.h"
 #include "trace.h"
@@ -167,7 +168,7 @@ static void avr_timer16_clksrc_update(AVRTimer16State *t16)
         break;
     }
     if (divider) {
-        t16->freq_hz = t16->cpu_freq_hz / divider;
+        t16->freq_hz = clock_get_hz(t16->clkin) / divider;
         t16->period_ns = NANOSECONDS_PER_SECOND / t16->freq_hz;
         trace_avr_timer16_clksrc_update(t16->freq_hz, t16->period_ns,
                                         (uint64_t)(1e6 / t16->freq_hz));
@@ -544,8 +545,6 @@ static const MemoryRegionOps avr_timer16_ifr_ops = {
 
 static Property avr_timer16_properties[] = {
     DEFINE_PROP_UINT8("id", struct AVRTimer16State, id, 0),
-    DEFINE_PROP_UINT64("cpu-frequency-hz", struct AVRTimer16State,
-                       cpu_freq_hz, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -564,6 +563,8 @@ static void avr_timer16_init(Object *obj)
 {
     AVRTimer16State *s = AVR_TIMER16(obj);
 
+    s->clkin = qdev_init_clock_in(DEVICE(obj), "clkt", NULL, s);
+
     sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->capt_irq);
     sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->compa_irq);
     sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->compb_irq);
@@ -587,11 +588,6 @@ static void avr_timer16_realize(DeviceState *dev, Error **errp)
 {
     AVRTimer16State *s = AVR_TIMER16(dev);
 
-    if (s->cpu_freq_hz == 0) {
-        error_setg(errp, "AVR timer16: cpu-frequency-hz property must be set");
-        return;
-    }
-
     s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, avr_timer16_interrupt, s);
     s->enabled = true;
 }
-- 
2.21.3

[PATCH 3/5] hw/char/avr_usart: Restrict register definitions to source

[Philippe Mathieu-Daudé]

Nothing out of our model implementation is supposed to access its
registers. Keep the definitions local.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/char/avr_usart.h | 30 ------------------------------
 hw/char/avr_usart.c         | 30 ++++++++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 30 deletions(-)

diff --git a/include/hw/char/avr_usart.h b/include/hw/char/avr_usart.h
index 5739aaf26f..46d6c76e50 100644
--- a/include/hw/char/avr_usart.h
+++ b/include/hw/char/avr_usart.h
@@ -26,36 +26,6 @@
 #include "chardev/char-fe.h"
 #include "hw/hw.h"
 
-/* Offsets of registers. */
-#define USART_DR   0x06
-#define USART_CSRA  0x00
-#define USART_CSRB  0x01
-#define USART_CSRC  0x02
-#define USART_BRRH 0x05
-#define USART_BRRL 0x04
-
-/* Relevant bits in regiters. */
-#define USART_CSRA_RXC    (1 << 7)
-#define USART_CSRA_TXC    (1 << 6)
-#define USART_CSRA_DRE    (1 << 5)
-#define USART_CSRA_MPCM   (1 << 0)
-
-#define USART_CSRB_RXCIE  (1 << 7)
-#define USART_CSRB_TXCIE  (1 << 6)
-#define USART_CSRB_DREIE  (1 << 5)
-#define USART_CSRB_RXEN   (1 << 4)
-#define USART_CSRB_TXEN   (1 << 3)
-#define USART_CSRB_CSZ2   (1 << 2)
-#define USART_CSRB_RXB8   (1 << 1)
-#define USART_CSRB_TXB8   (1 << 0)
-
-#define USART_CSRC_MSEL1  (1 << 7)
-#define USART_CSRC_MSEL0  (1 << 6)
-#define USART_CSRC_PM1    (1 << 5)
-#define USART_CSRC_PM0    (1 << 4)
-#define USART_CSRC_CSZ1   (1 << 2)
-#define USART_CSRC_CSZ0   (1 << 1)
-
 #define TYPE_AVR_USART "avr-usart"
 #define AVR_USART(obj) \
     OBJECT_CHECK(AVRUsartState, (obj), TYPE_AVR_USART)
diff --git a/hw/char/avr_usart.c b/hw/char/avr_usart.c
index fbe2a112b7..fd0b488ef9 100644
--- a/hw/char/avr_usart.c
+++ b/hw/char/avr_usart.c
@@ -25,6 +25,36 @@
 #include "hw/irq.h"
 #include "hw/qdev-properties.h"
 
+/* Offsets of registers. */
+#define USART_DR   0x06
+#define USART_CSRA  0x00
+#define USART_CSRB  0x01
+#define USART_CSRC  0x02
+#define USART_BRRH 0x05
+#define USART_BRRL 0x04
+
+/* Relevant bits in regiters. */
+#define USART_CSRA_RXC    (1 << 7)
+#define USART_CSRA_TXC    (1 << 6)
+#define USART_CSRA_DRE    (1 << 5)
+#define USART_CSRA_MPCM   (1 << 0)
+
+#define USART_CSRB_RXCIE  (1 << 7)
+#define USART_CSRB_TXCIE  (1 << 6)
+#define USART_CSRB_DREIE  (1 << 5)
+#define USART_CSRB_RXEN   (1 << 4)
+#define USART_CSRB_TXEN   (1 << 3)
+#define USART_CSRB_CSZ2   (1 << 2)
+#define USART_CSRB_RXB8   (1 << 1)
+#define USART_CSRB_TXB8   (1 << 0)
+
+#define USART_CSRC_MSEL1  (1 << 7)
+#define USART_CSRC_MSEL0  (1 << 6)
+#define USART_CSRC_PM1    (1 << 5)
+#define USART_CSRC_PM0    (1 << 4)
+#define USART_CSRC_CSZ1   (1 << 2)
+#define USART_CSRC_CSZ0   (1 << 1)
+
 static int avr_usart_can_receive(void *opaque)
 {
     AVRUsartState *usart = opaque;
-- 
2.21.3

[PATCH 4/5] hw/char/avr_usart: Use the Clock API

[Philippe Mathieu-Daudé]

Expose the 'xck' clock source. Connect the MCU I/O clock to it.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/char/avr_usart.h | 2 ++
 hw/avr/atmega.c             | 1 +
 hw/char/avr_usart.c         | 3 +++
 3 files changed, 6 insertions(+)

diff --git a/include/hw/char/avr_usart.h b/include/hw/char/avr_usart.h
index 46d6c76e50..13cfd5ea07 100644
--- a/include/hw/char/avr_usart.h
+++ b/include/hw/char/avr_usart.h
@@ -25,6 +25,7 @@
 #include "hw/sysbus.h"
 #include "chardev/char-fe.h"
 #include "hw/hw.h"
+#include "hw/clock.h"
 
 #define TYPE_AVR_USART "avr-usart"
 #define AVR_USART(obj) \
@@ -51,6 +52,7 @@ typedef struct {
     /* Baud Rate Registers (low/high byte) */
     uint8_t brrh;
     uint8_t brrl;
+    Clock *clkin;
 
     /* Receive Complete */
     qemu_irq rxc_irq;
diff --git a/hw/avr/atmega.c b/hw/avr/atmega.c
index f14b558140..b6e86a4531 100644
--- a/hw/avr/atmega.c
+++ b/hw/avr/atmega.c
@@ -302,6 +302,7 @@ static void atmega_realize(DeviceState *dev, Error **errp)
         object_initialize_child(OBJECT(dev), devname, &s->usart[i],
                                 TYPE_AVR_USART);
         qdev_prop_set_chr(DEVICE(&s->usart[i]), "chardev", serial_hd(i));
+        qdev_connect_clock_in(DEVICE(&s->usart[i]), "xck", s->ioclk);
         sbd = SYS_BUS_DEVICE(&s->usart[i]);
         sysbus_realize(sbd, &error_abort);
         sysbus_mmio_map(sbd, 0, OFFSET_DATA + mc->dev[USART(i)].addr);
diff --git a/hw/char/avr_usart.c b/hw/char/avr_usart.c
index fd0b488ef9..4a43492082 100644
--- a/hw/char/avr_usart.c
+++ b/hw/char/avr_usart.c
@@ -23,6 +23,7 @@
 #include "hw/char/avr_usart.h"
 #include "qemu/log.h"
 #include "hw/irq.h"
+#include "hw/qdev-clock.h"
 #include "hw/qdev-properties.h"
 
 /* Offsets of registers. */
@@ -307,12 +308,14 @@ static void avr_usart_pr(void *opaque, int irq, int level)
 static void avr_usart_init(Object *obj)
 {
     AVRUsartState *s = AVR_USART(obj);
+
     sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->rxc_irq);
     sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->dre_irq);
     sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->txc_irq);
     memory_region_init_io(&s->mmio, obj, &avr_usart_ops, s, TYPE_AVR_USART, 7);
     sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mmio);
     qdev_init_gpio_in(DEVICE(s), avr_usart_pr, 1);
+    s->clkin = qdev_init_clock_in(DEVICE(obj), "xck", NULL, s);
     s->enabled = true;
 }
 
-- 
2.21.3

[PATCH 5/5] hw/char/avr_usart: Trace baudrate changes

[Philippe Mathieu-Daudé]

Add a trace event to track baudrate changes.

Example when running the FreeRTOS acceptance test [1]:

  $ qemu-system-avr -machine arduino-mega-2560-v3 -bios demo.elf -trace avr\*
  2546@1597415281.399619:avr_usart_update_baudrate baudrate 0x0019 (38461 bauds)
  2546@1597415281.400029:avr_usart_update_baudrate baudrate 0x0019 (38461 bauds)

Which confirm the definition from the test [2]:

  #define mainCOM_TEST_BAUD_RATE        ( ( unsigned long ) 38400 )

[1] tests/acceptance/machine_avr6.py
[2] https://github.com/seharris/qemu-avr-tests/blob/9c0c24da1b1/free-rtos/Demo/AVR_ATMega2560_GCC/main.c#L80

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/char/avr_usart.c  | 13 +++++++++++++
 hw/char/trace-events |  3 +++
 2 files changed, 16 insertions(+)

diff --git a/hw/char/avr_usart.c b/hw/char/avr_usart.c
index 4a43492082..176158a96b 100644
--- a/hw/char/avr_usart.c
+++ b/hw/char/avr_usart.c
@@ -25,6 +25,7 @@
 #include "hw/irq.h"
 #include "hw/qdev-clock.h"
 #include "hw/qdev-properties.h"
+#include "trace.h"
 
 /* Offsets of registers. */
 #define USART_DR   0x06
@@ -56,6 +57,8 @@
 #define USART_CSRC_CSZ1   (1 << 2)
 #define USART_CSRC_CSZ0   (1 << 1)
 
+#define USART_CLOCK_DIVISOR  16      /* baudrate is input clock / 16 */
+
 static int avr_usart_can_receive(void *opaque)
 {
     AVRUsartState *usart = opaque;
@@ -120,6 +123,14 @@ static void update_char_mask(AVRUsartState *usart)
     }
 }
 
+static void avr_usart_update_baudrate(AVRUsartState *s)
+{
+    unsigned baudrate = (clock_get_hz(s->clkin) / USART_CLOCK_DIVISOR)
+                        / (((s->brrh << 8) | s->brrl) + 1);
+
+    trace_avr_usart_update_baudrate((s->brrh << 8) | s->brrl, baudrate);
+}
+
 static void avr_usart_reset(DeviceState *dev)
 {
     AVRUsartState *usart = AVR_USART(dev);
@@ -269,9 +280,11 @@ static void avr_usart_write(void *opaque, hwaddr addr, uint64_t value,
         break;
     case USART_BRRL:
         usart->brrl = value;
+        avr_usart_update_baudrate(usart);
         break;
     case USART_BRRH:
         usart->brrh = value & 0b00001111;
+        avr_usart_update_baudrate(usart);
         break;
     default:
         qemu_log_mask(
diff --git a/hw/char/trace-events b/hw/char/trace-events
index d20eafd56f..b92cecbfaa 100644
--- a/hw/char/trace-events
+++ b/hw/char/trace-events
@@ -100,3 +100,6 @@ exynos_uart_rx_timeout(uint32_t channel, uint32_t stat, uint32_t intsp) "UART%d:
 
 # hw/char/cadence_uart.c
 cadence_uart_baudrate(unsigned baudrate) "baudrate %u"
+
+# avr_usart.c
+avr_usart_update_baudrate(uint16_t regval, unsigned baudrate) "baudrate 0x%04x (%u bauds)"
-- 
2.21.3

Re: [PATCH 5/5] hw/char/avr_usart: Trace baudrate changes

[Richard Henderson]

On 8/14/20 9:39 AM, Philippe Mathieu-Daudé wrote:
> +static void avr_usart_update_baudrate(AVRUsartState *s)
> +{
> +    unsigned baudrate = (clock_get_hz(s->clkin) / USART_CLOCK_DIVISOR)
> +                        / (((s->brrh << 8) | s->brrl) + 1);
> +
> +    trace_avr_usart_update_baudrate((s->brrh << 8) | s->brrl, baudrate);

Would you pull that brrh|brrl expression out and give it a name?

I do wonder if one division would be better, e.g.

    baudrate = clock_get_hz / (DIVISOR * (regval + 1))


r~

Re: [PATCH 0/5] hw/avr: Start using the Clock API

[Michael Rolnik]

[-- Attachment #1: Type: text/plain, Size: 1114 bytes --]

Tested-by: Michael Rolnik <mrolnik@gmail.com>

On Fri, Aug 14, 2020 at 7:39 PM Philippe Mathieu-Daudé <f4bug@amsat.org>
wrote:

> In this series we slowly start to use the recently added
> Clock API in the AVR ATmega MCU.
>
> As the Clock Control Unit is not yet modelled, we simply
> connect the XTAL sink to the UART and Timer sources.
>
> Philippe Mathieu-Daudé (5):
>   hw/avr/atmega: Introduce the I/O clock
>   hw/timer/avr_timer16: Use the Clock API
>   hw/char/avr_usart: Restrict register definitions to source
>   hw/char/avr_usart: Use the Clock API
>   hw/char/avr_usart: Trace baudrate changes
>
>  hw/avr/atmega.h                |  2 ++
>  include/hw/char/avr_usart.h    | 32 ++---------------------
>  include/hw/timer/avr_timer16.h |  3 ++-
>  hw/avr/atmega.c                |  8 ++++--
>  hw/char/avr_usart.c            | 46 ++++++++++++++++++++++++++++++++++
>  hw/timer/avr_timer16.c         | 12 +++------
>  hw/char/trace-events           |  3 +++
>  7 files changed, 65 insertions(+), 41 deletions(-)
>
> --
> 2.21.3
>
>

-- 
Best Regards,
Michael Rolnik

[-- Attachment #2: Type: text/html, Size: 1678 bytes --]