[U-Boot] [PATCH] cmd_pci: Check for VendorID earlier
From: Fabio Estevam fabio.estevam@freescale.com
Since commit ff3e077bd2 ("dm: pci: Add a uclass for PCI") the following error message is seen:
=> pci 0 Scanning PCI devices on bus 0 BusDevFun VendorId DeviceId Device Class Sub-Class _____________________________________________________________ 00.01.00 0x16c3 0xabcd Bridge device 0x04 Cannot read bus configuration: -1
=> pci 1 Scanning PCI devices on bus 1 BusDevFun VendorId DeviceId Device Class Sub-Class _____________________________________________________________ 01.00.00 0x8086 0x08b1 Network controller 0x80 Cannot read bus configuration: -1
When we are done scanning the PCI devices pci_read_config_word() will return -1 and VendorID will contain 0xFFFF.
The original code would exit the 'for' loop in this condition.
Keep the same original behaviour by first testing the VendorID value and then checking and propagating the pci_read_config_word() error afterwards.
Signed-off-by: Fabio Estevam fabio.estevam@freescale.com --- common/cmd_pci.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/common/cmd_pci.c b/common/cmd_pci.c index dcecef8..92dc643 100644 --- a/common/cmd_pci.c +++ b/common/cmd_pci.c @@ -77,10 +77,10 @@ void pciinfo(int BusNum, int ShortPCIListing)
ret = pci_read_config_word(dev, PCI_VENDOR_ID, &VendorID); - if (ret) - goto error; if ((VendorID == 0xFFFF) || (VendorID == 0x0000)) continue; + if (ret) + goto error;
if (!Function) pci_read_config_byte(dev, PCI_HEADER_TYPE, &HeaderType);
Hi Fabio,
On 8 October 2015 at 00:37, Fabio Estevam festevam@gmail.com wrote:
From: Fabio Estevam fabio.estevam@freescale.com
Since commit ff3e077bd2 ("dm: pci: Add a uclass for PCI") the following error message is seen:
=> pci 0 Scanning PCI devices on bus 0 BusDevFun VendorId DeviceId Device Class Sub-Class _____________________________________________________________ 00.01.00 0x16c3 0xabcd Bridge device 0x04 Cannot read bus configuration: -1
=> pci 1 Scanning PCI devices on bus 1 BusDevFun VendorId DeviceId Device Class Sub-Class _____________________________________________________________ 01.00.00 0x8086 0x08b1 Network controller 0x80 Cannot read bus configuration: -1
When we are done scanning the PCI devices pci_read_config_word() will return -1 and VendorID will contain 0xFFFF.
The original code would exit the 'for' loop in this condition.
Keep the same original behaviour by first testing the VendorID value and then checking and propagating the pci_read_config_word() error afterwards.
Signed-off-by: Fabio Estevam fabio.estevam@freescale.com
common/cmd_pci.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/common/cmd_pci.c b/common/cmd_pci.c index dcecef8..92dc643 100644 --- a/common/cmd_pci.c +++ b/common/cmd_pci.c @@ -77,10 +77,10 @@ void pciinfo(int BusNum, int ShortPCIListing)
ret = pci_read_config_word(dev, PCI_VENDOR_ID, &VendorID);
if (ret)goto error; if ((VendorID == 0xFFFF) || (VendorID == 0x0000)) continue;
if (ret)goto error; if (!Function) pci_read_config_byte(dev, PCI_HEADER_TYPE, &HeaderType);-- 1.9.1
This seems really odd to me. Why would pci_read_config_word() return an error if there is no device there? Is that the real bug here?
Regards, Simon
On Fri, Oct 9, 2015 at 6:36 AM, Simon Glass sjg@chromium.org wrote:
This seems really odd to me. Why would pci_read_config_word() return an error if there is no device there? Is that the real bug here?
Looks like the expected behaviour: It tried to scan all the PCI elements in the bus and it failed to read when there is no more PCI device.
Regards,
Fabio Estevam
Hi Fabio,
On 9 October 2015 at 13:52, Fabio Estevam festevam@gmail.com wrote:
On Fri, Oct 9, 2015 at 6:36 AM, Simon Glass sjg@chromium.org wrote:
This seems really odd to me. Why would pci_read_config_word() return an error if there is no device there? Is that the real bug here?
Looks like the expected behaviour: It tried to scan all the PCI elements in the bus and it failed to read when there is no more PCI device.
I'm just surprised that it is failing when there is nothing there. I think it should succeed (and read 0xffff).
What board is this? Can you find the code that is returning this error? It may be a call to pci_set_ops() which sets read_word().
Regards, Simon
Hi Simon,
On Fri, Oct 9, 2015 at 10:01 AM, Simon Glass sjg@chromium.org wrote:
I'm just surprised that it is failing when there is nothing there. I think it should succeed (and read 0xffff).
What board is this? Can you find the code that is returning this error? It may be a call to pci_set_ops() which sets read_word().
It is a mx6qsabresd board.
I don't have the PCI device at the moment to give it a try, but looking at the code, in pcie_imx we have:
pci_set_ops(hose, pci_hose_read_config_byte_via_dword, pci_hose_read_config_word_via_dword, imx_pcie_read_config, pci_hose_write_config_byte_via_dword, pci_hose_write_config_word_via_dword, imx_pcie_write_config);
Then in drivers/pci/pci.c:
#define PCI_READ_VIA_DWORD_OP(size, type, off_mask) \ int pci_hose_read_config_##size##_via_dword(struct pci_controller *hose,\ pci_dev_t dev, \ int offset, type val) \ { \ u32 val32; \ \ if (pci_hose_read_config_dword(hose, dev, offset & 0xfc, &val32) < 0) { \ *val = -1; \ return -1; \ } \ \ *val = (val32 >> ((offset & (int)off_mask) * 8)); \ \ return 0; \ }
,which returns -1 (and also *val = -1) in the case of errror, which matches the values that I was reading yesterday
Regards,
Fabio Estevam
Hi Fabio,
On 9 October 2015 at 14:22, Fabio Estevam festevam@gmail.com wrote:
Hi Simon,
On Fri, Oct 9, 2015 at 10:01 AM, Simon Glass sjg@chromium.org wrote:
I'm just surprised that it is failing when there is nothing there. I think it should succeed (and read 0xffff).
What board is this? Can you find the code that is returning this error? It may be a call to pci_set_ops() which sets read_word().
It is a mx6qsabresd board.
I don't have the PCI device at the moment to give it a try, but looking at the code, in pcie_imx we have:
pci_set_ops(hose, pci_hose_read_config_byte_via_dword, pci_hose_read_config_word_via_dword, imx_pcie_read_config, pci_hose_write_config_byte_via_dword, pci_hose_write_config_word_via_dword, imx_pcie_write_config);Then in drivers/pci/pci.c:
#define PCI_READ_VIA_DWORD_OP(size, type, off_mask) \ int pci_hose_read_config_##size##_via_dword(struct pci_controller *hose,\ pci_dev_t dev, \ int offset, type val) \ { \ u32 val32; \ \ if (pci_hose_read_config_dword(hose, dev, offset & 0xfc, &val32) < 0) { \ *val = -1; \ return -1; \ } \ \ *val = (val32 >> ((offset & (int)off_mask) * 8)); \ \ return 0; \ }
,which returns -1 (and also *val = -1) in the case of errror, which matches the values that I was reading yesterday
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.
Regards, Simon
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));
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
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?
--- 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:
--- 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)
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.
participants (4)
-
Bin Meng -
Fabio Estevam -
Simon Glass -
Tom Rini