Early debug UART not working on AM33XX SoC

[Simon Glass]

Hi Felix,

On Tue, 1 Feb 2022 at 03:48, Felix Brack <fb at 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 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.
> >
> 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