[PATCH v1] clocksource: Sigma Designs Tango 27 MHz xtal

[PATCH v1] clocksource: Sigma Designs Tango 27 MHz xtal

[Marc Gonzalez]

Date: Tue, 6 Oct 2015 16:49:28 +0200
Subject: [PATCH] clocksource: Sigma Designs Tango 27 MHz xtal

Sigma Designs Tango platforms provide a 27 MHz crystal oscillator.
Use it for clocksource, sched_clock, and delay_timer.

Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
---
 drivers/clocksource/Makefile     |  1 +
 drivers/clocksource/tango_xtal.c | 46 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+)
 create mode 100644 drivers/clocksource/tango_xtal.c

diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index f228354961ca..46e405673d75 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -24,6 +24,7 @@ obj-$(CONFIG_ARCH_CLPS711X)	+= clps711x-timer.o
 obj-$(CONFIG_ARCH_ATLAS7)	+= timer-atlas7.o
 obj-$(CONFIG_ARCH_MOXART)	+= moxart_timer.o
 obj-$(CONFIG_ARCH_MXS)		+= mxs_timer.o
+obj-$(CONFIG_ARCH_TANGOX)	+= tango_xtal.o
 obj-$(CONFIG_CLKSRC_PXA)	+= pxa_timer.o
 obj-$(CONFIG_ARCH_PRIMA2)	+= timer-prima2.o
 obj-$(CONFIG_ARCH_U300)		+= timer-u300.o
diff --git a/drivers/clocksource/tango_xtal.c b/drivers/clocksource/tango_xtal.c
new file mode 100644
index 000000000000..0f2fb293ab75
--- /dev/null
+++ b/drivers/clocksource/tango_xtal.c
@@ -0,0 +1,46 @@
+#include <linux/clocksource.h>
+#include <linux/sched_clock.h>
+#include <linux/of_address.h>
+#include <linux/delay.h>
+#include <linux/clk.h>
+
+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_clocksource_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,tick-counter", tango_clocksource_init);
-- 
2.4.5

Re: [PATCH v1] clocksource: Sigma Designs Tango 27 MHz xtal

[Daniel Lezcano]

Hi Marc,

On 10/06/2015 05:10 PM, Marc Gonzalez wrote:
> Date: Tue, 6 Oct 2015 16:49:28 +0200
> Subject: [PATCH] clocksource: Sigma Designs Tango 27 MHz xtal

Fix the patch format.

Subject is clocksource/drivers/tango_xtal: Add new timer ...

> Sigma Designs Tango platforms provide a 27 MHz crystal oscillator.
> Use it for clocksource, sched_clock, and delay_timer.
>
> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> ---
>   drivers/clocksource/Makefile     |  1 +
>   drivers/clocksource/tango_xtal.c | 46 ++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 47 insertions(+)
>   create mode 100644 drivers/clocksource/tango_xtal.c
>
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index f228354961ca..46e405673d75 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -24,6 +24,7 @@ obj-$(CONFIG_ARCH_CLPS711X)	+= clps711x-timer.o
>   obj-$(CONFIG_ARCH_ATLAS7)	+= timer-atlas7.o
>   obj-$(CONFIG_ARCH_MOXART)	+= moxart_timer.o
>   obj-$(CONFIG_ARCH_MXS)		+= mxs_timer.o
> +obj-$(CONFIG_ARCH_TANGOX)	+= tango_xtal.o

Please use the following scheme:

Add in the clocksource's Kconfig an entry:

config CLKSRC_TANGO_XTAL
	bool "bla bla" if COMPILE_TEST
	select CLKSRC_OF

and then in the arch's Kconfig:

select CLKSRC_TANGO_XTAL


>   obj-$(CONFIG_CLKSRC_PXA)	+= pxa_timer.o
>   obj-$(CONFIG_ARCH_PRIMA2)	+= timer-prima2.o
>   obj-$(CONFIG_ARCH_U300)		+= timer-u300.o
> diff --git a/drivers/clocksource/tango_xtal.c b/drivers/clocksource/tango_xtal.c
> new file mode 100644
> index 000000000000..0f2fb293ab75
> --- /dev/null
> +++ b/drivers/clocksource/tango_xtal.c
> @@ -0,0 +1,46 @@
> +#include <linux/clocksource.h>
> +#include <linux/sched_clock.h>
> +#include <linux/of_address.h>
> +#include <linux/delay.h>
> +#include <linux/clk.h>

#include <linux/init.h> is missing.

> +
> +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 u64 *notrace* read_sched_clock(void)

> +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_clocksource_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);

check return code.

> +	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);

check return code.

> +}
> +
> +CLOCKSOURCE_OF_DECLARE(tango, "sigma,tick-counter", tango_clocksource_init);
>


-- 
  <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 v1] clocksource: Sigma Designs Tango 27 MHz xtal

[Marc Gonzalez]

Hello everyone,

On 07/10/2015 01:09, Daniel Lezcano wrote:

> On 10/06/2015 05:10 PM, Marc Gonzalez wrote:
>> Date: Tue, 6 Oct 2015 16:49:28 +0200
>> Subject: [PATCH] clocksource: Sigma Designs Tango 27 MHz xtal
> 
> Fix the patch format.

OK.

> Subject is clocksource/drivers/tango_xtal: Add new timer ...

OK.

(84583983c319 didn't seem to follow that convention)

>> Sigma Designs Tango platforms provide a 27 MHz crystal oscillator.
>> Use it for clocksource, sched_clock, and delay_timer.
>>
>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>> ---
>>   drivers/clocksource/Makefile     |  1 +
>>   drivers/clocksource/tango_xtal.c | 46 ++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 47 insertions(+)
>>   create mode 100644 drivers/clocksource/tango_xtal.c
>>
>> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
>> index f228354961ca..46e405673d75 100644
>> --- a/drivers/clocksource/Makefile
>> +++ b/drivers/clocksource/Makefile
>> @@ -24,6 +24,7 @@ obj-$(CONFIG_ARCH_CLPS711X)	+= clps711x-timer.o
>>   obj-$(CONFIG_ARCH_ATLAS7)	+= timer-atlas7.o
>>   obj-$(CONFIG_ARCH_MOXART)	+= moxart_timer.o
>>   obj-$(CONFIG_ARCH_MXS)		+= mxs_timer.o
>> +obj-$(CONFIG_ARCH_TANGOX)	+= tango_xtal.o
> 
> Please use the following scheme:
> 
> Add in the clocksource's Kconfig an entry:
> 
> config CLKSRC_TANGO_XTAL
> 	bool "bla bla" if COMPILE_TEST
> 	select CLKSRC_OF
> 
> and then in the arch's Kconfig:
> 
> select CLKSRC_TANGO_XTAL

OK.

>>   obj-$(CONFIG_CLKSRC_PXA)	+= pxa_timer.o
>>   obj-$(CONFIG_ARCH_PRIMA2)	+= timer-prima2.o
>>   obj-$(CONFIG_ARCH_U300)		+= timer-u300.o
>> diff --git a/drivers/clocksource/tango_xtal.c b/drivers/clocksource/tango_xtal.c
>> new file mode 100644
>> index 000000000000..0f2fb293ab75
>> --- /dev/null
>> +++ b/drivers/clocksource/tango_xtal.c
>> @@ -0,0 +1,46 @@
>> +#include <linux/clocksource.h>
>> +#include <linux/sched_clock.h>
>> +#include <linux/of_address.h>
>> +#include <linux/delay.h>
>> +#include <linux/clk.h>
> 
> #include <linux/init.h> is missing.

For my education: this header is already indirectly included;
The directive should be explicit to prevent breakage in case
other (implicit) includes are reorganized?

>> +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 u64 *notrace* read_sched_clock(void)

What about read_clocksource? and 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_clocksource_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);
> 
> check return code.

OK.

>> +	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);
> 
> check return code.

OK.

New patch coming up.

Regards.

Re: [PATCH v1] clocksource: Sigma Designs Tango 27 MHz xtal

[Daniel Lezcano]

On 10/07/2015 10:23 AM, Marc Gonzalez wrote:

[ ... ]

>>> +++ b/drivers/clocksource/tango_xtal.c
>>> @@ -0,0 +1,46 @@
>>> +#include <linux/clocksource.h>
>>> +#include <linux/sched_clock.h>
>>> +#include <linux/of_address.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/clk.h>
>>
>> #include <linux/init.h> is missing.
>
> For my education: this header is already indirectly included;
> The directive should be explicit to prevent breakage in case
> other (implicit) includes are reorganized?

Yes, exactly.

The 'clocksource.h' header includes 'init.h' only for the 
'clocksource_default_clock' function. If this function disappears, or 
the __init section is removed, then the 'init.h' inclusion won't make 
sense anymore in the header file. By removing it, that will break all 
code relying on the implicit inclusion.

So the rule of thumb I apply to myself is always to explicitly include 
the headers in the C file even if it is already implicitly included. 
This rule is valid only if the header file is not using intensively code 
coming from the implicit inclusion like static inlines etc ...

>>> +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 u64 *notrace* read_sched_clock(void)
>
> What about read_clocksource? and read_xtal_counter?

See commit 89e6a13b88.

   -- 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 v1] clocksource: Sigma Designs Tango 27 MHz xtal

[Marc Gonzalez]

On 07/10/2015 11:47, Daniel Lezcano wrote:
> On 10/07/2015 10:23 AM, Marc Gonzalez wrote:
>> Daniel Lezcano wrote:
>>
>>> static u64 *notrace* read_sched_clock(void)
>>
>> What about read_clocksource? and read_xtal_counter?
> 
> See commit 89e6a13b88.

If I understand correctly, since read_sched_clock calls read_xtal_counter,
both functions should be marked notrace.

Regards.

[PATCH v2] clocksource/drivers/tango_xtal: Add new timer for Tango SoCs

[Marc Gonzalez]

Sigma Designs Tango platforms provide a 27 MHz crystal oscillator.
Use it for clocksource, sched_clock, and delay_timer.

Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
---
AFAICS, clocksource_register_hz does not report failures via its
return value (always 0) but writes warnings to stdout?

Open question: can I call register_current_timer_delay,
sched_clock_register, clocksource_register_hz in any order?
---
 drivers/clocksource/Kconfig      |  4 ++++
 drivers/clocksource/Makefile     |  1 +
 drivers/clocksource/tango_xtal.c | 49 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 54 insertions(+)
 create mode 100644 drivers/clocksource/tango_xtal.c

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 4e57730e0be4..63c878d4d9e3 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -275,6 +275,10 @@ config CLKSRC_MIPS_GIC
 	depends on MIPS_GIC
 	select CLKSRC_OF
 
+config CLKSRC_TANGO_XTAL
+	bool
+	select CLKSRC_OF
+
 config CLKSRC_PXA
 	def_bool y if ARCH_PXA || ARCH_SA1100
 	select CLKSRC_OF if USE_OF
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index f228354961ca..160970a3963b 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -55,6 +55,7 @@ obj-$(CONFIG_ARCH_KEYSTONE)		+= timer-keystone.o
 obj-$(CONFIG_ARCH_INTEGRATOR_AP)	+= timer-integrator-ap.o
 obj-$(CONFIG_CLKSRC_VERSATILE)		+= versatile.o
 obj-$(CONFIG_CLKSRC_MIPS_GIC)		+= mips-gic-timer.o
+obj-$(CONFIG_CLKSRC_TANGO_XTAL)		+= tango_xtal.o
 obj-$(CONFIG_CLKSRC_IMX_GPT)		+= timer-imx-gpt.o
 obj-$(CONFIG_ASM9260_TIMER)		+= asm9260_timer.o
 obj-$(CONFIG_H8300)			+= h8300_timer8.o
diff --git a/drivers/clocksource/tango_xtal.c b/drivers/clocksource/tango_xtal.c
new file mode 100644
index 000000000000..c67599457782
--- /dev/null
+++ b/drivers/clocksource/tango_xtal.c
@@ -0,0 +1,49 @@
+#include <linux/clocksource.h>
+#include <linux/sched_clock.h>
+#include <linux/of_address.h>
+#include <linux/delay.h>
+#include <linux/init.h>
+#include <linux/clk.h>
+
+static void __iomem *xtal_in_cnt;
+static struct delay_timer delay_timer;
+
+static unsigned long notrace read_xtal_counter(void)
+{
+	return readl_relaxed(xtal_in_cnt);
+}
+
+static u64 notrace 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_clocksource_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);
+	if (xtal_in_cnt == NULL)
+		panic("%s: of_iomap failed\n", np->full_name);
+
+	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,tick-counter", tango_clocksource_init);
-- 
2.4.5

Re: [PATCH v1] clocksource: Sigma Designs Tango 27 MHz xtal

[Daniel Lezcano]

On 10/07/2015 01:14 PM, Marc Gonzalez wrote:
> On 07/10/2015 11:47, Daniel Lezcano wrote:
>> On 10/07/2015 10:23 AM, Marc Gonzalez wrote:
>>> Daniel Lezcano wrote:
>>>
>>>> static u64 *notrace* read_sched_clock(void)
>>>
>>> What about read_clocksource? and read_xtal_counter?
>>
>> See commit 89e6a13b88.
>
> If I understand correctly, since read_sched_clock calls read_xtal_counter,
> both functions should be marked notrace.

Yes. We have to prevent the recursion with the ftrace timestamp.


-- 
  <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 v2] clocksource/drivers/tango_xtal: Add new timer for Tango SoCs

[Daniel Lezcano]

On 10/07/2015 01:35 PM, Marc Gonzalez wrote:
> Sigma Designs Tango platforms provide a 27 MHz crystal oscillator.
> Use it for clocksource, sched_clock, and delay_timer.
>
> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> ---
> AFAICS, clocksource_register_hz does not report failures via its
> return value (always 0) but writes warnings to stdout?

Yeah, it returns always 0. I suggest you assume it is returning an error 
code, that will be safer for future changes in the framework (if any).

> Open question: can I call register_current_timer_delay,
> sched_clock_register, clocksource_register_hz in any order?
> ---

Yes, I think so. Thomas ?

[ ... ]

> +static void __init tango_clocksource_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);
> +	if (xtal_in_cnt == NULL)
> +		panic("%s: of_iomap failed\n", np->full_name);

   ^^^^^^^^^^^

That does not comply with the Linux kernel coding style.

	xtal_in_cnt = of_iomap(np, 0);
	if (!xtal_in_cnt) {
		pr_err("Argh!");
		return;
	}

	clk = of_clk_get(np, 0);
	if (!clk) {
		pr_err("grumf!");
		return;
	}

	freq = clk_get_rate(clk);


> +
> +	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);

ret = clocksource_register_hz(&tango_xtal, xtal_freq);
if (ret) {
	pr_err("oups!");
	return;
}



-- 
  <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 v2] clocksource/drivers/tango_xtal: Add new timer for Tango SoCs

[Marc Gonzalez]

On 07/10/2015 14:31, Daniel Lezcano wrote:
> On 10/07/2015 01:35 PM, Marc Gonzalez wrote:
>> Sigma Designs Tango platforms provide a 27 MHz crystal oscillator.
>> Use it for clocksource, sched_clock, and delay_timer.
>>
>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>> ---
>> AFAICS, clocksource_register_hz does not report failures via its
>> return value (always 0) but writes warnings to stdout?
> 
> Yeah, it returns always 0. I suggest you assume it is returning an error
> code, that will be safer for future changes in the framework (if any).

I suppose I'd also need to check the return value of of_clk_get?
(Looks like you mention it implicitly below.)

>> Open question: can I call register_current_timer_delay,
>> sched_clock_register, clocksource_register_hz in any order?
>> ---
> 
> Yes, I think so. Thomas ?
> 
> [ ... ]
> 
>> +static void __init tango_clocksource_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);
>> +	if (xtal_in_cnt == NULL)
>> +		panic("%s: of_iomap failed\n", np->full_name);
> 
>    ^^^^^^^^^^^
> 
> That does not comply with the Linux kernel coding style.

<confused>

scripts/checkpatch.pl only complains about a missing blank line after
the declaration block. (Sorry, I'll fix that.)

> 	xtal_in_cnt = of_iomap(np, 0);
> 	if (!xtal_in_cnt) {
> 		pr_err("Argh!");
> 		return;
> 	}

I know "!xtal_in_cnt" is equivalent to "xtal_in_cnt == NULL" but I'd
rather emphasize the fact that xtal_in_cnt is a pointer, not a bool.
(Documentation/CodingStyle does not mandate this particular idiom.)

I'm also confused that you've replaced panic() with pr_err/return.
AFAIU, if I don't have a clocksource/sched_clock, the system is dead
in the water. Might as well stop there, and wait for the operator to
fix whatever needs fixing. (Several clksrc drivers do this.)

> 	clk = of_clk_get(np, 0);
> 	if (!clk) {

AFAICT, checking for NULL is not good enough here.
of_clk_get returns ERR_PTR(rc) style errors.
Looks like I'd need "if (IS_ERR(clk))"

Thanks for the review.

Regards.

Re: [PATCH v2] clocksource/drivers/tango_xtal: Add new timer for Tango SoCs

[Daniel Lezcano]

On 10/07/2015 03:17 PM, Marc Gonzalez wrote:
> On 07/10/2015 14:31, Daniel Lezcano wrote:
>> On 10/07/2015 01:35 PM, Marc Gonzalez wrote:
>>> Sigma Designs Tango platforms provide a 27 MHz crystal oscillator.
>>> Use it for clocksource, sched_clock, and delay_timer.
>>>
>>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>>> ---
>>> AFAICS, clocksource_register_hz does not report failures via its
>>> return value (always 0) but writes warnings to stdout?
>>
>> Yeah, it returns always 0. I suggest you assume it is returning an error
>> code, that will be safer for future changes in the framework (if any).
>
> I suppose I'd also need to check the return value of of_clk_get?
> (Looks like you mention it implicitly below.)
>
>>> Open question: can I call register_current_timer_delay,
>>> sched_clock_register, clocksource_register_hz in any order?
>>> ---
>>
>> Yes, I think so. Thomas ?
>>
>> [ ... ]
>>
>>> +static void __init tango_clocksource_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);
>>> +	if (xtal_in_cnt == NULL)
>>> +		panic("%s: of_iomap failed\n", np->full_name);
>>
>>     ^^^^^^^^^^^
>>
>> That does not comply with the Linux kernel coding style.
>
> <confused>
>
> scripts/checkpatch.pl only complains about a missing blank line after
> the declaration block. (Sorry, I'll fix that.)
>
>> 	xtal_in_cnt = of_iomap(np, 0);
>> 	if (!xtal_in_cnt) {
>> 		pr_err("Argh!");
>> 		return;
>> 	}
>
> I know "!xtal_in_cnt" is equivalent to "xtal_in_cnt == NULL" but I'd
> rather emphasize the fact that xtal_in_cnt is a pointer, not a bool.
> (Documentation/CodingStyle does not mandate this particular idiom.)

Yes, if you prefer you can use != NULL.

> I'm also confused that you've replaced panic() with pr_err/return.
> AFAIU, if I don't have a clocksource/sched_clock, the system is dead
> in the water. Might as well stop there, and wait for the operator to
> fix whatever needs fixing. (Several clksrc drivers do this.)

Hmm, yeah that's true but also we have platforms with different 
clocksources, so we don't want to panic if the next clocksource will 
succeed. That's the logic behind not doing panic. There is some legacy 
code still using panic but that should be fixed.

I don't know if your platform can fall under this category, but it would 
be a good practice to pr_err or pr_warn instead of panic in order to be 
consistent with the current direction in the recent drivers.

>> 	clk = of_clk_get(np, 0);
>> 	if (!clk) {
>
> AFAICT, checking for NULL is not good enough here.
> of_clk_get returns ERR_PTR(rc) style errors.
> Looks like I'd need "if (IS_ERR(clk))"

Yep :)


   -- 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 v2] clocksource/drivers/tango_xtal: Add new timer for Tango SoCs

[Mason]

On 07/10/2015 18:03, Daniel Lezcano wrote:

> On 10/07/2015 03:17 PM, Marc Gonzalez wrote:
>
>> I'm also confused that you've replaced panic() with pr_err/return.
>> AFAIU, if I don't have a clocksource/sched_clock, the system is dead
>> in the water. Might as well stop there, and wait for the operator to
>> fix whatever needs fixing. (Several clksrc drivers do this.)
> 
> Hmm, yeah that's true but also we have platforms with different 
> clocksources, so we don't want to panic if the next clocksource will 
> succeed. That's the logic behind not doing panic. There is some legacy 
> code still using panic but that should be fixed.

There's so much legacy code lying around that it's really hard
to tell what the current best practices are :-(

> I don't know if your platform can fall under this category, but it would 
> be a good practice to pr_err or pr_warn instead of panic in order to be 
> consistent with the current direction in the recent drivers.

I think the system falls back to using the "jiffies clock" when
it doesn't find anything better? However, on my system, the
clockevent device is running at cpuclk/2, so once I add cpufreq
DFS into the mix, I don't think the "jiffies" clock is a very
good clocksource.

Anyway, updated patch is on the way.

Regards.

[PATCH v3] clocksource/drivers/tango_xtal: Add new timer for Tango SoCs

[Marc Gonzalez]

Sigma Designs Tango platforms provide a 27 MHz crystal oscillator.
Use it for clocksource, sched_clock, and delay_timer.

Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
---
I have a nagging feeling that the QUIT_IF macro will get this patch NAKed ;-)
My rationale: error-handling tends to take the focus away from the normal
path, and put it on the error path. Hiding the details away in a macro
helps to keep the error-handling noise to a minimum.
---
 drivers/clocksource/Kconfig      |  4 +++
 drivers/clocksource/Makefile     |  1 +
 drivers/clocksource/tango_xtal.c | 61 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 66 insertions(+)
 create mode 100644 drivers/clocksource/tango_xtal.c

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 4e57730e0be4..63c878d4d9e3 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -275,6 +275,10 @@ config CLKSRC_MIPS_GIC
 	depends on MIPS_GIC
 	select CLKSRC_OF
 
+config CLKSRC_TANGO_XTAL
+	bool
+	select CLKSRC_OF
+
 config CLKSRC_PXA
 	def_bool y if ARCH_PXA || ARCH_SA1100
 	select CLKSRC_OF if USE_OF
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index f228354961ca..160970a3963b 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -55,6 +55,7 @@ obj-$(CONFIG_ARCH_KEYSTONE)		+= timer-keystone.o
 obj-$(CONFIG_ARCH_INTEGRATOR_AP)	+= timer-integrator-ap.o
 obj-$(CONFIG_CLKSRC_VERSATILE)		+= versatile.o
 obj-$(CONFIG_CLKSRC_MIPS_GIC)		+= mips-gic-timer.o
+obj-$(CONFIG_CLKSRC_TANGO_XTAL)		+= tango_xtal.o
 obj-$(CONFIG_CLKSRC_IMX_GPT)		+= timer-imx-gpt.o
 obj-$(CONFIG_ASM9260_TIMER)		+= asm9260_timer.o
 obj-$(CONFIG_H8300)			+= h8300_timer8.o
diff --git a/drivers/clocksource/tango_xtal.c b/drivers/clocksource/tango_xtal.c
new file mode 100644
index 000000000000..a3c265eb7236
--- /dev/null
+++ b/drivers/clocksource/tango_xtal.c
@@ -0,0 +1,61 @@
+#include <linux/clocksource.h>
+#include <linux/sched_clock.h>
+#include <linux/of_address.h>
+#include <linux/delay.h>
+#include <linux/init.h>
+#include <linux/clk.h>
+
+#define QUIT_IF(cond, fmt, ...) do if (cond) {		\
+	static const char format[] __initconst = fmt;	\
+	pr_err(format, ## __VA_ARGS__);			\
+	return;						\
+} while (0)
+
+static void __iomem *xtal_in_cnt;
+static struct delay_timer delay_timer;
+
+static unsigned long notrace read_xtal_counter(void)
+{
+	return readl_relaxed(xtal_in_cnt);
+}
+
+static u64 notrace 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_clocksource_init(struct device_node *np)
+{
+	struct clk *clk;
+	int xtal_freq, err;
+
+	xtal_in_cnt = of_iomap(np, 0);
+	QUIT_IF(xtal_in_cnt == NULL, "%s: invalid address\n", np->full_name);
+
+	clk = of_clk_get(np, 0);
+	QUIT_IF(IS_ERR(clk), "%s: invalid clock\n", np->full_name);
+
+	xtal_freq = clk_get_rate(clk);
+	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);
+	err = clocksource_register_hz(&tango_xtal, xtal_freq);
+	QUIT_IF(err, "%s: registration failed\n", np->full_name);
+}
+
+CLOCKSOURCE_OF_DECLARE(tango, "sigma,tick-counter", tango_clocksource_init);
-- 
2.4.5

Re: [PATCH v3] clocksource/drivers/tango_xtal: Add new timer for Tango SoCs

[Daniel Lezcano]

On 10/09/2015 02:13 PM, Marc Gonzalez wrote:
> Sigma Designs Tango platforms provide a 27 MHz crystal oscillator.
> Use it for clocksource, sched_clock, and delay_timer.
>
> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> ---
> I have a nagging feeling that the QUIT_IF macro will get this patch NAKed ;-)
> My rationale: error-handling tends to take the focus away from the normal
> path, and put it on the error path. Hiding the details away in a macro
> helps to keep the error-handling noise to a minimum.

It is a right feeling :)

The Linux kernel code follows the same path all across the different 
sub-systems. So it is not a problem to write: if (err) ..., people is 
used to read such code and by introducing this macro, that makes the 
code less readable for them.

Moreover, the way you wrote the macro is strongly discouraged in the 
CodingStyle document because there is a 'return' inside.

   -- 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 v3] clocksource/drivers/tango_xtal: Add new timer for Tango SoCs

[Marc Gonzalez]

On 09/10/2015 15:24, Daniel Lezcano wrote:
> On 10/09/2015 02:13 PM, Marc Gonzalez wrote:
>> Sigma Designs Tango platforms provide a 27 MHz crystal oscillator.
>> Use it for clocksource, sched_clock, and delay_timer.
>>
>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>> ---
>> I have a nagging feeling that the QUIT_IF macro will get this patch NAKed ;-)
>> My rationale: error-handling tends to take the focus away from the normal
>> path, and put it on the error path. Hiding the details away in a macro
>> helps to keep the error-handling noise to a minimum.
> 
> It is a right feeling :)
> 
> The Linux kernel code follows the same path all across the different 
> sub-systems. So it is not a problem to write: if (err) ..., people is 
> used to read such code and by introducing this macro, that makes the 
> code less readable for them.
> 
> Moreover, the way you wrote the macro is strongly discouraged in the 
> CodingStyle document because there is a 'return' inside.

New patch coming right up.

On a tangential subject, it would seem that platforms with verbose logs
at init might benefit from marking as __initconst strings used in __init
functions.

I discussed this some time ago:
https://lkml.org/lkml/2015/3/25/688

which pointed to an earlier discussion:
https://lkml.org/lkml/2014/8/21/255
https://lkml.org/lkml/2014/6/22/149

Regards.

Re: [PATCH v3] clocksource/drivers/tango_xtal: Add new timer for Tango SoCs

[Daniel Lezcano]

On 10/09/2015 03:46 PM, Marc Gonzalez wrote:
> On 09/10/2015 15:24, Daniel Lezcano wrote:

[ ... ]

> On a tangential subject, it would seem that platforms with verbose logs
> at init might benefit from marking as __initconst strings used in __init
> functions.
>
> I discussed this some time ago:
> https://lkml.org/lkml/2015/3/25/688
>
> which pointed to an earlier discussion:
> https://lkml.org/lkml/2014/8/21/255
> https://lkml.org/lkml/2014/6/22/149

Thanks for the pointer.

Nicolas Pitre (Cc'ed) is working on optimizing the memory consumption by 
stripping unreferenced code at compile time. Perhaps he thought a 
solution for this particular use case.


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

[PATCH v4] clocksource/drivers/tango_xtal: Add new timer for Tango SoCs

[Marc Gonzalez]

Sigma Designs Tango platforms provide a 27 MHz crystal oscillator.
Use it for clocksource, sched_clock, and delay_timer.

Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
---
 drivers/clocksource/Kconfig      |  4 +++
 drivers/clocksource/Makefile     |  1 +
 drivers/clocksource/tango_xtal.c | 71 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 76 insertions(+)
 create mode 100644 drivers/clocksource/tango_xtal.c

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 4e57730e0be4..63c878d4d9e3 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -275,6 +275,10 @@ config CLKSRC_MIPS_GIC
 	depends on MIPS_GIC
 	select CLKSRC_OF
 
+config CLKSRC_TANGO_XTAL
+	bool
+	select CLKSRC_OF
+
 config CLKSRC_PXA
 	def_bool y if ARCH_PXA || ARCH_SA1100
 	select CLKSRC_OF if USE_OF
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index f228354961ca..160970a3963b 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -55,6 +55,7 @@ obj-$(CONFIG_ARCH_KEYSTONE)		+= timer-keystone.o
 obj-$(CONFIG_ARCH_INTEGRATOR_AP)	+= timer-integrator-ap.o
 obj-$(CONFIG_CLKSRC_VERSATILE)		+= versatile.o
 obj-$(CONFIG_CLKSRC_MIPS_GIC)		+= mips-gic-timer.o
+obj-$(CONFIG_CLKSRC_TANGO_XTAL)		+= tango_xtal.o
 obj-$(CONFIG_CLKSRC_IMX_GPT)		+= timer-imx-gpt.o
 obj-$(CONFIG_ASM9260_TIMER)		+= asm9260_timer.o
 obj-$(CONFIG_H8300)			+= h8300_timer8.o
diff --git a/drivers/clocksource/tango_xtal.c b/drivers/clocksource/tango_xtal.c
new file mode 100644
index 000000000000..da67bc2aec63
--- /dev/null
+++ b/drivers/clocksource/tango_xtal.c
@@ -0,0 +1,71 @@
+#include <linux/clocksource.h>
+#include <linux/sched_clock.h>
+#include <linux/of_address.h>
+#include <linux/printk.h>
+#include <linux/delay.h>
+#include <linux/init.h>
+#include <linux/clk.h>
+
+#define QUIT_IF(cond, fmt, ...) do if (cond) {		\
+	static const char format[] __initconst = fmt;	\
+	pr_err(format, ## __VA_ARGS__);			\
+	return;						\
+} while (0)
+
+static void __iomem *xtal_in_cnt;
+static struct delay_timer delay_timer;
+
+static unsigned long notrace read_xtal_counter(void)
+{
+	return readl_relaxed(xtal_in_cnt);
+}
+
+static u64 notrace 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_clocksource_init(struct device_node *np)
+{
+	struct clk *clk;
+	int xtal_freq, ret;
+
+	xtal_in_cnt = of_iomap(np, 0);
+	if (xtal_in_cnt == NULL) {
+		pr_err("%s: invalid address\n", np->full_name);
+		return;
+	}
+
+	clk = of_clk_get(np, 0);
+	if (IS_ERR(clk)) {
+		pr_err("%s: invalid clock\n", np->full_name);
+		return;
+	}
+
+	xtal_freq = clk_get_rate(clk);
+	delay_timer.freq = xtal_freq;
+	delay_timer.read_current_timer = read_xtal_counter;
+
+	ret = clocksource_register_hz(&tango_xtal, xtal_freq);
+	if (ret != 0) {
+		pr_err("%s: registration failed\n", np->full_name);
+		return;
+	}
+	sched_clock_register(read_sched_clock, 32, xtal_freq);
+	register_current_timer_delay(&delay_timer);
+}
+
+CLOCKSOURCE_OF_DECLARE(tango, "sigma,tick-counter", tango_clocksource_init);
-- 
2.4.5

Re: [PATCH v4] clocksource/drivers/tango_xtal: Add new timer for Tango SoCs

[Daniel Lezcano]

On 10/09/2015 04:37 PM, Marc Gonzalez wrote:
> Sigma Designs Tango platforms provide a 27 MHz crystal oscillator.
> Use it for clocksource, sched_clock, and delay_timer.
>
> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> ---

[ ... ]

> +
> +#define QUIT_IF(cond, fmt, ...) do if (cond) {		\
> +	static const char format[] __initconst = fmt;	\
> +	pr_err(format, ## __VA_ARGS__);			\
> +	return;						\
> +} while (0)

hmm ... forget something ?

> +
> +	ret = clocksource_register_hz(&tango_xtal, xtal_freq);
> +	if (ret != 0) {
> +		pr_err("%s: registration failed\n", np->full_name);
> +		return;
> +	}

missing line.

> +	sched_clock_register(read_sched_clock, 32, xtal_freq);
> +	register_current_timer_delay(&delay_timer);
> +}
> +
> +CLOCKSOURCE_OF_DECLARE(tango, "sigma,tick-counter", tango_clocksource_init);
>


-- 
  <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 v4] clocksource/drivers/tango_xtal: Add new timer for Tango SoCs

[Måns Rullgård]

Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes:

> Sigma Designs Tango platforms provide a 27 MHz crystal oscillator.
> Use it for clocksource, sched_clock, and delay_timer.

Given the nature of this hardware, I think it would make much more sense
to support it in a generic fashion.  Otherwise the next chip that comes
along with a similar counter will result in near duplicate of this
"driver", and so on.  I've suggested this before, and I even sent
patches for it (currently under discussion), but you keep refusing to
listen.  Are you that desperate to see your name on a commit?  The fact
that you keep rewriting, poorly, code you know I've already made
available suggests this might be the case.  You even admit in private
that you couldn't have done this without looking at my tango3 tree.
Frankly, I find your behaviour shameful.

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

[PATCH v5] clocksource/drivers/tango_xtal: Add new timer for Tango SoCs

[Marc Gonzalez]

Sigma Designs Tango platforms provide a 27 MHz crystal oscillator.
Use it for clocksource, sched_clock, and delay_timer.

Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
---
 drivers/clocksource/Kconfig      |  4 +++
 drivers/clocksource/Makefile     |  1 +
 drivers/clocksource/tango_xtal.c | 66 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 71 insertions(+)
 create mode 100644 drivers/clocksource/tango_xtal.c

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 4e57730e0be4..63c878d4d9e3 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -275,6 +275,10 @@ config CLKSRC_MIPS_GIC
 	depends on MIPS_GIC
 	select CLKSRC_OF
 
+config CLKSRC_TANGO_XTAL
+	bool
+	select CLKSRC_OF
+
 config CLKSRC_PXA
 	def_bool y if ARCH_PXA || ARCH_SA1100
 	select CLKSRC_OF if USE_OF
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index f228354961ca..160970a3963b 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -55,6 +55,7 @@ obj-$(CONFIG_ARCH_KEYSTONE)		+= timer-keystone.o
 obj-$(CONFIG_ARCH_INTEGRATOR_AP)	+= timer-integrator-ap.o
 obj-$(CONFIG_CLKSRC_VERSATILE)		+= versatile.o
 obj-$(CONFIG_CLKSRC_MIPS_GIC)		+= mips-gic-timer.o
+obj-$(CONFIG_CLKSRC_TANGO_XTAL)		+= tango_xtal.o
 obj-$(CONFIG_CLKSRC_IMX_GPT)		+= timer-imx-gpt.o
 obj-$(CONFIG_ASM9260_TIMER)		+= asm9260_timer.o
 obj-$(CONFIG_H8300)			+= h8300_timer8.o
diff --git a/drivers/clocksource/tango_xtal.c b/drivers/clocksource/tango_xtal.c
new file mode 100644
index 000000000000..d297b30d2bc0
--- /dev/null
+++ b/drivers/clocksource/tango_xtal.c
@@ -0,0 +1,66 @@
+#include <linux/clocksource.h>
+#include <linux/sched_clock.h>
+#include <linux/of_address.h>
+#include <linux/printk.h>
+#include <linux/delay.h>
+#include <linux/init.h>
+#include <linux/clk.h>
+
+static void __iomem *xtal_in_cnt;
+static struct delay_timer delay_timer;
+
+static unsigned long notrace read_xtal_counter(void)
+{
+	return readl_relaxed(xtal_in_cnt);
+}
+
+static u64 notrace 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_clocksource_init(struct device_node *np)
+{
+	struct clk *clk;
+	int xtal_freq, ret;
+
+	xtal_in_cnt = of_iomap(np, 0);
+	if (xtal_in_cnt == NULL) {
+		pr_err("%s: invalid address\n", np->full_name);
+		return;
+	}
+
+	clk = of_clk_get(np, 0);
+	if (IS_ERR(clk)) {
+		pr_err("%s: invalid clock\n", np->full_name);
+		return;
+	}
+
+	xtal_freq = clk_get_rate(clk);
+	delay_timer.freq = xtal_freq;
+	delay_timer.read_current_timer = read_xtal_counter;
+
+	ret = clocksource_register_hz(&tango_xtal, xtal_freq);
+	if (ret != 0) {
+		pr_err("%s: registration failed\n", np->full_name);
+		return;
+	}
+
+	sched_clock_register(read_sched_clock, 32, xtal_freq);
+	register_current_timer_delay(&delay_timer);
+}
+
+CLOCKSOURCE_OF_DECLARE(tango, "sigma,tick-counter", tango_clocksource_init);
-- 
2.4.5

Re: [PATCH v4] clocksource/drivers/tango_xtal: Add new timer for Tango SoCs

[Marc Gonzalez]

Måns Rullgård wrote:

> Marc Gonzalez wrote:
> 
>> Sigma Designs Tango platforms provide a 27 MHz crystal oscillator.
>> Use it for clocksource, sched_clock, and delay_timer.
> 
> Given the nature of this hardware, I think it would make much more sense
> to support it in a generic fashion.  Otherwise the next chip that comes
> along with a similar counter will result in near duplicate of this
> "driver", and so on.

I didn't /want/ to write this driver, or rather this "driver" as you put it
(implying that it is so trivial that I am lame even to submit it). It was
living happily in arch/arm/clock-tango.c, but Arnd pointed out that such
code must migrate to drivers/clocksource.

I find your claim that this minimal device (a single register really) should
be supported in a generic fashion questionable. No one seems to have ever
needed this, yet it has suddenly become urgent to have it right now?

I would probably have used your driver had it been mainlined; but it is not,
and Rob and Mark didn't seem convinced AFAICT...
(Also note that your driver doesn't set up the delay timer, which I want.)

I'm sorry if my mainlining effort is not compatible with your schedule, but
I've been working on this port for 6 months, and I can't wait a few more
weeks just because you're not quite ready. (Have you mainstreamed the eth
and intc driver? I would actually need those.)

> I've suggested this before, and I even sent
> patches for it (currently under discussion),

Yes, and you carefully omitted to CC me, despite my request that you do so.
Thanks for that.

> but you keep refusing to listen.  Are you that desperate to see your
> name on a commit?  The fact
> that you keep rewriting, poorly, code you know I've already made
> available suggests this might be the case.  You even admit in private
> that you couldn't have done this without looking at my tango3 tree.
> Frankly, I find your behaviour shameful.

Don't twist my words. I said I couldn't have written the eth and intc driver
(and relevant DT setup).

Are you now simultaneously claiming that

1) my driver is trivial
2) I couldn't have written it without your help

implying that I cannot code even trivial drivers?

EOT

Re: [PATCH v4] clocksource/drivers/tango_xtal: Add new timer for Tango SoCs

[Måns Rullgård]

Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes:

> Måns Rullgård wrote:
>
>> Marc Gonzalez wrote:
>> 
>>> Sigma Designs Tango platforms provide a 27 MHz crystal oscillator.
>>> Use it for clocksource, sched_clock, and delay_timer.
>> 
>> Given the nature of this hardware, I think it would make much more sense
>> to support it in a generic fashion.  Otherwise the next chip that comes
>> along with a similar counter will result in near duplicate of this
>> "driver", and so on.
>
> I didn't /want/ to write this driver, or rather this "driver" as you put it
> (implying that it is so trivial that I am lame even to submit it). It was
> living happily in arch/arm/clock-tango.c, but Arnd pointed out that such
> code must migrate to drivers/clocksource.
>
> I find your claim that this minimal device (a single register really) should
> be supported in a generic fashion questionable. No one seems to have ever
> needed this, yet it has suddenly become urgent to have it right now?

Apparently, nobody has needed it before (I can't say for sure that no
existing drivers could be simplified).  That doesn't mean that nobody
will need it again.  When something can be supported in a generic way,
it is usually a good idea to do that.  It saves work in the long term.

> I'm sorry if my mainlining effort is not compatible with your schedule, but
> I've been working on this port for 6 months, and I can't wait a few more
> weeks just because you're not quite ready. (Have you mainstreamed the eth
> and intc driver? I would actually need those.)

You (Sigma) had the chance to contract me to help out with this work and
strongly indicated ("we will send you a contract for review by the end
of the week") that you would.  Then you backtracked in a rather ugly
manner.  Forgive me if I'm less than motivated to suddenly spend hours
of unpaid time helping you get things in shape.  If you don't want to
pay me, you also don't get to set my schedule.  Besides, you've never
shown any interest whatsoever in upstreaming before, so I really don't
see why it is so urgent for you now.

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

Re: [PATCH v4] clocksource/drivers/tango_xtal: Add new timer for Tango SoCs

[Marc Gonzalez]

On 09/10/2015 18:01, Måns Rullgård wrote:

> Marc Gonzalez wrote:
> 
>> Måns Rullgård wrote:
>>
>>> Marc Gonzalez wrote:
>>>
>>>> Sigma Designs Tango platforms provide a 27 MHz crystal oscillator.
>>>> Use it for clocksource, sched_clock, and delay_timer.
>>>
>>> Given the nature of this hardware, I think it would make much more sense
>>> to support it in a generic fashion.  Otherwise the next chip that comes
>>> along with a similar counter will result in near duplicate of this
>>> "driver", and so on.
>>
>> I didn't /want/ to write this driver, or rather this "driver" as you put it
>> (implying that it is so trivial that I am lame even to submit it). It was
>> living happily in arch/arm/clock-tango.c, but Arnd pointed out that such
>> code must migrate to drivers/clocksource.
>>
>> I find your claim that this minimal device (a single register really) should
>> be supported in a generic fashion questionable. No one seems to have ever
>> needed this, yet it has suddenly become urgent to have it right now?
> 
> Apparently, nobody has needed it before (I can't say for sure that no
> existing drivers could be simplified).  That doesn't mean that nobody
> will need it again.  When something can be supported in a generic way,
> it is usually a good idea to do that.  It saves work in the long term.

You snipped this:

I would probably have used your driver had it been mainlined.
(Also note that your driver doesn't set up the delay timer, which I want.)

>> I'm sorry if my mainlining effort is not compatible with your schedule, but
>> I've been working on this port for 6 months, and I can't wait a few more
>> weeks just because you're not quite ready. (Have you mainstreamed the eth
>> and intc driver? I would actually need those.)
> 
> You (Sigma)

I am not Sigma, I am Marc. I am a software dev grunt, and I have
no influence on recruitment process, business decisions, etc.

> had the chance to contract me to help out with this work and
> strongly indicated ("we will send you a contract for review by the end
> of the week") that you would.  Then you backtracked in a rather ugly
> manner.

I am sorry that you were treated like that :-(

> Forgive me if I'm less than motivated to suddenly spend hours
> of unpaid time helping you get things in shape.

<confused> I am just asking that you stop popping up on MLs NAKing
each one of my patches.

> If you don't want to pay me, you also don't get to set my schedule.

<confused> Are you referring to the DT changes I've asked you to make?

> Besides, you've never
> shown any interest whatsoever in upstreaming before, so I really don't
> see why it is so urgent for you now.

Trying to turn the tables, nice :-)

It is urgent because I have been working full-time on this for two months,
and I'd like to have results to show for it. (Management is not convinced
that upstreaming is a good idea.)

If you take a step back, you'll see that I have accepted your input and
criticism every time I thought it was warranted, e.g. everything UART
related. (And why would I not? It's nice to benefit from great work.)
But your claims regarding the clock tree are inaccurate. (I'll address
them in a separate message.)

Regards.

Re: [PATCH v3] clocksource/drivers/tango_xtal: Add new timer for Tango SoCs

[Nicolas Pitre]

On Fri, 9 Oct 2015, Daniel Lezcano wrote:

> On 10/09/2015 03:46 PM, Marc Gonzalez wrote:
> > On 09/10/2015 15:24, Daniel Lezcano wrote:
> 
> [ ... ]
> 
> > On a tangential subject, it would seem that platforms with verbose logs
> > at init might benefit from marking as __initconst strings used in __init
> > functions.
> > 
> > I discussed this some time ago:
> > https://lkml.org/lkml/2015/3/25/688
> > 
> > which pointed to an earlier discussion:
> > https://lkml.org/lkml/2014/8/21/255
> > https://lkml.org/lkml/2014/6/22/149
> 
> Thanks for the pointer.
> 
> Nicolas Pitre (Cc'ed) is working on optimizing the memory consumption by
> stripping unreferenced code at compile time. Perhaps he thought a solution for
> this particular use case.

This is different.  Here those strings are actually referenced and can't 
be discarded.  The solution is to mark them so they go in the 
appropriate section as suggested.

It might be worth investigating the addition of a new gcc function 
attribute that provides a replacement section for .data, .rodata, etc. 
in addition to .text, that would apply to data objects created within 
that function.


Nicolas

Re: [PATCH v5] clocksource/drivers/tango_xtal: Add new timer for Tango SoCs

[Daniel Lezcano]

On 10/09/2015 04:59 PM, Marc Gonzalez wrote:
> Sigma Designs Tango platforms provide a 27 MHz crystal oscillator.
> Use it for clocksource, sched_clock, and delay_timer.
>
> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> ---

Applied to my tree for 4.4

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