| Message ID | 20220519094646.23009-1-johan+linaro@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: qcom: Add support for modular builds | expand |
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>
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
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 >
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
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");
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(-)