Hi Felix,
On Thu, 24 Feb 2022 at 03:01, Felix Brack <f...@ltec.ch> wrote:
>
> Hello Simon,
>
> On 23.02.22 23:58, Simon Glass wrote:
> > Hi Felix,
> >
> > On Mon, 14 Feb 2022 at 09:57, Felix Brack <f...@ltec.ch> wrote:
> >>
> >> The changes from commit 0dba45864b2a ("arm: Init the debug UART")
> >> prevent the early debug UART from being initialized correctly.
> >> By adding a new SoC specific early UART initialization function this
> >
> > SoC-specific
> >
> > You need the hyphen for this to make sense, since you are creating an
> > adjective.
> >
> >> patch provides a generic location to add the required code. This code
> >> has to be SoC and not board specific.
> >
> > board-specific
> >
> >> For the am33xx SoCs the fix consist of configuring early clocks.
> >>
> >> Signed-off-by: Felix Brack <f...@ltec.ch>
> >> ---
> >>
> >> arch/arm/mach-omap2/am33xx/board.c | 7 +++++++
> >> drivers/serial/Kconfig | 13 +++++++++++++
> >> include/debug_uart.h | 15 +++++++++++++++
> >> 3 files changed, 35 insertions(+)
> >
> > More nits below.
> >
> > Reviewed-by: Simon Glass <s...@chromium.org>
> >
> >>
> >> diff --git a/arch/arm/mach-omap2/am33xx/board.c
> >> b/arch/arm/mach-omap2/am33xx/board.c
> >> index c44667668e..a7f0445b93 100644
> >> --- a/arch/arm/mach-omap2/am33xx/board.c
> >> +++ b/arch/arm/mach-omap2/am33xx/board.c
> >> @@ -604,3 +604,10 @@ int arch_cpu_init_dm(void)
> >> #endif
> >> return 0;
> >> }
> >> +
> >> +#if IS_ENABLED(CONFIG_DEBUG_UART_SOC_INIT)
> >> +void soc_debug_uart_init(void)
> >> +{
> >> + setup_early_clocks();
> >> +}
> >> +#endif
> >> diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
> >> index 345d1881f5..3da4064d35 100644
> >> --- a/drivers/serial/Kconfig
> >> +++ b/drivers/serial/Kconfig
> >> @@ -490,6 +490,19 @@ config DEBUG_UART_SHIFT
> >> value. Use this value to specify the shift to use, where 0=byte
> >> registers, 2=32-bit word registers, etc.
> >>
> >> +config DEBUG_UART_SOC_INIT
> >> + bool "Enable SoC-specific debug UART init"
> >> + depends on DEBUG_UART
> >> + help
> >> + Boards using the same SoC might require some common settings
> >> before
> >> + the debug UART can be used. The board specific
> >> board_debug_uart_init()
> >> + is not the right place for such common settings as they would
> >> apply
> >> + to a specific board only instead of all boards using the same
> >> SoC.
> >> + When this option is enabled, the SoC specific function
> >> + soc_debug_uart_init() will be called when debug_uart_init() is
> >> called.
> >> + You can put any code here that is needed to set up the UART
> >> ready for
> >> + use.
> >
> > Please mention the order in which the board and SoC functions are called.
> >
> >> +
> >> config DEBUG_UART_BOARD_INIT
> >> bool "Enable board-specific debug UART init"
> >> depends on DEBUG_UART
> >> diff --git a/include/debug_uart.h b/include/debug_uart.h
> >> index 714b369e6f..ebc84c44cd 100644
> >> --- a/include/debug_uart.h
> >> +++ b/include/debug_uart.h
> >> @@ -42,6 +42,12 @@
> >> * - Immediately afterwards, add DEBUG_UART_FUNCS to define the rest of
> >> the
> >> * functionality (printch(), etc.)
> >> *
> >> + * If your SoC needs additional init for the UART to work, enable
> >> + * CONFIG_DEBUG_UART_SOC_INIT and write a function called
> >> + * soc_debug_uart_init() to perform that init. When debug_uart_init() is
> >> + * called, the init will happen automatically. Board specific code does
> >> not
> >> + * go here, see board_debug_uart_init() below.
> >
> > Again please fix your specifics.
> >
> >
> >> + *
> >> * If your board needs additional init for the UART to work, enable
> >> * CONFIG_DEBUG_UART_BOARD_INIT and write a function called
> >> * board_debug_uart_init() to perform that init. When debug_uart_init() is
> >> @@ -61,6 +67,14 @@
> >> */
> >> void debug_uart_init(void);
> >>
> >> +#ifdef CONFIG_DEBUG_UART_SOC_INIT
> >> +void soc_debug_uart_init(void);
> >> +#else
> >> +static inline void soc_debug_uart_init(void)
> >> +{
> >> +}
> >> +#endif
> >> +
> >> #ifdef CONFIG_DEBUG_UART_BOARD_INIT
> >> void board_debug_uart_init(void);
> >> #else
> >> @@ -192,6 +206,7 @@ void printdec(unsigned int value);
> >> \
> >> void debug_uart_init(void) \
> >> { \
> >> + soc_debug_uart_init(); \
> >> board_debug_uart_init(); \
> >> _debug_uart_init(); \
> >> _DEBUG_UART_ANNOUNCE \
> >> --
> >> 2.25.1
> >>
> Thanks for the review! However it looks like this patch will never get
> applied. If I'm wrong please let me know and I'll be happy to send a
> fixed version.
OK, I suppose it can wait until another board comes along that needs it.
Regards,
Simon
