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

[Marek Vasut]

On 08/09/2018 11:41 AM, Bin Meng wrote:

[...]

>>>>> Sorry this is a hack to current U-Boot implementation, not fix.
>>>>
>>>> I am waiting for a better solution or suggestion ...
>>>>
>>>>> The fix should be adding "ehci-pci" compatible string in the r8a7794.dtsi.
>>>>
>>>> Wrong. The DT is perfectly valid as is.
>>>>
>>>
>>> I did not say r8a7794.dtsi is invalid. Being valid does not mean it
>>> works everywhere. Being valid only means its syntax follows the DTS
>>> language and does not cause any build error. Adding a "compatible"
>>> string to a DT node is also perfectly valid. See "Binding Guidelines"
>>> in devicetree-specification-v0.2.pdf [1]
>>
>> Hacking up DT to work around bugs in an OS implementation makes it OS
>> specific and this is incorrect. It is the OS that should be fixed, in
>> this case U-Boot.
>>
> 
> It's not a bug of U-Boot implementation. U-Boot DM was designed this
> way.

Then the design is flawed. (and btw. the U-Boot DM was not designed that
way, you can take my word for it :-) )

> If you are in U-Boot, you must follow U-Boot's rule. And I don't
> see current DM is a wrong design.

Wrong, U-Boot is given some hardware to run on and a matching
description, DT. If both are valid and U-Boot cannot operate on them,
then U-Boot is broken and must be fixed.

You cannot claim that just because U-Boot behaves in some way, a
perfectly good hardware and/or it's description is wrong and needs to be
adjusted. That makes no sense.

Just assume the DT is in ROM, it's set in stone and it cannot be
changed. Then the entire discussion below becomes pointless and we can
focus on fixing U-Boot such that it'd be able to parse the DT properly.

>> Keep in mind that the DT may even be stored in ROM and can not be modified.
>>
>>> 4.1 Binding Guidelines
>>> 4.1.1 General Principles
>>> When creating a new devicetree representation for a device, a binding
>>> should be created that fully describes the required properties and
>>> value of the device. This set of properties shall be sufficiently
>>> descriptive to provide device drivers with needed attributes of the
>>> device. Some recommended practices include:
>>> 1. Define a *compatible* string using the conventions described in section 2.3.1
>>> ...
>>
>> Yes, "recommended" . The compatible string is not a hard requirement.
> 
> OK, since you mentioned "recommended", let me paste the bullet 2 from
> the same spec:
> 
> Some recommended practices include:
> 1. Define a compatible string using the conventions described in section 2.3.1.
> 2. Use the standard properties (defined in sections 2.3 and 2.4) as
> applicable for the new device. This usage typically includes the *reg*
> and interrupts properties at a minimum.
> 3. Use the conventions specified in section 4 (Device Bindings) if the
> new device fits into one the DTSpec defined device classes
> ...
> 
> Bullet 2 says: This usage *typically* includes the *reg* and
> interrupts properties at a minimum. That means "reg" is not mandatory.
> 
> And now let's see this patch:
> 
>         df = ofnode_get_addr_size(node, "reg", &size);
> 
> Who says a node must have a "reg" property? Using this as a sign to
> attach to DT is definitely wrong.

If there is no "reg" property, then you won't get a node associated with
the driver instance and that's perfectly fine. Without the "reg" prop,
you cannot figure out which PCI device/function the node describes, so
how could you possibly associate the node with a driver instance ? You
cannot.

>>>> The device sitting at a particular slot/function can very well be ie.
>>>> xhci controller and the DT node would be valid for it too, unless you
>>>> enforce a compatible, which will mess things up.
>>>>
>>>> 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.
>>>>
>>>
>>> No, it's not redundant but complementary to existing PCI enumeration
>>> (vendor/device/class/subclass...) mechanism. Please check "PCI Bus
>>> Binding" specification [2] which defines how we should describe a PCI
>>> device using "compatible" string.
>>>
>>> "compatible" Construct a list of names in most-specific to
>>> least-specific order. The names shall be derived from values of the
>>> Vendor ID, Device ID, Subsystem Vendor ID, Subsystem ID, Revision ID
>>> and Class Code bytes, and shall have the following form, and be placed
>>> in the list in the following order:
>>> pciVVVV,DDDD.SSSS.ssss.RR
>>> pciVVVV,DDDD.SSSS.ssss
>>> pciSSSS,ssss
>>> pciVVVV,DDDD.RR
>>> pciVVVV,DDDD
>>> pciclass,CCSSPP
>>> pciclass,CCSS
>>> ...
>>
>> Where does it say that the "compatible" string is mandatory ? I thought
>> you yourself quoted a paragraph from that spec which says it's
>> recommended, which means optional.
>>
> 
> You can't say adding an optional "compatible" is wrong either. Being
> optional means an implementation can choose to support it according to
> its needs. U-Boot's DM was designed to use "compatible" when it comes
> to device binding.

Please assume the DT is in ROM and it cannot be modified.

Again, that's not how U-Boot DM was designed, DM is independent of DT,
but discussing that is really off-topic here.

>>>> What is needed here is to assign a valid DT node to a driver instance of
>>>> a PCI device if such a matching node exists in DT and that is all this
>>>> patch does.
>>>>
>>>
>>> This patch fixes the wrong place. In pci_bind_bus_devices(), we have
>>> the following codes that firstly check if the device is in DT. If not,
>>> then go on with the driver binding using
>>> vendor/device/class/subclass... mechanism.
>>>
>>> /* Find this device in the device tree */
>>> ret = pci_bus_find_devfn(bus, PCI_MASK_BUS(bdf), &dev);
>>>
>>> /* If nothing in the device tree, bind a device */
>>> if (ret == -ENODEV) {
>>> ...
>>> ret = pci_find_and_bind_driver(bus, &find_id, bdf,
>>>        &dev);
>>> }
>>>
>>> Your patch adds some codes in pci_find_and_bind_driver() to touch DT,
>>> which is not the function supposed to do. Hence I call it a hack.
>>
>> You can call it whatever you want, even repeatedly, but that does not help.
>>
>> So what do you suggest ?
>>
> 
> My suggestion was in the first reply: adding "compatible" string in
> the USB nodes.

NAK, that's a hack to work around broken U-Boot code. Unacceptable, I
will not modify the DT for that reason.

Furthermore, the PCI code should match based on PCI ID or class, forcing
a compatible could cause problems in case someone exchanges the PCI
device and doesn't update the DT, which may not be possible (ROM).
Adding the compatible also defeats the PnP behavior of PCI.

>> Mind you, pci_find_and_bind_driver() seems like a perfectly reasonable
>> place to bind a driver instance and a DT node together in my mind.
>>
>>>>> I disagree DT is OS-agnostic. This are lots of stuff in DT that are
>>>>> OS-specific. eg: there are lots of bindings in DT that requires
>>>>> Linux's device driver framework to work with.
>>>>
>>>> This logic is flawed. If there exists a binding which depends on some
>>>> behavior of specific OS then the binding is likely wrong. That
>>>> specifically does not imply DT is OS-specific. Again, it is not and that
>>>> is by design. The DT must be usable by multiple OSes with very different
>>>> internal design, Solaris, *BSD, Linux, U-Boot to name a few.
>>>
>>> My suggested fix does not add any OS-specific property. It's one of
>>> the basic properties defined by DT. Linux works without "compatible"
>>> in this case that's probably due to Linux was designed to work this
>>> way. But that does NOT justify we cannot add a "compatible" string to
>>> make U-Boot's design work.
>>
>> Hacking up DT to work around bugs in U-Boot PCI code is not an option.
>> If U-Boot cannot parse a valid DT correctly, then U-Boot needs to be fixed.
>>
> 
> Again, I don't see this is a bug in the U-Boot PCI codes. The scenario
> of your board has been supported by U-Boot already. You keep saying
> "Linux can parse DT correctly" and "U-Boot cannot parse a valid DT",
> which makes no sense to me:
> 
> 1. U-Boot can parse DT correctly. Parse means decoding device tree
> information to U-Boot's internal structure.
> 2. U-Boot usage of DT is perfectly valid too. How U-Boot uses these
> information in its internal structure is implementation-specific.

That's good and all, but somehow the PCI controller subnodes are not
associated with the driver in case they are described in the DT. Thus,
U-Boot is not parsing the valid DT completely/correctly.

>> It is not because Linux was designed in any way, it is because Linux can
>> parse DT correctly, including all the details. U-Boot cannot.
>>
> 
> What _all the details_? Are you saying that U-Boot should follow
> Linux's device driver framework, to parse DT and use DT exactly the
> same way as Linux?

No, I am saying U-Boot should be able to extract necessary information
from a valid DT. How that happens is up to U-Boot. This is not happening
currently and thus U-Boot needs to be fixed.

> Sorry this is terribly wrong. Imagine someone
> writes another OS, and all he has is the device tree spec. He follows
> the spec and writes some codes to parse a valid DT, and it's done. How
> his OS makes use of the DT is his design decision and none of the
> device tree specs has hard requiement on it. Of course, using exact
> the same DT as Linux is nice-to-have feature but that's not the reason
> to attack has OS has bugs to parse DT.

There is supposed to be only one DT for all the OSes, which has nothing
to do with Linux or U-Boot or any of those. If some OS cannot parse an
valid DT completely, then it's lacking.

-- 
Best regards,
Marek Vasut