| Message ID | 1384201236-8689-1-git-send-email-u.kleine-koenig@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
On 11/11/2013 09:20 PM, Uwe Kleine-König wrote: > As CSR SiRF is converted to multi platform CLOCK_TICK_RATE is a dummy > value that seems to match the right value is used. > (arch/arm/mach-prima2/include/mach/timex.h which defined CLOCK_TICK_RATE > to 1000000 was removed in commit cf82e0e (ARM: sirf: enable > multiplatform support); marco used the same file.) > > To not depend on that dummy value use a local #define instead. I don't get this patch. It is to fix a compilation error ? > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > drivers/clocksource/timer-marco.c | 13 +++++++------ > drivers/clocksource/timer-prima2.c | 14 ++++++++------ > 2 files changed, 15 insertions(+), 12 deletions(-) > > diff --git a/drivers/clocksource/timer-marco.c b/drivers/clocksource/timer-marco.c > index 09a17d9a..0d367a7 100644 > --- a/drivers/clocksource/timer-marco.c > +++ b/drivers/clocksource/timer-marco.c > @@ -19,7 +19,8 @@ > #include <linux/of_irq.h> > #include <linux/of_address.h> > #include <linux/sched_clock.h> > -#include <asm/mach/time.h> Why do you remove this header ? It is related to CLOCK_TICK_RATE ? > + > +#define CLOCK_FREQ 1000000 Why don't you include <linux/timex.h> where <asm/timex.h> is pulled with the CLOCK_TICK_RATE definition for the multiplatform ? > #define SIRFSOC_TIMER_32COUNTER_0_CTRL 0x0000 > #define SIRFSOC_TIMER_32COUNTER_1_CTRL 0x0004 > @@ -191,7 +192,7 @@ static int sirfsoc_local_timer_setup(struct clock_event_device *ce) > ce->rating = 200; > ce->set_mode = sirfsoc_timer_set_mode; > ce->set_next_event = sirfsoc_timer_set_next_event; > - clockevents_calc_mult_shift(ce, CLOCK_TICK_RATE, 60); > + clockevents_calc_mult_shift(ce, CLOCK_FREQ, 60); > ce->max_delta_ns = clockevent_delta2ns(-2, ce); > ce->min_delta_ns = clockevent_delta2ns(2, ce); > ce->cpumask = cpumask_of(cpu); > @@ -263,11 +264,11 @@ static void __init sirfsoc_marco_timer_init(void) > BUG_ON(IS_ERR(clk)); > rate = clk_get_rate(clk); > > - BUG_ON(rate < CLOCK_TICK_RATE); > - BUG_ON(rate % CLOCK_TICK_RATE); > + BUG_ON(rate < CLOCK_FREQ); > + BUG_ON(rate % CLOCK_FREQ); > > /* Initialize the timer dividers */ > - timer_div = rate / CLOCK_TICK_RATE - 1; > + timer_div = rate / CLOCK_FREQ - 1; > writel_relaxed(timer_div << 16, sirfsoc_timer_base + SIRFSOC_TIMER_64COUNTER_CTRL); > writel_relaxed(timer_div << 16, sirfsoc_timer_base + SIRFSOC_TIMER_32COUNTER_0_CTRL); > writel_relaxed(timer_div << 16, sirfsoc_timer_base + SIRFSOC_TIMER_32COUNTER_1_CTRL); > @@ -283,7 +284,7 @@ static void __init sirfsoc_marco_timer_init(void) > /* Clear all interrupts */ > writel_relaxed(0xFFFF, sirfsoc_timer_base + SIRFSOC_TIMER_INTR_STATUS); > > - BUG_ON(clocksource_register_hz(&sirfsoc_clocksource, CLOCK_TICK_RATE)); > + BUG_ON(clocksource_register_hz(&sirfsoc_clocksource, CLOCK_FREQ)); > > sirfsoc_clockevent_init(); > } > diff --git a/drivers/clocksource/timer-prima2.c b/drivers/clocksource/timer-prima2.c > index 8a492d3..ff7d49e 100644 > --- a/drivers/clocksource/timer-prima2.c > +++ b/drivers/clocksource/timer-prima2.c > @@ -21,6 +21,8 @@ > #include <linux/sched_clock.h> > #include <asm/mach/time.h> > > +#define CLOCK_FREQ 1000000 > + > #define SIRFSOC_TIMER_COUNTER_LO 0x0000 > #define SIRFSOC_TIMER_COUNTER_HI 0x0004 > #define SIRFSOC_TIMER_MATCH_0 0x0008 > @@ -173,7 +175,7 @@ static u64 notrace sirfsoc_read_sched_clock(void) > static void __init sirfsoc_clockevent_init(void) > { > sirfsoc_clockevent.cpumask = cpumask_of(0); > - clockevents_config_and_register(&sirfsoc_clockevent, CLOCK_TICK_RATE, > + clockevents_config_and_register(&sirfsoc_clockevent, CLOCK_FREQ, > 2, -2); > } > > @@ -190,8 +192,8 @@ static void __init sirfsoc_prima2_timer_init(struct device_node *np) > > rate = clk_get_rate(clk); > > - BUG_ON(rate < CLOCK_TICK_RATE); > - BUG_ON(rate % CLOCK_TICK_RATE); > + BUG_ON(rate < CLOCK_FREQ); > + BUG_ON(rate % CLOCK_FREQ); > > sirfsoc_timer_base = of_iomap(np, 0); > if (!sirfsoc_timer_base) > @@ -199,14 +201,14 @@ static void __init sirfsoc_prima2_timer_init(struct device_node *np) > > sirfsoc_timer_irq.irq = irq_of_parse_and_map(np, 0); > > - writel_relaxed(rate / CLOCK_TICK_RATE / 2 - 1, sirfsoc_timer_base + SIRFSOC_TIMER_DIV); > + writel_relaxed(rate / CLOCK_FREQ / 2 - 1, sirfsoc_timer_base + SIRFSOC_TIMER_DIV); > writel_relaxed(0, sirfsoc_timer_base + SIRFSOC_TIMER_COUNTER_LO); > writel_relaxed(0, sirfsoc_timer_base + SIRFSOC_TIMER_COUNTER_HI); > writel_relaxed(BIT(0), sirfsoc_timer_base + SIRFSOC_TIMER_STATUS); > > - BUG_ON(clocksource_register_hz(&sirfsoc_clocksource, CLOCK_TICK_RATE)); > + BUG_ON(clocksource_register_hz(&sirfsoc_clocksource, CLOCK_FREQ)); > > - sched_clock_register(sirfsoc_read_sched_clock, 64, CLOCK_TICK_RATE); > + sched_clock_register(sirfsoc_read_sched_clock, 64, CLOCK_FREQ); > > BUG_ON(setup_irq(sirfsoc_timer_irq.irq, &sirfsoc_timer_irq)); > >
On Wed, Nov 13, 2013 at 09:00:52PM +0100, Daniel Lezcano wrote: > On 11/11/2013 09:20 PM, Uwe Kleine-König wrote: > >As CSR SiRF is converted to multi platform CLOCK_TICK_RATE is a dummy > >value that seems to match the right value is used. > >(arch/arm/mach-prima2/include/mach/timex.h which defined CLOCK_TICK_RATE > >to 1000000 was removed in commit cf82e0e (ARM: sirf: enable > >multiplatform support); marco used the same file.) > > > >To not depend on that dummy value use a local #define instead. > > I don't get this patch. It is to fix a compilation error ? No, the problem is that CLOCK_TICK_RATE used to be defined in a platform specific header <mach/timex.h>. For the ARM multiplatform stuff, this was dropped and now all multiplatform kernels use 1000000. For some platform (like SiRF) this happens to be correct, but actually it's pure luck. Further down the road I'd like to drop defining CLOCK_TICK_RATE for all platforms, so this is a preparing patch. But even independant from that it feels wrong to use a dummy value that was only introduced to prevent compile breakage. Would this change log be better: Since CSR SiRF was converted to multi platform in cf82e0e (ARM: sirf: enable multiplatform support) the symbol CLOCK_TICK_RATE isn't the platform specific definition any more, but a global dummy value. There was no harm introduced in cf82e0e because the global value happens to match the old platform specific one, still this dummy value isn't intended to be used and will hopefully disappear soon, so introduce a local #define and use that instead. So it's not urgent, but would be a nice cleanup for 3.14-rc1. Best regards Uwe
Hi Danial, On Thu, Nov 14, 2013 at 10:07:05AM +0100, Uwe Kleine-König wrote: > On Wed, Nov 13, 2013 at 09:00:52PM +0100, Daniel Lezcano wrote: > > On 11/11/2013 09:20 PM, Uwe Kleine-König wrote: > > >As CSR SiRF is converted to multi platform CLOCK_TICK_RATE is a dummy > > >value that seems to match the right value is used. > > >(arch/arm/mach-prima2/include/mach/timex.h which defined CLOCK_TICK_RATE > > >to 1000000 was removed in commit cf82e0e (ARM: sirf: enable > > >multiplatform support); marco used the same file.) > > > > > >To not depend on that dummy value use a local #define instead. > > > > I don't get this patch. It is to fix a compilation error ? > No, the problem is that CLOCK_TICK_RATE used to be defined in a platform > specific header <mach/timex.h>. For the ARM multiplatform stuff, this > was dropped and now all multiplatform kernels use 1000000. For some > platform (like SiRF) this happens to be correct, but actually it's pure > luck. Further down the road I'd like to drop defining CLOCK_TICK_RATE > for all platforms, so this is a preparing patch. But even independant > from that it feels wrong to use a dummy value that was only introduced > to prevent compile breakage. > > Would this change log be better: > > Since CSR SiRF was converted to multi platform in cf82e0e (ARM: > sirf: enable multiplatform support) the symbol CLOCK_TICK_RATE > isn't the platform specific definition any more, but a global > dummy value. There was no harm introduced in cf82e0e because the > global value happens to match the old platform specific one, > still this dummy value isn't intended to be used and will > hopefully disappear soon, so introduce a local #define and use > that instead. > > So it's not urgent, but would be a nice cleanup for 3.14-rc1. I'd like to depend on this patch to drop CLOCK_TICK_RATE for mach-prima2. Would it be ok for you when I include it in a pull request to the arm-soc people? If not, do you intend to take that patch, or do you still have objections? In that case I'd back out mach-prima2 from my CLOCK_TICK_RATE change. Best regards Uwe
On 11/26/2013 02:55 PM, Uwe Kleine-König wrote: > Hi Danial, > > On Thu, Nov 14, 2013 at 10:07:05AM +0100, Uwe Kleine-König wrote: >> On Wed, Nov 13, 2013 at 09:00:52PM +0100, Daniel Lezcano wrote: >>> On 11/11/2013 09:20 PM, Uwe Kleine-König wrote: >>>> As CSR SiRF is converted to multi platform CLOCK_TICK_RATE is a dummy >>>> value that seems to match the right value is used. >>>> (arch/arm/mach-prima2/include/mach/timex.h which defined CLOCK_TICK_RATE >>>> to 1000000 was removed in commit cf82e0e (ARM: sirf: enable >>>> multiplatform support); marco used the same file.) >>>> >>>> To not depend on that dummy value use a local #define instead. >>> >>> I don't get this patch. It is to fix a compilation error ? >> No, the problem is that CLOCK_TICK_RATE used to be defined in a platform >> specific header <mach/timex.h>. For the ARM multiplatform stuff, this >> was dropped and now all multiplatform kernels use 1000000. For some >> platform (like SiRF) this happens to be correct, but actually it's pure >> luck. Further down the road I'd like to drop defining CLOCK_TICK_RATE >> for all platforms, so this is a preparing patch. But even independant >> from that it feels wrong to use a dummy value that was only introduced >> to prevent compile breakage. >> >> Would this change log be better: >> >> Since CSR SiRF was converted to multi platform in cf82e0e (ARM: >> sirf: enable multiplatform support) the symbol CLOCK_TICK_RATE >> isn't the platform specific definition any more, but a global >> dummy value. There was no harm introduced in cf82e0e because the >> global value happens to match the old platform specific one, >> still this dummy value isn't intended to be used and will >> hopefully disappear soon, so introduce a local #define and use >> that instead. >> >> So it's not urgent, but would be a nice cleanup for 3.14-rc1. > I'd like to depend on this patch to drop CLOCK_TICK_RATE for > mach-prima2. Would it be ok for you when I include it in a pull request > to the arm-soc people? If not, do you intend to take that patch, or do > you still have objections? In that case I'd back out mach-prima2 from my > CLOCK_TICK_RATE change. I think it would be better to keep the macro name consistent and redefine it in the file with a comment. /* over riding default value because bla bla */ #ifdef CLOCK_TICK_RATE #undef CLOCK_TICK_RATE #define CLOCK_TICK_RATE 100000 #endif So people reading the code won't have to scratch their head to understand why CLOCK_FREQ is used instead of CLOCK_TICK_RATE. And that will limit the impact in the code. Alternatively, I am wondering if that shouldn't fall into the DT, without the declaration in the DT, it defaults to 100000.
Hello Daniel, On Thu, Nov 28, 2013 at 08:34:43AM +0100, Daniel Lezcano wrote: > On 11/26/2013 02:55 PM, Uwe Kleine-König wrote: > >Hi Danial, ooops, sorry for the typo here, > >> Since CSR SiRF was converted to multi platform in cf82e0e (ARM: > >> sirf: enable multiplatform support) the symbol CLOCK_TICK_RATE > >> isn't the platform specific definition any more, but a global > >> dummy value. There was no harm introduced in cf82e0e because the > >> global value happens to match the old platform specific one, > >> still this dummy value isn't intended to be used and will > >> hopefully disappear soon, so introduce a local #define and use > >> that instead. > >> > >>So it's not urgent, but would be a nice cleanup for 3.14-rc1. > >I'd like to depend on this patch to drop CLOCK_TICK_RATE for > >mach-prima2. Would it be ok for you when I include it in a pull request > >to the arm-soc people? If not, do you intend to take that patch, or do > >you still have objections? In that case I'd back out mach-prima2 from my > >CLOCK_TICK_RATE change. Note that this would make prima2 the only platform that would need special handling as all other patches are ready now. So I'd really like to get that resolved soon. > I think it would be better to keep the macro name consistent and > redefine it in the file with a comment. > > /* over riding default value because bla bla */ > #ifdef CLOCK_TICK_RATE > #undef CLOCK_TICK_RATE > #define CLOCK_TICK_RATE 100000 > #endif Hmm, I don't like that. Redefining symbols might easily result in surprises. Moreover I want to completely get rid of CLOCK_TICK_RATE (for ARM at least), doing that redefinition makes this driver result in false positives when grepping for sites still using CLOCK_TICK_RATE. > So people reading the code won't have to scratch their head to > understand why CLOCK_FREQ is used instead of CLOCK_TICK_RATE. And > that will limit the impact in the code. IMHO this is shortsighted. Seeing only a snipplet of code using CLOCK_TICK_RATE might be easy to understand for someone who knows about CLOCK_TICK_RATE. But in fact thats an illusion because the code looks like using a global constant, but in fact it isn't. If the reader doesn't see the redefinition the value might differ from his expectations; also worse. Now add that the global CLOCK_TICK_RATE will die, so it will become less common that people know about CLOCK_TICK_RATE. Probably using a name that doesn't suggest it might be global (like MARCO_CLOCK_FREQ) is still better. > Alternatively, I am wondering if that shouldn't fall into the DT, > without the declaration in the DT, it defaults to 100000. I didn't check how the constant is used, but I agree it should be an overwritable default. Given that I don't care much about that driver but my intention is to get rid of <mach/timex.h> for all ARM platforms my motivation to add features to a driver I cannot even test is low. Best regards Uwe
On 11/28/2013 10:11 AM, Uwe Kleine-König wrote: > Hello Daniel, > > On Thu, Nov 28, 2013 at 08:34:43AM +0100, Daniel Lezcano wrote: >> On 11/26/2013 02:55 PM, Uwe Kleine-König wrote: >>> Hi Danial, > ooops, sorry for the typo here, > >>>> Since CSR SiRF was converted to multi platform in cf82e0e (ARM: >>>> sirf: enable multiplatform support) the symbol CLOCK_TICK_RATE >>>> isn't the platform specific definition any more, but a global >>>> dummy value. There was no harm introduced in cf82e0e because the >>>> global value happens to match the old platform specific one, >>>> still this dummy value isn't intended to be used and will >>>> hopefully disappear soon, so introduce a local #define and use >>>> that instead. >>>> >>>> So it's not urgent, but would be a nice cleanup for 3.14-rc1. >>> I'd like to depend on this patch to drop CLOCK_TICK_RATE for >>> mach-prima2. Would it be ok for you when I include it in a pull request >>> to the arm-soc people? If not, do you intend to take that patch, or do >>> you still have objections? In that case I'd back out mach-prima2 from my >>> CLOCK_TICK_RATE change. > Note that this would make prima2 the only platform that would need > special handling as all other patches are ready now. So I'd really like > to get that resolved soon. > >> I think it would be better to keep the macro name consistent and >> redefine it in the file with a comment. >> >> /* over riding default value because bla bla */ >> #ifdef CLOCK_TICK_RATE >> #undef CLOCK_TICK_RATE >> #define CLOCK_TICK_RATE 100000 >> #endif > Hmm, I don't like that. Redefining symbols might easily result in > surprises. Moreover I want to completely get rid of CLOCK_TICK_RATE (for > ARM at least), doing that redefinition makes this driver result in false > positives when grepping for sites still using CLOCK_TICK_RATE. > >> So people reading the code won't have to scratch their head to >> understand why CLOCK_FREQ is used instead of CLOCK_TICK_RATE. And >> that will limit the impact in the code. > IMHO this is shortsighted. Seeing only a snipplet of code using > CLOCK_TICK_RATE might be easy to understand for someone who knows about > CLOCK_TICK_RATE. But in fact thats an illusion because the code looks > like using a global constant, but in fact it isn't. If the reader > doesn't see the redefinition the value might differ from his > expectations; also worse. Now add that the global CLOCK_TICK_RATE will > die, so it will become less common that people know about > CLOCK_TICK_RATE. Probably using a name that doesn't suggest it might be > global (like MARCO_CLOCK_FREQ) is still better. Yes, probably. >> Alternatively, I am wondering if that shouldn't fall into the DT, >> without the declaration in the DT, it defaults to 100000. > I didn't check how the constant is used, but I agree it should be an > overwritable default. Given that I don't care much about that driver but > my intention is to get rid of <mach/timex.h> for all ARM platforms my > motivation to add features to a driver I cannot even test is low. Ok, I am waiting for a new version. The new changelog you proposed for the patch sound more clear. Thanks -- Daniel
diff --git a/drivers/clocksource/timer-marco.c b/drivers/clocksource/timer-marco.c index 09a17d9a..0d367a7 100644 --- a/drivers/clocksource/timer-marco.c +++ b/drivers/clocksource/timer-marco.c @@ -19,7 +19,8 @@ #include <linux/of_irq.h> #include <linux/of_address.h> #include <linux/sched_clock.h> -#include <asm/mach/time.h> + +#define CLOCK_FREQ 1000000 #define SIRFSOC_TIMER_32COUNTER_0_CTRL 0x0000 #define SIRFSOC_TIMER_32COUNTER_1_CTRL 0x0004 @@ -191,7 +192,7 @@ static int sirfsoc_local_timer_setup(struct clock_event_device *ce) ce->rating = 200; ce->set_mode = sirfsoc_timer_set_mode; ce->set_next_event = sirfsoc_timer_set_next_event; - clockevents_calc_mult_shift(ce, CLOCK_TICK_RATE, 60); + clockevents_calc_mult_shift(ce, CLOCK_FREQ, 60); ce->max_delta_ns = clockevent_delta2ns(-2, ce); ce->min_delta_ns = clockevent_delta2ns(2, ce); ce->cpumask = cpumask_of(cpu); @@ -263,11 +264,11 @@ static void __init sirfsoc_marco_timer_init(void) BUG_ON(IS_ERR(clk)); rate = clk_get_rate(clk); - BUG_ON(rate < CLOCK_TICK_RATE); - BUG_ON(rate % CLOCK_TICK_RATE); + BUG_ON(rate < CLOCK_FREQ); + BUG_ON(rate % CLOCK_FREQ); /* Initialize the timer dividers */ - timer_div = rate / CLOCK_TICK_RATE - 1; + timer_div = rate / CLOCK_FREQ - 1; writel_relaxed(timer_div << 16, sirfsoc_timer_base + SIRFSOC_TIMER_64COUNTER_CTRL); writel_relaxed(timer_div << 16, sirfsoc_timer_base + SIRFSOC_TIMER_32COUNTER_0_CTRL); writel_relaxed(timer_div << 16, sirfsoc_timer_base + SIRFSOC_TIMER_32COUNTER_1_CTRL); @@ -283,7 +284,7 @@ static void __init sirfsoc_marco_timer_init(void) /* Clear all interrupts */ writel_relaxed(0xFFFF, sirfsoc_timer_base + SIRFSOC_TIMER_INTR_STATUS); - BUG_ON(clocksource_register_hz(&sirfsoc_clocksource, CLOCK_TICK_RATE)); + BUG_ON(clocksource_register_hz(&sirfsoc_clocksource, CLOCK_FREQ)); sirfsoc_clockevent_init(); } diff --git a/drivers/clocksource/timer-prima2.c b/drivers/clocksource/timer-prima2.c index 8a492d3..ff7d49e 100644 --- a/drivers/clocksource/timer-prima2.c +++ b/drivers/clocksource/timer-prima2.c @@ -21,6 +21,8 @@ #include <linux/sched_clock.h> #include <asm/mach/time.h> +#define CLOCK_FREQ 1000000 + #define SIRFSOC_TIMER_COUNTER_LO 0x0000 #define SIRFSOC_TIMER_COUNTER_HI 0x0004 #define SIRFSOC_TIMER_MATCH_0 0x0008 @@ -173,7 +175,7 @@ static u64 notrace sirfsoc_read_sched_clock(void) static void __init sirfsoc_clockevent_init(void) { sirfsoc_clockevent.cpumask = cpumask_of(0); - clockevents_config_and_register(&sirfsoc_clockevent, CLOCK_TICK_RATE, + clockevents_config_and_register(&sirfsoc_clockevent, CLOCK_FREQ, 2, -2); } @@ -190,8 +192,8 @@ static void __init sirfsoc_prima2_timer_init(struct device_node *np) rate = clk_get_rate(clk); - BUG_ON(rate < CLOCK_TICK_RATE); - BUG_ON(rate % CLOCK_TICK_RATE); + BUG_ON(rate < CLOCK_FREQ); + BUG_ON(rate % CLOCK_FREQ); sirfsoc_timer_base = of_iomap(np, 0); if (!sirfsoc_timer_base) @@ -199,14 +201,14 @@ static void __init sirfsoc_prima2_timer_init(struct device_node *np) sirfsoc_timer_irq.irq = irq_of_parse_and_map(np, 0); - writel_relaxed(rate / CLOCK_TICK_RATE / 2 - 1, sirfsoc_timer_base + SIRFSOC_TIMER_DIV); + writel_relaxed(rate / CLOCK_FREQ / 2 - 1, sirfsoc_timer_base + SIRFSOC_TIMER_DIV); writel_relaxed(0, sirfsoc_timer_base + SIRFSOC_TIMER_COUNTER_LO); writel_relaxed(0, sirfsoc_timer_base + SIRFSOC_TIMER_COUNTER_HI); writel_relaxed(BIT(0), sirfsoc_timer_base + SIRFSOC_TIMER_STATUS); - BUG_ON(clocksource_register_hz(&sirfsoc_clocksource, CLOCK_TICK_RATE)); + BUG_ON(clocksource_register_hz(&sirfsoc_clocksource, CLOCK_FREQ)); - sched_clock_register(sirfsoc_read_sched_clock, 64, CLOCK_TICK_RATE); + sched_clock_register(sirfsoc_read_sched_clock, 64, CLOCK_FREQ); BUG_ON(setup_irq(sirfsoc_timer_irq.irq, &sirfsoc_timer_irq));
As CSR SiRF is converted to multi platform CLOCK_TICK_RATE is a dummy value that seems to match the right value is used. (arch/arm/mach-prima2/include/mach/timex.h which defined CLOCK_TICK_RATE to 1000000 was removed in commit cf82e0e (ARM: sirf: enable multiplatform support); marco used the same file.) To not depend on that dummy value use a local #define instead. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/clocksource/timer-marco.c | 13 +++++++------ drivers/clocksource/timer-prima2.c | 14 ++++++++------ 2 files changed, 15 insertions(+), 12 deletions(-)