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@xxxxxxxxxxxxxxxxxx>
>>>>---
>>>> 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 devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
