> Date: Sun, 27 Feb 2022 09:56:38 +0100
> From: Anton Lindqvist <an...@basename.se>
>
> On Sun, Feb 27, 2022 at 06:19:02AM +0000, Visa Hankala wrote:
> > 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.
>
> I trusted the bits of the documentation you referred to but lets play it
> safe.
>
> >
> > > +#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?
>
> Sure, fixed.
>
> >
> > >
> > > 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?
>
> Nice catch. That's indeed some last minute sloppy refactoring.
>
> >
> > I think the comment could be clearer and more concise; "this amount is
> > available" is vague in this context.
>
> I tweaked it to only talk about fractions, better?
A potential problem with this diff is that pluart(4) is also used for
the so-called SBSA UART, which may only implement a subset of the
features of the genuine PL011. See Appendix B of the Server Base
System Architecture document (DEN0029) for information. It looks like
the FIFOs are part of that specification, but we should check.
> diff --git sys/dev/ic/pluart.c sys/dev/ic/pluart.c
> index eaa11b6c44b..3479c16c698 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,8 +122,18 @@
> #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) & 0xf0) >> 4)
> +#define UART_PID3 0xfec /* Peripheral identification
> register 3 */
> #define UART_SPACE 0x100
>
> +#define UART_FIFO_RX_SIZE_R2 16
> +#define UART_FIFO_TX_SIZE_R2 12
> +#define UART_FIFO_RX_SIZE_R3 32
> +#define UART_FIFO_TX_SIZE_R3 24
> +
> void pluartcnprobe(struct consdev *cp);
> void pluartcninit(struct consdev *cp);
> int pluartcngetc(dev_t dev);
> @@ -150,7 +167,13 @@ 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 ? UART_FIFO_RX_SIZE_R2 : UART_FIFO_RX_SIZE_R3);
>
> if (console) {
> /* Locate the major number. */
> @@ -159,7 +182,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 +194,25 @@ 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 1/4 of the FIFO size is utilized.
> + */
> + if (rev < 3)
> + sc->sc_fifolen = UART_FIFO_TX_SIZE_R2;
> + else
> + sc->sc_fifolen = UART_FIFO_TX_SIZE_R3;
> + 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 +232,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 +364,11 @@ pluart_param(struct tty *tp, struct termios *t)
> void
> pluart_start(struct tty *tp)
> {
> + char buf[UART_FIFO_TX_SIZE_R3]; /* 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 +378,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;
>
>
