[PATCH 0/2] irqchip: irq-mt58xx: Add mt58xx series interrupt

[PATCH 0/2] irqchip: irq-mt58xx: Add mt58xx series interrupt

[Mark-PK Tsai]

Mediatek DTV SoCs contain multiple legacy interrupt controllers that routes
interrupts to the GIC. And all the mt58xx series SoCs have this controller,
hence the name of the driver and binding.

Mark-PK Tsai (2):
  irqchip: irq-mt58xx: Add mt58xx interrupt controller support
  dt-bindings: interrupt-controller: Add MT58XX interrupt controller

 .../mediatek,mt58xx-intc.yaml                 |  70 +++++++
 drivers/irqchip/Kconfig                       |   7 +
 drivers/irqchip/Makefile                      |   1 +
 drivers/irqchip/irq-mt58xx.c                  | 196 ++++++++++++++++++
 4 files changed, 274 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.yaml
 create mode 100644 drivers/irqchip/irq-mt58xx.c

-- 
2.18.0

[PATCH 0/2] irqchip: irq-mt58xx: Add mt58xx series interrupt

[Mark-PK Tsai]

Mediatek DTV SoCs contain multiple legacy interrupt controllers that routes
interrupts to the GIC. And all the mt58xx series SoCs have this controller,
hence the name of the driver and binding.

Mark-PK Tsai (2):
  irqchip: irq-mt58xx: Add mt58xx interrupt controller support
  dt-bindings: interrupt-controller: Add MT58XX interrupt controller

 .../mediatek,mt58xx-intc.yaml                 |  70 +++++++
 drivers/irqchip/Kconfig                       |   7 +
 drivers/irqchip/Makefile                      |   1 +
 drivers/irqchip/irq-mt58xx.c                  | 196 ++++++++++++++++++
 4 files changed, 274 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.yaml
 create mode 100644 drivers/irqchip/irq-mt58xx.c

-- 
2.18.0
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

[PATCH 0/2] irqchip: irq-mt58xx: Add mt58xx series interrupt

[Mark-PK Tsai]

Mediatek DTV SoCs contain multiple legacy interrupt controllers that routes
interrupts to the GIC. And all the mt58xx series SoCs have this controller,
hence the name of the driver and binding.

Mark-PK Tsai (2):
  irqchip: irq-mt58xx: Add mt58xx interrupt controller support
  dt-bindings: interrupt-controller: Add MT58XX interrupt controller

 .../mediatek,mt58xx-intc.yaml                 |  70 +++++++
 drivers/irqchip/Kconfig                       |   7 +
 drivers/irqchip/Makefile                      |   1 +
 drivers/irqchip/irq-mt58xx.c                  | 196 ++++++++++++++++++
 4 files changed, 274 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.yaml
 create mode 100644 drivers/irqchip/irq-mt58xx.c

-- 
2.18.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Mark-PK Tsai]

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;
+	unsigned int irq_start, nr_irqs;
+	void __iomem *base;
+};
+
+static void mtk_poke_irq(struct irq_data *d, u32 offset)
+{
+	struct mtk_intc_chip_data *cd = irq_data_get_irq_chip_data(d);
+	void __iomem *base = cd->base;
+	u8 index = (u8)irqd_to_hwirq(d);
+	u16 val, mask;
+
+	mask = 1 << (index % 16);
+	val = readw_relaxed(base + offset + (index / 16) * 4) | mask;
+	writew_relaxed(val, base + offset + (index / 16) * 4);
+}
+
+static void mtk_clear_irq(struct irq_data *d, u32 offset)
+{
+	struct mtk_intc_chip_data *cd = irq_data_get_irq_chip_data(d);
+	void __iomem *base = cd->base;
+	u8 index = (u8)irqd_to_hwirq(d);
+	u16 val, mask;
+
+	mask = 1 << (index % 16);
+	val = readw_relaxed(base + offset + (index / 16) * 4) & ~mask;
+	writew_relaxed(val, base + offset + (index / 16) * 4);
+}
+
+static void mtk_intc_mask_irq(struct irq_data *d)
+{
+	mtk_poke_irq(d, INTC_MASK);
+	irq_chip_mask_parent(d);
+}
+
+static void mtk_intc_unmask_irq(struct irq_data *d)
+{
+	mtk_clear_irq(d, INTC_MASK);
+	irq_chip_unmask_parent(d);
+}
+
+static void mtk_intc_eoi_irq(struct irq_data *d)
+{
+	mtk_poke_irq(d, INTC_EOI);
+	irq_chip_eoi_parent(d);
+}
+
+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,
+	.irq_set_type		= irq_chip_set_type_parent,
+	.flags			= IRQCHIP_SET_TYPE_MASKED |
+				  IRQCHIP_SKIP_SET_WAKE |
+				  IRQCHIP_MASK_ON_SUSPEND,
+};
+
+static int mt58xx_intc_domain_translate(struct irq_domain *d,
+					struct irq_fwspec *fwspec,
+					unsigned long *hwirq,
+					unsigned int *type)
+{
+	if (is_of_node(fwspec->fwnode)) {
+		if (fwspec->param_count != 3)
+			return -EINVAL;
+
+		/* No PPI should point to this domain */
+		if (fwspec->param[0] != 0)
+			return -EINVAL;
+
+		*hwirq = fwspec->param[1];
+		*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static int mt58xx_intc_domain_alloc(struct irq_domain *domain, unsigned int virq,
+				    unsigned int nr_irqs, void *data)
+{
+	int i;
+	irq_hw_number_t hwirq;
+	struct irq_fwspec parent_fwspec, *fwspec = data;
+	struct mtk_intc_chip_data *cd = (struct mtk_intc_chip_data *)domain->host_data;
+
+	/* Not GIC compliant */
+	if (fwspec->param_count != 3)
+		return -EINVAL;
+
+	/* No PPI should point to this domain */
+	if (fwspec->param[0])
+		return -EINVAL;
+
+	if (fwspec->param[1] >= cd->nr_irqs)
+		return -EINVAL;
+
+	hwirq = fwspec->param[1];
+	for (i = 0; i < nr_irqs; i++)
+		irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
+					      &cd->chip,
+					      domain->host_data);
+
+	parent_fwspec = *fwspec;
+	parent_fwspec.fwnode = domain->parent->fwnode;
+	parent_fwspec.param[1] = cd->irq_start + hwirq;
+	return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &parent_fwspec);
+}
+
+static const struct irq_domain_ops mt58xx_intc_domain_ops = {
+	.translate	= mt58xx_intc_domain_translate,
+	.alloc		= mt58xx_intc_domain_alloc,
+	.free		= irq_domain_free_irqs_common,
+};
+
+int __init
+mt58xx_intc_of_init(struct device_node *dn, struct device_node *parent)
+{
+	static int nr_intc;
+	struct irq_domain *domain, *domain_parent;
+	struct mtk_intc_chip_data *cd;
+	unsigned int irq_start, irq_end;
+
+	domain_parent = irq_find_host(parent);
+	if (!domain_parent) {
+		pr_err("mt58xx-intc: interrupt-parent not found\n");
+		return -EINVAL;
+	}
+
+	cd = kzalloc(sizeof(*cd), GFP_KERNEL);
+	if (!cd)
+		return -ENOMEM;
+
+	cd->chip = mtk_intc_chip;
+	if (of_property_read_u32_index(dn, "mediatek,irqs-map-range", 0, &irq_start) ||
+	    of_property_read_u32_index(dn, "mediatek,irqs-map-range", 1, &irq_end)) {
+		kfree(cd);
+		return -EINVAL;
+	}
+
+	if (of_property_read_bool(dn, "mediatek,intc-no-eoi"))
+		cd->chip.irq_eoi = irq_chip_eoi_parent;
+
+	cd->irq_start = irq_start;
+	cd->nr_irqs = irq_end - irq_start + 1;
+	cd->chip.name = kasprintf(GFP_KERNEL, "mt58xx-intc-%d", nr_intc++);
+	if (!cd->chip.name) {
+		kfree(cd);
+		return -ENOMEM;
+	}
+
+	cd->base = of_iomap(dn, 0);
+	if (!cd->base) {
+		kfree(cd->chip.name);
+		kfree(cd);
+		return -ENOMEM;
+	}
+
+	domain = irq_domain_add_hierarchy(domain_parent, 0, cd->nr_irqs,
+					  dn, &mt58xx_intc_domain_ops, cd);
+	if (!domain) {
+		kfree(cd->chip.name);
+		iounmap(cd->base);
+		kfree(cd);
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+IRQCHIP_DECLARE(mt58xx_intc, "mediatek,mt58xx-intc", mt58xx_intc_of_init);
-- 
2.18.0

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

[Mark-PK Tsai]

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;
+	unsigned int irq_start, nr_irqs;
+	void __iomem *base;
+};
+
+static void mtk_poke_irq(struct irq_data *d, u32 offset)
+{
+	struct mtk_intc_chip_data *cd = irq_data_get_irq_chip_data(d);
+	void __iomem *base = cd->base;
+	u8 index = (u8)irqd_to_hwirq(d);
+	u16 val, mask;
+
+	mask = 1 << (index % 16);
+	val = readw_relaxed(base + offset + (index / 16) * 4) | mask;
+	writew_relaxed(val, base + offset + (index / 16) * 4);
+}
+
+static void mtk_clear_irq(struct irq_data *d, u32 offset)
+{
+	struct mtk_intc_chip_data *cd = irq_data_get_irq_chip_data(d);
+	void __iomem *base = cd->base;
+	u8 index = (u8)irqd_to_hwirq(d);
+	u16 val, mask;
+
+	mask = 1 << (index % 16);
+	val = readw_relaxed(base + offset + (index / 16) * 4) & ~mask;
+	writew_relaxed(val, base + offset + (index / 16) * 4);
+}
+
+static void mtk_intc_mask_irq(struct irq_data *d)
+{
+	mtk_poke_irq(d, INTC_MASK);
+	irq_chip_mask_parent(d);
+}
+
+static void mtk_intc_unmask_irq(struct irq_data *d)
+{
+	mtk_clear_irq(d, INTC_MASK);
+	irq_chip_unmask_parent(d);
+}
+
+static void mtk_intc_eoi_irq(struct irq_data *d)
+{
+	mtk_poke_irq(d, INTC_EOI);
+	irq_chip_eoi_parent(d);
+}
+
+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,
+	.irq_set_type		= irq_chip_set_type_parent,
+	.flags			= IRQCHIP_SET_TYPE_MASKED |
+				  IRQCHIP_SKIP_SET_WAKE |
+				  IRQCHIP_MASK_ON_SUSPEND,
+};
+
+static int mt58xx_intc_domain_translate(struct irq_domain *d,
+					struct irq_fwspec *fwspec,
+					unsigned long *hwirq,
+					unsigned int *type)
+{
+	if (is_of_node(fwspec->fwnode)) {
+		if (fwspec->param_count != 3)
+			return -EINVAL;
+
+		/* No PPI should point to this domain */
+		if (fwspec->param[0] != 0)
+			return -EINVAL;
+
+		*hwirq = fwspec->param[1];
+		*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static int mt58xx_intc_domain_alloc(struct irq_domain *domain, unsigned int virq,
+				    unsigned int nr_irqs, void *data)
+{
+	int i;
+	irq_hw_number_t hwirq;
+	struct irq_fwspec parent_fwspec, *fwspec = data;
+	struct mtk_intc_chip_data *cd = (struct mtk_intc_chip_data *)domain->host_data;
+
+	/* Not GIC compliant */
+	if (fwspec->param_count != 3)
+		return -EINVAL;
+
+	/* No PPI should point to this domain */
+	if (fwspec->param[0])
+		return -EINVAL;
+
+	if (fwspec->param[1] >= cd->nr_irqs)
+		return -EINVAL;
+
+	hwirq = fwspec->param[1];
+	for (i = 0; i < nr_irqs; i++)
+		irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
+					      &cd->chip,
+					      domain->host_data);
+
+	parent_fwspec = *fwspec;
+	parent_fwspec.fwnode = domain->parent->fwnode;
+	parent_fwspec.param[1] = cd->irq_start + hwirq;
+	return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &parent_fwspec);
+}
+
+static const struct irq_domain_ops mt58xx_intc_domain_ops = {
+	.translate	= mt58xx_intc_domain_translate,
+	.alloc		= mt58xx_intc_domain_alloc,
+	.free		= irq_domain_free_irqs_common,
+};
+
+int __init
+mt58xx_intc_of_init(struct device_node *dn, struct device_node *parent)
+{
+	static int nr_intc;
+	struct irq_domain *domain, *domain_parent;
+	struct mtk_intc_chip_data *cd;
+	unsigned int irq_start, irq_end;
+
+	domain_parent = irq_find_host(parent);
+	if (!domain_parent) {
+		pr_err("mt58xx-intc: interrupt-parent not found\n");
+		return -EINVAL;
+	}
+
+	cd = kzalloc(sizeof(*cd), GFP_KERNEL);
+	if (!cd)
+		return -ENOMEM;
+
+	cd->chip = mtk_intc_chip;
+	if (of_property_read_u32_index(dn, "mediatek,irqs-map-range", 0, &irq_start) ||
+	    of_property_read_u32_index(dn, "mediatek,irqs-map-range", 1, &irq_end)) {
+		kfree(cd);
+		return -EINVAL;
+	}
+
+	if (of_property_read_bool(dn, "mediatek,intc-no-eoi"))
+		cd->chip.irq_eoi = irq_chip_eoi_parent;
+
+	cd->irq_start = irq_start;
+	cd->nr_irqs = irq_end - irq_start + 1;
+	cd->chip.name = kasprintf(GFP_KERNEL, "mt58xx-intc-%d", nr_intc++);
+	if (!cd->chip.name) {
+		kfree(cd);
+		return -ENOMEM;
+	}
+
+	cd->base = of_iomap(dn, 0);
+	if (!cd->base) {
+		kfree(cd->chip.name);
+		kfree(cd);
+		return -ENOMEM;
+	}
+
+	domain = irq_domain_add_hierarchy(domain_parent, 0, cd->nr_irqs,
+					  dn, &mt58xx_intc_domain_ops, cd);
+	if (!domain) {
+		kfree(cd->chip.name);
+		iounmap(cd->base);
+		kfree(cd);
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+IRQCHIP_DECLARE(mt58xx_intc, "mediatek,mt58xx-intc", mt58xx_intc_of_init);
-- 
2.18.0
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

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

[Mark-PK Tsai]

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;
+	unsigned int irq_start, nr_irqs;
+	void __iomem *base;
+};
+
+static void mtk_poke_irq(struct irq_data *d, u32 offset)
+{
+	struct mtk_intc_chip_data *cd = irq_data_get_irq_chip_data(d);
+	void __iomem *base = cd->base;
+	u8 index = (u8)irqd_to_hwirq(d);
+	u16 val, mask;
+
+	mask = 1 << (index % 16);
+	val = readw_relaxed(base + offset + (index / 16) * 4) | mask;
+	writew_relaxed(val, base + offset + (index / 16) * 4);
+}
+
+static void mtk_clear_irq(struct irq_data *d, u32 offset)
+{
+	struct mtk_intc_chip_data *cd = irq_data_get_irq_chip_data(d);
+	void __iomem *base = cd->base;
+	u8 index = (u8)irqd_to_hwirq(d);
+	u16 val, mask;
+
+	mask = 1 << (index % 16);
+	val = readw_relaxed(base + offset + (index / 16) * 4) & ~mask;
+	writew_relaxed(val, base + offset + (index / 16) * 4);
+}
+
+static void mtk_intc_mask_irq(struct irq_data *d)
+{
+	mtk_poke_irq(d, INTC_MASK);
+	irq_chip_mask_parent(d);
+}
+
+static void mtk_intc_unmask_irq(struct irq_data *d)
+{
+	mtk_clear_irq(d, INTC_MASK);
+	irq_chip_unmask_parent(d);
+}
+
+static void mtk_intc_eoi_irq(struct irq_data *d)
+{
+	mtk_poke_irq(d, INTC_EOI);
+	irq_chip_eoi_parent(d);
+}
+
+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,
+	.irq_set_type		= irq_chip_set_type_parent,
+	.flags			= IRQCHIP_SET_TYPE_MASKED |
+				  IRQCHIP_SKIP_SET_WAKE |
+				  IRQCHIP_MASK_ON_SUSPEND,
+};
+
+static int mt58xx_intc_domain_translate(struct irq_domain *d,
+					struct irq_fwspec *fwspec,
+					unsigned long *hwirq,
+					unsigned int *type)
+{
+	if (is_of_node(fwspec->fwnode)) {
+		if (fwspec->param_count != 3)
+			return -EINVAL;
+
+		/* No PPI should point to this domain */
+		if (fwspec->param[0] != 0)
+			return -EINVAL;
+
+		*hwirq = fwspec->param[1];
+		*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static int mt58xx_intc_domain_alloc(struct irq_domain *domain, unsigned int virq,
+				    unsigned int nr_irqs, void *data)
+{
+	int i;
+	irq_hw_number_t hwirq;
+	struct irq_fwspec parent_fwspec, *fwspec = data;
+	struct mtk_intc_chip_data *cd = (struct mtk_intc_chip_data *)domain->host_data;
+
+	/* Not GIC compliant */
+	if (fwspec->param_count != 3)
+		return -EINVAL;
+
+	/* No PPI should point to this domain */
+	if (fwspec->param[0])
+		return -EINVAL;
+
+	if (fwspec->param[1] >= cd->nr_irqs)
+		return -EINVAL;
+
+	hwirq = fwspec->param[1];
+	for (i = 0; i < nr_irqs; i++)
+		irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
+					      &cd->chip,
+					      domain->host_data);
+
+	parent_fwspec = *fwspec;
+	parent_fwspec.fwnode = domain->parent->fwnode;
+	parent_fwspec.param[1] = cd->irq_start + hwirq;
+	return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &parent_fwspec);
+}
+
+static const struct irq_domain_ops mt58xx_intc_domain_ops = {
+	.translate	= mt58xx_intc_domain_translate,
+	.alloc		= mt58xx_intc_domain_alloc,
+	.free		= irq_domain_free_irqs_common,
+};
+
+int __init
+mt58xx_intc_of_init(struct device_node *dn, struct device_node *parent)
+{
+	static int nr_intc;
+	struct irq_domain *domain, *domain_parent;
+	struct mtk_intc_chip_data *cd;
+	unsigned int irq_start, irq_end;
+
+	domain_parent = irq_find_host(parent);
+	if (!domain_parent) {
+		pr_err("mt58xx-intc: interrupt-parent not found\n");
+		return -EINVAL;
+	}
+
+	cd = kzalloc(sizeof(*cd), GFP_KERNEL);
+	if (!cd)
+		return -ENOMEM;
+
+	cd->chip = mtk_intc_chip;
+	if (of_property_read_u32_index(dn, "mediatek,irqs-map-range", 0, &irq_start) ||
+	    of_property_read_u32_index(dn, "mediatek,irqs-map-range", 1, &irq_end)) {
+		kfree(cd);
+		return -EINVAL;
+	}
+
+	if (of_property_read_bool(dn, "mediatek,intc-no-eoi"))
+		cd->chip.irq_eoi = irq_chip_eoi_parent;
+
+	cd->irq_start = irq_start;
+	cd->nr_irqs = irq_end - irq_start + 1;
+	cd->chip.name = kasprintf(GFP_KERNEL, "mt58xx-intc-%d", nr_intc++);
+	if (!cd->chip.name) {
+		kfree(cd);
+		return -ENOMEM;
+	}
+
+	cd->base = of_iomap(dn, 0);
+	if (!cd->base) {
+		kfree(cd->chip.name);
+		kfree(cd);
+		return -ENOMEM;
+	}
+
+	domain = irq_domain_add_hierarchy(domain_parent, 0, cd->nr_irqs,
+					  dn, &mt58xx_intc_domain_ops, cd);
+	if (!domain) {
+		kfree(cd->chip.name);
+		iounmap(cd->base);
+		kfree(cd);
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+IRQCHIP_DECLARE(mt58xx_intc, "mediatek,mt58xx-intc", mt58xx_intc_of_init);
-- 
2.18.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/2] dt-bindings: interrupt-controller: Add MT58XX interrupt controller

[Mark-PK Tsai]

Add binding for MT58XX interrupt controller.

Signed-off-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
---
 .../mediatek,mt58xx-intc.yaml                 | 70 +++++++++++++++++++
 1 file changed, 70 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.yaml

diff --git a/Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.yaml b/Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.yaml
new file mode 100644
index 000000000000..23e04763ab86
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.yaml
@@ -0,0 +1,70 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/interrupt-controller/arm,gic.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MediaTek MT58XX Interrupt Controller
+
+maintainers:
+  - Mark-PK Tsai <mark-pk.tsai@mediatek.com>
+
+description: |+
+  Mediatek DTV SoCs contain multiple legacy interrupt controllers that
+  routes interrupts to the GIC. All the mt58xx SoCs have this
+  controller, hence the name of binding.
+
+  The HW block exposes a number of interrupt controllers, each
+  can support up to 64 interrupts.
+
+allOf:
+  - $ref: /schemas/interrupt-controller.yaml#
+
+properties:
+  compatible:
+    - items:
+        - const: mediatek,mt58xx-intc
+
+  interrupt-controller: true
+
+  "#address-cells":
+    enum: [ 0, 1 ]
+  "#size-cells":
+    const: 1
+
+  "#interrupt-cells":
+    const: 3
+    description: |
+      Use the same format as specified by GIC in arm,gic.yaml.
+
+  reg:
+    description: |
+      Physical base address of the MT58xx interrupt controller
+      registers and length of memory mapped region.
+
+  mediatek,irqs-map-range:
+    description: |
+      The range of parent interrupt controller's interrupt lines
+      that are hardwired to MT58xx interrupt controller.
+
+  mediatek,intc-no-eoi:
+    description: |
+      Mark this controller has no End Of Interrupt(EOI) implementation.
+      This is a empty, boolean property.
+
+required:
+  - compatible
+  - reg
+  - mediatek,irqs-map-range
+
+examples:
+  - |
+    mt58xx_intc0: interrupt-controller@1f2032d0 {
+      compatible = "mediatek,mt58xx-intc";
+      interrupt-controller;
+      #interrupt-cells = <0x3>;
+      interrupt-parent = <&gic>;
+      reg = <0x0 0x1f2032d0 0x0 0x30>;
+      mediatek,irqs-map-range = <0 63>;
+    };
+...
-- 
2.18.0

[PATCH 2/2] dt-bindings: interrupt-controller: Add MT58XX interrupt controller

[Mark-PK Tsai]

Add binding for MT58XX interrupt controller.

Signed-off-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
---
 .../mediatek,mt58xx-intc.yaml                 | 70 +++++++++++++++++++
 1 file changed, 70 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.yaml

diff --git a/Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.yaml b/Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.yaml
new file mode 100644
index 000000000000..23e04763ab86
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.yaml
@@ -0,0 +1,70 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/interrupt-controller/arm,gic.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MediaTek MT58XX Interrupt Controller
+
+maintainers:
+  - Mark-PK Tsai <mark-pk.tsai@mediatek.com>
+
+description: |+
+  Mediatek DTV SoCs contain multiple legacy interrupt controllers that
+  routes interrupts to the GIC. All the mt58xx SoCs have this
+  controller, hence the name of binding.
+
+  The HW block exposes a number of interrupt controllers, each
+  can support up to 64 interrupts.
+
+allOf:
+  - $ref: /schemas/interrupt-controller.yaml#
+
+properties:
+  compatible:
+    - items:
+        - const: mediatek,mt58xx-intc
+
+  interrupt-controller: true
+
+  "#address-cells":
+    enum: [ 0, 1 ]
+  "#size-cells":
+    const: 1
+
+  "#interrupt-cells":
+    const: 3
+    description: |
+      Use the same format as specified by GIC in arm,gic.yaml.
+
+  reg:
+    description: |
+      Physical base address of the MT58xx interrupt controller
+      registers and length of memory mapped region.
+
+  mediatek,irqs-map-range:
+    description: |
+      The range of parent interrupt controller's interrupt lines
+      that are hardwired to MT58xx interrupt controller.
+
+  mediatek,intc-no-eoi:
+    description: |
+      Mark this controller has no End Of Interrupt(EOI) implementation.
+      This is a empty, boolean property.
+
+required:
+  - compatible
+  - reg
+  - mediatek,irqs-map-range
+
+examples:
+  - |
+    mt58xx_intc0: interrupt-controller@1f2032d0 {
+      compatible = "mediatek,mt58xx-intc";
+      interrupt-controller;
+      #interrupt-cells = <0x3>;
+      interrupt-parent = <&gic>;
+      reg = <0x0 0x1f2032d0 0x0 0x30>;
+      mediatek,irqs-map-range = <0 63>;
+    };
+...
-- 
2.18.0
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

[PATCH 2/2] dt-bindings: interrupt-controller: Add MT58XX interrupt controller

[Mark-PK Tsai]

Add binding for MT58XX interrupt controller.

Signed-off-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
---
 .../mediatek,mt58xx-intc.yaml                 | 70 +++++++++++++++++++
 1 file changed, 70 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.yaml

diff --git a/Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.yaml b/Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.yaml
new file mode 100644
index 000000000000..23e04763ab86
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.yaml
@@ -0,0 +1,70 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/interrupt-controller/arm,gic.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MediaTek MT58XX Interrupt Controller
+
+maintainers:
+  - Mark-PK Tsai <mark-pk.tsai@mediatek.com>
+
+description: |+
+  Mediatek DTV SoCs contain multiple legacy interrupt controllers that
+  routes interrupts to the GIC. All the mt58xx SoCs have this
+  controller, hence the name of binding.
+
+  The HW block exposes a number of interrupt controllers, each
+  can support up to 64 interrupts.
+
+allOf:
+  - $ref: /schemas/interrupt-controller.yaml#
+
+properties:
+  compatible:
+    - items:
+        - const: mediatek,mt58xx-intc
+
+  interrupt-controller: true
+
+  "#address-cells":
+    enum: [ 0, 1 ]
+  "#size-cells":
+    const: 1
+
+  "#interrupt-cells":
+    const: 3
+    description: |
+      Use the same format as specified by GIC in arm,gic.yaml.
+
+  reg:
+    description: |
+      Physical base address of the MT58xx interrupt controller
+      registers and length of memory mapped region.
+
+  mediatek,irqs-map-range:
+    description: |
+      The range of parent interrupt controller's interrupt lines
+      that are hardwired to MT58xx interrupt controller.
+
+  mediatek,intc-no-eoi:
+    description: |
+      Mark this controller has no End Of Interrupt(EOI) implementation.
+      This is a empty, boolean property.
+
+required:
+  - compatible
+  - reg
+  - mediatek,irqs-map-range
+
+examples:
+  - |
+    mt58xx_intc0: interrupt-controller@1f2032d0 {
+      compatible = "mediatek,mt58xx-intc";
+      interrupt-controller;
+      #interrupt-cells = <0x3>;
+      interrupt-parent = <&gic>;
+      reg = <0x0 0x1f2032d0 0x0 0x30>;
+      mediatek,irqs-map-range = <0 63>;
+    };
+...
-- 
2.18.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Marc Zyngier]

On 2020-08-03 07:22, Mark-PK Tsai wrote:
> 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.

> +	unsigned int irq_start, nr_irqs;
> +	void __iomem *base;
> +};
> +
> +static void mtk_poke_irq(struct irq_data *d, u32 offset)
> +{
> +	struct mtk_intc_chip_data *cd = irq_data_get_irq_chip_data(d);
> +	void __iomem *base = cd->base;
> +	u8 index = (u8)irqd_to_hwirq(d);

Why the restrictive type? Why isn't unsigned int good enough?

> +	u16 val, mask;
> +
> +	mask = 1 << (index % 16);
> +	val = readw_relaxed(base + offset + (index / 16) * 4) | mask;
> +	writew_relaxed(val, base + offset + (index / 16) * 4);

RMW without locking, you will end-up with corruption.
Please store the address calculation in a temporaty variable to make it
more readable

> +}
> +
> +static void mtk_clear_irq(struct irq_data *d, u32 offset)
> +{
> +	struct mtk_intc_chip_data *cd = irq_data_get_irq_chip_data(d);
> +	void __iomem *base = cd->base;
> +	u8 index = (u8)irqd_to_hwirq(d);
> +	u16 val, mask;
> +
> +	mask = 1 << (index % 16);
> +	val = readw_relaxed(base + offset + (index / 16) * 4) & ~mask;
> +	writew_relaxed(val, base + offset + (index / 16) * 4);

Same comments.

> +}
> +
> +static void mtk_intc_mask_irq(struct irq_data *d)
> +{
> +	mtk_poke_irq(d, INTC_MASK);
> +	irq_chip_mask_parent(d);
> +}
> +
> +static void mtk_intc_unmask_irq(struct irq_data *d)
> +{
> +	mtk_clear_irq(d, INTC_MASK);
> +	irq_chip_unmask_parent(d);
> +}
> +
> +static void mtk_intc_eoi_irq(struct irq_data *d)
> +{
> +	mtk_poke_irq(d, INTC_EOI);
> +	irq_chip_eoi_parent(d);
> +}
> +
> +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?

> +	.irq_set_type		= irq_chip_set_type_parent,
> +	.flags			= IRQCHIP_SET_TYPE_MASKED |
> +				  IRQCHIP_SKIP_SET_WAKE |
> +				  IRQCHIP_MASK_ON_SUSPEND,
> +};
> +
> +static int mt58xx_intc_domain_translate(struct irq_domain *d,
> +					struct irq_fwspec *fwspec,
> +					unsigned long *hwirq,
> +					unsigned int *type)
> +{
> +	if (is_of_node(fwspec->fwnode)) {
> +		if (fwspec->param_count != 3)
> +			return -EINVAL;
> +
> +		/* No PPI should point to this domain */
> +		if (fwspec->param[0] != 0)
> +			return -EINVAL;
> +
> +		*hwirq = fwspec->param[1];
> +		*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int mt58xx_intc_domain_alloc(struct irq_domain *domain,
> unsigned int virq,
> +				    unsigned int nr_irqs, void *data)
> +{
> +	int i;
> +	irq_hw_number_t hwirq;
> +	struct irq_fwspec parent_fwspec, *fwspec = data;
> +	struct mtk_intc_chip_data *cd = (struct mtk_intc_chip_data
> *)domain->host_data;
> +
> +	/* Not GIC compliant */
> +	if (fwspec->param_count != 3)
> +		return -EINVAL;
> +
> +	/* No PPI should point to this domain */
> +	if (fwspec->param[0])
> +		return -EINVAL;
> +
> +	if (fwspec->param[1] >= cd->nr_irqs)
> +		return -EINVAL;
> +
> +	hwirq = fwspec->param[1];
> +	for (i = 0; i < nr_irqs; i++)
> +		irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
> +					      &cd->chip,
> +					      domain->host_data);
> +
> +	parent_fwspec = *fwspec;
> +	parent_fwspec.fwnode = domain->parent->fwnode;
> +	parent_fwspec.param[1] = cd->irq_start + hwirq;
> +	return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, 
> &parent_fwspec);
> +}
> +
> +static const struct irq_domain_ops mt58xx_intc_domain_ops = {
> +	.translate	= mt58xx_intc_domain_translate,
> +	.alloc		= mt58xx_intc_domain_alloc,
> +	.free		= irq_domain_free_irqs_common,
> +};
> +
> +int __init
> +mt58xx_intc_of_init(struct device_node *dn, struct device_node 
> *parent)
> +{
> +	static int nr_intc;
> +	struct irq_domain *domain, *domain_parent;
> +	struct mtk_intc_chip_data *cd;
> +	unsigned int irq_start, irq_end;
> +
> +	domain_parent = irq_find_host(parent);
> +	if (!domain_parent) {
> +		pr_err("mt58xx-intc: interrupt-parent not found\n");
> +		return -EINVAL;
> +	}
> +
> +	cd = kzalloc(sizeof(*cd), GFP_KERNEL);
> +	if (!cd)
> +		return -ENOMEM;
> +
> +	cd->chip = mtk_intc_chip;
> +	if (of_property_read_u32_index(dn, "mediatek,irqs-map-range", 0,
> &irq_start) ||
> +	    of_property_read_u32_index(dn, "mediatek,irqs-map-range", 1, 
> &irq_end)) {
> +		kfree(cd);
> +		return -EINVAL;
> +	}
> +
> +	if (of_property_read_bool(dn, "mediatek,intc-no-eoi"))
> +		cd->chip.irq_eoi = irq_chip_eoi_parent;

No. Just add a flag to your chip data structure, and check for this
flag in your irq_eoi callback. Or provide two distinct irq_chip
structures that will only differ by the irq_eoi method.

> +
> +	cd->irq_start = irq_start;
> +	cd->nr_irqs = irq_end - irq_start + 1;
> +	cd->chip.name = kasprintf(GFP_KERNEL, "mt58xx-intc-%d", nr_intc++);

Neither. That's not useful, and is a waste of memory. Stick to constant
names in the irq_chip structure.

> +	if (!cd->chip.name) {
> +		kfree(cd);
> +		return -ENOMEM;
> +	}
> +
> +	cd->base = of_iomap(dn, 0);
> +	if (!cd->base) {
> +		kfree(cd->chip.name);
> +		kfree(cd);
> +		return -ENOMEM;
> +	}
> +
> +	domain = irq_domain_add_hierarchy(domain_parent, 0, cd->nr_irqs,
> +					  dn, &mt58xx_intc_domain_ops, cd);
> +	if (!domain) {
> +		kfree(cd->chip.name);
> +		iounmap(cd->base);
> +		kfree(cd);
> +		return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +
> +IRQCHIP_DECLARE(mt58xx_intc, "mediatek,mt58xx-intc", 
> mt58xx_intc_of_init);

On a side note, the merge window has just opened. Please refrain from
reposting this until -rc1.

Thanks,

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

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

[Marc Zyngier]

On 2020-08-03 07:22, Mark-PK Tsai wrote:
> 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.

> +	unsigned int irq_start, nr_irqs;
> +	void __iomem *base;
> +};
> +
> +static void mtk_poke_irq(struct irq_data *d, u32 offset)
> +{
> +	struct mtk_intc_chip_data *cd = irq_data_get_irq_chip_data(d);
> +	void __iomem *base = cd->base;
> +	u8 index = (u8)irqd_to_hwirq(d);

Why the restrictive type? Why isn't unsigned int good enough?

> +	u16 val, mask;
> +
> +	mask = 1 << (index % 16);
> +	val = readw_relaxed(base + offset + (index / 16) * 4) | mask;
> +	writew_relaxed(val, base + offset + (index / 16) * 4);

RMW without locking, you will end-up with corruption.
Please store the address calculation in a temporaty variable to make it
more readable

> +}
> +
> +static void mtk_clear_irq(struct irq_data *d, u32 offset)
> +{
> +	struct mtk_intc_chip_data *cd = irq_data_get_irq_chip_data(d);
> +	void __iomem *base = cd->base;
> +	u8 index = (u8)irqd_to_hwirq(d);
> +	u16 val, mask;
> +
> +	mask = 1 << (index % 16);
> +	val = readw_relaxed(base + offset + (index / 16) * 4) & ~mask;
> +	writew_relaxed(val, base + offset + (index / 16) * 4);

Same comments.

> +}
> +
> +static void mtk_intc_mask_irq(struct irq_data *d)
> +{
> +	mtk_poke_irq(d, INTC_MASK);
> +	irq_chip_mask_parent(d);
> +}
> +
> +static void mtk_intc_unmask_irq(struct irq_data *d)
> +{
> +	mtk_clear_irq(d, INTC_MASK);
> +	irq_chip_unmask_parent(d);
> +}
> +
> +static void mtk_intc_eoi_irq(struct irq_data *d)
> +{
> +	mtk_poke_irq(d, INTC_EOI);
> +	irq_chip_eoi_parent(d);
> +}
> +
> +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?

> +	.irq_set_type		= irq_chip_set_type_parent,
> +	.flags			= IRQCHIP_SET_TYPE_MASKED |
> +				  IRQCHIP_SKIP_SET_WAKE |
> +				  IRQCHIP_MASK_ON_SUSPEND,
> +};
> +
> +static int mt58xx_intc_domain_translate(struct irq_domain *d,
> +					struct irq_fwspec *fwspec,
> +					unsigned long *hwirq,
> +					unsigned int *type)
> +{
> +	if (is_of_node(fwspec->fwnode)) {
> +		if (fwspec->param_count != 3)
> +			return -EINVAL;
> +
> +		/* No PPI should point to this domain */
> +		if (fwspec->param[0] != 0)
> +			return -EINVAL;
> +
> +		*hwirq = fwspec->param[1];
> +		*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int mt58xx_intc_domain_alloc(struct irq_domain *domain,
> unsigned int virq,
> +				    unsigned int nr_irqs, void *data)
> +{
> +	int i;
> +	irq_hw_number_t hwirq;
> +	struct irq_fwspec parent_fwspec, *fwspec = data;
> +	struct mtk_intc_chip_data *cd = (struct mtk_intc_chip_data
> *)domain->host_data;
> +
> +	/* Not GIC compliant */
> +	if (fwspec->param_count != 3)
> +		return -EINVAL;
> +
> +	/* No PPI should point to this domain */
> +	if (fwspec->param[0])
> +		return -EINVAL;
> +
> +	if (fwspec->param[1] >= cd->nr_irqs)
> +		return -EINVAL;
> +
> +	hwirq = fwspec->param[1];
> +	for (i = 0; i < nr_irqs; i++)
> +		irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
> +					      &cd->chip,
> +					      domain->host_data);
> +
> +	parent_fwspec = *fwspec;
> +	parent_fwspec.fwnode = domain->parent->fwnode;
> +	parent_fwspec.param[1] = cd->irq_start + hwirq;
> +	return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, 
> &parent_fwspec);
> +}
> +
> +static const struct irq_domain_ops mt58xx_intc_domain_ops = {
> +	.translate	= mt58xx_intc_domain_translate,
> +	.alloc		= mt58xx_intc_domain_alloc,
> +	.free		= irq_domain_free_irqs_common,
> +};
> +
> +int __init
> +mt58xx_intc_of_init(struct device_node *dn, struct device_node 
> *parent)
> +{
> +	static int nr_intc;
> +	struct irq_domain *domain, *domain_parent;
> +	struct mtk_intc_chip_data *cd;
> +	unsigned int irq_start, irq_end;
> +
> +	domain_parent = irq_find_host(parent);
> +	if (!domain_parent) {
> +		pr_err("mt58xx-intc: interrupt-parent not found\n");
> +		return -EINVAL;
> +	}
> +
> +	cd = kzalloc(sizeof(*cd), GFP_KERNEL);
> +	if (!cd)
> +		return -ENOMEM;
> +
> +	cd->chip = mtk_intc_chip;
> +	if (of_property_read_u32_index(dn, "mediatek,irqs-map-range", 0,
> &irq_start) ||
> +	    of_property_read_u32_index(dn, "mediatek,irqs-map-range", 1, 
> &irq_end)) {
> +		kfree(cd);
> +		return -EINVAL;
> +	}
> +
> +	if (of_property_read_bool(dn, "mediatek,intc-no-eoi"))
> +		cd->chip.irq_eoi = irq_chip_eoi_parent;

No. Just add a flag to your chip data structure, and check for this
flag in your irq_eoi callback. Or provide two distinct irq_chip
structures that will only differ by the irq_eoi method.

> +
> +	cd->irq_start = irq_start;
> +	cd->nr_irqs = irq_end - irq_start + 1;
> +	cd->chip.name = kasprintf(GFP_KERNEL, "mt58xx-intc-%d", nr_intc++);

Neither. That's not useful, and is a waste of memory. Stick to constant
names in the irq_chip structure.

> +	if (!cd->chip.name) {
> +		kfree(cd);
> +		return -ENOMEM;
> +	}
> +
> +	cd->base = of_iomap(dn, 0);
> +	if (!cd->base) {
> +		kfree(cd->chip.name);
> +		kfree(cd);
> +		return -ENOMEM;
> +	}
> +
> +	domain = irq_domain_add_hierarchy(domain_parent, 0, cd->nr_irqs,
> +					  dn, &mt58xx_intc_domain_ops, cd);
> +	if (!domain) {
> +		kfree(cd->chip.name);
> +		iounmap(cd->base);
> +		kfree(cd);
> +		return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +
> +IRQCHIP_DECLARE(mt58xx_intc, "mediatek,mt58xx-intc", 
> mt58xx_intc_of_init);

On a side note, the merge window has just opened. Please refrain from
reposting this until -rc1.

Thanks,

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

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

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

[Marc Zyngier]

On 2020-08-03 07:22, Mark-PK Tsai wrote:
> 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.

> +	unsigned int irq_start, nr_irqs;
> +	void __iomem *base;
> +};
> +
> +static void mtk_poke_irq(struct irq_data *d, u32 offset)
> +{
> +	struct mtk_intc_chip_data *cd = irq_data_get_irq_chip_data(d);
> +	void __iomem *base = cd->base;
> +	u8 index = (u8)irqd_to_hwirq(d);

Why the restrictive type? Why isn't unsigned int good enough?

> +	u16 val, mask;
> +
> +	mask = 1 << (index % 16);
> +	val = readw_relaxed(base + offset + (index / 16) * 4) | mask;
> +	writew_relaxed(val, base + offset + (index / 16) * 4);

RMW without locking, you will end-up with corruption.
Please store the address calculation in a temporaty variable to make it
more readable

> +}
> +
> +static void mtk_clear_irq(struct irq_data *d, u32 offset)
> +{
> +	struct mtk_intc_chip_data *cd = irq_data_get_irq_chip_data(d);
> +	void __iomem *base = cd->base;
> +	u8 index = (u8)irqd_to_hwirq(d);
> +	u16 val, mask;
> +
> +	mask = 1 << (index % 16);
> +	val = readw_relaxed(base + offset + (index / 16) * 4) & ~mask;
> +	writew_relaxed(val, base + offset + (index / 16) * 4);

Same comments.

> +}
> +
> +static void mtk_intc_mask_irq(struct irq_data *d)
> +{
> +	mtk_poke_irq(d, INTC_MASK);
> +	irq_chip_mask_parent(d);
> +}
> +
> +static void mtk_intc_unmask_irq(struct irq_data *d)
> +{
> +	mtk_clear_irq(d, INTC_MASK);
> +	irq_chip_unmask_parent(d);
> +}
> +
> +static void mtk_intc_eoi_irq(struct irq_data *d)
> +{
> +	mtk_poke_irq(d, INTC_EOI);
> +	irq_chip_eoi_parent(d);
> +}
> +
> +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?

> +	.irq_set_type		= irq_chip_set_type_parent,
> +	.flags			= IRQCHIP_SET_TYPE_MASKED |
> +				  IRQCHIP_SKIP_SET_WAKE |
> +				  IRQCHIP_MASK_ON_SUSPEND,
> +};
> +
> +static int mt58xx_intc_domain_translate(struct irq_domain *d,
> +					struct irq_fwspec *fwspec,
> +					unsigned long *hwirq,
> +					unsigned int *type)
> +{
> +	if (is_of_node(fwspec->fwnode)) {
> +		if (fwspec->param_count != 3)
> +			return -EINVAL;
> +
> +		/* No PPI should point to this domain */
> +		if (fwspec->param[0] != 0)
> +			return -EINVAL;
> +
> +		*hwirq = fwspec->param[1];
> +		*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int mt58xx_intc_domain_alloc(struct irq_domain *domain,
> unsigned int virq,
> +				    unsigned int nr_irqs, void *data)
> +{
> +	int i;
> +	irq_hw_number_t hwirq;
> +	struct irq_fwspec parent_fwspec, *fwspec = data;
> +	struct mtk_intc_chip_data *cd = (struct mtk_intc_chip_data
> *)domain->host_data;
> +
> +	/* Not GIC compliant */
> +	if (fwspec->param_count != 3)
> +		return -EINVAL;
> +
> +	/* No PPI should point to this domain */
> +	if (fwspec->param[0])
> +		return -EINVAL;
> +
> +	if (fwspec->param[1] >= cd->nr_irqs)
> +		return -EINVAL;
> +
> +	hwirq = fwspec->param[1];
> +	for (i = 0; i < nr_irqs; i++)
> +		irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
> +					      &cd->chip,
> +					      domain->host_data);
> +
> +	parent_fwspec = *fwspec;
> +	parent_fwspec.fwnode = domain->parent->fwnode;
> +	parent_fwspec.param[1] = cd->irq_start + hwirq;
> +	return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, 
> &parent_fwspec);
> +}
> +
> +static const struct irq_domain_ops mt58xx_intc_domain_ops = {
> +	.translate	= mt58xx_intc_domain_translate,
> +	.alloc		= mt58xx_intc_domain_alloc,
> +	.free		= irq_domain_free_irqs_common,
> +};
> +
> +int __init
> +mt58xx_intc_of_init(struct device_node *dn, struct device_node 
> *parent)
> +{
> +	static int nr_intc;
> +	struct irq_domain *domain, *domain_parent;
> +	struct mtk_intc_chip_data *cd;
> +	unsigned int irq_start, irq_end;
> +
> +	domain_parent = irq_find_host(parent);
> +	if (!domain_parent) {
> +		pr_err("mt58xx-intc: interrupt-parent not found\n");
> +		return -EINVAL;
> +	}
> +
> +	cd = kzalloc(sizeof(*cd), GFP_KERNEL);
> +	if (!cd)
> +		return -ENOMEM;
> +
> +	cd->chip = mtk_intc_chip;
> +	if (of_property_read_u32_index(dn, "mediatek,irqs-map-range", 0,
> &irq_start) ||
> +	    of_property_read_u32_index(dn, "mediatek,irqs-map-range", 1, 
> &irq_end)) {
> +		kfree(cd);
> +		return -EINVAL;
> +	}
> +
> +	if (of_property_read_bool(dn, "mediatek,intc-no-eoi"))
> +		cd->chip.irq_eoi = irq_chip_eoi_parent;

No. Just add a flag to your chip data structure, and check for this
flag in your irq_eoi callback. Or provide two distinct irq_chip
structures that will only differ by the irq_eoi method.

> +
> +	cd->irq_start = irq_start;
> +	cd->nr_irqs = irq_end - irq_start + 1;
> +	cd->chip.name = kasprintf(GFP_KERNEL, "mt58xx-intc-%d", nr_intc++);

Neither. That's not useful, and is a waste of memory. Stick to constant
names in the irq_chip structure.

> +	if (!cd->chip.name) {
> +		kfree(cd);
> +		return -ENOMEM;
> +	}
> +
> +	cd->base = of_iomap(dn, 0);
> +	if (!cd->base) {
> +		kfree(cd->chip.name);
> +		kfree(cd);
> +		return -ENOMEM;
> +	}
> +
> +	domain = irq_domain_add_hierarchy(domain_parent, 0, cd->nr_irqs,
> +					  dn, &mt58xx_intc_domain_ops, cd);
> +	if (!domain) {
> +		kfree(cd->chip.name);
> +		iounmap(cd->base);
> +		kfree(cd);
> +		return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +
> +IRQCHIP_DECLARE(mt58xx_intc, "mediatek,mt58xx-intc", 
> mt58xx_intc_of_init);

On a side note, the merge window has just opened. Please refrain from
reposting this until -rc1.

Thanks,

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Mark-PK Tsai]

From: Marc Zyngier <maz@kernel.org>

> On 2020-08-03 07:22, Mark-PK Tsai wrote:
> > 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?

> 
> > +	unsigned int irq_start, nr_irqs;
> > +	void __iomem *base;
> > +
> };
> > +
> > +static void mtk_poke_irq(struct irq_data *d, u32 offset)
> 	> +{
> > +	struct mtk_intc_chip_data *cd = irq_data_get_irq_chip_data(d);
> > +	void __iomem *base = cd->base;
> > +	u8 index = (u8)irqd_to_hwirq(d);
> 
> Why the restrictive type? Why isn't unsigned int good enough?

You're right, unsigned int is ok.
I'll fix it in patch v2.

> 
> > +	u16 val, mask;
> > +
> > +	mask = 1 << (index % 16);
> > +	val = readw_relaxed(base + offset + (index / 16) * 4) | mask;
> > +	writew_relaxed(val, base + offset + (index / 16) * 4);
> 
> RMW without locking, you will end-up with corruption.
> Please store the address calculation in a temporaty variable to make it
> more readable
> 

Thanks for the comment, I will fix it in pacth v2.

> > +
> 	}
> > +
> > +static void mtk_clear_irq(struct irq_data *d, u32 offset)
> 	> +{
> > +	struct mtk_intc_chip_data *cd = irq_data_get_irq_chip_data(d);
> > +	void __iomem *base = cd->base;
> > +	u8 index = (u8)irqd_to_hwirq(d);
> > +	u16 val, mask;
> > +
> > +	mask = 1 << (index % 16);
> > +	val = readw_relaxed(base + offset + (index / 16) * 4) & ~mask;
> > +	writew_relaxed(val, base + offset + (index / 16) * 4);
> 
> Same comments.
> 

You're right, unsigned int is ok.
I'll fix it in patch v2.

> > +
> 	}
> > +
> > +static void mtk_intc_mask_irq(struct irq_data *d)
> 	> +{
> > +	mtk_poke_irq(d, INTC_MASK);
> > +	irq_chip_mask_parent(d);
> > +
> 	}
> > +
> > +static void mtk_intc_unmask_irq(struct irq_data *d)
> 	> +{
> > +	mtk_clear_irq(d, INTC_MASK);
> > +	irq_chip_unmask_parent(d);
> > +
> 	}
> > +
> > +static void mtk_intc_eoi_irq(struct irq_data *d)
> 	> +{
> > +	mtk_poke_irq(d, INTC_EOI);
> > +	irq_chip_eoi_parent(d);
> > +
> 	}
> > +
> > +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?
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.
Should I remove ".irq_set_vcpu_affinity" here?

> > +	.irq_set_type		= irq_chip_set_type_parent,
> > +	.flags			= IRQCHIP_SET_TYPE_MASKED |
> > +				  IRQCHIP_SKIP_SET_WAKE |
> > +				  IRQCHIP_MASK_ON_SUSPEND,
> > +
> };
> > +
> > +static int mt58xx_intc_domain_translate(struct irq_domain *d,
> > +					struct irq_fwspec *fwspec,
> > +					unsigned long *hwirq,
> > +					unsigned int *type)
> 	> +{
> 		> +	if (is_of_node(fwspec->fwnode)) {
> > +		if (fwspec->param_count != 3)
> > +			return -EINVAL;
> > +
> > +		/* No PPI should point to this domain */
> > +		if (fwspec->param[0] != 0)
> > +			return -EINVAL;
> > +
> > +		*hwirq = fwspec->param[1];
> > +		*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
> > +		return 0;
> > +	
> 		}
> > +
> > +	return -EINVAL;
> > +
> 	}
> > +
> > +static int mt58xx_intc_domain_alloc(struct irq_domain *domain,
> > unsigned int virq,
> > +				    unsigned int nr_irqs, void *data)
> 	> +{
> > +	int i;
> > +	irq_hw_number_t hwirq;
> > +	struct irq_fwspec parent_fwspec, *fwspec = data;
> > +	struct mtk_intc_chip_data *cd = (struct mtk_intc_chip_data
> > *)domain->host_data;
> > +
> > +	/* Not GIC compliant */
> > +	if (fwspec->param_count != 3)
> > +		return -EINVAL;
> > +
> > +	/* No PPI should point to this domain */
> > +	if (fwspec->param[0])
> > +		return -EINVAL;
> > +
> > +	if (fwspec->param[1] >= cd->nr_irqs)
> > +		return -EINVAL;
> > +
> > +	hwirq = fwspec->param[1];
> > +	for (i = 0; i < nr_irqs; i++)
> > +		irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
> > +					      &cd->chip,
> > +					      domain->host_data);
> > +
> > +	parent_fwspec = *fwspec;
> > +	parent_fwspec.fwnode = domain->parent->fwnode;
> > +	parent_fwspec.param[1] = cd->irq_start + hwirq;
> > +	return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, 
> > &parent_fwspec);
> > +
> 	}
> > +
> > +static const struct irq_domain_ops mt58xx_intc_domain_ops = {
> > +	.translate	= mt58xx_intc_domain_translate,
> > +	.alloc		= mt58xx_intc_domain_alloc,
> > +	.free		= irq_domain_free_irqs_common,
> > +
> };
> > +
> > +int __init
> > +mt58xx_intc_of_init(struct device_node *dn, struct device_node 
> > *parent)
> > +{
> > +	static int nr_intc;
> > +	struct irq_domain *domain, *domain_parent;
> > +	struct mtk_intc_chip_data *cd;
> > +	unsigned int irq_start, irq_end;
> > +
> > +	domain_parent = irq_find_host(parent);
> > +	if (!domain_parent) {
> > +		pr_err("mt58xx-intc: interrupt-parent not found\n");
> > +		return -EINVAL;
> > +	
> }
> > +
> > +	cd = kzalloc(sizeof(*cd), GFP_KERNEL);
> > +	if (!cd)
> > +		return -ENOMEM;
> > +
> > +	cd->chip = mtk_intc_chip;
> > +	if (of_property_read_u32_index(dn, "mediatek,irqs-map-range", 0,
> > &irq_start) ||
> > +	    of_property_read_u32_index(dn, "mediatek,irqs-map-range", 1, 
> 	> &irq_end)) {
> > +		kfree(cd);
> > +		return -EINVAL;
> > +	
> }
> > +
> > +	if (of_property_read_bool(dn, "mediatek,intc-no-eoi"))
> > +		cd->chip.irq_eoi = irq_chip_eoi_parent;
> 
> No. Just add a flag to your chip data structure, and check for this
> flag in your irq_eoi callback. Or provide two distinct irq_chip
> structures that will only differ by the irq_eoi method.

Thanks for the comment, I will modify it in patch v2.

> 
> > +
> > +	cd->irq_start = irq_start;
> > +	cd->nr_irqs = irq_end - irq_start + 1;
> > +	cd->chip.name = kasprintf(GFP_KERNEL, "mt58xx-intc-%d", nr_intc++);
> 
> Neither. That's not useful, and is a waste of memory. Stick to constant
> names in the irq_chip structure.
> 

Actually we have multiple irq controller in our SoCs.
And if we use the constant names in irq_chip structure, the information in
"/proc/interrupts" will be hard to understand because all the irqchip name
is the same.
Do you have any suggestion for this?

> > +	if (!cd->chip.name) {
> > +		kfree(cd);
> > +		return -ENOMEM;
> > +	
> }
> > +
> > +	cd->base = of_iomap(dn, 0);
> > +	if (!cd->base) {
> > +		kfree(cd->chip.name);
> > +		kfree(cd);
> > +		return -ENOMEM;
> > +	
> }
> > +
> > +	domain = irq_domain_add_hierarchy(domain_parent, 0, cd->nr_irqs,
> > +					  dn, &mt58xx_intc_domain_ops, cd);
> > +	if (!domain) {
> > +		kfree(cd->chip.name);
> > +		iounmap(cd->base);
> > +		kfree(cd);
> > +		return -ENOMEM;
> > +	
> }
> > +
> > +	return 0;
> > +
> }
> > +
> > +IRQCHIP_DECLARE(mt58xx_intc, "mediatek,mt58xx-intc", 
> > mt58xx_intc_of_init);
> 
> On a side note, the merge window has just opened. Please refrain from
> reposting this until -rc1.

Got it, and thanks for your comments.
I'll update the patch and post it after -rc1.

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

[Mark-PK Tsai]

From: Marc Zyngier <maz@kernel.org>

> On 2020-08-03 07:22, Mark-PK Tsai wrote:
> > 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?

> 
> > +	unsigned int irq_start, nr_irqs;
> > +	void __iomem *base;
> > +
> };
> > +
> > +static void mtk_poke_irq(struct irq_data *d, u32 offset)
> 	> +{
> > +	struct mtk_intc_chip_data *cd = irq_data_get_irq_chip_data(d);
> > +	void __iomem *base = cd->base;
> > +	u8 index = (u8)irqd_to_hwirq(d);
> 
> Why the restrictive type? Why isn't unsigned int good enough?

You're right, unsigned int is ok.
I'll fix it in patch v2.

> 
> > +	u16 val, mask;
> > +
> > +	mask = 1 << (index % 16);
> > +	val = readw_relaxed(base + offset + (index / 16) * 4) | mask;
> > +	writew_relaxed(val, base + offset + (index / 16) * 4);
> 
> RMW without locking, you will end-up with corruption.
> Please store the address calculation in a temporaty variable to make it
> more readable
> 

Thanks for the comment, I will fix it in pacth v2.

> > +
> 	}
> > +
> > +static void mtk_clear_irq(struct irq_data *d, u32 offset)
> 	> +{
> > +	struct mtk_intc_chip_data *cd = irq_data_get_irq_chip_data(d);
> > +	void __iomem *base = cd->base;
> > +	u8 index = (u8)irqd_to_hwirq(d);
> > +	u16 val, mask;
> > +
> > +	mask = 1 << (index % 16);
> > +	val = readw_relaxed(base + offset + (index / 16) * 4) & ~mask;
> > +	writew_relaxed(val, base + offset + (index / 16) * 4);
> 
> Same comments.
> 

You're right, unsigned int is ok.
I'll fix it in patch v2.

> > +
> 	}
> > +
> > +static void mtk_intc_mask_irq(struct irq_data *d)
> 	> +{
> > +	mtk_poke_irq(d, INTC_MASK);
> > +	irq_chip_mask_parent(d);
> > +
> 	}
> > +
> > +static void mtk_intc_unmask_irq(struct irq_data *d)
> 	> +{
> > +	mtk_clear_irq(d, INTC_MASK);
> > +	irq_chip_unmask_parent(d);
> > +
> 	}
> > +
> > +static void mtk_intc_eoi_irq(struct irq_data *d)
> 	> +{
> > +	mtk_poke_irq(d, INTC_EOI);
> > +	irq_chip_eoi_parent(d);
> > +
> 	}
> > +
> > +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?
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.
Should I remove ".irq_set_vcpu_affinity" here?

> > +	.irq_set_type		= irq_chip_set_type_parent,
> > +	.flags			= IRQCHIP_SET_TYPE_MASKED |
> > +				  IRQCHIP_SKIP_SET_WAKE |
> > +				  IRQCHIP_MASK_ON_SUSPEND,
> > +
> };
> > +
> > +static int mt58xx_intc_domain_translate(struct irq_domain *d,
> > +					struct irq_fwspec *fwspec,
> > +					unsigned long *hwirq,
> > +					unsigned int *type)
> 	> +{
> 		> +	if (is_of_node(fwspec->fwnode)) {
> > +		if (fwspec->param_count != 3)
> > +			return -EINVAL;
> > +
> > +		/* No PPI should point to this domain */
> > +		if (fwspec->param[0] != 0)
> > +			return -EINVAL;
> > +
> > +		*hwirq = fwspec->param[1];
> > +		*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
> > +		return 0;
> > +	
> 		}
> > +
> > +	return -EINVAL;
> > +
> 	}
> > +
> > +static int mt58xx_intc_domain_alloc(struct irq_domain *domain,
> > unsigned int virq,
> > +				    unsigned int nr_irqs, void *data)
> 	> +{
> > +	int i;
> > +	irq_hw_number_t hwirq;
> > +	struct irq_fwspec parent_fwspec, *fwspec = data;
> > +	struct mtk_intc_chip_data *cd = (struct mtk_intc_chip_data
> > *)domain->host_data;
> > +
> > +	/* Not GIC compliant */
> > +	if (fwspec->param_count != 3)
> > +		return -EINVAL;
> > +
> > +	/* No PPI should point to this domain */
> > +	if (fwspec->param[0])
> > +		return -EINVAL;
> > +
> > +	if (fwspec->param[1] >= cd->nr_irqs)
> > +		return -EINVAL;
> > +
> > +	hwirq = fwspec->param[1];
> > +	for (i = 0; i < nr_irqs; i++)
> > +		irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
> > +					      &cd->chip,
> > +					      domain->host_data);
> > +
> > +	parent_fwspec = *fwspec;
> > +	parent_fwspec.fwnode = domain->parent->fwnode;
> > +	parent_fwspec.param[1] = cd->irq_start + hwirq;
> > +	return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, 
> > &parent_fwspec);
> > +
> 	}
> > +
> > +static const struct irq_domain_ops mt58xx_intc_domain_ops = {
> > +	.translate	= mt58xx_intc_domain_translate,
> > +	.alloc		= mt58xx_intc_domain_alloc,
> > +	.free		= irq_domain_free_irqs_common,
> > +
> };
> > +
> > +int __init
> > +mt58xx_intc_of_init(struct device_node *dn, struct device_node 
> > *parent)
> > +{
> > +	static int nr_intc;
> > +	struct irq_domain *domain, *domain_parent;
> > +	struct mtk_intc_chip_data *cd;
> > +	unsigned int irq_start, irq_end;
> > +
> > +	domain_parent = irq_find_host(parent);
> > +	if (!domain_parent) {
> > +		pr_err("mt58xx-intc: interrupt-parent not found\n");
> > +		return -EINVAL;
> > +	
> }
> > +
> > +	cd = kzalloc(sizeof(*cd), GFP_KERNEL);
> > +	if (!cd)
> > +		return -ENOMEM;
> > +
> > +	cd->chip = mtk_intc_chip;
> > +	if (of_property_read_u32_index(dn, "mediatek,irqs-map-range", 0,
> > &irq_start) ||
> > +	    of_property_read_u32_index(dn, "mediatek,irqs-map-range", 1, 
> 	> &irq_end)) {
> > +		kfree(cd);
> > +		return -EINVAL;
> > +	
> }
> > +
> > +	if (of_property_read_bool(dn, "mediatek,intc-no-eoi"))
> > +		cd->chip.irq_eoi = irq_chip_eoi_parent;
> 
> No. Just add a flag to your chip data structure, and check for this
> flag in your irq_eoi callback. Or provide two distinct irq_chip
> structures that will only differ by the irq_eoi method.

Thanks for the comment, I will modify it in patch v2.

> 
> > +
> > +	cd->irq_start = irq_start;
> > +	cd->nr_irqs = irq_end - irq_start + 1;
> > +	cd->chip.name = kasprintf(GFP_KERNEL, "mt58xx-intc-%d", nr_intc++);
> 
> Neither. That's not useful, and is a waste of memory. Stick to constant
> names in the irq_chip structure.
> 

Actually we have multiple irq controller in our SoCs.
And if we use the constant names in irq_chip structure, the information in
"/proc/interrupts" will be hard to understand because all the irqchip name
is the same.
Do you have any suggestion for this?

> > +	if (!cd->chip.name) {
> > +		kfree(cd);
> > +		return -ENOMEM;
> > +	
> }
> > +
> > +	cd->base = of_iomap(dn, 0);
> > +	if (!cd->base) {
> > +		kfree(cd->chip.name);
> > +		kfree(cd);
> > +		return -ENOMEM;
> > +	
> }
> > +
> > +	domain = irq_domain_add_hierarchy(domain_parent, 0, cd->nr_irqs,
> > +					  dn, &mt58xx_intc_domain_ops, cd);
> > +	if (!domain) {
> > +		kfree(cd->chip.name);
> > +		iounmap(cd->base);
> > +		kfree(cd);
> > +		return -ENOMEM;
> > +	
> }
> > +
> > +	return 0;
> > +
> }
> > +
> > +IRQCHIP_DECLARE(mt58xx_intc, "mediatek,mt58xx-intc", 
> > mt58xx_intc_of_init);
> 
> On a side note, the merge window has just opened. Please refrain from
> reposting this until -rc1.

Got it, and thanks for your comments.
I'll update the patch and post it after -rc1.
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

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

[Mark-PK Tsai]

From: Marc Zyngier <maz@kernel.org>

> On 2020-08-03 07:22, Mark-PK Tsai wrote:
> > 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?

> 
> > +	unsigned int irq_start, nr_irqs;
> > +	void __iomem *base;
> > +
> };
> > +
> > +static void mtk_poke_irq(struct irq_data *d, u32 offset)
> 	> +{
> > +	struct mtk_intc_chip_data *cd = irq_data_get_irq_chip_data(d);
> > +	void __iomem *base = cd->base;
> > +	u8 index = (u8)irqd_to_hwirq(d);
> 
> Why the restrictive type? Why isn't unsigned int good enough?

You're right, unsigned int is ok.
I'll fix it in patch v2.

> 
> > +	u16 val, mask;
> > +
> > +	mask = 1 << (index % 16);
> > +	val = readw_relaxed(base + offset + (index / 16) * 4) | mask;
> > +	writew_relaxed(val, base + offset + (index / 16) * 4);
> 
> RMW without locking, you will end-up with corruption.
> Please store the address calculation in a temporaty variable to make it
> more readable
> 

Thanks for the comment, I will fix it in pacth v2.

> > +
> 	}
> > +
> > +static void mtk_clear_irq(struct irq_data *d, u32 offset)
> 	> +{
> > +	struct mtk_intc_chip_data *cd = irq_data_get_irq_chip_data(d);
> > +	void __iomem *base = cd->base;
> > +	u8 index = (u8)irqd_to_hwirq(d);
> > +	u16 val, mask;
> > +
> > +	mask = 1 << (index % 16);
> > +	val = readw_relaxed(base + offset + (index / 16) * 4) & ~mask;
> > +	writew_relaxed(val, base + offset + (index / 16) * 4);
> 
> Same comments.
> 

You're right, unsigned int is ok.
I'll fix it in patch v2.

> > +
> 	}
> > +
> > +static void mtk_intc_mask_irq(struct irq_data *d)
> 	> +{
> > +	mtk_poke_irq(d, INTC_MASK);
> > +	irq_chip_mask_parent(d);
> > +
> 	}
> > +
> > +static void mtk_intc_unmask_irq(struct irq_data *d)
> 	> +{
> > +	mtk_clear_irq(d, INTC_MASK);
> > +	irq_chip_unmask_parent(d);
> > +
> 	}
> > +
> > +static void mtk_intc_eoi_irq(struct irq_data *d)
> 	> +{
> > +	mtk_poke_irq(d, INTC_EOI);
> > +	irq_chip_eoi_parent(d);
> > +
> 	}
> > +
> > +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?
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.
Should I remove ".irq_set_vcpu_affinity" here?

> > +	.irq_set_type		= irq_chip_set_type_parent,
> > +	.flags			= IRQCHIP_SET_TYPE_MASKED |
> > +				  IRQCHIP_SKIP_SET_WAKE |
> > +				  IRQCHIP_MASK_ON_SUSPEND,
> > +
> };
> > +
> > +static int mt58xx_intc_domain_translate(struct irq_domain *d,
> > +					struct irq_fwspec *fwspec,
> > +					unsigned long *hwirq,
> > +					unsigned int *type)
> 	> +{
> 		> +	if (is_of_node(fwspec->fwnode)) {
> > +		if (fwspec->param_count != 3)
> > +			return -EINVAL;
> > +
> > +		/* No PPI should point to this domain */
> > +		if (fwspec->param[0] != 0)
> > +			return -EINVAL;
> > +
> > +		*hwirq = fwspec->param[1];
> > +		*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
> > +		return 0;
> > +	
> 		}
> > +
> > +	return -EINVAL;
> > +
> 	}
> > +
> > +static int mt58xx_intc_domain_alloc(struct irq_domain *domain,
> > unsigned int virq,
> > +				    unsigned int nr_irqs, void *data)
> 	> +{
> > +	int i;
> > +	irq_hw_number_t hwirq;
> > +	struct irq_fwspec parent_fwspec, *fwspec = data;
> > +	struct mtk_intc_chip_data *cd = (struct mtk_intc_chip_data
> > *)domain->host_data;
> > +
> > +	/* Not GIC compliant */
> > +	if (fwspec->param_count != 3)
> > +		return -EINVAL;
> > +
> > +	/* No PPI should point to this domain */
> > +	if (fwspec->param[0])
> > +		return -EINVAL;
> > +
> > +	if (fwspec->param[1] >= cd->nr_irqs)
> > +		return -EINVAL;
> > +
> > +	hwirq = fwspec->param[1];
> > +	for (i = 0; i < nr_irqs; i++)
> > +		irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
> > +					      &cd->chip,
> > +					      domain->host_data);
> > +
> > +	parent_fwspec = *fwspec;
> > +	parent_fwspec.fwnode = domain->parent->fwnode;
> > +	parent_fwspec.param[1] = cd->irq_start + hwirq;
> > +	return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, 
> > &parent_fwspec);
> > +
> 	}
> > +
> > +static const struct irq_domain_ops mt58xx_intc_domain_ops = {
> > +	.translate	= mt58xx_intc_domain_translate,
> > +	.alloc		= mt58xx_intc_domain_alloc,
> > +	.free		= irq_domain_free_irqs_common,
> > +
> };
> > +
> > +int __init
> > +mt58xx_intc_of_init(struct device_node *dn, struct device_node 
> > *parent)
> > +{
> > +	static int nr_intc;
> > +	struct irq_domain *domain, *domain_parent;
> > +	struct mtk_intc_chip_data *cd;
> > +	unsigned int irq_start, irq_end;
> > +
> > +	domain_parent = irq_find_host(parent);
> > +	if (!domain_parent) {
> > +		pr_err("mt58xx-intc: interrupt-parent not found\n");
> > +		return -EINVAL;
> > +	
> }
> > +
> > +	cd = kzalloc(sizeof(*cd), GFP_KERNEL);
> > +	if (!cd)
> > +		return -ENOMEM;
> > +
> > +	cd->chip = mtk_intc_chip;
> > +	if (of_property_read_u32_index(dn, "mediatek,irqs-map-range", 0,
> > &irq_start) ||
> > +	    of_property_read_u32_index(dn, "mediatek,irqs-map-range", 1, 
> 	> &irq_end)) {
> > +		kfree(cd);
> > +		return -EINVAL;
> > +	
> }
> > +
> > +	if (of_property_read_bool(dn, "mediatek,intc-no-eoi"))
> > +		cd->chip.irq_eoi = irq_chip_eoi_parent;
> 
> No. Just add a flag to your chip data structure, and check for this
> flag in your irq_eoi callback. Or provide two distinct irq_chip
> structures that will only differ by the irq_eoi method.

Thanks for the comment, I will modify it in patch v2.

> 
> > +
> > +	cd->irq_start = irq_start;
> > +	cd->nr_irqs = irq_end - irq_start + 1;
> > +	cd->chip.name = kasprintf(GFP_KERNEL, "mt58xx-intc-%d", nr_intc++);
> 
> Neither. That's not useful, and is a waste of memory. Stick to constant
> names in the irq_chip structure.
> 

Actually we have multiple irq controller in our SoCs.
And if we use the constant names in irq_chip structure, the information in
"/proc/interrupts" will be hard to understand because all the irqchip name
is the same.
Do you have any suggestion for this?

> > +	if (!cd->chip.name) {
> > +		kfree(cd);
> > +		return -ENOMEM;
> > +	
> }
> > +
> > +	cd->base = of_iomap(dn, 0);
> > +	if (!cd->base) {
> > +		kfree(cd->chip.name);
> > +		kfree(cd);
> > +		return -ENOMEM;
> > +	
> }
> > +
> > +	domain = irq_domain_add_hierarchy(domain_parent, 0, cd->nr_irqs,
> > +					  dn, &mt58xx_intc_domain_ops, cd);
> > +	if (!domain) {
> > +		kfree(cd->chip.name);
> > +		iounmap(cd->base);
> > +		kfree(cd);
> > +		return -ENOMEM;
> > +	
> }
> > +
> > +	return 0;
> > +
> }
> > +
> > +IRQCHIP_DECLARE(mt58xx_intc, "mediatek,mt58xx-intc", 
> > mt58xx_intc_of_init);
> 
> On a side note, the merge window has just opened. Please refrain from
> reposting this until -rc1.

Got it, and thanks for your comments.
I'll update the patch and post it after -rc1.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Marc Zyngier]

On 2020-08-03 16:03, Mark-PK Tsai wrote:
> From: Marc Zyngier <maz@kernel.org>
> 
>> On 2020-08-03 07:22, Mark-PK Tsai wrote:
>> > 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.

[...]

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

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

[Marc Zyngier]

On 2020-08-03 16:03, Mark-PK Tsai wrote:
> From: Marc Zyngier <maz@kernel.org>
> 
>> On 2020-08-03 07:22, Mark-PK Tsai wrote:
>> > 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.

[...]

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

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

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

[Marc Zyngier]

On 2020-08-03 16:03, Mark-PK Tsai wrote:
> From: Marc Zyngier <maz@kernel.org>
> 
>> On 2020-08-03 07:22, Mark-PK Tsai wrote:
>> > 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.

[...]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] dt-bindings: interrupt-controller: Add MT58XX interrupt controller

[Rob Herring]

On Mon, 03 Aug 2020 14:22:14 +0800, Mark-PK Tsai wrote:
> Add binding for MT58XX interrupt controller.
> 
> Signed-off-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
> ---
>  .../mediatek,mt58xx-intc.yaml                 | 70 +++++++++++++++++++
>  1 file changed, 70 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.yaml
> 


My bot found errors running 'make dt_binding_check' on your patch:

/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.yaml: properties:compatible: [{'items': [{'const': 'mediatek,mt58xx-intc'}]}] is not of type 'object', 'boolean'
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.yaml: properties:mediatek,intc-no-eoi: {'description': 'Mark this controller has no End Of Interrupt(EOI) implementation.\nThis is a empty, boolean property.\n'} is not valid under any of the given schemas (Possible causes of the failure):
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.yaml: properties:mediatek,intc-no-eoi: 'not' is a required property

/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.yaml: properties:mediatek,irqs-map-range: {'description': "The range of parent interrupt controller's interrupt lines\nthat are hardwired to MT58xx interrupt controller.\n"} is not valid under any of the given schemas (Possible causes of the failure):
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.yaml: properties:mediatek,irqs-map-range: 'not' is a required property

Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.yaml: $id: relative path/filename doesn't match actual path or filename
	expected: http://devicetree.org/schemas/interrupt-controller/mediatek,mt58xx-intc.yaml#
Documentation/devicetree/bindings/Makefile:20: recipe for target 'Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.example.dts' failed
make[1]: *** [Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.example.dts] Error 1
make[1]: *** Waiting for unfinished jobs....
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.yaml: ignoring, error in schema: properties: compatible
warning: no schema found in file: ./Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.yaml: ignoring, error in schema: properties: compatible
warning: no schema found in file: ./Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.yaml
Makefile:1347: recipe for target 'dt_binding_check' failed
make: *** [dt_binding_check] Error 2


See https://patchwork.ozlabs.org/patch/1340146

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure dt-schema is up to date:

pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --upgrade

Please check and re-submit.

Re: [PATCH 2/2] dt-bindings: interrupt-controller: Add MT58XX interrupt controller

[Rob Herring]

On Mon, 03 Aug 2020 14:22:14 +0800, Mark-PK Tsai wrote:
> Add binding for MT58XX interrupt controller.
> 
> Signed-off-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
> ---
>  .../mediatek,mt58xx-intc.yaml                 | 70 +++++++++++++++++++
>  1 file changed, 70 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.yaml
> 


My bot found errors running 'make dt_binding_check' on your patch:

/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.yaml: properties:compatible: [{'items': [{'const': 'mediatek,mt58xx-intc'}]}] is not of type 'object', 'boolean'
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.yaml: properties:mediatek,intc-no-eoi: {'description': 'Mark this controller has no End Of Interrupt(EOI) implementation.\nThis is a empty, boolean property.\n'} is not valid under any of the given schemas (Possible causes of the failure):
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.yaml: properties:mediatek,intc-no-eoi: 'not' is a required property

/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.yaml: properties:mediatek,irqs-map-range: {'description': "The range of parent interrupt controller's interrupt lines\nthat are hardwired to MT58xx interrupt controller.\n"} is not valid under any of the given schemas (Possible causes of the failure):
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.yaml: properties:mediatek,irqs-map-range: 'not' is a required property

Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.yaml: $id: relative path/filename doesn't match actual path or filename
	expected: http://devicetree.org/schemas/interrupt-controller/mediatek,mt58xx-intc.yaml#
Documentation/devicetree/bindings/Makefile:20: recipe for target 'Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.example.dts' failed
make[1]: *** [Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.example.dts] Error 1
make[1]: *** Waiting for unfinished jobs....
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.yaml: ignoring, error in schema: properties: compatible
warning: no schema found in file: ./Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.yaml: ignoring, error in schema: properties: compatible
warning: no schema found in file: ./Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.yaml
Makefile:1347: recipe for target 'dt_binding_check' failed
make: *** [dt_binding_check] Error 2


See https://patchwork.ozlabs.org/patch/1340146

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure dt-schema is up to date:

pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --upgrade

Please check and re-submit.


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH 2/2] dt-bindings: interrupt-controller: Add MT58XX interrupt controller

[Rob Herring]

On Mon, 03 Aug 2020 14:22:14 +0800, Mark-PK Tsai wrote:
> Add binding for MT58XX interrupt controller.
> 
> Signed-off-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
> ---
>  .../mediatek,mt58xx-intc.yaml                 | 70 +++++++++++++++++++
>  1 file changed, 70 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.yaml
> 


My bot found errors running 'make dt_binding_check' on your patch:

/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.yaml: properties:compatible: [{'items': [{'const': 'mediatek,mt58xx-intc'}]}] is not of type 'object', 'boolean'
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.yaml: properties:mediatek,intc-no-eoi: {'description': 'Mark this controller has no End Of Interrupt(EOI) implementation.\nThis is a empty, boolean property.\n'} is not valid under any of the given schemas (Possible causes of the failure):
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.yaml: properties:mediatek,intc-no-eoi: 'not' is a required property

/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.yaml: properties:mediatek,irqs-map-range: {'description': "The range of parent interrupt controller's interrupt lines\nthat are hardwired to MT58xx interrupt controller.\n"} is not valid under any of the given schemas (Possible causes of the failure):
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.yaml: properties:mediatek,irqs-map-range: 'not' is a required property

Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.yaml: $id: relative path/filename doesn't match actual path or filename
	expected: http://devicetree.org/schemas/interrupt-controller/mediatek,mt58xx-intc.yaml#
Documentation/devicetree/bindings/Makefile:20: recipe for target 'Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.example.dts' failed
make[1]: *** [Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.example.dts] Error 1
make[1]: *** Waiting for unfinished jobs....
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.yaml: ignoring, error in schema: properties: compatible
warning: no schema found in file: ./Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.yaml: ignoring, error in schema: properties: compatible
warning: no schema found in file: ./Documentation/devicetree/bindings/interrupt-controller/mediatek,mt58xx-intc.yaml
Makefile:1347: recipe for target 'dt_binding_check' failed
make: *** [dt_binding_check] Error 2


See https://patchwork.ozlabs.org/patch/1340146

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure dt-schema is up to date:

pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --upgrade

Please check and re-submit.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/2] irqchip: irq-mt58xx: Add mt58xx series interrupt

[Daniel Palmer]

Hi Mark-PK,

Your driver seems to be for the same interrupt controller IP that is
present in MStar's TV and camera SoCs and now SigmaStar's SoCs.
I sent a series[0] for a driver very similar to yours but for the
MStar SoCs. Do you know if it would be possible to confirm if they are
the
same thing? MediaTek bought MStar a few years ago so it seems likely
but I have no hard information.

If they are the same thing could we work on making one series that
supports both use cases?
It's also possible that if the interrupt controller is the same some
other things like the SPI NOR controller etc are also common and
working
on a single driver for those would save us both time.

[0] - https://lore.kernel.org/linux-arm-kernel/20200805110052.2655487-1-daniel@0x0f.com/

Re: [PATCH 0/2] irqchip: irq-mt58xx: Add mt58xx series interrupt

[Daniel Palmer]

Hi Mark-PK,

Your driver seems to be for the same interrupt controller IP that is
present in MStar's TV and camera SoCs and now SigmaStar's SoCs.
I sent a series[0] for a driver very similar to yours but for the
MStar SoCs. Do you know if it would be possible to confirm if they are
the
same thing? MediaTek bought MStar a few years ago so it seems likely
but I have no hard information.

If they are the same thing could we work on making one series that
supports both use cases?
It's also possible that if the interrupt controller is the same some
other things like the SPI NOR controller etc are also common and
working
on a single driver for those would save us both time.

[0] - https://lore.kernel.org/linux-arm-kernel/20200805110052.2655487-1-daniel@0x0f.com/

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH 0/2] irqchip: irq-mt58xx: Add mt58xx series interrupt

[Daniel Palmer]

Hi Mark-PK,

Your driver seems to be for the same interrupt controller IP that is
present in MStar's TV and camera SoCs and now SigmaStar's SoCs.
I sent a series[0] for a driver very similar to yours but for the
MStar SoCs. Do you know if it would be possible to confirm if they are
the
same thing? MediaTek bought MStar a few years ago so it seems likely
but I have no hard information.

If they are the same thing could we work on making one series that
supports both use cases?
It's also possible that if the interrupt controller is the same some
other things like the SPI NOR controller etc are also common and
working
on a single driver for those would save us both time.

[0] - https://lore.kernel.org/linux-arm-kernel/20200805110052.2655487-1-daniel@0x0f.com/

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/2] irqchip: irq-mt58xx: Add mt58xx series interrupt

[Mark-PK Tsai]

From: Daniel Palmer <daniel@0x0f.com>

> Hi Mark-PK,
> 
> Your driver seems to be for the same interrupt controller IP that is
> present in MStar's TV and camera SoCs and now SigmaStar's SoCs.
> I sent a series[0] for a driver very similar to yours but for the
> MStar SoCs. Do you know if it would be possible to confirm if they are
> the
> same thing? MediaTek bought MStar a few years ago so it seems likely
> but I have no hard information.
> 

Yes, it's for the same interrupt controller IP.

> If they are the same thing could we work on making one series that
> supports both use cases?

Sure, and I think the irq controller driver should support both use cases.
So how about keep the MTK version driver?
I'll send patch v2 after -rc1 as I mentioned in the previous mail,
and keep you posted.
And any suggestion is welcome. :)

> It's also possible that if the interrupt controller is the same some
> other things like the SPI NOR controller etc are also common and
> working
> on a single driver for those would save us both time.
> 

I'm not sure about that.
I'll check the patches you contributed and confirm with our driver owners.

Re: [PATCH 0/2] irqchip: irq-mt58xx: Add mt58xx series interrupt

[Mark-PK Tsai]

From: Daniel Palmer <daniel@0x0f.com>

> Hi Mark-PK,
> 
> Your driver seems to be for the same interrupt controller IP that is
> present in MStar's TV and camera SoCs and now SigmaStar's SoCs.
> I sent a series[0] for a driver very similar to yours but for the
> MStar SoCs. Do you know if it would be possible to confirm if they are
> the
> same thing? MediaTek bought MStar a few years ago so it seems likely
> but I have no hard information.
> 

Yes, it's for the same interrupt controller IP.

> If they are the same thing could we work on making one series that
> supports both use cases?

Sure, and I think the irq controller driver should support both use cases.
So how about keep the MTK version driver?
I'll send patch v2 after -rc1 as I mentioned in the previous mail,
and keep you posted.
And any suggestion is welcome. :)

> It's also possible that if the interrupt controller is the same some
> other things like the SPI NOR controller etc are also common and
> working
> on a single driver for those would save us both time.
> 

I'm not sure about that.
I'll check the patches you contributed and confirm with our driver owners.
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH 0/2] irqchip: irq-mt58xx: Add mt58xx series interrupt

[Mark-PK Tsai]

From: Daniel Palmer <daniel@0x0f.com>

> Hi Mark-PK,
> 
> Your driver seems to be for the same interrupt controller IP that is
> present in MStar's TV and camera SoCs and now SigmaStar's SoCs.
> I sent a series[0] for a driver very similar to yours but for the
> MStar SoCs. Do you know if it would be possible to confirm if they are
> the
> same thing? MediaTek bought MStar a few years ago so it seems likely
> but I have no hard information.
> 

Yes, it's for the same interrupt controller IP.

> If they are the same thing could we work on making one series that
> supports both use cases?

Sure, and I think the irq controller driver should support both use cases.
So how about keep the MTK version driver?
I'll send patch v2 after -rc1 as I mentioned in the previous mail,
and keep you posted.
And any suggestion is welcome. :)

> It's also possible that if the interrupt controller is the same some
> other things like the SPI NOR controller etc are also common and
> working
> on a single driver for those would save us both time.
> 

I'm not sure about that.
I'll check the patches you contributed and confirm with our driver owners.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/2] irqchip: irq-mt58xx: Add mt58xx series interrupt

[Daniel Palmer]

Hi Mark-PK,

On Thu, 6 Aug 2020 at 23:08, Mark-PK Tsai <mark-pk.tsai@mediatek.com> wrote:
> > Do you know if it would be possible to confirm if they are
> > the
> > same thing? MediaTek bought MStar a few years ago so it seems likely
> > but I have no hard information.
> >
>
> Yes, it's for the same interrupt controller IP.

That's good news. :)

> > If they are the same thing could we work on making one series that
> > supports both use cases?
>
> Sure, and I think the irq controller driver should support both use cases.
> So how about keep the MTK version driver?

I'm fine with that. Maybe you can push the MTK version and I can send
a small patch after that to add the small bits I need?

> I'll send patch v2 after -rc1 as I mentioned in the previous mail,
> and keep you posted.
> And any suggestion is welcome. :)

I think Marc's comments on my series apply to your driver and I think
maybe answer some of the questions you had. For example what
to do about the read-modify-write sequence for updating the registers.

> > It's also possible that if the interrupt controller is the same some
> > other things like the SPI NOR controller etc are also common and
> > working
> > on a single driver for those would save us both time.
>
> I'm not sure about that.
> I'll check the patches you contributed and confirm with our driver owners.

I have a very messy tree with support for the SPI NOR controller, SPI, i2c etc
that were used in MStar chips. If any of those are shared it'd be great to know.

Thanks,

Daniel

Re: [PATCH 0/2] irqchip: irq-mt58xx: Add mt58xx series interrupt

[Daniel Palmer]

Hi Mark-PK,

On Thu, 6 Aug 2020 at 23:08, Mark-PK Tsai <mark-pk.tsai@mediatek.com> wrote:
> > Do you know if it would be possible to confirm if they are
> > the
> > same thing? MediaTek bought MStar a few years ago so it seems likely
> > but I have no hard information.
> >
>
> Yes, it's for the same interrupt controller IP.

That's good news. :)

> > If they are the same thing could we work on making one series that
> > supports both use cases?
>
> Sure, and I think the irq controller driver should support both use cases.
> So how about keep the MTK version driver?

I'm fine with that. Maybe you can push the MTK version and I can send
a small patch after that to add the small bits I need?

> I'll send patch v2 after -rc1 as I mentioned in the previous mail,
> and keep you posted.
> And any suggestion is welcome. :)

I think Marc's comments on my series apply to your driver and I think
maybe answer some of the questions you had. For example what
to do about the read-modify-write sequence for updating the registers.

> > It's also possible that if the interrupt controller is the same some
> > other things like the SPI NOR controller etc are also common and
> > working
> > on a single driver for those would save us both time.
>
> I'm not sure about that.
> I'll check the patches you contributed and confirm with our driver owners.

I have a very messy tree with support for the SPI NOR controller, SPI, i2c etc
that were used in MStar chips. If any of those are shared it'd be great to know.

Thanks,

Daniel

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH 0/2] irqchip: irq-mt58xx: Add mt58xx series interrupt

[Daniel Palmer]

Hi Mark-PK,

On Thu, 6 Aug 2020 at 23:08, Mark-PK Tsai <mark-pk.tsai@mediatek.com> wrote:
> > Do you know if it would be possible to confirm if they are
> > the
> > same thing? MediaTek bought MStar a few years ago so it seems likely
> > but I have no hard information.
> >
>
> Yes, it's for the same interrupt controller IP.

That's good news. :)

> > If they are the same thing could we work on making one series that
> > supports both use cases?
>
> Sure, and I think the irq controller driver should support both use cases.
> So how about keep the MTK version driver?

I'm fine with that. Maybe you can push the MTK version and I can send
a small patch after that to add the small bits I need?

> I'll send patch v2 after -rc1 as I mentioned in the previous mail,
> and keep you posted.
> And any suggestion is welcome. :)

I think Marc's comments on my series apply to your driver and I think
maybe answer some of the questions you had. For example what
to do about the read-modify-write sequence for updating the registers.

> > It's also possible that if the interrupt controller is the same some
> > other things like the SPI NOR controller etc are also common and
> > working
> > on a single driver for those would save us both time.
>
> I'm not sure about that.
> I'll check the patches you contributed and confirm with our driver owners.

I have a very messy tree with support for the SPI NOR controller, SPI, i2c etc
that were used in MStar chips. If any of those are shared it'd be great to know.

Thanks,

Daniel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/2] irqchip: irq-mt58xx: Add mt58xx series interrupt

[Marc Zyngier]

On 2020-08-06 15:58, Daniel Palmer wrote:
> Hi Mark-PK,
> 
> On Thu, 6 Aug 2020 at 23:08, Mark-PK Tsai <mark-pk.tsai@mediatek.com> 
> wrote:
>> > Do you know if it would be possible to confirm if they are
>> > the
>> > same thing? MediaTek bought MStar a few years ago so it seems likely
>> > but I have no hard information.
>> >
>> 
>> Yes, it's for the same interrupt controller IP.
> 
> That's good news. :)
> 
>> > If they are the same thing could we work on making one series that
>> > supports both use cases?
>> 
>> Sure, and I think the irq controller driver should support both use 
>> cases.
>> So how about keep the MTK version driver?
> 
> I'm fine with that. Maybe you can push the MTK version and I can send
> a small patch after that to add the small bits I need?

In the interest of being vendor agnostic, please rename the properties
such as mediatek,irqs-map-range to something less brand-specific.
The compatible string should be enough.

Thanks,

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

Re: [PATCH 0/2] irqchip: irq-mt58xx: Add mt58xx series interrupt

[Marc Zyngier]

On 2020-08-06 15:58, Daniel Palmer wrote:
> Hi Mark-PK,
> 
> On Thu, 6 Aug 2020 at 23:08, Mark-PK Tsai <mark-pk.tsai@mediatek.com> 
> wrote:
>> > Do you know if it would be possible to confirm if they are
>> > the
>> > same thing? MediaTek bought MStar a few years ago so it seems likely
>> > but I have no hard information.
>> >
>> 
>> Yes, it's for the same interrupt controller IP.
> 
> That's good news. :)
> 
>> > If they are the same thing could we work on making one series that
>> > supports both use cases?
>> 
>> Sure, and I think the irq controller driver should support both use 
>> cases.
>> So how about keep the MTK version driver?
> 
> I'm fine with that. Maybe you can push the MTK version and I can send
> a small patch after that to add the small bits I need?

In the interest of being vendor agnostic, please rename the properties
such as mediatek,irqs-map-range to something less brand-specific.
The compatible string should be enough.

Thanks,

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

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH 0/2] irqchip: irq-mt58xx: Add mt58xx series interrupt

[Marc Zyngier]

On 2020-08-06 15:58, Daniel Palmer wrote:
> Hi Mark-PK,
> 
> On Thu, 6 Aug 2020 at 23:08, Mark-PK Tsai <mark-pk.tsai@mediatek.com> 
> wrote:
>> > Do you know if it would be possible to confirm if they are
>> > the
>> > same thing? MediaTek bought MStar a few years ago so it seems likely
>> > but I have no hard information.
>> >
>> 
>> Yes, it's for the same interrupt controller IP.
> 
> That's good news. :)
> 
>> > If they are the same thing could we work on making one series that
>> > supports both use cases?
>> 
>> Sure, and I think the irq controller driver should support both use 
>> cases.
>> So how about keep the MTK version driver?
> 
> I'm fine with that. Maybe you can push the MTK version and I can send
> a small patch after that to add the small bits I need?

In the interest of being vendor agnostic, please rename the properties
such as mediatek,irqs-map-range to something less brand-specific.
The compatible string should be enough.

Thanks,

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/2] irqchip: irq-mt58xx: Add mt58xx series interrupt

[Mark-PK Tsai]

From: Marc Zyngier <maz@kernel.org>

> On 2020-08-06 15:58, Daniel Palmer wrote:
> > Hi Mark-PK,
> > 
> > On Thu, 6 Aug 2020 at 23:08, Mark-PK Tsai <mark-pk.tsai@mediatek.com> 
> > wrote:
> >> > Do you know if it would be possible to confirm if they are
> >> > the
> >> > same thing? MediaTek bought MStar a few years ago so it seems likely
> >> > but I have no hard information.
> >> >
> >> 
> >> Yes, it's for the same interrupt controller IP.
> > 
> > That's good news. :)
> > 
> >> > If they are the same thing could we work on making one series that
> >> > supports both use cases?
> >> 
> >> Sure, and I think the irq controller driver should support both use 
> >> cases.
> >> So how about keep the MTK version driver?
> > 
> > I'm fine with that. Maybe you can push the MTK version and I can send
> > a small patch after that to add the small bits I need?
> 
> In the interest of being vendor agnostic, please rename the properties
> such as mediatek,irqs-map-range to something less brand-specific.
> The compatible string should be enough.

I can't find the suitable property in standard ones that match the custom
properties here.
And the vendor prefixed rule is described in [1].

The interrupt controller is first used in Mstar TV SoCs.
Now it's used in MTK TV and Sigmastar SoCs.
So I think Mstar prefixed would make more sense.
I will rename the driver into mstar-intc, and MTK will maintain this driver.

[1] https://www.kernel.org/doc/Documentation/devicetree/booting-without-of.txt

Re: [PATCH 0/2] irqchip: irq-mt58xx: Add mt58xx series interrupt

[Mark-PK Tsai]

From: Marc Zyngier <maz@kernel.org>

> On 2020-08-06 15:58, Daniel Palmer wrote:
> > Hi Mark-PK,
> > 
> > On Thu, 6 Aug 2020 at 23:08, Mark-PK Tsai <mark-pk.tsai@mediatek.com> 
> > wrote:
> >> > Do you know if it would be possible to confirm if they are
> >> > the
> >> > same thing? MediaTek bought MStar a few years ago so it seems likely
> >> > but I have no hard information.
> >> >
> >> 
> >> Yes, it's for the same interrupt controller IP.
> > 
> > That's good news. :)
> > 
> >> > If they are the same thing could we work on making one series that
> >> > supports both use cases?
> >> 
> >> Sure, and I think the irq controller driver should support both use 
> >> cases.
> >> So how about keep the MTK version driver?
> > 
> > I'm fine with that. Maybe you can push the MTK version and I can send
> > a small patch after that to add the small bits I need?
> 
> In the interest of being vendor agnostic, please rename the properties
> such as mediatek,irqs-map-range to something less brand-specific.
> The compatible string should be enough.

I can't find the suitable property in standard ones that match the custom
properties here.
And the vendor prefixed rule is described in [1].

The interrupt controller is first used in Mstar TV SoCs.
Now it's used in MTK TV and Sigmastar SoCs.
So I think Mstar prefixed would make more sense.
I will rename the driver into mstar-intc, and MTK will maintain this driver.

[1] https://www.kernel.org/doc/Documentation/devicetree/booting-without-of.txt
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH 0/2] irqchip: irq-mt58xx: Add mt58xx series interrupt

[Mark-PK Tsai]

From: Marc Zyngier <maz@kernel.org>

> On 2020-08-06 15:58, Daniel Palmer wrote:
> > Hi Mark-PK,
> > 
> > On Thu, 6 Aug 2020 at 23:08, Mark-PK Tsai <mark-pk.tsai@mediatek.com> 
> > wrote:
> >> > Do you know if it would be possible to confirm if they are
> >> > the
> >> > same thing? MediaTek bought MStar a few years ago so it seems likely
> >> > but I have no hard information.
> >> >
> >> 
> >> Yes, it's for the same interrupt controller IP.
> > 
> > That's good news. :)
> > 
> >> > If they are the same thing could we work on making one series that
> >> > supports both use cases?
> >> 
> >> Sure, and I think the irq controller driver should support both use 
> >> cases.
> >> So how about keep the MTK version driver?
> > 
> > I'm fine with that. Maybe you can push the MTK version and I can send
> > a small patch after that to add the small bits I need?
> 
> In the interest of being vendor agnostic, please rename the properties
> such as mediatek,irqs-map-range to something less brand-specific.
> The compatible string should be enough.

I can't find the suitable property in standard ones that match the custom
properties here.
And the vendor prefixed rule is described in [1].

The interrupt controller is first used in Mstar TV SoCs.
Now it's used in MTK TV and Sigmastar SoCs.
So I think Mstar prefixed would make more sense.
I will rename the driver into mstar-intc, and MTK will maintain this driver.

[1] https://www.kernel.org/doc/Documentation/devicetree/booting-without-of.txt
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel