[2/9] ARC: [dts] Introduce Timer bindings
diff mbox

Message ID 1454410739-24444-3-git-send-email-vgupta@synopsys.com
State Superseded
Headers show

Commit Message

Vineet Gupta Feb. 2, 2016, 10:58 a.m. UTC
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

Comments

Alexey Brodkin Feb. 2, 2016, 12:48 p.m. UTC | #1
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
Alexey Brodkin Feb. 2, 2016, 1:15 p.m. UTC | #2
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
Vineet Gupta Feb. 2, 2016, 2:29 p.m. UTC | #3
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.
Alexey Brodkin Feb. 2, 2016, 3:36 p.m. UTC | #4
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
Rob Herring Feb. 2, 2016, 10:03 p.m. UTC | #5
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>;
> +	};
Alexey Brodkin Feb. 2, 2016, 10:57 p.m. UTC | #6
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
Vineet Gupta Feb. 3, 2016, 8:04 a.m. UTC | #7
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>;
>> +	};
> --
Alexey Brodkin Feb. 3, 2016, 1:44 p.m. UTC | #8
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
Alexey Brodkin Feb. 3, 2016, 1:50 p.m. UTC | #9
(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
Rob Herring Feb. 3, 2016, 3:39 p.m. UTC | #10
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
Vineet Gupta Feb. 16, 2016, 8:44 a.m. UTC | #11
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 ?

Patch
diff mbox

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 */