* Early debug UART not working on AM33XX SoC
@ 2022-01-26 14:02 Felix Brack
2022-01-27 15:05 ` Simon Glass
0 siblings, 1 reply; 10+ messages in thread
From: Felix Brack @ 2022-01-26 14:02 UTC (permalink / raw)
To: sjg; +Cc: u-boot
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.
--
Felix
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: Early debug UART not working on AM33XX SoC 2022-01-26 14:02 Early debug UART not working on AM33XX SoC Felix Brack @ 2022-01-27 15:05 ` Simon Glass 2022-01-27 16:27 ` Felix Brack 0 siblings, 1 reply; 10+ messages in thread From: Simon Glass @ 2022-01-27 15:05 UTC (permalink / raw) To: Felix Brack; +Cc: U-Boot Mailing List Hi Felix, On Wed, 26 Jan 2022 at 07:02, Felix Brack <fb@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 ? Regards, Simon ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Early debug UART not working on AM33XX SoC 2022-01-27 15:05 ` Simon Glass @ 2022-01-27 16:27 ` Felix Brack 2022-01-27 17:33 ` Simon Glass 0 siblings, 1 reply; 10+ messages in thread From: Felix Brack @ 2022-01-27 16:27 UTC (permalink / raw) To: Simon Glass; +Cc: U-Boot Mailing List Hello Simon, On 27.01.22 16:05, Simon Glass wrote: > Hi Felix, > > On Wed, 26 Jan 2022 at 07:02, Felix Brack <fb@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 2. the now redundant call to setup_early_clocks() in am33xx/board.c remains (at least it does not seem to hurt ;-)) I will prepare a patch with a modified board_debug_uart_init() for the PDU001 board then. -- Many thanks for your help and kind regards, Felix ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Early debug UART not working on AM33XX SoC 2022-01-27 16:27 ` Felix Brack @ 2022-01-27 17:33 ` Simon Glass 2022-01-31 9:43 ` Felix Brack 0 siblings, 1 reply; 10+ messages in thread From: Simon Glass @ 2022-01-27 17:33 UTC (permalink / raw) To: Felix Brack; +Cc: U-Boot Mailing List Hi Felix, On Thu, 27 Jan 2022 at 09:27, Felix Brack <fb@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 <fb@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. > 2. the now redundant call to setup_early_clocks() in am33xx/board.c > remains (at least it does not seem to hurt ;-)) Yes, it is possible to use flags to avoid that, but might not be worth it. > > I will prepare a patch with a modified board_debug_uart_init() for the > PDU001 board then. > > -- > Many thanks for your help and kind regards, Felix > Regards, Simon ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Early debug UART not working on AM33XX SoC 2022-01-27 17:33 ` Simon Glass @ 2022-01-31 9:43 ` Felix Brack 2022-01-31 16:12 ` Simon Glass 0 siblings, 1 reply; 10+ messages in thread From: Felix Brack @ 2022-01-31 9:43 UTC (permalink / raw) To: Simon Glass; +Cc: U-Boot Mailing List Hello Simon On 27.01.22 18:33, Simon Glass wrote: > Hi Felix, > > On Thu, 27 Jan 2022 at 09:27, Felix Brack <fb@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 <fb@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. 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? >> 2. the now redundant call to setup_early_clocks() in am33xx/board.c >> remains (at least it does not seem to hurt ;-)) > > Yes, it is possible to use flags to avoid that, but might not be worth it. > >> >> I will prepare a patch with a modified board_debug_uart_init() for the >> PDU001 board then. >> >> -- >> Many thanks for your help and kind regards, Felix >> > > Regards, > Simon -- Regards, Felix ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Early debug UART not working on AM33XX SoC 2022-01-31 9:43 ` Felix Brack @ 2022-01-31 16:12 ` Simon Glass 2022-02-01 10:48 ` Felix Brack 0 siblings, 1 reply; 10+ messages in thread From: Simon Glass @ 2022-01-31 16:12 UTC (permalink / raw) To: Felix Brack; +Cc: U-Boot Mailing List Hi Felix, On Mon, 31 Jan 2022 at 02:43, Felix Brack <fb@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 <fb@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 <fb@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. > > 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. > > >> 2. the now redundant call to setup_early_clocks() in am33xx/board.c > >> remains (at least it does not seem to hurt ;-)) > > > > Yes, it is possible to use flags to avoid that, but might not be worth it. > > > >> > >> I will prepare a patch with a modified board_debug_uart_init() for the > >> PDU001 board then. > >> > >> -- > >> Many thanks for your help and kind regards, Felix Regards, Simon ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Early debug UART not working on AM33XX SoC 2022-01-31 16:12 ` Simon Glass @ 2022-02-01 10:48 ` Felix Brack 2022-02-01 14:05 ` Simon Glass 0 siblings, 1 reply; 10+ messages in thread From: Felix Brack @ 2022-02-01 10:48 UTC (permalink / raw) To: Simon Glass; +Cc: U-Boot Mailing List Hello Simon, On 31.01.22 17:12, Simon Glass wrote: > Hi Felix, > > On Mon, 31 Jan 2022 at 02:43, Felix Brack <fb@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 <fb@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 <fb@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. But where to place the default, empty implementation? In the common file board_init.c probably? 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. > >> >>>> 2. the now redundant call to setup_early_clocks() in am33xx/board.c >>>> remains (at least it does not seem to hurt ;-)) >>> >>> Yes, it is possible to use flags to avoid that, but might not be worth it. >>> >>>> >>>> I will prepare a patch with a modified board_debug_uart_init() for the >>>> PDU001 board then. >>>> >>>> -- >>>> Many thanks for your help and kind regards, Felix > > Regards, > Simon -- Regards, Felix ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Early debug UART not working on AM33XX SoC 2022-02-01 10:48 ` Felix Brack @ 2022-02-01 14:05 ` Simon Glass 2022-02-01 16:54 ` Felix Brack 2022-02-01 17:13 ` Felix Brack 0 siblings, 2 replies; 10+ messages in thread From: Simon Glass @ 2022-02-01 14:05 UTC (permalink / raw) To: Felix Brack; +Cc: U-Boot Mailing List Hi Felix, On Tue, 1 Feb 2022 at 03:48, Felix Brack <fb@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 <fb@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 <fb@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 <fb@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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Early debug UART not working on AM33XX SoC 2022-02-01 14:05 ` Simon Glass @ 2022-02-01 16:54 ` Felix Brack 2022-02-01 17:13 ` Felix Brack 1 sibling, 0 replies; 10+ messages in thread From: Felix Brack @ 2022-02-01 16:54 UTC (permalink / raw) To: Simon Glass; +Cc: U-Boot Mailing List Hello Simon, On 01.02.22 15:05, Simon Glass wrote: > Hi Felix, > > On Tue, 1 Feb 2022 at 03:48, Felix Brack <fb@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 <fb@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 <fb@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 <fb@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()? > From what I understand board_debug_uart_init() is (and should remain) a board specific function. Looking at the boards enabling CONFIG_AM33XX I believe mine is currently the only one using board_debug_uart_init(). Hence all other boards enabling CONFIG_AM33XX would have to add a similar board_debug_uart_init() function to get fixed. In fact all ARM boards would probably have to do so. I think "board" is the wrong "level" to fix this, "SoC or platform" is the better "level". By the way: my quick and dirt fix does exactly that. It adds a call to setup_clocks_for_console() in my board's board_debug_uart_init(). So for me that would be fine. > But yes we could create another function. How about > soc_debug_uart_init() as a name, though? > Agreed. It is probably better to use soc_ instead of platform_. The idea behind using a weak function is as follows: it allows placing the call to the new function soc_debug_uart_init() in the generic debug_uart_init() function found in debug_uart.h, just before the existing call to board_debug_uart_init(). This would make it available to everybody. >> >> 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? Yes using CONFIG options is of course possible too. But then my idea of placing it into the generic debug_uart_init() function is bad. From my point of view such a new function should be called first. I would justify that decision with the idea of the more generic things (related to the SoC/platform) to be done before the more specific things (related to the board). But that could just as well be inverted by somebody having a different point of view. > That is why I feel that doing everything from board_debug_uart_init() > might be better. > Absolutely, as proven by my quick and (probably not so) dirty fix described above. But this is then best left to the individual ARM board maintainers. I only know how to fix this for AM33XX boards. I don't even know if it is a problem on none AM33XX boards at all. >> >> 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 -- Regards, Felix ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Early debug UART not working on AM33XX SoC 2022-02-01 14:05 ` Simon Glass 2022-02-01 16:54 ` Felix Brack @ 2022-02-01 17:13 ` Felix Brack 1 sibling, 0 replies; 10+ messages in thread From: Felix Brack @ 2022-02-01 17:13 UTC (permalink / raw) To: Simon Glass; +Cc: U-Boot Mailing List Hello Simon On 01.02.22 15:05, Simon Glass wrote: > Hi Felix, > > On Tue, 1 Feb 2022 at 03:48, Felix Brack <fb@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 <fb@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 <fb@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 <fb@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. Great! I can't imagine that this should provoke any additional problems. > > Regards, > Simon -- Regards, Felix ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-02-01 17:13 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-01-26 14:02 Early debug UART not working on AM33XX SoC Felix Brack 2022-01-27 15:05 ` Simon Glass 2022-01-27 16:27 ` Felix Brack 2022-01-27 17:33 ` Simon Glass 2022-01-31 9:43 ` Felix Brack 2022-01-31 16:12 ` Simon Glass 2022-02-01 10:48 ` Felix Brack 2022-02-01 14:05 ` Simon Glass 2022-02-01 16:54 ` Felix Brack 2022-02-01 17:13 ` Felix Brack
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.