Re: [U-Boot] [PATCH] core: ofnode: Add ofnode_pci_get_devfn()
Hi Simon, On 02.02.19 07:05, Simon Glass wrote: On Thu, 31 Jan 2019 at 03:45, Stefan Roese mailto:s...@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 mailto:s...@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 mailto:s...@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 mailto:s...@denx.de>> > Cc: Simon Glass mailto:s...@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 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] core: ofnode: Add ofnode_pci_get_devfn()
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 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] core: ofnode: Add ofnode_pci_get_devfn()
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(): +/** + * 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. 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? 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. Thanks, Stefan ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] core: ofnode: Add ofnode_pci_get_devfn()
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. > > > 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? Regards, Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] core: ofnode: Add ofnode_pci_get_devfn()
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? 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. Thanks, Stefan ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] core: ofnode: Add ofnode_pci_get_devfn()
Hi Stefan, 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? 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. Regards, Simon > > diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c > index 0e584c12dc..b74b95ff61 100644 > --- a/drivers/core/ofnode.c > +++ b/drivers/core/ofnode.c > @@ -666,6 +666,18 @@ int ofnode_read_pci_vendev(ofnode node, u16 *vendor, u16 > *device) > return -ENOENT; > } > > +int ofnode_pci_get_devfn(ofnode node) > +{ > + u32 reg[5]; > + int error; > + > + error = ofnode_read_u32_array(node, "reg", reg, ARRAY_SIZE(reg)); > + if (error) > + return error; > + > + return (reg[0] >> 8) & 0xff; > +} > + > int ofnode_read_addr_cells(ofnode node) > { > if (ofnode_is_np(node)) > diff --git a/include/dm/ofnode.h b/include/dm/ofnode.h > index d206ee2caa..66d7f8d055 100644 > --- a/include/dm/ofnode.h > +++ b/include/dm/ofnode.h > @@ -608,6 +608,17 @@ int ofnode_read_pci_addr(ofnode node, enum fdt_pci_space > type, > */ > int ofnode_read_pci_vendev(ofnode node, u16 *vendor, u16 *device); > > +/** > + * ofnode_pci_get_devfn() - Get device and function numbers for a device node > + * @ofnode: node to examine > + * > + * Parses a standard 5-cell PCI resource and returns an 8-bit value that can > + * be passed to the PCI_SLOT() and PCI_FUNC() macros to extract the device > + * and function numbers respectively. On error a negative error code is > + * returned. > + */ > +int ofnode_pci_get_devfn(ofnode node); > + > /** > * ofnode_read_addr_cells() - Get the number of address cells for a node > * > -- > 2.20.1 > ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH] core: ofnode: Add ofnode_pci_get_devfn()
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(+) diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c index 0e584c12dc..b74b95ff61 100644 --- a/drivers/core/ofnode.c +++ b/drivers/core/ofnode.c @@ -666,6 +666,18 @@ int ofnode_read_pci_vendev(ofnode node, u16 *vendor, u16 *device) return -ENOENT; } +int ofnode_pci_get_devfn(ofnode node) +{ + u32 reg[5]; + int error; + + error = ofnode_read_u32_array(node, "reg", reg, ARRAY_SIZE(reg)); + if (error) + return error; + + return (reg[0] >> 8) & 0xff; +} + int ofnode_read_addr_cells(ofnode node) { if (ofnode_is_np(node)) diff --git a/include/dm/ofnode.h b/include/dm/ofnode.h index d206ee2caa..66d7f8d055 100644 --- a/include/dm/ofnode.h +++ b/include/dm/ofnode.h @@ -608,6 +608,17 @@ int ofnode_read_pci_addr(ofnode node, enum fdt_pci_space type, */ int ofnode_read_pci_vendev(ofnode node, u16 *vendor, u16 *device); +/** + * ofnode_pci_get_devfn() - Get device and function numbers for a device node + * @ofnode: node to examine + * + * Parses a standard 5-cell PCI resource and returns an 8-bit value that can + * be passed to the PCI_SLOT() and PCI_FUNC() macros to extract the device + * and function numbers respectively. On error a negative error code is + * returned. + */ +int ofnode_pci_get_devfn(ofnode node); + /** * ofnode_read_addr_cells() - Get the number of address cells for a node * -- 2.20.1 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot