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

[Bin Meng]

Hi Marek,

On Wed, Sep 19, 2018 at 9:28 PM Marek Vasut <marek.vasut at gmail.com> wrote:
>
> On 09/19/2018 11:41 AM, Bin Meng wrote:
> > Hi Marek,
> >
> > On Wed, Sep 19, 2018 at 5:34 PM Marek Vasut <marek.vasut at gmail.com> wrote:
> >>
> >> On 09/19/2018 11:26 AM, Bin Meng wrote:
> >>> Hi Marek,
> >>>
> >>> On Wed, Sep 19, 2018 at 4:21 PM Marek Vasut <marek.vasut at gmail.com> wrote:
> >>>>
> >>>> 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.
> >>>
> >>> Dynamic binding is already supported on Sandbox. I guess Simon may
> >>> have missed the part.
> >>
> >> Well, where is an example of that ? Because I am not seeing one.
> >>
> >
> > I already pointed out in the previous email. In
> > arch/sandbox/dts/test.dts, the 2nd PCI controller has two swap_case
> > devices and the 3rd controller has one.
>
> By "second" you mean pci1: or pci2: ? Because pci1: is second , after
> pci0 . It'd really help if you were clearer in what you refer to.
>

It's pci1. You can see there is no subnode under pci1 there yet if you
type 'pci 1' from the U-Boot shell you see two PCI devices.

> > In swap_case.c, U_BOOT_PCI_DEVICE() is there which is also a clear
> > sign that the driver supports dynamic binding. Of course, the driver
> > supports "compatible" too as you noticed.
>
> Are you talking about sandbox,dev-info DT property here ?

This is the property Sandbox uses to make the dynamic binding work.
You can bypass this. The key here is that swap_case driver supports
both "compatible" and dynamic binding, so you can write test cases to
cover this newly added ofnode scenario.

Regards,
Bin