Re: [PATCH] serial: 8250 dw: clear FIFO before writting LCR

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 7436 bytes --]

On Fri, 22 Aug 2025, Adriana Nicolae wrote:

> On Wed, Aug 20, 2025 at 1:02 PM Ilpo Järvinen
> <ilpo.jarvinen@linux.intel.com> wrote:
> >
> > On Tue, 19 Aug 2025, adriana@arista.com wrote:
> >
> > > This patch is proposing a custom configuration for Synopsys DesignWare
> >
> > Please try to avoid starting sentences in the changelog with "This patch"
> > or other wordings with similar meaning. Write imperatively instead.
> >
> > Preferrable, describe the problem first, then the solution.
> >
> > > serial to be used by products with associated compatible string in the
> > > device tree.
> > >
> > > The PORT_DWAPB config will be used instead of the default PORT_16550A
> > > which does not include the UART_FCR_CLEAR_RCVR and UART_FCR_CLEAR_XMIT
> > > bits for the FIFO configuration register. Having those flags is necessary
> > > to clear FIFO when the serial port is reconfigured with do_set_termios.
> > >
> > > Additionally, inside the do_set_termios we use the LCR (Line Control
> > > Register) to enable DLAB bit in order to access DLL/DLM (Divisor Latch
> > > Low/High) registers for baud rate setting. These 2 registers are sharing
> > > the same address space with UART_TX/UART_RX and UART_IER. The sequence is:
> > >
> > > (1) enable DLAB -> (2) set baud -> (3) disable DLAB -> (4) reset FCR
> > >
> > > When there is a TX or RX flow on the serial while we attempt to set/clear
> > > DLAB, the LCR write will be ignored and we will get a IIR_BUSY interrupt
> > > afterwards which is cleared by only reading the USR (UART status register).
> > >
> > > The sequence above can leave the serial in an unstable state in two cases:
> > >
> > > - if UART is busy while (1), then LCR is still pointing to the normal set of
> > > registers, which means the code setting DLL/DLM is actually writing into TX or
> > > modifying interrupts in UART_IER which may end with either a garbage character
> > > on the console or with serial interrupts disabled.
> > >
> > > - if UART is busy while (3), then LCR remains pointing to DLL/DLM instead of
> > > moving back to RX/TX. The first transfer on the serial will be stuck because
> > > the transmit/receive registers are not accessible unless the DLAB bit
> > > is cleared.
> > >
> > > The changes in this patch include a specific serial_out function for this UART
> > > type similar to the one for Armada-38x devices in commit
> > > b7639b0b15ddd1a4686b0142e70dfb122eefc88f with some changes in the tx_wait_empty
> > > function to check the UART status by looking at the USR register and actively
> > > try to clear FIFO to reduce time before a LCR write since the characters will
> > > be lost otherwise after baud rate change.
> > >
> > > The USR register may report that UART is busy even if TX/TX FIFO is already
> > > empty so we will loop until all USR[0] (UART busy status) is cleared and USR[1]
> > > TX FIFO is empty (RX FIFO bits should be 0 in this case).
> > > Keeping the same timeout of 20ms as measurements with the 9600 baud when
> > > the console was busy it took max 1.9ms to get the UART free state.
> > >
> > > Signed-off-by: Adriana Nicolae <adriana@arista.com>
> > > ---
> > >  drivers/tty/serial/8250/8250_dw.c   |   52 +++++++++++++++++++++++++++++++++++
> > >  drivers/tty/serial/8250/8250_port.c |    8 +++++
> > >  include/uapi/linux/serial_core.h    |    3 ++
> > >  3 files changed, 63 insertions(+)
> > >
> > > diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> > > index a53ba04d9770..985a2650f3f3 100644
> > > --- a/drivers/tty/serial/8250/8250_dw.c
> > > +++ b/drivers/tty/serial/8250/8250_dw.c
> > > @@ -33,6 +33,9 @@
> > >  /* Offsets for the DesignWare specific registers */
> > >  #define DW_UART_USR  0x1f /* UART Status Register */
> > >  #define DW_UART_DMASA        0xa8 /* DMA Software Ack */
> > > +#define DW_UART_USR_BUSY 0x1 /* UART Busy status */
> > > +#define DW_UART_USR_TFNF 0x2 /* UART TX FIFO not full */
> > > +#define DW_UART_USR_TFE  0x4 /* UART TX FIFO empty */
> > >
> > >  #define OCTEON_UART_USR      0x27 /* UART Status Register */
> > >
> > > @@ -56,6 +59,10 @@
> > >  #define DW_UART_QUIRK_IS_DMA_FC              BIT(3)
> > >  #define DW_UART_QUIRK_APMC0D08               BIT(4)
> > >  #define DW_UART_QUIRK_CPR_VALUE              BIT(5)
> > > +#define DW_UART_QUIRK_APB            BIT(6)
> > > +
> > > +#define DW8250_REG( p, reg ) \
> > > +     ((void __iomem *)(p->membase + ((reg) << p->regshift)))
> >
> > What's wrong with dw8250_readl_ext() and dw8250_writel_ext()?
> >
> > >  struct dw8250_platform_data {
> > >       u8 usr_reg;
> > > @@ -220,6 +227,47 @@ static void dw8250_serial_out38x(struct uart_port *p, unsigned int offset, u32 v
> > >       dw8250_serial_out(p, offset, value);
> > >  }
> > >
> > > +/* Drain FIFO and wait for USR to be not busy and TX/RX FIFO empty */
> > > +static void dw8250_tx_wait_empty_apb(struct uart_port *p)
> > > +{
> > > +     unsigned int tries = 20000;
> > > +     unsigned int delay_threshold = tries - 1000;
> > > +     unsigned int usr;
> > > +
> > > +     while (tries--) {
> > > +             usr = readl(DW8250_REG(p, DW_UART_USR));
> > > +
> > > +             /* Check UART free and TX/RX FIFO empty */
> > > +             if ((usr & ~DW_UART_USR_TFNF) == DW_UART_USR_TFE)
> > > +                     break;
> > > +
> > > +             /* FIFO is still not empty, try to clear it */
> > > +             if (tries < delay_threshold) {
> > > +                     writel(UART_FCR_ENABLE_FIFO, DW8250_REG(p, UART_FCR));
> > > +                     writel(UART_FCR_ENABLE_FIFO | UART_FCR_CLEAR_RCVR |
> > > +                     UART_FCR_CLEAR_XMIT, DW8250_REG(p, UART_FCR));
> >
> > Please indent any continuation lines properly, in this case to the char
> > after the opening parenthesis.
> >
> > > +                     writel(0, DW8250_REG(p, UART_FCR));
> > > +                     udelay (1);
> > > +             }
> > > +     }
> >
> > This seems to be just rehashing the same non-robust algorithm. There's no
> > way to ensure UART wouldn't become BUSY again because of Rx at any time.
> > Thus, any amount of clearing FIFOs is just never going be fully safe.
> >
> > Long time ago, I attempted to create a more robust solution to this BUSY
> > problem by temporarily enabling loopback which should prevent any new Rx
> > from reaching the UART.
> >
> > Could you please try my patch that is attached to:
> >
> > https://lore.kernel.org/linux-serial/079c8fe6-9ce4-fa59-4b44-93e27dd376d6@linux.intel.com/
> Thanks! I've applied this patch on the device I'm using and ran the
> test that attempts
> to write a large buffer while do_set_termios is called. I don't see
> any softlockup
> anymore, but during one test iteration the console hangs.
> 
> I could ssh to the host, `stty -F /dev/ttyS0 sane` hangs and same if I try to
> write anything `echo test > /dev/ttyS0`. There are no errors in dmesg, and
> tty interrupts are not triggered anymore. I have kernel watchdog enabled but
> it didn't trigger.  I'm going to add some traces in dw8250_idle_enter and
> dw8250_idle_exit and update this thread. It takes a while to reproduce,
> sorry for the delayed response.

Hi Adriana,

Did you ever manage to figure out what goes wrong with the idle enter/exit 
patch?

-- 
 i.