Hi Daniel,

On 06/19/2014 01:46 PM, Daniel Thompson wrote:

On 19/06/14 12:29, Srinivas Kandagatla wrote:

quoted

Hi Dan,

On 19/06/14 11:38, Daniel Thompson wrote:

quoted

The architectures supported by this driver have expensive 
implementations of writel(), reliant on spin locks and explicit
L2 cache management. These architectures provide a cheaper
writel_relaxed() which is much better suited to peripherals
that do not perform DMA. The situation with
readl()/readl_relaxed()is similar although less acute.

This driver does not use DMA and will be more power efficient
and more robust (due to absense of spin locks during console
I/O) if it uses the relaxed variants.

This driver is cross compilable for testing purposes and
remains compilable on all architectures by falling back to
writel() when writel_relaxed() does not exist. We also include
explicit compiler barriers. There are redundant on ARM and SH
but important on x86 because it defines "relaxed" differently.

Why are we concern about x86 for this driver? As per my
understanding this IP is only seen on ARM and SH based CPUs so 
why cant we just use relaxed versions, why ifdefs? I think, this
would involve fixing the kconfig and make it depend on SH and ARM
based platforms only.

You mean just drop the COMPILE_TEST?

In generally I like as much code as possible to compile on x86.
Its worthwhile protection against the excessive/accidental ARMisms
which could easily impact less common architectures (such as SH).

Personally, dropping COMPILE_TEST is what I would prefer.

Thanks,
Maxime

quoted

On the other hand, This patch looks more generic and applicable
to most of the drivers. Am not sure which way is the right one.

I'm particularly keen on doing the right thing where
readl_relaxed() is concerned because this function has a compiler
barrier on ARM but not on x86.

Since having asc_in/asc_out made it easy to portably make these
changes I decided is was better to be redundantly exemplary than
conceal secret portability issues.

Don't feel that strongly though. Can easily change it if you're
unconvinced.

quoted

--srini

quoted

Signed-off-by: Daniel Thompson <redacted> Cc:
Srinivas Kandagatla [off-list ref] Cc: Maxime
Coquelin [off-list ref] Cc: Patrice Chotard
[off-list ref] Cc: Greg Kroah-Hartman
[off-list ref] Cc: Jiri Slaby [off-list ref] 
Cc: kernel@stlinux.com Cc: linux-serial@vger.kernel.org --- 
drivers/tty/serial/st-asc.c | 11 ++++++++++- 1 file changed, 10
insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/st-asc.c

b/drivers/tty/serial/st-asc.c index 4f376d8..58aa1c6 100644 ---
a/drivers/tty/serial/st-asc.c +++
b/drivers/tty/serial/st-asc.c @@ -152,12 +152,21 @@ static
inline struct asc_port *to_asc_port(struct uart_port *port)

static inline u32 asc_in(struct uart_port *port, u32 offset) { 
-    return readl(port->membase + offset); +    u32 r; + +    r
= readl_relaxed(port->membase + offset); +    barrier(); +
return r; }

static inline void asc_out(struct uart_port *port, u32 offset,
u32 value) { +#ifdef writel_relaxed +    writel_relaxed(value,
port->membase + offset); +    barrier(); +#else writel(value,
port->membase + offset); +#endif }

/*