All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] Revert "PCI: armada8k: Add support for gpio controlled reset signal"
@ 2019-01-24 21:42 ` Baruch Siach
  0 siblings, 0 replies; 4+ messages in thread
From: Baruch Siach @ 2019-01-24 21:42 UTC (permalink / raw)
  To: Thomas Petazzoni, Lorenzo Pieralisi, Bjorn Helgaas
  Cc: Andrew Lunn, Baruch Siach, Sven Auhagen, Jason Cooper, linux-pci,
	Gregory Clement, Russell King, linux-arm-kernel,
	Sebastian Hesselbarth

This reverts commit 3d71746c420c1c1c27cf5c4e48f8fa0a6cfdc185.

That commit breaks boot on Macchiatobin when a Mellanox NIC is in the
PCIe slot.

It turns out that full reset cycle requires first comphy serdes
initialization. Reset signal toggle without comphy initialization makes
access to PCI configuration registers stall indefinitely. U-Boot toggles
the Macchiatobin PCIe reset line already at boot, after initializing the
comphy serdes.

So while commit 3d71746c42 ("PCI: armada8k: Add support for gpio
controlled reset signal") enables PCIe on platforms that U-Boot does
not (yet) touch the reset line (like Clearfog GT-8K), it breaks PCIe
(and boot) on the Macchiatobin.

Revert commit 3d71746c42 ("PCI: armada8k: Add support for gpio
controlled reset signal") entirely for now to fix the Macchaitobin
regression. Proper PCIe reset support would require PCIe comphy setup
support. That must wait for another kernel release.

Reported-by: Sven Auhagen <sven.auhagen@voleatech.de>
Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
v2: Add commit title to commit id mentions
---
 drivers/pci/controller/dwc/pcie-armada8k.c | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-armada8k.c b/drivers/pci/controller/dwc/pcie-armada8k.c
index b171b6bc15c8..0c389a30ef5d 100644
--- a/drivers/pci/controller/dwc/pcie-armada8k.c
+++ b/drivers/pci/controller/dwc/pcie-armada8k.c
@@ -22,7 +22,6 @@
 #include <linux/resource.h>
 #include <linux/of_pci.h>
 #include <linux/of_irq.h>
-#include <linux/gpio/consumer.h>
 
 #include "pcie-designware.h"
 
@@ -30,7 +29,6 @@ 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
@@ -139,12 +137,6 @@ 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);
 
@@ -257,14 +249,6 @@ 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);
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread
* [PATCH v2] Revert "PCI: armada8k: Add support for gpio controlled reset signal"
@ 2019-01-24 21:42 ` Baruch Siach
  0 siblings, 0 replies; 4+ messages in thread
From: Baruch Siach @ 2019-01-24 21:42 UTC (permalink / raw)
  To: Thomas Petazzoni, Lorenzo Pieralisi, Bjorn Helgaas
  Cc: Andrew Lunn, Baruch Siach, Sven Auhagen, Jason Cooper, linux-pci,
	Gregory Clement, Russell King, linux-arm-kernel,
	Sebastian Hesselbarth

This reverts commit 3d71746c420c1c1c27cf5c4e48f8fa0a6cfdc185.

That commit breaks boot on Macchiatobin when a Mellanox NIC is in the
PCIe slot.

It turns out that full reset cycle requires first comphy serdes
initialization. Reset signal toggle without comphy initialization makes
access to PCI configuration registers stall indefinitely. U-Boot toggles
the Macchiatobin PCIe reset line already at boot, after initializing the
comphy serdes.

So while commit 3d71746c42 ("PCI: armada8k: Add support for gpio
controlled reset signal") enables PCIe on platforms that U-Boot does
not (yet) touch the reset line (like Clearfog GT-8K), it breaks PCIe
(and boot) on the Macchiatobin.

Revert commit 3d71746c42 ("PCI: armada8k: Add support for gpio
controlled reset signal") entirely for now to fix the Macchaitobin
regression. Proper PCIe reset support would require PCIe comphy setup
support. That must wait for another kernel release.

Reported-by: Sven Auhagen <sven.auhagen@voleatech.de>
Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
v2: Add commit title to commit id mentions
---
 drivers/pci/controller/dwc/pcie-armada8k.c | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-armada8k.c b/drivers/pci/controller/dwc/pcie-armada8k.c
index b171b6bc15c8..0c389a30ef5d 100644
--- a/drivers/pci/controller/dwc/pcie-armada8k.c
+++ b/drivers/pci/controller/dwc/pcie-armada8k.c
@@ -22,7 +22,6 @@
 #include <linux/resource.h>
 #include <linux/of_pci.h>
 #include <linux/of_irq.h>
-#include <linux/gpio/consumer.h>
 
 #include "pcie-designware.h"
 
@@ -30,7 +29,6 @@ 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
@@ -139,12 +137,6 @@ 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);
 
@@ -257,14 +249,6 @@ 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);
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH v2] Revert "PCI: armada8k: Add support for gpio controlled reset signal"
  2019-01-24 21:42 ` Baruch Siach
@ 2019-01-25 11:58   ` Lorenzo Pieralisi
  -1 siblings, 0 replies; 4+ messages in thread
From: Lorenzo Pieralisi @ 2019-01-25 11:58 UTC (permalink / raw)
  To: Baruch Siach
  Cc: Thomas Petazzoni, Bjorn Helgaas, Andrew Lunn, Sven Auhagen,
	Jason Cooper, linux-pci, Gregory Clement, Russell King,
	linux-arm-kernel, Sebastian Hesselbarth

On Thu, Jan 24, 2019 at 11:42:45PM +0200, Baruch Siach wrote:
> This reverts commit 3d71746c420c1c1c27cf5c4e48f8fa0a6cfdc185.
> 
> That commit breaks boot on Macchiatobin when a Mellanox NIC is in the
> PCIe slot.
> 
> It turns out that full reset cycle requires first comphy serdes
> initialization. Reset signal toggle without comphy initialization makes
> access to PCI configuration registers stall indefinitely. U-Boot toggles
> the Macchiatobin PCIe reset line already at boot, after initializing the
> comphy serdes.
> 
> So while commit 3d71746c42 ("PCI: armada8k: Add support for gpio
> controlled reset signal") enables PCIe on platforms that U-Boot does
> not (yet) touch the reset line (like Clearfog GT-8K), it breaks PCIe
> (and boot) on the Macchiatobin.

I have applied it and will send it upstream but please do test
changes and make sure u-boot configurations are consistent, this
commit log does not look promising in that respect.

Thanks,
Lorenzo

> Revert commit 3d71746c42 ("PCI: armada8k: Add support for gpio
> controlled reset signal") entirely for now to fix the Macchaitobin
> regression. Proper PCIe reset support would require PCIe comphy setup
> support. That must wait for another kernel release.
> 
> Reported-by: Sven Auhagen <sven.auhagen@voleatech.de>
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
> v2: Add commit title to commit id mentions
> ---
>  drivers/pci/controller/dwc/pcie-armada8k.c | 16 ----------------
>  1 file changed, 16 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-armada8k.c b/drivers/pci/controller/dwc/pcie-armada8k.c
> index b171b6bc15c8..0c389a30ef5d 100644
> --- a/drivers/pci/controller/dwc/pcie-armada8k.c
> +++ b/drivers/pci/controller/dwc/pcie-armada8k.c
> @@ -22,7 +22,6 @@
>  #include <linux/resource.h>
>  #include <linux/of_pci.h>
>  #include <linux/of_irq.h>
> -#include <linux/gpio/consumer.h>
>  
>  #include "pcie-designware.h"
>  
> @@ -30,7 +29,6 @@ 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
> @@ -139,12 +137,6 @@ 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);
>  
> @@ -257,14 +249,6 @@ 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);
> -- 
> 2.20.1
> 

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH v2] Revert "PCI: armada8k: Add support for gpio controlled reset signal"
@ 2019-01-25 11:58   ` Lorenzo Pieralisi
  0 siblings, 0 replies; 4+ messages in thread
From: Lorenzo Pieralisi @ 2019-01-25 11:58 UTC (permalink / raw)
  To: Baruch Siach
  Cc: Andrew Lunn, Sven Auhagen, Jason Cooper, linux-pci,
	Gregory Clement, Russell King, Thomas Petazzoni, Bjorn Helgaas,
	linux-arm-kernel, Sebastian Hesselbarth

On Thu, Jan 24, 2019 at 11:42:45PM +0200, Baruch Siach wrote:
> This reverts commit 3d71746c420c1c1c27cf5c4e48f8fa0a6cfdc185.
> 
> That commit breaks boot on Macchiatobin when a Mellanox NIC is in the
> PCIe slot.
> 
> It turns out that full reset cycle requires first comphy serdes
> initialization. Reset signal toggle without comphy initialization makes
> access to PCI configuration registers stall indefinitely. U-Boot toggles
> the Macchiatobin PCIe reset line already at boot, after initializing the
> comphy serdes.
> 
> So while commit 3d71746c42 ("PCI: armada8k: Add support for gpio
> controlled reset signal") enables PCIe on platforms that U-Boot does
> not (yet) touch the reset line (like Clearfog GT-8K), it breaks PCIe
> (and boot) on the Macchiatobin.

I have applied it and will send it upstream but please do test
changes and make sure u-boot configurations are consistent, this
commit log does not look promising in that respect.

Thanks,
Lorenzo

> Revert commit 3d71746c42 ("PCI: armada8k: Add support for gpio
> controlled reset signal") entirely for now to fix the Macchaitobin
> regression. Proper PCIe reset support would require PCIe comphy setup
> support. That must wait for another kernel release.
> 
> Reported-by: Sven Auhagen <sven.auhagen@voleatech.de>
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
> v2: Add commit title to commit id mentions
> ---
>  drivers/pci/controller/dwc/pcie-armada8k.c | 16 ----------------
>  1 file changed, 16 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-armada8k.c b/drivers/pci/controller/dwc/pcie-armada8k.c
> index b171b6bc15c8..0c389a30ef5d 100644
> --- a/drivers/pci/controller/dwc/pcie-armada8k.c
> +++ b/drivers/pci/controller/dwc/pcie-armada8k.c
> @@ -22,7 +22,6 @@
>  #include <linux/resource.h>
>  #include <linux/of_pci.h>
>  #include <linux/of_irq.h>
> -#include <linux/gpio/consumer.h>
>  
>  #include "pcie-designware.h"
>  
> @@ -30,7 +29,6 @@ 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
> @@ -139,12 +137,6 @@ 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);
>  
> @@ -257,14 +249,6 @@ 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);
> -- 
> 2.20.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-01-25 11:58 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-24 21:42 [PATCH v2] Revert "PCI: armada8k: Add support for gpio controlled reset signal" Baruch Siach
2019-01-24 21:42 ` Baruch Siach
2019-01-25 11:58 ` Lorenzo Pieralisi
2019-01-25 11:58   ` Lorenzo Pieralisi

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.