[v2,2/3] clocksource: tegra: Define timer bases in header file

Added timers that are present in tegra30 and later, that are NOT in tegra20.

Also, some of these timer bases are needed in the tegra watchdog driver, so
separate them out into a header file that both the clocksource driver and
the watchdog driver can share them.

Signed-off-by: Andrew Chew <achew@nvidia.com>
---
 drivers/clocksource/tegra20_timer.c | 15 ++++++-------
 include/clocksource/tegra_timer.h   | 43 +++++++++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+), 9 deletions(-)
 create mode 100644 include/clocksource/tegra_timer.h

On 02/04/2014 01:17 AM, Andrew Chew wrote:
> Added timers that are present in tegra30 and later, that are NOT in tegra20.
>
> Also, some of these timer bases are needed in the tegra watchdog driver, so
> separate them out into a header file that both the clocksource driver and
> the watchdog driver can share them.
>
> Signed-off-by: Andrew Chew <achew@nvidia.com>

When reading the patch 3/3, I don't see any define reused from this 
header except TEGRA30_TIMER_WDT_BASE which is only used for the 
watchdog. May be I missed something but I don't see any definition 
shared and thus I don't see the point of creating this header file.

> ---
>   drivers/clocksource/tegra20_timer.c | 15 ++++++-------
>   include/clocksource/tegra_timer.h   | 43 +++++++++++++++++++++++++++++++++++++
>   2 files changed, 49 insertions(+), 9 deletions(-)
>   create mode 100644 include/clocksource/tegra_timer.h
>
> diff --git a/drivers/clocksource/tegra20_timer.c b/drivers/clocksource/tegra20_timer.c
> index 73cfa56..2c49643 100644
> --- a/drivers/clocksource/tegra20_timer.c
> +++ b/drivers/clocksource/tegra20_timer.c
> @@ -28,6 +28,8 @@
>   #include <linux/of_irq.h>
>   #include <linux/sched_clock.h>
>
> +#include <clocksource/tegra_timer.h>
> +
>   #include <asm/mach/time.h>
>   #include <asm/smp_twd.h>
>
> @@ -39,11 +41,6 @@
>   #define TIMERUS_USEC_CFG 0x14
>   #define TIMERUS_CNTR_FREEZE 0x4c
>
> -#define TIMER1_BASE 0x0
> -#define TIMER2_BASE 0x8
> -#define TIMER3_BASE 0x50
> -#define TIMER4_BASE 0x58
> -
>   #define TIMER_PTV 0x0
>   #define TIMER_PCR 0x4
>
> @@ -64,7 +61,7 @@ static int tegra_timer_set_next_event(unsigned long cycles,
>   	u32 reg;
>
>   	reg = 0x80000000 | ((cycles > 1) ? (cycles-1) : 0);
> -	timer_writel(reg, TIMER3_BASE + TIMER_PTV);
> +	timer_writel(reg, TEGRA20_TIMER3_BASE + TIMER_PTV);
>
>   	return 0;
>   }
> @@ -74,12 +71,12 @@ static void tegra_timer_set_mode(enum clock_event_mode mode,
>   {
>   	u32 reg;
>
> -	timer_writel(0, TIMER3_BASE + TIMER_PTV);
> +	timer_writel(0, TEGRA20_TIMER3_BASE + TIMER_PTV);
>
>   	switch (mode) {
>   	case CLOCK_EVT_MODE_PERIODIC:
>   		reg = 0xC0000000 | ((1000000/HZ)-1);
> -		timer_writel(reg, TIMER3_BASE + TIMER_PTV);
> +		timer_writel(reg, TEGRA20_TIMER3_BASE + TIMER_PTV);
>   		break;
>   	case CLOCK_EVT_MODE_ONESHOT:
>   		break;
> @@ -142,7 +139,7 @@ static void tegra_read_persistent_clock(struct timespec *ts)
>   static irqreturn_t tegra_timer_interrupt(int irq, void *dev_id)
>   {
>   	struct clock_event_device *evt = (struct clock_event_device *)dev_id;
> -	timer_writel(1<<30, TIMER3_BASE + TIMER_PCR);
> +	timer_writel(1<<30, TEGRA20_TIMER3_BASE + TIMER_PCR);
>   	evt->event_handler(evt);
>   	return IRQ_HANDLED;
>   }
> diff --git a/include/clocksource/tegra_timer.h b/include/clocksource/tegra_timer.h
> new file mode 100644
> index 0000000..ea0bc8b
> --- /dev/null
> +++ b/include/clocksource/tegra_timer.h
> @@ -0,0 +1,43 @@
> +/*
> + * Copyright (C) 2010 Google, Inc.
> + *
> + * Author:
> + *	Colin Cross <ccross@google.com>

          ^^^^^^^^^^^^^^^^^^
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#ifndef __CLOCKSOURCE_TEGRA_TIMER_H
> +#define __CLOCKSOURCE_TEGRA_TIMER_H
> +
> +/* Tegra 20 timers */
> +#define TEGRA20_TIMER1_BASE	0x0
> +#define TEGRA20_TIMER2_BASE	0x8
> +#define TEGRA20_TIMER3_BASE	0x50
> +#define TEGRA20_TIMER4_BASE	0x58
> +
> +/* Tegra 30 timers */
> +#define TEGRA30_TIMER1_BASE	TEGRA20_TIMER1_BASE
> +#define TEGRA30_TIMER2_BASE	TEGRA20_TIMER2_BASE
> +#define TEGRA30_TIMER3_BASE	TEGRA20_TIMER3_BASE
> +#define TEGRA30_TIMER4_BASE	TEGRA20_TIMER4_BASE
> +#define TEGRA30_TIMER5_BASE	0x60
> +#define TEGRA30_TIMER6_BASE	0x68
> +#define TEGRA30_TIMER7_BASE	0x70
> +#define TEGRA30_TIMER8_BASE	0x78
> +#define TEGRA30_TIMER9_BASE	0x80
> +#define TEGRA30_TIMER0_BASE	0x88
> +
> +/* Used by the tegra watchdog timer */
> +#define TEGRA30_TIMER_WDT_BASE	TEGRA30_TIMER5_BASE
> +#define TEGRA30_TIMER_WDT_ID	5
> +
> +#endif /* __CLOCKSOURCE_TEGRA_TIMER_H */
>

> From: Daniel Lezcano [mailto:daniel.lezcano@linaro.org]

> Sent: Monday, February 03, 2014 11:55 PM

> To: Andrew Chew; tglx@linutronix.de; swarren@wwwdotorg.org;

> thierry.reding@gmail.com; rob@landley.net; grant.likely@linaro.org;

> robh+dt@kernel.org; abrestic@chromium.org; dgreid@chromium.org;

> katierh@chromium.org

> Cc: linux-kernel@vger.kernel.org; linux-tegra@vger.kernel.org; linux-

> watchdog@vger.kernel.org; linux-doc@vger.kernel.org

> Subject: Re: [PATCH v2 2/3] clocksource: tegra: Define timer bases in header

> file

> 

> On 02/04/2014 01:17 AM, Andrew Chew wrote:

> > Added timers that are present in tegra30 and later, that are NOT in tegra20.

> >

> > Also, some of these timer bases are needed in the tegra watchdog

> > driver, so separate them out into a header file that both the

> > clocksource driver and the watchdog driver can share them.

> >

> > Signed-off-by: Andrew Chew <achew@nvidia.com>

> 

> When reading the patch 3/3, I don't see any define reused from this header

> except TEGRA30_TIMER_WDT_BASE which is only used for the watchdog.

> May be I missed something but I don't see any definition shared and thus I

> don't see the point of creating this header file.


I guess the point is to make other potential drivers that make use of the timer
registers aware that this particular timer (timer 5) is provisioned for the watchdog
driver.  Right now, you're right, there really isn't much that's shared...the only
thing really is the base address of timer 5 (and its associated ID)...the timer bases
are kind of all over the place, so it's not straightforward to calculate the ID from the
base address.

But I believe the thought at the time (from Stephen Warren's suggestion) is to have
a central place where this stuff is defined so that it's more clear what timers are
provisioned for what purposes.  It just happens that today, the only users of
the timers, other than clocksource, is watchdog, as far as I can tell.

> > +++ b/include/clocksource/tegra_timer.h

> > @@ -0,0 +1,43 @@

> > +/*

> > + * Copyright (C) 2010 Google, Inc.

> > + *

> > + * Author:

> > + *	Colin Cross <ccross@google.com>


The contents of this new header file are largely just a cut and paste of the corresponding
snippet in tegra20-timer.c, so I thought I'd carry over the license verbatim.  I didn't author
this really...I just moved it around.  What should I do instead?

On 02/03/2014 05:17 PM, Andrew Chew wrote:
> Added timers that are present in tegra30 and later, that are NOT in tegra20.
> 
> Also, some of these timer bases are needed in the tegra watchdog driver, so
> separate them out into a header file that both the clocksource driver and
> the watchdog driver can share them.

> diff --git a/include/clocksource/tegra_timer.h b/include/clocksource/tegra_timer.h

> +/* Tegra 20 timers */
> +#define TEGRA20_TIMER1_BASE	0x0
> +#define TEGRA20_TIMER2_BASE	0x8
> +#define TEGRA20_TIMER3_BASE	0x50
> +#define TEGRA20_TIMER4_BASE	0x58
> +
> +/* Tegra 30 timers */
> +#define TEGRA30_TIMER1_BASE	TEGRA20_TIMER1_BASE
> +#define TEGRA30_TIMER2_BASE	TEGRA20_TIMER2_BASE
> +#define TEGRA30_TIMER3_BASE	TEGRA20_TIMER3_BASE
> +#define TEGRA30_TIMER4_BASE	TEGRA20_TIMER4_BASE
> +#define TEGRA30_TIMER5_BASE	0x60
> +#define TEGRA30_TIMER6_BASE	0x68
> +#define TEGRA30_TIMER7_BASE	0x70
> +#define TEGRA30_TIMER8_BASE	0x78
> +#define TEGRA30_TIMER9_BASE	0x80
> +#define TEGRA30_TIMER0_BASE	0x88

Why put the SoC name in the define names? Why not just have
TIMER1_BASE..TIMER10_BASE (that should be 10 not 0 as in your patch,
right?) and have the driver know that 1..4 are valid on Tegra20, and
1..10 are valid on later chips.

I guess if the defines are moved into a header file, adding a TEGRA_
prefix does make sense.

But I wonder if it wouldn't be simpler for the Tegra WDT driver to just
call a function on the Tegra clocksource driver to find out which timer
ID(s) to avoid using? Even simpler would be to just put a comment in the
WDT driver saying that timer 5 was chosen arbitrarily, but if it's
changed make sure not to conflict with the clocksource driver (and an
equivalent change to the clocksource driver).
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

> > +/* Tegra 20 timers */
> > +#define TEGRA20_TIMER1_BASE	0x0
> > +#define TEGRA20_TIMER2_BASE	0x8
> > +#define TEGRA20_TIMER3_BASE	0x50
> > +#define TEGRA20_TIMER4_BASE	0x58
> > +
> > +/* Tegra 30 timers */
> > +#define TEGRA30_TIMER1_BASE	TEGRA20_TIMER1_BASE
> > +#define TEGRA30_TIMER2_BASE	TEGRA20_TIMER2_BASE
> > +#define TEGRA30_TIMER3_BASE	TEGRA20_TIMER3_BASE
> > +#define TEGRA30_TIMER4_BASE	TEGRA20_TIMER4_BASE
> > +#define TEGRA30_TIMER5_BASE	0x60
> > +#define TEGRA30_TIMER6_BASE	0x68
> > +#define TEGRA30_TIMER7_BASE	0x70
> > +#define TEGRA30_TIMER8_BASE	0x78
> > +#define TEGRA30_TIMER9_BASE	0x80
> > +#define TEGRA30_TIMER0_BASE	0x88
> 
> Why put the SoC name in the define names? Why not just have
> TIMER1_BASE..TIMER10_BASE (that should be 10 not 0 as in your patch,
> right?) and have the driver know that 1..4 are valid on Tegra20, and
> 1..10 are valid on later chips.
> 
> I guess if the defines are moved into a header file, adding a TEGRA_ prefix
> does make sense.
> 
> But I wonder if it wouldn't be simpler for the Tegra WDT driver to just call a
> function on the Tegra clocksource driver to find out which timer
> ID(s) to avoid using? Even simpler would be to just put a comment in the
> WDT driver saying that timer 5 was chosen arbitrarily, but if it's changed make
> sure not to conflict with the clocksource driver (and an equivalent change to
> the clocksource driver).

Alright, I think I'll just go with putting a comment in the WDT driver then, so that
We don't need to add this new header file.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html