| Message ID | 1527014840-21236-4-git-send-email-ray.jui@broadcom.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, May 22, 2018 at 11:47:18AM -0700, Ray Jui wrote: > If the watchdog hardware is already enabled during the boot process, > when the Linux watchdog driver loads, it should reset the watchdog and > tell the watchdog framework. As a result, ping can be generated from > the watchdog framework, until the userspace watchdog daemon takes over > control > > Signed-off-by: Ray Jui <ray.jui@broadcom.com> > Reviewed-by: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com> > Reviewed-by: Scott Branden <scott.branden@broadcom.com> > --- > drivers/watchdog/sp805_wdt.c | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/drivers/watchdog/sp805_wdt.c b/drivers/watchdog/sp805_wdt.c > index 1484609..408ffbe 100644 > --- a/drivers/watchdog/sp805_wdt.c > +++ b/drivers/watchdog/sp805_wdt.c > @@ -42,6 +42,7 @@ > /* control register masks */ > #define INT_ENABLE (1 << 0) > #define RESET_ENABLE (1 << 1) > + #define ENABLE_MASK (INT_ENABLE | RESET_ENABLE) > #define WDTINTCLR 0x00C > #define WDTRIS 0x010 > #define WDTMIS 0x014 > @@ -74,6 +75,18 @@ module_param(nowayout, bool, 0); > MODULE_PARM_DESC(nowayout, > "Set to 1 to keep watchdog running after device release"); > > +/* returns true if wdt is running; otherwise returns false */ > +static bool wdt_is_running(struct watchdog_device *wdd) > +{ > + struct sp805_wdt *wdt = watchdog_get_drvdata(wdd); > + > + if ((readl_relaxed(wdt->base + WDTCONTROL) & ENABLE_MASK) == > + ENABLE_MASK) > + return true; > + else > + return false; return !!(readl_relaxed(wdt->base + WDTCONTROL) & ENABLE_MASK)); > +} > + > /* This routine finds load value that will reset system in required timout */ > static int wdt_setload(struct watchdog_device *wdd, unsigned int timeout) > { > @@ -239,6 +252,15 @@ sp805_wdt_probe(struct amba_device *adev, const struct amba_id *id) > watchdog_init_timeout(&wdt->wdd, 0, &adev->dev); > wdt_setload(&wdt->wdd, wdt->wdd.timeout); > > + /* > + * If HW is already running, enable/reset the wdt and set the running > + * bit to tell the wdt subsystem > + */ > + if (wdt_is_running(&wdt->wdd)) { > + wdt_enable(&wdt->wdd); > + set_bit(WDOG_HW_RUNNING, &wdt->wdd.status); > + } > + > ret = watchdog_register_device(&wdt->wdd); > if (ret) { > dev_err(&adev->dev, "watchdog_register_device() failed: %d\n", > -- > 2.1.4 >
Hi Guenter, On 5/22/2018 1:54 PM, Guenter Roeck wrote: > On Tue, May 22, 2018 at 11:47:18AM -0700, Ray Jui wrote: >> If the watchdog hardware is already enabled during the boot process, >> when the Linux watchdog driver loads, it should reset the watchdog and >> tell the watchdog framework. As a result, ping can be generated from >> the watchdog framework, until the userspace watchdog daemon takes over >> control >> >> Signed-off-by: Ray Jui <ray.jui@broadcom.com> >> Reviewed-by: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com> >> Reviewed-by: Scott Branden <scott.branden@broadcom.com> >> --- >> drivers/watchdog/sp805_wdt.c | 22 ++++++++++++++++++++++ >> 1 file changed, 22 insertions(+) >> >> diff --git a/drivers/watchdog/sp805_wdt.c b/drivers/watchdog/sp805_wdt.c >> index 1484609..408ffbe 100644 >> --- a/drivers/watchdog/sp805_wdt.c >> +++ b/drivers/watchdog/sp805_wdt.c >> @@ -42,6 +42,7 @@ >> /* control register masks */ >> #define INT_ENABLE (1 << 0) >> #define RESET_ENABLE (1 << 1) >> + #define ENABLE_MASK (INT_ENABLE | RESET_ENABLE) >> #define WDTINTCLR 0x00C >> #define WDTRIS 0x010 >> #define WDTMIS 0x014 >> @@ -74,6 +75,18 @@ module_param(nowayout, bool, 0); >> MODULE_PARM_DESC(nowayout, >> "Set to 1 to keep watchdog running after device release"); >> >> +/* returns true if wdt is running; otherwise returns false */ >> +static bool wdt_is_running(struct watchdog_device *wdd) >> +{ >> + struct sp805_wdt *wdt = watchdog_get_drvdata(wdd); >> + >> + if ((readl_relaxed(wdt->base + WDTCONTROL) & ENABLE_MASK) == >> + ENABLE_MASK) >> + return true; >> + else >> + return false; > > return !!(readl_relaxed(wdt->base + WDTCONTROL) & ENABLE_MASK)); > Note ENABLE_MASK contains two bits (INT_ENABLE and RESET_ENABLE); therefore, a simple !!(expression) would not work? That is, the masked result needs to be compared against the mask again to ensure both bits are set, right? Thanks, Ray
On 18-05-22 04:24 PM, Ray Jui wrote: > Hi Guenter, > > On 5/22/2018 1:54 PM, Guenter Roeck wrote: >> On Tue, May 22, 2018 at 11:47:18AM -0700, Ray Jui wrote: >>> If the watchdog hardware is already enabled during the boot process, >>> when the Linux watchdog driver loads, it should reset the watchdog and >>> tell the watchdog framework. As a result, ping can be generated from >>> the watchdog framework, until the userspace watchdog daemon takes over >>> control >>> >>> Signed-off-by: Ray Jui <ray.jui@broadcom.com> >>> Reviewed-by: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com> >>> Reviewed-by: Scott Branden <scott.branden@broadcom.com> >>> --- >>> drivers/watchdog/sp805_wdt.c | 22 ++++++++++++++++++++++ >>> 1 file changed, 22 insertions(+) >>> >>> diff --git a/drivers/watchdog/sp805_wdt.c >>> b/drivers/watchdog/sp805_wdt.c >>> index 1484609..408ffbe 100644 >>> --- a/drivers/watchdog/sp805_wdt.c >>> +++ b/drivers/watchdog/sp805_wdt.c >>> @@ -42,6 +42,7 @@ >>> /* control register masks */ >>> #define INT_ENABLE (1 << 0) >>> #define RESET_ENABLE (1 << 1) >>> + #define ENABLE_MASK (INT_ENABLE | RESET_ENABLE) >>> #define WDTINTCLR 0x00C >>> #define WDTRIS 0x010 >>> #define WDTMIS 0x014 >>> @@ -74,6 +75,18 @@ module_param(nowayout, bool, 0); >>> MODULE_PARM_DESC(nowayout, >>> "Set to 1 to keep watchdog running after device release"); >>> +/* returns true if wdt is running; otherwise returns false */ >>> +static bool wdt_is_running(struct watchdog_device *wdd) >>> +{ >>> + struct sp805_wdt *wdt = watchdog_get_drvdata(wdd); >>> + >>> + if ((readl_relaxed(wdt->base + WDTCONTROL) & ENABLE_MASK) == >>> + ENABLE_MASK) >>> + return true; >>> + else >>> + return false; >> >> return !!(readl_relaxed(wdt->base + WDTCONTROL) & ENABLE_MASK)); >> > > Note ENABLE_MASK contains two bits (INT_ENABLE and RESET_ENABLE); > therefore, a simple !!(expression) would not work? That is, the masked > result needs to be compared against the mask again to ensure both bits > are set, right? Ray - your original code looks correct to me. Easier to read and less prone to errors as shown in the attempted translation to a single statement. > > Thanks, > > Ray
On 23/05/18 08:52, Scott Branden wrote: > > > On 18-05-22 04:24 PM, Ray Jui wrote: >> Hi Guenter, >> >> On 5/22/2018 1:54 PM, Guenter Roeck wrote: >>> On Tue, May 22, 2018 at 11:47:18AM -0700, Ray Jui wrote: >>>> If the watchdog hardware is already enabled during the boot process, >>>> when the Linux watchdog driver loads, it should reset the watchdog and >>>> tell the watchdog framework. As a result, ping can be generated from >>>> the watchdog framework, until the userspace watchdog daemon takes over >>>> control >>>> >>>> Signed-off-by: Ray Jui <ray.jui@broadcom.com> >>>> Reviewed-by: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com> >>>> Reviewed-by: Scott Branden <scott.branden@broadcom.com> >>>> --- >>>> drivers/watchdog/sp805_wdt.c | 22 ++++++++++++++++++++++ >>>> 1 file changed, 22 insertions(+) >>>> >>>> diff --git a/drivers/watchdog/sp805_wdt.c >>>> b/drivers/watchdog/sp805_wdt.c >>>> index 1484609..408ffbe 100644 >>>> --- a/drivers/watchdog/sp805_wdt.c >>>> +++ b/drivers/watchdog/sp805_wdt.c >>>> @@ -42,6 +42,7 @@ >>>> /* control register masks */ >>>> #define INT_ENABLE (1 << 0) >>>> #define RESET_ENABLE (1 << 1) >>>> + #define ENABLE_MASK (INT_ENABLE | RESET_ENABLE) >>>> #define WDTINTCLR 0x00C >>>> #define WDTRIS 0x010 >>>> #define WDTMIS 0x014 >>>> @@ -74,6 +75,18 @@ module_param(nowayout, bool, 0); >>>> MODULE_PARM_DESC(nowayout, >>>> "Set to 1 to keep watchdog running after device release"); >>>> +/* returns true if wdt is running; otherwise returns false */ >>>> +static bool wdt_is_running(struct watchdog_device *wdd) >>>> +{ >>>> + struct sp805_wdt *wdt = watchdog_get_drvdata(wdd); >>>> + >>>> + if ((readl_relaxed(wdt->base + WDTCONTROL) & ENABLE_MASK) == >>>> + ENABLE_MASK) >>>> + return true; >>>> + else >>>> + return false; >>> >>> return !!(readl_relaxed(wdt->base + WDTCONTROL) & ENABLE_MASK)); >>> >> >> Note ENABLE_MASK contains two bits (INT_ENABLE and RESET_ENABLE); >> therefore, a simple !!(expression) would not work? That is, the masked >> result needs to be compared against the mask again to ensure both bits >> are set, right? > Ray - your original code looks correct to me. Easier to read and less > prone to errors as shown in the attempted translation to a single > statement. if (<boolean condition>) return true; else return false; still looks really dumb, though, and IMO is actually harder to read than just "return <boolean condition>;" because it forces you to stop and double-check that the logic is, in fact, only doing the obvious thing. Robin. p.s. No thanks for making me remember the mind-boggling horror of briefly maintaining part of this legacy codebase... :P $ grep -r '? true : false' --include=*.cpp . | wc -l 951
Hi Robin, On 5/23/2018 4:48 AM, Robin Murphy wrote: > On 23/05/18 08:52, Scott Branden wrote: >> >> >> On 18-05-22 04:24 PM, Ray Jui wrote: >>> Hi Guenter, >>> >>> On 5/22/2018 1:54 PM, Guenter Roeck wrote: >>>> On Tue, May 22, 2018 at 11:47:18AM -0700, Ray Jui wrote: >>>>> If the watchdog hardware is already enabled during the boot process, >>>>> when the Linux watchdog driver loads, it should reset the watchdog and >>>>> tell the watchdog framework. As a result, ping can be generated from >>>>> the watchdog framework, until the userspace watchdog daemon takes over >>>>> control >>>>> >>>>> Signed-off-by: Ray Jui <ray.jui@broadcom.com> >>>>> Reviewed-by: Vladimir Olovyannikov >>>>> <vladimir.olovyannikov@broadcom.com> >>>>> Reviewed-by: Scott Branden <scott.branden@broadcom.com> >>>>> --- >>>>> drivers/watchdog/sp805_wdt.c | 22 ++++++++++++++++++++++ >>>>> 1 file changed, 22 insertions(+) >>>>> >>>>> diff --git a/drivers/watchdog/sp805_wdt.c >>>>> b/drivers/watchdog/sp805_wdt.c >>>>> index 1484609..408ffbe 100644 >>>>> --- a/drivers/watchdog/sp805_wdt.c >>>>> +++ b/drivers/watchdog/sp805_wdt.c >>>>> @@ -42,6 +42,7 @@ >>>>> /* control register masks */ >>>>> #define INT_ENABLE (1 << 0) >>>>> #define RESET_ENABLE (1 << 1) >>>>> + #define ENABLE_MASK (INT_ENABLE | RESET_ENABLE) >>>>> #define WDTINTCLR 0x00C >>>>> #define WDTRIS 0x010 >>>>> #define WDTMIS 0x014 >>>>> @@ -74,6 +75,18 @@ module_param(nowayout, bool, 0); >>>>> MODULE_PARM_DESC(nowayout, >>>>> "Set to 1 to keep watchdog running after device release"); >>>>> +/* returns true if wdt is running; otherwise returns false */ >>>>> +static bool wdt_is_running(struct watchdog_device *wdd) >>>>> +{ >>>>> + struct sp805_wdt *wdt = watchdog_get_drvdata(wdd); >>>>> + >>>>> + if ((readl_relaxed(wdt->base + WDTCONTROL) & ENABLE_MASK) == >>>>> + ENABLE_MASK) >>>>> + return true; >>>>> + else >>>>> + return false; >>>> >>>> return !!(readl_relaxed(wdt->base + WDTCONTROL) & ENABLE_MASK)); >>>> >>> >>> Note ENABLE_MASK contains two bits (INT_ENABLE and RESET_ENABLE); >>> therefore, a simple !!(expression) would not work? That is, the >>> masked result needs to be compared against the mask again to ensure >>> both bits are set, right? >> Ray - your original code looks correct to me. Easier to read and less >> prone to errors as shown in the attempted translation to a single >> statement. > > if (<boolean condition>) > return true; > else > return false; > > still looks really dumb, though, and IMO is actually harder to read than > just "return <boolean condition>;" because it forces you to stop and > double-check that the logic is, in fact, only doing the obvious thing. If you can propose a way to modify my original code above to make it more readable, I'm fine to make the change. As I mentioned, I don't think the following change proposed by Guenter will work due to the reason I pointed out: return !!(readl_relaxed(wdt->base + WDTCONTROL) & ENABLE_MASK)); > > Robin. > > > > p.s. No thanks for making me remember the mind-boggling horror of > briefly maintaining part of this legacy codebase... :P > > $ grep -r '? true : false' --include=*.cpp . | wc -l > 951
On 23/05/18 17:29, Ray Jui wrote: > Hi Robin, > > On 5/23/2018 4:48 AM, Robin Murphy wrote: >> On 23/05/18 08:52, Scott Branden wrote: >>> >>> >>> On 18-05-22 04:24 PM, Ray Jui wrote: >>>> Hi Guenter, >>>> >>>> On 5/22/2018 1:54 PM, Guenter Roeck wrote: >>>>> On Tue, May 22, 2018 at 11:47:18AM -0700, Ray Jui wrote: >>>>>> If the watchdog hardware is already enabled during the boot process, >>>>>> when the Linux watchdog driver loads, it should reset the watchdog >>>>>> and >>>>>> tell the watchdog framework. As a result, ping can be generated from >>>>>> the watchdog framework, until the userspace watchdog daemon takes >>>>>> over >>>>>> control >>>>>> >>>>>> Signed-off-by: Ray Jui <ray.jui@broadcom.com> >>>>>> Reviewed-by: Vladimir Olovyannikov >>>>>> <vladimir.olovyannikov@broadcom.com> >>>>>> Reviewed-by: Scott Branden <scott.branden@broadcom.com> >>>>>> --- >>>>>> drivers/watchdog/sp805_wdt.c | 22 ++++++++++++++++++++++ >>>>>> 1 file changed, 22 insertions(+) >>>>>> >>>>>> diff --git a/drivers/watchdog/sp805_wdt.c >>>>>> b/drivers/watchdog/sp805_wdt.c >>>>>> index 1484609..408ffbe 100644 >>>>>> --- a/drivers/watchdog/sp805_wdt.c >>>>>> +++ b/drivers/watchdog/sp805_wdt.c >>>>>> @@ -42,6 +42,7 @@ >>>>>> /* control register masks */ >>>>>> #define INT_ENABLE (1 << 0) >>>>>> #define RESET_ENABLE (1 << 1) >>>>>> + #define ENABLE_MASK (INT_ENABLE | RESET_ENABLE) >>>>>> #define WDTINTCLR 0x00C >>>>>> #define WDTRIS 0x010 >>>>>> #define WDTMIS 0x014 >>>>>> @@ -74,6 +75,18 @@ module_param(nowayout, bool, 0); >>>>>> MODULE_PARM_DESC(nowayout, >>>>>> "Set to 1 to keep watchdog running after device release"); >>>>>> +/* returns true if wdt is running; otherwise returns false */ >>>>>> +static bool wdt_is_running(struct watchdog_device *wdd) >>>>>> +{ >>>>>> + struct sp805_wdt *wdt = watchdog_get_drvdata(wdd); >>>>>> + >>>>>> + if ((readl_relaxed(wdt->base + WDTCONTROL) & ENABLE_MASK) == >>>>>> + ENABLE_MASK) >>>>>> + return true; >>>>>> + else >>>>>> + return false; >>>>> >>>>> return !!(readl_relaxed(wdt->base + WDTCONTROL) & ENABLE_MASK)); >>>>> >>>> >>>> Note ENABLE_MASK contains two bits (INT_ENABLE and RESET_ENABLE); >>>> therefore, a simple !!(expression) would not work? That is, the >>>> masked result needs to be compared against the mask again to ensure >>>> both bits are set, right? >>> Ray - your original code looks correct to me. Easier to read and >>> less prone to errors as shown in the attempted translation to a >>> single statement. >> >> if (<boolean condition>) >> return true; >> else >> return false; >> >> still looks really dumb, though, and IMO is actually harder to read >> than just "return <boolean condition>;" because it forces you to stop >> and double-check that the logic is, in fact, only doing the obvious >> thing. > > If you can propose a way to modify my original code above to make it > more readable, I'm fine to make the change. Well, return readl_relaxed(wdt->base + WDTCONTROL) & ENABLE_MASK == ENABLE_MASK; would probably be reasonable to anyone other than the 80-column zealots, but removing the silly boolean-to-boolean translation idiom really only emphasises the fact that it's fundamentally a big complex statement; for maximum clarity I'd be inclined to separate the two logical operations (read and comparison), e.g.: u32 wdtcontrol = readl_relaxed(wdt->base + WDTCONTROL); return wdtcontrol & ENABLE_MASK == ENABLE_MASK; which is still -3 lines vs. the original. > As I mentioned, I don't think the following change proposed by Guenter > will work due to the reason I pointed out: > > return !!(readl_relaxed(wdt->base + WDTCONTROL) & ENABLE_MASK)); FWIW, getting the desired result should only need one logical not swapping for a bitwise one there: return !(~readl_relaxed(wdt->base + WDTCONTROL) & ENABLE_MASK); but that's well into "too clever for its own good" territory ;) Robin.
Raym On 18-05-23 09:29 AM, Ray Jui wrote: > Hi Robin, > > On 5/23/2018 4:48 AM, Robin Murphy wrote: >> On 23/05/18 08:52, Scott Branden wrote: >>> >>> >>> On 18-05-22 04:24 PM, Ray Jui wrote: >>>> Hi Guenter, >>>> >>>> On 5/22/2018 1:54 PM, Guenter Roeck wrote: >>>>> On Tue, May 22, 2018 at 11:47:18AM -0700, Ray Jui wrote: >>>>>> If the watchdog hardware is already enabled during the boot process, >>>>>> when the Linux watchdog driver loads, it should reset the >>>>>> watchdog and >>>>>> tell the watchdog framework. As a result, ping can be generated from >>>>>> the watchdog framework, until the userspace watchdog daemon takes >>>>>> over >>>>>> control >>>>>> >>>>>> Signed-off-by: Ray Jui <ray.jui@broadcom.com> >>>>>> Reviewed-by: Vladimir Olovyannikov >>>>>> <vladimir.olovyannikov@broadcom.com> >>>>>> Reviewed-by: Scott Branden <scott.branden@broadcom.com> >>>>>> --- >>>>>> drivers/watchdog/sp805_wdt.c | 22 ++++++++++++++++++++++ >>>>>> 1 file changed, 22 insertions(+) >>>>>> >>>>>> diff --git a/drivers/watchdog/sp805_wdt.c >>>>>> b/drivers/watchdog/sp805_wdt.c >>>>>> index 1484609..408ffbe 100644 >>>>>> --- a/drivers/watchdog/sp805_wdt.c >>>>>> +++ b/drivers/watchdog/sp805_wdt.c >>>>>> @@ -42,6 +42,7 @@ >>>>>> /* control register masks */ >>>>>> #define INT_ENABLE (1 << 0) >>>>>> #define RESET_ENABLE (1 << 1) >>>>>> + #define ENABLE_MASK (INT_ENABLE | RESET_ENABLE) >>>>>> #define WDTINTCLR 0x00C >>>>>> #define WDTRIS 0x010 >>>>>> #define WDTMIS 0x014 >>>>>> @@ -74,6 +75,18 @@ module_param(nowayout, bool, 0); >>>>>> MODULE_PARM_DESC(nowayout, >>>>>> "Set to 1 to keep watchdog running after device release"); >>>>>> +/* returns true if wdt is running; otherwise returns false */ >>>>>> +static bool wdt_is_running(struct watchdog_device *wdd) >>>>>> +{ >>>>>> + struct sp805_wdt *wdt = watchdog_get_drvdata(wdd); >>>>>> + >>>>>> + if ((readl_relaxed(wdt->base + WDTCONTROL) & ENABLE_MASK) == >>>>>> + ENABLE_MASK) >>>>>> + return true; >>>>>> + else >>>>>> + return false; >>>>> >>>>> return !!(readl_relaxed(wdt->base + WDTCONTROL) & ENABLE_MASK)); >>>>> >>>> >>>> Note ENABLE_MASK contains two bits (INT_ENABLE and RESET_ENABLE); >>>> therefore, a simple !!(expression) would not work? That is, the >>>> masked result needs to be compared against the mask again to ensure >>>> both bits are set, right? >>> Ray - your original code looks correct to me. Easier to read and >>> less prone to errors as shown in the attempted translation to a >>> single statement. >> >> if (<boolean condition>) >> return true; >> else >> return false; >> >> still looks really dumb, though, and IMO is actually harder to read >> than just "return <boolean condition>;" because it forces you to stop >> and double-check that the logic is, in fact, only doing the obvious >> thing. > > If you can propose a way to modify my original code above to make it > more readable, I'm fine to make the change. > > As I mentioned, I don't think the following change proposed by Guenter > will work due to the reason I pointed out: > > return !!(readl_relaxed(wdt->base + WDTCONTROL) & ENABLE_MASK)); What they are looking for is: return ((readl_relaxed(wdt->base + WDTCONTROL) & ENABLE_MASK) == ENABLE_MASK); or maybe: return !!((readl_relaxed(wdt->base + WDTCONTROL) & ENABLE_MASK) == ENABLE_MASK); > >> >> Robin. >> >> >> >> p.s. No thanks for making me remember the mind-boggling horror of >> briefly maintaining part of this legacy codebase... :P >> >> $ grep -r '? true : false' --include=*.cpp . | wc -l >> 951
On Wed, May 23, 2018 at 12:48:10PM +0100, Robin Murphy wrote: > On 23/05/18 08:52, Scott Branden wrote: > > > > > >On 18-05-22 04:24 PM, Ray Jui wrote: > >>Hi Guenter, > >> > >>On 5/22/2018 1:54 PM, Guenter Roeck wrote: > >>>On Tue, May 22, 2018 at 11:47:18AM -0700, Ray Jui wrote: > >>>>If the watchdog hardware is already enabled during the boot process, > >>>>when the Linux watchdog driver loads, it should reset the watchdog and > >>>>tell the watchdog framework. As a result, ping can be generated from > >>>>the watchdog framework, until the userspace watchdog daemon takes over > >>>>control > >>>> > >>>>Signed-off-by: Ray Jui <ray.jui@broadcom.com> > >>>>Reviewed-by: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com> > >>>>Reviewed-by: Scott Branden <scott.branden@broadcom.com> > >>>>--- > >>>> drivers/watchdog/sp805_wdt.c | 22 ++++++++++++++++++++++ > >>>> 1 file changed, 22 insertions(+) > >>>> > >>>>diff --git a/drivers/watchdog/sp805_wdt.c > >>>>b/drivers/watchdog/sp805_wdt.c > >>>>index 1484609..408ffbe 100644 > >>>>--- a/drivers/watchdog/sp805_wdt.c > >>>>+++ b/drivers/watchdog/sp805_wdt.c > >>>>@@ -42,6 +42,7 @@ > >>>> /* control register masks */ > >>>> #define INT_ENABLE (1 << 0) > >>>> #define RESET_ENABLE (1 << 1) > >>>>+ #define ENABLE_MASK (INT_ENABLE | RESET_ENABLE) > >>>> #define WDTINTCLR 0x00C > >>>> #define WDTRIS 0x010 > >>>> #define WDTMIS 0x014 > >>>>@@ -74,6 +75,18 @@ module_param(nowayout, bool, 0); > >>>> MODULE_PARM_DESC(nowayout, > >>>> "Set to 1 to keep watchdog running after device release"); > >>>> +/* returns true if wdt is running; otherwise returns false */ > >>>>+static bool wdt_is_running(struct watchdog_device *wdd) > >>>>+{ > >>>>+ struct sp805_wdt *wdt = watchdog_get_drvdata(wdd); > >>>>+ > >>>>+ if ((readl_relaxed(wdt->base + WDTCONTROL) & ENABLE_MASK) == > >>>>+ ENABLE_MASK) > >>>>+ return true; > >>>>+ else > >>>>+ return false; > >>> > >>> return !!(readl_relaxed(wdt->base + WDTCONTROL) & ENABLE_MASK)); > >>> > >> > >>Note ENABLE_MASK contains two bits (INT_ENABLE and RESET_ENABLE); > >>therefore, a simple !!(expression) would not work? That is, the masked > >>result needs to be compared against the mask again to ensure both bits > >>are set, right? > >Ray - your original code looks correct to me. Easier to read and less > >prone to errors as shown in the attempted translation to a single > >statement. > > if (<boolean condition>) > return true; > else > return false; > > still looks really dumb, though, and IMO is actually harder to read than > just "return <boolean condition>;" because it forces you to stop and > double-check that the logic is, in fact, only doing the obvious thing. > Yes, and I won't accept it, sorry. Guenter > Robin. > > > > p.s. No thanks for making me remember the mind-boggling horror of briefly > maintaining part of this legacy codebase... :P > > $ grep -r '? true : false' --include=*.cpp . | wc -l > 951
On Wed, May 23, 2018 at 06:15:14PM +0100, Robin Murphy wrote: > On 23/05/18 17:29, Ray Jui wrote: > >Hi Robin, > > > >On 5/23/2018 4:48 AM, Robin Murphy wrote: > >>On 23/05/18 08:52, Scott Branden wrote: > >>> > >>> > >>>On 18-05-22 04:24 PM, Ray Jui wrote: > >>>>Hi Guenter, > >>>> > >>>>On 5/22/2018 1:54 PM, Guenter Roeck wrote: > >>>>>On Tue, May 22, 2018 at 11:47:18AM -0700, Ray Jui wrote: > >>>>>>If the watchdog hardware is already enabled during the boot process, > >>>>>>when the Linux watchdog driver loads, it should reset the > >>>>>>watchdog and > >>>>>>tell the watchdog framework. As a result, ping can be generated from > >>>>>>the watchdog framework, until the userspace watchdog daemon > >>>>>>takes over > >>>>>>control > >>>>>> > >>>>>>Signed-off-by: Ray Jui <ray.jui@broadcom.com> > >>>>>>Reviewed-by: Vladimir Olovyannikov > >>>>>><vladimir.olovyannikov@broadcom.com> > >>>>>>Reviewed-by: Scott Branden <scott.branden@broadcom.com> > >>>>>>--- > >>>>>> drivers/watchdog/sp805_wdt.c | 22 ++++++++++++++++++++++ > >>>>>> 1 file changed, 22 insertions(+) > >>>>>> > >>>>>>diff --git a/drivers/watchdog/sp805_wdt.c > >>>>>>b/drivers/watchdog/sp805_wdt.c > >>>>>>index 1484609..408ffbe 100644 > >>>>>>--- a/drivers/watchdog/sp805_wdt.c > >>>>>>+++ b/drivers/watchdog/sp805_wdt.c > >>>>>>@@ -42,6 +42,7 @@ > >>>>>> /* control register masks */ > >>>>>> #define INT_ENABLE (1 << 0) > >>>>>> #define RESET_ENABLE (1 << 1) > >>>>>>+ #define ENABLE_MASK (INT_ENABLE | RESET_ENABLE) > >>>>>> #define WDTINTCLR 0x00C > >>>>>> #define WDTRIS 0x010 > >>>>>> #define WDTMIS 0x014 > >>>>>>@@ -74,6 +75,18 @@ module_param(nowayout, bool, 0); > >>>>>> MODULE_PARM_DESC(nowayout, > >>>>>> "Set to 1 to keep watchdog running after device release"); > >>>>>> +/* returns true if wdt is running; otherwise returns false */ > >>>>>>+static bool wdt_is_running(struct watchdog_device *wdd) > >>>>>>+{ > >>>>>>+ struct sp805_wdt *wdt = watchdog_get_drvdata(wdd); > >>>>>>+ > >>>>>>+ if ((readl_relaxed(wdt->base + WDTCONTROL) & ENABLE_MASK) == > >>>>>>+ ENABLE_MASK) > >>>>>>+ return true; > >>>>>>+ else > >>>>>>+ return false; > >>>>> > >>>>> return !!(readl_relaxed(wdt->base + WDTCONTROL) & ENABLE_MASK)); > >>>>> > >>>> > >>>>Note ENABLE_MASK contains two bits (INT_ENABLE and RESET_ENABLE); > >>>>therefore, a simple !!(expression) would not work? That is, the > >>>>masked result needs to be compared against the mask again to ensure > >>>>both bits are set, right? > >>>Ray - your original code looks correct to me. Easier to read and less > >>>prone to errors as shown in the attempted translation to a single > >>>statement. > >> > >> if (<boolean condition>) > >> return true; > >> else > >> return false; > >> > >>still looks really dumb, though, and IMO is actually harder to read than > >>just "return <boolean condition>;" because it forces you to stop and > >>double-check that the logic is, in fact, only doing the obvious thing. > > > >If you can propose a way to modify my original code above to make it more > >readable, I'm fine to make the change. > > Well, > > return readl_relaxed(wdt->base + WDTCONTROL) & ENABLE_MASK == ENABLE_MASK; > > would probably be reasonable to anyone other than the 80-column zealots, but > removing the silly boolean-to-boolean translation idiom really only > emphasises the fact that it's fundamentally a big complex statement; for > maximum clarity I'd be inclined to separate the two logical operations (read > and comparison), e.g.: > > u32 wdtcontrol = readl_relaxed(wdt->base + WDTCONTROL); > > return wdtcontrol & ENABLE_MASK == ENABLE_MASK; == has higher precendence than bitwise &, so this will need ( ), but otherwise I agree. > > which is still -3 lines vs. the original. > > >As I mentioned, I don't think the following change proposed by Guenter > >will work due to the reason I pointed out: > > > >return !!(readl_relaxed(wdt->base + WDTCONTROL) & ENABLE_MASK)); > > FWIW, getting the desired result should only need one logical not swapping > for a bitwise one there: > > return !(~readl_relaxed(wdt->base + WDTCONTROL) & ENABLE_MASK); > > but that's well into "too clever for its own good" territory ;) Yes, that would be confusing. > > Robin.
Hi Guenter/Robin, On 5/23/2018 11:09 AM, Guenter Roeck wrote: > On Wed, May 23, 2018 at 06:15:14PM +0100, Robin Murphy wrote: >> On 23/05/18 17:29, Ray Jui wrote: >>> Hi Robin, >>> >>> On 5/23/2018 4:48 AM, Robin Murphy wrote: >>>> On 23/05/18 08:52, Scott Branden wrote: >>>>> >>>>> >>>>> On 18-05-22 04:24 PM, Ray Jui wrote: >>>>>> Hi Guenter, >>>>>> >>>>>> On 5/22/2018 1:54 PM, Guenter Roeck wrote: >>>>>>> On Tue, May 22, 2018 at 11:47:18AM -0700, Ray Jui wrote: >>>>>>>> If the watchdog hardware is already enabled during the boot process, >>>>>>>> when the Linux watchdog driver loads, it should reset the >>>>>>>> watchdog and >>>>>>>> tell the watchdog framework. As a result, ping can be generated from >>>>>>>> the watchdog framework, until the userspace watchdog daemon >>>>>>>> takes over >>>>>>>> control >>>>>>>> >>>>>>>> Signed-off-by: Ray Jui <ray.jui@broadcom.com> >>>>>>>> Reviewed-by: Vladimir Olovyannikov >>>>>>>> <vladimir.olovyannikov@broadcom.com> >>>>>>>> Reviewed-by: Scott Branden <scott.branden@broadcom.com> >>>>>>>> --- >>>>>>>> drivers/watchdog/sp805_wdt.c | 22 ++++++++++++++++++++++ >>>>>>>> 1 file changed, 22 insertions(+) >>>>>>>> >>>>>>>> diff --git a/drivers/watchdog/sp805_wdt.c >>>>>>>> b/drivers/watchdog/sp805_wdt.c >>>>>>>> index 1484609..408ffbe 100644 >>>>>>>> --- a/drivers/watchdog/sp805_wdt.c >>>>>>>> +++ b/drivers/watchdog/sp805_wdt.c >>>>>>>> @@ -42,6 +42,7 @@ >>>>>>>> /* control register masks */ >>>>>>>> #define INT_ENABLE (1 << 0) >>>>>>>> #define RESET_ENABLE (1 << 1) >>>>>>>> + #define ENABLE_MASK (INT_ENABLE | RESET_ENABLE) >>>>>>>> #define WDTINTCLR 0x00C >>>>>>>> #define WDTRIS 0x010 >>>>>>>> #define WDTMIS 0x014 >>>>>>>> @@ -74,6 +75,18 @@ module_param(nowayout, bool, 0); >>>>>>>> MODULE_PARM_DESC(nowayout, >>>>>>>> "Set to 1 to keep watchdog running after device release"); >>>>>>>> +/* returns true if wdt is running; otherwise returns false */ >>>>>>>> +static bool wdt_is_running(struct watchdog_device *wdd) >>>>>>>> +{ >>>>>>>> + struct sp805_wdt *wdt = watchdog_get_drvdata(wdd); >>>>>>>> + >>>>>>>> + if ((readl_relaxed(wdt->base + WDTCONTROL) & ENABLE_MASK) == >>>>>>>> + ENABLE_MASK) >>>>>>>> + return true; >>>>>>>> + else >>>>>>>> + return false; >>>>>>> >>>>>>> return !!(readl_relaxed(wdt->base + WDTCONTROL) & ENABLE_MASK)); >>>>>>> >>>>>> >>>>>> Note ENABLE_MASK contains two bits (INT_ENABLE and RESET_ENABLE); >>>>>> therefore, a simple !!(expression) would not work? That is, the >>>>>> masked result needs to be compared against the mask again to ensure >>>>>> both bits are set, right? >>>>> Ray - your original code looks correct to me. Easier to read and less >>>>> prone to errors as shown in the attempted translation to a single >>>>> statement. >>>> >>>> if (<boolean condition>) >>>> return true; >>>> else >>>> return false; >>>> >>>> still looks really dumb, though, and IMO is actually harder to read than >>>> just "return <boolean condition>;" because it forces you to stop and >>>> double-check that the logic is, in fact, only doing the obvious thing. >>> >>> If you can propose a way to modify my original code above to make it more >>> readable, I'm fine to make the change. >> >> Well, >> >> return readl_relaxed(wdt->base + WDTCONTROL) & ENABLE_MASK == ENABLE_MASK; >> >> would probably be reasonable to anyone other than the 80-column zealots, but >> removing the silly boolean-to-boolean translation idiom really only >> emphasises the fact that it's fundamentally a big complex statement; for >> maximum clarity I'd be inclined to separate the two logical operations (read >> and comparison), e.g.: >> >> u32 wdtcontrol = readl_relaxed(wdt->base + WDTCONTROL); >> >> return wdtcontrol & ENABLE_MASK == ENABLE_MASK; > > == has higher precendence than bitwise &, so this will need ( ), > but otherwise I agree. > Sure. Let me change the code to the following: u32 wdtcontrol = readl_relaxed(wdt->base + WDTCONTROL); return (wdtcontrol & ENABLE_MASK) == ENABLE_MASK; Thanks. Ray >> >> which is still -3 lines vs. the original. >> >>> As I mentioned, I don't think the following change proposed by Guenter >>> will work due to the reason I pointed out: >>> >>> return !!(readl_relaxed(wdt->base + WDTCONTROL) & ENABLE_MASK)); >> >> FWIW, getting the desired result should only need one logical not swapping >> for a bitwise one there: >> >> return !(~readl_relaxed(wdt->base + WDTCONTROL) & ENABLE_MASK); >> >> but that's well into "too clever for its own good" territory ;) > > Yes, that would be confusing. > >> >> Robin.
diff --git a/drivers/watchdog/sp805_wdt.c b/drivers/watchdog/sp805_wdt.c index 1484609..408ffbe 100644 --- a/drivers/watchdog/sp805_wdt.c +++ b/drivers/watchdog/sp805_wdt.c @@ -42,6 +42,7 @@ /* control register masks */ #define INT_ENABLE (1 << 0) #define RESET_ENABLE (1 << 1) + #define ENABLE_MASK (INT_ENABLE | RESET_ENABLE) #define WDTINTCLR 0x00C #define WDTRIS 0x010 #define WDTMIS 0x014 @@ -74,6 +75,18 @@ module_param(nowayout, bool, 0); MODULE_PARM_DESC(nowayout, "Set to 1 to keep watchdog running after device release"); +/* returns true if wdt is running; otherwise returns false */ +static bool wdt_is_running(struct watchdog_device *wdd) +{ + struct sp805_wdt *wdt = watchdog_get_drvdata(wdd); + + if ((readl_relaxed(wdt->base + WDTCONTROL) & ENABLE_MASK) == + ENABLE_MASK) + return true; + else + return false; +} + /* This routine finds load value that will reset system in required timout */ static int wdt_setload(struct watchdog_device *wdd, unsigned int timeout) { @@ -239,6 +252,15 @@ sp805_wdt_probe(struct amba_device *adev, const struct amba_id *id) watchdog_init_timeout(&wdt->wdd, 0, &adev->dev); wdt_setload(&wdt->wdd, wdt->wdd.timeout); + /* + * If HW is already running, enable/reset the wdt and set the running + * bit to tell the wdt subsystem + */ + if (wdt_is_running(&wdt->wdd)) { + wdt_enable(&wdt->wdd); + set_bit(WDOG_HW_RUNNING, &wdt->wdd.status); + } + ret = watchdog_register_device(&wdt->wdd); if (ret) { dev_err(&adev->dev, "watchdog_register_device() failed: %d\n",