* [PATCH 3.17-rc3] serial: asc: Adopt readl_/writel_relaxed() @ 2014-09-03 12:00 Daniel Thompson 2014-09-08 23:27 ` Greg Kroah-Hartman 2014-09-09 10:03 ` [PATCH 3.17-rc3 v2] " Daniel Thompson 0 siblings, 2 replies; 5+ messages in thread From: Daniel Thompson @ 2014-09-03 12:00 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Daniel Thompson, linux-kernel, patches, linaro-kernel, Srinivas Kandagatla, Patrice Chotard, Jiri Slaby, kernel, linux-serial The architectures where this peripheral exists (ARM and SH) 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 change means the driver is no longer portable and therefore no longer suitable for compile testing. Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org> Cc: Srinivas Kandagatla <srinivas.kandagatla@gmail.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 Acked-by: Maxime Coquelin <maxime.coquelin@st.com> Acked-by: Peter Griffin <peter.griffin@linaro.org> --- drivers/tty/serial/Kconfig | 2 +- drivers/tty/serial/st-asc.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig index 26cec64d..e9b1735 100644 --- a/drivers/tty/serial/Kconfig +++ b/drivers/tty/serial/Kconfig @@ -1527,7 +1527,7 @@ config SERIAL_FSL_LPUART_CONSOLE config SERIAL_ST_ASC tristate "ST ASC serial port support" select SERIAL_CORE - depends on ARM || COMPILE_TEST + depends on ARM help This driver is for the on-chip Asychronous Serial Controller on STMicroelectronics STi SoCs. diff --git a/drivers/tty/serial/st-asc.c b/drivers/tty/serial/st-asc.c index 8b2d735..adadbc1 100644 --- a/drivers/tty/serial/st-asc.c +++ b/drivers/tty/serial/st-asc.c @@ -151,12 +151,12 @@ 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); + return readl_relaxed(port->membase + offset); } static inline void asc_out(struct uart_port *port, u32 offset, u32 value) { - writel(value, port->membase + offset); + writel_relaxed(value, port->membase + offset); } /* -- 1.9.3 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 3.17-rc3] serial: asc: Adopt readl_/writel_relaxed() 2014-09-03 12:00 [PATCH 3.17-rc3] serial: asc: Adopt readl_/writel_relaxed() Daniel Thompson @ 2014-09-08 23:27 ` Greg Kroah-Hartman 2014-09-09 9:38 ` Daniel Thompson 2014-09-09 10:03 ` [PATCH 3.17-rc3 v2] " Daniel Thompson 1 sibling, 1 reply; 5+ messages in thread From: Greg Kroah-Hartman @ 2014-09-08 23:27 UTC (permalink / raw) To: Daniel Thompson Cc: linux-kernel, patches, linaro-kernel, Srinivas Kandagatla, Patrice Chotard, Jiri Slaby, kernel, linux-serial On Wed, Sep 03, 2014 at 01:00:51PM +0100, Daniel Thompson wrote: > The architectures where this peripheral exists (ARM and SH) 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 change means the driver is no longer portable and therefore no > longer suitable for compile testing. > > Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org> > Cc: Srinivas Kandagatla <srinivas.kandagatla@gmail.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 > Acked-by: Maxime Coquelin <maxime.coquelin@st.com> > Acked-by: Peter Griffin <peter.griffin@linaro.org> > --- > drivers/tty/serial/Kconfig | 2 +- > drivers/tty/serial/st-asc.c | 4 ++-- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig > index 26cec64d..e9b1735 100644 > --- a/drivers/tty/serial/Kconfig > +++ b/drivers/tty/serial/Kconfig > @@ -1527,7 +1527,7 @@ config SERIAL_FSL_LPUART_CONSOLE > config SERIAL_ST_ASC > tristate "ST ASC serial port support" > select SERIAL_CORE > - depends on ARM || COMPILE_TEST > + depends on ARM I really don't like stuff that does this, sorry. I want to test build as many drivers as I can. COMPILE_TEST does not mean that the driver is "portable", only that it builds properly on all platforms. > help > This driver is for the on-chip Asychronous Serial Controller on > STMicroelectronics STi SoCs. > diff --git a/drivers/tty/serial/st-asc.c b/drivers/tty/serial/st-asc.c > index 8b2d735..adadbc1 100644 > --- a/drivers/tty/serial/st-asc.c > +++ b/drivers/tty/serial/st-asc.c > @@ -151,12 +151,12 @@ 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); > + return readl_relaxed(port->membase + offset); What plaforms do not provide readl_relaxed()? thanks, greg k-h ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 3.17-rc3] serial: asc: Adopt readl_/writel_relaxed() 2014-09-08 23:27 ` Greg Kroah-Hartman @ 2014-09-09 9:38 ` Daniel Thompson 0 siblings, 0 replies; 5+ messages in thread From: Daniel Thompson @ 2014-09-09 9:38 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: linux-kernel, patches, linaro-kernel@lists.linaro.org, Patrice Chotard, Jiri Slaby, kernel, linux-serial, Maxime COQUELIN, Srinivas Kandagatla On 09/09/14 00:27, Greg Kroah-Hartman wrote: > On Wed, Sep 03, 2014 at 01:00:51PM +0100, Daniel Thompson wrote: >> The architectures where this peripheral exists (ARM and SH) 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 change means the driver is no longer portable and therefore no >> longer suitable for compile testing. >> >> Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org> >> Cc: Srinivas Kandagatla <srinivas.kandagatla@gmail.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 >> Acked-by: Maxime Coquelin <maxime.coquelin@st.com> >> Acked-by: Peter Griffin <peter.griffin@linaro.org> >> --- >> drivers/tty/serial/Kconfig | 2 +- >> drivers/tty/serial/st-asc.c | 4 ++-- >> 2 files changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig >> index 26cec64d..e9b1735 100644 >> --- a/drivers/tty/serial/Kconfig >> +++ b/drivers/tty/serial/Kconfig >> @@ -1527,7 +1527,7 @@ config SERIAL_FSL_LPUART_CONSOLE >> config SERIAL_ST_ASC >> tristate "ST ASC serial port support" >> select SERIAL_CORE >> - depends on ARM || COMPILE_TEST >> + depends on ARM > > I really don't like stuff that does this, sorry. I want to test build > as many drivers as I can. COMPILE_TEST does not mean that the driver is > "portable", only that it builds properly on all platforms. I originally made this change compilable (and portable) but was asked to change it during review: http://thread.gmane.org/gmane.linux.ports.arm.kernel/331027/focus=333911 It sounds like I gave in too quickly. I'll re-post the original version shortly. >> help >> This driver is for the on-chip Asychronous Serial Controller on >> STMicroelectronics STi SoCs. >> diff --git a/drivers/tty/serial/st-asc.c b/drivers/tty/serial/st-asc.c >> index 8b2d735..adadbc1 100644 >> --- a/drivers/tty/serial/st-asc.c >> +++ b/drivers/tty/serial/st-asc.c >> @@ -151,12 +151,12 @@ 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); >> + return readl_relaxed(port->membase + offset); > > What plaforms do not provide readl_relaxed()? I'd never thought to ask that. However I think the answer is "only those that use asm-generic/io.h" meaning: blackfin, m68k, metag, openrisc, score and sparc. I'll look into a patch to fix that... The reason I never thought much about readl_relaxed() is that the compilability concerns centre around writel_relaxed() instead. This is much less widely implemented. It appears mostly on architectures where writel() is both expensive and (sometimes) overkill. It is currently found only on: alpha, arm, arm64, avr32, hexagon, microblaze, mips and sh. My original code to conceal the difference between the two looked like this (and the change is to a single accessor function, not littered though the code): --- cut here --- 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 } --- cut here --- Note that barrier() is not needed if our only goal is for the driver to pass the COMPILE_TEST but was included because different architectures have different rules about inclusion of barrier() within the _relaxed() macros making explicit barriers useful if this code were consumed by a copy 'n paste operation... ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 3.17-rc3 v2] serial: asc: Adopt readl_/writel_relaxed() 2014-09-03 12:00 [PATCH 3.17-rc3] serial: asc: Adopt readl_/writel_relaxed() Daniel Thompson 2014-09-08 23:27 ` Greg Kroah-Hartman @ 2014-09-09 10:03 ` Daniel Thompson [not found] ` <CALNtEFgd=Lw3AdO2cq_X66_kVdjRyu8sVKN5UX3tzKS5c6d_JA@mail.gmail.com> 1 sibling, 1 reply; 5+ messages in thread From: Daniel Thompson @ 2014-09-09 10:03 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Daniel Thompson, linux-kernel, patches, linaro-kernel, Srinivas Kandagatla, Maxime Coquelin, Patrice Chotard, Jiri Slaby, kernel, linux-serial The architectures supported by this driver, arm and sh, 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 absence of spin locks during console I/O) if it uses the relaxed variants. The driver supports COMPILE_TEST and therefore falls back to writel() when writel_relaxed() does not exist. 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 --- Notes: Changes since v1: * Added fallback to writel() to permit COMPILE_TESTing (review of Greg Kroah-Hartmam). drivers/tty/serial/st-asc.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/tty/serial/st-asc.c b/drivers/tty/serial/st-asc.c index 8b2d735..a3fc167 100644 --- a/drivers/tty/serial/st-asc.c +++ b/drivers/tty/serial/st-asc.c @@ -151,12 +151,16 @@ 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); + return readl_relaxed(port->membase + offset); } static inline void asc_out(struct uart_port *port, u32 offset, u32 value) { +#ifdef writel_relaxed + writel_relaxed(value, port->membase + offset); +#else writel(value, port->membase + offset); +#endif } /* -- 1.9.3 ^ permalink raw reply related [flat|nested] 5+ messages in thread
[parent not found: <CALNtEFgd=Lw3AdO2cq_X66_kVdjRyu8sVKN5UX3tzKS5c6d_JA@mail.gmail.com>]
* Re: [PATCH 3.17-rc3 v2] serial: asc: Adopt readl_/writel_relaxed() [not found] ` <CALNtEFgd=Lw3AdO2cq_X66_kVdjRyu8sVKN5UX3tzKS5c6d_JA@mail.gmail.com> @ 2014-09-09 11:15 ` Daniel Thompson 0 siblings, 0 replies; 5+ messages in thread From: Daniel Thompson @ 2014-09-09 11:15 UTC (permalink / raw) To: Srinivas Kandagatla Cc: Greg Kroah-Hartman, linux-kernel, patches, linaro-kernel, Maxime Coquelin, Patrice Chotard, Jiri Slaby, kernel, linux-serial On 09/09/14 12:08, Srinivas Kandagatla wrote: > Hi Dan, > > After Greg's comment, Am ok with the patch and sorry for the noise. > Good that you removed the barier() too. Yes. I got interested in Greg's question about which architectures don't have readl_relaxed() and, in the process, discovered I was wrong about that being needed anyway. > You can have my Ack. > > --srini > > On Tue, Sep 9, 2014 at 11:03 AM, Daniel Thompson > <daniel.thompson@linaro.org <mailto:daniel.thompson@linaro.org>> wrote: > > The architectures supported by this driver, arm and sh, 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 absence of spin locks during console I/O) if it uses the > relaxed variants. > > The driver supports COMPILE_TEST and therefore falls back to writel() > when writel_relaxed() does not exist. > > Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org > <mailto:daniel.thompson@linaro.org>> > Cc: Srinivas Kandagatla <srinivas.kandagatla@gmail.com > <mailto:srinivas.kandagatla@gmail.com>> > Cc: Maxime Coquelin <maxime.coquelin@st.com > <mailto:maxime.coquelin@st.com>> > Cc: Patrice Chotard <patrice.chotard@st.com > <mailto:patrice.chotard@st.com>> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org > <mailto:gregkh@linuxfoundation.org>> > Cc: Jiri Slaby <jslaby@suse.cz <mailto:jslaby@suse.cz>> > Cc: kernel@stlinux.com <mailto:kernel@stlinux.com> > Cc: linux-serial@vger.kernel.org <mailto:linux-serial@vger.kernel.org> > --- > > Notes: > Changes since v1: > > * Added fallback to writel() to permit COMPILE_TESTing (review > of Greg Kroah-Hartmam). > > drivers/tty/serial/st-asc.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/tty/serial/st-asc.c b/drivers/tty/serial/st-asc.c > index 8b2d735..a3fc167 100644 > --- a/drivers/tty/serial/st-asc.c > +++ b/drivers/tty/serial/st-asc.c > @@ -151,12 +151,16 @@ 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); > + return readl_relaxed(port->membase + offset); > } > > static inline void asc_out(struct uart_port *port, u32 offset, u32 > value) > { > +#ifdef writel_relaxed > + writel_relaxed(value, port->membase + offset); > +#else > writel(value, port->membase + offset); > +#endif > } > > /* > -- > 1.9.3 > > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-09-09 11:15 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-09-03 12:00 [PATCH 3.17-rc3] serial: asc: Adopt readl_/writel_relaxed() Daniel Thompson
2014-09-08 23:27 ` Greg Kroah-Hartman
2014-09-09 9:38 ` Daniel Thompson
2014-09-09 10:03 ` [PATCH 3.17-rc3 v2] " Daniel Thompson
[not found] ` <CALNtEFgd=Lw3AdO2cq_X66_kVdjRyu8sVKN5UX3tzKS5c6d_JA@mail.gmail.com>
2014-09-09 11:15 ` Daniel Thompson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).