| Message ID | 1379455361-13882-2-git-send-email-dinguyen@altera.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 17 Sep 2013, dinguyen@altera.com wrote: > read_sched_clock() should be the same as __apbt_read_clocksource(). So why is it using a separate function in the first place? > If a separate timer for the sched_clock exist, then read_sched_clock() > will return an incorrect value. The (sched_io_base + 0x4) needs to be in > the function for both cases. It really took me some time to understand the "separate timer" part of the above explanation. Though that made me look at the actual implementation and I have to say, that this abuse of the device tree is really a unique masterpiece: static int num_called; static void __init dw_apb_timer_init(struct device_node *timer) { switch (num_called) { case 0: pr_debug("%s: found clockevent timer\n", __func__); add_clockevent(timer); of_node_put(timer); break; case 1: pr_debug("%s: found clocksource timer\n", __func__); add_clocksource(timer); of_node_put(timer); init_sched_clock(); break; default: break; } num_called++; } So if you can use different nodes for clockevent and clocksource, why is that supposed to be dependent on the ordering? That's not how DT is supposed to be used. DT provides a clear description of the hardware, not some ordering dependent magic amended by utterly useless pr_debug() constructs. Thanks, tglx
On Wed 2013-09-18 00:42:36, Thomas Gleixner wrote: > On Tue, 17 Sep 2013, dinguyen@altera.com wrote: > > > read_sched_clock() should be the same as __apbt_read_clocksource(). > > So why is it using a separate function in the first place? > > If a separate timer for the sched_clock exist, then read_sched_clock() > > will return an incorrect value. The (sched_io_base + 0x4) needs to be in > > the function for both cases. > > It really took me some time to understand the "separate timer" part of > the above explanation. Though that made me look at the actual > implementation and I have to say, that this abuse of the device tree > is really a unique masterpiece: Well. We did a patch to fix timers on socfpga. You merged it. Then it clashed with Heiko Stubner's work. Our version was dropped, his version contained this code you complain about. _I also complained about that code_. http://www.spinics.net/lists/arm-kernel/msg252732.html . Seems it was merged anyway. (2 months ago.) And now we can't get the code fixed so that it at least works on our hardware, because, guess what, you noticed upstream merged the gem below, and you don't like it? > static int num_called; > static void __init dw_apb_timer_init(struct device_node *timer) > { > switch (num_called) { > case 0: > pr_debug("%s: found clockevent timer\n", __func__); > add_clockevent(timer); > of_node_put(timer); > break; > case 1: > pr_debug("%s: found clocksource timer\n", __func__); > add_clocksource(timer); > of_node_put(timer); > init_sched_clock(); > break; > default: > break; > } > > num_called++; > } > > So if you can use different nodes for clockevent and clocksource, why > is that supposed to be dependent on the ordering? That's not how DT is > supposed to be used. DT provides a clear description of the hardware, > not some ordering dependent magic amended by utterly useless pr_debug() > constructs. You already had non-ugly version in your tree. Alternatively, tell us what you want done. These boards have 2 to 4 identical timers, that can serve as both clockevent and clocksource. We'd like to use one as clockevent and one as clocksource. Regards, Pavel
Am Mittwoch, 18. September 2013, 13:01:59 schrieb Pavel Machek: > On Wed 2013-09-18 00:42:36, Thomas Gleixner wrote: > > On Tue, 17 Sep 2013, dinguyen@altera.com wrote: > And now we can't get the code fixed so that it at least works on our > hardware, because, guess what, you noticed upstream merged the gem > below, and you don't like it? > > > static int num_called; > > static void __init dw_apb_timer_init(struct device_node *timer) > > { > > > > switch (num_called) { > > > > case 0: > > pr_debug("%s: found clockevent timer\n", __func__); > > add_clockevent(timer); > > of_node_put(timer); > > break; > > > > case 1: > > pr_debug("%s: found clocksource timer\n", __func__); > > add_clocksource(timer); > > of_node_put(timer); > > init_sched_clock(); > > break; > > > > default: > > break; > > > > } > > > > num_called++; > > > > } > > > > So if you can use different nodes for clockevent and clocksource, why > > is that supposed to be dependent on the ordering? That's not how DT is > > supposed to be used. DT provides a clear description of the hardware, > > not some ordering dependent magic amended by utterly useless pr_debug() > > constructs. > > You already had non-ugly version in your tree. > > Alternatively, tell us what you want done. These boards have 2 to 4 > identical timers, that can serve as both clockevent and > clocksource. We'd like to use one as clockevent and one as > clocksource. I would also be interested in the "right" way to do this. As Pavel already said, the hardware is identical for all N separate timer blocks, so as the DT should be describing the hardware only, there is no way to specifiy one for the clockevent and another for the clocksource there. At first I kept using the non-standard init which required it being called from platform code, but got the request to convert the driver to use CLOCKSOURCE_OF_DECLARE to remove the need for separate call. As you will know CLOCKSOURCE_OF_DECLARE calls the init function for each found dt node for a matching device, resulting in N calls to dw_apb_timer_init. So my solution was to just grab the first one as clockevent and second one as clocksource. Therefore I'm all ears for how to solve this in a better way :-) Heiko
Hi Thomas, On Wed, 2013-09-18 at 13:32 +0200, Heiko Stübner wrote: > Am Mittwoch, 18. September 2013, 13:01:59 schrieb Pavel Machek: > > On Wed 2013-09-18 00:42:36, Thomas Gleixner wrote: > > > On Tue, 17 Sep 2013, dinguyen@altera.com wrote: > > And now we can't get the code fixed so that it at least works on our > > hardware, because, guess what, you noticed upstream merged the gem > > below, and you don't like it? > > > > > static int num_called; > > > static void __init dw_apb_timer_init(struct device_node *timer) > > > { > > > > > > switch (num_called) { > > > > > > case 0: > > > pr_debug("%s: found clockevent timer\n", __func__); > > > add_clockevent(timer); > > > of_node_put(timer); > > > break; > > > > > > case 1: > > > pr_debug("%s: found clocksource timer\n", __func__); > > > add_clocksource(timer); > > > of_node_put(timer); > > > init_sched_clock(); > > > break; > > > > > > default: > > > break; > > > > > > } > > > > > > num_called++; > > > > > > } > > > > > > So if you can use different nodes for clockevent and clocksource, why > > > is that supposed to be dependent on the ordering? That's not how DT is > > > supposed to be used. DT provides a clear description of the hardware, > > > not some ordering dependent magic amended by utterly useless pr_debug() > > > constructs. > > > > You already had non-ugly version in your tree. > > > > Alternatively, tell us what you want done. These boards have 2 to 4 > > identical timers, that can serve as both clockevent and > > clocksource. We'd like to use one as clockevent and one as > > clocksource. > > I would also be interested in the "right" way to do this. > > As Pavel already said, the hardware is identical for all N separate timer > blocks, so as the DT should be describing the hardware only, there is no way > to specifiy one for the clockevent and another for the clocksource there. > > At first I kept using the non-standard init which required it being called from > platform code, but got the request to convert the driver to use > CLOCKSOURCE_OF_DECLARE to remove the need for separate call. > > As you will know CLOCKSOURCE_OF_DECLARE calls the init function for each found > dt node for a matching device, resulting in N calls to dw_apb_timer_init. > So my solution was to just grab the first one as clockevent and second one as > clocksource. > > Therefore I'm all ears for how to solve this in a better way :-) > I'm just wondering if you have gotten a chance to give this patch anymore thought? The state of the socfpga platform for 3.12 is that it will not boot without this patch(mainly because of a DTS binding change). This patch mainly only fixes that issue. If you would like dw_apb_timer_init() fix for 3.13, can you please give us advice, so that we can get started on it in time for 3.13? Thanks, Dinh > > Heiko >
On 09/23/2013 05:58 PM, Dinh Nguyen wrote: > Hi Thomas, > > On Wed, 2013-09-18 at 13:32 +0200, Heiko Stübner wrote: >> Am Mittwoch, 18. September 2013, 13:01:59 schrieb Pavel Machek: >>> On Wed 2013-09-18 00:42:36, Thomas Gleixner wrote: >>>> On Tue, 17 Sep 2013, dinguyen@altera.com wrote: >>> And now we can't get the code fixed so that it at least works on our >>> hardware, because, guess what, you noticed upstream merged the gem >>> below, and you don't like it? >>> >>>> static int num_called; >>>> static void __init dw_apb_timer_init(struct device_node *timer) >>>> { >>>> >>>> switch (num_called) { >>>> >>>> case 0: >>>> pr_debug("%s: found clockevent timer\n", __func__); >>>> add_clockevent(timer); >>>> of_node_put(timer); >>>> break; >>>> >>>> case 1: >>>> pr_debug("%s: found clocksource timer\n", __func__); >>>> add_clocksource(timer); >>>> of_node_put(timer); >>>> init_sched_clock(); >>>> break; >>>> >>>> default: >>>> break; >>>> >>>> } >>>> >>>> num_called++; >>>> >>>> } >>>> >>>> So if you can use different nodes for clockevent and clocksource, why >>>> is that supposed to be dependent on the ordering? That's not how DT is >>>> supposed to be used. DT provides a clear description of the hardware, >>>> not some ordering dependent magic amended by utterly useless pr_debug() >>>> constructs. >>> >>> You already had non-ugly version in your tree. >>> >>> Alternatively, tell us what you want done. These boards have 2 to 4 >>> identical timers, that can serve as both clockevent and >>> clocksource. We'd like to use one as clockevent and one as >>> clocksource. >> >> I would also be interested in the "right" way to do this. >> >> As Pavel already said, the hardware is identical for all N separate timer >> blocks, so as the DT should be describing the hardware only, there is no way >> to specifiy one for the clockevent and another for the clocksource there. >> >> At first I kept using the non-standard init which required it being called from >> platform code, but got the request to convert the driver to use >> CLOCKSOURCE_OF_DECLARE to remove the need for separate call. >> >> As you will know CLOCKSOURCE_OF_DECLARE calls the init function for each found >> dt node for a matching device, resulting in N calls to dw_apb_timer_init. >> So my solution was to just grab the first one as clockevent and second one as >> clocksource. >> >> Therefore I'm all ears for how to solve this in a better way :-) >> > > I'm just wondering if you have gotten a chance to give this patch > anymore thought? The state of the socfpga platform for 3.12 is that it > will not boot without this patch(mainly because of a DTS binding > change). This patch mainly only fixes that issue. > > If you would like dw_apb_timer_init() fix for 3.13, can you please give > us advice, so that we can get started on it in time for 3.13? Dinh, I second Thomas comment's about the initialization code in the driver vs dt-binding, IMO it is worth to investigate what could be the "right way", as said Heiko, to do it. I suspect this situation will occur again for some other drivers. Perhaps, Grant Likely or Rob Herring can give us some advices (cc'ed). Grant/Rob, if a timer device description gives several timers where the driver use one for the clocksource and the other one for the clockevent, the initialization code has to deal multiple init and find a way to register them properly. This is the reason of the code snippet above. As the device tree is supposed to do stricly a hardware description, do you have an advice from a device tree perspective to implement something better than the code above ? Similar situation occurs with the efm32 driver [1][2]. Thanks ! -- Daniel [1] http://www.spinics.net/lists/arm-kernel/msg275403.html (at the end) [2] http://www.spinics.net/lists/arm-kernel/msg277261.html
diff --git a/drivers/clocksource/dw_apb_timer_of.c b/drivers/clocksource/dw_apb_timer_of.c index 4cbae4f..01c1238 100644 --- a/drivers/clocksource/dw_apb_timer_of.c +++ b/drivers/clocksource/dw_apb_timer_of.c @@ -102,18 +102,17 @@ static void add_clocksource(struct device_node *source_timer) * timer is found. sched_io_base then points to the current_value * register of the clocksource timer. */ - sched_io_base = iobase + 0x04; + sched_io_base = iobase; sched_rate = rate; } static u32 read_sched_clock(void) { - return __raw_readl(sched_io_base); + return ~__raw_readl(sched_io_base + APBTMR_N_CURRENT_VALUE); } static const struct of_device_id sptimer_ids[] __initconst = { { .compatible = "picochip,pc3x2-rtc" }, - { .compatible = "snps,dw-apb-timer-sp" }, { /* Sentinel */ }, }; @@ -153,4 +152,6 @@ static void __init dw_apb_timer_init(struct device_node *timer) num_called++; } CLOCKSOURCE_OF_DECLARE(pc3x2_timer, "picochip,pc3x2-timer", dw_apb_timer_init); -CLOCKSOURCE_OF_DECLARE(apb_timer, "snps,dw-apb-timer-osc", dw_apb_timer_init); +CLOCKSOURCE_OF_DECLARE(apb_timer_osc, "snps,dw-apb-timer-osc", dw_apb_timer_init); +CLOCKSOURCE_OF_DECLARE(apb_timer_sp, "snps,dw-apb-timer-sp", dw_apb_timer_init); +CLOCKSOURCE_OF_DECLARE(apb_timer, "snps,dw-apb-timer", dw_apb_timer_init);