On Mon, Aug 10, 2015 at 05:16:40PM +1000, Alexey Kardashevskiy wrote:
>On 08/06/2015 02:11 PM, Gavin Shan wrote:
>>The patch is adding 6 bitmaps, three to PE and three to PHB, to track
>
>The patch is also removing 2 arrays (io_segmap and m32_segmap), what is that
>all about? Also, there was no m64_segmap, now there is, needs an explanation
>may be.
>
Originally, the bitmaps (io_segmap and m32_segmap) are allocated dynamically.
Now, they have fixed sizes - 512 bits.
The subject "powerpc/powernv: Track IO/M32/M64 segments from PE" indicates
why m64_segmap is added.
>
>>the consumed by one particular PE, which can be released once the PE
>>is destroyed during PCI unplugging time. Also, we're using fixed
>>quantity of bits to trace the used IO, M32 and M64 segments by PEs
>>in one particular PHB.
>>
>
>Out of curiosity - have you considered having just 3 arrays, in PHB, storing
>PE numbers, and ditching PE's arrays? Does PE itself need to know what PEs it
>is using? Not sure about this master/slave PEs though.
>
I don't follow your suggestion. Can you rephrase and explain it a bit more?
>It would be easier to read patches if this one was right before
>[PATCH v6 23/42] powerpc/powernv: Release PEs dynamically
>
I'll try to reoder the patch, but not expect too much...
>
>
>>Signed-off-by: Gavin Shan <gwshan@xxxxxxxxxxxxxxxxxx>
>>---
>> arch/powerpc/platforms/powernv/pci-ioda.c | 29 +++++++++++++++--------------
>> arch/powerpc/platforms/powernv/pci.h | 18 ++++++++++++++----
>> 2 files changed, 29 insertions(+), 18 deletions(-)
>>
>>diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
>>index e4ac703..78b49a1 100644
>>--- a/arch/powerpc/platforms/powernv/pci-ioda.c
>>+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
>>@@ -388,6 +388,12 @@ static int pnv_ioda_pick_m64_pe(struct pci_bus *bus, bool all)
>> list_add_tail(&pe->list, &master_pe->slaves);
>> }
>>
>>+ /* M64 segments consumed by slave PEs are tracked
>>+ * by master PE
>>+ */
>>+ set_bit(pe->pe_number, master_pe->m64_segmap);
>>+ set_bit(pe->pe_number, phb->ioda.m64_segmap);
>>+
>> /* 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
>>@@ -2871,9 +2877,11 @@ static void pnv_ioda_setup_pe_seg(struct pci_controller *hose,
>>
>> while (index < phb->ioda.total_pe &&
>> region.start <= region.end) {
>>- phb->ioda.io_segmap[index] = pe->pe_number;
>>+ set_bit(index, pe->io_segmap);
>>+ set_bit(index, phb->ioda.io_segmap);
>> rc = opal_pci_map_pe_mmio_window(phb->opal_id,
>>- pe->pe_number, OPAL_IO_WINDOW_TYPE, 0, index);
>>+ pe->pe_number, OPAL_IO_WINDOW_TYPE,
>>+ 0, index);
>
>Unrelated change.
>
True, will drop. However, checkpatch.pl will complain wtih:
exceeding 80 characters.
>> if (rc != OPAL_SUCCESS) {
>> pr_err("%s: OPAL error %d when mapping IO "
>> "segment #%d to PE#%d\n",
>>@@ -2896,9 +2904,11 @@ static void pnv_ioda_setup_pe_seg(struct pci_controller *hose,
>>
>> while (index < phb->ioda.total_pe &&
>> region.start <= region.end) {
>>- phb->ioda.m32_segmap[index] = pe->pe_number;
>>+ set_bit(index, pe->m32_segmap);
>>+ set_bit(index, phb->ioda.m32_segmap);
>> rc = opal_pci_map_pe_mmio_window(phb->opal_id,
>>- pe->pe_number, OPAL_M32_WINDOW_TYPE, 0, index);
>>+ pe->pe_number, OPAL_M32_WINDOW_TYPE,
>>+ 0, index);
>
>Unrelated change.
>
same as above.
>> if (rc != OPAL_SUCCESS) {
>> pr_err("%s: OPAL error %d when mapping M32 "
>> "segment#%d to PE#%d",
>>@@ -3090,7 +3100,7 @@ static void __init pnv_pci_init_ioda_phb(struct device_node *np,
>> {
>> struct pci_controller *hose;
>> struct pnv_phb *phb;
>>- unsigned long size, m32map_off, pemap_off, iomap_off = 0;
>>+ unsigned long size, pemap_off;
>> const __be64 *prop64;
>> const __be32 *prop32;
>> int len;
>>@@ -3175,19 +3185,10 @@ static void __init pnv_pci_init_ioda_phb(struct device_node *np,
>>
>> /* Allocate aux data & arrays. We don't have IO ports on PHB3 */
>
>
>This comment came with if(IODA1) below, since you are removing the condition
>below, makes sense to remove the comment as well or move it where people will
>look for it (arch/powerpc/platforms/powernv/pci.h ?)
>
Yes, will do.
>
>> 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]);
>>- if (phb->type == PNV_PHB_IODA1) {
>>- iomap_off = size;
>>- size += phb->ioda.total_pe * sizeof(phb->ioda.io_segmap[0]);
>>- }
>> pemap_off = size;
>> size += phb->ioda.total_pe * sizeof(struct pnv_ioda_pe);
>> aux = memblock_virt_alloc(size, 0);
>
>
>After adding static arrays to PE and PHB, do you still need this "aux"?
>
"aux" is still needed to tell the boundary of pe_alloc_bitmap and pe_array.
>
>> phb->ioda.pe_alloc = aux;
>>- phb->ioda.m32_segmap = aux + m32map_off;
>>- if (phb->type == PNV_PHB_IODA1)
>>- phb->ioda.io_segmap = aux + iomap_off;
>> phb->ioda.pe_array = aux + pemap_off;
>> set_bit(phb->ioda.reserved_pe, phb->ioda.pe_alloc);
>>
>>diff --git a/arch/powerpc/platforms/powernv/pci.h b/arch/powerpc/platforms/powernv/pci.h
>>index 62239b1..08a4e57 100644
>>--- a/arch/powerpc/platforms/powernv/pci.h
>>+++ b/arch/powerpc/platforms/powernv/pci.h
>>@@ -49,6 +49,15 @@ struct pnv_ioda_pe {
>> /* PE number */
>> unsigned int pe_number;
>>
>>+ /* IO/M32/M64 segments consumed by the PE. Each PE can
>>+ * have one M64 segment at most, but M64 segments consumed
>>+ * by slave PEs will be contributed to the master PE. One
>>+ * PE can own multiple IO and M32 segments.
>
>
>A PE can have multiple IO and M32 segments but just one M64 segment? Is this
>correct for IODA1 or IODA2 or both? Is this a limitation of this
>implementation or it comes from P7IOC/PHB3 hardware?
>
It's correct for IO and M32. However, on IODA1 or IODA2, one PE can have
multiple M64 segments as well.
>>+ */
>>+ unsigned long io_segmap[8];
>>+ unsigned long m32_segmap[8];
>>+ unsigned long m64_segmap[8];
>
>Magic constant "8", 64bit*8 = 512 PEs - where did this come from?
>
>Anyway,
>
>#define PNV_IODA_MAX_PE_NUM 512
>
>unsigned long io_segmap[PNV_IODA_MAX_PE_NUM/BITS_PER_LONG]
>
I prefer "8", not macro for 3 reasons:
- The macro won't be used in the code.
- The total segment number of specific resource is variable
on IODA1 and IODA2. I just choosed the max value with margin.
- PNV_IODA_MAX_PE_NUM, indicating max PE number, isn't 512 on
IODA1 or IODA2.
>>+
>> /* "Weight" assigned to the PE for the sake of DMA resource
>> * allocations
>> */
>>@@ -145,15 +154,16 @@ struct pnv_phb {
>> unsigned int io_segsize;
>> unsigned int io_pci_base;
>>
>>+ /* IO, M32, M64 segment maps */
>>+ unsigned long io_segmap[8];
>>+ unsigned long m32_segmap[8];
>>+ unsigned long m64_segmap[8];
>>+
>> /* PE allocation */
>> struct mutex pe_alloc_mutex;
>> unsigned long *pe_alloc;
>> struct pnv_ioda_pe *pe_array;
>>
>>- /* M32 & IO segment maps */
>>- unsigned int *m32_segmap;
>>- unsigned int *io_segmap;
>>-
>> /* IRQ chip */
>> int irq_chip_init;
>> struct irq_chip irq_chip;
>>
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
