[U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
Bin Meng
bmeng.cn at gmail.com
Thu Aug 9 02:37:34 UTC 2018
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]
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
...
> 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
...
> 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.
>> 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.
>
>> As you said, DT is just
>> a standard to describe hardware and hardware only. But there are
>> various methods to describe hardware in DT that's why we have a proper
>> defined bindings in Linux.
>
> defined bindings, yes. In Linux ... no ... the HW is OS-independent, so
> is it's description in DT.
>
>> How OS parses and utilizes these
>> information is completely on their own.
>>
[1] https://github.com/devicetree-org/devicetree-specification/releases/tag/v0.2
[2] http://www.o3one.org/hwdocs/openfirmware/pci_supplement_2_1.pdf
Regards,
Bin
More information about the U-Boot
mailing list