[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 at 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 at gmail.com> wrote:
>> > On Thu, Jan 7, 2016 at 10:02 PM, Bin Meng <bmeng.cn at 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