Hello, I'm to implement a work-around in the driver to stop 0xff flood caused by malfunction of the hardware, and I got a few doubts when looking at the code. I'd appreciate any insights on the issues below. 1. URXD_DUMMY_READ bit logic introduced to handle CREAD[1] slightly gets in the way of implementing the work-around. I wonder why do we need URXD_DUMMY_READ in the first place? Why don't we just disable receiver in hardware by clearing UCR2_RXEN bit when CREAD is not set in termios? Any specific reason for that? Is it a requirement that we still do read everything from hardware, so that sender can't notice CREAD is cleared on our side, or what? 2. URXD_DUMMY_READ forces break from FIFO reading loop on the first read char, at imx.c:850. This seems to achieve nothing positive, yet it risks to cause either needless isr re-entries, or leaving some stuck chars in the FIFO waiting for URXD_DUMMY_READ to be cleared. Is it OK to continue FIFO reading (and still ignoring the chars) instead? 3. Related to (2), yet another instance of code that checks for bits to be ignored does break from the FIFO loop as well, but only after 100 chars has been ignored at this run, at imx.c:831. This looks like protection from something, but what's that, and how it'd protect from anything by just exiting the loop? Don't we then need something more drastic like disabling some IRQs or resetting the chip? It looks even more suspect as this is the only place in the loop with such "protection". OK to get rid of this suspect check? 4. uart_handle_sysrq_char() is called before error bits are checked, at imx.c:817. Do we really need to consider potentially random characters as sysrqs, and are we safe doing this? Would you support a change to pass only cleanly received chars to sysrq? 5. imx_uart_clear_rx_errors() that is used on DMA path seems to never considers IGN* flags: instead it always inserts BREAK, and never OVERRUN, PARITY, or FRAME. There are no comments in the code. What's going on here? 6. Related to (2), __imx_uart_rxint() checks USR2_BRCD in the loop when it also has two checks for URXD_BRK later in the same loop. This handling looks rather convoluted, yet no comments in the code are provided. It looks like USR2_BRCD check could be at least moved out of the loop, if not removed in favor of doing all that is needed in URXD_BRK check. 7. __imx_uart_rxint() reads USR2 two times for every char when one time seems to be enough. We can likely get even to zero USR2 reads, provided we rather use URXD_CHARRDY to terminate FIFO reading loop, and get rid of USR2_BRCD check (see (6) above). Worth to optimize? References: [1] URXD_DUMMY_READ has been introduced by: " commit 55d8693acd65b1c14e011cbcbfad2a15626472cd Author: Jiada Wang <jiada_wang@xxxxxxxxxx> Date: Tue Dec 9 18:11:22 2014 +0900 serial: imx: add CREAD flag support Add CREAD flag hanlding in set_termios and UART DMA mode which ignores all received chars when CREAD flag cleared. " Thanks, -- Sergey Organov
