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

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

[Andre Przywara]

This is the second revision of the SBSA UART support series.
It is now based on 4.0-rc2 and Dave's PL011 fixes[2], which fixes
all problems seen on the fast models and on some hardware.
I also added support for reporting back the actual baudrate to
userland, which needs to be passed in via the device tree by the
firmware now. An attempt to change that value will be ignored by the
driver, sane userland software (like stty) also rightfully complains
about not being able to change it:

# stty < /dev/ttyAMA1 | head -n 1
speed 115200 baud; line = 0;
# stty 38400 < /dev/ttyAMA1
stty: standard input: cannot perform all requested operations
# stty < /dev/ttyAMA1 | head -n 1
speed 115200 baud; line = 0;


----


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@[3]
(branch sbsa-uart/v2), 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 and the
baud rate with the "current-speed" property.
Of course testing with a real SBSA Generic UART is welcomed - as well
as regression testing with any PL011 implementation.

Changelog v1..v2:
- rebased on top of 4.0-rc1 and Dave's newest PL011 fix [2]
- added mandatory current-speed property and report that to userland

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-March/327631.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   |   10 +
 drivers/tty/serial/amba-pl011.c                    |  536 ++++++++++++++------
 2 files changed, 402 insertions(+), 144 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt

-- 
1.7.9.5

[PATCH v2 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 92783fc..961f9b0 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -2235,6 +2235,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;
@@ -2301,11 +2319,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;
 }
@@ -2313,20 +2328,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 v2 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 961f9b0..db2f90e 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1654,6 +1654,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 =
@@ -1665,12 +1691,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;
 
@@ -1693,20 +1714,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 v2 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 |   62 ++++++++++++++++++++++-----------------
 1 file changed, 35 insertions(+), 27 deletions(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index db2f90e..4e0036c 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1733,36 +1733,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;
 
-	cancel_delayed_work_sync(&uap->tx_softirq_work);
-
-	/*
-	 * disable all interrupts
-	 */
-	spin_lock_irq(&uap->port.lock);
-	uap->im = 0;
-	writew(uap->im, uap->port.membase + UART011_IMSC);
-	writew(0xffff & ~UART011_TXIS, 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);
@@ -1778,6 +1757,35 @@ 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)
+{
+	cancel_delayed_work_sync(&uap->tx_softirq_work);
+
+	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 & ~UART011_TXIS, 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 v2 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 4e0036c..37b55ee 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1807,6 +1807,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)
 {
@@ -1870,33 +1902,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 v2 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 |   89 ++++++++++++++++++++++++++-------------
 1 file changed, 60 insertions(+), 29 deletions(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 37b55ee..4fe0a0a 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -2274,13 +2274,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)
@@ -2289,48 +2286,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;
+	uap->port.line = index;
 	INIT_DELAYED_WORK(&uap->tx_softirq_work, pl011_tx_softirq);
-	pl011_dma_probe(&dev->dev, uap);
+	pl011_dma_probe(dev, uap);
 
-	/* Ensure interrupts from this UART are masked and cleared */
-	writew(0, uap->port.membase + UART011_IMSC);
-	writew(0xffff, uap->port.membase + UART011_ICR);
+	amba_ports[index] = uap;
 
-	snprintf(uap->type, sizeof(uap->type), "PL011 rev%u", amba_rev(dev));
+	return 0;
+}
 
-	amba_ports[i] = uap;
+static int pl011_register_port(struct uart_amba_port *uap)
+{
+	int ret;
 
-	amba_set_drvdata(dev, uap);
+	/* 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);
@@ -2347,6 +2346,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 v2 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 4fe0a0a..b8f46f3 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1418,11 +1418,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) {
@@ -1459,7 +1461,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 v2 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>

Conflicts:

	drivers/tty/serial/amba-pl011.c
---
 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 b8f46f3..bca5a3f 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1413,6 +1413,25 @@ static void pl011_tx_irq_seen(struct uart_amba_port *uap)
 		cancel_delayed_work(&uap->tx_softirq_work);
 }
 
+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;
@@ -1420,25 +1439,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 v2 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 bca5a3f..fdb9fa5 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -79,6 +79,7 @@ struct vendor_data {
 	bool			oversampling;
 	bool			dma_threshold;
 	bool			cts_event_workaround;
+	bool			always_enabled;
 
 	unsigned int (*get_fifosize)(struct amba_device *dev);
 };
@@ -95,6 +96,7 @@ static struct vendor_data vendor_arm = {
 	.oversampling		= false,
 	.dma_threshold		= false,
 	.cts_event_workaround	= false,
+	.always_enabled		= false,
 	.get_fifosize		= get_fifosize_arm,
 };
 
@@ -110,6 +112,7 @@ static struct vendor_data vendor_st = {
 	.oversampling		= true,
 	.dma_threshold		= true,
 	.cts_event_workaround	= true,
+	.always_enabled		= false,
 	.get_fifosize		= get_fifosize_st,
 };
 
@@ -2059,7 +2062,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;
 
@@ -2076,10 +2079,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);
 
@@ -2090,7 +2095,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 v2 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 |   17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index fdb9fa5..d3b034f 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -80,6 +80,7 @@ struct vendor_data {
 	bool			dma_threshold;
 	bool			cts_event_workaround;
 	bool			always_enabled;
+	bool			fixed_options;
 
 	unsigned int (*get_fifosize)(struct amba_device *dev);
 };
@@ -97,6 +98,7 @@ static struct vendor_data vendor_arm = {
 	.dma_threshold		= false,
 	.cts_event_workaround	= false,
 	.always_enabled		= false,
+	.fixed_options		= false,
 	.get_fifosize		= get_fifosize_arm,
 };
 
@@ -113,6 +115,7 @@ static struct vendor_data vendor_st = {
 	.dma_threshold		= true,
 	.cts_event_workaround	= true,
 	.always_enabled		= false,
+	.fixed_options		= false,
 	.get_fifosize		= get_fifosize_st,
 };
 
@@ -163,6 +166,7 @@ struct uart_amba_port {
 	struct delayed_work	tx_softirq_work;
 	bool			autorts;
 	unsigned int		tx_irq_seen;	/* 0=none, 1=1, 2=2 or more */
+	unsigned int		fixed_baud;	/* vendor-set fixed baud rate */
 	char			type[12];
 #ifdef CONFIG_DMA_ENGINE
 	/* DMA stuff */
@@ -2177,10 +2181,15 @@ static int __init pl011_console_setup(struct console *co, char *options)
 
 	uap->port.uartclk = clk_get_rate(uap->clk);
 
-	if (options)
-		uart_parse_options(options, &baud, &parity, &bits, &flow);
-	else
-		pl011_console_get_options(uap, &baud, &parity, &bits);
+	if (uap->vendor->fixed_options) {
+		baud = uap->fixed_baud;
+	} else {
+		if (options)
+			uart_parse_options(options,
+					   &baud, &parity, &bits, &flow);
+		else
+			pl011_console_get_options(uap, &baud, &parity, &bits);
+	}
 
 	return uart_set_options(&uap->port, co, baud, parity, bits, flow);
 }
-- 
1.7.9.5

[PATCH v2 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 to 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 with 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   |   10 ++
 drivers/tty/serial/amba-pl011.c                    |  167 ++++++++++++++++++++
 2 files changed, 177 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..4163e7e
--- /dev/null
+++ b/Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt
@@ -0,0 +1,10 @@
+* 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
+- current-speed: the (fixed) baud rate set by the firmware
diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index d3b034f..f3909ab 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -102,6 +102,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		= true,
+};
+
 static unsigned int get_fifosize_st(struct amba_device *dev)
 {
 	return 64;
@@ -1739,6 +1747,30 @@ 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;
+
+	/*
+	 * 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)
 {
@@ -1822,6 +1854,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)
 {
@@ -1973,6 +2018,24 @@ 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)
+{
+	struct uart_amba_port *uap =
+	    container_of(port, struct uart_amba_port, port);
+	unsigned long flags;
+
+	if (old)
+		tty_termios_copy_hw(termios, old);
+	tty_termios_encode_baud_rate(termios, uap->fixed_baud, uap->fixed_baud);
+
+	spin_lock_irqsave(&port->lock, flags);
+	uart_update_timeout(port, CS8, uap->fixed_baud);
+	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 =
@@ -2048,6 +2111,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
@@ -2435,6 +2532,76 @@ 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;
+	int baudrate;
+
+	/*
+	 * Check the mandatory baud rate parameter in the DT node early
+	 * so that we can easily exit with the error.
+	 */
+	if (pdev->dev.of_node) {
+		struct device_node *np = pdev->dev.of_node;
+
+		ret = of_property_read_u32(np, "current-speed", &baudrate);
+		if (ret)
+			return ret;
+	} else {
+		baudrate = 115200;
+	}
+
+	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;
+	uap->fixed_baud = baudrate;
+
+	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

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

[Greg KH]

On Wed, Mar 04, 2015 at 05:59:51PM +0000, Andre Przywara wrote:
> 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>
> 
> Conflicts:
> 
> 	drivers/tty/serial/amba-pl011.c

Why is this here in the changelog?  It doesn't make sense when sending
patches out :)

Please fix up and resend the patches I didn't apply.

thanks,

greg k-h

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

[Greg KH]

On Wed, Mar 04, 2015 at 05:59:44PM +0000, Andre Przywara wrote:
> This is the second revision of the SBSA UART support series.
> It is now based on 4.0-rc2 and Dave's PL011 fixes[2], which fixes
> all problems seen on the fast models and on some hardware.
> I also added support for reporting back the actual baudrate to
> userland, which needs to be passed in via the device tree by the
> firmware now. An attempt to change that value will be ignored by the
> driver, sane userland software (like stty) also rightfully complains
> about not being able to change it:
> 
> # stty < /dev/ttyAMA1 | head -n 1
> speed 115200 baud; line = 0;
> # stty 38400 < /dev/ttyAMA1
> stty: standard input: cannot perform all requested operations
> # stty < /dev/ttyAMA1 | head -n 1
> speed 115200 baud; line = 0;
> 
> 
> ----
> 
> 
> 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/v2), 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 and the
> baud rate with the "current-speed" property.
> Of course testing with a real SBSA Generic UART is welcomed - as well
> as regression testing with any PL011 implementation.
> 
> Changelog v1..v2:
> - rebased on top of 4.0-rc1 and Dave's newest PL011 fix [2]
> - added mandatory current-speed property and report that to userland

Even with Dave's latest fix, this series doesn't apply to my tty-testing
branch of tty.git, can you please refresh it and resend?

thanks,

greg k-h

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

[Dave Martin]

On Wed, Mar 04, 2015 at 05:59:54PM +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 to 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 with 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>

[...]

> +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);

Hmmm, this seems to break with CONFIG_MODULE -- it tries to create its
own module_{init,exit} that just register/unregister the platform
driver.

Instead, I think the existing init/exit functions need to register/
unregister both drivers.

Module initialisation shouldn't fail unless neither registration
succeeds (?) -- if only one succeeds we can carry on, but it's worth
a printk.  Or can the failure of either registration just be fatal?

Cheers
---Dave

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

[Russell King - ARM Linux]

On Wed, Mar 04, 2015 at 05:59:45PM +0000, Andre Przywara wrote:
> 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 92783fc..961f9b0 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -2235,6 +2235,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);
> +}

This is still racy, as I pointed out at the time this crap was dreamt
up.

There is _no_ locking between an individual driver's ->probe or ->remove
functions being called concurrently for different devices.  The only
locking which the driver model guarantees is that a single struct device
can only be probed by one driver at a time.

Multiple struct device's can be in-progress of ->probe or ->remove
simultaneously.

However, this isn't your bug to solve... it's those who were proponents
of this crap approach.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

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

[Russell King - ARM Linux]

On Wed, Mar 04, 2015 at 05:59:50PM +0000, Andre Przywara wrote:
> 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.

Do we really need to go all the way to the hardware for this?  Isn't
uap->im sufficient?  Reading from memory is potentially faster as it
could be in the CPU caches.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

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

[Russell King - ARM Linux]

On Wed, Mar 04, 2015 at 05:59:54PM +0000, Andre Przywara wrote:
> +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,

It is generally accepted that we don't mention pointers/values which are
initialised to NULL/0 in initialisers.

> +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);

No need to open code the initialisation, rather than using the
module_*_driver() helper macros to avoid the problem which Dave mentioned.

These macros are only there to avoid having to write out the same boiler
plate in loads of simple drivers.  As soon as a driver has more than one
device driver structure in it, it needs to be open coded.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

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

[Andre Przywara]

Hi Russel,

thanks a lot for looking at the patches!

On 12/03/15 10:52, Russell King - ARM Linux wrote:
> On Wed, Mar 04, 2015 at 05:59:54PM +0000, Andre Przywara wrote:
>> +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,
> 
> It is generally accepted that we don't mention pointers/values which are
> initialised to NULL/0 in initialisers.

Right, I forgot to delete those after development where I had them in
explicitly to make sure I covered everything.
Thanks for spotting.

> 
>> +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);
> 
> No need to open code the initialisation, rather than using the
> module_*_driver() helper macros to avoid the problem which Dave mentioned.
> 
> These macros are only there to avoid having to write out the same boiler
> plate in loads of simple drivers.  As soon as a driver has more than one
> device driver structure in it, it needs to be open coded.

Actually I prepared this already for the ACPI guys, which want to stuff
their ACPI table match function in there - I think then we need the open
coded version. So if you don't mind too much, I'd like to keep it like
this and hope for someone to actually use it ;-)

Cheers,
Andre.

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

[Russell King - ARM Linux]

On Thu, Mar 12, 2015 at 01:43:17PM +0000, Andre Przywara wrote:
> Hi Russel,
> 
> thanks a lot for looking at the patches!
> 
> On 12/03/15 10:52, Russell King - ARM Linux wrote:
> > On Wed, Mar 04, 2015 at 05:59:54PM +0000, Andre Przywara wrote:
> >> +module_platform_driver(arm_sbsa_uart_platform_driver);
> > 
> > No need to open code the initialisation, rather than using the
> > module_*_driver() helper macros to avoid the problem which Dave mentioned.
> > 
> > These macros are only there to avoid having to write out the same boiler
> > plate in loads of simple drivers.  As soon as a driver has more than one
> > device driver structure in it, it needs to be open coded.
> 
> Actually I prepared this already for the ACPI guys, which want to stuff
> their ACPI table match function in there - I think then we need the open
> coded version. So if you don't mind too much, I'd like to keep it like
> this and hope for someone to actually use it ;-)

Either your statement is ambiguous, or I'm not understanding you.

You can't "keep it like this" where "this" is the above code.  The
above will fail if the driver is built as a module, and cause a build
time error.  That is not acceptable.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

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

[Andre Przywara]

On 12/03/15 13:49, Russell King - ARM Linux wrote:
> On Thu, Mar 12, 2015 at 01:43:17PM +0000, Andre Przywara wrote:
>> Hi Russel,
>>
>> thanks a lot for looking at the patches!
>>
>> On 12/03/15 10:52, Russell King - ARM Linux wrote:
>>> On Wed, Mar 04, 2015 at 05:59:54PM +0000, Andre Przywara wrote:
>>>> +module_platform_driver(arm_sbsa_uart_platform_driver);
>>>
>>> No need to open code the initialisation, rather than using the
>>> module_*_driver() helper macros to avoid the problem which Dave mentioned.
>>>
>>> These macros are only there to avoid having to write out the same boiler
>>> plate in loads of simple drivers.  As soon as a driver has more than one
>>> device driver structure in it, it needs to be open coded.
>>
>> Actually I prepared this already for the ACPI guys, which want to stuff
>> their ACPI table match function in there - I think then we need the open
>> coded version. So if you don't mind too much, I'd like to keep it like
>> this and hope for someone to actually use it ;-)
> 
> Either your statement is ambiguous, or I'm not understanding you.
> 
> You can't "keep it like this" where "this" is the above code.  The
> above will fail if the driver is built as a module, and cause a build
> time error.  That is not acceptable.

Oh, you are right, I was forgetting about that one, sorry.
I meant I'd rather leave it open coded, but of course I will fix the
module issue.

Cheers,
Andre.

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

[Andre Przywara]

Hi Russell,

On 12/03/15 10:42, Russell King - ARM Linux wrote:
> On Wed, Mar 04, 2015 at 05:59:45PM +0000, Andre Przywara wrote:
>> 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 92783fc..961f9b0 100644
>> --- a/drivers/tty/serial/amba-pl011.c
>> +++ b/drivers/tty/serial/amba-pl011.c
>> @@ -2235,6 +2235,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);
>> +}
> 
> This is still racy, as I pointed out at the time this crap was dreamt
> up.
> 
> There is _no_ locking between an individual driver's ->probe or ->remove
> functions being called concurrently for different devices.  The only
> locking which the driver model guarantees is that a single struct device
> can only be probed by one driver at a time.
> 
> Multiple struct device's can be in-progress of ->probe or ->remove
> simultaneously.

OK, I see.

> However, this isn't your bug to solve... it's those who were proponents
> of this crap approach.

Does that mean you want me to drop this patch? It isn't strictly
necessary for my series. So do you want to postpone a fix until later
when there is a real solution (tm) for this issue or shall I include
this still in my series for fixing at least half of the issue?

Thanks,
Andre.

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

[Russell King - ARM Linux]

On Wed, Apr 08, 2015 at 04:39:03PM +0100, Andre Przywara wrote:
> Hi Russell,
> 
> On 12/03/15 10:42, Russell King - ARM Linux wrote:
> > On Wed, Mar 04, 2015 at 05:59:45PM +0000, Andre Przywara wrote:
> >> 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 92783fc..961f9b0 100644
> >> --- a/drivers/tty/serial/amba-pl011.c
> >> +++ b/drivers/tty/serial/amba-pl011.c
> >> @@ -2235,6 +2235,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);
> >> +}
> > 
> > This is still racy, as I pointed out at the time this crap was dreamt
> > up.
> > 
> > There is _no_ locking between an individual driver's ->probe or ->remove
> > functions being called concurrently for different devices.  The only
> > locking which the driver model guarantees is that a single struct device
> > can only be probed by one driver at a time.
> > 
> > Multiple struct device's can be in-progress of ->probe or ->remove
> > simultaneously.
> 
> OK, I see.
> 
> > However, this isn't your bug to solve... it's those who were proponents
> > of this crap approach.
> 
> Does that mean you want me to drop this patch? It isn't strictly
> necessary for my series. So do you want to postpone a fix until later
> when there is a real solution (tm) for this issue or shall I include
> this still in my series for fixing at least half of the issue?

Sorry, it's been way too long since my mail was sent, I've lost all
context about it.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.