| Message ID | 1438834307-26960-11-git-send-email-gwshan@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On 08/06/2015 02:11 PM, Gavin Shan wrote: > The original implementation of pnv_ioda_setup_dma() iterates the > list of PEs and configures the DMA32 space for them one by one. > The function was designed to be called during PHB fixup time. > When configuring PE's DMA32 space in pcibios_setup_bridge(), in > order to support PCI hotplug, we have to have the function PE > oriented. > > This renames pnv_ioda_setup_dma() to pnv_ioda1_setup_dma() and > adds one more argument "struct pnv_ioda_pe *pe" to it. The caller, > pnv_pci_ioda_setup_DMA(), gets PE from the list and passes to it > or pnv_pci_ioda2_setup_dma_pe(). The patch shouldn't cause behavioral > changes. > > Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> > --- > arch/powerpc/platforms/powernv/pci-ioda.c | 75 +++++++++++++++---------------- > 1 file changed, 36 insertions(+), 39 deletions(-) > > diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c > index 8456f37..cd22002 100644 > --- a/arch/powerpc/platforms/powernv/pci-ioda.c > +++ b/arch/powerpc/platforms/powernv/pci-ioda.c > @@ -2443,52 +2443,29 @@ static void pnv_pci_ioda2_setup_dma_pe(struct pnv_phb *phb, > pnv_ioda_setup_bus_dma(pe, pe->pbus); > } > > -static void pnv_ioda_setup_dma(struct pnv_phb *phb) > +static unsigned int pnv_ioda1_setup_dma(struct pnv_phb *phb, > + struct pnv_ioda_pe *pe, > + unsigned int base) > { > struct pci_controller *hose = phb->hose; > - struct pnv_ioda_pe *pe; > - unsigned int dma_weight; > + unsigned int dma_weight, segs; > > /* Calculate the PHB's DMA weight */ > dma_weight = pnv_ioda_phb_dma_weight(phb); > pr_info("PCI%04x has %ld DMA32 segments, total weight %d\n", > hose->global_number, phb->ioda.dma32_segcount, dma_weight); > > - pnv_pci_ioda_setup_opal_tce_kill(phb); > - > - /* Walk our PE list and configure their DMA segments, hand them > - * out one base segment plus any residual segments based on > - * weight > - */ > - list_for_each_entry(pe, &phb->ioda.pe_dma_list, dma_link) { > - if (!pe->dma32_weight) > - continue; > - > - /* > - * For IODA2 compliant PHB3, we needn't care about the weight. > - * The all available 32-bits DMA space will be assigned to > - * the specific PE. > - */ > - if (phb->type == PNV_PHB_IODA1) { > - unsigned int segs, base = 0; > - > - if (pe->dma32_weight < > - dma_weight / phb->ioda.dma32_segcount) > - segs = 1; > - else > - segs = (pe->dma32_weight * > - phb->ioda.dma32_segcount) / dma_weight; > - > - pe_info(pe, "DMA32 weight %d, assigned %d segments\n", > - pe->dma32_weight, segs); > - pnv_pci_ioda_setup_dma_pe(phb, pe, base, segs); > + if (pe->dma32_weight < > + dma_weight / phb->ioda.dma32_segcount) Can be one line now. > + segs = 1; > + else > + segs = (pe->dma32_weight * > + phb->ioda.dma32_segcount) / dma_weight; > + pe_info(pe, "DMA weight %d, assigned %d segments\n", > + pe->dma32_weight, segs); > + pnv_pci_ioda_setup_dma_pe(phb, pe, base, segs); Why not to merge pnv_ioda1_setup_dma() to pnv_pci_ioda_setup_dma_pe()? > > - base += segs; > - } else { > - pe_info(pe, "Assign DMA32 space\n"); > - pnv_pci_ioda2_setup_dma_pe(phb, pe); > - } > - } > + return segs; > } > > #ifdef CONFIG_PCI_MSI > @@ -2955,12 +2932,32 @@ static void pnv_pci_ioda_setup_DMA(void) > { > struct pci_controller *hose, *tmp; > struct pnv_phb *phb; > + struct pnv_ioda_pe *pe; > + unsigned int base; > > list_for_each_entry_safe(hose, tmp, &hose_list, list_node) { > - pnv_ioda_setup_dma(hose->private_data); > + phb = hose->private_data; > + pnv_pci_ioda_setup_opal_tce_kill(phb); > + > + base = 0; > + list_for_each_entry(pe, &phb->ioda.pe_dma_list, dma_link) { > + if (!pe->dma32_weight) > + continue; > + > + switch (phb->type) { > + case PNV_PHB_IODA1: > + base += pnv_ioda1_setup_dma(phb, pe, base); This @base handling seems never be tested between 8..11 as "[PATCH v6 11/42] powerpc/powernv: Trace DMA32 segments consumed by PE" removes it and I suspect you only tested the final version. Which is ok for the final result but not ok for bisectability. Looks like 8/42, 9/42, 10/42, 11/42 need to be rearranged or merged to remove this multiple @base touching. > + break; > + case PNV_PHB_IODA2: > + pnv_pci_ioda2_setup_dma_pe(phb, pe); > + break; > + default: > + pr_warn("%s: No DMA for PHB type %d\n", > + __func__, phb->type); > + } > + } > > /* Mark the PHB initialization done */ > - phb = hose->private_data; > phb->initialized = 1; > } > } >
On Mon, Aug 10, 2015 at 07:31:11PM +1000, Alexey Kardashevskiy wrote: >On 08/06/2015 02:11 PM, Gavin Shan wrote: >>The original implementation of pnv_ioda_setup_dma() iterates the >>list of PEs and configures the DMA32 space for them one by one. >>The function was designed to be called during PHB fixup time. >>When configuring PE's DMA32 space in pcibios_setup_bridge(), in >>order to support PCI hotplug, we have to have the function PE >>oriented. >> >>This renames pnv_ioda_setup_dma() to pnv_ioda1_setup_dma() and >>adds one more argument "struct pnv_ioda_pe *pe" to it. The caller, >>pnv_pci_ioda_setup_DMA(), gets PE from the list and passes to it >>or pnv_pci_ioda2_setup_dma_pe(). The patch shouldn't cause behavioral >>changes. >> >>Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> >>--- >> arch/powerpc/platforms/powernv/pci-ioda.c | 75 +++++++++++++++---------------- >> 1 file changed, 36 insertions(+), 39 deletions(-) >> >>diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c >>index 8456f37..cd22002 100644 >>--- a/arch/powerpc/platforms/powernv/pci-ioda.c >>+++ b/arch/powerpc/platforms/powernv/pci-ioda.c >>@@ -2443,52 +2443,29 @@ static void pnv_pci_ioda2_setup_dma_pe(struct pnv_phb *phb, >> pnv_ioda_setup_bus_dma(pe, pe->pbus); >> } >> >>-static void pnv_ioda_setup_dma(struct pnv_phb *phb) >>+static unsigned int pnv_ioda1_setup_dma(struct pnv_phb *phb, >>+ struct pnv_ioda_pe *pe, >>+ unsigned int base) >> { >> struct pci_controller *hose = phb->hose; >>- struct pnv_ioda_pe *pe; >>- unsigned int dma_weight; >>+ unsigned int dma_weight, segs; >> >> /* Calculate the PHB's DMA weight */ >> dma_weight = pnv_ioda_phb_dma_weight(phb); >> pr_info("PCI%04x has %ld DMA32 segments, total weight %d\n", >> hose->global_number, phb->ioda.dma32_segcount, dma_weight); >> >>- pnv_pci_ioda_setup_opal_tce_kill(phb); >>- >>- /* Walk our PE list and configure their DMA segments, hand them >>- * out one base segment plus any residual segments based on >>- * weight >>- */ >>- list_for_each_entry(pe, &phb->ioda.pe_dma_list, dma_link) { >>- if (!pe->dma32_weight) >>- continue; >>- >>- /* >>- * For IODA2 compliant PHB3, we needn't care about the weight. >>- * The all available 32-bits DMA space will be assigned to >>- * the specific PE. >>- */ >>- if (phb->type == PNV_PHB_IODA1) { >>- unsigned int segs, base = 0; >>- >>- if (pe->dma32_weight < >>- dma_weight / phb->ioda.dma32_segcount) >>- segs = 1; >>- else >>- segs = (pe->dma32_weight * >>- phb->ioda.dma32_segcount) / dma_weight; >>- >>- pe_info(pe, "DMA32 weight %d, assigned %d segments\n", >>- pe->dma32_weight, segs); >>- pnv_pci_ioda_setup_dma_pe(phb, pe, base, segs); >>+ if (pe->dma32_weight < >>+ dma_weight / phb->ioda.dma32_segcount) > >Can be one line now. > Indeed. >>+ segs = 1; >>+ else >>+ segs = (pe->dma32_weight * >>+ phb->ioda.dma32_segcount) / dma_weight; >>+ pe_info(pe, "DMA weight %d, assigned %d segments\n", >>+ pe->dma32_weight, segs); >>+ pnv_pci_ioda_setup_dma_pe(phb, pe, base, segs); > > >Why not to merge pnv_ioda1_setup_dma() to pnv_pci_ioda_setup_dma_pe()? > There're two reasons: - They're separate logically. One is calculating number of DMA32 segments required. Another one is allocate TCE32 tables and configure devices with them. - In PCI hotplug path, I need pnv_ioda1_setup_dma() which has "pe" as parameter. >> >>- base += segs; >>- } else { >>- pe_info(pe, "Assign DMA32 space\n"); >>- pnv_pci_ioda2_setup_dma_pe(phb, pe); >>- } >>- } >>+ return segs; >> } >> >> #ifdef CONFIG_PCI_MSI >>@@ -2955,12 +2932,32 @@ static void pnv_pci_ioda_setup_DMA(void) >> { >> struct pci_controller *hose, *tmp; >> struct pnv_phb *phb; >>+ struct pnv_ioda_pe *pe; >>+ unsigned int base; >> >> list_for_each_entry_safe(hose, tmp, &hose_list, list_node) { >>- pnv_ioda_setup_dma(hose->private_data); >>+ phb = hose->private_data; >>+ pnv_pci_ioda_setup_opal_tce_kill(phb); >>+ >>+ base = 0; >>+ list_for_each_entry(pe, &phb->ioda.pe_dma_list, dma_link) { >>+ if (!pe->dma32_weight) >>+ continue; >>+ >>+ switch (phb->type) { >>+ case PNV_PHB_IODA1: >>+ base += pnv_ioda1_setup_dma(phb, pe, base); > > >This @base handling seems never be tested between 8..11 as "[PATCH v6 11/42] >powerpc/powernv: Trace DMA32 segments consumed by PE" >removes it and I suspect you only tested the final version. Which is ok for >the final result but not ok for bisectability. > >Looks like 8/42, 9/42, 10/42, 11/42 need to be rearranged or merged to remove >this multiple @base touching. > Why ? > >>+ break; >>+ case PNV_PHB_IODA2: >>+ pnv_pci_ioda2_setup_dma_pe(phb, pe); >>+ break; >>+ default: >>+ pr_warn("%s: No DMA for PHB type %d\n", >>+ __func__, phb->type); >>+ } >>+ } >> >> /* Mark the PHB initialization done */ >>- phb = hose->private_data; >> phb->initialized = 1; >> } >> } >> 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
On 08/11/2015 10:29 AM, Gavin Shan wrote: > On Mon, Aug 10, 2015 at 07:31:11PM +1000, Alexey Kardashevskiy wrote: >> On 08/06/2015 02:11 PM, Gavin Shan wrote: >>> The original implementation of pnv_ioda_setup_dma() iterates the >>> list of PEs and configures the DMA32 space for them one by one. >>> The function was designed to be called during PHB fixup time. >>> When configuring PE's DMA32 space in pcibios_setup_bridge(), in >>> order to support PCI hotplug, we have to have the function PE >>> oriented. >>> >>> This renames pnv_ioda_setup_dma() to pnv_ioda1_setup_dma() and >>> adds one more argument "struct pnv_ioda_pe *pe" to it. The caller, >>> pnv_pci_ioda_setup_DMA(), gets PE from the list and passes to it >>> or pnv_pci_ioda2_setup_dma_pe(). The patch shouldn't cause behavioral >>> changes. >>> >>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> >>> --- >>> arch/powerpc/platforms/powernv/pci-ioda.c | 75 +++++++++++++++---------------- >>> 1 file changed, 36 insertions(+), 39 deletions(-) >>> >>> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c >>> index 8456f37..cd22002 100644 >>> --- a/arch/powerpc/platforms/powernv/pci-ioda.c >>> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c >>> @@ -2443,52 +2443,29 @@ static void pnv_pci_ioda2_setup_dma_pe(struct pnv_phb *phb, >>> pnv_ioda_setup_bus_dma(pe, pe->pbus); >>> } >>> >>> -static void pnv_ioda_setup_dma(struct pnv_phb *phb) >>> +static unsigned int pnv_ioda1_setup_dma(struct pnv_phb *phb, >>> + struct pnv_ioda_pe *pe, >>> + unsigned int base) >>> { >>> struct pci_controller *hose = phb->hose; >>> - struct pnv_ioda_pe *pe; >>> - unsigned int dma_weight; >>> + unsigned int dma_weight, segs; >>> >>> /* Calculate the PHB's DMA weight */ >>> dma_weight = pnv_ioda_phb_dma_weight(phb); >>> pr_info("PCI%04x has %ld DMA32 segments, total weight %d\n", >>> hose->global_number, phb->ioda.dma32_segcount, dma_weight); >>> >>> - pnv_pci_ioda_setup_opal_tce_kill(phb); >>> - >>> - /* Walk our PE list and configure their DMA segments, hand them >>> - * out one base segment plus any residual segments based on >>> - * weight >>> - */ >>> - list_for_each_entry(pe, &phb->ioda.pe_dma_list, dma_link) { >>> - if (!pe->dma32_weight) >>> - continue; >>> - >>> - /* >>> - * For IODA2 compliant PHB3, we needn't care about the weight. >>> - * The all available 32-bits DMA space will be assigned to >>> - * the specific PE. >>> - */ >>> - if (phb->type == PNV_PHB_IODA1) { >>> - unsigned int segs, base = 0; >>> - >>> - if (pe->dma32_weight < >>> - dma_weight / phb->ioda.dma32_segcount) >>> - segs = 1; >>> - else >>> - segs = (pe->dma32_weight * >>> - phb->ioda.dma32_segcount) / dma_weight; >>> - >>> - pe_info(pe, "DMA32 weight %d, assigned %d segments\n", >>> - pe->dma32_weight, segs); >>> - pnv_pci_ioda_setup_dma_pe(phb, pe, base, segs); >>> + if (pe->dma32_weight < >>> + dma_weight / phb->ioda.dma32_segcount) >> >> Can be one line now. >> > > Indeed. > >>> + segs = 1; >>> + else >>> + segs = (pe->dma32_weight * >>> + phb->ioda.dma32_segcount) / dma_weight; >>> + pe_info(pe, "DMA weight %d, assigned %d segments\n", >>> + pe->dma32_weight, segs); >>> + pnv_pci_ioda_setup_dma_pe(phb, pe, base, segs); >> >> >> Why not to merge pnv_ioda1_setup_dma() to pnv_pci_ioda_setup_dma_pe()? >> > > There're two reasons: > - They're separate logically. One is calculating number of DMA32 segments required. > Another one is allocate TCE32 tables and configure devices with them. > - In PCI hotplug path, I need pnv_ioda1_setup_dma() which has "pe" as parameter. And hotplug path does not care about dma weight why? > >>> >>> - base += segs; >>> - } else { >>> - pe_info(pe, "Assign DMA32 space\n"); >>> - pnv_pci_ioda2_setup_dma_pe(phb, pe); >>> - } >>> - } >>> + return segs; >>> } >>> >>> #ifdef CONFIG_PCI_MSI >>> @@ -2955,12 +2932,32 @@ static void pnv_pci_ioda_setup_DMA(void) >>> { >>> struct pci_controller *hose, *tmp; >>> struct pnv_phb *phb; >>> + struct pnv_ioda_pe *pe; >>> + unsigned int base; >>> >>> list_for_each_entry_safe(hose, tmp, &hose_list, list_node) { >>> - pnv_ioda_setup_dma(hose->private_data); >>> + phb = hose->private_data; >>> + pnv_pci_ioda_setup_opal_tce_kill(phb); >>> + >>> + base = 0; >>> + list_for_each_entry(pe, &phb->ioda.pe_dma_list, dma_link) { >>> + if (!pe->dma32_weight) >>> + continue; >>> + >>> + switch (phb->type) { >>> + case PNV_PHB_IODA1: >>> + base += pnv_ioda1_setup_dma(phb, pe, base); >> >> >> This @base handling seems never be tested between 8..11 as "[PATCH v6 11/42] >> powerpc/powernv: Trace DMA32 segments consumed by PE" >> removes it and I suspect you only tested the final version. Which is ok for >> the final result but not ok for bisectability. >> >> Looks like 8/42, 9/42, 10/42, 11/42 need to be rearranged or merged to remove >> this multiple @base touching. >> > > Why ? You are touching this @base from 8/42 to 11/12 and in between it is very broken, you only get it fixed (by removing) in 11/42. Read my comment for 8/42. After every single patch in any patchset the functionality should not break but it does in this patchset. > >> >>> + break; >>> + case PNV_PHB_IODA2: >>> + pnv_pci_ioda2_setup_dma_pe(phb, pe); >>> + break; >>> + default: >>> + pr_warn("%s: No DMA for PHB type %d\n", >>> + __func__, phb->type); >>> + } >>> + } >>> >>> /* Mark the PHB initialization done */ >>> - phb = hose->private_data; >>> phb->initialized = 1; >>> } >>> } >>> > > Thanks, > Gavin >
On Tue, Aug 11, 2015 at 12:39:02PM +1000, Alexey Kardashevskiy wrote: >On 08/11/2015 10:29 AM, Gavin Shan wrote: >>On Mon, Aug 10, 2015 at 07:31:11PM +1000, Alexey Kardashevskiy wrote: >>>On 08/06/2015 02:11 PM, Gavin Shan wrote: >>>>The original implementation of pnv_ioda_setup_dma() iterates the >>>>list of PEs and configures the DMA32 space for them one by one. >>>>The function was designed to be called during PHB fixup time. >>>>When configuring PE's DMA32 space in pcibios_setup_bridge(), in >>>>order to support PCI hotplug, we have to have the function PE >>>>oriented. >>>> >>>>This renames pnv_ioda_setup_dma() to pnv_ioda1_setup_dma() and >>>>adds one more argument "struct pnv_ioda_pe *pe" to it. The caller, >>>>pnv_pci_ioda_setup_DMA(), gets PE from the list and passes to it >>>>or pnv_pci_ioda2_setup_dma_pe(). The patch shouldn't cause behavioral >>>>changes. >>>> >>>>Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> >>>>--- >>>> arch/powerpc/platforms/powernv/pci-ioda.c | 75 +++++++++++++++---------------- >>>> 1 file changed, 36 insertions(+), 39 deletions(-) >>>> >>>>diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c >>>>index 8456f37..cd22002 100644 >>>>--- a/arch/powerpc/platforms/powernv/pci-ioda.c >>>>+++ b/arch/powerpc/platforms/powernv/pci-ioda.c >>>>@@ -2443,52 +2443,29 @@ static void pnv_pci_ioda2_setup_dma_pe(struct pnv_phb *phb, >>>> pnv_ioda_setup_bus_dma(pe, pe->pbus); >>>> } >>>> >>>>-static void pnv_ioda_setup_dma(struct pnv_phb *phb) >>>>+static unsigned int pnv_ioda1_setup_dma(struct pnv_phb *phb, >>>>+ struct pnv_ioda_pe *pe, >>>>+ unsigned int base) >>>> { >>>> struct pci_controller *hose = phb->hose; >>>>- struct pnv_ioda_pe *pe; >>>>- unsigned int dma_weight; >>>>+ unsigned int dma_weight, segs; >>>> >>>> /* Calculate the PHB's DMA weight */ >>>> dma_weight = pnv_ioda_phb_dma_weight(phb); >>>> pr_info("PCI%04x has %ld DMA32 segments, total weight %d\n", >>>> hose->global_number, phb->ioda.dma32_segcount, dma_weight); >>>> >>>>- pnv_pci_ioda_setup_opal_tce_kill(phb); >>>>- >>>>- /* Walk our PE list and configure their DMA segments, hand them >>>>- * out one base segment plus any residual segments based on >>>>- * weight >>>>- */ >>>>- list_for_each_entry(pe, &phb->ioda.pe_dma_list, dma_link) { >>>>- if (!pe->dma32_weight) >>>>- continue; >>>>- >>>>- /* >>>>- * For IODA2 compliant PHB3, we needn't care about the weight. >>>>- * The all available 32-bits DMA space will be assigned to >>>>- * the specific PE. >>>>- */ >>>>- if (phb->type == PNV_PHB_IODA1) { >>>>- unsigned int segs, base = 0; >>>>- >>>>- if (pe->dma32_weight < >>>>- dma_weight / phb->ioda.dma32_segcount) >>>>- segs = 1; >>>>- else >>>>- segs = (pe->dma32_weight * >>>>- phb->ioda.dma32_segcount) / dma_weight; >>>>- >>>>- pe_info(pe, "DMA32 weight %d, assigned %d segments\n", >>>>- pe->dma32_weight, segs); >>>>- pnv_pci_ioda_setup_dma_pe(phb, pe, base, segs); >>>>+ if (pe->dma32_weight < >>>>+ dma_weight / phb->ioda.dma32_segcount) >>> >>>Can be one line now. >>> >> >>Indeed. >> >>>>+ segs = 1; >>>>+ else >>>>+ segs = (pe->dma32_weight * >>>>+ phb->ioda.dma32_segcount) / dma_weight; >>>>+ pe_info(pe, "DMA weight %d, assigned %d segments\n", >>>>+ pe->dma32_weight, segs); >>>>+ pnv_pci_ioda_setup_dma_pe(phb, pe, base, segs); >>> >>> >>>Why not to merge pnv_ioda1_setup_dma() to pnv_pci_ioda_setup_dma_pe()? >>> >> >>There're two reasons: >>- They're separate logically. One is calculating number of DMA32 segments required. >> Another one is allocate TCE32 tables and configure devices with them. >>- In PCI hotplug path, I need pnv_ioda1_setup_dma() which has "pe" as parameter. > > >And hotplug path does not care about dma weight why? > PHB3 doesn't care about DMA weight, but P7IOC needs. >> >>>> >>>>- base += segs; >>>>- } else { >>>>- pe_info(pe, "Assign DMA32 space\n"); >>>>- pnv_pci_ioda2_setup_dma_pe(phb, pe); >>>>- } >>>>- } >>>>+ return segs; >>>> } >>>> >>>> #ifdef CONFIG_PCI_MSI >>>>@@ -2955,12 +2932,32 @@ static void pnv_pci_ioda_setup_DMA(void) >>>> { >>>> struct pci_controller *hose, *tmp; >>>> struct pnv_phb *phb; >>>>+ struct pnv_ioda_pe *pe; >>>>+ unsigned int base; >>>> >>>> list_for_each_entry_safe(hose, tmp, &hose_list, list_node) { >>>>- pnv_ioda_setup_dma(hose->private_data); >>>>+ phb = hose->private_data; >>>>+ pnv_pci_ioda_setup_opal_tce_kill(phb); >>>>+ >>>>+ base = 0; >>>>+ list_for_each_entry(pe, &phb->ioda.pe_dma_list, dma_link) { >>>>+ if (!pe->dma32_weight) >>>>+ continue; >>>>+ >>>>+ switch (phb->type) { >>>>+ case PNV_PHB_IODA1: >>>>+ base += pnv_ioda1_setup_dma(phb, pe, base); >>> >>> >>>This @base handling seems never be tested between 8..11 as "[PATCH v6 11/42] >>>powerpc/powernv: Trace DMA32 segments consumed by PE" >>>removes it and I suspect you only tested the final version. Which is ok for >>>the final result but not ok for bisectability. >>> >>>Looks like 8/42, 9/42, 10/42, 11/42 need to be rearranged or merged to remove >>>this multiple @base touching. >>> >> >>Why ? > >You are touching this @base from 8/42 to 11/12 and in between it is very >broken, you only get it fixed (by removing) in 11/42. Read my comment for >8/42. After every single patch in any patchset the functionality should not >break but it does in this patchset. > Please refer the reply to PATCH[8/42] then. > >> >>> >>>>+ break; >>>>+ case PNV_PHB_IODA2: >>>>+ pnv_pci_ioda2_setup_dma_pe(phb, pe); >>>>+ break; >>>>+ default: >>>>+ pr_warn("%s: No DMA for PHB type %d\n", >>>>+ __func__, phb->type); >>>>+ } >>>>+ } >>>> >>>> /* Mark the PHB initialization done */ >>>>- phb = hose->private_data; >>>> phb->initialized = 1; >>>> } >>>> } >>>> >> 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 8456f37..cd22002 100644 --- a/arch/powerpc/platforms/powernv/pci-ioda.c +++ b/arch/powerpc/platforms/powernv/pci-ioda.c @@ -2443,52 +2443,29 @@ static void pnv_pci_ioda2_setup_dma_pe(struct pnv_phb *phb, pnv_ioda_setup_bus_dma(pe, pe->pbus); } -static void pnv_ioda_setup_dma(struct pnv_phb *phb) +static unsigned int pnv_ioda1_setup_dma(struct pnv_phb *phb, + struct pnv_ioda_pe *pe, + unsigned int base) { struct pci_controller *hose = phb->hose; - struct pnv_ioda_pe *pe; - unsigned int dma_weight; + unsigned int dma_weight, segs; /* Calculate the PHB's DMA weight */ dma_weight = pnv_ioda_phb_dma_weight(phb); pr_info("PCI%04x has %ld DMA32 segments, total weight %d\n", hose->global_number, phb->ioda.dma32_segcount, dma_weight); - pnv_pci_ioda_setup_opal_tce_kill(phb); - - /* Walk our PE list and configure their DMA segments, hand them - * out one base segment plus any residual segments based on - * weight - */ - list_for_each_entry(pe, &phb->ioda.pe_dma_list, dma_link) { - if (!pe->dma32_weight) - continue; - - /* - * For IODA2 compliant PHB3, we needn't care about the weight. - * The all available 32-bits DMA space will be assigned to - * the specific PE. - */ - if (phb->type == PNV_PHB_IODA1) { - unsigned int segs, base = 0; - - if (pe->dma32_weight < - dma_weight / phb->ioda.dma32_segcount) - segs = 1; - else - segs = (pe->dma32_weight * - phb->ioda.dma32_segcount) / dma_weight; - - pe_info(pe, "DMA32 weight %d, assigned %d segments\n", - pe->dma32_weight, segs); - pnv_pci_ioda_setup_dma_pe(phb, pe, base, segs); + if (pe->dma32_weight < + dma_weight / phb->ioda.dma32_segcount) + segs = 1; + else + segs = (pe->dma32_weight * + phb->ioda.dma32_segcount) / dma_weight; + pe_info(pe, "DMA weight %d, assigned %d segments\n", + pe->dma32_weight, segs); + pnv_pci_ioda_setup_dma_pe(phb, pe, base, segs); - base += segs; - } else { - pe_info(pe, "Assign DMA32 space\n"); - pnv_pci_ioda2_setup_dma_pe(phb, pe); - } - } + return segs; } #ifdef CONFIG_PCI_MSI @@ -2955,12 +2932,32 @@ static void pnv_pci_ioda_setup_DMA(void) { struct pci_controller *hose, *tmp; struct pnv_phb *phb; + struct pnv_ioda_pe *pe; + unsigned int base; list_for_each_entry_safe(hose, tmp, &hose_list, list_node) { - pnv_ioda_setup_dma(hose->private_data); + phb = hose->private_data; + pnv_pci_ioda_setup_opal_tce_kill(phb); + + base = 0; + list_for_each_entry(pe, &phb->ioda.pe_dma_list, dma_link) { + if (!pe->dma32_weight) + continue; + + switch (phb->type) { + case PNV_PHB_IODA1: + base += pnv_ioda1_setup_dma(phb, pe, base); + break; + case PNV_PHB_IODA2: + pnv_pci_ioda2_setup_dma_pe(phb, pe); + break; + default: + pr_warn("%s: No DMA for PHB type %d\n", + __func__, phb->type); + } + } /* Mark the PHB initialization done */ - phb = hose->private_data; phb->initialized = 1; } }
The original implementation of pnv_ioda_setup_dma() iterates the list of PEs and configures the DMA32 space for them one by one. The function was designed to be called during PHB fixup time. When configuring PE's DMA32 space in pcibios_setup_bridge(), in order to support PCI hotplug, we have to have the function PE oriented. This renames pnv_ioda_setup_dma() to pnv_ioda1_setup_dma() and adds one more argument "struct pnv_ioda_pe *pe" to it. The caller, pnv_pci_ioda_setup_DMA(), gets PE from the list and passes to it or pnv_pci_ioda2_setup_dma_pe(). The patch shouldn't cause behavioral changes. Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> --- arch/powerpc/platforms/powernv/pci-ioda.c | 75 +++++++++++++++---------------- 1 file changed, 36 insertions(+), 39 deletions(-)