On Mon, Aug 10, 2015 at 04:30:09PM +1000, Alexey Kardashevskiy wrote:
>On 08/06/2015 02:11 PM, Gavin Shan wrote:
>>The patch enables M64 window on P7IOC, which has been enabled on
>>PHB3. Different from PHB3 where 16 M64 BARs are supported and each
>>of them can be owned by one particular PE# exclusively or divided
>>evenly to 256 segments, each P7IOC PHB has 16 M64 BARs and each
>>of them are divided into 8 segments.
>
>Is this a limitation of POWER7 chip or it is from IODA1?
>
>From IODA1.
>>So each P7IOC PHB can support
>>128 M64 segments only. Also, P7IOC has M64DT, which helps mapping
>>one particular M64 segment# to arbitrary PE#. PHB3 doesn't have
>>M64DT, indicating that one M64 segment can only be pinned to the
>>fixed PE#. In order to have similar logic to support M64 for PHB3
>>and P7IOC, we just provide 128 M64 (16 BARs) segments and fixed
>>mapping between PE# and M64 segment# on P7IOC. In turn, we just
>>need different phb->init_m64() hooks for P7IOC and PHB3 to support
>>M64.
>>
>>Signed-off-by: Gavin Shan <gwshan@xxxxxxxxxxxxxxxxxx>
>>---
>> arch/powerpc/platforms/powernv/pci-ioda.c | 116 ++++++++++++++++++++++++++----
>> 1 file changed, 104 insertions(+), 12 deletions(-)
>>
>>diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
>>index 38b5405..e4ac703 100644
>>--- a/arch/powerpc/platforms/powernv/pci-ioda.c
>>+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
>>@@ -172,6 +172,69 @@ 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;
>>+
>>+ /* There are as many M64 segments as the maximum number
>>+ * of PEs, which is 128.
>>+ */
>>+ for (seg = 0; seg < phb->ioda.total_pe; seg += 8) {
>
>
>This "8" is used a lot across the patch, please make it a macro
>(PNV_PHB_P7IOC_SEGNUM or PNV_PHB_IODA1_SEGNUM or whatever you think it is)
>with a short comment why it is "8". Or a pnv_phb member.
>
I would like to use "8". When having a macro, you have to check
the definition of the macro to get the real value of that. However,
it makes sense to add more comments explaining why it's 8 here.
>
>>+ unsigned long base;
>>+ int64_t rc;
>>+
>>+ 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(" Error %lld setting M64 PHB#%d-BAR#%d\n",
>>+ rc, phb->hose->global_number, seg / 8);
>>+ 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(" Error %lld enabling M64 PHB#%d-BAR#%d\n",
>>+ rc, phb->hose->global_number, seg / 8);
>>+ goto fail;
>>+ }
>>+ }
>>+
>>+ /* Strip off the segment used by the reserved PE, which
>
>What is this reserved PE on P7IOC? "Strip off" means "exclude" here?
>
127 that was exported from skiboot. "Strip off" means "exclude".
>
>>+ * is expected to be 0 or last supported PE#. The PHB's
>>+ * first memory window traces the 32-bits MMIO range
>
>s/traces/filters/ ? Or I did not understand this comment...
>
It seems you didn't understand it: there are two memory windows
in every PHB. The first one is tracing M32 resource and the
second one is tracing M64 resource.
>
>>+ * while the second one traces the 64-bits prefetchable
>>+ * MMIO range that the PHB supports.
>
>32/64 ranges comment seems irrelevant here.
>
Maybe it's not so relevant, but still. We're stripping off the
M64 segment from the 2nd resource (as above), not first one.
>
>>+ */
>>+ r = &phb->hose->mem_resources[1];
>>+ 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);
>>+
>>+ return -EIO;
>>+}
>>+
>> /* The default M64 BAR is shared by all PEs */
>> static int pnv_ioda2_init_m64(struct pnv_phb *phb)
>> {
>>@@ -256,9 +319,9 @@ static void pnv_ioda2_reserve_dev_m64_pe(struct pci_dev *pdev,
>> }
>> }
>>
>>-static void pnv_ioda2_reserve_m64_pe(struct pci_bus *bus,
>>- unsigned long *pe_bitmap,
>>- bool all)
>>+static void pnv_ioda_reserve_m64_pe(struct pci_bus *bus,
>>+ unsigned long *pe_bitmap,
>>+ bool all)
>> {
>> struct pci_dev *pdev;
>>
>>@@ -266,12 +329,12 @@ static void pnv_ioda2_reserve_m64_pe(struct pci_bus *bus,
>> pnv_ioda2_reserve_dev_m64_pe(pdev, pe_bitmap);
>>
>> if (all && pdev->subordinate)
>>- pnv_ioda2_reserve_m64_pe(pdev->subordinate,
>>- pe_bitmap, all);
>>+ pnv_ioda_reserve_m64_pe(pdev->subordinate,
>>+ pe_bitmap, all);
>> }
>> }
>>
>>-static int pnv_ioda2_pick_m64_pe(struct pci_bus *bus, bool all)
>>+static int pnv_ioda_pick_m64_pe(struct pci_bus *bus, bool all)
>> {
>> struct pci_controller *hose = pci_bus_to_host(bus);
>> struct pnv_phb *phb = hose->private_data;
>>@@ -293,7 +356,7 @@ static int pnv_ioda2_pick_m64_pe(struct pci_bus *bus, bool all)
>> }
>>
>> /* Figure out reserved PE numbers by the PE */
>>- pnv_ioda2_reserve_m64_pe(bus, pe_alloc, all);
>>+ pnv_ioda_reserve_m64_pe(bus, pe_alloc, all);
>>
>> /*
>> * the current bus might not own M64 window and that's all
>>@@ -324,6 +387,26 @@ static int pnv_ioda2_pick_m64_pe(struct pci_bus *bus, bool all)
>> pe->master = master_pe;
>> list_add_tail(&pe->list, &master_pe->slaves);
>> }
>>+
>>+ /* P7IOC supports M64DT, which helps mapping M64 segment
>>+ * to one particular PE#. However, PHB3 has fixed mapping
>>+ * between M64 segment and PE#. In order to have 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;
>>+
>>+ 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: Error %lld mapping M64 for PHB#%d-PE#%d\n",
>>+ __func__, rc, phb->hose->global_number,
>>+ pe->pe_number);
>>+ }
>> }
>>
>> kfree(pe_alloc);
>>@@ -338,8 +421,8 @@ 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) {
>>+ if (phb->type != PNV_PHB_IODA1 &&
>>+ phb->type != PNV_PHB_IODA2) {
>> pr_info(" Not support M64 window\n");
>> return;
>
>
>You are adding P7IOC support so at least "fixme" should go. Also,
>pnv_ioda_parse_m64_window() is only called from pnv_pci_init_ioda_phb() which
>is called only with PNV_PHB_IODA1 and PNV_PHB_IODA2 (no other value is passed
>there a type) so the check above will never succeed, just remove it.
>
The "fixme" is removed, isn't it?
As I explained last time, there will have another new type PHB and the function
will be called on the new type of PHB. The code has been there and it's not
in upstream yet. So it's reasonable to keep it, instead of removing it.
>> }
>>@@ -372,9 +455,18 @@ 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;
>>+ 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:
>>+ pr_debug(" M64 not supported\n");
>>+ }
>> }
>>
>> static void pnv_ioda_freeze_pe(struct pnv_phb *phb, int pe_no)
>>
Thanks,
Gavin
--
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
