| Message ID | 1446642770-4681-13-git-send-email-gwshan@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
Looks good. Will hold off on an official review until I can test the series. Regards, Daniel Gavin Shan <gwshan@linux.vnet.ibm.com> writes: > As we track M32 segment consumption, this introduces an array to > the PHB to track the mapping between M64 segment and PE number. > The information is going to be used to find M64 segment from the > PE number during PCI unplugging time in subsequent patches. > > Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> > --- > arch/powerpc/platforms/powernv/pci-ioda.c | 10 ++++++++-- > arch/powerpc/platforms/powernv/pci.h | 3 ++- > 2 files changed, 10 insertions(+), 3 deletions(-) > > diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c > index 4ab93f8..76ce694 100644 > --- a/arch/powerpc/platforms/powernv/pci-ioda.c > +++ b/arch/powerpc/platforms/powernv/pci-ioda.c > @@ -315,6 +315,7 @@ static int pnv_ioda2_pick_m64_pe(struct pci_bus *bus, bool all) > phb->ioda.total_pe_num) { > pe = &phb->ioda.pe_array[i]; > > + phb->ioda.m64_segmap[pe->pe_number] = pe->pe_number; > if (!master_pe) { > pe->flags |= PNV_IODA_PE_MASTER; > INIT_LIST_HEAD(&pe->slaves); > @@ -3018,7 +3019,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, m64map_off, m32map_off, pemap_off, iomap_off = 0; > const __be64 *prop64; > const __be32 *prop32; > int i, len; > @@ -3103,6 +3104,8 @@ static void __init pnv_pci_init_ioda_phb(struct device_node *np, > > /* Allocate aux data & arrays. We don't have IO ports on PHB3 */ > size = _ALIGN_UP(phb->ioda.total_pe_num / 8, sizeof(unsigned long)); > + m64map_off = size; > + size += phb->ioda.total_pe_num * sizeof(phb->ioda.m64_segmap[0]); > m32map_off = size; > size += phb->ioda.total_pe_num * sizeof(phb->ioda.m32_segmap[0]); > if (phb->type == PNV_PHB_IODA1) { > @@ -3113,9 +3116,12 @@ static void __init pnv_pci_init_ioda_phb(struct device_node *np, > size += phb->ioda.total_pe_num * sizeof(struct pnv_ioda_pe); > aux = memblock_virt_alloc(size, 0); > phb->ioda.pe_alloc = aux; > + phb->ioda.m64_segmap = aux + m64map_off; > phb->ioda.m32_segmap = aux + m32map_off; > - for (i = 0; i < phb->ioda.total_pe_num; i++) > + for (i = 0; i < phb->ioda.total_pe_num; i++) { > + phb->ioda.m64_segmap[i] = IODA_INVALID_PE; > phb->ioda.m32_segmap[i] = IODA_INVALID_PE; > + } > if (phb->type == PNV_PHB_IODA1) { > phb->ioda.io_segmap = aux + iomap_off; > for (i = 0; i < phb->ioda.total_pe_num; i++) > diff --git a/arch/powerpc/platforms/powernv/pci.h b/arch/powerpc/platforms/powernv/pci.h > index 2e01edd..671fd13 100644 > --- a/arch/powerpc/platforms/powernv/pci.h > +++ b/arch/powerpc/platforms/powernv/pci.h > @@ -147,7 +147,8 @@ struct pnv_phb { > unsigned long *pe_alloc; > struct pnv_ioda_pe *pe_array; > > - /* M32 & IO segment maps */ > + /* M64/M32/IO segment maps */ > + int *m64_segmap; > int *m32_segmap; > int *io_segmap; > > -- > 2.1.0 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/05/2015 12:12 AM, Gavin Shan wrote: > As we track M32 segment consumption, this introduces an array to > the PHB to track the mapping between M64 segment and PE number. > The information is going to be used to find M64 segment from the > PE number during PCI unplugging time in subsequent patches. It would not hurt to put a few words about how we managed to live without such a mapping for M64 before but we needed mapping for M32.
On Mon, Nov 16, 2015 at 07:01:59PM +1100, Alexey Kardashevskiy wrote: >On 11/05/2015 12:12 AM, Gavin Shan wrote: >>As we track M32 segment consumption, this introduces an array to >>the PHB to track the mapping between M64 segment and PE number. >>The information is going to be used to find M64 segment from the >>PE number during PCI unplugging time in subsequent patches. > > >It would not hurt to put a few words about how we managed to live without >such a mapping for M64 before but we needed mapping for M32. > The M32 mapping (phb->ioda.m32_segmap[]) isn't used for anything before this patcheset. There're no need for M64 mapping before this patchset similarly, no need to add the words. Thanks, Gavin
On 11/17/2015 12:04 PM, Gavin Shan wrote: > On Mon, Nov 16, 2015 at 07:01:59PM +1100, Alexey Kardashevskiy wrote: >> On 11/05/2015 12:12 AM, Gavin Shan wrote: >>> As we track M32 segment consumption, this introduces an array to >>> the PHB to track the mapping between M64 segment and PE number. >>> The information is going to be used to find M64 segment from the >>> PE number during PCI unplugging time in subsequent patches. >> >> >> It would not hurt to put a few words about how we managed to live without >> such a mapping for M64 before but we needed mapping for M32. >> > > The M32 mapping (phb->ioda.m32_segmap[]) isn't used for anything before > this patcheset. There're no need for M64 mapping before this patchset > similarly, no need to add the words. After years I learned that reviewers ask less questions about new but yet unused code when I put few words in the commit log confirming that it is not used now but it will be used for <here I put what it is for> later. And it is not obvious that m32_segment is not used. And m64_segmap is started being used only 13 patches later in: [PATCH v7 27/50] powerpc/powernv: Dynamically release PEs which is quite far, complicates reviewing. 12/50 is better be moved there (to make it 26/50) or just merged into 27/50.
On Thu, Nov 19, 2015 at 11:10:42AM +1100, Alexey Kardashevskiy wrote: >On 11/17/2015 12:04 PM, Gavin Shan wrote: >>On Mon, Nov 16, 2015 at 07:01:59PM +1100, Alexey Kardashevskiy wrote: >>>On 11/05/2015 12:12 AM, Gavin Shan wrote: >>>>As we track M32 segment consumption, this introduces an array to >>>>the PHB to track the mapping between M64 segment and PE number. >>>>The information is going to be used to find M64 segment from the >>>>PE number during PCI unplugging time in subsequent patches. >>> >>> >>>It would not hurt to put a few words about how we managed to live without >>>such a mapping for M64 before but we needed mapping for M32. >>> >> >>The M32 mapping (phb->ioda.m32_segmap[]) isn't used for anything before >>this patcheset. There're no need for M64 mapping before this patchset >>similarly, no need to add the words. > >After years I learned that reviewers ask less questions about new but yet >unused code when I put few words in the commit log confirming that it is not >used now but it will be used for <here I put what it is for> later. > >And it is not obvious that m32_segment is not used. And m64_segmap is started >being used only 13 patches later in: > >[PATCH v7 27/50] powerpc/powernv: Dynamically release PEs > >which is quite far, complicates reviewing. 12/50 is better be moved there (to >make it 26/50) or just merged into 27/50. > It doesn't make sense to me. As said in PATCH[00/50], the patchset consists of 3 separate parts: PowerNV PCI rework; Using PCI slot; Hotplug standalone driver; For the first part ("PowerNV PCI rework"), the patches are organized in order: refactor/cleanup, IO/M32/M64, DMA, PE allocation/deallocation. So I don't think I need move the patch around if you don't have a stronger reason. Thanks, Gavin
diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c index 4ab93f8..76ce694 100644 --- a/arch/powerpc/platforms/powernv/pci-ioda.c +++ b/arch/powerpc/platforms/powernv/pci-ioda.c @@ -315,6 +315,7 @@ static int pnv_ioda2_pick_m64_pe(struct pci_bus *bus, bool all) phb->ioda.total_pe_num) { pe = &phb->ioda.pe_array[i]; + phb->ioda.m64_segmap[pe->pe_number] = pe->pe_number; if (!master_pe) { pe->flags |= PNV_IODA_PE_MASTER; INIT_LIST_HEAD(&pe->slaves); @@ -3018,7 +3019,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, m64map_off, m32map_off, pemap_off, iomap_off = 0; const __be64 *prop64; const __be32 *prop32; int i, len; @@ -3103,6 +3104,8 @@ static void __init pnv_pci_init_ioda_phb(struct device_node *np, /* Allocate aux data & arrays. We don't have IO ports on PHB3 */ size = _ALIGN_UP(phb->ioda.total_pe_num / 8, sizeof(unsigned long)); + m64map_off = size; + size += phb->ioda.total_pe_num * sizeof(phb->ioda.m64_segmap[0]); m32map_off = size; size += phb->ioda.total_pe_num * sizeof(phb->ioda.m32_segmap[0]); if (phb->type == PNV_PHB_IODA1) { @@ -3113,9 +3116,12 @@ static void __init pnv_pci_init_ioda_phb(struct device_node *np, size += phb->ioda.total_pe_num * sizeof(struct pnv_ioda_pe); aux = memblock_virt_alloc(size, 0); phb->ioda.pe_alloc = aux; + phb->ioda.m64_segmap = aux + m64map_off; phb->ioda.m32_segmap = aux + m32map_off; - for (i = 0; i < phb->ioda.total_pe_num; i++) + for (i = 0; i < phb->ioda.total_pe_num; i++) { + phb->ioda.m64_segmap[i] = IODA_INVALID_PE; phb->ioda.m32_segmap[i] = IODA_INVALID_PE; + } if (phb->type == PNV_PHB_IODA1) { phb->ioda.io_segmap = aux + iomap_off; for (i = 0; i < phb->ioda.total_pe_num; i++) diff --git a/arch/powerpc/platforms/powernv/pci.h b/arch/powerpc/platforms/powernv/pci.h index 2e01edd..671fd13 100644 --- a/arch/powerpc/platforms/powernv/pci.h +++ b/arch/powerpc/platforms/powernv/pci.h @@ -147,7 +147,8 @@ struct pnv_phb { unsigned long *pe_alloc; struct pnv_ioda_pe *pe_array; - /* M32 & IO segment maps */ + /* M64/M32/IO segment maps */ + int *m64_segmap; int *m32_segmap; int *io_segmap;
As we track M32 segment consumption, this introduces an array to the PHB to track the mapping between M64 segment and PE number. The information is going to be used to find M64 segment from the PE number during PCI unplugging time in subsequent patches. Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> --- arch/powerpc/platforms/powernv/pci-ioda.c | 10 ++++++++-- arch/powerpc/platforms/powernv/pci.h | 3 ++- 2 files changed, 10 insertions(+), 3 deletions(-)