Re: [PATCH v6 5/5] clocksource/drivers/timer-mediatek: Add support for system timer

[Stanley Chu]

On Thu, 2018-07-05 at 02:13 +0200, Daniel Lezcano wrote:
> On 04/07/2018 16:50, Stanley Chu wrote:

Hi Daniel,

Well remove duplicate one in v7.

> > +
> > +static void __iomem *gpt_sched_reg __read_mostly;
> 
> duplicate 'gpt_sched_reg' variable.
> 
> 
> > +static void mtk_syst_ack_irq(struct timer_of *to)
> > +{
> > +	/* Clear and disable interrupt */
> > +	writel(SYST_CON_IRQ_CLR | SYST_CON_EN, SYST_CON_REG(to));
> > +}
> > +
> > +static irqreturn_t mtk_syst_handler(int irq, void *dev_id)
> > +{
> > +	struct clock_event_device *clkevt = dev_id;
> > +	struct timer_of *to = to_timer_of(clkevt);
> > +
> > +	mtk_syst_ack_irq(to);
> > +	clkevt->event_handler(clkevt);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int mtk_syst_clkevt_next_event(unsigned long ticks,
> > +				      struct clock_event_device *clkevt)
> > +{
> > +	struct timer_of *to = to_timer_of(clkevt);
> > +
> > +	/* Enable clock to allow timeout tick update later */
> > +	writel(SYST_CON_EN, SYST_CON_REG(to));
> > +
> > +	/*
> > +	 * Write new timeout ticks. Timer shall start countdown
> > +	 * after timeout ticks are updated.
> > +	 */
> > +	writel(ticks, SYST_VAL_REG(to));
> > +
> > +	/* Enable interrupt */
> > +	writel(SYST_CON_EN | SYST_CON_IRQ_EN, SYST_CON_REG(to));
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_syst_clkevt_shutdown(struct clock_event_device *clkevt)
> > +{
> > +	/* Disable timer */
> > +	writel(0, SYST_CON_REG(to_timer_of(clkevt)));
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_syst_clkevt_resume(struct clock_event_device *clkevt)
> > +{
> > +	return mtk_syst_clkevt_shutdown(clkevt);
> > +}
> > +
> > +static int mtk_syst_clkevt_oneshot(struct clock_event_device *clkevt)
> > +{
> > +	return 0;
> > +}
> > +
> >  static u64 notrace mtk_gpt_read_sched_clock(void)
> >  {
> >  	return readl_relaxed(gpt_sched_reg);
> > @@ -186,6 +266,34 @@ static void mtk_gpt_enable_irq(struct timer_of *to, u8 timer)
> >  	},
> >  };
> >  
> > +static int __init mtk_syst_init(struct device_node *node)
> > +{
> > +	int ret;
> > +
> > +	to.clkevt.features = CLOCK_EVT_FEAT_DYNIRQ | CLOCK_EVT_FEAT_ONESHOT;
> > +	to.clkevt.set_state_shutdown = mtk_syst_clkevt_shutdown;
> > +	to.clkevt.set_state_oneshot = mtk_syst_clkevt_oneshot;
> > +	to.clkevt.tick_resume = mtk_syst_clkevt_resume;
> > +	to.clkevt.set_next_event = mtk_syst_clkevt_next_event;
> > +	to.of_irq.handler = mtk_syst_handler;
> > +
> > +	ret = timer_of_init(node, &to);
> > +	if (ret)
> > +		goto err;
> > +
> > +	clockevents_config_and_register(&to.clkevt, timer_of_rate(&to),
> > +					TIMER_SYNC_TICKS, 0xffffffff);
> > +
> > +	pr_info("irq=%d, rate=%lu, max_ns: %llu, min_ns: %llu\n",
> > +		timer_of_irq(&to), timer_of_rate(&to),
> > +		to.clkevt.max_delta_ns, to.clkevt.min_delta_ns);
> 
> Please remove this trace. Most of this information is already available
> in /proc/timer_list.
> 
> However, if you think it is worth to add this trace, do a proposition in
> the time framework directly so all drivers can benefit this change.
> 

OK! Will remove this log in v7.

> > +	return 0;
> > +err:
> > +	timer_of_cleanup(&to);
> > +	return ret;
> > +}
> > +
> >  static int __init mtk_gpt_init(struct device_node *node)
> >  {
> >  	int ret;
> > @@ -218,9 +326,9 @@ static int __init mtk_gpt_init(struct device_node *node)
> >  	mtk_gpt_enable_irq(&to, TIMER_CLK_EVT);
> >  
> >  	return 0;
> > -
> >  err:
> >  	timer_of_cleanup(&to);
> >  	return ret;
> >  }
> >  TIMER_OF_DECLARE(mtk_mt6577, "mediatek,mt6577-timer", mtk_gpt_init);
> > +TIMER_OF_DECLARE(mtk_mt6765, "mediatek,mt6765-timer", mtk_syst_init);
> > 
> 
>