Date: Tue, 17 Feb 2015 11:11:04 +0000
From: Marc Zyngier <marc.zyngier@arm.com>
To: "Yun Wu (Abel)" <wuyun.wu@huawei.com>
Cc: "tglx@linutronix.de" <tglx@linutronix.de>,
        "jason@lakedaemon.net" <jason@lakedaemon.net>,
        "linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
        "linux-kernel@vger.kernel.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
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3041
Lines: 104

On Tue, 17 Feb 2015 10:15:15 +0000
"Yun Wu (Abel)" <wuyun.wu@huawei.com> wrote:

> On 2015/2/17 17:29, Marc Zyngier wrote:
> 
> > On Sun, 15 Feb 2015 09:32:02 +0000
> > Yun Wu <wuyun.wu@huawei.com> 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 <wuyun.wu@huawei.com>
> >> ---
> >>  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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/