From mboxrd@z Thu Jan 1 00:00:00 1970 From: srinivas.kandagatla@linaro.org (Srinivas Kandagatla) Date: Thu, 19 Jun 2014 13:01:57 +0100 Subject: [PATCH v4 11/13] serial: asc: Adopt readl_/writel_relaxed() In-Reply-To: <53A2CD8D.1050106@linaro.org> References: <1401961994-18033-1-git-send-email-daniel.thompson@linaro.org> <1403174303-25456-1-git-send-email-daniel.thompson@linaro.org> <1403174303-25456-12-git-send-email-daniel.thompson@linaro.org> <53A2C9A3.9090507@linaro.org> <53A2CD8D.1050106@linaro.org> Message-ID: <53A2D135.4020700@linaro.org> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 19/06/14 12:46, 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). That's fair. Does this mean that we are going do similar changes to other ST drivers too? > TBH, there is no SH based SOCs in mainline which uses this driver. I don't think ST would add SH only SOCs to mainline in near future. > >> 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. > My only concern is code duplication all across ST drivers. > 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. Your change would fit in nicely with as asc_in/out are wrappers and fix st-asc but this would be just for asc driver. What about other drivers which fall in same category? So I think we should just drop COMPILE_TEST and possibly make it specific to ARM and SH or ARM only. --srini > > Don't feel that strongly though. Can easily change it if you're unconvinced. > > > >> >> >> --srini >> >>> Signed-off-by: Daniel Thompson >>> Cc: Srinivas Kandagatla >>> Cc: Maxime Coquelin >>> Cc: Patrice Chotard >>> Cc: Greg Kroah-Hartman >>> Cc: Jiri Slaby >>> Cc: kernel at stlinux.com >>> Cc: linux-serial at 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 >>> } >>> >>> /* >>> >