[PATCH 1/3] clocksource: timer-keystone: introduce clocksource driver for Keystone

[sboyd@codeaurora.org (Stephen Boyd)]

On 12/16, ivan.khoronzhuk wrote:
> On 12/13/2013 03:42 AM, Stephen Boyd wrote:
> > On 12/11/13 10:00, Ivan Khoronzhuk wrote:
> >> +
> >> +static inline u32 keystone_timer_readl(unsigned long rg)
> >> +{
> >> +	return readl(timer.base + rg);
> >> +}
> >> +
> >> +static inline void keystone_timer_writel(u32 val, unsigned long rg)
> >> +{
> >> +	writel(val, timer.base + rg);
> >> +}
> > 
> > It's probably better to use the relaxed variants here to avoid any
> > memory barriers that aren't necessary.
> >
> 
> Yes, but the code has places where I cannot use relaxed variants.
> 
> From timer user guide:
> "Writes from the configuration bus to the timer registers are not allowed
> when the timer is active, except for stopping or resetting the timers.
> Registers that are protected by hardware include CNTLO, CNTHI, PRDLO,
> PRDHI, TGCR (except the TIMLORS and TIMHIRS bits), and TCR (except the
> ENAMODE bits)."
> 
> According to this I have to add keystone readl/write relaxed functions
> and use mixed calls of writel/writel_relaxed functions.
> 
> For instance, for keystone_timer_config() which is used by
> keystone_set_next_event(), I will do following:
> -----
> tcr = keystone_timer_readl_relaxed(TCR);
> 
> /* disable timer */
> tcr &= ~(TCR_ENAMODE_MASK);
> keystone_timer_writel_relaxed(tcr, TCR);
> ...
> /* reset counter to zero, set new period */
> *** here I have to be sure the timer is disabled ***
> keystone_timer_writel(0, TIM12);
> keystone_timer_writel_relaxed(0, TIM34);
> keystone_timer_writel_relaxed(period & 0xffffffff, PRD12);
> keystone_timer_writel_relaxed(period >> 32, PRD34);
> 
> /* enable timer */
> *** here I have to be sure that TIM and PRD registers are written ***
> keystone_timer_writel(tcr, TCR);
> ---
> 
> The same for keystone_timer_init().
> 
> Will it be better?

Why not just put the explicit memory barriers where you need them
and always use the relaxed variants in the mmio wrappers? That
way you're always sure the memory barriers are there and that
they're properly documented.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation