| Message ID | 1453330510-21926-1-git-send-email-rjui@broadcom.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
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@gmail.com> > Fixes: 943ebae781f5 ("PCI: iproc: Add PAXC interface support") > Signed-off-by: Ray Jui <rjui@broadcom.com> > --- > 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. 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. 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. 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. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@gmail.com> >> Fixes: 943ebae781f5 ("PCI: iproc: Add PAXC interface support") >> Signed-off-by: Ray Jui <rjui@broadcom.com> >> --- >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jan 26, 2016 at 10:37:20AM -0800, Ray Jui wrote: > 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@gmail.com> > >>Fixes: 943ebae781f5 ("PCI: iproc: Add PAXC interface support") > >>Signed-off-by: Ray Jui <rjui@broadcom.com> > >>--- > >> 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. OK, thanks for confirming all that. Something looks wrong in iproc_pcie_map_cfg_bus(). iproc_pcie_device_is_valid() returns true for device 00:00.1, but the "busno == 0" case in iproc_pcie_map_cfg_bus() doesn't use "fn". So the function number is ignored? That would mean there's no difference between 000:00.0, 00:00.1, 00:00.2, 00:00.3, etc. I think this would be clearer and less error-prone if iproc_pcie_device_is_valid() were folded directly into iproc_pcie_map_cfg_bus(). Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 1/26/2016 1:54 PM, Bjorn Helgaas wrote: > On Tue, Jan 26, 2016 at 10:37:20AM -0800, Ray Jui wrote: >> 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@gmail.com> >>>> Fixes: 943ebae781f5 ("PCI: iproc: Add PAXC interface support") >>>> Signed-off-by: Ray Jui <rjui@broadcom.com> >>>> --- >>>> 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. > > OK, thanks for confirming all that. > > Something looks wrong in iproc_pcie_map_cfg_bus(). > iproc_pcie_device_is_valid() returns true for device 00:00.1, > but the "busno == 0" case in iproc_pcie_map_cfg_bus() doesn't > use "fn". So the function number is ignored? That would mean > there's no difference between 000:00.0, 00:00.1, 00:00.2, > 00:00.3, etc. Okay, I should add a check to make sure only function zero is accepted on bus 0. > > I think this would be clearer and less error-prone if > iproc_pcie_device_is_valid() were folded directly into > iproc_pcie_map_cfg_bus(). Okay. I'll get rid of 'iproc_pcie_device_is_valid' and fold all of these check into 'iproc_pcie_map_cfg_bus' and then send out patch v2 for review > Bjorn > Thanks! Ray -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 */ @@ -199,7 +200,7 @@ static void __iomem *iproc_pcie_map_cfg_bus(struct pci_bus *bus, u32 val; u16 offset; - if (!iproc_pcie_device_is_valid(pcie, slot, fn)) + if (!iproc_pcie_device_is_valid(pcie, busno, slot, fn)) return NULL; /* root complex access */
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@gmail.com> Fixes: 943ebae781f5 ("PCI: iproc: Add PAXC interface support") Signed-off-by: Ray Jui <rjui@broadcom.com> --- drivers/pci/host/pcie-iproc.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)