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 > >>>>> --- > >>>>> 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 > >>>>> +#include > >>>>> +#include > >>>>> +#include > >>>>> +#include > >>>>> +#include > >>>>> +#include > >>>>> +#include > >>>>> +#include > >>>>> +#include > >>>>> +#include > >>>>> +#include > >>>>> +#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 >