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@xxxxxxxxxx>
>
> > 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@xxxxxxxxxxxx>
> > > ---
> > >  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@xxxxxxxxxxxx>
> > > + */
> > > +
> > > +#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...