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
