lkml.org 
[lkml]   [2021]   [Oct]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v1 1/2] clocksource/drivers/exynos_mct_v2: introduce Exynos MCT version 2 driver for next Exynos SoC
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
>
\
 
 \ /
  Last update: 2021-10-26 03:21    [W:0.188 / U:0.132 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site