[U-Boot] [PATCH] x86: Clean up the FSP support codes
Bin Meng
bmeng.cn at gmail.com
Mon Dec 15 06:42:37 CET 2014
Hi Simon,
On Sun, Dec 14, 2014 at 2:02 PM, Simon Glass <sjg at chromium.org> wrote:
> Hi Bin,
>
> On 13 December 2014 at 21:15, Bin Meng <bmeng.cn at gmail.com> wrote:
>> This is the follow-on patch to clean up the FSP support codes:
>>
>> - Remove the _t suffix on the structures defines
>> - Use U-Boot's assert()
>> - Use standard bool true/false
>> - Remove read_unaligned64()
>> - Use memcmp() in the compare_guid()
>> - Remove the cast in the memset() call
>> - Replace some magic numbers with macros
>> - Use panic() when no valid FSP image header is found
>> - Change some FSP utility routines to use an fsp_ prefix
>> - Add comment blocks for asm_continuation and fsp_init_done
>> - Add comments to mention find_fsp_header() may be called in a
>> stackless environment
>> - Add comments to mention init(¶ms) in fsp_init() cannot
>> be removed
>>
>> Signed-off-by: Bin Meng <bmeng.cn at gmail.com>
>> ---
>>
>> arch/x86/cpu/queensbay/fsp_configs.c | 2 +-
>> arch/x86/cpu/queensbay/fsp_support.c | 216 +++++++++------------
>> arch/x86/cpu/queensbay/tnc_dram.c | 6 +-
>> arch/x86/include/asm/arch-queensbay/fsp/fsp_api.h | 14 +-
>> arch/x86/include/asm/arch-queensbay/fsp/fsp_ffs.h | 24 +--
>> arch/x86/include/asm/arch-queensbay/fsp/fsp_fv.h | 14 +-
>> arch/x86/include/asm/arch-queensbay/fsp/fsp_hob.h | 52 ++---
>> .../asm/arch-queensbay/fsp/fsp_infoheader.h | 2 +-
>> .../include/asm/arch-queensbay/fsp/fsp_platform.h | 4 +-
>> .../include/asm/arch-queensbay/fsp/fsp_support.h | 63 +++---
>> .../x86/include/asm/arch-queensbay/fsp/fsp_types.h | 17 +-
>> arch/x86/include/asm/arch-queensbay/fsp/fsp_vpd.h | 6 +-
>> arch/x86/lib/cmd_hob.c | 2 +-
>> 13 files changed, 198 insertions(+), 224 deletions(-)
>
> Yes this looks good. I will take this patch once we figure out the
> microcode problem as emailed earlier.
>
> I have a few more comments below also - hoping to simplify a little further.
>
>>
>> diff --git a/arch/x86/cpu/queensbay/fsp_configs.c b/arch/x86/cpu/queensbay/fsp_configs.c
>> index aef18fc..af28e45 100644
>> --- a/arch/x86/cpu/queensbay/fsp_configs.c
>> +++ b/arch/x86/cpu/queensbay/fsp_configs.c
>> @@ -8,7 +8,7 @@
>> #include <common.h>
>> #include <asm/arch/fsp/fsp_support.h>
>>
>> -void update_fsp_upd(struct upd_region_t *fsp_upd)
>> +void update_fsp_upd(struct upd_region *fsp_upd)
>> {
>> /* Override any UPD setting if required */
>>
>> diff --git a/arch/x86/cpu/queensbay/fsp_support.c b/arch/x86/cpu/queensbay/fsp_support.c
>> index f830eeb..dc2de71 100644
>> --- a/arch/x86/cpu/queensbay/fsp_support.c
>> +++ b/arch/x86/cpu/queensbay/fsp_support.c
>> @@ -10,67 +10,48 @@
>> #include <asm/post.h>
>>
>> /**
>> - * Reads a 64-bit value from memory that may be unaligned.
>> - *
>> - * This function returns the 64-bit value pointed to by buf. The function
>> - * guarantees that the read operation does not produce an alignment fault.
>> - *
>> - * If the buf is NULL, then ASSERT().
>> - *
>> - * @buf: Pointer to a 64-bit value that may be unaligned.
>> - *
>> - * @return: The 64-bit value read from buf.
>> - */
>> -static u64 read_unaligned64(const u64 *buf)
>> -{
>> - ASSERT(buf != NULL);
>> -
>> - return *buf;
>> -}
>> -
>> -/**
>> * Compares two GUIDs
>> *
>> - * If the GUIDs are identical then TRUE is returned.
>> - * If there are any bit differences in the two GUIDs, then FALSE is returned.
>> - *
>> - * If guid1 is NULL, then ASSERT().
>> - * If guid2 is NULL, then ASSERT().
>> + * If the GUIDs are identical then true is returned.
>> + * If there are any bit differences in the two GUIDs, then false is returned.
>> *
>> * @guid1: A pointer to a 128 bit GUID.
>> * @guid2: A pointer to a 128 bit GUID.
>> *
>> - * @retval TRUE: guid1 and guid2 are identical.
>> - * @retval FALSE: guid1 and guid2 are not identical.
>> + * @retval true: guid1 and guid2 are identical.
>> + * @retval false: guid1 and guid2 are not identical.
>> */
>> -static unsigned char compare_guid(const struct efi_guid_t *guid1,
>> - const struct efi_guid_t *guid2)
>> +static bool compare_guid(const struct efi_guid *guid1,
>> + const struct efi_guid *guid2)
>> {
>> - u64 guid1_low;
>> - u64 guid2_low;
>> - u64 guid1_high;
>> - u64 guid2_high;
>> -
>> - guid1_low = read_unaligned64((const u64 *)guid1);
>> - guid2_low = read_unaligned64((const u64 *)guid2);
>> - guid1_high = read_unaligned64((const u64 *)guid1 + 1);
>> - guid2_high = read_unaligned64((const u64 *)guid2 + 1);
>> -
>> - return (unsigned char)(guid1_low == guid2_low && guid1_high == guid2_high);
>> + if (memcmp(guid1, guid2, sizeof(struct efi_guid)) == 0)
>> + return true;
>> + else
>> + return false;
>> }
>>
>> u32 __attribute__((optimize("O0"))) find_fsp_header(void)
>> {
>> + /*
>> + * This function may be called before the a stack is established,
>> + * so special care must be taken. First, it cannot declare any local
>> + * variable using stack. Only register variable can be used here.
>> + * Secondly, some compiler version will add prolog or epilog code
>> + * for the C function. If so the function call may not work before
>> + * stack is ready.
>
> OK, understood. This is supremely ugly - from what I understand this
> function will write to the ROM stack, fail, and it doesn't matter
> since the return address is already there?
No, this function will not write anything to the ROM stack. It only
declares a register variable fsp which is used as a pointer to move
within the FSP image header.
>> + *
>> + * GCC 4.8.1 has been verified to be working for the following codes.
>> + */
>> volatile register u8 *fsp asm("eax");
>>
>> /* Initalize the FSP base */
>> fsp = (u8 *)CONFIG_FSP_ADDR;
>>
>> /* Check the FV signature, _FVH */
>> - if (((struct fv_header_t *)fsp)->sign == 0x4856465F) {
>> + if (((struct fv_header *)fsp)->sign == EFI_FVH_SIGNATURE) {
>> /* Go to the end of the FV header and align the address */
>> - fsp += ((struct fv_header_t *)fsp)->ext_hdr_off;
>> - fsp += ((struct fv_ext_header_t *)fsp)->ext_hdr_size;
>> + fsp += ((struct fv_header *)fsp)->ext_hdr_off;
>> + fsp += ((struct fv_ext_header *)fsp)->ext_hdr_size;
>> fsp = (u8 *)(((u32)fsp + 7) & 0xFFFFFFF8);
>> } else {
>> fsp = 0;
>> @@ -78,20 +59,20 @@ u32 __attribute__((optimize("O0"))) find_fsp_header(void)
>>
>> /* Check the FFS GUID */
>> if (fsp &&
>> - (((u32 *)&(((struct ffs_file_header_t *)fsp)->name))[0] == 0x912740BE) &&
>> - (((u32 *)&(((struct ffs_file_header_t *)fsp)->name))[1] == 0x47342284) &&
>> - (((u32 *)&(((struct ffs_file_header_t *)fsp)->name))[2] == 0xB08471B9) &&
>> - (((u32 *)&(((struct ffs_file_header_t *)fsp)->name))[3] == 0x0C3F3527)) {
>> + (((u32 *)&(((struct ffs_file_header *)fsp)->name))[0] == 0x912740BE) &&
>> + (((u32 *)&(((struct ffs_file_header *)fsp)->name))[1] == 0x47342284) &&
>> + (((u32 *)&(((struct ffs_file_header *)fsp)->name))[2] == 0xB08471B9) &&
>> + (((u32 *)&(((struct ffs_file_header *)fsp)->name))[3] == 0x0C3F3527)) {
>
> Perhaps another way to fix this code. Since you have gone to the
> trouble of creating struct efi_guid, we could compare each field
> against FSP_GUID_DATA1, FSP_GUID_DATA2, etc. Also so far as I can see
> the data4 field is never used. Perhaps it could be changed to two
> 32-bit fields to simplify this?
I may have a try. I think the efi_guid is defined this way per the EFI
spec, so we'd better keep it as is. The data4 field is referenced as 8
bytes which you can see from those GUID macros like
FSP_HOB_RESOURCE_OWNER_GRAPHICS_GUID.
>
>> /* Add the FFS header size to find the raw section header */
>> - fsp += sizeof(struct ffs_file_header_t);
>> + fsp += sizeof(struct ffs_file_header);
>> } else {
>> fsp = 0;
>> }
>>
>> if (fsp &&
>> - ((struct raw_section_t *)fsp)->type == EFI_SECTION_RAW) {
>> + ((struct raw_section *)fsp)->type == EFI_SECTION_RAW) {
>> /* Add the raw section header size to find the FSP header */
>> - fsp += sizeof(struct raw_section_t);
>> + fsp += sizeof(struct raw_section);
>> } else {
>> fsp = 0;
>> }
>> @@ -99,7 +80,7 @@ u32 __attribute__((optimize("O0"))) find_fsp_header(void)
>> return (u32)fsp;
>> }
>>
>> -void fsp_continue(struct shared_data_t *shared_data, u32 status, void *hob_list)
>> +void fsp_continue(struct shared_data *shared_data, u32 status, void *hob_list)
>> {
>> u32 stack_len;
>> u32 stack_base;
>> @@ -107,18 +88,18 @@ void fsp_continue(struct shared_data_t *shared_data, u32 status, void *hob_list)
>>
>> post_code(POST_MRC);
>>
>> - ASSERT(status == 0);
>> + assert(status == 0);
>>
>> /* Get the migrated stack in normal memory */
>> - stack_base = (u32)get_bootloader_tmp_mem(hob_list, &stack_len);
>> - ASSERT(stack_base != 0);
>> + stack_base = (u32)fsp_get_bootloader_tmp_mem(hob_list, &stack_len);
>> + assert(stack_base != 0);
>> stack_top = stack_base + stack_len - sizeof(u32);
>>
>> /*
>> * Old stack base is stored at the very end of the stack top,
>> * use it to calculate the migrated shared data base
>> */
>> - shared_data = (struct shared_data_t *)(stack_base +
>> + shared_data = (struct shared_data *)(stack_base +
>> ((u32)shared_data - *(u32 *)stack_top));
>>
>> /* The boot loader main function entry */
>> @@ -127,50 +108,50 @@ void fsp_continue(struct shared_data_t *shared_data, u32 status, void *hob_list)
>>
>> void fsp_init(u32 stack_top, u32 boot_mode, void *nvs_buf)
>> {
>> - struct shared_data_t shared_data;
>> + struct shared_data shared_data;
>> fsp_init_f init;
>> - struct fsp_init_params_t params;
>> - struct fspinit_rtbuf_t rt_buf;
>> - struct vpd_region_t *fsp_vpd;
>> - struct fsp_header_t *fsp_hdr;
>> - struct fsp_init_params_t *params_ptr;
>> - struct upd_region_t *fsp_upd;
>> -
>> - fsp_hdr = (struct fsp_header_t *)find_fsp_header();
>> + struct fsp_init_params params;
>> + struct fspinit_rtbuf rt_buf;
>> + struct vpd_region *fsp_vpd;
>> + struct fsp_header *fsp_hdr;
>> + struct fsp_init_params *params_ptr;
>> + struct upd_region *fsp_upd;
>> +
>> + fsp_hdr = (struct fsp_header *)find_fsp_header();
>> if (fsp_hdr == NULL) {
>> /* No valid FSP info header was found */
>> - ASSERT(FALSE);
>> + panic("Invalid FSP header");
>> }
>>
>> - fsp_upd = (struct upd_region_t *)&shared_data.fsp_upd;
>> - memset((void *)&rt_buf, 0, sizeof(struct fspinit_rtbuf_t));
>> + fsp_upd = (struct upd_region *)&shared_data.fsp_upd;
>> + memset(&rt_buf, 0, sizeof(struct fspinit_rtbuf));
>>
>> /* Reserve a gap in stack top */
>> rt_buf.common.stack_top = (u32 *)stack_top - 32;
>> rt_buf.common.boot_mode = boot_mode;
>> - rt_buf.common.upd_data = (struct upd_region_t *)fsp_upd;
>> + rt_buf.common.upd_data = (struct upd_region *)fsp_upd;
>>
>> /* Get VPD region start */
>> - fsp_vpd = (struct vpd_region_t *)(fsp_hdr->img_base +
>> + fsp_vpd = (struct vpd_region *)(fsp_hdr->img_base +
>> fsp_hdr->cfg_region_off);
>>
>> /* Verifify the VPD data region is valid */
>> - ASSERT((fsp_vpd->img_rev == VPD_IMAGE_REV) &&
>> + assert((fsp_vpd->img_rev == VPD_IMAGE_REV) &&
>> (fsp_vpd->sign == VPD_IMAGE_ID));
>>
>> /* Copy default data from Flash */
>> memcpy(fsp_upd, (void *)(fsp_hdr->img_base + fsp_vpd->upd_offset),
>> - sizeof(struct upd_region_t));
>> + sizeof(struct upd_region));
>>
>> /* Verifify the UPD data region is valid */
>> - ASSERT(fsp_upd->terminator == 0x55AA);
>> + assert(fsp_upd->terminator == UPD_TERMINATOR);
>>
>> /* Override any UPD setting if required */
>> update_fsp_upd(fsp_upd);
>>
>> - memset((void *)¶ms, 0, sizeof(struct fsp_init_params_t));
>> + memset(¶ms, 0, sizeof(struct fsp_init_params));
>> params.nvs_buf = nvs_buf;
>> - params.rt_buf = (struct fspinit_rtbuf_t *)&rt_buf;
>> + params.rt_buf = (struct fspinit_rtbuf *)&rt_buf;
>> params.continuation = (fsp_continuation_f)asm_continuation;
>>
>> init = (fsp_init_f)(fsp_hdr->img_base + fsp_hdr->fsp_init);
>> @@ -199,32 +180,28 @@ void fsp_init(u32 stack_top, u32 boot_mode, void *nvs_buf)
>>
>> /*
>> * Should never get here.
>> - * Control will continue from romstage_main_continue_asm.
>> + * Control will continue from fsp_continue.
>> * This line below is to prevent the compiler from optimizing
>> * structure intialization.
>> + *
>> + * DO NOT REMOVE!
>> */
>> init(¶ms);
>> -
>> - /*
>> - * Should never return.
>> - * Control will continue from ContinuationFunc
>> - */
>> - ASSERT(FALSE);
>> }
>>
>> -u32 fsp_notify(struct fsp_header_t *fsp_hdr, u32 phase)
>> +u32 fsp_notify(struct fsp_header *fsp_hdr, u32 phase)
>> {
>> fsp_notify_f notify;
>> - struct fsp_notify_params_t params;
>> - struct fsp_notify_params_t *params_ptr;
>> + struct fsp_notify_params params;
>> + struct fsp_notify_params *params_ptr;
>> u32 status;
>>
>> if (!fsp_hdr)
>> - fsp_hdr = (struct fsp_header_t *)find_fsp_header();
>> + fsp_hdr = (struct fsp_header *)find_fsp_header();
>>
>> if (fsp_hdr == NULL) {
>> /* No valid FSP info header */
>> - ASSERT(FALSE);
>> + panic("Invalid FSP header");
>> }
>>
>> notify = (fsp_notify_f)(fsp_hdr->img_base + fsp_hdr->fsp_notify);
>> @@ -245,9 +222,9 @@ u32 fsp_notify(struct fsp_header_t *fsp_hdr, u32 phase)
>> return status;
>> }
>>
>> -u32 get_usable_lowmem_top(const void *hob_list)
>> +u32 fsp_get_usable_lowmem_top(const void *hob_list)
>> {
>> - union hob_pointers_t hob;
>> + union hob_pointers hob;
>> phys_addr_t phys_start;
>> u32 top;
>>
>> @@ -255,14 +232,14 @@ u32 get_usable_lowmem_top(const void *hob_list)
>> hob.raw = (void *)hob_list;
>>
>> /* * Collect memory ranges */
>> - top = 0x100000;
>> + top = FSP_LOWMEM_BASE;
>> while (!END_OF_HOB(hob)) {
>> if (hob.hdr->type == HOB_TYPE_RES_DESC) {
>> if (hob.res_desc->type == RES_SYS_MEM) {
>> phys_start = hob.res_desc->phys_start;
>> /* Need memory above 1MB to be collected here */
>> - if (phys_start >= 0x100000 &&
>> - phys_start < (phys_addr_t)0x100000000)
>> + if (phys_start >= FSP_LOWMEM_BASE &&
>> + phys_start < (phys_addr_t)FSP_HIGHMEM_BASE)
>> top += (u32)(hob.res_desc->len);
>> }
>> }
>> @@ -272,9 +249,9 @@ u32 get_usable_lowmem_top(const void *hob_list)
>> return top;
>> }
>>
>> -u64 get_usable_highmem_top(const void *hob_list)
>> +u64 fsp_get_usable_highmem_top(const void *hob_list)
>> {
>> - union hob_pointers_t hob;
>> + union hob_pointers hob;
>> phys_addr_t phys_start;
>> u64 top;
>>
>> @@ -282,13 +259,13 @@ u64 get_usable_highmem_top(const void *hob_list)
>> hob.raw = (void *)hob_list;
>>
>> /* Collect memory ranges */
>> - top = 0x100000000;
>> + top = FSP_HIGHMEM_BASE;
>> while (!END_OF_HOB(hob)) {
>> if (hob.hdr->type == HOB_TYPE_RES_DESC) {
>> if (hob.res_desc->type == RES_SYS_MEM) {
>> phys_start = hob.res_desc->phys_start;
>> /* Need memory above 1MB to be collected here */
>> - if (phys_start >= (phys_addr_t)0x100000000)
>> + if (phys_start >= (phys_addr_t)FSP_HIGHMEM_BASE)
>> top += (u32)(hob.res_desc->len);
>> }
>> }
>> @@ -298,10 +275,10 @@ u64 get_usable_highmem_top(const void *hob_list)
>> return top;
>> }
>>
>> -u64 get_fsp_reserved_mem_from_guid(const void *hob_list, u64 *len,
>> - struct efi_guid_t *guid)
>> +u64 fsp_get_reserved_mem_from_guid(const void *hob_list, u64 *len,
>> + struct efi_guid *guid)
>> {
>> - union hob_pointers_t hob;
>> + union hob_pointers hob;
>>
>> /* Get the HOB list for processing */
>> hob.raw = (void *)hob_list;
>> @@ -324,39 +301,39 @@ u64 get_fsp_reserved_mem_from_guid(const void *hob_list, u64 *len,
>> return 0;
>> }
>>
>> -u32 get_fsp_reserved_mem(const void *hob_list, u32 *len)
>> +u32 fsp_get_fsp_reserved_mem(const void *hob_list, u32 *len)
>> {
>> - const struct efi_guid_t guid = FSP_HOB_RESOURCE_OWNER_FSP_GUID;
>> + const struct efi_guid guid = FSP_HOB_RESOURCE_OWNER_FSP_GUID;
>> u64 length;
>> u32 base;
>>
>> - base = (u32)get_fsp_reserved_mem_from_guid(hob_list,
>> - &length, (struct efi_guid_t *)&guid);
>> + base = (u32)fsp_get_reserved_mem_from_guid(hob_list,
>> + &length, (struct efi_guid *)&guid);
>> if ((len != 0) && (base != 0))
>> *len = (u32)length;
>>
>> return base;
>> }
>>
>> -u32 get_tseg_reserved_mem(const void *hob_list, u32 *len)
>> +u32 fsp_get_tseg_reserved_mem(const void *hob_list, u32 *len)
>> {
>> - const struct efi_guid_t guid = FSP_HOB_RESOURCE_OWNER_TSEG_GUID;
>> + const struct efi_guid guid = FSP_HOB_RESOURCE_OWNER_TSEG_GUID;
>> u64 length;
>> u32 base;
>>
>> - base = (u32)get_fsp_reserved_mem_from_guid(hob_list,
>> - &length, (struct efi_guid_t *)&guid);
>> + base = (u32)fsp_get_reserved_mem_from_guid(hob_list,
>> + &length, (struct efi_guid *)&guid);
>> if ((len != 0) && (base != 0))
>> *len = (u32)length;
>>
>> return base;
>> }
>>
>> -void *get_next_hob(u16 type, const void *hob_list)
>> +void *fsp_get_next_hob(u16 type, const void *hob_list)
>> {
>> - union hob_pointers_t hob;
>> + union hob_pointers hob;
>>
>> - ASSERT(hob_list != NULL);
>> + assert(hob_list != NULL);
>>
>> hob.raw = (u8 *)hob_list;
>>
>> @@ -371,12 +348,12 @@ void *get_next_hob(u16 type, const void *hob_list)
>> return NULL;
>> }
>>
>> -void *get_next_guid_hob(const struct efi_guid_t *guid, const void *hob_list)
>> +void *fsp_get_next_guid_hob(const struct efi_guid *guid, const void *hob_list)
>> {
>> - union hob_pointers_t hob;
>> + union hob_pointers hob;
>>
>> hob.raw = (u8 *)hob_list;
>> - while ((hob.raw = get_next_hob(HOB_TYPE_GUID_EXT,
>> + while ((hob.raw = fsp_get_next_hob(HOB_TYPE_GUID_EXT,
>> hob.raw)) != NULL) {
>> if (compare_guid(guid, &hob.guid->name))
>> break;
>> @@ -386,11 +363,12 @@ void *get_next_guid_hob(const struct efi_guid_t *guid, const void *hob_list)
>> return hob.raw;
>> }
>>
>> -void *get_guid_hob_data(const void *hob_list, u32 *len, struct efi_guid_t *guid)
>> +void *fsp_get_guid_hob_data(const void *hob_list, u32 *len,
>> + struct efi_guid *guid)
>> {
>> u8 *guid_hob;
>>
>> - guid_hob = get_next_guid_hob(guid, hob_list);
>> + guid_hob = fsp_get_next_guid_hob(guid, hob_list);
>> if (guid_hob == NULL) {
>> return NULL;
>> } else {
>> @@ -401,16 +379,16 @@ void *get_guid_hob_data(const void *hob_list, u32 *len, struct efi_guid_t *guid)
>> }
>> }
>>
>> -void *get_fsp_nvs_data(const void *hob_list, u32 *len)
>> +void *fsp_get_nvs_data(const void *hob_list, u32 *len)
>> {
>> - const struct efi_guid_t guid = FSP_NON_VOLATILE_STORAGE_HOB_GUID;
>> + const struct efi_guid guid = FSP_NON_VOLATILE_STORAGE_HOB_GUID;
>>
>> - return get_guid_hob_data(hob_list, len, (struct efi_guid_t *)&guid);
>> + return fsp_get_guid_hob_data(hob_list, len, (struct efi_guid *)&guid);
>> }
>>
>> -void *get_bootloader_tmp_mem(const void *hob_list, u32 *len)
>> +void *fsp_get_bootloader_tmp_mem(const void *hob_list, u32 *len)
>> {
>> - const struct efi_guid_t guid = FSP_BOOTLOADER_TEMP_MEM_HOB_GUID;
>> + const struct efi_guid guid = FSP_BOOTLOADER_TEMP_MEM_HOB_GUID;
>>
>> - return get_guid_hob_data(hob_list, len, (struct efi_guid_t *)&guid);
>> + return fsp_get_guid_hob_data(hob_list, len, (struct efi_guid *)&guid);
>> }
>> diff --git a/arch/x86/cpu/queensbay/tnc_dram.c b/arch/x86/cpu/queensbay/tnc_dram.c
>> index dbc1710..378af01 100644
>> --- a/arch/x86/cpu/queensbay/tnc_dram.c
>> +++ b/arch/x86/cpu/queensbay/tnc_dram.c
>> @@ -14,7 +14,7 @@ DECLARE_GLOBAL_DATA_PTR;
>> int dram_init(void)
>> {
>> phys_size_t ram_size = 0;
>> - union hob_pointers_t hob;
>> + union hob_pointers hob;
>>
>> hob.raw = gd->arch.hob_list;
>> while (!END_OF_HOB(hob)) {
>> @@ -49,14 +49,14 @@ void dram_init_banksize(void)
>> */
>> ulong board_get_usable_ram_top(ulong total_size)
>> {
>> - return get_usable_lowmem_top(gd->arch.hob_list);
>> + return fsp_get_usable_lowmem_top(gd->arch.hob_list);
>> }
>>
>> unsigned install_e820_map(unsigned max_entries, struct e820entry *entries)
>> {
>> unsigned num_entries = 0;
>>
>> - union hob_pointers_t hob;
>> + union hob_pointers hob;
>>
>> hob.raw = gd->arch.hob_list;
>>
>> diff --git a/arch/x86/include/asm/arch-queensbay/fsp/fsp_api.h b/arch/x86/include/asm/arch-queensbay/fsp/fsp_api.h
>> index 25b938f..95f0e06 100644
>> --- a/arch/x86/include/asm/arch-queensbay/fsp/fsp_api.h
>> +++ b/arch/x86/include/asm/arch-queensbay/fsp/fsp_api.h
>> @@ -16,7 +16,7 @@ typedef void (*fsp_continuation_f)(u32 status, void *hob_list);
>>
>> #pragma pack(1)
>>
>> -struct fsp_init_params_t {
>> +struct fsp_init_params {
>> /* Non-volatile storage buffer pointer */
>> void *nvs_buf;
>> /* Runtime buffer pointer */
>> @@ -25,7 +25,7 @@ struct fsp_init_params_t {
>> fsp_continuation_f continuation;
>> };
>>
>> -struct common_buf_t {
>> +struct common_buf {
>> /*
>> * Stack top pointer used by the bootloader. The new stack frame will be
>> * set up at this location after FspInit API call.
>> @@ -36,24 +36,24 @@ struct common_buf_t {
>> u32 reserved[7]; /* Reserved */
>> };
>>
>> -enum fsp_phase_t {
>> +enum fsp_phase {
>> /* Notification code for post PCI enuermation */
>> INIT_PHASE_PCI = 0x20,
>> /* Notification code before transfering control to the payload */
>> INIT_PHASE_BOOT = 0x40
>> };
>>
>> -struct fsp_notify_params_t {
>> +struct fsp_notify_params {
>> /* Notification phase used for NotifyPhase API */
>> - enum fsp_phase_t phase;
>> + enum fsp_phase phase;
>> };
>>
>> #pragma pack()
>>
>> /* FspInit API function prototype */
>> -typedef u32 (*fsp_init_f)(struct fsp_init_params_t *param);
>> +typedef u32 (*fsp_init_f)(struct fsp_init_params *params);
>>
>> /* FspNotify API function prototype */
>> -typedef u32 (*fsp_notify_f)(struct fsp_notify_params_t *param);
>> +typedef u32 (*fsp_notify_f)(struct fsp_notify_params *params);
>>
>> #endif
>> diff --git a/arch/x86/include/asm/arch-queensbay/fsp/fsp_ffs.h b/arch/x86/include/asm/arch-queensbay/fsp/fsp_ffs.h
>> index 1f73680..7fef3e7 100644
>> --- a/arch/x86/include/asm/arch-queensbay/fsp/fsp_ffs.h
>> +++ b/arch/x86/include/asm/arch-queensbay/fsp/fsp_ffs.h
>> @@ -11,7 +11,7 @@
>> #pragma pack(1)
>>
>> /* Used to verify the integrity of the file */
>> -union ffs_integrity_t {
>> +union ffs_integrity {
>> struct {
>> /*
>> * The IntegrityCheck.checksum.header field is an 8-bit
>> @@ -43,14 +43,14 @@ union ffs_integrity_t {
>> * Each file begins with the header that describe the
>> * contents and state of the files.
>> */
>> -struct ffs_file_header_t {
>> +struct ffs_file_header {
>> /*
>> * This GUID is the file name.
>> * It is used to uniquely identify the file.
>> */
>> - struct efi_guid_t name;
>> + struct efi_guid name;
>> /* Used to verify the integrity of the file */
>> - union ffs_integrity_t integrity;
>> + union ffs_integrity integrity;
>> /* Identifies the type of file */
>> u8 type;
>> /* Declares various file attribute bits */
>> @@ -64,16 +64,16 @@ struct ffs_file_header_t {
>> u8 state;
>> };
>>
>> -struct ffs_file_header2_t {
>> +struct ffs_file_header2 {
>> /*
>> * This GUID is the file name. It is used to uniquely identify the file.
>> * There may be only one instance of a file with the file name GUID of
>> * Name in any given firmware volume, except if the file type is
>> * EFI_FV_FILE_TYPE_FFS_PAD.
>> */
>> - struct efi_guid_t name;
>> + struct efi_guid name;
>> /* Used to verify the integrity of the file */
>> - union ffs_integrity_t integrity;
>> + union ffs_integrity integrity;
>> /* Identifies the type of file */
>> u8 type;
>> /* Declares various file attribute bits */
>> @@ -81,9 +81,9 @@ struct ffs_file_header2_t {
>> /*
>> * The length of the file in bytes, including the FFS header.
>> * The length of the file data is either
>> - * (size - sizeof(struct ffs_file_header_t)). This calculation means a
>> + * (size - sizeof(struct ffs_file_header)). This calculation means a
>> * zero-length file has a size of 24 bytes, which is
>> - * sizeof(struct ffs_file_header_t). Size is not required to be a
>> + * sizeof(struct ffs_file_header). Size is not required to be a
>> * multiple of 8 bytes. Given a file F, the next file header is located
>> * at the next 8-byte aligned firmware volume offset following the last
>> * byte of the file F.
>> @@ -98,7 +98,7 @@ struct ffs_file_header2_t {
>> * If FFS_ATTRIB_LARGE_FILE is set in attr, then ext_size exists
>> * and size must be set to zero.
>> * If FFS_ATTRIB_LARGE_FILE is not set then
>> - * struct ffs_file_header_t is used.
>> + * struct ffs_file_header is used.
>> */
>> u32 ext_size;
>> };
>> @@ -129,7 +129,7 @@ struct ffs_file_header2_t {
>> #define EFI_SECTION_SMM_DEPEX 0x1C
>>
>> /* Common section header */
>> -struct raw_section_t {
>> +struct raw_section {
>> /*
>> * A 24-bit unsigned integer that contains the total size of
>> * the section in bytes, including the EFI_COMMON_SECTION_HEADER.
>> @@ -138,7 +138,7 @@ struct raw_section_t {
>> u8 type;
>> };
>>
>> -struct raw_section2_t {
>> +struct raw_section2 {
>> /*
>> * A 24-bit unsigned integer that contains the total size of
>> * the section in bytes, including the EFI_COMMON_SECTION_HEADER.
>> diff --git a/arch/x86/include/asm/arch-queensbay/fsp/fsp_fv.h b/arch/x86/include/asm/arch-queensbay/fsp/fsp_fv.h
>> index 01300db..a024451 100644
>> --- a/arch/x86/include/asm/arch-queensbay/fsp/fsp_fv.h
>> +++ b/arch/x86/include/asm/arch-queensbay/fsp/fsp_fv.h
>> @@ -63,7 +63,7 @@
>> #define EFI_FVB2_ALIGNMENT_1G 0x001E0000
>> #define EFI_FVB2_ALIGNMENT_2G 0x001F0000
>>
>> -struct fv_blkmap_entry_t {
>> +struct fv_blkmap_entry {
>> /* The number of sequential blocks which are of the same size */
>> u32 num_blocks;
>> /* The size of the blocks */
>> @@ -71,7 +71,7 @@ struct fv_blkmap_entry_t {
>> };
>>
>> /* Describes the features and layout of the firmware volume */
>> -struct fv_header_t {
>> +struct fv_header {
>> /*
>> * The first 16 bytes are reserved to allow for the reset vector of
>> * processors whose reset vector is at address 0.
>> @@ -81,7 +81,7 @@ struct fv_header_t {
>> * Declares the file system with which the firmware volume
>> * is formatted.
>> */
>> - struct efi_guid_t fs_guid;
>> + struct efi_guid fs_guid;
>> /*
>> * Length in bytes of the complete firmware volume, including
>> * the header.
>> @@ -118,18 +118,18 @@ struct fv_header_t {
>> * An array of run-length encoded FvBlockMapEntry structures.
>> * The array is terminated with an entry of {0,0}.
>> */
>> - struct fv_blkmap_entry_t block_map[1];
>> + struct fv_blkmap_entry block_map[1];
>> };
>>
>> -#define EFI_FVH_SIGNATURE SIGNATURE_32('_', 'F', 'V', 'H')
>> +#define EFI_FVH_SIGNATURE SIGNATURE_32('_', 'F', 'V', 'H')
>>
>> /* Firmware Volume Header Revision definition */
>> #define EFI_FVH_REVISION 0x02
>>
>> /* Extension header pointed by ExtHeaderOffset of volume header */
>> -struct fv_ext_header_t {
>> +struct fv_ext_header {
>> /* firmware volume name */
>> - struct efi_guid_t fv_name;
>> + struct efi_guid fv_name;
>> /* Size of the rest of the extension header including this structure */
>> u32 ext_hdr_size;
>> };
>> diff --git a/arch/x86/include/asm/arch-queensbay/fsp/fsp_hob.h b/arch/x86/include/asm/arch-queensbay/fsp/fsp_hob.h
>> index 44c0f90..b4fc11b 100644
>> --- a/arch/x86/include/asm/arch-queensbay/fsp/fsp_hob.h
>> +++ b/arch/x86/include/asm/arch-queensbay/fsp/fsp_hob.h
>> @@ -19,14 +19,14 @@
>> * Describes the format and size of the data inside the HOB.
>> * All HOBs must contain this generic HOB header.
>> */
>> -struct hob_header_t {
>> +struct hob_header {
>> u16 type; /* HOB type */
>> u16 len; /* HOB length */
>> u32 reserved; /* always zero */
>> };
>>
>> /* Enumeration of memory types introduced in UEFI */
>> -enum efi_mem_type_t {
>> +enum efi_mem_type {
>> EFI_RESERVED_MEMORY_TYPE,
>> /*
>> * The code portions of a loaded application.
>> @@ -87,16 +87,16 @@ enum efi_mem_type_t {
>> * exist outside the HOB list. This HOB type describes how memory is used,
>> * not the physical attributes of memory.
>> */
>> -struct hob_mem_alloc_t {
>> - struct hob_header_t hdr;
>> +struct hob_mem_alloc {
>> + struct hob_header hdr;
>> /*
>> * A GUID that defines the memory allocation region's type and purpose,
>> * as well as other fields within the memory allocation HOB. This GUID
>> * is used to define the additional data within the HOB that may be
>> - * present for the memory allocation HOB. Type efi_guid_t is defined in
>> + * present for the memory allocation HOB. Type efi_guid is defined in
>> * InstallProtocolInterface() in the UEFI 2.0 specification.
>> */
>> - struct efi_guid_t name;
>> + struct efi_guid name;
>> /*
>> * The base address of memory allocated by this HOB.
>> * Type phys_addr_t is defined in AllocatePages() in the UEFI 2.0
>> @@ -111,7 +111,7 @@ struct hob_mem_alloc_t {
>> * Type EFI_MEMORY_TYPE is defined in AllocatePages() in the UEFI 2.0
>> * specification.
>> */
>> - enum efi_mem_type_t mem_type;
>> + enum efi_mem_type mem_type;
>> /* padding */
>> u8 reserved[4];
>> };
>> @@ -155,14 +155,14 @@ struct hob_mem_alloc_t {
>> * Describes the resource properties of all fixed, nonrelocatable resource
>> * ranges found on the processor host bus during the HOB producer phase.
>> */
>> -struct hob_res_desc_t {
>> - struct hob_header_t hdr;
>> +struct hob_res_desc {
>> + struct hob_header hdr;
>> /*
>> * A GUID representing the owner of the resource. This GUID is
>> * used by HOB consumer phase components to correlate device
>> * ownership of a resource.
>> */
>> - struct efi_guid_t owner;
>> + struct efi_guid owner;
>> u32 type;
>> u32 attr;
>> /* The physical start address of the resource region */
>> @@ -175,19 +175,19 @@ struct hob_res_desc_t {
>> * Allows writers of executable content in the HOB producer phase to
>> * maintain and manage HOBs with specific GUID.
>> */
>> -struct hob_guid_t {
>> - struct hob_header_t hdr;
>> +struct hob_guid {
>> + struct hob_header hdr;
>> /* A GUID that defines the contents of this HOB */
>> - struct efi_guid_t name;
>> + struct efi_guid name;
>> /* GUID specific data goes here */
>> };
>>
>> /* Union of all the possible HOB Types */
>> -union hob_pointers_t {
>> - struct hob_header_t *hdr;
>> - struct hob_mem_alloc_t *mem_alloc;
>> - struct hob_res_desc_t *res_desc;
>> - struct hob_guid_t *guid;
>> +union hob_pointers {
>> + struct hob_header *hdr;
>> + struct hob_mem_alloc *mem_alloc;
>> + struct hob_res_desc *res_desc;
>> + struct hob_guid *guid;
>> u8 *raw;
>> };
>>
>> @@ -202,7 +202,7 @@ union hob_pointers_t {
>> * @return: HOB type.
>> */
>> #define GET_HOB_TYPE(hob) \
>> - ((*(struct hob_header_t **)&(hob))->type)
>> + ((*(struct hob_header **)&(hob))->type)
>
> Can we remove all these casts?
>
> How about hob->hdr.type? Also maybe convert these to static inline
> functions? (lower case)?
I think I can simplify this.
>>
>> /**
>> * Returns the length, in bytes, of a HOB.
>> @@ -215,7 +215,7 @@ union hob_pointers_t {
>> * @return: HOB length.
>> */
>> #define GET_HOB_LENGTH(hob) \
>> - ((*(struct hob_header_t **)&(hob))->len)
>> + ((*(struct hob_header **)&(hob))->len)
>
> hob->hdr.len?
This too.
>>
>> /**
>> * Returns a pointer to the next HOB in the HOB list.
>> @@ -234,13 +234,13 @@ union hob_pointers_t {
>> * Determines if a HOB is the last HOB in the HOB list.
>> *
>> * This macro determine if the HOB specified by hob is the last HOB in the
>> - * HOB list. If hob is last HOB in the HOB list, then TRUE is returned.
>> - * Otherwise, FALSE is returned.
>> + * HOB list. If hob is last HOB in the HOB list, then true is returned.
>> + * Otherwise, false is returned.
>> *
>> * @hob: A pointer to a HOB.
>> *
>> - * @retval TRUE: The HOB specified by hob is the last HOB in the HOB list.
>> - * @retval FALSE: The HOB specified by hob is not the last HOB in the HOB list.
>> + * @retval true: The HOB specified by hob is the last HOB in the HOB list.
>> + * @retval false: The HOB specified by hob is not the last HOB in the HOB list.
>> */
>> #define END_OF_HOB(hob) (GET_HOB_TYPE(hob) == (u16)HOB_TYPE_EOH)
>
> hob->hdr.type == HOB_TYPE_EOH
>
>>
>> @@ -255,7 +255,7 @@ union hob_pointers_t {
>> * @return: A pointer to the data buffer in a HOB.
>> */
>> #define GET_GUID_HOB_DATA(hob) \
>> - (void *)(*(u8 **)&(hob) + sizeof(struct hob_guid_t))
>> + (void *)(*(u8 **)&(hob) + sizeof(struct hob_guid))
>
> Similarly can we use the guid member?
>
>>
>> /**
>> * Returns the size of the data buffer from a HOB of type HOB_TYPE_GUID_EXT.
>> @@ -268,7 +268,7 @@ union hob_pointers_t {
>> * @return: The size of the data buffer.
>> */
>> #define GET_GUID_HOB_DATA_SIZE(hob) \
>> - (u16)(GET_HOB_LENGTH(hob) - sizeof(struct hob_guid_t))
>> + (u16)(GET_HOB_LENGTH(hob) - sizeof(struct hob_guid))
>>
>
> Would be good to simplify this one also.
>
Yes.
I will send a v2 later.
[snip]
Regards,
Bin
More information about the U-Boot
mailing list