Patchwork [5/9] clocksource: tegra: Enable ARM arch_timer with TSC

login
register
mail settings
Submitter Hiroshi Doyu
Date Dec. 20, 2012, 9:44 a.m.
Message ID <1355996654-6579-6-git-send-email-hdoyu@nvidia.com>
Download mbox | patch
Permalink /patch/207633/
State Changes Requested, archived
Headers show

Comments

Hiroshi Doyu - Dec. 20, 2012, 9:44 a.m.
Add platform enabler for ARM arch_timer(TSC). TSC is more fine grained
timer than TMR0. If it's available, it will be used for clock source
and sched_clock. Otherwise, TMR0 is used. In any case TMR0 is
necessary for clock event.

Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
---
 .../bindings/arm/tegra/nvidia,tegra114-tsc.txt     |   11 ++++
 drivers/clocksource/tegra20_timer.c                |   64 +++++++++++++++++++-
 2 files changed, 74 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/arm/tegra/nvidia,tegra114-tsc.txt
Marc Zyngier - Dec. 20, 2012, 11:01 a.m.
On 20/12/12 09:44, Hiroshi Doyu wrote:
> Add platform enabler for ARM arch_timer(TSC). TSC is more fine grained
> timer than TMR0. If it's available, it will be used for clock source
> and sched_clock. Otherwise, TMR0 is used. In any case TMR0 is
> necessary for clock event.
> 
> Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> ---
>  .../bindings/arm/tegra/nvidia,tegra114-tsc.txt     |   11 ++++
>  drivers/clocksource/tegra20_timer.c                |   64 +++++++++++++++++++-
>  2 files changed, 74 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/devicetree/bindings/arm/tegra/nvidia,tegra114-tsc.txt
> 
> diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra114-tsc.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra114-tsc.txt
> new file mode 100644
> index 0000000..9de936a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra114-tsc.txt
> @@ -0,0 +1,11 @@
> +NVIDIA Tegra Timer Stamp Counter(TSC)
> +
> +Required properties:
> +- compatible : "nvidia,tegra114-tsc
> +- reg : Should contain 1 register ranges(address and length)
> +
> +Example:
> +	tsc {
> +		compatible = "nvidia,tegra114-tsc";
> +		reg = <0x700f0000 0x20000>;
> +	};
> diff --git a/drivers/clocksource/tegra20_timer.c b/drivers/clocksource/tegra20_timer.c
> index 1d25de8..285a6f1 100644
> --- a/drivers/clocksource/tegra20_timer.c
> +++ b/drivers/clocksource/tegra20_timer.c
> @@ -30,6 +30,7 @@
>  #include <asm/mach/time.h>
>  #include <asm/smp_twd.h>
>  #include <asm/sched_clock.h>
> +#include <asm/arch_timer.h>
>  
>  #define RTC_SECONDS            0x08
>  #define RTC_SHADOW_SECONDS     0x0c
> @@ -271,10 +272,71 @@ static void __init tegra20_init_tmr(void)
>  	clockevents_register_device(&tegra_clockevent);
>  }
>  
> +#define TSC_CNTCR		0		/* TSC control registers */
> +#define TSC_CNTCR_ENABLE	(1 << 0)	/* Enable */
> +#define TSC_CNTCR_HDBG		(1 << 1)	/* Halt on debug */
> +
> +#define TSC_CNTCV0		0x8		/* TSC counter (LSW) */
> +#define TSC_CNTCV1		0xc		/* TSC counter (MSW) */
> +#define TSC_CNTFID0		0x20		/* TSC freq id 0 */
> +
> +static const struct of_device_id tegra_tsc_match[] __initconst = {
> +	{ .compatible = "nvidia,tegra114-tsc" },
> +	{}
> +};
> +
> +static int tegra_arch_timer_init(void)
> +{
> +	int err;
> +	struct device_node *np;
> +	struct clk *clk;
> +	void __iomem *tsc_base;
> +	u32 freq, val;
> +
> +	np = of_find_matching_node(NULL, tegra_tsc_match);
> +	if (!np)
> +		return -ENODEV;
> +
> +	tsc_base = of_iomap(np, 0);
> +	if (!tsc_base)
> +		return -ENODEV;
> +
> +	clk = clk_get_sys("clk_m", NULL);
> +	if (IS_ERR(clk)) {
> +		freq = 12000000;
> +		pr_warn("Unable to get timer clock. Assuming 12Mhz input clock.\n");
> +	} else {
> +		freq = clk_get_rate(clk);
> +		clk_put(clk);
> +	}
> +	writel_relaxed(freq, tsc_base + TSC_CNTFID0);
> +
> +	/* CNTFRQ */
> +	asm("mcr p15, 0, %0, c14, c0, 0\n" : : "r" (freq));
> +	asm("mrc p15, 0, %0, c14, c0, 0\n" : "=r" (val));
> +	BUG_ON(val != freq);

This is scary. CNTFRQ is only writable from secure mode, and will
explode in any other situation.

Also, writing to CNTFRQ doesn't change the timer frequency! This is just
a way for secure mode to tell the rest of the world the frequency the
timer is ticking at. Unless you've wired the input clock to be able to
change the frequency?

	M.
Hiroshi Doyu - Dec. 20, 2012, 11:57 a.m.
Marc Zyngier <marc.zyngier@arm.com> wrote @ Thu, 20 Dec 2012 12:01:15 +0100:

> On 20/12/12 09:44, Hiroshi Doyu wrote:
> > Add platform enabler for ARM arch_timer(TSC). TSC is more fine grained
> > timer than TMR0. If it's available, it will be used for clock source
> > and sched_clock. Otherwise, TMR0 is used. In any case TMR0 is
> > necessary for clock event.
> > 
> > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> > ---
> >  .../bindings/arm/tegra/nvidia,tegra114-tsc.txt     |   11 ++++
> >  drivers/clocksource/tegra20_timer.c                |   64 +++++++++++++++++++-
> >  2 files changed, 74 insertions(+), 1 deletion(-)
> >  create mode 100644 Documentation/devicetree/bindings/arm/tegra/nvidia,tegra114-tsc.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra114-tsc.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra114-tsc.txt
> > new file mode 100644
> > index 0000000..9de936a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra114-tsc.txt
> > @@ -0,0 +1,11 @@
> > +NVIDIA Tegra Timer Stamp Counter(TSC)
> > +
> > +Required properties:
> > +- compatible : "nvidia,tegra114-tsc
> > +- reg : Should contain 1 register ranges(address and length)
> > +
> > +Example:
> > +	tsc {
> > +		compatible = "nvidia,tegra114-tsc";
> > +		reg = <0x700f0000 0x20000>;
> > +	};
> > diff --git a/drivers/clocksource/tegra20_timer.c b/drivers/clocksource/tegra20_timer.c
> > index 1d25de8..285a6f1 100644
> > --- a/drivers/clocksource/tegra20_timer.c
> > +++ b/drivers/clocksource/tegra20_timer.c
> > @@ -30,6 +30,7 @@
> >  #include <asm/mach/time.h>
> >  #include <asm/smp_twd.h>
> >  #include <asm/sched_clock.h>
> > +#include <asm/arch_timer.h>
> >  
> >  #define RTC_SECONDS            0x08
> >  #define RTC_SHADOW_SECONDS     0x0c
> > @@ -271,10 +272,71 @@ static void __init tegra20_init_tmr(void)
> >  	clockevents_register_device(&tegra_clockevent);
> >  }
> >  
> > +#define TSC_CNTCR		0		/* TSC control registers */
> > +#define TSC_CNTCR_ENABLE	(1 << 0)	/* Enable */
> > +#define TSC_CNTCR_HDBG		(1 << 1)	/* Halt on debug */
> > +
> > +#define TSC_CNTCV0		0x8		/* TSC counter (LSW) */
> > +#define TSC_CNTCV1		0xc		/* TSC counter (MSW) */
> > +#define TSC_CNTFID0		0x20		/* TSC freq id 0 */
> > +
> > +static const struct of_device_id tegra_tsc_match[] __initconst = {
> > +	{ .compatible = "nvidia,tegra114-tsc" },
> > +	{}
> > +};
> > +
> > +static int tegra_arch_timer_init(void)
> > +{
> > +	int err;
> > +	struct device_node *np;
> > +	struct clk *clk;
> > +	void __iomem *tsc_base;
> > +	u32 freq, val;
> > +
> > +	np = of_find_matching_node(NULL, tegra_tsc_match);
> > +	if (!np)
> > +		return -ENODEV;
> > +
> > +	tsc_base = of_iomap(np, 0);
> > +	if (!tsc_base)
> > +		return -ENODEV;
> > +
> > +	clk = clk_get_sys("clk_m", NULL);
> > +	if (IS_ERR(clk)) {
> > +		freq = 12000000;
> > +		pr_warn("Unable to get timer clock. Assuming 12Mhz input clock.\n");
> > +	} else {
> > +		freq = clk_get_rate(clk);
> > +		clk_put(clk);
> > +	}
> > +	writel_relaxed(freq, tsc_base + TSC_CNTFID0);
> > +
> > +	/* CNTFRQ */
> > +	asm("mcr p15, 0, %0, c14, c0, 0\n" : : "r" (freq));
> > +	asm("mrc p15, 0, %0, c14, c0, 0\n" : "=r" (val));
> > +	BUG_ON(val != freq);
> 
> This is scary. CNTFRQ is only writable from secure mode, and will
> explode in any other situation.
> 
> Also, writing to CNTFRQ doesn't change the timer frequency! This is just
> a way for secure mode to tell the rest of the world the frequency the
> timer is ticking at. Unless you've wired the input clock to be able to
> change the frequency?

ATM, our upstream kernel is expected in secure mode. This situation
may be changed later, though....
--
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
Marc Zyngier - Dec. 20, 2012, 12:05 p.m.
On 20/12/12 11:57, Hiroshi Doyu wrote:
> Marc Zyngier <marc.zyngier@arm.com> wrote @ Thu, 20 Dec 2012 12:01:15 +0100:
> 
>> On 20/12/12 09:44, Hiroshi Doyu wrote:
>>> Add platform enabler for ARM arch_timer(TSC). TSC is more fine grained
>>> timer than TMR0. If it's available, it will be used for clock source
>>> and sched_clock. Otherwise, TMR0 is used. In any case TMR0 is
>>> necessary for clock event.
>>>
>>> Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
>>> ---
>>>  .../bindings/arm/tegra/nvidia,tegra114-tsc.txt     |   11 ++++
>>>  drivers/clocksource/tegra20_timer.c                |   64 +++++++++++++++++++-
>>>  2 files changed, 74 insertions(+), 1 deletion(-)
>>>  create mode 100644 Documentation/devicetree/bindings/arm/tegra/nvidia,tegra114-tsc.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra114-tsc.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra114-tsc.txt
>>> new file mode 100644
>>> index 0000000..9de936a
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra114-tsc.txt
>>> @@ -0,0 +1,11 @@
>>> +NVIDIA Tegra Timer Stamp Counter(TSC)
>>> +
>>> +Required properties:
>>> +- compatible : "nvidia,tegra114-tsc
>>> +- reg : Should contain 1 register ranges(address and length)
>>> +
>>> +Example:
>>> +	tsc {
>>> +		compatible = "nvidia,tegra114-tsc";
>>> +		reg = <0x700f0000 0x20000>;
>>> +	};
>>> diff --git a/drivers/clocksource/tegra20_timer.c b/drivers/clocksource/tegra20_timer.c
>>> index 1d25de8..285a6f1 100644
>>> --- a/drivers/clocksource/tegra20_timer.c
>>> +++ b/drivers/clocksource/tegra20_timer.c
>>> @@ -30,6 +30,7 @@
>>>  #include <asm/mach/time.h>
>>>  #include <asm/smp_twd.h>
>>>  #include <asm/sched_clock.h>
>>> +#include <asm/arch_timer.h>
>>>  
>>>  #define RTC_SECONDS            0x08
>>>  #define RTC_SHADOW_SECONDS     0x0c
>>> @@ -271,10 +272,71 @@ static void __init tegra20_init_tmr(void)
>>>  	clockevents_register_device(&tegra_clockevent);
>>>  }
>>>  
>>> +#define TSC_CNTCR		0		/* TSC control registers */
>>> +#define TSC_CNTCR_ENABLE	(1 << 0)	/* Enable */
>>> +#define TSC_CNTCR_HDBG		(1 << 1)	/* Halt on debug */
>>> +
>>> +#define TSC_CNTCV0		0x8		/* TSC counter (LSW) */
>>> +#define TSC_CNTCV1		0xc		/* TSC counter (MSW) */
>>> +#define TSC_CNTFID0		0x20		/* TSC freq id 0 */
>>> +
>>> +static const struct of_device_id tegra_tsc_match[] __initconst = {
>>> +	{ .compatible = "nvidia,tegra114-tsc" },
>>> +	{}
>>> +};
>>> +
>>> +static int tegra_arch_timer_init(void)
>>> +{
>>> +	int err;
>>> +	struct device_node *np;
>>> +	struct clk *clk;
>>> +	void __iomem *tsc_base;
>>> +	u32 freq, val;
>>> +
>>> +	np = of_find_matching_node(NULL, tegra_tsc_match);
>>> +	if (!np)
>>> +		return -ENODEV;
>>> +
>>> +	tsc_base = of_iomap(np, 0);
>>> +	if (!tsc_base)
>>> +		return -ENODEV;
>>> +
>>> +	clk = clk_get_sys("clk_m", NULL);
>>> +	if (IS_ERR(clk)) {
>>> +		freq = 12000000;
>>> +		pr_warn("Unable to get timer clock. Assuming 12Mhz input clock.\n");
>>> +	} else {
>>> +		freq = clk_get_rate(clk);
>>> +		clk_put(clk);
>>> +	}
>>> +	writel_relaxed(freq, tsc_base + TSC_CNTFID0);
>>> +
>>> +	/* CNTFRQ */
>>> +	asm("mcr p15, 0, %0, c14, c0, 0\n" : : "r" (freq));
>>> +	asm("mrc p15, 0, %0, c14, c0, 0\n" : "=r" (val));
>>> +	BUG_ON(val != freq);
>>
>> This is scary. CNTFRQ is only writable from secure mode, and will
>> explode in any other situation.
>>
>> Also, writing to CNTFRQ doesn't change the timer frequency! This is just
>> a way for secure mode to tell the rest of the world the frequency the
>> timer is ticking at. Unless you've wired the input clock to be able to
>> change the frequency?
> 
> ATM, our upstream kernel is expected in secure mode. This situation
> may be changed later, though....

I appreciate this. But I expect this kernel to be also used on the
non-secure side if someone tried to run KVM with it. And this would go
bang right away.

	M.
Peter De Schrijver - Dec. 20, 2012, 12:22 p.m.
> >>> +
> >>> +	/* CNTFRQ */
> >>> +	asm("mcr p15, 0, %0, c14, c0, 0\n" : : "r" (freq));
> >>> +	asm("mrc p15, 0, %0, c14, c0, 0\n" : "=r" (val));
> >>> +	BUG_ON(val != freq);
> >>
> >> This is scary. CNTFRQ is only writable from secure mode, and will
> >> explode in any other situation.
> >>
> >> Also, writing to CNTFRQ doesn't change the timer frequency! This is just
> >> a way for secure mode to tell the rest of the world the frequency the
> >> timer is ticking at. Unless you've wired the input clock to be able to
> >> change the frequency?
> > 
> > ATM, our upstream kernel is expected in secure mode. This situation
> > may be changed later, though....
> 
> I appreciate this. But I expect this kernel to be also used on the
> non-secure side if someone tried to run KVM with it. And this would go
> bang right away.
> 

But the guest wouldn't necessarily have this peripheral, or any other Tegra114
peripheral for that matter?

Cheers,

Peter.
--
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
Marc Zyngier - Dec. 20, 2012, 12:33 p.m.
On 20/12/12 12:22, Peter De Schrijver wrote:
>>>>> +
>>>>> +	/* CNTFRQ */
>>>>> +	asm("mcr p15, 0, %0, c14, c0, 0\n" : : "r" (freq));
>>>>> +	asm("mrc p15, 0, %0, c14, c0, 0\n" : "=r" (val));
>>>>> +	BUG_ON(val != freq);
>>>>
>>>> This is scary. CNTFRQ is only writable from secure mode, and will
>>>> explode in any other situation.
>>>>
>>>> Also, writing to CNTFRQ doesn't change the timer frequency! This is just
>>>> a way for secure mode to tell the rest of the world the frequency the
>>>> timer is ticking at. Unless you've wired the input clock to be able to
>>>> change the frequency?
>>>
>>> ATM, our upstream kernel is expected in secure mode. This situation
>>> may be changed later, though....
>>
>> I appreciate this. But I expect this kernel to be also used on the
>> non-secure side if someone tried to run KVM with it. And this would go
>> bang right away.
>>
> 
> But the guest wouldn't necessarily have this peripheral, or any other Tegra114
> peripheral for that matter?

The problem is not so much the guest but the host. The host has to be
booted in non-secure, so just saying "we do not support non-secure" is
not a very convincing argument.

Unless of course you've already decided that you don't want to support
KVM on this SoC...

	M.
Peter De Schrijver - Dec. 20, 2012, 12:55 p.m.
On Thu, Dec 20, 2012 at 01:33:42PM +0100, Marc Zyngier wrote:
> On 20/12/12 12:22, Peter De Schrijver wrote:
> >>>>> +
> >>>>> +	/* CNTFRQ */
> >>>>> +	asm("mcr p15, 0, %0, c14, c0, 0\n" : : "r" (freq));
> >>>>> +	asm("mrc p15, 0, %0, c14, c0, 0\n" : "=r" (val));
> >>>>> +	BUG_ON(val != freq);
> >>>>
> >>>> This is scary. CNTFRQ is only writable from secure mode, and will
> >>>> explode in any other situation.
> >>>>
> >>>> Also, writing to CNTFRQ doesn't change the timer frequency! This is just
> >>>> a way for secure mode to tell the rest of the world the frequency the
> >>>> timer is ticking at. Unless you've wired the input clock to be able to
> >>>> change the frequency?
> >>>
> >>> ATM, our upstream kernel is expected in secure mode. This situation
> >>> may be changed later, though....
> >>
> >> I appreciate this. But I expect this kernel to be also used on the
> >> non-secure side if someone tried to run KVM with it. And this would go
> >> bang right away.
> >>
> > 
> > But the guest wouldn't necessarily have this peripheral, or any other Tegra114
> > peripheral for that matter?
> 
> The problem is not so much the guest but the host. The host has to be
> booted in non-secure, so just saying "we do not support non-secure" is
> not a very convincing argument.
> 
> Unless of course you've already decided that you don't want to support
> KVM on this SoC...
> 

I guess that means we can't support KVM yet. Tegra does not have a secure
monitor by default. It all depends on what that system integrator does.

Cheers,

Peter.
--
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
Hiroshi Doyu - Dec. 20, 2012, 1:25 p.m.
Marc Zyngier <marc.zyngier@arm.com> wrote @ Thu, 20 Dec 2012 13:05:45 +0100:

> On 20/12/12 11:57, Hiroshi Doyu wrote:
> > Marc Zyngier <marc.zyngier@arm.com> wrote @ Thu, 20 Dec 2012 12:01:15 +0100:
> > 
> >> On 20/12/12 09:44, Hiroshi Doyu wrote:
> >>> Add platform enabler for ARM arch_timer(TSC). TSC is more fine grained
> >>> timer than TMR0. If it's available, it will be used for clock source
> >>> and sched_clock. Otherwise, TMR0 is used. In any case TMR0 is
> >>> necessary for clock event.
> >>>
> >>> Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
> >>> ---
> >>>  .../bindings/arm/tegra/nvidia,tegra114-tsc.txt     |   11 ++++
> >>>  drivers/clocksource/tegra20_timer.c                |   64 +++++++++++++++++++-
> >>>  2 files changed, 74 insertions(+), 1 deletion(-)
> >>>  create mode 100644 Documentation/devicetree/bindings/arm/tegra/nvidia,tegra114-tsc.txt
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra114-tsc.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra114-tsc.txt
> >>> new file mode 100644
> >>> index 0000000..9de936a
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra114-tsc.txt
> >>> @@ -0,0 +1,11 @@
> >>> +NVIDIA Tegra Timer Stamp Counter(TSC)
> >>> +
> >>> +Required properties:
> >>> +- compatible : "nvidia,tegra114-tsc
> >>> +- reg : Should contain 1 register ranges(address and length)
> >>> +
> >>> +Example:
> >>> +	tsc {
> >>> +		compatible = "nvidia,tegra114-tsc";
> >>> +		reg = <0x700f0000 0x20000>;
> >>> +	};
> >>> diff --git a/drivers/clocksource/tegra20_timer.c b/drivers/clocksource/tegra20_timer.c
> >>> index 1d25de8..285a6f1 100644
> >>> --- a/drivers/clocksource/tegra20_timer.c
> >>> +++ b/drivers/clocksource/tegra20_timer.c
> >>> @@ -30,6 +30,7 @@
> >>>  #include <asm/mach/time.h>
> >>>  #include <asm/smp_twd.h>
> >>>  #include <asm/sched_clock.h>
> >>> +#include <asm/arch_timer.h>
> >>>  
> >>>  #define RTC_SECONDS            0x08
> >>>  #define RTC_SHADOW_SECONDS     0x0c
> >>> @@ -271,10 +272,71 @@ static void __init tegra20_init_tmr(void)
> >>>  	clockevents_register_device(&tegra_clockevent);
> >>>  }
> >>>  
> >>> +#define TSC_CNTCR		0		/* TSC control registers */
> >>> +#define TSC_CNTCR_ENABLE	(1 << 0)	/* Enable */
> >>> +#define TSC_CNTCR_HDBG		(1 << 1)	/* Halt on debug */
> >>> +
> >>> +#define TSC_CNTCV0		0x8		/* TSC counter (LSW) */
> >>> +#define TSC_CNTCV1		0xc		/* TSC counter (MSW) */
> >>> +#define TSC_CNTFID0		0x20		/* TSC freq id 0 */
> >>> +
> >>> +static const struct of_device_id tegra_tsc_match[] __initconst = {
> >>> +	{ .compatible = "nvidia,tegra114-tsc" },
> >>> +	{}
> >>> +};
> >>> +
> >>> +static int tegra_arch_timer_init(void)
> >>> +{
> >>> +	int err;
> >>> +	struct device_node *np;
> >>> +	struct clk *clk;
> >>> +	void __iomem *tsc_base;
> >>> +	u32 freq, val;
> >>> +
> >>> +	np = of_find_matching_node(NULL, tegra_tsc_match);
> >>> +	if (!np)
> >>> +		return -ENODEV;
> >>> +
> >>> +	tsc_base = of_iomap(np, 0);
> >>> +	if (!tsc_base)
> >>> +		return -ENODEV;
> >>> +
> >>> +	clk = clk_get_sys("clk_m", NULL);
> >>> +	if (IS_ERR(clk)) {
> >>> +		freq = 12000000;
> >>> +		pr_warn("Unable to get timer clock. Assuming 12Mhz input clock.\n");
> >>> +	} else {
> >>> +		freq = clk_get_rate(clk);
> >>> +		clk_put(clk);
> >>> +	}
> >>> +	writel_relaxed(freq, tsc_base + TSC_CNTFID0);
> >>> +
> >>> +	/* CNTFRQ */
> >>> +	asm("mcr p15, 0, %0, c14, c0, 0\n" : : "r" (freq));
> >>> +	asm("mrc p15, 0, %0, c14, c0, 0\n" : "=r" (val));
> >>> +	BUG_ON(val != freq);
> >>
> >> This is scary. CNTFRQ is only writable from secure mode, and will
> >> explode in any other situation.
> >>
> >> Also, writing to CNTFRQ doesn't change the timer frequency! This is just
> >> a way for secure mode to tell the rest of the world the frequency the
> >> timer is ticking at. Unless you've wired the input clock to be able to
> >> change the frequency?
> > 
> > ATM, our upstream kernel is expected in secure mode. This situation
> > may be changed later, though....
> 
> I appreciate this. But I expect this kernel to be also used on the
> non-secure side if someone tried to run KVM with it. And this would go
> bang right away.

What is the common way to make the above code selective at run
time(secure or non-secure path)?
--
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
Marc Zyngier - Dec. 20, 2012, 1:32 p.m.
On 20/12/12 12:55, Peter De Schrijver wrote:
> On Thu, Dec 20, 2012 at 01:33:42PM +0100, Marc Zyngier wrote:
>> On 20/12/12 12:22, Peter De Schrijver wrote:
>>>>>>> +
>>>>>>> +	/* CNTFRQ */
>>>>>>> +	asm("mcr p15, 0, %0, c14, c0, 0\n" : : "r" (freq));
>>>>>>> +	asm("mrc p15, 0, %0, c14, c0, 0\n" : "=r" (val));
>>>>>>> +	BUG_ON(val != freq);
>>>>>>
>>>>>> This is scary. CNTFRQ is only writable from secure mode, and will
>>>>>> explode in any other situation.
>>>>>>
>>>>>> Also, writing to CNTFRQ doesn't change the timer frequency! This is just
>>>>>> a way for secure mode to tell the rest of the world the frequency the
>>>>>> timer is ticking at. Unless you've wired the input clock to be able to
>>>>>> change the frequency?
>>>>>
>>>>> ATM, our upstream kernel is expected in secure mode. This situation
>>>>> may be changed later, though....
>>>>
>>>> I appreciate this. But I expect this kernel to be also used on the
>>>> non-secure side if someone tried to run KVM with it. And this would go
>>>> bang right away.
>>>>
>>>
>>> But the guest wouldn't necessarily have this peripheral, or any other Tegra114
>>> peripheral for that matter?
>>
>> The problem is not so much the guest but the host. The host has to be
>> booted in non-secure, so just saying "we do not support non-secure" is
>> not a very convincing argument.
>>
>> Unless of course you've already decided that you don't want to support
>> KVM on this SoC...
>>
> 
> I guess that means we can't support KVM yet. Tegra does not have a secure
> monitor by default. It all depends on what that system integrator does.

VExpress doesn't have a secure monitor either, and yet we run KVM on it
(by switching to non-secure before loading the kernel). Same for Exynos5.

What I'm trying to say is that this code is rather pointless (this
should be done by the firmware/bootloader, not the kernel, or the
information should be provided in DT if CNTFRQ is not set).

This way, no breakage, no dependency on the security level.

	M.
Marc Zyngier - Dec. 20, 2012, 1:33 p.m.
On 20/12/12 13:25, Hiroshi Doyu wrote:
> Marc Zyngier <marc.zyngier@arm.com> wrote @ Thu, 20 Dec 2012 13:05:45 +0100:
> 
>> On 20/12/12 11:57, Hiroshi Doyu wrote:
>>> Marc Zyngier <marc.zyngier@arm.com> wrote @ Thu, 20 Dec 2012 12:01:15 +0100:
>>>
>>>> On 20/12/12 09:44, Hiroshi Doyu wrote:
>>>>> Add platform enabler for ARM arch_timer(TSC). TSC is more fine grained
>>>>> timer than TMR0. If it's available, it will be used for clock source
>>>>> and sched_clock. Otherwise, TMR0 is used. In any case TMR0 is
>>>>> necessary for clock event.
>>>>>
>>>>> Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com>
>>>>> ---
>>>>>  .../bindings/arm/tegra/nvidia,tegra114-tsc.txt     |   11 ++++
>>>>>  drivers/clocksource/tegra20_timer.c                |   64 +++++++++++++++++++-
>>>>>  2 files changed, 74 insertions(+), 1 deletion(-)
>>>>>  create mode 100644 Documentation/devicetree/bindings/arm/tegra/nvidia,tegra114-tsc.txt
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra114-tsc.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra114-tsc.txt
>>>>> new file mode 100644
>>>>> index 0000000..9de936a
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra114-tsc.txt
>>>>> @@ -0,0 +1,11 @@
>>>>> +NVIDIA Tegra Timer Stamp Counter(TSC)
>>>>> +
>>>>> +Required properties:
>>>>> +- compatible : "nvidia,tegra114-tsc
>>>>> +- reg : Should contain 1 register ranges(address and length)
>>>>> +
>>>>> +Example:
>>>>> +	tsc {
>>>>> +		compatible = "nvidia,tegra114-tsc";
>>>>> +		reg = <0x700f0000 0x20000>;
>>>>> +	};
>>>>> diff --git a/drivers/clocksource/tegra20_timer.c b/drivers/clocksource/tegra20_timer.c
>>>>> index 1d25de8..285a6f1 100644
>>>>> --- a/drivers/clocksource/tegra20_timer.c
>>>>> +++ b/drivers/clocksource/tegra20_timer.c
>>>>> @@ -30,6 +30,7 @@
>>>>>  #include <asm/mach/time.h>
>>>>>  #include <asm/smp_twd.h>
>>>>>  #include <asm/sched_clock.h>
>>>>> +#include <asm/arch_timer.h>
>>>>>  
>>>>>  #define RTC_SECONDS            0x08
>>>>>  #define RTC_SHADOW_SECONDS     0x0c
>>>>> @@ -271,10 +272,71 @@ static void __init tegra20_init_tmr(void)
>>>>>  	clockevents_register_device(&tegra_clockevent);
>>>>>  }
>>>>>  
>>>>> +#define TSC_CNTCR		0		/* TSC control registers */
>>>>> +#define TSC_CNTCR_ENABLE	(1 << 0)	/* Enable */
>>>>> +#define TSC_CNTCR_HDBG		(1 << 1)	/* Halt on debug */
>>>>> +
>>>>> +#define TSC_CNTCV0		0x8		/* TSC counter (LSW) */
>>>>> +#define TSC_CNTCV1		0xc		/* TSC counter (MSW) */
>>>>> +#define TSC_CNTFID0		0x20		/* TSC freq id 0 */
>>>>> +
>>>>> +static const struct of_device_id tegra_tsc_match[] __initconst = {
>>>>> +	{ .compatible = "nvidia,tegra114-tsc" },
>>>>> +	{}
>>>>> +};
>>>>> +
>>>>> +static int tegra_arch_timer_init(void)
>>>>> +{
>>>>> +	int err;
>>>>> +	struct device_node *np;
>>>>> +	struct clk *clk;
>>>>> +	void __iomem *tsc_base;
>>>>> +	u32 freq, val;
>>>>> +
>>>>> +	np = of_find_matching_node(NULL, tegra_tsc_match);
>>>>> +	if (!np)
>>>>> +		return -ENODEV;
>>>>> +
>>>>> +	tsc_base = of_iomap(np, 0);
>>>>> +	if (!tsc_base)
>>>>> +		return -ENODEV;
>>>>> +
>>>>> +	clk = clk_get_sys("clk_m", NULL);
>>>>> +	if (IS_ERR(clk)) {
>>>>> +		freq = 12000000;
>>>>> +		pr_warn("Unable to get timer clock. Assuming 12Mhz input clock.\n");
>>>>> +	} else {
>>>>> +		freq = clk_get_rate(clk);
>>>>> +		clk_put(clk);
>>>>> +	}
>>>>> +	writel_relaxed(freq, tsc_base + TSC_CNTFID0);
>>>>> +
>>>>> +	/* CNTFRQ */
>>>>> +	asm("mcr p15, 0, %0, c14, c0, 0\n" : : "r" (freq));
>>>>> +	asm("mrc p15, 0, %0, c14, c0, 0\n" : "=r" (val));
>>>>> +	BUG_ON(val != freq);
>>>>
>>>> This is scary. CNTFRQ is only writable from secure mode, and will
>>>> explode in any other situation.
>>>>
>>>> Also, writing to CNTFRQ doesn't change the timer frequency! This is just
>>>> a way for secure mode to tell the rest of the world the frequency the
>>>> timer is ticking at. Unless you've wired the input clock to be able to
>>>> change the frequency?
>>>
>>> ATM, our upstream kernel is expected in secure mode. This situation
>>> may be changed later, though....
>>
>> I appreciate this. But I expect this kernel to be also used on the
>> non-secure side if someone tried to run KVM with it. And this would go
>> bang right away.
> 
> What is the common way to make the above code selective at run
> time(secure or non-secure path)?

All platforms are currently doing it in their firmware/bootloader, which
runs on the secure side. The kernel itself should be able to run on both
sides.

	M.
Hiroshi Doyu - Dec. 20, 2012, 2:42 p.m.
Marc Zyngier <marc.zyngier@arm.com> wrote @ Thu, 20 Dec 2012 14:32:21 +0100:

> On 20/12/12 12:55, Peter De Schrijver wrote:
> > On Thu, Dec 20, 2012 at 01:33:42PM +0100, Marc Zyngier wrote:
> >> On 20/12/12 12:22, Peter De Schrijver wrote:
> >>>>>>> +
> >>>>>>> +	/* CNTFRQ */
> >>>>>>> +	asm("mcr p15, 0, %0, c14, c0, 0\n" : : "r" (freq));
> >>>>>>> +	asm("mrc p15, 0, %0, c14, c0, 0\n" : "=r" (val));
> >>>>>>> +	BUG_ON(val != freq);
> >>>>>>
> >>>>>> This is scary. CNTFRQ is only writable from secure mode, and will
> >>>>>> explode in any other situation.
> >>>>>>
> >>>>>> Also, writing to CNTFRQ doesn't change the timer frequency! This is just
> >>>>>> a way for secure mode to tell the rest of the world the frequency the
> >>>>>> timer is ticking at. Unless you've wired the input clock to be able to
> >>>>>> change the frequency?
> >>>>>
> >>>>> ATM, our upstream kernel is expected in secure mode. This situation
> >>>>> may be changed later, though....
> >>>>
> >>>> I appreciate this. But I expect this kernel to be also used on the
> >>>> non-secure side if someone tried to run KVM with it. And this would go
> >>>> bang right away.
> >>>>
> >>>
> >>> But the guest wouldn't necessarily have this peripheral, or any other Tegra114
> >>> peripheral for that matter?
> >>
> >> The problem is not so much the guest but the host. The host has to be
> >> booted in non-secure, so just saying "we do not support non-secure" is
> >> not a very convincing argument.
> >>
> >> Unless of course you've already decided that you don't want to support
> >> KVM on this SoC...
> >>
> > 
> > I guess that means we can't support KVM yet. Tegra does not have a secure
> > monitor by default. It all depends on what that system integrator does.
> 
> VExpress doesn't have a secure monitor either, and yet we run KVM on it
> (by switching to non-secure before loading the kernel). Same for Exynos5.
> 
> What I'm trying to say is that this code is rather pointless (this
> should be done by the firmware/bootloader, not the kernel, or the
> information should be provided in DT if CNTFRQ is not set).

"tegra114.dtsi" has the folloiwng "tsc" entry. So can we consider that
if dts has this entry, CNTFRQ is not set, which implies it's in secure
mode. kernel should set it up by itself? Otherwise, skip this setup
and use it. For example:

 	tsc {
 		compatible = "nvidia,tegra114-tsc";
 		reg = <0x700f0000 0x20000>;
+		setup-cntfrq;
 	};

Is this what you explained in the above?
At least, kernel can survive without bootloader/firmware support, ATM.
--
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
Marc Zyngier - Dec. 20, 2012, 5:09 p.m.
On 20/12/12 14:42, Hiroshi Doyu wrote:
> Marc Zyngier <marc.zyngier@arm.com> wrote @ Thu, 20 Dec 2012 14:32:21 +0100:
> 
>> On 20/12/12 12:55, Peter De Schrijver wrote:
>>> On Thu, Dec 20, 2012 at 01:33:42PM +0100, Marc Zyngier wrote:
>>>> On 20/12/12 12:22, Peter De Schrijver wrote:
>>>>>>>>> +
>>>>>>>>> +	/* CNTFRQ */
>>>>>>>>> +	asm("mcr p15, 0, %0, c14, c0, 0\n" : : "r" (freq));
>>>>>>>>> +	asm("mrc p15, 0, %0, c14, c0, 0\n" : "=r" (val));
>>>>>>>>> +	BUG_ON(val != freq);
>>>>>>>>
>>>>>>>> This is scary. CNTFRQ is only writable from secure mode, and will
>>>>>>>> explode in any other situation.
>>>>>>>>
>>>>>>>> Also, writing to CNTFRQ doesn't change the timer frequency! This is just
>>>>>>>> a way for secure mode to tell the rest of the world the frequency the
>>>>>>>> timer is ticking at. Unless you've wired the input clock to be able to
>>>>>>>> change the frequency?
>>>>>>>
>>>>>>> ATM, our upstream kernel is expected in secure mode. This situation
>>>>>>> may be changed later, though....
>>>>>>
>>>>>> I appreciate this. But I expect this kernel to be also used on the
>>>>>> non-secure side if someone tried to run KVM with it. And this would go
>>>>>> bang right away.
>>>>>>
>>>>>
>>>>> But the guest wouldn't necessarily have this peripheral, or any other Tegra114
>>>>> peripheral for that matter?
>>>>
>>>> The problem is not so much the guest but the host. The host has to be
>>>> booted in non-secure, so just saying "we do not support non-secure" is
>>>> not a very convincing argument.
>>>>
>>>> Unless of course you've already decided that you don't want to support
>>>> KVM on this SoC...
>>>>
>>>
>>> I guess that means we can't support KVM yet. Tegra does not have a secure
>>> monitor by default. It all depends on what that system integrator does.
>>
>> VExpress doesn't have a secure monitor either, and yet we run KVM on it
>> (by switching to non-secure before loading the kernel). Same for Exynos5.
>>
>> What I'm trying to say is that this code is rather pointless (this
>> should be done by the firmware/bootloader, not the kernel, or the
>> information should be provided in DT if CNTFRQ is not set).
> 
> "tegra114.dtsi" has the folloiwng "tsc" entry. So can we consider that
> if dts has this entry, CNTFRQ is not set, which implies it's in secure
> mode. kernel should set it up by itself? Otherwise, skip this setup
> and use it. For example:
> 
>  	tsc {
>  		compatible = "nvidia,tegra114-tsc";
>  		reg = <0x700f0000 0x20000>;
> +		setup-cntfrq;
>  	};
> 
> Is this what you explained in the above?
> At least, kernel can survive without bootloader/firmware support, ATM.

No. The DT should only describe the hardware, and not something that is
Linux specific.

Just use the "clock-frequency" attribute in the timer arch-timer node,
and get rid of this CNTFRQ setting. The driver already knows how to deal
with this situation if this attribute is set.

Thanks,

	M.
Peter De Schrijver - Dec. 20, 2012, 10:13 p.m.
> > 
> > "tegra114.dtsi" has the folloiwng "tsc" entry. So can we consider that
> > if dts has this entry, CNTFRQ is not set, which implies it's in secure
> > mode. kernel should set it up by itself? Otherwise, skip this setup
> > and use it. For example:
> > 
> >  	tsc {
> >  		compatible = "nvidia,tegra114-tsc";
> >  		reg = <0x700f0000 0x20000>;
> > +		setup-cntfrq;
> >  	};
> > 
> > Is this what you explained in the above?
> > At least, kernel can survive without bootloader/firmware support, ATM.
> 
> No. The DT should only describe the hardware, and not something that is
> Linux specific.
> 
> Just use the "clock-frequency" attribute in the timer arch-timer node,
> and get rid of this CNTFRQ setting. The driver already knows how to deal
> with this situation if this attribute is set.

The frequency is probed at boottime by the kernel though. It will not
necessarily be the same for every board.

Cheers,

Peter.
--
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

Patch

diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra114-tsc.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra114-tsc.txt
new file mode 100644
index 0000000..9de936a
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra114-tsc.txt
@@ -0,0 +1,11 @@ 
+NVIDIA Tegra Timer Stamp Counter(TSC)
+
+Required properties:
+- compatible : "nvidia,tegra114-tsc
+- reg : Should contain 1 register ranges(address and length)
+
+Example:
+	tsc {
+		compatible = "nvidia,tegra114-tsc";
+		reg = <0x700f0000 0x20000>;
+	};
diff --git a/drivers/clocksource/tegra20_timer.c b/drivers/clocksource/tegra20_timer.c
index 1d25de8..285a6f1 100644
--- a/drivers/clocksource/tegra20_timer.c
+++ b/drivers/clocksource/tegra20_timer.c
@@ -30,6 +30,7 @@ 
 #include <asm/mach/time.h>
 #include <asm/smp_twd.h>
 #include <asm/sched_clock.h>
+#include <asm/arch_timer.h>
 
 #define RTC_SECONDS            0x08
 #define RTC_SHADOW_SECONDS     0x0c
@@ -271,10 +272,71 @@  static void __init tegra20_init_tmr(void)
 	clockevents_register_device(&tegra_clockevent);
 }
 
+#define TSC_CNTCR		0		/* TSC control registers */
+#define TSC_CNTCR_ENABLE	(1 << 0)	/* Enable */
+#define TSC_CNTCR_HDBG		(1 << 1)	/* Halt on debug */
+
+#define TSC_CNTCV0		0x8		/* TSC counter (LSW) */
+#define TSC_CNTCV1		0xc		/* TSC counter (MSW) */
+#define TSC_CNTFID0		0x20		/* TSC freq id 0 */
+
+static const struct of_device_id tegra_tsc_match[] __initconst = {
+	{ .compatible = "nvidia,tegra114-tsc" },
+	{}
+};
+
+static int tegra_arch_timer_init(void)
+{
+	int err;
+	struct device_node *np;
+	struct clk *clk;
+	void __iomem *tsc_base;
+	u32 freq, val;
+
+	np = of_find_matching_node(NULL, tegra_tsc_match);
+	if (!np)
+		return -ENODEV;
+
+	tsc_base = of_iomap(np, 0);
+	if (!tsc_base)
+		return -ENODEV;
+
+	clk = clk_get_sys("clk_m", NULL);
+	if (IS_ERR(clk)) {
+		freq = 12000000;
+		pr_warn("Unable to get timer clock. Assuming 12Mhz input clock.\n");
+	} else {
+		freq = clk_get_rate(clk);
+		clk_put(clk);
+	}
+	writel_relaxed(freq, tsc_base + TSC_CNTFID0);
+
+	/* CNTFRQ */
+	asm("mcr p15, 0, %0, c14, c0, 0\n" : : "r" (freq));
+	asm("mrc p15, 0, %0, c14, c0, 0\n" : "=r" (val));
+	BUG_ON(val != freq);
+
+	val = readl_relaxed(tsc_base + TSC_CNTCR);
+	val |= TSC_CNTCR_ENABLE | TSC_CNTCR_HDBG;
+	writel_relaxed(val, tsc_base + TSC_CNTCR);
+
+	err = arch_timer_of_register();
+	if (!err)
+		err = arch_timer_sched_clock_init();
+	if (err)
+		pr_err("Failed to register ARM arch_timer(TSC)\n");
+	return err;
+}
+
 static void __init tegra20_init_timer(void)
 {
+	int err = -ENODEV;
+
 	tegra20_init_tmr();
-	setup_sched_clock(tegra_read_sched_clock, 32, 1000000);
+	if (IS_ENABLED(CONFIG_ARM_ARCH_TIMER))
+		err = tegra_arch_timer_init();
+	if (err)
+		setup_sched_clock(tegra_read_sched_clock, 32, 1000000);
 	tegra20_init_rtc();
 #ifdef CONFIG_HAVE_ARM_TWD
 	twd_local_timer_of_register();