[PATCH 1/2] lmb: Fix lmb property's defination under struct lmb

[PATCH 1/2] lmb: Fix lmb property's defination under struct lmb

[Michal Simek]

From: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>

Under struct lmb {} the lmb property's should be defined only if
CONFIG_LMB_MEMORY_REGIONS is defined.

Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---

 include/lmb.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/lmb.h b/include/lmb.h
index ab277ca80004..1476d78c2823 100644
--- a/include/lmb.h
+++ b/include/lmb.h
@@ -68,7 +68,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_MEMORY_REGIONS)
 	struct lmb_property memory_regions[CONFIG_LMB_MEMORY_REGIONS];
 	struct lmb_property reserved_regions[CONFIG_LMB_RESERVED_REGIONS];
 #endif
-- 
2.34.1

[PATCH 2/2] zynqmp: Run board_get_usable_ram_top() only on main U-Boot

[Michal Simek]

From: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>

With commit ce39ee28ec31 ("zynqmp: Do not place u-boot to reserved memory
location"), the function board_get_usable_ram_top() is allocating
MMU_SECTION_SIZE of about 2MB using lmb_alloc(). But we dont have this
much memory in case of mini U-Boot.

Keep these functions which use lmb under CONFIG_LMB so that they are
compiled and used only when LMB is enabled.

Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---

 board/xilinx/zynqmp/zynqmp.c | 3 +++
 boot/image-board.c           | 4 ++++
 2 files changed, 7 insertions(+)

diff --git a/board/xilinx/zynqmp/zynqmp.c b/board/xilinx/zynqmp/zynqmp.c
index 2b5239ccb475..c69fb939a83e 100644
--- a/board/xilinx/zynqmp/zynqmp.c
+++ b/board/xilinx/zynqmp/zynqmp.c
@@ -483,6 +483,7 @@ int dram_init(void)
 	return 0;
 }
 
+#if defined(CONFIG_LMB)
 ulong board_get_usable_ram_top(ulong total_size)
 {
 	phys_size_t size;
@@ -504,6 +505,8 @@ ulong board_get_usable_ram_top(ulong total_size)
 
 	return reg + size;
 }
+#endif
+
 #else
 int dram_init_banksize(void)
 {
diff --git a/boot/image-board.c b/boot/image-board.c
index bf8817165cab..9cdfbc44154c 100644
--- a/boot/image-board.c
+++ b/boot/image-board.c
@@ -546,6 +546,7 @@ int boot_get_ramdisk(int argc, char *const argv[], bootm_headers_t *images,
 	return 0;
 }
 
+#if defined(CONFIG_LMB)
 /**
  * boot_ramdisk_high - relocate init ramdisk
  * @lmb: pointer to lmb handle, will be used for memory mgmt
@@ -639,6 +640,7 @@ int boot_ramdisk_high(struct lmb *lmb, ulong rd_data, ulong rd_len,
 error:
 	return -1;
 }
+#endif
 
 int boot_get_setup(bootm_headers_t *images, u8 arch,
 		   ulong *setup_start, ulong *setup_len)
@@ -832,6 +834,7 @@ int boot_get_loadable(int argc, char *const argv[], bootm_headers_t *images,
 	return 0;
 }
 
+#if defined(CONFIG_LMB)
 /**
  * boot_get_cmdline - allocate and initialize kernel cmdline
  * @lmb: pointer to lmb handle, will be used for memory mgmt
@@ -939,6 +942,7 @@ int image_setup_linux(bootm_headers_t *images)
 
 	return 0;
 }
+#endif
 
 void genimg_print_size(uint32_t size)
 {
-- 
2.34.1

Re: [PATCH 1/2] lmb: Fix lmb property's defination under struct lmb

[Patrick DELAUNAY]

Hi,

On 1/14/22 1:14 PM, Michal Simek wrote:
> From: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>
>
> Under struct lmb {} the lmb property's should be defined only if
> CONFIG_LMB_MEMORY_REGIONS is defined.
>
> Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
>
>   include/lmb.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/lmb.h b/include/lmb.h
> index ab277ca80004..1476d78c2823 100644
> --- a/include/lmb.h
> +++ b/include/lmb.h
> @@ -68,7 +68,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_MEMORY_REGIONS)
>   	struct lmb_property memory_regions[CONFIG_LMB_MEMORY_REGIONS];
>   	struct lmb_property reserved_regions[CONFIG_LMB_RESERVED_REGIONS];
>   #endif


I think this patch don't change the exiting code

because in lib/Kconfig we have also the dependancy with LMB_USE_MAX_REGIONS:

   config LMB_MEMORY_REGIONS
       int "Number of memory regions in lmb lib"
       depends on LMB && !LMB_USE_MAX_REGIONS

=> memory_regions and reserved_regions are needed in struc lmb
    only if CONFIG_LMB_USE_MAX_REGIONS is not defined
    else it is defined in "struct lmb_region" under the SAME compilation flag


struct lmb_region {
	unsigned long cnt;
	unsigned long max;
#if IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS)
	struct lmb_property region[CONFIG_LMB_MAX_REGIONS];
#else
	struct lmb_property *region;
#endif
};

with .region = pointer to 'memory_regions[]' or 'reserved_regions[]' in "struct lmb"

I think it is more clear to have the compilation flag in "struct 
lmb_region" and in "struct lmb_region".

but I have no objection to change it.


PS: I introduce this flag to keep the previous behavior and previous 
struct size on other platform

       when I push the commit 6d66502bc741 ("lmb: Add 2 config to define 
the max number of regions")

Regards

Patrick

Re: [PATCH 1/2] lmb: Fix lmb property's defination under struct lmb

[Michal Simek]

On 1/17/22 17:23, Patrick DELAUNAY wrote:
> Hi,
> 
> On 1/14/22 1:14 PM, Michal Simek wrote:
>> From: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>
>>
>> Under struct lmb {} the lmb property's should be defined only if
>> CONFIG_LMB_MEMORY_REGIONS is defined.
>>
>> Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>
>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>> ---
>>
>>   include/lmb.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/lmb.h b/include/lmb.h
>> index ab277ca80004..1476d78c2823 100644
>> --- a/include/lmb.h
>> +++ b/include/lmb.h
>> @@ -68,7 +68,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_MEMORY_REGIONS)
>>       struct lmb_property memory_regions[CONFIG_LMB_MEMORY_REGIONS];
>>       struct lmb_property reserved_regions[CONFIG_LMB_RESERVED_REGIONS];
>>   #endif
> 
> 
> I think this patch don't change the exiting code
> 
> because in lib/Kconfig we have also the dependancy with LMB_USE_MAX_REGIONS:
> 
>    config LMB_MEMORY_REGIONS
>        int "Number of memory regions in lmb lib"
>        depends on LMB && !LMB_USE_MAX_REGIONS
> 
> => memory_regions and reserved_regions are needed in struc lmb
>     only if CONFIG_LMB_USE_MAX_REGIONS is not defined
>     else it is defined in "struct lmb_region" under the SAME compilation flag
> 
> 
> struct lmb_region {
>      unsigned long cnt;
>      unsigned long max;
> #if IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS)
>      struct lmb_property region[CONFIG_LMB_MAX_REGIONS];
> #else
>      struct lmb_property *region;
> #endif
> };
> 
> with .region = pointer to 'memory_regions[]' or 'reserved_regions[]' in "struct 
> lmb"
> 
> I think it is more clear to have the compilation flag in "struct lmb_region" and 
> in "struct lmb_region".
> 
> but I have no objection to change it.
> 
> 
> PS: I introduce this flag to keep the previous behavior and previous struct size 
> on other platform
> 
>        when I push the commit 6d66502bc741 ("lmb: Add 2 config to define the max 
> number of regions")
> 

When I run make xilinx_zynqmp_virt_defconfig and then disable LMB I am getting 
compilation failure just simply because of CONFIG_LMB_USE_MAX_REGIONS is 
disabled but because there is
#if !IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS)
which is simply #if 1
but CONFIG_LMB_MEMORY_REGIONS is not defined.

That's why patch is just correcting that if statement to be aligned with macro 
which should be defined.

Thanks,
Michal

Re: [PATCH 1/2] lmb: Fix lmb property's defination under struct lmb

[Heinrich Schuchardt]

On 1/17/22 17:23, Patrick DELAUNAY wrote:
> Hi,
>
> On 1/14/22 1:14 PM, Michal Simek wrote:
>> From: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>
>>
>> Under struct lmb {} the lmb property's should be defined only if
>> CONFIG_LMB_MEMORY_REGIONS is defined.
>>
>> Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>
>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>> ---
>>
>>   include/lmb.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/lmb.h b/include/lmb.h
>> index ab277ca80004..1476d78c2823 100644
>> --- a/include/lmb.h
>> +++ b/include/lmb.h
>> @@ -68,7 +68,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_MEMORY_REGIONS)
>>       struct lmb_property memory_regions[CONFIG_LMB_MEMORY_REGIONS];
>>       struct lmb_property reserved_regions[CONFIG_LMB_RESERVED_REGIONS];
>>   #endif
>
>
> I think this patch don't change the exiting code
>
> because in lib/Kconfig we have also the dependancy with
> LMB_USE_MAX_REGIONS:
>
>    config LMB_MEMORY_REGIONS
>        int "Number of memory regions in lmb lib"
>        depends on LMB && !LMB_USE_MAX_REGIONS
>
> => memory_regions and reserved_regions are needed in struc lmb
>     only if CONFIG_LMB_USE_MAX_REGIONS is not defined
>     else it is defined in "struct lmb_region" under the SAME compilation
> flag
>
>
> struct lmb_region {
>      unsigned long cnt;
>      unsigned long max;
> #if IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS)
>      struct lmb_property region[CONFIG_LMB_MAX_REGIONS];
> #else
>      struct lmb_property *region;
> #endif
> };
>
> with .region = pointer to 'memory_regions[]' or 'reserved_regions[]' in
> "struct lmb"
>
> I think it is more clear to have the compilation flag in "struct
> lmb_region" and in "struct lmb_region".
>
> but I have no objection to change it.
>
>
> PS: I introduce this flag to keep the previous behavior and previous
> struct size on other platform
>
>        when I push the commit 6d66502bc741 ("lmb: Add 2 config to define
> the max number of regions")
>
> Regards
>
> Patrick
>
>

Should we really use #ifdef for these structures to save some bytes on
the stack. Isn't it preferable to move the code from using '#ifdef
CONFIG' to 'if IS_ENABLED(CONFIG'?

Best regards

Heinrich

Re: [PATCH 1/2] lmb: Fix lmb property's defination under struct lmb

[Michal Simek]

pá 14. 1. 2022 v 13:14 odesílatel Michal Simek <michal.simek@xilinx.com> napsal:
>
> From: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>
>
> Under struct lmb {} the lmb property's should be defined only if
> CONFIG_LMB_MEMORY_REGIONS is defined.
>
> Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
>
>  include/lmb.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/lmb.h b/include/lmb.h
> index ab277ca80004..1476d78c2823 100644
> --- a/include/lmb.h
> +++ b/include/lmb.h
> @@ -68,7 +68,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_MEMORY_REGIONS)
>         struct lmb_property memory_regions[CONFIG_LMB_MEMORY_REGIONS];
>         struct lmb_property reserved_regions[CONFIG_LMB_RESERVED_REGIONS];
>  #endif
> --
> 2.34.1
>

applied.
M

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs

Re: [PATCH 2/2] zynqmp: Run board_get_usable_ram_top() only on main U-Boot

[Michal Simek]

pá 14. 1. 2022 v 13:14 odesílatel Michal Simek <michal.simek@xilinx.com> napsal:
>
> From: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>
>
> With commit ce39ee28ec31 ("zynqmp: Do not place u-boot to reserved memory
> location"), the function board_get_usable_ram_top() is allocating
> MMU_SECTION_SIZE of about 2MB using lmb_alloc(). But we dont have this
> much memory in case of mini U-Boot.
>
> Keep these functions which use lmb under CONFIG_LMB so that they are
> compiled and used only when LMB is enabled.
>
> Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
>
>  board/xilinx/zynqmp/zynqmp.c | 3 +++
>  boot/image-board.c           | 4 ++++
>  2 files changed, 7 insertions(+)
>
> diff --git a/board/xilinx/zynqmp/zynqmp.c b/board/xilinx/zynqmp/zynqmp.c
> index 2b5239ccb475..c69fb939a83e 100644
> --- a/board/xilinx/zynqmp/zynqmp.c
> +++ b/board/xilinx/zynqmp/zynqmp.c
> @@ -483,6 +483,7 @@ int dram_init(void)
>         return 0;
>  }
>
> +#if defined(CONFIG_LMB)
>  ulong board_get_usable_ram_top(ulong total_size)
>  {
>         phys_size_t size;
> @@ -504,6 +505,8 @@ ulong board_get_usable_ram_top(ulong total_size)
>
>         return reg + size;
>  }
> +#endif
> +
>  #else
>  int dram_init_banksize(void)
>  {
> diff --git a/boot/image-board.c b/boot/image-board.c
> index bf8817165cab..9cdfbc44154c 100644
> --- a/boot/image-board.c
> +++ b/boot/image-board.c
> @@ -546,6 +546,7 @@ int boot_get_ramdisk(int argc, char *const argv[], bootm_headers_t *images,
>         return 0;
>  }
>
> +#if defined(CONFIG_LMB)
>  /**
>   * boot_ramdisk_high - relocate init ramdisk
>   * @lmb: pointer to lmb handle, will be used for memory mgmt
> @@ -639,6 +640,7 @@ int boot_ramdisk_high(struct lmb *lmb, ulong rd_data, ulong rd_len,
>  error:
>         return -1;
>  }
> +#endif
>
>  int boot_get_setup(bootm_headers_t *images, u8 arch,
>                    ulong *setup_start, ulong *setup_len)
> @@ -832,6 +834,7 @@ int boot_get_loadable(int argc, char *const argv[], bootm_headers_t *images,
>         return 0;
>  }
>
> +#if defined(CONFIG_LMB)
>  /**
>   * boot_get_cmdline - allocate and initialize kernel cmdline
>   * @lmb: pointer to lmb handle, will be used for memory mgmt
> @@ -939,6 +942,7 @@ int image_setup_linux(bootm_headers_t *images)
>
>         return 0;
>  }
> +#endif
>
>  void genimg_print_size(uint32_t size)
>  {
> --
> 2.34.1
>

Applied.
M

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs

Re: [PATCH 1/2] lmb: Fix lmb property's defination under struct lmb

[Michal Simek]

po 7. 2. 2022 v 10:40 odesílatel Michal Simek <monstr@monstr.eu> napsal:
>
> pá 14. 1. 2022 v 13:14 odesílatel Michal Simek <michal.simek@xilinx.com> napsal:
> >
> > From: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>
> >
> > Under struct lmb {} the lmb property's should be defined only if
> > CONFIG_LMB_MEMORY_REGIONS is defined.
> >
> > Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>
> > Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> > ---
> >
> >  include/lmb.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/lmb.h b/include/lmb.h
> > index ab277ca80004..1476d78c2823 100644
> > --- a/include/lmb.h
> > +++ b/include/lmb.h
> > @@ -68,7 +68,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_MEMORY_REGIONS)
> >         struct lmb_property memory_regions[CONFIG_LMB_MEMORY_REGIONS];
> >         struct lmb_property reserved_regions[CONFIG_LMB_RESERVED_REGIONS];
> >  #endif
> > --
> > 2.34.1
> >
>
> applied.
> M

ci loop found an issue that's why I have sent v2 and remove these
patches from my queue.

M

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs