[PATCH] ram: rockchip: px30: Replace misleading log message
Łukasz Czechowski
lukasz.czechowski at thaumatec.com
Wed Apr 17 16:57:14 CEST 2024
Hi Quentin,
Maybe I was a little too fast with submitting this patch.
My original point of this commit was to get rid of the message "out",
that is printed without any context every time the board boots, or at
least replace it with something more descriptive, similarly like it is
done with other boards.
I've used the `debug` macro as it was used in the same context in e.g.
rk3399, but also already inside the sdram_px30.c file. Indeed I'll try
to revisit this topic once I'm back.
Best regards,
Łukasz
śr., 17 kwi 2024 o 15:58 Quentin Schulz
<quentin.schulz at theobroma-systems.com> napisał(a):
>
> Hi Lukasz,
>
> On 4/17/24 13:16, lukasz.czechowski at thaumatec.com wrote:
> > From: Lukasz Czechowski <lukas.czechowski at thaumatec.com>
> >
> > Remove the log message "out" from sdram_init function which
> > pollutes the console. It brings no meaningful information and
> > might be unwanted in case silencing the console is required.
> > Instead, add a debug log with a more meaningful message, printed
> > only if DEBUG is set. The same convention is used for other
> > boards, i.e. rk3399.
> >
> > Signed-off-by: Lukasz Czechowski <lukasz.czechowski at thaumatec.com>
> > ---
> > drivers/ram/rockchip/sdram_px30.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/ram/rockchip/sdram_px30.c b/drivers/ram/rockchip/sdram_px30.c
> > index 21498e8957..3b429973cd 100644
> > --- a/drivers/ram/rockchip/sdram_px30.c
> > +++ b/drivers/ram/rockchip/sdram_px30.c
> > @@ -712,7 +712,7 @@ int sdram_init(void)
> >
> > sdram_print_ddr_info(&sdram_params->ch.cap_info, &sdram_params->base, 0);
> >
> > - printascii("out\n");
> > + debug("Finish SDRAM initialization...\n");
>
> Mmmm I don't think this is appropriate. debug() is essentially replaced
> with printf() whenever the selected log level permits. The issue is that
> printf() != printascii() and I have a feeling we use printascii
> explicitly because it is a much smaller way to print data than printf.
>
> Additionally, we're extremely size constrained in TPL on PX30, so
> increasing the string size to print may also not be wise. If we are not
> enable to even build the TPL by defining DEBUG, this is basically dead
> code and it's as good to us as removing it entirely.
>
> So... questions:
> 1) does it actually build if you set #define DEBUG 1 or whatever is
> needed for having the debug message printed? If yes, does it boot?
> 2) What's the size of the TPL with this change and with this
> change+DEBUG set?
>
> What I can suggest instead is to guard all printascii (or at least the
> ones only useful for debugging) with the appropriate symbol, e.g. could
> be CONFIG_RAM_ROCKCHIP_DEBUG.
>
> I have not tested it (only build tested on Ringneck PX30 without
> CONFIG_RAM_ROCKCHIP_DEBUG nor CONFIG_TPL_SERIAL), but here's a quick
> attempt:
>
> """
> diff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig
> index b7a6f100d41..17656e99a2f 100644
> --- a/arch/arm/mach-rockchip/Kconfig
> +++ b/arch/arm/mach-rockchip/Kconfig
> @@ -11,7 +11,7 @@ config ROCKCHIP_PX30
> select TPL_NEEDS_SEPARATE_STACK if TPL
> imply SPL_SEPARATE_BSS
> select SPL_SERIAL
> - select TPL_SERIAL
> + imply TPL_SERIAL
> select DEBUG_UART_BOARD_INIT
> imply ROCKCHIP_COMMON_BOARD
> imply SPL_ROCKCHIP_COMMON_BOARD
> diff --git a/arch/arm/mach-rockchip/px30-board-tpl.c
> b/arch/arm/mach-rockchip/px30-board-tpl.c
> index 637a5e1b18b..8f56147ca7a 100644
> --- a/arch/arm/mach-rockchip/px30-board-tpl.c
> +++ b/arch/arm/mach-rockchip/px30-board-tpl.c
> @@ -36,7 +36,7 @@ void board_init_f(ulong dummy)
> {
> int ret;
>
> -#ifdef CONFIG_DEBUG_UART
> +#if defined(CONFIG_DEBUG_UART) && defined(CONFIG_TPL_SERIAL)
> debug_uart_init();
> /*
> * Debug UART can be used from here if required:
> @@ -51,8 +51,10 @@ void board_init_f(ulong dummy)
>
> secure_timer_init();
> ret = sdram_init();
> +#if defined(CONFIG_DEBUG_UART) && defined(CONFIG_TPL_SERIAL)
> if (ret)
> printascii("sdram_init failed\n");
> +#endif
>
> /* return to maskrom */
> back_to_bootrom(BROM_BOOT_NEXTSTAGE);
> diff --git a/drivers/ram/rockchip/Kconfig b/drivers/ram/rockchip/Kconfig
> index 67c63ecba04..cb59bcf0414 100644
> --- a/drivers/ram/rockchip/Kconfig
> +++ b/drivers/ram/rockchip/Kconfig
> @@ -16,6 +16,8 @@ if RAM_ROCKCHIP
> config RAM_ROCKCHIP_DEBUG
> bool "Rockchip ram drivers debugging"
> default y
> + depends on TPL_SERIAL if TPL_RAM
> + depends on SPL_SERIAL if SPL_RAM
> help
> This enables debugging ram driver API's for the platforms
> based on Rockchip SoCs.
> diff --git a/drivers/ram/rockchip/sdram_common.c
> b/drivers/ram/rockchip/sdram_common.c
> index 60fc90d0a5c..8076cfa9aad 100644
> --- a/drivers/ram/rockchip/sdram_common.c
> +++ b/drivers/ram/rockchip/sdram_common.c
> @@ -231,7 +231,9 @@ int sdram_detect_col(struct sdram_cap_info *cap_info,
> break;
> }
> if (col == 8) {
> +#ifdef CONFIG_RAM_ROCKCHIP_DEBUG
> printascii("col error\n");
> +#endif
> return -1;
> }
>
> @@ -348,7 +350,9 @@ int sdram_detect_row(struct sdram_cap_info *cap_info,
> break;
> }
> if (row == 12) {
> +#ifdef CONFIG_RAM_ROCKCHIP_DEBUG
> printascii("row error");
> +#endif
> return -1;
> }
>
> diff --git a/drivers/ram/rockchip/sdram_px30.c
> b/drivers/ram/rockchip/sdram_px30.c
> index 21498e89570..607d97c268e 100644
> --- a/drivers/ram/rockchip/sdram_px30.c
> +++ b/drivers/ram/rockchip/sdram_px30.c
> @@ -231,8 +231,10 @@ static unsigned int calculate_ddrconfig(struct
> px30_sdram_params *sdram_params)
> ddrconf = i;
> break;
> }
> +#ifdef CONFIG_RAM_ROCKCHIP_DEBUG
> if (i > 6)
> printascii("calculate ddrconfig error\n");
> +#endif
> }
>
> return ddrconf;
> @@ -281,8 +283,10 @@ static void set_ctl_address_map(struct dram_info *dram,
> &addrmap[ddrconf][0], 8 * 4);
> max_row = cs_pst - 1 - 8 - (addrmap[ddrconf][5] & 0xf);
>
> +#ifdef CONFIG_RAM_ROCKCHIP_DEBUG
> if (max_row < 12)
> printascii("set addrmap fail\n");
> +#endif
> /* need to disable row ahead of rank by set to 0xf */
> for (i = 17; i > max_row; i--)
> clrsetbits_le32(pctl_base + DDR_PCTL2_ADDRMAP6 +
> @@ -523,12 +527,16 @@ static int sdram_init_(struct dram_info *dram,
> /* do ddr gate training */
> redo_cs0_training:
> if (data_training(dram, 0, sdram_params->base.dramtype) != 0) {
> +#ifdef CONFIG_RAM_ROCKCHIP_DEBUG
> if (pre_init != 0)
> printascii("DTT cs0 error\n");
> +#endif
> return -1;
> }
> if (check_rd_gate(dram)) {
> +#ifdef CONFIG_RAM_ROCKCHIP_DEBUG
> printascii("re training cs0");
> +#endif
> goto redo_cs0_training;
> }
>
> @@ -543,11 +551,15 @@ redo_cs0_training:
> if (pre_init != 0 && cap_info->rank == 2) {
> redo_cs1_training:
> if (data_training(dram, 1, sdram_params->base.dramtype) != 0) {
> +#ifdef CONFIG_RAM_ROCKCHIP_DEBUG
> printascii("DTT cs1 error\n");
> +#endif
> return -1;
> }
> if (check_rd_gate(dram)) {
> +#ifdef CONFIG_RAM_ROCKCHIP_DEBUG
> printascii("re training cs1");
> +#endif
> goto redo_cs1_training;
> }
> }
> @@ -712,7 +724,9 @@ int sdram_init(void)
>
> sdram_print_ddr_info(&sdram_params->ch.cap_info, &sdram_params->base, 0);
>
> +#ifdef CONFIG_RAM_ROCKCHIP_DEBUG
> printascii("out\n");
> +#endif
> return ret;
> error:
> return (-1);
> """
>
> I haven't put too much thought into it, so this is probably not the
> right way to do it, or it's reaching too much. After all, there is some
> important information that needs to be printed sometimes :) Don't know
> which ones for the DDR init though :/
>
> Cheers,
> Quentin
More information about the U-Boot
mailing list