From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754588AbbKMJ2z (ORCPT ); Fri, 13 Nov 2015 04:28:55 -0500 Received: from mout.kundenserver.de ([212.227.17.24]:63403 "EHLO mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752670AbbKMJ2v (ORCPT ); Fri, 13 Nov 2015 04:28:51 -0500 From: Arnd Bergmann To: linux-arm-kernel@lists.infradead.org Cc: Jisheng Zhang , srinivas.kandagatla@gmail.com, maxime.coquelin@st.com, patrice.chotard@st.com, daniel.lezcano@linaro.org, tglx@linutronix.de, kernel@stlinux.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH] clocksource/drivers/arm_global_timer: Always use {readl|writel}_relaxed Date: Fri, 13 Nov 2015 10:28:01 +0100 Message-ID: <10575502.sxpiT76bOp@wuerfel> User-Agent: KMail/4.11.5 (Linux/3.16.0-10-generic; KDE/4.11.5; x86_64; ; ) In-Reply-To: <20151113164025.344a0faf@xhacker> References: <1447403678-7217-1-git-send-email-jszhang@marvell.com> <20151113164025.344a0faf@xhacker> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Provags-ID: V03:K0:c7set+OEmwD0UTfFtNQgb63vmsvliI7OoAzFTkrMCnqoXiAE77a sRXf3ueVq4tNltFj5h6Yf/qxmL3trfvi1pIRLJL72VIp5xbAjRAxr0XA7XjppZeLRUHvm2V vQvMxuau6uLNIOlaL+uuzO7YLRgQjZPwTD1GN34yCKgQUd0LhyIosC4bO33HAO+aP5MWvic feHXPamm9R4kTcB6cxTJA== X-UI-Out-Filterresults: notjunk:1;V01:K0:hT2/iufvLvY=:1XXL8FfFyYuGgoFYGfzpeF qSSo9D8tgB5wK5uLa1Mze6IGNkflidW1f98mnMlGvyMi8T7PAgsKKThauUo9hjCUFBq+8KSJG DMY4/qjrPsrrHKoYt2Z8fof1EWL/GJ7JnXEssnwcbiNU3YOPQkRyFz1MRJFbXXiypmS/SgV2M kDfGA7iBPDJE44Evp2G5v8aKo+kJ3uygr7q3meCPVtISJo/3YIn0Hl0lQUGx94mMmzGgrERk1 hMVGCtWB2cEHtuWgYOQByWfNQMhfU8cq1dO9t892Qt2/0Anp5vzPormK+RKQq93mCK7GzMu1C TFqGLCf/sEBFkO8BeUjTEsflbG2/pumD1Tu31Ll00Ec6sXklbe1CIaHlIxp4z6boZ9y9S0s+3 tEyYlOHAcYYEFt5i0U8bL0i+ajxBoWpJM6/vF3t/jPLKjJU93JgvfYZ4whlOzGITDzPftkhXv Hls/Kt+w1MWpKl9U3MUE1q8ain1q5qeHgTh597KmwL8EZjZArrAguBUXkBMptk6yHCB2mb2qh aZigyXp91u+AdI9VkgGiM6OLkLCR9/mN8ll5UrHzoGoqoGDxnMm6cmcWJW/OslXAb8BpckbCV LFsfM92M/zblOkuV5NMhGVENoBAyktLwe6ZTBbxFWVv6dQIQ/Wx5ImYV8aec0lMb4s98sSFpP rjwiQ8Rf1qvF4ZFgNvqplahgCe6WcG9F3w5TO92lyVg1/0WPQgWe+DsjYlP+Q8Ea/HL9dt3W8 7d/zM5cfNPCkQCrl Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Friday 13 November 2015 16:40:25 Jisheng Zhang wrote: > On Fri, 13 Nov 2015 16:34:38 +0800 > Jisheng Zhang wrote: > > > This driver use both readl/writel and their relaxed version, this patch > > tries to unify the io accesses. > > I'm sorry. This is the version I'd like to send for review and merge. Can you > please kindly have a review? I would prefer to use write_relaxed() as sparingly as we can, it is too hard to verify each case to ensure that we don't have to watch out for ordering or locking issues. > > diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c > > index a2cb6fa..84a5a5d 100644 > > --- a/drivers/clocksource/arm_global_timer.c > > +++ b/drivers/clocksource/arm_global_timer.c > > @@ -99,27 +99,27 @@ static void gt_compare_set(unsigned long delta, int periodic) > > > > counter += delta; > > ctrl = GT_CONTROL_TIMER_ENABLE; > > - writel(ctrl, gt_base + GT_CONTROL); > > - writel(lower_32_bits(counter), gt_base + GT_COMP0); > > - writel(upper_32_bits(counter), gt_base + GT_COMP1); > > + writel_relaxed(ctrl, gt_base + GT_CONTROL); > > + writel_relaxed(lower_32_bits(counter), gt_base + GT_COMP0); > > + writel_relaxed(upper_32_bits(counter), gt_base + GT_COMP1); > > > > if (periodic) { > > - writel(delta, gt_base + GT_AUTO_INC); > > + writel_relaxed(delta, gt_base + GT_AUTO_INC); > > ctrl |= GT_CONTROL_AUTO_INC; > > } > > > > ctrl |= GT_CONTROL_COMP_ENABLE | GT_CONTROL_IRQ_ENABLE; > > - writel(ctrl, gt_base + GT_CONTROL); > > + writel_relaxed(ctrl, gt_base + GT_CONTROL); > > } This seems fine. Do you have any performance numbers to show how much we save per call on a platform you care about, and how often it is called for a typical workload? I see that _gt_counter_read() already uses readl_relaxed(), and it seems to be a much bigger win there, as we read the clock more often than we write the comparator, so the person who did that probably thought that this one wasn't important enough. Can you add an explanation in the changelog why you think that was a mistake? Unifying the accessors across a driver is not enough of a reason I think. > > static int gt_clockevent_shutdown(struct clock_event_device *evt) > > { > > unsigned long ctrl; > > > > - ctrl = readl(gt_base + GT_CONTROL); > > + ctrl = readl_relaxed(gt_base + GT_CONTROL); > > ctrl &= ~(GT_CONTROL_COMP_ENABLE | GT_CONTROL_IRQ_ENABLE | > > GT_CONTROL_AUTO_INC); > > - writel(ctrl, gt_base + GT_CONTROL); > > + writel_relaxed(ctrl, gt_base + GT_CONTROL); > > return 0; > > } This is certainly not performance critical, better leave it using the standard accessors. > > @@ -212,11 +212,11 @@ static u64 notrace gt_sched_clock_read(void) > > > > static void __init gt_clocksource_init(void) > > { > > - writel(0, gt_base + GT_CONTROL); > > - writel(0, gt_base + GT_COUNTER0); > > - writel(0, gt_base + GT_COUNTER1); > > + writel_relaxed(0, gt_base + GT_CONTROL); > > + writel_relaxed(0, gt_base + GT_COUNTER0); > > + writel_relaxed(0, gt_base + GT_COUNTER1); > > /* enables timer on all the cores */ > > - writel(GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL); > > + writel_relaxed(GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL); > > > > #ifdef CONFIG_CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK > > sched_clock_register(gt_sched_clock_read, 64, gt_clk_rate); > > Same here. Arnd