On Sat, Feb 26, 2022 at 08:40:25AM +0100, Anton Lindqvist wrote: > Hi, > This enables fifo support in pluart(4). While here, I changed the > attachment output to look more like com(4). Tested on my rpi3b which has > a 16 byte fifo. > > Comments? OK? > > diff --git sys/dev/ic/pluart.c sys/dev/ic/pluart.c > index eaa11b6c44b..601435c0e0c 100644 > --- sys/dev/ic/pluart.c > +++ sys/dev/ic/pluart.c > @@ -99,6 +99,13 @@ > #define UART_CR_CTSE (1 << 14) /* CTS hardware flow control > enable */ > #define UART_CR_RTSE (1 << 15) /* RTS hardware flow control > enable */ > #define UART_IFLS 0x34 /* Interrupt FIFO level select > register */ > +#define UART_IFLS_RX_SHIFT 3 /* RX level in bits [5:3] */ > +#define UART_IFLS_TX_SHIFT 0 /* TX level in bits [2:0] */ > +#define UART_IFLS_1_8 0 /* FIFO 1/8 full */ > +#define UART_IFLS_1_4 1 /* FIFO 1/4 full */ > +#define UART_IFLS_1_2 2 /* FIFO 1/2 full */ > +#define UART_IFLS_3_4 3 /* FIFO 3/4 full */ > +#define UART_IFLS_7_8 4 /* FIFO 7/8 full */ > #define UART_IMSC 0x38 /* Interrupt mask set/clear > register */ > #define UART_IMSC_RIMIM (1 << 0) > #define UART_IMSC_CTSMIM (1 << 1) > @@ -115,6 +122,11 @@ > #define UART_MIS 0x40 /* Masked interrupt status > register */ > #define UART_ICR 0x44 /* Interrupt clear register */ > #define UART_DMACR 0x48 /* DMA control register */ > +#define UART_PID0 0xfe0 /* Peripheral identification > register 0 */ > +#define UART_PID1 0xfe4 /* Peripheral identification > register 1 */ > +#define UART_PID2 0xfe8 /* Peripheral identification > register 2 */ > +#define UART_PID2_REV(x) ((x) >> 4)
The revision field covers bits 7:4. Should other bits be masked out? PL011 r1p5 documentation says that bits 15:8 must read as zeroes, but that does not necessarily tell what possible future revisions will do. > +#define UART_PID3 0xfec /* Peripheral identification > register 3 */ > #define UART_SPACE 0x100 > > void pluartcnprobe(struct consdev *cp); > @@ -150,7 +162,12 @@ struct cdevsw pluartdev = > void > pluart_attach_common(struct pluart_softc *sc, int console) > { > - int maj; > + int maj, rev; > + > + rev = UART_PID2_REV(bus_space_read_4(sc->sc_iot, sc->sc_ioh, > + UART_PID2)); > + > + printf(": rev %d, %d byte fifo\n", rev, rev < 3 ? 16 : 32); Should there be constants for the FIFO sizes? > > if (console) { > /* Locate the major number. */ > @@ -159,7 +176,7 @@ pluart_attach_common(struct pluart_softc *sc, int console) > break; > cn_tab->cn_dev = makedev(maj, sc->sc_dev.dv_unit); > > - printf(": console"); > + printf("%s: console\n", sc->sc_dev.dv_xname); > SET(sc->sc_hwflags, COM_HW_CONSOLE); > } > > @@ -171,13 +188,26 @@ pluart_attach_common(struct pluart_softc *sc, int > console) > panic("%s: can't establish soft interrupt.", > sc->sc_dev.dv_xname); > > - bus_space_write_4(sc->sc_iot, sc->sc_ioh, UART_IMSC, (UART_IMSC_RXIM | > UART_IMSC_TXIM)); > + /* > + * The transmit FIFO size is set to 3/4 of the actual size as interrupts > + * can only be triggered when the FIFO is partially full. Once > + * triggered, we know that at least this amount is available in the > + * FIFO. > + */ > + if (rev < 3) > + sc->sc_fifolen = 24; > + else > + sc->sc_fifolen = 12; Have the assignments above been swapped by accident? I think the comment could be clearer and more concise; "this amount is available" is vague in this context. > + bus_space_write_4(sc->sc_iot, sc->sc_ioh, UART_IFLS, > + (UART_IFLS_3_4 << UART_IFLS_RX_SHIFT) | > + (UART_IFLS_1_4 << UART_IFLS_TX_SHIFT)); > + sc->sc_imsc = UART_IMSC_RXIM | UART_IMSC_RTIM | UART_IMSC_TXIM; > + bus_space_write_4(sc->sc_iot, sc->sc_ioh, UART_IMSC, sc->sc_imsc); > bus_space_write_4(sc->sc_iot, sc->sc_ioh, UART_ICR, 0x7ff); > + /* Enable FIFO. */ > bus_space_write_4(sc->sc_iot, sc->sc_ioh, UART_LCR_H, > - bus_space_read_4(sc->sc_iot, sc->sc_ioh, UART_LCR_H) & > - ~UART_LCR_H_FEN); > - > - printf("\n"); > + bus_space_read_4(sc->sc_iot, sc->sc_ioh, UART_LCR_H) | > + UART_LCR_H_FEN); > } > > int > @@ -197,19 +227,26 @@ pluart_intr(void *arg) > if (sc->sc_tty == NULL) > return 0; > > - if (!ISSET(is, UART_IMSC_RXIM) && !ISSET(is, UART_IMSC_TXIM)) > + if (!ISSET(is, UART_IMSC_RXIM) && !ISSET(is, UART_IMSC_RTIM) && > + !ISSET(is, UART_IMSC_TXIM)) > return 0; > > - if (ISSET(is, UART_IMSC_TXIM) && ISSET(tp->t_state, TS_BUSY)) { > - CLR(tp->t_state, TS_BUSY | TS_FLUSH); > - if (sc->sc_halt > 0) > - wakeup(&tp->t_outq); > - (*linesw[tp->t_line].l_start)(tp); > + if (ISSET(is, UART_IMSC_TXIM)) { > + /* Disable transmit interrupt. */ > + bus_space_write_4(sc->sc_iot, sc->sc_ioh, UART_IMSC, > + sc->sc_imsc & ~UART_IMSC_TXIM); > + > + if (ISSET(tp->t_state, TS_BUSY)) { > + CLR(tp->t_state, TS_BUSY | TS_FLUSH); > + if (sc->sc_halt > 0) > + wakeup(&tp->t_outq); > + (*linesw[tp->t_line].l_start)(tp); > + } > } > > p = sc->sc_ibufp; > > - while (ISSET(bus_space_read_4(iot, ioh, UART_FR), UART_FR_RXFF)) { > + while (!ISSET(bus_space_read_4(iot, ioh, UART_FR), UART_FR_RXFE)) { > c = bus_space_read_2(iot, ioh, UART_DR); > if (c & UART_DR_BE) { > #ifdef DDB > @@ -322,11 +359,11 @@ pluart_param(struct tty *tp, struct termios *t) > void > pluart_start(struct tty *tp) > { > + char buf[32]; /* fifo max size */ > struct pluart_softc *sc = pluart_cd.cd_devs[DEVUNIT(tp->t_dev)]; > bus_space_tag_t iot = sc->sc_iot; > bus_space_handle_t ioh = sc->sc_ioh; > - u_int16_t fr; > - int s; > + int i, n, s; > > s = spltty(); > if (ISSET(tp->t_state, TS_BUSY | TS_TIMEOUT | TS_TTSTOP)) > @@ -336,11 +373,13 @@ pluart_start(struct tty *tp) > goto out; > SET(tp->t_state, TS_BUSY); > > - fr = bus_space_read_4(iot, ioh, UART_FR); > - while (tp->t_outq.c_cc != 0 && ISSET(fr, UART_FR_TXFE)) { > - bus_space_write_4(iot, ioh, UART_DR, getc(&tp->t_outq)); > - fr = bus_space_read_4(iot, ioh, UART_FR); > - } > + /* Enable transmit interrupt. */ > + bus_space_write_4(sc->sc_iot, sc->sc_ioh, UART_IMSC, sc->sc_imsc); > + > + n = q_to_b(&tp->t_outq, buf, sc->sc_fifolen); > + for (i = 0; i < n; i++) > + bus_space_write_4(iot, ioh, UART_DR, buf[i]); > + > out: > splx(s); > } > diff --git sys/dev/ic/pluartvar.h sys/dev/ic/pluartvar.h > index 8fcbed14d5c..5181723a066 100644 > --- sys/dev/ic/pluartvar.h > +++ sys/dev/ic/pluartvar.h > @@ -45,6 +45,7 @@ struct pluart_softc { > #define COM_SW_MDMBUF 0x08 > #define COM_SW_PPS 0x10 > int sc_fifolen; > + int sc_imsc; > > u_int8_t sc_initialize; > u_int8_t sc_cua; >