| Message ID | 1391607050-540-3-git-send-email-marc.zyngier@arm.com |
|---|---|
| State | New |
| Headers | show |
Hi Arnab, On 07/02/14 05:41, Arnab Basu wrote: > Hi Marc > > Marc Zyngier <marc.zyngier <at> arm.com> writes: > >> + >> +AArch64 SMP cores are often associated with a GICv3, providing private >> +peripheral interrupts (PPI), shared peripheral interrupts (SPI), >> +software generated interrupts (SGI), and locality-specific peripheral >> +Interrupts (LPI). >> + >> +Main node required properties: >> + >> +- compatible : should at least contain "arm,gic-v3". >> +- interrupt-controller : Identifies the node as an interrupt controller >> +- #interrupt-cells : Specifies the number of cells needed to encode an >> + interrupt source. Must be a single cell with a value of at least 3. >> + >> + The 1st cell is the interrupt type; 0 for SPI interrupts, 1 for PPI >> + interrupts. Other values are reserved for future use. > > These values are defined in > "include/dt-bindings/interrupt-controller/arm-gic.h" maybe we should start > mentioning that here and encourage future device treese to use those defines > to improve readability. It may improve readability, but it makes the definition rely on something else. Definition and usage are two different things, and I want the definition to be completely self-contained and non ambiguous. In DTS files, people can use whatever macro they decide, and it is their problem. They will even have out of tree DTS files, for which the include file is not available. > >> + >> + The 2nd cell contains the interrupt number for the interrupt type. >> + SPI interrupts are in the range [0-987]. PPI interrupts are in the >> + range [0-15]. >> + >> + The 3rd cell is the flags, encoded as follows: >> + bits[3:0] trigger type and level flags. >> + 1 = edge triggered >> + 2 = edge triggered (deprecated, for compatibility with GICv2) >> + 4 = level triggered >> + 8 = level triggered (deprecated, for compatibility with GICv2) > > Similar to the above comment > "include/dt-bindings/interrupt-controller/irq.h" defines the trigger type > and level flags. Although this file currently contains the GICv2 bindings, > we could update them. Same as above. Thanks for the review! M.
On 13/02/14 13:26, Rob Herring wrote: > On Wed, Feb 5, 2014 at 7:30 AM, Marc Zyngier <marc.zyngier@arm.com> wrote: >> Add the necessary documentation to support GICv3. >> >> Acked-by: Catalin Marinas <catalin.marinas@arm.com> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >> --- >> Documentation/devicetree/bindings/arm/gic-v3.txt | 81 ++++++++++++++++++++++++ >> 1 file changed, 81 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/arm/gic-v3.txt >> >> diff --git a/Documentation/devicetree/bindings/arm/gic-v3.txt b/Documentation/devicetree/bindings/arm/gic-v3.txt >> new file mode 100644 >> index 0000000..93852f6 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/arm/gic-v3.txt >> @@ -0,0 +1,81 @@ >> +* ARM Generic Interrupt Controller, version 3 >> + >> +AArch64 SMP cores are often associated with a GICv3, providing private >> +peripheral interrupts (PPI), shared peripheral interrupts (SPI), >> +software generated interrupts (SGI), and locality-specific peripheral >> +Interrupts (LPI). >> + >> +Main node required properties: >> + >> +- compatible : should at least contain "arm,gic-v3". >> +- interrupt-controller : Identifies the node as an interrupt controller >> +- #interrupt-cells : Specifies the number of cells needed to encode an >> + interrupt source. Must be a single cell with a value of at least 3. >> + >> + The 1st cell is the interrupt type; 0 for SPI interrupts, 1 for PPI >> + interrupts. Other values are reserved for future use. >> + >> + The 2nd cell contains the interrupt number for the interrupt type. >> + SPI interrupts are in the range [0-987]. PPI interrupts are in the >> + range [0-15]. >> + >> + The 3rd cell is the flags, encoded as follows: >> + bits[3:0] trigger type and level flags. >> + 1 = edge triggered >> + 2 = edge triggered (deprecated, for compatibility with GICv2) >> + 4 = level triggered >> + 8 = level triggered (deprecated, for compatibility with GICv2) > > I don't really think compatibility is needed here. I'd just list 1 and 4. Fair enough. >> + >> + Cells 4 and beyond are reserved for future use. Where the 1st cell >> + has a value of 0 or 1, cells 4 and beyond act as padding, and may be >> + ignored. It is recommended that padding cells have a value of 0. > > What future use? LPIs I suppose. That's a possibility, for non-PCI devices. I'd rather not do that though, but I thought that leaving some room for (wacky) expansion. > Otherwise, looks fine to me: > > Acked-by: Rob Herring <robh@kernel.org> Thanks! M.
On Wed, Feb 05, 2014 at 01:30:34PM +0000, Marc Zyngier wrote: > Add the necessary documentation to support GICv3. > > Acked-by: Catalin Marinas <catalin.marinas@arm.com> > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > --- > Documentation/devicetree/bindings/arm/gic-v3.txt | 81 ++++++++++++++++++++++++ > 1 file changed, 81 insertions(+) > create mode 100644 Documentation/devicetree/bindings/arm/gic-v3.txt > > diff --git a/Documentation/devicetree/bindings/arm/gic-v3.txt b/Documentation/devicetree/bindings/arm/gic-v3.txt > new file mode 100644 > index 0000000..93852f6 > --- /dev/null > +++ b/Documentation/devicetree/bindings/arm/gic-v3.txt > @@ -0,0 +1,81 @@ > +* ARM Generic Interrupt Controller, version 3 > + > +AArch64 SMP cores are often associated with a GICv3, providing private > +peripheral interrupts (PPI), shared peripheral interrupts (SPI), > +software generated interrupts (SGI), and locality-specific peripheral > +Interrupts (LPI). Super-over-ridiculous-nit: capitalize the definitions, e.g. Private Peripheral Interrupts (PPI) > + > +Main node required properties: > + > +- compatible : should at least contain "arm,gic-v3". > +- interrupt-controller : Identifies the node as an interrupt controller > +- #interrupt-cells : Specifies the number of cells needed to encode an > + interrupt source. Must be a single cell with a value of at least 3. > + > + The 1st cell is the interrupt type; 0 for SPI interrupts, 1 for PPI > + interrupts. Other values are reserved for future use. > + > + The 2nd cell contains the interrupt number for the interrupt type. > + SPI interrupts are in the range [0-987]. PPI interrupts are in the > + range [0-15]. > + > + The 3rd cell is the flags, encoded as follows: > + bits[3:0] trigger type and level flags. > + 1 = edge triggered > + 2 = edge triggered (deprecated, for compatibility with GICv2) > + 4 = level triggered > + 8 = level triggered (deprecated, for compatibility with GICv2) > + > + Cells 4 and beyond are reserved for future use. Where the 1st cell > + has a value of 0 or 1, cells 4 and beyond act as padding, and may be > + ignored. It is recommended that padding cells have a value of 0. another-super-nit: Saying "If the 1st cell" or "When the 1st cell" sounds more clear to me, but I may be wrong. > + > +- reg : Specifies base physical address(s) and size of the GIC > + registers, in the following order: > + - GIC Distributor interface (GICD) > + - GIC Redistributors (GICR), one range per redistributor region > + - GIC CPU interface (GICC) > + - GIC Hypervisor interface (GICH) > + - GIC Virtual CPU interface (GICV) > + > + GICC, GICH and GICV are optional. > + > +- interrupts : Interrupt source of the VGIC maintenance interrupt. > + > +Optional > + > +- redistributor-stride : If using padding pages, specifies the stride > + of consecutive redistributors. Must be a multiple of 64kB. How is this going to be used? What would the size of the padding then be? I couldn't find anything about this in the specs. Or does this mean the total size of the set of contiguous pages for each of the redistributors? That seems to make the math in the example below work. > + > +- #redistributor-regions: The number of independent contiguous regions > + occupied by the redistributors. Required if more than one such > + region is present. > + > +Examples: > + > + gic: interrupt-controller@2cf00000 { > + compatible = "arm,gic-v3"; > + #interrupt-cells = <3>; > + interrupt-controller; > + reg = <0x0 0x2f000000 0 0x10000>, // GICD > + <0x0 0x2f100000 0 0x200000>, // GICR > + <0x0 0x2c000000 0 0x2000>, // GICC > + <0x0 0x2c010000 0 0x2000>, // GICH > + <0x0 0x2c020000 0 0x2000>; // GICV > + interrupts = <1 9 4>; > + }; > + > + gic: interrupt-controller@2c010000 { > + compatible = "arm,gic-v3"; > + #interrupt-cells = <3>; > + interrupt-controller; > + redistributor-stride = 0x40000; // 256kB stride I raised the point in patch 1 about the stide value being read as a 64-bit value, which would imply that the example should be the following, I think: + redistributor-stride = <0x0 0x40000>; // 256kB stride > + #redistributor-regions = <2>; > + reg = <0x0 0x2c010000 0 0x10000>, // GICD > + <0x0 0x2d000000 0 0x800000>, // GICR 1: CPUs 0-31 > + <0x0 0x2e000000 0 0x800000>; // GICR 2: CPUs 32-63 > + <0x0 0x2c040000 0 0x2000>, // GICC > + <0x0 0x2c060000 0 0x2000>, // GICH > + <0x0 0x2c080000 0 0x2000>; // GICV > + interrupts = <1 9 4>; > + }; > -- > 1.8.3.4 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel Otherwise it looks good.
On 17/02/14 01:21, Christoffer Dall wrote: > On Wed, Feb 05, 2014 at 01:30:34PM +0000, Marc Zyngier wrote: >> Add the necessary documentation to support GICv3. >> >> Acked-by: Catalin Marinas <catalin.marinas@arm.com> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >> --- >> Documentation/devicetree/bindings/arm/gic-v3.txt | 81 ++++++++++++++++++++++++ >> 1 file changed, 81 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/arm/gic-v3.txt >> >> diff --git a/Documentation/devicetree/bindings/arm/gic-v3.txt b/Documentation/devicetree/bindings/arm/gic-v3.txt >> new file mode 100644 >> index 0000000..93852f6 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/arm/gic-v3.txt >> @@ -0,0 +1,81 @@ >> +* ARM Generic Interrupt Controller, version 3 >> + >> +AArch64 SMP cores are often associated with a GICv3, providing private >> +peripheral interrupts (PPI), shared peripheral interrupts (SPI), >> +software generated interrupts (SGI), and locality-specific peripheral >> +Interrupts (LPI). > > Super-over-ridiculous-nit: capitalize the definitions, e.g. Private > Peripheral Interrupts (PPI) Sure. >> + >> +Main node required properties: >> + >> +- compatible : should at least contain "arm,gic-v3". >> +- interrupt-controller : Identifies the node as an interrupt controller >> +- #interrupt-cells : Specifies the number of cells needed to encode an >> + interrupt source. Must be a single cell with a value of at least 3. >> + >> + The 1st cell is the interrupt type; 0 for SPI interrupts, 1 for PPI >> + interrupts. Other values are reserved for future use. >> + >> + The 2nd cell contains the interrupt number for the interrupt type. >> + SPI interrupts are in the range [0-987]. PPI interrupts are in the >> + range [0-15]. >> + >> + The 3rd cell is the flags, encoded as follows: >> + bits[3:0] trigger type and level flags. >> + 1 = edge triggered >> + 2 = edge triggered (deprecated, for compatibility with GICv2) >> + 4 = level triggered >> + 8 = level triggered (deprecated, for compatibility with GICv2) >> + >> + Cells 4 and beyond are reserved for future use. Where the 1st cell >> + has a value of 0 or 1, cells 4 and beyond act as padding, and may be >> + ignored. It is recommended that padding cells have a value of 0. > > another-super-nit: Saying "If the 1st cell" or "When the 1st cell" > sounds more clear to me, but I may be wrong. Works for me. >> + >> +- reg : Specifies base physical address(s) and size of the GIC >> + registers, in the following order: >> + - GIC Distributor interface (GICD) >> + - GIC Redistributors (GICR), one range per redistributor region >> + - GIC CPU interface (GICC) >> + - GIC Hypervisor interface (GICH) >> + - GIC Virtual CPU interface (GICV) >> + >> + GICC, GICH and GICV are optional. >> + >> +- interrupts : Interrupt source of the VGIC maintenance interrupt. >> + >> +Optional >> + >> +- redistributor-stride : If using padding pages, specifies the stride >> + of consecutive redistributors. Must be a multiple of 64kB. > > How is this going to be used? What would the size of the padding then > be? I couldn't find anything about this in the specs. > > Or does this mean the total size of the set of contiguous pages for each > of the redistributors? That seems to make the math in the example below > work. Indeed. This is to allow overriding of the size of a single redistributor. The driver will follow the spec to the letter (two or four 64k pages, depending on GICR_TYPER.VLPI), except when this parameter is present. >> + >> +- #redistributor-regions: The number of independent contiguous regions >> + occupied by the redistributors. Required if more than one such >> + region is present. >> + >> +Examples: >> + >> + gic: interrupt-controller@2cf00000 { >> + compatible = "arm,gic-v3"; >> + #interrupt-cells = <3>; >> + interrupt-controller; >> + reg = <0x0 0x2f000000 0 0x10000>, // GICD >> + <0x0 0x2f100000 0 0x200000>, // GICR >> + <0x0 0x2c000000 0 0x2000>, // GICC >> + <0x0 0x2c010000 0 0x2000>, // GICH >> + <0x0 0x2c020000 0 0x2000>; // GICV >> + interrupts = <1 9 4>; >> + }; >> + >> + gic: interrupt-controller@2c010000 { >> + compatible = "arm,gic-v3"; >> + #interrupt-cells = <3>; >> + interrupt-controller; >> + redistributor-stride = 0x40000; // 256kB stride > > I raised the point in patch 1 about the stide value being read as a > 64-bit value, which would imply that the example should be the > following, I think: > > + redistributor-stride = <0x0 0x40000>; // 256kB stride Indeed. I'll fix this. Thanks for the review, M.
diff --git a/Documentation/devicetree/bindings/arm/gic-v3.txt b/Documentation/devicetree/bindings/arm/gic-v3.txt new file mode 100644 index 0000000..93852f6 --- /dev/null +++ b/Documentation/devicetree/bindings/arm/gic-v3.txt @@ -0,0 +1,81 @@ +* ARM Generic Interrupt Controller, version 3 + +AArch64 SMP cores are often associated with a GICv3, providing private +peripheral interrupts (PPI), shared peripheral interrupts (SPI), +software generated interrupts (SGI), and locality-specific peripheral +Interrupts (LPI). + +Main node required properties: + +- compatible : should at least contain "arm,gic-v3". +- interrupt-controller : Identifies the node as an interrupt controller +- #interrupt-cells : Specifies the number of cells needed to encode an + interrupt source. Must be a single cell with a value of at least 3. + + The 1st cell is the interrupt type; 0 for SPI interrupts, 1 for PPI + interrupts. Other values are reserved for future use. + + The 2nd cell contains the interrupt number for the interrupt type. + SPI interrupts are in the range [0-987]. PPI interrupts are in the + range [0-15]. + + The 3rd cell is the flags, encoded as follows: + bits[3:0] trigger type and level flags. + 1 = edge triggered + 2 = edge triggered (deprecated, for compatibility with GICv2) + 4 = level triggered + 8 = level triggered (deprecated, for compatibility with GICv2) + + Cells 4 and beyond are reserved for future use. Where the 1st cell + has a value of 0 or 1, cells 4 and beyond act as padding, and may be + ignored. It is recommended that padding cells have a value of 0. + +- reg : Specifies base physical address(s) and size of the GIC + registers, in the following order: + - GIC Distributor interface (GICD) + - GIC Redistributors (GICR), one range per redistributor region + - GIC CPU interface (GICC) + - GIC Hypervisor interface (GICH) + - GIC Virtual CPU interface (GICV) + + GICC, GICH and GICV are optional. + +- interrupts : Interrupt source of the VGIC maintenance interrupt. + +Optional + +- redistributor-stride : If using padding pages, specifies the stride + of consecutive redistributors. Must be a multiple of 64kB. + +- #redistributor-regions: The number of independent contiguous regions + occupied by the redistributors. Required if more than one such + region is present. + +Examples: + + gic: interrupt-controller@2cf00000 { + compatible = "arm,gic-v3"; + #interrupt-cells = <3>; + interrupt-controller; + reg = <0x0 0x2f000000 0 0x10000>, // GICD + <0x0 0x2f100000 0 0x200000>, // GICR + <0x0 0x2c000000 0 0x2000>, // GICC + <0x0 0x2c010000 0 0x2000>, // GICH + <0x0 0x2c020000 0 0x2000>; // GICV + interrupts = <1 9 4>; + }; + + gic: interrupt-controller@2c010000 { + compatible = "arm,gic-v3"; + #interrupt-cells = <3>; + interrupt-controller; + redistributor-stride = 0x40000; // 256kB stride + #redistributor-regions = <2>; + reg = <0x0 0x2c010000 0 0x10000>, // GICD + <0x0 0x2d000000 0 0x800000>, // GICR 1: CPUs 0-31 + <0x0 0x2e000000 0 0x800000>; // GICR 2: CPUs 32-63 + <0x0 0x2c040000 0 0x2000>, // GICC + <0x0 0x2c060000 0 0x2000>, // GICH + <0x0 0x2c080000 0 0x2000>; // GICV + interrupts = <1 9 4>; + };