Hi,

Suman Anna <s-a...@ti.com> writes:
>>>>> On 10/05/2015 02:48 PM, Balbi, Felipe wrote:
>>>>>> We actually want these devices to be created because
>>>>>> we will be moving timers to drivers/clocksource and
>>>>>> this will prevent them from probing.
>>>>>
>>>>> This logic is also used to remove the secure timer from being
>>>>> registered to the kernel on HS devices, while allowing the timer to be
>>>>> available on GP devices. Your patch actually would break that
>>>>> functionality. I suggest that you look at the history of the code that
>>>>
>>>> heh, that's a silly way of doing so. Could just detect if we're running
>>>> on HS or not.
>>>
>>> This function is invoked only after detecting that we are running on a
>>> HS device.
>> 
>> What actually disables the timer is omap_get_timer_dt() and that's
>> called in more than one place.
>
> Yes indeed, and this patch is changing the behavior of
> omap_get_timer_dt(), so you have to check all call-sites. Also, one
> another thing is that this trick was used for DT boots so that the
> timers used for clocksource and clockevent are eliminated from the
> omap_dm_timer_request() API. The legacy boot used a specific API to mark
> those as reserved so that the _request API does not give them out.
> Granted that it may not have been the best approach, but that needs a
> solution if you change the behavior of this API.

no doubt this needs a solution, but seesm like a solution here will have
to be incremental. See new revision of my series. I'm now skipping 32k
only and keeping it enabled so drivers/clocksource/ can use it.

>>>>>> Signed-off-by: Felipe Balbi <ba...@ti.com>
>>>>>> ---
>>>>>>
>>>>>> Tony, I wonder if you can get merged as a fix, or do you
>>>>>> prefer receiving it together with my timer series ?
>>>>>
>>>>> NAK for rc, as it breaks other stuff.
>>>>
>>>> a single stuff which is likely easy enough to fix. But fair enough
>>>
>>> Don't think this patch is fixing any critical bug either, better to club
>>> it with your cleanup series.
>> 
>> it is kinda critical when you consider that the timer is disabled
>> outside of the soc type detection.
>
> This has been like this since DMTimer is converted to DT (from 3.8
> kernel), and is a need for your counter32k clocksource series. The
> problem seems to stem from the reuse of omap_get_timer_dt for counter32k
> nodes. A better solution would be to remove the omap_get_timer_dt() from
> omap2_sync32k_clocksource_init() code and just replace it with
> of_find_compatible_node - you seem to be limiting the majority of that
> function to legacy mode in Patch 10 of your RFC series anyways.

I still need omap_hwmod_enable() to be called, that's why it's still
used. I don't think replacing it with of_find_compatible_node() will buy
us much, we will still need to check for of_device_is_available()
anyway. Sure we skip all the timer flags (alwon, dsp, pwm, secure), but
that really isn't big deal.

When converting gptimer to clocksource, then I'll look at what I can do
for the timer request side of things. For now, things are working,
apparently without regressions (or at least I couldn't catch any so far)
and it seems clean enough that it could possibly be queued for v4.4
merge window. For v4.5 I'll look at this again and try to figure out how
to handle gptimer.

-- 
balbi

Attachment: signature.asc
Description: PGP signature

Reply via email to