Exynos MCT version 2 is composed of 1 FRC and 12 comparators.
There are no global timer and local timer anymore.
The 1 of 64bit FRC serves as "up-counter"(not "comparators").
The 12 comaprators(not "counter") can be used as per-cpu event timer
so that it can support upto 12 cores.
And a RTC source can be used as backup clock source.
Signed-off-by: Youngmin Nam <[email protected]>
---
drivers/clocksource/Kconfig | 7 +
drivers/clocksource/Makefile | 1 +
drivers/clocksource/exynos_mct_v2.c | 298 ++++++++++++++++++++++++++++
drivers/clocksource/exynos_mct_v2.h | 71 +++++++
4 files changed, 377 insertions(+)
create mode 100644 drivers/clocksource/exynos_mct_v2.c
create mode 100644 drivers/clocksource/exynos_mct_v2.h
diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 0f5e3983951a..4d8f62ef1c7f 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -421,6 +421,13 @@ config CLKSRC_EXYNOS_MCT
help
Support for Multi Core Timer controller on Exynos SoCs.
+config CLKSRC_EXYNOS_MCT_V2
+ bool "Exynos multi core timer (ver 2) driver" if COMPILE_TEST
+ depends on ARM64
+ depends on ARCH_EXYNOS
+ help
+ Support for Multi Core Timer controller on Exynos SoCs.
+
config CLKSRC_SAMSUNG_PWM
bool "PWM timer driver for Samsung S3C, S5P" if COMPILE_TEST
depends on HAS_IOMEM
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index c17ee32a7151..dc7d5cf27516 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -43,6 +43,7 @@ obj-$(CONFIG_CADENCE_TTC_TIMER) += timer-cadence-ttc.o
obj-$(CONFIG_CLKSRC_STM32) += timer-stm32.o
obj-$(CONFIG_CLKSRC_STM32_LP) += timer-stm32-lp.o
obj-$(CONFIG_CLKSRC_EXYNOS_MCT) += exynos_mct.o
+obj-$(CONFIG_CLKSRC_EXYNOS_MCT_V2) += exynos_mct_v2.o
obj-$(CONFIG_CLKSRC_LPC32XX) += timer-lpc32xx.o
obj-$(CONFIG_CLKSRC_MPS2) += mps2-timer.o
obj-$(CONFIG_CLKSRC_SAMSUNG_PWM) += samsung_pwm_timer.o
diff --git a/drivers/clocksource/exynos_mct_v2.c b/drivers/clocksource/exynos_mct_v2.c
new file mode 100644
index 000000000000..a61fcdf9e6c9
--- /dev/null
+++ b/drivers/clocksource/exynos_mct_v2.c
@@ -0,0 +1,298 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2022 Samsung Electronics Co., Ltd.
+ * http://www.samsung.com
+ *
+ * Exynos MCT(Multi-Core Timer) version 2 support
+ */
+
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/err.h>
+#include <linux/clk.h>
+#include <linux/clockchips.h>
+#include <linux/cpu.h>
+#include <linux/delay.h>
+#include <linux/percpu.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
+#include <linux/clocksource.h>
+#include "exynos_mct_v2.h"
+
+static void __iomem *reg_base;
+static unsigned long osc_clk_rate;
+static int mct_irqs[MCT_NR_COMPS];
+
+static void exynos_mct_set_compensation(unsigned long osc, unsigned long rtc)
+{
+ unsigned int osc_rtc;
+ unsigned int incr_rtcclk;
+ unsigned int compen_val;
+
+ osc_rtc = (unsigned int)(osc * 1000 / rtc);
+
+ /* MCT_INCR_RTCCLK is integer part of (OSCCLK frequency/RTCCLK frequency). */
+ incr_rtcclk = (osc / rtc) + ((osc % rtc) ? 1 : 0);
+
+ /* MCT_COMPENSATE_VALUE is decimal part of (OSCCLK frequency/RTCCLK frequency). */
+ compen_val = ((osc_rtc + 5) / 10) % 100;
+ if (compen_val)
+ compen_val = 100 - compen_val;
+
+ pr_info("exynos-mct-v2: osc-%lu rtc-%lu incr_rtcclk:0x%08x compen_val:0x%08x\n",
+ osc, rtc, incr_rtcclk, compen_val);
+
+ writel_relaxed(incr_rtcclk, reg_base + EXYNOS_MCT_MCT_INCR_RTCCLK);
+ writel_relaxed(compen_val, reg_base + EXYNOS_MCT_COMPENSATE_VALUE);
+}
+
+/* Clocksource handling */
+static void exynos_mct_frc_start(void)
+{
+ writel_relaxed(MCT_FRC_ENABLE, reg_base + EXYNOS_MCT_MCT_FRC_ENABLE);
+}
+
+static void exynos_mct_comp_stop(struct mct_clock_event_device *mevt)
+{
+ unsigned int index = mevt->comp_index;
+ unsigned int comp_enable;
+ unsigned int loop_cnt = 0;
+
+ writel_relaxed(MCT_COMP_DISABLE, reg_base + EXYNOS_MCT_COMP_ENABLE(index));
+
+ /* Wait maximum 1 ms until COMP_ENABLE_n = 0 */
+ do {
+ comp_enable = readl_relaxed(reg_base + EXYNOS_MCT_COMP_ENABLE(index));
+ loop_cnt++;
+ } while (comp_enable != MCT_COMP_DISABLE && loop_cnt < WAIT_LOOP_CNT);
+
+ if (loop_cnt == WAIT_LOOP_CNT)
+ panic("MCT(comp%d) disable timeout\n", index);
+
+ writel_relaxed(MCT_COMP_NON_CIRCULAR_MODE, reg_base + EXYNOS_MCT_COMP_MODE(index));
+ writel_relaxed(MCT_INT_DISABLE, reg_base + EXYNOS_MCT_INT_ENB(index));
+ writel_relaxed(MCT_CSTAT_CLEAR, reg_base + EXYNOS_MCT_INT_CSTAT(index));
+}
+
+static void exynos_mct_comp_start(struct mct_clock_event_device *mevt,
+ bool periodic, unsigned long cycles)
+{
+ unsigned int index = mevt->comp_index;
+ unsigned int comp_enable;
+ unsigned int loop_cnt = 0;
+
+ comp_enable = readl_relaxed(reg_base + EXYNOS_MCT_COMP_ENABLE(index));
+ if (comp_enable == MCT_COMP_ENABLE)
+ exynos_mct_comp_stop(mevt);
+
+ if (periodic)
+ writel_relaxed(MCT_COMP_CIRCULAR_MODE, reg_base + EXYNOS_MCT_COMP_MODE(index));
+
+ writel_relaxed(cycles, reg_base + EXYNOS_MCT_COMP_PERIOD(index));
+ writel_relaxed(MCT_INT_ENABLE, reg_base + EXYNOS_MCT_INT_ENB(index));
+ writel_relaxed(MCT_COMP_ENABLE, reg_base + EXYNOS_MCT_COMP_ENABLE(index));
+
+ /* Wait maximum 1 ms until COMP_ENABLE_n = 1 */
+ do {
+ comp_enable = readl_relaxed(reg_base + EXYNOS_MCT_COMP_ENABLE(index));
+ loop_cnt++;
+ } while (comp_enable != MCT_COMP_ENABLE && loop_cnt < WAIT_LOOP_CNT);
+
+ if (loop_cnt == WAIT_LOOP_CNT)
+ panic("MCT(comp%d) enable timeout\n", index);
+}
+
+static int exynos_comp_set_next_event(unsigned long cycles, struct clock_event_device *evt)
+{
+ struct mct_clock_event_device *mevt;
+
+ mevt = container_of(evt, struct mct_clock_event_device, evt);
+
+ exynos_mct_comp_start(mevt, false, cycles);
+
+ return 0;
+}
+
+static int mct_set_state_shutdown(struct clock_event_device *evt)
+{
+ struct mct_clock_event_device *mevt;
+
+ mevt = container_of(evt, struct mct_clock_event_device, evt);
+
+ exynos_mct_comp_stop(mevt);
+
+ return 0;
+}
+
+static int mct_set_state_periodic(struct clock_event_device *evt)
+{
+ unsigned long cycles_per_jiffy;
+ struct mct_clock_event_device *mevt;
+
+ mevt = container_of(evt, struct mct_clock_event_device, evt);
+
+ cycles_per_jiffy = (((unsigned long long)NSEC_PER_SEC / HZ * evt->mult) >> evt->shift);
+ exynos_mct_comp_start(mevt, true, cycles_per_jiffy);
+
+ return 0;
+}
+
+static irqreturn_t exynos_mct_comp_isr(int irq, void *dev_id)
+{
+ struct mct_clock_event_device *mevt = dev_id;
+ struct clock_event_device *evt = &mevt->evt;
+ unsigned int index = mevt->comp_index;
+
+ writel_relaxed(MCT_CSTAT_CLEAR, reg_base + EXYNOS_MCT_INT_CSTAT(index));
+
+ evt->event_handler(evt);
+
+ return IRQ_HANDLED;
+}
+
+static DEFINE_PER_CPU(struct mct_clock_event_device, percpu_mct_tick);
+
+static int exynos_mct_starting_cpu(unsigned int cpu)
+{
+ struct mct_clock_event_device *mevt = per_cpu_ptr(&percpu_mct_tick, cpu);
+ struct clock_event_device *evt = &mevt->evt;
+
+ snprintf(mevt->name, sizeof(mevt->name), "mct_comp%d", cpu);
+
+ evt->name = mevt->name;
+ evt->cpumask = cpumask_of(cpu);
+ evt->set_next_event = exynos_comp_set_next_event;
+ evt->set_state_periodic = mct_set_state_periodic;
+ evt->set_state_shutdown = mct_set_state_shutdown;
+ evt->set_state_oneshot = mct_set_state_shutdown;
+ evt->set_state_oneshot_stopped = mct_set_state_shutdown;
+ evt->tick_resume = mct_set_state_shutdown;
+ evt->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT;
+ evt->rating = 500; /* use value higher than ARM arch timer */
+
+ if (evt->irq == -1)
+ return -EIO;
+
+ irq_force_affinity(evt->irq, cpumask_of(cpu));
+ enable_irq(evt->irq);
+ clockevents_config_and_register(evt, osc_clk_rate, 0xf, 0x7fffffff);
+
+ return 0;
+}
+
+static int exynos_mct_dying_cpu(unsigned int cpu)
+{
+ struct mct_clock_event_device *mevt = per_cpu_ptr(&percpu_mct_tick, cpu);
+ struct clock_event_device *evt = &mevt->evt;
+ unsigned int index = mevt->comp_index;
+
+ evt->set_state_shutdown(evt);
+ if (evt->irq != -1)
+ disable_irq_nosync(evt->irq);
+
+ writel_relaxed(MCT_CSTAT_CLEAR, reg_base + EXYNOS_MCT_INT_CSTAT(index));
+
+ return 0;
+}
+
+static int __init exynos_timer_resources(struct device_node *np, void __iomem *base)
+{
+ int err, cpu;
+
+ struct clk *mct_clk, *tick_clk, *rtc_clk;
+ unsigned long rtc_clk_rate;
+ int div;
+ int ret;
+
+ ret = of_property_read_u32(np, "div", &div);
+ if (ret || !div) {
+ pr_warn("MCT: fail to get the div value. set div to the default\n");
+ div = DEFAULT_CLK_DIV;
+ }
+
+ tick_clk = of_clk_get_by_name(np, "fin_pll");
+ if (IS_ERR(tick_clk))
+ panic("%s: unable to determine tick clock rate\n", __func__);
+ osc_clk_rate = clk_get_rate(tick_clk) / div;
+
+ mct_clk = of_clk_get_by_name(np, "mct");
+ if (IS_ERR(mct_clk))
+ panic("%s: unable to retrieve mct clock instance\n", __func__);
+ clk_prepare_enable(mct_clk);
+
+ rtc_clk = of_clk_get_by_name(np, "rtc");
+ if (IS_ERR(rtc_clk)) {
+ pr_warn("MCT: fail to get rtc clock. set to the default\n");
+ rtc_clk_rate = DEFAULT_RTC_CLK_RATE;
+ } else {
+ rtc_clk_rate = clk_get_rate(rtc_clk);
+ }
+
+ reg_base = base;
+ if (!reg_base)
+ panic("%s: unable to ioremap mct address space\n", __func__);
+
+ exynos_mct_set_compensation(osc_clk_rate, rtc_clk_rate);
+ exynos_mct_frc_start();
+
+ for_each_possible_cpu(cpu) {
+ int mct_irq = mct_irqs[cpu];
+ struct mct_clock_event_device *pcpu_mevt = per_cpu_ptr(&percpu_mct_tick, cpu);
+
+ pcpu_mevt->evt.irq = -1;
+ pcpu_mevt->comp_index = cpu;
+
+ irq_set_status_flags(mct_irq, IRQ_NOAUTOEN);
+ if (request_irq(mct_irq,
+ exynos_mct_comp_isr,
+ IRQF_TIMER | IRQF_NOBALANCING | IRQF_PERCPU,
+ "exynos-mct", pcpu_mevt)) {
+ pr_err("exynos-mct: cannot register IRQ (cpu%d)\n", cpu);
+ continue;
+ }
+ pcpu_mevt->evt.irq = mct_irq;
+ }
+
+ /* Install hotplug callbacks which configure the timer on this CPU */
+ err = cpuhp_setup_state(CPUHP_AP_EXYNOS4_MCT_TIMER_STARTING,
+ "clockevents/exynos/mct_timer_v2:starting",
+ exynos_mct_starting_cpu,
+ exynos_mct_dying_cpu);
+ if (err)
+ goto out_irq;
+
+ return 0;
+
+out_irq:
+ for_each_possible_cpu(cpu) {
+ struct mct_clock_event_device *pcpu_mevt = per_cpu_ptr(&percpu_mct_tick, cpu);
+
+ if (pcpu_mevt->evt.irq != -1) {
+ free_irq(pcpu_mevt->evt.irq, pcpu_mevt);
+ pcpu_mevt->evt.irq = -1;
+ }
+ }
+ return err;
+}
+
+static int __init mct_init_dt(struct device_node *np)
+{
+ u32 nr_irqs = 0, i;
+ int ret;
+
+ /*
+ * Find out the total number of irqs which can be produced by comparators.
+ */
+ nr_irqs = of_irq_count(np);
+
+ for (i = MCT_COMP0; i < nr_irqs; i++)
+ mct_irqs[i] = irq_of_parse_and_map(np, i);
+
+ pr_info("exynos-mct-v2: initializing timer resources\n");
+ ret = exynos_timer_resources(np, of_iomap(np, 0));
+
+ return ret;
+}
+
+TIMER_OF_DECLARE(s5e99xx, "samsung,s5e99xx-mct", mct_init_dt);
diff --git a/drivers/clocksource/exynos_mct_v2.h b/drivers/clocksource/exynos_mct_v2.h
new file mode 100644
index 000000000000..59b3675b86a7
--- /dev/null
+++ b/drivers/clocksource/exynos_mct_v2.h
@@ -0,0 +1,71 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2022 Samsung Electronics Co., Ltd.
+ * http://www.samsung.com
+ *
+ * Exynos MCT(Multi-Core Timer) version 2 support
+ */
+
+#ifndef __EXYNOS_MCT_V2_H__
+#define __EXYNOS_MCT_V2_H__
+
+#define EXYNOS_MCTREG(x) (x)
+#define EXYNOS_MCT_MCT_CFG EXYNOS_MCTREG(0x000)
+#define EXYNOS_MCT_MCT_INCR_RTCCLK EXYNOS_MCTREG(0x004)
+#define EXYNOS_MCT_MCT_FRC_ENABLE EXYNOS_MCTREG(0x100)
+#define EXYNOS_MCT_CNT_L EXYNOS_MCTREG(0x110)
+#define EXYNOS_MCT_CNT_U EXYNOS_MCTREG(0x114)
+#define EXYNOS_MCT_CLKMUX_SEL EXYNOS_MCTREG(0x120)
+#define EXYNOS_MCT_COMPENSATE_VALUE EXYNOS_MCTREG(0x124)
+#define EXYNOS_MCT_VER EXYNOS_MCTREG(0x128)
+#define EXYNOS_MCT_DIVCHG_ACK EXYNOS_MCTREG(0x12C)
+#define EXYNOS_MCT_COMP_L(i) EXYNOS_MCTREG(0x200 + ((i) * 0x100))
+#define EXYNOS_MCT_COMP_U(i) EXYNOS_MCTREG(0x204 + ((i) * 0x100))
+#define EXYNOS_MCT_COMP_MODE(i) EXYNOS_MCTREG(0x208 + ((i) * 0x100))
+#define EXYNOS_MCT_COMP_PERIOD(i) EXYNOS_MCTREG(0x20C + ((i) * 0x100))
+#define EXYNOS_MCT_COMP_ENABLE(i) EXYNOS_MCTREG(0x210 + ((i) * 0x100))
+#define EXYNOS_MCT_INT_ENB(i) EXYNOS_MCTREG(0x214 + ((i) * 0x100))
+#define EXYNOS_MCT_INT_CSTAT(i) EXYNOS_MCTREG(0x218 + ((i) * 0x100))
+
+#define MCT_FRC_ENABLE (0x1)
+#define MCT_COMP_ENABLE (0x1)
+#define MCT_COMP_DISABLE (0x0)
+
+#define MCT_COMP_CIRCULAR_MODE (0x1)
+#define MCT_COMP_NON_CIRCULAR_MODE (0x0)
+
+#define MCT_INT_ENABLE (0x1)
+#define MCT_INT_DISABLE (0x0)
+
+#define MCT_CSTAT_CLEAR (0x1)
+
+#define DEFAULT_RTC_CLK_RATE 32768
+#define DEFAULT_CLK_DIV 3
+
+#define WAIT_LOOP_CNT (loops_per_jiffy / 1000 * HZ)
+
+enum {
+ /* There are 12 comparators which can produce interrupts */
+ MCT_COMP0,
+ MCT_COMP1,
+ MCT_COMP2,
+ MCT_COMP3,
+ MCT_COMP4,
+ MCT_COMP5,
+ MCT_COMP6,
+ MCT_COMP7,
+ MCT_COMP8,
+ MCT_COMP9,
+ MCT_COMP10,
+ MCT_COMP11,
+
+ MCT_NR_COMPS,
+};
+
+struct mct_clock_event_device {
+ struct clock_event_device evt;
+ char name[10];
+ unsigned int comp_index;
+};
+
+#endif /* __EXYNOS_MCT_V2_H__ */
--
2.33.0
On Tue, Nov 02, 2021 at 09:11:21AM +0900, Youngmin Nam wrote:
> Exynos MCT version 2 is composed of 1 FRC and 12 comparators.
> There are no global timer and local timer anymore.
> The 1 of 64bit FRC serves as "up-counter"(not "comparators").
> The 12 comaprators(not "counter") can be used as per-cpu event timer
> so that it can support upto 12 cores.
> And a RTC source can be used as backup clock source.
[...]
> +static int exynos_mct_starting_cpu(unsigned int cpu)
> +{
> + struct mct_clock_event_device *mevt = per_cpu_ptr(&percpu_mct_tick, cpu);
> + struct clock_event_device *evt = &mevt->evt;
> +
> + snprintf(mevt->name, sizeof(mevt->name), "mct_comp%d", cpu);
> +
> + evt->name = mevt->name;
> + evt->cpumask = cpumask_of(cpu);
> + evt->set_next_event = exynos_comp_set_next_event;
> + evt->set_state_periodic = mct_set_state_periodic;
> + evt->set_state_shutdown = mct_set_state_shutdown;
> + evt->set_state_oneshot = mct_set_state_shutdown;
> + evt->set_state_oneshot_stopped = mct_set_state_shutdown;
> + evt->tick_resume = mct_set_state_shutdown;
> + evt->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT;
> + evt->rating = 500; /* use value higher than ARM arch timer */
Previously Will asked you to try CLOCK_EVT_FEAT_PERCPU here, and to set
the C3STOP flag on the arch timer via the DT when necessary, rather than
trying to override the arch timer like this:
https://lore.kernel.org/r/20211027073458.GA22231@willie-the-truck
There are a bunch of things that depend on the architected timer working
as a clocksource (e.g. vdso, kvm), and it *should* work as a lock
clockevent_device if configured correctly, and it's much more consistent
with *everyone else* to use the arhcitected timer by default.
Please try as Will suggested above, so that this works from day one.
Thanks,
Mark.
On 03/11/2021 01:09, Youngmin Nam wrote:
> On Tue, Nov 02, 2021 at 10:28:10AM +0000, Mark Rutland wrote:
>> On Tue, Nov 02, 2021 at 09:11:21AM +0900, Youngmin Nam wrote:
>>> Exynos MCT version 2 is composed of 1 FRC and 12 comparators.
>>> There are no global timer and local timer anymore.
>>> The 1 of 64bit FRC serves as "up-counter"(not "comparators").
>>> The 12 comaprators(not "counter") can be used as per-cpu event timer
>>> so that it can support upto 12 cores.
>>> And a RTC source can be used as backup clock source.
>>
>> [...]
>>
>>> +static int exynos_mct_starting_cpu(unsigned int cpu)
>>> +{
>>> + struct mct_clock_event_device *mevt = per_cpu_ptr(&percpu_mct_tick, cpu);
>>> + struct clock_event_device *evt = &mevt->evt;
>>> +
>>> + snprintf(mevt->name, sizeof(mevt->name), "mct_comp%d", cpu);
>>> +
>>> + evt->name = mevt->name;
>>> + evt->cpumask = cpumask_of(cpu);
>>> + evt->set_next_event = exynos_comp_set_next_event;
>>> + evt->set_state_periodic = mct_set_state_periodic;
>>> + evt->set_state_shutdown = mct_set_state_shutdown;
>>> + evt->set_state_oneshot = mct_set_state_shutdown;
>>> + evt->set_state_oneshot_stopped = mct_set_state_shutdown;
>>> + evt->tick_resume = mct_set_state_shutdown;
>>> + evt->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT;
>>> + evt->rating = 500; /* use value higher than ARM arch timer */
>>
>> Previously Will asked you to try CLOCK_EVT_FEAT_PERCPU here, and to set
>> the C3STOP flag on the arch timer via the DT when necessary, rather than
>> trying to override the arch timer like this:
>>
>> https://protect2.fireeye.com/v1/url?k=72526080-2dc9598b-7253ebcf-002590f5b904-ca603717c6462908&q=1&e=be56aa83-dbac-4639-913d-d388620fe3fc&u=https%3A%2F%2Flore.kernel.org%2Fr%2F20211027073458.GA22231%40willie-the-truck
>>
>> There are a bunch of things that depend on the architected timer working
>> as a clocksource (e.g. vdso, kvm), and it *should* work as a lock
>> clockevent_device if configured correctly, and it's much more consistent
>> with *everyone else* to use the arhcitected timer by default.
>>
>> Please try as Will suggested above, so that this works from day one.
>>
>> Thanks,
>> Mark.
>>
>
> Hi Mark.
> It looks like you missed my previous mail.
> https://lore.kernel.org/all/20211029035422.GA30523@perf/#t
>
> Yes, I believe Will's suggestion definitely will work.
> But that is for performance not functionality.
> As a driver for new H/W IP I would like to confirm functionality first.
>
> We need more time to test this feature with our exynos core power down feature.
> And we need to do a various regression test whether there is another corner case or not.
> So, how about we apply Will's suggetion later after the current patchset is merged first?
> After doing our regression test with our exynos core power down feature, we can confirm this.
>
Not really, because once it is merged there is no incentive to fix it or
simply changing it can be forgotten. Also similarly to commit
6282edb72bed ("clocksource/drivers/exynos_mct: Increase priority over
ARM arch timer"), there should be a valid and serious reason to
prioritize Exynos MCT.
Best regards,
Krzysztof
On 03/11/2021 10:24, Youngmin Nam wrote:
> On Wed, Nov 03, 2021 at 09:18:07AM +0100, Krzysztof Kozlowski wrote:
>> On 03/11/2021 01:09, Youngmin Nam wrote:
>>> On Tue, Nov 02, 2021 at 10:28:10AM +0000, Mark Rutland wrote:
>>>> On Tue, Nov 02, 2021 at 09:11:21AM +0900, Youngmin Nam wrote:
>>>>> Exynos MCT version 2 is composed of 1 FRC and 12 comparators.
>>>>> There are no global timer and local timer anymore.
>>>>> The 1 of 64bit FRC serves as "up-counter"(not "comparators").
>>>>> The 12 comaprators(not "counter") can be used as per-cpu event timer
>>>>> so that it can support upto 12 cores.
>>>>> And a RTC source can be used as backup clock source.
>>>>
>>>> [...]
>>>>
>>>>> +static int exynos_mct_starting_cpu(unsigned int cpu)
>>>>> +{
>>>>> + struct mct_clock_event_device *mevt = per_cpu_ptr(&percpu_mct_tick, cpu);
>>>>> + struct clock_event_device *evt = &mevt->evt;
>>>>> +
>>>>> + snprintf(mevt->name, sizeof(mevt->name), "mct_comp%d", cpu);
>>>>> +
>>>>> + evt->name = mevt->name;
>>>>> + evt->cpumask = cpumask_of(cpu);
>>>>> + evt->set_next_event = exynos_comp_set_next_event;
>>>>> + evt->set_state_periodic = mct_set_state_periodic;
>>>>> + evt->set_state_shutdown = mct_set_state_shutdown;
>>>>> + evt->set_state_oneshot = mct_set_state_shutdown;
>>>>> + evt->set_state_oneshot_stopped = mct_set_state_shutdown;
>>>>> + evt->tick_resume = mct_set_state_shutdown;
>>>>> + evt->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT;
>>>>> + evt->rating = 500; /* use value higher than ARM arch timer */
>>>>
>>>> Previously Will asked you to try CLOCK_EVT_FEAT_PERCPU here, and to set
>>>> the C3STOP flag on the arch timer via the DT when necessary, rather than
>>>> trying to override the arch timer like this:
>>>>
>>>> https://protect2.fireeye.com/v1/url?k=72526080-2dc9598b-7253ebcf-002590f5b904-ca603717c6462908&q=1&e=be56aa83-dbac-4639-913d-d388620fe3fc&u=https%3A%2F%2Flore.kernel.org%2Fr%2F20211027073458.GA22231%40willie-the-truck
>>>>
>>>> There are a bunch of things that depend on the architected timer working
>>>> as a clocksource (e.g. vdso, kvm), and it *should* work as a lock
>>>> clockevent_device if configured correctly, and it's much more consistent
>>>> with *everyone else* to use the arhcitected timer by default.
>>>>
>>>> Please try as Will suggested above, so that this works from day one.
>>>>
>>>> Thanks,
>>>> Mark.
>>>>
>>>
>>> Hi Mark.
>>> It looks like you missed my previous mail.
>>> https://protect2.fireeye.com/v1/url?k=ab15817a-cbf71c27-ab140a35-000babd9f1ba-123b7f313b1b1ccc&q=1&e=34c8716e-6d2e-4d8e-82fe-04777ebc5eb3&u=https%3A%2F%2Flore.kernel.org%2Fall%2F20211029035422.GA30523%40perf%2F%23t
>>>
>>> Yes, I believe Will's suggestion definitely will work.
>>> But that is for performance not functionality.
>>> As a driver for new H/W IP I would like to confirm functionality first.
>>>
>>> We need more time to test this feature with our exynos core power down feature.
>>> And we need to do a various regression test whether there is another corner case or not.
>>> So, how about we apply Will's suggetion later after the current patchset is merged first?
>>> After doing our regression test with our exynos core power down feature, we can confirm this.
>>>
>>
>> Not really, because once it is merged there is no incentive to fix it or
>> simply changing it can be forgotten. Also similarly to commit
>> 6282edb72bed ("clocksource/drivers/exynos_mct: Increase priority over
>> ARM arch timer"), there should be a valid and serious reason to
>> prioritize Exynos MCT.
>>
>
> No, it's not. I also want to decrease MCTv2 timer rating so that we want to use arm arch timer as a default.
> But this feature has to be confirmed with core power down feature enabled.
> Without core power down feature, we can't comfirm this.
> Ater that we need to check whether there is regression or not related power, stability, and so on.
> I'm not saying I will not apply Will's suggestion but I just want to apply later after some hard test.
>
You repeat the same argument, the same words. Nothing new. Repeating the
same won't change it, use the lower priority. This is a patch for new
kernel, so there is a plenty of time to test it and it won't affect your
production environment.
Best regards,
Krzysztof
On Wed, Nov 03, 2021 at 06:57:28PM +0900, Youngmin Nam wrote:
> On Wed, Nov 03, 2021 at 10:04:36AM +0100, Krzysztof Kozlowski wrote:
> > On 03/11/2021 10:24, Youngmin Nam wrote:
> > > On Wed, Nov 03, 2021 at 09:18:07AM +0100, Krzysztof Kozlowski wrote:
> > >> On 03/11/2021 01:09, Youngmin Nam wrote:
> > >>> On Tue, Nov 02, 2021 at 10:28:10AM +0000, Mark Rutland wrote:
> > >>>> On Tue, Nov 02, 2021 at 09:11:21AM +0900, Youngmin Nam wrote:
> > >>>>> + evt->rating = 500; /* use value higher than ARM arch timer */
> > >>>>
> > >>>> Previously Will asked you to try CLOCK_EVT_FEAT_PERCPU here, and to set
> > >>>> the C3STOP flag on the arch timer via the DT when necessary, rather than
> > >>>> trying to override the arch timer like this:
> > >>>>
> > >>>> https://protect2.fireeye.com/v1/url?k=72526080-2dc9598b-7253ebcf-002590f5b904-ca603717c6462908&q=1&e=be56aa83-dbac-4639-913d-d388620fe3fc&u=https%3A%2F%2Flore.kernel.org%2Fr%2F20211027073458.GA22231%40willie-the-truck
> > >>> Hi Mark.
> > >>> It looks like you missed my previous mail.
> > >>> https://protect2.fireeye.com/v1/url?k=ab15817a-cbf71c27-ab140a35-000babd9f1ba-123b7f313b1b1ccc&q=1&e=34c8716e-6d2e-4d8e-82fe-04777ebc5eb3&u=https%3A%2F%2Flore.kernel.org%2Fall%2F20211029035422.GA30523%40perf%2F%23t
> > >>>
> > >>> Yes, I believe Will's suggestion definitely will work.
Then please do so.
> > >>> But that is for performance not functionality.
No; it's about *consistency*, and avoiding unnecssary special cases. The
whole point is that marking the generic timer as C3STOP *accurately*
describes how the timer behaves on your platform, and marking the MCTv2
as a percpu timer which *can* act as a back-up also aligns with that.
That approach leaves the policy in the kernel, and we can play about
with that later without functional breakage.
> > >>> As a driver for new H/W IP I would like to confirm functionality first.
> > >>> We need more time to test this feature with our exynos core power down feature.
> > >>> And we need to do a various regression test whether there is another corner case or not.
> > >>> So, how about we apply Will's suggetion later after the current patchset is merged first?
> > >>> After doing our regression test with our exynos core power down feature, we can confirm this.
> > >>
> > >> Not really, because once it is merged there is no incentive to fix it or
> > >> simply changing it can be forgotten. Also similarly to commit
> > >> 6282edb72bed ("clocksource/drivers/exynos_mct: Increase priority over
> > >> ARM arch timer"), there should be a valid and serious reason to
> > >> prioritize Exynos MCT.
It's also worth nothing that the case described for 6282edb72bed is
really a system design erratum, since the counter is supposed to be in
an always-on power domain and should be counting well before a regular
OS kernel boots. The arm64 kernel requires the architected counter to be
running before it is entered, or there will be subtle breakage.
> > > No, it's not. I also want to decrease MCTv2 timer rating so that we want to use arm arch timer as a default.
> > > But this feature has to be confirmed with core power down feature enabled.
> > > Without core power down feature, we can't comfirm this.
> > > Ater that we need to check whether there is regression or not related power, stability, and so on.
> > > I'm not saying I will not apply Will's suggestion but I just want to apply later after some hard test.
> >
> > You repeat the same argument, the same words. Nothing new. Repeating the
> > same won't change it, use the lower priority. This is a patch for new
> > kernel, so there is a plenty of time to test it and it won't affect your
> > production environment.
> >
> So, how about we control timer rating value with DT ?
> Of course the default rating value should be lower than arm arch timer's.
> Do you agree with this?
No; placing a rating value in the DT is a hack. That should *not* live
in the DT because it's linux-internal detail and not a description of
the HW.
Thanks,
Mark.
Hi,
On Thu, Nov 04, 2021 at 09:21:02AM +0900, Youngmin Nam wrote:
> On Wed, Nov 03, 2021 at 10:04:18AM +0000, Mark Rutland wrote:
> > On Wed, Nov 03, 2021 at 06:57:28PM +0900, Youngmin Nam wrote:
> > > On Wed, Nov 03, 2021 at 10:04:36AM +0100, Krzysztof Kozlowski wrote:
> > > > On 03/11/2021 10:24, Youngmin Nam wrote:
> > > > > On Wed, Nov 03, 2021 at 09:18:07AM +0100, Krzysztof Kozlowski wrote:
> > > > >> On 03/11/2021 01:09, Youngmin Nam wrote:
> > > > >>> On Tue, Nov 02, 2021 at 10:28:10AM +0000, Mark Rutland wrote:
> > > > >>>> Previously Will asked you to try CLOCK_EVT_FEAT_PERCPU here, and to set
> > > > >>>> the C3STOP flag on the arch timer via the DT when necessary, rather than
> > > > >>>> trying to override the arch timer like this:
> > > > >>> Yes, I believe Will's suggestion definitely will work.
> > > So, how about we control timer rating value with DT ?
> > > Of course the default rating value should be lower than arm arch timer's.
> > > Do you agree with this?
> >
> > No; placing a rating value in the DT is a hack. That should *not* live
> > in the DT because it's linux-internal detail and not a description of
> > the HW.
>
> So, how do we use MCTv2 only for clock event device if there are some
> limitations caused by SoC design implemention ?
What limitations? Are you thinking of a known issue, or just in case
there is a bug in future HW?
If there is a problem, we'll need to describe that in the DT somehow,
and we need to know speciifcally what that limitation is.
Above you said that Will's suggestion will definitely work, which
implies no such limitations.
Thanks,
Mark.
On Fri, Nov 05, 2021 at 09:46:27AM +0900, Youngmin Nam wrote:
> On Thu, Nov 04, 2021 at 09:44:53AM +0000, Mark Rutland wrote:
> > Hi,
> >
> > On Thu, Nov 04, 2021 at 09:21:02AM +0900, Youngmin Nam wrote:
> > > On Wed, Nov 03, 2021 at 10:04:18AM +0000, Mark Rutland wrote:
> > > > On Wed, Nov 03, 2021 at 06:57:28PM +0900, Youngmin Nam wrote:
> > > > > On Wed, Nov 03, 2021 at 10:04:36AM +0100, Krzysztof Kozlowski wrote:
> > > > > > On 03/11/2021 10:24, Youngmin Nam wrote:
> > > > > > > On Wed, Nov 03, 2021 at 09:18:07AM +0100, Krzysztof Kozlowski wrote:
> > > > > > >> On 03/11/2021 01:09, Youngmin Nam wrote:
> > > > > > >>> On Tue, Nov 02, 2021 at 10:28:10AM +0000, Mark Rutland wrote:
> >
> > > > > > >>>> Previously Will asked you to try CLOCK_EVT_FEAT_PERCPU here, and to set
> > > > > > >>>> the C3STOP flag on the arch timer via the DT when necessary, rather than
> > > > > > >>>> trying to override the arch timer like this:
> >
> > > > > > >>> Yes, I believe Will's suggestion definitely will work.
> >
> > > > > So, how about we control timer rating value with DT ?
> > > > > Of course the default rating value should be lower than arm arch timer's.
> > > > > Do you agree with this?
> > > >
> > > > No; placing a rating value in the DT is a hack. That should *not* live
> > > > in the DT because it's linux-internal detail and not a description of
> > > > the HW.
> > >
> > > So, how do we use MCTv2 only for clock event device if there are some
> > > limitations caused by SoC design implemention ?
> >
> > What limitations? Are you thinking of a known issue, or just in case
> > there is a bug in future HW?
> >
> > If there is a problem, we'll need to describe that in the DT somehow,
> > and we need to know speciifcally what that limitation is.
> >
> > Above you said that Will's suggestion will definitely work, which
> > implies no such limitations.
> >
>
> Using arch timer for event device is highly related with Core power down feature so that it is also related with
> power saving scheme in SoC.
> Core power down and power saving depend on SoC design implemention.
> We can't confirm that using only arch timer can cover all scenario at production level.
Why not and what are we supposed to do about that? It's probably worth
pointing out that the majority of arm64-based SoCs have been using the arch
timer for *years* without problems. So I reckon you can make it work too.
> So we should be able to use MCTv2 as well.
>
> Why do you enforce using *only* arch timer ?
> Why aren't we allowed to use own timer of our SoC ?
You can do whatever you like downstream, but the upstream position is that
the arch timer is preferred: it's an architected solution from Arm which
supports things like virtualisation and userspace access. It's also a damn
sight quicker to access than the MCT.
On the downside, it's not usually possible to use it as a wakeup source,
so that's a great place for the MCT to shine. I've literally written the
code for you to do this.
Anyway, this MCTv2 driver is a dead duck whatever we do. Extend the existing
driver please.
Will