Re: [Qemu-arm] [Qemu-devel] [PATCH v2] aspeed: Link SCU to the watchdog

[Philippe Mathieu-Daudé]

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 <address@hidden>
> 
> 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 <address@hidden>
> 
> Thanks,
> 
> C.
> 
>> Signed-off-by: Cédric Le Goater <address@hidden>
>> ---
>> 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é <address@hidden>

>> +        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;
>>
> 
>