[PATCH 0/2] Add MStar interrupt controller support

[PATCH 0/2] Add MStar interrupt controller support

[Mark-PK Tsai]

MStar contain a legacy interrupt controller that routes interrupts
to the GIC. SigmaStar and Mediatek TV SoCs also have this interrupt
controller IP.

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

 .../interrupt-controller/mstar,mst-intc.yaml  |  82 ++++++++
 drivers/irqchip/Kconfig                       |   7 +
 drivers/irqchip/Makefile                      |   1 +
 drivers/irqchip/irq-mst-intc.c                | 195 ++++++++++++++++++
 4 files changed, 285 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mstar,mst-intc.yaml
 create mode 100644 drivers/irqchip/irq-mst-intc.c

-- 
2.18.0

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

[Mark-PK Tsai]

Add MStar 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-mst-intc.c | 195 +++++++++++++++++++++++++++++++++
 3 files changed, 203 insertions(+)
 create mode 100644 drivers/irqchip/irq-mst-intc.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index bb70b7177f94..c3a9d880a4ea 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -571,4 +571,11 @@ config LOONGSON_PCH_MSI
 	help
 	  Support for the Loongson PCH MSI Controller.
 
+config MST_IRQ
+	bool "MStar Interrupt Controller"
+	select IRQ_DOMAIN
+	select IRQ_DOMAIN_HIERARCHY
+	help
+	  Support MStar Interrupt Controller.
+
 endmenu
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 133f9c45744a..e2688a62403e 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_MST_IRQ)			+= irq-mst-intc.o
diff --git a/drivers/irqchip/irq-mst-intc.c b/drivers/irqchip/irq-mst-intc.c
new file mode 100644
index 000000000000..38d567741860
--- /dev/null
+++ b/drivers/irqchip/irq-mst-intc.c
@@ -0,0 +1,195 @@
+// 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>
+#include <linux/spinlock.h>
+
+#define INTC_MASK	0x0
+#define INTC_EOI	0x20
+
+struct mst_intc_chip_data {
+	raw_spinlock_t lock;
+	unsigned int irq_start, nr_irqs;
+	void __iomem *base;
+	bool no_eoi;
+};
+
+static void mst_poke_irq(struct irq_data *d, u32 offset)
+{
+	irq_hw_number_t hwirq = irqd_to_hwirq(d);
+	struct mst_intc_chip_data *cd = irq_data_get_irq_chip_data(d);
+	u16 val, mask;
+
+	mask = 1 << (hwirq % 16);
+	offset += (hwirq / 16) * 4;
+
+	raw_spin_lock(&cd->lock);
+	val = readw_relaxed(cd->base + offset) | mask;
+	writew_relaxed(val, cd->base + offset);
+	raw_spin_unlock(&cd->lock);
+}
+
+static void mst_clear_irq(struct irq_data *d, u32 offset)
+{
+	irq_hw_number_t hwirq = irqd_to_hwirq(d);
+	struct mst_intc_chip_data *cd = irq_data_get_irq_chip_data(d);
+	u16 val, mask;
+
+	mask = 1 << (hwirq % 16);
+	offset += (hwirq / 16) * 4;
+
+	raw_spin_lock(&cd->lock);
+	val = readw_relaxed(cd->base + offset) & ~mask;
+	writew_relaxed(val, cd->base + offset);
+	raw_spin_unlock(&cd->lock);
+}
+
+static void mst_intc_mask_irq(struct irq_data *d)
+{
+	mst_poke_irq(d, INTC_MASK);
+	irq_chip_mask_parent(d);
+}
+
+static void mst_intc_unmask_irq(struct irq_data *d)
+{
+	mst_clear_irq(d, INTC_MASK);
+	irq_chip_unmask_parent(d);
+}
+
+static void mst_intc_eoi_irq(struct irq_data *d)
+{
+	struct mst_intc_chip_data *cd = irq_data_get_irq_chip_data(d);
+
+	if (!cd->no_eoi)
+		mst_poke_irq(d, INTC_EOI);
+
+	irq_chip_eoi_parent(d);
+}
+
+static struct irq_chip mst_intc_chip = {
+	.name			= "mst-intc",
+	.irq_mask		= mst_intc_mask_irq,
+	.irq_unmask		= mst_intc_unmask_irq,
+	.irq_eoi		= mst_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,
+	.irq_retrigger		= irq_chip_retrigger_hierarchy,
+	.flags			= IRQCHIP_SET_TYPE_MASKED |
+				  IRQCHIP_SKIP_SET_WAKE |
+				  IRQCHIP_MASK_ON_SUSPEND,
+};
+
+static int mst_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 mst_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 mst_intc_chip_data *cd = (struct mst_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,
+					      &mst_intc_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 mst_intc_domain_ops = {
+	.translate	= mst_intc_domain_translate,
+	.alloc		= mst_intc_domain_alloc,
+	.free		= irq_domain_free_irqs_common,
+};
+
+int __init
+mst_intc_of_init(struct device_node *dn, struct device_node *parent)
+{
+	struct irq_domain *domain, *domain_parent;
+	struct mst_intc_chip_data *cd;
+	unsigned int irq_start, irq_end;
+
+	domain_parent = irq_find_host(parent);
+	if (!domain_parent) {
+		pr_err("mst-intc: interrupt-parent not found\n");
+		return -EINVAL;
+	}
+
+	if (of_property_read_u32_index(dn, "mstar,irqs-map-range", 0, &irq_start) ||
+	    of_property_read_u32_index(dn, "mstar,irqs-map-range", 1, &irq_end))
+		return -EINVAL;
+
+	cd = kzalloc(sizeof(*cd), GFP_KERNEL);
+	if (!cd)
+		return -ENOMEM;
+
+	cd->base = of_iomap(dn, 0);
+	if (!cd->base) {
+		kfree(cd);
+		return -ENOMEM;
+	}
+
+	cd->no_eoi = of_property_read_bool(dn, "mstar,intc-no-eoi");
+	raw_spin_lock_init(&cd->lock);
+	cd->irq_start = irq_start;
+	cd->nr_irqs = irq_end - irq_start + 1;
+	domain = irq_domain_add_hierarchy(domain_parent, 0, cd->nr_irqs, dn,
+					  &mst_intc_domain_ops, cd);
+	if (!domain) {
+		iounmap(cd->base);
+		kfree(cd);
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+IRQCHIP_DECLARE(mst_intc, "mstar,mst-intc", mst_intc_of_init);
-- 
2.18.0

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

[Mark-PK Tsai]

Add binding for MStar interrupt controller.

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

diff --git a/Documentation/devicetree/bindings/interrupt-controller/mstar,mst-intc.yaml b/Documentation/devicetree/bindings/interrupt-controller/mstar,mst-intc.yaml
new file mode 100644
index 000000000000..6e383315e529
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/mstar,mst-intc.yaml
@@ -0,0 +1,82 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/interrupt-controller/mstar,mst-intc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MStar Interrupt Controller
+
+maintainers:
+  - Mark-PK Tsai <mark-pk.tsai@mediatek.com>
+
+description: |+
+  MStar, SigmaStar and Mediatek DTV SoCs contain multiple legacy
+  interrupt controllers that routes interrupts to the GIC.
+
+  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: mstar,mst-intc
+      - enum:
+          - mediatek,mt58xx-intc
+
+  interrupt-controller: true
+
+  "#address-cells":
+    enum: [ 0, 1, 2 ]
+
+  "#size-cells":
+    enum: [ 1, 2 ]
+
+  "#interrupt-cells":
+    const: 3
+    description: |
+      Use the same format as specified by GIC in arm,gic.yaml.
+
+  reg:
+    description: |
+      Physical base address of the mstar interrupt controller
+      registers and length of memory mapped region.
+    minItems: 1
+
+  mstar,irqs-map-range:
+    description: |
+      The range of parent interrupt controller's interrupt lines
+      that are hardwired to mstar interrupt controller.
+    $ref: /schemas/types.yaml#/definitions/uint32-matrix
+    items:
+      minItems: 2
+      maxItems: 2
+
+  mstar,intc-no-eoi:
+    description: |
+      Mark this controller has no End Of Interrupt(EOI) implementation.
+      This is a empty, boolean property.
+    type: boolean
+
+required:
+  - compatible
+  - reg
+  - mstar,irqs-map-range
+
+additionalProperties: false
+
+examples:
+  - |
+    mst_intc0: interrupt-controller@1f2032d0 {
+      compatible = "mstar,mst-intc", "mediatek,mt58xx-intc";
+      interrupt-controller;
+      #interrupt-cells = <3>;
+      #address-cells = <1>;
+      #size-cells = <1>;
+      interrupt-parent = <&gic>;
+      reg = <0x1f2032d0 0x30>;
+      mstar,irqs-map-range = <0 63>;
+    };
+...
-- 
2.18.0

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

[Marc Zyngier]

On 2020-08-19 04:42, Mark-PK Tsai wrote:
> Add MStar 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-mst-intc.c | 195 +++++++++++++++++++++++++++++++++
>  3 files changed, 203 insertions(+)
>  create mode 100644 drivers/irqchip/irq-mst-intc.c
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index bb70b7177f94..c3a9d880a4ea 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -571,4 +571,11 @@ config LOONGSON_PCH_MSI
>  	help
>  	  Support for the Loongson PCH MSI Controller.
> 
> +config MST_IRQ
> +	bool "MStar Interrupt Controller"
> +	select IRQ_DOMAIN
> +	select IRQ_DOMAIN_HIERARCHY
> +	help
> +	  Support MStar Interrupt Controller.

What selects it? Can it have a default for the relevant platforms?

> +
>  endmenu
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 133f9c45744a..e2688a62403e 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_MST_IRQ)			+= irq-mst-intc.o
> diff --git a/drivers/irqchip/irq-mst-intc.c 
> b/drivers/irqchip/irq-mst-intc.c
> new file mode 100644
> index 000000000000..38d567741860
> --- /dev/null
> +++ b/drivers/irqchip/irq-mst-intc.c
> @@ -0,0 +1,195 @@
> +// 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>
> +#include <linux/spinlock.h>
> +
> +#define INTC_MASK	0x0
> +#define INTC_EOI	0x20
> +
> +struct mst_intc_chip_data {
> +	raw_spinlock_t lock;
> +	unsigned int irq_start, nr_irqs;
> +	void __iomem *base;
> +	bool no_eoi;

nit: please align the fields of the data structure on a vertical line.

> +};
> +
> +static void mst_poke_irq(struct irq_data *d, u32 offset)

Given that this is supposed to be the opposite of "clear", this
should be mst_set_irq().

> +{
> +	irq_hw_number_t hwirq = irqd_to_hwirq(d);
> +	struct mst_intc_chip_data *cd = irq_data_get_irq_chip_data(d);
> +	u16 val, mask;
> +
> +	mask = 1 << (hwirq % 16);
> +	offset += (hwirq / 16) * 4;
> +
> +	raw_spin_lock(&cd->lock);
> +	val = readw_relaxed(cd->base + offset) | mask;
> +	writew_relaxed(val, cd->base + offset);
> +	raw_spin_unlock(&cd->lock);

Small improvement, but you still miss the fact that a masking
operation can also occur from an interrupt handler, and this
will cause a deadlock. You need to disable interrupts as well.

> +}
> +
> +static void mst_clear_irq(struct irq_data *d, u32 offset)
> +{
> +	irq_hw_number_t hwirq = irqd_to_hwirq(d);
> +	struct mst_intc_chip_data *cd = irq_data_get_irq_chip_data(d);
> +	u16 val, mask;
> +
> +	mask = 1 << (hwirq % 16);
> +	offset += (hwirq / 16) * 4;
> +
> +	raw_spin_lock(&cd->lock);
> +	val = readw_relaxed(cd->base + offset) & ~mask;
> +	writew_relaxed(val, cd->base + offset);
> +	raw_spin_unlock(&cd->lock);
> +}
> +
> +static void mst_intc_mask_irq(struct irq_data *d)
> +{
> +	mst_poke_irq(d, INTC_MASK);
> +	irq_chip_mask_parent(d);
> +}
> +
> +static void mst_intc_unmask_irq(struct irq_data *d)
> +{
> +	mst_clear_irq(d, INTC_MASK);
> +	irq_chip_unmask_parent(d);
> +}
> +
> +static void mst_intc_eoi_irq(struct irq_data *d)
> +{
> +	struct mst_intc_chip_data *cd = irq_data_get_irq_chip_data(d);
> +
> +	if (!cd->no_eoi)
> +		mst_poke_irq(d, INTC_EOI);
> +
> +	irq_chip_eoi_parent(d);
> +}
> +
> +static struct irq_chip mst_intc_chip = {
> +	.name			= "mst-intc",
> +	.irq_mask		= mst_intc_mask_irq,
> +	.irq_unmask		= mst_intc_unmask_irq,
> +	.irq_eoi		= mst_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,
> +	.irq_retrigger		= irq_chip_retrigger_hierarchy,
> +	.flags			= IRQCHIP_SET_TYPE_MASKED |
> +				  IRQCHIP_SKIP_SET_WAKE |
> +				  IRQCHIP_MASK_ON_SUSPEND,
> +};
> +
> +static int mst_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 mst_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 mst_intc_chip_data *cd = (struct mst_intc_chip_data
> *)domain->host_data;

No cast necessary here.

> +
> +	/* 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)

This condition is bogus, as it doesn't take into account the nr_irqs
parameter.

> +		return -EINVAL;
> +
> +	hwirq = fwspec->param[1];
> +	for (i = 0; i < nr_irqs; i++)
> +		irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
> +					      &mst_intc_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 mst_intc_domain_ops = {
> +	.translate	= mst_intc_domain_translate,
> +	.alloc		= mst_intc_domain_alloc,
> +	.free		= irq_domain_free_irqs_common,
> +};
> +
> +int __init
> +mst_intc_of_init(struct device_node *dn, struct device_node *parent)
> +{
> +	struct irq_domain *domain, *domain_parent;
> +	struct mst_intc_chip_data *cd;
> +	unsigned int irq_start, irq_end;

unsigned int != u32.

> +
> +	domain_parent = irq_find_host(parent);
> +	if (!domain_parent) {
> +		pr_err("mst-intc: interrupt-parent not found\n");
> +		return -EINVAL;
> +	}
> +
> +	if (of_property_read_u32_index(dn, "mstar,irqs-map-range", 0, 
> &irq_start) ||
> +	    of_property_read_u32_index(dn, "mstar,irqs-map-range", 1, 
> &irq_end))
> +		return -EINVAL;
> +
> +	cd = kzalloc(sizeof(*cd), GFP_KERNEL);
> +	if (!cd)
> +		return -ENOMEM;
> +
> +	cd->base = of_iomap(dn, 0);
> +	if (!cd->base) {
> +		kfree(cd);
> +		return -ENOMEM;
> +	}
> +
> +	cd->no_eoi = of_property_read_bool(dn, "mstar,intc-no-eoi");
> +	raw_spin_lock_init(&cd->lock);
> +	cd->irq_start = irq_start;
> +	cd->nr_irqs = irq_end - irq_start + 1;
> +	domain = irq_domain_add_hierarchy(domain_parent, 0, cd->nr_irqs, dn,
> +					  &mst_intc_domain_ops, cd);
> +	if (!domain) {
> +		iounmap(cd->base);
> +		kfree(cd);
> +		return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +
> +IRQCHIP_DECLARE(mst_intc, "mstar,mst-intc", mst_intc_of_init);

Thanks,

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

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

[Mark-PK Tsai]

From: Marc Zyngier <maz@kernel.org>

> > +
> > +static int mst_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 mst_intc_chip_data *cd = (struct mst_intc_chip_data
> > *)domain->host_data;
> 
> No cast necessary here.
> 
> > +
> > +	/* 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)
> 
> This condition is bogus, as it doesn't take into account the nr_irqs
> parameter.
> 


The hwirq number need to be in the irq map range. (property: mstar,irqs-map-range)
If it's not, it must be incorrect configuration.
So how about use the condition as following?

if (hwirq >= cd->nr_irqs)
	return -EINVAL;

> > +		return -EINVAL;
> > +
> > +	hwirq = fwspec->param[1];
> > +	for (i = 0; i < nr_irqs; i++)
> > +		irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
> > +					      &mst_intc_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 mst_intc_domain_ops = {
> > +	.translate	= mst_intc_domain_translate,
> > +	.alloc		= mst_intc_domain_alloc,
> > +	.free		= irq_domain_free_irqs_common,
> > +};
> > +
> > +int __init
> > +mst_intc_of_init(struct device_node *dn, struct device_node *parent)
> > +{
> > +	struct irq_domain *domain, *domain_parent;
> > +	struct mst_intc_chip_data *cd;
> > +	unsigned int irq_start, irq_end;
> 
> unsigned int != u32.
> 
> > +
> > +	domain_parent = irq_find_host(parent);
> > +	if (!domain_parent) {
> > +		pr_err("mst-intc: interrupt-parent not found\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (of_property_read_u32_index(dn, "mstar,irqs-map-range", 0, 
> > &irq_start) ||
> > +	    of_property_read_u32_index(dn, "mstar,irqs-map-range", 1, 
> > &irq_end))
> > +		return -EINVAL;
> > +
> > +	cd = kzalloc(sizeof(*cd), GFP_KERNEL);
> > +	if (!cd)
> > +		return -ENOMEM;
> > +
> > +	cd->base = of_iomap(dn, 0);
> > +	if (!cd->base) {
> > +		kfree(cd);
> > +		return -ENOMEM;
> > +	}
> > +
> > +	cd->no_eoi = of_property_read_bool(dn, "mstar,intc-no-eoi");
> > +	raw_spin_lock_init(&cd->lock);
> > +	cd->irq_start = irq_start;
> > +	cd->nr_irqs = irq_end - irq_start + 1;
> > +	domain = irq_domain_add_hierarchy(domain_parent, 0, cd->nr_irqs, dn,
> > +					  &mst_intc_domain_ops, cd);
> > +	if (!domain) {
> > +		iounmap(cd->base);
> > +		kfree(cd);
> > +		return -ENOMEM;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +IRQCHIP_DECLARE(mst_intc, "mstar,mst-intc", mst_intc_of_init);
> 

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

[Marc Zyngier]

On 2020-08-19 14:31, Mark-PK Tsai wrote:
> From: Marc Zyngier <maz@kernel.org>
> 
>> > +
>> > +static int mst_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 mst_intc_chip_data *cd = (struct mst_intc_chip_data
>> > *)domain->host_data;
>> 
>> No cast necessary here.
>> 
>> > +
>> > +	/* 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)
>> 
>> This condition is bogus, as it doesn't take into account the nr_irqs
>> parameter.
>> 
> 
> 
> The hwirq number need to be in the irq map range. (property:
> mstar,irqs-map-range)
> If it's not, it must be incorrect configuration.

I agree. And since you are checking whether the configuration is 
correct,
it'd better be completely correct.

> So how about use the condition as following?
> 
> if (hwirq >= cd->nr_irqs)
> 	return -EINVAL;

Again, this says nothing of the validity of (hwirq + nr_irqs - 1)...

> 
>> > +		return -EINVAL;
>> > +
>> > +	hwirq = fwspec->param[1];
>> > +	for (i = 0; i < nr_irqs; i++)
>> > +		irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
>> > +					      &mst_intc_chip,
>> > +					      domain->host_data);

... which you are using here.

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

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

[Mark-PK Tsai]

From: Marc Zyngier <maz@kernel.org>

>On 2020-08-19 14:31, Mark-PK Tsai wrote:
>> From: Marc Zyngier <maz@kernel.org>
>> 
>>> > +
>>> > +static int mst_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 mst_intc_chip_data *cd = (struct mst_intc_chip_data
>>> > *)domain->host_data;
>>> 
>>> No cast necessary here.
>>> 
>>> > +
>>> > +	/* 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)
>>> 
>>> This condition is bogus, as it doesn't take into account the nr_irqs
>>> parameter.
>>> 
>> 
>> 
>> The hwirq number need to be in the irq map range. (property:
>> mstar,irqs-map-range)
>> If it's not, it must be incorrect configuration.
>
>I agree. And since you are checking whether the configuration is 
>correct,
>it'd better be completely correct.
>
>> So how about use the condition as following?
>> 
>> if (hwirq >= cd->nr_irqs)
>> 	return -EINVAL;
>
>Again, this says nothing of the validity of (hwirq + nr_irqs - 1)...
>

How about move this to mst_intc_domain_translate? Then all the irq_fwspec
point to domain mst_intc should be valid.

The mst_intc_domain_translate will be as following:

static int mst_intc_domain_translate(struct irq_domain *d,
				     struct irq_fwspec *fwspec,
				     unsigned long *hwirq,
				     unsigned int *type)
{
	struct mst_intc_chip_data *cd = d->host_data;

	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;

		if (fwspec->param[1] >= cd->nr_irqs)
			return -EINVAL;

		*hwirq = fwspec->param[1];
		*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
		return 0;
	}

	return -EINVAL;
}


>> 
>>> > +		return -EINVAL;
>>> > +
>>> > +	hwirq = fwspec->param[1];
>>> > +	for (i = 0; i < nr_irqs; i++)
>>> > +		irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
>>> > +					      &mst_intc_chip,
>>> > +					      domain->host_data);

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

[Marc Zyngier]

On 2020-08-19 15:55, Mark-PK Tsai wrote:
> From: Marc Zyngier <maz@kernel.org>
> 
>> On 2020-08-19 14:31, Mark-PK Tsai wrote:
>>> From: Marc Zyngier <maz@kernel.org>
>>> 
>>>> > +
>>>> > +static int mst_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 mst_intc_chip_data *cd = (struct mst_intc_chip_data
>>>> > *)domain->host_data;
>>>> 
>>>> No cast necessary here.
>>>> 
>>>> > +
>>>> > +	/* 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)
>>>> 
>>>> This condition is bogus, as it doesn't take into account the nr_irqs
>>>> parameter.
>>>> 
>>> 
>>> 
>>> The hwirq number need to be in the irq map range. (property:
>>> mstar,irqs-map-range)
>>> If it's not, it must be incorrect configuration.
>> 
>> I agree. And since you are checking whether the configuration is
>> correct,
>> it'd better be completely correct.
>> 
>>> So how about use the condition as following?
>>> 
>>> if (hwirq >= cd->nr_irqs)
>>> 	return -EINVAL;
>> 
>> Again, this says nothing of the validity of (hwirq + nr_irqs - 1)...
>> 
> 
> How about move this to mst_intc_domain_translate? Then all the 
> irq_fwspec
> point to domain mst_intc should be valid.
> 
> The mst_intc_domain_translate will be as following:
> 
> static int mst_intc_domain_translate(struct irq_domain *d,
> 				     struct irq_fwspec *fwspec,
> 				     unsigned long *hwirq,
> 				     unsigned int *type)
> {
> 	struct mst_intc_chip_data *cd = d->host_data;
> 
> 	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;
> 
> 		if (fwspec->param[1] >= cd->nr_irqs)
> 			return -EINVAL;
> 
> 		*hwirq = fwspec->param[1];
> 		*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
> 		return 0;
> 	}
> 
> 	return -EINVAL;
> }

It would make more sense.

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

[PATCH v2 1/2] irqchip: irq-mst: Add MStar interrupt controller support

[Mark-PK Tsai]

Add MStar interrupt controller support using hierarchy irq
domain.

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

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index bb70b7177f94..0b5ae5fa0d3c 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -571,4 +571,12 @@ config LOONGSON_PCH_MSI
 	help
 	  Support for the Loongson PCH MSI Controller.
 
+config MST_IRQ
+	bool "MStar Interrupt Controller"
+	default ARCH_MEDIATEK
+	select IRQ_DOMAIN
+	select IRQ_DOMAIN_HIERARCHY
+	help
+	  Support MStar Interrupt Controller.
+
 endmenu
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 133f9c45744a..e2688a62403e 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_MST_IRQ)			+= irq-mst-intc.o
diff --git a/drivers/irqchip/irq-mst-intc.c b/drivers/irqchip/irq-mst-intc.c
new file mode 100644
index 000000000000..4be077591898
--- /dev/null
+++ b/drivers/irqchip/irq-mst-intc.c
@@ -0,0 +1,199 @@
+// 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>
+#include <linux/spinlock.h>
+
+#define INTC_MASK	0x0
+#define INTC_EOI	0x20
+
+struct mst_intc_chip_data {
+	raw_spinlock_t	lock;
+	unsigned int	irq_start, nr_irqs;
+	void __iomem	*base;
+	bool		no_eoi;
+};
+
+static void mst_set_irq(struct irq_data *d, u32 offset)
+{
+	irq_hw_number_t hwirq = irqd_to_hwirq(d);
+	struct mst_intc_chip_data *cd = irq_data_get_irq_chip_data(d);
+	u16 val, mask;
+	unsigned long flags;
+
+	mask = 1 << (hwirq % 16);
+	offset += (hwirq / 16) * 4;
+
+	raw_spin_lock_irqsave(&cd->lock, flags);
+	val = readw_relaxed(cd->base + offset) | mask;
+	writew_relaxed(val, cd->base + offset);
+	raw_spin_unlock_irqrestore(&cd->lock, flags);
+}
+
+static void mst_clear_irq(struct irq_data *d, u32 offset)
+{
+	irq_hw_number_t hwirq = irqd_to_hwirq(d);
+	struct mst_intc_chip_data *cd = irq_data_get_irq_chip_data(d);
+	u16 val, mask;
+	unsigned long flags;
+
+	mask = 1 << (hwirq % 16);
+	offset += (hwirq / 16) * 4;
+
+	raw_spin_lock_irqsave(&cd->lock, flags);
+	val = readw_relaxed(cd->base + offset) & ~mask;
+	writew_relaxed(val, cd->base + offset);
+	raw_spin_unlock_irqrestore(&cd->lock, flags);
+}
+
+static void mst_intc_mask_irq(struct irq_data *d)
+{
+	mst_set_irq(d, INTC_MASK);
+	irq_chip_mask_parent(d);
+}
+
+static void mst_intc_unmask_irq(struct irq_data *d)
+{
+	mst_clear_irq(d, INTC_MASK);
+	irq_chip_unmask_parent(d);
+}
+
+static void mst_intc_eoi_irq(struct irq_data *d)
+{
+	struct mst_intc_chip_data *cd = irq_data_get_irq_chip_data(d);
+
+	if (!cd->no_eoi)
+		mst_set_irq(d, INTC_EOI);
+
+	irq_chip_eoi_parent(d);
+}
+
+static struct irq_chip mst_intc_chip = {
+	.name			= "mst-intc",
+	.irq_mask		= mst_intc_mask_irq,
+	.irq_unmask		= mst_intc_unmask_irq,
+	.irq_eoi		= mst_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,
+	.irq_retrigger		= irq_chip_retrigger_hierarchy,
+	.flags			= IRQCHIP_SET_TYPE_MASKED |
+				  IRQCHIP_SKIP_SET_WAKE |
+				  IRQCHIP_MASK_ON_SUSPEND,
+};
+
+static int mst_intc_domain_translate(struct irq_domain *d,
+				     struct irq_fwspec *fwspec,
+				     unsigned long *hwirq,
+				     unsigned int *type)
+{
+	struct mst_intc_chip_data *cd = d->host_data;
+
+	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;
+
+		if (fwspec->param[1] >= cd->nr_irqs)
+			return -EINVAL;
+
+		*hwirq = fwspec->param[1];
+		*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static int mst_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 mst_intc_chip_data *cd = 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;
+
+	hwirq = fwspec->param[1];
+	for (i = 0; i < nr_irqs; i++)
+		irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
+					      &mst_intc_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 mst_intc_domain_ops = {
+	.translate	= mst_intc_domain_translate,
+	.alloc		= mst_intc_domain_alloc,
+	.free		= irq_domain_free_irqs_common,
+};
+
+int __init
+mst_intc_of_init(struct device_node *dn, struct device_node *parent)
+{
+	struct irq_domain *domain, *domain_parent;
+	struct mst_intc_chip_data *cd;
+	u32 irq_start, irq_end;
+
+	domain_parent = irq_find_host(parent);
+	if (!domain_parent) {
+		pr_err("mst-intc: interrupt-parent not found\n");
+		return -EINVAL;
+	}
+
+	if (of_property_read_u32_index(dn, "mstar,irqs-map-range", 0, &irq_start) ||
+	    of_property_read_u32_index(dn, "mstar,irqs-map-range", 1, &irq_end))
+		return -EINVAL;
+
+	cd = kzalloc(sizeof(*cd), GFP_KERNEL);
+	if (!cd)
+		return -ENOMEM;
+
+	cd->base = of_iomap(dn, 0);
+	if (!cd->base) {
+		kfree(cd);
+		return -ENOMEM;
+	}
+
+	cd->no_eoi = of_property_read_bool(dn, "mstar,intc-no-eoi");
+	raw_spin_lock_init(&cd->lock);
+	cd->irq_start = irq_start;
+	cd->nr_irqs = irq_end - irq_start + 1;
+	domain = irq_domain_add_hierarchy(domain_parent, 0, cd->nr_irqs, dn,
+					  &mst_intc_domain_ops, cd);
+	if (!domain) {
+		iounmap(cd->base);
+		kfree(cd);
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+IRQCHIP_DECLARE(mst_intc, "mstar,mst-intc", mst_intc_of_init);
-- 
2.18.0

Re: [PATCH v2 1/2] irqchip: irq-mst: Add MStar interrupt controller support

[Daniel Palmer]

Hi Mark-PK, Marc

I'm not sure this will be the final version but I'm going to try to
integrate this with my current MStar/SigmaStar tree over the weekend
and then I guess I can give this a tested-by?

Assuming this version or the next is acceptable can I just follow up
with a small patch to add the instances I need in my dtsi or should I
wait until it's merged before doing that?

Thanks,

Daniel

On Thu, 20 Aug 2020 at 00:38, Mark-PK Tsai <mark-pk.tsai@mediatek.com> wrote:
>
> Add MStar interrupt controller support using hierarchy irq
> domain.
>
> Signed-off-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
> ---
>  drivers/irqchip/Kconfig        |   8 ++
>  drivers/irqchip/Makefile       |   1 +
>  drivers/irqchip/irq-mst-intc.c | 199 +++++++++++++++++++++++++++++++++
>  3 files changed, 208 insertions(+)
>  create mode 100644 drivers/irqchip/irq-mst-intc.c
>
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index bb70b7177f94..0b5ae5fa0d3c 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -571,4 +571,12 @@ config LOONGSON_PCH_MSI
>         help
>           Support for the Loongson PCH MSI Controller.
>
> +config MST_IRQ
> +       bool "MStar Interrupt Controller"
> +       default ARCH_MEDIATEK
> +       select IRQ_DOMAIN
> +       select IRQ_DOMAIN_HIERARCHY
> +       help
> +         Support MStar Interrupt Controller.
> +
>  endmenu
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 133f9c45744a..e2688a62403e 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_MST_IRQ)                  += irq-mst-intc.o
> diff --git a/drivers/irqchip/irq-mst-intc.c b/drivers/irqchip/irq-mst-intc.c
> new file mode 100644
> index 000000000000..4be077591898
> --- /dev/null
> +++ b/drivers/irqchip/irq-mst-intc.c
> @@ -0,0 +1,199 @@
> +// 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>
> +#include <linux/spinlock.h>
> +
> +#define INTC_MASK      0x0
> +#define INTC_EOI       0x20
> +
> +struct mst_intc_chip_data {
> +       raw_spinlock_t  lock;
> +       unsigned int    irq_start, nr_irqs;
> +       void __iomem    *base;
> +       bool            no_eoi;
> +};
> +
> +static void mst_set_irq(struct irq_data *d, u32 offset)
> +{
> +       irq_hw_number_t hwirq = irqd_to_hwirq(d);
> +       struct mst_intc_chip_data *cd = irq_data_get_irq_chip_data(d);
> +       u16 val, mask;
> +       unsigned long flags;
> +
> +       mask = 1 << (hwirq % 16);
> +       offset += (hwirq / 16) * 4;
> +
> +       raw_spin_lock_irqsave(&cd->lock, flags);
> +       val = readw_relaxed(cd->base + offset) | mask;
> +       writew_relaxed(val, cd->base + offset);
> +       raw_spin_unlock_irqrestore(&cd->lock, flags);
> +}
> +
> +static void mst_clear_irq(struct irq_data *d, u32 offset)
> +{
> +       irq_hw_number_t hwirq = irqd_to_hwirq(d);
> +       struct mst_intc_chip_data *cd = irq_data_get_irq_chip_data(d);
> +       u16 val, mask;
> +       unsigned long flags;
> +
> +       mask = 1 << (hwirq % 16);
> +       offset += (hwirq / 16) * 4;
> +
> +       raw_spin_lock_irqsave(&cd->lock, flags);
> +       val = readw_relaxed(cd->base + offset) & ~mask;
> +       writew_relaxed(val, cd->base + offset);
> +       raw_spin_unlock_irqrestore(&cd->lock, flags);
> +}
> +
> +static void mst_intc_mask_irq(struct irq_data *d)
> +{
> +       mst_set_irq(d, INTC_MASK);
> +       irq_chip_mask_parent(d);
> +}
> +
> +static void mst_intc_unmask_irq(struct irq_data *d)
> +{
> +       mst_clear_irq(d, INTC_MASK);
> +       irq_chip_unmask_parent(d);
> +}
> +
> +static void mst_intc_eoi_irq(struct irq_data *d)
> +{
> +       struct mst_intc_chip_data *cd = irq_data_get_irq_chip_data(d);
> +
> +       if (!cd->no_eoi)
> +               mst_set_irq(d, INTC_EOI);
> +
> +       irq_chip_eoi_parent(d);
> +}
> +
> +static struct irq_chip mst_intc_chip = {
> +       .name                   = "mst-intc",
> +       .irq_mask               = mst_intc_mask_irq,
> +       .irq_unmask             = mst_intc_unmask_irq,
> +       .irq_eoi                = mst_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,
> +       .irq_retrigger          = irq_chip_retrigger_hierarchy,
> +       .flags                  = IRQCHIP_SET_TYPE_MASKED |
> +                                 IRQCHIP_SKIP_SET_WAKE |
> +                                 IRQCHIP_MASK_ON_SUSPEND,
> +};
> +
> +static int mst_intc_domain_translate(struct irq_domain *d,
> +                                    struct irq_fwspec *fwspec,
> +                                    unsigned long *hwirq,
> +                                    unsigned int *type)
> +{
> +       struct mst_intc_chip_data *cd = d->host_data;
> +
> +       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;
> +
> +               if (fwspec->param[1] >= cd->nr_irqs)
> +                       return -EINVAL;
> +
> +               *hwirq = fwspec->param[1];
> +               *type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
> +               return 0;
> +       }
> +
> +       return -EINVAL;
> +}
> +
> +static int mst_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 mst_intc_chip_data *cd = 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;
> +
> +       hwirq = fwspec->param[1];
> +       for (i = 0; i < nr_irqs; i++)
> +               irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
> +                                             &mst_intc_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 mst_intc_domain_ops = {
> +       .translate      = mst_intc_domain_translate,
> +       .alloc          = mst_intc_domain_alloc,
> +       .free           = irq_domain_free_irqs_common,
> +};
> +
> +int __init
> +mst_intc_of_init(struct device_node *dn, struct device_node *parent)
> +{
> +       struct irq_domain *domain, *domain_parent;
> +       struct mst_intc_chip_data *cd;
> +       u32 irq_start, irq_end;
> +
> +       domain_parent = irq_find_host(parent);
> +       if (!domain_parent) {
> +               pr_err("mst-intc: interrupt-parent not found\n");
> +               return -EINVAL;
> +       }
> +
> +       if (of_property_read_u32_index(dn, "mstar,irqs-map-range", 0, &irq_start) ||
> +           of_property_read_u32_index(dn, "mstar,irqs-map-range", 1, &irq_end))
> +               return -EINVAL;
> +
> +       cd = kzalloc(sizeof(*cd), GFP_KERNEL);
> +       if (!cd)
> +               return -ENOMEM;
> +
> +       cd->base = of_iomap(dn, 0);
> +       if (!cd->base) {
> +               kfree(cd);
> +               return -ENOMEM;
> +       }
> +
> +       cd->no_eoi = of_property_read_bool(dn, "mstar,intc-no-eoi");
> +       raw_spin_lock_init(&cd->lock);
> +       cd->irq_start = irq_start;
> +       cd->nr_irqs = irq_end - irq_start + 1;
> +       domain = irq_domain_add_hierarchy(domain_parent, 0, cd->nr_irqs, dn,
> +                                         &mst_intc_domain_ops, cd);
> +       if (!domain) {
> +               iounmap(cd->base);
> +               kfree(cd);
> +               return -ENOMEM;
> +       }
> +
> +       return 0;
> +}
> +
> +IRQCHIP_DECLARE(mst_intc, "mstar,mst-intc", mst_intc_of_init);
> --
> 2.18.0

Re: [PATCH v2 1/2] irqchip: irq-mst: Add MStar interrupt controller support

[Marc Zyngier]

On 2020-08-20 13:36, Daniel Palmer wrote:
> Hi Mark-PK, Marc
> 
> I'm not sure this will be the final version but I'm going to try to
> integrate this with my current MStar/SigmaStar tree over the weekend
> and then I guess I can give this a tested-by?

That'd be good.

> Assuming this version or the next is acceptable can I just follow up
> with a small patch to add the instances I need in my dtsi or should I
> wait until it's merged before doing that?

No need to wait, although the platform-specific details should go
via the arm-soc tree.

I'm not going to review the new version before next week anyway
(I'm making a point in reviewing any given series at most once
a week).

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

Re: [PATCH v2 1/2] irqchip: irq-mst: Add MStar interrupt controller support

[Daniel Palmer]

Hi Mark-PK,

On Thu, 20 Aug 2020 at 00:38, Mark-PK Tsai <mark-pk.tsai@mediatek.com> wrote:
>
> Add MStar interrupt controller support using hierarchy irq
> domain.
>
> Signed-off-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>

I've integrated this version into my MStar/SigmaStar tree and tested
on an MStar MSC313E
based board (BreadBee) and I'm happy to say it seems to be working:

$ cat /proc/interrupts
          CPU0
17:       1219     GIC-0  29 Level     arch_timer
18:          0     GIC-0  30 Level     arch_timer
21:          0     GIC-0  42 Level     arm-pmu
24:          0  mst-intc  44 Level     1f002400.rtc
30:          0  mst-intc   2 Level     1f006000.wdt
31:          0  mst-intc   0 Level     1f006040.timer
32:          0  mst-intc   1 Level     1f006080.timer
33:          0  mst-intc  12 Level     1f0060c0.timer
34:          0  mst-intc  40 Level     1f200400.bdma
35:       3977  mst-intc  41 Level     1f200400.bdma
37:        196  mst-intc  34 Level     ttyS0
40:          0  mst-intc  30 Level     soc:usbphy@0
<snip>

So here's my tested by:

Tested-by: Daniel Palmer <daniel@thingy.jp>

I don't think your series contained an update to MAINTAINERS.
If/when you add this could you add my address above as a reviewer so
I'm in the loop if anyone makes changes to this going forward?

Thanks,

Daniel

Re: [PATCH v2 1/2] irqchip: irq-mst: Add MStar interrupt controller support

[Mark-PK Tsai]

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

>Hi Mark-PK,
>
>On Thu, 20 Aug 2020 at 00:38, Mark-PK Tsai <mark-pk.tsai@mediatek.com> wrote:
>>
>> Add MStar interrupt controller support using hierarchy irq
>> domain.
>>
>> Signed-off-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
>
>I've integrated this version into my MStar/SigmaStar tree and tested
>on an MStar MSC313E
>based board (BreadBee) and I'm happy to say it seems to be working:
>
>$ cat /proc/interrupts
>          CPU0
>17:       1219     GIC-0  29 Level     arch_timer
>18:          0     GIC-0  30 Level     arch_timer
>21:          0     GIC-0  42 Level     arm-pmu
>24:          0  mst-intc  44 Level     1f002400.rtc
>30:          0  mst-intc   2 Level     1f006000.wdt
>31:          0  mst-intc   0 Level     1f006040.timer
>32:          0  mst-intc   1 Level     1f006080.timer
>33:          0  mst-intc  12 Level     1f0060c0.timer
>34:          0  mst-intc  40 Level     1f200400.bdma
>35:       3977  mst-intc  41 Level     1f200400.bdma
>37:        196  mst-intc  34 Level     ttyS0
>40:          0  mst-intc  30 Level     soc:usbphy@0
><snip>
>
>So here's my tested by:
>
>Tested-by: Daniel Palmer <daniel@thingy.jp>
>

Thanks for your test.

>I don't think your series contained an update to MAINTAINERS.
>If/when you add this could you add my address above as a reviewer so
>I'm in the loop if anyone makes changes to this going forward?
>

Sure, I will add your address in there. :)
Can I just add a patch in this thread which only contain MAINTAINERS update?

[PATCH] MAINTAINERS: Add maintenance information for MStar Interrupt Controller

[Mark-PK Tsai]

Add entry for MStar Interrupt Controller.

Signed-off-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
---
 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index deaafb617361..8ab08fccd915 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11754,6 +11754,13 @@ Q:	http://patchwork.linuxtv.org/project/linux-media/list/
 T:	git git://linuxtv.org/anttip/media_tree.git
 F:	drivers/media/usb/msi2500/
 
+MSTAR INTERRUPT CONTROLLER DRIVER
+M:	Mark-PK Tsai <mark-pk.tsai@mediatek.com>
+M:	Daniel Palmer <daniel@thingy.jp>
+S:	Maintained
+F:	Documentation/devicetree/bindings/interrupt-controller/mstar,mst-intc.yaml
+F:	drivers/irqchip/irq-mst-intc.c
+
 MSYSTEMS DISKONCHIP G3 MTD DRIVER
 M:	Robert Jarzmik <robert.jarzmik@free.fr>
 L:	linux-mtd@lists.infradead.org
-- 
2.18.0

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

[Rob Herring]

On Wed, Aug 19, 2020 at 11:42:31AM +0800, Mark-PK Tsai wrote:
> Add binding for MStar interrupt controller.
> 
> Signed-off-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
> ---
>  .../interrupt-controller/mstar,mst-intc.yaml  | 82 +++++++++++++++++++
>  1 file changed, 82 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mstar,mst-intc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/mstar,mst-intc.yaml b/Documentation/devicetree/bindings/interrupt-controller/mstar,mst-intc.yaml
> new file mode 100644
> index 000000000000..6e383315e529
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/mstar,mst-intc.yaml
> @@ -0,0 +1,82 @@
> +# SPDX-License-Identifier: GPL-2.0

Dual license new bindings.

(GPL-2.0-only OR BSD-2-Clause)

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/interrupt-controller/mstar,mst-intc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MStar Interrupt Controller
> +
> +maintainers:
> +  - Mark-PK Tsai <mark-pk.tsai@mediatek.com>
> +
> +description: |+
> +  MStar, SigmaStar and Mediatek DTV SoCs contain multiple legacy
> +  interrupt controllers that routes interrupts to the GIC.
> +
> +  The HW block exposes a number of interrupt controllers, each
> +  can support up to 64 interrupts.
> +
> +allOf:
> +  - $ref: /schemas/interrupt-controller.yaml#

Drop this. It is applied based on node name matching already.

> +
> +properties:
> +  compatible:
> +    items:
> +      - const: mstar,mst-intc
> +      - enum:
> +          - mediatek,mt58xx-intc

Normally, the 1st entry would be enum as that's where you'd add new 
compatibles (as the fallback is constant). But if you don't forsee any 
additions, just make both 'const'

> +
> +  interrupt-controller: true
> +
> +  "#address-cells":
> +    enum: [ 0, 1, 2 ]

This would normally be 0 in an interrupt controller. It's only relevant 
if you have an 'interrupt-map' which this is the parent for.

> +  "#size-cells":
> +    enum: [ 1, 2 ]

And this should be dropped.

> +
> +  "#interrupt-cells":
> +    const: 3
> +    description: |
> +      Use the same format as specified by GIC in arm,gic.yaml.

That's odd. You have the same SPI and PPI stuff?

> +
> +  reg:
> +    description: |
> +      Physical base address of the mstar interrupt controller
> +      registers and length of memory mapped region.

Drop this. That's every 'reg' property.

> +    minItems: 1

maxItems is more logical.

> +
> +  mstar,irqs-map-range:
> +    description: |
> +      The range of parent interrupt controller's interrupt lines
> +      that are hardwired to mstar interrupt controller.

Is this <start size> or <start end>?

Really, this should just use 'interrupts' even though that's a bit 
verbose. Or be implied by the compatible string. What's the maximum 
number of parent interrupts?

In any case, we really need to stop having vendor specific properties 
for this.

> +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
> +    items:
> +      minItems: 2
> +      maxItems: 2
> +
> +  mstar,intc-no-eoi:
> +    description: |

Don't need '|' if there's no formatting.

> +      Mark this controller has no End Of Interrupt(EOI) implementation.
> +      This is a empty, boolean property.

You can drop this line. The schema says this.

> +    type: boolean
> +
> +required:
> +  - compatible
> +  - reg
> +  - mstar,irqs-map-range
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    mst_intc0: interrupt-controller@1f2032d0 {
> +      compatible = "mstar,mst-intc", "mediatek,mt58xx-intc";
> +      interrupt-controller;
> +      #interrupt-cells = <3>;
> +      #address-cells = <1>;
> +      #size-cells = <1>;
> +      interrupt-parent = <&gic>;
> +      reg = <0x1f2032d0 0x30>;
> +      mstar,irqs-map-range = <0 63>;

Is 0 a PPI or SPI? This property is making some assumption and wouldn't 
be able to support both types or another parent interrupt controller.

> +    };
> +...
> -- 
> 2.18.0

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

[Mark-PK Tsai]

From: Rob Herring <robh@kernel.org>

>> +
>> +  "#interrupt-cells":
>> +    const: 3
>> +    description: |
>> +      Use the same format as specified by GIC in arm,gic.yaml.
>
>That's odd. You have the same SPI and PPI stuff?
>

No, but I just want to keep the format same as arm,gic, and let the
driver bypass 1st and 3rd cell to the parent GIC.
The mst-intc driver translate the interrupt number in 2nd cell to the
interrupt on the parent GIC.

>> +
>> +  reg:
>> +    description: |
>> +      Physical base address of the mstar interrupt controller
>> +      registers and length of memory mapped region.
>
>Drop this. That's every 'reg' property.
>
>> +    minItems: 1
>
>maxItems is more logical.
>
>> +
>> +  mstar,irqs-map-range:
>> +    description: |
>> +      The range of parent interrupt controller's interrupt lines
>> +      that are hardwired to mstar interrupt controller.
>
>Is this <start size> or <start end>?
>

<start end>.
I will add this in the description.

>Really, this should just use 'interrupts' even though that's a bit 
>verbose. Or be implied by the compatible string. What's the maximum 
>number of parent interrupts?
>
>In any case, we really need to stop having vendor specific properties 
>for this.

We use 64 interrupts per interrupt controller.
As you mentions, if we use the standard property 'interrupts',
then we need to put 64 interrupt property in the interrupt
controller node, and it will be hard to understand.
So I suppose we need an vendor specific property here.

>
>> +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
>> +    items:
>> +      minItems: 2
>> +      maxItems: 2
>> +
>> +  mstar,intc-no-eoi:
>> +    description: |
>
>Don't need '|' if there's no formatting.
>
>> +      Mark this controller has no End Of Interrupt(EOI) implementation.
>> +      This is a empty, boolean property.
>
>You can drop this line. The schema says this.
>
>> +    type: boolean
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - mstar,irqs-map-range
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    mst_intc0: interrupt-controller@1f2032d0 {
>> +      compatible = "mstar,mst-intc", "mediatek,mt58xx-intc";
>> +      interrupt-controller;
>> +      #interrupt-cells = <3>;
>> +      #address-cells = <1>;
>> +      #size-cells = <1>;
>> +      interrupt-parent = <&gic>;
>> +      reg = <0x1f2032d0 0x30>;
>> +      mstar,irqs-map-range = <0 63>;
>
>Is 0 a PPI or SPI? This property is making some assumption and wouldn't 
>be able to support both types or another parent interrupt controller.
>

0 is the interrupt number of the parent interrupt controller.
There's no SPI and PPI stuff in mst-intc, it will bypass the
1st cell to parent controller.

e.g. The device node as following will point to GIC SPI 31
("value in 2nd cell" + "start in irqs-map-range").
	usb: {
		interrupt-parent = <&mtk_intc0>;
		interrupts = <0x0 31 0x4>;
		...
	};

>> +    };
>> +...
>> -- 
>> 2.18.0

Re: [PATCH v2 1/2] irqchip: irq-mst: Add MStar interrupt controller support

[Mark-PK Tsai]

From: Marc Zyngier <maz@kernel.org>

>On 2020-08-20 13:36, Daniel Palmer wrote:
>> Hi Mark-PK, Marc
>> 
>> I'm not sure this will be the final version but I'm going to try to
>> integrate this with my current MStar/SigmaStar tree over the weekend
>> and then I guess I can give this a tested-by?
>
>That'd be good.
>
>> Assuming this version or the next is acceptable can I just follow up
>> with a small patch to add the instances I need in my dtsi or should I
>> wait until it's merged before doing that?
>
>No need to wait, although the platform-specific details should go
>via the arm-soc tree.
>
>I'm not going to review the new version before next week anyway
>(I'm making a point in reviewing any given series at most once
>a week).
>
>         M.
>-- 
>Jazz is not dead. It just smells funny...

I've post the patch v3[1] and the driver is same as v2.
The difference is that I add the test-by label by Daniel and add
an entry in MAINTAINERS for mst-intc.

Please review it and let me know if you have any suggestions.
Thanks.

[1] https://lore.kernel.org/lkml/20200902063344.1852-2-mark-pk.tsai@mediatek.com/

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

[Mark-PK Tsai]

Hi,

I've updated the yaml and post it in the patch series v3[1].
But I still keep the vendor specific property mstar,irqs-map-range as I
mentioned in the last reply.
Please review it.

[1] https://lore.kernel.org/lkml/20200902063344.1852-3-mark-pk.tsai@mediatek.com/