On Monday 16 December 2013 03:40 PM, Stephen Boyd wrote:

On 12/16, ivan.khoronzhuk wrote:

quoted

On 12/13/2013 03:42 AM, Stephen Boyd wrote:

quoted

On 12/11/13 10:00, Ivan Khoronzhuk wrote:

quoted

+
+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.

I agree with Stephen.

Regards,
Santosh