> Date: Sat, 30 Apr 2022 09:40:24 +0200 > From: Anton Lindqvist <an...@basename.se> > > On Sun, Mar 13, 2022 at 04:17:07PM +0100, Mark Kettenis wrote: > > > Date: Fri, 11 Mar 2022 07:53:13 +0100 > > > From: Anton Lindqvist <an...@basename.se> > > > > > > On Tue, Mar 08, 2022 at 01:44:47PM +0000, Visa Hankala wrote: > > > > 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. > > > > > > I ended up tweaking the comments before committing. Thanks for all the > > > feedback. > > > > > > > Hi Anton, > > > > This diff seems to break things. When I boot my rpi4 it now prints: > > > > pluart0 at simplebus0: rev 0, 16 byte fifo > > pluart0: console > > bcmbsc0 at simplebus0 > > iic0 at bcmbsc0 > > > > so it appears that a carriage return character is lost here. > > > > Later on output stops at: > > > > reordering libraries: done. > > > > and only when I reboot the machine the login prompt appears, but with > > some wierd respawning: > > > > OpenBSD/arm64 (rutter.sibelius.xs4all.nl) (console) > > > > login: init: getty repeating too quickly on port /dev/console, sleeping > > init: getty repeating too quickly on port /dev/console, sleeping > > > > OpenBSD/arm64 (rutter.sibelius.xs4all.nl) (console) > > > > login: > > OpenBSD/arm64 (rutter.sibelius.xs4all.nl) (console) > > > > login: > > > > If you don't have a quick fix for this, may I suggest reverting the > > commit? We're heading towards release and we don't want the serial > > console on the rpi4 to be broken. > > Circling back to this: what happens is that no tx interrupt is raised > when sending less data than the configured interrupt fifo level, causing > the tty to end up in a forever busy state. Clearing the busy flag after > a successful transmission of all queued data solves the problem. > > While here, the hardware revision in the optional arm,primecell-periphid > fdt node have higher precedence. > > Testing would be much appreciated.
This fixes the issue I had with the previous diff. > diff --git sys/dev/acpi/pluart_acpi.c sys/dev/acpi/pluart_acpi.c > index e90e810e76f..284769d159e 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 7be34b37f19..a1d7e006f39 100644 > --- sys/dev/fdt/pluart_fdt.c > +++ sys/dev/fdt/pluart_fdt.c > @@ -63,12 +63,20 @@ pluart_fdt_attach(struct device *parent, struct device > *self, void *aux) > { > struct fdt_attach_args *faa = aux; > struct pluart_softc *sc = (struct pluart_softc *) self; > + uint32_t periphid; > > if (faa->fa_nreg < 1) { > printf(": no registers\n"); > return; > } > > + if (OF_is_compatible(faa->fa_node, "arm,sbsa-uart")) > + sc->sc_hwflags |= COM_HW_SBSA; > + > + periphid = OF_getpropint(faa->fa_node, "arm,primecell-periphid", 0); > + if (periphid != 0) > + sc->sc_hwrev = (periphid >> 20) & 0x0f; > + > 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 8a2b512eaeb..69654426a34 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,30 @@ 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) { > + if (sc->sc_hwrev == 0) > + sc->sc_hwrev = > UART_PID2_REV(bus_space_read_4(sc->sc_iot, > + sc->sc_ioh, UART_PID2)); > + if (sc->sc_hwrev < 3) > + fifolen = UART_FIFO_SIZE; > + else > + fifolen = UART_FIFO_SIZE_R3; > + printf(": rev %d, %d byte fifo\n", sc->sc_hwrev, 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 +197,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 +209,20 @@ 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) { > + 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,7 +242,8 @@ 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)) { > @@ -209,7 +255,7 @@ pluart_intr(void *arg) > > 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,22 +371,44 @@ 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(); > if (ISSET(tp->t_state, TS_BUSY | TS_TIMEOUT | TS_TTSTOP)) > goto out; > ttwakeupwr(tp); > - if (tp->t_outq.c_cc == 0) > + if (tp->t_outq.c_cc == 0) { > + /* Disable transmit interrupt. */ > + if (ISSET(sc->sc_imsc, UART_IMSC_TXIM)) { > + sc->sc_imsc &= ~UART_IMSC_TXIM; > + bus_space_write_4(sc->sc_iot, sc->sc_ioh, UART_IMSC, > + sc->sc_imsc); > + } > 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)); > + 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)); > } > + > + if (tp->t_outq.c_cc > 0) { > + /* Enable transmit interrupt. */ > + if (!ISSET(sc->sc_imsc, UART_IMSC_TXIM)) { > + sc->sc_imsc |= UART_IMSC_TXIM; > + bus_space_write_4(sc->sc_iot, sc->sc_ioh, UART_IMSC, > + sc->sc_imsc); > + } > + } else { > + CLR(tp->t_state, TS_BUSY); > + } > + > out: > splx(s); > } > diff --git sys/dev/ic/pluartvar.h sys/dev/ic/pluartvar.h > index 6fff43b249f..03854afa7d8 100644 > --- sys/dev/ic/pluartvar.h > +++ sys/dev/ic/pluartvar.h > @@ -34,10 +34,12 @@ struct pluart_softc { > u_int16_t sc_ucr3; > u_int16_t sc_ucr4; > u_int8_t sc_hwflags; > + u_int8_t sc_hwrev; > #define COM_HW_NOIEN 0x01 > #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 +47,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; >