| Message ID | 1530669383-17516-6-git-send-email-stanley.chu@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 4 Jul 2018, Stanley Chu wrote: > +/* system timer */ > +#define SYST_CON (0x0) > +#define SYST_VAL (0x4) > + > +#define SYST_CON_REG(to) (timer_of_base(to) + SYST_CON) > +#define SYST_VAL_REG(to) (timer_of_base(to) + SYST_VAL) > + > +#define SYST_CON_EN BIT(0) > +#define SYST_CON_IRQ_EN BIT(1) > +#define SYST_CON_IRQ_CLR BIT(4) > + > +static void __iomem *gpt_sched_reg __read_mostly; > + > +static void mtk_syst_reset(struct timer_of *to) > +{ > + /* clear and disable interrupt */ > + writel(SYST_CON_IRQ_CLR | SYST_CON_EN, SYST_CON_REG(to)); > + > + /* reset counter */ > + writel(0, SYST_VAL_REG(to)); > + > + /* disable timer */ > + writel(0, SYST_CON_REG(to)); Not having any insight into the inner workings of that hardware. This sequence does not make any sense. You first clear and disable the interrupt, but leave the counter running. Then you reset the counter and _after_ that you disable the timer. Is that a workaround for yet another completely misdesigned piece of timer hardware or is it just the software implementation which makes no sense? If it's a workaround for HW stupidity then you really want to document it proper. If not then, what's wrong with doing the obvious: { writel(SYST_CON_IRQ_CLR, SYST_CON_REG(to)); } which clears the interrupt and disables the timer in one go. Writing the counter is not required at all because the next_event() function will write it again and the counter value is irrelevant once the timer is disabled. > +static void mtk_syst_ack_irq(struct timer_of *to) > +{ > + mtk_syst_reset(to); > +} > + > +static irqreturn_t mtk_syst_handler(int irq, void *dev_id) > +{ > + struct clock_event_device *clkevt = (struct clock_event_device *)dev_id; No type casts from and to void. They are completely pointless. > + struct timer_of *to = to_timer_of(clkevt); > + > + mtk_syst_ack_irq(to); So you do the full dance here. > + 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); > + > + /* > + * reset timer first because we do not expect interrupt is triggered > + * by old compare value. > + */ > + mtk_syst_reset(to); And here, which gets called through the event_handler. > + > + writel(SYST_CON_EN, SYST_CON_REG(to)); Why are you enabling the timer _before_ setting the new counter value? This does not make any sense at least not w/o a comment explaining WHY this needs to be so convoluted. > + writel(ticks, SYST_VAL_REG(to)); > + > + writel(SYST_CON_EN | SYST_CON_IRQ_EN, SYST_CON_REG(to)); With a sane hardware implementation the whole thing can be boiled down to: { writel(ticks, SYST_VAL_REG(to)); writel(SYST_CON_EN | SYST_CON_IRQ_EN, SYST_CON_REG(to)); } There is no need to do the whole reset thing, unless the hardware is broken. Either you manage to overwrite the timer in time or not. The extra work of writing the control register before that is a cosmetic 'makes me feel' good hack, which just makes the normal case slower. If you have an occasional spurious interrupt, fine, but you can get them anyway. Thanks, tglx
On Wed, 2018-07-04 at 11:27 +0200, Thomas Gleixner wrote: > On Wed, 4 Jul 2018, Stanley Chu wrote: Hi Thomas, Thanks so much for very constructive comments. Summary of below response, 1, Yes. System timer has a "clock enable (i.e., SYST_CON_EN)" design which not only start timer to countdown but also be required to make interrupt function work. In other words, SYST_CON_EN shall be set before user changes interrupt related registers, for example, enable or disable interrupt. 2. The driver certainly has to be optimized and redundant register operations shall be removed. Both things will be done in v6. > > +/* system timer */ > > +#define SYST_CON (0x0) > > +#define SYST_VAL (0x4) > > + > > +#define SYST_CON_REG(to) (timer_of_base(to) + SYST_CON) > > +#define SYST_VAL_REG(to) (timer_of_base(to) + SYST_VAL) > > + > > +#define SYST_CON_EN BIT(0) > > +#define SYST_CON_IRQ_EN BIT(1) > > +#define SYST_CON_IRQ_CLR BIT(4) > > + > > +static void __iomem *gpt_sched_reg __read_mostly; > > + > > +static void mtk_syst_reset(struct timer_of *to) > > +{ > > + /* clear and disable interrupt */ > > + writel(SYST_CON_IRQ_CLR | SYST_CON_EN, SYST_CON_REG(to)); > > + > > + /* reset counter */ > > + writel(0, SYST_VAL_REG(to)); > > + > > + /* disable timer */ > > + writel(0, SYST_CON_REG(to)); > > Not having any insight into the inner workings of that hardware. This > sequence does not make any sense. > > You first clear and disable the interrupt, but leave the counter > running. Then you reset the counter and _after_ that you disable the > timer. > > Is that a workaround for yet another completely misdesigned piece of timer > hardware or is it just the software implementation which makes no sense? > > If it's a workaround for HW stupidity then you really want to document it > proper. > > If not then, what's wrong with doing the obvious: > { > writel(SYST_CON_IRQ_CLR, SYST_CON_REG(to)); > } As above summary said, SYST_CON_EN shall be set to 1. Change interrupt function. 2. Allow timeout tick to be updated. We will document those as comments in v6. > > which clears the interrupt and disables the timer in one go. Writing the > counter is not required at all because the next_event() function will write > it again and the counter value is irrelevant once the timer is disabled. > > > +static void mtk_syst_ack_irq(struct timer_of *to) > > +{ > > + mtk_syst_reset(to); > > +} > > + > > +static irqreturn_t mtk_syst_handler(int irq, void *dev_id) > > +{ > > + struct clock_event_device *clkevt = (struct clock_event_device *)dev_id; > > No type casts from and to void. They are completely pointless. Will be fixed in v6. > > > + struct timer_of *to = to_timer_of(clkevt); > > + > > + mtk_syst_ack_irq(to); > > So you do the full dance here. Here full dance is not necessary and we only need to "clear and disable interrupt". Will be optimized in v6. > > > + 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); > > + > > + /* > > + * reset timer first because we do not expect interrupt is triggered > > + * by old compare value. > > + */ > > + mtk_syst_reset(to); > > And here, which gets called through the event_handler. Timer reset here is not necessary. Will be removed in v6. > > > + > > + writel(SYST_CON_EN, SYST_CON_REG(to)); > > Why are you enabling the timer _before_ setting the new counter value? This > does not make any sense at least not w/o a comment explaining WHY this > needs to be so convoluted. As above response in mtk_syst_reset(). > > > + writel(ticks, SYST_VAL_REG(to)); > > + > > + writel(SYST_CON_EN | SYST_CON_IRQ_EN, SYST_CON_REG(to)); > > With a sane hardware implementation the whole thing can be boiled down to: > > { > writel(ticks, SYST_VAL_REG(to)); > writel(SYST_CON_EN | SYST_CON_IRQ_EN, SYST_CON_REG(to)); > } > > There is no need to do the whole reset thing, unless the hardware is > broken. Either you manage to overwrite the timer in time or not. The extra > work of writing the control register before that is a cosmetic 'makes me > feel' good hack, which just makes the normal case slower. If you have an > occasional spurious interrupt, fine, but you can get them anyway. > Yes, we have reviewed reset timing and found most of them are not necessary actually. We will optimize driver in v6. > Thanks, > > tglx Thanks, Stanley Chu > Linux-mediatek mailing list > Linux-mediatek@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-mediatek
diff --git a/drivers/clocksource/timer-mediatek.c b/drivers/clocksource/timer-mediatek.c index e57c4d7..44e81b3 100644 --- a/drivers/clocksource/timer-mediatek.c +++ b/drivers/clocksource/timer-mediatek.c @@ -58,6 +58,84 @@ static void __iomem *gpt_sched_reg __read_mostly; +/* system timer */ +#define SYST_CON (0x0) +#define SYST_VAL (0x4) + +#define SYST_CON_REG(to) (timer_of_base(to) + SYST_CON) +#define SYST_VAL_REG(to) (timer_of_base(to) + SYST_VAL) + +#define SYST_CON_EN BIT(0) +#define SYST_CON_IRQ_EN BIT(1) +#define SYST_CON_IRQ_CLR BIT(4) + +static void __iomem *gpt_sched_reg __read_mostly; + +static void mtk_syst_reset(struct timer_of *to) +{ + /* clear and disable interrupt */ + writel(SYST_CON_IRQ_CLR | SYST_CON_EN, SYST_CON_REG(to)); + + /* reset counter */ + writel(0, SYST_VAL_REG(to)); + + /* disable timer */ + writel(0, SYST_CON_REG(to)); +} + +static void mtk_syst_ack_irq(struct timer_of *to) +{ + mtk_syst_reset(to); +} + +static irqreturn_t mtk_syst_handler(int irq, void *dev_id) +{ + struct clock_event_device *clkevt = (struct clock_event_device *)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); + + /* + * reset timer first because we do not expect interrupt is triggered + * by old compare value. + */ + mtk_syst_reset(to); + + writel(SYST_CON_EN, SYST_CON_REG(to)); + + writel(ticks, SYST_VAL_REG(to)); + + 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) +{ + mtk_syst_reset(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 +264,36 @@ 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; + + mtk_syst_reset(&to); + + 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); + + 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);
This patch adds a new "System Timer" on the Mediatek SoCs. The System Timer is introduced as an always-on timer being clockevent device for tick-broadcasting. For clock, it is driven by 13 MHz system clock. The implementation uses the system clock with no clock source divider. For interrupt, the clock event timer can be used by all cores. Signed-off-by: Stanley Chu <stanley.chu@mediatek.com> --- drivers/clocksource/timer-mediatek.c | 110 +++++++++++++++++++++++++++++++++- 1 file changed, 109 insertions(+), 1 deletion(-)