Re: [PATCH v2 3/6] clocksource: exynos_mct: Add arch_timer cooperation mode for ARM64

Chanwoo Choi
2018-10-17T05:01:04Z

Hi Marek,

I tested it about kernel booting and CPU hotplug through sysfs path
on ARM64 Exynos5433-based TM2 board. It is well working.

Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
Tested-by: Chanwoo Choi <cw00.choi@samsung.com>

But,
I have a minor comment for code clean-up.
On exynos4_mct_starting_cpu() function, the initialization of 'evt' instance
are not mandatory because 'evt' instance is not registered
by clockevents_config_and_register().

On 2018년 10월 15일 21:31, Marek Szyprowski wrote:
> To get ARM Architected Timers working on Samsung Exynos SoCs, one has to
> first configure and enable Exynos Multi-Core Timer, because they both
> share some common hardware blocks. This patch adds a mode of cooperation
> with arch_timer driver, so kernel can use CP15 based timer interface via
> arch_timer driver, which is mandatory on ARM64. In such mode driver only
> configures MCT registers and starts the timer but don't register any
> clocksource or events in the system. Those are left to be handled by
> arch_timer driver.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
> drivers/clocksource/exynos_mct.c | 52 +++++++++++++++++++++-----------
> 1 file changed, 35 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
> index a379f11fad2d..06cd30a6d59a 100644
> --- a/drivers/clocksource/exynos_mct.c
> +++ b/drivers/clocksource/exynos_mct.c
> @@ -57,6 +57,7 @@
> #define TICK_BASE_CNT 1
>
> enum {
> + MCT_INT_NONE = 0,
> MCT_INT_SPI,
> MCT_INT_PPI
> };
> @@ -238,6 +239,9 @@ static int __init exynos4_clocksource_init(void)
> {
> exynos4_mct_frc_start();
>
> + if (!mct_int_type)
> + return 0;
> +
> #if defined(CONFIG_ARM)
> exynos4_delay_timer.read_current_timer = &exynos4_read_current_timer;
> exynos4_delay_timer.freq = clk_rate;
> @@ -343,6 +347,9 @@ static struct irqaction mct_comp_event_irq = {
>
> static int exynos4_clockevent_init(void)
> {
> + if (!mct_int_type)
> + return 0;
> +
> mct_comp_device.cpumask = cpumask_of(0);
> clockevents_config_and_register(&mct_comp_device, clk_rate,
> 0xf, 0xffffffff);
> @@ -476,12 +483,12 @@ static int exynos4_mct_starting_cpu(unsigned int cpu)
>
> irq_force_affinity(evt->irq, cpumask_of(cpu));
> enable_irq(evt->irq);
> - } else {
> + } else if (mct_int_type == MCT_INT_PPI) {
> enable_percpu_irq(mct_irqs[MCT_L0_IRQ], 0);
> }
> - clockevents_config_and_register(evt, clk_rate / (TICK_BASE_CNT + 1),
> - 0xf, 0x7fffffff);
> -
> + if (mct_int_type)
> + clockevents_config_and_register(evt,
> + clk_rate / (TICK_BASE_CNT + 1), 0xf, 0x7fffffff);
> return 0;
> }
>
> @@ -496,7 +503,7 @@ static int exynos4_mct_dying_cpu(unsigned int cpu)
> if (evt->irq != -1)
> disable_irq_nosync(evt->irq);
> exynos4_mct_write(0x1, mevt->base + MCT_L_INT_CSTAT_OFFSET);
> - } else {
> + } else if (mct_int_type == MCT_INT_PPI) {
> disable_percpu_irq(mct_irqs[MCT_L0_IRQ]);
> }
> return 0;
> @@ -529,7 +536,7 @@ static int __init exynos4_timer_resources(struct device_node *np, void __iomem *
> &percpu_mct_tick);
> WARN(err, "MCT: can't request IRQ %d (%d)\n",
> mct_irqs[MCT_L0_IRQ], err);
> - } else {
> + } else if (mct_int_type == MCT_INT_SPI) {
> for_each_possible_cpu(cpu) {
> int mct_irq = mct_irqs[MCT_L0_IRQ + cpu];
> struct mct_clock_event_device *pcpu_mevt =
> @@ -564,7 +571,7 @@ static int __init exynos4_timer_resources(struct device_node *np, void __iomem *
> out_irq:
> if (mct_int_type == MCT_INT_PPI) {
> free_percpu_irq(mct_irqs[MCT_L0_IRQ], &percpu_mct_tick);
> - } else {
> + } else if (mct_int_type == MCT_INT_SPI) {
> for_each_possible_cpu(cpu) {
> struct mct_clock_event_device *pcpu_mevt =
> per_cpu_ptr(&percpu_mct_tick, cpu);
> @@ -585,17 +592,28 @@ static int __init mct_init_dt(struct device_node *np, unsigned int int_type)
>
> mct_int_type = int_type;
>
> - /* This driver uses only one global timer interrupt */
> - mct_irqs[MCT_G0_IRQ] = irq_of_parse_and_map(np, MCT_G0_IRQ);
> + if (IS_ENABLED(CONFIG_ARM64) && IS_ENABLED(CONFIG_ARM_ARCH_TIMER)) {
> + struct device_node *np = of_find_compatible_node(NULL, NULL,
> + "arm,armv8-timer");
> + if (np) {
> + mct_int_type = MCT_INT_NONE;
> + of_node_put(np);
> + }
> + }
>
> - /*
> - * Find out the number of local irqs specified. The local
> - * timer irqs are specified after the four global timer
> - * irqs are specified.
> - */
> - nr_irqs = of_irq_count(np);
> - for (i = MCT_L0_IRQ; i < nr_irqs; i++)
> - mct_irqs[i] = irq_of_parse_and_map(np, i);
> + if (mct_int_type) {
> + /* This driver uses only one global timer interrupt */
> + mct_irqs[MCT_G0_IRQ] = irq_of_parse_and_map(np, MCT_G0_IRQ);
> +
> + /*
> + * Find out the number of local irqs specified. The local
> + * timer irqs are specified after the four global timer
> + * irqs are specified.
> + */
> + nr_irqs = of_irq_count(np);
> + for (i = MCT_L0_IRQ; i < nr_irqs; i++)
> + mct_irqs[i] = irq_of_parse_and_map(np, i);
> + }
>
> ret = exynos4_timer_resources(np, of_iomap(np, 0));
> if (ret)
>

--
Best Regards,
Chanwoo Choi
Samsung Electronics