Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 28 Sep 2022 13:26:56 +0300 (EEST)
From:   =?ISO-8859-15?Q?Ilpo_J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>
To:     Jiri Slaby <jslaby@suse.cz>
cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-serial <linux-serial@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Tobias Klauser <tklauser@distanz.ch>
Subject: Re: [PATCH 3/4] tty: serial: unify TX space reads under
 altera_jtaguart_tx_space()
In-Reply-To: <20220927111819.18516-3-jslaby@suse.cz>
Message-ID: <bcb0b523-3df0-82e0-51d2-ada0959134e@linux.intel.com>
References: <20220927111819.18516-1-jslaby@suse.cz> <20220927111819.18516-3-jslaby@suse.cz>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="8323329-337446478-1664360823=:1695"
Precedence: bulk

  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--8323329-337446478-1664360823=:1695
Content-Type: text/plain; charset=ISO-8859-15
Content-Transfer-Encoding: 8BIT

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.

--8323329-337446478-1664360823=:1695--