Early debug UART not working on AM33XX SoC

[Felix Brack]

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.

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