From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-14.0 required=3.0 tests=BAYES_00,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id EB9B1C4338F for ; Wed, 25 Aug 2021 08:40:31 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id CDD3B61247 for ; Wed, 25 Aug 2021 08:40:31 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S239455AbhHYIlQ (ORCPT ); Wed, 25 Aug 2021 04:41:16 -0400 Received: from mail.kernel.org ([198.145.29.99]:51790 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231765AbhHYIlP (ORCPT ); Wed, 25 Aug 2021 04:41:15 -0400 Received: from disco-boy.misterjones.org (disco-boy.misterjones.org [51.254.78.96]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id BA3B461176; Wed, 25 Aug 2021 08:40:29 +0000 (UTC) Received: from sofa.misterjones.org ([185.219.108.64] helo=why.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1mIoSR-0075y6-Kp; Wed, 25 Aug 2021 09:40:27 +0100 Date: Wed, 25 Aug 2021 09:40:27 +0100 Message-ID: <87pmu1q5ms.wl-maz@kernel.org> From: Marc Zyngier To: Huacai Chen Cc: Thomas Gleixner , linux-kernel@vger.kernel.org, Xuefeng Li , Huacai Chen , Jiaxun Yang Subject: Re: [PATCH V3 08/10] irqchip: Add LoongArch CPU interrupt controller support In-Reply-To: <20210825061152.3396398-9-chenhuacai@loongson.cn> References: <20210825061152.3396398-1-chenhuacai@loongson.cn> <20210825061152.3396398-9-chenhuacai@loongson.cn> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: chenhuacai@loongson.cn, tglx@linutronix.de, linux-kernel@vger.kernel.org, lixuefeng@loongson.cn, chenhuacai@gmail.com, jiaxun.yang@flygoat.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 25 Aug 2021 07:11:50 +0100, Huacai Chen 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 > --- > 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 > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > + > +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. You can solve this by: - Move over to CONFIG_GENERIC_IRQ_MULTI_HANDLER so that the interrupt controller can register itself with the core, rather than being defined at compile time. - Drop the do_IRQ() madness. Perform whenever stuff you need to do in the arch code *before* calling into the interrupt controller code. - Use generic_handle_irq() to call into the irq stack. It will handle all the irq_enter()/irq_exit() correctly. It will also avoid the silly double lookup of the irq_desc on interrupt handling. > +} > + > +static int loongarch_cpu_intc_map(struct irq_domain *d, unsigned int irq, > + irq_hw_number_t hwirq) > +{ > + struct irq_chip *chip; > + > + irq_set_noprobe(irq); > + chip = &loongarch_cpu_irq_controller; > + set_vi_handler(EXCCODE_INT_START + hwirq, default_handle_irq); What is that? Yet another MIPS legacy? Why does it have to be per interrupt if it obviously apply to each and every root interrupt? Given that 'vi' probably stands for "vectored interrupt", why isn't that the irq_enable() code? > + irq_set_chip_and_handler(irq, chip, handle_percpu_irq); > + > + return 0; > +} > + > +static const struct irq_domain_ops loongarch_cpu_intc_irq_domain_ops = { > + .map = loongarch_cpu_intc_map, > + .xlate = irq_domain_xlate_onecell, > +}; > + > +int __init loongarch_cpu_irq_init(void) > +{ > + /* Mask interrupts. */ > + clear_csr_ecfg(ECFG0_IM); > + clear_csr_estat(ESTATF_IP); > + > + irq_domain = irq_domain_add_simple(NULL, EXCCODE_INT_NUM, > + LOONGSON_CPU_IRQ_BASE, &loongarch_cpu_intc_irq_domain_ops, NULL); NAK. You still obviously have some static partitioning of the interrupt space, which is not acceptable for a new architecture. > + > + if (!irq_domain) > + panic("Failed to add irqdomain for LoongArch CPU"); > + > + return 0; > +} I haven't seen much progress from the first version I reviewed. This is still the same antiquated, broken MIPS code, only with a different name. M. -- Without deviation from the norm, progress is not possible.