> 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;
>
