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

[Stefan Roese]

Hi Simon,

On 02.02.19 07:05, Simon Glass wrote:
> On Thu, 31 Jan 2019 at 03:45, Stefan Roese <sr at denx.de <mailto: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 <mailto: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 <mailto: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 <mailto:sr at denx.de>>
>  > >>>> Cc: Simon Glass <sjg at chromium.org <mailto: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.

I'll do that with a follow-up patch, to not delay this series any longer
in this merge window.

Thanks,
Stefan