Skip to contentSkip to conversation

drivers: PL011: add ARM SBSA Generic UART support

PATCH New Andre Przywara 6 participants 32 messages Jan 16, 2015 – Mar 5, 2015

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

cover drivers: PL011: add ARM SBSA Generic UART support

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

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

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

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

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

Cheers,
Andre

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

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

 .../devicetree/bindings/serial/arm_sbsa_uart.txt   |    9 +
 drivers/tty/serial/amba-pl011.c                    |  510 ++++++++++++++------
 2 files changed, 381 insertions(+), 138 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt
Signature · 1 line
1.7.9.5
#1 lore raw

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

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

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

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

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

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

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

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

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

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

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

---
 drivers/tty/serial/amba-pl011.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/tty/serial/amba-pl011.c +4 -2
index e484551..e6c2746 100644
@@ -1350,11 +1350,13 @@ static irqreturn_t pl011_int(int irq, void *dev_id)
 	struct uart_amba_port *uap = dev_id;
 	unsigned long flags;
 	unsigned int status, pass_counter = AMBA_ISR_PASS_LIMIT;
+	u16 imsc;
 	int handled = 0;
 	unsigned int dummy_read;
 
 	spin_lock_irqsave(&uap->port.lock, flags);
-	status = readw(uap->port.membase + UART011_MIS);
+	imsc = readw(uap->port.membase + UART011_IMSC);
+	status = readw(uap->port.membase + UART011_RIS) & imsc;
 	if (status) {
 		do {
 			if (uap->vendor->cts_event_workaround) {
@@ -1391,7 +1393,7 @@ static irqreturn_t pl011_int(int irq, void *dev_id)
 			if (pass_counter-- == 0)
 				break;
 
-			status = readw(uap->port.membase + UART011_MIS);
+			status = readw(uap->port.membase + UART011_RIS) & imsc;
 		} while (status != 0);
 		handled = 1;
 	}
Signature · 1 line
1.7.9.5
#7 lore raw

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

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

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

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

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

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

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

In reply to Andre Przywara

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

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

---
 .../devicetree/bindings/serial/arm_sbsa_uart.txt   |    9 ++
 drivers/tty/serial/amba-pl011.c                    |  154 ++++++++++++++++++++
 2 files changed, 163 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt
Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt new file +9 -0
new file mode 100644
index 0000000..21d211f
@@ -0,0 +1,9 @@
+* ARM SBSA defined generic UART
+This UART uses a subset of the PL011 registers and consequently lives
+in the PL011 driver. It's baudrate and other communication parameters
+cannot be adjusted at runtime, so it lacks a clock specifier here.
+
+Required properties:
+- compatible: must be "arm,sbsa-uart"
+- reg: exactly one register range
+- interrupts: exactly one interrupt specifier
drivers/tty/serial/amba-pl011.c +154 -0

190 lines not on this page · raw message

Signature · 1 line
1.7.9.5
#11 lore raw

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

Quoted 44 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 the build a new struct uart_ops variable which points to some new functions avoiding access to the missing registers. We reuse as much existing PL011 code as possible.

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

---
 .../devicetree/bindings/serial/arm_sbsa_uart.txt   |    9 ++
 drivers/tty/serial/amba-pl011.c                    |  154 ++++++++++++++++++++
 2 files changed, 163 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt
Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt new file +9 -0
new file mode 100644
index 0000000..21d211f
@@ -0,0 +1,9 @@
+* ARM SBSA defined generic UART
+This UART uses a subset of the PL011 registers and consequently lives
+in the PL011 driver. It's baudrate and other communication parameters
+cannot be adjusted at runtime, so it lacks a clock specifier here.
+
+Required properties:
+- compatible: must be "arm,sbsa-uart"
+- reg: exactly one register range
+- interrupts: exactly one interrupt specifier
drivers/tty/serial/amba-pl011.c +0 -0
index a1c929f..596e641 100644

drivers/tty/serial/amba-pl011.c static struct vendor_data vendor_arm = { · line 110

Quoted 10 lines
drivers/tty/serial/amba-pl011.c +7 -0
@@ -101,6 +101,14 @@ static struct vendor_data vendor_arm = {
 	.get_fifosize		= get_fifosize_arm,
 };
 
+static struct vendor_data vendor_sbsa = {
+	.oversampling		= false,
+	.dma_threshold		= false,
+	.cts_event_workaround	= false,
+	.always_enabled		= true,
+	.fixed_options		= "115200n8",
+};

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

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

Thanks,
Mark.

#12 lore raw

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

Hi Mark,

Quoted 48 lines · Mark Rutland
Quoted 47 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 the build a new struct uart_ops variable which points to some new functions avoiding access to the missing registers. We reuse as much existing PL011 code as possible.

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

---
 .../devicetree/bindings/serial/arm_sbsa_uart.txt   |    9 ++
 drivers/tty/serial/amba-pl011.c                    |  154 ++++++++++++++++++++
 2 files changed, 163 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt
Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt new file +9 -0
new file mode 100644
index 0000000..21d211f
@@ -0,0 +1,9 @@
+* ARM SBSA defined generic UART
+This UART uses a subset of the PL011 registers and consequently lives
+in the PL011 driver. It's baudrate and other communication parameters
+cannot be adjusted at runtime, so it lacks a clock specifier here.
+
+Required properties:
+- compatible: must be "arm,sbsa-uart"
+- reg: exactly one register range
+- interrupts: exactly one interrupt specifier
drivers/tty/serial/amba-pl011.c +0 -0
index a1c929f..596e641 100644
@@ -101,6 +101,14 @@ static struct vendor_data vendor_arm = {
 	.get_fifosize		= get_fifosize_arm,
 };
 
Quoted 10 lines

drivers/tty/serial/amba-pl011.c static struct vendor_data vendor_arm = { · line 110

Quoted 7 lines
drivers/tty/serial/amba-pl011.c +7 -0
+static struct vendor_data vendor_sbsa = {
+	.oversampling		= false,
+	.dma_threshold		= false,
+	.cts_event_workaround	= false,
+	.always_enabled		= true,
+	.fixed_options		= "115200n8",
+};

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

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

Quoted 3 lines

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

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

Cheers,
Andre.

#13 lore raw

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

Quoted 62 lines · Andre Przywara

Hi Mark,

Quoted 48 lines · Mark Rutland
Quoted 47 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 the build a new struct uart_ops variable which points to some new functions avoiding access to the missing registers. We reuse as much existing PL011 code as possible.

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

---
 .../devicetree/bindings/serial/arm_sbsa_uart.txt   |    9 ++
 drivers/tty/serial/amba-pl011.c                    |  154 ++++++++++++++++++++
 2 files changed, 163 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt
Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt new file +9 -0
new file mode 100644
index 0000000..21d211f
@@ -0,0 +1,9 @@
+* ARM SBSA defined generic UART
+This UART uses a subset of the PL011 registers and consequently lives
+in the PL011 driver. It's baudrate and other communication parameters
+cannot be adjusted at runtime, so it lacks a clock specifier here.
+
+Required properties:
+- compatible: must be "arm,sbsa-uart"
+- reg: exactly one register range
+- interrupts: exactly one interrupt specifier
drivers/tty/serial/amba-pl011.c +0 -0
index a1c929f..596e641 100644
@@ -101,6 +101,14 @@ static struct vendor_data vendor_arm = {
 	.get_fifosize		= get_fifosize_arm,
 };
 
Quoted 10 lines

drivers/tty/serial/amba-pl011.c static struct vendor_data vendor_arm = { · line 110

Quoted 7 lines
drivers/tty/serial/amba-pl011.c +7 -0
+static struct vendor_data vendor_sbsa = {
+	.oversampling		= false,
+	.dma_threshold		= false,
+	.cts_event_workaround	= false,
+	.always_enabled		= true,
+	.fixed_options		= "115200n8",
+};

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

Quoted 4 lines

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

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

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

Quoted 9 lines
Quoted 3 lines

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

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

The fact that we cannot reconfigure it is orthogonal.

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

Mark.

#14 lore raw

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

Hi Mark,

Quoted 68 lines · Mark Rutland
Quoted 62 lines · Andre Przywara

Hi Mark,

Quoted 48 lines · Mark Rutland
Quoted 47 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 the build a new struct uart_ops variable which points to some new functions avoiding access to the missing registers. We reuse as much existing PL011 code as possible.

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

---
 .../devicetree/bindings/serial/arm_sbsa_uart.txt   |    9 ++
 drivers/tty/serial/amba-pl011.c                    |  154 ++++++++++++++++++++
 2 files changed, 163 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt
Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt new file +9 -0
new file mode 100644
index 0000000..21d211f
@@ -0,0 +1,9 @@
+* ARM SBSA defined generic UART
+This UART uses a subset of the PL011 registers and consequently lives
+in the PL011 driver. It's baudrate and other communication parameters
+cannot be adjusted at runtime, so it lacks a clock specifier here.
+
+Required properties:
+- compatible: must be "arm,sbsa-uart"
+- reg: exactly one register range
+- interrupts: exactly one interrupt specifier
drivers/tty/serial/amba-pl011.c +0 -0
index a1c929f..596e641 100644
@@ -101,6 +101,14 @@ static struct vendor_data vendor_arm = {
 	.get_fifosize		= get_fifosize_arm,
 };
 
Quoted 10 lines

drivers/tty/serial/amba-pl011.c static struct vendor_data vendor_arm = { · line 110

Quoted 7 lines
drivers/tty/serial/amba-pl011.c +7 -0
+static struct vendor_data vendor_sbsa = {
+	.oversampling		= false,
+	.dma_threshold		= false,
+	.cts_event_workaround	= false,
+	.always_enabled		= true,
+	.fixed_options		= "115200n8",
+};

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

Quoted 4 lines

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

Quoted 5 lines

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

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

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

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

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

Cheers,
Andre.

Quoted 17 lines
Quoted 9 lines
Quoted 3 lines

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

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

The fact that we cannot reconfigure it is orthogonal.

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

Mark.

#15 lore raw

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

Quoted 71 lines · Andre Przywara

Hi Mark,

Quoted 68 lines · Mark Rutland
Quoted 66 lines · Andre Przywara

Hi Mark,

Quoted 48 lines · Mark Rutland
Quoted 47 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 the build a new struct uart_ops variable which points to some new functions avoiding access to the missing registers. We reuse as much existing PL011 code as possible.

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

---
 .../devicetree/bindings/serial/arm_sbsa_uart.txt   |    9 ++
 drivers/tty/serial/amba-pl011.c                    |  154 ++++++++++++++++++++
 2 files changed, 163 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt
Documentation/devicetree/bindings/serial/arm_sbsa_uart.txt new file +9 -0
new file mode 100644
index 0000000..21d211f
@@ -0,0 +1,9 @@
+* ARM SBSA defined generic UART
+This UART uses a subset of the PL011 registers and consequently lives
+in the PL011 driver. It's baudrate and other communication parameters
+cannot be adjusted at runtime, so it lacks a clock specifier here.
+
+Required properties:
+- compatible: must be "arm,sbsa-uart"
+- reg: exactly one register range
+- interrupts: exactly one interrupt specifier
drivers/tty/serial/amba-pl011.c +0 -0
index a1c929f..596e641 100644
@@ -101,6 +101,14 @@ static struct vendor_data vendor_arm = {
 	.get_fifosize		= get_fifosize_arm,
 };
 
Quoted 10 lines

drivers/tty/serial/amba-pl011.c static struct vendor_data vendor_arm = { · line 110

Quoted 7 lines
drivers/tty/serial/amba-pl011.c +7 -0
+static struct vendor_data vendor_sbsa = {
+	.oversampling		= false,
+	.dma_threshold		= false,
+	.cts_event_workaround	= false,
+	.always_enabled		= true,
+	.fixed_options		= "115200n8",
+};

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

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

Quoted 8 lines
Quoted 5 lines

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

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

So you mean that the firmware (or the vendor) inserts the actual values here, which the kernel and eventually userland can read?

Precisely.

Quoted 7 lines

Makes some sense, so what about:
-----------------------
Required properties:
- compatible: must be "arm,sbsa-uart"
- reg: exactly one register range
- interrupts: exactly one interrupt specifier

Quoted 11 lines

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

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

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

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

Thanks,
Mark.

Quoted 28 lines

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

Cheers,
Andre.

Quoted 17 lines
Quoted 9 lines
Quoted 3 lines

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

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

The fact that we cannot reconfigure it is orthogonal.

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

Mark.

#16 lore raw

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

Quoted 20 lines · On Friday 16 January 2015 18:37:17 Mark Rutland wrote:
Quoted 8 lines

Makes some sense, so what about:
-----------------------
Required properties:
- compatible: must be "arm,sbsa-uart"
- reg: exactly one register range
- interrupts: exactly one interrupt specifier

Quoted 11 lines

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

Quoted 8 lines

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

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

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

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

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

Arnd

#17 lore raw

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

Hi,

Quoted 30 lines · Arnd Bergmann
Quoted 20 lines · On Friday 16 January 2015 18:37:17 Mark Rutland wrote:
Quoted 8 lines

Makes some sense, so what about:
-----------------------
Required properties:
- compatible: must be "arm,sbsa-uart"
- reg: exactly one register range
- interrupts: exactly one interrupt specifier

Quoted 11 lines

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

Quoted 8 lines

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

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

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

Quoted 6 lines

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

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

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

Cheers,
Andre.

#18 lore raw

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

Quoted 10 lines · On Monday 19 January 2015 13:44:41 Andre Przywara wrote:
Quoted 3 lines

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

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

Ok, great!

Arnd

#19 lore raw

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

In reply to Andre Przywara
Quoted 11 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 the build a new struct uart_ops variable which points to some new functions avoiding access to the missing registers. We reuse as much existing PL011 code as possible.

Quoted 8 lines

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

---

<snip>

Andre,

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

Philip

Signature · 3 lines
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation Collaborative Project
#20 lore raw

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

Quoted 21 lines · Philip Elcan
Quoted 11 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 the build a new struct uart_ops variable which points to some new functions avoiding access to the missing registers. We reuse as much existing PL011 code as possible.

Quoted 8 lines

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

---

Quoted 8 lines

<snip>

Andre,

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

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

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

Andre may already have an answer on this.

Cheers
---Dave

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

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

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

#21 lore raw

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

Philip,

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

Quoted 34 lines · Dave P Martin
Quoted 21 lines · Philip Elcan
Quoted 11 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 the build a new struct uart_ops variable which points to some new functions avoiding access to the missing registers. We reuse as much existing PL011 code as possible.

Quoted 8 lines

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

---

Quoted 8 lines

<snip>

Andre,

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

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

Quoted 6 lines

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

Andre may already have an answer on this.

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

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

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

Cheers,
Andre.

#22 lore raw

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

Quoted 6 lines · Andre Przywara

Philip,

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

Quoted 1 line · Dave P Martin

On Tue, Feb 17, 2015 at 10:55:35AM -0500, Philip Elcan wrote:

[...]

Quoted 22 lines
Quoted 8 lines
Quoted 4 lines

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

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

Quoted 6 lines

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

Andre may already have an answer on this.

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

Quoted 8 lines

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

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

+1

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

Cheers
---Dave

#23 lore raw

cover drivers: PL011: add ARM SBSA Generic UART support

In reply to Andre Przywara
Quoted 7 lines · On Friday 16 January 2015 17:22:56 Andre Przywara wrote:

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

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

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

Arnd

#24 lore raw

cover drivers: PL011: add ARM SBSA Generic UART support

Hi Arnd,

Quoted 9 lines · Arnd Bergmann
Quoted 7 lines · On Friday 16 January 2015 17:22:56 Andre Przywara wrote:

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

Quoted 4 lines

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

Thanks for looking at them!

Quoted 3 lines

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

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

Cheers,
Andre.

#25 lore raw

cover drivers: PL011: add ARM SBSA Generic UART support

In reply to Andre Przywara
Quoted 34 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.

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

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

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

Quoted 4 lines

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

I have tested this series on Juno where it seems to work and also on FVP model where there are some issues.

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

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

My FVP is version 5602

Graeme

Quoted 34 lines

Cheers,
Andre

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

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

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
#26 lore raw

cover drivers: PL011: add ARM SBSA Generic UART support

Hi Graeme,

Quoted 39 lines · Graeme Gregory
Quoted 34 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.

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

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

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

Quoted 4 lines

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

Quoted 5 lines

I have tested this series on Juno where it seems to work and also on FVP model where there are some issues.

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

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

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

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

Cheers,
Andre.

Quoted 41 lines

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

My FVP is version 5602

Graeme

Quoted 34 lines

Cheers,
Andre

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

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

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
#27 lore raw

cover drivers: PL011: add ARM SBSA Generic UART support

Quoted 52 lines · Andre Przywara

Hi Graeme,

Quoted 39 lines · Graeme Gregory
Quoted 34 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.

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

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

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

Quoted 4 lines

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

Quoted 5 lines

I have tested this series on Juno where it seems to work and also on FVP model where there are some issues.

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

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

Quoted 6 lines

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

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

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

Graeme

#28 lore raw

cover drivers: PL011: add ARM SBSA Generic UART support

Quoted 56 lines · Graeme Gregory
Quoted 55 lines · Andre Przywara

Hi Graeme,

Quoted 39 lines · Graeme Gregory
Quoted 34 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.

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

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

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

Quoted 4 lines

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

Quoted 5 lines

I have tested this series on Juno where it seems to work and also on FVP model where there are some issues.

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

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

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

Quoted 6 lines
Quoted 3 lines

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

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

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

Cheers,
Andre.

#29 lore raw

cover drivers: PL011: add ARM SBSA Generic UART support

Quoted 66 lines · Andre Przywara
Quoted 56 lines · Graeme Gregory
Quoted 55 lines · Andre Przywara

Hi Graeme,

Quoted 39 lines · Graeme Gregory
Quoted 34 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.

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

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

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

Quoted 4 lines

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

Quoted 5 lines

I have tested this series on Juno where it seems to work and also on FVP model where there are some issues.

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

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

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

Quoted 6 lines
Quoted 3 lines

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

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

Quoted 7 lines

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

Yes with this setting all works with untimed_fifos=0

I was not aware of the differences in versions thanks.

Graeme

#30 lore raw

cover drivers: PL011: add ARM SBSA Generic UART support

Quoted 74 lines · Graeme Gregory
Quoted 66 lines · Andre Przywara
Quoted 56 lines · Graeme Gregory
Quoted 55 lines · Andre Przywara

Hi Graeme,

Quoted 39 lines · Graeme Gregory
Quoted 34 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.

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

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

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

Quoted 4 lines

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

Quoted 5 lines

I have tested this series on Juno where it seems to work and also on FVP model where there are some issues.

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

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

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

Quoted 6 lines
Quoted 3 lines

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

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

Quoted 7 lines

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

Quoted 4 lines

Yes with this setting all works with untimed_fifos=0

I was not aware of the differences in versions thanks.

Now I know how to correctly configure model I have now tested on FVP and Juno so feel free to add my Tested-by:

Graeme

#31 lore raw

cover drivers: PL011: add ARM SBSA Generic UART support

In reply to Graeme Gregory

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

[...]

Quoted 9 lines

I have tested this series on Juno where it seems to work and also on FVP model where there are some issues.

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

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

My FVP is version 5602

Can you try this:

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

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

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

Cheers
---Dave