On Mon, 6 May 2024 at 10:34, Philippe Mathieu-Daudé <phi...@linaro.org> wrote:
>
> Hi,
>
> On 5/5/24 16:05, Inès Varhol wrote:
> > Signed-off-by: Inès Varhol <ines.var...@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
