[PATCH v3 0/2] PCI: qcom: Add support for modular builds

[PATCH v3 0/2] PCI: qcom: Add support for modular builds

[Johan Hovold]

Allow the Qualcomm PCIe controller driver to be built as a module, which
is useful for multi-platform kernels as well as during development.

There's no rush with this, but I figured I'd send an updated version
that has been rebased on 6.1-rc1 (where post_deinit() has been removed).

I also broke out the qcom_pcie_host_deinit() handler in a separate patch
as the host_deinit() callback has now been added to dwc core and can be
used to fixes some late-probe error handling.

Johan


Changes in v3
 - rebase on 6.1-rc1 where post_deinit() has been removed
 - split out host-init error handling
 - add Stan's ack

Changes in v2
 - rebase on next-20220720 (adjust context)
 - add Rob and Mani's reviewed-by tags


Johan Hovold (2):
  PCI: qcom: Fix host-init error handling
  PCI: qcom: Add support for modular builds

 drivers/pci/controller/dwc/Kconfig     |  2 +-
 drivers/pci/controller/dwc/pcie-qcom.c | 39 +++++++++++++++++++++++---
 2 files changed, 36 insertions(+), 5 deletions(-)

-- 
2.37.3

[PATCH v3 1/2] PCI: qcom: Fix host-init error handling

[Johan Hovold]

Implement the new host_deinit() callback so that the PHY is powered off
and regulators and clocks are disabled also on late host-init errors.

Fixes: 82a823833f4e ("PCI: qcom: Add Qualcomm PCIe controller driver")
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 0c13f976626f..417be4d225ed 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -1534,8 +1534,19 @@ static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
 	return ret;
 }
 
+static void qcom_pcie_host_deinit(struct dw_pcie_rp *pp)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct qcom_pcie *pcie = to_qcom_pcie(pci);
+
+	qcom_ep_reset_assert(pcie);
+	phy_power_off(pcie->phy);
+	pcie->cfg->ops->deinit(pcie);
+}
+
 static const struct dw_pcie_host_ops qcom_pcie_dw_ops = {
-	.host_init = qcom_pcie_host_init,
+	.host_init	= qcom_pcie_host_init,
+	.host_deinit	= qcom_pcie_host_deinit,
 };
 
 /* Qcom IP rev.: 2.1.0	Synopsys IP rev.: 4.01a */
-- 
2.37.3

Re: [PATCH v3 1/2] PCI: qcom: Fix host-init error handling

[Manivannan Sadhasivam]

On Mon, Oct 17, 2022 at 01:47:04PM +0200, Johan Hovold wrote:
> Implement the new host_deinit() callback so that the PHY is powered off
> and regulators and clocks are disabled also on late host-init errors.
> 
> Fixes: 82a823833f4e ("PCI: qcom: Add Qualcomm PCIe controller driver")
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>

Thanks,
Mani

> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 0c13f976626f..417be4d225ed 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -1534,8 +1534,19 @@ static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
>  	return ret;
>  }
>  
> +static void qcom_pcie_host_deinit(struct dw_pcie_rp *pp)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	struct qcom_pcie *pcie = to_qcom_pcie(pci);
> +
> +	qcom_ep_reset_assert(pcie);
> +	phy_power_off(pcie->phy);
> +	pcie->cfg->ops->deinit(pcie);
> +}
> +
>  static const struct dw_pcie_host_ops qcom_pcie_dw_ops = {
> -	.host_init = qcom_pcie_host_init,
> +	.host_init	= qcom_pcie_host_init,
> +	.host_deinit	= qcom_pcie_host_deinit,
>  };
>  
>  /* Qcom IP rev.: 2.1.0	Synopsys IP rev.: 4.01a */
> -- 
> 2.37.3
> 

-- 
மணிவண்ணன் சதாசிவம்

[PATCH v3 2/2] PCI: qcom: Add support for modular builds

[Johan Hovold]

Allow the Qualcomm PCIe controller driver to be built as a module, which
is useful for multi-platform kernels as well as during development.

Reviewed-by: Rob Herring <robh@kernel.org>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Acked-by: Stanimir Varbanov <svarbanov@mm-sol.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/pci/controller/dwc/Kconfig     |  2 +-
 drivers/pci/controller/dwc/pcie-qcom.c | 26 +++++++++++++++++++++++---
 2 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
index 62ce3abf0f19..230f56d1a268 100644
--- a/drivers/pci/controller/dwc/Kconfig
+++ b/drivers/pci/controller/dwc/Kconfig
@@ -168,7 +168,7 @@ config PCI_HISI
 	  Hip05 and Hip06 SoCs
 
 config PCIE_QCOM
-	bool "Qualcomm PCIe controller"
+	tristate "Qualcomm PCIe controller"
 	depends on OF && (ARCH_QCOM || COMPILE_TEST)
 	depends on PCI_MSI_IRQ_DOMAIN
 	select PCIE_DW_HOST
diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 417be4d225ed..699172c22ed4 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -17,7 +17,7 @@
 #include <linux/io.h>
 #include <linux/iopoll.h>
 #include <linux/kernel.h>
-#include <linux/init.h>
+#include <linux/module.h>
 #include <linux/of_device.h>
 #include <linux/of_gpio.h>
 #include <linux/pci.h>
@@ -1820,6 +1820,21 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 	return ret;
 }
 
+static int qcom_pcie_remove(struct platform_device *pdev)
+{
+	struct qcom_pcie *pcie = platform_get_drvdata(pdev);
+	struct device *dev = &pdev->dev;
+
+	dw_pcie_host_deinit(&pcie->pci->pp);
+
+	phy_exit(pcie->phy);
+
+	pm_runtime_put_sync(dev);
+	pm_runtime_disable(dev);
+
+	return 0;
+}
+
 static const struct of_device_id qcom_pcie_match[] = {
 	{ .compatible = "qcom,pcie-apq8064", .data = &cfg_2_1_0 },
 	{ .compatible = "qcom,pcie-apq8084", .data = &cfg_1_0_0 },
@@ -1841,6 +1856,7 @@ static const struct of_device_id qcom_pcie_match[] = {
 	{ .compatible = "qcom,pcie-sm8450-pcie1", .data = &cfg_1_9_0 },
 	{ }
 };
+MODULE_DEVICE_TABLE(of, qcom_pcie_match);
 
 static void qcom_fixup_class(struct pci_dev *dev)
 {
@@ -1856,10 +1872,14 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_QCOM, 0x1001, qcom_fixup_class);
 
 static struct platform_driver qcom_pcie_driver = {
 	.probe = qcom_pcie_probe,
+	.remove = qcom_pcie_remove,
 	.driver = {
 		.name = "qcom-pcie",
-		.suppress_bind_attrs = true,
 		.of_match_table = qcom_pcie_match,
 	},
 };
-builtin_platform_driver(qcom_pcie_driver);
+module_platform_driver(qcom_pcie_driver);
+
+MODULE_AUTHOR("Stanimir Varbanov <svarbanov@mm-sol.com>");
+MODULE_DESCRIPTION("Qualcomm PCIe root complex driver");
+MODULE_LICENSE("GPL");
-- 
2.37.3

Re: [PATCH v3 2/2] PCI: qcom: Add support for modular builds

[Bjorn Helgaas]

On Mon, Oct 17, 2022 at 01:47:05PM +0200, Johan Hovold wrote:
> Allow the Qualcomm PCIe controller driver to be built as a module, which
> is useful for multi-platform kernels as well as during development.

There are two different goals here, and there's no real reason to
bundle them together:

  1) Make qcom a loadable module.  This is a hard requirement so
     multi-platform kernels don't need to build in all drivers
     statically.

  2) Make qcom unloadable.  This is a high want, possibly even a
     requirement for developers, but is not really a big issue for
     users.

There are different changes required: 1) requires the Kconfig change;
2) requires .remove() to be implemented.  Since there's no requirement
that these be done together, let's split them into separate patches.

Then we can make sure that at least 1) gets done, and if for any
reason 2) isn't safe or breaks something, we can at least bisect and
if necessary revert it without losing 1).

> Reviewed-by: Rob Herring <robh@kernel.org>
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Acked-by: Stanimir Varbanov <svarbanov@mm-sol.com>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  drivers/pci/controller/dwc/Kconfig     |  2 +-
>  drivers/pci/controller/dwc/pcie-qcom.c | 26 +++++++++++++++++++++++---
>  2 files changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> index 62ce3abf0f19..230f56d1a268 100644
> --- a/drivers/pci/controller/dwc/Kconfig
> +++ b/drivers/pci/controller/dwc/Kconfig
> @@ -168,7 +168,7 @@ config PCI_HISI
>  	  Hip05 and Hip06 SoCs
>  
>  config PCIE_QCOM
> -	bool "Qualcomm PCIe controller"
> +	tristate "Qualcomm PCIe controller"
>  	depends on OF && (ARCH_QCOM || COMPILE_TEST)
>  	depends on PCI_MSI_IRQ_DOMAIN
>  	select PCIE_DW_HOST
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 417be4d225ed..699172c22ed4 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -17,7 +17,7 @@
>  #include <linux/io.h>
>  #include <linux/iopoll.h>
>  #include <linux/kernel.h>
> -#include <linux/init.h>
> +#include <linux/module.h>
>  #include <linux/of_device.h>
>  #include <linux/of_gpio.h>
>  #include <linux/pci.h>
> @@ -1820,6 +1820,21 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>  	return ret;
>  }
>  
> +static int qcom_pcie_remove(struct platform_device *pdev)
> +{
> +	struct qcom_pcie *pcie = platform_get_drvdata(pdev);
> +	struct device *dev = &pdev->dev;
> +
> +	dw_pcie_host_deinit(&pcie->pci->pp);
> +
> +	phy_exit(pcie->phy);
> +
> +	pm_runtime_put_sync(dev);
> +	pm_runtime_disable(dev);
> +
> +	return 0;
> +}
> +
>  static const struct of_device_id qcom_pcie_match[] = {
>  	{ .compatible = "qcom,pcie-apq8064", .data = &cfg_2_1_0 },
>  	{ .compatible = "qcom,pcie-apq8084", .data = &cfg_1_0_0 },
> @@ -1841,6 +1856,7 @@ static const struct of_device_id qcom_pcie_match[] = {
>  	{ .compatible = "qcom,pcie-sm8450-pcie1", .data = &cfg_1_9_0 },
>  	{ }
>  };
> +MODULE_DEVICE_TABLE(of, qcom_pcie_match);
>  
>  static void qcom_fixup_class(struct pci_dev *dev)
>  {
> @@ -1856,10 +1872,14 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_QCOM, 0x1001, qcom_fixup_class);
>  
>  static struct platform_driver qcom_pcie_driver = {
>  	.probe = qcom_pcie_probe,
> +	.remove = qcom_pcie_remove,
>  	.driver = {
>  		.name = "qcom-pcie",
> -		.suppress_bind_attrs = true,
>  		.of_match_table = qcom_pcie_match,
>  	},
>  };
> -builtin_platform_driver(qcom_pcie_driver);
> +module_platform_driver(qcom_pcie_driver);
> +
> +MODULE_AUTHOR("Stanimir Varbanov <svarbanov@mm-sol.com>");
> +MODULE_DESCRIPTION("Qualcomm PCIe root complex driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.37.3
> 

Re: [PATCH v3 2/2] PCI: qcom: Add support for modular builds

[Johan Hovold]

On Mon, Oct 17, 2022 at 12:34:22PM -0500, Bjorn Helgaas wrote:
> On Mon, Oct 17, 2022 at 01:47:05PM +0200, Johan Hovold wrote:
> > Allow the Qualcomm PCIe controller driver to be built as a module, which
> > is useful for multi-platform kernels as well as during development.
> 
> There are two different goals here, and there's no real reason to
> bundle them together:
> 
>   1) Make qcom a loadable module.  This is a hard requirement so
>      multi-platform kernels don't need to build in all drivers
>      statically.
> 
>   2) Make qcom unloadable.  This is a high want, possibly even a
>      requirement for developers, but is not really a big issue for
>      users.
> 
> There are different changes required: 1) requires the Kconfig change;
> 2) requires .remove() to be implemented.  Since there's no requirement
> that these be done together, let's split them into separate patches.
> 
> Then we can make sure that at least 1) gets done, and if for any
> reason 2) isn't safe or breaks something, we can at least bisect and
> if necessary revert it without losing 1).

Implementing 1) in itself requires more than simply splitting this
patch. And I don't think we should be making life harder for developers,
as well as users assisting during debugging, by going in that direction.

We have tons of modules in the kernel and very few that cannot be
unloaded. Anyone who doesn't trust root to not unload modules can
always disable unloading completely using CONFIG_MODULE_UNLOAD.

Johan

Re: [PATCH v3 2/2] PCI: qcom: Add support for modular builds

[Bjorn Helgaas]

On Tue, Oct 18, 2022 at 05:10:53PM +0200, Johan Hovold wrote:
> On Mon, Oct 17, 2022 at 12:34:22PM -0500, Bjorn Helgaas wrote:
> > On Mon, Oct 17, 2022 at 01:47:05PM +0200, Johan Hovold wrote:
> > > Allow the Qualcomm PCIe controller driver to be built as a module, which
> > > is useful for multi-platform kernels as well as during development.
> > 
> > There are two different goals here, and there's no real reason to
> > bundle them together:
> > 
> >   1) Make qcom a loadable module.  This is a hard requirement so
> >      multi-platform kernels don't need to build in all drivers
> >      statically.
> > 
> >   2) Make qcom unloadable.  This is a high want, possibly even a
> >      requirement for developers, but is not really a big issue for
> >      users.
> > 
> > There are different changes required: 1) requires the Kconfig change;
> > 2) requires .remove() to be implemented.  Since there's no requirement
> > that these be done together, let's split them into separate patches.
> > 
> > Then we can make sure that at least 1) gets done, and if for any
> > reason 2) isn't safe or breaks something, we can at least bisect and
> > if necessary revert it without losing 1).
> 
> Implementing 1) in itself requires more than simply splitting this
> patch. And I don't think we should be making life harder for developers,
> as well as users assisting during debugging, by going in that direction.

If you're saying this patch can't be split, can you elaborate on the
details of *why* it can't be split?

> We have tons of modules in the kernel and very few that cannot be
> unloaded. Anyone who doesn't trust root to not unload modules can
> always disable unloading completely using CONFIG_MODULE_UNLOAD.

This is all true, but IIUC, the issue is about unloading IRQ
controller drivers, and this doesn't address that.  I don't have a
clear understanding of the issue, and it would be nice if a patch that
specifically added unloadability could elaborate on that.  Then we can
decide that "yes, this is a risk, and we're willing to accept it." An
argument that "tons of modules do this" totally avoids the issues of
this particular case.

Bjorn

Re: [PATCH v3 0/2] PCI: qcom: Add support for modular builds

[Lorenzo Pieralisi]

On Mon, Oct 17, 2022 at 01:47:03PM +0200, Johan Hovold wrote:
> Allow the Qualcomm PCIe controller driver to be built as a module, which
> is useful for multi-platform kernels as well as during development.
> 
> There's no rush with this, but I figured I'd send an updated version
> that has been rebased on 6.1-rc1 (where post_deinit() has been removed).
> 
> I also broke out the qcom_pcie_host_deinit() handler in a separate patch
> as the host_deinit() callback has now been added to dwc core and can be
> used to fixes some late-probe error handling.

Waiting for the dust to settle on patch 2, does it make sense to
merge patch 1 on its own ?

Thanks,
Lorenzo

> Johan
> 
> 
> Changes in v3
>  - rebase on 6.1-rc1 where post_deinit() has been removed
>  - split out host-init error handling
>  - add Stan's ack
> 
> Changes in v2
>  - rebase on next-20220720 (adjust context)
>  - add Rob and Mani's reviewed-by tags
> 
> 
> Johan Hovold (2):
>   PCI: qcom: Fix host-init error handling
>   PCI: qcom: Add support for modular builds
> 
>  drivers/pci/controller/dwc/Kconfig     |  2 +-
>  drivers/pci/controller/dwc/pcie-qcom.c | 39 +++++++++++++++++++++++---
>  2 files changed, 36 insertions(+), 5 deletions(-)
> 
> -- 
> 2.37.3
> 

Re: [PATCH v3 0/2] PCI: qcom: Add support for modular builds

[Manivannan Sadhasivam]

On Fri, Jan 13, 2023 at 04:51:13PM +0100, Lorenzo Pieralisi wrote:
> On Mon, Oct 17, 2022 at 01:47:03PM +0200, Johan Hovold wrote:
> > Allow the Qualcomm PCIe controller driver to be built as a module, which
> > is useful for multi-platform kernels as well as during development.
> > 
> > There's no rush with this, but I figured I'd send an updated version
> > that has been rebased on 6.1-rc1 (where post_deinit() has been removed).
> > 
> > I also broke out the qcom_pcie_host_deinit() handler in a separate patch
> > as the host_deinit() callback has now been added to dwc core and can be
> > used to fixes some late-probe error handling.
> 
> Waiting for the dust to settle on patch 2, does it make sense to
> merge patch 1 on its own ?
> 

Yeah, that makes sense.

Thanks,
Mani

> Thanks,
> Lorenzo
> 
> > Johan
> > 
> > 
> > Changes in v3
> >  - rebase on 6.1-rc1 where post_deinit() has been removed
> >  - split out host-init error handling
> >  - add Stan's ack
> > 
> > Changes in v2
> >  - rebase on next-20220720 (adjust context)
> >  - add Rob and Mani's reviewed-by tags
> > 
> > 
> > Johan Hovold (2):
> >   PCI: qcom: Fix host-init error handling
> >   PCI: qcom: Add support for modular builds
> > 
> >  drivers/pci/controller/dwc/Kconfig     |  2 +-
> >  drivers/pci/controller/dwc/pcie-qcom.c | 39 +++++++++++++++++++++++---
> >  2 files changed, 36 insertions(+), 5 deletions(-)
> > 
> > -- 
> > 2.37.3
> > 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v3 0/2] PCI: qcom: Add support for modular builds

[Johan Hovold]

On Fri, Jan 13, 2023 at 04:51:13PM +0100, Lorenzo Pieralisi wrote:
> On Mon, Oct 17, 2022 at 01:47:03PM +0200, Johan Hovold wrote:
> > Allow the Qualcomm PCIe controller driver to be built as a module, which
> > is useful for multi-platform kernels as well as during development.
> > 
> > There's no rush with this, but I figured I'd send an updated version
> > that has been rebased on 6.1-rc1 (where post_deinit() has been removed).
> > 
> > I also broke out the qcom_pcie_host_deinit() handler in a separate patch
> > as the host_deinit() callback has now been added to dwc core and can be
> > used to fixes some late-probe error handling.
> 
> Waiting for the dust to settle on patch 2, does it make sense to
> merge patch 1 on its own ?

Sure, please do.

I haven't found the time and motivation to reiterate the arguments for
why I think blocking patch 2 is misguided, but I'm still confident that
anyone reviewing the arguments will reach the same conclusion (e.g. if
you want to dig through the archives). I'll get to it eventually.

Johan

> > Changes in v3
> >  - rebase on 6.1-rc1 where post_deinit() has been removed
> >  - split out host-init error handling
> >  - add Stan's ack
> > 
> > Changes in v2
> >  - rebase on next-20220720 (adjust context)
> >  - add Rob and Mani's reviewed-by tags
> > 
> > 
> > Johan Hovold (2):
> >   PCI: qcom: Fix host-init error handling
> >   PCI: qcom: Add support for modular builds
> > 
> >  drivers/pci/controller/dwc/Kconfig     |  2 +-
> >  drivers/pci/controller/dwc/pcie-qcom.c | 39 +++++++++++++++++++++++---
> >  2 files changed, 36 insertions(+), 5 deletions(-)
> > 
> > -- 
> > 2.37.3
> > 

Re: (subset) [PATCH v3 0/2] PCI: qcom: Add support for modular builds

[Lorenzo Pieralisi]

On Mon, 17 Oct 2022 13:47:03 +0200, Johan Hovold wrote:
> Allow the Qualcomm PCIe controller driver to be built as a module, which
> is useful for multi-platform kernels as well as during development.
> 
> There's no rush with this, but I figured I'd send an updated version
> that has been rebased on 6.1-rc1 (where post_deinit() has been removed).
> 
> I also broke out the qcom_pcie_host_deinit() handler in a separate patch
> as the host_deinit() callback has now been added to dwc core and can be
> used to fixes some late-probe error handling.
> 
> [...]

Applied to pci/qcom, thanks!

[1/2] PCI: qcom: Fix host-init error handling
      https://git.kernel.org/lpieralisi/pci/c/6996d631fa33

Thanks,
Lorenzo