Hi Simon, On 17 October 2018 at 14:02, Simon Goldschmidt <simon.k.r.goldschm...@gmail.com> wrote: > > > Simon Goldschmidt <simon.k.r.goldschm...@gmail.com> schrieb am Mi., 10. Okt. > 2018, 22:16: >> >> On Wed, Oct 10, 2018 at 10:03 PM Simon Glass <s...@chromium.org> wrote: >> > >> > Hi Simon, >> > >> > On 10 October 2018 at 07:28, Simon Goldschmidt >> > <simon.k.r.goldschm...@gmail.com> wrote: >> > > >> > > + Marek (as he commented on the original patch >> > > http://patchwork.ozlabs.org/patch/955765/) >> > > >> > > On 09.10.2018 07:06, Simon Goldschmidt wrote: >> > >> >> > >> On Tue, Oct 9, 2018 at 5:41 AM Simon Glass <s...@chromium.org> wrote: >> > >>> >> > >>> Hi, >> > >>> >> > >>> On 7 October 2018 at 11:52, Simon Goldschmidt >> > >>> <simon.k.r.goldschm...@gmail.com> wrote: >> > >>>> >> > >>>> At least on socfpga gen5, _debug_uart_putc() can be called >> > >>>> before debug_uart_init(), which leaves us stuck in an >> > >>>> infinite loop in the ns16550 debug uart driver. >> > >>> >> > >>> Can you fix that? That is a bug. >> > >> >> > >> I already posted a patch for that but it was rejected: >> > >> http://patchwork.ozlabs.org/patch/955765/ >> > > >> > > >> > > I'd have to add I still thing that that patch is good to fix this: >> > > It checks that the baudrate divisor is set, which effectively is >> > > an 'enable' bit for this hardware. >> > > >> > > There were comments about the style (we can talk about that) >> > > and Marek rejected it because he wanted a generic solution. >> > > But honestly, given your idea that some platforms init the debug >> > > uart before setting up gd, I don't think I can find a solution for >> > > this. >> > > >> > > So I'd really like to get my original patch applied (see above). >> > >> > Yes I think your patch is best - it is specific to the hardware which >> > is the only way you can safely detect whether the UART is enabled. >> > >> > We cannot rely on state like global_data to tell if the debug UART is >> > enabled. We could use a flag in the DATA section on some devices, but >> > that is also pretty nasty and we've been trying to avoid adding such >> > things and using global_data instead. >> > >> > So I am OK with the original patch. >> > >> > However before going further I'd like to understand your thoughts on >> > this question: I feel that you are checking for something that should >> > not happen - i.e. the debug UART must be inited before use, just like >> > any other device. We don't in general put these sorts of checks into >> > other drivers. Why is the debug UART different? >> >> On this specific platform (socfpga cylcone5/gen5), the preloader calls >> a function before initializing the debug uart that is also called later >> and >> that should emit an error in one case. And if this is the case (which it >> can be depending on the boot source), then the system will hang when >> debug uart is enabled (loop forever in the debug uart output code). >> >> Now you can say we can initialize the debug uart earlier on this platform, >> but that doesn't really work as it could be that the above function >> enables >> a bus that is required for the debug uart (on socfpga, I could well >> imagine >> the debug being implemented in FPGA and the offending function >> enables the FPGA bridge).
Don't put your debug UART on device that needs lots of init. Only Intel does that :-) >> >> Then you could argue to remove the offending printf, but I feel that debug >> uart should help in debugging, not prevent it by entering an infinite loop >> when printf is called at the wrong time. >> >> I just feel it's better to lose some printf messages at the start than >> adding >> a hang that's not present without debug uart. >> >> And as much as I'd love to solve this in a general way, the multitudes of >> different startup code does not seem to leave an easy way to solve this >> other than I did...? > > > So, without further answers, can we push the original patch? ( > http://patchwork.ozlabs.org/patch/955765/ ) I think so. Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
