* [PATCH v6 0/2] Add watchdog support for SbsaQemu @ 2020-10-15 16:34 Shashi Mallela 2020-10-15 16:34 ` [PATCH v6 1/2] hw/watchdog: Implement SBSA watchdog device Shashi Mallela 2020-10-15 16:34 ` [PATCH v6 2/2] hw/arm/sbsa-ref: add " Shashi Mallela 0 siblings, 2 replies; 6+ messages in thread From: Shashi Mallela @ 2020-10-15 16:34 UTC (permalink / raw) To: peter.maydell, leif, rad; +Cc: qemu-arm, qemu-devel This patch series adds watchdog timer support for SbsaQemu platform. The watchdog timer has been implemented first based on the generic watchdog timer specifications from ARM BSA v0.9 and then used in the SbsaQemu reference platform Changes in v6: - removed the fdt code for wdt as it has no significance in sbsa Shashi Mallela (2): hw/watchdog: Implement SBSA watchdog device hw/arm/sbsa-ref: add SBSA watchdog device hw/arm/Kconfig | 1 + hw/arm/sbsa-ref.c | 24 ++ hw/watchdog/Kconfig | 4 + hw/watchdog/meson.build | 1 + hw/watchdog/wdt_sbsa_gwdt.c | 346 ++++++++++++++++++++++++++++ include/hw/watchdog/wdt_sbsa_gwdt.h | 70 ++++++ 6 files changed, 446 insertions(+) create mode 100644 hw/watchdog/wdt_sbsa_gwdt.c create mode 100644 include/hw/watchdog/wdt_sbsa_gwdt.h -- 2.18.4 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v6 1/2] hw/watchdog: Implement SBSA watchdog device 2020-10-15 16:34 [PATCH v6 0/2] Add watchdog support for SbsaQemu Shashi Mallela @ 2020-10-15 16:34 ` Shashi Mallela 2020-10-20 14:28 ` Peter Maydell 2020-10-15 16:34 ` [PATCH v6 2/2] hw/arm/sbsa-ref: add " Shashi Mallela 1 sibling, 1 reply; 6+ messages in thread From: Shashi Mallela @ 2020-10-15 16:34 UTC (permalink / raw) To: peter.maydell, leif, rad; +Cc: qemu-arm, qemu-devel Generic watchdog device model has been implemented as per ARM BSAv0.9 Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org> --- hw/arm/Kconfig | 1 + hw/watchdog/Kconfig | 4 + hw/watchdog/meson.build | 1 + hw/watchdog/wdt_sbsa_gwdt.c | 346 ++++++++++++++++++++++++++++ include/hw/watchdog/wdt_sbsa_gwdt.h | 70 ++++++ 5 files changed, 422 insertions(+) create mode 100644 hw/watchdog/wdt_sbsa_gwdt.c create mode 100644 include/hw/watchdog/wdt_sbsa_gwdt.h diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig index f303c6bead25..6b97e64595d3 100644 --- a/hw/arm/Kconfig +++ b/hw/arm/Kconfig @@ -210,6 +210,7 @@ config SBSA_REF select PL031 # RTC select PL061 # GPIO select USB_EHCI_SYSBUS + select WDT_SBSA_GWDT config SABRELITE bool diff --git a/hw/watchdog/Kconfig b/hw/watchdog/Kconfig index 293209b291d6..ea9cadd66f22 100644 --- a/hw/watchdog/Kconfig +++ b/hw/watchdog/Kconfig @@ -17,3 +17,7 @@ config WDT_DIAG288 config WDT_IMX2 bool + +config WDT_SBSA_GWDT + bool + default y if SBSA_REF diff --git a/hw/watchdog/meson.build b/hw/watchdog/meson.build index 9b8725e64288..a9a23307acfe 100644 --- a/hw/watchdog/meson.build +++ b/hw/watchdog/meson.build @@ -5,3 +5,4 @@ softmmu_ss.add(when: 'CONFIG_WDT_IB700', if_true: files('wdt_ib700.c')) softmmu_ss.add(when: 'CONFIG_WDT_DIAG288', if_true: files('wdt_diag288.c')) softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files('wdt_aspeed.c')) softmmu_ss.add(when: 'CONFIG_WDT_IMX2', if_true: files('wdt_imx2.c')) +softmmu_ss.add(when: 'CONFIG_WDT_SBSA_GWDT', if_true: files('wdt_sbsa_gwdt.c')) diff --git a/hw/watchdog/wdt_sbsa_gwdt.c b/hw/watchdog/wdt_sbsa_gwdt.c new file mode 100644 index 000000000000..1c2fe04db7c7 --- /dev/null +++ b/hw/watchdog/wdt_sbsa_gwdt.c @@ -0,0 +1,346 @@ +/* + * Generic watchdog device model for SBSA + * + * Copyright Linaro.org 2020 + * + * Authors: + * Shashi Mallela <shashi.mallela@linaro.org> + * + * This work is licensed under the terms of the GNU GPL, version 2 or (at your + * option) any later version. See the COPYING file in the top-level directory. + * + */ + +#include "qemu/osdep.h" +#include "sysemu/reset.h" +#include "sysemu/watchdog.h" +#include "hw/watchdog/wdt_sbsa_gwdt.h" +#include "qemu/timer.h" +#include "migration/vmstate.h" +#include "qemu/log.h" +#include "qemu/module.h" + +static WatchdogTimerModel model = { + .wdt_name = TYPE_WDT_SBSA_GWDT, + .wdt_description = "sbsa_gwdt device for sbsa_ref platform", +}; + +static const VMStateDescription vmstate_sbsa_gwdt = { + .name = "vmstate_sbsa_gwdt", + .version_id = 1, + .minimum_version_id = 1, + .fields = (VMStateField[]) { + VMSTATE_TIMER_PTR(timer, SBSA_GWDTState), + VMSTATE_BOOL(enabled, SBSA_GWDTState), + VMSTATE_BOOL(ws0, SBSA_GWDTState), + VMSTATE_BOOL(ws1, SBSA_GWDTState), + VMSTATE_UINT32(wrr, SBSA_GWDTState), + VMSTATE_UINT32(wcs, SBSA_GWDTState), + VMSTATE_UINT32(worl, SBSA_GWDTState), + VMSTATE_UINT32(woru, SBSA_GWDTState), + VMSTATE_UINT32(wcvl, SBSA_GWDTState), + VMSTATE_UINT32(wcvu, SBSA_GWDTState), + VMSTATE_END_OF_LIST() + } +}; + +static uint64_t sbsa_gwdt_rread(void *opaque, hwaddr addr, unsigned int size) +{ + uint32_t ret = 0; + + if (addr == SBSA_GWDT_WRR) { + /* watch refresh read has no effect and returns 0 */ + ret = 0; + } else { + qemu_log_mask(LOG_GUEST_ERROR, "bad address in refresh frame read :" + " 0x%x\n", (int)addr); + } + return ret; +} + +static uint64_t sbsa_gwdt_read(void *opaque, hwaddr addr, unsigned int size) +{ + SBSA_GWDTState *s = SBSA_GWDT(opaque); + uint32_t ret = 0; + + switch (addr) { + case SBSA_GWDT_WCS: + ret = s->wcs; + break; + case SBSA_GWDT_WOR: + ret = s->worl; + break; + case SBSA_GWDT_WORU: + ret = s->woru; + break; + case SBSA_GWDT_WCV: + ret = s->wcvl; + break; + case SBSA_GWDT_WCVU: + ret = s->wcvu; + break; + case SBSA_GWDT_W_IIDR: + ret = s->id; + break; + default: + qemu_log_mask(LOG_GUEST_ERROR, "bad address in control frame read :" + " 0x%x\n", (int)addr); + } + return ret; +} + +static void sbsa_gwdt_update_timer(SBSA_GWDTState *s) +{ + uint64_t timeout = 0; + + if (s->enabled) { + /* + * Extract the upper 16 bits from woru & 32 bits from worl + * registers to construct the 48 bit offset value + */ + timeout = s->woru & SBSA_GWDT_WOR_MASK; + timeout <<= 32; + timeout |= s->worl; + timeout = muldiv64(timeout, NANOSECONDS_PER_SECOND, SBSA_TIMER_FREQ); + timeout += qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); + + /* store the current timeout value into compare registers */ + s->wcvu = timeout >> 32; + s->wcvl = timeout; + + if (!s->ws0) { + timer_del(s->timer); + timer_mod(s->ptimer, timeout); + } else { + timer_del(s->ptimer); + timer_mod(s->timer, timeout); + } + } else { + timer_del(s->ptimer); + timer_del(s->timer); + } +} + +static void sbsa_gwdt_rwrite(void *opaque, hwaddr offset, uint64_t data, + unsigned size) { + SBSA_GWDTState *s = SBSA_GWDT(opaque); + + if (offset == SBSA_GWDT_WRR) { + s->wrr = data; + s->wcs &= ~SBSA_GWDT_WCS_WS0; + s->wcs &= ~SBSA_GWDT_WCS_WS1; + s->ws0 = false; + s->ws1 = false; + sbsa_gwdt_update_timer(s); + } else { + qemu_log_mask(LOG_GUEST_ERROR, "bad address in refresh frame write :" + " 0x%x\n", (int)offset); + } +} + +static void sbsa_gwdt_write(void *opaque, hwaddr offset, uint64_t data, + unsigned size) { + SBSA_GWDTState *s = SBSA_GWDT(opaque); + bool enable; + + switch (offset) { + case SBSA_GWDT_WCS: + enable = data & SBSA_GWDT_WCS_EN; + if (enable) { + s->wcs |= SBSA_GWDT_WCS_EN; + s->enabled = true; + } else { + s->wcs &= ~SBSA_GWDT_WCS_EN; + s->enabled = false; + } + s->ws0 = false; + s->ws1 = false; + s->wcs &= ~SBSA_GWDT_WCS_WS0; + s->wcs &= ~SBSA_GWDT_WCS_WS1; + sbsa_gwdt_update_timer(s); + break; + + case SBSA_GWDT_WOR: + s->worl = data; + s->ws0 = false; + s->ws1 = false; + s->wcs &= ~SBSA_GWDT_WCS_WS0; + s->wcs &= ~SBSA_GWDT_WCS_WS1; + /* + * TODO:- setting woru to 0 and triggering update timer(below) is a + * temporary workaround to handle current linux driver which is + * based on earlier version of BSA specification.Once the linux + * driver is updated to BSA v0.9 will remove these next 2 lines. + */ + s->woru = 0; + sbsa_gwdt_update_timer(s); + break; + + case SBSA_GWDT_WORU: + s->woru = data; + s->ws0 = false; + s->ws1 = false; + s->wcs &= ~SBSA_GWDT_WCS_WS0; + s->wcs &= ~SBSA_GWDT_WCS_WS1; + sbsa_gwdt_update_timer(s); + break; + + case SBSA_GWDT_WCV: + s->wcvl = data; + break; + + case SBSA_GWDT_WCVU: + s->wcvu = data; + break; + + default: + qemu_log_mask(LOG_GUEST_ERROR, "bad address in control frame write :" + " 0x%x\n", (int)offset); + } + return; +} + +static void wdt_sbsa_gwdt_reset(DeviceState *dev) +{ + SBSA_GWDTState *s = SBSA_GWDT(dev); + + timer_del(s->ptimer); + timer_del(s->timer); + + s->enabled = false; + s->ws0 = false; + s->ws1 = false; + s->wcs &= ~SBSA_GWDT_WCS_EN; + s->wcs &= ~SBSA_GWDT_WCS_WS0; + s->wcs &= ~SBSA_GWDT_WCS_WS1; + s->wcvl = 0; + s->wcvu = 0; + s->worl = 0; + s->woru = 0; + s->id = SBSA_GWDT_ID; +} + +static void sbsa_gwdt_reset(void *opaque) +{ + DeviceState *sbsa_gwdt = opaque; + + wdt_sbsa_gwdt_reset(sbsa_gwdt); +} + +static void sbsa_gwdt_timer_sysinterrupt(void *opaque) +{ + SBSA_GWDTState *s = SBSA_GWDT(opaque); + + s->wcs |= SBSA_GWDT_WCS_WS0; + s->ws0 = true; + qemu_set_irq(s->irq, 1); + sbsa_gwdt_update_timer(s); +} + +static void sbsa_gwdt_timer_sysreset(void *dev) +{ + SBSA_GWDTState *s = SBSA_GWDT(dev); + + s->wcs |= SBSA_GWDT_WCS_WS1; + s->ws1 = true; + qemu_log_mask(CPU_LOG_RESET, "Watchdog timer expired.\n"); + /* + * Reset the watchdog only if the guest gets notified about + * expiry. watchdog_perform_action() may temporarily relinquish + * the BQL; reset before triggering the action to avoid races with + * sbsa_gwdt instructions. + */ + switch (get_watchdog_action()) { + case WATCHDOG_ACTION_DEBUG: + case WATCHDOG_ACTION_NONE: + case WATCHDOG_ACTION_PAUSE: + break; + default: + wdt_sbsa_gwdt_reset(dev); + } + watchdog_perform_action(); +} + +static const MemoryRegionOps sbsa_gwdt_rops = { + .read = sbsa_gwdt_rread, + .write = sbsa_gwdt_rwrite, + .endianness = DEVICE_LITTLE_ENDIAN, + .valid.min_access_size = 4, + .valid.max_access_size = 4, + .valid.unaligned = false, +}; + +static const MemoryRegionOps sbsa_gwdt_ops = { + .read = sbsa_gwdt_read, + .write = sbsa_gwdt_write, + .endianness = DEVICE_LITTLE_ENDIAN, + .valid.min_access_size = 4, + .valid.max_access_size = 4, + .valid.unaligned = false, +}; + +static void wdt_sbsa_gwdt_realize(DeviceState *dev, Error **errp) +{ + SBSA_GWDTState *s = SBSA_GWDT(dev); + SysBusDevice *sbd = SYS_BUS_DEVICE(dev); + + memory_region_init_io(&s->rmmio, OBJECT(dev), + &sbsa_gwdt_rops, s, + "sbsa_gwdt.refresh", + SBSA_GWDT_RMMIO_SIZE); + + memory_region_init_io(&s->cmmio, OBJECT(dev), + &sbsa_gwdt_ops, s, + "sbsa_gwdt.control", + SBSA_GWDT_CMMIO_SIZE); + + sysbus_init_mmio(sbd, &s->rmmio); + sysbus_init_mmio(sbd, &s->cmmio); + + sysbus_init_irq(sbd, &s->irq); + + qemu_register_reset(sbsa_gwdt_reset, s); + + s->ptimer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sbsa_gwdt_timer_sysinterrupt, + dev); + s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sbsa_gwdt_timer_sysreset, + dev); +} + +static void wdt_sbsa_gwdt_unrealize(DeviceState *dev) +{ + SBSA_GWDTState *s = SBSA_GWDT(dev); + + timer_del(s->ptimer); + timer_free(s->ptimer); + + timer_del(s->timer); + timer_free(s->timer); +} + +static void wdt_sbsa_gwdt_class_init(ObjectClass *klass, void *data) +{ + DeviceClass *dc = DEVICE_CLASS(klass); + + dc->realize = wdt_sbsa_gwdt_realize; + dc->unrealize = wdt_sbsa_gwdt_unrealize; + dc->reset = wdt_sbsa_gwdt_reset; + dc->hotpluggable = false; + set_bit(DEVICE_CATEGORY_MISC, dc->categories); + dc->vmsd = &vmstate_sbsa_gwdt; +} + +static const TypeInfo wdt_sbsa_gwdt_info = { + .class_init = wdt_sbsa_gwdt_class_init, + .parent = TYPE_SYS_BUS_DEVICE, + .name = TYPE_WDT_SBSA_GWDT, + .instance_size = sizeof(SBSA_GWDTState), +}; + +static void wdt_sbsa_gwdt_register_types(void) +{ + watchdog_add_model(&model); + type_register_static(&wdt_sbsa_gwdt_info); +} + +type_init(wdt_sbsa_gwdt_register_types) diff --git a/include/hw/watchdog/wdt_sbsa_gwdt.h b/include/hw/watchdog/wdt_sbsa_gwdt.h new file mode 100644 index 000000000000..70ba7abb3ace --- /dev/null +++ b/include/hw/watchdog/wdt_sbsa_gwdt.h @@ -0,0 +1,70 @@ +#ifndef WDT_SBSA_GWDT_H +#define WDT_SBSA_GWDT_H + +#include "qemu/bitops.h" +#include "hw/sysbus.h" +#include "hw/irq.h" + +#define TYPE_WDT_SBSA_GWDT "sbsa_gwdt" +#define SBSA_GWDT(obj) \ + OBJECT_CHECK(SBSA_GWDTState, (obj), TYPE_WDT_SBSA_GWDT) +#define SBSA_GWDT_CLASS(klass) \ + OBJECT_CLASS_CHECK(SBSA_GWDTClass, (klass), TYPE_WDT_SBSA_GWDT) +#define SBSA_GWDT_GET_CLASS(obj) \ + OBJECT_GET_CLASS(SBSA_GWDTClass, (obj), TYPE_WDT_SBSA_GWDT) + +/* SBSA Generic Watchdog register definitions */ +/* refresh frame */ +#define SBSA_GWDT_WRR 0x000 + +/* control frame */ +#define SBSA_GWDT_WCS 0x000 +#define SBSA_GWDT_WOR 0x008 +#define SBSA_GWDT_WORU 0x00C +#define SBSA_GWDT_WCV 0x010 +#define SBSA_GWDT_WCVU 0x014 + +/* Watchdog Interface Identification Register */ +#define SBSA_GWDT_W_IIDR 0xFCC + +/* Watchdog Control and Status Register Bits */ +#define SBSA_GWDT_WCS_EN BIT(0) +#define SBSA_GWDT_WCS_WS0 BIT(1) +#define SBSA_GWDT_WCS_WS1 BIT(2) + +#define SBSA_GWDT_WOR_MASK 0x0000FFFF + +/* Watchdog Interface Identification Register definition*/ +#define SBSA_GWDT_ID 0x1043B + +/* 2 Separate memory regions for each of refresh & control register frames */ +#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; + + /*< public >*/ + MemoryRegion rmmio; + MemoryRegion cmmio; + qemu_irq irq; + + QEMUTimer *ptimer, *timer; + + uint32_t id; + uint32_t wrr; + uint32_t wcs; + uint32_t worl; + uint32_t woru; + uint32_t wcvl; + uint32_t wcvu; + bool enabled; + bool ws0, ws1; + + /*< public >*/ +} SBSA_GWDTState; + +#endif /* WDT_SBSA_GWDT_H */ -- 2.18.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v6 1/2] hw/watchdog: Implement SBSA watchdog device 2020-10-15 16:34 ` [PATCH v6 1/2] hw/watchdog: Implement SBSA watchdog device Shashi Mallela @ 2020-10-20 14:28 ` Peter Maydell 2020-10-22 15:58 ` shashi.mallela 0 siblings, 1 reply; 6+ messages in thread From: Peter Maydell @ 2020-10-20 14:28 UTC (permalink / raw) To: Shashi Mallela Cc: Leif Lindholm, QEMU Developers, qemu-arm, Radoslaw Biernacki On Thu, 15 Oct 2020 at 17:35, Shashi Mallela <shashi.mallela@linaro.org> wrote: > > Generic watchdog device model has been implemented as per ARM BSAv0.9 Hmm, so the patch title, filenames, etc, refer to the SBSA, which is the Server Base System Architecture spec. Is the BSA a different spec ? We should be consistent with our terminology and clear about what spec we're implementing. (I've used the SBSA v6.0 as reference in the review comments below.) > Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org> > --- > hw/arm/Kconfig | 1 + > hw/watchdog/Kconfig | 4 + > hw/watchdog/meson.build | 1 + > hw/watchdog/wdt_sbsa_gwdt.c | 346 ++++++++++++++++++++++++++++ This filename is rather redundant as it says "watchdog" three times. hw/watchdog/sbsa_gwdt.c will do (similarly the .h file). > include/hw/watchdog/wdt_sbsa_gwdt.h | 70 ++++++ > 5 files changed, 422 insertions(+) > create mode 100644 hw/watchdog/wdt_sbsa_gwdt.c > create mode 100644 include/hw/watchdog/wdt_sbsa_gwdt.h > > diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig > index f303c6bead25..6b97e64595d3 100644 > --- a/hw/arm/Kconfig > +++ b/hw/arm/Kconfig > @@ -210,6 +210,7 @@ config SBSA_REF > select PL031 # RTC > select PL061 # GPIO > select USB_EHCI_SYSBUS > + select WDT_SBSA_GWDT > > config SABRELITE > bool > diff --git a/hw/watchdog/Kconfig b/hw/watchdog/Kconfig > index 293209b291d6..ea9cadd66f22 100644 > --- a/hw/watchdog/Kconfig > +++ b/hw/watchdog/Kconfig > @@ -17,3 +17,7 @@ config WDT_DIAG288 > > config WDT_IMX2 > bool > + > +config WDT_SBSA_GWDT > + bool > + default y if SBSA_REF I don't think you need the "default" line here. The board config "select"s the device, so if the user enables the board they'll get the device, which is all you need. > diff --git a/hw/watchdog/meson.build b/hw/watchdog/meson.build > index 9b8725e64288..a9a23307acfe 100644 > --- a/hw/watchdog/meson.build > +++ b/hw/watchdog/meson.build > @@ -5,3 +5,4 @@ softmmu_ss.add(when: 'CONFIG_WDT_IB700', if_true: files('wdt_ib700.c')) > softmmu_ss.add(when: 'CONFIG_WDT_DIAG288', if_true: files('wdt_diag288.c')) > softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files('wdt_aspeed.c')) > softmmu_ss.add(when: 'CONFIG_WDT_IMX2', if_true: files('wdt_imx2.c')) > +softmmu_ss.add(when: 'CONFIG_WDT_SBSA_GWDT', if_true: files('wdt_sbsa_gwdt.c')) > diff --git a/hw/watchdog/wdt_sbsa_gwdt.c b/hw/watchdog/wdt_sbsa_gwdt.c > new file mode 100644 > index 000000000000..1c2fe04db7c7 > --- /dev/null > +++ b/hw/watchdog/wdt_sbsa_gwdt.c > @@ -0,0 +1,346 @@ > +/* > + * Generic watchdog device model for SBSA > + * > + * Copyright Linaro.org 2020 > + * > + * Authors: > + * Shashi Mallela <shashi.mallela@linaro.org> > + * > + * This work is licensed under the terms of the GNU GPL, version 2 or (at your > + * option) any later version. See the COPYING file in the top-level directory. > + * > + */ This is a good place for a comment which gives the title and a URL for the specification the device is based on. It would also be helpful to say what revision of the watchdog we implement (the SBSA 6.0 spec says there is a "revision 0" and also a "revision 1"; looks like we're revision 1 ?) > + > +#include "qemu/osdep.h" > +#include "sysemu/reset.h" > +#include "sysemu/watchdog.h" > +#include "hw/watchdog/wdt_sbsa_gwdt.h" > +#include "qemu/timer.h" > +#include "migration/vmstate.h" > +#include "qemu/log.h" > +#include "qemu/module.h" > + > +static WatchdogTimerModel model = { > + .wdt_name = TYPE_WDT_SBSA_GWDT, > + .wdt_description = "sbsa_gwdt device for sbsa_ref platform", Other wdt_description strings use the human-facing name of the device, so something like "Generic SBSA-compliant watchdog device" seems a better match to the others. (The board name is 'sbsa-ref' with a hyphen, by the way.) > +}; > + > +static const VMStateDescription vmstate_sbsa_gwdt = { > + .name = "vmstate_sbsa_gwdt", You can drop the 'vmstate_' prefix. Just "sbsa-gwdt" will do. > + .version_id = 1, > + .minimum_version_id = 1, > + .fields = (VMStateField[]) { > + VMSTATE_TIMER_PTR(timer, SBSA_GWDTState), > + VMSTATE_BOOL(enabled, SBSA_GWDTState), > + VMSTATE_BOOL(ws0, SBSA_GWDTState), > + VMSTATE_BOOL(ws1, SBSA_GWDTState), > + VMSTATE_UINT32(wrr, SBSA_GWDTState), > + VMSTATE_UINT32(wcs, SBSA_GWDTState), > + VMSTATE_UINT32(worl, SBSA_GWDTState), > + VMSTATE_UINT32(woru, SBSA_GWDTState), > + VMSTATE_UINT32(wcvl, SBSA_GWDTState), > + VMSTATE_UINT32(wcvu, SBSA_GWDTState), > + VMSTATE_END_OF_LIST() This is missing 'ptimer' (but see below for a review comment where I recommend dropping 'ptimer' entirely). > + } > +}; > + > +static uint64_t sbsa_gwdt_rread(void *opaque, hwaddr addr, unsigned int size) > +{ > + uint32_t ret = 0; > + > + if (addr == SBSA_GWDT_WRR) { > + /* watch refresh read has no effect and returns 0 */ > + ret = 0; > + } else { > + qemu_log_mask(LOG_GUEST_ERROR, "bad address in refresh frame read :" > + " 0x%x\n", (int)addr); > + } The spec says that reading from 0xfcc is valid in both the refresh and the control frame (returning the same ID value in both cases). > + return ret; > +} > + > +static uint64_t sbsa_gwdt_read(void *opaque, hwaddr addr, unsigned int size) > +{ > + SBSA_GWDTState *s = SBSA_GWDT(opaque); > + uint32_t ret = 0; > + > + switch (addr) { > + case SBSA_GWDT_WCS: > + ret = s->wcs; > + break; > + case SBSA_GWDT_WOR: > + ret = s->worl; > + break; > + case SBSA_GWDT_WORU: > + ret = s->woru; > + break; > + case SBSA_GWDT_WCV: > + ret = s->wcvl; > + break; > + case SBSA_GWDT_WCVU: > + ret = s->wcvu; > + break; > + case SBSA_GWDT_W_IIDR: > + ret = s->id; > + break; > + default: > + qemu_log_mask(LOG_GUEST_ERROR, "bad address in control frame read :" > + " 0x%x\n", (int)addr); > + } > + return ret; > +} > + > +static void sbsa_gwdt_update_timer(SBSA_GWDTState *s) > +{ > + uint64_t timeout = 0; The spec talks about "explicit refresh" and "timeout refresh" events. Is this function implementing both of those ? If so, a function name indicating that would be helpful to the reader. If it's only implementing part of the refresh semantics then an explanatory comment would help. > + > + if (s->enabled) { > + /* > + * Extract the upper 16 bits from woru & 32 bits from worl > + * registers to construct the 48 bit offset value > + */ > + timeout = s->woru & SBSA_GWDT_WOR_MASK; > + timeout <<= 32; > + timeout |= s->worl; > + timeout = muldiv64(timeout, NANOSECONDS_PER_SECOND, SBSA_TIMER_FREQ); > + timeout += qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > + > + /* store the current timeout value into compare registers */ > + s->wcvu = timeout >> 32; > + s->wcvl = timeout; The spec says that for an SBSA level 2 or higher system, the compare value shouldn't be updated for a timeout-refresh when WS0 is asserted (and for SBSA level 0 or 1 the level 2 behaviour is permitted as an IMPDEF option). > + > + if (!s->ws0) { > + timer_del(s->timer); > + timer_mod(s->ptimer, timeout); > + } else { > + timer_del(s->ptimer); > + timer_mod(s->timer, timeout); > + } > + } else { > + timer_del(s->ptimer); > + timer_del(s->timer); Here you have two timers, "timer" and "ptimer", which are set up so that you always have exactly one running. It would be simpler to have a single timer, and then decide what you're doing on expiry by looking at the device state in its callback function. > + } > +} > + > +static void sbsa_gwdt_rwrite(void *opaque, hwaddr offset, uint64_t data, > + unsigned size) { > + SBSA_GWDTState *s = SBSA_GWDT(opaque); > + > + if (offset == SBSA_GWDT_WRR) { > + s->wrr = data; There is no code path which allows the guest to read s->wrr and nothing in the device model does anything with it, so there is no need to have it be state at all. That is, a write to WRR is a trigger for behaviour, but no state is stored. > + s->wcs &= ~SBSA_GWDT_WCS_WS0; > + s->wcs &= ~SBSA_GWDT_WCS_WS1; > + s->ws0 = false; > + s->ws1 = false; > + sbsa_gwdt_update_timer(s); > + } else { > + qemu_log_mask(LOG_GUEST_ERROR, "bad address in refresh frame write :" > + " 0x%x\n", (int)offset); > + } > +} > + > +static void sbsa_gwdt_write(void *opaque, hwaddr offset, uint64_t data, > + unsigned size) { > + SBSA_GWDTState *s = SBSA_GWDT(opaque); > + bool enable; > + > + switch (offset) { > + case SBSA_GWDT_WCS: > + enable = data & SBSA_GWDT_WCS_EN; > + if (enable) { > + s->wcs |= SBSA_GWDT_WCS_EN; > + s->enabled = true; > + } else { > + s->wcs &= ~SBSA_GWDT_WCS_EN; > + s->enabled = false; > + } > + s->ws0 = false; > + s->ws1 = false; > + s->wcs &= ~SBSA_GWDT_WCS_WS0; > + s->wcs &= ~SBSA_GWDT_WCS_WS1; You are storing state duplicated in two places here. You can either: * store the state of bits like the WS0, WS1, Enable as bits inside an s->wcs register value -- in this case reads and writes are easy, and if you want to test for "is device enabled" you do it with a logical operation on s->wcs; there would be no separate ws0/ws1/enabled fields in the state struct * or you can store them as separate boolean fields, in which case you don't need an s->wcs at all. Instead in the read function you assemble the value the guest reads from the ws0/ws1/enable booleans Which one is preferable depends on the device and whether there's a lot of code that often tests the various bits. You can pick either option, but don't do both at once the way this code is. Similarly, you have a choice of whether you prefer to hold the WCV and WOR values in the state struct as a pair of uint32_t (trivial to read/write, need to reassemble into the full value whenever you want to operate on it) or as a single uint64_t (easier for the device to operate on, register reads/writes become extract64/deposit64 operations). > + sbsa_gwdt_update_timer(s); > + break; > + > + case SBSA_GWDT_WOR: > + s->worl = data; > + s->ws0 = false; > + s->ws1 = false; > + s->wcs &= ~SBSA_GWDT_WCS_WS0; > + s->wcs &= ~SBSA_GWDT_WCS_WS1; > + /* > + * TODO:- setting woru to 0 and triggering update timer(below) is a > + * temporary workaround to handle current linux driver which is > + * based on earlier version of BSA specification.Once the linux > + * driver is updated to BSA v0.9 will remove these next 2 lines. > + */ No workarounds for buggy Linux guest drivers, please. Once they get into the code base they are almost impossible to ever get out again. You can either: 1) just implement the newer version of the specification, and fix the guest drivers 2) if you must have a system that works with older guests, then implement *both* older and newer specs, using a QOM property on the device to select which one. You'd need to implement the details of the older version of the spec completely, not just "this is the one thing that the Linux driver happens to care about". You'd also need a machine property on the sbsa-ref board so the user could select whether they wanted the old or new watchdog device. I'd recommend option 1, really. > + s->woru = 0; > + sbsa_gwdt_update_timer(s); > + break; > + > + case SBSA_GWDT_WORU: > + s->woru = data; This and WCVU have the top 16 bits RAZ/WI, so you should mask out the top half of 'data' so the guest can't write an over-large value. > + s->ws0 = false; > + s->ws1 = false; > + s->wcs &= ~SBSA_GWDT_WCS_WS0; > + s->wcs &= ~SBSA_GWDT_WCS_WS1; > + sbsa_gwdt_update_timer(s); > + break; > + > + case SBSA_GWDT_WCV: > + s->wcvl = data; > + break; > + > + case SBSA_GWDT_WCVU: > + s->wcvu = data; > + break; > + > + default: > + qemu_log_mask(LOG_GUEST_ERROR, "bad address in control frame write :" > + " 0x%x\n", (int)offset); > + } > + return; > +} > + > +static void wdt_sbsa_gwdt_reset(DeviceState *dev) > +{ > + SBSA_GWDTState *s = SBSA_GWDT(dev); > + > + timer_del(s->ptimer); > + timer_del(s->timer); > + > + s->enabled = false; > + s->ws0 = false; > + s->ws1 = false; > + s->wcs &= ~SBSA_GWDT_WCS_EN; > + s->wcs &= ~SBSA_GWDT_WCS_WS0; > + s->wcs &= ~SBSA_GWDT_WCS_WS1; > + s->wcvl = 0; > + s->wcvu = 0; > + s->worl = 0; > + s->woru = 0; > + s->id = SBSA_GWDT_ID; > +} > + > +static void sbsa_gwdt_reset(void *opaque) > +{ > + DeviceState *sbsa_gwdt = opaque; > + > + wdt_sbsa_gwdt_reset(sbsa_gwdt); > +} Why do you have two reset functions which do the same thing ? > + > +static void sbsa_gwdt_timer_sysinterrupt(void *opaque) > +{ > + SBSA_GWDTState *s = SBSA_GWDT(opaque); > + > + s->wcs |= SBSA_GWDT_WCS_WS0; > + s->ws0 = true; > + qemu_set_irq(s->irq, 1); > + sbsa_gwdt_update_timer(s); > +} > + > +static void sbsa_gwdt_timer_sysreset(void *dev) > +{ > + SBSA_GWDTState *s = SBSA_GWDT(dev); > + > + s->wcs |= SBSA_GWDT_WCS_WS1; > + s->ws1 = true; > + qemu_log_mask(CPU_LOG_RESET, "Watchdog timer expired.\n"); > + /* > + * Reset the watchdog only if the guest gets notified about > + * expiry. watchdog_perform_action() may temporarily relinquish > + * the BQL; reset before triggering the action to avoid races with > + * sbsa_gwdt instructions. > + */ > + switch (get_watchdog_action()) { > + case WATCHDOG_ACTION_DEBUG: > + case WATCHDOG_ACTION_NONE: > + case WATCHDOG_ACTION_PAUSE: > + break; > + default: > + wdt_sbsa_gwdt_reset(dev); > + } > + watchdog_perform_action(); > +} > + > +static const MemoryRegionOps sbsa_gwdt_rops = { > + .read = sbsa_gwdt_rread, > + .write = sbsa_gwdt_rwrite, > + .endianness = DEVICE_LITTLE_ENDIAN, > + .valid.min_access_size = 4, > + .valid.max_access_size = 4, > + .valid.unaligned = false, > +}; > + > +static const MemoryRegionOps sbsa_gwdt_ops = { > + .read = sbsa_gwdt_read, > + .write = sbsa_gwdt_write, > + .endianness = DEVICE_LITTLE_ENDIAN, > + .valid.min_access_size = 4, > + .valid.max_access_size = 4, > + .valid.unaligned = false, > +}; > + > +static void wdt_sbsa_gwdt_realize(DeviceState *dev, Error **errp) > +{ > + SBSA_GWDTState *s = SBSA_GWDT(dev); > + SysBusDevice *sbd = SYS_BUS_DEVICE(dev); > + > + memory_region_init_io(&s->rmmio, OBJECT(dev), > + &sbsa_gwdt_rops, s, > + "sbsa_gwdt.refresh", > + SBSA_GWDT_RMMIO_SIZE); > + > + memory_region_init_io(&s->cmmio, OBJECT(dev), > + &sbsa_gwdt_ops, s, > + "sbsa_gwdt.control", > + SBSA_GWDT_CMMIO_SIZE); > + > + sysbus_init_mmio(sbd, &s->rmmio); > + sysbus_init_mmio(sbd, &s->cmmio); > + > + sysbus_init_irq(sbd, &s->irq); > + > + qemu_register_reset(sbsa_gwdt_reset, s); No new code should call qemu_register_reset(). You can provide a reset function via dc->reset in your DeviceClass, and in fact you have. > + > + s->ptimer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sbsa_gwdt_timer_sysinterrupt, > + dev); > + s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sbsa_gwdt_timer_sysreset, > + dev); > +} > + > +static void wdt_sbsa_gwdt_unrealize(DeviceState *dev) > +{ > + SBSA_GWDTState *s = SBSA_GWDT(dev); > + > + timer_del(s->ptimer); > + timer_free(s->ptimer); > + > + timer_del(s->timer); > + timer_free(s->timer); > +} This device isn't hot-pluggable, so this unrealize method is unreachable code, I think. > + > +static void wdt_sbsa_gwdt_class_init(ObjectClass *klass, void *data) > +{ > + DeviceClass *dc = DEVICE_CLASS(klass); > + > + dc->realize = wdt_sbsa_gwdt_realize; > + dc->unrealize = wdt_sbsa_gwdt_unrealize; > + dc->reset = wdt_sbsa_gwdt_reset; > + dc->hotpluggable = false; > + set_bit(DEVICE_CATEGORY_MISC, dc->categories); > + dc->vmsd = &vmstate_sbsa_gwdt; > +} > + > +static const TypeInfo wdt_sbsa_gwdt_info = { > + .class_init = wdt_sbsa_gwdt_class_init, > + .parent = TYPE_SYS_BUS_DEVICE, > + .name = TYPE_WDT_SBSA_GWDT, > + .instance_size = sizeof(SBSA_GWDTState), > +}; > + > +static void wdt_sbsa_gwdt_register_types(void) > +{ > + watchdog_add_model(&model); > + type_register_static(&wdt_sbsa_gwdt_info); > +} > + > +type_init(wdt_sbsa_gwdt_register_types) > diff --git a/include/hw/watchdog/wdt_sbsa_gwdt.h b/include/hw/watchdog/wdt_sbsa_gwdt.h > new file mode 100644 > index 000000000000..70ba7abb3ace > --- /dev/null > +++ b/include/hw/watchdog/wdt_sbsa_gwdt.h > @@ -0,0 +1,70 @@ New files, including .h files, should start with a copyright-and-license comment. > +#ifndef WDT_SBSA_GWDT_H > +#define WDT_SBSA_GWDT_H > + > +#include "qemu/bitops.h" > +#include "hw/sysbus.h" > +#include "hw/irq.h" > + > +#define TYPE_WDT_SBSA_GWDT "sbsa_gwdt" > +#define SBSA_GWDT(obj) \ > + OBJECT_CHECK(SBSA_GWDTState, (obj), TYPE_WDT_SBSA_GWDT) > +#define SBSA_GWDT_CLASS(klass) \ > + OBJECT_CLASS_CHECK(SBSA_GWDTClass, (klass), TYPE_WDT_SBSA_GWDT) > +#define SBSA_GWDT_GET_CLASS(obj) \ > + OBJECT_GET_CLASS(SBSA_GWDTClass, (obj), TYPE_WDT_SBSA_GWDT) > + > +/* SBSA Generic Watchdog register definitions */ > +/* refresh frame */ > +#define SBSA_GWDT_WRR 0x000 > + > +/* control frame */ > +#define SBSA_GWDT_WCS 0x000 > +#define SBSA_GWDT_WOR 0x008 > +#define SBSA_GWDT_WORU 0x00C > +#define SBSA_GWDT_WCV 0x010 > +#define SBSA_GWDT_WCVU 0x014 > + > +/* Watchdog Interface Identification Register */ > +#define SBSA_GWDT_W_IIDR 0xFCC > + > +/* Watchdog Control and Status Register Bits */ > +#define SBSA_GWDT_WCS_EN BIT(0) > +#define SBSA_GWDT_WCS_WS0 BIT(1) > +#define SBSA_GWDT_WCS_WS1 BIT(2) > + > +#define SBSA_GWDT_WOR_MASK 0x0000FFFF > + > +/* Watchdog Interface Identification Register definition*/ Missing space before '*/' > +#define SBSA_GWDT_ID 0x1043B Bits [11:0] in this field are a JEP106 code for the company that implemented the device. Who are we claiming to be here ? > + > +/* 2 Separate memory regions for each of refresh & control register frames */ > +#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; > + > + /*< public >*/ > + MemoryRegion rmmio; > + MemoryRegion cmmio; > + qemu_irq irq; > + > + QEMUTimer *ptimer, *timer; > + > + uint32_t id; > + uint32_t wrr; > + uint32_t wcs; > + uint32_t worl; > + uint32_t woru; > + uint32_t wcvl; > + uint32_t wcvu; > + bool enabled; > + bool ws0, ws1; > + > + /*< public >*/ What's this extra 'public' comment for ? > +} SBSA_GWDTState; > + > +#endif /* WDT_SBSA_GWDT_H */ > -- > 2.18.4 > thanks -- PMM ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v6 1/2] hw/watchdog: Implement SBSA watchdog device 2020-10-20 14:28 ` Peter Maydell @ 2020-10-22 15:58 ` shashi.mallela 0 siblings, 0 replies; 6+ messages in thread From: shashi.mallela @ 2020-10-22 15:58 UTC (permalink / raw) To: peter.maydell; +Cc: qemu-devel On Thu, 15 Oct 2020 at 17:35, Shashi Mallela <shashi.mall...@linaro.org > wrote: > > Generic watchdog device model has been implemented as per ARM BSAv0.9 Hmm, so the patch title, filenames, etc, refer to the SBSA, which is the Server Base System Architecture spec. Is the BSA a different spec ? We should be consistent with our terminology and clear about what spec we're implementing. (I've used the SBSA v6.0 as reference in the review comments below.) Shashi,The implementation is based on SBSA v6.0, i will change the commit message accordingly > Signed-off-by: Shashi Mallela <shashi.mall...@linaro.org> > --- > hw/arm/Kconfig | 1 + > hw/watchdog/Kconfig | 4 + > hw/watchdog/meson.build | 1 + > hw/watchdog/wdt_sbsa_gwdt.c | 346 ++++++++++++++++++++++++++++ This filename is rather redundant as it says "watchdog" three times. hw/watchdog/sbsa_gwdt.c will do (similarly the .h file). Shashi,Done > include/hw/watchdog/wdt_sbsa_gwdt.h | 70 ++++++ > 5 files changed, 422 insertions(+) > create mode 100644 hw/watchdog/wdt_sbsa_gwdt.c > create mode 100644 include/hw/watchdog/wdt_sbsa_gwdt.h > > diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig > index f303c6bead25..6b97e64595d3 100644 > --- a/hw/arm/Kconfig > +++ b/hw/arm/Kconfig > @@ -210,6 +210,7 @@ config SBSA_REF > select PL031 # RTC > select PL061 # GPIO > select USB_EHCI_SYSBUS > + select WDT_SBSA_GWDT > > config SABRELITE > bool > diff --git a/hw/watchdog/Kconfig b/hw/watchdog/Kconfig > index 293209b291d6..ea9cadd66f22 100644 > --- a/hw/watchdog/Kconfig > +++ b/hw/watchdog/Kconfig > @@ -17,3 +17,7 @@ config WDT_DIAG288 > > config WDT_IMX2 > bool > + > +config WDT_SBSA_GWDT > + bool > + default y if SBSA_REF I don't think you need the "default" line here. The board config "select"s the device, so if the user enables the board they'll get the device, which is all you need. Shashi,Done > diff --git a/hw/watchdog/meson.build b/hw/watchdog/meson.build > index 9b8725e64288..a9a23307acfe 100644 > --- a/hw/watchdog/meson.build > +++ b/hw/watchdog/meson.build > @@ -5,3 +5,4 @@ softmmu_ss.add(when: 'CONFIG_WDT_IB700', if_true: > files('wdt_ib700.c')) > softmmu_ss.add(when: 'CONFIG_WDT_DIAG288', if_true: files('wdt_diag288.c')) > softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files('wdt_aspeed.c')) > softmmu_ss.add(when: 'CONFIG_WDT_IMX2', if_true: files('wdt_imx2.c')) > +softmmu_ss.add(when: 'CONFIG_WDT_SBSA_GWDT', if_true: > files('wdt_sbsa_gwdt.c')) > diff --git a/hw/watchdog/wdt_sbsa_gwdt.c b/hw/watchdog/wdt_sbsa_gwdt.c > new file mode 100644 > index 000000000000..1c2fe04db7c7 > --- /dev/null > +++ b/hw/watchdog/wdt_sbsa_gwdt.c > @@ -0,0 +1,346 @@ > +/* > + * Generic watchdog device model for SBSA > + * > + * Copyright Linaro.org 2020 > + * > + * Authors: > + * Shashi Mallela <shashi.mall...@linaro.org> > + * > + * This work is licensed under the terms of the GNU GPL, version 2 or (at > your > + * option) any later version. See the COPYING file in the top- level > directory. > + * > + */ This is a good place for a comment which gives the title and a URL for the specification the device is based on. It would also be helpful to say what revision of the watchdog we implement (the SBSA 6.0 spec says there is a "revision 0" and also a "revision 1"; looks like we're revision 1 ?) Shashi, Added a comment at the top of the file indicating the SBSA 6.0 spec and revision 1 > + > +#include "qemu/osdep.h" > +#include "sysemu/reset.h" > +#include "sysemu/watchdog.h" > +#include "hw/watchdog/wdt_sbsa_gwdt.h" > +#include "qemu/timer.h" > +#include "migration/vmstate.h" > +#include "qemu/log.h" > +#include "qemu/module.h" > + > +static WatchdogTimerModel model = { > + .wdt_name = TYPE_WDT_SBSA_GWDT, > + .wdt_description = "sbsa_gwdt device for sbsa_ref platform", Other wdt_description strings use the human-facing name of the device, so something like "Generic SBSA-compliant watchdog device" seems a better match to the others. (The board name is 'sbsa-ref' with a hyphen, by the way.) Shashi,Done > +}; > + > +static const VMStateDescription vmstate_sbsa_gwdt = { > + .name = "vmstate_sbsa_gwdt", You can drop the 'vmstate_' prefix. Just "sbsa-gwdt" will do. Shashi,Done > + .version_id = 1, > + .minimum_version_id = 1, > + .fields = (VMStateField[]) { > + VMSTATE_TIMER_PTR(timer, SBSA_GWDTState), > + VMSTATE_BOOL(enabled, SBSA_GWDTState), > + VMSTATE_BOOL(ws0, SBSA_GWDTState), > + VMSTATE_BOOL(ws1, SBSA_GWDTState), > + VMSTATE_UINT32(wrr, SBSA_GWDTState), > + VMSTATE_UINT32(wcs, SBSA_GWDTState), > + VMSTATE_UINT32(worl, SBSA_GWDTState), > + VMSTATE_UINT32(woru, SBSA_GWDTState), > + VMSTATE_UINT32(wcvl, SBSA_GWDTState), > + VMSTATE_UINT32(wcvu, SBSA_GWDTState), > + VMSTATE_END_OF_LIST() This is missing 'ptimer' (but see below for a review comment where I recommend dropping 'ptimer' entirely). Shashi, Removed ptimer usage as in the other comment > + } > +}; > + > +static uint64_t sbsa_gwdt_rread(void *opaque, hwaddr addr, unsigned int size) > +{ > + uint32_t ret = 0; > + > + if (addr == SBSA_GWDT_WRR) { > + /* watch refresh read has no effect and returns 0 */ > + ret = 0; > + } else { > + qemu_log_mask(LOG_GUEST_ERROR, "bad address in refresh frame read :" > + " 0x%x\n", (int)addr); > + } The spec says that reading from 0xfcc is valid in both the refresh and the control frame (returning the same ID value in both cases). Shashi, Added 0xfcc ID value read for refresh frame offset as well > + return ret; > +} > + > +static uint64_t sbsa_gwdt_read(void *opaque, hwaddr addr, unsigned int size) > +{ > + SBSA_GWDTState *s = SBSA_GWDT(opaque); > + uint32_t ret = 0; > + > + switch (addr) { > + case SBSA_GWDT_WCS: > + ret = s->wcs; > + break; > + case SBSA_GWDT_WOR: > + ret = s->worl; > + break; > + case SBSA_GWDT_WORU: > + ret = s->woru; > + break; > + case SBSA_GWDT_WCV: > + ret = s->wcvl; > + break; > + case SBSA_GWDT_WCVU: > + ret = s->wcvu; > + break; > + case SBSA_GWDT_W_IIDR: > + ret = s->id; > + break; > + default: > + qemu_log_mask(LOG_GUEST_ERROR, "bad address in control frame read :" > + " 0x%x\n", (int)addr); > + } > + return ret; > +} > + > +static void sbsa_gwdt_update_timer(SBSA_GWDTState *s) > +{ > + uint64_t timeout = 0; The spec talks about "explicit refresh" and "timeout refresh" events. Is this function implementing both of those ? If so, a function name indicating that would be helpful to the reader. If it's only implementing part of the refresh semantics then an explanatory comment would help. Shashi, Have updated this function signature to include an enum type (to indicate EXPLICIT_REFRESH & TIMEOUT_REFRESH ) as additional parameter (below) "static void sbsa_gwdt_update_timer(SBSA_GWDTState *s, WdtRefreshType rtype)" and the caller does "sbsa_gwdt_update_timer(s, TIMEOUT_REFRESH)". > + > + if (s->enabled) { > + /* > + * Extract the upper 16 bits from woru & 32 bits from worl > + * registers to construct the 48 bit offset value > + */ > + timeout = s->woru & SBSA_GWDT_WOR_MASK; > + timeout <<= 32; > + timeout |= s->worl; > + timeout = muldiv64(timeout, NANOSECONDS_PER_SECOND, SBSA_TIMER_FREQ); > + timeout += qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > + > + /* store the current timeout value into compare registers */ > + s->wcvu = timeout >> 32; > + s->wcvl = timeout; The spec says that for an SBSA level 2 or higher system, the compare value shouldn't be updated for a timeout-refresh when WS0 is asserted (and for SBSA level 0 or 1 the level 2 behaviour is permitted as an IMPDEF option). Shashi, The qemu sbsa-ref platform code is based for levels 2 or higher.Hence,have now added logic to not update compare value for timeout-refresh. > + > + if (!s->ws0) { > + timer_del(s->timer); > + timer_mod(s->ptimer, timeout); > + } else { > + timer_del(s->ptimer); > + timer_mod(s->timer, timeout); > + } > + } else { > + timer_del(s->ptimer); > + timer_del(s->timer); Here you have two timers, "timer" and "ptimer", which are set up so that you always have exactly one running. It would be simpler to have a single timer, and then decide what you're doing on expiry by looking at the device state in its callback function. Shashi, Done.Removed ptimer and adapted code for single timer usage > + } > +} > + > +static void sbsa_gwdt_rwrite(void *opaque, hwaddr offset, uint64_t data, > + unsigned size) { > + SBSA_GWDTState *s = SBSA_GWDT(opaque); > + > + if (offset == SBSA_GWDT_WRR) { > + s->wrr = data; There is no code path which allows the guest to read s->wrr and nothing in the device model does anything with it, so there is no need to have it be state at all. That is, a write to WRR is a trigger for behaviour, but no state is stored. Shashi, removed wrr from state > + s->wcs &= ~SBSA_GWDT_WCS_WS0; > + s->wcs &= ~SBSA_GWDT_WCS_WS1; > + s->ws0 = false; > + s->ws1 = false; > + sbsa_gwdt_update_timer(s); > + } else { > + qemu_log_mask(LOG_GUEST_ERROR, "bad address in refresh frame write :" > + " 0x%x\n", (int)offset); > + } > +} > + > +static void sbsa_gwdt_write(void *opaque, hwaddr offset, uint64_t data, > + unsigned size) { > + SBSA_GWDTState *s = SBSA_GWDT(opaque); > + bool enable; > + > + switch (offset) { > + case SBSA_GWDT_WCS: > + enable = data & SBSA_GWDT_WCS_EN; > + if (enable) { > + s->wcs |= SBSA_GWDT_WCS_EN; > + s->enabled = true; > + } else { > + s->wcs &= ~SBSA_GWDT_WCS_EN; > + s->enabled = false; > + } > + s->ws0 = false; > + s->ws1 = false; > + s->wcs &= ~SBSA_GWDT_WCS_WS0; > + s->wcs &= ~SBSA_GWDT_WCS_WS1; You are storing state duplicated in two places here. You can either: * store the state of bits like the WS0, WS1, Enable as bits inside an s->wcs register value -- in this case reads and writes are easy, and if you want to test for "is device enabled" you do it with a logical operation on s->wcs; there would be no separate ws0/ws1/enabled fields in the state struct * or you can store them as separate boolean fields, in which case you don't need an s->wcs at all. Instead in the read function you assemble the value the guest reads from the ws0/ws1/enable booleans Which one is preferable depends on the device and whether there's a lot of code that often tests the various bits. You can pick either option, but don't do both at once the way this code is. Similarly, you have a choice of whether you prefer to hold the WCV and WOR values in the state struct as a pair of uint32_t (trivial to read/write, need to reassemble into the full value whenever you want to operate on it) or as a single uint64_t (easier for the device to operate on, register reads/writes become extract64/deposit64 operations). Shashi, Done,removed bool flags like ws0,ws1,enabled from state and made use of s->wcs with relevant bit fields for all accesses > + sbsa_gwdt_update_timer(s); > + break; > + > + case SBSA_GWDT_WOR: > + s->worl = data; > + s->ws0 = false; > + s->ws1 = false; > + s->wcs &= ~SBSA_GWDT_WCS_WS0; > + s->wcs &= ~SBSA_GWDT_WCS_WS1; > + /* > + * TODO:- setting woru to 0 and triggering update timer(below) is a > + * temporary workaround to handle current linux driver which is > + * based on earlier version of BSA specification.Once the linux > + * driver is updated to BSA v0.9 will remove these next 2 lines. > + */ No workarounds for buggy Linux guest drivers, please. Once they get into the code base they are almost impossible to ever get out again. You can either: 1) just implement the newer version of the specification, and fix the guest drivers 2) if you must have a system that works with older guests, then implement *both* older and newer specs, using a QOM property on the device to select which one. You'd need to implement the details of the older version of the spec completely, not just "this is the one thing that the Linux driver happens to care about". You'd also need a machine property on the sbsa-ref board so the user could select whether they wanted the old or new watchdog device. I'd recommend option 1, really. Shashi, Done.implemented option 1 > + s->woru = 0; > + sbsa_gwdt_update_timer(s); > + break; > + > + case SBSA_GWDT_WORU: > + s->woru = data; This and WCVU have the top 16 bits RAZ/WI, so you should mask out the top half of 'data' so the guest can't write an over-large value. Shashi, The top 16 bits only applies to WORU(of the total 48 bits for WOR).Will mask the top half of data as you suggested. But,WCV is 64 bits overall (with upper 32 bits in WCVU & lower 32 bits in WCV).Hence there is no need to mask 16 bits for WCVU since it is not applicable. > + s->ws0 = false; > + s->ws1 = false; > + s->wcs &= ~SBSA_GWDT_WCS_WS0; > + s->wcs &= ~SBSA_GWDT_WCS_WS1; > + sbsa_gwdt_update_timer(s); > + break; > + > + case SBSA_GWDT_WCV: > + s->wcvl = data; > + break; > + > + case SBSA_GWDT_WCVU: > + s->wcvu = data; > + break; > + > + default: > + qemu_log_mask(LOG_GUEST_ERROR, "bad address in control frame write :" > + " 0x%x\n", (int)offset); > + } > + return; > +} > + > +static void wdt_sbsa_gwdt_reset(DeviceState *dev) > +{ > + SBSA_GWDTState *s = SBSA_GWDT(dev); > + > + timer_del(s->ptimer); > + timer_del(s->timer); > + > + s->enabled = false; > + s->ws0 = false; > + s->ws1 = false; > + s->wcs &= ~SBSA_GWDT_WCS_EN; > + s->wcs &= ~SBSA_GWDT_WCS_WS0; > + s->wcs &= ~SBSA_GWDT_WCS_WS1; > + s->wcvl = 0; > + s->wcvu = 0; > + s->worl = 0; > + s->woru = 0; > + s->id = SBSA_GWDT_ID; > +} > + > +static void sbsa_gwdt_reset(void *opaque) > +{ > + DeviceState *sbsa_gwdt = opaque; > + > + wdt_sbsa_gwdt_reset(sbsa_gwdt); > +} Why do you have two reset functions which do the same thing ? Shashi, Done.Removed 1 of the reset function > + > +static void sbsa_gwdt_timer_sysinterrupt(void *opaque) > +{ > + SBSA_GWDTState *s = SBSA_GWDT(opaque); > + > + s->wcs |= SBSA_GWDT_WCS_WS0; > + s->ws0 = true; > + qemu_set_irq(s->irq, 1); > + sbsa_gwdt_update_timer(s); > +} > + > +static void sbsa_gwdt_timer_sysreset(void *dev) > +{ > + SBSA_GWDTState *s = SBSA_GWDT(dev); > + > + s->wcs |= SBSA_GWDT_WCS_WS1; > + s->ws1 = true; > + qemu_log_mask(CPU_LOG_RESET, "Watchdog timer expired.\n"); > + /* > + * Reset the watchdog only if the guest gets notified about > + * expiry. watchdog_perform_action() may temporarily relinquish > + * the BQL; reset before triggering the action to avoid races with > + * sbsa_gwdt instructions. > + */ > + switch (get_watchdog_action()) { > + case WATCHDOG_ACTION_DEBUG: > + case WATCHDOG_ACTION_NONE: > + case WATCHDOG_ACTION_PAUSE: > + break; > + default: > + wdt_sbsa_gwdt_reset(dev); > + } > + watchdog_perform_action(); > +} > + > +static const MemoryRegionOps sbsa_gwdt_rops = { > + .read = sbsa_gwdt_rread, > + .write = sbsa_gwdt_rwrite, > + .endianness = DEVICE_LITTLE_ENDIAN, > + .valid.min_access_size = 4, > + .valid.max_access_size = 4, > + .valid.unaligned = false, > +}; > + > +static const MemoryRegionOps sbsa_gwdt_ops = { > + .read = sbsa_gwdt_read, > + .write = sbsa_gwdt_write, > + .endianness = DEVICE_LITTLE_ENDIAN, > + .valid.min_access_size = 4, > + .valid.max_access_size = 4, > + .valid.unaligned = false, > +}; > + > +static void wdt_sbsa_gwdt_realize(DeviceState *dev, Error **errp) > +{ > + SBSA_GWDTState *s = SBSA_GWDT(dev); > + SysBusDevice *sbd = SYS_BUS_DEVICE(dev); > + > + memory_region_init_io(&s->rmmio, OBJECT(dev), > + &sbsa_gwdt_rops, s, > + "sbsa_gwdt.refresh", > + SBSA_GWDT_RMMIO_SIZE); > + > + memory_region_init_io(&s->cmmio, OBJECT(dev), > + &sbsa_gwdt_ops, s, > + "sbsa_gwdt.control", > + SBSA_GWDT_CMMIO_SIZE); > + > + sysbus_init_mmio(sbd, &s->rmmio); > + sysbus_init_mmio(sbd, &s->cmmio); > + > + sysbus_init_irq(sbd, &s->irq); > + > + qemu_register_reset(sbsa_gwdt_reset, s); No new code should call qemu_register_reset(). You can provide a reset function via dc->reset in your DeviceClass, and in fact you have. Shashi, Done.Removed qemu_register_reset() call > + > + s->ptimer = timer_new_ns(QEMU_CLOCK_VIRTUAL, > sbsa_gwdt_timer_sysinterrupt, > + dev); > + s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sbsa_gwdt_timer_sysreset, > + dev); > +} > + > +static void wdt_sbsa_gwdt_unrealize(DeviceState *dev) > +{ > + SBSA_GWDTState *s = SBSA_GWDT(dev); > + > + timer_del(s->ptimer); > + timer_free(s->ptimer); > + > + timer_del(s->timer); > + timer_free(s->timer); > +} This device isn't hot-pluggable, so this unrealize method is unreachable code, I think. Shashi, Done.Removed unrealize > + > +static void wdt_sbsa_gwdt_class_init(ObjectClass *klass, void *data) > +{ > + DeviceClass *dc = DEVICE_CLASS(klass); > + > + dc->realize = wdt_sbsa_gwdt_realize; > + dc->unrealize = wdt_sbsa_gwdt_unrealize; > + dc->reset = wdt_sbsa_gwdt_reset; > + dc->hotpluggable = false; > + set_bit(DEVICE_CATEGORY_MISC, dc->categories); > + dc->vmsd = &vmstate_sbsa_gwdt; > +} > + > +static const TypeInfo wdt_sbsa_gwdt_info = { > + .class_init = wdt_sbsa_gwdt_class_init, > + .parent = TYPE_SYS_BUS_DEVICE, > + .name = TYPE_WDT_SBSA_GWDT, > + .instance_size = sizeof(SBSA_GWDTState), > +}; > + > +static void wdt_sbsa_gwdt_register_types(void) > +{ > + watchdog_add_model(&model); > + type_register_static(&wdt_sbsa_gwdt_info); > +} > + > +type_init(wdt_sbsa_gwdt_register_types) > diff --git a/include/hw/watchdog/wdt_sbsa_gwdt.h > b/include/hw/watchdog/wdt_sbsa_gwdt.h > new file mode 100644 > index 000000000000..70ba7abb3ace > --- /dev/null > +++ b/include/hw/watchdog/wdt_sbsa_gwdt.h > @@ -0,0 +1,70 @@ New files, including .h files, should start with a copyright-and-license comment. Shashi, Done > +#ifndef WDT_SBSA_GWDT_H > +#define WDT_SBSA_GWDT_H > + > +#include "qemu/bitops.h" > +#include "hw/sysbus.h" > +#include "hw/irq.h" > + > +#define TYPE_WDT_SBSA_GWDT "sbsa_gwdt" > +#define SBSA_GWDT(obj) \ > + OBJECT_CHECK(SBSA_GWDTState, (obj), TYPE_WDT_SBSA_GWDT) > +#define SBSA_GWDT_CLASS(klass) \ > + OBJECT_CLASS_CHECK(SBSA_GWDTClass, (klass), TYPE_WDT_SBSA_GWDT) > +#define SBSA_GWDT_GET_CLASS(obj) \ > + OBJECT_GET_CLASS(SBSA_GWDTClass, (obj), TYPE_WDT_SBSA_GWDT) > + > +/* SBSA Generic Watchdog register definitions */ > +/* refresh frame */ > +#define SBSA_GWDT_WRR 0x000 > + > +/* control frame */ > +#define SBSA_GWDT_WCS 0x000 > +#define SBSA_GWDT_WOR 0x008 > +#define SBSA_GWDT_WORU 0x00C > +#define SBSA_GWDT_WCV 0x010 > +#define SBSA_GWDT_WCVU 0x014 > + > +/* Watchdog Interface Identification Register */ > +#define SBSA_GWDT_W_IIDR 0xFCC > + > +/* Watchdog Control and Status Register Bits */ > +#define SBSA_GWDT_WCS_EN BIT(0) > +#define SBSA_GWDT_WCS_WS0 BIT(1) > +#define SBSA_GWDT_WCS_WS1 BIT(2) > + > +#define SBSA_GWDT_WOR_MASK 0x0000FFFF > + > +/* Watchdog Interface Identification Register definition*/ Missing space before '*/' Shashi, Done > +#define SBSA_GWDT_ID 0x1043B Bits [11:0] in this field are a JEP106 code for the company that implemented the device. Who are we claiming to be here ? Shashi,have added JEP106 code considering "ARM" as the company(since this is an ARM SBSA and Linaro doesnt have its own code) > + > +/* 2 Separate memory regions for each of refresh & control register frames */ > +#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; > + > + /*< public >*/ > + MemoryRegion rmmio; > + MemoryRegion cmmio; > + qemu_irq irq; > + > + QEMUTimer *ptimer, *timer; > + > + uint32_t id; > + uint32_t wrr; > + uint32_t wcs; > + uint32_t worl; > + uint32_t woru; > + uint32_t wcvl; > + uint32_t wcvu; > + bool enabled; > + bool ws0, ws1; > + > + /*< public >*/ What's this extra 'public' comment for ? Shashi, Done.Removed > +} SBSA_GWDTState; > + > +#endif /* WDT_SBSA_GWDT_H */ > -- > 2.18.4 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v6 2/2] hw/arm/sbsa-ref: add SBSA watchdog device 2020-10-15 16:34 [PATCH v6 0/2] Add watchdog support for SbsaQemu Shashi Mallela 2020-10-15 16:34 ` [PATCH v6 1/2] hw/watchdog: Implement SBSA watchdog device Shashi Mallela @ 2020-10-15 16:34 ` Shashi Mallela 2020-10-20 14:29 ` Peter Maydell 1 sibling, 1 reply; 6+ messages in thread From: Shashi Mallela @ 2020-10-15 16:34 UTC (permalink / raw) To: peter.maydell, leif, rad; +Cc: qemu-arm, qemu-devel Included the newly implemented SBSA generic watchdog device model into SBSA platform Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org> --- hw/arm/sbsa-ref.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c index 01863510d0f5..671859ec9711 100644 --- a/hw/arm/sbsa-ref.c +++ b/hw/arm/sbsa-ref.c @@ -30,6 +30,7 @@ #include "exec/hwaddr.h" #include "kvm_arm.h" #include "hw/arm/boot.h" +#include "hw/arm/fdt.h" #include "hw/block/flash.h" #include "hw/boards.h" #include "hw/ide/internal.h" @@ -40,6 +41,7 @@ #include "hw/qdev-properties.h" #include "hw/usb.h" #include "hw/char/pl011.h" +#include "hw/watchdog/wdt_sbsa_gwdt.h" #include "net/net.h" #include "qom/object.h" @@ -64,6 +66,9 @@ enum { SBSA_GIC_DIST, SBSA_GIC_REDIST, SBSA_SECURE_EC, + SBSA_GWDT, + SBSA_GWDT_REFRESH, + SBSA_GWDT_CONTROL, SBSA_SMMU, SBSA_UART, SBSA_RTC, @@ -104,6 +109,8 @@ static const MemMapEntry sbsa_ref_memmap[] = { [SBSA_GIC_DIST] = { 0x40060000, 0x00010000 }, [SBSA_GIC_REDIST] = { 0x40080000, 0x04000000 }, [SBSA_SECURE_EC] = { 0x50000000, 0x00001000 }, + [SBSA_GWDT_REFRESH] = { 0x50010000, 0x00001000 }, + [SBSA_GWDT_CONTROL] = { 0x50011000, 0x00001000 }, [SBSA_UART] = { 0x60000000, 0x00001000 }, [SBSA_RTC] = { 0x60010000, 0x00001000 }, [SBSA_GPIO] = { 0x60020000, 0x00001000 }, @@ -134,6 +141,7 @@ static const int sbsa_ref_irqmap[] = { [SBSA_AHCI] = 10, [SBSA_EHCI] = 11, [SBSA_SMMU] = 12, /* ... to 15 */ + [SBSA_GWDT] = 16, }; static uint64_t sbsa_ref_cpu_mp_affinity(SBSAMachineState *sms, int idx) @@ -448,6 +456,20 @@ static void create_rtc(const SBSAMachineState *sms) sysbus_create_simple("pl031", base, qdev_get_gpio_in(sms->gic, irq)); } +static void create_wdt(const SBSAMachineState *sms) +{ + hwaddr rbase = sbsa_ref_memmap[SBSA_GWDT_REFRESH].base; + hwaddr cbase = sbsa_ref_memmap[SBSA_GWDT_CONTROL].base; + DeviceState *dev = qdev_new(TYPE_WDT_SBSA_GWDT); + SysBusDevice *s = SYS_BUS_DEVICE(dev); + int irq = sbsa_ref_irqmap[SBSA_GWDT]; + + sysbus_realize_and_unref(s, &error_fatal); + sysbus_mmio_map(s, 0, rbase); + sysbus_mmio_map(s, 1, cbase); + sysbus_connect_irq(s, 0, qdev_get_gpio_in(sms->gic, irq)); +} + static DeviceState *gpio_key_dev; static void sbsa_ref_powerdown_req(Notifier *n, void *opaque) { @@ -731,6 +753,8 @@ static void sbsa_ref_init(MachineState *machine) create_rtc(sms); + create_wdt(sms); + create_gpio(sms); create_ahci(sms); -- 2.18.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v6 2/2] hw/arm/sbsa-ref: add SBSA watchdog device 2020-10-15 16:34 ` [PATCH v6 2/2] hw/arm/sbsa-ref: add " Shashi Mallela @ 2020-10-20 14:29 ` Peter Maydell 0 siblings, 0 replies; 6+ messages in thread From: Peter Maydell @ 2020-10-20 14:29 UTC (permalink / raw) To: Shashi Mallela Cc: Leif Lindholm, QEMU Developers, qemu-arm, Radoslaw Biernacki On Thu, 15 Oct 2020 at 17:35, Shashi Mallela <shashi.mallela@linaro.org> wrote: > > Included the newly implemented SBSA generic watchdog device model into > SBSA platform > > Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org> > --- > hw/arm/sbsa-ref.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c > index 01863510d0f5..671859ec9711 100644 > --- a/hw/arm/sbsa-ref.c > +++ b/hw/arm/sbsa-ref.c > @@ -30,6 +30,7 @@ > #include "exec/hwaddr.h" > #include "kvm_arm.h" > #include "hw/arm/boot.h" > +#include "hw/arm/fdt.h" > #include "hw/block/flash.h" > #include "hw/boards.h" > #include "hw/ide/internal.h" This new #include is a leftover from a previous version of the patchset and I think not needed ? Otherwise Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-10-22 15:59 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-10-15 16:34 [PATCH v6 0/2] Add watchdog support for SbsaQemu Shashi Mallela 2020-10-15 16:34 ` [PATCH v6 1/2] hw/watchdog: Implement SBSA watchdog device Shashi Mallela 2020-10-20 14:28 ` Peter Maydell 2020-10-22 15:58 ` shashi.mallela 2020-10-15 16:34 ` [PATCH v6 2/2] hw/arm/sbsa-ref: add " Shashi Mallela 2020-10-20 14:29 ` Peter Maydell
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.