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@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"??. 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. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
