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

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

[Ryan Chen]

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

AST2700 follows the same multi-instance SCU interrupt controller design
as AST2600, with four independent interrupt domains (scu-ic0 to 3).
However, unlike previous SoCs that use a single register for both enable
and status bits, AST2700 splits them into separate IER (Interrupt Enable)
and ISR (Interrupt Status) registers.

To support this, the driver is refactored to use a variant-based init
structure, selected by compatible string. Register access is also
converted from regmap to MMIO (via `of_iomap()`), and a per-variant
IRQ handler is used depending on register layout.

v2:
- Refactor SCU IC driver to support variant-based initialization
- Add AST2700 compatible strings to YAML and header files
- Extend DT bindings in mfd and irqchip for AST2700
- Add IRQ handler logic for separate IER/ISR layout

Ryan Chen (4):
  irqchip/aspeed-scu-ic: Refactor driver to support variant-based
    initialization
  dt-bindings: mfd: aspeed: Add AST2700 SCU compatibles
  dt-bindings: interrupt-controller: aspeed: Add AST2700 SCU IC
    compatibles
  irqchip/aspeed-scu-ic: Add support AST2700 SCU interrupt controllers

 .../aspeed,ast2500-scu-ic.yaml                |   6 +-
 .../bindings/mfd/aspeed,ast2x00-scu.yaml      |   4 +
 drivers/irqchip/irq-aspeed-scu-ic.c           | 238 ++++++++++--------
 .../interrupt-controller/aspeed-scu-ic.h      |  14 ++
 4 files changed, 163 insertions(+), 99 deletions(-)

-- 
2.34.1

[PATCH v2 1/4] irqchip/aspeed-scu-ic: Refactor driver to support variant-based initialization

[Ryan Chen]

The original SCU IC driver handled each AST2600 instance with separate
initialization functions and hardcoded register definitions. This patch
consolidates the implementation by introducing a variant-based structure,
selected via compatible string.

The driver now uses a unified init path and MMIO access via of_iomap().
This simplifies the code and prepares for upcoming SoCs like AST2700,
which require split register handling.

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

diff --git a/drivers/irqchip/irq-aspeed-scu-ic.c b/drivers/irqchip/irq-aspeed-scu-ic.c
index 1c7045467c48..cbfc35919281 100644
--- a/drivers/irqchip/irq-aspeed-scu-ic.c
+++ b/drivers/irqchip/irq-aspeed-scu-ic.c
@@ -7,52 +7,52 @@
  */
 
 #include <linux/bitops.h>
+#include <linux/io.h>
 #include <linux/irq.h>
 #include <linux/irqchip.h>
 #include <linux/irqchip/chained_irq.h>
 #include <linux/irqdomain.h>
-#include <linux/mfd/syscon.h>
+#include <linux/of_address.h>
 #include <linux/of_irq.h>
-#include <linux/regmap.h>
 
-#define ASPEED_SCU_IC_REG		0x018
-#define ASPEED_SCU_IC_SHIFT		0
-#define ASPEED_SCU_IC_ENABLE		GENMASK(15, ASPEED_SCU_IC_SHIFT)
-#define ASPEED_SCU_IC_NUM_IRQS		7
 #define ASPEED_SCU_IC_STATUS		GENMASK(28, 16)
 #define ASPEED_SCU_IC_STATUS_SHIFT	16
 
-#define ASPEED_AST2600_SCU_IC0_REG	0x560
-#define ASPEED_AST2600_SCU_IC0_SHIFT	0
-#define ASPEED_AST2600_SCU_IC0_ENABLE	\
-	GENMASK(5, ASPEED_AST2600_SCU_IC0_SHIFT)
-#define ASPEED_AST2600_SCU_IC0_NUM_IRQS	6
+struct aspeed_scu_ic_variant {
+	const char		*compatible;
+	unsigned long	irq_enable;
+	unsigned long	irq_shift;
+	unsigned int	num_irqs;
+};
+
+#define SCU_VARIANT(_compat, _shift, _enable, _num) { \
+	.compatible		=	_compat,	\
+	.irq_shift		=	_shift,		\
+	.irq_enable		=	_enable,	\
+	.num_irqs		=	_num,		\
+}
 
-#define ASPEED_AST2600_SCU_IC1_REG	0x570
-#define ASPEED_AST2600_SCU_IC1_SHIFT	4
-#define ASPEED_AST2600_SCU_IC1_ENABLE	\
-	GENMASK(5, ASPEED_AST2600_SCU_IC1_SHIFT)
-#define ASPEED_AST2600_SCU_IC1_NUM_IRQS	2
+static const struct aspeed_scu_ic_variant scu_ic_variants[]	__initconst = {
+	SCU_VARIANT("aspeed,ast2400-scu-ic",	0,	GENMASK(15, 0),	7),
+	SCU_VARIANT("aspeed,ast2500-scu-ic",	0,	GENMASK(15, 0), 7),
+	SCU_VARIANT("aspeed,ast2600-scu-ic0",	0,	GENMASK(5, 0),	6),
+	SCU_VARIANT("aspeed,ast2600-scu-ic1",	4,	GENMASK(5, 4),	2),
+};
 
 struct aspeed_scu_ic {
-	unsigned long irq_enable;
-	unsigned long irq_shift;
-	unsigned int num_irqs;
-	unsigned int reg;
-	struct regmap *scu;
-	struct irq_domain *irq_domain;
+	unsigned long		irq_enable;
+	unsigned long		irq_shift;
+	unsigned int		num_irqs;
+	void __iomem		*base;
+	struct irq_domain	*irq_domain;
 };
 
 static void aspeed_scu_ic_irq_handler(struct irq_desc *desc)
 {
-	unsigned int sts;
-	unsigned long bit;
-	unsigned long enabled;
-	unsigned long max;
-	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 long bit, enabled, max, status;
+	unsigned int sts, mask;
 
 	chained_irq_enter(chip, desc);
 
@@ -66,7 +66,7 @@ static void aspeed_scu_ic_irq_handler(struct irq_desc *desc)
 	 * 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);
+	sts = readl(scu_ic->base);
 	enabled = sts & scu_ic->irq_enable;
 	status = (sts >> ASPEED_SCU_IC_STATUS_SHIFT) & enabled;
 
@@ -76,9 +76,9 @@ static void aspeed_scu_ic_irq_handler(struct irq_desc *desc)
 	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));
+		writel((readl(scu_ic->base) & ~mask) |
+		       BIT(bit + ASPEED_SCU_IC_STATUS_SHIFT),
+		       scu_ic->base);
 	}
 
 	chained_irq_exit(chip, desc);
@@ -95,7 +95,7 @@ static void aspeed_scu_ic_irq_mask(struct irq_data *data)
 	 * 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);
+	writel(readl(scu_ic->base) & ~mask, scu_ic->base);
 }
 
 static void aspeed_scu_ic_irq_unmask(struct irq_data *data)
@@ -110,7 +110,7 @@ static void aspeed_scu_ic_irq_unmask(struct irq_data *data)
 	 * 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);
+	writel((readl(scu_ic->base) & ~mask) | bit, scu_ic->base);
 }
 
 static int aspeed_scu_ic_irq_set_affinity(struct irq_data *data,
@@ -146,18 +146,13 @@ static int aspeed_scu_ic_of_init_common(struct aspeed_scu_ic *scu_ic,
 	int irq;
 	int rc = 0;
 
-	if (!node->parent) {
-		rc = -ENODEV;
+	scu_ic->base = of_iomap(node, 0);
+	if (IS_ERR(scu_ic->base)) {
+		rc = PTR_ERR(scu_ic->base);
 		goto err;
 	}
-
-	scu_ic->scu = syscon_node_to_regmap(node->parent);
-	if (IS_ERR(scu_ic->scu)) {
-		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);
+	writel(ASPEED_SCU_IC_STATUS, scu_ic->base);
+	writel(0, scu_ic->base);
 
 	irq = irq_of_parse_and_map(node, 0);
 	if (!irq) {
@@ -166,8 +161,8 @@ static int aspeed_scu_ic_of_init_common(struct aspeed_scu_ic *scu_ic,
 	}
 
 	scu_ic->irq_domain = irq_domain_create_linear(of_fwnode_handle(node), scu_ic->num_irqs,
-						   &aspeed_scu_ic_domain_ops,
-						   scu_ic);
+						      &aspeed_scu_ic_domain_ops,
+						      scu_ic);
 	if (!scu_ic->irq_domain) {
 		rc = -ENOMEM;
 		goto err;
@@ -184,57 +179,37 @@ static int aspeed_scu_ic_of_init_common(struct aspeed_scu_ic *scu_ic,
 	return rc;
 }
 
-static int __init aspeed_scu_ic_of_init(struct device_node *node,
-					struct device_node *parent)
+static const struct aspeed_scu_ic_variant *
+aspeed_scu_ic_find_variant(struct device_node *np)
 {
-	struct aspeed_scu_ic *scu_ic = kzalloc(sizeof(*scu_ic), GFP_KERNEL);
-
-	if (!scu_ic)
-		return -ENOMEM;
+	for (int i = 0; i < ARRAY_SIZE(scu_ic_variants); i++)
+		if (of_device_is_compatible(np, scu_ic_variants[i].compatible))
+			return &scu_ic_variants[i];
 
-	scu_ic->irq_enable = ASPEED_SCU_IC_ENABLE;
-	scu_ic->irq_shift = ASPEED_SCU_IC_SHIFT;
-	scu_ic->num_irqs = ASPEED_SCU_IC_NUM_IRQS;
-	scu_ic->reg = ASPEED_SCU_IC_REG;
-
-	return aspeed_scu_ic_of_init_common(scu_ic, node);
+	return NULL;
 }
 
-static int __init aspeed_ast2600_scu_ic0_of_init(struct device_node *node,
-						 struct device_node *parent)
+static int __init aspeed_scu_ic_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;
+	const struct aspeed_scu_ic_variant *variant;
+	struct aspeed_scu_ic *scu_ic;
 
-	scu_ic->irq_enable = ASPEED_AST2600_SCU_IC0_ENABLE;
-	scu_ic->irq_shift = ASPEED_AST2600_SCU_IC0_SHIFT;
-	scu_ic->num_irqs = ASPEED_AST2600_SCU_IC0_NUM_IRQS;
-	scu_ic->reg = ASPEED_AST2600_SCU_IC0_REG;
-
-	return aspeed_scu_ic_of_init_common(scu_ic, node);
-}
-
-static int __init aspeed_ast2600_scu_ic1_of_init(struct device_node *node,
-						 struct device_node *parent)
-{
-	struct aspeed_scu_ic *scu_ic = kzalloc(sizeof(*scu_ic), GFP_KERNEL);
+	variant = aspeed_scu_ic_find_variant(node);
+	if (!variant)
+		return -ENODEV;
 
+	scu_ic = kzalloc(sizeof(*scu_ic), GFP_KERNEL);
 	if (!scu_ic)
 		return -ENOMEM;
 
-	scu_ic->irq_enable = ASPEED_AST2600_SCU_IC1_ENABLE;
-	scu_ic->irq_shift = ASPEED_AST2600_SCU_IC1_SHIFT;
-	scu_ic->num_irqs = ASPEED_AST2600_SCU_IC1_NUM_IRQS;
-	scu_ic->reg = ASPEED_AST2600_SCU_IC1_REG;
+	scu_ic->irq_enable    = variant->irq_enable;
+	scu_ic->irq_shift     = variant->irq_shift;
+	scu_ic->num_irqs      = variant->num_irqs;
 
 	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(ast2600_scu_ic0, "aspeed,ast2600-scu-ic0", aspeed_scu_ic_of_init);
+IRQCHIP_DECLARE(ast2600_scu_ic1, "aspeed,ast2600-scu-ic1",     aspeed_scu_ic_of_init);
-- 
2.34.1

Re: [PATCH v2 1/4] irqchip/aspeed-scu-ic: Refactor driver to support variant-based initialization

[Thomas Gleixner]

On Sun, Aug 31 2025 at 10:14, Ryan Chen wrote:
>  	scu_ic->irq_domain = irq_domain_create_linear(of_fwnode_handle(node), scu_ic->num_irqs,
> -						   &aspeed_scu_ic_domain_ops,
> -						   scu_ic);
> +						      &aspeed_scu_ic_domain_ops,
> +						      scu_ic);

Please move scu_ic to the previous line.

> +aspeed_scu_ic_find_variant(struct device_node *np)
>  {
> -	struct aspeed_scu_ic *scu_ic = kzalloc(sizeof(*scu_ic), GFP_KERNEL);
> -
> -	if (!scu_ic)
> -		return -ENOMEM;
> +	for (int i = 0; i < ARRAY_SIZE(scu_ic_variants); i++)
> +		if (of_device_is_compatible(np, scu_ic_variants[i].compatible))
> +			return &scu_ic_variants[i];

the for loop wants curly brackets.

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#bracket-rules

> +	scu_ic->irq_enable    = variant->irq_enable;
> +	scu_ic->irq_shift     = variant->irq_shift;
> +	scu_ic->num_irqs      = variant->num_irqs;

Please use a TAB not spaces when you want to align things.

> +IRQCHIP_DECLARE(ast2600_scu_ic0, "aspeed,ast2600-scu-ic0", aspeed_scu_ic_of_init);
> +IRQCHIP_DECLARE(ast2600_scu_ic1, "aspeed,ast2600-scu-ic1",     aspeed_scu_ic_of_init);

Stray TAB in the last line.

Thanks,

        tglx

RE: [PATCH v2 1/4] irqchip/aspeed-scu-ic: Refactor driver to support variant-based initialization

[Ryan Chen]

Thaks the review.

> Subject: Re: [PATCH v2 1/4] irqchip/aspeed-scu-ic: Refactor driver to support
> variant-based initialization
> 
> On Sun, Aug 31 2025 at 10:14, Ryan Chen wrote:
> >  	scu_ic->irq_domain = irq_domain_create_linear(of_fwnode_handle(node),
> scu_ic->num_irqs,
> > -						   &aspeed_scu_ic_domain_ops,
> > -						   scu_ic);
> > +						      &aspeed_scu_ic_domain_ops,
> > +						      scu_ic);
> 
> Please move scu_ic to the previous line.

Will update in next version.
        scu_ic->irq_domain = irq_domain_create_linear(of_fwnode_handle(node), scu_ic->num_irqs,
                                                      &aspeed_scu_ic_domain_ops, scu_ic);
> 
> > +aspeed_scu_ic_find_variant(struct device_node *np)
> >  {
> > -	struct aspeed_scu_ic *scu_ic = kzalloc(sizeof(*scu_ic), GFP_KERNEL);
> > -
> > -	if (!scu_ic)
> > -		return -ENOMEM;
> > +	for (int i = 0; i < ARRAY_SIZE(scu_ic_variants); i++)
> > +		if (of_device_is_compatible(np, scu_ic_variants[i].compatible))
> > +			return &scu_ic_variants[i];
> 
> the for loop wants curly brackets.
> 
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#bracket-
> rules

Thanks.
Will update

for (int i = 0; i < ARRAY_SIZE(scu_ic_variants); i++) {
	if (of_device_is_compatible(np, scu_ic_variants[i].compatible))
		return &scu_ic_variants[i];
}
> 
> > +	scu_ic->irq_enable    = variant->irq_enable;
> > +	scu_ic->irq_shift     = variant->irq_shift;
> > +	scu_ic->num_irqs      = variant->num_irqs;
> 
> Please use a TAB not spaces when you want to align things.
Thanks, will update
> 
> > +IRQCHIP_DECLARE(ast2600_scu_ic0, "aspeed,ast2600-scu-ic0",
> aspeed_scu_ic_of_init);
> > +IRQCHIP_DECLARE(ast2600_scu_ic1, "aspeed,ast2600-scu-ic1",
> aspeed_scu_ic_of_init);
> 
> Stray TAB in the last line.
Will remove tab
> 
> Thanks,
> 
>         tglx

[PATCH v2 2/4] dt-bindings: mfd: aspeed: Add AST2700 SCU compatibles

[Ryan Chen]

Add SCU interrupt controller compatible strings for the AST2700 SoC:
scu-ic0 to 3. This extends the MFD binding to support AST2700-based
platforms.

Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
---
 Documentation/devicetree/bindings/mfd/aspeed,ast2x00-scu.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/aspeed,ast2x00-scu.yaml b/Documentation/devicetree/bindings/mfd/aspeed,ast2x00-scu.yaml
index 5eccd10d95ce..67be6d095fe4 100644
--- a/Documentation/devicetree/bindings/mfd/aspeed,ast2x00-scu.yaml
+++ b/Documentation/devicetree/bindings/mfd/aspeed,ast2x00-scu.yaml
@@ -75,6 +75,10 @@ patternProperties:
             - 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
 
   '^silicon-id@[0-9a-f]+$':
     description: Unique hardware silicon identifiers within the SoC
-- 
2.34.1

Re: [PATCH v2 2/4] dt-bindings: mfd: aspeed: Add AST2700 SCU compatibles

[Rob Herring (Arm)]

On Sun, 31 Aug 2025 10:14:36 +0800, Ryan Chen wrote:
> Add SCU interrupt controller compatible strings for the AST2700 SoC:
> scu-ic0 to 3. This extends the MFD binding to support AST2700-based
> platforms.
> 
> Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
> ---
>  Documentation/devicetree/bindings/mfd/aspeed,ast2x00-scu.yaml | 4 ++++
>  1 file changed, 4 insertions(+)
> 

Acked-by: Rob Herring (Arm) <robh@kernel.org>

Re: (subset) [PATCH v2 2/4] dt-bindings: mfd: aspeed: Add AST2700 SCU compatibles

[Lee Jones]

On Sun, 31 Aug 2025 10:14:36 +0800, Ryan Chen wrote:
> Add SCU interrupt controller compatible strings for the AST2700 SoC:
> scu-ic0 to 3. This extends the MFD binding to support AST2700-based
> platforms.
> 
> 

Applied, thanks!

[2/4] dt-bindings: mfd: aspeed: Add AST2700 SCU compatibles
      commit: ef7d90dccee683bae63d6637cdb6c04fa1d7a8a8

--
Lee Jones [李琼斯]

[PATCH v2 3/4] dt-bindings: interrupt-controller: aspeed: Add AST2700 SCU IC compatibles

[Ryan Chen]

Add compatible strings for the four SCU interrupt controller instances
on the AST2700 SoC (scu-ic0 to 3), following the multi-instance model used
on AST2600.

Also define interrupt indices in the binding header.

Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
---
 .../aspeed,ast2500-scu-ic.yaml                     |  6 +++++-
 .../interrupt-controller/aspeed-scu-ic.h           | 14 ++++++++++++++
 2 files changed, 19 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
diff --git a/include/dt-bindings/interrupt-controller/aspeed-scu-ic.h b/include/dt-bindings/interrupt-controller/aspeed-scu-ic.h
index f315d5a7f5ee..7dd04424afcc 100644
--- a/include/dt-bindings/interrupt-controller/aspeed-scu-ic.h
+++ b/include/dt-bindings/interrupt-controller/aspeed-scu-ic.h
@@ -20,4 +20,18 @@
 #define ASPEED_AST2600_SCU_IC1_LPC_RESET_LO_TO_HI	0
 #define ASPEED_AST2600_SCU_IC1_LPC_RESET_HI_TO_LO	1
 
+#define ASPEED_AST2700_SCU_IC0_PCIE_PERST_LO_TO_HI	3
+#define ASPEED_AST2700_SCU_IC0_PCIE_PERST_HI_TO_LO	2
+
+#define ASPEED_AST2700_SCU_IC1_PCIE_RCRST_LO_TO_HI	3
+#define ASPEED_AST2700_SCU_IC1_PCIE_RCRST_HI_TO_LO	2
+
+#define ASPEED_AST2700_SCU_IC2_PCIE_PERST_LO_TO_HI	3
+#define ASPEED_AST2700_SCU_IC2_PCIE_PERST_HI_TO_LO	2
+#define ASPEED_AST2700_SCU_IC2_LPC_RESET_LO_TO_HI	1
+#define ASPEED_AST2700_SCU_IC2_LPC_RESET_HI_TO_LO	0
+
+#define ASPEED_AST2700_SCU_IC3_LPC_RESET_LO_TO_HI	1
+#define ASPEED_AST2700_SCU_IC3_LPC_RESET_HI_TO_LO	0
+
 #endif /* _DT_BINDINGS_INTERRUPT_CONTROLLER_ASPEED_SCU_IC_H_ */
-- 
2.34.1

Re: [PATCH v2 3/4] dt-bindings: interrupt-controller: aspeed: Add AST2700 SCU IC compatibles

[Rob Herring (Arm)]

On Sun, 31 Aug 2025 10:14:37 +0800, Ryan Chen wrote:
> Add compatible strings for the four SCU interrupt controller instances
> on the AST2700 SoC (scu-ic0 to 3), following the multi-instance model used
> on AST2600.
> 
> Also define interrupt indices in the binding header.
> 
> Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
> ---
>  .../aspeed,ast2500-scu-ic.yaml                     |  6 +++++-
>  .../interrupt-controller/aspeed-scu-ic.h           | 14 ++++++++++++++
>  2 files changed, 19 insertions(+), 1 deletion(-)
> 

Acked-by: Rob Herring (Arm) <robh@kernel.org>

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

[Ryan Chen]

The AST2700 continues the multi-instance SCU interrupt controller model
introduced in the AST2600, with four independent interrupt domains
(scu-ic0 to 3).

Unlike earlier generations that combine interrupt enable and status bits
into a single register, the AST2700 separates these into distinct IER and
ISR registers. Support for this layout is implemented by using register
offsets and separate chained IRQ handlers.

The variant table is extended to cover AST2700 IC instances, enabling
shared initialization logic while preserving support for previous SoCs.

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

diff --git a/drivers/irqchip/irq-aspeed-scu-ic.c b/drivers/irqchip/irq-aspeed-scu-ic.c
index cbfc35919281..ffdd9b4e44c1 100644
--- a/drivers/irqchip/irq-aspeed-scu-ic.c
+++ b/drivers/irqchip/irq-aspeed-scu-ic.c
@@ -17,12 +17,16 @@
 
 #define ASPEED_SCU_IC_STATUS		GENMASK(28, 16)
 #define ASPEED_SCU_IC_STATUS_SHIFT	16
+#define AST2700_SCU_IC_STATUS		GENMASK(15, 0)
 
 struct aspeed_scu_ic_variant {
 	const char		*compatible;
 	unsigned long	irq_enable;
 	unsigned long	irq_shift;
 	unsigned int	num_irqs;
+	bool			split_ier_isr;
+	unsigned long	ier;
+	unsigned long	isr;
 };
 
 #define SCU_VARIANT(_compat, _shift, _enable, _num) { \
@@ -30,13 +34,20 @@ struct aspeed_scu_ic_variant {
 	.irq_shift		=	_shift,		\
 	.irq_enable		=	_enable,	\
 	.num_irqs		=	_num,		\
+	.split_ier_isr	=	_split,		\
+	.ier			=	_ier,		\
+	.isr			=	_isr,		\
 }
 
 static const struct aspeed_scu_ic_variant scu_ic_variants[]	__initconst = {
-	SCU_VARIANT("aspeed,ast2400-scu-ic",	0,	GENMASK(15, 0),	7),
-	SCU_VARIANT("aspeed,ast2500-scu-ic",	0,	GENMASK(15, 0), 7),
-	SCU_VARIANT("aspeed,ast2600-scu-ic0",	0,	GENMASK(5, 0),	6),
-	SCU_VARIANT("aspeed,ast2600-scu-ic1",	4,	GENMASK(5, 4),	2),
+	SCU_VARIANT("aspeed,ast2400-scu-ic",	0, GENMASK(15, 0),	7,	false,	0,	0),
+	SCU_VARIANT("aspeed,ast2500-scu-ic",	0, GENMASK(15, 0),	7,	false,	0,	0),
+	SCU_VARIANT("aspeed,ast2600-scu-ic0",	0, GENMASK(5, 0),	6,	false,	0,	0),
+	SCU_VARIANT("aspeed,ast2600-scu-ic1",	4, GENMASK(5, 4),	2,	false,	0,	0),
+	SCU_VARIANT("aspeed,ast2700-scu-ic0",	0, GENMASK(3, 0),	4,	true,	0x00, 0x04),
+	SCU_VARIANT("aspeed,ast2700-scu-ic1",	0, GENMASK(3, 0),	4,	true,	0x00, 0x04),
+	SCU_VARIANT("aspeed,ast2700-scu-ic2",	0, GENMASK(3, 0),	4,	true,	0x04, 0x00),
+	SCU_VARIANT("aspeed,ast2700-scu-ic3",	0, GENMASK(1, 0),	2,	true,	0x04, 0x00),
 };
 
 struct aspeed_scu_ic {
@@ -45,9 +56,12 @@ struct aspeed_scu_ic {
 	unsigned int		num_irqs;
 	void __iomem		*base;
 	struct irq_domain	*irq_domain;
+	bool				split_ier_isr;
+	unsigned long		ier;
+	unsigned long		isr;
 };
 
-static void aspeed_scu_ic_irq_handler(struct irq_desc *desc)
+static void aspeed_scu_ic_irq_handler_combined(struct irq_desc *desc)
 {
 	struct aspeed_scu_ic *scu_ic = irq_desc_get_handler_data(desc);
 	struct irq_chip *chip = irq_desc_get_chip(desc);
@@ -84,33 +98,69 @@ static void aspeed_scu_ic_irq_handler(struct irq_desc *desc)
 	chained_irq_exit(chip, desc);
 }
 
+static void aspeed_scu_ic_irq_handler_split(struct irq_desc *desc)
+{
+	struct aspeed_scu_ic *scu_ic = irq_desc_get_handler_data(desc);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	unsigned long bit, enabled, max, status;
+	unsigned int sts, mask;
+
+	chained_irq_enter(chip, desc);
+
+	mask = scu_ic->irq_enable;
+	sts = readl(scu_ic->base + scu_ic->isr);
+	enabled = sts & scu_ic->irq_enable;
+	sts = readl(scu_ic->base + scu_ic->isr);
+	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);
+		writel(BIT(bit), scu_ic->base + scu_ic->isr); // clear interrupt
+	}
+
+	chained_irq_exit(chip, 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.
-	 */
-	writel(readl(scu_ic->base) & ~mask, scu_ic->base);
+	if (scu_ic->split_ier_isr) {
+		writel(readl(scu_ic->base) & ~BIT(data->hwirq + scu_ic->irq_shift),
+		       scu_ic->base + scu_ic->ier);
+	} else {
+		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.
+		 */
+		writel(readl(scu_ic->base) & ~mask, scu_ic->base);
+	}
 }
 
 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.
-	 */
-	writel((readl(scu_ic->base) & ~mask) | bit, scu_ic->base);
+	if (scu_ic->split_ier_isr) {
+		writel(readl(scu_ic->base) | bit, scu_ic->base + scu_ic->ier);
+	} else {
+		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.
+		 */
+		writel((readl(scu_ic->base) & ~mask) | bit, scu_ic->base);
+	}
 }
 
 static int aspeed_scu_ic_irq_set_affinity(struct irq_data *data,
@@ -151,8 +201,14 @@ static int aspeed_scu_ic_of_init_common(struct aspeed_scu_ic *scu_ic,
 		rc = PTR_ERR(scu_ic->base);
 		goto err;
 	}
-	writel(ASPEED_SCU_IC_STATUS, scu_ic->base);
-	writel(0, scu_ic->base);
+
+	if (scu_ic->split_ier_isr) {
+		writel(AST2700_SCU_IC_STATUS, scu_ic->base + scu_ic->isr);
+		writel(0, scu_ic->base + scu_ic->ier);
+	} else {
+		writel(ASPEED_SCU_IC_STATUS, scu_ic->base);
+		writel(0, scu_ic->base);
+	}
 
 	irq = irq_of_parse_and_map(node, 0);
 	if (!irq) {
@@ -168,8 +224,12 @@ static int aspeed_scu_ic_of_init_common(struct aspeed_scu_ic *scu_ic,
 		goto err;
 	}
 
-	irq_set_chained_handler_and_data(irq, aspeed_scu_ic_irq_handler,
-					 scu_ic);
+	if (scu_ic->split_ier_isr)
+		irq_set_chained_handler_and_data(irq, aspeed_scu_ic_irq_handler_split,
+						 scu_ic);
+	else
+		irq_set_chained_handler_and_data(irq, aspeed_scu_ic_irq_handler_combined,
+						 scu_ic);
 
 	return 0;
 
@@ -202,9 +262,12 @@ static int __init aspeed_scu_ic_of_init(struct device_node *node, struct device_
 	if (!scu_ic)
 		return -ENOMEM;
 
-	scu_ic->irq_enable    = variant->irq_enable;
-	scu_ic->irq_shift     = variant->irq_shift;
-	scu_ic->num_irqs      = variant->num_irqs;
+	scu_ic->irq_enable	= variant->irq_enable;
+	scu_ic->irq_shift	= variant->irq_shift;
+	scu_ic->num_irqs	= variant->num_irqs;
+	scu_ic->split_ier_isr	= variant->split_ier_isr;
+	scu_ic->ier		= variant->ier;
+	scu_ic->isr		= variant->isr;
 
 	return aspeed_scu_ic_of_init_common(scu_ic, node);
 }
@@ -213,3 +276,7 @@ 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_scu_ic_of_init);
 IRQCHIP_DECLARE(ast2600_scu_ic1, "aspeed,ast2600-scu-ic1",     aspeed_scu_ic_of_init);
+IRQCHIP_DECLARE(ast2700_scu_ic0, "aspeed,ast2700-scu-ic0", aspeed_scu_ic_of_init);
+IRQCHIP_DECLARE(ast2700_scu_ic1, "aspeed,ast2700-scu-ic1", aspeed_scu_ic_of_init);
+IRQCHIP_DECLARE(ast2700_scu_ic2, "aspeed,ast2700-scu-ic2", aspeed_scu_ic_of_init);
+IRQCHIP_DECLARE(ast2700_scu_ic3, "aspeed,ast2700-scu-ic3", aspeed_scu_ic_of_init);
-- 
2.34.1

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

[Thomas Gleixner]

On Sun, Aug 31 2025 at 10:14, Ryan Chen wrote:

> The AST2700 continues the multi-instance SCU interrupt controller model
> introduced in the AST2600, with four independent interrupt domains
> (scu-ic0 to 3).
>
> Unlike earlier generations that combine interrupt enable and status bits
> into a single register, the AST2700 separates these into distinct IER and
> ISR registers. Support for this layout is implemented by using register
> offsets and separate chained IRQ handlers.
>
> The variant table is extended to cover AST2700 IC instances, enabling
> shared initialization logic while preserving support for previous SoCs.
>
> Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
> ---
>  drivers/irqchip/irq-aspeed-scu-ic.c | 123 +++++++++++++++++++++-------
>  1 file changed, 95 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/irqchip/irq-aspeed-scu-ic.c b/drivers/irqchip/irq-aspeed-scu-ic.c
> index cbfc35919281..ffdd9b4e44c1 100644
> --- a/drivers/irqchip/irq-aspeed-scu-ic.c
> +++ b/drivers/irqchip/irq-aspeed-scu-ic.c
> @@ -17,12 +17,16 @@
>  
>  #define ASPEED_SCU_IC_STATUS		GENMASK(28, 16)
>  #define ASPEED_SCU_IC_STATUS_SHIFT	16
> +#define AST2700_SCU_IC_STATUS		GENMASK(15, 0)
>  
>  struct aspeed_scu_ic_variant {
>  	const char		*compatible;
>  	unsigned long	irq_enable;
>  	unsigned long	irq_shift;
>  	unsigned int	num_irqs;
> +	bool			split_ier_isr;

How does that end up aligned?

> +	unsigned long	ier;
> +	unsigned long	isr;
>  };
>  
>  #define SCU_VARIANT(_compat, _shift, _enable, _num) { \
> @@ -30,13 +34,20 @@ struct aspeed_scu_ic_variant {
>  	.irq_shift		=	_shift,		\
>  	.irq_enable		=	_enable,	\
>  	.num_irqs		=	_num,		\
> +	.split_ier_isr	=	_split,		\

Ditto.

> +	.ier			=	_ier,		\
> +	.isr			=	_isr,		\

But what's worse is that '_split, _ier and _isr' come out of thin air as
SCU_VARIANT does not have corresponding arguments. So how is that
supposed to work?

>  }
>  
>  struct aspeed_scu_ic {
> @@ -45,9 +56,12 @@ struct aspeed_scu_ic {
>  	unsigned int		num_irqs;
>  	void __iomem		*base;
>  	struct irq_domain	*irq_domain;
> +	bool				split_ier_isr;

Sigh...

> +	unsigned long		ier;
> +	unsigned long		isr;
>  };
>  
> -static void aspeed_scu_ic_irq_handler(struct irq_desc *desc)
> +static void aspeed_scu_ic_irq_handler_combined(struct irq_desc *desc)
>  {
>  	struct aspeed_scu_ic *scu_ic = irq_desc_get_handler_data(desc);
>  	struct irq_chip *chip = irq_desc_get_chip(desc);
> @@ -84,33 +98,69 @@ static void aspeed_scu_ic_irq_handler(struct irq_desc *desc)
>  	chained_irq_exit(chip, desc);
>  }
>  
> +static void aspeed_scu_ic_irq_handler_split(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.
> -	 */
> -	writel(readl(scu_ic->base) & ~mask, scu_ic->base);
> +	if (scu_ic->split_ier_isr) {
> +		writel(readl(scu_ic->base) & ~BIT(data->hwirq + scu_ic->irq_shift),
> +		       scu_ic->base + scu_ic->ier);
> +	} else {
> +		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.
> +		 */
> +		writel(readl(scu_ic->base) & ~mask, scu_ic->base);
> +	}

So you have two different handlers. Why can't you provide two different
mask/unmask/ functions along with a seperate irq chip instead of
cluttering the code with conditionals. Thes two variants share no code
at all.

> -	irq_set_chained_handler_and_data(irq, aspeed_scu_ic_irq_handler,
> -					 scu_ic);
> +	if (scu_ic->split_ier_isr)
> +		irq_set_chained_handler_and_data(irq, aspeed_scu_ic_irq_handler_split,
> +						 scu_ic);
> +	else
> +		irq_set_chained_handler_and_data(irq, aspeed_scu_ic_irq_handler_combined,
> +						 scu_ic);
>

Please get rid of the line break. You have 100 characters....

Thanks,

        tglx

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

[Ryan Chen]

> Subject: Re: [PATCH v2 4/4] irqchip/aspeed-scu-ic: Add support AST2700 SCU
> interrupt controllers
> 
> On Sun, Aug 31 2025 at 10:14, Ryan Chen wrote:
> 
> > The AST2700 continues the multi-instance SCU interrupt controller
> > model introduced in the AST2600, with four independent interrupt
> > domains
> > (scu-ic0 to 3).
> >
> > Unlike earlier generations that combine interrupt enable and status
> > bits into a single register, the AST2700 separates these into distinct
> > IER and ISR registers. Support for this layout is implemented by using
> > register offsets and separate chained IRQ handlers.
> >
> > The variant table is extended to cover AST2700 IC instances, enabling
> > shared initialization logic while preserving support for previous SoCs.
> >
> > Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
> > ---
> >  drivers/irqchip/irq-aspeed-scu-ic.c | 123
> > +++++++++++++++++++++-------
> >  1 file changed, 95 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-aspeed-scu-ic.c
> > b/drivers/irqchip/irq-aspeed-scu-ic.c
> > index cbfc35919281..ffdd9b4e44c1 100644
> > --- a/drivers/irqchip/irq-aspeed-scu-ic.c
> > +++ b/drivers/irqchip/irq-aspeed-scu-ic.c
> > @@ -17,12 +17,16 @@
> >
> >  #define ASPEED_SCU_IC_STATUS		GENMASK(28, 16)
> >  #define ASPEED_SCU_IC_STATUS_SHIFT	16
> > +#define AST2700_SCU_IC_STATUS		GENMASK(15, 0)
> >
> >  struct aspeed_scu_ic_variant {
> >  	const char		*compatible;
> >  	unsigned long	irq_enable;
> >  	unsigned long	irq_shift;
> >  	unsigned int	num_irqs;
> > +	bool			split_ier_isr;
> 
> How does that end up aligned?

Will update with Tab.
> 
> > +	unsigned long	ier;
> > +	unsigned long	isr;
> >  };
> >
> >  #define SCU_VARIANT(_compat, _shift, _enable, _num) { \ @@ -30,13
> > +34,20 @@ struct aspeed_scu_ic_variant {
> >  	.irq_shift		=	_shift,		\
> >  	.irq_enable		=	_enable,	\
> >  	.num_irqs		=	_num,		\
> > +	.split_ier_isr	=	_split,		\
> 
> Ditto.
Will update
> 
> > +	.ier			=	_ier,		\
> > +	.isr			=	_isr,		\
> 
> But what's worse is that '_split, _ier and _isr' come out of thin air as
> SCU_VARIANT does not have corresponding arguments. So how is that
> supposed to work?

Mistake, will update.
> 
> >  }
> >
> >  struct aspeed_scu_ic {
> > @@ -45,9 +56,12 @@ struct aspeed_scu_ic {
> >  	unsigned int		num_irqs;
> >  	void __iomem		*base;
> >  	struct irq_domain	*irq_domain;
> > +	bool				split_ier_isr;
> 
> Sigh...
Will update.
> 
> > +	unsigned long		ier;
> > +	unsigned long		isr;
> >  };
> >
> > -static void aspeed_scu_ic_irq_handler(struct irq_desc *desc)
> > +static void aspeed_scu_ic_irq_handler_combined(struct irq_desc *desc)
> >  {
> >  	struct aspeed_scu_ic *scu_ic = irq_desc_get_handler_data(desc);
> >  	struct irq_chip *chip = irq_desc_get_chip(desc); @@ -84,33 +98,69 @@
> > static void aspeed_scu_ic_irq_handler(struct irq_desc *desc)
> >  	chained_irq_exit(chip, desc);
> >  }
> >
> > +static void aspeed_scu_ic_irq_handler_split(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.
> > -	 */
> > -	writel(readl(scu_ic->base) & ~mask, scu_ic->base);
> > +	if (scu_ic->split_ier_isr) {
> > +		writel(readl(scu_ic->base) & ~BIT(data->hwirq + scu_ic->irq_shift),
> > +		       scu_ic->base + scu_ic->ier);
> > +	} else {
> > +		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.
> > +		 */
> > +		writel(readl(scu_ic->base) & ~mask, scu_ic->base);
> > +	}
> 
> So you have two different handlers. Why can't you provide two different
> mask/unmask/ functions along with a seperate irq chip instead of cluttering
> the code with conditionals. Thes two variants share no code at all.

I will add irq_chip in SCU_VARIANT, like following.

struct aspeed_scu_ic_variant {
..
+	struct irq_chip	*irq_chip;	
};

#define SCU_VARIANT(_compat, _shift, _enable, _num,  +_irq_chip, _split, _ier, _isr) { \
+	.irq_chip		=	_irq_chip,	\
.....
}

static const struct aspeed_scu_ic_variant scu_ic_variants[]	__initconst = {
	SCU_VARIANT("aspeed,ast2400-scu-ic",	0, GENMASK(15, 0),	7, &aspeed_scu_ic_chip_combined,	false,	0,	0),
	SCU_VARIANT("aspeed,ast2500-scu-ic",	0, GENMASK(15, 0),	7, &aspeed_scu_ic_chip_combined,	false,	0,	0),
	SCU_VARIANT("aspeed,ast2600-scu-ic0",	0, GENMASK(5, 0),	6, &aspeed_scu_ic_chip_combined,	false,	0,	0),
	SCU_VARIANT("aspeed,ast2600-scu-ic1",	4, GENMASK(5, 4),	2, &aspeed_scu_ic_chip_combined,	false,	0,	0),
	SCU_VARIANT("aspeed,ast2700-scu-ic0",	0, GENMASK(3, 0),	4, &aspeed_scu_ic_chip_split,	true,	0x00, 0x04),
	SCU_VARIANT("aspeed,ast2700-scu-ic1",	0, GENMASK(3, 0),	4, &aspeed_scu_ic_chip_split,	true,	0x00, 0x04),
	SCU_VARIANT("aspeed,ast2700-scu-ic2",	0, GENMASK(3, 0),	4, &aspeed_scu_ic_chip_split,	true,	0x04, 0x00),
	SCU_VARIANT("aspeed,ast2700-scu-ic3",	0, GENMASK(1, 0),	2, &aspeed_scu_ic_chip_split,	true,	0x04, 0x00),
};

Is this ok?

> 
> > -	irq_set_chained_handler_and_data(irq, aspeed_scu_ic_irq_handler,
> > -					 scu_ic);
> > +	if (scu_ic->split_ier_isr)
> > +		irq_set_chained_handler_and_data(irq,
> aspeed_scu_ic_irq_handler_split,
> > +						 scu_ic);
> > +	else
> > +		irq_set_chained_handler_and_data(irq,
> aspeed_scu_ic_irq_handler_combined,
> > +						 scu_ic);
> >
> 
> Please get rid of the line break. You have 100 characters....

I will update by following. Is it ok?
       irq_set_chained_handler_and_data(irq, scu_ic->split_ier_isr ?
                                         aspeed_scu_ic_irq_handler_split :
                                         aspeed_scu_ic_irq_handler_combined,
                                         scu_ic);


> 
> Thanks,
> 
>         tglx

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

[Thomas Gleixner]

On Fri, Sep 05 2025 at 05:55, Ryan Chen wrote:
>> So you have two different handlers. Why can't you provide two different
>> mask/unmask/ functions along with a seperate irq chip instead of cluttering
>> the code with conditionals. Thes two variants share no code at all.
>
> I will add irq_chip in SCU_VARIANT, like following.
>
> struct aspeed_scu_ic_variant {
> ..
> +	struct irq_chip	*irq_chip;	
> };
>
> #define SCU_VARIANT(_compat, _shift, _enable, _num,  +_irq_chip, _split, _ier, _isr) { \
> +	.irq_chip		=	_irq_chip,	\
> .....
> }
>
> static const struct aspeed_scu_ic_variant scu_ic_variants[]	__initconst = {
> 	SCU_VARIANT("aspeed,ast2400-scu-ic",	0, GENMASK(15, 0),	7, &aspeed_scu_ic_chip_combined,	false,	0,	0),
> 	SCU_VARIANT("aspeed,ast2500-scu-ic",	0, GENMASK(15, 0),	7, &aspeed_scu_ic_chip_combined,	false,	0,	0),
> 	SCU_VARIANT("aspeed,ast2600-scu-ic0",	0, GENMASK(5, 0),	6, &aspeed_scu_ic_chip_combined,	false,	0,	0),
> 	SCU_VARIANT("aspeed,ast2600-scu-ic1",	4, GENMASK(5, 4),	2, &aspeed_scu_ic_chip_combined,	false,	0,	0),
> 	SCU_VARIANT("aspeed,ast2700-scu-ic0",	0, GENMASK(3, 0),	4, &aspeed_scu_ic_chip_split,	true,	0x00, 0x04),
> 	SCU_VARIANT("aspeed,ast2700-scu-ic1",	0, GENMASK(3, 0),	4, &aspeed_scu_ic_chip_split,	true,	0x00, 0x04),
> 	SCU_VARIANT("aspeed,ast2700-scu-ic2",	0, GENMASK(3, 0),	4, &aspeed_scu_ic_chip_split,	true,	0x04, 0x00),
> 	SCU_VARIANT("aspeed,ast2700-scu-ic3",	0, GENMASK(1, 0),	2, &aspeed_scu_ic_chip_split,	true,	0x04, 0x00),
> };

You have this split_ier_isr field already, which should be good enough
to select the chip to assign, similar to what you do with the handler, no?

Thanks,

        tglx

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

[Ryan Chen]

> Subject: RE: [PATCH v2 4/4] irqchip/aspeed-scu-ic: Add support AST2700 SCU
> interrupt controllers
> 
> On Fri, Sep 05 2025 at 05:55, Ryan Chen wrote:
> >> So you have two different handlers. Why can't you provide two
> >> different mask/unmask/ functions along with a seperate irq chip
> >> instead of cluttering the code with conditionals. Thes two variants share no
> code at all.
> >
> > I will add irq_chip in SCU_VARIANT, like following.
> >
> > struct aspeed_scu_ic_variant {
> > ..
> > +	struct irq_chip	*irq_chip;
> > };
> >
> > #define SCU_VARIANT(_compat, _shift, _enable, _num,  +_irq_chip,
> > _split, _ier, _isr) { \
> > +	.irq_chip		=	_irq_chip,	\
> > .....
> > }
> >
> > static const struct aspeed_scu_ic_variant scu_ic_variants[]	__initconst = {
> > 	SCU_VARIANT("aspeed,ast2400-scu-ic",	0, GENMASK(15, 0),	7,
> &aspeed_scu_ic_chip_combined,	false,	0,	0),
> > 	SCU_VARIANT("aspeed,ast2500-scu-ic",	0, GENMASK(15, 0),	7,
> &aspeed_scu_ic_chip_combined,	false,	0,	0),
> > 	SCU_VARIANT("aspeed,ast2600-scu-ic0",	0, GENMASK(5, 0),	6,
> &aspeed_scu_ic_chip_combined,	false,	0,	0),
> > 	SCU_VARIANT("aspeed,ast2600-scu-ic1",	4, GENMASK(5, 4),	2,
> &aspeed_scu_ic_chip_combined,	false,	0,	0),
> > 	SCU_VARIANT("aspeed,ast2700-scu-ic0",	0, GENMASK(3, 0),	4,
> &aspeed_scu_ic_chip_split,	true,	0x00, 0x04),
> > 	SCU_VARIANT("aspeed,ast2700-scu-ic1",	0, GENMASK(3, 0),	4,
> &aspeed_scu_ic_chip_split,	true,	0x00, 0x04),
> > 	SCU_VARIANT("aspeed,ast2700-scu-ic2",	0, GENMASK(3, 0),	4,
> &aspeed_scu_ic_chip_split,	true,	0x04, 0x00),
> > 	SCU_VARIANT("aspeed,ast2700-scu-ic3",	0, GENMASK(1, 0),	2,
> &aspeed_scu_ic_chip_split,	true,	0x04, 0x00),
> > };
> 
> You have this split_ier_isr field already, which should be good enough to select
> the chip to assign, similar to what you do with the handler, no?
Got it, thanks your feedback.
> 
> Thanks,
> 
>         tglx