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 <gws...@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
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
