[PATCH] clocksource: tegra: wrap arch/arm-specific sections in CONFIG_ARM

[PATCH] clocksource: tegra: wrap arch/arm-specific sections in CONFIG_ARM

[Paul Walmsley]

Like several of the other files in drivers/clocksource,
tegra20_timer.c contains code that can only compile when CONFIG_ARM is
enabled.  This causes obvious problems when trying to compile this
code for NVIDIA ARM64-based SoCs, such as Tegra132.  The same timer IP
blocks exist, so it seems appropriate to provide support for them.

So until we figure out a better way to partition this code, wrap the
delay_timer and persistent_clock support code with preprocessor tests
for CONFIG_ARM.  (The delay_timer code should not be needed at all on
ARM64 due to the presence of the ARMv8 architected timer.  The
persistent_clock support code could become important once power
management modes are implemented that turn off the CPU complex.)

Signed-off-by: Paul Walmsley <paul@pwsan.com>
Signed-off-by: Paul Walmsley <pwalmsley@nvidia.com>
Cc: Allen Martin <amartin@nvidia.com>
Cc: Stephen Warren <swarren@nvidia.com>
Cc: Thierry Reding <treding@nvidia.com>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Alexandre Courbot <gnurou@gmail.com>
---
Applies against next-20141209.
Intended for v3.20.
Boot-tested on Tegra124 Jetson TK1 on next-20141209.
Also boot-tested on Tegra132 Norrin FFD on next-20141209 + extra, 
unrelated patches.

 drivers/clocksource/tegra20_timer.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/clocksource/tegra20_timer.c b/drivers/clocksource/tegra20_timer.c
index d2616ef16770..83a8f5c9e139 100644
--- a/drivers/clocksource/tegra20_timer.c
+++ b/drivers/clocksource/tegra20_timer.c
@@ -29,8 +29,10 @@
 #include <linux/sched_clock.h>
 #include <linux/delay.h>
 
+#ifdef CONFIG_ARM
 #include <asm/mach/time.h>
 #include <asm/smp_twd.h>
+#endif
 
 #define RTC_SECONDS            0x08
 #define RTC_SHADOW_SECONDS     0x0c
@@ -49,12 +51,14 @@
 #define TIMER_PCR 0x4
 
 static void __iomem *timer_reg_base;
+#ifdef CONFIG_ARM
 static void __iomem *rtc_base;
 
 static struct timespec persistent_ts;
 static u64 persistent_ms, last_persistent_ms;
 
 static struct delay_timer tegra_delay_timer;
+#endif
 
 #define timer_writel(value, reg) \
 	__raw_writel(value, timer_reg_base + (reg))
@@ -106,6 +110,7 @@ static u64 notrace tegra_read_sched_clock(void)
 	return timer_readl(TIMERUS_CNTR_1US);
 }
 
+#ifdef CONFIG_ARM
 /*
  * tegra_rtc_read - Reads the Tegra RTC registers
  * Care must be taken that this funciton is not called while the
@@ -146,6 +151,8 @@ static unsigned long tegra_delay_timer_read_counter_long(void)
 {
 	return readl(timer_reg_base + TIMERUS_CNTR_1US);
 }
+#endif /* CONFIG_ARM */
+
 
 static irqreturn_t tegra_timer_interrupt(int irq, void *dev_id)
 {
@@ -214,10 +221,12 @@ static void __init tegra20_init_timer(struct device_node *np)
 		BUG();
 	}
 
+#ifdef CONFIG_ARM
 	tegra_delay_timer.read_current_timer =
 			tegra_delay_timer_read_counter_long;
 	tegra_delay_timer.freq = 1000000;
 	register_current_timer_delay(&tegra_delay_timer);
+#endif
 
 	ret = setup_irq(tegra_timer_irq.irq, &tegra_timer_irq);
 	if (ret) {
@@ -232,6 +241,7 @@ static void __init tegra20_init_timer(struct device_node *np)
 }
 CLOCKSOURCE_OF_DECLARE(tegra20_timer, "nvidia,tegra20-timer", tegra20_init_timer);
 
+#ifdef CONFIG_ARM
 static void __init tegra20_init_rtc(struct device_node *np)
 {
 	struct clk *clk;
@@ -255,6 +265,7 @@ static void __init tegra20_init_rtc(struct device_node *np)
 	register_persistent_clock(NULL, tegra_read_persistent_clock);
 }
 CLOCKSOURCE_OF_DECLARE(tegra20_rtc, "nvidia,tegra20-rtc", tegra20_init_rtc);
+#endif /* CONFIG_ARM */
 
 #ifdef CONFIG_PM
 static u32 usec_config;
-- 
2.1.3

Re: [PATCH] clocksource: tegra: wrap arch/arm-specific sections in CONFIG_ARM

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3279 bytes --]

On Tue, Dec 09, 2014 at 10:07:18PM +0000, Paul Walmsley wrote:
> 
> Like several of the other files in drivers/clocksource,
> tegra20_timer.c contains code that can only compile when CONFIG_ARM is
> enabled.  This causes obvious problems when trying to compile this
> code for NVIDIA ARM64-based SoCs, such as Tegra132.  The same timer IP
> blocks exist, so it seems appropriate to provide support for them.
> 
> So until we figure out a better way to partition this code, wrap the
> delay_timer and persistent_clock support code with preprocessor tests
> for CONFIG_ARM.  (The delay_timer code should not be needed at all on
> ARM64 due to the presence of the ARMv8 architected timer.  The
> persistent_clock support code could become important once power
> management modes are implemented that turn off the CPU complex.)
> 
> Signed-off-by: Paul Walmsley <paul-DWxLp4Yu+b8AvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Paul Walmsley <pwalmsley-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> Cc: Allen Martin <amartin-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> Cc: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> Cc: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> Cc: Daniel Lezcano <daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
> Cc: Alexandre Courbot <gnurou-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> Applies against next-20141209.
> Intended for v3.20.
> Boot-tested on Tegra124 Jetson TK1 on next-20141209.
> Also boot-tested on Tegra132 Norrin FFD on next-20141209 + extra, 
> unrelated patches.
> 
>  drivers/clocksource/tegra20_timer.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)

You might want to read the following thread:

	https://lkml.org/lkml/2014/11/7/605

I haven't gotten around to look at this in detail, but it seems like we
may not need to register the RTC early here at all. If that's really the
case we can just get rid of this hackery and do everything within the
RTC driver rather than duplicate some of that code here.

On 32-bit ARM, Tegra114 and Tegra124 also support the architected timer,
so the only remaining issue would be for Tegra20 and Tegra30 hardware.
But I also seem to remember that on most boards the Tegra RTC can't be
used properly because it doesn't have a battery. So while it may still
work while the board is powered it is not very useful as RTC. Or for
timekeeping across suspend/resume for that matter. The majority of
boards seem to have a PMIC with an integrated RTC and will use that
instead.

Otherwise this looks good to me to get things going on 64-bit ARM while
we iron out the other issues. Can I assume that you plan on sending in
patches that enable 64-bit ARM Tegra in the 3.20 timeframe? If so it
might be best to take this patch through the Tegra tree, provided that
Daniel and Thomas have no objections, to make sure we don't have
intermittent build breakage depending on the order in which the trees
get pulled into linux-next and Linus' tree. Even more so because there
are a couple of other, similar patches which makes a stable branches
oriented approach to resolving these dependencies somewhat impractical.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] clocksource: tegra: wrap arch/arm-specific sections in CONFIG_ARM

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3048 bytes --]

On Tue, Dec 09, 2014 at 10:07:18PM +0000, Paul Walmsley wrote:
> 
> Like several of the other files in drivers/clocksource,
> tegra20_timer.c contains code that can only compile when CONFIG_ARM is
> enabled.  This causes obvious problems when trying to compile this
> code for NVIDIA ARM64-based SoCs, such as Tegra132.  The same timer IP
> blocks exist, so it seems appropriate to provide support for them.
> 
> So until we figure out a better way to partition this code, wrap the
> delay_timer and persistent_clock support code with preprocessor tests
> for CONFIG_ARM.  (The delay_timer code should not be needed at all on
> ARM64 due to the presence of the ARMv8 architected timer.  The
> persistent_clock support code could become important once power
> management modes are implemented that turn off the CPU complex.)
> 
> Signed-off-by: Paul Walmsley <paul@pwsan.com>
> Signed-off-by: Paul Walmsley <pwalmsley@nvidia.com>
> Cc: Allen Martin <amartin@nvidia.com>
> Cc: Stephen Warren <swarren@nvidia.com>
> Cc: Thierry Reding <treding@nvidia.com>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Alexandre Courbot <gnurou@gmail.com>
> ---
> Applies against next-20141209.
> Intended for v3.20.
> Boot-tested on Tegra124 Jetson TK1 on next-20141209.
> Also boot-tested on Tegra132 Norrin FFD on next-20141209 + extra, 
> unrelated patches.
> 
>  drivers/clocksource/tegra20_timer.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)

You might want to read the following thread:

	https://lkml.org/lkml/2014/11/7/605

I haven't gotten around to look at this in detail, but it seems like we
may not need to register the RTC early here at all. If that's really the
case we can just get rid of this hackery and do everything within the
RTC driver rather than duplicate some of that code here.

On 32-bit ARM, Tegra114 and Tegra124 also support the architected timer,
so the only remaining issue would be for Tegra20 and Tegra30 hardware.
But I also seem to remember that on most boards the Tegra RTC can't be
used properly because it doesn't have a battery. So while it may still
work while the board is powered it is not very useful as RTC. Or for
timekeeping across suspend/resume for that matter. The majority of
boards seem to have a PMIC with an integrated RTC and will use that
instead.

Otherwise this looks good to me to get things going on 64-bit ARM while
we iron out the other issues. Can I assume that you plan on sending in
patches that enable 64-bit ARM Tegra in the 3.20 timeframe? If so it
might be best to take this patch through the Tegra tree, provided that
Daniel and Thomas have no objections, to make sure we don't have
intermittent build breakage depending on the order in which the trees
get pulled into linux-next and Linus' tree. Even more so because there
are a couple of other, similar patches which makes a stable branches
oriented approach to resolving these dependencies somewhat impractical.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] clocksource: tegra: wrap arch/arm-specific sections in CONFIG_ARM

[Paul Walmsley]

+ lakml

Hello Thierry

On Wed, 10 Dec 2014, Thierry Reding wrote:

> On Tue, Dec 09, 2014 at 10:07:18PM +0000, Paul Walmsley wrote:
> > 
> > Like several of the other files in drivers/clocksource,
> > tegra20_timer.c contains code that can only compile when CONFIG_ARM is
> > enabled.  This causes obvious problems when trying to compile this
> > code for NVIDIA ARM64-based SoCs, such as Tegra132.  The same timer IP
> > blocks exist, so it seems appropriate to provide support for them.
> > 
> > So until we figure out a better way to partition this code, wrap the
> > delay_timer and persistent_clock support code with preprocessor tests
> > for CONFIG_ARM.  (The delay_timer code should not be needed at all on
> > ARM64 due to the presence of the ARMv8 architected timer.  The
> > persistent_clock support code could become important once power
> > management modes are implemented that turn off the CPU complex.)
> > 
> > Signed-off-by: Paul Walmsley <paul-DWxLp4Yu+b8AvxtiuMwx3w@public.gmane.org>
> > Signed-off-by: Paul Walmsley <pwalmsley-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > Cc: Allen Martin <amartin-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > Cc: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > Cc: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > Cc: Daniel Lezcano <daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > Cc: Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
> > Cc: Alexandre Courbot <gnurou-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > ---
> > Applies against next-20141209.
> > Intended for v3.20.
> > Boot-tested on Tegra124 Jetson TK1 on next-20141209.
> > Also boot-tested on Tegra132 Norrin FFD on next-20141209 + extra, 
> > unrelated patches.
> > 
> >  drivers/clocksource/tegra20_timer.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> 
> You might want to read the following thread:
> 
> 	https://lkml.org/lkml/2014/11/7/605

Thanks for the link, I hadn't seen that discussion.

> I haven't gotten around to look at this in detail, but it seems like we
> may not need to register the RTC early here at all. If that's really the
> case we can just get rid of this hackery and do everything within the
> RTC driver rather than duplicate some of that code here.

I'd suggest that we keep that project separate from just getting this file 
to compile on ARM64.

Broadly speaking, I agree that tegra20_timer.c needs to be cleaned up; it 
should not mix code for two distinct IP blocks into one driver.

> On 32-bit ARM, Tegra114 and Tegra124 also support the architected timer,
> so the only remaining issue would be for Tegra20 and Tegra30 hardware.

I believe on most NVIDIA chips (and probably most ARM SoCs) the ARM 
architected timer is only usable as a clockevent timer when the CPU 
doesn't enter deep low power states.  So the architected timer won't be 
enough.

> But I also seem to remember that on most boards the Tegra RTC can't be
> used properly because it doesn't have a battery. So while it may still
> work while the board is powered it is not very useful as RTC. Or for
> timekeeping across suspend/resume for that matter. The majority of
> boards seem to have a PMIC with an integrated RTC and will use that
> instead.

From my point of view, the Tegra RTC IP block's primary use case is to 
provide an always-on clockevent to wake the system from SC7 (aka LP0 
idle)[1].  In SC7, the ARM architected timer comparator logic and Tegra 
TMR IP blocks will be power-gated, and thus can't wake the system.

PMIC RTC timers are generally a bad fit for this purpose, since they tend 
to be low-resolution (+/- 1 second) and slow to program (via some slow 
serial bus).  They are good for keeping calendars, though...

> Otherwise this looks good to me to get things going on 64-bit ARM while
> we iron out the other issues. Can I assume that you plan on sending in
> patches that enable 64-bit ARM Tegra in the 3.20 timeframe? If so it
> might be best to take this patch through the Tegra tree, provided that
> Daniel and Thomas have no objections, to make sure we don't have
> intermittent build breakage depending on the order in which the trees
> get pulled into linux-next and Linus' tree. Even more so because there
> are a couple of other, similar patches which makes a stable branches
> oriented approach to resolving these dependencies somewhat impractical.

Given the rather small number of mainline T132 users, I'll probably just 
send these patches up via their corresponding trees and let them merge via 
their respective maintainers.  That keeps the risk of merge conflicts low. 
At this point I don't think there will be any significant dependency 
problems.

- Paul

1. Section 11.0 "Real-time Clock" of the _NVIDIA Tegra K1 Mobile Processor
   Technical Reference Manual v03p (DP-06905-001_v03p)_, 
   https://developer.nvidia.com/tegra-k1-technical-reference-manual

[PATCH] clocksource: tegra: wrap arch/arm-specific sections in CONFIG_ARM

[Paul Walmsley]

+ lakml

Hello Thierry

On Wed, 10 Dec 2014, Thierry Reding wrote:

> On Tue, Dec 09, 2014 at 10:07:18PM +0000, Paul Walmsley wrote:
> > 
> > Like several of the other files in drivers/clocksource,
> > tegra20_timer.c contains code that can only compile when CONFIG_ARM is
> > enabled.  This causes obvious problems when trying to compile this
> > code for NVIDIA ARM64-based SoCs, such as Tegra132.  The same timer IP
> > blocks exist, so it seems appropriate to provide support for them.
> > 
> > So until we figure out a better way to partition this code, wrap the
> > delay_timer and persistent_clock support code with preprocessor tests
> > for CONFIG_ARM.  (The delay_timer code should not be needed at all on
> > ARM64 due to the presence of the ARMv8 architected timer.  The
> > persistent_clock support code could become important once power
> > management modes are implemented that turn off the CPU complex.)
> > 
> > Signed-off-by: Paul Walmsley <paul@pwsan.com>
> > Signed-off-by: Paul Walmsley <pwalmsley@nvidia.com>
> > Cc: Allen Martin <amartin@nvidia.com>
> > Cc: Stephen Warren <swarren@nvidia.com>
> > Cc: Thierry Reding <treding@nvidia.com>
> > Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Alexandre Courbot <gnurou@gmail.com>
> > ---
> > Applies against next-20141209.
> > Intended for v3.20.
> > Boot-tested on Tegra124 Jetson TK1 on next-20141209.
> > Also boot-tested on Tegra132 Norrin FFD on next-20141209 + extra, 
> > unrelated patches.
> > 
> >  drivers/clocksource/tegra20_timer.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> 
> You might want to read the following thread:
> 
> 	https://lkml.org/lkml/2014/11/7/605

Thanks for the link, I hadn't seen that discussion.

> I haven't gotten around to look at this in detail, but it seems like we
> may not need to register the RTC early here at all. If that's really the
> case we can just get rid of this hackery and do everything within the
> RTC driver rather than duplicate some of that code here.

I'd suggest that we keep that project separate from just getting this file 
to compile on ARM64.

Broadly speaking, I agree that tegra20_timer.c needs to be cleaned up; it 
should not mix code for two distinct IP blocks into one driver.

> On 32-bit ARM, Tegra114 and Tegra124 also support the architected timer,
> so the only remaining issue would be for Tegra20 and Tegra30 hardware.

I believe on most NVIDIA chips (and probably most ARM SoCs) the ARM 
architected timer is only usable as a clockevent timer when the CPU 
doesn't enter deep low power states.  So the architected timer won't be 
enough.

> But I also seem to remember that on most boards the Tegra RTC can't be
> used properly because it doesn't have a battery. So while it may still
> work while the board is powered it is not very useful as RTC. Or for
> timekeeping across suspend/resume for that matter. The majority of
> boards seem to have a PMIC with an integrated RTC and will use that
> instead.

>From my point of view, the Tegra RTC IP block's primary use case is to 
provide an always-on clockevent to wake the system from SC7 (aka LP0 
idle)[1].  In SC7, the ARM architected timer comparator logic and Tegra 
TMR IP blocks will be power-gated, and thus can't wake the system.

PMIC RTC timers are generally a bad fit for this purpose, since they tend 
to be low-resolution (+/- 1 second) and slow to program (via some slow 
serial bus).  They are good for keeping calendars, though...

> Otherwise this looks good to me to get things going on 64-bit ARM while
> we iron out the other issues. Can I assume that you plan on sending in
> patches that enable 64-bit ARM Tegra in the 3.20 timeframe? If so it
> might be best to take this patch through the Tegra tree, provided that
> Daniel and Thomas have no objections, to make sure we don't have
> intermittent build breakage depending on the order in which the trees
> get pulled into linux-next and Linus' tree. Even more so because there
> are a couple of other, similar patches which makes a stable branches
> oriented approach to resolving these dependencies somewhat impractical.

Given the rather small number of mainline T132 users, I'll probably just 
send these patches up via their corresponding trees and let them merge via 
their respective maintainers.  That keeps the risk of merge conflicts low. 
At this point I don't think there will be any significant dependency 
problems.

- Paul

1. Section 11.0 "Real-time Clock" of the _NVIDIA Tegra K1 Mobile Processor
   Technical Reference Manual v03p (DP-06905-001_v03p)_, 
   https://developer.nvidia.com/tegra-k1-technical-reference-manual

Re: [PATCH] clocksource: tegra: wrap arch/arm-specific sections in CONFIG_ARM

[Paul Walmsley]

+ lakml

Hello Thierry

On Wed, 10 Dec 2014, Thierry Reding wrote:

> On Tue, Dec 09, 2014 at 10:07:18PM +0000, Paul Walmsley wrote:
> > 
> > Like several of the other files in drivers/clocksource,
> > tegra20_timer.c contains code that can only compile when CONFIG_ARM is
> > enabled.  This causes obvious problems when trying to compile this
> > code for NVIDIA ARM64-based SoCs, such as Tegra132.  The same timer IP
> > blocks exist, so it seems appropriate to provide support for them.
> > 
> > So until we figure out a better way to partition this code, wrap the
> > delay_timer and persistent_clock support code with preprocessor tests
> > for CONFIG_ARM.  (The delay_timer code should not be needed at all on
> > ARM64 due to the presence of the ARMv8 architected timer.  The
> > persistent_clock support code could become important once power
> > management modes are implemented that turn off the CPU complex.)
> > 
> > Signed-off-by: Paul Walmsley <paul@pwsan.com>
> > Signed-off-by: Paul Walmsley <pwalmsley@nvidia.com>
> > Cc: Allen Martin <amartin@nvidia.com>
> > Cc: Stephen Warren <swarren@nvidia.com>
> > Cc: Thierry Reding <treding@nvidia.com>
> > Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Alexandre Courbot <gnurou@gmail.com>
> > ---
> > Applies against next-20141209.
> > Intended for v3.20.
> > Boot-tested on Tegra124 Jetson TK1 on next-20141209.
> > Also boot-tested on Tegra132 Norrin FFD on next-20141209 + extra, 
> > unrelated patches.
> > 
> >  drivers/clocksource/tegra20_timer.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> 
> You might want to read the following thread:
> 
> 	https://lkml.org/lkml/2014/11/7/605

Thanks for the link, I hadn't seen that discussion.

> I haven't gotten around to look at this in detail, but it seems like we
> may not need to register the RTC early here at all. If that's really the
> case we can just get rid of this hackery and do everything within the
> RTC driver rather than duplicate some of that code here.

I'd suggest that we keep that project separate from just getting this file 
to compile on ARM64.

Broadly speaking, I agree that tegra20_timer.c needs to be cleaned up; it 
should not mix code for two distinct IP blocks into one driver.

> On 32-bit ARM, Tegra114 and Tegra124 also support the architected timer,
> so the only remaining issue would be for Tegra20 and Tegra30 hardware.

I believe on most NVIDIA chips (and probably most ARM SoCs) the ARM 
architected timer is only usable as a clockevent timer when the CPU 
doesn't enter deep low power states.  So the architected timer won't be 
enough.

> But I also seem to remember that on most boards the Tegra RTC can't be
> used properly because it doesn't have a battery. So while it may still
> work while the board is powered it is not very useful as RTC. Or for
> timekeeping across suspend/resume for that matter. The majority of
> boards seem to have a PMIC with an integrated RTC and will use that
> instead.

>From my point of view, the Tegra RTC IP block's primary use case is to 
provide an always-on clockevent to wake the system from SC7 (aka LP0 
idle)[1].  In SC7, the ARM architected timer comparator logic and Tegra 
TMR IP blocks will be power-gated, and thus can't wake the system.

PMIC RTC timers are generally a bad fit for this purpose, since they tend 
to be low-resolution (+/- 1 second) and slow to program (via some slow 
serial bus).  They are good for keeping calendars, though...

> Otherwise this looks good to me to get things going on 64-bit ARM while
> we iron out the other issues. Can I assume that you plan on sending in
> patches that enable 64-bit ARM Tegra in the 3.20 timeframe? If so it
> might be best to take this patch through the Tegra tree, provided that
> Daniel and Thomas have no objections, to make sure we don't have
> intermittent build breakage depending on the order in which the trees
> get pulled into linux-next and Linus' tree. Even more so because there
> are a couple of other, similar patches which makes a stable branches
> oriented approach to resolving these dependencies somewhat impractical.

Given the rather small number of mainline T132 users, I'll probably just 
send these patches up via their corresponding trees and let them merge via 
their respective maintainers.  That keeps the risk of merge conflicts low. 
At this point I don't think there will be any significant dependency 
problems.

- Paul

1. Section 11.0 "Real-time Clock" of the _NVIDIA Tegra K1 Mobile Processor
   Technical Reference Manual v03p (DP-06905-001_v03p)_, 
   https://developer.nvidia.com/tegra-k1-technical-reference-manual

Re: [PATCH] clocksource: tegra: wrap arch/arm-specific sections in CONFIG_ARM

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2062 bytes --]

On Tue, Dec 09, 2014 at 10:07:18PM +0000, Paul Walmsley wrote:
> 
> Like several of the other files in drivers/clocksource,
> tegra20_timer.c contains code that can only compile when CONFIG_ARM is
> enabled.  This causes obvious problems when trying to compile this
> code for NVIDIA ARM64-based SoCs, such as Tegra132.  The same timer IP
> blocks exist, so it seems appropriate to provide support for them.
> 
> So until we figure out a better way to partition this code, wrap the
> delay_timer and persistent_clock support code with preprocessor tests
> for CONFIG_ARM.  (The delay_timer code should not be needed at all on
> ARM64 due to the presence of the ARMv8 architected timer.  The
> persistent_clock support code could become important once power
> management modes are implemented that turn off the CPU complex.)
> 
> Signed-off-by: Paul Walmsley <paul-DWxLp4Yu+b8AvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Paul Walmsley <pwalmsley-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> Cc: Allen Martin <amartin-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> Cc: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> Cc: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> Cc: Daniel Lezcano <daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
> Cc: Alexandre Courbot <gnurou-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> Applies against next-20141209.
> Intended for v3.20.
> Boot-tested on Tegra124 Jetson TK1 on next-20141209.
> Also boot-tested on Tegra132 Norrin FFD on next-20141209 + extra, 
> unrelated patches.
> 
>  drivers/clocksource/tegra20_timer.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)

Daniel, Thomas,

This patch is a build dependency for 64-bit Tegra support that I'd like
to merge for v3.20. Would you be willing to give your Acked-by on this
so that I can take it into the same tree along with some other
dependencies and the patches that enable the 64-bit build?

Thanks,
Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] clocksource: tegra: wrap arch/arm-specific sections in CONFIG_ARM

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1831 bytes --]

On Tue, Dec 09, 2014 at 10:07:18PM +0000, Paul Walmsley wrote:
> 
> Like several of the other files in drivers/clocksource,
> tegra20_timer.c contains code that can only compile when CONFIG_ARM is
> enabled.  This causes obvious problems when trying to compile this
> code for NVIDIA ARM64-based SoCs, such as Tegra132.  The same timer IP
> blocks exist, so it seems appropriate to provide support for them.
> 
> So until we figure out a better way to partition this code, wrap the
> delay_timer and persistent_clock support code with preprocessor tests
> for CONFIG_ARM.  (The delay_timer code should not be needed at all on
> ARM64 due to the presence of the ARMv8 architected timer.  The
> persistent_clock support code could become important once power
> management modes are implemented that turn off the CPU complex.)
> 
> Signed-off-by: Paul Walmsley <paul@pwsan.com>
> Signed-off-by: Paul Walmsley <pwalmsley@nvidia.com>
> Cc: Allen Martin <amartin@nvidia.com>
> Cc: Stephen Warren <swarren@nvidia.com>
> Cc: Thierry Reding <treding@nvidia.com>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Alexandre Courbot <gnurou@gmail.com>
> ---
> Applies against next-20141209.
> Intended for v3.20.
> Boot-tested on Tegra124 Jetson TK1 on next-20141209.
> Also boot-tested on Tegra132 Norrin FFD on next-20141209 + extra, 
> unrelated patches.
> 
>  drivers/clocksource/tegra20_timer.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)

Daniel, Thomas,

This patch is a build dependency for 64-bit Tegra support that I'd like
to merge for v3.20. Would you be willing to give your Acked-by on this
so that I can take it into the same tree along with some other
dependencies and the patches that enable the 64-bit build?

Thanks,
Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] clocksource: tegra: wrap arch/arm-specific sections in CONFIG_ARM

[Daniel Lezcano]

On 12/09/2014 11:07 PM, Paul Walmsley wrote:
>
> Like several of the other files in drivers/clocksource,
> tegra20_timer.c contains code that can only compile when CONFIG_ARM is
> enabled.  This causes obvious problems when trying to compile this
> code for NVIDIA ARM64-based SoCs, such as Tegra132.  The same timer IP
> blocks exist, so it seems appropriate to provide support for them.
>
> So until we figure out a better way to partition this code, wrap the
> delay_timer and persistent_clock support code with preprocessor tests
> for CONFIG_ARM.
 >
>  (The delay_timer code should not be needed at all on
> ARM64 due to the presence of the ARMv8 architected timer.  The
> persistent_clock support code could become important once power
> management modes are implemented that turn off the CPU complex.)

IIUC, the cpuidle driver is not yet ready, right ?

If it is the case, this driver is not needed yet, no ?

Perhaps you can rework a bit this driver in the meantime to have a 
better fix than disabling the code with macros ?

Otherwise, please try at least to group the code into a minimal set of 
macros.

One comment below.

> Signed-off-by: Paul Walmsley <paul-DWxLp4Yu+b8AvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Paul Walmsley <pwalmsley-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> Cc: Allen Martin <amartin-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> Cc: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> Cc: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> Cc: Daniel Lezcano <daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
> Cc: Alexandre Courbot <gnurou-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> Applies against next-20141209.
> Intended for v3.20.
> Boot-tested on Tegra124 Jetson TK1 on next-20141209.
> Also boot-tested on Tegra132 Norrin FFD on next-20141209 + extra,
> unrelated patches.
>
>   drivers/clocksource/tegra20_timer.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
>
> diff --git a/drivers/clocksource/tegra20_timer.c b/drivers/clocksource/tegra20_timer.c
> index d2616ef16770..83a8f5c9e139 100644
> --- a/drivers/clocksource/tegra20_timer.c
> +++ b/drivers/clocksource/tegra20_timer.c
> @@ -29,8 +29,10 @@
>   #include <linux/sched_clock.h>
>   #include <linux/delay.h>
>
> +#ifdef CONFIG_ARM
>   #include <asm/mach/time.h>
>   #include <asm/smp_twd.h>

Is smp_twd.h really needed ?

> +#endif

[ ... ]

Thanks
   -- Daniel

-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH] clocksource: tegra: wrap arch/arm-specific sections in CONFIG_ARM

[Daniel Lezcano]

On 12/09/2014 11:07 PM, Paul Walmsley wrote:
>
> Like several of the other files in drivers/clocksource,
> tegra20_timer.c contains code that can only compile when CONFIG_ARM is
> enabled.  This causes obvious problems when trying to compile this
> code for NVIDIA ARM64-based SoCs, such as Tegra132.  The same timer IP
> blocks exist, so it seems appropriate to provide support for them.
>
> So until we figure out a better way to partition this code, wrap the
> delay_timer and persistent_clock support code with preprocessor tests
> for CONFIG_ARM.
 >
>  (The delay_timer code should not be needed at all on
> ARM64 due to the presence of the ARMv8 architected timer.  The
> persistent_clock support code could become important once power
> management modes are implemented that turn off the CPU complex.)

IIUC, the cpuidle driver is not yet ready, right ?

If it is the case, this driver is not needed yet, no ?

Perhaps you can rework a bit this driver in the meantime to have a 
better fix than disabling the code with macros ?

Otherwise, please try at least to group the code into a minimal set of 
macros.

One comment below.

> Signed-off-by: Paul Walmsley <paul@pwsan.com>
> Signed-off-by: Paul Walmsley <pwalmsley@nvidia.com>
> Cc: Allen Martin <amartin@nvidia.com>
> Cc: Stephen Warren <swarren@nvidia.com>
> Cc: Thierry Reding <treding@nvidia.com>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Alexandre Courbot <gnurou@gmail.com>
> ---
> Applies against next-20141209.
> Intended for v3.20.
> Boot-tested on Tegra124 Jetson TK1 on next-20141209.
> Also boot-tested on Tegra132 Norrin FFD on next-20141209 + extra,
> unrelated patches.
>
>   drivers/clocksource/tegra20_timer.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
>
> diff --git a/drivers/clocksource/tegra20_timer.c b/drivers/clocksource/tegra20_timer.c
> index d2616ef16770..83a8f5c9e139 100644
> --- a/drivers/clocksource/tegra20_timer.c
> +++ b/drivers/clocksource/tegra20_timer.c
> @@ -29,8 +29,10 @@
>   #include <linux/sched_clock.h>
>   #include <linux/delay.h>
>
> +#ifdef CONFIG_ARM
>   #include <asm/mach/time.h>
>   #include <asm/smp_twd.h>

Is smp_twd.h really needed ?

> +#endif

[ ... ]

Thanks
   -- Daniel

-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH] clocksource: tegra: wrap arch/arm-specific sections in CONFIG_ARM

[Paul Walmsley]

Hello Daniel

On Thu, 8 Jan 2015, Daniel Lezcano wrote:

> On 12/09/2014 11:07 PM, Paul Walmsley wrote:
> > 
> > Like several of the other files in drivers/clocksource,
> > tegra20_timer.c contains code that can only compile when CONFIG_ARM is
> > enabled.  This causes obvious problems when trying to compile this
> > code for NVIDIA ARM64-based SoCs, such as Tegra132.  The same timer IP
> > blocks exist, so it seems appropriate to provide support for them.
> > 
> > So until we figure out a better way to partition this code, wrap the
> > delay_timer and persistent_clock support code with preprocessor tests
> > for CONFIG_ARM.
> >
> >  (The delay_timer code should not be needed at all on
> > ARM64 due to the presence of the ARMv8 architected timer.  The
> > persistent_clock support code could become important once power
> > management modes are implemented that turn off the CPU complex.)
> 
> IIUC, the cpuidle driver is not yet ready, right ?
> 
> If it is the case, this driver is not needed yet, no ?

The point of the patch is to allow the hardware drivers selected by 
CONFIG_ARCH_TEGRA to build for an arm64 kernel, just as they build for 
32-bit ARM.  

There's nothing CPUIdle-specific about the patch - that is, this timer can 
be selected as a clockevent and clocksource provider without the use of 
CPUIdle - although low-power PM idle is likely to be a primary use-case.

> Perhaps you can rework a bit this driver in the meantime to have a better fix
> than disabling the code with macros ?

I'm happy to do that, but it would be nice to get the driver compiling 
first for ARM64 :-)

> Otherwise, please try at least to group the code into a minimal set of macros.

So, would it be accurate to say that you would prefer a patch that changes 
more lines of code, but minimizes preprocessor directives, to the current 
patch?

> One comment below.

> > diff --git a/drivers/clocksource/tegra20_timer.c
> > b/drivers/clocksource/tegra20_timer.c
> > index d2616ef16770..83a8f5c9e139 100644
> > --- a/drivers/clocksource/tegra20_timer.c
> > +++ b/drivers/clocksource/tegra20_timer.c
> > @@ -29,8 +29,10 @@
> >   #include <linux/sched_clock.h>
> >   #include <linux/delay.h>
> > 
> > +#ifdef CONFIG_ARM
> >   #include <asm/mach/time.h>
> >   #include <asm/smp_twd.h>
> 
> Is smp_twd.h really needed ?
> 
> > +#endif

No, it can be removed.  

Would you be willing to ack or merge a revision of this patch with 

1. the #include <asm/smp_twd.h> removed

2. a larger number of changed lines, in order to minimize the number of 
new #ifdefs?




- Paul

Re: [PATCH] clocksource: tegra: wrap arch/arm-specific sections in CONFIG_ARM

[Daniel Lezcano]

On 01/09/2015 03:09 AM, Paul Walmsley wrote:
> Hello Daniel
>
> On Thu, 8 Jan 2015, Daniel Lezcano wrote:
>
>> On 12/09/2014 11:07 PM, Paul Walmsley wrote:
>>>
>>> Like several of the other files in drivers/clocksource,
>>> tegra20_timer.c contains code that can only compile when CONFIG_ARM is
>>> enabled.  This causes obvious problems when trying to compile this
>>> code for NVIDIA ARM64-based SoCs, such as Tegra132.  The same timer IP
>>> blocks exist, so it seems appropriate to provide support for them.
>>>
>>> So until we figure out a better way to partition this code, wrap the
>>> delay_timer and persistent_clock support code with preprocessor tests
>>> for CONFIG_ARM.
>>>
>>>   (The delay_timer code should not be needed at all on
>>> ARM64 due to the presence of the ARMv8 architected timer.  The
>>> persistent_clock support code could become important once power
>>> management modes are implemented that turn off the CPU complex.)
>>
>> IIUC, the cpuidle driver is not yet ready, right ?
>>
>> If it is the case, this driver is not needed yet, no ?
>
> The point of the patch is to allow the hardware drivers selected by
> CONFIG_ARCH_TEGRA to build for an arm64 kernel, just as they build for
> 32-bit ARM.
>
> There's nothing CPUIdle-specific about the patch - that is, this timer can
> be selected as a clockevent and clocksource provider without the use of
> CPUIdle - although low-power PM idle is likely to be a primary use-case.

What I meant is this timer is not needed for the moment.

>> Perhaps you can rework a bit this driver in the meantime to have a better fix
>> than disabling the code with macros ?
>
> I'm happy to do that, but it would be nice to get the driver compiling
> first for ARM64 :-)
>
>> Otherwise, please try at least to group the code into a minimal set of macros.
>
> So, would it be accurate to say that you would prefer a patch that changes
> more lines of code, but minimizes preprocessor directives, to the current
> patch?

Yes at least an attempt to factor out a bit the driver. Those #ifdef are 
like #if 0, which is a quick fix. I am not strongly against this patch, 
but it would be nice to take the opportunity to reorganize it a bit.

>> One comment below.
>
>>> diff --git a/drivers/clocksource/tegra20_timer.c
>>> b/drivers/clocksource/tegra20_timer.c
>>> index d2616ef16770..83a8f5c9e139 100644
>>> --- a/drivers/clocksource/tegra20_timer.c
>>> +++ b/drivers/clocksource/tegra20_timer.c
>>> @@ -29,8 +29,10 @@
>>>    #include <linux/sched_clock.h>
>>>    #include <linux/delay.h>
>>>
>>> +#ifdef CONFIG_ARM
>>>    #include <asm/mach/time.h>
>>>    #include <asm/smp_twd.h>
>>
>> Is smp_twd.h really needed ?
>>
>>> +#endif
>
> No, it can be removed.
>
> Would you be willing to ack or merge a revision of this patch with
>
> 1. the #include <asm/smp_twd.h> removed
>
> 2. a larger number of changed lines, in order to minimize the number of
> new #ifdefs?

Yes.

Thanks

   -- Daniel


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH] clocksource: tegra: wrap arch/arm-specific sections in CONFIG_ARM

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2748 bytes --]

On Fri, Jan 09, 2015 at 09:31:08AM +0100, Daniel Lezcano wrote:
> On 01/09/2015 03:09 AM, Paul Walmsley wrote:
> >Hello Daniel
> >
> >On Thu, 8 Jan 2015, Daniel Lezcano wrote:
> >
> >>On 12/09/2014 11:07 PM, Paul Walmsley wrote:
> >>>
> >>>Like several of the other files in drivers/clocksource,
> >>>tegra20_timer.c contains code that can only compile when CONFIG_ARM is
> >>>enabled.  This causes obvious problems when trying to compile this
> >>>code for NVIDIA ARM64-based SoCs, such as Tegra132.  The same timer IP
> >>>blocks exist, so it seems appropriate to provide support for them.
> >>>
> >>>So until we figure out a better way to partition this code, wrap the
> >>>delay_timer and persistent_clock support code with preprocessor tests
> >>>for CONFIG_ARM.
> >>>
> >>>  (The delay_timer code should not be needed at all on
> >>>ARM64 due to the presence of the ARMv8 architected timer.  The
> >>>persistent_clock support code could become important once power
> >>>management modes are implemented that turn off the CPU complex.)
> >>
> >>IIUC, the cpuidle driver is not yet ready, right ?
> >>
> >>If it is the case, this driver is not needed yet, no ?
> >
> >The point of the patch is to allow the hardware drivers selected by
> >CONFIG_ARCH_TEGRA to build for an arm64 kernel, just as they build for
> >32-bit ARM.
> >
> >There's nothing CPUIdle-specific about the patch - that is, this timer can
> >be selected as a clockevent and clocksource provider without the use of
> >CPUIdle - although low-power PM idle is likely to be a primary use-case.
> 
> What I meant is this timer is not needed for the moment.
> 
> >>Perhaps you can rework a bit this driver in the meantime to have a better fix
> >>than disabling the code with macros ?
> >
> >I'm happy to do that, but it would be nice to get the driver compiling
> >first for ARM64 :-)
> >
> >>Otherwise, please try at least to group the code into a minimal set of macros.
> >
> >So, would it be accurate to say that you would prefer a patch that changes
> >more lines of code, but minimizes preprocessor directives, to the current
> >patch?
> 
> Yes at least an attempt to factor out a bit the driver. Those #ifdef are
> like #if 0, which is a quick fix. I am not strongly against this patch, but
> it would be nice to take the opportunity to reorganize it a bit.

How about we do something like the attached patch instead for now. That
avoids any #ifdef'ery and still we don't attempt (and fail) to build the
driver on 64-bit ARM.

With that applied we can incrementally make the changes to untangle the
ARM-specific parts and when the driver can build on 64-bit ARM we simply
select TEGRA_TIMER via Kconfig.

Thierry

[-- Attachment #1.2: 0001-clocksource-Build-Tegra-timer-on-32-bit-ARM-only.patch --]
[-- Type: text/x-diff, Size: 3103 bytes --]

From dccf6b8e0dcf5695eb29a8749406472e578b972e Mon Sep 17 00:00:00 2001
From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Date: Mon, 7 Jul 2014 15:26:30 +0200
Subject: [PATCH] clocksource: Build Tegra timer on 32-bit ARM only

Instead of directly using the ARCH_TEGRA Kconfig symbol to enable this
driver, add a new, non-user-visible Kconfig symbol (TEGRA_TIMER) which
can be selected by the various SoCs.

This is useful to disable building the driver on Tegra132 (64-bit ARM)
where it doesn't currently compile but also isn't needed (yet).

Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/mach-tegra/Kconfig  | 4 ++++
 drivers/clocksource/Kconfig  | 3 +++
 drivers/clocksource/Makefile | 2 +-
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-tegra/Kconfig b/arch/arm/mach-tegra/Kconfig
index f87684e600c7..667a48e2f7d4 100644
--- a/arch/arm/mach-tegra/Kconfig
+++ b/arch/arm/mach-tegra/Kconfig
@@ -29,6 +29,7 @@ config ARCH_TEGRA_2x_SOC
 	select PINCTRL_TEGRA20
 	select PL310_ERRATA_727915 if CACHE_L2X0
 	select PL310_ERRATA_769419 if CACHE_L2X0
+	select TEGRA_TIMER
 	help
 	  Support for NVIDIA Tegra AP20 and T20 processors, based on the
 	  ARM CortexA9MP CPU and the ARM PL310 L2 cache controller
@@ -39,6 +40,7 @@ config ARCH_TEGRA_3x_SOC
 	select ARM_ERRATA_764369 if SMP
 	select PINCTRL_TEGRA30
 	select PL310_ERRATA_769419 if CACHE_L2X0
+	select TEGRA_TIMER
 	help
 	  Support for NVIDIA Tegra T30 processor family, based on the
 	  ARM CortexA9MP CPU and the ARM PL310 L2 cache controller
@@ -49,6 +51,7 @@ config ARCH_TEGRA_114_SOC
 	select ARM_L1_CACHE_SHIFT_6
 	select HAVE_ARM_ARCH_TIMER
 	select PINCTRL_TEGRA114
+	select TEGRA_TIMER
 	help
 	  Support for NVIDIA Tegra T114 processor family, based on the
 	  ARM CortexA15MP CPU
@@ -58,6 +61,7 @@ config ARCH_TEGRA_124_SOC
 	select ARM_L1_CACHE_SHIFT_6
 	select HAVE_ARM_ARCH_TIMER
 	select PINCTRL_TEGRA124
+	select TEGRA_TIMER
 	help
 	  Support for NVIDIA Tegra T124 processor family, based on the
 	  ARM CortexA15MP CPU
diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index fc01ec27d3c8..c062b6105d49 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -47,6 +47,9 @@ config SUN5I_HSTIMER
 	select CLKSRC_MMIO
 	bool
 
+config TEGRA_TIMER
+	bool
+
 config VT8500_TIMER
 	bool
 
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 94d90b24b56b..ba9ebd868ec5 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -27,7 +27,7 @@ obj-$(CONFIG_ARCH_U300)		+= timer-u300.o
 obj-$(CONFIG_SUN4I_TIMER)	+= sun4i_timer.o
 obj-$(CONFIG_SUN5I_HSTIMER)	+= timer-sun5i.o
 obj-$(CONFIG_MESON6_TIMER)	+= meson6_timer.o
-obj-$(CONFIG_ARCH_TEGRA)	+= tegra20_timer.o
+obj-$(CONFIG_TEGRA_TIMER)	+= tegra20_timer.o
 obj-$(CONFIG_VT8500_TIMER)	+= vt8500_timer.o
 obj-$(CONFIG_ARCH_NSPIRE)	+= zevio-timer.o
 obj-$(CONFIG_ARCH_BCM_MOBILE)	+= bcm_kona_timer.o
-- 
2.1.3


[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] clocksource: tegra: wrap arch/arm-specific sections in CONFIG_ARM

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2748 bytes --]

On Fri, Jan 09, 2015 at 09:31:08AM +0100, Daniel Lezcano wrote:
> On 01/09/2015 03:09 AM, Paul Walmsley wrote:
> >Hello Daniel
> >
> >On Thu, 8 Jan 2015, Daniel Lezcano wrote:
> >
> >>On 12/09/2014 11:07 PM, Paul Walmsley wrote:
> >>>
> >>>Like several of the other files in drivers/clocksource,
> >>>tegra20_timer.c contains code that can only compile when CONFIG_ARM is
> >>>enabled.  This causes obvious problems when trying to compile this
> >>>code for NVIDIA ARM64-based SoCs, such as Tegra132.  The same timer IP
> >>>blocks exist, so it seems appropriate to provide support for them.
> >>>
> >>>So until we figure out a better way to partition this code, wrap the
> >>>delay_timer and persistent_clock support code with preprocessor tests
> >>>for CONFIG_ARM.
> >>>
> >>>  (The delay_timer code should not be needed at all on
> >>>ARM64 due to the presence of the ARMv8 architected timer.  The
> >>>persistent_clock support code could become important once power
> >>>management modes are implemented that turn off the CPU complex.)
> >>
> >>IIUC, the cpuidle driver is not yet ready, right ?
> >>
> >>If it is the case, this driver is not needed yet, no ?
> >
> >The point of the patch is to allow the hardware drivers selected by
> >CONFIG_ARCH_TEGRA to build for an arm64 kernel, just as they build for
> >32-bit ARM.
> >
> >There's nothing CPUIdle-specific about the patch - that is, this timer can
> >be selected as a clockevent and clocksource provider without the use of
> >CPUIdle - although low-power PM idle is likely to be a primary use-case.
> 
> What I meant is this timer is not needed for the moment.
> 
> >>Perhaps you can rework a bit this driver in the meantime to have a better fix
> >>than disabling the code with macros ?
> >
> >I'm happy to do that, but it would be nice to get the driver compiling
> >first for ARM64 :-)
> >
> >>Otherwise, please try at least to group the code into a minimal set of macros.
> >
> >So, would it be accurate to say that you would prefer a patch that changes
> >more lines of code, but minimizes preprocessor directives, to the current
> >patch?
> 
> Yes at least an attempt to factor out a bit the driver. Those #ifdef are
> like #if 0, which is a quick fix. I am not strongly against this patch, but
> it would be nice to take the opportunity to reorganize it a bit.

How about we do something like the attached patch instead for now. That
avoids any #ifdef'ery and still we don't attempt (and fail) to build the
driver on 64-bit ARM.

With that applied we can incrementally make the changes to untangle the
ARM-specific parts and when the driver can build on 64-bit ARM we simply
select TEGRA_TIMER via Kconfig.

Thierry

[-- Attachment #1.2: 0001-clocksource-Build-Tegra-timer-on-32-bit-ARM-only.patch --]
[-- Type: text/x-diff, Size: 3045 bytes --]

From dccf6b8e0dcf5695eb29a8749406472e578b972e Mon Sep 17 00:00:00 2001
From: Thierry Reding <treding@nvidia.com>
Date: Mon, 7 Jul 2014 15:26:30 +0200
Subject: [PATCH] clocksource: Build Tegra timer on 32-bit ARM only

Instead of directly using the ARCH_TEGRA Kconfig symbol to enable this
driver, add a new, non-user-visible Kconfig symbol (TEGRA_TIMER) which
can be selected by the various SoCs.

This is useful to disable building the driver on Tegra132 (64-bit ARM)
where it doesn't currently compile but also isn't needed (yet).

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 arch/arm/mach-tegra/Kconfig  | 4 ++++
 drivers/clocksource/Kconfig  | 3 +++
 drivers/clocksource/Makefile | 2 +-
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-tegra/Kconfig b/arch/arm/mach-tegra/Kconfig
index f87684e600c7..667a48e2f7d4 100644
--- a/arch/arm/mach-tegra/Kconfig
+++ b/arch/arm/mach-tegra/Kconfig
@@ -29,6 +29,7 @@ config ARCH_TEGRA_2x_SOC
 	select PINCTRL_TEGRA20
 	select PL310_ERRATA_727915 if CACHE_L2X0
 	select PL310_ERRATA_769419 if CACHE_L2X0
+	select TEGRA_TIMER
 	help
 	  Support for NVIDIA Tegra AP20 and T20 processors, based on the
 	  ARM CortexA9MP CPU and the ARM PL310 L2 cache controller
@@ -39,6 +40,7 @@ config ARCH_TEGRA_3x_SOC
 	select ARM_ERRATA_764369 if SMP
 	select PINCTRL_TEGRA30
 	select PL310_ERRATA_769419 if CACHE_L2X0
+	select TEGRA_TIMER
 	help
 	  Support for NVIDIA Tegra T30 processor family, based on the
 	  ARM CortexA9MP CPU and the ARM PL310 L2 cache controller
@@ -49,6 +51,7 @@ config ARCH_TEGRA_114_SOC
 	select ARM_L1_CACHE_SHIFT_6
 	select HAVE_ARM_ARCH_TIMER
 	select PINCTRL_TEGRA114
+	select TEGRA_TIMER
 	help
 	  Support for NVIDIA Tegra T114 processor family, based on the
 	  ARM CortexA15MP CPU
@@ -58,6 +61,7 @@ config ARCH_TEGRA_124_SOC
 	select ARM_L1_CACHE_SHIFT_6
 	select HAVE_ARM_ARCH_TIMER
 	select PINCTRL_TEGRA124
+	select TEGRA_TIMER
 	help
 	  Support for NVIDIA Tegra T124 processor family, based on the
 	  ARM CortexA15MP CPU
diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index fc01ec27d3c8..c062b6105d49 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -47,6 +47,9 @@ config SUN5I_HSTIMER
 	select CLKSRC_MMIO
 	bool
 
+config TEGRA_TIMER
+	bool
+
 config VT8500_TIMER
 	bool
 
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 94d90b24b56b..ba9ebd868ec5 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -27,7 +27,7 @@ obj-$(CONFIG_ARCH_U300)		+= timer-u300.o
 obj-$(CONFIG_SUN4I_TIMER)	+= sun4i_timer.o
 obj-$(CONFIG_SUN5I_HSTIMER)	+= timer-sun5i.o
 obj-$(CONFIG_MESON6_TIMER)	+= meson6_timer.o
-obj-$(CONFIG_ARCH_TEGRA)	+= tegra20_timer.o
+obj-$(CONFIG_TEGRA_TIMER)	+= tegra20_timer.o
 obj-$(CONFIG_VT8500_TIMER)	+= vt8500_timer.o
 obj-$(CONFIG_ARCH_NSPIRE)	+= zevio-timer.o
 obj-$(CONFIG_ARCH_BCM_MOBILE)	+= bcm_kona_timer.o
-- 
2.1.3


[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] clocksource: tegra: wrap arch/arm-specific sections in CONFIG_ARM

[Daniel Lezcano]

On 01/09/2015 01:21 PM, Thierry Reding wrote:
> On Fri, Jan 09, 2015 at 09:31:08AM +0100, Daniel Lezcano wrote:
>> On 01/09/2015 03:09 AM, Paul Walmsley wrote:
>>> Hello Daniel
>>>
>>> On Thu, 8 Jan 2015, Daniel Lezcano wrote:
>>>
>>>> On 12/09/2014 11:07 PM, Paul Walmsley wrote:
>>>>>
>>>>> Like several of the other files in drivers/clocksource,
>>>>> tegra20_timer.c contains code that can only compile when CONFIG_ARM is
>>>>> enabled.  This causes obvious problems when trying to compile this
>>>>> code for NVIDIA ARM64-based SoCs, such as Tegra132.  The same timer IP
>>>>> blocks exist, so it seems appropriate to provide support for them.
>>>>>
>>>>> So until we figure out a better way to partition this code, wrap the
>>>>> delay_timer and persistent_clock support code with preprocessor tests
>>>>> for CONFIG_ARM.
>>>>>
>>>>>   (The delay_timer code should not be needed at all on
>>>>> ARM64 due to the presence of the ARMv8 architected timer.  The
>>>>> persistent_clock support code could become important once power
>>>>> management modes are implemented that turn off the CPU complex.)
>>>>
>>>> IIUC, the cpuidle driver is not yet ready, right ?
>>>>
>>>> If it is the case, this driver is not needed yet, no ?
>>>
>>> The point of the patch is to allow the hardware drivers selected by
>>> CONFIG_ARCH_TEGRA to build for an arm64 kernel, just as they build for
>>> 32-bit ARM.
>>>
>>> There's nothing CPUIdle-specific about the patch - that is, this timer can
>>> be selected as a clockevent and clocksource provider without the use of
>>> CPUIdle - although low-power PM idle is likely to be a primary use-case.
>>
>> What I meant is this timer is not needed for the moment.
>>
>>>> Perhaps you can rework a bit this driver in the meantime to have a better fix
>>>> than disabling the code with macros ?
>>>
>>> I'm happy to do that, but it would be nice to get the driver compiling
>>> first for ARM64 :-)
>>>
>>>> Otherwise, please try at least to group the code into a minimal set of macros.
>>>
>>> So, would it be accurate to say that you would prefer a patch that changes
>>> more lines of code, but minimizes preprocessor directives, to the current
>>> patch?
>>
>> Yes at least an attempt to factor out a bit the driver. Those #ifdef are
>> like #if 0, which is a quick fix. I am not strongly against this patch, but
>> it would be nice to take the opportunity to reorganize it a bit.
>
> How about we do something like the attached patch instead for now. That
> avoids any #ifdef'ery and still we don't attempt (and fail) to build the
> driver on 64-bit ARM.
>
> With that applied we can incrementally make the changes to untangle the
> ARM-specific parts and when the driver can build on 64-bit ARM we simply
> select TEGRA_TIMER via Kconfig.

Yes, that is exactly what I was thinking about after sending the 
previous email. And by this way, you also fixed the Kconfig option 
selection logic.


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH] clocksource: tegra: wrap arch/arm-specific sections in CONFIG_ARM

[Daniel Lezcano]

On 01/09/2015 01:21 PM, Thierry Reding wrote:
> On Fri, Jan 09, 2015 at 09:31:08AM +0100, Daniel Lezcano wrote:
>> On 01/09/2015 03:09 AM, Paul Walmsley wrote:
>>> Hello Daniel
>>>
>>> On Thu, 8 Jan 2015, Daniel Lezcano wrote:
>>>
>>>> On 12/09/2014 11:07 PM, Paul Walmsley wrote:
>>>>>
>>>>> Like several of the other files in drivers/clocksource,
>>>>> tegra20_timer.c contains code that can only compile when CONFIG_ARM is
>>>>> enabled.  This causes obvious problems when trying to compile this
>>>>> code for NVIDIA ARM64-based SoCs, such as Tegra132.  The same timer IP
>>>>> blocks exist, so it seems appropriate to provide support for them.
>>>>>
>>>>> So until we figure out a better way to partition this code, wrap the
>>>>> delay_timer and persistent_clock support code with preprocessor tests
>>>>> for CONFIG_ARM.
>>>>>
>>>>>   (The delay_timer code should not be needed at all on
>>>>> ARM64 due to the presence of the ARMv8 architected timer.  The
>>>>> persistent_clock support code could become important once power
>>>>> management modes are implemented that turn off the CPU complex.)
>>>>
>>>> IIUC, the cpuidle driver is not yet ready, right ?
>>>>
>>>> If it is the case, this driver is not needed yet, no ?
>>>
>>> The point of the patch is to allow the hardware drivers selected by
>>> CONFIG_ARCH_TEGRA to build for an arm64 kernel, just as they build for
>>> 32-bit ARM.
>>>
>>> There's nothing CPUIdle-specific about the patch - that is, this timer can
>>> be selected as a clockevent and clocksource provider without the use of
>>> CPUIdle - although low-power PM idle is likely to be a primary use-case.
>>
>> What I meant is this timer is not needed for the moment.
>>
>>>> Perhaps you can rework a bit this driver in the meantime to have a better fix
>>>> than disabling the code with macros ?
>>>
>>> I'm happy to do that, but it would be nice to get the driver compiling
>>> first for ARM64 :-)
>>>
>>>> Otherwise, please try at least to group the code into a minimal set of macros.
>>>
>>> So, would it be accurate to say that you would prefer a patch that changes
>>> more lines of code, but minimizes preprocessor directives, to the current
>>> patch?
>>
>> Yes at least an attempt to factor out a bit the driver. Those #ifdef are
>> like #if 0, which is a quick fix. I am not strongly against this patch, but
>> it would be nice to take the opportunity to reorganize it a bit.
>
> How about we do something like the attached patch instead for now. That
> avoids any #ifdef'ery and still we don't attempt (and fail) to build the
> driver on 64-bit ARM.
>
> With that applied we can incrementally make the changes to untangle the
> ARM-specific parts and when the driver can build on 64-bit ARM we simply
> select TEGRA_TIMER via Kconfig.

Yes, that is exactly what I was thinking about after sending the 
previous email. And by this way, you also fixed the Kconfig option 
selection logic.


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH] clocksource: tegra: wrap arch/arm-specific sections in CONFIG_ARM

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3275 bytes --]

On Fri, Jan 09, 2015 at 02:24:24PM +0100, Daniel Lezcano wrote:
> On 01/09/2015 01:21 PM, Thierry Reding wrote:
> >On Fri, Jan 09, 2015 at 09:31:08AM +0100, Daniel Lezcano wrote:
> >>On 01/09/2015 03:09 AM, Paul Walmsley wrote:
> >>>Hello Daniel
> >>>
> >>>On Thu, 8 Jan 2015, Daniel Lezcano wrote:
> >>>
> >>>>On 12/09/2014 11:07 PM, Paul Walmsley wrote:
> >>>>>
> >>>>>Like several of the other files in drivers/clocksource,
> >>>>>tegra20_timer.c contains code that can only compile when CONFIG_ARM is
> >>>>>enabled.  This causes obvious problems when trying to compile this
> >>>>>code for NVIDIA ARM64-based SoCs, such as Tegra132.  The same timer IP
> >>>>>blocks exist, so it seems appropriate to provide support for them.
> >>>>>
> >>>>>So until we figure out a better way to partition this code, wrap the
> >>>>>delay_timer and persistent_clock support code with preprocessor tests
> >>>>>for CONFIG_ARM.
> >>>>>
> >>>>>  (The delay_timer code should not be needed at all on
> >>>>>ARM64 due to the presence of the ARMv8 architected timer.  The
> >>>>>persistent_clock support code could become important once power
> >>>>>management modes are implemented that turn off the CPU complex.)
> >>>>
> >>>>IIUC, the cpuidle driver is not yet ready, right ?
> >>>>
> >>>>If it is the case, this driver is not needed yet, no ?
> >>>
> >>>The point of the patch is to allow the hardware drivers selected by
> >>>CONFIG_ARCH_TEGRA to build for an arm64 kernel, just as they build for
> >>>32-bit ARM.
> >>>
> >>>There's nothing CPUIdle-specific about the patch - that is, this timer can
> >>>be selected as a clockevent and clocksource provider without the use of
> >>>CPUIdle - although low-power PM idle is likely to be a primary use-case.
> >>
> >>What I meant is this timer is not needed for the moment.
> >>
> >>>>Perhaps you can rework a bit this driver in the meantime to have a better fix
> >>>>than disabling the code with macros ?
> >>>
> >>>I'm happy to do that, but it would be nice to get the driver compiling
> >>>first for ARM64 :-)
> >>>
> >>>>Otherwise, please try at least to group the code into a minimal set of macros.
> >>>
> >>>So, would it be accurate to say that you would prefer a patch that changes
> >>>more lines of code, but minimizes preprocessor directives, to the current
> >>>patch?
> >>
> >>Yes at least an attempt to factor out a bit the driver. Those #ifdef are
> >>like #if 0, which is a quick fix. I am not strongly against this patch, but
> >>it would be nice to take the opportunity to reorganize it a bit.
> >
> >How about we do something like the attached patch instead for now. That
> >avoids any #ifdef'ery and still we don't attempt (and fail) to build the
> >driver on 64-bit ARM.
> >
> >With that applied we can incrementally make the changes to untangle the
> >ARM-specific parts and when the driver can build on 64-bit ARM we simply
> >select TEGRA_TIMER via Kconfig.
> 
> Yes, that is exactly what I was thinking about after sending the previous
> email. And by this way, you also fixed the Kconfig option selection logic.

Great. Will you give your Acked-by so that I can take that patch through
the Tegra tree to resolve the build dependency?

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] clocksource: tegra: wrap arch/arm-specific sections in CONFIG_ARM

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3275 bytes --]

On Fri, Jan 09, 2015 at 02:24:24PM +0100, Daniel Lezcano wrote:
> On 01/09/2015 01:21 PM, Thierry Reding wrote:
> >On Fri, Jan 09, 2015 at 09:31:08AM +0100, Daniel Lezcano wrote:
> >>On 01/09/2015 03:09 AM, Paul Walmsley wrote:
> >>>Hello Daniel
> >>>
> >>>On Thu, 8 Jan 2015, Daniel Lezcano wrote:
> >>>
> >>>>On 12/09/2014 11:07 PM, Paul Walmsley wrote:
> >>>>>
> >>>>>Like several of the other files in drivers/clocksource,
> >>>>>tegra20_timer.c contains code that can only compile when CONFIG_ARM is
> >>>>>enabled.  This causes obvious problems when trying to compile this
> >>>>>code for NVIDIA ARM64-based SoCs, such as Tegra132.  The same timer IP
> >>>>>blocks exist, so it seems appropriate to provide support for them.
> >>>>>
> >>>>>So until we figure out a better way to partition this code, wrap the
> >>>>>delay_timer and persistent_clock support code with preprocessor tests
> >>>>>for CONFIG_ARM.
> >>>>>
> >>>>>  (The delay_timer code should not be needed at all on
> >>>>>ARM64 due to the presence of the ARMv8 architected timer.  The
> >>>>>persistent_clock support code could become important once power
> >>>>>management modes are implemented that turn off the CPU complex.)
> >>>>
> >>>>IIUC, the cpuidle driver is not yet ready, right ?
> >>>>
> >>>>If it is the case, this driver is not needed yet, no ?
> >>>
> >>>The point of the patch is to allow the hardware drivers selected by
> >>>CONFIG_ARCH_TEGRA to build for an arm64 kernel, just as they build for
> >>>32-bit ARM.
> >>>
> >>>There's nothing CPUIdle-specific about the patch - that is, this timer can
> >>>be selected as a clockevent and clocksource provider without the use of
> >>>CPUIdle - although low-power PM idle is likely to be a primary use-case.
> >>
> >>What I meant is this timer is not needed for the moment.
> >>
> >>>>Perhaps you can rework a bit this driver in the meantime to have a better fix
> >>>>than disabling the code with macros ?
> >>>
> >>>I'm happy to do that, but it would be nice to get the driver compiling
> >>>first for ARM64 :-)
> >>>
> >>>>Otherwise, please try at least to group the code into a minimal set of macros.
> >>>
> >>>So, would it be accurate to say that you would prefer a patch that changes
> >>>more lines of code, but minimizes preprocessor directives, to the current
> >>>patch?
> >>
> >>Yes at least an attempt to factor out a bit the driver. Those #ifdef are
> >>like #if 0, which is a quick fix. I am not strongly against this patch, but
> >>it would be nice to take the opportunity to reorganize it a bit.
> >
> >How about we do something like the attached patch instead for now. That
> >avoids any #ifdef'ery and still we don't attempt (and fail) to build the
> >driver on 64-bit ARM.
> >
> >With that applied we can incrementally make the changes to untangle the
> >ARM-specific parts and when the driver can build on 64-bit ARM we simply
> >select TEGRA_TIMER via Kconfig.
> 
> Yes, that is exactly what I was thinking about after sending the previous
> email. And by this way, you also fixed the Kconfig option selection logic.

Great. Will you give your Acked-by so that I can take that patch through
the Tegra tree to resolve the build dependency?

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] clocksource: tegra: wrap arch/arm-specific sections in CONFIG_ARM

[Daniel Lezcano]

On 01/09/2015 02:33 PM, Thierry Reding wrote:
> On Fri, Jan 09, 2015 at 02:24:24PM +0100, Daniel Lezcano wrote:
>> On 01/09/2015 01:21 PM, Thierry Reding wrote:
>>> On Fri, Jan 09, 2015 at 09:31:08AM +0100, Daniel Lezcano wrote:
>>>> On 01/09/2015 03:09 AM, Paul Walmsley wrote:
>>>>> Hello Daniel
>>>>>
>>>>> On Thu, 8 Jan 2015, Daniel Lezcano wrote:
>>>>>
>>>>>> On 12/09/2014 11:07 PM, Paul Walmsley wrote:
>>>>>>>
>>>>>>> Like several of the other files in drivers/clocksource,
>>>>>>> tegra20_timer.c contains code that can only compile when CONFIG_ARM is
>>>>>>> enabled.  This causes obvious problems when trying to compile this
>>>>>>> code for NVIDIA ARM64-based SoCs, such as Tegra132.  The same timer IP
>>>>>>> blocks exist, so it seems appropriate to provide support for them.
>>>>>>>
>>>>>>> So until we figure out a better way to partition this code, wrap the
>>>>>>> delay_timer and persistent_clock support code with preprocessor tests
>>>>>>> for CONFIG_ARM.
>>>>>>>
>>>>>>>   (The delay_timer code should not be needed at all on
>>>>>>> ARM64 due to the presence of the ARMv8 architected timer.  The
>>>>>>> persistent_clock support code could become important once power
>>>>>>> management modes are implemented that turn off the CPU complex.)
>>>>>>
>>>>>> IIUC, the cpuidle driver is not yet ready, right ?
>>>>>>
>>>>>> If it is the case, this driver is not needed yet, no ?
>>>>>
>>>>> The point of the patch is to allow the hardware drivers selected by
>>>>> CONFIG_ARCH_TEGRA to build for an arm64 kernel, just as they build for
>>>>> 32-bit ARM.
>>>>>
>>>>> There's nothing CPUIdle-specific about the patch - that is, this timer can
>>>>> be selected as a clockevent and clocksource provider without the use of
>>>>> CPUIdle - although low-power PM idle is likely to be a primary use-case.
>>>>
>>>> What I meant is this timer is not needed for the moment.
>>>>
>>>>>> Perhaps you can rework a bit this driver in the meantime to have a better fix
>>>>>> than disabling the code with macros ?
>>>>>
>>>>> I'm happy to do that, but it would be nice to get the driver compiling
>>>>> first for ARM64 :-)
>>>>>
>>>>>> Otherwise, please try at least to group the code into a minimal set of macros.
>>>>>
>>>>> So, would it be accurate to say that you would prefer a patch that changes
>>>>> more lines of code, but minimizes preprocessor directives, to the current
>>>>> patch?
>>>>
>>>> Yes at least an attempt to factor out a bit the driver. Those #ifdef are
>>>> like #if 0, which is a quick fix. I am not strongly against this patch, but
>>>> it would be nice to take the opportunity to reorganize it a bit.
>>>
>>> How about we do something like the attached patch instead for now. That
>>> avoids any #ifdef'ery and still we don't attempt (and fail) to build the
>>> driver on 64-bit ARM.
>>>
>>> With that applied we can incrementally make the changes to untangle the
>>> ARM-specific parts and when the driver can build on 64-bit ARM we simply
>>> select TEGRA_TIMER via Kconfig.
>>
>> Yes, that is exactly what I was thinking about after sending the previous
>> email. And by this way, you also fixed the Kconfig option selection logic.
>
> Great. Will you give your Acked-by so that I can take that patch through
> the Tegra tree to resolve the build dependency?

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH] clocksource: tegra: wrap arch/arm-specific sections in CONFIG_ARM

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1006 bytes --]

On Fri, Jan 09, 2015 at 02:38:39PM +0100, Daniel Lezcano wrote:
> On 01/09/2015 02:33 PM, Thierry Reding wrote:
> >On Fri, Jan 09, 2015 at 02:24:24PM +0100, Daniel Lezcano wrote:
> >>On 01/09/2015 01:21 PM, Thierry Reding wrote:
[...]
> >>>How about we do something like the attached patch instead for now. That
> >>>avoids any #ifdef'ery and still we don't attempt (and fail) to build the
> >>>driver on 64-bit ARM.
> >>>
> >>>With that applied we can incrementally make the changes to untangle the
> >>>ARM-specific parts and when the driver can build on 64-bit ARM we simply
> >>>select TEGRA_TIMER via Kconfig.
> >>
> >>Yes, that is exactly what I was thinking about after sending the previous
> >>email. And by this way, you also fixed the Kconfig option selection logic.
> >
> >Great. Will you give your Acked-by so that I can take that patch through
> >the Tegra tree to resolve the build dependency?
> 
> Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Thanks,
Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] clocksource: tegra: wrap arch/arm-specific sections in CONFIG_ARM

[Daniel Lezcano]

On 12/09/2014 11:07 PM, Paul Walmsley wrote:
>
> Like several of the other files in drivers/clocksource,
> tegra20_timer.c contains code that can only compile when CONFIG_ARM is
> enabled.  This causes obvious problems when trying to compile this
> code for NVIDIA ARM64-based SoCs, such as Tegra132.  The same timer IP
> blocks exist, so it seems appropriate to provide support for them.
>
> So until we figure out a better way to partition this code, wrap the
> delay_timer and persistent_clock support code with preprocessor tests
> for CONFIG_ARM.  (The delay_timer code should not be needed at all on
> ARM64 due to the presence of the ARMv8 architected timer.  The
> persistent_clock support code could become important once power
> management modes are implemented that turn off the CPU complex.)

Well actually putting #ifdef macros in the C code should be avoided if 
possible.

May be you can replace those macros by:

if (IS_ENABLED(CONFIG_ARM64)) {
	...
}

?



> Signed-off-by: Paul Walmsley <paul@pwsan.com>
> Signed-off-by: Paul Walmsley <pwalmsley@nvidia.com>
> Cc: Allen Martin <amartin@nvidia.com>
> Cc: Stephen Warren <swarren@nvidia.com>
> Cc: Thierry Reding <treding@nvidia.com>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Alexandre Courbot <gnurou@gmail.com>
> ---
> Applies against next-20141209.
> Intended for v3.20.
> Boot-tested on Tegra124 Jetson TK1 on next-20141209.
> Also boot-tested on Tegra132 Norrin FFD on next-20141209 + extra,
> unrelated patches.
>
>   drivers/clocksource/tegra20_timer.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
>
> diff --git a/drivers/clocksource/tegra20_timer.c b/drivers/clocksource/tegra20_timer.c
> index d2616ef16770..83a8f5c9e139 100644
> --- a/drivers/clocksource/tegra20_timer.c
> +++ b/drivers/clocksource/tegra20_timer.c
> @@ -29,8 +29,10 @@
>   #include <linux/sched_clock.h>
>   #include <linux/delay.h>
>
> +#ifdef CONFIG_ARM
>   #include <asm/mach/time.h>
>   #include <asm/smp_twd.h>
> +#endif
>
>   #define RTC_SECONDS            0x08
>   #define RTC_SHADOW_SECONDS     0x0c
> @@ -49,12 +51,14 @@
>   #define TIMER_PCR 0x4
>
>   static void __iomem *timer_reg_base;
> +#ifdef CONFIG_ARM
>   static void __iomem *rtc_base;
>
>   static struct timespec persistent_ts;
>   static u64 persistent_ms, last_persistent_ms;
>
>   static struct delay_timer tegra_delay_timer;
> +#endif
>
>   #define timer_writel(value, reg) \
>   	__raw_writel(value, timer_reg_base + (reg))
> @@ -106,6 +110,7 @@ static u64 notrace tegra_read_sched_clock(void)
>   	return timer_readl(TIMERUS_CNTR_1US);
>   }
>
> +#ifdef CONFIG_ARM
>   /*
>    * tegra_rtc_read - Reads the Tegra RTC registers
>    * Care must be taken that this funciton is not called while the
> @@ -146,6 +151,8 @@ static unsigned long tegra_delay_timer_read_counter_long(void)
>   {
>   	return readl(timer_reg_base + TIMERUS_CNTR_1US);
>   }
> +#endif /* CONFIG_ARM */
> +
>
>   static irqreturn_t tegra_timer_interrupt(int irq, void *dev_id)
>   {
> @@ -214,10 +221,12 @@ static void __init tegra20_init_timer(struct device_node *np)
>   		BUG();
>   	}
>
> +#ifdef CONFIG_ARM
>   	tegra_delay_timer.read_current_timer =
>   			tegra_delay_timer_read_counter_long;
>   	tegra_delay_timer.freq = 1000000;
>   	register_current_timer_delay(&tegra_delay_timer);
> +#endif
>
>   	ret = setup_irq(tegra_timer_irq.irq, &tegra_timer_irq);
>   	if (ret) {
> @@ -232,6 +241,7 @@ static void __init tegra20_init_timer(struct device_node *np)
>   }
>   CLOCKSOURCE_OF_DECLARE(tegra20_timer, "nvidia,tegra20-timer", tegra20_init_timer);
>
> +#ifdef CONFIG_ARM
>   static void __init tegra20_init_rtc(struct device_node *np)
>   {
>   	struct clk *clk;
> @@ -255,6 +265,7 @@ static void __init tegra20_init_rtc(struct device_node *np)
>   	register_persistent_clock(NULL, tegra_read_persistent_clock);
>   }
>   CLOCKSOURCE_OF_DECLARE(tegra20_rtc, "nvidia,tegra20-rtc", tegra20_init_rtc);
> +#endif /* CONFIG_ARM */
>
>   #ifdef CONFIG_PM
>   static u32 usec_config;
>


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH] clocksource: tegra: wrap arch/arm-specific sections in CONFIG_ARM

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1243 bytes --]

On Thu, Jan 08, 2015 at 03:21:58PM +0100, Daniel Lezcano wrote:
> On 12/09/2014 11:07 PM, Paul Walmsley wrote:
> >
> >Like several of the other files in drivers/clocksource,
> >tegra20_timer.c contains code that can only compile when CONFIG_ARM is
> >enabled.  This causes obvious problems when trying to compile this
> >code for NVIDIA ARM64-based SoCs, such as Tegra132.  The same timer IP
> >blocks exist, so it seems appropriate to provide support for them.
> >
> >So until we figure out a better way to partition this code, wrap the
> >delay_timer and persistent_clock support code with preprocessor tests
> >for CONFIG_ARM.  (The delay_timer code should not be needed at all on
> >ARM64 due to the presence of the ARMv8 architected timer.  The
> >persistent_clock support code could become important once power
> >management modes are implemented that turn off the CPU complex.)
> 
> Well actually putting #ifdef macros in the C code should be avoided if
> possible.
> 
> May be you can replace those macros by:
> 
> if (IS_ENABLED(CONFIG_ARM64)) {
> 	...
> }
> 
> ?

Unfortunately we can't. Some of the symbols in this file are only
defined in headers specific to ARM and not available on ARM64.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] clocksource: tegra: wrap arch/arm-specific sections in CONFIG_ARM

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1243 bytes --]

On Thu, Jan 08, 2015 at 03:21:58PM +0100, Daniel Lezcano wrote:
> On 12/09/2014 11:07 PM, Paul Walmsley wrote:
> >
> >Like several of the other files in drivers/clocksource,
> >tegra20_timer.c contains code that can only compile when CONFIG_ARM is
> >enabled.  This causes obvious problems when trying to compile this
> >code for NVIDIA ARM64-based SoCs, such as Tegra132.  The same timer IP
> >blocks exist, so it seems appropriate to provide support for them.
> >
> >So until we figure out a better way to partition this code, wrap the
> >delay_timer and persistent_clock support code with preprocessor tests
> >for CONFIG_ARM.  (The delay_timer code should not be needed at all on
> >ARM64 due to the presence of the ARMv8 architected timer.  The
> >persistent_clock support code could become important once power
> >management modes are implemented that turn off the CPU complex.)
> 
> Well actually putting #ifdef macros in the C code should be avoided if
> possible.
> 
> May be you can replace those macros by:
> 
> if (IS_ENABLED(CONFIG_ARM64)) {
> 	...
> }
> 
> ?

Unfortunately we can't. Some of the symbols in this file are only
defined in headers specific to ARM and not available on ARM64.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]