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

[Paul Kocialkowski <paulk@sys-base.io>]

[-- Attachment #1: Type: text/plain, Size: 7748 bytes --]

Hi,

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.

> > 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.

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?

Cheers,

Paul

> > > > 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.

-- 
Paul Kocialkowski,

Independent contractor - sys-base - https://www.sys-base.io/
Free software developer - https://www.paulk.fr/

Expertise in multimedia, graphics and embedded hardware support with Linux.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]