Re: serial8250: can not change baudrate while the controller is busy

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

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

Hi,

Did you ever manage to take a look at the approach I tried in the patch 
below or perhaps even test if it works?

On Wed, 10 May 2023, Ilpo Järvinen wrote:

> On Tue, 9 May 2023, qianfan wrote:
> 
> > It's not constant, agreed. But your comment states that it's at most one
> > frame worth of time so that should be the worst-case waiting time. Once
> > the UART starts to do things like temporary switch to loopback and/or
> > reinit/clearing FIFO, it doesn't seem that harmful to wait slightly
> > longer for the worst-case frame time, so essentially you'd only need this
> > (+ comment):
> > 
> > 	p->serial_out(p, UART_MCR, up->mcr | UART_MCR_LOOP);
> > 	dw8250_force_idle(p);
> > 	ndelay(p->frame_time);
> > 	p->serial_out(p, UART_LCR, up->lcr | UART_LCR_DLAB);
> > 
> > If you insist, ndelay() could be replaced by:
> > 	ret = read_poll_timeout_atomic(p->serial_in, usr, !(usr &
> > DW_UART_USR_BUSY),
> > 				       1, DIV_ROUND_UP(p->frame_time,
> > NSEC_PER_USEC), false,
> > 				       p, d->usr_reg);
> > 
> > You also don't explain why force_idle() is needed inside to loop, why
> > doing it once before the loop is not enough? I can see the need for that
> > in loop for dw8250_check_lcr() because it doesn't enable loopback, but
> > here, so why?
> > 
> > Under my test, this code will not work:
> > 
> > p->serial_out(p, UART_MCR, up->mcr | UART_MCR_LOOP);
> > dw8250_force_idle(p);
> > /* waiting until not busy... */
> > 
> > Current byte maybe not finished when we set UART_MCR_LOOP and reset fifo,
> > dw-uart will continue receive even if LOOP bit is set. After this byte
> > is finished it will push data to rx fifo and remark BUSY flag again.
> > That's why I leave dw8250_force_idle inside to loop.
> > 
> > It's a bit odd because BUSY should indicate a character is being received 
> > (asserted around middle of the start bit according to the databook), not 
> > 
> > Is the databoot is public and may I read it?
> 
> I'm not sure. The title is "DesignWare DW_apb_uart Databook" in case that 
> helps.
> 
> > that it was pushed to FIFO. Is it trying to indicate BUSY due to rxing 
> > another character but then the hypothesis that enabling loopback takes at 
> > most 1 frame would be false.
> > 
> > To understand better what's going on here during debugging, it might be 
> > useful the check DR in LSR in the case when the BUSY is still held to 
> > exclude data-in-RBR condition which is another condition that could 
> > assert BUSY. Since the FIFO is being toggled off/on, there's window of 
> > time when BUSY could be held due to something arriving into RBR.
> > 
> > IMHO, it would be nice if the FIFO reset turns out to be entirely 
> > unnecessary for enforcing not-busy because of that very window of 
> > receiving something into RBR. A character in RBR would need to be handled 
> > by an extra read from UART_RX which is what dw8250_force_idle() actually 
> > does, it looked earlier pretty magic to me but now realizing this race 
> > window w/o FIFO, maybe that's the explanation. If FIFO reset is not 
> > necessary, it would take away the need to understand how instanteneous 
> > toggling UART_FCR_ENABLE_FIFO is wrt. arriving character.
> > 
> > Thanks and now I understand why we should read UART_RX register when dw8250_
> > force_idle,
> > the loopback mode takes affect right now when we set LOOP bit, the fifo is r
> > eseted and
> > the next incoming data will goto RBR register, not FIFO.
> > 
> > So the most important code in dw8250_force_idle after set loopback mode is r
> > ead UART_RX, 
> > not clear fifo. Is it right?
> > 
> > So the next code is better:
> > p->serial_out(p, UART_MCR, up->mcr | UART_MCR_LOOP);
> > if (p->serial_in(p, d->usr_reg) & DW_UART_USR_BUSY)
> > 	ndelay(p->frame_time);
> > dw8250_force_idle(p);
> >
> > Or maybe we should make dw8250_force_idle after ndelay such as:
> > 
> > p->serial_out(p, UART_MCR, up->mcr | UART_MCR_LOOP);
> > ndelay(p->frame_time);
> > dw8250_force_idle(p);
> > 
> > But that requires a test to see if it works.
> > 
> > Is Tx side empty during your test or can it interfere too here? (Just 
> > thinking out loud, you probably considered that yourself).
> > 
> > I started to wonder whether dw8250_check_lcr() should also temporarily
> > switch to loopback mode to ensure the UART becomes idle. Some common macro
> > could be created which wraps the idle forcing for both use cases +
> > restoring LCR & MCR. That is, dw8250_force_idle() + little bit extra ->
> > __dw8250_idle_enter() and __dw8250_idle_exit() that are called by the
> > macro.
> > 
> > Switch to loopback mode in dw8250_check_lcr is not enough. We also need intr
> > oduce 
> > sometings such as dw8250_check_dll() and dw8250_check_dlm(). That is not a g
> > ood
> > idea.
> 
> No. My point was that it would be nice to figure out a sequence of 
> operations that is guaranteed to get BUSY deasserted. It helps both LCR 
> (only) write problem and the divisor update problem because the cause for 
> them seems to be the same.
> 
> So replacing dw8250_force_idle() with something like what is below might 
> be useful.
> 
> I still don't understand why the FIFO needs to be disabled but included it 
> into the idle sequence anyway.
> 
> In contrast to earlier:
>   - FIFO is not reset but it could be added into the idle exit function if 
>     it's a desirable feature.
>   - IER is zeroed to prevent RBR filling triggering an interrupt (with 
>     port's lock held here it's going to just keep attempt to acquire the 
>     lock).
>   - LCR write is handled as well (w/o now triggering extra FIFO reinits).
>   - Flush DMA Rx
> 
> 
> [PATCH 1/1] serial: 8250_dw: Ensure BUSY is deasserted
> 
> DW UART cannot write to LCR, DLL, and DLH while BUSY is asserted.
> Existance of BUSY depends on uart_16550_compatible, if UART HW is
> configured with 16550 compatible those registers can always be
> written.
> 
> There currently is dw8250_force_idle() which attempts to archive
> non-BUSY state by disabling FIFO, however, the solution is unreliable
> when Rx keeps getting more and more characters.
> 
> Create a sequence of operations to enforce that ensures UART cannot
> keep BUSY asserted indefinitely. The new sequence relies on enabling
> loopback mode temporarily to prevent incoming Rx characters keeping
> UART BUSY.
> 
> Use the new dw8250_idle_enter/exit() to do divisor writes and LCR
> writes.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  drivers/tty/serial/8250/8250_dw.c    | 141 ++++++++++++++++++++-------
>  drivers/tty/serial/8250/8250_dwlib.h |   1 +
>  2 files changed, 106 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index 7db51781289e..c9d9b4bda36a 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -44,6 +44,8 @@
>  /* DesignWare specific register fields */
>  #define DW_UART_MCR_SIRE		BIT(6)
>  
> +#define DW_UART_USR_BUSY		BIT(0)
> +
>  /* Renesas specific register fields */
>  #define RZN1_UART_xDMACR_DMA_EN		BIT(0)
>  #define RZN1_UART_xDMACR_1_WORD_BURST	(0 << 1)
> @@ -80,57 +82,123 @@ static inline int dw8250_modify_msr(struct uart_port *p, int offset, int value)
>  	return value;
>  }
>  
> -static void dw8250_force_idle(struct uart_port *p)
> +/*
> + * Ensure BUSY is not asserted. If DW UART is configured with
> + * !uart_16550_compatible, the writes to LCR, DLL, and DLH fail while
> + * BUSY is asserted.
> + *
> + * Context: port's lock must be held
> + */
> +static int dw8250_idle_enter(struct uart_port *p)
>  {
> +	struct dw8250_data *d = to_dw8250_data(p->private_data);
>  	struct uart_8250_port *up = up_to_u8250p(p);
> -	unsigned int lsr;
> +	u32 lsr;
>  
> -	serial8250_clear_and_reinit_fifos(up);
> +	if (d->uart_16550_compatible)
> +		return 0;
>  
> -	/*
> -	 * With PSLVERR_RESP_EN parameter set to 1, the device generates an
> -	 * error response when an attempt to read an empty RBR with FIFO
> -	 * enabled.
> -	 */
> -	if (up->fcr & UART_FCR_ENABLE_FIFO) {
> -		lsr = p->serial_in(p, UART_LSR);
> -		if (!(lsr & UART_LSR_DR))
> -			return;
> +	d->in_idle = 1;
> +
> +	/* Prevent triggering interrupt from RBR filling */
> +	p->serial_out(p, UART_IER, 0);
> +
> +	serial8250_rx_dma_flush(up);
> +	// What about Tx DMA? Should probably pause that too and resume
> +	// afterwards.
> +
> +	p->serial_out(p, UART_MCR, up->mcr | UART_MCR_LOOP);
> +	if (up->capabilities & UART_CAP_FIFO)
> +		p->serial_out(p, UART_FCR, 0);
> +
> +	if (p->serial_in(p, d->pdata->usr_reg) & DW_UART_USR_BUSY)
> +		ndelay(p->frame_time);
> +
> +	lsr = serial_lsr_in(up);
> +	if (lsr & UART_LSR_DR) {
> +		p->serial_in(p, UART_RX);
> +		up->lsr_saved_flags = 0;
>  	}
>  
> -	(void)p->serial_in(p, UART_RX);
> +	/* Now guaranteed to have BUSY deasserted? Just sanity check */
> +	if (p->serial_in(p, d->pdata->usr_reg) & DW_UART_USR_BUSY)
> +		return -EBUSY;
> +
> +	return 0;
> +}
> +
> +static void dw8250_idle_exit(struct uart_port *p)
> +{
> +	struct dw8250_data *d = to_dw8250_data(p->private_data);
> +	struct uart_8250_port *up = up_to_u8250p(p);
> +
> +	if (d->uart_16550_compatible)
> +		return;
> +
> +	if (up->capabilities & UART_CAP_FIFO)
> +		p->serial_out(p, UART_FCR, up->fcr);
> +	p->serial_out(p, UART_MCR, up->mcr);
> +	p->serial_out(p, UART_IER, up->ier);
> +
> +	// Maybe move the DMA Rx restart check in dma_rx_complete() to own
> +	// function (serial8250_rx_dma_restart()) and call it from here.
> +	// DMA Tx resume
> +
> +	d->in_idle = 0;
> +}
> +
> +static void dw8250_set_divisor(struct uart_port *p, unsigned int baud,
> +			       unsigned int quot, unsigned int quot_frac)
> +{
> +	struct uart_8250_port *up = up_to_u8250p(p);
> +	int ret;
> +
> +	ret = dw8250_idle_enter(p);
> +	if (ret < 0)
> +		goto idle_failed;
> +
> +	p->serial_out(p, UART_LCR, up->lcr | UART_LCR_DLAB);
> +	if (!(p->serial_in(p, UART_LCR) & UART_LCR_DLAB))
> +		goto idle_failed;
> +
> +	serial_dl_write(up, quot);
> +	p->serial_out(p, UART_LCR, up->lcr);
> +
> +idle_failed:
> +	dw8250_idle_exit(p);
>  }
>  
>  static void dw8250_check_lcr(struct uart_port *p, int value)
>  {
> -	void __iomem *offset = p->membase + (UART_LCR << p->regshift);
> -	int tries = 1000;
> +	struct dw8250_data *d = to_dw8250_data(p->private_data);
> +	unsigned int lcr = p->serial_in(p, UART_LCR);
> +	int ret;
>  
>  	/* Make sure LCR write wasn't ignored */
> -	while (tries--) {
> -		unsigned int lcr = p->serial_in(p, UART_LCR);
> -
> -		if ((value & ~UART_LCR_SPAR) == (lcr & ~UART_LCR_SPAR))
> -			return;
> +	if ((value & ~UART_LCR_SPAR) == (lcr & ~UART_LCR_SPAR))
> +		return;
>  
> -		dw8250_force_idle(p);
> +	if (d->in_idle) {
> +		/*
> +		 * FIXME: this deadlocks if port->lock is already held
> +		 * dev_err(p->dev, "Couldn't set LCR to %d\n", value);
> +		 */
> +		return;
> +	}
>  
> -#ifdef CONFIG_64BIT
> -		if (p->type == PORT_OCTEON)
> -			__raw_writeq(value & 0xff, offset);
> -		else
> -#endif
> -		if (p->iotype == UPIO_MEM32)
> -			writel(value, offset);
> -		else if (p->iotype == UPIO_MEM32BE)
> -			iowrite32be(value, offset);
> -		else
> -			writeb(value, offset);
> +	ret = dw8250_idle_enter(p);
> +	if (ret < 0) {
> +		/*
> +		 * FIXME: this deadlocks if port->lock is already held
> +		 * dev_err(p->dev, "Couldn't set LCR to %d\n", value);
> +		 */
> +		goto idle_failed;
>  	}
> -	/*
> -	 * FIXME: this deadlocks if port->lock is already held
> -	 * dev_err(p->dev, "Couldn't set LCR to %d\n", value);
> -	 */
> +
> +	p->serial_out(p, UART_LCR, value);
> +
> +idle_failed:
> +	dw8250_idle_exit(p);
>  }
>  
>  /* Returns once the transmitter is empty or we run out of retries */
> @@ -540,6 +608,7 @@ static int dw8250_probe(struct platform_device *pdev)
>  	p->serial_out	= dw8250_serial_out;
>  	p->set_ldisc	= dw8250_set_ldisc;
>  	p->set_termios	= dw8250_set_termios;
> +	p->set_divisor	= dw8250_set_divisor;
>  
>  	p->membase = devm_ioremap(dev, regs->start, resource_size(regs));
>  	if (!p->membase)
> diff --git a/drivers/tty/serial/8250/8250_dwlib.h b/drivers/tty/serial/8250/8250_dwlib.h
> index f13e91f2cace..bafacd327fc4 100644
> --- a/drivers/tty/serial/8250/8250_dwlib.h
> +++ b/drivers/tty/serial/8250/8250_dwlib.h
> @@ -45,6 +45,7 @@ struct dw8250_data {
>  
>  	unsigned int		skip_autocfg:1;
>  	unsigned int		uart_16550_compatible:1;
> +	unsigned int		in_idle:1;
>  };
>  
>  void dw8250_do_set_termios(struct uart_port *p, struct ktermios *termios, const struct ktermios *old);
> 


-- 
 i.