[PATCH v2 0/2] Indroduce Exynos Multi Core Timer version 2

[PATCH v2 0/2] Indroduce Exynos Multi Core Timer version 2

[Youngmin Nam]

Exynos Multi Core Timer version 2 is a new main system timer of next Exynos SoC.
Exynos MCT v2 consists of 1 64bit FRC(Free Running Counter) and 12 comparators.
Each comaprator produces interrupt when the value of current FRC + periods value matches with the
increasinig value of FRC.
So, 12 comaprators can be used as per-cpu event timer.
And RTC can be used as a backup clock source.

Changes in v2:
  - Add config dependency with ARCH_EXYNOS.
  - Remove 32bit of "mct-frc" which was used as a clock source in legacy SoC.
    (Currently, clock source was replaced with ARM arch timer)
  - Fix non-linux style comments and debug messege.
  - Fix license comments.
  - Update commit messege a bit to make it clear.

Youngmin Nam (2):
  clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2
    driver for next Exynos SoC
  dt-bindings: timer: samsung,s5e99xx-mct: Document s5e99xx-mct bindings

 .../bindings/timer/samsung,s5e99xx-mct.yaml   |  91 ++++++
 drivers/clocksource/Kconfig                   |   7 +
 drivers/clocksource/Makefile                  |   1 +
 drivers/clocksource/exynos_mct_v2.c           | 298 ++++++++++++++++++
 drivers/clocksource/exynos_mct_v2.h           |  71 +++++
 5 files changed, 468 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/timer/samsung,s5e99xx-mct.yaml
 create mode 100644 drivers/clocksource/exynos_mct_v2.c
 create mode 100644 drivers/clocksource/exynos_mct_v2.h

-- 
2.33.0

[PATCH v2 1/2] clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2 driver for next Exynos SoC

[Youngmin Nam]

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 <youngmin.nam@samsung.com>
---
 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

Re: [PATCH v2 1/2] clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2 driver for next Exynos SoC

[Mark Rutland]

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.

Re: [PATCH v2 1/2] clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2 driver for next Exynos SoC

[Youngmin Nam]

[-- Attachment #1: Type: text/plain, Size: 2774 bytes --]

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.

Thanks.

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2 1/2] clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2 driver for next Exynos SoC

[Krzysztof Kozlowski]

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

Re: [PATCH v2 1/2] clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2 driver for next Exynos SoC

[Youngmin Nam]

[-- Attachment #1: Type: text/plain, Size: 3985 bytes --]

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.

> 
> Best regards,
> Krzysztof
> 





[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2 1/2] clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2 driver for next Exynos SoC

[Krzysztof Kozlowski]

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

Re: [PATCH v2 1/2] clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2 driver for next Exynos SoC

[Youngmin Nam]

[-- Attachment #1: Type: text/plain, Size: 4665 bytes --]

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:
> >>>>> 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.
> 
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?

> Best regards,
> Krzysztof
> 

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2 1/2] clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2 driver for next Exynos SoC

[Mark Rutland]

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.

Re: [PATCH v2 1/2] clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2 driver for next Exynos SoC

[Youngmin Nam]

[-- Attachment #1: Type: text/plain, Size: 4361 bytes --]

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

So, how do we use MCTv2 only for clock event device if there are some limitations caused by SoC design implemention ?

> Thanks,
> Mark.
> 

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2 1/2] clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2 driver for next Exynos SoC

[Mark Rutland]

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.

Re: [PATCH v2 1/2] clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2 driver for next Exynos SoC

[Youngmin Nam]

[-- Attachment #1: Type: text/plain, Size: 2329 bytes --]

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

What I meant that replied to Will was
I think this will work generally, but we can't confirm this will cover all cases.

> Thanks,
> Mark.
> 

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2 1/2] clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2 driver for next Exynos SoC

[Will Deacon]

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

[PATCH v2 2/2] dt-bindings: timer: samsung,s5e99xx-mct: Document s5e99xx-mct bindings

[Youngmin Nam]

Add the MCT version 2 bindings for the s5e99xx SoC from samsung.

Signed-off-by: Youngmin Nam <youngmin.nam@samsung.com>
---
 .../bindings/timer/samsung,s5e99xx-mct.yaml   | 91 +++++++++++++++++++
 1 file changed, 91 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/timer/samsung,s5e99xx-mct.yaml

diff --git a/Documentation/devicetree/bindings/timer/samsung,s5e99xx-mct.yaml b/Documentation/devicetree/bindings/timer/samsung,s5e99xx-mct.yaml
new file mode 100644
index 000000000000..c887c7797ca8
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/samsung,s5e99xx-mct.yaml
@@ -0,0 +1,91 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/timer/samsung,s5e99xx-mct.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Samsung Exynos SoC Multi Core Timer (MCT)
+
+maintainers:
+  - Krzysztof Kozlowski <krzk@kernel.org>
+
+description: |+
+  The Samsung's Multi Core Timer (MCT) version 2 module includes
+  one 64-bit FRC(Free Running Counter) and 12 comparators.
+  The FRC serves up-counter and starts running at power-on.
+  The 12 comparators use the FRC value to produce interrupts.
+  They will produce interrupts when their internal value is matched with the FRC value.
+  Theses interrupts can be used as local timer interrupt of each CPU.
+
+properties:
+  compatible:
+    enum:
+      - samsung,s5e99xx-mct
+
+  clocks:
+    items:
+      - description: OSC clock
+      - description: PCLK clock
+      - description: RTC clock(optional)
+    minItems: 2
+
+  clock-names:
+    items:
+      - const: fin_pll
+      - const: mct
+      - const: rtc
+    minItems: 2
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    description: |
+      Interrupts should be put in specific order.
+      0: Local Timer Interrupt 0
+      1: Local Timer Interrupt 1
+      2: Local Timer Interrupt 2
+      3: ..
+      4: ..
+      i: Local Timer Interrupt n
+    minItems: 1              #  1 local timer interrupts
+    maxItems: 12             # 12 local timer interrupts
+
+  div:
+    description: If present, OSC clock freqency will be divided with this value.
+      And the divided value will be provided to MCT module.
+
+required:
+  - compatible
+  - clock-names
+  - clocks
+  - interrupts
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    // In this example, the IP contains 12 local timers, using separate interrupts,
+    // so 12 local timer interrupts have been specified,
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    timer@10050000 {
+        compatible = "samsung,s5e99xx-mct";
+        reg = <0x10050000 0x800>;
+        clocks = <&clock 1>, <&clock 10>;
+        clock-names = "fin_pll", "mct";
+
+        interrupts = <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 59 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 60 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 61 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 62 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 63 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 65 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 66 IRQ_TYPE_LEVEL_HIGH>;
+    };
-- 
2.33.0