* [PATCH v2 2/4] watchdog: da9062: reset board on watchdog timeout
2021-11-30 13:42 [PATCH v2 1/4] mfd: da9062: make register CONFIG_I writable Andrej Picej
@ 2021-11-30 13:42 ` Andrej Picej
2021-11-30 13:54 ` Andrej Picej
2021-11-30 13:42 ` [PATCH v2 3/4] dt-bindings: watchdog: da9062: add watchdog timeout mode Andrej Picej
` (2 subsequent siblings)
3 siblings, 1 reply; 16+ messages in thread
From: Andrej Picej @ 2021-11-30 13:42 UTC (permalink / raw)
Cc: andrej.picej, support.opensource, wim, linux, linux-watchdog,
linux-kernel, robh+dt, devicetree, shawnguo, s.hauer, kernel,
festevam, linux-imx, linux-arm-kernel
Implement a method to change watchdog timeout configuration based on DT
binding ("dlg,wdt-sd"). There is a possibility to change the bahaviour
of watchdog reset. Setting WATCHDOG_SD bit enables SHUTDOWN mode, and
clearing it enables POWERDOWN mode on watchdog timeout.
If no DT binding is specified the WATCHDOG_SD bit stays in default
configuration, not breaking behaviour of devices which might depend on
default fuse configuration.
Note: This patch requires that the config register CONFIG_I is
configured as writable in the da9062 multi function device.
Signed-off-by: Andrej Picej <andrej.picej@norik.com>
---
drivers/watchdog/da9062_wdt.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/drivers/watchdog/da9062_wdt.c b/drivers/watchdog/da9062_wdt.c
index f02cbd530538..e342e9e50cb1 100644
--- a/drivers/watchdog/da9062_wdt.c
+++ b/drivers/watchdog/da9062_wdt.c
@@ -85,8 +85,33 @@ static int da9062_wdt_start(struct watchdog_device *wdd)
{
struct da9062_watchdog *wdt = watchdog_get_drvdata(wdd);
unsigned int selector;
+ unsigned int mask;
+ u32 val;
int ret;
+ /* Configure what happens on watchdog timeout. Can be specified with
+ * "dlg,wdt-sd" dt-binding (0 -> POWERDOWN, 1 -> SHUTDOWN).
+ * If "dlg,wdt-sd" dt-binding is NOT set use the default.
+ */
+ ret = device_property_read_u32(wdd->parent, "dlg,wdt-sd", &val);
+ if (!ret) {
+ if (val)
+ /* Use da9062's SHUTDOWN mode */
+ mask = DA9062AA_WATCHDOG_SD_MASK;
+ else
+ /* Use da9062's POWERDOWN mode. */
+ mask = 0x0;
+
+ ret = regmap_update_bits(wdt->hw->regmap,
+ DA9062AA_CONFIG_I,
+ DA9062AA_WATCHDOG_SD_MASK,
+ mask);
+
+ if (ret)
+ dev_err(wdt->hw->dev, "failed to set wdt reset mode: %d\n",
+ ret);
+ }
+
selector = da9062_wdt_timeout_to_sel(wdt->wdtdev.timeout);
ret = da9062_wdt_update_timeout_register(wdt, selector);
if (ret)
--
2.25.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/4] watchdog: da9062: reset board on watchdog timeout
2021-11-30 13:42 ` [PATCH v2 2/4] watchdog: da9062: reset board on watchdog timeout Andrej Picej
@ 2021-11-30 13:54 ` Andrej Picej
0 siblings, 0 replies; 16+ messages in thread
From: Andrej Picej @ 2021-11-30 13:54 UTC (permalink / raw)
Cc: support.opensource, wim, linux, linux-watchdog, linux-kernel,
robh+dt, devicetree, shawnguo, s.hauer, kernel, festevam,
linux-imx, linux-arm-kernel
On 30. 11. 21 14:42, Andrej Picej wrote:
> Implement a method to change watchdog timeout configuration based on DT
> binding ("dlg,wdt-sd"). There is a possibility to change the bahaviour
> of watchdog reset. Setting WATCHDOG_SD bit enables SHUTDOWN mode, and
> clearing it enables POWERDOWN mode on watchdog timeout.
>
> If no DT binding is specified the WATCHDOG_SD bit stays in default
> configuration, not breaking behaviour of devices which might depend on
> default fuse configuration.
>
> Note: This patch requires that the config register CONFIG_I is
> configured as writable in the da9062 multi function device.
>
> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
> ---
> drivers/watchdog/da9062_wdt.c | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/drivers/watchdog/da9062_wdt.c b/drivers/watchdog/da9062_wdt.c
> index f02cbd530538..e342e9e50cb1 100644
> --- a/drivers/watchdog/da9062_wdt.c
> +++ b/drivers/watchdog/da9062_wdt.c
> @@ -85,8 +85,33 @@ static int da9062_wdt_start(struct watchdog_device *wdd)
> {
> struct da9062_watchdog *wdt = watchdog_get_drvdata(wdd);
> unsigned int selector;
> + unsigned int mask;
> + u32 val;
> int ret;
>
> + /* Configure what happens on watchdog timeout. Can be specified with
> + * "dlg,wdt-sd" dt-binding (0 -> POWERDOWN, 1 -> SHUTDOWN).
> + * If "dlg,wdt-sd" dt-binding is NOT set use the default.
> + */
> + ret = device_property_read_u32(wdd->parent, "dlg,wdt-sd", &val);
> + if (!ret) {
> + if (val)
> + /* Use da9062's SHUTDOWN mode */
> + mask = DA9062AA_WATCHDOG_SD_MASK;
> + else
> + /* Use da9062's POWERDOWN mode. */
> + mask = 0x0;
> +
> + ret = regmap_update_bits(wdt->hw->regmap,
> + DA9062AA_CONFIG_I,
> + DA9062AA_WATCHDOG_SD_MASK,
> + mask);
> +
> + if (ret)
> + dev_err(wdt->hw->dev, "failed to set wdt reset mode: %d\n",
> + ret);
> + }
> +
> selector = da9062_wdt_timeout_to_sel(wdt->wdtdev.timeout);
> ret = da9062_wdt_update_timeout_register(wdt, selector);
> if (ret)
>
Changes in v2:
- don't force the "reset" for all da9062-watchdog users, instead add DT
binding where the behavior can be selected
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 3/4] dt-bindings: watchdog: da9062: add watchdog timeout mode
2021-11-30 13:42 [PATCH v2 1/4] mfd: da9062: make register CONFIG_I writable Andrej Picej
2021-11-30 13:42 ` [PATCH v2 2/4] watchdog: da9062: reset board on watchdog timeout Andrej Picej
@ 2021-11-30 13:42 ` Andrej Picej
2021-11-30 13:54 ` Andrej Picej
2021-11-30 14:32 ` Guenter Roeck
2021-11-30 13:42 ` [PATCH v2 4/4] ARM: dts: imx6: phycore-som: set watchdog timeout mode to shutdown Andrej Picej
2021-11-30 13:49 ` [PATCH v2 1/4] mfd: da9062: make register CONFIG_I writable Andrej Picej
3 siblings, 2 replies; 16+ messages in thread
From: Andrej Picej @ 2021-11-30 13:42 UTC (permalink / raw)
Cc: andrej.picej, support.opensource, wim, linux, linux-watchdog,
linux-kernel, robh+dt, devicetree, shawnguo, s.hauer, kernel,
festevam, linux-imx, linux-arm-kernel
Document the watchdog timeout mode property. If this property is used
the user can select what happens on watchdog timeout. Set this property
to 1 to enable SHUTDOWN (the device resets), set it to 0 and the device
will go to POWERDOWN on watchdog timeout.
Signed-off-by: Andrej Picej <andrej.picej@norik.com>
---
Documentation/devicetree/bindings/watchdog/da9062-wdt.txt | 3 +++
1 file changed, 3 insertions(+)
diff --git a/Documentation/devicetree/bindings/watchdog/da9062-wdt.txt b/Documentation/devicetree/bindings/watchdog/da9062-wdt.txt
index 950e4fba8dbc..e3e6e56cee21 100644
--- a/Documentation/devicetree/bindings/watchdog/da9062-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/da9062-wdt.txt
@@ -10,6 +10,9 @@ Optional properties:
- dlg,use-sw-pm: Add this property to disable the watchdog during suspend.
Only use this option if you can't use the watchdog automatic suspend
function during a suspend (see register CONTROL_B).
+- dlg,wdt-sd: Set what happens on watchdog timeout. If this bit is set the
+ watchdog timeout triggers SHUTDOWN, if cleared the watchdog triggers
+ POWERDOWN. Can be 0 or 1.
Example: DA9062
--
2.25.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/4] dt-bindings: watchdog: da9062: add watchdog timeout mode
2021-11-30 13:42 ` [PATCH v2 3/4] dt-bindings: watchdog: da9062: add watchdog timeout mode Andrej Picej
@ 2021-11-30 13:54 ` Andrej Picej
2021-11-30 14:32 ` Guenter Roeck
1 sibling, 0 replies; 16+ messages in thread
From: Andrej Picej @ 2021-11-30 13:54 UTC (permalink / raw)
Cc: support.opensource, wim, linux, linux-watchdog, linux-kernel,
robh+dt, devicetree, shawnguo, s.hauer, kernel, festevam,
linux-imx, linux-arm-kernel
On 30. 11. 21 14:42, Andrej Picej wrote:
> Document the watchdog timeout mode property. If this property is used
> the user can select what happens on watchdog timeout. Set this property
> to 1 to enable SHUTDOWN (the device resets), set it to 0 and the device
> will go to POWERDOWN on watchdog timeout.
>
> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
> ---
> Documentation/devicetree/bindings/watchdog/da9062-wdt.txt | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/watchdog/da9062-wdt.txt b/Documentation/devicetree/bindings/watchdog/da9062-wdt.txt
> index 950e4fba8dbc..e3e6e56cee21 100644
> --- a/Documentation/devicetree/bindings/watchdog/da9062-wdt.txt
> +++ b/Documentation/devicetree/bindings/watchdog/da9062-wdt.txt
> @@ -10,6 +10,9 @@ Optional properties:
> - dlg,use-sw-pm: Add this property to disable the watchdog during suspend.
> Only use this option if you can't use the watchdog automatic suspend
> function during a suspend (see register CONTROL_B).
> +- dlg,wdt-sd: Set what happens on watchdog timeout. If this bit is set the
> + watchdog timeout triggers SHUTDOWN, if cleared the watchdog triggers
> + POWERDOWN. Can be 0 or 1.
>
> Example: DA9062
>
>
Changes in v2:
- new patch, document new DT binding
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/4] dt-bindings: watchdog: da9062: add watchdog timeout mode
2021-11-30 13:42 ` [PATCH v2 3/4] dt-bindings: watchdog: da9062: add watchdog timeout mode Andrej Picej
2021-11-30 13:54 ` Andrej Picej
@ 2021-11-30 14:32 ` Guenter Roeck
2021-11-30 16:11 ` Adam Thomson
1 sibling, 1 reply; 16+ messages in thread
From: Guenter Roeck @ 2021-11-30 14:32 UTC (permalink / raw)
To: Andrej Picej
Cc: support.opensource, wim, linux-watchdog, linux-kernel, robh+dt,
devicetree, shawnguo, s.hauer, kernel, festevam, linux-imx,
linux-arm-kernel
On 11/30/21 5:42 AM, Andrej Picej wrote:
> Document the watchdog timeout mode property. If this property is used
> the user can select what happens on watchdog timeout. Set this property
> to 1 to enable SHUTDOWN (the device resets), set it to 0 and the device
> will go to POWERDOWN on watchdog timeout.
>
> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
> ---
> Documentation/devicetree/bindings/watchdog/da9062-wdt.txt | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/watchdog/da9062-wdt.txt b/Documentation/devicetree/bindings/watchdog/da9062-wdt.txt
> index 950e4fba8dbc..e3e6e56cee21 100644
> --- a/Documentation/devicetree/bindings/watchdog/da9062-wdt.txt
> +++ b/Documentation/devicetree/bindings/watchdog/da9062-wdt.txt
> @@ -10,6 +10,9 @@ Optional properties:
> - dlg,use-sw-pm: Add this property to disable the watchdog during suspend.
> Only use this option if you can't use the watchdog automatic suspend
> function during a suspend (see register CONTROL_B).
> +- dlg,wdt-sd: Set what happens on watchdog timeout. If this bit is set the
> + watchdog timeout triggers SHUTDOWN, if cleared the watchdog triggers
> + POWERDOWN. Can be 0 or 1.
>
Why does it need a value ? Why not just bool ?
Guenter
^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH v2 3/4] dt-bindings: watchdog: da9062: add watchdog timeout mode
2021-11-30 14:32 ` Guenter Roeck
@ 2021-11-30 16:11 ` Adam Thomson
2021-11-30 16:40 ` Guenter Roeck
0 siblings, 1 reply; 16+ messages in thread
From: Adam Thomson @ 2021-11-30 16:11 UTC (permalink / raw)
To: Guenter Roeck, Andrej Picej
Cc: Support Opensource, wim, linux-watchdog, linux-kernel, robh+dt,
devicetree, shawnguo, s.hauer, kernel, festevam, linux-imx,
linux-arm-kernel
On Guenter Roeck wrote:
> > Document the watchdog timeout mode property. If this property is used
> > the user can select what happens on watchdog timeout. Set this property
> > to 1 to enable SHUTDOWN (the device resets), set it to 0 and the device
> > will go to POWERDOWN on watchdog timeout.
> >
> > Signed-off-by: Andrej Picej <andrej.picej@norik.com>
> > ---
> > Documentation/devicetree/bindings/watchdog/da9062-wdt.txt | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/watchdog/da9062-wdt.txt
> b/Documentation/devicetree/bindings/watchdog/da9062-wdt.txt
> > index 950e4fba8dbc..e3e6e56cee21 100644
> > --- a/Documentation/devicetree/bindings/watchdog/da9062-wdt.txt
> > +++ b/Documentation/devicetree/bindings/watchdog/da9062-wdt.txt
> > @@ -10,6 +10,9 @@ Optional properties:
> > - dlg,use-sw-pm: Add this property to disable the watchdog during suspend.
> > Only use this option if you can't use the watchdog automatic suspend
> > function during a suspend (see register CONTROL_B).
> > +- dlg,wdt-sd: Set what happens on watchdog timeout. If this bit is set the
> > + watchdog timeout triggers SHUTDOWN, if cleared the watchdog triggers
> > + POWERDOWN. Can be 0 or 1.
> >
>
> Why does it need a value ? Why not just bool ?
One argument might be that if the property isn't provided then the OTP
configured value can persist without needing a FW change around this DT binding.
My belief though is that the majority of users would have this property set to 0
by default in OTP, so a boolean would be OK I think here to enable watchdog
shutdown.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/4] dt-bindings: watchdog: da9062: add watchdog timeout mode
2021-11-30 16:11 ` Adam Thomson
@ 2021-11-30 16:40 ` Guenter Roeck
2021-11-30 17:46 ` Adam Thomson
0 siblings, 1 reply; 16+ messages in thread
From: Guenter Roeck @ 2021-11-30 16:40 UTC (permalink / raw)
To: Adam Thomson, Andrej Picej
Cc: Support Opensource, wim, linux-watchdog, linux-kernel, robh+dt,
devicetree, shawnguo, s.hauer, kernel, festevam, linux-imx,
linux-arm-kernel
On 11/30/21 8:11 AM, Adam Thomson wrote:
> On Guenter Roeck wrote:
>
>>> Document the watchdog timeout mode property. If this property is used
>>> the user can select what happens on watchdog timeout. Set this property
>>> to 1 to enable SHUTDOWN (the device resets), set it to 0 and the device
>>> will go to POWERDOWN on watchdog timeout.
>>>
>>> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
>>> ---
>>> Documentation/devicetree/bindings/watchdog/da9062-wdt.txt | 3 +++
>>> 1 file changed, 3 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/watchdog/da9062-wdt.txt
>> b/Documentation/devicetree/bindings/watchdog/da9062-wdt.txt
>>> index 950e4fba8dbc..e3e6e56cee21 100644
>>> --- a/Documentation/devicetree/bindings/watchdog/da9062-wdt.txt
>>> +++ b/Documentation/devicetree/bindings/watchdog/da9062-wdt.txt
>>> @@ -10,6 +10,9 @@ Optional properties:
>>> - dlg,use-sw-pm: Add this property to disable the watchdog during suspend.
>>> Only use this option if you can't use the watchdog automatic suspend
>>> function during a suspend (see register CONTROL_B).
>>> +- dlg,wdt-sd: Set what happens on watchdog timeout. If this bit is set the
>>> + watchdog timeout triggers SHUTDOWN, if cleared the watchdog triggers
>>> + POWERDOWN. Can be 0 or 1.
>>>
>>
>> Why does it need a value ? Why not just bool ?
>
> One argument might be that if the property isn't provided then the OTP
> configured value can persist without needing a FW change around this DT binding.
>
> My belief though is that the majority of users would have this property set to 0
> by default in OTP, so a boolean would be OK I think here to enable watchdog
> shutdown.
>
Sorry, you lost me.
dlg,wdt-sd = <0>;
is the current situation, and identical to not having the property in
the first place.
dlg,wdt-sd = <1>;
is new. I don't see the difference to
dlg,wdt-sd;
vs. not having the property at all (which is, again, the current situation).
Since it has to be backward compatible,
dlg,wdt-sd = <0>;
will always be identical to not having the property at all.
I can not find a situation where an integer would have any benefits over a boolean.
Guenter
^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH v2 3/4] dt-bindings: watchdog: da9062: add watchdog timeout mode
2021-11-30 16:40 ` Guenter Roeck
@ 2021-11-30 17:46 ` Adam Thomson
2021-12-01 6:42 ` Andrej Picej
0 siblings, 1 reply; 16+ messages in thread
From: Adam Thomson @ 2021-11-30 17:46 UTC (permalink / raw)
To: Guenter Roeck, Adam Thomson, Andrej Picej
Cc: Support Opensource, wim, linux-watchdog, linux-kernel, robh+dt,
devicetree, shawnguo, s.hauer, kernel, festevam, linux-imx,
linux-arm-kernel
On 30 November 2021 16:40, Guenter Roeck wrote:
> >> Why does it need a value ? Why not just bool ?
> >
> > One argument might be that if the property isn't provided then the OTP
> > configured value can persist without needing a FW change around this DT
> binding.
> >
> > My belief though is that the majority of users would have this property set to 0
> > by default in OTP, so a boolean would be OK I think here to enable watchdog
> > shutdown.
> >
>
> Sorry, you lost me.
> dlg,wdt-sd = <0>;
> is the current situation, and identical to not having the property in
> the first place.
> dlg,wdt-sd = <1>;
> is new. I don't see the difference to
> dlg,wdt-sd;
> vs. not having the property at all (which is, again, the current situation).
> Since it has to be backward compatible,
> dlg,wdt-sd = <0>;
> will always be identical to not having the property at all.
> I can not find a situation where an integer would have any benefits over a
> boolean.
So if you have a binary DT binding, it's either there or it isn't which implies
the bit to be set to 0/1 in this case. If you have a binding which has a value,
there can be 3 outcomes in this discussion:
1) Binding = 0, bit is set to 0
2) Binding = 1, bit is set to 1
3) Binding NOT present in DT, OTP default value in HW remains untouched
Say a platform updates to a later kernel version, but sticks with existing DT
FW (i.e. the new boolean binding isn't present in FW), then the following could
happen:
1) OTP for DA9061/2 has this bit set to 1, system expectation is that watchdog
triggers SHUTDOWN.
2) New driver checks existance of 'dlg,wdt-sd' but it's obviously not there so
assumes the bit should be set to 0 and does so
3) When the watchdog fires, it will no longer trigger SHUTDOWN but instead
POWER-DOWN due to binary handling of new boolean binding.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/4] dt-bindings: watchdog: da9062: add watchdog timeout mode
2021-11-30 17:46 ` Adam Thomson
@ 2021-12-01 6:42 ` Andrej Picej
2021-12-01 7:01 ` Guenter Roeck
0 siblings, 1 reply; 16+ messages in thread
From: Andrej Picej @ 2021-12-01 6:42 UTC (permalink / raw)
To: Adam Thomson, Guenter Roeck
Cc: Support Opensource, wim, linux-watchdog, linux-kernel, robh+dt,
devicetree, shawnguo, s.hauer, kernel, festevam, linux-imx,
linux-arm-kernel
On 30. 11. 21 18:46, Adam Thomson wrote:
> On 30 November 2021 16:40, Guenter Roeck wrote:
>
>>>> Why does it need a value ? Why not just bool ?
>>>
>>> One argument might be that if the property isn't provided then the OTP
>>> configured value can persist without needing a FW change around this DT
>> binding.
>>>
>>> My belief though is that the majority of users would have this property set to 0
>>> by default in OTP, so a boolean would be OK I think here to enable watchdog
>>> shutdown.
>>>
>>
>> Sorry, you lost me.
>> dlg,wdt-sd = <0>;
>> is the current situation, and identical to not having the property in
>> the first place.
>> dlg,wdt-sd = <1>;
>> is new. I don't see the difference to
>> dlg,wdt-sd;
>> vs. not having the property at all (which is, again, the current situation).
>> Since it has to be backward compatible,
>> dlg,wdt-sd = <0>;
>> will always be identical to not having the property at all.
>> I can not find a situation where an integer would have any benefits over a
>> boolean.
>
> So if you have a binary DT binding, it's either there or it isn't which implies
> the bit to be set to 0/1 in this case. If you have a binding which has a value,
> there can be 3 outcomes in this discussion:
>
> 1) Binding = 0, bit is set to 0
> 2) Binding = 1, bit is set to 1
> 3) Binding NOT present in DT, OTP default value in HW remains untouched
>
> Say a platform updates to a later kernel version, but sticks with existing DT
> FW (i.e. the new boolean binding isn't present in FW), then the following could
> happen:
>
> 1) OTP for DA9061/2 has this bit set to 1, system expectation is that watchdog
> triggers SHUTDOWN.
> 2) New driver checks existance of 'dlg,wdt-sd' but it's obviously not there so
> assumes the bit should be set to 0 and does so
> 3) When the watchdog fires, it will no longer trigger SHUTDOWN but instead
> POWER-DOWN due to binary handling of new boolean binding.
>
This was my thinking exactly. I also first thought about boolean value,
but I then moved to the integer value of 0 or 1 after checking the OTP
default for this bit. The da9062 I'm working with has the bit set to 1
by default.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/4] dt-bindings: watchdog: da9062: add watchdog timeout mode
2021-12-01 6:42 ` Andrej Picej
@ 2021-12-01 7:01 ` Guenter Roeck
2021-12-01 7:05 ` Andrej Picej
0 siblings, 1 reply; 16+ messages in thread
From: Guenter Roeck @ 2021-12-01 7:01 UTC (permalink / raw)
To: Andrej Picej, Adam Thomson
Cc: Support Opensource, wim, linux-watchdog, linux-kernel, robh+dt,
devicetree, shawnguo, s.hauer, kernel, festevam, linux-imx,
linux-arm-kernel
On 11/30/21 10:42 PM, Andrej Picej wrote:
>
> On 30. 11. 21 18:46, Adam Thomson wrote:
>> On 30 November 2021 16:40, Guenter Roeck wrote:
>>
>>>>> Why does it need a value ? Why not just bool ?
>>>>
>>>> One argument might be that if the property isn't provided then the OTP
>>>> configured value can persist without needing a FW change around this DT
>>> binding.
>>>>
>>>> My belief though is that the majority of users would have this property set to 0
>>>> by default in OTP, so a boolean would be OK I think here to enable watchdog
>>>> shutdown.
>>>>
>>>
>>> Sorry, you lost me.
>>> dlg,wdt-sd = <0>;
>>> is the current situation, and identical to not having the property in
>>> the first place.
>>> dlg,wdt-sd = <1>;
>>> is new. I don't see the difference to
>>> dlg,wdt-sd;
>>> vs. not having the property at all (which is, again, the current situation).
>>> Since it has to be backward compatible,
>>> dlg,wdt-sd = <0>;
>>> will always be identical to not having the property at all.
>>> I can not find a situation where an integer would have any benefits over a
>>> boolean.
>>
>> So if you have a binary DT binding, it's either there or it isn't which implies
>> the bit to be set to 0/1 in this case. If you have a binding which has a value,
>> there can be 3 outcomes in this discussion:
>>
>> 1) Binding = 0, bit is set to 0
>> 2) Binding = 1, bit is set to 1
>> 3) Binding NOT present in DT, OTP default value in HW remains untouched
>>
>> Say a platform updates to a later kernel version, but sticks with existing DT
>> FW (i.e. the new boolean binding isn't present in FW), then the following could
>> happen:
>>
>> 1) OTP for DA9061/2 has this bit set to 1, system expectation is that watchdog
>> triggers SHUTDOWN.
>> 2) New driver checks existance of 'dlg,wdt-sd' but it's obviously not there so
>> assumes the bit should be set to 0 and does so
>> 3) When the watchdog fires, it will no longer trigger SHUTDOWN but instead
>> POWER-DOWN due to binary handling of new boolean binding.
>>
>
> This was my thinking exactly. I also first thought about boolean value, but I then moved to the integer value of 0 or 1 after checking the OTP default for this bit. The da9062 I'm working with has the bit set to 1 by default.
That needs to be be documented.
Guenter
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/4] dt-bindings: watchdog: da9062: add watchdog timeout mode
2021-12-01 7:01 ` Guenter Roeck
@ 2021-12-01 7:05 ` Andrej Picej
0 siblings, 0 replies; 16+ messages in thread
From: Andrej Picej @ 2021-12-01 7:05 UTC (permalink / raw)
To: Guenter Roeck, Adam Thomson
Cc: Support Opensource, wim, linux-watchdog, linux-kernel, robh+dt,
devicetree, shawnguo, s.hauer, kernel, festevam, linux-imx,
linux-arm-kernel
On 1. 12. 21 08:01, Guenter Roeck wrote:
> On 11/30/21 10:42 PM, Andrej Picej wrote:
>>
>> On 30. 11. 21 18:46, Adam Thomson wrote:
>>> On 30 November 2021 16:40, Guenter Roeck wrote:
>>>
>>>>>> Why does it need a value ? Why not just bool ?
>>>>>
>>>>> One argument might be that if the property isn't provided then the OTP
>>>>> configured value can persist without needing a FW change around
>>>>> this DT
>>>> binding.
>>>>>
>>>>> My belief though is that the majority of users would have this
>>>>> property set to 0
>>>>> by default in OTP, so a boolean would be OK I think here to enable
>>>>> watchdog
>>>>> shutdown.
>>>>>
>>>>
>>>> Sorry, you lost me.
>>>> dlg,wdt-sd = <0>;
>>>> is the current situation, and identical to not having the property in
>>>> the first place.
>>>> dlg,wdt-sd = <1>;
>>>> is new. I don't see the difference to
>>>> dlg,wdt-sd;
>>>> vs. not having the property at all (which is, again, the current
>>>> situation).
>>>> Since it has to be backward compatible,
>>>> dlg,wdt-sd = <0>;
>>>> will always be identical to not having the property at all.
>>>> I can not find a situation where an integer would have any benefits
>>>> over a
>>>> boolean.
>>>
>>> So if you have a binary DT binding, it's either there or it isn't
>>> which implies
>>> the bit to be set to 0/1 in this case. If you have a binding which
>>> has a value,
>>> there can be 3 outcomes in this discussion:
>>>
>>> 1) Binding = 0, bit is set to 0
>>> 2) Binding = 1, bit is set to 1
>>> 3) Binding NOT present in DT, OTP default value in HW remains
>>> untouched
>>>
>>> Say a platform updates to a later kernel version, but sticks with
>>> existing DT
>>> FW (i.e. the new boolean binding isn't present in FW), then the
>>> following could
>>> happen:
>>>
>>> 1) OTP for DA9061/2 has this bit set to 1, system expectation is
>>> that watchdog
>>> triggers SHUTDOWN.
>>> 2) New driver checks existance of 'dlg,wdt-sd' but it's obviously
>>> not there so
>>> assumes the bit should be set to 0 and does so
>>> 3) When the watchdog fires, it will no longer trigger SHUTDOWN but
>>> instead
>>> POWER-DOWN due to binary handling of new boolean binding.
>>>
>>
>> This was my thinking exactly. I also first thought about boolean
>> value, but I then moved to the integer value of 0 or 1 after checking
>> the OTP default for this bit. The da9062 I'm working with has the bit
>> set to 1 by default.
>
> That needs to be be documented.
>
> Guenter
Ok, I will add a note about the default value and how it is defined by
OTP. Will submit a v3.
Thanks for review.
BR,
Andrej
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 4/4] ARM: dts: imx6: phycore-som: set watchdog timeout mode to shutdown
2021-11-30 13:42 [PATCH v2 1/4] mfd: da9062: make register CONFIG_I writable Andrej Picej
2021-11-30 13:42 ` [PATCH v2 2/4] watchdog: da9062: reset board on watchdog timeout Andrej Picej
2021-11-30 13:42 ` [PATCH v2 3/4] dt-bindings: watchdog: da9062: add watchdog timeout mode Andrej Picej
@ 2021-11-30 13:42 ` Andrej Picej
2021-11-30 13:56 ` Andrej Picej
2021-11-30 14:31 ` Guenter Roeck
2021-11-30 13:49 ` [PATCH v2 1/4] mfd: da9062: make register CONFIG_I writable Andrej Picej
3 siblings, 2 replies; 16+ messages in thread
From: Andrej Picej @ 2021-11-30 13:42 UTC (permalink / raw)
Cc: andrej.picej, support.opensource, wim, linux, linux-watchdog,
linux-kernel, robh+dt, devicetree, shawnguo, s.hauer, kernel,
festevam, linux-imx, linux-arm-kernel
Enable system restart when the watchdog timeout occurs.
Signed-off-by: Andrej Picej <andrej.picej@norik.com>
---
arch/arm/boot/dts/imx6qdl-phytec-phycore-som.dtsi | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/boot/dts/imx6qdl-phytec-phycore-som.dtsi b/arch/arm/boot/dts/imx6qdl-phytec-phycore-som.dtsi
index a80aa08a37cb..743343e525cf 100644
--- a/arch/arm/boot/dts/imx6qdl-phytec-phycore-som.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-phytec-phycore-som.dtsi
@@ -111,6 +111,7 @@ da9062_onkey: onkey {
watchdog {
compatible = "dlg,da9062-watchdog";
dlg,use-sw-pm;
+ dlg,wdt-sd = <1>;
};
regulators {
--
2.25.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 4/4] ARM: dts: imx6: phycore-som: set watchdog timeout mode to shutdown
2021-11-30 13:42 ` [PATCH v2 4/4] ARM: dts: imx6: phycore-som: set watchdog timeout mode to shutdown Andrej Picej
@ 2021-11-30 13:56 ` Andrej Picej
2021-11-30 14:31 ` Guenter Roeck
1 sibling, 0 replies; 16+ messages in thread
From: Andrej Picej @ 2021-11-30 13:56 UTC (permalink / raw)
Cc: support.opensource, wim, linux, linux-watchdog, linux-kernel,
robh+dt, devicetree, shawnguo, s.hauer, kernel, festevam,
linux-imx, linux-arm-kernel
On 30. 11. 21 14:42, Andrej Picej wrote:
> Enable system restart when the watchdog timeout occurs.
>
> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
> ---
> arch/arm/boot/dts/imx6qdl-phytec-phycore-som.dtsi | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm/boot/dts/imx6qdl-phytec-phycore-som.dtsi b/arch/arm/boot/dts/imx6qdl-phytec-phycore-som.dtsi
> index a80aa08a37cb..743343e525cf 100644
> --- a/arch/arm/boot/dts/imx6qdl-phytec-phycore-som.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-phytec-phycore-som.dtsi
> @@ -111,6 +111,7 @@ da9062_onkey: onkey {
> watchdog {
> compatible = "dlg,da9062-watchdog";
> dlg,use-sw-pm;
> + dlg,wdt-sd = <1>;
> };
>
> regulators {
>
Changes in v2:
- new patch, enable shutdown mode for phytec-phycore (da9062 user)
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 4/4] ARM: dts: imx6: phycore-som: set watchdog timeout mode to shutdown
2021-11-30 13:42 ` [PATCH v2 4/4] ARM: dts: imx6: phycore-som: set watchdog timeout mode to shutdown Andrej Picej
2021-11-30 13:56 ` Andrej Picej
@ 2021-11-30 14:31 ` Guenter Roeck
1 sibling, 0 replies; 16+ messages in thread
From: Guenter Roeck @ 2021-11-30 14:31 UTC (permalink / raw)
To: Andrej Picej
Cc: support.opensource, wim, linux-watchdog, linux-kernel, robh+dt,
devicetree, shawnguo, s.hauer, kernel, festevam, linux-imx,
linux-arm-kernel
On 11/30/21 5:42 AM, Andrej Picej wrote:
> Enable system restart when the watchdog timeout occurs.
>
> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
> ---
> arch/arm/boot/dts/imx6qdl-phytec-phycore-som.dtsi | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm/boot/dts/imx6qdl-phytec-phycore-som.dtsi b/arch/arm/boot/dts/imx6qdl-phytec-phycore-som.dtsi
> index a80aa08a37cb..743343e525cf 100644
> --- a/arch/arm/boot/dts/imx6qdl-phytec-phycore-som.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-phytec-phycore-som.dtsi
> @@ -111,6 +111,7 @@ da9062_onkey: onkey {
> watchdog {
> compatible = "dlg,da9062-watchdog";
> dlg,use-sw-pm;
> + dlg,wdt-sd = <1>;
dlg,wdt-sd should be defined as bool.
Guenter
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/4] mfd: da9062: make register CONFIG_I writable
2021-11-30 13:42 [PATCH v2 1/4] mfd: da9062: make register CONFIG_I writable Andrej Picej
` (2 preceding siblings ...)
2021-11-30 13:42 ` [PATCH v2 4/4] ARM: dts: imx6: phycore-som: set watchdog timeout mode to shutdown Andrej Picej
@ 2021-11-30 13:49 ` Andrej Picej
3 siblings, 0 replies; 16+ messages in thread
From: Andrej Picej @ 2021-11-30 13:49 UTC (permalink / raw)
Cc: support.opensource, wim, linux, linux-watchdog, linux-kernel,
robh+dt, devicetree, shawnguo, s.hauer, kernel, festevam,
linux-imx, linux-arm-kernel
Hi,
On 30. 11. 21 14:42, Andrej Picej wrote:
> From: Stefan Christ <s.christ@phytec.de>
>
> Make the config register CONFIG_I writable to change the watchdog mode.
>
> Signed-off-by: Stefan Christ <s.christ@phytec.de>
> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
> ---
> drivers/mfd/da9062-core.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/mfd/da9062-core.c b/drivers/mfd/da9062-core.c
> index 01f8e10dfa55..7041ba53efb4 100644
> --- a/drivers/mfd/da9062-core.c
> +++ b/drivers/mfd/da9062-core.c
> @@ -556,6 +556,7 @@ static const struct regmap_range da9062_aa_writeable_ranges[] = {
> regmap_reg_range(DA9062AA_VBUCK3_B, DA9062AA_VBUCK3_B),
> regmap_reg_range(DA9062AA_VLDO1_B, DA9062AA_VLDO4_B),
> regmap_reg_range(DA9062AA_BBAT_CONT, DA9062AA_BBAT_CONT),
> + regmap_reg_range(DA9062AA_CONFIG_I, DA9062AA_CONFIG_I),
> regmap_reg_range(DA9062AA_GP_ID_0, DA9062AA_GP_ID_19),
> };
>
>
Changes in v2:
- no changes
sorry, just noticed I forgot to add what I changed for v2 patch series.
Should I resend the patches with that fixed/added? Should that be v3
then or still v2?
Thanks.
^ permalink raw reply [flat|nested] 16+ messages in thread