Re: [PATCH v4 02/21] powerpc/powernv: Enable M64 on P7IOC

[Gavin Shan <gwshan@linux.vnet.ibm.com>]

On Sat, May 09, 2015 at 10:18:42AM +1000, Alexey Kardashevskiy wrote:
>On 05/01/2015 04:02 PM, Gavin Shan wrote:
>>The patch enables M64 window on P7IOC, which has been enabled on
>>PHB3. Comparing to PHB3, there are 16 M64 BARs and each of them
>>are divided to 8 segments.
>
>"compared to something" means you will tell about PHB3 too :)
>

Ok. I'll add something about PHB3 in next revision.

>Do I understand correctly that IODA==IODA1==P7IOC  and P7IOC != IODA2? The
>code does not use "PHB3" or "P7IOC" acronym so it is a bit confusing.
>
>

Your understanding is correct.

>>So each PHB can support 128 M64 segments.
>>Also, P7IOC has M64DT, which helps mapping one particular M64
>>segment# to arbitrary PE#. However, we just provide 128 M64 (16 BARs)
>>segments and fixed mapping between PE# and M64 segment# in order
>>to keep same logic to support M64 for PHB3 and P7IOC. In turn, we
>>just need different phb->init_m64() hooks for P7IOC and PHB3.
>>
>>Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>>---
>>  arch/powerpc/platforms/powernv/pci-ioda.c | 115 ++++++++++++++++++++++++++----
>>  1 file changed, 103 insertions(+), 12 deletions(-)
>>
>>diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
>>index f8bc950..646962f 100644
>>--- a/arch/powerpc/platforms/powernv/pci-ioda.c
>>+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
>>@@ -165,6 +165,67 @@ static void pnv_ioda_free_pe(struct pnv_phb *phb, int pe)
>>  	clear_bit(pe, phb->ioda.pe_alloc);
>>  }
>>
>>+static int pnv_ioda1_init_m64(struct pnv_phb *phb)
>>+{
>>+	struct resource *r;
>>+	int seg;
>>+	s64 rc;
>
>Here @rc is of the "s64" type.
>

Ok. I'll have "int64_t rc" as you pointed in the following replies.

>>+
>>+	/* Each PHB supports 16 separate M64 BARs, each of which are
>>+	 * divided into 8 segments. So there are number of M64 segments
>>+	 * as total PE#, which is 128.
>>+	 */
>
>"there are as many M64 segments as a maximum number of PEs which is 128"?
>

Thanks, your description is obviously more clear. I will have it in
next revision.

>>+	for (seg = 0; seg < phb->ioda.total_pe; seg += 8) {
>>+		unsigned long base;
>>+
>>+		base = phb->ioda.m64_base + seg * phb->ioda.m64_segsize;
>>+		rc = opal_pci_set_phb_mem_window(phb->opal_id,
>>+						 OPAL_M64_WINDOW_TYPE,
>>+						 seg / 8,
>>+						 base,
>>+						 0, /* unused */
>>+						 8 * phb->ioda.m64_segsize);
>>+		if (rc != OPAL_SUCCESS) {
>>+			pr_warn("  Failure %lld configuring M64 BAR#%d on PHB#%d\n",
>>+				rc, seg / 8, phb->hose->global_number);
>>+			goto fail;
>>+		}
>>+
>>+		rc = opal_pci_phb_mmio_enable(phb->opal_id,
>>+					      OPAL_M64_WINDOW_TYPE,
>>+					      seg / 8,
>>+					      OPAL_ENABLE_M64_SPLIT);
>>+		if (rc != OPAL_SUCCESS) {
>>+			pr_warn("  Failure %lld enabling M64 BAR#%d on PHB#%d\n",
>>+				rc, seg / 8, phb->hose->global_number);
>>+			goto fail;
>>+		}
>>+	}
>>+
>>+	/* Strip of the segment used by the reserved PE, which
>>+	 * is expected to be 0 or last supported PE#
>>+	 */
>>+	r = &phb->hose->mem_resources[1];
>
>mem_resources[0] is IO, mem_resources[1] is MMIO, mem_resources[2] is for
>what? Would be nice to have this commented somewhere.
>

The fixed layout is determined by skiboot firmware. mem_resource[2] is for
64-bits prefetchable MMIO. I'll see if I can put some comments about them
somewhere in next revision.

>>+	if (phb->ioda.reserved_pe == 0)
>>+		r->start += phb->ioda.m64_segsize;
>>+	else if (phb->ioda.reserved_pe == (phb->ioda.total_pe - 1))
>>+		r->end -= phb->ioda.m64_segsize;
>>+	else
>>+		pr_warn("  Cannot strip M64 segment for reserved PE#%d\n",
>>+			phb->ioda.reserved_pe);
>>+
>>+	return 0;
>>+
>>+fail:
>>+	for ( ; seg >= 0; seg -= 8)
>>+		opal_pci_phb_mmio_enable(phb->opal_id,
>>+					 OPAL_M64_WINDOW_TYPE,
>>+					 seg / 8,
>>+					 OPAL_DISABLE_M64);
>
>Out of curiosity - is not there a counterpart for
>opal_pci_set_phb_mem_window() for cleanup?
>
>

No.

>>+
>>+	return -EIO;
>>+}
>>+
>>  /* The default M64 BAR is shared by all PEs */
>>  static int pnv_ioda2_init_m64(struct pnv_phb *phb)
>>  {
>>@@ -222,7 +283,7 @@ fail:
>>  	return -EIO;
>>  }
>>
>>-static void pnv_ioda2_reserve_m64_pe(struct pnv_phb *phb)
>>+static void pnv_ioda_reserve_m64_pe(struct pnv_phb *phb)
>>  {
>>  	resource_size_t sgsz = phb->ioda.m64_segsize;
>>  	struct pci_dev *pdev;
>>@@ -248,8 +309,8 @@ static void pnv_ioda2_reserve_m64_pe(struct pnv_phb *phb)
>>  	}
>>  }
>>
>>-static int pnv_ioda2_pick_m64_pe(struct pnv_phb *phb,
>>-				 struct pci_bus *bus, int all)
>>+static int pnv_ioda_pick_m64_pe(struct pnv_phb *phb,
>>+				struct pci_bus *bus, int all)
>>  {
>>  	resource_size_t segsz = phb->ioda.m64_segsize;
>>  	struct pci_dev *pdev;
>>@@ -346,6 +407,28 @@ done:
>>  			pe->master = master_pe;
>>  			list_add_tail(&pe->list, &master_pe->slaves);
>>  		}
>>+
>>+		/* P7IOC supports M64DT, which helps mapping M64 segment
>>+		 * to one particular PE#. Unfortunately, PHB3 has fixed
>
>Why is it "Unfortunately"? This is just the way it is :)
>

It's true that PHB3 is designed without M64DT while P7IOC has. I think
it's a nice thing providing more flexibility: Arbitrary M64 segment
can be mapped to any one PE# with its help. So I said "unfortunately" :-)

>>+		 * mapping between M64 segment and PE#. In order for same
>>+		 * logic for P7IOC and PHB3, we enforce fixed mapping
>>+		 * between M64 segment and PE# on P7IOC.
>>+		 */
>>+		if (phb->type == PNV_PHB_IODA1) {
>>+			int64_t rc;
>
>Here @rc is of the "int64_t" type. And this one and the one above are used
>for return code from OPAL API. Make them the same (int64_t or long, up to
>you).
>

Yep. It will be "int64_t rc" as I said above.

>>+
>>+			rc = opal_pci_map_pe_mmio_window(phb->opal_id,
>>+							 pe->pe_number,
>>+							 OPAL_M64_WINDOW_TYPE,
>>+							 pe->pe_number / 8,
>>+							 pe->pe_number % 8);
>>+			if (rc != OPAL_SUCCESS)
>>+				pr_warn("%s: Failure %lld mapping "
>>+					"M64 for PHB#%d-PE#%d\n",
>>+					__func__, rc,
>>+					phb->hose->global_number,
>>+					pe->pe_number);
>>+		}
>>  	}
>>
>>  	kfree(pe_alloc);
>>@@ -360,12 +443,6 @@ static void __init pnv_ioda_parse_m64_window(struct pnv_phb *phb)
>>  	const u32 *r;
>>  	u64 pci_addr;
>>
>>-	/* FIXME: Support M64 for P7IOC */
>>-	if (phb->type != PNV_PHB_IODA2) {
>>-		pr_info("  Not support M64 window\n");
>>-		return;
>>-	}
>>-
>>  	if (!firmware_has_feature(FW_FEATURE_OPALv3)) {
>>  		pr_info("  Firmware too old to support M64 window\n");
>>  		return;
>>@@ -394,9 +471,23 @@ static void __init pnv_ioda_parse_m64_window(struct pnv_phb *phb)
>>
>>  	/* Use last M64 BAR to cover M64 window */
>>  	phb->ioda.m64_bar_idx = 15;
>>-	phb->init_m64 = pnv_ioda2_init_m64;
>>-	phb->reserve_m64_pe = pnv_ioda2_reserve_m64_pe;
>>-	phb->pick_m64_pe = pnv_ioda2_pick_m64_pe;
>>+	phb->reserve_m64_pe = pnv_ioda_reserve_m64_pe;
>
>
>reserve_m64_pe() is called once from pnv_pci_ioda_setup_PEs() so it is
>IODA-only and in this case reserve_m64_pe != NULL and
>pnv_ioda_reserve_m64_pe() will be called always.
>
>In general, it feels like pnv_phb has too many callbacks while they could be
>just direct calls.
>

We will have another type of IODA compatible PHB soon. I'm not sure if it's
legal to reveal its name now. The new PHB won't have M64 support. I do think
callbacks give us more flexibility (for supporting M64 or not). Lets keep it.

>>+	phb->pick_m64_pe = pnv_ioda_pick_m64_pe;
>>+	switch (phb->type) {
>>+	case PNV_PHB_IODA1:
>>+		phb->init_m64 = pnv_ioda1_init_m64;
>>+		break;
>>+	case PNV_PHB_IODA2:
>>+		phb->init_m64 = pnv_ioda2_init_m64;
>>+		break;
>>+	default:
>>+		phb->init_m64 = NULL;
>>+		phb->reserve_m64_pe = NULL;
>>+		phb->pick_m64_pe = NULL;
>>+		phb->ioda.m64_size = 0;
>>+		phb->ioda.m64_segsize = 0;
>>+		phb->ioda.m64_base = 0;
>
>There are just 2 PHB types - IODA1 and IODA2, right? And the fields you reset
>after "default" - they have to be zeroes already, no? And on what hardware
>would the default branch actuall work? None?
>

Yeah, you're right those piece of garbage can be removed in next revision.

Thanks,
Gavin

>
>>+	}
>>  }
>>
>>  static void pnv_ioda_freeze_pe(struct pnv_phb *phb, int pe_no)
>>
>
>
>-- 
>Alexey
>