PCI: qcom: Add support for modular builds
Subsystems (4)
PATCH 1/1 PCI: qcom: Add support for modular builds
Allow the Qualcomm PCIe controller driver to be built as a module, which is useful for multi-platform kernels as well as during development.
--- drivers/pci/controller/dwc/Kconfig | 2 +- drivers/pci/controller/dwc/pcie-qcom.c | 36 +++++++++++++++++++++++--- 2 files changed, 34 insertions(+), 4 deletions(-)
drivers/pci/controller/dwc/Kconfig +1 -1
@@ -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
drivers/pci/controller/dwc/pcie-qcom.c +33 -3
@@ -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 <[email protected]>" ); +MODULE_DESCRIPTION ("Qualcomm PCIe root complex driver" ); +MODULE_LICENSE ("GPL" );
Signature · 1 line
2.35.1
Quoted 8 lines · 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 <[email protected]>--- drivers/pci/controller/dwc/Kconfig | 2 +- drivers/pci/controller/dwc/pcie-qcom.c | 36 +++++++++++++++++++++++--- 2 files changed, 34 insertions(+), 4 deletions(-)
Quoted 11 lines · Rob Herring
Quoted 8 lines · 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 <[email protected]>--- 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 <[email protected]>
Lorenzo, Bjorn H, any comments to this one?
Johan
Quoted 36 lines · 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 <[email protected]>--- drivers/pci/controller/dwc/Kconfig | 2 +- drivers/pci/controller/dwc/pcie-qcom.c | 36 +++++++++++++++++++++++--- 2 files changed, 34 insertions(+), 4 deletions(-)drivers/pci/controller/dwc/Kconfig +1
-1@@ -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_HOSTdrivers/pci/controller/dwc/pcie-qcom.c +1
-1@@ -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>
drivers/pci/controller/dwc/pcie-qcom.c static int qcom_pcie_host_init(struct pcie_port *pp) · line 1434
Quoted 10 lines
drivers/pci/controller/dwc/pcie-qcom.c +7
-0@@ -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()".
Quoted 9 lines
drivers/pci/controller/dwc/pcie-qcom.c +2
-0static 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) +{
drivers/pci/controller/dwc/pcie-qcom.c static int qcom_pcie_probe(struct platform_device *pdev) · line 1674
Quoted 10 lines
drivers/pci/controller/dwc/pcie-qcom.c +10
-0+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_disableQuoted 34 lines
drivers/pci/controller/dwc/pcie-qcom.c +10
-2+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 <[email protected]>" ); +MODULE_DESCRIPTION ("Qualcomm PCIe root complex driver" ); +MODULE_LICENSE ("GPL" );Signature · 1 line
2.35.1
Quoted 7 lines · Bjorn Helgaas
Quoted 5 lines · 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 <[email protected]>---
Quoted 11 lines
drivers/pci/controller/dwc/pcie-qcom.c static int qcom_pcie_host_init(struct pcie_port *pp) · line 1434
Quoted 7 lines
drivers/pci/controller/dwc/pcie-qcom.c +7
-0+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.
Quoted 7 lines
Quoted 7 lines
static const struct
dw_pcie_host_opsqcom_pcie_dw_ops= {drivers/pci/controller/dwc/pcie-qcom.c +0
-0.host_init = qcom_pcie_host_init, };@@ -1651,6 +1660,22 @@ static int qcom_pcie_probe(struct platform_device *pdev)return ret; }
Quoted 16 lines
drivers/pci/controller/dwc/pcie-qcom.c static int qcom_pcie_probe(struct platform_device *pdev) · line 1674
Quoted 12 lines
drivers/pci/controller/dwc/pcie-qcom.c +12
-0+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 structdw_pcie_host_opsneeds a new .host_deinit() pointer that would be called fromdw_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/[email protected]
Also note that there are other drivers that implement remove() without this callback already today.
Quoted 18 lines
In the probe path, we have this:
qcom_pcie_probepm_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
Quoted 9 lines
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_disableYeah, 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
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
Quoted 9 lines · 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 <[email protected]>--- drivers/pci/controller/dwc/Kconfig | 2 +- drivers/pci/controller/dwc/pcie-qcom.c | 36 +++++++++++++++++++++++--- 2 files changed, 34 insertions(+), 4 deletions(-)
Signature · 2 lines
regards, Stan
Hi Stanimir,
Quoted 8 lines · Stanimir Varbanov
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.
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.
Quoted 13 lines · Johan Hovold
Quoted 9 lines
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 <[email protected]>--- drivers/pci/controller/dwc/Kconfig | 2 +- drivers/pci/controller/dwc/pcie-qcom.c | 36 +++++++++++++++++++++++--- 2 files changed, 34 insertions(+), 4 deletions(-)
Signature · 2 lines
With best wishes Dmitry
Quoted 6 lines · Dmitry Baryshkov
Quoted 5 lines · Stanimir Varbanov
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.
Quoted 6 lines
Quoted 1 line
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
Quoted 19 lines · Bjorn Helgaas
Quoted 6 lines · Dmitry Baryshkov
Quoted 5 lines · Stanimir Varbanov
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.
Quoted 6 lines
Quoted 1 line
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:
-
Kconfigis bool, so it can't be built as a module
- there's no sensible use case for unbind
Quoted 6 lines
Those described the situation at the time, and there's no point in having .remove() and using
module_platform_driver()ifKconfigis 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.
Quoted 3 lines
I think drivers *should* be modular unless there's a technical reason they can't be.
I agree.
Quoted 2 lines
Bjorn
Signature · 2 lines
regards, Stan
Quoted 6 lines · Stanimir Varbanov
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.
Quoted 11 lines
[1] https://lkml.org/lkml/2016/8/24/694
Quoted 8 lines · 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 <[email protected]>--- drivers/pci/controller/dwc/Kconfig | 2 +- drivers/pci/controller/dwc/pcie-qcom.c | 36 +++++++++++++++++++++++--- 2 files changed, 34 insertions(+), 4 deletions(-)
Johan
Quoted 4 lines · 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 <[email protected]>
Thanks,
Mani
Quoted 98 lines
--- drivers/pci/controller/dwc/Kconfig | 2 +- drivers/pci/controller/dwc/pcie-qcom.c | 36 +++++++++++++++++++++++--- 2 files changed, 34 insertions(+), 4 deletions(-)drivers/pci/controller/dwc/Kconfig +1
-1@@ -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_HOSTdrivers/pci/controller/dwc/pcie-qcom.c +33
-3@@ -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 <[email protected]>" ); +MODULE_DESCRIPTION ("Qualcomm PCIe root complex driver" ); +MODULE_LICENSE ("GPL" );Signature · 1 line
2.35.1
Quoted 4 lines · 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 <[email protected]>
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
Quoted 10 lines · Bjorn Helgaas
Quoted 4 lines · 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 <[email protected]>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/[email protected]/
Johan