| Message ID | 20241009115813.2908803-3-kevin_chen@aspeedtech.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for AST2700 INTC driver | expand |
… > To support ASPEED interrupt controller(INTC) maps the internal interrupt > sources of the AST27XX device to an parent interrupt controller. > --- * I miss your tag “Signed-off-by”. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.12-rc2#n396 * How do you think about to choose an additional imperative wording for an improved change description? https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.12-rc2#n94 … > +++ b/drivers/irqchip/irq-aspeed-intc.c > @@ -0,0 +1,139 @@ … > +static void aspeed_intc_ic_irq_handler(struct irq_desc *desc) +{ > + struct aspeed_intc_ic *intc_ic = irq_desc_get_handler_data(desc); > + struct irq_chip *chip = irq_desc_get_chip(desc); > + unsigned long bit, status; I suggest to reduce the scopes for three local variables. > + > + chained_irq_enter(chip, desc); Would you become interested to collaborate with another scoped guard for this programming interface? https://elixir.bootlin.com/linux/v6.12-rc2/source/include/linux/irqchip/chained_irq.h#L13 > + > + scoped_guard(raw_spinlock, &intc_ic->gic_lock) { > + status = readl(intc_ic->base + INTC_INT_STATUS_REG); > + for_each_set_bit(bit, &status, IRQS_PER_WORD) { > + generic_handle_domain_irq(intc_ic->irq_domain, bit); > + writel(BIT(bit), intc_ic->base + INTC_INT_STATUS_REG); > + } > + } > + > + chained_irq_exit(chip, desc); > +} Regards, Markus
Hi Markus, Thank you for your review. Could you check the message below. Thanks a lot. > > … > > To support ASPEED interrupt controller(INTC) maps the internal > > interrupt sources of the AST27XX device to an parent interrupt controller. > > --- > > * I miss your tag “Signed-off-by”. > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Docume > ntation/process/submitting-patches.rst?h=v6.12-rc2#n396 > > * How do you think about to choose an additional imperative wording > for an improved change description? > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Docume > ntation/process/submitting-patches.rst?h=v6.12-rc2#n94 > > > … > > +++ b/drivers/irqchip/irq-aspeed-intc.c > > @@ -0,0 +1,139 @@ > … > > +static void aspeed_intc_ic_irq_handler(struct irq_desc *desc) > +{ > > + struct aspeed_intc_ic *intc_ic = irq_desc_get_handler_data(desc); > > + struct irq_chip *chip = irq_desc_get_chip(desc); > > + unsigned long bit, status; > > I suggest to reduce the scopes for three local variables. May I check the scopes of bit and status usage? Variables of bit and status are used in for_each_set_bit. How could I reduce the scopes? > > > > + > > + chained_irq_enter(chip, desc); > > Would you become interested to collaborate with another scoped guard for > this programming interface? > https://elixir.bootlin.com/linux/v6.12-rc2/source/include/linux/irqchip/chained > _irq.h#L13 Maybe like the change in the following? diff --git a/drivers/irqchip/irq-aspeed-intc.c b/drivers/irqchip/irq-aspeed-intc.c index ef1c095ad09e..54d1881c56c6 100644 --- a/drivers/irqchip/irq-aspeed-intc.c +++ b/drivers/irqchip/irq-aspeed-intc.c @@ -32,7 +32,7 @@ static void aspeed_intc_ic_irq_handler(struct irq_desc *desc) struct irq_chip *chip = irq_desc_get_chip(desc); unsigned long bit, status; - chained_irq_enter(chip, desc); + guard(chained_irq)(desc); scoped_guard(raw_spinlock, &intc_ic->gic_lock) { status = readl(intc_ic->base + INTC_INT_STATUS_REG); @@ -41,8 +41,6 @@ static void aspeed_intc_ic_irq_handler(struct irq_desc *desc) writel(BIT(bit), intc_ic->base + INTC_INT_STATUS_REG); } } - - chained_irq_exit(chip, desc); } static void aspeed_intc_irq_mask(struct irq_data *data) diff --git a/include/linux/irqchip/chained_irq.h b/include/linux/irqchip/chained_irq.h index dd8b3c476666..399a4ae30205 100644 --- a/include/linux/irqchip/chained_irq.h +++ b/include/linux/irqchip/chained_irq.h @@ -38,4 +38,5 @@ static inline void chained_irq_exit(struct irq_chip *chip, chip->irq_unmask(&desc->irq_data); } +DEFINE_GUARD (chained_irq, struct irq_desc * , chained_irq_exit ( _T ->irq_data.chip, _T ), chained_irq_enter (_T->irq_data.chip, _T)) #endif /* __IRQCHIP_CHAINED_IRQ_H */ > > > > + > > + scoped_guard(raw_spinlock, &intc_ic->gic_lock) { > > + status = readl(intc_ic->base + INTC_INT_STATUS_REG); > > + for_each_set_bit(bit, &status, IRQS_PER_WORD) { > > + generic_handle_domain_irq(intc_ic->irq_domain, bit); > > + writel(BIT(bit), intc_ic->base + INTC_INT_STATUS_REG); > > + } > > + } > > + > > + chained_irq_exit(chip, desc); > > +} > > > Regards, > Markus
On Wed, Oct 09, 2024 at 07:58:13PM +0800, Kevin Chen wrote: > +static int __init aspeed_intc_ic_of_init(struct device_node *node, > + struct device_node *parent) > +{ > + struct aspeed_intc_ic *intc_ic; > + int ret = 0; > + int irq, i; > + > + intc_ic = kzalloc(sizeof(*intc_ic), GFP_KERNEL); > + if (!intc_ic) > + return -ENOMEM; > + > + intc_ic->base = of_iomap(node, 0); > + if (!intc_ic->base) { > + pr_err("Failed to iomap intc_ic base\n"); > + ret = -ENOMEM; > + goto err_free_ic; > + } > + writel(0xffffffff, intc_ic->base + INTC_INT_STATUS_REG); > + writel(0x0, intc_ic->base + INTC_INT_ENABLE_REG); > + > + intc_ic->irq_domain = irq_domain_add_linear(node, 32, > + &aspeed_intc_ic_irq_domain_ops, intc_ic); > + if (!intc_ic->irq_domain) { > + ret = -ENOMEM; > + goto err_iounmap; > + } > + > + raw_spin_lock_init(&intc_ic->gic_lock); > + raw_spin_lock_init(&intc_ic->intc_lock); > + > + /* Check all the irq numbers valid. If not, unmaps all the base and frees the data. */ > + for (i = 0; i < of_irq_count(node); i++) { > + irq = irq_of_parse_and_map(node, i); > + if (!irq) { > + pr_err("Failed to get irq number\n"); > + ret = -EINVAL; > + goto err_iounmap; > + } > + } > + > + for (i = 0; i < of_irq_count(node); i++) { > + irq = irq_of_parse_and_map(node, i); > + irq_set_chained_handler_and_data(irq, aspeed_intc_ic_irq_handler, intc_ic); There is an extra tab on this line. regards, dan carpenter > + } > + > + return 0;
>> … >>> +++ b/drivers/irqchip/irq-aspeed-intc.c >>> @@ -0,0 +1,139 @@ >> … >>> +static void aspeed_intc_ic_irq_handler(struct irq_desc *desc) >> +{ >>> + struct aspeed_intc_ic *intc_ic = irq_desc_get_handler_data(desc); >>> + struct irq_chip *chip = irq_desc_get_chip(desc); >>> + unsigned long bit, status; >> >> I suggest to reduce the scopes for three local variables. > May I check the scopes of bit and status usage? > Variables of bit and status are used in for_each_set_bit. > How could I reduce the scopes? I propose to move selected variable definitions into corresponding compound statements (by using extra curly brackets). https://refactoring.com/catalog/reduceScopeOfVariable.html >> Would you become interested to collaborate with another scoped guard for >> this programming interface? >> https://elixir.bootlin.com/linux/v6.12-rc2/source/include/linux/irqchip/chained >> _irq.h#L13 > > Maybe like the change in the following? > > diff --git a/drivers/irqchip/irq-aspeed-intc.c b/drivers/irqchip/irq-aspeed-intc.c > index ef1c095ad09e..54d1881c56c6 100644 > --- a/drivers/irqchip/irq-aspeed-intc.c > +++ b/drivers/irqchip/irq-aspeed-intc.c > @@ -32,7 +32,7 @@ static void aspeed_intc_ic_irq_handler(struct irq_desc *desc) > struct irq_chip *chip = irq_desc_get_chip(desc); > unsigned long bit, status; > > - chained_irq_enter(chip, desc); > + guard(chained_irq)(desc); > > scoped_guard(raw_spinlock, &intc_ic->gic_lock) { > status = readl(intc_ic->base + INTC_INT_STATUS_REG); Perhaps. > @@ -41,8 +41,6 @@ static void aspeed_intc_ic_irq_handler(struct irq_desc *desc) > writel(BIT(bit), intc_ic->base + INTC_INT_STATUS_REG); > } > } > - > - chained_irq_exit(chip, desc); > } … Probably, yes. … > +++ b/include/linux/irqchip/chained_irq.h > @@ -38,4 +38,5 @@ static inline void chained_irq_exit(struct irq_chip *chip, > chip->irq_unmask(&desc->irq_data); > } > > +DEFINE_GUARD (chained_irq, struct irq_desc * , chained_irq_exit ( _T ->irq_data.chip, _T ), chained_irq_enter (_T->irq_data.chip, _T)) … * Such a macro call looks promising. Would you like to omit any space characters before open parentheses? * Would you like to support scoped guard variants accordingly? Regards, Markus
> > +static int __init aspeed_intc_ic_of_init(struct device_node *node, > > + struct device_node *parent) > > +{ > > + struct aspeed_intc_ic *intc_ic; > > + int ret = 0; > > + int irq, i; > > + > > + intc_ic = kzalloc(sizeof(*intc_ic), GFP_KERNEL); > > + if (!intc_ic) > > + return -ENOMEM; > > + > > + intc_ic->base = of_iomap(node, 0); > > + if (!intc_ic->base) { > > + pr_err("Failed to iomap intc_ic base\n"); > > + ret = -ENOMEM; > > + goto err_free_ic; > > + } > > + writel(0xffffffff, intc_ic->base + INTC_INT_STATUS_REG); > > + writel(0x0, intc_ic->base + INTC_INT_ENABLE_REG); > > + > > + intc_ic->irq_domain = irq_domain_add_linear(node, 32, > > + &aspeed_intc_ic_irq_domain_ops, intc_ic); > > + if (!intc_ic->irq_domain) { > > + ret = -ENOMEM; > > + goto err_iounmap; > > + } > > + > > + raw_spin_lock_init(&intc_ic->gic_lock); > > + raw_spin_lock_init(&intc_ic->intc_lock); > > + > > + /* Check all the irq numbers valid. If not, unmaps all the base and frees > the data. */ > > + for (i = 0; i < of_irq_count(node); i++) { > > + irq = irq_of_parse_and_map(node, i); > > + if (!irq) { > > + pr_err("Failed to get irq number\n"); > > + ret = -EINVAL; > > + goto err_iounmap; > > + } > > + } > > + > > + for (i = 0; i < of_irq_count(node); i++) { > > + irq = irq_of_parse_and_map(node, i); > > + irq_set_chained_handler_and_data(irq, > aspeed_intc_ic_irq_handler, intc_ic); > > There is an extra tab on this line. OK. Fixed. Thanks a lot. > > regards, > dan carpenter > > > + } > > + > > + return 0; >
> >> … > >>> +++ b/drivers/irqchip/irq-aspeed-intc.c > >>> @@ -0,0 +1,139 @@ > >> … > >>> +static void aspeed_intc_ic_irq_handler(struct irq_desc *desc) > >> +{ > >>> + struct aspeed_intc_ic *intc_ic = irq_desc_get_handler_data(desc); > >>> + struct irq_chip *chip = irq_desc_get_chip(desc); > >>> + unsigned long bit, status; > >> > >> I suggest to reduce the scopes for three local variables. > > May I check the scopes of bit and status usage? > > Variables of bit and status are used in for_each_set_bit. > > How could I reduce the scopes? > > I propose to move selected variable definitions into corresponding compound > statements (by using extra curly brackets). > https://refactoring.com/catalog/reduceScopeOfVariable.html OK. I moved these two local variables into scoped_guard. +static void aspeed_intc_ic_irq_handler(struct irq_desc *desc) +{ + struct aspeed_intc_ic *intc_ic = irq_desc_get_handler_data(desc); + + guard(chained_irq)(desc); + scoped_guard(raw_spinlock, &intc_ic->gic_lock) { + unsigned long bit, status; + + status = readl(intc_ic->base + INTC_INT_STATUS_REG); + for_each_set_bit(bit, &status, IRQS_PER_WORD) { + generic_handle_domain_irq(intc_ic->irq_domain, bit); + writel(BIT(bit), intc_ic->base + INTC_INT_STATUS_REG); + } + } +} > > > >> Would you become interested to collaborate with another scoped guard > >> for this programming interface? > >> https://elixir.bootlin.com/linux/v6.12-rc2/source/include/linux/irqch > >> ip/chained > >> _irq.h#L13 > > > > Maybe like the change in the following? > > > > diff --git a/drivers/irqchip/irq-aspeed-intc.c > > b/drivers/irqchip/irq-aspeed-intc.c > > index ef1c095ad09e..54d1881c56c6 100644 > > --- a/drivers/irqchip/irq-aspeed-intc.c > > +++ b/drivers/irqchip/irq-aspeed-intc.c > > @@ -32,7 +32,7 @@ static void aspeed_intc_ic_irq_handler(struct irq_desc > *desc) > > struct irq_chip *chip = irq_desc_get_chip(desc); > > unsigned long bit, status; > > > > - chained_irq_enter(chip, desc); > > + guard(chained_irq)(desc); > > > > scoped_guard(raw_spinlock, &intc_ic->gic_lock) { > > status = readl(intc_ic->base + INTC_INT_STATUS_REG); > > Perhaps. > > > > @@ -41,8 +41,6 @@ static void aspeed_intc_ic_irq_handler(struct irq_desc > *desc) > > writel(BIT(bit), intc_ic->base + > INTC_INT_STATUS_REG); > > } > > } > > - > > - chained_irq_exit(chip, desc); > > } > … > > Probably, yes. > > > … > > +++ b/include/linux/irqchip/chained_irq.h > > @@ -38,4 +38,5 @@ static inline void chained_irq_exit(struct irq_chip *chip, > > chip->irq_unmask(&desc->irq_data); > > } > > > > +DEFINE_GUARD (chained_irq, struct irq_desc * , chained_irq_exit ( _T > > +->irq_data.chip, _T ), chained_irq_enter (_T->irq_data.chip, _T)) > … > > * Such a macro call looks promising. > Would you like to omit any space characters before open parentheses? OK. Fixed. diff --git a/include/linux/irqchip/chained_irq.h b/include/linux/irqchip/chained_irq.h index dd8b3c476666..7ee29e478152 100644 --- a/include/linux/irqchip/chained_irq.h +++ b/include/linux/irqchip/chained_irq.h @@ -38,4 +38,6 @@ static inline void chained_irq_exit(struct irq_chip *chip, chip->irq_unmask(&desc->irq_data); } +DEFINE_GUARD(chained_irq, struct irq_desc *, chained_irq_exit((_T->irq_data.chip), (_T)), + chained_irq_enter((_T->irq_data.chip), (_T))) > > * Would you like to support scoped guard variants accordingly? Do you mean that I need to change the MACRO name for this usage? > > > Regards, > Markus
> > I propose to move selected variable definitions into corresponding compound > > statements (by using extra curly brackets). > > https://refactoring.com/catalog/reduceScopeOfVariable.html > OK. I moved these two local variables into scoped_guard. Will development interests grow for further refactorings? > +static void aspeed_intc_ic_irq_handler(struct irq_desc *desc) > +{ > + struct aspeed_intc_ic *intc_ic = irq_desc_get_handler_data(desc); Another update candidate (for scope reduction)? > + > + guard(chained_irq)(desc); Using another macro call “scoped_guard(…) { … }”? > + scoped_guard(raw_spinlock, &intc_ic->gic_lock) { Would you like to reconsider the proposed macro mixture once more? > + unsigned long bit, status; … … > +++ b/include/linux/irqchip/chained_irq.h > @@ -38,4 +38,6 @@ static inline void chained_irq_exit(struct irq_chip *chip, > chip->irq_unmask(&desc->irq_data); > } > > +DEFINE_GUARD(chained_irq, struct irq_desc *, chained_irq_exit((_T->irq_data.chip), (_T)), > + chained_irq_enter((_T->irq_data.chip), (_T))) Would you like to add a #include directive in this header file accordingly? Regards, Markus
> > > > I propose to move selected variable definitions into corresponding > > > compound statements (by using extra curly brackets). > > > https://refactoring.com/catalog/reduceScopeOfVariable.html > > OK. I moved these two local variables into scoped_guard. > > Will development interests grow for further refactorings? Do you have any example for this refactorings? > > > > +static void aspeed_intc_ic_irq_handler(struct irq_desc *desc) { > > + struct aspeed_intc_ic *intc_ic = > > +irq_desc_get_handler_data(desc); > > Another update candidate (for scope reduction)? Variable of intc_ic used in "scoped_guard(raw_spinlock, &intc_ic->gic_lock) {". Or, how can I reduce the scope of intc_ic? > > > > + > > + guard(chained_irq)(desc); > > Using another macro call “scoped_guard(…) { … }”? Is it necessary to use scoped_guard(...) {...}? In the end of aspeed_intc_ic_irq_handler, chained_irq_exit would be called as destructor. Only one reason I thought is that the chained_irq_exit is needed to be called in the middle of aspeed_intc_ic_irq_handler. > > > > + scoped_guard(raw_spinlock, &intc_ic->gic_lock) { > > Would you like to reconsider the proposed macro mixture once more? Could I check the reason for once more? > > > > + unsigned long bit, status; > … > > … > > +++ b/include/linux/irqchip/chained_irq.h > > @@ -38,4 +38,6 @@ static inline void chained_irq_exit(struct irq_chip *chip, > > chip->irq_unmask(&desc->irq_data); > > } > > > > +DEFINE_GUARD(chained_irq, struct irq_desc *, > chained_irq_exit((_T->irq_data.chip), (_T)), > > + chained_irq_enter((_T->irq_data.chip), (_T))) > > Would you like to add a #include directive in this header file accordingly? Can you give me an example? > > Regards, > Markus
… > > > +static void aspeed_intc_ic_irq_handler(struct irq_desc *desc) { … > > > + guard(chained_irq)(desc); > > > > Using another macro call “scoped_guard(…) { … }”? > Is it necessary to use scoped_guard(...) {...}? It depends on corresponding case disintions. > > > + scoped_guard(raw_spinlock, &intc_ic->gic_lock) { > > > > Would you like to reconsider the proposed macro mixture once more? > Could I check the reason for once more? Coding style concerns …? > > > +++ b/include/linux/irqchip/chained_irq.h > > > @@ -38,4 +38,6 @@ static inline void chained_irq_exit(struct irq_chip *chip, > > > chip->irq_unmask(&desc->irq_data); > > > } > > > > > > +DEFINE_GUARD(chained_irq, struct irq_desc *, > > chained_irq_exit((_T->irq_data.chip), (_T)), > > > + chained_irq_enter((_T->irq_data.chip), (_T))) > > > > Would you like to add a #include directive in this header file accordingly? > Can you give me an example? See also: https://elixir.bootlin.com/linux/v6.12-rc3/source/include/linux/device.h#L33 Regards, Markus
On Wed, Oct 09 2024 at 14:32, Markus Elfring wrote: >> + >> + chained_irq_enter(chip, desc); > > Would you become interested to collaborate with another scoped guard > for this programming interface? Collaborate in which way? What are you collaborating on? You are merely asking people to do work which you think is useful. You can do that, but that does not make it useful. Making a guard variant of chained_irq_enter/exit needs some thought and a general plan for cleaning the whole chained irq usage up. It's on the cleanup list already with quite some other items. We are not adhoc adding a guard variant because guards are hip right now. And no this does not need a scoped variant ever. guards are not the panacea for everything. > https://elixir.bootlin.com/linux/v6.12-rc2/source/include/linux/irqchip/chained_irq.h#L13 Please refrain from these silly links. People know to find the functions on their own. Kevin, please update the change log, add your SOB and move the local variables (unsigned long bit, status;) into the scoped_guard() zone. Leave chained_irq_enter/exit() alone and resubmit. Thanks, tglx
> >> + > >> + chained_irq_enter(chip, desc); > > > > Would you become interested to collaborate with another scoped guard > > for this programming interface? > > Collaborate in which way? What are you collaborating on? > > You are merely asking people to do work which you think is useful. You can do > that, but that does not make it useful. > > Making a guard variant of chained_irq_enter/exit needs some thought and a > general plan for cleaning the whole chained irq usage up. It's on the cleanup > list already with quite some other items. > > We are not adhoc adding a guard variant because guards are hip right now. > And no this does not need a scoped variant ever. > > guards are not the panacea for everything. > > > https://elixir.bootlin.com/linux/v6.12-rc2/source/include/linux/irqchi > > p/chained_irq.h#L13 > > Please refrain from these silly links. People know to find the functions on their > own. > > Kevin, please update the change log, add your SOB and move the local > variables (unsigned long bit, status;) into the scoped_guard() zone. OK. I will do this change. > > Leave chained_irq_enter/exit() alone and resubmit. OK. I will send the v4 patch with chained_irq_enter/exit() alone. Thanks a lot. > > Thanks, > > tglx
> Making a guard variant of chained_irq_enter/exit needs some thought and > a general plan for cleaning the whole chained irq usage up. It's on the > cleanup list already with quite some other items. I became also curious how API usage will evolve further here. > We are not adhoc adding a guard variant because guards are hip right now. Application interests are growing, aren't they? > And no this does not need a scoped variant ever. There are subsystems which seem to prefer such a programming interface occasionally. > guards are not the panacea for everything. Their usage might become more popular. Regards, Markus
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile index e3679ec2b9f7..086911bf4db6 100644 --- a/drivers/irqchip/Makefile +++ b/drivers/irqchip/Makefile @@ -84,6 +84,7 @@ obj-$(CONFIG_MVEBU_SEI) += irq-mvebu-sei.o obj-$(CONFIG_LS_EXTIRQ) += irq-ls-extirq.o obj-$(CONFIG_LS_SCFG_MSI) += irq-ls-scfg-msi.o obj-$(CONFIG_ARCH_ASPEED) += irq-aspeed-vic.o irq-aspeed-i2c-ic.o irq-aspeed-scu-ic.o +obj-$(CONFIG_ARCH_ASPEED) += irq-aspeed-intc.o obj-$(CONFIG_STM32MP_EXTI) += irq-stm32mp-exti.o obj-$(CONFIG_STM32_EXTI) += irq-stm32-exti.o obj-$(CONFIG_QCOM_IRQ_COMBINER) += qcom-irq-combiner.o diff --git a/drivers/irqchip/irq-aspeed-intc.c b/drivers/irqchip/irq-aspeed-intc.c new file mode 100644 index 000000000000..ef1c095ad09e --- /dev/null +++ b/drivers/irqchip/irq-aspeed-intc.c @@ -0,0 +1,139 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Aspeed Interrupt Controller. + * + * Copyright (C) 2023 ASPEED Technology Inc. + */ + +#include <linux/bitops.h> +#include <linux/irq.h> +#include <linux/irqchip.h> +#include <linux/irqchip/chained_irq.h> +#include <linux/irqdomain.h> +#include <linux/of_address.h> +#include <linux/of_irq.h> +#include <linux/io.h> +#include <linux/spinlock.h> + +#define INTC_INT_ENABLE_REG 0x00 +#define INTC_INT_STATUS_REG 0x04 +#define IRQS_PER_WORD 32 + +struct aspeed_intc_ic { + void __iomem *base; + raw_spinlock_t gic_lock; + raw_spinlock_t intc_lock; + struct irq_domain *irq_domain; +}; + +static void aspeed_intc_ic_irq_handler(struct irq_desc *desc) +{ + struct aspeed_intc_ic *intc_ic = irq_desc_get_handler_data(desc); + struct irq_chip *chip = irq_desc_get_chip(desc); + unsigned long bit, status; + + chained_irq_enter(chip, desc); + + scoped_guard(raw_spinlock, &intc_ic->gic_lock) { + status = readl(intc_ic->base + INTC_INT_STATUS_REG); + for_each_set_bit(bit, &status, IRQS_PER_WORD) { + generic_handle_domain_irq(intc_ic->irq_domain, bit); + writel(BIT(bit), intc_ic->base + INTC_INT_STATUS_REG); + } + } + + chained_irq_exit(chip, desc); +} + +static void aspeed_intc_irq_mask(struct irq_data *data) +{ + struct aspeed_intc_ic *intc_ic = irq_data_get_irq_chip_data(data); + unsigned int mask = readl(intc_ic->base + INTC_INT_ENABLE_REG) & ~BIT(data->hwirq); + + guard(raw_spinlock)(&intc_ic->intc_lock); + writel(mask, intc_ic->base + INTC_INT_ENABLE_REG); +} + +static void aspeed_intc_irq_unmask(struct irq_data *data) +{ + struct aspeed_intc_ic *intc_ic = irq_data_get_irq_chip_data(data); + unsigned int unmask = readl(intc_ic->base + INTC_INT_ENABLE_REG) | BIT(data->hwirq); + + guard(raw_spinlock)(&intc_ic->intc_lock); + writel(unmask, intc_ic->base + INTC_INT_ENABLE_REG); +} + +static struct irq_chip aspeed_intc_chip = { + .name = "ASPEED INTC", + .irq_mask = aspeed_intc_irq_mask, + .irq_unmask = aspeed_intc_irq_unmask, +}; + +static int aspeed_intc_ic_map_irq_domain(struct irq_domain *domain, unsigned int irq, + irq_hw_number_t hwirq) +{ + irq_set_chip_and_handler(irq, &aspeed_intc_chip, handle_level_irq); + irq_set_chip_data(irq, domain->host_data); + + return 0; +} + +static const struct irq_domain_ops aspeed_intc_ic_irq_domain_ops = { + .map = aspeed_intc_ic_map_irq_domain, +}; + +static int __init aspeed_intc_ic_of_init(struct device_node *node, + struct device_node *parent) +{ + struct aspeed_intc_ic *intc_ic; + int ret = 0; + int irq, i; + + intc_ic = kzalloc(sizeof(*intc_ic), GFP_KERNEL); + if (!intc_ic) + return -ENOMEM; + + intc_ic->base = of_iomap(node, 0); + if (!intc_ic->base) { + pr_err("Failed to iomap intc_ic base\n"); + ret = -ENOMEM; + goto err_free_ic; + } + writel(0xffffffff, intc_ic->base + INTC_INT_STATUS_REG); + writel(0x0, intc_ic->base + INTC_INT_ENABLE_REG); + + intc_ic->irq_domain = irq_domain_add_linear(node, 32, + &aspeed_intc_ic_irq_domain_ops, intc_ic); + if (!intc_ic->irq_domain) { + ret = -ENOMEM; + goto err_iounmap; + } + + raw_spin_lock_init(&intc_ic->gic_lock); + raw_spin_lock_init(&intc_ic->intc_lock); + + /* Check all the irq numbers valid. If not, unmaps all the base and frees the data. */ + for (i = 0; i < of_irq_count(node); i++) { + irq = irq_of_parse_and_map(node, i); + if (!irq) { + pr_err("Failed to get irq number\n"); + ret = -EINVAL; + goto err_iounmap; + } + } + + for (i = 0; i < of_irq_count(node); i++) { + irq = irq_of_parse_and_map(node, i); + irq_set_chained_handler_and_data(irq, aspeed_intc_ic_irq_handler, intc_ic); + } + + return 0; + +err_iounmap: + iounmap(intc_ic->base); +err_free_ic: + kfree(intc_ic); + return ret; +} + +IRQCHIP_DECLARE(ast2700_intc_ic, "aspeed,ast2700-intc-ic", aspeed_intc_ic_of_init);