Re: [PATCH 1/5] clocksource: Add MStar MSC313e timer support

From: Daniel Lezcano
Date: Mon Nov 29 2021 - 15:28:35 EST


On 26/11/2021 21:21, Romain Perier wrote:
> The MSC313e-compatible SoCs have 3 timer hardware blocks. All of these
> are free running 32-bit increasing counters and can generate interrupts.
> This commit adds basic support for these timers, the first timer block
> being used as clocksource/sched_clock and delay, while the others will
> be used as clockevents.

Please you elaborate a bit more the internals of this timer as it is a
initial submission


> Signed-off-by: Romain Perier <romain.perier@xxxxxxxxx>
> Co-developed-by: Daniel Palmer <daniel@xxxxxxxx>
> Signed-off-by: Daniel Palmer <daniel@xxxxxxxx>
> ---
> MAINTAINERS | 1 +
> drivers/clocksource/Kconfig | 10 ++
> drivers/clocksource/Makefile | 1 +
> drivers/clocksource/timer-msc313e.c | 228 ++++++++++++++++++++++++++++
> 4 files changed, 240 insertions(+)
> create mode 100644 drivers/clocksource/timer-msc313e.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7a2345ce8521..f39a1617bf50 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2282,6 +2282,7 @@ F: Documentation/devicetree/bindings/gpio/mstar,msc313-gpio.yaml
> F: arch/arm/boot/dts/mstar-*
> F: arch/arm/mach-mstar/
> F: drivers/clk/mstar/
> +F: drivers/clocksource/timer-msc313e.c
> F: drivers/gpio/gpio-msc313.c
> F: drivers/rtc/rtc-msc313.c
> F: drivers/watchdog/msc313e_wdt.c
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index f65e31bab9ae..822e711da284 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -671,6 +671,16 @@ config MILBEAUT_TIMER
> help
> Enables the support for Milbeaut timer driver.
>
> +config MSC313E_TIMER
> + bool "MSC313E timer driver"

Silent option please and platform config option enables it.

> + depends on ARCH_MSTARV7 || COMPILE_TEST
> + select TIMER_OF
> + select CLKSRC_MMIO
> + help
> + Enables support for the MStar MSC313E timer driver.
> + This provides access to multiple interrupt generating
> + programmable 32-bit free running incrementing counters.
> +
> config INGENIC_TIMER
> bool "Clocksource/timer using the TCU in Ingenic JZ SoCs"
> default MACH_INGENIC
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index c17ee32a7151..fa5f624eadb6 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -88,3 +88,4 @@ obj-$(CONFIG_CSKY_MP_TIMER) += timer-mp-csky.o
> obj-$(CONFIG_GX6605S_TIMER) += timer-gx6605s.o
> obj-$(CONFIG_HYPERV_TIMER) += hyperv_timer.o
> obj-$(CONFIG_MICROCHIP_PIT64B) += timer-microchip-pit64b.o
> +obj-$(CONFIG_MSC313E_TIMER) += timer-msc313e.o
> diff --git a/drivers/clocksource/timer-msc313e.c b/drivers/clocksource/timer-msc313e.c
> new file mode 100644
> index 000000000000..81f161744349
> --- /dev/null
> +++ b/drivers/clocksource/timer-msc313e.c
> @@ -0,0 +1,228 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * MStar timer driver
> + *
> + * Copyright (C) 2021 Daniel Palmer
> + * Copyright (C) 2021 Romain Perier
> + *
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clockchips.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/irqreturn.h>
> +#include <linux/sched_clock.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/delay.h>
> +
> +#include "timer-of.h"
> +
> +#define TIMER_NAME "msc313e_timer"
> +
> +#define MSC313E_REG_CTRL 0x00
> +#define MSC313E_REG_CTRL_TIMER_EN BIT(0)
> +#define MSC313E_REG_CTRL_TIMER_TRIG BIT(1)
> +#define MSC313E_REG_CTRL_TIMER_INT_EN BIT(8)
> +#define MSC313E_REG_TIMER_MAX_LOW 0x08
> +#define MSC313E_REG_TIMER_MAX_HIGH 0x0c
> +#define MSC313E_REG_COUNTER_LOW 0x10
> +#define MSC313E_REG_COUNTER_HIGH 0x14
> +
> +#define TIMER_SYNC_TICKS 3
> +
> +struct msc313e_delay {
> + void __iomem *base;
> + struct delay_timer delay;
> +};
> +
> +static void __iomem *msc313e_clksrc;
> +static struct msc313e_delay msc313e_delay;

I'm not sure that compiles on other platform than mstarv7

> +static void msc313e_timer_stop(void __iomem *base)
> +{
> + writew(0, base + MSC313E_REG_CTRL);
> +}
> +
> +static void msc313e_timer_start(void __iomem *base, bool periodic)
> +{
> + u16 reg;
> +
> + reg = readw(base + MSC313E_REG_CTRL);
> + if (periodic)
> + reg |= MSC313E_REG_CTRL_TIMER_EN;
> + else
> + reg |= MSC313E_REG_CTRL_TIMER_TRIG;
> + writew(reg | MSC313E_REG_CTRL_TIMER_INT_EN, base + MSC313E_REG_CTRL);
> +}
> +
> +static void msc313e_timer_setup(void __iomem *base, unsigned long delay)
> +{
> + writew(delay >> 16, base + MSC313E_REG_TIMER_MAX_HIGH);
> + writew(delay & 0xffff, base + MSC313E_REG_TIMER_MAX_LOW);

Is it safe? (eg. arc_timer.c?)

> +}
> +
> +static unsigned long msc313e_timer_current_value(void __iomem *base)
> +{
> + unsigned long result;
> +
> + result = readw(base + MSC313E_REG_COUNTER_LOW);
> + result |= readw(base + MSC313E_REG_COUNTER_HIGH) << 16;

Is it safe?

cf. timer-microchip-pit64b.c and/or arc_timer.c

> + return result;
> +}
> +
> +static int msc313e_timer_clkevt_shutdown(struct clock_event_device *evt)
> +{
> + struct timer_of *timer = to_timer_of(evt);
> +
> + msc313e_timer_stop(timer_of_base(timer));
> +
> + return 0;
> +}
> +
> +static int msc313e_timer_clkevt_set_oneshot(struct clock_event_device *evt)
> +{
> + struct timer_of *timer = to_timer_of(evt);
> +
> + msc313e_timer_stop(timer_of_base(timer));
> + msc313e_timer_start(timer_of_base(timer), false);
> +
> + return 0;
> +}
> +
> +static int msc313e_timer_clkevt_set_periodic(struct clock_event_device *evt)
> +{
> + struct timer_of *timer = to_timer_of(evt);
> +
> + msc313e_timer_stop(timer_of_base(timer));
> + msc313e_timer_setup(timer_of_base(timer), timer_of_period(timer));
> + msc313e_timer_start(timer_of_base(timer), true);
> +
> + return 0;
> +}
> +
> +static int msc313e_timer_clkevt_next_event(unsigned long evt, struct clock_event_device *clkevt)
> +{
> + struct timer_of *timer = to_timer_of(clkevt);
> +
> + msc313e_timer_stop(timer_of_base(timer));
> + msc313e_timer_setup(timer_of_base(timer), evt);
> + msc313e_timer_start(timer_of_base(timer), false);
> +
> + return 0;
> +}
> +
> +static irqreturn_t msc313e_timer_clkevt_irq(int irq, void *dev_id)
> +{
> + struct clock_event_device *evt = dev_id;
> +
> + evt->event_handler(evt);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static u64 msc313e_timer_clksrc_read(struct clocksource *cs)
> +{
> + return msc313e_timer_current_value(msc313e_clksrc) & cs->mask;
> +}
> +
> +static unsigned long msc313e_read_delay_timer_read(void)
> +{
> + return msc313e_timer_current_value(msc313e_delay.base);
> +}
> +
> +static u64 msc313e_timer_sched_clock_read(void)
> +{
> + return msc313e_timer_current_value(msc313e_clksrc);
> +}
> +
> +static struct clock_event_device msc313e_clkevt = {
> + .name = TIMER_NAME,
> + .rating = 300,
> + .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
> + .set_state_shutdown = msc313e_timer_clkevt_shutdown,
> + .set_state_periodic = msc313e_timer_clkevt_set_periodic,
> + .set_state_oneshot = msc313e_timer_clkevt_set_oneshot,
> + .tick_resume = msc313e_timer_clkevt_shutdown,
> + .set_next_event = msc313e_timer_clkevt_next_event,
> +};
> +
> +static int __init msc313e_clkevt_init(struct device_node *np)
> +{
> + int ret;
> + struct timer_of *to;
> +
> + to = kzalloc(sizeof(*to), GFP_KERNEL);
> + if (!to)
> + return -ENOMEM;
> +
> + to->flags = TIMER_OF_IRQ | TIMER_OF_CLOCK | TIMER_OF_BASE;
> + to->of_irq.handler = msc313e_timer_clkevt_irq;
> + ret = timer_of_init(np, to);
> + if (ret)
> + return ret;
> +
> + msc313e_clkevt.cpumask = cpu_possible_mask;
> + msc313e_clkevt.irq = to->of_irq.irq;
> + to->clkevt = msc313e_clkevt;
> +
> + clockevents_config_and_register(&to->clkevt, timer_of_rate(to),
> + TIMER_SYNC_TICKS, 0xffffffff);
> + return 0;
> +}
> +
> +static int __init msc313e_clksrc_init(struct device_node *np)
> +{
> + struct timer_of to = { 0 };
> + int ret;
> + u16 reg;
> +
> + to.flags = TIMER_OF_BASE | TIMER_OF_CLOCK;
> + ret = timer_of_init(np, &to);
> + if (ret)
> + return ret;
> +
> + msc313e_delay.base = timer_of_base(&to);
> + msc313e_delay.delay.read_current_timer = msc313e_read_delay_timer_read;
> + msc313e_delay.delay.freq = timer_of_rate(&to);
> +
> + msc313e_clksrc = timer_of_base(&to);
> + reg = readw(msc313e_clksrc + MSC313E_REG_CTRL);
> + reg |= MSC313E_REG_CTRL_TIMER_EN;
> + writew(reg, msc313e_clksrc + MSC313E_REG_CTRL);
> +
> + register_current_timer_delay(&msc313e_delay.delay);
> +
> + sched_clock_register(msc313e_timer_sched_clock_read, 32, timer_of_rate(&to));
> + return clocksource_mmio_init(timer_of_base(&to), TIMER_NAME, timer_of_rate(&to), 300, 32,
> + msc313e_timer_clksrc_read);

format 80char max please, run checkpatch.pl before submitting

> +}
> +
> +static int __init msc313e_timer_init(struct device_node *np)
> +{
> + int ret = 0;
> + static int num_called;
> +
> + switch (num_called) {
> + case 0:
> + ret = msc313e_clksrc_init(np);
> + if (ret)
> + return ret;
> + break;
> +
> + default:
> + ret = msc313e_clkevt_init(np);
> + if (ret)
> + return ret;
> + break;
> + }
> +
> + num_called++;
> +
> + return 0;
> +}
> +
> +TIMER_OF_DECLARE(msc313, "mstar,msc313e-timer", msc313e_timer_init);
>


--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog