[PATCH 0/5] Enhance support for the SP805 WDT

[PATCH 0/5] Enhance support for the SP805 WDT

[Ray Jui]

This patch series enhances the support for the SP805 watchdog timer.
First of all, 'timeout-sec' devicetree property is added. In addition,
support is also added to allow the driver to reset the watchdog if it
has been detected that watchdot has been started in the bootloader. In
this case, the driver will initiate the ping service from the kernel
watchdog subsystem, before a user mode daemon takes over. This series
also enables SP805 in the default ARM64 defconfig

This patch series is based off v4.17-rc5 and is available on GIHUB:
repo: https://github.com/Broadcom/arm64-linux.git
branch: sp805-wdt-v1

Ray Jui (5):
  Documentation: DT: Add optional 'timeout-sec' property for sp805
  watchdog: sp805: add 'timeout-sec' DT property support
  watchdog: sp805: set WDOG_HW_RUNNING when appropriate
  arm64: dt: set initial SR watchdog timeout to 60 seconds
  arm64: defconfig: add CONFIG_ARM_SP805_WATCHDOG

 .../devicetree/bindings/watchdog/sp805-wdt.txt     |  2 ++
 .../arm64/boot/dts/broadcom/stingray/stingray.dtsi |  1 +
 arch/arm64/configs/defconfig                       |  1 +
 drivers/watchdog/sp805_wdt.c                       | 31 +++++++++++++++++++++-
 4 files changed, 34 insertions(+), 1 deletion(-)

-- 
2.1.4

[PATCH 1/5] Documentation: DT: Add optional 'timeout-sec' property for sp805

[Ray Jui]

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
 
 Examples:
 
-- 
2.1.4

[PATCH 2/5] watchdog: sp805: add 'timeout-sec' DT property support

[Ray Jui]

Add support for optional devicetree property 'timeout-sec'.
'timeout-sec' is used in the driver if specified in devicetree.
Otherwise, fall back to driver default, i.e., 60 seconds

Signed-off-by: Ray Jui <ray.jui@broadcom.com>
Reviewed-by: Scott Branden <scott.branden@broadcom.com>
---
 drivers/watchdog/sp805_wdt.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/sp805_wdt.c b/drivers/watchdog/sp805_wdt.c
index 03805bc..1484609 100644
--- a/drivers/watchdog/sp805_wdt.c
+++ b/drivers/watchdog/sp805_wdt.c
@@ -230,7 +230,14 @@ sp805_wdt_probe(struct amba_device *adev, const struct amba_id *id)
 	spin_lock_init(&wdt->lock);
 	watchdog_set_nowayout(&wdt->wdd, nowayout);
 	watchdog_set_drvdata(&wdt->wdd, wdt);
-	wdt_setload(&wdt->wdd, DEFAULT_TIMEOUT);
+
+	/*
+	 * If 'timeout-sec' devicetree property is specified, use that.
+	 * Otherwise, use DEFAULT_TIMEOUT
+	 */
+	wdt->wdd.timeout = DEFAULT_TIMEOUT;
+	watchdog_init_timeout(&wdt->wdd, 0, &adev->dev);
+	wdt_setload(&wdt->wdd, wdt->wdd.timeout);
 
 	ret = watchdog_register_device(&wdt->wdd);
 	if (ret) {
-- 
2.1.4

[PATCH 3/5] watchdog: sp805: set WDOG_HW_RUNNING when appropriate

[Ray Jui]

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;
+}
+
 /* 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

[PATCH 4/5] arm64: dt: set initial SR watchdog timeout to 60 seconds

[Ray Jui]

Set initial Stingray watchdog timeout to 60 seconds

By the time when the userspace watchdog daemon is ready and taking
control over, the watchdog timeout will then be reset to what's
configured in the daemon

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>
---
 arch/arm64/boot/dts/broadcom/stingray/stingray.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/broadcom/stingray/stingray.dtsi b/arch/arm64/boot/dts/broadcom/stingray/stingray.dtsi
index 99aaff0..1e1cf49 100644
--- a/arch/arm64/boot/dts/broadcom/stingray/stingray.dtsi
+++ b/arch/arm64/boot/dts/broadcom/stingray/stingray.dtsi
@@ -420,6 +420,7 @@
 			interrupts = <GIC_SPI 189 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&hsls_25m_div2_clk>, <&hsls_div4_clk>;
 			clock-names = "wdogclk", "apb_pclk";
+			timeout-sec = <60>;
 		};
 
 		gpio_hsls: gpio@d0000 {
-- 
2.1.4

[PATCH 5/5] arm64: defconfig: add CONFIG_ARM_SP805_WATCHDOG

[Ray Jui]

Enable the SP805 watchdog timer

Signed-off-by: Ray Jui <ray.jui@broadcom.com>
---
 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index ecf6137..3fe5eb5 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -351,6 +351,7 @@ CONFIG_ROCKCHIP_THERMAL=m
 CONFIG_TEGRA_BPMP_THERMAL=m
 CONFIG_UNIPHIER_THERMAL=y
 CONFIG_WATCHDOG=y
+CONFIG_ARM_SP805_WATCHDOG=y
 CONFIG_S3C2410_WATCHDOG=y
 CONFIG_MESON_GXBB_WATCHDOG=m
 CONFIG_MESON_WATCHDOG=m
-- 
2.1.4

Re: [PATCH 3/5] watchdog: sp805: set WDOG_HW_RUNNING when appropriate

[Guenter Roeck]

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
> 

Re: [PATCH 1/5] Documentation: DT: Add optional 'timeout-sec' property for sp805

[Guenter Roeck]

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
> 

Re: [PATCH 2/5] watchdog: sp805: add 'timeout-sec' DT property support

[Guenter Roeck]

On Tue, May 22, 2018 at 11:47:17AM -0700, Ray Jui wrote:
> Add support for optional devicetree property 'timeout-sec'.
> 'timeout-sec' is used in the driver if specified in devicetree.
> Otherwise, fall back to driver default, i.e., 60 seconds
> 
> 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>

> ---
>  drivers/watchdog/sp805_wdt.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/sp805_wdt.c b/drivers/watchdog/sp805_wdt.c
> index 03805bc..1484609 100644
> --- a/drivers/watchdog/sp805_wdt.c
> +++ b/drivers/watchdog/sp805_wdt.c
> @@ -230,7 +230,14 @@ sp805_wdt_probe(struct amba_device *adev, const struct amba_id *id)
>  	spin_lock_init(&wdt->lock);
>  	watchdog_set_nowayout(&wdt->wdd, nowayout);
>  	watchdog_set_drvdata(&wdt->wdd, wdt);
> -	wdt_setload(&wdt->wdd, DEFAULT_TIMEOUT);
> +
> +	/*
> +	 * If 'timeout-sec' devicetree property is specified, use that.
> +	 * Otherwise, use DEFAULT_TIMEOUT
> +	 */
> +	wdt->wdd.timeout = DEFAULT_TIMEOUT;
> +	watchdog_init_timeout(&wdt->wdd, 0, &adev->dev);
> +	wdt_setload(&wdt->wdd, wdt->wdd.timeout);
>  
>  	ret = watchdog_register_device(&wdt->wdd);
>  	if (ret) {
> -- 
> 2.1.4
> 

Re: [PATCH 3/5] watchdog: sp805: set WDOG_HW_RUNNING when appropriate

[Ray Jui]

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

Re: [PATCH 3/5] watchdog: sp805: set WDOG_HW_RUNNING when appropriate

[Scott Branden]

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

Re: [PATCH 1/5] Documentation: DT: Add optional 'timeout-sec' property for sp805

[Robin Murphy]

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:
>   
> 

Re: [PATCH 3/5] watchdog: sp805: set WDOG_HW_RUNNING when appropriate

[Robin Murphy]

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

Re: [PATCH 1/5] Documentation: DT: Add optional 'timeout-sec' property for sp805

[Ray Jui]

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:
>>

Re: [PATCH 3/5] watchdog: sp805: set WDOG_HW_RUNNING when appropriate

[Ray Jui]

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

Re: [PATCH 3/5] watchdog: sp805: set WDOG_HW_RUNNING when appropriate

[Robin Murphy]

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.

Re: [PATCH 3/5] watchdog: sp805: set WDOG_HW_RUNNING when appropriate

[Scott Branden]

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

Re: [PATCH 3/5] watchdog: sp805: set WDOG_HW_RUNNING when appropriate

[Guenter Roeck]

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

Re: [PATCH 3/5] watchdog: sp805: set WDOG_HW_RUNNING when appropriate

[Guenter Roeck]

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.

Re: [PATCH 1/5] Documentation: DT: Add optional 'timeout-sec' property for sp805

[Guenter Roeck]

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

Re: [PATCH 1/5] Documentation: DT: Add optional 'timeout-sec' property for sp805

[Rob Herring]

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

Re: [PATCH 1/5] Documentation: DT: Add optional 'timeout-sec' property for sp805

[Ray Jui]

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
> 

Re: [PATCH 3/5] watchdog: sp805: set WDOG_HW_RUNNING when appropriate

[Ray Jui]

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.

Re: [PATCH 1/5] Documentation: DT: Add optional 'timeout-sec' property for sp805

[Robin Murphy]

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.

Re: [PATCH 1/5] Documentation: DT: Add optional 'timeout-sec' property for sp805

[Robin Murphy]

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.

Re: [PATCH 1/5] Documentation: DT: Add optional 'timeout-sec' property for sp805

[Guenter Roeck]

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