[PATCH 00/10] drivers: PL011: add ARM SBSA Generic UART support

[PATCH 00/10] drivers: PL011: add ARM SBSA Generic UART support

[Andre Przywara]

The ARM Server Base System Architecture[1] document describes a
generic UART which is a subset of the PL011 UART.
It lacks DMA support, baud rate control and modem status line
control, among other things.
The idea is to move the UART initialization and setup into the
firmware (which does this job today already) and let the kernel just
use the UART for sending and receiving characters.

This patchset integrates support for this UART subset into the
existing PL011 driver - basically by refactoring some
functions and providing a new uart_ops structure for it. It also has
a separate probe function to be not dependent on AMBA/PrimeCell.
It provides a device tree binding, but can easily be adapted to other
device configuration systems.
Beside the obvious effect of code sharing reusing most of the PL011
code has the advantage of not introducing another serial device
prefix, so it can go with ttyAMA, which seems to be pretty common.

This series relies on Dave's recent PL011 fix[2], which gets rid of
the loopback trick to get the UART going. There is a repo at [3]
(branch sbsa-uart/v1), which has this patch already integrated.

Patch 1/10 contains a bug fix which applies to the PL011 part also,
it should be considered regardless of the rest of the series.
Patch 2-7 refactor some PL011 functions by splitting them up into
smaller pieces, so that most of the code can be reused later by the
SBSA part.
Patch 8 and 9 introduce two new properties for the vendor structure,
this is for SBSA functionality which cannot be controlled by
separate uart_ops members only.
Patch 10 then finally drops in the SBSA specific code, by providing
a new uart_ops, vendor struct and probe function for it. Also the new
device tree binding is documented.

For testing you should be able to take any hardware which has a PL011
and change the DT to use a "arm,sbsa-uart" compatible string.
Of course testing with a real SBSA Generic UART is welcomed!

Cheers,
Andre

[1] ARM-DEN-0029 Server Base System Architecture, available (click-
    thru...) from http://infocenter.arm.com
[2] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/315248.html
[3] http://www.linux-arm.org/git?p=linux-ap.git
    git://linux-arm.org/linux-ap.git

Andre Przywara (10):
  drivers: PL011: avoid potential unregister_driver call
  drivers: PL011: refactor pl011_startup()
  drivers: PL011: refactor pl011_shutdown()
  drivers: PL011: refactor pl011_set_termios()
  drivers: PL011: refactor pl011_probe()
  drivers: PL011: replace UART_MIS reading with _RIS & _IMSC
  drivers: PL011: move cts_event workaround into separate function
  drivers: PL011: allow avoiding UART enabling/disabling
  drivers: PL011: allow to supply fixed option string
  drivers: PL011: add support for the ARM SBSA generic UART

 .../devicetree/bindings/serial/arm_sbsa_uart.txt   |    9 +
 drivers/tty/serial/amba-pl011.c                    |  510 ++++++++++++++------
 2 files changed, 381 insertions(+), 138 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt

-- 
1.7.9.5

[PATCH 01/10] drivers: PL011: avoid potential unregister_driver call

[Andre Przywara]

Although we care about not unregistering the driver if there are
still ports connected during the .remove callback, we do miss this
check in the pl011_probe function. So if the current port allocation
fails, but there are other ports already registered, we will kill
those.
So factor out the port removal into a separate function and use that
in the probe function, too.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/tty/serial/amba-pl011.c |   38 +++++++++++++++++++++-----------------
 1 file changed, 21 insertions(+), 17 deletions(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index eb397c7..dbd3a21 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -2166,6 +2166,24 @@ static int pl011_probe_dt_alias(int index, struct device *dev)
 	return ret;
 }
 
+/* unregisters the driver also if no more ports are left */
+static void pl011_unregister_port(struct uart_amba_port *uap)
+{
+	int i;
+	bool busy = false;
+
+	for (i = 0; i < ARRAY_SIZE(amba_ports); i++) {
+		if (amba_ports[i] == uap)
+			amba_ports[i] = NULL;
+		else if (amba_ports[i])
+			busy = true;
+	}
+	pl011_dma_remove(uap);
+	if (!busy)
+		uart_unregister_driver(&amba_reg);
+}
+
+
 static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
 {
 	struct uart_amba_port *uap;
@@ -2231,11 +2249,8 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
 	}
 
 	ret = uart_add_one_port(&amba_reg, &uap->port);
-	if (ret) {
-		amba_ports[i] = NULL;
-		uart_unregister_driver(&amba_reg);
-		pl011_dma_remove(uap);
-	}
+	if (ret)
+		pl011_unregister_port(uap);
 
 	return ret;
 }
@@ -2243,20 +2258,9 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
 static int pl011_remove(struct amba_device *dev)
 {
 	struct uart_amba_port *uap = amba_get_drvdata(dev);
-	bool busy = false;
-	int i;
 
 	uart_remove_one_port(&amba_reg, &uap->port);
-
-	for (i = 0; i < ARRAY_SIZE(amba_ports); i++)
-		if (amba_ports[i] == uap)
-			amba_ports[i] = NULL;
-		else if (amba_ports[i])
-			busy = true;
-
-	pl011_dma_remove(uap);
-	if (!busy)
-		uart_unregister_driver(&amba_reg);
+	pl011_unregister_port(uap);
 	return 0;
 }
 
-- 
1.7.9.5

[PATCH 02/10] drivers: PL011: refactor pl011_startup()

[Andre Przywara]

Split the pl011_startup() function into smaller chunks to allow
easier reuse later when adding SBSA support.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/tty/serial/amba-pl011.c |   48 +++++++++++++++++++++++----------------
 1 file changed, 28 insertions(+), 20 deletions(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index dbd3a21..4be4c19 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1590,6 +1590,32 @@ static void pl011_write_lcr_h(struct uart_amba_port *uap, unsigned int lcr_h)
 	}
 }
 
+static int pl011_allocate_irq(struct uart_amba_port *uap)
+{
+	writew(uap->im, uap->port.membase + UART011_IMSC);
+
+	return request_irq(uap->port.irq, pl011_int, 0, "uart-pl011", uap);
+}
+
+/*
+ * Enable interrupts, only timeouts when using DMA
+ * if initial RX DMA job failed, start in interrupt mode
+ * as well.
+ */
+static void pl011_enable_interrupts(struct uart_amba_port *uap)
+{
+	spin_lock_irq(&uap->port.lock);
+
+	/* Clear out any spuriously appearing RX interrupts */
+	writew(UART011_RTIS | UART011_RXIS,
+		uap->port.membase + UART011_ICR);
+	uap->im = UART011_RTIM;
+	if (!pl011_dma_rx_running(uap))
+		uap->im |= UART011_RXIM;
+	writew(uap->im, uap->port.membase + UART011_IMSC);
+	spin_unlock_irq(&uap->port.lock);
+}
+
 static int pl011_startup(struct uart_port *port)
 {
 	struct uart_amba_port *uap =
@@ -1601,12 +1627,7 @@ static int pl011_startup(struct uart_port *port)
 	if (retval)
 		goto clk_dis;
 
-	writew(uap->im, uap->port.membase + UART011_IMSC);
-
-	/*
-	 * Allocate the IRQ
-	 */
-	retval = request_irq(uap->port.irq, pl011_int, 0, "uart-pl011", uap);
+	retval = pl011_allocate_irq(uap);
 	if (retval)
 		goto clk_dis;
 
@@ -1626,20 +1647,7 @@ static int pl011_startup(struct uart_port *port)
 	/* Startup DMA */
 	pl011_dma_startup(uap);
 
-	/*
-	 * Finally, enable interrupts, only timeouts when using DMA
-	 * if initial RX DMA job failed, start in interrupt mode
-	 * as well.
-	 */
-	spin_lock_irq(&uap->port.lock);
-	/* Clear out any spuriously appearing RX interrupts */
-	 writew(UART011_RTIS | UART011_RXIS,
-		uap->port.membase + UART011_ICR);
-	uap->im = UART011_RTIM;
-	if (!pl011_dma_rx_running(uap))
-		uap->im |= UART011_RXIM;
-	writew(uap->im, uap->port.membase + UART011_IMSC);
-	spin_unlock_irq(&uap->port.lock);
+	pl011_enable_interrupts(uap);
 
 	return 0;
 
-- 
1.7.9.5

[PATCH 03/10] drivers: PL011: refactor pl011_shutdown()

[Andre Przywara]

Split the pl011_shutdown() function into smaller chunks to allow
easier reuse later when adding SBSA support.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/tty/serial/amba-pl011.c |   59 ++++++++++++++++++++++-----------------
 1 file changed, 34 insertions(+), 25 deletions(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 4be4c19..7373060 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1666,34 +1666,15 @@ static void pl011_shutdown_channel(struct uart_amba_port *uap,
       writew(val, uap->port.membase + lcrh);
 }
 
-static void pl011_shutdown(struct uart_port *port)
+/*
+ * disable the port. It should not disable RTS and DTR.
+ * Also RTS and DTR state should be preserved to restore
+ * it during startup().
+ */
+static void pl011_disable_uart(struct uart_amba_port *uap)
 {
-	struct uart_amba_port *uap =
-	    container_of(port, struct uart_amba_port, port);
 	unsigned int cr;
 
-	/*
-	 * disable all interrupts
-	 */
-	spin_lock_irq(&uap->port.lock);
-	uap->im = 0;
-	writew(uap->im, uap->port.membase + UART011_IMSC);
-	writew(0xffff, uap->port.membase + UART011_ICR);
-	spin_unlock_irq(&uap->port.lock);
-
-	pl011_dma_shutdown(uap);
-
-	/*
-	 * Free the interrupt
-	 */
-	free_irq(uap->port.irq, uap);
-
-	/*
-	 * disable the port
-	 * disable the port. It should not disable RTS and DTR.
-	 * Also RTS and DTR state should be preserved to restore
-	 * it during startup().
-	 */
 	uap->autorts = false;
 	spin_lock_irq(&uap->port.lock);
 	cr = readw(uap->port.membase + UART011_CR);
@@ -1709,6 +1690,34 @@ static void pl011_shutdown(struct uart_port *port)
 	pl011_shutdown_channel(uap, uap->lcrh_rx);
 	if (uap->lcrh_rx != uap->lcrh_tx)
 		pl011_shutdown_channel(uap, uap->lcrh_tx);
+}
+
+static void pl011_disable_interrupts(struct uart_amba_port *uap)
+{
+	spin_lock_irq(&uap->port.lock);
+
+	/*
+	 * mask all interrupts and clear all pending ones
+	 */
+	uap->im = 0;
+	writew(uap->im, uap->port.membase + UART011_IMSC);
+	writew(0xffff, uap->port.membase + UART011_ICR);
+
+	spin_unlock_irq(&uap->port.lock);
+}
+
+static void pl011_shutdown(struct uart_port *port)
+{
+	struct uart_amba_port *uap =
+		container_of(port, struct uart_amba_port, port);
+
+	pl011_disable_interrupts(uap);
+
+	pl011_dma_shutdown(uap);
+
+	free_irq(uap->port.irq, uap);
+
+	pl011_disable_uart(uap);
 
 	/*
 	 * Shut down the clock producer
-- 
1.7.9.5

[PATCH 04/10] drivers: PL011: refactor pl011_set_termios()

[Andre Przywara]

Split the pl011_set_termios() function into smaller chunks to allow
easier reuse later when adding SBSA support.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/tty/serial/amba-pl011.c |   60 +++++++++++++++++++++------------------
 1 file changed, 33 insertions(+), 27 deletions(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 7373060..2153b98 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1739,6 +1739,38 @@ static void pl011_shutdown(struct uart_port *port)
 }
 
 static void
+pl011_setup_status_masks(struct uart_port *port, struct ktermios *termios)
+{
+	port->read_status_mask = UART011_DR_OE | 255;
+	if (termios->c_iflag & INPCK)
+		port->read_status_mask |= UART011_DR_FE | UART011_DR_PE;
+	if (termios->c_iflag & (IGNBRK | BRKINT | PARMRK))
+		port->read_status_mask |= UART011_DR_BE;
+
+	/*
+	 * Characters to ignore
+	 */
+	port->ignore_status_mask = 0;
+	if (termios->c_iflag & IGNPAR)
+		port->ignore_status_mask |= UART011_DR_FE | UART011_DR_PE;
+	if (termios->c_iflag & IGNBRK) {
+		port->ignore_status_mask |= UART011_DR_BE;
+		/*
+		 * If we're ignoring parity and break indicators,
+		 * ignore overruns too (for real raw support).
+		 */
+		if (termios->c_iflag & IGNPAR)
+			port->ignore_status_mask |= UART011_DR_OE;
+	}
+
+	/*
+	 * Ignore all characters if CREAD is not set.
+	 */
+	if ((termios->c_cflag & CREAD) == 0)
+		port->ignore_status_mask |= UART_DUMMY_DR_RX;
+}
+
+static void
 pl011_set_termios(struct uart_port *port, struct ktermios *termios,
 		     struct ktermios *old)
 {
@@ -1802,33 +1834,7 @@ pl011_set_termios(struct uart_port *port, struct ktermios *termios,
 	 */
 	uart_update_timeout(port, termios->c_cflag, baud);
 
-	port->read_status_mask = UART011_DR_OE | 255;
-	if (termios->c_iflag & INPCK)
-		port->read_status_mask |= UART011_DR_FE | UART011_DR_PE;
-	if (termios->c_iflag & (IGNBRK | BRKINT | PARMRK))
-		port->read_status_mask |= UART011_DR_BE;
-
-	/*
-	 * Characters to ignore
-	 */
-	port->ignore_status_mask = 0;
-	if (termios->c_iflag & IGNPAR)
-		port->ignore_status_mask |= UART011_DR_FE | UART011_DR_PE;
-	if (termios->c_iflag & IGNBRK) {
-		port->ignore_status_mask |= UART011_DR_BE;
-		/*
-		 * If we're ignoring parity and break indicators,
-		 * ignore overruns too (for real raw support).
-		 */
-		if (termios->c_iflag & IGNPAR)
-			port->ignore_status_mask |= UART011_DR_OE;
-	}
-
-	/*
-	 * Ignore all characters if CREAD is not set.
-	 */
-	if ((termios->c_cflag & CREAD) == 0)
-		port->ignore_status_mask |= UART_DUMMY_DR_RX;
+	pl011_setup_status_masks(port, termios);
 
 	if (UART_ENABLE_MS(port, termios->c_cflag))
 		pl011_enable_ms(port);
-- 
1.7.9.5

[PATCH 05/10] drivers: PL011: refactor pl011_probe()

[Andre Przywara]

Currently the pl011_probe() function is relying on some AMBA IDs
and a device tree node to initialize the driver and a port.
Both features are not necessarily required for the driver:
- we lack AMBA IDs in the ARM SBSA generic UART and
- we lack a DT node in ACPI systems.
So lets refactor the function to ease later reuse.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/tty/serial/amba-pl011.c |   90 ++++++++++++++++++++++++++-------------
 1 file changed, 61 insertions(+), 29 deletions(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 2153b98..e484551 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -2206,13 +2206,10 @@ static void pl011_unregister_port(struct uart_amba_port *uap)
 		uart_unregister_driver(&amba_reg);
 }
 
-
-static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
+static int pl011_allocate_port(struct device *dev, struct uart_amba_port **_uap)
 {
 	struct uart_amba_port *uap;
-	struct vendor_data *vendor = id->data;
-	void __iomem *base;
-	int i, ret;
+	int i;
 
 	for (i = 0; i < ARRAY_SIZE(amba_ports); i++)
 		if (amba_ports[i] == NULL)
@@ -2221,47 +2218,50 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
 	if (i == ARRAY_SIZE(amba_ports))
 		return -EBUSY;
 
-	uap = devm_kzalloc(&dev->dev, sizeof(struct uart_amba_port),
-			   GFP_KERNEL);
+	uap = devm_kzalloc(dev, sizeof(struct uart_amba_port), GFP_KERNEL);
 	if (uap == NULL)
 		return -ENOMEM;
 
-	i = pl011_probe_dt_alias(i, &dev->dev);
+	if (_uap)
+		*_uap = uap;
+
+	return i;
+}
 
-	base = devm_ioremap(&dev->dev, dev->res.start,
-			    resource_size(&dev->res));
+static int pl011_setup_port(struct device *dev, struct uart_amba_port *uap,
+			    struct resource *mmiobase, int index)
+{
+	void __iomem *base;
+
+	base = devm_ioremap_resource(dev, mmiobase);
 	if (!base)
 		return -ENOMEM;
 
-	uap->clk = devm_clk_get(&dev->dev, NULL);
-	if (IS_ERR(uap->clk))
-		return PTR_ERR(uap->clk);
+	index = pl011_probe_dt_alias(index, dev);
 
-	uap->vendor = vendor;
-	uap->lcrh_rx = vendor->lcrh_rx;
-	uap->lcrh_tx = vendor->lcrh_tx;
 	uap->old_cr = 0;
-	uap->fifosize = vendor->get_fifosize(dev);
-	uap->port.dev = &dev->dev;
-	uap->port.mapbase = dev->res.start;
+	uap->port.dev = dev;
+	uap->port.mapbase = mmiobase->start;
 	uap->port.membase = base;
 	uap->port.iotype = UPIO_MEM;
-	uap->port.irq = dev->irq[0];
 	uap->port.fifosize = uap->fifosize;
-	uap->port.ops = &amba_pl011_pops;
 	uap->port.flags = UPF_BOOT_AUTOCONF;
-	uap->port.line = i;
-	pl011_dma_probe(&dev->dev, uap);
+	uap->port.line = index;
 
-	/* Ensure interrupts from this UART are masked and cleared */
-	writew(0, uap->port.membase + UART011_IMSC);
-	writew(0xffff, uap->port.membase + UART011_ICR);
+	pl011_dma_probe(dev, uap);
 
-	snprintf(uap->type, sizeof(uap->type), "PL011 rev%u", amba_rev(dev));
+	amba_ports[index] = uap;
 
-	amba_ports[i] = uap;
+	return 0;
+}
 
-	amba_set_drvdata(dev, uap);
+static int pl011_register_port(struct uart_amba_port *uap)
+{
+	int ret;
+
+	/* Ensure interrupts from this UART are masked and cleared */
+	writew(0, uap->port.membase + UART011_IMSC);
+	writew(0xffff, uap->port.membase + UART011_ICR);
 
 	if (!amba_reg.state) {
 		ret = uart_register_driver(&amba_reg);
@@ -2278,6 +2278,38 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
 	return ret;
 }
 
+static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
+{
+	struct uart_amba_port *uap;
+	struct vendor_data *vendor = id->data;
+	int portnr, ret;
+
+	portnr = pl011_allocate_port(&dev->dev, &uap);
+	if (portnr < 0)
+		return portnr;
+
+	uap->clk = devm_clk_get(&dev->dev, NULL);
+	if (IS_ERR(uap->clk))
+		return PTR_ERR(uap->clk);
+
+	uap->vendor = vendor;
+	uap->lcrh_rx = vendor->lcrh_rx;
+	uap->lcrh_tx = vendor->lcrh_tx;
+	uap->fifosize = vendor->get_fifosize(dev);
+	uap->port.irq = dev->irq[0];
+	uap->port.ops = &amba_pl011_pops;
+
+	snprintf(uap->type, sizeof(uap->type), "PL011 rev%u", amba_rev(dev));
+
+	ret = pl011_setup_port(&dev->dev, uap, &dev->res, portnr);
+	if (ret)
+		return ret;
+
+	amba_set_drvdata(dev, uap);
+
+	return pl011_register_port(uap);
+}
+
 static int pl011_remove(struct amba_device *dev)
 {
 	struct uart_amba_port *uap = amba_get_drvdata(dev);
-- 
1.7.9.5

[PATCH 06/10] drivers: PL011: replace UART_MIS reading with _RIS & _IMSC

[Andre Przywara]

The PL011 register UART_MIS is actually a bitwise AND of the
UART_RIS and the UART_MISC register.
Since the SBSA UART does not include the _MIS register, use the
two separate registers to get the same behaviour. Since we are
inside the spinlock and we read the _IMSC register only once, there
should be no race issue.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/tty/serial/amba-pl011.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index e484551..e6c2746 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1350,11 +1350,13 @@ static irqreturn_t pl011_int(int irq, void *dev_id)
 	struct uart_amba_port *uap = dev_id;
 	unsigned long flags;
 	unsigned int status, pass_counter = AMBA_ISR_PASS_LIMIT;
+	u16 imsc;
 	int handled = 0;
 	unsigned int dummy_read;
 
 	spin_lock_irqsave(&uap->port.lock, flags);
-	status = readw(uap->port.membase + UART011_MIS);
+	imsc = readw(uap->port.membase + UART011_IMSC);
+	status = readw(uap->port.membase + UART011_RIS) & imsc;
 	if (status) {
 		do {
 			if (uap->vendor->cts_event_workaround) {
@@ -1391,7 +1393,7 @@ static irqreturn_t pl011_int(int irq, void *dev_id)
 			if (pass_counter-- == 0)
 				break;
 
-			status = readw(uap->port.membase + UART011_MIS);
+			status = readw(uap->port.membase + UART011_RIS) & imsc;
 		} while (status != 0);
 		handled = 1;
 	}
-- 
1.7.9.5

[PATCH 07/10] drivers: PL011: move cts_event workaround into separate function

[Andre Przywara]

To avoid lines with more than 80 characters and to make the
pl011_int() function more readable, move the workaround out into a
separate function.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/tty/serial/amba-pl011.c |   33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index e6c2746..b36dc68 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1345,6 +1345,25 @@ static void pl011_modem_status(struct uart_amba_port *uap)
 	wake_up_interruptible(&uap->port.state->port.delta_msr_wait);
 }
 
+static void check_apply_cts_event_workaround(struct uart_amba_port *uap)
+{
+	unsigned int dummy_read;
+
+	if (!uap->vendor->cts_event_workaround)
+		return;
+
+	/* workaround to make sure that all bits are unlocked.. */
+	writew(0x00, uap->port.membase + UART011_ICR);
+
+	/*
+	 * WA: introduce 26ns(1 uart clk) delay before W1C;
+	 * single apb access will incur 2 pclk(133.12Mhz) delay,
+	 * so add 2 dummy reads
+	 */
+	dummy_read = readw(uap->port.membase + UART011_ICR);
+	dummy_read = readw(uap->port.membase + UART011_ICR);
+}
+
 static irqreturn_t pl011_int(int irq, void *dev_id)
 {
 	struct uart_amba_port *uap = dev_id;
@@ -1352,25 +1371,13 @@ static irqreturn_t pl011_int(int irq, void *dev_id)
 	unsigned int status, pass_counter = AMBA_ISR_PASS_LIMIT;
 	u16 imsc;
 	int handled = 0;
-	unsigned int dummy_read;
 
 	spin_lock_irqsave(&uap->port.lock, flags);
 	imsc = readw(uap->port.membase + UART011_IMSC);
 	status = readw(uap->port.membase + UART011_RIS) & imsc;
 	if (status) {
 		do {
-			if (uap->vendor->cts_event_workaround) {
-				/* workaround to make sure that all bits are unlocked.. */
-				writew(0x00, uap->port.membase + UART011_ICR);
-
-				/*
-				 * WA: introduce 26ns(1 uart clk) delay before W1C;
-				 * single apb access will incur 2 pclk(133.12Mhz) delay,
-				 * so add 2 dummy reads
-				 */
-				dummy_read = readw(uap->port.membase + UART011_ICR);
-				dummy_read = readw(uap->port.membase + UART011_ICR);
-			}
+			check_apply_cts_event_workaround(uap);
 
 			writew(status & ~(UART011_TXIS|UART011_RTIS|
 					  UART011_RXIS),
-- 
1.7.9.5

[PATCH 08/10] drivers: PL011: allow avoiding UART enabling/disabling

[Andre Przywara]

The SBSA UART should not be enabled or disabled (it is always on),
and consequently the spec lacks the UART_CR register.
Add a vendor specific property to skip disabling or enabling of the
UART. This will be used later by the SBSA UART support.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/tty/serial/amba-pl011.c |   18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index b36dc68..833c399 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -78,6 +78,7 @@ struct vendor_data {
 	bool			oversampling;
 	bool			dma_threshold;
 	bool			cts_event_workaround;
+	bool			always_enabled;
 
 	unsigned int (*get_fifosize)(struct amba_device *dev);
 };
@@ -94,6 +95,7 @@ static struct vendor_data vendor_arm = {
 	.oversampling		= false,
 	.dma_threshold		= false,
 	.cts_event_workaround	= false,
+	.always_enabled		= false,
 	.get_fifosize		= get_fifosize_arm,
 };
 
@@ -109,6 +111,7 @@ static struct vendor_data vendor_st = {
 	.oversampling		= true,
 	.dma_threshold		= true,
 	.cts_event_workaround	= true,
+	.always_enabled		= false,
 	.get_fifosize		= get_fifosize_st,
 };
 
@@ -1991,7 +1994,7 @@ static void
 pl011_console_write(struct console *co, const char *s, unsigned int count)
 {
 	struct uart_amba_port *uap = amba_ports[co->index];
-	unsigned int status, old_cr, new_cr;
+	unsigned int status, old_cr = 0, new_cr;
 	unsigned long flags;
 	int locked = 1;
 
@@ -2008,10 +2011,12 @@ pl011_console_write(struct console *co, const char *s, unsigned int count)
 	/*
 	 *	First save the CR then disable the interrupts
 	 */
-	old_cr = readw(uap->port.membase + UART011_CR);
-	new_cr = old_cr & ~UART011_CR_CTSEN;
-	new_cr |= UART01x_CR_UARTEN | UART011_CR_TXE;
-	writew(new_cr, uap->port.membase + UART011_CR);
+	if (!uap->vendor->always_enabled) {
+		old_cr = readw(uap->port.membase + UART011_CR);
+		new_cr = old_cr & ~UART011_CR_CTSEN;
+		new_cr |= UART01x_CR_UARTEN | UART011_CR_TXE;
+		writew(new_cr, uap->port.membase + UART011_CR);
+	}
 
 	uart_console_write(&uap->port, s, count, pl011_console_putchar);
 
@@ -2022,7 +2027,8 @@ pl011_console_write(struct console *co, const char *s, unsigned int count)
 	do {
 		status = readw(uap->port.membase + UART01x_FR);
 	} while (status & UART01x_FR_BUSY);
-	writew(old_cr, uap->port.membase + UART011_CR);
+	if (!uap->vendor->always_enabled)
+		writew(old_cr, uap->port.membase + UART011_CR);
 
 	if (locked)
 		spin_unlock(&uap->port.lock);
-- 
1.7.9.5

[PATCH 09/10] drivers: PL011: allow to supply fixed option string

[Andre Przywara]

The SBSA UART has a fixed baud rate and flow control setting, which
cannot be changed or queried by software.
Add a vendor specific property to always return fixed values when
trying to read the console options.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/tty/serial/amba-pl011.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 833c399..a1c929f 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -79,6 +79,7 @@ struct vendor_data {
 	bool			dma_threshold;
 	bool			cts_event_workaround;
 	bool			always_enabled;
+	char			*fixed_options;
 
 	unsigned int (*get_fifosize)(struct amba_device *dev);
 };
@@ -96,6 +97,7 @@ static struct vendor_data vendor_arm = {
 	.dma_threshold		= false,
 	.cts_event_workaround	= false,
 	.always_enabled		= false,
+	.fixed_options		= NULL,
 	.get_fifosize		= get_fifosize_arm,
 };
 
@@ -112,6 +114,7 @@ static struct vendor_data vendor_st = {
 	.dma_threshold		= true,
 	.cts_event_workaround	= true,
 	.always_enabled		= false,
+	.fixed_options		= NULL,
 	.get_fifosize		= get_fifosize_st,
 };
 
@@ -2109,6 +2112,9 @@ static int __init pl011_console_setup(struct console *co, char *options)
 
 	uap->port.uartclk = clk_get_rate(uap->clk);
 
+	if (!options && uap->vendor->fixed_options)
+		options = uap->vendor->fixed_options;
+
 	if (options)
 		uart_parse_options(options, &baud, &parity, &bits, &flow);
 	else
-- 
1.7.9.5

[PATCH 10/10] drivers: PL011: add support for the ARM SBSA generic UART

[Andre Przywara]

The ARM Server Base System Architecture[1] document describes a
generic UART which is a subset of the PL011 UART.
It lacks DMA support, baud rate control and modem status line
control, among other things.
The idea is to move the UART initialization and setup into the
firmware (which does this job today already) and let the kernel just
use the UART for sending and receiving characters.
We use the recent refactoring the build a new struct uart_ops
variable which points to some new functions avoiding access to the
missing registers. We reuse as much existing PL011 code as possible.

In contrast to the PL011 the SBSA UART does not define any AMBA or
PrimeCell relations, so we go a pretty generic probe function
which only uses platform device functions.
A DT binding is provided, but other systems can easily attach to it,
too (hint, hint!).

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 .../devicetree/bindings/serial/arm_sbsa_uart.txt   |    9 ++
 drivers/tty/serial/amba-pl011.c                    |  154 ++++++++++++++++++++
 2 files changed, 163 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt

diff --git a/Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt b/Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt
new file mode 100644
index 0000000..21d211f
--- /dev/null
+++ b/Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt
@@ -0,0 +1,9 @@
+* ARM SBSA defined generic UART
+This UART uses a subset of the PL011 registers and consequently lives
+in the PL011 driver. It's baudrate and other communication parameters
+cannot be adjusted at runtime, so it lacks a clock specifier here.
+
+Required properties:
+- compatible: must be "arm,sbsa-uart"
+- reg: exactly one register range
+- interrupts: exactly one interrupt specifier
diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index a1c929f..596e641 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -101,6 +101,14 @@ static struct vendor_data vendor_arm = {
 	.get_fifosize		= get_fifosize_arm,
 };
 
+static struct vendor_data vendor_sbsa = {
+	.oversampling		= false,
+	.dma_threshold		= false,
+	.cts_event_workaround	= false,
+	.always_enabled		= true,
+	.fixed_options		= "115200n8",
+};
+
 static unsigned int get_fifosize_st(struct amba_device *dev)
 {
 	return 64;
@@ -1671,6 +1679,32 @@ static int pl011_startup(struct uart_port *port)
 	return retval;
 }
 
+static int sbsa_uart_startup(struct uart_port *port)
+{
+	struct uart_amba_port *uap =
+		container_of(port, struct uart_amba_port, port);
+	int retval;
+
+	retval = pl011_hwinit(port);
+	if (retval)
+		return retval;
+
+	retval = pl011_allocate_irq(uap);
+	if (retval)
+		return retval;
+
+	uap->tx_avail = uap->fifosize; /* FIFO should be empty */
+
+	/*
+	 * The SBSA UART does not support any modem status lines.
+	 */
+	uap->old_status = 0;
+
+	pl011_enable_interrupts(uap);
+
+	return 0;
+}
+
 static void pl011_shutdown_channel(struct uart_amba_port *uap,
 					unsigned int lcrh)
 {
@@ -1753,6 +1787,19 @@ static void pl011_shutdown(struct uart_port *port)
 		uap->port.ops->flush_buffer(port);
 }
 
+static void sbsa_uart_shutdown(struct uart_port *port)
+{
+	struct uart_amba_port *uap =
+		container_of(port, struct uart_amba_port, port);
+
+	pl011_disable_interrupts(uap);
+
+	free_irq(uap->port.irq, uap);
+
+	if (uap->port.ops->flush_buffer)
+		uap->port.ops->flush_buffer(port);
+}
+
 static void
 pl011_setup_status_masks(struct uart_port *port, struct ktermios *termios)
 {
@@ -1904,6 +1951,26 @@ pl011_set_termios(struct uart_port *port, struct ktermios *termios,
 	spin_unlock_irqrestore(&port->lock, flags);
 }
 
+static void
+sbsa_uart_set_termios(struct uart_port *port, struct ktermios *termios,
+		      struct ktermios *old)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&port->lock, flags);
+
+	/*
+	 * We don't know the clock or baud rate of the UART, so we do
+	 * some kind of worst case assumption here. The code adds a
+	 * 20ms buffer anyway, so going with 19200 should be fine.
+	 */
+	uart_update_timeout(port, CS8, 19200);
+
+	pl011_setup_status_masks(port, termios);
+
+	spin_unlock_irqrestore(&port->lock, flags);
+}
+
 static const char *pl011_type(struct uart_port *port)
 {
 	struct uart_amba_port *uap =
@@ -1979,6 +2046,40 @@ static struct uart_ops amba_pl011_pops = {
 #endif
 };
 
+static void sbsa_uart_set_mctrl(struct uart_port *port, unsigned int mctrl)
+{
+}
+
+static unsigned int sbsa_uart_get_mctrl(struct uart_port *port)
+{
+	return 0;
+}
+
+static struct uart_ops sbsa_uart_pops = {
+	.tx_empty	= pl011_tx_empty,
+	.set_mctrl	= sbsa_uart_set_mctrl,
+	.get_mctrl	= sbsa_uart_get_mctrl,
+	.stop_tx	= pl011_stop_tx,
+	.start_tx	= pl011_start_tx,
+	.stop_rx	= pl011_stop_rx,
+	.enable_ms	= NULL,
+	.break_ctl	= NULL,
+	.startup	= sbsa_uart_startup,
+	.shutdown	= sbsa_uart_shutdown,
+	.flush_buffer	= NULL,
+	.set_termios	= sbsa_uart_set_termios,
+	.type		= pl011_type,
+	.release_port	= pl011_release_port,
+	.request_port	= pl011_request_port,
+	.config_port	= pl011_config_port,
+	.verify_port	= pl011_verify_port,
+#ifdef CONFIG_CONSOLE_POLL
+	.poll_init     = pl011_hwinit,
+	.poll_get_char = pl011_get_poll_char,
+	.poll_put_char = pl011_put_poll_char,
+#endif
+};
+
 static struct uart_amba_port *amba_ports[UART_NR];
 
 #ifdef CONFIG_SERIAL_AMBA_PL011_CONSOLE
@@ -2364,6 +2465,59 @@ static int pl011_resume(struct device *dev)
 
 static SIMPLE_DEV_PM_OPS(pl011_dev_pm_ops, pl011_suspend, pl011_resume);
 
+static int sbsa_uart_probe(struct platform_device *pdev)
+{
+	struct uart_amba_port *uap;
+	struct resource *r;
+	int portnr, ret;
+
+	portnr = pl011_allocate_port(&pdev->dev, &uap);
+	if (portnr < 0)
+		return portnr;
+
+	uap->vendor	= &vendor_sbsa;
+	uap->fifosize	= 32;
+	uap->port.irq	= platform_get_irq(pdev, 0);
+	uap->port.ops	= &sbsa_uart_pops;
+
+	snprintf(uap->type, sizeof(uap->type), "SBSA");
+
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	ret = pl011_setup_port(&pdev->dev, uap, r, portnr);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, uap);
+
+	return pl011_register_port(uap);
+}
+
+static int sbsa_uart_remove(struct platform_device *pdev)
+{
+	struct uart_amba_port *uap = platform_get_drvdata(pdev);
+
+	uart_remove_one_port(&amba_reg, &uap->port);
+	pl011_unregister_port(uap);
+	return 0;
+}
+
+static const struct of_device_id sbsa_uart_match[] = {
+	{ .compatible = "arm,sbsa-uart", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, sbsa_uart_match);
+
+static struct platform_driver arm_sbsa_uart_platform_driver = {
+	.probe		= sbsa_uart_probe,
+	.remove		= sbsa_uart_remove,
+	.driver	= {
+		.name	= "sbsa-uart",
+		.of_match_table = of_match_ptr(sbsa_uart_match),
+	},
+};
+
+module_platform_driver(arm_sbsa_uart_platform_driver);
+
 static struct amba_id pl011_ids[] = {
 	{
 		.id	= 0x00041011,
-- 
1.7.9.5

Re: [PATCH 00/10] drivers: PL011: add ARM SBSA Generic UART support

[Arnd Bergmann]

On Friday 16 January 2015 17:22:56 Andre Przywara wrote:
> The ARM Server Base System Architecture[1] document describes a
> generic UART which is a subset of the PL011 UART.
> It lacks DMA support, baud rate control and modem status line
> control, among other things.
> The idea is to move the UART initialization and setup into the
> firmware (which does this job today already) and let the kernel just
> use the UART for sending and receiving characters.

Given that all other approaches that have been tried have failed, this
seems like the best way forward. I don't know enough about the driver
to do a detailed review, but all patches looked good to me as far as
I could tell.

One question: How does the name space and minor number allocation work
if you have both sbsa-uart and real pl011 in the same system? Do
they share the ttyAMA name space without collisions?

	Arnd

Re: [PATCH 10/10] drivers: PL011: add support for the ARM SBSA generic UART

[Mark Rutland]

On Fri, Jan 16, 2015 at 05:23:06PM +0000, Andre Przywara wrote:
> The ARM Server Base System Architecture[1] document describes a
> generic UART which is a subset of the PL011 UART.
> It lacks DMA support, baud rate control and modem status line
> control, among other things.
> The idea is to move the UART initialization and setup into the
> firmware (which does this job today already) and let the kernel just
> use the UART for sending and receiving characters.
> We use the recent refactoring the build a new struct uart_ops
> variable which points to some new functions avoiding access to the
> missing registers. We reuse as much existing PL011 code as possible.
> 
> In contrast to the PL011 the SBSA UART does not define any AMBA or
> PrimeCell relations, so we go a pretty generic probe function
> which only uses platform device functions.
> A DT binding is provided, but other systems can easily attach to it,
> too (hint, hint!).
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  .../devicetree/bindings/serial/arm_sbsa_uart.txt   |    9 ++
>  drivers/tty/serial/amba-pl011.c                    |  154 ++++++++++++++++++++
>  2 files changed, 163 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt
> 
> diff --git a/Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt b/Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt
> new file mode 100644
> index 0000000..21d211f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt
> @@ -0,0 +1,9 @@
> +* ARM SBSA defined generic UART
> +This UART uses a subset of the PL011 registers and consequently lives
> +in the PL011 driver. It's baudrate and other communication parameters
> +cannot be adjusted at runtime, so it lacks a clock specifier here.
> +
> +Required properties:
> +- compatible: must be "arm,sbsa-uart"
> +- reg: exactly one register range
> +- interrupts: exactly one interrupt specifier
> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index a1c929f..596e641 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -101,6 +101,14 @@ static struct vendor_data vendor_arm = {
>  	.get_fifosize		= get_fifosize_arm,
>  };
>  
> +static struct vendor_data vendor_sbsa = {
> +	.oversampling		= false,
> +	.dma_threshold		= false,
> +	.cts_event_workaround	= false,
> +	.always_enabled		= true,
> +	.fixed_options		= "115200n8",
> +};

Is this configuration mandated by the SBSA? If so, please mandate it in
the binding document.

If the rate and so on are not mandated, they should probably be
described by the binding so software has a chance of getting the real
configuration details.

Thanks,
Mark.

Re: [PATCH 00/10] drivers: PL011: add ARM SBSA Generic UART support

[Andre Przywara]

Hi Arnd,

On 16/01/15 17:31, Arnd Bergmann wrote:
> On Friday 16 January 2015 17:22:56 Andre Przywara wrote:
>> The ARM Server Base System Architecture[1] document describes a
>> generic UART which is a subset of the PL011 UART.
>> It lacks DMA support, baud rate control and modem status line
>> control, among other things.
>> The idea is to move the UART initialization and setup into the
>> firmware (which does this job today already) and let the kernel just
>> use the UART for sending and receiving characters.
> 
> Given that all other approaches that have been tried have failed, this
> seems like the best way forward. I don't know enough about the driver
> to do a detailed review, but all patches looked good to me as far as
> I could tell.

Thanks for looking at them!

> One question: How does the name space and minor number allocation work
> if you have both sbsa-uart and real pl011 in the same system? Do
> they share the ttyAMA name space without collisions?

Yes, that works. Both parts use the same pl011_probe_dt_alias() routine
to do the numbering.
For testing I specified the second and fourth port as SBSA in the DT,
leaving the first and third as PL011. This is what you get then:
...
Serial: AMBA PL011 UART driver
uart-pl011 1c090000.uart: ttyAMA0 at MMIO 0x1c090000 (irq = 15,
base_baud = 0) is a PL011 rev2
...
uart-pl011 1c0b0000.uart: ttyAMA2 at MMIO 0x1c0b0000 (irq = 17,
base_baud = 0) is a PL011 rev2
....
sbsa-uart 1c0a0000.uart: ttyAMA1 at MMIO 0x1c0a0000 (irq = 16, base_baud
= 0) is a SBSA
sbsa-uart 1c0c0000.uart: ttyAMA3 at MMIO 0x1c0c0000 (irq = 18, base_baud
= 0) is a SBSA

Cheers,
Andre.

Re: [PATCH 10/10] drivers: PL011: add support for the ARM SBSA generic UART

[Andre Przywara]

Hi Mark,

On 16/01/15 17:34, Mark Rutland wrote:
> On Fri, Jan 16, 2015 at 05:23:06PM +0000, Andre Przywara wrote:
>> The ARM Server Base System Architecture[1] document describes a
>> generic UART which is a subset of the PL011 UART.
>> It lacks DMA support, baud rate control and modem status line
>> control, among other things.
>> The idea is to move the UART initialization and setup into the
>> firmware (which does this job today already) and let the kernel just
>> use the UART for sending and receiving characters.
>> We use the recent refactoring the build a new struct uart_ops
>> variable which points to some new functions avoiding access to the
>> missing registers. We reuse as much existing PL011 code as possible.
>>
>> In contrast to the PL011 the SBSA UART does not define any AMBA or
>> PrimeCell relations, so we go a pretty generic probe function
>> which only uses platform device functions.
>> A DT binding is provided, but other systems can easily attach to it,
>> too (hint, hint!).
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  .../devicetree/bindings/serial/arm_sbsa_uart.txt   |    9 ++
>>  drivers/tty/serial/amba-pl011.c                    |  154 ++++++++++++++++++++
>>  2 files changed, 163 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt
>>
>> diff --git a/Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt b/Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt
>> new file mode 100644
>> index 0000000..21d211f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt
>> @@ -0,0 +1,9 @@
>> +* ARM SBSA defined generic UART
>> +This UART uses a subset of the PL011 registers and consequently lives
>> +in the PL011 driver. It's baudrate and other communication parameters
>> +cannot be adjusted at runtime, so it lacks a clock specifier here.
>> +
>> +Required properties:
>> +- compatible: must be "arm,sbsa-uart"
>> +- reg: exactly one register range
>> +- interrupts: exactly one interrupt specifier
>> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
>> index a1c929f..596e641 100644
>> --- a/drivers/tty/serial/amba-pl011.c
>> +++ b/drivers/tty/serial/amba-pl011.c
>> @@ -101,6 +101,14 @@ static struct vendor_data vendor_arm = {
>>  	.get_fifosize		= get_fifosize_arm,
>>  };
>>  
>> +static struct vendor_data vendor_sbsa = {
>> +	.oversampling		= false,
>> +	.dma_threshold		= false,
>> +	.cts_event_workaround	= false,
>> +	.always_enabled		= true,
>> +	.fixed_options		= "115200n8",
>> +};
> 
> Is this configuration mandated by the SBSA? If so, please mandate it in
> the binding document.

No, actually it is just a placeholder. The driver needs some values to
avoid querying the device and to make the upper levels happy, so I went
with those. Actually 38400 would make more sense here, since that is
some kind of Linux serial default value.

> If the rate and so on are not mandated, they should probably be
> described by the binding so software has a chance of getting the real
> configuration details.

What the actual settings are is actually totally up to the firmware. By
definition software cannot learn these settings and it shouldn't care,
as the SBSA UART is just "meant to work(TM)". Though from userland it
looks like one can change the baudrate and the other parameters, the
driver totally ignores those settings (though it reflects it back).

Cheers,
Andre.

Re: [PATCH 10/10] drivers: PL011: add support for the ARM SBSA generic UART

[Mark Rutland]

On Fri, Jan 16, 2015 at 06:07:42PM +0000, Andre Przywara wrote:
> Hi Mark,
> 
> On 16/01/15 17:34, Mark Rutland wrote:
> > On Fri, Jan 16, 2015 at 05:23:06PM +0000, Andre Przywara wrote:
> >> The ARM Server Base System Architecture[1] document describes a
> >> generic UART which is a subset of the PL011 UART.
> >> It lacks DMA support, baud rate control and modem status line
> >> control, among other things.
> >> The idea is to move the UART initialization and setup into the
> >> firmware (which does this job today already) and let the kernel just
> >> use the UART for sending and receiving characters.
> >> We use the recent refactoring the build a new struct uart_ops
> >> variable which points to some new functions avoiding access to the
> >> missing registers. We reuse as much existing PL011 code as possible.
> >>
> >> In contrast to the PL011 the SBSA UART does not define any AMBA or
> >> PrimeCell relations, so we go a pretty generic probe function
> >> which only uses platform device functions.
> >> A DT binding is provided, but other systems can easily attach to it,
> >> too (hint, hint!).
> >>
> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> >> ---
> >>  .../devicetree/bindings/serial/arm_sbsa_uart.txt   |    9 ++
> >>  drivers/tty/serial/amba-pl011.c                    |  154 ++++++++++++++++++++
> >>  2 files changed, 163 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt b/Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt
> >> new file mode 100644
> >> index 0000000..21d211f
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt
> >> @@ -0,0 +1,9 @@
> >> +* ARM SBSA defined generic UART
> >> +This UART uses a subset of the PL011 registers and consequently lives
> >> +in the PL011 driver. It's baudrate and other communication parameters
> >> +cannot be adjusted at runtime, so it lacks a clock specifier here.
> >> +
> >> +Required properties:
> >> +- compatible: must be "arm,sbsa-uart"
> >> +- reg: exactly one register range
> >> +- interrupts: exactly one interrupt specifier
> >> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> >> index a1c929f..596e641 100644
> >> --- a/drivers/tty/serial/amba-pl011.c
> >> +++ b/drivers/tty/serial/amba-pl011.c
> >> @@ -101,6 +101,14 @@ static struct vendor_data vendor_arm = {
> >>  	.get_fifosize		= get_fifosize_arm,
> >>  };
> >>  
> >> +static struct vendor_data vendor_sbsa = {
> >> +	.oversampling		= false,
> >> +	.dma_threshold		= false,
> >> +	.cts_event_workaround	= false,
> >> +	.always_enabled		= true,
> >> +	.fixed_options		= "115200n8",
> >> +};
> > 
> > Is this configuration mandated by the SBSA? If so, please mandate it in
> > the binding document.
> 
> No, actually it is just a placeholder. The driver needs some values to
> avoid querying the device and to make the upper levels happy, so I went
> with those. Actually 38400 would make more sense here, since that is
> some kind of Linux serial default value.

Please let's have the real values rather than something made up.

If I ask my UART how it's configured, I expect it to tell me the truth.
It's nice to know before I connect something to the other end of the
line.

> > If the rate and so on are not mandated, they should probably be
> > described by the binding so software has a chance of getting the real
> > configuration details.
> 
> What the actual settings are is actually totally up to the firmware. By
> definition software cannot learn these settings and it shouldn't care,
> as the SBSA UART is just "meant to work(TM)". Though from userland it
> looks like one can change the baudrate and the other parameters, the
> driver totally ignores those settings (though it reflects it back).

The fact that we cannot reconfigure it is orthogonal.

Given that all we should need is baud-rate, parity, and bits, it should
be relatively easy to describe and handle.

Mark.

Re: [PATCH 10/10] drivers: PL011: add support for the ARM SBSA generic UART

[Andre Przywara]

Hi Mark,

On 16/01/15 18:12, Mark Rutland wrote:
> On Fri, Jan 16, 2015 at 06:07:42PM +0000, Andre Przywara wrote:
>> Hi Mark,
>>
>> On 16/01/15 17:34, Mark Rutland wrote:
>>> On Fri, Jan 16, 2015 at 05:23:06PM +0000, Andre Przywara wrote:
>>>> The ARM Server Base System Architecture[1] document describes a
>>>> generic UART which is a subset of the PL011 UART.
>>>> It lacks DMA support, baud rate control and modem status line
>>>> control, among other things.
>>>> The idea is to move the UART initialization and setup into the
>>>> firmware (which does this job today already) and let the kernel just
>>>> use the UART for sending and receiving characters.
>>>> We use the recent refactoring the build a new struct uart_ops
>>>> variable which points to some new functions avoiding access to the
>>>> missing registers. We reuse as much existing PL011 code as possible.
>>>>
>>>> In contrast to the PL011 the SBSA UART does not define any AMBA or
>>>> PrimeCell relations, so we go a pretty generic probe function
>>>> which only uses platform device functions.
>>>> A DT binding is provided, but other systems can easily attach to it,
>>>> too (hint, hint!).
>>>>
>>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>>> ---
>>>>  .../devicetree/bindings/serial/arm_sbsa_uart.txt   |    9 ++
>>>>  drivers/tty/serial/amba-pl011.c                    |  154 ++++++++++++++++++++
>>>>  2 files changed, 163 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt b/Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt
>>>> new file mode 100644
>>>> index 0000000..21d211f
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt
>>>> @@ -0,0 +1,9 @@
>>>> +* ARM SBSA defined generic UART
>>>> +This UART uses a subset of the PL011 registers and consequently lives
>>>> +in the PL011 driver. It's baudrate and other communication parameters
>>>> +cannot be adjusted at runtime, so it lacks a clock specifier here.
>>>> +
>>>> +Required properties:
>>>> +- compatible: must be "arm,sbsa-uart"
>>>> +- reg: exactly one register range
>>>> +- interrupts: exactly one interrupt specifier
>>>> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
>>>> index a1c929f..596e641 100644
>>>> --- a/drivers/tty/serial/amba-pl011.c
>>>> +++ b/drivers/tty/serial/amba-pl011.c
>>>> @@ -101,6 +101,14 @@ static struct vendor_data vendor_arm = {
>>>>  	.get_fifosize		= get_fifosize_arm,
>>>>  };
>>>>  
>>>> +static struct vendor_data vendor_sbsa = {
>>>> +	.oversampling		= false,
>>>> +	.dma_threshold		= false,
>>>> +	.cts_event_workaround	= false,
>>>> +	.always_enabled		= true,
>>>> +	.fixed_options		= "115200n8",
>>>> +};
>>>
>>> Is this configuration mandated by the SBSA? If so, please mandate it in
>>> the binding document.
>>
>> No, actually it is just a placeholder. The driver needs some values to
>> avoid querying the device and to make the upper levels happy, so I went
>> with those. Actually 38400 would make more sense here, since that is
>> some kind of Linux serial default value.
> 
> Please let's have the real values rather than something made up.
> 
> If I ask my UART how it's configured, I expect it to tell me the truth.
> It's nice to know before I connect something to the other end of the
> line.

So you mean that the firmware (or the vendor) inserts the actual values
here, which the kernel and eventually userland can read?
Makes some sense, so what about:
-----------------------
Required properties:
- compatible: must be "arm,sbsa-uart"
- reg: exactly one register range
- interrupts: exactly one interrupt specifier

Optional properties:
- current-speed : the current active speed of the UART
- fifo-size : always 32 as per the SBSA specification
- word-size : the number of payload bits per word
- parity : used parity method, can be:
	"n": no parity
	"e": even parity
	"o": odd parity
	"m": always mark (logical 1)
	"s": always space (logical 0)
- stop-bits : the number of stop bits after the payload

The vendor or the firmware should set these values to match the actual
configuration of the UART. The SBSA spec does not provide ways of
changing those values.
----------------------------
While I copied the first two of the optional properties from
of-serial.txt, I made up the rest. Does they make sense?

Cheers,
Andre.

>>> If the rate and so on are not mandated, they should probably be
>>> described by the binding so software has a chance of getting the real
>>> configuration details.
>>
>> What the actual settings are is actually totally up to the firmware. By
>> definition software cannot learn these settings and it shouldn't care,
>> as the SBSA UART is just "meant to work(TM)". Though from userland it
>> looks like one can change the baudrate and the other parameters, the
>> driver totally ignores those settings (though it reflects it back).
> 
> The fact that we cannot reconfigure it is orthogonal.
> 
> Given that all we should need is baud-rate, parity, and bits, it should
> be relatively easy to describe and handle.
> 
> Mark.
> 

Re: [PATCH 10/10] drivers: PL011: add support for the ARM SBSA generic UART

[Mark Rutland]

On Fri, Jan 16, 2015 at 06:33:04PM +0000, Andre Przywara wrote:
> Hi Mark,
> 
> On 16/01/15 18:12, Mark Rutland wrote:
> > On Fri, Jan 16, 2015 at 06:07:42PM +0000, Andre Przywara wrote:
> >> Hi Mark,
> >>
> >> On 16/01/15 17:34, Mark Rutland wrote:
> >>> On Fri, Jan 16, 2015 at 05:23:06PM +0000, Andre Przywara wrote:
> >>>> The ARM Server Base System Architecture[1] document describes a
> >>>> generic UART which is a subset of the PL011 UART.
> >>>> It lacks DMA support, baud rate control and modem status line
> >>>> control, among other things.
> >>>> The idea is to move the UART initialization and setup into the
> >>>> firmware (which does this job today already) and let the kernel just
> >>>> use the UART for sending and receiving characters.
> >>>> We use the recent refactoring the build a new struct uart_ops
> >>>> variable which points to some new functions avoiding access to the
> >>>> missing registers. We reuse as much existing PL011 code as possible.
> >>>>
> >>>> In contrast to the PL011 the SBSA UART does not define any AMBA or
> >>>> PrimeCell relations, so we go a pretty generic probe function
> >>>> which only uses platform device functions.
> >>>> A DT binding is provided, but other systems can easily attach to it,
> >>>> too (hint, hint!).
> >>>>
> >>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> >>>> ---
> >>>>  .../devicetree/bindings/serial/arm_sbsa_uart.txt   |    9 ++
> >>>>  drivers/tty/serial/amba-pl011.c                    |  154 ++++++++++++++++++++
> >>>>  2 files changed, 163 insertions(+)
> >>>>  create mode 100644 Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt b/Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt
> >>>> new file mode 100644
> >>>> index 0000000..21d211f
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt
> >>>> @@ -0,0 +1,9 @@
> >>>> +* ARM SBSA defined generic UART
> >>>> +This UART uses a subset of the PL011 registers and consequently lives
> >>>> +in the PL011 driver. It's baudrate and other communication parameters
> >>>> +cannot be adjusted at runtime, so it lacks a clock specifier here.
> >>>> +
> >>>> +Required properties:
> >>>> +- compatible: must be "arm,sbsa-uart"
> >>>> +- reg: exactly one register range
> >>>> +- interrupts: exactly one interrupt specifier
> >>>> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> >>>> index a1c929f..596e641 100644
> >>>> --- a/drivers/tty/serial/amba-pl011.c
> >>>> +++ b/drivers/tty/serial/amba-pl011.c
> >>>> @@ -101,6 +101,14 @@ static struct vendor_data vendor_arm = {
> >>>>  	.get_fifosize		= get_fifosize_arm,
> >>>>  };
> >>>>  
> >>>> +static struct vendor_data vendor_sbsa = {
> >>>> +	.oversampling		= false,
> >>>> +	.dma_threshold		= false,
> >>>> +	.cts_event_workaround	= false,
> >>>> +	.always_enabled		= true,
> >>>> +	.fixed_options		= "115200n8",
> >>>> +};
> >>>
> >>> Is this configuration mandated by the SBSA? If so, please mandate it in
> >>> the binding document.
> >>
> >> No, actually it is just a placeholder. The driver needs some values to
> >> avoid querying the device and to make the upper levels happy, so I went
> >> with those. Actually 38400 would make more sense here, since that is
> >> some kind of Linux serial default value.
> > 
> > Please let's have the real values rather than something made up.
> > 
> > If I ask my UART how it's configured, I expect it to tell me the truth.
> > It's nice to know before I connect something to the other end of the
> > line.
> 
> So you mean that the firmware (or the vendor) inserts the actual values
> here, which the kernel and eventually userland can read?

Precisely.

> Makes some sense, so what about:
> -----------------------
> Required properties:
> - compatible: must be "arm,sbsa-uart"
> - reg: exactly one register range
> - interrupts: exactly one interrupt specifier
> 
> Optional properties:
> - current-speed : the current active speed of the UART
> - fifo-size : always 32 as per the SBSA specification
> - word-size : the number of payload bits per word
> - parity : used parity method, can be:
> 	"n": no parity
> 	"e": even parity
> 	"o": odd parity
> 	"m": always mark (logical 1)
> 	"s": always space (logical 0)
> - stop-bits : the number of stop bits after the payload

Generally those look fine, though someone more familiar with serial
should take a look.

We can drop fifo-size given it's fixed. Anything that we know outright
doesn't need to be described.

We might want to make them mandatory. I don't see any value in not
knowing.

Thanks,
Mark.

> 
> The vendor or the firmware should set these values to match the actual
> configuration of the UART. The SBSA spec does not provide ways of
> changing those values.
> ----------------------------
> While I copied the first two of the optional properties from
> of-serial.txt, I made up the rest. Does they make sense?
> 
> Cheers,
> Andre.
> 
> >>> If the rate and so on are not mandated, they should probably be
> >>> described by the binding so software has a chance of getting the real
> >>> configuration details.
> >>
> >> What the actual settings are is actually totally up to the firmware. By
> >> definition software cannot learn these settings and it shouldn't care,
> >> as the SBSA UART is just "meant to work(TM)". Though from userland it
> >> looks like one can change the baudrate and the other parameters, the
> >> driver totally ignores those settings (though it reflects it back).
> > 
> > The fact that we cannot reconfigure it is orthogonal.
> > 
> > Given that all we should need is baud-rate, parity, and bits, it should
> > be relatively easy to describe and handle.
> > 
> > Mark.
> > 

Re: [PATCH 10/10] drivers: PL011: add support for the ARM SBSA generic UART

[Arnd Bergmann]

On Friday 16 January 2015 18:37:17 Mark Rutland wrote:
> 
> > Makes some sense, so what about:
> > -----------------------
> > Required properties:
> > - compatible: must be "arm,sbsa-uart"
> > - reg: exactly one register range
> > - interrupts: exactly one interrupt specifier
> > 
> > Optional properties:
> > - current-speed : the current active speed of the UART
> > - fifo-size : always 32 as per the SBSA specification
> > - word-size : the number of payload bits per word
> > - parity : used parity method, can be:
> >       "n": no parity
> >       "e": even parity
> >       "o": odd parity
> >       "m": always mark (logical 1)
> >       "s": always space (logical 0)
> > - stop-bits : the number of stop bits after the payload
> 
> Generally those look fine, though someone more familiar with serial
> should take a look.
> 
> We can drop fifo-size given it's fixed. Anything that we know outright
> doesn't need to be described.
> 
> We might want to make them mandatory. I don't see any value in not
> knowing.

I think making them optional is useless, so either make them mandatory or
drop them if we can avoid it. Unfortunately, LCR is not part of SBSA,
that would let us drop the properties and still report the correct settings.

Would we be better off documenting that 8-n-1 is the only allowed settting?
I can't think of a reason why anyone would use something else.

	Arnd

Re: [PATCH 10/10] drivers: PL011: add support for the ARM SBSA generic UART

[Andre Przywara]

Hi,

On 19/01/15 13:31, Arnd Bergmann wrote:
> On Friday 16 January 2015 18:37:17 Mark Rutland wrote:
>>
>>> Makes some sense, so what about:
>>> -----------------------
>>> Required properties:
>>> - compatible: must be "arm,sbsa-uart"
>>> - reg: exactly one register range
>>> - interrupts: exactly one interrupt specifier
>>>
>>> Optional properties:
>>> - current-speed : the current active speed of the UART
>>> - fifo-size : always 32 as per the SBSA specification
>>> - word-size : the number of payload bits per word
>>> - parity : used parity method, can be:
>>>       "n": no parity
>>>       "e": even parity
>>>       "o": odd parity
>>>       "m": always mark (logical 1)
>>>       "s": always space (logical 0)
>>> - stop-bits : the number of stop bits after the payload
>>
>> Generally those look fine, though someone more familiar with serial
>> should take a look.
>>
>> We can drop fifo-size given it's fixed. Anything that we know outright
>> doesn't need to be described.
>>
>> We might want to make them mandatory. I don't see any value in not
>> knowing.
> 
> I think making them optional is useless, so either make them mandatory or
> drop them if we can avoid it. Unfortunately, LCR is not part of SBSA,
> that would let us drop the properties and still report the correct settings.
> 
> Would we be better off documenting that 8-n-1 is the only allowed settting?
> I can't think of a reason why anyone would use something else.

Reading the spec again I see that 8-bit word len is mandated:
"The generic UART uses 8-bit words, equivalent to UARTLCR_H.WLEN == b11."
So given this I guess anything other than (8)n1 is pretty useless and
wouldn't probably be used anyways. This means that we can get rid of the
optional properties altogether and just add "current-speed" to the list
of required ones. I am about to implement this.

Cheers,
Andre.

Re: [PATCH 10/10] drivers: PL011: add support for the ARM SBSA generic UART

[Arnd Bergmann]

On Monday 19 January 2015 13:44:41 Andre Przywara wrote:
> 
> > Would we be better off documenting that 8-n-1 is the only allowed settting?
> > I can't think of a reason why anyone would use something else.
> 
> Reading the spec again I see that 8-bit word len is mandated:
> "The generic UART uses 8-bit words, equivalent to UARTLCR_H.WLEN == b11."
> So given this I guess anything other than (8)n1 is pretty useless and
> wouldn't probably be used anyways. This means that we can get rid of the
> optional properties altogether and just add "current-speed" to the list
> of required ones. I am about to implement this.

Ok, great!

	Arnd

Re: [PATCH 00/10] drivers: PL011: add ARM SBSA Generic UART support

[Graeme Gregory]

On Fri, Jan 16, 2015 at 05:22:56PM +0000, Andre Przywara wrote:
> The ARM Server Base System Architecture[1] document describes a
> generic UART which is a subset of the PL011 UART.
> It lacks DMA support, baud rate control and modem status line
> control, among other things.
> The idea is to move the UART initialization and setup into the
> firmware (which does this job today already) and let the kernel just
> use the UART for sending and receiving characters.
> 
> This patchset integrates support for this UART subset into the
> existing PL011 driver - basically by refactoring some
> functions and providing a new uart_ops structure for it. It also has
> a separate probe function to be not dependent on AMBA/PrimeCell.
> It provides a device tree binding, but can easily be adapted to other
> device configuration systems.
> Beside the obvious effect of code sharing reusing most of the PL011
> code has the advantage of not introducing another serial device
> prefix, so it can go with ttyAMA, which seems to be pretty common.
> 
> This series relies on Dave's recent PL011 fix[2], which gets rid of
> the loopback trick to get the UART going. There is a repo at [3]
> (branch sbsa-uart/v1), which has this patch already integrated.
> 
> Patch 1/10 contains a bug fix which applies to the PL011 part also,
> it should be considered regardless of the rest of the series.
> Patch 2-7 refactor some PL011 functions by splitting them up into
> smaller pieces, so that most of the code can be reused later by the
> SBSA part.
> Patch 8 and 9 introduce two new properties for the vendor structure,
> this is for SBSA functionality which cannot be controlled by
> separate uart_ops members only.
> Patch 10 then finally drops in the SBSA specific code, by providing
> a new uart_ops, vendor struct and probe function for it. Also the new
> device tree binding is documented.
> 
> For testing you should be able to take any hardware which has a PL011
> and change the DT to use a "arm,sbsa-uart" compatible string.
> Of course testing with a real SBSA Generic UART is welcomed!
> 
I have tested this series on Juno where it seems to work and also on FVP
model where there are some issues.

On the FVP when we enter usespace a couple of 32 character strings are
printed then nothing else. 32 Characters is a suspicious number.

This occurs with both OE based FS from linaro and debian ubstable FS.

My FVP is version 5602

Graeme

> Cheers,
> Andre
> 
> [1] ARM-DEN-0029 Server Base System Architecture, available (click-
>     thru...) from http://infocenter.arm.com
> [2] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/315248.html
> [3] http://www.linux-arm.org/git?p=linux-ap.git
>     git://linux-arm.org/linux-ap.git
> 
> Andre Przywara (10):
>   drivers: PL011: avoid potential unregister_driver call
>   drivers: PL011: refactor pl011_startup()
>   drivers: PL011: refactor pl011_shutdown()
>   drivers: PL011: refactor pl011_set_termios()
>   drivers: PL011: refactor pl011_probe()
>   drivers: PL011: replace UART_MIS reading with _RIS & _IMSC
>   drivers: PL011: move cts_event workaround into separate function
>   drivers: PL011: allow avoiding UART enabling/disabling
>   drivers: PL011: allow to supply fixed option string
>   drivers: PL011: add support for the ARM SBSA generic UART
> 
>  .../devicetree/bindings/serial/arm_sbsa_uart.txt   |    9 +
>  drivers/tty/serial/amba-pl011.c                    |  510 ++++++++++++++------
>  2 files changed, 381 insertions(+), 138 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt
> 
> -- 
> 1.7.9.5
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 00/10] drivers: PL011: add ARM SBSA Generic UART support

[Andre Przywara]

Hi Graeme,

On 20/01/15 13:08, Graeme Gregory wrote:
> On Fri, Jan 16, 2015 at 05:22:56PM +0000, Andre Przywara wrote:
>> The ARM Server Base System Architecture[1] document describes a
>> generic UART which is a subset of the PL011 UART.
>> It lacks DMA support, baud rate control and modem status line
>> control, among other things.
>> The idea is to move the UART initialization and setup into the
>> firmware (which does this job today already) and let the kernel just
>> use the UART for sending and receiving characters.
>>
>> This patchset integrates support for this UART subset into the
>> existing PL011 driver - basically by refactoring some
>> functions and providing a new uart_ops structure for it. It also has
>> a separate probe function to be not dependent on AMBA/PrimeCell.
>> It provides a device tree binding, but can easily be adapted to other
>> device configuration systems.
>> Beside the obvious effect of code sharing reusing most of the PL011
>> code has the advantage of not introducing another serial device
>> prefix, so it can go with ttyAMA, which seems to be pretty common.
>>
>> This series relies on Dave's recent PL011 fix[2], which gets rid of
>> the loopback trick to get the UART going. There is a repo at [3]
>> (branch sbsa-uart/v1), which has this patch already integrated.
>>
>> Patch 1/10 contains a bug fix which applies to the PL011 part also,
>> it should be considered regardless of the rest of the series.
>> Patch 2-7 refactor some PL011 functions by splitting them up into
>> smaller pieces, so that most of the code can be reused later by the
>> SBSA part.
>> Patch 8 and 9 introduce two new properties for the vendor structure,
>> this is for SBSA functionality which cannot be controlled by
>> separate uart_ops members only.
>> Patch 10 then finally drops in the SBSA specific code, by providing
>> a new uart_ops, vendor struct and probe function for it. Also the new
>> device tree binding is documented.
>>
>> For testing you should be able to take any hardware which has a PL011
>> and change the DT to use a "arm,sbsa-uart" compatible string.
>> Of course testing with a real SBSA Generic UART is welcomed!
>>
> I have tested this series on Juno where it seems to work and also on FVP
> model where there are some issues.
> 
> On the FVP when we enter usespace a couple of 32 character strings are
> printed then nothing else. 32 Characters is a suspicious number.

Can you try to add:
-C bp.pl011_uart0.untimed_fifos=0
to your model command line? (given that your model supports this option)

We discovered that lately, also not sure how the default for this
setting is handled these days in the various model versions.

We are about to investigate this here atm.
This issue seems to be introduced by Dave's FIFO patch.

Cheers,
Andre.

> This occurs with both OE based FS from linaro and debian ubstable FS.
> 
> My FVP is version 5602
> 
> Graeme
> 
>> Cheers,
>> Andre
>>
>> [1] ARM-DEN-0029 Server Base System Architecture, available (click-
>>     thru...) from http://infocenter.arm.com
>> [2] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/315248.html
>> [3] http://www.linux-arm.org/git?p=linux-ap.git
>>     git://linux-arm.org/linux-ap.git
>>
>> Andre Przywara (10):
>>   drivers: PL011: avoid potential unregister_driver call
>>   drivers: PL011: refactor pl011_startup()
>>   drivers: PL011: refactor pl011_shutdown()
>>   drivers: PL011: refactor pl011_set_termios()
>>   drivers: PL011: refactor pl011_probe()
>>   drivers: PL011: replace UART_MIS reading with _RIS & _IMSC
>>   drivers: PL011: move cts_event workaround into separate function
>>   drivers: PL011: allow avoiding UART enabling/disabling
>>   drivers: PL011: allow to supply fixed option string
>>   drivers: PL011: add support for the ARM SBSA generic UART
>>
>>  .../devicetree/bindings/serial/arm_sbsa_uart.txt   |    9 +
>>  drivers/tty/serial/amba-pl011.c                    |  510 ++++++++++++++------
>>  2 files changed, 381 insertions(+), 138 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt
>>
>> -- 
>> 1.7.9.5
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Re: [PATCH 00/10] drivers: PL011: add ARM SBSA Generic UART support

[Graeme Gregory]

On Tue, Jan 20, 2015 at 01:55:01PM +0000, Andre Przywara wrote:
> Hi Graeme,
> 
> On 20/01/15 13:08, Graeme Gregory wrote:
> > On Fri, Jan 16, 2015 at 05:22:56PM +0000, Andre Przywara wrote:
> >> The ARM Server Base System Architecture[1] document describes a
> >> generic UART which is a subset of the PL011 UART.
> >> It lacks DMA support, baud rate control and modem status line
> >> control, among other things.
> >> The idea is to move the UART initialization and setup into the
> >> firmware (which does this job today already) and let the kernel just
> >> use the UART for sending and receiving characters.
> >>
> >> This patchset integrates support for this UART subset into the
> >> existing PL011 driver - basically by refactoring some
> >> functions and providing a new uart_ops structure for it. It also has
> >> a separate probe function to be not dependent on AMBA/PrimeCell.
> >> It provides a device tree binding, but can easily be adapted to other
> >> device configuration systems.
> >> Beside the obvious effect of code sharing reusing most of the PL011
> >> code has the advantage of not introducing another serial device
> >> prefix, so it can go with ttyAMA, which seems to be pretty common.
> >>
> >> This series relies on Dave's recent PL011 fix[2], which gets rid of
> >> the loopback trick to get the UART going. There is a repo at [3]
> >> (branch sbsa-uart/v1), which has this patch already integrated.
> >>
> >> Patch 1/10 contains a bug fix which applies to the PL011 part also,
> >> it should be considered regardless of the rest of the series.
> >> Patch 2-7 refactor some PL011 functions by splitting them up into
> >> smaller pieces, so that most of the code can be reused later by the
> >> SBSA part.
> >> Patch 8 and 9 introduce two new properties for the vendor structure,
> >> this is for SBSA functionality which cannot be controlled by
> >> separate uart_ops members only.
> >> Patch 10 then finally drops in the SBSA specific code, by providing
> >> a new uart_ops, vendor struct and probe function for it. Also the new
> >> device tree binding is documented.
> >>
> >> For testing you should be able to take any hardware which has a PL011
> >> and change the DT to use a "arm,sbsa-uart" compatible string.
> >> Of course testing with a real SBSA Generic UART is welcomed!
> >>
> > I have tested this series on Juno where it seems to work and also on FVP
> > model where there are some issues.
> > 
> > On the FVP when we enter usespace a couple of 32 character strings are
> > printed then nothing else. 32 Characters is a suspicious number.
> 
> Can you try to add:
> -C bp.pl011_uart0.untimed_fifos=0
> to your model command line? (given that your model supports this option)
> 
> We discovered that lately, also not sure how the default for this
> setting is handled these days in the various model versions.
> 
> We are about to investigate this here atm.
> This issue seems to be introduced by Dave's FIFO patch.
> 

With this setting userspace functions better but loses characters
fairly regularly.

Graeme

Re: [PATCH 00/10] drivers: PL011: add ARM SBSA Generic UART support

[Dave P Martin]

On Tue, Jan 20, 2015 at 01:08:32PM +0000, Graeme Gregory wrote:

[...]

> I have tested this series on Juno where it seems to work and also on FVP
> model where there are some issues.
> 
> On the FVP when we enter usespace a couple of 32 character strings are
> printed then nothing else. 32 Characters is a suspicious number.
> 
> This occurs with both OE based FS from linaro and debian ubstable FS.
> 
> My FVP is version 5602

Can you try this:

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index eb397c7..9ca78db 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1291,6 +1291,7 @@ static void pl011_tx_chars(struct uart_amba_port *uap)
 		writew(uap->port.x_char, uap->port.membase + UART01x_DR);
 		uap->port.icount.tx++;
 		uap->port.x_char = 0;
+		BUG_ON(uap->tx_avail < 1);
 		uap->tx_avail--;
 		return;
 	}


If we can hit that BUG_ON, then it's possible tx_avail is wrapping round
here, though I'm not sure if it is likely to occur in practice.

I'll have a look for other potential issues...

Cheers
---Dave

Re: [PATCH 00/10] drivers: PL011: add ARM SBSA Generic UART support

[Andre Przywara]

On 20/01/15 14:26, Graeme Gregory wrote:
> On Tue, Jan 20, 2015 at 01:55:01PM +0000, Andre Przywara wrote:
>> Hi Graeme,
>>
>> On 20/01/15 13:08, Graeme Gregory wrote:
>>> On Fri, Jan 16, 2015 at 05:22:56PM +0000, Andre Przywara wrote:
>>>> The ARM Server Base System Architecture[1] document describes a
>>>> generic UART which is a subset of the PL011 UART.
>>>> It lacks DMA support, baud rate control and modem status line
>>>> control, among other things.
>>>> The idea is to move the UART initialization and setup into the
>>>> firmware (which does this job today already) and let the kernel just
>>>> use the UART for sending and receiving characters.
>>>>
>>>> This patchset integrates support for this UART subset into the
>>>> existing PL011 driver - basically by refactoring some
>>>> functions and providing a new uart_ops structure for it. It also has
>>>> a separate probe function to be not dependent on AMBA/PrimeCell.
>>>> It provides a device tree binding, but can easily be adapted to other
>>>> device configuration systems.
>>>> Beside the obvious effect of code sharing reusing most of the PL011
>>>> code has the advantage of not introducing another serial device
>>>> prefix, so it can go with ttyAMA, which seems to be pretty common.
>>>>
>>>> This series relies on Dave's recent PL011 fix[2], which gets rid of
>>>> the loopback trick to get the UART going. There is a repo at [3]
>>>> (branch sbsa-uart/v1), which has this patch already integrated.
>>>>
>>>> Patch 1/10 contains a bug fix which applies to the PL011 part also,
>>>> it should be considered regardless of the rest of the series.
>>>> Patch 2-7 refactor some PL011 functions by splitting them up into
>>>> smaller pieces, so that most of the code can be reused later by the
>>>> SBSA part.
>>>> Patch 8 and 9 introduce two new properties for the vendor structure,
>>>> this is for SBSA functionality which cannot be controlled by
>>>> separate uart_ops members only.
>>>> Patch 10 then finally drops in the SBSA specific code, by providing
>>>> a new uart_ops, vendor struct and probe function for it. Also the new
>>>> device tree binding is documented.
>>>>
>>>> For testing you should be able to take any hardware which has a PL011
>>>> and change the DT to use a "arm,sbsa-uart" compatible string.
>>>> Of course testing with a real SBSA Generic UART is welcomed!
>>>>
>>> I have tested this series on Juno where it seems to work and also on FVP
>>> model where there are some issues.
>>>
>>> On the FVP when we enter usespace a couple of 32 character strings are
>>> printed then nothing else. 32 Characters is a suspicious number.
>>
>> Can you try to add:
>> -C bp.pl011_uart0.untimed_fifos=0
>> to your model command line? (given that your model supports this option)
>>
>> We discovered that lately, also not sure how the default for this
>> setting is handled these days in the various model versions.
>>
>> We are about to investigate this here atm.
>> This issue seems to be introduced by Dave's FIFO patch.
>>
> 
> With this setting userspace functions better but loses characters
> fairly regularly.

Are you using arm,sbsa-uart in the DT, but a default PL011 in the model
config? In my model the default PL011 version has a 16 byte FIFO only,
specifying:
-C bp.pl011_uart0.revision="r1p5"
upgrades this to the SBSA specified 32 byte FIFO size.
With this setting I don't see any character loses.

Cheers,
Andre.

Re: [PATCH 00/10] drivers: PL011: add ARM SBSA Generic UART support

[Graeme Gregory]

On Tue, Jan 20, 2015 at 02:33:00PM +0000, Andre Przywara wrote:
> 
> 
> On 20/01/15 14:26, Graeme Gregory wrote:
> > On Tue, Jan 20, 2015 at 01:55:01PM +0000, Andre Przywara wrote:
> >> Hi Graeme,
> >>
> >> On 20/01/15 13:08, Graeme Gregory wrote:
> >>> On Fri, Jan 16, 2015 at 05:22:56PM +0000, Andre Przywara wrote:
> >>>> The ARM Server Base System Architecture[1] document describes a
> >>>> generic UART which is a subset of the PL011 UART.
> >>>> It lacks DMA support, baud rate control and modem status line
> >>>> control, among other things.
> >>>> The idea is to move the UART initialization and setup into the
> >>>> firmware (which does this job today already) and let the kernel just
> >>>> use the UART for sending and receiving characters.
> >>>>
> >>>> This patchset integrates support for this UART subset into the
> >>>> existing PL011 driver - basically by refactoring some
> >>>> functions and providing a new uart_ops structure for it. It also has
> >>>> a separate probe function to be not dependent on AMBA/PrimeCell.
> >>>> It provides a device tree binding, but can easily be adapted to other
> >>>> device configuration systems.
> >>>> Beside the obvious effect of code sharing reusing most of the PL011
> >>>> code has the advantage of not introducing another serial device
> >>>> prefix, so it can go with ttyAMA, which seems to be pretty common.
> >>>>
> >>>> This series relies on Dave's recent PL011 fix[2], which gets rid of
> >>>> the loopback trick to get the UART going. There is a repo at [3]
> >>>> (branch sbsa-uart/v1), which has this patch already integrated.
> >>>>
> >>>> Patch 1/10 contains a bug fix which applies to the PL011 part also,
> >>>> it should be considered regardless of the rest of the series.
> >>>> Patch 2-7 refactor some PL011 functions by splitting them up into
> >>>> smaller pieces, so that most of the code can be reused later by the
> >>>> SBSA part.
> >>>> Patch 8 and 9 introduce two new properties for the vendor structure,
> >>>> this is for SBSA functionality which cannot be controlled by
> >>>> separate uart_ops members only.
> >>>> Patch 10 then finally drops in the SBSA specific code, by providing
> >>>> a new uart_ops, vendor struct and probe function for it. Also the new
> >>>> device tree binding is documented.
> >>>>
> >>>> For testing you should be able to take any hardware which has a PL011
> >>>> and change the DT to use a "arm,sbsa-uart" compatible string.
> >>>> Of course testing with a real SBSA Generic UART is welcomed!
> >>>>
> >>> I have tested this series on Juno where it seems to work and also on FVP
> >>> model where there are some issues.
> >>>
> >>> On the FVP when we enter usespace a couple of 32 character strings are
> >>> printed then nothing else. 32 Characters is a suspicious number.
> >>
> >> Can you try to add:
> >> -C bp.pl011_uart0.untimed_fifos=0
> >> to your model command line? (given that your model supports this option)
> >>
> >> We discovered that lately, also not sure how the default for this
> >> setting is handled these days in the various model versions.
> >>
> >> We are about to investigate this here atm.
> >> This issue seems to be introduced by Dave's FIFO patch.
> >>
> > 
> > With this setting userspace functions better but loses characters
> > fairly regularly.
> 
> Are you using arm,sbsa-uart in the DT, but a default PL011 in the model
> config? In my model the default PL011 version has a 16 byte FIFO only,
> specifying:
> -C bp.pl011_uart0.revision="r1p5"
> upgrades this to the SBSA specified 32 byte FIFO size.
> With this setting I don't see any character loses.
> 
Yes with this setting all works with untimed_fifos=0

I was not aware of the differences in versions thanks.

Graeme

Re: [PATCH 00/10] drivers: PL011: add ARM SBSA Generic UART support

[Graeme Gregory]

On Tue, Jan 20, 2015 at 02:52:21PM +0000, Graeme Gregory wrote:
> On Tue, Jan 20, 2015 at 02:33:00PM +0000, Andre Przywara wrote:
> > 
> > 
> > On 20/01/15 14:26, Graeme Gregory wrote:
> > > On Tue, Jan 20, 2015 at 01:55:01PM +0000, Andre Przywara wrote:
> > >> Hi Graeme,
> > >>
> > >> On 20/01/15 13:08, Graeme Gregory wrote:
> > >>> On Fri, Jan 16, 2015 at 05:22:56PM +0000, Andre Przywara wrote:
> > >>>> The ARM Server Base System Architecture[1] document describes a
> > >>>> generic UART which is a subset of the PL011 UART.
> > >>>> It lacks DMA support, baud rate control and modem status line
> > >>>> control, among other things.
> > >>>> The idea is to move the UART initialization and setup into the
> > >>>> firmware (which does this job today already) and let the kernel just
> > >>>> use the UART for sending and receiving characters.
> > >>>>
> > >>>> This patchset integrates support for this UART subset into the
> > >>>> existing PL011 driver - basically by refactoring some
> > >>>> functions and providing a new uart_ops structure for it. It also has
> > >>>> a separate probe function to be not dependent on AMBA/PrimeCell.
> > >>>> It provides a device tree binding, but can easily be adapted to other
> > >>>> device configuration systems.
> > >>>> Beside the obvious effect of code sharing reusing most of the PL011
> > >>>> code has the advantage of not introducing another serial device
> > >>>> prefix, so it can go with ttyAMA, which seems to be pretty common.
> > >>>>
> > >>>> This series relies on Dave's recent PL011 fix[2], which gets rid of
> > >>>> the loopback trick to get the UART going. There is a repo at [3]
> > >>>> (branch sbsa-uart/v1), which has this patch already integrated.
> > >>>>
> > >>>> Patch 1/10 contains a bug fix which applies to the PL011 part also,
> > >>>> it should be considered regardless of the rest of the series.
> > >>>> Patch 2-7 refactor some PL011 functions by splitting them up into
> > >>>> smaller pieces, so that most of the code can be reused later by the
> > >>>> SBSA part.
> > >>>> Patch 8 and 9 introduce two new properties for the vendor structure,
> > >>>> this is for SBSA functionality which cannot be controlled by
> > >>>> separate uart_ops members only.
> > >>>> Patch 10 then finally drops in the SBSA specific code, by providing
> > >>>> a new uart_ops, vendor struct and probe function for it. Also the new
> > >>>> device tree binding is documented.
> > >>>>
> > >>>> For testing you should be able to take any hardware which has a PL011
> > >>>> and change the DT to use a "arm,sbsa-uart" compatible string.
> > >>>> Of course testing with a real SBSA Generic UART is welcomed!
> > >>>>
> > >>> I have tested this series on Juno where it seems to work and also on FVP
> > >>> model where there are some issues.
> > >>>
> > >>> On the FVP when we enter usespace a couple of 32 character strings are
> > >>> printed then nothing else. 32 Characters is a suspicious number.
> > >>
> > >> Can you try to add:
> > >> -C bp.pl011_uart0.untimed_fifos=0
> > >> to your model command line? (given that your model supports this option)
> > >>
> > >> We discovered that lately, also not sure how the default for this
> > >> setting is handled these days in the various model versions.
> > >>
> > >> We are about to investigate this here atm.
> > >> This issue seems to be introduced by Dave's FIFO patch.
> > >>
> > > 
> > > With this setting userspace functions better but loses characters
> > > fairly regularly.
> > 
> > Are you using arm,sbsa-uart in the DT, but a default PL011 in the model
> > config? In my model the default PL011 version has a 16 byte FIFO only,
> > specifying:
> > -C bp.pl011_uart0.revision="r1p5"
> > upgrades this to the SBSA specified 32 byte FIFO size.
> > With this setting I don't see any character loses.
> > 
> Yes with this setting all works with untimed_fifos=0
> 
> I was not aware of the differences in versions thanks.
> 
Now I know how to correctly configure model I have now tested on FVP and
Juno so feel free to add my Tested-by:

Graeme

Re: [PATCH 10/10] drivers: PL011: add support for the ARM SBSA generic UART

[Philip Elcan]

On 01/16/2015 12:23 PM, Andre Przywara wrote:
> The ARM Server Base System Architecture[1] document describes a
> generic UART which is a subset of the PL011 UART.
> It lacks DMA support, baud rate control and modem status line
> control, among other things.
> The idea is to move the UART initialization and setup into the
> firmware (which does this job today already) and let the kernel just
> use the UART for sending and receiving characters.
> We use the recent refactoring the build a new struct uart_ops
> variable which points to some new functions avoiding access to the
> missing registers. We reuse as much existing PL011 code as possible.
> 
> In contrast to the PL011 the SBSA UART does not define any AMBA or
> PrimeCell relations, so we go a pretty generic probe function
> which only uses platform device functions.
> A DT binding is provided, but other systems can easily attach to it,
> too (hint, hint!).
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---

<snip>

Andre,

I'm a little late to address this patchset, but the SBSA defines all
the Generic UART registers 32-bit wide. However, the amba-pl011 driver
uses 16-bit accessors. How will you be handling that? Can the ARM PL011
hardware handle 32-bit access?

Philip

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation Collaborative Project

Re: [PATCH 10/10] drivers: PL011: add support for the ARM SBSA generic UART

[Dave Martin]

On Tue, Feb 17, 2015 at 10:55:35AM -0500, Philip Elcan wrote:
> On 01/16/2015 12:23 PM, Andre Przywara wrote:
> > The ARM Server Base System Architecture[1] document describes a
> > generic UART which is a subset of the PL011 UART.
> > It lacks DMA support, baud rate control and modem status line
> > control, among other things.
> > The idea is to move the UART initialization and setup into the
> > firmware (which does this job today already) and let the kernel just
> > use the UART for sending and receiving characters.
> > We use the recent refactoring the build a new struct uart_ops
> > variable which points to some new functions avoiding access to the
> > missing registers. We reuse as much existing PL011 code as possible.
> >
> > In contrast to the PL011 the SBSA UART does not define any AMBA or
> > PrimeCell relations, so we go a pretty generic probe function
> > which only uses platform device functions.
> > A DT binding is provided, but other systems can easily attach to it,
> > too (hint, hint!).
> >
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
>
> <snip>
>
> Andre,
>
> I'm a little late to address this patchset, but the SBSA defines all
> the Generic UART registers 32-bit wide. However, the amba-pl011 driver
> uses 16-bit accessors. How will you be handling that? Can the ARM PL011
> hardware handle 32-bit access?

Interesting question.  The PL011 TRM [1] specifies only a 16-bit-wide
APB bus interface, but does not say that 32-bit accesses won't work.

I suspect that 32-bit accesses will work on all or most PL011s -- if
that looks too risky or we can't find enough test platforms to be sure
of this, then we could maybe abstract the register access size as a
quirk.

Andre may already have an answer on this.

Cheers
---Dave

[1] ARM DDI 0183G, http://infocenter.arm.com/ -> CoreLink controllers
and peripherals

-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium.  Thank you.

ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No:  2557590
ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No:  2548782

Re: [PATCH 10/10] drivers: PL011: add support for the ARM SBSA generic UART

[Andre Przywara]

Philip,

sorry for the late reply, that was stuck in my Drafts folder :-(

On 02/17/2015 04:16 PM, Dave P Martin wrote:
> On Tue, Feb 17, 2015 at 10:55:35AM -0500, Philip Elcan wrote:
>> On 01/16/2015 12:23 PM, Andre Przywara wrote:
>>> The ARM Server Base System Architecture[1] document describes a
>>> generic UART which is a subset of the PL011 UART.
>>> It lacks DMA support, baud rate control and modem status line
>>> control, among other things.
>>> The idea is to move the UART initialization and setup into the
>>> firmware (which does this job today already) and let the kernel just
>>> use the UART for sending and receiving characters.
>>> We use the recent refactoring the build a new struct uart_ops
>>> variable which points to some new functions avoiding access to the
>>> missing registers. We reuse as much existing PL011 code as possible.
>>>
>>> In contrast to the PL011 the SBSA UART does not define any AMBA or
>>> PrimeCell relations, so we go a pretty generic probe function
>>> which only uses platform device functions.
>>> A DT binding is provided, but other systems can easily attach to it,
>>> too (hint, hint!).
>>>
>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>> ---
>>
>> <snip>
>>
>> Andre,
>>
>> I'm a little late to address this patchset, but the SBSA defines all
>> the Generic UART registers 32-bit wide. However, the amba-pl011 driver
>> uses 16-bit accessors. How will you be handling that? Can the ARM PL011
>> hardware handle 32-bit access?
> 
> Interesting question.  The PL011 TRM [1] specifies only a 16-bit-wide
> APB bus interface, but does not say that 32-bit accesses won't work.
> 
> I suspect that 32-bit accesses will work on all or most PL011s -- if
> that looks too risky or we can't find enough test platforms to be sure
> of this, then we could maybe abstract the register access size as a
> quirk.
> 
> Andre may already have an answer on this.

I am not sure if the 32-bit register _width_ mentioned in the spec
really mandates 32-bit accesses, also the width may be just a spec bug.
Since the SBSA states that an ARM PL011r1p5 is a valid SBSA-UART
implementation, I wonder how this goes together. Also the highest
non-reserved bit in the registers is bit 15 in PL011 and bit 11 in the
SBSA subset.

I fear the actual bus connection is an implementation detail. Given the
fact that all existing PL011 hardware so far works with the 16bit
accesses, I don't dare to change this.

So I'd suggest to keep it as readw/writew for now and the revisit this
topic if some SBSA UART users complain. That makes it easier to justify
the rather invasive change to all MMIO accessors (which I already tried
on one machine for now without problems, btw).

Cheers,
Andre.

Re: [PATCH 10/10] drivers: PL011: add support for the ARM SBSA generic UART

[Dave Martin]

On Wed, Mar 04, 2015 at 05:47:40PM +0000, Andre Przywara wrote:
> Philip,
> 
> sorry for the late reply, that was stuck in my Drafts folder :-(
> 
> On 02/17/2015 04:16 PM, Dave P Martin wrote:
> > On Tue, Feb 17, 2015 at 10:55:35AM -0500, Philip Elcan wrote:

[...]

> >> I'm a little late to address this patchset, but the SBSA defines all
> >> the Generic UART registers 32-bit wide. However, the amba-pl011 driver
> >> uses 16-bit accessors. How will you be handling that? Can the ARM PL011
> >> hardware handle 32-bit access?
> > 
> > Interesting question.  The PL011 TRM [1] specifies only a 16-bit-wide
> > APB bus interface, but does not say that 32-bit accesses won't work.
> > 
> > I suspect that 32-bit accesses will work on all or most PL011s -- if
> > that looks too risky or we can't find enough test platforms to be sure
> > of this, then we could maybe abstract the register access size as a
> > quirk.
> > 
> > Andre may already have an answer on this.
> 
> I am not sure if the 32-bit register _width_ mentioned in the spec
> really mandates 32-bit accesses, also the width may be just a spec bug.
> Since the SBSA states that an ARM PL011r1p5 is a valid SBSA-UART
> implementation, I wonder how this goes together. Also the highest
> non-reserved bit in the registers is bit 15 in PL011 and bit 11 in the
> SBSA subset.
> 
> I fear the actual bus connection is an implementation detail. Given the
> fact that all existing PL011 hardware so far works with the 16bit
> accesses, I don't dare to change this.
> 
> So I'd suggest to keep it as readw/writew for now and the revisit this
> topic if some SBSA UART users complain. That makes it easier to justify
> the rather invasive change to all MMIO accessors (which I already tried
> on one machine for now without problems, btw).

+1

As I understand it, the PL011 bus interface does not take access size
into account at all, so providing "large-enough" accesses are used to
cover all the bits meaningful to the UART, things are likely to work
fine -- and this seems to be the case for all platforms currently
supported by the amba-pl011 driver.

Cheers
---Dave