The NMI controller in the Allwinner A523 is almost compatible to the
previous versions of this IP, but requires the extra bit 31 to be set in
the enable register to actually report the NMI.

Add a mask to allow such an enable bit to be specified, and add this to
the per-SoC data structure. As this struct was just for different register
offsets so far, it was consequently named "reg_offs", which is now no
longer applicable, so rename this to the more generic "data" on the way.

Also add the respective Allwinner A523 compatible string, and set bit 31
in its enable mask, to add support for this SoC.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/irqchip/irq-sunxi-nmi.c | 50 ++++++++++++++++++++++-----------
 1 file changed, 33 insertions(+), 17 deletions(-)

diff --git a/drivers/irqchip/irq-sunxi-nmi.c b/drivers/irqchip/irq-sunxi-nmi.c
index 0b43121520243..9380c3cafeeb5 100644
--- a/drivers/irqchip/irq-sunxi-nmi.c
+++ b/drivers/irqchip/irq-sunxi-nmi.c
@@ -48,30 +48,38 @@ enum {
 	SUNXI_SRC_TYPE_EDGE_RISING,
 };
 
-struct sunxi_sc_nmi_reg_offs {
+struct sunxi_sc_nmi_data {
 	u32 ctrl;
 	u32 pend;
 	u32 enable;
+	u32 enable_val;
 };
 
-static const struct sunxi_sc_nmi_reg_offs sun6i_reg_offs __initconst = {
+static const struct sunxi_sc_nmi_data sun6i_data __initconst = {
 	.ctrl	= SUN6I_NMI_CTRL,
 	.pend	= SUN6I_NMI_PENDING,
 	.enable	= SUN6I_NMI_ENABLE,
 };
 
-static const struct sunxi_sc_nmi_reg_offs sun7i_reg_offs __initconst = {
+static const struct sunxi_sc_nmi_data sun7i_data __initconst = {
 	.ctrl	= SUN7I_NMI_CTRL,
 	.pend	= SUN7I_NMI_PENDING,
 	.enable	= SUN7I_NMI_ENABLE,
 };
 
-static const struct sunxi_sc_nmi_reg_offs sun9i_reg_offs __initconst = {
+static const struct sunxi_sc_nmi_data sun9i_data __initconst = {
 	.ctrl	= SUN9I_NMI_CTRL,
 	.pend	= SUN9I_NMI_PENDING,
 	.enable	= SUN9I_NMI_ENABLE,
 };
 
+static const struct sunxi_sc_nmi_data sun55i_a523_data __initconst = {
+	.ctrl	= SUN9I_NMI_CTRL,
+	.pend	= SUN9I_NMI_PENDING,
+	.enable	= SUN9I_NMI_ENABLE,
+	.enable_val = BIT(31),
+};
+
 static inline void sunxi_sc_nmi_write(struct irq_chip_generic *gc, u32 off,
 				      u32 val)
 {
@@ -143,7 +151,7 @@ static int sunxi_sc_nmi_set_type(struct irq_data *data, unsigned int flow_type)
 }
 
 static int __init sunxi_sc_nmi_irq_init(struct device_node *node,
-					const struct sunxi_sc_nmi_reg_offs *reg_offs)
+					const struct sunxi_sc_nmi_data *data)
 {
 	struct irq_domain *domain;
 	struct irq_chip_generic *gc;
@@ -186,27 +194,28 @@ static int __init sunxi_sc_nmi_irq_init(struct device_node *node,
 	gc->chip_types[0].chip.irq_unmask	= irq_gc_mask_set_bit;
 	gc->chip_types[0].chip.irq_eoi		= irq_gc_ack_set_bit;
 	gc->chip_types[0].chip.irq_set_type	= sunxi_sc_nmi_set_type;
-	gc->chip_types[0].chip.flags		= IRQCHIP_EOI_THREADED | IRQCHIP_EOI_IF_HANDLED |
+	gc->chip_types[0].chip.flags		= IRQCHIP_EOI_THREADED |
+						  IRQCHIP_EOI_IF_HANDLED |
 						  IRQCHIP_SKIP_SET_WAKE;
-	gc->chip_types[0].regs.ack		= reg_offs->pend;
-	gc->chip_types[0].regs.mask		= reg_offs->enable;
-	gc->chip_types[0].regs.type		= reg_offs->ctrl;
+	gc->chip_types[0].regs.ack		= data->pend;
+	gc->chip_types[0].regs.mask		= data->enable;
+	gc->chip_types[0].regs.type		= data->ctrl;
 
 	gc->chip_types[1].type			= IRQ_TYPE_EDGE_BOTH;
 	gc->chip_types[1].chip.irq_ack		= irq_gc_ack_set_bit;
 	gc->chip_types[1].chip.irq_mask		= irq_gc_mask_clr_bit;
 	gc->chip_types[1].chip.irq_unmask	= irq_gc_mask_set_bit;
 	gc->chip_types[1].chip.irq_set_type	= sunxi_sc_nmi_set_type;
-	gc->chip_types[1].regs.ack		= reg_offs->pend;
-	gc->chip_types[1].regs.mask		= reg_offs->enable;
-	gc->chip_types[1].regs.type		= reg_offs->ctrl;
+	gc->chip_types[1].regs.ack		= data->pend;
+	gc->chip_types[1].regs.mask		= data->enable;
+	gc->chip_types[1].regs.type		= data->ctrl;
 	gc->chip_types[1].handler		= handle_edge_irq;
 
 	/* Disable any active interrupts */
-	sunxi_sc_nmi_write(gc, reg_offs->enable, 0);
+	sunxi_sc_nmi_write(gc, data->enable, data->enable_val);
 
 	/* Clear any pending NMI interrupts */
-	sunxi_sc_nmi_write(gc, reg_offs->pend, SUNXI_NMI_IRQ_BIT);
+	sunxi_sc_nmi_write(gc, data->pend, SUNXI_NMI_IRQ_BIT);
 
 	irq_set_chained_handler_and_data(irq, sunxi_sc_nmi_handle_irq, domain);
 
@@ -221,20 +230,27 @@ static int __init sunxi_sc_nmi_irq_init(struct device_node *node,
 static int __init sun6i_sc_nmi_irq_init(struct device_node *node,
 					struct device_node *parent)
 {
-	return sunxi_sc_nmi_irq_init(node, &sun6i_reg_offs);
+	return sunxi_sc_nmi_irq_init(node, &sun6i_data);
 }
 IRQCHIP_DECLARE(sun6i_sc_nmi, "allwinner,sun6i-a31-sc-nmi", sun6i_sc_nmi_irq_init);
 
 static int __init sun7i_sc_nmi_irq_init(struct device_node *node,
 					struct device_node *parent)
 {
-	return sunxi_sc_nmi_irq_init(node, &sun7i_reg_offs);
+	return sunxi_sc_nmi_irq_init(node, &sun7i_data);
 }
 IRQCHIP_DECLARE(sun7i_sc_nmi, "allwinner,sun7i-a20-sc-nmi", sun7i_sc_nmi_irq_init);
 
 static int __init sun9i_nmi_irq_init(struct device_node *node,
 				     struct device_node *parent)
 {
-	return sunxi_sc_nmi_irq_init(node, &sun9i_reg_offs);
+	return sunxi_sc_nmi_irq_init(node, &sun9i_data);
 }
 IRQCHIP_DECLARE(sun9i_nmi, "allwinner,sun9i-a80-nmi", sun9i_nmi_irq_init);
+
+static int __init sun55i_nmi_irq_init(struct device_node *node,
+				      struct device_node *parent)
+{
+	return sunxi_sc_nmi_irq_init(node, &sun55i_a523_data);
+}
+IRQCHIP_DECLARE(sun55i_nmi, "allwinner,sun55i-a523-nmi", sun55i_nmi_irq_init);
-- 
2.46.3

On Tue, Mar 04 2025 at 22:23, Andre Przywara wrote:
>  
> -struct sunxi_sc_nmi_reg_offs {
> +struct sunxi_sc_nmi_data {
>  	u32 ctrl;
>  	u32 pend;
>  	u32 enable;
> +	u32 enable_val;

The data structure name and the corresponding variable/argument name
were making the code pretty obvious, but now this is opaque and
incomprehensible.

data::ctrl does not even give the slightest hint what this is about. You
need to read up in the code to figure out what it means. Something like:

struct sunxi_sc_nmi_data {
	u32	reg_offs_ctrl;
  	u32	reg_offs_pend;
  	u32	reg_offs_enable;
	u32	enable_val;
};

or even better:

struct sunxi_sc_nmi_data {
	struct {
		u32	ctrl;
  		u32	pend;
  		u32	enable;
        } reg_offs;
	u32		enable_val;
};

makes it clear and obvious, no?

> +static const struct sunxi_sc_nmi_data sun55i_a523_data __initconst = {
> +	.ctrl	= SUN9I_NMI_CTRL,
> +	.pend	= SUN9I_NMI_PENDING,
> +	.enable	= SUN9I_NMI_ENABLE,
> +	.enable_val = BIT(31),

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#struct-declarations-and-initializers

Thanks,

        tglx

On Wed, 05 Mar 2025 08:41:28 +0100
Thomas Gleixner <tglx@linutronix.de> wrote:

Hi Thomas,

thanks for having a look!

> On Tue, Mar 04 2025 at 22:23, Andre Przywara wrote:
> >  
> > -struct sunxi_sc_nmi_reg_offs {
> > +struct sunxi_sc_nmi_data {
> >  	u32 ctrl;
> >  	u32 pend;
> >  	u32 enable;
> > +	u32 enable_val;  
> 
> The data structure name and the corresponding variable/argument name
> were making the code pretty obvious, but now this is opaque and
> incomprehensible.
> 
> data::ctrl does not even give the slightest hint what this is about. You
> need to read up in the code to figure out what it means. Something like:
> 
> struct sunxi_sc_nmi_data {
> 	u32	reg_offs_ctrl;
>   	u32	reg_offs_pend;
>   	u32	reg_offs_enable;
> 	u32	enable_val;
> };
> 
> or even better:
> 
> struct sunxi_sc_nmi_data {
> 	struct {
> 		u32	ctrl;
>   		u32	pend;
>   		u32	enable;
>         } reg_offs;
> 	u32		enable_val;
> };
> 
> makes it clear and obvious, no?

Sure, will change it, it was just the usual decision between reworking the
existing code or just adding my small change in. 50% chance of getting that
right, I guess ;-)

> 
> > +static const struct sunxi_sc_nmi_data sun55i_a523_data __initconst = {
> > +	.ctrl	= SUN9I_NMI_CTRL,
> > +	.pend	= SUN9I_NMI_PENDING,
> > +	.enable	= SUN9I_NMI_ENABLE,
> > +	.enable_val = BIT(31),  
> 
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#struct-declarations-and-initializers

Oops, missed that.

Thanks,
Andre

> 
> Thanks,
> 
>         tglx