Hi Felix, On Tue, 1 Feb 2022 at 03:48, Felix Brack <f...@ltec.ch> wrote: > > Hello Simon, > > On 31.01.22 17:12, Simon Glass wrote: > > Hi Felix, > > > > On Mon, 31 Jan 2022 at 02:43, Felix Brack <f...@ltec.ch> wrote: > >> > >> Hello Simon > >> > >> On 27.01.22 18:33, Simon Glass wrote: > >>> Hi Felix, > >>> > >>> On Thu, 27 Jan 2022 at 09:27, Felix Brack <f...@ltec.ch> wrote: > >>>> > >>>> Hello Simon, > >>>> > >>>> On 27.01.22 16:05, Simon Glass wrote: > >>>>> Hi Felix, > >>>>> > >>>>> On Wed, 26 Jan 2022 at 07:02, Felix Brack <f...@ltec.ch> wrote: > >>>>>> > >>>>>> Hello Simon, > >>>>>> > >>>>>> I am trying to get the current U-Boot master working on the PDU001 > >>>>>> board. This involves the use of an early debug UART. > >>>>>> > >>>>>> With commit 0dba4586 (arm: Init the debug UART) the early debug UART on > >>>>>> the AM33XX SoC doesn't work anymore. > >>>>>> > >>>>>> By looking at the code involved I believe a call to > >>>>>> setup_clocks_for_console() implemented in clock_am33xx.c before the > >>>>>> call > >>>>>> to debug_uart_init() is missing. This is also what happens prior to > >>>>>> commit 0dba4586 by a call to early_system_init() which in turn calls > >>>>>> setup_early_clocks() which then calls setup_clocks_for_console(). > >>>>>> > >>>>>> My quick and dirty fix consist of inserting a call in crt0.S to > >>>>>> setup_clocks_for_console() just before the call to debug_uart_init() > >>>>>> which was added in commit 0dba4586. I have guarded this call with > >>>>>> #if/#endif testing for CONFIG_AM33XX. The code sequence in crt0.S now > >>>>>> looks like this: > >>>>>> > >>>>>> #if defined(CONFIG_DEBUG_UART) && CONFIG_IS_ENABLED(SERIAL) > >>>>>> #if defined(CONFIG_AM33XX) > >>>>>> bl setup_clocks_for_console > >>>>>> #endif > >>>>>> bl debug_uart_init > >>>>>> #endif > >>>>>> > >>>>>> However, from what I understand the crt0.S is for _all_ ARM boards > >>>>>> hence > >>>>>> it's probably a bad idea polluting it with such #if/#endif tests for > >>>>>> different SoCs. What do you think? > >>>>>> > >>>>>> If my quick and dirty fix is not that dirty I would be happy to send a > >>>>>> patch which would also include the removal of the currently remaining > >>>>>> call to debug_uart_init() in am33xx/board.c > >>>>>> > >>>>>> Please apologize my narrow view of the matter dealing only with AM33XX > >>>>>> SoCs. > >>>>> > >>>>> Are you able to put that call into board_debug_uart_init() and enable > >>>>> CONFIG_DEBUG_UART_BOARD_INIT ? > >>>>> > >>>> Sure I can but there two drawbacks: > >>>> 1. this fixes the problem only for my board, others might remain broken > >>> > >>> I suggest you make the function common to all boards that need it. > >>> > >> I will check that. Maybe the board_debug_uart_init() is not the right > >> place then as it is board specific. Probably better to have a AM33XX > >> platform specific function. > > > > Despite the board_ prefix you can define it in an SoC file, for > > example. It may not always be board-specific. > > > Thanks for the hint! In fact I was not aware of that. However for the > PDU001 board board_debug_uart_init() must remain with the board as it > (also) configures the pin multiplexer which is specific to the PDU001 board. > > For the patch to be useful for other boards too, I think a platform > specific function is required. A weak function named something like > platform_debug_uart_init(). This function could then be implemented in > the platform specific board.c file, in my case the one for AM33XX.
Would it be OK to create a non-weak function that is called from board_debug_uart_init()? But yes we could create another function. How about soc_debug_uart_init() as a name, though? > > But where to place the default, empty implementation? In the common file > board_init.c probably? At present we have a CONFIG option to enable board_debug_uart_init() so we could do the same for soc. But should it run first or second? That is why I feel that doing everything from board_debug_uart_init() might be better. > > Frankly I don't feel really comfortable with my own proposal above. It > sounds a little bit like patchwork to me. On the other hand I don't see > a better solution given the fact that it should solve not just my > problem. Should I use it as foundation for sending a patch anyway? > > >> > >> Having said that there is still something I don't understand: commit > >> 0dba4586 has added a call to debug_uart_init() in crt0.S but not removed > >> any existing call to debug_uart_init(). Hence this function is called > >> twice. > >> Is this intentional (and if so, why ?) or are the remaining calls some > >> sort of leftovers? > > > > Just that I was not confident removing it from the various affected > > boards since some have the same problem as you found with yours. > > Calling it twice is generally fine. > > > Agreed. It is just a little bit confusing especially with > DEBUG_UART_ANNOUNCE enabled. I will do a patch to remove them and see what happens. Regards, Simon