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

[Daniel Lezcano]

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 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?
>>>
>>>> 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