Re: [PATCH 03/12] of: Add binding document for MIPS GIC

[Andrew Bresticker]

On Tue, Sep 2, 2014 at 2:33 AM, Mark Rutland <mark.rutland@xxxxxxx> wrote:
> On Tue, Sep 02, 2014 at 01:53:18AM +0100, Andrew Bresticker wrote:
>> On Mon, Sep 1, 2014 at 4:01 AM, Mark Rutland <mark.rutland@xxxxxxx> wrote:
>> > On Fri, Aug 29, 2014 at 11:14:30PM +0100, Andrew Bresticker wrote:
>> >> The Global Interrupt Controller (GIC) present on certain MIPS systems
>> >> can be used to route external interrupts to individual VPEs and CPU
>> >> interrupt vectors.  It also supports a timer and software-generated
>> >> interrupts.
>> >>
>> >> Signed-off-by: Andrew Bresticker <abrestic@xxxxxxxxxxxx>
>> >> ---
>> >>  Documentation/devicetree/bindings/mips/gic.txt | 50 ++++++++++++++++++++++++++
>> >>  1 file changed, 50 insertions(+)
>> >>  create mode 100644 Documentation/devicetree/bindings/mips/gic.txt
>> >>
>> >> diff --git a/Documentation/devicetree/bindings/mips/gic.txt b/Documentation/devicetree/bindings/mips/gic.txt
>> >> new file mode 100644
>> >> index 0000000..725f1ef
>> >> --- /dev/null
>> >> +++ b/Documentation/devicetree/bindings/mips/gic.txt
>> >> @@ -0,0 +1,50 @@
>> >> +MIPS Global Interrupt Controller (GIC)
>> >> +
>> >> +The MIPS GIC routes external interrupts to individual VPEs and IRQ pins.
>> >> +It also supports a timer and software-generated interrupts which can be
>> >> +used as IPIs.
>> >> +
>> >> +Required properties:
>> >> +- compatible : Should be "mti,global-interrupt-controller"
>> >
>> > I couldn't find "mti" in vendor-prefixes.txt (as of v3.17-rc3). If
>> > there's not a patch to add it elsewhere, would you mind providing one
>> > with this series?
>>
>> Sure.  As James points out, "img" could also be used but I chose "mti"
>> since the CPU interrupt controller also uses "mti" and I believe the
>> GIC IP was developed before the acquisition by Imagination (though I'm
>> not sure if that actually matters).
>
> Using 'mti' sounds like the right choice to me given both of those
> points.
>
>> >> +- reg : Base address and length of the GIC registers.
>> >> +- interrupts : Core interrupts to which the GIC may route external interrupts.
>> >
>> > How many?
>>
>> Up to 6, one for each of the possible core hardware interrupts (i.e.
>> interrupt vectors 2 - 7).  Which ones are available to the GIC depend
>> on the system, for example Malta has an i8259 PIC hooked up to CPU
>> interrupt vector 2, so that vector should not be used by the GIC.
>>
>> > In any order?
>>
>> They can technically be in any order, but when in strictly
>> increasing/decreasing order they can be used along with the 3rd cell
>> (described below) to prioritize interrupts.
>
> Ok. Could you try to place that into the property description?
>
>> >> +- interrupt-controller : Identifies the node as an interrupt controller
>> >> +- #interrupt-cells : Specifies the number of cells needed to encode an
>> >> +  interrupt specifier.  Should be 3.
>> >> +  - The first cell is the GIC interrupt number.
>> >> +  - The second cell encodes the interrupt flags.
>> >> +    See <include/dt-bindings/interrupt-controller/irq.h> for a list of valid
>> >> +    flags.
>> >
>> > Are all the flags valid for this interrupt controller?
>>
>> Yes.
>
> Ok.
>
>> >> +  - The optional third cell indicates which CPU interrupt vector the GIC
>> >> +    interrupt should be routed to.  It is a 0-based index into the list of
>> >> +    GIC-to-CPU interrupts specified in the "interrupts" property described
>> >> +    above.  For example, a '2' in this cell will route the interrupt to the
>> >> +    3rd core interrupt listed in 'interrupts'.  If omitted, the interrupt will
>> >> +    be routed to the 1st core interrupt.
>> >
>> > I don't follow why this should be in the DT. Why is this necessary?
>>
>> Since the GIC can route external interrupts to any of the CPU hardware
>> interrupt vectors, it can be used to assign priorities to external
>> interrupts.  If the CPU is in vectored interrupt mode, the highest
>> numbered interrupt vector (7) has the highest priority.  An example:
>>
>> gic: interrupt-controller@1bdc0000 {
>>         ...
>>         interrupts = <3>, <4>;
>>         ...
>> };
>>
>> uart {
>>         ...
>>         interrupt-parent = <&gic>;
>>         interrupts = <24 IRQ_TYPE_LEVEL_HIGH 1>;
>>         ...
>> };
>>
>> i2c {
>>         ...
>>         interrupt-parent = <&gic>;
>>         interrupts = <33 IRQ_TYPE_LEVEL_HIGH 0>;
>>         ...
>> };
>>
>> Since the third cell for the UART is '1', it maps to CPU interrupt
>> vector 4 and thus has a higher priority than the I2C (whose third cell
>> is 0, mapping to CPU interrupt vector 3).
>>
>> Perhaps, though, this is an instance of software policy being
>> specified in device-tree.  Other options would be to a) evenly
>> distribute the GIC external interrupts across the CPU interrupt
>> vectors available to the GIC, or b) just map all GIC external
>> interrupts to a single interrupt vector.
>
> As a user I don't see why the DT author should be in charge of whether
> my UART gets higher priority than my I2C controller. That priority is
> not a fixed property of the interrupt (as the line and flags are).
>
> That said, this is a grey area. Are there any cases where this
> prioritisation is critical on existing devices?

I am not aware of any boards for which this is critical.  Malta and
SEAD-3 are development boards and do not appear to need any sort of
prioritization of GIC interrupts.  John Crispin is working on a
Mediatek board which has a GIC, but it looks like all external
interrupts are routed to a single CPU vector.  It's possible that it
could be useful on the platform I'm working on, though it's looking
more and more unlikely.

Given all that, perhaps it's better to stick with the 2-cell
(no-prioritization) binding for now and add the 3-cell binding later
if necessary.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/