From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1751342AbbLUKAH (ORCPT <rfc822;w@1wt.eu>);
	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 <rfc822;linux-kernel@vger.kernel.org>);
	Mon, 21 Dec 2015 05:00:05 -0500
X-Injected-Via-Gmane: http://gmane.org/
To: linux-kernel@vger.kernel.org
From: Alexey Charkov <alchark@gmail.com>
Subject: Re: [PATCH 4/4] clocksource/vt8500: Add register R/W functions
Date: Mon, 21 Dec 2015 09:54:36 +0000 (UTC)
Message-ID: <loom.20151221T103308-220@post.gmane.org>
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: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Roman Volkov <v1ron <at> mail.ru> writes:

> 
> From: Roman Volkov <rvolkov <at> 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 <rvolkov <at> 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
>  <at>  <at>  -38,36 +38,75  <at>  <at> 
> 
>  #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.

<skip>

>  <at>  <at>  -75,23 +114,24  <at>  <at>  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.

<skip>

Best regards,
Alexey