watchdog: add support for QCOM WDT
cover watchdog: add support for QCOM WDT
This patchset provides support for the Watchdog Timer (WDT) found in the Krait Processor Sub-system (KPSS) of the MSM8960, APQ8064, and IPQ8064 chips.
This driver is implemented ontop of WATCHDOG_CORE, and therefore its primary interface is through userspace. The implemantion is currently very basic (i.e. it doesn't support PRETIMEOUT, even though it could be implemented through the WDT's BARK functionality). It should also be fairly easy to extend this driver in the future to support newer chipsets as well.
Patch 3 also extends the driver to also register a restart_notifier, making it possible for the WDT to act as a restart mechanism if more favorable mechanisms don't work. This is important for some boards which don't support PS_HOLD, like the IPQ8064-based AP148 board.
Josh Cartwright (3):
watchdog: qcom: add support for KPSS WDT
watchdog: qcom: document device tree bindings
watchdog: qcom: register a restart notifier
.../devicetree/bindings/watchdog/qcom-wdt.txt | 21 +++ drivers/watchdog/Kconfig | 10 ++ drivers/watchdog/Makefile | 1 + drivers/watchdog/qcom-wdt.c | 176 +++++++++++++++++++++ 4 files changed, 208 insertions(+) create mode 100644 Documentation/devicetree/bindings/watchdog/qcom-wdt.txt create mode 100644 drivers/watchdog/qcom-wdt.c
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
PATCH 1/3 watchdog: qcom: add support for KPSS WDT
Add a driver for the watchdog timer block found in the Krait Processor Subsystem (KPSS) on the MSM8960, APQ8064, and IPQ8064.
--- drivers/watchdog/Kconfig | 10 +++ drivers/watchdog/Makefile | 1 + drivers/watchdog/qcom-wdt.c | 145 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 156 insertions(+) create mode 100644 drivers/watchdog/qcom-wdt.c
drivers/watchdog/Kconfig +10 -0
@@ -443,6 +443,16 @@ config TEGRA_WATCHDOG To compile this driver as a module, choose M here: the module will be called tegra_wdt. +config QCOM_WDT + bool "QCOM watchdog" + depends on HAS_IOMEM + depends on ARCH_QCOM + select WATCHDOG_CORE + help + Say Y here to include Watchdog timer support for the watchdog found + on QCOM chipsets. Currently supported targets are the MSM8960, + APQ8064, and IPQ8064. + # AVR32 Architecture config AT32AP700X_WDT
drivers/watchdog/Makefile +1 -0
@@ -57,6 +57,7 @@ obj-$(CONFIG_RETU_WATCHDOG) += retu_wdt.o obj-$(CONFIG_BCM2835_WDT) += bcm2835_wdt.o obj-$(CONFIG_MOXART_WDT) += moxart_wdt.o obj-$(CONFIG_SIRFSOC_WATCHDOG) += sirfsoc_wdt.o +obj-$(CONFIG_QCOM_WDT) += qcom-wdt.o obj-$(CONFIG_BCM_KONA_WDT) += bcm_kona_wdt.o obj-$(CONFIG_TEGRA_WATCHDOG) += tegra_wdt.o
drivers/watchdog/qcom-wdt.c new file +145 -0
145 lines not on this page · raw message
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Guenter Roeck
maintainerRe: [PATCH 1/3] watchdog: qcom: add support for KPSS WDT
Quoted 4 lines · Josh Cartwright
Add a driver for the watchdog timer block found in the Krait Processor Subsystem (KPSS) on the MSM8960, APQ8064, and IPQ8064.
Signed-off-by Josh Cartwright <[email protected]>
Hi Josh,
comments inline.
Thanks,
Guenter
Quoted 144 lines
--- drivers/watchdog/Kconfig | 10 +++ drivers/watchdog/Makefile | 1 + drivers/watchdog/qcom-wdt.c | 145 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 156 insertions(+)create mode 100644
drivers/watchdog/qcom-wdt.cdrivers/watchdog/Kconfig +10
-0@@ -443,6 +443,16 @@ config TEGRA_WATCHDOG To compile this driver as a module, choose M here: the module will be called tegra_wdt. +config QCOM_WDT + bool "QCOM watchdog" + depends on HAS_IOMEM + depends on ARCH_QCOM + select WATCHDOG_CORE + help + Say Y here to include Watchdog timer support for the watchdog found + on QCOM chipsets. Currently supported targets are the MSM8960, + APQ8064, and IPQ8064. + # AVR32 Architecture config AT32AP700X_WDTdrivers/watchdog/Makefile +1
-0@@ -57,6 +57,7 @@ obj-$(CONFIG_RETU_WATCHDOG) += retu_wdt.o obj-$(CONFIG_BCM2835_WDT) += bcm2835_wdt.o obj-$(CONFIG_MOXART_WDT) += moxart_wdt.o obj-$(CONFIG_SIRFSOC_WATCHDOG) += sirfsoc_wdt.o +obj-$(CONFIG_QCOM_WDT) += qcom-wdt.o obj-$(CONFIG_BCM_KONA_WDT) += bcm_kona_wdt.o obj-$(CONFIG_TEGRA_WATCHDOG) += tegra_wdt.odrivers/watchdog/qcom-wdt.c new file +98
-0@@ -0,0 +1,145 @@ +/* Copyright (c) 2014, The Linux Foundation. All rights reserved. +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License version 2 and +* only version 2 as published by the Free Software Foundation. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +* +*/ +#include <linux/io.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/watchdog.h> + +#define WDT_RST 0x0 +#define WDT_EN 0x8 +#define WDT_BITE_TIME 0x24 + +struct qcom_wdt { +struct watchdog_device wdd; +unsigned long freq; +void __iomem *base; +}; + +static inline +struct qcom_wdt *to_qcom_wdt (struct watchdog_device *wdd) +{ + return container_of(wdd, struct qcom_wdt, wdd); +} + +static int qcom_wdt_start (struct watchdog_device *wdd) +{ +struct qcom_wdt *wdt =to_qcom_wdt (wdd); + +writel (0 , wdt->base +WDT_EN ); +writel (1 , wdt->base +WDT_RST ); +writel (wdd->timeout * wdt->freq, wdt->base +WDT_BITE_TIME ); +writel (1 , wdt->base +WDT_EN ); +return 0 ; +} + +static int qcom_wdt_stop (struct watchdog_device *wdd) +{ +struct qcom_wdt *wdt =to_qcom_wdt (wdd); + +writel (0 , wdt->base +WDT_EN ); +return 0 ; +} + +static int qcom_wdt_ping (struct watchdog_device *wdd) +{ +struct qcom_wdt *wdt =to_qcom_wdt (wdd); + +writel (1 , wdt->base +WDT_RST ); +return 0 ; +} + +static int qcom_wdt_set_timeout (struct watchdog_device *wdd, +unsigned int timeout) +{ + wdd->timeout = timeout; +return qcom_wdt_start (wdd); +} + +static const struct watchdog_ops qcom_wdt_ops = { + .start = qcom_wdt_start, + .stop = qcom_wdt_stop, + .ping = qcom_wdt_ping, + .set_timeout = qcom_wdt_set_timeout, + .owner =THIS_MODULE , +}; + +static const struct watchdog_info qcom_wdt_info = { + .options =WDIOF_KEEPALIVEPING + |WDIOF_MAGICCLOSE + |WDIOF_SETTIMEOUT , + .identity =KBUILD_MODNAME , +}; + +static int qcom_watchdog_probe (struct platform_device *pdev) +{ +struct qcom_wdt *wdt; +struct resource *res; +u32 tmp; +int ret; + + wdt =devm_kzalloc (&pdev->dev,sizeof (*wdt),GFP_KERNEL ); +if (!wdt) +return -ENOMEM ; + +platform_set_drvdata (pdev, wdt); + + res =platform_get_resource (pdev,IORESOURCE_MEM ,0 );
drivers/watchdog/qcom-wdt.c line 108
Quoted 10 lines
drivers/watchdog/qcom-wdt.c new file +10
-0+ wdt->base =devm_ioremap_resource (&pdev->dev, res); +if (IS_ERR (wdt->base)) +return PTR_ERR (wdt->base); + + ret =of_property_read_u32 (pdev->dev.of_node,"clock-frequency" , &tmp); +if (ret) { +dev_err (&pdev->dev,"unable to get clock-frequency\n" ); +return ret; + } +
You might want to use a clock property here, and the complete sequence of
devm_clk_get
clk_prepare_enable
clk_disable_unprepare
clk_get_rate
drivers/watchdog/qcom-wdt.c line 115
Quoted 7 lines
drivers/watchdog/qcom-wdt.c new file +7
-0+ wdt->freq = tmp; + + wdt->wdd.dev = &pdev->dev; + wdt->wdd.info = &qcom_wdt_info; + wdt->wdd.ops = &qcom_wdt_ops; + wdt->wdd.min_timeout =1 ; + wdt->wdd.max_timeout =0x10000000U / wdt->freq;
As written, wdt->freq can be 0, which results in a nice division by zero here.
drivers/watchdog/qcom-wdt.c line 116
Quoted 1 line
drivers/watchdog/qcom-wdt.c new file +1
-0+watchdog_init_timeout (&wdt->wdd,0 , &pdev->dev);
That leaves you with no default timeout if timeout-sec is not set in devicetree, which if I understand the code correctly might result in an immediate reset. Is this really what you want to happen ?
Quoted 10 lines
drivers/watchdog/qcom-wdt.c new file +10
-0+ + ret =watchdog_register_device (&wdt->wdd); +if (ret) { +dev_err (&pdev->dev,"failed to register watchdog\n" ); +return ret; + } + +return 0 ; +} +
drivers/watchdog/qcom-wdt.c line 136
Quoted 10 lines
drivers/watchdog/qcom-wdt.c new file +10
-0+static const struct of_device_id qcom_wdt_of_table[] = { + { .compatible ="qcom,kpss-wdt-msm8960" , }, + { .compatible ="qcom,kpss-wdt-apq8064" , }, + { .compatible ="qcom,kpss-wdt-ipq8064" , }, + { }, +}; +MODULE_DEVICE_TABLE (of, qcom_wdt_of_table); + +static struct platform_driver qcom_watchdog_driver = { + .probe = qcom_watchdog_probe,
No remove function ?
Yes, you don't need it, because the driver can only be built into the kernel, but there is a practical impact: It means the driver must always be built into the kernel even if the image is supposed to be used on different systems, some of which may not support this specific watchdog.
Sure, you might say that you don't care about images supporting more than one hardware, but the tendency seems to be multi-target images nowadays.
Quoted 10 lines
drivers/watchdog/qcom-wdt.c new file +9
-0+ .driver = { + .name = KBUILD_MODNAME, + .of_match_table = qcom_wdt_of_table, + }, +}; +module_platform_driver (qcom_watchdog_driver); + +MODULE_DESCRIPTION ("QCOM KPSS Watchdog Driver" ); +MODULE_LICENSE ("GPL v2" );
Re: [PATCH 1/3] watchdog: qcom: add support for KPSS WDT
Quoted 9 lines · Guenter Roeck
Quoted 4 lines · Josh Cartwright
Add a driver for the watchdog timer block found in the Krait Processor Subsystem (KPSS) on the MSM8960, APQ8064, and IPQ8064.
Signed-off-by Josh Cartwright <[email protected]>Hi Josh,
comments inline.
Thanks for taking a look!
[..]
Quoted 22 lines
Quoted 22 lines
drivers/watchdog/qcom-wdt.c new file +22
-0+static int qcom_watchdog_probe (struct platform_device *pdev) +{ +struct qcom_wdt *wdt; +struct resource *res; +u32 tmp; +int ret; + + wdt =devm_kzalloc (&pdev->dev,sizeof (*wdt),GFP_KERNEL ); +if (!wdt) +return -ENOMEM ; + +platform_set_drvdata (pdev, wdt); + + res =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); + wdt->base =devm_ioremap_resource (&pdev->dev, res); +if (IS_ERR (wdt->base)) +return PTR_ERR (wdt->base); + + ret =of_property_read_u32 (pdev->dev.of_node,"clock-frequency" , &tmp); +if (ret) { +dev_err (&pdev->dev,"unable to get clock-frequency\n" ); +return ret;
Quoted 8 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ } +You might want to use a clock property here, and the complete sequence of
devm_clk_get
clk_prepare_enable
clk_disable_unprepare
clk_get_rate
Agreed. I think this would be ideal. I'll need to take a closer look at how this thing is clocked, and how/if the clocks are currently being modelled.
Quoted 9 lines
drivers/watchdog/qcom-wdt.c line 115
Quoted 7 lines
drivers/watchdog/qcom-wdt.c new file +7
-0+ wdt->freq = tmp; + + wdt->wdd.dev = &pdev->dev; + wdt->wdd.info = &qcom_wdt_info; + wdt->wdd.ops = &qcom_wdt_ops; + wdt->wdd.min_timeout =1 ; + wdt->wdd.max_timeout =0x10000000U / wdt->freq;As written,
wdt->freqcan be 0, which results in a nice division by zero here.
Indeed. I'll add a check.
Quoted 5 lines
drivers/watchdog/qcom-wdt.c line 116
Quoted 1 line
drivers/watchdog/qcom-wdt.c new file +1
-0+watchdog_init_timeout (&wdt->wdd,0 , &pdev->dev);That leaves you with no default timeout if timeout-sec is not set in devicetree, which if I understand the code correctly might result in an immediate reset. Is this really what you want to happen ?
I think I'd like to handle timeout-sec being unspecified as an error at probe. If someone explicitly sets timeout-sec = <0>, then they get what they ask for. I'll take another look to see how to make this happen.
Quoted 23 lines
Quoted 10 lines
drivers/watchdog/qcom-wdt.c new file +10
-0+ + ret =watchdog_register_device (&wdt->wdd); +if (ret) { +dev_err (&pdev->dev,"failed to register watchdog\n" ); +return ret; + } + +return 0 ; +} +drivers/watchdog/qcom-wdt.c line 136
Quoted 10 lines
drivers/watchdog/qcom-wdt.c new file +10
-0+static const struct of_device_id qcom_wdt_of_table[] = { + { .compatible ="qcom,kpss-wdt-msm8960" , }, + { .compatible ="qcom,kpss-wdt-apq8064" , }, + { .compatible ="qcom,kpss-wdt-ipq8064" , }, + { }, +}; +MODULE_DEVICE_TABLE (of, qcom_wdt_of_table); + +static struct platform_driver qcom_watchdog_driver = { + .probe = qcom_watchdog_probe,No remove function ?
Quoted 7 lines
Yes, you don't need it, because the driver can only be built into the kernel, but there is a practical impact: It means the driver must always be built into the kernel even if the image is supposed to be used on different systems, some of which may not support this specific watchdog.
Sure, you might say that you don't care about images supporting more than one hardware, but the tendency seems to be multi-target images nowadays.
This was motivated by the addition of the restart_handler bits in patch
3. For some reason I was thinking there were race conditions between module unloading/the restart_handler mechanism, but looking at it again, I'm not so sure. Is it safe to implement these handlers in modules? If so, I'll revisit this.
Thanks again,
Josh
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Guenter Roeck
maintainerRe: [PATCH 1/3] watchdog: qcom: add support for KPSS WDT
Quoted 45 lines · Josh Cartwright
Quoted 9 lines · Guenter Roeck
Quoted 4 lines · Josh Cartwright
Add a driver for the watchdog timer block found in the Krait Processor Subsystem (KPSS) on the MSM8960, APQ8064, and IPQ8064.
Signed-off-by Josh Cartwright <[email protected]>Hi Josh,
comments inline.
Thanks for taking a look!
[..]
Quoted 22 lines
Quoted 22 lines
drivers/watchdog/qcom-wdt.c new file +22
-0+static int qcom_watchdog_probe (struct platform_device *pdev) +{ +struct qcom_wdt *wdt; +struct resource *res; +u32 tmp; +int ret; + + wdt =devm_kzalloc (&pdev->dev,sizeof (*wdt),GFP_KERNEL ); +if (!wdt) +return -ENOMEM ; + +platform_set_drvdata (pdev, wdt); + + res =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); + wdt->base =devm_ioremap_resource (&pdev->dev, res); +if (IS_ERR (wdt->base)) +return PTR_ERR (wdt->base); + + ret =of_property_read_u32 (pdev->dev.of_node,"clock-frequency" , &tmp); +if (ret) { +dev_err (&pdev->dev,"unable to get clock-frequency\n" ); +return ret;Quoted 8 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ } +You might want to use a clock property here, and the complete sequence of
devm_clk_get
clk_prepare_enable
clk_disable_unprepare
clk_get_rate
Quoted 4 lines
Agreed. I think this would be ideal. I'll need to take a closer look at how this thing is clocked, and how/if the clocks are currently being modelled.
I think you should be able to specify some kind of "fixed" clock. Other watchdog drivers use the mechanism; maybe you can find some examples.
Quoted 18 lines
Quoted 9 lines
drivers/watchdog/qcom-wdt.c line 115
Quoted 7 lines
drivers/watchdog/qcom-wdt.c new file +7
-0+ wdt->freq = tmp; + + wdt->wdd.dev = &pdev->dev; + wdt->wdd.info = &qcom_wdt_info; + wdt->wdd.ops = &qcom_wdt_ops; + wdt->wdd.min_timeout =1 ; + wdt->wdd.max_timeout =0x10000000U / wdt->freq;As written,
wdt->freqcan be 0, which results in a nice division by zero here.Indeed. I'll add a check.
Quoted 5 lines
drivers/watchdog/qcom-wdt.c line 116
Quoted 1 line
drivers/watchdog/qcom-wdt.c new file +1
-0+watchdog_init_timeout (&wdt->wdd,0 , &pdev->dev);That leaves you with no default timeout if timeout-sec is not set in devicetree, which if I understand the code correctly might result in an immediate reset. Is this really what you want to happen ?
Quoted 4 lines
I think I'd like to handle timeout-sec being unspecified as an error at probe. If someone explicitly sets timeout-sec = <0>, then they get what they ask for. I'll take another look to see how to make this happen.
Hmm.. kind of unusual. Usual would be to initialize the timeout together with min_timeout / max_timeout above and only force the user to specify a value if the default timeout is not desirable. You don't really gain anything by making timeout-sec mandatory.
Quoted 28 lines
Quoted 28 lines
Quoted 10 lines
drivers/watchdog/qcom-wdt.c new file +10
-0+ + ret =watchdog_register_device (&wdt->wdd); +if (ret) { +dev_err (&pdev->dev,"failed to register watchdog\n" ); +return ret; + } + +return 0 ; +} +drivers/watchdog/qcom-wdt.c line 136
Quoted 10 lines
drivers/watchdog/qcom-wdt.c new file +10
-0+static const struct of_device_id qcom_wdt_of_table[] = { + { .compatible ="qcom,kpss-wdt-msm8960" , }, + { .compatible ="qcom,kpss-wdt-apq8064" , }, + { .compatible ="qcom,kpss-wdt-ipq8064" , }, + { }, +}; +MODULE_DEVICE_TABLE (of, qcom_wdt_of_table); + +static struct platform_driver qcom_watchdog_driver = { + .probe = qcom_watchdog_probe,No remove function ?
Yes, you don't need it, because the driver can only be built into the kernel, but there is a practical impact: It means the driver must always be built into the kernel even if the image is supposed to be used on different systems, some of which may not support this specific watchdog.
Quoted 9 lines
Quoted 2 lines
Sure, you might say that you don't care about images supporting more than one hardware, but the tendency seems to be multi-target images nowadays.
This was motivated by the addition of the
restart_handlerbits in patch
3. For some reason I was thinking there were race conditions between module unloading/therestart_handlermechanism, but looking at it again, I'm not so sure. Is it safe to implement these handlers in modules? If so, I'll revisit this.
Yes, it is safe. To ensure there are no race conditions was one of the reasons for implementing the restart handler as notifier call chain.
Guenter
Re: [PATCH 1/3] watchdog: qcom: add support for KPSS WDT
Quoted 14 lines · Guenter Roeck
Quoted 13 lines · Josh Cartwright
Quoted 9 lines · Guenter Roeck
Quoted 4 lines · Josh Cartwright
Add a driver for the watchdog timer block found in the Krait Processor Subsystem (KPSS) on the MSM8960, APQ8064, and IPQ8064.
Signed-off-by Josh Cartwright <[email protected]>Hi Josh,
comments inline.
Thanks for taking a look!
[..]
Quoted 11 lines
Quoted 10 lines
Quoted 5 lines
drivers/watchdog/qcom-wdt.c line 116
Quoted 1 line
drivers/watchdog/qcom-wdt.c new file +1
-0+watchdog_init_timeout (&wdt->wdd,0 , &pdev->dev);That leaves you with no default timeout if timeout-sec is not set in devicetree, which if I understand the code correctly might result in an immediate reset. Is this really what you want to happen ?
I think I'd like to handle timeout-sec being unspecified as an error at probe. If someone explicitly sets timeout-sec = <0>, then they get what they ask for. I'll take another look to see how to make this happen.
Quoted 4 lines
Hmm.. kind of unusual. Usual would be to initialize the timeout together with
min_timeout/max_timeoutabove and only force the user to specify a value if the default timeout is not desirable. You don't really gain anything by making timeout-sec mandatory.
Making timeout-sec mandatory makes it so I don't have to decide what a "sane default" is. :)
It's even less clear about what a sane default is looking at the other watchdog drivers. From the drivers I looked at, it ranges any where from 30s to 2mins. Am I just to choose? Why do these even differ between all of the drivers?
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Guenter Roeck
maintainerRe: [PATCH 1/3] watchdog: qcom: add support for KPSS WDT
Quoted 27 lines · Josh Cartwright
Quoted 14 lines · Guenter Roeck
Quoted 13 lines · Josh Cartwright
Quoted 9 lines · Guenter Roeck
Quoted 4 lines · Josh Cartwright
Add a driver for the watchdog timer block found in the Krait Processor Subsystem (KPSS) on the MSM8960, APQ8064, and IPQ8064.
Signed-off-by Josh Cartwright <[email protected]>Hi Josh,
comments inline.
Thanks for taking a look!
[..]
Quoted 11 lines
Quoted 10 lines
Quoted 5 lines
drivers/watchdog/qcom-wdt.c line 116
Quoted 1 line
drivers/watchdog/qcom-wdt.c new file +1
-0+watchdog_init_timeout (&wdt->wdd,0 , &pdev->dev);That leaves you with no default timeout if timeout-sec is not set in devicetree, which if I understand the code correctly might result in an immediate reset. Is this really what you want to happen ?
I think I'd like to handle timeout-sec being unspecified as an error at probe. If someone explicitly sets timeout-sec = <0>, then they get what they ask for. I'll take another look to see how to make this happen.
Quoted 8 lines
Quoted 4 lines
Hmm.. kind of unusual. Usual would be to initialize the timeout together with
min_timeout/max_timeoutabove and only force the user to specify a value if the default timeout is not desirable. You don't really gain anything by making timeout-sec mandatory.Making timeout-sec mandatory makes it so I don't have to decide what a "sane default" is. :)
Bad excuse ;-). You just force others to make the decision for you.
Quoted 5 lines
It's even less clear about what a sane default is looking at the other watchdog drivers. From the drivers I looked at, it ranges any where from 30s to 2mins. Am I just to choose? Why do these even differ between all of the drivers?
Sanity is defined as the majority opinion, as expressed quite nicely in "Everyone is insane but me".
Just pick something in between. If you don't want to make a decision, pick 30 seconds and blame it on me.
Maybe someone at some point finds a common ground. Until then it is per driver.
Thanks,
Guenter
PATCH 2/3 watchdog: qcom: document device tree bindings
In reply to Josh CartwrightThe Qualcomm Krait Processor Sub-system (KPSS) contains one or more instances of the WDT. Provide documentation on how to describe these in the device tree.
--- .../devicetree/bindings/watchdog/qcom-wdt.txt | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) create mode 100644 Documentation/devicetree/bindings/watchdog/qcom-wdt.txt
Documentation/devicetree/bindings/watchdog/qcom-wdt.txt new file +21 -0
@@ -0,0 +1,21 @@ +Qualcomm Krait Processor Sub-system (KPSS) Watchdog +--------------------------------------------------- + +Required properties : +- compatible : shall contain only one of the following: + + "qcom,kpss-wdt-msm8960" + "qcom,kpss-wdt-apq8064" + "qcom,kpss-wdt-ipq8064" + +- reg : shall contain base register location and length +- clock-frequency : shall contain the frequency at which the watchdog ticks +- timeout-sec : shall contain the default watchdog timeout in seconds + +Example: + watchdog at 208a038 { + compatible = "qcom,kpss-wdt-ipq8064"; + reg = <0x0208a038 0x40>; + clock-frequency = <32768>; + timeout-sec = <10>; + };
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
PATCH 3/3 watchdog: qcom: register a restart notifier
In reply to Josh CartwrightThe WDT's BITE_TIME warm-reset behavior can be leveraged as a last resort mechanism for triggering chip reset. Usually, other restart methods (such as PS_HOLD) are preferrable for issuing a more complete reset of the chip. As such, keep the priority of the watchdog notifier low.
--- drivers/watchdog/qcom-wdt.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)
drivers/watchdog/qcom-wdt.c +31 -0
@@ -15,6 +15,7 @@#include <linux/module.h> #include <linux/of.h> #include <linux/platform_device.h> +#include <linux/reboot.h> #include <linux/watchdog.h> #define WDT_RST 0x0
@@ -24,6 +25,7 @@struct qcom_wdt {struct watchdog_device wdd;unsigned long freq; +struct notifier_block restart_nb;void __iomem *base; };
@@ -82,6 +84,24 @@ static const struct watchdog_info qcom_wdt_info = { .identity = KBUILD_MODNAME, }; +static int qcom_wdt_restart (struct notifier_block *nb,unsigned long action, +void *data) +{ + struct qcom_wdt *wdt = container_of(nb, struct qcom_wdt, restart_nb); + +/* +* Trigger watchdog bite: +* Setup BITE_TIME to be very low, and enable WDT. +*/ +mutex_lock (&wdt->wdd.lock); +writel_relaxed (0 , wdt->base +WDT_EN ); +writel_relaxed (1 , wdt->base +WDT_RST ); +writel_relaxed (0x31F3 , wdt->base +WDT_BITE_TIME ); +writel_relaxed (1 , wdt->base +WDT_EN ); +mutex_unlock (&wdt->wdd.lock); +return NOTIFY_DONE ; +} +static int qcom_watchdog_probe (struct platform_device *pdev) {struct qcom_wdt *wdt;
@@ -121,6 +141,17 @@ static int qcom_watchdog_probe(struct platform_device *pdev)return ret; } +/* +* WDT restart notifier has priority 0 (use as a last resort) +*/ + wdt->restart_nb.notifier_call = qcom_wdt_restart; + ret =register_restart_handler (&wdt->restart_nb); +if (ret) { +dev_err (&pdev->dev,"failed to setup restart handler\n" ); +watchdog_unregister_device (&wdt->wdd); +return ret; + } +return 0 ; }
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Guenter Roeck
maintainerRe: [PATCH 3/3] watchdog: qcom: register a restart notifier
Quoted 35 lines · Josh Cartwright
The WDT's
BITE_TIMEwarm-reset behavior can be leveraged as a last resort mechanism for triggering chip reset. Usually, other restart methods (such asPS_HOLD) are preferrable for issuing a more complete reset of the chip. As such, keep the priority of the watchdog notifier low.Signed-off-by Josh Cartwright <[email protected]>--- drivers/watchdog/qcom-wdt.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)drivers/watchdog/qcom-wdt.c +2
-0@@ -15,6 +15,7 @@#include <linux/module.h> #include <linux/of.h> #include <linux/platform_device.h> +#include <linux/reboot.h> #include <linux/watchdog.h> #define WDT_RST 0x0@@ -24,6 +25,7 @@struct qcom_wdt {struct watchdog_device wdd;unsigned long freq; +struct notifier_block restart_nb;void __iomem *base; };@@ -82,6 +84,24 @@ static const struct watchdog_info qcom_wdt_info = { .identity = KBUILD_MODNAME, };
drivers/watchdog/qcom-wdt.c static const struct watchdog_info qcom_wdt_info = { · line 96
Quoted 10 lines
drivers/watchdog/qcom-wdt.c +10
-0+static int qcom_wdt_restart (struct notifier_block *nb,unsigned long action, +void *data) +{ + struct qcom_wdt *wdt = container_of(nb, struct qcom_wdt, restart_nb); + +/* +* Trigger watchdog bite: +* Setup BITE_TIME to be very low, and enable WDT. +*/ +mutex_lock (&wdt->wdd.lock);
At this time you don't need to worry about locks.
Actually, this might be dangerous if the lock happens to be taken, as it won't be released (there is no other code running anymore when this function is called).
drivers/watchdog/qcom-wdt.c static const struct watchdog_info qcom_wdt_info = { · line 99
Quoted 3 lines
drivers/watchdog/qcom-wdt.c +3
-0+writel_relaxed (0 , wdt->base +WDT_EN ); +writel_relaxed (1 , wdt->base +WDT_RST ); +writel_relaxed (0x31F3 , wdt->base +WDT_BITE_TIME );
What is the magic here, ie what does 0x31F3 stand for ?
Quoted 11 lines
drivers/watchdog/qcom-wdt.c +5
-0+writel_relaxed (1 , wdt->base +WDT_EN ); +mutex_unlock (&wdt->wdd.lock); +return NOTIFY_DONE ; +} +static int qcom_watchdog_probe (struct platform_device *pdev) {struct qcom_wdt *wdt;@@ -121,6 +141,17 @@ static int qcom_watchdog_probe(struct platform_device *pdev)return ret; }
drivers/watchdog/qcom-wdt.c static int qcom_watchdog_probe(struct platform_device *pdev) · line 152
Quoted 10 lines
drivers/watchdog/qcom-wdt.c +9
-0+/* +* WDT restart notifier has priority 0 (use as a last resort) +*/ + wdt->restart_nb.notifier_call = qcom_wdt_restart; + ret =register_restart_handler (&wdt->restart_nb); +if (ret) { +dev_err (&pdev->dev,"failed to setup restart handler\n" ); +watchdog_unregister_device (&wdt->wdd); +return ret;
Sure you want to return an error here ? The watchdog itself is still working, and this is supposed to be a restart method of last resort. Causing the driver to fail loading because it can not register its restart handler seems to be a bit aggressive.
Thanks,
Guenter
Quoted 6 lines
drivers/watchdog/qcom-wdt.c +2
-0+ } +return 0 ; }
Re: [PATCH 3/3] watchdog: qcom: register a restart notifier
Quoted 8 lines · Guenter Roeck
Quoted 7 lines · Josh Cartwright
The WDT's
BITE_TIMEwarm-reset behavior can be leveraged as a last resort mechanism for triggering chip reset. Usually, other restart methods (such asPS_HOLD) are preferrable for issuing a more complete reset of the chip. As such, keep the priority of the watchdog notifier low.Signed-off-by Josh Cartwright <[email protected]>
[..]
Quoted 13 lines
drivers/watchdog/qcom-wdt.c static const struct watchdog_info qcom_wdt_info = { · line 96
Quoted 10 lines
drivers/watchdog/qcom-wdt.c +10
-0+static int qcom_wdt_restart (struct notifier_block *nb,unsigned long action, +void *data) +{ + struct qcom_wdt *wdt = container_of(nb, struct qcom_wdt, restart_nb); + +/* +* Trigger watchdog bite: +* Setup BITE_TIME to be very low, and enable WDT. +*/ +mutex_lock (&wdt->wdd.lock);At this time you don't need to worry about locks.
Quoted 3 lines
Actually, this might be dangerous if the lock happens to be taken, as it won't be released (there is no other code running anymore when this function is called).
Ah, great. I'll drop the locking.
Quoted 5 lines
drivers/watchdog/qcom-wdt.c static const struct watchdog_info qcom_wdt_info = { · line 99
Quoted 3 lines
drivers/watchdog/qcom-wdt.c +3
-0+writel_relaxed (0 , wdt->base +WDT_EN ); +writel_relaxed (1 , wdt->base +WDT_RST ); +writel_relaxed (0x31F3 , wdt->base +WDT_BITE_TIME );What is the magic here, ie what does
0x31F3stand for ?
Nothing magic, it's just a reasonably low value to set the bite time counter at. It also happens to be the value at reset.
Quoted 12 lines
Quoted 12 lines
drivers/watchdog/qcom-wdt.c +5
-0+writel_relaxed (1 , wdt->base +WDT_EN ); +mutex_unlock (&wdt->wdd.lock); +return NOTIFY_DONE ; +} +static int qcom_watchdog_probe (struct platform_device *pdev) {struct qcom_wdt *wdt;@@ -121,6 +141,17 @@ static int qcom_watchdog_probe(struct platform_device *pdev)return ret; }
Quoted 14 lines
drivers/watchdog/qcom-wdt.c static int qcom_watchdog_probe(struct platform_device *pdev) · line 152
Quoted 9 lines
drivers/watchdog/qcom-wdt.c +9
-0+/* +* WDT restart notifier has priority 0 (use as a last resort) +*/ + wdt->restart_nb.notifier_call = qcom_wdt_restart; + ret =register_restart_handler (&wdt->restart_nb); +if (ret) { +dev_err (&pdev->dev,"failed to setup restart handler\n" ); +watchdog_unregister_device (&wdt->wdd); +return ret;Sure you want to return an error here ? The watchdog itself is still working, and this is supposed to be a restart method of last resort. Causing the driver to fail loading because it can not register its restart handler seems to be a bit aggressive.
It is a bit aggressive. I'll at least drop it to a dev_warn(); even though it is a "last resort", on some boards it's the only available mechanism for reliable restart.
Thanks!
Josh
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Guenter Roeck
maintainerRe: [PATCH 3/3] watchdog: qcom: register a restart notifier
Quoted 29 lines · Josh Cartwright
Quoted 8 lines · Guenter Roeck
Quoted 7 lines · Josh Cartwright
The WDT's
BITE_TIMEwarm-reset behavior can be leveraged as a last resort mechanism for triggering chip reset. Usually, other restart methods (such asPS_HOLD) are preferrable for issuing a more complete reset of the chip. As such, keep the priority of the watchdog notifier low.Signed-off-by Josh Cartwright <[email protected]>[..]
Quoted 13 lines
drivers/watchdog/qcom-wdt.c static const struct watchdog_info qcom_wdt_info = { · line 96
Quoted 10 lines
drivers/watchdog/qcom-wdt.c +10
-0+static int qcom_wdt_restart (struct notifier_block *nb,unsigned long action, +void *data) +{ + struct qcom_wdt *wdt = container_of(nb, struct qcom_wdt, restart_nb); + +/* +* Trigger watchdog bite: +* Setup BITE_TIME to be very low, and enable WDT. +*/ +mutex_lock (&wdt->wdd.lock);At this time you don't need to worry about locks.
Quoted 3 lines
Actually, this might be dangerous if the lock happens to be taken, as it won't be released (there is no other code running anymore when this function is called).
Ah, great. I'll drop the locking.
Quoted 9 lines
Quoted 5 lines
drivers/watchdog/qcom-wdt.c static const struct watchdog_info qcom_wdt_info = { · line 99
Quoted 3 lines
drivers/watchdog/qcom-wdt.c +3
-0+writel_relaxed (0 , wdt->base +WDT_EN ); +writel_relaxed (1 , wdt->base +WDT_RST ); +writel_relaxed (0x31F3 , wdt->base +WDT_BITE_TIME );What is the magic here, ie what does
0x31F3stand for ?Nothing magic, it's just a reasonably low value to set the bite time counter at. It also happens to be the value at reset.
Can you add a note explaining that this reflects a reset delay of <n> milliseconds or whatever it is ?
Quoted 27 lines
Quoted 12 lines
Quoted 12 lines
drivers/watchdog/qcom-wdt.c +5
-0+writel_relaxed (1 , wdt->base +WDT_EN ); +mutex_unlock (&wdt->wdd.lock); +return NOTIFY_DONE ; +} +static int qcom_watchdog_probe (struct platform_device *pdev) {struct qcom_wdt *wdt;@@ -121,6 +141,17 @@ static int qcom_watchdog_probe(struct platform_device *pdev)return ret; }Quoted 14 lines
drivers/watchdog/qcom-wdt.c static int qcom_watchdog_probe(struct platform_device *pdev) · line 152
Quoted 9 lines
drivers/watchdog/qcom-wdt.c +9
-0+/* +* WDT restart notifier has priority 0 (use as a last resort) +*/ + wdt->restart_nb.notifier_call = qcom_wdt_restart; + ret =register_restart_handler (&wdt->restart_nb); +if (ret) { +dev_err (&pdev->dev,"failed to setup restart handler\n" ); +watchdog_unregister_device (&wdt->wdd); +return ret;Sure you want to return an error here ? The watchdog itself is still working, and this is supposed to be a restart method of last resort. Causing the driver to fail loading because it can not register its restart handler seems to be a bit aggressive.
Quoted 4 lines
It is a bit aggressive. I'll at least drop it to a
dev_warn(); even though it is a "last resort", on some boards it's the only available mechanism for reliable restart.
Sure, but the problem is that you don't let people use the watchdog just because restart handler registration failed. dev_err or dev_warn is fine, my concern was not the message but that you abort watchdog registration in this case.
Kind of letting people not use their car just because one of its lights failed.
Guenter
Re: [PATCH 3/3] watchdog: qcom: register a restart notifier
Quoted 3 lines · Guenter Roeck
Quoted 2 lines · Josh Cartwright
Quoted 1 line · Guenter Roeck
On 09/18/2014 03:27 PM, Josh Cartwright wrote:
[..]
Quoted 6 lines
Quoted 4 lines
Nothing magic, it's just a reasonably low value to set the bite time counter at. It also happens to be the value at reset.
Can you add a note explaining that this reflects a reset delay of <n> milliseconds or whatever it is ?
Certainly. Will do.
Thanks,
Josh
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH 3/3] watchdog: qcom: register a restart notifier
In reply to Josh CartwrightHi Josh,
Tested this patch on IFC6410 by disabling pinctrl ps_hold reset implementation and on top of Guenter's patchset for restart_notifier.
I picked up DT changes from bindings as they are from this patchset.
Thanks
Pramod
Quoted 75 lines · Josh Cartwright
The WDT's
BITE_TIMEwarm-reset behavior can be leveraged as a last resort mechanism for triggering chip reset. Usually, other restart methods (such asPS_HOLD) are preferrable for issuing a more complete reset of the chip. As such, keep the priority of the watchdog notifier low.Signed-off-by Josh Cartwright <[email protected]>--- drivers/watchdog/qcom-wdt.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)drivers/watchdog/qcom-wdt.c +31
-0@@ -15,6 +15,7 @@#include <linux/module.h> #include <linux/of.h> #include <linux/platform_device.h> +#include <linux/reboot.h> #include <linux/watchdog.h> #define WDT_RST 0x0@@ -24,6 +25,7 @@struct qcom_wdt {struct watchdog_device wdd;unsigned long freq; +struct notifier_block restart_nb;void __iomem *base; };@@ -82,6 +84,24 @@ static const struct watchdog_info qcom_wdt_info = { .identity = KBUILD_MODNAME, }; +static int qcom_wdt_restart (struct notifier_block *nb,unsigned long action, +void *data) +{ + struct qcom_wdt *wdt = container_of(nb, struct qcom_wdt, restart_nb); + +/* +* Trigger watchdog bite: +* Setup BITE_TIME to be very low, and enable WDT. +*/ +mutex_lock (&wdt->wdd.lock); +writel_relaxed (0 , wdt->base +WDT_EN ); +writel_relaxed (1 , wdt->base +WDT_RST ); +writel_relaxed (0x31F3 , wdt->base +WDT_BITE_TIME ); +writel_relaxed (1 , wdt->base +WDT_EN ); +mutex_unlock (&wdt->wdd.lock); +return NOTIFY_DONE ; +} +static int qcom_watchdog_probe (struct platform_device *pdev) {struct qcom_wdt *wdt;@@ -121,6 +141,17 @@ static int qcom_watchdog_probe(struct platform_device *pdev)return ret; } +/* +* WDT restart notifier has priority 0 (use as a last resort) +*/ + wdt->restart_nb.notifier_call = qcom_wdt_restart; + ret =register_restart_handler (&wdt->restart_nb); +if (ret) { +dev_err (&pdev->dev,"failed to setup restart handler\n" ); +watchdog_unregister_device (&wdt->wdd); +return ret; + } +return 0 ; }