Hi Gavin,
Sorry to have taken so long to resume these reviews!
> 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/)?
> 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?
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.
> - /*
> - * 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;
> + } else {
> + return 0;
The return codes are currently unused, but should this get a more
informative return code? Are there any invalid ones that should be
flagged, or is it just safe to ignore stuff we don't recognise?
> + }
> +static void pnv_ioda_setup_pe_seg(struct pnv_ioda_pe *pe)
> +{
> + struct pci_dev *pdev;
> + struct resource *res;
> + int i;
> +
> + /* This function only works for bus dependent PE */
> + WARN_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(pe, res))
> + return;
As I mentioned earlier, setup_one_res can potentially return -EIO:
should we be trying to propagate that up?
> + }
> +
> + /*
> + * If the PE contains all subordinate PCI buses, the
> + * windows of the child bridges should be mapped to
> + * the PE as well.
> + */
> + if (!(pe->flags & PNV_IODA_PE_BUS_ALL && pci_is_bridge(pdev)))
> + 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(pe, res))
> + return;
> }
> }
> }
Regards,
Daniel
Attachment:
signature.asc
Description: PGP signature
