Re: [PATCH v3 1/3] Convert CONFIG_SYS_L2_PL310 to Kconfig

[Tom Rini <trini@konsulko.com>]

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

On Tue, Aug 09, 2022 at 01:32:24PM +0200, Pali Rohár wrote:
> On Tuesday 09 August 2022 13:27:38 Marek Vasut wrote:
> > On 8/9/22 13:21, Pali Rohár wrote:
> > 
> > (reducing the CC list)
> > 
> > > On Tuesday 09 August 2022 13:16:41 Marek Vasut wrote:
> > > > On 8/9/22 12:58, Pali Rohár wrote:
> > > > > On Tuesday 09 August 2022 12:07:00 Philip Oberfichtner wrote:
> > > > > > This converts CONFIG_SYS_L2_PL310 to Kconfig.
> > > > > ...
> > > > > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> > > > > > index 949ebb46ba..dde06bdd96 100644
> > > > > > --- a/arch/arm/Kconfig
> > > > > > +++ b/arch/arm/Kconfig
> > > > > > @@ -488,6 +488,10 @@ config TPL_SYS_THUMB_BUILD
> > > > > >    	   density. For ARM architectures that support Thumb2 this flag will
> > > > > >    	   result in Thumb2 code generated by GCC.
> > > > > > +config SYS_L2_PL310
> > > > > > +	bool "ARM PL310 L2 cache controller"

This needs a depends on !SYS_L2CACHE_OFF.

> > > > > > +	help
> > > > > > +	  Enable support for ARM PL310 L2 cache controller in U-Boot
> > > > > >    config SYS_L2CACHE_OFF
> > > > > >    	bool "L2cache off"
> > > > > > diff --git a/arch/arm/mach-mvebu/include/mach/config.h b/arch/arm/mach-mvebu/include/mach/config.h
> > > > > > index 4add0d9e10..0bba0a4cf9 100644
> > > > > > --- a/arch/arm/mach-mvebu/include/mach/config.h
> > > > > > +++ b/arch/arm/mach-mvebu/include/mach/config.h
> > > > > > @@ -25,8 +25,6 @@
> > > > > >    #define MV88F78X60 /* for the DDR training bin_hdr code */
> > > > > >    #endif
> > > > > > -#define CONFIG_SYS_L2_PL310
> > > > > > -
> > > > > >    #define MV_UART_CONSOLE_BASE		MVEBU_UART0_BASE
> > > > > >    /* Needed for SPI NOR booting in SPL */
> > > > > 
> > > > > This option is required for mvebu SoC and is not user (de)-selectable.
> > > > > So please do not define it in each individual mvebu board. It would make
> > > > > it harder to introduce a new mvebu board into U-Boot. Instead enable it
> > > > > for mvebu SoCs like it was before this change. It can be done e.g. by
> > > > > "select" Kconfig keyword in mvebu Kconfig file.
> > > > 
> > > > Should it rather be 'default y if MVEBU' in that new PL310 Kconfig option ?
> > > 
> > > No, because this is just default value of this option and still allows
> > > end-user to de-select this option.
> > > 
> > > "select" is IIRC the only way how to force Kconfig to always enable some
> > > symbol without any option for end-user to disable it.
> > > 
> > > At least I do not know a way how CONFIG_SYS_L2_PL310 symbol could decide
> > > that it is required for CONFIG_MVEBU. Just symbol CONFIG_MVEBU can
> > > decide that it requires CONFIG_SYS_L2_PL310 symbol (and not in opposite
> > > direction).
> > 
> > So why should the user be unable to deselect L2CC support on MVEBU ?
> > I can very well disable L2CC support on MX6 and the platform works without
> > it just fine. Maybe the MVEBU needs to be fixed to support the same instead
> > ?
> 
> Well, I'm not sure, currently it is non-deselectable option. Maybe it is
> a bug... but at least change which is doing kconfig conversion should
> not change behavior.

It's tricky to say when to "select" or just "default y if .." or "imply"
a given option. It's not only a matter of "can you disable X and the
system is functional" but "would you normally want to". There are
certainly system bring-up cases and similar where you want to disable
L2CC because you're tracking down something. But it's really a feature
of the chip and expected to work and something we want enabled, and the
scope of when it would be disabled is very very narrow. Looking back at
the patch itself, the config.h files that enabled this are almost all a
SoC-common file (ti_am335x_common.h should have been setting this I bet,
something else for the TODO pile for me), so my thought is that it
should be a select'd option, but if there's strong opinion that it's
useful to make it easy to turn off when needed, imply'd instead by the
various ARCH's in question instead.

That said, thinking about the missing dependency I listed above, that's
how to disable L2 when needed, so perhaps select SYS_L2_PL310 if
!SYS_L2CACHE_OFF under the various ARCH's is the right way.

-- 
Tom

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