2021-12-01 08:15:40

by Andrej Picej

[permalink] [raw]
Subject: [PATCH v3 2/4] watchdog: da9062: reset board on watchdog timeout

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 <[email protected]>
---
Changes in v3:
- no changes

Changes in v2:
- don't force the "reset" for all da9062-watchdog users, instead add DT
binding where the behavior can be selected
---
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



2021-12-01 20:07:49

by Adam Thomson

[permalink] [raw]
Subject: RE: [PATCH v3 2/4] watchdog: da9062: reset board on watchdog timeout

On 01 December 2021 08:15, 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 <[email protected]>
> ---
> Changes in v3:
> - no changes
>
> Changes in v2:
> - don't force the "reset" for all da9062-watchdog users, instead add DT
> binding where the behavior can be selected
> ---
> 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);
> + }
> +

Personally I'd stick this code in the probe(). The value won't change once it's
set, and that seems the more logical place for it in my opinion.

> selector = da9062_wdt_timeout_to_sel(wdt->wdtdev.timeout);
> ret = da9062_wdt_update_timeout_register(wdt, selector);
> if (ret)
> --
> 2.25.1

2021-12-01 21:27:38

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH v3 2/4] watchdog: da9062: reset board on watchdog timeout

On 12/1/21 12:15 AM, 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 <[email protected]>
> ---
> Changes in v3:
> - no changes
>
> Changes in v2:
> - don't force the "reset" for all da9062-watchdog users, instead add DT
> binding where the behavior can be selected
> ---
> 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.
> + */

Please use standard multi-line comments. This is not the networking
subsystem.

Also, if you think this code should be here and not in the probe function,
as suggested by Adam, please provide a rationale.

Thanks,
Guenter

> + 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)
>


2021-12-02 07:43:32

by Andrej Picej

[permalink] [raw]
Subject: Re: [PATCH v3 2/4] watchdog: da9062: reset board on watchdog timeout

Hi Christoph,

On 1. 12. 21 15:11, Christoph Niedermaier wrote:
> From: Andrej Picej
> Sent: Wednesday, December 1, 2021 9:15 AM
>> 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 <[email protected]>
>> ---
>> Changes in v3:
>> - no changes
>>
>> Changes in v2:
>> - don't force the "reset" for all da9062-watchdog users, instead add DT
>> binding where the behavior can be selected
>> ---
>> 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
>>
>
> I have a question how to correctly restart the system after
> watchdog timeout.
> If I understand it correct after watchdog timeout the system
> restarts only if WATCHDOG_SD (Bit 3) in register CONFIG_I is
> set.
> What is the difference if WATCHDOG_SD isn't set, but WAKE_UP
> (Bit 2) in register CONTROL_F is set? From outside on my
> system I observe the same behavior. After watchdog timeout
> my system restarts. So where are the differences?
> It would be nice if you could answer this question, as you
> certainly know this chip very well.

To be honest I don't really know the chip that well, I'm just trying to
add this feature and hopefully help others if they run into the same
problem. I think @Adam will be more helpful here.

But from quick look at da9062 datasheet, mainly chapter "8.8 Power
Modes" I see next main differences:
- setting WATCHDOG_SD enables SHUTDOWN sequence when the watchdog
timeout is triggered. This puts the chip (da9062) in RESET mode.
Taken from DA9062 datasheet:
> In RESET mode, the internal supplies, and LDO1 (if configured as an always-on supply) are enabled.
> All other DA9062 supplies are disabled.
> DA9062 is in RESET mode whenever a complete application shutdown is required
> The DA9062’s register configuration will be re-loaded from OTP when leaving the RESET mode

- if you set the WAKE_UP bit than the chip enters POWERDOWN mode on
watchdog timeout. I understand the POWERDOWN mode as a not that "deep"
mode as a RESET mode Device will go from RESET mode to POWERDOWN mode in
the sequence of powering-up.

The above explanation is just my understanding after quick look, @Adam
please correct me if I'm talking nonsense.

Please have a look at the DA9062 datasheet for more information. Sorry,
that I can't be more helpful here.

Best regards,
Andrej

2021-12-02 08:31:47

by Andrej Picej

[permalink] [raw]
Subject: Re: [PATCH v3 2/4] watchdog: da9062: reset board on watchdog timeout


>
> Personally I'd stick this code in the probe(). The value won't change once it's
> set, and that seems the more logical place for it in my opinion.
>
I think that's a good idea and I don't have a reason why we shouldn't do
that.

Will send the next version of the patch series with this change.

Thanks for review.

BR,
Andrej

2021-12-02 08:34:39

by Andrej Picej

[permalink] [raw]
Subject: Re: [PATCH v3 2/4] watchdog: da9062: reset board on watchdog timeout



On 1. 12. 21 22:26, Guenter Roeck wrote:
> On 12/1/21 12:15 AM, 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 <[email protected]>
>> ---
>> Changes in v3:
>>   - no changes
>>
>> Changes in v2:
>>   - don't force the "reset" for all da9062-watchdog users, instead add DT
>>     binding where the behavior can be selected
>> ---
>>   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.
>> +     */
>
> Please use standard multi-line comments. This is not the networking
> subsystem.
>
> Also, if you think this code should be here and not in the probe function,
> as suggested by Adam, please provide a rationale.
>

I will fix the multi-line comment, move this code to probe and
submit the changes in the next patch version.

Thanks,
Andrej

2021-12-02 12:12:29

by Adam Thomson

[permalink] [raw]
Subject: RE: [PATCH v3 2/4] watchdog: da9062: reset board on watchdog timeout

On 02 December 2021 07:43, Andrej Picej wrote:

> > I have a question how to correctly restart the system after
> > watchdog timeout.
> > If I understand it correct after watchdog timeout the system
> > restarts only if WATCHDOG_SD (Bit 3) in register CONFIG_I is
> > set.
> > What is the difference if WATCHDOG_SD isn't set, but WAKE_UP
> > (Bit 2) in register CONTROL_F is set? From outside on my
> > system I observe the same behavior. After watchdog timeout
> > my system restarts. So where are the differences?
> > It would be nice if you could answer this question, as you
> > certainly know this chip very well.
>
> To be honest I don't really know the chip that well, I'm just trying to
> add this feature and hopefully help others if they run into the same
> problem. I think @Adam will be more helpful here.
>
> But from quick look at da9062 datasheet, mainly chapter "8.8 Power
> Modes" I see next main differences:
> - setting WATCHDOG_SD enables SHUTDOWN sequence when the watchdog
> timeout is triggered. This puts the chip (da9062) in RESET mode.
> Taken from DA9062 datasheet:
> > In RESET mode, the internal supplies, and LDO1 (if configured as an always-on
> supply) are enabled.
> > All other DA9062 supplies are disabled.
> > DA9062 is in RESET mode whenever a complete application shutdown is
> required
> > The DA9062’s register configuration will be re-loaded from OTP when leaving
> the RESET mode
>
> - if you set the WAKE_UP bit than the chip enters POWERDOWN mode on
> watchdog timeout. I understand the POWERDOWN mode as a not that "deep"
> mode as a RESET mode Device will go from RESET mode to POWERDOWN mode in
> the sequence of powering-up.
>
> The above explanation is just my understanding after quick look, @Adam
> please correct me if I'm talking nonsense.
>
> Please have a look at the DA9062 datasheet for more information. Sorry,
> that I can't be more helpful here.

Yes, POWERDOWN doesn't go to RESET and thus doesn't re-read OTP, so some
settings will persist. Also, depending on the state of NRES_MODE, the nRESET pin
state may or may not be modified to reset the attached processor. The behaviour
of POWERDOWN and which regulators are disabled is down to the OTP configuration
of the device.

For transition to POWERDOWN, if the WAKE_UP bit is set then the device will
immediately transition back out of POWERDOWN to ACTIVE and the WAKE_UP bit will
be cleared after a time.