| Message ID | 1479922177-20136-7-git-send-email-al.kochet@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> In order to use the patch you have to setup the timer using > 'rockchip,clocksource' device tree property Just came in mind, that it is better to replace 'rockchip,clocksource' device tree property with KConfig option in order to enable clocksource on dedicated timer? Someting like: [ ] enable clocksource clocksource timer name: Any feedback would be appreciated. Alexander.
Am Donnerstag, 24. November 2016, 12:36:20 schrieb Alexander Kochetkov: > > In order to use the patch you have to setup the timer using > > 'rockchip,clocksource' device tree property > > Just came in mind, that it is better to replace 'rockchip,clocksource' > device tree property with KConfig option in order to enable clocksource on > dedicated timer? > > Someting like: > [ ] enable clocksource > clocksource timer name: That would mean recompiling the kernel for a maybe board-specific setting and is definitly not how things are handled these days :-) . I.e. the overall goal is to have one kernel image that can actually run on multiple arm architectures (rockchip, imx, etc) and only gets configured by the devicetree. In your dts-patch you reuse the rk3288-timer compatible value, which is also non-ideal. What you may want to do is introduce a rockchip,rk3188-timer compatible and then make the timer-driver act accordingly, as you then know you are on a rk3188-board ... see drivers attaching specific structs to the of_device_id entries. From the documentation it also shouldn't really matter which timer you use as clocksource, as on the rk3188 it seems all of them act the same way (except timer3 being always on). When touching devicetree-properties, please also adapt the binding document Documentation/devicetree/bindings/timer,rockchip,rk-timer.txt in this case and also include the devicetree maintainers. Heiko
Hello Heiko! > 24 нояб. 2016 г., в 15:17, Heiko Stübner <heiko@sntech.de> написал(а): > > In your dts-patch you reuse the rk3288-timer compatible value, which is also > non-ideal. rockchip,rk-timer.txt states what I should use rockchip,rk3288-timer for rk3188 timers: Required properties: - compatible: shall be one of: "rockchip,rk3288-timer" - for rk3066, rk3036, rk3188, rk322x, rk3288, rk3368 Should I add "rockchip,rk3188-timer» (or better "rockchip,rk3036-timer») ? Or may be second approach should be used? > What you may want to do is introduce a rockchip,rk3188-timer compatible and > then make the timer-driver act accordingly, as you then know you are on a > rk3188-board ... see drivers attaching specific structs to the of_device_id > entries. From the documentation May be it is better to prepend compatible string with "rockchip,rk3188-timer» in the dts file only? elinux[1] recommend build compatible string from "exact device» string and «other devices» that the device is compatible with. As «other devices» string defined already, I use it. [1] http://elinux.org/Device_Tree_Usage#Understanding_the_compatible_Property timer0: timer@20038000 { compatible = "rockchip,rk3188-timer", "rockchip,rk3288-timer»; … }; Just found what rk3036 already declared such way: rk3036.dtsi: timer: timer@20044000 { rk3036.dtsi: compatible = "rockchip,rk3036-timer", "rockchip,rk3288-timer»; > > it also shouldn't really matter which timer > you use as clocksource, as on the rk3188 it seems all of them act the same way > (except timer3 being always on). You are right. I sent dts file with general timer settings, without clockevent enabled, so one could pick up timer and setup it in the board dts (radxarock, for example) like: &timer0 { rockchip,clocksource; status = «okay»; }; > When touching devicetree-properties, please also adapt the binding document > Documentation/devicetree/bindings/timer,rockchip,rk-timer.txt > in this case and also include the devicetree maintainers. If the patch[2] ok, I'll resend it and include devicetree maintainers. [2] http://lists.infradead.org/pipermail/linux-rockchip/2016-November/013148.html Regards, Alexander.
Hi Alexander, Am Donnerstag, 24. November 2016, 16:05:55 schrieb Alexander Kochetkov: > Hello Heiko! > > > 24 нояб. 2016 г., в 15:17, Heiko Stübner <heiko@sntech.de> написал(а): > > > > In your dts-patch you reuse the rk3288-timer compatible value, which is > > also non-ideal. > > rockchip,rk-timer.txt states what I should use rockchip,rk3288-timer for > rk3188 timers: > > Required properties: > - compatible: shall be one of: > "rockchip,rk3288-timer" - for rk3066, rk3036, rk3188, rk322x, rk3288, > rk3368 > > Should I add "rockchip,rk3188-timer» (or better "rockchip,rk3036-timer») ? > Or may be second approach should be used? > > > What you may want to do is introduce a rockchip,rk3188-timer compatible > > and > > then make the timer-driver act accordingly, as you then know you are on a > > rk3188-board ... see drivers attaching specific structs to the > > of_device_id > > entries. From the documentation > > May be it is better to prepend compatible string with > "rockchip,rk3188-timer» in the dts file only? elinux[1] recommend build > compatible string from "exact device» string and «other devices» that the > device is compatible with. > > As «other devices» string defined already, I use it. > > [1] > http://elinux.org/Device_Tree_Usage#Understanding_the_compatible_Property > > timer0: timer@20038000 { > compatible = "rockchip,rk3188-timer", "rockchip,rk3288-timer»; > … > }; > > Just found what rk3036 already declared such way: > > rk3036.dtsi: timer: timer@20044000 { > rk3036.dtsi: compatible = "rockchip,rk3036-timer", "rockchip,rk3288- timer»; correct, use both but also update the binding, like mmc/rockchip-dw-mshc.txt does. > > it also shouldn't really matter which timer > > you use as clocksource, as on the rk3188 it seems all of them act the same > > way (except timer3 being always on). > > You are right. > > I sent dts file with general timer settings, without clockevent enabled, so > one could pick up timer and setup it in the board dts (radxarock, for > example) like: > > &timer0 { > rockchip,clocksource; > status = «okay»; > }; what I actually meant was that the driver could also recognize the rk3188- timer compatible as "we need a clocksource" and it shouldn't matter which timer actually gets used for this. > > When touching devicetree-properties, please also adapt the binding > > document > > > > Documentation/devicetree/bindings/timer,rockchip,rk-timer.txt > > > > in this case and also include the devicetree maintainers. > > If the patch[2] ok, I'll resend it and include devicetree maintainers. > > [2] > http://lists.infradead.org/pipermail/linux-rockchip/2016-November/013148.ht > ml Only devicetree people can really tell you if that is ok :-) . The devicetree is supposed to be a hardware-description and implementation- independent, so rockchip,clocksource sounds pretty much like linux-specific configuration things leaking into the devicetree. Heiko
> 24 нояб. 2016 г., в 16:21, Heiko Stübner <heiko@sntech.de> написал(а): > > what I actually meant was that the driver could also recognize the rk3188- > timer compatible as "we need a clocksource" and it shouldn't matter which > timer actually gets used for this. One rockchip timer cannot be used as clockevent and clocksource at the same time. In case of clockevent we want interrupts from it at specified times. So we load one value into timer counter and it generates an interrupt. In case of clocksource we load max value into timer counter, run timer and read current value on demand. rockchip_timer driver currently implement clockevent. So, if I create only one timer in the device tree, it should be clockevent timer. As that behavior already expected from driver by people used it. I may suggest such solution here: if I want clocksource, I have to declare two timer in device tree. First probed timer would be clockevent and second one would be clocksource. All other timers will be ignored. Is that solution good? If I want one timer and want it be clocksource not clockevent how that situation should be configured? Device tree not good for this. Kconfig not good. Pass that configuration on kernel command line? > Only devicetree people can really tell you if that is ok :-) . > > The devicetree is supposed to be a hardware-description and implementation- > independent, so rockchip,clocksource sounds pretty much like linux-specific > configuration things leaking into the devicetree. You are right. They don’t allow pass linux configuration using device tree. Regards, Alexander.
Am Donnerstag, 24. November 2016, 17:14:56 schrieb Alexander Kochetkov: > > 24 нояб. 2016 г., в 16:21, Heiko Stübner <heiko@sntech.de> написал(а): > > > > what I actually meant was that the driver could also recognize the rk3188- > > timer compatible as "we need a clocksource" and it shouldn't matter which > > timer actually gets used for this. > > One rockchip timer cannot be used as clockevent and clocksource at the same > time. > > In case of clockevent we want interrupts from it at specified times. So we > load one value into timer counter and it generates an interrupt. > > In case of clocksource we load max value into timer counter, run timer and > read current value on demand. > > rockchip_timer driver currently implement clockevent. So, if I create only > one timer in the device tree, it should be clockevent timer. As that > behavior already expected from driver by people used it. > > I may suggest such solution here: if I want clocksource, I have to declare > two timer in device tree. First probed timer would be clockevent and second > one would be clocksource. All other timers will be ignored. Is that > solution good? yep, sounds good, especially as with your patch 9/9 you already declare these necessary timers. > If I want one timer and want it be clocksource not clockevent how that > situation should be configured? Device tree not good for this. Kconfig not > good. Pass that configuration on kernel command line? simply ignore that case :-) I.e. newer kernels are supposed to be able to run old devicetrees and in that case they will have the global-timer as (slightly unstable) clocksource. Also on the cortex-a9 we also still have the smp-twd as clockevent device. Heiko
Hello Heiko! > 24 нояб. 2016 г., в 15:17, Heiko Stübner <heiko@sntech.de> написал(а): > > When touching devicetree-properties, please also adapt the binding document > Documentation/devicetree/bindings/timer,rockchip,rk-timer.txt > in this case and also include the devicetree maintainers. Could you please clarify, i should send whole patch series and include devicetree maintainers for whole patch series or i should send devicetree patches separately? > 24 нояб. 2016 г., в 16:21, Heiko Stübner <heiko@sntech.de> написал(а): > > correct, use both but also update the binding, like > mmc/rockchip-dw-mshc.txt does. Here devicetree patch for this: http://www.spinics.net/lists/devicetree/msg152246.html > 24 нояб. 2016 г., в 17:32, Heiko Stübner <heiko@sntech.de> написал(а): > >> I may suggest such solution here: if I want clocksource, I have to declare >> two timer in device tree. First probed timer would be clockevent and second >> one would be clocksource. All other timers will be ignored. Is that >> solution good? > > yep, sounds good, especially as with your patch 9/9 you already declare these > necessary timers. > >> If I want one timer and want it be clocksource not clockevent how that >> situation should be configured? Device tree not good for this. Kconfig not >> good. Pass that configuration on kernel command line? > > simply ignore that case :-) Here devicetree patch for this: http://www.spinics.net/lists/devicetree/msg152247.html And I’ll going to resend new patch series with discussed modifications. It will contain only rk_timer and dts modifications. I have to do more tests. Regards, Alexander.
diff --git a/drivers/clocksource/rockchip_timer.c b/drivers/clocksource/rockchip_timer.c index 61787c5..77bea97 100644 --- a/drivers/clocksource/rockchip_timer.c +++ b/drivers/clocksource/rockchip_timer.c @@ -11,6 +11,7 @@ #include <linux/clockchips.h> #include <linux/init.h> #include <linux/interrupt.h> +#include <linux/sched_clock.h> #include <linux/of.h> #include <linux/of_address.h> #include <linux/of_irq.h> @@ -42,7 +43,13 @@ struct rk_clock_event_device { struct rk_timer timer; }; +struct rk_clocksource { + struct clocksource cs; + struct rk_timer timer; +}; + static struct rk_clock_event_device bc_timer; +static struct rk_clocksource cs_timer; static inline struct rk_clock_event_device* rk_clock_event_device(struct clock_event_device *ce) @@ -139,14 +146,35 @@ static irqreturn_t rk_timer_interrupt(int irq, void *dev_id) return IRQ_HANDLED; } +static cycle_t rk_timer_clocksource_read(struct clocksource *cs) +{ + struct rk_clocksource *_cs = container_of(cs, struct rk_clocksource, cs); + return ~rk_timer_counter_read(&_cs->timer); +} + +static u64 notrace rk_timer_sched_clock_read(void) +{ + struct rk_clocksource *_cs = &cs_timer; + return ~rk_timer_counter_read(&_cs->timer); +} + static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg) { - struct clock_event_device *ce = &bc_timer.ce; - struct rk_timer *timer = &bc_timer.timer; + struct clock_event_device *ce = NULL; + struct clocksource *cs = NULL; + struct rk_timer *timer = NULL; struct clk *timer_clk; struct clk *pclk; int ret = -EINVAL, irq; + if (of_property_read_bool(np, "rockchip,clocksource")) { + cs = &cs_timer.cs; + timer = &cs_timer.timer; + } else { + ce = &bc_timer.ce; + timer = &bc_timer.timer; + } + timer->base = of_iomap(np, 0); if (!timer->base) { pr_err("Failed to get base address for '%s'\n", TIMER_NAME); @@ -189,26 +217,45 @@ static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg) goto out_irq; } - ce->name = TIMER_NAME; - ce->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT | - CLOCK_EVT_FEAT_DYNIRQ; - ce->set_next_event = rk_timer_set_next_event; - ce->set_state_shutdown = rk_timer_shutdown; - ce->set_state_periodic = rk_timer_set_periodic; - ce->irq = irq; - ce->cpumask = cpu_possible_mask; - ce->rating = 250; + if (ce) { + ce->name = TIMER_NAME; + ce->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT | + CLOCK_EVT_FEAT_DYNIRQ; + ce->set_next_event = rk_timer_set_next_event; + ce->set_state_shutdown = rk_timer_shutdown; + ce->set_state_periodic = rk_timer_set_periodic; + ce->irq = irq; + ce->cpumask = cpu_possible_mask; + ce->rating = 250; + } + + if (cs) { + cs->name = TIMER_NAME; + cs->flags = CLOCK_SOURCE_IS_CONTINUOUS; + cs->mask = CLOCKSOURCE_MASK(64); + cs->read = rk_timer_clocksource_read; + cs->rating = 350; + } rk_timer_interrupt_clear(timer); rk_timer_disable(timer); - ret = request_irq(irq, rk_timer_interrupt, IRQF_TIMER, TIMER_NAME, ce); - if (ret) { - pr_err("Failed to initialize '%s': %d\n", TIMER_NAME, ret); - goto out_irq; + if (ce) { + ret = request_irq(irq, rk_timer_interrupt, IRQF_TIMER, TIMER_NAME, ce); + if (ret) { + pr_err("Failed to initialize '%s': %d\n", TIMER_NAME, ret); + goto out_irq; + } + + clockevents_config_and_register(ce, timer->freq, 1, UINT_MAX); } - clockevents_config_and_register(ce, timer->freq, 1, UINT_MAX); + if (cs) { + rk_timer_update_counter(U64_MAX, timer); + rk_timer_enable(timer, 0); + clocksource_register_hz(cs, timer->freq); + sched_clock_register(rk_timer_sched_clock_read, 64, timer->freq); + } return 0;
The patch implement stable clocksource for rk3188. It register one of the timers as clocksource and sched_clock. Also it override arm_global_timer clocksource using rating property (350). arm_global_timer enabled on rk3188 provide unstable clocksource. It's rate depend on ARM CPU rate. As result the command 'sleep 60' could run either ~60s (with CPU freq 312 MHZ) or ~45s (with CPU freq 1.6GHz). In order to use the patch you have to setup the timer using 'rockchip,clocksource' device tree property. timer6 was used as clocksource in kernel 3.0 from rockchip SDK. cpufreq-set -f 1.6GHZ date; sleep 60; date Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com> --- drivers/clocksource/rockchip_timer.c | 79 +++++++++++++++++++++++++++------- 1 file changed, 63 insertions(+), 16 deletions(-)