[PATCH 0/9] Further mvebu PCIe patches

[PATCH 0/9] Further mvebu PCIe patches

[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.

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

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

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

[Russell King]

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 <rmk+kernel@arm.linux.org.uk>
---
 drivers/pci/host/pci-mvebu.c | 130 ++++++++++++++++++++++++-------------------
 1 file changed, 74 insertions(+), 56 deletions(-)

diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index 19144ed7bdad..13ab0350f7fb 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -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;
-- 
2.1.0

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

[Russell King]

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 <rmk+kernel@arm.linux.org.uk>
---
 drivers/pci/host/pci-mvebu.c | 50 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 43 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index 13ab0350f7fb..e8c51bb58e99 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -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;
-- 
2.1.0

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

[Thomas Petazzoni]

Russell,

On Sat, 03 Oct 2015 19:13:02 +0100, Russell King wrote:

> +	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.

> @@ -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
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

[Andrew Lunn]

On Fri, Oct 09, 2015 at 04:52:40PM +0200, Thomas Petazzoni wrote:
> Russell,
> 
> On Sat, 03 Oct 2015 19:13:02 +0100, Russell King wrote:
> 
> > +	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.
 
> > @@ -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.

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

    Andrew

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

[Thomas Petazzoni]

Hello

(Julia added in the list of recipients).

On Fri, 9 Oct 2015 17:07:10 +0200, Andrew Lunn wrote:

> > > @@ -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.
> 
> 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
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

[Russell King - ARM Linux]

On Fri, Oct 09, 2015 at 05:14:42PM +0200, Thomas Petazzoni wrote:
> 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.

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.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

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

[Julia Lawall]

On Fri, 9 Oct 2015, Russell King - ARM Linux wrote:

> On Fri, Oct 09, 2015 at 05:14:42PM +0200, Thomas Petazzoni wrote:
> > 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.
>
> 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.

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

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

[Russell King - ARM Linux]

On Fri, Oct 09, 2015 at 06:39:47PM +0200, Julia Lawall wrote:
> 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!

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

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

[Julia Lawall]

On Fri, 9 Oct 2015, Thomas Petazzoni wrote:

> Hello
>
> (Julia added in the list of recipients).
>
> On Fri, 9 Oct 2015 17:07:10 +0200, Andrew Lunn wrote:
>
> > > > @@ -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.
> >
> > 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.

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

julia

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

[Russell King - ARM Linux]

On Fri, Oct 09, 2015 at 04:52:40PM +0200, Thomas Petazzoni wrote:
> Russell,
> 
> On Sat, 03 Oct 2015 19:13:02 +0100, Russell King wrote:
> 
> > +	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.
> 
> > @@ -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.

Yes.

> 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.

> 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.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

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

[Russell King]

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 <rmk+kernel@arm.linux.org.uk>
---
 drivers/pci/host/pci-mvebu.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index e8c51bb58e99..92c777e1aa3c 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -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;
-- 
2.1.0

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

[Russell King]

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 <rmk+kernel@arm.linux.org.uk>
---
 drivers/pci/host/pci-mvebu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index 92c777e1aa3c..00467c5a58ac 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -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);
 		}
 
-- 
2.1.0

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

[Russell King]

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

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/pci/host/pci-mvebu.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index 00467c5a58ac..7282bb06c7a8 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -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;
-- 
2.1.0

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

[Russell King]

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 <rmk+kernel@arm.linux.org.uk>
---
 drivers/pci/host/pci-mvebu.c | 39 ++++++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 15 deletions(-)

diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index 7282bb06c7a8..ab619ee0ef49 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -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);
-- 
2.1.0

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

[Russell King]

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 <rmk+kernel@arm.linux.org.uk>
---
 drivers/pci/host/pci-mvebu.c | 56 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 43 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index ab619ee0ef49..97208233d901 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -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;
 		}
 
-- 
2.1.0

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

[Russell King]

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 <rmk+kernel@arm.linux.org.uk>
---
 drivers/pci/host/pci-mvebu.c | 137 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 133 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index 97208233d901..6310f2a84cfd 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -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;
 	}
-- 
2.1.0

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

[Russell King]

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 <rmk+kernel@arm.linux.org.uk>
---
 drivers/pci/host/pci-mvebu.c | 24 ------------------------
 1 file changed, 24 deletions(-)

diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index 6310f2a84cfd..53b79c5f0559 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -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);
-- 
2.1.0

Re: [PATCH 0/9] Further mvebu PCIe patches

[Thomas Petazzoni]

Hello Russell,

On Sat, 3 Oct 2015 19:12:28 +0100, Russell King - ARM Linux wrote:

> 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.

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
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 0/9] Further mvebu PCIe patches

[Willy Tarreau]

Hi,

On Sat, Oct 03, 2015 at 08:00:26PM +0100, Thomas Petazzoni wrote:
> Hello Russell,
> 
> On Sat, 3 Oct 2015 19:12:28 +0100, Russell King - ARM Linux wrote:
> 
> > 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.
> 
> 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

Re: [PATCH 0/9] Further mvebu PCIe patches

[Andrew Lunn]

On Sat, Oct 03, 2015 at 07:12:28PM +0100, Russell King - ARM Linux wrote:
> 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 <andrew@lunn.ch>

Thanks
	   Andrew

Re: [PATCH 0/9] Further mvebu PCIe patches

[Thomas Petazzoni]

Rusell, all,

On Sat, 3 Oct 2015 19:12:28 +0100, Russell King - ARM Linux wrote:

> 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.

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 <thomas.petazzoni@free-electrons.com> 
Tested-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 0/9] Further mvebu PCIe patches

[Bjorn Helgaas]

On Sat, Oct 03, 2015 at 07:12:28PM +0100, Russell King - ARM Linux wrote:
> 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(-)

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