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 <ma...@denx.de> wrote: >>>> >>>> On 1/8/19 1:06 PM, Simon Goldschmidt wrote: >>>>> On Tue, Jan 8, 2019 at 12:20 PM Marek Vasut <ma...@denx.de> wrote: >>>>>> >>>>>> On 1/8/19 7:41 AM, Simon Goldschmidt wrote: >>>>>>> On Mon, Jan 7, 2019 at 11:58 PM Marek Vasut <ma...@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.goldschm...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
