| Message ID | 1527014840-21236-2-git-send-email-ray.jui@broadcom.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, May 22, 2018 at 11:47:16AM -0700, Ray Jui wrote: > Update the SP805 binding document to add optional 'timeout-sec' > devicetree property > > Signed-off-by: Ray Jui <ray.jui@broadcom.com> > Reviewed-by: Scott Branden <scott.branden@broadcom.com> Reviewed-by: Guenter Roeck <linux@roeck-us.net> > --- > Documentation/devicetree/bindings/watchdog/sp805-wdt.txt | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt b/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt > index edc4f0e..f898a86 100644 > --- a/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt > +++ b/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt > @@ -19,6 +19,8 @@ Required properties: > > Optional properties: > - interrupts : Should specify WDT interrupt number. > +- timeout-sec : Should specify default WDT timeout in seconds. If unset, the > + default timeout is 30 seconds > > Examples: > > -- > 2.1.4 >
On 22/05/18 19:47, Ray Jui wrote: > Update the SP805 binding document to add optional 'timeout-sec' > devicetree property > > Signed-off-by: Ray Jui <ray.jui@broadcom.com> > Reviewed-by: Scott Branden <scott.branden@broadcom.com> > --- > Documentation/devicetree/bindings/watchdog/sp805-wdt.txt | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt b/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt > index edc4f0e..f898a86 100644 > --- a/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt > +++ b/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt > @@ -19,6 +19,8 @@ Required properties: > > Optional properties: > - interrupts : Should specify WDT interrupt number. > +- timeout-sec : Should specify default WDT timeout in seconds. If unset, the > + default timeout is 30 seconds According to the SP805 TRM, the default interval is dependent on the rate of WDOGCLK, but would typically be a lot longer than that :/ On a related note, anyone have any idea why we seem to have two subtly different SP805 bindings defined? Robin. > > Examples: > >
On 5/23/2018 3:57 AM, Robin Murphy wrote: > On 22/05/18 19:47, Ray Jui wrote: >> Update the SP805 binding document to add optional 'timeout-sec' >> devicetree property >> >> Signed-off-by: Ray Jui <ray.jui@broadcom.com> >> Reviewed-by: Scott Branden <scott.branden@broadcom.com> >> --- >> Documentation/devicetree/bindings/watchdog/sp805-wdt.txt | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt >> b/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt >> index edc4f0e..f898a86 100644 >> --- a/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt >> +++ b/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt >> @@ -19,6 +19,8 @@ Required properties: >> Optional properties: >> - interrupts : Should specify WDT interrupt number. >> +- timeout-sec : Should specify default WDT timeout in seconds. If >> unset, the >> + default timeout is 30 seconds > > According to the SP805 TRM, the default interval is dependent on the > rate of WDOGCLK, but would typically be a lot longer than that :/ > > On a related note, anyone have any idea why we seem to have two subtly > different SP805 bindings defined? Interesting, I did not even know that until you pointed this out (and it's funny that I found that I actually reviewed arm,sp805.txt internally in Broadcom code review). It looks like one was done by Bhupesh Sharma (sp805-wdt.txt) and the other was done by Anup Patel (arm,sp805.txt). Both were merged at the same time around March 20, 2016: 915c56bc01d6. I'd assume both were sent out at around the same time. It sounds like we should definitely remove one of them. Given that sp805-wdt.txt appears to have more detailed descriptions on the use of the clocks, should we remove arm,sp805.txt? Thanks, Ray > > Robin. > >> Examples: >>
On Wed, May 23, 2018 at 11:57:25AM +0100, Robin Murphy wrote: > On 22/05/18 19:47, Ray Jui wrote: > >Update the SP805 binding document to add optional 'timeout-sec' > >devicetree property > > > >Signed-off-by: Ray Jui <ray.jui@broadcom.com> > >Reviewed-by: Scott Branden <scott.branden@broadcom.com> > >--- > > Documentation/devicetree/bindings/watchdog/sp805-wdt.txt | 2 ++ > > 1 file changed, 2 insertions(+) > > > >diff --git a/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt b/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt > >index edc4f0e..f898a86 100644 > >--- a/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt > >+++ b/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt > >@@ -19,6 +19,8 @@ Required properties: > > Optional properties: > > - interrupts : Should specify WDT interrupt number. > >+- timeout-sec : Should specify default WDT timeout in seconds. If unset, the > >+ default timeout is 30 seconds > > According to the SP805 TRM, the default interval is dependent on the rate of > WDOGCLK, but would typically be a lot longer than that :/ > Depends on the definition of "default". In the context of watchdog drivers, it is (or should be) a driver default, not a chip default. Guenter > On a related note, anyone have any idea why we seem to have two subtly > different SP805 bindings defined? > > Robin. > > > Examples: > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 23, 2018 at 09:25:49AM -0700, Ray Jui wrote: > > > On 5/23/2018 3:57 AM, Robin Murphy wrote: > > On 22/05/18 19:47, Ray Jui wrote: > > > Update the SP805 binding document to add optional 'timeout-sec' > > > devicetree property > > > > > > Signed-off-by: Ray Jui <ray.jui@broadcom.com> > > > Reviewed-by: Scott Branden <scott.branden@broadcom.com> > > > --- > > > Documentation/devicetree/bindings/watchdog/sp805-wdt.txt | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git > > > a/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt > > > b/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt > > > index edc4f0e..f898a86 100644 > > > --- a/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt > > > +++ b/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt > > > @@ -19,6 +19,8 @@ Required properties: > > > Optional properties: > > > - interrupts : Should specify WDT interrupt number. > > > +- timeout-sec : Should specify default WDT timeout in seconds. If > > > unset, the > > > + default timeout is 30 seconds > > > > According to the SP805 TRM, the default interval is dependent on the > > rate of WDOGCLK, but would typically be a lot longer than that :/ > > > > On a related note, anyone have any idea why we seem to have two subtly > > different SP805 bindings defined? Sigh. > Interesting, I did not even know that until you pointed this out (and it's > funny that I found that I actually reviewed arm,sp805.txt internally in > Broadcom code review). > > It looks like one was done by Bhupesh Sharma (sp805-wdt.txt) and the other > was done by Anup Patel (arm,sp805.txt). Both were merged at the same time > around March 20, 2016: 915c56bc01d6. I'd assume both were sent out at around > the same time. > > It sounds like we should definitely remove one of them. Given that > sp805-wdt.txt appears to have more detailed descriptions on the use of the > clocks, should we remove arm,sp805.txt? Take whichever text you like, but I prefer filenames using the compatible string and the correct string is 'arm,sp805' because '-wdt' is redundant. You can probably safely just update all the dts files with 'arm,sp805' and just remove 'arm,sp805-wdt' because it is not actually used (as the ID registers are). Rob
On 5/23/2018 11:59 AM, Rob Herring wrote: > On Wed, May 23, 2018 at 09:25:49AM -0700, Ray Jui wrote: >> >> >> On 5/23/2018 3:57 AM, Robin Murphy wrote: >>> On 22/05/18 19:47, Ray Jui wrote: >>>> Update the SP805 binding document to add optional 'timeout-sec' >>>> devicetree property >>>> >>>> Signed-off-by: Ray Jui <ray.jui@broadcom.com> >>>> Reviewed-by: Scott Branden <scott.branden@broadcom.com> >>>> --- >>>> Documentation/devicetree/bindings/watchdog/sp805-wdt.txt | 2 ++ >>>> 1 file changed, 2 insertions(+) >>>> >>>> diff --git >>>> a/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt >>>> b/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt >>>> index edc4f0e..f898a86 100644 >>>> --- a/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt >>>> +++ b/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt >>>> @@ -19,6 +19,8 @@ Required properties: >>>> Optional properties: >>>> - interrupts : Should specify WDT interrupt number. >>>> +- timeout-sec : Should specify default WDT timeout in seconds. If >>>> unset, the >>>> + default timeout is 30 seconds >>> >>> According to the SP805 TRM, the default interval is dependent on the >>> rate of WDOGCLK, but would typically be a lot longer than that :/ >>> >>> On a related note, anyone have any idea why we seem to have two subtly >>> different SP805 bindings defined? > > Sigh. > >> Interesting, I did not even know that until you pointed this out (and it's >> funny that I found that I actually reviewed arm,sp805.txt internally in >> Broadcom code review). >> >> It looks like one was done by Bhupesh Sharma (sp805-wdt.txt) and the other >> was done by Anup Patel (arm,sp805.txt). Both were merged at the same time >> around March 20, 2016: 915c56bc01d6. I'd assume both were sent out at around >> the same time. >> >> It sounds like we should definitely remove one of them. Given that >> sp805-wdt.txt appears to have more detailed descriptions on the use of the >> clocks, should we remove arm,sp805.txt? > > Take whichever text you like, but I prefer filenames using the > compatible string and the correct string is 'arm,sp805' because '-wdt' > is redundant. You can probably safely just update all the dts files with > 'arm,sp805' and just remove 'arm,sp805-wdt' because it is not actually > used (as the ID registers are). Okay. I'll consolidate everything into arm,sp805.txt. Will also fix all DTS files to use "arm,sp805". The fix for actual DTS files will be in a different patch series. > > Rob >
On 23/05/18 19:10, Guenter Roeck wrote: > On Wed, May 23, 2018 at 11:57:25AM +0100, Robin Murphy wrote: >> On 22/05/18 19:47, Ray Jui wrote: >>> Update the SP805 binding document to add optional 'timeout-sec' >>> devicetree property >>> >>> Signed-off-by: Ray Jui <ray.jui@broadcom.com> >>> Reviewed-by: Scott Branden <scott.branden@broadcom.com> >>> --- >>> Documentation/devicetree/bindings/watchdog/sp805-wdt.txt | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt b/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt >>> index edc4f0e..f898a86 100644 >>> --- a/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt >>> +++ b/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt >>> @@ -19,6 +19,8 @@ Required properties: >>> Optional properties: >>> - interrupts : Should specify WDT interrupt number. >>> +- timeout-sec : Should specify default WDT timeout in seconds. If unset, the >>> + default timeout is 30 seconds >> >> According to the SP805 TRM, the default interval is dependent on the rate of >> WDOGCLK, but would typically be a lot longer than that :/ >> > Depends on the definition of "default". In the context of watchdog drivers, > it is (or should be) a driver default, not a chip default. DT describes hardware, not driver behaviour. I appreciate that where a timeout *is* specified, that is effectively a hardware aspect even if it's something an OS consuming the binding still has to voluntarily program into the device. The notion of "this is the longest period of time for which you can reasonably expect to see no activity under normal operation" is indeed a property of the platform as a whole - a system with user-accessible PCIe slots may need to reflect the worst case of one CPU waiting for an ATS invalidation timeout with interrupts disabled, whereas a much shorter period might be appropriate for the same SoC in some closed-down embedded device. The absence of the property, though, doesn't convey anything other than "I don't know" and/or "it doesn't really matter", and in that situation the default is always going to be "whatever the OS thinks is appropriate". The binding itself can't possibly know, whereas an OS might be configured for some pseudo-real-time application which it knows warrants a maximum of 100ms regardless of what the DT does or doesn't say. In the case of SP805, if the OS doesn't reconfigure it at all, there happens to be an actual hardware default of (2^32 / WDOGCLK), but since that's already implicit in the compatible it doesn't really need saying either. Optional properties don't need to explicitly state what their absence might infer, especially when it's not directly meaningful (just imagine trying to do that for bindings/regulator/regulator.txt...), so I would suggest following the 93% of existing bindings which simply don't try to claim some default value for this property. I also think the fact that, within the context of this patch series, the Linux driver doesn't even do what the binding claims only goes to help make my point ;) Robin.
On 23/05/18 20:29, Ray Jui wrote: > > > On 5/23/2018 11:59 AM, Rob Herring wrote: >> On Wed, May 23, 2018 at 09:25:49AM -0700, Ray Jui wrote: >>> >>> >>> On 5/23/2018 3:57 AM, Robin Murphy wrote: >>>> On 22/05/18 19:47, Ray Jui wrote: >>>>> Update the SP805 binding document to add optional 'timeout-sec' >>>>> devicetree property >>>>> >>>>> Signed-off-by: Ray Jui <ray.jui@broadcom.com> >>>>> Reviewed-by: Scott Branden <scott.branden@broadcom.com> >>>>> --- >>>>> Documentation/devicetree/bindings/watchdog/sp805-wdt.txt | 2 ++ >>>>> 1 file changed, 2 insertions(+) >>>>> >>>>> diff --git >>>>> a/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt >>>>> b/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt >>>>> index edc4f0e..f898a86 100644 >>>>> --- a/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt >>>>> +++ b/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt >>>>> @@ -19,6 +19,8 @@ Required properties: >>>>> Optional properties: >>>>> - interrupts : Should specify WDT interrupt number. >>>>> +- timeout-sec : Should specify default WDT timeout in seconds. If >>>>> unset, the >>>>> + default timeout is 30 seconds >>>> >>>> According to the SP805 TRM, the default interval is dependent on the >>>> rate of WDOGCLK, but would typically be a lot longer than that :/ >>>> >>>> On a related note, anyone have any idea why we seem to have two subtly >>>> different SP805 bindings defined? >> >> Sigh. >> >>> Interesting, I did not even know that until you pointed this out (and >>> it's >>> funny that I found that I actually reviewed arm,sp805.txt internally in >>> Broadcom code review). >>> >>> It looks like one was done by Bhupesh Sharma (sp805-wdt.txt) and the >>> other >>> was done by Anup Patel (arm,sp805.txt). Both were merged at the same >>> time >>> around March 20, 2016: 915c56bc01d6. I'd assume both were sent out at >>> around >>> the same time. >>> >>> It sounds like we should definitely remove one of them. Given that >>> sp805-wdt.txt appears to have more detailed descriptions on the use >>> of the >>> clocks, should we remove arm,sp805.txt? >> >> Take whichever text you like, but I prefer filenames using the >> compatible string and the correct string is 'arm,sp805' because '-wdt' >> is redundant. You can probably safely just update all the dts files with >> 'arm,sp805' and just remove 'arm,sp805-wdt' because it is not actually >> used (as the ID registers are). > > Okay. I'll consolidate everything into arm,sp805.txt. Will also fix all > DTS files to use "arm,sp805". The fix for actual DTS files will be in a > different patch series. Looking at the current in-tree DTs, for extra fun try to figure out which binding each instance was following for the clocks. The most common answer seems to be "neither"... :( Robin.
On Thu, May 24, 2018 at 02:25:34PM +0100, Robin Murphy wrote: > On 23/05/18 19:10, Guenter Roeck wrote: > >On Wed, May 23, 2018 at 11:57:25AM +0100, Robin Murphy wrote: > >>On 22/05/18 19:47, Ray Jui wrote: > >>>Update the SP805 binding document to add optional 'timeout-sec' > >>>devicetree property > >>> > >>>Signed-off-by: Ray Jui <ray.jui@broadcom.com> > >>>Reviewed-by: Scott Branden <scott.branden@broadcom.com> > >>>--- > >>> Documentation/devicetree/bindings/watchdog/sp805-wdt.txt | 2 ++ > >>> 1 file changed, 2 insertions(+) > >>> > >>>diff --git a/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt b/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt > >>>index edc4f0e..f898a86 100644 > >>>--- a/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt > >>>+++ b/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt > >>>@@ -19,6 +19,8 @@ Required properties: > >>> Optional properties: > >>> - interrupts : Should specify WDT interrupt number. > >>>+- timeout-sec : Should specify default WDT timeout in seconds. If unset, the > >>>+ default timeout is 30 seconds > >> > >>According to the SP805 TRM, the default interval is dependent on the rate of > >>WDOGCLK, but would typically be a lot longer than that :/ > >> > >Depends on the definition of "default". In the context of watchdog drivers, > >it is (or should be) a driver default, not a chip default. > > DT describes hardware, not driver behaviour. > In this case it describes expected system behavior. Most definitely it does not describe some hardware default. Please note that I do not engage in discussions I consider bike-shedding. This is one of those. Dropping out. Guenter
diff --git a/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt b/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt index edc4f0e..f898a86 100644 --- a/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt +++ b/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt @@ -19,6 +19,8 @@ Required properties: Optional properties: - interrupts : Should specify WDT interrupt number. +- timeout-sec : Should specify default WDT timeout in seconds. If unset, the + default timeout is 30 seconds Examples: