Re: [PATCH 2/4] ARM: qcom: add description of KPSS WDT for IPQ8064

[Josh Cartwright <joshc@codeaurora.org>]

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