On Mon, Sep 25, 2023 at 05:04:10PM -0700, Zev Weiss wrote:
> On Sun, Sep 24, 2023 at 07:42:45PM PDT, Andrew Jeffery wrote:
> >
> >
> > On Fri, 22 Sep 2023, at 20:12, Zev Weiss wrote:
> > > This property configures the Aspeed watchdog timer's reset mask, which
> > > controls which peripherals are reset when the watchdog timer expires.
> > > Some platforms require that certain devices be left untouched across a
> > > reboot; aspeed,reset-mask can now be used to express such constraints.
> > >
> > > Signed-off-by: Zev Weiss <zev@xxxxxxxxxxxxxxxxx>
> > > ---
> > > .../bindings/watchdog/aspeed-wdt.txt | 18 +++-
> > > include/dt-bindings/watchdog/aspeed-wdt.h | 92 +++++++++++++++++++
> > > 2 files changed, 109 insertions(+), 1 deletion(-)
> > > create mode 100644 include/dt-bindings/watchdog/aspeed-wdt.h
> > >
> > > diff --git a/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
> > > b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
> > > index a8197632d6d2..3208adb3e52e 100644
> > > --- a/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
> > > +++ b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
> > > @@ -47,7 +47,15 @@ Optional properties for AST2500-compatible watchdogs:
> > > is configured as push-pull, then set the pulse
> > > polarity to active-high. The default is active-low.
> > >
> > > -Example:
> > > +Optional properties for AST2500- and AST2600-compatible watchdogs:
> > > + - aspeed,reset-mask: A bitmask indicating which peripherals will be reset if
> > > + the watchdog timer expires. On AST2500 this should be a
> > > + single word defined using the AST2500_WDT_RESET_* macros;
> > > + on AST2600 this should be a two-word array with the first
> > > + word defined using the AST2600_WDT_RESET1_* macros and the
> > > + second word defined using the AST2600_WDT_RESET2_* macros.
> > > +
> > > +Examples:
> > >
> > > wdt1: watchdog@1e785000 {
> > > compatible = "aspeed,ast2400-wdt";
> > > @@ -55,3 +63,11 @@ Example:
> > > aspeed,reset-type = "system";
> > > aspeed,external-signal;
> > > };
> > > +
> > > + #include <dt-bindings/watchdog/aspeed-wdt.h>
> > > + wdt2: watchdog@1e785040 {
> > > + compatible = "aspeed,ast2600-wdt";
> > > + reg = <0x1e785040 0x40>;
> > > + aspeed,reset-mask = <AST2600_WDT_RESET1_DEFAULT
> > > + (AST2600_WDT_RESET2_DEFAULT & ~AST2600_WDT_RESET2_LPC)>;
> > > + };
> >
> > Rob has acked your current approach already, but I do wonder about an
> > alternative that aligns more with the clock/reset/interrupt properties.
> > Essentially, define a new generic watchdog property that is specified on
> > the controllers to be reset by the watchdog (or even on just the
> > watchdog node itself, emulating what you've proposed here):
> >
> > watchdog-resets = <phandle index>;
> >
> > The phandle links to the watchdog of interest, and the index specifies
> > the controller associated with the configuration. It might even be
> > useful to do:
> >
> > watchdog-resets = <phandle index enable>;
> >
> > "enable" could provide explicit control over whether somethings should
> > be reset or not (as a way to prevent reset if the controller targeted by
> > the provided index would otherwise be reset in accordance with the
> > default reset value in the watchdog controller).
> >
> > The macros from the dt-bindings header can then use macros to name the
> > indexes rather than define a mask tied to the register layout. The index
> > may still in some way represent the mask position. This has the benefit
> > of hiding the issue of one vs two configuration registers between the
> > AST2500 and AST2600 while also allowing other controllers to exploit the
> > binding (Nuvoton BMCs? Though maybe it's generalising too early?).
> >
>
> Sorry, I'm having a bit of a hard time picturing exactly what you're
> suggesting here...to start with:
>
> > property that is specified on the controllers to be reset by the
> > watchdog
>
> and
>
> > or even on just the watchdog node itself
>
> seem on the face of it like two fairly different approaches to me. The
> former sounds more like existing clock/reset/etc. stuff, where the
> peripheral has a property describing its relationship to the "central"
> subsystem, and various peripheral drivers are all individually responsible
> for observing that property and calling in to the central subsystem to
> configure things for that peripheral appropriately; if I'm understanding you
> correctly, it might look something like:
>
> &spi1 {
> watchdog-resets = <&wdt1 WDT_INDEX_SPI1 0>;
> };
>
> Or maybe something more like how pinctrl works, via phandles to subnodes of
> the central device?
>
> &wdt1 {
> wdt1_spi1_reset: spi1_reset {
> reg = <0x1c>;
> bit = <24>;
> };
> };
>
> &spi1 {
> watchdog-resets = <&wdt1_spi1_reset 0>;
> };
>
> Either way, it seems like it'd be complicated by any insufficient
> granularity in the watchdog w.r.t. having independent control over the
> individual devices represented by separate DT nodes (such as how the AST2500
> watchdog has a single SPI controller reset bit instead of one per SPI
> interface, or its "misc SOC controller" bit governing all sorts of odds and
> ends).
>
> In the latter case (property on the wdt node), would it essentially just be
> kind of an indirection layer mapping hardware-independent device indices to
> specific registers/bits? It's not obvious to me what purpose a phandle to
> the peripheral device node would serve (would the wdt driver have a good way
> of identifying what specific peripheral it's pointing to to know what bit to
> twiddle?), but maybe I'm misunderstanding what you're suggesting...
>
>
> I guess my other uncertainty is the balance between generalization and
> applicability -- how many other watchdog devices have sufficient comparable
> configurability to make use of it? I haven't pored over all of them, but
> from a random sampling of 20 so of the other existing wdt drivers I don't
> see any obvious candidates -- the closest I saw were cpwd.c, which
> apparently can distinguish between a CPU reset and a CPU/backplane/board
> reset, and realtek_otto_wdt.c, which can do a CPU or a SOC reset (though I
> don't have any of the hardware docs to know what capabilities other devices
> might provide that the drivers don't use). Do the Nuvoton BMCs have
> watchdogs with peripheral-granularity reset configuration?
>
Quite frankly, I don't like where this is going. It is getting way
too complicated. And when something is becoming way too complicated,
I tend to put it on my backburner list. The length of that list quickly
approaches maxint.
Guenter
