From: Stefan Christ <[email protected]>
Make the config register CONFIG_I writable to change the watchdog mode.
Signed-off-by: Stefan Christ <[email protected]>
Signed-off-by: Andrej Picej <[email protected]>
---
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
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 <[email protected]>
---
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
Hi,
On 30. 11. 21 14:42, Andrej Picej wrote:
> From: Stefan Christ <[email protected]>
>
> Make the config register CONFIG_I writable to change the watchdog mode.
>
> Signed-off-by: Stefan Christ <[email protected]>
> Signed-off-by: Andrej Picej <[email protected]>
> ---
> 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.
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> > ---
> > 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.
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 <[email protected]>
>>> ---
>>> 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
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.
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.
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
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