Re: [PATCH v4 11/13] serial: asc: Adopt readl_/writel_relaxed()

[Maxime Coquelin <maxime.coquelin@st.com>]

Hi Daniel,

On 06/19/2014 01:46 PM, Daniel Thompson wrote:
> On 19/06/14 12:29, Srinivas Kandagatla wrote:
>> Hi Dan,
>> 
>> On 19/06/14 11:38, Daniel Thompson wrote:
>>> 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


> 
> 
>> 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.
> 
> 
> 
>> 
>> 
>> --srini
>> 
>>> Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org> Cc:
>>> Srinivas Kandagatla <srinivas.kandagatla@gmail.com> Cc: Maxime
>>> Coquelin <maxime.coquelin@st.com> Cc: Patrice Chotard
>>> <patrice.chotard@st.com> Cc: Greg Kroah-Hartman
>>> <gregkh@linuxfoundation.org> Cc: Jiri Slaby <jslaby@suse.cz> 
>>> 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 }
>>> 
>>> /*
>>> 
>