| Message ID | 1454410739-24444-3-git-send-email-vgupta@synopsys.com |
|---|---|
| State | Superseded |
| Headers | show |
Hi Vineet, On Tue, 2016-02-02 at 16:28 +0530, Vineet Gupta wrote: > ARC Timers have historically been probed directly. > As precursor to start probing Timers thru DT introduce these bindings > > Cc: Daniel Lezcano <daniel.lezcano@linaro.org> > Cc: Rob Herring <robh@kernel.org> > Cc: devicetree@vger.kernel.org > Signed-off-by: Vineet Gupta <vgupta@synopsys.com> > --- [snip] > +Required properties: > + > +- compatible : should be "snps,arc-timer0" > +- interrupts : single Interrupt going into parent intc > + (16 for ARCHS cores, 3 for ARC700 cores) > +- clocks : phandle to the source clock > + > +Optional properties: > + > +- interrupt-parent : phandle to parent intc > + > +Example: > + > + timer0: timer_clkevt { > + compatible = "snps,arc-timer0"; > + interrupts = <3>; > + interrupt-parent = <&core_intc>; > + clocks = <&timer0_clk>; Even though this is an example maybe we may use the same "core_clk" as in real .dts below? -Alexey
Hi Vineet, On Tue, 2016-02-02 at 16:28 +0530, Vineet Gupta wrote: > ARC Timers have historically been probed directly. > As precursor to start probing Timers thru DT introduce these bindings > > Cc: Daniel Lezcano <daniel.lezcano@linaro.org> > Cc: Rob Herring <robh@kernel.org> > Cc: devicetree@vger.kernel.org > Signed-off-by: Vineet Gupta <vgupta@synopsys.com> > --- [snip] > diff --git a/Documentation/devicetree/bindings/timer/snps,arc-timer0.txt > b/Documentation/devicetree/bindings/timer/snps,arc-timer0.txt > new file mode 100644 > index 000000000000..ceb80c72a90b > --- /dev/null > +++ b/Documentation/devicetree/bindings/timer/snps,arc-timer0.txt > @@ -0,0 +1,23 @@ > +Synopsys ARC Local Timer with Interrupt Capabilities > +- Found on all ARC CPUs (ARC700/ARCHS) > +- Mandatory clockevent provider > + > +Required properties: > + > +- compatible : should be "snps,arc-timer0" > +- interrupts : single Interrupt going into parent intc > + (16 for ARCHS cores, 3 for ARC700 cores) > +- clocks : phandle to the source clock Actually we're not flexible here. See we have hard-coded "core_clk" in [PATCH 8/9]. We use it directly in show_cpuinfo() for reading clock speed as well as in axs103_early_init(). So "source clock" here MUST be "core_clk", otherwise /proc/cpuinfo will report junk instead of meaningful data at least. > + > +Optional properties: > + > +- interrupt-parent : phandle to parent intc > + > +Example: > + > + timer0: timer_clkevt { > + compatible = "snps,arc-timer0"; > + interrupts = <3>; > + interrupt-parent = <&core_intc>; > + clocks = <&timer0_clk>; > + }; > diff --git a/Documentation/devicetree/bindings/timer/snps,arc-timer1.txt > b/Documentation/devicetree/bindings/timer/snps,arc-timer1.txt > new file mode 100644 > index 000000000000..4886192ce2f2 > --- /dev/null > +++ b/Documentation/devicetree/bindings/timer/snps,arc-timer1.txt > @@ -0,0 +1,17 @@ > +Synopsys ARC Free Running Local 32-bit Timer > +- Found on all ARC CPUs (ARC700/ARCHS) > +- Mandatory clocksource provider on ARC700 > +- Optional clocksource provider on UP ARC HS CPUs > + (and if better timer archs-rtc not available in SoC) > + > +Required properties: > + > +- compatible : should be "snps,arc-timer1" > +- clocks : phandle to the source clock > + > +Example: > + > + timer1: timer_clksrc { > + compatible = "snps,arc-timer1"; > + clocks = <&timer0_clk>; Ditto, "clocks = <&core_clk>". > + }; > diff --git a/Documentation/devicetree/bindings/timer/snps,archs-gfrc.txt > b/Documentation/devicetree/bindings/timer/snps,archs-gfrc.txt > new file mode 100644 > index 000000000000..cce60e16aa0d > --- /dev/null > +++ b/Documentation/devicetree/bindings/timer/snps,archs-gfrc.txt > @@ -0,0 +1,14 @@ > +Synopsys ARC Free Running 64-bit Global Timer for ARC HS CPUs > +- clocksourc provider for SMP SoC > + > +Required properties: > + > +- compatible : should be "snps,archs-gfrc" > +- clocks : phandle to the source clock > + > +Example: > + > + timer1: timer_clksrc { > + compatible = "snps,archs-gfrc"; > + clocks = <&timer0_clk>; Ditto, "clocks = <&core_clk>". > + }; > diff --git a/Documentation/devicetree/bindings/timer/snps,archs-rtc.txt > b/Documentation/devicetree/bindings/timer/snps,archs-rtc.txt > new file mode 100644 > index 000000000000..f3b49938812b > --- /dev/null > +++ b/Documentation/devicetree/bindings/timer/snps,archs-rtc.txt > @@ -0,0 +1,14 @@ > +Synopsys ARC Free Running 64-bit Local Timer for ARC HS CPUs > +- clocksourc provider for UP SoC > + > +Required properties: > + > +- compatible : should be "snps,archs-rtc" > +- clocks : phandle to the source clock > + > +Example: > + > + timer1: timer_clksrc { > + compatible = "snps,arc-rtc"; > + clocks = <&timer0_clk>; > + }; > diff --git a/arch/arc/boot/dts/abilis_tb10x.dtsi b/arch/arc/boot/dts/abilis_tb10x.dtsi > index cfb5052239a1..f9f138efa92c 100644 > --- a/arch/arc/boot/dts/abilis_tb10x.dtsi > +++ b/arch/arc/boot/dts/abilis_tb10x.dtsi > @@ -35,6 +35,18 @@ > }; > }; > > + timer0: timer_clkevt { > + compatible = "snps,arc-timer0"; > + interrupts = <3>; > + interrupt-parent = <&intc>; > + clocks = <&cpu_clk>; > > + }; > + > + timer1: timer_clksrc { > + compatible = "snps,arc-timer1"; > + clocks = <&cpu_clk>; > + }; > + Hm now that's a question how to fix /proc/cpuinfo output for Abilis? There's no "core_clk" DTS node for Abilis and so show_cpuinfo() won't get proper clock value. Probably we may fix it with modification of their "pll" node from ------------------------>8---------------------- pll0: oscillator { clock-frequency = <1000000000>; }; ------------------------>8---------------------- to ------------------------>8---------------------- core_clk: oscillator { clock -frequency = <1000000000>; }; ------------------------>8---------------------- -Alexey
Hi Alexey, On Tuesday 02 February 2016 06:45 PM, Alexey Brodkin wrote: > Hi Vineet, > > On Tue, 2016-02-02 at 16:28 +0530, Vineet Gupta wrote: >> + >> +Required properties: >> + >> +- compatible : should be "snps,arc-timer0" >> +- interrupts : single Interrupt going into parent intc >> + (16 for ARCHS cores, 3 for ARC700 cores) >> +- clocks : phandle to the source clock > > Actually we're not flexible here. > See we have hard-coded "core_clk" in [PATCH 8/9]. > We use it directly in show_cpuinfo() for reading clock speed > as well as in axs103_early_init(). > > So "source clock" here MUST be "core_clk", otherwise > /proc/cpuinfo will report junk instead of meaningful data at least. Using hardcoded DT names in generic code is total BS and I slap myself for missing that in reviewing 8/9. Please fix it ! FWIW, it is OK to have such hardcoding in say AXS103 DTS and AXS103 platform code but it is not the way to go in setup.c >> +Required properties: >> + >> +- compatible : should be "snps,arc-timer1" >> +- clocks : phandle to the source clock >> + >> +Example: >> + >> + timer1: timer_clksrc { >> + compatible = "snps,arc-timer1"; >> + clocks = <&timer0_clk>; > > Ditto, "clocks = <&core_clk>". Yeah I fixed all those ! >> diff --git a/arch/arc/boot/dts/abilis_tb10x.dtsi b/arch/arc/boot/dts/abilis_tb10x.dtsi >> index cfb5052239a1..f9f138efa92c 100644 >> --- a/arch/arc/boot/dts/abilis_tb10x.dtsi >> +++ b/arch/arc/boot/dts/abilis_tb10x.dtsi >> @@ -35,6 +35,18 @@ >> }; >> }; >> >> + timer0: timer_clkevt { >> + compatible = "snps,arc-timer0"; >> + interrupts = <3>; >> + interrupt-parent = <&intc>; >> + clocks = <&cpu_clk>; >> >> + }; >> + >> + timer1: timer_clksrc { >> + compatible = "snps,arc-timer1"; >> + clocks = <&cpu_clk>; >> + }; >> + > > Hm now that's a question how to fix /proc/cpuinfo output > for Abilis? There's no "core_clk" DTS node for Abilis and so > show_cpuinfo() won't get proper clock value. > > Probably we may fix it with modification of their "pll" node > from > ------------------------>8---------------------- > pll0: oscillator { > clock-frequency = <1000000000>; > }; > ------------------------>8---------------------- > > to > ------------------------>8---------------------- > core_clk: oscillator { > clock > -frequency = <1000000000>; > }; > ------------------------>8---------------------- This is all moot once we fix the orig problem.
Hi Vineet, On Tue, 2016-02-02 at 19:59 +0530, Vineet Gupta wrote: > Hi Alexey, > > On Tuesday 02 February 2016 06:45 PM, Alexey Brodkin wrote: > > Hi Vineet, > > > > On Tue, 2016-02-02 at 16:28 +0530, Vineet Gupta wrote: > > > + > > > +Required properties: > > > + > > > +- compatible : should be "snps,arc-timer0" > > > +- interrupts : single Interrupt going into parent intc > > > + (16 for ARCHS cores, 3 for ARC700 cores) > > > +- clocks : phandle to the source clock > > > > Actually we're not flexible here. > > See we have hard-coded "core_clk" in [PATCH 8/9]. > > We use it directly in show_cpuinfo() for reading clock speed > > as well as in axs103_early_init(). > > > > So "source clock" here MUST be "core_clk", otherwise > > /proc/cpuinfo will report junk instead of meaningful data at least. > > Using hardcoded DT names in generic code is total BS and I slap myself for missing > that in reviewing 8/9. Please fix it ! But the only other alternative to hard-coded name is use of some internal variable like "arc_timer_freq". I.e. we make "arc_timer_freq" global and use it for displaying core frequency. Are you OK with that? -Alexey
On Tue, Feb 02, 2016 at 04:28:52PM +0530, Vineet Gupta wrote: > ARC Timers have historically been probed directly. > As precursor to start probing Timers thru DT introduce these bindings > > Cc: Daniel Lezcano <daniel.lezcano@linaro.org> > Cc: Rob Herring <robh@kernel.org> > Cc: devicetree@vger.kernel.org > Signed-off-by: Vineet Gupta <vgupta@synopsys.com> > --- > .../devicetree/bindings/timer/snps,arc-timer0.txt | 23 ++++++++++++++++++++++ > .../devicetree/bindings/timer/snps,arc-timer1.txt | 17 ++++++++++++++++ > .../devicetree/bindings/timer/snps,archs-gfrc.txt | 14 +++++++++++++ > .../devicetree/bindings/timer/snps,archs-rtc.txt | 14 +++++++++++++ > arch/arc/boot/dts/abilis_tb10x.dtsi | 12 +++++++++++ > arch/arc/boot/dts/skeleton.dtsi | 12 +++++++++++ > arch/arc/boot/dts/skeleton_hs.dtsi | 12 +++++++++++ > arch/arc/boot/dts/skeleton_hs_idu.dtsi | 12 +++++++++++ > 8 files changed, 116 insertions(+) > create mode 100644 Documentation/devicetree/bindings/timer/snps,arc-timer0.txt > create mode 100644 Documentation/devicetree/bindings/timer/snps,arc-timer1.txt > create mode 100644 Documentation/devicetree/bindings/timer/snps,archs-gfrc.txt > create mode 100644 Documentation/devicetree/bindings/timer/snps,archs-rtc.txt > > diff --git a/Documentation/devicetree/bindings/timer/snps,arc-timer0.txt b/Documentation/devicetree/bindings/timer/snps,arc-timer0.txt > new file mode 100644 > index 000000000000..ceb80c72a90b > --- /dev/null > +++ b/Documentation/devicetree/bindings/timer/snps,arc-timer0.txt > @@ -0,0 +1,23 @@ > +Synopsys ARC Local Timer with Interrupt Capabilities > +- Found on all ARC CPUs (ARC700/ARCHS) > +- Mandatory clockevent provider > + > +Required properties: > + > +- compatible : should be "snps,arc-timer0" timer0 and timer1 are different h/w blocks, not just different instances? > +- interrupts : single Interrupt going into parent intc > + (16 for ARCHS cores, 3 for ARC700 cores) > +- clocks : phandle to the source clock > + > +Optional properties: > + > +- interrupt-parent : phandle to parent intc > + > +Example: > + > + timer0: timer_clkevt { just "timer" for node name. clkevt is a Linuxism. > + compatible = "snps,arc-timer0"; > + interrupts = <3>; > + interrupt-parent = <&core_intc>; > + clocks = <&timer0_clk>; > + }; > diff --git a/Documentation/devicetree/bindings/timer/snps,arc-timer1.txt b/Documentation/devicetree/bindings/timer/snps,arc-timer1.txt > new file mode 100644 > index 000000000000..4886192ce2f2 > --- /dev/null > +++ b/Documentation/devicetree/bindings/timer/snps,arc-timer1.txt > @@ -0,0 +1,17 @@ > +Synopsys ARC Free Running Local 32-bit Timer > +- Found on all ARC CPUs (ARC700/ARCHS) > +- Mandatory clocksource provider on ARC700 > +- Optional clocksource provider on UP ARC HS CPUs > + (and if better timer archs-rtc not available in SoC) > + > +Required properties: > + > +- compatible : should be "snps,arc-timer1" > +- clocks : phandle to the source clock No interrupt because it doesn't have one or you use this as a clocksource and don't need it? > + > +Example: > + > + timer1: timer_clksrc { > + compatible = "snps,arc-timer1"; > + clocks = <&timer0_clk>; > + }; > diff --git a/Documentation/devicetree/bindings/timer/snps,archs-gfrc.txt b/Documentation/devicetree/bindings/timer/snps,archs-gfrc.txt > new file mode 100644 > index 000000000000..cce60e16aa0d > --- /dev/null > +++ b/Documentation/devicetree/bindings/timer/snps,archs-gfrc.txt > @@ -0,0 +1,14 @@ > +Synopsys ARC Free Running 64-bit Global Timer for ARC HS CPUs > +- clocksourc provider for SMP SoC > + > +Required properties: > + > +- compatible : should be "snps,archs-gfrc" > +- clocks : phandle to the source clock > + > +Example: > + > + timer1: timer_clksrc { > + compatible = "snps,archs-gfrc"; > + clocks = <&timer0_clk>; > + }; > diff --git a/Documentation/devicetree/bindings/timer/snps,archs-rtc.txt b/Documentation/devicetree/bindings/timer/snps,archs-rtc.txt > new file mode 100644 > index 000000000000..f3b49938812b > --- /dev/null > +++ b/Documentation/devicetree/bindings/timer/snps,archs-rtc.txt > @@ -0,0 +1,14 @@ > +Synopsys ARC Free Running 64-bit Local Timer for ARC HS CPUs > +- clocksourc provider for UP SoC local timer on a UP processor? > + > +Required properties: > + > +- compatible : should be "snps,archs-rtc" > +- clocks : phandle to the source clock > + > +Example: > + > + timer1: timer_clksrc { > + compatible = "snps,arc-rtc"; > + clocks = <&timer0_clk>; > + };
Hi Vineet, On Tue, 2016-02-02 at 18:36 +0300, Alexey Brodkin wrote: > Hi Vineet, > > On Tue, 2016-02-02 at 19:59 +0530, Vineet Gupta wrote: > > Hi Alexey, > > > > On Tuesday 02 February 2016 06:45 PM, Alexey Brodkin wrote: > > > Hi Vineet, > > > > > > On Tue, 2016-02-02 at 16:28 +0530, Vineet Gupta wrote: > > > > + > > > > +Required properties: > > > > + > > > > +- compatible : should be "snps,arc-timer0" > > > > +- interrupts : single Interrupt going into parent intc > > > > + (16 for ARCHS cores, 3 for ARC700 cores) > > > > +- clocks : phandle to the source clock > > > > > > Actually we're not flexible here. > > > See we have hard-coded "core_clk" in [PATCH 8/9]. > > > We use it directly in show_cpuinfo() for reading clock speed > > > as well as in axs103_early_init(). > > > > > > So "source clock" here MUST be "core_clk", otherwise > > > /proc/cpuinfo will report junk instead of meaningful data at least. > > > > Using hardcoded DT names in generic code is total BS and I slap myself for missing > > that in reviewing 8/9. Please fix it ! > > But the only other alternative to hard-coded name is use of some internal variable > like "arc_timer_freq". > > I.e. we make "arc_timer_freq" global and use it for displaying core frequency. Well actually there's another possibility that is used on many other platforms (ARM both 32 and 64-bit flavors is a good example) - just print bogomips instead of additional core frequency. -Alexey
Hi Rob, On Wednesday 03 February 2016 03:33 AM, Rob Herring wrote: > On Tue, Feb 02, 2016 at 04:28:52PM +0530, Vineet Gupta wrote: >> +Required properties: >> + >> +- compatible : should be "snps,arc-timer0" > > timer0 and timer1 are different h/w blocks, not just different > instances? Functionality wise they are identical (only the address of aux regs used to program them are different). Either can be configured to interrupt-on-limit or free-run-and-wrap-around. So we can indeed consider them 2 instances. ARC Linux uses timer0 for tick handling, timer1 for gtod. Do you prefer they not be differentiated as timer0 and timer1 ? So we have CLOCKSOURCE_OF_DECLARE(arc_clkevt, "snps,arc-timer0", arc_clockevent_setup); CLOCKSOURCE_OF_DECLARE(arc_timer1, "snps,arc-timer1", arc_cs_setup_timer1); I don't know how to achieve above, by keeping the DT names the same. > >> +- interrupts : single Interrupt going into parent intc >> + (16 for ARCHS cores, 3 for ARC700 cores) >> +- clocks : phandle to the source clock >> + >> +Optional properties: >> + >> +- interrupt-parent : phandle to parent intc >> + >> +Example: >> + >> + timer0: timer_clkevt { > > just "timer" for node name. clkevt is a Linuxism. OK. So to document that this is for clockevent, change the label ? timer_clkevent: timer { > >> + compatible = "snps,arc-timer0"; >> + interrupts = <3>; >> + interrupt-parent = <&core_intc>; >> + clocks = <&timer0_clk>; >> + }; >> diff --git a/Documentation/devicetree/bindings/timer/snps,arc-timer1.txt b/Documentation/devicetree/bindings/timer/snps,arc-timer1.txt >> new file mode 100644 >> index 000000000000..4886192ce2f2 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/timer/snps,arc-timer1.txt >> @@ -0,0 +1,17 @@ >> +Synopsys ARC Free Running Local 32-bit Timer >> +- Found on all ARC CPUs (ARC700/ARCHS) >> +- Mandatory clocksource provider on ARC700 >> +- Optional clocksource provider on UP ARC HS CPUs >> + (and if better timer archs-rtc not available in SoC) >> + >> +Required properties: >> + >> +- compatible : should be "snps,arc-timer1" >> +- clocks : phandle to the source clock > > No interrupt because it doesn't have one or you use this as a > clocksource and don't need it? Latter ! >> + >> +Example: >> + >> + timer1: timer_clksrc { >> + compatible = "snps,arc-timer1"; >> + clocks = <&timer0_clk>; >> + }; >> diff --git a/Documentation/devicetree/bindings/timer/snps,archs-gfrc.txt b/Documentation/devicetree/bindings/timer/snps,archs-gfrc.txt >> new file mode 100644 >> index 000000000000..cce60e16aa0d >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/timer/snps,archs-gfrc.txt >> @@ -0,0 +1,14 @@ >> +Synopsys ARC Free Running 64-bit Global Timer for ARC HS CPUs >> +- clocksourc provider for SMP SoC >> + >> +Required properties: >> + >> +- compatible : should be "snps,archs-gfrc" >> +- clocks : phandle to the source clock >> + >> +Example: >> + >> + timer1: timer_clksrc { >> + compatible = "snps,archs-gfrc"; >> + clocks = <&timer0_clk>; >> + }; >> diff --git a/Documentation/devicetree/bindings/timer/snps,archs-rtc.txt b/Documentation/devicetree/bindings/timer/snps,archs-rtc.txt >> new file mode 100644 >> index 000000000000..f3b49938812b >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/timer/snps,archs-rtc.txt >> @@ -0,0 +1,14 @@ >> +Synopsys ARC Free Running 64-bit Local Timer for ARC HS CPUs >> +- clocksourc provider for UP SoC > > local timer on a UP processor? Not sure what you mean to change. This timer could be present in UP or SMP hardware configs but only usable as clocksource for UP since it is local and we don't do tick broadcast etc for SMP. To me clocksource is one level higher in level of abstraction than processor and thus applies to overall system / SoC than the processor. Thx, -Vineet >> +Required properties: >> + >> +- compatible : should be "snps,archs-rtc" >> +- clocks : phandle to the source clock >> + >> +Example: >> + >> + timer1: timer_clksrc { >> + compatible = "snps,arc-rtc"; >> + clocks = <&timer0_clk>; >> + }; > --
Hi Mike, On Wed, 2016-02-03 at 01:57 +0300, Alexey Brodkin wrote: > Hi Vineet, > > On Tue, 2016-02-02 at 18:36 +0300, Alexey Brodkin wrote: > > Hi Vineet, > > > > On Tue, 2016-02-02 at 19:59 +0530, Vineet Gupta wrote: > > > Hi Alexey, > > > > > > On Tuesday 02 February 2016 06:45 PM, Alexey Brodkin wrote: > > > > Hi Vineet, > > > > > > > > On Tue, 2016-02-02 at 16:28 +0530, Vineet Gupta wrote: > > > > > + > > > > > +Required properties: > > > > > + > > > > > +- compatible : should be "snps,arc-timer0" > > > > > +- interrupts : single Interrupt going into parent intc > > > > > + (16 for ARCHS cores, 3 for ARC700 cores) > > > > > +- clocks : phandle to the source clock > > > > > > > > Actually we're not flexible here. > > > > See we have hard-coded "core_clk" in [PATCH 8/9]. > > > > We use it directly in show_cpuinfo() for reading clock speed > > > > as well as in axs103_early_init(). > > > > > > > > So "source clock" here MUST be "core_clk", otherwise > > > > /proc/cpuinfo will report junk instead of meaningful data at least. > > > > > > Using hardcoded DT names in generic code is total BS and I slap myself for missing > > > that in reviewing 8/9. Please fix it ! > > > > But the only other alternative to hard-coded name is use of some internal variable > > like "arc_timer_freq". > > > > I.e. we make "arc_timer_freq" global and use it for displaying core frequency. > > Well actually there's another possibility that is used on many other platforms > (ARM both 32 and 64-bit flavors is a good example) - just print bogomips instead > of additional core frequency. We're in the process of switching ARC to generic clk framework. One of the problems we're trying to solve now is how to obtain precise CPU frequency value for outputting it for example by /proc/cpuinfo. This precise (in terms of what value was set via Device Tree or extracted and decoded from CPU configuration registers) CPU frequency is very useful for example for benchmarking. In comparison bogomips might be misleading at times. Before moving to clk framework we used to have 2 ARC-specific calls arc_get_core_freq() and arc_set_core_freq() which were basically wrappers for one variable where we stored CPU frequency. I took a look at what other architectures do and so far saw these options: [1] Just print bogomips (ARM both 32- and 64-bit, m64k, Microblaze, Mips, mn10300, openrisc, s390, sh, um, unicore32, ) [2] Get frequency from some kind of architecture-specific structure or variable (Alpha, AVR32, c6x, nios2, powerpc, sparc, tile, xtensa) [3] Get frequency from cpufreq framework (ia64, x86) [4] Decode frequency from hardware registers (Blackfin) Any thoughts on what's the best way to get CPU frequency in run-time (preferably with use of clk framework so we'll need no arch-specific variables)? Regards, Alexey
(re-sending because Mike's email @ti is no longer valid) Hi Mike, On Wed, 2016-02-03 at 01:57 +0300, Alexey Brodkin wrote: > Hi Vineet, > > On Tue, 2016-02-02 at 18:36 +0300, Alexey Brodkin wrote: > > Hi Vineet, > > > > On Tue, 2016-02-02 at 19:59 +0530, Vineet Gupta wrote: > > > Hi Alexey, > > > > > > On Tuesday 02 February 2016 06:45 PM, Alexey Brodkin wrote: > > > > Hi Vineet, > > > > > > > > On Tue, 2016-02-02 at 16:28 +0530, Vineet Gupta wrote: > > > > > + > > > > > +Required properties: > > > > > + > > > > > +- compatible : should be "snps,arc-timer0" > > > > > +- interrupts : single Interrupt going into parent intc > > > > > + (16 for ARCHS cores, 3 for ARC700 cores) > > > > > +- clocks : phandle to the source clock > > > > > > > > Actually we're not flexible here. > > > > See we have hard-coded "core_clk" in [PATCH 8/9]. > > > > We use it directly in show_cpuinfo() for reading clock speed > > > > as well as in axs103_early_init(). > > > > > > > > So "source clock" here MUST be "core_clk", otherwise > > > > /proc/cpuinfo will report junk instead of meaningful data at least. > > > > > > Using hardcoded DT names in generic code is total BS and I slap myself for missing > > > that in reviewing 8/9. Please fix it ! > > > > But the only other alternative to hard-coded name is use of some internal variable > > like "arc_timer_freq". > > > > I.e. we make "arc_timer_freq" global and use it for displaying core frequency. > > Well actually there's another possibility that is used on many other platforms > (ARM both 32 and 64-bit flavors is a good example) - just print bogomips instead > of additional core frequency. We're in the process of switching ARC to generic clk framework. One of the problems we're trying to solve now is how to obtain precise CPU frequency value for outputting it for example by /proc/cpuinfo. This precise (in terms of what value was set via Device Tree or extracted and decoded from CPU configuration registers) CPU frequency is very useful for example for benchmarking. In comparison bogomips might be misleading at times. Before moving to clk framework we used to have 2 ARC-specific calls arc_get_core_freq() and arc_set_core_freq() which were basically wrappers for one variable where we stored CPU frequency. I took a look at what other architectures do and so far saw these options: [1] Just print bogomips (ARM both 32- and 64-bit, m64k, Microblaze, Mips, mn10300, openrisc, s390, sh, um, unicore32, ) [2] Get frequency from some kind of architecture-specific structure or variable (Alpha, AVR32, c6x, nios2, powerpc, sparc, tile, xtensa) [3] Get frequency from cpufreq framework (ia64, x86) [4] Decode frequency from hardware registers (Blackfin) Any thoughts on what's the best way to get CPU frequency in run-time (preferably with use of clk framework so we'll need no arch-specific variables)? Regards, Alexey
On Wed, Feb 3, 2016 at 2:04 AM, Vineet Gupta <Vineet.Gupta1@synopsys.com> wrote: > Hi Rob, > > On Wednesday 03 February 2016 03:33 AM, Rob Herring wrote: >> On Tue, Feb 02, 2016 at 04:28:52PM +0530, Vineet Gupta wrote: >>> +Required properties: >>> + >>> +- compatible : should be "snps,arc-timer0" >> >> timer0 and timer1 are different h/w blocks, not just different >> instances? > > Functionality wise they are identical (only the address of aux regs used to > program them are different). Either can be configured to interrupt-on-limit or > free-run-and-wrap-around. So we can indeed consider them 2 instances. ARC Linux > uses timer0 for tick handling, timer1 for gtod. > > Do you prefer they not be differentiated as timer0 and timer1 ? > > So we have > > CLOCKSOURCE_OF_DECLARE(arc_clkevt, "snps,arc-timer0", arc_clockevent_setup); > CLOCKSOURCE_OF_DECLARE(arc_timer1, "snps,arc-timer1", arc_cs_setup_timer1); > > I don't know how to achieve above, by keeping the DT names the same. You just need a single CLOCKSOURCE_OF_DECLARE which will be called twice. On the first call, setup one timer and on the 2nd call setup the other one. IIRC the sp804 timer has something similar. You'll need a unit address in the node name to distinguish them. > >> >>> +- interrupts : single Interrupt going into parent intc >>> + (16 for ARCHS cores, 3 for ARC700 cores) >>> +- clocks : phandle to the source clock >>> + >>> +Optional properties: >>> + >>> +- interrupt-parent : phandle to parent intc >>> + >>> +Example: >>> + >>> + timer0: timer_clkevt { >> >> just "timer" for node name. clkevt is a Linuxism. > > OK. So to document that this is for clockevent, change the label ? That shouldn't be documented in the DT at all. > > timer_clkevent: timer { > >> >>> + compatible = "snps,arc-timer0"; >>> + interrupts = <3>; >>> + interrupt-parent = <&core_intc>; >>> + clocks = <&timer0_clk>; >>> + }; >>> diff --git a/Documentation/devicetree/bindings/timer/snps,arc-timer1.txt b/Documentation/devicetree/bindings/timer/snps,arc-timer1.txt >>> new file mode 100644 >>> index 000000000000..4886192ce2f2 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/timer/snps,arc-timer1.txt >>> @@ -0,0 +1,17 @@ >>> +Synopsys ARC Free Running Local 32-bit Timer >>> +- Found on all ARC CPUs (ARC700/ARCHS) >>> +- Mandatory clocksource provider on ARC700 >>> +- Optional clocksource provider on UP ARC HS CPUs >>> + (and if better timer archs-rtc not available in SoC) >>> + >>> +Required properties: >>> + >>> +- compatible : should be "snps,arc-timer1" >>> +- clocks : phandle to the source clock >> >> No interrupt because it doesn't have one or you use this as a >> clocksource and don't need it? > > Latter ! Then you should have the interrupt in the DT anyway. Rob
On Wednesday 03 February 2016 09:09 PM, Rob Herring wrote: > On Wed, Feb 3, 2016 at 2:04 AM, Vineet Gupta <Vineet.Gupta1-HKixBCOQz3hWk0Htik3J/w@public.gmane.org> wrote: >> Hi Rob, >> >> On Wednesday 03 February 2016 03:33 AM, Rob Herring wrote: >>> On Tue, Feb 02, 2016 at 04:28:52PM +0530, Vineet Gupta wrote: >>>> +Required properties: >>>> + >>>> +- compatible : should be "snps,arc-timer0" >>> >>> timer0 and timer1 are different h/w blocks, not just different >>> instances? >> >> Functionality wise they are identical (only the address of aux regs used to >> program them are different). Either can be configured to interrupt-on-limit or >> free-run-and-wrap-around. So we can indeed consider them 2 instances. ARC Linux >> uses timer0 for tick handling, timer1 for gtod. >> >> Do you prefer they not be differentiated as timer0 and timer1 ? >> >> So we have >> >> CLOCKSOURCE_OF_DECLARE(arc_clkevt, "snps,arc-timer0", arc_clockevent_setup); >> CLOCKSOURCE_OF_DECLARE(arc_timer1, "snps,arc-timer1", arc_cs_setup_timer1); >> >> I don't know how to achieve above, by keeping the DT names the same. > > You just need a single CLOCKSOURCE_OF_DECLARE which will be called > twice. On the first call, setup one timer and on the 2nd call setup > the other one. IIRC the sp804 timer has something similar. > > You'll need a unit address in the node name to distinguish them. So this is just to distinguish them in DT, but the callback uses a static counter to do 1st vs. 2nd (and this unit address is not really exposed to code) - correct ?
diff --git a/Documentation/devicetree/bindings/timer/snps,arc-timer0.txt b/Documentation/devicetree/bindings/timer/snps,arc-timer0.txt new file mode 100644 index 000000000000..ceb80c72a90b --- /dev/null +++ b/Documentation/devicetree/bindings/timer/snps,arc-timer0.txt @@ -0,0 +1,23 @@ +Synopsys ARC Local Timer with Interrupt Capabilities +- Found on all ARC CPUs (ARC700/ARCHS) +- Mandatory clockevent provider + +Required properties: + +- compatible : should be "snps,arc-timer0" +- interrupts : single Interrupt going into parent intc + (16 for ARCHS cores, 3 for ARC700 cores) +- clocks : phandle to the source clock + +Optional properties: + +- interrupt-parent : phandle to parent intc + +Example: + + timer0: timer_clkevt { + compatible = "snps,arc-timer0"; + interrupts = <3>; + interrupt-parent = <&core_intc>; + clocks = <&timer0_clk>; + }; diff --git a/Documentation/devicetree/bindings/timer/snps,arc-timer1.txt b/Documentation/devicetree/bindings/timer/snps,arc-timer1.txt new file mode 100644 index 000000000000..4886192ce2f2 --- /dev/null +++ b/Documentation/devicetree/bindings/timer/snps,arc-timer1.txt @@ -0,0 +1,17 @@ +Synopsys ARC Free Running Local 32-bit Timer +- Found on all ARC CPUs (ARC700/ARCHS) +- Mandatory clocksource provider on ARC700 +- Optional clocksource provider on UP ARC HS CPUs + (and if better timer archs-rtc not available in SoC) + +Required properties: + +- compatible : should be "snps,arc-timer1" +- clocks : phandle to the source clock + +Example: + + timer1: timer_clksrc { + compatible = "snps,arc-timer1"; + clocks = <&timer0_clk>; + }; diff --git a/Documentation/devicetree/bindings/timer/snps,archs-gfrc.txt b/Documentation/devicetree/bindings/timer/snps,archs-gfrc.txt new file mode 100644 index 000000000000..cce60e16aa0d --- /dev/null +++ b/Documentation/devicetree/bindings/timer/snps,archs-gfrc.txt @@ -0,0 +1,14 @@ +Synopsys ARC Free Running 64-bit Global Timer for ARC HS CPUs +- clocksourc provider for SMP SoC + +Required properties: + +- compatible : should be "snps,archs-gfrc" +- clocks : phandle to the source clock + +Example: + + timer1: timer_clksrc { + compatible = "snps,archs-gfrc"; + clocks = <&timer0_clk>; + }; diff --git a/Documentation/devicetree/bindings/timer/snps,archs-rtc.txt b/Documentation/devicetree/bindings/timer/snps,archs-rtc.txt new file mode 100644 index 000000000000..f3b49938812b --- /dev/null +++ b/Documentation/devicetree/bindings/timer/snps,archs-rtc.txt @@ -0,0 +1,14 @@ +Synopsys ARC Free Running 64-bit Local Timer for ARC HS CPUs +- clocksourc provider for UP SoC + +Required properties: + +- compatible : should be "snps,archs-rtc" +- clocks : phandle to the source clock + +Example: + + timer1: timer_clksrc { + compatible = "snps,arc-rtc"; + clocks = <&timer0_clk>; + }; diff --git a/arch/arc/boot/dts/abilis_tb10x.dtsi b/arch/arc/boot/dts/abilis_tb10x.dtsi index cfb5052239a1..f9f138efa92c 100644 --- a/arch/arc/boot/dts/abilis_tb10x.dtsi +++ b/arch/arc/boot/dts/abilis_tb10x.dtsi @@ -35,6 +35,18 @@ }; }; + timer0: timer_clkevt { + compatible = "snps,arc-timer0"; + interrupts = <3>; + interrupt-parent = <&intc>; + clocks = <&cpu_clk>; + }; + + timer1: timer_clksrc { + compatible = "snps,arc-timer1"; + clocks = <&cpu_clk>; + }; + soc100 { #address-cells = <1>; #size-cells = <1>; diff --git a/arch/arc/boot/dts/skeleton.dtsi b/arch/arc/boot/dts/skeleton.dtsi index 296d371a335c..bcb08b36210d 100644 --- a/arch/arc/boot/dts/skeleton.dtsi +++ b/arch/arc/boot/dts/skeleton.dtsi @@ -30,6 +30,18 @@ }; }; + timer0: timer_clkevt { + compatible = "snps,arc-timer0"; + interrupts = <3>; + interrupt-parent = <&core_intc>; + clocks = <&core_clk>; + }; + + timer1: timer_clksrc { + compatible = "snps,arc-timer1"; + clocks = <&core_clk>; + }; + memory { device_type = "memory"; reg = <0x80000000 0x10000000>; /* 256M */ diff --git a/arch/arc/boot/dts/skeleton_hs.dtsi b/arch/arc/boot/dts/skeleton_hs.dtsi index a53876669030..46c5b05aea90 100644 --- a/arch/arc/boot/dts/skeleton_hs.dtsi +++ b/arch/arc/boot/dts/skeleton_hs.dtsi @@ -25,6 +25,18 @@ }; }; + timer0: timer_clkevt { + compatible = "snps,arc-timer0"; + interrupts = <16>; + interrupt-parent = <&core_intc>; + clocks = <&core_clk>; + }; + + timer1: timer_clksrc { + compatible = "snps,arc-timer1"; + clocks = <&core_clk>; + }; + memory { device_type = "memory"; reg = <0x80000000 0x10000000>; /* 256M */ diff --git a/arch/arc/boot/dts/skeleton_hs_idu.dtsi b/arch/arc/boot/dts/skeleton_hs_idu.dtsi index 74898d017f7a..2a40bd9e2e2a 100644 --- a/arch/arc/boot/dts/skeleton_hs_idu.dtsi +++ b/arch/arc/boot/dts/skeleton_hs_idu.dtsi @@ -25,6 +25,18 @@ }; }; + timer0: timer_clkevt { + compatible = "snps,arc-timer0"; + interrupts = <16>; + interrupt-parent = <&core_intc>; + clocks = <&core_clk>; + }; + + timer1: timer_clksrc { + compatible = "snps,archs-timer-gfrc"; + clocks = <&core_clk>; + }; + memory { device_type = "memory"; reg = <0x80000000 0x10000000>; /* 256M */
ARC Timers have historically been probed directly. As precursor to start probing Timers thru DT introduce these bindings Cc: Daniel Lezcano <daniel.lezcano@linaro.org> Cc: Rob Herring <robh@kernel.org> Cc: devicetree@vger.kernel.org Signed-off-by: Vineet Gupta <vgupta@synopsys.com> --- .../devicetree/bindings/timer/snps,arc-timer0.txt | 23 ++++++++++++++++++++++ .../devicetree/bindings/timer/snps,arc-timer1.txt | 17 ++++++++++++++++ .../devicetree/bindings/timer/snps,archs-gfrc.txt | 14 +++++++++++++ .../devicetree/bindings/timer/snps,archs-rtc.txt | 14 +++++++++++++ arch/arc/boot/dts/abilis_tb10x.dtsi | 12 +++++++++++ arch/arc/boot/dts/skeleton.dtsi | 12 +++++++++++ arch/arc/boot/dts/skeleton_hs.dtsi | 12 +++++++++++ arch/arc/boot/dts/skeleton_hs_idu.dtsi | 12 +++++++++++ 8 files changed, 116 insertions(+) create mode 100644 Documentation/devicetree/bindings/timer/snps,arc-timer0.txt create mode 100644 Documentation/devicetree/bindings/timer/snps,arc-timer1.txt create mode 100644 Documentation/devicetree/bindings/timer/snps,archs-gfrc.txt create mode 100644 Documentation/devicetree/bindings/timer/snps,archs-rtc.txt