Message ID | A1484485FD99714DB2AB2C5EF81E7AC2AA764F8C@ORSMSX116.amr.corp.intel.com |
---|---|
State | Superseded |
Delegated to: | Bin Meng |
Headers | show |
Series | x86: Add basic Slim Bootloader payload support | expand |
On Fri, Aug 2, 2019 at 10:03 AM Park, Aiden <aiden.park@intel.com> wrote: > > Slim Bootloader provides memory map info thru its HOB list pointer. > Configure memory size and relocation memory from the HOB data, and > provide e820 entries as well. > - Get memory size from the memory map info HOB > - Set available top memory lower than 4GB for U-Boot relocation > - Provide e820 entries from the memory map info HOB > +#define for_each_memory_map_entry_reversed(iter, entries) \ > + for (iter = entries->count - 1; iter >= 0; iter--) \ > + if (entries->entry[iter].type == E820_RAM) > + It seems you missed my answer to Bin. This is simple incorrect. Checkpatch sometimes is wrong.
On Fri, Aug 2, 2019 at 10:49 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Fri, Aug 2, 2019 at 10:03 AM Park, Aiden <aiden.park@intel.com> wrote: > > > > Slim Bootloader provides memory map info thru its HOB list pointer. > > Configure memory size and relocation memory from the HOB data, and > > provide e820 entries as well. > > - Get memory size from the memory map info HOB > > - Set available top memory lower than 4GB for U-Boot relocation > > - Provide e820 entries from the memory map info HOB > > > +#define for_each_memory_map_entry_reversed(iter, entries) \ > > + for (iter = entries->count - 1; iter >= 0; iter--) \ > > + if (entries->entry[iter].type == E820_RAM) > > + > > It seems you missed my answer to Bin. > This is simple incorrect. Checkpatch sometimes is wrong. Let me elaborate why. The idea of having if (foo) {} else pattern is to avoid weirndess like for_each_...() { ... } else { ...WTF!.. }
Hi Andy, > -----Original Message----- > From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com] > Sent: Friday, August 2, 2019 4:51 PM > To: Park, Aiden <aiden.park@intel.com> > Cc: Bin Meng <bmeng.cn@gmail.com>; U-Boot Mailing List <u- > boot@lists.denx.de>; Simon Glass <sjg@chromium.org> > Subject: Re: [PATCH v8 4/9] x86: slimbootloader: Add memory > configuration > > On Fri, Aug 2, 2019 at 10:49 AM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > > > On Fri, Aug 2, 2019 at 10:03 AM Park, Aiden <aiden.park@intel.com> > wrote: > > > > > > Slim Bootloader provides memory map info thru its HOB list pointer. > > > Configure memory size and relocation memory from the HOB data, and > > > provide e820 entries as well. > > > - Get memory size from the memory map info HOB > > > - Set available top memory lower than 4GB for U-Boot relocation > > > - Provide e820 entries from the memory map info HOB > > > > > +#define for_each_memory_map_entry_reversed(iter, entries) \ > > > + for (iter = entries->count - 1; iter >= 0; iter--) \ > > > + if (entries->entry[iter].type == E820_RAM) > > > + > > > > It seems you missed my answer to Bin. > > This is simple incorrect. Checkpatch sometimes is wrong. > > Let me elaborate why. > > The idea of having > > if (foo) {} else > > pattern is to avoid weirndess like > > for_each_...() { > ... > } else { > ...WTF!.. > } > Sorry for missing your comment. I understand '{} else' is better way. It looks checkpatch issue is bigger scope than this one. What about this in this series? #define for_each_if(condition) if (!(condition)) {} else #define for_each_memory_map_entry_reversed(iter, entries) \ for (iter = entries->count - 1; iter >= 0; iter--) \ for_each_if(entries->entry[iter].type == E820_RAM) tricky, but checkpatch does not report ERROR. > > -- > With Best Regards, > Andy Shevchenko Best Regards, Aiden
On Fri, Aug 2, 2019 at 1:19 PM Park, Aiden <aiden.park@intel.com> wrote: > > -----Original Message----- > > From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com] > > Sent: Friday, August 2, 2019 4:51 PM > > To: Park, Aiden <aiden.park@intel.com> > > Cc: Bin Meng <bmeng.cn@gmail.com>; U-Boot Mailing List <u- > > boot@lists.denx.de>; Simon Glass <sjg@chromium.org> > > Subject: Re: [PATCH v8 4/9] x86: slimbootloader: Add memory > > configuration > > On Fri, Aug 2, 2019 at 10:49 AM Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: > > > On Fri, Aug 2, 2019 at 10:03 AM Park, Aiden <aiden.park@intel.com> > > wrote: > > > > +#define for_each_memory_map_entry_reversed(iter, entries) \ > > > > + for (iter = entries->count - 1; iter >= 0; iter--) \ > > > > + if (entries->entry[iter].type == E820_RAM) > > > > + > > > > > > It seems you missed my answer to Bin. > > > This is simple incorrect. Checkpatch sometimes is wrong. > > > > Let me elaborate why. > > > > The idea of having > > > > if (foo) {} else > > > > pattern is to avoid weirndess like > > > > for_each_...() { > > ... > > } else { > > ...WTF!.. > > } > > > Sorry for missing your comment. I understand '{} else' is better way. > It looks checkpatch issue is bigger scope than this one. > What about this in this series? > > #define for_each_if(condition) if (!(condition)) {} else > #define for_each_memory_map_entry_reversed(iter, entries) \ > for (iter = entries->count - 1; iter >= 0; iter--) \ > for_each_if(entries->entry[iter].type == E820_RAM) > > tricky, but checkpatch does not report ERROR. Fine for me. If Bin is okay with it, it will be good then.
On Fri, Aug 2, 2019 at 6:29 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Fri, Aug 2, 2019 at 1:19 PM Park, Aiden <aiden.park@intel.com> wrote: > > > -----Original Message----- > > > From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com] > > > Sent: Friday, August 2, 2019 4:51 PM > > > To: Park, Aiden <aiden.park@intel.com> > > > Cc: Bin Meng <bmeng.cn@gmail.com>; U-Boot Mailing List <u- > > > boot@lists.denx.de>; Simon Glass <sjg@chromium.org> > > > Subject: Re: [PATCH v8 4/9] x86: slimbootloader: Add memory > > > configuration > > > On Fri, Aug 2, 2019 at 10:49 AM Andy Shevchenko > > > <andy.shevchenko@gmail.com> wrote: > > > > On Fri, Aug 2, 2019 at 10:03 AM Park, Aiden <aiden.park@intel.com> > > > wrote: > > > > > > +#define for_each_memory_map_entry_reversed(iter, entries) \ > > > > > + for (iter = entries->count - 1; iter >= 0; iter--) \ > > > > > + if (entries->entry[iter].type == E820_RAM) > > > > > + > > > > > > > > It seems you missed my answer to Bin. > > > > This is simple incorrect. Checkpatch sometimes is wrong. > > > > > > Let me elaborate why. > > > > > > The idea of having > > > > > > if (foo) {} else > > > > > > pattern is to avoid weirndess like > > > > > > for_each_...() { > > > ... > > > } else { > > > ...WTF!.. > > > } > > > > > Sorry for missing your comment. I understand '{} else' is better way. > > It looks checkpatch issue is bigger scope than this one. > > What about this in this series? > > > > #define for_each_if(condition) if (!(condition)) {} else > > #define for_each_memory_map_entry_reversed(iter, entries) \ > > for (iter = entries->count - 1; iter >= 0; iter--) \ > > for_each_if(entries->entry[iter].type == E820_RAM) > > > > tricky, but checkpatch does not report ERROR. > > Fine for me. If Bin is okay with it, it will be good then. I am good too. Thanks for working on fixing checkpatch warnings! Regards, Bin
diff --git a/arch/x86/cpu/slimbootloader/Makefile b/arch/x86/cpu/slimbootloader/Makefile index 627a721e8c..9e6822a6ec 100644 --- a/arch/x86/cpu/slimbootloader/Makefile +++ b/arch/x86/cpu/slimbootloader/Makefile @@ -2,4 +2,4 @@ # # Copyright (C) 2019 Intel Corporation <www.intel.com> -obj-y += car.o slimbootloader.o +obj-y += car.o slimbootloader.o sdram.o diff --git a/arch/x86/cpu/slimbootloader/sdram.c b/arch/x86/cpu/slimbootloader/sdram.c new file mode 100644 index 0000000000..2b2b220783 --- /dev/null +++ b/arch/x86/cpu/slimbootloader/sdram.c @@ -0,0 +1,149 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright (C) 2019 Intel Corporation <www.intel.com> + */ + +#include <common.h> +#include <linux/sizes.h> +#include <asm/e820.h> +#include <asm/arch/slimbootloader.h> + +DECLARE_GLOBAL_DATA_PTR; + +/** + * This returns a data pointer of memory map info from the guid hob. + * + * @return: A data pointer of memory map info hob + */ +static struct sbl_memory_map_info *get_memory_map_info(void) +{ + struct sbl_memory_map_info *data; + const efi_guid_t guid = SBL_MEMORY_MAP_INFO_GUID; + + if (!gd->arch.hob_list) + return NULL; + + data = hob_get_guid_hob_data(gd->arch.hob_list, NULL, &guid); + if (!data) + panic("memory map info hob not found\n"); + if (!data->count) + panic("invalid number of memory map entries\n"); + + return data; +} + +#define for_each_memory_map_entry_reversed(iter, entries) \ + for (iter = entries->count - 1; iter >= 0; iter--) \ + if (entries->entry[iter].type == E820_RAM) + +/** + * This is to give usable memory region information for u-boot relocation. + * so search usable memory region lower than 4GB. + * The memory map entries from Slim Bootloader hob are already sorted. + * + * @total_size: The memory size that u-boot occupies + * @return : The top available memory address lower than 4GB + */ +ulong board_get_usable_ram_top(ulong total_size) +{ + struct sbl_memory_map_info *data; + int i; + u64 addr_start; + u64 addr_end; + ulong ram_top; + + data = get_memory_map_info(); + + /** + * sorted memory map entries from Slim Bootloader based on physical + * start memory address, from low to high. So do reversed search to + * get highest usable, suitable size, 4KB aligned available memory + * under 4GB. + */ + ram_top = 0; + for_each_memory_map_entry_reversed(i, data) { + addr_start = data->entry[i].addr; + addr_end = addr_start + data->entry[i].size; + + if (addr_start > SZ_4G) + continue; + + if (addr_end > SZ_4G) + addr_end = SZ_4G; + + if (addr_end < total_size) + continue; + + /* to relocate u-boot at 4K aligned memory */ + addr_end = rounddown(addr_end - total_size, SZ_4K); + if (addr_end >= addr_start) { + ram_top = (ulong)addr_end + total_size; + break; + } + } + + if (!ram_top) + panic("failed to find available memory for relocation!"); + + return ram_top; +} + +/** + * The memory initialization has already been done in previous Slim Bootloader + * stage thru FSP-M. Instead, this sets the ram_size from the memory map info + * hob. + */ +int dram_init(void) +{ + struct sbl_memory_map_info *data; + int i; + u64 ram_size; + + data = get_memory_map_info(); + + /** + * sorted memory map entries from Slim Bootloader based on physical + * start memory address, from low to high. So do reversed search to + * simply get highest usable memory address as RAM size + */ + ram_size = 0; + for_each_memory_map_entry_reversed(i, data) { + /* simply use the highest usable memory address as RAM size */ + ram_size = data->entry[i].addr + data->entry[i].size; + break; + } + + if (!ram_size) + panic("failed to detect memory size"); + + gd->ram_size = ram_size; + return 0; +} + +int dram_init_banksize(void) +{ + if (!CONFIG_NR_DRAM_BANKS) + return 0; + + /* simply use a single bank to have whole size for now */ + gd->bd->bi_dram[0].start = 0; + gd->bd->bi_dram[0].size = gd->ram_size; + return 0; +} + +unsigned int install_e820_map(unsigned int max_entries, + struct e820_entry *entries) +{ + struct sbl_memory_map_info *data; + unsigned int i; + + data = get_memory_map_info(); + + for (i = 0; i < data->count; i++) { + entries[i].addr = data->entry[i].addr; + entries[i].size = data->entry[i].size; + entries[i].type = data->entry[i].type; + } + + return i; +} diff --git a/arch/x86/include/asm/arch-slimbootloader/slimbootloader.h b/arch/x86/include/asm/arch-slimbootloader/slimbootloader.h new file mode 100644 index 0000000000..4ee8a0627e --- /dev/null +++ b/arch/x86/include/asm/arch-slimbootloader/slimbootloader.h @@ -0,0 +1,53 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/* + * Copyright (C) 2019 Intel Corporation <www.intel.com> + */ + +#ifndef __SLIMBOOTLOADER_ARCH_H__ +#define __SLIMBOOTLOADER_ARCH_H__ + +#include <common.h> +#include <asm/hob.h> + +/** + * A GUID to get MemoryMap info hob which is provided by Slim Bootloader + */ +#define SBL_MEMORY_MAP_INFO_GUID \ + EFI_GUID(0xa1ff7424, 0x7a1a, 0x478e, \ + 0xa9, 0xe4, 0x92, 0xf3, 0x57, 0xd1, 0x28, 0x32) + +/** + * A single entry of memory map information + * + * @addr: start address of a memory map entry + * @size: size of a memory map entry + * @type: usable:1, reserved:2, acpi:3, nvs:4, unusable:5 + * @flag: only used in Slim Bootloader + * @rsvd: padding for alignment + */ +struct sbl_memory_map_entry { + u64 addr; + u64 size; + u8 type; + u8 flag; + u8 rsvd[6]; +}; + +/** + * This includes all memory map entries which is sorted based on physical start + * address, from low to high, and carved out reserved, acpi nvs, acpi reclaim + * and usable memory. + * + * @rev : revision of memory_map_info structure. currently 1. + * @rsvd : padding for alignment + * @count: the number of memory map entries + * @entry: array of all memory map entries + */ +struct sbl_memory_map_info { + u8 rev; + u8 rsvd[3]; + u32 count; + struct sbl_memory_map_entry entry[0]; +}; + +#endif /* __SLIMBOOTLOADER_ARCH_H__ */