Re: [PATCH] PCI: iproc: Fix BCMA PCIe bus scanning regression

[Ray Jui]

Hi Bjorn,

On 1/26/2016 10:22 AM, Bjorn Helgaas wrote:
> Hi Ray,
>
> On Wed, Jan 20, 2016 at 02:55:10PM -0800, Ray Jui wrote:
> > Commit 943ebae781f5 ("PCI: iproc: Add PAXC interface support") causes
> > regression on EP device detection on BCMA based platforms. This patch
> > fixes the issue by allowing multiple devices to be configured on the
> > same bus, for all PAXB based child buses
> >
> > Reported-by: Rafal Milecki <zajec5@xxxxxxxxx>
> > Fixes: 943ebae781f5 ("PCI: iproc: Add PAXC interface support")
> > Signed-off-by: Ray Jui <rjui@xxxxxxxxxxxx>
> > ---
> >   drivers/pci/host/pcie-iproc.c | 5 +++--
> >   1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
> > index 5816bce..4627561 100644
> > --- a/drivers/pci/host/pcie-iproc.c
> > +++ b/drivers/pci/host/pcie-iproc.c
> > @@ -171,10 +171,11 @@ static inline void iproc_pcie_ob_write(struct iproc_pcie *pcie,
> >   }
> >
> >   static inline bool iproc_pcie_device_is_valid(struct iproc_pcie *pcie,
> > +					      unsigned int busnum,
> >   					      unsigned int slot,
> >   					      unsigned int fn)
> >   {
> > -	if (slot > 0)
> > +	if ((pcie->type == IPROC_PCIE_PAXC || busnum == 0) && slot > 0)
> >   		return false;
> >
> >   	/* PAXC can only support limited number of functions */
>
> I don't understand this.  Here's the whole function (with this patch
> applied):
>
>      static inline bool iproc_pcie_device_is_valid(struct iproc_pcie *pcie,
> 						  unsigned int busnum,
> 						  unsigned int slot,
> 						  unsigned int fn)
>      {
> 	    if ((pcie->type == IPROC_PCIE_PAXC || busnum == 0) && slot > 0)
> 		    return false;
>
> 	    /* PAXC can only support limited number of functions */
> 	    if (pcie->type == IPROC_PCIE_PAXC && fn >= MAX_NUM_PAXC_PF)
> 		    return false;
>
> 	    return true;
>      }
>
> This says:
>
>    - On bus 00, device 0 is the only valid device.  That seems
>      plausible because the devices on bus 00 are probably built-in to
>      the SoC.
>
>    - On PAXC-based systems, device 0 is the only valid device on *any*
>      bus.  Is that really true?  If there's any way to add a plug-in
>      card, this seems overly restrictive.

Yah, PAXC is connected with one internal device within the SoC. There's 
no connection brought out of the chip.

>      PCIe devices are generally all device 0, but this would mean you
>      cannot plug in a PCIe-to-PCI bridge leading to a PCI device with a
>      non-zero device number.
>
>      I think it also means you could not plug in a PCIe device with ARI
>      enabled, because I think we store the upper 5 bits of the 8-bit
>      ARI function number in the PCI_SLOT bits.
>
>    - On PAXC-based systems, only functions 0, 1, 2, and 3 are valid
>      anywhere in the hierarchy.  I think this again restricts what what
>      cards can be plugged in.

Yes, the internal device connected to PAXC supports 4 physical functions.

> If iProc only supports devices built directly into the SoC, maybe
> these constraints are valid.  But if it supports any plugin or
> external devices, they don't seem to make sense.

Correct. PAXC only connects to one built-in device, while PAXB can 
support external EP devices.

> Also, is it the case that an iProc root bus is always bus number zero?
> That's certainly not the case for many other host controllers, but
> maybe you only have one possible host controller per system and the
> base number is not programmable.

An iProc based SoC can potentially have multiple root complexes, with 
each of them resides on separate PCIe domain (and always on bus 0). I 
think this is similar to how Exynos PCIe host controller is modeled.

> Bjorn

Thanks,

Ray