From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Glass Date: Fri, 1 Feb 2019 23:05:55 -0700 Subject: [U-Boot] [PATCH] core: ofnode: Add ofnode_pci_get_devfn() In-Reply-To: <97d0e7b0-0249-1489-bd24-491c993f7370@denx.de> References: <20190118114642.19412-1-sr@denx.de> <1c1532ba-5895-271a-66a4-8c6daa3d9e84@denx.de> <97d0e7b0-0249-1489-bd24-491c993f7370@denx.de> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Stefan, On Thu, 31 Jan 2019 at 03:45, Stefan Roese wrote: > > Hi Simon, > > On 31.01.19 11:04, Simon Glass wrote: > > Hi Stefan, > > > > On Tue, 22 Jan 2019 at 02:36, Stefan Roese 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 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 > >>>> Cc: Simon Glass > >>>> --- > >>>> 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