From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933825AbbBQLLT (ORCPT ); Tue, 17 Feb 2015 06:11:19 -0500 Received: from foss.arm.com ([217.140.101.70]:52796 "EHLO usa-sjc-mx-foss1.foss.arm.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1755156AbbBQLLS (ORCPT ); Tue, 17 Feb 2015 06:11:18 -0500 Date: Tue, 17 Feb 2015 11:11:04 +0000 From: Marc Zyngier To: "Yun Wu (Abel)" Cc: "tglx@linutronix.de" , "jason@lakedaemon.net" , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH v2 5/6] irqchip: gicv3-its: add support for power down Message-ID: <20150217111104.5718d6f1@arm.com> In-Reply-To: <54E314B3.40803@huawei.com> References: <1423992723-5028-1-git-send-email-wuyun.wu@huawei.com> <1423992723-5028-6-git-send-email-wuyun.wu@huawei.com> <20150217092935.1fefcb24@arm.com> <54E314B3.40803@huawei.com> Organization: ARM Ltd X-Mailer: Claws Mail 3.11.1 (GTK+ 2.24.25; arm-unknown-linux-gnueabihf) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 17 Feb 2015 10:15:15 +0000 "Yun Wu (Abel)" wrote: > On 2015/2/17 17:29, Marc Zyngier wrote: > > > On Sun, 15 Feb 2015 09:32:02 +0000 > > Yun Wu wrote: > > > >> It's unsafe to change the configurations of an activated ITS > >> directly since this will lead to unpredictable results. This patch > >> guarantees a safe quiescent status before initializing an ITS. > > > > Please change the title of this patch to reflect what it actually > > does. Nothing here is about powering down anything. > > My miss, I will fix this in next version. > > > > >> Signed-off-by: Yun Wu > >> --- > >> drivers/irqchip/irq-gic-v3-its.c | 32 > >> ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) > >> > >> diff --git a/drivers/irqchip/irq-gic-v3-its.c > >> b/drivers/irqchip/irq-gic-v3-its.c index 42c03b2..29eb665 100644 > >> --- a/drivers/irqchip/irq-gic-v3-its.c > >> +++ b/drivers/irqchip/irq-gic-v3-its.c > >> @@ -1321,6 +1321,31 @@ static const struct irq_domain_ops > >> its_domain_ops = { .deactivate = > >> its_irq_domain_deactivate, }; > >> > >> +static int its_check_quiesced(void __iomem *base) > >> +{ > >> + u32 count = 1000000; /* 1s */ > >> + u32 val; > >> + > >> + val = readl_relaxed(base + GITS_CTLR); > >> + if (val & GITS_CTLR_QUIESCENT) > >> + return 0; > >> + > >> + /* Disable the generation of all interrupts to this ITS */ > >> + val &= ~GITS_CTLR_ENABLE; > >> + writel_relaxed(val, base + GITS_CTLR); > >> + > >> + /* Poll GITS_CTLR and wait until ITS becomes quiescent */ > >> + while (count--) { > >> + val = readl_relaxed(base + GITS_CTLR); > >> + if (val & GITS_CTLR_QUIESCENT) > >> + return 0; > >> + cpu_relax(); > >> + udelay(1); > >> + } > > > > You're now introducing a third variant of a 1s timeout loop. Notice > > a pattern? > > > > I am not sure I know exactly what you suggest. Do you mean I should > code like below to keep the coding style same as the other 2 loops? > > while (1) { > val = readl_relaxed(base + GITS_CTLR); > if (val & GITS_CTLR_QUIESCENT) > return 0; > > count--; > if (!count) > return -EBUSY; > > cpu_relax(); > udelay(1); > } That'd be a good start. But given that this is starting to be a common construct, it could probably be rewritten as: static int its_poll_timeout(struct its_node *its, void *data, int (*fn)(struct its_node *its, void *data)) { while (1) { if (!fn(its, data)) return 0; ... } } and have the call sites to provide the right utility function. We also have two similar timeout loops in the main GICv3 driver, so there should be room for improvement. Thoughts? Thanks, M. -- Jazz is not dead. It just smells funny.