| Message ID | 20240426122913.3427983-4-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | target/arm: Make the counter frequency default 1GHz for new CPUs, machines | expand |
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)); > sysbus_realize_and_unref(s, &error_fatal); > sysbus_mmio_map(s, 0, rbase); > sysbus_mmio_map(s, 1, cbase);
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.) -- PMM
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>
diff --git a/include/hw/watchdog/sbsa_gwdt.h b/include/hw/watchdog/sbsa_gwdt.h index 70b137de301..4bdc6c6fdb6 100644 --- a/include/hw/watchdog/sbsa_gwdt.h +++ b/include/hw/watchdog/sbsa_gwdt.h @@ -55,8 +55,6 @@ #define SBSA_GWDT_RMMIO_SIZE 0x1000 #define SBSA_GWDT_CMMIO_SIZE 0x1000 -#define SBSA_TIMER_FREQ 62500000 /* Hz */ - typedef struct SBSA_GWDTState { /* <private> */ SysBusDevice parent_obj; @@ -67,6 +65,7 @@ typedef struct SBSA_GWDTState { qemu_irq irq; QEMUTimer *timer; + uint64_t freq; uint32_t id; uint32_t wcs; 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); sysbus_realize_and_unref(s, &error_fatal); sysbus_mmio_map(s, 0, rbase); sysbus_mmio_map(s, 1, cbase); diff --git a/hw/watchdog/sbsa_gwdt.c b/hw/watchdog/sbsa_gwdt.c index 96895d76369..d437535cc66 100644 --- a/hw/watchdog/sbsa_gwdt.c +++ b/hw/watchdog/sbsa_gwdt.c @@ -18,6 +18,7 @@ #include "qemu/osdep.h" #include "sysemu/reset.h" #include "sysemu/watchdog.h" +#include "hw/qdev-properties.h" #include "hw/watchdog/sbsa_gwdt.h" #include "qemu/timer.h" #include "migration/vmstate.h" @@ -109,7 +110,7 @@ static void sbsa_gwdt_update_timer(SBSA_GWDTState *s, WdtRefreshType rtype) timeout = s->woru; timeout <<= 32; timeout |= s->worl; - timeout = muldiv64(timeout, NANOSECONDS_PER_SECOND, SBSA_TIMER_FREQ); + timeout = muldiv64(timeout, NANOSECONDS_PER_SECOND, s->freq); timeout += qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); if ((rtype == EXPLICIT_REFRESH) || ((rtype == TIMEOUT_REFRESH) && @@ -261,6 +262,17 @@ static void wdt_sbsa_gwdt_realize(DeviceState *dev, Error **errp) dev); } +static Property wdt_sbsa_gwdt_props[] = { + /* + * Timer frequency in Hz. This must match the frequency used by + * the CPU's generic timer. Default 62.5Hz matches QEMU's legacy + * CPU timer frequency default. + */ + DEFINE_PROP_UINT64("clock-frequency", struct SBSA_GWDTState, freq, + 62500000), + DEFINE_PROP_END_OF_LIST(), +}; + static void wdt_sbsa_gwdt_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); @@ -271,6 +283,7 @@ static void wdt_sbsa_gwdt_class_init(ObjectClass *klass, void *data) set_bit(DEVICE_CATEGORY_WATCHDOG, dc->categories); dc->vmsd = &vmstate_sbsa_gwdt; dc->desc = "SBSA-compliant generic watchdog device"; + device_class_set_props(dc, wdt_sbsa_gwdt_props); } static const TypeInfo wdt_sbsa_gwdt_info = {
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(-)