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). > > 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/ ) Simon > Simon > > > > > Regards, > > Simon > > > > > > > > Simon > > > > > > > > >> > > >> As patman automatically choses the CC addresses, you weren't > > >> on the CC list back then, since that patch covered different filfes. > > >> > > >> > > >> Simon > > >> > > >>>> Since this prevents debugging startup problems instead > > >>>> of helping, let's add a field to 'gd' that prevents > > >>>> calling the _debug_uart_putc() until debug_uart_init() > > >>>> has been called. > > >>>> > > >>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschm...@gmail.com> > > >>>> --- > > >>>> > > >>>> include/asm-generic/global_data.h | 3 +++ > > >>>> include/debug_uart.h | 19 ++++++++++++++----- > > >>>> 2 files changed, 17 insertions(+), 5 deletions(-) > > >>>> > > >>>> diff --git a/include/asm-generic/global_data.h > b/include/asm-generic/global_data.h > > >>>> index c83fc01b76..9de7f48476 100644 > > >>>> --- a/include/asm-generic/global_data.h > > >>>> +++ b/include/asm-generic/global_data.h > > >>>> @@ -122,6 +122,9 @@ typedef struct global_data { > > >>>> struct list_head log_head; /* List of struct > log_device */ > > >>>> int log_fmt; /* Mask containing log > format info */ > > >>>> #endif > > >>>> +#ifdef CONFIG_DEBUG_UART > > >>>> + int debug_uart_initialized; /* No print before > debug_uart_init */ > > >>>> +#endif > > >>> > > >>> There is no requirement that gd be set up before the debug UART is > > >>> running. It certainly isn't on the few platforms I know about. > > >>> > > >>> Regards, > > >>> Simon > > > > > > > > > > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
