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

[Peter Maydell]

On Mon, 5 Jun 2023 at 11:20, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 3/6/23 20: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(-)
>
>
> >> -static void arm_timer_reset(ArmTimerState *s)
> >> +static void arm_timer_reset(DeviceState *dev)
> >>   {
> >> +    ArmTimerState *s = ARM_TIMER(dev);
> >> +
> >>       s->control = TIMER_CTRL_IE;
> >>   }
> >
> > If you're currently set up to test the ARM timers with these changes, is
> > it worth considering converting this to use the Resettable interface at
> > the same time?
>
> Good point. Then ARM_TIMER doesn't need to inherit from SysBus: if the
> parent device resets it explicitly, it can be a plan QDev.
>
> Peter, what do you think?

I'm not a super-fan of either plain qdevs or devices explicitly
resetting other devices. What we have today isn't great
(reset along the sysbus tree) but I feel like ad-hoc deviations
from it don't help and arguably hinder in any future attempts
to attack the problem more systematically.

> Even generically, I wonder if a QDev could resets all its QOM children,
> propagating each ResetType.

Propagating reset along the QOM tree is something I've thought
about, yes. The other option is to have a 'reset tree' (distinct
from both the QOM tree and the 'bus tree', but perhaps defaulting
to the QOM tree if not explicitly set otherwise). As usual, the
difficulty is the transition from the current setup to any
proposed new reset handling model. I haven't really had time
to think about this recently.

-- PMM