From: Fu Wei fu.wei@linaro.org
This patchset: (1)Introduce Documentation/devicetree/bindings/watchdog/sbsa-gwdt.txt for FDT info of SBSA Generic Watchdog, and give two examples of adding SBSA Generic Watchdog device node into the dts files: foundation-v8.dts and amd-seattle-soc.dtsi.
(2)Introduce "pretimeout" into the watchdog framework, and update Documentation/watchdog/watchdog-kernel-api.txt to introduce: (1)the new elements in the watchdog_device and watchdog_ops struct; (2)the new API "watchdog_init_timeouts".
(3)Introduce ARM SBSA watchdog driver: a.Use linux kernel watchdog framework; b.Work with FDT on ARM64; c.Use "pretimeout" in watchdog framework; d.Support getting timeout and pretimeout from parameter and FDT at the driver init stage. e.In the first timeout, do panic to save system context; f.In the second stage, user can still feed the dog without cleaning WS0. By this feature, we can avoid the panic infinite loops, while backing up a large system context in a server. g.In the second stage, can trigger WS1 by setting pretimeout = 0 if necessary.
This patchset has been tested with watchdog daemon (ACPI/FDT, module/build-in) on the following platforms: (1)ARM Foundation v8 model (2)AMD Seattle platform
This patchset has been tested with kdump successfully.
Changelog: v8: Rebase to latest kernel version(4.3-rc7). Separate the patches of GTDT support and arm_arch_timer. This clocksource relevant patch will upstreamed in a individual patchset. Update all the default timeout and pretimeout to 30s and 60s. Improve documentation and inline comments. Fix a bug in pretimeout support which makes timeout and pretimeout parameters initialization fail.
v7: Rebase to latest kernel version(4.2-rc7). Improve FDT support: geting resource by order, instead of name. According to the FDT support, Update the example dts file, gtdt.c and sbsa_gwdt.c. Pass the sparse test, and fix the warning. Fix the max_pretimeout and max_timeout value overflow bug. Delete the WCV output value.
v6: Improve the dtb example files: reduce the register frame size to 4K. Improve pretimeout support: (1) improve watchdog_init_timeouts function (2) rename watchdog_check_min_max_timeouts back to the original name (1) improve watchdog_timeout_invalid/watchdog_pretimeout_invalid Add the new features in the sbsa_gwdt driver: (1) In the second stage, user can feed the dog without cleaning WS0. (2) In the second stage, user can trigger WS1 by setting pretimeout = 0. (3) expand the max value of pretimeout, in case 10 second is not enough for a kdump kernel reboot in panic.
v5: Improve pretimeout support: (1)fix typo in documentation and comments. (2)fix the timeout limits validation bug. Simplify sbsa_gwdt driver: (1)integrate all the registers access functions into caller.
v4: Refactor GTDT support code: remove it from arch/arm64/kernel/acpi.c, put it into drivers/acpi/gtdt.c file. Integrate the GTDT code of drivers/clocksource/arm_arch_timer.c into drivers/acpi/gtdt.c. Improve pretimeout support, fix "pretimeout == 0" problem. Simplify sbsa_gwdt driver: (1)timeout/pretimeout limits setup; (2)keepalive function; (3)delete "clk == 0" check; (4)delete WS0 status bit check in interrupt routine; (5)sbsa_gwdt_set_wcv function.
v3: Delete "export arch_timer_get_rate" patch. Driver back to use arch_timer_get_cntfrq. Improve watchdog_init_timeouts function and update relevant documentation. Improve watchdog_timeout_invalid and watchdog_pretimeout_invalid. Improve foundation-v8.dts: delete the unnecessary tag of device node. Remove "ARM64 || COMPILE_TEST" from Kconfig. Add comments in arch/arm64/kernel/acpi.c Fix typoes and incorrect comments.
v2: Improve watchdog-kernel-api.txt documentation for pretimeout support. Export "arch_timer_get_rate" in arm_arch_timer.c. Add watchdog_init_timeouts API for pretimeout support in framework. Improve suspend and resume foundation in driver Improve timeout/pretimeout values init code in driver. Delete unnecessary items of the sbsa_gwdt struct and #define. Delete all unnecessary debug info in driver. Fix 64bit division bug. Use the arch_timer interface to get watchdog clock rate. Add MODULE_DEVICE_TABLE for platform device id. Fix typoes.
v1: The first version upstream patchset to linux mailing list.
Fu Wei (7): Documentation: add sbsa-gwdt.txt documentation ARM64: add SBSA Generic Watchdog device node in foundation-v8.dts ARM64: add SBSA Generic Watchdog device node in amd-seattle-soc.dtsi Watchdog: introdouce "pretimeout" into framework Watchdog: introduce ARM SBSA watchdog driver ACPI: add GTDT table parse driver into ACPI driver Watchdog: enable ACPI GTDT support for ARM SBSA watchdog driver
Fu Wei (5): Documentation: add sbsa-gwdt driver documentation ARM64: add SBSA Generic Watchdog device node in foundation-v8.dts ARM64: add SBSA Generic Watchdog device node in amd-seattle-soc.dtsi Watchdog: introdouce "pretimeout" into framework Watchdog: introduce ARM SBSA watchdog driver
.../devicetree/bindings/watchdog/sbsa-gwdt.txt | 46 +++ Documentation/watchdog/watchdog-kernel-api.txt | 55 ++- Documentation/watchdog/watchdog-parameters.txt | 6 + arch/arm64/boot/dts/amd/amd-seattle-soc.dtsi | 8 + arch/arm64/boot/dts/arm/foundation-v8.dts | 7 + drivers/watchdog/Kconfig | 14 + drivers/watchdog/Makefile | 1 + drivers/watchdog/sbsa_gwdt.c | 459 +++++++++++++++++++++ drivers/watchdog/watchdog_core.c | 127 ++++-- drivers/watchdog/watchdog_dev.c | 53 +++ include/linux/watchdog.h | 39 +- 11 files changed, 773 insertions(+), 42 deletions(-) create mode 100644 Documentation/devicetree/bindings/watchdog/sbsa-gwdt.txt create mode 100644 drivers/watchdog/sbsa_gwdt.c
From: Fu Wei fu.wei@linaro.org
The sbsa-gwdt.txt documentation in devicetree/bindings/watchdog is for introducing SBSA(Server Base System Architecture) Generic Watchdog device node info into FDT.
Also add sbsa-gwdt introduction in watchdog-parameters.txt
Acked-by: Arnd Bergmann arnd@arndb.de Signed-off-by: Fu Wei fu.wei@linaro.org --- .../devicetree/bindings/watchdog/sbsa-gwdt.txt | 46 ++++++++++++++++++++++ Documentation/watchdog/watchdog-parameters.txt | 6 +++ 2 files changed, 52 insertions(+)
diff --git a/Documentation/devicetree/bindings/watchdog/sbsa-gwdt.txt b/Documentation/devicetree/bindings/watchdog/sbsa-gwdt.txt new file mode 100644 index 0000000..ad8e99a --- /dev/null +++ b/Documentation/devicetree/bindings/watchdog/sbsa-gwdt.txt @@ -0,0 +1,46 @@ +* SBSA (Server Base System Architecture) Generic Watchdog + +The SBSA Generic Watchdog Timer is used to force a reset of the system +after two stages of timeout have elapsed. A detailed definition of the +watchdog timer can be found in the ARM document: ARM-DEN-0029 - Server +Base System Architecture (SBSA) + +Required properties: +- compatible: Should at least contain "arm,sbsa-gwdt". + +- reg: Each entry specifies the base physical 64-bit address of a register + frame and the 64-bit length of that frame; currently, two frames must be + defined, in this order: + 1: Watchdog control frame + 2: Refresh frame. + +- interrupts: At least one interrupt must be defined that will be used as + the WS0 interrupt. A WS1 interrupt definition can be provided, but is + optional. The interrupts must be defined in this order: + 1: WS0 interrupt + 2: WS1 interrupt + +Optional properties +- timeout-sec: To use a timeout value that is different from the driver + default values, use this property. If used, at least one timeout value + (in seconds) must be provided. A second optional timeout value (in + seconds) may also be provided and will be used as the pre-timeout value, + if it is given. + + There are two possible sources for driver default timeout values: + (1) the driver contains hard-coded default values, or + (2) module parameters can be given when the module is loaded + + If timeout/pretimeout values are provided when the module loads, they + will take priority. Second priority will be the timeout-sec from DTB, + and third the hard-coded driver values. + +Example for FVP Foundation Model v8: + +watchdog@2a440000 { + compatible = "arm,sbsa-gwdt"; + reg = <0x0 0x2a440000 0 0x1000>, + <0x0 0x2a450000 0 0x1000>; + interrupts = <0 27 4>; + timeout-sec = <60 30>; +}; diff --git a/Documentation/watchdog/watchdog-parameters.txt b/Documentation/watchdog/watchdog-parameters.txt index 9f9ec9f..e62c8c4 100644 --- a/Documentation/watchdog/watchdog-parameters.txt +++ b/Documentation/watchdog/watchdog-parameters.txt @@ -284,6 +284,12 @@ sbc_fitpc2_wdt: margin: Watchdog margin in seconds (default 60s) nowayout: Watchdog cannot be stopped once started ------------------------------------------------- +sbsa_gwdt: +timeout: Watchdog timeout in seconds. (default 60s) +pretimeout: Watchdog pretimeout in seconds. (default 30s) +nowayout: Watchdog cannot be stopped once started + (default=kernel config parameter) +------------------------------------------------- sc1200wdt: isapnp: When set to 0 driver ISA PnP support will be disabled (default=1) io: io port
On Wed, Oct 28, 2015 at 12:06:35AM +0800, fu.wei@linaro.org wrote:
From: Fu Wei fu.wei@linaro.org
The sbsa-gwdt.txt documentation in devicetree/bindings/watchdog is for introducing SBSA(Server Base System Architecture) Generic Watchdog device node info into FDT.
Also add sbsa-gwdt introduction in watchdog-parameters.txt
Acked-by: Arnd Bergmann arnd@arndb.de Signed-off-by: Fu Wei fu.wei@linaro.org
.../devicetree/bindings/watchdog/sbsa-gwdt.txt | 46 ++++++++++++++++++++++ Documentation/watchdog/watchdog-parameters.txt | 6 +++ 2 files changed, 52 insertions(+)
diff --git a/Documentation/devicetree/bindings/watchdog/sbsa-gwdt.txt b/Documentation/devicetree/bindings/watchdog/sbsa-gwdt.txt new file mode 100644 index 0000000..ad8e99a --- /dev/null +++ b/Documentation/devicetree/bindings/watchdog/sbsa-gwdt.txt @@ -0,0 +1,46 @@ +* SBSA (Server Base System Architecture) Generic Watchdog
+The SBSA Generic Watchdog Timer is used to force a reset of the system +after two stages of timeout have elapsed. A detailed definition of the +watchdog timer can be found in the ARM document: ARM-DEN-0029 - Server +Base System Architecture (SBSA)
+Required properties: +- compatible: Should at least contain "arm,sbsa-gwdt".
+- reg: Each entry specifies the base physical 64-bit address of a register
- frame and the 64-bit length of that frame; currently, two frames must be
Remove "64-bit" here. This depends on #address-cells and #size-cells, as usual.
- defined, in this order:
- 1: Watchdog control frame
- 2: Refresh frame.
+- interrupts: At least one interrupt must be defined that will be used as
- the WS0 interrupt. A WS1 interrupt definition can be provided, but is
- optional. The interrupts must be defined in this order:
- 1: WS0 interrupt
- 2: WS1 interrupt
Why is WS1 optional?
+Optional properties +- timeout-sec: To use a timeout value that is different from the driver
- default values, use this property.
Either define a default value, or don't state anything about the behaviour when this is not present.
If used, at least one timeout value
- (in seconds) must be provided. A second optional timeout value (in
- seconds) may also be provided and will be used as the pre-timeout value,
- if it is given.
- There are two possible sources for driver default timeout values:
- (1) the driver contains hard-coded default values, or
- (2) module parameters can be given when the module is loaded
- If timeout/pretimeout values are provided when the module loads, they
- will take priority. Second priority will be the timeout-sec from DTB,
- and third the hard-coded driver values.
The last two paragraphs should go. They describe Linux behaviour rather than the binding.
Thanks, Mark.
Hi Mark
Thanks for your rapid feedback, I appreciate your help very much.
On 28 October 2015 at 00:22, Mark Rutland mark.rutland@arm.com wrote:
On Wed, Oct 28, 2015 at 12:06:35AM +0800, fu.wei@linaro.org wrote:
From: Fu Wei fu.wei@linaro.org
The sbsa-gwdt.txt documentation in devicetree/bindings/watchdog is for introducing SBSA(Server Base System Architecture) Generic Watchdog device node info into FDT.
Also add sbsa-gwdt introduction in watchdog-parameters.txt
Acked-by: Arnd Bergmann arnd@arndb.de Signed-off-by: Fu Wei fu.wei@linaro.org
.../devicetree/bindings/watchdog/sbsa-gwdt.txt | 46 ++++++++++++++++++++++ Documentation/watchdog/watchdog-parameters.txt | 6 +++ 2 files changed, 52 insertions(+)
diff --git a/Documentation/devicetree/bindings/watchdog/sbsa-gwdt.txt b/Documentation/devicetree/bindings/watchdog/sbsa-gwdt.txt new file mode 100644 index 0000000..ad8e99a --- /dev/null +++ b/Documentation/devicetree/bindings/watchdog/sbsa-gwdt.txt @@ -0,0 +1,46 @@ +* SBSA (Server Base System Architecture) Generic Watchdog
+The SBSA Generic Watchdog Timer is used to force a reset of the system +after two stages of timeout have elapsed. A detailed definition of the +watchdog timer can be found in the ARM document: ARM-DEN-0029 - Server +Base System Architecture (SBSA)
+Required properties: +- compatible: Should at least contain "arm,sbsa-gwdt".
+- reg: Each entry specifies the base physical 64-bit address of a register
- frame and the 64-bit length of that frame; currently, two frames must be
Remove "64-bit" here. This depends on #address-cells and #size-cells, as usual.
Ah, right, Thanks , will do
- defined, in this order:
- 1: Watchdog control frame
- 2: Refresh frame.
+- interrupts: At least one interrupt must be defined that will be used as
- the WS0 interrupt. A WS1 interrupt definition can be provided, but is
- optional. The interrupts must be defined in this order:
- 1: WS0 interrupt
- 2: WS1 interrupt
Why is WS1 optional?
According to the description of WS1 in SBSA 2.3 (5.2 Watchdog Operation) page 21 ----------------- The signal is fed to a higher agent as an interrupt or reset for it to take executive action. ----------------
So WS1 maybe a interrupt.
In a real Hardware, WS1 hooks to a reset signal pin of BMC, if this pin is triggered, BMC will do a real warm reset. In this case, WS1 is a reset, Linux doesn't need to deal with that.
For now , I haven't found a hardware use WS1 as interrupt. In <ARM v8-A Foundation Platform User Guide> 3.2 Interrupt maps Page 22 Table 3-3 Shared peripheral interrupt assignments IRQ ID SPI offset Device 60 28 EL2 Generic Watchdog WS1
But I don't have further info about it.
Anyway, because this signal could be interrupt or reset, Linux don't need know this signal sometimes. So I think it should be optional in binding info.
Do I miss something? Any suggestion ? Please correct me, thanks.
+Optional properties +- timeout-sec: To use a timeout value that is different from the driver
- default values, use this property.
Either define a default value, or don't state anything about the behaviour when this is not present.
OK, thanks :-)
If used, at least one timeout value
- (in seconds) must be provided. A second optional timeout value (in
- seconds) may also be provided and will be used as the pre-timeout value,
- if it is given.
- There are two possible sources for driver default timeout values:
- (1) the driver contains hard-coded default values, or
- (2) module parameters can be given when the module is loaded
- If timeout/pretimeout values are provided when the module loads, they
- will take priority. Second priority will be the timeout-sec from DTB,
- and third the hard-coded driver values.
The last two paragraphs should go. They describe Linux behaviour rather than the binding.
yes, maybe that should be in the watchdog documentation?
Thanks, Mark.
On Tue, Oct 27, 2015 at 11:10 PM, Fu Wei fu.wei@linaro.org wrote:
Why is WS1 optional?
According to the description of WS1 in SBSA 2.3 (5.2 Watchdog Operation) page 21
The signal is fed to a higher agent as an interrupt or reset for it to take executive action.
So WS1 maybe a interrupt.
In a real Hardware, WS1 hooks to a reset signal pin of BMC, if this pin is triggered, BMC will do a real warm reset. In this case, WS1 is a reset, Linux doesn't need to deal with that.
For now , I haven't found a hardware use WS1 as interrupt. In <ARM v8-A Foundation Platform User Guide> 3.2 Interrupt maps Page 22 Table 3-3 Shared peripheral interrupt assignments IRQ ID SPI offset Device 60 28 EL2 Generic Watchdog WS1
But I don't have further info about it.
Anyway, because this signal could be interrupt or reset, Linux don't need know this signal sometimes. So I think it should be optional in binding info.
Do I miss something? Any suggestion ? Please correct me, thanks.
I think maybe Mark was asking why WS1 is optional, not the WS1 interrupt. Maybe you can reword the documentation to make is clear that only the *interrupt* for WS1 is optional.
However, the ACPI table only allows for one interrupt, and it's not clear whether that's the WS0 or WS1 interrupt. So if both WS0 and WS1 generate an interrupt, how does the driver handle that?
Hi Timur
On 31 October 2015 at 01:46, Timur Tabi timur@codeaurora.org wrote:
On Tue, Oct 27, 2015 at 11:10 PM, Fu Wei fu.wei@linaro.org wrote:
Why is WS1 optional?
According to the description of WS1 in SBSA 2.3 (5.2 Watchdog Operation) page 21
The signal is fed to a higher agent as an interrupt or reset for it to take executive action.
So WS1 maybe a interrupt.
In a real Hardware, WS1 hooks to a reset signal pin of BMC, if this pin is triggered, BMC will do a real warm reset. In this case, WS1 is a reset, Linux doesn't need to deal with that.
For now , I haven't found a hardware use WS1 as interrupt. In <ARM v8-A Foundation Platform User Guide> 3.2 Interrupt maps Page 22 Table 3-3 Shared peripheral interrupt assignments IRQ ID SPI offset Device 60 28 EL2 Generic Watchdog WS1
But I don't have further info about it.
Anyway, because this signal could be interrupt or reset, Linux don't need know this signal sometimes. So I think it should be optional in binding info.
Do I miss something? Any suggestion ? Please correct me, thanks.
I think maybe Mark was asking why WS1 is optional, not the WS1
My answer is for "why WS1 is optional"!
interrupt. Maybe you can reword the documentation to make is clear that
I didn't say : "only the *interrupt* for WS1 is optional."
However, the ACPI table only allows for one interrupt, and it's not clear whether that's the WS0 or WS1 interrupt. So if both WS0 and WS1 generate an interrupt, how does the driver handle that?
register a interrupt handle for both
-- Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
On 10/30/2015 01:35 PM, Fu Wei wrote:
I think maybe Mark was asking why WS1 is optional, not the WS1
My answer is for "why WS1 is optional"!
interrupt. Maybe you can reword the documentation to make is clear that
I didn't say : "only the*interrupt* for WS1 is optional."
WS1 itself is not optional. The spec says that WS0 and WS1 are separate events, and doesn't saying anything about either being optional. The *interrupt* for WS1, however, is optional.
Besides, what does the driver do with the WS1 interrupt? If it's specified in the device tree, it appears to be ignored by the driver. And the ACPI table only allows for specifying ONE interrupt. So how would the driver register a handler for WS1 on an ACPI system?
On Fri, Oct 30, 2015 at 01:53:24PM -0500, Timur Tabi wrote:
On 10/30/2015 01:35 PM, Fu Wei wrote:
I think maybe Mark was asking why WS1 is optional, not the WS1
My answer is for "why WS1 is optional"!
interrupt. Maybe you can reword the documentation to make is clear that
I didn't say : "only the*interrupt* for WS1 is optional."
WS1 itself is not optional. The spec says that WS0 and WS1 are separate events, and doesn't saying anything about either being optional. The *interrupt* for WS1, however, is optional.
This is a moot point. The distintion between the signal and the interrupt doens't matter here.
I was only asking why the interrupt was optional, and it seems per the spec it's expected to be handed to an agent at a higher exception level.
That implies that the OS should only care about WS0, assuming that I've understood correctly.
Thanks, Mark.
On 10/30/2015 02:05 PM, Mark Rutland wrote:
I was only asking why the interrupt was optional, and it seems per the spec it's expected to be handed to an agent at a higher exception level.
That implies that the OS should only care about WS0, assuming that I've understood correctly.
Yes, this my understand as well. Apologies if I didn't get that across.
Hi Timur.
On 31 October 2015 at 04:37, Timur Tabi timur@codeaurora.org wrote:
On 10/30/2015 02:05 PM, Mark Rutland wrote:
I was only asking why the interrupt was optional, and it seems per the spec it's expected to be handed to an agent at a higher exception level.
That implies that the OS should only care about WS0, assuming that I've understood correctly.
Yes, this my understand as well. Apologies if I didn't get that across.
Sorry for misunderstanding your meaning. maybe Linux don't need to handle WS1, but I am not sure if there is a hardware with WS1 interrupt we need to handle.
-- Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Hi Mark,
Great thanks for your feedback.
On 31 October 2015 at 03:05, Mark Rutland mark.rutland@arm.com wrote:
On Fri, Oct 30, 2015 at 01:53:24PM -0500, Timur Tabi wrote:
On 10/30/2015 01:35 PM, Fu Wei wrote:
I think maybe Mark was asking why WS1 is optional, not the WS1
My answer is for "why WS1 is optional"!
interrupt. Maybe you can reword the documentation to make is clear that
I didn't say : "only the*interrupt* for WS1 is optional."
WS1 itself is not optional. The spec says that WS0 and WS1 are separate events, and doesn't saying anything about either being optional. The *interrupt* for WS1, however, is optional.
This is a moot point. The distintion between the signal and the interrupt doens't matter here.
I was only asking why the interrupt was optional, and it seems per the spec it's expected to be handed to an agent at a higher exception level.
yes, that is the good point. Thanks I have thought about it My thought is : In virtualization system, Linux kernel with KVM support as a Hypervisor, and guest are using a one of SBSA watchdog. WS0 is handled by guest OS, and WS1 will be handled by Hypervisor.
And in datasheet of Foundation model, we can see: IRQ ID SPI offset Device 60 28 EL2 Generic Watchdog WS1
So maybe we need WS1 interrupt info, Maybe not. So I say : WS1 info in FDT binding info is optional.
*BUT*, (1) I don't see any hardware need to handle WS1 for now, because AMD seattle is the only real hardware with SBSA watchdog I can test now. (2) In GTDT, there is not data about WS1 (3) I don't handle WS1 in this driver.
That implies that the OS should only care about WS0, assuming that I've understood correctly.
yes, after getting your and Timur's email. I have thought about this in the weekend, Maybe we can forget about WS1 in the FDT binding info temporary until we need to handle WS1 in Linux on any hardware(or model).
Thanks, Mark.
Fu Wei wrote:
In virtualization system, Linux kernel with KVM support as a Hypervisor, and guest are using a one of SBSA watchdog. WS0 is handled by guest OS, and WS1 will be handled by Hypervisor.
I don't see how that would work, because the host kernel cannot reconfigure the behavior of WS1.
Besides, don't KVM guests use some kind of software and/or paravirtualized watchdog?
On 11/01/2015 11:06 PM, Timur Tabi wrote:
Fu Wei wrote:
In virtualization system, Linux kernel with KVM support as a Hypervisor, and guest are using a one of SBSA watchdog. WS0 is handled by guest OS, and WS1 will be handled by Hypervisor.
I don't see how that would work, because the host kernel cannot reconfigure the behavior of WS1.
Besides, don't KVM guests use some kind of software and/or paravirtualized watchdog?
Indeed, this is my expectation. IIRC the hardware watchdog initially defined in SBSA was intended for bare metal use to have something people could start implementing while we worked on the future (the language around register frames relates to secure/non-secure ELs in my recollection). For now, I expect an emulated watchdog for VMs, which ideally would be a software implementation of the same SBSA watchdog.
Jon.
From: Fu Wei fu.wei@linaro.org
This can be a example of adding SBSA Generic Watchdog device node into some dts files for the Soc which contains SBSA Generic Watchdog.
Acked-by: Arnd Bergmann arnd@arndb.de Signed-off-by: Fu Wei fu.wei@linaro.org --- arch/arm64/boot/dts/arm/foundation-v8.dts | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/arch/arm64/boot/dts/arm/foundation-v8.dts b/arch/arm64/boot/dts/arm/foundation-v8.dts index 4eac8dc..1a4fc40 100644 --- a/arch/arm64/boot/dts/arm/foundation-v8.dts +++ b/arch/arm64/boot/dts/arm/foundation-v8.dts @@ -237,4 +237,11 @@ }; }; }; + watchdog@2a440000 { + compatible = "arm,sbsa-gwdt"; + reg = <0x0 0x2a440000 0 0x1000>, + <0x0 0x2a450000 0 0x1000>; + interrupts = <0 27 4>; + timeout-sec = <60 30>; + }; };
From: Fu Wei fu.wei@linaro.org
This can be a example of adding SBSA Generic Watchdog device node into some dts files for the Soc which contains SBSA Generic Watchdog.
Acked-by: Arnd Bergmann arnd@arndb.de Acked-by: Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com Tested-by: Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com Signed-off-by: Fu Wei fu.wei@linaro.org --- arch/arm64/boot/dts/amd/amd-seattle-soc.dtsi | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/arch/arm64/boot/dts/amd/amd-seattle-soc.dtsi b/arch/arm64/boot/dts/amd/amd-seattle-soc.dtsi index 2874d92..f62d60c1 100644 --- a/arch/arm64/boot/dts/amd/amd-seattle-soc.dtsi +++ b/arch/arm64/boot/dts/amd/amd-seattle-soc.dtsi @@ -84,6 +84,14 @@ clock-names = "uartclk", "apb_pclk"; };
+ watchdog0: watchdog@e0bb0000 { + compatible = "arm,sbsa-gwdt"; + reg = <0x0 0xe0bc0000 0 0x1000>, + <0x0 0xe0bb0000 0 0x1000>; + interrupts = <0 337 4>; + timeout-sec = <60 30>; + }; + spi0: ssp@e1020000 { status = "disabled"; compatible = "arm,pl022", "arm,primecell";
From: Fu Wei fu.wei@linaro.org
Also update Documentation/watchdog/watchdog-kernel-api.txt to introduce: (1)the new elements in the watchdog_device and watchdog_ops struct; (2)the new API "watchdog_init_timeouts"
Reasons: (1)kernel already has two watchdog drivers are using "pretimeout": drivers/char/ipmi/ipmi_watchdog.c drivers/watchdog/kempld_wdt.c(but the definition is different) (2)some other drivers are going to use this: ARM SBSA Generic Watchdog
Signed-off-by: Fu Wei fu.wei@linaro.org Tested-by: Pratyush Anand panand@redhat.com --- Documentation/watchdog/watchdog-kernel-api.txt | 55 +++++++++-- drivers/watchdog/watchdog_core.c | 127 +++++++++++++++++++------ drivers/watchdog/watchdog_dev.c | 53 +++++++++++ include/linux/watchdog.h | 39 +++++++- 4 files changed, 232 insertions(+), 42 deletions(-)
diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt index d8b0d33..9be942c 100644 --- a/Documentation/watchdog/watchdog-kernel-api.txt +++ b/Documentation/watchdog/watchdog-kernel-api.txt @@ -53,6 +53,9 @@ struct watchdog_device { unsigned int timeout; unsigned int min_timeout; unsigned int max_timeout; + unsigned int pretimeout; + unsigned int min_pretimeout; + unsigned int max_pretimeout; void *driver_data; struct mutex lock; unsigned long status; @@ -75,6 +78,9 @@ It contains following fields: * timeout: the watchdog timer's timeout value (in seconds). * min_timeout: the watchdog timer's minimum timeout value (in seconds). * max_timeout: the watchdog timer's maximum timeout value (in seconds). +* pretimeout: the watchdog timer's pretimeout value (in seconds). +* min_pretimeout: the watchdog timer's minimum pretimeout value (in seconds). +* max_pretimeout: the watchdog timer's maximum pretimeout value (in seconds). * bootstatus: status of the device after booting (reported with watchdog WDIOF_* status bits). * driver_data: a pointer to the drivers private data of a watchdog device. @@ -99,6 +105,7 @@ struct watchdog_ops { int (*ping)(struct watchdog_device *); unsigned int (*status)(struct watchdog_device *); int (*set_timeout)(struct watchdog_device *, unsigned int); + int (*set_pretimeout)(struct watchdog_device *, unsigned int); unsigned int (*get_timeleft)(struct watchdog_device *); void (*ref)(struct watchdog_device *); void (*unref)(struct watchdog_device *); @@ -156,13 +163,24 @@ they are supported. These optional routines/operations are: * status: this routine checks the status of the watchdog timer device. The status of the device is reported with watchdog WDIOF_* status flags/bits. * set_timeout: this routine checks and changes the timeout of the watchdog - timer device. It returns 0 on success, -EINVAL for "parameter out of range" - and -EIO for "could not write value to the watchdog". On success this - routine should set the timeout value of the watchdog_device to the - achieved timeout value (which may be different from the requested one - because the watchdog does not necessarily has a 1 second resolution). + timer device. It returns 0 on success, -EINVAL for "parameter out of + range" (e.g., if the driver supports pretimeout, then the requested + timeout value must be greater than the pretimeout value) and -EIO for + "could not write value to the watchdog". On success this routine will + set the timeout value of the watchdog_device to an actual timeout value + (which may be different from the requested one because the watchdog does + not necessarily have a 1 second resolution). (Note: the WDIOF_SETTIMEOUT needs to be set in the options field of the watchdog's info structure). +* set_pretimeout: this routine checks and changes the pretimeout of the + watchdog timer device. It returns 0 on success, -EINVAL for "parameter + out of range" and -EIO for "could not write value to the watchdog". On + success this routine will set the pretimeout value of the + watchdog_device to an actual pretimeout value (which may be different + from the requested one because the watchdog does not necessarily have a + 1 second resolution). + (Note: the WDIOF_PRETIMEOUT needs to be set in the options field of the + watchdog's info structure). * get_timeleft: this routines returns the time that's left before a reset. * ref: the operation that calls kref_get on the kref of a dynamically allocated watchdog_device struct. @@ -226,8 +244,27 @@ extern int watchdog_init_timeout(struct watchdog_device *wdd, unsigned int timeout_parm, struct device *dev);
The watchdog_init_timeout function allows you to initialize the timeout field -using the module timeout parameter or by retrieving the timeout-sec property from -the device tree (if the module timeout parameter is invalid). Best practice is -to set the default timeout value as timeout value in the watchdog_device and -then use this function to set the user "preferred" timeout value. +using the module timeout parameter or by retrieving the first element of +the timeout-sec property from the device tree (if the module timeout +parameter is invalid). Best practice is to set the default timeout value +as the timeout value in the watchdog_device and then use this function to +set the user preferred timeout value. +This routine returns zero on success and a negative errno code for failure. + +Some watchdog timers have two stages of timeout (timeout and pretimeout). +To initialize the timeout and pretimeout fields at the same time, the +following function can be used: + +int watchdog_init_timeouts(struct watchdog_device *wdd, + unsigned int pretimeout_parm, + unsigned int timeout_parm, + struct device *dev); + +The watchdog_init_timeouts function allows you to initialize the +pretimeout and timeout fields using the module pretimeout and timeout +parameter or by retrieving the elements in the timeout-sec property from +the device tree (if the module pretimeout and timeout parameter are +invalid). Best practice is to set the default pretimeout and timeout +values as pretimeout and timeout values in the watchdog_device and then +use this function to set the user preferred pretimeout value. This routine returns zero on success and a negative errno code for failure. diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c index 1a80594..44918d5 100644 --- a/drivers/watchdog/watchdog_core.c +++ b/drivers/watchdog/watchdog_core.c @@ -85,57 +85,126 @@ static void watchdog_deferred_registration_del(struct watchdog_device *wdd) static void watchdog_check_min_max_timeout(struct watchdog_device *wdd) { /* - * Check that we have valid min and max timeout values, if - * not reset them both to 0 (=not used or unknown) + * Check that we have valid min and max pretimeout and timeout values, + * if not, reset them all to 0 (=not used or unknown) */ - if (wdd->min_timeout > wdd->max_timeout) { - pr_info("Invalid min and max timeout values, resetting to 0!\n"); + if (wdd->min_pretimeout > wdd->max_pretimeout || + wdd->min_timeout > wdd->max_timeout || + wdd->min_timeout < wdd->min_pretimeout || + wdd->max_timeout < wdd->max_pretimeout) { + pr_info("Invalid min or max timeouts, resetting to 0\n"); + wdd->min_pretimeout = 0; + wdd->max_pretimeout = 0; wdd->min_timeout = 0; wdd->max_timeout = 0; } }
/** - * watchdog_init_timeout() - initialize the timeout field + * watchdog_init_timeouts() - initialize the pretimeout and timeout field + * @pretimeout_parm: pretimeout module parameter * @timeout_parm: timeout module parameter * @dev: Device that stores the timeout-sec property * - * Initialize the timeout field of the watchdog_device struct with either the - * timeout module parameter (if it is valid value) or the timeout-sec property - * (only if it is a valid value and the timeout_parm is out of bounds). - * If none of them are valid then we keep the old value (which should normally - * be the default timeout value. + * Initialize the pretimeout and timeout field of the watchdog_device struct + * with both the pretimeout and timeout module parameters (if they are valid) or + * the timeout-sec property (only if they are valid and the pretimeout_parm or + * timeout_parm is out of bounds). If one of them is invalid, then we keep + * the old value (which should normally be the default timeout value). * * A zero is returned on success and -EINVAL for failure. */ -int watchdog_init_timeout(struct watchdog_device *wdd, - unsigned int timeout_parm, struct device *dev) +int watchdog_init_timeouts(struct watchdog_device *wdd, + unsigned int pretimeout_parm, + unsigned int timeout_parm, + struct device *dev) { - unsigned int t = 0; - int ret = 0; + int ret = 0, length = 0; + u32 timeouts[2] = {0}; + struct property *prop;
watchdog_check_min_max_timeout(wdd);
- /* try to get the timeout module parameter first */ - if (!watchdog_timeout_invalid(wdd, timeout_parm) && timeout_parm) { - wdd->timeout = timeout_parm; - return ret; - } - if (timeout_parm) + /* + * Backup the timeouts of wdd, and set them to the parameters, + * because watchdog_pretimeout_invalid uses wdd->timeout to validate + * the pretimeout_parm, and watchdog_timeout_invalid uses + * wdd->pretimeout to validate timeout_parm. + * if any of parameters is wrong, restore the default values before + * return. + */ + timeouts[0] = wdd->timeout; + timeouts[1] = wdd->pretimeout; + wdd->timeout = timeout_parm; + wdd->pretimeout = pretimeout_parm; + + /* + * Try to get the pretimeout module parameter first. + * Note: zero is a valid value for pretimeout. + */ + if (watchdog_pretimeout_invalid(wdd, pretimeout_parm)) ret = -EINVAL;
- /* try to get the timeout_sec property */ - if (dev == NULL || dev->of_node == NULL) - return ret; - of_property_read_u32(dev->of_node, "timeout-sec", &t); - if (!watchdog_timeout_invalid(wdd, t) && t) - wdd->timeout = t; - else + /* + * Try to get the timeout module parameter, + * if it's valid and pretimeout is valid(ret == 0), + * assignment and return zero. Otherwise, try dtb. + */ + if (timeout_parm && !ret) { + if (!watchdog_timeout_invalid(wdd, timeout_parm)) + return 0; ret = -EINVAL; + }
- return ret; + /* + * Either at least one of the module parameters is invalid, + * or timeout_parm is 0. Try to get the timeout_sec property. + */ + if (!dev || !dev->of_node) { + wdd->timeout = timeouts[0]; + wdd->pretimeout = timeouts[1]; + return ret; + } + + /* + * Backup default values to *_parms, + * timeouts[] will be used by of_property_read_u32_array. + */ + timeout_parm = timeouts[0]; + pretimeout_parm = timeouts[1]; + + prop = of_find_property(dev->of_node, "timeout-sec", &length); + if (prop && length > 0 && length <= sizeof(u32) * 2) { + of_property_read_u32_array(dev->of_node, + "timeout-sec", timeouts, + length / sizeof(u32)); + wdd->timeout = timeouts[0]; + wdd->pretimeout = timeouts[1]; + + if (length == sizeof(u32) * 2) { + if (watchdog_pretimeout_invalid(wdd, timeouts[1])) + goto error; + ret = 0; + } else { + ret = -EINVAL; + } + + if (!watchdog_timeout_invalid(wdd, timeouts[0]) && + timeouts[0]) { + if (ret) /* Only one value in "timeout-sec" */ + wdd->pretimeout = pretimeout_parm; + return 0; + } + } + +error: + /* restore default values */ + wdd->timeout = timeout_parm; + wdd->pretimeout = pretimeout_parm; + + return -EINVAL; } -EXPORT_SYMBOL_GPL(watchdog_init_timeout); +EXPORT_SYMBOL_GPL(watchdog_init_timeouts);
static int __watchdog_register_device(struct watchdog_device *wdd) { diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c index 6aaefba..af0777e 100644 --- a/drivers/watchdog/watchdog_dev.c +++ b/drivers/watchdog/watchdog_dev.c @@ -218,6 +218,38 @@ out_timeout: }
/* + * watchdog_set_pretimeout: set the watchdog timer pretimeout + * @wddev: the watchdog device to set the timeout for + * @pretimeout: pretimeout to set in seconds + */ + +static int watchdog_set_pretimeout(struct watchdog_device *wddev, + unsigned int pretimeout) +{ + int err; + + if (!wddev->ops->set_pretimeout || + !(wddev->info->options & WDIOF_PRETIMEOUT)) + return -EOPNOTSUPP; + + if (watchdog_pretimeout_invalid(wddev, pretimeout)) + return -EINVAL; + + mutex_lock(&wddev->lock); + + if (test_bit(WDOG_UNREGISTERED, &wddev->status)) { + err = -ENODEV; + goto out_pretimeout; + } + + err = wddev->ops->set_pretimeout(wddev, pretimeout); + +out_pretimeout: + mutex_unlock(&wddev->lock); + return err; +} + +/* * watchdog_get_timeleft: wrapper to get the time left before a reboot * @wddev: the watchdog device to get the remaining time from * @timeleft: the time that's left @@ -388,6 +420,27 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd, if (wdd->timeout == 0) return -EOPNOTSUPP; return put_user(wdd->timeout, p); + case WDIOC_SETPRETIMEOUT: + /* check if we support the pretimeout */ + if (!(wdd->info->options & WDIOF_PRETIMEOUT)) + return -EOPNOTSUPP; + if (get_user(val, p)) + return -EFAULT; + err = watchdog_set_pretimeout(wdd, val); + if (err < 0) + return err; + /* + * If the watchdog is active then we send a keepalive ping + * to make sure that the watchdog keeps running (and if + * possible that it takes the new pretimeout) + */ + watchdog_ping(wdd); + /* Fall */ + case WDIOC_GETPRETIMEOUT: + /* check if we support the pretimeout */ + if (wdd->info->options & WDIOF_PRETIMEOUT) + return put_user(wdd->pretimeout, p); + return -EOPNOTSUPP; case WDIOC_GETTIMELEFT: err = watchdog_get_timeleft(wdd, &val); if (err) diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h index d74a0e9..eaea7b0 100644 --- a/include/linux/watchdog.h +++ b/include/linux/watchdog.h @@ -25,6 +25,7 @@ struct watchdog_device; * @ping: The routine that sends a keepalive ping to the watchdog device. * @status: The routine that shows the status of the watchdog device. * @set_timeout:The routine for setting the watchdog devices timeout value. + * @set_pretimeout:The routine for setting the watchdog devices pretimeout value * @get_timeleft:The routine that get's the time that's left before a reset. * @ref: The ref operation for dyn. allocated watchdog_device structs * @unref: The unref operation for dyn. allocated watchdog_device structs @@ -44,6 +45,7 @@ struct watchdog_ops { int (*ping)(struct watchdog_device *); unsigned int (*status)(struct watchdog_device *); int (*set_timeout)(struct watchdog_device *, unsigned int); + int (*set_pretimeout)(struct watchdog_device *, unsigned int); unsigned int (*get_timeleft)(struct watchdog_device *); void (*ref)(struct watchdog_device *); void (*unref)(struct watchdog_device *); @@ -62,6 +64,9 @@ struct watchdog_ops { * @timeout: The watchdog devices timeout value. * @min_timeout:The watchdog devices minimum timeout value. * @max_timeout:The watchdog devices maximum timeout value. + * @pretimeout: The watchdog devices pretimeout value. + * @min_pretimeout:The watchdog devices minimum pretimeout value. + * @max_pretimeout:The watchdog devices maximum pretimeout value. * @driver-data:Pointer to the drivers private data. * @lock: Lock for watchdog core internal use only. * @status: Field that contains the devices internal status bits. @@ -88,6 +93,9 @@ struct watchdog_device { unsigned int timeout; unsigned int min_timeout; unsigned int max_timeout; + unsigned int pretimeout; + unsigned int min_pretimeout; + unsigned int max_pretimeout; void *driver_data; struct mutex lock; unsigned long status; @@ -119,8 +127,22 @@ static inline void watchdog_set_nowayout(struct watchdog_device *wdd, bool noway /* Use the following function to check if a timeout value is invalid */ static inline bool watchdog_timeout_invalid(struct watchdog_device *wdd, unsigned int t) { - return ((wdd->max_timeout != 0) && - (t < wdd->min_timeout || t > wdd->max_timeout)); + return (wdd->max_timeout && + (t < wdd->min_timeout || t > wdd->max_timeout)) || + (wdd->pretimeout && t <= wdd->pretimeout); +} + +/* + * Use the following function to check if a pretimeout value is invalid. + * It can be "0", that means we don't use pretimeout. + * This function returns false, when pretimeout is 0. + */ +static inline bool watchdog_pretimeout_invalid(struct watchdog_device *wdd, + unsigned int t) +{ + return t && ((wdd->max_pretimeout && + (t < wdd->min_pretimeout || t > wdd->max_pretimeout)) || + (wdd->timeout && t >= wdd->timeout)); }
/* Use the following functions to manipulate watchdog driver specific data */ @@ -135,8 +157,17 @@ static inline void *watchdog_get_drvdata(struct watchdog_device *wdd) }
/* drivers/watchdog/watchdog_core.c */ -extern int watchdog_init_timeout(struct watchdog_device *wdd, - unsigned int timeout_parm, struct device *dev); +int watchdog_init_timeouts(struct watchdog_device *wdd, + unsigned int pretimeout_parm, + unsigned int timeout_parm, + struct device *dev); +static inline int watchdog_init_timeout(struct watchdog_device *wdd, + unsigned int timeout_parm, + struct device *dev) +{ + return watchdog_init_timeouts(wdd, 0, timeout_parm, dev); +} + extern int watchdog_register_device(struct watchdog_device *); extern void watchdog_unregister_device(struct watchdog_device *);
From: Fu Wei fu.wei@linaro.org
This driver bases on linux kernel watchdog framework, and use "pretimeout" in the framework. It supports getting timeout and pretimeout from parameter and FDT at the driver init stage. In first timeout, the interrupt routine run panic to save system context.
Signed-off-by: Fu Wei fu.wei@linaro.org Tested-by: Pratyush Anand panand@redhat.com --- drivers/watchdog/Kconfig | 14 ++ drivers/watchdog/Makefile | 1 + drivers/watchdog/sbsa_gwdt.c | 459 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 474 insertions(+)
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index 79e1aa1..5cc8455 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -173,6 +173,20 @@ config ARM_SP805_WATCHDOG ARM Primecell SP805 Watchdog timer. This will reboot your system when the timeout is reached.
+config ARM_SBSA_WATCHDOG + tristate "ARM SBSA Generic Watchdog" + depends on ARM64 + depends on ARM_ARCH_TIMER + select WATCHDOG_CORE + help + ARM SBSA Generic Watchdog. This watchdog has two Watchdog timeouts. + The first timeout will trigger a panic; the second timeout will + trigger a system reset. + More details: ARM DEN0029B - Server Base System Architecture (SBSA) + + To compile this driver as module, choose M here: The module + will be called sbsa_gwdt. + config AT91RM9200_WATCHDOG tristate "AT91RM9200 watchdog" depends on SOC_AT91RM9200 && MFD_SYSCON diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index 0c616e3..b74a3ea 100644 --- a/drivers/watchdog/Makefile +++ b/drivers/watchdog/Makefile @@ -30,6 +30,7 @@ obj-$(CONFIG_USBPCWATCHDOG) += pcwd_usb.o
# ARM Architecture obj-$(CONFIG_ARM_SP805_WATCHDOG) += sp805_wdt.o +obj-$(CONFIG_ARM_SBSA_WATCHDOG) += sbsa_gwdt.o obj-$(CONFIG_AT91RM9200_WATCHDOG) += at91rm9200_wdt.o obj-$(CONFIG_AT91SAM9X_WATCHDOG) += at91sam9_wdt.o obj-$(CONFIG_CADENCE_WATCHDOG) += cadence_wdt.o diff --git a/drivers/watchdog/sbsa_gwdt.c b/drivers/watchdog/sbsa_gwdt.c new file mode 100644 index 0000000..6fd1c63 --- /dev/null +++ b/drivers/watchdog/sbsa_gwdt.c @@ -0,0 +1,459 @@ +/* + * SBSA(Server Base System Architecture) Generic Watchdog driver + * + * Copyright (c) 2015, Linaro Ltd. + * Author: Fu Wei fu.wei@linaro.org + * Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License 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. + * + * The SBSA Generic watchdog driver is compatible with the pretimeout + * concept of Linux kernel. + * The timeout and pretimeout are determined by WCV or WOR. + * The first watch period is set by writing WCV directly, that can + * support more than 10s timeout at the maximum system counter + * frequency (400MHz). + * When WS0 is triggered, the second watch period (pretimeout) is + * determined by one of these registers: + * (1)WOR: 32bit register, this gives a maximum watch period of + * around 10s at the maximum system counter frequency. It's loaded + * automatically by hardware. + * (2)WCV: If the pretimeout value is greater then "max_wor_timeout", + * it will be loaded in WS0 interrupt routine. If system is in + * ws0_mode (reboot with watchdog enabled and WS0 == true), the ping + * operation will only reload WCV. + * More details about the hardware specification of this device: + * ARM DEN0029B - Server Base System Architecture (SBSA) + * + * Kernel/API: P------------------| pretimeout + * |----------------------------------------T timeout + * SBSA GWDT: P---WOR (or WCV)---WS1 pretimeout + * |-------WCV----------WS0~~~(ws0_mode)~~~~T timeout + */ + +#include <linux/io.h> +#include <linux/interrupt.h> +#include <linux/module.h> +#include <linux/moduleparam.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/platform_device.h> +#include <linux/uaccess.h> +#include <linux/watchdog.h> +#include <asm/arch_timer.h> + +/* SBSA Generic Watchdog register definitions */ +/* refresh frame */ +#define SBSA_GWDT_WRR 0x000 + +/* control frame */ +#define SBSA_GWDT_WCS 0x000 +#define SBSA_GWDT_WOR 0x008 +#define SBSA_GWDT_WCV_LO 0x010 +#define SBSA_GWDT_WCV_HI 0x014 + +/* refresh/control frame */ +#define SBSA_GWDT_W_IIDR 0xfcc +#define SBSA_GWDT_IDR 0xfd0 + +/* Watchdog Control and Status Register */ +#define SBSA_GWDT_WCS_EN BIT(0) +#define SBSA_GWDT_WCS_WS0 BIT(1) +#define SBSA_GWDT_WCS_WS1 BIT(2) + +/** + * struct sbsa_gwdt - Internal representation of the SBSA GWDT + * @wdd: kernel watchdog_device structure + * @clk: store the System Counter clock frequency, in Hz. + * @ws0_mode: indicate the system boot in the second stage timeout. + * @max_wor_timeout: the maximum timeout value for WOR (in seconds). + * @refresh_base: Virtual address of the watchdog refresh frame + * @control_base: Virtual address of the watchdog control frame + */ +struct sbsa_gwdt { + struct watchdog_device wdd; + u32 clk; + bool ws0_mode; + int max_wor_timeout; + void __iomem *refresh_base; + void __iomem *control_base; +}; + +#define to_sbsa_gwdt(e) container_of(e, struct sbsa_gwdt, wdd) + +#define DEFAULT_TIMEOUT 60 /* seconds, the 1st + 2nd watch periods*/ +#define DEFAULT_PRETIMEOUT 30 /* seconds, the 2nd watch period*/ + +static unsigned int timeout; +module_param(timeout, uint, 0); +MODULE_PARM_DESC(timeout, + "Watchdog timeout in seconds. (>=0, default=" + __MODULE_STRING(DEFAULT_TIMEOUT) ")"); + +static unsigned int pretimeout; +module_param(pretimeout, uint, 0); +MODULE_PARM_DESC(pretimeout, + "Watchdog pretimeout in seconds. (>=0, default=" + __MODULE_STRING(DEFAULT_PRETIMEOUT) ")"); + +static bool nowayout = WATCHDOG_NOWAYOUT; +module_param(nowayout, bool, S_IRUGO); +MODULE_PARM_DESC(nowayout, + "Watchdog cannot be stopped once started (default=" + __MODULE_STRING(WATCHDOG_NOWAYOUT) ")"); + +/* + * help functions for accessing 64bit WCV register + */ +static u64 sbsa_gwdt_get_wcv(struct watchdog_device *wdd) +{ + u32 wcv_lo, wcv_hi; + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd); + + do { + wcv_hi = readl_relaxed(gwdt->control_base + SBSA_GWDT_WCV_HI); + wcv_lo = readl_relaxed(gwdt->control_base + SBSA_GWDT_WCV_LO); + } while (wcv_hi != readl_relaxed(gwdt->control_base + + SBSA_GWDT_WCV_HI)); + + return (((u64)wcv_hi << 32) | wcv_lo); +} + +static void sbsa_gwdt_set_wcv(struct watchdog_device *wdd, unsigned int t) +{ + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd); + u64 wcv; + + wcv = arch_counter_get_cntvct() + (u64)t * gwdt->clk; + + writel_relaxed(upper_32_bits(wcv), + gwdt->control_base + SBSA_GWDT_WCV_HI); + writel_relaxed(lower_32_bits(wcv), + gwdt->control_base + SBSA_GWDT_WCV_LO); +} + +/* + * inline functions for reloading 64bit WCV register + */ +static inline void reload_pretimeout_to_wcv(struct watchdog_device *wdd) +{ + sbsa_gwdt_set_wcv(wdd, wdd->pretimeout); +} + +static inline void reload_first_stage_to_wcv(struct watchdog_device *wdd) +{ + sbsa_gwdt_set_wcv(wdd, wdd->timeout - wdd->pretimeout); +} + +/* + * watchdog operation functions + */ +static int sbsa_gwdt_set_timeout(struct watchdog_device *wdd, + unsigned int timeout) +{ + wdd->timeout = timeout; + + return 0; +} + +static int sbsa_gwdt_set_pretimeout(struct watchdog_device *wdd, + unsigned int pretimeout) +{ + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd); + u32 wor; + + wdd->pretimeout = pretimeout; + + /* If ws0_mode == true, we won't touch WOR */ + if (!gwdt->ws0_mode) { + if (!pretimeout) + /* + * If pretimeout is 0, it gives driver a timeslot (1s) + * to update WCV after an explicit refresh + * (sbsa_gwdt_start) + */ + wor = gwdt->clk; + else + if (pretimeout > gwdt->max_wor_timeout) + wor = U32_MAX; + else + wor = pretimeout * gwdt->clk; + + /* wtite WOR, that will cause an explicit watchdog refresh */ + writel_relaxed(wor, gwdt->control_base + SBSA_GWDT_WOR); + } + + return 0; +} + +static unsigned int sbsa_gwdt_get_timeleft(struct watchdog_device *wdd) +{ + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd); + u64 timeleft = sbsa_gwdt_get_wcv(wdd) - arch_counter_get_cntvct(); + + do_div(timeleft, gwdt->clk); + + return timeleft; +} + +static int sbsa_gwdt_keepalive(struct watchdog_device *wdd) +{ + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd); + + if (gwdt->ws0_mode) + reload_pretimeout_to_wcv(wdd); + else + reload_first_stage_to_wcv(wdd); + + return 0; +} + +static int sbsa_gwdt_start(struct watchdog_device *wdd) +{ + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd); + + /* If ws0_mode == true, the watchdog is enabled */ + if (!gwdt->ws0_mode) + /* writing WCS will cause an explicit watchdog refresh */ + writel_relaxed(SBSA_GWDT_WCS_EN, + gwdt->control_base + SBSA_GWDT_WCS); + + return sbsa_gwdt_keepalive(wdd); +} + +static int sbsa_gwdt_stop(struct watchdog_device *wdd) +{ + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd); + + writel_relaxed(0, gwdt->control_base + SBSA_GWDT_WCS); + /* + * Writing WCS has caused an explicit watchdog refresh. + * Both watchdog signals are deasserted, so clean ws0_mode flag. + */ + gwdt->ws0_mode = false; + + return 0; +} + +static irqreturn_t sbsa_gwdt_interrupt(int irq, void *dev_id) +{ + struct sbsa_gwdt *gwdt = (struct sbsa_gwdt *)dev_id; + struct watchdog_device *wdd = &gwdt->wdd; + + /* We don't use pretimeout, trigger WS1 now */ + if (!wdd->pretimeout) + sbsa_gwdt_set_wcv(wdd, 0); + + /* + * The pretimeout is valid, go panic + * If pretimeout is greater then "max_wor_timeout", + * reload the right value to WCV, then panic + */ + if (wdd->pretimeout > gwdt->max_wor_timeout) + reload_pretimeout_to_wcv(wdd); + panic("SBSA Watchdog pre-timeout"); + + return IRQ_HANDLED; +} + +static struct watchdog_info sbsa_gwdt_info = { + .identity = "SBSA Generic Watchdog", + .options = WDIOF_SETTIMEOUT | + WDIOF_KEEPALIVEPING | + WDIOF_MAGICCLOSE | + WDIOF_PRETIMEOUT | + WDIOF_CARDRESET, +}; + +static struct watchdog_ops sbsa_gwdt_ops = { + .owner = THIS_MODULE, + .start = sbsa_gwdt_start, + .stop = sbsa_gwdt_stop, + .ping = sbsa_gwdt_keepalive, + .set_timeout = sbsa_gwdt_set_timeout, + .set_pretimeout = sbsa_gwdt_set_pretimeout, + .get_timeleft = sbsa_gwdt_get_timeleft, +}; + +static int sbsa_gwdt_probe(struct platform_device *pdev) +{ + void __iomem *rf_base, *cf_base; + struct device *dev = &pdev->dev; + struct watchdog_device *wdd; + struct sbsa_gwdt *gwdt; + struct resource *res; + int ret, irq; + u32 status; + + gwdt = devm_kzalloc(dev, sizeof(*gwdt), GFP_KERNEL); + if (!gwdt) + return -ENOMEM; + platform_set_drvdata(pdev, gwdt); + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + cf_base = devm_ioremap_resource(dev, res); + if (IS_ERR(cf_base)) + return PTR_ERR(cf_base); + + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); + rf_base = devm_ioremap_resource(dev, res); + if (IS_ERR(rf_base)) + return PTR_ERR(rf_base); + + irq = platform_get_irq(pdev, 0); + if (irq < 0) { + dev_err(dev, "unable to get ws0 interrupt.\n"); + return irq; + } + + /* + * Get the frequency of system counter from the cp15 interface of ARM + * Generic timer. We don't need to check it, because if it returns "0", + * system would panic in very early stage. + */ + gwdt->clk = arch_timer_get_cntfrq(); + gwdt->refresh_base = rf_base; + gwdt->control_base = cf_base; + gwdt->max_wor_timeout = U32_MAX / gwdt->clk; + gwdt->ws0_mode = false; + + wdd = &gwdt->wdd; + wdd->parent = dev; + wdd->info = &sbsa_gwdt_info; + wdd->ops = &sbsa_gwdt_ops; + watchdog_set_drvdata(wdd, gwdt); + watchdog_set_nowayout(wdd, nowayout); + + wdd->min_pretimeout = 0; + wdd->min_timeout = 1; + + /* + * Because the maximum of gwdt->clk is 400MHz and the maximum of WCV is + * U64_MAX, so the result of (U64_MAX / gwdt->clk) is always greater + * than U32_MAX. And the maximum of "unsigned int" is U32_MAX on ARM64. + * So we set the maximum value of pretimeout and timeout below. + */ + wdd->max_pretimeout = U32_MAX - 1; + wdd->max_timeout = U32_MAX; + + wdd->pretimeout = DEFAULT_PRETIMEOUT; + wdd->timeout = DEFAULT_TIMEOUT; + watchdog_init_timeouts(wdd, pretimeout, timeout, dev); + + status = readl_relaxed(gwdt->control_base + SBSA_GWDT_WCS); + if (status & SBSA_GWDT_WCS_WS1) { + dev_warn(dev, "System reset by WDT.\n"); + wdd->bootstatus |= WDIOF_CARDRESET; + } else if (status == (SBSA_GWDT_WCS_WS0 | SBSA_GWDT_WCS_EN)) { + gwdt->ws0_mode = true; + } + + ret = devm_request_irq(dev, irq, sbsa_gwdt_interrupt, 0, + pdev->name, gwdt); + if (ret) { + dev_err(dev, "unable to request IRQ %d\n", irq); + return ret; + } + + ret = watchdog_register_device(wdd); + if (ret) + return ret; + + /* If ws0_mode == true, the line won't update WOR */ + sbsa_gwdt_set_pretimeout(wdd, wdd->pretimeout); + + /* + * If watchdog is already enabled, do a ping operation + * to keep system running + */ + if (status & SBSA_GWDT_WCS_EN) + sbsa_gwdt_keepalive(wdd); + + dev_info(dev, "Initialized with %ds timeout, %ds pretimeout @ %u Hz%s\n", + wdd->timeout, wdd->pretimeout, gwdt->clk, + status & SBSA_GWDT_WCS_EN ? + gwdt->ws0_mode ? " [second stage]" : " [enabled]" : + ""); + + return 0; +} + +static void sbsa_gwdt_shutdown(struct platform_device *pdev) +{ + struct sbsa_gwdt *gwdt = platform_get_drvdata(pdev); + + sbsa_gwdt_stop(&gwdt->wdd); +} + +static int sbsa_gwdt_remove(struct platform_device *pdev) +{ + struct sbsa_gwdt *gwdt = platform_get_drvdata(pdev); + + watchdog_unregister_device(&gwdt->wdd); + + return 0; +} + +/* Disable watchdog if it is active during suspend */ +static int __maybe_unused sbsa_gwdt_suspend(struct device *dev) +{ + struct sbsa_gwdt *gwdt = dev_get_drvdata(dev); + + if (watchdog_active(&gwdt->wdd)) + sbsa_gwdt_stop(&gwdt->wdd); + + return 0; +} + +/* Enable watchdog and configure it if necessary */ +static int __maybe_unused sbsa_gwdt_resume(struct device *dev) +{ + struct sbsa_gwdt *gwdt = dev_get_drvdata(dev); + + if (watchdog_active(&gwdt->wdd)) + sbsa_gwdt_start(&gwdt->wdd); + + return 0; +} + +static const struct dev_pm_ops sbsa_gwdt_pm_ops = { + SET_SYSTEM_SLEEP_PM_OPS(sbsa_gwdt_suspend, sbsa_gwdt_resume) +}; + +static const struct of_device_id sbsa_gwdt_of_match[] = { + { .compatible = "arm,sbsa-gwdt", }, + {}, +}; +MODULE_DEVICE_TABLE(of, sbsa_gwdt_of_match); + +static const struct platform_device_id sbsa_gwdt_pdev_match[] = { + { .name = "sbsa-gwdt", }, + {}, +}; +MODULE_DEVICE_TABLE(platform, sbsa_gwdt_pdev_match); + +static struct platform_driver sbsa_gwdt_driver = { + .driver = { + .name = "sbsa-gwdt", + .pm = &sbsa_gwdt_pm_ops, + .of_match_table = sbsa_gwdt_of_match, + }, + .probe = sbsa_gwdt_probe, + .remove = sbsa_gwdt_remove, + .shutdown = sbsa_gwdt_shutdown, + .id_table = sbsa_gwdt_pdev_match, +}; + +module_platform_driver(sbsa_gwdt_driver); + +MODULE_DESCRIPTION("SBSA Generic Watchdog Driver"); +MODULE_AUTHOR("Fu Wei fu.wei@linaro.org"); +MODULE_AUTHOR("Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com"); +MODULE_LICENSE("GPL v2");
On Tue, Oct 27, 2015 at 11:06 AM, fu.wei@linaro.org wrote:
+static irqreturn_t sbsa_gwdt_interrupt(int irq, void *dev_id) +{
struct sbsa_gwdt *gwdt = (struct sbsa_gwdt *)dev_id;struct watchdog_device *wdd = &gwdt->wdd;/* We don't use pretimeout, trigger WS1 now */if (!wdd->pretimeout)sbsa_gwdt_set_wcv(wdd, 0);
So I'm still concerned about the fact this driver depends on an interrupt handler in order to properly program the hardware. Unlike some other devices, the SBSA watchdog does not need assistance to reset on a timeout -- it is a "fire and forget" device. What happens if there is a hard lockup, and interrupts no longer work?
Keep in mind that 99% of watchdog daemons will not enable the pre-timeout feature because it's new.
On 11/04/2015 05:59 PM, Timur Tabi wrote:
On Tue, Oct 27, 2015 at 11:06 AM, fu.wei@linaro.org wrote:
+static irqreturn_t sbsa_gwdt_interrupt(int irq, void *dev_id) +{
struct sbsa_gwdt *gwdt = (struct sbsa_gwdt *)dev_id;struct watchdog_device *wdd = &gwdt->wdd;/* We don't use pretimeout, trigger WS1 now */if (!wdd->pretimeout)sbsa_gwdt_set_wcv(wdd, 0);So I'm still concerned about the fact this driver depends on an interrupt handler in order to properly program the hardware. Unlike some other devices, the SBSA watchdog does not need assistance to reset on a timeout -- it is a "fire and forget" device. What happens if there is a hard lockup, and interrupts no longer work?
Keep in mind that 99% of watchdog daemons will not enable the pre-timeout feature because it's new.
Same here, really.
I would feel much more comfortable if the driver would just use the standard watchdog timeout and live with (worst case) 20 seconds timeout for now. This limitation will be gone once the infrastructure is in place to handle such short timeouts in the watchdog core. Until then, I would argue that the system designers asked for it if they really select the highest possible clock rate.
Guenter
Hi Guenter,
Great thanks for your feedback!
On 5 November 2015 at 13:13, Guenter Roeck linux@roeck-us.net wrote:
On 11/04/2015 05:59 PM, Timur Tabi wrote:
On Tue, Oct 27, 2015 at 11:06 AM, fu.wei@linaro.org wrote:
+static irqreturn_t sbsa_gwdt_interrupt(int irq, void *dev_id) +{
struct sbsa_gwdt *gwdt = (struct sbsa_gwdt *)dev_id;struct watchdog_device *wdd = &gwdt->wdd;/* We don't use pretimeout, trigger WS1 now */if (!wdd->pretimeout)sbsa_gwdt_set_wcv(wdd, 0);So I'm still concerned about the fact this driver depends on an interrupt handler in order to properly program the hardware. Unlike some other devices, the SBSA watchdog does not need assistance to reset on a timeout -- it is a "fire and forget" device. What happens if there is a hard lockup, and interrupts no longer work?
The reason for this design(program WCV in interrupt handler): (1) if we don't, the second timeout stage(pretimeout) is only (worst case) 10 seconds This short time is not enough for kexec(let alone kdump), that make panic less useful. (2)if a hard lockup really happens, panic won't work too.But we still can reboot system by the help of WS1 in this case, if clk is 400MHz, we just need to wait (worst case) 10 seconds for WS1 reboot system
Keep in mind that 99% of watchdog daemons will not enable the pre-timeout feature because it's new.
Answer: (1)It is not new. pre-timeout concept has been used by two drivers before this driver. and this concept has been in kernel documentation.
(2)even it's new, it doesn't mean we can not do this at this time. Because according to the info I got, I believe that is right way to do. After I make a "non-pretimeout" version. and compare with the original pretimeout version, I still believe pretimeout is best solution for now.
Reason for using pretimeout: (1) if we don't, for this two stages timeout, we have to config them by one value. that means "the first stage timeout" have to be equal to "the second stage timeout", For example, if we need 60 second for "the second stage timeout", 30 or less for "the first stage timeout". then "the first stage timeout" have to be 60s too. I don't think it 's good idea.
Same here, really.
I would feel much more comfortable if the driver would just use the standard watchdog timeout and live with (worst case) 20 seconds timeout for now.
The worst case is 10s. like I said above, This short time is not enough for kexec(let alone kdump), that make WS0(and panic, even this two stages design) less useful
This limitation will be gone once the infrastructure is in place to handle such short timeouts in the watchdog core. Until then, I would argue that the
unless WOR become 64 bit (or more then 32bit), this limitation will be there.
system designers asked for it if they really select the highest possible clock rate.
even we can make clk to be 100MHz or lower, it is not very helpful for a really server which has big memory. they need more time for dumping memory for debug/analysis
Guenter
Fu Wei wrote:
(1)It is not new. pre-timeout concept has been used by two drivers before this driver. and this concept has been in kernel documentation.
It's "new" in that it's a new infrastructure. The private API of two other drivers doesn't count.
(1) if we don't, for this two stages timeout, we have to config them by one value. that means "the first stage timeout" have to be equal to "the second stage timeout", For example, if we need 60 second for "the second stage timeout", 30 or less for "the first stage timeout". then "the first stage timeout" have to be 60s too. I don't think it 's good idea.
Why do we care about two stages? Don't have a pre-timeout, and just have one stage: the WS1 reset. Ignore the WS0 interrupt, and program the timeout so that WS1 is the reset.
I'm not saying that pre-timeout is a terrible idea and we should never do it. I'm saying that it's not an important feature, and we should only support it to the extent that the hardware provides the feature. We should definitely not make the driver more complicated and less safe.
If we agree that an SBSA watchdog allows for a pre-timeout at half-way through timeout, and that software can't change this, then we can use WS0 as the pre-timeout and applications just have to deal with that. The hardware is programmed to reset via WS1, and all we do in the interrupt handler is notify the application that a pre-timeout has occurred.
Guenter Roeck wrote:
I would feel much more comfortable if the driver would just use the standard watchdog timeout and live with (worst case) 20 seconds timeout for now.
Actually, I'm wondering where the 20 seconds comes from. When I load my driver on our hardware, it calculates a maximum timeout of 214 seconds, and that's just to WS0.
Hi Timur
On 5 November 2015 at 21:47, Timur Tabi timur@codeaurora.org wrote:
Guenter Roeck wrote:
I would feel much more comfortable if the driver would just use the standard watchdog timeout and live with (worst case) 20 seconds timeout for now.
Actually, I'm wondering where the 20 seconds comes from. When I load my driver on our hardware, it calculates a maximum timeout of 214 seconds, and that's just to WS0.
SBSA 2.3 Page 23 : Note: the watchdog offset register is 32 bits wide. This gives a maximum watch period of around 10s at a system counter frequency of 400MHz. If a larger watch period is required then the compare value can be programmed directly into the compare value register.
214s means your system counter is approximately at 20MHz which is in the range of (10MHz ~ 400MHz)
SBSA 2.3 Page 13 : The System Counter (of the Generic Timer) shall run at a minimum frequency of 10MHz and maximum of 400MHz.
-- Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation.
Fu Wei wrote:
SBSA 2.3 Page 23 : Note: the watchdog offset register is 32 bits wide. This gives a maximum watch period of around 10s at a system counter frequency of 400MHz. If a larger watch period is required then the compare value can be programmed directly into the compare value register.
214s means your system counter is approximately at 20MHz which is in the range of (10MHz ~ 400MHz)
SBSA 2.3 Page 13 : The System Counter (of the Generic Timer) shall run at a minimum frequency of 10MHz and maximum of 400MHz.
Thanks, that explains a lot.
If we expected customers to have a lower system counter frequency, then we wouldn't have to worry about the timeouts being too short. It seems to me that the SBSA spec says that if you want a longer timeout, you have to lower the frequency. We shouldn't be complicating the driver because some customers might not follow the spec.
Hi Timur,
On 5 November 2015 at 22:08, Timur Tabi timur@codeaurora.org wrote:
Fu Wei wrote:
SBSA 2.3 Page 23 : Note: the watchdog offset register is 32 bits wide. This gives a maximum watch period of around 10s at a system counter frequency of 400MHz. If a larger watch period is required then the compare value can be programmed directly into the compare value register.
214s means your system counter is approximately at 20MHz which is in the range of (10MHz ~ 400MHz)
SBSA 2.3 Page 13 : The System Counter (of the Generic Timer) shall run at a minimum frequency of 10MHz and maximum of 400MHz.
Thanks, that explains a lot.
If we expected customers to have a lower system counter frequency, then we wouldn't have to worry about the timeouts being too short. It seems to me that the SBSA spec says that if you want a longer timeout, you have to lower the frequency.
Did you really read the "Note" above???????? OK, let me paste it again and again:
SBSA 2.3 Page 23 : If a larger watch period is required then the compare value can be programmed directly into the compare value register.
We shouldn't be complicating the driver because some customers might not follow the spec.
OK it this customer might not follow the spec, that watchdog is not a SBSA watchdog, So please don't use SBSA watchdog driver on that non-SBSA watchdog device, Thanks a lot
-- Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation.
Fu Wei wrote:
Did you really read the "Note" above???????? OK, let me paste it again and again:
SBSA 2.3 Page 23 : If a larger watch period is required then the compare value can be programmed directly into the compare value register.
Well, okay. Sorry, I should have read what you pasted more closely. But I think that means during initialization, not during the WS0 timeout.
Anyway, I still don't like the fact that you're programming WCV in the interrupt handler, but I'm not going to make a big deal about it any more.
Hi Timur,
On 5 November 2015 at 22:40, Timur Tabi timur@codeaurora.org wrote:
Fu Wei wrote:
Did you really read the "Note" above???????? OK, let me paste it again and again:
SBSA 2.3 Page 23 : If a larger watch period is required then the compare value can be programmed directly into the compare value register.
Well, okay. Sorry, I should have read what you pasted more closely. But I
Thanks for reading it again.
think that means during initialization, not during the WS0 timeout.
I really don't see SBSA say "during initialization, not during the WS0 timeout", please point it out the page number and the line number in SBSA spec. maybe I miss it? Thanks for your help in advance.
Anyway, I still don't like the fact that you're programming WCV in the
"you don't like" doesn't mean "it is wrong" or "we can't do this", so I will keep this way unless we have better idea to extend second stage timeout.
interrupt handler, but I'm not going to make a big deal about it any more.
Deal, Thanks a lot.
-- Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation.
On 11/05/2015 07:00 AM, Fu Wei wrote:
Hi Timur,
On 5 November 2015 at 22:40, Timur Tabi timur@codeaurora.org wrote:
Fu Wei wrote:
Did you really read the "Note" above???????? OK, let me paste it again and again:
SBSA 2.3 Page 23 : If a larger watch period is required then the compare value can be programmed directly into the compare value register.
Well, okay. Sorry, I should have read what you pasted more closely. But I
Thanks for reading it again.
think that means during initialization, not during the WS0 timeout.
I really don't see SBSA say "during initialization, not during the WS0 timeout", please point it out the page number and the line number in SBSA spec. maybe I miss it? Thanks for your help in advance.
Anyway, I still don't like the fact that you're programming WCV in the
"you don't like" doesn't mean "it is wrong" or "we can't do this", so I will keep this way unless we have better idea to extend second stage timeout.
interrupt handler, but I'm not going to make a big deal about it any more.
Deal, Thanks a lot.
The problem with your driver, as I see it, is that dealing with WS0/WS1 and pretimeout makes the driver so complex that, at least for my part, I am very wary about it. The driver could long since have been accepted if it were not for that. Besides that, I really believe that any system designer using the highest permitted frequency should be willing to live with the consequences, and not force the implementation of a a complex driver.
Ultimately, you'll have to decide if you want a simple driver accepted, or a complex driver hanging in the review queue forever.
Thanks, Guenter
Hi Guenter,
Great thanks for that you are still reviewing this patchset, thanks for your patient.
On 6 November 2015 at 00:41, Guenter Roeck linux@roeck-us.net wrote:
On 11/05/2015 07:00 AM, Fu Wei wrote:
Hi Timur,
On 5 November 2015 at 22:40, Timur Tabi timur@codeaurora.org wrote:
Fu Wei wrote:
Did you really read the "Note" above???????? OK, let me paste it again and again:
SBSA 2.3 Page 23 : If a larger watch period is required then the compare value can be programmed directly into the compare value register.
Well, okay. Sorry, I should have read what you pasted more closely. But I
Thanks for reading it again.
think that means during initialization, not during the WS0 timeout.
I really don't see SBSA say "during initialization, not during the WS0 timeout", please point it out the page number and the line number in SBSA spec. maybe I miss it? Thanks for your help in advance.
Anyway, I still don't like the fact that you're programming WCV in the
"you don't like" doesn't mean "it is wrong" or "we can't do this", so I will keep this way unless we have better idea to extend second stage timeout.
interrupt handler, but I'm not going to make a big deal about it any more.
Deal, Thanks a lot.
The problem with your driver, as I see it, is that dealing with WS0/WS1 and pretimeout makes the driver so complex that, at least for my part, I am very wary about it. The driver could long since have been accepted if it were not for that. Besides that, I really believe that any system designer using the highest permitted frequency should be willing to live with the consequences, and not force the implementation of a a complex driver.
yes, comparing with those "one stage" watchdogs driver, I admit my SBSA driver is a little complex this complex come from : (1) In SBSA spec, there are two stage.
because of these two stages, I looked for the solution in the kernel, then I found "pretimeout". I have explained a lot why I decide to use pretimeout which is existing concept in Linux kernel I just tried to introduce a existing concept into watchdog framework. Maybe people will say, there is only two or one drivers use that, but it doesn't mean we can't have more in the future. Maybe people will say, let's do this when we have more two stages watchdog. But if I need this now, why not just make it this time.
if we use this driver as one stage watchdog, it violates the SBSA spec, and it can not be call SBSA watchdog driver.
(2) in SBSA watchdog, WOR is a 32bit register, we have a timeout limitation.
first of all, according to kernel documentation, Documentation/watchdog/watchdog-api.txt ------------- Pretimeouts:
Some watchdog timers can be set to have a trigger go off before the actual time they will reset the system. This can be done with an NMI, interrupt, or other mechanism. This allows Linux to record useful information (like panic information and kernel coredumps) before it resets. --------------- So I decide to use panic() in first timeout handler, then we can use kdump
But in the real practice, 10s is not enough for kexec(let alone kdump), even we can have 100s, 200s, it is not enough for a real server which has huge ram to finish kdump. So I decide to reprogram WCV for longer timer value.
except this two point, there is not more complex than other watchdog drivers.
And for these two complex point, I have explained the reason. And this driver has been test on two platforms
Foundation model Seattle
and will be more.
And this driver has been test with kdump, and works.
Ultimately, you'll have to decide if you want a simple driver accepted, or a complex driver hanging in the review queue forever.
I have tried to simplify this driver, and I understand why we need a simple driver, and why you are wary about it. Even we want "simple" driver, but we can't ignore the SBSA watchdog feature in the spec.
If I can find a better way to solve these two complex point to make this driver simple, I will absolutely do it. But before we get better solution, I can not make a "simple" non-SBSA watchdog driver to be accepted just for "my driver be accepted". at least I need to convince myself: my driver is not just a simple driver, but also a right driver for target device. Maybe it will take a very long time, but at least for now, I believe I am doing the right way.
If you have suggestion to make this driver simple, I absolutely listen to it and try to do it just like previous review.
Again, I appreciate your help and review very much. :-)
Thanks, Guenter
On 11/05/2015 10:41 AM, Guenter Roeck wrote:
Ultimately, you'll have to decide if you want a simple driver accepted, or a complex driver hanging in the review queue forever.
Please note that I did post such a driver back in May:
http://www.spinics.net/lists/linux-watchdog/msg06567.html
Officially I withdrew it in favor of Fu's, but the decision which driver to pick up is not mine to make. I can resubmit driver if you'd like.
Hi Timur
On 6 November 2015 at 01:59, Timur Tabi timur@codeaurora.org wrote:
On 11/05/2015 10:41 AM, Guenter Roeck wrote:
Ultimately, you'll have to decide if you want a simple driver accepted, or a complex driver hanging in the review queue forever.
Please note that I did post such a driver back in May:
http://www.spinics.net/lists/linux-watchdog/msg06567.htmlOfficially I withdrew it in favor of Fu's, but the decision which driver to pick up is not mine to make. I can resubmit driver if you'd like.
I totally can not agree with "make WS1 as backup", and that not a SBSA watchdog dirver, because that just have one stage
And the short timeout is useless for kexec/kdump in real practice.
-- Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
On 11/05/2015 09:41 AM, Guenter Roeck wrote:
On 11/05/2015 07:00 AM, Fu Wei wrote:
Hi Timur,
On 5 November 2015 at 22:40, Timur Tabi timur@codeaurora.org wrote:
Fu Wei wrote:
Did you really read the "Note" above???????? OK, let me paste it again and again:
SBSA 2.3 Page 23 : If a larger watch period is required then the compare value can be programmed directly into the compare value register.
Well, okay. Sorry, I should have read what you pasted more closely. But I
Thanks for reading it again.
think that means during initialization, not during the WS0 timeout.
I really don't see SBSA say "during initialization, not during the WS0 timeout", please point it out the page number and the line number in SBSA spec. maybe I miss it? Thanks for your help in advance.
Anyway, I still don't like the fact that you're programming WCV in the
"you don't like" doesn't mean "it is wrong" or "we can't do this", so I will keep this way unless we have better idea to extend second stage timeout.
interrupt handler, but I'm not going to make a big deal about it any more.
Deal, Thanks a lot.
The problem with your driver, as I see it, is that dealing with WS0/WS1 and pretimeout makes the driver so complex that, at least for my part, I am very wary about it. The driver could long since have been accepted if it were not for that. Besides that, I really believe that any system designer using the highest permitted frequency should be willing to live with the consequences, and not force the implementation of a a complex driver.
Ultimately, you'll have to decide if you want a simple driver accepted, or a complex driver hanging in the review queue forever.
Thanks, Guenter
Sorry to poke my head in late like this, but I do have a vested interest in the outcome so I'm very curious. For my work, I need to have an ACPI-supported, SBSA-compliant watchdog timer for arm64, and this series is one of the key pieces to getting there. The plan for me has been: (1) get an FDT based SBSA watchdog timer, (2) add in kernel code to handle the ACPI GTDT table describing timers, then (3) munge the SBSA watchdog timer for use by ACPI.
So, is this an actual NAK of the patch series as is? I think it is, but I want it to be clear, and it has not been explicit yet.
If it is a NAK, that's fine, but I also want to be sure I understand what the objections are. Based on my understanding of the discussion so far over the multiple versions, I think the primary objection is that the use of pretimeout makes this driver too complex, and indeed complex enough that there is some concern that it could destabilize a running system. Do I have that right?
The other possible item I could conclude out of the discussion is that we do not want to have the pretimeout code as part of the watchdog framework; is that also the case or did I misunderstand?
And finally, a simpler, single stage timeout watchdog driver would be a reasonable thing to accept, yes? I can see where that would make sense.
The issue for me in that case is that the SBSA requires a two stage timeout, so a single stage driver has no real value for me. Now, if I can later add in changes to make the driver into a two stage driver so it is SBSA-compliant, that would also work, but it will make the driver more complex again. At that point, I think I've now gone in a logical circle and the changes would not be accepted so I could never get to my goal of an SBSA-compliant driver. I don't think that's what was meant, so what did I miss?
Thanks in advance for any clarifications that can be provided.....I really do appreciate it. Email is not always the clearest mechanism for communication so sometimes I have to ask odd questions like these so I can understand what is happening.
On 11/12/2015 06:06 PM, Al Stone wrote:
If it is a NAK, that's fine, but I also want to be sure I understand what the objections are. Based on my understanding of the discussion so far over the multiple versions, I think the primary objection is that the use of pretimeout makes this driver too complex, and indeed complex enough that there is some concern that it could destabilize a running system. Do I have that right?
I don't have a problem with the concept of pre-timeout per se. My primary objection is this code:
+static irqreturn_t sbsa_gwdt_interrupt(int irq, void *dev_id) +{
struct sbsa_gwdt *gwdt = (struct sbsa_gwdt *)dev_id;struct watchdog_device *wdd = &gwdt->wdd;/* We don't use pretimeout, trigger WS1 now */if (!wdd->pretimeout)sbsa_gwdt_set_wcv(wdd, 0);
This driver depends on an interrupt handler in order to properly program the hardware. Unlike some other devices, the SBSA watchdog does not need assistance to reset on a timeout -- it is a "fire and forget" device. What happens if there is a hard lockup, and interrupts no longer work?
The reason why Fu does this is because he wants to support a pre-timeout value that's independent of the timeout value. The SBSA watchdog is normally programmed where real timeout equals twice the pre-timeout. I would prefer that the driver adhere to this limitation. That would eliminate the need to pre-program the hardware in the interrupt handler.
And finally, a simpler, single stage timeout watchdog driver would be a reasonable thing to accept, yes? I can see where that would make sense.
I would be okay with merging such a driver, and then enhancing it later to add pre-timeout support.
The issue for me in that case is that the SBSA requires a two stage timeout, so a single stage driver has no real value for me.
There are plenty of existing watchdog devices that have a two-stage timeout but the driver treats it as a single stage. The PowerPC watchdog driver is like that. The hardware is programmed for the second stage to cause a hardware reset, and the interrupt handler is typically a no-op or just a printk().
Sorry for the delayed response...I've got some difficult family things to work on IRL that are taking priority...
On 11/12/2015 05:23 PM, Timur Tabi wrote:
On 11/12/2015 06:06 PM, Al Stone wrote:
If it is a NAK, that's fine, but I also want to be sure I understand what the objections are. Based on my understanding of the discussion so far over the multiple versions, I think the primary objection is that the use of pretimeout makes this driver too complex, and indeed complex enough that there is some concern that it could destabilize a running system. Do I have that right?
I don't have a problem with the concept of pre-timeout per se. My primary objection is this code:
+static irqreturn_t sbsa_gwdt_interrupt(int irq, void *dev_id) +{
struct sbsa_gwdt *gwdt = (struct sbsa_gwdt *)dev_id;struct watchdog_device *wdd = &gwdt->wdd;/* We don't use pretimeout, trigger WS1 now */if (!wdd->pretimeout)sbsa_gwdt_set_wcv(wdd, 0);This driver depends on an interrupt handler in order to properly program the hardware. Unlike some other devices, the SBSA watchdog does not need assistance to reset on a timeout -- it is a "fire and forget" device. What happens if there is a hard lockup, and interrupts no longer work?
Aha. I see now. That helps clarify a lot. Thanks.
The reason why Fu does this is because he wants to support a pre-timeout value that's independent of the timeout value. The SBSA watchdog is normally programmed where real timeout equals twice the pre-timeout. I would prefer that the driver adhere to this limitation. That would eliminate the need to pre-program the hardware in the interrupt handler.
The "normally programmed" limitation described is interesting; forgive my ignorance, but where is that specified? I couldn't find anything that specific in the SBSA, or the ARM ARM, but I could have missed it. That being said, keeping them independent at least seems like a good idea; if I think about kdump/kexec or some other recovery mechanism wanting to perhaps copy part of RAM or flush a filesystem/database, or maybe do some other magic to recover enough to be able to reset the timer, that may be a really long interval on a large server. I could easily see that being very different from a watchdog timer that's meant to just make sure the platform is still making progress. Conversely, I could see that recovery interval being very small or zero on a guest OS, for example, and the watchdog still different.
And finally, a simpler, single stage timeout watchdog driver would be a reasonable thing to accept, yes? I can see where that would make sense.
I would be okay with merging such a driver, and then enhancing it later to add pre-timeout support.
The issue for me in that case is that the SBSA requires a two stage timeout, so a single stage driver has no real value for me.
There are plenty of existing watchdog devices that have a two-stage timeout but the driver treats it as a single stage. The PowerPC watchdog driver is like that. The hardware is programmed for the second stage to cause a hardware reset, and the interrupt handler is typically a no-op or just a printk().
Hrm. Thanks for the pointer. I _think_ I see a way to do that with arm64, and perhaps combine this driver's functionality with what Timur did originally, but still have it reasonably straightforward. I need to do the experiments, though, and see if it actually works first.
On 11/12/2015 04:06 PM, Al Stone wrote:
On 11/05/2015 09:41 AM, Guenter Roeck wrote:
On 11/05/2015 07:00 AM, Fu Wei wrote:
Hi Timur,
On 5 November 2015 at 22:40, Timur Tabi timur@codeaurora.org wrote:
Fu Wei wrote:
Did you really read the "Note" above???????? OK, let me paste it again and again:
SBSA 2.3 Page 23 : If a larger watch period is required then the compare value can be programmed directly into the compare value register.
Well, okay. Sorry, I should have read what you pasted more closely. But I
Thanks for reading it again.
think that means during initialization, not during the WS0 timeout.
I really don't see SBSA say "during initialization, not during the WS0 timeout", please point it out the page number and the line number in SBSA spec. maybe I miss it? Thanks for your help in advance.
Anyway, I still don't like the fact that you're programming WCV in the
"you don't like" doesn't mean "it is wrong" or "we can't do this", so I will keep this way unless we have better idea to extend second stage timeout.
interrupt handler, but I'm not going to make a big deal about it any more.
Deal, Thanks a lot.
The problem with your driver, as I see it, is that dealing with WS0/WS1 and pretimeout makes the driver so complex that, at least for my part, I am very wary about it. The driver could long since have been accepted if it were not for that. Besides that, I really believe that any system designer using the highest permitted frequency should be willing to live with the consequences, and not force the implementation of a a complex driver.
Ultimately, you'll have to decide if you want a simple driver accepted, or a complex driver hanging in the review queue forever.
Thanks, Guenter
Sorry to poke my head in late like this, but I do have a vested interest in the outcome so I'm very curious. For my work, I need to have an ACPI-supported, SBSA-compliant watchdog timer for arm64, and this series is one of the key pieces to getting there. The plan for me has been: (1) get an FDT based SBSA watchdog timer, (2) add in kernel code to handle the ACPI GTDT table describing timers, then (3) munge the SBSA watchdog timer for use by ACPI.
So, is this an actual NAK of the patch series as is? I think it is, but I want it to be clear, and it has not been explicit yet.
I am not the maintainer, so I don't make the call. All I am saying is that I don't feel comfortable with the code as is. Part of it is due to the the specification's complexity, which leaves space for (mis)interpretations and bad implementations.
Either case, this is just my personal opinion. All you'll have to do is to convince Wim to accept your patch.
If it is a NAK, that's fine, but I also want to be sure I understand what the objections are. Based on my understanding of the discussion so far over the multiple versions, I think the primary objection is that the use of pretimeout makes this driver too complex, and indeed complex enough that there is some concern that it could destabilize a running system. Do I have that right?
The other possible item I could conclude out of the discussion is that we do not want to have the pretimeout code as part of the watchdog framework; is that also the case or did I misunderstand?
Nothing really to do with pretimeout, but with the complexity of implementing it for this driver.
As for pretimeout, it does have its issues. Some people say that hitting a pretimeout should result in a panic, others just as strongly say that it should just dump a message to the console. Which does make me a bit wary, since it means that it may be implemented differently in different drivers, which I consider highly undesirable.
And finally, a simpler, single stage timeout watchdog driver would be a reasonable thing to accept, yes? I can see where that would make sense.
I am quite sure that such a driver would long since have been accepted.
The issue for me in that case is that the SBSA requires a two stage timeout,
Hmm - really ? This makes me want to step back a bit and re-read the specification to understand where it says that, and what the reasoning might be for such a requirement.
so a single stage driver has no real value for me. Now, if I can later add in changes to make the driver into a two stage driver so it is SBSA-compliant, that would also work, but it will make the driver more complex again. At that point, I think I've now gone in a logical circle and the changes would not be accepted so I could never get to my goal of an SBSA-compliant driver. I don't think that's what was meant, so what did I miss?
Thanks in advance for any clarifications that can be provided.....I really do appreciate it. Email is not always the clearest mechanism for communication so sometimes I have to ask odd questions like these so I can understand what is happening.
I don't really follow the logic here. Why ask if a single stage driver would have been accepted just to point out that it would have no value for you ?
Really, just convince Wim to accept the driver.
Thanks, Guenter
On 11/12/2015 05:25 PM, Guenter Roeck wrote:
On 11/12/2015 04:06 PM, Al Stone wrote:
On 11/05/2015 09:41 AM, Guenter Roeck wrote:
On 11/05/2015 07:00 AM, Fu Wei wrote:
Hi Timur,
On 5 November 2015 at 22:40, Timur Tabi timur@codeaurora.org wrote:
Fu Wei wrote:
Did you really read the "Note" above???????? OK, let me paste it again and again:
SBSA 2.3 Page 23 : If a larger watch period is required then the compare value can be programmed directly into the compare value register.
Well, okay. Sorry, I should have read what you pasted more closely. But I
Thanks for reading it again.
think that means during initialization, not during the WS0 timeout.
I really don't see SBSA say "during initialization, not during the WS0 timeout", please point it out the page number and the line number in SBSA spec. maybe I miss it? Thanks for your help in advance.
Anyway, I still don't like the fact that you're programming WCV in the
"you don't like" doesn't mean "it is wrong" or "we can't do this", so I will keep this way unless we have better idea to extend second stage timeout.
interrupt handler, but I'm not going to make a big deal about it any more.
Deal, Thanks a lot.
The problem with your driver, as I see it, is that dealing with WS0/WS1 and pretimeout makes the driver so complex that, at least for my part, I am very wary about it. The driver could long since have been accepted if it were not for that. Besides that, I really believe that any system designer using the highest permitted frequency should be willing to live with the consequences, and not force the implementation of a a complex driver.
Ultimately, you'll have to decide if you want a simple driver accepted, or a complex driver hanging in the review queue forever.
Thanks, Guenter
Sorry to poke my head in late like this, but I do have a vested interest in the outcome so I'm very curious. For my work, I need to have an ACPI-supported, SBSA-compliant watchdog timer for arm64, and this series is one of the key pieces to getting there. The plan for me has been: (1) get an FDT based SBSA watchdog timer, (2) add in kernel code to handle the ACPI GTDT table describing timers, then (3) munge the SBSA watchdog timer for use by ACPI.
So, is this an actual NAK of the patch series as is? I think it is, but I want it to be clear, and it has not been explicit yet.
I am not the maintainer, so I don't make the call. All I am saying is that I don't feel comfortable with the code as is. Part of it is due to the the specification's complexity, which leaves space for (mis)interpretations and bad implementations.
Either case, this is just my personal opinion. All you'll have to do is to convince Wim to accept your patch.
Ah, okay. I was a little confused then about who was maintainer. Sorry about that.
If it is a NAK, that's fine, but I also want to be sure I understand what the objections are. Based on my understanding of the discussion so far over the multiple versions, I think the primary objection is that the use of pretimeout makes this driver too complex, and indeed complex enough that there is some concern that it could destabilize a running system. Do I have that right?
The other possible item I could conclude out of the discussion is that we do not want to have the pretimeout code as part of the watchdog framework; is that also the case or did I misunderstand?
Nothing really to do with pretimeout, but with the complexity of implementing it for this driver.
As for pretimeout, it does have its issues. Some people say that hitting a pretimeout should result in a panic, others just as strongly say that it should just dump a message to the console. Which does make me a bit wary, since it means that it may be implemented differently in different drivers, which I consider highly undesirable.
Right. Based on Timur's input, I think I understand this much better now. I'm not 100% convinced I know how to fix it, but I'll try some things on hardware to test out some ideas.
And finally, a simpler, single stage timeout watchdog driver would be a reasonable thing to accept, yes? I can see where that would make sense.
I am quite sure that such a driver would long since have been accepted.
The issue for me in that case is that the SBSA requires a two stage timeout,
Hmm - really ? This makes me want to step back a bit and re-read the specification to understand where it says that, and what the reasoning might be for such a requirement.
As far as I can tell, that's what the SBSA is requiring. My understanding is that the hardware is to first assert a WS0 signal when the timer expires. If the timer expires and WS0 has already been asserted, the WS1 signal is to be asserted. When WS1 is asserted, the system is to do a hard reset (Section 5.2, "Server Base System Architecture", ARM-DEN-0029 Version 2.3). I'm interpreting the occurrence of WS0 as the first stage and WS1 as the second.
To me, at least, this makes sense in a server environment. The WS0 occurs, which gives me some time to save key info or try to recover before WS1 occurs (or kexec, or any other cleverness).
so a single stage driver has no real value for me. Now, if I can later add in changes to make the driver into a two stage driver so it is SBSA-compliant, that would also work, but it will make the driver more complex again. At that point, I think I've now gone in a logical circle and the changes would not be accepted so I could never get to my goal of an SBSA-compliant driver. I don't think that's what was meant, so what did I miss?
Thanks in advance for any clarifications that can be provided.....I really do appreciate it. Email is not always the clearest mechanism for communication so sometimes I have to ask odd questions like these so I can understand what is happening.
I don't really follow the logic here. Why ask if a single stage driver would have been accepted just to point out that it would have no value for you ?
Sorry, I was not being very clear. I was just trying to figure out if there was a risk of inadvertently painting myself into a corner with an incremental approach to developing the functionality needed, versus doing it as a single increment.
Really, just convince Wim to accept the driver.
Nod. Thanks.
Thanks, Guenter
Al Stone wrote:
The issue for me in that case is that the SBSA requires a two stage timeout,
Hmm - really ? This makes me want to step back a bit and re-read the specification to understand where it says that, and what the reasoning might be for such a requirement.
As far as I can tell, that's what the SBSA is requiring. My understanding is that the hardware is to first assert a WS0 signal when the timer expires. If the timer expires and WS0 has already been asserted, the WS1 signal is to be asserted. When WS1 is asserted, the system is to do a hard reset (Section 5.2, "Server Base System Architecture", ARM-DEN-0029 Version 2.3). I'm interpreting the occurrence of WS0 as the first stage and WS1 as the second.
To me, at least, this makes sense in a server environment. The WS0 occurs, which gives me some time to save key info or try to recover before WS1 occurs (or kexec, or any other cleverness).
I'm having some problem with the word "requires".
I think it applies only to the hardware. That is, there must be a WS0 timeout/event, and then after that there must be a WS1 timeout/event.
I don't think there is any "requirement" for software to do anything with WS0. That's why I don't think pre-timeout is necessary for the driver to be SBSA-compliant. All of the drivers for existing two-stage watchdog devices treat the device as a one-stage device, because the watchdog API does not support pre-timeout. Are all of them also "broken"? No. So it would not be broken for an SBSA watchdog driver to ignore WS0.
I would have no problem with the following sequence of events:
1) We merge in a driver that treats the SBSA watchdog as a single-stage watchdog that does a hard reset at WS1 and ignores WS0. My driver, with a few changes, would qualify.
2) We add support for pre-timeout to the Watchdog interface. Fu can take all the time in the world (as far as I'm concerned) getting this perfected. My only request is that the new pre-timeout API supports hardware where the timeout between stages must be equal. That is, if timeout to stage 1 (call it T1) is X seconds, then the timeout to stage 2 (call it T2) is another X seconds. T2 = T1. Of course, the API should also support hardware where T2 != T1.
3) The existing SBSA watchdog driver is updated to support the new pre-timeout API. It would enforce the requirement that T2 = T1.
This approach will allow us to get a working SBSA watchdog driver into 4.5 without much fuss.
-
Al Stone -
Fu Wei -
fu.wei＠linaro.org
-
Guenter Roeck -
Jon Masters -
Mark Rutland -
Timur Tabi