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-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
