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 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id B6D0AC433F5 for ; Fri, 29 Apr 2022 21:46:43 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S239304AbiD2VuA (ORCPT ); Fri, 29 Apr 2022 17:50:00 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:38540 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S239040AbiD2Vt4 (ORCPT ); Fri, 29 Apr 2022 17:49:56 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0EB2982D12 for ; Fri, 29 Apr 2022 14:46:36 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 65A61622D6 for ; Fri, 29 Apr 2022 21:46:36 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id BF7D8C385A7; Fri, 29 Apr 2022 21:46:35 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1651268795; bh=uEqk8GJFvq7V+Q6NBIO4P9k1yYCtwvYnvhz9DXyiRGs=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=TgfIVQxqNP9HZOS70lWzl+/Y/IB6OPgURSBpo0+qx5M5o3PLSFUUiZXRc+dct49BP uDSa/rDmVsjrY80mvZMRJwmQswTOArzGVek1zTAjXZ4duYW3yc+1tdEyAfv6Gm/ojD oa88+QgFu3chePsB3vOeMaj1iYOh8gfVQzm7LIH7XSCTrY9sFlTv/9uRnLXppPccZD MicKIC9zrg+KZ8wgGrgmWKX3KmIXtOIvXMvrnxoxTugUxy7mlDPrD9XBnBHBT5gcuD gJuQ4aPtjjfZ/O+qRdK2mJOb+nuq55+qadNcpnTK5EeY6PwEzoNFM8mRTFFBXNRnO4 ecBMYcV+aT7uw== 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 1nkYRd-0082nC-8P; Fri, 29 Apr 2022 22:46:33 +0100 Date: Fri, 29 Apr 2022 22:46:33 +0100 Message-ID: <87k0b78sw6.wl-maz@kernel.org> From: Marc Zyngier To: Daniel Thompson Cc: Thomas Gleixner , Catalin Marinas , Will Deacon , Ard Biesheuvel , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2] irqchip/exiu: Fix acknowledgment of edge triggered interrupts In-Reply-To: <20220429183605.gld37gz6taa5k7fk@maple.lan> References: <20220429165314.2343705-1-daniel.thompson@linaro.org> <87pmkz94bc.wl-maz@kernel.org> <20220429183605.gld37gz6taa5k7fk@maple.lan> 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: daniel.thompson@linaro.org, tglx@linutronix.de, catalin.marinas@arm.com, will@kernel.org, ardb@kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org 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 Fri, 29 Apr 2022 19:36:05 +0100, Daniel Thompson wrote: > > On Fri, Apr 29, 2022 at 06:39:51PM +0100, Marc Zyngier wrote: > > On Fri, 29 Apr 2022 17:53:14 +0100, > > Daniel Thompson wrote: > > > > > > + > > > + if (!(edge_triggered & BIT(d->hwirq))) > > > + writel(BIT(d->hwirq), data->base + EIREQCLR); > > > > Is this write even needed for a level interrupt? Or does the register > > always behave as a latch irrespective of the trigger? > > It is unconditionally latched; must be cleared or the interrupt will be > jammed on. Of course, for level interrupts that are still asserted then > the write will not clear the interrupt. OK. The HW folks missed a trick here, but hey. > > > > > irq_chip_eoi_parent(d); > > > } > > > > > > @@ -91,10 +100,13 @@ static int exiu_irq_set_type(struct irq_data *d, unsigned int type) > > > writel_relaxed(val, data->base + EILVL); > > > > > > val = readl_relaxed(data->base + EIEDG); > > > - if (type == IRQ_TYPE_LEVEL_LOW || type == IRQ_TYPE_LEVEL_HIGH) > > > + if (type == IRQ_TYPE_LEVEL_LOW || type == IRQ_TYPE_LEVEL_HIGH) { > > > val &= ~BIT(d->hwirq); > > > - else > > > + irq_set_handler_locked(d, handle_fasteoi_irq); > > > + } else { > > > val |= BIT(d->hwirq); > > > + irq_set_handler_locked(d, handle_fasteoi_ack_irq); > > > + } > > > writel_relaxed(val, data->base + EIEDG); > > > > > > writel_relaxed(BIT(d->hwirq), data->base + EIREQCLR); > > > @@ -104,6 +116,7 @@ static int exiu_irq_set_type(struct irq_data *d, unsigned int type) > > > > > > static struct irq_chip exiu_irq_chip = { > > > .name = "EXIU", > > > + .irq_ack = exiu_irq_ack, > > > .irq_eoi = exiu_irq_eoi, > > > .irq_enable = exiu_irq_enable, > > > .irq_mask = exiu_irq_mask, > > > @@ -148,6 +161,8 @@ static int exiu_domain_alloc(struct irq_domain *dom, unsigned int virq, > > > struct irq_fwspec parent_fwspec; > > > struct exiu_irq_data *info = dom->host_data; > > > irq_hw_number_t hwirq; > > > + int i, ret; > > > + u32 edge_triggered; > > > > > > parent_fwspec = *fwspec; > > > if (is_of_node(dom->parent->fwnode)) { > > > @@ -165,7 +180,17 @@ static int exiu_domain_alloc(struct irq_domain *dom, unsigned int virq, > > > irq_domain_set_hwirq_and_chip(dom, virq, hwirq, &exiu_irq_chip, info); > > > > > > parent_fwspec.fwnode = dom->parent->fwnode; > > > - return irq_domain_alloc_irqs_parent(dom, virq, nr_irqs, &parent_fwspec); > > > + ret = irq_domain_alloc_irqs_parent(dom, virq, nr_irqs, &parent_fwspec); > > > + if (ret) > > > + return ret; > > > + > > > + edge_triggered = readl_relaxed(info->base + EIEDG); > > > + for (i = 0; i < nr_irqs; i++) > > > + irq_set_handler(virq + i, edge_triggered & BIT(i) ? > > > + handle_fasteoi_ack_irq : > > > + handle_fasteoi_irq); > > > + > > > + return 0; > > > > Why do you need this at allocation time? I would have expected the > > trigger configuration to be enough. > > I saw the following in the description of the interrupt trigger modes > : When requesting an interrupt without specifying a IRQF_TRIGGER, the > : setting should be assumed to be "as already configured", which may > : be as per machine or firmware initialisation. > > From that I was concerned that the callback to set the trigger mode > would not be called in all cases. Thus when I saw that calling > __irq_set_trigger() was on a conditional code path in __setup_irq() > then I wrote the above logic. > > I assume I overlooked something? Is a call to exiu_irq_set_type() > guaranteed to happen in all cases? My expectations are that the interrupt is configured from irq_create_fwspec_mapping(), which will set the trigger it obtained from the firmware, long before the interrupt is setup. The conditional code you saw in __setup_irq() is to handle the case where request_irq() is passing a trigger configuration that isn't the default one. Either way, you should be able to safely remove this from the allocation side. M. -- Without deviation from the norm, progress is not possible.