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

[Bin Meng]

Hi Marek,

On Tue, Aug 21, 2018 at 12:30 PM, Marek Vasut <marek.vasut at gmail.com> wrote:
> On 08/21/2018 06:15 AM, Bin Meng wrote:
> [...]
>
>>>>>>> From what I understand, Marek and Bin are discussing whether a
>>>>>>> compatible string is needed to bind a driver.
>>>>>>>
>>>>>>> Generally it is. But with PCI and USB we have a search mechanism which
>>>>>>> can be used instead.
>>>>>>>
>>>>>>> The patch Marek submitted does not seems at all desirable to me.
>>>>>>
>>>>>> Can you explain why ?
>>>>>
>>>>> We already have a compatible string as the standard way to attach
>>>>> drivers to devices.
>>>>>
>>>>> For PCI, we already have PCI_DEVICE() and friends for when we can
>>>>> attach a driver for a PCI device without using a compatible string.
>>>>>
>>>>> Both of these are defined in the DT specification.
>>>>>
>>>>> The patch seems to be a rework of PCI_DEVICE() and I cannot why it is necessary.
>>>>>
>>>>>>
>>>>>>> I would like to see what Bin proposes.
>>>>>>
>>>>>> Me too, so far I only see "not Marek's patch" and no real alternative.
>>>>>
>>>>> Bin, do you have a patch you can share?
>>>>
>>>> No, I don't have any patch series for now, although I offered to work
>>>> on a series to implement my proposal. I haven't started it as I wanted
>>>> to hear your thoughts. The proposal I made is to satisfy the
>>>> requirement that Marek insisted on. Basically Marek thought current DM
>>>> PCI implementation is wrong to ask for a "compatible" string of a PCI
>>>> device in the device tree, because he thought adding "compatible" to
>>>> DT is invalid and Linux does not do that either. While I disagree we
>>>> have to 100% follow Linux's implementation, I am still open for any
>>>> possible design changes, if that's the preferable practice in U-Boot
>>>> (but we have to make it clear and document this officially somewhere).
>>>>
>>>> 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.

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.

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

>
>>>> * Assign the DT node to the bound device in pci_find_and_bind_driver()
>>>> if there is a valid PCI "reg" encoding for a specific PCI device in
>>>> the device tree
>>>
>>> This is what this patch does. And in fact, I have real hardware which
>>> needs this patch to be useful and on which I can test if this works.
>>>
>>
>> You can do this in your own fork. Nothing prevents you from doing that.
>
> I am at loss for words, really.
>
>>>> * 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 ?
>
> 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. Second, doing such is necessary to make
the design clean and complete. 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.

Regards,
Bin