[PATCH 01/14] configs: Disable LMB and BDI for tools-only
These two options are useless for tools-only build, since they pull in LMB support which is only useful in a running U-Boot.
Signed-off-by: Marek Vasut marek.vasut+renesas@gmail.com Cc: Simon Glass sjg@chromium.org Cc: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com Cc: Tom Rini trini@konsulko.com --- configs/tools-only_defconfig | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/configs/tools-only_defconfig b/configs/tools-only_defconfig index f54bc1802c..edfefd5475 100644 --- a/configs/tools-only_defconfig +++ b/configs/tools-only_defconfig @@ -5,6 +5,7 @@ CONFIG_ANDROID_BOOT_IMAGE=y CONFIG_FIT=y CONFIG_FIT_SIGNATURE=y CONFIG_MISC_INIT_F=y +# CONFIG_CMD_BDI is not set # CONFIG_CMD_BOOTD is not set # CONFIG_CMD_BOOTM is not set # CONFIG_CMD_ELF is not set @@ -30,3 +31,4 @@ CONFIG_SYSRESET=y # CONFIG_VIRTIO_PCI is not set # CONFIG_VIRTIO_SANDBOX is not set # CONFIG_EFI_LOADER is not set +# CONFIG_LMB is not set
The CONFIG_LMB_*_REGIONS are defined only if CONFIG_LMB is enabled, protect access to those two config options to avoid undefined macro errors.
Signed-off-by: Marek Vasut marek.vasut+renesas@gmail.com Cc: Simon Glass sjg@chromium.org Cc: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com Cc: Tom Rini trini@konsulko.com --- include/lmb.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/lmb.h b/include/lmb.h index 3c4afdf9f0..fa1474a360 100644 --- a/include/lmb.h +++ b/include/lmb.h @@ -44,7 +44,7 @@ struct lmb_property { struct lmb_region { unsigned long cnt; unsigned long max; -#if IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) +#if IS_ENABLED(CONFIG_LMB) && IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) struct lmb_property region[CONFIG_LMB_MAX_REGIONS]; #else struct lmb_property *region; @@ -67,7 +67,7 @@ struct lmb_region { struct lmb { struct lmb_region memory; struct lmb_region reserved; -#if !IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) +#if IS_ENABLED(CONFIG_LMB) && !IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) struct lmb_property memory_regions[CONFIG_LMB_MEMORY_REGIONS]; struct lmb_property reserved_regions[CONFIG_LMB_RESERVED_REGIONS]; #endif
On Sun, Aug 15, 2021 at 08:13:02PM +0200, Marek Vasut wrote:
The CONFIG_LMB_*_REGIONS are defined only if CONFIG_LMB is enabled, protect access to those two config options to avoid undefined macro errors.
Signed-off-by: Marek Vasut marek.vasut+renesas@gmail.com Cc: Simon Glass sjg@chromium.org Cc: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com Cc: Tom Rini trini@konsulko.com
include/lmb.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/lmb.h b/include/lmb.h index 3c4afdf9f0..fa1474a360 100644 --- a/include/lmb.h +++ b/include/lmb.h @@ -44,7 +44,7 @@ struct lmb_property { struct lmb_region { unsigned long cnt; unsigned long max; -#if IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) +#if IS_ENABLED(CONFIG_LMB) && IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) struct lmb_property region[CONFIG_LMB_MAX_REGIONS]; #else struct lmb_property *region; @@ -67,7 +67,7 @@ struct lmb_region { struct lmb { struct lmb_region memory; struct lmb_region reserved; -#if !IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) +#if IS_ENABLED(CONFIG_LMB) && !IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) struct lmb_property memory_regions[CONFIG_LMB_MEMORY_REGIONS]; struct lmb_property reserved_regions[CONFIG_LMB_RESERVED_REGIONS]; #endif
We shouldn't need this at all. LMB and LMB_USE_MAX_REGIONS are both in Kconfig and have the dependencies expressed that way.
On 8/15/21 9:47 PM, Tom Rini wrote:
On Sun, Aug 15, 2021 at 08:13:02PM +0200, Marek Vasut wrote:
The CONFIG_LMB_*_REGIONS are defined only if CONFIG_LMB is enabled, protect access to those two config options to avoid undefined macro errors.
Signed-off-by: Marek Vasut marek.vasut+renesas@gmail.com Cc: Simon Glass sjg@chromium.org Cc: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com Cc: Tom Rini trini@konsulko.com
include/lmb.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/lmb.h b/include/lmb.h index 3c4afdf9f0..fa1474a360 100644 --- a/include/lmb.h +++ b/include/lmb.h @@ -44,7 +44,7 @@ struct lmb_property { struct lmb_region { unsigned long cnt; unsigned long max; -#if IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) +#if IS_ENABLED(CONFIG_LMB) && IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) struct lmb_property region[CONFIG_LMB_MAX_REGIONS]; #else struct lmb_property *region; @@ -67,7 +67,7 @@ struct lmb_region { struct lmb { struct lmb_region memory; struct lmb_region reserved; -#if !IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) +#if IS_ENABLED(CONFIG_LMB) && !IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) struct lmb_property memory_regions[CONFIG_LMB_MEMORY_REGIONS]; struct lmb_property reserved_regions[CONFIG_LMB_RESERVED_REGIONS]; #endif
We shouldn't need this at all. LMB and LMB_USE_MAX_REGIONS are both in Kconfig and have the dependencies expressed that way.
However, CONFIG_LMB_MEMORY_REGIONS and CONFIG_LMB_RESERVED_REGIONS may be undefined if CONFIG_LMB and !CONFIG_LMB_USE_MAX_REGIONS . They are four different symbols.
On Sun, Aug 29, 2021 at 06:26:23PM +0200, Marek Vasut wrote:
On 8/15/21 9:47 PM, Tom Rini wrote:
On Sun, Aug 15, 2021 at 08:13:02PM +0200, Marek Vasut wrote:
The CONFIG_LMB_*_REGIONS are defined only if CONFIG_LMB is enabled, protect access to those two config options to avoid undefined macro errors.
Signed-off-by: Marek Vasut marek.vasut+renesas@gmail.com Cc: Simon Glass sjg@chromium.org Cc: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com Cc: Tom Rini trini@konsulko.com
include/lmb.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/lmb.h b/include/lmb.h index 3c4afdf9f0..fa1474a360 100644 --- a/include/lmb.h +++ b/include/lmb.h @@ -44,7 +44,7 @@ struct lmb_property { struct lmb_region { unsigned long cnt; unsigned long max; -#if IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) +#if IS_ENABLED(CONFIG_LMB) && IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS)
This doesn't make sense to me, still. You cannot enable CONFIG_LMB_USE_MAX_REGIONS without CONFIG_LMB as the former depends on the latter in Kconfig.
struct lmb_property region[CONFIG_LMB_MAX_REGIONS]; #else struct lmb_property *region; @@ -67,7 +67,7 @@ struct lmb_region { struct lmb { struct lmb_region memory; struct lmb_region reserved; -#if !IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) +#if IS_ENABLED(CONFIG_LMB) && !IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) struct lmb_property memory_regions[CONFIG_LMB_MEMORY_REGIONS]; struct lmb_property reserved_regions[CONFIG_LMB_RESERVED_REGIONS]; #endif
We shouldn't need this at all. LMB and LMB_USE_MAX_REGIONS are both in Kconfig and have the dependencies expressed that way.
However, CONFIG_LMB_MEMORY_REGIONS and CONFIG_LMB_RESERVED_REGIONS may be undefined if CONFIG_LMB and !CONFIG_LMB_USE_MAX_REGIONS . They are four different symbols.
I'm still not seeing it, sorry. Is there some case where we're trying to access a struct lmb without CONFIG_LMB enabled?
On 8/29/21 8:02 PM, Tom Rini wrote:
On Sun, Aug 29, 2021 at 06:26:23PM +0200, Marek Vasut wrote:
On 8/15/21 9:47 PM, Tom Rini wrote:
On Sun, Aug 15, 2021 at 08:13:02PM +0200, Marek Vasut wrote:
The CONFIG_LMB_*_REGIONS are defined only if CONFIG_LMB is enabled, protect access to those two config options to avoid undefined macro errors.
Signed-off-by: Marek Vasut marek.vasut+renesas@gmail.com Cc: Simon Glass sjg@chromium.org Cc: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com Cc: Tom Rini trini@konsulko.com
include/lmb.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/lmb.h b/include/lmb.h index 3c4afdf9f0..fa1474a360 100644 --- a/include/lmb.h +++ b/include/lmb.h @@ -44,7 +44,7 @@ struct lmb_property { struct lmb_region { unsigned long cnt; unsigned long max; -#if IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) +#if IS_ENABLED(CONFIG_LMB) && IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS)
This doesn't make sense to me, still. You cannot enable CONFIG_LMB_USE_MAX_REGIONS without CONFIG_LMB as the former depends on the latter in Kconfig.
struct lmb_property region[CONFIG_LMB_MAX_REGIONS];#else struct lmb_property *region; @@ -67,7 +67,7 @@ struct lmb_region { struct lmb { struct lmb_region memory; struct lmb_region reserved; -#if !IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) +#if IS_ENABLED(CONFIG_LMB) && !IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) struct lmb_property memory_regions[CONFIG_LMB_MEMORY_REGIONS]; struct lmb_property reserved_regions[CONFIG_LMB_RESERVED_REGIONS]; #endif
We shouldn't need this at all. LMB and LMB_USE_MAX_REGIONS are both in Kconfig and have the dependencies expressed that way.
However, CONFIG_LMB_MEMORY_REGIONS and CONFIG_LMB_RESERVED_REGIONS may be undefined if CONFIG_LMB and !CONFIG_LMB_USE_MAX_REGIONS . They are four different symbols.
I'm still not seeing it, sorry. Is there some case where we're trying to access a struct lmb without CONFIG_LMB enabled?
See build failure https://source.denx.de/u-boot/custodians/u-boot-sh/-/jobs/315331
On Sun, Aug 29, 2021 at 09:24:46PM +0200, Marek Vasut wrote:
On 8/29/21 8:02 PM, Tom Rini wrote:
On Sun, Aug 29, 2021 at 06:26:23PM +0200, Marek Vasut wrote:
On 8/15/21 9:47 PM, Tom Rini wrote:
On Sun, Aug 15, 2021 at 08:13:02PM +0200, Marek Vasut wrote:
The CONFIG_LMB_*_REGIONS are defined only if CONFIG_LMB is enabled, protect access to those two config options to avoid undefined macro errors.
Signed-off-by: Marek Vasut marek.vasut+renesas@gmail.com Cc: Simon Glass sjg@chromium.org Cc: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com Cc: Tom Rini trini@konsulko.com
include/lmb.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/lmb.h b/include/lmb.h index 3c4afdf9f0..fa1474a360 100644 --- a/include/lmb.h +++ b/include/lmb.h @@ -44,7 +44,7 @@ struct lmb_property { struct lmb_region { unsigned long cnt; unsigned long max; -#if IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) +#if IS_ENABLED(CONFIG_LMB) && IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS)
This doesn't make sense to me, still. You cannot enable CONFIG_LMB_USE_MAX_REGIONS without CONFIG_LMB as the former depends on the latter in Kconfig.
struct lmb_property region[CONFIG_LMB_MAX_REGIONS];#else struct lmb_property *region; @@ -67,7 +67,7 @@ struct lmb_region { struct lmb { struct lmb_region memory; struct lmb_region reserved; -#if !IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) +#if IS_ENABLED(CONFIG_LMB) && !IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) struct lmb_property memory_regions[CONFIG_LMB_MEMORY_REGIONS]; struct lmb_property reserved_regions[CONFIG_LMB_RESERVED_REGIONS]; #endif
We shouldn't need this at all. LMB and LMB_USE_MAX_REGIONS are both in Kconfig and have the dependencies expressed that way.
However, CONFIG_LMB_MEMORY_REGIONS and CONFIG_LMB_RESERVED_REGIONS may be undefined if CONFIG_LMB and !CONFIG_LMB_USE_MAX_REGIONS . They are four different symbols.
I'm still not seeing it, sorry. Is there some case where we're trying to access a struct lmb without CONFIG_LMB enabled?
See build failure https://source.denx.de/u-boot/custodians/u-boot-sh/-/jobs/315331
Ah, progress. Drop <lmb.h> from <image.h> since we already have a forward declaration of struct lmb? But it's not failing without this series too, so what's changing?
On 8/29/21 9:32 PM, Tom Rini wrote:
On Sun, Aug 29, 2021 at 09:24:46PM +0200, Marek Vasut wrote:
On 8/29/21 8:02 PM, Tom Rini wrote:
On Sun, Aug 29, 2021 at 06:26:23PM +0200, Marek Vasut wrote:
On 8/15/21 9:47 PM, Tom Rini wrote:
On Sun, Aug 15, 2021 at 08:13:02PM +0200, Marek Vasut wrote:
The CONFIG_LMB_*_REGIONS are defined only if CONFIG_LMB is enabled, protect access to those two config options to avoid undefined macro errors.
Signed-off-by: Marek Vasut marek.vasut+renesas@gmail.com Cc: Simon Glass sjg@chromium.org Cc: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com Cc: Tom Rini trini@konsulko.com
include/lmb.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)diff --git a/include/lmb.h b/include/lmb.h index 3c4afdf9f0..fa1474a360 100644 --- a/include/lmb.h +++ b/include/lmb.h @@ -44,7 +44,7 @@ struct lmb_property { struct lmb_region { unsigned long cnt; unsigned long max; -#if IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) +#if IS_ENABLED(CONFIG_LMB) && IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS)
This doesn't make sense to me, still. You cannot enable CONFIG_LMB_USE_MAX_REGIONS without CONFIG_LMB as the former depends on the latter in Kconfig.
struct lmb_property region[CONFIG_LMB_MAX_REGIONS]; #else struct lmb_property *region;@@ -67,7 +67,7 @@ struct lmb_region { struct lmb { struct lmb_region memory; struct lmb_region reserved; -#if !IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) +#if IS_ENABLED(CONFIG_LMB) && !IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) struct lmb_property memory_regions[CONFIG_LMB_MEMORY_REGIONS]; struct lmb_property reserved_regions[CONFIG_LMB_RESERVED_REGIONS]; #endif
We shouldn't need this at all. LMB and LMB_USE_MAX_REGIONS are both in Kconfig and have the dependencies expressed that way.
However, CONFIG_LMB_MEMORY_REGIONS and CONFIG_LMB_RESERVED_REGIONS may be undefined if CONFIG_LMB and !CONFIG_LMB_USE_MAX_REGIONS . They are four different symbols.
I'm still not seeing it, sorry. Is there some case where we're trying to access a struct lmb without CONFIG_LMB enabled?
See build failure https://source.denx.de/u-boot/custodians/u-boot-sh/-/jobs/315331
Ah, progress. Drop <lmb.h> from <image.h> since we already have a forward declaration of struct lmb? But it's not failing without this series too, so what's changing?
See 01/14 in this series.
On Sun, Aug 29, 2021 at 11:47:58PM +0200, Marek Vasut wrote:
On 8/29/21 9:32 PM, Tom Rini wrote:
On Sun, Aug 29, 2021 at 09:24:46PM +0200, Marek Vasut wrote:
On 8/29/21 8:02 PM, Tom Rini wrote:
On Sun, Aug 29, 2021 at 06:26:23PM +0200, Marek Vasut wrote:
On 8/15/21 9:47 PM, Tom Rini wrote:
On Sun, Aug 15, 2021 at 08:13:02PM +0200, Marek Vasut wrote:
> The CONFIG_LMB_*_REGIONS are defined only if CONFIG_LMB is enabled, > protect access to those two config options to avoid undefined macro > errors. > > Signed-off-by: Marek Vasut marek.vasut+renesas@gmail.com > Cc: Simon Glass sjg@chromium.org > Cc: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com > Cc: Tom Rini trini@konsulko.com > --- > include/lmb.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/include/lmb.h b/include/lmb.h > index 3c4afdf9f0..fa1474a360 100644 > --- a/include/lmb.h > +++ b/include/lmb.h > @@ -44,7 +44,7 @@ struct lmb_property { > struct lmb_region { > unsigned long cnt; > unsigned long max; > -#if IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) > +#if IS_ENABLED(CONFIG_LMB) && IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS)
This doesn't make sense to me, still. You cannot enable CONFIG_LMB_USE_MAX_REGIONS without CONFIG_LMB as the former depends on the latter in Kconfig.
> struct lmb_property region[CONFIG_LMB_MAX_REGIONS]; > #else > struct lmb_property *region; > @@ -67,7 +67,7 @@ struct lmb_region { > struct lmb { > struct lmb_region memory; > struct lmb_region reserved; > -#if !IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) > +#if IS_ENABLED(CONFIG_LMB) && !IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) > struct lmb_property memory_regions[CONFIG_LMB_MEMORY_REGIONS]; > struct lmb_property reserved_regions[CONFIG_LMB_RESERVED_REGIONS]; > #endif
We shouldn't need this at all. LMB and LMB_USE_MAX_REGIONS are both in Kconfig and have the dependencies expressed that way.
However, CONFIG_LMB_MEMORY_REGIONS and CONFIG_LMB_RESERVED_REGIONS may be undefined if CONFIG_LMB and !CONFIG_LMB_USE_MAX_REGIONS . They are four different symbols.
I'm still not seeing it, sorry. Is there some case where we're trying to access a struct lmb without CONFIG_LMB enabled?
See build failure https://source.denx.de/u-boot/custodians/u-boot-sh/-/jobs/315331
Ah, progress. Drop <lmb.h> from <image.h> since we already have a forward declaration of struct lmb? But it's not failing without this series too, so what's changing?
See 01/14 in this series.
Ah, so drop 1/14 then.
On 8/30/21 12:10 AM, Tom Rini wrote:
On Sun, Aug 29, 2021 at 11:47:58PM +0200, Marek Vasut wrote:
On 8/29/21 9:32 PM, Tom Rini wrote:
On Sun, Aug 29, 2021 at 09:24:46PM +0200, Marek Vasut wrote:
On 8/29/21 8:02 PM, Tom Rini wrote:
On Sun, Aug 29, 2021 at 06:26:23PM +0200, Marek Vasut wrote:
On 8/15/21 9:47 PM, Tom Rini wrote: > On Sun, Aug 15, 2021 at 08:13:02PM +0200, Marek Vasut wrote: > >> The CONFIG_LMB_*_REGIONS are defined only if CONFIG_LMB is enabled, >> protect access to those two config options to avoid undefined macro >> errors. >> >> Signed-off-by: Marek Vasut marek.vasut+renesas@gmail.com >> Cc: Simon Glass sjg@chromium.org >> Cc: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com >> Cc: Tom Rini trini@konsulko.com >> --- >> include/lmb.h | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/include/lmb.h b/include/lmb.h >> index 3c4afdf9f0..fa1474a360 100644 >> --- a/include/lmb.h >> +++ b/include/lmb.h >> @@ -44,7 +44,7 @@ struct lmb_property { >> struct lmb_region { >> unsigned long cnt; >> unsigned long max; >> -#if IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) >> +#if IS_ENABLED(CONFIG_LMB) && IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS)
This doesn't make sense to me, still. You cannot enable CONFIG_LMB_USE_MAX_REGIONS without CONFIG_LMB as the former depends on the latter in Kconfig.
>> struct lmb_property region[CONFIG_LMB_MAX_REGIONS]; >> #else >> struct lmb_property *region; >> @@ -67,7 +67,7 @@ struct lmb_region { >> struct lmb { >> struct lmb_region memory; >> struct lmb_region reserved; >> -#if !IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) >> +#if IS_ENABLED(CONFIG_LMB) && !IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) >> struct lmb_property memory_regions[CONFIG_LMB_MEMORY_REGIONS]; >> struct lmb_property reserved_regions[CONFIG_LMB_RESERVED_REGIONS]; >> #endif > > We shouldn't need this at all. LMB and LMB_USE_MAX_REGIONS are both in > Kconfig and have the dependencies expressed that way.
However, CONFIG_LMB_MEMORY_REGIONS and CONFIG_LMB_RESERVED_REGIONS may be undefined if CONFIG_LMB and !CONFIG_LMB_USE_MAX_REGIONS . They are four different symbols.
I'm still not seeing it, sorry. Is there some case where we're trying to access a struct lmb without CONFIG_LMB enabled?
See build failure https://source.denx.de/u-boot/custodians/u-boot-sh/-/jobs/315331
Ah, progress. Drop <lmb.h> from <image.h> since we already have a forward declaration of struct lmb? But it's not failing without this series too, so what's changing?
See 01/14 in this series.
Ah, so drop 1/14 then.
Why ? That patch is correct.
On Mon, Aug 30, 2021 at 12:19:59AM +0200, Marek Vasut wrote:
On 8/30/21 12:10 AM, Tom Rini wrote:
On Sun, Aug 29, 2021 at 11:47:58PM +0200, Marek Vasut wrote:
On 8/29/21 9:32 PM, Tom Rini wrote:
On Sun, Aug 29, 2021 at 09:24:46PM +0200, Marek Vasut wrote:
On 8/29/21 8:02 PM, Tom Rini wrote:
On Sun, Aug 29, 2021 at 06:26:23PM +0200, Marek Vasut wrote: > On 8/15/21 9:47 PM, Tom Rini wrote: > > On Sun, Aug 15, 2021 at 08:13:02PM +0200, Marek Vasut wrote: > > > > > The CONFIG_LMB_*_REGIONS are defined only if CONFIG_LMB is enabled, > > > protect access to those two config options to avoid undefined macro > > > errors. > > > > > > Signed-off-by: Marek Vasut marek.vasut+renesas@gmail.com > > > Cc: Simon Glass sjg@chromium.org > > > Cc: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com > > > Cc: Tom Rini trini@konsulko.com > > > --- > > > include/lmb.h | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/include/lmb.h b/include/lmb.h > > > index 3c4afdf9f0..fa1474a360 100644 > > > --- a/include/lmb.h > > > +++ b/include/lmb.h > > > @@ -44,7 +44,7 @@ struct lmb_property { > > > struct lmb_region { > > > unsigned long cnt; > > > unsigned long max; > > > -#if IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) > > > +#if IS_ENABLED(CONFIG_LMB) && IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS)
This doesn't make sense to me, still. You cannot enable CONFIG_LMB_USE_MAX_REGIONS without CONFIG_LMB as the former depends on the latter in Kconfig.
> > > struct lmb_property region[CONFIG_LMB_MAX_REGIONS]; > > > #else > > > struct lmb_property *region; > > > @@ -67,7 +67,7 @@ struct lmb_region { > > > struct lmb { > > > struct lmb_region memory; > > > struct lmb_region reserved; > > > -#if !IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) > > > +#if IS_ENABLED(CONFIG_LMB) && !IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) > > > struct lmb_property memory_regions[CONFIG_LMB_MEMORY_REGIONS]; > > > struct lmb_property reserved_regions[CONFIG_LMB_RESERVED_REGIONS]; > > > #endif > > > > We shouldn't need this at all. LMB and LMB_USE_MAX_REGIONS are both in > > Kconfig and have the dependencies expressed that way. > > However, CONFIG_LMB_MEMORY_REGIONS and CONFIG_LMB_RESERVED_REGIONS may be > undefined if CONFIG_LMB and !CONFIG_LMB_USE_MAX_REGIONS . They are four > different symbols.
I'm still not seeing it, sorry. Is there some case where we're trying to access a struct lmb without CONFIG_LMB enabled?
See build failure https://source.denx.de/u-boot/custodians/u-boot-sh/-/jobs/315331
Ah, progress. Drop <lmb.h> from <image.h> since we already have a forward declaration of struct lmb? But it's not failing without this series too, so what's changing?
See 01/14 in this series.
Ah, so drop 1/14 then.
Why ? That patch is correct.
It's not quite right, 1/14 and then 2/14 are papering over the fact that lmb.h, and it's including headers / files, need to be cleaned up so that we don't need to have redundant tests in the header.
On 8/30/21 12:23 AM, Tom Rini wrote:
On Mon, Aug 30, 2021 at 12:19:59AM +0200, Marek Vasut wrote:
On 8/30/21 12:10 AM, Tom Rini wrote:
On Sun, Aug 29, 2021 at 11:47:58PM +0200, Marek Vasut wrote:
On 8/29/21 9:32 PM, Tom Rini wrote:
On Sun, Aug 29, 2021 at 09:24:46PM +0200, Marek Vasut wrote:
On 8/29/21 8:02 PM, Tom Rini wrote: > On Sun, Aug 29, 2021 at 06:26:23PM +0200, Marek Vasut wrote: >> On 8/15/21 9:47 PM, Tom Rini wrote: >>> On Sun, Aug 15, 2021 at 08:13:02PM +0200, Marek Vasut wrote: >>> >>>> The CONFIG_LMB_*_REGIONS are defined only if CONFIG_LMB is enabled, >>>> protect access to those two config options to avoid undefined macro >>>> errors. >>>> >>>> Signed-off-by: Marek Vasut marek.vasut+renesas@gmail.com >>>> Cc: Simon Glass sjg@chromium.org >>>> Cc: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com >>>> Cc: Tom Rini trini@konsulko.com >>>> --- >>>> include/lmb.h | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/include/lmb.h b/include/lmb.h >>>> index 3c4afdf9f0..fa1474a360 100644 >>>> --- a/include/lmb.h >>>> +++ b/include/lmb.h >>>> @@ -44,7 +44,7 @@ struct lmb_property { >>>> struct lmb_region { >>>> unsigned long cnt; >>>> unsigned long max; >>>> -#if IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) >>>> +#if IS_ENABLED(CONFIG_LMB) && IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) > > This doesn't make sense to me, still. You cannot enable > CONFIG_LMB_USE_MAX_REGIONS without CONFIG_LMB as the former depends on > the latter in Kconfig. > >>>> struct lmb_property region[CONFIG_LMB_MAX_REGIONS]; >>>> #else >>>> struct lmb_property *region; >>>> @@ -67,7 +67,7 @@ struct lmb_region { >>>> struct lmb { >>>> struct lmb_region memory; >>>> struct lmb_region reserved; >>>> -#if !IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) >>>> +#if IS_ENABLED(CONFIG_LMB) && !IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) >>>> struct lmb_property memory_regions[CONFIG_LMB_MEMORY_REGIONS]; >>>> struct lmb_property reserved_regions[CONFIG_LMB_RESERVED_REGIONS]; >>>> #endif >>> >>> We shouldn't need this at all. LMB and LMB_USE_MAX_REGIONS are both in >>> Kconfig and have the dependencies expressed that way. >> >> However, CONFIG_LMB_MEMORY_REGIONS and CONFIG_LMB_RESERVED_REGIONS may be >> undefined if CONFIG_LMB and !CONFIG_LMB_USE_MAX_REGIONS . They are four >> different symbols. > > I'm still not seeing it, sorry. Is there some case where we're trying > to access a struct lmb without CONFIG_LMB enabled? >
See build failure https://source.denx.de/u-boot/custodians/u-boot-sh/-/jobs/315331
Ah, progress. Drop <lmb.h> from <image.h> since we already have a forward declaration of struct lmb? But it's not failing without this series too, so what's changing?
See 01/14 in this series.
Ah, so drop 1/14 then.
Why ? That patch is correct.
It's not quite right, 1/14 and then 2/14 are papering over the fact that lmb.h, and it's including headers / files, need to be cleaned up so that we don't need to have redundant tests in the header.
1/14 disables LMB and CMD_BDI for tools build, we do not need those, so 1/14 is correct.
What kind of cleanup of lmb.h do you have in mind ?
On Mon, Aug 30, 2021 at 12:40:07AM +0200, Marek Vasut wrote:
On 8/30/21 12:23 AM, Tom Rini wrote:
On Mon, Aug 30, 2021 at 12:19:59AM +0200, Marek Vasut wrote:
On 8/30/21 12:10 AM, Tom Rini wrote:
On Sun, Aug 29, 2021 at 11:47:58PM +0200, Marek Vasut wrote:
On 8/29/21 9:32 PM, Tom Rini wrote:
On Sun, Aug 29, 2021 at 09:24:46PM +0200, Marek Vasut wrote: > On 8/29/21 8:02 PM, Tom Rini wrote: > > On Sun, Aug 29, 2021 at 06:26:23PM +0200, Marek Vasut wrote: > > > On 8/15/21 9:47 PM, Tom Rini wrote: > > > > On Sun, Aug 15, 2021 at 08:13:02PM +0200, Marek Vasut wrote: > > > > > > > > > The CONFIG_LMB_*_REGIONS are defined only if CONFIG_LMB is enabled, > > > > > protect access to those two config options to avoid undefined macro > > > > > errors. > > > > > > > > > > Signed-off-by: Marek Vasut marek.vasut+renesas@gmail.com > > > > > Cc: Simon Glass sjg@chromium.org > > > > > Cc: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com > > > > > Cc: Tom Rini trini@konsulko.com > > > > > --- > > > > > include/lmb.h | 4 ++-- > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/include/lmb.h b/include/lmb.h > > > > > index 3c4afdf9f0..fa1474a360 100644 > > > > > --- a/include/lmb.h > > > > > +++ b/include/lmb.h > > > > > @@ -44,7 +44,7 @@ struct lmb_property { > > > > > struct lmb_region { > > > > > unsigned long cnt; > > > > > unsigned long max; > > > > > -#if IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) > > > > > +#if IS_ENABLED(CONFIG_LMB) && IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) > > > > This doesn't make sense to me, still. You cannot enable > > CONFIG_LMB_USE_MAX_REGIONS without CONFIG_LMB as the former depends on > > the latter in Kconfig. > > > > > > > struct lmb_property region[CONFIG_LMB_MAX_REGIONS]; > > > > > #else > > > > > struct lmb_property *region; > > > > > @@ -67,7 +67,7 @@ struct lmb_region { > > > > > struct lmb { > > > > > struct lmb_region memory; > > > > > struct lmb_region reserved; > > > > > -#if !IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) > > > > > +#if IS_ENABLED(CONFIG_LMB) && !IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) > > > > > struct lmb_property memory_regions[CONFIG_LMB_MEMORY_REGIONS]; > > > > > struct lmb_property reserved_regions[CONFIG_LMB_RESERVED_REGIONS]; > > > > > #endif > > > > > > > > We shouldn't need this at all. LMB and LMB_USE_MAX_REGIONS are both in > > > > Kconfig and have the dependencies expressed that way. > > > > > > However, CONFIG_LMB_MEMORY_REGIONS and CONFIG_LMB_RESERVED_REGIONS may be > > > undefined if CONFIG_LMB and !CONFIG_LMB_USE_MAX_REGIONS . They are four > > > different symbols. > > > > I'm still not seeing it, sorry. Is there some case where we're trying > > to access a struct lmb without CONFIG_LMB enabled? > > > > See build failure > https://source.denx.de/u-boot/custodians/u-boot-sh/-/jobs/315331
Ah, progress. Drop <lmb.h> from <image.h> since we already have a forward declaration of struct lmb? But it's not failing without this series too, so what's changing?
See 01/14 in this series.
Ah, so drop 1/14 then.
Why ? That patch is correct.
It's not quite right, 1/14 and then 2/14 are papering over the fact that lmb.h, and it's including headers / files, need to be cleaned up so that we don't need to have redundant tests in the header.
1/14 disables LMB and CMD_BDI for tools build, we do not need those, so 1/14 is correct.
We don't need to build u-boot at all for tools-only, only the tools-only build target. It's just annoying to exclude the tools-only_defconfig from "sandbox" in CI.
What kind of cleanup of lmb.h do you have in mind ?
Remove it from include/image.h and fix any fall-out from that of files that got <lmb.h> indirectly when they needed it directly instead.
On 8/30/21 12:51 AM, Tom Rini wrote:
On Mon, Aug 30, 2021 at 12:40:07AM +0200, Marek Vasut wrote:
On 8/30/21 12:23 AM, Tom Rini wrote:
On Mon, Aug 30, 2021 at 12:19:59AM +0200, Marek Vasut wrote:
On 8/30/21 12:10 AM, Tom Rini wrote:
On Sun, Aug 29, 2021 at 11:47:58PM +0200, Marek Vasut wrote:
On 8/29/21 9:32 PM, Tom Rini wrote: > On Sun, Aug 29, 2021 at 09:24:46PM +0200, Marek Vasut wrote: >> On 8/29/21 8:02 PM, Tom Rini wrote: >>> On Sun, Aug 29, 2021 at 06:26:23PM +0200, Marek Vasut wrote: >>>> On 8/15/21 9:47 PM, Tom Rini wrote: >>>>> On Sun, Aug 15, 2021 at 08:13:02PM +0200, Marek Vasut wrote: >>>>> >>>>>> The CONFIG_LMB_*_REGIONS are defined only if CONFIG_LMB is enabled, >>>>>> protect access to those two config options to avoid undefined macro >>>>>> errors. >>>>>> >>>>>> Signed-off-by: Marek Vasut marek.vasut+renesas@gmail.com >>>>>> Cc: Simon Glass sjg@chromium.org >>>>>> Cc: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com >>>>>> Cc: Tom Rini trini@konsulko.com >>>>>> --- >>>>>> include/lmb.h | 4 ++-- >>>>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/include/lmb.h b/include/lmb.h >>>>>> index 3c4afdf9f0..fa1474a360 100644 >>>>>> --- a/include/lmb.h >>>>>> +++ b/include/lmb.h >>>>>> @@ -44,7 +44,7 @@ struct lmb_property { >>>>>> struct lmb_region { >>>>>> unsigned long cnt; >>>>>> unsigned long max; >>>>>> -#if IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) >>>>>> +#if IS_ENABLED(CONFIG_LMB) && IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) >>> >>> This doesn't make sense to me, still. You cannot enable >>> CONFIG_LMB_USE_MAX_REGIONS without CONFIG_LMB as the former depends on >>> the latter in Kconfig. >>> >>>>>> struct lmb_property region[CONFIG_LMB_MAX_REGIONS]; >>>>>> #else >>>>>> struct lmb_property *region; >>>>>> @@ -67,7 +67,7 @@ struct lmb_region { >>>>>> struct lmb { >>>>>> struct lmb_region memory; >>>>>> struct lmb_region reserved; >>>>>> -#if !IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) >>>>>> +#if IS_ENABLED(CONFIG_LMB) && !IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) >>>>>> struct lmb_property memory_regions[CONFIG_LMB_MEMORY_REGIONS]; >>>>>> struct lmb_property reserved_regions[CONFIG_LMB_RESERVED_REGIONS]; >>>>>> #endif >>>>> >>>>> We shouldn't need this at all. LMB and LMB_USE_MAX_REGIONS are both in >>>>> Kconfig and have the dependencies expressed that way. >>>> >>>> However, CONFIG_LMB_MEMORY_REGIONS and CONFIG_LMB_RESERVED_REGIONS may be >>>> undefined if CONFIG_LMB and !CONFIG_LMB_USE_MAX_REGIONS . They are four >>>> different symbols. >>> >>> I'm still not seeing it, sorry. Is there some case where we're trying >>> to access a struct lmb without CONFIG_LMB enabled? >>> >> >> See build failure >> https://source.denx.de/u-boot/custodians/u-boot-sh/-/jobs/315331 > > Ah, progress. Drop <lmb.h> from <image.h> since we already have a > forward declaration of struct lmb? But it's not failing without this > series too, so what's changing?
See 01/14 in this series.
Ah, so drop 1/14 then.
Why ? That patch is correct.
It's not quite right, 1/14 and then 2/14 are papering over the fact that lmb.h, and it's including headers / files, need to be cleaned up so that we don't need to have redundant tests in the header.
1/14 disables LMB and CMD_BDI for tools build, we do not need those, so 1/14 is correct.
We don't need to build u-boot at all for tools-only, only the tools-only build target. It's just annoying to exclude the tools-only_defconfig from "sandbox" in CI.
So, what exactly is the problem with that 01/14 ? Please elaborate, I believe the patch is correct.
What kind of cleanup of lmb.h do you have in mind ?
Remove it from include/image.h and fix any fall-out from that of files that got <lmb.h> indirectly when they needed it directly instead.
Uh ... that is likely for a separate series, and a big one.
On Mon, Aug 30, 2021 at 01:00:02AM +0200, Marek Vasut wrote:
On 8/30/21 12:51 AM, Tom Rini wrote:
On Mon, Aug 30, 2021 at 12:40:07AM +0200, Marek Vasut wrote:
On 8/30/21 12:23 AM, Tom Rini wrote:
On Mon, Aug 30, 2021 at 12:19:59AM +0200, Marek Vasut wrote:
On 8/30/21 12:10 AM, Tom Rini wrote:
On Sun, Aug 29, 2021 at 11:47:58PM +0200, Marek Vasut wrote: > On 8/29/21 9:32 PM, Tom Rini wrote: > > On Sun, Aug 29, 2021 at 09:24:46PM +0200, Marek Vasut wrote: > > > On 8/29/21 8:02 PM, Tom Rini wrote: > > > > On Sun, Aug 29, 2021 at 06:26:23PM +0200, Marek Vasut wrote: > > > > > On 8/15/21 9:47 PM, Tom Rini wrote: > > > > > > On Sun, Aug 15, 2021 at 08:13:02PM +0200, Marek Vasut wrote: > > > > > > > > > > > > > The CONFIG_LMB_*_REGIONS are defined only if CONFIG_LMB is enabled, > > > > > > > protect access to those two config options to avoid undefined macro > > > > > > > errors. > > > > > > > > > > > > > > Signed-off-by: Marek Vasut marek.vasut+renesas@gmail.com > > > > > > > Cc: Simon Glass sjg@chromium.org > > > > > > > Cc: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com > > > > > > > Cc: Tom Rini trini@konsulko.com > > > > > > > --- > > > > > > > include/lmb.h | 4 ++-- > > > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > diff --git a/include/lmb.h b/include/lmb.h > > > > > > > index 3c4afdf9f0..fa1474a360 100644 > > > > > > > --- a/include/lmb.h > > > > > > > +++ b/include/lmb.h > > > > > > > @@ -44,7 +44,7 @@ struct lmb_property { > > > > > > > struct lmb_region { > > > > > > > unsigned long cnt; > > > > > > > unsigned long max; > > > > > > > -#if IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) > > > > > > > +#if IS_ENABLED(CONFIG_LMB) && IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) > > > > > > > > This doesn't make sense to me, still. You cannot enable > > > > CONFIG_LMB_USE_MAX_REGIONS without CONFIG_LMB as the former depends on > > > > the latter in Kconfig. > > > > > > > > > > > struct lmb_property region[CONFIG_LMB_MAX_REGIONS]; > > > > > > > #else > > > > > > > struct lmb_property *region; > > > > > > > @@ -67,7 +67,7 @@ struct lmb_region { > > > > > > > struct lmb { > > > > > > > struct lmb_region memory; > > > > > > > struct lmb_region reserved; > > > > > > > -#if !IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) > > > > > > > +#if IS_ENABLED(CONFIG_LMB) && !IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) > > > > > > > struct lmb_property memory_regions[CONFIG_LMB_MEMORY_REGIONS]; > > > > > > > struct lmb_property reserved_regions[CONFIG_LMB_RESERVED_REGIONS]; > > > > > > > #endif > > > > > > > > > > > > We shouldn't need this at all. LMB and LMB_USE_MAX_REGIONS are both in > > > > > > Kconfig and have the dependencies expressed that way. > > > > > > > > > > However, CONFIG_LMB_MEMORY_REGIONS and CONFIG_LMB_RESERVED_REGIONS may be > > > > > undefined if CONFIG_LMB and !CONFIG_LMB_USE_MAX_REGIONS . They are four > > > > > different symbols. > > > > > > > > I'm still not seeing it, sorry. Is there some case where we're trying > > > > to access a struct lmb without CONFIG_LMB enabled? > > > > > > > > > > See build failure > > > https://source.denx.de/u-boot/custodians/u-boot-sh/-/jobs/315331 > > > > Ah, progress. Drop <lmb.h> from <image.h> since we already have a > > forward declaration of struct lmb? But it's not failing without this > > series too, so what's changing? > > See 01/14 in this series.
Ah, so drop 1/14 then.
Why ? That patch is correct.
It's not quite right, 1/14 and then 2/14 are papering over the fact that lmb.h, and it's including headers / files, need to be cleaned up so that we don't need to have redundant tests in the header.
1/14 disables LMB and CMD_BDI for tools build, we do not need those, so 1/14 is correct.
We don't need to build u-boot at all for tools-only, only the tools-only build target. It's just annoying to exclude the tools-only_defconfig from "sandbox" in CI.
So, what exactly is the problem with that 01/14 ? Please elaborate, I believe the patch is correct.
You disable LMB in a target that's only building "all" in CI because wasn't ever worth adding ",sandbox" to the all other arches job until perhaps now.
Disabling LMB in tools-only_defconfig then exposes that <lmb.h> can only be included safely when CONFIG_LMB is set.
Adding / extending an #if test in code for something that's already checked for in Kconfig is bad. We spent so much time already removing and shrinking #if tests in the code.
What kind of cleanup of lmb.h do you have in mind ?
Remove it from include/image.h and fix any fall-out from that of files that got <lmb.h> indirectly when they needed it directly instead.
Uh ... that is likely for a separate series, and a big one.
Honestly, checking again, I'm not sure LMB=n is valid, ever. That's how we keep our running U-Boot from being trivially overwritten and a huge number of security issues from being re-opened.
At this point, I think you should rework things to stop making CONFIG_LMB be optional, it should be a def_bool y.
On 8/30/21 1:11 AM, Tom Rini wrote:
On Mon, Aug 30, 2021 at 01:00:02AM +0200, Marek Vasut wrote:
On 8/30/21 12:51 AM, Tom Rini wrote:
On Mon, Aug 30, 2021 at 12:40:07AM +0200, Marek Vasut wrote:
On 8/30/21 12:23 AM, Tom Rini wrote:
On Mon, Aug 30, 2021 at 12:19:59AM +0200, Marek Vasut wrote:
On 8/30/21 12:10 AM, Tom Rini wrote: > On Sun, Aug 29, 2021 at 11:47:58PM +0200, Marek Vasut wrote: >> On 8/29/21 9:32 PM, Tom Rini wrote: >>> On Sun, Aug 29, 2021 at 09:24:46PM +0200, Marek Vasut wrote: >>>> On 8/29/21 8:02 PM, Tom Rini wrote: >>>>> On Sun, Aug 29, 2021 at 06:26:23PM +0200, Marek Vasut wrote: >>>>>> On 8/15/21 9:47 PM, Tom Rini wrote: >>>>>>> On Sun, Aug 15, 2021 at 08:13:02PM +0200, Marek Vasut wrote: >>>>>>> >>>>>>>> The CONFIG_LMB_*_REGIONS are defined only if CONFIG_LMB is enabled, >>>>>>>> protect access to those two config options to avoid undefined macro >>>>>>>> errors. >>>>>>>> >>>>>>>> Signed-off-by: Marek Vasut marek.vasut+renesas@gmail.com >>>>>>>> Cc: Simon Glass sjg@chromium.org >>>>>>>> Cc: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com >>>>>>>> Cc: Tom Rini trini@konsulko.com >>>>>>>> --- >>>>>>>> include/lmb.h | 4 ++-- >>>>>>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>>>>>> >>>>>>>> diff --git a/include/lmb.h b/include/lmb.h >>>>>>>> index 3c4afdf9f0..fa1474a360 100644 >>>>>>>> --- a/include/lmb.h >>>>>>>> +++ b/include/lmb.h >>>>>>>> @@ -44,7 +44,7 @@ struct lmb_property { >>>>>>>> struct lmb_region { >>>>>>>> unsigned long cnt; >>>>>>>> unsigned long max; >>>>>>>> -#if IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) >>>>>>>> +#if IS_ENABLED(CONFIG_LMB) && IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) >>>>> >>>>> This doesn't make sense to me, still. You cannot enable >>>>> CONFIG_LMB_USE_MAX_REGIONS without CONFIG_LMB as the former depends on >>>>> the latter in Kconfig. >>>>> >>>>>>>> struct lmb_property region[CONFIG_LMB_MAX_REGIONS]; >>>>>>>> #else >>>>>>>> struct lmb_property *region; >>>>>>>> @@ -67,7 +67,7 @@ struct lmb_region { >>>>>>>> struct lmb { >>>>>>>> struct lmb_region memory; >>>>>>>> struct lmb_region reserved; >>>>>>>> -#if !IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) >>>>>>>> +#if IS_ENABLED(CONFIG_LMB) && !IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) >>>>>>>> struct lmb_property memory_regions[CONFIG_LMB_MEMORY_REGIONS]; >>>>>>>> struct lmb_property reserved_regions[CONFIG_LMB_RESERVED_REGIONS]; >>>>>>>> #endif >>>>>>> >>>>>>> We shouldn't need this at all. LMB and LMB_USE_MAX_REGIONS are both in >>>>>>> Kconfig and have the dependencies expressed that way. >>>>>> >>>>>> However, CONFIG_LMB_MEMORY_REGIONS and CONFIG_LMB_RESERVED_REGIONS may be >>>>>> undefined if CONFIG_LMB and !CONFIG_LMB_USE_MAX_REGIONS . They are four >>>>>> different symbols. >>>>> >>>>> I'm still not seeing it, sorry. Is there some case where we're trying >>>>> to access a struct lmb without CONFIG_LMB enabled? >>>>> >>>> >>>> See build failure >>>> https://source.denx.de/u-boot/custodians/u-boot-sh/-/jobs/315331 >>> >>> Ah, progress. Drop <lmb.h> from <image.h> since we already have a >>> forward declaration of struct lmb? But it's not failing without this >>> series too, so what's changing? >> >> See 01/14 in this series. > > Ah, so drop 1/14 then.
Why ? That patch is correct.
It's not quite right, 1/14 and then 2/14 are papering over the fact that lmb.h, and it's including headers / files, need to be cleaned up so that we don't need to have redundant tests in the header.
1/14 disables LMB and CMD_BDI for tools build, we do not need those, so 1/14 is correct.
We don't need to build u-boot at all for tools-only, only the tools-only build target. It's just annoying to exclude the tools-only_defconfig from "sandbox" in CI.
So, what exactly is the problem with that 01/14 ? Please elaborate, I believe the patch is correct.
You disable LMB in a target that's only building "all" in CI because wasn't ever worth adding ",sandbox" to the all other arches job until perhaps now.
Disabling LMB in tools-only_defconfig then exposes that <lmb.h> can only be included safely when CONFIG_LMB is set.
Adding / extending an #if test in code for something that's already checked for in Kconfig is bad. We spent so much time already removing and shrinking #if tests in the code.
So, the patch is correct, the headers need further clean up.
What kind of cleanup of lmb.h do you have in mind ?
Remove it from include/image.h and fix any fall-out from that of files that got <lmb.h> indirectly when they needed it directly instead.
Uh ... that is likely for a separate series, and a big one.
Honestly, checking again, I'm not sure LMB=n is valid, ever.
Why wouldn't it be ? For tools, LMB=n is perfectly valid.
That's how we keep our running U-Boot from being trivially overwritten and a huge number of security issues from being re-opened.
Tools are not running U-Boot.
At this point, I think you should rework things to stop making CONFIG_LMB be optional, it should be a def_bool y.
I disagree, see above.
On Mon, Aug 30, 2021 at 11:45:02AM +0200, Marek Vasut wrote:
On 8/30/21 1:11 AM, Tom Rini wrote:
On Mon, Aug 30, 2021 at 01:00:02AM +0200, Marek Vasut wrote:
On 8/30/21 12:51 AM, Tom Rini wrote:
On Mon, Aug 30, 2021 at 12:40:07AM +0200, Marek Vasut wrote:
On 8/30/21 12:23 AM, Tom Rini wrote:
On Mon, Aug 30, 2021 at 12:19:59AM +0200, Marek Vasut wrote: > On 8/30/21 12:10 AM, Tom Rini wrote: > > On Sun, Aug 29, 2021 at 11:47:58PM +0200, Marek Vasut wrote: > > > On 8/29/21 9:32 PM, Tom Rini wrote: > > > > On Sun, Aug 29, 2021 at 09:24:46PM +0200, Marek Vasut wrote: > > > > > On 8/29/21 8:02 PM, Tom Rini wrote: > > > > > > On Sun, Aug 29, 2021 at 06:26:23PM +0200, Marek Vasut wrote: > > > > > > > On 8/15/21 9:47 PM, Tom Rini wrote: > > > > > > > > On Sun, Aug 15, 2021 at 08:13:02PM +0200, Marek Vasut wrote: > > > > > > > > > > > > > > > > > The CONFIG_LMB_*_REGIONS are defined only if CONFIG_LMB is enabled, > > > > > > > > > protect access to those two config options to avoid undefined macro > > > > > > > > > errors. > > > > > > > > > > > > > > > > > > Signed-off-by: Marek Vasut marek.vasut+renesas@gmail.com > > > > > > > > > Cc: Simon Glass sjg@chromium.org > > > > > > > > > Cc: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com > > > > > > > > > Cc: Tom Rini trini@konsulko.com > > > > > > > > > --- > > > > > > > > > include/lmb.h | 4 ++-- > > > > > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > > > > > diff --git a/include/lmb.h b/include/lmb.h > > > > > > > > > index 3c4afdf9f0..fa1474a360 100644 > > > > > > > > > --- a/include/lmb.h > > > > > > > > > +++ b/include/lmb.h > > > > > > > > > @@ -44,7 +44,7 @@ struct lmb_property { > > > > > > > > > struct lmb_region { > > > > > > > > > unsigned long cnt; > > > > > > > > > unsigned long max; > > > > > > > > > -#if IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) > > > > > > > > > +#if IS_ENABLED(CONFIG_LMB) && IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) > > > > > > > > > > > > This doesn't make sense to me, still. You cannot enable > > > > > > CONFIG_LMB_USE_MAX_REGIONS without CONFIG_LMB as the former depends on > > > > > > the latter in Kconfig. > > > > > > > > > > > > > > > struct lmb_property region[CONFIG_LMB_MAX_REGIONS]; > > > > > > > > > #else > > > > > > > > > struct lmb_property *region; > > > > > > > > > @@ -67,7 +67,7 @@ struct lmb_region { > > > > > > > > > struct lmb { > > > > > > > > > struct lmb_region memory; > > > > > > > > > struct lmb_region reserved; > > > > > > > > > -#if !IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) > > > > > > > > > +#if IS_ENABLED(CONFIG_LMB) && !IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) > > > > > > > > > struct lmb_property memory_regions[CONFIG_LMB_MEMORY_REGIONS]; > > > > > > > > > struct lmb_property reserved_regions[CONFIG_LMB_RESERVED_REGIONS]; > > > > > > > > > #endif > > > > > > > > > > > > > > > > We shouldn't need this at all. LMB and LMB_USE_MAX_REGIONS are both in > > > > > > > > Kconfig and have the dependencies expressed that way. > > > > > > > > > > > > > > However, CONFIG_LMB_MEMORY_REGIONS and CONFIG_LMB_RESERVED_REGIONS may be > > > > > > > undefined if CONFIG_LMB and !CONFIG_LMB_USE_MAX_REGIONS . They are four > > > > > > > different symbols. > > > > > > > > > > > > I'm still not seeing it, sorry. Is there some case where we're trying > > > > > > to access a struct lmb without CONFIG_LMB enabled? > > > > > > > > > > > > > > > > See build failure > > > > > https://source.denx.de/u-boot/custodians/u-boot-sh/-/jobs/315331 > > > > > > > > Ah, progress. Drop <lmb.h> from <image.h> since we already have a > > > > forward declaration of struct lmb? But it's not failing without this > > > > series too, so what's changing? > > > > > > See 01/14 in this series. > > > > Ah, so drop 1/14 then. > > Why ? That patch is correct.
It's not quite right, 1/14 and then 2/14 are papering over the fact that lmb.h, and it's including headers / files, need to be cleaned up so that we don't need to have redundant tests in the header.
1/14 disables LMB and CMD_BDI for tools build, we do not need those, so 1/14 is correct.
We don't need to build u-boot at all for tools-only, only the tools-only build target. It's just annoying to exclude the tools-only_defconfig from "sandbox" in CI.
So, what exactly is the problem with that 01/14 ? Please elaborate, I believe the patch is correct.
You disable LMB in a target that's only building "all" in CI because wasn't ever worth adding ",sandbox" to the all other arches job until perhaps now.
Disabling LMB in tools-only_defconfig then exposes that <lmb.h> can only be included safely when CONFIG_LMB is set.
Adding / extending an #if test in code for something that's already checked for in Kconfig is bad. We spent so much time already removing and shrinking #if tests in the code.
So, the patch is correct, the headers need further clean up.
No, it's not. The first patch is wrong because disabling CONFIG_LMB is invalid. The second patch is conceptually wrong because if we're enforcing a check in C for a dependency that's enforced in Kconfig, we have another problem to investigate. Which I did, LMB is non-optional.
What kind of cleanup of lmb.h do you have in mind ?
Remove it from include/image.h and fix any fall-out from that of files that got <lmb.h> indirectly when they needed it directly instead.
Uh ... that is likely for a separate series, and a big one.
Honestly, checking again, I'm not sure LMB=n is valid, ever.
Why wouldn't it be ? For tools, LMB=n is perfectly valid.
Because it's never valid to disable LMB, LMB is what protects the running U-Boot.
It's nonsense to build u-boot on tools-only_defconfig but we don't have a way currently to remove "u-boot" from the all target. Maybe once a few more of the hard/tricky CONFIG symbols get migrated to Kconfig we can then set tools-only_defconfig to NOT build u-boot at all.
That's how we keep our running U-Boot from being trivially overwritten and a huge number of security issues from being re-opened.
Tools are not running U-Boot.
At this point, I think you should rework things to stop making CONFIG_LMB be optional, it should be a def_bool y.
I disagree, see above.
The only reason "tools-only_defconfig" builds a useless u-boot binary today is in CI where it would be more work than it's worth to make CI exclude that from the build list. But if you want to just do that instead, I'll also accept adding -x tools-only to the azure/gitlab jobs that build all other architectures, as tools-only is tested in its own build job, for it's only valid build target.
On 8/30/21 2:01 PM, Tom Rini wrote:
[...]
>>>>>>>>> We shouldn't need this at all. LMB and LMB_USE_MAX_REGIONS are both in >>>>>>>>> Kconfig and have the dependencies expressed that way. >>>>>>>> >>>>>>>> However, CONFIG_LMB_MEMORY_REGIONS and CONFIG_LMB_RESERVED_REGIONS may be >>>>>>>> undefined if CONFIG_LMB and !CONFIG_LMB_USE_MAX_REGIONS . They are four >>>>>>>> different symbols. >>>>>>> >>>>>>> I'm still not seeing it, sorry. Is there some case where we're trying >>>>>>> to access a struct lmb without CONFIG_LMB enabled? >>>>>>> >>>>>> >>>>>> See build failure >>>>>> https://source.denx.de/u-boot/custodians/u-boot-sh/-/jobs/315331 >>>>> >>>>> Ah, progress. Drop <lmb.h> from <image.h> since we already have a >>>>> forward declaration of struct lmb? But it's not failing without this >>>>> series too, so what's changing? >>>> >>>> See 01/14 in this series. >>> >>> Ah, so drop 1/14 then. >> >> Why ? That patch is correct. > > It's not quite right, 1/14 and then 2/14 are papering over the fact that > lmb.h, and it's including headers / files, need to be cleaned up so that > we don't need to have redundant tests in the header.
1/14 disables LMB and CMD_BDI for tools build, we do not need those, so 1/14 is correct.
We don't need to build u-boot at all for tools-only, only the tools-only build target. It's just annoying to exclude the tools-only_defconfig from "sandbox" in CI.
So, what exactly is the problem with that 01/14 ? Please elaborate, I believe the patch is correct.
You disable LMB in a target that's only building "all" in CI because wasn't ever worth adding ",sandbox" to the all other arches job until perhaps now.
Disabling LMB in tools-only_defconfig then exposes that <lmb.h> can only be included safely when CONFIG_LMB is set.
Adding / extending an #if test in code for something that's already checked for in Kconfig is bad. We spent so much time already removing and shrinking #if tests in the code.
So, the patch is correct, the headers need further clean up.
No, it's not. The first patch is wrong because disabling CONFIG_LMB is invalid.
Please explain why the patch disabling LMB support for tools-only build is invalid. I disagree with this statement, LMB support in tools-only build is useless, because LMB protects parts of running U-Boot from being overwritten.
The second patch is conceptually wrong because if we're enforcing a check in C for a dependency that's enforced in Kconfig, we have another problem to investigate. Which I did, LMB is non-optional.
Please explain why is LMB non-optional ? I disagree. LMB for tools-only build is useless, hence it should not be enabled.
What kind of cleanup of lmb.h do you have in mind ?
Remove it from include/image.h and fix any fall-out from that of files that got <lmb.h> indirectly when they needed it directly instead.
Uh ... that is likely for a separate series, and a big one.
Honestly, checking again, I'm not sure LMB=n is valid, ever.
Why wouldn't it be ? For tools, LMB=n is perfectly valid.
Because it's never valid to disable LMB, LMB is what protects the running U-Boot.
We are talking about tools-only build here, not running U-Boot.
It's nonsense to build u-boot on tools-only_defconfig but we don't have a way currently to remove "u-boot" from the all target. Maybe once a few more of the hard/tricky CONFIG symbols get migrated to Kconfig we can then set tools-only_defconfig to NOT build u-boot at all.
That's how we keep our running U-Boot from being trivially overwritten and a huge number of security issues from being re-opened.
Tools are not running U-Boot.
At this point, I think you should rework things to stop making CONFIG_LMB be optional, it should be a def_bool y.
I disagree, see above.
The only reason "tools-only_defconfig" builds a useless u-boot binary today is in CI where it would be more work than it's worth to make CI exclude that from the build list. But if you want to just do that instead, I'll also accept adding -x tools-only to the azure/gitlab jobs that build all other architectures, as tools-only is tested in its own build job, for it's only valid build target.
The tools-only build is also used elsewhere, to build just that, tools.
[...]
On Sat, Sep 04, 2021 at 04:03:25PM +0200, Marek Vasut wrote:
On 8/30/21 2:01 PM, Tom Rini wrote:
[...]
> > > > > > > > > > We shouldn't need this at all. LMB and LMB_USE_MAX_REGIONS are both in > > > > > > > > > > Kconfig and have the dependencies expressed that way. > > > > > > > > > > > > > > > > > > However, CONFIG_LMB_MEMORY_REGIONS and CONFIG_LMB_RESERVED_REGIONS may be > > > > > > > > > undefined if CONFIG_LMB and !CONFIG_LMB_USE_MAX_REGIONS . They are four > > > > > > > > > different symbols. > > > > > > > > > > > > > > > > I'm still not seeing it, sorry. Is there some case where we're trying > > > > > > > > to access a struct lmb without CONFIG_LMB enabled? > > > > > > > > > > > > > > > > > > > > > > See build failure > > > > > > > https://source.denx.de/u-boot/custodians/u-boot-sh/-/jobs/315331 > > > > > > > > > > > > Ah, progress. Drop <lmb.h> from <image.h> since we already have a > > > > > > forward declaration of struct lmb? But it's not failing without this > > > > > > series too, so what's changing? > > > > > > > > > > See 01/14 in this series. > > > > > > > > Ah, so drop 1/14 then. > > > > > > Why ? That patch is correct. > > > > It's not quite right, 1/14 and then 2/14 are papering over the fact that > > lmb.h, and it's including headers / files, need to be cleaned up so that > > we don't need to have redundant tests in the header. > > 1/14 disables LMB and CMD_BDI for tools build, we do not need those, so 1/14 > is correct.
We don't need to build u-boot at all for tools-only, only the tools-only build target. It's just annoying to exclude the tools-only_defconfig from "sandbox" in CI.
So, what exactly is the problem with that 01/14 ? Please elaborate, I believe the patch is correct.
You disable LMB in a target that's only building "all" in CI because wasn't ever worth adding ",sandbox" to the all other arches job until perhaps now.
Disabling LMB in tools-only_defconfig then exposes that <lmb.h> can only be included safely when CONFIG_LMB is set.
Adding / extending an #if test in code for something that's already checked for in Kconfig is bad. We spent so much time already removing and shrinking #if tests in the code.
So, the patch is correct, the headers need further clean up.
No, it's not. The first patch is wrong because disabling CONFIG_LMB is invalid.
Please explain why the patch disabling LMB support for tools-only build is invalid. I disagree with this statement, LMB support in tools-only build is useless, because LMB protects parts of running U-Boot from being overwritten.
The second patch is conceptually wrong because if we're enforcing a check in C for a dependency that's enforced in Kconfig, we have another problem to investigate. Which I did, LMB is non-optional.
Please explain why is LMB non-optional ? I disagree. LMB for tools-only build is useless, hence it should not be enabled.
> What kind of cleanup of lmb.h do you have in mind ?
Remove it from include/image.h and fix any fall-out from that of files that got <lmb.h> indirectly when they needed it directly instead.
Uh ... that is likely for a separate series, and a big one.
Honestly, checking again, I'm not sure LMB=n is valid, ever.
Why wouldn't it be ? For tools, LMB=n is perfectly valid.
Because it's never valid to disable LMB, LMB is what protects the running U-Boot.
We are talking about tools-only build here, not running U-Boot.
It's nonsense to build u-boot on tools-only_defconfig but we don't have a way currently to remove "u-boot" from the all target. Maybe once a few more of the hard/tricky CONFIG symbols get migrated to Kconfig we can then set tools-only_defconfig to NOT build u-boot at all.
That's how we keep our running U-Boot from being trivially overwritten and a huge number of security issues from being re-opened.
Tools are not running U-Boot.
At this point, I think you should rework things to stop making CONFIG_LMB be optional, it should be a def_bool y.
I disagree, see above.
The only reason "tools-only_defconfig" builds a useless u-boot binary today is in CI where it would be more work than it's worth to make CI exclude that from the build list. But if you want to just do that instead, I'll also accept adding -x tools-only to the azure/gitlab jobs that build all other architectures, as tools-only is tested in its own build job, for it's only valid build target.
The tools-only build is also used elsewhere, to build just that, tools.
I've repeatedly explained myself and what I'm looking for in v2 of this series. I will summarize one last time. The "tools-only_defconfig" is for tools, only. Building anything other than the "tools-only" target isn't useful. In U-Boot itself, LMB is required as that is how we prevent a number of CVEs from being trivial to exploit. v2 of this series needs to drop patches 1 and 2 of v1 of this series. It can further do any of: 1. Nothing else. 2. Add tools-only to the exclude list in the "build everything else" CI job. 3. Make CONFIG_LMB be def_bool y.
On 9/4/21 4:10 PM, Tom Rini wrote: [...]
At this point, I think you should rework things to stop making CONFIG_LMB be optional, it should be a def_bool y.
I disagree, see above.
The only reason "tools-only_defconfig" builds a useless u-boot binary today is in CI where it would be more work than it's worth to make CI exclude that from the build list. But if you want to just do that instead, I'll also accept adding -x tools-only to the azure/gitlab jobs that build all other architectures, as tools-only is tested in its own build job, for it's only valid build target.
The tools-only build is also used elsewhere, to build just that, tools.
I've repeatedly explained myself and what I'm looking for in v2 of this series. I will summarize one last time. The "tools-only_defconfig" is for tools, only. Building anything other than the "tools-only" target isn't useful. In U-Boot itself, LMB is required as that is how we prevent a number of CVEs from being trivial to exploit. v2 of this series needs to drop patches 1 and 2 of v1 of this series. It can further do any of:
- Nothing else.
- Add tools-only to the exclude list in the "build everything else" CI job.
- Make CONFIG_LMB be def_bool y.
If tools-only is for tools, only, then why should it enable LMB ? The tools are userspace tools, they do not need LMB, and so LMB can be disabled.
This is the part which is unclear to me.
On Sat, Sep 04, 2021 at 05:15:45PM +0200, Marek Vasut wrote:
On 9/4/21 4:10 PM, Tom Rini wrote: [...]
At this point, I think you should rework things to stop making CONFIG_LMB be optional, it should be a def_bool y.
I disagree, see above.
The only reason "tools-only_defconfig" builds a useless u-boot binary today is in CI where it would be more work than it's worth to make CI exclude that from the build list. But if you want to just do that instead, I'll also accept adding -x tools-only to the azure/gitlab jobs that build all other architectures, as tools-only is tested in its own build job, for it's only valid build target.
The tools-only build is also used elsewhere, to build just that, tools.
I've repeatedly explained myself and what I'm looking for in v2 of this series. I will summarize one last time. The "tools-only_defconfig" is for tools, only. Building anything other than the "tools-only" target isn't useful. In U-Boot itself, LMB is required as that is how we prevent a number of CVEs from being trivial to exploit. v2 of this series needs to drop patches 1 and 2 of v1 of this series. It can further do any of:
- Nothing else.
- Add tools-only to the exclude list in the "build everything else" CI job.
- Make CONFIG_LMB be def_bool y.
If tools-only is for tools, only, then why should it enable LMB ? The tools are userspace tools, they do not need LMB, and so LMB can be disabled.
This is the part which is unclear to me.
I don't know why it's unclear to you at this point, sorry.
On 9/4/21 5:17 PM, Tom Rini wrote:
On Sat, Sep 04, 2021 at 05:15:45PM +0200, Marek Vasut wrote:
On 9/4/21 4:10 PM, Tom Rini wrote: [...]
> At this point, I think you should rework things to stop making > CONFIG_LMB be optional, it should be a def_bool y.
I disagree, see above.
The only reason "tools-only_defconfig" builds a useless u-boot binary today is in CI where it would be more work than it's worth to make CI exclude that from the build list. But if you want to just do that instead, I'll also accept adding -x tools-only to the azure/gitlab jobs that build all other architectures, as tools-only is tested in its own build job, for it's only valid build target.
The tools-only build is also used elsewhere, to build just that, tools.
I've repeatedly explained myself and what I'm looking for in v2 of this series. I will summarize one last time. The "tools-only_defconfig" is for tools, only. Building anything other than the "tools-only" target isn't useful. In U-Boot itself, LMB is required as that is how we prevent a number of CVEs from being trivial to exploit. v2 of this series needs to drop patches 1 and 2 of v1 of this series. It can further do any of:
- Nothing else.
- Add tools-only to the exclude list in the "build everything else" CI job.
- Make CONFIG_LMB be def_bool y.
If tools-only is for tools, only, then why should it enable LMB ? The tools are userspace tools, they do not need LMB, and so LMB can be disabled.
This is the part which is unclear to me.
I don't know why it's unclear to you at this point, sorry.
Well why exactly does a userspace program require LMB enabled ? What does LMB protect in there ? obviously not U-Boot.
On Sat, Sep 04, 2021 at 06:05:50PM +0200, Marek Vasut wrote:
On 9/4/21 5:17 PM, Tom Rini wrote:
On Sat, Sep 04, 2021 at 05:15:45PM +0200, Marek Vasut wrote:
On 9/4/21 4:10 PM, Tom Rini wrote: [...]
> > At this point, I think you should rework things to stop making > > CONFIG_LMB be optional, it should be a def_bool y. > > I disagree, see above.
The only reason "tools-only_defconfig" builds a useless u-boot binary today is in CI where it would be more work than it's worth to make CI exclude that from the build list. But if you want to just do that instead, I'll also accept adding -x tools-only to the azure/gitlab jobs that build all other architectures, as tools-only is tested in its own build job, for it's only valid build target.
The tools-only build is also used elsewhere, to build just that, tools.
I've repeatedly explained myself and what I'm looking for in v2 of this series. I will summarize one last time. The "tools-only_defconfig" is for tools, only. Building anything other than the "tools-only" target isn't useful. In U-Boot itself, LMB is required as that is how we prevent a number of CVEs from being trivial to exploit. v2 of this series needs to drop patches 1 and 2 of v1 of this series. It can further do any of:
- Nothing else.
- Add tools-only to the exclude list in the "build everything else" CI job.
- Make CONFIG_LMB be def_bool y.
If tools-only is for tools, only, then why should it enable LMB ? The tools are userspace tools, they do not need LMB, and so LMB can be disabled.
This is the part which is unclear to me.
I don't know why it's unclear to you at this point, sorry.
Well why exactly does a userspace program require LMB enabled ? What does LMB protect in there ? obviously not U-Boot.
I feel like you've lost the thread.
On 9/4/21 6:09 PM, Tom Rini wrote:
On Sat, Sep 04, 2021 at 06:05:50PM +0200, Marek Vasut wrote:
On 9/4/21 5:17 PM, Tom Rini wrote:
On Sat, Sep 04, 2021 at 05:15:45PM +0200, Marek Vasut wrote:
On 9/4/21 4:10 PM, Tom Rini wrote: [...]
>>> At this point, I think you should rework things to stop making >>> CONFIG_LMB be optional, it should be a def_bool y. >> >> I disagree, see above. > > The only reason "tools-only_defconfig" builds a useless u-boot binary > today is in CI where it would be more work than it's worth to make CI > exclude that from the build list. But if you want to just do that > instead, I'll also accept adding -x tools-only to the azure/gitlab jobs > that build all other architectures, as tools-only is tested in its own > build job, for it's only valid build target.
The tools-only build is also used elsewhere, to build just that, tools.
I've repeatedly explained myself and what I'm looking for in v2 of this series. I will summarize one last time. The "tools-only_defconfig" is for tools, only. Building anything other than the "tools-only" target isn't useful. In U-Boot itself, LMB is required as that is how we prevent a number of CVEs from being trivial to exploit. v2 of this series needs to drop patches 1 and 2 of v1 of this series. It can further do any of:
- Nothing else.
- Add tools-only to the exclude list in the "build everything else" CI job.
- Make CONFIG_LMB be def_bool y.
If tools-only is for tools, only, then why should it enable LMB ? The tools are userspace tools, they do not need LMB, and so LMB can be disabled.
This is the part which is unclear to me.
I don't know why it's unclear to you at this point, sorry.
Well why exactly does a userspace program require LMB enabled ? What does LMB protect in there ? obviously not U-Boot.
I feel like you've lost the thread.
Can you please answer my questions above ?
This is exactly what patch 1/14 is about -- disable LMB for tools build, because it is useless for tools build.
[trimming the CC list]
On Sat, Sep 04, 2021 at 06:49:03PM +0200, Marek Vasut wrote:
On 9/4/21 6:09 PM, Tom Rini wrote:
On Sat, Sep 04, 2021 at 06:05:50PM +0200, Marek Vasut wrote:
On 9/4/21 5:17 PM, Tom Rini wrote:
On Sat, Sep 04, 2021 at 05:15:45PM +0200, Marek Vasut wrote:
On 9/4/21 4:10 PM, Tom Rini wrote: [...]
> > > > At this point, I think you should rework things to stop making > > > > CONFIG_LMB be optional, it should be a def_bool y. > > > > > > I disagree, see above. > > > > The only reason "tools-only_defconfig" builds a useless u-boot binary > > today is in CI where it would be more work than it's worth to make CI > > exclude that from the build list. But if you want to just do that > > instead, I'll also accept adding -x tools-only to the azure/gitlab jobs > > that build all other architectures, as tools-only is tested in its own > > build job, for it's only valid build target. > > The tools-only build is also used elsewhere, to build just that, tools.
I've repeatedly explained myself and what I'm looking for in v2 of this series. I will summarize one last time. The "tools-only_defconfig" is for tools, only. Building anything other than the "tools-only" target isn't useful. In U-Boot itself, LMB is required as that is how we prevent a number of CVEs from being trivial to exploit. v2 of this series needs to drop patches 1 and 2 of v1 of this series. It can further do any of:
- Nothing else.
- Add tools-only to the exclude list in the "build everything else" CI job.
- Make CONFIG_LMB be def_bool y.
If tools-only is for tools, only, then why should it enable LMB ? The tools are userspace tools, they do not need LMB, and so LMB can be disabled.
This is the part which is unclear to me.
I don't know why it's unclear to you at this point, sorry.
Well why exactly does a userspace program require LMB enabled ? What does LMB protect in there ? obviously not U-Boot.
I feel like you've lost the thread.
Can you please answer my questions above ?
I have.
On 9/4/21 7:01 PM, Tom Rini wrote:
[trimming the CC list]
On Sat, Sep 04, 2021 at 06:49:03PM +0200, Marek Vasut wrote:
On 9/4/21 6:09 PM, Tom Rini wrote:
On Sat, Sep 04, 2021 at 06:05:50PM +0200, Marek Vasut wrote:
On 9/4/21 5:17 PM, Tom Rini wrote:
On Sat, Sep 04, 2021 at 05:15:45PM +0200, Marek Vasut wrote:
On 9/4/21 4:10 PM, Tom Rini wrote: [...]
>>>>> At this point, I think you should rework things to stop making >>>>> CONFIG_LMB be optional, it should be a def_bool y. >>>> >>>> I disagree, see above. >>> >>> The only reason "tools-only_defconfig" builds a useless u-boot binary >>> today is in CI where it would be more work than it's worth to make CI >>> exclude that from the build list. But if you want to just do that >>> instead, I'll also accept adding -x tools-only to the azure/gitlab jobs >>> that build all other architectures, as tools-only is tested in its own >>> build job, for it's only valid build target. >> >> The tools-only build is also used elsewhere, to build just that, tools. > > I've repeatedly explained myself and what I'm looking for in v2 of this > series. I will summarize one last time. The "tools-only_defconfig" is > for tools, only. Building anything other than the "tools-only" target > isn't useful. In U-Boot itself, LMB is required as that is how we > prevent a number of CVEs from being trivial to exploit. v2 of this > series needs to drop patches 1 and 2 of v1 of this series. It can > further do any of: > 1. Nothing else. > 2. Add tools-only to the exclude list in the "build everything else" CI > job. > 3. Make CONFIG_LMB be def_bool y.
If tools-only is for tools, only, then why should it enable LMB ? The tools are userspace tools, they do not need LMB, and so LMB can be disabled.
This is the part which is unclear to me.
I don't know why it's unclear to you at this point, sorry.
Well why exactly does a userspace program require LMB enabled ? What does LMB protect in there ? obviously not U-Boot.
I feel like you've lost the thread.
Can you please answer my questions above ?
I have.
This attitude is not helpful. Please answer my questions, if necessary please reiterate, otherwise this discussion cannot be resolved and will only lead to frustration.
On Sat, Sep 04, 2021 at 09:37:39PM +0200, Marek Vasut wrote:
On 9/4/21 7:01 PM, Tom Rini wrote:
[trimming the CC list]
On Sat, Sep 04, 2021 at 06:49:03PM +0200, Marek Vasut wrote:
On 9/4/21 6:09 PM, Tom Rini wrote:
On Sat, Sep 04, 2021 at 06:05:50PM +0200, Marek Vasut wrote:
On 9/4/21 5:17 PM, Tom Rini wrote:
On Sat, Sep 04, 2021 at 05:15:45PM +0200, Marek Vasut wrote: > On 9/4/21 4:10 PM, Tom Rini wrote: > [...] > > > > > > > At this point, I think you should rework things to stop making > > > > > > CONFIG_LMB be optional, it should be a def_bool y. > > > > > > > > > > I disagree, see above. > > > > > > > > The only reason "tools-only_defconfig" builds a useless u-boot binary > > > > today is in CI where it would be more work than it's worth to make CI > > > > exclude that from the build list. But if you want to just do that > > > > instead, I'll also accept adding -x tools-only to the azure/gitlab jobs > > > > that build all other architectures, as tools-only is tested in its own > > > > build job, for it's only valid build target. > > > > > > The tools-only build is also used elsewhere, to build just that, tools. > > > > I've repeatedly explained myself and what I'm looking for in v2 of this > > series. I will summarize one last time. The "tools-only_defconfig" is > > for tools, only. Building anything other than the "tools-only" target > > isn't useful. In U-Boot itself, LMB is required as that is how we > > prevent a number of CVEs from being trivial to exploit. v2 of this > > series needs to drop patches 1 and 2 of v1 of this series. It can > > further do any of: > > 1. Nothing else. > > 2. Add tools-only to the exclude list in the "build everything else" CI > > job. > > 3. Make CONFIG_LMB be def_bool y. > > If tools-only is for tools, only, then why should it enable LMB ? > The tools are userspace tools, they do not need LMB, and so LMB can be > disabled. > > This is the part which is unclear to me.
I don't know why it's unclear to you at this point, sorry.
Well why exactly does a userspace program require LMB enabled ? What does LMB protect in there ? obviously not U-Boot.
I feel like you've lost the thread.
Can you please answer my questions above ?
I have.
This attitude is not helpful. Please answer my questions, if necessary please reiterate, otherwise this discussion cannot be resolved and will only lead to frustration.
One last time then. The only reason tools-only_defconfig is ever built for a target other than "tools-only" is because CI does not exclude it from the world build stage. You can fix this by doing option #2 still quoted above.
The only CONFIG options that are at all valid for "tools-only" and so the host tools related, are LOCALVERSION (which is why there's a tools-only defconfig at all) and now TOOLS_LIBCRYPTO. Nothing else at all should matter as the tools should always be the same. So your point about "what does userspace need LMB for" is irrelevant. The host tools should need NO option be enabled/disabled.
Further, "disabling FOO breaks the build" means we need to investigate what the correct resolution is. In this case, LMB needs to be def_bool y. This is option #3 above. Why does u-boot-as-sandbox need LMB? Because that's how we ensure that the tests that check for overlap fail as expected.
Finally, you can just drop the first two patches and call me too stubborn. This is option #1 above.
The arch_lmb_reserve() is called by lib/lmb.c lmb_reserve_common() even if CMD_BOOT{I,M,Z} is not enabled. However, the arm32/arm64 variant of arch_lmb_reserve() is only compiled in if CMD_BOOT{I,M,Z} is enabled.
This currently does not trigger build error, because there is an empty weak implementation of arch_lmb_reserve(), however that is not the function that should be used on arm32/arm64.
Fix this by moving the arch_lmb_reserve() implementation into common code and always compile it in.
Signed-off-by: Marek Vasut marek.vasut+renesas@gmail.com Cc: Simon Glass sjg@chromium.org Cc: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com Cc: Tom Rini trini@konsulko.com --- arch/arm/lib/bootm.c | 45 -------------------------------------------- arch/arm/lib/stack.c | 45 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 45 deletions(-)
diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c index f60ee3a7e6..dd6a69315a 100644 --- a/arch/arm/lib/bootm.c +++ b/arch/arm/lib/bootm.c @@ -16,7 +16,6 @@ #include <command.h> #include <cpu_func.h> #include <dm.h> -#include <lmb.h> #include <log.h> #include <asm/global_data.h> #include <dm/root.h> @@ -43,50 +42,6 @@ DECLARE_GLOBAL_DATA_PTR;
static struct tag *params;
-static ulong get_sp(void) -{ - ulong ret; - - asm("mov %0, sp" : "=r"(ret) : ); - return ret; -} - -void arch_lmb_reserve(struct lmb *lmb) -{ - ulong sp, bank_end; - int bank; - - /* - * Booting a (Linux) kernel image - * - * Allocate space for command line and board info - the - * address should be as high as possible within the reach of - * the kernel (see CONFIG_SYS_BOOTMAPSZ settings), but in unused - * memory, which means far enough below the current stack - * pointer. - */ - sp = get_sp(); - debug("## Current stack ends at 0x%08lx ", sp); - - /* adjust sp by 4K to be safe */ - sp -= 4096; - for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) { - if (!gd->bd->bi_dram[bank].size || - sp < gd->bd->bi_dram[bank].start) - continue; - /* Watch out for RAM at end of address space! */ - bank_end = gd->bd->bi_dram[bank].start + - gd->bd->bi_dram[bank].size - 1; - if (sp > bank_end) - continue; - if (bank_end > gd->ram_top) - bank_end = gd->ram_top - 1; - - lmb_reserve(lmb, sp, bank_end - sp + 1); - break; - } -} - __weak void board_quiesce_devices(void) { } diff --git a/arch/arm/lib/stack.c b/arch/arm/lib/stack.c index b03e1cfc80..3f961f4454 100644 --- a/arch/arm/lib/stack.c +++ b/arch/arm/lib/stack.c @@ -12,6 +12,7 @@ */ #include <common.h> #include <init.h> +#include <lmb.h> #include <asm/global_data.h>
DECLARE_GLOBAL_DATA_PTR; @@ -33,3 +34,47 @@ int arch_reserve_stacks(void)
return 0; } + +static ulong get_sp(void) +{ + ulong ret; + + asm("mov %0, sp" : "=r"(ret) : ); + return ret; +} + +void arch_lmb_reserve(struct lmb *lmb) +{ + ulong sp, bank_end; + int bank; + + /* + * Booting a (Linux) kernel image + * + * Allocate space for command line and board info - the + * address should be as high as possible within the reach of + * the kernel (see CONFIG_SYS_BOOTMAPSZ settings), but in unused + * memory, which means far enough below the current stack + * pointer. + */ + sp = get_sp(); + debug("## Current stack ends at 0x%08lx ", sp); + + /* adjust sp by 4K to be safe */ + sp -= 4096; + for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) { + if (!gd->bd->bi_dram[bank].size || + sp < gd->bd->bi_dram[bank].start) + continue; + /* Watch out for RAM at end of address space! */ + bank_end = gd->bd->bi_dram[bank].start + + gd->bd->bi_dram[bank].size - 1; + if (sp > bank_end) + continue; + if (bank_end > gd->ram_top) + bank_end = gd->ram_top - 1; + + lmb_reserve(lmb, sp, bank_end - sp + 1); + break; + } +}
On Sun, Aug 15, 2021 at 08:13:03PM +0200, Marek Vasut wrote:
The arch_lmb_reserve() is called by lib/lmb.c lmb_reserve_common() even if CMD_BOOT{I,M,Z} is not enabled. However, the arm32/arm64 variant of arch_lmb_reserve() is only compiled in if CMD_BOOT{I,M,Z} is enabled.
This currently does not trigger build error, because there is an empty weak implementation of arch_lmb_reserve(), however that is not the function that should be used on arm32/arm64.
Fix this by moving the arch_lmb_reserve() implementation into common code and always compile it in.
Signed-off-by: Marek Vasut marek.vasut+renesas@gmail.com Cc: Simon Glass sjg@chromium.org Cc: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com Cc: Tom Rini trini@konsulko.com
Reviewed-by: Tom Rini trini@konsulko.com
The arch_lmb_reserve() is called by lib/lmb.c lmb_reserve_common() even if CMD_BOOTM is not enabled. However, the arc variant of arch_lmb_reserve() is only compiled in if CMD_BOOTM is enabled.
This currently does not trigger build error, because there is an empty weak implementation of arch_lmb_reserve(), however that is not the function that should be used on ar.
Fix this by moving the arch_lmb_reserve() implementation into common code and always compile it in.
Signed-off-by: Marek Vasut marek.vasut+renesas@gmail.com Cc: Simon Glass sjg@chromium.org Cc: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com Cc: Tom Rini trini@konsulko.com --- arch/arc/lib/bootm.c | 30 ------------------------------ arch/arc/lib/cache.c | 30 ++++++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 30 deletions(-)
diff --git a/arch/arc/lib/bootm.c b/arch/arc/lib/bootm.c index 8a8d394a5f..41408c2b46 100644 --- a/arch/arc/lib/bootm.c +++ b/arch/arc/lib/bootm.c @@ -8,42 +8,12 @@ #include <env.h> #include <image.h> #include <irq_func.h> -#include <lmb.h> #include <log.h> #include <asm/cache.h> #include <asm/global_data.h>
DECLARE_GLOBAL_DATA_PTR;
-static ulong get_sp(void) -{ - ulong ret; - - asm("mov %0, sp" : "=r"(ret) : ); - return ret; -} - -void arch_lmb_reserve(struct lmb *lmb) -{ - ulong sp; - - /* - * Booting a (Linux) kernel image - * - * Allocate space for command line and board info - the - * address should be as high as possible within the reach of - * the kernel (see CONFIG_SYS_BOOTMAPSZ settings), but in unused - * memory, which means far enough below the current stack - * pointer. - */ - sp = get_sp(); - debug("## Current stack ends at 0x%08lx ", sp); - - /* adjust sp by 4K to be safe */ - sp -= 4096; - lmb_reserve(lmb, sp, (CONFIG_SYS_SDRAM_BASE + gd->ram_size - sp)); -} - static int cleanup_before_linux(void) { disable_interrupts(); diff --git a/arch/arc/lib/cache.c b/arch/arc/lib/cache.c index f807cd83d6..4ba180482c 100644 --- a/arch/arc/lib/cache.c +++ b/arch/arc/lib/cache.c @@ -11,6 +11,7 @@ #include <linux/compiler.h> #include <linux/kernel.h> #include <linux/log2.h> +#include <lmb.h> #include <asm/arcregs.h> #include <asm/arc-bcr.h> #include <asm/cache.h> @@ -820,3 +821,32 @@ void sync_n_cleanup_cache_all(void)
__ic_entire_invalidate(); } + +static ulong get_sp(void) +{ + ulong ret; + + asm("mov %0, sp" : "=r"(ret) : ); + return ret; +} + +void arch_lmb_reserve(struct lmb *lmb) +{ + ulong sp; + + /* + * Booting a (Linux) kernel image + * + * Allocate space for command line and board info - the + * address should be as high as possible within the reach of + * the kernel (see CONFIG_SYS_BOOTMAPSZ settings), but in unused + * memory, which means far enough below the current stack + * pointer. + */ + sp = get_sp(); + debug("## Current stack ends at 0x%08lx ", sp); + + /* adjust sp by 4K to be safe */ + sp -= 4096; + lmb_reserve(lmb, sp, (CONFIG_SYS_SDRAM_BASE + gd->ram_size - sp)); +}
The arc/arm/m68k/microblaze/mips/ppc arch_lmb_reserve() implementations are all mostly the same, except for a couple of details. Implement a generic arch_lmb_reserve_generic() function which can be parametrized enough to cater for those differences between architectures. This can also be parametrized enough so it can handle cases where U-Boot is not relocated to the end of DRAM e.g. because there is some other reserved memory past U-Boot (e.g. unmovable firmware for coprocessor), it is not relocated at all, and other such use cases.
Signed-off-by: Marek Vasut marek.vasut+renesas@gmail.com Cc: Alexey Brodkin alexey.brodkin@synopsys.com Cc: Angelo Dureghello angelo@sysam.it Cc: Daniel Schwierzeck daniel.schwierzeck@gmail.com Cc: Eugeniy Paltsev Eugeniy.Paltsev@synopsys.com Cc: Hai Pham hai.pham.ud@renesas.com Cc: Michal Simek monstr@monstr.eu Cc: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com Cc: Tom Rini trini@konsulko.com Cc: Wolfgang Denk wd@denx.de --- include/lmb.h | 1 + lib/lmb.c | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+)
diff --git a/include/lmb.h b/include/lmb.h index fa1474a360..deca551ee7 100644 --- a/include/lmb.h +++ b/include/lmb.h @@ -122,6 +122,7 @@ lmb_size_bytes(struct lmb_region *type, unsigned long region_nr)
void board_lmb_reserve(struct lmb *lmb); void arch_lmb_reserve(struct lmb *lmb); +void arch_lmb_reserve_generic(struct lmb *lmb, ulong sp, ulong end, ulong align);
/* Low level functions */
diff --git a/lib/lmb.c b/lib/lmb.c index 7bd1255f7a..fed3c341a7 100644 --- a/lib/lmb.c +++ b/lib/lmb.c @@ -12,6 +12,10 @@ #include <log.h> #include <malloc.h>
+#include <asm/global_data.h> + +DECLARE_GLOBAL_DATA_PTR; + #define LMB_ALLOC_ANYWHERE 0
static void lmb_dump_region(struct lmb_region *rgn, char *name) @@ -113,6 +117,41 @@ void lmb_init(struct lmb *lmb) lmb->reserved.cnt = 0; }
+void arch_lmb_reserve_generic(struct lmb *lmb, ulong sp, ulong end, ulong align) +{ + ulong bank_end; + int bank; + + /* + * Booting a (Linux) kernel image + * + * Allocate space for command line and board info - the + * address should be as high as possible within the reach of + * the kernel (see CONFIG_SYS_BOOTMAPSZ settings), but in unused + * memory, which means far enough below the current stack + * pointer. + */ + debug("## Current stack ends at 0x%08lx ", sp); + + /* adjust sp by 4K to be safe */ + sp -= align; + for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) { + if (!gd->bd->bi_dram[bank].size || + sp < gd->bd->bi_dram[bank].start) + continue; + /* Watch out for RAM at end of address space! */ + bank_end = gd->bd->bi_dram[bank].start + + gd->bd->bi_dram[bank].size - 1; + if (sp > bank_end) + continue; + if (bank_end > end) + bank_end = end - 1; + + lmb_reserve(lmb, sp, bank_end - sp + 1); + break; + } +} + static void lmb_reserve_common(struct lmb *lmb, void *fdt_blob) { arch_lmb_reserve(lmb);
On Sun, Aug 15, 2021 at 08:13:05PM +0200, Marek Vasut wrote:
The arc/arm/m68k/microblaze/mips/ppc arch_lmb_reserve() implementations are all mostly the same, except for a couple of details. Implement a generic arch_lmb_reserve_generic() function which can be parametrized enough to cater for those differences between architectures. This can also be parametrized enough so it can handle cases where U-Boot is not relocated to the end of DRAM e.g. because there is some other reserved memory past U-Boot (e.g. unmovable firmware for coprocessor), it is not relocated at all, and other such use cases.
[snip]
+void arch_lmb_reserve_generic(struct lmb *lmb, ulong sp, ulong end, ulong align) +{
- ulong bank_end;
- int bank;
- /*
* Booting a (Linux) kernel image** Allocate space for command line and board info - the* address should be as high as possible within the reach of* the kernel (see CONFIG_SYS_BOOTMAPSZ settings), but in unused* memory, which means far enough below the current stack* pointer.*/
This comment is wrong, and we need to fix it, rather than continue to perpetuate it.
Switch arc/arm/m68k/microblaze/mips/ppc arch_lmb_reserve() to arch_lmb_reserve_generic().
Signed-off-by: Marek Vasut marek.vasut+renesas@gmail.com Cc: Alexey Brodkin alexey.brodkin@synopsys.com Cc: Angelo Dureghello angelo@sysam.it Cc: Daniel Schwierzeck daniel.schwierzeck@gmail.com Cc: Eugeniy Paltsev Eugeniy.Paltsev@synopsys.com Cc: Hai Pham hai.pham.ud@renesas.com Cc: Michal Simek monstr@monstr.eu Cc: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com Cc: Tom Rini trini@konsulko.com Cc: Wolfgang Denk wd@denx.de --- arch/arc/lib/cache.c | 18 +----------------- arch/arm/lib/stack.c | 33 +-------------------------------- arch/m68k/lib/bootm.c | 18 +----------------- arch/microblaze/lib/bootm.c | 28 +--------------------------- arch/mips/lib/bootm.c | 9 +-------- arch/powerpc/lib/bootm.c | 18 ++---------------- 6 files changed, 7 insertions(+), 117 deletions(-)
diff --git a/arch/arc/lib/cache.c b/arch/arc/lib/cache.c index 4ba180482c..4c696cb53a 100644 --- a/arch/arc/lib/cache.c +++ b/arch/arc/lib/cache.c @@ -832,21 +832,5 @@ static ulong get_sp(void)
void arch_lmb_reserve(struct lmb *lmb) { - ulong sp; - - /* - * Booting a (Linux) kernel image - * - * Allocate space for command line and board info - the - * address should be as high as possible within the reach of - * the kernel (see CONFIG_SYS_BOOTMAPSZ settings), but in unused - * memory, which means far enough below the current stack - * pointer. - */ - sp = get_sp(); - debug("## Current stack ends at 0x%08lx ", sp); - - /* adjust sp by 4K to be safe */ - sp -= 4096; - lmb_reserve(lmb, sp, (CONFIG_SYS_SDRAM_BASE + gd->ram_size - sp)); + arch_lmb_reserve_generic(lmb, get_sp(), gd->ram_top, 4096); } diff --git a/arch/arm/lib/stack.c b/arch/arm/lib/stack.c index 3f961f4454..52d9f15298 100644 --- a/arch/arm/lib/stack.c +++ b/arch/arm/lib/stack.c @@ -45,36 +45,5 @@ static ulong get_sp(void)
void arch_lmb_reserve(struct lmb *lmb) { - ulong sp, bank_end; - int bank; - - /* - * Booting a (Linux) kernel image - * - * Allocate space for command line and board info - the - * address should be as high as possible within the reach of - * the kernel (see CONFIG_SYS_BOOTMAPSZ settings), but in unused - * memory, which means far enough below the current stack - * pointer. - */ - sp = get_sp(); - debug("## Current stack ends at 0x%08lx ", sp); - - /* adjust sp by 4K to be safe */ - sp -= 4096; - for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) { - if (!gd->bd->bi_dram[bank].size || - sp < gd->bd->bi_dram[bank].start) - continue; - /* Watch out for RAM at end of address space! */ - bank_end = gd->bd->bi_dram[bank].start + - gd->bd->bi_dram[bank].size - 1; - if (sp > bank_end) - continue; - if (bank_end > gd->ram_top) - bank_end = gd->ram_top - 1; - - lmb_reserve(lmb, sp, bank_end - sp + 1); - break; - } + arch_lmb_reserve_generic(lmb, get_sp(), gd->ram_top, 4096); } diff --git a/arch/m68k/lib/bootm.c b/arch/m68k/lib/bootm.c index 51a6f93858..27729db67e 100644 --- a/arch/m68k/lib/bootm.c +++ b/arch/m68k/lib/bootm.c @@ -32,23 +32,7 @@ static void set_clocks_in_mhz (struct bd_info *kbd);
void arch_lmb_reserve(struct lmb *lmb) { - ulong sp; - - /* - * Booting a (Linux) kernel image - * - * Allocate space for command line and board info - the - * address should be as high as possible within the reach of - * the kernel (see CONFIG_SYS_BOOTMAPSZ settings), but in unused - * memory, which means far enough below the current stack - * pointer. - */ - sp = get_sp(); - debug ("## Current stack ends at 0x%08lx ", sp); - - /* adjust sp by 1K to be safe */ - sp -= 1024; - lmb_reserve(lmb, sp, (CONFIG_SYS_SDRAM_BASE + gd->ram_size - sp)); + arch_lmb_reserve_generic(lmb, get_sp(), gd->ram_top, 1024); }
int do_bootm_linux(int flag, int argc, char *const argv[], diff --git a/arch/microblaze/lib/bootm.c b/arch/microblaze/lib/bootm.c index 6695ac63c7..3a6da6e29f 100644 --- a/arch/microblaze/lib/bootm.c +++ b/arch/microblaze/lib/bootm.c @@ -34,33 +34,7 @@ static ulong get_sp(void)
void arch_lmb_reserve(struct lmb *lmb) { - ulong sp, bank_end; - int bank; - - /* - * Booting a (Linux) kernel image - * - * Allocate space for command line and board info - the - * address should be as high as possible within the reach of - * the kernel (see CONFIG_SYS_BOOTMAPSZ settings), but in unused - * memory, which means far enough below the current stack - * pointer. - */ - sp = get_sp(); - debug("## Current stack ends at 0x%08lx ", sp); - - /* adjust sp by 4K to be safe */ - sp -= 4096; - for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) { - if (sp < gd->bd->bi_dram[bank].start) - continue; - bank_end = gd->bd->bi_dram[bank].start + - gd->bd->bi_dram[bank].size; - if (sp >= bank_end) - continue; - lmb_reserve(lmb, sp, bank_end - sp); - break; - } + arch_lmb_reserve_generic(lmb, get_sp(), gd->ram_top, 4096); }
static void boot_jump_linux(bootm_headers_t *images, int flag) diff --git a/arch/mips/lib/bootm.c b/arch/mips/lib/bootm.c index fde90fced4..cab8da4860 100644 --- a/arch/mips/lib/bootm.c +++ b/arch/mips/lib/bootm.c @@ -39,14 +39,7 @@ static ulong arch_get_sp(void)
void arch_lmb_reserve(struct lmb *lmb) { - ulong sp; - - sp = arch_get_sp(); - debug("## Current stack ends at 0x%08lx\n", sp); - - /* adjust sp by 4K to be safe */ - sp -= 4096; - lmb_reserve(lmb, sp, gd->ram_top - sp); + arch_lmb_reserve_generic(lmb, arch_get_sp(), gd->ram_top, 4096); }
static void linux_cmdline_init(void) diff --git a/arch/powerpc/lib/bootm.c b/arch/powerpc/lib/bootm.c index 31c17b5bb3..8d65047aa4 100644 --- a/arch/powerpc/lib/bootm.c +++ b/arch/powerpc/lib/bootm.c @@ -119,7 +119,7 @@ static void boot_jump_linux(bootm_headers_t *images) void arch_lmb_reserve(struct lmb *lmb) { phys_size_t bootm_size; - ulong size, sp, bootmap_base; + ulong size, bootmap_base;
bootmap_base = env_get_bootm_low(); bootm_size = env_get_bootm_size(); @@ -141,21 +141,7 @@ void arch_lmb_reserve(struct lmb *lmb) lmb_reserve(lmb, base, bootm_size - size); }
- /* - * Booting a (Linux) kernel image - * - * Allocate space for command line and board info - the - * address should be as high as possible within the reach of - * the kernel (see CONFIG_SYS_BOOTMAPSZ settings), but in unused - * memory, which means far enough below the current stack - * pointer. - */ - sp = get_sp(); - debug("## Current stack ends at 0x%08lx\n", sp); - - /* adjust sp by 4K to be safe */ - sp -= 4096; - lmb_reserve(lmb, sp, (CONFIG_SYS_SDRAM_BASE + get_effective_memsize() - sp)); + arch_lmb_reserve_generic(lmb, get_sp(), gd->ram_top, 4096);
#ifdef CONFIG_MP cpu_mp_lmb_reserve(lmb);
On Sun, Aug 15, 2021 at 08:13:06PM +0200, Marek Vasut wrote:
Switch arc/arm/m68k/microblaze/mips/ppc arch_lmb_reserve() to arch_lmb_reserve_generic().
Signed-off-by: Marek Vasut marek.vasut+renesas@gmail.com Cc: Alexey Brodkin alexey.brodkin@synopsys.com Cc: Angelo Dureghello angelo@sysam.it Cc: Daniel Schwierzeck daniel.schwierzeck@gmail.com Cc: Eugeniy Paltsev Eugeniy.Paltsev@synopsys.com Cc: Hai Pham hai.pham.ud@renesas.com Cc: Michal Simek monstr@monstr.eu Cc: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com Cc: Tom Rini trini@konsulko.com Cc: Wolfgang Denk wd@denx.de
Reviewed-by: Tom Rini trini@konsulko.com
Add arch_lmb_reserve() implemented using arch_lmb_reserve_generic(). It is rather likely this architecture also needs to cover U-Boot with LMB before booting Linux.
Signed-off-by: Marek Vasut marek.vasut+renesas@gmail.com Cc: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com Cc: Thomas Chou thomas@wytron.com.tw Cc: Tom Rini trini@konsulko.com --- arch/nios2/lib/bootm.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/arch/nios2/lib/bootm.c b/arch/nios2/lib/bootm.c index 5037467151..3cb59bd977 100644 --- a/arch/nios2/lib/bootm.c +++ b/arch/nios2/lib/bootm.c @@ -10,6 +10,9 @@ #include <image.h> #include <irq_func.h> #include <log.h> +#include <asm/global_data.h> + +DECLARE_GLOBAL_DATA_PTR;
#define NIOS_MAGIC 0x534f494e /* enable command line and initrd passing */
@@ -60,3 +63,16 @@ int do_bootm_linux(int flag, int argc, char *const argv[],
return 1; } + +static ulong get_sp(void) +{ + ulong ret; + + asm("mov %0, sp" : "=r"(ret) : ); + return ret; +} + +void arch_lmb_reserve(struct lmb *lmb) +{ + arch_lmb_reserve_generic(lmb, get_sp(), gd->ram_top, 4096); +}
Add arch_lmb_reserve() implemented using arch_lmb_reserve_generic(). It is rather likely this architecture also needs to cover U-Boot with LMB before booting Linux.
Signed-off-by: Marek Vasut marek.vasut+renesas@gmail.com Cc: Rick Chen rick@andestech.com Cc: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com Cc: Tom Rini trini@konsulko.com --- arch/nds32/lib/bootm.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/arch/nds32/lib/bootm.c b/arch/nds32/lib/bootm.c index 4cb0f530ae..a7c8978f23 100644 --- a/arch/nds32/lib/bootm.c +++ b/arch/nds32/lib/bootm.c @@ -245,3 +245,16 @@ static void setup_end_tag(struct bd_info *bd) }
#endif /* CONFIG_SETUP_MEMORY_TAGS || CONFIG_CMDLINE_TAG || CONFIG_INITRD_TAG */ + +static ulong get_sp(void) +{ + ulong ret; + + asm("move %0, $sp" : "=r"(ret) : ); + return ret; +} + +void arch_lmb_reserve(struct lmb *lmb) +{ + arch_lmb_reserve_generic(lmb, get_sp(), gd->ram_top, 4096); +}
Add arch_lmb_reserve() implemented using arch_lmb_reserve_generic(). It is rather likely this architecture also needs to cover U-Boot with LMB before booting Linux.
Signed-off-by: Marek Vasut marek.vasut+renesas@gmail.com Cc: Atish Patra atish.patra@wdc.com Cc: Leo ycliang@andestech.com Cc: Rick Chen rick@andestech.com Cc: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com Cc: Tom Rini trini@konsulko.com --- arch/riscv/lib/bootm.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/arch/riscv/lib/bootm.c b/arch/riscv/lib/bootm.c index 8dd1820540..ff1bdf7131 100644 --- a/arch/riscv/lib/bootm.c +++ b/arch/riscv/lib/bootm.c @@ -135,3 +135,16 @@ int do_bootm_vxworks(int flag, int argc, char *const argv[], { return do_bootm_linux(flag, argc, argv, images); } + +static ulong get_sp(void) +{ + ulong ret; + + asm("mv %0, sp" : "=r"(ret) : ); + return ret; +} + +void arch_lmb_reserve(struct lmb *lmb) +{ + arch_lmb_reserve_generic(lmb, get_sp(), gd->ram_top, 4096); +}
Add arch_lmb_reserve() implemented using arch_lmb_reserve_generic(). This architecture also needs to cover U-Boot with LMB before booting Linux.
Signed-off-by: Marek Vasut marek.vasut+renesas@gmail.com Cc: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com Cc: Tom Rini trini@konsulko.com --- arch/sh/lib/bootm.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/arch/sh/lib/bootm.c b/arch/sh/lib/bootm.c index dc94f83785..9b71424dfe 100644 --- a/arch/sh/lib/bootm.c +++ b/arch/sh/lib/bootm.c @@ -12,8 +12,11 @@ #include <env.h> #include <image.h> #include <asm/byteorder.h> +#include <asm/global_data.h> #include <asm/zimage.h>
+DECLARE_GLOBAL_DATA_PTR; + #ifdef CONFIG_SYS_DEBUG static void hexdump(unsigned char *buf, int len) { @@ -111,3 +114,16 @@ int do_bootm_linux(int flag, int argc, char *const argv[], /* does not return */ return 1; } + +static ulong get_sp(void) +{ + ulong ret; + + asm("mov r15, %0" : "=r"(ret) : ); + return ret; +} + +void arch_lmb_reserve(struct lmb *lmb) +{ + arch_lmb_reserve_generic(lmb, get_sp(), gd->ram_top, 4096); +}
Add arch_lmb_reserve() implemented using arch_lmb_reserve_generic(). It is rather likely this architecture also needs to cover U-Boot with LMB before booting Linux.
Signed-off-by: Marek Vasut marek.vasut+renesas@gmail.com Cc: Chris Zankel chris@zankel.net Cc: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com Cc: Tom Rini trini@konsulko.com --- arch/xtensa/lib/bootm.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/arch/xtensa/lib/bootm.c b/arch/xtensa/lib/bootm.c index bb1e2886ab..277af18168 100644 --- a/arch/xtensa/lib/bootm.c +++ b/arch/xtensa/lib/bootm.c @@ -197,3 +197,15 @@ int do_bootm_linux(int flag, int argc, char *argv[], bootm_headers_t *images) return 1; }
+static ulong get_sp(void) +{ + ulong ret; + + asm("mov %0, a1" : "=r"(ret) : ); + return ret; +} + +void arch_lmb_reserve(struct lmb *lmb) +{ + arch_lmb_reserve_generic(lmb, get_sp(), gd->ram_top, 4096); +}
Add arch_lmb_reserve() implemented using arch_lmb_reserve_generic(). It is rather likely this architecture also needs to cover U-Boot with LMB before booting Linux.
Signed-off-by: Marek Vasut marek.vasut+renesas@gmail.com Cc: Simon Glass sjg@chromium.org Cc: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com Cc: Tom Rini trini@konsulko.com --- arch/x86/lib/bootm.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/arch/x86/lib/bootm.c b/arch/x86/lib/bootm.c index 733dd71257..667e5e689e 100644 --- a/arch/x86/lib/bootm.c +++ b/arch/x86/lib/bootm.c @@ -223,3 +223,21 @@ int do_bootm_linux(int flag, int argc, char *const argv[],
return boot_jump_linux(images); } + +static ulong get_sp(void) +{ + ulong ret; + +#if CONFIG_IS_ENABLED(X86_64) + ret = gd->start_addr_sp; +#else + asm("mov %%esp, %0" : "=r"(ret) : ); +#endif + + return ret; +} + +void arch_lmb_reserve(struct lmb *lmb) +{ + arch_lmb_reserve_generic(lmb, get_sp(), gd->ram_top, 4096); +}
Mark arch_lmb_reserve() weak on architecture level, so it can be overridden if necessary. This might be necessary if there is some sort of architectural exception where e.g. more LMB areas have to be reserved.
Note that sandbox now needs an empty implementation of arch_lmb_reserve().
Signed-off-by: Marek Vasut marek.vasut+renesas@gmail.com Cc: Alexey Brodkin alexey.brodkin@synopsys.com Cc: Angelo Dureghello angelo@sysam.it Cc: Daniel Schwierzeck daniel.schwierzeck@gmail.com Cc: Eugeniy Paltsev Eugeniy.Paltsev@synopsys.com Cc: Hai Pham hai.pham.ud@renesas.com Cc: Michal Simek monstr@monstr.eu Cc: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com Cc: Tom Rini trini@konsulko.com Cc: Wolfgang Denk wd@denx.de --- arch/arc/lib/cache.c | 2 +- arch/arm/lib/stack.c | 2 +- arch/m68k/lib/bootm.c | 2 +- arch/microblaze/lib/bootm.c | 2 +- arch/mips/lib/bootm.c | 2 +- arch/powerpc/lib/bootm.c | 2 +- arch/riscv/lib/bootm.c | 2 +- arch/sandbox/lib/bootm.c | 5 +++++ arch/sh/lib/bootm.c | 2 +- arch/x86/lib/bootm.c | 2 +- arch/xtensa/lib/bootm.c | 2 +- lib/lmb.c | 5 ----- 12 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/arch/arc/lib/cache.c b/arch/arc/lib/cache.c index 4c696cb53a..23bca1fffc 100644 --- a/arch/arc/lib/cache.c +++ b/arch/arc/lib/cache.c @@ -830,7 +830,7 @@ static ulong get_sp(void) return ret; }
-void arch_lmb_reserve(struct lmb *lmb) +__weak void arch_lmb_reserve(struct lmb *lmb) { arch_lmb_reserve_generic(lmb, get_sp(), gd->ram_top, 4096); } diff --git a/arch/arm/lib/stack.c b/arch/arm/lib/stack.c index 52d9f15298..93a69a1688 100644 --- a/arch/arm/lib/stack.c +++ b/arch/arm/lib/stack.c @@ -43,7 +43,7 @@ static ulong get_sp(void) return ret; }
-void arch_lmb_reserve(struct lmb *lmb) +__weak void arch_lmb_reserve(struct lmb *lmb) { arch_lmb_reserve_generic(lmb, get_sp(), gd->ram_top, 4096); } diff --git a/arch/m68k/lib/bootm.c b/arch/m68k/lib/bootm.c index 27729db67e..fb4784187a 100644 --- a/arch/m68k/lib/bootm.c +++ b/arch/m68k/lib/bootm.c @@ -30,7 +30,7 @@ DECLARE_GLOBAL_DATA_PTR; static ulong get_sp (void); static void set_clocks_in_mhz (struct bd_info *kbd);
-void arch_lmb_reserve(struct lmb *lmb) +__weak void arch_lmb_reserve(struct lmb *lmb) { arch_lmb_reserve_generic(lmb, get_sp(), gd->ram_top, 1024); } diff --git a/arch/microblaze/lib/bootm.c b/arch/microblaze/lib/bootm.c index 3a6da6e29f..2621770082 100644 --- a/arch/microblaze/lib/bootm.c +++ b/arch/microblaze/lib/bootm.c @@ -32,7 +32,7 @@ static ulong get_sp(void) return ret; }
-void arch_lmb_reserve(struct lmb *lmb) +__weak void arch_lmb_reserve(struct lmb *lmb) { arch_lmb_reserve_generic(lmb, get_sp(), gd->ram_top, 4096); } diff --git a/arch/mips/lib/bootm.c b/arch/mips/lib/bootm.c index cab8da4860..57873f200b 100644 --- a/arch/mips/lib/bootm.c +++ b/arch/mips/lib/bootm.c @@ -37,7 +37,7 @@ static ulong arch_get_sp(void) return ret; }
-void arch_lmb_reserve(struct lmb *lmb) +__weak void arch_lmb_reserve(struct lmb *lmb) { arch_lmb_reserve_generic(lmb, arch_get_sp(), gd->ram_top, 4096); } diff --git a/arch/powerpc/lib/bootm.c b/arch/powerpc/lib/bootm.c index 8d65047aa4..482bacdfe6 100644 --- a/arch/powerpc/lib/bootm.c +++ b/arch/powerpc/lib/bootm.c @@ -116,7 +116,7 @@ static void boot_jump_linux(bootm_headers_t *images) return ; }
-void arch_lmb_reserve(struct lmb *lmb) +__weak void arch_lmb_reserve(struct lmb *lmb) { phys_size_t bootm_size; ulong size, bootmap_base; diff --git a/arch/riscv/lib/bootm.c b/arch/riscv/lib/bootm.c index ff1bdf7131..377c77b909 100644 --- a/arch/riscv/lib/bootm.c +++ b/arch/riscv/lib/bootm.c @@ -144,7 +144,7 @@ static ulong get_sp(void) return ret; }
-void arch_lmb_reserve(struct lmb *lmb) +__weak void arch_lmb_reserve(struct lmb *lmb) { arch_lmb_reserve_generic(lmb, get_sp(), gd->ram_top, 4096); } diff --git a/arch/sandbox/lib/bootm.c b/arch/sandbox/lib/bootm.c index d1d460b84a..17c02fc9f7 100644 --- a/arch/sandbox/lib/bootm.c +++ b/arch/sandbox/lib/bootm.c @@ -61,3 +61,8 @@ int do_bootm_linux(int flag, int argc, char *argv[], bootm_headers_t *images)
return 0; } + +void arch_lmb_reserve(struct lmb *lmb) +{ + /* do nothing */ +} diff --git a/arch/sh/lib/bootm.c b/arch/sh/lib/bootm.c index 9b71424dfe..ec4a74b43c 100644 --- a/arch/sh/lib/bootm.c +++ b/arch/sh/lib/bootm.c @@ -123,7 +123,7 @@ static ulong get_sp(void) return ret; }
-void arch_lmb_reserve(struct lmb *lmb) +__weak void arch_lmb_reserve(struct lmb *lmb) { arch_lmb_reserve_generic(lmb, get_sp(), gd->ram_top, 4096); } diff --git a/arch/x86/lib/bootm.c b/arch/x86/lib/bootm.c index 667e5e689e..b46411d93d 100644 --- a/arch/x86/lib/bootm.c +++ b/arch/x86/lib/bootm.c @@ -237,7 +237,7 @@ static ulong get_sp(void) return ret; }
-void arch_lmb_reserve(struct lmb *lmb) +__weak void arch_lmb_reserve(struct lmb *lmb) { arch_lmb_reserve_generic(lmb, get_sp(), gd->ram_top, 4096); } diff --git a/arch/xtensa/lib/bootm.c b/arch/xtensa/lib/bootm.c index 277af18168..a8ad62aa54 100644 --- a/arch/xtensa/lib/bootm.c +++ b/arch/xtensa/lib/bootm.c @@ -205,7 +205,7 @@ static ulong get_sp(void) return ret; }
-void arch_lmb_reserve(struct lmb *lmb) +__weak void arch_lmb_reserve(struct lmb *lmb) { arch_lmb_reserve_generic(lmb, get_sp(), gd->ram_top, 4096); } diff --git a/lib/lmb.c b/lib/lmb.c index fed3c341a7..00498944a0 100644 --- a/lib/lmb.c +++ b/lib/lmb.c @@ -504,8 +504,3 @@ __weak void board_lmb_reserve(struct lmb *lmb) { /* please define platform specific board_lmb_reserve() */ } - -__weak void arch_lmb_reserve(struct lmb *lmb) -{ - /* please define platform specific arch_lmb_reserve() */ -}
On Sun, Aug 15, 2021 at 08:13:13PM +0200, Marek Vasut wrote:
Mark arch_lmb_reserve() weak on architecture level, so it can be overridden if necessary. This might be necessary if there is some sort of architectural exception where e.g. more LMB areas have to be reserved.
Note that sandbox now needs an empty implementation of arch_lmb_reserve().
I'm not sure we need this. We have the arch and board call-outs, and I've already addressed the imx example. I really want to see how this works on the I believe you've said rcar example.
On 8/15/21 9:50 PM, Tom Rini wrote:
On Sun, Aug 15, 2021 at 08:13:13PM +0200, Marek Vasut wrote:
Mark arch_lmb_reserve() weak on architecture level, so it can be overridden if necessary. This might be necessary if there is some sort of architectural exception where e.g. more LMB areas have to be reserved.
Note that sandbox now needs an empty implementation of arch_lmb_reserve().
I'm not sure we need this. We have the arch and board call-outs, and I've already addressed the imx example. I really want to see how this works on the I believe you've said rcar example.
Since they disable relocation in the bsp too, gd->ram_top would be replaced by fixed value there. I can skip this patch for now.
On Sun, Aug 29, 2021 at 06:46:38PM +0200, Marek Vasut wrote:
On 8/15/21 9:50 PM, Tom Rini wrote:
On Sun, Aug 15, 2021 at 08:13:13PM +0200, Marek Vasut wrote:
Mark arch_lmb_reserve() weak on architecture level, so it can be overridden if necessary. This might be necessary if there is some sort of architectural exception where e.g. more LMB areas have to be reserved.
Note that sandbox now needs an empty implementation of arch_lmb_reserve().
I'm not sure we need this. We have the arch and board call-outs, and I've already addressed the imx example. I really want to see how this works on the I believe you've said rcar example.
Since they disable relocation in the bsp too, gd->ram_top would be replaced by fixed value there. I can skip this patch for now.
OK.
This function is clearly architecture specific code, not board specific code. The only difference from the generic arm arch_lmb_reserve() is the extra reservation of 16k of memory below the stack bottom, rather than the default 4k. Switch this from board_lmb_reserve() to arch_lmb_reserve() and use arch_lmb_reserve_generic() with 16k stack reservation parameter instead of replicating older version of arch_lmb_reserve() here.
Signed-off-by: Marek Vasut marek.vasut+renesas@gmail.com Cc: Alexey Brodkin alexey.brodkin@synopsys.com Cc: Angelo Dureghello angelo@sysam.it Cc: Daniel Schwierzeck daniel.schwierzeck@gmail.com Cc: Eugeniy Paltsev Eugeniy.Paltsev@synopsys.com Cc: Hai Pham hai.pham.ud@renesas.com Cc: Michal Simek monstr@monstr.eu Cc: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com Cc: Tom Rini trini@konsulko.com Cc: Wolfgang Denk wd@denx.de Cc: Ye Li ye.li@nxp.com --- arch/arm/mach-imx/misc.c | 21 ++------------------- 1 file changed, 2 insertions(+), 19 deletions(-)
diff --git a/arch/arm/mach-imx/misc.c b/arch/arm/mach-imx/misc.c index d82efa7f8f..529601156f 100644 --- a/arch/arm/mach-imx/misc.c +++ b/arch/arm/mach-imx/misc.c @@ -86,24 +86,7 @@ static ulong get_sp(void) return ret; }
-void board_lmb_reserve(struct lmb *lmb) +void arch_lmb_reserve(struct lmb *lmb) { - ulong sp, bank_end; - int bank; - - sp = get_sp(); - debug("## Current stack ends at 0x%08lx ", sp); - - /* adjust sp by 16K to be safe */ - sp -= 4096 << 2; - for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) { - if (sp < gd->bd->bi_dram[bank].start) - continue; - bank_end = gd->bd->bi_dram[bank].start + - gd->bd->bi_dram[bank].size; - if (sp >= bank_end) - continue; - lmb_reserve(lmb, sp, bank_end - sp); - break; - } + arch_lmb_reserve_generic(lmb, get_sp(), gd->ram_top, 16384); }
On Sun, Aug 15, 2021 at 08:13:14PM +0200, Marek Vasut wrote:
This function is clearly architecture specific code, not board specific code. The only difference from the generic arm arch_lmb_reserve() is the extra reservation of 16k of memory below the stack bottom, rather than the default 4k. Switch this from board_lmb_reserve() to arch_lmb_reserve() and use arch_lmb_reserve_generic() with 16k stack reservation parameter instead of replicating older version of arch_lmb_reserve() here.
Signed-off-by: Marek Vasut marek.vasut+renesas@gmail.com Cc: Alexey Brodkin alexey.brodkin@synopsys.com Cc: Angelo Dureghello angelo@sysam.it Cc: Daniel Schwierzeck daniel.schwierzeck@gmail.com Cc: Eugeniy Paltsev Eugeniy.Paltsev@synopsys.com Cc: Hai Pham hai.pham.ud@renesas.com Cc: Michal Simek monstr@monstr.eu Cc: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com Cc: Tom Rini trini@konsulko.com Cc: Wolfgang Denk wd@denx.de Cc: Ye Li ye.li@nxp.com
arch/arm/mach-imx/misc.c | 21 ++------------------- 1 file changed, 2 insertions(+), 19 deletions(-)
diff --git a/arch/arm/mach-imx/misc.c b/arch/arm/mach-imx/misc.c index d82efa7f8f..529601156f 100644 --- a/arch/arm/mach-imx/misc.c +++ b/arch/arm/mach-imx/misc.c @@ -86,24 +86,7 @@ static ulong get_sp(void) return ret; }
-void board_lmb_reserve(struct lmb *lmb) +void arch_lmb_reserve(struct lmb *lmb) {
- ulong sp, bank_end;
- int bank;
- sp = get_sp();
- debug("## Current stack ends at 0x%08lx ", sp);
- /* adjust sp by 16K to be safe */
- sp -= 4096 << 2;
- for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) {
if (sp < gd->bd->bi_dram[bank].start)continue;bank_end = gd->bd->bi_dram[bank].start +gd->bd->bi_dram[bank].size;if (sp >= bank_end)continue;lmb_reserve(lmb, sp, bank_end - sp);break;- }
- arch_lmb_reserve_generic(lmb, get_sp(), gd->ram_top, 16384);
}
As I previously said, this should be the generic ARM one. And arguably all device-tree using architectures should give themselves more headroom here, based on the additional context we got. Calling the iMX entry the arch one is wrong. If we don't want to address the general problem more, we should keep this as a board one that reserves an additional 12KiB.
participants (3)
-
Marek Vasut -
Marek Vasut -
Tom Rini