| Message ID | 20211201081512.3580837-2-andrej.picej@norik.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3,1/4] mfd: da9062: make register CONFIG_I writable | expand |
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
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) >
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
> > 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
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
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.
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)
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(+)