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

[Mark Rutland <mark.rutland@arm.com>]

On Wed, Jul 02, 2014 at 09:04:09PM +0100, Suravee Suthikulanit wrote:
> 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@amd.com wrote:
> >> From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> >> 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"??.

I'll query this internally.

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

As far as I can see it's inconsistent because you didn't add a newline
before the "interrupt-controller" line immediately before.

I'm not against adding spacing between the properties, so long as it is
consistent.

> >> @@ -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.

Neither am I.

However, what I said was that we should figure out how we _expect_ to
handle that case. If we have to make changes to handle it, that's fine
given we already have to make changes to support GICv2m.

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

I was referring to "This requires the register region index 4".

How about:

- msi-controller : Identifies the node as an MSI controller. Requried
  for GICv2m.

> 
> > [...]
> >
> >> +#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 :)

Which spec?

In the GICv2 spec I see:

* "Interrupt numbers ID32-ID1019 are used for SPIs." in 
  2.2.1 Interrupt IDs.

* "The GIC architecture reserves interrupt ID numbers 1020-1023 for
  special purposes." in 3.2.5 Special interrupt numbers.

> 
> >> +#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().

Ok.

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

Per my comments, I still think the function is broken given it returns
the size of the last contiguous block of available interrupts.

If we don't yet have support for multi-MSI, just return an error code
for now. The code isn't helpful and that path isn't tested.

We can fix up the driver when we add multi-MSI support.

Thanks,
Mark.