[PATCH 1/2] serial/aspeed-vuart: Only disable host tx discard when serial port is in use

[PATCH 1/2] serial/aspeed-vuart: Only disable host tx discard when serial port is in use

[Jeremy Kerr]

OpenPOWER firmware doesn't like it when the host-side of the VUART's
FIFO is not drained. This change shifts only disables host TX discard
mode when the port has been opened.

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
---
 drivers/tty/serial/aspeed-vuart.c | 40 +++++++++++++++++++++++++++++++++++----
 1 file changed, 36 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/aspeed-vuart.c b/drivers/tty/serial/aspeed-vuart.c
index f34884a..73bb0e7 100644
--- a/drivers/tty/serial/aspeed-vuart.c
+++ b/drivers/tty/serial/aspeed-vuart.c
@@ -144,16 +144,45 @@ static ssize_t ast_vuart_set_sirq(struct device *dev,
 static DEVICE_ATTR(sirq, S_IWUSR | S_IRUGO,
 		ast_vuart_show_sirq, ast_vuart_set_sirq);
 
-static void ast_vuart_setup(struct ast_vuart *vuart)
+static void ast_vuart_set_host_tx_discard(struct ast_vuart *vuart, bool discard)
 {
 	u8 reg;
 
-	/* disable TX discard mode */
 	reg = readb(vuart->regs + AST_VUART_GCRA);
-	reg |= AST_VUART_GCRA_HOST_TX_DISCARD;
+
+	/* if the HOST_TX_DISCARD bit is set, discard is *disabled* */
+	reg &= ~AST_VUART_GCRA_HOST_TX_DISCARD;
+	if (!discard)
+		reg |= AST_VUART_GCRA_HOST_TX_DISCARD;
+
 	writeb(reg, vuart->regs + AST_VUART_GCRA);
 }
 
+static int ast_vuart_startup(struct uart_port *uart_port)
+{
+	struct uart_8250_port *uart_8250_port = up_to_u8250p(uart_port);
+	struct ast_vuart *vuart = uart_8250_port->port.private_data;
+	int rc;
+
+	rc = serial8250_do_startup(uart_port);
+	if (rc)
+		return rc;
+
+	ast_vuart_set_host_tx_discard(vuart, false);
+
+	return 0;
+}
+
+static void ast_vuart_shutdown(struct uart_port *uart_port)
+{
+	struct uart_8250_port *uart_8250_port = up_to_u8250p(uart_port);
+	struct ast_vuart *vuart = uart_8250_port->port.private_data;
+
+	ast_vuart_set_host_tx_discard(vuart, true);
+
+	serial8250_do_shutdown(uart_port);
+}
+
 
 /**
  * The device tree parsing code here is heavily based on that of the of_serial
@@ -190,9 +219,12 @@ static int ast_vuart_probe(struct platform_device *pdev)
 	}
 
 	memset(&port, 0, sizeof(port));
+	port.port.private_data = vuart;
 	port.port.membase = vuart->regs;
 	port.port.mapbase = resource.start;
 	port.port.mapsize = resource_size(&resource);
+	port.port.startup = ast_vuart_startup;
+	port.port.shutdown = ast_vuart_shutdown;
 
 	if (of_property_read_u32(np, "clock-frequency", &clk)) {
 		vuart->clk = devm_clk_get(&pdev->dev, NULL);
@@ -272,8 +304,8 @@ static int ast_vuart_probe(struct platform_device *pdev)
 
 
 	vuart->line = rc;
+	ast_vuart_set_host_tx_discard(vuart, true);
 	platform_set_drvdata(pdev, vuart);
-	ast_vuart_setup(vuart);
 
 	/* extra sysfs control */
 	rc = device_create_file(&pdev->dev, &dev_attr_lpc_address);
-- 
2.5.0

[RFC PATCH 2/2] serial/aspeed-vuart: Perform enable/disable on driver bind/unbind

[Jeremy Kerr]

Rather than exposing an enable sysfs attribute, we can just set the
VUART enabled bit when we bind to the device, and clear it on unbind.

We don't want to do this on open/release, as the host may be using this
bit to configure serial output modes, which is independent of whether
the devices has been opened by BMC userspace.

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
---
 drivers/tty/serial/aspeed-vuart.c | 53 +++++++++++----------------------------
 1 file changed, 14 insertions(+), 39 deletions(-)

diff --git a/drivers/tty/serial/aspeed-vuart.c b/drivers/tty/serial/aspeed-vuart.c
index 73bb0e7..020c815 100644
--- a/drivers/tty/serial/aspeed-vuart.c
+++ b/drivers/tty/serial/aspeed-vuart.c
@@ -35,42 +35,6 @@ struct ast_vuart {
 	int			line;
 };
 
-static ssize_t ast_vuart_show_enabled(struct device *dev,
-		struct device_attribute *attr, char *buf)
-{
-	struct ast_vuart *vuart = dev_get_drvdata(dev);
-	u8 reg;
-
-	reg = readb(vuart->regs + AST_VUART_GCRA) & AST_VUART_GCRA_VUART_EN;
-
-	return snprintf(buf, PAGE_SIZE - 1, "%u\n", reg ? 1 : 0);
-}
-
-static ssize_t ast_vuart_set_enabled(struct device *dev,
-		struct device_attribute *attr,
-		const char *buf, size_t count)
-{
-	struct ast_vuart *vuart = dev_get_drvdata(dev);
-	unsigned long val;
-	int err;
-	u8 reg;
-
-	err = kstrtoul(buf, 0, &val);
-	if (err)
-		return err;
-
-	reg = readb(vuart->regs + AST_VUART_GCRA);
-	reg &= ~AST_VUART_GCRA_VUART_EN;
-	if (val)
-		reg |= AST_VUART_GCRA_VUART_EN;
-	writeb(reg, vuart->regs + AST_VUART_GCRA);
-
-	return count;
-}
-
-static DEVICE_ATTR(enabled, S_IWUSR | S_IRUGO,
-		ast_vuart_show_enabled, ast_vuart_set_enabled);
-
 static ssize_t ast_vuart_show_addr(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
@@ -144,6 +108,17 @@ static ssize_t ast_vuart_set_sirq(struct device *dev,
 static DEVICE_ATTR(sirq, S_IWUSR | S_IRUGO,
 		ast_vuart_show_sirq, ast_vuart_set_sirq);
 
+static void ast_vuart_set_enabled(struct ast_vuart *vuart, bool enabled)
+{
+	u8 reg;
+
+	reg = readb(vuart->regs + AST_VUART_GCRA);
+	reg &= ~AST_VUART_GCRA_VUART_EN;
+	if (enabled)
+		reg |= AST_VUART_GCRA_VUART_EN;
+	writeb(reg, vuart->regs + AST_VUART_GCRA);
+}
+
 static void ast_vuart_set_host_tx_discard(struct ast_vuart *vuart, bool discard)
 {
 	u8 reg;
@@ -304,6 +279,7 @@ static int ast_vuart_probe(struct platform_device *pdev)
 
 
 	vuart->line = rc;
+	ast_vuart_set_enabled(vuart, true);
 	ast_vuart_set_host_tx_discard(vuart, true);
 	platform_set_drvdata(pdev, vuart);
 
@@ -314,9 +290,6 @@ static int ast_vuart_probe(struct platform_device *pdev)
 	rc = device_create_file(&pdev->dev, &dev_attr_sirq);
 	if (rc)
 		dev_warn(&pdev->dev, "can't create sirq file\n");
-	rc = device_create_file(&pdev->dev, &dev_attr_enabled);
-	if (rc)
-		dev_warn(&pdev->dev, "can't create enabled file\n");
 
 	return 0;
 
@@ -332,6 +305,8 @@ static int ast_vuart_remove(struct platform_device *pdev)
 {
 	struct ast_vuart *vuart = platform_get_drvdata(pdev);
 
+	ast_vuart_set_enabled(vuart, false);
+
 	if (vuart->clk)
 		clk_disable_unprepare(vuart->clk);
 	return 0;
-- 
2.5.0

Re: [RFC PATCH 2/2] serial/aspeed-vuart: Perform enable/disable on driver bind/unbind

[Joel Stanley]

On Thu, Feb 11, 2016 at 6:56 PM, Jeremy Kerr <jk@ozlabs.org> wrote:
> Rather than exposing an enable sysfs attribute, we can just set the
> VUART enabled bit when we bind to the device, and clear it on unbind.
>
> We don't want to do this on open/release, as the host may be using this
> bit to configure serial output modes, which is independent of whether
> the devices has been opened by BMC userspace.
>
> Signed-off-by: Jeremy Kerr <jk@ozlabs.org>

Applied, as discussed on IRC.

Cheers,

Joel

Re: [RFC PATCH 2/2] serial/aspeed-vuart: Perform enable/disable on driver bind/unbind

[Norman James]

[-- Attachment #1.1: Type: text/plain, Size: 4001 bytes --]


Having the driver handle the enable is better.   Looks good to me.


Regards,
Norman James
IBM - POWER Systems Architect
Phone: 1-512-286-6807 (T/L: 363-6807)
Internet: njames@us.ibm.com




From:	Jeremy Kerr <jk@ozlabs.org>
To:	openbmc@lists.ozlabs.org
Date:	02/11/2016 03:03 AM
Subject:	[RFC PATCH 2/2] serial/aspeed-vuart: Perform enable/disable on
            driver bind/unbind
Sent by:	"openbmc" <openbmc-bounces+njames=us.ibm.com@lists.ozlabs.org>



Rather than exposing an enable sysfs attribute, we can just set the
VUART enabled bit when we bind to the device, and clear it on unbind.

We don't want to do this on open/release, as the host may be using this
bit to configure serial output modes, which is independent of whether
the devices has been opened by BMC userspace.

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
Reviewed-by:  Norman James <nkskjames@gmail.com>
---
 drivers/tty/serial/aspeed-vuart.c | 53 ++++++++++
+----------------------------
 1 file changed, 14 insertions(+), 39 deletions(-)

diff --git a/drivers/tty/serial/aspeed-vuart.c
b/drivers/tty/serial/aspeed-vuart.c
index 73bb0e7..020c815 100644
--- a/drivers/tty/serial/aspeed-vuart.c
+++ b/drivers/tty/serial/aspeed-vuart.c
@@ -35,42 +35,6 @@ struct ast_vuart {
 		 int		 		 		 line;
 };

-static ssize_t ast_vuart_show_enabled(struct device *dev,
-		 		 struct device_attribute *attr, char *buf)
-{
-		 struct ast_vuart *vuart = dev_get_drvdata(dev);
-		 u8 reg;
-
-		 reg = readb(vuart->regs + AST_VUART_GCRA) &
AST_VUART_GCRA_VUART_EN;
-
-		 return snprintf(buf, PAGE_SIZE - 1, "%u\n", reg ? 1 : 0);
-}
-
-static ssize_t ast_vuart_set_enabled(struct device *dev,
-		 		 struct device_attribute *attr,
-		 		 const char *buf, size_t count)
-{
-		 struct ast_vuart *vuart = dev_get_drvdata(dev);
-		 unsigned long val;
-		 int err;
-		 u8 reg;
-
-		 err = kstrtoul(buf, 0, &val);
-		 if (err)
-		 		 return err;
-
-		 reg = readb(vuart->regs + AST_VUART_GCRA);
-		 reg &= ~AST_VUART_GCRA_VUART_EN;
-		 if (val)
-		 		 reg |= AST_VUART_GCRA_VUART_EN;
-		 writeb(reg, vuart->regs + AST_VUART_GCRA);
-
-		 return count;
-}
-
-static DEVICE_ATTR(enabled, S_IWUSR | S_IRUGO,
-		 		 ast_vuart_show_enabled, ast_vuart_set_enabled);
-
 static ssize_t ast_vuart_show_addr(struct device *dev,
 		 		 struct device_attribute *attr, char *buf)
 {
@@ -144,6 +108,17 @@ static ssize_t ast_vuart_set_sirq(struct device *dev,
 static DEVICE_ATTR(sirq, S_IWUSR | S_IRUGO,
 		 		 ast_vuart_show_sirq, ast_vuart_set_sirq);

+static void ast_vuart_set_enabled(struct ast_vuart *vuart, bool enabled)
+{
+		 u8 reg;
+
+		 reg = readb(vuart->regs + AST_VUART_GCRA);
+		 reg &= ~AST_VUART_GCRA_VUART_EN;
+		 if (enabled)
+		 		 reg |= AST_VUART_GCRA_VUART_EN;
+		 writeb(reg, vuart->regs + AST_VUART_GCRA);
+}
+
 static void ast_vuart_set_host_tx_discard(struct ast_vuart *vuart, bool
discard)
 {
 		 u8 reg;
@@ -304,6 +279,7 @@ static int ast_vuart_probe(struct platform_device
*pdev)


 		 vuart->line = rc;
+		 ast_vuart_set_enabled(vuart, true);
 		 ast_vuart_set_host_tx_discard(vuart, true);
 		 platform_set_drvdata(pdev, vuart);

@@ -314,9 +290,6 @@ static int ast_vuart_probe(struct platform_device
*pdev)
 		 rc = device_create_file(&pdev->dev, &dev_attr_sirq);
 		 if (rc)
 		 		 dev_warn(&pdev->dev, "can't create sirq file\n");
-		 rc = device_create_file(&pdev->dev, &dev_attr_enabled);
-		 if (rc)
-		 		 dev_warn(&pdev->dev, "can't create enabled file
\n");

 		 return 0;

@@ -332,6 +305,8 @@ static int ast_vuart_remove(struct platform_device
*pdev)
 {
 		 struct ast_vuart *vuart = platform_get_drvdata(pdev);

+		 ast_vuart_set_enabled(vuart, false);
+
 		 if (vuart->clk)
 		 		 clk_disable_unprepare(vuart->clk);
 		 return 0;
--
2.5.0

_______________________________________________
openbmc mailing list
openbmc@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/openbmc


[-- Attachment #1.2: Type: text/html, Size: 6196 bytes --]

[-- Attachment #2: graycol.gif --]
[-- Type: image/gif, Size: 105 bytes --]

[PATCH 1/2 v2] serial/aspeed-vuart: Only disable host tx discard when serial port is in use

[Jeremy Kerr]

OpenPOWER firmware doesn't like it when the host-side of the VUART's
FIFO is not drained. This change only disables host TX discard mode when
the port has been opened.

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>

--

v2:
  Fix wording of changelog

---
 drivers/tty/serial/aspeed-vuart.c | 40 +++++++++++++++++++++++++++++++++++----
 1 file changed, 36 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/aspeed-vuart.c b/drivers/tty/serial/aspeed-vuart.c
index f34884a..73bb0e7 100644
--- a/drivers/tty/serial/aspeed-vuart.c
+++ b/drivers/tty/serial/aspeed-vuart.c
@@ -144,16 +144,45 @@ static ssize_t ast_vuart_set_sirq(struct device *dev,
 static DEVICE_ATTR(sirq, S_IWUSR | S_IRUGO,
 		ast_vuart_show_sirq, ast_vuart_set_sirq);
 
-static void ast_vuart_setup(struct ast_vuart *vuart)
+static void ast_vuart_set_host_tx_discard(struct ast_vuart *vuart, bool discard)
 {
 	u8 reg;
 
-	/* disable TX discard mode */
 	reg = readb(vuart->regs + AST_VUART_GCRA);
-	reg |= AST_VUART_GCRA_HOST_TX_DISCARD;
+
+	/* if the HOST_TX_DISCARD bit is set, discard is *disabled* */
+	reg &= ~AST_VUART_GCRA_HOST_TX_DISCARD;
+	if (!discard)
+		reg |= AST_VUART_GCRA_HOST_TX_DISCARD;
+
 	writeb(reg, vuart->regs + AST_VUART_GCRA);
 }
 
+static int ast_vuart_startup(struct uart_port *uart_port)
+{
+	struct uart_8250_port *uart_8250_port = up_to_u8250p(uart_port);
+	struct ast_vuart *vuart = uart_8250_port->port.private_data;
+	int rc;
+
+	rc = serial8250_do_startup(uart_port);
+	if (rc)
+		return rc;
+
+	ast_vuart_set_host_tx_discard(vuart, false);
+
+	return 0;
+}
+
+static void ast_vuart_shutdown(struct uart_port *uart_port)
+{
+	struct uart_8250_port *uart_8250_port = up_to_u8250p(uart_port);
+	struct ast_vuart *vuart = uart_8250_port->port.private_data;
+
+	ast_vuart_set_host_tx_discard(vuart, true);
+
+	serial8250_do_shutdown(uart_port);
+}
+
 
 /**
  * The device tree parsing code here is heavily based on that of the of_serial
@@ -190,9 +219,12 @@ static int ast_vuart_probe(struct platform_device *pdev)
 	}
 
 	memset(&port, 0, sizeof(port));
+	port.port.private_data = vuart;
 	port.port.membase = vuart->regs;
 	port.port.mapbase = resource.start;
 	port.port.mapsize = resource_size(&resource);
+	port.port.startup = ast_vuart_startup;
+	port.port.shutdown = ast_vuart_shutdown;
 
 	if (of_property_read_u32(np, "clock-frequency", &clk)) {
 		vuart->clk = devm_clk_get(&pdev->dev, NULL);
@@ -272,8 +304,8 @@ static int ast_vuart_probe(struct platform_device *pdev)
 
 
 	vuart->line = rc;
+	ast_vuart_set_host_tx_discard(vuart, true);
 	platform_set_drvdata(pdev, vuart);
-	ast_vuart_setup(vuart);
 
 	/* extra sysfs control */
 	rc = device_create_file(&pdev->dev, &dev_attr_lpc_address);
-- 
2.5.0

Re: [PATCH 1/2 v2] serial/aspeed-vuart: Only disable host tx discard when serial port is in use

[Stewart Smith]

Jeremy Kerr <jk@ozlabs.org> writes:
> OpenPOWER firmware doesn't like it when the host-side of the VUART's
> FIFO is not drained. This change only disables host TX discard mode when
> the port has been opened.

Do we have what's arguably a bug somewhere?

-- 
Stewart Smith
OPAL Architect, IBM.

Re: [PATCH 1/2 v2] serial/aspeed-vuart: Only disable host tx discard when serial port is in use

[Jeremy Kerr]

Hi Stewart,

>> OpenPOWER firmware doesn't like it when the host-side of the VUART's
>> FIFO is not drained. This change only disables host TX discard mode when
>> the port has been opened.
> 
> Do we have what's arguably a bug somewhere?

I'm not 100% sure that it's a bug; it's fairly reasonable to expect that
a UART's TX FIFO will be drained. That expectation is present in a lot
of different UART implementations (including skiboot & Linux).

Cheers,


Jeremy

Re: [PATCH 1/2 v2] serial/aspeed-vuart: Only disable host tx discard when serial port is in use

[Joel Stanley]

On Thu, Feb 11, 2016 at 6:59 PM, Jeremy Kerr <jk@ozlabs.org> wrote:
> OpenPOWER firmware doesn't like it when the host-side of the VUART's
> FIFO is not drained. This change only disables host TX discard mode when
> the port has been opened.
>
> Signed-off-by: Jeremy Kerr <jk@ozlabs.org>

Thanks, applied.

Re: [PATCH 1/2] serial/aspeed-vuart: Only disable host tx discard when serial port is in use

[Norman James]

[-- Attachment #1.1: Type: text/plain, Size: 3589 bytes --]


I tried this with various cases of console usage.  Starting console in
beginning, in the middle, never, etc.  Seems to work.  Please get merged
ASAP.  Thanks!


Regards,
Norman James
IBM - POWER Systems Architect
Phone: 1-512-286-6807 (T/L: 363-6807)
Internet: njames@us.ibm.com




From:	Jeremy Kerr <jk@ozlabs.org>
To:	openbmc@lists.ozlabs.org
Date:	02/11/2016 03:03 AM
Subject:	[PATCH 1/2] serial/aspeed-vuart: Only disable host tx discard
            when serial port is in use
Sent by:	"openbmc" <openbmc-bounces+njames=us.ibm.com@lists.ozlabs.org>



OpenPOWER firmware doesn't like it when the host-side of the VUART's
FIFO is not drained. This change shifts only disables host TX discard
mode when the port has been opened.

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
---
 drivers/tty/serial/aspeed-vuart.c | 40 ++++++++++++++++++++++++++++++++++
+----
 1 file changed, 36 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/aspeed-vuart.c
b/drivers/tty/serial/aspeed-vuart.c
index f34884a..73bb0e7 100644
--- a/drivers/tty/serial/aspeed-vuart.c
+++ b/drivers/tty/serial/aspeed-vuart.c
@@ -144,16 +144,45 @@ static ssize_t ast_vuart_set_sirq(struct device *dev,
 static DEVICE_ATTR(sirq, S_IWUSR | S_IRUGO,
 		 		 ast_vuart_show_sirq, ast_vuart_set_sirq);

-static void ast_vuart_setup(struct ast_vuart *vuart)
+static void ast_vuart_set_host_tx_discard(struct ast_vuart *vuart, bool
discard)
 {
 		 u8 reg;

-		 /* disable TX discard mode */
 		 reg = readb(vuart->regs + AST_VUART_GCRA);
-		 reg |= AST_VUART_GCRA_HOST_TX_DISCARD;
+
+		 /* if the HOST_TX_DISCARD bit is set, discard is *disabled* */
+		 reg &= ~AST_VUART_GCRA_HOST_TX_DISCARD;
+		 if (!discard)
+		 		 reg |= AST_VUART_GCRA_HOST_TX_DISCARD;
+
 		 writeb(reg, vuart->regs + AST_VUART_GCRA);
 }

+static int ast_vuart_startup(struct uart_port *uart_port)
+{
+		 struct uart_8250_port *uart_8250_port = up_to_u8250p
(uart_port);
+		 struct ast_vuart *vuart = uart_8250_port->port.private_data;
+		 int rc;
+
+		 rc = serial8250_do_startup(uart_port);
+		 if (rc)
+		 		 return rc;
+
+		 ast_vuart_set_host_tx_discard(vuart, false);
+
+		 return 0;
+}
+
+static void ast_vuart_shutdown(struct uart_port *uart_port)
+{
+		 struct uart_8250_port *uart_8250_port = up_to_u8250p
(uart_port);
+		 struct ast_vuart *vuart = uart_8250_port->port.private_data;
+
+		 ast_vuart_set_host_tx_discard(vuart, true);
+
+		 serial8250_do_shutdown(uart_port);
+}
+

 /**
  * The device tree parsing code here is heavily based on that of the
of_serial
@@ -190,9 +219,12 @@ static int ast_vuart_probe(struct platform_device
*pdev)
 		 }

 		 memset(&port, 0, sizeof(port));
+		 port.port.private_data = vuart;
 		 port.port.membase = vuart->regs;
 		 port.port.mapbase = resource.start;
 		 port.port.mapsize = resource_size(&resource);
+		 port.port.startup = ast_vuart_startup;
+		 port.port.shutdown = ast_vuart_shutdown;

 		 if (of_property_read_u32(np, "clock-frequency", &clk)) {
 		 		 vuart->clk = devm_clk_get(&pdev->dev, NULL);
@@ -272,8 +304,8 @@ static int ast_vuart_probe(struct platform_device
*pdev)


 		 vuart->line = rc;
+		 ast_vuart_set_host_tx_discard(vuart, true);
 		 platform_set_drvdata(pdev, vuart);
-		 ast_vuart_setup(vuart);

 		 /* extra sysfs control */
 		 rc = device_create_file(&pdev->dev, &dev_attr_lpc_address);
--
2.5.0

_______________________________________________
openbmc mailing list
openbmc@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/openbmc


[-- Attachment #1.2: Type: text/html, Size: 5406 bytes --]

[-- Attachment #2: graycol.gif --]
[-- Type: image/gif, Size: 105 bytes --]