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.2 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable 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 94013C433E0 for ; Thu, 7 Jan 2021 10:20:13 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 2F99B2313B for ; Thu, 7 Jan 2021 10:20:13 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 2F99B2313B Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Type: Content-Transfer-Encoding:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:Message-ID:References:In-Reply-To:Subject:To:From: Date:MIME-Version:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=6UadCv9qnQwd1/41MzN4Pw/GfdSGSEEHvBZpRpe3cbs=; b=bwKXhRVe5STUrb+UJVAk5qOrQ KRQZNLnLNbhmMSB42hW4yjfZm9jlzjUeWsRHoYRGf4eE34G2Gh6aZiLXEHB+Lfl5X7pDuQ4LIH+6t zG4smluSp+TyDPCGo6WgN2m5kEeERDEkOG7g4DztLTRtzkQ7Dx8xoks2rbQ1I80srMNigcJHSegM2 IGgJ6Wb77ddl8VH2cGQUtY1+eXf6XK8Pr+rGC04IWCNbfqUOLVhmp38w9M3EAy/0GWqtSzyvhBmIk qq08GHk0Fx0wyO6cJhwOy72L+mscBNCuFmcVIL9xrWWEjZJi+HTEVwzXSyZmL9/8KKa2dXB4I5Ix2 L+CiM2UiA==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kxSMS-0000oY-DH; Thu, 07 Jan 2021 10:17:44 +0000 Received: from mail.kernel.org ([198.145.29.99]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kxSMP-0000oB-Gi for linux-arm-kernel@lists.infradead.org; Thu, 07 Jan 2021 10:17:42 +0000 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 36E2023138; Thu, 7 Jan 2021 10:17:40 +0000 (UTC) Received: from disco-boy.misterjones.org ([51.254.78.96] helo=www.loen.fr) by disco-boy.misterjones.org with esmtpsa (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.94) (envelope-from ) id 1kxSML-005oKv-V9; Thu, 07 Jan 2021 10:17:38 +0000 MIME-Version: 1.0 Date: Thu, 07 Jan 2021 10:17:37 +0000 From: Marc Zyngier To: ChiaWei Wang Subject: Re: [PATCH 4/6] irqchip/aspeed: Add Aspeed eSPI interrupt controller In-Reply-To: References: <20210106055939.19386-1-chiawei_wang@aspeedtech.com> <20210106055939.19386-5-chiawei_wang@aspeedtech.com> <123bc25c72b3b17c0c4154d8bd8ce3b0@kernel.org> User-Agent: Roundcube Webmail/1.4.9 Message-ID: X-Sender: maz@kernel.org X-SA-Exim-Connect-IP: 51.254.78.96 X-SA-Exim-Rcpt-To: chiawei_wang@aspeedtech.com, robh+dt@kernel.org, joel@jms.id.au, andrew@aj.id.au, tglx@linutronix.de, p.zabel@pengutronix.de, linux-aspeed@lists.ozlabs.org, openbmc@lists.ozlabs.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, BMC-SW@aspeedtech.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210107_051742_044277_74CFDE39 X-CRM114-Status: GOOD ( 26.23 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: devicetree@vger.kernel.org, linux-aspeed@lists.ozlabs.org, BMC-SW , andrew@aj.id.au, openbmc@lists.ozlabs.org, linux-kernel@vger.kernel.org, robh+dt@kernel.org, joel@jms.id.au, p.zabel@pengutronix.de, tglx@linutronix.de, linux-arm-kernel@lists.infradead.org Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 2021-01-07 02:59, ChiaWei Wang wrote: > Hi Marc, > >> -----Original Message----- >> From: Marc Zyngier >> Sent: Wednesday, January 6, 2021 6:59 PM >> To: ChiaWei Wang >> Subject: Re: [PATCH 4/6] irqchip/aspeed: Add Aspeed eSPI interrupt >> controller >> >> On 2021-01-06 05:59, Chia-Wei, Wang wrote: >> > The eSPI interrupt controller acts as a SW IRQ number decoder to >> > correctly control/dispatch interrupts of the eSPI peripheral, virtual >> > wire, out-of-band, and flash channels. >> > >> > Signed-off-by: Chia-Wei, Wang >> > --- >> > drivers/irqchip/Makefile | 2 +- >> > drivers/irqchip/irq-aspeed-espi-ic.c | 251 ++++++++++++++++++++++++ >> > include/soc/aspeed/espi.h | 279 >> +++++++++++++++++++++++++++ >> > 3 files changed, 531 insertions(+), 1 deletion(-) create mode 100644 >> > drivers/irqchip/irq-aspeed-espi-ic.c >> > create mode 100644 include/soc/aspeed/espi.h >> > >> > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile index >> > 0ac93bfaec61..56da4a3123f8 100644 >> > --- a/drivers/irqchip/Makefile >> > +++ b/drivers/irqchip/Makefile >> > @@ -86,7 +86,7 @@ obj-$(CONFIG_MVEBU_PIC) += >> irq-mvebu-pic.o >> > obj-$(CONFIG_MVEBU_SEI) += irq-mvebu-sei.o >> > obj-$(CONFIG_LS_EXTIRQ) += irq-ls-extirq.o >> > obj-$(CONFIG_LS_SCFG_MSI) += irq-ls-scfg-msi.o >> > -obj-$(CONFIG_ARCH_ASPEED) += irq-aspeed-vic.o irq-aspeed-i2c-ic.o >> > irq-aspeed-scu-ic.o >> > +obj-$(CONFIG_ARCH_ASPEED) += irq-aspeed-vic.o irq-aspeed-i2c-ic.o >> > irq-aspeed-scu-ic.o irq-aspeed-espi-ic.o >> > obj-$(CONFIG_STM32_EXTI) += irq-stm32-exti.o >> > obj-$(CONFIG_QCOM_IRQ_COMBINER) += qcom-irq-combiner.o >> > obj-$(CONFIG_IRQ_UNIPHIER_AIDET) += irq-uniphier-aidet.o >> > diff --git a/drivers/irqchip/irq-aspeed-espi-ic.c >> > b/drivers/irqchip/irq-aspeed-espi-ic.c >> > new file mode 100644 >> > index 000000000000..8a5cc8fe3f0c >> > --- /dev/null >> > +++ b/drivers/irqchip/irq-aspeed-espi-ic.c >> > @@ -0,0 +1,251 @@ >> > +// SPDX-License-Identifier: GPL-2.0-or-later >> > +/* >> > + * Copyright (c) 2020 Aspeed Technology Inc. >> > + */ >> > +#include >> > +#include >> > +#include >> > +#include >> > +#include #include >> > +#include #include #include >> > + #include #include >> > + >> > +#include >> > +#include >> > + >> > +#define DEVICE_NAME "aspeed-espi-ic" >> > +#define IRQCHIP_NAME "eSPI-IC" >> > + >> > +#define ESPI_IC_IRQ_NUM 7 >> > + >> > +struct aspeed_espi_ic { >> > + struct regmap *map; >> > + int irq; >> > + int gpio_irq; >> > + struct irq_domain *irq_domain; >> > +}; >> > + >> > +static void aspeed_espi_ic_gpio_isr(struct irq_desc *desc) { >> > + unsigned int irq; >> > + struct aspeed_espi_ic *espi_ic = irq_desc_get_handler_data(desc); >> > + struct irq_chip *chip = irq_desc_get_chip(desc); >> > + >> > + chained_irq_enter(chip, desc); >> > + >> > + irq = irq_find_mapping(espi_ic->irq_domain, >> > + ASPEED_ESPI_IC_CTRL_RESET); >> > + generic_handle_irq(irq); >> > + >> > + irq = irq_find_mapping(espi_ic->irq_domain, >> > + ASPEED_ESPI_IC_CHAN_RESET); >> > + generic_handle_irq(irq); >> >> So for each mux interrupt, you generate two endpoints interrupt, >> without even >> checking whether they are pending? That's no good. > > As the eSPI IC driver is chained to Aspeed GPIO IC, the pending is > checked in the gpio-aspeed.c That's not the place to do that. > >> > + >> > + chained_irq_exit(chip, desc); >> > +} >> > + >> > +static void aspeed_espi_ic_isr(struct irq_desc *desc) { >> > + unsigned int sts; >> > + unsigned int irq; >> > + struct aspeed_espi_ic *espi_ic = irq_desc_get_handler_data(desc); >> > + struct irq_chip *chip = irq_desc_get_chip(desc); >> > + >> > + chained_irq_enter(chip, desc); >> > + >> > + regmap_read(espi_ic->map, ESPI_INT_STS, &sts); >> > + >> > + if (sts & ESPI_INT_STS_PERIF_BITS) { >> > + irq = irq_find_mapping(espi_ic->irq_domain, >> > + ASPEED_ESPI_IC_PERIF_EVENT); >> > + generic_handle_irq(irq); >> > + } >> > + >> > + if (sts & ESPI_INT_STS_VW_BITS) { >> > + irq = irq_find_mapping(espi_ic->irq_domain, >> > + ASPEED_ESPI_IC_VW_EVENT); >> > + generic_handle_irq(irq); >> > + } >> > + >> > + if (sts & ESPI_INT_STS_OOB_BITS) { >> > + irq = irq_find_mapping(espi_ic->irq_domain, >> > + ASPEED_ESPI_IC_OOB_EVENT); >> > + generic_handle_irq(irq); >> > + } >> > + >> > + if (sts & ESPI_INT_STS_FLASH_BITS) { >> > + irq = irq_find_mapping(espi_ic->irq_domain, >> > + ASPEED_ESPI_IC_FLASH_EVENT); >> > + generic_handle_irq(irq); >> > + } >> > + >> > + if (sts & ESPI_INT_STS_HW_RST_DEASSERT) { >> > + irq = irq_find_mapping(espi_ic->irq_domain, >> > + ASPEED_ESPI_IC_CTRL_EVENT); >> > + generic_handle_irq(irq); >> > + } >> >> This is horrible. Why can't you just use fls() in a loop? > > The bits in the interrupt status register for a eSPI channel are not > sequentially arranged. > Using fls() may invoke an eSPI channel ISR multiple times. > So I collected the bitmap for each channel, respectively, and call the > ISR at once. And that's equally wrong. You need to handle interrupts individually, as they are different signal. If you are to implement an interrupt controller, please do it properly. Otherwise, get rid of it and move everything into your pet driver. There is no need to do a half-baked job. As it is, there is no way this code can be merged. M. -- Jazz is not dead. It just smells funny... _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel