[PATCH v1 0/2] Add support for AST2700 INTC driver

[PATCH v1 0/2] Add support for AST2700 INTC driver

[Kevin Chen]

Introduce to the AST27XX INTC modules, which contain two conponents in
CPU die(12nm) and IO die(40mm) comunicating by SLI or LTPI protocol.

There are lots of device in IO die, which need to be serviced in
requested interrupt handler. As two die ICs, combine 32 interrupt source
in IO die into 1 interrupt in CPU die.

soc0_intc11 represent CPU die INTC, which each bit mapping to soc1_intcX.
soc1_intcX represent IO die INTC, which combines 32 interrupt sources.

Kevin Chen (2):
  dt-bindings: interrupt-controller: Add support for ASPEED AST27XX INTC
  irqchip/aspeed-intc: Add support for 10 INTC interrupts on AST27XX
    platforms

 .../aspeed,ast2700-intc.yaml                  | 120 +++++++++++
 drivers/irqchip/Makefile                      |   1 +
 drivers/irqchip/irq-aspeed-intc.c             | 198 ++++++++++++++++++
 3 files changed, 319 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml
 create mode 100644 drivers/irqchip/irq-aspeed-intc.c

-- 
2.34.1

[PATCH v2 0/2] Add support for AST2700 INTC

[Kevin Chen]

Support for the Aspeed Interrupt Controller found on Aspeed 7th Geration Silicon
SoCs.

ASPEED interrupt controller(INTC) maps the internal interrupt sources of
the AST27XX devices to an parent interrupt controller.

Changes since v2:
Combine the aspeed_intc_ic_of_init and aspeed_intc_ic_of_init_v2.
Switch raw_spin_lock_irqsave to scoped_guard and guard.
Fix the error of make dt_binding_check.
Refine the aspeed,ast2700-intc.yaml.


Kevin Chen (2):
  dt-bindings: interrupt-controller: Add support for ASPEED AST27XX INTC
  irqchip/aspeed-intc: Add support for AST27XX INTC

 .../aspeed,ast2700-intc.yaml                  |  71 +++++++++
 drivers/irqchip/Makefile                      |   1 +
 drivers/irqchip/irq-aspeed-intc.c             | 137 ++++++++++++++++++
 3 files changed, 209 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml
 create mode 100644 drivers/irqchip/irq-aspeed-intc.c

-- 
2.34.1

Re: [PATCH v2 0/2] Add support for AST2700 INTC

[Krzysztof Kozlowski]

On 14/08/2024 13:41, Kevin Chen wrote:
> Support for the Aspeed Interrupt Controller found on Aspeed 7th Geration Silicon
> SoCs.
> 
> ASPEED interrupt controller(INTC) maps the internal interrupt sources of
> the AST27XX devices to an parent interrupt controller.
> 
> Changes since v2:
> Combine the aspeed_intc_ic_of_init and aspeed_intc_ic_of_init_v2.
> Switch raw_spin_lock_irqsave to scoped_guard and guard.
> Fix the error of make dt_binding_check.
> Refine the aspeed,ast2700-intc.yaml.

It seems that entire Aspeed has troubles working with people in the
community. You do not address feedback, you ignore it and asks us to
re-review the same crap.

Before we proceed further:
1. Answer, inline, without confidentiality notice (after asking you this
5 times, I think you should fix it finally) that you:
 - agree (ack/ok/agree)
 - disagree with explanation why

2. Then double check that your new version implements everything above.

3. Then send new version (max once per 24h) with changelog and
versioning (just use `b4`).

If you keep ignoring step 1 and 2, you will annoy reviewers up to the
point of automatic NAK or being ignored.

Best regards,
Krzysztof

[PATCH v2 1/2] dt-bindings: interrupt-controller: Add support for ASPEED AST27XX INTC

[Kevin Chen]

The ASPEED AST27XX interrupt controller(INTC) contain second level and
third level interrupt controller. The third level INTC combines 32 interrupt
sources into 1 interrupt into parent interrupt controller. The second
level INTC doing hand shake with third level INTC.
---
 .../aspeed,ast2700-intc.yaml                  | 71 +++++++++++++++++++
 1 file changed, 71 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml

diff --git a/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml b/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml
new file mode 100644
index 000000000000..9a76d5c3b66b
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml
@@ -0,0 +1,71 @@
+# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/interrupt-controller/aspeed,ast2700-intc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Aspeed AST2700 Interrupt Controller
+
+description:
+  This interrupt controller hardware is second level interrupt controller that
+  is hooked to a parent interrupt controller. It's useful to combine multiple
+  interrupt sources into 1 interrupt to parent interrupt controller.
+
+maintainers:
+  - Kevin Chen <kevin_chen@aspeedtech.com>
+
+properties:
+  compatible:
+    enum:
+      - aspeed,ast2700-intc-ic
+
+  reg:
+    minItems: 1
+
+  interrupt-controller: true
+
+  '#interrupt-cells':
+    const: 2
+
+  interrupts:
+    minItems: 1
+    maxItems: 10
+    description:
+      It contains two types of interrupt controller. The first type is multiple
+      interrupt sources into parent interrupt controller. The second type is 
+      1 interrupt source to parent interrupt controller.
+
+required:
+  - compatible
+  - reg
+  - interrupt-controller
+  - '#interrupt-cells'
+  - interrupts
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    bus {
+        #address-cells = <2>;
+        #size-cells = <2>;
+    
+        interrupt-controller@12101b00 {
+          compatible = "aspeed,ast2700-intc-ic";
+          reg = <0 0x12101b00 0 0x10>;
+          #interrupt-cells = <2>;
+          interrupt-controller;
+          interrupts = <GIC_SPI 192 IRQ_TYPE_LEVEL_HIGH>,
+                       <GIC_SPI 193 IRQ_TYPE_LEVEL_HIGH>,
+                       <GIC_SPI 194 IRQ_TYPE_LEVEL_HIGH>,
+                       <GIC_SPI 195 IRQ_TYPE_LEVEL_HIGH>,
+                       <GIC_SPI 196 IRQ_TYPE_LEVEL_HIGH>,
+                       <GIC_SPI 197 IRQ_TYPE_LEVEL_HIGH>,
+                       <GIC_SPI 198 IRQ_TYPE_LEVEL_HIGH>,
+                       <GIC_SPI 199 IRQ_TYPE_LEVEL_HIGH>,
+                       <GIC_SPI 200 IRQ_TYPE_LEVEL_HIGH>,
+                       <GIC_SPI 201 IRQ_TYPE_LEVEL_HIGH>;
+        };
+    };
-- 
2.34.1

Re: [PATCH v2 1/2] dt-bindings: interrupt-controller: Add support for ASPEED AST27XX INTC

[Krzysztof Kozlowski]

On 14/08/2024 13:41, Kevin Chen wrote:
> The ASPEED AST27XX interrupt controller(INTC) contain second level and
> third level interrupt controller. The third level INTC combines 32 interrupt
> sources into 1 interrupt into parent interrupt controller. The second
> level INTC doing hand shake with third level INTC.


> +maintainers:
> +  - Kevin Chen <kevin_chen@aspeedtech.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - aspeed,ast2700-intc-ic
> +
> +  reg:
> +    minItems: 1

That's unconstrained. Instead: maxItems: 1

> +
> +  interrupt-controller: true
> +
> +  '#interrupt-cells':
> +    const: 2
> +
> +  interrupts:
> +    minItems: 1
> +    maxItems: 10
> +    description:
> +      It contains two types of interrupt controller. The first type is multiple
> +      interrupt sources into parent interrupt controller. The second type is 
> +      1 interrupt source to parent interrupt controller.

I think I asked already - list the items with description.

Why the number is flexible?

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupt-controller
> +  - '#interrupt-cells'
> +  - interrupts
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    bus {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +    
> +        interrupt-controller@12101b00 {
> +          compatible = "aspeed,ast2700-intc-ic";

Messed indentation.



Best regards,
Krzysztof

RE: [PATCH v2 1/2] dt-bindings: interrupt-controller: Add support for ASPEED AST27XX INTC

[Kevin Chen]

> > The ASPEED AST27XX interrupt controller(INTC) contain second level and
> > third level interrupt controller. The third level INTC combines 32
> > interrupt sources into 1 interrupt into parent interrupt controller.
> > The second level INTC doing hand shake with third level INTC.
> 
> 
> > +maintainers:
> > +  - Kevin Chen <kevin_chen@aspeedtech.com>
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - aspeed,ast2700-intc-ic
> > +
> > +  reg:
> > +    minItems: 1
> 
> That's unconstrained. Instead: maxItems: 1
Agree.

> 
> > +
> > +  interrupt-controller: true
> > +
> > +  '#interrupt-cells':
> > +    const: 2
> > +
> > +  interrupts:
> > +    minItems: 1
> > +    maxItems: 10
> > +    description:
> > +      It contains two types of interrupt controller. The first type is multiple
> > +      interrupt sources into parent interrupt controller. The second type is
> > +      1 interrupt source to parent interrupt controller.
> 
> I think I asked already - list the items with description.
> 
> Why the number is flexible?
Depend to which INTC0 or INTC1 used.
INTC0 and INTC1 are two kinds of interrupt controller with enable and raw status registers for use.
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.
INTC1 is used to assert INTC0 if interrupt of modules asserted. There are 32 module interrupts used in one INTC1.

> 
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupt-controller
> > +  - '#interrupt-cells'
> > +  - interrupts
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +
> > +    bus {
> > +        #address-cells = <2>;
> > +        #size-cells = <2>;
> > +
> > +        interrupt-controller@12101b00 {
> > +          compatible = "aspeed,ast2700-intc-ic";
> 
> Messed indentation.
Agree. Would change to the following.
examples:
  - |
    #include <dt-bindings/interrupt-controller/arm-gic.h>

    bus {
        #address-cells = <2>;
        #size-cells = <2>;

        interrupt-controller@12101b00 {
            compatible = "aspeed,ast2700-intc-ic";
            reg = <0 0x12101b00 0 0x10>;
            #interrupt-cells = <2>;
            interrupt-controller;
            interrupts = <GIC_SPI 192 IRQ_TYPE_LEVEL_HIGH>,
                         <GIC_SPI 193 IRQ_TYPE_LEVEL_HIGH>,
                         <GIC_SPI 194 IRQ_TYPE_LEVEL_HIGH>,
                         <GIC_SPI 195 IRQ_TYPE_LEVEL_HIGH>,
                         <GIC_SPI 196 IRQ_TYPE_LEVEL_HIGH>,
                         <GIC_SPI 197 IRQ_TYPE_LEVEL_HIGH>,
                         <GIC_SPI 198 IRQ_TYPE_LEVEL_HIGH>,
                         <GIC_SPI 199 IRQ_TYPE_LEVEL_HIGH>,
                         <GIC_SPI 200 IRQ_TYPE_LEVEL_HIGH>,
                         <GIC_SPI 201 IRQ_TYPE_LEVEL_HIGH>;
        };
    };

> 
> Best regards,
> Krzysztof

Re: [PATCH v2 1/2] dt-bindings: interrupt-controller: Add support for ASPEED AST27XX INTC

[Rob Herring]

On Wed, Aug 14, 2024 at 07:41:05PM +0800, Kevin Chen wrote:
> The ASPEED AST27XX interrupt controller(INTC) contain second level and
> third level interrupt controller. The third level INTC combines 32 interrupt
> sources into 1 interrupt into parent interrupt controller. The second
> level INTC doing hand shake with third level INTC.

Missing Signed-off-by. checkpatch.pl also reports trailing whitespace.

[PATCH v2 2/2] irqchip/aspeed-intc: Add support for AST27XX INTC

[Kevin Chen]

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
 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);
-- 
2.34.1

Re: [PATCH v2 2/2] irqchip/aspeed-intc: Add support for AST27XX INTC

[Krzysztof Kozlowski]

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

RE: [PATCH v2 2/2] irqchip/aspeed-intc: Add support for AST27XX INTC

[Kevin Chen]

> > 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

Re: [PATCH v2 2/2] irqchip/aspeed-intc: Add support for AST27XX INTC

[Thomas Gleixner]

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

Re: [PATCH v2 2/2] irqchip/aspeed-intc: Add support for AST27XX INTC

[Krzysztof Kozlowski]

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

[PATCH v1 0/2] Add support for AST2700 INTC driver

[Kevin Chen]

Introduce to the AST27XX INTC modules, which contain two conponents in
CPU die(12nm) and IO die(40mm) comunicating by SLI or LTPI protocol.

There are lots of device in IO die, which need to be serviced in
requested interrupt handler. As two die ICs, combine 32 interrupt source
in IO die into 1 interrupt in CPU die.

soc0_intc11 represent CPU die INTC, which each bit mapping to soc1_intcX.
soc1_intcX represent IO die INTC, which combines 32 interrupt sources.

Kevin Chen (2):
  dt-bindings: interrupt-controller: Add support for ASPEED AST27XX INTC
  irqchip/aspeed-intc: Add support for 10 INTC interrupts on AST27XX
    platforms

 .../aspeed,ast2700-intc.yaml                  | 120 +++++++++++
 drivers/irqchip/Makefile                      |   1 +
 drivers/irqchip/irq-aspeed-intc.c             | 198 ++++++++++++++++++
 3 files changed, 319 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2700-intc.yaml
 create mode 100644 drivers/irqchip/irq-aspeed-intc.c

-- 
2.34.1