* [PATCH v1 1/2] clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2 driver for next Exynos SoC
@ 2021-10-21 6:18 ` Youngmin Nam
0 siblings, 0 replies; 40+ messages in thread
From: Youngmin Nam @ 2021-10-21 6:18 UTC (permalink / raw)
To: krzysztof.kozlowski
Cc: daniel.lezcano, tglx, linux-kernel, linux-arm-kernel,
linux-samsung-soc, pullip.cho, hoony.yu, hajun.sung,
myung-su.cha, Youngmin Nam
Exynos MCT version 2 is composed of 1 FRC and 12 comparators.
The 12 comparators can produces interrupts independently,
so they can be used as local timer of each CPU.
Signed-off-by: Youngmin Nam <youngmin.nam@samsung.com>
---
drivers/clocksource/Kconfig | 6 +
drivers/clocksource/Makefile | 1 +
drivers/clocksource/exynos_mct_v2.c | 336 ++++++++++++++++++++++++++++
drivers/clocksource/exynos_mct_v2.h | 74 ++++++
4 files changed, 417 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..8ac04dd7f713 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -421,6 +421,12 @@ 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
+ 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..2da6d5401629
--- /dev/null
+++ b/drivers/clocksource/exynos_mct_v2.c
@@ -0,0 +1,336 @@
+// 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("MCT: 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);
+}
+
+/**
+ * exynos_read_count_32 - Read the lower 32-bits of the global counter
+ *
+ * This will read just the lower 32-bits of the global counter.
+ *
+ * Returns the number of cycles in the global counter (lower 32 bits).
+ */
+static u32 exynos_read_count_32(void)
+{
+ return readl_relaxed(reg_base + EXYNOS_MCT_CNT_L);
+}
+
+static u64 exynos_frc_read(struct clocksource *cs)
+{
+ return exynos_read_count_32();
+}
+
+static struct clocksource mct_frc = {
+ .name = "mct-frc",
+ .rating = 350, /* use value lower than ARM arch timer */
+ .read = exynos_frc_read,
+ .mask = CLOCKSOURCE_MASK(32),
+ .flags = CLOCK_SOURCE_IS_CONTINUOUS,
+};
+
+static int __init exynos_clocksource_init(void)
+{
+ if (clocksource_register_hz(&mct_frc, osc_clk_rate))
+ panic("%s: can't register clocksource\n", mct_frc.name);
+
+ return 0;
+}
+
+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_timer_resources\n");
+ ret = exynos_timer_resources(np, of_iomap(np, 0));
+ if (ret)
+ return ret;
+
+ pr_info("## exynos_clocksource_init\n");
+ ret = exynos_clocksource_init();
+
+ 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..377421803bbe
--- /dev/null
+++ b/drivers/clocksource/exynos_mct_v2.h
@@ -0,0 +1,74 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/**
+ * exynos_mct_v2.h - Samsung Exynos MCT(Multi-Core Timer) Driver Header file
+ *
+ * Copyright (C) 2021 Samsung Electronics Co., Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#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 // 32.768Khz
+#define DEFAULT_CLK_DIV 3 // 1/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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH v1 1/2] clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2 driver for next Exynos SoC
2021-10-21 6:18 ` Youngmin Nam
@ 2021-10-21 6:18 ` Krzysztof Kozlowski
-1 siblings, 0 replies; 40+ messages in thread
From: Krzysztof Kozlowski @ 2021-10-21 6:18 UTC (permalink / raw)
To: Youngmin Nam
Cc: daniel.lezcano, tglx, linux-kernel, linux-arm-kernel,
linux-samsung-soc, pullip.cho, hoony.yu, hajun.sung,
myung-su.cha
On 21/10/2021 08:18, Youngmin Nam wrote:
> Exynos MCT version 2 is composed of 1 FRC and 12 comparators.
> The 12 comparators can produces interrupts independently,
> so they can be used as local timer of each CPU.
>
> Signed-off-by: Youngmin Nam <youngmin.nam@samsung.com>
> ---
> drivers/clocksource/Kconfig | 6 +
> drivers/clocksource/Makefile | 1 +
> drivers/clocksource/exynos_mct_v2.c | 336 ++++++++++++++++++++++++++++
> drivers/clocksource/exynos_mct_v2.h | 74 ++++++
> 4 files changed, 417 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..8ac04dd7f713 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -421,6 +421,12 @@ 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..2da6d5401629
> --- /dev/null
> +++ b/drivers/clocksource/exynos_mct_v2.c
> @@ -0,0 +1,336 @@
> +// 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("MCT: 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);
> +}
> +
> +/**
> + * exynos_read_count_32 - Read the lower 32-bits of the global counter
> + *
> + * This will read just the lower 32-bits of the global counter.
> + *
> + * Returns the number of cycles in the global counter (lower 32 bits).
> + */
All this looks like a modification of Exynos MCT driver, so you should
extend that one instead. It does not look like we need two drivers.
Please integrate it into existing driver instead of sending a new piece
of code copied from vendor tree.
> +static u32 exynos_read_count_32(void)
> +{
> + return readl_relaxed(reg_base + EXYNOS_MCT_CNT_L);
> +}
> +
> +static u64 exynos_frc_read(struct clocksource *cs)
> +{
> + return exynos_read_count_32();
> +}
> +
> +static struct clocksource mct_frc = {
> + .name = "mct-frc",
> + .rating = 350, /* use value lower than ARM arch timer */
> + .read = exynos_frc_read,
> + .mask = CLOCKSOURCE_MASK(32),
> + .flags = CLOCK_SOURCE_IS_CONTINUOUS,
> +};
> +
> +static int __init exynos_clocksource_init(void)
> +{
> + if (clocksource_register_hz(&mct_frc, osc_clk_rate))
> + panic("%s: can't register clocksource\n", mct_frc.name);
> +
> + return 0;
> +}
> +
> +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");
Why timer needs a RTC clock?
> + 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_timer_resources\n");
Not a Linux kernel style of debug message.
> + ret = exynos_timer_resources(np, of_iomap(np, 0));
> + if (ret)
> + return ret;
> +
> + pr_info("## exynos_clocksource_init\n");
> + ret = exynos_clocksource_init();
> +
> + 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..377421803bbe
> --- /dev/null
> +++ b/drivers/clocksource/exynos_mct_v2.h
> @@ -0,0 +1,74 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/**
> + * exynos_mct_v2.h - Samsung Exynos MCT(Multi-Core Timer) Driver Header file
> + *
> + * Copyright (C) 2021 Samsung Electronics Co., Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
No need for license text.
> + */
> +
> +#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 // 32.768Khz
> +#define DEFAULT_CLK_DIV 3 // 1/3
Such comments are not useful.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v1 1/2] clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2 driver for next Exynos SoC
@ 2021-10-21 6:18 ` Krzysztof Kozlowski
0 siblings, 0 replies; 40+ messages in thread
From: Krzysztof Kozlowski @ 2021-10-21 6:18 UTC (permalink / raw)
To: Youngmin Nam
Cc: daniel.lezcano, tglx, linux-kernel, linux-arm-kernel,
linux-samsung-soc, pullip.cho, hoony.yu, hajun.sung,
myung-su.cha
On 21/10/2021 08:18, Youngmin Nam wrote:
> Exynos MCT version 2 is composed of 1 FRC and 12 comparators.
> The 12 comparators can produces interrupts independently,
> so they can be used as local timer of each CPU.
>
> Signed-off-by: Youngmin Nam <youngmin.nam@samsung.com>
> ---
> drivers/clocksource/Kconfig | 6 +
> drivers/clocksource/Makefile | 1 +
> drivers/clocksource/exynos_mct_v2.c | 336 ++++++++++++++++++++++++++++
> drivers/clocksource/exynos_mct_v2.h | 74 ++++++
> 4 files changed, 417 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..8ac04dd7f713 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -421,6 +421,12 @@ 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..2da6d5401629
> --- /dev/null
> +++ b/drivers/clocksource/exynos_mct_v2.c
> @@ -0,0 +1,336 @@
> +// 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("MCT: 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);
> +}
> +
> +/**
> + * exynos_read_count_32 - Read the lower 32-bits of the global counter
> + *
> + * This will read just the lower 32-bits of the global counter.
> + *
> + * Returns the number of cycles in the global counter (lower 32 bits).
> + */
All this looks like a modification of Exynos MCT driver, so you should
extend that one instead. It does not look like we need two drivers.
Please integrate it into existing driver instead of sending a new piece
of code copied from vendor tree.
> +static u32 exynos_read_count_32(void)
> +{
> + return readl_relaxed(reg_base + EXYNOS_MCT_CNT_L);
> +}
> +
> +static u64 exynos_frc_read(struct clocksource *cs)
> +{
> + return exynos_read_count_32();
> +}
> +
> +static struct clocksource mct_frc = {
> + .name = "mct-frc",
> + .rating = 350, /* use value lower than ARM arch timer */
> + .read = exynos_frc_read,
> + .mask = CLOCKSOURCE_MASK(32),
> + .flags = CLOCK_SOURCE_IS_CONTINUOUS,
> +};
> +
> +static int __init exynos_clocksource_init(void)
> +{
> + if (clocksource_register_hz(&mct_frc, osc_clk_rate))
> + panic("%s: can't register clocksource\n", mct_frc.name);
> +
> + return 0;
> +}
> +
> +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");
Why timer needs a RTC clock?
> + 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_timer_resources\n");
Not a Linux kernel style of debug message.
> + ret = exynos_timer_resources(np, of_iomap(np, 0));
> + if (ret)
> + return ret;
> +
> + pr_info("## exynos_clocksource_init\n");
> + ret = exynos_clocksource_init();
> +
> + 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..377421803bbe
> --- /dev/null
> +++ b/drivers/clocksource/exynos_mct_v2.h
> @@ -0,0 +1,74 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/**
> + * exynos_mct_v2.h - Samsung Exynos MCT(Multi-Core Timer) Driver Header file
> + *
> + * Copyright (C) 2021 Samsung Electronics Co., Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
No need for license text.
> + */
> +
> +#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 // 32.768Khz
> +#define DEFAULT_CLK_DIV 3 // 1/3
Such comments are not useful.
Best regards,
Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v1 1/2] clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2 driver for next Exynos SoC
2021-10-21 6:18 ` Krzysztof Kozlowski
@ 2021-10-21 8:26 ` Youngmin Nam
-1 siblings, 0 replies; 40+ messages in thread
From: Youngmin Nam @ 2021-10-21 8:26 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: daniel.lezcano, tglx, linux-kernel, linux-arm-kernel,
linux-samsung-soc, pullip.cho, hoony.yu, hajun.sung,
myung-su.cha
[-- Attachment #1: Type: text/plain, Size: 16455 bytes --]
On Thu, Oct 21, 2021 at 08:18:36AM +0200, Krzysztof Kozlowski wrote:
> On 21/10/2021 08:18, Youngmin Nam wrote:
> > Exynos MCT version 2 is composed of 1 FRC and 12 comparators.
> > The 12 comparators can produces interrupts independently,
> > so they can be used as local timer of each CPU.
> >
> > Signed-off-by: Youngmin Nam <youngmin.nam@samsung.com>
> > ---
> > drivers/clocksource/Kconfig | 6 +
> > drivers/clocksource/Makefile | 1 +
> > drivers/clocksource/exynos_mct_v2.c | 336 ++++++++++++++++++++++++++++
> > drivers/clocksource/exynos_mct_v2.h | 74 ++++++
> > 4 files changed, 417 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..8ac04dd7f713 100644
> > --- a/drivers/clocksource/Kconfig
> > +++ b/drivers/clocksource/Kconfig
> > @@ -421,6 +421,12 @@ 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.
>
Okay
> > + 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..2da6d5401629
> > --- /dev/null
> > +++ b/drivers/clocksource/exynos_mct_v2.c
> > @@ -0,0 +1,336 @@
> > +// 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("MCT: 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);
> > +}
> > +
> > +/**
> > + * exynos_read_count_32 - Read the lower 32-bits of the global counter
> > + *
> > + * This will read just the lower 32-bits of the global counter.
> > + *
> > + * Returns the number of cycles in the global counter (lower 32 bits).
> > + */
>
> All this looks like a modification of Exynos MCT driver, so you should
> extend that one instead. It does not look like we need two drivers.
> Please integrate it into existing driver instead of sending a new piece
> of code copied from vendor tree.
>
MCT version 2 is a completely different HW IP compared to previous MCT.
The new MCT has a lot of different resister sets and there are many changes on programming guide.
So we cannot share the previous code. At first, I also considered that way you mentioned,
but it would be better to implement the driver seperately to maintain the new driver cleanly.
> > +static u32 exynos_read_count_32(void)
> > +{
> > + return readl_relaxed(reg_base + EXYNOS_MCT_CNT_L);
> > +}
> > +
> > +static u64 exynos_frc_read(struct clocksource *cs)
> > +{
> > + return exynos_read_count_32();
> > +}
> > +
> > +static struct clocksource mct_frc = {
> > + .name = "mct-frc",
> > + .rating = 350, /* use value lower than ARM arch timer */
> > + .read = exynos_frc_read,
> > + .mask = CLOCKSOURCE_MASK(32),
> > + .flags = CLOCK_SOURCE_IS_CONTINUOUS,
> > +};
> > +
> > +static int __init exynos_clocksource_init(void)
> > +{
> > + if (clocksource_register_hz(&mct_frc, osc_clk_rate))
> > + panic("%s: can't register clocksource\n", mct_frc.name);
> > +
> > + return 0;
> > +}
> > +
> > +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");
>
> Why timer needs a RTC clock?
>
On the new MCT, RTC clock can be used as backup clock instead of OSC clock.
> > + 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_timer_resources\n");
>
> Not a Linux kernel style of debug message.
>
Okay
> > + ret = exynos_timer_resources(np, of_iomap(np, 0));
> > + if (ret)
> > + return ret;
> > +
> > + pr_info("## exynos_clocksource_init\n");
> > + ret = exynos_clocksource_init();
> > +
> > + 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..377421803bbe
> > --- /dev/null
> > +++ b/drivers/clocksource/exynos_mct_v2.h
> > @@ -0,0 +1,74 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/**
> > + * exynos_mct_v2.h - Samsung Exynos MCT(Multi-Core Timer) Driver Header file
> > + *
> > + * Copyright (C) 2021 Samsung Electronics Co., Ltd.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
>
> No need for license text.
>
Okay
> > + */
> > +
> > +#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 // 32.768Khz
> > +#define DEFAULT_CLK_DIV 3 // 1/3
>
> Such comments are not useful.
>
Okay
> Best regards,
> Krzysztof
>
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
[-- Attachment #3: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v1 1/2] clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2 driver for next Exynos SoC
@ 2021-10-21 8:26 ` Youngmin Nam
0 siblings, 0 replies; 40+ messages in thread
From: Youngmin Nam @ 2021-10-21 8:26 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: daniel.lezcano, tglx, linux-kernel, linux-arm-kernel,
linux-samsung-soc, pullip.cho, hoony.yu, hajun.sung,
myung-su.cha
[-- Attachment #1: Type: text/plain, Size: 16455 bytes --]
On Thu, Oct 21, 2021 at 08:18:36AM +0200, Krzysztof Kozlowski wrote:
> On 21/10/2021 08:18, Youngmin Nam wrote:
> > Exynos MCT version 2 is composed of 1 FRC and 12 comparators.
> > The 12 comparators can produces interrupts independently,
> > so they can be used as local timer of each CPU.
> >
> > Signed-off-by: Youngmin Nam <youngmin.nam@samsung.com>
> > ---
> > drivers/clocksource/Kconfig | 6 +
> > drivers/clocksource/Makefile | 1 +
> > drivers/clocksource/exynos_mct_v2.c | 336 ++++++++++++++++++++++++++++
> > drivers/clocksource/exynos_mct_v2.h | 74 ++++++
> > 4 files changed, 417 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..8ac04dd7f713 100644
> > --- a/drivers/clocksource/Kconfig
> > +++ b/drivers/clocksource/Kconfig
> > @@ -421,6 +421,12 @@ 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.
>
Okay
> > + 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..2da6d5401629
> > --- /dev/null
> > +++ b/drivers/clocksource/exynos_mct_v2.c
> > @@ -0,0 +1,336 @@
> > +// 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("MCT: 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);
> > +}
> > +
> > +/**
> > + * exynos_read_count_32 - Read the lower 32-bits of the global counter
> > + *
> > + * This will read just the lower 32-bits of the global counter.
> > + *
> > + * Returns the number of cycles in the global counter (lower 32 bits).
> > + */
>
> All this looks like a modification of Exynos MCT driver, so you should
> extend that one instead. It does not look like we need two drivers.
> Please integrate it into existing driver instead of sending a new piece
> of code copied from vendor tree.
>
MCT version 2 is a completely different HW IP compared to previous MCT.
The new MCT has a lot of different resister sets and there are many changes on programming guide.
So we cannot share the previous code. At first, I also considered that way you mentioned,
but it would be better to implement the driver seperately to maintain the new driver cleanly.
> > +static u32 exynos_read_count_32(void)
> > +{
> > + return readl_relaxed(reg_base + EXYNOS_MCT_CNT_L);
> > +}
> > +
> > +static u64 exynos_frc_read(struct clocksource *cs)
> > +{
> > + return exynos_read_count_32();
> > +}
> > +
> > +static struct clocksource mct_frc = {
> > + .name = "mct-frc",
> > + .rating = 350, /* use value lower than ARM arch timer */
> > + .read = exynos_frc_read,
> > + .mask = CLOCKSOURCE_MASK(32),
> > + .flags = CLOCK_SOURCE_IS_CONTINUOUS,
> > +};
> > +
> > +static int __init exynos_clocksource_init(void)
> > +{
> > + if (clocksource_register_hz(&mct_frc, osc_clk_rate))
> > + panic("%s: can't register clocksource\n", mct_frc.name);
> > +
> > + return 0;
> > +}
> > +
> > +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");
>
> Why timer needs a RTC clock?
>
On the new MCT, RTC clock can be used as backup clock instead of OSC clock.
> > + 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_timer_resources\n");
>
> Not a Linux kernel style of debug message.
>
Okay
> > + ret = exynos_timer_resources(np, of_iomap(np, 0));
> > + if (ret)
> > + return ret;
> > +
> > + pr_info("## exynos_clocksource_init\n");
> > + ret = exynos_clocksource_init();
> > +
> > + 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..377421803bbe
> > --- /dev/null
> > +++ b/drivers/clocksource/exynos_mct_v2.h
> > @@ -0,0 +1,74 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/**
> > + * exynos_mct_v2.h - Samsung Exynos MCT(Multi-Core Timer) Driver Header file
> > + *
> > + * Copyright (C) 2021 Samsung Electronics Co., Ltd.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
>
> No need for license text.
>
Okay
> > + */
> > +
> > +#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 // 32.768Khz
> > +#define DEFAULT_CLK_DIV 3 // 1/3
>
> Such comments are not useful.
>
Okay
> Best regards,
> Krzysztof
>
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v1 1/2] clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2 driver for next Exynos SoC
2021-10-21 8:26 ` Youngmin Nam
@ 2021-10-21 8:07 ` Krzysztof Kozlowski
-1 siblings, 0 replies; 40+ messages in thread
From: Krzysztof Kozlowski @ 2021-10-21 8:07 UTC (permalink / raw)
To: Youngmin Nam
Cc: daniel.lezcano, tglx, linux-kernel, linux-arm-kernel,
linux-samsung-soc, pullip.cho, hoony.yu, hajun.sung,
myung-su.cha
On 21/10/2021 10:26, Youngmin Nam wrote:
> On Thu, Oct 21, 2021 at 08:18:36AM +0200, Krzysztof Kozlowski wrote:
>> On 21/10/2021 08:18, Youngmin Nam wrote:
>>> Exynos MCT version 2 is composed of 1 FRC and 12 comparators.
>>> The 12 comparators can produces interrupts independently,
>>> so they can be used as local timer of each CPU.
>>>
>>> Signed-off-by: Youngmin Nam <youngmin.nam@samsung.com>
>>> ---
>>> drivers/clocksource/Kconfig | 6 +
>>> drivers/clocksource/Makefile | 1 +
>>> drivers/clocksource/exynos_mct_v2.c | 336 ++++++++++++++++++++++++++++
>>> drivers/clocksource/exynos_mct_v2.h | 74 ++++++
>>> 4 files changed, 417 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..8ac04dd7f713 100644
>>> --- a/drivers/clocksource/Kconfig
>>> +++ b/drivers/clocksource/Kconfig
>>> @@ -421,6 +421,12 @@ 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.
>>
> Okay
>>> + 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..2da6d5401629
>>> --- /dev/null
>>> +++ b/drivers/clocksource/exynos_mct_v2.c
>>> @@ -0,0 +1,336 @@
>>> +// 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("MCT: 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);
>>> +}
>>> +
>>> +/**
>>> + * exynos_read_count_32 - Read the lower 32-bits of the global counter
>>> + *
>>> + * This will read just the lower 32-bits of the global counter.
>>> + *
>>> + * Returns the number of cycles in the global counter (lower 32 bits).
>>> + */
>>
>> All this looks like a modification of Exynos MCT driver, so you should
>> extend that one instead. It does not look like we need two drivers.
>> Please integrate it into existing driver instead of sending a new piece
>> of code copied from vendor tree.
>>
> MCT version 2 is a completely different HW IP compared to previous MCT.
> The new MCT has a lot of different resister sets and there are many changes on programming guide.
> So we cannot share the previous code. At first, I also considered that way you mentioned,
> but it would be better to implement the driver seperately to maintain the new driver cleanly.
We have several drivers supporting different devices and we avoid mostly
duplicating new ones. The different register layout is not the valid
reason - we support differences in several places. Just take a look at
Samsung PMIC drivers where register layout is quite different between
designs. Still one clock driver, one RTC driver and 2-3 regulator
drivers (for ~7 devices).
Similarly to SoC clock, pinctrl, PMU and other drivers - we re-use
instead of duplicating entire driver.
I am sorry but the argument that block is different is not enough. What
is exactly not compatible with current driver which could not be modeled
by different driver data (or structure with ops)?
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v1 1/2] clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2 driver for next Exynos SoC
@ 2021-10-21 8:07 ` Krzysztof Kozlowski
0 siblings, 0 replies; 40+ messages in thread
From: Krzysztof Kozlowski @ 2021-10-21 8:07 UTC (permalink / raw)
To: Youngmin Nam
Cc: daniel.lezcano, tglx, linux-kernel, linux-arm-kernel,
linux-samsung-soc, pullip.cho, hoony.yu, hajun.sung,
myung-su.cha
On 21/10/2021 10:26, Youngmin Nam wrote:
> On Thu, Oct 21, 2021 at 08:18:36AM +0200, Krzysztof Kozlowski wrote:
>> On 21/10/2021 08:18, Youngmin Nam wrote:
>>> Exynos MCT version 2 is composed of 1 FRC and 12 comparators.
>>> The 12 comparators can produces interrupts independently,
>>> so they can be used as local timer of each CPU.
>>>
>>> Signed-off-by: Youngmin Nam <youngmin.nam@samsung.com>
>>> ---
>>> drivers/clocksource/Kconfig | 6 +
>>> drivers/clocksource/Makefile | 1 +
>>> drivers/clocksource/exynos_mct_v2.c | 336 ++++++++++++++++++++++++++++
>>> drivers/clocksource/exynos_mct_v2.h | 74 ++++++
>>> 4 files changed, 417 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..8ac04dd7f713 100644
>>> --- a/drivers/clocksource/Kconfig
>>> +++ b/drivers/clocksource/Kconfig
>>> @@ -421,6 +421,12 @@ 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.
>>
> Okay
>>> + 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..2da6d5401629
>>> --- /dev/null
>>> +++ b/drivers/clocksource/exynos_mct_v2.c
>>> @@ -0,0 +1,336 @@
>>> +// 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("MCT: 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);
>>> +}
>>> +
>>> +/**
>>> + * exynos_read_count_32 - Read the lower 32-bits of the global counter
>>> + *
>>> + * This will read just the lower 32-bits of the global counter.
>>> + *
>>> + * Returns the number of cycles in the global counter (lower 32 bits).
>>> + */
>>
>> All this looks like a modification of Exynos MCT driver, so you should
>> extend that one instead. It does not look like we need two drivers.
>> Please integrate it into existing driver instead of sending a new piece
>> of code copied from vendor tree.
>>
> MCT version 2 is a completely different HW IP compared to previous MCT.
> The new MCT has a lot of different resister sets and there are many changes on programming guide.
> So we cannot share the previous code. At first, I also considered that way you mentioned,
> but it would be better to implement the driver seperately to maintain the new driver cleanly.
We have several drivers supporting different devices and we avoid mostly
duplicating new ones. The different register layout is not the valid
reason - we support differences in several places. Just take a look at
Samsung PMIC drivers where register layout is quite different between
designs. Still one clock driver, one RTC driver and 2-3 regulator
drivers (for ~7 devices).
Similarly to SoC clock, pinctrl, PMU and other drivers - we re-use
instead of duplicating entire driver.
I am sorry but the argument that block is different is not enough. What
is exactly not compatible with current driver which could not be modeled
by different driver data (or structure with ops)?
Best regards,
Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v1 1/2] clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2 driver for next Exynos SoC
2021-10-21 8:07 ` Krzysztof Kozlowski
@ 2021-10-22 4:21 ` Youngmin Nam
-1 siblings, 0 replies; 40+ messages in thread
From: Youngmin Nam @ 2021-10-22 4:21 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: daniel.lezcano, tglx, linux-kernel, linux-arm-kernel,
linux-samsung-soc, pullip.cho, hoony.yu, hajun.sung,
myung-su.cha
[-- Attachment #1: Type: text/plain, Size: 7094 bytes --]
On Thu, Oct 21, 2021 at 10:07:25AM +0200, Krzysztof Kozlowski wrote:
> On 21/10/2021 10:26, Youngmin Nam wrote:
> > On Thu, Oct 21, 2021 at 08:18:36AM +0200, Krzysztof Kozlowski wrote:
> >> On 21/10/2021 08:18, Youngmin Nam wrote:
> >>> Exynos MCT version 2 is composed of 1 FRC and 12 comparators.
> >>> The 12 comparators can produces interrupts independently,
> >>> so they can be used as local timer of each CPU.
> >>>
> >>> Signed-off-by: Youngmin Nam <youngmin.nam@samsung.com>
> >>> ---
> >>> drivers/clocksource/Kconfig | 6 +
> >>> drivers/clocksource/Makefile | 1 +
> >>> drivers/clocksource/exynos_mct_v2.c | 336 ++++++++++++++++++++++++++++
> >>> drivers/clocksource/exynos_mct_v2.h | 74 ++++++
> >>> 4 files changed, 417 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..8ac04dd7f713 100644
> >>> --- a/drivers/clocksource/Kconfig
> >>> +++ b/drivers/clocksource/Kconfig
> >>> @@ -421,6 +421,12 @@ 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.
> >>
> > Okay
> >>> + 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..2da6d5401629
> >>> --- /dev/null
> >>> +++ b/drivers/clocksource/exynos_mct_v2.c
> >>> @@ -0,0 +1,336 @@
> >>> +// 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("MCT: 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);
> >>> +}
> >>> +
> >>> +/**
> >>> + * exynos_read_count_32 - Read the lower 32-bits of the global counter
> >>> + *
> >>> + * This will read just the lower 32-bits of the global counter.
> >>> + *
> >>> + * Returns the number of cycles in the global counter (lower 32 bits).
> >>> + */
> >>
> >> All this looks like a modification of Exynos MCT driver, so you should
> >> extend that one instead. It does not look like we need two drivers.
> >> Please integrate it into existing driver instead of sending a new piece
> >> of code copied from vendor tree.
> >>
> > MCT version 2 is a completely different HW IP compared to previous MCT.
> > The new MCT has a lot of different resister sets and there are many changes on programming guide.
> > So we cannot share the previous code. At first, I also considered that way you mentioned,
> > but it would be better to implement the driver seperately to maintain the new driver cleanly.
>
> We have several drivers supporting different devices and we avoid mostly
> duplicating new ones. The different register layout is not the valid
> reason - we support differences in several places. Just take a look at
> Samsung PMIC drivers where register layout is quite different between
> designs. Still one clock driver, one RTC driver and 2-3 regulator
> drivers (for ~7 devices).
>
> Similarly to SoC clock, pinctrl, PMU and other drivers - we re-use
> instead of duplicating entire driver.
>
> I am sorry but the argument that block is different is not enough. What
> is exactly not compatible with current driver which could not be modeled
> by different driver data (or structure with ops)?
>
I've checked samsung regulator driver and there are 3 types of driver as you mentioned.
They are being maintained separately because they are not compatible each other.
These are comparison of previous MCT and new MCT.
* Previous MCT
- 4 Global timer + 8 Local timer
- Clock Source is OSC only
* New MCT
- 1 Free Running Counter + 12 comparators
- Clock Sources are OSC and RTC
- Programming guide is totally different comapared to previous MCT.
MCTv2 is totally newly designed for the next Exynos Series.
IP design, the way of operating and register configurations are different as well register layout.
It is new generation of Exynos system timer. It's not compatible with the previous MCT.
This is the start of implementation for the new MCT driver and we might have a lot of
changes for new feature.
If we maintain as one driver, everytime we change the new MCT driver we should test
all of SoC which doesn't have the new MCT. And vice versa.
> Best regards,
> Krzysztof
>
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v1 1/2] clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2 driver for next Exynos SoC
@ 2021-10-22 4:21 ` Youngmin Nam
0 siblings, 0 replies; 40+ messages in thread
From: Youngmin Nam @ 2021-10-22 4:21 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: daniel.lezcano, tglx, linux-kernel, linux-arm-kernel,
linux-samsung-soc, pullip.cho, hoony.yu, hajun.sung,
myung-su.cha
[-- Attachment #1: Type: text/plain, Size: 7094 bytes --]
On Thu, Oct 21, 2021 at 10:07:25AM +0200, Krzysztof Kozlowski wrote:
> On 21/10/2021 10:26, Youngmin Nam wrote:
> > On Thu, Oct 21, 2021 at 08:18:36AM +0200, Krzysztof Kozlowski wrote:
> >> On 21/10/2021 08:18, Youngmin Nam wrote:
> >>> Exynos MCT version 2 is composed of 1 FRC and 12 comparators.
> >>> The 12 comparators can produces interrupts independently,
> >>> so they can be used as local timer of each CPU.
> >>>
> >>> Signed-off-by: Youngmin Nam <youngmin.nam@samsung.com>
> >>> ---
> >>> drivers/clocksource/Kconfig | 6 +
> >>> drivers/clocksource/Makefile | 1 +
> >>> drivers/clocksource/exynos_mct_v2.c | 336 ++++++++++++++++++++++++++++
> >>> drivers/clocksource/exynos_mct_v2.h | 74 ++++++
> >>> 4 files changed, 417 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..8ac04dd7f713 100644
> >>> --- a/drivers/clocksource/Kconfig
> >>> +++ b/drivers/clocksource/Kconfig
> >>> @@ -421,6 +421,12 @@ 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.
> >>
> > Okay
> >>> + 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..2da6d5401629
> >>> --- /dev/null
> >>> +++ b/drivers/clocksource/exynos_mct_v2.c
> >>> @@ -0,0 +1,336 @@
> >>> +// 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("MCT: 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);
> >>> +}
> >>> +
> >>> +/**
> >>> + * exynos_read_count_32 - Read the lower 32-bits of the global counter
> >>> + *
> >>> + * This will read just the lower 32-bits of the global counter.
> >>> + *
> >>> + * Returns the number of cycles in the global counter (lower 32 bits).
> >>> + */
> >>
> >> All this looks like a modification of Exynos MCT driver, so you should
> >> extend that one instead. It does not look like we need two drivers.
> >> Please integrate it into existing driver instead of sending a new piece
> >> of code copied from vendor tree.
> >>
> > MCT version 2 is a completely different HW IP compared to previous MCT.
> > The new MCT has a lot of different resister sets and there are many changes on programming guide.
> > So we cannot share the previous code. At first, I also considered that way you mentioned,
> > but it would be better to implement the driver seperately to maintain the new driver cleanly.
>
> We have several drivers supporting different devices and we avoid mostly
> duplicating new ones. The different register layout is not the valid
> reason - we support differences in several places. Just take a look at
> Samsung PMIC drivers where register layout is quite different between
> designs. Still one clock driver, one RTC driver and 2-3 regulator
> drivers (for ~7 devices).
>
> Similarly to SoC clock, pinctrl, PMU and other drivers - we re-use
> instead of duplicating entire driver.
>
> I am sorry but the argument that block is different is not enough. What
> is exactly not compatible with current driver which could not be modeled
> by different driver data (or structure with ops)?
>
I've checked samsung regulator driver and there are 3 types of driver as you mentioned.
They are being maintained separately because they are not compatible each other.
These are comparison of previous MCT and new MCT.
* Previous MCT
- 4 Global timer + 8 Local timer
- Clock Source is OSC only
* New MCT
- 1 Free Running Counter + 12 comparators
- Clock Sources are OSC and RTC
- Programming guide is totally different comapared to previous MCT.
MCTv2 is totally newly designed for the next Exynos Series.
IP design, the way of operating and register configurations are different as well register layout.
It is new generation of Exynos system timer. It's not compatible with the previous MCT.
This is the start of implementation for the new MCT driver and we might have a lot of
changes for new feature.
If we maintain as one driver, everytime we change the new MCT driver we should test
all of SoC which doesn't have the new MCT. And vice versa.
> Best regards,
> Krzysztof
>
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
[-- Attachment #3: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v1 1/2] clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2 driver for next Exynos SoC
2021-10-22 4:21 ` Youngmin Nam
@ 2021-10-25 8:18 ` Krzysztof Kozlowski
-1 siblings, 0 replies; 40+ messages in thread
From: Krzysztof Kozlowski @ 2021-10-25 8:18 UTC (permalink / raw)
To: Youngmin Nam
Cc: daniel.lezcano, tglx, linux-kernel, linux-arm-kernel,
linux-samsung-soc, pullip.cho, hoony.yu, hajun.sung,
myung-su.cha
On 22/10/2021 06:21, Youngmin Nam wrote:
> On Thu, Oct 21, 2021 at 10:07:25AM +0200, Krzysztof Kozlowski wrote:
>> On 21/10/2021 10:26, Youngmin Nam wrote:
>>> On Thu, Oct 21, 2021 at 08:18:36AM +0200, Krzysztof Kozlowski wrote:
>>>> On 21/10/2021 08:18, Youngmin Nam wrote:
>>>>> Exynos MCT version 2 is composed of 1 FRC and 12 comparators.
>>>>> The 12 comparators can produces interrupts independently,
>>>>> so they can be used as local timer of each CPU.
>>>>>
>>>>> Signed-off-by: Youngmin Nam <youngmin.nam@samsung.com>
>>>>> ---
>>>>> drivers/clocksource/Kconfig | 6 +
>>>>> drivers/clocksource/Makefile | 1 +
>>>>> drivers/clocksource/exynos_mct_v2.c | 336 ++++++++++++++++++++++++++++
>>>>> drivers/clocksource/exynos_mct_v2.h | 74 ++++++
>>>>> 4 files changed, 417 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..8ac04dd7f713 100644
>>>>> --- a/drivers/clocksource/Kconfig
>>>>> +++ b/drivers/clocksource/Kconfig
>>>>> @@ -421,6 +421,12 @@ 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.
>>>>
>>> Okay
>>>>> + 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..2da6d5401629
>>>>> --- /dev/null
>>>>> +++ b/drivers/clocksource/exynos_mct_v2.c
>>>>> @@ -0,0 +1,336 @@
>>>>> +// 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("MCT: 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);
>>>>> +}
>>>>> +
>>>>> +/**
>>>>> + * exynos_read_count_32 - Read the lower 32-bits of the global counter
>>>>> + *
>>>>> + * This will read just the lower 32-bits of the global counter.
>>>>> + *
>>>>> + * Returns the number of cycles in the global counter (lower 32 bits).
>>>>> + */
>>>>
>>>> All this looks like a modification of Exynos MCT driver, so you should
>>>> extend that one instead. It does not look like we need two drivers.
>>>> Please integrate it into existing driver instead of sending a new piece
>>>> of code copied from vendor tree.
>>>>
>>> MCT version 2 is a completely different HW IP compared to previous MCT.
>>> The new MCT has a lot of different resister sets and there are many changes on programming guide.
>>> So we cannot share the previous code. At first, I also considered that way you mentioned,
>>> but it would be better to implement the driver seperately to maintain the new driver cleanly.
>>
>> We have several drivers supporting different devices and we avoid mostly
>> duplicating new ones. The different register layout is not the valid
>> reason - we support differences in several places. Just take a look at
>> Samsung PMIC drivers where register layout is quite different between
>> designs. Still one clock driver, one RTC driver and 2-3 regulator
>> drivers (for ~7 devices).
>>
>> Similarly to SoC clock, pinctrl, PMU and other drivers - we re-use
>> instead of duplicating entire driver.
>>
>> I am sorry but the argument that block is different is not enough. What
>> is exactly not compatible with current driver which could not be modeled
>> by different driver data (or structure with ops)?
>>
> I've checked samsung regulator driver and there are 3 types of driver as you mentioned.
> They are being maintained separately because they are not compatible each other.
That's not correct. We integrated 5 separate devices into s2mps11
regulator driver, around 7 devices into a MFD driver, 4 devices into RTC
driver and 4 devices into clock driver.
>
> These are comparison of previous MCT and new MCT.
> * Previous MCT
> - 4 Global timer + 8 Local timer
> - Clock Source is OSC only
>
> * New MCT
> - 1 Free Running Counter + 12 comparators
One FRC was also in previous MCT, wasn't it? It supported 4 comparators,
but FRC was only one.
> - Clock Sources are OSC and RTC
> - Programming guide is totally different comapared to previous MCT.
Thanks, I got it from the driver. Linux supports handling such
differences, including differences in register map. In the same time
just quick look at the code shows several re-used functions.
>
> MCTv2 is totally newly designed for the next Exynos Series.
> IP design, the way of operating and register configurations are different as well register layout.
We handle different register layouts without big issue. There are
several drivers showing this example, for example mentioned earlier
Samsung PMIC drivers. 4 different register layouts for RTC driver in one
driver and you mention here that two is some big difference?
The way of operating could be indeed a trouble but looking at the code
it is actually very, very similar.
> It is new generation of Exynos system timer. It's not compatible with the previous MCT.
> This is the start of implementation for the new MCT driver and we might have a lot of
> changes for new feature.
> If we maintain as one driver, everytime we change the new MCT driver we should test
> all of SoC which doesn't have the new MCT. And vice versa.
If everyone added a new driver to avoid integrating with existing code,
we would have huge kernel with thousands of duplicated solutions. The
kernel also would be unmaintained.
Such arguments were brought before several times - "I don't want to
integrating with existing code", "My use case is different", "I would
need to test the other cases", "It's complicated for me".
Instead of pushing a new vendor driver you should integrate it with
existing code.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v1 1/2] clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2 driver for next Exynos SoC
@ 2021-10-25 8:18 ` Krzysztof Kozlowski
0 siblings, 0 replies; 40+ messages in thread
From: Krzysztof Kozlowski @ 2021-10-25 8:18 UTC (permalink / raw)
To: Youngmin Nam
Cc: daniel.lezcano, tglx, linux-kernel, linux-arm-kernel,
linux-samsung-soc, pullip.cho, hoony.yu, hajun.sung,
myung-su.cha
On 22/10/2021 06:21, Youngmin Nam wrote:
> On Thu, Oct 21, 2021 at 10:07:25AM +0200, Krzysztof Kozlowski wrote:
>> On 21/10/2021 10:26, Youngmin Nam wrote:
>>> On Thu, Oct 21, 2021 at 08:18:36AM +0200, Krzysztof Kozlowski wrote:
>>>> On 21/10/2021 08:18, Youngmin Nam wrote:
>>>>> Exynos MCT version 2 is composed of 1 FRC and 12 comparators.
>>>>> The 12 comparators can produces interrupts independently,
>>>>> so they can be used as local timer of each CPU.
>>>>>
>>>>> Signed-off-by: Youngmin Nam <youngmin.nam@samsung.com>
>>>>> ---
>>>>> drivers/clocksource/Kconfig | 6 +
>>>>> drivers/clocksource/Makefile | 1 +
>>>>> drivers/clocksource/exynos_mct_v2.c | 336 ++++++++++++++++++++++++++++
>>>>> drivers/clocksource/exynos_mct_v2.h | 74 ++++++
>>>>> 4 files changed, 417 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..8ac04dd7f713 100644
>>>>> --- a/drivers/clocksource/Kconfig
>>>>> +++ b/drivers/clocksource/Kconfig
>>>>> @@ -421,6 +421,12 @@ 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.
>>>>
>>> Okay
>>>>> + 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..2da6d5401629
>>>>> --- /dev/null
>>>>> +++ b/drivers/clocksource/exynos_mct_v2.c
>>>>> @@ -0,0 +1,336 @@
>>>>> +// 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("MCT: 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);
>>>>> +}
>>>>> +
>>>>> +/**
>>>>> + * exynos_read_count_32 - Read the lower 32-bits of the global counter
>>>>> + *
>>>>> + * This will read just the lower 32-bits of the global counter.
>>>>> + *
>>>>> + * Returns the number of cycles in the global counter (lower 32 bits).
>>>>> + */
>>>>
>>>> All this looks like a modification of Exynos MCT driver, so you should
>>>> extend that one instead. It does not look like we need two drivers.
>>>> Please integrate it into existing driver instead of sending a new piece
>>>> of code copied from vendor tree.
>>>>
>>> MCT version 2 is a completely different HW IP compared to previous MCT.
>>> The new MCT has a lot of different resister sets and there are many changes on programming guide.
>>> So we cannot share the previous code. At first, I also considered that way you mentioned,
>>> but it would be better to implement the driver seperately to maintain the new driver cleanly.
>>
>> We have several drivers supporting different devices and we avoid mostly
>> duplicating new ones. The different register layout is not the valid
>> reason - we support differences in several places. Just take a look at
>> Samsung PMIC drivers where register layout is quite different between
>> designs. Still one clock driver, one RTC driver and 2-3 regulator
>> drivers (for ~7 devices).
>>
>> Similarly to SoC clock, pinctrl, PMU and other drivers - we re-use
>> instead of duplicating entire driver.
>>
>> I am sorry but the argument that block is different is not enough. What
>> is exactly not compatible with current driver which could not be modeled
>> by different driver data (or structure with ops)?
>>
> I've checked samsung regulator driver and there are 3 types of driver as you mentioned.
> They are being maintained separately because they are not compatible each other.
That's not correct. We integrated 5 separate devices into s2mps11
regulator driver, around 7 devices into a MFD driver, 4 devices into RTC
driver and 4 devices into clock driver.
>
> These are comparison of previous MCT and new MCT.
> * Previous MCT
> - 4 Global timer + 8 Local timer
> - Clock Source is OSC only
>
> * New MCT
> - 1 Free Running Counter + 12 comparators
One FRC was also in previous MCT, wasn't it? It supported 4 comparators,
but FRC was only one.
> - Clock Sources are OSC and RTC
> - Programming guide is totally different comapared to previous MCT.
Thanks, I got it from the driver. Linux supports handling such
differences, including differences in register map. In the same time
just quick look at the code shows several re-used functions.
>
> MCTv2 is totally newly designed for the next Exynos Series.
> IP design, the way of operating and register configurations are different as well register layout.
We handle different register layouts without big issue. There are
several drivers showing this example, for example mentioned earlier
Samsung PMIC drivers. 4 different register layouts for RTC driver in one
driver and you mention here that two is some big difference?
The way of operating could be indeed a trouble but looking at the code
it is actually very, very similar.
> It is new generation of Exynos system timer. It's not compatible with the previous MCT.
> This is the start of implementation for the new MCT driver and we might have a lot of
> changes for new feature.
> If we maintain as one driver, everytime we change the new MCT driver we should test
> all of SoC which doesn't have the new MCT. And vice versa.
If everyone added a new driver to avoid integrating with existing code,
we would have huge kernel with thousands of duplicated solutions. The
kernel also would be unmaintained.
Such arguments were brought before several times - "I don't want to
integrating with existing code", "My use case is different", "I would
need to test the other cases", "It's complicated for me".
Instead of pushing a new vendor driver you should integrate it with
existing code.
Best regards,
Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v1 1/2] clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2 driver for next Exynos SoC
2021-10-25 8:18 ` Krzysztof Kozlowski
@ 2021-10-26 1:47 ` Youngmin Nam
-1 siblings, 0 replies; 40+ messages in thread
From: Youngmin Nam @ 2021-10-26 1:47 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: daniel.lezcano, tglx, linux-kernel, linux-arm-kernel,
linux-samsung-soc, pullip.cho, hoony.yu, hajun.sung,
myung-su.cha
[-- Attachment #1: Type: text/plain, Size: 9090 bytes --]
On Mon, Oct 25, 2021 at 10:18:04AM +0200, Krzysztof Kozlowski wrote:
> On 22/10/2021 06:21, Youngmin Nam wrote:
> > On Thu, Oct 21, 2021 at 10:07:25AM +0200, Krzysztof Kozlowski wrote:
> >> On 21/10/2021 10:26, Youngmin Nam wrote:
> >>> On Thu, Oct 21, 2021 at 08:18:36AM +0200, Krzysztof Kozlowski wrote:
> >>>> On 21/10/2021 08:18, Youngmin Nam wrote:
> >>>>> Exynos MCT version 2 is composed of 1 FRC and 12 comparators.
> >>>>> The 12 comparators can produces interrupts independently,
> >>>>> so they can be used as local timer of each CPU.
> >>>>>
> >>>>> Signed-off-by: Youngmin Nam <youngmin.nam@samsung.com>
> >>>>> ---
> >>>>> drivers/clocksource/Kconfig | 6 +
> >>>>> drivers/clocksource/Makefile | 1 +
> >>>>> drivers/clocksource/exynos_mct_v2.c | 336 ++++++++++++++++++++++++++++
> >>>>> drivers/clocksource/exynos_mct_v2.h | 74 ++++++
> >>>>> 4 files changed, 417 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..8ac04dd7f713 100644
> >>>>> --- a/drivers/clocksource/Kconfig
> >>>>> +++ b/drivers/clocksource/Kconfig
> >>>>> @@ -421,6 +421,12 @@ 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.
> >>>>
> >>> Okay
> >>>>> + 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..2da6d5401629
> >>>>> --- /dev/null
> >>>>> +++ b/drivers/clocksource/exynos_mct_v2.c
> >>>>> @@ -0,0 +1,336 @@
> >>>>> +// 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("MCT: 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);
> >>>>> +}
> >>>>> +
> >>>>> +/**
> >>>>> + * exynos_read_count_32 - Read the lower 32-bits of the global counter
> >>>>> + *
> >>>>> + * This will read just the lower 32-bits of the global counter.
> >>>>> + *
> >>>>> + * Returns the number of cycles in the global counter (lower 32 bits).
> >>>>> + */
> >>>>
> >>>> All this looks like a modification of Exynos MCT driver, so you should
> >>>> extend that one instead. It does not look like we need two drivers.
> >>>> Please integrate it into existing driver instead of sending a new piece
> >>>> of code copied from vendor tree.
> >>>>
> >>> MCT version 2 is a completely different HW IP compared to previous MCT.
> >>> The new MCT has a lot of different resister sets and there are many changes on programming guide.
> >>> So we cannot share the previous code. At first, I also considered that way you mentioned,
> >>> but it would be better to implement the driver seperately to maintain the new driver cleanly.
> >>
> >> We have several drivers supporting different devices and we avoid mostly
> >> duplicating new ones. The different register layout is not the valid
> >> reason - we support differences in several places. Just take a look at
> >> Samsung PMIC drivers where register layout is quite different between
> >> designs. Still one clock driver, one RTC driver and 2-3 regulator
> >> drivers (for ~7 devices).
> >>
> >> Similarly to SoC clock, pinctrl, PMU and other drivers - we re-use
> >> instead of duplicating entire driver.
> >>
> >> I am sorry but the argument that block is different is not enough. What
> >> is exactly not compatible with current driver which could not be modeled
> >> by different driver data (or structure with ops)?
> >>
> > I've checked samsung regulator driver and there are 3 types of driver as you mentioned.
> > They are being maintained separately because they are not compatible each other.
>
> That's not correct. We integrated 5 separate devices into s2mps11
> regulator driver, around 7 devices into a MFD driver, 4 devices into RTC
> driver and 4 devices into clock driver.
>
> >
> > These are comparison of previous MCT and new MCT.
> > * Previous MCT
> > - 4 Global timer + 8 Local timer
> > - Clock Source is OSC only
> >
> > * New MCT
> > - 1 Free Running Counter + 12 comparators
>
> One FRC was also in previous MCT, wasn't it? It supported 4 comparators,
> but FRC was only one.
>
> > - Clock Sources are OSC and RTC
> > - Programming guide is totally different comapared to previous MCT.
>
> Thanks, I got it from the driver. Linux supports handling such
> differences, including differences in register map. In the same time
> just quick look at the code shows several re-used functions.
>
> >
> > MCTv2 is totally newly designed for the next Exynos Series.
> > IP design, the way of operating and register configurations are different as well register layout.
>
> We handle different register layouts without big issue. There are
> several drivers showing this example, for example mentioned earlier
> Samsung PMIC drivers. 4 different register layouts for RTC driver in one
> driver and you mention here that two is some big difference?
>
> The way of operating could be indeed a trouble but looking at the code
> it is actually very, very similar.
>
> > It is new generation of Exynos system timer. It's not compatible with the previous MCT.
> > This is the start of implementation for the new MCT driver and we might have a lot of
> > changes for new feature.
> > If we maintain as one driver, everytime we change the new MCT driver we should test
> > all of SoC which doesn't have the new MCT. And vice versa.
>
> If everyone added a new driver to avoid integrating with existing code,
> we would have huge kernel with thousands of duplicated solutions. The
> kernel also would be unmaintained.
>
> Such arguments were brought before several times - "I don't want to
> integrating with existing code", "My use case is different", "I would
> need to test the other cases", "It's complicated for me".
>
> Instead of pushing a new vendor driver you should integrate it with
> existing code.
>
Let me ask you one question.
If we maintain as one driver, how can people who don't have the new MCT test the new driver?
> Best regards,
> Krzysztof
>
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v1 1/2] clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2 driver for next Exynos SoC
@ 2021-10-26 1:47 ` Youngmin Nam
0 siblings, 0 replies; 40+ messages in thread
From: Youngmin Nam @ 2021-10-26 1:47 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: daniel.lezcano, tglx, linux-kernel, linux-arm-kernel,
linux-samsung-soc, pullip.cho, hoony.yu, hajun.sung,
myung-su.cha
[-- Attachment #1: Type: text/plain, Size: 9090 bytes --]
On Mon, Oct 25, 2021 at 10:18:04AM +0200, Krzysztof Kozlowski wrote:
> On 22/10/2021 06:21, Youngmin Nam wrote:
> > On Thu, Oct 21, 2021 at 10:07:25AM +0200, Krzysztof Kozlowski wrote:
> >> On 21/10/2021 10:26, Youngmin Nam wrote:
> >>> On Thu, Oct 21, 2021 at 08:18:36AM +0200, Krzysztof Kozlowski wrote:
> >>>> On 21/10/2021 08:18, Youngmin Nam wrote:
> >>>>> Exynos MCT version 2 is composed of 1 FRC and 12 comparators.
> >>>>> The 12 comparators can produces interrupts independently,
> >>>>> so they can be used as local timer of each CPU.
> >>>>>
> >>>>> Signed-off-by: Youngmin Nam <youngmin.nam@samsung.com>
> >>>>> ---
> >>>>> drivers/clocksource/Kconfig | 6 +
> >>>>> drivers/clocksource/Makefile | 1 +
> >>>>> drivers/clocksource/exynos_mct_v2.c | 336 ++++++++++++++++++++++++++++
> >>>>> drivers/clocksource/exynos_mct_v2.h | 74 ++++++
> >>>>> 4 files changed, 417 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..8ac04dd7f713 100644
> >>>>> --- a/drivers/clocksource/Kconfig
> >>>>> +++ b/drivers/clocksource/Kconfig
> >>>>> @@ -421,6 +421,12 @@ 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.
> >>>>
> >>> Okay
> >>>>> + 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..2da6d5401629
> >>>>> --- /dev/null
> >>>>> +++ b/drivers/clocksource/exynos_mct_v2.c
> >>>>> @@ -0,0 +1,336 @@
> >>>>> +// 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("MCT: 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);
> >>>>> +}
> >>>>> +
> >>>>> +/**
> >>>>> + * exynos_read_count_32 - Read the lower 32-bits of the global counter
> >>>>> + *
> >>>>> + * This will read just the lower 32-bits of the global counter.
> >>>>> + *
> >>>>> + * Returns the number of cycles in the global counter (lower 32 bits).
> >>>>> + */
> >>>>
> >>>> All this looks like a modification of Exynos MCT driver, so you should
> >>>> extend that one instead. It does not look like we need two drivers.
> >>>> Please integrate it into existing driver instead of sending a new piece
> >>>> of code copied from vendor tree.
> >>>>
> >>> MCT version 2 is a completely different HW IP compared to previous MCT.
> >>> The new MCT has a lot of different resister sets and there are many changes on programming guide.
> >>> So we cannot share the previous code. At first, I also considered that way you mentioned,
> >>> but it would be better to implement the driver seperately to maintain the new driver cleanly.
> >>
> >> We have several drivers supporting different devices and we avoid mostly
> >> duplicating new ones. The different register layout is not the valid
> >> reason - we support differences in several places. Just take a look at
> >> Samsung PMIC drivers where register layout is quite different between
> >> designs. Still one clock driver, one RTC driver and 2-3 regulator
> >> drivers (for ~7 devices).
> >>
> >> Similarly to SoC clock, pinctrl, PMU and other drivers - we re-use
> >> instead of duplicating entire driver.
> >>
> >> I am sorry but the argument that block is different is not enough. What
> >> is exactly not compatible with current driver which could not be modeled
> >> by different driver data (or structure with ops)?
> >>
> > I've checked samsung regulator driver and there are 3 types of driver as you mentioned.
> > They are being maintained separately because they are not compatible each other.
>
> That's not correct. We integrated 5 separate devices into s2mps11
> regulator driver, around 7 devices into a MFD driver, 4 devices into RTC
> driver and 4 devices into clock driver.
>
> >
> > These are comparison of previous MCT and new MCT.
> > * Previous MCT
> > - 4 Global timer + 8 Local timer
> > - Clock Source is OSC only
> >
> > * New MCT
> > - 1 Free Running Counter + 12 comparators
>
> One FRC was also in previous MCT, wasn't it? It supported 4 comparators,
> but FRC was only one.
>
> > - Clock Sources are OSC and RTC
> > - Programming guide is totally different comapared to previous MCT.
>
> Thanks, I got it from the driver. Linux supports handling such
> differences, including differences in register map. In the same time
> just quick look at the code shows several re-used functions.
>
> >
> > MCTv2 is totally newly designed for the next Exynos Series.
> > IP design, the way of operating and register configurations are different as well register layout.
>
> We handle different register layouts without big issue. There are
> several drivers showing this example, for example mentioned earlier
> Samsung PMIC drivers. 4 different register layouts for RTC driver in one
> driver and you mention here that two is some big difference?
>
> The way of operating could be indeed a trouble but looking at the code
> it is actually very, very similar.
>
> > It is new generation of Exynos system timer. It's not compatible with the previous MCT.
> > This is the start of implementation for the new MCT driver and we might have a lot of
> > changes for new feature.
> > If we maintain as one driver, everytime we change the new MCT driver we should test
> > all of SoC which doesn't have the new MCT. And vice versa.
>
> If everyone added a new driver to avoid integrating with existing code,
> we would have huge kernel with thousands of duplicated solutions. The
> kernel also would be unmaintained.
>
> Such arguments were brought before several times - "I don't want to
> integrating with existing code", "My use case is different", "I would
> need to test the other cases", "It's complicated for me".
>
> Instead of pushing a new vendor driver you should integrate it with
> existing code.
>
Let me ask you one question.
If we maintain as one driver, how can people who don't have the new MCT test the new driver?
> Best regards,
> Krzysztof
>
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
[-- Attachment #3: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v1 1/2] clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2 driver for next Exynos SoC
2021-10-26 1:47 ` Youngmin Nam
@ 2021-10-26 7:10 ` Krzysztof Kozlowski
-1 siblings, 0 replies; 40+ messages in thread
From: Krzysztof Kozlowski @ 2021-10-26 7:10 UTC (permalink / raw)
To: Youngmin Nam
Cc: daniel.lezcano, tglx, linux-kernel, linux-arm-kernel,
linux-samsung-soc, pullip.cho, hoony.yu, hajun.sung,
myung-su.cha
On 26/10/2021 03:47, Youngmin Nam wrote:
>> If everyone added a new driver to avoid integrating with existing code,
>> we would have huge kernel with thousands of duplicated solutions. The
>> kernel also would be unmaintained.
>>
>> Such arguments were brought before several times - "I don't want to
>> integrating with existing code", "My use case is different", "I would
>> need to test the other cases", "It's complicated for me".
>>
>> Instead of pushing a new vendor driver you should integrate it with
>> existing code.
>>
> Let me ask you one question.
> If we maintain as one driver, how can people who don't have the new MCT test the new driver?
I assume you talk about a case when someone else later changes something
in the driver. Such person doesn't necessarily have to test it. The same
as in all other cases (Exynos MCT is not special here): just ask for
testing on platform one doesn't have.
Even if you submit this as separate driver, there is the exact same
problem. People will change the MCTv2 driver without access to hardware.
None of these differ for Exynos MCT from other drivers, e.g. mentioned
Samsung PMIC drivers, recently modified (by Will and Sam) the SoC clock
drivers or the ChipID drivers (changed by Chanho).
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v1 1/2] clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2 driver for next Exynos SoC
@ 2021-10-26 7:10 ` Krzysztof Kozlowski
0 siblings, 0 replies; 40+ messages in thread
From: Krzysztof Kozlowski @ 2021-10-26 7:10 UTC (permalink / raw)
To: Youngmin Nam
Cc: daniel.lezcano, tglx, linux-kernel, linux-arm-kernel,
linux-samsung-soc, pullip.cho, hoony.yu, hajun.sung,
myung-su.cha
On 26/10/2021 03:47, Youngmin Nam wrote:
>> If everyone added a new driver to avoid integrating with existing code,
>> we would have huge kernel with thousands of duplicated solutions. The
>> kernel also would be unmaintained.
>>
>> Such arguments were brought before several times - "I don't want to
>> integrating with existing code", "My use case is different", "I would
>> need to test the other cases", "It's complicated for me".
>>
>> Instead of pushing a new vendor driver you should integrate it with
>> existing code.
>>
> Let me ask you one question.
> If we maintain as one driver, how can people who don't have the new MCT test the new driver?
I assume you talk about a case when someone else later changes something
in the driver. Such person doesn't necessarily have to test it. The same
as in all other cases (Exynos MCT is not special here): just ask for
testing on platform one doesn't have.
Even if you submit this as separate driver, there is the exact same
problem. People will change the MCTv2 driver without access to hardware.
None of these differ for Exynos MCT from other drivers, e.g. mentioned
Samsung PMIC drivers, recently modified (by Will and Sam) the SoC clock
drivers or the ChipID drivers (changed by Chanho).
Best regards,
Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v1 1/2] clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2 driver for next Exynos SoC
2021-10-26 7:10 ` Krzysztof Kozlowski
@ 2021-10-26 10:45 ` Youngmin Nam
-1 siblings, 0 replies; 40+ messages in thread
From: Youngmin Nam @ 2021-10-26 10:45 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: daniel.lezcano, tglx, linux-kernel, linux-arm-kernel,
linux-samsung-soc, pullip.cho, hoony.yu, hajun.sung,
myung-su.cha, kgene
[-- Attachment #1: Type: text/plain, Size: 2011 bytes --]
On Tue, Oct 26, 2021 at 09:10:28AM +0200, Krzysztof Kozlowski wrote:
> On 26/10/2021 03:47, Youngmin Nam wrote:
> >> If everyone added a new driver to avoid integrating with existing code,
> >> we would have huge kernel with thousands of duplicated solutions. The
> >> kernel also would be unmaintained.
> >>
> >> Such arguments were brought before several times - "I don't want to
> >> integrating with existing code", "My use case is different", "I would
> >> need to test the other cases", "It's complicated for me".
> >>
> >> Instead of pushing a new vendor driver you should integrate it with
> >> existing code.
> >>
> > Let me ask you one question.
> > If we maintain as one driver, how can people who don't have the new MCT test the new driver?
>
> I assume you talk about a case when someone else later changes something
> in the driver. Such person doesn't necessarily have to test it. The same
> as in all other cases (Exynos MCT is not special here): just ask for
> testing on platform one doesn't have.
>
> Even if you submit this as separate driver, there is the exact same
> problem. People will change the MCTv2 driver without access to hardware.
>
Yes, I can test the new MCT driver if someone ask for testing after modifying the new driver.
But in this case, we don't need to test the previous MCT driver. We have only to test the new MCT driver.
> None of these differ for Exynos MCT from other drivers, e.g. mentioned
> Samsung PMIC drivers, recently modified (by Will and Sam) the SoC clock
> drivers or the ChipID drivers (changed by Chanho).
From HW point of view, the previous MCT is almost 10-year-old IP without any major change and
it will not be used on next new Exynos SoC.
MCTv2 is the totally newly designed IP and it will replace the Exynos system timer.
Device driver would be dependent with H/W. We are going to apply a lot of changes for this new MCT.
For maintenance, I think we should separate the new MCT driver for maintenance.
>
>
>
> Best regards,
> Krzysztof
>
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v1 1/2] clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2 driver for next Exynos SoC
@ 2021-10-26 10:45 ` Youngmin Nam
0 siblings, 0 replies; 40+ messages in thread
From: Youngmin Nam @ 2021-10-26 10:45 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: daniel.lezcano, tglx, linux-kernel, linux-arm-kernel,
linux-samsung-soc, pullip.cho, hoony.yu, hajun.sung,
myung-su.cha, kgene
[-- Attachment #1: Type: text/plain, Size: 2011 bytes --]
On Tue, Oct 26, 2021 at 09:10:28AM +0200, Krzysztof Kozlowski wrote:
> On 26/10/2021 03:47, Youngmin Nam wrote:
> >> If everyone added a new driver to avoid integrating with existing code,
> >> we would have huge kernel with thousands of duplicated solutions. The
> >> kernel also would be unmaintained.
> >>
> >> Such arguments were brought before several times - "I don't want to
> >> integrating with existing code", "My use case is different", "I would
> >> need to test the other cases", "It's complicated for me".
> >>
> >> Instead of pushing a new vendor driver you should integrate it with
> >> existing code.
> >>
> > Let me ask you one question.
> > If we maintain as one driver, how can people who don't have the new MCT test the new driver?
>
> I assume you talk about a case when someone else later changes something
> in the driver. Such person doesn't necessarily have to test it. The same
> as in all other cases (Exynos MCT is not special here): just ask for
> testing on platform one doesn't have.
>
> Even if you submit this as separate driver, there is the exact same
> problem. People will change the MCTv2 driver without access to hardware.
>
Yes, I can test the new MCT driver if someone ask for testing after modifying the new driver.
But in this case, we don't need to test the previous MCT driver. We have only to test the new MCT driver.
> None of these differ for Exynos MCT from other drivers, e.g. mentioned
> Samsung PMIC drivers, recently modified (by Will and Sam) the SoC clock
> drivers or the ChipID drivers (changed by Chanho).
From HW point of view, the previous MCT is almost 10-year-old IP without any major change and
it will not be used on next new Exynos SoC.
MCTv2 is the totally newly designed IP and it will replace the Exynos system timer.
Device driver would be dependent with H/W. We are going to apply a lot of changes for this new MCT.
For maintenance, I think we should separate the new MCT driver for maintenance.
>
>
>
> Best regards,
> Krzysztof
>
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
[-- Attachment #3: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v1 1/2] clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2 driver for next Exynos SoC
2021-10-26 10:45 ` Youngmin Nam
@ 2021-10-26 11:00 ` Krzysztof Kozlowski
-1 siblings, 0 replies; 40+ messages in thread
From: Krzysztof Kozlowski @ 2021-10-26 11:00 UTC (permalink / raw)
To: Youngmin Nam, Will Deacon, Mark Rutland, daniel.lezcano
Cc: tglx, linux-kernel, linux-arm-kernel, linux-samsung-soc,
pullip.cho, hoony.yu, hajun.sung, myung-su.cha, kgene
On 26/10/2021 12:45, Youngmin Nam wrote:
> On Tue, Oct 26, 2021 at 09:10:28AM +0200, Krzysztof Kozlowski wrote:
>> On 26/10/2021 03:47, Youngmin Nam wrote:
>>>> If everyone added a new driver to avoid integrating with existing code,
>>>> we would have huge kernel with thousands of duplicated solutions. The
>>>> kernel also would be unmaintained.
>>>>
>>>> Such arguments were brought before several times - "I don't want to
>>>> integrating with existing code", "My use case is different", "I would
>>>> need to test the other cases", "It's complicated for me".
>>>>
>>>> Instead of pushing a new vendor driver you should integrate it with
>>>> existing code.
>>>>
>>> Let me ask you one question.
>>> If we maintain as one driver, how can people who don't have the new MCT test the new driver?
>>
>> I assume you talk about a case when someone else later changes something
>> in the driver. Such person doesn't necessarily have to test it. The same
>> as in all other cases (Exynos MCT is not special here): just ask for
>> testing on platform one doesn't have.
>>
>> Even if you submit this as separate driver, there is the exact same
>> problem. People will change the MCTv2 driver without access to hardware.
>>
> Yes, I can test the new MCT driver if someone ask for testing after modifying the new driver.
> But in this case, we don't need to test the previous MCT driver. We have only to test the new MCT driver.
Like with everything in Linux kernel. We merge instead of duplicate.
It's not an argument.
>> None of these differ for Exynos MCT from other drivers, e.g. mentioned
>> Samsung PMIC drivers, recently modified (by Will and Sam) the SoC clock
>> drivers or the ChipID drivers (changed by Chanho).
> From HW point of view, the previous MCT is almost 10-year-old IP without any major change and
> it will not be used on next new Exynos SoC.
> MCTv2 is the totally newly designed IP and it will replace the Exynos system timer.
> Device driver would be dependent with H/W. We are going to apply a lot of changes for this new MCT.
> For maintenance, I think we should separate the new MCT driver for maintenance.
>
There are several similarities which actually suggest that you
exaggerate the differences.
The number of interrupts is the same (4+8 in older one, 12 in new one...).
You assign the MCT priority also as higher than Architected Timer
(+Cc Will and Mark - is it ok for you?)
evt->rating = 500; /* use value higher than ARM arch timer *
All these point that block is not different. Again, let me repeat, we
support old Samsung PMICs with new Samsung PMICs in one driver. Even
though the "old one" won't be changed, as you mentioned here. The same
Samsung SoC clock drivers are used for old Exynos and for new ones...
Similarly to pinctrl drivers. The same ChipId.
Everywhere we follow the same concept of unification instead of
duplication. Maybe Exynos MCT timer is an exception but you did not
provide any arguments supporting this. Why Exynos MCTv2 should be
treated differently than Exynos850 clocks, chipid, pinctrl and other blocks?
Daniel,
Any preferences from you? Integrating MCT into existing driver (thus
growing it) or having a new one?
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v1 1/2] clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2 driver for next Exynos SoC
@ 2021-10-26 11:00 ` Krzysztof Kozlowski
0 siblings, 0 replies; 40+ messages in thread
From: Krzysztof Kozlowski @ 2021-10-26 11:00 UTC (permalink / raw)
To: Youngmin Nam, Will Deacon, Mark Rutland, daniel.lezcano
Cc: tglx, linux-kernel, linux-arm-kernel, linux-samsung-soc,
pullip.cho, hoony.yu, hajun.sung, myung-su.cha, kgene
On 26/10/2021 12:45, Youngmin Nam wrote:
> On Tue, Oct 26, 2021 at 09:10:28AM +0200, Krzysztof Kozlowski wrote:
>> On 26/10/2021 03:47, Youngmin Nam wrote:
>>>> If everyone added a new driver to avoid integrating with existing code,
>>>> we would have huge kernel with thousands of duplicated solutions. The
>>>> kernel also would be unmaintained.
>>>>
>>>> Such arguments were brought before several times - "I don't want to
>>>> integrating with existing code", "My use case is different", "I would
>>>> need to test the other cases", "It's complicated for me".
>>>>
>>>> Instead of pushing a new vendor driver you should integrate it with
>>>> existing code.
>>>>
>>> Let me ask you one question.
>>> If we maintain as one driver, how can people who don't have the new MCT test the new driver?
>>
>> I assume you talk about a case when someone else later changes something
>> in the driver. Such person doesn't necessarily have to test it. The same
>> as in all other cases (Exynos MCT is not special here): just ask for
>> testing on platform one doesn't have.
>>
>> Even if you submit this as separate driver, there is the exact same
>> problem. People will change the MCTv2 driver without access to hardware.
>>
> Yes, I can test the new MCT driver if someone ask for testing after modifying the new driver.
> But in this case, we don't need to test the previous MCT driver. We have only to test the new MCT driver.
Like with everything in Linux kernel. We merge instead of duplicate.
It's not an argument.
>> None of these differ for Exynos MCT from other drivers, e.g. mentioned
>> Samsung PMIC drivers, recently modified (by Will and Sam) the SoC clock
>> drivers or the ChipID drivers (changed by Chanho).
> From HW point of view, the previous MCT is almost 10-year-old IP without any major change and
> it will not be used on next new Exynos SoC.
> MCTv2 is the totally newly designed IP and it will replace the Exynos system timer.
> Device driver would be dependent with H/W. We are going to apply a lot of changes for this new MCT.
> For maintenance, I think we should separate the new MCT driver for maintenance.
>
There are several similarities which actually suggest that you
exaggerate the differences.
The number of interrupts is the same (4+8 in older one, 12 in new one...).
You assign the MCT priority also as higher than Architected Timer
(+Cc Will and Mark - is it ok for you?)
evt->rating = 500; /* use value higher than ARM arch timer *
All these point that block is not different. Again, let me repeat, we
support old Samsung PMICs with new Samsung PMICs in one driver. Even
though the "old one" won't be changed, as you mentioned here. The same
Samsung SoC clock drivers are used for old Exynos and for new ones...
Similarly to pinctrl drivers. The same ChipId.
Everywhere we follow the same concept of unification instead of
duplication. Maybe Exynos MCT timer is an exception but you did not
provide any arguments supporting this. Why Exynos MCTv2 should be
treated differently than Exynos850 clocks, chipid, pinctrl and other blocks?
Daniel,
Any preferences from you? Integrating MCT into existing driver (thus
growing it) or having a new one?
Best regards,
Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread[parent not found: <CGME20211027011125epcas2p2916524051416ede854b750c91a19073b@epcas2p2.samsung.com>]
* Re: [PATCH v1 1/2] clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2 driver for next Exynos SoC
[not found] ` <CGME20211027011125epcas2p2916524051416ede854b750c91a19073b@epcas2p2.samsung.com>
@ 2021-10-27 1:38 ` Youngmin Nam
0 siblings, 0 replies; 40+ messages in thread
From: Youngmin Nam @ 2021-10-27 1:38 UTC (permalink / raw)
To: krzysztof.kozlowski, will, mark.rutland, daniel.lezcano
Cc: tglx, linux-kernel, linux-arm-kernel, linux-samsung-soc,
pullip.cho, hoony.yu, hajun.sung, myung-su.cha, kgene
[-- Attachment #1: Type: text/plain, Size: 5669 bytes --]
On Tue, Oct 26, 2021 at 01:00:51PM +0200, Krzysztof Kozlowski wrote:
> On 26/10/2021 12:45, Youngmin Nam wrote:
> > On Tue, Oct 26, 2021 at 09:10:28AM +0200, Krzysztof Kozlowski wrote:
> >> On 26/10/2021 03:47, Youngmin Nam wrote:
> >>>> If everyone added a new driver to avoid integrating with existing code,
> >>>> we would have huge kernel with thousands of duplicated solutions. The
> >>>> kernel also would be unmaintained.
> >>>>
> >>>> Such arguments were brought before several times - "I don't want to
> >>>> integrating with existing code", "My use case is different", "I would
> >>>> need to test the other cases", "It's complicated for me".
> >>>>
> >>>> Instead of pushing a new vendor driver you should integrate it with
> >>>> existing code.
> >>>>
> >>> Let me ask you one question.
> >>> If we maintain as one driver, how can people who don't have the new MCT test the new driver?
> >>
> >> I assume you talk about a case when someone else later changes something
> >> in the driver. Such person doesn't necessarily have to test it. The same
> >> as in all other cases (Exynos MCT is not special here): just ask for
> >> testing on platform one doesn't have.
> >>
> >> Even if you submit this as separate driver, there is the exact same
> >> problem. People will change the MCTv2 driver without access to hardware.
> >>
> > Yes, I can test the new MCT driver if someone ask for testing after modifying the new driver.
> > But in this case, we don't need to test the previous MCT driver. We have only to test the new MCT driver.
>
> Like with everything in Linux kernel. We merge instead of duplicate.
> It's not an argument.
>
> >> None of these differ for Exynos MCT from other drivers, e.g. mentioned
> >> Samsung PMIC drivers, recently modified (by Will and Sam) the SoC clock
> >> drivers or the ChipID drivers (changed by Chanho).
> > From HW point of view, the previous MCT is almost 10-year-old IP without any major change and
> > it will not be used on next new Exynos SoC.
> > MCTv2 is the totally newly designed IP and it will replace the Exynos system timer.
> > Device driver would be dependent with H/W. We are going to apply a lot of changes for this new MCT.
> > For maintenance, I think we should separate the new MCT driver for maintenance.
> >
>
> There are several similarities which actually suggest that you
> exaggerate the differences.
>
> The number of interrupts is the same (4+8 in older one, 12 in new one...).
I didn't "exaggerate" at all.
The numer of interrups is the same. But their usage is completely different.
The type of each timer is different.
And previous MCT can only support upto 8 cores.
* MCTv1 (Let me call previous MCT as MCTv1)
- 4 global timer + 8 local timer
- Global timer and local timer are totally different.
- 4 global timer have only one 64bit FRC that serves as the "up-counter" with 4 "comparators"
- 8 local timer have 8 of 32bit FRC that serves as the "down-counter" without any "comparators".(just expire timer)
- local timer can be used as per-cpu event timer, so it can only support upto 8 cores.
* MCTv2
- There are no global timer and local timer anymore.
- 1 of 64bit FRC that serves as "up-counter" (just counter without "comparators")
- 12 comaprators (These are not "counter") can be used as per-cpu event timer so that it can support upto 12 cores.
- RTC source can be used as backup source.
> You assign the MCT priority also as higher than Architected Timer
> (+Cc Will and Mark - is it ok for you?)
> evt->rating = 500; /* use value higher than ARM arch timer *
>
Yes, this is absolutely correct on event timer.
We cannot use arm arch timer which is operating based on PPI as per-cpu event timer because of poewr mode.
We have to use SPI for per-cpu timer interrupt. (This is the same in all Exynos platform)
> All these point that block is not different. Again, let me repeat, we
> support old Samsung PMICs with new Samsung PMICs in one driver. Even
> though the "old one" won't be changed, as you mentioned here. The same
> Samsung SoC clock drivers are used for old Exynos and for new ones...
> Similarly to pinctrl drivers. The same ChipId.
>
> Everywhere we follow the same concept of unification instead of
> duplication. Maybe Exynos MCT timer is an exception but you did not
> provide any arguments supporting this. Why Exynos MCTv2 should be
> treated differently than Exynos850 clocks, chipid, pinctrl and other blocks?
>
If MCTv2 has only changes in register layout, I can consider merging work.
But this is not that case.
You gave a example with PMIC, SoC clock, Pinctrl, ChipId.
These H/W IP have only changes in register layout which came from difference of each SoC.
Were these H/W IP version changed?
Were these H/W IP control method changed ?
No. It only has minor chagnes not major changes.
* PMIC - controls the PMIC reigster with I2C interface regarding their SoC usecase.
- there is no changes on H/W control method itself.
* SoC Clock - changes only in register layout regarding SoC
- Clock control method still the same.
* Pinctrl - changes only in gpio pin register layout (pin number, pin type, pin map..) regarding SoC.
- Is there any changes on control method ?
* Chipid - This is very simple H/W IP. It only supports unique chip id value with read-only register.
- It really only have changes in register layout.
MCTv2 is different.
Not only register layout but also it's control method has to be changed regarding H/W difference.
> Daniel,
> Any preferences from you? Integrating MCT into existing driver (thus
> growing it) or having a new one?
>
> Best regards,
> Krzysztof
>
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v1 1/2] clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2 driver for next Exynos SoC
@ 2021-10-27 1:38 ` Youngmin Nam
0 siblings, 0 replies; 40+ messages in thread
From: Youngmin Nam @ 2021-10-27 1:38 UTC (permalink / raw)
To: krzysztof.kozlowski, will, mark.rutland, daniel.lezcano
Cc: tglx, linux-kernel, linux-arm-kernel, linux-samsung-soc,
pullip.cho, hoony.yu, hajun.sung, myung-su.cha, kgene
[-- Attachment #1: Type: text/plain, Size: 5669 bytes --]
On Tue, Oct 26, 2021 at 01:00:51PM +0200, Krzysztof Kozlowski wrote:
> On 26/10/2021 12:45, Youngmin Nam wrote:
> > On Tue, Oct 26, 2021 at 09:10:28AM +0200, Krzysztof Kozlowski wrote:
> >> On 26/10/2021 03:47, Youngmin Nam wrote:
> >>>> If everyone added a new driver to avoid integrating with existing code,
> >>>> we would have huge kernel with thousands of duplicated solutions. The
> >>>> kernel also would be unmaintained.
> >>>>
> >>>> Such arguments were brought before several times - "I don't want to
> >>>> integrating with existing code", "My use case is different", "I would
> >>>> need to test the other cases", "It's complicated for me".
> >>>>
> >>>> Instead of pushing a new vendor driver you should integrate it with
> >>>> existing code.
> >>>>
> >>> Let me ask you one question.
> >>> If we maintain as one driver, how can people who don't have the new MCT test the new driver?
> >>
> >> I assume you talk about a case when someone else later changes something
> >> in the driver. Such person doesn't necessarily have to test it. The same
> >> as in all other cases (Exynos MCT is not special here): just ask for
> >> testing on platform one doesn't have.
> >>
> >> Even if you submit this as separate driver, there is the exact same
> >> problem. People will change the MCTv2 driver without access to hardware.
> >>
> > Yes, I can test the new MCT driver if someone ask for testing after modifying the new driver.
> > But in this case, we don't need to test the previous MCT driver. We have only to test the new MCT driver.
>
> Like with everything in Linux kernel. We merge instead of duplicate.
> It's not an argument.
>
> >> None of these differ for Exynos MCT from other drivers, e.g. mentioned
> >> Samsung PMIC drivers, recently modified (by Will and Sam) the SoC clock
> >> drivers or the ChipID drivers (changed by Chanho).
> > From HW point of view, the previous MCT is almost 10-year-old IP without any major change and
> > it will not be used on next new Exynos SoC.
> > MCTv2 is the totally newly designed IP and it will replace the Exynos system timer.
> > Device driver would be dependent with H/W. We are going to apply a lot of changes for this new MCT.
> > For maintenance, I think we should separate the new MCT driver for maintenance.
> >
>
> There are several similarities which actually suggest that you
> exaggerate the differences.
>
> The number of interrupts is the same (4+8 in older one, 12 in new one...).
I didn't "exaggerate" at all.
The numer of interrups is the same. But their usage is completely different.
The type of each timer is different.
And previous MCT can only support upto 8 cores.
* MCTv1 (Let me call previous MCT as MCTv1)
- 4 global timer + 8 local timer
- Global timer and local timer are totally different.
- 4 global timer have only one 64bit FRC that serves as the "up-counter" with 4 "comparators"
- 8 local timer have 8 of 32bit FRC that serves as the "down-counter" without any "comparators".(just expire timer)
- local timer can be used as per-cpu event timer, so it can only support upto 8 cores.
* MCTv2
- There are no global timer and local timer anymore.
- 1 of 64bit FRC that serves as "up-counter" (just counter without "comparators")
- 12 comaprators (These are not "counter") can be used as per-cpu event timer so that it can support upto 12 cores.
- RTC source can be used as backup source.
> You assign the MCT priority also as higher than Architected Timer
> (+Cc Will and Mark - is it ok for you?)
> evt->rating = 500; /* use value higher than ARM arch timer *
>
Yes, this is absolutely correct on event timer.
We cannot use arm arch timer which is operating based on PPI as per-cpu event timer because of poewr mode.
We have to use SPI for per-cpu timer interrupt. (This is the same in all Exynos platform)
> All these point that block is not different. Again, let me repeat, we
> support old Samsung PMICs with new Samsung PMICs in one driver. Even
> though the "old one" won't be changed, as you mentioned here. The same
> Samsung SoC clock drivers are used for old Exynos and for new ones...
> Similarly to pinctrl drivers. The same ChipId.
>
> Everywhere we follow the same concept of unification instead of
> duplication. Maybe Exynos MCT timer is an exception but you did not
> provide any arguments supporting this. Why Exynos MCTv2 should be
> treated differently than Exynos850 clocks, chipid, pinctrl and other blocks?
>
If MCTv2 has only changes in register layout, I can consider merging work.
But this is not that case.
You gave a example with PMIC, SoC clock, Pinctrl, ChipId.
These H/W IP have only changes in register layout which came from difference of each SoC.
Were these H/W IP version changed?
Were these H/W IP control method changed ?
No. It only has minor chagnes not major changes.
* PMIC - controls the PMIC reigster with I2C interface regarding their SoC usecase.
- there is no changes on H/W control method itself.
* SoC Clock - changes only in register layout regarding SoC
- Clock control method still the same.
* Pinctrl - changes only in gpio pin register layout (pin number, pin type, pin map..) regarding SoC.
- Is there any changes on control method ?
* Chipid - This is very simple H/W IP. It only supports unique chip id value with read-only register.
- It really only have changes in register layout.
MCTv2 is different.
Not only register layout but also it's control method has to be changed regarding H/W difference.
> Daniel,
> Any preferences from you? Integrating MCT into existing driver (thus
> growing it) or having a new one?
>
> Best regards,
> Krzysztof
>
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
[-- Attachment #3: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v1 1/2] clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2 driver for next Exynos SoC
2021-10-27 1:38 ` Youngmin Nam
@ 2021-10-27 6:39 ` Krzysztof Kozlowski
-1 siblings, 0 replies; 40+ messages in thread
From: Krzysztof Kozlowski @ 2021-10-27 6:39 UTC (permalink / raw)
To: Youngmin Nam, will, mark.rutland, daniel.lezcano
Cc: tglx, linux-kernel, linux-arm-kernel, linux-samsung-soc,
pullip.cho, hoony.yu, hajun.sung, myung-su.cha, kgene
On 27/10/2021 03:38, Youngmin Nam wrote:
> On Tue, Oct 26, 2021 at 01:00:51PM +0200, Krzysztof Kozlowski wrote:
>> On 26/10/2021 12:45, Youngmin Nam wrote:
>>> On Tue, Oct 26, 2021 at 09:10:28AM +0200, Krzysztof Kozlowski wrote:
>>>> On 26/10/2021 03:47, Youngmin Nam wrote:
>>>>>> If everyone added a new driver to avoid integrating with existing code,
>>>>>> we would have huge kernel with thousands of duplicated solutions. The
>>>>>> kernel also would be unmaintained.
>>>>>>
>>>>>> Such arguments were brought before several times - "I don't want to
>>>>>> integrating with existing code", "My use case is different", "I would
>>>>>> need to test the other cases", "It's complicated for me".
>>>>>>
>>>>>> Instead of pushing a new vendor driver you should integrate it with
>>>>>> existing code.
>>>>>>
>>>>> Let me ask you one question.
>>>>> If we maintain as one driver, how can people who don't have the new MCT test the new driver?
>>>>
>>>> I assume you talk about a case when someone else later changes something
>>>> in the driver. Such person doesn't necessarily have to test it. The same
>>>> as in all other cases (Exynos MCT is not special here): just ask for
>>>> testing on platform one doesn't have.
>>>>
>>>> Even if you submit this as separate driver, there is the exact same
>>>> problem. People will change the MCTv2 driver without access to hardware.
>>>>
>>> Yes, I can test the new MCT driver if someone ask for testing after modifying the new driver.
>>> But in this case, we don't need to test the previous MCT driver. We have only to test the new MCT driver.
>>
>> Like with everything in Linux kernel. We merge instead of duplicate.
>> It's not an argument.
>>
>>>> None of these differ for Exynos MCT from other drivers, e.g. mentioned
>>>> Samsung PMIC drivers, recently modified (by Will and Sam) the SoC clock
>>>> drivers or the ChipID drivers (changed by Chanho).
>>> From HW point of view, the previous MCT is almost 10-year-old IP without any major change and
>>> it will not be used on next new Exynos SoC.
>>> MCTv2 is the totally newly designed IP and it will replace the Exynos system timer.
>>> Device driver would be dependent with H/W. We are going to apply a lot of changes for this new MCT.
>>> For maintenance, I think we should separate the new MCT driver for maintenance.
>>>
>>
>> There are several similarities which actually suggest that you
>> exaggerate the differences.
>>
>> The number of interrupts is the same (4+8 in older one, 12 in new one...).
>
> I didn't "exaggerate" at all.
> The numer of interrups is the same. But their usage is completely different.
> The type of each timer is different.
> And previous MCT can only support upto 8 cores.
>
> * MCTv1 (Let me call previous MCT as MCTv1)
> - 4 global timer + 8 local timer
> - Global timer and local timer are totally different.
> - 4 global timer have only one 64bit FRC that serves as the "up-counter" with 4 "comparators"
> - 8 local timer have 8 of 32bit FRC that serves as the "down-counter" without any "comparators".(just expire timer)
> - local timer can be used as per-cpu event timer, so it can only support upto 8 cores.
>
> * MCTv2
> - There are no global timer and local timer anymore.
> - 1 of 64bit FRC that serves as "up-counter" (just counter without "comparators")
> - 12 comaprators (These are not "counter") can be used as per-cpu event timer so that it can support upto 12 cores.
> - RTC source can be used as backup source.
>
>> You assign the MCT priority also as higher than Architected Timer
>> (+Cc Will and Mark - is it ok for you?)
>> evt->rating = 500; /* use value higher than ARM arch timer *
>>
> Yes, this is absolutely correct on event timer.
> We cannot use arm arch timer which is operating based on PPI as per-cpu event timer because of poewr mode.
> We have to use SPI for per-cpu timer interrupt. (This is the same in all Exynos platform)
No, this is not correct, was explained several times and it is one of
the reasons why I am holding back to reuse of existing driver. You
copied few 32-bit ARM (ARMv7) solutions from old MCT driver into a new
one which is only 64-bit. These 32-bit solutions are some optimizations
or hacks matching 32-bit ARM Exynos processors. If you copy them to
entirely new driver for entirely different IP block, it means this is
not entirely different IP block.
Therefore I see a point now in having a Exynos MCTv2 driver for new IP
blocks after removing all that legacy 32-bit ARM stuff.
Therefore:
> evt->rating = 350;
Not 500. Use the same value as old timer driver for ARMv8. See previous
discussions and commits from Marek and Will.
Other:
> static u32 exynos_read_count_32(void)
This is u64, not u32. Get rid of 32-bit optimization/hack or explain it
similarly as Doug did (40 lines of reasoning).
> .mask = CLOCKSOURCE_MASK(32),
Mask is 64.
These are the 32-bit legacies I found now (maybe there are more...).
Don't copy them if this is a new driver getting rid of legacy.
>
>> All these point that block is not different. Again, let me repeat, we
>> support old Samsung PMICs with new Samsung PMICs in one driver. Even
>> though the "old one" won't be changed, as you mentioned here. The same
>> Samsung SoC clock drivers are used for old Exynos and for new ones...
>> Similarly to pinctrl drivers. The same ChipId.
>>
>> Everywhere we follow the same concept of unification instead of
>> duplication. Maybe Exynos MCT timer is an exception but you did not
>> provide any arguments supporting this. Why Exynos MCTv2 should be
>> treated differently than Exynos850 clocks, chipid, pinctrl and other blocks?
>>
>
> If MCTv2 has only changes in register layout, I can consider merging work.
> But this is not that case.
>
> You gave a example with PMIC, SoC clock, Pinctrl, ChipId.
> These H/W IP have only changes in register layout which came from difference of each SoC.
>
> Were these H/W IP version changed?
> Were these H/W IP control method changed ?
> No. It only has minor chagnes not major changes.
>
> * PMIC - controls the PMIC reigster with I2C interface regarding their SoC usecase.
> - there is no changes on H/W control method itself.
>
> * SoC Clock - changes only in register layout regarding SoC
> - Clock control method still the same.
>
> * Pinctrl - changes only in gpio pin register layout (pin number, pin type, pin map..) regarding SoC.
> - Is there any changes on control method ?
>
> * Chipid - This is very simple H/W IP. It only supports unique chip id value with read-only register.
> - It really only have changes in register layout.
>
> MCTv2 is different.
> Not only register layout but also it's control method has to be changed regarding H/W difference.
Yes, I see some differences in HW control which we also solve in several
other cases through structure ops. Indeed here it looks like the number
of differences in control is bigger than in other cases.
If Daniel is okay in having two drivers and you get rid of all 32-bit
legacy (including ordering against architected timers), I am fine with it.
>> Daniel,
>> Any preferences from you? Integrating MCT into existing driver (thus
>> growing it) or having a new one?
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v1 1/2] clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2 driver for next Exynos SoC
@ 2021-10-27 6:39 ` Krzysztof Kozlowski
0 siblings, 0 replies; 40+ messages in thread
From: Krzysztof Kozlowski @ 2021-10-27 6:39 UTC (permalink / raw)
To: Youngmin Nam, will, mark.rutland, daniel.lezcano
Cc: tglx, linux-kernel, linux-arm-kernel, linux-samsung-soc,
pullip.cho, hoony.yu, hajun.sung, myung-su.cha, kgene
On 27/10/2021 03:38, Youngmin Nam wrote:
> On Tue, Oct 26, 2021 at 01:00:51PM +0200, Krzysztof Kozlowski wrote:
>> On 26/10/2021 12:45, Youngmin Nam wrote:
>>> On Tue, Oct 26, 2021 at 09:10:28AM +0200, Krzysztof Kozlowski wrote:
>>>> On 26/10/2021 03:47, Youngmin Nam wrote:
>>>>>> If everyone added a new driver to avoid integrating with existing code,
>>>>>> we would have huge kernel with thousands of duplicated solutions. The
>>>>>> kernel also would be unmaintained.
>>>>>>
>>>>>> Such arguments were brought before several times - "I don't want to
>>>>>> integrating with existing code", "My use case is different", "I would
>>>>>> need to test the other cases", "It's complicated for me".
>>>>>>
>>>>>> Instead of pushing a new vendor driver you should integrate it with
>>>>>> existing code.
>>>>>>
>>>>> Let me ask you one question.
>>>>> If we maintain as one driver, how can people who don't have the new MCT test the new driver?
>>>>
>>>> I assume you talk about a case when someone else later changes something
>>>> in the driver. Such person doesn't necessarily have to test it. The same
>>>> as in all other cases (Exynos MCT is not special here): just ask for
>>>> testing on platform one doesn't have.
>>>>
>>>> Even if you submit this as separate driver, there is the exact same
>>>> problem. People will change the MCTv2 driver without access to hardware.
>>>>
>>> Yes, I can test the new MCT driver if someone ask for testing after modifying the new driver.
>>> But in this case, we don't need to test the previous MCT driver. We have only to test the new MCT driver.
>>
>> Like with everything in Linux kernel. We merge instead of duplicate.
>> It's not an argument.
>>
>>>> None of these differ for Exynos MCT from other drivers, e.g. mentioned
>>>> Samsung PMIC drivers, recently modified (by Will and Sam) the SoC clock
>>>> drivers or the ChipID drivers (changed by Chanho).
>>> From HW point of view, the previous MCT is almost 10-year-old IP without any major change and
>>> it will not be used on next new Exynos SoC.
>>> MCTv2 is the totally newly designed IP and it will replace the Exynos system timer.
>>> Device driver would be dependent with H/W. We are going to apply a lot of changes for this new MCT.
>>> For maintenance, I think we should separate the new MCT driver for maintenance.
>>>
>>
>> There are several similarities which actually suggest that you
>> exaggerate the differences.
>>
>> The number of interrupts is the same (4+8 in older one, 12 in new one...).
>
> I didn't "exaggerate" at all.
> The numer of interrups is the same. But their usage is completely different.
> The type of each timer is different.
> And previous MCT can only support upto 8 cores.
>
> * MCTv1 (Let me call previous MCT as MCTv1)
> - 4 global timer + 8 local timer
> - Global timer and local timer are totally different.
> - 4 global timer have only one 64bit FRC that serves as the "up-counter" with 4 "comparators"
> - 8 local timer have 8 of 32bit FRC that serves as the "down-counter" without any "comparators".(just expire timer)
> - local timer can be used as per-cpu event timer, so it can only support upto 8 cores.
>
> * MCTv2
> - There are no global timer and local timer anymore.
> - 1 of 64bit FRC that serves as "up-counter" (just counter without "comparators")
> - 12 comaprators (These are not "counter") can be used as per-cpu event timer so that it can support upto 12 cores.
> - RTC source can be used as backup source.
>
>> You assign the MCT priority also as higher than Architected Timer
>> (+Cc Will and Mark - is it ok for you?)
>> evt->rating = 500; /* use value higher than ARM arch timer *
>>
> Yes, this is absolutely correct on event timer.
> We cannot use arm arch timer which is operating based on PPI as per-cpu event timer because of poewr mode.
> We have to use SPI for per-cpu timer interrupt. (This is the same in all Exynos platform)
No, this is not correct, was explained several times and it is one of
the reasons why I am holding back to reuse of existing driver. You
copied few 32-bit ARM (ARMv7) solutions from old MCT driver into a new
one which is only 64-bit. These 32-bit solutions are some optimizations
or hacks matching 32-bit ARM Exynos processors. If you copy them to
entirely new driver for entirely different IP block, it means this is
not entirely different IP block.
Therefore I see a point now in having a Exynos MCTv2 driver for new IP
blocks after removing all that legacy 32-bit ARM stuff.
Therefore:
> evt->rating = 350;
Not 500. Use the same value as old timer driver for ARMv8. See previous
discussions and commits from Marek and Will.
Other:
> static u32 exynos_read_count_32(void)
This is u64, not u32. Get rid of 32-bit optimization/hack or explain it
similarly as Doug did (40 lines of reasoning).
> .mask = CLOCKSOURCE_MASK(32),
Mask is 64.
These are the 32-bit legacies I found now (maybe there are more...).
Don't copy them if this is a new driver getting rid of legacy.
>
>> All these point that block is not different. Again, let me repeat, we
>> support old Samsung PMICs with new Samsung PMICs in one driver. Even
>> though the "old one" won't be changed, as you mentioned here. The same
>> Samsung SoC clock drivers are used for old Exynos and for new ones...
>> Similarly to pinctrl drivers. The same ChipId.
>>
>> Everywhere we follow the same concept of unification instead of
>> duplication. Maybe Exynos MCT timer is an exception but you did not
>> provide any arguments supporting this. Why Exynos MCTv2 should be
>> treated differently than Exynos850 clocks, chipid, pinctrl and other blocks?
>>
>
> If MCTv2 has only changes in register layout, I can consider merging work.
> But this is not that case.
>
> You gave a example with PMIC, SoC clock, Pinctrl, ChipId.
> These H/W IP have only changes in register layout which came from difference of each SoC.
>
> Were these H/W IP version changed?
> Were these H/W IP control method changed ?
> No. It only has minor chagnes not major changes.
>
> * PMIC - controls the PMIC reigster with I2C interface regarding their SoC usecase.
> - there is no changes on H/W control method itself.
>
> * SoC Clock - changes only in register layout regarding SoC
> - Clock control method still the same.
>
> * Pinctrl - changes only in gpio pin register layout (pin number, pin type, pin map..) regarding SoC.
> - Is there any changes on control method ?
>
> * Chipid - This is very simple H/W IP. It only supports unique chip id value with read-only register.
> - It really only have changes in register layout.
>
> MCTv2 is different.
> Not only register layout but also it's control method has to be changed regarding H/W difference.
Yes, I see some differences in HW control which we also solve in several
other cases through structure ops. Indeed here it looks like the number
of differences in control is bigger than in other cases.
If Daniel is okay in having two drivers and you get rid of all 32-bit
legacy (including ordering against architected timers), I am fine with it.
>> Daniel,
>> Any preferences from you? Integrating MCT into existing driver (thus
>> growing it) or having a new one?
Best regards,
Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v1 1/2] clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2 driver for next Exynos SoC
2021-10-27 6:39 ` Krzysztof Kozlowski
@ 2021-11-01 6:04 ` Youngmin Nam
-1 siblings, 0 replies; 40+ messages in thread
From: Youngmin Nam @ 2021-11-01 6:04 UTC (permalink / raw)
To: Krzysztof Kozlowski, will, mark.rutland, daniel.lezcano
Cc: tglx, linux-kernel, linux-arm-kernel, linux-samsung-soc,
pullip.cho, hoony.yu, hajun.sung, myung-su.cha, kgene
[-- Attachment #1: Type: text/plain, Size: 8158 bytes --]
On Wed, Oct 27, 2021 at 08:39:47AM +0200, Krzysztof Kozlowski wrote:
> On 27/10/2021 03:38, Youngmin Nam wrote:
> > On Tue, Oct 26, 2021 at 01:00:51PM +0200, Krzysztof Kozlowski wrote:
> >> On 26/10/2021 12:45, Youngmin Nam wrote:
> >>> On Tue, Oct 26, 2021 at 09:10:28AM +0200, Krzysztof Kozlowski wrote:
> >>>> On 26/10/2021 03:47, Youngmin Nam wrote:
> >>>>>> If everyone added a new driver to avoid integrating with existing code,
> >>>>>> we would have huge kernel with thousands of duplicated solutions. The
> >>>>>> kernel also would be unmaintained.
> >>>>>>
> >>>>>> Such arguments were brought before several times - "I don't want to
> >>>>>> integrating with existing code", "My use case is different", "I would
> >>>>>> need to test the other cases", "It's complicated for me".
> >>>>>>
> >>>>>> Instead of pushing a new vendor driver you should integrate it with
> >>>>>> existing code.
> >>>>>>
> >>>>> Let me ask you one question.
> >>>>> If we maintain as one driver, how can people who don't have the new MCT test the new driver?
> >>>>
> >>>> I assume you talk about a case when someone else later changes something
> >>>> in the driver. Such person doesn't necessarily have to test it. The same
> >>>> as in all other cases (Exynos MCT is not special here): just ask for
> >>>> testing on platform one doesn't have.
> >>>>
> >>>> Even if you submit this as separate driver, there is the exact same
> >>>> problem. People will change the MCTv2 driver without access to hardware.
> >>>>
> >>> Yes, I can test the new MCT driver if someone ask for testing after modifying the new driver.
> >>> But in this case, we don't need to test the previous MCT driver. We have only to test the new MCT driver.
> >>
> >> Like with everything in Linux kernel. We merge instead of duplicate.
> >> It's not an argument.
> >>
> >>>> None of these differ for Exynos MCT from other drivers, e.g. mentioned
> >>>> Samsung PMIC drivers, recently modified (by Will and Sam) the SoC clock
> >>>> drivers or the ChipID drivers (changed by Chanho).
> >>> From HW point of view, the previous MCT is almost 10-year-old IP without any major change and
> >>> it will not be used on next new Exynos SoC.
> >>> MCTv2 is the totally newly designed IP and it will replace the Exynos system timer.
> >>> Device driver would be dependent with H/W. We are going to apply a lot of changes for this new MCT.
> >>> For maintenance, I think we should separate the new MCT driver for maintenance.
> >>>
> >>
> >> There are several similarities which actually suggest that you
> >> exaggerate the differences.
> >>
> >> The number of interrupts is the same (4+8 in older one, 12 in new one...).
> >
> > I didn't "exaggerate" at all.
> > The numer of interrups is the same. But their usage is completely different.
> > The type of each timer is different.
> > And previous MCT can only support upto 8 cores.
> >
> > * MCTv1 (Let me call previous MCT as MCTv1)
> > - 4 global timer + 8 local timer
> > - Global timer and local timer are totally different.
> > - 4 global timer have only one 64bit FRC that serves as the "up-counter" with 4 "comparators"
> > - 8 local timer have 8 of 32bit FRC that serves as the "down-counter" without any "comparators".(just expire timer)
> > - local timer can be used as per-cpu event timer, so it can only support upto 8 cores.
> >
> > * MCTv2
> > - There are no global timer and local timer anymore.
> > - 1 of 64bit FRC that serves as "up-counter" (just counter without "comparators")
> > - 12 comaprators (These are not "counter") can be used as per-cpu event timer so that it can support upto 12 cores.
> > - RTC source can be used as backup source.
> >
> >> You assign the MCT priority also as higher than Architected Timer
> >> (+Cc Will and Mark - is it ok for you?)
> >> evt->rating = 500; /* use value higher than ARM arch timer *
> >>
> > Yes, this is absolutely correct on event timer.
> > We cannot use arm arch timer which is operating based on PPI as per-cpu event timer because of poewr mode.
> > We have to use SPI for per-cpu timer interrupt. (This is the same in all Exynos platform)
>
> No, this is not correct, was explained several times and it is one of
> the reasons why I am holding back to reuse of existing driver. You
> copied few 32-bit ARM (ARMv7) solutions from old MCT driver into a new
> one which is only 64-bit. These 32-bit solutions are some optimizations
> or hacks matching 32-bit ARM Exynos processors. If you copy them to
> entirely new driver for entirely different IP block, it means this is
> not entirely different IP block.
>
> Therefore I see a point now in having a Exynos MCTv2 driver for new IP
> blocks after removing all that legacy 32-bit ARM stuff.
>
> Therefore:
> > evt->rating = 350;
> Not 500. Use the same value as old timer driver for ARMv8. See previous
> discussions and commits from Marek and Will.
>
I've read the whole history from Marek and Will.
As I explained to Will, we need more time to test regarding decreasing the rating of MCTv2.
Decreasing the rating is related only performance not functionality.
So from regression perspective, it's not easy to change the rating on this new driver at this time.
> Other:
> > static u32 exynos_read_count_32(void)
>
> This is u64, not u32. Get rid of 32-bit optimization/hack or explain it
> similarly as Doug did (40 lines of reasoning).
>
> > .mask = CLOCKSOURCE_MASK(32),
>
> Mask is 64.
>
> These are the 32-bit legacies I found now (maybe there are more...).
> Don't copy them if this is a new driver getting rid of legacy.
>
Thanks for pointing out and sharing "Doug Anderson"'s explanation.
As we use ARM arch timer as a clock source currently, we should get rid of it.
> >
> >> All these point that block is not different. Again, let me repeat, we
> >> support old Samsung PMICs with new Samsung PMICs in one driver. Even
> >> though the "old one" won't be changed, as you mentioned here. The same
> >> Samsung SoC clock drivers are used for old Exynos and for new ones...
> >> Similarly to pinctrl drivers. The same ChipId.
> >>
> >> Everywhere we follow the same concept of unification instead of
> >> duplication. Maybe Exynos MCT timer is an exception but you did not
> >> provide any arguments supporting this. Why Exynos MCTv2 should be
> >> treated differently than Exynos850 clocks, chipid, pinctrl and other blocks?
> >>
> >
> > If MCTv2 has only changes in register layout, I can consider merging work.
> > But this is not that case.
> >
> > You gave a example with PMIC, SoC clock, Pinctrl, ChipId.
> > These H/W IP have only changes in register layout which came from difference of each SoC.
> >
> > Were these H/W IP version changed?
> > Were these H/W IP control method changed ?
> > No. It only has minor chagnes not major changes.
> >
> > * PMIC - controls the PMIC reigster with I2C interface regarding their SoC usecase.
> > - there is no changes on H/W control method itself.
> >
> > * SoC Clock - changes only in register layout regarding SoC
> > - Clock control method still the same.
> >
> > * Pinctrl - changes only in gpio pin register layout (pin number, pin type, pin map..) regarding SoC.
> > - Is there any changes on control method ?
> >
> > * Chipid - This is very simple H/W IP. It only supports unique chip id value with read-only register.
> > - It really only have changes in register layout.
> >
> > MCTv2 is different.
> > Not only register layout but also it's control method has to be changed regarding H/W difference.
>
> Yes, I see some differences in HW control which we also solve in several
> other cases through structure ops. Indeed here it looks like the number
> of differences in control is bigger than in other cases.
>
> If Daniel is okay in having two drivers and you get rid of all 32-bit
> legacy (including ordering against architected timers), I am fine with it.
>
Thanks for understanding.
Let me send patch v2 soon.
> >> Daniel,
> >> Any preferences from you? Integrating MCT into existing driver (thus
> >> growing it) or having a new one?
>
>
>
> Best regards,
> Krzysztof
>
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v1 1/2] clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2 driver for next Exynos SoC
@ 2021-11-01 6:04 ` Youngmin Nam
0 siblings, 0 replies; 40+ messages in thread
From: Youngmin Nam @ 2021-11-01 6:04 UTC (permalink / raw)
To: Krzysztof Kozlowski, will, mark.rutland, daniel.lezcano
Cc: tglx, linux-kernel, linux-arm-kernel, linux-samsung-soc,
pullip.cho, hoony.yu, hajun.sung, myung-su.cha, kgene
[-- Attachment #1: Type: text/plain, Size: 8158 bytes --]
On Wed, Oct 27, 2021 at 08:39:47AM +0200, Krzysztof Kozlowski wrote:
> On 27/10/2021 03:38, Youngmin Nam wrote:
> > On Tue, Oct 26, 2021 at 01:00:51PM +0200, Krzysztof Kozlowski wrote:
> >> On 26/10/2021 12:45, Youngmin Nam wrote:
> >>> On Tue, Oct 26, 2021 at 09:10:28AM +0200, Krzysztof Kozlowski wrote:
> >>>> On 26/10/2021 03:47, Youngmin Nam wrote:
> >>>>>> If everyone added a new driver to avoid integrating with existing code,
> >>>>>> we would have huge kernel with thousands of duplicated solutions. The
> >>>>>> kernel also would be unmaintained.
> >>>>>>
> >>>>>> Such arguments were brought before several times - "I don't want to
> >>>>>> integrating with existing code", "My use case is different", "I would
> >>>>>> need to test the other cases", "It's complicated for me".
> >>>>>>
> >>>>>> Instead of pushing a new vendor driver you should integrate it with
> >>>>>> existing code.
> >>>>>>
> >>>>> Let me ask you one question.
> >>>>> If we maintain as one driver, how can people who don't have the new MCT test the new driver?
> >>>>
> >>>> I assume you talk about a case when someone else later changes something
> >>>> in the driver. Such person doesn't necessarily have to test it. The same
> >>>> as in all other cases (Exynos MCT is not special here): just ask for
> >>>> testing on platform one doesn't have.
> >>>>
> >>>> Even if you submit this as separate driver, there is the exact same
> >>>> problem. People will change the MCTv2 driver without access to hardware.
> >>>>
> >>> Yes, I can test the new MCT driver if someone ask for testing after modifying the new driver.
> >>> But in this case, we don't need to test the previous MCT driver. We have only to test the new MCT driver.
> >>
> >> Like with everything in Linux kernel. We merge instead of duplicate.
> >> It's not an argument.
> >>
> >>>> None of these differ for Exynos MCT from other drivers, e.g. mentioned
> >>>> Samsung PMIC drivers, recently modified (by Will and Sam) the SoC clock
> >>>> drivers or the ChipID drivers (changed by Chanho).
> >>> From HW point of view, the previous MCT is almost 10-year-old IP without any major change and
> >>> it will not be used on next new Exynos SoC.
> >>> MCTv2 is the totally newly designed IP and it will replace the Exynos system timer.
> >>> Device driver would be dependent with H/W. We are going to apply a lot of changes for this new MCT.
> >>> For maintenance, I think we should separate the new MCT driver for maintenance.
> >>>
> >>
> >> There are several similarities which actually suggest that you
> >> exaggerate the differences.
> >>
> >> The number of interrupts is the same (4+8 in older one, 12 in new one...).
> >
> > I didn't "exaggerate" at all.
> > The numer of interrups is the same. But their usage is completely different.
> > The type of each timer is different.
> > And previous MCT can only support upto 8 cores.
> >
> > * MCTv1 (Let me call previous MCT as MCTv1)
> > - 4 global timer + 8 local timer
> > - Global timer and local timer are totally different.
> > - 4 global timer have only one 64bit FRC that serves as the "up-counter" with 4 "comparators"
> > - 8 local timer have 8 of 32bit FRC that serves as the "down-counter" without any "comparators".(just expire timer)
> > - local timer can be used as per-cpu event timer, so it can only support upto 8 cores.
> >
> > * MCTv2
> > - There are no global timer and local timer anymore.
> > - 1 of 64bit FRC that serves as "up-counter" (just counter without "comparators")
> > - 12 comaprators (These are not "counter") can be used as per-cpu event timer so that it can support upto 12 cores.
> > - RTC source can be used as backup source.
> >
> >> You assign the MCT priority also as higher than Architected Timer
> >> (+Cc Will and Mark - is it ok for you?)
> >> evt->rating = 500; /* use value higher than ARM arch timer *
> >>
> > Yes, this is absolutely correct on event timer.
> > We cannot use arm arch timer which is operating based on PPI as per-cpu event timer because of poewr mode.
> > We have to use SPI for per-cpu timer interrupt. (This is the same in all Exynos platform)
>
> No, this is not correct, was explained several times and it is one of
> the reasons why I am holding back to reuse of existing driver. You
> copied few 32-bit ARM (ARMv7) solutions from old MCT driver into a new
> one which is only 64-bit. These 32-bit solutions are some optimizations
> or hacks matching 32-bit ARM Exynos processors. If you copy them to
> entirely new driver for entirely different IP block, it means this is
> not entirely different IP block.
>
> Therefore I see a point now in having a Exynos MCTv2 driver for new IP
> blocks after removing all that legacy 32-bit ARM stuff.
>
> Therefore:
> > evt->rating = 350;
> Not 500. Use the same value as old timer driver for ARMv8. See previous
> discussions and commits from Marek and Will.
>
I've read the whole history from Marek and Will.
As I explained to Will, we need more time to test regarding decreasing the rating of MCTv2.
Decreasing the rating is related only performance not functionality.
So from regression perspective, it's not easy to change the rating on this new driver at this time.
> Other:
> > static u32 exynos_read_count_32(void)
>
> This is u64, not u32. Get rid of 32-bit optimization/hack or explain it
> similarly as Doug did (40 lines of reasoning).
>
> > .mask = CLOCKSOURCE_MASK(32),
>
> Mask is 64.
>
> These are the 32-bit legacies I found now (maybe there are more...).
> Don't copy them if this is a new driver getting rid of legacy.
>
Thanks for pointing out and sharing "Doug Anderson"'s explanation.
As we use ARM arch timer as a clock source currently, we should get rid of it.
> >
> >> All these point that block is not different. Again, let me repeat, we
> >> support old Samsung PMICs with new Samsung PMICs in one driver. Even
> >> though the "old one" won't be changed, as you mentioned here. The same
> >> Samsung SoC clock drivers are used for old Exynos and for new ones...
> >> Similarly to pinctrl drivers. The same ChipId.
> >>
> >> Everywhere we follow the same concept of unification instead of
> >> duplication. Maybe Exynos MCT timer is an exception but you did not
> >> provide any arguments supporting this. Why Exynos MCTv2 should be
> >> treated differently than Exynos850 clocks, chipid, pinctrl and other blocks?
> >>
> >
> > If MCTv2 has only changes in register layout, I can consider merging work.
> > But this is not that case.
> >
> > You gave a example with PMIC, SoC clock, Pinctrl, ChipId.
> > These H/W IP have only changes in register layout which came from difference of each SoC.
> >
> > Were these H/W IP version changed?
> > Were these H/W IP control method changed ?
> > No. It only has minor chagnes not major changes.
> >
> > * PMIC - controls the PMIC reigster with I2C interface regarding their SoC usecase.
> > - there is no changes on H/W control method itself.
> >
> > * SoC Clock - changes only in register layout regarding SoC
> > - Clock control method still the same.
> >
> > * Pinctrl - changes only in gpio pin register layout (pin number, pin type, pin map..) regarding SoC.
> > - Is there any changes on control method ?
> >
> > * Chipid - This is very simple H/W IP. It only supports unique chip id value with read-only register.
> > - It really only have changes in register layout.
> >
> > MCTv2 is different.
> > Not only register layout but also it's control method has to be changed regarding H/W difference.
>
> Yes, I see some differences in HW control which we also solve in several
> other cases through structure ops. Indeed here it looks like the number
> of differences in control is bigger than in other cases.
>
> If Daniel is okay in having two drivers and you get rid of all 32-bit
> legacy (including ordering against architected timers), I am fine with it.
>
Thanks for understanding.
Let me send patch v2 soon.
> >> Daniel,
> >> Any preferences from you? Integrating MCT into existing driver (thus
> >> growing it) or having a new one?
>
>
>
> Best regards,
> Krzysztof
>
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
[-- Attachment #3: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v1 1/2] clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2 driver for next Exynos SoC
2021-10-27 1:38 ` Youngmin Nam
@ 2021-10-27 7:34 ` Will Deacon
-1 siblings, 0 replies; 40+ messages in thread
From: Will Deacon @ 2021-10-27 7:34 UTC (permalink / raw)
To: Youngmin Nam
Cc: krzysztof.kozlowski, mark.rutland, daniel.lezcano, tglx,
linux-kernel, linux-arm-kernel, linux-samsung-soc, pullip.cho,
hoony.yu, hajun.sung, myung-su.cha, kgene
On Wed, Oct 27, 2021 at 10:38:37AM +0900, Youngmin Nam wrote:
> On Tue, Oct 26, 2021 at 01:00:51PM +0200, Krzysztof Kozlowski wrote:
> > On 26/10/2021 12:45, Youngmin Nam wrote:
> > > On Tue, Oct 26, 2021 at 09:10:28AM +0200, Krzysztof Kozlowski wrote:
> > >> On 26/10/2021 03:47, Youngmin Nam wrote:
> > >>>> If everyone added a new driver to avoid integrating with existing code,
> > >>>> we would have huge kernel with thousands of duplicated solutions. The
> > >>>> kernel also would be unmaintained.
> > >>>>
> > >>>> Such arguments were brought before several times - "I don't want to
> > >>>> integrating with existing code", "My use case is different", "I would
> > >>>> need to test the other cases", "It's complicated for me".
> > >>>>
> > >>>> Instead of pushing a new vendor driver you should integrate it with
> > >>>> existing code.
> > >>>>
> > >>> Let me ask you one question.
> > >>> If we maintain as one driver, how can people who don't have the new MCT test the new driver?
> > >>
> > >> I assume you talk about a case when someone else later changes something
> > >> in the driver. Such person doesn't necessarily have to test it. The same
> > >> as in all other cases (Exynos MCT is not special here): just ask for
> > >> testing on platform one doesn't have.
> > >>
> > >> Even if you submit this as separate driver, there is the exact same
> > >> problem. People will change the MCTv2 driver without access to hardware.
> > >>
> > > Yes, I can test the new MCT driver if someone ask for testing after modifying the new driver.
> > > But in this case, we don't need to test the previous MCT driver. We have only to test the new MCT driver.
> >
> > Like with everything in Linux kernel. We merge instead of duplicate.
> > It's not an argument.
> >
> > >> None of these differ for Exynos MCT from other drivers, e.g. mentioned
> > >> Samsung PMIC drivers, recently modified (by Will and Sam) the SoC clock
> > >> drivers or the ChipID drivers (changed by Chanho).
> > > From HW point of view, the previous MCT is almost 10-year-old IP without any major change and
> > > it will not be used on next new Exynos SoC.
> > > MCTv2 is the totally newly designed IP and it will replace the Exynos system timer.
> > > Device driver would be dependent with H/W. We are going to apply a lot of changes for this new MCT.
> > > For maintenance, I think we should separate the new MCT driver for maintenance.
> > >
> >
> > There are several similarities which actually suggest that you
> > exaggerate the differences.
> >
> > The number of interrupts is the same (4+8 in older one, 12 in new one...).
>
> I didn't "exaggerate" at all.
> The numer of interrups is the same. But their usage is completely different.
> The type of each timer is different.
> And previous MCT can only support upto 8 cores.
>
> * MCTv1 (Let me call previous MCT as MCTv1)
> - 4 global timer + 8 local timer
> - Global timer and local timer are totally different.
> - 4 global timer have only one 64bit FRC that serves as the "up-counter" with 4 "comparators"
> - 8 local timer have 8 of 32bit FRC that serves as the "down-counter" without any "comparators".(just expire timer)
> - local timer can be used as per-cpu event timer, so it can only support upto 8 cores.
>
> * MCTv2
> - There are no global timer and local timer anymore.
> - 1 of 64bit FRC that serves as "up-counter" (just counter without "comparators")
> - 12 comaprators (These are not "counter") can be used as per-cpu event timer so that it can support upto 12 cores.
> - RTC source can be used as backup source.
>
> > You assign the MCT priority also as higher than Architected Timer
> > (+Cc Will and Mark - is it ok for you?)
> > evt->rating = 500; /* use value higher than ARM arch timer *
> >
> Yes, this is absolutely correct on event timer.
> We cannot use arm arch timer which is operating based on PPI as per-cpu event timer because of poewr mode.
You should be able to now that I've added support for per-cpu wakeup timers.
As long as the Arm arch timer is marked as C3STOP (e.g. by sticking the
"local-timer-stop" property in the DT notes), then the MCT will be used
as the wakeup source if you set the CLOCK_EVT_FEAT_PERCPU feature flag.
Give it a try.
Will
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v1 1/2] clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2 driver for next Exynos SoC
@ 2021-10-27 7:34 ` Will Deacon
0 siblings, 0 replies; 40+ messages in thread
From: Will Deacon @ 2021-10-27 7:34 UTC (permalink / raw)
To: Youngmin Nam
Cc: krzysztof.kozlowski, mark.rutland, daniel.lezcano, tglx,
linux-kernel, linux-arm-kernel, linux-samsung-soc, pullip.cho,
hoony.yu, hajun.sung, myung-su.cha, kgene
On Wed, Oct 27, 2021 at 10:38:37AM +0900, Youngmin Nam wrote:
> On Tue, Oct 26, 2021 at 01:00:51PM +0200, Krzysztof Kozlowski wrote:
> > On 26/10/2021 12:45, Youngmin Nam wrote:
> > > On Tue, Oct 26, 2021 at 09:10:28AM +0200, Krzysztof Kozlowski wrote:
> > >> On 26/10/2021 03:47, Youngmin Nam wrote:
> > >>>> If everyone added a new driver to avoid integrating with existing code,
> > >>>> we would have huge kernel with thousands of duplicated solutions. The
> > >>>> kernel also would be unmaintained.
> > >>>>
> > >>>> Such arguments were brought before several times - "I don't want to
> > >>>> integrating with existing code", "My use case is different", "I would
> > >>>> need to test the other cases", "It's complicated for me".
> > >>>>
> > >>>> Instead of pushing a new vendor driver you should integrate it with
> > >>>> existing code.
> > >>>>
> > >>> Let me ask you one question.
> > >>> If we maintain as one driver, how can people who don't have the new MCT test the new driver?
> > >>
> > >> I assume you talk about a case when someone else later changes something
> > >> in the driver. Such person doesn't necessarily have to test it. The same
> > >> as in all other cases (Exynos MCT is not special here): just ask for
> > >> testing on platform one doesn't have.
> > >>
> > >> Even if you submit this as separate driver, there is the exact same
> > >> problem. People will change the MCTv2 driver without access to hardware.
> > >>
> > > Yes, I can test the new MCT driver if someone ask for testing after modifying the new driver.
> > > But in this case, we don't need to test the previous MCT driver. We have only to test the new MCT driver.
> >
> > Like with everything in Linux kernel. We merge instead of duplicate.
> > It's not an argument.
> >
> > >> None of these differ for Exynos MCT from other drivers, e.g. mentioned
> > >> Samsung PMIC drivers, recently modified (by Will and Sam) the SoC clock
> > >> drivers or the ChipID drivers (changed by Chanho).
> > > From HW point of view, the previous MCT is almost 10-year-old IP without any major change and
> > > it will not be used on next new Exynos SoC.
> > > MCTv2 is the totally newly designed IP and it will replace the Exynos system timer.
> > > Device driver would be dependent with H/W. We are going to apply a lot of changes for this new MCT.
> > > For maintenance, I think we should separate the new MCT driver for maintenance.
> > >
> >
> > There are several similarities which actually suggest that you
> > exaggerate the differences.
> >
> > The number of interrupts is the same (4+8 in older one, 12 in new one...).
>
> I didn't "exaggerate" at all.
> The numer of interrups is the same. But their usage is completely different.
> The type of each timer is different.
> And previous MCT can only support upto 8 cores.
>
> * MCTv1 (Let me call previous MCT as MCTv1)
> - 4 global timer + 8 local timer
> - Global timer and local timer are totally different.
> - 4 global timer have only one 64bit FRC that serves as the "up-counter" with 4 "comparators"
> - 8 local timer have 8 of 32bit FRC that serves as the "down-counter" without any "comparators".(just expire timer)
> - local timer can be used as per-cpu event timer, so it can only support upto 8 cores.
>
> * MCTv2
> - There are no global timer and local timer anymore.
> - 1 of 64bit FRC that serves as "up-counter" (just counter without "comparators")
> - 12 comaprators (These are not "counter") can be used as per-cpu event timer so that it can support upto 12 cores.
> - RTC source can be used as backup source.
>
> > You assign the MCT priority also as higher than Architected Timer
> > (+Cc Will and Mark - is it ok for you?)
> > evt->rating = 500; /* use value higher than ARM arch timer *
> >
> Yes, this is absolutely correct on event timer.
> We cannot use arm arch timer which is operating based on PPI as per-cpu event timer because of poewr mode.
You should be able to now that I've added support for per-cpu wakeup timers.
As long as the Arm arch timer is marked as C3STOP (e.g. by sticking the
"local-timer-stop" property in the DT notes), then the MCT will be used
as the wakeup source if you set the CLOCK_EVT_FEAT_PERCPU feature flag.
Give it a try.
Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v1 1/2] clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2 driver for next Exynos SoC
2021-10-27 7:34 ` Will Deacon
@ 2021-10-29 3:54 ` Youngmin Nam
-1 siblings, 0 replies; 40+ messages in thread
From: Youngmin Nam @ 2021-10-29 3:54 UTC (permalink / raw)
To: Will Deacon
Cc: krzysztof.kozlowski, mark.rutland, daniel.lezcano, tglx,
linux-kernel, linux-arm-kernel, linux-samsung-soc, pullip.cho,
hoony.yu, hajun.sung, myung-su.cha, kgene
[-- Attachment #1: Type: text/plain, Size: 4841 bytes --]
On Wed, Oct 27, 2021 at 08:34:58AM +0100, Will Deacon wrote:
> On Wed, Oct 27, 2021 at 10:38:37AM +0900, Youngmin Nam wrote:
> > On Tue, Oct 26, 2021 at 01:00:51PM +0200, Krzysztof Kozlowski wrote:
> > > On 26/10/2021 12:45, Youngmin Nam wrote:
> > > > On Tue, Oct 26, 2021 at 09:10:28AM +0200, Krzysztof Kozlowski wrote:
> > > >> On 26/10/2021 03:47, Youngmin Nam wrote:
> > > >>>> If everyone added a new driver to avoid integrating with existing code,
> > > >>>> we would have huge kernel with thousands of duplicated solutions. The
> > > >>>> kernel also would be unmaintained.
> > > >>>>
> > > >>>> Such arguments were brought before several times - "I don't want to
> > > >>>> integrating with existing code", "My use case is different", "I would
> > > >>>> need to test the other cases", "It's complicated for me".
> > > >>>>
> > > >>>> Instead of pushing a new vendor driver you should integrate it with
> > > >>>> existing code.
> > > >>>>
> > > >>> Let me ask you one question.
> > > >>> If we maintain as one driver, how can people who don't have the new MCT test the new driver?
> > > >>
> > > >> I assume you talk about a case when someone else later changes something
> > > >> in the driver. Such person doesn't necessarily have to test it. The same
> > > >> as in all other cases (Exynos MCT is not special here): just ask for
> > > >> testing on platform one doesn't have.
> > > >>
> > > >> Even if you submit this as separate driver, there is the exact same
> > > >> problem. People will change the MCTv2 driver without access to hardware.
> > > >>
> > > > Yes, I can test the new MCT driver if someone ask for testing after modifying the new driver.
> > > > But in this case, we don't need to test the previous MCT driver. We have only to test the new MCT driver.
> > >
> > > Like with everything in Linux kernel. We merge instead of duplicate.
> > > It's not an argument.
> > >
> > > >> None of these differ for Exynos MCT from other drivers, e.g. mentioned
> > > >> Samsung PMIC drivers, recently modified (by Will and Sam) the SoC clock
> > > >> drivers or the ChipID drivers (changed by Chanho).
> > > > From HW point of view, the previous MCT is almost 10-year-old IP without any major change and
> > > > it will not be used on next new Exynos SoC.
> > > > MCTv2 is the totally newly designed IP and it will replace the Exynos system timer.
> > > > Device driver would be dependent with H/W. We are going to apply a lot of changes for this new MCT.
> > > > For maintenance, I think we should separate the new MCT driver for maintenance.
> > > >
> > >
> > > There are several similarities which actually suggest that you
> > > exaggerate the differences.
> > >
> > > The number of interrupts is the same (4+8 in older one, 12 in new one...).
> >
> > I didn't "exaggerate" at all.
> > The numer of interrups is the same. But their usage is completely different.
> > The type of each timer is different.
> > And previous MCT can only support upto 8 cores.
> >
> > * MCTv1 (Let me call previous MCT as MCTv1)
> > - 4 global timer + 8 local timer
> > - Global timer and local timer are totally different.
> > - 4 global timer have only one 64bit FRC that serves as the "up-counter" with 4 "comparators"
> > - 8 local timer have 8 of 32bit FRC that serves as the "down-counter" without any "comparators".(just expire timer)
> > - local timer can be used as per-cpu event timer, so it can only support upto 8 cores.
> >
> > * MCTv2
> > - There are no global timer and local timer anymore.
> > - 1 of 64bit FRC that serves as "up-counter" (just counter without "comparators")
> > - 12 comaprators (These are not "counter") can be used as per-cpu event timer so that it can support upto 12 cores.
> > - RTC source can be used as backup source.
> >
> > > You assign the MCT priority also as higher than Architected Timer
> > > (+Cc Will and Mark - is it ok for you?)
> > > evt->rating = 500; /* use value higher than ARM arch timer *
> > >
> > Yes, this is absolutely correct on event timer.
> > We cannot use arm arch timer which is operating based on PPI as per-cpu event timer because of poewr mode.
>
> You should be able to now that I've added support for per-cpu wakeup timers.
>
> As long as the Arm arch timer is marked as C3STOP (e.g. by sticking the
> "local-timer-stop" property in the DT notes), then the MCT will be used
> as the wakeup source if you set the CLOCK_EVT_FEAT_PERCPU feature flag.
>
> Give it a try.
>
> Will
>
Hi Will. Thanks for sharing information.
In MCTv2, we need more time to test because this patchset is the start for MCTv2 new driver.
This feature is for better performance not functionality.
How about considering this later after the current patchset is merged first ?
After doing our regression test, we will be able to consider applying this.
Thanks.
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v1 1/2] clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2 driver for next Exynos SoC
@ 2021-10-29 3:54 ` Youngmin Nam
0 siblings, 0 replies; 40+ messages in thread
From: Youngmin Nam @ 2021-10-29 3:54 UTC (permalink / raw)
To: Will Deacon
Cc: krzysztof.kozlowski, mark.rutland, daniel.lezcano, tglx,
linux-kernel, linux-arm-kernel, linux-samsung-soc, pullip.cho,
hoony.yu, hajun.sung, myung-su.cha, kgene
[-- Attachment #1: Type: text/plain, Size: 4841 bytes --]
On Wed, Oct 27, 2021 at 08:34:58AM +0100, Will Deacon wrote:
> On Wed, Oct 27, 2021 at 10:38:37AM +0900, Youngmin Nam wrote:
> > On Tue, Oct 26, 2021 at 01:00:51PM +0200, Krzysztof Kozlowski wrote:
> > > On 26/10/2021 12:45, Youngmin Nam wrote:
> > > > On Tue, Oct 26, 2021 at 09:10:28AM +0200, Krzysztof Kozlowski wrote:
> > > >> On 26/10/2021 03:47, Youngmin Nam wrote:
> > > >>>> If everyone added a new driver to avoid integrating with existing code,
> > > >>>> we would have huge kernel with thousands of duplicated solutions. The
> > > >>>> kernel also would be unmaintained.
> > > >>>>
> > > >>>> Such arguments were brought before several times - "I don't want to
> > > >>>> integrating with existing code", "My use case is different", "I would
> > > >>>> need to test the other cases", "It's complicated for me".
> > > >>>>
> > > >>>> Instead of pushing a new vendor driver you should integrate it with
> > > >>>> existing code.
> > > >>>>
> > > >>> Let me ask you one question.
> > > >>> If we maintain as one driver, how can people who don't have the new MCT test the new driver?
> > > >>
> > > >> I assume you talk about a case when someone else later changes something
> > > >> in the driver. Such person doesn't necessarily have to test it. The same
> > > >> as in all other cases (Exynos MCT is not special here): just ask for
> > > >> testing on platform one doesn't have.
> > > >>
> > > >> Even if you submit this as separate driver, there is the exact same
> > > >> problem. People will change the MCTv2 driver without access to hardware.
> > > >>
> > > > Yes, I can test the new MCT driver if someone ask for testing after modifying the new driver.
> > > > But in this case, we don't need to test the previous MCT driver. We have only to test the new MCT driver.
> > >
> > > Like with everything in Linux kernel. We merge instead of duplicate.
> > > It's not an argument.
> > >
> > > >> None of these differ for Exynos MCT from other drivers, e.g. mentioned
> > > >> Samsung PMIC drivers, recently modified (by Will and Sam) the SoC clock
> > > >> drivers or the ChipID drivers (changed by Chanho).
> > > > From HW point of view, the previous MCT is almost 10-year-old IP without any major change and
> > > > it will not be used on next new Exynos SoC.
> > > > MCTv2 is the totally newly designed IP and it will replace the Exynos system timer.
> > > > Device driver would be dependent with H/W. We are going to apply a lot of changes for this new MCT.
> > > > For maintenance, I think we should separate the new MCT driver for maintenance.
> > > >
> > >
> > > There are several similarities which actually suggest that you
> > > exaggerate the differences.
> > >
> > > The number of interrupts is the same (4+8 in older one, 12 in new one...).
> >
> > I didn't "exaggerate" at all.
> > The numer of interrups is the same. But their usage is completely different.
> > The type of each timer is different.
> > And previous MCT can only support upto 8 cores.
> >
> > * MCTv1 (Let me call previous MCT as MCTv1)
> > - 4 global timer + 8 local timer
> > - Global timer and local timer are totally different.
> > - 4 global timer have only one 64bit FRC that serves as the "up-counter" with 4 "comparators"
> > - 8 local timer have 8 of 32bit FRC that serves as the "down-counter" without any "comparators".(just expire timer)
> > - local timer can be used as per-cpu event timer, so it can only support upto 8 cores.
> >
> > * MCTv2
> > - There are no global timer and local timer anymore.
> > - 1 of 64bit FRC that serves as "up-counter" (just counter without "comparators")
> > - 12 comaprators (These are not "counter") can be used as per-cpu event timer so that it can support upto 12 cores.
> > - RTC source can be used as backup source.
> >
> > > You assign the MCT priority also as higher than Architected Timer
> > > (+Cc Will and Mark - is it ok for you?)
> > > evt->rating = 500; /* use value higher than ARM arch timer *
> > >
> > Yes, this is absolutely correct on event timer.
> > We cannot use arm arch timer which is operating based on PPI as per-cpu event timer because of poewr mode.
>
> You should be able to now that I've added support for per-cpu wakeup timers.
>
> As long as the Arm arch timer is marked as C3STOP (e.g. by sticking the
> "local-timer-stop" property in the DT notes), then the MCT will be used
> as the wakeup source if you set the CLOCK_EVT_FEAT_PERCPU feature flag.
>
> Give it a try.
>
> Will
>
Hi Will. Thanks for sharing information.
In MCTv2, we need more time to test because this patchset is the start for MCTv2 new driver.
This feature is for better performance not functionality.
How about considering this later after the current patchset is merged first ?
After doing our regression test, we will be able to consider applying this.
Thanks.
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
[-- Attachment #3: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v1 1/2] clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2 driver for next Exynos SoC
2021-10-26 10:45 ` Youngmin Nam
@ 2021-10-26 11:00 ` Krzysztof Kozlowski
-1 siblings, 0 replies; 40+ messages in thread
From: Krzysztof Kozlowski @ 2021-10-26 11:00 UTC (permalink / raw)
To: Youngmin Nam, Will Deacon, Mark Rutland, daniel.lezcano
Cc: tglx, linux-kernel, linux-arm-kernel, linux-samsung-soc,
pullip.cho, hoony.yu, hajun.sung, myung-su.cha, kgene
On 26/10/2021 12:45, Youngmin Nam wrote:
> On Tue, Oct 26, 2021 at 09:10:28AM +0200, Krzysztof Kozlowski wrote:
>> On 26/10/2021 03:47, Youngmin Nam wrote:
>>>> If everyone added a new driver to avoid integrating with existing code,
>>>> we would have huge kernel with thousands of duplicated solutions. The
>>>> kernel also would be unmaintained.
>>>>
>>>> Such arguments were brought before several times - "I don't want to
>>>> integrating with existing code", "My use case is different", "I would
>>>> need to test the other cases", "It's complicated for me".
>>>>
>>>> Instead of pushing a new vendor driver you should integrate it with
>>>> existing code.
>>>>
>>> Let me ask you one question.
>>> If we maintain as one driver, how can people who don't have the new MCT test the new driver?
>>
>> I assume you talk about a case when someone else later changes something
>> in the driver. Such person doesn't necessarily have to test it. The same
>> as in all other cases (Exynos MCT is not special here): just ask for
>> testing on platform one doesn't have.
>>
>> Even if you submit this as separate driver, there is the exact same
>> problem. People will change the MCTv2 driver without access to hardware.
>>
> Yes, I can test the new MCT driver if someone ask for testing after modifying the new driver.
> But in this case, we don't need to test the previous MCT driver. We have only to test the new MCT driver.
Like with everything in Linux kernel. We merge instead of duplicate.
It's not an argument.
>> None of these differ for Exynos MCT from other drivers, e.g. mentioned
>> Samsung PMIC drivers, recently modified (by Will and Sam) the SoC clock
>> drivers or the ChipID drivers (changed by Chanho).
> From HW point of view, the previous MCT is almost 10-year-old IP without any major change and
> it will not be used on next new Exynos SoC.
> MCTv2 is the totally newly designed IP and it will replace the Exynos system timer.
> Device driver would be dependent with H/W. We are going to apply a lot of changes for this new MCT.
> For maintenance, I think we should separate the new MCT driver for maintenance.
>
There are several similarities which actually suggest that you
exaggerate the differences.
The number of interrupts is the same (4+8 in older one, 12 in new one...).
You assign the MCT priority also as higher than Architected Timer
(+Cc Will and Mark - is it ok for you?)
evt->rating = 500; /* use value higher than ARM arch timer *
All these point that block is not different. Again, let me repeat, we
support old Samsung PMICs with new Samsung PMICs in one driver. Even
though the "old one" won't be changed, as you mentioned here. The same
Samsung SoC clock drivers are used for old Exynos and for new ones...
Similarly to pinctrl drivers. The same ChipId.
Everywhere we follow the same concept of unification instead of
duplication. Maybe Exynos MCT timer is an exception but you did not
provide any arguments supporting this. Why Exynos MCTv2 should be
treated differently than Exynos850 clocks, chipid, pinctrl and other blocks?
Daniel,
Any preferences from you? Integrating MCT into existing driver (thus
growing it) or having a new one?
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v1 1/2] clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2 driver for next Exynos SoC
@ 2021-10-26 11:00 ` Krzysztof Kozlowski
0 siblings, 0 replies; 40+ messages in thread
From: Krzysztof Kozlowski @ 2021-10-26 11:00 UTC (permalink / raw)
To: Youngmin Nam, Will Deacon, Mark Rutland, daniel.lezcano
Cc: tglx, linux-kernel, linux-arm-kernel, linux-samsung-soc,
pullip.cho, hoony.yu, hajun.sung, myung-su.cha, kgene
On 26/10/2021 12:45, Youngmin Nam wrote:
> On Tue, Oct 26, 2021 at 09:10:28AM +0200, Krzysztof Kozlowski wrote:
>> On 26/10/2021 03:47, Youngmin Nam wrote:
>>>> If everyone added a new driver to avoid integrating with existing code,
>>>> we would have huge kernel with thousands of duplicated solutions. The
>>>> kernel also would be unmaintained.
>>>>
>>>> Such arguments were brought before several times - "I don't want to
>>>> integrating with existing code", "My use case is different", "I would
>>>> need to test the other cases", "It's complicated for me".
>>>>
>>>> Instead of pushing a new vendor driver you should integrate it with
>>>> existing code.
>>>>
>>> Let me ask you one question.
>>> If we maintain as one driver, how can people who don't have the new MCT test the new driver?
>>
>> I assume you talk about a case when someone else later changes something
>> in the driver. Such person doesn't necessarily have to test it. The same
>> as in all other cases (Exynos MCT is not special here): just ask for
>> testing on platform one doesn't have.
>>
>> Even if you submit this as separate driver, there is the exact same
>> problem. People will change the MCTv2 driver without access to hardware.
>>
> Yes, I can test the new MCT driver if someone ask for testing after modifying the new driver.
> But in this case, we don't need to test the previous MCT driver. We have only to test the new MCT driver.
Like with everything in Linux kernel. We merge instead of duplicate.
It's not an argument.
>> None of these differ for Exynos MCT from other drivers, e.g. mentioned
>> Samsung PMIC drivers, recently modified (by Will and Sam) the SoC clock
>> drivers or the ChipID drivers (changed by Chanho).
> From HW point of view, the previous MCT is almost 10-year-old IP without any major change and
> it will not be used on next new Exynos SoC.
> MCTv2 is the totally newly designed IP and it will replace the Exynos system timer.
> Device driver would be dependent with H/W. We are going to apply a lot of changes for this new MCT.
> For maintenance, I think we should separate the new MCT driver for maintenance.
>
There are several similarities which actually suggest that you
exaggerate the differences.
The number of interrupts is the same (4+8 in older one, 12 in new one...).
You assign the MCT priority also as higher than Architected Timer
(+Cc Will and Mark - is it ok for you?)
evt->rating = 500; /* use value higher than ARM arch timer *
All these point that block is not different. Again, let me repeat, we
support old Samsung PMICs with new Samsung PMICs in one driver. Even
though the "old one" won't be changed, as you mentioned here. The same
Samsung SoC clock drivers are used for old Exynos and for new ones...
Similarly to pinctrl drivers. The same ChipId.
Everywhere we follow the same concept of unification instead of
duplication. Maybe Exynos MCT timer is an exception but you did not
provide any arguments supporting this. Why Exynos MCTv2 should be
treated differently than Exynos850 clocks, chipid, pinctrl and other blocks?
Daniel,
Any preferences from you? Integrating MCT into existing driver (thus
growing it) or having a new one?
Best regards,
Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v1 1/2] clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2 driver for next Exynos SoC
2021-10-21 6:18 ` Youngmin Nam
@ 2021-10-27 8:38 ` Krzysztof Kozlowski
-1 siblings, 0 replies; 40+ messages in thread
From: Krzysztof Kozlowski @ 2021-10-27 8:38 UTC (permalink / raw)
To: Youngmin Nam
Cc: daniel.lezcano, tglx, linux-kernel, linux-arm-kernel,
linux-samsung-soc, pullip.cho, hoony.yu, hajun.sung,
myung-su.cha
On 21/10/2021 08:18, Youngmin Nam wrote:
> Exynos MCT version 2 is composed of 1 FRC and 12 comparators.
> The 12 comparators can produces interrupts independently,
> so they can be used as local timer of each CPU.
>
...
> +
> +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));
This is unsigned long, so 64-bit on your platform. Use writeq_relaxed or
handle it somehow.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v1 1/2] clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2 driver for next Exynos SoC
@ 2021-10-27 8:38 ` Krzysztof Kozlowski
0 siblings, 0 replies; 40+ messages in thread
From: Krzysztof Kozlowski @ 2021-10-27 8:38 UTC (permalink / raw)
To: Youngmin Nam
Cc: daniel.lezcano, tglx, linux-kernel, linux-arm-kernel,
linux-samsung-soc, pullip.cho, hoony.yu, hajun.sung,
myung-su.cha
On 21/10/2021 08:18, Youngmin Nam wrote:
> Exynos MCT version 2 is composed of 1 FRC and 12 comparators.
> The 12 comparators can produces interrupts independently,
> so they can be used as local timer of each CPU.
>
...
> +
> +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));
This is unsigned long, so 64-bit on your platform. Use writeq_relaxed or
handle it somehow.
Best regards,
Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v1 1/2] clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2 driver for next Exynos SoC
2021-10-27 8:38 ` Krzysztof Kozlowski
@ 2021-11-01 5:25 ` Youngmin Nam
-1 siblings, 0 replies; 40+ messages in thread
From: Youngmin Nam @ 2021-11-01 5:25 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: daniel.lezcano, tglx, linux-kernel, linux-arm-kernel,
linux-samsung-soc, pullip.cho, hoony.yu, hajun.sung,
myung-su.cha
[-- Attachment #1: Type: text/plain, Size: 1312 bytes --]
On Wed, Oct 27, 2021 at 10:38:09AM +0200, Krzysztof Kozlowski wrote:
> On 21/10/2021 08:18, Youngmin Nam wrote:
> > Exynos MCT version 2 is composed of 1 FRC and 12 comparators.
> > The 12 comparators can produces interrupts independently,
> > so they can be used as local timer of each CPU.
> >
>
> ...
>
> > +
> > +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));
>
> This is unsigned long, so 64-bit on your platform. Use writeq_relaxed or
> handle it somehow.
Thanks for your review.
I checked again and data sheet of MCTv2 shows compartor period has 32-bit data width.
Once we write 32bit value to comp_period register, H/W will produce interrupt when increasing FRC
is the same with the value of "current FRC + comp_period".
>
>
> Best regards,
> Krzysztof
>
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v1 1/2] clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2 driver for next Exynos SoC
@ 2021-11-01 5:25 ` Youngmin Nam
0 siblings, 0 replies; 40+ messages in thread
From: Youngmin Nam @ 2021-11-01 5:25 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: daniel.lezcano, tglx, linux-kernel, linux-arm-kernel,
linux-samsung-soc, pullip.cho, hoony.yu, hajun.sung,
myung-su.cha
[-- Attachment #1: Type: text/plain, Size: 1312 bytes --]
On Wed, Oct 27, 2021 at 10:38:09AM +0200, Krzysztof Kozlowski wrote:
> On 21/10/2021 08:18, Youngmin Nam wrote:
> > Exynos MCT version 2 is composed of 1 FRC and 12 comparators.
> > The 12 comparators can produces interrupts independently,
> > so they can be used as local timer of each CPU.
> >
>
> ...
>
> > +
> > +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));
>
> This is unsigned long, so 64-bit on your platform. Use writeq_relaxed or
> handle it somehow.
Thanks for your review.
I checked again and data sheet of MCTv2 shows compartor period has 32-bit data width.
Once we write 32bit value to comp_period register, H/W will produce interrupt when increasing FRC
is the same with the value of "current FRC + comp_period".
>
>
> Best regards,
> Krzysztof
>
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
[-- Attachment #3: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread