[U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
Bin Meng
bmeng.cn at gmail.com
Thu Aug 9 09:41:23 UTC 2018
Hi Marek,
On Thu, Aug 9, 2018 at 3:54 PM, Marek Vasut <marek.vasut at gmail.com> wrote:
> On 08/09/2018 04:37 AM, Bin Meng wrote:
>> Hi Marek,
>>
>> On Thu, Aug 9, 2018 at 8:36 AM, Marek Vasut <marek.vasut at gmail.com> wrote:
>>> On 08/09/2018 01:24 AM, Bin Meng wrote:
>>>> Hi Marek,
>>>>
>>>> On Thu, Aug 9, 2018 at 3:37 AM, Marek Vasut <marek.vasut at gmail.com> wrote:
>>>>> On 08/08/2018 05:32 PM, Bin Meng wrote:
>>>>>> Hi Marek,
>>>>>>
>>>>>> On Wed, Aug 8, 2018 at 10:33 PM, Marek Vasut <marek.vasut at gmail.com> wrote:
>>>>>>> On 08/08/2018 03:39 PM, Bin Meng wrote:
>>>>>>>> Hi Marek,
>>>>>>>>
>>>>>>>> On Wed, Aug 8, 2018 at 9:24 PM, Marek Vasut <marek.vasut at gmail.com> wrote:
>>>>>>>>> On 08/08/2018 03:14 PM, Bin Meng wrote:
>>>>>>>>>> Hi Marek,
>>>>>>>>>>
>>>>>>>>>> On Wed, Aug 8, 2018 at 9:03 PM, Marek Vasut <marek.vasut at gmail.com> wrote:
>>>>>>>>>>> The PCI controller can have DT subnodes describing extra properties
>>>>>>>>>>> of particular PCI devices, ie. a PHY attached to an EHCI controller
>>>>>>>>>>> on a PCI bus. This patch parses those DT subnodes and assigns a node
>>>>>>>>>>> to the PCI device instance, so that the driver can extract details
>>>>>>>>>>> from that node and ie. configure the PHY using the PHY subsystem.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas at gmail.com>
>>>>>>>>>>> Cc: Simon Glass <sjg at chromium.org>
>>>>>>>>>>> ---
>>>>>>>>>>> drivers/pci/pci-uclass.c | 14 ++++++++++++++
>>>>>>>>>>> 1 file changed, 14 insertions(+)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
>>>>>>>>>>> index 46e9c71bdf..306bea0dbf 100644
>>>>>>>>>>> --- a/drivers/pci/pci-uclass.c
>>>>>>>>>>> +++ b/drivers/pci/pci-uclass.c
>>>>>>>>>>> @@ -662,6 +662,8 @@ static int pci_find_and_bind_driver(struct udevice *parent,
>>>>>>>>>>> for (id = entry->match;
>>>>>>>>>>> id->vendor || id->subvendor || id->class_mask;
>>>>>>>>>>> id++) {
>>>>>>>>>>> + ofnode node;
>>>>>>>>>>> +
>>>>>>>>>>> if (!pci_match_one_id(id, find_id))
>>>>>>>>>>> continue;
>>>>>>>>>>>
>>>>>>>>>>> @@ -691,6 +693,18 @@ static int pci_find_and_bind_driver(struct udevice *parent,
>>>>>>>>>>> goto error;
>>>>>>>>>>> debug("%s: Match found: %s\n", __func__, drv->name);
>>>>>>>>>>> dev->driver_data = find_id->driver_data;
>>>>>>>>>>> +
>>>>>>>>>>> + dev_for_each_subnode(node, parent) {
>>>>>>>>>>> + phys_addr_t df, size;
>>>>>>>>>>> + df = ofnode_get_addr_size(node, "reg", &size);
>>>>>>>>>>> +
>>>>>>>>>>> + if (PCI_FUNC(df) == PCI_FUNC(bdf) &&
>>>>>>>>>>> + PCI_DEV(df) == PCI_DEV(bdf)) {
>>>>>>>>>>> + dev->node = node;
>>>>>>>>>>> + break;
>>>>>>>>>>> + }
>>>>>>>>>>
>>>>>>>>>> The function pci_find_and_bind_driver() is supposed to bind devices
>>>>>>>>>> that are NOT in the device tree. Adding device tree access in this
>>>>>>>>>> routine is quite odd. You can add the EHCI controller that need such
>>>>>>>>>> PHY subnodes in the device tree and there is no need to modify
>>>>>>>>>> anything I believe. If you are looking for an example, please check
>>>>>>>>>> pciuart0 in arch/x86/dts/crownbay.dts.
>>>>>>>>>
>>>>>>>>> Well this does not work for me, the EHCI PCI doesn't get a DT node
>>>>>>>>> assigned, check r8a7794.dtsi for the PCI devices I use.
>>>>>>>>>
>>>>>>>>
>>>>>>>> I think that's because you don't specify a "compatible" string for
>>>>>>>> these two EHCI PCI nodes.
>>>>>>>
>>>>>>> That's perfectly fine, why should I specify it ? Linux has no problem
>>>>>>> with it either.
>>>>>>>
>>>>>>
>>>>>> Without a "compatible" string, DM does not bind any device in the
>>>>>> device tree to a driver, hence no device node created. This is not
>>>>>> Linux.
>>>>>
>>>>> DT is NOT Linux specific, it is OS-agnostic, DT describes hardware and
>>>>> hardware only. If U-Boot cannot parse DT correctly, U-Boot is broken and
>>>>> must be fixed.
>>>>>
>>>>> This is a fix. If there is a better fix, I am open to it.
>>>>>
>>>>
>>>> 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. If you are in U-Boot, you must follow U-Boot's rule. And I don't
see current DM is a wrong design.
> 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.
>
>>> 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.
>>> 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.
> 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.
> 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? 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.
Regards,
Bin
More information about the U-Boot
mailing list