[PATCH 01/14] configs: Disable LMB and BDI for tools-only

[PATCH 01/14] configs: Disable LMB and BDI for tools-only

[Marek Vasut]

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
-- 
2.30.2

[PATCH 02/14] lmb: Use CONFIG_LMB_*_REGIONS only if they are defined

[Marek Vasut]

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
-- 
2.30.2

Re: [PATCH 02/14] lmb: Use CONFIG_LMB_*_REGIONS only if they are defined

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1511 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 02/14] lmb: Use CONFIG_LMB_*_REGIONS only if they are defined

[Marek Vasut]

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.

Re: [PATCH 02/14] lmb: Use CONFIG_LMB_*_REGIONS only if they are defined

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2244 bytes --]

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?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 02/14] lmb: Use CONFIG_LMB_*_REGIONS only if they are defined

[Marek Vasut]

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

Re: [PATCH 02/14] lmb: Use CONFIG_LMB_*_REGIONS only if they are defined

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2844 bytes --]

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?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 02/14] lmb: Use CONFIG_LMB_*_REGIONS only if they are defined

[Marek Vasut]

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.

Re: [PATCH 02/14] lmb: Use CONFIG_LMB_*_REGIONS only if they are defined

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3270 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 02/14] lmb: Use CONFIG_LMB_*_REGIONS only if they are defined

[Marek Vasut]

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.

Re: [PATCH 02/14] lmb: Use CONFIG_LMB_*_REGIONS only if they are defined

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3902 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 02/14] lmb: Use CONFIG_LMB_*_REGIONS only if they are defined

[Marek Vasut]

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 ?

Re: [PATCH 02/14] lmb: Use CONFIG_LMB_*_REGIONS only if they are defined

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 4793 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 02/14] lmb: Use CONFIG_LMB_*_REGIONS only if they are defined

[Marek Vasut]

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.

Re: [PATCH 02/14] lmb: Use CONFIG_LMB_*_REGIONS only if they are defined

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 6220 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 02/14] lmb: Use CONFIG_LMB_*_REGIONS only if they are defined

[Marek Vasut]

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.

Re: [PATCH 02/14] lmb: Use CONFIG_LMB_*_REGIONS only if they are defined

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 8054 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 02/14] lmb: Use CONFIG_LMB_*_REGIONS only if they are defined

[Marek Vasut]

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.

[...]

Re: [PATCH 02/14] lmb: Use CONFIG_LMB_*_REGIONS only if they are defined

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 5990 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 02/14] lmb: Use CONFIG_LMB_*_REGIONS only if they are defined

[Marek Vasut]

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.

Re: [PATCH 02/14] lmb: Use CONFIG_LMB_*_REGIONS only if they are defined

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1756 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 02/14] lmb: Use CONFIG_LMB_*_REGIONS only if they are defined

[Marek Vasut]

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.

Re: [PATCH 02/14] lmb: Use CONFIG_LMB_*_REGIONS only if they are defined

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2173 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 02/14] lmb: Use CONFIG_LMB_*_REGIONS only if they are defined

[Marek Vasut]

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 ?

This is exactly what patch 1/14 is about -- disable LMB for tools build, 
because it is useless for tools build.

Re: [PATCH 02/14] lmb: Use CONFIG_LMB_*_REGIONS only if they are defined

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2537 bytes --]

[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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 02/14] lmb: Use CONFIG_LMB_*_REGIONS only if they are defined

[Marek Vasut]

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.

Re: [PATCH 02/14] lmb: Use CONFIG_LMB_*_REGIONS only if they are defined

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 4071 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH 03/14] lmb: Always compile arch_lmb_reserve() into U-Boot on arm

[Marek Vasut]

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;
+	}
+}
-- 
2.30.2

Re: [PATCH 03/14] lmb: Always compile arch_lmb_reserve() into U-Boot on arm

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 850 bytes --]

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>

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH 04/14] lmb: Always compile arch_lmb_reserve() into U-Boot on arc

[Marek Vasut]

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));
+}
-- 
2.30.2

[PATCH 05/14] lmb: Add generic arch_lmb_reserve_generic()

[Marek Vasut]

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);
-- 
2.30.2

Re: [PATCH 05/14] lmb: Add generic arch_lmb_reserve_generic()

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1209 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH 06/14] lmb: Switch to generic arch_lmb_reserve_generic()

[Marek Vasut]

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);
-- 
2.30.2

Re: [PATCH 06/14] lmb: Switch to generic arch_lmb_reserve_generic()

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 701 bytes --]

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>

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH 07/14] lmb: nios2: Add arch_lmb_reserve()

[Marek Vasut]

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);
+}
-- 
2.30.2

[PATCH 08/14] lmb: nds32: Add arch_lmb_reserve()

[Marek Vasut]

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);
+}
-- 
2.30.2

Re: [PATCH 08/14] lmb: nds32: Add arch_lmb_reserve()

[Rick Chen]

> From: Marek Vasut <marek.vasut@gmail.com>
> Sent: Monday, August 16, 2021 2:13 AM
> To: u-boot@lists.denx.de
> Cc: Marek Vasut <marek.vasut+renesas@gmail.com>; Rick Jian-Zhi Chen(陳建志) <rick@andestech.com>; Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>; Tom Rini <trini@konsulko.com>
> Subject: [PATCH 08/14] lmb: nds32: Add arch_lmb_reserve()
>
> 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(+)

Reviewed-by: Rick Chen <rick@andestech.com>

[PATCH 09/14] lmb: riscv: Add arch_lmb_reserve()

[Marek Vasut]

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);
+}
-- 
2.30.2

Re: [PATCH 09/14] lmb: riscv: Add arch_lmb_reserve()

[Rick Chen]

> From: Marek Vasut <marek.vasut@gmail.com>
> Sent: Monday, August 16, 2021 2:13 AM
> To: u-boot@lists.denx.de
> Cc: Marek Vasut <marek.vasut+renesas@gmail.com>; Atish Patra <atish.patra@wdc.com>; Leo Yu-Chi Liang(梁育齊) <ycliang@andestech.com>; Rick Jian-Zhi Chen(陳建志) <rick@andestech.com>; Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>; Tom Rini <trini@konsulko.com>
> Subject: [PATCH 09/14] lmb: riscv: Add arch_lmb_reserve()
>
> 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(+)

Reviewed-by: Rick Chen <rick@andestech.com>

[PATCH 10/14] lmb: sh: Add arch_lmb_reserve()

[Marek Vasut]

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);
+}
-- 
2.30.2

[PATCH 11/14] lmb: xtensa: Add arch_lmb_reserve()

[Marek Vasut]

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);
+}
-- 
2.30.2

[PATCH 12/14] lmb: x86: Add arch_lmb_reserve()

[Marek Vasut]

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);
+}
-- 
2.30.2

[PATCH 13/14] lmb: Mark arch_lmb_reserve() as weak symbol

[Marek Vasut]

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() */
-}
-- 
2.30.2

Re: [PATCH 13/14] lmb: Mark arch_lmb_reserve() as weak symbol

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 578 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 13/14] lmb: Mark arch_lmb_reserve() as weak symbol

[Marek Vasut]

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.

Re: [PATCH 13/14] lmb: Mark arch_lmb_reserve() as weak symbol

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 877 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH 14/14] lmb: Switch imx board_lmb_reserve() to arch_lmb_reserve()

[Marek Vasut]

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);
 }
-- 
2.30.2

Re: [PATCH 14/14] lmb: Switch imx board_lmb_reserve() to arch_lmb_reserve()

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2432 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]