On 2013-03-27 14:37, Andreas Bießmann wrote:
Dear Manfred Huber,
---8<--- abiessmann@punisher % pwclient get 230994 Saved patch to U-Boot-1-1-v2-omap3_beagle-Enabling-UART3-first-allows-the-Transmitter-to-be-empty.patch abiessmann@punisher % git am U-Boot-1-1-v2-omap3_beagle-Enabling-UART3-first-allows-the-Transmitter-to-be-empty.patch Patch does not have a valid e-mail address. abiessmann@punisher % ./tools/checkpatch.pl U-Boot-1-1-v2-omap3_beagle-Enabling-UART3-first-allows-the-Transmitter-to-be-empty.patch ERROR: trailing whitespace #39: FILE: drivers/serial/ns16550.c:40: +^Iif ((serial_in(&com_port->lsr) & (UART_LSR_TEMT | UART_LSR_THRE)) == $
ERROR: patch seems to be corrupt (line wrapped?) #40: FILE: drivers/serial/ns16550.c:40: UART_LSR_THRE) {
total: 2 errors, 0 warnings, 20 lines checked
NOTE: whitespace errors detected, you may wish to use scripts/cleanpatch or scripts/cleanfile
NOTE: Ignored message types: COMPLEX_MACRO CONSIDER_KSTRTO MINMAX MULTISTATEMENT_MACRO_USE_DO_WHILE
U-Boot-1-1-v2-omap3_beagle-Enabling-UART3-first-allows-the-Transmitter-to-be-empty.patch has style problems, please review.
If any of these errors are false positives, please report them to the maintainer, see CHECKPATCH in MAINTAINERS. --->8---
Can you please fix these errors?
I will do it.
On 03/25/2013 11:02 PM, Manfred Huber wrote:
From: Manfred Huber
Due to a Bug in the ROM code of some OMAP3 devices, the TEMT bit is not set if UART3 is configured before (only THRE is set). Reason is the disabling of UART3 even though the Transmitter is not empty. Enabling UART3 allows the Transmitter to be empty.
Signed-off-by: Manfred Huber man.huber@arcor.de
drivers/serial/ns16550.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c index b2da8b3..24ff84f 100644 --- a/drivers/serial/ns16550.c +++ b/drivers/serial/ns16550.c @@ -36,10 +36,18 @@
void NS16550_init(NS16550_t com_port, int baud_divisor) { -#if (!defined(CONFIG_SYS_NS16550_BROKEN_TEMT)) +#if (defined(CONFIG_SPL_BUILD) && defined(CONFIG_OMAP34XX))
I think a comment here would be good.
I will do it.
- if ((serial_in(&com_port->lsr) & (UART_LSR_TEMT | UART_LSR_THRE))
== UART_LSR_THRE) {
The patch is broken here, even if you fix the wrapping you will get an 'over 80 char' error here.
serial_out(UART_LCR_DLAB, &com_port->lcr);serial_out(baud_divisor & 0xff, &com_port->dll);serial_out((baud_divisor >> 8) & 0xff, &com_port->dlm);serial_out(UART_LCRVAL, &com_port->lcr);serial_out(0, &com_port->mdr1);Do we need to setup baud a.s.o. here? Isn't it enough to issue an soft reset of the UART? I'm not in this material, I just wonder if we can omit some of the lines here cause we set e.g. the BAUD later on.
The reason to setup the baud is for the shift register. It only works with programmed baud registers. A soft reset would also work, but as Scott Wood said it would corrupt the last character. On the other hand the character should be corrupted by disabling the UART. I have no preferred solution: programming the UART or a soft reset. Maybe someone wants to decide.
- }
+#endif
while (!(serial_in(&com_port->lsr) & UART_LSR_TEMT)) ;-#endif
serial_out(CONFIG_SYS_NS16550_IER, &com_port->ier);#if (defined(CONFIG_OMAP) && !defined(CONFIG_OMAP3_ZOOM2)) || \
I managed to apply your patch anyhow. A short test on a tricorder board showed no harm to the boot process. So please get your patch clean and resend, then I will add my tested-by.
As Javier pointed out please think about using the CONFIG_SYS_NS16550_BROKEN_TEMT instead of SPL && OMAP34XX. Another solution could be to have this TEMT | THRE check in unconditionally, this however would require a lot more testing. Especially with the release date in mind.
It's not critical. So I guess it's not needed for this release.
Best regards
Andreas Bießmann
Best regards, Manfred