[PATCH] ram: rockchip: px30: Replace misleading log message

[Quentin Schulz]

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