| Message ID | 1464780204-17737-3-git-send-email-boris.brezillon@free-electrons.com |
|---|---|
| State | Changes Requested |
| Headers | show |
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). If you want to do this there needs to be a separate bool config that controls whether the hex config exists. And there'd be no need to rename hex symbol. -Scott
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.
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. > > 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. > > > 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. :-) 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_". -Scott
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.
On Sat, 2016-06-04 at 13:06 +0200, Boris Brezillon wrote: > 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. Ah, right. Still, I think it would be less confusing to not have two different names for the same thing, both of which would be present (albeit only one is used) in the legacy case -- especially if we start adding references directly to the SPL name in some drivers. The bool could eventually be reversed so that only the legacy targets need it. -Scott
On Mon, 06 Jun 2016 12:16:33 -0500 Scott Wood <oss@buserror.net> wrote: > On Sat, 2016-06-04 at 13:06 +0200, Boris Brezillon wrote: > > 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. > > Ah, right. Still, I think it would be less confusing to not have two > different names for the same thing, both of which would be present (albeit > only one is used) in the legacy case -- especially if we start adding > references directly to the SPL name in some drivers. The bool could > eventually be reversed so that only the legacy targets need it. I posted a new version with the extra bool option this morning ;).
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 + #if defined(CONFIG_SPL_NAND_RAW_ONLY) int spl_nand_load_image(void) { diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig index 2fc73ef..4b0f92c 100644 --- a/drivers/mtd/nand/Kconfig +++ b/drivers/mtd/nand/Kconfig @@ -99,16 +99,17 @@ config SYS_NAND_BUSWIDTH_16BIT not available while configuring controller. So a static CONFIG_NAND_xx is needed to know the device's bus-width in advance. -# 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. -if SPL config SPL_NAND_DENALI bool "Support Denali NAND controller for SPL" diff --git a/drivers/mtd/nand/sunxi_nand_spl.c b/drivers/mtd/nand/sunxi_nand_spl.c index 1739da2..85cb127 100644 --- a/drivers/mtd/nand/sunxi_nand_spl.c +++ b/drivers/mtd/nand/sunxi_nand_spl.c @@ -348,14 +348,14 @@ int nand_spl_load_image(uint32_t offs, unsigned int size, void *dest) * u-boot partition sits after 2 eraseblocks (spl, spl-backup), look * for backup u-boot 1 erase block further. */ - const uint32_t eraseblock_size = CONFIG_SYS_NAND_U_BOOT_OFFS / 2; + const uint32_t eraseblock_size = CONFIG_SPL_NAND_U_BOOT_OFFS / 2; const uint32_t boot_offsets[] = { - CONFIG_SYS_NAND_U_BOOT_OFFS, - CONFIG_SYS_NAND_U_BOOT_OFFS + eraseblock_size, + CONFIG_SPL_NAND_U_BOOT_OFFS, + CONFIG_SPL_NAND_U_BOOT_OFFS + eraseblock_size, }; int i; - if (offs == CONFIG_SYS_NAND_U_BOOT_OFFS) { + if (offs == CONFIG_SPL_NAND_U_BOOT_OFFS) { for (i = 0; i < ARRAY_SIZE(boot_offsets); i++) { if (nand_read_buffer(boot_offsets[i], size, dest) == 0)