On Sun, Feb 27, 2022 at 09:56:38AM +0100, Anton Lindqvist wrote:
> 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.
I think the documentation is strict about the (currently) reserved bits
so that any future revisions have the option to use them more easily.
Hence applying the mask on the revision field is mandatory.
> >
> > > +#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?
>
> 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
It looks that the 16-byte FIFO has been there from the beginning.
Therefore the _R2 suffix is misleading.
This would be better in my opinion:
#define UART_FIFO_SIZE 16
#define UART_FIFO_SIZE_R3 32
The driver's 3/4 fill level for the Tx FIFO can be derived by computation:
fifolen * 3 / 4
Alternatively, the driver could check the TXFF bit in the Flag Register
before feeding a byte into the Tx FIFO. This would be slightly slower,
though.
> +
> 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;
>
