| Message ID | 1377006768-23174-2-git-send-email-t.figa@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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>*/; > + };
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
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>;
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 --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>*/; + };