[PATCH 1/1] Clocksource: Add sched_clock to Atmel TCB clocksource

[PATCH 1/1] Clocksource: Add sched_clock to Atmel TCB clocksource

[Carsten Emde]

[-- Attachment #1: drivers-clocksource-tcb-register-sched-clock.patch --]
[-- Type: text/plain, Size: 1958 bytes --]

The default sched_clock provides a maximum resolution of 1 ms which
normally is not sufficient to investigate scheduling related
irregularities - more so, if a low-latency or even a real-time kernel is
used. Therefore, register the Atmel TCB clocksource that provides a
resolution of 63 ns as sched_clock.

Signed-off-by: Carsten Emde <C.Emde@osadl.org>

---
 drivers/clocksource/tcb_clksrc.c |   15 +++++++++++++++
 1 file changed, 15 insertions(+)

Index: linux-3.18.13-rt10-r7s4/drivers/clocksource/tcb_clksrc.c
===================================================================
--- linux-3.18.13-rt10-r7s4.orig/drivers/clocksource/tcb_clksrc.c
+++ linux-3.18.13-rt10-r7s4/drivers/clocksource/tcb_clksrc.c
@@ -10,6 +10,7 @@
 #include <linux/io.h>
 #include <linux/platform_device.h>
 #include <linux/atmel_tc.h>
+#include <linux/sched_clock.h>
 
 
 /*
@@ -40,6 +41,7 @@
  */
 
 static void __iomem *tcaddr;
+static cycle_t (*do_sched_clock_get_cycles)(struct clocksource *cs);
 
 static cycle_t tc_get_cycles(struct clocksource *cs)
 {
@@ -61,6 +63,13 @@ static cycle_t tc_get_cycles32(struct cl
 	return __raw_readl(tcaddr + ATMEL_TC_REG(0, CV));
 }
 
+u64 sched_clock_get_cycles(void)
+{
+	struct clocksource *cs = NULL;
+
+	return (u64) do_sched_clock_get_cycles(cs);
+}
+
 static struct clocksource clksrc = {
 	.name           = "tcb_clksrc",
 	.rating         = 200,
@@ -273,6 +282,7 @@ static int __init tcb_clksrc_init(void)
 	int clk32k_divisor_idx = -1;
 	int i;
 	int ret;
+	unsigned long flags;
 
 	tc = atmel_tc_alloc(CONFIG_ATMEL_TCB_CLKSRC_BLOCK);
 	if (!tc) {
@@ -339,6 +349,11 @@ static int __init tcb_clksrc_init(void)
 	if (ret)
 		goto err_disable_t1;
 
+	do_sched_clock_get_cycles = clksrc.read;
+	local_irq_save(flags);
+	sched_clock_register(sched_clock_get_cycles, 32, divided_rate);
+	local_irq_restore(flags);
+
 	/* channel 2:  periodic and oneshot timer support */
 	ret = setup_clkevents(tc, clk32k_divisor_idx);
 	if (ret)

Re: [PATCH 1/1] Clocksource: Add sched_clock to Atmel TCB clocksource

[Thomas Gleixner]

On Tue, 2 Jun 2015, Carsten Emde wrote:
>  static void __iomem *tcaddr;
> +static cycle_t (*do_sched_clock_get_cycles)(struct clocksource *cs);

what's that for? Indirection for indirection sake?

>  static cycle_t tc_get_cycles(struct clocksource *cs)
>  {
> @@ -61,6 +63,13 @@ static cycle_t tc_get_cycles32(struct cl
>  	return __raw_readl(tcaddr + ATMEL_TC_REG(0, CV));
>  }
>  
> +u64 sched_clock_get_cycles(void)

Static? 

> +{
> +	struct clocksource *cs = NULL;
> +
> +	return (u64) do_sched_clock_get_cycles(cs);


What's wrong with implementing this like:

static u64 sched_clock_get_cycles(void)
{
	return tc_get_cycles(NULL); 
}

Hmm?

> @@ -273,6 +282,7 @@ static int __init tcb_clksrc_init(void)
>  	int clk32k_divisor_idx = -1;
>  	int i;
>  	int ret;
> +	unsigned long flags;
>  
>  	tc = atmel_tc_alloc(CONFIG_ATMEL_TCB_CLKSRC_BLOCK);
>  	if (!tc) {
> @@ -339,6 +349,11 @@ static int __init tcb_clksrc_init(void)
>  	if (ret)
>  		goto err_disable_t1;
>  
> +	do_sched_clock_get_cycles = clksrc.read;

Sigh.

> +	local_irq_save(flags);

What's the purpose of this?

Thanks,

	tglx

Re: [PATCH 1/1] Clocksource: Add sched_clock to Atmel TCB clocksource

[Carsten Emde]

Hi Thomas,

>>   static void __iomem *tcaddr;
>> +static cycle_t (*do_sched_clock_get_cycles)(struct clocksource *cs);
>
> what's that for? Indirection for indirection sake?
The code apparently provides two different functions to read the current 
clock - depending on whether the clock is setup in single-channel mode 
or not.

The default function is predefined in the struct clocksource clksrc:
   .read = tc_get_cycles
This structure element may later on be overwritten, if the counter is 
configured in single-channel mode:
   clksrc.read = tc_get_cycles32;

This is why my code sets do_sched_clock_get_cycles to clksrc.read before 
sched_clock is registered to make sure the appropriate read function 
will be used.

>> +u64 sched_clock_get_cycles(void)
> Static?
Yes, sure, should be static. Will fix it.

> What's wrong with implementing this like:
>
> static u64 sched_clock_get_cycles(void)
> {
> 	return tc_get_cycles(NULL);
> }
> Hmm?
See above.

>> +	local_irq_save(flags);
> What's the purpose of this?
The sched_clock_register() function contains
   WARN_ON(!irqs_disabled());
which I did not want to trigger.

Thanks,
	-Carsten.

Re: [PATCH 1/1] Clocksource: Add sched_clock to Atmel TCB clocksource

[Thomas Gleixner]

On Tue, 2 Jun 2015, Carsten Emde wrote:
> > >   static void __iomem *tcaddr;
> > > +static cycle_t (*do_sched_clock_get_cycles)(struct clocksource *cs);
> > 
> > what's that for? Indirection for indirection sake?
> The code apparently provides two different functions to read the current clock
> - depending on whether the clock is setup in single-channel mode or not.
> 
> The default function is predefined in the struct clocksource clksrc:
>   .read = tc_get_cycles
> This structure element may later on be overwritten, if the counter is
> configured in single-channel mode:
>   clksrc.read = tc_get_cycles32;
> 
> This is why my code sets do_sched_clock_get_cycles to clksrc.read before
> sched_clock is registered to make sure the appropriate read function will be
> used.

  return clksrc.read(NULL);

then?
 
> > > +	local_irq_save(flags);
> > What's the purpose of this?
> The sched_clock_register() function contains
>   WARN_ON(!irqs_disabled());
> which I did not want to trigger.

That code is run in the early setup with interrupts disabled unless
I'm missing something ...

Thanks,

	tglx

Re: [PATCH 1/1] Clocksource: Add sched_clock to Atmel TCB clocksource

[Carsten Emde]

Hi Thomas,

>>>>    static void __iomem *tcaddr;
>>>> +static cycle_t (*do_sched_clock_get_cycles)(struct clocksource *cs);
>>>
>>> what's that for? Indirection for indirection sake?
>> The code apparently provides two different functions to read the current clock
>> - depending on whether the clock is setup in single-channel mode or not.
>>
>> The default function is predefined in the struct clocksource clksrc:
>>    .read = tc_get_cycles
>> This structure element may later on be overwritten, if the counter is
>> configured in single-channel mode:
>>    clksrc.read = tc_get_cycles32;
>>
>> This is why my code sets do_sched_clock_get_cycles to clksrc.read before
>> sched_clock is registered to make sure the appropriate read function will be
>> used.
>    return clksrc.read(NULL);
> then?
Much better, of course, V2 will have it.

>>>> +	local_irq_save(flags);
>>> What's the purpose of this?
>> The sched_clock_register() function contains
>>    WARN_ON(!irqs_disabled());
>> which I did not want to trigger.
> That code is run in the early setup with interrupts disabled unless
> I'm missing something ...
Maybe, something else is wrong, and interrupts are enabled too early on 
this system. I added the lines around sched_clock_register(), because 
WARN_ON triggered. Will investigate further.

Thanks,
	-Carsten.