pci: Add error values definitions from the kernel

From: Fabio Estevam <[email protected]>

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 <[email protected]>

From: Fabio Estevam <[email protected]>

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 <[email protected]>

Signed-off-by Fabio Estevam <[email protected]>

From: Fabio Estevam <[email protected]>

Add PCI error values definitions from the kernel (include/linux/pci.h).

Signed-off-by Fabio Estevam <[email protected]>

From: Fabio Estevam <[email protected]>

Add PCI error values definitions from the kernel (include/linux/pci.h).

Quoted 3 lines · Fabio Estevam

From: Fabio Estevam <[email protected]>

Add PCI error values definitions from the kernel (include/linux/pci.h).

You forgot the kernel hash you took this from :)

Quoted 7 lines · Tom Rini

Quoted 3 lines · Fabio Estevam

From: Fabio Estevam <[email protected]>

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?

From: Fabio Estevam <[email protected]>

Add PCI error values definitions from the kernel (include/linux/pci.h).

Signed-off-by Fabio Estevam <[email protected]>

---
 include/pci.h | 9 +++++++++
 1 file changed, 9 insertions(+)

include/pci.h +3 -0

index 2adca85..0ff3c6e 100644
@@ -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

Why should we introduce another set of error values just to fix this

specific PCIe issue? Isn't -EINVAL enough?

Hi Bin,

On Thu, Jan 7, 2016 at 12:11 AM, Bin Meng <[email protected]> wrote:

Quoted 1 line

Why should we introduce another set of error values just to fix this

We should better align this with the kernel code.

Quoted 1 line

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.

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.

And why returning zero is bad? From a working PCIe controller (either

Quoted 3 lines · Bin Meng

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.

Quoted 1 line

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.

Quoted 6 lines · Fabio Estevam

Quoted 3 lines · Bin Meng

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.

Quoted 5 lines

The problem only happens if we return a negative value.

Quoted 1 line

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?

Quoted 9 lines · Tom Rini

Quoted 8 lines · Fabio Estevam

Quoted 3 lines · Bin Meng

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.

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

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.

Quoted 3 lines · Bin Meng

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.

Quoted 20 lines · Fabio Estevam

Quoted 3 lines · Bin Meng

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

Quoted 1 line

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.

Quoted 2 lines

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

Quoted 30 lines · Bin Meng

Quoted 20 lines · Fabio Estevam

Quoted 3 lines · Bin Meng

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

Quoted 1 line

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.

Quoted 2 lines

Feel free to submit a patch with your proposal and the maintainer can then decide which one is more adequate for U-boot.

Quoted 4 lines

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.

Quoted 6 lines

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.

Quoted 4 lines

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.

Quoted 2 lines

Simon, what's your opinion on this?

Quoted 3 lines · Bin Meng

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.

Quoted 2 lines

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.

Hi Fabio,

Quoted 7 lines · Fabio Estevam

Quoted 3 lines · Bin Meng

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.

Quoted 4 lines

Quoted 2 lines

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

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.

Hi Bin,

Quoted 9 lines · Bin Meng

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:

Quoted 5 lines

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?

Yep, agreed. I can send a patch if others don't object :)