| Message ID | 1446642770-4681-11-git-send-email-gwshan@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
Gavin Shan <gwshan@linux.vnet.ibm.com> writes: > The original implementation of pnv_ioda_setup_pe_seg() configures > IO and M32 segments by separate logics, which can be merged by > by caching @segmap, @seg_size, @win in advance. This shouldn't > cause any behavioural changes. > > Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> > --- > arch/powerpc/platforms/powernv/pci-ioda.c | 62 ++++++++++++++----------------- > 1 file changed, 28 insertions(+), 34 deletions(-) > > diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c > index 7ee7cfe..553d3f3 100644 > --- a/arch/powerpc/platforms/powernv/pci-ioda.c > +++ b/arch/powerpc/platforms/powernv/pci-ioda.c > @@ -2752,8 +2752,10 @@ static void pnv_ioda_setup_pe_seg(struct pci_controller *hose, > struct pnv_phb *phb = hose->private_data; > struct pci_bus_region region; > struct resource *res; > - int i, index; > - int rc; > + unsigned int segsize; > + int *segmap, index, i; > + uint16_t win; > + int64_t rc; Good catch! Opal return codes are 64 bit and that should be explicit in the type. However, I seem to remember that we preferred a different type for 64 bit ints in the kernel. I think it's s64, and there are some other uses of that in pci_ioda.c for return codes. (I'm actually surprised that's not picked up as a compiler warning. Maybe that's something to look at in future.) The rest of the patch looks good on casual inspection - to be sure I'll test the entire series on a machine. (hopefully, time permitting!) Regards, Daniel > > /* > * NOTE: We only care PCI bus based PE for now. For PCI > @@ -2770,23 +2772,9 @@ static void pnv_ioda_setup_pe_seg(struct pci_controller *hose, > if (res->flags & IORESOURCE_IO) { > region.start = res->start - phb->ioda.io_pci_base; > region.end = res->end - phb->ioda.io_pci_base; > - index = region.start / phb->ioda.io_segsize; > - > - while (index < phb->ioda.total_pe_num && > - region.start <= region.end) { > - phb->ioda.io_segmap[index] = pe->pe_number; > - rc = opal_pci_map_pe_mmio_window(phb->opal_id, > - pe->pe_number, OPAL_IO_WINDOW_TYPE, 0, index); > - if (rc != OPAL_SUCCESS) { > - pr_err("%s: OPAL error %d when mapping IO " > - "segment #%d to PE#%d\n", > - __func__, rc, index, pe->pe_number); > - break; > - } > - > - region.start += phb->ioda.io_segsize; > - index++; > - } > + segsize = phb->ioda.io_segsize; > + segmap = phb->ioda.io_segmap; > + win = OPAL_IO_WINDOW_TYPE; > } else if ((res->flags & IORESOURCE_MEM) && > !pnv_pci_is_mem_pref_64(res->flags)) { > region.start = res->start - > @@ -2795,23 +2783,29 @@ static void pnv_ioda_setup_pe_seg(struct pci_controller *hose, > region.end = res->end - > hose->mem_offset[0] - > phb->ioda.m32_pci_base; > - index = region.start / phb->ioda.m32_segsize; > - > - while (index < phb->ioda.total_pe_num && > - region.start <= region.end) { > - phb->ioda.m32_segmap[index] = pe->pe_number; > - rc = opal_pci_map_pe_mmio_window(phb->opal_id, > - pe->pe_number, OPAL_M32_WINDOW_TYPE, 0, index); > - if (rc != OPAL_SUCCESS) { > - pr_err("%s: OPAL error %d when mapping M32 " > - "segment#%d to PE#%d", > - __func__, rc, index, pe->pe_number); > - break; > - } > + segsize = phb->ioda.m32_segsize; > + segmap = phb->ioda.m32_segmap; > + win = OPAL_M32_WINDOW_TYPE; > + } else { > + continue; > + } > > - region.start += phb->ioda.m32_segsize; > - index++; > + index = region.start / segsize; > + while (index < phb->ioda.total_pe_num && > + region.start <= region.end) { > + segmap[index] = pe->pe_number; > + rc = opal_pci_map_pe_mmio_window(phb->opal_id, > + pe->pe_number, win, 0, index); > + if (rc != OPAL_SUCCESS) { > + pr_warn("%s: Error %lld mapping (%d) seg#%d to PHB#%d-PE#%d\n", > + __func__, rc, win, index, > + pe->phb->hose->global_number, > + pe->pe_number); > + break; > } > + > + region.start += segsize; > + index++; > } > } > } > -- > 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 Fri, Nov 06, 2015 at 09:56:06AM +1100, Daniel Axtens wrote: >Gavin Shan <gwshan@linux.vnet.ibm.com> writes: > >> The original implementation of pnv_ioda_setup_pe_seg() configures >> IO and M32 segments by separate logics, which can be merged by >> by caching @segmap, @seg_size, @win in advance. This shouldn't >> cause any behavioural changes. >> >> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> >> --- >> arch/powerpc/platforms/powernv/pci-ioda.c | 62 ++++++++++++++----------------- >> 1 file changed, 28 insertions(+), 34 deletions(-) >> >> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c >> index 7ee7cfe..553d3f3 100644 >> --- a/arch/powerpc/platforms/powernv/pci-ioda.c >> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c >> @@ -2752,8 +2752,10 @@ static void pnv_ioda_setup_pe_seg(struct pci_controller *hose, >> struct pnv_phb *phb = hose->private_data; >> struct pci_bus_region region; >> struct resource *res; >> - int i, index; >> - int rc; >> + unsigned int segsize; >> + int *segmap, index, i; >> + uint16_t win; >> + int64_t rc; > >Good catch! Opal return codes are 64 bit and that should be explicit >in the type. However, I seem to remember that we preferred a different >type for 64 bit ints in the kernel. I think it's s64, and there are some >other uses of that in pci_ioda.c for return codes. > Both int64_t and s64 are fine. I used s64 for the OPAL return value, but Alexey likes "int64_t", which is ok to me as well. I won't change it back to s64 :-) >(I'm actually surprised that's not picked up as a compiler >warning. Maybe that's something to look at in future.) > Indeed, I didn't see a warning from gcc. >The rest of the patch looks good on casual inspection - to be sure I'll >test the entire series on a machine. (hopefully, time permitting!) > I run scripts/checkpatch.pl on the patchset. Only one warning came from [PATCH 44/50], but I won't bother to change that as the warning was brought by original code. If you want to test this patchset, you need run it on Tuleta where the hotpluggable PCI slots are supported. Thanks, Gavin >> >> /* >> * NOTE: We only care PCI bus based PE for now. For PCI >> @@ -2770,23 +2772,9 @@ static void pnv_ioda_setup_pe_seg(struct pci_controller *hose, >> if (res->flags & IORESOURCE_IO) { >> region.start = res->start - phb->ioda.io_pci_base; >> region.end = res->end - phb->ioda.io_pci_base; >> - index = region.start / phb->ioda.io_segsize; >> - >> - while (index < phb->ioda.total_pe_num && >> - region.start <= region.end) { >> - phb->ioda.io_segmap[index] = pe->pe_number; >> - rc = opal_pci_map_pe_mmio_window(phb->opal_id, >> - pe->pe_number, OPAL_IO_WINDOW_TYPE, 0, index); >> - if (rc != OPAL_SUCCESS) { >> - pr_err("%s: OPAL error %d when mapping IO " >> - "segment #%d to PE#%d\n", >> - __func__, rc, index, pe->pe_number); >> - break; >> - } >> - >> - region.start += phb->ioda.io_segsize; >> - index++; >> - } >> + segsize = phb->ioda.io_segsize; >> + segmap = phb->ioda.io_segmap; >> + win = OPAL_IO_WINDOW_TYPE; >> } else if ((res->flags & IORESOURCE_MEM) && >> !pnv_pci_is_mem_pref_64(res->flags)) { >> region.start = res->start - >> @@ -2795,23 +2783,29 @@ static void pnv_ioda_setup_pe_seg(struct pci_controller *hose, >> region.end = res->end - >> hose->mem_offset[0] - >> phb->ioda.m32_pci_base; >> - index = region.start / phb->ioda.m32_segsize; >> - >> - while (index < phb->ioda.total_pe_num && >> - region.start <= region.end) { >> - phb->ioda.m32_segmap[index] = pe->pe_number; >> - rc = opal_pci_map_pe_mmio_window(phb->opal_id, >> - pe->pe_number, OPAL_M32_WINDOW_TYPE, 0, index); >> - if (rc != OPAL_SUCCESS) { >> - pr_err("%s: OPAL error %d when mapping M32 " >> - "segment#%d to PE#%d", >> - __func__, rc, index, pe->pe_number); >> - break; >> - } >> + segsize = phb->ioda.m32_segsize; >> + segmap = phb->ioda.m32_segmap; >> + win = OPAL_M32_WINDOW_TYPE; >> + } else { >> + continue; >> + } >> >> - region.start += phb->ioda.m32_segsize; >> - index++; >> + index = region.start / segsize; >> + while (index < phb->ioda.total_pe_num && >> + region.start <= region.end) { >> + segmap[index] = pe->pe_number; >> + rc = opal_pci_map_pe_mmio_window(phb->opal_id, >> + pe->pe_number, win, 0, index); >> + if (rc != OPAL_SUCCESS) { >> + pr_warn("%s: Error %lld mapping (%d) seg#%d to PHB#%d-PE#%d\n", >> + __func__, rc, win, index, >> + pe->phb->hose->global_number, >> + pe->pe_number); >> + break; >> } >> + >> + region.start += segsize; >> + index++; >> } >> } >> } >> -- >> 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 -- 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/06/2015 10:52 AM, Gavin Shan wrote: > On Fri, Nov 06, 2015 at 09:56:06AM +1100, Daniel Axtens wrote: >> Gavin Shan <gwshan@linux.vnet.ibm.com> writes: >> >>> The original implementation of pnv_ioda_setup_pe_seg() configures >>> IO and M32 segments by separate logics, which can be merged by >>> by caching @segmap, @seg_size, @win in advance. This shouldn't >>> cause any behavioural changes. >>> >>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> >>> --- >>> arch/powerpc/platforms/powernv/pci-ioda.c | 62 ++++++++++++++----------------- >>> 1 file changed, 28 insertions(+), 34 deletions(-) >>> >>> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c >>> index 7ee7cfe..553d3f3 100644 >>> --- a/arch/powerpc/platforms/powernv/pci-ioda.c >>> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c >>> @@ -2752,8 +2752,10 @@ static void pnv_ioda_setup_pe_seg(struct pci_controller *hose, >>> struct pnv_phb *phb = hose->private_data; >>> struct pci_bus_region region; >>> struct resource *res; >>> - int i, index; >>> - int rc; >>> + unsigned int segsize; >>> + int *segmap, index, i; >>> + uint16_t win; >>> + int64_t rc; >> >> Good catch! Opal return codes are 64 bit and that should be explicit >> in the type. However, I seem to remember that we preferred a different >> type for 64 bit ints in the kernel. I think it's s64, and there are some >> other uses of that in pci_ioda.c for return codes. >> > > Both int64_t and s64 are fine. I used s64 for the OPAL return value, but > Alexey likes "int64_t", which is ok to me as well. I won't change it back > to s64 :-) > >> (I'm actually surprised that's not picked up as a compiler >> warning. Maybe that's something to look at in future.) >> > > Indeed, I didn't see a warning from gcc. > >> The rest of the patch looks good on casual inspection - to be sure I'll >> test the entire series on a machine. (hopefully, time permitting!) >> > > I run scripts/checkpatch.pl on the patchset. Only one warning came from > [PATCH 44/50], but I won't bother to change that as the warning was > brought by original code. None of these patches failed checkpatch.pl check, what was the error in 44/50?
On Mon, Nov 16, 2015 at 07:01:18PM +1100, Alexey Kardashevskiy wrote: >On 11/06/2015 10:52 AM, Gavin Shan wrote: >>On Fri, Nov 06, 2015 at 09:56:06AM +1100, Daniel Axtens wrote: >>>Gavin Shan <gwshan@linux.vnet.ibm.com> writes: >>> >>>>The original implementation of pnv_ioda_setup_pe_seg() configures >>>>IO and M32 segments by separate logics, which can be merged by >>>>by caching @segmap, @seg_size, @win in advance. This shouldn't >>>>cause any behavioural changes. >>>> >>>>Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> >>>>--- >>>> arch/powerpc/platforms/powernv/pci-ioda.c | 62 ++++++++++++++----------------- >>>> 1 file changed, 28 insertions(+), 34 deletions(-) >>>> >>>>diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c >>>>index 7ee7cfe..553d3f3 100644 >>>>--- a/arch/powerpc/platforms/powernv/pci-ioda.c >>>>+++ b/arch/powerpc/platforms/powernv/pci-ioda.c >>>>@@ -2752,8 +2752,10 @@ static void pnv_ioda_setup_pe_seg(struct pci_controller *hose, >>>> struct pnv_phb *phb = hose->private_data; >>>> struct pci_bus_region region; >>>> struct resource *res; >>>>- int i, index; >>>>- int rc; >>>>+ unsigned int segsize; >>>>+ int *segmap, index, i; >>>>+ uint16_t win; >>>>+ int64_t rc; >>> >>>Good catch! Opal return codes are 64 bit and that should be explicit >>>in the type. However, I seem to remember that we preferred a different >>>type for 64 bit ints in the kernel. I think it's s64, and there are some >>>other uses of that in pci_ioda.c for return codes. >>> >> >>Both int64_t and s64 are fine. I used s64 for the OPAL return value, but >>Alexey likes "int64_t", which is ok to me as well. I won't change it back >>to s64 :-) >> >>>(I'm actually surprised that's not picked up as a compiler >>>warning. Maybe that's something to look at in future.) >>> >> >>Indeed, I didn't see a warning from gcc. >> >>>The rest of the patch looks good on casual inspection - to be sure I'll >>>test the entire series on a machine. (hopefully, time permitting!) >>> >> >>I run scripts/checkpatch.pl on the patchset. Only one warning came from >>[PATCH 44/50], but I won't bother to change that as the warning was >>brought by original code. > >None of these patches failed checkpatch.pl check, what was the error in 44/50? > You're right that none of those patches failed with checkpatch.pl. I had revison 6.1, 6.2, 6.3 before this revision (v7) was posted. There was a warning for 44/50 in 6.3 (perhaps). I run checkpatch.pl on all (v7) patches, no warning reported. Thanks, Gavin -- 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
diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c index 7ee7cfe..553d3f3 100644 --- a/arch/powerpc/platforms/powernv/pci-ioda.c +++ b/arch/powerpc/platforms/powernv/pci-ioda.c @@ -2752,8 +2752,10 @@ static void pnv_ioda_setup_pe_seg(struct pci_controller *hose, struct pnv_phb *phb = hose->private_data; struct pci_bus_region region; struct resource *res; - int i, index; - int rc; + unsigned int segsize; + int *segmap, index, i; + uint16_t win; + int64_t rc; /* * NOTE: We only care PCI bus based PE for now. For PCI @@ -2770,23 +2772,9 @@ static void pnv_ioda_setup_pe_seg(struct pci_controller *hose, if (res->flags & IORESOURCE_IO) { region.start = res->start - phb->ioda.io_pci_base; region.end = res->end - phb->ioda.io_pci_base; - index = region.start / phb->ioda.io_segsize; - - while (index < phb->ioda.total_pe_num && - region.start <= region.end) { - phb->ioda.io_segmap[index] = pe->pe_number; - rc = opal_pci_map_pe_mmio_window(phb->opal_id, - pe->pe_number, OPAL_IO_WINDOW_TYPE, 0, index); - if (rc != OPAL_SUCCESS) { - pr_err("%s: OPAL error %d when mapping IO " - "segment #%d to PE#%d\n", - __func__, rc, index, pe->pe_number); - break; - } - - region.start += phb->ioda.io_segsize; - index++; - } + segsize = phb->ioda.io_segsize; + segmap = phb->ioda.io_segmap; + win = OPAL_IO_WINDOW_TYPE; } else if ((res->flags & IORESOURCE_MEM) && !pnv_pci_is_mem_pref_64(res->flags)) { region.start = res->start - @@ -2795,23 +2783,29 @@ static void pnv_ioda_setup_pe_seg(struct pci_controller *hose, region.end = res->end - hose->mem_offset[0] - phb->ioda.m32_pci_base; - index = region.start / phb->ioda.m32_segsize; - - while (index < phb->ioda.total_pe_num && - region.start <= region.end) { - phb->ioda.m32_segmap[index] = pe->pe_number; - rc = opal_pci_map_pe_mmio_window(phb->opal_id, - pe->pe_number, OPAL_M32_WINDOW_TYPE, 0, index); - if (rc != OPAL_SUCCESS) { - pr_err("%s: OPAL error %d when mapping M32 " - "segment#%d to PE#%d", - __func__, rc, index, pe->pe_number); - break; - } + segsize = phb->ioda.m32_segsize; + segmap = phb->ioda.m32_segmap; + win = OPAL_M32_WINDOW_TYPE; + } else { + continue; + } - region.start += phb->ioda.m32_segsize; - index++; + index = region.start / segsize; + while (index < phb->ioda.total_pe_num && + region.start <= region.end) { + segmap[index] = pe->pe_number; + rc = opal_pci_map_pe_mmio_window(phb->opal_id, + pe->pe_number, win, 0, index); + if (rc != OPAL_SUCCESS) { + pr_warn("%s: Error %lld mapping (%d) seg#%d to PHB#%d-PE#%d\n", + __func__, rc, win, index, + pe->phb->hose->global_number, + pe->pe_number); + break; } + + region.start += segsize; + index++; } } }
The original implementation of pnv_ioda_setup_pe_seg() configures IO and M32 segments by separate logics, which can be merged by by caching @segmap, @seg_size, @win in advance. This shouldn't cause any behavioural changes. Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> --- arch/powerpc/platforms/powernv/pci-ioda.c | 62 ++++++++++++++----------------- 1 file changed, 28 insertions(+), 34 deletions(-)