Linux-GPIO Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2 0/5] Allow for qcom-pdc, pinctrl-msm and qcom-scm drivers to be loadable as modules
@ 2020-06-25  0:10 John Stultz
  2020-06-25  0:10 ` [PATCH v2 1/5] irq: irqdomain: Export irq_domain_update_bus_token John Stultz
                   ` (4 more replies)
  0 siblings, 5 replies; 21+ messages in thread
From: John Stultz @ 2020-06-25  0:10 UTC (permalink / raw)
  To: lkml
  Cc: John Stultz, Andy Gross, Bjorn Andersson, Joerg Roedel,
	Thomas Gleixner, Jason Cooper, Marc Zyngier, Linus Walleij,
	Lina Iyer, Maulik Shah, Saravana Kannan, Todd Kjos,
	Greg Kroah-Hartman, linux-arm-msm, iommu, linux-gpio

This patch series provides exports and config tweaks to allow
the qcom-pdc, pinctrl-msm and qcom-scm drivers to be able to be
configured as permement modules (particularlly useful for the
Android Generic Kernel Image efforts).

Feedback would be appreciated!

New in v2:
* Fix up MSM_PINCTRL Kconfig dependency logic so we match
  QCOM_SCM.
* Minor tweaks and corrections suggested by Bjorn and Maulik


thanks
-john

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: Lina Iyer <ilina@codeaurora.org>
Cc: Maulik Shah <mkshah@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


John Stultz (5):
  irq: irqdomain: Export irq_domain_update_bus_token
  irq: irqchip: Export irq_chip_retrigger_hierarchy and
    irq_chip_set_vcpu_affinity_parent
  irqchip: Allow QCOM_PDC to be loadable as a permanent module
  pinctrl: qcom: Allow pinctrl-msm code to be loadable as a module
  firmware: QCOM_SCM: Allow qcom_scm driver to be loadable as a
    permenent module

 drivers/firmware/Kconfig           |  2 +-
 drivers/firmware/Makefile          |  3 ++-
 drivers/firmware/qcom_scm.c        |  4 ++++
 drivers/iommu/Kconfig              |  2 ++
 drivers/irqchip/Kconfig            |  2 +-
 drivers/irqchip/qcom-pdc.c         | 31 ++++++++++++++++++++++++++++++
 drivers/pinctrl/qcom/Kconfig       | 24 ++++++++++++++++++++++-
 drivers/pinctrl/qcom/pinctrl-msm.c |  2 ++
 kernel/irq/chip.c                  |  3 ++-
 kernel/irq/irqdomain.c             |  1 +
 10 files changed, 69 insertions(+), 5 deletions(-)

-- 
2.17.1


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

* [PATCH v2 1/5] irq: irqdomain: Export irq_domain_update_bus_token
  2020-06-25  0:10 [PATCH v2 0/5] Allow for qcom-pdc, pinctrl-msm and qcom-scm drivers to be loadable as modules John Stultz
@ 2020-06-25  0:10 ` John Stultz
  2020-06-25  0:10 ` [PATCH v2 2/5] irq: irqchip: Export irq_chip_retrigger_hierarchy and irq_chip_set_vcpu_affinity_parent John Stultz
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 21+ messages in thread
From: John Stultz @ 2020-06-25  0:10 UTC (permalink / raw)
  To: lkml
  Cc: John Stultz, Andy Gross, Bjorn Andersson, Joerg Roedel,
	Thomas Gleixner, Jason Cooper, Marc Zyngier, Linus Walleij,
	Maulik Shah, Lina Iyer, Saravana Kannan, Todd Kjos,
	Greg Kroah-Hartman, linux-arm-msm, iommu, linux-gpio

Add export for irq_domain_update_bus_token() so that
we can allow drivers like the qcom-pdc driver to be
loadable as a module.

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>
---
 kernel/irq/irqdomain.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index a4c2c915511d..ca974d965fda 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -281,6 +281,7 @@ void irq_domain_update_bus_token(struct irq_domain *domain,
 
 	mutex_unlock(&irq_domain_mutex);
 }
+EXPORT_SYMBOL_GPL(irq_domain_update_bus_token);
 
 /**
  * irq_domain_add_simple() - Register an irq_domain and optionally map a range of irqs
-- 
2.17.1


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

* [PATCH v2 2/5] irq: irqchip: Export irq_chip_retrigger_hierarchy and irq_chip_set_vcpu_affinity_parent
  2020-06-25  0:10 [PATCH v2 0/5] Allow for qcom-pdc, pinctrl-msm and qcom-scm drivers to be loadable as modules John Stultz
  2020-06-25  0:10 ` [PATCH v2 1/5] irq: irqdomain: Export irq_domain_update_bus_token John Stultz
@ 2020-06-25  0:10 ` John Stultz
  2020-06-25  0:10 ` [PATCH v2 3/5] irqchip: Allow QCOM_PDC to be loadable as a permanent module John Stultz
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 21+ messages in thread
From: John Stultz @ 2020-06-25  0:10 UTC (permalink / raw)
  To: lkml
  Cc: John Stultz, Andy Gross, Bjorn Andersson, Joerg Roedel,
	Thomas Gleixner, Jason Cooper, Marc Zyngier, Linus Walleij,
	Maulik Shah, Lina Iyer, Saravana Kannan, Todd Kjos,
	Greg Kroah-Hartman, linux-arm-msm, iommu, linux-gpio

Add EXPORT_SYMBOL_GPL entries for irq_chip_retrigger_hierarchy()
and irq_chip_set_vcpu_affinity_parent() so that we can allow
drivers like the qcom-pdc driver to be loadable as a module.

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>
---
 kernel/irq/chip.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 41e7e37a0928..ba6ce66d7ed6 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -1478,6 +1478,7 @@ int irq_chip_retrigger_hierarchy(struct irq_data *data)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(irq_chip_retrigger_hierarchy);
 
 /**
  * irq_chip_set_vcpu_affinity_parent - Set vcpu affinity on the parent interrupt
@@ -1492,7 +1493,7 @@ int irq_chip_set_vcpu_affinity_parent(struct irq_data *data, void *vcpu_info)
 
 	return -ENOSYS;
 }
-
+EXPORT_SYMBOL_GPL(irq_chip_set_vcpu_affinity_parent);
 /**
  * irq_chip_set_wake_parent - Set/reset wake-up on the parent interrupt
  * @data:	Pointer to interrupt specific data
-- 
2.17.1


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

* [PATCH v2 3/5] irqchip: Allow QCOM_PDC to be loadable as a permanent module
  2020-06-25  0:10 [PATCH v2 0/5] Allow for qcom-pdc, pinctrl-msm and qcom-scm drivers to be loadable as modules John Stultz
  2020-06-25  0:10 ` [PATCH v2 1/5] irq: irqdomain: Export irq_domain_update_bus_token John Stultz
  2020-06-25  0:10 ` [PATCH v2 2/5] irq: irqchip: Export irq_chip_retrigger_hierarchy and irq_chip_set_vcpu_affinity_parent John Stultz
@ 2020-06-25  0:10 ` John Stultz
  2020-06-26  7:42   ` Stephen Boyd
  2020-06-25  0:10 ` [PATCH v2 4/5] pinctrl: qcom: Allow pinctrl-msm code to be loadable as a module John Stultz
  2020-06-25  0:10 ` [PATCH v2 5/5] firmware: QCOM_SCM: Allow qcom_scm driver to be loadable as a permenent module John Stultz
  4 siblings, 1 reply; 21+ messages in thread
From: John Stultz @ 2020-06-25  0:10 UTC (permalink / raw)
  To: lkml
  Cc: John Stultz, Andy Gross, Bjorn Andersson, Joerg Roedel,
	Thomas Gleixner, Jason Cooper, Marc Zyngier, Linus Walleij,
	Maulik Shah, Lina Iyer, Saravana Kannan, Todd Kjos,
	Greg Kroah-Hartman, linux-arm-msm, iommu, linux-gpio

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(-)

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");
-- 
2.17.1


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

* [PATCH v2 4/5] pinctrl: qcom: Allow pinctrl-msm code to be loadable as a module
  2020-06-25  0:10 [PATCH v2 0/5] Allow for qcom-pdc, pinctrl-msm and qcom-scm drivers to be loadable as modules John Stultz
                   ` (2 preceding siblings ...)
  2020-06-25  0:10 ` [PATCH v2 3/5] irqchip: Allow QCOM_PDC to be loadable as a permanent module John Stultz
@ 2020-06-25  0:10 ` John Stultz
  2020-06-25  0:10 ` [PATCH v2 5/5] firmware: QCOM_SCM: Allow qcom_scm driver to be loadable as a permenent module John Stultz
  4 siblings, 0 replies; 21+ messages in thread
From: John Stultz @ 2020-06-25  0:10 UTC (permalink / raw)
  To: lkml
  Cc: John Stultz, Andy Gross, Bjorn Andersson, Joerg Roedel,
	Thomas Gleixner, Jason Cooper, Marc Zyngier, Linus Walleij,
	Maulik Shah, Lina Iyer, Saravana Kannan, Todd Kjos,
	Greg Kroah-Hartman, linux-arm-msm, iommu, linux-gpio

Tweaks to allow pinctrl-msm code to be loadable as a module.
This is needed in order to support having the qcom-scm driver,
which pinctrl-msm calls into, configured as a module.

This requires that we tweak Kconfigs selecting PINCTRL_MSM to
also depend on QCOM_SCM || QCOM_SCM=n so that we match the
module setting of QCOM_SCM.

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:
* Module description and whitespace fixes suggested by Bjorn
* Added QCOM_SCM || QCOM_SCM=n bits on Kconfigs selecting
  PINCTRL_MSM. Reported by both Todd and Bjorn.
---
 drivers/pinctrl/qcom/Kconfig       | 24 +++++++++++++++++++++++-
 drivers/pinctrl/qcom/pinctrl-msm.c |  2 ++
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/qcom/Kconfig b/drivers/pinctrl/qcom/Kconfig
index ff1ee159dca2..11228ae3d826 100644
--- a/drivers/pinctrl/qcom/Kconfig
+++ b/drivers/pinctrl/qcom/Kconfig
@@ -2,7 +2,7 @@
 if (ARCH_QCOM || COMPILE_TEST)
 
 config PINCTRL_MSM
-	bool
+	tristate
 	select PINMUX
 	select PINCONF
 	select GENERIC_PINCONF
@@ -11,6 +11,7 @@ config PINCTRL_MSM
 config PINCTRL_APQ8064
 	tristate "Qualcomm APQ8064 pin controller driver"
 	depends on GPIOLIB && OF
+	depends on QCOM_SCM || !QCOM_SCM
 	select PINCTRL_MSM
 	help
 	  This is the pinctrl, pinmux, pinconf and gpiolib driver for the
@@ -19,6 +20,7 @@ config PINCTRL_APQ8064
 config PINCTRL_APQ8084
 	tristate "Qualcomm APQ8084 pin controller driver"
 	depends on GPIOLIB && OF
+	depends on QCOM_SCM || !QCOM_SCM
 	select PINCTRL_MSM
 	help
 	  This is the pinctrl, pinmux, pinconf and gpiolib driver for the
@@ -27,6 +29,7 @@ config PINCTRL_APQ8084
 config PINCTRL_IPQ4019
 	tristate "Qualcomm IPQ4019 pin controller driver"
 	depends on GPIOLIB && OF
+	depends on QCOM_SCM || !QCOM_SCM
 	select PINCTRL_MSM
 	help
 	  This is the pinctrl, pinmux, pinconf and gpiolib driver for the
@@ -35,6 +38,7 @@ config PINCTRL_IPQ4019
 config PINCTRL_IPQ8064
 	tristate "Qualcomm IPQ8064 pin controller driver"
 	depends on GPIOLIB && OF
+	depends on QCOM_SCM || !QCOM_SCM
 	select PINCTRL_MSM
 	help
 	  This is the pinctrl, pinmux, pinconf and gpiolib driver for the
@@ -43,6 +47,7 @@ config PINCTRL_IPQ8064
 config PINCTRL_IPQ8074
 	tristate "Qualcomm Technologies, Inc. IPQ8074 pin controller driver"
 	depends on GPIOLIB && OF
+	depends on QCOM_SCM || !QCOM_SCM
 	select PINCTRL_MSM
 	help
 	  This is the pinctrl, pinmux, pinconf and gpiolib driver for
@@ -53,6 +58,7 @@ config PINCTRL_IPQ8074
 config PINCTRL_IPQ6018
 	tristate "Qualcomm Technologies, Inc. IPQ6018 pin controller driver"
 	depends on GPIOLIB && OF
+	depends on QCOM_SCM || !QCOM_SCM
 	select PINCTRL_MSM
 	help
 	  This is the pinctrl, pinmux, pinconf and gpiolib driver for
@@ -63,6 +69,7 @@ config PINCTRL_IPQ6018
 config PINCTRL_MSM8660
 	tristate "Qualcomm 8660 pin controller driver"
 	depends on GPIOLIB && OF
+	depends on QCOM_SCM || !QCOM_SCM
 	select PINCTRL_MSM
 	help
 	  This is the pinctrl, pinmux, pinconf and gpiolib driver for the
@@ -71,6 +78,7 @@ config PINCTRL_MSM8660
 config PINCTRL_MSM8960
 	tristate "Qualcomm 8960 pin controller driver"
 	depends on GPIOLIB && OF
+	depends on QCOM_SCM || !QCOM_SCM
 	select PINCTRL_MSM
 	help
 	  This is the pinctrl, pinmux, pinconf and gpiolib driver for the
@@ -79,6 +87,7 @@ config PINCTRL_MSM8960
 config PINCTRL_MDM9615
 	tristate "Qualcomm 9615 pin controller driver"
 	depends on GPIOLIB && OF
+	depends on QCOM_SCM || !QCOM_SCM
 	select PINCTRL_MSM
 	help
 	  This is the pinctrl, pinmux, pinconf and gpiolib driver for the
@@ -87,6 +96,7 @@ config PINCTRL_MDM9615
 config PINCTRL_MSM8X74
 	tristate "Qualcomm 8x74 pin controller driver"
 	depends on GPIOLIB && OF
+	depends on QCOM_SCM || !QCOM_SCM
 	select PINCTRL_MSM
 	help
 	  This is the pinctrl, pinmux, pinconf and gpiolib driver for the
@@ -95,6 +105,7 @@ config PINCTRL_MSM8X74
 config PINCTRL_MSM8916
 	tristate "Qualcomm 8916 pin controller driver"
 	depends on GPIOLIB && OF
+	depends on QCOM_SCM || !QCOM_SCM
 	select PINCTRL_MSM
 	help
 	  This is the pinctrl, pinmux, pinconf and gpiolib driver for the
@@ -103,6 +114,7 @@ config PINCTRL_MSM8916
 config PINCTRL_MSM8976
 	tristate "Qualcomm 8976 pin controller driver"
 	depends on GPIOLIB && OF
+	depends on QCOM_SCM || !QCOM_SCM
 	select PINCTRL_MSM
 	help
 	  This is the pinctrl, pinmux, pinconf and gpiolib driver for the
@@ -113,6 +125,7 @@ config PINCTRL_MSM8976
 config PINCTRL_MSM8994
 	tristate "Qualcomm 8994 pin controller driver"
 	depends on GPIOLIB && OF
+	depends on QCOM_SCM || !QCOM_SCM
 	select PINCTRL_MSM
 	help
 	  This is the pinctrl, pinmux, pinconf and gpiolib driver for the
@@ -122,6 +135,7 @@ config PINCTRL_MSM8994
 config PINCTRL_MSM8996
 	tristate "Qualcomm MSM8996 pin controller driver"
 	depends on GPIOLIB && OF
+	depends on QCOM_SCM || !QCOM_SCM
 	select PINCTRL_MSM
 	help
 	  This is the pinctrl, pinmux, pinconf and gpiolib driver for the
@@ -130,6 +144,7 @@ config PINCTRL_MSM8996
 config PINCTRL_MSM8998
 	tristate "Qualcomm MSM8998 pin controller driver"
 	depends on GPIOLIB && OF
+	depends on QCOM_SCM || !QCOM_SCM
 	select PINCTRL_MSM
 	help
 	  This is the pinctrl, pinmux, pinconf and gpiolib driver for the
@@ -138,6 +153,7 @@ config PINCTRL_MSM8998
 config PINCTRL_QCS404
 	tristate "Qualcomm QCS404 pin controller driver"
 	depends on GPIOLIB && OF
+	depends on QCOM_SCM || !QCOM_SCM
 	select PINCTRL_MSM
 	help
 	  This is the pinctrl, pinmux, pinconf and gpiolib driver for the
@@ -146,6 +162,7 @@ config PINCTRL_QCS404
 config PINCTRL_QDF2XXX
 	tristate "Qualcomm Technologies QDF2xxx pin controller driver"
 	depends on GPIOLIB && ACPI
+	depends on QCOM_SCM || !QCOM_SCM
 	select PINCTRL_MSM
 	help
 	  This is the GPIO driver for the TLMM block found on the
@@ -183,6 +200,7 @@ config PINCTRL_QCOM_SSBI_PMIC
 config PINCTRL_SC7180
 	tristate "Qualcomm Technologies Inc SC7180 pin controller driver"
 	depends on GPIOLIB && OF
+	depends on QCOM_SCM || !QCOM_SCM
 	select PINCTRL_MSM
 	help
 	  This is the pinctrl, pinmux, pinconf and gpiolib driver for the
@@ -192,6 +210,7 @@ config PINCTRL_SC7180
 config PINCTRL_SDM660
 	tristate "Qualcomm Technologies Inc SDM660 pin controller driver"
 	depends on GPIOLIB && OF
+	depends on QCOM_SCM || !QCOM_SCM
 	select PINCTRL_MSM
 	help
 	 This is the pinctrl, pinmux, pinconf and gpiolib driver for the
@@ -201,6 +220,7 @@ config PINCTRL_SDM660
 config PINCTRL_SDM845
 	tristate "Qualcomm Technologies Inc SDM845 pin controller driver"
 	depends on GPIOLIB && (OF || ACPI)
+	depends on QCOM_SCM || !QCOM_SCM
 	select PINCTRL_MSM
 	help
 	 This is the pinctrl, pinmux, pinconf and gpiolib driver for the
@@ -210,6 +230,7 @@ config PINCTRL_SDM845
 config PINCTRL_SM8150
 	tristate "Qualcomm Technologies Inc SM8150 pin controller driver"
 	depends on GPIOLIB && OF
+	depends on QCOM_SCM || !QCOM_SCM
 	select PINCTRL_MSM
 	help
 	 This is the pinctrl, pinmux, pinconf and gpiolib driver for the
@@ -219,6 +240,7 @@ config PINCTRL_SM8150
 config PINCTRL_SM8250
 	tristate "Qualcomm Technologies Inc SM8250 pin controller driver"
 	depends on GPIOLIB && OF
+	depends on QCOM_SCM || !QCOM_SCM
 	select PINCTRL_MSM
 	help
 	  This is the pinctrl, pinmux, pinconf and gpiolib driver for the
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 83b7d64bc4c1..e8e3ba8207af 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -1355,3 +1355,5 @@ int msm_pinctrl_remove(struct platform_device *pdev)
 }
 EXPORT_SYMBOL(msm_pinctrl_remove);
 
+MODULE_DESCRIPTION("Qualcomm Technologies, Inc. TLMM driver");
+MODULE_LICENSE("GPL v2");
-- 
2.17.1


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

* [PATCH v2 5/5] firmware: QCOM_SCM: Allow qcom_scm driver to be loadable as a permenent module
  2020-06-25  0:10 [PATCH v2 0/5] Allow for qcom-pdc, pinctrl-msm and qcom-scm drivers to be loadable as modules John Stultz
                   ` (3 preceding siblings ...)
  2020-06-25  0:10 ` [PATCH v2 4/5] pinctrl: qcom: Allow pinctrl-msm code to be loadable as a module John Stultz
@ 2020-06-25  0:10 ` John Stultz
  2020-07-02 12:47   ` Greg Kroah-Hartman
  2020-07-02 14:18   ` Will Deacon
  4 siblings, 2 replies; 21+ messages in thread
From: John Stultz @ 2020-06-25  0:10 UTC (permalink / raw)
  To: lkml
  Cc: John Stultz, Andy Gross, Bjorn Andersson, Joerg Roedel,
	Thomas Gleixner, Jason Cooper, Marc Zyngier, Linus Walleij,
	Maulik Shah, Lina Iyer, Saravana Kannan, Todd Kjos,
	Greg Kroah-Hartman, linux-arm-msm, iommu, linux-gpio

Allow the qcom_scm driver to be loadable as a
permenent module.

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>
---
 drivers/firmware/Kconfig    | 2 +-
 drivers/firmware/Makefile   | 3 ++-
 drivers/firmware/qcom_scm.c | 4 ++++
 drivers/iommu/Kconfig       | 2 ++
 4 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index fbd785dd0513..9e533a462bf4 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -236,7 +236,7 @@ config INTEL_STRATIX10_RSU
 	  Say Y here if you want Intel RSU support.
 
 config QCOM_SCM
-	bool
+	tristate "Qcom SCM driver"
 	depends on ARM || ARM64
 	select RESET_CONTROLLER
 
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index 99510be9f5ed..cf24d674216b 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -17,7 +17,8 @@ obj-$(CONFIG_ISCSI_IBFT)	+= iscsi_ibft.o
 obj-$(CONFIG_FIRMWARE_MEMMAP)	+= memmap.o
 obj-$(CONFIG_RASPBERRYPI_FIRMWARE) += raspberrypi.o
 obj-$(CONFIG_FW_CFG_SYSFS)	+= qemu_fw_cfg.o
-obj-$(CONFIG_QCOM_SCM)		+= qcom_scm.o qcom_scm-smc.o qcom_scm-legacy.o
+obj-$(CONFIG_QCOM_SCM)		+= qcom-scm.o
+qcom-scm-objs += qcom_scm.o qcom_scm-smc.o qcom_scm-legacy.o
 obj-$(CONFIG_TI_SCI_PROTOCOL)	+= ti_sci.o
 obj-$(CONFIG_TRUSTED_FOUNDATIONS) += trusted_foundations.o
 obj-$(CONFIG_TURRIS_MOX_RWTM)	+= turris-mox-rwtm.o
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 0e7233a20f34..b5e88bf66975 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -1155,6 +1155,7 @@ static const struct of_device_id qcom_scm_dt_match[] = {
 	{ .compatible = "qcom,scm" },
 	{}
 };
+MODULE_DEVICE_TABLE(of, qcom_scm_dt_match);
 
 static struct platform_driver qcom_scm_driver = {
 	.driver = {
@@ -1170,3 +1171,6 @@ static int __init qcom_scm_init(void)
 	return platform_driver_register(&qcom_scm_driver);
 }
 subsys_initcall(qcom_scm_init);
+
+MODULE_DESCRIPTION("Qualcomm Technologies, Inc. SCM driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index b510f67dfa49..714893535dd2 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -381,6 +381,7 @@ config SPAPR_TCE_IOMMU
 config ARM_SMMU
 	tristate "ARM Ltd. System MMU (SMMU) Support"
 	depends on (ARM64 || ARM || (COMPILE_TEST && !GENERIC_ATOMIC64)) && MMU
+	depends on QCOM_SCM || !QCOM_SCM #if QCOM_SCM=m this can't be =y
 	select IOMMU_API
 	select IOMMU_IO_PGTABLE_LPAE
 	select ARM_DMA_USE_IOMMU if ARM
@@ -500,6 +501,7 @@ config QCOM_IOMMU
 	# Note: iommu drivers cannot (yet?) be built as modules
 	bool "Qualcomm IOMMU Support"
 	depends on ARCH_QCOM || (COMPILE_TEST && !GENERIC_ATOMIC64)
+	depends on QCOM_SCM=y
 	select IOMMU_API
 	select IOMMU_IO_PGTABLE_LPAE
 	select ARM_DMA_USE_IOMMU
-- 
2.17.1


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

* Re: [PATCH v2 3/5] irqchip: Allow QCOM_PDC to be loadable as a permanent module
  2020-06-25  0:10 ` [PATCH v2 3/5] irqchip: Allow QCOM_PDC to be loadable as a permanent module John Stultz
@ 2020-06-26  7:42   ` Stephen Boyd
  2020-06-27  1:34     ` John Stultz
  0 siblings, 1 reply; 21+ messages in thread
From: Stephen Boyd @ 2020-06-26  7:42 UTC (permalink / raw)
  To: John Stultz, lkml
  Cc: John Stultz, Andy Gross, Bjorn Andersson, Joerg Roedel,
	Thomas Gleixner, Jason Cooper, Marc Zyngier, Linus Walleij,
	Maulik Shah, Lina Iyer, Saravana Kannan, Todd Kjos,
	Greg Kroah-Hartman, linux-arm-msm, iommu, linux-gpio

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");

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

* Re: [PATCH v2 3/5] irqchip: Allow QCOM_PDC to be loadable as a permanent module
  2020-06-26  7:42   ` Stephen Boyd
@ 2020-06-27  1:34     ` John Stultz
  2020-06-27  9:37       ` Marc Zyngier
  0 siblings, 1 reply; 21+ messages in thread
From: John Stultz @ 2020-06-27  1:34 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: lkml, Andy Gross, Bjorn Andersson, Joerg Roedel, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Linus Walleij, Maulik Shah,
	Lina Iyer, Saravana Kannan, Todd Kjos, Greg Kroah-Hartman,
	linux-arm-msm, iommu, linux-gpio

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

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

* Re: [PATCH v2 3/5] irqchip: Allow QCOM_PDC to be loadable as a permanent module
  2020-06-27  1:34     ` John Stultz
@ 2020-06-27  9:37       ` Marc Zyngier
  2020-07-10  6:02         ` Stephen Boyd
  0 siblings, 1 reply; 21+ messages in thread
From: Marc Zyngier @ 2020-06-27  9:37 UTC (permalink / raw)
  To: John Stultz
  Cc: Stephen Boyd, lkml, Andy Gross, Bjorn Andersson, Joerg Roedel,
	Thomas Gleixner, Jason Cooper, Linus Walleij, Maulik Shah,
	Lina Iyer, Saravana Kannan, Todd Kjos, Greg Kroah-Hartman,
	linux-arm-msm, iommu, linux-gpio

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.

-- 
Without deviation from the norm, progress is not possible.

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

* Re: [PATCH v2 5/5] firmware: QCOM_SCM: Allow qcom_scm driver to be loadable as a permenent module
  2020-06-25  0:10 ` [PATCH v2 5/5] firmware: QCOM_SCM: Allow qcom_scm driver to be loadable as a permenent module John Stultz
@ 2020-07-02 12:47   ` Greg Kroah-Hartman
  2020-07-02 14:18   ` Will Deacon
  1 sibling, 0 replies; 21+ messages in thread
From: Greg Kroah-Hartman @ 2020-07-02 12:47 UTC (permalink / raw)
  To: John Stultz
  Cc: lkml, Andy Gross, Bjorn Andersson, Joerg Roedel, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Linus Walleij, Maulik Shah,
	Lina Iyer, Saravana Kannan, Todd Kjos, linux-arm-msm, iommu,
	linux-gpio

On Thu, Jun 25, 2020 at 12:10:39AM +0000, John Stultz wrote:
> Allow the qcom_scm driver to be loadable as a
> permenent module.
> 
> 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>


Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

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

* Re: [PATCH v2 5/5] firmware: QCOM_SCM: Allow qcom_scm driver to be loadable as a permenent module
  2020-06-25  0:10 ` [PATCH v2 5/5] firmware: QCOM_SCM: Allow qcom_scm driver to be loadable as a permenent module John Stultz
  2020-07-02 12:47   ` Greg Kroah-Hartman
@ 2020-07-02 14:18   ` Will Deacon
  2020-07-10  3:28     ` John Stultz
  1 sibling, 1 reply; 21+ messages in thread
From: Will Deacon @ 2020-07-02 14:18 UTC (permalink / raw)
  To: John Stultz
  Cc: lkml, Andy Gross, Bjorn Andersson, Joerg Roedel, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Linus Walleij, Maulik Shah,
	Lina Iyer, Saravana Kannan, Todd Kjos, Greg Kroah-Hartman,
	linux-arm-msm, iommu, linux-gpio

On Thu, Jun 25, 2020 at 12:10:39AM +0000, John Stultz wrote:
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index b510f67dfa49..714893535dd2 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -381,6 +381,7 @@ config SPAPR_TCE_IOMMU
>  config ARM_SMMU
>  	tristate "ARM Ltd. System MMU (SMMU) Support"
>  	depends on (ARM64 || ARM || (COMPILE_TEST && !GENERIC_ATOMIC64)) && MMU
> +	depends on QCOM_SCM || !QCOM_SCM #if QCOM_SCM=m this can't be =y
>  	select IOMMU_API
>  	select IOMMU_IO_PGTABLE_LPAE
>  	select ARM_DMA_USE_IOMMU if ARM

This looks like a giant hack. Is there another way to handle this?

Will

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

* Re: [PATCH v2 5/5] firmware: QCOM_SCM: Allow qcom_scm driver to be loadable as a permenent module
  2020-07-02 14:18   ` Will Deacon
@ 2020-07-10  3:28     ` John Stultz
  2020-07-10  7:54       ` Will Deacon
  0 siblings, 1 reply; 21+ messages in thread
From: John Stultz @ 2020-07-10  3:28 UTC (permalink / raw)
  To: Will Deacon
  Cc: lkml, Andy Gross, Bjorn Andersson, Joerg Roedel, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Linus Walleij, Maulik Shah,
	Lina Iyer, Saravana Kannan, Todd Kjos, Greg Kroah-Hartman,
	linux-arm-msm, iommu, linux-gpio

On Thu, Jul 2, 2020 at 7:18 AM Will Deacon <will@kernel.org> wrote:
> On Thu, Jun 25, 2020 at 12:10:39AM +0000, John Stultz wrote:
> > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> > index b510f67dfa49..714893535dd2 100644
> > --- a/drivers/iommu/Kconfig
> > +++ b/drivers/iommu/Kconfig
> > @@ -381,6 +381,7 @@ config SPAPR_TCE_IOMMU
> >  config ARM_SMMU
> >       tristate "ARM Ltd. System MMU (SMMU) Support"
> >       depends on (ARM64 || ARM || (COMPILE_TEST && !GENERIC_ATOMIC64)) && MMU
> > +     depends on QCOM_SCM || !QCOM_SCM #if QCOM_SCM=m this can't be =y
> >       select IOMMU_API
> >       select IOMMU_IO_PGTABLE_LPAE
> >       select ARM_DMA_USE_IOMMU if ARM
>
> This looks like a giant hack. Is there another way to handle this?

Sorry for the slow response here.

So, I agree the syntax looks strange (requiring a comment obviously
isn't a good sign), but it's a fairly common way to ensure drivers
don't get built in if they optionally depend on another driver that
can be built as a module.
  See "RFKILL || !RFKILL", "EXTCON || !EXTCON", or "USB_GADGET ||
!USB_GADGET" in various Kconfig files.

I'm open to using a different method, and in a different thread you
suggested using something like symbol_get(). I need to look into it
more, but that approach looks even more messy and prone to runtime
failures. Blocking the unwanted case at build time seems a bit cleaner
to me, even if the syntax is odd.

thanks
-john

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

* Re: [PATCH v2 3/5] irqchip: Allow QCOM_PDC to be loadable as a permanent module
  2020-06-27  9:37       ` Marc Zyngier
@ 2020-07-10  6:02         ` Stephen Boyd
  2020-07-10 22:44           ` John Stultz
  0 siblings, 1 reply; 21+ messages in thread
From: Stephen Boyd @ 2020-07-10  6:02 UTC (permalink / raw)
  To: John Stultz, Marc Zyngier
  Cc: lkml, Andy Gross, Bjorn Andersson, Joerg Roedel, Thomas Gleixner,
	Jason Cooper, Linus Walleij, Maulik Shah, Lina Iyer,
	Saravana Kannan, Todd Kjos, Greg Kroah-Hartman, linux-arm-msm,
	iommu, linux-gpio

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?

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

* Re: [PATCH v2 5/5] firmware: QCOM_SCM: Allow qcom_scm driver to be loadable as a permenent module
  2020-07-10  3:28     ` John Stultz
@ 2020-07-10  7:54       ` Will Deacon
  2020-07-10 22:21         ` John Stultz
  0 siblings, 1 reply; 21+ messages in thread
From: Will Deacon @ 2020-07-10  7:54 UTC (permalink / raw)
  To: John Stultz
  Cc: lkml, Andy Gross, Bjorn Andersson, Joerg Roedel, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Linus Walleij, Maulik Shah,
	Lina Iyer, Saravana Kannan, Todd Kjos, Greg Kroah-Hartman,
	linux-arm-msm, iommu, linux-gpio

On Thu, Jul 09, 2020 at 08:28:45PM -0700, John Stultz wrote:
> On Thu, Jul 2, 2020 at 7:18 AM Will Deacon <will@kernel.org> wrote:
> > On Thu, Jun 25, 2020 at 12:10:39AM +0000, John Stultz wrote:
> > > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> > > index b510f67dfa49..714893535dd2 100644
> > > --- a/drivers/iommu/Kconfig
> > > +++ b/drivers/iommu/Kconfig
> > > @@ -381,6 +381,7 @@ config SPAPR_TCE_IOMMU
> > >  config ARM_SMMU
> > >       tristate "ARM Ltd. System MMU (SMMU) Support"
> > >       depends on (ARM64 || ARM || (COMPILE_TEST && !GENERIC_ATOMIC64)) && MMU
> > > +     depends on QCOM_SCM || !QCOM_SCM #if QCOM_SCM=m this can't be =y
> > >       select IOMMU_API
> > >       select IOMMU_IO_PGTABLE_LPAE
> > >       select ARM_DMA_USE_IOMMU if ARM
> >
> > This looks like a giant hack. Is there another way to handle this?
> 
> Sorry for the slow response here.
> 
> So, I agree the syntax looks strange (requiring a comment obviously
> isn't a good sign), but it's a fairly common way to ensure drivers
> don't get built in if they optionally depend on another driver that
> can be built as a module.
>   See "RFKILL || !RFKILL", "EXTCON || !EXTCON", or "USB_GADGET ||
> !USB_GADGET" in various Kconfig files.
> 
> I'm open to using a different method, and in a different thread you
> suggested using something like symbol_get(). I need to look into it
> more, but that approach looks even more messy and prone to runtime
> failures. Blocking the unwanted case at build time seems a bit cleaner
> to me, even if the syntax is odd.

Maybe just split it out then, so that the ARM_SMMU entry doesn't have this,
as that driver _really_ doesn't care about SoC details like this. In other
words, add a new entry along the lines of:

	config ARM_SMMU_QCOM_IMPL
	default y
	#if QCOM_SCM=m this can't be =y
	depends on ARM_SMMU & (QCOM_SCM || !QCOM_SCM)

and then have arm-smmu.h provide a static inline qcom_smmu_impl_init()
which returns -ENODEV if CONFIG_ARM_SMMU_QCOM_IMPL=n and hack the Makefile
so that we don't bother to compile arm-smmu-qcom.o in that case.

Would that work?

Will

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

* Re: [PATCH v2 5/5] firmware: QCOM_SCM: Allow qcom_scm driver to be loadable as a permenent module
  2020-07-10  7:54       ` Will Deacon
@ 2020-07-10 22:21         ` John Stultz
  2020-07-13 20:41           ` Will Deacon
  0 siblings, 1 reply; 21+ messages in thread
From: John Stultz @ 2020-07-10 22:21 UTC (permalink / raw)
  To: Will Deacon
  Cc: lkml, Andy Gross, Bjorn Andersson, Joerg Roedel, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Linus Walleij, Maulik Shah,
	Lina Iyer, Saravana Kannan, Todd Kjos, Greg Kroah-Hartman,
	linux-arm-msm, iommu, linux-gpio

On Fri, Jul 10, 2020 at 12:54 AM Will Deacon <will@kernel.org> wrote:
> On Thu, Jul 09, 2020 at 08:28:45PM -0700, John Stultz wrote:
> > On Thu, Jul 2, 2020 at 7:18 AM Will Deacon <will@kernel.org> wrote:
> > > On Thu, Jun 25, 2020 at 12:10:39AM +0000, John Stultz wrote:
> > > > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> > > > index b510f67dfa49..714893535dd2 100644
> > > > --- a/drivers/iommu/Kconfig
> > > > +++ b/drivers/iommu/Kconfig
> > > > @@ -381,6 +381,7 @@ config SPAPR_TCE_IOMMU
> > > >  config ARM_SMMU
> > > >       tristate "ARM Ltd. System MMU (SMMU) Support"
> > > >       depends on (ARM64 || ARM || (COMPILE_TEST && !GENERIC_ATOMIC64)) && MMU
> > > > +     depends on QCOM_SCM || !QCOM_SCM #if QCOM_SCM=m this can't be =y
> > > >       select IOMMU_API
> > > >       select IOMMU_IO_PGTABLE_LPAE
> > > >       select ARM_DMA_USE_IOMMU if ARM
> > >
> > > This looks like a giant hack. Is there another way to handle this?
> >
> > Sorry for the slow response here.
> >
> > So, I agree the syntax looks strange (requiring a comment obviously
> > isn't a good sign), but it's a fairly common way to ensure drivers
> > don't get built in if they optionally depend on another driver that
> > can be built as a module.
> >   See "RFKILL || !RFKILL", "EXTCON || !EXTCON", or "USB_GADGET ||
> > !USB_GADGET" in various Kconfig files.
> >
> > I'm open to using a different method, and in a different thread you
> > suggested using something like symbol_get(). I need to look into it
> > more, but that approach looks even more messy and prone to runtime
> > failures. Blocking the unwanted case at build time seems a bit cleaner
> > to me, even if the syntax is odd.
>
> Maybe just split it out then, so that the ARM_SMMU entry doesn't have this,
> as that driver _really_ doesn't care about SoC details like this. In other
> words, add a new entry along the lines of:
>
>         config ARM_SMMU_QCOM_IMPL
>         default y
>         #if QCOM_SCM=m this can't be =y
>         depends on ARM_SMMU & (QCOM_SCM || !QCOM_SCM)
>
> and then have arm-smmu.h provide a static inline qcom_smmu_impl_init()
> which returns -ENODEV if CONFIG_ARM_SMMU_QCOM_IMPL=n and hack the Makefile
> so that we don't bother to compile arm-smmu-qcom.o in that case.
>
> Would that work?

I think this proposal still has problems with the directionality of the call.

The arm-smmu-impl.o calls to arm-smmu-qcom.o which calls qcom_scm.o
So if qcom_scm.o is part of a module, the calling code in
arm-smmu-qcom.o also needs to be a module, which means CONFIG_ARM_SMMU
needs to be a module.

I know you said the arm-smmu driver doesn't care about SoC details,
but the trouble is that currently the arm-smmu driver does directly
call the qcom-scm code. So it is a real dependency. However, if
QCOM_SCM is not configured, it calls stubs and that's ok.  In that
way, the "depends on QCOM_SCM || !QCOM_SCM" line actually makes sense.
It looks terrible because we're used to boolean logic, but it's
ternary.

Maybe can have the ARM_SMMU_QCOM_IMPL approach you suggest above, but
that just holds the issue out at arms length, because we're still
going to need to have:
  depends on ARM_SMMU_QCOM_IMPL || !ARM_SMMU_QCOM_IMPL
in the ARM_SMMU definition, which I suspect you're wanting to avoid.

Otherwise the only thing I can think of is a deeper reworking of the
arm-smmu-impl code so that the arm-smmu-qcom code probes itself and
registers its hooks with the arm-smmu core.
That way the arm-smmu driver would not directly call any SoC specific
code (and thus have no dependencies outward). But it's probably a fair
amount of churn vs the extra depends string.

thanks
-john

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

* Re: [PATCH v2 3/5] irqchip: Allow QCOM_PDC to be loadable as a permanent module
  2020-07-10  6:02         ` Stephen Boyd
@ 2020-07-10 22:44           ` John Stultz
  2020-07-10 23:27             ` Stephen Boyd
  0 siblings, 1 reply; 21+ messages in thread
From: John Stultz @ 2020-07-10 22:44 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Marc Zyngier, lkml, Andy Gross, Bjorn Andersson, Joerg Roedel,
	Thomas Gleixner, Jason Cooper, Linus Walleij, Maulik Shah,
	Lina Iyer, Saravana Kannan, Todd Kjos, Greg Kroah-Hartman,
	linux-arm-msm, iommu, linux-gpio

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

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

* Re: [PATCH v2 3/5] irqchip: Allow QCOM_PDC to be loadable as a permanent module
  2020-07-10 22:44           ` John Stultz
@ 2020-07-10 23:27             ` Stephen Boyd
  2020-07-12  9:27               ` Marc Zyngier
  0 siblings, 1 reply; 21+ messages in thread
From: Stephen Boyd @ 2020-07-10 23:27 UTC (permalink / raw)
  To: John Stultz
  Cc: Marc Zyngier, lkml, Andy Gross, Bjorn Andersson, Joerg Roedel,
	Thomas Gleixner, Jason Cooper, Linus Walleij, Maulik Shah,
	Lina Iyer, Saravana Kannan, Todd Kjos, Greg Kroah-Hartman,
	linux-arm-msm, iommu, linux-gpio

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.

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

* Re: [PATCH v2 3/5] irqchip: Allow QCOM_PDC to be loadable as a permanent module
  2020-07-10 23:27             ` Stephen Boyd
@ 2020-07-12  9:27               ` Marc Zyngier
  0 siblings, 0 replies; 21+ messages in thread
From: Marc Zyngier @ 2020-07-12  9:27 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: John Stultz, lkml, Andy Gross, Bjorn Andersson, Joerg Roedel,
	Thomas Gleixner, Jason Cooper, Linus Walleij, Maulik Shah,
	Lina Iyer, Saravana Kannan, Todd Kjos, Greg Kroah-Hartman,
	linux-arm-msm, iommu, linux-gpio

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.

-- 
Without deviation from the norm, progress is not possible.

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

* Re: [PATCH v2 5/5] firmware: QCOM_SCM: Allow qcom_scm driver to be loadable as a permenent module
  2020-07-10 22:21         ` John Stultz
@ 2020-07-13 20:41           ` Will Deacon
  2020-07-13 20:48             ` John Stultz
  0 siblings, 1 reply; 21+ messages in thread
From: Will Deacon @ 2020-07-13 20:41 UTC (permalink / raw)
  To: John Stultz
  Cc: lkml, Andy Gross, Bjorn Andersson, Joerg Roedel, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Linus Walleij, Maulik Shah,
	Lina Iyer, Saravana Kannan, Todd Kjos, Greg Kroah-Hartman,
	linux-arm-msm, iommu, linux-gpio

On Fri, Jul 10, 2020 at 03:21:53PM -0700, John Stultz wrote:
> On Fri, Jul 10, 2020 at 12:54 AM Will Deacon <will@kernel.org> wrote:
> > On Thu, Jul 09, 2020 at 08:28:45PM -0700, John Stultz wrote:
> > > On Thu, Jul 2, 2020 at 7:18 AM Will Deacon <will@kernel.org> wrote:
> > > > On Thu, Jun 25, 2020 at 12:10:39AM +0000, John Stultz wrote:
> > > > > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> > > > > index b510f67dfa49..714893535dd2 100644
> > > > > --- a/drivers/iommu/Kconfig
> > > > > +++ b/drivers/iommu/Kconfig
> > > > > @@ -381,6 +381,7 @@ config SPAPR_TCE_IOMMU
> > > > >  config ARM_SMMU
> > > > >       tristate "ARM Ltd. System MMU (SMMU) Support"
> > > > >       depends on (ARM64 || ARM || (COMPILE_TEST && !GENERIC_ATOMIC64)) && MMU
> > > > > +     depends on QCOM_SCM || !QCOM_SCM #if QCOM_SCM=m this can't be =y
> > > > >       select IOMMU_API
> > > > >       select IOMMU_IO_PGTABLE_LPAE
> > > > >       select ARM_DMA_USE_IOMMU if ARM
> > > >
> > > > This looks like a giant hack. Is there another way to handle this?
> > >
> > > Sorry for the slow response here.
> > >
> > > So, I agree the syntax looks strange (requiring a comment obviously
> > > isn't a good sign), but it's a fairly common way to ensure drivers
> > > don't get built in if they optionally depend on another driver that
> > > can be built as a module.
> > >   See "RFKILL || !RFKILL", "EXTCON || !EXTCON", or "USB_GADGET ||
> > > !USB_GADGET" in various Kconfig files.
> > >
> > > I'm open to using a different method, and in a different thread you
> > > suggested using something like symbol_get(). I need to look into it
> > > more, but that approach looks even more messy and prone to runtime
> > > failures. Blocking the unwanted case at build time seems a bit cleaner
> > > to me, even if the syntax is odd.
> >
> > Maybe just split it out then, so that the ARM_SMMU entry doesn't have this,
> > as that driver _really_ doesn't care about SoC details like this. In other
> > words, add a new entry along the lines of:
> >
> >         config ARM_SMMU_QCOM_IMPL
> >         default y
> >         #if QCOM_SCM=m this can't be =y
> >         depends on ARM_SMMU & (QCOM_SCM || !QCOM_SCM)
> >
> > and then have arm-smmu.h provide a static inline qcom_smmu_impl_init()
> > which returns -ENODEV if CONFIG_ARM_SMMU_QCOM_IMPL=n and hack the Makefile
> > so that we don't bother to compile arm-smmu-qcom.o in that case.
> >
> > Would that work?
> 
> I think this proposal still has problems with the directionality of the call.
> 
> The arm-smmu-impl.o calls to arm-smmu-qcom.o which calls qcom_scm.o
> So if qcom_scm.o is part of a module, the calling code in
> arm-smmu-qcom.o also needs to be a module, which means CONFIG_ARM_SMMU
> needs to be a module.
> 
> I know you said the arm-smmu driver doesn't care about SoC details,
> but the trouble is that currently the arm-smmu driver does directly
> call the qcom-scm code. So it is a real dependency. However, if
> QCOM_SCM is not configured, it calls stubs and that's ok.  In that
> way, the "depends on QCOM_SCM || !QCOM_SCM" line actually makes sense.
> It looks terrible because we're used to boolean logic, but it's
> ternary.

Yes, it looks ugly, but the part I really have issues with is that building
QCOM_SCM=m and ARM_SMMU=y is perfectly fine if you don't run on an SoC
with the qcom implementation. I don't see why we need to enforce things
here beyond making sure that all selectable permutations _build_ and
fail gracefully at runtime on the qcom SoC if SCM isn't available.

Will

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

* Re: [PATCH v2 5/5] firmware: QCOM_SCM: Allow qcom_scm driver to be loadable as a permenent module
  2020-07-13 20:41           ` Will Deacon
@ 2020-07-13 20:48             ` John Stultz
  2020-07-14  7:56               ` Will Deacon
  0 siblings, 1 reply; 21+ messages in thread
From: John Stultz @ 2020-07-13 20:48 UTC (permalink / raw)
  To: Will Deacon
  Cc: lkml, Andy Gross, Bjorn Andersson, Joerg Roedel, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Linus Walleij, Maulik Shah,
	Lina Iyer, Saravana Kannan, Todd Kjos, Greg Kroah-Hartman,
	linux-arm-msm, iommu, linux-gpio

On Mon, Jul 13, 2020 at 1:41 PM Will Deacon <will@kernel.org> wrote:
>
> On Fri, Jul 10, 2020 at 03:21:53PM -0700, John Stultz wrote:
> > On Fri, Jul 10, 2020 at 12:54 AM Will Deacon <will@kernel.org> wrote:
> > > On Thu, Jul 09, 2020 at 08:28:45PM -0700, John Stultz wrote:
> > > > On Thu, Jul 2, 2020 at 7:18 AM Will Deacon <will@kernel.org> wrote:
> > > > > On Thu, Jun 25, 2020 at 12:10:39AM +0000, John Stultz wrote:
> > > > > > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> > > > > > index b510f67dfa49..714893535dd2 100644
> > > > > > --- a/drivers/iommu/Kconfig
> > > > > > +++ b/drivers/iommu/Kconfig
> > > > > > @@ -381,6 +381,7 @@ config SPAPR_TCE_IOMMU
> > > > > >  config ARM_SMMU
> > > > > >       tristate "ARM Ltd. System MMU (SMMU) Support"
> > > > > >       depends on (ARM64 || ARM || (COMPILE_TEST && !GENERIC_ATOMIC64)) && MMU
> > > > > > +     depends on QCOM_SCM || !QCOM_SCM #if QCOM_SCM=m this can't be =y
> > > > > >       select IOMMU_API
> > > > > >       select IOMMU_IO_PGTABLE_LPAE
> > > > > >       select ARM_DMA_USE_IOMMU if ARM
> > > > >
> > > > > This looks like a giant hack. Is there another way to handle this?
> > > >
> > > > Sorry for the slow response here.
> > > >
> > > > So, I agree the syntax looks strange (requiring a comment obviously
> > > > isn't a good sign), but it's a fairly common way to ensure drivers
> > > > don't get built in if they optionally depend on another driver that
> > > > can be built as a module.
> > > >   See "RFKILL || !RFKILL", "EXTCON || !EXTCON", or "USB_GADGET ||
> > > > !USB_GADGET" in various Kconfig files.
> > > >
> > > > I'm open to using a different method, and in a different thread you
> > > > suggested using something like symbol_get(). I need to look into it
> > > > more, but that approach looks even more messy and prone to runtime
> > > > failures. Blocking the unwanted case at build time seems a bit cleaner
> > > > to me, even if the syntax is odd.
> > >
> > > Maybe just split it out then, so that the ARM_SMMU entry doesn't have this,
> > > as that driver _really_ doesn't care about SoC details like this. In other
> > > words, add a new entry along the lines of:
> > >
> > >         config ARM_SMMU_QCOM_IMPL
> > >         default y
> > >         #if QCOM_SCM=m this can't be =y
> > >         depends on ARM_SMMU & (QCOM_SCM || !QCOM_SCM)
> > >
> > > and then have arm-smmu.h provide a static inline qcom_smmu_impl_init()
> > > which returns -ENODEV if CONFIG_ARM_SMMU_QCOM_IMPL=n and hack the Makefile
> > > so that we don't bother to compile arm-smmu-qcom.o in that case.
> > >
> > > Would that work?
> >
> > I think this proposal still has problems with the directionality of the call.
> >
> > The arm-smmu-impl.o calls to arm-smmu-qcom.o which calls qcom_scm.o
> > So if qcom_scm.o is part of a module, the calling code in
> > arm-smmu-qcom.o also needs to be a module, which means CONFIG_ARM_SMMU
> > needs to be a module.
> >
> > I know you said the arm-smmu driver doesn't care about SoC details,
> > but the trouble is that currently the arm-smmu driver does directly
> > call the qcom-scm code. So it is a real dependency. However, if
> > QCOM_SCM is not configured, it calls stubs and that's ok.  In that
> > way, the "depends on QCOM_SCM || !QCOM_SCM" line actually makes sense.
> > It looks terrible because we're used to boolean logic, but it's
> > ternary.
>
> Yes, it looks ugly, but the part I really have issues with is that building
> QCOM_SCM=m and ARM_SMMU=y is perfectly fine if you don't run on an SoC
> with the qcom implementation. I don't see why we need to enforce things
> here beyond making sure that all selectable permutations _build_ and
> fail gracefully at runtime on the qcom SoC if SCM isn't available.

Ok. Thanks, that context/rationale makes sense to me now!  I'll dig in
and see if I can figure out the runtime get_symbol handling you
suggested for the scm callout.

Thanks again for the feedback!
-john

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

* Re: [PATCH v2 5/5] firmware: QCOM_SCM: Allow qcom_scm driver to be loadable as a permenent module
  2020-07-13 20:48             ` John Stultz
@ 2020-07-14  7:56               ` Will Deacon
  0 siblings, 0 replies; 21+ messages in thread
From: Will Deacon @ 2020-07-14  7:56 UTC (permalink / raw)
  To: John Stultz
  Cc: lkml, Andy Gross, Bjorn Andersson, Joerg Roedel, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Linus Walleij, Maulik Shah,
	Lina Iyer, Saravana Kannan, Todd Kjos, Greg Kroah-Hartman,
	linux-arm-msm, iommu, linux-gpio

On Mon, Jul 13, 2020 at 01:48:29PM -0700, John Stultz wrote:
> On Mon, Jul 13, 2020 at 1:41 PM Will Deacon <will@kernel.org> wrote:
> > On Fri, Jul 10, 2020 at 03:21:53PM -0700, John Stultz wrote:
> > > On Fri, Jul 10, 2020 at 12:54 AM Will Deacon <will@kernel.org> wrote:
> > > > On Thu, Jul 09, 2020 at 08:28:45PM -0700, John Stultz wrote:
> > > > > On Thu, Jul 2, 2020 at 7:18 AM Will Deacon <will@kernel.org> wrote:
> > > > > > On Thu, Jun 25, 2020 at 12:10:39AM +0000, John Stultz wrote:
> > > > > > > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> > > > > > > index b510f67dfa49..714893535dd2 100644
> > > > > > > --- a/drivers/iommu/Kconfig
> > > > > > > +++ b/drivers/iommu/Kconfig
> > > > > > > @@ -381,6 +381,7 @@ config SPAPR_TCE_IOMMU
> > > > > > >  config ARM_SMMU
> > > > > > >       tristate "ARM Ltd. System MMU (SMMU) Support"
> > > > > > >       depends on (ARM64 || ARM || (COMPILE_TEST && !GENERIC_ATOMIC64)) && MMU
> > > > > > > +     depends on QCOM_SCM || !QCOM_SCM #if QCOM_SCM=m this can't be =y
> > > > > > >       select IOMMU_API
> > > > > > >       select IOMMU_IO_PGTABLE_LPAE
> > > > > > >       select ARM_DMA_USE_IOMMU if ARM
> > > > > >
> > > > > > This looks like a giant hack. Is there another way to handle this?
> > > > >
> > > > > Sorry for the slow response here.
> > > > >
> > > > > So, I agree the syntax looks strange (requiring a comment obviously
> > > > > isn't a good sign), but it's a fairly common way to ensure drivers
> > > > > don't get built in if they optionally depend on another driver that
> > > > > can be built as a module.
> > > > >   See "RFKILL || !RFKILL", "EXTCON || !EXTCON", or "USB_GADGET ||
> > > > > !USB_GADGET" in various Kconfig files.
> > > > >
> > > > > I'm open to using a different method, and in a different thread you
> > > > > suggested using something like symbol_get(). I need to look into it
> > > > > more, but that approach looks even more messy and prone to runtime
> > > > > failures. Blocking the unwanted case at build time seems a bit cleaner
> > > > > to me, even if the syntax is odd.
> > > >
> > > > Maybe just split it out then, so that the ARM_SMMU entry doesn't have this,
> > > > as that driver _really_ doesn't care about SoC details like this. In other
> > > > words, add a new entry along the lines of:
> > > >
> > > >         config ARM_SMMU_QCOM_IMPL
> > > >         default y
> > > >         #if QCOM_SCM=m this can't be =y
> > > >         depends on ARM_SMMU & (QCOM_SCM || !QCOM_SCM)
> > > >
> > > > and then have arm-smmu.h provide a static inline qcom_smmu_impl_init()
> > > > which returns -ENODEV if CONFIG_ARM_SMMU_QCOM_IMPL=n and hack the Makefile
> > > > so that we don't bother to compile arm-smmu-qcom.o in that case.
> > > >
> > > > Would that work?
> > >
> > > I think this proposal still has problems with the directionality of the call.
> > >
> > > The arm-smmu-impl.o calls to arm-smmu-qcom.o which calls qcom_scm.o
> > > So if qcom_scm.o is part of a module, the calling code in
> > > arm-smmu-qcom.o also needs to be a module, which means CONFIG_ARM_SMMU
> > > needs to be a module.
> > >
> > > I know you said the arm-smmu driver doesn't care about SoC details,
> > > but the trouble is that currently the arm-smmu driver does directly
> > > call the qcom-scm code. So it is a real dependency. However, if
> > > QCOM_SCM is not configured, it calls stubs and that's ok.  In that
> > > way, the "depends on QCOM_SCM || !QCOM_SCM" line actually makes sense.
> > > It looks terrible because we're used to boolean logic, but it's
> > > ternary.
> >
> > Yes, it looks ugly, but the part I really have issues with is that building
> > QCOM_SCM=m and ARM_SMMU=y is perfectly fine if you don't run on an SoC
> > with the qcom implementation. I don't see why we need to enforce things
> > here beyond making sure that all selectable permutations _build_ and
> > fail gracefully at runtime on the qcom SoC if SCM isn't available.
> 
> Ok. Thanks, that context/rationale makes sense to me now!  I'll dig in
> and see if I can figure out the runtime get_symbol handling you
> suggested for the scm callout.

Cheers, but in the interest of not being a massive pain in the bum, please
yell if it ends up being tonnes of work and I'll close my eyes while
applying your original patch.

Will

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

end of thread, back to index

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-25  0:10 [PATCH v2 0/5] Allow for qcom-pdc, pinctrl-msm and qcom-scm drivers to be loadable as modules John Stultz
2020-06-25  0:10 ` [PATCH v2 1/5] irq: irqdomain: Export irq_domain_update_bus_token John Stultz
2020-06-25  0:10 ` [PATCH v2 2/5] irq: irqchip: Export irq_chip_retrigger_hierarchy and irq_chip_set_vcpu_affinity_parent John Stultz
2020-06-25  0:10 ` [PATCH v2 3/5] irqchip: Allow QCOM_PDC to be loadable as a permanent module John Stultz
2020-06-26  7:42   ` Stephen Boyd
2020-06-27  1:34     ` John Stultz
2020-06-27  9:37       ` Marc Zyngier
2020-07-10  6:02         ` Stephen Boyd
2020-07-10 22:44           ` John Stultz
2020-07-10 23:27             ` Stephen Boyd
2020-07-12  9:27               ` Marc Zyngier
2020-06-25  0:10 ` [PATCH v2 4/5] pinctrl: qcom: Allow pinctrl-msm code to be loadable as a module John Stultz
2020-06-25  0:10 ` [PATCH v2 5/5] firmware: QCOM_SCM: Allow qcom_scm driver to be loadable as a permenent module John Stultz
2020-07-02 12:47   ` Greg Kroah-Hartman
2020-07-02 14:18   ` Will Deacon
2020-07-10  3:28     ` John Stultz
2020-07-10  7:54       ` Will Deacon
2020-07-10 22:21         ` John Stultz
2020-07-13 20:41           ` Will Deacon
2020-07-13 20:48             ` John Stultz
2020-07-14  7:56               ` Will Deacon

Linux-GPIO Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-gpio/0 linux-gpio/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-gpio linux-gpio/ https://lore.kernel.org/linux-gpio \
		linux-gpio@vger.kernel.org
	public-inbox-index linux-gpio

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-gpio


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git