[U-Boot] cmd_pci: Check for VendorID earlier

On Fri, Oct 9, 2015 at 10:31 AM, Simon Glass <sjg@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?


Regards,

Fabio Estevam

Hi Fabio,

On 9 October 2015 at 14:36, Fabio Estevam <festevam@gmail.com> wrote:
> On Fri, Oct 9, 2015 at 10:31 AM, Simon Glass <sjg@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.

Regards,
Simon

+Alison, York,

Hi,

On Fri, Oct 9, 2015 at 9:44 PM, Simon Glass <sjg@chromium.org> wrote:
> Hi Fabio,
>
> On 9 October 2015 at 14:36, Fabio Estevam <festevam@gmail.com> wrote:
>> On Fri, Oct 9, 2015 at 10:31 AM, Simon Glass <sjg@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