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

Mon Jan 14 18:53:38 UTC 2019

Am 14.01.2019 um 19:26 schrieb 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 ?

Yes: The real fix is that the uart driver should not sit in an endless 
loop (because the tx buffer does not get empty) when its 'putc' function 
is called but it isn't enabled.

The above mentione patch does this, but it only does so when 
specifically enabled because this patch somehow fails at least on 
rockchip. And since the driver (ns16550) is pretty generic, it might 
fail on other chips, too.

Regards,
Simon