lkml.org 
[lkml]   [2016]   [Jan]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] PCI: iproc: Fix BCMA PCIe bus scanning regression
From
Date


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

\
 
 \ /
  Last update: 2016-01-27 00:01    [W:0.326 / U:0.044 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site