Re: Early debug UART not working on AM33XX SoC

[Felix Brack <fb@ltec.ch>]

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