[PATCH 0/4] Check clock connection between STM32L4x5 RCC and peripherals

[PATCH 0/4] Check clock connection between STM32L4x5 RCC and peripherals

[Inès Varhol]

Among implemented STM32L4x5 devices, USART, GPIO and SYSCFG
have a clock source, but none has a corresponding test in QEMU.

This patch makes sure that all 3 devices create a clock,
have a QOM property to access the clock frequency,
and adds QTests checking that clock enable in RCC has the
expected results.

Signed-off-by: Inès Varhol <ines.varhol@telecom-paris.fr>

Inès Varhol (4):
  hw/misc: Create STM32L4x5 SYSCFG clock
  hw/gpio: Handle clock migration in STM32L4x5 gpios
  hw/char: Add QOM property for STM32L4x5 USART clock frequency
  tests/qtest: Check STM32L4x5 clock connections

 include/hw/misc/stm32l4x5_syscfg.h  |  1 +
 hw/arm/stm32l4x5_soc.c              |  2 ++
 hw/char/stm32l4x5_usart.c           | 12 ++++++++
 hw/gpio/stm32l4x5_gpio.c            |  2 ++
 hw/misc/stm32l4x5_syscfg.c          | 26 ++++++++++++++++
 tests/qtest/stm32l4x5_gpio-test.c   | 39 +++++++++++++++++++++++
 tests/qtest/stm32l4x5_syscfg-test.c | 38 +++++++++++++++++++++--
 tests/qtest/stm32l4x5_usart-test.c  | 48 +++++++++++++++++++++++++++++
 8 files changed, 166 insertions(+), 2 deletions(-)

-- 
2.43.2

[PATCH 1/4] hw/misc: Create STM32L4x5 SYSCFG clock

[Inès Varhol]

Signed-off-by: Inès Varhol <ines.varhol@telecom-paris.fr>
---
 include/hw/misc/stm32l4x5_syscfg.h |  1 +
 hw/arm/stm32l4x5_soc.c             |  2 ++
 hw/misc/stm32l4x5_syscfg.c         | 26 ++++++++++++++++++++++++++
 3 files changed, 29 insertions(+)

diff --git a/include/hw/misc/stm32l4x5_syscfg.h b/include/hw/misc/stm32l4x5_syscfg.h
index 23bb564150..c450df2b9e 100644
--- a/include/hw/misc/stm32l4x5_syscfg.h
+++ b/include/hw/misc/stm32l4x5_syscfg.h
@@ -48,6 +48,7 @@ struct Stm32l4x5SyscfgState {
     uint32_t swpr2;
 
     qemu_irq gpio_out[GPIO_NUM_PINS];
+    Clock *clk;
 };
 
 #endif
diff --git a/hw/arm/stm32l4x5_soc.c b/hw/arm/stm32l4x5_soc.c
index 38f7a2d5d9..fb2afa6cfe 100644
--- a/hw/arm/stm32l4x5_soc.c
+++ b/hw/arm/stm32l4x5_soc.c
@@ -236,6 +236,8 @@ static void stm32l4x5_soc_realize(DeviceState *dev_soc, Error **errp)
 
     /* System configuration controller */
     busdev = SYS_BUS_DEVICE(&s->syscfg);
+    qdev_connect_clock_in(DEVICE(&s->syscfg), "clk",
+        qdev_get_clock_out(DEVICE(&(s->rcc)), "syscfg-out"));
     if (!sysbus_realize(busdev, errp)) {
         return;
     }
diff --git a/hw/misc/stm32l4x5_syscfg.c b/hw/misc/stm32l4x5_syscfg.c
index a5a1ce2680..a82864c33d 100644
--- a/hw/misc/stm32l4x5_syscfg.c
+++ b/hw/misc/stm32l4x5_syscfg.c
@@ -26,6 +26,10 @@
 #include "trace.h"
 #include "hw/irq.h"
 #include "migration/vmstate.h"
+#include "hw/clock.h"
+#include "hw/qdev-clock.h"
+#include "qapi/visitor.h"
+#include "qapi/error.h"
 #include "hw/misc/stm32l4x5_syscfg.h"
 #include "hw/gpio/stm32l4x5_gpio.h"
 
@@ -202,6 +206,14 @@ static void stm32l4x5_syscfg_write(void *opaque, hwaddr addr,
     }
 }
 
+static void clock_freq_get(Object *obj, Visitor *v,
+    const char *name, void *opaque, Error **errp)
+{
+    Stm32l4x5SyscfgState *s = STM32L4X5_SYSCFG(obj);
+    uint32_t clock_freq_hz = clock_get_hz(s->clk);
+    visit_type_uint32(v, name, &clock_freq_hz, errp);
+}
+
 static const MemoryRegionOps stm32l4x5_syscfg_ops = {
     .read = stm32l4x5_syscfg_read,
     .write = stm32l4x5_syscfg_write,
@@ -225,6 +237,18 @@ static void stm32l4x5_syscfg_init(Object *obj)
     qdev_init_gpio_in(DEVICE(obj), stm32l4x5_syscfg_set_irq,
                       GPIO_NUM_PINS * NUM_GPIOS);
     qdev_init_gpio_out(DEVICE(obj), s->gpio_out, GPIO_NUM_PINS);
+    s->clk = qdev_init_clock_in(DEVICE(s), "clk", NULL, s, 0);
+    object_property_add(obj, "clock-freq-hz", "uint32", clock_freq_get, NULL,
+                        NULL, NULL);
+}
+
+static void stm32l4x5_syscfg_realize(DeviceState *dev, Error **errp)
+{
+    Stm32l4x5SyscfgState *s = STM32L4X5_SYSCFG(dev);
+    if (!clock_has_source(s->clk)) {
+        error_setg(errp, "SYSCFG: clk input must be connected");
+        return;
+    }
 }
 
 static const VMStateDescription vmstate_stm32l4x5_syscfg = {
@@ -241,6 +265,7 @@ static const VMStateDescription vmstate_stm32l4x5_syscfg = {
         VMSTATE_UINT32(swpr, Stm32l4x5SyscfgState),
         VMSTATE_UINT32(skr, Stm32l4x5SyscfgState),
         VMSTATE_UINT32(swpr2, Stm32l4x5SyscfgState),
+        VMSTATE_CLOCK(clk, Stm32l4x5SyscfgState),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -251,6 +276,7 @@ static void stm32l4x5_syscfg_class_init(ObjectClass *klass, void *data)
     ResettableClass *rc = RESETTABLE_CLASS(klass);
 
     dc->vmsd = &vmstate_stm32l4x5_syscfg;
+    dc->realize = stm32l4x5_syscfg_realize;
     rc->phases.hold = stm32l4x5_syscfg_hold_reset;
 }
 
-- 
2.43.2

[PATCH 2/4] hw/gpio: Handle clock migration in STM32L4x5 gpios

[Inès Varhol]

Signed-off-by: Inès Varhol <ines.varhol@telecom-paris.fr>
---
 hw/gpio/stm32l4x5_gpio.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/gpio/stm32l4x5_gpio.c b/hw/gpio/stm32l4x5_gpio.c
index 71bf5fddb2..14e6618d30 100644
--- a/hw/gpio/stm32l4x5_gpio.c
+++ b/hw/gpio/stm32l4x5_gpio.c
@@ -20,6 +20,7 @@
 #include "qemu/log.h"
 #include "hw/gpio/stm32l4x5_gpio.h"
 #include "hw/irq.h"
+#include "hw/clock.h"
 #include "hw/qdev-clock.h"
 #include "hw/qdev-properties.h"
 #include "qapi/visitor.h"
@@ -441,6 +442,7 @@ static const VMStateDescription vmstate_stm32l4x5_gpio = {
         VMSTATE_UINT32(ascr, Stm32l4x5GpioState),
         VMSTATE_UINT16(disconnected_pins, Stm32l4x5GpioState),
         VMSTATE_UINT16(pins_connected_high, Stm32l4x5GpioState),
+        VMSTATE_CLOCK(clk, Stm32l4x5GpioState),
         VMSTATE_END_OF_LIST()
     }
 };
-- 
2.43.2

[PATCH 3/4] hw/char: Add QOM property for STM32L4x5 USART clock frequency

[Inès Varhol]

Signed-off-by: Inès Varhol <ines.varhol@telecom-paris.fr>
---
 hw/char/stm32l4x5_usart.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/hw/char/stm32l4x5_usart.c b/hw/char/stm32l4x5_usart.c
index fc5dcac0c4..ee7727481c 100644
--- a/hw/char/stm32l4x5_usart.c
+++ b/hw/char/stm32l4x5_usart.c
@@ -26,6 +26,7 @@
 #include "hw/clock.h"
 #include "hw/irq.h"
 #include "hw/qdev-clock.h"
+#include "qapi/visitor.h"
 #include "hw/qdev-properties.h"
 #include "hw/qdev-properties-system.h"
 #include "hw/registerfields.h"
@@ -523,6 +524,14 @@ static Property stm32l4x5_usart_base_properties[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
+static void clock_freq_get(Object *obj, Visitor *v,
+    const char *name, void *opaque, Error **errp)
+{
+    Stm32l4x5UsartBaseState *s = STM32L4X5_USART_BASE(obj);
+    uint32_t clock_freq_hz = clock_get_hz(s->clk);
+    visit_type_uint32(v, name, &clock_freq_hz, errp);
+}
+
 static void stm32l4x5_usart_base_init(Object *obj)
 {
     Stm32l4x5UsartBaseState *s = STM32L4X5_USART_BASE(obj);
@@ -534,6 +543,9 @@ static void stm32l4x5_usart_base_init(Object *obj)
     sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mmio);
 
     s->clk = qdev_init_clock_in(DEVICE(s), "clk", NULL, s, 0);
+
+    object_property_add(obj, "clock-freq-hz", "uint32",
+                        clock_freq_get, NULL, NULL, NULL);
 }
 
 static int stm32l4x5_usart_base_post_load(void *opaque, int version_id)
-- 
2.43.2

[PATCH 4/4] tests/qtest: Check STM32L4x5 clock connections

[Inès Varhol]

For USART, GPIO and SYSCFG devices, check that clock frequency before
and after enabling the peripheral clock in RCC is correct.

Signed-off-by: Inès Varhol <ines.varhol@telecom-paris.fr>
---
Hello,

Should these tests be regrouped in stm32l4x5_rcc-test.c ?

Best regards,

Inès Varhol

 tests/qtest/stm32l4x5_gpio-test.c   | 39 +++++++++++++++++++++++
 tests/qtest/stm32l4x5_syscfg-test.c | 38 +++++++++++++++++++++--
 tests/qtest/stm32l4x5_usart-test.c  | 48 +++++++++++++++++++++++++++++
 3 files changed, 123 insertions(+), 2 deletions(-)

diff --git a/tests/qtest/stm32l4x5_gpio-test.c b/tests/qtest/stm32l4x5_gpio-test.c
index 72a7823406..896c16ad59 100644
--- a/tests/qtest/stm32l4x5_gpio-test.c
+++ b/tests/qtest/stm32l4x5_gpio-test.c
@@ -25,6 +25,14 @@
 #define GPIO_G 0x48001800
 #define GPIO_H 0x48001C00
 
+/*
+ * MSI (4 MHz) is used as system clock source after startup
+ * from Reset.
+ * AHB prescaler is set to 1 at reset.
+ */
+#define SYSCLK_FREQ_HZ 4000000
+#define RCC_AHB2ENR 0x4002104C
+
 #define MODER 0x00
 #define OTYPER 0x04
 #define PUPDR 0x0C
@@ -168,6 +176,21 @@ static uint32_t reset(uint32_t gpio, unsigned int offset)
     return 0x0;
 }
 
+static uint32_t get_clock_freq_hz(unsigned int gpio)
+{
+    g_autofree char *path = g_strdup_printf("/machine/soc/gpio%c",
+                                            get_gpio_id(gpio) + 'a');
+    uint32_t clock_freq_hz = 0;
+    QDict *r;
+
+    r = qtest_qmp(global_qtest, "{ 'execute': 'qom-get', 'arguments':"
+        " { 'path': %s, 'property': 'clock-freq-hz'} }", path);
+    g_assert_false(qdict_haskey(r, "error"));
+    clock_freq_hz = qdict_get_int(r, "return");
+    qobject_unref(r);
+    return clock_freq_hz;
+}
+
 static void system_reset(void)
 {
     QDict *r;
@@ -505,6 +528,20 @@ static void test_bsrr_brr(const void *data)
     gpio_writel(gpio, ODR, reset(gpio, ODR));
 }
 
+static void test_clock_enable(void)
+{
+    /*
+     * For each GPIO, enable its clock in RCC
+     * and check that its clock frequency changes to SYSCLK_FREQ_HZ
+     */
+    for (uint32_t gpio = GPIO_A; gpio <= GPIO_H; gpio += GPIO_B - GPIO_A) {
+        g_assert_cmpuint(get_clock_freq_hz(gpio), ==, 0);
+        /* Enable the gpio clock */
+        writel(RCC_AHB2ENR, readl(RCC_AHB2ENR) | (0x1 << get_gpio_id(gpio)));
+        g_assert_cmpuint(get_clock_freq_hz(gpio), ==, SYSCLK_FREQ_HZ);
+    }
+}
+
 int main(int argc, char **argv)
 {
     int ret;
@@ -556,6 +593,8 @@ int main(int argc, char **argv)
     qtest_add_data_func("stm32l4x5/gpio/test_bsrr_brr2",
                         test_data(GPIO_D, 0),
                         test_bsrr_brr);
+    qtest_add_func("stm32l4x5/gpio/test_clock_enable",
+                   test_clock_enable);
 
     qtest_start("-machine b-l475e-iot01a");
     ret = g_test_run();
diff --git a/tests/qtest/stm32l4x5_syscfg-test.c b/tests/qtest/stm32l4x5_syscfg-test.c
index 506ca08bc2..616106460d 100644
--- a/tests/qtest/stm32l4x5_syscfg-test.c
+++ b/tests/qtest/stm32l4x5_syscfg-test.c
@@ -26,9 +26,18 @@
 #define INVALID_ADDR 0x2C
 
 /* SoC forwards GPIOs to SysCfg */
-#define SYSCFG "/machine/soc"
+#define SOC "/machine/soc"
+#define SYSCFG "/machine/soc/syscfg"
 #define EXTI "/machine/soc/exti"
 
+/*
+ * MSI (4 MHz) is used as system clock source after startup
+ * from Reset.
+ * AHB and APB2 prescalers are set to 1 at reset.
+ */
+#define SYSCLK_FREQ_HZ 4000000
+#define RCC_APB2ENR 0x40021060
+
 static void syscfg_writel(unsigned int offset, uint32_t value)
 {
     writel(SYSCFG_BASE_ADDR + offset, value);
@@ -41,7 +50,7 @@ static uint32_t syscfg_readl(unsigned int offset)
 
 static void syscfg_set_irq(int num, int level)
 {
-   qtest_set_irq_in(global_qtest, SYSCFG, NULL, num, level);
+   qtest_set_irq_in(global_qtest, SOC, NULL, num, level);
 }
 
 static void system_reset(void)
@@ -52,6 +61,19 @@ static void system_reset(void)
     qobject_unref(response);
 }
 
+static uint32_t get_clock_freq_hz()
+{
+    uint32_t clock_freq_hz = 0;
+    QDict *r;
+
+    r = qtest_qmp(global_qtest, "{ 'execute': 'qom-get', 'arguments':"
+        " { 'path': %s, 'property': 'clock-freq-hz'} }", SYSCFG);
+    g_assert_false(qdict_haskey(r, "error"));
+    clock_freq_hz = qdict_get_int(r, "return");
+    qobject_unref(r);
+    return clock_freq_hz;
+}
+
 static void test_reset(void)
 {
     /*
@@ -301,6 +323,16 @@ static void test_irq_gpio_multiplexer(void)
     syscfg_writel(SYSCFG_EXTICR1, 0x00000000);
 }
 
+static void test_clock_enable(void)
+{
+    g_assert_cmpuint(get_clock_freq_hz(), ==, 0);
+
+    /* Enable SYSCFG clock */
+    writel(RCC_APB2ENR, readl(RCC_APB2ENR) | (0x1 << 0));
+
+    g_assert_cmpuint(get_clock_freq_hz(), ==, SYSCLK_FREQ_HZ);
+}
+
 int main(int argc, char **argv)
 {
     int ret;
@@ -325,6 +357,8 @@ int main(int argc, char **argv)
                    test_irq_pin_multiplexer);
     qtest_add_func("stm32l4x5/syscfg/test_irq_gpio_multiplexer",
                    test_irq_gpio_multiplexer);
+    qtest_add_func("stm32l4x5/syscfg/test_clock_enable",
+                   test_clock_enable);
 
     qtest_start("-machine b-l475e-iot01a");
     ret = g_test_run();
diff --git a/tests/qtest/stm32l4x5_usart-test.c b/tests/qtest/stm32l4x5_usart-test.c
index 8902518233..6e6e66b6ab 100644
--- a/tests/qtest/stm32l4x5_usart-test.c
+++ b/tests/qtest/stm32l4x5_usart-test.c
@@ -17,6 +17,16 @@
 /* Use USART 1 ADDR, assume the others work the same */
 #define USART1_BASE_ADDR 0x40013800
 
+/*
+ * MSI (4 MHz) is used as system clock source after startup
+ * from Reset.
+ * AHB, APB1 and APB2 prescaler are set to 1 at reset.
+ */
+#define SYSCLK_FREQ_HZ 4000000
+#define RCC_APB1ENR1 0x40021058
+#define RCC_APB1ENR2 0x4002105C
+#define RCC_APB2ENR  0x40021060
+
 /* See stm32l4x5_usart for definitions */
 REG32(CR1, 0x00)
     FIELD(CR1, M1, 28, 1)
@@ -64,6 +74,19 @@ static bool clear_nvic_pending(QTestState *qts, unsigned int n)
     return true;
 }
 
+static uint32_t get_clock_freq_hz(QTestState *qts, const char *path)
+{
+    uint32_t clock_freq_hz = 0;
+    QDict *r;
+
+    r = qtest_qmp(qts, "{ 'execute': 'qom-get', 'arguments':"
+        " { 'path': %s, 'property': 'clock-freq-hz'} }", path);
+    g_assert_false(qdict_haskey(r, "error"));
+    clock_freq_hz = qdict_get_int(r, "return");
+    qobject_unref(r);
+    return clock_freq_hz;
+}
+
 /*
  * Wait indefinitely for the flag to be updated.
  * If this is run on a slow CI runner,
@@ -296,6 +319,30 @@ static void test_send_str(void)
     qtest_quit(qts);
 }
 
+static void check_clock(QTestState *qts, const char *path, uint32_t rcc_reg,
+                        uint32_t reg_offset)
+{
+    g_assert_cmpuint(get_clock_freq_hz(qts, path), ==, 0);
+    qtest_writel(qts, rcc_reg, qtest_readl(qts, rcc_reg) | (0x1 << reg_offset));
+    g_assert_cmpuint(get_clock_freq_hz(qts, path), ==, SYSCLK_FREQ_HZ);
+}
+
+static void test_clock_enable(void)
+{
+    /*
+     * For each USART device, enable its clock in RCC
+     * and check that its clock frequency is SYSCLK_FREQ_HZ
+     */
+    QTestState *qts = qtest_init("-M b-l475e-iot01a");
+
+    check_clock(qts, "machine/soc/usart[0]", RCC_APB2ENR, 14);
+    check_clock(qts, "machine/soc/usart[1]", RCC_APB1ENR1, 17);
+    check_clock(qts, "machine/soc/usart[2]", RCC_APB1ENR1, 18);
+    check_clock(qts, "machine/soc/uart[0]", RCC_APB1ENR1, 19);
+    check_clock(qts, "machine/soc/uart[1]", RCC_APB1ENR1, 20);
+    check_clock(qts, "machine/soc/lpuart1", RCC_APB1ENR2, 0);
+}
+
 int main(int argc, char **argv)
 {
     int ret;
@@ -308,6 +355,7 @@ int main(int argc, char **argv)
     qtest_add_func("stm32l4x5/usart/send_char", test_send_char);
     qtest_add_func("stm32l4x5/usart/receive_str", test_receive_str);
     qtest_add_func("stm32l4x5/usart/send_str", test_send_str);
+    qtest_add_func("stm32l4x5/usart/clock_enable", test_clock_enable);
     ret = g_test_run();
 
     return ret;
-- 
2.43.2

Re: [PATCH 4/4] tests/qtest: Check STM32L4x5 clock connections

[Thomas Huth]

On 05/05/2024 16.05, Inès Varhol wrote:
> For USART, GPIO and SYSCFG devices, check that clock frequency before
> and after enabling the peripheral clock in RCC is correct.
> 
> Signed-off-by: Inès Varhol <ines.varhol@telecom-paris.fr>
> ---
> Hello,
> 
> Should these tests be regrouped in stm32l4x5_rcc-test.c ?

  Hi,

sounds mostly like a matter of taste at a first glance. Or what would be the 
benefit of putting everything into the *rcc-test.c file? Could you maybe 
consolidate the get_clock_freq_hz() function that way? (maybe that 
get_clock_freq_hz() function could also be consolidated as a inline function 
in a shared header instead?)

  Thomas

Re: [PATCH 3/4] hw/char: Add QOM property for STM32L4x5 USART clock frequency

[Philippe Mathieu-Daudé]

Hi,

On 5/5/24 16:05, Inès Varhol wrote:
> Signed-off-by: Inès Varhol <ines.varhol@telecom-paris.fr>
> ---
>   hw/char/stm32l4x5_usart.c | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
> diff --git a/hw/char/stm32l4x5_usart.c b/hw/char/stm32l4x5_usart.c
> index fc5dcac0c4..ee7727481c 100644
> --- a/hw/char/stm32l4x5_usart.c
> +++ b/hw/char/stm32l4x5_usart.c
> @@ -26,6 +26,7 @@
>   #include "hw/clock.h"
>   #include "hw/irq.h"
>   #include "hw/qdev-clock.h"
> +#include "qapi/visitor.h"
>   #include "hw/qdev-properties.h"
>   #include "hw/qdev-properties-system.h"
>   #include "hw/registerfields.h"
> @@ -523,6 +524,14 @@ static Property stm32l4x5_usart_base_properties[] = {
>       DEFINE_PROP_END_OF_LIST(),
>   };
>   
> +static void clock_freq_get(Object *obj, Visitor *v,
> +    const char *name, void *opaque, Error **errp)
> +{
> +    Stm32l4x5UsartBaseState *s = STM32L4X5_USART_BASE(obj);
> +    uint32_t clock_freq_hz = clock_get_hz(s->clk);
> +    visit_type_uint32(v, name, &clock_freq_hz, errp);
> +}
> +
>   static void stm32l4x5_usart_base_init(Object *obj)
>   {
>       Stm32l4x5UsartBaseState *s = STM32L4X5_USART_BASE(obj);
> @@ -534,6 +543,9 @@ static void stm32l4x5_usart_base_init(Object *obj)
>       sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mmio);
>   
>       s->clk = qdev_init_clock_in(DEVICE(s), "clk", NULL, s, 0);
> +
> +    object_property_add(obj, "clock-freq-hz", "uint32",
> +                        clock_freq_get, NULL, NULL, NULL);

Patch LGTM, but I wonder if registering QOM getter without setter
is recommended. Perhaps we should encourage parity? In normal HW
emulation we shouldn't update this clock externally, but thinking
about testing, this could be interesting to introduce jitter.

Any opinion on this?

>   }
>   
>   static int stm32l4x5_usart_base_post_load(void *opaque, int version_id)

Re: [PATCH 2/4] hw/gpio: Handle clock migration in STM32L4x5 gpios

[Philippe Mathieu-Daudé]

Hi Inès,

On 5/5/24 16:05, Inès Varhol wrote:

Fixes: 1cdcfb6e93 ("hw/gpio: Implement STM32L4x5 GPIO")

> Signed-off-by: Inès Varhol <ines.varhol@telecom-paris.fr>
> ---
>   hw/gpio/stm32l4x5_gpio.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/hw/gpio/stm32l4x5_gpio.c b/hw/gpio/stm32l4x5_gpio.c
> index 71bf5fddb2..14e6618d30 100644
> --- a/hw/gpio/stm32l4x5_gpio.c
> +++ b/hw/gpio/stm32l4x5_gpio.c
> @@ -20,6 +20,7 @@
>   #include "qemu/log.h"
>   #include "hw/gpio/stm32l4x5_gpio.h"
>   #include "hw/irq.h"
> +#include "hw/clock.h"
>   #include "hw/qdev-clock.h"
>   #include "hw/qdev-properties.h"
>   #include "qapi/visitor.h"
> @@ -441,6 +442,7 @@ static const VMStateDescription vmstate_stm32l4x5_gpio = {
>           VMSTATE_UINT32(ascr, Stm32l4x5GpioState),
>           VMSTATE_UINT16(disconnected_pins, Stm32l4x5GpioState),
>           VMSTATE_UINT16(pins_connected_high, Stm32l4x5GpioState),
> +        VMSTATE_CLOCK(clk, Stm32l4x5GpioState),

IIUC we need to increase vmstate_stm32l4x5_gpio version_id (see
commit 8fd34dc0c4 "hw/arm/armsse: Wire up clocks" for example).

>           VMSTATE_END_OF_LIST()
>       }
>   };

Re: [PATCH 3/4] hw/char: Add QOM property for STM32L4x5 USART clock frequency

[Philippe Mathieu-Daudé]

(+Luc & Damien for Clock API)

On 6/5/24 11:34, Philippe Mathieu-Daudé wrote:
> Hi,
> 
> On 5/5/24 16:05, Inès Varhol wrote:
>> Signed-off-by: Inès Varhol <ines.varhol@telecom-paris.fr>
>> ---
>>   hw/char/stm32l4x5_usart.c | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/hw/char/stm32l4x5_usart.c b/hw/char/stm32l4x5_usart.c
>> index fc5dcac0c4..ee7727481c 100644
>> --- a/hw/char/stm32l4x5_usart.c
>> +++ b/hw/char/stm32l4x5_usart.c
>> @@ -26,6 +26,7 @@
>>   #include "hw/clock.h"
>>   #include "hw/irq.h"
>>   #include "hw/qdev-clock.h"
>> +#include "qapi/visitor.h"
>>   #include "hw/qdev-properties.h"
>>   #include "hw/qdev-properties-system.h"
>>   #include "hw/registerfields.h"
>> @@ -523,6 +524,14 @@ static Property stm32l4x5_usart_base_properties[] 
>> = {
>>       DEFINE_PROP_END_OF_LIST(),
>>   };
>> +static void clock_freq_get(Object *obj, Visitor *v,
>> +    const char *name, void *opaque, Error **errp)
>> +{
>> +    Stm32l4x5UsartBaseState *s = STM32L4X5_USART_BASE(obj);
>> +    uint32_t clock_freq_hz = clock_get_hz(s->clk);
>> +    visit_type_uint32(v, name, &clock_freq_hz, errp);
>> +}
>> +
>>   static void stm32l4x5_usart_base_init(Object *obj)
>>   {
>>       Stm32l4x5UsartBaseState *s = STM32L4X5_USART_BASE(obj);
>> @@ -534,6 +543,9 @@ static void stm32l4x5_usart_base_init(Object *obj)
>>       sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mmio);
>>       s->clk = qdev_init_clock_in(DEVICE(s), "clk", NULL, s, 0);
>> +
>> +    object_property_add(obj, "clock-freq-hz", "uint32",
>> +                        clock_freq_get, NULL, NULL, NULL);
> 
> Patch LGTM, but I wonder if registering QOM getter without setter
> is recommended. Perhaps we should encourage parity? In normal HW
> emulation we shouldn't update this clock externally, but thinking
> about testing, this could be interesting to introduce jitter.

Orthogonal to this doubt, we could add the clock properties
directly in qdev_init_clock_in(). Seems useful for the QTest
framework.

> Any opinion on this?
> 
>>   }
>>   static int stm32l4x5_usart_base_post_load(void *opaque, int version_id)
> 

Re: [PATCH 4/4] tests/qtest: Check STM32L4x5 clock connections

[Inès Varhol]

----- Le 6 Mai 24, à 6:16, Thomas Huth thuth@redhat.com a écrit :

> On 05/05/2024 16.05, Inès Varhol wrote:
>> For USART, GPIO and SYSCFG devices, check that clock frequency before
>> and after enabling the peripheral clock in RCC is correct.
>> 
>> Signed-off-by: Inès Varhol <ines.varhol@telecom-paris.fr>
>> ---
>> Hello,
>> 
>> Should these tests be regrouped in stm32l4x5_rcc-test.c ?
> 
>  Hi,
> 
> sounds mostly like a matter of taste at a first glance. Or what would be the
> benefit of putting everything into the *rcc-test.c file? Could you maybe
> consolidate the get_clock_freq_hz() function that way? (maybe that
> get_clock_freq_hz() function could also be consolidated as a inline function
> in a shared header instead?)
> 
>  Thomas

Hello,

I was indeed looking to consolidate the functions get_clock_freq_hz() and
check_clock() from *usart-test.c (along with the definitions for RCC
registers).
Thank you for your suggestion, I'll use a header file.

Inès Varhol

Re: [PATCH 1/4] hw/misc: Create STM32L4x5 SYSCFG clock

[Peter Maydell]

On Sun, 5 May 2024 at 15:06, Inès Varhol <ines.varhol@telecom-paris.fr> wrote:
>
> Signed-off-by: Inès Varhol <ines.varhol@telecom-paris.fr>

In general you should try to avoid commits with no commit message.
Sometimes there really isn't anything to say beyond what the
subject line is, but that should be the exception rather than
the usual thing.

> ---
>  include/hw/misc/stm32l4x5_syscfg.h |  1 +
>  hw/arm/stm32l4x5_soc.c             |  2 ++
>  hw/misc/stm32l4x5_syscfg.c         | 26 ++++++++++++++++++++++++++
>  3 files changed, 29 insertions(+)
>
> diff --git a/include/hw/misc/stm32l4x5_syscfg.h b/include/hw/misc/stm32l4x5_syscfg.h
> index 23bb564150..c450df2b9e 100644
> --- a/include/hw/misc/stm32l4x5_syscfg.h
> +++ b/include/hw/misc/stm32l4x5_syscfg.h
> @@ -48,6 +48,7 @@ struct Stm32l4x5SyscfgState {
>      uint32_t swpr2;
>
>      qemu_irq gpio_out[GPIO_NUM_PINS];
> +    Clock *clk;
>  };
>
>  #endif
> diff --git a/hw/arm/stm32l4x5_soc.c b/hw/arm/stm32l4x5_soc.c
> index 38f7a2d5d9..fb2afa6cfe 100644
> --- a/hw/arm/stm32l4x5_soc.c
> +++ b/hw/arm/stm32l4x5_soc.c
> @@ -236,6 +236,8 @@ static void stm32l4x5_soc_realize(DeviceState *dev_soc, Error **errp)
>
>      /* System configuration controller */
>      busdev = SYS_BUS_DEVICE(&s->syscfg);
> +    qdev_connect_clock_in(DEVICE(&s->syscfg), "clk",
> +        qdev_get_clock_out(DEVICE(&(s->rcc)), "syscfg-out"));
>      if (!sysbus_realize(busdev, errp)) {
>          return;
>      }
> diff --git a/hw/misc/stm32l4x5_syscfg.c b/hw/misc/stm32l4x5_syscfg.c
> index a5a1ce2680..a82864c33d 100644
> --- a/hw/misc/stm32l4x5_syscfg.c
> +++ b/hw/misc/stm32l4x5_syscfg.c
> @@ -26,6 +26,10 @@
>  #include "trace.h"
>  #include "hw/irq.h"
>  #include "migration/vmstate.h"
> +#include "hw/clock.h"
> +#include "hw/qdev-clock.h"
> +#include "qapi/visitor.h"
> +#include "qapi/error.h"
>  #include "hw/misc/stm32l4x5_syscfg.h"
>  #include "hw/gpio/stm32l4x5_gpio.h"
>
> @@ -202,6 +206,14 @@ static void stm32l4x5_syscfg_write(void *opaque, hwaddr addr,
>      }
>  }
>
> +static void clock_freq_get(Object *obj, Visitor *v,
> +    const char *name, void *opaque, Error **errp)
> +{
> +    Stm32l4x5SyscfgState *s = STM32L4X5_SYSCFG(obj);
> +    uint32_t clock_freq_hz = clock_get_hz(s->clk);
> +    visit_type_uint32(v, name, &clock_freq_hz, errp);
> +}
> +
>  static const MemoryRegionOps stm32l4x5_syscfg_ops = {
>      .read = stm32l4x5_syscfg_read,
>      .write = stm32l4x5_syscfg_write,
> @@ -225,6 +237,18 @@ static void stm32l4x5_syscfg_init(Object *obj)
>      qdev_init_gpio_in(DEVICE(obj), stm32l4x5_syscfg_set_irq,
>                        GPIO_NUM_PINS * NUM_GPIOS);
>      qdev_init_gpio_out(DEVICE(obj), s->gpio_out, GPIO_NUM_PINS);
> +    s->clk = qdev_init_clock_in(DEVICE(s), "clk", NULL, s, 0);
> +    object_property_add(obj, "clock-freq-hz", "uint32", clock_freq_get, NULL,
> +                        NULL, NULL);

Why do we need this property? The clock on this device is an input,
so the device doesn't control its frequency.

> +}
> +
> +static void stm32l4x5_syscfg_realize(DeviceState *dev, Error **errp)
> +{
> +    Stm32l4x5SyscfgState *s = STM32L4X5_SYSCFG(dev);
> +    if (!clock_has_source(s->clk)) {
> +        error_setg(errp, "SYSCFG: clk input must be connected");
> +        return;
> +    }
>  }
>
>  static const VMStateDescription vmstate_stm32l4x5_syscfg = {
> @@ -241,6 +265,7 @@ static const VMStateDescription vmstate_stm32l4x5_syscfg = {
>          VMSTATE_UINT32(swpr, Stm32l4x5SyscfgState),
>          VMSTATE_UINT32(skr, Stm32l4x5SyscfgState),
>          VMSTATE_UINT32(swpr2, Stm32l4x5SyscfgState),
> +        VMSTATE_CLOCK(clk, Stm32l4x5SyscfgState),

Adding a field to the vmstate means we must bump the version number,
since it's a migration compatibility break.

>          VMSTATE_END_OF_LIST()
>      }
>  };

thanks
-- PMM

Re: [PATCH 3/4] hw/char: Add QOM property for STM32L4x5 USART clock frequency

[Peter Maydell]

On Mon, 6 May 2024 at 10:34, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Hi,
>
> On 5/5/24 16:05, Inès Varhol wrote:
> > Signed-off-by: Inès Varhol <ines.varhol@telecom-paris.fr>
> > ---
> >   hw/char/stm32l4x5_usart.c | 12 ++++++++++++
> >   1 file changed, 12 insertions(+)
> >
> > diff --git a/hw/char/stm32l4x5_usart.c b/hw/char/stm32l4x5_usart.c
> > index fc5dcac0c4..ee7727481c 100644
> > --- a/hw/char/stm32l4x5_usart.c
> > +++ b/hw/char/stm32l4x5_usart.c
> > @@ -26,6 +26,7 @@
> >   #include "hw/clock.h"
> >   #include "hw/irq.h"
> >   #include "hw/qdev-clock.h"
> > +#include "qapi/visitor.h"
> >   #include "hw/qdev-properties.h"
> >   #include "hw/qdev-properties-system.h"
> >   #include "hw/registerfields.h"
> > @@ -523,6 +524,14 @@ static Property stm32l4x5_usart_base_properties[] = {
> >       DEFINE_PROP_END_OF_LIST(),
> >   };
> >
> > +static void clock_freq_get(Object *obj, Visitor *v,
> > +    const char *name, void *opaque, Error **errp)
> > +{
> > +    Stm32l4x5UsartBaseState *s = STM32L4X5_USART_BASE(obj);
> > +    uint32_t clock_freq_hz = clock_get_hz(s->clk);
> > +    visit_type_uint32(v, name, &clock_freq_hz, errp);
> > +}
> > +
> >   static void stm32l4x5_usart_base_init(Object *obj)
> >   {
> >       Stm32l4x5UsartBaseState *s = STM32L4X5_USART_BASE(obj);
> > @@ -534,6 +543,9 @@ static void stm32l4x5_usart_base_init(Object *obj)
> >       sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mmio);
> >
> >       s->clk = qdev_init_clock_in(DEVICE(s), "clk", NULL, s, 0);
> > +
> > +    object_property_add(obj, "clock-freq-hz", "uint32",
> > +                        clock_freq_get, NULL, NULL, NULL);
>
> Patch LGTM, but I wonder if registering QOM getter without setter
> is recommended. Perhaps we should encourage parity? In normal HW
> emulation we shouldn't update this clock externally, but thinking
> about testing, this could be interesting to introduce jitter.

object_property_add() with the set function NULL is fine,
and is documented to mean "property cannot be set". Attempts
to set it will be failed (in object_property_set()) with a
reasonable error.

But it's not clear to me why we want the property in the first
place -- we don't generally have devices which take a Clock
input have properties exposing its frequency. If we did want
that it would probably be better if we could do it generically
rather than by adding more boilerplate code to each device.
Mostly "frequency" properties on devices are for the case
where  they *don't* have a Clock input and instead have
ad-hoc legacy handling where the board/SoC that creates the
device sets an integer property to define the input frequency
because it doesn't model the clock tree with Clock objects.

thanks
-- PMM

Re: [PATCH 1/4] hw/misc: Create STM32L4x5 SYSCFG clock

[Inès Varhol]

----- Le 7 Mai 24, à 11:50, peter maydell peter.maydell@linaro.org a écrit :

> On Sun, 5 May 2024 at 15:06, Inès Varhol <ines.varhol@telecom-paris.fr> wrote:
>>
>> Signed-off-by: Inès Varhol <ines.varhol@telecom-paris.fr>
> 
> In general you should try to avoid commits with no commit message.
> Sometimes there really isn't anything to say beyond what the
> subject line is, but that should be the exception rather than
> the usual thing.

Hello,

Understood, I'll add messages.

> 
>> ---
>>  include/hw/misc/stm32l4x5_syscfg.h |  1 +
>>  hw/arm/stm32l4x5_soc.c             |  2 ++
>>  hw/misc/stm32l4x5_syscfg.c         | 26 ++++++++++++++++++++++++++
>>  3 files changed, 29 insertions(+)
>>
>> diff --git a/include/hw/misc/stm32l4x5_syscfg.h
>> b/include/hw/misc/stm32l4x5_syscfg.h
>> index 23bb564150..c450df2b9e 100644
>> --- a/include/hw/misc/stm32l4x5_syscfg.h
>> +++ b/include/hw/misc/stm32l4x5_syscfg.h
>> @@ -48,6 +48,7 @@ struct Stm32l4x5SyscfgState {
>>      uint32_t swpr2;
>>
>>      qemu_irq gpio_out[GPIO_NUM_PINS];
>> +    Clock *clk;
>>  };
>>
>>  #endif
>> diff --git a/hw/arm/stm32l4x5_soc.c b/hw/arm/stm32l4x5_soc.c
>> index 38f7a2d5d9..fb2afa6cfe 100644
>> --- a/hw/arm/stm32l4x5_soc.c
>> +++ b/hw/arm/stm32l4x5_soc.c
>> @@ -236,6 +236,8 @@ static void stm32l4x5_soc_realize(DeviceState *dev_soc,
>> Error **errp)
>>
>>      /* System configuration controller */
>>      busdev = SYS_BUS_DEVICE(&s->syscfg);
>> +    qdev_connect_clock_in(DEVICE(&s->syscfg), "clk",
>> +        qdev_get_clock_out(DEVICE(&(s->rcc)), "syscfg-out"));
>>      if (!sysbus_realize(busdev, errp)) {
>>          return;
>>      }
>> diff --git a/hw/misc/stm32l4x5_syscfg.c b/hw/misc/stm32l4x5_syscfg.c
>> index a5a1ce2680..a82864c33d 100644
>> --- a/hw/misc/stm32l4x5_syscfg.c
>> +++ b/hw/misc/stm32l4x5_syscfg.c
>> @@ -26,6 +26,10 @@
>>  #include "trace.h"
>>  #include "hw/irq.h"
>>  #include "migration/vmstate.h"
>> +#include "hw/clock.h"
>> +#include "hw/qdev-clock.h"
>> +#include "qapi/visitor.h"
>> +#include "qapi/error.h"
>>  #include "hw/misc/stm32l4x5_syscfg.h"
>>  #include "hw/gpio/stm32l4x5_gpio.h"
>>
>> @@ -202,6 +206,14 @@ static void stm32l4x5_syscfg_write(void *opaque, hwaddr
>> addr,
>>      }
>>  }
>>
>> +static void clock_freq_get(Object *obj, Visitor *v,
>> +    const char *name, void *opaque, Error **errp)
>> +{
>> +    Stm32l4x5SyscfgState *s = STM32L4X5_SYSCFG(obj);
>> +    uint32_t clock_freq_hz = clock_get_hz(s->clk);
>> +    visit_type_uint32(v, name, &clock_freq_hz, errp);
>> +}
>> +
>>  static const MemoryRegionOps stm32l4x5_syscfg_ops = {
>>      .read = stm32l4x5_syscfg_read,
>>      .write = stm32l4x5_syscfg_write,
>> @@ -225,6 +237,18 @@ static void stm32l4x5_syscfg_init(Object *obj)
>>      qdev_init_gpio_in(DEVICE(obj), stm32l4x5_syscfg_set_irq,
>>                        GPIO_NUM_PINS * NUM_GPIOS);
>>      qdev_init_gpio_out(DEVICE(obj), s->gpio_out, GPIO_NUM_PINS);
>> +    s->clk = qdev_init_clock_in(DEVICE(s), "clk", NULL, s, 0);
>> +    object_property_add(obj, "clock-freq-hz", "uint32", clock_freq_get, NULL,
>> +                        NULL, NULL);
> 
> Why do we need this property? The clock on this device is an input,
> so the device doesn't control its frequency.

Using a QOM property allows to read the clock frequency from a QTest.
(npcm7xx_pwm-test.c does this, I didn't find other examples of reading a
frequency)

Best regards,

Inès Varhol

[PATCH 2/4] hw/gpio: Handle clock migration in STM32L4x5 gpios

[Inès Varhol]

STM32L4x5 GPIO wasn't migrating its clock.

Signed-off-by: Inès Varhol <ines.varhol@telecom-paris.fr>
---
 hw/gpio/stm32l4x5_gpio.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/gpio/stm32l4x5_gpio.c b/hw/gpio/stm32l4x5_gpio.c
index 71bf5fddb2..30d8d6cba4 100644
--- a/hw/gpio/stm32l4x5_gpio.c
+++ b/hw/gpio/stm32l4x5_gpio.c
@@ -20,6 +20,7 @@
 #include "qemu/log.h"
 #include "hw/gpio/stm32l4x5_gpio.h"
 #include "hw/irq.h"
+#include "hw/clock.h"
 #include "hw/qdev-clock.h"
 #include "hw/qdev-properties.h"
 #include "qapi/visitor.h"
@@ -426,8 +427,8 @@ static void stm32l4x5_gpio_realize(DeviceState *dev, Error **errp)
 
 static const VMStateDescription vmstate_stm32l4x5_gpio = {
     .name = TYPE_STM32L4X5_GPIO,
-    .version_id = 1,
-    .minimum_version_id = 1,
+    .version_id = 2,
+    .minimum_version_id = 2,
     .fields = (VMStateField[]){
         VMSTATE_UINT32(moder, Stm32l4x5GpioState),
         VMSTATE_UINT32(otyper, Stm32l4x5GpioState),
@@ -441,6 +442,7 @@ static const VMStateDescription vmstate_stm32l4x5_gpio = {
         VMSTATE_UINT32(ascr, Stm32l4x5GpioState),
         VMSTATE_UINT16(disconnected_pins, Stm32l4x5GpioState),
         VMSTATE_UINT16(pins_connected_high, Stm32l4x5GpioState),
+        VMSTATE_CLOCK(clk, Stm32l4x5GpioState),
         VMSTATE_END_OF_LIST()
     }
 };
-- 
2.43.2

Re: [PATCH 2/4] hw/gpio: Handle clock migration in STM32L4x5 gpios

[Philippe Mathieu-Daudé]

On 7/5/24 20:55, Inès Varhol wrote:
> STM32L4x5 GPIO wasn't migrating its clock.
> 
> Signed-off-by: Inès Varhol <ines.varhol@telecom-paris.fr>
> ---
>   hw/gpio/stm32l4x5_gpio.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 3/4] hw/char: Add QOM property for STM32L4x5 USART clock frequency

[Philippe Mathieu-Daudé]

Hi,

On 7/5/24 11:54, Peter Maydell wrote:
> On Mon, 6 May 2024 at 10:34, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Hi,
>>
>> On 5/5/24 16:05, Inès Varhol wrote:
>>> Signed-off-by: Inès Varhol <ines.varhol@telecom-paris.fr>
>>> ---
>>>    hw/char/stm32l4x5_usart.c | 12 ++++++++++++
>>>    1 file changed, 12 insertions(+)
>>>
>>> diff --git a/hw/char/stm32l4x5_usart.c b/hw/char/stm32l4x5_usart.c
>>> index fc5dcac0c4..ee7727481c 100644
>>> --- a/hw/char/stm32l4x5_usart.c
>>> +++ b/hw/char/stm32l4x5_usart.c
>>> @@ -26,6 +26,7 @@
>>>    #include "hw/clock.h"
>>>    #include "hw/irq.h"
>>>    #include "hw/qdev-clock.h"
>>> +#include "qapi/visitor.h"
>>>    #include "hw/qdev-properties.h"
>>>    #include "hw/qdev-properties-system.h"
>>>    #include "hw/registerfields.h"
>>> @@ -523,6 +524,14 @@ static Property stm32l4x5_usart_base_properties[] = {
>>>        DEFINE_PROP_END_OF_LIST(),
>>>    };
>>>
>>> +static void clock_freq_get(Object *obj, Visitor *v,
>>> +    const char *name, void *opaque, Error **errp)
>>> +{
>>> +    Stm32l4x5UsartBaseState *s = STM32L4X5_USART_BASE(obj);
>>> +    uint32_t clock_freq_hz = clock_get_hz(s->clk);
>>> +    visit_type_uint32(v, name, &clock_freq_hz, errp);
>>> +}
>>> +
>>>    static void stm32l4x5_usart_base_init(Object *obj)
>>>    {
>>>        Stm32l4x5UsartBaseState *s = STM32L4X5_USART_BASE(obj);
>>> @@ -534,6 +543,9 @@ static void stm32l4x5_usart_base_init(Object *obj)
>>>        sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mmio);
>>>
>>>        s->clk = qdev_init_clock_in(DEVICE(s), "clk", NULL, s, 0);
>>> +
>>> +    object_property_add(obj, "clock-freq-hz", "uint32",
>>> +                        clock_freq_get, NULL, NULL, NULL);
>>
>> Patch LGTM, but I wonder if registering QOM getter without setter
>> is recommended. Perhaps we should encourage parity? In normal HW
>> emulation we shouldn't update this clock externally, but thinking
>> about testing, this could be interesting to introduce jitter.
> 
> object_property_add() with the set function NULL is fine,
> and is documented to mean "property cannot be set". Attempts
> to set it will be failed (in object_property_set()) with a
> reasonable error.
> 
> But it's not clear to me why we want the property in the first
> place -- we don't generally have devices which take a Clock
> input have properties exposing its frequency. If we did want
> that it would probably be better if we could do it generically
> rather than by adding more boilerplate code to each device.

Inès qtest checking (via HMP) the configured clock has a
correct scaled frequency seems a good use case.

> Mostly "frequency" properties on devices are for the case
> where  they *don't* have a Clock input and instead have
> ad-hoc legacy handling where the board/SoC that creates the
> device sets an integer property to define the input frequency
> because it doesn't model the clock tree with Clock objects.
> 
> thanks
> -- PMM