| Message ID | 1455512263-18183-3-git-send-email-qnguyen@apm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 15/02/16 04:57, Quan Nguyen wrote: > Update description for X-Gene standby GPIO controller DTS binding to > support GPIO line configuration as input, output or external IRQ pin. > > Signed-off-by: Y Vo <yvo@apm.com> > Signed-off-by: Quan Nguyen <qnguyen@apm.com> > --- > .../devicetree/bindings/gpio/gpio-xgene-sb.txt | 47 ++++++++++++++++++---- > 1 file changed, 40 insertions(+), 7 deletions(-) > > diff --git a/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt b/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt > index dae1300..7b8b4cb 100644 > --- a/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt > +++ b/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt > @@ -1,10 +1,20 @@ > APM X-Gene Standby GPIO controller bindings > > -This is a gpio controller in the standby domain. > - > -There are 20 GPIO pins from 0..21. There is no GPIO_DS14 or GPIO_DS15, > -only GPIO_DS8..GPIO_DS13 support interrupts. The IRQ mapping > -is currently 1-to-1 on interrupts 0x28 thru 0x2d. > +This is a gpio controller in the standby domain. It also supports interrupt in > +some particular pins which are sourced to its parent interrupt controller > +as diagram below: > + +-----------------+ > + | X-Gene standby | > + | GPIO controller +--------- GPIO_0 > ++------------+ | | ... > +| Parent IRQ | | +--------- GPIO_8/EXT_INT_0 > +| controller | EXT_INT_0 | | ... > +| (GICv2) +-------------+ +--------- GPIO_[N+8]/EXT_INT_N > +| | ... | | > +| | EXT_INT_N | +--------- GPIO_[N+9] > +| +-------------+ | ... > +| | | +--------- GPIO_MAX > ++------------+ +-----------------+ > > Required properties: > - compatible: "apm,xgene-gpio-sb" for the X-Gene Standby GPIO controller > @@ -15,10 +25,18 @@ Required properties: > 0 = active high > 1 = active low > - gpio-controller: Marks the device node as a GPIO controller. > -- interrupts: Shall contain exactly 6 interrupts. > +- interrupts: The EXT_INT_0 parent interrupt resource must be listed first. > +- interrupt-parent: Phandle of the parent interrupt controller. > +- interrupt-cells: Should be two. > + - first cell is 0-N coresponding for EXT_INT_0 to EXT_INT_N. > + - second cell is used to specify flags. > +- interrupt-controller: Marks the device node as an interrupt controller. > +- apm,nr-gpios: Optional, specify number of gpios pin. > +- apm,nr-irqs: Optional, specify number of interrupt pins. > +- apm,irq-start: Optional, specify lowest gpio pin support interrupt. This is quite ambiguous. Is that relative to the GIC? Assuming this is the case, you should then document it, specify the type of interrupt (SPI?), and whether this is 0- or 32-based (the code seems to indicate that is is 0-based). > > Example: > - sbgpio: sbgpio@17001000 { > + sbgpio: gpio@17001000{ > compatible = "apm,xgene-gpio-sb"; > reg = <0x0 0x17001000 0x0 0x400>; > #gpio-cells = <2>; > @@ -29,4 +47,19 @@ Example: > <0x0 0x2b 0x1>, > <0x0 0x2c 0x1>, > <0x0 0x2d 0x1>; > + interrupt-parent = <&gic>; > + #interrupt-cells = <2>; > + interrupt-controller; > + apm,nr-gpios = <22>; > + apm,nr-irqs = <6>; > + apm,irq-start = <8>; > + }; > + > + testuser { > + compatible = "example,testuser"; > + /* Use the GPIO_13/EXT_INT_5 line as an active high triggered > + * level interrupt > + */ > + interrupts = <5 4>; > + interrupt-parent = <&sbgpio>; > }; > Thanks, M.
On Mon, Feb 15, 2016 at 7:54 PM, Marc Zyngier <marc.zyngier@arm.com> wrote: > On 15/02/16 04:57, Quan Nguyen wrote: >> Update description for X-Gene standby GPIO controller DTS binding to >> support GPIO line configuration as input, output or external IRQ pin. >> >> Signed-off-by: Y Vo <yvo@apm.com> >> Signed-off-by: Quan Nguyen <qnguyen@apm.com> >> --- >> .../devicetree/bindings/gpio/gpio-xgene-sb.txt | 47 ++++++++++++++++++---- >> 1 file changed, 40 insertions(+), 7 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt b/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt >> index dae1300..7b8b4cb 100644 >> --- a/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt >> +++ b/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt >> @@ -1,10 +1,20 @@ >> APM X-Gene Standby GPIO controller bindings >> >> -This is a gpio controller in the standby domain. >> - >> -There are 20 GPIO pins from 0..21. There is no GPIO_DS14 or GPIO_DS15, >> -only GPIO_DS8..GPIO_DS13 support interrupts. The IRQ mapping >> -is currently 1-to-1 on interrupts 0x28 thru 0x2d. >> +This is a gpio controller in the standby domain. It also supports interrupt in >> +some particular pins which are sourced to its parent interrupt controller >> +as diagram below: >> + +-----------------+ >> + | X-Gene standby | >> + | GPIO controller +--------- GPIO_0 >> ++------------+ | | ... >> +| Parent IRQ | | +--------- GPIO_8/EXT_INT_0 >> +| controller | EXT_INT_0 | | ... >> +| (GICv2) +-------------+ +--------- GPIO_[N+8]/EXT_INT_N >> +| | ... | | >> +| | EXT_INT_N | +--------- GPIO_[N+9] >> +| +-------------+ | ... >> +| | | +--------- GPIO_MAX >> ++------------+ +-----------------+ >> >> Required properties: >> - compatible: "apm,xgene-gpio-sb" for the X-Gene Standby GPIO controller >> @@ -15,10 +25,18 @@ Required properties: >> 0 = active high >> 1 = active low >> - gpio-controller: Marks the device node as a GPIO controller. >> -- interrupts: Shall contain exactly 6 interrupts. >> +- interrupts: The EXT_INT_0 parent interrupt resource must be listed first. >> +- interrupt-parent: Phandle of the parent interrupt controller. >> +- interrupt-cells: Should be two. >> + - first cell is 0-N coresponding for EXT_INT_0 to EXT_INT_N. >> + - second cell is used to specify flags. >> +- interrupt-controller: Marks the device node as an interrupt controller. >> +- apm,nr-gpios: Optional, specify number of gpios pin. >> +- apm,nr-irqs: Optional, specify number of interrupt pins. >> +- apm,irq-start: Optional, specify lowest gpio pin support interrupt. > > This is quite ambiguous. Is that relative to the GIC? Assuming this is > the case, you should then document it, specify the type of interrupt > (SPI?), and whether this is 0- or 32-based (the code seems to indicate > that is is 0-based). > Hi Marc, Let me explain by diagram below. As you can also see in diagram above, the lowest gpio pin that supports interrupt is GPIO_8 (EXT_INT_0), and hence, apm,irq-start=<8> as default. +-----------------+ | X-Gene standby | | GPIO controller +--------- GPIO_0 +------------+ | | ... | Parent IRQ | | +--------- GPIO_[apm,irq-start]/EXT_INT_0 | controller | EXT_INT_0 | | ... | (GICv2) +-----------------------------+ +--------- GPIO_[apm,nr-irqs + apm,irq-start - 1]/EXT_INT_[apm,nr-irqs - 1] | | ... | | | | EXT_INT_[apm,nr-irqs - 1] | +--------- GPIO_[apm,nr-irqs + apm,irq-start] | +-----------------------------+ | ... | | | +--------- GPIO_MAX +------------+ +-----------------+ To fix this ambiguity, I'm thinking to change it to: "apm,irq-start: Optional, specify index of first gpio pin corresponding to EXT_INT_0, default is 8." Thanks, -- Quan Nguyen
On 15/02/16 15:31, Quan Nguyen wrote: > On Mon, Feb 15, 2016 at 7:54 PM, Marc Zyngier <marc.zyngier@arm.com> wrote: >> On 15/02/16 04:57, Quan Nguyen wrote: >>> Update description for X-Gene standby GPIO controller DTS binding to >>> support GPIO line configuration as input, output or external IRQ pin. >>> >>> Signed-off-by: Y Vo <yvo@apm.com> >>> Signed-off-by: Quan Nguyen <qnguyen@apm.com> >>> --- >>> .../devicetree/bindings/gpio/gpio-xgene-sb.txt | 47 ++++++++++++++++++---- >>> 1 file changed, 40 insertions(+), 7 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt b/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt >>> index dae1300..7b8b4cb 100644 >>> --- a/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt >>> +++ b/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt >>> @@ -1,10 +1,20 @@ >>> APM X-Gene Standby GPIO controller bindings >>> >>> -This is a gpio controller in the standby domain. >>> - >>> -There are 20 GPIO pins from 0..21. There is no GPIO_DS14 or GPIO_DS15, >>> -only GPIO_DS8..GPIO_DS13 support interrupts. The IRQ mapping >>> -is currently 1-to-1 on interrupts 0x28 thru 0x2d. >>> +This is a gpio controller in the standby domain. It also supports interrupt in >>> +some particular pins which are sourced to its parent interrupt controller >>> +as diagram below: >>> + +-----------------+ >>> + | X-Gene standby | >>> + | GPIO controller +--------- GPIO_0 >>> ++------------+ | | ... >>> +| Parent IRQ | | +--------- GPIO_8/EXT_INT_0 >>> +| controller | EXT_INT_0 | | ... >>> +| (GICv2) +-------------+ +--------- GPIO_[N+8]/EXT_INT_N >>> +| | ... | | >>> +| | EXT_INT_N | +--------- GPIO_[N+9] >>> +| +-------------+ | ... >>> +| | | +--------- GPIO_MAX >>> ++------------+ +-----------------+ >>> >>> Required properties: >>> - compatible: "apm,xgene-gpio-sb" for the X-Gene Standby GPIO controller >>> @@ -15,10 +25,18 @@ Required properties: >>> 0 = active high >>> 1 = active low >>> - gpio-controller: Marks the device node as a GPIO controller. >>> -- interrupts: Shall contain exactly 6 interrupts. >>> +- interrupts: The EXT_INT_0 parent interrupt resource must be listed first. >>> +- interrupt-parent: Phandle of the parent interrupt controller. >>> +- interrupt-cells: Should be two. >>> + - first cell is 0-N coresponding for EXT_INT_0 to EXT_INT_N. >>> + - second cell is used to specify flags. >>> +- interrupt-controller: Marks the device node as an interrupt controller. >>> +- apm,nr-gpios: Optional, specify number of gpios pin. >>> +- apm,nr-irqs: Optional, specify number of interrupt pins. >>> +- apm,irq-start: Optional, specify lowest gpio pin support interrupt. >> >> This is quite ambiguous. Is that relative to the GIC? Assuming this is >> the case, you should then document it, specify the type of interrupt >> (SPI?), and whether this is 0- or 32-based (the code seems to indicate >> that is is 0-based). >> > > Hi Marc, Let me explain by diagram below. > As you can also see in diagram above, the lowest gpio pin that > supports interrupt is GPIO_8 (EXT_INT_0), > and hence, apm,irq-start=<8> as default. > > +-----------------+ > | X-Gene standby | > | GPIO controller +--------- GPIO_0 > +------------+ | | ... > | Parent IRQ | | > +--------- GPIO_[apm,irq-start]/EXT_INT_0 > | controller | EXT_INT_0 | | ... > | (GICv2) +-----------------------------+ > +--------- GPIO_[apm,nr-irqs + apm,irq-start - 1]/EXT_INT_[apm,nr-irqs > - 1] > | | ... | | > | | EXT_INT_[apm,nr-irqs - 1] | > +--------- GPIO_[apm,nr-irqs + apm,irq-start] > | +-----------------------------+ | ... > | | | +--------- GPIO_MAX > +------------+ +-----------------+ > > To fix this ambiguity, I'm thinking to change it to: > "apm,irq-start: Optional, specify index of first gpio pin > corresponding to EXT_INT_0, default is 8." I think you are missing the point I'm trying to make. There are two ways to refer to an SPI: either by its absolute number (INT32 for example) or by its relative number (SPI0). SPI0 and INT32 are the same interrupt. Just with a different base. My question is: which base are you using? By looking at the code, I think you're using the absolute version (INTx). And as your controller seems to be very GIC specific, you should add a pointer to the GIC documentation. Thanks, M.
diff --git a/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt b/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt index dae1300..7b8b4cb 100644 --- a/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt +++ b/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt @@ -1,10 +1,20 @@ APM X-Gene Standby GPIO controller bindings -This is a gpio controller in the standby domain. - -There are 20 GPIO pins from 0..21. There is no GPIO_DS14 or GPIO_DS15, -only GPIO_DS8..GPIO_DS13 support interrupts. The IRQ mapping -is currently 1-to-1 on interrupts 0x28 thru 0x2d. +This is a gpio controller in the standby domain. It also supports interrupt in +some particular pins which are sourced to its parent interrupt controller +as diagram below: + +-----------------+ + | X-Gene standby | + | GPIO controller +--------- GPIO_0 ++------------+ | | ... +| Parent IRQ | | +--------- GPIO_8/EXT_INT_0 +| controller | EXT_INT_0 | | ... +| (GICv2) +-------------+ +--------- GPIO_[N+8]/EXT_INT_N +| | ... | | +| | EXT_INT_N | +--------- GPIO_[N+9] +| +-------------+ | ... +| | | +--------- GPIO_MAX ++------------+ +-----------------+ Required properties: - compatible: "apm,xgene-gpio-sb" for the X-Gene Standby GPIO controller @@ -15,10 +25,18 @@ Required properties: 0 = active high 1 = active low - gpio-controller: Marks the device node as a GPIO controller. -- interrupts: Shall contain exactly 6 interrupts. +- interrupts: The EXT_INT_0 parent interrupt resource must be listed first. +- interrupt-parent: Phandle of the parent interrupt controller. +- interrupt-cells: Should be two. + - first cell is 0-N coresponding for EXT_INT_0 to EXT_INT_N. + - second cell is used to specify flags. +- interrupt-controller: Marks the device node as an interrupt controller. +- apm,nr-gpios: Optional, specify number of gpios pin. +- apm,nr-irqs: Optional, specify number of interrupt pins. +- apm,irq-start: Optional, specify lowest gpio pin support interrupt. Example: - sbgpio: sbgpio@17001000 { + sbgpio: gpio@17001000{ compatible = "apm,xgene-gpio-sb"; reg = <0x0 0x17001000 0x0 0x400>; #gpio-cells = <2>; @@ -29,4 +47,19 @@ Example: <0x0 0x2b 0x1>, <0x0 0x2c 0x1>, <0x0 0x2d 0x1>; + interrupt-parent = <&gic>; + #interrupt-cells = <2>; + interrupt-controller; + apm,nr-gpios = <22>; + apm,nr-irqs = <6>; + apm,irq-start = <8>; + }; + + testuser { + compatible = "example,testuser"; + /* Use the GPIO_13/EXT_INT_5 line as an active high triggered + * level interrupt + */ + interrupts = <5 4>; + interrupt-parent = <&sbgpio>; };