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

[Simon Goldschmidt]

Am 08.01.2019 um 13:56 schrieb Marek Vasut:
> On 1/8/19 1:42 PM, Simon Goldschmidt wrote:
>> 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.
> 
> Uhhhhhh, unless you find a better option, that'd be fine by me.

This patch should be dropped in favour to another patch taht adds a 
Kconfig option to check the baudrate prescaler:

https://patchwork.ozlabs.org/patch/1022600/

Regards,
Simon