Hi Lukasz,
On 4/17/24 13:16, lukasz.czechow...@thaumatec.com wrote:
From: Lukasz Czechowski <lukas.czechow...@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.czechow...@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
