| Message ID | 20241007063408.2360874-2-chin-ting_kuo@aspeedtech.com |
|---|---|
| State | New |
| Headers | show |
| Series | Update ASPEED WDT bootstatus | expand |
Hi Krzysztof, Thanks for the review. > -----Original Message----- > From: Krzysztof Kozlowski <krzk@kernel.org> > Sent: Monday, October 7, 2024 2:58 PM > Subject: Re: [PATCH 1/4] dt-bindings: watchdog: aspeed: Add property for WDT > SW reset > > On 07/10/2024 08:34, Chin-Ting Kuo wrote: > > Add "aspeed,restart-sw" property to distinguish normal WDT reset from > > system restart triggered by SW consciously. > > > > Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com> > > --- > > .../bindings/watchdog/aspeed,ast2400-wdt.yaml | 11 > +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git > > a/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml > > b/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml > > index be78a9865584..6cc3604c295a 100644 > > --- > > a/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml > > +++ b/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.ya > > +++ ml > > @@ -95,6 +95,17 @@ properties: > > array with the first word defined using the AST2600_WDT_RESET1_* > macros, > > and the second word defined using the AST2600_WDT_RESET2_* > macros. > > > > + aspeed,restart-sw: > > + $ref: /schemas/types.yaml#/definitions/flag > > + description: > > > Why >? > ">" will be removed in the next patch series and the description content will be concatenated after the colon, ":". > > + Normally, ASPEED WDT reset may occur when system hangs or > reboot > > + triggered by SW consciously. However, system doesn't know whether > the > > + restart is triggered by SW consciously since the reset event flag is > > + the same as normal WDT timeout reset. With this property, SW > > + can > > So DTS has this property and watchdog bites (timeout) but you will ignore it > and claim that it was software choice? > No. Normally, when WDT is enabled, a counter is also be enabled. When the counter is equal to an expected value, timeout event occurs. AST2600 hardware supports a SW mode, when a magic number is filled into a specific register, WDT reset is triggered immediately without controlling the counter and the counter is not counted. Thus, WDT timeout doesn't occur. > This does not make much sense to me, at least based on this explanation > > > + restart the system immediately and directly without wait for WDT > > + timeout occurs. The reset event flag is also different from the > normal > > + WDT reset. This property is only supported since AST2600 platform. > > Supported as drivers? How is this related? Or you mean hardware? Then > property should be restricted there. > It is a hardware supported function on AST2600. For platform compatibility, without this property, all behaviors are the same as the previous generation platform, AST2500. This property may be removed in the next patch series with referring to Rob suggestion in the other reply. After checking with the major users, it is feasible to remove this property and using SW reset by default when the restart operation is triggered by SW deliberately on AST2600 platform. > Best regards, > Krzysztof Best Wishes, Chin-Ting
Hi Rob, Thanks for the review. > -----Original Message----- > From: Rob Herring <robh@kernel.org> > Sent: Tuesday, October 8, 2024 2:00 AM > Subject: Re: [PATCH 1/4] dt-bindings: watchdog: aspeed: Add property for WDT > SW reset > > On Mon, Oct 07, 2024 at 02:34:05PM +0800, Chin-Ting Kuo wrote: > > Add "aspeed,restart-sw" property to distinguish normal WDT reset from > > system restart triggered by SW consciously. > > > > Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com> > > --- > > .../bindings/watchdog/aspeed,ast2400-wdt.yaml | 11 > +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git > > a/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml > > b/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml > > index be78a9865584..6cc3604c295a 100644 > > --- > > a/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml > > +++ b/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.ya > > +++ ml > > @@ -95,6 +95,17 @@ properties: > > array with the first word defined using the AST2600_WDT_RESET1_* > macros, > > and the second word defined using the AST2600_WDT_RESET2_* > macros. > > > > + aspeed,restart-sw: > > + $ref: /schemas/types.yaml#/definitions/flag > > + description: > > > + Normally, ASPEED WDT reset may occur when system hangs or > reboot > > + triggered by SW consciously. However, system doesn't know whether > the > > + restart is triggered by SW consciously since the reset event flag is > > + the same as normal WDT timeout reset. With this property, SW can > > + restart the system immediately and directly without wait for WDT > > + timeout occurs. The reset event flag is also different from the > normal > > + WDT reset. This property is only supported since AST2600 platform. > > Why can't this be implicit based on the ast2600 compatible string? > Yes, this property will be implicit based on the ast2600 compatible string in the next patch series. > Rob Chin-Ting
Hi Guenter, Thanks for the review. > -----Original Message----- > From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck > Sent: Tuesday, October 8, 2024 3:55 AM > Subject: Re: [PATCH 1/4] dt-bindings: watchdog: aspeed: Add property for WDT > SW reset > > On 10/7/24 10:59, Rob Herring wrote: > > On Mon, Oct 07, 2024 at 02:34:05PM +0800, Chin-Ting Kuo wrote: > >> Add "aspeed,restart-sw" property to distinguish normal WDT reset from > >> system restart triggered by SW consciously. > >> > >> Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com> > >> --- > >> .../bindings/watchdog/aspeed,ast2400-wdt.yaml | 11 > +++++++++++ > >> 1 file changed, 11 insertions(+) > >> > >> diff --git > >> a/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml > >> b/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml > >> index be78a9865584..6cc3604c295a 100644 > >> --- > >> a/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml > >> +++ b/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.y > >> +++ aml > >> @@ -95,6 +95,17 @@ properties: > >> array with the first word defined using the > AST2600_WDT_RESET1_* macros, > >> and the second word defined using the AST2600_WDT_RESET2_* > macros. > >> > >> + aspeed,restart-sw: > >> + $ref: /schemas/types.yaml#/definitions/flag > >> + description: > > >> + Normally, ASPEED WDT reset may occur when system hangs or > reboot > >> + triggered by SW consciously. However, system doesn't know > whether the > >> + restart is triggered by SW consciously since the reset event flag is > >> + the same as normal WDT timeout reset. With this property, SW can > >> + restart the system immediately and directly without wait for WDT > >> + timeout occurs. The reset event flag is also different from the > normal > >> + WDT reset. This property is only supported since AST2600 platform. > > > > Why can't this be implicit based on the ast2600 compatible string? > > > > Same question here. > Yes, this property will be implicit based on the ast2600 compatible string in the next patch series. > Guenter Chin-Ting
On 14/10/2024 04:07, Chin-Ting Kuo wrote: > Hi Krzysztof, > > Thanks for the review. > >> -----Original Message----- >> From: Krzysztof Kozlowski <krzk@kernel.org> >> Sent: Monday, October 7, 2024 2:58 PM >> Subject: Re: [PATCH 1/4] dt-bindings: watchdog: aspeed: Add property for WDT >> SW reset >> >> On 07/10/2024 08:34, Chin-Ting Kuo wrote: >>> Add "aspeed,restart-sw" property to distinguish normal WDT reset from >>> system restart triggered by SW consciously. >>> >>> Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com> >>> --- >>> .../bindings/watchdog/aspeed,ast2400-wdt.yaml | 11 >> +++++++++++ >>> 1 file changed, 11 insertions(+) >>> >>> diff --git >>> a/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml >>> b/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml >>> index be78a9865584..6cc3604c295a 100644 >>> --- >>> a/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml >>> +++ b/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.ya >>> +++ ml >>> @@ -95,6 +95,17 @@ properties: >>> array with the first word defined using the AST2600_WDT_RESET1_* >> macros, >>> and the second word defined using the AST2600_WDT_RESET2_* >> macros. >>> >>> + aspeed,restart-sw: >>> + $ref: /schemas/types.yaml#/definitions/flag >>> + description: > >> >> Why >? >> > > ">" will be removed in the next patch series and the description content will be > concatenated after the colon, ":". > >>> + Normally, ASPEED WDT reset may occur when system hangs or >> reboot >>> + triggered by SW consciously. However, system doesn't know whether >> the >>> + restart is triggered by SW consciously since the reset event flag is >>> + the same as normal WDT timeout reset. With this property, SW >>> + can >> >> So DTS has this property and watchdog bites (timeout) but you will ignore it >> and claim that it was software choice? >> > > No. Normally, when WDT is enabled, a counter is also be enabled. When the counter > is equal to an expected value, timeout event occurs. AST2600 hardware supports a SW > mode, when a magic number is filled into a specific register, WDT reset is triggered > immediately without controlling the counter and the counter is not counted. > Thus, WDT timeout doesn't occur. How is this a no? > >> This does not make much sense to me, at least based on this explanation >> >>> + restart the system immediately and directly without wait for WDT >>> + timeout occurs. The reset event flag is also different from the >> normal >>> + WDT reset. This property is only supported since AST2600 platform. >> >> Supported as drivers? How is this related? Or you mean hardware? Then >> property should be restricted there. >> > > It is a hardware supported function on AST2600. For platform compatibility, without > this property, all behaviors are the same as the previous generation platform, AST2500. > > This property may be removed in the next patch series with referring to Rob suggestion s/may/will/ > in the other reply. After checking with the major users, it is feasible to remove this > property and using SW reset by default when the restart operation is triggered by SW > deliberately on AST2600 platform. > Best regards, Krzysztof
Hi Krzysztof, > -----Original Message----- > From: Krzysztof Kozlowski <krzk@kernel.org> > Sent: Monday, October 14, 2024 2:53 PM > Subject: Re: [PATCH 1/4] dt-bindings: watchdog: aspeed: Add property for WDT > SW reset > > On 14/10/2024 04:07, Chin-Ting Kuo wrote: > > Hi Krzysztof, > > > > Thanks for the review. > > > >> -----Original Message----- > >> From: Krzysztof Kozlowski <krzk@kernel.org> > >> Sent: Monday, October 7, 2024 2:58 PM > >> Subject: Re: [PATCH 1/4] dt-bindings: watchdog: aspeed: Add property > >> for WDT SW reset > >> > >> On 07/10/2024 08:34, Chin-Ting Kuo wrote: > >>> Add "aspeed,restart-sw" property to distinguish normal WDT reset > >>> from system restart triggered by SW consciously. > >>> > >>> Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com> > >>> --- > >>> .../bindings/watchdog/aspeed,ast2400-wdt.yaml | 11 > >> +++++++++++ > >>> 1 file changed, 11 insertions(+) > >>> > >>> diff --git > >>> > a/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml > >>> > b/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml > >>> index be78a9865584..6cc3604c295a 100644 > >>> --- > >>> > a/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml > >>> +++ b/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt. > >>> +++ ya > >>> +++ ml > >>> @@ -95,6 +95,17 @@ properties: > >>> array with the first word defined using the > >>> AST2600_WDT_RESET1_* > >> macros, > >>> and the second word defined using the AST2600_WDT_RESET2_* > >> macros. > >>> > >>> + aspeed,restart-sw: > >>> + $ref: /schemas/types.yaml#/definitions/flag > >>> + description: > > >> > >> Why >? > >> > > > > ">" will be removed in the next patch series and the description > > content will be concatenated after the colon, ":". > > > >>> + Normally, ASPEED WDT reset may occur when system hangs or > >> reboot > >>> + triggered by SW consciously. However, system doesn't know > >>> + whether > >> the > >>> + restart is triggered by SW consciously since the reset event flag is > >>> + the same as normal WDT timeout reset. With this property, SW > >>> + can > >> > >> So DTS has this property and watchdog bites (timeout) but you will > >> ignore it and claim that it was software choice? > >> > > > > No. Normally, when WDT is enabled, a counter is also be enabled. When > > the counter is equal to an expected value, timeout event occurs. > > AST2600 hardware supports a SW mode, when a magic number is filled > > into a specific register, WDT reset is triggered immediately without > controlling the counter and the counter is not counted. > > Thus, WDT timeout doesn't occur. > > How is this a no? > It is used to emphasize that the driver doesn’t ignore the timeout event because the counter is not counted when SW mode is used and thus, no timeout occurs. > > > >> This does not make much sense to me, at least based on this > >> explanation > >> > >>> + restart the system immediately and directly without wait for WDT > >>> + timeout occurs. The reset event flag is also different from > >>> + the > >> normal > >>> + WDT reset. This property is only supported since AST2600 > platform. > >> > >> Supported as drivers? How is this related? Or you mean hardware? Then > >> property should be restricted there. > >> > > > > It is a hardware supported function on AST2600. For platform > > compatibility, without this property, all behaviors are the same as the > previous generation platform, AST2500. > > > > This property may be removed in the next patch series with referring > > to Rob suggestion > > s/may/will/ > This property will be removed in the next patch series. > > in the other reply. After checking with the major users, it is > > feasible to remove this property and using SW reset by default when > > the restart operation is triggered by SW deliberately on AST2600 platform. > > > > Best Wishes, Chin-Ting
diff --git a/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml b/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml index be78a9865584..6cc3604c295a 100644 --- a/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml +++ b/Documentation/devicetree/bindings/watchdog/aspeed,ast2400-wdt.yaml @@ -95,6 +95,17 @@ properties: array with the first word defined using the AST2600_WDT_RESET1_* macros, and the second word defined using the AST2600_WDT_RESET2_* macros. + aspeed,restart-sw: + $ref: /schemas/types.yaml#/definitions/flag + description: > + Normally, ASPEED WDT reset may occur when system hangs or reboot + triggered by SW consciously. However, system doesn't know whether the + restart is triggered by SW consciously since the reset event flag is + the same as normal WDT timeout reset. With this property, SW can + restart the system immediately and directly without wait for WDT + timeout occurs. The reset event flag is also different from the normal + WDT reset. This property is only supported since AST2600 platform. + required: - compatible - reg
Add "aspeed,restart-sw" property to distinguish normal WDT reset from system restart triggered by SW consciously. Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com> --- .../bindings/watchdog/aspeed,ast2400-wdt.yaml | 11 +++++++++++ 1 file changed, 11 insertions(+)