[PATCH 1/4] rockchip: rk3399-roc-pc: Hook sysreset gpio to enable full reset

[PATCH 1/4] rockchip: rk3399-roc-pc: Hook sysreset gpio to enable full reset

[Paul Kocialkowski]

From: Paul Kocialkowski <contact@paulk.fr>

The reset mechanism used by Linux to reset the SoC is known to only
partially reset the logic. A mechanism is implemented in
rk3399_force_power_on_reset to use a GPIO connected to the PMIC's
over-temperature (OTP) reset pin, which fully resets all logic.

Hook the associated GPIO where the function expects it to enable this
reset mechanism and avoid any possible side-effect of partially-reset
units.

Without this patch, reading from the micro sd slot fails after a reset.
With this mechanism, U-Boot is able to boot from it reliably.

Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
---
 arch/arm/dts/rk3399-roc-pc-u-boot.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/dts/rk3399-roc-pc-u-boot.dtsi b/arch/arm/dts/rk3399-roc-pc-u-boot.dtsi
index aecf7dbe383c..883d399a06a3 100644
--- a/arch/arm/dts/rk3399-roc-pc-u-boot.dtsi
+++ b/arch/arm/dts/rk3399-roc-pc-u-boot.dtsi
@@ -7,6 +7,10 @@
 #include "rk3399-sdram-lpddr4-100.dtsi"
 
 / {
+	config {
+		sysreset-gpio = <&gpio1 RK_PA6 GPIO_ACTIVE_HIGH>;
+	};
+
 	vcc_hub_en: vcc_hub_en-regulator {
 		compatible = "regulator-fixed";
 		enable-active-high;
@@ -36,6 +40,10 @@
 	bootph-pre-ram;
 };
 
+&gpio1 {
+	bootph-pre-ram;
+};
+
 &spi1 {
 	flash@0 {
 		bootph-pre-ram;
-- 
2.46.2

[PATCH 2/4] rockchip: rk3399-rockpro64: Hook sysreset gpio to enable full reset

[Paul Kocialkowski]

From: Paul Kocialkowski <contact@paulk.fr>

The reset mechanism used by Linux to reset the SoC is known to only
partially reset the logic. A mechanism is implemented in
rk3399_force_power_on_reset to use a GPIO connected to the PMIC's
over-temperature (OTP) reset pin, which fully resets all logic.

Hook the associated GPIO where the function expects it to enable this
reset mechanism and avoid any possible side-effect of partially-reset
units.

Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
---
 arch/arm/dts/rk3399-rockpro64-u-boot.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/dts/rk3399-rockpro64-u-boot.dtsi b/arch/arm/dts/rk3399-rockpro64-u-boot.dtsi
index 43b67991fe5a..cd84269dab48 100644
--- a/arch/arm/dts/rk3399-rockpro64-u-boot.dtsi
+++ b/arch/arm/dts/rk3399-rockpro64-u-boot.dtsi
@@ -7,6 +7,10 @@
 #include "rk3399-sdram-lpddr4-100.dtsi"
 
 / {
+	config {
+		sysreset-gpio = <&gpio1 RK_PA6 GPIO_ACTIVE_HIGH>;
+	};
+
         smbios {
                 compatible = "u-boot,sysinfo-smbios";
                 smbios {
@@ -32,6 +36,10 @@
 	bootph-pre-ram;
 };
 
+&gpio1 {
+	bootph-pre-ram;
+};
+
 &sdhci {
 	cap-mmc-highspeed;
 	mmc-ddr-1_8v;
-- 
2.46.2

Re: [PATCH 2/4] rockchip: rk3399-rockpro64: Hook sysreset gpio to enable full reset

[Quentin Schulz]

Hi Paul,

On 9/26/24 8:31 PM, Paul Kocialkowski wrote:
> From: Paul Kocialkowski <contact@paulk.fr>
> 
> The reset mechanism used by Linux to reset the SoC is known to only
> partially reset the logic. A mechanism is implemented in
> rk3399_force_power_on_reset to use a GPIO connected to the PMIC's
> over-temperature (OTP) reset pin, which fully resets all logic.
> 
> Hook the associated GPIO where the function expects it to enable this
> reset mechanism and avoid any possible side-effect of partially-reset
> units.
> 
> Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
> ---
>   arch/arm/dts/rk3399-rockpro64-u-boot.dtsi | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/arch/arm/dts/rk3399-rockpro64-u-boot.dtsi b/arch/arm/dts/rk3399-rockpro64-u-boot.dtsi
> index 43b67991fe5a..cd84269dab48 100644
> --- a/arch/arm/dts/rk3399-rockpro64-u-boot.dtsi
> +++ b/arch/arm/dts/rk3399-rockpro64-u-boot.dtsi
> @@ -7,6 +7,10 @@
>   #include "rk3399-sdram-lpddr4-100.dtsi"
>   
>   / {
> +	config {
> +		sysreset-gpio = <&gpio1 RK_PA6 GPIO_ACTIVE_HIGH>;
> +	};
> +

We've been using this on RK3399 Puma for a while already, and a similar 
routing can be observed on both boards, therefore:

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>

Thanks!
Quentin

Re: [PATCH 2/4] rockchip: rk3399-rockpro64: Hook sysreset gpio to enable full reset

[Quentin Schulz]

Hi Paul,

On 9/26/24 8:31 PM, Paul Kocialkowski wrote:
> From: Paul Kocialkowski <contact@paulk.fr>
> 
> The reset mechanism used by Linux to reset the SoC is known to only
> partially reset the logic. A mechanism is implemented in
> rk3399_force_power_on_reset to use a GPIO connected to the PMIC's
> over-temperature (OTP) reset pin, which fully resets all logic.
> 
> Hook the associated GPIO where the function expects it to enable this
> reset mechanism and avoid any possible side-effect of partially-reset
> units.
> 
> Signed-off-by: Paul Kocialkowski <contact@paulk.fr>

Since patches 3 and 4 seems to be a bit controversial, maybe send 
patches 1 and 2 in a different patch series so those can be merged 
separately?

You may want to have a look at 
https://lore.kernel.org/u-boot/20241105-rk3399-sysreset-gpio-tpl-v1-0-12caff07a4e4@cherry.de/T/#t 
too BTW.

Cheers,
Quentin

Re: [PATCH 2/4] rockchip: rk3399-rockpro64: Hook sysreset gpio to enable full reset

[Paul Kocialkowski]

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

Hi,

Le Tue 05 Nov 24, 16:38, Quentin Schulz a écrit :
> Hi Paul,
> 
> On 9/26/24 8:31 PM, Paul Kocialkowski wrote:
> > From: Paul Kocialkowski <contact@paulk.fr>
> > 
> > The reset mechanism used by Linux to reset the SoC is known to only
> > partially reset the logic. A mechanism is implemented in
> > rk3399_force_power_on_reset to use a GPIO connected to the PMIC's
> > over-temperature (OTP) reset pin, which fully resets all logic.
> > 
> > Hook the associated GPIO where the function expects it to enable this
> > reset mechanism and avoid any possible side-effect of partially-reset
> > units.
> > 
> > Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
> 
> Since patches 3 and 4 seems to be a bit controversial, maybe send patches 1
> and 2 in a different patch series so those can be merged separately?
> 
> You may want to have a look at https://lore.kernel.org/u-boot/20241105-rk3399-sysreset-gpio-tpl-v1-0-12caff07a4e4@cherry.de/T/#t

Sounds like a good idea. I should probably wait for your series to get-in
and respin a version of just the sysreset patches with the right bootph flags.

Cheers,

Paul

-- 
Paul Kocialkowski,

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

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

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

[PATCH 3/4] rockchip: rk3399-rockpro64: Disable bootstage instrumentation config

[Paul Kocialkowski]

From: Paul Kocialkowski <contact@paulk.fr>

The boot timing and reporting (bootstage) infrastructure is useful for
performance analysis and debug but adds overhead and console noise when
using the device normally. Remove it from the device config.

Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
---
 configs/rockpro64-rk3399_defconfig | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/configs/rockpro64-rk3399_defconfig b/configs/rockpro64-rk3399_defconfig
index fc0804a0b80d..095d27ffaa04 100644
--- a/configs/rockpro64-rk3399_defconfig
+++ b/configs/rockpro64-rk3399_defconfig
@@ -19,8 +19,6 @@ CONFIG_SPL_SPI=y
 CONFIG_SYS_LOAD_ADDR=0x800800
 CONFIG_PCI=y
 CONFIG_DEBUG_UART=y
-CONFIG_BOOTSTAGE=y
-CONFIG_BOOTSTAGE_REPORT=y
 CONFIG_DEFAULT_FDT_FILE="rockchip/rk3399-rockpro64.dtb"
 CONFIG_DISPLAY_BOARDINFO_LATE=y
 CONFIG_SPL_MAX_SIZE=0x40000
@@ -38,7 +36,6 @@ CONFIG_CMD_POWEROFF=y
 CONFIG_CMD_USB=y
 # CONFIG_CMD_SETEXPR is not set
 CONFIG_CMD_TIME=y
-CONFIG_CMD_BOOTSTAGE=y
 CONFIG_SPL_OF_CONTROL=y
 CONFIG_OF_SPL_REMOVE_PROPS="clock-names interrupt-parent assigned-clocks assigned-clock-rates assigned-clock-parents"
 CONFIG_ENV_IS_IN_SPI_FLASH=y
-- 
2.46.2

Re: [PATCH 3/4] rockchip: rk3399-rockpro64: Disable bootstage instrumentation config

[Peter Robinson]

On Thu, 26 Sept 2024 at 19:32, Paul Kocialkowski <paulk@sys-base.io> wrote:
>
> From: Paul Kocialkowski <contact@paulk.fr>
>
> The boot timing and reporting (bootstage) infrastructure is useful for
> performance analysis and debug but adds overhead and console noise when
> using the device normally. Remove it from the device config.
>
> Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
Reviewed-by: Peter Robinson <pbrobinson@gmail.com>

This makes sense, for those that want this information will know how
to build it for testing.
> ---
>  configs/rockpro64-rk3399_defconfig | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/configs/rockpro64-rk3399_defconfig b/configs/rockpro64-rk3399_defconfig
> index fc0804a0b80d..095d27ffaa04 100644
> --- a/configs/rockpro64-rk3399_defconfig
> +++ b/configs/rockpro64-rk3399_defconfig
> @@ -19,8 +19,6 @@ CONFIG_SPL_SPI=y
>  CONFIG_SYS_LOAD_ADDR=0x800800
>  CONFIG_PCI=y
>  CONFIG_DEBUG_UART=y
> -CONFIG_BOOTSTAGE=y
> -CONFIG_BOOTSTAGE_REPORT=y
>  CONFIG_DEFAULT_FDT_FILE="rockchip/rk3399-rockpro64.dtb"
>  CONFIG_DISPLAY_BOARDINFO_LATE=y
>  CONFIG_SPL_MAX_SIZE=0x40000
> @@ -38,7 +36,6 @@ CONFIG_CMD_POWEROFF=y
>  CONFIG_CMD_USB=y
>  # CONFIG_CMD_SETEXPR is not set
>  CONFIG_CMD_TIME=y
> -CONFIG_CMD_BOOTSTAGE=y
>  CONFIG_SPL_OF_CONTROL=y
>  CONFIG_OF_SPL_REMOVE_PROPS="clock-names interrupt-parent assigned-clocks assigned-clock-rates assigned-clock-parents"
>  CONFIG_ENV_IS_IN_SPI_FLASH=y
> --
> 2.46.2
>

Re: [PATCH 3/4] rockchip: rk3399-rockpro64: Disable bootstage instrumentation config

[Simon Glass]

Hi,

On Mon, 30 Sept 2024 at 03:03, Peter Robinson <pbrobinson@gmail.com> wrote:
>
> On Thu, 26 Sept 2024 at 19:32, Paul Kocialkowski <paulk@sys-base.io> wrote:
> >
> > From: Paul Kocialkowski <contact@paulk.fr>
> >
> > The boot timing and reporting (bootstage) infrastructure is useful for
> > performance analysis and debug but adds overhead and console noise when
> > using the device normally. Remove it from the device config.
> >
> > Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
> Reviewed-by: Peter Robinson <pbrobinson@gmail.com>
>
> This makes sense, for those that want this information will know how
> to build it for testing.

Can you just disable the report? There should be no need to disable
bootstage itself.

Regards,
Simon


> > ---
> >  configs/rockpro64-rk3399_defconfig | 3 ---
> >  1 file changed, 3 deletions(-)
> >
> > diff --git a/configs/rockpro64-rk3399_defconfig b/configs/rockpro64-rk3399_defconfig
> > index fc0804a0b80d..095d27ffaa04 100644
> > --- a/configs/rockpro64-rk3399_defconfig
> > +++ b/configs/rockpro64-rk3399_defconfig
> > @@ -19,8 +19,6 @@ CONFIG_SPL_SPI=y
> >  CONFIG_SYS_LOAD_ADDR=0x800800
> >  CONFIG_PCI=y
> >  CONFIG_DEBUG_UART=y
> > -CONFIG_BOOTSTAGE=y
> > -CONFIG_BOOTSTAGE_REPORT=y
> >  CONFIG_DEFAULT_FDT_FILE="rockchip/rk3399-rockpro64.dtb"
> >  CONFIG_DISPLAY_BOARDINFO_LATE=y
> >  CONFIG_SPL_MAX_SIZE=0x40000
> > @@ -38,7 +36,6 @@ CONFIG_CMD_POWEROFF=y
> >  CONFIG_CMD_USB=y
> >  # CONFIG_CMD_SETEXPR is not set
> >  CONFIG_CMD_TIME=y
> > -CONFIG_CMD_BOOTSTAGE=y
> >  CONFIG_SPL_OF_CONTROL=y
> >  CONFIG_OF_SPL_REMOVE_PROPS="clock-names interrupt-parent assigned-clocks assigned-clock-rates assigned-clock-parents"
> >  CONFIG_ENV_IS_IN_SPI_FLASH=y
> > --
> > 2.46.2
> >

Re: [PATCH 3/4] rockchip: rk3399-rockpro64: Disable bootstage instrumentation config

[Paul Kocialkowski]

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

Hi,

Le Mon 30 Sep 24, 12:52, Simon Glass a écrit :
> On Mon, 30 Sept 2024 at 03:03, Peter Robinson <pbrobinson@gmail.com> wrote:
> > On Thu, 26 Sept 2024 at 19:32, Paul Kocialkowski <paulk@sys-base.io> wrote:
> > > The boot timing and reporting (bootstage) infrastructure is useful for
> > > performance analysis and debug but adds overhead and console noise when
> > > using the device normally. Remove it from the device config.
> > >
> > > Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
> > Reviewed-by: Peter Robinson <pbrobinson@gmail.com>
> >
> > This makes sense, for those that want this information will know how
> > to build it for testing.
> 
> Can you just disable the report? There should be no need to disable
> bootstage itself.

I see bootstage as a debug/development feature, so I don't really see why it
should be enabled on default builds.

Besides the console noise aspect, my intent here is also to tidy up this config.

Cheers,

Paul

> Regards,
> Simon
> 
> 
> > > ---
> > >  configs/rockpro64-rk3399_defconfig | 3 ---
> > >  1 file changed, 3 deletions(-)
> > >
> > > diff --git a/configs/rockpro64-rk3399_defconfig b/configs/rockpro64-rk3399_defconfig
> > > index fc0804a0b80d..095d27ffaa04 100644
> > > --- a/configs/rockpro64-rk3399_defconfig
> > > +++ b/configs/rockpro64-rk3399_defconfig
> > > @@ -19,8 +19,6 @@ CONFIG_SPL_SPI=y
> > >  CONFIG_SYS_LOAD_ADDR=0x800800
> > >  CONFIG_PCI=y
> > >  CONFIG_DEBUG_UART=y
> > > -CONFIG_BOOTSTAGE=y
> > > -CONFIG_BOOTSTAGE_REPORT=y
> > >  CONFIG_DEFAULT_FDT_FILE="rockchip/rk3399-rockpro64.dtb"
> > >  CONFIG_DISPLAY_BOARDINFO_LATE=y
> > >  CONFIG_SPL_MAX_SIZE=0x40000
> > > @@ -38,7 +36,6 @@ CONFIG_CMD_POWEROFF=y
> > >  CONFIG_CMD_USB=y
> > >  # CONFIG_CMD_SETEXPR is not set
> > >  CONFIG_CMD_TIME=y
> > > -CONFIG_CMD_BOOTSTAGE=y
> > >  CONFIG_SPL_OF_CONTROL=y
> > >  CONFIG_OF_SPL_REMOVE_PROPS="clock-names interrupt-parent assigned-clocks assigned-clock-rates assigned-clock-parents"
> > >  CONFIG_ENV_IS_IN_SPI_FLASH=y
> > > --
> > > 2.46.2
> > >

-- 
Paul Kocialkowski,

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

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

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

Re: [PATCH 3/4] rockchip: rk3399-rockpro64: Disable bootstage instrumentation config

[Simon Glass]

Hi Paul,

On Mon, 30 Sept 2024 at 13:07, Paul Kocialkowski <paulk@sys-base.io> wrote:
>
> Hi,
>
> Le Mon 30 Sep 24, 12:52, Simon Glass a écrit :
> > On Mon, 30 Sept 2024 at 03:03, Peter Robinson <pbrobinson@gmail.com> wrote:
> > > On Thu, 26 Sept 2024 at 19:32, Paul Kocialkowski <paulk@sys-base.io> wrote:
> > > > The boot timing and reporting (bootstage) infrastructure is useful for
> > > > performance analysis and debug but adds overhead and console noise when
> > > > using the device normally. Remove it from the device config.
> > > >
> > > > Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
> > > Reviewed-by: Peter Robinson <pbrobinson@gmail.com>
> > >
> > > This makes sense, for those that want this information will know how
> > > to build it for testing.
> >
> > Can you just disable the report? There should be no need to disable
> > bootstage itself.
>
> I see bootstage as a debug/development feature, so I don't really see why it
> should be enabled on default builds.
>
> Besides the console noise aspect, my intent here is also to tidy up this config.

Well OK.

BTW one day I'd like to see bootstage be on by default, since it
provides a way to monitor boot times for devices. But that would need
some improvements: the current shared-memory approach between SPL and
U-Boot is not great and we could use bloblist instead. Also I'm not
sure how the information could be sent to the OS.

Reviewed-by: Simon Glass <sjg@chromium.org>

Regards,
Simon


>
> Cheers,
>
> Paul
>
> > Regards,
> > Simon
> >
> >
> > > > ---
> > > >  configs/rockpro64-rk3399_defconfig | 3 ---
> > > >  1 file changed, 3 deletions(-)
> > > >
> > > > diff --git a/configs/rockpro64-rk3399_defconfig b/configs/rockpro64-rk3399_defconfig
> > > > index fc0804a0b80d..095d27ffaa04 100644
> > > > --- a/configs/rockpro64-rk3399_defconfig
> > > > +++ b/configs/rockpro64-rk3399_defconfig
> > > > @@ -19,8 +19,6 @@ CONFIG_SPL_SPI=y
> > > >  CONFIG_SYS_LOAD_ADDR=0x800800
> > > >  CONFIG_PCI=y
> > > >  CONFIG_DEBUG_UART=y
> > > > -CONFIG_BOOTSTAGE=y
> > > > -CONFIG_BOOTSTAGE_REPORT=y
> > > >  CONFIG_DEFAULT_FDT_FILE="rockchip/rk3399-rockpro64.dtb"
> > > >  CONFIG_DISPLAY_BOARDINFO_LATE=y
> > > >  CONFIG_SPL_MAX_SIZE=0x40000
> > > > @@ -38,7 +36,6 @@ CONFIG_CMD_POWEROFF=y
> > > >  CONFIG_CMD_USB=y
> > > >  # CONFIG_CMD_SETEXPR is not set
> > > >  CONFIG_CMD_TIME=y
> > > > -CONFIG_CMD_BOOTSTAGE=y
> > > >  CONFIG_SPL_OF_CONTROL=y
> > > >  CONFIG_OF_SPL_REMOVE_PROPS="clock-names interrupt-parent assigned-clocks assigned-clock-rates assigned-clock-parents"
> > > >  CONFIG_ENV_IS_IN_SPI_FLASH=y
> > > > --
> > > > 2.46.2
> > > >
>
> --
> Paul Kocialkowski,
>
> Independent contractor - sys-base - https://www.sys-base.io/
> Free software developer - https://www.paulk.fr/
>
> Specialist in multimedia, graphics and embedded hardware support with Linux.

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

[Paul Kocialkowski]

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.

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

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

[Dragan Simic]

Hello Paul,

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.

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

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

[Paul Kocialkowski]

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

Hi,

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.

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.

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

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

[Dragan Simic]

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.

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

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

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

[Paul Kocialkowski]

[-- 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 --]

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

[Dragan Simic]

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.

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

[Paul Kocialkowski]

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

On Thu 26 Sep 24, 23:24, Dragan Simic wrote:
> 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.

I like the idea that U-Boot behaves more or less the same regardless of the
platform and that there is some consistency in what is shown on the console.
At least that's how I understand the "universal" aspect of it.

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

I disagree. Most of what is shown is understandable for technical users and
provides informative value. I'm obviously not talking about users that are not
interested in understand how their technology works here, for whom nothing shown
on the console will have any value.

There is a fundamental difference between printing status information and
printing debug information. I think this clearly falls under debug information
(and the name of the config option agrees).

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

I don't see how it is useful to have a deeper understanding of some DRAM-related
issue if nothing can be done about it.

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

I'd be curious to see cases where this information helped resolve any actual
issue. Honestly I'm not very convinced and haven't changed my mind.

Of course it will be up to maintainers to decide whether this change is good
or not. I can still live with the debug info on by default, I've just been
annoyed by it for a while and thought most people would better like it gone.

Thanks for sharing your thoughts!

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/

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

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

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

[Dragan Simic]

On 2024-09-26 23:39, Paul Kocialkowski wrote:
> On Thu 26 Sep 24, 23:24, Dragan Simic wrote:
>> 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.
> 
> I like the idea that U-Boot behaves more or less the same regardless of 
> the
> platform and that there is some consistency in what is shown on the 
> console.
> At least that's how I understand the "universal" aspect of it.

Well, have a look at TF-A for Allwinner A64, for example, and what it
ends up producing in the U-Boot console.  That's highly non-universal,
not just what gets printed out, but also what the TF-A for A64 ends
up doing, such as setting up some voltage regulators.

>> 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.
> 
> I disagree. Most of what is shown is understandable for technical users 
> and
> provides informative value. I'm obviously not talking about users that 
> are not
> interested in understand how their technology works here, for whom 
> nothing shown
> on the console will have any value.
> 
> There is a fundamental difference between printing status information 
> and
> printing debug information. I think this clearly falls under debug 
> information
> (and the name of the config option agrees).

The config option could be named CONFIG_RAM_ROCKCHIP_VERBOSE instead,
would that change anything?

However, I don't see us agreeing on this, so I think it's best to agree
to disagree and wait for additional opinions from other people.

>> > > > 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.
> 
> I don't see how it is useful to have a deeper understanding of some 
> DRAM-related
> issue if nothing can be done about it.
> 
>> > 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.
> 
> I'd be curious to see cases where this information helped resolve any 
> actual
> issue. Honestly I'm not very convinced and haven't changed my mind.
> 
> Of course it will be up to maintainers to decide whether this change is 
> good
> or not. I can still live with the debug info on by default, I've just 
> been
> annoyed by it for a while and thought most people would better like it 
> gone.
> 
> Thanks for sharing your thoughts!
> 
> 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.

Re: [PATCH 1/4] rockchip: rk3399-roc-pc: Hook sysreset gpio to enable full reset

[Quentin Schulz]

Hi Paul,

I'm not entirely sure on whose side the issue is, but I didn't receive 
your mails, either from the U-Boot mailing list or directly from the Cc 
field. I however could find the patch on lore.kernel.org... and I also 
received yours and Dragan's exchange on patch 4 (but not the patch 
itself). Any chance you received something from my mail server? Does 
anyone in Cc of this mail actually received the mail?

On 9/26/24 8:31 PM, Paul Kocialkowski wrote:
> From: Paul Kocialkowski <contact@paulk.fr>
> 
> The reset mechanism used by Linux to reset the SoC is known to only
> partially reset the logic. A mechanism is implemented in
> rk3399_force_power_on_reset to use a GPIO connected to the PMIC's
> over-temperature (OTP) reset pin, which fully resets all logic.
> 
> Hook the associated GPIO where the function expects it to enable this
> reset mechanism and avoid any possible side-effect of partially-reset
> units.
> 
> Without this patch, reading from the micro sd slot fails after a reset.
> With this mechanism, U-Boot is able to boot from it reliably.
> 
> Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
> ---
>   arch/arm/dts/rk3399-roc-pc-u-boot.dtsi | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/arch/arm/dts/rk3399-roc-pc-u-boot.dtsi b/arch/arm/dts/rk3399-roc-pc-u-boot.dtsi
> index aecf7dbe383c..883d399a06a3 100644
> --- a/arch/arm/dts/rk3399-roc-pc-u-boot.dtsi
> +++ b/arch/arm/dts/rk3399-roc-pc-u-boot.dtsi
> @@ -7,6 +7,10 @@
>   #include "rk3399-sdram-lpddr4-100.dtsi"
>   
>   / {
> +	config {
> +		sysreset-gpio = <&gpio1 RK_PA6 GPIO_ACTIVE_HIGH>;

I think this is the wrong pin to use.

The routing of GPIO1_A6 is similar on RK3399 Puma and Pine64 RockPro64, 
but it differs massively for the Firefly Roc PC.

However, a similar routing is done for GPIO1_A5 on the Firefly, I 
believe that one is more appropriate. What do you think?

Cheers,
Quentin

Re: [PATCH 1/4] rockchip: rk3399-roc-pc: Hook sysreset gpio to enable full reset

[Paul Kocialkowski]

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

Hi Quentin,

Thanks for looking into this!

Le Fri 27 Sep 24, 11:25, Quentin Schulz a écrit :
> I'm not entirely sure on whose side the issue is, but I didn't receive your
> mails, either from the U-Boot mailing list or directly from the Cc field. I
> however could find the patch on lore.kernel.org... and I also received yours
> and Dragan's exchange on patch 4 (but not the patch itself). Any chance you
> received something from my mail server? Does anyone in Cc of this mail
> actually received the mail?

No automatic reply from your mail server and the logs look good on my side,
with a 250 result on all sent patches. Strange indeed...

> > diff --git a/arch/arm/dts/rk3399-roc-pc-u-boot.dtsi b/arch/arm/dts/rk3399-roc-pc-u-boot.dtsi
> > index aecf7dbe383c..883d399a06a3 100644
> > --- a/arch/arm/dts/rk3399-roc-pc-u-boot.dtsi
> > +++ b/arch/arm/dts/rk3399-roc-pc-u-boot.dtsi
> > @@ -7,6 +7,10 @@
> >   #include "rk3399-sdram-lpddr4-100.dtsi"
> >   / {
> > +	config {
> > +		sysreset-gpio = <&gpio1 RK_PA6 GPIO_ACTIVE_HIGH>;
> 
> I think this is the wrong pin to use.
> 
> The routing of GPIO1_A6 is similar on RK3399 Puma and Pine64 RockPro64, but
> it differs massively for the Firefly Roc PC.
> 
> However, a similar routing is done for GPIO1_A5 on the Firefly, I believe
> that one is more appropriate. What do you think?

I just double-checked the schematics (ROC_3399_PC), looking at signal OTP_OUT_H
which is definitely connected to GPIO1_A6 (P26).

Also it clearly resets the board when toggled and solves the MMC reset issue
I was having on this exact board, so I'm rather confident that it's the right
one to use :)

Cheers,

Paul

-- 
Paul Kocialkowski,

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

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

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

Re: [PATCH 1/4] rockchip: rk3399-roc-pc: Hook sysreset gpio to enable full reset

[Quentin Schulz]

Hi Paul,

On 9/27/24 11:53 AM, Paul Kocialkowski wrote:
> Hi Quentin,
> 
> Thanks for looking into this!
> 
> Le Fri 27 Sep 24, 11:25, Quentin Schulz a écrit :
>> I'm not entirely sure on whose side the issue is, but I didn't receive your
>> mails, either from the U-Boot mailing list or directly from the Cc field. I
>> however could find the patch on lore.kernel.org... and I also received yours
>> and Dragan's exchange on patch 4 (but not the patch itself). Any chance you
>> received something from my mail server? Does anyone in Cc of this mail
>> actually received the mail?
> 
> No automatic reply from your mail server and the logs look good on my side,
> with a 250 result on all sent patches. Strange indeed...
> 
>>> diff --git a/arch/arm/dts/rk3399-roc-pc-u-boot.dtsi b/arch/arm/dts/rk3399-roc-pc-u-boot.dtsi
>>> index aecf7dbe383c..883d399a06a3 100644
>>> --- a/arch/arm/dts/rk3399-roc-pc-u-boot.dtsi
>>> +++ b/arch/arm/dts/rk3399-roc-pc-u-boot.dtsi
>>> @@ -7,6 +7,10 @@
>>>    #include "rk3399-sdram-lpddr4-100.dtsi"
>>>    / {
>>> +	config {
>>> +		sysreset-gpio = <&gpio1 RK_PA6 GPIO_ACTIVE_HIGH>;
>>
>> I think this is the wrong pin to use.
>>
>> The routing of GPIO1_A6 is similar on RK3399 Puma and Pine64 RockPro64, but
>> it differs massively for the Firefly Roc PC.
>>
>> However, a similar routing is done for GPIO1_A5 on the Firefly, I believe
>> that one is more appropriate. What do you think?
> 
> I just double-checked the schematics (ROC_3399_PC), looking at signal OTP_OUT_H
> which is definitely connected to GPIO1_A6 (P26).
> 
> Also it clearly resets the board when toggled and solves the MMC reset issue
> I was having on this exact board, so I'm rather confident that it's the right
> one to use :)
> 

At least we're on the same page for using OTP_OUT_H, but it's routed to 
GPIO1_A5 on the schematics I found:

https://www.t-firefly.com/download/Firefly-RK3399/hardware/Firefly-RK3399_V10/Firefly-RK3399_V10_SCH_(2017-2-8).pdf

Mmmmmm seems like I was looking at the wrong schematics?
https://en.t-firefly.com/doc/download/page/id/51.html does route 
GPIO1_A6 to the OTP_OUT_H signal..
https://en.t-firefly.com/doc/download/page/id/78.html
https://en.t-firefly.com/doc/download/page/id/127.html

So, the Roc PC, Roc PC Plus and Roc PC Pro all seem to have a similar 
routing for this pin, therefore:

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>

Thanks!
Quentin

Re: [PATCH 1/4] rockchip: rk3399-roc-pc: Hook sysreset gpio to enable full reset

[Paul Kocialkowski]

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

Le Fri 27 Sep 24, 12:07, Quentin Schulz a écrit :
> > > > diff --git a/arch/arm/dts/rk3399-roc-pc-u-boot.dtsi b/arch/arm/dts/rk3399-roc-pc-u-boot.dtsi
> > > > index aecf7dbe383c..883d399a06a3 100644
> > > > --- a/arch/arm/dts/rk3399-roc-pc-u-boot.dtsi
> > > > +++ b/arch/arm/dts/rk3399-roc-pc-u-boot.dtsi
> > > > @@ -7,6 +7,10 @@
> > > >    #include "rk3399-sdram-lpddr4-100.dtsi"
> > > >    / {
> > > > +	config {
> > > > +		sysreset-gpio = <&gpio1 RK_PA6 GPIO_ACTIVE_HIGH>;
> > > 
> > > I think this is the wrong pin to use.
> > > 
> > > The routing of GPIO1_A6 is similar on RK3399 Puma and Pine64 RockPro64, but
> > > it differs massively for the Firefly Roc PC.
> > > 
> > > However, a similar routing is done for GPIO1_A5 on the Firefly, I believe
> > > that one is more appropriate. What do you think?
> > 
> > I just double-checked the schematics (ROC_3399_PC), looking at signal OTP_OUT_H
> > which is definitely connected to GPIO1_A6 (P26).
> > 
> > Also it clearly resets the board when toggled and solves the MMC reset issue
> > I was having on this exact board, so I'm rather confident that it's the right
> > one to use :)
> 
> At least we're on the same page for using OTP_OUT_H, but it's routed to
> GPIO1_A5 on the schematics I found:
> 
> https://www.t-firefly.com/download/Firefly-RK3399/hardware/Firefly-RK3399_V10/Firefly-RK3399_V10_SCH_(2017-2-8).pdf
> 
> Mmmmmm seems like I was looking at the wrong schematics?

Ah yes I think the Firefly-RK3399 and ROC-RK3399-PC are two distinct designs.

> https://en.t-firefly.com/doc/download/page/id/51.html does route GPIO1_A6 to
> the OTP_OUT_H signal..
> https://en.t-firefly.com/doc/download/page/id/78.html
> https://en.t-firefly.com/doc/download/page/id/127.html
> 
> So, the Roc PC, Roc PC Plus and Roc PC Pro all seem to have a similar
> routing for this pin, therefore:
> 
> Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>

Right, and for the record the version I have is the ROC-RK3399-PC Plus.

Thanks!

-- 
Paul Kocialkowski,

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

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

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