Hi Philippe,
On Tue, Jan 16, 2024 at 11:04 AM Philippe Mathieu-Daudé <phi...@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 <li...@roeck-us.net>
>
Guenter, thank you for submitting this change. The commit looks fine to me,
so
Reviewed-by: Strahinja Jankovic <strahinja.p.janko...@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);
> > +
>
