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

[Youngmin Nam <youngmin.nam@samsung.com>]

[-- 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 --]