From: dinguyen at altera.com <hidden> Date: 2013-09-17 22:02:40
From: Dinh Nguyen <redacted>
The timer defines currently reside in dw_apb_timer.c, and
dw_apb_timer_of.c needs to make use of these defines as well.
Signed-off-by: Dinh Nguyen <redacted>
CC: Rob Herring <redacted>
CC: Arnd Bergmann <arnd@arndb.de>
Cc: Olof Johansson <redacted>
CC: Jamie Iles <redacted>
Cc: John Stultz <redacted>
Cc: Daniel Lezcano <redacted>
Cc: Thomas Gleixner <redacted>
Cc: Linus Walleij <redacted>
Cc: Pavel Machek <redacted>
Cc: Heiko Stuebner <heiko@sntech.de>
Cc: linux-arm-kernel at lists.infradead.org
---
v3:
- Explain WHY in the changelog.
v2:
- Remove the defines in dw_apb_timer.c
drivers/clocksource/dw_apb_timer.c | 19 -------------------
include/linux/dw_apb_timer.h | 19 +++++++++++++++++++
2 files changed, 19 insertions(+), 19 deletions(-)
From: dinguyen at altera.com <hidden> Date: 2013-09-17 22:02:41
From: Dinh Nguyen <redacted>
The read_sched_clock should return the ~value because the clock is a
countdown implementation. read_sched_clock() should be the same as
__apbt_read_clocksource().
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.
Maintain backwards compatibility for "dw-apb-timer-sp" and
"dw-apb-timer-osc".
Signed-off-by: Dinh Nguyen <redacted>
Acked-by: Jamie Iles <redacted>
Acked-by: Linus Walleij <redacted>
CC: Rob Herring <redacted>
CC: Arnd Bergmann <arnd@arndb.de>
Cc: Olof Johansson <redacted>
Cc: John Stultz <redacted>
Cc: Daniel Lezcano <redacted>
Cc: Thomas Gleixner <redacted>
Cc: Pavel Machek <redacted>
Cc: Heiko Stuebner <heiko@sntech.de>
Cc: linux-arm-kernel at lists.infradead.org
---
v3:
- Use APBTMR_N_CURRENT_VALUE define in read_sched_clock()
v2:
- Maintain backwards compatibility for "dw-apb-timer-sp" and
"dw-apb-timer-osc".
drivers/clocksource/dw_apb_timer_of.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
From: Thomas Gleixner <hidden> Date: 2013-09-17 22:42:36
On Tue, 17 Sep 2013, dinguyen at 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
From: Pavel Machek <hidden> Date: 2013-09-18 11:01:59
On Wed 2013-09-18 00:42:36, Thomas Gleixner wrote:
On Tue, 17 Sep 2013, dinguyen at altera.com wrote:
quoted
read_sched_clock() should be the same as __apbt_read_clocksource().
So why is it using a separate function in the first place?
quoted
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.
Am Mittwoch, 18. September 2013, 13:01:59 schrieb Pavel Machek:
On Wed 2013-09-18 00:42:36, Thomas Gleixner wrote:
quoted
On Tue, 17 Sep 2013, dinguyen at 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?
quoted
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:
quoted
On Wed 2013-09-18 00:42:36, Thomas Gleixner wrote:
quoted
On Tue, 17 Sep 2013, dinguyen at 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?
quoted
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
From: Daniel Lezcano <hidden> Date: 2013-10-07 16:28:32
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:
quoted
Am Mittwoch, 18. September 2013, 13:01:59 schrieb Pavel Machek:
quoted
On Wed 2013-09-18 00:42:36, Thomas Gleixner wrote:
quoted
On Tue, 17 Sep 2013, dinguyen at 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?
quoted
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
--
<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