diff mbox series

[2/4] ram: rockchip: Kconfig: Fix dependency of RAM_ROCKCHIP_DEBUG

Message ID 20240827142058.594556-3-lukasz.czechowski@thaumatec.com
State Superseded
Delegated to: Kever Yang
Headers show
Series Rockchip: Allow to silent TPL/SPL debug console | expand

Commit Message

Lukasz Czechowski Aug. 27, 2024, 2:20 p.m. UTC
The RAM_ROCKCHIP_DEBUG can be used only if DEBUG_UART is
available, otherwise it won't have any effect.
Add negative dependency to TPL_SILENT_CONSOLE.

Signed-off-by: Lukasz Czechowski <lukasz.czechowski@thaumatec.com>
---
 drivers/ram/rockchip/Kconfig | 2 ++
 1 file changed, 2 insertions(+)

Comments

Quentin Schulz Aug. 29, 2024, 10:19 a.m. UTC | #1
Hi Lukasz,

On 8/27/24 4:20 PM, Lukasz Czechowski wrote:
> The RAM_ROCKCHIP_DEBUG can be used only if DEBUG_UART is
> available, otherwise it won't have any effect.
> Add negative dependency to TPL_SILENT_CONSOLE.
> 
> Signed-off-by: Lukasz Czechowski <lukasz.czechowski@thaumatec.com>
> ---
>   drivers/ram/rockchip/Kconfig | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/ram/rockchip/Kconfig b/drivers/ram/rockchip/Kconfig
> index 67c63ecba04..4a6a6328676 100644
> --- a/drivers/ram/rockchip/Kconfig
> +++ b/drivers/ram/rockchip/Kconfig
> @@ -15,6 +15,8 @@ if RAM_ROCKCHIP
>   
>   config RAM_ROCKCHIP_DEBUG
>   	bool "Rockchip ram drivers debugging"
> +	depends on DEBUG_UART
> +	depends on !TPL_SILENT_CONSOLE

I am wondering if we should also do this for TPL-less systems? i.e. the 
ones for which the dram is configured in SPL.

Something like:

depends on !SPL_SILENT_CONSOLE if SPL_RAM
depends on !TPL_SILENT_CONSOLE if TPL_RAM

maybe?

Cheers,
Quentin
Lukasz Czechowski Sept. 2, 2024, 2:57 p.m. UTC | #2
Hi Quentin,

Fair point. Unfortunately the syntax "depends on <symbol> if <expr>"
seem to be not supported in Kconfig
I can do something like

depends on !TPL_SILENT_CONSOLE && TPL_RAM || !SPL_SILENT_CONSOLE && SPL_RAM

instead. What do you think?

Best regards,
Łukasz


czw., 29 sie 2024 o 12:19 Quentin Schulz <quentin.schulz@cherry.de> napisał(a):
>
> Hi Lukasz,
>
> On 8/27/24 4:20 PM, Lukasz Czechowski wrote:
> > The RAM_ROCKCHIP_DEBUG can be used only if DEBUG_UART is
> > available, otherwise it won't have any effect.
> > Add negative dependency to TPL_SILENT_CONSOLE.
> >
> > Signed-off-by: Lukasz Czechowski <lukasz.czechowski@thaumatec.com>
> > ---
> >   drivers/ram/rockchip/Kconfig | 2 ++
> >   1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/ram/rockchip/Kconfig b/drivers/ram/rockchip/Kconfig
> > index 67c63ecba04..4a6a6328676 100644
> > --- a/drivers/ram/rockchip/Kconfig
> > +++ b/drivers/ram/rockchip/Kconfig
> > @@ -15,6 +15,8 @@ if RAM_ROCKCHIP
> >
> >   config RAM_ROCKCHIP_DEBUG
> >       bool "Rockchip ram drivers debugging"
> > +     depends on DEBUG_UART
> > +     depends on !TPL_SILENT_CONSOLE
>
> I am wondering if we should also do this for TPL-less systems? i.e. the
> ones for which the dram is configured in SPL.
>
> Something like:
>
> depends on !SPL_SILENT_CONSOLE if SPL_RAM
> depends on !TPL_SILENT_CONSOLE if TPL_RAM
>
> maybe?
>
> Cheers,
> Quentin
Quentin Schulz Sept. 2, 2024, 3:27 p.m. UTC | #3
Hi Lukasz,

On 9/2/24 4:57 PM, Łukasz Czechowski wrote:
> Hi Quentin,
> 
> Fair point. Unfortunately the syntax "depends on <symbol> if <expr>"
> seem to be not supported in Kconfig

Shoot, it's the ONLY Kconfig attribute that doesn't support if :)

> I can do something like
> 
> depends on !TPL_SILENT_CONSOLE && TPL_RAM || !SPL_SILENT_CONSOLE && SPL_RAM
> 
> instead. What do you think?
> 

Mmmm no, that wouldn't work I think.

E.g. this would allow RAM_ROCKCHIP_DEBUG to be enabled in TPL even if 
TPL_SILENT_CONSOLE is enabled, if SPL_RAM=y and SPL_SILENT_CONSOLE=n.

I think we have two options:
1) depends on (!SPL_SILENT_CONSOLE && SPL_RAM) || !SPL_RAM
depends on (!TPL_SILENT_CONSOLE && TPL_RAM) || !TPL_RAM

This means RAM_ROCKCHIP_DEBUG would be selectable for RAM=y, TPL_RAM=n, 
SPL_RAM=n. I don't think anyone would want TPL_RAM AND SPL_RAM set at 
the same time, so having SPL_SILENT_CONSOLE set but not 
TPL_SILENT_CONSOLE wouldn't be an issue here.

2) Create an SPL and TPL symbol for RAM_ROCKCHIP_DEBUG, i.e. 
SPL_RAM_ROCKCHIP_DEBUG and TPL_RAM_ROCKCHIP_DEBUG.

This is a bit more involved in terms of changes but is closer to how 
U-Boot works when SPL/TPL is involved. We basically would need to change
IS_ENABLED(CONFIG_RAM_ROCKCHIP_DEBUG) into 
CONFIG_IS_ENABLED(RAM_ROCKCHIP_DEBUG) and #ifdef 
CONFIG_RAM_ROCKCHIP_DEBUG into #if CONFIG_IS_ENABLED(RAM_ROCKCHIP_DEBUG).

I assume we would want those new symbols to default to y too. If that's 
the case, then we need to manually disable those symbols in defconfigs 
that disable RAM_ROCMCHIP_DEBUG right now:
configs/anbernic-rgxx3-rk3566_defconfig
configs/neu2-io-rv1126_defconfig
configs/roc-pc-mezzanine-rk3399_defconfig
configs/roc-pc-rk3399_defconfig
configs/rock-pi-n10-rk3399pro_defconfig
configs/rock-pi-n8-rk3288_defconfig
configs/sonoff-ihost-rv1126_defconfig

Cheers,
Quentin
diff mbox series

Patch

diff --git a/drivers/ram/rockchip/Kconfig b/drivers/ram/rockchip/Kconfig
index 67c63ecba04..4a6a6328676 100644
--- a/drivers/ram/rockchip/Kconfig
+++ b/drivers/ram/rockchip/Kconfig
@@ -15,6 +15,8 @@  if RAM_ROCKCHIP
 
 config RAM_ROCKCHIP_DEBUG
 	bool "Rockchip ram drivers debugging"
+	depends on DEBUG_UART
+	depends on !TPL_SILENT_CONSOLE
 	default y
 	help
 	  This enables debugging ram driver API's for the platforms