| Message ID | 20240814114106.2809876-4-kevin_chen@aspeedtech.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for AST2700 INTC driver | expand |
On 14/08/2024 13:41, Kevin Chen wrote: > Support for the Aspeed Interrupt Controller found on Aspeed Silicon SoCs, > such as the AST2700, which is arm64 architecture. > > To support ASPEED interrupt controller(INTC) maps the internal interrupt > sources of the AST27XX device to an parent interrupt controller. > --- > drivers/irqchip/Makefile | 1 + > drivers/irqchip/irq-aspeed-intc.c | 137 ++++++++++++++++++++++++++++++ > 2 files changed, 138 insertions(+) > create mode 100644 drivers/irqchip/irq-aspeed-intc.c > > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile > index 15635812b2d6..5da3f2f4eede 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_ASPEED_G7_INTC) += irq-aspeed-intc.o There is no such config symbol. You already got this exact comment. Replacing one broken code with other broken code is not the solution. Best regards, Krzysztof
On Wed, Aug 14 2024 at 19:41, Kevin Chen wrote: > Support for the Aspeed Interrupt Controller found on Aspeed Silicon SoCs, > such as the AST2700, which is arm64 architecture. > > To support ASPEED interrupt controller(INTC) maps the internal interrupt > sources of the AST27XX device to an parent interrupt controller. This still lacks a Signed-off-by: tag and my comment about the error path in the init function is still valid. Do you think that addressing review feedback is optional? Feel free to ignore it, but don't be surprised if I ignore further patches from you. Take your time and go through stuff properly and do not rush out half baked patches in a frenzy. Thanks, tglx
On 14/08/2024 17:06, Thomas Gleixner wrote: > On Wed, Aug 14 2024 at 19:41, Kevin Chen wrote: >> Support for the Aspeed Interrupt Controller found on Aspeed Silicon SoCs, >> such as the AST2700, which is arm64 architecture. >> >> To support ASPEED interrupt controller(INTC) maps the internal interrupt >> sources of the AST27XX device to an parent interrupt controller. > > This still lacks a Signed-off-by: tag and my comment about the error > path in the init function is still valid. > > Do you think that addressing review feedback is optional? > > Feel free to ignore it, but don't be surprised if I ignore further > patches from you. > > Take your time and go through stuff properly and do not rush out half > baked patches in a frenzy. > That's like fourth or fifth patchset for AST27xx within last week and all previous ones ignored given feedback. It's like sending almost the same stuff hoping maintainers will get bored and finally accept it. Tricky to say, maybe this works well in corporations? Best regards, Krzysztof
> > Support for the Aspeed Interrupt Controller found on Aspeed Silicon > > SoCs, such as the AST2700, which is arm64 architecture. > > > > To support ASPEED interrupt controller(INTC) maps the internal > > interrupt sources of the AST27XX device to an parent interrupt controller. > > --- > > drivers/irqchip/Makefile | 1 + > > drivers/irqchip/irq-aspeed-intc.c | 137 > > ++++++++++++++++++++++++++++++ > > 2 files changed, 138 insertions(+) > > create mode 100644 drivers/irqchip/irq-aspeed-intc.c > > > > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile index > > 15635812b2d6..5da3f2f4eede 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_ASPEED_G7_INTC) += irq-aspeed-intc.o > > > There is no such config symbol. Agree, I will use CONFIG_ARCH_ASPEED as the following. obj-$(CONFIG_ARCH_ASPEED) += irq-aspeed-intc.o > > You already got this exact comment. Replacing one broken code with other > broken code is not the solution. > > > Best regards, > Krzysztof
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile index 15635812b2d6..5da3f2f4eede 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_ASPEED_G7_INTC) += 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..748cc30b1485 --- /dev/null +++ b/drivers/irqchip/irq-aspeed-intc.c @@ -0,0 +1,137 @@ +// 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); + + intc_ic->irq_domain->name = "aspeed-intc-domain"; + + 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; + } else { + 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);