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

Tue Aug 21 07:16:37 UTC 2018

Hi Marek,

On Tue, Aug 21, 2018 at 1:43 PM, Marek Vasut <marek.vasut at gmail.com> wrote:
> On 08/21/2018 06:56 AM, Bin Meng wrote:
> [...]
>>>>>> The proposal I made is:
>>>>>>
>>>>>> * Keep pci-uclass driver's post_bind() and child_post_bind() only for
>>>>>> Sandbox configuration
>>>>>> * Keep the call to pci_bus_find_devfn() in pci_bind_bus_devices() only
>>>>>> for Sandbox configuration
>>>>>> * Sandbox is special. We should limit the mechanism of matching PCI
>>>>>> emulation device via "compatible" to sandbox only
>>>>>
>>>>> The above three points can be done separately and I don't care about
>>>>> this too much.
>>>>>
>>>>
>>>> That's my concern. Only doing this created confusing and incomplete
>>>> design.
>>>
>>> Incomplete how ?
>>
>> I explained several times. I have to repeat here once again:
>>
>> Currently U-Boot supports two scenarios for PCI driver binding:
>>
>> - Declare a PCI device in the device tree. That requires specifying a
>> 'compatible' string as well as 'reg' property as defined by the 'PCI
>> Bus Binding' spec. DM uses the 'compatible' string to bind the driver
>> for the device.
>> - Don't declare a PCI device in the device tree. Instead, using
>> U_BOOT_PCI_DEVICE() to declare a device and driver mapping.
>>
>> You are creating a 3rd scenario which is a mix up of both scenario#1
>> and #2 in this patch, yet without any documentation, plus any
>> additional changes to impacted drivers.
>
> I am extending both, it's not a "third scenario".
>
> I said in the previous discussion I am willing to update the
> documentation to match the implementation, but that's about it.
>
>> So far almost all PCI device drivers in U-Boot supports both
>> scenarios, except PCI UART which currently only supports scenario#1.
>> If you declare what you do in this patch is pureblood then you should
>> revoke the other 2 at the same time. Leaving such in the mainstream
>> only creates chaos in the future and we should avoid doing that, given
>> we already had lots of discussion here.
>
> No. The compatible is valid for some PCI subdevs, ie. bridges, so that
> has to stay. You also need compatible for sandbox, you said that
> yourself. And declaring a PCI device with BFD in DT is needed, ie. for
> the r8a7794-style bindings.
>

OK, now new comments :) So you admit "compatible" can be valid for
some cases. I have to point out that your theory on PCI device probing
is self-contradictory. You refuse to add a "compatible" string to your
PCI device because you said the vendor/device id/class provides enough
information to bind the driver, then why do you want to specify your
devices in the device tree in the first place. According to your
theory, "Each PCI device already has a PCI ID and class which is used
to identify it and based on which the drivers bind to it, so a DT
compatible is NOT needed and is actually redundant and harmful.", I
would argue why not just creating a dedicate PCI device driver using
PCI ID and class information to do the driver binding and start from
there. As you mentioned PCI bus is probable bus like USB, everything
can be self-contained in the PCI device driver itself. There is no
need to create nothing in the device tree. If you want an example,
check 8250_pci.c in the Linux kernel. Everything that is needed to
configure the driver is included in the driver itself. It does not
read anything from the device tree.

> So again, how is the state of PCI more incomplete with this patch than
> without it ?
>
>>>> Yes, you probably don't care about this. But I care about this
>>>> as it impacts some other things like PCI uart driver which is used by
>>>> some x86 boards.
>>>
>>> Sandbox changes impact PCI UART driver and x86 boards how ?
>>
>> See above. It's not about sandbox!
>
> So what is it about ? And how does this core change impact anything ?
> Does it change behavior for some board or break some tests for you ?
>
>>>>>> * Create DM PCI test case against the DT node assignment
>>>>>> * Remove all compatible string in U-Boot's PCI device drivers: eg:
>>>>>> ehci_pci_ids[], xhci_pci_ids[], etc. IOW, all PCI device drivers
>>>>>> should only use U_BOOT_PCI_DEVICE(), aka the original U-Boot option 2
>>>>>> * Fork a "pci-ns16550" driver to support U_BOOT_PCI_DEVICE(), as
>>>>>> currently PCI ns16550 device driver uses "compatible" string to do the
>>>>>> matching, and update crownbay.dts and galileo.dts (so far I only know
>>>>>> two boards are using PCI ns16550 serial port)
>>>>>> * Make sure all DM PCI test cases are not broken
>>>>>> * Document all of the above changes in doc/driver-model/pci-info.txt
>>>>>
>>>>> I think you're just adding completely orthogonal stuff to this 5-liner
>>>>> patch into the list and overly complicate the situation. Sure, if you
>>>>> want to do all this, go ahead, but I don't see how this prevents this
>>>>> particular patch from being applied , except maybe for the documentation
>>>>> tweak.
>>>>
>>>> Please, think from the maintainer perspective. It's nothing related to
>>>> number of LOCs. It's that we need create a clean design. You probably
>>>> don't want someone just submits a single patch that changed current
>>>> USB design without a full consideration on all possible impacts.
>>>
>>> OK, after reading all this, what I gather from the discussion is that
>>> this patch somehow breaks the design or the other platforms, which is
>>> why you fight against it so much. Does it, yes or no? And if so, you
>>> still failed to explain how, so, how ?
>
> You did not answer this question.
>

It's too early to go testing at this point because we are still
discussing the design. BTW your patch does not check the return value
of ofnode_get_addr_size(). It's possible to write a DT without "reg"
property in a device node.

>>> Improving sandbox and x86 PCI handling is orthogonal to this change, so
>>> are ns16550 driver improvements and if you want to work on them, nothing
>>> prevents you from doing so.
>>
>> First of all, it's not x86 PCI handling. It's PCI device driver clean
>> up to match the new design.
>
> It is not a new design, just an extension of the current one.
>
> Driver cleanups where you match on PCI IDs or classes instead of
> compatible strings are completely orthogonal to this change, you don't
> even need this patch to do such a cleanup. Same applies for the x86 DT
> cleanup, you don't need this patch for those either.
>

Why would I? If there is no design changes, the clean up work to
current drivers is out of the question as these drivers are supported
user cases by current U-Boot PCI implementation.

> So please stop lumping those together with this patch, they are unrelated.
>
>> Second, doing such is necessary to make
>> the design clean and complete.
>
> No, doing so is orthogonal, see above.
>
>> Yes, I am glad to do the whole changes
>> if everyone agrees. I am more glad if you can do such changes in a
>> batch too.
>
> Feel free to submit your sandbox and ns16550 and whatever other
> improvements. I have neither an x86 with U-Boot nor ns16550 card
> available, so I cannot test those.
>

Regards,
Bin