[U-Boot] cmd_pci: Check for VendorID earlier

Hi Bin,

On Thu, Dec 31, 2015 at 7:32 AM, Bin Meng <bmeng.cn@gmail.com> wrote:

> 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.

Do you mean like this for imx?


Regards,

Fabio Estevam

Hi Fabio,

On Thu, Dec 31, 2015 at 11:20 PM, Fabio Estevam <festevam@gmail.com> wrote:
> Hi Bin,
>
> On Thu, Dec 31, 2015 at 7:32 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
>
>> 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.
>
> Do you mean like this for imx?
>

Yes

> --- a/drivers/pci/pcie_imx.c
> +++ b/drivers/pci/pcie_imx.c
> @@ -381,7 +381,7 @@ static int imx_pcie_read_config(struct pci_controller *hose,
>         ret = imx_pcie_addr_valid(d);
>         if (ret) {
>                 *val = 0xffffffff;
> -               return ret;
> +               return 0;
>         }
>
>         va_address = get_bus_address(d, where);
>
> and like this for layerscape:
>

Yes

> --- a/drivers/pci/pcie_layerscape.c
> +++ b/drivers/pci/pcie_layerscape.c
> @@ -314,7 +314,7 @@ static int ls_pcie_read_config(struct pci_controller *hose,
>
>         if (ls_pcie_addr_valid(hose, d)) {
>                 *val = 0xffffffff;
> -               return -EINVAL;
> +               return 0;
>         }
>
>         if (PCI_BUS(d) == hose->first_busno)
>

Again, I was wondering why we created two drivers for the same (or
similar) PCIe IPs.

Regards,
Bin

On Mon, Jan 04, 2016 at 11:11:18AM +0800, Bin Meng wrote:
> Hi Fabio,
> 
> On Thu, Dec 31, 2015 at 11:20 PM, Fabio Estevam <festevam@gmail.com> wrote:
> > Hi Bin,
> >
> > On Thu, Dec 31, 2015 at 7:32 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> >> 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.
> >
> > Do you mean like this for imx?
> >
> 
> Yes
> 
> > --- a/drivers/pci/pcie_imx.c
> > +++ b/drivers/pci/pcie_imx.c
> > @@ -381,7 +381,7 @@ static int imx_pcie_read_config(struct pci_controller *hose,
> >         ret = imx_pcie_addr_valid(d);
> >         if (ret) {
> >                 *val = 0xffffffff;
> > -               return ret;
> > +               return 0;
> >         }
> >
> >         va_address = get_bus_address(d, where);
> >
> > and like this for layerscape:
> >
> 
> Yes
> 
> > --- a/drivers/pci/pcie_layerscape.c
> > +++ b/drivers/pci/pcie_layerscape.c
> > @@ -314,7 +314,7 @@ static int ls_pcie_read_config(struct pci_controller *hose,
> >
> >         if (ls_pcie_addr_valid(hose, d)) {
> >                 *val = 0xffffffff;
> > -               return -EINVAL;
> > +               return 0;
> >         }
> >
> >         if (PCI_BUS(d) == hose->first_busno)
> >
> 
> Again, I was wondering why we created two drivers for the same (or
> similar) PCIe IPs.

Something to consolidate for the next release it sounds like.  However
we need this fixed this release yes?  Can I get a v2 of this patch with
a proper commit message?  Thanks!

On Mon, Jan 4, 2016 at 2:22 PM, Tom Rini <trini@konsulko.com> wrote:

> Something to consolidate for the next release it sounds like.  However

Agreed.

> we need this fixed this release yes?  Can I get a v2 of this patch with
> a proper commit message?  Thanks!

Yes, will send it today.

Hi Bin,

On Thu, Dec 31, 2015 at 1:20 PM, Fabio Estevam <festevam@gmail.com> wrote:

> Do you mean like this for imx?
>
> --- a/drivers/pci/pcie_imx.c
> +++ b/drivers/pci/pcie_imx.c
> @@ -381,7 +381,7 @@ static int imx_pcie_read_config(struct pci_controller *hose,
>         ret = imx_pcie_addr_valid(d);
>         if (ret) {
>                 *val = 0xffffffff;
> -               return ret;
> +               return 0;

Thinking a bit more about this: shouldn't we fix imx_pcie_addr_valid() instead?

I am not sure if the above change is the correct fix. At least I am
not able to prepare a convincing commit log for it :-)

On Tue, Jan 5, 2016 at 9:58 PM, Fabio Estevam <festevam@gmail.com> wrote:
> Hi Bin,
>
> On Thu, Dec 31, 2015 at 1:20 PM, Fabio Estevam <festevam@gmail.com> wrote:
>
>> Do you mean like this for imx?
>>
>> --- a/drivers/pci/pcie_imx.c
>> +++ b/drivers/pci/pcie_imx.c
>> @@ -381,7 +381,7 @@ static int imx_pcie_read_config(struct pci_controller *hose,
>>         ret = imx_pcie_addr_valid(d);
>>         if (ret) {
>>                 *val = 0xffffffff;
>> -               return ret;
>> +               return 0;
>
> Thinking a bit more about this: shouldn't we fix imx_pcie_addr_valid() instead?
>
> I am not sure if the above change is the correct fix. At least I am
> not able to prepare a convincing commit log for it :-)

Ok, looked at the kernel PCI designware driver and will send a patch
series that fixes this issue by using the approach done in the kernel.