[PATCH 0/5] PCIe aardvark controller improvements

[PATCH 0/5] PCIe aardvark controller improvements

[Marek Behún]

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.

This series applies on top of Linus' master branch.

Marek & Pali

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 | 102 ++++++++++++++++----------
 drivers/pci/pci-bridge-emul.c         |   4 +
 3 files changed, 69 insertions(+), 39 deletions(-)

-- 
2.26.2

[PATCH 1/5] PCI: aardvark: Fix compilation on s390

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

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'

Link: https://lore.kernel.org/r/202006211118.LxtENQfl%25lkp@intel.com
Reported-by: kernel test robot <lkp@intel.com>
Fixes: 5169a9851da ("PCI: aardvark: Issue PERST via GPIO")
Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <marek.behun@nic.cz>
---
 drivers/pci/controller/pci-aardvark.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 90ff291c24f0..8caa80b19cf8 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -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>
-- 
2.26.2

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

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

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

Fixes: 8a3ebd8de328 ("PCI: aardvark: Implement emulated root PCI bridge config space")
Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <marek.behun@nic.cz>
---
 drivers/pci/controller/pci-aardvark.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 8caa80b19cf8..d5f58684d962 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -608,7 +608,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;
 
@@ -634,8 +634,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,
@@ -1169,7 +1168,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) {
-- 
2.26.2

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

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

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

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <marek.behun@nic.cz>
---
 drivers/pci/pci-bridge-emul.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/pci/pci-bridge-emul.c b/drivers/pci/pci-bridge-emul.c
index ccf26d12ec61..139869d50eb2 100644
--- a/drivers/pci/pci-bridge-emul.c
+++ b/drivers/pci/pci-bridge-emul.c
@@ -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);
-- 
2.26.2

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

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

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.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <marek.behun@nic.cz>
---
 drivers/pci/controller/Kconfig        |  2 +-
 drivers/pci/controller/pci-aardvark.c | 25 ++++++++++++++++++++++---
 2 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
index adddf21fa381..f9da5ff2c517 100644
--- a/drivers/pci/controller/Kconfig
+++ b/drivers/pci/controller/Kconfig
@@ -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
diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index d5f58684d962..0a5aa6d77f5d 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -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>
@@ -1114,6 +1115,7 @@ static int advk_pcie_probe(struct platform_device *pdev)
 
 	pcie = pci_host_bridge_priv(bridge);
 	pcie->pdev = pdev;
+	platform_set_drvdata(pdev, pcie);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	pcie->base = devm_ioremap_resource(dev, res);
@@ -1204,18 +1206,35 @@ 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_stop_root_bus(bridge->bus);
+	pci_remove_root_bus(bridge->bus);
+
+	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");
-- 
2.26.2

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

[Rob Herring]

On Wed, Jul 15, 2020 at 04:25:56PM +0200, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> 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.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Marek Behún <marek.behun@nic.cz>
> ---
>  drivers/pci/controller/Kconfig        |  2 +-
>  drivers/pci/controller/pci-aardvark.c | 25 ++++++++++++++++++++++---
>  2 files changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
> index adddf21fa381..f9da5ff2c517 100644
> --- a/drivers/pci/controller/Kconfig
> +++ b/drivers/pci/controller/Kconfig
> @@ -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
> diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> index d5f58684d962..0a5aa6d77f5d 100644
> --- a/drivers/pci/controller/pci-aardvark.c
> +++ b/drivers/pci/controller/pci-aardvark.c
> @@ -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>
> @@ -1114,6 +1115,7 @@ static int advk_pcie_probe(struct platform_device *pdev)
>  
>  	pcie = pci_host_bridge_priv(bridge);
>  	pcie->pdev = pdev;
> +	platform_set_drvdata(pdev, pcie);
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	pcie->base = devm_ioremap_resource(dev, res);
> @@ -1204,18 +1206,35 @@ 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_stop_root_bus(bridge->bus);
> +	pci_remove_root_bus(bridge->bus);

Based on pci_host_common_remove() implementation, doesn't this need a 
lock around it (pci_lock_rescan_remove)?

We should probably have a common function (say pci_bridge_remove) to 
implement this. You could use pci_host_common_remove(), but you'd have 
to adjust drvdata.

> +
> +	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");
> -- 
> 2.26.2
> 

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

[Pali Rohár]

On Wednesday 29 July 2020 12:48:09 Rob Herring wrote:
> On Wed, Jul 15, 2020 at 04:25:56PM +0200, Marek Behún wrote:
> > From: Pali Rohár <pali@kernel.org>
> > 
> > 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.
> > 
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > Reviewed-by: Marek Behún <marek.behun@nic.cz>
> > ---
> >  drivers/pci/controller/Kconfig        |  2 +-
> >  drivers/pci/controller/pci-aardvark.c | 25 ++++++++++++++++++++++---
> >  2 files changed, 23 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
> > index adddf21fa381..f9da5ff2c517 100644
> > --- a/drivers/pci/controller/Kconfig
> > +++ b/drivers/pci/controller/Kconfig
> > @@ -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
> > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> > index d5f58684d962..0a5aa6d77f5d 100644
> > --- a/drivers/pci/controller/pci-aardvark.c
> > +++ b/drivers/pci/controller/pci-aardvark.c
> > @@ -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>
> > @@ -1114,6 +1115,7 @@ static int advk_pcie_probe(struct platform_device *pdev)
> >  
> >  	pcie = pci_host_bridge_priv(bridge);
> >  	pcie->pdev = pdev;
> > +	platform_set_drvdata(pdev, pcie);
> >  
> >  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >  	pcie->base = devm_ioremap_resource(dev, res);
> > @@ -1204,18 +1206,35 @@ 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_stop_root_bus(bridge->bus);
> > +	pci_remove_root_bus(bridge->bus);
> 
> Based on pci_host_common_remove() implementation, doesn't this need a 
> lock around it (pci_lock_rescan_remove)?

Well, I'm not sure. I looked into other pci drivers and none of
following drivers pci-tegra.c, pcie-altera.c, pcie-brcmstb.c,
pcie-iproc.c, pcie-mediatek.c, pcie-rockchip-host.c calls
pci_lock_rescan_remove() and pci_unlock_rescan_remove().

Only pci-host-common.c and pci-hyperv.c protect pci_stop_root_bus() and
pci_remove_root_bus() by locks.

> We should probably have a common function (say pci_bridge_remove) to 
> implement this. You could use pci_host_common_remove(), but you'd have 
> to adjust drvdata.

Ok, I agree that some common function could be useful as the same code
(pci_stop_root_bus(); pci_remove_root_bus();) is called in more pci
drivers.

But first we need to know if locks are required or not.

> > +
> > +	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");
> > -- 
> > 2.26.2
> > 

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

[Rob Herring]

On Mon, Aug 3, 2020 at 8:46 AM Pali Rohár <pali@kernel.org> wrote:
>
> On Wednesday 29 July 2020 12:48:09 Rob Herring wrote:
> > On Wed, Jul 15, 2020 at 04:25:56PM +0200, Marek Behún wrote:
> > > From: Pali Rohár <pali@kernel.org>
> > >
> > > 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.
> > >
> > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > Reviewed-by: Marek Behún <marek.behun@nic.cz>
> > > ---
> > >  drivers/pci/controller/Kconfig        |  2 +-
> > >  drivers/pci/controller/pci-aardvark.c | 25 ++++++++++++++++++++++---
> > >  2 files changed, 23 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
> > > index adddf21fa381..f9da5ff2c517 100644
> > > --- a/drivers/pci/controller/Kconfig
> > > +++ b/drivers/pci/controller/Kconfig
> > > @@ -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
> > > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> > > index d5f58684d962..0a5aa6d77f5d 100644
> > > --- a/drivers/pci/controller/pci-aardvark.c
> > > +++ b/drivers/pci/controller/pci-aardvark.c
> > > @@ -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>
> > > @@ -1114,6 +1115,7 @@ static int advk_pcie_probe(struct platform_device *pdev)
> > >
> > >     pcie = pci_host_bridge_priv(bridge);
> > >     pcie->pdev = pdev;
> > > +   platform_set_drvdata(pdev, pcie);
> > >
> > >     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > >     pcie->base = devm_ioremap_resource(dev, res);
> > > @@ -1204,18 +1206,35 @@ 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_stop_root_bus(bridge->bus);
> > > +   pci_remove_root_bus(bridge->bus);
> >
> > Based on pci_host_common_remove() implementation, doesn't this need a
> > lock around it (pci_lock_rescan_remove)?
>
> Well, I'm not sure. I looked into other pci drivers and none of
> following drivers pci-tegra.c, pcie-altera.c, pcie-brcmstb.c,
> pcie-iproc.c, pcie-mediatek.c, pcie-rockchip-host.c calls
> pci_lock_rescan_remove() and pci_unlock_rescan_remove().

The mutex protects the bus->devices list, so yes I believe it is needed.

Rob

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

[Pali Rohár]

On Monday 03 August 2020 14:00:37 Rob Herring wrote:
> On Mon, Aug 3, 2020 at 8:46 AM Pali Rohár <pali@kernel.org> wrote:
> >
> > On Wednesday 29 July 2020 12:48:09 Rob Herring wrote:
> > > On Wed, Jul 15, 2020 at 04:25:56PM +0200, Marek Behún wrote:
> > > > From: Pali Rohár <pali@kernel.org>
> > > >
> > > > 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.
> > > >
> > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > > Reviewed-by: Marek Behún <marek.behun@nic.cz>
> > > > ---
> > > >  drivers/pci/controller/Kconfig        |  2 +-
> > > >  drivers/pci/controller/pci-aardvark.c | 25 ++++++++++++++++++++++---
> > > >  2 files changed, 23 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
> > > > index adddf21fa381..f9da5ff2c517 100644
> > > > --- a/drivers/pci/controller/Kconfig
> > > > +++ b/drivers/pci/controller/Kconfig
> > > > @@ -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
> > > > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> > > > index d5f58684d962..0a5aa6d77f5d 100644
> > > > --- a/drivers/pci/controller/pci-aardvark.c
> > > > +++ b/drivers/pci/controller/pci-aardvark.c
> > > > @@ -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>
> > > > @@ -1114,6 +1115,7 @@ static int advk_pcie_probe(struct platform_device *pdev)
> > > >
> > > >     pcie = pci_host_bridge_priv(bridge);
> > > >     pcie->pdev = pdev;
> > > > +   platform_set_drvdata(pdev, pcie);
> > > >
> > > >     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > >     pcie->base = devm_ioremap_resource(dev, res);
> > > > @@ -1204,18 +1206,35 @@ 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_stop_root_bus(bridge->bus);
> > > > +   pci_remove_root_bus(bridge->bus);
> > >
> > > Based on pci_host_common_remove() implementation, doesn't this need a
> > > lock around it (pci_lock_rescan_remove)?
> >
> > Well, I'm not sure. I looked into other pci drivers and none of
> > following drivers pci-tegra.c, pcie-altera.c, pcie-brcmstb.c,
> > pcie-iproc.c, pcie-mediatek.c, pcie-rockchip-host.c calls
> > pci_lock_rescan_remove() and pci_unlock_rescan_remove().
> 
> The mutex protects the bus->devices list, so yes I believe it is needed.
> 
> Rob

Thank you Rob! It means that all above pci drivers should be fixed too.

I will prepare a new version of aardvark patches with protecting pci
stop/remove functions. And later I can look at some common bridge remove
function for fixing those other pci drivers.

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

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

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.

Signed-off-by: Pali Rohár <pali@kernel.org>
Tested-by: Marek Behún <marek.behun@nic.cz>
---
 drivers/pci/controller/pci-aardvark.c | 64 ++++++++++++++-------------
 1 file changed, 34 insertions(+), 30 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 0a5aa6d77f5d..ab39b01474e8 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -253,6 +253,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;
@@ -300,6 +319,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.
@@ -332,31 +366,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;
@@ -433,15 +446,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);
 
 	/*
-- 
2.26.2