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

[Eddie James <eajames@linux.ibm.com>]

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