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

Bin Meng
2018-08-20T07:18:42Z

Hi Marek,

On Fri, Aug 17, 2018 at 6:27 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> On 08/17/2018 03:51 AM, Bin Meng wrote:
>> Hi Marek,
>>
>> On Thu, Aug 16, 2018 at 7:47 PM, Marek Vasut <marek.vasut@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@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@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@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@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@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@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@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@gmail.com>
>>>>>>>>>>>>>>>>>>>> Cc: Simon Glass <sjg@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.
>
> Your proposal mostly covers sandbox and that can be done separately.
> My proposal is to update documentation and fix the code.

There are more changes than the sandbox. Given the participants in
this thread, I suspect nobody else would care the design if you had
only changed whatever you like in a patch or two. Then we will end up
leaving such incomplete design in the source tree and confusing people
maybe for years until someone tries to fix up the things all together.
I hope we can do things right from start.

Regards,
Bin