Differential D20072 Diff 59528 sys/dev/uart/uart_dev_ns8250.c

Changeset View

Standalone View

sys/dev/uart/uart_dev_ns8250.c

Show First 20 Lines • Show All 58 Lines • ▼ Show 20 Lines
#define DEFAULT_RCLK 1843200		#define DEFAULT_RCLK 1843200

static int broken_txfifo = 0;		static int broken_txfifo = 0;
SYSCTL_INT(_hw, OID_AUTO, broken_txfifo, CTLFLAG_RW \| CTLFLAG_TUN,		SYSCTL_INT(_hw, OID_AUTO, broken_txfifo, CTLFLAG_RW \| CTLFLAG_TUN,
&broken_txfifo, 0, "UART FIFO has QEMU emulation bug");		&broken_txfifo, 0, "UART FIFO has QEMU emulation bug");
TUNABLE_INT("hw.broken_txfifo", &broken_txfifo);		TUNABLE_INT("hw.broken_txfifo", &broken_txfifo);

/*		/*
		* Clear the saved LSR value.
		*/
		static void
		ns8250_clear_saved_lsr(struct ns8250_softc *ns8250)
		{
		ns8250->lsr_saved_flags = 0;
		}

		/*
		* Update the LSR saved value, using bit OR between new and existing value
		*/
		static void
		ns8250_update_saved_lsr(struct ns8250_softc *ns8250, uint8_t lsr)
		{
		ns8250->lsr_saved_flags \|= (lsr & LSR_SAVE_FLAGS);
		}

		/*
* Clear pending interrupts. THRE is cleared by reading IIR. Data		* Clear pending interrupts. THRE is cleared by reading IIR. Data
* that may have been received gets lost here.		* that may have been received gets lost here.
*/		*/
static void		static void
ns8250_clrint(struct uart_bas *bas)		ns8250_clrint(struct uart_bas *bas)
{		{
uint8_t iir, lsr;		uint8_t iir, lsr;

iir = uart_getreg(bas, REG_IIR);		iir = uart_getreg(bas, REG_IIR);
		impUnsubmitted Not Done Inline Actions why do we clear it here? Does the hardware clear it? this seems like exactly the case where we'd NOT want to clear it. imp: why do we clear it here? Does the hardware clear it? this seems like exactly the case where…
		smaryus_gmail.comAuthorUnsubmitted Done Inline Actions Maybe it makes more sense to be moved into the while on line 83, but save the value for the else on line 82 `if (lsr & (LSR_BI\|LSR_FE\|LSR_PE)` The hardware clears the error bits every time there is a `uart_getreg(bas, REG_LSR)`. Also if there is a `uart_getreg(bas, REG_DATA)` then there must also be a LSR read, if not the errors flags are not anymore in sync with the data. smaryus_gmail.com: Maybe it makes more sense to be moved into the while on line 83, but save the value for the…
while ((iir & IIR_NOPEND) == 0) {		while ((iir & IIR_NOPEND) == 0) {
iir &= IIR_IMASK;		iir &= IIR_IMASK;
if (iir == IIR_RLS) {		if (iir == IIR_RLS) {
lsr = uart_getreg(bas, REG_LSR);		lsr = uart_getreg(bas, REG_LSR);
if (lsr & (LSR_BI\|LSR_FE\|LSR_PE))		if (lsr & (LSR_BI\|LSR_FE\|LSR_PE)) {
(void)uart_getreg(bas, REG_DATA);		(void)uart_getreg(bas, REG_DATA);
} else if (iir == IIR_RXRDY \|\| iir == IIR_RXTOUT)		ns8250_clear_saved_lsr((struct ns8250_softc *)bas);
		}
		} else if (iir == IIR_RXRDY \|\| iir == IIR_RXTOUT) {
(void)uart_getreg(bas, REG_DATA);		(void)uart_getreg(bas, REG_DATA);
else if (iir == IIR_MLSC)		ns8250_clear_saved_lsr((struct ns8250_softc *)bas);
		} else if (iir == IIR_MLSC)
(void)uart_getreg(bas, REG_MSR);		(void)uart_getreg(bas, REG_MSR);
uart_barrier(bas);		uart_barrier(bas);
iir = uart_getreg(bas, REG_IIR);		iir = uart_getreg(bas, REG_IIR);
}		}
}		}

static int		static int
ns8250_delay(struct uart_bas *bas)		ns8250_delay(struct uart_bas *bas)
▲ Show 20 Lines • Show All 41 Lines • ▼ Show 20 Lines

static int		static int
ns8250_drain(struct uart_bas *bas, int what)		ns8250_drain(struct uart_bas *bas, int what)
{		{
int delay, limit;		int delay, limit;

delay = ns8250_delay(bas);		delay = ns8250_delay(bas);

if (what & UART_DRAIN_TRANSMITTER) {		if (what & UART_DRAIN_TRANSMITTER) {
		impUnsubmitted Not Done Inline Actions Why clear it here? imp: Why clear it here?
		smaryus_gmail.comAuthorUnsubmitted Done Inline Actions I'll move this into the `if (what & UART_DRAIN_RECEIVER)`, it might make more sense to clear it only then. But there I think is needed, because there is a `uart_getreg(bas, REG_LSR)` follow by `uart_getreg(bas, REG_DATA)` , to be set to `0` or to the latest `LSR` value. But since LSR is read until there is no data then saving the value it might not make sense, since it is used only into then reading. smaryus_gmail.com: I'll move this into the `if (what & UART_DRAIN_RECEIVER)`, it might make more sense to clear it…
/*		/*
* Pick an arbitrary high limit to avoid getting stuck in		* Pick an arbitrary high limit to avoid getting stuck in
* an infinite loop when the hardware is broken. Make the		* an infinite loop when the hardware is broken. Make the
* limit high enough to handle large FIFOs.		* limit high enough to handle large FIFOs.
*/		*/
limit = 10*1024;		limit = 10*1024;
while ((uart_getreg(bas, REG_LSR) & LSR_TEMT) == 0 && --limit)		while ((uart_getreg(bas, REG_LSR) & LSR_TEMT) == 0 && --limit)
DELAY(delay);		DELAY(delay);
if (limit == 0) {		if (limit == 0) {
/* printf("ns8250: transmitter appears stuck... "); */		/* printf("ns8250: transmitter appears stuck... "); */
return (EIO);		return (EIO);
}		}
}		}

if (what & UART_DRAIN_RECEIVER) {		if (what & UART_DRAIN_RECEIVER) {
/*		/*
* Pick an arbitrary high limit to avoid getting stuck in		* Pick an arbitrary high limit to avoid getting stuck in
* an infinite loop when the hardware is broken. Make the		* an infinite loop when the hardware is broken. Make the
* limit high enough to handle large FIFOs and integrated		* limit high enough to handle large FIFOs and integrated
* UARTs. The HP rx2600 for example has 3 UARTs on the		* UARTs. The HP rx2600 for example has 3 UARTs on the
* management board that tend to get a lot of data send		* management board that tend to get a lot of data send
* to it when the UART is first activated.		* to it when the UART is first activated.
*/		*/
		ns8250_clear_saved_lsr((struct ns8250_softc *)bas);
limit=10*4096;		limit=10*4096;
while ((uart_getreg(bas, REG_LSR) & LSR_RXRDY) && --limit) {		while ((uart_getreg(bas, REG_LSR) & LSR_RXRDY) && --limit) {
(void)uart_getreg(bas, REG_DATA);		(void)uart_getreg(bas, REG_DATA);
uart_barrier(bas);		uart_barrier(bas);
DELAY(delay << 2);		DELAY(delay << 2);
}		}
if (limit == 0) {		if (limit == 0) {
/* printf("ns8250: receiver appears broken... "); */		/* printf("ns8250: receiver appears broken... "); */
Show All 11 Lines
static void		static void
ns8250_flush(struct uart_bas *bas, int what)		ns8250_flush(struct uart_bas *bas, int what)
{		{
uint8_t fcr;		uint8_t fcr;

fcr = FCR_ENABLE;		fcr = FCR_ENABLE;
if (what & UART_FLUSH_TRANSMITTER)		if (what & UART_FLUSH_TRANSMITTER)
fcr \|= FCR_XMT_RST;		fcr \|= FCR_XMT_RST;
if (what & UART_FLUSH_RECEIVER)		if (what & UART_FLUSH_RECEIVER) {
fcr \|= FCR_RCV_RST;		fcr \|= FCR_RCV_RST;
		ns8250_clear_saved_lsr((struct ns8250_softc *)bas);
		impUnsubmitted Not Done Inline Actions I'd be tempted to have a ns8250_clear_saved_lsr() function instead. It would be fewer style violations and easier to grok. imp: I'd be tempted to have a ns8250_clear_saved_lsr() function instead. It would be fewer style…
		smaryus_gmail.comAuthorUnsubmitted Done Inline Actions Yes, I'll make a function. smaryus_gmail.com: Yes, I'll make a function.
		}
uart_setreg(bas, REG_FCR, fcr);		uart_setreg(bas, REG_FCR, fcr);
uart_barrier(bas);		uart_barrier(bas);
}		}

static int		static int
ns8250_param(struct uart_bas *bas, int baudrate, int databits, int stopbits,		ns8250_param(struct uart_bas *bas, int baudrate, int databits, int stopbits,
int parity)		int parity)
{		{
int divisor;		int divisor;
uint8_t lcr;		uint8_t lcr;

		ns8250_clear_saved_lsr((struct ns8250_softc *)bas);
lcr = 0;		lcr = 0;
if (databits >= 8)		if (databits >= 8)
lcr \|= LCR_8BITS;		lcr \|= LCR_8BITS;
else if (databits == 7)		else if (databits == 7)
lcr \|= LCR_7BITS;		lcr \|= LCR_7BITS;
else if (databits == 6)		else if (databits == 6)
lcr \|= LCR_6BITS;		lcr \|= LCR_6BITS;
else		else
▲ Show 20 Lines • Show All 114 Lines • ▼ Show 20 Lines	ns8250_putc(struct uart_bas *bas, int c)
limit = 250000;		limit = 250000;
while ((uart_getreg(bas, REG_LSR) & LSR_TEMT) == 0 && --limit)		while ((uart_getreg(bas, REG_LSR) & LSR_TEMT) == 0 && --limit)
DELAY(4);		DELAY(4);
}		}

static int		static int
ns8250_rxready(struct uart_bas *bas)		ns8250_rxready(struct uart_bas *bas)
{		{
		uint8_t lsr;

return ((uart_getreg(bas, REG_LSR) & LSR_RXRDY) != 0 ? 1 : 0);		lsr = uart_getreg(bas, REG_LSR);
		ns8250_update_saved_lsr((struct ns8250_softc *)bas, lsr);
		return ((lsr & LSR_RXRDY) != 0 ? 1 : 0);
		impUnsubmitted Not Done Inline Actions I'd be tempted to have a read_lsr here. Also, why aren't we \|= the bits and then clearing them as we process them? imp: I'd be tempted to have a read_lsr here. Also, why aren't we \|= the bits and then clearing them…
		smaryus_gmail.comAuthorUnsubmitted Done Inline Actions Yes, it can be a function to read `LSR` and save it if is needed into the `lsr_saved_flags` and be called only where is needed. Because there are places where LSR is read but the value doesn't need to be save. Regarding \|= as far as I saw the only problem with `LSR` is when data is read for parity errors. So I've thought not to complicate the logic more and break something else, since everything else works fine. smaryus_gmail.com: Yes, it can be a function to read `LSR` and save it if is needed into the `lsr_saved_flags` and…
}		}

static int		static int
ns8250_getc(struct uart_bas bas, struct mtx hwmtx)		ns8250_getc(struct uart_bas bas, struct mtx hwmtx)
{		{
int c;		int c;

uart_lock(hwmtx);		uart_lock(hwmtx);

while ((uart_getreg(bas, REG_LSR) & LSR_RXRDY) == 0) {		while ((uart_getreg(bas, REG_LSR) & LSR_RXRDY) == 0) {
		impUnsubmitted Not Done Inline Actions why read it twice, here and below. I don't understand that. It adds a lot of inb's to the critical path. imp: why read it twice, here and below. I don't understand that. It adds a lot of inb's to the…
		smaryus_gmail.comAuthorUnsubmitted Done Inline Actions I'll change this and call the future function that will read & save `LSR`. smaryus_gmail.com: I'll change this and call the future function that will read & save `LSR`.
uart_unlock(hwmtx);		uart_unlock(hwmtx);
DELAY(4);		DELAY(4);
uart_lock(hwmtx);		uart_lock(hwmtx);
}		}

c = uart_getreg(bas, REG_DATA);		c = uart_getreg(bas, REG_DATA);
		ns8250_clear_saved_lsr((struct ns8250_softc*)bas);

uart_unlock(hwmtx);		uart_unlock(hwmtx);

return (c);		return (c);
}		}

static kobj_method_t ns8250_methods[] = {		static kobj_method_t ns8250_methods[] = {
KOBJMETHOD(uart_attach, ns8250_bus_attach),		KOBJMETHOD(uart_attach, ns8250_bus_attach),
▲ Show 20 Lines • Show All 316 Lines • ▼ Show 20 Lines	ns8250_bus_ipend(struct uart_softc *sc)
}		}
if (iir & IIR_NOPEND) {		if (iir & IIR_NOPEND) {
uart_unlock(sc->sc_hwmtx);		uart_unlock(sc->sc_hwmtx);
return (0);		return (0);
}		}
ipend = 0;		ipend = 0;
if (iir & IIR_RXRDY) {		if (iir & IIR_RXRDY) {
lsr = uart_getreg(bas, REG_LSR);		lsr = uart_getreg(bas, REG_LSR);
if (lsr & LSR_OE)		if (lsr & LSR_OE) {
ipend \|= SER_INT_OVERRUN;		ipend \|= SER_INT_OVERRUN;
if (lsr & LSR_BI)		lsr &= ~LSR_OE;
		}
		if (lsr & LSR_BI) {
ipend \|= SER_INT_BREAK;		ipend \|= SER_INT_BREAK;
if (lsr & LSR_RXRDY)		lsr &= ~LSR_BI;
		}
		impUnsubmitted Not Done Inline Actions here we're dealing with the lsr bits. We should CLEAR them as we add stuff to ipend. imp: here we're dealing with the lsr bits. We should CLEAR them as we add stuff to ipend.
		smaryus_gmail.comAuthorUnsubmitted Done Inline Actions Ok, I'll clear them. smaryus_gmail.com: Ok, I'll clear them.
		if (lsr & LSR_RXRDY) {
ipend \|= SER_INT_RXREADY;		ipend \|= SER_INT_RXREADY;
		lsr &= ~LSR_RXRDY;
		}
		ns8250_update_saved_lsr(ns8250, lsr);
} else {		} else {
if (iir & IIR_TXRDY) {		if (iir & IIR_TXRDY) {
ipend \|= SER_INT_TXIDLE;		ipend \|= SER_INT_TXIDLE;
uart_setreg(bas, REG_IER, ns8250->ier);		uart_setreg(bas, REG_IER, ns8250->ier);
} else		} else
ipend \|= SER_INT_SIGCHG;		ipend \|= SER_INT_SIGCHG;
}		}
if (ipend == 0)		if (ipend == 0)
▲ Show 20 Lines • Show All 189 Lines • ▼ Show 20 Lines	#endif

return (0);		return (0);
}		}

int		int
ns8250_bus_receive(struct uart_softc *sc)		ns8250_bus_receive(struct uart_softc *sc)
{		{
struct uart_bas *bas;		struct uart_bas *bas;
		struct ns8250_softc* ns8250;
int xc;		int xc;
uint8_t lsr;		uint8_t lsr;

bas = &sc->sc_bas;		bas = &sc->sc_bas;
uart_lock(sc->sc_hwmtx);		uart_lock(sc->sc_hwmtx);
lsr = uart_getreg(bas, REG_LSR);		ns8250 = (struct ns8250_softc*)sc;
		lsr = uart_getreg(bas, REG_LSR) \| ns8250->lsr_saved_flags;
		ns8250->lsr_saved_flags = 0;

while (lsr & LSR_RXRDY) {		while (lsr & LSR_RXRDY) {
if (uart_rx_full(sc)) {		if (uart_rx_full(sc)) {
sc->sc_rxbuf[sc->sc_rxput] = UART_STAT_OVERRUN;		sc->sc_rxbuf[sc->sc_rxput] = UART_STAT_OVERRUN;
break;		break;
}		}
xc = uart_getreg(bas, REG_DATA);		xc = uart_getreg(bas, REG_DATA);
if (lsr & LSR_FE)		if (lsr & LSR_FE)
xc \|= UART_STAT_FRAMERR;		xc \|= UART_STAT_FRAMERR;
▲ Show 20 Lines • Show All 102 Lines • Show Last 20 Lines