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

[Bin Meng]

Hi Marek,

On Wed, Aug 22, 2018 at 5:57 PM, Marek Vasut <marek.vasut at gmail.com> wrote:
> On 08/22/2018 04:14 AM, Bin Meng wrote:
> [...]
>>>>> 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 ?
>>>
>>
>> You can create a new EHCI PCI driver to match your USB EHCI
>> controller's vendor id/device id, and do any additional PHY setup in
>> that driver. BTW what's the vendor id and device id of the EHCI
>> controller on your board? I can have a look up at the PCI database.
>
> You can not.
>
> 1033:00e0 -- https://pci-ids.ucw.cz/read/PC/1033/00e0
>
> You can buy that controller as a discrete chip on a PCI card or have one
> in an SoC on a PCI bus, I have both.

Hardware vendors like to create such fantastic stuff in their SoCs,
sigh. So how about subvendor and subdevice id? The embedded one can't
be the same as the the discrete one. If they are really all the same,
then why can't we add a specific compatible string to describe such
device? The compatible idea was invented to describe devices that
cannot be discovered via some probeable ways.

>
>>>> 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.
>>
>> Please explain why it does not work. I see Intel e1000 driver in the
>> Linux kernel has lots of PHY configuration within the driver. It does
>> not use any external PHY (eg: Marvell 88exxx) driver that Linux
>> already has. The Ethernet PHY are not probeable on the PCI bus too.
>
> That's a different thing, the ethernet MACs have an internal MDIO
> interface and you can scan the MDIO bus and read out PHY registers 0/1
> to get the PHY ID.
>

Yes, I know. But my point is that Linux drivers are not always
consistent. There is diversity and Linux is fine with that. There is
also diversity when it comes to U-Boot PCI support and we have 2
supported PCI scenarios, the 1st of which can satisfy your use case.

> This does not apply to this particular usecase here. In this case, the
> controller can be either embedded or discrete, with PHY or without PHY.
>

Does the embedded one have no register that can identify the PHY presence?

>>>>> 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 ...
>>>
>>
>> I tend not to test a patch that is still under discussion to save some
>> time. As there are several x86 boards to test with, along with
>> sandbox. BTW you can test sandbox yourself.
>
> I am positive this has nothing to do with sandbox, so I will not do
> that. If you think this patch has some potential impact on sandbox,
> please detail that or drop the sandbox topic from the discussion.
>
>>>> 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.
>>
>> "Leave them as is", so what do we want to tell other PCI device driver
>> developers in the community? To use whatever way they like? Eventually
>> you created a 3rd scenario.
>
> The message right now is there there is massive unwillingness to take
> PCI core changes, the current discussion just drags on and on and is
> just tiring, without any progress or constructive feedback. The only
> input I see is a mix of requests for clean-ups of unrelated DTs and
> drivers, which I cannot test and which can be done independently of this
> patch. It all feels like stalling to me, but I don't understand what the
> real goal is.
>
> And regarding "leave them as is", yes, asking developers to clean up
> random unrelated part of U-Boot, just because they want to get a core
> patch in, doesn't make sense. If you want to send cleanups for your x86
> or sandbox stuff, fine by me.
>

Regards,
Bin