| Message ID | 50c0ec1514173ce07641a95839e939dcda41b110.1412182773.git.joshc@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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>; > + }; > +
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>; > > + }; > > +
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.
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.
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>;
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
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>;
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 <joshc@codeaurora.org> --- arch/arm/boot/dts/qcom-ipq8064.dtsi | 15 +++++++++++++++ 1 file changed, 15 insertions(+)