Re: [PATCH v6 07/42] powerpc/powernv: Improve IO and M32 mapping

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 08/11/2015 10:12 AM, Gavin Shan wrote:
> On Mon, Aug 10, 2015 at 05:40:08PM +1000, Alexey Kardashevskiy wrote:
>> On 08/06/2015 02:11 PM, Gavin Shan wrote:
>>> There're 3 windows (IO, M32 and M64) for PHB, root port and upstream
>>
>> These are actually IO, non-prefetchable and prefetchable windows which happen
>> to be IO, 32bit and 64bit windows but this has nothing to do with the M32/M64
>> BAR registers in P7IOC/PHB3, do I understand this correctly?
>>
>
> In pci-ioda.c, we have below definiations that are defined when
> developing the code, not from any specification:
>
> IO  - resources with IO property
> M32 - 32-bits or non-prefetchable resources
> M64 - 64-bits and prefetchable resources


This what I am saying - it is incorrect and confusing. M32/M64 are PHB3 
register names and associated windows (with "M" in the beginning) but not 
device resources.


>>> port of the PCIE switch behind root port. In order to support PCI
>>> hotplug, we extend the start/end address of those 3 windows of root
>>> port or upstream port to the start/end address of the 3 PHB's windows.
>>> The current implementation, assigning IO or M32 segment based on the
>>> bridge's windows, isn't reliable.
>>>
>>> The patch fixes above issue by calculating PE's consumed IO or M32
>>> segments from its contained devices, no PCI bridge windows involved
>>> if the PE doesn't contain all the subordinate PCI buses.
>>
>> Please, rephrase it. How can PCI bridges be involved in PE consumption?
>>
>
> Ok. Will add something like below:
>
> if the PE, corresponding to the PCI bus, doesn't contain all the subordinate
> PCI buses.


No, my question was about "PCI bridge windows involved" - what do you do to 
the windows if PE does not own all child buses?



>>
>>> Otherwise,
>>> the PCI bridge windows still contribute to PE's consumed IO or M32
>>> segments.
>>
>> PCI bridge windows themselves consume PEs? Is that correct?
>>
>
> PCI bridge windows consume IO, M32, M64 segments, not PEs.

Ah, right.


>>>
>>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>>> ---
>>>   arch/powerpc/platforms/powernv/pci-ioda.c | 136 +++++++++++++++++-------------
>>>   1 file changed, 79 insertions(+), 57 deletions(-)
>>>
>>> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
>>> index 488a53e..713f4b4 100644
>>> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
>>> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
>>> @@ -2844,75 +2844,97 @@ static void pnv_pci_ioda_fixup_iov_resources(struct pci_dev *pdev)
>>>   }
>>>   #endif /* CONFIG_PCI_IOV */
>>>
>>> -/*
>>> - * This function is supposed to be called on basis of PE from top
>>> - * to bottom style. So the the I/O or MMIO segment assigned to
>>> - * parent PE could be overrided by its child PEs if necessary.
>>> - */
>>> -static void pnv_ioda_setup_pe_seg(struct pci_controller *hose,
>>> -				  struct pnv_ioda_pe *pe)
>>> +static int pnv_ioda_setup_one_res(struct pci_controller *hose,
>>> +				  struct pnv_ioda_pe *pe,
>>> +				  struct resource *res)
>>>   {
>>>   	struct pnv_phb *phb = hose->private_data;
>>>   	struct pci_bus_region region;
>>> -	struct resource *res;
>>> -	int i, index;
>>> -	unsigned int segsize;
>>> +	unsigned int index, segsize;
>>>   	unsigned long *segmap, *pe_segmap;
>>>   	uint16_t win;
>>>   	int64_t rc;
>>>
>>> -	/*
>>> -	 * NOTE: We only care PCI bus based PE for now. For PCI
>>> -	 * device based PE, for example SRIOV sensitive VF should
>>> -	 * be figured out later.
>>> -	 */
>>> -	BUG_ON(!(pe->flags & (PNV_IODA_PE_BUS | PNV_IODA_PE_BUS_ALL)));
>>> +	/* Check if we need map the resource */
>>> +	if (!res->parent || !res->flags || res->start > res->end)
>>
>> res->start >= res->end ?
>>
>
> No, res->start == res->end is valid.
>
>>
>>> +		return 0;
>>>
>>> -	pci_bus_for_each_resource(pe->pbus, res, i) {
>>> -		if (!res || !res->flags ||
>>> -		    res->start > res->end)
>>> -			continue;
>>> +	if (res->flags & IORESOURCE_IO) {
>>> +		region.start = res->start - phb->ioda.io_pci_base;
>>> +		region.end   = res->end - phb->ioda.io_pci_base;
>>> +		segsize      = phb->ioda.io_segsize;
>>> +		segmap       = phb->ioda.io_segmap;
>>> +		pe_segmap    = pe->io_segmap;
>>> +		win          = OPAL_IO_WINDOW_TYPE;
>>> +	} else if ((res->flags & IORESOURCE_MEM) &&
>>> +		   !pnv_pci_is_mem_pref_64(res->flags)) {
>>> +		region.start = res->start -
>>> +			       hose->mem_offset[0] -
>>> +			       phb->ioda.m32_pci_base;
>>> +		region.end   = res->end -
>>> +			       hose->mem_offset[0] -
>>> +			       phb->ioda.m32_pci_base;
>>> +		segsize      = phb->ioda.m32_segsize;
>>> +		segmap       = phb->ioda.m32_segmap;
>>> +		pe_segmap    = pe->m32_segmap;
>>> +		win          = OPAL_M32_WINDOW_TYPE;
>>> +	} else {
>>> +		return 0;
>>> +	}
>>>
>>> -		if (res->flags & IORESOURCE_IO) {
>>> -			region.start = res->start - phb->ioda.io_pci_base;
>>> -			region.end   = res->end - phb->ioda.io_pci_base;
>>> -			segsize      = phb->ioda.io_segsize;
>>> -			segmap       = phb->ioda.io_segmap;
>>> -			pe_segmap    = pe->io_segmap;
>>> -			win          = OPAL_IO_WINDOW_TYPE;
>>> -		} else if ((res->flags & IORESOURCE_MEM) &&
>>> -			   !pnv_pci_is_mem_pref_64(res->flags)) {
>>> -			region.start = res->start -
>>> -				       hose->mem_offset[0] -
>>> -				       phb->ioda.m32_pci_base;
>>> -			region.end   = res->end -
>>> -				       hose->mem_offset[0] -
>>> -				       phb->ioda.m32_pci_base;
>>> -			segsize      = phb->ioda.m32_segsize;
>>> -			segmap       = phb->ioda.m32_segmap;
>>> -			pe_segmap    = pe->m32_segmap;
>>> -			win          = OPAL_M32_WINDOW_TYPE;
>>> -		} else {
>>> -			continue;
>>> +	region.start = _ALIGN_DOWN(region.start, segsize);
>>> +	region.end   = _ALIGN_UP(region.end, segsize);
>>> +	index = region.start / segsize;
>>> +	while (index < phb->ioda.total_pe &&
>>> +	       region.start < region.end) {
>>> +		rc = opal_pci_map_pe_mmio_window(phb->opal_id,
>>> +				pe->pe_number, win, 0, index);
>>> +		if (rc != OPAL_SUCCESS) {
>>> +			pr_warn("%s: Error %lld mapping (%d) seg#%d to PHB#%d-PE#%d\n",
>>> +				__func__, rc, win, index,
>>> +				pe->phb->hose->global_number,
>>> +				pe->pe_number);
>>> +			return -EIO;
>>>   		}
>>>
>>> -		index = region.start / phb->ioda.io_segsize;
>>> -		while (index < phb->ioda.total_pe &&
>>> -		       region.start <= region.end) {
>>> -			set_bit(index, segmap);
>>> -			set_bit(index, pe_segmap);
>>> -			rc = opal_pci_map_pe_mmio_window(phb->opal_id,
>>> -					pe->pe_number, win, 0, index);
>>> -			if (rc != OPAL_SUCCESS) {
>>> -				pr_warn("%s: Error %lld mapping (%d) seg#%d to PHB#%d-PE#%d\n",
>>> -					__func__, rc, win, index,
>>> -					pe->phb->hose->global_number,
>>> -					pe->pe_number);
>>> -				break;
>>> -			}
>>> +		set_bit(index, segmap);
>>> +		set_bit(index, pe_segmap);
>>> +		region.start += segsize;
>>> +		index++;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static void pnv_ioda_setup_pe_seg(struct pci_controller *hose,
>>> +				  struct pnv_ioda_pe *pe)
>>> +{
>>> +	struct pci_dev *pdev;
>>> +	struct resource *res;
>>> +	int i;
>>> +
>>> +	/* This function only works for bus dependent PE */
>>> +	BUG_ON(!(pe->flags & (PNV_IODA_PE_BUS | PNV_IODA_PE_BUS_ALL)));
>>> +
>>> +	list_for_each_entry(pdev, &pe->pbus->devices, bus_list) {
>>> +		for (i = 0; i <= PCI_ROM_RESOURCE; i++) {
>>> +			res = &pdev->resource[i];
>>> +			if (pnv_ioda_setup_one_res(hose, pe, res))
>>> +				return;
>>> +		}
>>> +
>>> +		/* If the PE contains all subordinate PCI buses, the
>>> +		 * resources of the child bridges should be mapped
>>> +		 * to the PE as well.
>>> +		 */
>>> +		if (!(pe->flags & PNV_IODA_PE_BUS_ALL) ||
>>> +		    (pdev->class >> 8) != PCI_CLASS_BRIDGE_PCI)
>>> +			continue;
>>>
>>> -			region.start += segsize;
>>> -			index++;
>>> +		for (i = 0; i <= PCI_BRIDGE_RESOURCE_NUM; i++) {
>>> +			res = &pdev->resource[PCI_BRIDGE_RESOURCES + i];
>>> +			if (pnv_ioda_setup_one_res(hose, pe, res))
>>> +				return;
>>>   		}
>>>   	}
>>>   }
>>>
>
> Thanks,
> Gavin
>


-- 
Alexey