Re: [PATCH 1/9] irqchip/sun6i-r: Switch to a stacked irqchip driver

[Samuel Holland]

Hello, and thanks for the feedback!

I know this is quite the delay in responding; I wanted to make sure my
understanding of the hardware was as clear as possible before sending a v2.

After experimentation, I came up with a diagram describing the hardware
architecture, available here:
https://linux-sunxi.org/images/5/5c/R_INTC.png (PNG)
https://sholland.org/files/R_INTC_v2.svg (SVG)

Based on that, your feedback, and similar examples like the great explanation of
the robustness requirements in 6dd859508336 ("gpio: zynq: Fix IRQ handlers"), I
think v2 will work properly for both edge and level interrupts. I tested both
triggers, albeit with the same source of (level) interrupts connected to the NMI
pin.

On 1/20/20 4:52 AM, Marc Zyngier wrote:
> Hi Samuel,
> 
> On 2020-01-13 05:49, Samuel Holland wrote:
>> The R_INTC in the A31 and newer sun8i/sun50i SoCs is more similar to the
>> original sun4i interrupt controller than the sun7i/sun9i NMI controller.
>> It is used for two distinct purposes:
>> Â1) To control the trigger and mask for the NMI input pin
>> Â2) To provide the interrupt input for the ARISC coprocessor
>>
>> As this interrupt controller is not documented, information about it
>> comes from reverse-engineering the BSP-provided ARISC firmware.
>>
>> Like the original sun4i interrupt controller, it has:
>> Â- A VECTOR_REG at 0x00 (configurable via the BASE_ADDR_REG at 0x04)
>> Â- A NMI_CTRL_REG, PENDING_REG, and ENABLE_REG as used by both the
>> ÂÂ sun4i and sunxi-nmi drivers
>> Â- A MASK_REG at 0x50
>>
>> Differences from the sun4i interrupt controller appear to be:
>> Â- It is only known to have one register of each kind (max 32 inputs)
>> Â- There is no FIQ-related logic
>> Â- There is no interrupt priority logic
>>
>> In order to fulfill its two purposes, this hardware block combines two
>> types of IRQs. First, the NMI pin is routed to the "IRQ 0" input on this
>> chip, with a trigger type controlled by the NMI_CTRL_REG. The (masked)
>> "IRQ 0 pending" output from this chip is then routed to a non-maskable
>> SPI IRQ input on the GIC, as IRQ_TYPE_LEVEL_HIGH. In other words, bit 0
> 
> I object to the "non-maskable" wording here. It may be non-maskable
> at this irqchip level (and yet you seem to have code to that effect),
> but the GIC definitely should be able to mask things.

You're 100% correct here. I had thought IRQ 0 was non-maskable because the MASK
register didn't affect the IRQ being sent to the GIC. Disabling the IRQ via
GICD_ICENABLER does indeed work.

>> of ENABLE_REG and MASK_REG *do* affect the IRQs seen at the GIC.
>>
>> The NMI is then followed by a contiguous block of (at least) 15 IRQ
>> inputs that are connected *in parallel* to both R_INTC and the GIC. Or
>> in other words, the other bits of ENABLE_REG and MASK_REG *do not*
>> affect the IRQs seen at the GIC.
>>
>> Finally, the global "IRQ pending" output from R_INTC is connected to the
>> "external interrupt" input of the ARISC CPU (an OR1200).
>>
>> Because of the 1:1 correspondence between R_INTC and GIC inputs, this is
>> a perfect scenario for using a stacked irqchip driver. We want to hook
>> into enabling/disabling/masking IRQs to add more features to the GIC
>> (specifically to allow masking the NMI and setting its trigger type),
>> but we don't need to actually *handle* the IRQ.
>>
>> And since R_INTC is in the always-on power domain, and its output is
>> connected directly in to the power management coprocessor, a stacked
>> irqchip driver provides a simple way to add wakeup support to this set
>> of IRQs. That is a future patch; for now, just the NMI is moved over.
>>
>> This driver keeps the same DT binding as the existing driver. The
>> "interrupt" property of the R_INTC node is used to determine 1) the
>> offset between GIC and R_INTC hwirq numbers and 2) the type of trigger
>> between the R_INTC "IRQ 0 pending" output and the GIC NMI input.
>>
>> This commit mostly reverts commit 173bda53b340 ("irqchip/sunxi-nmi:
>> Support sun6i-a31-r-intc compatible").
>>
>> Signed-off-by: Samuel Holland <samuel@xxxxxxxxxxxx>
>> ---
>> Âarch/arm/mach-sunxi/KconfigÂÂÂÂ |ÂÂ 1 +
>> Âarch/arm64/Kconfig.platformsÂÂÂ |ÂÂ 1 +
>> Âdrivers/irqchip/MakefileÂÂÂÂÂÂÂ |ÂÂ 1 +
>> Âdrivers/irqchip/irq-sun6i-r.cÂÂ | 220 ++++++++++++++++++++++++++++++++
>> Âdrivers/irqchip/irq-sunxi-nmi.c |Â 26 +---
>> Â5 files changed, 226 insertions(+), 23 deletions(-)
>> Âcreate mode 100644 drivers/irqchip/irq-sun6i-r.c
>>
>> diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig
>> index eeadb1a4dcfe..ef1cc25902b5 100644
>> --- a/arch/arm/mach-sunxi/Kconfig
>> +++ b/arch/arm/mach-sunxi/Kconfig
>> @@ -6,6 +6,7 @@ menuconfig ARCH_SUNXI
>> ÂÂÂÂ select CLKSRC_MMIO
>> ÂÂÂÂ select GENERIC_IRQ_CHIP
>> ÂÂÂÂ select GPIOLIB
>> +ÂÂÂ select IRQ_DOMAIN_HIERARCHY
>> ÂÂÂÂ select PINCTRL
>> ÂÂÂÂ select PM_OPP
>> ÂÂÂÂ select SUN4I_TIMER
>> diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
>> index 16d761475a86..d282d0a1d17d 100644
>> --- a/arch/arm64/Kconfig.platforms
>> +++ b/arch/arm64/Kconfig.platforms
>> @@ -17,6 +17,7 @@ config ARCH_SUNXI
>> ÂÂÂÂ bool "Allwinner sunxi 64-bit SoC Family"
>> ÂÂÂÂ select ARCH_HAS_RESET_CONTROLLER
>> ÂÂÂÂ select GENERIC_IRQ_CHIP
>> +ÂÂÂ select IRQ_DOMAIN_HIERARCHY
>> ÂÂÂÂ select PINCTRL
>> ÂÂÂÂ select RESET_CONTROLLER
>> ÂÂÂÂ help
>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>> index cc7c43932f16..41996d98c30a 100644
>> --- a/drivers/irqchip/Makefile
>> +++ b/drivers/irqchip/Makefile
>> @@ -24,6 +24,7 @@ obj-$(CONFIG_OR1K_PIC)ÂÂÂÂÂÂÂÂÂÂÂ += irq-or1k-pic.o
>> Âobj-$(CONFIG_ORION_IRQCHIP)ÂÂÂÂÂÂÂ += irq-orion.o
>> Âobj-$(CONFIG_OMAP_IRQCHIP)ÂÂÂÂÂÂÂ += irq-omap-intc.o
>> Âobj-$(CONFIG_ARCH_SUNXI)ÂÂÂÂÂÂÂ += irq-sun4i.o
>> +obj-$(CONFIG_ARCH_SUNXI)ÂÂÂÂÂÂÂ += irq-sun6i-r.o
>> Âobj-$(CONFIG_ARCH_SUNXI)ÂÂÂÂÂÂÂ += irq-sunxi-nmi.o
>> Âobj-$(CONFIG_ARCH_SPEAR3XX)ÂÂÂÂÂÂÂ += spear-shirq.o
>> Âobj-$(CONFIG_ARM_GIC)ÂÂÂÂÂÂÂÂÂÂÂ += irq-gic.o irq-gic-common.o
>> diff --git a/drivers/irqchip/irq-sun6i-r.c b/drivers/irqchip/irq-sun6i-r.c
>> new file mode 100644
>> index 000000000000..37b6e9c60bf8
>> --- /dev/null
>> +++ b/drivers/irqchip/irq-sun6i-r.c
>> @@ -0,0 +1,220 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +//
>> +// Allwinner A31 and newer SoCs R_INTC driver
>> +//
>> +
>> +#include <linux/irq.h>
>> +#include <linux/irqchip.h>
>> +#include <linux/irqdomain.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_irq.h>
>> +
>> +#include <dt-bindings/interrupt-controller/arm-gic.h>
>> +
>> +#define NMI_HWIRQÂÂÂÂÂÂÂ 0
>> +
>> +#define SUN6I_R_INTC_NR_IRQSÂÂÂ 16
>> +
>> +#define SUN6I_R_INTC_CTRLÂÂÂ 0x0c
>> +#define SUN6I_R_INTC_PENDINGÂÂÂ 0x10
>> +#define SUN6I_R_INTC_ENABLEÂÂÂ 0x40
>> +#define SUN6I_R_INTC_MASKÂÂÂ 0x50
>> +
>> +enum {
>> +ÂÂÂ SUNXI_SRC_TYPE_LEVEL_LOW = 0,
>> +ÂÂÂ SUNXI_SRC_TYPE_EDGE_FALLING,
>> +ÂÂÂ SUNXI_SRC_TYPE_LEVEL_HIGH,
>> +ÂÂÂ SUNXI_SRC_TYPE_EDGE_RISING,
>> +};
> 
> It is unusual to use an enum for values that get directly programmed
> into the HW.

These definitons match the existing driver this was split from. I will inline
these for v2.

>> +
>> +static void __iomem *base;
>> +static irq_hw_number_t parent_offset;
>> +static u32 parent_type;
>> +
>> +static void sun6i_r_intc_irq_enable(struct irq_data *data)
>> +{
>> +ÂÂÂ if (data->hwirq == NMI_HWIRQ)
>> +ÂÂÂÂÂÂÂ writel(BIT(NMI_HWIRQ), base + SUN6I_R_INTC_ENABLE);
> 
> Please consider using _relaxed() accessors.

I've done this for v2.

>> +
>> +ÂÂÂ irq_chip_enable_parent(data);
>> +}
>> +
>> +static void sun6i_r_intc_irq_disable(struct irq_data *data)
>> +{
>> +ÂÂÂ if (data->hwirq == NMI_HWIRQ)
>> +ÂÂÂÂÂÂÂ writel(0, base + SUN6I_R_INTC_ENABLE);
>> +
>> +ÂÂÂ irq_chip_disable_parent(data);
>> +}
>> +
>> +static void sun6i_r_intc_irq_mask(struct irq_data *data)
>> +{
>> +ÂÂÂ if (data->hwirq == NMI_HWIRQ)
>> +ÂÂÂÂÂÂÂ writel(BIT(NMI_HWIRQ), base + SUN6I_R_INTC_MASK);
>> +
>> +ÂÂÂ irq_chip_mask_parent(data);
>> +}
>> +
>> +static void sun6i_r_intc_irq_unmask(struct irq_data *data)
>> +{
>> +ÂÂÂ if (data->hwirq == NMI_HWIRQ)
>> +ÂÂÂÂÂÂÂ writel(0, base + SUN6I_R_INTC_MASK);
>> +
>> +ÂÂÂ irq_chip_unmask_parent(data);
>> +}
>> +
>> +static void sun6i_r_intc_irq_eoi(struct irq_data *data)
>> +{
>> +ÂÂÂ if (data->hwirq == NMI_HWIRQ)
>> +ÂÂÂÂÂÂÂ writel(BIT(NMI_HWIRQ), base + SUN6I_R_INTC_PENDING);
> 
> Are you sure about this? Clearing the pending bit is not quite an EOI.
> It won't hurt a level interrupt, but could be pretty deadly with
> edge signaling (you'd loose that interrupt). But does this register
> actually latch the input until you clear it? Or does it follow the
> level of its input?

For bit 0 (the only one Linux cares about), there is a latch. This latch gets
set whenever the IRQ is triggered (once for edge, continuously for level), and
it gets reset by writing 1 to PENDING.

What I've done for v2 is set this bit in .irq_ack for edge, and .irq_eoi for level.

>> +
>> +ÂÂÂ irq_chip_eoi_parent(data);
>> +}
>> +
>> +static int sun6i_r_intc_irq_set_type(struct irq_data *data, unsigned int type)
>> +{
>> +ÂÂÂ if (data->hwirq == NMI_HWIRQ) {
>> +ÂÂÂÂÂÂÂ u32 src_type;
>> +
>> +ÂÂÂÂÂÂÂ switch (type) {
>> +ÂÂÂÂÂÂÂ case IRQ_TYPE_EDGE_FALLING:
>> +ÂÂÂÂÂÂÂÂÂÂÂ src_type = SUNXI_SRC_TYPE_EDGE_FALLING;
>> +ÂÂÂÂÂÂÂÂÂÂÂ break;
>> +ÂÂÂÂÂÂÂ case IRQ_TYPE_EDGE_RISING:
>> +ÂÂÂÂÂÂÂÂÂÂÂ src_type = SUNXI_SRC_TYPE_EDGE_RISING;
>> +ÂÂÂÂÂÂÂÂÂÂÂ break;
>> +ÂÂÂÂÂÂÂ case IRQ_TYPE_LEVEL_HIGH:
>> +ÂÂÂÂÂÂÂÂÂÂÂ src_type = SUNXI_SRC_TYPE_LEVEL_HIGH;
>> +ÂÂÂÂÂÂÂÂÂÂÂ break;
>> +ÂÂÂÂÂÂÂ case IRQ_TYPE_NONE:
> 
> What does "IRQ_TYPE_NONE" mean here?

It means that IRQ_TYPE_NONE was put in the specifier in the device tree (this is
copied from the other driver). Since this should never happen, I've removed this
case in v2.

>> +ÂÂÂÂÂÂÂ case IRQ_TYPE_LEVEL_LOW:
>> +ÂÂÂÂÂÂÂÂÂÂÂ src_type = SUNXI_SRC_TYPE_LEVEL_LOW;
>> +ÂÂÂÂÂÂÂÂÂÂÂ break;
>> +ÂÂÂÂÂÂÂ default:
>> +ÂÂÂÂÂÂÂÂÂÂÂ pr_err("%pOF: invalid trigger type %d for IRQ %d\n",
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ irq_domain_get_of_node(data->domain), type,
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ data->irq);
>> +ÂÂÂÂÂÂÂÂÂÂÂ return -EBADR;
>> +ÂÂÂÂÂÂÂ }
>> +ÂÂÂÂÂÂÂ writel(src_type, base + SUN6I_R_INTC_CTRL);
>> +
>> +ÂÂÂÂÂÂÂ irqd_set_trigger_type(data, type);
> 
> It is odd to update this from a driver. Specially that you change it
> before finding out if the parent call has succeeded or not.

Yes, I'm not sure what I was doing there. I've removed this.

>> +
>> +ÂÂÂÂÂÂÂ /* Send the R_INTC -> GIC trigger type to the GIC driver. */
>> +ÂÂÂÂÂÂÂ type = parent_type;
>> +ÂÂÂ }
>> +
>> +ÂÂÂ return irq_chip_set_type_parent(data, type);
> 
> Half of the above signaling modes are invalid for the GIC. Does this
> widget actually invert the signalling when the input is either
> level low or falling edge?

Yes. The signal sent to the GIC is effectively bit 0 of the PENDING register. So
it's a "1" when the IRQ is triggered, regardless of the physical pin trigger type.

>> +}
>> +
>> +static struct irq_chip sun6i_r_intc_chip = {
>> +ÂÂÂ .nameÂÂÂÂÂÂÂÂÂÂÂ = "sun6i-r-intc",
>> +ÂÂÂ .irq_enableÂÂÂÂÂÂÂ = sun6i_r_intc_irq_enable,
>> +ÂÂÂ .irq_disableÂÂÂÂÂÂÂ = sun6i_r_intc_irq_disable,
>> +ÂÂÂ .irq_maskÂÂÂÂÂÂÂ = sun6i_r_intc_irq_mask,
>> +ÂÂÂ .irq_unmaskÂÂÂÂÂÂÂ = sun6i_r_intc_irq_unmask,
> 
> What is the upshot of having both enable/disable and mask/unmask?
> Given that the GIC only supports the latter, I'd expect this driver
> to leave everything enabled, and only deal with mask/unmask.

This makes sense. I've done this for v2.

>> +ÂÂÂ .irq_eoiÂÂÂÂÂÂÂ = sun6i_r_intc_irq_eoi,
>> +ÂÂÂ .irq_set_affinityÂÂÂ = irq_chip_set_affinity_parent,
>> +ÂÂÂ .irq_retriggerÂÂÂÂÂÂÂ = irq_chip_retrigger_hierarchy,
>> +ÂÂÂ .irq_set_typeÂÂÂÂÂÂÂ = sun6i_r_intc_irq_set_type,
>> +ÂÂÂ .irq_set_vcpu_affinityÂÂÂ = irq_chip_set_vcpu_affinity_parent,
> 
> Under which circumstances do you expect this to be called?

Under the same circumstances as the underlying GIC callback. It's my
understanding that irq_chip hooks always go to the outermost irqdomain in the
hierarchy. So the only way the GIC driver functions get called is if I call them
here with irq_chip_*_parent. Presumably, all of the GIC functions are there for
a reason, so I should expose them.

The same appears to be the case for .flags: flags from the inner irqdomains have
to be duplicated here, because the core interrupt handling code only looks at
the flags of desc->irq_data.chip->flags, which is the outermost irqdomain in the
hierarchy.

>> +};
>> +
>> +static int sun6i_r_intc_domain_translate(struct irq_domain *domain,
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct irq_fwspec *fwspec,
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned long *hwirq,
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned int *type)
>> +{
>> +ÂÂÂ if (!is_of_node(fwspec->fwnode) || fwspec->param_count != 2)
>> +ÂÂÂÂÂÂÂ return -EINVAL;
>> +
>> +ÂÂÂ *hwirq = fwspec->param[0];
>> + *type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
>> +
>> +ÂÂÂ return 0;
>> +}
>> +
>> +static int sun6i_r_intc_domain_alloc(struct irq_domain *domain,
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned int virq,
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned int nr_irqs, void *arg)
>> +{
>> +ÂÂÂ struct irq_fwspec *fwspec = arg;
>> +ÂÂÂ struct irq_fwspec gic_fwspec;
>> +ÂÂÂ irq_hw_number_t hwirq;
>> +ÂÂÂ unsigned int type;
>> +ÂÂÂ int i, ret;
>> +
>> +ÂÂÂ ret = sun6i_r_intc_domain_translate(domain, fwspec, &hwirq, &type);
>> +ÂÂÂ if (ret)
>> +ÂÂÂÂÂÂÂ return ret;
>> +ÂÂÂ if (hwirq + nr_irqs > SUN6I_R_INTC_NR_IRQS)
>> +ÂÂÂÂÂÂÂ return -EINVAL;
>> +
>> +ÂÂÂ /* Construct a GIC-compatible fwspec from this fwspec. */
>> +ÂÂÂ gic_fwspec = (struct irq_fwspec) {
>> +ÂÂÂÂÂÂÂ .fwnodeÂÂÂÂÂ = domain->parent->fwnode,
>> +ÂÂÂÂÂÂÂ .param_count = 3,
>> +ÂÂÂÂÂÂÂ .paramÂÂÂÂÂÂ = { GIC_SPI, parent_offset + hwirq, type },
> 
> Same problem here. The GIC only supports level-high and rising-edge
> for SPIs.

This actually doesn't cause any errors. The only check that the GIC does on the
"type" parameter during .translate is verifying that it is not IRQ_TYPE_NONE,
and no additional checks are done during .alloc. In fact, nothing seems to
really care about "type" during .alloc. They just want "hwirq", and it's
convenient to use .translate to get that.

>> +ÂÂÂ };
>> +
>> +ÂÂÂ for (i = 0; i < nr_irqs; ++i)
>> +ÂÂÂÂÂÂÂ irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ &sun6i_r_intc_chip, NULL);
>> +
>> +ÂÂÂ return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &gic_fwspec);
>> +}
>> +
>> +static const struct irq_domain_ops sun6i_r_intc_domain_ops = {
>> +ÂÂÂ .translateÂÂÂ = sun6i_r_intc_domain_translate,
>> +ÂÂÂ .allocÂÂÂÂÂÂÂ = sun6i_r_intc_domain_alloc,
>> +ÂÂÂ .freeÂÂÂÂÂÂÂ = irq_domain_free_irqs_common,
>> +};
>> +
>> +static int __init sun6i_r_intc_init(struct device_node *node,
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct device_node *parent)
>> +{
>> +ÂÂÂ struct irq_domain *domain, *parent_domain;
>> +ÂÂÂ struct of_phandle_args parent_irq;
>> +ÂÂÂ int ret;
>> +
>> +ÂÂÂ /* Extract the R_INTC -> GIC mapping from the OF node. */
>> +ÂÂÂ ret = of_irq_parse_one(node, 0, &parent_irq);
>> +ÂÂÂ if (ret)
>> +ÂÂÂÂÂÂÂ return ret;
>> +ÂÂÂ if (parent_irq.args_count != 3 || parent_irq.args[0] != GIC_SPI)
>> +ÂÂÂÂÂÂÂ return -EINVAL;
>> +ÂÂÂ parent_offset = parent_irq.args[1];
>> +ÂÂÂ parent_typeÂÂ = parent_irq.args[2];
>> +
>> +ÂÂÂ parent_domain = irq_find_host(parent);
>> +ÂÂÂ if (!parent_domain) {
>> +ÂÂÂÂÂÂÂ pr_err("%pOF: Failed to obtain parent domain\n", node);
>> +ÂÂÂÂÂÂÂ return -ENXIO;
>> +ÂÂÂ }
>> +
>> +ÂÂÂ base = of_io_request_and_map(node, 0, NULL);
>> +ÂÂÂ if (IS_ERR(base)) {
>> +ÂÂÂÂÂÂÂ pr_err("%pOF: Failed to map MMIO region\n", node);
>> +ÂÂÂÂÂÂÂ return PTR_ERR(base);
>> +ÂÂÂ }
>> +
>> +ÂÂÂ domain = irq_domain_add_hierarchy(parent_domain, 0,
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ SUN6I_R_INTC_NR_IRQS, node,
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ &sun6i_r_intc_domain_ops, NULL);
>> +ÂÂÂ if (!domain) {
>> +ÂÂÂÂÂÂÂ pr_err("%pOF: Failed to allocate domain\n", node);
>> +ÂÂÂÂÂÂÂ iounmap(base);
>> +ÂÂÂÂÂÂÂ return -ENOMEM;
>> +ÂÂÂ }
>> +
>> +ÂÂÂ /* Disable and unmask all interrupts. */
>> +ÂÂÂ writel(0, base + SUN6I_R_INTC_ENABLE);
>> +ÂÂÂ writel(0, base + SUN6I_R_INTC_MASK);
>> +
>> +ÂÂÂ /* Clear any pending interrupts. */
>> +ÂÂÂ writel(~0, base + SUN6I_R_INTC_PENDING);
>> +
>> +ÂÂÂ return 0;
>> +}
>> +IRQCHIP_DECLARE(sun6i_r_intc, "allwinner,sun6i-a31-r-intc", sun6i_r_intc_init);
>> diff --git a/drivers/irqchip/irq-sunxi-nmi.c b/drivers/irqchip/irq-sunxi-nmi.c
>> index a412b5d5d0fa..9f2bd0c5d289 100644
>> --- a/drivers/irqchip/irq-sunxi-nmi.c
>> +++ b/drivers/irqchip/irq-sunxi-nmi.c
>> @@ -27,18 +27,12 @@
>>
>> Â#define SUNXI_NMI_IRQ_BITÂÂÂ BIT(0)
>>
>> -#define SUN6I_R_INTC_CTRLÂÂÂ 0x0c
>> -#define SUN6I_R_INTC_PENDINGÂÂÂ 0x10
>> -#define SUN6I_R_INTC_ENABLEÂÂÂ 0x40
>> -
>> Â/*
>> Â * For deprecated sun6i-a31-sc-nmi compatible.
>> - * Registers are offset by 0x0c.
>> Â */
>> -#define SUN6I_R_INTC_NMI_OFFSETÂÂÂ 0x0c
>> -#define SUN6I_NMI_CTRLÂÂÂÂÂÂÂ (SUN6I_R_INTC_CTRL - SUN6I_R_INTC_NMI_OFFSET)
>> -#define SUN6I_NMI_PENDINGÂÂÂ (SUN6I_R_INTC_PENDING - SUN6I_R_INTC_NMI_OFFSET)
>> -#define SUN6I_NMI_ENABLEÂÂÂ (SUN6I_R_INTC_ENABLE - SUN6I_R_INTC_NMI_OFFSET)
>> +#define SUN6I_NMI_CTRLÂÂÂÂÂÂÂ 0x00
>> +#define SUN6I_NMI_PENDINGÂÂÂ 0x04
>> +#define SUN6I_NMI_ENABLEÂÂÂ 0x34
>>
>> Â#define SUN7I_NMI_CTRLÂÂÂÂÂÂÂ 0x00
>> Â#define SUN7I_NMI_PENDINGÂÂÂ 0x04
>> @@ -61,12 +55,6 @@ struct sunxi_sc_nmi_reg_offs {
>> ÂÂÂÂ u32 enable;
>> Â};
>>
>> -static const struct sunxi_sc_nmi_reg_offs sun6i_r_intc_reg_offs __initconst = {
>> -ÂÂÂ .ctrlÂÂÂ = SUN6I_R_INTC_CTRL,
>> -ÂÂÂ .pendÂÂÂ = SUN6I_R_INTC_PENDING,
>> -ÂÂÂ .enableÂÂÂ = SUN6I_R_INTC_ENABLE,
>> -};
>> -
>> Âstatic const struct sunxi_sc_nmi_reg_offs sun6i_reg_offs __initconst = {
>> ÂÂÂÂ .ctrlÂÂÂ = SUN6I_NMI_CTRL,
>> ÂÂÂÂ .pendÂÂÂ = SUN6I_NMI_PENDING,
>> @@ -232,14 +220,6 @@ static int __init sunxi_sc_nmi_irq_init(struct
>> device_node *node,
>> ÂÂÂÂ return ret;
>> Â}
>>
>> -static int __init sun6i_r_intc_irq_init(struct device_node *node,
>> -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct device_node *parent)
>> -{
>> -ÂÂÂ return sunxi_sc_nmi_irq_init(node, &sun6i_r_intc_reg_offs);
>> -}
>> -IRQCHIP_DECLARE(sun6i_r_intc, "allwinner,sun6i-a31-r-intc",
>> -ÂÂÂÂÂÂÂ sun6i_r_intc_irq_init);
>> -
>> Âstatic int __init sun6i_sc_nmi_irq_init(struct device_node *node,
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct device_node *parent)
>> Â{
> 
> Thanks,
> 
> ÂÂÂÂÂÂÂ M.

Thanks again for your review,
Samuel