[PATCH 0/2] irqchip: Add support for Aspeed AST2700 SCU interrupt controller

[PATCH 0/2] irqchip: Add support for Aspeed AST2700 SCU interrupt controller

[Ryan Chen]

This patch series adds support for the SCU (System Control Unit) interrupt
controllers found in the Aspeed AST2700 SoC.

The AST2700 integrates multiple SCU interrupt groups, architecturally
similar to those in the AST2600, but using split registers for interrupt
enable (IER) and interrupt status (ISR), whereas AST2600 uses combined
registers.

Ryan Chen (2):
  dt-bindings: interrupt-controller: aspeed: add AST2700 SCU IC
    compatibles
  irqchip/aspeed-scu-ic: Add support for AST2700 SCU interrupt
    controllers

 .../aspeed,ast2500-scu-ic.yaml                |   6 +-
 drivers/irqchip/irq-aspeed-scu-ic.c           | 240 ++++++++++++++----
 2 files changed, 200 insertions(+), 46 deletions(-)

-- 
2.34.1

[PATCH 1/2] dt-bindings: interrupt-controller: aspeed: add AST2700 SCU IC compatibles

[Ryan Chen]

- Add "aspeed,ast2700-scu-ic0,1,2,3" to the compatible
 list in aspeed,ast2500-scu-ic.yaml.
- Document support for AST27XX SCU interrupt controllers.

Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
---
 .../interrupt-controller/aspeed,ast2500-scu-ic.yaml         | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2500-scu-ic.yaml b/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2500-scu-ic.yaml
index d5287a2bf866..d998a9d69b91 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2500-scu-ic.yaml
+++ b/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2500-scu-ic.yaml
@@ -5,7 +5,7 @@
 $id: http://devicetree.org/schemas/interrupt-controller/aspeed,ast2500-scu-ic.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: Aspeed AST25XX and AST26XX SCU Interrupt Controller
+title: Aspeed AST25XX, AST26XX, AST27XX SCU Interrupt Controller
 
 maintainers:
   - Eddie James <eajames@linux.ibm.com>
@@ -16,6 +16,10 @@ properties:
       - aspeed,ast2500-scu-ic
       - aspeed,ast2600-scu-ic0
       - aspeed,ast2600-scu-ic1
+      - aspeed,ast2700-scu-ic0
+      - aspeed,ast2700-scu-ic1
+      - aspeed,ast2700-scu-ic2
+      - aspeed,ast2700-scu-ic3
 
   reg:
     maxItems: 1
-- 
2.34.1

Re: [PATCH 1/2] dt-bindings: interrupt-controller: aspeed: add AST2700 SCU IC compatibles

[Krzysztof Kozlowski]

On 04/08/2025 07:34, Ryan Chen wrote:
> - Add "aspeed,ast2700-scu-ic0,1,2,3" to the compatible
>  list in aspeed,ast2500-scu-ic.yaml.
> - Document support for AST27XX SCU interrupt controllers.

We see that from the diff.  Explain the differences between them. Also,
do not use lists - it seems you are mixing two independent tasks in one
commit.



Best regards,
Krzysztof

RE: [PATCH 1/2] dt-bindings: interrupt-controller: aspeed: add AST2700 SCU IC compatibles

[Ryan Chen]

> Subject: Re: [PATCH 1/2] dt-bindings: interrupt-controller: aspeed: add AST2700
> SCU IC compatibles
> 
> On 04/08/2025 07:34, Ryan Chen wrote:
> > - Add "aspeed,ast2700-scu-ic0,1,2,3" to the compatible  list in
> > aspeed,ast2500-scu-ic.yaml.
> > - Document support for AST27XX SCU interrupt controllers.
> 
> We see that from the diff.  Explain the differences between them. Also, do not
> use lists - it seems you are mixing two independent tasks in one commit.
> 
I will modify by following.

The AST2700, Aspeed’s seventh-generation SoC, features 4 SCU 
interrupt controllers.

> 
> 
> Best regards,
> Krzysztof

[PATCH 2/2] irqchip/aspeed-scu-ic: Add support for AST2700 SCU interrupt controllers

[Ryan Chen]

The AST2700 SoC follows the multi-instance interrupt controller architecture
introduced in the AST2600, where each SCU interrupt group (IC0–IC3) is treated
as an independent interrupt domain.

Unlike the AST2600, which uses a combined register for interrupt enable and
status bits, the AST2700 separates these into distinct registers: one for
interrupt enable (IER) and another for interrupt status (ISR). This architectural
change requires explicit handling of split registers for interrupt control.

- Register definitions and configuration for AST2700 SCU IC instances
  (compatible: aspeed,ast2700-scu-ic0/1/2/3)
- Initialization logic for handling split IER/ISR registers
- Chained IRQ handling and mask/unmask logic
- Table-driven registration using IRQCHIP_DECLARE per compatible

Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
---
 drivers/irqchip/irq-aspeed-scu-ic.c | 240 ++++++++++++++++++++++------
 1 file changed, 195 insertions(+), 45 deletions(-)

diff --git a/drivers/irqchip/irq-aspeed-scu-ic.c b/drivers/irqchip/irq-aspeed-scu-ic.c
index 1c7045467c48..b6f3ba269c5b 100644
--- a/drivers/irqchip/irq-aspeed-scu-ic.c
+++ b/drivers/irqchip/irq-aspeed-scu-ic.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 /*
- * Aspeed AST24XX, AST25XX, and AST26XX SCU Interrupt Controller
+ * Aspeed AST24XX, AST25XX, AST26XX, AST27XX SCU Interrupt Controller
  * Copyright 2019 IBM Corporation
  *
  * Eddie James <eajames@linux.ibm.com>
@@ -34,11 +34,42 @@
 	GENMASK(5, ASPEED_AST2600_SCU_IC1_SHIFT)
 #define ASPEED_AST2600_SCU_IC1_NUM_IRQS	2
 
+#define ASPEED_AST2700_SCU_IC0_EN_REG	0x1d0
+#define ASPEED_AST2700_SCU_IC0_STS_REG	0x1d4
+#define ASPEED_AST2700_SCU_IC0_SHIFT	0
+#define ASPEED_AST2700_SCU_IC0_ENABLE	\
+	GENMASK(3, ASPEED_AST2700_SCU_IC0_SHIFT)
+#define ASPEED_AST2700_SCU_IC0_NUM_IRQS	4
+
+#define ASPEED_AST2700_SCU_IC1_EN_REG	0x1e0
+#define ASPEED_AST2700_SCU_IC1_STS_REG	0x1e4
+#define ASPEED_AST2700_SCU_IC1_SHIFT	0
+#define ASPEED_AST2700_SCU_IC1_ENABLE	\
+	GENMASK(3, ASPEED_AST2700_SCU_IC1_SHIFT)
+#define ASPEED_AST2700_SCU_IC1_NUM_IRQS	4
+
+#define ASPEED_AST2700_SCU_IC2_EN_REG	0x104
+#define ASPEED_AST2700_SCU_IC2_STS_REG	0x100
+#define ASPEED_AST2700_SCU_IC2_SHIFT	0
+#define ASPEED_AST2700_SCU_IC2_ENABLE	\
+	GENMASK(3, ASPEED_AST2700_SCU_IC2_SHIFT)
+#define ASPEED_AST2700_SCU_IC2_NUM_IRQS	4
+
+#define ASPEED_AST2700_SCU_IC3_EN_REG	0x10c
+#define ASPEED_AST2700_SCU_IC3_STS_REG	0x108
+#define ASPEED_AST2700_SCU_IC3_SHIFT	0
+#define ASPEED_AST2700_SCU_IC3_ENABLE	\
+	GENMASK(1, ASPEED_AST2700_SCU_IC3_SHIFT)
+#define ASPEED_AST2700_SCU_IC3_NUM_IRQS	2
+
 struct aspeed_scu_ic {
 	unsigned long irq_enable;
 	unsigned long irq_shift;
 	unsigned int num_irqs;
 	unsigned int reg;
+	unsigned int en_reg;
+	unsigned int sts_reg;
+	bool split_ier_isr;
 	struct regmap *scu;
 	struct irq_domain *irq_domain;
 };
@@ -52,33 +83,51 @@ static void aspeed_scu_ic_irq_handler(struct irq_desc *desc)
 	unsigned long status;
 	struct aspeed_scu_ic *scu_ic = irq_desc_get_handler_data(desc);
 	struct irq_chip *chip = irq_desc_get_chip(desc);
-	unsigned int mask = scu_ic->irq_enable << ASPEED_SCU_IC_STATUS_SHIFT;
+	unsigned int mask;
 
 	chained_irq_enter(chip, desc);
 
-	/*
-	 * The SCU IC has just one register to control its operation and read
-	 * status. The interrupt enable bits occupy the lower 16 bits of the
-	 * register, while the interrupt status bits occupy the upper 16 bits.
-	 * The status bit for a given interrupt is always 16 bits shifted from
-	 * the enable bit for the same interrupt.
-	 * Therefore, perform the IRQ operations in the enable bit space by
-	 * shifting the status down to get the mapping and then back up to
-	 * clear the bit.
-	 */
-	regmap_read(scu_ic->scu, scu_ic->reg, &sts);
-	enabled = sts & scu_ic->irq_enable;
-	status = (sts >> ASPEED_SCU_IC_STATUS_SHIFT) & enabled;
-
-	bit = scu_ic->irq_shift;
-	max = scu_ic->num_irqs + bit;
-
-	for_each_set_bit_from(bit, &status, max) {
-		generic_handle_domain_irq(scu_ic->irq_domain,
-					  bit - scu_ic->irq_shift);
-
-		regmap_write_bits(scu_ic->scu, scu_ic->reg, mask,
-				  BIT(bit + ASPEED_SCU_IC_STATUS_SHIFT));
+	if (scu_ic->split_ier_isr) {
+		mask = scu_ic->irq_enable;
+		regmap_read(scu_ic->scu, scu_ic->en_reg, &sts);
+		enabled = sts & scu_ic->irq_enable;
+		regmap_read(scu_ic->scu, scu_ic->sts_reg, &sts);
+		status = sts & enabled;
+
+		bit = scu_ic->irq_shift;
+		max = scu_ic->num_irqs + bit;
+
+		for_each_set_bit_from(bit, &status, max) {
+			generic_handle_domain_irq(scu_ic->irq_domain, bit - scu_ic->irq_shift);
+
+			regmap_write_bits(scu_ic->scu, scu_ic->sts_reg, mask, BIT(bit));
+		}
+	} else {
+		mask = scu_ic->irq_enable << ASPEED_SCU_IC_STATUS_SHIFT;
+		/*
+		 * The SCU IC has just one register to control its operation and read
+		 * status. The interrupt enable bits occupy the lower 16 bits of the
+		 * register, while the interrupt status bits occupy the upper 16 bits.
+		 * The status bit for a given interrupt is always 16 bits shifted from
+		 * the enable bit for the same interrupt.
+		 * Therefore, perform the IRQ operations in the enable bit space by
+		 * shifting the status down to get the mapping and then back up to
+		 * clear the bit.
+		 */
+		regmap_read(scu_ic->scu, scu_ic->reg, &sts);
+		enabled = sts & scu_ic->irq_enable;
+		status = (sts >> ASPEED_SCU_IC_STATUS_SHIFT) & enabled;
+
+		bit = scu_ic->irq_shift;
+		max = scu_ic->num_irqs + bit;
+
+		for_each_set_bit_from(bit, &status, max) {
+			generic_handle_domain_irq(scu_ic->irq_domain,
+						  bit - scu_ic->irq_shift);
+
+			regmap_write_bits(scu_ic->scu, scu_ic->reg, mask,
+					  BIT(bit + ASPEED_SCU_IC_STATUS_SHIFT));
+		}
 	}
 
 	chained_irq_exit(chip, desc);
@@ -87,30 +136,42 @@ static void aspeed_scu_ic_irq_handler(struct irq_desc *desc)
 static void aspeed_scu_ic_irq_mask(struct irq_data *data)
 {
 	struct aspeed_scu_ic *scu_ic = irq_data_get_irq_chip_data(data);
-	unsigned int mask = BIT(data->hwirq + scu_ic->irq_shift) |
-		(scu_ic->irq_enable << ASPEED_SCU_IC_STATUS_SHIFT);
-
-	/*
-	 * Status bits are cleared by writing 1. In order to prevent the mask
-	 * operation from clearing the status bits, they should be under the
-	 * mask and written with 0.
-	 */
-	regmap_update_bits(scu_ic->scu, scu_ic->reg, mask, 0);
+	unsigned int mask;
+
+	if (scu_ic->split_ier_isr) {
+		mask = BIT(data->hwirq + scu_ic->irq_shift);
+		regmap_update_bits(scu_ic->scu, scu_ic->en_reg, mask, 0);
+	} else {
+		mask = BIT(data->hwirq + scu_ic->irq_shift) |
+		       (scu_ic->irq_enable << ASPEED_SCU_IC_STATUS_SHIFT);
+		/*
+		 * Status bits are cleared by writing 1. In order to prevent the mask
+		 * operation from clearing the status bits, they should be under the
+		 * mask and written with 0.
+		 */
+		regmap_update_bits(scu_ic->scu, scu_ic->reg, mask, 0);
+	}
 }
 
 static void aspeed_scu_ic_irq_unmask(struct irq_data *data)
 {
 	struct aspeed_scu_ic *scu_ic = irq_data_get_irq_chip_data(data);
 	unsigned int bit = BIT(data->hwirq + scu_ic->irq_shift);
-	unsigned int mask = bit |
-		(scu_ic->irq_enable << ASPEED_SCU_IC_STATUS_SHIFT);
-
-	/*
-	 * Status bits are cleared by writing 1. In order to prevent the unmask
-	 * operation from clearing the status bits, they should be under the
-	 * mask and written with 0.
-	 */
-	regmap_update_bits(scu_ic->scu, scu_ic->reg, mask, bit);
+	unsigned int mask;
+
+	if (scu_ic->split_ier_isr) {
+		mask = bit;
+		regmap_update_bits(scu_ic->scu, scu_ic->en_reg, mask, bit);
+	} else {
+		mask = bit | (scu_ic->irq_enable << ASPEED_SCU_IC_STATUS_SHIFT);
+
+		/*
+		 * Status bits are cleared by writing 1. In order to prevent the unmask
+		 * operation from clearing the status bits, they should be under the
+		 * mask and written with 0.
+		 */
+		regmap_update_bits(scu_ic->scu, scu_ic->reg, mask, bit);
+	}
 }
 
 static int aspeed_scu_ic_irq_set_affinity(struct irq_data *data,
@@ -156,8 +217,17 @@ static int aspeed_scu_ic_of_init_common(struct aspeed_scu_ic *scu_ic,
 		rc = PTR_ERR(scu_ic->scu);
 		goto err;
 	}
-	regmap_write_bits(scu_ic->scu, scu_ic->reg, ASPEED_SCU_IC_STATUS, ASPEED_SCU_IC_STATUS);
-	regmap_write_bits(scu_ic->scu, scu_ic->reg, ASPEED_SCU_IC_ENABLE, 0);
+
+	if (scu_ic->split_ier_isr) {
+		regmap_write_bits(scu_ic->scu, scu_ic->sts_reg,
+				  scu_ic->irq_enable, scu_ic->irq_enable);
+		regmap_write_bits(scu_ic->scu, scu_ic->en_reg, scu_ic->irq_enable, 0);
+
+	} else {
+		regmap_write_bits(scu_ic->scu, scu_ic->reg,
+				  ASPEED_SCU_IC_STATUS, ASPEED_SCU_IC_STATUS);
+		regmap_write_bits(scu_ic->scu, scu_ic->reg, ASPEED_SCU_IC_ENABLE, 0);
+	}
 
 	irq = irq_of_parse_and_map(node, 0);
 	if (!irq) {
@@ -232,9 +302,89 @@ static int __init aspeed_ast2600_scu_ic1_of_init(struct device_node *node,
 	return aspeed_scu_ic_of_init_common(scu_ic, node);
 }
 
+static int __init aspeed_ast2700_scu_ic0_of_init(struct device_node *node,
+						 struct device_node *parent)
+{
+	struct aspeed_scu_ic *scu_ic = kzalloc(sizeof(*scu_ic), GFP_KERNEL);
+
+	if (!scu_ic)
+		return -ENOMEM;
+
+	scu_ic->irq_enable = ASPEED_AST2700_SCU_IC0_ENABLE;
+	scu_ic->irq_shift = ASPEED_AST2700_SCU_IC0_SHIFT;
+	scu_ic->num_irqs = ASPEED_AST2700_SCU_IC0_NUM_IRQS;
+	scu_ic->split_ier_isr = true;
+	scu_ic->en_reg = ASPEED_AST2700_SCU_IC0_EN_REG;
+	scu_ic->sts_reg = ASPEED_AST2700_SCU_IC0_STS_REG;
+
+	return aspeed_scu_ic_of_init_common(scu_ic, node);
+}
+
+static int __init aspeed_ast2700_scu_ic1_of_init(struct device_node *node,
+						 struct device_node *parent)
+{
+	struct aspeed_scu_ic *scu_ic = kzalloc(sizeof(*scu_ic), GFP_KERNEL);
+
+	if (!scu_ic)
+		return -ENOMEM;
+
+	scu_ic->irq_enable = ASPEED_AST2700_SCU_IC1_ENABLE;
+	scu_ic->irq_shift = ASPEED_AST2700_SCU_IC1_SHIFT;
+	scu_ic->num_irqs = ASPEED_AST2700_SCU_IC1_NUM_IRQS;
+	scu_ic->split_ier_isr = true;
+	scu_ic->en_reg = ASPEED_AST2700_SCU_IC1_EN_REG;
+	scu_ic->sts_reg = ASPEED_AST2700_SCU_IC1_STS_REG;
+
+	return aspeed_scu_ic_of_init_common(scu_ic, node);
+}
+
+static int __init aspeed_ast2700_scu_ic2_of_init(struct device_node *node,
+						 struct device_node *parent)
+{
+	struct aspeed_scu_ic *scu_ic = kzalloc(sizeof(*scu_ic), GFP_KERNEL);
+
+	if (!scu_ic)
+		return -ENOMEM;
+
+	scu_ic->irq_enable = ASPEED_AST2700_SCU_IC2_ENABLE;
+	scu_ic->irq_shift = ASPEED_AST2700_SCU_IC2_SHIFT;
+	scu_ic->num_irqs = ASPEED_AST2700_SCU_IC2_NUM_IRQS;
+	scu_ic->split_ier_isr = true;
+	scu_ic->en_reg = ASPEED_AST2700_SCU_IC2_EN_REG;
+	scu_ic->sts_reg = ASPEED_AST2700_SCU_IC2_STS_REG;
+
+	return aspeed_scu_ic_of_init_common(scu_ic, node);
+}
+
+static int __init aspeed_ast2700_scu_ic3_of_init(struct device_node *node,
+						 struct device_node *parent)
+{
+	struct aspeed_scu_ic *scu_ic = kzalloc(sizeof(*scu_ic), GFP_KERNEL);
+
+	if (!scu_ic)
+		return -ENOMEM;
+
+	scu_ic->irq_enable = ASPEED_AST2700_SCU_IC3_ENABLE;
+	scu_ic->irq_shift = ASPEED_AST2700_SCU_IC3_SHIFT;
+	scu_ic->num_irqs = ASPEED_AST2700_SCU_IC3_NUM_IRQS;
+	scu_ic->split_ier_isr = true;
+	scu_ic->en_reg = ASPEED_AST2700_SCU_IC3_EN_REG;
+	scu_ic->sts_reg = ASPEED_AST2700_SCU_IC3_STS_REG;
+
+	return aspeed_scu_ic_of_init_common(scu_ic, node);
+}
+
 IRQCHIP_DECLARE(ast2400_scu_ic, "aspeed,ast2400-scu-ic", aspeed_scu_ic_of_init);
 IRQCHIP_DECLARE(ast2500_scu_ic, "aspeed,ast2500-scu-ic", aspeed_scu_ic_of_init);
 IRQCHIP_DECLARE(ast2600_scu_ic0, "aspeed,ast2600-scu-ic0",
 		aspeed_ast2600_scu_ic0_of_init);
 IRQCHIP_DECLARE(ast2600_scu_ic1, "aspeed,ast2600-scu-ic1",
 		aspeed_ast2600_scu_ic1_of_init);
+IRQCHIP_DECLARE(ast2700_scu_ic0, "aspeed,ast2700-scu-ic0",
+		aspeed_ast2700_scu_ic0_of_init);
+IRQCHIP_DECLARE(ast2700_scu_ic1, "aspeed,ast2700-scu-ic1",
+		aspeed_ast2700_scu_ic1_of_init);
+IRQCHIP_DECLARE(ast2700_scu_ic2, "aspeed,ast2700-scu-ic2",
+		aspeed_ast2700_scu_ic2_of_init);
+IRQCHIP_DECLARE(ast2700_scu_ic3, "aspeed,ast2700-scu-ic3",
+		aspeed_ast2700_scu_ic3_of_init);
-- 
2.34.1

Re: [PATCH 2/2] irqchip/aspeed-scu-ic: Add support for AST2700 SCU interrupt controllers

[Thomas Gleixner]

On Mon, Aug 04 2025 at 13:34, Ryan Chen wrote:
> +#define ASPEED_AST2700_SCU_IC0_EN_REG	0x1d0
> +#define ASPEED_AST2700_SCU_IC0_STS_REG	0x1d4
> +#define ASPEED_AST2700_SCU_IC0_SHIFT	0
> +#define ASPEED_AST2700_SCU_IC0_ENABLE	\
> +	GENMASK(3, ASPEED_AST2700_SCU_IC0_SHIFT)

Let it stick out, you have 100 characters

>  struct aspeed_scu_ic {
>  	unsigned long irq_enable;
>  	unsigned long irq_shift;
>  	unsigned int num_irqs;
>  	unsigned int reg;
> +	unsigned int en_reg;
> +	unsigned int sts_reg;
> +	bool split_ier_isr;

Please reformat this struct declaration according to:

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

>  	struct regmap *scu;
>  	struct irq_domain *irq_domain;
>  };
> @@ -52,33 +83,51 @@ static void aspeed_scu_ic_irq_handler(struct irq_desc *desc)
>  	unsigned long status;
>  	struct aspeed_scu_ic *scu_ic = irq_desc_get_handler_data(desc);
>  	struct irq_chip *chip = irq_desc_get_chip(desc);
> -	unsigned int mask = scu_ic->irq_enable << ASPEED_SCU_IC_STATUS_SHIFT;
> +	unsigned int mask;

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

>  	chained_irq_enter(chip, desc);
>  
> -	/*
> -	 * The SCU IC has just one register to control its operation and read
> -	 * status. The interrupt enable bits occupy the lower 16 bits of the
> -	 * register, while the interrupt status bits occupy the upper 16 bits.
> -	 * The status bit for a given interrupt is always 16 bits shifted from
> -	 * the enable bit for the same interrupt.
> -	 * Therefore, perform the IRQ operations in the enable bit space by
> -	 * shifting the status down to get the mapping and then back up to
> -	 * clear the bit.
> -	 */
> -	regmap_read(scu_ic->scu, scu_ic->reg, &sts);
> -	enabled = sts & scu_ic->irq_enable;
> -	status = (sts >> ASPEED_SCU_IC_STATUS_SHIFT) & enabled;
> -
> -	bit = scu_ic->irq_shift;
> -	max = scu_ic->num_irqs + bit;
> -
> -	for_each_set_bit_from(bit, &status, max) {
> -		generic_handle_domain_irq(scu_ic->irq_domain,
> -					  bit - scu_ic->irq_shift);
> -
> -		regmap_write_bits(scu_ic->scu, scu_ic->reg, mask,
> -				  BIT(bit + ASPEED_SCU_IC_STATUS_SHIFT));
> +	if (scu_ic->split_ier_isr) {
> +		mask = scu_ic->irq_enable;
> +		regmap_read(scu_ic->scu, scu_ic->en_reg, &sts);
> +		enabled = sts & scu_ic->irq_enable;
> +		regmap_read(scu_ic->scu, scu_ic->sts_reg, &sts);
> +		status = sts & enabled;
> +
> +		bit = scu_ic->irq_shift;
> +		max = scu_ic->num_irqs + bit;
> +
> +		for_each_set_bit_from(bit, &status, max) {
> +			generic_handle_domain_irq(scu_ic->irq_domain, bit - scu_ic->irq_shift);
> +
> +			regmap_write_bits(scu_ic->scu, scu_ic->sts_reg, mask, BIT(bit));
> +		}
> +	} else {
> +		mask = scu_ic->irq_enable << ASPEED_SCU_IC_STATUS_SHIFT;
> +		/*
> +		 * The SCU IC has just one register to control its operation and read
> +		 * status. The interrupt enable bits occupy the lower 16 bits of the
> +		 * register, while the interrupt status bits occupy the upper 16 bits.
> +		 * The status bit for a given interrupt is always 16 bits shifted from
> +		 * the enable bit for the same interrupt.
> +		 * Therefore, perform the IRQ operations in the enable bit space by
> +		 * shifting the status down to get the mapping and then back up to
> +		 * clear the bit.
> +		 */
> +		regmap_read(scu_ic->scu, scu_ic->reg, &sts);
> +		enabled = sts & scu_ic->irq_enable;
> +		status = (sts >> ASPEED_SCU_IC_STATUS_SHIFT) & enabled;
> +
> +		bit = scu_ic->irq_shift;
> +		max = scu_ic->num_irqs + bit;
> +
> +		for_each_set_bit_from(bit, &status, max) {
> +			generic_handle_domain_irq(scu_ic->irq_domain,
> +						  bit - scu_ic->irq_shift);
> +
> +			regmap_write_bits(scu_ic->scu, scu_ic->reg, mask,
> +					  BIT(bit + ASPEED_SCU_IC_STATUS_SHIFT));
> +		}

This is horrible, really. Either rework the code so that both chips can
share it with minimal conditionals or provide seperate handlers. It's
not rocket science.
  
>  	chained_irq_exit(chip, desc);
> @@ -87,30 +136,42 @@ static void aspeed_scu_ic_irq_handler(struct irq_desc *desc)

>  static void aspeed_scu_ic_irq_mask(struct irq_data *data)
>  static void aspeed_scu_ic_irq_unmask(struct irq_data *data)

>  {
>  	struct aspeed_scu_ic *scu_ic = irq_data_get_irq_chip_data(data);
>  	unsigned int bit = BIT(data->hwirq + scu_ic->irq_shift);
> -	unsigned int mask = bit |
> -		(scu_ic->irq_enable << ASPEED_SCU_IC_STATUS_SHIFT);
> -
> -	/*
> -	 * Status bits are cleared by writing 1. In order to prevent the unmask
> -	 * operation from clearing the status bits, they should be under the
> -	 * mask and written with 0.
> -	 */
> -	regmap_update_bits(scu_ic->scu, scu_ic->reg, mask, bit);
> +	unsigned int mask;
> +
> +	if (scu_ic->split_ier_isr) {
> +		mask = bit;
> +		regmap_update_bits(scu_ic->scu, scu_ic->en_reg, mask, bit);
> +	} else {
> +		mask = bit | (scu_ic->irq_enable << ASPEED_SCU_IC_STATUS_SHIFT);
> +
> +		/*
> +		 * Status bits are cleared by writing 1. In order to prevent the unmask
> +		 * operation from clearing the status bits, they should be under the
> +		 * mask and written with 0.
> +		 */
> +		regmap_update_bits(scu_ic->scu, scu_ic->reg, mask, bit);
> +	}

This also wants to be consolidated or seperated.

>  }
>  
> +static int __init aspeed_ast2700_scu_ic0_of_init(struct device_node *node,
> +						 struct device_node *parent)
> +{
> +	struct aspeed_scu_ic *scu_ic = kzalloc(sizeof(*scu_ic), GFP_KERNEL);
> +
> +	if (!scu_ic)
> +		return -ENOMEM;
> +
> +	scu_ic->irq_enable = ASPEED_AST2700_SCU_IC0_ENABLE;
> +	scu_ic->irq_shift = ASPEED_AST2700_SCU_IC0_SHIFT;
> +	scu_ic->num_irqs = ASPEED_AST2700_SCU_IC0_NUM_IRQS;
> +	scu_ic->split_ier_isr = true;
> +	scu_ic->en_reg = ASPEED_AST2700_SCU_IC0_EN_REG;
> +	scu_ic->sts_reg = ASPEED_AST2700_SCU_IC0_STS_REG;
> +
> +	return aspeed_scu_ic_of_init_common(scu_ic, node);
> +}
> +
> +static int __init aspeed_ast2700_scu_ic1_of_init(struct device_node *node,
> +						 struct device_node *parent)
> +{
> +	struct aspeed_scu_ic *scu_ic = kzalloc(sizeof(*scu_ic), GFP_KERNEL);
> +
> +	if (!scu_ic)
> +		return -ENOMEM;
> +
> +	scu_ic->irq_enable = ASPEED_AST2700_SCU_IC1_ENABLE;
> +	scu_ic->irq_shift = ASPEED_AST2700_SCU_IC1_SHIFT;
> +	scu_ic->num_irqs = ASPEED_AST2700_SCU_IC1_NUM_IRQS;
> +	scu_ic->split_ier_isr = true;
> +	scu_ic->en_reg = ASPEED_AST2700_SCU_IC1_EN_REG;
> +	scu_ic->sts_reg = ASPEED_AST2700_SCU_IC1_STS_REG;
> +
> +	return aspeed_scu_ic_of_init_common(scu_ic, node);
> +}
> +
> +static int __init aspeed_ast2700_scu_ic2_of_init(struct device_node *node,
> +						 struct device_node *parent)
> +{
> +	struct aspeed_scu_ic *scu_ic = kzalloc(sizeof(*scu_ic), GFP_KERNEL);
> +
> +	if (!scu_ic)
> +		return -ENOMEM;
> +
> +	scu_ic->irq_enable = ASPEED_AST2700_SCU_IC2_ENABLE;
> +	scu_ic->irq_shift = ASPEED_AST2700_SCU_IC2_SHIFT;
> +	scu_ic->num_irqs = ASPEED_AST2700_SCU_IC2_NUM_IRQS;
> +	scu_ic->split_ier_isr = true;
> +	scu_ic->en_reg = ASPEED_AST2700_SCU_IC2_EN_REG;
> +	scu_ic->sts_reg = ASPEED_AST2700_SCU_IC2_STS_REG;
> +
> +	return aspeed_scu_ic_of_init_common(scu_ic, node);
> +}
> +
> +static int __init aspeed_ast2700_scu_ic3_of_init(struct device_node *node,
> +						 struct device_node *parent)
> +{
> +	struct aspeed_scu_ic *scu_ic = kzalloc(sizeof(*scu_ic), GFP_KERNEL);
> +
> +	if (!scu_ic)
> +		return -ENOMEM;
> +
> +	scu_ic->irq_enable = ASPEED_AST2700_SCU_IC3_ENABLE;
> +	scu_ic->irq_shift = ASPEED_AST2700_SCU_IC3_SHIFT;
> +	scu_ic->num_irqs = ASPEED_AST2700_SCU_IC3_NUM_IRQS;
> +	scu_ic->split_ier_isr = true;
> +	scu_ic->en_reg = ASPEED_AST2700_SCU_IC3_EN_REG;
> +	scu_ic->sts_reg = ASPEED_AST2700_SCU_IC3_STS_REG;
> +
> +	return aspeed_scu_ic_of_init_common(scu_ic, node);
> +}

You seriously have no better idea than this copy & pasta orgy?

static struct scu_variant variants = {
	SCU("aspeed,ast2400-scu-ic", ......, whatever...),
        ...
	SCU("aspeed,ast2700-scu-ic0", 0x1D0, 0x1D4, 0, GENMASK(3, 0),
            4, whatever...),
        ...
	SCU("aspeed,ast2700-scu-ic3", 0x10C, 0x108, 0, GENMASK(1, 0),
            2, ......),
} __initdata;

static struct scu_variant *find_variant(struct device_node *node)
{
        for (int i = 0; i < ARRAY_SIZE(variant); i++) {
        	if (!strcmp(variant[i].name, node->name))
                	return &variant[i];
	}                               
        return NULL;
}

static int __init ast_scu_of_init(struct device_node *node, struct device_node *parent)
{
        struct variant *v = find_variant(node);

        if (!v)
          	...

        scu_ic = kzalloc(...);
        *scu_ic = v->scu;
        ...

You get the idea.

This rework needs to come first and then you can add your new 2700 muck
on top.

Thanks,

        tglx

RE: [PATCH 2/2] irqchip/aspeed-scu-ic: Add support for AST2700 SCU interrupt controllers

[Ryan Chen]

> Subject: Re: [PATCH 2/2] irqchip/aspeed-scu-ic: Add support for AST2700 SCU
> interrupt controllers
> 
> On Mon, Aug 04 2025 at 13:34, Ryan Chen wrote:
> > +#define ASPEED_AST2700_SCU_IC0_EN_REG	0x1d0
> > +#define ASPEED_AST2700_SCU_IC0_STS_REG	0x1d4
> > +#define ASPEED_AST2700_SCU_IC0_SHIFT	0
> > +#define ASPEED_AST2700_SCU_IC0_ENABLE	\
> > +	GENMASK(3, ASPEED_AST2700_SCU_IC0_SHIFT)
> 
> Let it stick out, you have 100 characters
Yes, will update to
#define ASPEED_AST2700_SCU_IC0_ENABLE   GENMASK(3, ASPEED_AST2700_SCU_IC0_SHIFT)
And another question, in original code define for AST2600, can I also modify it in this patch? 
or I should be another patch?
#define ASPEED_AST2600_SCU_IC1_ENABLE   \
        GENMASK(5, ASPEED_AST2600_SCU_IC1_SHIFT)
> 
> >  struct aspeed_scu_ic {
> >  	unsigned long irq_enable;
> >  	unsigned long irq_shift;
> >  	unsigned int num_irqs;
> >  	unsigned int reg;
> > +	unsigned int en_reg;
> > +	unsigned int sts_reg;
> > +	bool split_ier_isr;
> 
> Please reformat this struct declaration according to:
> 
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#struct-de
> clarations-and-initializers

Will update struct aspeed_scu_ic to following.

struct aspeed_scu_ic {
        unsigned long           irq_enable;
        unsigned long           irq_shift;
        unsigned int            num_irqs;
        unsigned int            reg;
        unsigned int            en_reg;
        unsigned int            sts_reg;
        bool                    split_ier_isr;
        struct regmap           *scu;
        struct irq_domain       *irq_domain;
};
> 
> >  	struct regmap *scu;
> >  	struct irq_domain *irq_domain;
> >  };
> > @@ -52,33 +83,51 @@ static void aspeed_scu_ic_irq_handler(struct
> irq_desc *desc)
> >  	unsigned long status;
> >  	struct aspeed_scu_ic *scu_ic = irq_desc_get_handler_data(desc);
> >  	struct irq_chip *chip = irq_desc_get_chip(desc);
> > -	unsigned int mask = scu_ic->irq_enable <<
> ASPEED_SCU_IC_STATUS_SHIFT;
> > +	unsigned int mask;
> 
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#variable-
> declarations

Will update by following.

struct aspeed_scu_ic *scu_ic;
struct irq_chip *chip;
unsigned long bit, enabled, max, status;
unsigned int sts, mask;

scu_ic = irq_desc_get_handler_data(desc);
chip = irq_desc_get_chip(desc);

> 
> >  	chained_irq_enter(chip, desc);
> >
> > -	/*
> > -	 * The SCU IC has just one register to control its operation and read
> > -	 * status. The interrupt enable bits occupy the lower 16 bits of the
> > -	 * register, while the interrupt status bits occupy the upper 16 bits.
> > -	 * The status bit for a given interrupt is always 16 bits shifted from
> > -	 * the enable bit for the same interrupt.
> > -	 * Therefore, perform the IRQ operations in the enable bit space by
> > -	 * shifting the status down to get the mapping and then back up to
> > -	 * clear the bit.
> > -	 */
> > -	regmap_read(scu_ic->scu, scu_ic->reg, &sts);
> > -	enabled = sts & scu_ic->irq_enable;
> > -	status = (sts >> ASPEED_SCU_IC_STATUS_SHIFT) & enabled;
> > -
> > -	bit = scu_ic->irq_shift;
> > -	max = scu_ic->num_irqs + bit;
> > -
> > -	for_each_set_bit_from(bit, &status, max) {
> > -		generic_handle_domain_irq(scu_ic->irq_domain,
> > -					  bit - scu_ic->irq_shift);
> > -
> > -		regmap_write_bits(scu_ic->scu, scu_ic->reg, mask,
> > -				  BIT(bit + ASPEED_SCU_IC_STATUS_SHIFT));
> > +	if (scu_ic->split_ier_isr) {
> > +		mask = scu_ic->irq_enable;
> > +		regmap_read(scu_ic->scu, scu_ic->en_reg, &sts);
> > +		enabled = sts & scu_ic->irq_enable;
> > +		regmap_read(scu_ic->scu, scu_ic->sts_reg, &sts);
> > +		status = sts & enabled;
> > +
> > +		bit = scu_ic->irq_shift;
> > +		max = scu_ic->num_irqs + bit;
> > +
> > +		for_each_set_bit_from(bit, &status, max) {
> > +			generic_handle_domain_irq(scu_ic->irq_domain, bit -
> > +scu_ic->irq_shift);
> > +
> > +			regmap_write_bits(scu_ic->scu, scu_ic->sts_reg, mask, BIT(bit));
> > +		}
> > +	} else {
> > +		mask = scu_ic->irq_enable << ASPEED_SCU_IC_STATUS_SHIFT;
> > +		/*
> > +		 * The SCU IC has just one register to control its operation and read
> > +		 * status. The interrupt enable bits occupy the lower 16 bits of the
> > +		 * register, while the interrupt status bits occupy the upper 16 bits.
> > +		 * The status bit for a given interrupt is always 16 bits shifted from
> > +		 * the enable bit for the same interrupt.
> > +		 * Therefore, perform the IRQ operations in the enable bit space by
> > +		 * shifting the status down to get the mapping and then back up to
> > +		 * clear the bit.
> > +		 */
> > +		regmap_read(scu_ic->scu, scu_ic->reg, &sts);
> > +		enabled = sts & scu_ic->irq_enable;
> > +		status = (sts >> ASPEED_SCU_IC_STATUS_SHIFT) & enabled;
> > +
> > +		bit = scu_ic->irq_shift;
> > +		max = scu_ic->num_irqs + bit;
> > +
> > +		for_each_set_bit_from(bit, &status, max) {
> > +			generic_handle_domain_irq(scu_ic->irq_domain,
> > +						  bit - scu_ic->irq_shift);
> > +
> > +			regmap_write_bits(scu_ic->scu, scu_ic->reg, mask,
> > +					  BIT(bit + ASPEED_SCU_IC_STATUS_SHIFT));
> > +		}
> 
> This is horrible, really. Either rework the code so that both chips can share it
> with minimal conditionals or provide seperate handlers. It's not rocket science.

Yes, I will update by separate handlers.

older
static struct irq_chip aspeed_scu_ic_chip = {
	.name			= "aspeed-scu-ic",
	.irq_mask		= aspeed_scu_ic_irq_mask,
	.irq_unmask		= aspeed_scu_ic_irq_unmask,
	.irq_set_affinity	= aspeed_scu_ic_irq_set_affinity,
};
Add new 

static struct irq_chip ast2700_scu_ic_chip = {
	.name			= "aspeed-scu-ic",
	.irq_mask		= ast2700_scu_ic_irq_mask,
	.irq_unmask		= ast2700_scu_ic_irq_unmask,
	.irq_set_affinity	= ast2700_scu_ic_irq_set_affinity,
};

if (scu_ic->split_ier_isr)
    irq_set_chained_handler_and_data(irq, ast2700_scu_ic_irq_handler, scu_ic);
else
    irq_set_chained_handler_and_data(irq, aspeed_scu_ic_irq_handler,, scu_ic);

> 
> >  	chained_irq_exit(chip, desc);
> > @@ -87,30 +136,42 @@ static void aspeed_scu_ic_irq_handler(struct
> > irq_desc *desc)
> 
> >  static void aspeed_scu_ic_irq_mask(struct irq_data *data)  static
> > void aspeed_scu_ic_irq_unmask(struct irq_data *data)
> 
> >  {
> >  	struct aspeed_scu_ic *scu_ic = irq_data_get_irq_chip_data(data);
> >  	unsigned int bit = BIT(data->hwirq + scu_ic->irq_shift);
> > -	unsigned int mask = bit |
> > -		(scu_ic->irq_enable << ASPEED_SCU_IC_STATUS_SHIFT);
> > -
> > -	/*
> > -	 * Status bits are cleared by writing 1. In order to prevent the unmask
> > -	 * operation from clearing the status bits, they should be under the
> > -	 * mask and written with 0.
> > -	 */
> > -	regmap_update_bits(scu_ic->scu, scu_ic->reg, mask, bit);
> > +	unsigned int mask;
> > +
> > +	if (scu_ic->split_ier_isr) {
> > +		mask = bit;
> > +		regmap_update_bits(scu_ic->scu, scu_ic->en_reg, mask, bit);
> > +	} else {
> > +		mask = bit | (scu_ic->irq_enable << ASPEED_SCU_IC_STATUS_SHIFT);
> > +
> > +		/*
> > +		 * Status bits are cleared by writing 1. In order to prevent the
> unmask
> > +		 * operation from clearing the status bits, they should be under the
> > +		 * mask and written with 0.
> > +		 */
> > +		regmap_update_bits(scu_ic->scu, scu_ic->reg, mask, bit);
> > +	}
> 
> This also wants to be consolidated or seperated.
Yes, will do separated.
> 
> >  }
> >
> > +static int __init aspeed_ast2700_scu_ic0_of_init(struct device_node *node,
> > +						 struct device_node *parent)
> > +{
> > +	struct aspeed_scu_ic *scu_ic = kzalloc(sizeof(*scu_ic), GFP_KERNEL);
> > +
> > +	if (!scu_ic)
> > +		return -ENOMEM;
> > +
> > +	scu_ic->irq_enable = ASPEED_AST2700_SCU_IC0_ENABLE;
> > +	scu_ic->irq_shift = ASPEED_AST2700_SCU_IC0_SHIFT;
> > +	scu_ic->num_irqs = ASPEED_AST2700_SCU_IC0_NUM_IRQS;
> > +	scu_ic->split_ier_isr = true;
> > +	scu_ic->en_reg = ASPEED_AST2700_SCU_IC0_EN_REG;
> > +	scu_ic->sts_reg = ASPEED_AST2700_SCU_IC0_STS_REG;
> > +
> > +	return aspeed_scu_ic_of_init_common(scu_ic, node); }
> > +
> > +static int __init aspeed_ast2700_scu_ic1_of_init(struct device_node *node,
> > +						 struct device_node *parent)
> > +{
> > +	struct aspeed_scu_ic *scu_ic = kzalloc(sizeof(*scu_ic), GFP_KERNEL);
> > +
> > +	if (!scu_ic)
> > +		return -ENOMEM;
> > +
> > +	scu_ic->irq_enable = ASPEED_AST2700_SCU_IC1_ENABLE;
> > +	scu_ic->irq_shift = ASPEED_AST2700_SCU_IC1_SHIFT;
> > +	scu_ic->num_irqs = ASPEED_AST2700_SCU_IC1_NUM_IRQS;
> > +	scu_ic->split_ier_isr = true;
> > +	scu_ic->en_reg = ASPEED_AST2700_SCU_IC1_EN_REG;
> > +	scu_ic->sts_reg = ASPEED_AST2700_SCU_IC1_STS_REG;
> > +
> > +	return aspeed_scu_ic_of_init_common(scu_ic, node); }
> > +
> > +static int __init aspeed_ast2700_scu_ic2_of_init(struct device_node *node,
> > +						 struct device_node *parent)
> > +{
> > +	struct aspeed_scu_ic *scu_ic = kzalloc(sizeof(*scu_ic), GFP_KERNEL);
> > +
> > +	if (!scu_ic)
> > +		return -ENOMEM;
> > +
> > +	scu_ic->irq_enable = ASPEED_AST2700_SCU_IC2_ENABLE;
> > +	scu_ic->irq_shift = ASPEED_AST2700_SCU_IC2_SHIFT;
> > +	scu_ic->num_irqs = ASPEED_AST2700_SCU_IC2_NUM_IRQS;
> > +	scu_ic->split_ier_isr = true;
> > +	scu_ic->en_reg = ASPEED_AST2700_SCU_IC2_EN_REG;
> > +	scu_ic->sts_reg = ASPEED_AST2700_SCU_IC2_STS_REG;
> > +
> > +	return aspeed_scu_ic_of_init_common(scu_ic, node); }
> > +
> > +static int __init aspeed_ast2700_scu_ic3_of_init(struct device_node *node,
> > +						 struct device_node *parent)
> > +{
> > +	struct aspeed_scu_ic *scu_ic = kzalloc(sizeof(*scu_ic), GFP_KERNEL);
> > +
> > +	if (!scu_ic)
> > +		return -ENOMEM;
> > +
> > +	scu_ic->irq_enable = ASPEED_AST2700_SCU_IC3_ENABLE;
> > +	scu_ic->irq_shift = ASPEED_AST2700_SCU_IC3_SHIFT;
> > +	scu_ic->num_irqs = ASPEED_AST2700_SCU_IC3_NUM_IRQS;
> > +	scu_ic->split_ier_isr = true;
> > +	scu_ic->en_reg = ASPEED_AST2700_SCU_IC3_EN_REG;
> > +	scu_ic->sts_reg = ASPEED_AST2700_SCU_IC3_STS_REG;
> > +
> > +	return aspeed_scu_ic_of_init_common(scu_ic, node); }
> 
> You seriously have no better idea than this copy & pasta orgy?
> 
> static struct scu_variant variants = {
> 	SCU("aspeed,ast2400-scu-ic", ......, whatever...),
>         ...
> 	SCU("aspeed,ast2700-scu-ic0", 0x1D0, 0x1D4, 0, GENMASK(3, 0),
>             4, whatever...),
>         ...
> 	SCU("aspeed,ast2700-scu-ic3", 0x10C, 0x108, 0, GENMASK(1, 0),
>             2, ......),
> } __initdata;
> 
> static struct scu_variant *find_variant(struct device_node *node) {
>         for (int i = 0; i < ARRAY_SIZE(variant); i++) {
>         	if (!strcmp(variant[i].name, node->name))
>                 	return &variant[i];
> 	}
>         return NULL;
> }
> 
> static int __init ast_scu_of_init(struct device_node *node, struct device_node
> *parent) {
>         struct variant *v = find_variant(node);
> 
>         if (!v)
>           	...
> 
>         scu_ic = kzalloc(...);
>         *scu_ic = v->scu;
>         ...
> 
> You get the idea.
> 
Thanks, I will do rework first, then add AST2700. 

> This rework needs to come first and then you can add your new 2700 muck on
> top.
> 
> Thanks,
> 
>         tglx

Re: [PATCH 2/2] irqchip/aspeed-scu-ic: Add support for AST2700 SCU interrupt controllers

[Rob Herring]

On Mon, Aug 04, 2025 at 01:34:45PM +0800, Ryan Chen wrote:
> The AST2700 SoC follows the multi-instance interrupt controller architecture
> introduced in the AST2600, where each SCU interrupt group (IC0–IC3) is treated
> as an independent interrupt domain.
> 
> Unlike the AST2600, which uses a combined register for interrupt enable and
> status bits, the AST2700 separates these into distinct registers: one for
> interrupt enable (IER) and another for interrupt status (ISR). This architectural
> change requires explicit handling of split registers for interrupt control.
> 
> - Register definitions and configuration for AST2700 SCU IC instances
>   (compatible: aspeed,ast2700-scu-ic0/1/2/3)
> - Initialization logic for handling split IER/ISR registers
> - Chained IRQ handling and mask/unmask logic
> - Table-driven registration using IRQCHIP_DECLARE per compatible
> 
> Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
> ---
>  drivers/irqchip/irq-aspeed-scu-ic.c | 240 ++++++++++++++++++++++------
>  1 file changed, 195 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-aspeed-scu-ic.c b/drivers/irqchip/irq-aspeed-scu-ic.c
> index 1c7045467c48..b6f3ba269c5b 100644
> --- a/drivers/irqchip/irq-aspeed-scu-ic.c
> +++ b/drivers/irqchip/irq-aspeed-scu-ic.c
> @@ -1,6 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0-or-later
>  /*
> - * Aspeed AST24XX, AST25XX, and AST26XX SCU Interrupt Controller
> + * Aspeed AST24XX, AST25XX, AST26XX, AST27XX SCU Interrupt Controller
>   * Copyright 2019 IBM Corporation
>   *
>   * Eddie James <eajames@linux.ibm.com>
> @@ -34,11 +34,42 @@
>  	GENMASK(5, ASPEED_AST2600_SCU_IC1_SHIFT)
>  #define ASPEED_AST2600_SCU_IC1_NUM_IRQS	2
>  
> +#define ASPEED_AST2700_SCU_IC0_EN_REG	0x1d0
> +#define ASPEED_AST2700_SCU_IC0_STS_REG	0x1d4
> +#define ASPEED_AST2700_SCU_IC0_SHIFT	0
> +#define ASPEED_AST2700_SCU_IC0_ENABLE	\
> +	GENMASK(3, ASPEED_AST2700_SCU_IC0_SHIFT)
> +#define ASPEED_AST2700_SCU_IC0_NUM_IRQS	4
> +
> +#define ASPEED_AST2700_SCU_IC1_EN_REG	0x1e0
> +#define ASPEED_AST2700_SCU_IC1_STS_REG	0x1e4
> +#define ASPEED_AST2700_SCU_IC1_SHIFT	0
> +#define ASPEED_AST2700_SCU_IC1_ENABLE	\
> +	GENMASK(3, ASPEED_AST2700_SCU_IC1_SHIFT)
> +#define ASPEED_AST2700_SCU_IC1_NUM_IRQS	4
> +
> +#define ASPEED_AST2700_SCU_IC2_EN_REG	0x104
> +#define ASPEED_AST2700_SCU_IC2_STS_REG	0x100
> +#define ASPEED_AST2700_SCU_IC2_SHIFT	0
> +#define ASPEED_AST2700_SCU_IC2_ENABLE	\
> +	GENMASK(3, ASPEED_AST2700_SCU_IC2_SHIFT)
> +#define ASPEED_AST2700_SCU_IC2_NUM_IRQS	4
> +
> +#define ASPEED_AST2700_SCU_IC3_EN_REG	0x10c
> +#define ASPEED_AST2700_SCU_IC3_STS_REG	0x108
> +#define ASPEED_AST2700_SCU_IC3_SHIFT	0
> +#define ASPEED_AST2700_SCU_IC3_ENABLE	\
> +	GENMASK(1, ASPEED_AST2700_SCU_IC3_SHIFT)
> +#define ASPEED_AST2700_SCU_IC3_NUM_IRQS	2
> +

The reason for ic0/ic1 compatibles before was the enable field was 
different. Now it's at least at the same shift. Do you really need a 
different value for IC3? 

The register addresses should come from "reg". I don't understand why 
they are hardcoded in the driver.

Rob

RE: [PATCH 2/2] irqchip/aspeed-scu-ic: Add support for AST2700 SCU interrupt controllers

[Ryan Chen]

> Subject: Re: [PATCH 2/2] irqchip/aspeed-scu-ic: Add support for AST2700 SCU
> interrupt controllers
> 
> On Mon, Aug 04, 2025 at 01:34:45PM +0800, Ryan Chen wrote:
> > The AST2700 SoC follows the multi-instance interrupt controller
> > architecture introduced in the AST2600, where each SCU interrupt group
> > (IC0–IC3) is treated as an independent interrupt domain.
> >
> > Unlike the AST2600, which uses a combined register for interrupt
> > enable and status bits, the AST2700 separates these into distinct
> > registers: one for interrupt enable (IER) and another for interrupt
> > status (ISR). This architectural change requires explicit handling of split
> registers for interrupt control.
> >
> > - Register definitions and configuration for AST2700 SCU IC instances
> >   (compatible: aspeed,ast2700-scu-ic0/1/2/3)
> > - Initialization logic for handling split IER/ISR registers
> > - Chained IRQ handling and mask/unmask logic
> > - Table-driven registration using IRQCHIP_DECLARE per compatible
> >
> > Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
> > ---
> >  drivers/irqchip/irq-aspeed-scu-ic.c | 240
> > ++++++++++++++++++++++------
> >  1 file changed, 195 insertions(+), 45 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-aspeed-scu-ic.c
> > b/drivers/irqchip/irq-aspeed-scu-ic.c
> > index 1c7045467c48..b6f3ba269c5b 100644
> > --- a/drivers/irqchip/irq-aspeed-scu-ic.c
> > +++ b/drivers/irqchip/irq-aspeed-scu-ic.c
> > @@ -1,6 +1,6 @@
> >  // SPDX-License-Identifier: GPL-2.0-or-later
> >  /*
> > - * Aspeed AST24XX, AST25XX, and AST26XX SCU Interrupt Controller
> > + * Aspeed AST24XX, AST25XX, AST26XX, AST27XX SCU Interrupt Controller
> >   * Copyright 2019 IBM Corporation
> >   *
> >   * Eddie James <eajames@linux.ibm.com> @@ -34,11 +34,42 @@
> >  	GENMASK(5, ASPEED_AST2600_SCU_IC1_SHIFT)
> >  #define ASPEED_AST2600_SCU_IC1_NUM_IRQS	2
> >
> > +#define ASPEED_AST2700_SCU_IC0_EN_REG	0x1d0
> > +#define ASPEED_AST2700_SCU_IC0_STS_REG	0x1d4
> > +#define ASPEED_AST2700_SCU_IC0_SHIFT	0
> > +#define ASPEED_AST2700_SCU_IC0_ENABLE	\
> > +	GENMASK(3, ASPEED_AST2700_SCU_IC0_SHIFT)
> > +#define ASPEED_AST2700_SCU_IC0_NUM_IRQS	4
> > +
> > +#define ASPEED_AST2700_SCU_IC1_EN_REG	0x1e0
> > +#define ASPEED_AST2700_SCU_IC1_STS_REG	0x1e4
> > +#define ASPEED_AST2700_SCU_IC1_SHIFT	0
> > +#define ASPEED_AST2700_SCU_IC1_ENABLE	\
> > +	GENMASK(3, ASPEED_AST2700_SCU_IC1_SHIFT)
> > +#define ASPEED_AST2700_SCU_IC1_NUM_IRQS	4
> > +
> > +#define ASPEED_AST2700_SCU_IC2_EN_REG	0x104
> > +#define ASPEED_AST2700_SCU_IC2_STS_REG	0x100
> > +#define ASPEED_AST2700_SCU_IC2_SHIFT	0
> > +#define ASPEED_AST2700_SCU_IC2_ENABLE	\
> > +	GENMASK(3, ASPEED_AST2700_SCU_IC2_SHIFT)
> > +#define ASPEED_AST2700_SCU_IC2_NUM_IRQS	4
> > +
> > +#define ASPEED_AST2700_SCU_IC3_EN_REG	0x10c
> > +#define ASPEED_AST2700_SCU_IC3_STS_REG	0x108
> > +#define ASPEED_AST2700_SCU_IC3_SHIFT	0
> > +#define ASPEED_AST2700_SCU_IC3_ENABLE	\
> > +	GENMASK(1, ASPEED_AST2700_SCU_IC3_SHIFT)
> > +#define ASPEED_AST2700_SCU_IC3_NUM_IRQS	2
> > +
> 
> The reason for ic0/ic1 compatibles before was the enable field was different.
> Now it's at least at the same shift. Do you really need a different value for IC3?
> 
OK, I can remove this define.

> The register addresses should come from "reg". I don't understand why they
> are hardcoded in the driver.
The original code register is come from parent. scu_ic->scu = syscon_node_to_regmap(node->parent);
I keep the original code logic, and add AST2700.
> 
> Rob

Re: [PATCH 2/2] irqchip/aspeed-scu-ic: Add support for AST2700 SCU interrupt controllers

[Eddie James]

On 8/6/25 2:14 AM, Ryan Chen wrote:
>> Subject: Re: [PATCH 2/2] irqchip/aspeed-scu-ic: Add support for AST2700 SCU
>> interrupt controllers
>>
>> On Mon, Aug 04, 2025 at 01:34:45PM +0800, Ryan Chen wrote:
>>> The AST2700 SoC follows the multi-instance interrupt controller
>>> architecture introduced in the AST2600, where each SCU interrupt group
>>> (IC0–IC3) is treated as an independent interrupt domain.
>>>
>>> Unlike the AST2600, which uses a combined register for interrupt
>>> enable and status bits, the AST2700 separates these into distinct
>>> registers: one for interrupt enable (IER) and another for interrupt
>>> status (ISR). This architectural change requires explicit handling of split
>> registers for interrupt control.
>>> - Register definitions and configuration for AST2700 SCU IC instances
>>>    (compatible: aspeed,ast2700-scu-ic0/1/2/3)
>>> - Initialization logic for handling split IER/ISR registers
>>> - Chained IRQ handling and mask/unmask logic
>>> - Table-driven registration using IRQCHIP_DECLARE per compatible
>>>
>>> Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
>>> ---
>>>   drivers/irqchip/irq-aspeed-scu-ic.c | 240
>>> ++++++++++++++++++++++------
>>>   1 file changed, 195 insertions(+), 45 deletions(-)
>>>
>>> diff --git a/drivers/irqchip/irq-aspeed-scu-ic.c
>>> b/drivers/irqchip/irq-aspeed-scu-ic.c
>>> index 1c7045467c48..b6f3ba269c5b 100644
>>> --- a/drivers/irqchip/irq-aspeed-scu-ic.c
>>> +++ b/drivers/irqchip/irq-aspeed-scu-ic.c
>>> @@ -1,6 +1,6 @@
>>>   // SPDX-License-Identifier: GPL-2.0-or-later
>>>   /*
>>> - * Aspeed AST24XX, AST25XX, and AST26XX SCU Interrupt Controller
>>> + * Aspeed AST24XX, AST25XX, AST26XX, AST27XX SCU Interrupt Controller
>>>    * Copyright 2019 IBM Corporation
>>>    *
>>>    * Eddie James <eajames@linux.ibm.com> @@ -34,11 +34,42 @@
>>>   	GENMASK(5, ASPEED_AST2600_SCU_IC1_SHIFT)
>>>   #define ASPEED_AST2600_SCU_IC1_NUM_IRQS	2
>>>
>>> +#define ASPEED_AST2700_SCU_IC0_EN_REG	0x1d0
>>> +#define ASPEED_AST2700_SCU_IC0_STS_REG	0x1d4
>>> +#define ASPEED_AST2700_SCU_IC0_SHIFT	0
>>> +#define ASPEED_AST2700_SCU_IC0_ENABLE	\
>>> +	GENMASK(3, ASPEED_AST2700_SCU_IC0_SHIFT)
>>> +#define ASPEED_AST2700_SCU_IC0_NUM_IRQS	4
>>> +
>>> +#define ASPEED_AST2700_SCU_IC1_EN_REG	0x1e0
>>> +#define ASPEED_AST2700_SCU_IC1_STS_REG	0x1e4
>>> +#define ASPEED_AST2700_SCU_IC1_SHIFT	0
>>> +#define ASPEED_AST2700_SCU_IC1_ENABLE	\
>>> +	GENMASK(3, ASPEED_AST2700_SCU_IC1_SHIFT)
>>> +#define ASPEED_AST2700_SCU_IC1_NUM_IRQS	4
>>> +
>>> +#define ASPEED_AST2700_SCU_IC2_EN_REG	0x104
>>> +#define ASPEED_AST2700_SCU_IC2_STS_REG	0x100
>>> +#define ASPEED_AST2700_SCU_IC2_SHIFT	0
>>> +#define ASPEED_AST2700_SCU_IC2_ENABLE	\
>>> +	GENMASK(3, ASPEED_AST2700_SCU_IC2_SHIFT)
>>> +#define ASPEED_AST2700_SCU_IC2_NUM_IRQS	4
>>> +
>>> +#define ASPEED_AST2700_SCU_IC3_EN_REG	0x10c
>>> +#define ASPEED_AST2700_SCU_IC3_STS_REG	0x108
>>> +#define ASPEED_AST2700_SCU_IC3_SHIFT	0
>>> +#define ASPEED_AST2700_SCU_IC3_ENABLE	\
>>> +	GENMASK(1, ASPEED_AST2700_SCU_IC3_SHIFT)
>>> +#define ASPEED_AST2700_SCU_IC3_NUM_IRQS	2
>>> +
>> The reason for ic0/ic1 compatibles before was the enable field was different.
>> Now it's at least at the same shift. Do you really need a different value for IC3?
>>
> OK, I can remove this define.
>
>> The register addresses should come from "reg". I don't understand why they
>> are hardcoded in the driver.
> The original code register is come from parent. scu_ic->scu = syscon_node_to_regmap(node->parent);
> I keep the original code logic, and add AST2700.


Hi Ryan,


How much is common with 2500/2600? I wonder if it would be easier to 
just create a new driver only for AST2700, instead of all in the same 
file/driver?


Thanks,

Eddie


>> Rob

Re: [PATCH 2/2] irqchip/aspeed-scu-ic: Add support for AST2700 SCU interrupt controllers

[Thomas Gleixner]

On Wed, Aug 06 2025 at 08:41, Eddie James wrote:
> On 8/6/25 2:14 AM, Ryan Chen wrote:
>
> How much is common with 2500/2600? I wonder if it would be easier to 
> just create a new driver only for AST2700, instead of all in the same 
> file/driver?

There is enough consolidation potential to keep them in the same file.

RE: [PATCH 2/2] irqchip/aspeed-scu-ic: Add support for AST2700 SCU interrupt controllers

[Ryan Chen]

> Subject: Re: [PATCH 2/2] irqchip/aspeed-scu-ic: Add support for AST2700 SCU
> interrupt controllers
> 
> On Wed, Aug 06 2025 at 08:41, Eddie James wrote:
> > On 8/6/25 2:14 AM, Ryan Chen wrote:
> >
> > How much is common with 2500/2600? I wonder if it would be easier to
> > just create a new driver only for AST2700, instead of all in the same
> > file/driver?
> 
> There is enough consolidation potential to keep them in the same file.

Yes, I will keep the same file.