[U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
Marek Vasut
marek.vasut at gmail.com
Tue Aug 21 18:56:37 UTC 2018
On 08/21/2018 08:29 PM, Simon Glass wrote:
> Hi Marek,
>
> On 21 August 2018 at 12:26, Marek Vasut <marek.vasut at gmail.com> wrote:
>>
>> On 08/21/2018 07:32 PM, Simon Glass wrote:
>>> Hi Bin,
>>>
>>> On 20 August 2018 at 21:46, Bin Meng <bmeng.cn at gmail.com> wrote:
>>>> Hi Simon,
>>>>
>>>> On Tue, Aug 21, 2018 at 3:29 AM, Simon Glass <sjg at chromium.org> wrote:
>>>>> Hi Marek,
>>>>>
>>>>> On 20 August 2018 at 12:42, Marek Vasut <marek.vasut at gmail.com> wrote:
>>>>>>
>>>>>> On 08/20/2018 06:57 PM, Simon Glass wrote:
>>>>>>> Hi Bin,
>>>>>>>
>>>>>>> On 16 August 2018 at 19:51, Bin Meng <bmeng.cn at gmail.com> wrote:
>>>>>>>> Hi Marek,
>>>>>>>>
>>>>>>>> On Thu, Aug 16, 2018 at 7:47 PM, Marek Vasut <marek.vasut at gmail.com> wrote:
>>>>>>>>> On 08/15/2018 01:25 PM, Tom Rini wrote:
>>>>>>>>>> On Wed, Aug 15, 2018 at 06:19:25PM +0800, Bin Meng wrote:
>>>>>>>>>>> Hi Marek,
>>>>>>>>>>>
>>>>>>>>>>> On Wed, Aug 15, 2018 at 5:22 PM, Marek Vasut <marek.vasut at gmail.com> wrote:
>>>>>>>>>>>> On 08/14/2018 11:40 AM, Bin Meng wrote:
>>>>>>>>>>>>> Hi Marek,
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Tue, Aug 14, 2018 at 4:55 PM, Marek Vasut <marek.vasut at gmail.com> wrote:
>>>>>>>>>>>>>> On 08/14/2018 03:46 AM, Bin Meng wrote:
>>>>>>>>>>>>>>> Hi Marek,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On Mon, Aug 13, 2018 at 9:46 PM, Marek Vasut <marek.vasut at gmail.com> wrote:
>>>>>>>>>>>>>>>> On 08/13/2018 04:24 AM, Bin Meng wrote:
>>>>>>>>>>>>>>>>> Hi Marek,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> On Fri, Aug 10, 2018 at 8:38 PM, Marek Vasut <marek.vasut at gmail.com> wrote:
>>>>>>>>>>>>>>>>>> On 08/10/2018 02:01 PM, Tom Rini wrote:
>>>>>>>>>>>>>>>>>>> On Wed, Aug 08, 2018 at 09:37:25PM +0200, Marek Vasut 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.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> DT should but isn't always OS agnostic. DTS files that reside in the
>>>>>>>>>>>>>>>>>>> Linux Kernel are in practice is Linux-centric with the expectation that
>>>>>>>>>>>>>>>>>>> even if you could solve a given problem with valid DTS changes you make
>>>>>>>>>>>>>>>>>>> whatever is parsing it do additional logic instead. That,
>>>>>>>>>>>>>>>>>>> approximately, is what your patch is doing. If you added some HW
>>>>>>>>>>>>>>>>>>> description information to the dtsi file everything would work as
>>>>>>>>>>>>>>>>>>> expected as your DTS is describing the hardware and U-Boot is reading
>>>>>>>>>>>>>>>>>>> that description and figuring out what to do with it.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Yes, you need additional logic to match the PCI controller subnode in DT
>>>>>>>>>>>>>>>>>> with PCI device BFD, that's expected. You do NOT need extra compatibles,
>>>>>>>>>>>>>>>>>> the PCI bus gives you enough information to match a driver on them. In
>>>>>>>>>>>>>>>>>> fact, adding a compatible can interfere with this matching.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Please, read U-Boot's doc/driver-model/pci-info.txt. You really don't
>>>>>>>>>>>>>>>>> understand current implementation in U-Boot. In short, U-Boot supports
>>>>>>>>>>>>>>>>> two scenarios for PCI driver binding:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> That documentation is wrong and needs to be fixed. The compatible is
>>>>>>>>>>>>>>>> optional.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> No it is not wrong. The documentation reflects the update-to-date
>>>>>>>>>>>>>>> U-Boot support of PCI bus with DM.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Which is incomplete, as it cannot parse subnodes without compatible strings.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> No, it's by design, as I said many times. It can support parsing
>>>>>>>>>>>>> subnodes with a "compatible" string existence.
>>>>>>>>>>>>
>>>>>>>>>>>> It can support parsing subnodes with a "compatible" string existence AND
>>>>>>>>>>>> It can NOT support parsing subnodes without a "compatible" string
>>>>>>>>>>>> existence THUS It is incomplete.
>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> - 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 can choose either two when you support PCI devices on your board,
>>>>>>>>>>>>>>>>> but you cannot mix both support together and make them a mess. In this
>>>>>>>>>>>>>>>>> patch, you hacked pci_find_and_bind_driver() which is the 2nd scenario
>>>>>>>>>>>>>>>>> to support the 1st scenario.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Again, the DT contains all the required information to bind the node and
>>>>>>>>>>>>>>>> the driver instance. Clearly, we need option 3 for this.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Then that's a new design proposal. Anything that wants to mess up
>>>>>>>>>>>>>>> current design is a hack.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> That means every single patch anyone submits is now a hack ? Please ...
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> I never said "every single patch anyone submits is now a hack". "You
>>>>>>>>>>>>> are inserting words into my mouth and I dislike that." I said your
>>>>>>>>>>>>> current patch is against the design, and mess up current design which
>>>>>>>>>>>>> is a hack.
>>>>>>>>>>>>
>>>>>>>>>>>> But then every patch which changes the behavior is against "the design"
>>>>>>>>>>>> and thus is a hack. Ultimately, most improvements would be considered a
>>>>>>>>>>>> hack.
>>>>>>>>>>>
>>>>>>>>>>> No it depends. For this case, there are two options that DM PCI
>>>>>>>>>>> currently provides. You created a 3rd option that bring option 1 and 2
>>>>>>>>>>> together in a mixed way, yet without any documenting and additional
>>>>>>>>>>> other changes. If you posted such changes in a series and have all
>>>>>>>>>>> stuff well considered, I would not consider it a hack, but a proposed
>>>>>>>>>>> design change.
>>>>>>>>>>
>>>>>>>>>> Also, the design document is not immutable and can and should be updated
>>>>>>>>>> as needed to match changes in the code.
>>>>>>>>>
>>>>>>>>> So what is the conclusion here ? Patch the design document and apply
>>>>>>>>> this patch as is ?
>>>>>>>>>
>>>>>>>>
>>>>>>>> I think we should see Simon's comments before we move forward. The
>>>>>>>> proposal I made before should come in a series, not just
>>>>>>>> documentation.
>>>>>>>
>>>>>>> This thread is too long :-)
>>>>>>>
>>>>
>>>> Yes, too long discussion :)
>>>>
>>>>>>> From what I understand, Marek and Bin are discussing whether a
>>>>>>> compatible string is needed to bind a driver.
>>>>>>>
>>>>>>> Generally it is. But with PCI and USB we have a search mechanism which
>>>>>>> can be used instead.
>>>>>>>
>>>>>>> The patch Marek submitted does not seems at all desirable to me.
>>>>>>
>>>>>> Can you explain why ?
>>>>>
>>>>> We already have a compatible string as the standard way to attach
>>>>> drivers to devices.
>>>>>
>>>>> For PCI, we already have PCI_DEVICE() and friends for when we can
>>>>> attach a driver for a PCI device without using a compatible string.
>>>>>
>>>>> Both of these are defined in the DT specification.
>>>>>
>>>>> The patch seems to be a rework of PCI_DEVICE() and I cannot why it is necessary.
>>>>>
>>>>>>
>>>>>>> I would like to see what Bin proposes.
>>>>>>
>>>>>> Me too, so far I only see "not Marek's patch" and no real alternative.
>>>>>
>>>>> Bin, do you have a patch you can share?
>>>>
>>>> No, I don't have any patch series for now, although I offered to work
>>>> on a series to implement my proposal. I haven't started it as I wanted
>>>> to hear your thoughts. The proposal I made is to satisfy the
>>>> requirement that Marek insisted on. Basically Marek thought current DM
>>>> PCI implementation is wrong to ask for a "compatible" string of a PCI
>>>> device in the device tree, because he thought adding "compatible" to
>>>> DT is invalid and Linux does not do that either. While I disagree we
>>>> have to 100% follow Linux's implementation, I am still open for any
>>>> possible design changes, if that's the preferable practice in U-Boot
>>>> (but we have to make it clear and document this officially somewhere).
>>>>
>>>> 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
>>>> * Assign the DT node to the bound device in pci_find_and_bind_driver()
>>>> if there is a valid PCI "reg" encoding for a specific PCI device in
>>>> the device tree
>>>> * 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
>>>>
>>>
>>> Thanks very much for all the info. But I don't understand why we want
>>> to remove compatible strings? They are part of the DT specification.
>>
>> Unrelated orthogonal cleanup / optimization IMO.
>
> So can you just add a compatible string and everything works?
No, because that makes no sense. The code can very well match the driver
on the PCI IDs or classes, the DT node is supplementary information and
it needs to be made available to the matched driver if present in the
DT. The DT node is matched on the BFD (the reg property in the DT).
The compatible string would only add redundancy and can in fact be
harmful in case the PCI device could be replaced (ie. swapping USB EHCI
controller for USB xHCI controller while retaining the USB PHY ; the
controller itself could be discovered on the PCI bus, but the PHY can
not and must be described in the DT).
--
Best regards,
Marek Vasut
More information about the U-Boot
mailing list