| Message ID | 20240115182757.1095012-4-linux@roeck-us.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/arm: Add support for USB, SATA, and watchdog to Allwinner R40 | expand |
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); > +
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
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 --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];
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(-)