Date: Wed, 10 Dec 2014 23:14:10 +0100 From: Zbigniew Bodek <zbb@semihalf.com> To: "freebsd-arm@freebsd.org" <freebsd-arm@freebsd.org> Subject: Minor fixes to PL011 UART Message-ID: <CAG7dG%2BzAwmysuygugEh1mB767KPw5n1UB1YnotPSCdozcFC_yQ@mail.gmail.com>
next in thread | raw e-mail | index | archive | help
[-- Attachment #1 --]
Hello,
We have few minor fixes to ARM PL011 driver. The patch is attached to
this e-mail.
One change that might be considered controversial is adding
uart_barrier() to each __uart_write().
In other UART drivers uart_barrier() is used after every uart_write()
unless the write is related to the same register (but the other half).
In our case it would be necessary to add barrier after each write
anyway (if we like to follow this barrier convention)...
Nevertheless, please test and let me know what do you think.
I would like to push this by Monday.
Thanks and best regards
zbb
[-- Attachment #2 --]
From 0a4add83d34ec053ee2ae9184793ce32a2e26487 Mon Sep 17 00:00:00 2001
From: Zbigniew Bodek <zbb@semihalf.com>
Date: Tue, 9 Dec 2014 17:36:13 +0100
Subject: [PATCH] Fix some obvious mistakes in ARM PL011 UART
- Don't forget uart_barrier() on write.
- After disabling UART, wait for transmission to finish before
changing control and line. Timeout and fail if something's wrong
with the hardware.
- Check for break and overrun only on RX.
- Change masking of TXEMPTY interrupt to clearing it as this was
the original intention.
---
sys/dev/uart/uart_dev_pl011.c | 52 ++++++++++++++++++++++++++++++-------------
1 file changed, 37 insertions(+), 15 deletions(-)
diff --git a/sys/dev/uart/uart_dev_pl011.c b/sys/dev/uart/uart_dev_pl011.c
index d951070..bb1dfdf 100644
--- a/sys/dev/uart/uart_dev_pl011.c
+++ b/sys/dev/uart/uart_dev_pl011.c
@@ -50,6 +50,7 @@ __FBSDID("$FreeBSD$");
#define DR_OE (1 << 11) /* Overrun error */
#define UART_FR 0x06 /* Flag register */
+#define FR_BUSY (1 << 3) /* UART is busy */
#define FR_TXFF (1 << 5) /* Transmit FIFO/reg full */
#define FR_RXFF (1 << 6) /* Receive FIFO/reg full */
#define FR_TXFE (1 << 7) /* Transmit FIFO/reg empty */
@@ -93,8 +94,12 @@ __FBSDID("$FreeBSD$");
*/
#define __uart_getreg(bas, reg) \
bus_space_read_4((bas)->bst, (bas)->bsh, uart_regofs(bas, reg))
-#define __uart_setreg(bas, reg, value) \
- bus_space_write_4((bas)->bst, (bas)->bsh, uart_regofs(bas, reg), value)
+#define __uart_setreg(bas, reg, value) \
+do { \
+ bus_space_write_4((bas)->bst, \
+ (bas)->bsh, uart_regofs(bas, reg), value); \
+ uart_barrier(bas); \
+} while (0)
/*
* Low-level UART interface.
@@ -148,12 +153,13 @@ uart_pl011_probe(struct uart_bas *bas)
return (0);
}
-static void
+static int
uart_pl011_param(struct uart_bas *bas, int baudrate, int databits, int stopbits,
int parity)
{
uint32_t ctrl, line;
uint32_t baud;
+ size_t timeout;
/*
* Zero all settings to make sure
@@ -161,6 +167,19 @@ uart_pl011_param(struct uart_bas *bas, int baudrate, int databits, int stopbits,
*/
ctrl = line = 0x0;
__uart_setreg(bas, UART_CR, ctrl);
+
+ /*
+ * UART will finish pending transmissions before it's disabled.
+ * Wait until busy bit is cleared for a limited (but relatively
+ * significant) amount of time to avoid being trapped in an infinite
+ * loop.
+ */
+ timeout = 10 * 1024;
+ while ((__uart_getreg(bas, UART_FR) & FR_BUSY) && --timeout)
+ DELAY(4);
+
+ if (timeout == 0)
+ return (ENXIO);
/* As we know UART is disabled we may setup the line */
switch (databits) {
@@ -201,6 +220,8 @@ uart_pl011_param(struct uart_bas *bas, int baudrate, int databits, int stopbits,
~0xff) | line);
__uart_setreg(bas, UART_CR, ctrl);
+
+ return (0);
}
static void
@@ -211,7 +232,7 @@ uart_pl011_init(struct uart_bas *bas, int baudrate, int databits, int stopbits,
__uart_setreg(bas, UART_IMSC, __uart_getreg(bas, UART_IMSC) &
~IMSC_MASK_ALL);
- uart_pl011_param(bas, baudrate, databits, stopbits, parity);
+ (void)uart_pl011_param(bas, baudrate, databits, stopbits, parity);
}
static void
@@ -373,17 +394,18 @@ uart_pl011_bus_ipend(struct uart_softc *sc)
ints = __uart_getreg(bas, UART_MIS);
ipend = 0;
- if (ints & UART_RXREADY)
+ if (ints & UART_RXREADY) {
ipend |= SER_INT_RXREADY;
- if (ints & RIS_BE)
- ipend |= SER_INT_BREAK;
- if (ints & RIS_OE)
- ipend |= SER_INT_OVERRUN;
+ if (ints & RIS_BE)
+ ipend |= SER_INT_BREAK;
+ if (ints & RIS_OE)
+ ipend |= SER_INT_OVERRUN;
+ }
if (ints & UART_TXEMPTY) {
if (sc->sc_txbusy)
ipend |= SER_INT_TXIDLE;
- __uart_setreg(bas, UART_IMSC, UART_RXREADY);
+ __uart_setreg(bas, UART_ICR, UART_TXEMPTY);
}
uart_unlock(sc->sc_hwmtx);
@@ -395,12 +417,13 @@ static int
uart_pl011_bus_param(struct uart_softc *sc, int baudrate, int databits,
int stopbits, int parity)
{
+ int error;
uart_lock(sc->sc_hwmtx);
- uart_pl011_param(&sc->sc_bas, baudrate, databits, stopbits, parity);
+ error = uart_pl011_param(&sc->sc_bas, baudrate, databits, stopbits, parity);
uart_unlock(sc->sc_hwmtx);
- return (0);
+ return (error);
}
static int
@@ -466,10 +489,9 @@ uart_pl011_bus_transmit(struct uart_softc *sc)
bas = &sc->sc_bas;
uart_lock(sc->sc_hwmtx);
- for (i = 0; i < sc->sc_txdatasz; i++) {
+ for (i = 0; i < sc->sc_txdatasz; i++)
__uart_setreg(bas, UART_DR, sc->sc_txbuf[i]);
- uart_barrier(bas);
- }
+
sc->sc_txbusy = 1;
__uart_setreg(bas, UART_IMSC, (UART_RXREADY | UART_TXEMPTY));
uart_unlock(sc->sc_hwmtx);
--
2.1.2
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAG7dG%2BzAwmysuygugEh1mB767KPw5n1UB1YnotPSCdozcFC_yQ>