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
