[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ed77d57facaaef0be796b4c6a742dc7bf3bff479.camel@codeconstruct.com.au>
Date: Fri, 08 Nov 2024 10:19:32 +1030
From: Andrew Jeffery <andrew@...econstruct.com.au>
To: Chin-Ting Kuo <chin-ting_kuo@...eedtech.com>, Patrick Williams
<patrick@...cx.xyz>, "wim@...ux-watchdog.org" <wim@...ux-watchdog.org>,
"linux@...ck-us.net"
<linux@...ck-us.net>
Cc: "joel@....id.au" <joel@....id.au>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>, "linux-aspeed@...ts.ozlabs.org"
<linux-aspeed@...ts.ozlabs.org>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, "linux-watchdog@...r.kernel.org"
<linux-watchdog@...r.kernel.org>, "Peter.Yin@...ntatw.com"
<Peter.Yin@...ntatw.com>, "Patrick_NC_Lin@...ynn.com"
<Patrick_NC_Lin@...ynn.com>, "Bonnie_Lo@...ynn.com" <Bonnie_Lo@...ynn.com>,
"DELPHINE_CHIU@...ynn.com" <DELPHINE_CHIU@...ynn.com>, BMC-SW
<BMC-SW@...eedtech.com>, "chnguyen@...erecomputing.com"
<chnguyen@...erecomputing.com>
Subject: Re: [PATCH v4 1/3] watchdog: aspeed: Update bootstatus handling
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@...econstruct.com.au>
> > 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
Powered by blists - more mailing lists