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

[Marek Vasut]

On 1/14/19 4:21 PM, Simon Goldschmidt wrote:
> 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/

So that's the real fix here ?

-- 
Best regards,
Marek Vasut