Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751342AbbLUKAH (ORCPT ); Mon, 21 Dec 2015 05:00:07 -0500 Received: from plane.gmane.org ([80.91.229.3]:36971 "EHLO plane.gmane.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751074AbbLUKAF (ORCPT ); Mon, 21 Dec 2015 05:00:05 -0500 X-Injected-Via-Gmane: http://gmane.org/ To: linux-kernel@vger.kernel.org From: Alexey Charkov Subject: Re: [PATCH 4/4] clocksource/vt8500: Add register R/W functions Date: Mon, 21 Dec 2015 09:54:36 +0000 (UTC) Message-ID: References: <1450650492-18996-1-git-send-email-v1ron@mail.ru> <1450650492-18996-5-git-send-email-v1ron@mail.ru> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit X-Complaints-To: usenet@ger.gmane.org X-Gmane-NNTP-Posting-Host: sea.gmane.org User-Agent: Loom/3.14 (http://gmane.org/) X-Loom-IP: 109.173.114.243 (Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Firefox/38.0) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4659 Lines: 119 Roman Volkov mail.ru> writes: > > From: Roman Volkov v1ros.org> > > vt8500 timer requires special synchronization for accessing some of its > registers. Define special read and write functions to handle this process > transparently. Maybe introduce such accessor functions (conditionally) into the PXA driver and kill this one altogether then? If I understood you right, this extra bus synchronization is the only thing that makes vt8500 different from PXA, so merging the two files right away might be a better long-term option. > To perform a read from the Timer Count register, user must write a one > to the Timer Control register and wait for completion flag by polling the > Timer Read Count Active bit. > > To perform a write to the Count or Match registers, user must poll the > write completion flag for the corresponding register to ensure that the > previous write completed and then write the actual value. > > Signed-off-by: Roman Volkov v1ros.org> > --- > drivers/clocksource/vt8500_timer.c | 90 +++++++++++++++++++++++++++----------- > 1 file changed, 65 insertions(+), 25 deletions(-) > > diff --git a/drivers/clocksource/vt8500_timer.c b/drivers/clocksource/vt8500_timer.c > index 7649852..4d7513f 100644 > --- a/drivers/clocksource/vt8500_timer.c > +++ b/drivers/clocksource/vt8500_timer.c > -38,36 +38,75 > > #define VT8500_TIMER_OFFSET 0x0100 > #define VT8500_TIMER_HZ 3000000 > -#define TIMER_MATCH_VAL 0x0000 > +#define TIMER_MATCH0_VAL 0 > +#define TIMER_MATCH1_VAL 0x04 > +#define TIMER_MATCH2_VAL 0x08 > +#define TIMER_MATCH3_VAL 0x0c > #define TIMER_COUNT_VAL 0x0010 > #define TIMER_STATUS_VAL 0x0014 > #define TIMER_IER_VAL 0x001c /* interrupt enable */ > #define TIMER_CTRL_VAL 0x0020 > #define TIMER_AS_VAL 0x0024 /* access status */ > -#define TIMER_COUNT_R_ACTIVE (1 << 5) /* not ready for read */ > -#define TIMER_COUNT_W_ACTIVE (1 << 4) /* not ready for write */ > -#define TIMER_MATCH_W_ACTIVE (1 << 0) /* not ready for write */ > - > -#define timer_readl(addr) readl_relaxed(regbase + addr) > -#define timer_writel(v, addr) writel_relaxed(v, regbase + addr) > +/* R/W status flags */ > +#define TIMER_COUNT_R_ACTIVE (1 << 5) > +#define TIMER_COUNT_W_ACTIVE (1 << 4) > +#define TIMER_MATCH3_W_ACTIVE (1 << 3) > +#define TIMER_MATCH2_W_ACTIVE (1 << 2) > +#define TIMER_MATCH1_W_ACTIVE (1 << 1) > +#define TIMER_MATCH0_W_ACTIVE (1 << 0) > + > +#define vt8500_timer_sync(bit) { while (readl_relaxed \ > + (regbase + TIMER_AS_VAL) & bit) \ > + cpu_relax(); } The whole issue around 'loops' counter in these busy waits basically boils down to whether we would like a way to try and recover from a potential hardware misbehavior. You can of course argue that when the system timer misbehaves you already have bigger issues to worry about, but does a 10 msec limit that was in the original version really hurt? > #define MIN_OSCR_DELTA 16 > > static void __iomem *regbase; > > -static cycle_t vt8500_timer_read(struct clocksource *cs) > +static void vt8500_timer_write(unsigned long reg, u32 value) Maybe define this with 'value' first, 'reg' second - to be in line with the common prototype of writel and such? Plus if you could take the same name for the macro above (timer_writel) and this accessor (vt8500_timer_write) that would somewhat reduce extra additions/deletions in this patch. Same for the read function. > -75,23 +114,24 static struct clocksource clocksource = { > static int vt8500_timer_set_next_event(unsigned long cycles, > struct clock_event_device *evt) > { > - cycle_t alarm = clocksource.read(&clocksource) + cycles; > - while (timer_readl(TIMER_AS_VAL) & TIMER_MATCH_W_ACTIVE) > - cpu_relax(); > - timer_writel((unsigned long)alarm, TIMER_MATCH_VAL); > + unsigned long alarm = vt8500_timer_read(TIMER_COUNT_VAL) + cycles; I personally like the form above better (via clocksource.read) - even if just for the fact that it's shorter and reduces the number of places where we use TIMER_COUNT_VAL definition. Any specific reasons to rewrite it? > - if ((signed)(alarm - clocksource.read(&clocksource)) <= MIN_OSCR_DELTA) > + vt8500_timer_write(TIMER_MATCH0_VAL, alarm); > + if ((signed)(alarm - vt8500_timer_read( > + TIMER_COUNT_VAL)) <= MIN_OSCR_DELTA) { Same here. Best regards, Alexey -- 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/