[PATCH 00/11] Modular Broadcom irqchip drivers

[PATCH 00/11] Modular Broadcom irqchip drivers

[Florian Fainelli]

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

[PATCH 01/11] arch: Export cpu_logical_map to modules

[Florian Fainelli]

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

Re: [PATCH 01/11] arch: Export cpu_logical_map to modules

[Rob Herring]

On Fri, Sep 24, 2021 at 12:06 PM Florian Fainelli <f.fainelli@gmail.com> 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

Re: [PATCH 01/11] arch: Export cpu_logical_map to modules

[Florian Fainelli]

On 9/27/21 12:32 PM, Rob Herring wrote:
> On Fri, Sep 24, 2021 at 12:06 PM Florian Fainelli <f.fainelli@gmail.com> 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.

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

[PATCH 02/11] genirq: Export irq_to_desc() again to modules

[Florian Fainelli]

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

Re: [PATCH 02/11] genirq: Export irq_to_desc() again to modules

[Thomas Gleixner]

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

Re: [PATCH 02/11] genirq: Export irq_to_desc() again to modules

[Florian Fainelli]

On 9/25/2021 2:00 PM, Thomas Gleixner wrote:
> 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.

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

[PATCH 03/11] genirq: Export irq_set_affinity_locked()

[Florian Fainelli]

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

Re: [PATCH 03/11] genirq: Export irq_set_affinity_locked()

[Marc Zyngier]

On Fri, 24 Sep 2021 18:05:38 +0100,
Florian Fainelli <f.fainelli@gmail.com> wrote:
> 
> 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

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.

Re: [PATCH 03/11] genirq: Export irq_set_affinity_locked()

[Thomas Gleixner]

On Sat, Sep 25 2021 at 12:48, Marc Zyngier wrote:
> On Fri, 24 Sep 2021 18:05:38 +0100, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>  }
>> +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



    

Re: [PATCH 03/11] genirq: Export irq_set_affinity_locked()

[Thomas Gleixner]

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

> On Sat, Sep 25 2021 at 12:48, Marc Zyngier wrote:
>> On Fri, 24 Sep 2021 18:05:38 +0100, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>  }
>>> +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

Re: [PATCH 03/11] genirq: Export irq_set_affinity_locked()

[Florian Fainelli]

On 9/25/21 2:37 PM, Thomas Gleixner wrote:
> On Sat, Sep 25 2021 at 23:21, Thomas Gleixner wrote:
> 
>> On Sat, Sep 25 2021 at 12:48, Marc Zyngier wrote:
>>> On Fri, 24 Sep 2021 18:05:38 +0100, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>>  }
>>>> +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

Re: [PATCH 03/11] genirq: Export irq_set_affinity_locked()

[Thomas Gleixner]

On Mon, Sep 27 2021 at 10:47, Florian Fainelli wrote:
> On 9/25/21 2:37 PM, Thomas Gleixner wrote:
>>> 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

Re: [PATCH 03/11] genirq: Export irq_set_affinity_locked()

[Florian Fainelli]

On 9/27/21 11:18 AM, Thomas Gleixner wrote:
> On Mon, Sep 27 2021 at 10:47, Florian Fainelli wrote:
>> On 9/25/21 2:37 PM, Thomas Gleixner wrote:
>>>> 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

[PATCH 04/11] irqchip/irq-bcm7038-l1: Switch to IRQCHIP_PLATFORM_DRIVER

[Florian Fainelli]

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

[PATCH 05/11] irqchip/irq-brcmstb-l2: Switch to IRQCHIP_PLATFORM_DRIVER

[Florian Fainelli]

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

[PATCH 06/11] genirq: Export irq_gc_{unmask_enable,mask_disable}_reg

[Florian Fainelli]

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

Re: [PATCH 06/11] genirq: Export irq_gc_{unmask_enable,mask_disable}_reg

[Florian Fainelli]

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

[PATCH 07/11] of/irq: Export of_irq_count to drivers

[Florian Fainelli]

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

Re: [PATCH 07/11] of/irq: Export of_irq_count to drivers

[Rob Herring]

On Fri, Sep 24, 2021 at 12:07 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>
> 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.

Rob

Re: [PATCH 07/11] of/irq: Export of_irq_count to drivers

[Florian Fainelli]

On 9/27/21 12:08 PM, Rob Herring wrote:
> On Fri, Sep 24, 2021 at 12:07 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>>
>> 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

Re: [PATCH 07/11] of/irq: Export of_irq_count to drivers

[Rob Herring]

On Mon, Sep 27, 2021 at 2:28 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>
> On 9/27/21 12:08 PM, Rob Herring wrote:
> > On Fri, Sep 24, 2021 at 12:07 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
> >>
> >> 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

Re: [PATCH 07/11] of/irq: Export of_irq_count to drivers

[Florian Fainelli]

On 9/27/21 12:43 PM, Rob Herring wrote:
> On Mon, Sep 27, 2021 at 2:28 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>>
>> On 9/27/21 12:08 PM, Rob Herring wrote:
>>> On Fri, Sep 24, 2021 at 12:07 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>>
>>>> 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

Re: [PATCH 07/11] of/irq: Export of_irq_count to drivers

[Rob Herring]

On Mon, Sep 27, 2021 at 2:49 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>
> On 9/27/21 12:43 PM, Rob Herring wrote:
> > On Mon, Sep 27, 2021 at 2:28 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
> >>
> >> On 9/27/21 12:08 PM, Rob Herring wrote:
> >>> On Fri, Sep 24, 2021 at 12:07 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
> >>>>
> >>>> 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

Re: [PATCH 07/11] of/irq: Export of_irq_count to drivers

[Marc Zyngier]

On Mon, 27 Sep 2021 20:49:46 +0100,
Florian Fainelli <f.fainelli@gmail.com> wrote:
> 
> On 9/27/21 12:43 PM, Rob Herring wrote:
> > On Mon, Sep 27, 2021 at 2:28 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
> >>
> >> On 9/27/21 12:08 PM, Rob Herring wrote:
> >>> On Fri, Sep 24, 2021 at 12:07 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
> >>>>
> >>>> 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.

[PATCH 08/11] genirq: Export irq_gc_noop()

[Florian Fainelli]

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

[PATCH 09/11] irqchip/irq-bcm7120-l2: Switch to IRQCHIP_PLATFORM_DRIVER

[Florian Fainelli]

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

[PATCH 10/11] arm64: broadcom: Removed forced select of interrupt controllers

[Florian Fainelli]

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

Re: [PATCH 10/11] arm64: broadcom: Removed forced select of interrupt controllers

[Marc Zyngier]

On Fri, 24 Sep 2021 18:05:45 +0100,
Florian Fainelli <f.fainelli@gmail.com> wrote:
> 
> 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

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.

Re: [PATCH 10/11] arm64: broadcom: Removed forced select of interrupt controllers

[Arnd Bergmann]

On Sat, Sep 25, 2021 at 2:10 PM Marc Zyngier <maz@kernel.org> wrote:
> On Fri, 24 Sep 2021 18:05:45 +0100, Florian Fainelli <f.fainelli@gmail.com> 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

[PATCH 11/11] ARM: bcm: Removed forced select of interrupt controllers

[Florian Fainelli]

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