Early debug UART not working on AM33XX SoC

[Simon Glass]

Hi Felix,

On Mon, 31 Jan 2022 at 02:43, Felix Brack <fb at 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 at 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 at 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