Skip to contentSkip to conversation

PCI: armada8k: add support for gpio controlled reset signal

PATCH New Baruch Siach 4 participants 9 messages Oct 3, 2018 – Nov 22, 2018

Subsystems (3)
PCI PCI native host bridge and endpoint drivers PCI driver for armada 8K
lore
Share & tools
lore

Baruch Siach

author
#0 lore raw

PATCH 1/1 PCI: armada8k: add support for gpio controlled reset signal

This commit adds support for the gpio reset signal binding as described in the designware-pcie.txt DT binding document. Both the documented 'reset-gpio' property name, and the more standard 'reset-gpios' name are supported.

Signed-off-by Baruch Siach <[email protected]>
---
 drivers/pci/controller/dwc/pcie-armada8k.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
drivers/pci/controller/dwc/pcie-armada8k.c +16 -0
index 0c389a30ef5d..b171b6bc15c8 100644
@@ -22,6 +22,7 @@
 #include <linux/resource.h>
 #include <linux/of_pci.h>
 #include <linux/of_irq.h>
+#include <linux/gpio/consumer.h>
 
 #include "pcie-designware.h"
 

@@ -29,6 +30,7 @@ struct armada8k_pcie {
 	struct dw_pcie *pci;
 	struct clk *clk;
 	struct clk *clk_reg;
+	struct gpio_desc *reset_gpio;
 };
 
 #define PCIE_VENDOR_REGS_OFFSET		0x8000

@@ -137,6 +139,12 @@ static int armada8k_pcie_host_init(struct pcie_port *pp)
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
 	struct armada8k_pcie *pcie = to_armada8k_pcie(pci);
 
+	if (pcie->reset_gpio) {
+		/* assert and then deassert the reset signal */
+		gpiod_set_value_cansleep(pcie->reset_gpio, 1);
+		msleep(100);
+		gpiod_set_value_cansleep(pcie->reset_gpio, 0);
+	}
 	dw_pcie_setup_rc(pp);
 	armada8k_pcie_establish_link(pcie);
 

@@ -249,6 +257,14 @@ static int armada8k_pcie_probe(struct platform_device *pdev)
 		goto fail_clkreg;
 	}
 
+	/* Get reset gpio signal and hold asserted (logically high) */
+	pcie->reset_gpio = devm_gpiod_get_optional(dev, "reset",
+						   GPIOD_OUT_HIGH);
+	if (IS_ERR(pcie->reset_gpio)) {
+		ret = PTR_ERR(pcie->reset_gpio);
+		goto fail_clkreg;
+	}
+
 	platform_set_drvdata(pdev, pcie);
 
 	ret = armada8k_add_pcie_port(pcie, pdev);
Signature · 1 line
2.19.0
#1 lore raw

PATCH 1/1 PCI: armada8k: add support for gpio controlled reset signal

Quoted 4 lines · Baruch Siach

This commit adds support for the gpio reset signal binding as described in the designware-pcie.txt DT binding document. Both the documented 'reset-gpio' property name, and the more standard 'reset-gpios' name are supported.

Hi Baruch

I don't know this code at all, so maybe a dumb question. Why support the old none-standard binding of reset-gpio on new hardware? I'm assuming reset-gpio is marked a deprecated and reset-gpios is recommended?

Andrew

author
#2 lore raw

PATCH 1/1 PCI: armada8k: add support for gpio controlled reset signal

Hi Andrew,

Quoted 10 lines · Andrew Lunn
Quoted 4 lines · Baruch Siach

This commit adds support for the gpio reset signal binding as described in the designware-pcie.txt DT binding document. Both the documented 'reset-gpio' property name, and the more standard 'reset-gpios' name are supported.

I don't know this code at all, so maybe a dumb question. Why support the old none-standard binding of reset-gpio on new hardware? I'm assuming reset-gpio is marked a deprecated and reset-gpios is recommended?

This is all hidden behind the devm_gpiod_get_optional() call that only sees the "reset" string. The generic code supports both the new property name and also the older one for backward compatibility. This patch changes nothing in this regard.

The designware-pcie.txt document mentions the older 'reset-gpio' name. So I mentioned that name as well in the commit log.

baruch

Signature · 3 lines
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -
#3 lore raw

PATCH 1/1 PCI: armada8k: add support for gpio controlled reset signal

Quoted 14 lines · Baruch Siach

Hi Andrew,

Quoted 10 lines · Andrew Lunn
Quoted 4 lines · Baruch Siach

This commit adds support for the gpio reset signal binding as described in the designware-pcie.txt DT binding document. Both the documented 'reset-gpio' property name, and the more standard 'reset-gpios' name are supported.

I don't know this code at all, so maybe a dumb question. Why support the old none-standard binding of reset-gpio on new hardware? I'm assuming reset-gpio is marked a deprecated and reset-gpios is recommended?

Quoted 7 lines

This is all hidden behind the devm_gpiod_get_optional() call that only sees the "reset" string. The generic code supports both the new property name and also the older one for backward compatibility. This patch changes nothing in this regard.

The designware-pcie.txt document mentions the older 'reset-gpio' name. So I mentioned that name as well in the commit log.

I need Thomas' ACK to proceed with this patch.

Thanks,
Lorenzo

author
#4 lore raw

PATCH 1/1 PCI: armada8k: add support for gpio controlled reset signal

Hi Thomas,

As the listed maintainer of this driver, can you review this patch? This patch makes the PCIe slot reset sequence independent from the bootloader.

Thanks,
baruch

Quoted 18 lines · Lorenzo Pieralisi
Quoted 17 lines · Baruch Siach
Quoted 10 lines · Andrew Lunn
Quoted 4 lines · Baruch Siach

This commit adds support for the gpio reset signal binding as described in the designware-pcie.txt DT binding document. Both the documented 'reset-gpio' property name, and the more standard 'reset-gpios' name are supported.

I don't know this code at all, so maybe a dumb question. Why support the old none-standard binding of reset-gpio on new hardware? I'm assuming reset-gpio is marked a deprecated and reset-gpios is recommended?

This is all hidden behind the devm_gpiod_get_optional() call that only sees the "reset" string. The generic code supports both the new property name and also the older one for backward compatibility. This patch changes nothing in this regard.

Quoted 7 lines
Quoted 2 lines

The designware-pcie.txt document mentions the older 'reset-gpio' name. So I mentioned that name as well in the commit log.

I need Thomas' ACK to proceed with this patch.

Thanks,
Lorenzo

--
http://baruch.siach.name/blog/ ~. .~ Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
- baruch at tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

maintainer
#5 lore raw

PATCH 1/1 PCI: armada8k: add support for gpio controlled reset signal

Hello,

Quoted 3 lines · Baruch Siach

As the listed maintainer of this driver, can you review this patch? This patch makes the PCIe slot reset sequence independent from the bootloader.

Yes, I'll have a look. Sorry for the delay.

Thomas

Signature · 3 lines
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
maintainer
#6 lore raw

PATCH 1/1 PCI: armada8k: add support for gpio controlled reset signal

In reply to Baruch Siach

Hello Baruch,

Sorry for the delayed review.

drivers/pci/controller/dwc/pcie-armada8k.c static int armada8k_pcie_host_init(struct pcie_port *pp) · line 142

Quoted 6 lines · Baruch Siach

#define PCIE_VENDOR_REGS_OFFSET 0x8000

drivers/pci/controller/dwc/pcie-armada8k.c +1 -0
@@ -137,6 +139,12 @@ static int armada8k_pcie_host_init(struct pcie_port *pp)
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
 	struct armada8k_pcie *pcie = to_armada8k_pcie(pci);
 
+	if (pcie->reset_gpio) {

This should be:

	if (!IS_ERR(pcie->reset_gpio))

Indeed, in the case of an error, pcie->reset_gpio will be non-NULL, with the error encoded as a ERR_PTR().

drivers/pci/controller/dwc/pcie-armada8k.c static int armada8k_pcie_host_init(struct pcie_port *pp) · line 148

Quoted 6 lines
drivers/pci/controller/dwc/pcie-armada8k.c +5 -0
+		/* assert and then deassert the reset signal */
+		gpiod_set_value_cansleep(pcie->reset_gpio, 1);
+		msleep(100);
+		gpiod_set_value_cansleep(pcie->reset_gpio, 0);
+	}
 	dw_pcie_setup_rc(pp);

An empty new line here would be good before the dw_pcie_setup_rc() call.

If you send a new iteration with those two issues fixed, you can directly add my

Acked-by Thomas Petazzoni <[email protected]>

Thanks!

Thomas

Signature · 3 lines
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
maintainer
#7 lore raw

PATCH 1/1 PCI: armada8k: add support for gpio controlled reset signal

Hello,

Quoted 6 lines · Thomas Petazzoni

This should be:

	if (!IS_ERR(pcie->reset_gpio))

Indeed, in the case of an error, pcie->reset_gpio will be non-NULL, with the error encoded as a ERR_PTR().

Meh, scrap that. If pcie->reset_gpio was an error, probe() has failed. So by the time we are in armada8k_pcie_host_init(), pcie->reset_gpio is either NULL or a valid GPIO. So my comment was stupid.

Since the newline thing is way too minor to require a new iteration:

Acked-by Thomas Petazzoni <[email protected]>

Thanks, and sorry for the noise.

Thomas

Signature · 3 lines
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Applied
#8 lore raw

PATCH 1/1 PCI: armada8k: add support for gpio controlled reset signal

In reply to Baruch Siach
Quoted 9 lines · Baruch Siach

This commit adds support for the gpio reset signal binding as described in the designware-pcie.txt DT binding document. Both the documented 'reset-gpio' property name, and the more standard 'reset-gpios' name are supported.

Signed-off-by Baruch Siach <[email protected]>
---
 drivers/pci/controller/dwc/pcie-armada8k.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Applied to pci/dwc for v4.21, thanks.

Lorenzo

Quoted 51 lines
drivers/pci/controller/dwc/pcie-armada8k.c +16 -0
index 0c389a30ef5d..b171b6bc15c8 100644
@@ -22,6 +22,7 @@
 #include <linux/resource.h>
 #include <linux/of_pci.h>
 #include <linux/of_irq.h>
+#include <linux/gpio/consumer.h>
 
 #include "pcie-designware.h"
 

@@ -29,6 +30,7 @@ struct armada8k_pcie {
 	struct dw_pcie *pci;
 	struct clk *clk;
 	struct clk *clk_reg;
+	struct gpio_desc *reset_gpio;
 };
 
 #define PCIE_VENDOR_REGS_OFFSET		0x8000

@@ -137,6 +139,12 @@ static int armada8k_pcie_host_init(struct pcie_port *pp)
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
 	struct armada8k_pcie *pcie = to_armada8k_pcie(pci);
 
+	if (pcie->reset_gpio) {
+		/* assert and then deassert the reset signal */
+		gpiod_set_value_cansleep(pcie->reset_gpio, 1);
+		msleep(100);
+		gpiod_set_value_cansleep(pcie->reset_gpio, 0);
+	}
 	dw_pcie_setup_rc(pp);
 	armada8k_pcie_establish_link(pcie);
 

@@ -249,6 +257,14 @@ static int armada8k_pcie_probe(struct platform_device *pdev)
 		goto fail_clkreg;
 	}
 
+	/* Get reset gpio signal and hold asserted (logically high) */
+	pcie->reset_gpio = devm_gpiod_get_optional(dev, "reset",
+						   GPIOD_OUT_HIGH);
+	if (IS_ERR(pcie->reset_gpio)) {
+		ret = PTR_ERR(pcie->reset_gpio);
+		goto fail_clkreg;
+	}
+
 	platform_set_drvdata(pdev, pcie);
 
 	ret = armada8k_add_pcie_port(pcie, pdev);
Signature · 1 line
2.19.0