Re: [PATCH 15/15] hw/timer/arm_timer: QOM'ify ARM_TIMER

[Philippe Mathieu-Daudé]

On 3/6/23 20:12, Mark Cave-Ayland wrote:
> On 03/06/2023 19:07, Mark Cave-Ayland wrote:
>
> > On 31/05/2023 21:35, Philippe Mathieu-Daudé wrote:
> >
> > > Introduce the ARM_TIMER sysbus device.
> > >
> > > arm_timer_new() is converted as QOM instance init()/finalize()
> > > handlers. Note in arm_timer_finalize() we release a ptimer handle
> > > which was previously leaked.
> > >
> > > ArmTimerState is directly embedded into SP804State/IcpPitState,
> > > and is initialized as a QOM child.
> > >
> > > Since the timer frequency belongs to ARM_TIMER, have it hold the
> > > QOM property. SP804State/IcpPitState directly access it.
> > >
> > > Similarly the SP804State/IcpPitState input IRQ becomes the
> > > ARM_TIMER sysbus output IRQ.
> > >
> > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > ---
> > >   hw/timer/arm_timer.c | 109 +++++++++++++++++++++++++++----------------
> > >   1 file changed, 70 insertions(+), 39 deletions(-)
> > >
> > > diff --git a/hw/timer/arm_timer.c b/hw/timer/arm_timer.c
> > > index 82123b40c0..a929fbba62 100644
> > > --- a/hw/timer/arm_timer.c
> > > +++ b/hw/timer/arm_timer.c
> > > @@ -17,6 +17,7 @@
> > >   #include "qemu/module.h"
> > >   #include "qemu/log.h"
> > >   #include "qom/object.h"
> > > +#include "qapi/error.h"
> > >   /* Common timer implementation.  */
> > > @@ -29,14 +30,18 @@
> > >   #define TIMER_CTRL_PERIODIC     (1 << 6)
> > >   #define TIMER_CTRL_ENABLE       (1 << 7)
> > > -typedef struct {
> > > +#define TYPE_ARM_TIMER "arm-timer"
> > > +OBJECT_DECLARE_SIMPLE_TYPE(ArmTimerState, ARM_TIMER)
> >
> > As per our QOM guidelines ArmTimerState and the OBJECT_* macro should 
> > live in a separate header file.
>
> Ah wait: does "ArmTimerState is directly embedded into 
> SP804State/IcpPitState, and is initialized as a QOM child." mean that 
> ARM_TIMER is never instantiated externally?

Correct, while the type is exposed as any QOM type, it is internal to
the two devices, thus local to this unit.

I don't mind exposing the state to have a consistent QOM style.

What was discussed with Alex is:
- We don't need to convert all non-QOM devices, but
- Heterogeneous machines must contain only QOM devices;

- If a non-QOM device forces incorrect API use or abuses,
  better convert it.

> > > +struct ArmTimerState {
> > > +    SysBusDevice parent_obj;
> >
> > And don't forget to add a blank line here too.

OK.

> > >       ptimer_state *timer;
> > >       uint32_t control;
> > >       uint32_t limit;
> > >       uint32_t freq;
> > >       int int_level;
> > >       qemu_irq irq;
> > > -} ArmTimerState;
> > > +};