[U-Boot] [PATCH V2 1/2] pci: Support parsing PCI controller DT subnodes

[Simon Glass]

Hi Marek,

On 30 August 2018 at 07:42, Marek Vasut <marek.vasut at gmail.com> wrote:
> On 08/30/2018 03:32 PM, Bin Meng wrote:
>> Hi Marek,
>>
>> On Thu, Aug 30, 2018 at 1:07 AM Marek Vasut <marek.vasut at gmail.com> wrote:
>>>
>>> On 08/29/2018 05:15 PM, Bin Meng wrote:
>>>> +Simon
>>>>
>>>> Hi Marek,
>>>>
>>>> On Wed, Aug 29, 2018 at 10:22 PM Marek Vasut <marek.vasut at gmail.com> wrote:
>>>>>
>>>>> On 08/24/2018 08:27 PM, Marek Vasut 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>
>>>>>> Cc: Tom Rini <trini at konsulko.com>
>>>>>
>>>>> Well, bump ?
>>>>>
>>>>> This is the only missing patch to get my hardware working properly.
>>>>
>>>> I don't think we ever had an agreement on the v1 patch. Simon had a
>>>> long email that pointed out what Linux does seems like a 'fallback' to
>>>> find a node with no compatible string.
>>>>
>>>> Back to this, if we have to go with this way, please create a test
>>>> case to cover this scenario.
>>>
>>> The fact that it works on a particular board is not tested enough?
>>> Do we need a custom, special, synthetic test ?
>>>
>>
>> I believe that's always been the requirement against the DM code
>> changes. I was requested in the past when I changed something in the
>> DM and I see other people were asked to do so. Like Alex said, it does
>> not mean this patch was not tested enough, but to ensure future
>> commits won't break this.
>
> So, do you have any suggestion how to implement this test ? It seems
> Alex posed the same question. It doesn't seem to be trivial in the
> context of sandbox.

I suppose you need a PCI_DEVICE() declaration for sandbox, with an
associated DT node and no compatible string. Then check that you can
locate the device and that it read a DT property correctly.

>
>>> Anyway, any feedback on the patch ? Did you test it ? I again only see
>>> "do this random stuff and that random stuff" , but zero actual feedback.
>>>
>>
>> If "this and that random stuff" means test case I asked for, please
>> check my proposal on the v1 patch thread which indicated that a proper
>> test case should be created. You seems to have missed that.
>
> So, any feedback on this actual patch ?

What is 'potention'?

Is there any check needed that it does not attach the same DT node to
two different devices? Or perhaps that cannot happen, since we
shouldn't expect two nodes to share a BDF?

I think it looks OK, assuming this is the way we want to go.

Regards,
Simon