| Message ID | 1444261051-5468-1-git-send-email-festevam@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Glass |
| Headers | show |
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
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);
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