[U-Boot] [PATCH] cmd_pci: Check for VendorID earlier

[Bin Meng]

+Alison, York,

Hi,

On Fri, Oct 9, 2015 at 9:44 PM, Simon Glass <sjg at chromium.org> wrote:
> Hi Fabio,
>
> On 9 October 2015 at 14:36, Fabio Estevam <festevam at gmail.com> wrote:
>> On Fri, Oct 9, 2015 at 10:31 AM, Simon Glass <sjg at chromium.org> wrote:
>>
>>> If you look down one more level, these end up calling
>>> imx_pcie_read_config() which calls imx_pcie_addr_valid():
>>>
>>> static int imx_pcie_addr_valid(pci_dev_t d)
>>> {
>>>    if ((PCI_BUS(d) == 0) && (PCI_DEV(d) > 1))
>>>       return -EINVAL;
>>>    if ((PCI_BUS(d) == 1) && (PCI_DEV(d) > 0))
>>>       return -EINVAL;
>>>    return 0;
>>> }
>>>
>>> I can understand the bus check, but why return an access error if the
>>> device does not exist on the bus? That seems like a bug to me.
>>
>> Is your suggestion like this?
>>
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -67,7 +67,7 @@ int pci_hose_read_config_##size##_via_dword(struct pci_control
>>                                                                         \
>>         if (pci_hose_read_config_dword(hose, dev, offset & 0xfc, &val32) < 0) {
>>                 *val = -1;                                              \
>> -               return -1;                                              \
>> +               return 0;                                               \
>>         }                                                               \
>>                                                                         \
>>         *val = (val32 >> ((offset & (int)off_mask) * 8));
>
> Not really.
>
> I don't understand the hardware so I don't know what exactly is wrong.
>
> To me, imx_pcie_addr_valid() should ideally not fail when the device
> number is in range (0..31) but references a missing device. It should
> be possible for its caller (imx_pcie_read_config()) to read from that
> address and get 0xffff as expected.
>
> If that works, then I'd suggest changing to imx_pcie_addr_valid() to
> ignore PCI_DEV(f).
>
> If not, then I'd suggest changing imx_pcie_read_config() to return 0
> even when imx_pcie_addr_valid() does not, and add a comment as to why
> the error is being squashed.
>

I also see this behavior on ls1021atwr board. I agree with Simon, the
correct fix should fix the PCIe driver to return 0 instead of -EINVAL.

BTW: it looks to me that both imx and layerscape PCIe IPs are derived
from Synopsis Designware. Why didn't we create a single driver for
that IP?

Regards,
Bin