| Message ID | 1363108124-17484-6-git-send-email-haojian.zhuang@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On Tuesday 12 March 2013, Haojian Zhuang wrote: > > - aliases { > - arm,timer-primary = &timer2; > - arm,timer-secondary = &timer1; > - }; > - > chosen { > bootargs = "root=/dev/ram0 console=ttyAMA0,38400n8 earlyprintk"; > }; > @@ -29,10 +24,14 @@ > > timer1: timer@13000100 { > compatible = "arm,sp804", "arm,primecell"; > + arm,sp804-clockevent = <0>; > + status = "ok"; > }; I would prefer to keep the old way of doing this. The "aliases" node is meant for configuration, while the timer device itself should not care whether it is used as a clockevent or clocksource or some other device. We could probably change the identifiers in the aliases to "linux,clocksource" and "linux,clockevents" instead of "arm,timer-primary" and "arm,timer-secondary". Arnd
On 03/12/2013 12:08 PM, Haojian Zhuang wrote: > Remove all code to parse sp804 in integrator platform driver. Use > clocksource_of_init() instead since these code are implemented in sp804 > driver already. > > Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org> > --- > arch/arm/boot/dts/integrator.dtsi | 3 +++ > arch/arm/boot/dts/integratorap.dts | 15 +++++------ > arch/arm/boot/dts/integratorcp.dts | 9 +++---- > arch/arm/mach-integrator/Kconfig | 3 +++ > arch/arm/mach-integrator/integrator_ap.c | 41 +----------------------------- > arch/arm/mach-integrator/integrator_cp.c | 35 ++----------------------- > 6 files changed, 20 insertions(+), 86 deletions(-) > diff --git a/arch/arm/boot/dts/integratorap.dts b/arch/arm/boot/dts/integratorap.dts > index c9c3fa3..70e321c 100644 > --- a/arch/arm/boot/dts/integratorap.dts > +++ b/arch/arm/boot/dts/integratorap.dts > @@ -9,11 +9,6 @@ > model = "ARM Integrator/AP"; > compatible = "arm,integrator-ap"; > > - aliases { > - arm,timer-primary = &timer2; > - arm,timer-secondary = &timer1; > - }; > - > chosen { > bootargs = "root=/dev/ram0 console=ttyAM0,38400n8 earlyprintk"; > }; > @@ -24,15 +19,19 @@ > }; > > timer0: timer@13000000 { > - compatible = "arm,integrator-timer"; > + compatible = "arm,sp804", "arm,primecell"; You are breaking existing dtb's changing this, but this is wrong for other reasons. The integrator does not have an SP804. It is the same programming model, but is a single timer and not the dual timer. So having a different compatible string is the correct way. I doubt it has the primecell ID registers which is what "arm,primecell" indicates. > }; > > timer1: timer@13000100 { > - compatible = "arm,integrator-timer"; > + compatible = "arm,sp804", "arm,primecell"; > + arm,sp804-clockevent = <0>; I don't like this nor the old way with aliases. We should describe the h/w features of the timer to determine what to use it for. AFAICT, all 3 timers are identical on integrator and it does not matter which one Linux picks for clocksource vs. clockevent. Rob
On Tuesday 12 March 2013, Rob Herring wrote: > On 03/12/2013 12:08 PM, Haojian Zhuang wrote: > You are breaking existing dtb's changing this, but this is wrong for > other reasons. The integrator does not have an SP804. It is the same > programming model, but is a single timer and not the dual timer. So > having a different compatible string is the correct way. I doubt it has > the primecell ID registers which is what "arm,primecell" indicates. At least the qemu model has the primecell ID only for actual sp804 but not for the integrator, see http://git.qemu.org/?p=qemu.git;a=blob;f=hw/arm_timer.c > > }; > > > > timer1: timer@13000100 { > > - compatible = "arm,integrator-timer"; > > + compatible = "arm,sp804", "arm,primecell"; > > + arm,sp804-clockevent = <0>; > > I don't like this nor the old way with aliases. We should describe the > h/w features of the timer to determine what to use it for. AFAICT, all 3 > timers are identical on integrator and it does not matter which one > Linux picks for clocksource vs. clockevent. Interesting point. I remember we had a discussion about this when the initial binding integrator code was merged, but I don't remember what the intent was for doing it like this. Probably just minimizing the changes to the existing code. Arnd
On 03/12/2013 02:33 PM, Arnd Bergmann wrote: > On Tuesday 12 March 2013, Rob Herring wrote: >> On 03/12/2013 12:08 PM, Haojian Zhuang wrote: > >> You are breaking existing dtb's changing this, but this is wrong for >> other reasons. The integrator does not have an SP804. It is the same >> programming model, but is a single timer and not the dual timer. So >> having a different compatible string is the correct way. I doubt it has >> the primecell ID registers which is what "arm,primecell" indicates. > > At least the qemu model has the primecell ID only for actual sp804 but > not for the integrator, see http://git.qemu.org/?p=qemu.git;a=blob;f=hw/arm_timer.c Right. I think I added that to qemu... Since the primecell ID registers are defined to be at 0xFF? offset, it is quite impossible for the integrator timers to have the ID registers within their 0x100 address space. > >>> }; >>> >>> timer1: timer@13000100 { >>> - compatible = "arm,integrator-timer"; >>> + compatible = "arm,sp804", "arm,primecell"; >>> + arm,sp804-clockevent = <0>; >> >> I don't like this nor the old way with aliases. We should describe the >> h/w features of the timer to determine what to use it for. AFAICT, all 3 >> timers are identical on integrator and it does not matter which one >> Linux picks for clocksource vs. clockevent. > > Interesting point. I remember we had a discussion about this when the > initial binding integrator code was merged, but I don't remember what > the intent was for doing it like this. Probably just minimizing the > changes to the existing code. Re-reading the thread on this for Integrator, I don't think any conclusion was really made, but it got merged. I don't think we should a) change the existing alias names or b) expand their usage. OMAP is a good example of lots of timers and using h/w properties to select particular instances based on properties. I think on the ARM boards the selection has been pretty arbitrary. I traced the commit history back for the timer code and could not find any evidence to the contrary except on VExpress which has broken sp804 on the core tile. Another part is sched_clock selection. I use the sp804 on highbank, but the ARM boards have their 24MHz counter. Also, if I'm on midway, then I want to use the arch timers even though the sp804 is still there. My sched_clock patches yesterday attempt to address that. Any comments on what characteristics should be used to select timer would be helpful. Higher frequency and/or bits is better, but to what limit in frequency? Rob
On 13 March 2013 02:54, Arnd Bergmann <arnd@arndb.de> wrote: > On Tuesday 12 March 2013, Haojian Zhuang wrote: >> >> - aliases { >> - arm,timer-primary = &timer2; >> - arm,timer-secondary = &timer1; >> - }; >> - >> chosen { >> bootargs = "root=/dev/ram0 console=ttyAMA0,38400n8 earlyprintk"; >> }; >> @@ -29,10 +24,14 @@ >> >> timer1: timer@13000100 { >> compatible = "arm,sp804", "arm,primecell"; >> + arm,sp804-clockevent = <0>; >> + status = "ok"; >> }; > > I would prefer to keep the old way of doing this. The "aliases" node is > meant for configuration, while the timer device itself should not care > whether it is used as a clockevent or clocksource or some other device. > > We could probably change the identifiers in the aliases to > "linux,clocksource" and "linux,clockevents" instead of > "arm,timer-primary" and "arm,timer-secondary". > > Arnd SP804 is a dual timer. Either TIMER1 or TIMER2 in the same SP804 could be configured as clock source. And either TIMER1 or TIMER2 in the same SP804 could be configured as clock event. Integratorap/cp is always using TIMER1 of SP804-A as clocksource, and TIMER1 of SP804-B as clockevent. This usage case always drop TIMER2 usage. I tried to add arm,sp804-clocksource to specify whether TIMER1 or TIMER2 is configured as clocksource. For example, realview is using TIMER2 of SP804-B as clockevent and TIMER1 of SP804-A as clocksource. If we decide to support the property like "arm,sp804-clocksource", we could get the TIMER offset & clock source information at the same time. Then alias configuration is duplicated. If we decide to support alais configuration like "linux,clocksource", we could get clock source/event information. But we don't know which TIMER is used in the same SP804. We still need one property to identify it. And people are also using both TIMERs in the same SP804, like vexpress v2m. Which method do we choose? Regards Haojian
On 13 March 2013 04:52, Rob Herring <robherring2@gmail.com> wrote: > On 03/12/2013 02:33 PM, Arnd Bergmann wrote: >> On Tuesday 12 March 2013, Rob Herring wrote: >>> On 03/12/2013 12:08 PM, Haojian Zhuang wrote: > > Since the primecell ID registers are defined to be at 0xFF? offset, it > is quite impossible for the integrator timers to have the ID registers > within their 0x100 address space. > I'm sorry that I don't know this. I can add "arm,integrator-timer" into compatible table of sp804 driver. I think that we could resolve this issue. What's your opinion? >> >>>> }; >>>> >>>> timer1: timer@13000100 { >>>> - compatible = "arm,integrator-timer"; >>>> + compatible = "arm,sp804", "arm,primecell"; >>>> + arm,sp804-clockevent = <0>; >>> >>> I don't like this nor the old way with aliases. We should describe the >>> h/w features of the timer to determine what to use it for. AFAICT, all 3 >>> timers are identical on integrator and it does not matter which one >>> Linux picks for clocksource vs. clockevent. >> >> Interesting point. I remember we had a discussion about this when the >> initial binding integrator code was merged, but I don't remember what >> the intent was for doing it like this. Probably just minimizing the >> changes to the existing code. > > Re-reading the thread on this for Integrator, I don't think any > conclusion was really made, but it got merged. I don't think we should > a) change the existing alias names or b) expand their usage. OMAP is a > good example of lots of timers and using h/w properties to select > particular instances based on properties. I think on the ARM boards the > selection has been pretty arbitrary. I traced the commit history back > for the timer code and could not find any evidence to the contrary > except on VExpress which has broken sp804 on the core tile. > > Another part is sched_clock selection. I use the sp804 on highbank, but > the ARM boards have their 24MHz counter. Also, if I'm on midway, then I > want to use the arch timers even though the sp804 is still there. My > sched_clock patches yesterday attempt to address that. Any comments on > what characteristics should be used to select timer would be helpful. > Higher frequency and/or bits is better, but to what limit in frequency? > > Rob >
On Tue, Mar 12, 2013 at 7:54 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Tuesday 12 March 2013, Haojian Zhuang wrote: >> timer1: timer@13000100 { >> compatible = "arm,sp804", "arm,primecell"; >> + arm,sp804-clockevent = <0>; >> + status = "ok"; >> }; > > I would prefer to keep the old way of doing this. The "aliases" node is > meant for configuration, while the timer device itself should not care > whether it is used as a clockevent or clocksource or some other device. > > We could probably change the identifiers in the aliases to > "linux,clocksource" and "linux,clockevents" instead of > "arm,timer-primary" and "arm,timer-secondary". I named it like that exactly to be OS-neutral. The idea is that what timer is to be used for primary and secondary may have implications on things like power consumption and one timer may be broken in the HW etc. That is not a Linux-specific thing. A bit hard to do things right here :-/ Yours, Linus Walleij
On Tue, Mar 12, 2013 at 6:08 PM, Haojian Zhuang <haojian.zhuang@linaro.org> wrote: > Remove all code to parse sp804 in integrator platform driver. Use > clocksource_of_init() instead since these code are implemented in sp804 > driver already. (...) > --- a/arch/arm/boot/dts/integratorap.dts > +++ b/arch/arm/boot/dts/integratorap.dts > timer0: timer@13000000 { > - compatible = "arm,integrator-timer"; > + compatible = "arm,sp804", "arm,primecell"; > }; > > timer1: timer@13000100 { > - compatible = "arm,integrator-timer"; > + compatible = "arm,sp804", "arm,primecell"; > + arm,sp804-clockevent = <0>; > + status = "ok"; > }; > > timer2: timer@13000200 { > - compatible = "arm,integrator-timer"; > + compatible = "arm,sp804", "arm,primecell"; > + arm,sp804-clocksource = <0>; > + status = "ok"; > }; This is wrong. The Integrator/AP timer is not an SP804. It may look similar but it's a totally different kind of beast. What is needed is to break out the timer code from integrator_ap.c and move it to drivers/clocksource, but wait with that until you fixed the SP804 in the Integrator/CP, which is probably what you want to prioritize. (...) > --- a/arch/arm/mach-integrator/Kconfig > +++ b/arch/arm/mach-integrator/Kconfig (...) > -static void __init ap_of_timer_init(void) > -{ > - struct device_node *node; > - const char *path; > - void __iomem *base; > - int err; > - int irq; > - struct clk *clk; > - unsigned long rate; > - > - clk = clk_get_sys("ap_timer", NULL); As you see you are not changing this clock lookup either, which will never work. Please restrict the patch to altering Integrator/CP. Yours, Linus Walleij
On Wed, Mar 13, 2013 at 3:04 AM, Haojian Zhuang <haojian.zhuang@linaro.org> wrote: > On 13 March 2013 04:52, Rob Herring <robherring2@gmail.com> wrote: >> On 03/12/2013 02:33 PM, Arnd Bergmann wrote: >>> On Tuesday 12 March 2013, Rob Herring wrote: >>>> On 03/12/2013 12:08 PM, Haojian Zhuang wrote: >> >> Since the primecell ID registers are defined to be at 0xFF? offset, it >> is quite impossible for the integrator timers to have the ID registers >> within their 0x100 address space. >> > I'm sorry that I don't know this. I can add "arm,integrator-timer" into > compatible table of sp804 driver. I think that we could resolve this > issue. What's your opinion? As per the other discussion: The Integrator/AP has a timer which is different from the SP804. The Integrator/CP has an SP804. Please restrict the patch set to Integrator/CP. Yours, Linus Walleij
On 13 March 2013 14:41, Linus Walleij <linus.walleij@linaro.org> wrote: > On Wed, Mar 13, 2013 at 3:04 AM, Haojian Zhuang > <haojian.zhuang@linaro.org> wrote: >> On 13 March 2013 04:52, Rob Herring <robherring2@gmail.com> wrote: >>> On 03/12/2013 02:33 PM, Arnd Bergmann wrote: >>>> On Tuesday 12 March 2013, Rob Herring wrote: >>>>> On 03/12/2013 12:08 PM, Haojian Zhuang wrote: >>> >>> Since the primecell ID registers are defined to be at 0xFF? offset, it >>> is quite impossible for the integrator timers to have the ID registers >>> within their 0x100 address space. >>> >> I'm sorry that I don't know this. I can add "arm,integrator-timer" into >> compatible table of sp804 driver. I think that we could resolve this >> issue. What's your opinion? > > As per the other discussion: > > The Integrator/AP has a timer which is different from the SP804. > > The Integrator/CP has an SP804. > > Please restrict the patch set to Integrator/CP. > > Yours, > Linus Walleij According to Rob's comment, I thought that only prime-cell ID registers are not contained in integrator-timer controller. Yes, I missed to include lookup to handle integrator-ap timer. I think that we can use a string name that are binded to different compatible name. Code is in below. #define SP804_TIMER 0 #define INTEGRATOR_TIMER 1 #define ID_BUF_SIZE 16 match table: {.compatible = "arm,sp804", .data = SP804_TIMER, }, {.compaitlbe = "arm,integrator-timer", .data = INTEGRATOR_TIMER, }, sp804_get_clock_rate(): char dev_id_buf[ID_BUF_SIZE]; if (match->data) snprintf(dev_id_buf, ID_BUF_SIZE, "ap_timer"); else snprintf(dev_id_buf, ID_BUF_SIZE, "sp804"); clk = clk_get_sys(dev_id_buf, name); sp804_dt_init_clk(): char dev_id_buf[ID_BUF_SIZE]; if (match->data) snprintf(dev_id_buf, ID_BUF_SIZE, "ap_timer"); else snprintf(dev_id_buf, ID_BUF_SIZE, "sp804"); lookup = clkdev_alloc(clk, name, dev_id_buf); I think it won't break out code any more. Since most code are same, we could handle it in the same driver. Regards Haojian
On Wednesday 13 March 2013, Haojian Zhuang wrote: > match table: > {.compatible = "arm,sp804", .data = SP804_TIMER, }, > {.compaitlbe = "arm,integrator-timer", .data = INTEGRATOR_TIMER, }, > > sp804_get_clock_rate(): > char dev_id_buf[ID_BUF_SIZE]; > if (match->data) > snprintf(dev_id_buf, ID_BUF_SIZE, "ap_timer"); > else > snprintf(dev_id_buf, ID_BUF_SIZE, "sp804"); > clk = clk_get_sys(dev_id_buf, name); > > sp804_dt_init_clk(): > char dev_id_buf[ID_BUF_SIZE]; > if (match->data) > snprintf(dev_id_buf, ID_BUF_SIZE, "ap_timer"); > else > snprintf(dev_id_buf, ID_BUF_SIZE, "sp804"); > lookup = clkdev_alloc(clk, name, dev_id_buf); > > I think it won't break out code any more. Since most code are same, we > could handle it in the same driver. > The data member of struct of_device_id is a pointer, you could point that directly to a const string containing the name, if that is the only difference, or to another structure that describes the other parameters. Arnd
On Wed, Mar 13, 2013 at 8:09 AM, Haojian Zhuang <haojian.zhuang@linaro.org> wrote: > On 13 March 2013 14:41, Linus Walleij <linus.walleij@linaro.org> wrote: >> Please restrict the patch set to Integrator/CP. > > According to Rob's comment, I thought that only prime-cell ID registers > are not contained in integrator-timer controller. No, you have missed the most important aspect of all. The Integrator/AP timer is only 16 bit. Look: static void integrator_clocksource_init(unsigned long inrate, void __iomem *base) { (...) clocksource_mmio_init(base + TIMER_VALUE, "timer2", rate, 200, 16, clocksource_mmio_readl_down); (...) } Compare: void __init __sp804_clocksource_and_sched_clock_init(void __iomem *base, const char *name, int use_sched_clock) { (...) clocksource_mmio_init(base + TIMER_VALUE, name, rate, 200, 32, clocksource_mmio_readl_down); (...) } I had the idea of merging the drivers in the past so I'm positive to the change from a larger perspective. But then you also have to take the bit-width precision into account. If you apply the patch, the Integrator/AP will appear to work, then if you leave it dormant until the counter wraps around it will hang or screw up the timeline. Another part of the problem I see is that the code in the SP804 driver is lacking features found in the code for the driver inside integrator_ap.c, I detail these below... > Yes, I missed to include lookup to handle integrator-ap timer. I think that > we can use a string name that are binded to different compatible name. > Code is in below. (...) > sp804_get_clock_rate(): > char dev_id_buf[ID_BUF_SIZE]; > if (match->data) > snprintf(dev_id_buf, ID_BUF_SIZE, "ap_timer"); > else > snprintf(dev_id_buf, ID_BUF_SIZE, "sp804"); > clk = clk_get_sys(dev_id_buf, name); > > sp804_dt_init_clk(): > char dev_id_buf[ID_BUF_SIZE]; > if (match->data) > snprintf(dev_id_buf, ID_BUF_SIZE, "ap_timer"); > else > snprintf(dev_id_buf, ID_BUF_SIZE, "sp804"); > lookup = clkdev_alloc(clk, name, dev_id_buf); If this is what you want to attain, I think it would be better to just patch the clock lookups in drivers/clk/clk-integrator.c and rename the "ap_timer" lookup to "sp804" and skip all the above. > I think it won't break out code any more. Since most code are same, we > could handle it in the same driver. So it will definately break stuff. Being 16bit is not all, here are other differences: static u32 notrace integrator_read_sched_clock(void) { return -readl((void __iomem *) TIMER2_VA_BASE + TIMER_VALUE); } static void integrator_clocksource_init(unsigned long inrate, void __iomem *base) { (...) setup_sched_clock(integrator_read_sched_clock, 16, rate); } Since you do not call even integrator_clocksource_init() anymore after this, you are breaking sched_clock() for the Integrator/AP. The SP804 doesn't even have a sched_clock option. You make it fall back to using jiffies. So you would have to add this to the SP804 driver, and make sure sched_clock() is registered for the "arm,integrator" compatible string. Then the Integrator/AP timer has this code in the integrator_clockevent_init() function: clkevt_base = base; /* Calculate and program a divisor */ if (rate > 0x100000 * HZ) { rate /= 256; ctrl |= TIMER_CTRL_DIV256; } else if (rate > 0x10000 * HZ) { rate /= 16; ctrl |= TIMER_CTRL_DIV16; } It appears the SP804 has no code to handle this divisor/prescaler at all. Which means that you regress the Integrator/AP again. It appears (from arm_timer.h) that the SP804 actually has this prescaler, but then first make a separate patch to make the SP804 driver use that divisor (like above) so you don't degrade the quality of the AP timer. The prescaler make it possible to let the system sleep for longer times when using NO_HZ so it's *pretty* important (and it will increas the quality of all SP804-based machines as well). As you surely understand, since the counter on the Integrator/AP is only 16 bit this is very important, since we don't want it to wake up too often. Then the clocksource init function has this: if (rate >= 1500000) { rate /= 16; ctrl |= TIMER_CTRL_DIV16; } Here I'm more uncertain. As I know from extensive discussions with John and Thomas, there are many many factors to decide the clocksource. Basically the above lines limits the precision of the clocksource in exchange for a longer time until wrap-around. Again, the 16bit nature of the Integrator/AP timer is a factor here. Yours, Linus Walleij
On Tue, Mar 12, 2013 at 8:33 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Tuesday 12 March 2013, Rob Herring wrote: >> On 03/12/2013 12:08 PM, Haojian Zhuang wrote: > >> You are breaking existing dtb's changing this, but this is wrong for >> other reasons. The integrator does not have an SP804. It is the same >> programming model, but is a single timer and not the dual timer. So >> having a different compatible string is the correct way. I doubt it has >> the primecell ID registers which is what "arm,primecell" indicates. > > At least the qemu model has the primecell ID only for actual sp804 but > not for the integrator, see http://git.qemu.org/?p=qemu.git;a=blob;f=hw/arm_timer.c That is the for the Integrator/CP model right? Part of the problem with the current patch series is assuming similarity between the Integrator/AP and Integrator/CP. The hardware (VHDL code) for the Integrator/CP SP804 block is certainly based on the Integrator/AP (unnamed "integrator timer") but many changes were made, the most drastic extending the counters from 16 to 32 bits. Yours, Linus Walleij
On 03/13/2013 01:41 AM, Linus Walleij wrote: > On Wed, Mar 13, 2013 at 3:04 AM, Haojian Zhuang > <haojian.zhuang@linaro.org> wrote: >> On 13 March 2013 04:52, Rob Herring <robherring2@gmail.com> wrote: >>> On 03/12/2013 02:33 PM, Arnd Bergmann wrote: >>>> On Tuesday 12 March 2013, Rob Herring wrote: >>>>> On 03/12/2013 12:08 PM, Haojian Zhuang wrote: >>> >>> Since the primecell ID registers are defined to be at 0xFF? offset, it >>> is quite impossible for the integrator timers to have the ID registers >>> within their 0x100 address space. >>> >> I'm sorry that I don't know this. I can add "arm,integrator-timer" into >> compatible table of sp804 driver. I think that we could resolve this >> issue. What's your opinion? > > As per the other discussion: > > The Integrator/AP has a timer which is different from the SP804. > > The Integrator/CP has an SP804. Not quite. I was wrong that they are the same, The AP has 16-bit timers and the CP has 32-bit timers. While the individual timer on the AP is the same programming model as the sp804, it is still not the sp804 block. Rob > > Please restrict the patch set to Integrator/CP. > > Yours, > Linus Walleij >
On Tue, Mar 12, 2013 at 02:15:09PM -0500, Rob Herring wrote: > You are breaking existing dtb's changing this, but this is wrong for > other reasons. The integrator does not have an SP804. It is the same > programming model, No, it is not the same programming model. It is a subset of the model. Integrator/AP is only 16-bit, and needs to have a prescaler configured according to the clockrate. Integrator/CP and later platforms have SP804, which looks the same, but is 32-bit instead. If you look at arch/arm/include/asm/hardware/arm_timer.h, you will see that I fully documented what registers and bits are present on each platform of AP, CP, Versatile/AB,PB and Realview. Note that even through the CP and Versatile platforms are both called SP804, there are subtle differences between the implementations.
On Wed, Mar 13, 2013 at 10:00:14AM +0100, Linus Walleij wrote: > Then the Integrator/AP timer has this code in the > integrator_clockevent_init() function: > > clkevt_base = base; > /* Calculate and program a divisor */ > if (rate > 0x100000 * HZ) { > rate /= 256; > ctrl |= TIMER_CTRL_DIV256; > } else if (rate > 0x10000 * HZ) { > rate /= 16; > ctrl |= TIMER_CTRL_DIV16; > } > > It appears the SP804 has no code to handle this > divisor/prescaler at all. Which means that you regress > the Integrator/AP again. > > It appears (from arm_timer.h) that the SP804 actually > has this prescaler, but then first make a separate patch > to make the SP804 driver use that divisor (like above) so > you don't degrade the quality of the AP timer. The prescaler > make it possible to let the system sleep for longer times > when using NO_HZ so it's *pretty* important (and it will > increas the quality of all SP804-based machines as well). > > As you surely understand, since the counter on the > Integrator/AP is only 16 bit this is very important, since > we don't want it to wake up too often. > > Then the clocksource init function has this: > > if (rate >= 1500000) { > rate /= 16; > ctrl |= TIMER_CTRL_DIV16; > } > > Here I'm more uncertain. As I know from extensive > discussions with John and Thomas, there are many many > factors to decide the clocksource. Basically the above lines > limits the precision of the clocksource in exchange for a > longer time until wrap-around. Again, the 16bit nature of the > Integrator/AP timer is a factor here. Integrator/AP selects the prescaler based on the fact that it is only a 16-bit counter, which has to be clocked at a rate which gives us our desired periodic tick interval. SP804 does not do this because, being a 32-bit counter, it is completely unnecessary to use the prescaler - and using the prescaler will result in loss of fine-grained timing accuracy. Making use of the prescaler there is absurd - the timer will run to 4000 odd seconds before wrapping with a prescaler of 1. You end up needing 64-bit arithmetic if you try to apply the Integrator/AP logic to SP804 - it becomes this: if (rate > 0x1000000000 * HZ) { rate /= 256; ctrl |= TIMER_CTRL_DIV256; } else if (rate > 0x100000000 * HZ) { rate /= 16; ctrl |= TIMER_CTRL_DIV16; } or more precisely: if (rate > HZ * 0x100 * ((unsigned long long)wrap + 1)) { ... } else if (rate > HZ * 0x10 * ((unsigned long long)wrap + 1)) { ... You need that wrap value anyway for clockevents_config_and_register.
On Fri, Mar 15, 2013 at 1:15 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > Integrator/AP selects the prescaler based on the fact that it is only a > 16-bit counter, which has to be clocked at a rate which gives us our > desired periodic tick interval. Ah! I knew there was some simple explanation behind this. Thanks Russell! Linus Walleij
diff --git a/arch/arm/boot/dts/integrator.dtsi b/arch/arm/boot/dts/integrator.dtsi index 813b91d..749ac21 100644 --- a/arch/arm/boot/dts/integrator.dtsi +++ b/arch/arm/boot/dts/integrator.dtsi @@ -9,18 +9,21 @@ reg = <0x13000000 0x100>; interrupt-parent = <&pic>; interrupts = <5>; + status = "disabled"; }; timer@13000100 { reg = <0x13000100 0x100>; interrupt-parent = <&pic>; interrupts = <6>; + status = "disabled"; }; timer@13000200 { reg = <0x13000200 0x100>; interrupt-parent = <&pic>; interrupts = <7>; + status = "disabled"; }; pic@14000000 { diff --git a/arch/arm/boot/dts/integratorap.dts b/arch/arm/boot/dts/integratorap.dts index c9c3fa3..70e321c 100644 --- a/arch/arm/boot/dts/integratorap.dts +++ b/arch/arm/boot/dts/integratorap.dts @@ -9,11 +9,6 @@ model = "ARM Integrator/AP"; compatible = "arm,integrator-ap"; - aliases { - arm,timer-primary = &timer2; - arm,timer-secondary = &timer1; - }; - chosen { bootargs = "root=/dev/ram0 console=ttyAM0,38400n8 earlyprintk"; }; @@ -24,15 +19,19 @@ }; timer0: timer@13000000 { - compatible = "arm,integrator-timer"; + compatible = "arm,sp804", "arm,primecell"; }; timer1: timer@13000100 { - compatible = "arm,integrator-timer"; + compatible = "arm,sp804", "arm,primecell"; + arm,sp804-clockevent = <0>; + status = "ok"; }; timer2: timer@13000200 { - compatible = "arm,integrator-timer"; + compatible = "arm,sp804", "arm,primecell"; + arm,sp804-clocksource = <0>; + status = "ok"; }; pic: pic@14000000 { diff --git a/arch/arm/boot/dts/integratorcp.dts b/arch/arm/boot/dts/integratorcp.dts index 8b11939..19b2e3e 100644 --- a/arch/arm/boot/dts/integratorcp.dts +++ b/arch/arm/boot/dts/integratorcp.dts @@ -9,11 +9,6 @@ model = "ARM Integrator/CP"; compatible = "arm,integrator-cp"; - aliases { - arm,timer-primary = &timer2; - arm,timer-secondary = &timer1; - }; - chosen { bootargs = "root=/dev/ram0 console=ttyAMA0,38400n8 earlyprintk"; }; @@ -29,10 +24,14 @@ timer1: timer@13000100 { compatible = "arm,sp804", "arm,primecell"; + arm,sp804-clockevent = <0>; + status = "ok"; }; timer2: timer@13000200 { compatible = "arm,sp804", "arm,primecell"; + arm,sp804-clocksource = <0>; + status = "ok"; }; pic: pic@14000000 { diff --git a/arch/arm/mach-integrator/Kconfig b/arch/arm/mach-integrator/Kconfig index abeff25..031f43a 100644 --- a/arch/arm/mach-integrator/Kconfig +++ b/arch/arm/mach-integrator/Kconfig @@ -5,6 +5,7 @@ menu "Integrator Options" config ARCH_INTEGRATOR_AP bool "Support Integrator/AP and Integrator/PP2 platforms" select CLKSRC_MMIO + select CLKSRC_OF select MIGHT_HAVE_PCI select SERIAL_AMBA_PL010 select SERIAL_AMBA_PL010_CONSOLE @@ -15,6 +16,8 @@ config ARCH_INTEGRATOR_AP config ARCH_INTEGRATOR_CP bool "Support Integrator/CP platform" + select CLKSRC_MMIO + select CLKSRC_OF select ARCH_CINTEGRATOR select ARM_TIMER_SP804 select PLAT_VERSATILE_CLCD diff --git a/arch/arm/mach-integrator/integrator_ap.c b/arch/arm/mach-integrator/integrator_ap.c index c2112ff..2266944 100644 --- a/arch/arm/mach-integrator/integrator_ap.c +++ b/arch/arm/mach-integrator/integrator_ap.c @@ -425,45 +425,6 @@ void __init ap_init_early(void) #ifdef CONFIG_OF -static void __init ap_of_timer_init(void) -{ - struct device_node *node; - const char *path; - void __iomem *base; - int err; - int irq; - struct clk *clk; - unsigned long rate; - - clk = clk_get_sys("ap_timer", NULL); - BUG_ON(IS_ERR(clk)); - clk_prepare_enable(clk); - rate = clk_get_rate(clk); - - err = of_property_read_string(of_aliases, - "arm,timer-primary", &path); - if (WARN_ON(err)) - return; - node = of_find_node_by_path(path); - base = of_iomap(node, 0); - if (WARN_ON(!base)) - return; - writel(0, base + TIMER_CTRL); - integrator_clocksource_init(rate, base); - - err = of_property_read_string(of_aliases, - "arm,timer-secondary", &path); - if (WARN_ON(err)) - return; - node = of_find_node_by_path(path); - base = of_iomap(node, 0); - if (WARN_ON(!base)) - return; - irq = irq_of_parse_and_map(node, 0); - writel(0, base + TIMER_CTRL); - integrator_clockevent_init(rate, base, irq); -} - static const struct of_device_id fpga_irq_of_match[] __initconst = { { .compatible = "arm,versatile-fpga-irq", .data = fpga_irq_of_init, }, { /* Sentinel */ } @@ -582,7 +543,7 @@ DT_MACHINE_START(INTEGRATOR_AP_DT, "ARM Integrator/AP (Device Tree)") .init_early = ap_init_early, .init_irq = ap_init_irq_of, .handle_irq = fpga_handle_irq, - .init_time = ap_of_timer_init, + .init_time = clocksource_of_init, .init_machine = ap_init_of, .restart = integrator_restart, .dt_compat = ap_dt_board_compat, diff --git a/arch/arm/mach-integrator/integrator_cp.c b/arch/arm/mach-integrator/integrator_cp.c index 40373ec..6e1c340 100644 --- a/arch/arm/mach-integrator/integrator_cp.c +++ b/arch/arm/mach-integrator/integrator_cp.c @@ -28,6 +28,7 @@ #include <linux/of_address.h> #include <linux/of_platform.h> #include <linux/sys_soc.h> +#include <linux/clocksource.h> #include <clocksource/arm_timer.h> #include <clocksource/timer-sp.h> @@ -251,38 +252,6 @@ static void __init intcp_init_early(void) #ifdef CONFIG_OF -static void __init cp_of_timer_init(void) -{ - struct device_node *node; - const char *path; - void __iomem *base; - int err; - int irq; - - err = of_property_read_string(of_aliases, - "arm,timer-primary", &path); - if (WARN_ON(err)) - return; - node = of_find_node_by_path(path); - base = of_iomap(node, 0); - if (WARN_ON(!base)) - return; - writel(0, base + TIMER_CTRL); - sp804_clocksource_init(base, node->name); - - err = of_property_read_string(of_aliases, - "arm,timer-secondary", &path); - if (WARN_ON(err)) - return; - node = of_find_node_by_path(path); - base = of_iomap(node, 0); - if (WARN_ON(!base)) - return; - irq = irq_of_parse_and_map(node, 0); - writel(0, base + TIMER_CTRL); - sp804_clockevents_init(base, irq, node->name); -} - static const struct of_device_id fpga_irq_of_match[] __initconst = { { .compatible = "arm,versatile-fpga-irq", .data = fpga_irq_of_init, }, { /* Sentinel */ } @@ -386,7 +355,7 @@ DT_MACHINE_START(INTEGRATOR_CP_DT, "ARM Integrator/CP (Device Tree)") .init_early = intcp_init_early, .init_irq = intcp_init_irq_of, .handle_irq = fpga_handle_irq, - .init_time = cp_of_timer_init, + .init_time = clocksource_of_init, .init_machine = intcp_init_of, .restart = integrator_restart, .dt_compat = intcp_dt_board_compat,
Remove all code to parse sp804 in integrator platform driver. Use clocksource_of_init() instead since these code are implemented in sp804 driver already. Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org> --- arch/arm/boot/dts/integrator.dtsi | 3 +++ arch/arm/boot/dts/integratorap.dts | 15 +++++------ arch/arm/boot/dts/integratorcp.dts | 9 +++---- arch/arm/mach-integrator/Kconfig | 3 +++ arch/arm/mach-integrator/integrator_ap.c | 41 +----------------------------- arch/arm/mach-integrator/integrator_cp.c | 35 ++----------------------- 6 files changed, 20 insertions(+), 86 deletions(-)