Trivial clocksource driver

Trivial clocksource driver

[Mason]

Hello everyone,

I am trying to submit a new ARM port, and Arnd pointed out that the
clocksource code could not live in arch/arm/$PLATFORM, but had to
move to drivers/clocksource (and it had to support DT).

Did I understand correctly? Is this the right place to submit code
as provided below?

Regards.


#include <linux/delay.h>	/* register_current_timer_delay	*/
#include <linux/clocksource.h>	/* clocksource_register_hz	*/
#include <linux/sched_clock.h>	/* sched_clock_register		*/
#include <linux/of_address.h>	/* of_iomap			*/
#include <linux/clk.h>		/* of_clk_get, clk_get_rate	*/

static void __iomem *xtal_in_cnt;
static struct delay_timer delay_timer;

static unsigned long read_xtal_counter(void)
{
	return readl_relaxed(xtal_in_cnt);
}

static u64 read_sched_clock(void)
{
	return read_xtal_counter();
}

static cycle_t read_clocksource(struct clocksource *cs)
{
	return read_xtal_counter();
}

static struct clocksource tango_xtal = {
	.name	= "tango-xtal",
	.rating	= 350,
	.read	= read_clocksource,
	.mask	= CLOCKSOURCE_MASK(32),
	.flags	= CLOCK_SOURCE_IS_CONTINUOUS,
};

static void __init tango_clksrc_init(struct device_node *np)
{
	struct clk *clk = of_clk_get(np, 0);
	unsigned int xtal_freq = clk_get_rate(clk);
	xtal_in_cnt = of_iomap(np, 0);

	delay_timer.freq = xtal_freq;
	delay_timer.read_current_timer = read_xtal_counter;
	register_current_timer_delay(&delay_timer);
	sched_clock_register(read_sched_clock, 32, xtal_freq);
	clocksource_register_hz(&tango_xtal, xtal_freq);
}

CLOCKSOURCE_OF_DECLARE(tango, "sigma,xtal_in_cnt", tango_clksrc_init);

Re: Trivial clocksource driver

[Thomas Gleixner]

On Tue, 29 Sep 2015, Mason wrote:
> Hello everyone,
> 
> I am trying to submit a new ARM port, and Arnd pointed out that the
> clocksource code could not live in arch/arm/$PLATFORM, but had to
> move to drivers/clocksource (and it had to support DT).
> 
> Did I understand correctly? Is this the right place to submit code
> as provided below?


Yes, drivers/clocksource is the right place. You just need to submit a
formal patch, which includes a proper subject line, changelog, plus
the necessary Makefile and Kconfig modifications.
 
> #include <linux/delay.h>	/* register_current_timer_delay	*/

Please get rid of these silly tail comments. They provide absolutely
no value.

Other than that this looks reasonable.

Thanks,

	tglx

Re: Trivial clocksource driver

[Mason]

On 29/09/2015 20:32, Thomas Gleixner wrote:

> On Tue, 29 Sep 2015, Mason wrote:
> 
>> I am trying to submit a new ARM port, and Arnd pointed out that the
>> clocksource code could not live in arch/arm/$PLATFORM, but had to
>> move to drivers/clocksource (and it had to support DT).
>>
>> Did I understand correctly? Is this the right place to submit code
>> as provided below?
> 
> Yes, drivers/clocksource is the right place. You just need to submit a
> formal patch, which includes a proper subject line, changelog, plus
> the necessary Makefile and Kconfig modifications.

OK, I'll send a formal patch tomorrow.
There are no Kconfig modifications, is that OK?

Also, that patch is part of a larger patch-set (most of the
patches intended for arch/arm). I should send you only the
clocksource patch, or the whole patch-set?

>> #include <linux/delay.h>	/* register_current_timer_delay	*/
> 
> Please get rid of these silly tail comments. They provide absolutely
> no value.

I will remove them, since you asked.

In my opinion, they serve one purpose: if code is refactored,
and the function call is removed, the comment is a reminder
to also remove the relevant include directive.

Do you disagree?

> Other than that this looks reasonable.

Just wanted to ask:
Can register_current_timer_delay, sched_clock_register, and
clocksource_register_hz be called in any order?

Regards.

Re: Trivial clocksource driver

[Måns Rullgård]

Mason <slash.tmp@free.fr> writes:

> On 29/09/2015 20:32, Thomas Gleixner wrote:
>
>> On Tue, 29 Sep 2015, Mason wrote:
>> 
>>> I am trying to submit a new ARM port, and Arnd pointed out that the
>>> clocksource code could not live in arch/arm/$PLATFORM, but had to
>>> move to drivers/clocksource (and it had to support DT).
>>>
>>> Did I understand correctly? Is this the right place to submit code
>>> as provided below?
>> 
>> Yes, drivers/clocksource is the right place. You just need to submit a
>> formal patch, which includes a proper subject line, changelog, plus
>> the necessary Makefile and Kconfig modifications.
>
> OK, I'll send a formal patch tomorrow.
> There are no Kconfig modifications, is that OK?

Why don't you use my driver?  It's even simpler, and it works just fine
on the 87xx chip.

https://github.com/mansr/linux-tangox/blob/master/drivers/clocksource/clksrc-tangox.c

-- 
Måns Rullgård
mans@mansr.com

Re: Trivial clocksource driver

[Thomas Gleixner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1055 bytes --]

On Tue, 29 Sep 2015, Måns Rullgård wrote:
> Mason <slash.tmp@free.fr> writes:
> 
> > On 29/09/2015 20:32, Thomas Gleixner wrote:
> >
> >> On Tue, 29 Sep 2015, Mason wrote:
> >> 
> >>> I am trying to submit a new ARM port, and Arnd pointed out that the
> >>> clocksource code could not live in arch/arm/$PLATFORM, but had to
> >>> move to drivers/clocksource (and it had to support DT).
> >>>
> >>> Did I understand correctly? Is this the right place to submit code
> >>> as provided below?
> >> 
> >> Yes, drivers/clocksource is the right place. You just need to submit a
> >> formal patch, which includes a proper subject line, changelog, plus
> >> the necessary Makefile and Kconfig modifications.
> >
> > OK, I'll send a formal patch tomorrow.
> > There are no Kconfig modifications, is that OK?
> 
> Why don't you use my driver?  It's even simpler, and it works just fine
> on the 87xx chip.
> 
> https://github.com/mansr/linux-tangox/blob/master/drivers/clocksource/clksrc-tangox.c

Perhaps because that driver is not upstream either?

Thanks,

	tglx

Re: Trivial clocksource driver

[Mason]

Hello Mans,

On 29/09/2015 22:18, Måns Rullgård wrote:

> Mason writes:
> 
>> On 29/09/2015 20:32, Thomas Gleixner wrote:
>>
>>> On Tue, 29 Sep 2015, Mason wrote:
>>>
>>>> I am trying to submit a new ARM port, and Arnd pointed out that the
>>>> clocksource code could not live in arch/arm/$PLATFORM, but had to
>>>> move to drivers/clocksource (and it had to support DT).
>>>>
>>>> Did I understand correctly? Is this the right place to submit code
>>>> as provided below?
>>>
>>> Yes, drivers/clocksource is the right place. You just need to submit a
>>> formal patch, which includes a proper subject line, changelog, plus
>>> the necessary Makefile and Kconfig modifications.
>>
>> OK, I'll send a formal patch tomorrow.
>> There are no Kconfig modifications, is that OK?
> 
> Why don't you use my driver?  It's even simpler, and it works just fine
> on the 87xx chip.
> 
> https://github.com/mansr/linux-tangox/blob/master/drivers/clocksource/clksrc-tangox.c

As you know, I am using two of your complex drivers, namely
ethernet and interrupt controller (which would have taken me
several weeks to write). I will be submitting them upstream
shortly, is that OK with you?

I'm not using this particular driver of yours because I had
already written the code, and porting it to DT was a good
learning process. This is probably my only opportunity to
actually write any kind of code for this port.

Regards.

Re: Trivial clocksource driver

[Måns Rullgård]

Mason <slash.tmp@free.fr> writes:

> Hello Mans,
>
> On 29/09/2015 22:18, Måns Rullgård wrote:
>
>> Mason writes:
>> 
>>> On 29/09/2015 20:32, Thomas Gleixner wrote:
>>>
>>>> On Tue, 29 Sep 2015, Mason wrote:
>>>>
>>>>> I am trying to submit a new ARM port, and Arnd pointed out that the
>>>>> clocksource code could not live in arch/arm/$PLATFORM, but had to
>>>>> move to drivers/clocksource (and it had to support DT).
>>>>>
>>>>> Did I understand correctly? Is this the right place to submit code
>>>>> as provided below?
>>>>
>>>> Yes, drivers/clocksource is the right place. You just need to submit a
>>>> formal patch, which includes a proper subject line, changelog, plus
>>>> the necessary Makefile and Kconfig modifications.
>>>
>>> OK, I'll send a formal patch tomorrow.
>>> There are no Kconfig modifications, is that OK?
>> 
>> Why don't you use my driver?  It's even simpler, and it works just fine
>> on the 87xx chip.
>> 
>> https://github.com/mansr/linux-tangox/blob/master/drivers/clocksource/clksrc-tangox.c
>
> As you know, I am using two of your complex drivers, namely
> ethernet and interrupt controller (which would have taken me
> several weeks to write). I will be submitting them upstream
> shortly, is that OK with you?

I want to test them a bit more on the 87xx first.  It's booting, but
there are a couple of niggles that should be looked into.  Getting some
documentation would expedite that process.

> I'm not using this particular driver of yours because I had
> already written the code, and porting it to DT was a good
> learning process. This is probably my only opportunity to
> actually write any kind of code for this port.

You should still be using the existing clocksource_mmio helper.  In
fact, that interface could be exposed using a generic DT binding.

-- 
Måns Rullgård
mans@mansr.com