Early debug UART not working on AM33XX SoC

Early debug UART not working on AM33XX SoC

[Felix Brack]

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.

-- 
Felix

Re: Early debug UART not working on AM33XX SoC

[Simon Glass]

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 ?

Regards,
Simon

Re: Early debug UART not working on AM33XX SoC

[Felix Brack]

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
2. the now redundant call to setup_early_clocks() in am33xx/board.c
   remains (at least it does not seem to hurt ;-))

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

Re: Early debug UART not working on AM33XX SoC

[Simon Glass]

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.

> 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

Re: Early debug UART not working on AM33XX SoC

[Felix Brack]

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.

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?

>> 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

Re: Early debug UART not working on AM33XX SoC

[Simon Glass]

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.

>
> 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

Re: 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@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.

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

Re: Early debug UART not working on AM33XX SoC

[Simon Glass]

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.

Regards,
Simon

Re: 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@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()?
>
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

Re: 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@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