* [Qemu-devel] [PATCH v2] aspeed: Link SCU to the watchdog
@ 2019-06-21 6:52 Joel Stanley
2019-06-21 8:25 ` Cédric Le Goater
2019-07-01 13:36 ` Peter Maydell
0 siblings, 2 replies; 5+ messages in thread
From: Joel Stanley @ 2019-06-21 6:52 UTC (permalink / raw)
To: Cédric Le Goater, Peter Maydell; +Cc: Andrew Jeffery, qemu-arm, qemu-devel
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
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [Qemu-devel] [PATCH v2] aspeed: Link SCU to the watchdog
2019-06-21 6:52 [Qemu-devel] [PATCH v2] aspeed: Link SCU to the watchdog Joel Stanley
@ 2019-06-21 8:25 ` Cédric Le Goater
2019-06-21 9:06 ` Philippe Mathieu-Daudé
2019-07-01 13:36 ` Peter Maydell
1 sibling, 1 reply; 5+ messages in thread
From: Cédric Le Goater @ 2019-06-21 8:25 UTC (permalink / raw)
To: Joel Stanley, Peter Maydell; +Cc: Andrew Jeffery, qemu-arm, qemu-devel
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;
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Qemu-devel] [PATCH v2] aspeed: Link SCU to the watchdog
2019-06-21 8:25 ` Cédric Le Goater
@ 2019-06-21 9:06 ` Philippe Mathieu-Daudé
2019-06-25 6:47 ` Joel Stanley
0 siblings, 1 reply; 5+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-06-21 9:06 UTC (permalink / raw)
To: Cédric Le Goater, Joel Stanley, Peter Maydell
Cc: Andrew Jeffery, qemu-arm, qemu-devel
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;
>>
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Qemu-devel] [PATCH v2] aspeed: Link SCU to the watchdog
2019-06-21 9:06 ` Philippe Mathieu-Daudé
@ 2019-06-25 6:47 ` Joel Stanley
0 siblings, 0 replies; 5+ messages in thread
From: Joel Stanley @ 2019-06-25 6:47 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Andrew Jeffery, Peter Maydell, qemu-arm, Cédric Le Goater,
QEMU Developers
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v2] aspeed: Link SCU to the watchdog
2019-06-21 6:52 [Qemu-devel] [PATCH v2] aspeed: Link SCU to the watchdog Joel Stanley
2019-06-21 8:25 ` Cédric Le Goater
@ 2019-07-01 13:36 ` Peter Maydell
1 sibling, 0 replies; 5+ messages in thread
From: Peter Maydell @ 2019-07-01 13:36 UTC (permalink / raw)
To: Joel Stanley
Cc: Andrew Jeffery, qemu-arm, Cédric Le Goater, QEMU Developers
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-07-01 14:08 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-21 6:52 [Qemu-devel] [PATCH v2] aspeed: Link SCU to the watchdog Joel Stanley
2019-06-21 8:25 ` Cédric Le Goater
2019-06-21 9:06 ` Philippe Mathieu-Daudé
2019-06-25 6:47 ` Joel Stanley
2019-07-01 13:36 ` Peter Maydell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).