[U-Boot] [PATCH v3 0/4] dm: pci: Support pci devices in the device tree with driver model
This is the 3rd attempt to support pci uart devices in the device tree with driver model. The v2 patch series is at [1].
With this series, pci uart now works on Intel Crown Bay with driver model. "dm tree" output below:
=> dm tree Class Probed Name ---------------------------------------- root [ + ] root_driver serial [ ] |-- serial rtc [ ] |-- rtc gpio [ ] |-- gpioa gpio [ ] |-- gpiob spi [ + ] |-- spi spi_flash [ ] | `-- spi-flash@0 pci [ + ] |-- pci pci [ + ] | |-- pcie@17,0 pci [ + ] | | `-- topcliff@0,0 serial [ ] | | |-- uart@a,1 serial [ ] | | |-- uart@a,2 serial [ + ] | | |-- uart@a,3 serial [ ] | | |-- uart@a,4 pci_generic [ ] | | |-- pci_2:0.0 pci_generic [ ] | | |-- pci_2:0.1 pci_generic [ ] | | |-- pci_2:0.2 pci_generic [ ] | | |-- pci_2:2.0 pci_generic [ ] | | |-- pci_2:2.1 pci_generic [ ] | | |-- pci_2:2.2 pci_generic [ ] | | |-- pci_2:2.3 pci_generic [ ] | | |-- pci_2:6.0 pci_generic [ ] | | |-- pci_2:8.0 pci_generic [ ] | | |-- pci_2:8.1 pci_generic [ ] | | |-- pci_2:8.2 pci_generic [ ] | | |-- pci_2:8.3 pci_generic [ ] | | |-- pci_2:a.0 pci_generic [ ] | | |-- pci_2:c.0 pci_generic [ ] | | |-- pci_2:c.2 pci_generic [ ] | | `-- pci_2:c.3 pci_generic [ ] | |-- pci_0:0.0 pci_generic [ ] | |-- pci_0:2.0 pci [ + ] | |-- pci_0:18.0 pci [ + ] | |-- pci_0:19.0 pci [ + ] | |-- pci_0:1a.0 pci_generic [ ] | |-- pci_0:1b.0 pci_generic [ ] | `-- pci_0:1f.0 simple_bus [ + ] `-- cpus cpu [ + ] |-- cpu@0 cpu [ + ] `-- cpu@1
[1] http://lists.denx.de/pipermail/u-boot/2015-August/224603.html
Changes in v3: - Rebase on u-boot-x86/master - Drop v2 patches which were already applied
Bin Meng (4): dm: pci: Optimize pci_uclass_post_bind() dm: core: Test dev->flags after device_probe() in device_probe_child() x86: crownbay: Support Topcliff integrated pci uart devices with driver model dm: pci: Document binding of pci device drivers
arch/x86/dts/crownbay.dts | 15 ++++++--- doc/driver-model/pci-info.txt | 71 +++++++++++++++++++++++++++++++++++++++++-- drivers/core/device.c | 8 +++++ drivers/pci/pci-uclass.c | 7 +++++ 4 files changed, 93 insertions(+), 8 deletions(-)
If there is no pci device listed in the device treee, don't bother scanning the device tree.
Signed-off-by: Bin Meng bmeng.cn@gmail.com
--- Simon, I see the following v2 patches were applied, but don't see you replied the patch thread to mention it's been applied. - x86: fsp: Call fsp_init_phase_pci() in pci_uclass_post_probe() - fdtdec: Fix possible infinite loop in fdtdec_get_pci_vendev() - dm: pci: Save devfn without bus number in pci_uclass_child_post_bind()
I assume these patches look good, so I don't include those patches in the v3 series. Let me know if there is any issue.
Changes in v3: - Rebase on u-boot-x86/master - Drop v2 patches which were already applied
drivers/pci/pci-uclass.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c index 2d12344..b82ab4a 100644 --- a/drivers/pci/pci-uclass.c +++ b/drivers/pci/pci-uclass.c @@ -632,6 +632,13 @@ error: static int pci_uclass_post_bind(struct udevice *bus) { /* + * If there is no pci device listed in the device treee, + * don't bother scanning the device tree. + */ + if (bus->of_offset == -1) + return 0; + + /* * Scan the device tree for devices. This does not probe the PCI bus, * as this is not permitted while binding. It just finds devices * mentioned in the device tree.
On Sun, Aug 23, 2015 at 4:32 PM, Bin Meng bmeng.cn@gmail.com wrote:
If there is no pci device listed in the device treee, don't bother scanning the device tree.
Signed-off-by: Bin Meng bmeng.cn@gmail.com
Sorry, just found a typo of this patch and sent a respin one @ http://patchwork.ozlabs.org/patch/509849/
Regards, Bin
The device might have already been probed during the call to device_probe() on its parent device (e.g. PCI bridge devices). Test the flags again so that we don't mess up the device.
Signed-off-by: Bin Meng bmeng.cn@gmail.com ---
Changes in v3: None
drivers/core/device.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/drivers/core/device.c b/drivers/core/device.c index e23a872..7a62812 100644 --- a/drivers/core/device.c +++ b/drivers/core/device.c @@ -266,6 +266,14 @@ int device_probe_child(struct udevice *dev, void *parent_priv) goto fail; }
+ /* + * The device might have already been probed during the call to + * device_probe() on its parent device (e.g. PCI bridge devices). + * Test the flags again so that we don't mess up the device. + */ + if (dev->flags & DM_FLAG_ACTIVATED) + return 0; + seq = uclass_resolve_seq(dev); if (seq < 0) { ret = seq;
Hi Bin,
On 23 August 2015 at 02:32, Bin Meng bmeng.cn@gmail.com wrote:
The device might have already been probed during the call to device_probe() on its parent device (e.g. PCI bridge devices). Test the flags again so that we don't mess up the device.
Signed-off-by: Bin Meng bmeng.cn@gmail.com
Changes in v3: None
drivers/core/device.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/drivers/core/device.c b/drivers/core/device.c index e23a872..7a62812 100644 --- a/drivers/core/device.c +++ b/drivers/core/device.c @@ -266,6 +266,14 @@ int device_probe_child(struct udevice *dev, void
*parent_priv)
goto fail; }
- /*
- The device might have already been probed during the call to
- device_probe() on its parent device (e.g. PCI bridge devices).
- Test the flags again so that we don't mess up the device.
- */
- if (dev->flags & DM_FLAG_ACTIVATED)
- return 0;
seq = uclass_resolve_seq(dev); if (seq < 0) { ret = seq; -- 1.8.2.1
This seems problematic since presumably we reenter device_probe_child() in this case. We then re-allocate the memory and so there is a memory leak.
Firstly I think this code should go inside the if (dev->parent) part.
Secondly I think we need a way to detect thie re-entry and do the right thing. But it is pretty ugly - we essentially need to skip the memory allocation. Maybe another flag? Ick.
Can you give me your thoughts on this and maybe we can work out a solution that isn't too ugly. The expectation is that probing a parent should not probe its children, so we need to deal with it as a special case. We should probably also put an assert in to detect this problem.
Regards, Simon
Hi Simon,
On Mon, Aug 24, 2015 at 9:04 AM, Simon Glass sjg@chromium.org wrote:
Hi Bin,
On 23 August 2015 at 02:32, Bin Meng bmeng.cn@gmail.com wrote:
The device might have already been probed during the call to device_probe() on its parent device (e.g. PCI bridge devices). Test the flags again so that we don't mess up the device.
Signed-off-by: Bin Meng bmeng.cn@gmail.com
Changes in v3: None
drivers/core/device.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/drivers/core/device.c b/drivers/core/device.c index e23a872..7a62812 100644 --- a/drivers/core/device.c +++ b/drivers/core/device.c @@ -266,6 +266,14 @@ int device_probe_child(struct udevice *dev, void *parent_priv) goto fail; }
- /*
- The device might have already been probed during the call to
- device_probe() on its parent device (e.g. PCI bridge devices).
- Test the flags again so that we don't mess up the device.
- */
- if (dev->flags & DM_FLAG_ACTIVATED)
- return 0;
seq = uclass_resolve_seq(dev); if (seq < 0) { ret = seq; -- 1.8.2.1
This seems problematic since presumably we reenter device_probe_child() in this case. We then re-allocate the memory and so there is a memory leak.
Ah, yes! There is a memory leak.
Firstly I think this code should go inside the if (dev->parent) part.
Secondly I think we need a way to detect thie re-entry and do the right thing. But it is pretty ugly - we essentially need to skip the memory allocation. Maybe another flag? Ick.
I think we can just test these to see if they are NULL? Like this:
/* Allocate private data if requested */ if (drv->priv_auto_alloc_size && !dev->priv) { dev->priv = alloc_priv(drv->priv_auto_alloc_size, drv->flags); if (!dev->priv) { ret = -ENOMEM; goto fail; } }
Can you give me your thoughts on this and maybe we can work out a solution that isn't too ugly. The expectation is that probing a parent should not probe its children, so we need to deal with it as a special case. We should probably also put an assert in to detect this problem.
If probing a parent should not probe its children, I am afraid the pci-uclass driver needs rewritten.
Regards, Bin
In order to make a pci uart device node to be properly bound to its driver, we need make sure its parent node has a compatible string which matches a driver that scans all of its child device nodes in the device tree.
Change all pci bridge nodes under root pci node to use "pci-bridge" compatible driver, as well as corresponding <reg> properties to indicate its devfn. At last, adding "u-boot,dm-pre-reloc" to each of these nodes for driver model to initialize them before relocation.
Signed-off-by: Bin Meng bmeng.cn@gmail.com ---
Changes in v3: None
arch/x86/dts/crownbay.dts | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/arch/x86/dts/crownbay.dts b/arch/x86/dts/crownbay.dts index 3af9cc3..9f00c67 100644 --- a/arch/x86/dts/crownbay.dts +++ b/arch/x86/dts/crownbay.dts @@ -91,7 +91,6 @@ #address-cells = <3>; #size-cells = <2>; compatible = "pci-x86"; - device_type = "pci"; u-boot,dm-pre-reloc; ranges = <0x02000000 0x0 0x40000000 0x40000000 0 0x80000000 0x42000000 0x0 0xc0000000 0xc0000000 0 0x20000000 @@ -100,14 +99,16 @@ pcie@17,0 { #address-cells = <3>; #size-cells = <2>; - compatible = "intel,pci"; - device_type = "pci"; + compatible = "pci-bridge"; + u-boot,dm-pre-reloc; + reg = <0x0000b800 0x0 0x0 0x0 0x0>;
topcliff@0,0 { #address-cells = <3>; #size-cells = <2>; - compatible = "intel,pci"; - device_type = "pci"; + compatible = "pci-bridge"; + u-boot,dm-pre-reloc; + reg = <0x00010000 0x0 0x0 0x0 0x0>;
pciuart0: uart@a,1 { compatible = "pci8086,8811.00", @@ -115,6 +116,7 @@ "pciclass,070002", "pciclass,0700", "x86-uart"; + u-boot,dm-pre-reloc; reg = <0x00025100 0x0 0x0 0x0 0x0 0x01025110 0x0 0x0 0x0 0x0>; reg-shift = <0>; @@ -128,6 +130,7 @@ "pciclass,070002", "pciclass,0700", "x86-uart"; + u-boot,dm-pre-reloc; reg = <0x00025200 0x0 0x0 0x0 0x0 0x01025210 0x0 0x0 0x0 0x0>; reg-shift = <0>; @@ -141,6 +144,7 @@ "pciclass,070002", "pciclass,0700", "x86-uart"; + u-boot,dm-pre-reloc; reg = <0x00025300 0x0 0x0 0x0 0x0 0x01025310 0x0 0x0 0x0 0x0>; reg-shift = <0>; @@ -154,6 +158,7 @@ "pciclass,070002", "pciclass,0700", "x86-uart"; + u-boot,dm-pre-reloc; reg = <0x00025400 0x0 0x0 0x0 0x0 0x01025410 0x0 0x0 0x0 0x0>; reg-shift = <0>;
On 23 August 2015 at 02:33, Bin Meng bmeng.cn@gmail.com wrote:
In order to make a pci uart device node to be properly bound to its driver, we need make sure its parent node has a compatible string which matches a driver that scans all of its child device nodes in the device tree.
Change all pci bridge nodes under root pci node to use "pci-bridge" compatible driver, as well as corresponding <reg> properties to indicate its devfn. At last, adding "u-boot,dm-pre-reloc" to each of these nodes for driver model to initialize them before relocation.
Signed-off-by: Bin Meng bmeng.cn@gmail.com
Changes in v3: None
arch/x86/dts/crownbay.dts | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
Acked-by: Simon Glass sjg@chromium.org
Document how pci devices are bound to device drivers.
Signed-off-by: Bin Meng bmeng.cn@gmail.com
---
Changes in v3: None
doc/driver-model/pci-info.txt | 71 +++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 68 insertions(+), 3 deletions(-)
diff --git a/doc/driver-model/pci-info.txt b/doc/driver-model/pci-info.txt index cf69167..52b4389 100644 --- a/doc/driver-model/pci-info.txt +++ b/doc/driver-model/pci-info.txt @@ -34,9 +34,74 @@ under that bus. Note that this is all done on a lazy basis, as needed, so until something is touched on PCI (eg: a call to pci_find_devices()) it will not be probed.
-PCI devices can appear in the device tree. If they do this serves to specify -the driver to use for the device. In this case they will be bound at -start-up. +PCI devices can appear in the flattened device tree. If they do this serves to +specify the driver to use for the device. In this case they will be bound at +first. Each PCI device node must have a compatible string list as well as a +<reg> property, as defined by the IEEE Std 1275-1994 PCI bus binding document +v2.1. Note we must describe PCI devices with the same bus hierarchy as the +hardware, otherwise driver model cannot detect the correct parent/children +relationship during PCI bus enumeration thus PCI devices won't be bound to +their drivers accordingly. A working example like below: + + pci { + #address-cells = <3>; + #size-cells = <2>; + compatible = "pci-x86"; + u-boot,dm-pre-reloc; + ranges = <0x02000000 0x0 0x40000000 0x40000000 0 0x80000000 + 0x42000000 0x0 0xc0000000 0xc0000000 0 0x20000000 + 0x01000000 0x0 0x2000 0x2000 0 0xe000>; + + pcie@17,0 { + #address-cells = <3>; + #size-cells = <2>; + compatible = "pci-bridge"; + u-boot,dm-pre-reloc; + reg = <0x0000b800 0x0 0x0 0x0 0x0>; + + topcliff@0,0 { + #address-cells = <3>; + #size-cells = <2>; + compatible = "pci-bridge"; + u-boot,dm-pre-reloc; + reg = <0x00010000 0x0 0x0 0x0 0x0>; + + pciuart0: uart@a,1 { + compatible = "pci8086,8811.00", + "pci8086,8811", + "pciclass,070002", + "pciclass,0700", + "x86-uart"; + u-boot,dm-pre-reloc; + reg = <0x00025100 0x0 0x0 0x0 0x0 + 0x01025110 0x0 0x0 0x0 0x0>; + ...... + }; + + ...... + }; + }; + + ...... + }; + +In this example, the root PCI bus node is the "/pci" which matches "pci-x86" +driver. It has a subnode "pcie@17,0" with driver "pci-bridge". "pcie@17,0" +also has subnode "topcliff@0,0" which is a "pci-bridge" too. Under that bridge, +a PCI UART device "uart@a,1" is described. This exactly reflects the hardware +bus hierarchy: on the root PCI bus, there is a PCIe root port which connects +to a downstream device Topcliff chipset. Inside Topcliff chipset, it has a +PCIe-to-PCI bridge and all the chipset integrated devices like the PCI UART +device are on the PCI bus. Like other devices in the device tree, if we want +to bind PCI devices before relocation, "u-boot,dm-pre-reloc" must be declared +in each of these nodes. + +If PCI devices are not listed in the device tree, U_BOOT_PCI_DEVICE can be used +to specify the driver to use for the device. The device tree takes precedence +over U_BOOT_PCI_DEVICE. Plese note with U_BOOT_PCI_DEVICE, only drivers with +DM_FLAG_PRE_RELOC will be bound before relocation. If neither device tree nor +U_BOOT_PCI_DEVICE is provided, the built-in driver (either pci_bridge_drv or +pci_generic_drv) will be used.
Sandbox
On 23 August 2015 at 02:33, Bin Meng bmeng.cn@gmail.com wrote:
Document how pci devices are bound to device drivers.
Signed-off-by: Bin Meng bmeng.cn@gmail.com
Changes in v3: None
doc/driver-model/pci-info.txt | 71
+++++++++++++++++++++++++++++++++++++++++--
1 file changed, 68 insertions(+), 3 deletions(-)
Nice!
Acked-by: Simon Glass sjg@chromium.org
participants (2)
-
Bin Meng -
Simon Glass