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

[Krzysztof Kozlowski]

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@xxxxxxxxxxx>
>>>>> ---
>>>>>  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