[PATCHv4 2/2] clocksource: dw_apb_timer_of: Fix read_sched_clock

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