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

[Krzysztof Kozlowski]

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

Best regards,
Krzysztof