| Message ID | 1403174303-25456-12-git-send-email-daniel.thompson@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. 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. --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 > } > > /* >
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). > 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 >> } >> >> /* >>
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 } >>> >>> /* >>> >
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 <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 >>> } >>> >>> /* >>> >
On 19/06/14 13:01, Srinivas Kandagatla wrote: >>> 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? I didn't give any thought at all to other ST drivers. I don't see why a *general* preference (of mine or anyone else) would override what is right for any particular driver. I don't think "both manage ST peripherals" means drivers have much in common. >>> 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. I really struggle to understand this. Why would anyone copy code out of the asc driver into the network driver (or any other ST driver)? >> 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. I'm slightly uneasy about this primarily because all the rationale above describes a concern about drivers other than the one I seek to change. They ought to be outside the scope of this change. Nevertheless, since I said I don't feel that strongly about it, as you wish... I'll change this in v5.
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 } /*
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. 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(-)