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

[Marek Vasut]

On 09/21/2018 11:08 AM, Bin Meng wrote:
> 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.

So how would the test look like ? I am asking because even if you do
bind the driver here, the bind() function is called with a lot of
missing stuff, like parent_plat_data (so dm_pci_*() calls do not work
there), the DT node is not associated yet. Oh, and the swap-case driver
is never probe()d, so you cannot perform the test in probe either ...

-- 
Best regards,
Marek Vasut