[PATCH 1/3] powerpc/powernv: Supports PHB3

[PATCH 1/3] powerpc/powernv: Supports PHB3

[Gavin Shan]

The patch intends to initialize PHB3 during system boot stage. The
flag "PNV_PHB_MODEL_PHB3" is introduced to differentiate IODA2
compatible PHB3 from other types of PHBs.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 arch/powerpc/platforms/powernv/pci-ioda.c |   62 +++++++++++++++--------------
 arch/powerpc/platforms/powernv/pci.c      |    7 +++-
 arch/powerpc/platforms/powernv/pci.h      |    8 ++-
 3 files changed, 43 insertions(+), 34 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
index 8e90e89..8993242 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
@@ -852,18 +852,19 @@ static u32 pnv_ioda_bdfn_to_pe(struct pnv_phb *phb, struct pci_bus *bus,
 	return phb->ioda.pe_rmap[(bus->number << 8) | devfn];
 }
 
-void __init pnv_pci_init_ioda1_phb(struct device_node *np)
+void __init pnv_pci_init_ioda_phb(struct device_node *np, int ioda_type)
 {
 	struct pci_controller *hose;
 	static int primary = 1;
 	struct pnv_phb *phb;
 	unsigned long size, m32map_off, iomap_off, pemap_off;
 	const u64 *prop64;
+	const u32 *prop32;
 	u64 phb_id;
 	void *aux;
 	long rc;
 
-	pr_info(" Initializing IODA OPAL PHB %s\n", np->full_name);
+	pr_info(" Initializing IODA%d OPAL PHB %s\n", ioda_type, np->full_name);
 
 	prop64 = of_get_property(np, "ibm,opal-phbid", NULL);
 	if (!prop64) {
@@ -890,37 +891,34 @@ void __init pnv_pci_init_ioda1_phb(struct device_node *np)
 	hose->last_busno = 0xff;
 	hose->private_data = phb;
 	phb->opal_id = phb_id;
-	phb->type = PNV_PHB_IODA1;
+	phb->type = ioda_type;
 
 	/* Detect specific models for error handling */
 	if (of_device_is_compatible(np, "ibm,p7ioc-pciex"))
 		phb->model = PNV_PHB_MODEL_P7IOC;
+	else if (of_device_is_compatible(np, "ibm,p8-pciex"))
+		phb->model = PNV_PHB_MODEL_PHB3;
 	else
 		phb->model = PNV_PHB_MODEL_UNKNOWN;
 
-	/* We parse "ranges" now since we need to deduce the register base
-	 * from the IO base
-	 */
+	/* Parse 32-bit and IO ranges (if any) */
 	pci_process_bridge_OF_ranges(phb->hose, np, primary);
 	primary = 0;
 
-	/* Magic formula from Milton */
+	/* Get registers */
 	phb->regs = of_iomap(np, 0);
 	if (phb->regs == NULL)
 		pr_err("  Failed to map registers !\n");
 
-
-	/* XXX This is hack-a-thon. This needs to be changed so that:
-	 *  - we obtain stuff like PE# etc... from device-tree
-	 *  - we properly re-allocate M32 ourselves
-	 *    (the OFW one isn't very good)
-	 */
-
 	/* Initialize more IODA stuff */
-	phb->ioda.total_pe = 128;
+	prop32 = of_get_property(np, "ibm,opal-num-pes", NULL);
+	if (!prop32)
+		phb->ioda.total_pe = 1;
+	else
+		phb->ioda.total_pe = *prop32;
 
 	phb->ioda.m32_size = resource_size(&hose->mem_resources[0]);
-	/* OFW Has already off top 64k of M32 space (MSI space) */
+	/* FW Has already off top 64k of M32 space (MSI space) */
 	phb->ioda.m32_size += 0x10000;
 
 	phb->ioda.m32_segsize = phb->ioda.m32_size / phb->ioda.total_pe;
@@ -930,7 +928,10 @@ void __init pnv_pci_init_ioda1_phb(struct device_node *np)
 	phb->ioda.io_segsize = phb->ioda.io_size / phb->ioda.total_pe;
 	phb->ioda.io_pci_base = 0; /* XXX calculate this ? */
 
-	/* Allocate aux data & arrays */
+	/* Allocate aux data & arrays
+	 *
+	 * XXX TODO: Don't allocate io segmap on PHB3
+	 */
 	size = _ALIGN_UP(phb->ioda.total_pe / 8, sizeof(unsigned long));
 	m32map_off = size;
 	size += phb->ioda.total_pe * sizeof(phb->ioda.m32_segmap[0]);
@@ -960,7 +961,7 @@ void __init pnv_pci_init_ioda1_phb(struct device_node *np)
 	hose->mem_resources[2].start = 0;
 	hose->mem_resources[2].end = 0;
 
-#if 0
+#if 0 /* We should really do that ... */
 	rc = opal_pci_set_phb_mem_window(opal->phb_id,
 					 window_type,
 					 window_num,
@@ -974,16 +975,6 @@ void __init pnv_pci_init_ioda1_phb(struct device_node *np)
 		phb->ioda.m32_size, phb->ioda.m32_segsize,
 		phb->ioda.io_size, phb->ioda.io_segsize);
 
-	if (phb->regs)  {
-		pr_devel(" BUID     = 0x%016llx\n", in_be64(phb->regs + 0x100));
-		pr_devel(" PHB2_CR  = 0x%016llx\n", in_be64(phb->regs + 0x160));
-		pr_devel(" IO_BAR   = 0x%016llx\n", in_be64(phb->regs + 0x170));
-		pr_devel(" IO_BAMR  = 0x%016llx\n", in_be64(phb->regs + 0x178));
-		pr_devel(" IO_SAR   = 0x%016llx\n", in_be64(phb->regs + 0x180));
-		pr_devel(" M32_BAR  = 0x%016llx\n", in_be64(phb->regs + 0x190));
-		pr_devel(" M32_BAMR = 0x%016llx\n", in_be64(phb->regs + 0x198));
-		pr_devel(" M32_SAR  = 0x%016llx\n", in_be64(phb->regs + 0x1a0));
-	}
 	phb->hose->ops = &pnv_pci_ops;
 
 	/* Setup RID -> PE mapping function */
@@ -1011,7 +1002,18 @@ void __init pnv_pci_init_ioda1_phb(struct device_node *np)
 	rc = opal_pci_reset(phb_id, OPAL_PCI_IODA_TABLE_RESET, OPAL_ASSERT_RESET);
 	if (rc)
 		pr_warning("  OPAL Error %ld performing IODA table reset !\n", rc);
-	opal_pci_set_pe(phb_id, 0, 0, 7, 1, 1 , OPAL_MAP_PE);
+
+	/*
+	 * On IODA1 map everything to PE#0, on IODA2 we assume the IODA reset
+	 * has cleared the RTT which has the same effect
+	 */
+	if (ioda_type == PNV_PHB_IODA1)
+		opal_pci_set_pe(phb_id, 0, 0, 7, 1, 1 , OPAL_MAP_PE);
+}
+
+void pnv_pci_init_ioda2_phb(struct device_node *np)
+{
+	pnv_pci_init_ioda_phb(np, PNV_PHB_IODA2);
 }
 
 void __init pnv_pci_init_ioda_hub(struct device_node *np)
@@ -1034,6 +1036,6 @@ void __init pnv_pci_init_ioda_hub(struct device_node *np)
 	for_each_child_of_node(np, phbn) {
 		/* Look for IODA1 PHBs */
 		if (of_device_is_compatible(phbn, "ibm,ioda-phb"))
-			pnv_pci_init_ioda1_phb(phbn);
+			pnv_pci_init_ioda_phb(phbn, PNV_PHB_IODA1);
 	}
 }
diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c
index b8b8e0b..e088dc7 100644
--- a/arch/powerpc/platforms/powernv/pci.c
+++ b/arch/powerpc/platforms/powernv/pci.c
@@ -525,12 +525,13 @@ static void pnv_pci_dma_dev_setup(struct pci_dev *pdev)
 		pnv_pci_dma_fallback_setup(hose, pdev);
 }
 
-/* Fixup wrong class code in p7ioc root complex */
+/* Fixup wrong class code in p7ioc and p8 root complex */
 static void pnv_p7ioc_rc_quirk(struct pci_dev *dev)
 {
 	dev->class = PCI_CLASS_BRIDGE_PCI << 8;
 }
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_IBM, 0x3b9, pnv_p7ioc_rc_quirk);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_IBM, 0x2da, pnv_p7ioc_rc_quirk);
 
 static int pnv_pci_probe_mode(struct pci_bus *bus)
 {
@@ -591,6 +592,10 @@ void __init pnv_pci_init(void)
 		if (!found_ioda)
 			for_each_compatible_node(np, NULL, "ibm,p5ioc2")
 				pnv_pci_init_p5ioc2_hub(np);
+
+		/* Look for ioda2 built-in PHB3's */
+		for_each_compatible_node(np, NULL, "ibm,ioda2-phb")
+			pnv_pci_init_ioda2_phb(np);
 	}
 
 	/* Setup the linkage between OF nodes and PHBs */
diff --git a/arch/powerpc/platforms/powernv/pci.h b/arch/powerpc/platforms/powernv/pci.h
index 7cfb7c8..4ce91f5 100644
--- a/arch/powerpc/platforms/powernv/pci.h
+++ b/arch/powerpc/platforms/powernv/pci.h
@@ -4,9 +4,9 @@
 struct pci_dn;
 
 enum pnv_phb_type {
-	PNV_PHB_P5IOC2,
-	PNV_PHB_IODA1,
-	PNV_PHB_IODA2,
+	PNV_PHB_P5IOC2	= 0,
+	PNV_PHB_IODA1	= 1,
+	PNV_PHB_IODA2	= 2,
 };
 
 /* Precise PHB model for error management */
@@ -14,6 +14,7 @@ enum pnv_phb_model {
 	PNV_PHB_MODEL_UNKNOWN,
 	PNV_PHB_MODEL_P5IOC2,
 	PNV_PHB_MODEL_P7IOC,
+	PNV_PHB_MODEL_PHB3,
 };
 
 #define PNV_PCI_DIAG_BUF_SIZE	4096
@@ -150,6 +151,7 @@ extern void pnv_pci_setup_iommu_table(struct iommu_table *tbl,
 				      u64 dma_offset);
 extern void pnv_pci_init_p5ioc2_hub(struct device_node *np);
 extern void pnv_pci_init_ioda_hub(struct device_node *np);
+extern void pnv_pci_init_ioda2_phb(struct device_node *np);
 
 
 #endif /* __POWERNV_PCI_H */
-- 
1.7.5.4

[PATCH 2/3] powerpc/powernv: Configure IODA2 tables explicitly

[Gavin Shan]

The PHB3, which is compatible with IODA2, have lots of tables (RTT/
PETLV/PEST/IVT/RBA) in system memory and have corresponding BARs to
trace the system memory address. The patch configures the addresses
of variable tables explicitly through OPAL API.

Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/opal.h           |    5 +--
 arch/powerpc/platforms/powernv/pci-ioda.c |   32 +++++++++++++++++++++++++++++
 arch/powerpc/platforms/powernv/pci.h      |   25 ++++++++++++++++++++++
 3 files changed, 59 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
index a4b28f1..0af7ba0 100644
--- a/arch/powerpc/include/asm/opal.h
+++ b/arch/powerpc/include/asm/opal.h
@@ -491,9 +491,8 @@ int64_t opal_pci_map_pe_mmio_window(uint64_t phb_id, uint16_t pe_number,
 				    uint16_t window_type, uint16_t window_num,
 				    uint16_t segment_num);
 int64_t opal_pci_set_phb_table_memory(uint64_t phb_id, uint64_t rtt_addr,
-				      uint64_t ivt_addr, uint64_t ivt_len,
-				      uint64_t reject_array_addr,
-				      uint64_t peltv_addr);
+				      uint64_t peltv_addr, uint64_t pest_addr,
+				      uint64_t ivt_addr, uint64_t rba_addr);
 int64_t opal_pci_set_pe(uint64_t phb_id, uint64_t pe_number, uint64_t bus_dev_func,
 			uint8_t bus_compare, uint8_t dev_compare, uint8_t func_compare,
 			uint8_t pe_action);
diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
index 8993242..d8d5baa 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
@@ -998,6 +998,38 @@ void __init pnv_pci_init_ioda_phb(struct device_node *np, int ioda_type)
 	ppc_md.pcibios_window_alignment = pnv_pci_window_alignment;
 	pci_add_flags(PCI_REASSIGN_ALL_RSRC);
 
+	/*
+	 * Initialize variable tables for IODA2. We might share the table
+	 * size between firmware and Linux someday. For now, we have fixed
+	 * values for them
+	 */
+	if (ioda_type == PNV_PHB_IODA2) {
+		phb->ioda.tbl_rtt = alloc_bootmem_align(PNV_PHB3_RTT_TBL_SIZE,
+						PNV_PHB3_RTT_TBL_SIZE);
+		phb->ioda.tbl_peltv = alloc_bootmem_align(PNV_PHB3_PELTV_TBL_SIZE,
+						PNV_PHB3_PELTV_TBL_SIZE);
+		phb->ioda.tbl_pest = alloc_bootmem_align(PNV_PHB3_PEST_TBL_SIZE,
+						PNV_PHB3_PEST_TBL_SIZE);
+		phb->ioda.tbl_ivt = alloc_bootmem_align(PNV_PHB3_IVT_TBL_SIZE,
+						PNV_PHB3_IVT_TBL_SIZE);
+		phb->ioda.tbl_rba = alloc_bootmem_align(PNV_PHB3_RBA_TBL_SIZE,
+						PNV_PHB3_RBA_TBL_SIZE);
+		if (!phb->ioda.tbl_rtt || !phb->ioda.tbl_peltv ||
+		    !phb->ioda.tbl_pest || !phb->ioda.tbl_ivt ||
+		    !phb->ioda.tbl_rba)
+			pr_warning("  No memory for IODA2 tables\n");
+		else {
+			rc = opal_pci_set_phb_table_memory(phb_id,
+					__pa(phb->ioda.tbl_rtt),
+					__pa(phb->ioda.tbl_peltv),
+					__pa(phb->ioda.tbl_pest),
+					__pa(phb->ioda.tbl_ivt),
+					__pa(phb->ioda.tbl_rba));
+			if (rc != OPAL_SUCCESS)
+				pr_warning("  Failed to set IODA2 tables\n");
+		}
+	}
+
 	/* Reset IODA tables to a clean state */
 	rc = opal_pci_reset(phb_id, OPAL_PCI_IODA_TABLE_RESET, OPAL_ASSERT_RESET);
 	if (rc)
diff --git a/arch/powerpc/platforms/powernv/pci.h b/arch/powerpc/platforms/powernv/pci.h
index 4ce91f5..fcd5135 100644
--- a/arch/powerpc/platforms/powernv/pci.h
+++ b/arch/powerpc/platforms/powernv/pci.h
@@ -64,6 +64,24 @@ struct pnv_ioda_pe {
 	struct list_head	list;
 };
 
+/*
+ * The sizes of variable PHB3 tables. The IVT table size is variable
+ * and depends on the IVE stride (16-bytes or 128-bytes)
+ */
+#define PNV_PHB3_IVT_TBL_IVE_16B
+
+#define PNV_PHB3_RTT_TBL_SIZE		0x20000
+#define PNV_PHB3_PELTV_TBL_SIZE		0x2000
+#define PNV_PHB3_PEST_TBL_SIZE		0x1000
+#ifdef PNV_PHB3_IVT_TBL_IVE_16B
+#define PNV_PHB3_IVT_TBL_SIZE		0x8000
+#define PNV_PHB3_IVT_TBL_STRIDE		2	/* double-words */
+#else
+#define PNV_PHB3_IVT_TBL_SIZE		0x40000
+#define PNV_PHB3_IVT_TBL_STRIDE		16	/* double-words */
+#endif
+#define PNV_PHB3_RBA_TBL_SIZE		0x1000
+
 struct pnv_phb {
 	struct pci_controller	*hose;
 	enum pnv_phb_type	type;
@@ -102,6 +120,13 @@ struct pnv_phb {
 			unsigned int		io_segsize;
 			unsigned int		io_pci_base;
 
+			/* Variable tables for IODA2 */
+			void			*tbl_rtt;
+			void			*tbl_peltv;
+			void			*tbl_pest;
+			void			*tbl_ivt;
+			void			*tbl_rba;
+
 			/* PE allocation bitmap */
 			unsigned long		*pe_alloc;
 
-- 
1.7.5.4

[PATCH 3/3] powerpc/powernv: Patch MSI EOI handler on P8

[Gavin Shan]

The EOI handler of MSI/MSI-X interrupts for P8 (PHB3) need additional
steps to handle the P/Q bits in IVE before EOIing the corresponding
interrupt. The patch changes the EOI handler to cover that.

Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
---
 arch/powerpc/platforms/powernv/pci-ioda.c |   33 +++++++++++++++++++++++++++++
 arch/powerpc/platforms/powernv/pci.c      |   19 ++++++++++++++++
 arch/powerpc/platforms/powernv/pci.h      |    1 +
 arch/powerpc/sysdev/xics/icp-native.c     |   33 ++++++++++++++++++++++++++++-
 4 files changed, 85 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
index d8d5baa..de4a4a9 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
@@ -645,6 +645,37 @@ static int pnv_pci_ioda_msi_setup(struct pnv_phb *phb, struct pci_dev *dev,
 	return 0;
 }
 
+static int pnv_pci_ioda_msi_eoi(struct pnv_phb *phb, unsigned int hw_irq)
+{
+	u8 p_bit = 1, q_bit = 1;
+	long rc;
+
+	while (p_bit || q_bit) {
+		rc = opal_pci_get_xive_reissue(phb->opal_id,
+				hw_irq - phb->msi_base, &p_bit, &q_bit);
+		if (rc) {
+			pr_warning("%s: Failed to get P/Q bits of IRQ#%d "
+				   "on PHB#%d, rc=%ld\n", __func__, hw_irq,
+				   phb->hose->global_number, rc);
+			return -EIO;
+		}
+		if (!p_bit && !q_bit)
+			break;
+
+		rc = opal_pci_set_xive_reissue(phb->opal_id,
+				hw_irq - phb->msi_base, p_bit, q_bit);
+		if (rc) {
+			pr_warning("%s: Failed to clear P/Q (%01d/%01d) of "
+				   "IRQ#%d on PHB#%d, rc=%ld\n", __func__,
+				   p_bit, q_bit, hw_irq,
+				   phb->hose->global_number, rc);
+			return -EIO;
+		}
+	}
+
+	return 0;
+}
+
 static void pnv_pci_init_ioda_msis(struct pnv_phb *phb)
 {
 	unsigned int bmap_size;
@@ -667,6 +698,8 @@ static void pnv_pci_init_ioda_msis(struct pnv_phb *phb)
 		return;
 	}
 	phb->msi_setup = pnv_pci_ioda_msi_setup;
+	if (phb->type == PNV_PHB_IODA2)
+		phb->msi_eoi = pnv_pci_ioda_msi_eoi;
 	phb->msi32_support = 1;
 	pr_info("  Allocated bitmap for %d MSIs (base IRQ 0x%x)\n",
 		phb->msi_count, phb->msi_base);
diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c
index e088dc7..439dfc5 100644
--- a/arch/powerpc/platforms/powernv/pci.c
+++ b/arch/powerpc/platforms/powernv/pci.c
@@ -148,6 +148,25 @@ static void pnv_teardown_msi_irqs(struct pci_dev *pdev)
 		irq_dispose_mapping(entry->irq);
 	}
 }
+
+int pnv_pci_msi_eoi(unsigned int hw_irq)
+{
+	struct pci_controller *hose, *tmp;
+	struct pnv_phb *phb = NULL;
+
+	list_for_each_entry_safe(hose, tmp, &hose_list, list_node) {
+		phb = hose->private_data;
+		if (hw_irq >= phb->msi_base &&
+		    hw_irq < phb->msi_base + phb->msi_count) {
+			if (!phb->msi_eoi)
+				return -EEXIST;
+			return phb->msi_eoi(phb, hw_irq);
+		}
+	}
+
+	/* For LSI interrupts, we needn't do it */
+	return 0;
+}
 #endif /* CONFIG_PCI_MSI */
 
 static void pnv_pci_dump_p7ioc_diag_data(struct pnv_phb *phb)
diff --git a/arch/powerpc/platforms/powernv/pci.h b/arch/powerpc/platforms/powernv/pci.h
index fcd5135..4ae015b 100644
--- a/arch/powerpc/platforms/powernv/pci.h
+++ b/arch/powerpc/platforms/powernv/pci.h
@@ -101,6 +101,7 @@ struct pnv_phb {
 	int (*msi_setup)(struct pnv_phb *phb, struct pci_dev *dev,
 			 unsigned int hwirq, unsigned int is_64,
 			 struct msi_msg *msg);
+	int (*msi_eoi)(struct pnv_phb *phb, unsigned int hw_irq);
 	void (*dma_dev_setup)(struct pnv_phb *phb, struct pci_dev *pdev);
 	void (*fixup_phb)(struct pci_controller *hose);
 	u32 (*bdfn_to_pe)(struct pnv_phb *phb, struct pci_bus *bus, u32 devfn);
diff --git a/arch/powerpc/sysdev/xics/icp-native.c b/arch/powerpc/sysdev/xics/icp-native.c
index 48861d3..289355e 100644
--- a/arch/powerpc/sysdev/xics/icp-native.c
+++ b/arch/powerpc/sysdev/xics/icp-native.c
@@ -27,6 +27,10 @@
 #include <asm/xics.h>
 #include <asm/kvm_ppc.h>
 
+#if defined(CONFIG_PPC_POWERNV) && defined(CONFIG_PCI_MSI)
+extern int pnv_pci_msi_eoi(unsigned int hw_irq);
+#endif
+
 struct icp_ipl {
 	union {
 		u32 word;
@@ -89,6 +93,24 @@ static void icp_native_eoi(struct irq_data *d)
 	icp_native_set_xirr((xics_pop_cppr() << 24) | hw_irq);
 }
 
+static void icp_p8_native_eoi(struct irq_data *d)
+{
+	unsigned int hw_irq = (unsigned int)irqd_to_hwirq(d);
+	int ret;
+
+	/* Let firmware handle P/Q bits */
+#if defined(CONFIG_PPC_POWERNV) && defined(CONFIG_PCI_MSI)
+	if (hw_irq != XICS_IPI) {
+		ret = pnv_pci_msi_eoi(hw_irq);
+		WARN_ON_ONCE(ret);
+	}
+#endif
+
+	/* EOI on ICP */
+	iosync();
+	icp_native_set_xirr((xics_pop_cppr() << 24) | hw_irq);
+}
+
 static void icp_native_teardown_cpu(void)
 {
 	int cpu = smp_processor_id();
@@ -264,7 +286,7 @@ static int __init icp_native_init_one_node(struct device_node *np,
 	return 0;
 }
 
-static const struct icp_ops icp_native_ops = {
+static struct icp_ops icp_native_ops = {
 	.get_irq	= icp_native_get_irq,
 	.eoi		= icp_native_eoi,
 	.set_priority	= icp_native_set_cpu_priority,
@@ -296,6 +318,15 @@ int __init icp_native_init(void)
 	if (found == 0)
 		return -ENODEV;
 
+	/* Change the EOI handler for P8 */
+#if defined(CONFIG_PPC_POWERNV) && defined(CONFIG_PCI_MSI)
+	np = of_find_compatible_node(NULL, NULL, "ibm,power8-xicp");
+	if (np) {
+		icp_native_ops.eoi = icp_p8_native_eoi;
+		of_node_put(np);
+	}
+#endif
+
 	icp_ops = &icp_native_ops;
 
 	return 0;
-- 
1.7.5.4

Re: [PATCH 3/3] powerpc/powernv: Patch MSI EOI handler on P8

[Michael Ellerman]

On Fri, Apr 19, 2013 at 05:32:45PM +0800, Gavin Shan wrote:
> The EOI handler of MSI/MSI-X interrupts for P8 (PHB3) need additional
> steps to handle the P/Q bits in IVE before EOIing the corresponding
> interrupt. The patch changes the EOI handler to cover that.

> diff --git a/arch/powerpc/sysdev/xics/icp-native.c b/arch/powerpc/sysdev/xics/icp-native.c
> index 48861d3..289355e 100644
> --- a/arch/powerpc/sysdev/xics/icp-native.c
> +++ b/arch/powerpc/sysdev/xics/icp-native.c
> @@ -27,6 +27,10 @@
>  #include <asm/xics.h>
>  #include <asm/kvm_ppc.h>
>  
> +#if defined(CONFIG_PPC_POWERNV) && defined(CONFIG_PCI_MSI)
> +extern int pnv_pci_msi_eoi(unsigned int hw_irq);
> +#endif

You don't need to #ifdef the extern. But it should be in a header, not
here.

> @@ -89,6 +93,24 @@ static void icp_native_eoi(struct irq_data *d)
>  	icp_native_set_xirr((xics_pop_cppr() << 24) | hw_irq);
>  }
>  
> +static void icp_p8_native_eoi(struct irq_data *d)
> +{
> +	unsigned int hw_irq = (unsigned int)irqd_to_hwirq(d);
> +	int ret;
> +
> +	/* Let firmware handle P/Q bits */
> +#if defined(CONFIG_PPC_POWERNV) && defined(CONFIG_PCI_MSI)
> +	if (hw_irq != XICS_IPI) {
> +		ret = pnv_pci_msi_eoi(hw_irq);
> +		WARN_ON_ONCE(ret);
> +	}
> +#endif

Why the ifdef in here? You only ever hook this function up if those are
true, so why do you need to check them again?

> @@ -296,6 +318,15 @@ int __init icp_native_init(void)
>  	if (found == 0)
>  		return -ENODEV;
>  
> +	/* Change the EOI handler for P8 */
> +#if defined(CONFIG_PPC_POWERNV) && defined(CONFIG_PCI_MSI)

This would be neater if you created CONFIG_POWERNV_MSI, like we have
CONFIG_PSERIES_MSI.

cheers

Re: [PATCH 3/3] powerpc/powernv: Patch MSI EOI handler on P8

[Gavin Shan]

On Mon, Apr 22, 2013 at 09:34:36AM +1000, Michael Ellerman wrote:
>On Fri, Apr 19, 2013 at 05:32:45PM +0800, Gavin Shan wrote:
>> The EOI handler of MSI/MSI-X interrupts for P8 (PHB3) need additional
>> steps to handle the P/Q bits in IVE before EOIing the corresponding
>> interrupt. The patch changes the EOI handler to cover that.

Thanks for your time to review it, Michael. By the way, I think I need
rebase the patch since the patch fb1b55d654a7038ca6337fbf55839a308c9bc1a7
("Using bitmap to manage MSI") has been merged to linux-next.

>> diff --git a/arch/powerpc/sysdev/xics/icp-native.c b/arch/powerpc/sysdev/xics/icp-native.c
>> index 48861d3..289355e 100644
>> --- a/arch/powerpc/sysdev/xics/icp-native.c
>> +++ b/arch/powerpc/sysdev/xics/icp-native.c
>> @@ -27,6 +27,10 @@
>>  #include <asm/xics.h>
>>  #include <asm/kvm_ppc.h>
>>  
>> +#if defined(CONFIG_PPC_POWERNV) && defined(CONFIG_PCI_MSI)
>> +extern int pnv_pci_msi_eoi(unsigned int hw_irq);
>> +#endif
>
>You don't need to #ifdef the extern. But it should be in a header, not
>here.
>

Ok. I'll put it into asm/xics.h, but I want to confirm we needn't
#ifdef when moving it to asm/xics.h?

>> @@ -89,6 +93,24 @@ static void icp_native_eoi(struct irq_data *d)
>>  	icp_native_set_xirr((xics_pop_cppr() << 24) | hw_irq);
>>  }
>>  
>> +static void icp_p8_native_eoi(struct irq_data *d)
>> +{
>> +	unsigned int hw_irq = (unsigned int)irqd_to_hwirq(d);
>> +	int ret;
>> +
>> +	/* Let firmware handle P/Q bits */
>> +#if defined(CONFIG_PPC_POWERNV) && defined(CONFIG_PCI_MSI)
>> +	if (hw_irq != XICS_IPI) {
>> +		ret = pnv_pci_msi_eoi(hw_irq);
>> +		WARN_ON_ONCE(ret);
>> +	}
>> +#endif
>
>Why the ifdef in here? You only ever hook this function up if those are
>true, so why do you need to check them again?
>

Right. I will remove #ifdef here in next version.

>> @@ -296,6 +318,15 @@ int __init icp_native_init(void)
>>  	if (found == 0)
>>  		return -ENODEV;
>>  
>> +	/* Change the EOI handler for P8 */
>> +#if defined(CONFIG_PPC_POWERNV) && defined(CONFIG_PCI_MSI)
>
>This would be neater if you created CONFIG_POWERNV_MSI, like we have
>CONFIG_PSERIES_MSI.
>

Sure. I'll introduce CONFIG_PSERIES_MSI in next version.

Thanks,
Gavin

Re: [PATCH 3/3] powerpc/powernv: Patch MSI EOI handler on P8

[Michael Ellerman]

On Mon, Apr 22, 2013 at 09:45:33AM +0800, Gavin Shan wrote:
> On Mon, Apr 22, 2013 at 09:34:36AM +1000, Michael Ellerman wrote:
> >On Fri, Apr 19, 2013 at 05:32:45PM +0800, Gavin Shan wrote:
> >> The EOI handler of MSI/MSI-X interrupts for P8 (PHB3) need additional
> >> steps to handle the P/Q bits in IVE before EOIing the corresponding
> >> interrupt. The patch changes the EOI handler to cover that.
> 
> Thanks for your time to review it, Michael. By the way, I think I need
> rebase the patch since the patch fb1b55d654a7038ca6337fbf55839a308c9bc1a7
> ("Using bitmap to manage MSI") has been merged to linux-next.
> 
> >> diff --git a/arch/powerpc/sysdev/xics/icp-native.c b/arch/powerpc/sysdev/xics/icp-native.c
> >> index 48861d3..289355e 100644
> >> --- a/arch/powerpc/sysdev/xics/icp-native.c
> >> +++ b/arch/powerpc/sysdev/xics/icp-native.c
> >> @@ -27,6 +27,10 @@
> >>  #include <asm/xics.h>
> >>  #include <asm/kvm_ppc.h>
> >>  
> >> +#if defined(CONFIG_PPC_POWERNV) && defined(CONFIG_PCI_MSI)
> >> +extern int pnv_pci_msi_eoi(unsigned int hw_irq);
> >> +#endif
> >
> >You don't need to #ifdef the extern. But it should be in a header, not
> >here.
> >
> 
> Ok. I'll put it into asm/xics.h, but I want to confirm we needn't
> #ifdef when moving it to asm/xics.h?

No you don't need it #ifdef'd. It's just extra noise in the file, and
doesn't really add anything IMHO.

cheers

Re: [PATCH 3/3] powerpc/powernv: Patch MSI EOI handler on P8

[Gavin Shan]

On Mon, Apr 22, 2013 at 12:56:37PM +1000, Michael Ellerman wrote:
>On Mon, Apr 22, 2013 at 09:45:33AM +0800, Gavin Shan wrote:
>> On Mon, Apr 22, 2013 at 09:34:36AM +1000, Michael Ellerman wrote:
>> >On Fri, Apr 19, 2013 at 05:32:45PM +0800, Gavin Shan wrote:
>> >> The EOI handler of MSI/MSI-X interrupts for P8 (PHB3) need additional
>> >> steps to handle the P/Q bits in IVE before EOIing the corresponding
>> >> interrupt. The patch changes the EOI handler to cover that.
>> 
>> Thanks for your time to review it, Michael. By the way, I think I need
>> rebase the patch since the patch fb1b55d654a7038ca6337fbf55839a308c9bc1a7
>> ("Using bitmap to manage MSI") has been merged to linux-next.
>> 
>> >> diff --git a/arch/powerpc/sysdev/xics/icp-native.c b/arch/powerpc/sysdev/xics/icp-native.c
>> >> index 48861d3..289355e 100644
>> >> --- a/arch/powerpc/sysdev/xics/icp-native.c
>> >> +++ b/arch/powerpc/sysdev/xics/icp-native.c
>> >> @@ -27,6 +27,10 @@
>> >>  #include <asm/xics.h>
>> >>  #include <asm/kvm_ppc.h>
>> >>  
>> >> +#if defined(CONFIG_PPC_POWERNV) && defined(CONFIG_PCI_MSI)
>> >> +extern int pnv_pci_msi_eoi(unsigned int hw_irq);
>> >> +#endif
>> >
>> >You don't need to #ifdef the extern. But it should be in a header, not
>> >here.
>> >
>> 
>> Ok. I'll put it into asm/xics.h, but I want to confirm we needn't
>> #ifdef when moving it to asm/xics.h?
>
>No you don't need it #ifdef'd. It's just extra noise in the file, and
>doesn't really add anything IMHO.
>

Michael, I'm a bit confused about your point. asm/xics.h is shared between
PowerNV and pSeries platform, and pnv_pci_msi_eoi() is only implemented on
PowerNV platform, so the code should look like this (with newly introduced
option - CONFIG_POWERNV_MSI)

#ifdef CONFIG_POWERNV_MSI
extern int pnv_pci_msi_eoi(unsigned int hw_irq);
#endif

Thanks,
Gavin

Re: [PATCH 3/3] powerpc/powernv: Patch MSI EOI handler on P8

[Michael Ellerman]

On Mon, Apr 22, 2013 at 07:06:17PM +0800, Gavin Shan wrote:
> On Mon, Apr 22, 2013 at 12:56:37PM +1000, Michael Ellerman wrote:
> >On Mon, Apr 22, 2013 at 09:45:33AM +0800, Gavin Shan wrote:
> >> On Mon, Apr 22, 2013 at 09:34:36AM +1000, Michael Ellerman wrote:
> >> >On Fri, Apr 19, 2013 at 05:32:45PM +0800, Gavin Shan wrote:
> >> >> The EOI handler of MSI/MSI-X interrupts for P8 (PHB3) need additional
> >> >> steps to handle the P/Q bits in IVE before EOIing the corresponding
> >> >> interrupt. The patch changes the EOI handler to cover that.
> >> 
> >> Thanks for your time to review it, Michael. By the way, I think I need
> >> rebase the patch since the patch fb1b55d654a7038ca6337fbf55839a308c9bc1a7
> >> ("Using bitmap to manage MSI") has been merged to linux-next.
> >> 
> >> >> diff --git a/arch/powerpc/sysdev/xics/icp-native.c b/arch/powerpc/sysdev/xics/icp-native.c
> >> >> index 48861d3..289355e 100644
> >> >> --- a/arch/powerpc/sysdev/xics/icp-native.c
> >> >> +++ b/arch/powerpc/sysdev/xics/icp-native.c
> >> >> @@ -27,6 +27,10 @@
> >> >>  #include <asm/xics.h>
> >> >>  #include <asm/kvm_ppc.h>
> >> >>  
> >> >> +#if defined(CONFIG_PPC_POWERNV) && defined(CONFIG_PCI_MSI)
> >> >> +extern int pnv_pci_msi_eoi(unsigned int hw_irq);
> >> >> +#endif
> >> >
> >> >You don't need to #ifdef the extern. But it should be in a header, not
> >> >here.
> >> >
> >> 
> >> Ok. I'll put it into asm/xics.h, but I want to confirm we needn't
> >> #ifdef when moving it to asm/xics.h?
> >
> >No you don't need it #ifdef'd. It's just extra noise in the file, and
> >doesn't really add anything IMHO.
> >
> 
> Michael, I'm a bit confused about your point. asm/xics.h is shared between
> PowerNV and pSeries platform, and pnv_pci_msi_eoi() is only implemented on
> PowerNV platform, so the code should look like this (with newly introduced
> option - CONFIG_POWERNV_MSI)
> 
> #ifdef CONFIG_POWERNV_MSI
> extern int pnv_pci_msi_eoi(unsigned int hw_irq);
> #endif

You can do that. But there's not much value added by adding an
#ifdef around the extern.

Assuming the body of pnv_pci_msi_eoi() is only available when
CONFIG_POWERNV_MSI is defined (which is the whole point), imagine there
is code in platforms/pseries which accidentally calls it.

If we have the extern protected by an ifdef we will get a warning that
we are calling an undeclared function, eg something like:

  pseries.c:30:2: warning: implicit declaration of function ‘pnv_pci_msi_eoi’ [-Wimplicit-function-declaration]

But more importantly we will not be able to link the kernel, because the
body of pnv_pci_msi_eoi() is missing (because CONFIG_POWERNV_MSI=n).

If we have the extern visible in the header, ie. not inside #ifdef, then
we will not see the warning because the compiler can see the
declaration.

But even so the kernel will still not link.

So my point is that having the #ifdef around the extern just gives you
an extra warning, which is not all that useful because you are going to
notice anyway as soon as the kernel fails to link.

Anyway it's a minor point so don't worry about it too much :)

cheers

Re: [PATCH 3/3] powerpc/powernv: Patch MSI EOI handler on P8

[Gavin Shan]

On Tue, Apr 23, 2013 at 09:34:16AM +1000, Michael Ellerman wrote:
>On Mon, Apr 22, 2013 at 07:06:17PM +0800, Gavin Shan wrote:
>> On Mon, Apr 22, 2013 at 12:56:37PM +1000, Michael Ellerman wrote:
>> >On Mon, Apr 22, 2013 at 09:45:33AM +0800, Gavin Shan wrote:
>> >> On Mon, Apr 22, 2013 at 09:34:36AM +1000, Michael Ellerman wrote:
>> >> >On Fri, Apr 19, 2013 at 05:32:45PM +0800, Gavin Shan wrote:

.../...

>> >> >> diff --git a/arch/powerpc/sysdev/xics/icp-native.c b/arch/powerp=
c/sysdev/xics/icp-native.c
>> >> >> index 48861d3..289355e 100644
>> >> >> --- a/arch/powerpc/sysdev/xics/icp-native.c
>> >> >> +++ b/arch/powerpc/sysdev/xics/icp-native.c
>> >> >> @@ -27,6 +27,10 @@
>> >> >>  #include <asm/xics.h>
>> >> >>  #include <asm/kvm_ppc.h>
>> >> >> =20
>> >> >> +#if defined(CONFIG_PPC_POWERNV) && defined(CONFIG_PCI_MSI)
>> >> >> +extern int pnv_pci_msi_eoi(unsigned int hw_irq);
>> >> >> +#endif
>> >> >
>> >> >You don't need to #ifdef the extern. But it should be in a header,=
 not
>> >> >here.
>> >> >
>> >>=20
>> >> Ok. I'll put it into asm/xics.h, but I want to confirm we needn't
>> >> #ifdef when moving it to asm/xics.h?
>> >
>> >No you don't need it #ifdef'd. It's just extra noise in the file, and
>> >doesn't really add anything IMHO.
>> >
>>=20
>> Michael, I'm a bit confused about your point. asm/xics.h is shared bet=
ween
>> PowerNV and pSeries platform, and pnv_pci_msi_eoi() is only implemente=
d on
>> PowerNV platform, so the code should look like this (with newly introd=
uced
>> option - CONFIG_POWERNV_MSI)
>>=20
>> #ifdef CONFIG_POWERNV_MSI
>> extern int pnv_pci_msi_eoi(unsigned int hw_irq);
>> #endif
>
>You can do that. But there's not much value added by adding an
>#ifdef around the extern.
>
>Assuming the body of pnv_pci_msi_eoi() is only available when
>CONFIG_POWERNV_MSI is defined (which is the whole point), imagine there
>is code in platforms/pseries which accidentally calls it.
>
>If we have the extern protected by an ifdef we will get a warning that
>we are calling an undeclared function, eg something like:
>
>  pseries.c:30:2: warning: implicit declaration of function =E2=80=98pnv=
_pci_msi_eoi=E2=80=99 [-Wimplicit-function-declaration]
>
>But more importantly we will not be able to link the kernel, because the
>body of pnv_pci_msi_eoi() is missing (because CONFIG_POWERNV_MSI=3Dn).
>
>If we have the extern visible in the header, ie. not inside #ifdef, then
>we will not see the warning because the compiler can see the
>declaration.
>
>But even so the kernel will still not link.
>
>So my point is that having the #ifdef around the extern just gives you
>an extra warning, which is not all that useful because you are going to
>notice anyway as soon as the kernel fails to link.
>
>Anyway it's a minor point so don't worry about it too much :)
>

Thanks for your time to explain it with details, Michael. I will
remove that "#ifdef" ;-)

Thanks,
Gavin