diff mbox series

[v2,3/3] hw/arm: Add watchdog timer to Allwinner H40 and Bananapi board

Message ID 20240115182757.1095012-4-linux@roeck-us.net
State New
Headers show
Series hw/arm: Add support for USB, SATA, and watchdog to Allwinner R40 | expand

Commit Message

Guenter Roeck Jan. 15, 2024, 6:27 p.m. UTC
Add watchdog timer support to Allwinner-H40 and Bananapi.
The watchdog timer is added as an overlay to the Timer
module memory map.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 docs/system/arm/bananapi_m2u.rst | 2 +-
 hw/arm/Kconfig                   | 1 +
 hw/arm/allwinner-r40.c           | 8 ++++++++
 include/hw/arm/allwinner-r40.h   | 3 +++
 4 files changed, 13 insertions(+), 1 deletion(-)

Comments

Philippe Mathieu-Daudé Jan. 16, 2024, 10:04 a.m. UTC | #1
Hi,

(Cc'ing Li, Strahinja and Niek)

On 15/1/24 19:27, Guenter Roeck wrote:
> Add watchdog timer support to Allwinner-H40 and Bananapi.
> The watchdog timer is added as an overlay to the Timer
> module memory map.

I'm confused by these registers from TYPE_AW_A10_PIT
and the TYPE_AW_WDT implementation you are using:

   #define AW_A10_PIT_WDOG_CONTROL    0x90
   #define AW_A10_PIT_WDOG_MODE       0x94

Do we have 2 implementations of the same peripheral?

Should we instanciate AW_WDT within AW_A10_PIT?

> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>   docs/system/arm/bananapi_m2u.rst | 2 +-
>   hw/arm/Kconfig                   | 1 +
>   hw/arm/allwinner-r40.c           | 8 ++++++++
>   include/hw/arm/allwinner-r40.h   | 3 +++
>   4 files changed, 13 insertions(+), 1 deletion(-)


> diff --git a/hw/arm/allwinner-r40.c b/hw/arm/allwinner-r40.c
> index 534be4a735..a28e5b3886 100644
> --- a/hw/arm/allwinner-r40.c
> +++ b/hw/arm/allwinner-r40.c
> @@ -53,6 +53,7 @@ const hwaddr allwinner_r40_memmap[] = {
>       [AW_R40_DEV_OHCI2]      = 0x01c1c400,
>       [AW_R40_DEV_CCU]        = 0x01c20000,
>       [AW_R40_DEV_PIT]        = 0x01c20c00,
> +    [AW_R40_DEV_WDT]        = 0x01c20c90,
>       [AW_R40_DEV_UART0]      = 0x01c28000,
>       [AW_R40_DEV_UART1]      = 0x01c28400,
>       [AW_R40_DEV_UART2]      = 0x01c28800,
> @@ -279,6 +280,8 @@ static void allwinner_r40_init(Object *obj)
>       object_property_add_alias(obj, "clk1-freq", OBJECT(&s->timer),
>                                 "clk1-freq");
>   
> +    object_initialize_child(obj, "wdt", &s->wdt, TYPE_AW_WDT_SUN4I);
> +
>       object_initialize_child(obj, "ccu", &s->ccu, TYPE_AW_R40_CCU);
>   
>       for (int i = 0; i < AW_R40_NUM_MMCS; i++) {
> @@ -545,6 +548,11 @@ static void allwinner_r40_realize(DeviceState *dev, Error **errp)
>       sysbus_connect_irq(SYS_BUS_DEVICE(&s->emac), 0,
>                          qdev_get_gpio_in(DEVICE(&s->gic), AW_R40_GIC_SPI_EMAC));
>   
> +    /* WDT */
> +    sysbus_realize(SYS_BUS_DEVICE(&s->wdt), &error_fatal);
> +    sysbus_mmio_map_overlap(SYS_BUS_DEVICE(&s->wdt), 0,
> +                            allwinner_r40_memmap[AW_R40_DEV_WDT], 1);
> +
Guenter Roeck Jan. 16, 2024, 3:32 p.m. UTC | #2
On 1/16/24 02:04, Philippe Mathieu-Daudé wrote:
> Hi,
> 
> (Cc'ing Li, Strahinja and Niek)
> 
> On 15/1/24 19:27, Guenter Roeck wrote:
>> Add watchdog timer support to Allwinner-H40 and Bananapi.
>> The watchdog timer is added as an overlay to the Timer
>> module memory map.
> 
> I'm confused by these registers from TYPE_AW_A10_PIT
> and the TYPE_AW_WDT implementation you are using:
> 
>    #define AW_A10_PIT_WDOG_CONTROL    0x90
>    #define AW_A10_PIT_WDOG_MODE       0x94
> 
> Do we have 2 implementations of the same peripheral?
> 

The watchdog core in A10 and H40 is the same. Linux devicetree uses
allwinner,sun4i-a10-wdt for several chips.

arch/arm/boot/dts/allwinner/sun4i-a10.dtsi:                     compatible = "allwinner,sun4i-a10-wdt";
arch/arm/boot/dts/allwinner/sun5i.dtsi:                 compatible = "allwinner,sun4i-a10-wdt";
arch/arm/boot/dts/allwinner/sun7i-a20.dtsi:                     compatible = "allwinner,sun4i-a10-wdt";
arch/arm/boot/dts/allwinner/sun8i-r40.dtsi:                     compatible = "allwinner,sun4i-a10-wdt";

> Should we instanciate AW_WDT within AW_A10_PIT?
> 


As far as I can see, AW_A10_PIT_WDOG_CONTROL and AW_A10_PIT_WDOG_MODE
are not really handled in hw/timer/allwinner-a10-pit.c because of the
memory range overlay,  and the defines might as well be dropped from there.
It made some sense to have them when the watchdog wasn't implemented
to avoid emulation errors, but afaics they are now obsolete.

The wdt type (TYPE_AW_WDT_SUN4I) more accurately reflects the watchdog
type. Control and mode registers are model specific. For example,
sun6i and sun9i use the same timer and watchdog cores but with
different register offsets and bit values for the watchdog registers.
sun8i uses the same watchdog core (and same register offsets) as
sun6i/sun9i, but the timer module itself is different. Implementing that
with the current model (using memory map overlays) is straightforward.
I don't know how one would do that from the timer modules; it seems
more complex to me than the current implementation.

On the other side, I don't know how the "proper" implementation in
qemu would look like, so I don't really have a strong opinion either
way.

Guenter
Strahinja Jankovic Jan. 17, 2024, 10:43 p.m. UTC | #3
Hi Philippe,

On Tue, Jan 16, 2024 at 11:04 AM Philippe Mathieu-Daudé <philmd@linaro.org>
wrote:

> Hi,
>
> (Cc'ing Li, Strahinja and Niek)
>
> On 15/1/24 19:27, Guenter Roeck wrote:
> > Add watchdog timer support to Allwinner-H40 and Bananapi.
> > The watchdog timer is added as an overlay to the Timer
> > module memory map.
>
> I'm confused by these registers from TYPE_AW_A10_PIT
> and the TYPE_AW_WDT implementation you are using:
>
>    #define AW_A10_PIT_WDOG_CONTROL    0x90
>    #define AW_A10_PIT_WDOG_MODE       0x94
>
> Do we have 2 implementations of the same peripheral?
>

The inspiration for the AW_WDT implementation, with overlay of WDOG_CONTROL
and WDOG_MODE registers in the PIT memory map, was taken from the AW_RTC
implementation.
That was the easiest way to implement watchdog functionality, and also
keeps logical functionality in the appropriate place (hw/watchdog) instead
of bundling it with the timer.
As Guenter commented, the AW_A10_PIT does not do anything with those two
registers and with overlay it does not matter anymore.


>
> Should we instanciate AW_WDT within AW_A10_PIT?
>

Unfortunately, I don't know what that would look like and what benefits we
would have from it. Can you point me to an example that already exists?


> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>

Guenter, thank you for submitting this change. The commit looks fine to me,
so

Reviewed-by: Strahinja Jankovic <strahinja.p.jankovic@gmail.com>

Best regards,
Strahinja


> ---
> >   docs/system/arm/bananapi_m2u.rst | 2 +-
> >   hw/arm/Kconfig                   | 1 +
> >   hw/arm/allwinner-r40.c           | 8 ++++++++
> >   include/hw/arm/allwinner-r40.h   | 3 +++
> >   4 files changed, 13 insertions(+), 1 deletion(-)
>
>
> > diff --git a/hw/arm/allwinner-r40.c b/hw/arm/allwinner-r40.c
> > index 534be4a735..a28e5b3886 100644
> > --- a/hw/arm/allwinner-r40.c
> > +++ b/hw/arm/allwinner-r40.c
> > @@ -53,6 +53,7 @@ const hwaddr allwinner_r40_memmap[] = {
> >       [AW_R40_DEV_OHCI2]      = 0x01c1c400,
> >       [AW_R40_DEV_CCU]        = 0x01c20000,
> >       [AW_R40_DEV_PIT]        = 0x01c20c00,
> > +    [AW_R40_DEV_WDT]        = 0x01c20c90,
> >       [AW_R40_DEV_UART0]      = 0x01c28000,
> >       [AW_R40_DEV_UART1]      = 0x01c28400,
> >       [AW_R40_DEV_UART2]      = 0x01c28800,
> > @@ -279,6 +280,8 @@ static void allwinner_r40_init(Object *obj)
> >       object_property_add_alias(obj, "clk1-freq", OBJECT(&s->timer),
> >                                 "clk1-freq");
> >
> > +    object_initialize_child(obj, "wdt", &s->wdt, TYPE_AW_WDT_SUN4I);
> > +
> >       object_initialize_child(obj, "ccu", &s->ccu, TYPE_AW_R40_CCU);
> >
> >       for (int i = 0; i < AW_R40_NUM_MMCS; i++) {
> > @@ -545,6 +548,11 @@ static void allwinner_r40_realize(DeviceState *dev,
> Error **errp)
> >       sysbus_connect_irq(SYS_BUS_DEVICE(&s->emac), 0,
> >                          qdev_get_gpio_in(DEVICE(&s->gic),
> AW_R40_GIC_SPI_EMAC));
> >
> > +    /* WDT */
> > +    sysbus_realize(SYS_BUS_DEVICE(&s->wdt), &error_fatal);
> > +    sysbus_mmio_map_overlap(SYS_BUS_DEVICE(&s->wdt), 0,
> > +                            allwinner_r40_memmap[AW_R40_DEV_WDT], 1);
> > +
>
diff mbox series

Patch

diff --git a/docs/system/arm/bananapi_m2u.rst b/docs/system/arm/bananapi_m2u.rst
index 542310591d..587b488655 100644
--- a/docs/system/arm/bananapi_m2u.rst
+++ b/docs/system/arm/bananapi_m2u.rst
@@ -25,6 +25,7 @@  The Banana Pi M2U machine supports the following devices:
  * SATA
  * TWI (I2C)
  * USB 2.0
+ * Hardware Watchdog
 
 Limitations
 """""""""""
@@ -33,7 +34,6 @@  Currently, Banana Pi M2U does *not* support the following features:
 
 - Graphical output via HDMI, GPU and/or the Display Engine
 - Audio output
-- Hardware Watchdog
 - Real Time Clock
 
 Also see the 'unimplemented' array in the Allwinner R40 SoC module
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 98ca5ebc7d..386edbae15 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -411,6 +411,7 @@  config ALLWINNER_R40
     select AHCI
     select ALLWINNER_SRAMC
     select ALLWINNER_A10_PIT
+    select ALLWINNER_WDT
     select AXP2XX_PMU
     select SERIAL
     select ARM_TIMER
diff --git a/hw/arm/allwinner-r40.c b/hw/arm/allwinner-r40.c
index 534be4a735..a28e5b3886 100644
--- a/hw/arm/allwinner-r40.c
+++ b/hw/arm/allwinner-r40.c
@@ -53,6 +53,7 @@  const hwaddr allwinner_r40_memmap[] = {
     [AW_R40_DEV_OHCI2]      = 0x01c1c400,
     [AW_R40_DEV_CCU]        = 0x01c20000,
     [AW_R40_DEV_PIT]        = 0x01c20c00,
+    [AW_R40_DEV_WDT]        = 0x01c20c90,
     [AW_R40_DEV_UART0]      = 0x01c28000,
     [AW_R40_DEV_UART1]      = 0x01c28400,
     [AW_R40_DEV_UART2]      = 0x01c28800,
@@ -279,6 +280,8 @@  static void allwinner_r40_init(Object *obj)
     object_property_add_alias(obj, "clk1-freq", OBJECT(&s->timer),
                               "clk1-freq");
 
+    object_initialize_child(obj, "wdt", &s->wdt, TYPE_AW_WDT_SUN4I);
+
     object_initialize_child(obj, "ccu", &s->ccu, TYPE_AW_R40_CCU);
 
     for (int i = 0; i < AW_R40_NUM_MMCS; i++) {
@@ -545,6 +548,11 @@  static void allwinner_r40_realize(DeviceState *dev, Error **errp)
     sysbus_connect_irq(SYS_BUS_DEVICE(&s->emac), 0,
                        qdev_get_gpio_in(DEVICE(&s->gic), AW_R40_GIC_SPI_EMAC));
 
+    /* WDT */
+    sysbus_realize(SYS_BUS_DEVICE(&s->wdt), &error_fatal);
+    sysbus_mmio_map_overlap(SYS_BUS_DEVICE(&s->wdt), 0,
+                            allwinner_r40_memmap[AW_R40_DEV_WDT], 1);
+
     /* Unimplemented devices */
     for (unsigned i = 0; i < ARRAY_SIZE(r40_unimplemented); i++) {
         create_unimplemented_device(r40_unimplemented[i].device_name,
diff --git a/include/hw/arm/allwinner-r40.h b/include/hw/arm/allwinner-r40.h
index c589fcc1c1..66c38e7d90 100644
--- a/include/hw/arm/allwinner-r40.h
+++ b/include/hw/arm/allwinner-r40.h
@@ -33,6 +33,7 @@ 
 #include "hw/net/allwinner-sun8i-emac.h"
 #include "hw/usb/hcd-ohci.h"
 #include "hw/usb/hcd-ehci.h"
+#include "hw/watchdog/allwinner-wdt.h"
 #include "target/arm/cpu.h"
 #include "sysemu/block-backend.h"
 
@@ -54,6 +55,7 @@  enum {
     AW_R40_DEV_OHCI2,
     AW_R40_DEV_CCU,
     AW_R40_DEV_PIT,
+    AW_R40_DEV_WDT,
     AW_R40_DEV_UART0,
     AW_R40_DEV_UART1,
     AW_R40_DEV_UART2,
@@ -114,6 +116,7 @@  struct AwR40State {
     const hwaddr *memmap;
     AwSRAMCState sramc;
     AwA10PITState timer;
+    AwWdtState wdt;
     AllwinnerAHCIState sata;
     AwSdHostState mmc[AW_R40_NUM_MMCS];
     EHCISysBusState ehci[AW_R40_NUM_USB];