Re: [PATCH v2 2/6] hw/char: Implement STM32L4x5 USART skeleton

[Peter Maydell <peter.maydell@linaro.org>]

On Sun, 24 Mar 2024 at 16:56, Arnaud Minier
<arnaud.minier@telecom-paris.fr> wrote:
>
> Add the basic infrastructure (register read/write, type...)
> to implement the STM32L4x5 USART.
>
> Also create different types for the USART, UART and LPUART
> of the STM32L4x5 to deduplicate code and enable the
> implementation of different behaviors depending on the type.
>
> Signed-off-by: Arnaud Minier <arnaud.minier@telecom-paris.fr>
> Signed-off-by: Inès Varhol <ines.varhol@telecom-paris.fr>
> ---
>  MAINTAINERS                       |   1 +
>  hw/char/Kconfig                   |   3 +
>  hw/char/meson.build               |   1 +
>  hw/char/stm32l4x5_usart.c         | 395 ++++++++++++++++++++++++++++++
>  hw/char/trace-events              |   4 +
>  include/hw/char/stm32l4x5_usart.h |  66 +++++
>  6 files changed, 470 insertions(+)
>  create mode 100644 hw/char/stm32l4x5_usart.c
>  create mode 100644 include/hw/char/stm32l4x5_usart.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 409d7db4d4..deba4a54ce 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1128,6 +1128,7 @@ M: Inès Varhol <ines.varhol@telecom-paris.fr>
>  L: qemu-arm@nongnu.org
>  S: Maintained
>  F: hw/arm/stm32l4x5_soc.c
> +F: hw/char/stm32l4x5_usart.c
>  F: hw/misc/stm32l4x5_exti.c
>  F: hw/misc/stm32l4x5_syscfg.c
>  F: hw/misc/stm32l4x5_rcc.c
> diff --git a/hw/char/Kconfig b/hw/char/Kconfig
> index 6b6cf2fc1d..4fd74ea878 100644
> --- a/hw/char/Kconfig
> +++ b/hw/char/Kconfig
> @@ -41,6 +41,9 @@ config VIRTIO_SERIAL
>  config STM32F2XX_USART
>      bool
>
> +config STM32L4X5_USART
> +    bool
> +
>  config CMSDK_APB_UART
>      bool
>
> diff --git a/hw/char/meson.build b/hw/char/meson.build
> index 006d20f1e2..e5b13b6958 100644
> --- a/hw/char/meson.build
> +++ b/hw/char/meson.build
> @@ -31,6 +31,7 @@ system_ss.add(when: 'CONFIG_RENESAS_SCI', if_true: files('renesas_sci.c'))
>  system_ss.add(when: 'CONFIG_SIFIVE_UART', if_true: files('sifive_uart.c'))
>  system_ss.add(when: 'CONFIG_SH_SCI', if_true: files('sh_serial.c'))
>  system_ss.add(when: 'CONFIG_STM32F2XX_USART', if_true: files('stm32f2xx_usart.c'))
> +system_ss.add(when: 'CONFIG_STM32L4X5_USART', if_true: files('stm32l4x5_usart.c'))
>  system_ss.add(when: 'CONFIG_MCHP_PFSOC_MMUART', if_true: files('mchp_pfsoc_mmuart.c'))
>  system_ss.add(when: 'CONFIG_HTIF', if_true: files('riscv_htif.c'))
>  system_ss.add(when: 'CONFIG_GOLDFISH_TTY', if_true: files('goldfish_tty.c'))
> diff --git a/hw/char/stm32l4x5_usart.c b/hw/char/stm32l4x5_usart.c
> new file mode 100644
> index 0000000000..46e69bb096
> --- /dev/null
> +++ b/hw/char/stm32l4x5_usart.c
> @@ -0,0 +1,395 @@
> +/*
> + * STM32L4X5 USART (Universal Synchronous Asynchronous Receiver Transmitter)
> + *
> + * Copyright (c) 2023 Arnaud Minier <arnaud.minier@telecom-paris.fr>
> + * Copyright (c) 2023 Inès Varhol <ines.varhol@telecom-paris.fr>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + * The STM32L4X5 USART is heavily inspired by the stm32f2xx_usart
> + * by Alistair Francis.
> + * The reference used is the STMicroElectronics RM0351 Reference manual
> + * for STM32L4x5 and STM32L4x6 advanced Arm ® -based 32-bit MCUs.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "qemu/module.h"
> +#include "qapi/error.h"
> +#include "chardev/char-fe.h"
> +#include "chardev/char-serial.h"
> +#include "migration/vmstate.h"
> +#include "hw/char/stm32l4x5_usart.h"
> +#include "hw/clock.h"
> +#include "hw/irq.h"
> +#include "hw/qdev-clock.h"
> +#include "hw/qdev-properties.h"
> +#include "hw/qdev-properties-system.h"
> +#include "hw/registerfields.h"
> +#include "trace.h"
> +
> +
> +REG32(CR1, 0x00)
> +    FIELD(CR1, M1, 28, 1)    /* Word length (part 2, see M0)*/

Missing space before "*/"

> +static const TypeInfo stm32l4x5_usart_types[] = {
> +    {
> +        .name           = TYPE_STM32L4X5_USART_BASE,
> +        .parent         = TYPE_SYS_BUS_DEVICE,
> +        .instance_size  = sizeof(Stm32l4x5UsartBaseState),
> +        .instance_init  = stm32l4x5_usart_base_init,
> +        .class_init     = stm32l4x5_usart_base_class_init,

This should also have
    .abstract = true,

so you can't create an instance of this class, only of
the specific subclasses.

> +    }, {
> +        .name           = TYPE_STM32L4X5_USART,
> +        .parent         = TYPE_STM32L4X5_USART_BASE,
> +        .class_init     = stm32l4x5_usart_class_init,
> +    }, {
> +        .name           = TYPE_STM32L4X5_UART,
> +        .parent         = TYPE_STM32L4X5_USART_BASE,
> +        .class_init     = stm32l4x5_uart_class_init,
> +    }, {
> +        .name           = TYPE_STM32L4X5_LPUART,
> +        .parent         = TYPE_STM32L4X5_USART_BASE,
> +        .class_init     = stm32l4x5_lpuart_class_init,
> +    }
> +};
> +

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM