On Tue, Mar 08, 2022 at 08:04:36AM +0100, Anton Lindqvist wrote:
> On Mon, Mar 07, 2022 at 07:36:35AM +0000, Visa Hankala wrote:
> > I still think that checking TXFF and using the same code for both
> > SBSA and true PL011 UARTs would be the best choice. This would avoid
> > fragmenting the code and improve robustness by relying on functionality
> > that is common to the different controller variants.
>
> Fair enough, new diff.
Maybe the comments should omit the FIFO space description and just
mention the lack of the level control register in the SBSA UART
register interface.
OK visa@
> diff --git sys/dev/acpi/pluart_acpi.c sys/dev/acpi/pluart_acpi.c
> index dc8ea5e9922..08ebe13ffbc 100644
> --- sys/dev/acpi/pluart_acpi.c
> +++ sys/dev/acpi/pluart_acpi.c
> @@ -91,6 +91,8 @@ pluart_acpi_attach(struct device *parent, struct device
> *self, void *aux)
> return;
> }
>
> + sc->sc.sc_hwflags |= COM_HW_SBSA;
> +
> pluart_attach_common(&sc->sc, pluart_acpi_is_console(sc));
> }
>
> diff --git sys/dev/fdt/pluart_fdt.c sys/dev/fdt/pluart_fdt.c
> index 7f17365f1d6..798250593bf 100644
> --- sys/dev/fdt/pluart_fdt.c
> +++ sys/dev/fdt/pluart_fdt.c
> @@ -69,6 +69,9 @@ pluart_fdt_attach(struct device *parent, struct device
> *self, void *aux)
> return;
> }
>
> + if (OF_is_compatible(faa->fa_node, "arm,sbsa-uart"))
> + sc->sc_hwflags |= COM_HW_SBSA;
> +
> 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..457c88d8cad 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,31 @@ 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_SBSA) == 0) {
> + 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 {
> + /*
> + * 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.
> + */
> + fifolen = 0;
> + printf("\n");
> + }
>
> if (console) {
> /* Locate the major number. */
> @@ -159,7 +198,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 +210,28 @@ 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 (fifolen > 0) {
> + /*
> + * 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 (fifolen > 0) {
> + lcr |= UART_LCR_H_FEN;
> + } else {
> + lcr &= ~UART_LCR_H_FEN;
> + }
> + bus_space_write_4(sc->sc_iot, sc->sc_ioh, UART_LCR_H, lcr);
> }
>
> int
> @@ -197,19 +251,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 +386,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 +396,19 @@ 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);
> +
> + while (tp->t_outq.c_cc > 0) {
> + uint16_t fr;
> +
> fr = bus_space_read_4(iot, ioh, UART_FR);
> + if (ISSET(fr, UART_FR_TXFF))
> + break;
> +
> + bus_space_write_4(iot, ioh, UART_DR, getc(&tp->t_outq));
> }
> +
> out:
> splx(s);
> }
> diff --git sys/dev/ic/pluartvar.h sys/dev/ic/pluartvar.h
> index 8fcbed14d5c..ba9748286ba 100644
> --- sys/dev/ic/pluartvar.h
> +++ sys/dev/ic/pluartvar.h
> @@ -38,6 +38,7 @@ struct pluart_softc {
> #define COM_HW_FIFO 0x02
> #define COM_HW_SIR 0x20
> #define COM_HW_CONSOLE 0x40
> +#define COM_HW_SBSA 0x80
> u_int8_t sc_swflags;
> #define COM_SW_SOFTCAR 0x01
> #define COM_SW_CLOCAL 0x02
> @@ -45,6 +46,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;
>
