[U-Boot] [PATCH v2 2/7] spl: nand: rename the SYS_NAND_U_BOOT_OFFS Kconfig option

[Boris Brezillon <boris.brezillon@free-electrons.com>]

On Sat, 04 Jun 2016 02:14:09 -0500
Scott Wood <oss@buserror.net> wrote:

> On Sat, 2016-06-04 at 08:06 +0200, Boris Brezillon wrote:
> > On Fri, 03 Jun 2016 20:08:49 -0500
> > Scott Wood <oss@buserror.net> wrote:
> >  
> > > This doesn't work.  CONFIG_SPL_NAND_U_BOOT_OFFS will always be defined
> > > when SPL is defined, and the user will be forced to enter a value before
> > > kconfig will continue (or kconfig will error out in an automated build).  
> > 
> > Yes, CONFIG_SPL_NAND_U_BOOT_OFFS will always be defined, but won't be
> > used if CONFIG_SYS_NAND_U_BOOT_OFFS is defined in the config header
> > file.
> > And for the "user will forced to enter a value before Kconfig
> > continue" comment, we could just have
> > 
> > config SPL_NAND_U_BOOT_OFFS
> >   	hex "Location in NAND to read U-Boot from"
> >   	default 0x8000 if NAND_SUNXI
> > 	default 0x0
> > 	...  
> 
> If you do that, then that zero will override CONFIG_SYS_NAND_U_BOOT_OFFS from
> the header.

Nope, because the condition is

#ifndef CONFIG_SYS_NAND_U_BOOT_OFFS
#define CONFIG_SYS_NAND_U_BOOT_OFFS CONFIG_SPL_NAND_U_BOOT_OFFS
#endif

and not

#ifdef CONFIG_SPL_NAND_U_BOOT_OFFS
#define CONFIG_SYS_NAND_U_BOOT_OFFS CONFIG_SPL_NAND_U_BOOT_OFFS
#endif

So CONFIG_SYS_NAND_U_BOOT_OFFS is always preferred over
CONFIG_SPL_NAND_U_BOOT_OFFS if it's defined.

> 
> > > If you want to do this there needs to be a separate bool config that
> > > controls whether the hex config exists.  
> > 
> > I can add an extra Kconfig option, but is it really necessary:
> > if people are relying on it they will choose a valid value, and leave
> > it to 0 otherwise.
> > It's just a detail, so I'm fine adding this extra option if you think
> > it's really useful.  
> 
> Zero *is* a valid value.  Several boards already have that value for this
> symbol.  Even if that weren't the case,  we want a mechanism for migrating
> from header value to kconfig value that works for more than just this one
> specific symbol.

Sure, 0 is a perfectly valid value. The "default 0" is just here to
prevent make from blocking because of a missing definition in the
_defconfig.

> 
> >   
> > > And there'd be no need to rename hex symbol.  
> > 
> > Well, functionally there's no problem keeping the existing SYS_ prefix
> > if we add this extra option to activate the U_OFFS config in Kconfig,
> > but I'm not sure this is a good idea to reuse config header names in
> > Kconfig.
> > 
> > And what happens if the user enabled this option (some like to enable
> > everything :-)) and CONFIG_SYS_NAND_U_BOOT_OFFS is also defined in the
> > board config header?  
> 
> Then the build fails with a redefined symbol, and the user learns their
> lesson. :-)

Fair enough.

> 
> The "SYS" in CONFIG_SYS means it's not a user-tunable knob.  From README:
> 
> > There are two classes of configuration variables:
> > 
> > * Configuration _OPTIONS_:
> >   These are selectable by the user and have names beginning with
> >   "CONFIG_".
> > 
> > * Configuration _SETTINGS_:
> >   These depend on the hardware etc. and should not be meddled with if
> >   you don't know what you're doing; they have names beginning with
> >   "CONFIG_SYS_".  

Okay. I'll switch back to SYS_NAND_U_BOOT_OFFS, add an intermediate
option to unlock this one in the config menu and rename
CONFIG_SPL_NAND_U_BOOT_OFFS_REDUND into
CONFIG_SYS_NAND_U_BOOT_OFFS_REDUND.


-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com