[U-Boot] [PATCH 1/3] pci: Add error values definitions from the kernel
From: Fabio Estevam fabio.estevam@nxp.com
Add PCI error values definitions from the kernel (include/linux/pci.h).
Signed-off-by: Fabio Estevam fabio.estevam@nxp.com --- include/pci.h | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/include/pci.h b/include/pci.h index 2adca85..0ff3c6e 100644 --- a/include/pci.h +++ b/include/pci.h @@ -451,6 +451,15 @@ #define PCI_EXT_CAP_ID_PMUX 0x1A /* Protocol Multiplexing */ #define PCI_EXT_CAP_ID_PASID 0x1B /* Process Address Space ID */
+/* Error values that may be returned by PCI functions. */ +#define PCIBIOS_SUCCESSFUL 0x00 +#define PCIBIOS_FUNC_NOT_SUPPORTED 0x81 +#define PCIBIOS_BAD_VENDOR_ID 0x83 +#define PCIBIOS_DEVICE_NOT_FOUND 0x86 +#define PCIBIOS_BAD_REGISTER_NUMBER 0x87 +#define PCIBIOS_SET_FAILED 0x88 +#define PCIBIOS_BUFFER_TOO_SMALL 0x89 + /* Include the ID list */
#include <pci_ids.h>
From: Fabio Estevam fabio.estevam@nxp.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
We can avoid this error by using the same approach as done in the linux kernel PCI designware driver: (drivers/pci/host/pcie-designware.c)
static int dw_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where, int size, u32 *val) { struct pcie_port *pp = bus->sysdata; int ret;
if (dw_pcie_valid_config(pp, bus, PCI_SLOT(devfn)) == 0) { *val = 0xffffffff; return PCIBIOS_DEVICE_NOT_FOUND; }
,where PCIBIOS_DEVICE_NOT_FOUND is returned when an invalid address is given.
Signed-off-by: Fabio Estevam fabio.estevam@nxp.com --- drivers/pci/pcie_imx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pci/pcie_imx.c b/drivers/pci/pcie_imx.c index f1e189e..02867e2 100644 --- 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, pci_dev_t d, ret = imx_pcie_addr_valid(d); if (ret) { *val = 0xffffffff; - return ret; + return PCIBIOS_DEVICE_NOT_FOUND; }
va_address = get_bus_address(d, where);
On Wednesday, January 06, 2016 at 09:53:17 PM, Fabio Estevam wrote:
From: Fabio Estevam fabio.estevam@nxp.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
We can avoid this error by using the same approach as done in the linux kernel PCI designware driver: (drivers/pci/host/pcie-designware.c)
static int dw_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where, int size, u32 *val) { struct pcie_port *pp = bus->sysdata; int ret;
if (dw_pcie_valid_config(pp, bus, PCI_SLOT(devfn)) == 0) { *val = 0xffffffff; return PCIBIOS_DEVICE_NOT_FOUND; }
,where PCIBIOS_DEVICE_NOT_FOUND is returned when an invalid address is given.
Signed-off-by: Fabio Estevam fabio.estevam@nxp.com
Acked-by: Marek Vasut marex@denx.de
Best regards, Marek Vasut
From: Fabio Estevam fabio.estevam@nxp.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
We can avoid this error by using the same approach as done in the linux kernel PCI designware driver: (drivers/pci/host/pcie-designware.c)
static int dw_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where, int size, u32 *val) { struct pcie_port *pp = bus->sysdata; int ret;
if (dw_pcie_valid_config(pp, bus, PCI_SLOT(devfn)) == 0) { *val = 0xffffffff; return PCIBIOS_DEVICE_NOT_FOUND; }
,where PCIBIOS_DEVICE_NOT_FOUND is returned when an invalid address is given.
Reported-by: Bin Meng bmeng.cn@gmail.com Signed-off-by: Fabio Estevam fabio.estevam@nxp.com --- drivers/pci/pcie_layerscape.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pci/pcie_layerscape.c b/drivers/pci/pcie_layerscape.c index 58e88ae..af0fd71 100644 --- 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, pci_dev_t d,
if (ls_pcie_addr_valid(hose, d)) { *val = 0xffffffff; - return -EINVAL; + return PCIBIOS_DEVICE_NOT_FOUND; }
if (PCI_BUS(d) == hose->first_busno) {
On Wednesday, January 06, 2016 at 09:53:18 PM, Fabio Estevam wrote:
From: Fabio Estevam fabio.estevam@nxp.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
We can avoid this error by using the same approach as done in the linux kernel PCI designware driver: (drivers/pci/host/pcie-designware.c)
static int dw_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where, int size, u32 *val) { struct pcie_port *pp = bus->sysdata; int ret;
if (dw_pcie_valid_config(pp, bus, PCI_SLOT(devfn)) == 0) { *val = 0xffffffff; return PCIBIOS_DEVICE_NOT_FOUND; }
,where PCIBIOS_DEVICE_NOT_FOUND is returned when an invalid address is given.
Reported-by: Bin Meng bmeng.cn@gmail.com Signed-off-by: Fabio Estevam fabio.estevam@nxp.com
Good stuff, thanks!
Acked-by: Marek Vasut marex@denx.de
I added Stefano on CC, so I added him. I'd like to see all three in mainline for this MW.
Best regards, Marek Vasut
On Wednesday, January 06, 2016 at 09:53:16 PM, Fabio Estevam wrote:
From: Fabio Estevam fabio.estevam@nxp.com
Add PCI error values definitions from the kernel (include/linux/pci.h).
Signed-off-by: Fabio Estevam fabio.estevam@nxp.com
Acked-by: Marek Vasut marex@denx.de
Best regards, Marek Vasut
On Wed, Jan 06, 2016 at 06:53:16PM -0200, Fabio Estevam wrote:
From: Fabio Estevam fabio.estevam@nxp.com
Add PCI error values definitions from the kernel (include/linux/pci.h).
You forgot the kernel hash you took this from :)
On Wed, Jan 6, 2016 at 8:55 PM, Tom Rini trini@konsulko.com wrote:
On Wed, Jan 06, 2016 at 06:53:16PM -0200, Fabio Estevam wrote:
From: Fabio Estevam fabio.estevam@nxp.com
Add PCI error values definitions from the kernel (include/linux/pci.h).
You forgot the kernel hash you took this from :)
It is from kernel 4.3.3. Could this be included while applying it?
On Wed, Jan 06, 2016 at 08:58:30PM -0200, Fabio Estevam wrote:
On Wed, Jan 6, 2016 at 8:55 PM, Tom Rini trini@konsulko.com wrote:
On Wed, Jan 06, 2016 at 06:53:16PM -0200, Fabio Estevam wrote:
From: Fabio Estevam fabio.estevam@nxp.com
Add PCI error values definitions from the kernel (include/linux/pci.h).
You forgot the kernel hash you took this from :)
It is from kernel 4.3.3. Could this be included while applying it?
OK.
Hi Fabio,
On Thu, Jan 7, 2016 at 4:53 AM, Fabio Estevam festevam@gmail.com wrote:
From: Fabio Estevam fabio.estevam@nxp.com
Add PCI error values definitions from the kernel (include/linux/pci.h).
Signed-off-by: Fabio Estevam fabio.estevam@nxp.com
include/pci.h | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/include/pci.h b/include/pci.h index 2adca85..0ff3c6e 100644 --- a/include/pci.h +++ b/include/pci.h @@ -451,6 +451,15 @@ #define PCI_EXT_CAP_ID_PMUX 0x1A /* Protocol Multiplexing */ #define PCI_EXT_CAP_ID_PASID 0x1B /* Process Address Space ID */
+/* Error values that may be returned by PCI functions. */ +#define PCIBIOS_SUCCESSFUL 0x00 +#define PCIBIOS_FUNC_NOT_SUPPORTED 0x81 +#define PCIBIOS_BAD_VENDOR_ID 0x83 +#define PCIBIOS_DEVICE_NOT_FOUND 0x86 +#define PCIBIOS_BAD_REGISTER_NUMBER 0x87 +#define PCIBIOS_SET_FAILED 0x88 +#define PCIBIOS_BUFFER_TOO_SMALL 0x89
/* Include the ID list */
#include <pci_ids.h>
Why should we introduce another set of error values just to fix this specific PCIe issue? Isn't -EINVAL enough?
Regards, Bin
Hi Bin,
On Thu, Jan 7, 2016 at 12:11 AM, Bin Meng bmeng.cn@gmail.com wrote:
Why should we introduce another set of error values just to fix this
We should better align this with the kernel code.
specific PCIe issue? Isn't -EINVAL enough?
Returning -EINVAL is what we do today and it causes the issue when running 'pci 0' command on imx and layerscape.
With this series we get the following results:
1. Do not get the error when running 'pci 0' command
2. Align the PCI return error code with the kernel.
Both are good things IMHO.
Regards,
Fabio Estevam
Hi Fabio,
On Thu, Jan 7, 2016 at 8:15 PM, Fabio Estevam festevam@gmail.com wrote:
Hi Bin,
On Thu, Jan 7, 2016 at 12:11 AM, Bin Meng bmeng.cn@gmail.com wrote:
Why should we introduce another set of error values just to fix this
We should better align this with the kernel code.
specific PCIe issue? Isn't -EINVAL enough?
Returning -EINVAL is what we do today and it causes the issue when running 'pci 0' command on imx and layerscape.
Ah, yes! Sorry I wanted to say the original proposal to "return 0" instead. But now I failed to understand why this fixed the issue because those error numbers are not zero.
With this series we get the following results:
Do not get the error when running 'pci 0' command
Align the PCI return error code with the kernel.
But if we introduce those error numbers, we should promote its usage in the PCI codes. Otherwise I fail to see its value as it is just used in one or two drivers.
Both are good things IMHO.
And why returning zero is bad? From a working PCIe controller (either the one in those Freescale's PowerPC processors, or Intel's), when probing a non-existent PCI device, the controller just returns 0xFFFFFFFF automatically in its register (done by hardware), and nothing fails there (IOW, PCI config read op returns zero for these controllers). Based on my read of the imx and layerscape PCIe driver, it looks we are trying to workaround the buggy hardware as it returns 0xFFFFFFFF in a pure software way.
Regards, Bin
On Thu, Jan 7, 2016 at 10:58 AM, Bin Meng bmeng.cn@gmail.com wrote:
Ah, yes! Sorry I wanted to say the original proposal to "return 0" instead. But now I failed to understand why this fixed the issue because those error numbers are not zero.
The problem only happens if we return a negative value.
And why returning zero is bad? From a working PCIe controller (either
It is not bad, but I still prefer to keep in sync with the kernel driver.
On Thu, Jan 07, 2016 at 11:31:06AM -0200, Fabio Estevam wrote:
On Thu, Jan 7, 2016 at 10:58 AM, Bin Meng bmeng.cn@gmail.com wrote:
Ah, yes! Sorry I wanted to say the original proposal to "return 0" instead. But now I failed to understand why this fixed the issue because those error numbers are not zero.
The problem only happens if we return a negative value.
And why returning zero is bad? From a working PCIe controller (either
It is not bad, but I still prefer to keep in sync with the kernel driver.
And would allow further features later on?
On Thu, Jan 7, 2016 at 2:28 PM, Tom Rini trini@konsulko.com wrote:
On Thu, Jan 07, 2016 at 11:31:06AM -0200, Fabio Estevam wrote:
On Thu, Jan 7, 2016 at 10:58 AM, Bin Meng bmeng.cn@gmail.com wrote:
Ah, yes! Sorry I wanted to say the original proposal to "return 0" instead. But now I failed to understand why this fixed the issue because those error numbers are not zero.
The problem only happens if we return a negative value.
And why returning zero is bad? From a working PCIe controller (either
It is not bad, but I still prefer to keep in sync with the kernel driver.
And would allow further features later on?
That's correct, Tom.
Hi Fabio,
On Fri, Jan 8, 2016 at 12:49 AM, Fabio Estevam festevam@gmail.com wrote:
On Thu, Jan 7, 2016 at 2:28 PM, Tom Rini trini@konsulko.com wrote:
On Thu, Jan 07, 2016 at 11:31:06AM -0200, Fabio Estevam wrote:
On Thu, Jan 7, 2016 at 10:58 AM, Bin Meng bmeng.cn@gmail.com wrote:
Ah, yes! Sorry I wanted to say the original proposal to "return 0" instead. But now I failed to understand why this fixed the issue because those error numbers are not zero.
The problem only happens if we return a negative value.
And why returning zero is bad? From a working PCIe controller (either
It is not bad, but I still prefer to keep in sync with the kernel driver.
And would allow further features later on?
That's correct, Tom.
What new feature would benefit from this? These two PCIe drivers are non-DM drivers and DM PCI is not utilizing those error codes, instead DM PCI is using U-Boot standard error codes.
Regards, Bin
On Thu, Jan 7, 2016 at 10:02 PM, Bin Meng bmeng.cn@gmail.com wrote:
What new feature would benefit from this? These two PCIe drivers are non-DM drivers and DM PCI is not utilizing those error codes, instead DM PCI is using U-Boot standard error codes.
but what prevents DM PCI to use the kernel error codes in the future?
Returning PCIBIOS_DEVICE_NOT_FOUND when the config is invalid is a common pattern in the kernel.
Take a look at these drivers:
drivers/pci/access.c drivers/pci/host/pci-mvebu.c drivers/pci/host/pci-xgene.c drivers/pci/host/pcie-altera.c drivers/pci/host/pcie-designware.c drivers/pci/host/pcie-rcar.c drivers/pci/xen-pcifront.c
I don't see why we can't do the same in U-boot.
Feel free to submit a patch with your proposal and the maintainer can then decide which one is more adequate for U-boot.
On Fri, Jan 8, 2016 at 8:15 AM, Fabio Estevam festevam@gmail.com wrote:
On Thu, Jan 7, 2016 at 10:02 PM, Bin Meng bmeng.cn@gmail.com wrote:
What new feature would benefit from this? These two PCIe drivers are non-DM drivers and DM PCI is not utilizing those error codes, instead DM PCI is using U-Boot standard error codes.
but what prevents DM PCI to use the kernel error codes in the future?
Returning PCIBIOS_DEVICE_NOT_FOUND when the config is invalid is a common pattern in the kernel.
Take a look at these drivers:
drivers/pci/access.c drivers/pci/host/pci-mvebu.c drivers/pci/host/pci-xgene.c drivers/pci/host/pcie-altera.c drivers/pci/host/pcie-designware.c drivers/pci/host/pcie-rcar.c drivers/pci/xen-pcifront.c
I don't see why we can't do the same in U-boot.
I am sorry but this does not convince me. Kernel is using these codes, but I don't see any PCI library codes in kernel that actually parses these error codes.
Feel free to submit a patch with your proposal and the maintainer can then decide which one is more adequate for U-boot.
My points are: 1). These two drivers are non-DM drivers. We should start converting them to DM drivers first instead of adding new stuff which is only used by legacy drivers. I have pcie_layerscape on my TODO list. 2). We should stick to the correct behavior. The PCIe controller is buggy that it cannot return 0xFFFFFFFF by hardware, like other controllers do. IMHO it's completely a horrible controller, that comments in ls_pcie_addr_valid() says: "Controller does not support multi-function in RC mode". 3). If we want to align with kernel, I want to see a plan of at least adopting these error codes in DM PCI. This is something which is not clear for now.
Simon, what's your opinion on this?
Regards, Bin
On Friday, January 08, 2016 at 01:31:17 AM, Bin Meng wrote:
On Fri, Jan 8, 2016 at 8:15 AM, Fabio Estevam festevam@gmail.com wrote:
On Thu, Jan 7, 2016 at 10:02 PM, Bin Meng bmeng.cn@gmail.com wrote:
What new feature would benefit from this? These two PCIe drivers are non-DM drivers and DM PCI is not utilizing those error codes, instead DM PCI is using U-Boot standard error codes.
but what prevents DM PCI to use the kernel error codes in the future?
Returning PCIBIOS_DEVICE_NOT_FOUND when the config is invalid is a common pattern in the kernel.
Take a look at these drivers:
drivers/pci/access.c drivers/pci/host/pci-mvebu.c drivers/pci/host/pci-xgene.c drivers/pci/host/pcie-altera.c drivers/pci/host/pcie-designware.c drivers/pci/host/pcie-rcar.c drivers/pci/xen-pcifront.c
I don't see why we can't do the same in U-boot.
I am sorry but this does not convince me. Kernel is using these codes, but I don't see any PCI library codes in kernel that actually parses these error codes.
Feel free to submit a patch with your proposal and the maintainer can then decide which one is more adequate for U-boot.
My points are: 1). These two drivers are non-DM drivers. We should start converting them to DM drivers first instead of adding new stuff which is only used by legacy drivers. I have pcie_layerscape on my TODO list.
This point is invalid, I see this patch series is just fixing bugs and converting drivers to DM is _far_ beyond the scope of this series.
2). We should stick to the correct behavior. The PCIe controller is buggy that it cannot return 0xFFFFFFFF by hardware, like other controllers do. IMHO it's completely a horrible controller, that comments in ls_pcie_addr_valid() says: "Controller does not support multi-function in RC mode".
Can you elaborate on why do you think it is a horrible controller ? Most of the controllers I saw used on ARM were this way.
3). If we want to align with kernel, I want to see a plan of at least adopting these error codes in DM PCI. This is something which is not clear for now.
No, this is again far beyond the scope of this series. I want PCI on iMX and LS to work in the current release, period. So from my side, I want these patches to go in.
Simon, what's your opinion on this?
Regards, Bin
Best regards, Marek Vasut
Hi Marek,
On Fri, Jan 8, 2016 at 8:35 AM, Marek Vasut marex@denx.de wrote:
On Friday, January 08, 2016 at 01:31:17 AM, Bin Meng wrote:
On Fri, Jan 8, 2016 at 8:15 AM, Fabio Estevam festevam@gmail.com wrote:
On Thu, Jan 7, 2016 at 10:02 PM, Bin Meng bmeng.cn@gmail.com wrote:
What new feature would benefit from this? These two PCIe drivers are non-DM drivers and DM PCI is not utilizing those error codes, instead DM PCI is using U-Boot standard error codes.
but what prevents DM PCI to use the kernel error codes in the future?
Returning PCIBIOS_DEVICE_NOT_FOUND when the config is invalid is a common pattern in the kernel.
Take a look at these drivers:
drivers/pci/access.c drivers/pci/host/pci-mvebu.c drivers/pci/host/pci-xgene.c drivers/pci/host/pcie-altera.c drivers/pci/host/pcie-designware.c drivers/pci/host/pcie-rcar.c drivers/pci/xen-pcifront.c
I don't see why we can't do the same in U-boot.
I am sorry but this does not convince me. Kernel is using these codes, but I don't see any PCI library codes in kernel that actually parses these error codes.
Feel free to submit a patch with your proposal and the maintainer can then decide which one is more adequate for U-boot.
My points are: 1). These two drivers are non-DM drivers. We should start converting them to DM drivers first instead of adding new stuff which is only used by legacy drivers. I have pcie_layerscape on my TODO list.
This point is invalid, I see this patch series is just fixing bugs and converting drivers to DM is _far_ beyond the scope of this series.
I am not indicating we should convert these drivers to DM to _fix_ this simple issue. I was saying we should be careful to introduce new stuff to legacy drivers from now on given DM PCI is already in a good shape.
2). We should stick to the correct behavior. The PCIe controller is buggy that it cannot return 0xFFFFFFFF by hardware, like other controllers do. IMHO it's completely a horrible controller, that comments in ls_pcie_addr_valid() says: "Controller does not support multi-function in RC mode".
Can you elaborate on why do you think it is a horrible controller ? Most of the controllers I saw used on ARM were this way.
First it does not return 0xFFFFFFFF automatically by hardware when probing non-existent devices. This is suggested by the PCI spec. See the following adopted from PCI local bus spec 3.0 chapter 6.1 Configuration Space Organization:
System software may need to scan the PCI bus to determine what devices are actually present. To do this, the configuration software must read the Vendor ID in each possible PCI "slot." The host bus to PCI bridge must unambiguously report attempts to read the Vendor ID of non-existent devices. Since FFFFh is an invalid Vendor ID, it is adequate for the host bus to PCI bridge to return a value of all 1's on read accesses to Configuration Space registers of non-existent devices. (Note that these accesses will be terminated with a Master-Abort.)
AFAIK, all Intel chipsets behave this way (no surprise, PCI spec was originally developed by Intel). Freescale's MPC85xx, QorIQ PowerPC processors' PCI/PCIe controllers behave this way too.
Secondly, from the comment "Controller does not support multi-function in RC mode", if this is true, then why? I understand that it is a case when the PCIe IP isn't mature or in a very early IP validation phase I would categorize into hardware errata. But looks this Designware IP is widely used in ARM systems today in many SoCs. It's already in production phase but it still has such limitation?
"Most of the controllers I saw used on ARM were this way" does not justify it is not horrible. I even saw more "horrible" PCI controller in an ARM SoC from Mindspeed years ago, that accessing PCI memory space from CPU requires indirect access, IOW it is not memory-mapped, directly accessible.
3). If we want to align with kernel, I want to see a plan of at least adopting these error codes in DM PCI. This is something which is not clear for now.
No, this is again far beyond the scope of this series. I want PCI on iMX and LS to work in the current release, period. So from my side, I want these patches to go in.
I raised this for open discussion. I was worried that we borrowed this stuff from kernel to U-Boot just for fixing this specific issue, and we never touch this again.
Simon, what's your opinion on this?
I just figured out a solid reason why this is not a correct fix but still a workaround.
Regards, Bin
Hi Fabio,
On Thu, Jan 7, 2016 at 9:31 PM, Fabio Estevam festevam@gmail.com wrote:
On Thu, Jan 7, 2016 at 10:58 AM, Bin Meng bmeng.cn@gmail.com wrote:
Ah, yes! Sorry I wanted to say the original proposal to "return 0" instead. But now I failed to understand why this fixed the issue because those error numbers are not zero.
The problem only happens if we return a negative value.
OK, now I am pretty sure this fix is not correct, that "the problem only happens if we return a negative value".
The imx and layerscape PCIe driver registers PCI config ops as following:
pci_set_ops(hose, pci_hose_read_config_byte_via_dword, pci_hose_read_config_word_via_dword, ls_pcie_read_config, pci_hose_write_config_byte_via_dword, pci_hose_write_config_word_via_dword, ls_pcie_write_config);
The pci_hose_read_config_byte_via_dword() and pci_hose_read_config_word_via_dword() only return -1 if the error number < 0. What if I call:
u32 data; ret = pci_read_config_dword(dev, PCI_VENDOR_ID, &data);
This will fail with error number 0x86, but if we do:
u16 data; ret = pci_read_config_dword(dev, PCI_VENDOR_ID, &data);
This will _not_ fail. This is inconsistent. You are just trying to workaround the 'pciinfo' command to make it output no error message.
And why returning zero is bad? From a working PCIe controller (either
It is not bad, but I still prefer to keep in sync with the kernel driver.
Regards, Bin
On Fri, Jan 8, 2016 at 8:46 AM, Bin Meng bmeng.cn@gmail.com wrote:
Hi Fabio,
On Thu, Jan 7, 2016 at 9:31 PM, Fabio Estevam festevam@gmail.com wrote:
On Thu, Jan 7, 2016 at 10:58 AM, Bin Meng bmeng.cn@gmail.com wrote:
Ah, yes! Sorry I wanted to say the original proposal to "return 0" instead. But now I failed to understand why this fixed the issue because those error numbers are not zero.
The problem only happens if we return a negative value.
OK, now I am pretty sure this fix is not correct, that "the problem only happens if we return a negative value".
The imx and layerscape PCIe driver registers PCI config ops as following:
pci_set_ops(hose, pci_hose_read_config_byte_via_dword, pci_hose_read_config_word_via_dword, ls_pcie_read_config, pci_hose_write_config_byte_via_dword, pci_hose_write_config_word_via_dword, ls_pcie_write_config);
The pci_hose_read_config_byte_via_dword() and pci_hose_read_config_word_via_dword() only return -1 if the error number < 0. What if I call:
u32 data; ret = pci_read_config_dword(dev, PCI_VENDOR_ID, &data);
This will fail with error number 0x86, but if we do:
u16 data; ret = pci_read_config_dword(dev, PCI_VENDOR_ID, &data);
oops, this should be: pci_read_config_word()
This will _not_ fail. This is inconsistent. You are just trying to workaround the 'pciinfo' command to make it output no error message.
And why returning zero is bad? From a working PCIe controller (either
It is not bad, but I still prefer to keep in sync with the kernel driver.
Regards, Bin
Hi Bin,
On Thu, Jan 7, 2016 at 10:46 PM, Bin Meng bmeng.cn@gmail.com wrote:
The pci_hose_read_config_byte_via_dword() and pci_hose_read_config_word_via_dword() only return -1 if the error number < 0. What if I call:
u32 data; ret = pci_read_config_dword(dev, PCI_VENDOR_ID, &data);
This will fail with error number 0x86, but if we do:
u16 data; ret = pci_read_config_dword(dev, PCI_VENDOR_ID, &data);
This will _not_ fail. This is inconsistent. You are just trying to workaround the 'pciinfo' command to make it output no error message.
Yes, I can see this inconsistency here, thanks. It also happens before my patch.
This inconsistency is gone if we do as you proposed earlier:
ret = imx_pcie_addr_valid(d); if (ret) { *val = 0xffffffff; return 0; }
Do you still agree with it? If so, maybe you could send a patch for it?
Thanks
Hi Fabio,
On Fri, Jan 8, 2016 at 10:09 AM, Fabio Estevam festevam@gmail.com wrote:
Hi Bin,
On Thu, Jan 7, 2016 at 10:46 PM, Bin Meng bmeng.cn@gmail.com wrote:
The pci_hose_read_config_byte_via_dword() and pci_hose_read_config_word_via_dword() only return -1 if the error number < 0. What if I call:
u32 data; ret = pci_read_config_dword(dev, PCI_VENDOR_ID, &data);
This will fail with error number 0x86, but if we do:
u16 data; ret = pci_read_config_dword(dev, PCI_VENDOR_ID, &data);
This will _not_ fail. This is inconsistent. You are just trying to workaround the 'pciinfo' command to make it output no error message.
Yes, I can see this inconsistency here, thanks. It also happens before my patch.
Thanks for trying that on your side too.
This inconsistency is gone if we do as you proposed earlier:
ret = imx_pcie_addr_valid(d); if (ret) { *val = 0xffffffff; return 0; }Do you still agree with it? If so, maybe you could send a patch for it?
Yep, agreed. I can send a patch if others don't object :)
Regards, Bin
Hi Bin,
On Fri, Jan 8, 2016 at 12:18 AM, Bin Meng bmeng.cn@gmail.com wrote:
Yep, agreed. I can send a patch if others don't object :)
If you send it, please add a 'Tested-by: Fabio Estevam fabio.estevam@nxp.com' on the imx patch.
Thanks for your patience,
Fabio Estevam
participants (4)
-
Bin Meng -
Fabio Estevam -
Marek Vasut -
Tom Rini