Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date:   Mon, 03 Aug 2020 16:52:01 +0100
From:   Marc Zyngier <maz@kernel.org>
To:     Mark-PK Tsai <mark-pk.tsai@mediatek.com>
Cc:     alix.wu@mediatek.com, devicetree@vger.kernel.org,
        jason@lakedaemon.net, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org,
        matthias.bgg@gmail.com, robh+dt@kernel.org, tglx@linutronix.de,
        yj.chiang@mediatek.com
Subject: Re: [PATCH 1/2] irqchip: irq-mt58xx: Add mt58xx interrupt controller
 support
In-Reply-To: <43f5cba1199f89cde68c8a577103f28b@kernel.org> (raw)
References: <43f5cba1199f89cde68c8a577103f28b@kernel.org>
 <43f5cba1199f89cde68c8a577103f28b@kernel.org> (raw)
User-Agent: Roundcube Webmail/1.4.5
Message-ID: <18809de59c26b1817320992c7f7bb0f4@kernel.org>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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