On 6/21/19 10:25 AM, Cédric Le Goater wrote: > On 21/06/2019 08:52, Joel Stanley wrote: >> The ast2500 uses the watchdog to reset the SDRAM controller. This >> operation is usually performed by u-boot's memory training procedure, >> and it is enabled by setting a bit in the SCU and then causing the >> watchdog to expire. Therefore, we need the watchdog to be able to >> access the SCU's register space. >> >> This causes the watchdog to not perform a system reset when the bit is >> set. In the future it could perform a reset of the SDMC model. >> >> Signed-off-by: Joel Stanley <j...@jms.id.au> > > I was keeping this patch in my tree (hence the Sob) hoping that > someone could find the time to study the reset question. But this > patch is useful as it is and I think we should merge it. > > Reviewed-by: Cédric Le Goater <c...@kaod.org> > > Thanks, > > C. > >> Signed-off-by: Cédric Le Goater <c...@kaod.org> >> --- >> v2: rebase on upstream, rework commit message >> --- >> hw/arm/aspeed_soc.c | 2 ++ >> hw/watchdog/wdt_aspeed.c | 20 ++++++++++++++++++++ >> include/hw/watchdog/wdt_aspeed.h | 1 + >> 3 files changed, 23 insertions(+) >> >> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c >> index a2ea8c748449..ddd5dfacd7d6 100644 >> --- a/hw/arm/aspeed_soc.c >> +++ b/hw/arm/aspeed_soc.c >> @@ -155,6 +155,8 @@ static void aspeed_soc_init(Object *obj) >> sizeof(s->wdt[i]), TYPE_ASPEED_WDT); >> qdev_prop_set_uint32(DEVICE(&s->wdt[i]), "silicon-rev", >> sc->info->silicon_rev); >> + object_property_add_const_link(OBJECT(&s->wdt[i]), "scu", >> + OBJECT(&s->scu), &error_abort); >> } >> >> sysbus_init_child_obj(obj, "ftgmac100", OBJECT(&s->ftgmac100), >> diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c >> index 4a8409f0daf5..57fe24ae6b1f 100644 >> --- a/hw/watchdog/wdt_aspeed.c >> +++ b/hw/watchdog/wdt_aspeed.c >> @@ -44,6 +44,9 @@ >> >> #define WDT_RESTART_MAGIC 0x4755 >> >> +#define SCU_RESET_CONTROL1 (0x04 / 4) >> +#define SCU_RESET_SDRAM BIT(0) >> + >> static bool aspeed_wdt_is_enabled(const AspeedWDTState *s) >> { >> return s->regs[WDT_CTRL] & WDT_CTRL_ENABLE; >> @@ -222,6 +225,13 @@ static void aspeed_wdt_timer_expired(void *dev) >> { >> AspeedWDTState *s = ASPEED_WDT(dev); >> >> + /* Do not reset on SDRAM controller reset */ >> + if (s->scu->regs[SCU_RESET_CONTROL1] & SCU_RESET_SDRAM) {
This would be cleaner as an static inlined function in "hw/misc/aspeed_scu.h" IMO, maybe 'bool scu_sdram_is_reset()'. Anyway the patch looks sane: Reviewed-by: Philippe Mathieu-Daudé <phi...@redhat.com> >> + timer_del(s->timer); >> + s->regs[WDT_CTRL] = 0; >> + return; >> + } >> + >> qemu_log_mask(CPU_LOG_RESET, "Watchdog timer expired.\n"); >> watchdog_perform_action(); >> timer_del(s->timer); >> @@ -233,6 +243,16 @@ static void aspeed_wdt_realize(DeviceState *dev, Error >> **errp) >> { >> SysBusDevice *sbd = SYS_BUS_DEVICE(dev); >> AspeedWDTState *s = ASPEED_WDT(dev); >> + Error *err = NULL; >> + Object *obj; >> + >> + obj = object_property_get_link(OBJECT(dev), "scu", &err); >> + if (!obj) { >> + error_propagate(errp, err); >> + error_prepend(errp, "required link 'scu' not found: "); >> + return; >> + } >> + s->scu = ASPEED_SCU(obj); >> >> if (!is_supported_silicon_rev(s->silicon_rev)) { >> error_setg(errp, "Unknown silicon revision: 0x%" PRIx32, >> diff --git a/include/hw/watchdog/wdt_aspeed.h >> b/include/hw/watchdog/wdt_aspeed.h >> index 88d8be4f78d6..daef0c0e230b 100644 >> --- a/include/hw/watchdog/wdt_aspeed.h >> +++ b/include/hw/watchdog/wdt_aspeed.h >> @@ -27,6 +27,7 @@ typedef struct AspeedWDTState { >> MemoryRegion iomem; >> uint32_t regs[ASPEED_WDT_REGS_MAX]; >> >> + AspeedSCUState *scu; >> uint32_t pclk_freq; >> uint32_t silicon_rev; >> uint32_t ext_pulse_width_mask; >> > >