drivers: PL011: add ARM SBSA Generic UART support

PATCH Superseded Andre Przywara 4 participants 22 messages Mar 4, 2015 – Apr 8, 2015

Subsystems (3)

Device tree bindings TTY layer and serial drivers Arm primecell UART PL010 and PL011 drivers

lore

Share & tools

lore

Andre Przywara

author

2015-03-04 17:59 UTC #0 lore raw

v2 cover drivers: PL011: add ARM SBSA Generic UART support

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

Signature · 1 line

1.7.9.5

Andre Przywara

author

2015-03-04 17:59 UTC #1 lore raw

PATCH v2 1/10 drivers: PL011: avoid potential unregister_driver call

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 <[email protected]>

---
 drivers/tty/serial/amba-pl011.c |   38 +++++++++++++++++++++-----------------
 1 file changed, 21 insertions(+), 17 deletions(-)

drivers/tty/serial/amba-pl011.c +21 ~~-17~~

index 92783fc..961f9b0 100644
@@ -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;
 }

Signature · 1 line

1.7.9.5

Russell King - ARM Linux

2015-03-12 10:42 UTC #2 lore raw

PATCH v2 1/10 drivers: PL011: avoid potential unregister_driver call

Quoted 27 lines · 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 <[email protected]>
---
 drivers/tty/serial/amba-pl011.c |   38 +++++++++++++++++++++-----------------
 1 file changed, 21 insertions(+), 17 deletions(-)
drivers/tty/serial/amba-pl011.c +6 -0
index 92783fc..961f9b0 100644
@@ -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;
+

drivers/tty/serial/amba-pl011.c static int pl011_probe_dt_alias(int index, struct device *dev) · line 2253

Quoted 10 lines

drivers/tty/serial/amba-pl011.c +10 -0

+	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.

Signature · 2 lines

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

Andre Przywara

author

2015-04-08 15:39 UTC #3 lore raw

PATCH v2 1/10 drivers: PL011: avoid potential unregister_driver call

Hi Russell,

Quoted 42 lines · Russell King - ARM Linux

Quoted 27 lines · 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 <[email protected]>
---
 drivers/tty/serial/amba-pl011.c |   38 +++++++++++++++++++++-----------------
 1 file changed, 21 insertions(+), 17 deletions(-)
drivers/tty/serial/amba-pl011.c +6 -0
index 92783fc..961f9b0 100644
@@ -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;
+
drivers/tty/serial/amba-pl011.c static int pl011_probe_dt_alias(int index, struct device *dev) · line 2253
Quoted 10 lines
drivers/tty/serial/amba-pl011.c +10 -0
+	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.

Quoted 7 lines

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.

Quoted 2 lines

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.

Russell King - ARM Linux

2015-04-08 18:14 UTC #4 lore raw

PATCH v2 1/10 drivers: PL011: avoid potential unregister_driver call

Quoted 55 lines · Andre Przywara

Hi Russell,
Quoted 42 lines · Russell King - ARM Linux
Quoted 27 lines · 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 <[email protected]>
---
 drivers/tty/serial/amba-pl011.c |   38 +++++++++++++++++++++-----------------
 1 file changed, 21 insertions(+), 17 deletions(-)
drivers/tty/serial/amba-pl011.c +6 -0
index 92783fc..961f9b0 100644
@@ -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;
+
drivers/tty/serial/amba-pl011.c static int pl011_probe_dt_alias(int index, struct device *dev) · line 2253
Quoted 10 lines
drivers/tty/serial/amba-pl011.c +10 -0
+	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.
Quoted 7 lines
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.

Quoted 7 lines

Quoted 2 lines
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.

Signature · 2 lines

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

Andre Przywara

author

2015-03-04 17:59 UTC #5 lore raw

PATCH v2 2/10 drivers: PL011: refactor pl011_startup()

In reply to Andre Przywara

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

Signed-off-by Andre Przywara <[email protected]>

---
 drivers/tty/serial/amba-pl011.c |   48 +++++++++++++++++++++++----------------
 1 file changed, 28 insertions(+), 20 deletions(-)

drivers/tty/serial/amba-pl011.c +28 ~~-20~~

index 961f9b0..db2f90e 100644
@@ -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;

Signature · 1 line

1.7.9.5

Andre Przywara

author

2015-03-04 17:59 UTC #6 lore raw

PATCH v2 3/10 drivers: PL011: refactor pl011_shutdown()

In reply to Andre Przywara

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

Signed-off-by Andre Przywara <[email protected]>

---
 drivers/tty/serial/amba-pl011.c |   62 ++++++++++++++++++++++-----------------
 1 file changed, 35 insertions(+), 27 deletions(-)

drivers/tty/serial/amba-pl011.c +35 ~~-27~~

index db2f90e..4e0036c 100644
@@ -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

Signature · 1 line

1.7.9.5

Andre Przywara

author

2015-03-04 17:59 UTC #7 lore raw

PATCH v2 4/10 drivers: PL011: refactor pl011_set_termios()

In reply to 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 <[email protected]>

---
 drivers/tty/serial/amba-pl011.c |   60 +++++++++++++++++++++------------------
 1 file changed, 33 insertions(+), 27 deletions(-)

drivers/tty/serial/amba-pl011.c +33 ~~-27~~

index 4e0036c..37b55ee 100644
@@ -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);

Signature · 1 line

1.7.9.5

Andre Przywara

author

2015-03-04 17:59 UTC #8 lore raw

PATCH v2 5/10 drivers: PL011: refactor pl011_probe()

In reply to 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 <[email protected]>

---
 drivers/tty/serial/amba-pl011.c |   89 ++++++++++++++++++++++++++-------------
 1 file changed, 60 insertions(+), 29 deletions(-)

drivers/tty/serial/amba-pl011.c +60 ~~-29~~

index 37b55ee..4fe0a0a 100644
@@ -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);

Signature · 1 line

1.7.9.5

Andre Przywara

author

2015-03-04 17:59 UTC #9 lore raw

PATCH v2 6/10 drivers: PL011: replace UART_MIS reading with _RIS & _IMSC

In reply to 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 <[email protected]>

---
 drivers/tty/serial/amba-pl011.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

drivers/tty/serial/amba-pl011.c +4 -2

index 4fe0a0a..b8f46f3 100644
@@ -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;
 	}

Signature · 1 line

1.7.9.5

Russell King - ARM Linux

2015-03-12 10:46 UTC #10 lore raw

PATCH v2 6/10 drivers: PL011: replace UART_MIS reading with _RIS & _IMSC

Quoted 6 lines · 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.

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.

Signature · 2 lines

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

Andre Przywara

author

2015-03-04 17:59 UTC #11 lore raw

PATCH v2 7/10 drivers: PL011: move cts_event workaround into separate function

In reply to 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 <[email protected]>

Conflicts:

	drivers/tty/serial/amba-pl011.c
---
 drivers/tty/serial/amba-pl011.c |   33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

drivers/tty/serial/amba-pl011.c +20 ~~-13~~

index b8f46f3..bca5a3f 100644
@@ -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),

Signature · 1 line

1.7.9.5

Greg KH

maintainer

2015-03-07 03:00 UTC #12 lore raw

PATCH v2 7/10 drivers: PL011: move cts_event workaround into separate function

Quoted 9 lines · 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 <[email protected]>
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

Andre Przywara

author

2015-03-04 17:59 UTC #13 lore raw

PATCH v2 8/10 drivers: PL011: allow avoiding UART enabling/disabling

In reply to 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 <[email protected]>

---
 drivers/tty/serial/amba-pl011.c |   18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

drivers/tty/serial/amba-pl011.c +12 -6

index bca5a3f..fdb9fa5 100644
@@ -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);

Signature · 1 line

1.7.9.5

Andre Przywara

author

2015-03-04 17:59 UTC #14 lore raw

PATCH v2 9/10 drivers: PL011: allow to supply fixed option string

In reply to 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 <[email protected]>

---
 drivers/tty/serial/amba-pl011.c |   17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

drivers/tty/serial/amba-pl011.c +13 -4

index fdb9fa5..d3b034f 100644
@@ -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);
 }

Signature · 1 line

1.7.9.5

Andre Przywara

author

2015-03-04 17:59 UTC #15 lore raw

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

In reply to Andre Przywara

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 <[email protected]>

---
 .../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

Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt new file +10 -0

new file mode 100644
index 0000000..4163e7e
@@ -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

drivers/tty/serial/amba-pl011.c +167 -0

203 lines not on this page · raw message

Signature · 1 line

1.7.9.5

Dave Martin

2015-03-09 15:59 UTC #16 lore raw

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

Quoted 19 lines · 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 <[email protected]>

[...]

drivers/tty/serial/amba-pl011.c static int pl011_resume(struct device *dev) · line 2603

Quoted 12 lines

drivers/tty/serial/amba-pl011.c +12 -0

+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

Russell King - ARM Linux

2015-03-12 10:52 UTC #17 lore raw

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

In reply to Andre Przywara

drivers/tty/serial/amba-pl011.c static struct uart_ops amba_pl011_pops = { · line 2131

Quoted 9 lines · Andre Przywara

drivers/tty/serial/amba-pl011.c +9 -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,

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

drivers/tty/serial/amba-pl011.c static int pl011_resume(struct device *dev) · line 2603

Quoted 10 lines

drivers/tty/serial/amba-pl011.c +10 -0

+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.

Signature · 2 lines

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

Andre Przywara

author

2015-03-12 13:43 UTC #18 lore raw

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

Hi Russel,

thanks a lot for looking at the patches!

Quoted 13 lines · Russell King - ARM Linux

drivers/tty/serial/amba-pl011.c static struct uart_ops amba_pl011_pops = { · line 2131
Quoted 9 lines · Andre Przywara
drivers/tty/serial/amba-pl011.c +9 -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,
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.

Quoted 15 lines

drivers/tty/serial/amba-pl011.c static int pl011_resume(struct device *dev) · line 2603
Quoted 11 lines
drivers/tty/serial/amba-pl011.c +10 -0
+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.

Quoted 3 lines

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.

Russell King - ARM Linux

2015-03-12 13:49 UTC #19 lore raw

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

Quoted 11 lines · Andre Przywara

Hi Russel,
thanks a lot for looking at the patches!
Quoted 6 lines · Russell King - ARM Linux
Quoted 1 line · Andre Przywara
+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.

Quoted 8 lines

Quoted 3 lines
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.

Signature · 2 lines

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

Andre Przywara

author

2015-03-12 13:58 UTC #20 lore raw

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

Quoted 21 lines · Russell King - ARM Linux

Quoted 11 lines · Andre Przywara
Hi Russel,
thanks a lot for looking at the patches!
Quoted 6 lines · Russell King - ARM Linux
Quoted 1 line · Andre Przywara
+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.
Quoted 8 lines
Quoted 3 lines
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 ;-)

Quoted 5 lines

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.

Greg KH

maintainer

2015-03-07 03:01 UTC #21 lore raw

v2 cover drivers: PL011: add ARM SBSA Generic UART support

In reply to Andre Przywara

Quoted 60 lines · 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 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.

Quoted 3 lines

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