| Message ID | 20200625001039.56174-4-john.stultz@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Allow for qcom-pdc, pinctrl-msm and qcom-scm drivers to be loadable as modules | expand |
Quoting John Stultz (2020-06-24 17:10:37) > diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c > index 6ae9e1f0819d..3fee8b655da1 100644 > --- a/drivers/irqchip/qcom-pdc.c > +++ b/drivers/irqchip/qcom-pdc.c > @@ -430,4 +432,33 @@ static int qcom_pdc_init(struct device_node *node, struct device_node *parent) > return ret; > } > > +#ifdef MODULE > +static int qcom_pdc_probe(struct platform_device *pdev) > +{ > + struct device_node *np = pdev->dev.of_node; > + struct device_node *parent = of_irq_find_parent(np); > + > + return qcom_pdc_init(np, parent); > +} > + > +static const struct of_device_id qcom_pdc_match_table[] = { > + { .compatible = "qcom,pdc" }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, qcom_pdc_match_table); > + > +static struct platform_driver qcom_pdc_driver = { > + .probe = qcom_pdc_probe, > + .driver = { > + .name = "qcom-pdc", > + .of_match_table = qcom_pdc_match_table, > + .suppress_bind_attrs = true, > + }, > +}; > +module_platform_driver(qcom_pdc_driver); > +#else > IRQCHIP_DECLARE(qcom_pdc, "qcom,pdc", qcom_pdc_init); Is there any reason to use IRQCHIP_DECLARE if this can work as a platform device driver? > +#endif > + > +MODULE_DESCRIPTION("Qualcomm Technologies, Inc. Power Domain Controller"); > +MODULE_LICENSE("GPL v2");
On Fri, Jun 26, 2020 at 12:42 AM Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting John Stultz (2020-06-24 17:10:37) > > diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c > > index 6ae9e1f0819d..3fee8b655da1 100644 > > --- a/drivers/irqchip/qcom-pdc.c > > +++ b/drivers/irqchip/qcom-pdc.c > > @@ -430,4 +432,33 @@ static int qcom_pdc_init(struct device_node *node, struct device_node *parent) > > return ret; > > } > > > > +#ifdef MODULE > > +static int qcom_pdc_probe(struct platform_device *pdev) > > +{ > > + struct device_node *np = pdev->dev.of_node; > > + struct device_node *parent = of_irq_find_parent(np); > > + > > + return qcom_pdc_init(np, parent); > > +} > > + > > +static const struct of_device_id qcom_pdc_match_table[] = { > > + { .compatible = "qcom,pdc" }, > > + {} > > +}; > > +MODULE_DEVICE_TABLE(of, qcom_pdc_match_table); > > + > > +static struct platform_driver qcom_pdc_driver = { > > + .probe = qcom_pdc_probe, > > + .driver = { > > + .name = "qcom-pdc", > > + .of_match_table = qcom_pdc_match_table, > > + .suppress_bind_attrs = true, > > + }, > > +}; > > +module_platform_driver(qcom_pdc_driver); > > +#else > > IRQCHIP_DECLARE(qcom_pdc, "qcom,pdc", qcom_pdc_init); > > Is there any reason to use IRQCHIP_DECLARE if this can work as a > platform device driver? > Hey! Thanks so much for the review! Mostly it was done this way to minimize the change in the non-module case. But if you'd rather avoid the #ifdefery I'll respin it without. thanks -john
On Sat, 27 Jun 2020 02:34:25 +0100, John Stultz <john.stultz@linaro.org> wrote: > > On Fri, Jun 26, 2020 at 12:42 AM Stephen Boyd <swboyd@chromium.org> wrote: > > > > Quoting John Stultz (2020-06-24 17:10:37) > > > diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c > > > index 6ae9e1f0819d..3fee8b655da1 100644 > > > --- a/drivers/irqchip/qcom-pdc.c > > > +++ b/drivers/irqchip/qcom-pdc.c > > > @@ -430,4 +432,33 @@ static int qcom_pdc_init(struct device_node *node, struct device_node *parent) > > > return ret; > > > } > > > > > > +#ifdef MODULE > > > +static int qcom_pdc_probe(struct platform_device *pdev) > > > +{ > > > + struct device_node *np = pdev->dev.of_node; > > > + struct device_node *parent = of_irq_find_parent(np); > > > + > > > + return qcom_pdc_init(np, parent); > > > +} > > > + > > > +static const struct of_device_id qcom_pdc_match_table[] = { > > > + { .compatible = "qcom,pdc" }, > > > + {} > > > +}; > > > +MODULE_DEVICE_TABLE(of, qcom_pdc_match_table); > > > + > > > +static struct platform_driver qcom_pdc_driver = { > > > + .probe = qcom_pdc_probe, > > > + .driver = { > > > + .name = "qcom-pdc", > > > + .of_match_table = qcom_pdc_match_table, > > > + .suppress_bind_attrs = true, > > > + }, > > > +}; > > > +module_platform_driver(qcom_pdc_driver); > > > +#else > > > IRQCHIP_DECLARE(qcom_pdc, "qcom,pdc", qcom_pdc_init); > > > > Is there any reason to use IRQCHIP_DECLARE if this can work as a > > platform device driver? > > > > Hey! Thanks so much for the review! > > Mostly it was done this way to minimize the change in the non-module > case. But if you'd rather avoid the #ifdefery I'll respin it without. That would certainly be my own preference. In general, IRQCHIP_DECLARE and platform drivers should be mutually exclusive in the same driver: if you can delay the probing and have it as a proper platform device, then this should be the one true way. Thanks, M.
Quoting Marc Zyngier (2020-06-27 02:37:47) > On Sat, 27 Jun 2020 02:34:25 +0100, > John Stultz <john.stultz@linaro.org> wrote: > > > > On Fri, Jun 26, 2020 at 12:42 AM Stephen Boyd <swboyd@chromium.org> wrote: > > > > > > > > > Is there any reason to use IRQCHIP_DECLARE if this can work as a > > > platform device driver? > > > > > > > Hey! Thanks so much for the review! > > > > Mostly it was done this way to minimize the change in the non-module > > case. But if you'd rather avoid the #ifdefery I'll respin it without. > > That would certainly be my own preference. In general, IRQCHIP_DECLARE > and platform drivers should be mutually exclusive in the same driver: > if you can delay the probing and have it as a proper platform device, > then this should be the one true way. > Does it work? I haven't looked in detail but I worry that the child irqdomain (i.e. pinctrl-msm) would need to delay probing until this parent irqdomain is registered. Or has the hierarchical irqdomain code been updated to handle the parent child relationship and wait for things to probe or be loaded?
On Thu, Jul 9, 2020 at 11:02 PM Stephen Boyd <swboyd@chromium.org> wrote: > Quoting Marc Zyngier (2020-06-27 02:37:47) > > On Sat, 27 Jun 2020 02:34:25 +0100, > > John Stultz <john.stultz@linaro.org> wrote: > > > > > > On Fri, Jun 26, 2020 at 12:42 AM Stephen Boyd <swboyd@chromium.org> wrote: > > > > > > > > > > > > Is there any reason to use IRQCHIP_DECLARE if this can work as a > > > > platform device driver? > > > > > > > > > > Hey! Thanks so much for the review! > > > > > > Mostly it was done this way to minimize the change in the non-module > > > case. But if you'd rather avoid the #ifdefery I'll respin it without. > > > > That would certainly be my own preference. In general, IRQCHIP_DECLARE > > and platform drivers should be mutually exclusive in the same driver: > > if you can delay the probing and have it as a proper platform device, > > then this should be the one true way. > > > > Does it work? I haven't looked in detail but I worry that the child > irqdomain (i.e. pinctrl-msm) would need to delay probing until this > parent irqdomain is registered. Or has the hierarchical irqdomain code > been updated to handle the parent child relationship and wait for things > to probe or be loaded? So I can't say I know the underlying hardware particularly well, but I've been using this successfully on the Dragonboard 845c with both static builds as well as module enabled builds. And the same patch has been in the android-mainline and android-5.4 kernels for a while without objections from QCOM. As to the probe ordering question, Saravana can maybe speak in more detail if it's involved in this case but the fw_devlink code has addressed many of these sorts of ordering issues. However, I'm not sure if I'm lucking into the right probe order, as we have been able to boot android-mainline w/ both fw_devlink=on and fw_devlink=off (though in the =off case, we need deferred_probe_timeout=30 to give us a bit more time for modules to load after init starts). thanks -john
Quoting John Stultz (2020-07-10 15:44:18) > On Thu, Jul 9, 2020 at 11:02 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > Does it work? I haven't looked in detail but I worry that the child > > irqdomain (i.e. pinctrl-msm) would need to delay probing until this > > parent irqdomain is registered. Or has the hierarchical irqdomain code > > been updated to handle the parent child relationship and wait for things > > to probe or be loaded? > > So I can't say I know the underlying hardware particularly well, but > I've been using this successfully on the Dragonboard 845c with both > static builds as well as module enabled builds. > And the same patch has been in the android-mainline and android-5.4 > kernels for a while without objections from QCOM. > > As to the probe ordering question, Saravana can maybe speak in more > detail if it's involved in this case but the fw_devlink code has > addressed many of these sorts of ordering issues. > However, I'm not sure if I'm lucking into the right probe order, as we > have been able to boot android-mainline w/ both fw_devlink=on and > fw_devlink=off (though in the =off case, we need > deferred_probe_timeout=30 to give us a bit more time for modules to > load after init starts). > Ok I looked at the code (sorry for not checking earlier) and I see this in msm_gpio_init() np = of_parse_phandle(pctrl->dev->of_node, "wakeup-parent", 0); if (np) { chip->irq.parent_domain = irq_find_matching_host(np, DOMAIN_BUS_WAKEUP); of_node_put(np); if (!chip->irq.parent_domain) return -EPROBE_DEFER; so it looks like we'll probe defer the pinctrl driver until the pdc module loads. Meaning it should work to have pinctrl builtin and pdc as a module.
On Sat, 11 Jul 2020 00:27:45 +0100, Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting John Stultz (2020-07-10 15:44:18) > > On Thu, Jul 9, 2020 at 11:02 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > > > Does it work? I haven't looked in detail but I worry that the child > > > irqdomain (i.e. pinctrl-msm) would need to delay probing until this > > > parent irqdomain is registered. Or has the hierarchical irqdomain code > > > been updated to handle the parent child relationship and wait for things > > > to probe or be loaded? > > > > So I can't say I know the underlying hardware particularly well, but > > I've been using this successfully on the Dragonboard 845c with both > > static builds as well as module enabled builds. > > And the same patch has been in the android-mainline and android-5.4 > > kernels for a while without objections from QCOM. > > > > As to the probe ordering question, Saravana can maybe speak in more > > detail if it's involved in this case but the fw_devlink code has > > addressed many of these sorts of ordering issues. > > However, I'm not sure if I'm lucking into the right probe order, as we > > have been able to boot android-mainline w/ both fw_devlink=on and > > fw_devlink=off (though in the =off case, we need > > deferred_probe_timeout=30 to give us a bit more time for modules to > > load after init starts). > > > > Ok I looked at the code (sorry for not checking earlier) and I see this in > msm_gpio_init() > > np = of_parse_phandle(pctrl->dev->of_node, "wakeup-parent", 0); > if (np) { > chip->irq.parent_domain = irq_find_matching_host(np, > DOMAIN_BUS_WAKEUP); > of_node_put(np); > if (!chip->irq.parent_domain) > return -EPROBE_DEFER; > > so it looks like we'll probe defer the pinctrl driver until the pdc module > loads. Meaning it should work to have pinctrl builtin and pdc as a module. What I hope is that eventually fw_devlink will become the norm (on by default), and that probe deferral will become a thing of the past. M.
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig index 29fead208cad..12765bed08f9 100644 --- a/drivers/irqchip/Kconfig +++ b/drivers/irqchip/Kconfig @@ -425,7 +425,7 @@ config GOLDFISH_PIC for Goldfish based virtual platforms. config QCOM_PDC - bool "QCOM PDC" + tristate "QCOM PDC" depends on ARCH_QCOM select IRQ_DOMAIN_HIERARCHY help diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c index 6ae9e1f0819d..3fee8b655da1 100644 --- a/drivers/irqchip/qcom-pdc.c +++ b/drivers/irqchip/qcom-pdc.c @@ -11,9 +11,11 @@ #include <linux/irqdomain.h> #include <linux/io.h> #include <linux/kernel.h> +#include <linux/module.h> #include <linux/of.h> #include <linux/of_address.h> #include <linux/of_device.h> +#include <linux/of_irq.h> #include <linux/soc/qcom/irq.h> #include <linux/spinlock.h> #include <linux/slab.h> @@ -430,4 +432,33 @@ static int qcom_pdc_init(struct device_node *node, struct device_node *parent) return ret; } +#ifdef MODULE +static int qcom_pdc_probe(struct platform_device *pdev) +{ + struct device_node *np = pdev->dev.of_node; + struct device_node *parent = of_irq_find_parent(np); + + return qcom_pdc_init(np, parent); +} + +static const struct of_device_id qcom_pdc_match_table[] = { + { .compatible = "qcom,pdc" }, + {} +}; +MODULE_DEVICE_TABLE(of, qcom_pdc_match_table); + +static struct platform_driver qcom_pdc_driver = { + .probe = qcom_pdc_probe, + .driver = { + .name = "qcom-pdc", + .of_match_table = qcom_pdc_match_table, + .suppress_bind_attrs = true, + }, +}; +module_platform_driver(qcom_pdc_driver); +#else IRQCHIP_DECLARE(qcom_pdc, "qcom,pdc", qcom_pdc_init); +#endif + +MODULE_DESCRIPTION("Qualcomm Technologies, Inc. Power Domain Controller"); +MODULE_LICENSE("GPL v2");
Allows qcom-pdc driver to be loaded as a permanent module Also, due to the fact that IRQCHIP_DECLARE becomes a no-op when building as a module, we have to add the platform driver hooks explicitly. Thanks to Saravana for his help on pointing out the IRQCHIP_DECLARE issue and guidance on a solution. Cc: Andy Gross <agross@kernel.org> Cc: Bjorn Andersson <bjorn.andersson@linaro.org> Cc: Joerg Roedel <joro@8bytes.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Jason Cooper <jason@lakedaemon.net> Cc: Marc Zyngier <maz@kernel.org> Cc: Linus Walleij <linus.walleij@linaro.org> Cc: Maulik Shah <mkshah@codeaurora.org> Cc: Lina Iyer <ilina@codeaurora.org> Cc: Saravana Kannan <saravanak@google.com> Cc: Todd Kjos <tkjos@google.com> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: linux-arm-msm@vger.kernel.org Cc: iommu@lists.linux-foundation.org Cc: linux-gpio@vger.kernel.org Signed-off-by: John Stultz <john.stultz@linaro.org> --- v2: Fix spelling, include order and set suppress_bind_attrs suggested by Maulik Shah --- drivers/irqchip/Kconfig | 2 +- drivers/irqchip/qcom-pdc.c | 31 +++++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-)