Re: [PATCH 1/2] irqchip: irq-mt58xx: Add mt58xx interrupt controller support

On 2020-08-03 16:03, Mark-PK Tsai wrote:

From: Marc Zyngier <maz@kernel.org>

quoted

On 2020-08-03 07:22, Mark-PK Tsai wrote:

quoted

Add mt58xx interrupt controller support using hierarchy irq
domain.

Signed-off-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
---
 drivers/irqchip/Kconfig      |   7 ++
 drivers/irqchip/Makefile     |   1 +
 drivers/irqchip/irq-mt58xx.c | 196 +++++++++++++++++++++++++++++++++++
 3 files changed, 204 insertions(+)
 create mode 100644 drivers/irqchip/irq-mt58xx.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 216b3b8392b5..00453af78be0 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig

@@ -572,4 +572,11 @@ config LOONGSON_PCH_MSI
 	help
 	  Support for the Loongson PCH MSI Controller.

+config MT58XX_IRQ
+	bool "MT58XX IRQ"
+	select IRQ_DOMAIN
+	select IRQ_DOMAIN_HIERARCHY
+	help
+	  Support Mediatek MT58XX Interrupt Controller.
+
 endmenu

diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 133f9c45744a..5062e9bfa92d 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile

@@ -111,3 +111,4 @@ obj-$(CONFIG_LOONGSON_HTPIC)		+=

irq-loongson-htpic.o
 obj-$(CONFIG_LOONGSON_HTVEC)		+= irq-loongson-htvec.o
 obj-$(CONFIG_LOONGSON_PCH_PIC)		+= irq-loongson-pch-pic.o
 obj-$(CONFIG_LOONGSON_PCH_MSI)		+= irq-loongson-pch-msi.o
+obj-$(CONFIG_MT58XX_IRQ)		+= irq-mt58xx.o

diff --git a/drivers/irqchip/irq-mt58xx.c

b/drivers/irqchip/irq-mt58xx.c
new file mode 100644
index 000000000000..e45ad023afa6

--- /dev/null
+++ b/drivers/irqchip/irq-mt58xx.c

@@ -0,0 +1,196 @@
+// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
+/*
+ * Copyright (c) 2020 MediaTek Inc.
+ * Author Mark-PK Tsai <mark-pk.tsai@mediatek.com>
+ */
+
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/irqchip.h>
+#include <linux/irqdomain.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/slab.h>
+
+#define INTC_MASK	0x0
+#define INTC_EOI	0x20
+
+struct mtk_intc_chip_data {
+	char *name;
+	struct irq_chip chip;

There is no need to embed a full struct irqchip per controller, see
below.

We want to distinguish which controller the device interrupts are 
belong to
by "cat /proc/interrupts".
And if all the controller share the same struct, the name field will be 
the
same.
Do you have suggestion for this?

Yes. /proc/interrupts is not a debug tool, and doesn't need to
show the interrupt routing. We have a debugfs option for this
purpose, and that is what you should use. If it is missing
any information, just say so and I will see what we can do.

[...]

quoted

quoted

+static struct irq_chip mtk_intc_chip = {
+	.irq_mask		= mtk_intc_mask_irq,
+	.irq_unmask		= mtk_intc_unmask_irq,
+	.irq_eoi		= mtk_intc_eoi_irq,
+	.irq_get_irqchip_state	= irq_chip_get_parent_state,
+	.irq_set_irqchip_state	= irq_chip_set_parent_state,
+	.irq_set_affinity	= irq_chip_set_affinity_parent,
+	.irq_set_vcpu_affinity	= irq_chip_set_vcpu_affinity_parent,

How about retrigger?

What is retrigger means?

It allows the HW to regenerate an interrupt. Set irq_retrigger
to irq_chip_retrigger_hierarchy, and you'll be fine. It is
going to be implemented shortly in the GIC driver.

To be honest, I just try to direct all the irqchip ops implemented in
/drivers/irqchip/irq-gic.c to gic driver.
But "irq_set_vcpu_affinity" is not used in our projects now.

I assume you are not contributing this code just for your
own project...

Should I remove ".irq_set_vcpu_affinity" here?

No, just leave it. Who knows, just in case virtualization becomes
a thing...

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