> Date: Sun, 27 Feb 2022 16:01:25 +0100
> From: Anton Lindqvist <an...@basename.se>
>
> On Sun, Feb 27, 2022 at 11:08:14AM +0100, Mark Kettenis wrote:
> > > Date: Sun, 27 Feb 2022 09:56:38 +0100
> > > From: Anton Lindqvist <an...@basename.se>
> > >
> > > 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.
> > >
> > > >
> > > > > +#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?
> >
> > A potential problem with this diff is that pluart(4) is also used for
> > the so-called SBSA UART, which may only implement a subset of the
> > features of the genuine PL011. See Appendix B of the Server Base
> > System Architecture document (DEN0029) for information. It looks like
> > the FIFOs are part of that specification, but we should check.
>
> According to the specification:
>
> > The registers specified in this specification are a subset of the Arm
> > PL011 r1p5 UART. An instance of the PL011 r1p5 UART will be compliant
> > with this specification.
> >
> > An implementation of the Generic UART must provide a transmit FIFO and a
> > receive FIFO. Both FIFOs must have the same number of entries, and this
> > number must be at least 32. The generic UART does not support DMA
> > Features, Modem control features, Hardware flow control features, or
> > IrDA SIR features.
>
> 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.
