linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] PCI: qcom: Add support for modular builds
@ 2022-05-19  9:46 Johan Hovold
  2022-05-26 20:53 ` Rob Herring
                   ` (4 more replies)
  0 siblings, 5 replies; 13+ messages in thread
From: Johan Hovold @ 2022-05-19  9:46 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: Rob Herring, Krzysztof Wilczyński, Bjorn Helgaas,
	Stanimir Varbanov, Andy Gross, Bjorn Andersson, linux-pci,
	linux-arm-msm, linux-kernel, 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.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/pci/controller/dwc/Kconfig     |  2 +-
 drivers/pci/controller/dwc/pcie-qcom.c | 36 +++++++++++++++++++++++---
 2 files changed, 34 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 8523b5ef9d16..e25d5c09657c 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -16,7 +16,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>
@@ -1425,6 +1425,15 @@ static int qcom_pcie_host_init(struct pcie_port *pp)
 	return ret;
 }
 
+static void qcom_pcie_host_deinit(struct qcom_pcie *pcie)
+{
+	qcom_ep_reset_assert(pcie);
+	if (pcie->cfg->ops->post_deinit)
+		pcie->cfg->ops->post_deinit(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,
 };
@@ -1651,6 +1660,22 @@ 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);
+	qcom_pcie_host_deinit(pcie);
+
+	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-apq8084", .data = &apq8084_cfg },
 	{ .compatible = "qcom,pcie-ipq8064", .data = &ipq8064_cfg },
@@ -1669,6 +1694,7 @@ static const struct of_device_id qcom_pcie_match[] = {
 	{ .compatible = "qcom,pcie-sc7280", .data = &sc7280_cfg },
 	{ }
 };
+MODULE_DEVICE_TABLE(of, qcom_pcie_match);
 
 static void qcom_fixup_class(struct pci_dev *dev)
 {
@@ -1684,10 +1710,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.35.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] PCI: qcom: Add support for modular builds
  2022-05-19  9:46 [PATCH] PCI: qcom: Add support for modular builds Johan Hovold
@ 2022-05-26 20:53 ` Rob Herring
  2022-06-23 11:40   ` Johan Hovold
  2022-06-23 15:52 ` Bjorn Helgaas
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 13+ messages in thread
From: Rob Herring @ 2022-05-26 20:53 UTC (permalink / raw)
  To: Johan Hovold
  Cc: Lorenzo Pieralisi, Krzysztof Wilczyński, Bjorn Helgaas,
	Stanimir Varbanov, Andy Gross, Bjorn Andersson, linux-pci,
	linux-arm-msm, linux-kernel

On Thu, May 19, 2022 at 11:46:46AM +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.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  drivers/pci/controller/dwc/Kconfig     |  2 +-
>  drivers/pci/controller/dwc/pcie-qcom.c | 36 +++++++++++++++++++++++---
>  2 files changed, 34 insertions(+), 4 deletions(-)

Reviewed-by: Rob Herring <robh@kernel.org>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] PCI: qcom: Add support for modular builds
  2022-05-26 20:53 ` Rob Herring
@ 2022-06-23 11:40   ` Johan Hovold
  0 siblings, 0 replies; 13+ messages in thread
From: Johan Hovold @ 2022-06-23 11:40 UTC (permalink / raw)
  To: Lorenzo Pieralisi, Bjorn Helgaas
  Cc: Johan Hovold, Rob Herring, Krzysztof Wilczyński,
	Stanimir Varbanov, Andy Gross, Bjorn Andersson, linux-pci,
	linux-arm-msm, linux-kernel

On Thu, May 26, 2022 at 03:53:04PM -0500, Rob Herring wrote:
> On Thu, May 19, 2022 at 11:46:46AM +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.
> > 
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > ---
> >  drivers/pci/controller/dwc/Kconfig     |  2 +-
> >  drivers/pci/controller/dwc/pcie-qcom.c | 36 +++++++++++++++++++++++---
> >  2 files changed, 34 insertions(+), 4 deletions(-)
> 
> Reviewed-by: Rob Herring <robh@kernel.org>

Lorenzo, Bjorn H, any comments to this one?

Johan

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] PCI: qcom: Add support for modular builds
  2022-05-19  9:46 [PATCH] PCI: qcom: Add support for modular builds Johan Hovold
  2022-05-26 20:53 ` Rob Herring
@ 2022-06-23 15:52 ` Bjorn Helgaas
  2022-06-27  7:31   ` Johan Hovold
  2022-07-14 12:19 ` Stanimir Varbanov
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 13+ messages in thread
From: Bjorn Helgaas @ 2022-06-23 15:52 UTC (permalink / raw)
  To: Johan Hovold
  Cc: Lorenzo Pieralisi, Rob Herring, Krzysztof Wilczyński,
	Bjorn Helgaas, Stanimir Varbanov, Andy Gross, Bjorn Andersson,
	linux-pci, linux-arm-msm, linux-kernel

On Thu, May 19, 2022 at 11:46:46AM +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.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  drivers/pci/controller/dwc/Kconfig     |  2 +-
>  drivers/pci/controller/dwc/pcie-qcom.c | 36 +++++++++++++++++++++++---
>  2 files changed, 34 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 8523b5ef9d16..e25d5c09657c 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -16,7 +16,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>
> @@ -1425,6 +1425,15 @@ static int qcom_pcie_host_init(struct pcie_port *pp)
>  	return ret;
>  }
>  
> +static void qcom_pcie_host_deinit(struct qcom_pcie *pcie)
> +{
> +	qcom_ep_reset_assert(pcie);
> +	if (pcie->cfg->ops->post_deinit)
> +		pcie->cfg->ops->post_deinit(pcie);
> +	phy_power_off(pcie->phy);
> +	pcie->cfg->ops->deinit(pcie);

These post_deinit/deinit names look backwards.  Why would we call a
"post_deinit()" method *before* the "deinit()" method?  It would make
sense if we called "pre_deinit()" followed by "deinit()".

>  static const struct dw_pcie_host_ops qcom_pcie_dw_ops = {
>  	.host_init = qcom_pcie_host_init,
>  };
> @@ -1651,6 +1660,22 @@ 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);
> +	qcom_pcie_host_deinit(pcie);
> +
> +	phy_exit(pcie->phy);
> +
> +	pm_runtime_put_sync(dev);
> +	pm_runtime_disable(dev);

Why is this not more symmetric with qcom_pcie_probe()?  Maybe struct
dw_pcie_host_ops needs a new .host_deinit() pointer that would be
called from dw_pcie_host_deinit()?

In the probe path, we have this:

  qcom_pcie_probe
    pm_runtime_enable
    pm_runtime_get_sync
    phy_init(pcie->phy)
    dw_pcie_host_init
      pp->ops->host_init
        qcom_pcie_host_init             # .host_init()
          pcie->cfg->ops->init(pcie)
          phy_power_on(pcie->phy)
          pcie->cfg->ops->post_init(pcie)
          qcom_ep_reset_deassert(pcie)

The remove path does do things in the opposite order, which makes
sense, but the call to qcom_pcie_host_deinit() breaks the symmetry:

  qcom_pcie_remove
    dw_pcie_host_deinit
    qcom_pcie_host_deinit
      qcom_ep_reset_assert
      pcie->cfg->ops->post_deinit
      phy_power_off(pcie->phy)
      pcie->cfg->ops->deinit
    phy_exit(pcie->phy)
    pm_runtime_put_sync
    pm_runtime_disable

> +	return 0;
> +}
> +
>  static const struct of_device_id qcom_pcie_match[] = {
>  	{ .compatible = "qcom,pcie-apq8084", .data = &apq8084_cfg },
>  	{ .compatible = "qcom,pcie-ipq8064", .data = &ipq8064_cfg },
> @@ -1669,6 +1694,7 @@ static const struct of_device_id qcom_pcie_match[] = {
>  	{ .compatible = "qcom,pcie-sc7280", .data = &sc7280_cfg },
>  	{ }
>  };
> +MODULE_DEVICE_TABLE(of, qcom_pcie_match);
>  
>  static void qcom_fixup_class(struct pci_dev *dev)
>  {
> @@ -1684,10 +1710,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.35.1
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] PCI: qcom: Add support for modular builds
  2022-06-23 15:52 ` Bjorn Helgaas
@ 2022-06-27  7:31   ` Johan Hovold
  0 siblings, 0 replies; 13+ messages in thread
From: Johan Hovold @ 2022-06-27  7:31 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Johan Hovold, Lorenzo Pieralisi, Rob Herring,
	Krzysztof Wilczyński, Bjorn Helgaas, Stanimir Varbanov,
	Andy Gross, Bjorn Andersson, linux-pci, linux-arm-msm,
	linux-kernel

On Thu, Jun 23, 2022 at 10:52:13AM -0500, Bjorn Helgaas wrote:
> On Thu, May 19, 2022 at 11:46:46AM +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.
> > 
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > ---

> > +static void qcom_pcie_host_deinit(struct qcom_pcie *pcie)
> > +{
> > +	qcom_ep_reset_assert(pcie);
> > +	if (pcie->cfg->ops->post_deinit)
> > +		pcie->cfg->ops->post_deinit(pcie);
> > +	phy_power_off(pcie->phy);
> > +	pcie->cfg->ops->deinit(pcie);
> 
> These post_deinit/deinit names look backwards.  Why would we call a
> "post_deinit()" method *before* the "deinit()" method?  It would make
> sense if we called "pre_deinit()" followed by "deinit()".

Yeah, that annoys me as well, but those are the names the driver
currently use.

I considered renaming the deinit callback but instead sent a follow up
patch to remove both of these callbacks now that the pipe clock rework
that depends on them has been merged, but seems like the post_init one
will be needed for the DBI accesses.

I can respin the above mentioned patch to drop or or rename the badly
named one when things settle down a bit.

> >  static const struct dw_pcie_host_ops qcom_pcie_dw_ops = {
> >  	.host_init = qcom_pcie_host_init,
> >  };
> > @@ -1651,6 +1660,22 @@ 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);
> > +	qcom_pcie_host_deinit(pcie);
> > +
> > +	phy_exit(pcie->phy);
> > +
> > +	pm_runtime_put_sync(dev);
> > +	pm_runtime_disable(dev);
> 
> Why is this not more symmetric with qcom_pcie_probe()?  Maybe struct
> dw_pcie_host_ops needs a new .host_deinit() pointer that would be
> called from dw_pcie_host_deinit()?

Yeah, I considered that too but decided it's not needed to implement
modular builds for this driver. Instead I did the ground work by adding
a deinit helper function so that it's easier to track any additions to
the host_init() callback and which can later be called by core if
someone decides to clean up core and add a deinit callback.

Looks like someone just posted something along these lines, but this
would conflict with the split MSI series which is otherwise ready to be
merged:

	https://lore.kernel.org/r/20220624143947.8991-9-Sergey.Semin@baikalelectronics.ru

Also note that there are other drivers that implement remove() without
this callback already today.

> In the probe path, we have this:
> 
>   qcom_pcie_probe
>     pm_runtime_enable
>     pm_runtime_get_sync
>     phy_init(pcie->phy)
>     dw_pcie_host_init
>       pp->ops->host_init
>         qcom_pcie_host_init             # .host_init()
>           pcie->cfg->ops->init(pcie)
>           phy_power_on(pcie->phy)
>           pcie->cfg->ops->post_init(pcie)
>           qcom_ep_reset_deassert(pcie)
> 
> The remove path does do things in the opposite order, which makes
> sense, but the call to qcom_pcie_host_deinit() breaks the symmetry:
> 
>   qcom_pcie_remove
>     dw_pcie_host_deinit
>     qcom_pcie_host_deinit
>       qcom_ep_reset_assert
>       pcie->cfg->ops->post_deinit
>       phy_power_off(pcie->phy)
>       pcie->cfg->ops->deinit
>     phy_exit(pcie->phy)
>     pm_runtime_put_sync
>     pm_runtime_disable

Yeah, I didn't want to go rewrite core just for this basic driver
functionality. Especially with so many things already in flux.

As mentioned above, everything is instead prepared to move over to such
a callback if and when it materialises.

Johan

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] PCI: qcom: Add support for modular builds
  2022-05-19  9:46 [PATCH] PCI: qcom: Add support for modular builds Johan Hovold
  2022-05-26 20:53 ` Rob Herring
  2022-06-23 15:52 ` Bjorn Helgaas
@ 2022-07-14 12:19 ` Stanimir Varbanov
  2022-07-14 13:05   ` Dmitry Baryshkov
  2022-07-14 13:10   ` Johan Hovold
  2022-07-20 16:27 ` Manivannan Sadhasivam
  2022-07-20 21:20 ` Bjorn Helgaas
  4 siblings, 2 replies; 13+ messages in thread
From: Stanimir Varbanov @ 2022-07-14 12:19 UTC (permalink / raw)
  To: Johan Hovold, Lorenzo Pieralisi
  Cc: Rob Herring, Krzysztof Wilczyński, Bjorn Helgaas,
	Andy Gross, Bjorn Andersson, linux-pci, linux-arm-msm,
	linux-kernel

Hi Johan,

Please take a look why we made it built-in first [1].

If arguments there are still valid I don't see why to make it a module
again.

[1] https://lkml.org/lkml/2016/8/24/694

On 5/19/22 12:46, 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.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  drivers/pci/controller/dwc/Kconfig     |  2 +-
>  drivers/pci/controller/dwc/pcie-qcom.c | 36 +++++++++++++++++++++++---
>  2 files changed, 34 insertions(+), 4 deletions(-)
> 


-- 
regards,
Stan

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] PCI: qcom: Add support for modular builds
  2022-07-14 12:19 ` Stanimir Varbanov
@ 2022-07-14 13:05   ` Dmitry Baryshkov
  2022-07-15 16:56     ` Bjorn Helgaas
  2022-07-14 13:10   ` Johan Hovold
  1 sibling, 1 reply; 13+ messages in thread
From: Dmitry Baryshkov @ 2022-07-14 13:05 UTC (permalink / raw)
  To: Stanimir Varbanov, Johan Hovold, Lorenzo Pieralisi
  Cc: Rob Herring, Krzysztof Wilczyński, Bjorn Helgaas,
	Andy Gross, Bjorn Andersson, linux-pci, linux-arm-msm,
	linux-kernel

Hi Stanimir,

On 14/07/2022 15:19, Stanimir Varbanov wrote:
> Hi Johan,
> 
> Please take a look why we made it built-in first [1].
> 
> If arguments there are still valid I don't see why to make it a module
> again.
> 
> [1] https://lkml.org/lkml/2016/8/24/694

It looks like there is a move to make all non-essential drivers 
buildable as modules. For example, the Kirin, dra7xx, Meson PCI 
controllers are now buildable as modules. So I think we can follow that 
and allow building the pcie-qcom as a module.

> 
> On 5/19/22 12:46, 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.
>>
>> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
>> ---
>>   drivers/pci/controller/dwc/Kconfig     |  2 +-
>>   drivers/pci/controller/dwc/pcie-qcom.c | 36 +++++++++++++++++++++++---
>>   2 files changed, 34 insertions(+), 4 deletions(-)
>>
> 
> 


-- 
With best wishes
Dmitry

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] PCI: qcom: Add support for modular builds
  2022-07-14 12:19 ` Stanimir Varbanov
  2022-07-14 13:05   ` Dmitry Baryshkov
@ 2022-07-14 13:10   ` Johan Hovold
  1 sibling, 0 replies; 13+ messages in thread
From: Johan Hovold @ 2022-07-14 13:10 UTC (permalink / raw)
  To: Stanimir Varbanov
  Cc: Johan Hovold, Lorenzo Pieralisi, Rob Herring,
	Krzysztof Wilczyński, Bjorn Helgaas, Andy Gross,
	Bjorn Andersson, linux-pci, linux-arm-msm, linux-kernel

On Thu, Jul 14, 2022 at 03:19:49PM +0300, Stanimir Varbanov wrote:
> Hi Johan,
> 
> Please take a look why we made it built-in first [1].
> 
> If arguments there are still valid I don't see why to make it a module
> again.

Yeah, I've seen that patch, and many just like that one by the same
author, and I don't think the arguments spelled out there are valid.

Sure, the Kconfig symbol for this driver was bool at the time so the
remove() code could not have received much testing, but the patch
ignores the fact that preventing drivers to be built as modules is
detrimental to multi-platform builds (e.g. Android GKI).

As I mention in passing below, being able to build a driver as a module
is also really useful during development. Not least to be able to test
power-sequencing and making sure that you're not unknowingly relying on
boot firmware to have set things up for you.

> [1] https://lkml.org/lkml/2016/8/24/694
> 
> On 5/19/22 12:46, 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.
> > 
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > ---
> >  drivers/pci/controller/dwc/Kconfig     |  2 +-
> >  drivers/pci/controller/dwc/pcie-qcom.c | 36 +++++++++++++++++++++++---
> >  2 files changed, 34 insertions(+), 4 deletions(-)

Johan

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] PCI: qcom: Add support for modular builds
  2022-07-14 13:05   ` Dmitry Baryshkov
@ 2022-07-15 16:56     ` Bjorn Helgaas
  2022-07-18  7:43       ` Stanimir Varbanov
  0 siblings, 1 reply; 13+ messages in thread
From: Bjorn Helgaas @ 2022-07-15 16:56 UTC (permalink / raw)
  To: Dmitry Baryshkov
  Cc: Stanimir Varbanov, Johan Hovold, Lorenzo Pieralisi, Rob Herring,
	Krzysztof Wilczyński, Bjorn Helgaas, Andy Gross,
	Bjorn Andersson, linux-pci, linux-arm-msm, linux-kernel

On Thu, Jul 14, 2022 at 04:05:41PM +0300, Dmitry Baryshkov wrote:
> On 14/07/2022 15:19, Stanimir Varbanov wrote:
> > Please take a look why we made it built-in first [1].
> > 
> > If arguments there are still valid I don't see why to make it a module
> > again.
> > 
> > [1] https://lkml.org/lkml/2016/8/24/694
> 
> It looks like there is a move to make all non-essential drivers buildable as
> modules. For example, the Kirin, dra7xx, Meson PCI controllers are now
> buildable as modules. So I think we can follow that and allow building the
> pcie-qcom as a module.

IIUC the arguments in [1] are that:

  - Kconfig is bool, so it can't be built as a module
  - there's no sensible use case for unbind

Those described the situation at the time, and there's no point in
having .remove() and using module_platform_driver() if Kconfig is
bool.

But they don't seem like arguments for why the driver couldn't be
*made* modular.

I think drivers *should* be modular unless there's a technical reason
they can't be.

Bjorn

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] PCI: qcom: Add support for modular builds
  2022-07-15 16:56     ` Bjorn Helgaas
@ 2022-07-18  7:43       ` Stanimir Varbanov
  0 siblings, 0 replies; 13+ messages in thread
From: Stanimir Varbanov @ 2022-07-18  7:43 UTC (permalink / raw)
  To: Bjorn Helgaas, Dmitry Baryshkov
  Cc: Johan Hovold, Lorenzo Pieralisi, Rob Herring,
	Krzysztof Wilczyński, Bjorn Helgaas, Andy Gross,
	Bjorn Andersson, linux-pci, linux-arm-msm, linux-kernel



On 7/15/22 19:56, Bjorn Helgaas wrote:
> On Thu, Jul 14, 2022 at 04:05:41PM +0300, Dmitry Baryshkov wrote:
>> On 14/07/2022 15:19, Stanimir Varbanov wrote:
>>> Please take a look why we made it built-in first [1].
>>>
>>> If arguments there are still valid I don't see why to make it a module
>>> again.
>>>
>>> [1] https://lkml.org/lkml/2016/8/24/694
>>
>> It looks like there is a move to make all non-essential drivers buildable as
>> modules. For example, the Kirin, dra7xx, Meson PCI controllers are now
>> buildable as modules. So I think we can follow that and allow building the
>> pcie-qcom as a module.
> 
> IIUC the arguments in [1] are that:
> 
>   - Kconfig is bool, so it can't be built as a module
>   - there's no sensible use case for unbind
> 
> Those described the situation at the time, and there's no point in
> having .remove() and using module_platform_driver() if Kconfig is
> bool.
> 
> But they don't seem like arguments for why the driver couldn't be
> *made* modular.

I guess the core of the problem was lack of dw_pcie_host_deinit() at
that time.
> 
> I think drivers *should* be modular unless there's a technical reason
> they can't be.

I agree.

> 
> Bjorn

-- 
regards,
Stan

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] PCI: qcom: Add support for modular builds
  2022-05-19  9:46 [PATCH] PCI: qcom: Add support for modular builds Johan Hovold
                   ` (2 preceding siblings ...)
  2022-07-14 12:19 ` Stanimir Varbanov
@ 2022-07-20 16:27 ` Manivannan Sadhasivam
  2022-07-20 21:20 ` Bjorn Helgaas
  4 siblings, 0 replies; 13+ messages in thread
From: Manivannan Sadhasivam @ 2022-07-20 16:27 UTC (permalink / raw)
  To: Johan Hovold
  Cc: Lorenzo Pieralisi, Rob Herring, Krzysztof Wilczyński,
	Bjorn Helgaas, Stanimir Varbanov, Andy Gross, Bjorn Andersson,
	linux-pci, linux-arm-msm, linux-kernel

On Thu, May 19, 2022 at 11:46:46AM +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.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thanks,
Mani

> ---
>  drivers/pci/controller/dwc/Kconfig     |  2 +-
>  drivers/pci/controller/dwc/pcie-qcom.c | 36 +++++++++++++++++++++++---
>  2 files changed, 34 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 8523b5ef9d16..e25d5c09657c 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -16,7 +16,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>
> @@ -1425,6 +1425,15 @@ static int qcom_pcie_host_init(struct pcie_port *pp)
>  	return ret;
>  }
>  
> +static void qcom_pcie_host_deinit(struct qcom_pcie *pcie)
> +{
> +	qcom_ep_reset_assert(pcie);
> +	if (pcie->cfg->ops->post_deinit)
> +		pcie->cfg->ops->post_deinit(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,
>  };
> @@ -1651,6 +1660,22 @@ 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);
> +	qcom_pcie_host_deinit(pcie);
> +
> +	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-apq8084", .data = &apq8084_cfg },
>  	{ .compatible = "qcom,pcie-ipq8064", .data = &ipq8064_cfg },
> @@ -1669,6 +1694,7 @@ static const struct of_device_id qcom_pcie_match[] = {
>  	{ .compatible = "qcom,pcie-sc7280", .data = &sc7280_cfg },
>  	{ }
>  };
> +MODULE_DEVICE_TABLE(of, qcom_pcie_match);
>  
>  static void qcom_fixup_class(struct pci_dev *dev)
>  {
> @@ -1684,10 +1710,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.35.1
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] PCI: qcom: Add support for modular builds
  2022-05-19  9:46 [PATCH] PCI: qcom: Add support for modular builds Johan Hovold
                   ` (3 preceding siblings ...)
  2022-07-20 16:27 ` Manivannan Sadhasivam
@ 2022-07-20 21:20 ` Bjorn Helgaas
  2022-07-21  6:51   ` Johan Hovold
  4 siblings, 1 reply; 13+ messages in thread
From: Bjorn Helgaas @ 2022-07-20 21:20 UTC (permalink / raw)
  To: Johan Hovold
  Cc: Lorenzo Pieralisi, Rob Herring, Krzysztof Wilczyński,
	Bjorn Helgaas, Stanimir Varbanov, Andy Gross, Bjorn Andersson,
	linux-pci, linux-arm-msm, linux-kernel

On Thu, May 19, 2022 at 11:46:46AM +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.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

This no longer applies on my pci/ctrl/qcom branch:
839fbdee4c08 ("dt-bindings: PCI: qcom: Fix reset conditional")

Do you want to refresh it, or should we wait until the next cycle?

Bjorn

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] PCI: qcom: Add support for modular builds
  2022-07-20 21:20 ` Bjorn Helgaas
@ 2022-07-21  6:51   ` Johan Hovold
  0 siblings, 0 replies; 13+ messages in thread
From: Johan Hovold @ 2022-07-21  6:51 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Johan Hovold, Lorenzo Pieralisi, Rob Herring,
	Krzysztof Wilczyński, Bjorn Helgaas, Stanimir Varbanov,
	Andy Gross, Bjorn Andersson, linux-pci, linux-arm-msm,
	linux-kernel

On Wed, Jul 20, 2022 at 04:20:53PM -0500, Bjorn Helgaas wrote:
> On Thu, May 19, 2022 at 11:46:46AM +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.
> > 
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> 
> This no longer applies on my pci/ctrl/qcom branch:
> 839fbdee4c08 ("dt-bindings: PCI: qcom: Fix reset conditional")
> 
> Do you want to refresh it, or should we wait until the next cycle?

I just sent a rebased v2 (the context had changed due to the addition of
the ipq6018 device-id entry):

	https://lore.kernel.org/all/20220721064720.10762-1-johan+linaro@kernel.org/

Johan

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2022-07-21  6:51 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-19  9:46 [PATCH] PCI: qcom: Add support for modular builds Johan Hovold
2022-05-26 20:53 ` Rob Herring
2022-06-23 11:40   ` Johan Hovold
2022-06-23 15:52 ` Bjorn Helgaas
2022-06-27  7:31   ` Johan Hovold
2022-07-14 12:19 ` Stanimir Varbanov
2022-07-14 13:05   ` Dmitry Baryshkov
2022-07-15 16:56     ` Bjorn Helgaas
2022-07-18  7:43       ` Stanimir Varbanov
2022-07-14 13:10   ` Johan Hovold
2022-07-20 16:27 ` Manivannan Sadhasivam
2022-07-20 21:20 ` Bjorn Helgaas
2022-07-21  6:51   ` Johan Hovold

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).