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

[Marek Vasut]

On 09/18/2018 03:52 PM, Simon Glass wrote:
> Hi Marek,
> 
> On 18 September 2018 at 13:36, Marek Vasut <marek.vasut at gmail.com> wrote:
>>
>> On 09/14/2018 06:41 AM, Simon Glass wrote:
>>> Hi Marek,
>>>
>>> On 10 September 2018 at 01:38, Marek Vasut <marek.vasut at gmail.com> wrote:
>>>>
>>>> On 09/02/2018 03:07 AM, Simon Glass wrote:
>>>>> Hi Marek,
>>>>
>>>> Hi,
>>>>
>>>>> On 1 September 2018 at 16:45, Marek Vasut <marek.vasut at gmail.com> wrote:
>>>>>>
>>>>>> On 09/01/2018 11:50 PM, Simon Glass wrote:
>>>>>>> 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.
>>>>>>
>>>>>> Is there any example of this stuff already ?
>>>>>
>>>>> See the bottom of swap_case.c. You might be able to add a new one of those,
>>>>>
>>>>> If you look at pci-controller2 in test.dts it has a device with a
>>>>> compatible string. You could try adding a second device with no
>>>>> compatible string.
>>>>
>>>> And how does that test anything ?
>>>
>>> You can test that your code actually attaches the DT node to the
>>> probed device. Without you code the test would fail. Wit it, it would
>>> pass.
>>
>> Well it won't, because the sandbox swap_case.c requires the compatible.
>> This all seems like a big hack to support virtual PCI devices.
>>
>> The driver binds with a compatible and then pins the read/write config
>> reg accessors to emulate their return values. Those include PCI IDs. So
>> you cannot instantiate virtual PCI device without this compatible string
>> and thus also cannot write such a test easily.
>>
>> Now I also understand where this whole discussion about compatible
>> strings came from though.
> 
> The compatible string is needed for the emulation driver but not for
> the thing that connects to it. However as things stand you can't
> attach an emulator to a bus without nesting it under the device which
> it attaches to.
> 
> I suspect the best answer is to move the emulator so it is a direct
> child of the bus. You would need to update sandbox_pci_get_emul() to
> call device_find_first_child() on 'bus' instead of 'dev'.

Sounds to me _way_ out of scope for this patchset.

-- 
Best regards,
Marek Vasut