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

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

[Joel Stanley]

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 <joel@jms.id.au>
Signed-off-by: Cédric Le Goater <clg@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) {
+        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;
-- 
2.20.1

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

[Cédric Le Goater]

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 <joel@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 <clg@kaod.org>

Thanks,

C.

> Signed-off-by: Cédric Le Goater <clg@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) {
> +        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;
> 

Re: [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 <joel@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 <clg@kaod.org>
> 
> Thanks,
> 
> C.
> 
>> Signed-off-by: Cédric Le Goater <clg@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é <philmd@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;
>>
> 
> 

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

[Joel Stanley]

On Fri, 21 Jun 2019 at 09:06, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> 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 <joel@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 <clg@kaod.org>
> >
> > Thanks,
> >
> > C.
> >
> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>

> >> --- 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()'.

I will take this suggestion on board in the future when I model the
watchdog reset behavior in more detail.

>
> Anyway the patch looks sane:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Thanks.

Joel

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

[Peter Maydell]

On Fri, 21 Jun 2019 at 07:52, Joel Stanley <joel@jms.id.au> 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 <joel@jms.id.au>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> v2: rebase on upstream, rework commit message



Applied to target-arm.next, thanks.

-- PMM