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=-7.0 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,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 A70B5C4CECC for ; Sun, 15 Sep 2019 14:24:55 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (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 53ED5214D8 for ; Sun, 15 Sep 2019 14:24:55 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="MC1COp3B" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 53ED5214D8 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-riscv-bounces+infradead-linux-riscv=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Subject:To:From:Message-ID:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=XO4M76ulfLJbVXvjv7nXHOIlBE0JKA3FU1WGJ+0/brY=; b=MC1COp3BJaQ8CO JTu505c4Q8piDzZ6vilT7tj7XTDqB6blkVn1YTCxO8O0N0PCsNr5AlrCcwYHdPIrGf41s2YvGvTWf ArrqwjQ3NdM7OFeJZYbsddIZXQKBYMjnWs3jALhWYnUIG+mL7aFJLtREMrfEwnfRVzVL8KM5YGOgv NfktQBjSrRZiFjPJ3h/6jSIsDFgXzIx557y2DtLdrZuXFeSWHVQFPP96fDHW8TB45Kr0AAjLAdfof ctS/m9RWgcWpCaf6a13Bse1MLT7TjoeIy/DTImjxfPLGDe7gJK67hP1IEevXD0h/ruXMYctT+L4fM OiD4lUUP372arpjN0Miw==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92.2 #3 (Red Hat Linux)) id 1i9VSP-0007b8-RL; Sun, 15 Sep 2019 14:24:53 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.92.2 #3 (Red Hat Linux)) id 1i9VS4-0007a4-Pc for linux-riscv@lists.infradead.org; Sun, 15 Sep 2019 14:24:47 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 4A28128; Sun, 15 Sep 2019 07:24:30 -0700 (PDT) Received: from big-swifty.misterjones.org (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 586D63F575; Sun, 15 Sep 2019 07:24:27 -0700 (PDT) Date: Sun, 15 Sep 2019 15:24:20 +0100 Message-ID: <8636gxskmj.wl-maz@kernel.org> From: Marc Zyngier To: Darius Rad Subject: Re: [PATCH] irqchip/sifive-plic: add irq_mask and irq_unmask In-Reply-To: <529ec882-734f-17ae-e4cb-3aeb563ad1d5@bluespec.com> References: <529ec882-734f-17ae-e4cb-3aeb563ad1d5@bluespec.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 EasyPG/1.0.0 Emacs/26 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190915_072437_147114_392D0890 X-CRM114-Status: GOOD ( 24.21 ) X-BeenThere: linux-riscv@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Jason Cooper , Palmer Dabbelt , linux-kernel@vger.kernel.org, Paul Walmsley , linux-riscv@lists.infradead.org, Thomas Gleixner Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-riscv" Errors-To: linux-riscv-bounces+infradead-linux-riscv=archiver.kernel.org@lists.infradead.org On Thu, 12 Sep 2019 22:40:34 +0100, Darius Rad wrote: Hi Darius, > > As per the existing comment, irq_mask and irq_unmask do not need > to do anything for the PLIC. However, the functions must exist > (the pointers cannot be NULL) as they are not optional, based on > the documentation (Documentation/core-api/genericirq.rst) as well > as existing usage (e.g., include/linux/irqchip/chained_irq.h). > > Signed-off-by: Darius Rad > --- > drivers/irqchip/irq-sifive-plic.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c > index cf755964f2f8..52d5169f924f 100644 > --- a/drivers/irqchip/irq-sifive-plic.c > +++ b/drivers/irqchip/irq-sifive-plic.c > @@ -111,6 +111,13 @@ static void plic_irq_disable(struct irq_data *d) > plic_irq_toggle(cpu_possible_mask, d->hwirq, 0); > } > > +/* > + * There is no need to mask/unmask PLIC interrupts. They are "masked" > + * by reading claim and "unmasked" when writing it back. > + */ > +static void plic_irq_mask(struct irq_data *d) { } > +static void plic_irq_unmask(struct irq_data *d) { } This outlines a bigger issue. If your irqchip doesn't require mask/unmask, you're probably not using the right interrupt flow. Looking at the code, I see you're using handle_simple_irq, which is almost universally wrong. As per the description above, these interrupts should be using the fasteoi flow, which is designed for this exact behaviour (the interrupt controller knows which interrupt is in flight and doesn't require SW to do anything bar signalling the EOI). Another thing is that mask/unmask tends to be a requirement, while enable/disable tends to be optional. There is no hard line here, but the expectations are that: (a) A disabled line can drop interrupts (b) A masked line cannot drop interrupts Depending what the PLIC architecture mandates, you'll need to implement one and/or the other. Having just (a) is indicative of a HW bug, and I'm not assuming that this is the case. (b) only is pretty common, and (a)+(b) has a few adepts. My bet is that it requires (b) only. > + > #ifdef CONFIG_SMP > static int plic_set_affinity(struct irq_data *d, > const struct cpumask *mask_val, bool force) > @@ -138,12 +145,10 @@ static int plic_set_affinity(struct irq_data *d, > > static struct irq_chip plic_chip = { > .name = "SiFive PLIC", > - /* > - * There is no need to mask/unmask PLIC interrupts. They are "masked" > - * by reading claim and "unmasked" when writing it back. > - */ > .irq_enable = plic_irq_enable, > .irq_disable = plic_irq_disable, > + .irq_mask = plic_irq_mask, > + .irq_unmask = plic_irq_unmask, > #ifdef CONFIG_SMP > .irq_set_affinity = plic_set_affinity, > #endif Can you give the following patch a go? It brings the irq flow in line with what the HW can do. It is of course fully untested (not even compile tested...). Thanks, M. >From c0ce33a992ec18f5d3bac7f70de62b1ba2b42090 Mon Sep 17 00:00:00 2001 From: Marc Zyngier Date: Sun, 15 Sep 2019 15:17:45 +0100 Subject: [PATCH] irqchip/sifive-plic: Switch to fasteoi flow The SiFive PLIC interrupt controller seems to have all the HW features to support the fasteoi flow, but the driver seems to be stuck in a distant past. Bring it into the 21st century. Signed-off-by: Marc Zyngier --- drivers/irqchip/irq-sifive-plic.c | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c index cf755964f2f8..8fea384d392b 100644 --- a/drivers/irqchip/irq-sifive-plic.c +++ b/drivers/irqchip/irq-sifive-plic.c @@ -97,7 +97,7 @@ static inline void plic_irq_toggle(const struct cpumask *mask, } } -static void plic_irq_enable(struct irq_data *d) +static void plic_irq_mask(struct irq_data *d) { unsigned int cpu = cpumask_any_and(irq_data_get_affinity_mask(d), cpu_online_mask); @@ -106,7 +106,7 @@ static void plic_irq_enable(struct irq_data *d) plic_irq_toggle(cpumask_of(cpu), d->hwirq, 1); } -static void plic_irq_disable(struct irq_data *d) +static void plic_irq_unmask(struct irq_data *d) { plic_irq_toggle(cpu_possible_mask, d->hwirq, 0); } @@ -125,10 +125,8 @@ static int plic_set_affinity(struct irq_data *d, if (cpu >= nr_cpu_ids) return -EINVAL; - if (!irqd_irq_disabled(d)) { - plic_irq_toggle(cpu_possible_mask, d->hwirq, 0); - plic_irq_toggle(cpumask_of(cpu), d->hwirq, 1); - } + plic_irq_toggle(cpu_possible_mask, d->hwirq, 0); + plic_irq_toggle(cpumask_of(cpu), d->hwirq, 1); irq_data_update_effective_affinity(d, cpumask_of(cpu)); @@ -136,14 +134,18 @@ static int plic_set_affinity(struct irq_data *d, } #endif +static void plic_irq_eoi(struct irq_data *d) +{ + struct plic_handler *handler = this_cpu_ptr(&plic_handlers); + + writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM); +} + static struct irq_chip plic_chip = { .name = "SiFive PLIC", - /* - * There is no need to mask/unmask PLIC interrupts. They are "masked" - * by reading claim and "unmasked" when writing it back. - */ - .irq_enable = plic_irq_enable, - .irq_disable = plic_irq_disable, + .irq_mask = plic_irq_mask, + .irq_unmask = plic_irq_unmask, + .irq_eoi = plic_irq_eoi, #ifdef CONFIG_SMP .irq_set_affinity = plic_set_affinity, #endif @@ -152,7 +154,7 @@ static struct irq_chip plic_chip = { static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hwirq) { - irq_set_chip_and_handler(irq, &plic_chip, handle_simple_irq); + irq_set_chip_and_handler(irq, &plic_chip, handle_fasteoi_irq); irq_set_chip_data(irq, NULL); irq_set_noprobe(irq); return 0; @@ -188,7 +190,6 @@ static void plic_handle_irq(struct pt_regs *regs) hwirq); else generic_handle_irq(irq); - writel(hwirq, claim); } csr_set(sie, SIE_SEIE); } -- 2.20.1 -- Jazz is not dead, it just smells funny. _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv 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=-7.1 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED 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 5AF34C4CECC for ; Sun, 15 Sep 2019 14:24:32 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 23924214D8 for ; Sun, 15 Sep 2019 14:24:32 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1568557472; bh=ClUIEDCHC3LbaAf25emqs+pADt4j7X1utlfi0P+mDNU=; h=Date:From:To:Cc:Subject:In-Reply-To:References:List-ID:From; b=Ue6Mp/cQLHznCND/SYaz/JW6nU7jVjcLvn3SQb+Mw64axxnJHpAKUBOAt19jqlcpG vFUKMwLcibdb1P3UhuSEJlM38VyZOtvKueqKzDxbc98m5rTuT5DmgoB2sS1c0VvOGq EzHKkLRy9PTgb6jp9EG/XDWHEoZlbsF57Hm9KROA= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1731682AbfIOOYb convert rfc822-to-8bit (ORCPT ); Sun, 15 Sep 2019 10:24:31 -0400 Received: from foss.arm.com ([217.140.110.172]:35088 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1730611AbfIOOYb (ORCPT ); Sun, 15 Sep 2019 10:24:31 -0400 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 4A28128; Sun, 15 Sep 2019 07:24:30 -0700 (PDT) Received: from big-swifty.misterjones.org (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 586D63F575; Sun, 15 Sep 2019 07:24:27 -0700 (PDT) Date: Sun, 15 Sep 2019 15:24:20 +0100 Message-ID: <8636gxskmj.wl-maz@kernel.org> From: Marc Zyngier To: Darius Rad Cc: linux-riscv@lists.infradead.org, Palmer Dabbelt , Paul Walmsley , linux-kernel@vger.kernel.org, Thomas Gleixner , Jason Cooper Subject: Re: [PATCH] irqchip/sifive-plic: add irq_mask and irq_unmask In-Reply-To: <529ec882-734f-17ae-e4cb-3aeb563ad1d5@bluespec.com> References: <529ec882-734f-17ae-e4cb-3aeb563ad1d5@bluespec.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 EasyPG/1.0.0 Emacs/26 (aarch64-unknown-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 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 12 Sep 2019 22:40:34 +0100, Darius Rad wrote: Hi Darius, > > As per the existing comment, irq_mask and irq_unmask do not need > to do anything for the PLIC. However, the functions must exist > (the pointers cannot be NULL) as they are not optional, based on > the documentation (Documentation/core-api/genericirq.rst) as well > as existing usage (e.g., include/linux/irqchip/chained_irq.h). > > Signed-off-by: Darius Rad > --- > drivers/irqchip/irq-sifive-plic.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c > index cf755964f2f8..52d5169f924f 100644 > --- a/drivers/irqchip/irq-sifive-plic.c > +++ b/drivers/irqchip/irq-sifive-plic.c > @@ -111,6 +111,13 @@ static void plic_irq_disable(struct irq_data *d) > plic_irq_toggle(cpu_possible_mask, d->hwirq, 0); > } > > +/* > + * There is no need to mask/unmask PLIC interrupts. They are "masked" > + * by reading claim and "unmasked" when writing it back. > + */ > +static void plic_irq_mask(struct irq_data *d) { } > +static void plic_irq_unmask(struct irq_data *d) { } This outlines a bigger issue. If your irqchip doesn't require mask/unmask, you're probably not using the right interrupt flow. Looking at the code, I see you're using handle_simple_irq, which is almost universally wrong. As per the description above, these interrupts should be using the fasteoi flow, which is designed for this exact behaviour (the interrupt controller knows which interrupt is in flight and doesn't require SW to do anything bar signalling the EOI). Another thing is that mask/unmask tends to be a requirement, while enable/disable tends to be optional. There is no hard line here, but the expectations are that: (a) A disabled line can drop interrupts (b) A masked line cannot drop interrupts Depending what the PLIC architecture mandates, you'll need to implement one and/or the other. Having just (a) is indicative of a HW bug, and I'm not assuming that this is the case. (b) only is pretty common, and (a)+(b) has a few adepts. My bet is that it requires (b) only. > + > #ifdef CONFIG_SMP > static int plic_set_affinity(struct irq_data *d, > const struct cpumask *mask_val, bool force) > @@ -138,12 +145,10 @@ static int plic_set_affinity(struct irq_data *d, > > static struct irq_chip plic_chip = { > .name = "SiFive PLIC", > - /* > - * There is no need to mask/unmask PLIC interrupts. They are "masked" > - * by reading claim and "unmasked" when writing it back. > - */ > .irq_enable = plic_irq_enable, > .irq_disable = plic_irq_disable, > + .irq_mask = plic_irq_mask, > + .irq_unmask = plic_irq_unmask, > #ifdef CONFIG_SMP > .irq_set_affinity = plic_set_affinity, > #endif Can you give the following patch a go? It brings the irq flow in line with what the HW can do. It is of course fully untested (not even compile tested...). Thanks, M. >From c0ce33a992ec18f5d3bac7f70de62b1ba2b42090 Mon Sep 17 00:00:00 2001 From: Marc Zyngier Date: Sun, 15 Sep 2019 15:17:45 +0100 Subject: [PATCH] irqchip/sifive-plic: Switch to fasteoi flow The SiFive PLIC interrupt controller seems to have all the HW features to support the fasteoi flow, but the driver seems to be stuck in a distant past. Bring it into the 21st century. Signed-off-by: Marc Zyngier --- drivers/irqchip/irq-sifive-plic.c | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c index cf755964f2f8..8fea384d392b 100644 --- a/drivers/irqchip/irq-sifive-plic.c +++ b/drivers/irqchip/irq-sifive-plic.c @@ -97,7 +97,7 @@ static inline void plic_irq_toggle(const struct cpumask *mask, } } -static void plic_irq_enable(struct irq_data *d) +static void plic_irq_mask(struct irq_data *d) { unsigned int cpu = cpumask_any_and(irq_data_get_affinity_mask(d), cpu_online_mask); @@ -106,7 +106,7 @@ static void plic_irq_enable(struct irq_data *d) plic_irq_toggle(cpumask_of(cpu), d->hwirq, 1); } -static void plic_irq_disable(struct irq_data *d) +static void plic_irq_unmask(struct irq_data *d) { plic_irq_toggle(cpu_possible_mask, d->hwirq, 0); } @@ -125,10 +125,8 @@ static int plic_set_affinity(struct irq_data *d, if (cpu >= nr_cpu_ids) return -EINVAL; - if (!irqd_irq_disabled(d)) { - plic_irq_toggle(cpu_possible_mask, d->hwirq, 0); - plic_irq_toggle(cpumask_of(cpu), d->hwirq, 1); - } + plic_irq_toggle(cpu_possible_mask, d->hwirq, 0); + plic_irq_toggle(cpumask_of(cpu), d->hwirq, 1); irq_data_update_effective_affinity(d, cpumask_of(cpu)); @@ -136,14 +134,18 @@ static int plic_set_affinity(struct irq_data *d, } #endif +static void plic_irq_eoi(struct irq_data *d) +{ + struct plic_handler *handler = this_cpu_ptr(&plic_handlers); + + writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM); +} + static struct irq_chip plic_chip = { .name = "SiFive PLIC", - /* - * There is no need to mask/unmask PLIC interrupts. They are "masked" - * by reading claim and "unmasked" when writing it back. - */ - .irq_enable = plic_irq_enable, - .irq_disable = plic_irq_disable, + .irq_mask = plic_irq_mask, + .irq_unmask = plic_irq_unmask, + .irq_eoi = plic_irq_eoi, #ifdef CONFIG_SMP .irq_set_affinity = plic_set_affinity, #endif @@ -152,7 +154,7 @@ static struct irq_chip plic_chip = { static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hwirq) { - irq_set_chip_and_handler(irq, &plic_chip, handle_simple_irq); + irq_set_chip_and_handler(irq, &plic_chip, handle_fasteoi_irq); irq_set_chip_data(irq, NULL); irq_set_noprobe(irq); return 0; @@ -188,7 +190,6 @@ static void plic_handle_irq(struct pt_regs *regs) hwirq); else generic_handle_irq(irq); - writel(hwirq, claim); } csr_set(sie, SIE_SEIE); } -- 2.20.1 -- Jazz is not dead, it just smells funny.