[PATCH v2 1/4] mfd: da9062: make register CONFIG_I writable

[PATCH v2 1/4] mfd: da9062: make register CONFIG_I writable

[Andrej Picej]

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),
 };
 
-- 
2.25.1

[PATCH v2 2/4] watchdog: da9062: reset board on watchdog timeout

[Andrej Picej]

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

[PATCH v2 3/4] dt-bindings: watchdog: da9062: add watchdog timeout mode

[Andrej Picej]

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

[PATCH v2 4/4] ARM: dts: imx6: phycore-som: set watchdog timeout mode to shutdown

[Andrej Picej]

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

Re: [PATCH v2 1/4] mfd: da9062: make register CONFIG_I writable

[Andrej Picej]

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.

Re: [PATCH v2 2/4] watchdog: da9062: reset board on watchdog timeout

[Andrej Picej]

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

Re: [PATCH v2 3/4] dt-bindings: watchdog: da9062: add watchdog timeout mode

[Andrej Picej]

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

Re: [PATCH v2 4/4] ARM: dts: imx6: phycore-som: set watchdog timeout mode to shutdown

[Andrej Picej]

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)

Re: [PATCH v2 4/4] ARM: dts: imx6: phycore-som: set watchdog timeout mode to shutdown

[Guenter Roeck]

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

Re: [PATCH v2 3/4] dt-bindings: watchdog: da9062: add watchdog timeout mode

[Guenter Roeck]

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

RE: [PATCH v2 3/4] dt-bindings: watchdog: da9062: add watchdog timeout mode

[Adam Thomson]

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.

Re: [PATCH v2 3/4] dt-bindings: watchdog: da9062: add watchdog timeout mode

[Guenter Roeck]

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

RE: [PATCH v2 3/4] dt-bindings: watchdog: da9062: add watchdog timeout mode

[Adam Thomson]

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.

Re: [PATCH v2 3/4] dt-bindings: watchdog: da9062: add watchdog timeout mode

[Andrej Picej]

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.

Re: [PATCH v2 3/4] dt-bindings: watchdog: da9062: add watchdog timeout mode

[Guenter Roeck]

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

Re: [PATCH v2 3/4] dt-bindings: watchdog: da9062: add watchdog timeout mode

[Andrej Picej]

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