Now that the Krait Processor Sub-system (KPSS) Watchdog driver has been pulled
into Wim's tree destined for 3.18, add descriptions of the WDT in the relevant
device trees, and include the driver in qcom_defconfig.
Josh Cartwright (4):
ARM: qcom: add QCOM_WDT driver to qcom_defconfig
ARM: qcom: add description of KPSS WDT for IPQ8064
ARM: qcom: add description of KPSS WDT for APQ8064
ARM: qcom: add description of KPSS WDT for MSM8960
arch/arm/boot/dts/qcom-apq8064.dtsi | 15 +++++++++++++++
arch/arm/boot/dts/qcom-ipq8064.dtsi | 15 +++++++++++++++
arch/arm/boot/dts/qcom-msm8960.dtsi | 15 +++++++++++++++
arch/arm/configs/qcom_defconfig | 2 ++
4 files changed, 47 insertions(+)
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
Select QCOM_WDT to allow for the watchdog driver to be built.
Signed-off-by: Josh Cartwright <[email protected]>
---
arch/arm/configs/qcom_defconfig | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/arm/configs/qcom_defconfig b/arch/arm/configs/qcom_defconfig
index 8c7da33..80bad9a 100644
--- a/arch/arm/configs/qcom_defconfig
+++ b/arch/arm/configs/qcom_defconfig
@@ -103,6 +103,8 @@ CONFIG_POWER_SUPPLY=y
CONFIG_POWER_RESET=y
CONFIG_POWER_RESET_MSM=y
CONFIG_THERMAL=y
+CONFIG_WATCHDOG=y
+CONFIG_QCOM_WDT=y
CONFIG_REGULATOR=y
CONFIG_REGULATOR_FIXED_VOLTAGE=y
CONFIG_MEDIA_SUPPORT=y
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
Describe the Krait Processor Sub-system (KPSS) Watchdog timer in the
IPQ8064 device tree. Also, add a fixed-clock description of SLEEP_CLK,
which will do for now.
Signed-off-by: Josh Cartwright <[email protected]>
---
arch/arm/boot/dts/qcom-ipq8064.dtsi | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/arch/arm/boot/dts/qcom-ipq8064.dtsi b/arch/arm/boot/dts/qcom-ipq8064.dtsi
index 244f857..3372b49 100644
--- a/arch/arm/boot/dts/qcom-ipq8064.dtsi
+++ b/arch/arm/boot/dts/qcom-ipq8064.dtsi
@@ -60,6 +60,14 @@
};
};
+ clocks {
+ sleep_clk: sleep_clk {
+ compatible = "fixed-clock";
+ clock-frequency = <32768>;
+ #clock-cells = <0>;
+ };
+ };
+
soc: soc {
#address-cells = <1>;
#size-cells = <1>;
@@ -96,6 +104,13 @@
cpu-offset = <0x80000>;
};
+ watchdog@208a038 {
+ compatible = "qcom,kpss-wdt-ipq8064";
+ reg = <0x0208a038 0x40>;
+ clocks = <&sleep_clk>;
+ timeout-sec = <10>;
+ };
+
acc0: clock-controller@2088000 {
compatible = "qcom,kpss-acc-v1";
reg = <0x02088000 0x1000>, <0x02008000 0x1000>;
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
Describe the Krait Processor Sub-system (KPSS) Watchdog timer in the
APQ8064 device tree. Also, add a fixed-clock description of SLEEP_CLK,
which will do for now.
Signed-off-by: Josh Cartwright <[email protected]>
---
arch/arm/boot/dts/qcom-apq8064.dtsi | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/arch/arm/boot/dts/qcom-apq8064.dtsi b/arch/arm/boot/dts/qcom-apq8064.dtsi
index b3154c0..8439afa 100644
--- a/arch/arm/boot/dts/qcom-apq8064.dtsi
+++ b/arch/arm/boot/dts/qcom-apq8064.dtsi
@@ -66,6 +66,14 @@
interrupts = <1 10 0x304>;
};
+ clocks {
+ sleep_clk: sleep_clk {
+ compatible = "fixed-clock";
+ clock-frequency = <32768>;
+ #clock-cells = <0>;
+ };
+ };
+
soc: soc {
#address-cells = <1>;
#size-cells = <1>;
@@ -119,6 +127,13 @@
cpu-offset = <0x80000>;
};
+ watchdog@208a038 {
+ compatible = "qcom,kpss-wdt-apq8064";
+ reg = <0x0208a038 0x40>;
+ clocks = <&sleep_clk>;
+ timeout-sec = <10>;
+ };
+
acc0: clock-controller@2088000 {
compatible = "qcom,kpss-acc-v1";
reg = <0x02088000 0x1000>, <0x02008000 0x1000>;
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
Describe the Krait Processor Sub-system (KPSS) Watchdog timer in the
MSM8960 device tree. Also, add a fixed-clock description of SLEEP_CLK,
which will do for now.
Signed-off-by: Josh Cartwright <[email protected]>
---
arch/arm/boot/dts/qcom-msm8960.dtsi | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/arch/arm/boot/dts/qcom-msm8960.dtsi b/arch/arm/boot/dts/qcom-msm8960.dtsi
index e1b0d5c..072866b 100644
--- a/arch/arm/boot/dts/qcom-msm8960.dtsi
+++ b/arch/arm/boot/dts/qcom-msm8960.dtsi
@@ -48,6 +48,14 @@
qcom,no-pc-write;
};
+ clocks {
+ sleep_clk: sleep_clk {
+ compatible = "fixed-clock";
+ clock-frequency = <32768>;
+ #clock-cells = <0>;
+ };
+ };
+
soc: soc {
#address-cells = <1>;
#size-cells = <1>;
@@ -73,6 +81,13 @@
cpu-offset = <0x80000>;
};
+ watchdog@208a038 {
+ compatible = "qcom,kpss-wdt-msm8960";
+ reg = <0x0208a038 0x40>;
+ clocks = <&sleep_clk>;
+ timeout-sec = <10>;
+ };
+
msmgpio: gpio@800000 {
compatible = "qcom,msm-gpio";
gpio-controller;
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
On 10/01, Josh Cartwright wrote:
> @@ -96,6 +104,13 @@
> cpu-offset = <0x80000>;
> };
>
> + watchdog@208a038 {
> + compatible = "qcom,kpss-wdt-ipq8064";
> + reg = <0x0208a038 0x40>;
Not being aligned to 4k or 1k raises red flags.
The watchdog is part of the timer block (on this chip it's
"qcom,kpss-timer"). We should add qcom,kpss-wdt to the compatible
list in the timer binding and extend that binding to have clocks
and timeout-sec (watchdog-timeout-sec?). This would follow DT
best practices of having one node per device. We could also add
the interrupts too, even if they're not used by the driver right
now.
> + clocks = <&sleep_clk>;
> + timeout-sec = <10>;
> + };
> +
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
Hey Stephen-
Thanks for taking a look.
On Wed, Oct 01, 2014 at 10:28:55AM -0700, Stephen Boyd wrote:
> On 10/01, Josh Cartwright wrote:
> > @@ -96,6 +104,13 @@
> > cpu-offset = <0x80000>;
> > };
> >
> > + watchdog@208a038 {
> > + compatible = "qcom,kpss-wdt-ipq8064";
> > + reg = <0x0208a038 0x40>;
>
> Not being aligned to 4k or 1k raises red flags.
>
> The watchdog is part of the timer block (on this chip it's
> "qcom,kpss-timer"). We should add qcom,kpss-wdt to the compatible
> list in the timer binding and extend that binding to have clocks
> and timeout-sec (watchdog-timeout-sec?).
Yeah, the description of this thing is a bit awkward.
I'm not sure how I'd feel about just just adding "qcom,kpss-wdt" to the
timer node compatible. I'm wondering if the WDT(s) should be a
subnode(s) of the timer node instead?
The percpu-ness of the two WDTs makes configuration even more
interesting, as it's possible you'd want to independently configure
timeouts for CPU0_WDT0 and CPU1_WDT0, supporting this with a coalesced
timer/wdt would be cumbersome.
Something like this perhaps:
timer@200a000 {
compatible = "qcom,kpss-timer", "qcom,msm-timer";
interrupts = <1 1 0x301>,
<1 2 0x301>,
<1 3 0x301>;
reg = <0x0200a000 0x100>;
clock-frequency = <25000000>,
<32768>;
cpu-offset = <0x80000>;
#address-cells = <1>;
#size-cells = <1>;
ranges;
cpu0_wdt0: watchdog@208a038 {
compatible = "qcom,kpss-wdt";
reg = <0x208a038 0x40>;
interrupts = <1 4 0x301>,
clocks = <&sleep_clk>;
timeout-sec = <10>;
cpu = <&cpu0>;
};
cpu0_wdt1: watchdog@208a060 {
compatible = "qcom,kpss-wdt";
reg = <0x208a060 0x40>;
interrupts = <1 5 0x301>,
clocks = <&sleep_clk>;
timeout-sec = <20>;
cpu = <&cpu0>;
};
cpu1_wdt0: watchdog@209a038 {
compatible = "qcom,kpss-wdt";
reg = <0x209a038 0x40>;
interrupts = <1 4 0x301>,
clocks = <&sleep_clk>;
timeout-sec = <8>;
cpu = <&cpu1>;
};
cpu1_wdt1: watchdog@209a060 {
compatible = "qcom,kpss-wdt";
reg = <0x209a060 0x40>;
interrupts = <1 5 0x301>,
clocks = <&sleep_clk>;
timeout-sec = <15>;
cpu = <&cpu1>;
};
};
> This would follow DT best practices of having one node per device. We
> could also add the interrupts too, even if they're not used by the
> driver right now.
>
> > + clocks = <&sleep_clk>;
> > + timeout-sec = <10>;
> > + };
> > +
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
On 10/01/14 11:15, Josh Cartwright wrote:
> Yeah, the description of this thing is a bit awkward.
:-/ I tried to make the binding future proof.
>
> I'm not sure how I'd feel about just just adding "qcom,kpss-wdt" to the
> timer node compatible. I'm wondering if the WDT(s) should be a
> subnode(s) of the timer node instead?
But then we don't make the timers subnodes? That seems odd. What's the
benefit of multiple sub-nodes for the watchdogs?
It would also be fine to use the current compatible string and just
append the extra two interrupts for the two watchdogs and then make an
optional clocks property and deprecate the clock-frequency property.
>
> The percpu-ness of the two WDTs makes configuration even more
> interesting, as it's possible you'd want to independently configure
> timeouts for CPU0_WDT0 and CPU1_WDT0, supporting this with a coalesced
> timer/wdt would be cumbersome.
We already do similar things for the timers on each cpu. It doesn't seem
that bad, but that's a matter of opinion.
>
> Something like this perhaps:
>
> timer@200a000 {
> compatible = "qcom,kpss-timer", "qcom,msm-timer";
> interrupts = <1 1 0x301>,
> <1 2 0x301>,
> <1 3 0x301>;
> reg = <0x0200a000 0x100>;
> clock-frequency = <25000000>,
> <32768>;
> cpu-offset = <0x80000>;
>
> #address-cells = <1>;
> #size-cells = <1>;
> ranges;
>
> cpu0_wdt0: watchdog@208a038 {
> compatible = "qcom,kpss-wdt";
> reg = <0x208a038 0x40>;
> interrupts = <1 4 0x301>,
> clocks = <&sleep_clk>;
> timeout-sec = <10>;
> cpu = <&cpu0>;
> };
>
> cpu0_wdt1: watchdog@208a060 {
> compatible = "qcom,kpss-wdt";
> reg = <0x208a060 0x40>;
> interrupts = <1 5 0x301>,
> clocks = <&sleep_clk>;
> timeout-sec = <20>;
> cpu = <&cpu0>;
> };
>
> cpu1_wdt0: watchdog@209a038 {
> compatible = "qcom,kpss-wdt";
> reg = <0x209a038 0x40>;
> interrupts = <1 4 0x301>,
> clocks = <&sleep_clk>;
> timeout-sec = <8>;
> cpu = <&cpu1>;
> };
>
> cpu1_wdt1: watchdog@209a060 {
> compatible = "qcom,kpss-wdt";
> reg = <0x209a060 0x40>;
> interrupts = <1 5 0x301>,
> clocks = <&sleep_clk>;
> timeout-sec = <15>;
> cpu = <&cpu1>;
> };
> };
>
>
I'm thinking:
timer@200a000 {
compatible = "qcom,kpss-timer", "qcom,msm-timer";
interrupts = <1 1 0x301>,
<1 2 0x301>,
<1 3 0x301>,
<1 4 0x301>,
<1 5 0x301>;
reg = <0x0200a000 0x100>;
clock-frequency = <27000000>,
<32768>;
clocks = <&cxo>, <&sleep_clk>;
clock-names = "ref", "sleep";
cpu-offset = <0x80000>;
};
Can you explain the need for the cpu handle? Luckily this device only
exists in configurations that have up to 4 CPUs and so mapping the
logical CPU number to the watchdog for that CPU is "easy" in that we can
convert the CPU from logical to physical and then do the math taking
into account the cpu-offset to figure out where the non-aliased
registers are. Once we get into pairs of watchdogs for different
clusters this isn't so easy and it's better to have the phandle
somewhere (either in the watchdog node or the cpu node) and then have
multiple nodes for the watchdog block per-cpu so that we can map the CPU
to the device. We realized this when making the saw binding.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
On Thu, Oct 02, 2014 at 12:08:38PM -0700, Stephen Boyd wrote:
[..]
> On 10/01/14 11:15, Josh Cartwright wrote:
> > The percpu-ness of the two WDTs makes configuration even more
> > interesting, as it's possible you'd want to independently configure
> > timeouts for CPU0_WDT0 and CPU1_WDT0, supporting this with a coalesced
> > timer/wdt would be cumbersome.
>
> We already do similar things for the timers on each cpu. It doesn't seem
> that bad, but that's a matter of opinion.
I think the difference in this case is that each of the 2 per-cpu WDTs
can conceivably be registered independently, each with their own default
timeout configuration. If we wanted to allow the most flexibility, it
seemed to me that making use of subnodes would be the best, but maybe
you have some other idea.
> > Something like this perhaps:
> >
> > timer@200a000 {
> > compatible = "qcom,kpss-timer", "qcom,msm-timer";
> > interrupts = <1 1 0x301>,
> > <1 2 0x301>,
> > <1 3 0x301>;
> > reg = <0x0200a000 0x100>;
> > clock-frequency = <25000000>,
> > <32768>;
> > cpu-offset = <0x80000>;
> >
> > #address-cells = <1>;
> > #size-cells = <1>;
> > ranges;
> >
> > cpu0_wdt0: watchdog@208a038 {
> > compatible = "qcom,kpss-wdt";
> > reg = <0x208a038 0x40>;
> > interrupts = <1 4 0x301>,
> > clocks = <&sleep_clk>;
> > timeout-sec = <10>;
> > cpu = <&cpu0>;
> > };
> >
> > cpu0_wdt1: watchdog@208a060 {
> > compatible = "qcom,kpss-wdt";
> > reg = <0x208a060 0x40>;
> > interrupts = <1 5 0x301>,
> > clocks = <&sleep_clk>;
> > timeout-sec = <20>;
> > cpu = <&cpu0>;
> > };
> >
> > cpu1_wdt0: watchdog@209a038 {
> > compatible = "qcom,kpss-wdt";
> > reg = <0x209a038 0x40>;
> > interrupts = <1 4 0x301>,
> > clocks = <&sleep_clk>;
> > timeout-sec = <8>;
> > cpu = <&cpu1>;
> > };
> >
> > cpu1_wdt1: watchdog@209a060 {
> > compatible = "qcom,kpss-wdt";
> > reg = <0x209a060 0x40>;
> > interrupts = <1 5 0x301>,
> > clocks = <&sleep_clk>;
> > timeout-sec = <15>;
> > cpu = <&cpu1>;
> > };
> > };
> >
> >
>
> I'm thinking:
>
> timer@200a000 {
> compatible = "qcom,kpss-timer", "qcom,msm-timer";
> interrupts = <1 1 0x301>,
> <1 2 0x301>,
> <1 3 0x301>,
> <1 4 0x301>,
> <1 5 0x301>;
> reg = <0x0200a000 0x100>;
> clock-frequency = <27000000>,
> <32768>;
> clocks = <&cxo>, <&sleep_clk>;
> clock-names = "ref", "sleep";
> cpu-offset = <0x80000>;
> };
Where'd the default timeout configuration go? Or, should we have one
timeout-sec property and not allow setting the default timeouts per WDT
instance? Or no configurable timeout at all?
> Can you explain the need for the cpu handle? Luckily this device only
> exists in configurations that have up to 4 CPUs and so mapping the
> logical CPU number to the watchdog for that CPU is "easy" in that we can
> convert the CPU from logical to physical and then do the math taking
> into account the cpu-offset to figure out where the non-aliased
> registers are. Once we get into pairs of watchdogs for different
> clusters this isn't so easy and it's better to have the phandle
> somewhere (either in the watchdog node or the cpu node) and then have
> multiple nodes for the watchdog block per-cpu so that we can map the CPU
> to the device. We realized this when making the saw binding.
Ah, yeah, now that I think about it, it's fairly straightforward to map
backward using the parent cpu-offset to determine the corresponding CPU,
so it wouldn't be necessary.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
On 10/07/2014 03:10 PM, Josh Cartwright wrote:
> On Thu, Oct 02, 2014 at 12:08:38PM -0700, Stephen Boyd wrote:
> [..]
>> On 10/01/14 11:15, Josh Cartwright wrote:
>>> Something like this perhaps:
>>>
>>> timer@200a000 {
>>> compatible = "qcom,kpss-timer", "qcom,msm-timer";
>>> interrupts = <1 1 0x301>,
>>> <1 2 0x301>,
>>> <1 3 0x301>;
>>> reg = <0x0200a000 0x100>;
>>> clock-frequency = <25000000>,
>>> <32768>;
>>> cpu-offset = <0x80000>;
>>>
>>> #address-cells = <1>;
>>> #size-cells = <1>;
>>> ranges;
>>>
>>> cpu0_wdt0: watchdog@208a038 {
>>> compatible = "qcom,kpss-wdt";
>>> reg = <0x208a038 0x40>;
>>> interrupts = <1 4 0x301>,
>>> clocks = <&sleep_clk>;
>>> timeout-sec = <10>;
>>> cpu = <&cpu0>;
>>> };
>>>
>>> cpu0_wdt1: watchdog@208a060 {
>>> compatible = "qcom,kpss-wdt";
>>> reg = <0x208a060 0x40>;
>>> interrupts = <1 5 0x301>,
>>> clocks = <&sleep_clk>;
>>> timeout-sec = <20>;
>>> cpu = <&cpu0>;
>>> };
>>>
>>> cpu1_wdt0: watchdog@209a038 {
>>> compatible = "qcom,kpss-wdt";
>>> reg = <0x209a038 0x40>;
>>> interrupts = <1 4 0x301>,
>>> clocks = <&sleep_clk>;
>>> timeout-sec = <8>;
>>> cpu = <&cpu1>;
>>> };
>>>
>>> cpu1_wdt1: watchdog@209a060 {
>>> compatible = "qcom,kpss-wdt";
>>> reg = <0x209a060 0x40>;
>>> interrupts = <1 5 0x301>,
>>> clocks = <&sleep_clk>;
>>> timeout-sec = <15>;
>>> cpu = <&cpu1>;
>>> };
>>> };
>>>
>>>
>> I'm thinking:
>>
>> timer@200a000 {
>> compatible = "qcom,kpss-timer", "qcom,msm-timer";
>> interrupts = <1 1 0x301>,
>> <1 2 0x301>,
>> <1 3 0x301>,
>> <1 4 0x301>,
>> <1 5 0x301>;
>> reg = <0x0200a000 0x100>;
>> clock-frequency = <27000000>,
>> <32768>;
>> clocks = <&cxo>, <&sleep_clk>;
>> clock-names = "ref", "sleep";
>> cpu-offset = <0x80000>;
>> };
> Where'd the default timeout configuration go? Or, should we have one
> timeout-sec property and not allow setting the default timeouts per WDT
> instance? Or no configurable timeout at all?
Ah sorry. How about a timeout-sec-0, timeout-sec-1 property that is
per-cpu and maps to the first and second watchdog timer? Something like:
timeout-sec-wdt0 = <10 8>;
timeout-sec-wdt1 = <20 15>;
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
On Tue, Oct 07, 2014 at 04:07:43PM -0700, Stephen Boyd wrote:
> On 10/07/2014 03:10 PM, Josh Cartwright wrote:
> >On Thu, Oct 02, 2014 at 12:08:38PM -0700, Stephen Boyd wrote:
[..]
> >>I'm thinking:
> >>
> >> timer@200a000 {
> >> compatible = "qcom,kpss-timer", "qcom,msm-timer";
> >> interrupts = <1 1 0x301>,
> >> <1 2 0x301>,
> >> <1 3 0x301>,
> >> <1 4 0x301>,
> >> <1 5 0x301>;
> >> reg = <0x0200a000 0x100>;
> >> clock-frequency = <27000000>,
> >> <32768>;
> >> clocks = <&cxo>, <&sleep_clk>;
> >> clock-names = "ref", "sleep";
> >> cpu-offset = <0x80000>;
> >> };
> >Where'd the default timeout configuration go? Or, should we have one
> >timeout-sec property and not allow setting the default timeouts per WDT
> >instance? Or no configurable timeout at all?
>
> Ah sorry. How about a timeout-sec-0, timeout-sec-1 property that is per-cpu
> and maps to the first and second watchdog timer? Something like:
>
> timeout-sec-wdt0 = <10 8>;
> timeout-sec-wdt1 = <20 15>;
Okay, yeah, this is much more concise. I'll take a stab at implementing
it.
Thanks,
Josh
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation