[U-Boot] [PATCH] arm: socfpga: make debug uart work on socfpga_gen5

[Simon Goldschmidt]

On Tue, Jan 8, 2019 at 1:08 PM Marek Vasut <marex at denx.de> wrote:
>
> On 1/8/19 1:06 PM, Simon Goldschmidt wrote:
> > On Tue, Jan 8, 2019 at 12:20 PM Marek Vasut <marex at denx.de> wrote:
> >>
> >> On 1/8/19 7:41 AM, Simon Goldschmidt wrote:
> >>> On Mon, Jan 7, 2019 at 11:58 PM Marek Vasut <marex at denx.de> wrote:
> >>>>
> >>>> On 1/7/19 10:01 PM, Simon Goldschmidt wrote:
> >>>>> Am 07.01.2019 um 21:47 schrieb Marek Vasut:
> >>>>>> On 1/7/19 9:33 PM, Simon Goldschmidt wrote:
> >>>>>>> Am 07.01.2019 um 21:25 schrieb Marek Vasut:
> >>>>>>>> On 1/7/19 9:24 PM, Simon Goldschmidt wrote:
> >>>>>>>>> Am 07.01.2019 um 21:19 schrieb Marek Vasut:
> >>>>>>>>>> On 1/7/19 8:36 PM, Simon Goldschmidt wrote:
> >>>>>>>>>>> When debug UART is enabled on socfpga_gen5, the debug uart driver
> >>>>>>>>>>> hangs
> >>>>>>>>>>> in an endless loop because 'socfpga_bridges_reset' calls printf
> >>>>>>>>>>> before
> >>>>>>>>>>> the debug UART is initialized.
> >>>>>>>>>>>
> >>>>>>>>>>> After the generic fix for this in the UART driver did not work
> >>>>>>>>>>> due to
> >>>>>>>>>>> portability issues, let's just drop this printf statement when
> >>>>>>>>>>> called
> >>>>>>>>>>> from SPL with debug UART enabled.
> >>>>>>>>>>>
> >>>>>>>>>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com>
> >>>>>>>>>>
> >>>>>>>>>> Can we have an un-portable fix which at least works on SoCFPGA ? :)
> >>>>>>>>>
> >>>>>>>>> This one worked on socfpga but broke rockchip:
> >>>>>>>>>
> >>>>>>>>> https://patchwork.ozlabs.org/patch/992553/
> >>>>>>>>>
> >>>>>>>>> However, the message below wasn't shown either with that patch
> >>>>>>>>> applied.
> >>>>>>>>> The code just runs too early to enable the UART.
> >>>>>>>>>
> >>>>>>>>> Do you want to keep the message (although I failed to see in which
> >>>>>>>>> situation it can be printed) or do you just dislike the #ifdef thing?
> >>>>>>>>
> >>>>>>>> I'd like to keep the error message if possible. Is it possible ?
> >>>>>>>
> >>>>>>> I have *never* seen this message yet. I have failed to produce a
> >>>>>>> situation where it is shown.
> >>>>>>
> >>>>>> I believe that.
> >>>>>>
> >>>>>>> This function ('socfpga_bridges_reset') is called 5 times throughout the
> >>>>>>> code, but only *one* single time with 'reset=0' as argument (only with
> >>>>>>> 0, the code in question is executed). And this is in SPL before
> >>>>>>> initializing the console and even before the debug UART can be
> >>>>>>> initialized.
> >>>>>>>
> >>>>>>> As I could see, the printf *is* executed on every boot (I saw the code
> >>>>>>> hanging when enabling debug UART). However, when not booting from FPGA,
> >>>>>>> it is just normal that the FPGA is not ready when running SPL. Why do
> >>>>>>> you want an error message here anyway?
> >>>>>>
> >>>>>> I was under the impression this is an error message, but it might not be
> >>>>>> so ? Maybe the wording is incorrect ?
> >>>>>
> >>>>> Now that I re-read it, "aborting" is incorrect, yes.
> >>>>>
> >>>>> So how should we proceed? This is an error message that can never be
> >>>>> shown (like the code is now) but breaks debug UART.
> >>>>>
> >>>>> I'd say we can drop it altogether. It might only be interesint if (in
> >>>>> the future) that code would get called from somewhere else (i.e. later,
> >>>>> after console initialization).
> >>>>>
> >>>>> Re-reading spl_gen5.c, there are some 'debug' calls before the debug
> >>>>> uart is initialized which probably would need to be removed as well, but
> >>>>> that's a different story...
> >>>>
> >>>> How come those don't hang the system then ?
> >>>
> >>> I just haven't enabled debug output in spl_gen5.c, yet. I guess they would hang
> >>> the system when enabling them.
> >>>
> >>> While it would be easy to remove these debug statements, to be future-proof
> >>> it would of course make sense to make the debug UART robust against this.
> >>>
> >>> But given the problems with Rockchip ns16550, we would need a dedicated
> >>> debug UART for socfpga to solve this. And that would probably mean code
> >>> duplication.
> >>
> >> What is the problem with Rockchip ? I don't want various SoCs blocking
> >> others.
> >
> > I had sent a patch that does not wait for the TX fifo to hold more bytes if the
> > baudrate prescaler is 0 (according to both the socfgpa and the rockchip docs,
> > the UART is disabled if the prescaler is 0).
> >
> > However, it seems that the prescaler was read back as 0 on a rockchip board
> > which caused chars to be missing from the console output.
> >
> > See this mail:
> > https://lists.denx.de/pipermail/u-boot/2018-December/350355.html
> >
> > I checked with Henri and did not find a solution so I reverted the patch:
> > https://patchwork.ozlabs.org/patch/1007211/
> >
> > Keeping this patch but only for selected platforms would be my favourite, but it
> > would at least mean we need yet another debug UART selection, plus some changes
> > to make the "prescaler == 0" detection specific to this new debug UART.
> > Would this be better acceptable?
>
> Doesn't the DT compatible tell you the UART type ? It does, so you can
> match on that and apply the workaround accordingly .

We're talking about debug UART here only. No DT compatible involved.

> Or you can cache the prescaler.

We already had that. Simon mentioned that on some platforms you neither have bss
available nor 'gd' at the point where the debug UART is inialized, so
there's not portable
storage to cache the prescaler, either :-(

The best I can think of right now would probably be to extend the list
of debug UARTS
with an socfpga specific type which compiles the same code as DEBUG_UART_NS16550
but enables this prescaler check.

Regards,
Simon