On Tue, 9 May 2023, qianfan wrote: > 在 2023/5/8 21:34, Ilpo Järvinen 写道: > > On Mon, 8 May 2023, qianfan wrote: > > > 在 2023/5/8 18:17, Ilpo Järvinen 写道: > > > > On Fri, 5 May 2023, qianfan wrote: > > > > > 在 2023/4/14 20:10, Ilpo Järvinen 写道: > > > > > > On Fri, 14 Apr 2023, qianfan wrote: > > > > > > > My custom board is based on allwinner R40, the uart is compatibled > > > > > > > with > > > > > > > serial8250. Based on it's datasheet: > > > > > > > > > > > > > > > When TX transmit data, or RX receives data, or TX FIFO is not > > > > > > > > empty, > > > > > > > > then > > > > > > > the > > > > > > > > BUSY flag bit can be set to 1 by hardware, which indicates the > > > > > > > > UART > > > > > > > > controller is busy. > > > > > > > We cannot write LCR and DLL to update UART params such as baudrate > > > > > > > and > > > > > > > partity > > > > > > > while the UART is busy, however `serial8250_do_set_termios` is a > > > > > > > void > > > > > > > function, > > > > > > > the upper level always assume the uart params is updated. > > > > > > > > > > > > > > The upper level `uart_set_termios` do noting if ktermios params is > > > > > > > not > > > > > > > changed, > > > > > > > it will not update when the user space program running tcsetattr > > > > > > > set a > > > > > > > same > > > > > > > baudrate again. > > > > > > > > > > > > > > So we can not fix the baudrate when > > > > > > > `serial8250_do_set_termios` > > > > > > > failed. > > > > > > > > > > > > > > Allwinner R40's datasheet provided a way for this case. > > > > > > > > > > > > > > > CHCFG_AT_BUSY(configure at busy): Enable the bit, software can > > > > > > > > also > > > > > > > > set > > > > > > > > UART > > > > > > > > controller when UART is busy, such as the LCR, DLH, DLL > > > > > > > > register. > > > > > > > > CHANGE_UPDATE(change update): If CHCFG_AT_BUSY is enabled, and > > > > > > > > CHANGE_UPDATE > > > > > > > > is written to 1, the configuration of UART controller can be > > > > > > > > updated. > > > > > > > > After completed update, the bit is cleared to 0 automatically. > > > > > > > I can't know this feature is expanded by allwinner, or it is a > > > > > > > common > > > > > > > functiton > > > > > > > of serial8250. Perhaps the serial8250 driver need this. > > > > > > tcsetattr() can be given a flag which enforces TX empty condition > > > > > > before > > > > > > core calls into the lower layer HW set_termios function. Would that > > > > > > be > > > > > > enough to solve the case you're interested in? > > > > > > > > > > > > Obviously, nothing can prevent Rx from occuring as it's not under > > > > > > local > > > > > > UART's control (e.g. a busy flag check would still be racy). But > > > > > > does > > > > > > writing those registers actually break something or just corrupts > > > > > > the > > > > > > character under Tx/Rx (which can be handled by flushing)? > > > > > Hi: > > > > > > > > > > I speed long times to create a common solution for this problem. > > > > > > > > > > (I had create two commit, the first one add some sysfs debug interface > > > > > and the second one try solve this problem. So the next following patch > > > > > has only patch-2. Let's we discuess this solution and I will send all > > > > > patches if it is good.) > > > > Thanks a lot, it's much easier to discuss now with something concrete at > > > > hand. > > > > > > > > > Allwinner introduce some bits in HALT_TX register which can change > > > > > baudrate while the serial is busy. But that is not a common feature > > > > > of dw-uart. Rockchip's uart is also based on dw-uart and they doesn't > > > > > has such feature. > > > > > > > > > > The loopback is a common feature of 16450/16550 serial, so we can set > > > > > loopback mode to cut down the external serial line to force the serial > > > > > to idle. > > > > > > > > > > Next is the second patch: > > > > > > > > > > From 171e981c3695e3efcc76a2c4f0d0937d366d6e2a Mon Sep 17 00:00:00 > > > > > 2001 > > > > > From: qianfan Zhao <qianfanguijin@xxxxxxx> > > > > > Date: Fri, 5 May 2023 08:46:50 +0800 > > > > > Subject: [PATCH] drivers: serial: 8250_dw: Make uart idle before set > > > > > baudrate > > > > > > > > > > Some registers which control the baudrate such as DLL, DLM can not > > > > > write while the uart is busy. So set the controller to loopback mode > > > > > and clear fifos to force idle before change baudrate. > > > > > > > > > > Signed-off-by: qianfan Zhao <qianfanguijin@xxxxxxx> > > > And the total waiting time is not a const value so we need polling. > > 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 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. > 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. > > It might also make sense to add delayed error reporting for the failures. > > The error printing can safely happen only after releasing port's lock. > Yes, this is really useful. I checked it briefly yesterday and only the break_ctl would need to be wrapped besides set_divisor and set_termios. In any case, it should be done in a separate patch but definitely looked doable. -- i.
