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

[Simon Glass]

Hi Marek,

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.

>
> >>>> 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'?
>
> potential typo .
>
> > 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 guess it could happen and I didn't find a good solution to this even
> in Linux. The current take on this possibility seems to be "let's live
> with it".

OK.

>
> > I think it looks OK, assuming this is the way we want to go.
> >
> > Regards,
> > Simon
> >
>
>
> --
> Best regards,
> Marek Vasut

Regards,
Simon