[PATCH 0/6] serial: meson: series with smaller fixes and improvements

[PATCH 0/6] serial: meson: series with smaller fixes and improvements

[Heiner Kallweit]

This series includes a few smaller fixes and improvements for the
meson uart driver.

Heiner Kallweit (6):
  serial: meson: fix setting number of stop bits
  serial: meson: remove dead code in meson_uart_change_speed
  serial: meson: remove unneeded variable assignment in meson_serial_port_write
  serial: meson: make use of uart_port member mapsize
  serial: meson: remove use of flag UPF_IOREMAP
  serial: meson: change interrupt description to of node name

 drivers/tty/serial/meson_uart.c | 52 ++++++++++-------------------------------
 1 file changed, 12 insertions(+), 40 deletions(-)

-- 
2.12.2

[PATCH 1/6] serial: meson: fix setting number of stop bits

[Heiner Kallweit]

The stop bit value as to be or'ed, so far this worked only just by chance
because AML_UART_STOP_BIN_1SB is 0.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/tty/serial/meson_uart.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index 60f16795..e2e25da1 100644
--- a/drivers/tty/serial/meson_uart.c
+++ b/drivers/tty/serial/meson_uart.c
@@ -355,7 +355,7 @@ static void meson_uart_set_termios(struct uart_port *port,
 	if (cflags & CSTOPB)
 		val |= AML_UART_STOP_BIN_2SB;
 	else
-		val &= ~AML_UART_STOP_BIN_1SB;
+		val |= AML_UART_STOP_BIN_1SB;
 
 	if (cflags & CRTSCTS)
 		val &= ~AML_UART_TWO_WIRE_EN;
-- 
2.12.2

[PATCH 2/6] serial: meson: remove dead code in meson_uart_change_speed

[Heiner Kallweit]

val is set in both branches of the if clause, therefore the two
removed lines are dead code.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/tty/serial/meson_uart.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index e2e25da1..22857f1e 100644
--- a/drivers/tty/serial/meson_uart.c
+++ b/drivers/tty/serial/meson_uart.c
@@ -298,8 +298,6 @@ static void meson_uart_change_speed(struct uart_port *port, unsigned long baud)
 	while (!meson_uart_tx_empty(port))
 		cpu_relax();
 
-	val = readl(port->membase + AML_UART_REG5);
-	val &= ~AML_UART_BAUD_MASK;
 	if (port->uartclk == 24000000) {
 		val = ((port->uartclk / 3) / baud) - 1;
 		val |= AML_UART_BAUD_XTAL;
-- 
2.12.2

[PATCH 3/6] serial: meson: remove unneeded variable assignment in meson_serial_port_write

[Heiner Kallweit]

AML_UART_TX_EN is set in meson_uart_startup and there's no place in
the driver where it gets cleared. Therefore we don't have to set it
here.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/tty/serial/meson_uart.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index 22857f1e..714b29ad 100644
--- a/drivers/tty/serial/meson_uart.c
+++ b/drivers/tty/serial/meson_uart.c
@@ -497,7 +497,6 @@ static void meson_serial_port_write(struct uart_port *port, const char *s,
 	}
 
 	val = readl(port->membase + AML_UART_CONTROL);
-	val |= AML_UART_TX_EN;
 	tmp = val & ~(AML_UART_TX_INT_EN | AML_UART_RX_INT_EN);
 	writel(tmp, port->membase + AML_UART_CONTROL);
 
-- 
2.12.2

[PATCH 4/6] serial: meson: make use of uart_port member mapsize

[Heiner Kallweit]

Member mapsize of struct uart_port is meant to store the resource size.
By using it we can get rid of meson_uart_res_size().

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/tty/serial/meson_uart.c | 29 +++++------------------------
 1 file changed, 5 insertions(+), 24 deletions(-)

diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index 714b29ad..8d2e7203 100644
--- a/drivers/tty/serial/meson_uart.c
+++ b/drivers/tty/serial/meson_uart.c
@@ -393,26 +393,11 @@ static int meson_uart_verify_port(struct uart_port *port,
 	return ret;
 }
 
-static int meson_uart_res_size(struct uart_port *port)
-{
-	struct platform_device *pdev = to_platform_device(port->dev);
-	struct resource *res;
-
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!res) {
-		dev_err(port->dev, "cannot obtain I/O memory region");
-		return -ENODEV;
-	}
-
-	return resource_size(res);
-}
-
 static void meson_uart_release_port(struct uart_port *port)
 {
-	int size = meson_uart_res_size(port);
-
 	if (port->flags & UPF_IOREMAP) {
-		devm_release_mem_region(port->dev, port->mapbase, size);
+		devm_release_mem_region(port->dev, port->mapbase,
+					port->mapsize);
 		devm_iounmap(port->dev, port->membase);
 		port->membase = NULL;
 	}
@@ -420,12 +405,7 @@ static void meson_uart_release_port(struct uart_port *port)
 
 static int meson_uart_request_port(struct uart_port *port)
 {
-	int size = meson_uart_res_size(port);
-
-	if (size < 0)
-		return size;
-
-	if (!devm_request_mem_region(port->dev, port->mapbase, size,
+	if (!devm_request_mem_region(port->dev, port->mapbase, port->mapsize,
 				     dev_name(port->dev))) {
 		dev_err(port->dev, "Memory region busy\n");
 		return -EBUSY;
@@ -434,7 +414,7 @@ static int meson_uart_request_port(struct uart_port *port)
 	if (port->flags & UPF_IOREMAP) {
 		port->membase = devm_ioremap_nocache(port->dev,
 						     port->mapbase,
-						     size);
+						     port->mapsize);
 		if (port->membase == NULL)
 			return -ENOMEM;
 	}
@@ -629,6 +609,7 @@ static int meson_uart_probe(struct platform_device *pdev)
 	port->uartclk = clk_get_rate(clk);
 	port->iotype = UPIO_MEM;
 	port->mapbase = res_mem->start;
+	port->mapsize = resource_size(res_mem);
 	port->irq = res_irq->start;
 	port->flags = UPF_BOOT_AUTOCONF | UPF_IOREMAP | UPF_LOW_LATENCY;
 	port->dev = &pdev->dev;
-- 
2.12.2

[PATCH 5/6] serial: meson: remove use of flag UPF_IOREMAP

[Heiner Kallweit]

Flag UPF_IOREMAP is used by the 8250 subsystem only, it's not used
by the serial core. Therefore I don't see any benefit in using it
here.

In addition fix the order of calls in meson_uart_release_port.
Unmapping needs to be done first, reversing call order in
meson_uart_request_port.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/tty/serial/meson_uart.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index 8d2e7203..c8b626e3 100644
--- a/drivers/tty/serial/meson_uart.c
+++ b/drivers/tty/serial/meson_uart.c
@@ -395,12 +395,9 @@ static int meson_uart_verify_port(struct uart_port *port,
 
 static void meson_uart_release_port(struct uart_port *port)
 {
-	if (port->flags & UPF_IOREMAP) {
-		devm_release_mem_region(port->dev, port->mapbase,
-					port->mapsize);
-		devm_iounmap(port->dev, port->membase);
-		port->membase = NULL;
-	}
+	devm_iounmap(port->dev, port->membase);
+	port->membase = NULL;
+	devm_release_mem_region(port->dev, port->mapbase, port->mapsize);
 }
 
 static int meson_uart_request_port(struct uart_port *port)
@@ -411,13 +408,10 @@ static int meson_uart_request_port(struct uart_port *port)
 		return -EBUSY;
 	}
 
-	if (port->flags & UPF_IOREMAP) {
-		port->membase = devm_ioremap_nocache(port->dev,
-						     port->mapbase,
-						     port->mapsize);
-		if (port->membase == NULL)
-			return -ENOMEM;
-	}
+	port->membase = devm_ioremap_nocache(port->dev, port->mapbase,
+					     port->mapsize);
+	if (!port->membase)
+		return -ENOMEM;
 
 	return 0;
 }
@@ -611,7 +605,7 @@ static int meson_uart_probe(struct platform_device *pdev)
 	port->mapbase = res_mem->start;
 	port->mapsize = resource_size(res_mem);
 	port->irq = res_irq->start;
-	port->flags = UPF_BOOT_AUTOCONF | UPF_IOREMAP | UPF_LOW_LATENCY;
+	port->flags = UPF_BOOT_AUTOCONF | UPF_LOW_LATENCY;
 	port->dev = &pdev->dev;
 	port->line = pdev->id;
 	port->type = PORT_MESON;
-- 
2.12.2

[PATCH 6/6] serial: meson: change interrupt description to of node name

[Heiner Kallweit]

Change interrupt description from driver name to of node name.
If multiple serial ports are enabled this allows to determine
which interrupt belongs to which port in /proc/interrupts.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/tty/serial/meson_uart.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index c8b626e3..d0682231 100644
--- a/drivers/tty/serial/meson_uart.c
+++ b/drivers/tty/serial/meson_uart.c
@@ -286,7 +286,7 @@ static int meson_uart_startup(struct uart_port *port)
 	writel(val, port->membase + AML_UART_MISC);
 
 	ret = request_irq(port->irq, meson_uart_interrupt, 0,
-			  meson_uart_type(port), port);
+			  dev_name(port->dev), port);
 
 	return ret;
 }
-- 
2.12.2

[PATCH 6/6] serial: meson: change interrupt description to of node name

[Ben Dooks]

On 16/04/17 21:23, Heiner Kallweit wrote:
> Change interrupt description from driver name to of node name.
> If multiple serial ports are enabled this allows to determine
> which interrupt belongs to which port in /proc/interrupts.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/tty/serial/meson_uart.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
> index c8b626e3..d0682231 100644
> --- a/drivers/tty/serial/meson_uart.c
> +++ b/drivers/tty/serial/meson_uart.c
> @@ -286,7 +286,7 @@ static int meson_uart_startup(struct uart_port *port)
>  	writel(val, port->membase + AML_UART_MISC);
>
>  	ret = request_irq(port->irq, meson_uart_interrupt, 0,
> -			  meson_uart_type(port), port);
> +			  dev_name(port->dev), port);
>
>  	return ret;
>  }

Good idea.


-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

[PATCH 4/6] serial: meson: make use of uart_port member mapsize

[Ben Dooks]

On 16/04/17 21:15, Heiner Kallweit wrote:
> Member mapsize of struct uart_port is meant to store the resource size.
> By using it we can get rid of meson_uart_res_size().
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/tty/serial/meson_uart.c | 29 +++++------------------------
>  1 file changed, 5 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
> index 714b29ad..8d2e7203 100644
> --- a/drivers/tty/serial/meson_uart.c
> +++ b/drivers/tty/serial/meson_uart.c
> @@ -393,26 +393,11 @@ static int meson_uart_verify_port(struct uart_port *port,
>  	return ret;
>  }
>
> -static int meson_uart_res_size(struct uart_port *port)
> -{
> -	struct platform_device *pdev = to_platform_device(port->dev);
> -	struct resource *res;
> -
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (!res) {
> -		dev_err(port->dev, "cannot obtain I/O memory region");
> -		return -ENODEV;
> -	}
> -
> -	return resource_size(res);
> -}
> -
>  static void meson_uart_release_port(struct uart_port *port)
>  {
> -	int size = meson_uart_res_size(port);
> -
>  	if (port->flags & UPF_IOREMAP) {
> -		devm_release_mem_region(port->dev, port->mapbase, size);
> +		devm_release_mem_region(port->dev, port->mapbase,
> +					port->mapsize);
>  		devm_iounmap(port->dev, port->membase);
>  		port->membase = NULL;
>  	}
> @@ -420,12 +405,7 @@ static void meson_uart_release_port(struct uart_port *port)
>
>  static int meson_uart_request_port(struct uart_port *port)
>  {
> -	int size = meson_uart_res_size(port);
> -
> -	if (size < 0)
> -		return size;
> -
> -	if (!devm_request_mem_region(port->dev, port->mapbase, size,
> +	if (!devm_request_mem_region(port->dev, port->mapbase, port->mapsize,
>  				     dev_name(port->dev))) {
>  		dev_err(port->dev, "Memory region busy\n");
>  		return -EBUSY;
> @@ -434,7 +414,7 @@ static int meson_uart_request_port(struct uart_port *port)
>  	if (port->flags & UPF_IOREMAP) {
>  		port->membase = devm_ioremap_nocache(port->dev,
>  						     port->mapbase,
> -						     size);
> +						     port->mapsize);
>  		if (port->membase == NULL)
>  			return -ENOMEM;
>  	}
> @@ -629,6 +609,7 @@ static int meson_uart_probe(struct platform_device *pdev)
>  	port->uartclk = clk_get_rate(clk);
>  	port->iotype = UPIO_MEM;
>  	port->mapbase = res_mem->start;
> +	port->mapsize = resource_size(res_mem);
>  	port->irq = res_irq->start;
>  	port->flags = UPF_BOOT_AUTOCONF | UPF_IOREMAP | UPF_LOW_LATENCY;
>  	port->dev = &pdev->dev;

Looks good.


-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

[PATCH 3/6] serial: meson: remove unneeded variable assignment in meson_serial_port_write

[Ben Dooks]

On 16/04/17 21:13, Heiner Kallweit wrote:
> AML_UART_TX_EN is set in meson_uart_startup and there's no place in
> the driver where it gets cleared. Therefore we don't have to set it
> here.

I think this is a leftover of the patches I did when trying to fix
the UART driver when using debian/systemd and having strange console
problems.

The only comment is it might be worth leaving this in as it isn't
a high cost to the driver and it is possible someone might accidentally
clear it later.

> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/tty/serial/meson_uart.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
> index 22857f1e..714b29ad 100644
> --- a/drivers/tty/serial/meson_uart.c
> +++ b/drivers/tty/serial/meson_uart.c
> @@ -497,7 +497,6 @@ static void meson_serial_port_write(struct uart_port *port, const char *s,
>  	}
>
>  	val = readl(port->membase + AML_UART_CONTROL);
> -	val |= AML_UART_TX_EN;
>  	tmp = val & ~(AML_UART_TX_INT_EN | AML_UART_RX_INT_EN);
>  	writel(tmp, port->membase + AML_UART_CONTROL);
>
>


-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

[PATCH 1/6] serial: meson: fix setting number of stop bits

[Neil Armstrong]

On 04/16/2017 10:06 PM, Heiner Kallweit wrote:
> The stop bit value as to be or'ed, so far this worked only just by chance
> because AML_UART_STOP_BIN_1SB is 0.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/tty/serial/meson_uart.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
> index 60f16795..e2e25da1 100644
> --- a/drivers/tty/serial/meson_uart.c
> +++ b/drivers/tty/serial/meson_uart.c
> @@ -355,7 +355,7 @@ static void meson_uart_set_termios(struct uart_port *port,
>  	if (cflags & CSTOPB)
>  		val |= AML_UART_STOP_BIN_2SB;
>  	else
> -		val &= ~AML_UART_STOP_BIN_1SB;
> +		val |= AML_UART_STOP_BIN_1SB;
>  
>  	if (cflags & CRTSCTS)
>  		val &= ~AML_UART_TWO_WIRE_EN;
> 

Hi,

in fact you can totally drop the else statement since it's a no-op in both cases.

Neil

[PATCH 2/6] serial: meson: remove dead code in meson_uart_change_speed

[Neil Armstrong]

On 04/16/2017 10:10 PM, Heiner Kallweit wrote:
> val is set in both branches of the if clause, therefore the two
> removed lines are dead code.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/tty/serial/meson_uart.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
> index e2e25da1..22857f1e 100644
> --- a/drivers/tty/serial/meson_uart.c
> +++ b/drivers/tty/serial/meson_uart.c
> @@ -298,8 +298,6 @@ static void meson_uart_change_speed(struct uart_port *port, unsigned long baud)
>  	while (!meson_uart_tx_empty(port))
>  		cpu_relax();
>  
> -	val = readl(port->membase + AML_UART_REG5);
> -	val &= ~AML_UART_BAUD_MASK;
>  	if (port->uartclk == 24000000) {
>  		val = ((port->uartclk / 3) / baud) - 1;
>  		val |= AML_UART_BAUD_XTAL;
> 


Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>

[PATCH 3/6] serial: meson: remove unneeded variable assignment in meson_serial_port_write

[Neil Armstrong]

On 04/17/2017 03:58 PM, Ben Dooks wrote:
> On 16/04/17 21:13, Heiner Kallweit wrote:
>> AML_UART_TX_EN is set in meson_uart_startup and there's no place in
>> the driver where it gets cleared. Therefore we don't have to set it
>> here.
> 
> I think this is a leftover of the patches I did when trying to fix
> the UART driver when using debian/systemd and having strange console
> problems.
> 
> The only comment is it might be worth leaving this in as it isn't
> a high cost to the driver and it is possible someone might accidentally
> clear it later.
> 
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/tty/serial/meson_uart.c | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
>> index 22857f1e..714b29ad 100644
>> --- a/drivers/tty/serial/meson_uart.c
>> +++ b/drivers/tty/serial/meson_uart.c
>> @@ -497,7 +497,6 @@ static void meson_serial_port_write(struct uart_port *port, const char *s,
>>      }
>>
>>      val = readl(port->membase + AML_UART_CONTROL);
>> -    val |= AML_UART_TX_EN;
>>      tmp = val & ~(AML_UART_TX_INT_EN | AML_UART_RX_INT_EN);
>>      writel(tmp, port->membase + AML_UART_CONTROL);
>>
>>
> 
> 

I'm ok to remove it, having it in uart_startup is enough.

Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>

[PATCH 4/6] serial: meson: make use of uart_port member mapsize

[Neil Armstrong]

On 04/16/2017 10:15 PM, Heiner Kallweit wrote:
> Member mapsize of struct uart_port is meant to store the resource size.
> By using it we can get rid of meson_uart_res_size().
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/tty/serial/meson_uart.c | 29 +++++------------------------
>  1 file changed, 5 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
> index 714b29ad..8d2e7203 100644
> --- a/drivers/tty/serial/meson_uart.c
> +++ b/drivers/tty/serial/meson_uart.c
> @@ -393,26 +393,11 @@ static int meson_uart_verify_port(struct uart_port *port,
>  	return ret;
>  }
>  
> -static int meson_uart_res_size(struct uart_port *port)
> -{
> -	struct platform_device *pdev = to_platform_device(port->dev);
> -	struct resource *res;
> -
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (!res) {
> -		dev_err(port->dev, "cannot obtain I/O memory region");
> -		return -ENODEV;
> -	}
> -
> -	return resource_size(res);
> -}
> -
>  static void meson_uart_release_port(struct uart_port *port)
>  {
> -	int size = meson_uart_res_size(port);
> -
>  	if (port->flags & UPF_IOREMAP) {
> -		devm_release_mem_region(port->dev, port->mapbase, size);
> +		devm_release_mem_region(port->dev, port->mapbase,
> +					port->mapsize);
>  		devm_iounmap(port->dev, port->membase);
>  		port->membase = NULL;
>  	}
> @@ -420,12 +405,7 @@ static void meson_uart_release_port(struct uart_port *port)
>  
>  static int meson_uart_request_port(struct uart_port *port)
>  {
> -	int size = meson_uart_res_size(port);
> -
> -	if (size < 0)
> -		return size;
> -
> -	if (!devm_request_mem_region(port->dev, port->mapbase, size,
> +	if (!devm_request_mem_region(port->dev, port->mapbase, port->mapsize,
>  				     dev_name(port->dev))) {
>  		dev_err(port->dev, "Memory region busy\n");
>  		return -EBUSY;
> @@ -434,7 +414,7 @@ static int meson_uart_request_port(struct uart_port *port)
>  	if (port->flags & UPF_IOREMAP) {
>  		port->membase = devm_ioremap_nocache(port->dev,
>  						     port->mapbase,
> -						     size);
> +						     port->mapsize);
>  		if (port->membase == NULL)
>  			return -ENOMEM;
>  	}
> @@ -629,6 +609,7 @@ static int meson_uart_probe(struct platform_device *pdev)
>  	port->uartclk = clk_get_rate(clk);
>  	port->iotype = UPIO_MEM;
>  	port->mapbase = res_mem->start;
> +	port->mapsize = resource_size(res_mem);
>  	port->irq = res_irq->start;
>  	port->flags = UPF_BOOT_AUTOCONF | UPF_IOREMAP | UPF_LOW_LATENCY;
>  	port->dev = &pdev->dev;
> 

Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>

[PATCH 5/6] serial: meson: remove use of flag UPF_IOREMAP

[Neil Armstrong]

On 04/16/2017 10:21 PM, Heiner Kallweit wrote:
> Flag UPF_IOREMAP is used by the 8250 subsystem only, it's not used
> by the serial core. Therefore I don't see any benefit in using it
> here.
> 
> In addition fix the order of calls in meson_uart_release_port.
> Unmapping needs to be done first, reversing call order in
> meson_uart_request_port.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/tty/serial/meson_uart.c | 22 ++++++++--------------
>  1 file changed, 8 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
> index 8d2e7203..c8b626e3 100644
> --- a/drivers/tty/serial/meson_uart.c
> +++ b/drivers/tty/serial/meson_uart.c
> @@ -395,12 +395,9 @@ static int meson_uart_verify_port(struct uart_port *port,
>  
>  static void meson_uart_release_port(struct uart_port *port)
>  {
> -	if (port->flags & UPF_IOREMAP) {
> -		devm_release_mem_region(port->dev, port->mapbase,
> -					port->mapsize);
> -		devm_iounmap(port->dev, port->membase);
> -		port->membase = NULL;
> -	}
> +	devm_iounmap(port->dev, port->membase);
> +	port->membase = NULL;
> +	devm_release_mem_region(port->dev, port->mapbase, port->mapsize);
>  }
>  
>  static int meson_uart_request_port(struct uart_port *port)
> @@ -411,13 +408,10 @@ static int meson_uart_request_port(struct uart_port *port)
>  		return -EBUSY;
>  	}
>  
> -	if (port->flags & UPF_IOREMAP) {
> -		port->membase = devm_ioremap_nocache(port->dev,
> -						     port->mapbase,
> -						     port->mapsize);
> -		if (port->membase == NULL)
> -			return -ENOMEM;
> -	}
> +	port->membase = devm_ioremap_nocache(port->dev, port->mapbase,
> +					     port->mapsize);
> +	if (!port->membase)
> +		return -ENOMEM;
>  
>  	return 0;
>  }
> @@ -611,7 +605,7 @@ static int meson_uart_probe(struct platform_device *pdev)
>  	port->mapbase = res_mem->start;
>  	port->mapsize = resource_size(res_mem);
>  	port->irq = res_irq->start;
> -	port->flags = UPF_BOOT_AUTOCONF | UPF_IOREMAP | UPF_LOW_LATENCY;
> +	port->flags = UPF_BOOT_AUTOCONF | UPF_LOW_LATENCY;
>  	port->dev = &pdev->dev;
>  	port->line = pdev->id;
>  	port->type = PORT_MESON;
> 

Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>

[PATCH 6/6] serial: meson: change interrupt description to of node name

[Neil Armstrong]

On 04/16/2017 10:23 PM, Heiner Kallweit wrote:
> Change interrupt description from driver name to of node name.
> If multiple serial ports are enabled this allows to determine
> which interrupt belongs to which port in /proc/interrupts.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/tty/serial/meson_uart.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
> index c8b626e3..d0682231 100644
> --- a/drivers/tty/serial/meson_uart.c
> +++ b/drivers/tty/serial/meson_uart.c
> @@ -286,7 +286,7 @@ static int meson_uart_startup(struct uart_port *port)
>  	writel(val, port->membase + AML_UART_MISC);
>  
>  	ret = request_irq(port->irq, meson_uart_interrupt, 0,
> -			  meson_uart_type(port), port);
> +			  dev_name(port->dev), port);
>  
>  	return ret;
>  }
> 

Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>

[PATCH 3/6] serial: meson: remove unneeded variable assignment in meson_serial_port_write

[Andreas Färber]

Am 17.04.2017 um 17:27 schrieb Neil Armstrong:
> On 04/17/2017 03:58 PM, Ben Dooks wrote:
>> On 16/04/17 21:13, Heiner Kallweit wrote:
>>> AML_UART_TX_EN is set in meson_uart_startup and there's no place in
>>> the driver where it gets cleared. Therefore we don't have to set it
>>> here.
>>
>> I think this is a leftover of the patches I did when trying to fix
>> the UART driver when using debian/systemd and having strange console
>> problems.
>>
>> The only comment is it might be worth leaving this in as it isn't
>> a high cost to the driver and it is possible someone might accidentally
>> clear it later.
>>
>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>> ---
>>>  drivers/tty/serial/meson_uart.c | 1 -
>>>  1 file changed, 1 deletion(-)
>>>
>>> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
>>> index 22857f1e..714b29ad 100644
>>> --- a/drivers/tty/serial/meson_uart.c
>>> +++ b/drivers/tty/serial/meson_uart.c
>>> @@ -497,7 +497,6 @@ static void meson_serial_port_write(struct uart_port *port, const char *s,
>>>      }
>>>
>>>      val = readl(port->membase + AML_UART_CONTROL);
>>> -    val |= AML_UART_TX_EN;
>>>      tmp = val & ~(AML_UART_TX_INT_EN | AML_UART_RX_INT_EN);
>>>      writel(tmp, port->membase + AML_UART_CONTROL);
>>>
>>>
>>
>>
> 
> I'm ok to remove it, having it in uart_startup is enough.
> 
> Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>

Have you checked the earlycon code paths? That does not run through the
standard setup function, so it is dependent on what the firmware set
last. I believe I am calling into this function through some wrapper.

Since we're writing the register anyway, it shouldn't hurt to leave this in.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 N?rnberg, Germany
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)

[PATCH 1/6] serial: meson: fix setting number of stop bits

[Heiner Kallweit]

Am 17.04.2017 um 17:24 schrieb Neil Armstrong:
> On 04/16/2017 10:06 PM, Heiner Kallweit wrote:
>> The stop bit value as to be or'ed, so far this worked only just by chance
>> because AML_UART_STOP_BIN_1SB is 0.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/tty/serial/meson_uart.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
>> index 60f16795..e2e25da1 100644
>> --- a/drivers/tty/serial/meson_uart.c
>> +++ b/drivers/tty/serial/meson_uart.c
>> @@ -355,7 +355,7 @@ static void meson_uart_set_termios(struct uart_port *port,
>>  	if (cflags & CSTOPB)
>>  		val |= AML_UART_STOP_BIN_2SB;
>>  	else
>> -		val &= ~AML_UART_STOP_BIN_1SB;
>> +		val |= AML_UART_STOP_BIN_1SB;
>>  
>>  	if (cflags & CRTSCTS)
>>  		val &= ~AML_UART_TWO_WIRE_EN;
>>
> 
> Hi,
> 
> in fact you can totally drop the else statement since it's a no-op in both cases.
> 
That's right. However I think leaving this in makes the code better understandable.
And most likely the compiler will remove this no-op anyway.

Heiner

> Neil
> 

[PATCH 2/6] serial: meson: remove dead code in meson_uart_change_speed

[Andreas Färber]

Am 17.04.2017 um 17:25 schrieb Neil Armstrong:
> On 04/16/2017 10:10 PM, Heiner Kallweit wrote:
>> val is set in both branches of the if clause, therefore the two
>> removed lines are dead code.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/tty/serial/meson_uart.c | 2 --
>>  1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
>> index e2e25da1..22857f1e 100644
>> --- a/drivers/tty/serial/meson_uart.c
>> +++ b/drivers/tty/serial/meson_uart.c
>> @@ -298,8 +298,6 @@ static void meson_uart_change_speed(struct uart_port *port, unsigned long baud)
>>  	while (!meson_uart_tx_empty(port))
>>  		cpu_relax();
>>  
>> -	val = readl(port->membase + AML_UART_REG5);
>> -	val &= ~AML_UART_BAUD_MASK;
>>  	if (port->uartclk == 24000000) {
>>  		val = ((port->uartclk / 3) / baud) - 1;
>>  		val |= AML_UART_BAUD_XTAL;
>>
> 
> 
> Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>

I've double-checked that this was already done before I added the
GXBB-enabling XTAL case:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/tty/serial/meson_uart.c?id=146f3808e08faabba46ea9574133a66aa4a9468d

Present since the beginning:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/tty/serial/meson_uart.c?id=ff7693d079e58fb62d735b7b8085b53fcfb74528

In general it is worth pointing out that reading a register may
sometimes be required - I hope that BayLibre have checked against the
manuals that this is not the case here? Assuming it is okay,

Reviewed-by: Andreas F?rber <afaerber@suse.de>

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 N?rnberg, Germany
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)

[PATCH 3/6] serial: meson: remove unneeded variable assignment in meson_serial_port_write

[Heiner Kallweit]

Am 17.04.2017 um 17:37 schrieb Andreas F?rber:
> Am 17.04.2017 um 17:27 schrieb Neil Armstrong:
>> On 04/17/2017 03:58 PM, Ben Dooks wrote:
>>> On 16/04/17 21:13, Heiner Kallweit wrote:
>>>> AML_UART_TX_EN is set in meson_uart_startup and there's no place in
>>>> the driver where it gets cleared. Therefore we don't have to set it
>>>> here.
>>>
>>> I think this is a leftover of the patches I did when trying to fix
>>> the UART driver when using debian/systemd and having strange console
>>> problems.
>>>
>>> The only comment is it might be worth leaving this in as it isn't
>>> a high cost to the driver and it is possible someone might accidentally
>>> clear it later.
>>>
>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>>> ---
>>>>  drivers/tty/serial/meson_uart.c | 1 -
>>>>  1 file changed, 1 deletion(-)
>>>>
>>>> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
>>>> index 22857f1e..714b29ad 100644
>>>> --- a/drivers/tty/serial/meson_uart.c
>>>> +++ b/drivers/tty/serial/meson_uart.c
>>>> @@ -497,7 +497,6 @@ static void meson_serial_port_write(struct uart_port *port, const char *s,
>>>>      }
>>>>
>>>>      val = readl(port->membase + AML_UART_CONTROL);
>>>> -    val |= AML_UART_TX_EN;
>>>>      tmp = val & ~(AML_UART_TX_INT_EN | AML_UART_RX_INT_EN);
>>>>      writel(tmp, port->membase + AML_UART_CONTROL);
>>>>
>>>>
>>>
>>>
>>
>> I'm ok to remove it, having it in uart_startup is enough.
>>
>> Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
> 
> Have you checked the earlycon code paths? That does not run through the
> standard setup function, so it is dependent on what the firmware set
> last. I believe I am calling into this function through some wrapper.
> 
> Since we're writing the register anyway, it shouldn't hurt to leave this in.
> 
> Regards,
> Andreas
> 
Provided that everybody is fine with the other patches of this series:
Can you simply omit patch 3 when applying the series or would you prefer
sending an updated series w/o patch 3?

Heiner

[PATCH 3/6] serial: meson: remove unneeded variable assignment in meson_serial_port_write

[Neil Armstrong]

On 04/17/2017 05:37 PM, Andreas F?rber wrote:
> Am 17.04.2017 um 17:27 schrieb Neil Armstrong:
>> On 04/17/2017 03:58 PM, Ben Dooks wrote:
>>> On 16/04/17 21:13, Heiner Kallweit wrote:
>>>> AML_UART_TX_EN is set in meson_uart_startup and there's no place in
>>>> the driver where it gets cleared. Therefore we don't have to set it
>>>> here.
>>>
>>> I think this is a leftover of the patches I did when trying to fix
>>> the UART driver when using debian/systemd and having strange console
>>> problems.
>>>
>>> The only comment is it might be worth leaving this in as it isn't
>>> a high cost to the driver and it is possible someone might accidentally
>>> clear it later.
>>>
>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>>> ---
>>>>  drivers/tty/serial/meson_uart.c | 1 -
>>>>  1 file changed, 1 deletion(-)
>>>>
>>>> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
>>>> index 22857f1e..714b29ad 100644
>>>> --- a/drivers/tty/serial/meson_uart.c
>>>> +++ b/drivers/tty/serial/meson_uart.c
>>>> @@ -497,7 +497,6 @@ static void meson_serial_port_write(struct uart_port *port, const char *s,
>>>>      }
>>>>
>>>>      val = readl(port->membase + AML_UART_CONTROL);
>>>> -    val |= AML_UART_TX_EN;
>>>>      tmp = val & ~(AML_UART_TX_INT_EN | AML_UART_RX_INT_EN);
>>>>      writel(tmp, port->membase + AML_UART_CONTROL);
>>>>
>>>>
>>>
>>>
>>
>> I'm ok to remove it, having it in uart_startup is enough.
>>
>> Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
> 
> Have you checked the earlycon code paths? That does not run through the
> standard setup function, so it is dependent on what the firmware set
> last. I believe I am calling into this function through some wrapper.
> 
> Since we're writing the register anyway, it shouldn't hurt to leave this in.
> 
> Regards,
> Andreas
> 

Indeed, it's on the earlycon path, but it's a hack and should be moved inside meson_serial_early_console_setup() and meson_serial_console_setup() instead.

Writing TX_EN at each meson_serial_port_write() is really unnecessary...

Neil

[PATCH 1/6] serial: meson: fix setting number of stop bits

[Kevin Hilman]

Heiner Kallweit <hkallweit1@gmail.com> writes:

> Am 17.04.2017 um 17:24 schrieb Neil Armstrong:
>> On 04/16/2017 10:06 PM, Heiner Kallweit wrote:
>>> The stop bit value as to be or'ed, so far this worked only just by chance
>>> because AML_UART_STOP_BIN_1SB is 0.
>>>
>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>> ---
>>>  drivers/tty/serial/meson_uart.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
>>> index 60f16795..e2e25da1 100644
>>> --- a/drivers/tty/serial/meson_uart.c
>>> +++ b/drivers/tty/serial/meson_uart.c
>>> @@ -355,7 +355,7 @@ static void meson_uart_set_termios(struct uart_port *port,
>>>  	if (cflags & CSTOPB)
>>>  		val |= AML_UART_STOP_BIN_2SB;
>>>  	else
>>> -		val &= ~AML_UART_STOP_BIN_1SB;
>>> +		val |= AML_UART_STOP_BIN_1SB;
>>>  
>>>  	if (cflags & CRTSCTS)
>>>  		val &= ~AML_UART_TWO_WIRE_EN;
>>>
>> 
>> Hi,
>> 
>> in fact you can totally drop the else statement since it's a no-op in both cases.
>> 
> That's right. However I think leaving this in makes the code better understandable.
> And most likely the compiler will remove this no-op anyway.

Agreed.  I like it for readability.

Kevin