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

[Gavin Shan]

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.

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