Re: [PATCH 3/3 V2] irqchip: gic: Add supports for ARM GICv2m MSI(-X)

[Suravee Suthikulanit]

Thanks again for the review. Please see my comments below.

On 7/2/2014 11:39 AM, Mark Rutland wrote:
> On Wed, Jul 02, 2014 at 04:22:23PM +0100, suravee.suthikulpanit@xxxxxxx wrote:
> > From: Suravee Suthikulpanit <Suravee.Suthikulpanit@xxxxxxx>
> > diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt
> > index 5573c08..9e46f7f 100644
> > --- a/Documentation/devicetree/bindings/arm/gic.txt
> > +++ b/Documentation/devicetree/bindings/arm/gic.txt
> > @@ -12,11 +12,13 @@ Main node required properties:
> >
> >   - compatible : should be one of:
> >          "arm,gic-400"
> > +       "arm,gic-400-plus"
>
> I am not keen on this name.

Ok, I'll change it. Any suggestion on name?  I'm not sure what is the 
"official" product name. I've seen this in some slides from ARM. What 
about "gic-400-v2m"??.

> >          "arm,cortex-a15-gic"
> >          "arm,cortex-a9-gic"
> >          "arm,cortex-a7-gic"
> >          "arm,arm11mp-gic"
> >   - interrupt-controller : Identifies the node as an interrupt controller
> > +
> >   - #interrupt-cells : Specifies the number of cells needed to encode an
> >     interrupt source.  The type shall be a <u32> and the value shall be 3.
>
> Random (inconsistent) whitespace change?
It looks to me like there should have been a space here to keep the 
consistent look, and make it easy to read

> > @@ -37,9 +39,16 @@ Main node required properties:
> >          the 8 possible cpus attached to the GIC.  A bit set to '1' indicated
> >          the interrupt is wired to that CPU.  Only valid for PPI interrupts.
> >
> > -- reg : Specifies base physical address(s) and size of the GIC registers. The
> > -  first region is the GIC distributor register base and size. The 2nd region is
> > -  the GIC cpu interface register base and size.
> > +- reg : Specifies base physical address(s) and size of the GIC register frames.
> > +
> > +  Region | Description
> > +  Index  |
> > +  -------------------------------------------------------------------
> > +     0   | GIC distributor register base and size
> > +     1   | GIC cpu interface register base and size
> > +     2   | VGIC interface control register base and size (Optional)
> > +     3   | VGIC CPU interface register base and size (Optional)
> > +     4   | GICv2m MSI interface register base and size (Optional)
>
> As far as I am aware, the MSI interface is completely orthogonal to
> having a GICH and GICV.

Agree. I'm not doing anything with it. I'm just listing them here since 
they are also mentioned in the gic.txt

> We should figure out how we expect to handle that (zero-sized reg
> entries? reg-names?).

I'm not sure how VGIC stuff handles reg/size = 0.

> >   Optional
> >   - interrupts   : Interrupt source of the parent interrupt controller on
> > @@ -55,6 +64,10 @@ Optional
> >                    by a crossbar/multiplexer preceding the GIC. The GIC irq
> >                    input line is assigned dynamically when the corresponding
> >                    peripheral's crossbar line is mapped.
> > +
> > +- msi-controller : Identifies the node as an MSI controller.  This requires
> > +  the register region index 4.
>
> That last clarifying comment is more confusing than helpful.

If you are referring to the table, I added that since it was easier to 
see than scanning the text.

> [...]
>
> > +#define GIC_V2M_MIN_SPI                        32
> > +#define GIC_V2M_MAX_SPI                        1024
>
> GIC interrupt IDs 1020 and above are reserved, no?
>
> Surely the max ID an SPI can take is 1019?

Right, thanks for catching. But the spec says that the SPI ID value must 
be in the range of 32 to 1020. I guess it was a bit unclear, but 
definitely not 1024 :)

> > +#define GIC_OF_MSIV2M_RANGE_INDEX      4
> > +
> > +/**
> > + * alloc_msi_irq - Allocate MSIs from avaialbe MSI bitmap.
> > + * @data: Pointer to v2m_data
> > + * @nvec: Number of interrupts to allocate
> > + * @irq: Pointer to the allocated irq
> > + *
> > + * Allocates interrupts only if the contiguous range of MSIs
> > + * with specified nvec are available. Otherwise return the number
> > + * of available interrupts. If none are available, then returns -ENOENT.
> > + */
>
> This function is overly complicated, and pointlessly so.
>
> It doesn't achieve anything useful as it returns the size of the _last_
> contiguous block rather than the _largest_ contiguous block, and the
> only caller (gicv2m_setup_msi_irq) doesn't even care.
>
> Simplify this to just return an error code if allocation is impossible.

Actually, I made another mistake in the gicv2m_setup_msi_irq when 
returning from the alloc_msi_irq().

My understanding is the arch_setup_msi_irqs() is supposed to return the 
number of available vectors if the requested amount could not be 
allocated. I notice that the current "drivers/pci/msi.c: 
arch_setup_msi_irqs()" does not do so, which is okay.

However, We are also working on adding support for multi-MSI support 
since some of the devices we have are using it, which means we will need 
to provide a different version of the "arch_setup_msi_irqs()" as the 
current one does not allow (PCI_CAP_ID_MSI && nvec > 1).

Therefore, I implemented the "alloc_msi_irq" this way to account for 
future changes.

Thanks,

Suravee

--
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/