Re: pluart(4): fifo support
On Fri, Jun 10, 2022 at 01:15:12PM +, Visa Hankala wrote:
> On Wed, Jun 08, 2022 at 06:50:18AM +0200, Anton Lindqvist wrote:
> > On Sun, May 01, 2022 at 04:17:34PM +, Visa Hankala wrote:
> > > On Sat, Apr 30, 2022 at 09:40:24AM +0200, Anton Lindqvist wrote:
> > > > 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
> > > > > >
> > > > > > On Tue, Mar 08, 2022 at 01:44:47PM +, 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 +, 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.
> > >
> > > Are you sure about the behaviour of the interrupt?
> > >
> > > One possible problem is that pluart_intr() uses the raw, unmasked,
> > > interrupt status to clear interrupts. Your previous patch always
> > > disabled the Tx interrupt whenever the raw status indicated a Tx FIFO
> > > level event.
> > >
> > > This new patch might very well be correct. However, it feels strange
> > > if the hardware raises the Tx interrupt only at one specific level of
> > > FIFO state change.
> > >
> > > It would be nice to know if a comstart()-style arrangement of interrupt
> > > masking worked in pluart_start().
> >
> > What did work was to not clear the tx interrupt in pluart_intr().
> > Updated diff with some additional changes:
> >
> > * Flush any pending transmission before configuring the device during
> > attachment. Prevents the next dmesg line from being mangled.
> >
> > * Make pluart_start() mimic comstart() as proposed by visa@.
> >
> > * While entering ddb (i.e. poll mode), disable interrupts.
>
>
>
> > @@ -792,4 +870,13 @@ pluartcnputc(dev_t dev, int c)
> > void
> > pluartcnpollc(dev_t dev, int on)
> > {
> > + int s;
> > +
> > + s = splhigh();
> > + if (on)
> > + bus_space_write_4(pluartconsiot, pluartconsioh, UART_IMSC, 0);
> > + else
> > + bus_space_write_4(pluartconsiot, pluartconsioh, UART_IMSC,
> > + UART_IMSC_RXIM | UART_IMSC_RTIM);
> > + splx(s);
> > }
>
> Does this fix an actual issue? If not, I would leave it out. ddb entry
> can happen in unexpected places. There is a risk that the mask gets
> messed up in particular when leaving the debugger.
It's not. I will drop this chunk before getting this committed.
Re: pluart(4): fifo support
On Wed, Jun 08, 2022 at 06:50:18AM +0200, Anton Lindqvist wrote:
> On Sun, May 01, 2022 at 04:17:34PM +, Visa Hankala wrote:
> > On Sat, Apr 30, 2022 at 09:40:24AM +0200, Anton Lindqvist wrote:
> > > 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
> > > > >
> > > > > On Tue, Mar 08, 2022 at 01:44:47PM +, 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 +, 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.
> >
> > Are you sure about the behaviour of the interrupt?
> >
> > One possible problem is that pluart_intr() uses the raw, unmasked,
> > interrupt status to clear interrupts. Your previous patch always
> > disabled the Tx interrupt whenever the raw status indicated a Tx FIFO
> > level event.
> >
> > This new patch might very well be correct. However, it feels strange
> > if the hardware raises the Tx interrupt only at one specific level of
> > FIFO state change.
> >
> > It would be nice to know if a comstart()-style arrangement of interrupt
> > masking worked in pluart_start().
>
> What did work was to not clear the tx interrupt in pluart_intr().
> Updated diff with some additional changes:
>
> * Flush any pending transmission before configuring the device during
> attachment. Prevents the next dmesg line from being mangled.
>
> * Make pluart_start() mimic comstart() as proposed by visa@.
>
> * While entering ddb (i.e. poll mode), disable interrupts.
> @@ -792,4 +870,13 @@ pluartcnputc(dev_t dev, int c)
> void
> pluartcnpollc(dev_t dev, int on)
> {
> + int s;
> +
> + s = splhigh();
> + if (on)
> + bus_space_write_4(pluartconsiot, pluartconsioh, UART_IMSC, 0);
> + else
> + bus_space_write_4(pluartconsiot, pluartconsioh, UART_IMSC,
> + UART_IMSC_RXIM | UART_IMSC_RTIM);
> + splx(s);
> }
Does this fix an actual issue? If not, I would leave it out. ddb entry
can happen in unexpected places. There is a risk that the mask gets
messed up in particular when leaving the debugger.
Otherwise, OK visa@
Re: pluart(4): fifo support
On Sun, May 01, 2022 at 04:17:34PM +, Visa Hankala wrote:
> On Sat, Apr 30, 2022 at 09:40:24AM +0200, Anton Lindqvist wrote:
> > 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
> > > >
> > > > On Tue, Mar 08, 2022 at 01:44:47PM +, 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 +, 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.
>
> Are you sure about the behaviour of the interrupt?
>
> One possible problem is that pluart_intr() uses the raw, unmasked,
> interrupt status to clear interrupts. Your previous patch always
> disabled the Tx interrupt whenever the raw status indicated a Tx FIFO
> level event.
>
> This new patch might very well be correct. However, it feels strange
> if the hardware raises the Tx interrupt only at one specific level of
> FIFO state change.
>
> It would be nice to know if a comstart()-style arrangement of interrupt
> masking worked in pluart_start().
What did work was to not clear the tx interrupt in pluart_intr().
Updated diff with some additional changes:
* Flush any pending transmission before configuring the device during
attachment. Prevents the next dmesg line from being mangled.
* Make pluart_start() mimic comstart() as proposed by visa@.
* While entering ddb (i.e. poll mode), disable interrupts.
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..6264723266e 100644
--- sys/dev/ic/pluart.c
+++ sys/dev/ic/pluart.c
@@ -99,6 +9
Re: pluart(4): fifo support
On Sat, Apr 30, 2022 at 09:40:24AM +0200, Anton Lindqvist wrote: > 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 > > > > > > On Tue, Mar 08, 2022 at 01:44:47PM +, 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 +, 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. Are you sure about the behaviour of the interrupt? One possible problem is that pluart_intr() uses the raw, unmasked, interrupt status to clear interrupts. Your previous patch always disabled the Tx interrupt whenever the raw status indicated a Tx FIFO level event. This new patch might very well be correct. However, it feels strange if the hardware raises the Tx interrupt only at one specific level of FIFO state change. It would be nice to know if a comstart()-style arrangement of interrupt masking worked in pluart_start().
Re: pluart(4): fifo support
> Date: Sat, 30 Apr 2022 09:40:24 +0200
> From: Anton Lindqvist
>
> 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
> > >
> > > On Tue, Mar 08, 2022 at 01:44:47PM +, 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 +, 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_IFLS0x34/* 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_80 /* FIFO 1/8 full */
> +#define UART_IFLS_1_41 /* FIFO 1/4 full */
> +#define UART_IFLS_1_22 /* FIFO 1/2 full */
> +#define UART_IFLS_3_43 /* FIFO 3/4 full */
> +#define UART_IFLS_7_84 /* FIFO 7/8 full */
> #define UART_IMSC0x38/*
Re: pluart(4): fifo support
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
> >
> > On Tue, Mar 08, 2022 at 01:44:47PM +, 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 +, 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.
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
Re: pluart(4): fifo support
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 > > > > On Tue, Mar 08, 2022 at 01:44:47PM +, 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 +, 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. I don't have a quick fix. I'm not able to revert until Tuesday, could you or anyone else go ahead and revert the commit? Would be much appreciated.
Re: pluart(4): fifo support
> Date: Fri, 11 Mar 2022 07:53:13 +0100 > From: Anton Lindqvist > > On Tue, Mar 08, 2022 at 01:44:47PM +, 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 +, 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. Cheers, Mark
Re: pluart(4): fifo support
On Tue, Mar 08, 2022 at 01:44:47PM +, 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 +, 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.
Re: pluart(4): fifo support
On Tue, Mar 08, 2022 at 08:04:36AM +0100, Anton Lindqvist wrote:
> On Mon, Mar 07, 2022 at 07:36:35AM +, 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_IFLS0x34/* 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_80 /* FIFO 1/8 full */
> +#define UART_IFLS_1_41 /* FIFO 1/4 full */
> +#define UART_IFLS_1_22 /* FIFO 1/2 full */
> +#define UART_IFLS_3_43 /* FIFO 3/4 full */
> +#define UART_IFLS_7_84 /* FIFO 7/8 full */
> #define UART_IMSC0x38/* 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_PID00xfe0 /* Peripheral identification
> register 0 */
> +#define UART_PID10xfe4 /* Peripheral identification
> register 1 */
> +#define UART_PID20xfe8 /* Peripheral identification
> register 2 */
> +#define UART_PID2_REV(x) (((x) & 0xf0) >> 4)
> +#define UART_PID30xfec /* Peripheral identification
> register 3 */
> #define UART_SPACE 0x100
>
> +#define UART_FIFO_SIZE 16
> +#define UART_FIFO_SIZE_R332
> +
> 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;
>
Re: pluart(4): fifo support
On Mon, Mar 07, 2022 at 07:36:35AM +, Visa Hankala wrote: > On Wed, Mar 02, 2022 at 07:25:36AM +0100, Anton Lindqvist wrote: > > On Tue, Mar 01, 2022 at 01:41:10PM +, Visa Hankala wrote: > > > On Tue, Mar 01, 2022 at 07:08:51AM +0100, Anton Lindqvist wrote: > > > > On Sun, Feb 27, 2022 at 05:33:17PM +0100, Mark Kettenis wrote: > > > > > > Date: Sun, 27 Feb 2022 16:01:25 +0100 > > > > > > From: Anton Lindqvist > > > > > > > > > > > > > > > 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. > > > > > > > > Here's a first stab at handling such devices. Attaching over ACPI also > > > > keeps the FIFO disabled. > > > > > > > > diff --git sys/dev/fdt/pluart_fdt.c sys/dev/fdt/pluart_fdt.c > > > > index 7f17365f1d6..45e31f9872f 100644 > > > > --- sys/dev/fdt/pluart_fdt.c > > > > +++ sys/dev/fdt/pluart_fdt.c > > > > @@ -69,6 +69,16 @@ pluart_fdt_attach(struct device *parent, struct > > > > device *self, void *aux) > > > > return; > > > > } > > > > > > > > + /* > > > > +* 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. > > > > +*/ > > > > + if (!OF_is_compatible(faa->fa_node, "arm,sbsa-uart")) > > > > + sc->sc_hwflags |= COM_HW_FIFO; > > > > + > > > > > > Could this piece of code use a dedicated flag, say COM_HW_SBSA (or > > > inversely COM_HW_PL011), that tells that the UART is an SBSA UART (or > > > true PL011 UART). > > > > > > To me, this use of the COM_HW_FIFO is a bit obscure; you need to look > > > at the comment in pluart_fdt_attach() to understand the logic. > > > > New diff replacing usage of COM_HW_FIFO with COM_HW_SBSA. > > > > > Is worrying about the exact size of the FIFO really worthwhile? As I > > > suggested earlier, pluart_start() could check the TXFF flag in the > > > Flag Register before feeding each byte to the FIFO. This would allow > > > the same transmit code apply to both SBSA and true PL011 UARTs. > > > > > > The SBSA UART interface does not have the IFLS register. However, it > > > looks that the FIFO and the interrupt are usable nevertheless. At least > > > Linux appears to enable them. > > > > I did look at Linux as well. They do not initialize the ifls field for > > SBSA devices causing 0 to be written to the UARTIFLS register which > > implies an interrupt level of 1/8 for both RX and TX. However, it's > > still unknown whenever this have any effect or not. > > I think the Linux driver does not use ifls when setting up > the SBSA UART. Compare pl011_startup() and sbsa_uart_startup() on Linux. > > > 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..61233290e0d 100644 > > --- sys/dev/ic/pluart.c > > +++ sys/dev/ic/plu
Re: pluart(4): fifo support
On Wed, Mar 02, 2022 at 07:25:36AM +0100, Anton Lindqvist wrote: > On Tue, Mar 01, 2022 at 01:41:10PM +, Visa Hankala wrote: > > On Tue, Mar 01, 2022 at 07:08:51AM +0100, Anton Lindqvist wrote: > > > On Sun, Feb 27, 2022 at 05:33:17PM +0100, Mark Kettenis wrote: > > > > > Date: Sun, 27 Feb 2022 16:01:25 +0100 > > > > > From: Anton Lindqvist > > > > > > > > > > > > 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. > > > > > > Here's a first stab at handling such devices. Attaching over ACPI also > > > keeps the FIFO disabled. > > > > > > diff --git sys/dev/fdt/pluart_fdt.c sys/dev/fdt/pluart_fdt.c > > > index 7f17365f1d6..45e31f9872f 100644 > > > --- sys/dev/fdt/pluart_fdt.c > > > +++ sys/dev/fdt/pluart_fdt.c > > > @@ -69,6 +69,16 @@ pluart_fdt_attach(struct device *parent, struct device > > > *self, void *aux) > > > return; > > > } > > > > > > + /* > > > + * 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. > > > + */ > > > + if (!OF_is_compatible(faa->fa_node, "arm,sbsa-uart")) > > > + sc->sc_hwflags |= COM_HW_FIFO; > > > + > > > > Could this piece of code use a dedicated flag, say COM_HW_SBSA (or > > inversely COM_HW_PL011), that tells that the UART is an SBSA UART (or > > true PL011 UART). > > > > To me, this use of the COM_HW_FIFO is a bit obscure; you need to look > > at the comment in pluart_fdt_attach() to understand the logic. > > New diff replacing usage of COM_HW_FIFO with COM_HW_SBSA. > > > Is worrying about the exact size of the FIFO really worthwhile? As I > > suggested earlier, pluart_start() could check the TXFF flag in the > > Flag Register before feeding each byte to the FIFO. This would allow > > the same transmit code apply to both SBSA and true PL011 UARTs. > > > > The SBSA UART interface does not have the IFLS register. However, it > > looks that the FIFO and the interrupt are usable nevertheless. At least > > Linux appears to enable them. > > I did look at Linux as well. They do not initialize the ifls field for > SBSA devices causing 0 to be written to the UARTIFLS register which > implies an interrupt level of 1/8 for both RX and TX. However, it's > still unknown whenever this have any effect or not. I think the Linux driver does not use ifls when setting up the SBSA UART. Compare pl011_startup() and sbsa_uart_startup() on Linux. > 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..61233290e0d 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_IFLS0x34/* Interrupt FIFO level select > register */ > +#define UART_IFLS_RX_SHIFT 3 /* RX level in bits
Re: pluart(4): fifo support
On Tue, Mar 01, 2022 at 01:41:10PM +, Visa Hankala wrote: > On Tue, Mar 01, 2022 at 07:08:51AM +0100, Anton Lindqvist wrote: > > On Sun, Feb 27, 2022 at 05:33:17PM +0100, Mark Kettenis wrote: > > > > Date: Sun, 27 Feb 2022 16:01:25 +0100 > > > > From: Anton Lindqvist > > > > > > > > > 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. > > > > Here's a first stab at handling such devices. Attaching over ACPI also > > keeps the FIFO disabled. > > > > diff --git sys/dev/fdt/pluart_fdt.c sys/dev/fdt/pluart_fdt.c > > index 7f17365f1d6..45e31f9872f 100644 > > --- sys/dev/fdt/pluart_fdt.c > > +++ sys/dev/fdt/pluart_fdt.c > > @@ -69,6 +69,16 @@ pluart_fdt_attach(struct device *parent, struct device > > *self, void *aux) > > return; > > } > > > > + /* > > +* 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. > > +*/ > > + if (!OF_is_compatible(faa->fa_node, "arm,sbsa-uart")) > > + sc->sc_hwflags |= COM_HW_FIFO; > > + > > Could this piece of code use a dedicated flag, say COM_HW_SBSA (or > inversely COM_HW_PL011), that tells that the UART is an SBSA UART (or > true PL011 UART). > > To me, this use of the COM_HW_FIFO is a bit obscure; you need to look > at the comment in pluart_fdt_attach() to understand the logic. New diff replacing usage of COM_HW_FIFO with COM_HW_SBSA. > Is worrying about the exact size of the FIFO really worthwhile? As I > suggested earlier, pluart_start() could check the TXFF flag in the > Flag Register before feeding each byte to the FIFO. This would allow > the same transmit code apply to both SBSA and true PL011 UARTs. > > The SBSA UART interface does not have the IFLS register. However, it > looks that the FIFO and the interrupt are usable nevertheless. At least > Linux appears to enable them. I did look at Linux as well. They do not initialize the ifls field for SBSA devices causing 0 to be written to the UARTIFLS register which implies an interrupt level of 1/8 for both RX and TX. However, it's still unknown whenever this have any effect or not. 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..61233290e0d 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_I
Re: pluart(4): fifo support
On Tue, Mar 01, 2022 at 07:08:51AM +0100, Anton Lindqvist wrote: > On Sun, Feb 27, 2022 at 05:33:17PM +0100, Mark Kettenis wrote: > > > Date: Sun, 27 Feb 2022 16:01:25 +0100 > > > From: Anton Lindqvist > > > > > > 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. > > Here's a first stab at handling such devices. Attaching over ACPI also > keeps the FIFO disabled. > > diff --git sys/dev/fdt/pluart_fdt.c sys/dev/fdt/pluart_fdt.c > index 7f17365f1d6..45e31f9872f 100644 > --- sys/dev/fdt/pluart_fdt.c > +++ sys/dev/fdt/pluart_fdt.c > @@ -69,6 +69,16 @@ pluart_fdt_attach(struct device *parent, struct device > *self, void *aux) > return; > } > > + /* > + * 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. > + */ > + if (!OF_is_compatible(faa->fa_node, "arm,sbsa-uart")) > + sc->sc_hwflags |= COM_HW_FIFO; > + Could this piece of code use a dedicated flag, say COM_HW_SBSA (or inversely COM_HW_PL011), that tells that the UART is an SBSA UART (or true PL011 UART). To me, this use of the COM_HW_FIFO is a bit obscure; you need to look at the comment in pluart_fdt_attach() to understand the logic. Is worrying about the exact size of the FIFO really worthwhile? As I suggested earlier, pluart_start() could check the TXFF flag in the Flag Register before feeding each byte to the FIFO. This would allow the same transmit code apply to both SBSA and true PL011 UARTs. The SBSA UART interface does not have the IFLS register. However, it looks that the FIFO and the interrupt are usable nevertheless. At least Linux appears to enable them. > 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..023fd66d776 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_IFLS0x34/* 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_80 /* FIFO 1/8 full */ > +#define UART_IFLS_1_41 /* FIFO 1/4 full */ > +#define UART_IFLS_1_22 /* FIFO 1/2 full */ > +#define UART_IFLS_3_43 /* FIFO 3/4 full */ > +#define UART_IFLS_7_84 /* FIFO 7/8 full */ > #define UART_IMSC0x38/* 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_PID00xfe0 /* Peripheral identification > register 0 */ > +#define UART_PID10xfe4 /* Peripheral identification > register 1 */ > +#define UART_PID20xfe8 /* Peripheral identification > register 2 */ > +#define UART_PID2_REV(x) (((x) & 0xf0) >> 4) > +#define UART_PID30xfec /* Peripheral identification > register 3 */ > #define UART_SPACE 0x100 > > +#define UART_FIFO_SIZE 16 > +#define UART_FIFO_SIZE_R332 > + > void pluartcnprobe(struct consdev *cp); > void pluartcninit(struct consdev *cp); > int pluartcngetc(dev_t dev); > @@ -150,7 +165,22 @@ struct cdevsw pluartdev = > void > pl
Re: pluart(4): fifo support
On Sun, Feb 27, 2022 at 05:33:17PM +0100, Mark Kettenis wrote:
> > Date: Sun, 27 Feb 2022 16:01:25 +0100
> > From: Anton Lindqvist
> >
> > 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
> > > >
> > > > On Sun, Feb 27, 2022 at 06:19:02AM +, 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 s
Re: pluart(4): fifo support
> Date: Sun, 27 Feb 2022 16:01:25 +0100
> From: Anton Lindqvist
>
> 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
> > >
> > > On Sun, Feb 27, 2022 at 06:19:02AM +, 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_IFLS0x34/* 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_80 /* FIFO 1/8
> > > > > full */
> > > > > +#define UART_IFLS_1_41 /* FIFO 1/4
> > > > > full */
> > > > > +#define UART_IFLS_1_22 /* FIFO 1/2
> > > > > full */
> > > > > +#define UART_IFLS_3_43 /* FIFO 3/4
> > > > > full */
> > > > > +#define UART_IFLS_7_84 /* FIFO 7/8
> > > > > full */
> > > > > #define UART_IMSC0x38/* 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_PID00xfe0 /* Peripheral
> > > > > identification register 0 */
> > > > > +#define UART_PID10xfe4 /* Peripheral
> > > > > identification register 1 */
> > > > > +#define UART_PID20xfe8 /* 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_PID30xfec /* 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
> >
Re: pluart(4): fifo support
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
> >
> > On Sun, Feb 27, 2022 at 06:19:02AM +, 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
Re: pluart(4): fifo support
On Sun, Feb 27, 2022 at 10:46:00AM +, Visa Hankala wrote:
> On Sun, Feb 27, 2022 at 09:56:38AM +0100, Anton Lindqvist wrote:
> > On Sun, Feb 27, 2022 at 06:19:02AM +, 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.
>
> I think the documentation is strict about the (currently) reserved bits
> so that any future revisions have the option to use them more easily.
> Hence applying the mask on the revision field is mandatory.
>
> > >
> > > > +#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)
> > > > +
Re: pluart(4): fifo support
On Sun, Feb 27, 2022 at 09:56:38AM +0100, Anton Lindqvist wrote:
> On Sun, Feb 27, 2022 at 06:19:02AM +, 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_IFLS0x34/* 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_80 /* FIFO 1/8 full */
> > > +#define UART_IFLS_1_41 /* FIFO 1/4 full */
> > > +#define UART_IFLS_1_22 /* FIFO 1/2 full */
> > > +#define UART_IFLS_3_43 /* FIFO 3/4 full */
> > > +#define UART_IFLS_7_84 /* FIFO 7/8 full */
> > > #define UART_IMSC0x38/* 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_PID00xfe0 /* Peripheral
> > > identification register 0 */
> > > +#define UART_PID10xfe4 /* Peripheral
> > > identification register 1 */
> > > +#define UART_PID20xfe8 /* 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.
I think the documentation is strict about the (currently) reserved bits
so that any future revisions have the option to use them more easily.
Hence applying the mask on the revision field is mandatory.
> >
> > > +#define UART_PID30xfec /* 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?
>
> diff --git sys/dev/ic/pluart.c sys/dev/ic/pluart.c
> index
Re: pluart(4): fifo support
> Date: Sun, 27 Feb 2022 09:56:38 +0100
> From: Anton Lindqvist
>
> On Sun, Feb 27, 2022 at 06:19:02AM +, 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_IFLS0x34/* 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_80 /* FIFO 1/8 full */
> > > +#define UART_IFLS_1_41 /* FIFO 1/4 full */
> > > +#define UART_IFLS_1_22 /* FIFO 1/2 full */
> > > +#define UART_IFLS_3_43 /* FIFO 3/4 full */
> > > +#define UART_IFLS_7_84 /* FIFO 7/8 full */
> > > #define UART_IMSC0x38/* 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_PID00xfe0 /* Peripheral
> > > identification register 0 */
> > > +#define UART_PID10xfe4 /* Peripheral
> > > identification register 1 */
> > > +#define UART_PID20xfe8 /* 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_PID30xfec /* 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
Re: pluart(4): fifo support
On Sun, Feb 27, 2022 at 06:19:02AM +, 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?
diff --git sys/dev/ic/pluart.c sys/dev/ic/pluart.c
index eaa11b6c44b..3479c16c698 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 i
Re: pluart(4): fifo support
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_IFLS0x34/* 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_80 /* FIFO 1/8 full */
> +#define UART_IFLS_1_41 /* FIFO 1/4 full */
> +#define UART_IFLS_1_22 /* FIFO 1/2 full */
> +#define UART_IFLS_3_43 /* FIFO 3/4 full */
> +#define UART_IFLS_7_84 /* FIFO 7/8 full */
> #define UART_IMSC0x38/* 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_PID00xfe0 /* Peripheral identification
> register 0 */
> +#define UART_PID10xfe4 /* Peripheral identification
> register 1 */
> +#define UART_PID20xfe8 /* 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.
> +#define UART_PID30xfec /* 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?
>
> 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?
I think the comment could be clearer and more concise; "this amount is
available" is vague in this context.
> + 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);
> + /* Enable FIFO. */
> 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");
> + bus_space_read_4(sc->sc_iot, sc->sc_ioh, UART_LCR_H) |
> + UART_LCR_H_FEN);
> }
>
> int
> @@ -197,19 +227,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) &&
> +
