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. -- Best regards, Marek Vasut _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
