Re: [PATCH 3/5] clocksource: exynos_mct: Increase priority over ARM arch timer

From: Daniel Lezcano <daniel.lezcano@linaro.org>
To: Marek Szyprowski <m.szyprowski@samsung.com>,
	linux-samsung-soc@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org
Cc: Chanwoo Choi <cw00.choi@samsung.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Krzysztof Kozlowski <krzk@kernel.org>,
	Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Subject: Re: [PATCH 3/5] clocksource: exynos_mct: Increase priority over ARM arch timer
Date: Mon, 18 Feb 2019 10:24:48 +0100	[thread overview]
Message-ID: <4b054e7d-5dd5-5796-6c08-22f15060f940@linaro.org> (raw)
In-Reply-To: <7b994b86-9a84-39a7-e88d-203c64b30de8@samsung.com>

On 18/02/2019 08:41, Marek Szyprowski wrote:
> Hi Daniel,
> 
> On 2019-02-15 17:35, Daniel Lezcano wrote:
>> On 15/02/2019 13:52, Marek Szyprowski wrote:
>>> Exynos Multi-Core Timer driver (exynos_mct) must be started before ARM
>>> Architected Timers (arch_timer), because they both share some common
>>> hardware blocks (global system counter) and turning on MCT is needed
>>> to get ARM Architected Timer working properly. Increase MCT timer rating
>>> and hotplug priority over ARM Archictected timer driver to achieve that.
>> This is a hack.
>>
>> There are mechanisms to handle this kind of dependency.
>>
>> eg. https://www.kernel.org/doc/html/v4.15/driver-api/device_link.html
> 
> Sorry, but this is not a hack, but proper way to handle this in
> clocksource/timers framework. The ratings assigned to each driver and
> cpu hotplug priority list are exactly for managing the requested order
> of operations if more than one driver has been registered.

Actually, I'm reluctant with this change (even if it is correct) because
it introduces an implicit dependency in the cpu hotplug enumeration for
the timer subsystem while it may be more correct to have it explicitly
with the hardware description (or whatever).

However I don't see a proper way to describe this dependency without
touching the DT and add a phandle to the arch timer in the exynos_mct
timer description.

> Device links operates on the 'kernel device' objects, which are created
> and managed much later than clocksource/timers are initialized. I see no
> use of device links in this context.

Yes, that was just an example to show there are mechanisms to allow
proper ordering. As said before, not the best example for this situation.

>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>> ---
>>>  drivers/clocksource/exynos_mct.c | 4 ++--
>>>  include/linux/cpuhotplug.h       | 2 +-
>>>  2 files changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
>>> index 49413900b24c..58090f57ada3 100644
>>> --- a/drivers/clocksource/exynos_mct.c
>>> +++ b/drivers/clocksource/exynos_mct.c
>>> @@ -211,7 +211,7 @@ static void exynos4_frc_resume(struct clocksource *cs)
>>>  
>>>  static struct clocksource mct_frc = {
>>>  	.name		= "mct-frc",
>>> -	.rating		= 400,
>>> +	.rating		= 450,
>>>  	.read		= exynos4_frc_read,
>>>  	.mask		= CLOCKSOURCE_MASK(32),
>>>  	.flags		= CLOCK_SOURCE_IS_CONTINUOUS,
>>> @@ -465,7 +465,7 @@ static int exynos4_mct_starting_cpu(unsigned int cpu)
>>>  	evt->set_state_oneshot_stopped = set_state_shutdown;
>>>  	evt->tick_resume = set_state_shutdown;
>>>  	evt->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT;
>>> -	evt->rating = 450;
>>> +	evt->rating = 500;
>>>  
>>>  	exynos4_mct_write(TICK_BASE_CNT, mevt->base + MCT_L_TCNTB_OFFSET);
>>>  
>>> diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
>>> index fd586d0301e7..bdd544f21102 100644
>>> --- a/include/linux/cpuhotplug.h
>>> +++ b/include/linux/cpuhotplug.h
>>> @@ -115,10 +115,10 @@ enum cpuhp_state {
>>>  	CPUHP_AP_PERF_ARM_ACPI_STARTING,
>>>  	CPUHP_AP_PERF_ARM_STARTING,
>>>  	CPUHP_AP_ARM_L2X0_STARTING,
>>> +	CPUHP_AP_EXYNOS4_MCT_TIMER_STARTING,
>>>  	CPUHP_AP_ARM_ARCH_TIMER_STARTING,
>>>  	CPUHP_AP_ARM_GLOBAL_TIMER_STARTING,
>>>  	CPUHP_AP_JCORE_TIMER_STARTING,
>>> -	CPUHP_AP_EXYNOS4_MCT_TIMER_STARTING,
>>>  	CPUHP_AP_ARM_TWD_STARTING,
>>>  	CPUHP_AP_QCOM_TIMER_STARTING,
>>>  	CPUHP_AP_ARMADA_TIMER_STARTING,
>>>
>>
> Best regards
> 

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel