Re: [PATCH 2/2] clocksource/drivers/mtk_systimer: Add support for Mediatek SoCs

[Stanley Chu]

On Tue, 2018-06-26 at 09:41 +0200, Daniel Lezcano wrote:
> On 25/06/2018 09:10, Stanley Chu wrote:
> > This patch adds a new clock event for the timer
> > found on the Mediatek SoCs.
> > 
> > The Mediatek System Timer has several 32-bit timers.
> > Only one timer is used by this driver as a clock event
> > supporting oneshot events.
> > 
> > The System Timer can be run with two clocks. A 13 MHz system
> > clock and the RTC clock running at 32 KHz. This implementation
> > uses the system clock with no clock source divider.
> > 
> > The interrupts are shared between the different timers.
> > We just enable one interrupt for the clock event. The clock
> > event timer is used by all cores.
> > 
> > Signed-off-by: Stanley Chu <stanley.chu@xxxxxxxxxxxx>
> > ---
> >  drivers/clocksource/Kconfig        |   13 ++-
> >  drivers/clocksource/Makefile       |    1 +
> >  drivers/clocksource/mtk_systimer.c |  177 ++++++++++++++++++++++++++++++++++++
> >
> 
> >  3 files changed, 189 insertions(+), 2 deletions(-)
> >  create mode 100644 drivers/clocksource/mtk_systimer.c
> > 
> > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> > index dec0dd8..362c110 100644
> > --- a/drivers/clocksource/Kconfig
> > +++ b/drivers/clocksource/Kconfig
> > @@ -442,12 +442,21 @@ config SYS_SUPPORTS_SH_CMT
> >          bool
> >  
> >  config MTK_TIMER
> > -	bool "Mediatek timer driver" if COMPILE_TEST
> > +	bool "Mediatek general purpose timer driver" if COMPILE_TEST
> >  	depends on HAS_IOMEM
> >  	select TIMER_OF
> >  	select CLKSRC_MMIO
> >  	help
> > -	  Support for Mediatek timer driver.
> > +	  Support for Mediatek General Purpose Timer (GPT) driver.
> > +
> > +config MTK_TIMER_SYSTIMER
> > +	bool "Mediatek system timer driver" if COMPILE_TEST
> > +	depends on HAS_IOMEM
> > +	select TIMER_OF
> > +	select CLKSRC_MMIO
> > +	help
> > +	  Support for Mediatek System Timer (sys_timer) driver used as
> > +	  a clock event supporting oneshot events.
> >  
> >  config SPRD_TIMER
> >  	bool "Spreadtrum timer driver" if EXPERT
> > diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> > index 00caf37..cdd34b2 100644
> > --- a/drivers/clocksource/Makefile
> > +++ b/drivers/clocksource/Makefile
> > @@ -50,6 +50,7 @@ obj-$(CONFIG_FSL_FTM_TIMER)	+= fsl_ftm_timer.o
> >  obj-$(CONFIG_VF_PIT_TIMER)	+= vf_pit_timer.o
> >  obj-$(CONFIG_CLKSRC_QCOM)	+= qcom-timer.o
> >  obj-$(CONFIG_MTK_TIMER)		+= mtk_timer.o
> > +obj-$(CONFIG_MTK_TIMER_SYSTIMER)	+= mtk_systimer.o
> >  obj-$(CONFIG_CLKSRC_PISTACHIO)	+= time-pistachio.o
> >  obj-$(CONFIG_CLKSRC_TI_32K)	+= timer-ti-32k.o
> >  obj-$(CONFIG_CLKSRC_NPS)	+= timer-nps.o
> > diff --git a/drivers/clocksource/mtk_systimer.c b/drivers/clocksource/mtk_systimer.c
> > new file mode 100644
> > index 0000000..06e74ce
> > --- /dev/null
> > +++ b/drivers/clocksource/mtk_systimer.c
> > @@ -0,0 +1,177 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +//
> > +//  Copyright (C) 2018 MediaTek Inc.
> > +
> > +#include <linux/init.h>
> > +#include <linux/module.h>
> > +#include <linux/kernel.h>
> > +#include <linux/irq.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/irqreturn.h>
> > +#include <linux/jiffies.h>
> > +#include <linux/clockchips.h>
> > +#include <linux/clocksource.h>
> > +#include <linux/clk.h>
> > +#include <linux/io.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_irq.h>
> > +#include <clocksource/arm_arch_timer.h>
> 
> Do a cleanup with the headers. They are not all necessary.
> 
> > +#define STMR_CLKEVT_DEFAULT_RATE  (13000000)
> > +
> > +/* registers */
> > +#define STMR_CON                  (0x0)
> > +#define STMR_VAL                  (0x4)
> > +
> > +/* STMR_CON */
> > +#define STMR_CON_EN               BIT(0)
> > +#define STMR_CON_IRQ_EN           BIT(1)
> > +#define STMR_CON_IRQ_CLR          BIT(4)
> > +
> > +struct mtk_stmr_clkevt_device {
> > +	void __iomem *base;
> > +	struct clock_event_device dev;
> > +};
> 
> Use the timer_of structure, that will save you some lines of code as
> well as redundant structure definition.
> 
> > +static inline struct mtk_stmr_clkevt_device*
> > +	to_mtk_clkevt_device(struct clock_event_device *c)> +{
> > +	return container_of(c, struct mtk_stmr_clkevt_device, dev);
> > +}
> > +
> > +static void mtk_stmr_reset(struct mtk_stmr_clkevt_device *evt)
> > +{
> > +	/* Clear IRQ */
> > +	writel(STMR_CON_IRQ_CLR | STMR_CON_EN, evt->base + STMR_CON);
> > +
> > +	/* Reset counter */
> > +	writel(0, evt->base + STMR_VAL);
> > +
> > +	/* Disable timer */
> > +	writel(0, evt->base + STMR_CON);
> > +}
> > +
> > +static void mtk_stmr_ack_irq(struct mtk_stmr_clkevt_device *evt)
> > +{
> > +	mtk_stmr_reset(evt);
> > +}
> > +
> > +static irqreturn_t mtk_stmr_handler(int irq, void *priv)
> > +{
> > +	struct mtk_stmr_clkevt_device *evt = priv;
> > +
> > +	mtk_stmr_ack_irq(evt);
> > +	evt->dev.event_handler(&evt->dev);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int mtk_stmr_clkevt_next_event(unsigned long ticks,
> > +				      struct clock_event_device *dev)
> 
> Indent.
> 
> > +{
> > +	struct mtk_stmr_clkevt_device *evt = to_mtk_clkevt_device(dev);
> > +
> > +	/*
> > +	 * reset timer first because we do not expect interrupt is triggered
> > +	 * by old compare value.
> > +	 */
> > +	mtk_stmr_reset(evt);
> > +
> > +	writel(STMR_CON_EN, evt->base + STMR_CON);
> > +
> > +	writel(ticks, evt->base + STMR_VAL);
> > +
> > +	writel(STMR_CON_EN | STMR_CON_IRQ_EN, evt->base + STMR_CON);
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_stmr_clkevt_shutdown(struct clock_event_device *dev)
> > +{
> > +	struct mtk_stmr_clkevt_device *evt = to_mtk_clkevt_device(dev);
> > +
> > +	mtk_stmr_reset(evt);
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_stmr_clkevt_resume(struct clock_event_device *clk)
> > +{
> > +	return mtk_stmr_clkevt_shutdown(clk);
> > +}
> > +
> > +static int mtk_stmr_clkevt_oneshot(struct clock_event_device *clk)
> > +{
> > +	return 0;
> > +}
> > +
> > +static int __init mtk_stmr_init(struct device_node *node)
> > +{
> > +	struct mtk_stmr_clkevt_device *evt;
> > +	struct resource res;
> > +	u32 freq;
> > +
> > +	evt = kzalloc(sizeof(*evt), GFP_KERNEL);
> > +	if (!evt)
> > +		return -ENOMEM;
> > +
> > +	evt->dev.name = "mtk-clkevt";
> > +	evt->dev.shift = 32;
> > +	evt->dev.rating = 300;
> > +	evt->dev.features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_DYNIRQ;
> > +	evt->dev.set_state_shutdown = mtk_stmr_clkevt_shutdown;
> > +	evt->dev.set_state_oneshot = mtk_stmr_clkevt_oneshot;
> > +	evt->dev.tick_resume = mtk_stmr_clkevt_resume;
> > +	evt->dev.set_next_event = mtk_stmr_clkevt_next_event;
> > +	evt->dev.cpumask = cpu_possible_mask;
> > +
> > +	evt->base = of_iomap(node, 0);
> > +	if (IS_ERR(evt->base)) {
> > +		pr_err("Can't get resource\n");
> > +		goto err_kzalloc;
> > +	}
> > +
> > +	mtk_stmr_reset(evt);
> > +
> > +	evt->dev.irq = irq_of_parse_and_map(node, 0);
> > +	if (evt->dev.irq <= 0) {
> > +		pr_err("Can't parse IRQ\n");
> > +		goto err_mem;
> > +	}
> > +
> > +	if (of_property_read_u32(node, "clock-frequency", &freq)) {
> > +		pr_err("Can't get clk rate\n");
> > +		freq = STMR_CLKEVT_DEFAULT_RATE;
> > +	}
> > +
> > +	evt->dev.mult = div_sc(freq, NSEC_PER_SEC, evt->dev.shift);
> > +	evt->dev.max_delta_ns = clockevent_delta2ns(0xffffffff, &evt->dev);
> > +	evt->dev.min_delta_ns = clockevent_delta2ns(3, &evt->dev);
> > +
> > +	clockevents_register_device(&evt->dev);
> > +
> > +	if (request_irq(evt->dev.irq, mtk_stmr_handler,
> > +			IRQF_TIMER | IRQF_IRQPOLL |
> > +			IRQF_TRIGGER_HIGH | IRQF_PERCPU,
> > +			"mtk-clkevt", evt)) {
> > +		pr_err("failed to setup irq %d\n", evt->dev.irq);
> > +		goto err_mem;
> > +	}
> > +
> 
> Remove all the above by using:
> 
>         ret = timer_of_init(node, &to);
>         if (ret)
>                 return ret;
> 
> 
> > +	pr_info("mtk_stmr: base=0x%lx, irq=%d, freq=%d, hz=%d\n",
> > +		(unsigned long)evt->base, evt->dev.irq, freq, HZ);
> > +
> > +	return 0;
> > +
> > +err_mem:
> > +	iounmap(evt->base);
> > +	of_address_to_resource(node, 0, &res);
> > +	release_mem_region(res.start, resource_size(&res));
> > +err_kzalloc:
> > +	kfree(evt);
> > +
> > +	return -EINVAL;
> > +}
> > +
> > +TIMER_OF_DECLARE(mtk_systimer, "mediatek,sys_timer", mtk_stmr_init);
> > 
> 
> 
Hi Daniel,

Good suggestion for timer_of structure! We will use it and fix above all
in v2.

Thanks.
Stanley Chu