Message ID | 20250408-wdt_reset_reason-v1-5-e6ec30c2c926@oss.qualcomm.com |
---|---|
State | New |
Headers | show |
Series | Add support to read the restart reason from IMEM | expand |
On 4/8/25 01:49, 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> > --- > drivers/watchdog/qcom-wdt.c | 40 +++++++++++++++++++++++++++++++++++++++- > 1 file changed, 39 insertions(+), 1 deletion(-) > > diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c > index 006f9c61aa64fd2b4ee9db493aeb54c8fafac818..54d6eaa132ab9f63e1312a69ad51b7a14f78fe2d 100644 > --- a/drivers/watchdog/qcom-wdt.c > +++ b/drivers/watchdog/qcom-wdt.c > @@ -9,6 +9,7 @@ > #include <linux/kernel.h> > #include <linux/module.h> > #include <linux/of.h> > +#include <linux/of_address.h> > #include <linux/platform_device.h> > #include <linux/watchdog.h> > > @@ -22,6 +23,8 @@ enum wdt_reg { > > #define QCOM_WDT_ENABLE BIT(0) > > +#define NON_SECURE_WDT_RESET 0x5 > + > static const u32 reg_offset_data_apcs_tmr[] = { > [WDT_RST] = 0x38, > [WDT_EN] = 0x40, > @@ -187,6 +190,39 @@ 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) > +{ > + struct device_node *np; > + struct resource imem; > + void __iomem *base; > + int ret; > + > + np = of_find_compatible_node(NULL, NULL, "qcom,restart-reason-info"); > + if (!np) > + return -ENOENT; > + > + ret = of_address_to_resource(np, 0, &imem); > + of_node_put(np); > + if (ret < 0) { > + dev_err(wdt->wdd.parent, "can't translate OF node address\n"); > + return ret; > + } > + > + base = ioremap(imem.start, resource_size(&imem)); > + if (!base) { > + dev_err(wdt->wdd.parent, "failed to map restart reason info region\n"); > + return -ENOMEM; > + } > + > + memcpy_fromio(&ret, base, sizeof(ret)); > + iounmap(base); > + > + if (ret == NON_SECURE_WDT_RESET) > + wdt->wdd.bootstatus = WDIOF_CARDRESET; > + > + return 0; > +} > + > static int qcom_wdt_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > @@ -267,7 +303,9 @@ 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) > + ret = qcom_wdt_get_restart_reason(wdt); > + if (ret == -ENOENT && > + readl(wdt_addr(wdt, WDT_STS)) & 1) > wdt->wdd.bootstatus = WDIOF_CARDRESET; This ignores all other error returns from qcom_wdt_get_restart_reason(), but in that function it generates several dev_err(). Either make those messages less than an error, or treat them as error and drop out here. Thanks, Guenter
On 08/04/2025 10:49, 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> > --- > drivers/watchdog/qcom-wdt.c | 40 +++++++++++++++++++++++++++++++++++++++- > 1 file changed, 39 insertions(+), 1 deletion(-) > > diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c > index 006f9c61aa64fd2b4ee9db493aeb54c8fafac818..54d6eaa132ab9f63e1312a69ad51b7a14f78fe2d 100644 > --- a/drivers/watchdog/qcom-wdt.c > +++ b/drivers/watchdog/qcom-wdt.c > @@ -9,6 +9,7 @@ > #include <linux/kernel.h> > #include <linux/module.h> > #include <linux/of.h> > +#include <linux/of_address.h> > #include <linux/platform_device.h> > #include <linux/watchdog.h> > > @@ -22,6 +23,8 @@ enum wdt_reg { > > #define QCOM_WDT_ENABLE BIT(0) > > +#define NON_SECURE_WDT_RESET 0x5 > + > static const u32 reg_offset_data_apcs_tmr[] = { > [WDT_RST] = 0x38, > [WDT_EN] = 0x40, > @@ -187,6 +190,39 @@ 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) > +{ > + struct device_node *np; > + struct resource imem; > + void __iomem *base; > + int ret; > + > + np = of_find_compatible_node(NULL, NULL, "qcom,restart-reason-info"); > + if (!np) That's not how you express dependencies between devices. > + return -ENOENT; > + > + ret = of_address_to_resource(np, 0, &imem); > + of_node_put(np); > + if (ret < 0) { > + dev_err(wdt->wdd.parent, "can't translate OF node address\n"); > + return ret; > + } > + > + base = ioremap(imem.start, resource_size(&imem)); > + if (!base) { > + dev_err(wdt->wdd.parent, "failed to map restart reason info region\n"); > + return -ENOMEM; > + } > + > + memcpy_fromio(&ret, base, sizeof(ret)); > + iounmap(base); All this is wrong usage of syscon API, missing devlinks, messing up with other device's address space. Best regards, Krzysztof
On 4/8/2025 6:03 PM, Guenter Roeck wrote: >> static int qcom_wdt_probe(struct platform_device *pdev) >> { >> struct device *dev = &pdev->dev; >> @@ -267,7 +303,9 @@ 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) >> + ret = qcom_wdt_get_restart_reason(wdt); >> + if (ret == -ENOENT && >> + readl(wdt_addr(wdt, WDT_STS)) & 1) >> wdt->wdd.bootstatus = WDIOF_CARDRESET; > > This ignores all other error returns from qcom_wdt_get_restart_reason(), > but in that function it generates several dev_err(). Either make those > messages less than an error, or treat them as error and drop out here. Thanks for pointing this out. I will handle these errors in the next version.
On 4/9/2025 12:33 PM, Krzysztof Kozlowski wrote: >> +static int qcom_wdt_get_restart_reason(struct qcom_wdt *wdt) >> +{ >> + struct device_node *np; >> + struct resource imem; >> + void __iomem *base; >> + int ret; >> + >> + np = of_find_compatible_node(NULL, NULL, "qcom,restart-reason-info"); >> + if (!np) > That's not how you express dependencies between devices. As I mentioned in the bindings patch, I leveraged this from the qcom_pil_info.c[1]. I shall use the syscon_regmap_lookup_by_compatible() function. > >> + return -ENOENT; >> + >> + ret = of_address_to_resource(np, 0, &imem); >> + of_node_put(np); >> + if (ret < 0) { >> + dev_err(wdt->wdd.parent, "can't translate OF node address\n"); >> + return ret; >> + } >> + >> + base = ioremap(imem.start, resource_size(&imem)); >> + if (!base) { >> + dev_err(wdt->wdd.parent, "failed to map restart reason info region\n"); >> + return -ENOMEM; >> + } >> + >> + memcpy_fromio(&ret, base, sizeof(ret)); >> + iounmap(base); > All this is wrong usage of syscon API, missing devlinks, messing up with > other device's address space. I shall use regmap_read() instead of memcpy_fromio().
diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c index 006f9c61aa64fd2b4ee9db493aeb54c8fafac818..54d6eaa132ab9f63e1312a69ad51b7a14f78fe2d 100644 --- a/drivers/watchdog/qcom-wdt.c +++ b/drivers/watchdog/qcom-wdt.c @@ -9,6 +9,7 @@ #include <linux/kernel.h> #include <linux/module.h> #include <linux/of.h> +#include <linux/of_address.h> #include <linux/platform_device.h> #include <linux/watchdog.h> @@ -22,6 +23,8 @@ enum wdt_reg { #define QCOM_WDT_ENABLE BIT(0) +#define NON_SECURE_WDT_RESET 0x5 + static const u32 reg_offset_data_apcs_tmr[] = { [WDT_RST] = 0x38, [WDT_EN] = 0x40, @@ -187,6 +190,39 @@ 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) +{ + struct device_node *np; + struct resource imem; + void __iomem *base; + int ret; + + np = of_find_compatible_node(NULL, NULL, "qcom,restart-reason-info"); + if (!np) + return -ENOENT; + + ret = of_address_to_resource(np, 0, &imem); + of_node_put(np); + if (ret < 0) { + dev_err(wdt->wdd.parent, "can't translate OF node address\n"); + return ret; + } + + base = ioremap(imem.start, resource_size(&imem)); + if (!base) { + dev_err(wdt->wdd.parent, "failed to map restart reason info region\n"); + return -ENOMEM; + } + + memcpy_fromio(&ret, base, sizeof(ret)); + iounmap(base); + + if (ret == NON_SECURE_WDT_RESET) + wdt->wdd.bootstatus = WDIOF_CARDRESET; + + return 0; +} + static int qcom_wdt_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; @@ -267,7 +303,9 @@ 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) + ret = qcom_wdt_get_restart_reason(wdt); + if (ret == -ENOENT && + readl(wdt_addr(wdt, WDT_STS)) & 1) wdt->wdd.bootstatus = WDIOF_CARDRESET; /*
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> --- drivers/watchdog/qcom-wdt.c | 40 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-)