[PATCH 1/4] tty: serial: extend lqasc_tx_ready() to lqasc_console_putchar()

[PATCH 1/4] tty: serial: extend lqasc_tx_ready() to lqasc_console_putchar()

[Jiri Slaby]

There is one more place where lqasc_tx_ready() can be used now:
lqasc_console_putchar(). So replace the open-coded variant by the
helper.

Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/tty/serial/lantiq.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serial/lantiq.c b/drivers/tty/serial/lantiq.c
index 6da1b7496c6c..ba9739af30ed 100644
--- a/drivers/tty/serial/lantiq.c
+++ b/drivers/tty/serial/lantiq.c
@@ -606,15 +606,12 @@ static const struct uart_ops lqasc_pops = {
 static void
 lqasc_console_putchar(struct uart_port *port, unsigned char ch)
 {
-	int fifofree;
-
 	if (!port->membase)
 		return;
 
-	do {
-		fifofree = (__raw_readl(port->membase + LTQ_ASC_FSTAT)
-			& ASCFSTAT_TXFREEMASK) >> ASCFSTAT_TXFREEOFF;
-	} while (fifofree == 0);
+	while (!lqasc_tx_ready(port))
+		;
+
 	writeb(ch, port->membase + LTQ_ASC_TBUF);
 }
 
-- 
2.37.3

[PATCH 2/4] tty: serial: use FIELD_GET() in lqasc_tx_ready()

[Jiri Slaby]

FIELD_GET() can do the job smarter and more readable. We don't even need
ASCFSTAT_TXFREEOFF. So switch to the former and remove the latter.

Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/tty/serial/lantiq.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/lantiq.c b/drivers/tty/serial/lantiq.c
index ba9739af30ed..c892f3c7d1ab 100644
--- a/drivers/tty/serial/lantiq.c
+++ b/drivers/tty/serial/lantiq.c
@@ -8,6 +8,7 @@
  * Copyright (C) 2010 Thomas Langer, <thomas.langer@lantiq.com>
  */
 
+#include <linux/bitfield.h>
 #include <linux/clk.h>
 #include <linux/console.h>
 #include <linux/device.h>
@@ -93,7 +94,6 @@
 #define ASCFSTAT_RXFFLMASK	0x003F
 #define ASCFSTAT_TXFFLMASK	0x3F00
 #define ASCFSTAT_TXFREEMASK	0x3F000000
-#define ASCFSTAT_TXFREEOFF	24
 
 static void lqasc_tx_chars(struct uart_port *port);
 static struct ltq_uart_port *lqasc_port[MAXPORTS];
@@ -143,7 +143,7 @@ static bool lqasc_tx_ready(struct uart_port *port)
 {
 	u32 fstat = __raw_readl(port->membase + LTQ_ASC_FSTAT);
 
-	return (fstat & ASCFSTAT_TXFREEMASK) >> ASCFSTAT_TXFREEOFF;
+	return FIELD_GET(ASCFSTAT_TXFREEMASK, fstat);
 }
 
 static void
-- 
2.37.3

[PATCH 3/4] tty: serial: unify TX space reads under altera_jtaguart_tx_space()

[Jiri Slaby]

TX space reads from the control register are performed in various forms
on 4 places in altera_jtaguart. Unify all those and do the read and
masking on a single place.

The new helper altera_jtaguart_tx_space() uses FIELD_GET(), so we can
drop ALTERA_JTAGUART_CONTROL_WSPACE_OFF now.

Cc: Tobias Klauser <tklauser@distanz.ch>
Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/tty/serial/altera_jtaguart.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/serial/altera_jtaguart.c b/drivers/tty/serial/altera_jtaguart.c
index 23f339757894..ac8ce418de36 100644
--- a/drivers/tty/serial/altera_jtaguart.c
+++ b/drivers/tty/serial/altera_jtaguart.c
@@ -9,6 +9,7 @@
  * (C) Copyright 2010, Tobias Klauser <tklauser@distanz.ch>
  */
 
+#include <linux/bitfield.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>
@@ -48,7 +49,6 @@
 #define ALTERA_JTAGUART_CONTROL_WI_MSK		0x00000200
 #define ALTERA_JTAGUART_CONTROL_AC_MSK		0x00000400
 #define ALTERA_JTAGUART_CONTROL_WSPACE_MSK	0xFFFF0000
-#define ALTERA_JTAGUART_CONTROL_WSPACE_OFF	16
 
 /*
  * Local per-uart structure.
@@ -59,10 +59,19 @@ struct altera_jtaguart {
 	unsigned long imr;	/* Local IMR mirror */
 };
 
+static unsigned int altera_jtaguart_tx_space(struct uart_port *port, u32 *ctlp)
+{
+	u32 ctl = readl(port->membase + ALTERA_JTAGUART_CONTROL_REG);
+
+	if (ctlp)
+		*ctlp = ctl;
+
+	return FIELD_GET(ALTERA_JTAGUART_CONTROL_WSPACE_MSK, ctl);
+}
+
 static unsigned int altera_jtaguart_tx_empty(struct uart_port *port)
 {
-	return (readl(port->membase + ALTERA_JTAGUART_CONTROL_REG) &
-		ALTERA_JTAGUART_CONTROL_WSPACE_MSK) ? TIOCSER_TEMT : 0;
+	return altera_jtaguart_tx_space(port, NULL) ? TIOCSER_TEMT : 0;
 }
 
 static unsigned int altera_jtaguart_get_mctrl(struct uart_port *port)
@@ -150,9 +159,7 @@ static void altera_jtaguart_tx_chars(struct altera_jtaguart *pp)
 
 	pending = uart_circ_chars_pending(xmit);
 	if (pending > 0) {
-		count = (readl(port->membase + ALTERA_JTAGUART_CONTROL_REG) &
-				ALTERA_JTAGUART_CONTROL_WSPACE_MSK) >>
-			ALTERA_JTAGUART_CONTROL_WSPACE_OFF;
+		count = altera_jtaguart_tx_space(port, NULL);
 		if (count > pending)
 			count = pending;
 		if (count > 0) {
@@ -298,12 +305,11 @@ static struct altera_jtaguart altera_jtaguart_ports[ALTERA_JTAGUART_MAXPORTS];
 #if defined(CONFIG_SERIAL_ALTERA_JTAGUART_CONSOLE_BYPASS)
 static void altera_jtaguart_console_putc(struct uart_port *port, unsigned char c)
 {
-	unsigned long status;
 	unsigned long flags;
+	u32 status;
 
 	spin_lock_irqsave(&port->lock, flags);
-	while (((status = readl(port->membase + ALTERA_JTAGUART_CONTROL_REG)) &
-		ALTERA_JTAGUART_CONTROL_WSPACE_MSK) == 0) {
+	while (!altera_jtaguart_tx_space(port, &status)) {
 		if ((status & ALTERA_JTAGUART_CONTROL_AC_MSK) == 0) {
 			spin_unlock_irqrestore(&port->lock, flags);
 			return;	/* no connection activity */
@@ -321,8 +327,7 @@ static void altera_jtaguart_console_putc(struct uart_port *port, unsigned char c
 	unsigned long flags;
 
 	spin_lock_irqsave(&port->lock, flags);
-	while ((readl(port->membase + ALTERA_JTAGUART_CONTROL_REG) &
-		ALTERA_JTAGUART_CONTROL_WSPACE_MSK) == 0) {
+	while (!altera_jtaguart_tx_space(port, NULL)) {
 		spin_unlock_irqrestore(&port->lock, flags);
 		cpu_relax();
 		spin_lock_irqsave(&port->lock, flags);
-- 
2.37.3

[PATCH 4/4] tty: serial: do unlock on a common path in altera_jtaguart_console_putc()

[Jiri Slaby]

port->lock is unlocked in each branch in altera_jtaguart_console_putc(),
so do it before the "if". "status" needs not be under the lock, as the
register was already read.

Cc: Tobias Klauser <tklauser@distanz.ch>
Signed-off-by: Jiri Slaby <jslaby@suse.cz>
---
 drivers/tty/serial/altera_jtaguart.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/altera_jtaguart.c b/drivers/tty/serial/altera_jtaguart.c
index ac8ce418de36..c2d154d78e54 100644
--- a/drivers/tty/serial/altera_jtaguart.c
+++ b/drivers/tty/serial/altera_jtaguart.c
@@ -310,11 +310,12 @@ static void altera_jtaguart_console_putc(struct uart_port *port, unsigned char c
 
 	spin_lock_irqsave(&port->lock, flags);
 	while (!altera_jtaguart_tx_space(port, &status)) {
+		spin_unlock_irqrestore(&port->lock, flags);
+
 		if ((status & ALTERA_JTAGUART_CONTROL_AC_MSK) == 0) {
-			spin_unlock_irqrestore(&port->lock, flags);
 			return;	/* no connection activity */
 		}
-		spin_unlock_irqrestore(&port->lock, flags);
+
 		cpu_relax();
 		spin_lock_irqsave(&port->lock, flags);
 	}
-- 
2.37.3

Re: [PATCH 1/4] tty: serial: extend lqasc_tx_ready() to lqasc_console_putchar()

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1119 bytes --]

On Tue, 27 Sep 2022, Jiri Slaby wrote:

> There is one more place where lqasc_tx_ready() can be used now:
> lqasc_console_putchar(). So replace the open-coded variant by the
> helper.
> 
> Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

> ---
>  drivers/tty/serial/lantiq.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/tty/serial/lantiq.c b/drivers/tty/serial/lantiq.c
> index 6da1b7496c6c..ba9739af30ed 100644
> --- a/drivers/tty/serial/lantiq.c
> +++ b/drivers/tty/serial/lantiq.c
> @@ -606,15 +606,12 @@ static const struct uart_ops lqasc_pops = {
>  static void
>  lqasc_console_putchar(struct uart_port *port, unsigned char ch)
>  {
> -	int fifofree;
> -
>  	if (!port->membase)
>  		return;
>  
> -	do {
> -		fifofree = (__raw_readl(port->membase + LTQ_ASC_FSTAT)
> -			& ASCFSTAT_TXFREEMASK) >> ASCFSTAT_TXFREEOFF;
> -	} while (fifofree == 0);
> +	while (!lqasc_tx_ready(port))
> +		;
> +
>  	writeb(ch, port->membase + LTQ_ASC_TBUF);
>  }
>  
> 

Re: [PATCH 2/4] tty: serial: use FIELD_GET() in lqasc_tx_ready()

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1430 bytes --]

On Tue, 27 Sep 2022, Jiri Slaby wrote:

> FIELD_GET() can do the job smarter and more readable. We don't even need
> ASCFSTAT_TXFREEOFF. So switch to the former and remove the latter.
> 
> Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

> ---
>  drivers/tty/serial/lantiq.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/lantiq.c b/drivers/tty/serial/lantiq.c
> index ba9739af30ed..c892f3c7d1ab 100644
> --- a/drivers/tty/serial/lantiq.c
> +++ b/drivers/tty/serial/lantiq.c
> @@ -8,6 +8,7 @@
>   * Copyright (C) 2010 Thomas Langer, <thomas.langer@lantiq.com>
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/clk.h>
>  #include <linux/console.h>
>  #include <linux/device.h>
> @@ -93,7 +94,6 @@
>  #define ASCFSTAT_RXFFLMASK	0x003F
>  #define ASCFSTAT_TXFFLMASK	0x3F00
>  #define ASCFSTAT_TXFREEMASK	0x3F000000
> -#define ASCFSTAT_TXFREEOFF	24
>  
>  static void lqasc_tx_chars(struct uart_port *port);
>  static struct ltq_uart_port *lqasc_port[MAXPORTS];
> @@ -143,7 +143,7 @@ static bool lqasc_tx_ready(struct uart_port *port)
>  {
>  	u32 fstat = __raw_readl(port->membase + LTQ_ASC_FSTAT);
>  
> -	return (fstat & ASCFSTAT_TXFREEMASK) >> ASCFSTAT_TXFREEOFF;
> +	return FIELD_GET(ASCFSTAT_TXFREEMASK, fstat);
>  }
>  
>  static void
> 

Re: [PATCH 4/4] tty: serial: do unlock on a common path in altera_jtaguart_console_putc()

[Tobias Klauser]

On 2022-09-27 at 13:18:19 +0200, Jiri Slaby <jslaby@suse.cz> wrote:
> port->lock is unlocked in each branch in altera_jtaguart_console_putc(),
> so do it before the "if". "status" needs not be under the lock, as the
> register was already read.
> 
> Cc: Tobias Klauser <tklauser@distanz.ch>
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>

Acked-by: Tobias Klauser <tklauser@distanz.ch>

Thanks

Re: [PATCH 3/4] tty: serial: unify TX space reads under altera_jtaguart_tx_space()

[Tobias Klauser]

On 2022-09-27 at 13:18:18 +0200, Jiri Slaby <jslaby@suse.cz> wrote:
> TX space reads from the control register are performed in various forms
> on 4 places in altera_jtaguart. Unify all those and do the read and
> masking on a single place.
> 
> The new helper altera_jtaguart_tx_space() uses FIELD_GET(), so we can
> drop ALTERA_JTAGUART_CONTROL_WSPACE_OFF now.
> 
> Cc: Tobias Klauser <tklauser@distanz.ch>
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>

Acked-by: Tobias Klauser <tklauser@distanz.ch>

Thanks

Re: [PATCH 3/4] tty: serial: unify TX space reads under altera_jtaguart_tx_space()

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 4103 bytes --]

On Tue, 27 Sep 2022, Jiri Slaby wrote:

> TX space reads from the control register are performed in various forms
> on 4 places in altera_jtaguart. Unify all those and do the read and
> masking on a single place.
> 
> The new helper altera_jtaguart_tx_space() uses FIELD_GET(), so we can
> drop ALTERA_JTAGUART_CONTROL_WSPACE_OFF now.
> 
> Cc: Tobias Klauser <tklauser@distanz.ch>
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> ---
>  drivers/tty/serial/altera_jtaguart.c | 27 ++++++++++++++++-----------
>  1 file changed, 16 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/tty/serial/altera_jtaguart.c b/drivers/tty/serial/altera_jtaguart.c
> index 23f339757894..ac8ce418de36 100644
> --- a/drivers/tty/serial/altera_jtaguart.c
> +++ b/drivers/tty/serial/altera_jtaguart.c
> @@ -9,6 +9,7 @@
>   * (C) Copyright 2010, Tobias Klauser <tklauser@distanz.ch>
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/kernel.h>
>  #include <linux/init.h>
>  #include <linux/interrupt.h>
> @@ -48,7 +49,6 @@
>  #define ALTERA_JTAGUART_CONTROL_WI_MSK		0x00000200
>  #define ALTERA_JTAGUART_CONTROL_AC_MSK		0x00000400
>  #define ALTERA_JTAGUART_CONTROL_WSPACE_MSK	0xFFFF0000
> -#define ALTERA_JTAGUART_CONTROL_WSPACE_OFF	16
>  
>  /*
>   * Local per-uart structure.
> @@ -59,10 +59,19 @@ struct altera_jtaguart {
>  	unsigned long imr;	/* Local IMR mirror */
>  };
>  
> +static unsigned int altera_jtaguart_tx_space(struct uart_port *port, u32 *ctlp)
> +{
> +	u32 ctl = readl(port->membase + ALTERA_JTAGUART_CONTROL_REG);
> +
> +	if (ctlp)
> +		*ctlp = ctl;
> +
> +	return FIELD_GET(ALTERA_JTAGUART_CONTROL_WSPACE_MSK, ctl);
> +}
> +
>  static unsigned int altera_jtaguart_tx_empty(struct uart_port *port)
>  {
> -	return (readl(port->membase + ALTERA_JTAGUART_CONTROL_REG) &
> -		ALTERA_JTAGUART_CONTROL_WSPACE_MSK) ? TIOCSER_TEMT : 0;
> +	return altera_jtaguart_tx_space(port, NULL) ? TIOCSER_TEMT : 0;
>  }
>  
>  static unsigned int altera_jtaguart_get_mctrl(struct uart_port *port)
> @@ -150,9 +159,7 @@ static void altera_jtaguart_tx_chars(struct altera_jtaguart *pp)
>  
>  	pending = uart_circ_chars_pending(xmit);
>  	if (pending > 0) {
> -		count = (readl(port->membase + ALTERA_JTAGUART_CONTROL_REG) &
> -				ALTERA_JTAGUART_CONTROL_WSPACE_MSK) >>
> -			ALTERA_JTAGUART_CONTROL_WSPACE_OFF;
> +		count = altera_jtaguart_tx_space(port, NULL);
>  		if (count > pending)
>  			count = pending;
>  		if (count > 0) {
> @@ -298,12 +305,11 @@ static struct altera_jtaguart altera_jtaguart_ports[ALTERA_JTAGUART_MAXPORTS];
>  #if defined(CONFIG_SERIAL_ALTERA_JTAGUART_CONSOLE_BYPASS)
>  static void altera_jtaguart_console_putc(struct uart_port *port, unsigned char c)
>  {
> -	unsigned long status;
>  	unsigned long flags;
> +	u32 status;
>  
>  	spin_lock_irqsave(&port->lock, flags);
> -	while (((status = readl(port->membase + ALTERA_JTAGUART_CONTROL_REG)) &
> -		ALTERA_JTAGUART_CONTROL_WSPACE_MSK) == 0) {
> +	while (!altera_jtaguart_tx_space(port, &status)) {
>  		if ((status & ALTERA_JTAGUART_CONTROL_AC_MSK) == 0) {
>  			spin_unlock_irqrestore(&port->lock, flags);
>  			return;	/* no connection activity */
> @@ -321,8 +327,7 @@ static void altera_jtaguart_console_putc(struct uart_port *port, unsigned char c
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&port->lock, flags);
> -	while ((readl(port->membase + ALTERA_JTAGUART_CONTROL_REG) &
> -		ALTERA_JTAGUART_CONTROL_WSPACE_MSK) == 0) {
> +	while (!altera_jtaguart_tx_space(port, NULL)) {
>  		spin_unlock_irqrestore(&port->lock, flags);
>  		cpu_relax();
>  		spin_lock_irqsave(&port->lock, flags);

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

With caveats. This is not incorrect per se but I don't particularly like 
that pointer thing done in altera_jtaguart_tx_space(). IMHO, 
altera_jtaguart_console_putc() that needs the control register also for 
other uses than tx space check should read it locally and 
__altera_jtaguart_tx_space() could be added to handle just the 
tx space check without read. But this is of course just my opinion, 
there's no technical issue things done either way.

-- 
 i.

Re: [PATCH 4/4] tty: serial: do unlock on a common path in altera_jtaguart_console_putc()

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1289 bytes --]

On Tue, 27 Sep 2022, Jiri Slaby wrote:

> port->lock is unlocked in each branch in altera_jtaguart_console_putc(),
> so do it before the "if". "status" needs not be under the lock, as the
> register was already read.
> 
> Cc: Tobias Klauser <tklauser@distanz.ch>
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> ---
>  drivers/tty/serial/altera_jtaguart.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/altera_jtaguart.c b/drivers/tty/serial/altera_jtaguart.c
> index ac8ce418de36..c2d154d78e54 100644
> --- a/drivers/tty/serial/altera_jtaguart.c
> +++ b/drivers/tty/serial/altera_jtaguart.c
> @@ -310,11 +310,12 @@ static void altera_jtaguart_console_putc(struct uart_port *port, unsigned char c
>  
>  	spin_lock_irqsave(&port->lock, flags);
>  	while (!altera_jtaguart_tx_space(port, &status)) {
> +		spin_unlock_irqrestore(&port->lock, flags);
> +
>  		if ((status & ALTERA_JTAGUART_CONTROL_AC_MSK) == 0) {
> -			spin_unlock_irqrestore(&port->lock, flags);
>  			return;	/* no connection activity */
>  		}

There braces are now unnecessary.

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.


> -		spin_unlock_irqrestore(&port->lock, flags);
> +
>  		cpu_relax();
>  		spin_lock_irqsave(&port->lock, flags);
>  	}
>