On Fri, Jun 10, 2022 at 01:15:12PM +0000, 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 +0000, 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 <an...@basename.se>
> > > > > >
> > > > > > On Tue, Mar 08, 2022 at 01:44:47PM +0000, Visa Hankala wrote:
> > > > > > > On Tue, Mar 08, 2022 at 08:04:36AM +0100, Anton Lindqvist wrote:
> > > > > > > > On Mon, Mar 07, 2022 at 07:36:35AM +0000, Visa Hankala wrote:
> > > > > > > > > I still think that checking TXFF and using the same code for
> > > > > > > > > both
> > > > > > > > > SBSA and true PL011 UARTs would be the best choice. This
> > > > > > > > > would avoid
> > > > > > > > > fragmenting the code and improve robustness by relying on
> > > > > > > > > functionality
> > > > > > > > > that is common to the different controller variants.
> > > > > > > >
> > > > > > > > Fair enough, new diff.
> > > > > > >
> > > > > > > Maybe the comments should omit the FIFO space description and just
> > > > > > > mention the lack of the level control register in the SBSA UART
> > > > > > > register interface.
> > > > > >
> > > > > > I ended up tweaking the comments before committing. Thanks for all
> > > > > > the
> > > > > > feedback.
> > > > > >
> > > > >
> > > > > Hi Anton,
> > > > >
> > > > > This diff seems to break things. When I boot my rpi4 it now prints:
> > > > >
> > > > > pluart0 at simplebus0: rev 0, 16 byte fifo
> > > > > pluart0: console
> > > > > bcmbsc0 at simplebus0
> > > > > iic0 at bcmbsc0
> > > > >
> > > > > so it appears that a carriage return character is lost here.
> > > > >
> > > > > Later on output stops at:
> > > > >
> > > > > reordering libraries: done.
> > > > >
> > > > > and only when I reboot the machine the login prompt appears, but with
> > > > > some wierd respawning:
> > > > >
> > > > > OpenBSD/arm64 (rutter.sibelius.xs4all.nl) (console)
> > > > >
> > > > > login: init: getty repeating too quickly on port /dev/console,
> > > > > sleeping
> > > > > init: getty repeating too quickly on port /dev/console, sleeping
> > > > >
> > > > > OpenBSD/arm64 (rutter.sibelius.xs4all.nl) (console)
> > > > >
> > > > > login:
> > > > > OpenBSD/arm64 (rutter.sibelius.xs4all.nl) (console)
> > > > >
> > > > > login:
> > > > >
> > > > > If you don't have a quick fix for this, may I suggest reverting the
> > > > > commit? We're heading towards release and we don't want the serial
> > > > > console on the rpi4 to be broken.
> > > >
> > > > Circling back to this: what happens is that no tx interrupt is raised
> > > > when sending less data than the configured interrupt fifo level, causing
> > > > the tty to end up in a forever busy state. Clearing the busy flag after
> > > > a successful transmission of all queued data solves the problem.
> > >
> > > 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.
>
> <snip>
>
> > @@ -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.
