Early debug UART not working on AM33XX SoC

[Felix Brack]

Hello Simon,

On 01.02.22 15:05, Simon Glass wrote:
> 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()?
>
>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