[U-Boot] [PATCH] core: ofnode: Add ofnode_pci_get_devfn()

[Simon Glass]

Hi Stefan,

On Thu, 31 Jan 2019 at 03:45, Stefan Roese <sr at denx.de> wrote:
>
> Hi Simon,
>
> On 31.01.19 11:04, Simon Glass wrote:
> > Hi Stefan,
> >
> > On Tue, 22 Jan 2019 at 02:36, Stefan Roese <sr at denx.de> wrote:
> >>
> >> Hi Simon,
> >>
> >> (added Bin, whom I forgot in this PCI patches)
> >>
> >> On 21.01.19 19:15, Simon Glass wrote:
> >>> On Sat, 19 Jan 2019 at 00:46, Stefan Roese <sr at denx.de> wrote:
> >>>>
> >>>> This function will be used by the Marvell Armada XP/38x PCIe driver,
> >>>> which is moved to DM right now. It's mostly copied from the Linux
> >>>> version.
> >>>>
> >>>> Signed-off-by: Stefan Roese <sr at denx.de>
> >>>> Cc: Simon Glass <sjg at chromium.org>
> >>>> ---
> >>>>    drivers/core/ofnode.c | 12 ++++++++++++
> >>>>    include/dm/ofnode.h   | 11 +++++++++++
> >>>>    2 files changed, 23 insertions(+)
> >>>
> >>> The code to do this right now is in pci_uclass_child_post_bind(). Do
> >>> you think you could break that out into a pci_... function that you
> >>> can call from your new function?
> >>
> >> Sure, I'll give it a try. While working on it, I noticed this
difference
> >> in the current DEVFN usage in this pci_uclass_child_post_bind()
> >> implementation:
> >>
> >>                  pplat->devfn = addr.phys_hi & 0xff00;
> >>
> >> So, pplat->devfn uses bits 15-8 for DEVFN. Linux uses this definition
> >> instead:
> >>
> >> include/uapi/linux/pci.h:
> >> /*
> >>    * The PCI interface treats multi-function devices as independent
> >>    * devices.  The slot/function address of each device is encoded
> >>    * in a single byte as follows:
> >>    *
> >>    *     7:3 = slot
> >>    *     2:0 = function
> >>    */
> >> #define PCI_DEVFN(slot, func)   ((((slot) & 0x1f) << 3) | ((func) &
0x07))
> >> #define PCI_SLOT(devfn)         (((devfn) >> 3) & 0x1f)
> >> #define PCI_FUNC(devfn)         ((devfn) & 0x07)
> >>
> >> So here devfn uses bits 7-0 instead, which is what the MVEBU PCIe
> >> driver also expects. Do you know why there is this different
> >> implementation for devfn here in pci_uclass_child_post_bind()?
> >> Is this a bug which I should fix by shifting the bits correspondingly?
> >
> > Yes I think it should be consistent. I hope this is a simple fix and
> > does not affect the drivers much.
>
> As you might have spotted in my later patch version (e.g. v3), I've
> moved my patch to use the U-Boot "devfn" usage in bits 15-8. I've
> commented this in the driver (ported from Linux) and also added a
> comment about this in the function header of pci_get_devfn():

OK.

>
> +/**
> + * pci_get_devfn() - Extract the devfn from fdt_pci_addr of the device
> + *
> + * Get devfn from fdt_pci_addr of the specifified device
> + *
> + * @dev:       PCI device
> + * @return devfn in bits 15...8 if found, -ENODEV if not found
>
> This seemed to be the least intrusive option. I hesitate to completely
> move to the Linux "devfn" usage in bits 7-0, as this might have serious
> problems with the current U-Boot implementation in its drivers and
> interfaces.

Yes that sounds best..

>
> One thing we might do though, is to add a comment about this difference
> in the U-Boot PCI_DEVFN macro definition. Should I generate a patch for
> this?

Yes that's a good idea.

>
> >>
> >>> Also I had a look at the code you wrote that calls this. Ideally we
> >>> would have the PCIe nodes have their own driver so that driver model
> >>> will automatically bind them, but I am not sure that is feasible.
> >>
> >> IIRC, this approach of the MISC bind function creating the PCI device
> >> instances for the child nodes was suggested by you when I wrote the
> >> Marvell MVPP2 ethernet driver (drivers/net/mvpp2.c) a few years ago.
> >
> > OK. Can these be added to the device tree, or are they bound
dynamically?
>
> The child nodes are already in the device tree. Each child represents
> a PCIe root port with its own register base. But the compatible
> property resides in the parent node. Please note that I don't want to
> make any changes to the DT, as this is the original Linux version.

OK, fair enough.

Regards,
Simon