[U-Boot] [PATCH 1/3] pci: Add error values definitions from the kernel

[U-Boot] [PATCH 1/3] pci: Add error values definitions from the kernel

[Fabio Estevam]

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

[U-Boot] [PATCH 2/3] pcie_imx: Adjust the return value when imx_pcie_addr_valid() fails

[Fabio Estevam]

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);
-- 
1.9.1

[U-Boot] [PATCH 2/3] pcie_imx: Adjust the return value when imx_pcie_addr_valid() fails

[Marek Vasut]

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

[U-Boot] [PATCH 3/3] pcie_layerscape: Adjust the return value when ls_pcie_addr_valid() fails

[Fabio Estevam]

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) {
-- 
1.9.1

[U-Boot] [PATCH 3/3] pcie_layerscape: Adjust the return value when ls_pcie_addr_valid() fails

[Marek Vasut]

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

[U-Boot] [PATCH 1/3] pci: Add error values definitions from the kernel

[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

[U-Boot] [PATCH 1/3] pci: Add error values definitions from the kernel

[Tom Rini]

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 :)

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160106/00c4ed93/attachment.sig>

[U-Boot] [PATCH 1/3] pci: Add error values definitions from the kernel

[Fabio Estevam]

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?

[U-Boot] [PATCH 1/3] pci: Add error values definitions from the kernel

[Tom Rini]

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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160106/2730d4e0/attachment.sig>

[U-Boot] [PATCH 1/3] pci: Add error values definitions from the kernel

[Bin Meng]

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

[U-Boot] [PATCH 1/3] pci: Add error values definitions from the kernel

[Fabio Estevam]

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

[U-Boot] [PATCH 1/3] pci: Add error values definitions from the kernel

[Bin Meng]

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:
>
> 1. Do not get the error when running 'pci 0' command
>
> 2. 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

[U-Boot] [PATCH 1/3] pci: Add error values definitions from the kernel

[Fabio Estevam]

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.

[U-Boot] [PATCH 1/3] pci: Add error values definitions from the kernel

[Tom Rini]

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?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160107/51ef1fb5/attachment.sig>

[U-Boot] [PATCH 1/3] pci: Add error values definitions from the kernel

[Fabio Estevam]

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.

[U-Boot] [PATCH 1/3] pci: Add error values definitions from the kernel

[Bin Meng]

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

[U-Boot] [PATCH 1/3] pci: Add error values definitions from the kernel

[Fabio Estevam]

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.

[U-Boot] [PATCH 1/3] pci: Add error values definitions from the kernel

[Bin Meng]

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

[U-Boot] [PATCH 1/3] pci: Add error values definitions from the kernel

[Marek Vasut]

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

[U-Boot] [PATCH 1/3] pci: Add error values definitions from the kernel

[Bin Meng]

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

[U-Boot] [PATCH 1/3] pci: Add error values definitions from the kernel

[Bin Meng]

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

[U-Boot] [PATCH 1/3] pci: Add error values definitions from the kernel

[Bin Meng]

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

[U-Boot] [PATCH 1/3] pci: Add error values definitions from the kernel

[Fabio Estevam]

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

[U-Boot] [PATCH 1/3] pci: Add error values definitions from the kernel

[Bin Meng]

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

[U-Boot] [PATCH 1/3] pci: Add error values definitions from the kernel

[Fabio Estevam]

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