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

[Tomasz Figa <tomasz.figa@gmail.com>]

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