mbox series

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

Message ID 20200803062214.24076-1-mark-pk.tsai@mediatek.com
Headers show
Series irqchip: irq-mt58xx: Add mt58xx series interrupt | expand

Message

Mark-PK Tsai Aug. 3, 2020, 6:22 a.m. UTC 
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

Comments

Marc Zyngier Aug. 3, 2020, 8:04 a.m. UTC | #1 
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.
Mark-PK Tsai Aug. 3, 2020, 3:03 p.m. UTC | #2 
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.
Marc Zyngier Aug. 3, 2020, 3:52 p.m. UTC | #3 
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.
Daniel Palmer Aug. 6, 2020, 10:12 a.m. UTC | #4 
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/
Mark-PK Tsai Aug. 6, 2020, 2:07 p.m. UTC | #5 
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.
Daniel Palmer Aug. 6, 2020, 2:58 p.m. UTC | #6 
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
Marc Zyngier Aug. 6, 2020, 3:12 p.m. UTC | #7 
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.
Mark-PK Tsai Aug. 7, 2020, 12:52 p.m. UTC | #8 
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