[U-Boot] [PATCH V3 2/2] pci: Update documentation to make 'compatible' string optional

[Bin Meng]

Hi Marek,

On Thu, Sep 20, 2018 at 6:34 PM Marek Vasut <marek.vasut at gmail.com> wrote:
>
> On 09/20/2018 03:55 AM, Bin Meng wrote:
> > Hi Marek,
> >
> > On Wed, Sep 19, 2018 at 9:29 PM Marek Vasut <marek.vasut at gmail.com> wrote:
> >>
> >> On 09/18/2018 04:02 PM, Simon Glass wrote:
> >>> Hi Marek,
> >>
> >> Hi,
> >>
> >>> On 18 September 2018 at 05:47, Marek Vasut <marek.vasut at gmail.com> wrote:
> >>>>
> >>>> On 09/14/2018 06:41 AM, Simon Glass wrote:
> >>>>> Hi Marex,
> >>>>
> >>>> It's Marek btw ...
> >>>>
> >>>>> On 11 September 2018 at 14:58, Marek Vasut <marek.vasut at gmail.com> wrote:
> >>>>>> Reword the documentation to make it clear the compatible string is now
> >>>>>> optional, yet still matching on it takes precedence over PCI IDs and
> >>>>>> PCI classes.
> >>>>>>
> >>>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas at gmail.com>
> >>>>>> Cc: Simon Glass <sjg at chromium.org>
> >>>>>> Cc: Tom Rini <trini at konsulko.com>
> >>>>>> ---
> >>>>>> V3: No change
> >>>>>> V2: New patch
> >>>>>> ---
> >>>>>>  doc/driver-model/pci-info.txt | 14 +++++++++-----
> >>>>>>  1 file changed, 9 insertions(+), 5 deletions(-)
> >>>>>>
> >>>>>> diff --git a/doc/driver-model/pci-info.txt b/doc/driver-model/pci-info.txt
> >>>>>> index e1701d1fbc..14364c5c75 100644
> >>>>>> --- a/doc/driver-model/pci-info.txt
> >>>>>> +++ b/doc/driver-model/pci-info.txt
> >>>>>> @@ -34,11 +34,15 @@ under that bus.
> >>>>>>  Note that this is all done on a lazy basis, as needed, so until something is
> >>>>>>  touched on PCI (eg: a call to pci_find_devices()) it will not be probed.
> >>>>>>
> >>>>>> -PCI devices can appear in the flattened device tree. If they do this serves to
> >>>>>> -specify the driver to use for the device. In this case they will be bound at
> >>>>>> -first. Each PCI device node must have a compatible string list as well as a
> >>>>>> -<reg> property, as defined by the IEEE Std 1275-1994 PCI bus binding document
> >>>>>> -v2.1. Note we must describe PCI devices with the same bus hierarchy as the
> >>>>>> +PCI devices can appear in the flattened device tree. If they do, their node
> >>>>>> +often contains extra information which cannot be derived from the PCI IDs or
> >>>>>> +PCI class of the device. Each PCI device node must have a <reg> property, as
> >>>>>> +defined by the IEEE Std 1275-1994 PCI bus binding document v2.1. Compatible
> >>>>>> +string list is optional and generally not needed, since PCI is discoverable
> >>>>>
> >>>>> I really don't like 'generally not needed'. How about 'generally not
> >>>>> essential'? Or that you can usually avoid it if desired.
> >>>>
> >>>> Must be a language nuance, but the compatible string is really not
> >>>> needed. I am starting to understand where this mindset of "compat
> >>>> strings are generally needed" comes from, which is the design of the
> >>>> virtual PCI devices in sandbox, but that's not the usual case.
> >>>
> >>> Well it's more than that, as I mentioned before. Finding a compatible
> >>> string in the source code is easier, and if we are matching with a DT
> >>> node anyway, makes more sense IMO.
> >>
> >> It's about as easy as finding PCI ID.
> >>
> >> And PCI is a discoverable bus, so using a compatible string is some
> >> obscure edge-case.
> >>
> >>> Anyway since DTs likely come from
> >>> the newly pleasant Linux we'll just end up with what they have there.
> >>> This mostly applies for things like x86 which don't use DT in Linux.
> >>>
> >>>>
> >>>>> I'd like to say that it is optional since U_BOOT_PCI_DEVICE() can be
> >>>>> used to specific the driver based on conditions like the PCI vendor/,
> >>>>> PCI class, etc. If U-Boot does not find a compatible string then it
> >>>>> will search these U_BOOT_PCI_DEVICE() records to find a driver;
> >>>>> assuming it finds one it will then search for the device-tree node
> >>>>> whose reg property matches the bus/device/function of the device, and
> >>>>> attached that node to the device so that it is accessible to the
> >>>>> driver.
> >>>>
> >>>> Can you rephrase it better then ? I can paste it into the docs.
> >>>
> >>> How about:
> >>>
> >>> The compatible string is optional since U_BOOT_PCI_DEVICE() can be
> >>> used to specific
> >>
> >> specify ?
> >>
> >>> the driver based on conditions like the PCI vendor/
> >>> PCI class, etc. If U-Boot does not find a compatible string then it
> >>> will search these U_BOOT_PCI_DEVICE() records to find a driver;
> >>
> >> This implies the compatible string is preferred, it is not.
> >>
> >
> > I think Simon was describing the *current* U-Boot implementation, that
> > "compatible" string is looked up first, then U_BOOT_PCI_DEVICE().
>
> This patch updates the documentation to match reality though.
>

Am I looking at a different pci uclass driver implementation from yours?

Currently in pci_bind_bus_devices(), we have:

/* Find this device in the device tree */
ret = pci_bus_find_devfn(bus, PCI_MASK_BUS(bdf), &dev);

This is "compatible" based driver binding, and it goes first.

And we have:

/* If nothing in the device tree, bind a device */
if (ret == -ENODEV) {
...
ret = pci_find_and_bind_driver(bus, &find_id, bdf,
       &dev);
...
}

this is the dynamic driver binding based vid/pid, etc.

Your patch does not change the fact that "compatible" comes first than
dynamic binding.

Regards,
Bin