On Tue, Aug 06, 2019 at 06:24:07PM +0300, Andy Shevchenko wrote: > On Tue, Aug 06, 2019 at 09:40:41AM -0400, Robert Middleton wrote: > > On Mon, Aug 5, 2019 at 10:22 AM Andy Shevchenko > > <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote: > > > On Mon, Aug 05, 2019 at 09:49:24AM -0400, Robert Middleton wrote: > > > > Unfortunately this will re-introduce the bug that it was attempting to > > > > solve, that is ensuring that the buffer in the kernel and the buffer > > > > on the chip are clear before going into shutdown on the chip. > > > > Breaking at the beginning of the loop means that the kernel has > > > > written everything to the internal buffer on the chip, but until the > > > > LSR bits are clear the bytes have not been transmitted yet. > > > > > > So, the difference here, that you have a long delay with mdelay(1) which > > > "fixes" your issue. > > > > The important fix is really this part: > > lsr = serial_in(up, UART_LSR); > > if (lsr & (UART_LSR_TEMT | UART_LSR_THRE)) > > > > Both of those bits must be set before the shutdown can occur. The > > msleep(1) is so that it is not a busy loop(maybe usleep_range would be > > better?). > > If the transmitter is not empty and the transmitter FIFO is > > not empty, when the shutdown occurs the remaining data will be > > discarded by the exar chip. Assuming the FIFO on the exar is full(256 > > bytes to transmit), it would take ~250ms to transmit all of the bytes > > at 9600 baud, so our check would happen ~250 times(assuming mdelay(1) > > sleeps for 1ms and not up to 20). > > > > This is based off of the reference driver from Max Linear: > > https://github.com/rm5248/exar-driver/blob/5dcbac6aa564cf5604512ecd6c9e02c577b70fa2/xr17v35x.c#L1282 > > I see, you explicitly force to continue in your case. > Thanks for explanation. One more though. Let's consider your code carefully. do { lsr = serial_in(up, UART_LSR); if (lsr & (UART_LSR_TEMT | UART_LSR_THRE)) tx_complete = 1; else tx_complete = 0; Here we set a boolean to be true or false, fine... msleep(1); ...wait for some undefined long time... } while (!uart_circ_empty(xmit) && !tx_complete && i++ < 1000); ...first conditional is to check kernel buffer for emptiness, and since it's an AND operation in use, break if it false... (In comparison to what I proposed, the difference is, that I'm not reading register and waiting for some milliseconds) ...then, you check a boolean variable... (similar what I have done with a 'break' statement) ...and at last we have check the iteration counter. (this same in both cases). So, I'm puzzled how your cases works reliably if it based on msleep(). With enough slow baud rate you won't get fix working AFAIU. Where am I wrong? -- With Best Regards, Andy Shevchenko
