Re: [PATCH v2 3/4] hw/watchdog/sbsa_gwdt: Make watchdog timer frequency a QOM property

[Philippe Mathieu-Daudé]

On 26/4/24 15:28, Peter Maydell wrote:
> On Fri, 26 Apr 2024 at 13:46, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> > Hi Peter,
> >
> > On 26/4/24 14:29, Peter Maydell wrote:
> > > Currently the sbsa_gdwt watchdog device hardcodes its frequency at
> > > 62.5MHz. In real hardware, this watchdog is supposed to be driven
> > > from the system counter, which also drives the CPU generic timers.
> > > Newer CPU types (in particular from Armv8.6) should have a CPU
> > > generic timer frequency of 1GHz, so we can't leave the watchdog
> > > on the old QEMU default of 62.5GHz.
> > >
> > > Make the frequency a QOM property so it can be set by the board,
> > > and have our only board that uses this device set that frequency
> > > to the same value it sets the CPU frequency.
> > >
> > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > > ---
> > >    include/hw/watchdog/sbsa_gwdt.h |  3 +--
> > >    hw/arm/sbsa-ref.c               |  1 +
> > >    hw/watchdog/sbsa_gwdt.c         | 15 ++++++++++++++-
> > >    3 files changed, 16 insertions(+), 3 deletions(-)
> >
> >
> > > diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
> > > index 36f6f717b4b..57c337fd92a 100644
> > > --- a/hw/arm/sbsa-ref.c
> > > +++ b/hw/arm/sbsa-ref.c
> > > @@ -543,6 +543,7 @@ static void create_wdt(const SBSAMachineState *sms)
> > >        SysBusDevice *s = SYS_BUS_DEVICE(dev);
> > >        int irq = sbsa_ref_irqmap[SBSA_GWDT_WS0];
> > >
> > > +    qdev_prop_set_uint64(dev, "clock-frequency", SBSA_GTIMER_HZ);
> >
> > Since we have access to the CPU and its generic timer, what about
> > just keep the wdg in sync, as smth like:
> >
> >     qdev_prop_set_uint64(dev, "clock-frequency",
> >                          object_property_get_uint(OBJECT(some_cpu),
> >                                                   "cntfrq", errp));
>
> That introduces an implicit ordering requirement that
> the CPU has been created before the watchdog, which I'm
> not super enthusiastic about. "The platform knows the
> frequency and sets it on the devices that care" seems
> more straightforward to me.
>
> (The really-follow-the-hardware approach here would be to
> model the memory mapped system counter and then wire that
> up to both the CPUs and the watchdog, but that's a lot
> of extra work. I have some half-baked patches in that
> direction but for the moment I figure doing the simple
> thing is all we need.)

Yeah, since the clock is fixed, the current patch is
good enough.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>