[PATCH v6 10/42] powerpc/powernv: pnv_ioda_setup_dma() configure one PE only

[Gavin Shan]

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 at 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