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

Boris Brezillon
2016-06-04T06:06:25Z

On Fri, 03 Jun 2016 20:08:49 -0500
Scott Wood <oss@buserror.net> wrote:

> On Wed, 2016-06-01 at 13:23 +0200, Boris Brezillon wrote:
> > The SYS_NAND_U_BOOT_OFFS is quite generic, but the Kconfig entry is forced
> > to explicitly depend on platforms that are not already defining it in their
> > include/configs/<board>.h header.
> >
> > Rename this Kconfig option into SPL_NAND_U_BOOT_OFFS, remove the dependency
> > on NAND_SUNXI and make it dependent on SPL selection.
> >
> > common/spl/spl_nand.c now sets CONFIG_SYS_NAND_U_BOOT_OFFS to
> > CONFIG_SPL_NAND_U_BOOT_OFFS only when it's undefined. This way we stay
> > compatible with the existing behavior.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > Acked-by: Hans de Goede <hdegoede@redhat.com>
> > ---
> > common/spl/spl_nand.c | 4 ++++
> > drivers/mtd/nand/Kconfig | 9 +++++----
> > drivers/mtd/nand/sunxi_nand_spl.c | 8 ++++----
> > 3 files changed, 13 insertions(+), 8 deletions(-)
> >
> > diff --git a/common/spl/spl_nand.c b/common/spl/spl_nand.c
> > index bbd9546..612bd4a 100644
> > --- a/common/spl/spl_nand.c
> > +++ b/common/spl/spl_nand.c
> > @@ -10,6 +10,10 @@
> > #include <asm/io.h>
> > #include <nand.h>
> >
> > +#ifndef CONFIG_SYS_NAND_U_BOOT_OFFS
> > +#define CONFIG_SYS_NAND_U_BOOT_OFFS CONFIG_SPL_NAND_U_BOOT_OFFS
> > +#endif
> [snip]
> > -# Enhance depends when converting drivers to Kconfig which use this config
> > -config SYS_NAND_U_BOOT_OFFS
> > +if SPL
> > +
> > +# This option should be used in replacement of CONFIG_SYS_NAND_U_BOOT_OFFS.
> > +# CONFIG_SYS_NAND_U_BOOT_OFFS is still preferred if defined.
> > +config SPL_NAND_U_BOOT_OFFS
> > hex "Location in NAND to read U-Boot from"
> > default 0x8000 if NAND_SUNXI
> > - depends on NAND_SUNXI
> > help
> > Set the offset from the start of the nand where u-boot should be
> > loaded from.
>
> 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 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.

> 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?
That's what I was trying to avoid. By renaming the Kconfig option we
make sure the value defined in include/configs/<board>.h always take
precedence over the Kconfig value.

--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering