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