clocksource: Sigma Designs Tango 27 MHz xtal
PATCH 1/1 clocksource: Sigma Designs Tango 27 MHz xtal
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.
--- drivers/clocksource/Makefile | 1 + drivers/clocksource/tango_xtal.c | 46 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+) create mode 100644 drivers/clocksource/tango_xtal.c
drivers/clocksource/Makefile +1 -0
@@ -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
drivers/clocksource/tango_xtal.c new file +46 -0
46 lines not on this page · raw message
Signature · 1 line
2.4.5
Hi Marc,
Quoted 2 lines · Marc Gonzalez
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 ...
Quoted 15 lines
Sigma Designs Tango platforms provide a 27 MHz crystal oscillator. Use it for clocksource,
sched_clock, anddelay_timer.Signed-off-by Marc Gonzalez <[email protected]>--- drivers/clocksource/Makefile | 1 + drivers/clocksource/tango_xtal.c | 46 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+)create mode 100644
drivers/clocksource/tango_xtal.cdrivers/clocksource/Makefile +0
-0
drivers/clocksource/Makefile obj-$(CONFIG_ARCH_CLPS711X) += clps711x-timer.o · line 27
Quoted 4 lines
drivers/clocksource/Makefile +1
-0@@ -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
Quoted 9 lines
drivers/clocksource/Makefile +0
-0obj-$(CONFIG_CLKSRC_PXA) += pxa_timer.o obj-$(CONFIG_ARCH_PRIMA2) += timer-prima2.o obj-$(CONFIG_ARCH_U300) += timer-u300.odrivers/clocksource/tango_xtal.c new file +0
-0
drivers/clocksource/tango_xtal.c line 5
Quoted 5 lines
drivers/clocksource/tango_xtal.c new file +5
-0@@ -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.
drivers/clocksource/tango_xtal.c line 18
Quoted 13 lines
drivers/clocksource/tango_xtal.c new file +13
-0+ +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)
Quoted 8 lines
drivers/clocksource/tango_xtal.c new file +8
-0+static cycle_t read_clocksource (struct clocksource *cs) +{ +return read_xtal_counter (); +} + +static struct clocksource tango_xtal = { + .name ="tango-xtal" , + .rating =350 ,
drivers/clocksource/tango_xtal.c line 37
Quoted 10 lines
drivers/clocksource/tango_xtal.c new file +10
-0+ .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.
drivers/clocksource/tango_xtal.c line 43
Quoted 5 lines
drivers/clocksource/tango_xtal.c new file +5
-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);
check return code.
Quoted 4 lines
drivers/clocksource/tango_xtal.c new file +3
-0+} + +CLOCKSOURCE_OF_DECLARE (tango,"sigma,tick-counter" , tango_clocksource_init);
Signature · 5 lines
<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
Hello everyone,
Quoted 5 lines · Daniel Lezcano
Quoted 2 lines · Marc Gonzalez
Date: Tue, 6 Oct 2015 16:49:28 +0200
Subject: [PATCH] clocksource: Sigma Designs Tango 27 MHz xtalFix the patch format.
OK.
Quoted 1 line
Subject is clocksource/drivers/tango_xtal: Add new timer ...
OK.
(84583983c319 didn't seem to follow that convention)
Quoted 24 lines
Quoted 15 lines
Sigma Designs Tango platforms provide a 27 MHz crystal oscillator. Use it for clocksource,
sched_clock, anddelay_timer.Signed-off-by Marc Gonzalez <[email protected]>--- drivers/clocksource/Makefile | 1 + drivers/clocksource/tango_xtal.c | 46 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+)create mode 100644
drivers/clocksource/tango_xtal.cdrivers/clocksource/Makefile +0
-0drivers/clocksource/Makefile obj-$(CONFIG_ARCH_CLPS711X) += clps711x-timer.o · line 27
Quoted 4 lines
drivers/clocksource/Makefile +1
-0@@ -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.oPlease use the following scheme:
Add in the clocksource's
Kconfigan entry:
Quoted 7 lines
config
CLKSRC_TANGO_XTAL
bool "bla bla" ifCOMPILE_TEST
selectCLKSRC_OFand then in the arch's
Kconfig:select
CLKSRC_TANGO_XTAL
OK.
Quoted 8 lines
Quoted 8 lines
drivers/clocksource/Makefile +0
-0obj-$(CONFIG_CLKSRC_PXA) += pxa_timer.o obj-$(CONFIG_ARCH_PRIMA2) += timer-prima2.o obj-$(CONFIG_ARCH_U300) += timer-u300.odrivers/clocksource/tango_xtal.c new file +0
-0
Quoted 8 lines
drivers/clocksource/tango_xtal.c line 5
Quoted 6 lines
drivers/clocksource/tango_xtal.c new file +5
-0@@ -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?
Quoted 6 lines
Quoted 6 lines
drivers/clocksource/tango_xtal.c new file +6
-0+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);
Quoted 8 lines
drivers/clocksource/tango_xtal.c line 18
Quoted 6 lines
drivers/clocksource/tango_xtal.c new file +6
-0+} + +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?
Quoted 12 lines
Quoted 12 lines
drivers/clocksource/tango_xtal.c new file +12
-0+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 , +};
Quoted 8 lines
drivers/clocksource/tango_xtal.c line 37
Quoted 6 lines
drivers/clocksource/tango_xtal.c new file +6
-0+ +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.
Quoted 7 lines
drivers/clocksource/tango_xtal.c line 43
Quoted 5 lines
drivers/clocksource/tango_xtal.c new file +5
-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);check return code.
OK.
New patch coming up.
Regards.
On 10/07/2015 10:23 AM, Marc Gonzalez wrote:
[ ... ]
Quoted 10 lines
Quoted 9 lines
drivers/clocksource/tango_xtal.c line 5
Quoted 7 lines
+++
b/drivers/clocksource/tango_xtal.cdrivers/clocksource/tango_xtal.c new file +5
-0@@ -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.
Quoted 3 lines
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 ...
Quoted 15 lines
Quoted 6 lines
Quoted 6 lines
drivers/clocksource/tango_xtal.c new file +6
-0+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);Quoted 8 lines
drivers/clocksource/tango_xtal.c line 18
Quoted 6 lines
drivers/clocksource/tango_xtal.c new file +6
-0+} + +static u64 read_sched_clock (void ) +{ +return read_xtal_counter (); +}static u64 *notrace*
read_sched_clock(void)
Quoted 1 line
What about
read_clocksource? andread_xtal_counter?
See commit 89e6a13b88.
-- Daniel
Signature · 5 lines
<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
Quoted 8 lines · Daniel Lezcano
Quoted 5 lines · Marc Gonzalez
Quoted 1 line · Daniel Lezcano
static u64 *notrace*
read_sched_clock(void)What about
read_clocksource? andread_xtal_counter?See commit
89e6a13b88.
If I understand correctly, since read_sched_clock calls read_xtal_counter, both functions should be marked notrace.
Regards.
Quoted 4 lines · Marc Gonzalez
Quoted 3 lines · Daniel Lezcano
Quoted 2 lines · Marc Gonzalez
Daniel Lezcano wrote:
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
static u64 *notrace*
read_sched_clock(void)What about
read_clocksource? andread_xtal_counter?See commit
89e6a13b88.If I understand correctly, since
read_sched_clockcallsread_xtal_counter, both functions should be marked notrace.
Yes. We have to prevent the recursion with the ftrace timestamp.
Signature · 5 lines
<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 v2 1/1 clocksource/drivers/tango_xtal: Add new timer for Tango SoCs
In reply to Marc GonzalezSigma Designs Tango platforms provide a 27 MHz crystal oscillator. Use it for clocksource, sched_clock, and delay_timer.
---
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
drivers/clocksource/Kconfig +4 -0
@@ -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
drivers/clocksource/Makefile +1 -0
@@ -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
drivers/clocksource/tango_xtal.c new file +49 -0
49 lines not on this page · raw message
Signature · 1 line
2.4.5
Re: [PATCH v2] clocksource/drivers/tango_xtal: Add new timer for Tango SoCs
Quoted 7 lines · Marc Gonzalez
Sigma Designs Tango platforms provide a 27 MHz crystal oscillator. Use it for clocksource,
sched_clock, anddelay_timer.Signed-off-by Marc Gonzalez <[email protected]>---
AFAICS,clocksource_register_hzdoes 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).
Quoted 3 lines
Open question: can I call
register_current_timer_delay,sched_clock_register,clocksource_register_hzin any order?
---
Yes, I think so. Thomas ?
[ ... ]
drivers/clocksource/tango_xtal.c line 40
Quoted 7 lines
drivers/clocksource/tango_xtal.c new file +7
-0+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);drivers/clocksource/tango_xtal.c line 46
Quoted 6 lines
drivers/clocksource/tango_xtal.c new file +6
-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);
ret = clocksource_register_hz(&tango_xtal, xtal_freq);
if (ret) {
pr_err("oups!");
return;
}Signature · 5 lines
<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
Quoted 11 lines · Daniel Lezcano
Quoted 7 lines · Marc Gonzalez
Sigma Designs Tango platforms provide a 27 MHz crystal oscillator. Use it for clocksource,
sched_clock, anddelay_timer.Signed-off-by Marc Gonzalez <[email protected]>---
AFAICS,clocksource_register_hzdoes 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.)
Quoted 18 lines
Quoted 3 lines
Open question: can I call
register_current_timer_delay,sched_clock_register,clocksource_register_hzin any order?
---Yes, I think so. Thomas ?
[ ... ]
drivers/clocksource/tango_xtal.c line 40
Quoted 7 lines
drivers/clocksource/tango_xtal.c new file +7
-0+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);^^^^^^^^^^^
Quoted 1 line
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.)
Quoted 5 lines
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.)
Quoted 2 lines
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
Quoted 47 lines · Marc Gonzalez
Quoted 11 lines · Daniel Lezcano
Quoted 7 lines · Marc Gonzalez
Sigma Designs Tango platforms provide a 27 MHz crystal oscillator. Use it for clocksource,
sched_clock, anddelay_timer.Signed-off-by Marc Gonzalez <[email protected]>---
AFAICS,clocksource_register_hzdoes 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.)Quoted 18 lines
Quoted 3 lines
Open question: can I call
register_current_timer_delay,sched_clock_register,clocksource_register_hzin any order?
---Yes, I think so. Thomas ?
[ ... ]
drivers/clocksource/tango_xtal.c line 40
Quoted 7 lines
drivers/clocksource/tango_xtal.c new file +7
-0+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);^^^^^^^^^^^Quoted 1 line
That does not comply with the Linux kernel coding style.
<confused>
scripts/checkpatch.plonly complains about a missing blank line after the declaration block. (Sorry, I'll fix that.)Quoted 5 lines
xtal_in_cnt = of_iomap(np, 0); if (!xtal_in_cnt) { pr_err("Argh!"); return; }
Quoted 3 lines
I know "!xtal_in_cnt" is equivalent to "
xtal_in_cnt== NULL" but I'd rather emphasize the fact thatxtal_in_cntis a pointer, not a bool. (Documentation/CodingStyledoes not mandate this particular idiom.)
Yes, if you prefer you can use != NULL.
Quoted 4 lines
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.
Quoted 6 lines
Quoted 2 lines
clk = of_clk_get(np, 0); if (!clk) {AFAICT, checking for NULL is not good enough here.
of_clk_getreturnsERR_PTR(rc)style errors. Looks like I'd need "if (IS_ERR(clk))"
Yep :)
-- Daniel
Signature · 5 lines
<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
Quoted 11 lines · Daniel Lezcano
Quoted 4 lines · Marc Gonzalez
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 :-(
Quoted 3 lines
I don't know if your platform can fall under this category, but it would be a good practice to
pr_errorpr_warninstead 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 1/1 clocksource/drivers/tango_xtal: Add new timer for Tango SoCs
Sigma Designs Tango platforms provide a 27 MHz crystal oscillator. Use it for clocksource, sched_clock, and delay_timer.
---
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
drivers/clocksource/Kconfig +4 -0
@@ -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
drivers/clocksource/Makefile +1 -0
@@ -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
drivers/clocksource/tango_xtal.c new file +61 -0
61 lines not on this page · raw message
Signature · 1 line
2.4.5
Re: [PATCH v3] clocksource/drivers/tango_xtal: Add new timer for Tango SoCs
Quoted 9 lines · Marc Gonzalez
Sigma Designs Tango platforms provide a 27 MHz crystal oscillator. Use it for clocksource,
sched_clock, anddelay_timer.Signed-off-by Marc Gonzalez <[email protected]>---
I have a nagging feeling that theQUIT_IFmacro 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
Signature · 5 lines
<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
Quoted 13 lines · Daniel Lezcano
Quoted 9 lines · Marc Gonzalez
Sigma Designs Tango platforms provide a 27 MHz crystal oscillator. Use it for clocksource,
sched_clock, anddelay_timer.Signed-off-by Marc Gonzalez <[email protected]>---
I have a nagging feeling that theQUIT_IFmacro 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 :)
Quoted 7 lines
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
Quoted 1 line · Marc Gonzalez
On 09/10/2015 15:24, Daniel Lezcano wrote:
[ ... ]
Quoted 10 lines
On a tangential subject, it would seem that platforms with verbose logs at init might benefit from marking as
__initconststrings used in__initfunctions.I discussed this some time ago:
https://lkml.org/lkml/2015/3/25/688which 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
Signature · 5 lines
<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
Quoted 16 lines · Daniel Lezcano
Quoted 1 line · Marc Gonzalez
On 09/10/2015 15:24, Daniel Lezcano wrote:
[ ... ]
Quoted 10 lines
On a tangential subject, it would seem that platforms with verbose logs at init might benefit from marking as
__initconststrings used in__initfunctions.I discussed this some time ago:
https://lkml.org/lkml/2015/3/25/688which pointed to an earlier discussion:
https://lkml.org/lkml/2014/8/21/255
https://lkml.org/lkml/2014/6/22/149
Quoted 5 lines
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
PATCH v4 1/1 clocksource/drivers/tango_xtal: Add new timer for Tango SoCs
In reply to Daniel LezcanoSigma Designs Tango platforms provide a 27 MHz crystal oscillator. Use it for clocksource, sched_clock, and delay_timer.
--- 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
drivers/clocksource/Kconfig +4 -0
@@ -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
drivers/clocksource/Makefile +1 -0
@@ -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
drivers/clocksource/tango_xtal.c new file +71 -0
71 lines not on this page · raw message
Signature · 1 line
2.4.5
Re: [PATCH v4] clocksource/drivers/tango_xtal: Add new timer for Tango SoCs
Quoted 5 lines · Marc Gonzalez
Sigma Designs Tango platforms provide a 27 MHz crystal oscillator. Use it for clocksource,
sched_clock, anddelay_timer.Signed-off-by Marc Gonzalez <[email protected]>---
[ ... ]
drivers/clocksource/tango_xtal.c line 13
Quoted 6 lines
drivers/clocksource/tango_xtal.c new file +6
-0+ +#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 ?
drivers/clocksource/tango_xtal.c line 66
Quoted 6 lines
drivers/clocksource/tango_xtal.c new file +6
-0+ + ret =clocksource_register_hz (&tango_xtal, xtal_freq); +if (ret !=0 ) { +pr_err ("%s: registration failed\n" , np->full_name); +return ; + }
missing line.
Quoted 6 lines
drivers/clocksource/tango_xtal.c new file +5
-0+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);
Signature · 5 lines
<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 v5 1/1 clocksource/drivers/tango_xtal: Add new timer for Tango SoCs
Sigma Designs Tango platforms provide a 27 MHz crystal oscillator. Use it for clocksource, sched_clock, and delay_timer.
--- 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
drivers/clocksource/Kconfig +4 -0
@@ -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
drivers/clocksource/Makefile +1 -0
@@ -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
drivers/clocksource/tango_xtal.c new file +66 -0
66 lines not on this page · raw message
Signature · 1 line
2.4.5
Daniel Lezcano
AppliedRe: [PATCH v5] clocksource/drivers/tango_xtal: Add new timer for Tango SoCs
Quoted 5 lines · Marc Gonzalez
Sigma Designs Tango platforms provide a 27 MHz crystal oscillator. Use it for clocksource,
sched_clock, anddelay_timer.Signed-off-by Marc Gonzalez <[email protected]>---
Applied to my tree for 4.4
Thanks !
-- Daniel
Signature · 5 lines
<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
In reply to Marc GonzalezQuoted 2 lines · Marc Gonzalez
Sigma Designs Tango platforms provide a 27 MHz crystal oscillator. Use it for clocksource,
sched_clock, anddelay_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.
Signature · 2 lines
Måns Rullgård [email protected]
Re: [PATCH v4] clocksource/drivers/tango_xtal: Add new timer for Tango SoCs
Quoted 9 lines · Måns Rullgård
Quoted 2 lines · Marc Gonzalez
Sigma Designs Tango platforms provide a 27 MHz crystal oscillator. Use it for clocksource,
sched_clock, anddelay_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.)
Quoted 2 lines
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.
Quoted 6 lines
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
Quoted 12 lines · Marc Gonzalez
Quoted 9 lines · Måns Rullgård
Quoted 2 lines · Marc Gonzalez
Sigma Designs Tango platforms provide a 27 MHz crystal oscillator. Use it for clocksource,
sched_clock, anddelay_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.
Quoted 8 lines
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 todrivers/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.
Quoted 4 lines
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.
Signature · 2 lines
Måns Rullgård [email protected]
Re: [PATCH v4] clocksource/drivers/tango_xtal: Add new timer for Tango SoCs
Quoted 19 lines · Måns Rullgård
Quoted 17 lines · Marc Gonzalez
Quoted 9 lines · Måns Rullgård
Quoted 2 lines · Marc Gonzalez
Sigma Designs Tango platforms provide a 27 MHz crystal oscillator. Use it for clocksource,
sched_clock, anddelay_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 todrivers/clocksource.
Quoted 8 lines
Quoted 3 lines
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.)
Quoted 6 lines
Quoted 4 lines
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.
Quoted 4 lines
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 :-(
Quoted 2 lines
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.
Quoted 1 line
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?
Quoted 3 lines
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.