Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 22 Oct 2021 13:21:22 +0900
From:   Youngmin Nam <youngmin.nam@samsung.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
Cc:     daniel.lezcano@linaro.org, tglx@linutronix.de,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-samsung-soc@vger.kernel.org, pullip.cho@samsung.com,
        hoony.yu@samsung.com, hajun.sung@samsung.com,
        myung-su.cha@samsung.com
Subject: Re: [PATCH v1 1/2] clocksource/drivers/exynos_mct_v2: introduce
 Exynos MCT version 2 driver for next Exynos SoC
Message-ID: <20211022042116.GA30645@perf>
MIME-Version: 1.0
In-Reply-To: <1b93aaf3-ed64-b105-dec4-07b6f27b385b@canonical.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Content-Type: multipart/mixed;
        boundary="----TI73IYgzdP03rYJ9SiN0qM3o3mlayfAqXHuFFMbUzbY.6Ea-=_3a807_"
CMS-TYPE: 102P
DLP-Filter: Pass
References: <20211021061804.39118-1-youngmin.nam@samsung.com>
        <CGME20211021055112epcas2p278145beb21cd6cc4217813a41c1e1407@epcas2p2.samsung.com>
        <20211021061804.39118-2-youngmin.nam@samsung.com>
        <0c5dcdab-7aa3-a98f-e615-acbe98489935@canonical.com>
        <20211021082650.GA30741@perf>
        <1b93aaf3-ed64-b105-dec4-07b6f27b385b@canonical.com>
Precedence: bulk

------TI73IYgzdP03rYJ9SiN0qM3o3mlayfAqXHuFFMbUzbY.6Ea-=_3a807_
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline

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
> 

------TI73IYgzdP03rYJ9SiN0qM3o3mlayfAqXHuFFMbUzbY.6Ea-=_3a807_
Content-Type: text/plain; charset="utf-8"


------TI73IYgzdP03rYJ9SiN0qM3o3mlayfAqXHuFFMbUzbY.6Ea-=_3a807_--