* [PATCH v1 2/2] irqchip/aspeed-intc: Add support for 10 INTC interrupts on AST27XX platforms
@ 2024-08-13 7:43 ` Kevin Chen
0 siblings, 0 replies; 22+ messages in thread
From: Kevin Chen @ 2024-08-13 7:43 UTC (permalink / raw)
To: tglx, robh, krzk+dt, conor+dt, joel, andrew, kevin_chen,
linux-kernel, devicetree, linux-arm-kernel, linux-aspeed
There are 10 interrupt source of soc0_intc in CPU die INTC.
1. 6 interrupt sources in IO die of soc1_intc0~soc1_intc5.
2. 2 interrupt sources in LTPI of ltpi0_intc0 and ltpi0_intc1.
3. 2 interrupt sources in LTPI of ltpi1_intc0 and ltpi1_intc1.
Request GIC interrupt to check each bit in status register to do next
level INTC handler.
In next level INTC handler of IO die or LTPI INTC using soc1_intcX combining
32 interrupt sources into soc0_intc11 in CPU die. In soc1_intcX, handler
would check 32 bit of status register to do the requested device
handler.
---
drivers/irqchip/Makefile | 1 +
drivers/irqchip/irq-aspeed-intc.c | 198 ++++++++++++++++++++++++++++++
2 files changed, 199 insertions(+)
create mode 100644 drivers/irqchip/irq-aspeed-intc.c
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 15635812b2d6..d2fe686ae018 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_MACH_ASPEED_G7) += 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..71407475fb27
--- /dev/null
+++ b/drivers/irqchip/irq-aspeed-intc.c
@@ -0,0 +1,198 @@
+// 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
+
+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, flags;
+
+ chained_irq_enter(chip, desc);
+
+ raw_spin_lock_irqsave(&intc_ic->gic_lock, flags);
+ status = readl(intc_ic->base + INTC_INT_STATUS_REG);
+ for_each_set_bit(bit, &status, 32) {
+ generic_handle_domain_irq(intc_ic->irq_domain, bit);
+ writel(BIT(bit), intc_ic->base + INTC_INT_STATUS_REG);
+ }
+ raw_spin_unlock_irqrestore(&intc_ic->gic_lock, flags);
+
+ 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);
+ unsigned long flags;
+
+ raw_spin_lock_irqsave(&intc_ic->intc_lock, flags);
+ writel(mask, intc_ic->base + INTC_INT_ENABLE_REG);
+ raw_spin_unlock_irqrestore(&intc_ic->intc_lock, flags);
+}
+
+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);
+ unsigned long flags;
+
+ raw_spin_lock_irqsave(&intc_ic->intc_lock, flags);
+ writel(unmask, intc_ic->base + INTC_INT_ENABLE_REG);
+ raw_spin_unlock_irqrestore(&intc_ic->intc_lock, flags);
+}
+
+static int aspeed_intc_irq_set_affinity(struct irq_data *data, const struct cpumask *dest,
+ bool force)
+{
+ return -EINVAL;
+}
+
+static struct irq_chip aspeed_intc_chip = {
+ .name = "ASPEED INTC",
+ .irq_mask = aspeed_intc_irq_mask,
+ .irq_unmask = aspeed_intc_irq_unmask,
+ .irq_set_affinity = aspeed_intc_irq_set_affinity,
+};
+
+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;
+
+ 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);
+
+ irq = irq_of_parse_and_map(node, 0);
+ if (!irq) {
+ pr_err("Failed to get irq number\n");
+ ret = -EINVAL;
+ goto err_iounmap;
+ }
+
+ 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";
+
+ 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;
+}
+
+static int __init aspeed_intc_ic_of_init_v2(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-1.0", aspeed_intc_ic_of_init);
+IRQCHIP_DECLARE(ast2700_intc_icv2, "aspeed,ast2700-intc-2.0", aspeed_intc_ic_of_init_v2);
--
2.34.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v1 2/2] irqchip/aspeed-intc: Add support for 10 INTC interrupts on AST27XX platforms
2024-08-13 7:43 ` Kevin Chen
@ 2024-08-13 8:50 ` Krzysztof Kozlowski
-1 siblings, 0 replies; 22+ messages in thread
From: Krzysztof Kozlowski @ 2024-08-13 8:50 UTC (permalink / raw)
To: linux-aspeed
On 13/08/2024 09:43, Kevin Chen wrote:
> There are 10 interrupt source of soc0_intc in CPU die INTC.
> 1. 6 interrupt sources in IO die of soc1_intc0~soc1_intc5.
> 2. 2 interrupt sources in LTPI of ltpi0_intc0 and ltpi0_intc1.
> 3. 2 interrupt sources in LTPI of ltpi1_intc0 and ltpi1_intc1.
> Request GIC interrupt to check each bit in status register to do next
> level INTC handler.
>
> In next level INTC handler of IO die or LTPI INTC using soc1_intcX combining
> 32 interrupt sources into soc0_intc11 in CPU die. In soc1_intcX, handler
> would check 32 bit of status register to do the requested device
> handler.
> ---
> drivers/irqchip/Makefile | 1 +
> drivers/irqchip/irq-aspeed-intc.c | 198 ++++++++++++++++++++++++++++++
> 2 files changed, 199 insertions(+)
> create mode 100644 drivers/irqchip/irq-aspeed-intc.c
>
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 15635812b2d6..d2fe686ae018 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_MACH_ASPEED_G7) += irq-aspeed-intc.o
There is no such thing as CONFIG_MACH_ASPEED_G7. And there will never be.
You already received feedback on this, so why do you keep pushing your
solution? You did not respond to any feedback given, just send the same
and the same till we agree?
NAK.
> 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..71407475fb27
...
> +static int __init aspeed_intc_ic_of_init_v2(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;
> +}
Don't duplicate code. These are almost the same, so define one function
which is then called by aspeed_intc_ic_of_init and
aspeed_intc_ic_of_init_v2.
> +
> +IRQCHIP_DECLARE(ast2700_intc_ic, "aspeed,ast2700-intc-1.0", aspeed_intc_ic_of_init);
> +IRQCHIP_DECLARE(ast2700_intc_icv2, "aspeed,ast2700-intc-2.0", aspeed_intc_ic_of_init_v2);
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v1 2/2] irqchip/aspeed-intc: Add support for 10 INTC interrupts on AST27XX platforms
@ 2024-08-13 8:50 ` Krzysztof Kozlowski
0 siblings, 0 replies; 22+ messages in thread
From: Krzysztof Kozlowski @ 2024-08-13 8:50 UTC (permalink / raw)
To: Kevin Chen, tglx, robh, krzk+dt, conor+dt, joel, andrew,
linux-kernel, devicetree, linux-arm-kernel, linux-aspeed
On 13/08/2024 09:43, Kevin Chen wrote:
> There are 10 interrupt source of soc0_intc in CPU die INTC.
> 1. 6 interrupt sources in IO die of soc1_intc0~soc1_intc5.
> 2. 2 interrupt sources in LTPI of ltpi0_intc0 and ltpi0_intc1.
> 3. 2 interrupt sources in LTPI of ltpi1_intc0 and ltpi1_intc1.
> Request GIC interrupt to check each bit in status register to do next
> level INTC handler.
>
> In next level INTC handler of IO die or LTPI INTC using soc1_intcX combining
> 32 interrupt sources into soc0_intc11 in CPU die. In soc1_intcX, handler
> would check 32 bit of status register to do the requested device
> handler.
> ---
> drivers/irqchip/Makefile | 1 +
> drivers/irqchip/irq-aspeed-intc.c | 198 ++++++++++++++++++++++++++++++
> 2 files changed, 199 insertions(+)
> create mode 100644 drivers/irqchip/irq-aspeed-intc.c
>
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 15635812b2d6..d2fe686ae018 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_MACH_ASPEED_G7) += irq-aspeed-intc.o
There is no such thing as CONFIG_MACH_ASPEED_G7. And there will never be.
You already received feedback on this, so why do you keep pushing your
solution? You did not respond to any feedback given, just send the same
and the same till we agree?
NAK.
> 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..71407475fb27
...
> +static int __init aspeed_intc_ic_of_init_v2(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;
> +}
Don't duplicate code. These are almost the same, so define one function
which is then called by aspeed_intc_ic_of_init and
aspeed_intc_ic_of_init_v2.
> +
> +IRQCHIP_DECLARE(ast2700_intc_ic, "aspeed,ast2700-intc-1.0", aspeed_intc_ic_of_init);
> +IRQCHIP_DECLARE(ast2700_intc_icv2, "aspeed,ast2700-intc-2.0", aspeed_intc_ic_of_init_v2);
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 22+ messages in thread* 回覆: [PATCH v1 2/2] irqchip/aspeed-intc: Add support for 10 INTC interrupts on AST27XX platforms
2024-08-13 8:50 ` Krzysztof Kozlowski
(?)
@ 2024-08-13 9:44 ` Kevin Chen
2024-08-13 9:48 ` Krzysztof Kozlowski
-1 siblings, 1 reply; 22+ messages in thread
From: Kevin Chen @ 2024-08-13 9:44 UTC (permalink / raw)
To: linux-aspeed
Hi Krzk,
In ASPEED, ast2400/2500/2600 use arm architecture with KCONFIG_ARCH_ASPEED which slect MACH_ASPEED_G4/G5/G6 in arch/arm/mach-aspeed/Kconfig.
In the fureture, there would be ast2800/2900/... using arm64. We need to clarify the IC generation between 7th/8th/9th/....
Maybe change ARCH_ASPEED/MACH_ASPEEDG7 to ARCH_ASPEED first.
Or, do you have better Kconfig usage?
> +config ARCH_ASPEED
> + bool "Aspeed SoC family"
> + select MACH_ASPEED_G7
> + help
> + Say yes if you intend to run on an Aspeed ast2700 or similar
> + seventh generation Aspeed BMCs.
> +
> +config MACH_ASPEED_G7
> + bool "Aspeed SoC AST2700"
There are no MACHines for arm64. Look at this code. Do you see MACH
anywhere else? No. Then why Aspeed must be different?
--
Best Regards,
Kevin. Chen
________________________________
???: Krzysztof Kozlowski <krzk@kernel.org>
????: 2024?8?13? ?? 04:50
???: Kevin Chen <kevin_chen@aspeedtech.com>; tglx at linutronix.de <tglx@linutronix.de>; robh at kernel.org <robh@kernel.org>; krzk+dt at kernel.org <krzk+dt@kernel.org>; conor+dt at kernel.org <conor+dt@kernel.org>; joel at jms.id.au <joel@jms.id.au>; andrew at codeconstruct.com.au <andrew@codeconstruct.com.au>; linux-kernel at vger.kernel.org <linux-kernel@vger.kernel.org>; devicetree at vger.kernel.org <devicetree@vger.kernel.org>; linux-arm-kernel at lists.infradead.org <linux-arm-kernel@lists.infradead.org>; linux-aspeed at lists.ozlabs.org <linux-aspeed@lists.ozlabs.org>
??: Re: [PATCH v1 2/2] irqchip/aspeed-intc: Add support for 10 INTC interrupts on AST27XX platforms
On 13/08/2024 09:43, Kevin Chen wrote:
> There are 10 interrupt source of soc0_intc in CPU die INTC.
> 1. 6 interrupt sources in IO die of soc1_intc0~soc1_intc5.
> 2. 2 interrupt sources in LTPI of ltpi0_intc0 and ltpi0_intc1.
> 3. 2 interrupt sources in LTPI of ltpi1_intc0 and ltpi1_intc1.
> Request GIC interrupt to check each bit in status register to do next
> level INTC handler.
>
> In next level INTC handler of IO die or LTPI INTC using soc1_intcX combining
> 32 interrupt sources into soc0_intc11 in CPU die. In soc1_intcX, handler
> would check 32 bit of status register to do the requested device
> handler.
> ---
> drivers/irqchip/Makefile | 1 +
> drivers/irqchip/irq-aspeed-intc.c | 198 ++++++++++++++++++++++++++++++
> 2 files changed, 199 insertions(+)
> create mode 100644 drivers/irqchip/irq-aspeed-intc.c
>
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 15635812b2d6..d2fe686ae018 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_MACH_ASPEED_G7) += irq-aspeed-intc.o
There is no such thing as CONFIG_MACH_ASPEED_G7. And there will never be.
You already received feedback on this, so why do you keep pushing your
solution? You did not respond to any feedback given, just send the same
and the same till we agree?
NAK.
> 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..71407475fb27
...
> +static int __init aspeed_intc_ic_of_init_v2(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;
> +}
Don't duplicate code. These are almost the same, so define one function
which is then called by aspeed_intc_ic_of_init and
aspeed_intc_ic_of_init_v2.
> +
> +IRQCHIP_DECLARE(ast2700_intc_ic, "aspeed,ast2700-intc-1.0", aspeed_intc_ic_of_init);
> +IRQCHIP_DECLARE(ast2700_intc_icv2, "aspeed,ast2700-intc-2.0", aspeed_intc_ic_of_init_v2);
Best regards,
Krzysztof
************* Email Confidentiality Notice ********************
????:
???(????)????????????????? ???????????????????????????, ??????????????????????????????!
DISCLAIMER:
This message (and any attachments) may contain legally privileged and/or other confidential information. If you have received it in error, please notify the sender by reply e-mail and immediately delete the e-mail and any attachments without copying or disclosing the contents. Thank you.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/linux-aspeed/attachments/20240813/82333479/attachment-0001.htm>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: 回覆: [PATCH v1 2/2] irqchip/aspeed-intc: Add support for 10 INTC interrupts on AST27XX platforms
2024-08-13 9:44 ` 回覆: " Kevin Chen
@ 2024-08-13 9:48 ` Krzysztof Kozlowski
0 siblings, 0 replies; 22+ messages in thread
From: Krzysztof Kozlowski @ 2024-08-13 9:48 UTC (permalink / raw)
To: linux-aspeed
On 13/08/2024 11:44, Kevin Chen wrote:
> Hi Krzk,
>
> In ASPEED, ast2400/2500/2600 use arm architecture with KCONFIG_ARCH_ASPEED which slect MACH_ASPEED_G4/G5/G6 in arch/arm/mach-aspeed/Kconfig.
> In the fureture, there would be ast2800/2900/... using arm64. We need to clarify the IC generation between 7th/8th/9th/....
>
> Maybe change ARCH_ASPEED/MACH_ASPEEDG7 to ARCH_ASPEED first.
> Or, do you have better Kconfig usage?
Fix your quotes and do not top-post.
Please respond inline, instead of top-posting, because it makes your
emails hard to follow.
https://elixir.bootlin.com/linux/v6.8-rc7/source/Documentation/process/submitting-patches.rst#L340
>
>
>> +config ARCH_ASPEED
>> + bool "Aspeed SoC family"
>> + select MACH_ASPEED_G7
>> + help
>> + Say yes if you intend to run on an Aspeed ast2700 or similar
>> + seventh generation Aspeed BMCs.
>> +
>> +config MACH_ASPEED_G7
>> + bool "Aspeed SoC AST2700"
>
> There are no MACHines for arm64. Look at this code. Do you see MACH
> anywhere else? No. Then why Aspeed must be different?
What is this?
>
> --
> Best Regards,
> Kevin. Chen
>
> ________________________________
> ???: Krzysztof Kozlowski <krzk@kernel.org>
> ????: 2024?8?13? ?? 04:50
> ???: Kevin Chen <kevin_chen@aspeedtech.com>; tglx at linutronix.de <tglx@linutronix.de>; robh at kernel.org <robh@kernel.org>; krzk+dt at kernel.org <krzk+dt@kernel.org>; conor+dt at kernel.org <conor+dt@kernel.org>; joel at jms.id.au <joel@jms.id.au>; andrew at codeconstruct.com.au <andrew@codeconstruct.com.au>; linux-kernel at vger.kernel.org <linux-kernel@vger.kernel.org>; devicetree at vger.kernel.org <devicetree@vger.kernel.org>; linux-arm-kernel at lists.infradead.org <linux-arm-kernel@lists.infradead.org>; linux-aspeed at lists.ozlabs.org <linux-aspeed@lists.ozlabs.org>
> ??: Re: [PATCH v1 2/2] irqchip/aspeed-intc: Add support for 10 INTC interrupts on AST27XX platforms
>
...
>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>> index 15635812b2d6..d2fe686ae018 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_MACH_ASPEED_G7) += irq-aspeed-intc.o
>
> There is no such thing as CONFIG_MACH_ASPEED_G7. And there will never be.
>
> You already received feedback on this, so why do you keep pushing your
> solution? You did not respond to any feedback given, just send the same
> and the same till we agree?
>
> NAK.
And this?
>
>> 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..71407475fb27
>
...
>
> ************* Email Confidentiality Notice ********************
> ????:
> ???(????)????????????????? ???????????????????????????, ??????????????????????????????!
>
> DISCLAIMER:
> This message (and any attachments) may contain legally privileged and/or other confidential information. If you have received it in error, please notify the sender by reply e-mail and immediately delete the e-mail and any attachments without copying or disclosing the contents. Thank you.
Maybe I am the intended recipient of your message, maybe not. I don't
want to have any legal questions regarding upstream, public
collaboration, thus probably I should just remove your messages.
Please talk with your IT that such disclaimers in open-source are not
desired (and maybe even harmful).
If you do not understand why, please also see:
https://www.youtube.com/live/fMeH7wqOwXA?si=GY7igfbda6vnjXlJ&t=835
If you need to go around company SMTP server, then consider using b4
web-relay: https://b4.docs.kernel.org/en/latest/contributor/send.html
Please be informed that by responding to this email you agree that all
communications from you and/or your company is made public. In other
words, all messages originating from you and/or your company will be
made public.
You already received exactly this feedback. Around three times. If you
keep ignoring feedback, I will keep NAKing your patches.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: 回覆: [PATCH v1 2/2] irqchip/aspeed-intc: Add support for 10 INTC interrupts on AST27XX platforms
@ 2024-08-13 9:48 ` Krzysztof Kozlowski
0 siblings, 0 replies; 22+ messages in thread
From: Krzysztof Kozlowski @ 2024-08-13 9:48 UTC (permalink / raw)
To: Kevin Chen, tglx@linutronix.de, robh@kernel.org,
krzk+dt@kernel.org, conor+dt@kernel.org, joel@jms.id.au,
andrew@codeconstruct.com.au, linux-kernel@vger.kernel.org,
devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-aspeed@lists.ozlabs.org
On 13/08/2024 11:44, Kevin Chen wrote:
> Hi Krzk,
>
> In ASPEED, ast2400/2500/2600 use arm architecture with KCONFIG_ARCH_ASPEED which slect MACH_ASPEED_G4/G5/G6 in arch/arm/mach-aspeed/Kconfig.
> In the fureture, there would be ast2800/2900/... using arm64. We need to clarify the IC generation between 7th/8th/9th/....
>
> Maybe change ARCH_ASPEED/MACH_ASPEEDG7 to ARCH_ASPEED first.
> Or, do you have better Kconfig usage?
Fix your quotes and do not top-post.
Please respond inline, instead of top-posting, because it makes your
emails hard to follow.
https://elixir.bootlin.com/linux/v6.8-rc7/source/Documentation/process/submitting-patches.rst#L340
>
>
>> +config ARCH_ASPEED
>> + bool "Aspeed SoC family"
>> + select MACH_ASPEED_G7
>> + help
>> + Say yes if you intend to run on an Aspeed ast2700 or similar
>> + seventh generation Aspeed BMCs.
>> +
>> +config MACH_ASPEED_G7
>> + bool "Aspeed SoC AST2700"
>
> There are no MACHines for arm64. Look at this code. Do you see MACH
> anywhere else? No. Then why Aspeed must be different?
What is this?
>
> --
> Best Regards,
> Kevin. Chen
>
> ________________________________
> 寄件者: Krzysztof Kozlowski <krzk@kernel.org>
> 寄件日期: 2024年8月13日 下午 04:50
> 收件者: Kevin Chen <kevin_chen@aspeedtech.com>; tglx@linutronix.de <tglx@linutronix.de>; robh@kernel.org <robh@kernel.org>; krzk+dt@kernel.org <krzk+dt@kernel.org>; conor+dt@kernel.org <conor+dt@kernel.org>; joel@jms.id.au <joel@jms.id.au>; andrew@codeconstruct.com.au <andrew@codeconstruct.com.au>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>; devicetree@vger.kernel.org <devicetree@vger.kernel.org>; linux-arm-kernel@lists.infradead.org <linux-arm-kernel@lists.infradead.org>; linux-aspeed@lists.ozlabs.org <linux-aspeed@lists.ozlabs.org>
> 主旨: Re: [PATCH v1 2/2] irqchip/aspeed-intc: Add support for 10 INTC interrupts on AST27XX platforms
>
...
>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>> index 15635812b2d6..d2fe686ae018 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_MACH_ASPEED_G7) += irq-aspeed-intc.o
>
> There is no such thing as CONFIG_MACH_ASPEED_G7. And there will never be.
>
> You already received feedback on this, so why do you keep pushing your
> solution? You did not respond to any feedback given, just send the same
> and the same till we agree?
>
> NAK.
And this?
>
>> 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..71407475fb27
>
...
>
> ************* Email Confidentiality Notice ********************
> 免責聲明:
> 本信件(或其附件)可能包含機密資訊，並受法律保護。如 台端非指定之收件者，請以電子郵件通知本電子郵件之發送者, 並請立即刪除本電子郵件及其附件和銷毀所有複印件。謝謝您的合作!
>
> DISCLAIMER:
> This message (and any attachments) may contain legally privileged and/or other confidential information. If you have received it in error, please notify the sender by reply e-mail and immediately delete the e-mail and any attachments without copying or disclosing the contents. Thank you.
Maybe I am the intended recipient of your message, maybe not. I don't
want to have any legal questions regarding upstream, public
collaboration, thus probably I should just remove your messages.
Please talk with your IT that such disclaimers in open-source are not
desired (and maybe even harmful).
If you do not understand why, please also see:
https://www.youtube.com/live/fMeH7wqOwXA?si=GY7igfbda6vnjXlJ&t=835
If you need to go around company SMTP server, then consider using b4
web-relay: https://b4.docs.kernel.org/en/latest/contributor/send.html
Please be informed that by responding to this email you agree that all
communications from you and/or your company is made public. In other
words, all messages originating from you and/or your company will be
made public.
You already received exactly this feedback. Around three times. If you
keep ignoring feedback, I will keep NAKing your patches.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 22+ messages in thread
* 回覆: 回覆: [PATCH v1 2/2] irqchip/aspeed-intc: Add support for 10 INTC interrupts on AST27XX platforms
2024-08-13 9:48 ` Krzysztof Kozlowski
(?)
@ 2024-08-13 10:33 ` Kevin Chen
-1 siblings, 0 replies; 22+ messages in thread
From: Kevin Chen @ 2024-08-13 10:33 UTC (permalink / raw)
To: linux-aspeed
Hi Krzk,
>> Hi Krzk,
>>
>> In ASPEED, ast2400/2500/2600 use arm architecture with KCONFIG_ARCH_ASPEED which slect MACH_ASPEED_G4/G5/G6 in arch/arm/mach-aspeed/Kconfig.
>> In the fureture, there would be ast2800/2900/... using arm64. We need to clarify the IC generation between 7th/8th/9th/....
>>
>> Maybe change ARCH_ASPEED/MACH_ASPEEDG7 to ARCH_ASPEED first.
>> Or, do you have better Kconfig usage?
>
>Fix your quotes and do not top-post.
>
>Please respond inline, instead of top-posting, because it makes your
>emails hard to follow.
>https://elixir.bootlin.com/linux/v6.8-rc7/source/Documentation/process/submitting-patches.rst#L340
>
>>
>>
>>> +config ARCH_ASPEED
>>> + bool "Aspeed SoC family"
>>> + select MACH_ASPEED_G7
>>> + help
>>> + Say yes if you intend to run on an Aspeed ast2700 or similar
>>> + seventh generation Aspeed BMCs.
>>> +
>>> +config MACH_ASPEED_G7
>>> + bool "Aspeed SoC AST2700"
>>
>> There are no MACHines for arm64. Look at this code. Do you see MACH
>> anywhere else? No. Then why Aspeed must be different?
>
>What is this?
OK. I will remove MACH_ASPEED_G7 and save ARCH_ASPEED for use.
--
Best Regards,
Kevin. Chen
________________________________
???: Krzysztof Kozlowski <krzk@kernel.org>
????: 2024?8?13? ?? 05:48
???: Kevin Chen <kevin_chen@aspeedtech.com>; tglx at linutronix.de <tglx@linutronix.de>; robh at kernel.org <robh@kernel.org>; krzk+dt at kernel.org <krzk+dt@kernel.org>; conor+dt at kernel.org <conor+dt@kernel.org>; joel at jms.id.au <joel@jms.id.au>; andrew at codeconstruct.com.au <andrew@codeconstruct.com.au>; linux-kernel at vger.kernel.org <linux-kernel@vger.kernel.org>; devicetree at vger.kernel.org <devicetree@vger.kernel.org>; linux-arm-kernel at lists.infradead.org <linux-arm-kernel@lists.infradead.org>; linux-aspeed at lists.ozlabs.org <linux-aspeed@lists.ozlabs.org>
??: Re: ??: [PATCH v1 2/2] irqchip/aspeed-intc: Add support for 10 INTC interrupts on AST27XX platforms
On 13/08/2024 11:44, Kevin Chen wrote:
> Hi Krzk,
>
> In ASPEED, ast2400/2500/2600 use arm architecture with KCONFIG_ARCH_ASPEED which slect MACH_ASPEED_G4/G5/G6 in arch/arm/mach-aspeed/Kconfig.
> In the fureture, there would be ast2800/2900/... using arm64. We need to clarify the IC generation between 7th/8th/9th/....
>
> Maybe change ARCH_ASPEED/MACH_ASPEEDG7 to ARCH_ASPEED first.
> Or, do you have better Kconfig usage?
Fix your quotes and do not top-post.
Please respond inline, instead of top-posting, because it makes your
emails hard to follow.
https://elixir.bootlin.com/linux/v6.8-rc7/source/Documentation/process/submitting-patches.rst#L340
>
>
>> +config ARCH_ASPEED
>> + bool "Aspeed SoC family"
>> + select MACH_ASPEED_G7
>> + help
>> + Say yes if you intend to run on an Aspeed ast2700 or similar
>> + seventh generation Aspeed BMCs.
>> +
>> +config MACH_ASPEED_G7
>> + bool "Aspeed SoC AST2700"
>
> There are no MACHines for arm64. Look at this code. Do you see MACH
> anywhere else? No. Then why Aspeed must be different?
What is this?
>
> --
> Best Regards,
> Kevin. Chen
>
> ________________________________
> ???: Krzysztof Kozlowski <krzk@kernel.org>
> ????: 2024?8?13? ?? 04:50
> ???: Kevin Chen <kevin_chen@aspeedtech.com>; tglx at linutronix.de <tglx@linutronix.de>; robh at kernel.org <robh@kernel.org>; krzk+dt at kernel.org <krzk+dt@kernel.org>; conor+dt at kernel.org <conor+dt@kernel.org>; joel at jms.id.au <joel@jms.id.au>; andrew at codeconstruct.com.au <andrew@codeconstruct.com.au>; linux-kernel at vger.kernel.org <linux-kernel@vger.kernel.org>; devicetree at vger.kernel.org <devicetree@vger.kernel.org>; linux-arm-kernel at lists.infradead.org <linux-arm-kernel@lists.infradead.org>; linux-aspeed at lists.ozlabs.org <linux-aspeed@lists.ozlabs.org>
> ??: Re: [PATCH v1 2/2] irqchip/aspeed-intc: Add support for 10 INTC interrupts on AST27XX platforms
>
...
>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>> index 15635812b2d6..d2fe686ae018 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_MACH_ASPEED_G7) += irq-aspeed-intc.o
>
> There is no such thing as CONFIG_MACH_ASPEED_G7. And there will never be.
>
> You already received feedback on this, so why do you keep pushing your
> solution? You did not respond to any feedback given, just send the same
> and the same till we agree?
>
> NAK.
And this?
>
>> 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..71407475fb27
>
...
>
> ************* Email Confidentiality Notice ********************
> ????:
> ???(????)????????????????? ???????????????????????????, ??????????????????????????????!
>
> DISCLAIMER:
> This message (and any attachments) may contain legally privileged and/or other confidential information. If you have received it in error, please notify the sender by reply e-mail and immediately delete the e-mail and any attachments without copying or disclosing the contents. Thank you.
Maybe I am the intended recipient of your message, maybe not. I don't
want to have any legal questions regarding upstream, public
collaboration, thus probably I should just remove your messages.
Please talk with your IT that such disclaimers in open-source are not
desired (and maybe even harmful).
If you do not understand why, please also see:
https://www.youtube.com/live/fMeH7wqOwXA?si=GY7igfbda6vnjXlJ&t=835
If you need to go around company SMTP server, then consider using b4
web-relay: https://b4.docs.kernel.org/en/latest/contributor/send.html
Please be informed that by responding to this email you agree that all
communications from you and/or your company is made public. In other
words, all messages originating from you and/or your company will be
made public.
You already received exactly this feedback. Around three times. If you
keep ignoring feedback, I will keep NAKing your patches.
Best regards,
Krzysztof
************* Email Confidentiality Notice ********************
????:
???(????)????????????????? ???????????????????????????, ??????????????????????????????!
DISCLAIMER:
This message (and any attachments) may contain legally privileged and/or other confidential information. If you have received it in error, please notify the sender by reply e-mail and immediately delete the e-mail and any attachments without copying or disclosing the contents. Thank you.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/linux-aspeed/attachments/20240813/a51d2c53/attachment-0001.htm>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v1 2/2] irqchip/aspeed-intc: Add support for 10 INTC interrupts on AST27XX platforms
2024-08-13 7:43 ` Kevin Chen
@ 2024-08-13 9:35 ` Thomas Gleixner
-1 siblings, 0 replies; 22+ messages in thread
From: Thomas Gleixner @ 2024-08-13 9:35 UTC (permalink / raw)
To: linux-aspeed
On Tue, Aug 13 2024 at 15:43, Kevin Chen wrote:
> There are 10 interrupt source of soc0_intc in CPU die INTC.
> 1. 6 interrupt sources in IO die of soc1_intc0~soc1_intc5.
> 2. 2 interrupt sources in LTPI of ltpi0_intc0 and ltpi0_intc1.
> 3. 2 interrupt sources in LTPI of ltpi1_intc0 and ltpi1_intc1.
> Request GIC interrupt to check each bit in status register to do next
> level INTC handler.
>
> In next level INTC handler of IO die or LTPI INTC using soc1_intcX combining
> 32 interrupt sources into soc0_intc11 in CPU die. In soc1_intcX, handler
> would check 32 bit of status register to do the requested device
> handler.
I can't figure out what this word salad is trying to tell me. Nothing in
the code does any combining. The handler reads the very same
INTC_INT_STATUS_REG.
>
This lacks a Signed-off-by: tag. See Documentation/process/
> ---
> drivers/irqchip/Makefile | 1 +
> drivers/irqchip/irq-aspeed-intc.c | 198 ++++++++++++++++++++++++++++++
> +
> +#define INTC_INT_ENABLE_REG 0x00
> +#define INTC_INT_STATUS_REG 0x04
> +
> +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, flags;
> +
> + chained_irq_enter(chip, desc);
> +
> + raw_spin_lock_irqsave(&intc_ic->gic_lock, flags);
There is no point for irqsave(). This code is invoked with interrupts
disabled and please convert to:
scoped_guard(raw_spinlock, &intc_ic->gic_lock) {
> + status = readl(intc_ic->base + INTC_INT_STATUS_REG);
> + for_each_set_bit(bit, &status, 32) {
Please use a define and not a hardcoded number.
> + generic_handle_domain_irq(intc_ic->irq_domain, bit);
> + writel(BIT(bit), intc_ic->base + INTC_INT_STATUS_REG);
> + }
}
> + raw_spin_unlock_irqrestore(&intc_ic->gic_lock, flags);
> +
> + 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);
> + unsigned long flags;
Invoked with interrupts disabled too.
> + raw_spin_lock_irqsave(&intc_ic->intc_lock, flags);
> + writel(mask, intc_ic->base + INTC_INT_ENABLE_REG);
> + raw_spin_unlock_irqrestore(&intc_ic->intc_lock, flags);
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);
> + unsigned long flags;
Ditto.
> + raw_spin_lock_irqsave(&intc_ic->intc_lock, flags);
> + writel(unmask, intc_ic->base + INTC_INT_ENABLE_REG);
> + raw_spin_unlock_irqrestore(&intc_ic->intc_lock, flags);
> +}
> +
> +static int aspeed_intc_irq_set_affinity(struct irq_data *data, const struct cpumask *dest,
> + bool force)
> +{
> + return -EINVAL;
> +}
No point for this stub, just leave irq_set_affinity uninitialized. The
core code checks that pointer for NULL. Aside of that this stub and the
assignment would need a #ifdef CONFIG_SMP guard.
> +static struct irq_chip aspeed_intc_chip = {
> + .name = "ASPEED INTC",
> + .irq_mask = aspeed_intc_irq_mask,
> + .irq_unmask = aspeed_intc_irq_unmask,
> + .irq_set_affinity = aspeed_intc_irq_set_affinity,
> +};
> +
> +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,
.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;
int irq, ret;
No point in initializing ret.
> + 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);
> +
> + irq = irq_of_parse_and_map(node, 0);
> + if (!irq) {
> + pr_err("Failed to get irq number\n");
> + ret = -EINVAL;
> + goto err_iounmap;
> + }
> +
> + 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";
See above.
> + 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;
> +}
> +
> +static int __init aspeed_intc_ic_of_init_v2(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";
So up to this point aspeed_intc_ic_of_init_v2() is a verbatim copy of
aspeed_intc_ic_of_init(). Why can't you reuse that function? It's not
rocket science to make that work.
> + 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;
Assume #0 and #1 succeed. #2 fails and leaves the chained handlers and
the irqdomain around, but then unmaps the base and frees the data which
the handler and the domain code needs. Seriously?
> + } else {
Pointless else as the if clause terminates with a goto.
> + irq_set_chained_handler_and_data(irq, aspeed_intc_ic_irq_handler, intc_ic);
So if I understand the code correctly then the hardware coalesces the
pending bits into a single status register, but depending on which part
of the SoC raised the interrupt one of the demultiplex interrupts is
raised in the GIC.
Any of those demultiplex interrupt handles _all_ pending bits and
therefore you need gic_lock to serialize them, right?
Thanks,
tglx
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v1 2/2] irqchip/aspeed-intc: Add support for 10 INTC interrupts on AST27XX platforms
@ 2024-08-13 9:35 ` Thomas Gleixner
0 siblings, 0 replies; 22+ messages in thread
From: Thomas Gleixner @ 2024-08-13 9:35 UTC (permalink / raw)
To: Kevin Chen, robh, krzk+dt, conor+dt, joel, andrew, kevin_chen,
linux-kernel, devicetree, linux-arm-kernel, linux-aspeed
On Tue, Aug 13 2024 at 15:43, Kevin Chen wrote:
> There are 10 interrupt source of soc0_intc in CPU die INTC.
> 1. 6 interrupt sources in IO die of soc1_intc0~soc1_intc5.
> 2. 2 interrupt sources in LTPI of ltpi0_intc0 and ltpi0_intc1.
> 3. 2 interrupt sources in LTPI of ltpi1_intc0 and ltpi1_intc1.
> Request GIC interrupt to check each bit in status register to do next
> level INTC handler.
>
> In next level INTC handler of IO die or LTPI INTC using soc1_intcX combining
> 32 interrupt sources into soc0_intc11 in CPU die. In soc1_intcX, handler
> would check 32 bit of status register to do the requested device
> handler.
I can't figure out what this word salad is trying to tell me. Nothing in
the code does any combining. The handler reads the very same
INTC_INT_STATUS_REG.
>
This lacks a Signed-off-by: tag. See Documentation/process/
> ---
> drivers/irqchip/Makefile | 1 +
> drivers/irqchip/irq-aspeed-intc.c | 198 ++++++++++++++++++++++++++++++
> +
> +#define INTC_INT_ENABLE_REG 0x00
> +#define INTC_INT_STATUS_REG 0x04
> +
> +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, flags;
> +
> + chained_irq_enter(chip, desc);
> +
> + raw_spin_lock_irqsave(&intc_ic->gic_lock, flags);
There is no point for irqsave(). This code is invoked with interrupts
disabled and please convert to:
scoped_guard(raw_spinlock, &intc_ic->gic_lock) {
> + status = readl(intc_ic->base + INTC_INT_STATUS_REG);
> + for_each_set_bit(bit, &status, 32) {
Please use a define and not a hardcoded number.
> + generic_handle_domain_irq(intc_ic->irq_domain, bit);
> + writel(BIT(bit), intc_ic->base + INTC_INT_STATUS_REG);
> + }
}
> + raw_spin_unlock_irqrestore(&intc_ic->gic_lock, flags);
> +
> + 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);
> + unsigned long flags;
Invoked with interrupts disabled too.
> + raw_spin_lock_irqsave(&intc_ic->intc_lock, flags);
> + writel(mask, intc_ic->base + INTC_INT_ENABLE_REG);
> + raw_spin_unlock_irqrestore(&intc_ic->intc_lock, flags);
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);
> + unsigned long flags;
Ditto.
> + raw_spin_lock_irqsave(&intc_ic->intc_lock, flags);
> + writel(unmask, intc_ic->base + INTC_INT_ENABLE_REG);
> + raw_spin_unlock_irqrestore(&intc_ic->intc_lock, flags);
> +}
> +
> +static int aspeed_intc_irq_set_affinity(struct irq_data *data, const struct cpumask *dest,
> + bool force)
> +{
> + return -EINVAL;
> +}
No point for this stub, just leave irq_set_affinity uninitialized. The
core code checks that pointer for NULL. Aside of that this stub and the
assignment would need a #ifdef CONFIG_SMP guard.
> +static struct irq_chip aspeed_intc_chip = {
> + .name = "ASPEED INTC",
> + .irq_mask = aspeed_intc_irq_mask,
> + .irq_unmask = aspeed_intc_irq_unmask,
> + .irq_set_affinity = aspeed_intc_irq_set_affinity,
> +};
> +
> +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,
.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;
int irq, ret;
No point in initializing ret.
> + 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);
> +
> + irq = irq_of_parse_and_map(node, 0);
> + if (!irq) {
> + pr_err("Failed to get irq number\n");
> + ret = -EINVAL;
> + goto err_iounmap;
> + }
> +
> + 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";
See above.
> + 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;
> +}
> +
> +static int __init aspeed_intc_ic_of_init_v2(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";
So up to this point aspeed_intc_ic_of_init_v2() is a verbatim copy of
aspeed_intc_ic_of_init(). Why can't you reuse that function? It's not
rocket science to make that work.
> + 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;
Assume #0 and #1 succeed. #2 fails and leaves the chained handlers and
the irqdomain around, but then unmaps the base and frees the data which
the handler and the domain code needs. Seriously?
> + } else {
Pointless else as the if clause terminates with a goto.
> + irq_set_chained_handler_and_data(irq, aspeed_intc_ic_irq_handler, intc_ic);
So if I understand the code correctly then the hardware coalesces the
pending bits into a single status register, but depending on which part
of the SoC raised the interrupt one of the demultiplex interrupts is
raised in the GIC.
Any of those demultiplex interrupt handles _all_ pending bits and
therefore you need gic_lock to serialize them, right?
Thanks,
tglx
^ permalink raw reply [flat|nested] 22+ messages in thread* [PATCH v1 2/2] irqchip/aspeed-intc: Add support for 10 INTC interrupts on AST27XX platforms
2024-08-13 9:35 ` Thomas Gleixner
@ 2024-10-08 1:50 ` Kevin Chen
-1 siblings, 0 replies; 22+ messages in thread
From: Kevin Chen @ 2024-10-08 1:50 UTC (permalink / raw)
To: linux-aspeed
> > There are 10 interrupt sources of soc0_intc in CPU die INTC.
> > 1. 6 interrupt sources in IO die of soc1_intc0~soc1_intc5.
> > 2. 2 interrupt sources in LTPI of ltpi0_intc0 and ltpi0_intc1.
> > 3. 2 interrupt sources in LTPI of ltpi1_intc0 and ltpi1_intc1.
> > Request GIC interrupt to check each bit in status register to do next
> > level INTC handler.
> >
> > In next level INTC handler of IO die or LTPI INTC using soc1_intcX
> > combining
> > 32 interrupt sources into soc0_intc11 in CPU die. In soc1_intcX,
> > handler would check 32 bit of status register to do the requested
> > device handler.
>
> I can't figure out what this word salad is trying to tell me. Nothing in the code
> does any combining. The handler reads the very same INTC_INT_STATUS_REG.
According to AST2700 datasheet, there are two kinds of interrupt controller with enable and raw status registers for use.
1. INTC0 is used to assert GIC(#192~#197) if interrupt in INTC1 asserted. There are 6 GIC interrupt number(#192~#197) used in one INTC0.
2. INTC1 is used to assert INTC0 if interrupt of modules asserted. There are 32 module interrupts used in one INTC1.
+------+ +---------+ +-----------+ ---module0
| GIC | -----|INTC0 | ---+----| INTC1_0|---module2...
+------+ +---------+ | +-----------+---module31
|
| +-----------+---module0
+-----| INTC1_0|---module2...
| +-----------+---module31
...
| +-----------+---module0
+-----| INTC1_5|---module2...
| +-----------+---module31
>
> >
>
> This lacks a Signed-off-by: tag. See Documentation/process/
>
> > ---
> > drivers/irqchip/Makefile | 1 +
> > drivers/irqchip/irq-aspeed-intc.c | 198
> > ++++++++++++++++++++++++++++++
> > +
> > +#define INTC_INT_ENABLE_REG 0x00
> > +#define INTC_INT_STATUS_REG 0x04
> > +
> > +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, flags;
> > +
> > + chained_irq_enter(chip, desc);
> > +
> > + raw_spin_lock_irqsave(&intc_ic->gic_lock, flags);
>
> There is no point for irqsave(). This code is invoked with interrupts disabled and
> please convert to:
>
> scoped_guard(raw_spinlock, &intc_ic->gic_lock) {
Agree.
>
> > + status = readl(intc_ic->base + INTC_INT_STATUS_REG);
> > + for_each_set_bit(bit, &status, 32) {
>
> Please use a define and not a hardcoded number.
Agree.
>
> > + generic_handle_domain_irq(intc_ic->irq_domain, bit);
> > + writel(BIT(bit), intc_ic->base + INTC_INT_STATUS_REG);
> > + }
>
> }
>
> > + raw_spin_unlock_irqrestore(&intc_ic->gic_lock, flags);
> > +
> > + 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);
> > + unsigned long flags;
>
> Invoked with interrupts disabled too.
Agree.
>
> > + raw_spin_lock_irqsave(&intc_ic->intc_lock, flags);
> > + writel(mask, intc_ic->base + INTC_INT_ENABLE_REG);
> > + raw_spin_unlock_irqrestore(&intc_ic->intc_lock, flags);
>
> guard(raw_spinlock)(&intc_ic->intc_lock);
Agree.
> 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);
> > + unsigned long flags;
>
> Ditto.
Agree.
>
> > + raw_spin_lock_irqsave(&intc_ic->intc_lock, flags);
> > + writel(unmask, intc_ic->base + INTC_INT_ENABLE_REG);
> > + raw_spin_unlock_irqrestore(&intc_ic->intc_lock, flags); }
> > +
> > +static int aspeed_intc_irq_set_affinity(struct irq_data *data, const struct
> cpumask *dest,
> > + bool force)
> > +{
> > + return -EINVAL;
> > +}
>
> No point for this stub, just leave irq_set_affinity uninitialized. The core code
> checks that pointer for NULL. Aside of that this stub and the assignment would
> need a #ifdef CONFIG_SMP guard.
Agree.
>
> > +static struct irq_chip aspeed_intc_chip = {
> > + .name = "ASPEED INTC",
> > + .irq_mask = aspeed_intc_irq_mask,
> > + .irq_unmask = aspeed_intc_irq_unmask,
> > + .irq_set_affinity = aspeed_intc_irq_set_affinity,
> > +};
> > +
> > +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,
>
> .map = aspeed_intc_ic_map_irq_domain,
Agree.
>
> > +};
> > +
> > +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;
>
> int irq, ret;
Agree.
>
> No point in initializing ret.
Agree.
>
> > + 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);
> > +
> > + irq = irq_of_parse_and_map(node, 0);
> > + if (!irq) {
> > + pr_err("Failed to get irq number\n");
> > + ret = -EINVAL;
> > + goto err_iounmap;
> > + }
> > +
> > + 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";
>
> See above.
Do you mean the name of "ASPEED INTC" would be covered by "aspeed-intc-doman"?
>
> > + 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;
> > +}
> > +
> > +static int __init aspeed_intc_ic_of_init_v2(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";
>
> So up to this point aspeed_intc_ic_of_init_v2() is a verbatim copy of
> aspeed_intc_ic_of_init(). Why can't you reuse that function? It's not rocket
> science to make that work.
Agree.
>
> > + 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;
>
> Assume #0 and #1 succeed. #2 fails and leaves the chained handlers and the
> irqdomain around, but then unmaps the base and frees the data which the
> handler and the domain code needs. Seriously?
So, do you recommend moving check irq out of for loop?
And, irq_set_chained_hanlder_and_data in another for loop?
>
> > + } else {
>
> Pointless else as the if clause terminates with a goto.
Agree. I will remove the else
>
> > + irq_set_chained_handler_and_data(irq,
> aspeed_intc_ic_irq_handler,
> > +intc_ic);
>
> So if I understand the code correctly then the hardware coalesces the pending
> bits into a single status register, but depending on which part of the SoC raised
> the interrupt one of the demultiplex interrupts is raised in the GIC.
Yes.
>
> Any of those demultiplex interrupt handles _all_ pending bits and therefore
> you need gic_lock to serialize them, right?
Yes.
>
> Thanks,
>
> tglx
Thanks a lot for your review.
^ permalink raw reply [flat|nested] 22+ messages in thread* RE: [PATCH v1 2/2] irqchip/aspeed-intc: Add support for 10 INTC interrupts on AST27XX platforms
@ 2024-10-08 1:50 ` Kevin Chen
0 siblings, 0 replies; 22+ messages in thread
From: Kevin Chen @ 2024-10-08 1:50 UTC (permalink / raw)
To: Thomas Gleixner, robh@kernel.org, krzk+dt@kernel.org,
conor+dt@kernel.org, joel@jms.id.au, andrew@codeconstruct.com.au,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-aspeed@lists.ozlabs.org, BMC-SW
> > There are 10 interrupt sources of soc0_intc in CPU die INTC.
> > 1. 6 interrupt sources in IO die of soc1_intc0~soc1_intc5.
> > 2. 2 interrupt sources in LTPI of ltpi0_intc0 and ltpi0_intc1.
> > 3. 2 interrupt sources in LTPI of ltpi1_intc0 and ltpi1_intc1.
> > Request GIC interrupt to check each bit in status register to do next
> > level INTC handler.
> >
> > In next level INTC handler of IO die or LTPI INTC using soc1_intcX
> > combining
> > 32 interrupt sources into soc0_intc11 in CPU die. In soc1_intcX,
> > handler would check 32 bit of status register to do the requested
> > device handler.
>
> I can't figure out what this word salad is trying to tell me. Nothing in the code
> does any combining. The handler reads the very same INTC_INT_STATUS_REG.
According to AST2700 datasheet, there are two kinds of interrupt controller with enable and raw status registers for use.
1. INTC0 is used to assert GIC(#192~#197) if interrupt in INTC1 asserted. There are 6 GIC interrupt number(#192~#197) used in one INTC0.
2. INTC1 is used to assert INTC0 if interrupt of modules asserted. There are 32 module interrupts used in one INTC1.
+------+ +---------+ +-----------+ ---module0
| GIC | -----|INTC0 | ---+----| INTC1_0|---module2...
+------+ +---------+ | +-----------+---module31
|
| +-----------+---module0
+-----| INTC1_0|---module2...
| +-----------+---module31
...
| +-----------+---module0
+-----| INTC1_5|---module2...
| +-----------+---module31
>
> >
>
> This lacks a Signed-off-by: tag. See Documentation/process/
>
> > ---
> > drivers/irqchip/Makefile | 1 +
> > drivers/irqchip/irq-aspeed-intc.c | 198
> > ++++++++++++++++++++++++++++++
> > +
> > +#define INTC_INT_ENABLE_REG 0x00
> > +#define INTC_INT_STATUS_REG 0x04
> > +
> > +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, flags;
> > +
> > + chained_irq_enter(chip, desc);
> > +
> > + raw_spin_lock_irqsave(&intc_ic->gic_lock, flags);
>
> There is no point for irqsave(). This code is invoked with interrupts disabled and
> please convert to:
>
> scoped_guard(raw_spinlock, &intc_ic->gic_lock) {
Agree.
>
> > + status = readl(intc_ic->base + INTC_INT_STATUS_REG);
> > + for_each_set_bit(bit, &status, 32) {
>
> Please use a define and not a hardcoded number.
Agree.
>
> > + generic_handle_domain_irq(intc_ic->irq_domain, bit);
> > + writel(BIT(bit), intc_ic->base + INTC_INT_STATUS_REG);
> > + }
>
> }
>
> > + raw_spin_unlock_irqrestore(&intc_ic->gic_lock, flags);
> > +
> > + 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);
> > + unsigned long flags;
>
> Invoked with interrupts disabled too.
Agree.
>
> > + raw_spin_lock_irqsave(&intc_ic->intc_lock, flags);
> > + writel(mask, intc_ic->base + INTC_INT_ENABLE_REG);
> > + raw_spin_unlock_irqrestore(&intc_ic->intc_lock, flags);
>
> guard(raw_spinlock)(&intc_ic->intc_lock);
Agree.
> 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);
> > + unsigned long flags;
>
> Ditto.
Agree.
>
> > + raw_spin_lock_irqsave(&intc_ic->intc_lock, flags);
> > + writel(unmask, intc_ic->base + INTC_INT_ENABLE_REG);
> > + raw_spin_unlock_irqrestore(&intc_ic->intc_lock, flags); }
> > +
> > +static int aspeed_intc_irq_set_affinity(struct irq_data *data, const struct
> cpumask *dest,
> > + bool force)
> > +{
> > + return -EINVAL;
> > +}
>
> No point for this stub, just leave irq_set_affinity uninitialized. The core code
> checks that pointer for NULL. Aside of that this stub and the assignment would
> need a #ifdef CONFIG_SMP guard.
Agree.
>
> > +static struct irq_chip aspeed_intc_chip = {
> > + .name = "ASPEED INTC",
> > + .irq_mask = aspeed_intc_irq_mask,
> > + .irq_unmask = aspeed_intc_irq_unmask,
> > + .irq_set_affinity = aspeed_intc_irq_set_affinity,
> > +};
> > +
> > +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,
>
> .map = aspeed_intc_ic_map_irq_domain,
Agree.
>
> > +};
> > +
> > +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;
>
> int irq, ret;
Agree.
>
> No point in initializing ret.
Agree.
>
> > + 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);
> > +
> > + irq = irq_of_parse_and_map(node, 0);
> > + if (!irq) {
> > + pr_err("Failed to get irq number\n");
> > + ret = -EINVAL;
> > + goto err_iounmap;
> > + }
> > +
> > + 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";
>
> See above.
Do you mean the name of "ASPEED INTC" would be covered by "aspeed-intc-doman"?
>
> > + 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;
> > +}
> > +
> > +static int __init aspeed_intc_ic_of_init_v2(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";
>
> So up to this point aspeed_intc_ic_of_init_v2() is a verbatim copy of
> aspeed_intc_ic_of_init(). Why can't you reuse that function? It's not rocket
> science to make that work.
Agree.
>
> > + 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;
>
> Assume #0 and #1 succeed. #2 fails and leaves the chained handlers and the
> irqdomain around, but then unmaps the base and frees the data which the
> handler and the domain code needs. Seriously?
So, do you recommend moving check irq out of for loop?
And, irq_set_chained_hanlder_and_data in another for loop?
>
> > + } else {
>
> Pointless else as the if clause terminates with a goto.
Agree. I will remove the else
>
> > + irq_set_chained_handler_and_data(irq,
> aspeed_intc_ic_irq_handler,
> > +intc_ic);
>
> So if I understand the code correctly then the hardware coalesces the pending
> bits into a single status register, but depending on which part of the SoC raised
> the interrupt one of the demultiplex interrupts is raised in the GIC.
Yes.
>
> Any of those demultiplex interrupt handles _all_ pending bits and therefore
> you need gic_lock to serialize them, right?
Yes.
>
> Thanks,
>
> tglx
Thanks a lot for your review.
^ permalink raw reply [flat|nested] 22+ messages in thread