On Sat, 10 May 2025 at 07:57, <jc...@duck.com> wrote:
>
> This patch implements UART support for the MAX78000 SOC
>
> Signed-off-by: Jackson Donaldson <jc...@duck.com>
> ---
> hw/arm/Kconfig | 1 +
> hw/arm/max78000_soc.c | 27 +++-
> hw/char/Kconfig | 3 +
> hw/char/max78000_uart.c | 263 ++++++++++++++++++++++++++++++++
> hw/char/meson.build | 1 +
> include/hw/arm/max78000_soc.h | 3 +
> include/hw/char/max78000_uart.h | 77 ++++++++++
> 7 files changed, 370 insertions(+), 5 deletions(-)
> create mode 100644 hw/char/max78000_uart.c
> create mode 100644 include/hw/char/max78000_uart.h
Some of the comments I had on patch 2 apply also to this
and the other devices:
* separate patches for "new device" and "wire up new
device into the SoC"
* missing migration state support
* use DEVICE_LITTLE_ENDIAN
* set .impl.min_access_size and .impl_max_access_size
appropriately for the device
* make sure you've implemented a reset method (this device
has one but some of the later ones don't)
* fix case indent and excess braces if necessary
* make sure you're not using the _realize_and_unref functions
> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> index 3f23af3244..59450dc3cb 100644
> --- a/hw/arm/Kconfig
> +++ b/hw/arm/Kconfig
> @@ -368,6 +368,7 @@ config MAX78000_SOC
> bool
> select ARM_V7M
> select MAX78000_ICC
> + select MAX78000_UART
>
> config RASPI
> bool
> diff --git a/hw/arm/max78000_soc.c b/hw/arm/max78000_soc.c
> index 4d598bddd4..6334d8b49b 100644
> --- a/hw/arm/max78000_soc.c
> +++ b/hw/arm/max78000_soc.c
> @@ -16,7 +16,11 @@
> #include "hw/qdev-clock.h"
> #include "hw/misc/unimp.h"
>
> -static const uint32_t max78000_icc_addr[] = {0x4002a000, 0x4002a800};
> +static const uint32_t max78000_icc_addr[] = {0x4002a000, 0x4002a800};
> +static const uint32_t max78000_uart_addr[] = {0x40042000, 0x40043000,
> + 0x40044000};
> +
> +static const int max78000_uart_irq[] = {30, 31, 50};
The GPIO inputs to the ARM7M object are only the external
interrupt lines, so GPIO 0 is the first external interrupt,
which is 16 in the datasheet's Table 3-3. So you need to
subtract 16 from all the numbers in the table to get
the right GPIO index. The UARTs here are at 14, 15, 34.
>
> static void max78000_soc_initfn(Object *obj)
> {
> @@ -29,6 +33,10 @@ static void max78000_soc_initfn(Object *obj)
> object_initialize_child(obj, "icc[*]", &s->icc[i],
> TYPE_MAX78000_ICC);
> }
>
> + for (i = 0; i < MAX78000_NUM_UART; i++) {
> + object_initialize_child(obj, "uart[*]", &s->uart[i],
> TYPE_MAX78000_UART);
I didn't notice this for the icc patch, but rather than using the
same string for each UART, you can give them unique names with
g_autofree char *name = g_strdup_printf("uart%d", i);
object_initialize_child(obj, name, ...)
See eg hw/arm/stm32l4x5_soc.c for examples.
> + }
> +
> s->sysclk = qdev_init_clock_in(DEVICE(s), "sysclk", NULL, NULL, 0);
> s->refclk = qdev_init_clock_in(DEVICE(s), "refclk", NULL, NULL, 0);
> }
> @@ -38,6 +46,7 @@ static void max78000_soc_realize(DeviceState *dev_soc,
> Error **errp)
> MAX78000State *s = MAX78000_SOC(dev_soc);
> MemoryRegion *system_memory = get_system_memory();
> DeviceState *dev, *armv7m;
> + SysBusDevice *busdev;
> Error *err = NULL;
> int i;
>
> @@ -101,6 +110,18 @@ static void max78000_soc_realize(DeviceState *dev_soc,
> Error **errp)
> sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, max78000_icc_addr[i]);
> }
>
> + for (i = 0; i < MAX78000_NUM_UART; i++) {
> + dev = DEVICE(&(s->uart[i]));
> + qdev_prop_set_chr(dev, "chardev", serial_hd(i));
> + if (!sysbus_realize(SYS_BUS_DEVICE(&s->uart[i]), errp)) {
> + return;
> + }
> + busdev = SYS_BUS_DEVICE(dev);
> + sysbus_mmio_map(busdev, 0, max78000_uart_addr[i]);
> + sysbus_connect_irq(busdev, 0, qdev_get_gpio_in(armv7m,
> +
> max78000_uart_irq[i]));
> + }
> +
>
> create_unimplemented_device("globalControl", 0x40000000, 0x400);
> create_unimplemented_device("systemInterface", 0x40000400, 0x400);
> @@ -140,10 +161,6 @@ static void max78000_soc_realize(DeviceState *dev_soc,
> Error **errp)
> create_unimplemented_device("oneWireMaster", 0x4003d000, 0x1000);
> create_unimplemented_device("semaphore", 0x4003e000, 0x1000);
>
> - create_unimplemented_device("uart0", 0x40042000, 0x1000);
> - create_unimplemented_device("uart1", 0x40043000, 0x1000);
> - create_unimplemented_device("uart2", 0x40044000, 0x1000);
> -
> create_unimplemented_device("spi1", 0x40046000, 0x2000);
> create_unimplemented_device("trng", 0x4004d000, 0x1000);
> create_unimplemented_device("i2s", 0x40060000, 0x1000);
> diff --git a/hw/char/Kconfig b/hw/char/Kconfig
> index 9d517f3e28..020c0a84bb 100644
> --- a/hw/char/Kconfig
> +++ b/hw/char/Kconfig
> @@ -48,6 +48,9 @@ config VIRTIO_SERIAL
> default y
> depends on VIRTIO
>
> +config MAX78000_UART
> + bool
> +
> config STM32F2XX_USART
> bool
>
> diff --git a/hw/char/max78000_uart.c b/hw/char/max78000_uart.c
> new file mode 100644
> index 0000000000..edd39c5a8b
> --- /dev/null
> +++ b/hw/char/max78000_uart.c
> @@ -0,0 +1,263 @@
> +/*
> + * MAX78000 UART
> + *
> + * Copyright (c) 2025 Jackson Donaldson <jc...@duck.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/char/max78000_uart.h"
> +#include "hw/irq.h"
> +#include "hw/qdev-properties.h"
> +#include "hw/qdev-properties-system.h"
> +#include "qemu/log.h"
> +#include "qemu/module.h"
> +#include "trace.h"
> +
> +static int max78000_uart_can_receive(void *opaque)
> +{
> + Max78000UartState *s = opaque;
> + if (!(s->ctrl & UART_BCLKEN)) {
> + return 0;
> + }
> + return fifo8_num_free(&s->rx_fifo);
> +}
> +
> +static void max78000_update_irq(Max78000UartState *s)
> +{
> + int interrupt_level = 0;
> + uint32_t rx_threshold = s->ctrl & 0xf;
> +
> + /*
> + * Because tx is synchronous and we should have no frame errors, the only
> + * possible interrupt is receive fifo threshold
> + */
> + if ((s->int_en & UART_RX_THD) && fifo8_num_used(&s->rx_fifo) >=
> rx_threshold) {
> + interrupt_level = 1;
> + s->int_fl = s->int_fl & UART_RX_THD;
> + } else{
> + s->int_fl = s->int_fl & ~UART_RX_THD;
> + }
This looks a little odd. The usual pattern for interrupt
status bits is:
* when we notice the condition (in this case, when we receive
a char and it puts us above the rx threshold), set the
bit in the int_fl register
* option A: devices where the flag bit tracks the underlying
condition:
- when we notice that the condition is no longer true (in this
case, when the guest reads a char from the rx fifo and it
puts us below the rx threshold again), clear the int_fl bit,
* option B: devices where the flag bit latches and the guest must
explicitly clear it:
- no action when the condition is no longer true
The MAX78000 UART is an "option B" device -- see section 12.5.
* in the update_irq() function, set the interrupt if
the int_fl bit and the int_en bit are set, otherwise clear it.
If the bit definitions in the two registers line up, this is
as simple as
bool interrupt_level = s->int_fl & s->int_en;
If they don't line up then it needs a bit more manual work.
The code you have at the moment doesn't implement the "int_fl
bits latch and must be explicitly cleared by software" behaviour.
> + qemu_set_irq(s->irq, interrupt_level);
> +}
> +
> +static void max78000_uart_receive(void *opaque, const uint8_t *buf, int size)
> +{
> + Max78000UartState *s = opaque;
> +
> + if (size <= fifo8_num_free(&s->rx_fifo)) {
> + fifo8_push_all(&s->rx_fifo, buf, size);
> + } else{
> + fifo8_push_all(&s->rx_fifo, buf, fifo8_num_free(&s->rx_fifo));
> + printf("rx_fifo overrun!\n");
This should be a "can't happen" condition -- your receive
method will never be passed more bytes of data than you
said you could handle in can_receive.
Don't printf() in device code. Your options are:
* if it's something that the device spec says is
forbidden but a badly behaved guest can trigger,
use qemu_log(LOG_GUEST_ERROR, ...)
* if it's something that the device should implement but
we don't, use qemu_log(LOG_UNIMP, ...)
* if it's something that can't happen unless there's a bug
in QEMU, use some kind of assert()
* if it's nice-to-have information for debugging or for
a user to see what the device is doing, use a tracepoint
(trace_* functions, see docs/devel/tracing.rst)
>From your register write function:
> + case UART_INT_FL:
> + s->int_fl = value;
> + return;
This register's bits are W1C, i.e. write-1-to-clear.
thanks
-- PMM
