On 2021-01-07 02:59, ChiaWei Wang wrote:

Hi Marc,

quoted

-----Original Message-----
From: Marc Zyngier <maz@kernel.org>
Sent: Wednesday, January 6, 2021 6:59 PM
To: ChiaWei Wang <redacted>
Subject: Re: [PATCH 4/6] irqchip/aspeed: Add Aspeed eSPI interrupt 
controller

On 2021-01-06 05:59, Chia-Wei, Wang wrote:

quoted

The eSPI interrupt controller acts as a SW IRQ number decoder to
correctly control/dispatch interrupts of the eSPI peripheral, virtual
wire, out-of-band, and flash channels.

Signed-off-by: Chia-Wei, Wang <redacted>
---
 drivers/irqchip/Makefile             |   2 +-
 drivers/irqchip/irq-aspeed-espi-ic.c | 251 ++++++++++++++++++++++++
 include/soc/aspeed/espi.h            | 279

+++++++++++++++++++++++++++

quoted

 3 files changed, 531 insertions(+), 1 deletion(-)  create mode 100644
drivers/irqchip/irq-aspeed-espi-ic.c
 create mode 100644 include/soc/aspeed/espi.h

diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile index

0ac93bfaec61..56da4a3123f8 100644

--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile

@@ -86,7 +86,7 @@ obj-$(CONFIG_MVEBU_PIC)			+=

irq-mvebu-pic.o

quoted

 obj-$(CONFIG_MVEBU_SEI)			+= irq-mvebu-sei.o
 obj-$(CONFIG_LS_EXTIRQ)			+= irq-ls-extirq.o
 obj-$(CONFIG_LS_SCFG_MSI)		+= irq-ls-scfg-msi.o
-obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o irq-aspeed-i2c-ic.o
irq-aspeed-scu-ic.o
+obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o irq-aspeed-i2c-ic.o
irq-aspeed-scu-ic.o irq-aspeed-espi-ic.o
 obj-$(CONFIG_STM32_EXTI) 		+= irq-stm32-exti.o
 obj-$(CONFIG_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
 obj-$(CONFIG_IRQ_UNIPHIER_AIDET)	+= irq-uniphier-aidet.o

diff --git a/drivers/irqchip/irq-aspeed-espi-ic.c

b/drivers/irqchip/irq-aspeed-espi-ic.c
new file mode 100644
index 000000000000..8a5cc8fe3f0c

--- /dev/null
+++ b/drivers/irqchip/irq-aspeed-espi-ic.c

@@ -0,0 +1,251 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2020 Aspeed Technology Inc.
+ */
+#include <linux/bitops.h>
+#include <linux/module.h>
+#include <linux/irq.h>
+#include <linux/irqchip.h>
+#include <linux/irqchip/chained_irq.h> #include <linux/irqdomain.h>
+#include <linux/interrupt.h> #include <linux/mfd/syscon.h> #include
+<linux/regmap.h> #include <linux/of.h> #include <linux/of_platform.h>
+
+#include <soc/aspeed/espi.h>
+#include <dt-bindings/interrupt-controller/aspeed-espi-ic.h>
+
+#define DEVICE_NAME	"aspeed-espi-ic"
+#define IRQCHIP_NAME	"eSPI-IC"
+
+#define ESPI_IC_IRQ_NUM	7
+
+struct aspeed_espi_ic {
+	struct regmap *map;
+	int irq;
+	int gpio_irq;
+	struct irq_domain *irq_domain;
+};
+
+static void aspeed_espi_ic_gpio_isr(struct irq_desc *desc) {
+	unsigned int irq;
+	struct aspeed_espi_ic *espi_ic = irq_desc_get_handler_data(desc);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+
+	chained_irq_enter(chip, desc);
+
+	irq = irq_find_mapping(espi_ic->irq_domain,
+				   ASPEED_ESPI_IC_CTRL_RESET);
+	generic_handle_irq(irq);
+
+	irq = irq_find_mapping(espi_ic->irq_domain,
+				   ASPEED_ESPI_IC_CHAN_RESET);
+	generic_handle_irq(irq);

So for each mux interrupt, you generate two endpoints interrupt, 
without even
checking whether they are pending? That's no good.

As the eSPI IC driver is chained to Aspeed GPIO IC, the pending is
checked in the gpio-aspeed.c

That's not the place to do that.

quoted

quoted

+
+	chained_irq_exit(chip, desc);
+}
+
+static void aspeed_espi_ic_isr(struct irq_desc *desc) {
+	unsigned int sts;
+	unsigned int irq;
+	struct aspeed_espi_ic *espi_ic = irq_desc_get_handler_data(desc);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+
+	chained_irq_enter(chip, desc);
+
+	regmap_read(espi_ic->map, ESPI_INT_STS, &sts);
+
+	if (sts & ESPI_INT_STS_PERIF_BITS) {
+		irq = irq_find_mapping(espi_ic->irq_domain,
+				       ASPEED_ESPI_IC_PERIF_EVENT);
+		generic_handle_irq(irq);
+	}
+
+	if (sts & ESPI_INT_STS_VW_BITS) {
+		irq = irq_find_mapping(espi_ic->irq_domain,
+				       ASPEED_ESPI_IC_VW_EVENT);
+		generic_handle_irq(irq);
+	}
+
+	if (sts & ESPI_INT_STS_OOB_BITS) {
+		irq = irq_find_mapping(espi_ic->irq_domain,
+				       ASPEED_ESPI_IC_OOB_EVENT);
+		generic_handle_irq(irq);
+	}
+
+	if (sts & ESPI_INT_STS_FLASH_BITS) {
+		irq = irq_find_mapping(espi_ic->irq_domain,
+				       ASPEED_ESPI_IC_FLASH_EVENT);
+		generic_handle_irq(irq);
+	}
+
+	if (sts & ESPI_INT_STS_HW_RST_DEASSERT) {
+		irq = irq_find_mapping(espi_ic->irq_domain,
+				       ASPEED_ESPI_IC_CTRL_EVENT);
+		generic_handle_irq(irq);
+	}

This is horrible. Why can't you just use fls() in a loop?

The bits in the interrupt status register for a eSPI channel are not
sequentially arranged.
Using fls() may invoke an eSPI channel ISR multiple times.
So I collected the bitmap for each channel, respectively, and call the
ISR at once.

And that's equally wrong. You need to handle interrupts individually,
as they are different signal. If you are to implement an interrupt
controller, please do it properly.

Otherwise, get rid of it and move everything into your pet driver.
There is no need to do a half-baked job.

As it is, there is no way this code can be merged.

         M.
-- 
Jazz is not dead. It just smells funny...