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

[Guenter Roeck]

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