On Mon, Feb 15, 2016 at 11:08 PM, Marc Zyngier <marc.zyngier@xxxxxxx> wrote: > On 15/02/16 15:31, Quan Nguyen wrote: >> On Mon, Feb 15, 2016 at 7:54 PM, Marc Zyngier <marc.zyngier@xxxxxxx> 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@xxxxxxx> >>>> Signed-off-by: Quan Nguyen <qnguyen@xxxxxxx> >>>> --- >>>> .../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. > Sorry, Marc, I was misunderstood. It is relative number, EXT_INT_0 corresponding with SPI40 and so on. So, how about adding more specific description into the diagram as below ? --- a/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt +++ b/Documentation/devicetree/bindings/gpio/gpio-xgene-sb.txt @@ -3,18 +3,18 @@ APM X-Gene Standby GPIO controller bindings 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 -+------------+ +-----------------+ + +-----------------+ + | X-Gene standby | + | GPIO controller +------ GPIO_0 ++------------+ | | ... +| Parent IRQ | EXT_INT_0 | +------ GPIO_8/EXT_INT_0 +| controller | (SPI40) | | ... +| (GICv2) +--------------+ +------ GPIO_[N+8]/EXT_INT_N +| | ... | | +| | EXT_INT_N | +------ GPIO_[N+9] +| | (SPI[40 + N])| | ... +| +--------------+ +------ GPIO_MAX ++------------+ +-----------------+ Required properties: - compatible: "apm,xgene-gpio-sb" for the X-Gene Standby GPIO controller Thanks, -- Quan Nguyen -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
