diff mbox

[1/8] Documentation: devicetree: Update Exynos MCT bindings description

Message ID 1377006768-23174-2-git-send-email-t.figa@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tomasz Figa Aug. 20, 2013, 1:52 p.m. UTC
This patch updates description of device tree bindings for Exynos MCT
(multicore timers). Namely:
 - added note about simplified specification of local timer interrupts,
   when using single per-processor interrupt for all local timers,
 - changed first example that was incorrectly suggesting that global
   timer interrupts are optional,
 - simplified example interrupt map,
 - added example showing simplified local timer interrupt specification.

Signed-off-by: Tomasz Figa <t.figa@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 .../bindings/timer/samsung,exynos4210-mct.txt      | 49 ++++++++++++++--------
 1 file changed, 32 insertions(+), 17 deletions(-)

Comments

Stephen Warren Aug. 20, 2013, 5 p.m. UTC | #1
On 08/20/2013 07:52 AM, Tomasz Figa wrote:
> This patch updates description of device tree bindings for Exynos MCT
> (multicore timers). Namely:
>  - added note about simplified specification of local timer interrupts,
>    when using single per-processor interrupt for all local timers,
>  - changed first example that was incorrectly suggesting that global
>    timer interrupts are optional,
>  - simplified example interrupt map,
>  - added example showing simplified local timer interrupt specification.

> diff --git a/Documentation/devicetree/bindings/timer/samsung,exynos4210-mct.txt b/Documentation/devicetree/bindings/timer/samsung,exynos4210-mct.txt

> -Example 1: In this example, the system uses only the first global timer
> -	   interrupt generated by MCT and the remaining three global timer
> -	   interrupts are unused. Two local timer interrupts have been
> -	   specified.
> +  For MCT block that uses a per-processor interrupt for local timers, such
> +  as ones compatible with "samusng,exynos4412-mct", only one local timer

samsung is typo'd there.

> +Example 2: In this example, the timer interrupts are connected to two separate
> +	   interrupt controllers. Hence, an interrupt-map is created to map
> +	   the interrupts to the respective interrupt controllers.
>  
>  	mct@101C0000 {
>  		compatible = "samsung,exynos4210-mct";
>  		reg = <0x101C0000 0x800>;
> -		interrupt-controller;
> -		#interrups-cells = <2>;
>  		interrupt-parent = <&mct_map>;
> -		interrupts = <0 0>, <1 0>, <2 0>, <3 0>,
> -			     <4 0>, <5 0>;
> +		interrupts = <0>, <1>, <2>, <3>, <4>, <5>;
>  
>  		mct_map: mct-map {
> -			#interrupt-cells = <2>;
> +			#interrupt-cells = <1>;

>  			#address-cells = <0>;
>  			#size-cells = <0>;

I don't believe you need either of those two properties in a node solely
used as an interrupt map.

Also, why not put the interrupt-map property directly into the main mct
node; I don't believe there's any requirement nor advantage to it being
a separate node.

> -			interrupt-map = <0x0 0 &combiner 23 3>,
> -					<0x4 0 &gic 0 120 0>,
> -					<0x5 0 &gic 0 121 0>;
> +			interrupt-map = <0 &gic 0 57 0>,
> +					<1 &gic 0 69 0>,
> +					<2 &combiner 12 6>,
> +					<3 &combiner 12 7>,
> +					<4 &gic 0 42 0>,
> +					<5 &gic 0 48 0>;
>  		};
>  	};

> +Example 3: In this example, the IP contains four local timers, but using
> +	   a per-processor interrupt to handle them. Either all the local
> +	   timer interrupts can be specified, with the same interrupt specifier
> +	   value or just the first one.

That sounds like it should be two separate examples.

Actually, there's already a 2-timer example above using separate
interrupts, so why not make this example *just* be for the
single-interrupt case?

> +	mct@10050000 {
> +		compatible = "samsung,exynos4412-mct";
> +		reg = <0x10050000 0x800>;
> +		interrupts = <0 57 0>, <0 69 0>, <0 70 0>, <0 71 0>,
> +			     <0 42 0>/*, <0 42 0>, <0 42 0>, <0 42 0>*/;
> +	};
Tomasz Figa Aug. 20, 2013, 5:12 p.m. UTC | #2
On Tuesday 20 of August 2013 11:00:53 Stephen Warren wrote:
> On 08/20/2013 07:52 AM, Tomasz Figa wrote:
> > This patch updates description of device tree bindings for Exynos MCT
> > 
> > (multicore timers). Namely:
> >  - added note about simplified specification of local timer interrupts,
> >  
> >    when using single per-processor interrupt for all local timers,
> >  
> >  - changed first example that was incorrectly suggesting that global
> >  
> >    timer interrupts are optional,
> >  
> >  - simplified example interrupt map,
> >  - added example showing simplified local timer interrupt
> >  specification.
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/timer/samsung,exynos4210-mct.txt
> > b/Documentation/devicetree/bindings/timer/samsung,exynos4210-mct.txt
> > 
> > -Example 1: In this example, the system uses only the first global
> > timer
> > -	   interrupt generated by MCT and the remaining three global timer
> > -	   interrupts are unused. Two local timer interrupts have been
> > -	   specified.
> > +  For MCT block that uses a per-processor interrupt for local timers,
> > such +  as ones compatible with "samusng,exynos4412-mct", only one
> > local timer
> samsung is typo'd there.

Oops. ;)

> > +Example 2: In this example, the timer interrupts are connected to two
> > separate +	   interrupt controllers. Hence, an interrupt-map is
> > created to map +	   the interrupts to the respective interrupt
> > controllers.
> > 
> >  	mct@101C0000 {
> >  	
> >  		compatible = "samsung,exynos4210-mct";
> >  		reg = <0x101C0000 0x800>;
> > 
> > -		interrupt-controller;
> > -		#interrups-cells = <2>;
> > 
> >  		interrupt-parent = <&mct_map>;
> > 
> > -		interrupts = <0 0>, <1 0>, <2 0>, <3 0>,
> > -			     <4 0>, <5 0>;
> > +		interrupts = <0>, <1>, <2>, <3>, <4>, <5>;
> > 
> >  		mct_map: mct-map {
> > 
> > -			#interrupt-cells = <2>;
> > +			#interrupt-cells = <1>;
> > 
> >  			#address-cells = <0>;
> >  			#size-cells = <0>;
> 
> I don't believe you need either of those two properties in a node solely
> used as an interrupt map.

Well, you don't need #size-cells, as it is not used for interrupt-map 
property.

As for #address-cell property, you need it, as it defines how many cells 
are used in interrupt map specifier for unit address. See ePAPR 2.4.3.1 or 
[1] for a description of interrupt-map property format.

[1] - http://devicetree.org/Device_Tree_Usage#Advanced_Interrupt_Mapping

> Also, why not put the interrupt-map property directly into the main mct
> node; I don't believe there's any requirement nor advantage to it being
> a separate node.

It is more readable, as you don't mix virtual (helper) properties, with 
those describing the hardware. Otherwise both ways are technically correct, 
but not for all cases, i.e. only when #address-cells and #interrupt-cells 
properties aren't used for device's own purposes.

> > -			interrupt-map = <0x0 0 &combiner 23 3>,
> > -					<0x4 0 &gic 0 120 0>,
> > -					<0x5 0 &gic 0 121 0>;
> > +			interrupt-map = <0 &gic 0 57 0>,
> > +					<1 &gic 0 69 0>,
> > +					<2 &combiner 12 6>,
> > +					<3 &combiner 12 7>,
> > +					<4 &gic 0 42 0>,
> > +					<5 &gic 0 48 0>;
> > 
> >  		};
> >  	
> >  	};
> > 
> > +Example 3: In this example, the IP contains four local timers, but
> > using +	   a per-processor interrupt to handle them. Either all the
> > local +	   timer interrupts can be specified, with the same interrupt
> > specifier +	   value or just the first one.
> 
> That sounds like it should be two separate examples.
> 
> Actually, there's already a 2-timer example above using separate
> interrupts, so why not make this example *just* be for the
> single-interrupt case?

Well, I wanted to show that both ways of specification would be equivalent 
here. If you insist on making it a single example, then I can send next 
version with this changed.

Best regards,
Tomasz
Stephen Warren Aug. 20, 2013, 8:41 p.m. UTC | #3
On 08/20/2013 11:12 AM, Tomasz Figa wrote:
> On Tuesday 20 of August 2013 11:00:53 Stephen Warren wrote:
>> On 08/20/2013 07:52 AM, Tomasz Figa wrote:
>>> This patch updates description of device tree bindings for Exynos MCT
>>>
>>> (multicore timers). Namely:
>>>  - added note about simplified specification of local timer interrupts,
>>>  
>>>    when using single per-processor interrupt for all local timers,
>>>  
>>>  - changed first example that was incorrectly suggesting that global
>>>  
>>>    timer interrupts are optional,
>>>  
>>>  - simplified example interrupt map,
>>>  - added example showing simplified local timer interrupt
>>>  specification.
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/timer/samsung,exynos4210-mct.txt
>>> b/Documentation/devicetree/bindings/timer/samsung,exynos4210-mct.txt
>>>
>>> -Example 1: In this example, the system uses only the first global
>>> timer
>>> -	   interrupt generated by MCT and the remaining three global timer
>>> -	   interrupts are unused. Two local timer interrupts have been
>>> -	   specified.
>>> +  For MCT block that uses a per-processor interrupt for local timers,
>>> such +  as ones compatible with "samusng,exynos4412-mct", only one
>>> local timer
>> samsung is typo'd there.
> 
> Oops. ;)
> 
>>> +Example 2: In this example, the timer interrupts are connected to two
>>> separate +	   interrupt controllers. Hence, an interrupt-map is
>>> created to map +	   the interrupts to the respective interrupt
>>> controllers.
>>>
>>>  	mct@101C0000 {
>>>  	
>>>  		compatible = "samsung,exynos4210-mct";
>>>  		reg = <0x101C0000 0x800>;
>>>
>>> -		interrupt-controller;
>>> -		#interrups-cells = <2>;
>>>
>>>  		interrupt-parent = <&mct_map>;
>>>
>>> -		interrupts = <0 0>, <1 0>, <2 0>, <3 0>,
>>> -			     <4 0>, <5 0>;
>>> +		interrupts = <0>, <1>, <2>, <3>, <4>, <5>;
>>>
>>>  		mct_map: mct-map {
>>>
>>> -			#interrupt-cells = <2>;
>>> +			#interrupt-cells = <1>;
>>>
>>>  			#address-cells = <0>;
>>>  			#size-cells = <0>;
>>
>> I don't believe you need either of those two properties in a node solely
>> used as an interrupt map.
> 
> Well, you don't need #size-cells, as it is not used for interrupt-map 
> property.
> 
> As for #address-cell property, you need it, as it defines how many cells 
> are used in interrupt map specifier for unit address. See ePAPR 2.4.3.1 or 
> [1] for a description of interrupt-map property format.
> 
> [1] - http://devicetree.org/Device_Tree_Usage#Advanced_Interrupt_Mapping

Uggh. Yes, you're right.

>> Also, why not put the interrupt-map property directly into the main mct
>> node; I don't believe there's any requirement nor advantage to it being
>> a separate node.
> 
> It is more readable, as you don't mix virtual (helper) properties, with 
> those describing the hardware. Otherwise both ways are technically correct, 
> but not for all cases, i.e. only when #address-cells and #interrupt-cells 
> properties aren't used for device's own purposes.

Hmm. I see the argument.

>>> +Example 3: In this example, the IP contains four local timers, but
>>> using +	   a per-processor interrupt to handle them. Either all the
>>> local +	   timer interrupts can be specified, with the same interrupt
>>> specifier +	   value or just the first one.
>>
>> That sounds like it should be two separate examples.
>>
>> Actually, there's already a 2-timer example above using separate
>> interrupts, so why not make this example *just* be for the
>> single-interrupt case?
> 
> Well, I wanted to show that both ways of specification would be equivalent 
> here. If you insist on making it a single example, then I can send next 
> version with this changed.

Oh! I didn't see the /* */ at all in the third example...

I think it'd be more obvious if you wrote the whole property out twice:

+		interrupts = <0 57 0>, <0 69 0>, <0 70 0>, <0 71 0>,
+			     <0 42 0>/*, <0 42 0>, <0 42 0>, <0 42 0>*/;
+		/* or: */
+		interrupts = <0 57 0>, <0 69 0>, <0 70 0>, <0 71 0>,
+			     <0 42 0>;
Tomasz Figa Aug. 20, 2013, 10:13 p.m. UTC | #4
On Tuesday 20 of August 2013 14:41:15 Stephen Warren wrote:
> On 08/20/2013 11:12 AM, Tomasz Figa wrote:
> > On Tuesday 20 of August 2013 11:00:53 Stephen Warren wrote:
> >> On 08/20/2013 07:52 AM, Tomasz Figa wrote:
> >>> This patch updates description of device tree bindings for Exynos
> >>> MCT
> >>> 
> >>> (multicore timers). Namely:
> >>>  - added note about simplified specification of local timer
> >>>  interrupts,
> >>>  
> >>>    when using single per-processor interrupt for all local timers,
> >>>  
> >>>  - changed first example that was incorrectly suggesting that global
> >>>  
> >>>    timer interrupts are optional,
> >>>  
> >>>  - simplified example interrupt map,
> >>>  - added example showing simplified local timer interrupt
> >>>  specification.
> >>> 
> >>> diff --git
> >>> a/Documentation/devicetree/bindings/timer/samsung,exynos4210-mct.txt
> >>> b/Documentation/devicetree/bindings/timer/samsung,exynos4210-mct.txt
> >>> 
> >>> -Example 1: In this example, the system uses only the first global
> >>> timer
> >>> -	   interrupt generated by MCT and the remaining three global timer
> >>> -	   interrupts are unused. Two local timer interrupts have been
> >>> -	   specified.
> >>> +  For MCT block that uses a per-processor interrupt for local
> >>> timers,
> >>> such +  as ones compatible with "samusng,exynos4412-mct", only one
> >>> local timer
> >> 
> >> samsung is typo'd there.
> > 
> > Oops. ;)
> > 
> >>> +Example 2: In this example, the timer interrupts are connected to
> >>> two
> >>> separate +	   interrupt controllers. Hence, an interrupt-map 
is
> >>> created to map +	   the interrupts to the respective interrupt
> >>> controllers.
> >>> 
> >>>  	mct@101C0000 {
> >>>  	
> >>>  		compatible = "samsung,exynos4210-mct";
> >>>  		reg = <0x101C0000 0x800>;
> >>> 
> >>> -		interrupt-controller;
> >>> -		#interrups-cells = <2>;
> >>> 
> >>>  		interrupt-parent = <&mct_map>;
> >>> 
> >>> -		interrupts = <0 0>, <1 0>, <2 0>, <3 0>,
> >>> -			     <4 0>, <5 0>;
> >>> +		interrupts = <0>, <1>, <2>, <3>, <4>, <5>;
> >>> 
> >>>  		mct_map: mct-map {
> >>> 
> >>> -			#interrupt-cells = <2>;
> >>> +			#interrupt-cells = <1>;
> >>> 
> >>>  			#address-cells = <0>;
> >>>  			#size-cells = <0>;
> >> 
> >> I don't believe you need either of those two properties in a node
> >> solely used as an interrupt map.
> > 
> > Well, you don't need #size-cells, as it is not used for interrupt-map
> > property.
> > 
> > As for #address-cell property, you need it, as it defines how many
> > cells are used in interrupt map specifier for unit address. See ePAPR
> > 2.4.3.1 or [1] for a description of interrupt-map property format.
> > 
> > [1] -
> > http://devicetree.org/Device_Tree_Usage#Advanced_Interrupt_Mapping
> Uggh. Yes, you're right.
> 
> >> Also, why not put the interrupt-map property directly into the main
> >> mct
> >> node; I don't believe there's any requirement nor advantage to it
> >> being
> >> a separate node.
> > 
> > It is more readable, as you don't mix virtual (helper) properties,
> > with
> > those describing the hardware. Otherwise both ways are technically
> > correct, but not for all cases, i.e. only when #address-cells and
> > #interrupt-cells properties aren't used for device's own purposes.
> 
> Hmm. I see the argument.
> 
> >>> +Example 3: In this example, the IP contains four local timers, but
> >>> using +	   a per-processor interrupt to handle them. Either all 
the
> >>> local +	   timer interrupts can be specified, with the same
> >>> interrupt
> >>> specifier +	   value or just the first one.
> >> 
> >> That sounds like it should be two separate examples.
> >> 
> >> Actually, there's already a 2-timer example above using separate
> >> interrupts, so why not make this example *just* be for the
> >> single-interrupt case?
> > 
> > Well, I wanted to show that both ways of specification would be
> > equivalent here. If you insist on making it a single example, then I
> > can send next version with this changed.
> 
> Oh! I didn't see the /* */ at all in the third example...
> 
> I think it'd be more obvious if you wrote the whole property out twice:
> 
> +		interrupts = <0 57 0>, <0 69 0>, <0 70 0>, <0 71 0>,
> +			     <0 42 0>/*, <0 42 0>, <0 42 0>, <0 42 0>*/;
> +		/* or: */
> +		interrupts = <0 57 0>, <0 69 0>, <0 70 0>, <0 71 0>,
> +			     <0 42 0>;

That's a good idea. I will send new version with the typo above fixed and 
this example done the way you proposed.

Best regards,
Tomasz
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/timer/samsung,exynos4210-mct.txt b/Documentation/devicetree/bindings/timer/samsung,exynos4210-mct.txt
index b5a86d2..a5d6891f 100644
--- a/Documentation/devicetree/bindings/timer/samsung,exynos4210-mct.txt
+++ b/Documentation/devicetree/bindings/timer/samsung,exynos4210-mct.txt
@@ -31,38 +31,53 @@  Required properties:
 	7: ..
 	i: Local Timer Interrupt n
 
-Example 1: In this example, the system uses only the first global timer
-	   interrupt generated by MCT and the remaining three global timer
-	   interrupts are unused. Two local timer interrupts have been
-	   specified.
+  For MCT block that uses a per-processor interrupt for local timers, such
+  as ones compatible with "samusng,exynos4412-mct", only one local timer
+  interrupt might be specified, meaning that all local timers use the same
+  per processor interrupt.
+
+Example 1: In this example, the IP contains two local timers, using separate
+	   interrupts, so two local timer interrupts have been specified,
+	   in addition to four global timer interrupts.
 
 	mct@10050000 {
 		compatible = "samsung,exynos4210-mct";
 		reg = <0x10050000 0x800>;
-		interrupts = <0 57 0>, <0 0 0>, <0 0 0>, <0 0 0>,
+		interrupts = <0 57 0>, <0 69 0>, <0 70 0>, <0 71 0>,
 			     <0 42 0>, <0 48 0>;
 	};
 
-Example 2: In this example, the MCT global and local timer interrupts are
-	   connected to two separate interrupt controllers. Hence, an
-	   interrupt-map is created to map the interrupts to the respective
-	   interrupt controllers.
+Example 2: In this example, the timer interrupts are connected to two separate
+	   interrupt controllers. Hence, an interrupt-map is created to map
+	   the interrupts to the respective interrupt controllers.
 
 	mct@101C0000 {
 		compatible = "samsung,exynos4210-mct";
 		reg = <0x101C0000 0x800>;
-		interrupt-controller;
-		#interrups-cells = <2>;
 		interrupt-parent = <&mct_map>;
-		interrupts = <0 0>, <1 0>, <2 0>, <3 0>,
-			     <4 0>, <5 0>;
+		interrupts = <0>, <1>, <2>, <3>, <4>, <5>;
 
 		mct_map: mct-map {
-			#interrupt-cells = <2>;
+			#interrupt-cells = <1>;
 			#address-cells = <0>;
 			#size-cells = <0>;
-			interrupt-map = <0x0 0 &combiner 23 3>,
-					<0x4 0 &gic 0 120 0>,
-					<0x5 0 &gic 0 121 0>;
+			interrupt-map = <0 &gic 0 57 0>,
+					<1 &gic 0 69 0>,
+					<2 &combiner 12 6>,
+					<3 &combiner 12 7>,
+					<4 &gic 0 42 0>,
+					<5 &gic 0 48 0>;
 		};
 	};
+
+Example 3: In this example, the IP contains four local timers, but using
+	   a per-processor interrupt to handle them. Either all the local
+	   timer interrupts can be specified, with the same interrupt specifier
+	   value or just the first one.
+
+	mct@10050000 {
+		compatible = "samsung,exynos4412-mct";
+		reg = <0x10050000 0x800>;
+		interrupts = <0 57 0>, <0 69 0>, <0 70 0>, <0 71 0>,
+			     <0 42 0>/*, <0 42 0>, <0 42 0>, <0 42 0>*/;
+	};