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

[Marek Vasut]

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 ?

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

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

-- 
Best regards,
Marek Vasut