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

[Simon Glass]

Hi Marek,

On 23 August 2018 at 06:58, Marek Vasut <marek.vasut at gmail.com> wrote:
> 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.
>

Everything up to here seems OK.

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

What do you mean? Your example of why you can't use compatible strings
says you might have two different PHYs. But I think you should answer
my questions:

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

[...]

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

OK, well let's make sure we have some compatible notes too in sandbox,
so we retain testing.

Regards,
Simon