Re: [PATCH 4/4] rockchip: Disable DRAM debug by default

[Dragan Simic <dsimic@manjaro.org>]

On 2024-09-26 23:16, Paul Kocialkowski wrote:
> Le Thu 26 Sep 24, 23:04, Dragan Simic a écrit :
>> On 2024-09-26 22:51, Paul Kocialkowski wrote:
>> > Le Thu 26 Sep 24, 22:17, Dragan Simic a écrit :
>> > > On 2024-09-26 20:31, Paul Kocialkowski wrote:
>> > > > From: Paul Kocialkowski <contact@paulk.fr>
>> > > >
>> > > > Printing debug details about DRAM is not useful in regular use and
>> > > > adds visual pollution to the log. Disable it by default.
>> > >
>> > > With all the respect, I disagree with disabling this by default.
>> > > This prints just a couple of lines that can actually be very helpful
>> > > when figuring out what's going on in case of some DRAM-related issues
>> > > on random devices in the field.
>> >
>> > Well this rationale could apply to lots of things and we generally don't
>> > print debug info about anything else by default.
>> 
>> I'd rather see these messages as some kind of additional verbosity,
>> rather than some true debugging messages for the DRAM init.  It's just
>> a couple of additional lines printed on the console, in the end.
> 
> It's mostly the fact that it's platform-specific and makes about no 
> sense
> to a non-developer user that makes me think it's not welcome by 
> default.
> It's really printing internal variables, not providing user-readable 
> information
> that can be useful outside of the scope of development.

Well, each and every U-Boot build is platform- and device-specific, so
I see no problems with some of the messages produced by default being
platform- or device-specific.

In practice, anything that U-Boot prints on the console has little to no
meaning to the vast majority of people who actually happen to see and
register those messages.  As a result, those messages are mostly useful
to the developers only.

>> > Maybe DRAM is more likely to be a source of issues than other hardware
>> > aspects
>> > that are maybe more stable, but I don't see what would prevent
>> > rebuilding a
>> > u-boot binary with debug enabled. If the DRAM config needs tweaking it
>> > will be
>> > necessary to rebuild a binary anyway.
>> 
>> In theory, rebuilding a U-Boot image and reinstalling it is rather 
>> easy.
>> In practice, it's hardly doable on random devices in the field, and 
>> it's
>> sometimes virtually impossible.  It's simply that not every end user 
>> is
>> willing or capable of doing things like that.
> 
> Well that is more or less what makes me think this is not welcome by 
> default:
> a non-developer user would not be able to do anything useful with this
> information, since they won't be able to rebuild a binary to change the 
> DRAM
> config and solve any DRAM-related issue.

A non-developer may be able to forward the messages to a developer, who
in turn may be able to use them to roughly figure out what's going on.

> Just knowing this information "on the field" won't help solve any issue 
> if
> there is no possibility to undergo development and build a binary with 
> a
> modified DRAM config.
> 
> In what scenario exactly do you think this would be valuable 
> information to
> a non-developer?

I'm actually speaking from experience.  Those messages have already
proven to be useful in a few cases that I happened to witness.

>> > > > Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
>> > > > ---
>> > > >  configs/anbernic-rgxx3-rk3566_defconfig   | 1 -
>> > > >  configs/neu2-io-rv1126_defconfig          | 1 -
>> > > >  configs/roc-pc-mezzanine-rk3399_defconfig | 1 -
>> > > >  configs/roc-pc-rk3399_defconfig           | 1 -
>> > > >  configs/rock-pi-n10-rk3399pro_defconfig   | 1 -
>> > > >  configs/rock-pi-n8-rk3288_defconfig       | 1 -
>> > > >  configs/sonoff-ihost-rv1126_defconfig     | 1 -
>> > > >  drivers/ram/rockchip/Kconfig              | 1 -
>> > > >  8 files changed, 8 deletions(-)
>> > > >
>> > > > diff --git a/configs/anbernic-rgxx3-rk3566_defconfig
>> > > > b/configs/anbernic-rgxx3-rk3566_defconfig
>> > > > index a03509bf4671..5c074cffeb44 100644
>> > > > --- a/configs/anbernic-rgxx3-rk3566_defconfig
>> > > > +++ b/configs/anbernic-rgxx3-rk3566_defconfig
>> > > > @@ -67,7 +67,6 @@ CONFIG_SPL_DM_REGULATOR_FIXED=y
>> > > >  CONFIG_REGULATOR_RK8XX=y
>> > > >  CONFIG_PWM_ROCKCHIP=y
>> > > >  CONFIG_SPL_RAM=y
>> > > > -# CONFIG_RAM_ROCKCHIP_DEBUG is not set
>> > > >  # CONFIG_RNG_SMCCC_TRNG is not set
>> > > >  CONFIG_BAUDRATE=1500000
>> > > >  CONFIG_DEBUG_UART_SHIFT=2
>> > > > diff --git a/configs/neu2-io-rv1126_defconfig
>> > > > b/configs/neu2-io-rv1126_defconfig
>> > > > index 2a4c9b45a04f..84e4465f2c5f 100644
>> > > > --- a/configs/neu2-io-rv1126_defconfig
>> > > > +++ b/configs/neu2-io-rv1126_defconfig
>> > > > @@ -45,7 +45,6 @@ CONFIG_MMC_DW=y
>> > > >  CONFIG_MMC_DW_ROCKCHIP=y
>> > > >  CONFIG_REGULATOR_PWM=y
>> > > >  CONFIG_PWM_ROCKCHIP=y
>> > > > -# CONFIG_RAM_ROCKCHIP_DEBUG is not set
>> > > >  CONFIG_BAUDRATE=1500000
>> > > >  CONFIG_DEBUG_UART_SHIFT=2
>> > > >  CONFIG_SYSRESET=y
>> > > > diff --git a/configs/roc-pc-mezzanine-rk3399_defconfig
>> > > > b/configs/roc-pc-mezzanine-rk3399_defconfig
>> > > > index a57899bfdfa0..b4041902b381 100644
>> > > > --- a/configs/roc-pc-mezzanine-rk3399_defconfig
>> > > > +++ b/configs/roc-pc-mezzanine-rk3399_defconfig
>> > > > @@ -65,7 +65,6 @@ CONFIG_REGULATOR_PWM=y
>> > > >  CONFIG_SPL_DM_REGULATOR_FIXED=y
>> > > >  CONFIG_REGULATOR_RK8XX=y
>> > > >  CONFIG_PWM_ROCKCHIP=y
>> > > > -# CONFIG_RAM_ROCKCHIP_DEBUG is not set
>> > > >  CONFIG_RAM_ROCKCHIP_LPDDR4=y
>> > > >  CONFIG_BAUDRATE=1500000
>> > > >  CONFIG_DEBUG_UART_SHIFT=2
>> > > > diff --git a/configs/roc-pc-rk3399_defconfig
>> > > > b/configs/roc-pc-rk3399_defconfig
>> > > > index b45f0e0a8994..922f67320c20 100644
>> > > > --- a/configs/roc-pc-rk3399_defconfig
>> > > > +++ b/configs/roc-pc-rk3399_defconfig
>> > > > @@ -62,7 +62,6 @@ CONFIG_REGULATOR_PWM=y
>> > > >  CONFIG_SPL_DM_REGULATOR_FIXED=y
>> > > >  CONFIG_REGULATOR_RK8XX=y
>> > > >  CONFIG_PWM_ROCKCHIP=y
>> > > > -# CONFIG_RAM_ROCKCHIP_DEBUG is not set
>> > > >  CONFIG_RAM_ROCKCHIP_LPDDR4=y
>> > > >  CONFIG_BAUDRATE=1500000
>> > > >  CONFIG_DEBUG_UART_SHIFT=2
>> > > > diff --git a/configs/rock-pi-n10-rk3399pro_defconfig
>> > > > b/configs/rock-pi-n10-rk3399pro_defconfig
>> > > > index ec995a54a0ee..17fe939ec989 100644
>> > > > --- a/configs/rock-pi-n10-rk3399pro_defconfig
>> > > > +++ b/configs/rock-pi-n10-rk3399pro_defconfig
>> > > > @@ -51,7 +51,6 @@ CONFIG_PHY_ROCKCHIP_TYPEC=y
>> > > >  CONFIG_PMIC_RK8XX=y
>> > > >  CONFIG_REGULATOR_RK8XX=y
>> > > >  CONFIG_PWM_ROCKCHIP=y
>> > > > -# CONFIG_RAM_ROCKCHIP_DEBUG is not set
>> > > >  CONFIG_BAUDRATE=1500000
>> > > >  CONFIG_DEBUG_UART_SHIFT=2
>> > > >  CONFIG_SYS_NS16550_MEM32=y
>> > > > diff --git a/configs/rock-pi-n8-rk3288_defconfig
>> > > > b/configs/rock-pi-n8-rk3288_defconfig
>> > > > index 4c09b9137ef8..af0fa8879421 100644
>> > > > --- a/configs/rock-pi-n8-rk3288_defconfig
>> > > > +++ b/configs/rock-pi-n8-rk3288_defconfig
>> > > > @@ -73,7 +73,6 @@ CONFIG_REGULATOR_RK8XX=y
>> > > >  CONFIG_PWM_ROCKCHIP=y
>> > > >  CONFIG_RAM=y
>> > > >  CONFIG_SPL_RAM=y
>> > > > -# CONFIG_RAM_ROCKCHIP_DEBUG is not set
>> > > >  CONFIG_DEBUG_UART_SHIFT=2
>> > > >  CONFIG_SYS_NS16550_MEM32=y
>> > > >  CONFIG_SYSRESET=y
>> > > > diff --git a/configs/sonoff-ihost-rv1126_defconfig
>> > > > b/configs/sonoff-ihost-rv1126_defconfig
>> > > > index 4890644c7e6f..739adb49ce93 100644
>> > > > --- a/configs/sonoff-ihost-rv1126_defconfig
>> > > > +++ b/configs/sonoff-ihost-rv1126_defconfig
>> > > > @@ -46,7 +46,6 @@ CONFIG_MMC_DW=y
>> > > >  CONFIG_MMC_DW_ROCKCHIP=y
>> > > >  CONFIG_REGULATOR_PWM=y
>> > > >  CONFIG_PWM_ROCKCHIP=y
>> > > > -# CONFIG_RAM_ROCKCHIP_DEBUG is not set
>> > > >  CONFIG_BAUDRATE=1500000
>> > > >  CONFIG_DEBUG_UART_SHIFT=2
>> > > >  CONFIG_SYSRESET=y
>> > > > diff --git a/drivers/ram/rockchip/Kconfig b/drivers/ram/rockchip/Kconfig
>> > > > index 67c63ecba047..e030c982eccb 100644
>> > > > --- a/drivers/ram/rockchip/Kconfig
>> > > > +++ b/drivers/ram/rockchip/Kconfig
>> > > > @@ -15,7 +15,6 @@ if RAM_ROCKCHIP
>> > > >
>> > > >  config RAM_ROCKCHIP_DEBUG
>> > > >  	bool "Rockchip ram drivers debugging"
>> > > > -	default y
>> > > >  	help
>> > > >  	  This enables debugging ram driver API's for the platforms
>> > > >  	  based on Rockchip SoCs.