[U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes

[Simon Glass]

Hi Marek,

On 22 August 2018 at 14:19, Marek Vasut <marek.vasut at gmail.com> wrote:
> On 08/22/2018 08:08 PM, Simon Glass wrote:
> [...]
>>> This is explained in the patch description and the thread again. Please
>>> read the thread before replying. Take a look at the r8a7794.dtsi and its
>>> PCI bindings, there are PCI controller subnodes which add extra
>>> information for PCI devices on the bus. These nodes do not have a
>>> compatible string, only a BFD.
>>>
>>> This is perfectly valid, since you can match a driver on the PCI IDs or
>>> classes (PCI_DEVICE()), but the driver doesn't have a DT node associated
>>> with it. If there is a DT node with a matching BFD, it is associated
>>> with the driver instance by this patch.
>>>
>>> This allows ie. the EHCI PCI driver to access that DT node and extract
>>> information about PHYs from the DT (in case of the r8a7794).
>>
>> But the code to do this already exists in pci_bind_bus_devices().
>>
>> I know I am late to this thread but no one is going to read 45 messages, sorry.
>
> But then what is the point of having ML discussions archived at all if I
> have to explain the same thing over and over to everyone who comes into
> the thread ?

It is an in-built admonition for participants to avoid generating more
heat than light :-)

I thought someone pulled me into this thread, but perhaps I am mistaken.

>
>> Why is pci_bus_find_devfn() failing?
>
> Because this function is a hack to force-bind drivers to PCI devices
> which are described in the DT with a compatible string. This does not
> apply to this case.

pci_bus_find_devfn() does not check compatible strings. It checks the
PCI devfn stuff. If the node is in the DT and has the correct devfn,
then pci_bus_find_devfn() should find it.

It does require that the node produced a device, which does indeed
require a compatible string, I think.

We support either using DT with a compatible string or using
PCI_DEVICE() without a DT node. But you already know that.

>
> If this function fails (correctly, no force-binding needs to happen
> because there is no compatible string, we want to match on PCI IDs),
> the code will continue and match on PCI ID/class with
> pci_find_and_bind_driver() . That function is where we need to verify if
> there isn't a PCI controller subnode in DT with a matching BFD which
> should be associated with the new driver instance.

So all of this is to avoid having a compatible string in the DT node for PCI?

Earlier I said:

>> So can you just add a compatible string and everything works?

and you replied:

> No, because that makes no sense. The code can very well match the driver
> on the PCI IDs or classes, the DT node is supplementary information and
> it needs to be made available to the matched driver if present in the
> DT. The DT node is matched on the BFD (the reg property in the DT).

Yes the DT node is matched on the BFD. Yes we can match the node as you say.

> The compatible string would only add redundancy and can in fact be
> harmful in case the PCI device could be replaced (ie. swapping USB EHCI
> controller for USB xHCI controller while retaining the USB PHY ; the
> controller itself could be discovered on the PCI bus, but the PHY can
> not and must be described in the DT).

This seems a bit odd. Without the compatible string we don't know what
driver to use, except through the PCI_DEVICE() mechanism. Is that what
you are using?

So is this a reason why we need this 3rd option? For your example are
you saying that there are two different settings for a device, which
result in using a different driver? How would DT normally handle this?
Overlays? Having two nodes and marking one status = "disabled"? This
problem does not seem unique to PCI. I could have a similar problem
with USB or even with a standard SoC having a memory-mapped USB
controller that can support both protocols (controlled by a setting
somewhere).

If you have both EHCI and a xHCI controller which can occupy the same
BFD, then how would you supply in the DT options needed by the
controller itself? Don't you need two nodes in that case?

Also, what if these two devices each had their own PHY and there were
settings in that PHY DT node? You would need a separate node for each
one, right?

I suggest that we don't NEED this third way. The question in my mind
is whether it is necessary and doesn't just add confusion.

The code in Linux seems to be:

struct device_node *of_pci_find_child_device(struct device_node *parent,
     unsigned int devfn)
{
        struct device_node *node, *node2;

        for_each_child_of_node(parent, node) {
                if (__of_pci_pci_compare(node, devfn))
                        return node;
               /*
               * Some OFs create a parent node "multifunc-device" as
               * a fake root for all functions of a multi-function
               * device we go down them as well.
               */
               if (!strcmp(node->name, "multifunc-device")) {
                       for_each_child_of_node(node, node2) {
                               if (__of_pci_pci_compare(node2, devfn)) {
                                       of_node_put(node);
                                       return node2;
                               }
                       }
               }
        }
        return NULL;
}

void pci_set_of_node(struct pci_dev *dev)
{
        if (!dev->bus->dev.of_node)
                return;
        dev->dev.of_node = of_pci_find_child_device(dev->bus->dev.of_node,
                  dev->devfn);
}

There is no comment but it looks like this is where Linux does its
'fallback' to find a node with no compatible string. The relevant
commit is:

commit 98d9f30c820d509145757e6ecbc36013aa02f7bc
Author: Benjamin Herrenschmidt <benh at kernel.crashing.org>
Date:   Mon Apr 11 11:37:07 2011 +1000

    pci/of: Match PCI devices to OF nodes dynamically

    powerpc has two different ways of matching PCI devices to their
    corresponding OF node (if any) for historical reasons. The ppc64 one
    does a scan looking for matching bus/dev/fn, while the ppc32 one does a
    scan looking only for matching dev/fn on each level in order to be
    agnostic to busses being renumbered (which Linux does on some
    platforms).

    This removes both and instead moves the matching code to the PCI core
    itself. It's the most logical place to do it: when a pci_dev is created,
    we know the parent and thus can do a single level scan for the matching
    device_node (if any).

    The benefit is that all archs now get the matching for free. There's one
    hook the arch might want to provide to match a PHB bus to its device
    node. A default weak implementation is provided that looks for the
    parent device device node, but it's not entirely reliable on powerpc for
    various reasons so powerpc provides its own.


I don't know if this helps us very much. Having just 2 ways seems
better than 3, and the compatible string is definitely a preferred way
in general. The mention of different ways of doing things seems like
something we should avoid in U-Boot if we can. It's a bootloader and
PCI is probed in SPL on some platforms, so code size is at a premium.
We also end up with confusion since it's hard or impossible to find
the drivers (right?).

But if we want to follow what Linux does, then we need a 3rd way.
So...how much do we want to follow Linux?

I think Tom should make the call here.

Whichever we do, the documentation needs updating. If we change it,
tests need updating too.

In any case, re Bin's list of things that need doing, I worry about
having different code for sandbox than other archs. It invalidates our
sandbox testing. Granted, we have to support the PCI emulators, but
that's OK since that code is not used except in sandbox. We still want
to support compatible strings in the DT for PCI devices, right?

Regards,
Simon