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

Tue Aug 21 10:28:45 UTC 2018

On 08/21/2018 09:16 AM, Bin Meng wrote:
> 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.

_some_ select ones, and that is a _very_ important distinction.

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

Because of the USB PHY , which is attached to that device and can not be
probed on the PCI bus, unlike the device itself ?

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

Because the same device with the same PCI ID can be used without that PHY .

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

Well, see above for why this approach doesn't work.

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

OK, so basically you didn't even try this patch to see if it actually
has any impact anywhere. Sigh ...

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

That would be invalid DT, since you won't be able to match on the BFD.
But that check can be added in V2.

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

Then leave them as is. But don't request unrelated cleansup from me
either. I really do not understand why I should do those cleanups to get
in an completely unrelated patch, that's just nonsense.

[...]

-- 
Best regards,
Marek Vasut