[PATCH v7 11/50] powerpc/powernv: IO and M32 mapping based on PCI device resources

[Alexey Kardashevskiy]

On 11/12/2015 03:55 PM, Gavin Shan wrote:
> On Thu, Nov 12, 2015 at 02:30:27PM +1100, Daniel Axtens wrote:
>> Hi Gavin,
>>
>> Sorry to have taken so long to resume these reviews!
>>
>
> Thanks for your review, Daniel!
>
>>> Currently, the IO and M32 segments are mapped to the corresponding
>>> PE based on the windows of the parent bridge of PE's primary bus.
>>> It's not going to work when the windows of root port or upstream
>>> port of the PCIe switch behind root port are extended to PHB's
>>> aperatuses in order to support hotplug in subsequent patch.
>> I'm not _entirely_ sure I understand this.
>>
>> I *think* you mean PHB's apertures (i.e. s/aperatuses/apertures/)?
>>
>
> I'll fix the typo in next revision.
>
>>> This fixes the issue by mapping IO and M32 segments based on the
>>> resources of the PCI devices included in the PE, instead of the
>>> windows of the parent bridge of the PE's primary bus.
>>
>> This solution seems to make a lot of sense, but I don't have a very good
>> understanding of PCI yet: why was it done that way and not this way
>> originally? Looking at the code, it looks like the old way was simple
>> but didn't support SR-IOV?
>>
>
> It's not related to SRIOV. Originally, the IO or M32 segments are mapped
> according to the bridge's windows.


Sorry, I do not understand what this means...


> The bridge windows on root port or the
> upstream port of the switch behind that will be extended to PHB's apertures.

What does "extended" mean here and why would the windows be extended anyway?


> If we still use bridge's windows, all IO and M32 resources are mapped/assigned
> to the PE corresponding to PCI bus#1 or PCI bus#2. That's not correct any more.
> So the correct way is to do the mapping based on IO or M32 BARs of the devices
> included in the PE.

In this patch I see quite a lot of code movements and I fail to spot the 
actual change here...


It used to be a single loop:

pci_bus_for_each_resource(pe->pbus, res, i) {
	/* do stuff for each @res */
}

and now it is 2 loops inside another loop:

list_for_each_entry(pdev, &pe->pbus->devices, bus_list) {
	for (i = 0; i <= PCI_ROM_RESOURCE; i++) {
		res = &pdev->resource[i];
		/* do stuff for each @res */
	}

	for (i = 0; i <= PCI_BRIDGE_RESOURCE_NUM; i++) {
	        res = &pdev->resource[PCI_BRIDGE_RESOURCES + i];
		/* do stuff for each @res */
	}
}


Is that correct? If yes, why is not the patch as simple as this? If no, 
what did I miss?



>
>> There are a few comments inline as well.
>>
>>> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
>>> index 553d3f3..4ab93f8 100644
>>> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
>>> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
>>> @@ -2741,71 +2741,90 @@ truncate_iov:
>>>   }
>>>   #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 pnv_ioda_pe *pe,
>>> +				  struct resource *res)
>>>   {
>>> -	struct pnv_phb *phb = hose->private_data;
>>> +	struct pnv_phb *phb = pe->phb;
>>>   	struct pci_bus_region region;
>>> -	struct resource *res;
>>> -	unsigned int segsize;
>>> -	int *segmap, index, i;
>>> +	unsigned int index, segsize;
>>> +	int *segmap;
>>>   	uint16_t win;
>>>   	int64_t rc;
>>
>> s/int64_t/s64/;
>> I think we might also want to change the uint16_t as well.
>>
>
> As I explained before, I changed it from s64 to int64_t and I won't change it
> back since both of them are fine. Same situation to uint16 here. If we really
> want to clean it all at once, I can do that later, but not in this patchset.
>
>>> -	/*
>>> -	 * 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)));
>>> +	if (!res->parent || !res->flags || res->start > res->end)
>>> +		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;
>>> +		win          = OPAL_IO_WINDOW_TYPE;
>>> +	} else if ((res->flags & IORESOURCE_MEM) &&
>>> +		   !pnv_pci_is_mem_pref_64(res->flags)) {
>>> +		region.start = res->start -
>>> +			       phb->hose->mem_offset[0] -
>>> +			       phb->ioda.m32_pci_base;
>>> +		region.end   = res->end -
>>> +			       phb->hose->mem_offset[0] -
>>> +			       phb->ioda.m32_pci_base;
>>> +		segsize      = phb->ioda.m32_segsize;
>>> +		segmap       = phb->ioda.m32_segmap;
>>> +		win          = OPAL_M32_WINDOW_TYPE;



This code asks for a helper function

pnv_ioda_do_setup_one_res(start, end, segsize, segmap, win)

and then you won't need many local variables (region, segsize, segmap, win) ;)





-- 
Alexey