Here's the series for irqchip-bcm283x on arm64 for the Pi3. Since I'd
like to land Makefile changes that would enable building
irqchip-bcm283x on arm64, it would be nice if I had either a stable
branch to merge from, or an ack to merge them through the arm64 tree.
Eric Anholt (4):
irqchip: bcm2835: Avoid arch/arm-specific handle_IRQ
irqchip: bcm2836: Drop smp_set_ops on arm64 builds
irqchip: bcm2836: Fix compiler warning on 64-bit build
irqchip: bcm2836: Use a more generic memory barrier call
drivers/irqchip/irq-bcm2835.c | 3 +--
drivers/irqchip/irq-bcm2836.c | 12 ++++++++----
2 files changed, 9 insertions(+), 6 deletions(-)
--
2.7.0
This is equivalent and works for arm64 as well.
Signed-off-by: Eric Anholt <[email protected]>
---
drivers/irqchip/irq-bcm2835.c | 3 +--
drivers/irqchip/irq-bcm2836.c | 2 +-
2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/irqchip/irq-bcm2835.c b/drivers/irqchip/irq-bcm2835.c
index bf9cc5f..44d7c38 100644
--- a/drivers/irqchip/irq-bcm2835.c
+++ b/drivers/irqchip/irq-bcm2835.c
@@ -52,7 +52,6 @@
#include <linux/irqdomain.h>
#include <asm/exception.h>
-#include <asm/mach/irq.h>
/* Put the bank and irq (32 bits) into the hwirq */
#define MAKE_HWIRQ(b, n) ((b << 5) | (n))
@@ -242,7 +241,7 @@ static void __exception_irq_entry bcm2835_handle_irq(
u32 hwirq;
while ((hwirq = get_next_armctrl_hwirq()) != ~0)
- handle_IRQ(irq_linear_revmap(intc.domain, hwirq), regs);
+ handle_domain_irq(intc.domain, hwirq, regs);
}
static void bcm2836_chained_handle_irq(struct irq_desc *desc)
diff --git a/drivers/irqchip/irq-bcm2836.c b/drivers/irqchip/irq-bcm2836.c
index b6e950d..233ccdd 100644
--- a/drivers/irqchip/irq-bcm2836.c
+++ b/drivers/irqchip/irq-bcm2836.c
@@ -180,7 +180,7 @@ __exception_irq_entry bcm2836_arm_irqchip_handle_irq(struct pt_regs *regs)
} else if (stat) {
u32 hwirq = ffs(stat) - 1;
- handle_IRQ(irq_linear_revmap(intc.domain, hwirq), regs);
+ handle_domain_irq(intc.domain, hwirq, regs);
}
}
--
2.7.0
Signed-off-by: Eric Anholt <[email protected]>
---
drivers/irqchip/irq-bcm2836.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/irqchip/irq-bcm2836.c b/drivers/irqchip/irq-bcm2836.c
index 4ae9f76..ee62413 100644
--- a/drivers/irqchip/irq-bcm2836.c
+++ b/drivers/irqchip/irq-bcm2836.c
@@ -253,7 +253,7 @@ bcm2836_arm_irqchip_smp_init(void)
/* Unmask IPIs to the boot CPU. */
bcm2836_arm_irqchip_cpu_notify(&bcm2836_arm_irqchip_cpu_notifier,
CPU_STARTING,
- (void *)smp_processor_id());
+ (void *)(uintptr_t)smp_processor_id());
register_cpu_notifier(&bcm2836_arm_irqchip_cpu_notifier);
set_smp_cross_call(bcm2836_arm_irqchip_send_ipi);
--
2.7.0
For arm64, the bootloader will instead be implementing the spin-table
enable method.
Signed-off-by: Eric Anholt <[email protected]>
---
drivers/irqchip/irq-bcm2836.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/irqchip/irq-bcm2836.c b/drivers/irqchip/irq-bcm2836.c
index 233ccdd..4ae9f76 100644
--- a/drivers/irqchip/irq-bcm2836.c
+++ b/drivers/irqchip/irq-bcm2836.c
@@ -223,6 +223,7 @@ static struct notifier_block bcm2836_arm_irqchip_cpu_notifier = {
.priority = 100,
};
+#ifdef ARM
int __init bcm2836_smp_boot_secondary(unsigned int cpu,
struct task_struct *idle)
{
@@ -238,7 +239,7 @@ int __init bcm2836_smp_boot_secondary(unsigned int cpu,
static const struct smp_operations bcm2836_smp_ops __initconst = {
.smp_boot_secondary = bcm2836_smp_boot_secondary,
};
-
+#endif
#endif
static const struct irq_domain_ops bcm2836_arm_irqchip_intc_ops = {
@@ -256,8 +257,11 @@ bcm2836_arm_irqchip_smp_init(void)
register_cpu_notifier(&bcm2836_arm_irqchip_cpu_notifier);
set_smp_cross_call(bcm2836_arm_irqchip_send_ipi);
+
+#ifdef ARM
smp_set_ops(&bcm2836_smp_ops);
#endif
+#endif
}
/*
--
2.7.0
dsb() requires an argument on arm64, so we needed to add "sy".
Instead, take this opportunity to switch to the same smp_wmb() call
that gic uses for its IPIs. This is a less strong barrier than we
were doing before (dmb(ishst) compared to dsb(sy)), but it seems to be
the correct one.
Signed-off-by: Eric Anholt <[email protected]>
---
drivers/irqchip/irq-bcm2836.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/irqchip/irq-bcm2836.c b/drivers/irqchip/irq-bcm2836.c
index ee62413..a99b630 100644
--- a/drivers/irqchip/irq-bcm2836.c
+++ b/drivers/irqchip/irq-bcm2836.c
@@ -195,7 +195,7 @@ static void bcm2836_arm_irqchip_send_ipi(const struct cpumask *mask,
* Ensure that stores to normal memory are visible to the
* other CPUs before issuing the IPI.
*/
- dsb();
+ smp_wmb();
for_each_cpu(cpu, mask) {
writel(1 << ipi, mailbox0_base + 16 * cpu);
--
2.7.0
Eric,
On 05/04/16 04:44, Eric Anholt wrote:
> For arm64, the bootloader will instead be implementing the spin-table
> enable method.
You may also want to add that SMP ops simply do not exist on arm64,
hence the bootloader [...].
>
> Signed-off-by: Eric Anholt <[email protected]>
> ---
> drivers/irqchip/irq-bcm2836.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/irqchip/irq-bcm2836.c b/drivers/irqchip/irq-bcm2836.c
> index 233ccdd..4ae9f76 100644
> --- a/drivers/irqchip/irq-bcm2836.c
> +++ b/drivers/irqchip/irq-bcm2836.c
> @@ -223,6 +223,7 @@ static struct notifier_block bcm2836_arm_irqchip_cpu_notifier = {
> .priority = 100,
> };
>
> +#ifdef ARM
That should really be CONFIG_ARM.
> int __init bcm2836_smp_boot_secondary(unsigned int cpu,
> struct task_struct *idle)
> {
> @@ -238,7 +239,7 @@ int __init bcm2836_smp_boot_secondary(unsigned int cpu,
> static const struct smp_operations bcm2836_smp_ops __initconst = {
> .smp_boot_secondary = bcm2836_smp_boot_secondary,
> };
> -
> +#endif
> #endif
>
> static const struct irq_domain_ops bcm2836_arm_irqchip_intc_ops = {
> @@ -256,8 +257,11 @@ bcm2836_arm_irqchip_smp_init(void)
> register_cpu_notifier(&bcm2836_arm_irqchip_cpu_notifier);
>
> set_smp_cross_call(bcm2836_arm_irqchip_send_ipi);
> +
> +#ifdef ARM
> smp_set_ops(&bcm2836_smp_ops);
> #endif
> +#endif
> }
>
> /*
>
Thanks,
M.
--
Jazz is not dead. It just smells funny...
On 04/04/2016 09:44 PM, Eric Anholt wrote:
> dsb() requires an argument on arm64, so we needed to add "sy".
> Instead, take this opportunity to switch to the same smp_wmb() call
> that gic uses for its IPIs. This is a less strong barrier than we
> were doing before (dmb(ishst) compared to dsb(sy)), but it seems to be
> the correct one.
I assume all MMIO is part of the ish domain?
If so, the series,
Acked-by: Stephen Warren <[email protected]>
Stephen Warren <[email protected]> writes:
> On 04/04/2016 09:44 PM, Eric Anholt wrote:
>> dsb() requires an argument on arm64, so we needed to add "sy".
>> Instead, take this opportunity to switch to the same smp_wmb() call
>> that gic uses for its IPIs. This is a less strong barrier than we
>> were doing before (dmb(ishst) compared to dsb(sy)), but it seems to be
>> the correct one.
>
> I assume all MMIO is part of the ish domain?
>
> If so, the series,
> Acked-by: Stephen Warren <[email protected]>
I don't know if this barrier implies ordering all the way out to AXI on
this HW, but I don't think that's a requirement of this function.
On Tuesday 05 April 2016, Eric Anholt wrote:
> For arm64, the bootloader will instead be implementing the spin-table
> enable method.
>
> Signed-off-by: Eric Anholt <[email protected]>
> ---
> drivers/irqchip/irq-bcm2836.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/irqchip/irq-bcm2836.c b/drivers/irqchip/irq-bcm2836.c
> index 233ccdd..4ae9f76 100644
> --- a/drivers/irqchip/irq-bcm2836.c
> +++ b/drivers/irqchip/irq-bcm2836.c
> @@ -223,6 +223,7 @@ static struct notifier_block bcm2836_arm_irqchip_cpu_notifier = {
> .priority = 100,
> };
>
> +#ifdef ARM
> int __init bcm2836_smp_boot_secondary(unsigned int cpu,
> struct task_struct *idle)
> {
> @@ -238,7 +239,7 @@ int __init bcm2836_smp_boot_secondary(unsigned int cpu,
> static const struct smp_operations bcm2836_smp_ops __initconst = {
> .smp_boot_secondary = bcm2836_smp_boot_secondary,
> };
> -
> +#endif
> #endif
>
> static const struct irq_domain_ops bcm2836_arm_irqchip_intc_ops = {
> @@ -256,8 +257,11 @@ bcm2836_arm_irqchip_smp_init(void)
> register_cpu_notifier(&bcm2836_arm_irqchip_cpu_notifier);
>
> set_smp_cross_call(bcm2836_arm_irqchip_send_ipi);
> +
> +#ifdef ARM
> smp_set_ops(&bcm2836_smp_ops);
> #endif
> +#endif
> }
I'd suggest instead using CPU_METHOD_OF_DECLARE and moving the SMP code
to arch/arm/mach-bcm/platsmp-bcm2835.c. It doesn't really belong in the
irqchip code.
Arnd
On 04/08/2016 12:20 PM, Eric Anholt wrote:
> Stephen Warren <[email protected]> writes:
>
>> On 04/04/2016 09:44 PM, Eric Anholt wrote:
>>> dsb() requires an argument on arm64, so we needed to add "sy".
>>> Instead, take this opportunity to switch to the same smp_wmb() call
>>> that gic uses for its IPIs. This is a less strong barrier than we
>>> were doing before (dmb(ishst) compared to dsb(sy)), but it seems to be
>>> the correct one.
>>
>> I assume all MMIO is part of the ish domain?
>>
>> If so, the series,
>> Acked-by: Stephen Warren <[email protected]>
>
> I don't know if this barrier implies ordering all the way out to AXI on
> this HW, but I don't think that's a requirement of this function.
My understanding was that the barrier was explicitly to work around a
bug in the bus fabric of the SoC, and hence the barrier very much does
have to affect the transaction all the way out to AXI. Re-reading
BCM2835-ARM-Peripherals.pdf section 1.3 "Peripheral access precautions
for correct memory ordering" seems to confirm this.
Stephen Warren <[email protected]> writes:
> On 04/08/2016 12:20 PM, Eric Anholt wrote:
>> Stephen Warren <[email protected]> writes:
>>
>>> On 04/04/2016 09:44 PM, Eric Anholt wrote:
>>>> dsb() requires an argument on arm64, so we needed to add "sy".
>>>> Instead, take this opportunity to switch to the same smp_wmb() call
>>>> that gic uses for its IPIs. This is a less strong barrier than we
>>>> were doing before (dmb(ishst) compared to dsb(sy)), but it seems to be
>>>> the correct one.
>>>
>>> I assume all MMIO is part of the ish domain?
>>>
>>> If so, the series,
>>> Acked-by: Stephen Warren <[email protected]>
>>
>> I don't know if this barrier implies ordering all the way out to AXI on
>> this HW, but I don't think that's a requirement of this function.
>
> My understanding was that the barrier was explicitly to work around a
> bug in the bus fabric of the SoC, and hence the barrier very much does
> have to affect the transaction all the way out to AXI. Re-reading
> BCM2835-ARM-Peripherals.pdf section 1.3 "Peripheral access precautions
> for correct memory ordering" seems to confirm this.
My understanding of the explicit barrier here, which was copied from
other irqchips, is "Make sure that normal memory writes before our IPI
on this CPU appear on the other CPUs before they get the IPI" (like the
comment says). This barrier was not put in to deal with the
283x-specific weird AXI behavior.
Note that we had previously decided that the weird AXI ordering
behavior, which is about repeated reads or repeated writes from the same
CPU across different peripherals, is already covered by the barriers
present in readl() and writel(). The writel() barrier happens to be a
dsb() as well, so this explicit barrier is actually redundant.
On 04/10/2016 12:32 PM, Eric Anholt wrote:
> Stephen Warren <[email protected]> writes:
>
>> On 04/08/2016 12:20 PM, Eric Anholt wrote:
>>> Stephen Warren <[email protected]> writes:
>>>
>>>> On 04/04/2016 09:44 PM, Eric Anholt wrote:
>>>>> dsb() requires an argument on arm64, so we needed to add "sy".
>>>>> Instead, take this opportunity to switch to the same smp_wmb() call
>>>>> that gic uses for its IPIs. This is a less strong barrier than we
>>>>> were doing before (dmb(ishst) compared to dsb(sy)), but it seems to be
>>>>> the correct one.
>>>>
>>>> I assume all MMIO is part of the ish domain?
>>>>
>>>> If so, the series,
>>>> Acked-by: Stephen Warren <[email protected]>
>>>
>>> I don't know if this barrier implies ordering all the way out to AXI on
>>> this HW, but I don't think that's a requirement of this function.
>>
>> My understanding was that the barrier was explicitly to work around a
>> bug in the bus fabric of the SoC, and hence the barrier very much does
>> have to affect the transaction all the way out to AXI. Re-reading
>> BCM2835-ARM-Peripherals.pdf section 1.3 "Peripheral access precautions
>> for correct memory ordering" seems to confirm this.
>
> My understanding of the explicit barrier here, which was copied from
> other irqchips, is "Make sure that normal memory writes before our IPI
> on this CPU appear on the other CPUs before they get the IPI" (like the
> comment says). This barrier was not put in to deal with the
> 283x-specific weird AXI behavior.
>
> Note that we had previously decided that the weird AXI ordering
> behavior, which is about repeated reads or repeated writes from the same
> CPU across different peripherals, is already covered by the barriers
> present in readl() and writel(). The writel() barrier happens to be a
> dsb() as well, so this explicit barrier is actually redundant.
Ah OK. In that case, the change seems fine.
Arnd Bergmann <[email protected]> writes:
> On Tuesday 05 April 2016, Eric Anholt wrote:
>> For arm64, the bootloader will instead be implementing the spin-table
>> enable method.
>>
>> Signed-off-by: Eric Anholt <[email protected]>
>> ---
>> drivers/irqchip/irq-bcm2836.c | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/irqchip/irq-bcm2836.c b/drivers/irqchip/irq-bcm2836.c
>> index 233ccdd..4ae9f76 100644
>> --- a/drivers/irqchip/irq-bcm2836.c
>> +++ b/drivers/irqchip/irq-bcm2836.c
>> @@ -223,6 +223,7 @@ static struct notifier_block bcm2836_arm_irqchip_cpu_notifier = {
>> .priority = 100,
>> };
>>
>> +#ifdef ARM
>> int __init bcm2836_smp_boot_secondary(unsigned int cpu,
>> struct task_struct *idle)
>> {
>> @@ -238,7 +239,7 @@ int __init bcm2836_smp_boot_secondary(unsigned int cpu,
>> static const struct smp_operations bcm2836_smp_ops __initconst = {
>> .smp_boot_secondary = bcm2836_smp_boot_secondary,
>> };
>> -
>> +#endif
>> #endif
>>
>> static const struct irq_domain_ops bcm2836_arm_irqchip_intc_ops = {
>> @@ -256,8 +257,11 @@ bcm2836_arm_irqchip_smp_init(void)
>> register_cpu_notifier(&bcm2836_arm_irqchip_cpu_notifier);
>>
>> set_smp_cross_call(bcm2836_arm_irqchip_send_ipi);
>> +
>> +#ifdef ARM
>> smp_set_ops(&bcm2836_smp_ops);
>> #endif
>> +#endif
>> }
>
> I'd suggest instead using CPU_METHOD_OF_DECLARE and moving the SMP code
> to arch/arm/mach-bcm/platsmp-bcm2835.c. It doesn't really belong in the
> irqchip code.
I think because of DT ABI (sigh) we're stuck with the current
implementation.
FWIW, I had started with the SMP bits in a platsmp.c, but moved it into
the irqchip because that was so much simpler than reaching over into the
irqchip node to map its registers.