On 18/02/2019 10:49, Marek Szyprowski wrote: > Hi Daniel, > > On 2019-02-18 10:24, Daniel Lezcano wrote: >> On 18/02/2019 08:41, Marek Szyprowski wrote: >>> 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). > > Maybe additional comment in the cpu hotplug list will convince you? For > the current code changing the order of enums it is more than enough. > There are no dynamic CPU hotplug priorities, so MCT driver cannot change > its behavior depending on any dt-property or so. Increasing MCT priority > has no side-effects in relation to other drivers (as they cannot > existing on the same board). It is Thomas's call, I will let him decide if it is ok . >> 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@xxxxxxxxxxx> >>>>> --- >>>>> 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
