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

[Marek Vasut]

On 08/23/2018 12:45 PM, Simon Glass wrote:
[...]
>>> 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.

Well, yeah, I skipped the middle step for the sake of brevity.
Ultimately, you need a compat string there.

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

Right

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

Yes, because it is redundant and possibly harmful. And because I want to
parse existing DTs without special U-Boot modifications or
hacking/twisting U-Boot in some obscure way which doesn't scale.

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

Of course, you can match on PCI IDs or PCI class already, PCI is a
discoverable bus.

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

What I am saying is the following needs to be supported:

pci-controller at 100 {
	...
	usb at 1,0 {
		reg = <0x800 0 0 0 0>;
		phys = <&usb0 0>;
		phy-names = "usb";
	};
	...
};

You want to match the PCI device in BFD x.1.0 using PCI ID (ie. the
driver that matches in this case is *hci-pci) and then associate the
node usb at 1,0 , which contains PHY information for that *hci-pci driver,
with it.

> How would DT normally handle this?

That's how DTs do it, even for other than r8axxxx devices. There are iMX
devices which use the same approach for associating ethernet MAC with a
PCI NIC for example.

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

You don't need overlays at all, the base DT is sufficient as is.

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

For the PHY case, it's controller-type-independent.

> 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?).

Well the DT parsing functions are already in U-Boot if you use the PCI,
so the code size impact of this change is negligible.

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

My argument is that I want to be able to parse DTs without hacking them
up for U-Boot. If adding a few lines of code and few bytes to the size
of bulky subsystem like PCI does that, that's fine in my book.

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

We do, since there seems to be usecase for those too.

-- 
Best regards,
Marek Vasut