Skip to contentSkip to conversation

Further mvebu PCIe patches

PATCH New Russell King - ARM Linux 7 participants 23 messages Oct 3, 2015 – Oct 9, 2015 PCI

lore
Share & tools
lore

Russell King - ARM Linux

author
#0 lore raw

cover Further mvebu PCIe patches

Jason, Thomas,

Here are further PCIe patches which follow on from the previous set of six I sent earlier in September. This set:

* Separates the DT parsing from the use of this parsed data.
* Fixes memory leaks from kasprintf() and refcount leaks of the DT node where we break out from the loop early.
* Uses gpio_set_value_cansleep() so that GPIOs on I2C expanders can be used for the PCIe reset functionality.
* Switch to using devm_kcalloc() instead of devm_kzalloc(), which eliminates the multiplication, moving it into core code.
* Switch to using a gpio descriptor for the reset gpio, which can contain the active level information from DT. (It would be nice if gpiolib automated some of the resource claiming there.)
* Make PERST# vs clock timing to match PCIe specifications. PCIe specs require the clock to be running for 100us prior to releasing reset.
* Add the standard PCIe capability block in root port form to the emulated PCIe configuration block. This allows the PCI layer to identify the "host bridge" as a PCIe device, and allows us to take advantage of the code in drivers/pci/pcie, particularly aspm for link power management.
* As a result of identifying ourselves as a PCIe root port, this eliminates the need to special case accesses to non-slot 0 in the driver; the lack of other "slots" is something which the generic PCI code knows about for PCIe root ports.

 drivers/pci/host/pci-mvebu.c | 404 ++++++++++++++++++++++++++++++++-----------
 1 file changed, 305 insertions(+), 99 deletions(-)
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
#1 lore raw

PATCH 1/9 pci: mvebu: move port parsing and resource claiming to separate function

Move the PCIe port parsing and resource claiming to a separate function in preparation to add proper cleanup of claimed resources.

Signed-off-by Russell King <[email protected]>
---
 drivers/pci/host/pci-mvebu.c | 130 ++++++++++++++++++++++++-------------------
 1 file changed, 74 insertions(+), 56 deletions(-)
drivers/pci/host/pci-mvebu.c +74 -56
index 19144ed7bdad..13ab0350f7fb 100644
@@ -928,6 +928,76 @@ static int mvebu_pcie_resume(struct device *dev)
 	return 0;
 }
 
+static int mvebu_pcie_parse_port(struct mvebu_pcie *pcie,
+	struct mvebu_pcie_port *port, struct device_node *child)
+{
+	struct device *dev = &pcie->pdev->dev;
+	enum of_gpio_flags flags;
+	int ret;
+
+	port->pcie = pcie;
+
+	if (of_property_read_u32(child, "marvell,pcie-port", &port->port)) {
+		dev_warn(dev, "ignoring %s, missing pcie-port property\n",
+			 of_node_full_name(child));
+		goto skip;
+	}
+
+	if (of_property_read_u32(child, "marvell,pcie-lane", &port->lane))
+		port->lane = 0;
+
+	port->name = kasprintf(GFP_KERNEL, "pcie%d.%d", port->port, port->lane);
+
+	port->devfn = of_pci_get_devfn(child);
+	if (port->devfn < 0)
+		goto skip;
+
+	ret = mvebu_get_tgt_attr(dev->of_node, port->devfn, IORESOURCE_MEM,
+				 &port->mem_target, &port->mem_attr);
+	if (ret < 0) {
+		dev_err(dev, "%s: cannot get tgt/attr for mem window\n",
+			port->name);
+		goto skip;
+	}
+
+	if (resource_size(&pcie->io) != 0)
+		mvebu_get_tgt_attr(dev->of_node, port->devfn, IORESOURCE_IO,
+				   &port->io_target, &port->io_attr);
+	else {
+		port->io_target = -1;
+		port->io_attr = -1;
+	}
+
+	port->reset_gpio = of_get_named_gpio_flags(child, "reset-gpios", 0,
+						   &flags);
+	if (gpio_is_valid(port->reset_gpio)) {
+		port->reset_active_low = flags & OF_GPIO_ACTIVE_LOW;
+		port->reset_name = kasprintf(GFP_KERNEL, "%s-reset",
+					     port->name);
+
+		ret = devm_gpio_request_one(dev, port->reset_gpio,
+					    GPIOF_DIR_OUT, port->reset_name);
+		if (ret) {
+			if (ret == -EPROBE_DEFER)
+				goto err;
+			goto skip;
+		}
+	}
+
+	port->clk = of_clk_get_by_name(child, NULL);
+	if (IS_ERR(port->clk)) {
+		dev_err(dev, "%s: cannot get clock\n", port->name);
+		goto skip;
+	}
+
+	return 1;
+
+skip:
+	ret = 0;
+err:
+	return ret;
+}
+
 static int mvebu_pcie_probe(struct platform_device *pdev)
 {
 	struct mvebu_pcie *pcie;

@@ -980,76 +1050,24 @@ static int mvebu_pcie_probe(struct platform_device *pdev)
 	i = 0;
 	for_each_available_child_of_node(pdev->dev.of_node, child) {
 		struct mvebu_pcie_port *port = &pcie->ports[i];
-		enum of_gpio_flags flags;
-
-		port->pcie = pcie;
 
-		if (of_property_read_u32(child, "marvell,pcie-port",
-					 &port->port)) {
-			dev_warn(&pdev->dev,
-				 "ignoring %s, missing pcie-port property\n",
-				 of_node_full_name(child));
+		ret = mvebu_pcie_parse_port(pcie, port, child);
+		if (ret < 0)
+			return ret;
+		else if (ret == 0)
 			continue;
-		}
 
-		if (of_property_read_u32(child, "marvell,pcie-lane",
-					 &port->lane))
-			port->lane = 0;
-
-		port->name = kasprintf(GFP_KERNEL, "pcie%d.%d",
-				       port->port, port->lane);
-
-		port->devfn = of_pci_get_devfn(child);
-		if (port->devfn < 0)
-			continue;
-
-		ret = mvebu_get_tgt_attr(np, port->devfn, IORESOURCE_MEM,
-					 &port->mem_target, &port->mem_attr);
-		if (ret < 0) {
-			dev_err(&pdev->dev, "%s: cannot get tgt/attr for mem window\n",
-				port->name);
-			continue;
-		}
-
-		if (resource_size(&pcie->io) != 0)
-			mvebu_get_tgt_attr(np, port->devfn, IORESOURCE_IO,
-					   &port->io_target, &port->io_attr);
-		else {
-			port->io_target = -1;
-			port->io_attr = -1;
-		}
-
-		port->reset_gpio = of_get_named_gpio_flags(child,
-						   "reset-gpios", 0, &flags);
 		if (gpio_is_valid(port->reset_gpio)) {
 			u32 reset_udelay = 20000;
 
-			port->reset_active_low = flags & OF_GPIO_ACTIVE_LOW;
-			port->reset_name = kasprintf(GFP_KERNEL, "%s-reset",
-						     port->name);
 			of_property_read_u32(child, "reset-delay-us",
 					     &reset_udelay);
 
-			ret = devm_gpio_request_one(&pdev->dev,
-			    port->reset_gpio, GPIOF_DIR_OUT, port->reset_name);
-			if (ret) {
-				if (ret == -EPROBE_DEFER)
-					return ret;
-				continue;
-			}
-
 			gpio_set_value(port->reset_gpio,
 				       (port->reset_active_low) ? 1 : 0);
 			msleep(reset_udelay/1000);
 		}
 
-		port->clk = of_clk_get_by_name(child, NULL);
-		if (IS_ERR(port->clk)) {
-			dev_err(&pdev->dev, "%s: cannot get clock\n",
-				port->name);
-			continue;
-		}
-
 		ret = clk_prepare_enable(port->clk);
 		if (ret)
 			continue;
Signature · 1 line
2.1.0
#2 lore raw

PATCH 2/9 pci: mvebu: fix memory leaks and refcount leaks

In reply to Russell King - ARM Linux

The mvebu PCI port parsing is weak due to:

1) allocations via kasprintf() were not cleaned up when we encounter an error or decide to skip the port.
2) kasprintf() wasn't checked for failure.
3) of_get_named_gpio_flags() returns EPROBE_DEFER if the GPIO is not present, not devm_gpio_request_one().
4) the of_node was not being put when terminating the loop.

Fix these oversights.

Signed-off-by Russell King <[email protected]>
---
 drivers/pci/host/pci-mvebu.c | 50 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 43 insertions(+), 7 deletions(-)
drivers/pci/host/pci-mvebu.c +43 -7
index 13ab0350f7fb..e8c51bb58e99 100644
@@ -928,6 +928,13 @@ static int mvebu_pcie_resume(struct device *dev)
 	return 0;
 }
 
+static void mvebu_pcie_port_clk_put(void *data)
+{
+	struct mvebu_pcie_port *port = data;
+
+	clk_put(port->clk);
+}
+
 static int mvebu_pcie_parse_port(struct mvebu_pcie *pcie,
 	struct mvebu_pcie_port *port, struct device_node *child)
 {

@@ -946,7 +953,12 @@ static int mvebu_pcie_parse_port(struct mvebu_pcie *pcie,
 	if (of_property_read_u32(child, "marvell,pcie-lane", &port->lane))
 		port->lane = 0;
 
-	port->name = kasprintf(GFP_KERNEL, "pcie%d.%d", port->port, port->lane);
+	port->name = devm_kasprintf(dev, GFP_KERNEL, "pcie%d.%d", port->port,
+				    port->lane);
+	if (!port->name) {
+		ret = -ENOMEM;
+		goto err;
+	}
 
 	port->devfn = of_pci_get_devfn(child);
 	if (port->devfn < 0)

@@ -960,20 +972,29 @@ static int mvebu_pcie_parse_port(struct mvebu_pcie *pcie,
 		goto skip;
 	}
 
-	if (resource_size(&pcie->io) != 0)
+	if (resource_size(&pcie->io) != 0) {
 		mvebu_get_tgt_attr(dev->of_node, port->devfn, IORESOURCE_IO,
 				   &port->io_target, &port->io_attr);
-	else {
+	} else {
 		port->io_target = -1;
 		port->io_attr = -1;
 	}
 
 	port->reset_gpio = of_get_named_gpio_flags(child, "reset-gpios", 0,
 						   &flags);
+	if (port->reset_gpio == -EPROBE_DEFER) {
+		ret = port->reset_gpio;
+		goto err;
+	}
+
 	if (gpio_is_valid(port->reset_gpio)) {
 		port->reset_active_low = flags & OF_GPIO_ACTIVE_LOW;
-		port->reset_name = kasprintf(GFP_KERNEL, "%s-reset",
-					     port->name);
+		port->reset_name = devm_kasprintf(dev, GFP_KERNEL, "%s-reset",
+						  port->name);
+		if (!port->reset_name) {
+			ret = -ENOMEM;
+			goto err;
+		}
 
 		ret = devm_gpio_request_one(dev, port->reset_gpio,
 					    GPIOF_DIR_OUT, port->reset_name);

@@ -990,10 +1011,23 @@ static int mvebu_pcie_parse_port(struct mvebu_pcie *pcie,
 		goto skip;
 	}
 
+	ret = devm_add_action(dev, mvebu_pcie_port_clk_put, port);
+	if (ret < 0) {
+		clk_put(port->clk);
+		goto err;
+	}
+
 	return 1;
 
 skip:
 	ret = 0;
+
+	/* In the case of skipping, we need to free these */
+	devm_kfree(dev, port->reset_name);
+	port->reset_name = NULL;
+	devm_kfree(dev, port->name);
+	port->name = NULL;
+
 err:
 	return ret;
 }

@@ -1052,10 +1086,12 @@ static int mvebu_pcie_probe(struct platform_device *pdev)
 		struct mvebu_pcie_port *port = &pcie->ports[i];
 
 		ret = mvebu_pcie_parse_port(pcie, port, child);
-		if (ret < 0)
+		if (ret < 0) {
+			of_node_put(child);
 			return ret;
-		else if (ret == 0)
+		} else if (ret == 0) {
 			continue;
+		}
 
 		if (gpio_is_valid(port->reset_gpio)) {
 			u32 reset_udelay = 20000;
Signature · 1 line
2.1.0
#3 lore raw

PATCH 2/9 pci: mvebu: fix memory leaks and refcount leaks

Russell,

drivers/pci/host/pci-mvebu.c static int mvebu_pcie_parse_port(struct mvebu_pcie *pcie, · line 1014

Quoted 1 line · Russell King
drivers/pci/host/pci-mvebu.c +1 -0
+	ret = devm_add_action(dev, mvebu_pcie_port_clk_put, port);

I didn't know about devm_add_action(). Definitely very useful for such situations.

drivers/pci/host/pci-mvebu.c static int mvebu_pcie_probe(struct platform_device *pdev) · line 1094

Quoted 12 lines
drivers/pci/host/pci-mvebu.c +4 -2
@@ -1052,10 +1086,12 @@ static int mvebu_pcie_probe(struct platform_device *pdev)
 		struct mvebu_pcie_port *port = &pcie->ports[i];
 
 		ret = mvebu_pcie_parse_port(pcie, port, child);
-		if (ret < 0)
+		if (ret < 0) {
+			of_node_put(child);
 			return ret;
-		else if (ret == 0)
+		} else if (ret == 0) {
 			continue;
+		}

This is not trivial. If I understand correctly, for_each_available_child_of_node() will automatically release the reference on the previous node and take the reference on the new one before entering the loop code. So in the skipping case, we don't need to release the reference as it will be done by the next iteration of the loop, but in the error case, since we are unexpectedly breaking the loop, we need to do it manually.

The sort of tricky thing that should be documented near for_each_child_of_node(), since I believe a lot of code gets this wrong. See:

http://lxr.free-electrons.com/source/arch/arm/mach-shmobile/pm-rmobile.c#L367
http://lxr.free-electrons.com/source/drivers/phy/phy-miphy365x.c#L564

and many more.

Thomas

Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
#4 lore raw

PATCH 2/9 pci: mvebu: fix memory leaks and refcount leaks

Quoted 8 lines · Thomas Petazzoni

Russell,

drivers/pci/host/pci-mvebu.c static int mvebu_pcie_parse_port(struct mvebu_pcie *pcie, · line 1014

Quoted 1 line · Russell King
drivers/pci/host/pci-mvebu.c +1 -0
+	ret = devm_add_action(dev, mvebu_pcie_port_clk_put, port);

I didn't know about devm_add_action(). Definitely very useful for such situations.

I was also surprised by this. Nice.

Quoted 21 lines

drivers/pci/host/pci-mvebu.c static int mvebu_pcie_probe(struct platform_device *pdev) · line 1094

Quoted 12 lines
drivers/pci/host/pci-mvebu.c +4 -2
@@ -1052,10 +1086,12 @@ static int mvebu_pcie_probe(struct platform_device *pdev)
 		struct mvebu_pcie_port *port = &pcie->ports[i];
 
 		ret = mvebu_pcie_parse_port(pcie, port, child);
-		if (ret < 0)
+		if (ret < 0) {
+			of_node_put(child);
 			return ret;
-		else if (ret == 0)
+		} else if (ret == 0) {
 			continue;
+		}

This is not trivial. If I understand correctly, for_each_available_child_of_node() will automatically release the reference on the previous node and take the reference on the new one before entering the loop code. So in the skipping case, we don't need to release the reference as it will be done by the next iteration of the loop, but in the error case, since we are unexpectedly breaking the loop, we need to do it manually.

Quoted 3 lines

The sort of tricky thing that should be documented near for_each_child_of_node(), since I believe a lot of code gets this wrong.

Sounds like a good candidate for a coccinelle script. Maybe you could ask Julia Lawall?

Andrew

#5 lore raw

PATCH 2/9 pci: mvebu: fix memory leaks and refcount leaks

Hello

(Julia added in the list of recipients).

Quoted 21 lines · Andrew Lunn
Quoted 21 lines

drivers/pci/host/pci-mvebu.c static int mvebu_pcie_probe(struct platform_device *pdev) · line 1094

Quoted 12 lines
drivers/pci/host/pci-mvebu.c +4 -2
@@ -1052,10 +1086,12 @@ static int mvebu_pcie_probe(struct platform_device *pdev)
 		struct mvebu_pcie_port *port = &pcie->ports[i];
 
 		ret = mvebu_pcie_parse_port(pcie, port, child);
-		if (ret < 0)
+		if (ret < 0) {
+			of_node_put(child);
 			return ret;
-		else if (ret == 0)
+		} else if (ret == 0) {
 			continue;
+		}

This is not trivial. If I understand correctly, for_each_available_child_of_node() will automatically release the reference on the previous node and take the reference on the new one before entering the loop code. So in the skipping case, we don't need to release the reference as it will be done by the next iteration of the loop, but in the error case, since we are unexpectedly breaking the loop, we need to do it manually.

Quoted 6 lines
Quoted 3 lines

The sort of tricky thing that should be documented near for_each_child_of_node(), since I believe a lot of code gets this wrong.

Sounds like a good candidate for a coccinelle script. Maybe you could ask Julia Lawall?

Julia: some of the Device Tree iterator functions have a somewhat "interesting" behavior in that they take the reference on the current child when entering the loop, and release it before entering the next iteration (which will also take a reference to the next child).

This means that if you have an exit point inside the loop (break or return), you are loosing a reference. For example,
http://lxr.free-electrons.com/source/arch/arm/mach-shmobile/pm-rmobile.c#L367
is wrong because there is a missing of_node_put(np) before the "return -ENOMEM".

So essentially, every exit point in such Device Tree iteration loops should make to call of_node_put() on the current element before exiting the loop unexpectedly.

As Andrew suggested, this is probably something a Coccinelle semantic patch could easily detect and probably fix automatically.

Thanks,

Thomas

Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
author
#6 lore raw

PATCH 2/9 pci: mvebu: fix memory leaks and refcount leaks

Quoted 11 lines · Thomas Petazzoni

Julia: some of the Device Tree iterator functions have a somewhat "interesting" behavior in that they take the reference on the current child when entering the loop, and release it before entering the next iteration (which will also take a reference to the next child).

This means that if you have an exit point inside the loop (break or return), you are loosing a reference. For example,
http://lxr.free-electrons.com/source/arch/arm/mach-shmobile/pm-rmobile.c#L367
is wrong because there is a missing of_node_put(np) before the "return -ENOMEM".

Quoted 3 lines

So essentially, every exit point in such Device Tree iteration loops should make to call of_node_put() on the current element before exiting the loop unexpectedly.

There is an exception to that... if the loop is part of a DT node lookup function, which would return a DT node based on some search criteria, it would be valid to break out of the loop while holding a reference.

For example, of_get_child_by_name(), of_find_next_cache_node(), of_graph_get_port_by_id(), are valid breaker-outer cases. ;)

However, of_platform_bus_probe(), of_platform_populate(), of_overlay_apply_one(), overlay_subtree_check() are examples of incorrect break-out cases.

Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
#7 lore raw

PATCH 2/9 pci: mvebu: fix memory leaks and refcount leaks

Quoted 20 lines · Russell King - ARM Linux
Quoted 11 lines · Thomas Petazzoni

Julia: some of the Device Tree iterator functions have a somewhat "interesting" behavior in that they take the reference on the current child when entering the loop, and release it before entering the next iteration (which will also take a reference to the next child).

This means that if you have an exit point inside the loop (break or return), you are loosing a reference. For example,
http://lxr.free-electrons.com/source/arch/arm/mach-shmobile/pm-rmobile.c#L367
is wrong because there is a missing of_node_put(np) before the "return -ENOMEM".

Quoted 3 lines

So essentially, every exit point in such Device Tree iteration loops should make to call of_node_put() on the current element before exiting the loop unexpectedly.

There is an exception to that... if the loop is part of a DT node lookup function, which would return a DT node based on some search criteria, it would be valid to break out of the loop while holding a reference.

Quoted 6 lines

For example, of_get_child_by_name(), of_find_next_cache_node(), of_graph_get_port_by_id(), are valid breaker-outer cases. ;)

However, of_platform_bus_probe(), of_platform_populate(), of_overlay_apply_one(), overlay_subtree_check() are examples of incorrect break-out cases.

Thanks for the examples.

I guess that this incorrect too, for the opposite reason (double put)?

drivers/clk/tegra/clk-emc.c:

	for_each_child_of_node(np, node) {
                err = of_property_read_u32(node, "nvidia,ram-code",
                                           &node_ram_code);
                if (err) {
                        of_node_put(node);
                        continue;
		}

julia

author
#8 lore raw

PATCH 2/9 pci: mvebu: fix memory leaks and refcount leaks

Quoted 11 lines · Julia Lawall

I guess that this incorrect too, for the opposite reason (double put)?

drivers/clk/tegra/clk-emc.c:

	for_each_child_of_node(np, node) {
                err = of_property_read_u32(node, "nvidia,ram-code",
                                           &node_ram_code);
                if (err) {
                        of_node_put(node);
                        continue;
		}

Oh yes, most definitely. Nice find!

Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
#9 lore raw

PATCH 2/9 pci: mvebu: fix memory leaks and refcount leaks

In reply to Thomas Petazzoni
Quoted 45 lines · Thomas Petazzoni

Hello

(Julia added in the list of recipients).

Quoted 21 lines · Andrew Lunn
Quoted 21 lines

drivers/pci/host/pci-mvebu.c static int mvebu_pcie_probe(struct platform_device *pdev) · line 1094

Quoted 12 lines
drivers/pci/host/pci-mvebu.c +4 -2
@@ -1052,10 +1086,12 @@ static int mvebu_pcie_probe(struct platform_device *pdev)
 		struct mvebu_pcie_port *port = &pcie->ports[i];
 
 		ret = mvebu_pcie_parse_port(pcie, port, child);
-		if (ret < 0)
+		if (ret < 0) {
+			of_node_put(child);
 			return ret;
-		else if (ret == 0)
+		} else if (ret == 0) {
 			continue;
+		}

This is not trivial. If I understand correctly, for_each_available_child_of_node() will automatically release the reference on the previous node and take the reference on the new one before entering the loop code. So in the skipping case, we don't need to release the reference as it will be done by the next iteration of the loop, but in the error case, since we are unexpectedly breaking the loop, we need to do it manually.

Quoted 6 lines
Quoted 3 lines

The sort of tricky thing that should be documented near for_each_child_of_node(), since I believe a lot of code gets this wrong.

Sounds like a good candidate for a coccinelle script. Maybe you could ask Julia Lawall?

Julia: some of the Device Tree iterator functions have a somewhat "interesting" behavior in that they take the reference on the current child when entering the loop, and release it before entering the next iteration (which will also take a reference to the next child).

This means that if you have an exit point inside the loop (break or return), you are loosing a reference. For example,
http://lxr.free-electrons.com/source/arch/arm/mach-shmobile/pm-rmobile.c#L367
is wrong because there is a missing of_node_put(np) before the "return -ENOMEM".

Quoted 6 lines

So essentially, every exit point in such Device Tree iteration loops should make to call of_node_put() on the current element before exiting the loop unexpectedly.

As Andrew suggested, this is probably something a Coccinelle semantic patch could easily detect and probably fix automatically.

Yeah, I looked at this along time ago, including the interesting loop behavior. I'll check on it again. Thanks for the pointer.

julia

author
#10 lore raw

PATCH 2/9 pci: mvebu: fix memory leaks and refcount leaks

In reply to Thomas Petazzoni
Quoted 12 lines · Thomas Petazzoni

Russell,

drivers/pci/host/pci-mvebu.c static int mvebu_pcie_parse_port(struct mvebu_pcie *pcie, · line 1014

Quoted 1 line · Russell King
drivers/pci/host/pci-mvebu.c +1 -0
+	ret = devm_add_action(dev, mvebu_pcie_port_clk_put, port);

I didn't know about devm_add_action(). Definitely very useful for such situations.

Quoted 3 lines
drivers/pci/host/pci-mvebu.c +0 -0
@@ -1052,10 +1086,12 @@ static int mvebu_pcie_probe(struct platform_device *pdev)
 		struct mvebu_pcie_port *port = &pcie->ports[i];
Quoted 14 lines

drivers/pci/host/pci-mvebu.c static int mvebu_pcie_probe(struct platform_device *pdev) · line 1094

Quoted 9 lines
drivers/pci/host/pci-mvebu.c +4 -2
 		ret = mvebu_pcie_parse_port(pcie, port, child);
-		if (ret < 0)
+		if (ret < 0) {
+			of_node_put(child);
 			return ret;
-		else if (ret == 0)
+		} else if (ret == 0) {
 			continue;
+		}

This is not trivial. If I understand correctly, for_each_available_child_of_node() will automatically release the reference on the previous node and take the reference on the new one before entering the loop code.

Yes.

Quoted 4 lines

So in the skipping case, we don't need
to release the reference as it will be done by the next iteration of the loop, but in the error case, since we are unexpectedly breaking the loop, we need to do it manually.

Correct.

Quoted 3 lines

The sort of tricky thing that should be documented near for_each_child_of_node(), since I believe a lot of code gets this wrong.

Yes, lots of code gets it wrong, even the core DT code gets it wrong. The problem will be stopping people getting it wrong in the future, now that they've learnt the wrong way to do it - I don't think documenting the site where for_each_child_of_node() is defined will work anymore - how many people know of for_each_child_of_node() and don't need to look it up, yet code exactly this bug?

TBH, the fundamental issue here is wrapping this all up in what looks to be a harmless for_each_child_of_node() thing. for_each_child_of_node() has side-effects that are completely non-obvious, except to those who have been educated about it.

I think the only solution now is to kill for_each_child_of_node() and similar, and have people open-code the for() loop so they can _see_ what's going on, because we can't stop people from breaking out of a for() loop. That's a major change though, outside the scope of this series.

I raised the issue last month with DT people, but there isn't any interest in fixing the issue; they know about it but (iirc) have no plans to fix it.

Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
#11 lore raw

PATCH 3/9 pci: mvebu: split port parsing and resource claiming from port setup

In reply to Russell King - ARM Linux

Split the PCIe port DT parsing and resource claiming from setting up the actual ports. This allows us to gather all the resources first, before touching the hardware. This is important as some of these resources (such as the GPIO for the PCIe reset) may defer probing.

Signed-off-by Russell King <[email protected]>
---
 drivers/pci/host/pci-mvebu.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)
drivers/pci/host/pci-mvebu.c +12 -3
index e8c51bb58e99..92c777e1aa3c 100644
@@ -1093,6 +1093,18 @@ static int mvebu_pcie_probe(struct platform_device *pdev)
 			continue;
 		}
 
+		port->dn = child;
+		i++;
+	}
+	pcie->nports = i;
+
+	for (i = 0; i < pcie->nports; i++) {
+		struct mvebu_pcie_port *port = &pcie->ports[i];
+
+		child = port->dn;
+		if (!child)
+			continue;
+
 		if (gpio_is_valid(port->reset_gpio)) {
 			u32 reset_udelay = 20000;
 

@@ -1118,10 +1130,7 @@ static int mvebu_pcie_probe(struct platform_device *pdev)
 		}
 
 		mvebu_pcie_set_local_dev_nr(port, 1);
-
-		port->dn = child;
 		mvebu_sw_pci_bridge_init(port);
-		i++;
 	}
 
 	pcie->nports = i;
Signature · 1 line
2.1.0
#12 lore raw

PATCH 4/9 pci: mvebu: use gpio_set_value_cansleep()

In reply to Russell King - ARM Linux

We are in a context where we can sleep, and the PCIe reset gpio may be on an I2C expander. Use the cansleep() variant when setting the GPIO value.

Signed-off-by Russell King <[email protected]>
---
 drivers/pci/host/pci-mvebu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/pci/host/pci-mvebu.c +2 -2
index 92c777e1aa3c..00467c5a58ac 100644
@@ -1111,8 +1111,8 @@ static int mvebu_pcie_probe(struct platform_device *pdev)
 			of_property_read_u32(child, "reset-delay-us",
 					     &reset_udelay);
 
-			gpio_set_value(port->reset_gpio,
-				       (port->reset_active_low) ? 1 : 0);
+			gpio_set_value_cansleep(port->reset_gpio,
+						!!port->reset_active_low);
 			msleep(reset_udelay/1000);
 		}
Signature · 1 line
2.1.0
#13 lore raw

PATCH 5/9 pci: mvebu: use devm_kcalloc() to allocate an array

In reply to Russell King - ARM Linux

Rather than using devm_kzalloc() and multiplying the element and number, use the provided devm_kcalloc() helper for this.

Signed-off-by Russell King <[email protected]>
---
 drivers/pci/host/pci-mvebu.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/pci/host/pci-mvebu.c +1 -2
index 00467c5a58ac..7282bb06c7a8 100644
@@ -1075,8 +1075,7 @@ static int mvebu_pcie_probe(struct platform_device *pdev)
 
 	num = of_get_available_child_count(pdev->dev.of_node);
 
-	pcie->ports = devm_kzalloc(&pdev->dev, num *
-				   sizeof(struct mvebu_pcie_port),
+	pcie->ports = devm_kcalloc(&pdev->dev, num, sizeof(*pcie->ports),
 				   GFP_KERNEL);
 	if (!pcie->ports)
 		return -ENOMEM;
Signature · 1 line
2.1.0
#14 lore raw

PATCH 6/9 pci: mvebu: use gpio_desc to carry around gpio

In reply to Russell King - ARM Linux

Use a gpio_desc to carry around the gpio, so we can then make use of the GPIOF_ACTIVE_LOW property rather than carrying that around as well. This also avoids needing to use gpio_is_valid() to check whether we have a GPIO; checking for a non-NULL descriptor is simpler.

Signed-off-by Russell King <[email protected]>
---
 drivers/pci/host/pci-mvebu.c | 39 ++++++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 15 deletions(-)
drivers/pci/host/pci-mvebu.c +24 -15
index 7282bb06c7a8..ab619ee0ef49 100644
@@ -119,8 +119,7 @@ struct mvebu_pcie_port {
 	unsigned int io_target;
 	unsigned int io_attr;
 	struct clk *clk;
-	int reset_gpio;
-	int reset_active_low;
+	struct gpio_desc *reset_gpio;
 	char *reset_name;
 	struct mvebu_sw_pci_bridge bridge;
 	struct device_node *dn;

@@ -940,7 +939,7 @@ static int mvebu_pcie_parse_port(struct mvebu_pcie *pcie,
 {
 	struct device *dev = &pcie->pdev->dev;
 	enum of_gpio_flags flags;
-	int ret;
+	int reset_gpio, ret;
 
 	port->pcie = pcie;
 

@@ -980,15 +979,15 @@ static int mvebu_pcie_parse_port(struct mvebu_pcie *pcie,
 		port->io_attr = -1;
 	}
 
-	port->reset_gpio = of_get_named_gpio_flags(child, "reset-gpios", 0,
-						   &flags);
-	if (port->reset_gpio == -EPROBE_DEFER) {
-		ret = port->reset_gpio;
+	reset_gpio = of_get_named_gpio_flags(child, "reset-gpios", 0, &flags);
+	if (reset_gpio == -EPROBE_DEFER) {
+		ret = reset_gpio;
 		goto err;
 	}
 
-	if (gpio_is_valid(port->reset_gpio)) {
-		port->reset_active_low = flags & OF_GPIO_ACTIVE_LOW;
+	if (gpio_is_valid(reset_gpio)) {
+		unsigned long gpio_flags;
+
 		port->reset_name = devm_kasprintf(dev, GFP_KERNEL, "%s-reset",
 						  port->name);
 		if (!port->reset_name) {

@@ -996,13 +995,24 @@ static int mvebu_pcie_parse_port(struct mvebu_pcie *pcie,
 			goto err;
 		}
 
-		ret = devm_gpio_request_one(dev, port->reset_gpio,
-					    GPIOF_DIR_OUT, port->reset_name);
+		if (flags & OF_GPIO_ACTIVE_LOW) {
+			dev_info(dev, "%s: reset gpio is active low\n",
+				 of_node_full_name(child));
+			gpio_flags = GPIOF_ACTIVE_LOW |
+				     GPIOF_OUT_INIT_LOW;
+		} else {
+			gpio_flags = GPIOF_OUT_INIT_HIGH;
+		}
+
+		ret = devm_gpio_request_one(dev, reset_gpio, gpio_flags,
+					    port->reset_name);
 		if (ret) {
 			if (ret == -EPROBE_DEFER)
 				goto err;
 			goto skip;
 		}
+
+		port->reset_gpio = gpio_to_desc(reset_gpio);
 	}
 
 	port->clk = of_clk_get_by_name(child, NULL);

@@ -1104,15 +1114,14 @@ static int mvebu_pcie_probe(struct platform_device *pdev)
 		if (!child)
 			continue;
 
-		if (gpio_is_valid(port->reset_gpio)) {
+		if (port->reset_gpio) {
 			u32 reset_udelay = 20000;
 
 			of_property_read_u32(child, "reset-delay-us",
 					     &reset_udelay);
 
-			gpio_set_value_cansleep(port->reset_gpio,
-						!!port->reset_active_low);
-			msleep(reset_udelay/1000);
+			gpiod_set_value_cansleep(port->reset_gpio, 0);
+			msleep(reset_udelay / 1000);
 		}
 
 		ret = clk_prepare_enable(port->clk);
Signature · 1 line
2.1.0
#15 lore raw

PATCH 7/9 pci: mvebu: better implementation of clock/reset handling

In reply to Russell King - ARM Linux

Add an implementation to handle clock and reset handling that is compliant with the PCIe specification. The clock should be running and stable for 100us prior to reset being released, and we should re-assert reset prior to stopping the clock.

Signed-off-by Russell King <[email protected]>
---
 drivers/pci/host/pci-mvebu.c | 56 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 43 insertions(+), 13 deletions(-)
drivers/pci/host/pci-mvebu.c +43 -13
index ab619ee0ef49..97208233d901 100644
@@ -1042,6 +1042,46 @@ static int mvebu_pcie_parse_port(struct mvebu_pcie *pcie,
 	return ret;
 }
 
+/*
+ * Power up a PCIe port.  PCIe requires the refclk to be stable for 100?s
+ * prior to releasing PERST.  See table 2-4 in section 2.6.2 AC Specifications
+ * of the PCI Express Card Electromechanical Specification, 1.1.
+ */
+static int mvebu_pcie_powerup(struct mvebu_pcie_port *port)
+{
+	int ret;
+
+	ret = clk_prepare_enable(port->clk);
+	if (ret < 0)
+		return ret;
+
+	if (port->reset_gpio) {
+		u32 reset_udelay = 20000;
+
+		of_property_read_u32(port->dn, "reset-delay-us",
+				     &reset_udelay);
+
+		udelay(100);
+
+		gpiod_set_value_cansleep(port->reset_gpio, 0);
+		msleep(reset_udelay / 1000);
+	}
+
+	return 0;
+}
+
+/*
+ * Power down a PCIe port.  Strictly, PCIe requires us to place the card
+ * in D3hot state before asserting PERST#.
+ */
+static void mvebu_pcie_powerdown(struct mvebu_pcie_port *port)
+{
+	if (port->reset_gpio)
+		gpiod_set_value_cansleep(port->reset_gpio, 1);
+
+	clk_disable_unprepare(port->clk);
+}
+
 static int mvebu_pcie_probe(struct platform_device *pdev)
 {
 	struct mvebu_pcie *pcie;

@@ -1114,18 +1154,8 @@ static int mvebu_pcie_probe(struct platform_device *pdev)
 		if (!child)
 			continue;
 
-		if (port->reset_gpio) {
-			u32 reset_udelay = 20000;
-
-			of_property_read_u32(child, "reset-delay-us",
-					     &reset_udelay);
-
-			gpiod_set_value_cansleep(port->reset_gpio, 0);
-			msleep(reset_udelay / 1000);
-		}
-
-		ret = clk_prepare_enable(port->clk);
-		if (ret)
+		ret = mvebu_pcie_powerup(port);
+		if (ret < 0)
 			continue;
 
 		port->base = mvebu_pcie_map_registers(pdev, child, port);

@@ -1133,7 +1163,7 @@ static int mvebu_pcie_probe(struct platform_device *pdev)
 			dev_err(&pdev->dev, "%s: cannot map registers\n",
 				port->name);
 			port->base = NULL;
-			clk_disable_unprepare(port->clk);
+			mvebu_pcie_powerdown(port);
 			continue;
 		}
Signature · 1 line
2.1.0
#16 lore raw

PATCH 8/9 pci: mvebu: add PCI Express root complex capability block

In reply to Russell King - ARM Linux

Add a PCI Express root complex capability block so the PCI layer identifies the bridge as a PCI Express device.

We expose this as a version 1 PCIe capability block, with slot support. We disable the clock power management capability as this depends on boards wiring the CLKREQ# signal.

Signed-off-by Russell King <[email protected]>
---
 drivers/pci/host/pci-mvebu.c | 137 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 133 insertions(+), 4 deletions(-)
drivers/pci/host/pci-mvebu.c +133 -4
index 97208233d901..6310f2a84cfd 100644
@@ -30,6 +30,7 @@
 #define PCIE_DEV_REV_OFF	0x0008
 #define PCIE_BAR_LO_OFF(n)	(0x0010 + ((n) << 3))
 #define PCIE_BAR_HI_OFF(n)	(0x0014 + ((n) << 3))
+#define PCIE_CAP_PCIEXP		0x0060
 #define PCIE_HEADER_LOG_4_OFF	0x0128
 #define PCIE_BAR_CTRL_OFF(n)	(0x1804 + (((n) - 1) * 4))
 #define PCIE_WIN04_CTRL_OFF(n)	(0x1820 + ((n) << 4))

@@ -57,14 +58,35 @@
 #define  PCIE_STAT_BUS                  0xff00
 #define  PCIE_STAT_DEV                  0x1f0000
 #define  PCIE_STAT_LINK_DOWN		BIT(0)
+#define PCIE_RC_RTSTA		0x1a14
 #define PCIE_DEBUG_CTRL         0x1a60
 #define  PCIE_DEBUG_SOFT_RESET		BIT(20)
 
+enum {
+	PCISWCAP = PCI_BRIDGE_CONTROL + 2,
+	PCISWCAP_EXP_LIST_ID	= PCISWCAP + PCI_CAP_LIST_ID,
+	PCISWCAP_EXP_DEVCAP	= PCISWCAP + PCI_EXP_DEVCAP,
+	PCISWCAP_EXP_DEVCTL	= PCISWCAP + PCI_EXP_DEVCTL,
+	PCISWCAP_EXP_LNKCAP	= PCISWCAP + PCI_EXP_LNKCAP,
+	PCISWCAP_EXP_LNKCTL	= PCISWCAP + PCI_EXP_LNKCTL,
+	PCISWCAP_EXP_SLTCAP	= PCISWCAP + PCI_EXP_SLTCAP,
+	PCISWCAP_EXP_SLTCTL	= PCISWCAP + PCI_EXP_SLTCTL,
+	PCISWCAP_EXP_RTCTL	= PCISWCAP + PCI_EXP_RTCTL,
+	PCISWCAP_EXP_RTSTA	= PCISWCAP + PCI_EXP_RTSTA,
+	PCISWCAP_EXP_DEVCAP2	= PCISWCAP + PCI_EXP_DEVCAP2,
+	PCISWCAP_EXP_DEVCTL2	= PCISWCAP + PCI_EXP_DEVCTL2,
+	PCISWCAP_EXP_LNKCAP2	= PCISWCAP + PCI_EXP_LNKCAP2,
+	PCISWCAP_EXP_LNKCTL2	= PCISWCAP + PCI_EXP_LNKCTL2,
+	PCISWCAP_EXP_SLTCAP2	= PCISWCAP + PCI_EXP_SLTCAP2,
+	PCISWCAP_EXP_SLTCTL2	= PCISWCAP + PCI_EXP_SLTCTL2,
+};
+
 /* PCI configuration space of a PCI-to-PCI bridge */
 struct mvebu_sw_pci_bridge {
 	u16 vendor;
 	u16 device;
 	u16 command;
+	u16 status;
 	u16 class;
 	u8 interface;
 	u8 revision;

@@ -84,13 +106,15 @@ struct mvebu_sw_pci_bridge {
 	u16 memlimit;
 	u16 iobaseupper;
 	u16 iolimitupper;
-	u8 cappointer;
-	u8 reserved1;
-	u16 reserved2;
 	u32 romaddr;
 	u8 intline;
 	u8 intpin;
 	u16 bridgectrl;
+
+	/* PCI express capability */
+	u32 pcie_sltcap;
+	u16 pcie_devctl;
+	u16 pcie_rtctl;
 };
 
 struct mvebu_pcie_port;

@@ -451,6 +475,9 @@ static void mvebu_sw_pci_bridge_init(struct mvebu_pcie_port *port)
 	/* We support 32 bits I/O addressing */
 	bridge->iobase = PCI_IO_RANGE_TYPE_32;
 	bridge->iolimit = PCI_IO_RANGE_TYPE_32;
+
+	/* Add capabilities */
+	bridge->status = PCI_STATUS_CAP_LIST;
 }
 
 /*

@@ -468,7 +495,7 @@ static int mvebu_sw_pci_bridge_read(struct mvebu_pcie_port *port,
 		break;
 
 	case PCI_COMMAND:
-		*value = bridge->command;
+		*value = bridge->command | bridge->status << 16;
 		break;
 
 	case PCI_CLASS_REVISION:

@@ -513,6 +540,10 @@ static int mvebu_sw_pci_bridge_read(struct mvebu_pcie_port *port,
 		*value = (bridge->iolimitupper << 16 | bridge->iobaseupper);
 		break;
 
+	case PCI_CAPABILITY_LIST:
+		*value = PCISWCAP;
+		break;
+
 	case PCI_ROM_ADDRESS1:
 		*value = 0;
 		break;

@@ -522,6 +553,59 @@ static int mvebu_sw_pci_bridge_read(struct mvebu_pcie_port *port,
 		*value = 0;
 		break;
 
+	case PCISWCAP_EXP_LIST_ID:
+		/* Set PCIe v2, root port, slot support */
+		*value = (PCI_EXP_TYPE_ROOT_PORT << 4 | 2 |
+			  PCI_EXP_FLAGS_SLOT) << 16 | PCI_CAP_ID_EXP;
+		break;
+
+	case PCISWCAP_EXP_DEVCAP:
+		*value = mvebu_readl(port, PCIE_CAP_PCIEXP + PCI_EXP_DEVCAP);
+		break;
+
+	case PCISWCAP_EXP_DEVCTL:
+		*value = mvebu_readl(port, PCIE_CAP_PCIEXP + PCI_EXP_DEVCTL) &
+				 ~(PCI_EXP_DEVCTL_URRE | PCI_EXP_DEVCTL_FERE |
+				   PCI_EXP_DEVCTL_NFERE | PCI_EXP_DEVCTL_CERE);
+		*value |= bridge->pcie_devctl;
+		break;
+
+	case PCISWCAP_EXP_LNKCAP:
+		/*
+		 * PCIe requires the clock power management capability to be
+		 * hard-wired to zero for downstream ports
+		 */
+		*value = mvebu_readl(port, PCIE_CAP_PCIEXP + PCI_EXP_LNKCAP) &
+			 ~PCI_EXP_LNKCAP_CLKPM;
+		break;
+
+	case PCISWCAP_EXP_LNKCTL:
+		*value = mvebu_readl(port, PCIE_CAP_PCIEXP + PCI_EXP_LNKCTL);
+		break;
+
+	case PCISWCAP_EXP_SLTCAP:
+		*value = bridge->pcie_sltcap;
+		break;
+
+	case PCISWCAP_EXP_SLTCTL:
+		*value = PCI_EXP_SLTSTA_PDS << 16;
+		break;
+
+	case PCISWCAP_EXP_RTCTL:
+		*value = bridge->pcie_rtctl;
+		break;
+
+	case PCISWCAP_EXP_RTSTA:
+		*value = mvebu_readl(port, PCIE_RC_RTSTA);
+		break;
+
+	/* PCIe requires the v2 fields to be hard-wired to zero */
+	case PCISWCAP_EXP_DEVCAP2:
+	case PCISWCAP_EXP_DEVCTL2:
+	case PCISWCAP_EXP_LNKCAP2:
+	case PCISWCAP_EXP_LNKCTL2:
+	case PCISWCAP_EXP_SLTCAP2:
+	case PCISWCAP_EXP_SLTCTL2:
 	default:
 		/*
 		 * PCI defines configuration read accesses to reserved or

@@ -614,6 +698,51 @@ static int mvebu_sw_pci_bridge_write(struct mvebu_pcie_port *port,
 		mvebu_pcie_set_local_bus_nr(port, bridge->secondary_bus);
 		break;
 
+	case PCISWCAP_EXP_DEVCTL:
+		/*
+		 * Armada370 data says these bits must always
+		 * be zero when in root complex mode.
+		 */
+		value &= ~(PCI_EXP_DEVCTL_URRE | PCI_EXP_DEVCTL_FERE |
+			   PCI_EXP_DEVCTL_NFERE | PCI_EXP_DEVCTL_CERE);
+
+		/*
+		 * If the mask is 0xffff0000, then we only want to write
+		 * the device control register, rather than clearing the
+		 * RW1C bits in the device status register.  Mask out the
+		 * status register bits.
+		 */
+		if (mask == 0xffff0000)
+			value &= 0xffff;
+
+		mvebu_writel(port, value, PCIE_CAP_PCIEXP + PCI_EXP_DEVCTL);
+		break;
+
+	case PCISWCAP_EXP_LNKCTL:
+		/*
+		 * If we don't support CLKREQ, we must ensure that the
+		 * CLKREQ enable bit always reads zero.  Since we haven't
+		 * had this capability, and it's dependent on board wiring,
+		 * disable it for the time being.
+		 */
+		value &= ~PCI_EXP_LNKCTL_CLKREQ_EN;
+
+		/*
+		 * If the mask is 0xffff0000, then we only want to write
+		 * the link control register, rather than clearing the
+		 * RW1C bits in the link status register.  Mask out the
+		 * status register bits.
+		 */
+		if (mask == 0xffff0000)
+			value &= 0xffff;
+
+		mvebu_writel(port, value, PCIE_CAP_PCIEXP + PCI_EXP_LNKCTL);
+		break;
+
+	case PCISWCAP_EXP_RTSTA:
+		mvebu_writel(port, value, PCIE_RC_RTSTA);
+		break;
+
 	default:
 		break;
 	}
Signature · 1 line
2.1.0
#17 lore raw

PATCH 9/9 pci: mvebu: remove code restricting accesses to slot 0

In reply to Russell King - ARM Linux

Now that we advertise a PCIe capability, the Linux PCI layer will not scan the bus for devices other than in slot 0. This makes the work-around to trap accesses to devices other than slot 0 unnecessary.

Signed-off-by Russell King <[email protected]>
---
 drivers/pci/host/pci-mvebu.c | 24 ------------------------
 1 file changed, 24 deletions(-)
drivers/pci/host/pci-mvebu.c +0 -24
index 6310f2a84cfd..53b79c5f0559 100644
@@ -794,17 +794,6 @@ static int mvebu_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
 	if (!mvebu_pcie_link_up(port))
 		return PCIBIOS_DEVICE_NOT_FOUND;
 
-	/*
-	 * On the secondary bus, we don't want to expose any other
-	 * device than the device physically connected in the PCIe
-	 * slot, visible in slot 0. In slot 1, there's a special
-	 * Marvell device that only makes sense when the Armada is
-	 * used as a PCIe endpoint.
-	 */
-	if (bus->number == port->bridge.secondary_bus &&
-	    PCI_SLOT(devfn) != 0)
-		return PCIBIOS_DEVICE_NOT_FOUND;
-
 	/* Access the real PCIe interface */
 	ret = mvebu_pcie_hw_wr_conf(port, bus, devfn,
 				    where, size, val);

@@ -835,19 +824,6 @@ static int mvebu_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where,
 		return PCIBIOS_DEVICE_NOT_FOUND;
 	}
 
-	/*
-	 * On the secondary bus, we don't want to expose any other
-	 * device than the device physically connected in the PCIe
-	 * slot, visible in slot 0. In slot 1, there's a special
-	 * Marvell device that only makes sense when the Armada is
-	 * used as a PCIe endpoint.
-	 */
-	if (bus->number == port->bridge.secondary_bus &&
-	    PCI_SLOT(devfn) != 0) {
-		*val = 0xffffffff;
-		return PCIBIOS_DEVICE_NOT_FOUND;
-	}
-
 	/* Access the real PCIe interface */
 	ret = mvebu_pcie_hw_rd_conf(port, bus, devfn,
 				    where, size, val);
Signature · 1 line
2.1.0
#18 lore raw

cover Further mvebu PCIe patches

In reply to Russell King - ARM Linux

Hello Russell,

Quoted 17 lines · Russell King - ARM Linux

Here are further PCIe patches which follow on from the previous set of six I sent earlier in September. This set:

* Separates the DT parsing from the use of this parsed data.
* Fixes memory leaks from kasprintf() and refcount leaks of the DT node where we break out from the loop early.
* Uses gpio_set_value_cansleep() so that GPIOs on I2C expanders can be used for the PCIe reset functionality.
* Switch to using devm_kcalloc() instead of devm_kzalloc(), which eliminates the multiplication, moving it into core code.
* Switch to using a gpio descriptor for the reset gpio, which can contain the active level information from DT. (It would be nice if gpiolib automated some of the resource claiming there.)
* Make PERST# vs clock timing to match PCIe specifications. PCIe specs require the clock to be running for 100us prior to releasing reset.
* Add the standard PCIe capability block in root port form to the

Quoted 8 lines

emulated PCIe configuration block. This allows the PCI layer to identify the "host bridge" as a PCIe device, and allows us to take advantage of the code in drivers/pci/pcie, particularly aspm for link power management.
* As a result of identifying ourselves as a PCIe root port, this eliminates the need to special case accesses to non-slot 0 in the driver; the lack of other "slots" is something which the generic PCI code knows about for PCIe root ports.

Thanks for these patches! They look good. However, I'm away for the Embedded Linux Conference Europe in Dublin right now, and therefore don't have access to my boards to test that it works OK on platforms other than Armada 38x (which you tested). I will try to do this testing next week, hopefully on Friday.

Thanks!

Thomas

Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
#19 lore raw

cover Further mvebu PCIe patches

Hi,

Quoted 30 lines · Thomas Petazzoni

Hello Russell,

Quoted 17 lines · Russell King - ARM Linux

Here are further PCIe patches which follow on from the previous set of six I sent earlier in September. This set:

* Separates the DT parsing from the use of this parsed data.
* Fixes memory leaks from kasprintf() and refcount leaks of the DT node where we break out from the loop early.
* Uses gpio_set_value_cansleep() so that GPIOs on I2C expanders can be used for the PCIe reset functionality.
* Switch to using devm_kcalloc() instead of devm_kzalloc(), which eliminates the multiplication, moving it into core code.
* Switch to using a gpio descriptor for the reset gpio, which can contain the active level information from DT. (It would be nice if gpiolib automated some of the resource claiming there.)
* Make PERST# vs clock timing to match PCIe specifications. PCIe specs require the clock to be running for 100us prior to releasing reset.
* Add the standard PCIe capability block in root port form to the

Quoted 8 lines

emulated PCIe configuration block. This allows the PCI layer to identify the "host bridge" as a PCIe device, and allows us to take advantage of the code in drivers/pci/pcie, particularly aspm for link power management.
* As a result of identifying ourselves as a PCIe root port, this eliminates the need to special case accesses to non-slot 0 in the driver; the lack of other "slots" is something which the generic PCI code knows about for PCIe root ports.

Quoted 5 lines

Thanks for these patches! They look good. However, I'm away for the Embedded Linux Conference Europe in Dublin right now, and therefore don't have access to my boards to test that it works OK on platforms other than Armada 38x (which you tested). I will try to do this testing next week, hopefully on Friday.

Just FWIW, I could run this series on top of the previous one on top of 4.3-rc4. It ran fine (ie no regressions observed) on :

- Iomega iconnect (kirkwood) with an RT3090 WiFi card (I could verify that the card was detected in lspci, module loaded without error and the interface appeared)

- mirabox (armada370) with a dual-igb NIC (i350), Marvell 88E8053 (sky2), realtek 8168, and bcm5721. For the first one, I only verified that the driver loaded properly and that I could set the links up on the two NICs. For the last 3, I could even check that the NIC could receive traffic.

I could not test on the XPGP board, it didn't boot regardless of the patches, I'll have to redo a clean config. This happens once in a while during "make oldconfig" over a too large version jump.

So far so good!

Cheers,
Willy

#20 lore raw

cover Further mvebu PCIe patches

In reply to Russell King - ARM Linux
Quoted 4 lines · Russell King - ARM Linux

Jason, Thomas,

Here are further PCIe patches which follow on from the previous set of six I sent earlier in September. This set:

Hi Russell

Tested on my Kirkwood based DLINK DIR665 with a topdog wireless lan card. This does not make use of the GPIO reset, so those parts of the patch are untested. But otherwise, no obvious regressions, i can join the wireless LAN etc.

Tested-by Andrew Lunn <[email protected]>

Thanks
Andrew

#21 lore raw

cover Further mvebu PCIe patches

In reply to Russell King - ARM Linux

Rusell, all,

Quoted 17 lines · Russell King - ARM Linux

Here are further PCIe patches which follow on from the previous set of six I sent earlier in September. This set:

* Separates the DT parsing from the use of this parsed data.
* Fixes memory leaks from kasprintf() and refcount leaks of the DT node where we break out from the loop early.
* Uses gpio_set_value_cansleep() so that GPIOs on I2C expanders can be used for the PCIe reset functionality.
* Switch to using devm_kcalloc() instead of devm_kzalloc(), which eliminates the multiplication, moving it into core code.
* Switch to using a gpio descriptor for the reset gpio, which can contain the active level information from DT. (It would be nice if gpiolib automated some of the resource claiming there.)
* Make PERST# vs clock timing to match PCIe specifications. PCIe specs require the clock to be running for 100us prior to releasing reset.
* Add the standard PCIe capability block in root port form to the

Quoted 8 lines

emulated PCIe configuration block. This allows the PCI layer to identify the "host bridge" as a PCIe device, and allows us to take advantage of the code in drivers/pci/pcie, particularly aspm for link power management.
* As a result of identifying ourselves as a PCIe root port, this eliminates the need to special case accesses to non-slot 0 in the driver; the lack of other "slots" is something which the generic PCI code knows about for PCIe root ports.

Entire series tested on Armada XP GP, with 3 PCIe cards plugged in. They seem to work, and the lspci -v output seems sane. I've also reviewed the patch series, and it looks good to me.

Reviewed-by Thomas Petazzoni <[email protected]>
Tested-by Thomas Petazzoni <[email protected]>

Thanks!

Thomas

Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
#22 lore raw

cover Further mvebu PCIe patches

In reply to Russell King - ARM Linux
Quoted 28 lines · Russell King - ARM Linux

Jason, Thomas,

Here are further PCIe patches which follow on from the previous set of six I sent earlier in September. This set:

* Separates the DT parsing from the use of this parsed data.
* Fixes memory leaks from kasprintf() and refcount leaks of the DT node where we break out from the loop early.
* Uses gpio_set_value_cansleep() so that GPIOs on I2C expanders can be used for the PCIe reset functionality.
* Switch to using devm_kcalloc() instead of devm_kzalloc(), which eliminates the multiplication, moving it into core code.
* Switch to using a gpio descriptor for the reset gpio, which can contain the active level information from DT. (It would be nice if gpiolib automated some of the resource claiming there.)
* Make PERST# vs clock timing to match PCIe specifications. PCIe specs require the clock to be running for 100us prior to releasing reset.
* Add the standard PCIe capability block in root port form to the emulated PCIe configuration block. This allows the PCI layer to identify the "host bridge" as a PCIe device, and allows us to take advantage of the code in drivers/pci/pcie, particularly aspm for link power management.
* As a result of identifying ourselves as a PCIe root port, this eliminates the need to special case accesses to non-slot 0 in the driver; the lack of other "slots" is something which the generic PCI code knows about for PCIe root ports.

Quoted 2 lines
 drivers/pci/host/pci-mvebu.c | 404 ++++++++++++++++++++++++++++++++-----------
 1 file changed, 305 insertions(+), 99 deletions(-)

Applied to pci/host-mvebu for v4.4 with Tested-by from Willy, Andrew, and Thomas, and Reviewed-by from Thomas. I adjusted subject line capitalization to match the history. Thanks, Russell and reviewers and testers!

Bjorn