On Tue, Mar 01, 2022 at 07:08:51AM +0100, Anton Lindqvist wrote:
> On Sun, Feb 27, 2022 at 05:33:17PM +0100, Mark Kettenis wrote:
> > > Date: Sun, 27 Feb 2022 16:01:25 +0100
> > > From: Anton Lindqvist <an...@basename.se>
> > >
> > > r1p5 is revision 3 with a 32 bytes FIFO. My only concern is that the
> > > following registers which this diff makes use of are not enumerated in
> > > the "Base UART Register Set" listing:
> > >
> > > 1. UARTPeriphID2 - used to deduce the revision
> > > 2. UARTIFLS - interrupt FIFO level select register
> >
> > Right. So you can't really use those registers, at least not
> > unconditionally.
> >
> > What we could do is add a flag to the softc to indicate that we're
> > dealing with an SBSA UART instead of a genuine PL011 UART. This would
> > skip reading the UARTPeriphID2 (assuming r1p5 properties) and skip
> > touching UARTIFLS (and perhaps some other potentially unimplemented
> > registers).
> >
> > We would set this flag based on the compatible string in pluart_fdt.c
> > and based on the _HID (or maybe even unconditionally) in
> > pluart_acpi.c.
>
> Here's a first stab at handling such devices. Attaching over ACPI also
> keeps the FIFO disabled.
>
> diff --git sys/dev/fdt/pluart_fdt.c sys/dev/fdt/pluart_fdt.c
> index 7f17365f1d6..45e31f9872f 100644
> --- sys/dev/fdt/pluart_fdt.c
> +++ sys/dev/fdt/pluart_fdt.c
> @@ -69,6 +69,16 @@ pluart_fdt_attach(struct device *parent, struct device
> *self, void *aux)
> return;
> }
>
> + /*
> + * The SBSA UART is PL011 r1p5 compliant which implies revision 3 with a
> + * 32 byte FIFO. However, we cannot expect to configure RX/TX interrupt
> + * levels using the UARTIFLS register making it impossible to make
> + * assumptions about the number of available bytes in the FIFO.
> + * Therefore disable FIFO support for such devices.
> + */
> + if (!OF_is_compatible(faa->fa_node, "arm,sbsa-uart"))
> + sc->sc_hwflags |= COM_HW_FIFO;
> +
Could this piece of code use a dedicated flag, say COM_HW_SBSA (or
inversely COM_HW_PL011), that tells that the UART is an SBSA UART (or
true PL011 UART).
To me, this use of the COM_HW_FIFO is a bit obscure; you need to look
at the comment in pluart_fdt_attach() to understand the logic.
Is worrying about the exact size of the FIFO really worthwhile? As I
suggested earlier, pluart_start() could check the TXFF flag in the
Flag Register before feeding each byte to the FIFO. This would allow
the same transmit code apply to both SBSA and true PL011 UARTs.
The SBSA UART interface does not have the IFLS register. However, it
looks that the FIFO and the interrupt are usable nevertheless. At least
Linux appears to enable them.
> sc->sc_irq = fdt_intr_establish(faa->fa_node, IPL_TTY, pluart_intr,
> sc, sc->sc_dev.dv_xname);
>
> diff --git sys/dev/ic/pluart.c sys/dev/ic/pluart.c
> index eaa11b6c44b..023fd66d776 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,16 @@
> #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_SIZE 16
> +#define UART_FIFO_SIZE_R3 32
> +
> void pluartcnprobe(struct consdev *cp);
> void pluartcninit(struct consdev *cp);
> int pluartcngetc(dev_t dev);
> @@ -150,7 +165,22 @@ struct cdevsw pluartdev =
> void
> pluart_attach_common(struct pluart_softc *sc, int console)
> {
> - int maj;
> + int fifolen, maj;
> + int lcr;
> +
> + if (sc->sc_hwflags & COM_HW_FIFO) {
> + int rev;
> +
> + rev = UART_PID2_REV(bus_space_read_4(sc->sc_iot, sc->sc_ioh,
> + UART_PID2));
> + if (rev < 3)
> + fifolen = UART_FIFO_SIZE;
> + else
> + fifolen = UART_FIFO_SIZE_R3;
> + printf(": rev %d, %d byte fifo\n", rev, fifolen);
> + } else {
> + printf("\n");
> + }
>
> if (console) {
> /* Locate the major number. */
> @@ -159,7 +189,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 +201,27 @@ 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));
> + if (sc->sc_hwflags & COM_HW_FIFO) {
> + /*
> + * 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.
> + */
> + sc->sc_fifolen = (fifolen * 3)/4;
> + 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);
> - 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");
> + lcr = bus_space_read_4(sc->sc_iot, sc->sc_ioh, UART_LCR_H);
> + if (sc->sc_hwflags & COM_HW_FIFO)
> + lcr |= UART_LCR_H;
> + else
> + lcr &= ~UART_LCR_H;
> + bus_space_write_4(sc->sc_iot, sc->sc_ioh, UART_LCR_H, lcr);
> }
>
> int
> @@ -197,19 +241,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
> @@ -325,7 +376,6 @@ pluart_start(struct tty *tp)
> 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;
>
> s = spltty();
> @@ -336,11 +386,26 @@ 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));
> + /* Enable transmit interrupt. */
> + bus_space_write_4(sc->sc_iot, sc->sc_ioh, UART_IMSC, sc->sc_imsc);
> +
> + if (sc->sc_hwflags & COM_HW_FIFO) {
> + char buf[UART_FIFO_SIZE_R3]; /* fifo max size */
> + int i, n;
> +
> + 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]);
> + } else {
> + u_int16_t fr;
> +
> 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);
> + }
> }
> +
> 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;
>
