diff mbox series

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

Message ID 20240505140556.373711-4-ines.varhol@telecom-paris.fr (mailing list archive)
State New, archived
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>
---
 hw/char/stm32l4x5_usart.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Philippe Mathieu-Daudé May 6, 2024, 9:34 a.m. UTC | #1 
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)
Philippe Mathieu-Daudé May 6, 2024, 9:43 a.m. UTC | #2 
(+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)
>
Peter Maydell May 7, 2024, 9:54 a.m. UTC | #3 
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
Philippe Mathieu-Daudé May 8, 2024, 2:08 p.m. UTC | #4 
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
diff mbox series

Patch

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)