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

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

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)

(+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)
>

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

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