linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Marc Zyngier <maz@kernel.org>
To: Huacai Chen <chenhuacai@gmail.com>
Cc: Huacai Chen <chenhuacai@loongson.cn>,
	Thomas Gleixner <tglx@linutronix.de>,
	LKML <linux-kernel@vger.kernel.org>,
	Xuefeng Li <lixuefeng@loongson.cn>,
	Jiaxun Yang <jiaxun.yang@flygoat.com>
Subject: Re: [PATCH V3 08/10] irqchip: Add LoongArch CPU interrupt controller support
Date: Sat, 28 Aug 2021 12:07:17 +0100	[thread overview]
Message-ID: <87v93pddzu.wl-maz@kernel.org> (raw)
In-Reply-To: <CAAhV-H5yadRbTt9a-i-65Mvd6mBxm58R_+mWLfJrauuAe3+qyg@mail.gmail.com>

Huacai,

On Sat, 28 Aug 2021 11:07:16 +0100,
Huacai Chen <chenhuacai@gmail.com> wrote:
> 
> Hi, Marc,
> 
> On Wed, Aug 25, 2021 at 4:40 PM Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Wed, 25 Aug 2021 07:11:50 +0100,
> > Huacai Chen <chenhuacai@loongson.cn> wrote:
> > >
> > > We are preparing to add new Loongson (based on LoongArch, not MIPS)
> >
> > You keep saying "not MIPS", and yet all I see is a blind copy of the
> > MIPS code.
> >
> > > support. This patch add LoongArch CPU interrupt controller support.
> > >
> > > Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
> > > ---
> > >  drivers/irqchip/Kconfig             | 10 ++++
> > >  drivers/irqchip/Makefile            |  1 +
> > >  drivers/irqchip/irq-loongarch-cpu.c | 76 +++++++++++++++++++++++++++++
> > >  3 files changed, 87 insertions(+)
> > >  create mode 100644 drivers/irqchip/irq-loongarch-cpu.c
> > >
> > > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> > > index 084bc4c2eebd..443c3a7a0cc1 100644
> > > --- a/drivers/irqchip/Kconfig
> > > +++ b/drivers/irqchip/Kconfig
> > > @@ -528,6 +528,16 @@ config EXYNOS_IRQ_COMBINER
> > >         Say yes here to add support for the IRQ combiner devices embedded
> > >         in Samsung Exynos chips.
> > >
> > > +config IRQ_LOONGARCH_CPU
> > > +     bool
> > > +     select GENERIC_IRQ_CHIP
> > > +     select IRQ_DOMAIN
> > > +     select GENERIC_IRQ_EFFECTIVE_AFF_MASK
> > > +     help
> > > +       Support for the LoongArch CPU Interrupt Controller. For details of
> > > +       irq chip hierarchy on LoongArch platforms please read the document
> > > +       Documentation/loongarch/irq-chip-model.rst.
> > > +
> > >  config LOONGSON_LIOINTC
> > >       bool "Loongson Local I/O Interrupt Controller"
> > >       depends on MACH_LOONGSON64
> > > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> > > index f88cbf36a9d2..4e34eebe180b 100644
> > > --- a/drivers/irqchip/Makefile
> > > +++ b/drivers/irqchip/Makefile
> > > @@ -105,6 +105,7 @@ obj-$(CONFIG_LS1X_IRQ)                    += irq-ls1x.o
> > >  obj-$(CONFIG_TI_SCI_INTR_IRQCHIP)    += irq-ti-sci-intr.o
> > >  obj-$(CONFIG_TI_SCI_INTA_IRQCHIP)    += irq-ti-sci-inta.o
> > >  obj-$(CONFIG_TI_PRUSS_INTC)          += irq-pruss-intc.o
> > > +obj-$(CONFIG_IRQ_LOONGARCH_CPU)              += irq-loongarch-cpu.o
> > >  obj-$(CONFIG_LOONGSON_LIOINTC)               += irq-loongson-liointc.o
> > >  obj-$(CONFIG_LOONGSON_HTPIC)         += irq-loongson-htpic.o
> > >  obj-$(CONFIG_LOONGSON_HTVEC)         += irq-loongson-htvec.o
> > > diff --git a/drivers/irqchip/irq-loongarch-cpu.c b/drivers/irqchip/irq-loongarch-cpu.c
> > > new file mode 100644
> > > index 000000000000..8e9e8d39cb22
> > > --- /dev/null
> > > +++ b/drivers/irqchip/irq-loongarch-cpu.c
> > > @@ -0,0 +1,76 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright (C) 2020-2021 Loongson Technology Corporation Limited
> > > + */
> > > +
> > > +#include <linux/init.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/irq.h>
> > > +#include <linux/irqchip.h>
> > > +#include <linux/irqdomain.h>
> > > +
> > > +#include <asm/loongarch.h>
> > > +#include <asm/setup.h>
> > > +
> > > +static struct irq_domain *irq_domain;
> > > +
> > > +static inline void enable_loongarch_irq(struct irq_data *d)
> >
> > Why 'inline' given that it is used as a function pointer?
> >
> > > +{
> > > +     set_csr_ecfg(ECFGF(d->hwirq));
> > > +}
> > > +
> > > +#define eoi_loongarch_irq enable_loongarch_irq
> >
> > NAK. EOI and enable cannot be the same operation.
> >
> > > +
> > > +static inline void disable_loongarch_irq(struct irq_data *d)
> > > +{
> > > +     clear_csr_ecfg(ECFGF(d->hwirq));
> > > +}
> > > +
> > > +#define ack_loongarch_irq disable_loongarch_irq
> >
> > Same thing. Either you have different operations, or this only
> > supports mask/unmask.
> >
> > > +
> > > +static struct irq_chip loongarch_cpu_irq_controller = {
> > > +     .name           = "LoongArch",
> > > +     .irq_ack        = ack_loongarch_irq,
> > > +     .irq_eoi        = eoi_loongarch_irq,
> > > +     .irq_enable     = enable_loongarch_irq,
> > > +     .irq_disable    = disable_loongarch_irq,
> > > +};
> > > +
> > > +asmlinkage void default_handle_irq(int irq)
> > > +{
> > > +     do_IRQ(irq_linear_revmap(irq_domain, irq));
> >
> > This looks both wrong and short sighted:
> >
> > - irq_linear_revmap() is now another name for irq_find_mapping().
> >   Which means it uses a RCU read critical section. If, as I expect,
> >   this is just a blind copy of the MIPS code, do_IRQ() will not do
> >   anything with respect to irq_enter()/irq_exit(), which will result
> >   in something pretty bad on the exit from idle path. Lockdep will
> >   probably shout at you pretty loudly.
> >
> > - A single root interrupt controller is, in my modest experience,
> >   something that rarely happen. You will eventually have a variety of
> >   them, and you will have to join the other arches such as arm, arm64,
> >   riscv and csky that use CONFIG_GENERIC_IRQ_MULTI_HANDLER instead of
> >   following the existing MIPS model.
> I try to use CONFIG_GENERIC_IRQ_MULTI_HANDLER and
> set_handle_irq()/handle_arch_irq() as arm64, riscv and csky do. But I
> found a problem:
> The main handler (e.g., handle_arch_irq()) take only one argument
> (i.e., struct pt_regs *regs) and polling all interrupts, but we want
> to use vectored interrupts which take a "irq" argument (as
> default_handle_irq() does) which can directly handle it.

Are you saying that there is no way for the interrupt controller
driver to figure out the hwirq number on its own?  That would seem
pretty odd (even the MIPS GIC has that). Worse case, you can provide
an arch-specific helper that exposes the current hwirq based on the
vector that triggered.

do_IRQ() is a terrible abstraction, and only outlines that your arch
code is badly structured. What does the arch code have to do with a
Linux irq number? It shouldn't care at all, because as a value it has
no significance to the arch code at all. You just go back there
because the management of your interrupt context is upside down, and
it really shouldn't matter *what interrupt fired*.

> This seems that if I want to use vectored interrupts, then I will fall
> to the MIPS model.

Not happening, I'm afraid.

	M.

-- 
Without deviation from the norm, progress is not possible.

  reply	other threads:[~2021-08-28 11:07 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-25  6:11 [PATCH V3 00/10] irqchip: Add LoongArch-related irqchip drivers Huacai Chen
2021-08-25  6:11 ` [PATCH V3 01/10] irqchip: Adjust Kconfig for Loongson Huacai Chen
2021-08-25  6:11 ` [PATCH V3 02/10] irqchip/loongson-pch-pic: Add ACPI init support Huacai Chen
2021-08-25  6:11 ` [PATCH V3 03/10] irqchip/loongson-pch-pic: Add suspend/resume support Huacai Chen
2021-08-25  6:11 ` [PATCH V3 04/10] irqchip/loongson-pch-msi: Add ACPI init support Huacai Chen
2021-08-25  6:11 ` [PATCH V3 05/10] irqchip/loongson-htvec: " Huacai Chen
2021-08-25  6:11 ` [PATCH V3 06/10] irqchip/loongson-htvec: Add suspend/resume support Huacai Chen
2021-08-25  6:11 ` [PATCH V3 07/10] irqchip/loongson-liointc: Add ACPI init support Huacai Chen
2021-08-25  6:11 ` [PATCH V3 08/10] irqchip: Add LoongArch CPU interrupt controller support Huacai Chen
2021-08-25  8:40   ` Marc Zyngier
2021-08-27  8:12     ` Huacai Chen
2021-08-28 10:07     ` Huacai Chen
2021-08-28 11:07       ` Marc Zyngier [this message]
2021-08-29  9:37         ` Huacai Chen
2021-08-29 10:10           ` Marc Zyngier
2021-08-29 10:34             ` Huacai Chen
2021-08-29 11:00               ` Marc Zyngier
2021-08-30  3:19                 ` Huacai Chen
2021-08-25  6:11 ` [PATCH V3 09/10] irqchip: Add Loongson Extended I/O " Huacai Chen
2021-08-25  6:11 ` [PATCH V3 10/10] irqchip: Add Loongson PCH LPC " Huacai Chen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87v93pddzu.wl-maz@kernel.org \
    --to=maz@kernel.org \
    --cc=chenhuacai@gmail.com \
    --cc=chenhuacai@loongson.cn \
    --cc=jiaxun.yang@flygoat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=lixuefeng@loongson.cn \
    --cc=tglx@linutronix.de \
    --subject='Re: [PATCH V3 08/10] irqchip: Add LoongArch CPU interrupt controller support' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).