Hi Chin-Ting, On Thu, 2024-11-07 at 05:35 +0000, Chin-Ting Kuo wrote: > Hi Andrew, > > Thanks for the check. > > > -----Original Message----- > > From: Andrew Jeffery <andrew@xxxxxxxxxxxxxxxxxxxx> > > Sent: Monday, November 4, 2024 8:02 AM > > Subject: Re: [PATCH v4 1/3] watchdog: aspeed: Update bootstatus > > handling > > > > On Fri, 2024-11-01 at 14:21 -0400, Patrick Williams wrote: > > > On Fri, Nov 01, 2024 at 08:11:59PM +0800, Chin-Ting Kuo wrote: > > > > The boot status mapping rule follows the latest design guide > > > > from > > > > the OpenBMC shown as below. > > > > https://github.com/openbmc/docs/blob/master/designs/bmc-reboot-cause > > > > -update.md#proposed-design > > > > - WDIOF_EXTERN1 => system is reset by Software > > > > - WDIOF_CARDRESET => system is reset by WDT SoC reset > > > > - Others => other reset events, e.g., power on reset. > > > > > > I'm quite surprised that the above is relevant for a kernel > > > driver at > > > all. Isn't "EXTERN1" a name of a real watchdog signal from your > > > hardware (my recollection is that there are 2 external > > > watchdogs). > > > > I think you may be referring to WDTRST1 (and WDTRST2) here. > > > > WDTRST1, wdt_ext, is a pulse signal generated when WDT timeout > occurs. However, depending on the HW board design, wdt_ext doesn’t > always affect the system reset. Thus, EXTERN1 boot status can be > ignored in ASPEED WDT driver and just implement "CARDRESET" and > "others" types since EXTERN1 is not always affected/controlled by WDT > controller directly. Or, an additional property in dts can be added > to > distinguish whether the current EXTRST# reset event is triggered by > wdt_ext signal. Yep, I understand how it works. I was responding to Patrick's query to clear up some confusion around the watchdog signal names. > > > > > > > > Having said that, it was known that there would need to be > > > changes to > > > the driver because some of these conditions were not adequately > > > exposed at all. I'm just still surprised that we're needing to > > > reference that document as part of these changes. > > > > I think the main question is whether an internal, graceful > > (userspace- > > requested) reset is a reasonable use of WDIOF_EXTERN[12]. My > > feeling no. I > > wonder whether defining a new flag (WDIOF_REBOOT? > > WDIOF_GRACEFUL?) in the UAPI would be acceptable? > > > > Agree, but this is out of the scope of this patch series and can be > discussed and > implemented in the other future patches. I disagree, because then you're changing the userspace-visible behaviour of the driver yet again. I don't prefer the proposed patch as the way forward because I think it is abusing the meaning of WDIOF_EXTERN1. I think the concept needs input from the watchdog maintainers. Andrew
