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

[Marek Vasut]

On 08/10/2018 02:01 PM, Tom Rini wrote:
> On Wed, Aug 08, 2018 at 09:37:25PM +0200, Marek Vasut wrote:
>> On 08/08/2018 05:32 PM, Bin Meng wrote:
>>> Hi Marek,
>>>
>>> On Wed, Aug 8, 2018 at 10:33 PM, Marek Vasut <marek.vasut at gmail.com> wrote:
>>>> On 08/08/2018 03:39 PM, Bin Meng wrote:
>>>>> Hi Marek,
>>>>>
>>>>> On Wed, Aug 8, 2018 at 9:24 PM, Marek Vasut <marek.vasut at gmail.com> wrote:
>>>>>> On 08/08/2018 03:14 PM, Bin Meng wrote:
>>>>>>> Hi Marek,
>>>>>>>
>>>>>>> On Wed, Aug 8, 2018 at 9:03 PM, Marek Vasut <marek.vasut at gmail.com> wrote:
>>>>>>>> The PCI controller can have DT subnodes describing extra properties
>>>>>>>> of particular PCI devices, ie. a PHY attached to an EHCI controller
>>>>>>>> on a PCI bus. This patch parses those DT subnodes and assigns a node
>>>>>>>> to the PCI device instance, so that the driver can extract details
>>>>>>>> from that node and ie. configure the PHY using the PHY subsystem.
>>>>>>>>
>>>>>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas at gmail.com>
>>>>>>>> Cc: Simon Glass <sjg at chromium.org>
>>>>>>>> ---
>>>>>>>>  drivers/pci/pci-uclass.c | 14 ++++++++++++++
>>>>>>>>  1 file changed, 14 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
>>>>>>>> index 46e9c71bdf..306bea0dbf 100644
>>>>>>>> --- a/drivers/pci/pci-uclass.c
>>>>>>>> +++ b/drivers/pci/pci-uclass.c
>>>>>>>> @@ -662,6 +662,8 @@ static int pci_find_and_bind_driver(struct udevice *parent,
>>>>>>>>                 for (id = entry->match;
>>>>>>>>                      id->vendor || id->subvendor || id->class_mask;
>>>>>>>>                      id++) {
>>>>>>>> +                       ofnode node;
>>>>>>>> +
>>>>>>>>                         if (!pci_match_one_id(id, find_id))
>>>>>>>>                                 continue;
>>>>>>>>
>>>>>>>> @@ -691,6 +693,18 @@ static int pci_find_and_bind_driver(struct udevice *parent,
>>>>>>>>                                 goto error;
>>>>>>>>                         debug("%s: Match found: %s\n", __func__, drv->name);
>>>>>>>>                         dev->driver_data = find_id->driver_data;
>>>>>>>> +
>>>>>>>> +                       dev_for_each_subnode(node, parent) {
>>>>>>>> +                               phys_addr_t df, size;
>>>>>>>> +                               df = ofnode_get_addr_size(node, "reg", &size);
>>>>>>>> +
>>>>>>>> +                               if (PCI_FUNC(df) == PCI_FUNC(bdf) &&
>>>>>>>> +                                   PCI_DEV(df) == PCI_DEV(bdf)) {
>>>>>>>> +                                       dev->node = node;
>>>>>>>> +                                       break;
>>>>>>>> +                               }
>>>>>>>
>>>>>>> The function pci_find_and_bind_driver() is supposed to bind devices
>>>>>>> that are NOT in the device tree. Adding device tree access in this
>>>>>>> routine is quite odd. You can add the EHCI controller that need such
>>>>>>> PHY subnodes in the device tree and there is no need to modify
>>>>>>> anything I believe. If you are looking for an example, please check
>>>>>>> pciuart0 in arch/x86/dts/crownbay.dts.
>>>>>>
>>>>>> Well this does not work for me, the EHCI PCI doesn't get a DT node
>>>>>> assigned, check r8a7794.dtsi for the PCI devices I use.
>>>>>>
>>>>>
>>>>> I think that's because you don't specify a "compatible" string for
>>>>> these two EHCI PCI nodes.
>>>>
>>>> That's perfectly fine, why should I specify it ? Linux has no problem
>>>> with it either.
>>>>
>>>
>>> Without a "compatible" string, DM does not bind any device in the
>>> device tree to a driver, hence no device node created. This is not
>>> Linux.
>>
>> DT is NOT Linux specific, it is OS-agnostic, DT describes hardware and
>> hardware only. If U-Boot cannot parse DT correctly, U-Boot is broken and
>> must be fixed.
>>
>> This is a fix. If there is a better fix, I am open to it.
> 
> DT should but isn't always OS agnostic.  DTS files that reside in the
> Linux Kernel are in practice is Linux-centric with the expectation that
> even if you could solve a given problem with valid DTS changes you make
> whatever is parsing it do additional logic instead.  That,
> approximately, is what your patch is doing.  If you added some HW
> description information to the dtsi file everything would work as
> expected as your DTS is describing the hardware and U-Boot is reading
> that description and figuring out what to do with it.

Yes, you need additional logic to match the PCI controller subnode in DT
with PCI device BFD, that's expected. You do NOT need extra compatibles,
the PCI bus gives you enough information to match a driver on them. In
fact, adding a compatible can interfere with this matching.

> The problem here is that in Linux, something that sees the compatible
> renesas,pci-r8a7794 or renesas,pci-rcar-gen2 is doing whatever else
> needs to be done.  Or something else?  Please explain how what you want
> to have work here in U-Boot is working in the Linux kernel.  Thanks!

This has nothing to do with a specific controller. iMX6 does the same
thing, see ie arch/arm/boot/dts/imx6q-utilite-pro.dts .

-- 
Best regards,
Marek Vasut