diff mbox series

[RFC,5/6] watchdog: qcom-wdt: add support to read the restart reason from IMEM

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

Commit Message

Kathiravan Thirumoorthy April 8, 2025, 8:49 a.m. UTC
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(-)

Comments

Guenter Roeck April 8, 2025, 12:33 p.m. UTC | #1
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
Krzysztof Kozlowski April 9, 2025, 7:03 a.m. UTC | #2
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
Kathiravan Thirumoorthy April 11, 2025, 5:29 a.m. UTC | #3
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.
Kathiravan Thirumoorthy April 11, 2025, 5:34 a.m. UTC | #4
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 mbox series

Patch

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;
 
 	/*