| Message ID | 20250416-wdt_reset_reason-v2-5-c65bba312914@oss.qualcomm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support to read the restart reason from IMEM | expand |
On 4/16/25 10:29 AM, Kathiravan Thirumoorthy wrote: > When the system boots up after a watchdog reset, the EXPIRED_STATUS bit > in the WDT_STS register is cleared. To identify if the system was restarted > due to WDT expiry, bootloaders update the information in the IMEM region. > Update the driver to read the restart reason from IMEM and populate the > bootstatus accordingly. > > For backward compatibility, keep the EXPIRED_STATUS bit check. Add a new > function qcom_wdt_get_restart_reason() to read the restart reason from > IMEM. > > Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com> > --- > Changes in v2: > - Use the syscon API to access the IMEM region > - Handle the error cases returned by qcom_wdt_get_restart_reason > - Define device specific data to retrieve the IMEM compatible, > offset and the value for non secure WDT, which allows to > extend the support for other SoCs > --- > drivers/watchdog/qcom-wdt.c | 47 +++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 45 insertions(+), 2 deletions(-) > > diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c > index 006f9c61aa64fd2b4ee9db493aeb54c8fafac818..94ba9ec9907a19854cd45a94f8da17d6e6eb33bc 100644 > --- a/drivers/watchdog/qcom-wdt.c > +++ b/drivers/watchdog/qcom-wdt.c > @@ -7,9 +7,11 @@ > #include <linux/interrupt.h> > #include <linux/io.h> > #include <linux/kernel.h> > +#include <linux/mfd/syscon.h> > #include <linux/module.h> > #include <linux/of.h> > #include <linux/platform_device.h> > +#include <linux/regmap.h> > #include <linux/watchdog.h> > > enum wdt_reg { > @@ -39,6 +41,9 @@ static const u32 reg_offset_data_kpss[] = { > }; > > struct qcom_wdt_match_data { > + const char *compatible; > + unsigned int restart_reason_offset; > + unsigned int non_secure_wdt_val; > const u32 *offset; > bool pretimeout; > u32 max_tick_count; > @@ -175,6 +180,15 @@ static const struct watchdog_info qcom_wdt_pt_info = { > .identity = KBUILD_MODNAME, > }; > > +static const struct qcom_wdt_match_data match_data_ipq5424 = { > + .compatible = "qcom,ipq5424-imem", > + .restart_reason_offset = 0x7b0, > + .non_secure_wdt_val = 0x5, > + .offset = reg_offset_data_kpss, > + .pretimeout = true, > + .max_tick_count = 0xFFFFFU, > +}; > + > static const struct qcom_wdt_match_data match_data_apcs_tmr = { > .offset = reg_offset_data_apcs_tmr, > .pretimeout = false, > @@ -187,6 +201,29 @@ static const struct qcom_wdt_match_data match_data_kpss = { > .max_tick_count = 0xFFFFFU, > }; > > +static int qcom_wdt_get_restart_reason(struct qcom_wdt *wdt, double space> + const struct qcom_wdt_match_data *data) Please align this > +{ > + struct regmap *imem; > + unsigned int val; > + int ret; > + > + imem = syscon_regmap_lookup_by_compatible(data->compatible); I still think nvmem could be better here, as it allows to plug in more magic values Konrad
On 4/16/2025 8:21 PM, Konrad Dybcio wrote: >> .max_tick_count = 0xFFFFFU, >> }; >> >> +static int qcom_wdt_get_restart_reason(struct qcom_wdt *wdt, > double space> + const struct qcom_wdt_match_data *data) > > Please align this Ack. >> +{ >> + struct regmap *imem; >> + unsigned int val; >> + int ret; >> + >> + imem = syscon_regmap_lookup_by_compatible(data->compatible); > I still think nvmem could be better here, as it allows to plug in > more magic values Sure, I will be on vacation next week. I shall check on how to use nvmem here once I'm back. > > Konrad
On 4/17/25 8:19 AM, Kathiravan Thirumoorthy wrote: > > On 4/16/2025 8:21 PM, Konrad Dybcio wrote: >>> .max_tick_count = 0xFFFFFU, >>> }; >>> +static int qcom_wdt_get_restart_reason(struct qcom_wdt *wdt, >> double space> + const struct qcom_wdt_match_data *data) >> >> Please align this > > > Ack. > > >>> +{ >>> + struct regmap *imem; >>> + unsigned int val; >>> + int ret; >>> + >>> + imem = syscon_regmap_lookup_by_compatible(data->compatible); >> I still think nvmem could be better here, as it allows to plug in >> more magic values > > > Sure, I will be on vacation next week. I shall check on how to use nvmem here once I'm back. We talked offline and I learned that IMEM is not in fact non-volatile, so while good looking, the nvram APIs are probably not really fit for it. Let's continue with the syscon approach. Konrad
On 4/30/2025 7:58 PM, Konrad Dybcio wrote: > On 4/17/25 8:19 AM, Kathiravan Thirumoorthy wrote: >> On 4/16/2025 8:21 PM, Konrad Dybcio wrote: >>>> .max_tick_count = 0xFFFFFU, >>>> }; >>>> +static int qcom_wdt_get_restart_reason(struct qcom_wdt *wdt, >>> double space> + const struct qcom_wdt_match_data *data) >>> >>> Please align this >> >> Ack. >> >> >>>> +{ >>>> + struct regmap *imem; >>>> + unsigned int val; >>>> + int ret; >>>> + >>>> + imem = syscon_regmap_lookup_by_compatible(data->compatible); >>> I still think nvmem could be better here, as it allows to plug in >>> more magic values >> >> Sure, I will be on vacation next week. I shall check on how to use nvmem here once I'm back. > We talked offline and I learned that IMEM is not in fact non-volatile, so > while good looking, the nvram APIs are probably not really fit for it. > > Let's continue with the syscon approach. Thanks Konrad for the discussion. > > Konrad
On 16/04/2025 09:29, Kathiravan Thirumoorthy wrote: > When the system boots up after a watchdog reset, the EXPIRED_STATUS bit > in the WDT_STS register is cleared. To identify if the system was restarted > due to WDT expiry, bootloaders update the information in the IMEM region. > Update the driver to read the restart reason from IMEM and populate the > bootstatus accordingly. Which bootloaders ? Do you mean bootrom or one of the subsequent phase bootloaders ? Please be specific about which bootloader populates this data i.e. if I switch my bootloader to u-boot do I loose the added flag ? > For backward compatibility, keep the EXPIRED_STATUS bit check. Add a new > function qcom_wdt_get_restart_reason() to read the restart reason from > IMEM. > > Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com> What I'd really love to see here is an example of reading out the data from sysfs. How do I as a user/consumer of this new functionality parse the new data it provides ? Ideally do this in the commit log and recommend doing it in the cover letter to, as people don't always read both when commenting on patches. > --- > Changes in v2: > - Use the syscon API to access the IMEM region > - Handle the error cases returned by qcom_wdt_get_restart_reason > - Define device specific data to retrieve the IMEM compatible, > offset and the value for non secure WDT, which allows to > extend the support for other SoCs > --- > drivers/watchdog/qcom-wdt.c | 47 +++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 45 insertions(+), 2 deletions(-) > > diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c > index 006f9c61aa64fd2b4ee9db493aeb54c8fafac818..94ba9ec9907a19854cd45a94f8da17d6e6eb33bc 100644 > --- a/drivers/watchdog/qcom-wdt.c > +++ b/drivers/watchdog/qcom-wdt.c > @@ -7,9 +7,11 @@ > #include <linux/interrupt.h> > #include <linux/io.h> > #include <linux/kernel.h> > +#include <linux/mfd/syscon.h> > #include <linux/module.h> > #include <linux/of.h> > #include <linux/platform_device.h> > +#include <linux/regmap.h> > #include <linux/watchdog.h> > > enum wdt_reg { > @@ -39,6 +41,9 @@ static const u32 reg_offset_data_kpss[] = { > }; > > struct qcom_wdt_match_data { > + const char *compatible; > + unsigned int restart_reason_offset; > + unsigned int non_secure_wdt_val; > const u32 *offset; > bool pretimeout; > u32 max_tick_count; > @@ -175,6 +180,15 @@ static const struct watchdog_info qcom_wdt_pt_info = { > .identity = KBUILD_MODNAME, > }; > > +static const struct qcom_wdt_match_data match_data_ipq5424 = { > + .compatible = "qcom,ipq5424-imem", > + .restart_reason_offset = 0x7b0, > + .non_secure_wdt_val = 0x5, > + .offset = reg_offset_data_kpss, > + .pretimeout = true, > + .max_tick_count = 0xFFFFFU, > +}; > + You should separate the addition of your compatibles and their descriptor tables from generic functional extensions. i.e. add the compat string and the above table in a subsequent patch. > static const struct qcom_wdt_match_data match_data_apcs_tmr = { > .offset = reg_offset_data_apcs_tmr, > .pretimeout = false, > @@ -187,6 +201,29 @@ static const struct qcom_wdt_match_data match_data_kpss = { > .max_tick_count = 0xFFFFFU, > }; > > +static int qcom_wdt_get_restart_reason(struct qcom_wdt *wdt, > + const struct qcom_wdt_match_data *data) > +{ > + struct regmap *imem; > + unsigned int val; > + int ret; > + > + imem = syscon_regmap_lookup_by_compatible(data->compatible); > + if (IS_ERR(imem)) > + return PTR_ERR(imem); > + > + ret = regmap_read(imem, data->restart_reason_offset, &val); > + if (ret) { > + dev_err(wdt->wdd.parent, "failed to read the restart reason info\n"); > + return ret; > + } > + > + if (val == data->non_secure_wdt_val) > + wdt->wdd.bootstatus = WDIOF_CARDRESET; > + > + return 0; > +} > + > static int qcom_wdt_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > @@ -267,8 +304,13 @@ static int qcom_wdt_probe(struct platform_device *pdev) > wdt->wdd.parent = dev; > wdt->layout = data->offset; > > - if (readl(wdt_addr(wdt, WDT_STS)) & 1) > - wdt->wdd.bootstatus = WDIOF_CARDRESET; > + ret = qcom_wdt_get_restart_reason(wdt, data); > + if (ret == -ENODEV) { > + if (readl(wdt_addr(wdt, WDT_STS)) & 1) > + wdt->wdd.bootstatus = WDIOF_CARDRESET; > + } else if (ret) { > + return ret; > + } > > /* > * If 'timeout-sec' unspecified in devicetree, assume a 30 second > @@ -322,6 +364,7 @@ static const struct dev_pm_ops qcom_wdt_pm_ops = { > }; > > static const struct of_device_id qcom_wdt_of_table[] = { > + { .compatible = "qcom,apss-wdt-ipq5424", .data = &match_data_ipq5424 }, > { .compatible = "qcom,kpss-timer", .data = &match_data_apcs_tmr }, > { .compatible = "qcom,scss-timer", .data = &match_data_apcs_tmr }, > { .compatible = "qcom,kpss-wdt", .data = &match_data_kpss }, > > -- > 2.34.1 > >
On 5/1/2025 5:31 AM, Bryan O'Donoghue wrote: > On 16/04/2025 09:29, Kathiravan Thirumoorthy wrote: >> When the system boots up after a watchdog reset, the EXPIRED_STATUS bit >> in the WDT_STS register is cleared. To identify if the system was restarted >> due to WDT expiry, bootloaders update the information in the IMEM region. >> Update the driver to read the restart reason from IMEM and populate the >> bootstatus accordingly. > Which bootloaders ? > > Do you mean bootrom or one of the subsequent phase bootloaders ? It is updated by the XBL. I shall mention it explicitly. > > Please be specific about which bootloader populates this data i.e. if I > switch my bootloader to u-boot do I loose the added flag ? > >> For backward compatibility, keep the EXPIRED_STATUS bit check. Add a new >> function qcom_wdt_get_restart_reason() to read the restart reason from >> IMEM. >> >> Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com> > What I'd really love to see here is an example of reading out the data > from sysfs. > > How do I as a user/consumer of this new functionality parse the new data > it provides ? > > Ideally do this in the commit log and recommend doing it in the cover > letter to, as people don't always read both when commenting on patches. Sure, will mention the sysfs path and its output in the commit log and cover letter. > >> --- >> Changes in v2: >> - Use the syscon API to access the IMEM region >> - Handle the error cases returned by qcom_wdt_get_restart_reason >> - Define device specific data to retrieve the IMEM compatible, >> offset and the value for non secure WDT, which allows to >> extend the support for other SoCs >> --- >> drivers/watchdog/qcom-wdt.c | 47 +++++++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 45 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c >> index 006f9c61aa64fd2b4ee9db493aeb54c8fafac818..94ba9ec9907a19854cd45a94f8da17d6e6eb33bc 100644 >> --- a/drivers/watchdog/qcom-wdt.c >> +++ b/drivers/watchdog/qcom-wdt.c >> @@ -7,9 +7,11 @@ >> #include <linux/interrupt.h> >> #include <linux/io.h> >> #include <linux/kernel.h> >> +#include <linux/mfd/syscon.h> >> #include <linux/module.h> >> #include <linux/of.h> >> #include <linux/platform_device.h> >> +#include <linux/regmap.h> >> #include <linux/watchdog.h> >> >> enum wdt_reg { >> @@ -39,6 +41,9 @@ static const u32 reg_offset_data_kpss[] = { >> }; >> >> struct qcom_wdt_match_data { >> + const char *compatible; >> + unsigned int restart_reason_offset; >> + unsigned int non_secure_wdt_val; >> const u32 *offset; >> bool pretimeout; >> u32 max_tick_count; >> @@ -175,6 +180,15 @@ static const struct watchdog_info qcom_wdt_pt_info = { >> .identity = KBUILD_MODNAME, >> }; >> >> +static const struct qcom_wdt_match_data match_data_ipq5424 = { >> + .compatible = "qcom,ipq5424-imem", >> + .restart_reason_offset = 0x7b0, >> + .non_secure_wdt_val = 0x5, >> + .offset = reg_offset_data_kpss, >> + .pretimeout = true, >> + .max_tick_count = 0xFFFFFU, >> +}; >> + > You should separate the addition of your compatibles and their > descriptor tables from generic functional extensions. > > i.e. add the compat string and the above table in a subsequent patch. Got it. Will split the patch into 2. > >> static const struct qcom_wdt_match_data match_data_apcs_tmr = { >> .offset = reg_offset_data_apcs_tmr, >> .pretimeout = false, >> @@ -187,6 +201,29 @@ static const struct qcom_wdt_match_data match_data_kpss = { >> .max_tick_count = 0xFFFFFU, >> }; >> >> +static int qcom_wdt_get_restart_reason(struct qcom_wdt *wdt, >> + const struct qcom_wdt_match_data *data) >> +{ >> + struct regmap *imem; >> + unsigned int val; >> + int ret; >> + >> + imem = syscon_regmap_lookup_by_compatible(data->compatible); >> + if (IS_ERR(imem)) >> + return PTR_ERR(imem); >> + >> + ret = regmap_read(imem, data->restart_reason_offset, &val); >> + if (ret) { >> + dev_err(wdt->wdd.parent, "failed to read the restart reason info\n"); >> + return ret; >> + } >> + >> + if (val == data->non_secure_wdt_val) >> + wdt->wdd.bootstatus = WDIOF_CARDRESET; >> + >> + return 0; >> +} >> + >> static int qcom_wdt_probe(struct platform_device *pdev) >> { >> struct device *dev = &pdev->dev; >> @@ -267,8 +304,13 @@ static int qcom_wdt_probe(struct platform_device *pdev) >> wdt->wdd.parent = dev; >> wdt->layout = data->offset; >> >> - if (readl(wdt_addr(wdt, WDT_STS)) & 1) >> - wdt->wdd.bootstatus = WDIOF_CARDRESET; >> + ret = qcom_wdt_get_restart_reason(wdt, data); >> + if (ret == -ENODEV) { >> + if (readl(wdt_addr(wdt, WDT_STS)) & 1) >> + wdt->wdd.bootstatus = WDIOF_CARDRESET; >> + } else if (ret) { >> + return ret; >> + } >> >> /* >> * If 'timeout-sec' unspecified in devicetree, assume a 30 second >> @@ -322,6 +364,7 @@ static const struct dev_pm_ops qcom_wdt_pm_ops = { >> }; >> >> static const struct of_device_id qcom_wdt_of_table[] = { >> + { .compatible = "qcom,apss-wdt-ipq5424", .data = &match_data_ipq5424 }, >> { .compatible = "qcom,kpss-timer", .data = &match_data_apcs_tmr }, >> { .compatible = "qcom,scss-timer", .data = &match_data_apcs_tmr }, >> { .compatible = "qcom,kpss-wdt", .data = &match_data_kpss }, >> >> -- >> 2.34.1 >> >>
diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c index 006f9c61aa64fd2b4ee9db493aeb54c8fafac818..94ba9ec9907a19854cd45a94f8da17d6e6eb33bc 100644 --- a/drivers/watchdog/qcom-wdt.c +++ b/drivers/watchdog/qcom-wdt.c @@ -7,9 +7,11 @@ #include <linux/interrupt.h> #include <linux/io.h> #include <linux/kernel.h> +#include <linux/mfd/syscon.h> #include <linux/module.h> #include <linux/of.h> #include <linux/platform_device.h> +#include <linux/regmap.h> #include <linux/watchdog.h> enum wdt_reg { @@ -39,6 +41,9 @@ static const u32 reg_offset_data_kpss[] = { }; struct qcom_wdt_match_data { + const char *compatible; + unsigned int restart_reason_offset; + unsigned int non_secure_wdt_val; const u32 *offset; bool pretimeout; u32 max_tick_count; @@ -175,6 +180,15 @@ static const struct watchdog_info qcom_wdt_pt_info = { .identity = KBUILD_MODNAME, }; +static const struct qcom_wdt_match_data match_data_ipq5424 = { + .compatible = "qcom,ipq5424-imem", + .restart_reason_offset = 0x7b0, + .non_secure_wdt_val = 0x5, + .offset = reg_offset_data_kpss, + .pretimeout = true, + .max_tick_count = 0xFFFFFU, +}; + static const struct qcom_wdt_match_data match_data_apcs_tmr = { .offset = reg_offset_data_apcs_tmr, .pretimeout = false, @@ -187,6 +201,29 @@ static const struct qcom_wdt_match_data match_data_kpss = { .max_tick_count = 0xFFFFFU, }; +static int qcom_wdt_get_restart_reason(struct qcom_wdt *wdt, + const struct qcom_wdt_match_data *data) +{ + struct regmap *imem; + unsigned int val; + int ret; + + imem = syscon_regmap_lookup_by_compatible(data->compatible); + if (IS_ERR(imem)) + return PTR_ERR(imem); + + ret = regmap_read(imem, data->restart_reason_offset, &val); + if (ret) { + dev_err(wdt->wdd.parent, "failed to read the restart reason info\n"); + return ret; + } + + if (val == data->non_secure_wdt_val) + wdt->wdd.bootstatus = WDIOF_CARDRESET; + + return 0; +} + static int qcom_wdt_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; @@ -267,8 +304,13 @@ static int qcom_wdt_probe(struct platform_device *pdev) wdt->wdd.parent = dev; wdt->layout = data->offset; - if (readl(wdt_addr(wdt, WDT_STS)) & 1) - wdt->wdd.bootstatus = WDIOF_CARDRESET; + ret = qcom_wdt_get_restart_reason(wdt, data); + if (ret == -ENODEV) { + if (readl(wdt_addr(wdt, WDT_STS)) & 1) + wdt->wdd.bootstatus = WDIOF_CARDRESET; + } else if (ret) { + return ret; + } /* * If 'timeout-sec' unspecified in devicetree, assume a 30 second @@ -322,6 +364,7 @@ static const struct dev_pm_ops qcom_wdt_pm_ops = { }; static const struct of_device_id qcom_wdt_of_table[] = { + { .compatible = "qcom,apss-wdt-ipq5424", .data = &match_data_ipq5424 }, { .compatible = "qcom,kpss-timer", .data = &match_data_apcs_tmr }, { .compatible = "qcom,scss-timer", .data = &match_data_apcs_tmr }, { .compatible = "qcom,kpss-wdt", .data = &match_data_kpss },
When the system boots up after a watchdog reset, the EXPIRED_STATUS bit in the WDT_STS register is cleared. To identify if the system was restarted due to WDT expiry, bootloaders update the information in the IMEM region. Update the driver to read the restart reason from IMEM and populate the bootstatus accordingly. For backward compatibility, keep the EXPIRED_STATUS bit check. Add a new function qcom_wdt_get_restart_reason() to read the restart reason from IMEM. Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com> --- Changes in v2: - Use the syscon API to access the IMEM region - Handle the error cases returned by qcom_wdt_get_restart_reason - Define device specific data to retrieve the IMEM compatible, offset and the value for non secure WDT, which allows to extend the support for other SoCs --- drivers/watchdog/qcom-wdt.c | 47 +++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 45 insertions(+), 2 deletions(-)