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

[PATCH v3 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>
---
Changes in v3:
 - no chagnes

Changes in v2:
 - no changes
---
 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 v3 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>
---
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

[PATCH v3 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.

If this property is not set, don't touch the WATCHDOG_SD bit and leave
the configuration to OTP. This way backward compatibility is not broken.

Signed-off-by: Andrej Picej <andrej.picej@norik.com>
---
Changes in v3:
 - add note about using the default OTP setting if this DT binding is
   not specified

Changes in v2:
 - new patch, document new DT binding
---
 Documentation/devicetree/bindings/watchdog/da9062-wdt.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/watchdog/da9062-wdt.txt b/Documentation/devicetree/bindings/watchdog/da9062-wdt.txt
index 950e4fba8dbc..354314d854ef 100644
--- a/Documentation/devicetree/bindings/watchdog/da9062-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/da9062-wdt.txt
@@ -10,6 +10,12 @@ 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. Only use this option if you want to change the
+	default chip's OTP setting for WATCHDOG_SD bit. If this property is NOT
+	set the WATCHDOG_SD bit and on timeout watchdog behavior will match the
+	chip's OTP settings.
 
 Example: DA9062
 
-- 
2.25.1

[PATCH v3 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>
---
Changes in v3:
 - no changes

Changes in v2:
 - new patch, enable shutdown mode for phytec-phycore (da9062 user)
---
 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 v3 2/4] watchdog: da9062: reset board on watchdog timeout

[Adam Thomson]

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 <andrej.picej@norik.com>
> ---
> 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

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

[Guenter Roeck]

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 <andrej.picej@norik.com>
> ---
> 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)
> 

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

[Andrej Picej]

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 <andrej.picej@norik.com>
>> ---
>> 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

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

[Andrej Picej]

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

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

[Andrej Picej]

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 <andrej.picej@norik.com>
>> ---
>> 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

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

[Adam Thomson]

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.