Skip to contentSkip to conversation

PCIe aardvark controller improvements

PATCH Applied Pali Rohár 2 participants 7 messages Sep 7, 2020

Subsystems (3)
lore
Share & tools
lore

Pali Rohár

author maintainer
#0 lore raw

v3 cover PCIe aardvark controller improvements

Hi,

we have some more improvements for PCIe aardvark controller (Armada 3720 SOC - EspressoBIN and Turris MOX).

The main improvement is that with these patches the driver can be compiled as a module, and can be reloaded at runtime.

Marek & Pali

Changes in V3:
* Rebased on top of the v5.9-rc1 release

Changes in V2 for patch 4/5:
* Protect pci_stop_root_bus() and pci_remove_root_bus() function calls by pci_lock_rescan_remove() and pci_unlock_rescan_remove()

Pali Rohár (5):
PCI: aardvark: Fix compilation on s390
PCI: aardvark: Check for errors from pci_bridge_emul_init() call
PCI: pci-bridge-emul: Export API functions
PCI: aardvark: Implement driver 'remove' function and allow to build
it as module
PCI: aardvark: Move PCIe reset card code to advk_pcie_train_link()

 drivers/pci/controller/Kconfig        |   2 +-
 drivers/pci/controller/pci-aardvark.c | 104 ++++++++++++++++----------
 drivers/pci/pci-bridge-emul.c         |   4 +
 3 files changed, 71 insertions(+), 39 deletions(-)
Signature · 1 line
2.20.1
author maintainer
#1 lore raw

PATCH v3 1/5 PCI: aardvark: Fix compilation on s390

Include linux/gpio/consumer.h instead of linux/gpio.h, as is said in the latter file.

This was reported by kernel test bot when compiling for s390.

drivers/pci/controller/pci-aardvark.c:350:2: error: implicit declaration of function 'gpiod_set_value_cansleep' [-Werror,-Wimplicit-function-declaration] drivers/pci/controller/pci-aardvark.c:1074:21: error: implicit declaration of function 'devm_gpiod_get_from_of_node' [-Werror,-Wimplicit-function-declaration] drivers/pci/controller/pci-aardvark.c:1076:14: error: use of undeclared identifier 'GPIOD_OUT_LOW'

---
 drivers/pci/controller/pci-aardvark.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pci/controller/pci-aardvark.c +1 -1
index 1559f79e63b6..1c5f2fd47c51 100644
@@ -9,7 +9,7 @@
  */
 
 #include <linux/delay.h>
-#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <linux/irqdomain.h>
Signature · 1 line
2.20.1
author maintainer
#2 lore raw

PATCH v3 2/5 PCI: aardvark: Check for errors from pci_bridge_emul_init() call

In reply to Pali Rohár

Function pci_bridge_emul_init() may fail so correctly check for errors.

Fixes 8a3ebd8de328 ("PCI: aardvark: Implement emulated root PCI bridge config space")
---
 drivers/pci/controller/pci-aardvark.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)
drivers/pci/controller/pci-aardvark.c +7 -4
index 1c5f2fd47c51..2e2e2a2ff51d 100644
@@ -607,7 +607,7 @@ static struct pci_bridge_emul_ops advk_pci_bridge_emul_ops = {
  * Initialize the configuration space of the PCI-to-PCI bridge
  * associated with the given PCIe interface.
  */
-static void advk_sw_pci_bridge_init(struct advk_pcie *pcie)
+static int advk_sw_pci_bridge_init(struct advk_pcie *pcie)
 {
 	struct pci_bridge_emul *bridge = &pcie->bridge;
 
@@ -633,8 +633,7 @@ static void advk_sw_pci_bridge_init(struct advk_pcie *pcie)
 	bridge->data = pcie;
 	bridge->ops = &advk_pci_bridge_emul_ops;
 
-	pci_bridge_emul_init(bridge, 0);
-
+	return pci_bridge_emul_init(bridge, 0);
 }
 
 static bool advk_pcie_valid_device(struct advk_pcie *pcie, struct pci_bus *bus,
@@ -1167,7 +1166,11 @@ static int advk_pcie_probe(struct platform_device *pdev)
 
 	advk_pcie_setup_hw(pcie);
 
-	advk_sw_pci_bridge_init(pcie);
+	ret = advk_sw_pci_bridge_init(pcie);
+	if (ret) {
+		dev_err(dev, "Failed to register emulated root PCI bridge\n");
+		return ret;
+	}
 
 	ret = advk_pcie_init_irq_domain(pcie);
 	if (ret) {
Signature · 1 line
2.20.1
#3 lore raw
In reply to Pali Rohár
Quoted 5 lines · Pali Rohár

Hi,

we have some more improvements for PCIe aardvark controller (Armada 3720 SOC - EspressoBIN and Turris MOX).

Quoted 8 lines

The main improvement is that with these patches the driver can be compiled as a module, and can be reloaded at runtime.

Marek & Pali

Changes in V3:
* Rebased on top of the v5.9-rc1 release

Applied to pci/aardvark, thanks !

Lorenzo

Quoted 20 lines

Changes in V2 for patch 4/5:
* Protect pci_stop_root_bus() and pci_remove_root_bus() function calls by pci_lock_rescan_remove() and pci_unlock_rescan_remove()

Pali Rohár (5):
PCI: aardvark: Fix compilation on s390
PCI: aardvark: Check for errors from pci_bridge_emul_init() call
PCI: pci-bridge-emul: Export API functions
PCI: aardvark: Implement driver 'remove' function and allow to build
it as module
PCI: aardvark: Move PCIe reset card code to advk_pcie_train_link()

 drivers/pci/controller/Kconfig        |   2 +-
 drivers/pci/controller/pci-aardvark.c | 104 ++++++++++++++++----------
 drivers/pci/pci-bridge-emul.c         |   4 +
 3 files changed, 71 insertions(+), 39 deletions(-)
Signature · 1 line
2.20.1
author maintainer
#4 lore raw

PATCH v3 3/5 PCI: pci-bridge-emul: Export API functions

In reply to Pali Rohár

It allows kernel modules which are not compiled into kernel image to use pci-bridge-emul API functions.

---
 drivers/pci/pci-bridge-emul.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/pci/pci-bridge-emul.c +4 -0
index ccf26d12ec61..139869d50eb2 100644
@@ -294,6 +294,7 @@ int pci_bridge_emul_init(struct pci_bridge_emul *bridge,
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(pci_bridge_emul_init);
 
 /*
  * Cleanup a pci_bridge_emul structure that was previously initialized
@@ -305,6 +306,7 @@ void pci_bridge_emul_cleanup(struct pci_bridge_emul *bridge)
 		kfree(bridge->pcie_cap_regs_behavior);
 	kfree(bridge->pci_regs_behavior);
 }
+EXPORT_SYMBOL_GPL(pci_bridge_emul_cleanup);
 
 /*
  * Should be called by the PCI controller driver when reading the PCI
@@ -366,6 +368,7 @@ int pci_bridge_emul_conf_read(struct pci_bridge_emul *bridge, int where,
 
 	return PCIBIOS_SUCCESSFUL;
 }
+EXPORT_SYMBOL_GPL(pci_bridge_emul_conf_read);
 
 /*
  * Should be called by the PCI controller driver when writing the PCI
@@ -430,3 +433,4 @@ int pci_bridge_emul_conf_write(struct pci_bridge_emul *bridge, int where,
 
 	return PCIBIOS_SUCCESSFUL;
 }
+EXPORT_SYMBOL_GPL(pci_bridge_emul_conf_write);
Signature · 1 line
2.20.1
author maintainer
#5 lore raw

PATCH v3 4/5 PCI: aardvark: Implement driver 'remove' function and allow to build it as module

In reply to Pali Rohár

Providing driver's 'remove' function allows kernel to bind and unbind devices from aardvark driver. It also allows to build aardvark driver as a module.

Compiling aardvark as a module simplifies development and debugging of this driver as it can be reloaded at runtime without the need to reboot to new kernel.

---
 drivers/pci/controller/Kconfig        |  2 +-
 drivers/pci/controller/pci-aardvark.c | 27 ++++++++++++++++++++++++---
 2 files changed, 25 insertions(+), 4 deletions(-)
drivers/pci/controller/Kconfig +1 -1
index f18c3725ef80..a7aa22512a92 100644
@@ -12,7 +12,7 @@ config PCI_MVEBU
 	select PCI_BRIDGE_EMUL
 
 config PCI_AARDVARK
-	bool "Aardvark PCIe controller"
+	tristate "Aardvark PCIe controller"
 	depends on (ARCH_MVEBU && ARM64) || COMPILE_TEST
 	depends on OF
 	depends on PCI_MSI_IRQ_DOMAIN
drivers/pci/controller/pci-aardvark.c +24 -3
index 2e2e2a2ff51d..b16822e344ab 100644
@@ -14,6 +14,7 @@
 #include <linux/irq.h>
 #include <linux/irqdomain.h>
 #include <linux/kernel.h>
+#include <linux/module.h>
 #include <linux/pci.h>
 #include <linux/init.h>
 #include <linux/phy/phy.h>
@@ -1121,6 +1122,7 @@ static int advk_pcie_probe(struct platform_device *pdev)
 
 	pcie = pci_host_bridge_priv(bridge);
 	pcie->pdev = pdev;
+	platform_set_drvdata(pdev, pcie);
 
 	pcie->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(pcie->base))
@@ -1198,18 +1200,37 @@ static int advk_pcie_probe(struct platform_device *pdev)
 	return 0;
 }
 
+static int advk_pcie_remove(struct platform_device *pdev)
+{
+	struct advk_pcie *pcie = platform_get_drvdata(pdev);
+	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
+
+	pci_lock_rescan_remove();
+	pci_stop_root_bus(bridge->bus);
+	pci_remove_root_bus(bridge->bus);
+	pci_unlock_rescan_remove();
+
+	advk_pcie_remove_msi_irq_domain(pcie);
+	advk_pcie_remove_irq_domain(pcie);
+
+	return 0;
+}
+
 static const struct of_device_id advk_pcie_of_match_table[] = {
 	{ .compatible = "marvell,armada-3700-pcie", },
 	{},
 };
+MODULE_DEVICE_TABLE(of, advk_pcie_of_match_table);
 
 static struct platform_driver advk_pcie_driver = {
 	.driver = {
 		.name = "advk-pcie",
 		.of_match_table = advk_pcie_of_match_table,
-		/* Driver unloading/unbinding currently not supported */
-		.suppress_bind_attrs = true,
 	},
 	.probe = advk_pcie_probe,
+	.remove = advk_pcie_remove,
 };
-builtin_platform_driver(advk_pcie_driver);
+module_platform_driver(advk_pcie_driver);
+
+MODULE_DESCRIPTION("Aardvark PCIe controller");
+MODULE_LICENSE("GPL v2");
Signature · 1 line
2.20.1
author maintainer
#6 lore raw

PATCH v3 5/5 PCI: aardvark: Move PCIe reset card code to advk_pcie_train_link()

In reply to Pali Rohár

Move code which belongs to link training (delays and resets) into advk_pcie_train_link() function, so everything related to link training, including timings is at one place.

After experiments it can be observed that link training in aardvark hardware is very sensitive to timings and delays, so it is a good idea to have this code at the same place as link training calls.

This patch does not change behavior of aardvark initialization.

---
 drivers/pci/controller/pci-aardvark.c | 64 ++++++++++++++-------------
 1 file changed, 34 insertions(+), 30 deletions(-)
drivers/pci/controller/pci-aardvark.c +34 -30
index b16822e344ab..50ab6d7519ae 100644
@@ -252,6 +252,25 @@ static void advk_pcie_wait_for_retrain(struct advk_pcie *pcie)
 	}
 }
 
+static void advk_pcie_issue_perst(struct advk_pcie *pcie)
+{
+	u32 reg;
+
+	if (!pcie->reset_gpio)
+		return;
+
+	/* PERST does not work for some cards when link training is enabled */
+	reg = advk_readl(pcie, PCIE_CORE_CTRL0_REG);
+	reg &= ~LINK_TRAINING_EN;
+	advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG);
+
+	/* 10ms delay is needed for some cards */
+	dev_info(&pcie->pdev->dev, "issuing PERST via reset GPIO for 10ms\n");
+	gpiod_set_value_cansleep(pcie->reset_gpio, 1);
+	usleep_range(10000, 11000);
+	gpiod_set_value_cansleep(pcie->reset_gpio, 0);
+}
+
 static int advk_pcie_train_at_gen(struct advk_pcie *pcie, int gen)
 {
 	int ret, neg_gen;
@@ -299,6 +318,21 @@ static void advk_pcie_train_link(struct advk_pcie *pcie)
 	struct device *dev = &pcie->pdev->dev;
 	int neg_gen = -1, gen;
 
+	/*
+	 * Reset PCIe card via PERST# signal. Some cards are not detected
+	 * during link training when they are in some non-initial state.
+	 */
+	advk_pcie_issue_perst(pcie);
+
+	/*
+	 * PERST# signal could have been asserted by pinctrl subsystem before
+	 * probe() callback has been called or issued explicitly by reset gpio
+	 * function advk_pcie_issue_perst(), making the endpoint going into
+	 * fundamental reset. As required by PCI Express spec a delay for at
+	 * least 100ms after such a reset before link training is needed.
+	 */
+	msleep(PCI_PM_D3COLD_WAIT);
+
 	/*
 	 * Try link training at link gen specified by device tree property
 	 * 'max-link-speed'. If this fails, iteratively train at lower gen.
@@ -331,31 +365,10 @@ static void advk_pcie_train_link(struct advk_pcie *pcie)
 	dev_err(dev, "link never came up\n");
 }
 
-static void advk_pcie_issue_perst(struct advk_pcie *pcie)
-{
-	u32 reg;
-
-	if (!pcie->reset_gpio)
-		return;
-
-	/* PERST does not work for some cards when link training is enabled */
-	reg = advk_readl(pcie, PCIE_CORE_CTRL0_REG);
-	reg &= ~LINK_TRAINING_EN;
-	advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG);
-
-	/* 10ms delay is needed for some cards */
-	dev_info(&pcie->pdev->dev, "issuing PERST via reset GPIO for 10ms\n");
-	gpiod_set_value_cansleep(pcie->reset_gpio, 1);
-	usleep_range(10000, 11000);
-	gpiod_set_value_cansleep(pcie->reset_gpio, 0);
-}
-
 static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 {
 	u32 reg;
 
-	advk_pcie_issue_perst(pcie);
-
 	/* Enable TX */
 	reg = advk_readl(pcie, PCIE_CORE_REF_CLK_REG);
 	reg |= PCIE_CORE_REF_CLK_TX_ENABLE;
@@ -432,15 +445,6 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 	reg |= PIO_CTRL_ADDR_WIN_DISABLE;
 	advk_writel(pcie, reg, PIO_CTRL);
 
-	/*
-	 * PERST# signal could have been asserted by pinctrl subsystem before
-	 * probe() callback has been called or issued explicitly by reset gpio
-	 * function advk_pcie_issue_perst(), making the endpoint going into
-	 * fundamental reset. As required by PCI Express spec a delay for at
-	 * least 100ms after such a reset before link training is needed.
-	 */
-	msleep(PCI_PM_D3COLD_WAIT);
-
 	advk_pcie_train_link(pcie);
 
 	/*
Signature · 1 line
2.20.1