diff mbox series

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

Message ID 20240505140556.373711-2-ines.varhol@telecom-paris.fr (mailing list archive)
State New
Headers show
Series Check clock connection between STM32L4x5 RCC and peripherals | expand

Commit Message

Inès Varhol May 5, 2024, 2:05 p.m. UTC 
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(+)

Comments

Peter Maydell May 7, 2024, 9:50 a.m. UTC | #1 
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
Inès Varhol May 7, 2024, 5:30 p.m. UTC | #2 
----- 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
diff mbox series

Patch

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;
 }