[PATCH 00/11] Modular Broadcom irqchip drivers

Hi Thomas, Marc,

This patch series aims at allowing the 3 interrupt controller drivers
used on Broadcom STB platforms to be built as modules in order for those
to be shipped in a GKI enabled system (Android).

The irq-bcm7038-l1 requires us to export a number of symbols, which is
not great, but there are not obvious solutions other than adding
accessor functions to get the same information.

Assuming you are happy with the changes though, please do take the last
two changes as well through your tree.

Thanks!

Florian Fainelli (11):
  arch: Export cpu_logical_map to modules
  genirq: Export irq_to_desc() again to modules
  genirq: Export irq_set_affinity_locked()
  irqchip/irq-bcm7038-l1: Switch to IRQCHIP_PLATFORM_DRIVER
  irqchip/irq-brcmstb-l2: Switch to IRQCHIP_PLATFORM_DRIVER
  genirq: Export irq_gc_{unmask_enable,mask_disable}_reg
  of/irq: Export of_irq_count to drivers
  genirq: Export irq_gc_noop()
  irqchip/irq-bcm7120-l2: Switch to IRQCHIP_PLATFORM_DRIVER
  arm64: broadcom: Removed forced select of interrupt controllers
  ARM: bcm: Removed forced select of interrupt controllers

 arch/arm/kernel/setup.c          |  1 +
 arch/arm/mach-bcm/Kconfig        |  4 ----
 arch/arm64/Kconfig.platforms     |  3 ---
 arch/arm64/kernel/setup.c        |  1 +
 arch/sh/kernel/smp.c             |  1 +
 drivers/irqchip/Kconfig          | 12 +++++++++---
 drivers/irqchip/irq-bcm7038-l1.c |  6 +++++-
 drivers/irqchip/irq-bcm7120-l2.c | 11 ++++++-----
 drivers/irqchip/irq-brcmstb-l2.c | 16 +++++++++-------
 drivers/of/irq.c                 |  1 +
 kernel/irq/generic-chip.c        |  3 +++
 kernel/irq/irqdesc.c             |  2 --
 kernel/irq/manage.c              |  1 +
 13 files changed, 37 insertions(+), 25 deletions(-)

-- 
2.25.1

In order to allow drivers/irqchip/irq-bcm7038-l1.c to be built as a
module and usable in GKI, export cpu_logical_map or __cpu_logical_map
towards the modules.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 arch/arm/kernel/setup.c   | 1 +
 arch/arm64/kernel/setup.c | 1 +
 arch/sh/kernel/smp.c      | 1 +
 3 files changed, 3 insertions(+)

diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index 284a80c0b6e1..abd5c999bdb2 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c

@@ -585,6 +585,7 @@ void notrace cpu_init(void)
 }
 
 u32 __cpu_logical_map[NR_CPUS] = { [0 ... NR_CPUS-1] = MPIDR_INVALID };
+EXPORT_SYMBOL(__cpu_logical_map);
 
 void __init smp_setup_processor_id(void)
 {

diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index be5f85b0a24d..d8f796ae13c4 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c

@@ -290,6 +290,7 @@ u64 cpu_logical_map(unsigned int cpu)
 {
 	return __cpu_logical_map[cpu];
 }
+EXPORT_SYMBOL(cpu_logical_map);
 
 void __init __no_sanitize_address setup_arch(char **cmdline_p)
 {

diff --git a/arch/sh/kernel/smp.c b/arch/sh/kernel/smp.c
index 65924d9ec245..8f16cfaad238 100644
--- a/arch/sh/kernel/smp.c
+++ b/arch/sh/kernel/smp.c

@@ -30,6 +30,7 @@
 
 int __cpu_number_map[NR_CPUS];		/* Map physical to logical */
 int __cpu_logical_map[NR_CPUS];		/* Map logical to physical */
+EXPORT_SYMBOL(__cpu_logical_map);
 
 struct plat_smp_ops *mp_ops = NULL;
 

-- 
2.25.1

In order to build drivers/irqchip/irq-bcm7038-l1.c as a module (for use
in GKI), we need to export_to_desc() which is used in this snippet of
code:

	irqd_set_single_target(irq_desc_get_irq_data(irq_to_desc(virq)));

This effectively reverts 64a1b95bb9fe ("genirq: Restrict export of
irq_to_desc()").

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 kernel/irq/irqdesc.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index 4e3c29bb603c..5bc231036042 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c

@@ -352,9 +352,7 @@ struct irq_desc *irq_to_desc(unsigned int irq)
 {
 	return radix_tree_lookup(&irq_desc_tree, irq);
 }
-#ifdef CONFIG_KVM_BOOK3S_64_HV_MODULE
 EXPORT_SYMBOL_GPL(irq_to_desc);
-#endif
 
 static void delete_irq_desc(unsigned int irq)
 {

-- 
2.25.1

In order to build drivers/irqchip/irq-bcm7120-l2.c as a module, we will
need to have of_irq_count() exported to modules.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/of/irq.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 352e14b007e7..949b9d1f8729 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c

@@ -440,6 +440,7 @@ int of_irq_count(struct device_node *dev)
 
 	return nr;
 }
+EXPORT_SYMBOL_GPL(of_irq_count);
 
 /**
  * of_irq_to_resource_table - Fill in resource table with node's IRQ info

-- 
2.25.1

In order to allow drivers/irqchip/irq-brcmstb-l2.c to be built as a
module we need to export: irq_gc_unmask_enable_reg() and
irq_gc_mask_disable_reg().

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 kernel/irq/generic-chip.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/irq/generic-chip.c b/kernel/irq/generic-chip.c
index cc7cdd26e23e..4c011c21bb1a 100644
--- a/kernel/irq/generic-chip.c
+++ b/kernel/irq/generic-chip.c

@@ -44,6 +44,7 @@ void irq_gc_mask_disable_reg(struct irq_data *d)
 	*ct->mask_cache &= ~mask;
 	irq_gc_unlock(gc);
 }
+EXPORT_SYMBOL_GPL(irq_gc_mask_disable_reg);
 
 /**
  * irq_gc_mask_set_bit - Mask chip via setting bit in mask register

@@ -103,6 +104,7 @@ void irq_gc_unmask_enable_reg(struct irq_data *d)
 	*ct->mask_cache |= mask;
 	irq_gc_unlock(gc);
 }
+EXPORT_SYMBOL_GPL(irq_gc_unmask_enable_reg);
 
 /**
  * irq_gc_ack_set_bit - Ack pending interrupt via setting bit

-- 
2.25.1

Allow the user selection and building of this interrupt controller
driver as a module since it is used on ARM/ARM64 based systems as a
second level interrupt controller hanging off the ARM GIC and is
therefore loadable during boot.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/irqchip/Kconfig          |  4 +++-
 drivers/irqchip/irq-brcmstb-l2.c | 16 +++++++++-------
 2 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 3022f6137096..dfe54bf9b35f 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig

@@ -128,7 +128,9 @@ config BCM7120_L2_IRQ
 	select IRQ_DOMAIN
 
 config BRCMSTB_L2_IRQ
-	bool
+	tristate "Broadcom STB generic L2 interrupt controller driver"
+	depends on ARCH_BCM2835 || ARCH_BRCMSTB || BMIPS_GENERIC
+	default ARCH_BCM2835 || ARCH_BRCMSTB || BMIPS_GENERIC
 	select GENERIC_IRQ_CHIP
 	select IRQ_DOMAIN
 

diff --git a/drivers/irqchip/irq-brcmstb-l2.c b/drivers/irqchip/irq-brcmstb-l2.c
index 8e0911561f2d..e4efc08ac594 100644
--- a/drivers/irqchip/irq-brcmstb-l2.c
+++ b/drivers/irqchip/irq-brcmstb-l2.c

@@ -275,16 +275,18 @@ static int __init brcmstb_l2_edge_intc_of_init(struct device_node *np,
 {
 	return brcmstb_l2_intc_of_init(np, parent, &l2_edge_intc_init);
 }
-IRQCHIP_DECLARE(brcmstb_l2_intc, "brcm,l2-intc", brcmstb_l2_edge_intc_of_init);
-IRQCHIP_DECLARE(brcmstb_hif_spi_l2_intc, "brcm,hif-spi-l2-intc",
-		brcmstb_l2_edge_intc_of_init);
-IRQCHIP_DECLARE(brcmstb_upg_aux_aon_l2_intc, "brcm,upg-aux-aon-l2-intc",
-		brcmstb_l2_edge_intc_of_init);
 
 static int __init brcmstb_l2_lvl_intc_of_init(struct device_node *np,
 	struct device_node *parent)
 {
 	return brcmstb_l2_intc_of_init(np, parent, &l2_lvl_intc_init);
 }
-IRQCHIP_DECLARE(bcm7271_l2_intc, "brcm,bcm7271-l2-intc",
-	brcmstb_l2_lvl_intc_of_init);
+
+IRQCHIP_PLATFORM_DRIVER_BEGIN(brcmstb_l2)
+IRQCHIP_MATCH("brcm,l2-intc", brcmstb_l2_edge_intc_of_init)
+IRQCHIP_MATCH("brcm,hif-spi-l2-intc", brcmstb_l2_edge_intc_of_init)
+IRQCHIP_MATCH("brcm,upg-aux-aon-l2-intc", brcmstb_l2_edge_intc_of_init)
+IRQCHIP_MATCH("brcm,bcm7271-l2-intc", brcmstb_l2_lvl_intc_of_init)
+IRQCHIP_PLATFORM_DRIVER_END(brcmstb_l2)
+MODULE_DESCRIPTION("Broadcom STB generic L2 interrupt controller");
+MODULE_LICENSE("GPL v2");

-- 
2.25.1

In order to build drivers/irqchip/irq-bcm7120-l2.c as a module which
references irq_gc_noop(), we need to export it towards modules.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 kernel/irq/generic-chip.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/irq/generic-chip.c b/kernel/irq/generic-chip.c
index 4c011c21bb1a..6f29bf4c8515 100644
--- a/kernel/irq/generic-chip.c
+++ b/kernel/irq/generic-chip.c

@@ -25,6 +25,7 @@ static DEFINE_RAW_SPINLOCK(gc_lock);
 void irq_gc_noop(struct irq_data *d)
 {
 }
+EXPORT_SYMBOL_GPL(irq_gc_noop);
 
 /**
  * irq_gc_mask_disable_reg - Mask chip via disable register

-- 
2.25.1

Now that the various second level interrupt controllers have been moved
to IRQCHIP_PLATFORM_DRIVER and they do default to ARCH_BRCMSTB and
ARCH_BCM2835 where relevant, remove their forced selection from the
machine entry to allow an user to build them as modules.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 arch/arm64/Kconfig.platforms | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
index b0ce18d4cc98..2e9440f2da22 100644
--- a/arch/arm64/Kconfig.platforms
+++ b/arch/arm64/Kconfig.platforms

@@ -44,7 +44,6 @@ config ARCH_BCM2835
 	select ARM_AMBA
 	select ARM_GIC
 	select ARM_TIMER_SP804
-	select BRCMSTB_L2_IRQ
 	help
 	  This enables support for the Broadcom BCM2837 and BCM2711 SoC.
 	  These SoCs are used in the Raspberry Pi 3 and 4 devices.

@@ -82,8 +81,6 @@ config ARCH_BITMAIN
 config ARCH_BRCMSTB
 	bool "Broadcom Set-Top-Box SoCs"
 	select ARCH_HAS_RESET_CONTROLLER
-	select BCM7038_L1_IRQ
-	select BRCMSTB_L2_IRQ
 	select GENERIC_IRQ_CHIP
 	select PINCTRL
 	help

-- 
2.25.1

Allow the user selection and building of this interrupt controller
driver as a module since it is used on ARM/ARM64 based systems as a
second level interrupt controller hanging off the ARM GIC and is
therefore loadable during boot.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/irqchip/Kconfig          |  4 +++-
 drivers/irqchip/irq-bcm7120-l2.c | 11 ++++++-----
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index dfe54bf9b35f..c7320bed5668 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig

@@ -123,7 +123,9 @@ config BCM7038_L1_IRQ
 	select GENERIC_IRQ_EFFECTIVE_AFF_MASK
 
 config BCM7120_L2_IRQ
-	bool
+	tristate "Broadcom STB 7120-style L2 interrupt controller driver"
+	depends on ARCH_BRCMSTB || BMIPS_GENERIC
+	default ARCH_BRCMSTB || BMIPS_GENERIC
 	select GENERIC_IRQ_CHIP
 	select IRQ_DOMAIN
 

diff --git a/drivers/irqchip/irq-bcm7120-l2.c b/drivers/irqchip/irq-bcm7120-l2.c
index f23d7651ea84..c1676419dee7 100644
--- a/drivers/irqchip/irq-bcm7120-l2.c
+++ b/drivers/irqchip/irq-bcm7120-l2.c

@@ -347,8 +347,9 @@ static int __init bcm7120_l2_intc_probe_3380(struct device_node *dn,
 				     "BCM3380 L2");
 }
 
-IRQCHIP_DECLARE(bcm7120_l2_intc, "brcm,bcm7120-l2-intc",
-		bcm7120_l2_intc_probe_7120);
-
-IRQCHIP_DECLARE(bcm3380_l2_intc, "brcm,bcm3380-l2-intc",
-		bcm7120_l2_intc_probe_3380);
+IRQCHIP_PLATFORM_DRIVER_BEGIN(bcm7120_l2)
+IRQCHIP_MATCH("brcm,bcm7120-l2-intc", bcm7120_l2_intc_probe_7120)
+IRQCHIP_MATCH("brcm,bcm3380-l2-intc", bcm7120_l2_intc_probe_3380)
+IRQCHIP_PLATFORM_DRIVER_END(bcm7120_l2)
+MODULE_DESCRIPTION("Broadcom STB 7120-style L2 interrupt controller driver");
+MODULE_LICENSE("GPL v2");

-- 
2.25.1

Now that the various second level interrupt controllers have been moved
to IRQCHIP_PLATFORM_DRIVER and they do default to ARCH_BRCMSTB and
ARCH_BCM2835 where relevant, remove their forced selection from the
machine entry to allow an user to build them as modules.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 arch/arm/mach-bcm/Kconfig | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/arch/arm/mach-bcm/Kconfig b/arch/arm/mach-bcm/Kconfig
index 2890e61b2b46..bd3f82788ebc 100644
--- a/arch/arm/mach-bcm/Kconfig
+++ b/arch/arm/mach-bcm/Kconfig

@@ -161,7 +161,6 @@ config ARCH_BCM2835
 	select ARM_TIMER_SP804
 	select HAVE_ARM_ARCH_TIMER if ARCH_MULTI_V7
 	select BCM2835_TIMER
-	select BRCMSTB_L2_IRQ
 	select PINCTRL
 	select PINCTRL_BCM2835
 	select MFD_CORE

@@ -209,9 +208,6 @@ config ARCH_BRCMSTB
 	select ARM_GIC
 	select ARM_ERRATA_798181 if SMP
 	select HAVE_ARM_ARCH_TIMER
-	select BCM7038_L1_IRQ
-	select BRCMSTB_L2_IRQ
-	select BCM7120_L2_IRQ
 	select ZONE_DMA if ARM_LPAE
 	select SOC_BRCMSTB
 	select SOC_BUS

-- 
2.25.1

Allow the user selection and building of this interrupt controller
driver as a module since it is used on ARM/ARM64 based systems as a
second level interrupt controller hanging off the ARM GIC and is
therefore loadable during boot.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/irqchip/Kconfig          | 4 +++-
 drivers/irqchip/irq-bcm7038-l1.c | 6 +++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 4d5924e9f766..3022f6137096 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig

@@ -115,7 +115,9 @@ config BCM6345_L1_IRQ
 	select GENERIC_IRQ_EFFECTIVE_AFF_MASK
 
 config BCM7038_L1_IRQ
-	bool
+	tristate "Broadcom STB 7038-style L1/L2 interrupt controller driver"
+	depends on ARCH_BRCMSTB || BMIPS_GENERIC
+	default ARCH_BRCMSTB || BMIPS_GENERIC
 	select GENERIC_IRQ_CHIP
 	select IRQ_DOMAIN
 	select GENERIC_IRQ_EFFECTIVE_AFF_MASK

diff --git a/drivers/irqchip/irq-bcm7038-l1.c b/drivers/irqchip/irq-bcm7038-l1.c
index a035c385ca7a..80e74cf3e165 100644
--- a/drivers/irqchip/irq-bcm7038-l1.c
+++ b/drivers/irqchip/irq-bcm7038-l1.c

@@ -484,4 +484,8 @@ static int __init bcm7038_l1_of_init(struct device_node *dn,
 	return ret;
 }
 
-IRQCHIP_DECLARE(bcm7038_l1, "brcm,bcm7038-l1-intc", bcm7038_l1_of_init);
+IRQCHIP_PLATFORM_DRIVER_BEGIN(bcm7038_l1)
+IRQCHIP_MATCH("brcm,bcm7038-l1-intc", bcm7038_l1_of_init)
+IRQCHIP_PLATFORM_DRIVER_END(bcm7038_l1)
+MODULE_DESCRIPTION("Broadcom STB 7038-style L1/L2 interrupt controller");
+MODULE_LICENSE("GPL v2");

-- 
2.25.1

irq-bcm7038-l1 uses that symbol and we want to make it a loadable module
in subsequent changes.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 kernel/irq/manage.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 7405e384e5ed..e0c573e5d249 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c

@@ -369,6 +369,7 @@ int irq_set_affinity_locked(struct irq_data *data, const struct cpumask *mask,
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(irq_set_affinity_locked);
 
 /**
  * irq_update_affinity_desc - Update affinity management for an interrupt

-- 
2.25.1

On Fri, 24 Sep 2021 18:05:38 +0100,
Florian Fainelli [off-list ref] wrote:

This doesn't seem right.

This driver seem to try and move interrupts on its own when the CPU
goes down. Why can't it rely on the normal CPU hotplug infrastructure
to do so like all the other drivers (bar some Cavium driver that does
the same thing)?

I'd rather you take this opportunity to move these drivers into the
21st century, so that we can kill irq_cpu_offline() and co altogether.

Thanks,

	M.

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

On Fri, 24 Sep 2021 18:05:45 +0100,
Florian Fainelli [off-list ref] wrote:

How does the user know about that? People will build a kernel
selecting their platform, and find out it doesn't work. This seems
terribly counter-productive to me.

	M.

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

On Sat, Sep 25, 2021 at 2:10 PM Marc Zyngier [off-list ref] wrote:

On Fri, 24 Sep 2021 18:05:45 +0100, Florian Fainelli [off-list ref] wrote:

How does the user know about that? People will build a kernel
selecting their platform, and find out it doesn't work. This seems
terribly counter-productive to me.

It's default-enabled on the platforms that need it, see one of the
earlier patches.

Having the option to make it a loadable module is a definite benefit as
far as I'm concerned, and I generally like the idea of having individually
selectable symbols for consistency as that is what we have in other
subsystems as well.

Ideally I'd do away with all the 'select' statements for the platforms and
only have them control dependencies as we do for most other subsystems.
irqchip is one of the few exceptions here, though I understand the reason
for having the most important drivers tied to the platform more closely.

       Arnd

On Fri, Sep 24 2021 at 10:05, Florian Fainelli wrote:

In order to build drivers/irqchip/irq-bcm7038-l1.c as a module (for use
in GKI), we need to export_to_desc() which is used in this snippet of
code:

	irqd_set_single_target(irq_desc_get_irq_data(irq_to_desc(virq)));

This effectively reverts 64a1b95bb9fe ("genirq: Restrict export of
irq_to_desc()").

No. I'm not reexporting this. We've spent quite some time to prevent all
kind of drivers for fiddle with irq descriptors and I'm not going
to reopen that can of worms.

irq_get_irq_data() is exported and provides you what you need.

Thanks,

        tglx

On Sat, Sep 25 2021 at 12:48, Marc Zyngier wrote:

On Fri, 24 Sep 2021 18:05:38 +0100, Florian Fainelli [off-list ref] wrote:

quoted

 }
+EXPORT_SYMBOL_GPL(irq_set_affinity_locked);

This doesn't seem right.

This driver seem to try and move interrupts on its own when the CPU
goes down. Why can't it rely on the normal CPU hotplug infrastructure
to do so like all the other drivers (bar some Cavium driver that does
the same thing)?

I'd rather you take this opportunity to move these drivers into the
21st century, so that we can kill irq_cpu_offline() and co altogether.

I wanted to kill these callbacks years ago. Cavium has two variants of
those offline/online callbacks:

 1) octeon_irq_cpu_offline_ciu() which is doing the same as that BCM
    driver. These really can go away. Just remove the callback and
    everything just works.

 2) Two other variants to fiddle with chip internals, but those chips do
    not have an irq_affinity() callback which makes it more interesting.

    I don't see a proper way to solve that except for removing Cavium
    alltogether, but once the BCM one is gone, we just can make this
    muck depend on CAVIUM and be done with it. And I mean depend and not
    select.

Thanks,

        tglx



    

On Sat, Sep 25 2021 at 23:21, Thomas Gleixner wrote:

On Sat, Sep 25 2021 at 12:48, Marc Zyngier wrote:

quoted

On Fri, 24 Sep 2021 18:05:38 +0100, Florian Fainelli [off-list ref] wrote:

quoted

 }
+EXPORT_SYMBOL_GPL(irq_set_affinity_locked);

This doesn't seem right.

This driver seem to try and move interrupts on its own when the CPU
goes down. Why can't it rely on the normal CPU hotplug infrastructure
to do so like all the other drivers (bar some Cavium driver that does
the same thing)?

I'd rather you take this opportunity to move these drivers into the
21st century, so that we can kill irq_cpu_offline() and co altogether.

I wanted to kill these callbacks years ago. Cavium has two variants of
those offline/online callbacks:

 1) octeon_irq_cpu_offline_ciu() which is doing the same as that BCM
    driver. These really can go away. Just remove the callback and
    everything just works.

For BCM this works today when that chip is used on ARM[64] simply
because the only architecture which invokes irq_cpu_offline() is MIPS.

Thanks,

        tglx

On 9/25/2021 2:00 PM, Thomas Gleixner wrote:

On Fri, Sep 24 2021 at 10:05, Florian Fainelli wrote:

quoted

In order to build drivers/irqchip/irq-bcm7038-l1.c as a module (for use
in GKI), we need to export_to_desc() which is used in this snippet of
code:

	irqd_set_single_target(irq_desc_get_irq_data(irq_to_desc(virq)));

This effectively reverts 64a1b95bb9fe ("genirq: Restrict export of
irq_to_desc()").

No. I'm not reexporting this. We've spent quite some time to prevent all
kind of drivers for fiddle with irq descriptors and I'm not going
to reopen that can of worms.

irq_get_irq_data() is exported and provides you what you need.

That is exactly what I was looking for and somehow missed it during my 
search the other day, thanks!
-- 
Florian

On 9/24/2021 10:05 AM, Florian Fainelli wrote:

In order to allow drivers/irqchip/irq-brcmstb-l2.c to be built as a
module we need to export: irq_gc_unmask_enable_reg() and
irq_gc_mask_disable_reg().

Note to self: this needs to come before patch 5 to avoid a modular build 
  linking failure.
-- 
Florian

On 9/25/21 2:37 PM, Thomas Gleixner wrote:

On Sat, Sep 25 2021 at 23:21, Thomas Gleixner wrote:

quoted

On Sat, Sep 25 2021 at 12:48, Marc Zyngier wrote:

quoted

On Fri, 24 Sep 2021 18:05:38 +0100, Florian Fainelli [off-list ref] wrote:

quoted

 }
+EXPORT_SYMBOL_GPL(irq_set_affinity_locked);

This doesn't seem right.

This driver seem to try and move interrupts on its own when the CPU
goes down. Why can't it rely on the normal CPU hotplug infrastructure
to do so like all the other drivers (bar some Cavium driver that does
the same thing)?

I'd rather you take this opportunity to move these drivers into the
21st century, so that we can kill irq_cpu_offline() and co altogether.

I wanted to kill these callbacks years ago. Cavium has two variants of
those offline/online callbacks:

 1) octeon_irq_cpu_offline_ciu() which is doing the same as that BCM
    driver. These really can go away. Just remove the callback and
    everything just works.

For BCM this works today when that chip is used on ARM[64] simply
because the only architecture which invokes irq_cpu_offline() is MIPS.

That is correct. How would you recommend addressing that? In premise
when this driver is used on ARM[64] it is used as a second level
interrupt controller hanging off the ARM GIC (or another ARM CPU
interrupt controller), so in that case I suppose I could make the
irq_set_cpu_offline be dependent upon CONFIG_SMP and CONFIG_MIPS, would
that be acceptable?
-- 
Florian

On Mon, Sep 27 2021 at 10:47, Florian Fainelli wrote:

On 9/25/21 2:37 PM, Thomas Gleixner wrote:

quoted

quoted

I wanted to kill these callbacks years ago. Cavium has two variants of
those offline/online callbacks:

 1) octeon_irq_cpu_offline_ciu() which is doing the same as that BCM
    driver. These really can go away. Just remove the callback and
    everything just works.

For BCM this works today when that chip is used on ARM[64] simply
because the only architecture which invokes irq_cpu_offline() is MIPS.

That is correct. How would you recommend addressing that? In premise
when this driver is used on ARM[64] it is used as a second level
interrupt controller hanging off the ARM GIC (or another ARM CPU
interrupt controller), so in that case I suppose I could make the
irq_set_cpu_offline be dependent upon CONFIG_SMP and CONFIG_MIPS, would
that be acceptable?

Why? Just get rid of the callback in that driver and ensure that
irq_migrate_all_off_this_cpu() is invoked when the CPU dies.

arch/mips/kernel/smp-cps.c already does that, but I don't know whether
your MIPS platform uses those SMP ops. If not you surely have a template
there.

Thanks,

        tglx

On 9/27/21 11:18 AM, Thomas Gleixner wrote:

On Mon, Sep 27 2021 at 10:47, Florian Fainelli wrote:

quoted

On 9/25/21 2:37 PM, Thomas Gleixner wrote:

quoted

quoted

I wanted to kill these callbacks years ago. Cavium has two variants of
those offline/online callbacks:

 1) octeon_irq_cpu_offline_ciu() which is doing the same as that BCM
    driver. These really can go away. Just remove the callback and
    everything just works.

For BCM this works today when that chip is used on ARM[64] simply
because the only architecture which invokes irq_cpu_offline() is MIPS.

That is correct. How would you recommend addressing that? In premise
when this driver is used on ARM[64] it is used as a second level
interrupt controller hanging off the ARM GIC (or another ARM CPU
interrupt controller), so in that case I suppose I could make the
irq_set_cpu_offline be dependent upon CONFIG_SMP and CONFIG_MIPS, would
that be acceptable?

Why? Just get rid of the callback in that driver and ensure that
irq_migrate_all_off_this_cpu() is invoked when the CPU dies.

arch/mips/kernel/smp-cps.c already does that, but I don't know whether
your MIPS platform uses those SMP ops. If not you surely have a template
there.

We use arch/mips/kernel/smp-bmips.c but I do see the path forward, thanks!
-- 
-- 
Florian

On Fri, Sep 24, 2021 at 12:07 PM Florian Fainelli [off-list ref] wrote:

Please convert to use platform_irq_count() instead.

Rob

On 9/27/21 12:08 PM, Rob Herring wrote:

On Fri, Sep 24, 2021 at 12:07 PM Florian Fainelli [off-list ref] wrote:

quoted

In order to build drivers/irqchip/irq-bcm7120-l2.c as a module, we will
need to have of_irq_count() exported to modules.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/of/irq.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 352e14b007e7..949b9d1f8729 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c

@@ -440,6 +440,7 @@ int of_irq_count(struct device_node *dev)

        return nr;
 }
+EXPORT_SYMBOL_GPL(of_irq_count);

Please convert to use platform_irq_count() instead.

That requires a platform_device to be passed to platform_irq_count(),
will that work even when the drivers remain built into the kernel and
get initialized early on?
-- 
Florian

On Fri, Sep 24, 2021 at 12:06 PM Florian Fainelli [off-list ref] wrote:

In order to allow drivers/irqchip/irq-bcm7038-l1.c to be built as a
module and usable in GKI, export cpu_logical_map or __cpu_logical_map
towards the modules.

This is the usage:

#ifdef CONFIG_SMP
        cpu = intc->cpus[cpu_logical_map(smp_processor_id())];
#else
        cpu = intc->cpus[0];
#endif

This is totally broken! cpu_logical_map() takes the logical cpu
number, 0-N, and returns the MPIDR which you then use as an array
index.

Rob

On 9/27/21 12:32 PM, Rob Herring wrote:

On Fri, Sep 24, 2021 at 12:06 PM Florian Fainelli [off-list ref] wrote:

quoted

In order to allow drivers/irqchip/irq-bcm7038-l1.c to be built as a
module and usable in GKI, export cpu_logical_map or __cpu_logical_map
towards the modules.

This is the usage:

#ifdef CONFIG_SMP
        cpu = intc->cpus[cpu_logical_map(smp_processor_id())];
#else
        cpu = intc->cpus[0];
#endif

This is totally broken! cpu_logical_map() takes the logical cpu
number, 0-N, and returns the MPIDR which you then use as an array
index.

There is no MPIDR on MIPS, which is where this code is being primarily
used as-is. On ARM/ARM64 the driver is used as a second level interrupt
controller with only a single "bank" of registers as opposed to one
per-CPU, meaning that we would always use intc->cpus[0] because you
cannot change the interrupt affinity of a second level interrupt
controller AFAICT. Maybe the above deserves to be made CONFIG_SMP &&
CONFIG_MIPS somehow.
-- 
Florian

On Mon, Sep 27, 2021 at 2:28 PM Florian Fainelli [off-list ref] wrote:

On 9/27/21 12:08 PM, Rob Herring wrote:

quoted

On Fri, Sep 24, 2021 at 12:07 PM Florian Fainelli [off-list ref] wrote:

quoted

In order to build drivers/irqchip/irq-bcm7120-l2.c as a module, we will
need to have of_irq_count() exported to modules.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/of/irq.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 352e14b007e7..949b9d1f8729 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c

@@ -440,6 +440,7 @@ int of_irq_count(struct device_node *dev)

        return nr;
 }
+EXPORT_SYMBOL_GPL(of_irq_count);

Please convert to use platform_irq_count() instead.

That requires a platform_device to be passed to platform_irq_count(),
will that work even when the drivers remain built into the kernel and
get initialized early on?

No, does your irqchip using this do both? Looks to me like it is
always a platform_device.

Rob

On 9/27/21 12:43 PM, Rob Herring wrote:

On Mon, Sep 27, 2021 at 2:28 PM Florian Fainelli [off-list ref] wrote:

quoted

On 9/27/21 12:08 PM, Rob Herring wrote:

quoted

On Fri, Sep 24, 2021 at 12:07 PM Florian Fainelli [off-list ref] wrote:

quoted

In order to build drivers/irqchip/irq-bcm7120-l2.c as a module, we will
need to have of_irq_count() exported to modules.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/of/irq.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 352e14b007e7..949b9d1f8729 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c

@@ -440,6 +440,7 @@ int of_irq_count(struct device_node *dev)

        return nr;
 }
+EXPORT_SYMBOL_GPL(of_irq_count);

Please convert to use platform_irq_count() instead.

That requires a platform_device to be passed to platform_irq_count(),
will that work even when the drivers remain built into the kernel and
get initialized early on?

No, does your irqchip using this do both? Looks to me like it is
always a platform_device.

On ARM/ARM64 not using GKI as well as MIPS, we would want the module to
be built into the kernel image, however when using GKI that driver would
become a module. How do you suggest reconciling both usages?
-- 
Florian

On Mon, Sep 27, 2021 at 2:49 PM Florian Fainelli [off-list ref] wrote:

On 9/27/21 12:43 PM, Rob Herring wrote:

quoted

On Mon, Sep 27, 2021 at 2:28 PM Florian Fainelli [off-list ref] wrote:

quoted

On 9/27/21 12:08 PM, Rob Herring wrote:

quoted

On Fri, Sep 24, 2021 at 12:07 PM Florian Fainelli [off-list ref] wrote:

quoted

In order to build drivers/irqchip/irq-bcm7120-l2.c as a module, we will
need to have of_irq_count() exported to modules.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/of/irq.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 352e14b007e7..949b9d1f8729 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c

@@ -440,6 +440,7 @@ int of_irq_count(struct device_node *dev)

        return nr;
 }
+EXPORT_SYMBOL_GPL(of_irq_count);

Please convert to use platform_irq_count() instead.

That requires a platform_device to be passed to platform_irq_count(),
will that work even when the drivers remain built into the kernel and
get initialized early on?

No, does your irqchip using this do both? Looks to me like it is
always a platform_device.

On ARM/ARM64 not using GKI as well as MIPS, we would want the module to
be built into the kernel image, however when using GKI that driver would
become a module. How do you suggest reconciling both usages?

What's there to resolve? Every driver that works as a module can be
built-in. Is there something special about irqchip drivers?

The only issue I see here is platform_irqchip_probe() doesn't pass the
platform_device pointer to the irq_init_cb function. There's 3 ways to
fix that. Add a platform_device pointer to the init hook. That's a
global change though. That's the right thing to do IMO. Or you can use
of_find_device_by_node(). That's fairly expensive, but easy and
isolated. You could also set device_node.data pointer to the
platform_device, but ideally I'd like to get rid of that pointer as
it's hardly used.

Rob

On Mon, 27 Sep 2021 20:49:46 +0100,
Florian Fainelli [off-list ref] wrote:

On 9/27/21 12:43 PM, Rob Herring wrote:

quoted

On Mon, Sep 27, 2021 at 2:28 PM Florian Fainelli [off-list ref] wrote:

quoted

On 9/27/21 12:08 PM, Rob Herring wrote:

quoted

On Fri, Sep 24, 2021 at 12:07 PM Florian Fainelli [off-list ref] wrote:

quoted

In order to build drivers/irqchip/irq-bcm7120-l2.c as a module, we will
need to have of_irq_count() exported to modules.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/of/irq.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 352e14b007e7..949b9d1f8729 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c

@@ -440,6 +440,7 @@ int of_irq_count(struct device_node *dev)

        return nr;
 }
+EXPORT_SYMBOL_GPL(of_irq_count);

Please convert to use platform_irq_count() instead.

That requires a platform_device to be passed to platform_irq_count(),
will that work even when the drivers remain built into the kernel and
get initialized early on?

No, does your irqchip using this do both? Looks to me like it is
always a platform_device.

On ARM/ARM64 not using GKI as well as MIPS, we would want the module to
be built into the kernel image, however when using GKI that driver would
become a module. How do you suggest reconciling both usages?

I don't see what GKI has to do with anything. Either the driver can be
built as a module (and it is in this case a platform device at all
times, built-in or not), or it cannot, and it falls into the
IRQCHIP_DECLARE() category (and there is no export problem).

Pick your poison!

Thanks,

	M.

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