On Tue, Aug 11, 2015 at 12:06:26PM +1000, Alexey Kardashevskiy wrote:
>On 08/11/2015 09:45 AM, Gavin Shan wrote:
>>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.
>
>Give it a good name then.
>
>
>>However,
>>it makes sense to add more comments explaining why it's 8 here.
>
>You cannot comment it everywhere and everywhere is exact place when you'll
>have to comment it as I believe sometime it is segments-per-M64 and sometime
>it is number of bits in a byte (or not? anyway, this is will always distract
>unless you use macro for segments-per-M64).
>
Ok. I will use PNV_PHB_IODA1_SEGNUM then.
>>
>>>
>>>>+ 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".
>
>I like "exclude" lot better.
>
Ok. Will use it.
>>
>>>
>>>>+ * 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.
>
>
>Tracing means logging, pretty much. Is this what you mean here?
>
No, it means "recording", not "logging". So it would be appropriate
to replace it with "track"?
>>
>>>
>>>>+ * 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.
>
>Not relevant -> remove it. Put this text to the commit log.
>
Ok.
>>We're stripping off the
>>M64 segment from the 2nd resource (as above), not first one.
>
>
>2nd window (not _resource_), you mean?
>
I mean struct pci_controller::mem_resources[1].
>
>>
>>>
>>>>+ */
>>>>+ 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?
>
>Ah, my bad.
>
>
>>As I explained last time, there will have another new type PHB and the function
>>will be called on the new type of PHB.
>
>Then a new patch adding new PHB should take care of this check too. This is
>not something which can possibly happen on a real machine, we support one of
>2 (later - 3) PHBs and if a machine got something else, we won't get that far
>anyway and we cannot gracefully fallback to some "generic PHB" (like 440fx on
>x86) as we do not have one.
>
>At least make it BUG_ON() to document it.
>
ok. I'll change accordingly.
>>The code has been there and it's not
>>in upstream yet. So it's reasonable to keep it, instead of removing it.
>
>No, not really.
>
>>
>>>> }
>>>>@@ -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 devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
