| Message ID | 20190221101343.13590-2-martyn.welch@collabora.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [U-Boot,1/3] Add support for the MT41K128M16JT125K memory modules | expand |
On Thu, Feb 21, 2019 at 10:13:42AM +0000, Martyn Welch wrote: > Currently CONFIG_ENV_OFFSET_REDUND is checked regardless of the type of > build being performed, but this doesn't seem to be needed in SPL builds. > > Don't check this configuration option for SPL builds. > > Signed-off-by: Martyn Welch <martyn.welch@collabora.com> > --- > > env/nand.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/env/nand.c b/env/nand.c > index 29eda66fad..d0b95f483d 100644 > --- a/env/nand.c > +++ b/env/nand.c > @@ -26,7 +26,7 @@ > #if defined(CONFIG_CMD_SAVEENV) && defined(CONFIG_CMD_NAND) && \ > !defined(CONFIG_SPL_BUILD) > #define CMD_SAVEENV > -#elif defined(CONFIG_ENV_OFFSET_REDUND) > +#elif defined(CONFIG_ENV_OFFSET_REDUND) && !defined(CONFIG_SPL_BUILD) > #error CONFIG_ENV_OFFSET_REDUND must have CONFIG_CMD_SAVEENV & CONFIG_CMD_NAND > #endif I'm confused. If we have redundant env, and we have env in nand, we need to know. That said, I guess this is just a sanity check for build time, and until we have ENV_OFFSET_REDUND (and others) move to Kconfig we can't also delete those #error lines. Am I at least right about where/how you hit this problem? Thanks!
On Thu, 2019-02-21 at 22:33 -0500, Tom Rini wrote: > On Thu, Feb 21, 2019 at 10:13:42AM +0000, Martyn Welch wrote: > > > Currently CONFIG_ENV_OFFSET_REDUND is checked regardless of the > > type of > > build being performed, but this doesn't seem to be needed in SPL > > builds. > > > > Don't check this configuration option for SPL builds. > > > > Signed-off-by: Martyn Welch <martyn.welch@collabora.com> > > --- > > > > env/nand.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/env/nand.c b/env/nand.c > > index 29eda66fad..d0b95f483d 100644 > > --- a/env/nand.c > > +++ b/env/nand.c > > @@ -26,7 +26,7 @@ > > #if defined(CONFIG_CMD_SAVEENV) && defined(CONFIG_CMD_NAND) && \ > > !defined(CONFIG_SPL_BUILD) > > #define CMD_SAVEENV > > -#elif defined(CONFIG_ENV_OFFSET_REDUND) > > +#elif defined(CONFIG_ENV_OFFSET_REDUND) && > > !defined(CONFIG_SPL_BUILD) > > #error CONFIG_ENV_OFFSET_REDUND must have CONFIG_CMD_SAVEENV & > > CONFIG_CMD_NAND > > #endif > > I'm confused. If we have redundant env, and we have env in nand, we > need to know. That said, I guess this is just a sanity check for > build > time, and until we have ENV_OFFSET_REDUND (and others) move to > Kconfig > we can't also delete those #error lines. Am I at least right about > where/how you hit this problem? Thanks! > We are booting the board with an SPL. We can either do this from NAND, SDCard or via USB with the boot ROM loader. The boot ROM in the am335x can use RNDIS via the USB and thus we use gadget eth from the SPL to load the main U-Boot image. To enable CONFIG_SPL_ETH_SUPPORT, we must enable CONFIG_SPL_ENV_SUPPORT as the environment is used by the eth support, but we don't actually need to have environment variables saved in the SPL environment. We do however have environment variables saved in the main U-Boot image and enable CONFIG_ENV_OFFSET_REDUND (we are storing in raw NAND) and my .config shows that both CONFIG_CMD_SAVEENV and CONFIG_CMD_NAND are set, they just don't seem to be visible when building the SPL. This didn't seem overly odd so haven't looked into why, my assumption was that the above combination wasn't widely used and thus the need to avoid the check when building the SPL in this instance hadn't been anticipated. Am I just looking at this from the wrong angle? Martyn
On Fri, Feb 22, 2019 at 03:35:51PM +0000, Martyn Welch wrote: > On Thu, 2019-02-21 at 22:33 -0500, Tom Rini wrote: > > On Thu, Feb 21, 2019 at 10:13:42AM +0000, Martyn Welch wrote: > > > > > Currently CONFIG_ENV_OFFSET_REDUND is checked regardless of the > > > type of > > > build being performed, but this doesn't seem to be needed in SPL > > > builds. > > > > > > Don't check this configuration option for SPL builds. > > > > > > Signed-off-by: Martyn Welch <martyn.welch@collabora.com> > > > --- > > > > > > env/nand.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/env/nand.c b/env/nand.c > > > index 29eda66fad..d0b95f483d 100644 > > > --- a/env/nand.c > > > +++ b/env/nand.c > > > @@ -26,7 +26,7 @@ > > > #if defined(CONFIG_CMD_SAVEENV) && defined(CONFIG_CMD_NAND) && \ > > > !defined(CONFIG_SPL_BUILD) > > > #define CMD_SAVEENV > > > -#elif defined(CONFIG_ENV_OFFSET_REDUND) > > > +#elif defined(CONFIG_ENV_OFFSET_REDUND) && > > > !defined(CONFIG_SPL_BUILD) > > > #error CONFIG_ENV_OFFSET_REDUND must have CONFIG_CMD_SAVEENV & > > > CONFIG_CMD_NAND > > > #endif > > > > I'm confused. If we have redundant env, and we have env in nand, we > > need to know. That said, I guess this is just a sanity check for > > build > > time, and until we have ENV_OFFSET_REDUND (and others) move to > > Kconfig > > we can't also delete those #error lines. Am I at least right about > > where/how you hit this problem? Thanks! > > > > We are booting the board with an SPL. We can either do this from NAND, > SDCard or via USB with the boot ROM loader. The boot ROM in the am335x > can use RNDIS via the USB and thus we use gadget eth from the SPL to > load the main U-Boot image. To enable CONFIG_SPL_ETH_SUPPORT, we must > enable CONFIG_SPL_ENV_SUPPORT as the environment is used by the eth > support, but we don't actually need to have environment variables saved > in the SPL environment. > > We do however have environment variables saved in the main U-Boot image > and enable CONFIG_ENV_OFFSET_REDUND (we are storing in raw NAND) and my > .config shows that both CONFIG_CMD_SAVEENV and CONFIG_CMD_NAND are set, > they just don't seem to be visible when building the SPL. This didn't > seem overly odd so haven't looked into why, my assumption was that the > above combination wasn't widely used and thus the need to avoid the > check when building the SPL in this instance hadn't been anticipated. > > Am I just looking at this from the wrong angle? OK, so I think your patch is OK in that yes, for now there's some sanity checks we do with #errors that should be Kconfig, once things are migrated fully. But you may also want CONFIG_ENV_IS_IN_NAND and CONFIG_SPL_ENV_IS_NOWHERE for CONFIG_SPL_ETH_SUPPORT as that would avoid this problem and perhaps save you some space?
> On 21.02.2019, at 11:13, Martyn Welch <martyn.welch@collabora.com> wrote: > > Currently CONFIG_ENV_OFFSET_REDUND is checked regardless of the type of > build being performed, but this doesn't seem to be needed in SPL builds. > > Don't check this configuration option for SPL builds. > > Signed-off-by: Martyn Welch <martyn.welch@collabora.com> > --- > > env/nand.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/env/nand.c b/env/nand.c > index 29eda66fad..d0b95f483d 100644 > --- a/env/nand.c > +++ b/env/nand.c > @@ -26,7 +26,7 @@ > #if defined(CONFIG_CMD_SAVEENV) && defined(CONFIG_CMD_NAND) && \ > !defined(CONFIG_SPL_BUILD) > #define CMD_SAVEENV > -#elif defined(CONFIG_ENV_OFFSET_REDUND) > +#elif defined(CONFIG_ENV_OFFSET_REDUND) && !defined(CONFIG_SPL_BUILD) > #error CONFIG_ENV_OFFSET_REDUND must have CONFIG_CMD_SAVEENV & CONFIG_CMD_NAND > #endif Could we use CONFIG_IS_ENABLED and then differentiate via Kconfig and ENV_OFFSET_REDUND, SPL_ENV_OFFSET_REDUND and TPL_ENV_OFFSET_REDUND?
On Fri, 2019-02-22 at 11:36 -0500, Tom Rini wrote: > On Fri, Feb 22, 2019 at 03:35:51PM +0000, Martyn Welch wrote: > > On Thu, 2019-02-21 at 22:33 -0500, Tom Rini wrote: > > > On Thu, Feb 21, 2019 at 10:13:42AM +0000, Martyn Welch wrote: > > > > > > > Currently CONFIG_ENV_OFFSET_REDUND is checked regardless of the > > > > type of > > > > build being performed, but this doesn't seem to be needed in > > > > SPL > > > > builds. > > > > > > > > Don't check this configuration option for SPL builds. > > > > > > > > Signed-off-by: Martyn Welch <martyn.welch@collabora.com> > > > > --- > > > > > > > > env/nand.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/env/nand.c b/env/nand.c > > > > index 29eda66fad..d0b95f483d 100644 > > > > --- a/env/nand.c > > > > +++ b/env/nand.c > > > > @@ -26,7 +26,7 @@ > > > > #if defined(CONFIG_CMD_SAVEENV) && defined(CONFIG_CMD_NAND) && > > > > \ > > > > !defined(CONFIG_SPL_BUILD) > > > > #define CMD_SAVEENV > > > > -#elif defined(CONFIG_ENV_OFFSET_REDUND) > > > > +#elif defined(CONFIG_ENV_OFFSET_REDUND) && > > > > !defined(CONFIG_SPL_BUILD) > > > > #error CONFIG_ENV_OFFSET_REDUND must have CONFIG_CMD_SAVEENV & > > > > CONFIG_CMD_NAND > > > > #endif > > > > > > I'm confused. If we have redundant env, and we have env in nand, > > > we > > > need to know. That said, I guess this is just a sanity check for > > > build > > > time, and until we have ENV_OFFSET_REDUND (and others) move to > > > Kconfig > > > we can't also delete those #error lines. Am I at least right > > > about > > > where/how you hit this problem? Thanks! > > > > > > > We are booting the board with an SPL. We can either do this from > > NAND, > > SDCard or via USB with the boot ROM loader. The boot ROM in the > > am335x > > can use RNDIS via the USB and thus we use gadget eth from the SPL > > to > > load the main U-Boot image. To enable CONFIG_SPL_ETH_SUPPORT, we > > must > > enable CONFIG_SPL_ENV_SUPPORT as the environment is used by the eth > > support, but we don't actually need to have environment variables > > saved > > in the SPL environment. > > > > We do however have environment variables saved in the main U-Boot > > image > > and enable CONFIG_ENV_OFFSET_REDUND (we are storing in raw NAND) > > and my > > .config shows that both CONFIG_CMD_SAVEENV and CONFIG_CMD_NAND are > > set, > > they just don't seem to be visible when building the SPL. This > > didn't > > seem overly odd so haven't looked into why, my assumption was that > > the > > above combination wasn't widely used and thus the need to avoid the > > check when building the SPL in this instance hadn't been > > anticipated. > > > > Am I just looking at this from the wrong angle? > > OK, so I think your patch is OK in that yes, for now there's some > sanity > checks we do with #errors that should be Kconfig, once things are > migrated fully. But you may also want CONFIG_ENV_IS_IN_NAND and > CONFIG_SPL_ENV_IS_NOWHERE for CONFIG_SPL_ETH_SUPPORT as that would > avoid > this problem and perhaps save you some space? > Those are already set in the config :-) Thanks, Martyn
On Fri, Feb 22, 2019 at 05:38:44PM +0100, Philipp Tomsich wrote: > > > > On 21.02.2019, at 11:13, Martyn Welch <martyn.welch@collabora.com> wrote: > > > > Currently CONFIG_ENV_OFFSET_REDUND is checked regardless of the type of > > build being performed, but this doesn't seem to be needed in SPL builds. > > > > Don't check this configuration option for SPL builds. > > > > Signed-off-by: Martyn Welch <martyn.welch@collabora.com> > > --- > > > > env/nand.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/env/nand.c b/env/nand.c > > index 29eda66fad..d0b95f483d 100644 > > --- a/env/nand.c > > +++ b/env/nand.c > > @@ -26,7 +26,7 @@ > > #if defined(CONFIG_CMD_SAVEENV) && defined(CONFIG_CMD_NAND) && \ > > !defined(CONFIG_SPL_BUILD) > > #define CMD_SAVEENV > > -#elif defined(CONFIG_ENV_OFFSET_REDUND) > > +#elif defined(CONFIG_ENV_OFFSET_REDUND) && !defined(CONFIG_SPL_BUILD) > > #error CONFIG_ENV_OFFSET_REDUND must have CONFIG_CMD_SAVEENV & CONFIG_CMD_NAND > > #endif > > Could we use CONFIG_IS_ENABLED and then differentiate via Kconfig > and ENV_OFFSET_REDUND, SPL_ENV_OFFSET_REDUND and > TPL_ENV_OFFSET_REDUND? We need another round of non-trivial ENV related Kconfig migration and then we would remove this hunk entirely and express things correctly. CONFIG_CMD_NAND check here is an abuse from before we had globally a real CONFIG_NAND to check on, and I'm not sure CMD_SAVEENV is _really_ needed (I mean, it is, in order to populate the alt location, I think anyhow).
diff --git a/env/nand.c b/env/nand.c index 29eda66fad..d0b95f483d 100644 --- a/env/nand.c +++ b/env/nand.c @@ -26,7 +26,7 @@ #if defined(CONFIG_CMD_SAVEENV) && defined(CONFIG_CMD_NAND) && \ !defined(CONFIG_SPL_BUILD) #define CMD_SAVEENV -#elif defined(CONFIG_ENV_OFFSET_REDUND) +#elif defined(CONFIG_ENV_OFFSET_REDUND) && !defined(CONFIG_SPL_BUILD) #error CONFIG_ENV_OFFSET_REDUND must have CONFIG_CMD_SAVEENV & CONFIG_CMD_NAND #endif
Currently CONFIG_ENV_OFFSET_REDUND is checked regardless of the type of build being performed, but this doesn't seem to be needed in SPL builds. Don't check this configuration option for SPL builds. Signed-off-by: Martyn Welch <martyn.welch@collabora.com> --- env/nand.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)