| Message ID | 20180809190420.28093-6-simon.k.r.goldschmidt@gmail.com |
|---|---|
| State | Accepted |
| Commit | 6f57c34473d37b8da5e6a3764d0d377d748aeef1 |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | Get socfpga gen5 SPL working again. | expand |
On Thu, Aug 9, 2018 at 2:08 PM Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> wrote: > > If _debug_uart_putc() is called before _debug_uart_init(), the > ns16550 debug uart driver hangs in a tight loop waiting for the > tx FIFO to get empty. > > As this can happen via a printf sneaking in before the port calls > debug_uart_init(), let's rather ignore characters before the debug > uart is initialized. > > This is done by reading the baudrate divisor and aborting if is zero. > > Tested on socfpga_cyclone5_socrates. > > Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> > --- > v2: > - this patch is new in v2 of the series. It replaces the printf/debug > change in reset_manager_gen5.c from v1 > > drivers/serial/ns16550.c | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) > > diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c > index 9c80090aa7..475075c03c 100644 > --- a/drivers/serial/ns16550.c > +++ b/drivers/serial/ns16550.c > @@ -266,12 +266,26 @@ static inline void _debug_uart_init(void) > serial_dout(&com_port->lcr, UART_LCRVAL); > } > > +static inline int NS16550_read_baud_divisor(struct NS16550 *com_port) > +{ > + int ret; > + > + serial_dout(&com_port->lcr, UART_LCR_BKSE | UART_LCRVAL); > + ret = serial_din(&com_port->dll) & 0xff; > + ret |= (serial_din(&com_port->dlm) & 0xff) << 8; > + serial_dout(&com_port->lcr, UART_LCRVAL); > + > + return ret; > +} > + > static inline void _debug_uart_putc(int ch) > { > struct NS16550 *com_port = (struct NS16550 *)CONFIG_DEBUG_UART_BASE; > > - while (!(serial_din(&com_port->lsr) & UART_LSR_THRE)) > - ; > + while (!(serial_din(&com_port->lsr) & UART_LSR_THRE)) { > + if (!NS16550_read_baud_divisor(com_port)) Unless there is a change that the read_baud_divisor will change while we're waiting for the character, could we move this check before the while statement? This would reduce the check for the divisor to 1x and the while statement would only have one comparison to do. I realize it's rather trivial, but the way I see it, there is no reason to do the while statement at all if the read_baud_divisor fails and there if there is a baud divisor, we should only need to check it once. just my two-cents. adam > + return; > + } > serial_dout(&com_port->thr, ch); > } > > -- > 2.17.1 > > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > https://lists.denx.de/listinfo/u-boot
On 08/09/2018 11:13 PM, Adam Ford wrote: > On Thu, Aug 9, 2018 at 2:08 PM Simon Goldschmidt > <simon.k.r.goldschmidt@gmail.com> wrote: >> >> If _debug_uart_putc() is called before _debug_uart_init(), the >> ns16550 debug uart driver hangs in a tight loop waiting for the >> tx FIFO to get empty. >> >> As this can happen via a printf sneaking in before the port calls >> debug_uart_init(), let's rather ignore characters before the debug >> uart is initialized. >> >> This is done by reading the baudrate divisor and aborting if is zero. >> >> Tested on socfpga_cyclone5_socrates. >> >> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> >> --- >> v2: >> - this patch is new in v2 of the series. It replaces the printf/debug >> change in reset_manager_gen5.c from v1 >> >> drivers/serial/ns16550.c | 18 ++++++++++++++++-- >> 1 file changed, 16 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c >> index 9c80090aa7..475075c03c 100644 >> --- a/drivers/serial/ns16550.c >> +++ b/drivers/serial/ns16550.c >> @@ -266,12 +266,26 @@ static inline void _debug_uart_init(void) >> serial_dout(&com_port->lcr, UART_LCRVAL); >> } >> >> +static inline int NS16550_read_baud_divisor(struct NS16550 *com_port) >> +{ >> + int ret; >> + >> + serial_dout(&com_port->lcr, UART_LCR_BKSE | UART_LCRVAL); >> + ret = serial_din(&com_port->dll) & 0xff; >> + ret |= (serial_din(&com_port->dlm) & 0xff) << 8; >> + serial_dout(&com_port->lcr, UART_LCRVAL); >> + >> + return ret; >> +} >> + >> static inline void _debug_uart_putc(int ch) >> { >> struct NS16550 *com_port = (struct NS16550 *)CONFIG_DEBUG_UART_BASE; >> >> - while (!(serial_din(&com_port->lsr) & UART_LSR_THRE)) >> - ; >> + while (!(serial_din(&com_port->lsr) & UART_LSR_THRE)) { >> + if (!NS16550_read_baud_divisor(com_port)) > > Unless there is a change that the read_baud_divisor will change while > we're waiting for the character, could we move this check before the > while statement? This would reduce the check for the divisor to 1x > and the while statement would only have one comparison to do. I > realize it's rather trivial, but the way I see it, there is no reason > to do the while statement at all if the read_baud_divisor fails and > there if there is a baud divisor, we should only need to check it > once. This looks like a massive hack -- what about having a flag which says that the debug uart was/was not inited somewhere ?
On Fri, Aug 10, 2018 at 12:45 AM, Marek Vasut <marex@denx.de> wrote: > On 08/09/2018 11:13 PM, Adam Ford wrote: >> On Thu, Aug 9, 2018 at 2:08 PM Simon Goldschmidt >> <simon.k.r.goldschmidt@gmail.com> wrote: >>> >>> If _debug_uart_putc() is called before _debug_uart_init(), the >>> ns16550 debug uart driver hangs in a tight loop waiting for the >>> tx FIFO to get empty. >>> >>> As this can happen via a printf sneaking in before the port calls >>> debug_uart_init(), let's rather ignore characters before the debug >>> uart is initialized. >>> >>> This is done by reading the baudrate divisor and aborting if is zero. >>> static inline void _debug_uart_putc(int ch) >>> { >>> struct NS16550 *com_port = (struct NS16550 *)CONFIG_DEBUG_UART_BASE; >>> + while (!(serial_din(&com_port->lsr) & UART_LSR_THRE)) { >>> + if (!NS16550_read_baud_divisor(com_port)) >> >> Unless there is a change that the read_baud_divisor will change while >> we're waiting for the character, could we move this check before the >> while statement? This would reduce the check for the divisor to 1x >> and the while statement would only have one comparison to do. I >> realize it's rather trivial, but the way I see it, there is no reason >> to do the while statement at all if the read_baud_divisor fails and >> there if there is a baud divisor, we should only need to check it >> once. > > This looks like a massive hack -- what about having a flag which says > that the debug uart was/was not inited somewhere ? Agree, why not to cache divisor value, for example, instead of doing slow I/O?
On 08/10/2018 12:35 AM, Andy Shevchenko wrote: > On Fri, Aug 10, 2018 at 12:45 AM, Marek Vasut <marex@denx.de> wrote: >> On 08/09/2018 11:13 PM, Adam Ford wrote: >>> On Thu, Aug 9, 2018 at 2:08 PM Simon Goldschmidt >>> <simon.k.r.goldschmidt@gmail.com> wrote: >>>> >>>> If _debug_uart_putc() is called before _debug_uart_init(), the >>>> ns16550 debug uart driver hangs in a tight loop waiting for the >>>> tx FIFO to get empty. >>>> >>>> As this can happen via a printf sneaking in before the port calls >>>> debug_uart_init(), let's rather ignore characters before the debug >>>> uart is initialized. >>>> >>>> This is done by reading the baudrate divisor and aborting if is zero. > >>>> static inline void _debug_uart_putc(int ch) >>>> { >>>> struct NS16550 *com_port = (struct NS16550 *)CONFIG_DEBUG_UART_BASE; > >>>> + while (!(serial_din(&com_port->lsr) & UART_LSR_THRE)) { >>>> + if (!NS16550_read_baud_divisor(com_port)) >>> >>> Unless there is a change that the read_baud_divisor will change while >>> we're waiting for the character, could we move this check before the >>> while statement? This would reduce the check for the divisor to 1x >>> and the while statement would only have one comparison to do. I >>> realize it's rather trivial, but the way I see it, there is no reason >>> to do the while statement at all if the read_baud_divisor fails and >>> there if there is a baud divisor, we should only need to check it >>> once. >> >> This looks like a massive hack -- what about having a flag which says >> that the debug uart was/was not inited somewhere ? > > Agree, why not to cache divisor value, for example, instead of doing slow I/O? But why do we care about the divisor at all ? The real problem I believe is that someone can call debug UART print/read functions before it is inited.
On 10.08.2018 00:41, Marek Vasut wrote: > On 08/10/2018 12:35 AM, Andy Shevchenko wrote: >> On Fri, Aug 10, 2018 at 12:45 AM, Marek Vasut <marex@denx.de> wrote: >>> On 08/09/2018 11:13 PM, Adam Ford wrote: >>>> On Thu, Aug 9, 2018 at 2:08 PM Simon Goldschmidt >>>> <simon.k.r.goldschmidt@gmail.com> wrote: >>>>> If _debug_uart_putc() is called before _debug_uart_init(), the >>>>> ns16550 debug uart driver hangs in a tight loop waiting for the >>>>> tx FIFO to get empty. >>>>> >>>>> As this can happen via a printf sneaking in before the port calls >>>>> debug_uart_init(), let's rather ignore characters before the debug >>>>> uart is initialized. >>>>> >>>>> This is done by reading the baudrate divisor and aborting if is zero. >>>>> static inline void _debug_uart_putc(int ch) >>>>> { >>>>> struct NS16550 *com_port = (struct NS16550 *)CONFIG_DEBUG_UART_BASE; >>>>> + while (!(serial_din(&com_port->lsr) & UART_LSR_THRE)) { >>>>> + if (!NS16550_read_baud_divisor(com_port)) >>>> Unless there is a change that the read_baud_divisor will change while >>>> we're waiting for the character, could we move this check before the >>>> while statement? This would reduce the check for the divisor to 1x >>>> and the while statement would only have one comparison to do. I >>>> realize it's rather trivial, but the way I see it, there is no reason >>>> to do the while statement at all if the read_baud_divisor fails and >>>> there if there is a baud divisor, we should only need to check it >>>> once. >>> This looks like a massive hack -- what about having a flag which says >>> that the debug uart was/was not inited somewhere ? >> Agree, why not to cache divisor value, for example, instead of doing slow I/O? > But why do we care about the divisor at all ? Because if the divisor is zero, the UART is disabled. > The real problem I believe > is that someone can call debug UART print/read functions before it is > inited. > I know this is a hack. I did it like that because I need something like this to get debug uart to work on socfpga gen5 (there always is a printf before debug uart init is possible). A generic solution for all debug uarts would be better of course, but given the point in SPL runtime, we might have to add a field to 'gd' for that, or does a global variable work at that point already? Simon
On 08/10/2018 07:22 AM, Simon Goldschmidt wrote: > On 10.08.2018 00:41, Marek Vasut wrote: >> On 08/10/2018 12:35 AM, Andy Shevchenko wrote: >>> On Fri, Aug 10, 2018 at 12:45 AM, Marek Vasut <marex@denx.de> wrote: >>>> On 08/09/2018 11:13 PM, Adam Ford wrote: >>>>> On Thu, Aug 9, 2018 at 2:08 PM Simon Goldschmidt >>>>> <simon.k.r.goldschmidt@gmail.com> wrote: >>>>>> If _debug_uart_putc() is called before _debug_uart_init(), the >>>>>> ns16550 debug uart driver hangs in a tight loop waiting for the >>>>>> tx FIFO to get empty. >>>>>> >>>>>> As this can happen via a printf sneaking in before the port calls >>>>>> debug_uart_init(), let's rather ignore characters before the debug >>>>>> uart is initialized. >>>>>> >>>>>> This is done by reading the baudrate divisor and aborting if is zero. >>>>>> static inline void _debug_uart_putc(int ch) >>>>>> { >>>>>> struct NS16550 *com_port = (struct NS16550 >>>>>> *)CONFIG_DEBUG_UART_BASE; >>>>>> + while (!(serial_din(&com_port->lsr) & UART_LSR_THRE)) { >>>>>> + if (!NS16550_read_baud_divisor(com_port)) >>>>> Unless there is a change that the read_baud_divisor will change while >>>>> we're waiting for the character, could we move this check before the >>>>> while statement? This would reduce the check for the divisor to 1x >>>>> and the while statement would only have one comparison to do. I >>>>> realize it's rather trivial, but the way I see it, there is no reason >>>>> to do the while statement at all if the read_baud_divisor fails and >>>>> there if there is a baud divisor, we should only need to check it >>>>> once. >>>> This looks like a massive hack -- what about having a flag which says >>>> that the debug uart was/was not inited somewhere ? >>> Agree, why not to cache divisor value, for example, instead of doing >>> slow I/O? >> But why do we care about the divisor at all ? > > Because if the divisor is zero, the UART is disabled. > >> The real problem I believe >> is that someone can call debug UART print/read functions before it is >> inited. >> > I know this is a hack. I did it like that because I need something like > this to get debug uart to work on socfpga gen5 (there always is a printf > before debug uart init is possible). > > A generic solution for all debug uarts would be better of course, but > given the point in SPL runtime, we might have to add a field to 'gd' for > that, or does a global variable work at that point already? GD field might be needed indeed.
On 10.08.2018 11:51, Marek Vasut wrote: > On 08/10/2018 07:22 AM, Simon Goldschmidt wrote: >> On 10.08.2018 00:41, Marek Vasut wrote: >>> On 08/10/2018 12:35 AM, Andy Shevchenko wrote: >>>> On Fri, Aug 10, 2018 at 12:45 AM, Marek Vasut <marex@denx.de> wrote: >>>>> On 08/09/2018 11:13 PM, Adam Ford wrote: >>>>>> On Thu, Aug 9, 2018 at 2:08 PM Simon Goldschmidt >>>>>> <simon.k.r.goldschmidt@gmail.com> wrote: >>>>>>> If _debug_uart_putc() is called before _debug_uart_init(), the >>>>>>> ns16550 debug uart driver hangs in a tight loop waiting for the >>>>>>> tx FIFO to get empty. >>>>>>> >>>>>>> As this can happen via a printf sneaking in before the port calls >>>>>>> debug_uart_init(), let's rather ignore characters before the debug >>>>>>> uart is initialized. >>>>>>> >>>>>>> This is done by reading the baudrate divisor and aborting if is zero. >>>>>>> static inline void _debug_uart_putc(int ch) >>>>>>> { >>>>>>> struct NS16550 *com_port = (struct NS16550 >>>>>>> *)CONFIG_DEBUG_UART_BASE; >>>>>>> + while (!(serial_din(&com_port->lsr) & UART_LSR_THRE)) { >>>>>>> + if (!NS16550_read_baud_divisor(com_port)) >>>>>> Unless there is a change that the read_baud_divisor will change while >>>>>> we're waiting for the character, could we move this check before the >>>>>> while statement? This would reduce the check for the divisor to 1x >>>>>> and the while statement would only have one comparison to do. I >>>>>> realize it's rather trivial, but the way I see it, there is no reason >>>>>> to do the while statement at all if the read_baud_divisor fails and >>>>>> there if there is a baud divisor, we should only need to check it >>>>>> once. >>>>> This looks like a massive hack -- what about having a flag which says >>>>> that the debug uart was/was not inited somewhere ? >>>> Agree, why not to cache divisor value, for example, instead of doing >>>> slow I/O? >>> But why do we care about the divisor at all ? >> Because if the divisor is zero, the UART is disabled. >> >>> The real problem I believe >>> is that someone can call debug UART print/read functions before it is >>> inited. >>> >> I know this is a hack. I did it like that because I need something like >> this to get debug uart to work on socfpga gen5 (there always is a printf >> before debug uart init is possible). >> >> A generic solution for all debug uarts would be better of course, but >> given the point in SPL runtime, we might have to add a field to 'gd' for >> that, or does a global variable work at that point already? > GD field might be needed indeed. > Right. I'll drop this patch in the next version of the series and instead I'll try to work out something that works for all debug uarts drivers using a gd field. Thanks, Simon
On 08/10/2018 01:37 PM, Simon Goldschmidt wrote: > On 10.08.2018 11:51, Marek Vasut wrote: >> On 08/10/2018 07:22 AM, Simon Goldschmidt wrote: >>> On 10.08.2018 00:41, Marek Vasut wrote: >>>> On 08/10/2018 12:35 AM, Andy Shevchenko wrote: >>>>> On Fri, Aug 10, 2018 at 12:45 AM, Marek Vasut <marex@denx.de> wrote: >>>>>> On 08/09/2018 11:13 PM, Adam Ford wrote: >>>>>>> On Thu, Aug 9, 2018 at 2:08 PM Simon Goldschmidt >>>>>>> <simon.k.r.goldschmidt@gmail.com> wrote: >>>>>>>> If _debug_uart_putc() is called before _debug_uart_init(), the >>>>>>>> ns16550 debug uart driver hangs in a tight loop waiting for the >>>>>>>> tx FIFO to get empty. >>>>>>>> >>>>>>>> As this can happen via a printf sneaking in before the port calls >>>>>>>> debug_uart_init(), let's rather ignore characters before the debug >>>>>>>> uart is initialized. >>>>>>>> >>>>>>>> This is done by reading the baudrate divisor and aborting if is >>>>>>>> zero. >>>>>>>> static inline void _debug_uart_putc(int ch) >>>>>>>> { >>>>>>>> struct NS16550 *com_port = (struct NS16550 >>>>>>>> *)CONFIG_DEBUG_UART_BASE; >>>>>>>> + while (!(serial_din(&com_port->lsr) & UART_LSR_THRE)) { >>>>>>>> + if (!NS16550_read_baud_divisor(com_port)) >>>>>>> Unless there is a change that the read_baud_divisor will change >>>>>>> while >>>>>>> we're waiting for the character, could we move this check before the >>>>>>> while statement? This would reduce the check for the divisor to 1x >>>>>>> and the while statement would only have one comparison to do. I >>>>>>> realize it's rather trivial, but the way I see it, there is no >>>>>>> reason >>>>>>> to do the while statement at all if the read_baud_divisor fails and >>>>>>> there if there is a baud divisor, we should only need to check it >>>>>>> once. >>>>>> This looks like a massive hack -- what about having a flag which says >>>>>> that the debug uart was/was not inited somewhere ? >>>>> Agree, why not to cache divisor value, for example, instead of doing >>>>> slow I/O? >>>> But why do we care about the divisor at all ? >>> Because if the divisor is zero, the UART is disabled. >>> >>>> The real problem I believe >>>> is that someone can call debug UART print/read functions before it is >>>> inited. >>>> >>> I know this is a hack. I did it like that because I need something like >>> this to get debug uart to work on socfpga gen5 (there always is a printf >>> before debug uart init is possible). >>> >>> A generic solution for all debug uarts would be better of course, but >>> given the point in SPL runtime, we might have to add a field to 'gd' for >>> that, or does a global variable work at that point already? >> GD field might be needed indeed. >> > Right. I'll drop this patch in the next version of the series and > instead I'll try to work out something that works for all debug uarts > drivers using a gd field. Thanks, much appreciated :)
On 9 August 2018 at 13:04, Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> wrote: > If _debug_uart_putc() is called before _debug_uart_init(), the > ns16550 debug uart driver hangs in a tight loop waiting for the > tx FIFO to get empty. > > As this can happen via a printf sneaking in before the port calls > debug_uart_init(), let's rather ignore characters before the debug > uart is initialized. > > This is done by reading the baudrate divisor and aborting if is zero. > > Tested on socfpga_cyclone5_socrates. > > Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> > --- > v2: > - this patch is new in v2 of the series. It replaces the printf/debug > change in reset_manager_gen5.c from v1 > > drivers/serial/ns16550.c | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) We cannot use global_data before it is set up, so I think this is the best solution. Acked-by: Simon Glass <sjg@chromium.org> Regards, Simon
On 10/19/2018 05:25 AM, Simon Glass wrote: > On 9 August 2018 at 13:04, Simon Goldschmidt > <simon.k.r.goldschmidt@gmail.com> wrote: >> If _debug_uart_putc() is called before _debug_uart_init(), the >> ns16550 debug uart driver hangs in a tight loop waiting for the >> tx FIFO to get empty. >> >> As this can happen via a printf sneaking in before the port calls >> debug_uart_init(), let's rather ignore characters before the debug >> uart is initialized. >> >> This is done by reading the baudrate divisor and aborting if is zero. >> >> Tested on socfpga_cyclone5_socrates. >> >> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> >> --- >> v2: >> - this patch is new in v2 of the series. It replaces the printf/debug >> change in reset_manager_gen5.c from v1 >> >> drivers/serial/ns16550.c | 18 ++++++++++++++++-- >> 1 file changed, 16 insertions(+), 2 deletions(-) > > We cannot use global_data before it is set up, so I think this is the > best solution. > > Acked-by: Simon Glass <sjg@chromium.org> So there's no GD available when using debug uart ? Hum. btw. Does the NS16550_read_baud_divisor() need to be called within the while loop ?
On Tue, Oct 23, 2018 at 11:01 AM Marek Vasut <marex@denx.de> wrote: > > On 10/19/2018 05:25 AM, Simon Glass wrote: > > On 9 August 2018 at 13:04, Simon Goldschmidt > > <simon.k.r.goldschmidt@gmail.com> wrote: > >> If _debug_uart_putc() is called before _debug_uart_init(), the > >> ns16550 debug uart driver hangs in a tight loop waiting for the > >> tx FIFO to get empty. > >> > >> As this can happen via a printf sneaking in before the port calls > >> debug_uart_init(), let's rather ignore characters before the debug > >> uart is initialized. > >> > >> This is done by reading the baudrate divisor and aborting if is zero. > >> > >> Tested on socfpga_cyclone5_socrates. > >> > >> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> > >> --- > >> v2: > >> - this patch is new in v2 of the series. It replaces the printf/debug > >> change in reset_manager_gen5.c from v1 > >> > >> drivers/serial/ns16550.c | 18 ++++++++++++++++-- > >> 1 file changed, 16 insertions(+), 2 deletions(-) > > > > We cannot use global_data before it is set up, so I think this is the > > best solution. > > > > Acked-by: Simon Glass <sjg@chromium.org> > > So there's no GD available when using debug uart ? Hum. > > btw. Does the NS16550_read_baud_divisor() need to be called within the > while loop ? No. I just decided to put it there so that it is not executed unless we wait in a tight loop anyway. So if the transmit buffer is empty, code flow is unchanged by this patch. Only if it is not empty, the baud divisor is read. But in this case, we would cycle the while loop a few hundred times, anyway, I guess... Simon
On Tue, Oct 23, 2018 at 11:01 AM Marek Vasut <marex@denx.de> wrote: > > On 10/19/2018 05:25 AM, Simon Glass wrote: > > On 9 August 2018 at 13:04, Simon Goldschmidt > > <simon.k.r.goldschmidt@gmail.com> wrote: > >> If _debug_uart_putc() is called before _debug_uart_init(), the > >> ns16550 debug uart driver hangs in a tight loop waiting for the > >> tx FIFO to get empty. > >> > >> As this can happen via a printf sneaking in before the port calls > >> debug_uart_init(), let's rather ignore characters before the debug > >> uart is initialized. > >> > >> This is done by reading the baudrate divisor and aborting if is zero. > >> > >> Tested on socfpga_cyclone5_socrates. > >> > >> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> > >> --- > >> v2: > >> - this patch is new in v2 of the series. It replaces the printf/debug > >> change in reset_manager_gen5.c from v1 > >> > >> drivers/serial/ns16550.c | 18 ++++++++++++++++-- > >> 1 file changed, 16 insertions(+), 2 deletions(-) > > > > We cannot use global_data before it is set up, so I think this is the > > best solution. > > > > Acked-by: Simon Glass <sjg@chromium.org> > > So there's no GD available when using debug uart ? Hum. > > btw. Does the NS16550_read_baud_divisor() need to be called within the > while loop ? No. I just decided to put it there so that it is not executed unless we wait in a tight loop anyway. So if the transmit buffer is empty, code flow is unchanged by this patch. Only if it is not empty, the baud divisor is read. But in this case, we would cycle the while loop a few hundred times, anyway, I guess... Simon Applied to u-boot-dm/next, thanks!
diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c index 9c80090aa7..475075c03c 100644 --- a/drivers/serial/ns16550.c +++ b/drivers/serial/ns16550.c @@ -266,12 +266,26 @@ static inline void _debug_uart_init(void) serial_dout(&com_port->lcr, UART_LCRVAL); } +static inline int NS16550_read_baud_divisor(struct NS16550 *com_port) +{ + int ret; + + serial_dout(&com_port->lcr, UART_LCR_BKSE | UART_LCRVAL); + ret = serial_din(&com_port->dll) & 0xff; + ret |= (serial_din(&com_port->dlm) & 0xff) << 8; + serial_dout(&com_port->lcr, UART_LCRVAL); + + return ret; +} + static inline void _debug_uart_putc(int ch) { struct NS16550 *com_port = (struct NS16550 *)CONFIG_DEBUG_UART_BASE; - while (!(serial_din(&com_port->lsr) & UART_LSR_THRE)) - ; + while (!(serial_din(&com_port->lsr) & UART_LSR_THRE)) { + if (!NS16550_read_baud_divisor(com_port)) + return; + } serial_dout(&com_port->thr, ch); }
If _debug_uart_putc() is called before _debug_uart_init(), the ns16550 debug uart driver hangs in a tight loop waiting for the tx FIFO to get empty. As this can happen via a printf sneaking in before the port calls debug_uart_init(), let's rather ignore characters before the debug uart is initialized. This is done by reading the baudrate divisor and aborting if is zero. Tested on socfpga_cyclone5_socrates. Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> --- v2: - this patch is new in v2 of the series. It replaces the printf/debug change in reset_manager_gen5.c from v1 drivers/serial/ns16550.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)