From mboxrd@z Thu Jan 1 00:00:00 1970 From: Manfred Huber Date: Thu, 28 Mar 2013 07:06:36 +0100 Subject: [U-Boot] [PATCH 1/1 v2] omap3_beagle: Enabling UART3 first allows the Transmitter to be empty In-Reply-To: <5152F60E.50600@gmail.com> References: <1679653192.50708.1361970554739.JavaMail.ngmail@webmail21.arcor-online.net> <51447012.6010303@arcor.de> <20130319144957.GF25919@bill-the-cat> <5150C987.1040802@arcor.de> <5152F60E.50600@gmail.com> Message-ID: <5153DDEC.6030300@arcor.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 2013-03-27 14:37, Andreas Bie?mann wrote: > Dear Manfred Huber, > > ---8<--- > abiessmann at 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 at 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 at 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 >> --- >> 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