| Message ID | 20240417111625.288202-1-lukasz.czechowski@thaumatec.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Kever Yang |
| Headers | show |
| Series | ram: rockchip: px30: Replace misleading log message | expand |
Hi Lukasz, On 4/17/24 13:16, lukasz.czechowski@thaumatec.com wrote: > From: Lukasz Czechowski <lukas.czechowski@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@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
BTW, On 4/17/24 13:16, lukasz.czechowski@thaumatec.com wrote: > From: Lukasz Czechowski <lukas.czechowski@thaumatec.com> > There is a typo in your mail address. I assume your git config user.email may be wrong, because your signed-off-by is correct. Cheers, Quentin
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@theobroma-systems.com> napisał(a): > > Hi Lukasz, > > On 4/17/24 13:16, lukasz.czechowski@thaumatec.com wrote: > > From: Lukasz Czechowski <lukas.czechowski@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@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
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"); return ret; error: return (-1);