[PATCH 0/2] drivers: serial: Aspeed VUART driver

[PATCH 0/2] drivers: serial: Aspeed VUART driver

[Joel Stanley]

This is a driver for the Aspeed VUART. The VUART is a serial device on the BMC
side of the LPC bus that connects a BMC to it's host processor.

We add a flag to the serial core to allow the driver to skip probing of the
THRE irq behaviour, which could hang due to the host not reading bytes out of
the buffer.

We've been using this on systems for over a year, so it has seen a good amount
of testing.

Cheers,

Joel


Jeremy Kerr (1):
  drivers/serial: Add driver for Aspeed virtual UART

Joel Stanley (1):
  serial: 8250: Add flag so drivers can avoid THRE probe

 Documentation/devicetree/bindings/serial/8250.txt |   2 +
 drivers/tty/serial/8250/8250_port.c               |   2 +-
 drivers/tty/serial/Kconfig                        |  10 +
 drivers/tty/serial/Makefile                       |   1 +
 drivers/tty/serial/aspeed-vuart.c                 | 335 ++++++++++++++++++++++
 include/linux/serial_core.h                       |   1 +
 6 files changed, 350 insertions(+), 1 deletion(-)
 create mode 100644 drivers/tty/serial/aspeed-vuart.c

-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] serial: 8250: Add flag so drivers can avoid THRE probe

[Joel Stanley]

The probing of THRE irq behaviour assumes the other end will be reading
bytes out of the buffer in order to probe the port at driver init. In
some cases the other end cannot be relied upon to read these bytes, so
provide a flag for them to skip this step.

Bit 16 was chosen as the flags are a int and the top bits are taken.

Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 drivers/tty/serial/8250/8250_port.c | 2 +-
 include/linux/serial_core.h         | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index fe4399b41df6..f4c6da107dec 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2205,7 +2205,7 @@ int serial8250_do_startup(struct uart_port *port)
 		}
 	}
 
-	if (port->irq) {
+	if (port->irq && !(up->port.flags & UPF_NO_THRE_TEST)) {
 		unsigned char iir1;
 		/*
 		 * Test for UARTs that do not reassert THRE when the
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 5def8e830fb0..f9e1fa39f553 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -195,6 +195,7 @@ struct uart_port {
 #define UPF_NO_TXEN_TEST	((__force upf_t) (1 << 15))
 #define UPF_MAGIC_MULTIPLIER	((__force upf_t) ASYNC_MAGIC_MULTIPLIER /* 16 */ )
 
+#define UPF_NO_THRE_TEST	((__force upf_t) (1 << 19))
 /* Port has hardware-assisted h/w flow control */
 #define UPF_AUTO_CTS		((__force upf_t) (1 << 20))
 #define UPF_AUTO_RTS		((__force upf_t) (1 << 21))
-- 
2.11.0

[PATCH 2/2] drivers/serial: Add driver for Aspeed virtual UART

[Joel Stanley]

From: Jeremy Kerr <jk@ozlabs.org>

This change adds a driver for the 16550-based Aspeed virtual UART
device. We use a similar process to the of_serial driver for device
probe, but expose some VUART-specific functions through sysfs too.

OpenPOWER host firmware doesn't like it when the host-side of the
VUART's FIFO is not drained. This driver only disables host TX discard
mode when the port is in use. We 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>
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 Documentation/devicetree/bindings/serial/8250.txt |   2 +
 drivers/tty/serial/Kconfig                        |  10 +
 drivers/tty/serial/Makefile                       |   1 +
 drivers/tty/serial/aspeed-vuart.c                 | 335 ++++++++++++++++++++++
 4 files changed, 348 insertions(+)
 create mode 100644 drivers/tty/serial/aspeed-vuart.c

diff --git a/Documentation/devicetree/bindings/serial/8250.txt b/Documentation/devicetree/bindings/serial/8250.txt
index f86bb06c39e9..a12e9277ac5d 100644
--- a/Documentation/devicetree/bindings/serial/8250.txt
+++ b/Documentation/devicetree/bindings/serial/8250.txt
@@ -19,6 +19,8 @@ Required properties:
 	- "altr,16550-FIFO128"
 	- "fsl,16550-FIFO64"
 	- "fsl,ns16550"
+	- "aspeed,ast2400-vuart"
+	- "aspeed,ast2500-vuart"
 	- "serial" if the port type is unknown.
 - reg : offset and length of the register set for the device.
 - interrupts : should contain uart interrupt.
diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index e9cf5b67f1b7..758b69a51078 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -1129,6 +1129,16 @@ config SERIAL_NETX_CONSOLE
 	  If you have enabled the serial port on the Hilscher NetX SoC
 	  you can make it the console by answering Y to this option.
 
+config SERIAL_ASPEED_VUART
+	tristate "Aspeed Virtual UART"
+	depends on OF
+	depends on SERIAL_8250
+	help
+	  If you want to use the virtual UART (VUART) device on Aspeed
+	  BMC platforms, enable this option. This enables the 16550A-
+	  compatible device on the local LPC bus, giving a UART device
+	  with no physical RS232 connections.
+
 config SERIAL_OMAP
 	tristate "OMAP serial port support"
 	depends on ARCH_OMAP2PLUS
diff --git a/drivers/tty/serial/Makefile b/drivers/tty/serial/Makefile
index 2d6288bc4554..5b97b0fa29e2 100644
--- a/drivers/tty/serial/Makefile
+++ b/drivers/tty/serial/Makefile
@@ -22,6 +22,7 @@ obj-$(CONFIG_SERIAL_8250) += 8250/
 
 obj-$(CONFIG_SERIAL_AMBA_PL010) += amba-pl010.o
 obj-$(CONFIG_SERIAL_AMBA_PL011) += amba-pl011.o
+obj-$(CONFIG_SERIAL_ASPEED_VUART) += aspeed-vuart.o
 obj-$(CONFIG_SERIAL_CLPS711X) += clps711x.o
 obj-$(CONFIG_SERIAL_PXA_NON8250) += pxa.o
 obj-$(CONFIG_SERIAL_PNX8XXX) += pnx8xxx_uart.o
diff --git a/drivers/tty/serial/aspeed-vuart.c b/drivers/tty/serial/aspeed-vuart.c
new file mode 100644
index 000000000000..fc6fa6d243c8
--- /dev/null
+++ b/drivers/tty/serial/aspeed-vuart.c
@@ -0,0 +1,335 @@
+/*
+ *  Serial Port driver for Aspeed VUART device
+ *
+ *    Copyright (C) 2016 Jeremy Kerr <jk@ozlabs.org>, IBM Corp.
+ *    Copyright (C) 2006 Arnd Bergmann <arnd@arndb.de>, IBM Corp.
+ *
+ *  This program is free software; you can redistribute it and/or
+ *  modify it under the terms of the GNU General Public License
+ *  as published by the Free Software Foundation; either version
+ *  2 of the License, or (at your option) any later version.
+ *
+ */
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
+#include <linux/clk.h>
+
+#include "8250/8250.h"
+
+#define AST_VUART_GCRA		0x20
+#define AST_VUART_GCRA_VUART_EN		0x01
+#define AST_VUART_GCRA_HOST_TX_DISCARD	0x20
+#define AST_VUART_GCRB		0x24
+#define AST_VUART_GCRB_HOST_SIRQ_MASK	0xf0
+#define AST_VUART_GCRB_HOST_SIRQ_SHIFT	4
+#define AST_VUART_ADDRL		0x28
+#define AST_VUART_ADDRH		0x2c
+
+struct ast_vuart {
+	struct platform_device *pdev;
+	void __iomem		*regs;
+	struct clk		*clk;
+	int			line;
+};
+
+static ssize_t ast_vuart_show_addr(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct ast_vuart *vuart = dev_get_drvdata(dev);
+	u16 addr;
+
+	addr = (readb(vuart->regs + AST_VUART_ADDRH) << 8) |
+		(readb(vuart->regs + AST_VUART_ADDRL));
+
+	return snprintf(buf, PAGE_SIZE - 1, "0x%x\n", addr);
+}
+
+static ssize_t ast_vuart_set_addr(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;
+
+	err = kstrtoul(buf, 0, &val);
+	if (err)
+		return err;
+
+	writeb((val >> 8) & 0xff, vuart->regs + AST_VUART_ADDRH);
+	writeb((val >> 0) & 0xff, vuart->regs + AST_VUART_ADDRL);
+
+	return count;
+}
+
+static DEVICE_ATTR(lpc_address, 0664,
+		ast_vuart_show_addr, ast_vuart_set_addr);
+
+static ssize_t ast_vuart_show_sirq(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_GCRB);
+	reg &= AST_VUART_GCRB_HOST_SIRQ_MASK;
+	reg >>= AST_VUART_GCRB_HOST_SIRQ_SHIFT;
+
+	return snprintf(buf, PAGE_SIZE - 1, "%u\n", reg);
+}
+
+static ssize_t ast_vuart_set_sirq(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;
+
+	val <<= AST_VUART_GCRB_HOST_SIRQ_SHIFT;
+	val &= AST_VUART_GCRB_HOST_SIRQ_MASK;
+
+	reg = readb(vuart->regs + AST_VUART_GCRB);
+	reg &= ~AST_VUART_GCRB_HOST_SIRQ_MASK;
+	reg |= val;
+	writeb(reg, vuart->regs + AST_VUART_GCRB);
+
+	return count;
+}
+
+static DEVICE_ATTR(sirq, 0664,
+		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;
+
+	reg = readb(vuart->regs + AST_VUART_GCRA);
+
+	/* 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
+ * driver, but we have a few core differences, as we need to use our own
+ * ioremapping for extra register support
+ */
+static int ast_vuart_probe(struct platform_device *pdev)
+{
+	struct uart_8250_port port;
+	struct resource resource;
+	struct ast_vuart *vuart;
+	struct device_node *np;
+	u32 clk, prop;
+	int rc;
+
+	np = pdev->dev.of_node;
+
+	vuart = devm_kzalloc(&pdev->dev, sizeof(*vuart), GFP_KERNEL);
+	if (!vuart)
+		return -ENOMEM;
+
+	vuart->pdev = pdev;
+	rc = of_address_to_resource(np, 0, &resource);
+	if (rc) {
+		dev_warn(&pdev->dev, "invalid address\n");
+		return rc;
+	}
+
+	/* create our own mapping for VUART-specific registers */
+	vuart->regs = devm_ioremap_resource(&pdev->dev, &resource);
+	if (IS_ERR(vuart->regs)) {
+		dev_warn(&pdev->dev, "failed to map registers\n");
+		return PTR_ERR(vuart->regs);
+	}
+
+	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);
+		if (IS_ERR(vuart->clk)) {
+			dev_warn(&pdev->dev,
+				"clk or clock-frequency not defined\n");
+			return PTR_ERR(vuart->clk);
+		}
+
+		rc = clk_prepare_enable(vuart->clk);
+		if (rc < 0)
+			return rc;
+
+		clk = clk_get_rate(vuart->clk);
+	}
+
+	/* If current-speed was set, then try not to change it. */
+	if (of_property_read_u32(np, "current-speed", &prop) == 0)
+		port.port.custom_divisor = clk / (16 * prop);
+
+	/* Check for shifted address mapping */
+	if (of_property_read_u32(np, "reg-offset", &prop) == 0)
+		port.port.mapbase += prop;
+
+	/* Check for registers offset within the devices address range */
+	if (of_property_read_u32(np, "reg-shift", &prop) == 0)
+		port.port.regshift = prop;
+
+	/* Check for fifo size */
+	if (of_property_read_u32(np, "fifo-size", &prop) == 0)
+		port.port.fifosize = prop;
+
+	/* Check for a fixed line number */
+	rc = of_alias_get_id(np, "serial");
+	if (rc >= 0)
+		port.port.line = rc;
+
+	port.port.irq = irq_of_parse_and_map(np, 0);
+	port.port.irqflags = IRQF_SHARED;
+	port.port.iotype = UPIO_MEM;
+	if (of_property_read_u32(np, "reg-io-width", &prop) == 0) {
+		switch (prop) {
+		case 1:
+			port.port.iotype = UPIO_MEM;
+			break;
+		case 4:
+			port.port.iotype = of_device_is_big_endian(np) ?
+				       UPIO_MEM32BE : UPIO_MEM32;
+			break;
+		default:
+			dev_warn(&pdev->dev, "unsupported reg-io-width (%d)\n",
+				 prop);
+			rc = -EINVAL;
+			goto err_clk_disable;
+		}
+	}
+
+	port.port.type = PORT_16550A;
+	port.port.uartclk = clk;
+	port.port.flags = UPF_SHARE_IRQ | UPF_BOOT_AUTOCONF
+		| UPF_FIXED_PORT | UPF_FIXED_TYPE | UPF_NO_THRE_TEST;
+
+	if (of_find_property(np, "no-loopback-test", NULL))
+		port.port.flags |= UPF_SKIP_TEST;
+
+	port.port.dev = &pdev->dev;
+
+	if (port.port.fifosize)
+		port.capabilities = UART_CAP_FIFO;
+
+	if (of_property_read_bool(pdev->dev.of_node,
+				  "auto-flow-control"))
+		port.capabilities |= UART_CAP_AFE;
+
+	rc = serial8250_register_8250_port(&port);
+	if (rc < 0)
+		goto err_clk_disable;
+
+
+	vuart->line = rc;
+	ast_vuart_set_enabled(vuart, true);
+	ast_vuart_set_host_tx_discard(vuart, true);
+	platform_set_drvdata(pdev, vuart);
+
+	/* extra sysfs control */
+	rc = device_create_file(&pdev->dev, &dev_attr_lpc_address);
+	if (rc)
+		dev_warn(&pdev->dev, "can't create lpc_address file\n");
+	rc = device_create_file(&pdev->dev, &dev_attr_sirq);
+	if (rc)
+		dev_warn(&pdev->dev, "can't create sirq file\n");
+
+	return 0;
+
+err_clk_disable:
+	if (vuart->clk)
+		clk_disable_unprepare(vuart->clk);
+
+	irq_dispose_mapping(port.port.irq);
+	return rc;
+}
+
+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;
+}
+
+static const struct of_device_id ast_vuart_table[] = {
+	{ .compatible = "aspeed,ast2400-vuart" },
+	{ .compatible = "aspeed,ast2500-vuart" },
+	{ },
+};
+
+static struct platform_driver ast_vuart_driver = {
+	.driver = {
+		.name = "aspeed-vuart",
+		.of_match_table = ast_vuart_table,
+	},
+	.probe = ast_vuart_probe,
+	.remove = ast_vuart_remove,
+};
+
+module_platform_driver(ast_vuart_driver);
+
+MODULE_AUTHOR("Jeremy Kerr <jk@ozlabs.org>");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Driver for Aspeed VUART device");
-- 
2.11.0

Re: [PATCH 2/2] drivers/serial: Add driver for Aspeed virtual UART

[Greg Kroah-Hartman]

On Tue, Mar 28, 2017 at 04:14:58PM +1030, Joel Stanley wrote:
> From: Jeremy Kerr <jk-mnsaURCQ41sdnm+yROfE0A@public.gmane.org>
> 
> This change adds a driver for the 16550-based Aspeed virtual UART
> device. We use a similar process to the of_serial driver for device
> probe, but expose some VUART-specific functions through sysfs too.
> 
> OpenPOWER host firmware doesn't like it when the host-side of the
> VUART's FIFO is not drained. This driver only disables host TX discard
> mode when the port is in use. We 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-mnsaURCQ41sdnm+yROfE0A@public.gmane.org>
> Signed-off-by: Joel Stanley <joel-U3u1mxZcP9KHXe+LvDLADg@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/serial/8250.txt |   2 +
>  drivers/tty/serial/Kconfig                        |  10 +
>  drivers/tty/serial/Makefile                       |   1 +
>  drivers/tty/serial/aspeed-vuart.c                 | 335 ++++++++++++++++++++++
>  4 files changed, 348 insertions(+)
>  create mode 100644 drivers/tty/serial/aspeed-vuart.c
> 
> diff --git a/Documentation/devicetree/bindings/serial/8250.txt b/Documentation/devicetree/bindings/serial/8250.txt
> index f86bb06c39e9..a12e9277ac5d 100644
> --- a/Documentation/devicetree/bindings/serial/8250.txt
> +++ b/Documentation/devicetree/bindings/serial/8250.txt
> @@ -19,6 +19,8 @@ Required properties:
>  	- "altr,16550-FIFO128"
>  	- "fsl,16550-FIFO64"
>  	- "fsl,ns16550"
> +	- "aspeed,ast2400-vuart"
> +	- "aspeed,ast2500-vuart"
>  	- "serial" if the port type is unknown.
>  - reg : offset and length of the register set for the device.
>  - interrupts : should contain uart interrupt.
> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> index e9cf5b67f1b7..758b69a51078 100644
> --- a/drivers/tty/serial/Kconfig
> +++ b/drivers/tty/serial/Kconfig
> @@ -1129,6 +1129,16 @@ config SERIAL_NETX_CONSOLE
>  	  If you have enabled the serial port on the Hilscher NetX SoC
>  	  you can make it the console by answering Y to this option.
>  
> +config SERIAL_ASPEED_VUART
> +	tristate "Aspeed Virtual UART"
> +	depends on OF
> +	depends on SERIAL_8250
> +	help
> +	  If you want to use the virtual UART (VUART) device on Aspeed
> +	  BMC platforms, enable this option. This enables the 16550A-
> +	  compatible device on the local LPC bus, giving a UART device
> +	  with no physical RS232 connections.
> +
>  config SERIAL_OMAP
>  	tristate "OMAP serial port support"
>  	depends on ARCH_OMAP2PLUS
> diff --git a/drivers/tty/serial/Makefile b/drivers/tty/serial/Makefile
> index 2d6288bc4554..5b97b0fa29e2 100644
> --- a/drivers/tty/serial/Makefile
> +++ b/drivers/tty/serial/Makefile
> @@ -22,6 +22,7 @@ obj-$(CONFIG_SERIAL_8250) += 8250/
>  
>  obj-$(CONFIG_SERIAL_AMBA_PL010) += amba-pl010.o
>  obj-$(CONFIG_SERIAL_AMBA_PL011) += amba-pl011.o
> +obj-$(CONFIG_SERIAL_ASPEED_VUART) += aspeed-vuart.o
>  obj-$(CONFIG_SERIAL_CLPS711X) += clps711x.o
>  obj-$(CONFIG_SERIAL_PXA_NON8250) += pxa.o
>  obj-$(CONFIG_SERIAL_PNX8XXX) += pnx8xxx_uart.o
> diff --git a/drivers/tty/serial/aspeed-vuart.c b/drivers/tty/serial/aspeed-vuart.c
> new file mode 100644
> index 000000000000..fc6fa6d243c8
> --- /dev/null
> +++ b/drivers/tty/serial/aspeed-vuart.c
> @@ -0,0 +1,335 @@
> +/*
> + *  Serial Port driver for Aspeed VUART device
> + *
> + *    Copyright (C) 2016 Jeremy Kerr <jk-mnsaURCQ41sdnm+yROfE0A@public.gmane.org>, IBM Corp.
> + *    Copyright (C) 2006 Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>, IBM Corp.
> + *
> + *  This program is free software; you can redistribute it and/or
> + *  modify it under the terms of the GNU General Public License
> + *  as published by the Free Software Foundation; either version
> + *  2 of the License, or (at your option) any later version.
> + *
> + */
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> +#include <linux/clk.h>
> +
> +#include "8250/8250.h"
> +
> +#define AST_VUART_GCRA		0x20
> +#define AST_VUART_GCRA_VUART_EN		0x01
> +#define AST_VUART_GCRA_HOST_TX_DISCARD	0x20
> +#define AST_VUART_GCRB		0x24
> +#define AST_VUART_GCRB_HOST_SIRQ_MASK	0xf0
> +#define AST_VUART_GCRB_HOST_SIRQ_SHIFT	4
> +#define AST_VUART_ADDRL		0x28
> +#define AST_VUART_ADDRH		0x2c
> +
> +struct ast_vuart {
> +	struct platform_device *pdev;
> +	void __iomem		*regs;
> +	struct clk		*clk;
> +	int			line;
> +};
> +
> +static ssize_t ast_vuart_show_addr(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct ast_vuart *vuart = dev_get_drvdata(dev);
> +	u16 addr;
> +
> +	addr = (readb(vuart->regs + AST_VUART_ADDRH) << 8) |
> +		(readb(vuart->regs + AST_VUART_ADDRL));
> +
> +	return snprintf(buf, PAGE_SIZE - 1, "0x%x\n", addr);
> +}
> +
> +static ssize_t ast_vuart_set_addr(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;
> +
> +	err = kstrtoul(buf, 0, &val);
> +	if (err)
> +		return err;
> +
> +	writeb((val >> 8) & 0xff, vuart->regs + AST_VUART_ADDRH);
> +	writeb((val >> 0) & 0xff, vuart->regs + AST_VUART_ADDRL);
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR(lpc_address, 0664,
> +		ast_vuart_show_addr, ast_vuart_set_addr);

DEVICE_ATTR_RW()?

And why random sysfs files for a uart?  Where have you documented these?

> +
> +static ssize_t ast_vuart_show_sirq(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_GCRB);
> +	reg &= AST_VUART_GCRB_HOST_SIRQ_MASK;
> +	reg >>= AST_VUART_GCRB_HOST_SIRQ_SHIFT;
> +
> +	return snprintf(buf, PAGE_SIZE - 1, "%u\n", reg);
> +}
> +
> +static ssize_t ast_vuart_set_sirq(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;
> +
> +	val <<= AST_VUART_GCRB_HOST_SIRQ_SHIFT;
> +	val &= AST_VUART_GCRB_HOST_SIRQ_MASK;
> +
> +	reg = readb(vuart->regs + AST_VUART_GCRB);
> +	reg &= ~AST_VUART_GCRB_HOST_SIRQ_MASK;
> +	reg |= val;
> +	writeb(reg, vuart->regs + AST_VUART_GCRB);
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR(sirq, 0664,
> +		ast_vuart_show_sirq, ast_vuart_set_sirq);

DEVICE_ATTR_RW().

> +
> +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;
> +
> +	reg = readb(vuart->regs + AST_VUART_GCRA);
> +
> +	/* 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
> + * driver, but we have a few core differences, as we need to use our own
> + * ioremapping for extra register support
> + */
> +static int ast_vuart_probe(struct platform_device *pdev)
> +{
> +	struct uart_8250_port port;
> +	struct resource resource;
> +	struct ast_vuart *vuart;
> +	struct device_node *np;
> +	u32 clk, prop;
> +	int rc;
> +
> +	np = pdev->dev.of_node;
> +
> +	vuart = devm_kzalloc(&pdev->dev, sizeof(*vuart), GFP_KERNEL);
> +	if (!vuart)
> +		return -ENOMEM;
> +
> +	vuart->pdev = pdev;
> +	rc = of_address_to_resource(np, 0, &resource);
> +	if (rc) {
> +		dev_warn(&pdev->dev, "invalid address\n");
> +		return rc;
> +	}
> +
> +	/* create our own mapping for VUART-specific registers */
> +	vuart->regs = devm_ioremap_resource(&pdev->dev, &resource);
> +	if (IS_ERR(vuart->regs)) {
> +		dev_warn(&pdev->dev, "failed to map registers\n");
> +		return PTR_ERR(vuart->regs);
> +	}
> +
> +	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);
> +		if (IS_ERR(vuart->clk)) {
> +			dev_warn(&pdev->dev,
> +				"clk or clock-frequency not defined\n");
> +			return PTR_ERR(vuart->clk);
> +		}
> +
> +		rc = clk_prepare_enable(vuart->clk);
> +		if (rc < 0)
> +			return rc;
> +
> +		clk = clk_get_rate(vuart->clk);
> +	}
> +
> +	/* If current-speed was set, then try not to change it. */
> +	if (of_property_read_u32(np, "current-speed", &prop) == 0)
> +		port.port.custom_divisor = clk / (16 * prop);
> +
> +	/* Check for shifted address mapping */
> +	if (of_property_read_u32(np, "reg-offset", &prop) == 0)
> +		port.port.mapbase += prop;
> +
> +	/* Check for registers offset within the devices address range */
> +	if (of_property_read_u32(np, "reg-shift", &prop) == 0)
> +		port.port.regshift = prop;
> +
> +	/* Check for fifo size */
> +	if (of_property_read_u32(np, "fifo-size", &prop) == 0)
> +		port.port.fifosize = prop;
> +
> +	/* Check for a fixed line number */
> +	rc = of_alias_get_id(np, "serial");
> +	if (rc >= 0)
> +		port.port.line = rc;
> +
> +	port.port.irq = irq_of_parse_and_map(np, 0);
> +	port.port.irqflags = IRQF_SHARED;
> +	port.port.iotype = UPIO_MEM;
> +	if (of_property_read_u32(np, "reg-io-width", &prop) == 0) {
> +		switch (prop) {
> +		case 1:
> +			port.port.iotype = UPIO_MEM;
> +			break;
> +		case 4:
> +			port.port.iotype = of_device_is_big_endian(np) ?
> +				       UPIO_MEM32BE : UPIO_MEM32;
> +			break;
> +		default:
> +			dev_warn(&pdev->dev, "unsupported reg-io-width (%d)\n",
> +				 prop);
> +			rc = -EINVAL;
> +			goto err_clk_disable;
> +		}
> +	}
> +
> +	port.port.type = PORT_16550A;
> +	port.port.uartclk = clk;
> +	port.port.flags = UPF_SHARE_IRQ | UPF_BOOT_AUTOCONF
> +		| UPF_FIXED_PORT | UPF_FIXED_TYPE | UPF_NO_THRE_TEST;
> +
> +	if (of_find_property(np, "no-loopback-test", NULL))
> +		port.port.flags |= UPF_SKIP_TEST;
> +
> +	port.port.dev = &pdev->dev;
> +
> +	if (port.port.fifosize)
> +		port.capabilities = UART_CAP_FIFO;
> +
> +	if (of_property_read_bool(pdev->dev.of_node,
> +				  "auto-flow-control"))
> +		port.capabilities |= UART_CAP_AFE;
> +
> +	rc = serial8250_register_8250_port(&port);
> +	if (rc < 0)
> +		goto err_clk_disable;
> +
> +
> +	vuart->line = rc;
> +	ast_vuart_set_enabled(vuart, true);
> +	ast_vuart_set_host_tx_discard(vuart, true);
> +	platform_set_drvdata(pdev, vuart);
> +
> +	/* extra sysfs control */
> +	rc = device_create_file(&pdev->dev, &dev_attr_lpc_address);
> +	if (rc)
> +		dev_warn(&pdev->dev, "can't create lpc_address file\n");
> +	rc = device_create_file(&pdev->dev, &dev_attr_sirq);
> +	if (rc)
> +		dev_warn(&pdev->dev, "can't create sirq file\n");

use an attribute group?  You just raced userspace and lost :(

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] drivers/serial: Add driver for Aspeed virtual UART

[Benjamin Herrenschmidt]

On Fri, 2017-03-31 at 15:31 +0200, Greg Kroah-Hartman wrote:
> DEVICE_ATTR_RW()?
> 
> And why random sysfs files for a uart?  Where have you documented
> these?

We should stick a file somewhere in Documentation I suppose or
at least put that in a comment blob at the head of the file.

FYI:

The VUART is basically two UART 'front ends' connected by their FIFO
(no actual serial line in between). One is on the BMC side (management
controller) and one is on the host CPU side.

It allows the BMC to provide to the host a "UART" that pipes into
the BMC itself and can then be turned by the BMC into a network console
of some sort for example.

This driver is for the BMC side. The sysfs files allow the BMC
userspace which owns the system configuration policy, to specify
at what IO port and interrupt number the host side will appear
to the host on the Host <-> BMC LPC bus. It could be different on a
different system (though most of them use 3f8/4).

Cheers,
Ben.

Re: [PATCH 2/2] drivers/serial: Add driver for Aspeed virtual UART

[Andy Shevchenko]

On Tue, Mar 28, 2017 at 8:44 AM, Joel Stanley <joel-U3u1mxZcP9KHXe+LvDLADg@public.gmane.org> wrote:
> This change adds a driver for the 16550-based Aspeed virtual UART
> device. We use a similar process to the of_serial driver for device
> probe, but expose some VUART-specific functions through sysfs too.
>
> OpenPOWER host firmware doesn't like it when the host-side of the
> VUART's FIFO is not drained. This driver only disables host TX discard
> mode when the port is in use. We 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.

>  Documentation/devicetree/bindings/serial/8250.txt |   2 +
>  drivers/tty/serial/Kconfig                        |  10 +
>  drivers/tty/serial/Makefile                       |   1 +
>  drivers/tty/serial/aspeed-vuart.c                 | 335 ++++++++++++++++++++++

And why it's not under 8250 folder?

> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> +#include <linux/clk.h>
> +
> +#include "8250/8250.h"
> +
> +#define AST_VUART_GCRA         0x20

> +#define AST_VUART_GCRA_VUART_EN                0x01
> +#define AST_VUART_GCRA_HOST_TX_DISCARD 0x20

BIT(x) ?

> +#define AST_VUART_GCRB         0x24

> +#define AST_VUART_GCRB_HOST_SIRQ_MASK  0xf0

GENMASK() ?

> +#define AST_VUART_GCRB_HOST_SIRQ_SHIFT 4
> +#define AST_VUART_ADDRL                0x28
> +#define AST_VUART_ADDRH                0x2c
> +
> +struct ast_vuart {

> +       struct platform_device *pdev;

Ususally there is no need to keep pointer to sturct platform_device,
rahter we need struct device *dev.

> +       void __iomem            *regs;
> +       struct clk              *clk;
> +       int                     line;
> +};
> +
> +static ssize_t ast_vuart_show_addr(struct device *dev,
> +               struct device_attribute *attr, char *buf)
> +{
> +       struct ast_vuart *vuart = dev_get_drvdata(dev);
> +       u16 addr;
> +

> +       addr = (readb(vuart->regs + AST_VUART_ADDRH) << 8) |
> +               (readb(vuart->regs + AST_VUART_ADDRL));

It looks like you have register shift 2 bits and byte accessors. We
have some helpers for that (serial_in() / serial_out() or alike).

> +
> +       return snprintf(buf, PAGE_SIZE - 1, "0x%x\n", addr);
> +}
> +
> +static ssize_t ast_vuart_set_addr(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;
> +
> +       err = kstrtoul(buf, 0, &val);
> +       if (err)
> +               return err;
> +

> +       writeb((val >> 8) & 0xff, vuart->regs + AST_VUART_ADDRH);
> +       writeb((val >> 0) & 0xff, vuart->regs + AST_VUART_ADDRL);

Useless & 0xff.

> +
> +       return count;
> +}

> +}
> +
> +

Extra empty line.

> +/**

If you are going to use kernel doc do it in accordance with howto.

> + * The device tree parsing code here is heavily based on that of the of_serial
> + * driver, but we have a few core differences, as we need to use our own
> + * ioremapping for extra register support
> + */

> +static int ast_vuart_probe(struct platform_device *pdev)
> +{
> +       struct uart_8250_port port;
> +       struct resource resource;
> +       struct ast_vuart *vuart;
> +       struct device_node *np;
> +       u32 clk, prop;
> +       int rc;
> +

> +       np = pdev->dev.of_node;

And if np == NULL?

> +
> +       vuart = devm_kzalloc(&pdev->dev, sizeof(*vuart), GFP_KERNEL);
> +       if (!vuart)
> +               return -ENOMEM;
> +
> +       vuart->pdev = pdev;


> +       rc = of_address_to_resource(np, 0, &resource);
> +       if (rc) {
> +               dev_warn(&pdev->dev, "invalid address\n");
> +               return rc;
> +       }
> +
> +       /* create our own mapping for VUART-specific registers */
> +       vuart->regs = devm_ioremap_resource(&pdev->dev, &resource);
> +       if (IS_ERR(vuart->regs)) {
> +               dev_warn(&pdev->dev, "failed to map registers\n");
> +               return PTR_ERR(vuart->regs);
> +       }

Can you use platform_get-resource() + devm_ioremap_resource() ?
If no, why?

> +
> +       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);
> +               if (IS_ERR(vuart->clk)) {
> +                       dev_warn(&pdev->dev,
> +                               "clk or clock-frequency not defined\n");
> +                       return PTR_ERR(vuart->clk);
> +               }
> +
> +               rc = clk_prepare_enable(vuart->clk);
> +               if (rc < 0)
> +                       return rc;
> +
> +               clk = clk_get_rate(vuart->clk);
> +       }
> +

> +       /* If current-speed was set, then try not to change it. */
> +       if (of_property_read_u32(np, "current-speed", &prop) == 0)
> +               port.port.custom_divisor = clk / (16 * prop);
> +
> +       /* Check for shifted address mapping */
> +       if (of_property_read_u32(np, "reg-offset", &prop) == 0)
> +               port.port.mapbase += prop;
> +
> +       /* Check for registers offset within the devices address range */
> +       if (of_property_read_u32(np, "reg-shift", &prop) == 0)
> +               port.port.regshift = prop;
> +
> +       /* Check for fifo size */
> +       if (of_property_read_u32(np, "fifo-size", &prop) == 0)
> +               port.port.fifosize = prop;

Perhaps you need other way around, check for error and supply a
default in such case.

> +
> +       /* Check for a fixed line number */
> +       rc = of_alias_get_id(np, "serial");
> +       if (rc >= 0)
> +               port.port.line = rc;
> +
> +       port.port.irq = irq_of_parse_and_map(np, 0);

> +       port.port.irqflags = IRQF_SHARED;

This is set by core. You already supplied correct flag for that below.

> +       port.port.iotype = UPIO_MEM;
> +       if (of_property_read_u32(np, "reg-io-width", &prop) == 0) {

You hide an error code from of_property_read_u32() here. Why?

And if there is an error you are continuing with what? 0?

> +               switch (prop) {
> +               case 1:
> +                       port.port.iotype = UPIO_MEM;
> +                       break;
> +               case 4:
> +                       port.port.iotype = of_device_is_big_endian(np) ?
> +                                      UPIO_MEM32BE : UPIO_MEM32;
> +                       break;
> +               default:
> +                       dev_warn(&pdev->dev, "unsupported reg-io-width (%d)\n",
> +                                prop);
> +                       rc = -EINVAL;
> +                       goto err_clk_disable;
> +               }
> +       }
> +
> +       port.port.type = PORT_16550A;
> +       port.port.uartclk = clk;
> +       port.port.flags = UPF_SHARE_IRQ | UPF_BOOT_AUTOCONF
> +               | UPF_FIXED_PORT | UPF_FIXED_TYPE | UPF_NO_THRE_TEST;
> +

> +       if (of_find_property(np, "no-loopback-test", NULL))
> +               port.port.flags |= UPF_SKIP_TEST;

You perhaps meant _read_bool() for sake of consistency.

> +
> +       port.port.dev = &pdev->dev;
> +
> +       if (port.port.fifosize)
> +               port.capabilities = UART_CAP_FIFO;
> +

> +       if (of_property_read_bool(pdev->dev.of_node,
> +                                 "auto-flow-control"))

One line?

> +               port.capabilities |= UART_CAP_AFE;
> +
> +       rc = serial8250_register_8250_port(&port);
> +       if (rc < 0)
> +               goto err_clk_disable;
> +
> +
> +       vuart->line = rc;
> +       ast_vuart_set_enabled(vuart, true);
> +       ast_vuart_set_host_tx_discard(vuart, true);
> +       platform_set_drvdata(pdev, vuart);
> +
> +       /* extra sysfs control */
> +       rc = device_create_file(&pdev->dev, &dev_attr_lpc_address);
> +       if (rc)
> +               dev_warn(&pdev->dev, "can't create lpc_address file\n");
> +       rc = device_create_file(&pdev->dev, &dev_attr_sirq);
> +       if (rc)
> +               dev_warn(&pdev->dev, "can't create sirq file\n");
> +
> +       return 0;
> +
> +err_clk_disable:
> +       if (vuart->clk)
> +               clk_disable_unprepare(vuart->clk);
> +
> +       irq_dispose_mapping(port.port.irq);
> +       return rc;
> +}
> +
> +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;
> +}

-- 
With Best Regards,
Andy Shevchenk
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] drivers/serial: Add driver for Aspeed virtual UART

[Benjamin Herrenschmidt]

On Sun, 2017-04-02 at 16:07 +0300, Andy Shevchenko wrote:
> > +       port.port.irqflags = IRQF_SHARED;
> 
> This is set by core. You already supplied correct flag for that
> below.

Are you sure ? Where ?

We had a bug the other day because that wasn't being set...

Cheers,
Ben.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] drivers/serial: Add driver for Aspeed virtual UART

[Joel Stanley]

Hi Andy,

Thanks for the review. I've incorporatd most of your comments in a v2
that I'll send out once I've given it a spin on hardware.

On Sun, Apr 2, 2017 at 10:37 PM, Andy Shevchenko
<andy.shevchenko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> +static ssize_t ast_vuart_show_addr(struct device *dev,
>> +               struct device_attribute *attr, char *buf)
>> +{
>> +       struct ast_vuart *vuart = dev_get_drvdata(dev);
>> +       u16 addr;
>> +
>
>> +       addr = (readb(vuart->regs + AST_VUART_ADDRH) << 8) |
>> +               (readb(vuart->regs + AST_VUART_ADDRL));
>
> It looks like you have register shift 2 bits and byte accessors. We
> have some helpers for that (serial_in() / serial_out() or alike).

Thanks for the pointer. I took a look at this. It looks like I need to
define my own accessor?

I don't think it's worth it for the one read and one write we have in
this driver.

>
>> +
>> +       return snprintf(buf, PAGE_SIZE - 1, "0x%x\n", addr);
>> +}
>> +

>> +static int ast_vuart_probe(struct platform_device *pdev)
>> +{
>> +       struct uart_8250_port port;
>> +       struct resource resource;
>> +       struct ast_vuart *vuart;
>> +       struct device_node *np;
>> +       u32 clk, prop;
>> +       int rc;
>> +
>
>> +       np = pdev->dev.of_node;
>
> And if np == NULL?

The driver will fail to probe due to the of_property_read_u32 calls
returning an error.

>> +       /* If current-speed was set, then try not to change it. */
>> +       if (of_property_read_u32(np, "current-speed", &prop) == 0)
>> +               port.port.custom_divisor = clk / (16 * prop);
>> +
>> +       /* Check for shifted address mapping */
>> +       if (of_property_read_u32(np, "reg-offset", &prop) == 0)
>> +               port.port.mapbase += prop;
>> +
>> +       /* Check for registers offset within the devices address range */
>> +       if (of_property_read_u32(np, "reg-shift", &prop) == 0)
>> +               port.port.regshift = prop;
>> +
>> +       /* Check for fifo size */
>> +       if (of_property_read_u32(np, "fifo-size", &prop) == 0)
>> +               port.port.fifosize = prop;
>
> Perhaps you need other way around, check for error and supply a
> default in such case.

We leave port.fifosize unmodified (set to zero) if there is no valid
device tree property. As the property is optional, it's not an error
if it's not present.

>> +
>> +       /* Check for a fixed line number */
>> +       rc = of_alias_get_id(np, "serial");
>> +       if (rc >= 0)
>> +               port.port.line = rc;
>> +
>> +       port.port.irq = irq_of_parse_and_map(np, 0);
>
>> +       port.port.irqflags = IRQF_SHARED;
>
> This is set by core. You already supplied correct flag for that below.

By setting UPF_SHARE_IRQ the core does correctly requset_irq with
IRQF_SHARED set. However, it does not store this in in port->irqflags,
so other tests in eg. serial8250_do_startup that test for IRQF_SHARED
will fail. This is a bug that we hit the other day.

Would you like a patch to the core that either tests for
UPF_SHARE_IRQ, or set IRQF_SHARED early on?

>
>> +       port.port.iotype = UPIO_MEM;
>> +       if (of_property_read_u32(np, "reg-io-width", &prop) == 0) {
>
> You hide an error code from of_property_read_u32() here. Why?

The property is optional, so if it doesn't exist we want to continue
without error.

We return EINVAL if the property is invalid, as the device tree code
will give us ENODATA or EOVERFLOW, which I don't think is informative
for a driver to return.

> And if there is an error you are continuing with what? 0?

we continue with port.port.iotype = UPIO_MEM from above.

>> +               switch (prop) {
>> +               case 1:
>> +                       port.port.iotype = UPIO_MEM;
>> +                       break;
>> +               case 4:
>> +                       port.port.iotype = of_device_is_big_endian(np) ?
>> +                                      UPIO_MEM32BE : UPIO_MEM32;
>> +                       break;
>> +               default:
>> +                       dev_warn(&pdev->dev, "unsupported reg-io-width (%d)\n",
>> +                                prop);
>> +                       rc = -EINVAL;
>> +                       goto err_clk_disable;
>> +               }
>> +       }

Cheers,

Joel
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] drivers/serial: Add driver for Aspeed virtual UART

[Rob Herring]

On Tue, Mar 28, 2017 at 04:14:58PM +1030, Joel Stanley wrote:
> From: Jeremy Kerr <jk@ozlabs.org>
> 
> This change adds a driver for the 16550-based Aspeed virtual UART
> device. We use a similar process to the of_serial driver for device
> probe, but expose some VUART-specific functions through sysfs too.
> 
> OpenPOWER host firmware doesn't like it when the host-side of the
> VUART's FIFO is not drained. This driver only disables host TX discard
> mode when the port is in use. We 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>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
>  Documentation/devicetree/bindings/serial/8250.txt |   2 +

For the binding:

Acked-by: Rob Herring <robh@kernel.org>

>  drivers/tty/serial/Kconfig                        |  10 +
>  drivers/tty/serial/Makefile                       |   1 +
>  drivers/tty/serial/aspeed-vuart.c                 | 335 ++++++++++++++++++++++
>  4 files changed, 348 insertions(+)
>  create mode 100644 drivers/tty/serial/aspeed-vuart.c