[U-Boot] [PATCH] x86: Simplify the fsp hob access functions

Bin Meng bmeng.cn at gmail.com
Wed Dec 31 09:22:43 CET 2014


Hi Simon,

On Wed, Dec 31, 2014 at 7:02 AM, Simon Glass <sjg at chromium.org> wrote:
> Hi Bin,
>
> On 30 December 2014 at 01:02, Bin Meng <bmeng.cn at gmail.com> wrote:
>> Remove the troublesome union hob_pointers so that some annoying casts
>> are no longer needed in those hob access routines. This also improves
>> the readability.
>>
>> Signed-off-by: Bin Meng <bmeng.cn at gmail.com>
>> ---
>>
>>  arch/x86/cpu/queensbay/fsp_support.c               | 95 ++++++++++++----------
>>  arch/x86/cpu/queensbay/tnc_dram.c                  | 39 +++++----
>>  arch/x86/include/asm/arch-queensbay/fsp/fsp_hob.h  | 46 ++++-------
>>  .../include/asm/arch-queensbay/fsp/fsp_support.h   |  5 +-
>>  arch/x86/lib/cmd_hob.c                             | 16 ++--
>>  5 files changed, 101 insertions(+), 100 deletions(-)
>>
>
> Yes a big improvement - see a few additional ideas for a follow-on patch below.
>
> Acked-by: Simon Glass <sjg at chromium.org>
>
>> diff --git a/arch/x86/cpu/queensbay/fsp_support.c b/arch/x86/cpu/queensbay/fsp_support.c
>> index ef1916b..4764e3c 100644
>> --- a/arch/x86/cpu/queensbay/fsp_support.c
>> +++ b/arch/x86/cpu/queensbay/fsp_support.c
>> @@ -231,26 +231,28 @@ u32 fsp_notify(struct fsp_header *fsp_hdr, u32 phase)
>>
>>  u32 fsp_get_usable_lowmem_top(const void *hob_list)
>>  {
>> -       union hob_pointers hob;
>> +       const struct hob_header *hdr;
>> +       struct hob_res_desc *res_desc;
>>         phys_addr_t phys_start;
>>         u32 top;
>>
>>         /* Get the HOB list for processing */
>> -       hob.raw = (void *)hob_list;
>> +       hdr = hob_list;
>>
>>         /* * Collect memory ranges */
>>         top = FSP_LOWMEM_BASE;
>> -       while (!end_of_hob(hob)) {
>> -               if (get_hob_type(hob) == HOB_TYPE_RES_DESC) {
>> -                       if (hob.res_desc->type == RES_SYS_MEM) {
>> -                               phys_start = hob.res_desc->phys_start;
>> +       while (!end_of_hob(hdr)) {
>> +               if (get_hob_type(hdr) == HOB_TYPE_RES_DESC) {
>> +                       res_desc = (struct hob_res_desc *)hdr;
>> +                       if (res_desc->type == RES_SYS_MEM) {
>> +                               phys_start = res_desc->phys_start;
>>                                 /* Need memory above 1MB to be collected here */
>>                                 if (phys_start >= FSP_LOWMEM_BASE &&
>>                                     phys_start < (phys_addr_t)FSP_HIGHMEM_BASE)
>> -                                       top += (u32)(hob.res_desc->len);
>> +                                       top += (u32)(res_desc->len);
>>                         }
>>                 }
>> -               hob.raw = get_next_hob(hob);
>> +               hdr = get_next_hob(hdr);
>>         }
>>
>>         return top;
>> @@ -258,25 +260,27 @@ u32 fsp_get_usable_lowmem_top(const void *hob_list)
>>
>>  u64 fsp_get_usable_highmem_top(const void *hob_list)
>>  {
>> -       union hob_pointers hob;
>> +       const struct hob_header *hdr;
>> +       struct hob_res_desc *res_desc;
>>         phys_addr_t phys_start;
>>         u64 top;
>>
>>         /* Get the HOB list for processing */
>> -       hob.raw = (void *)hob_list;
>> +       hdr = hob_list;
>>
>>         /* Collect memory ranges */
>>         top = FSP_HIGHMEM_BASE;
>> -       while (!end_of_hob(hob)) {
>> -               if (get_hob_type(hob) == HOB_TYPE_RES_DESC) {
>> -                       if (hob.res_desc->type == RES_SYS_MEM) {
>> -                               phys_start = hob.res_desc->phys_start;
>> +       while (!end_of_hob(hdr)) {
>> +               if (get_hob_type(hdr) == HOB_TYPE_RES_DESC) {
>> +                       res_desc = (struct hob_res_desc *)hdr;
>> +                       if (res_desc->type == RES_SYS_MEM) {
>> +                               phys_start = res_desc->phys_start;
>>                                 /* Need memory above 1MB to be collected here */
>>                                 if (phys_start >= (phys_addr_t)FSP_HIGHMEM_BASE)
>> -                                       top += (u32)(hob.res_desc->len);
>> +                                       top += (u32)(res_desc->len);
>>                         }
>>                 }
>> -               hob.raw = get_next_hob(hob);
>> +               hdr = get_next_hob(hdr);
>>         }
>>
>>         return top;
>> @@ -285,24 +289,26 @@ u64 fsp_get_usable_highmem_top(const void *hob_list)
>>  u64 fsp_get_reserved_mem_from_guid(const void *hob_list, u64 *len,
>>                                    struct efi_guid *guid)
>>  {
>> -       union hob_pointers hob;
>> +       const struct hob_header *hdr;
>> +       struct hob_res_desc *res_desc;
>>
>>         /* Get the HOB list for processing */
>> -       hob.raw = (void *)hob_list;
>> +       hdr = hob_list;
>>
>>         /* Collect memory ranges */
>> -       while (!end_of_hob(hob)) {
>> -               if (get_hob_type(hob) == HOB_TYPE_RES_DESC) {
>> -                       if (hob.res_desc->type == RES_MEM_RESERVED) {
>> -                               if (compare_guid(&hob.res_desc->owner, guid)) {
>> +       while (!end_of_hob(hdr)) {
>> +               if (get_hob_type(hdr) == HOB_TYPE_RES_DESC) {
>> +                       res_desc = (struct hob_res_desc *)hdr;
>> +                       if (res_desc->type == RES_MEM_RESERVED) {
>> +                               if (compare_guid(&res_desc->owner, guid)) {
>>                                         if (len)
>> -                                               *len = (u32)(hob.res_desc->len);
>> +                                               *len = (u32)(res_desc->len);
>>
>> -                                       return (u64)(hob.res_desc->phys_start);
>> +                                       return (u64)(res_desc->phys_start);
>>                                 }
>>                         }
>>                 }
>> -               hob.raw = get_next_hob(hob);
>> +               hdr = get_next_hob(hdr);
>>         }
>>
>>         return 0;
>> @@ -336,44 +342,45 @@ u32 fsp_get_tseg_reserved_mem(const void *hob_list, u32 *len)
>>         return base;
>>  }
>>
>> -void *fsp_get_next_hob(u16 type, const void *hob_list)
>> +const struct hob_header *fsp_get_next_hob(u16 type, const void *hob_list)
>
> I'd suggest just using uint instead of u16 in this sort of situation.
> To me the u16 doesn't add anything to a parameter context, and it
> often bloats the code due to the masking that each function needs to
> implement.

Yes, will do in a follow-on patch.

>>  {
>> -       union hob_pointers hob;
>> +       const struct hob_header *hdr;
>>
>> -       assert(hob_list != NULL);
>> -
>> -       hob.raw = (u8 *)hob_list;
>> +       hdr = hob_list;
>>
>>         /* Parse the HOB list until end of list or matching type is found */
>> -       while (!end_of_hob(hob)) {
>> -               if (get_hob_type(hob) == type)
>> -                       return hob.raw;
>> +       while (!end_of_hob(hdr)) {
>> +               if (get_hob_type(hdr) == type)
>> +                       return hdr;
>>
>> -               hob.raw = get_next_hob(hob);
>> +               hdr = get_next_hob(hdr);
>>         }
>>
>>         return NULL;
>>  }
>>
>> -void *fsp_get_next_guid_hob(const struct efi_guid *guid, const void *hob_list)
>> +const struct hob_header *fsp_get_next_guid_hob(const struct efi_guid *guid,
>> +                                              const void *hob_list)
>>  {
>> -       union hob_pointers hob;
>> -
>> -       hob.raw = (u8 *)hob_list;
>> -       while ((hob.raw = fsp_get_next_hob(HOB_TYPE_GUID_EXT,
>> -                       hob.raw)) != NULL) {
>> -               if (compare_guid(guid, &hob.guid->name))
>> +       const struct hob_header *hdr;
>> +       struct hob_guid *guid_hob;
>> +
>> +       hdr = hob_list;
>> +       while ((hdr = fsp_get_next_hob(HOB_TYPE_GUID_EXT,
>> +                       hdr)) != NULL) {
>> +               guid_hob = (struct hob_guid *)hdr;
>> +               if (compare_guid(guid, &(guid_hob->name)))
>>                         break;
>> -               hob.raw = get_next_hob(hob);
>> +               hdr = get_next_hob(hdr);
>>         }
>>
>> -       return hob.raw;
>> +       return hdr;
>>  }
>>
>>  void *fsp_get_guid_hob_data(const void *hob_list, u32 *len,
>>                             struct efi_guid *guid)
>>  {
>> -       u8 *guid_hob;
>> +       const struct hob_header *guid_hob;
>>
>>         guid_hob = fsp_get_next_guid_hob(guid, hob_list);
>>         if (guid_hob == NULL) {
>> diff --git a/arch/x86/cpu/queensbay/tnc_dram.c b/arch/x86/cpu/queensbay/tnc_dram.c
>> index 8e97c9b..b669dbc 100644
>> --- a/arch/x86/cpu/queensbay/tnc_dram.c
>> +++ b/arch/x86/cpu/queensbay/tnc_dram.c
>> @@ -14,17 +14,19 @@ DECLARE_GLOBAL_DATA_PTR;
>>  int dram_init(void)
>>  {
>>         phys_size_t ram_size = 0;
>> -       union hob_pointers hob;
>> +       const struct hob_header *hdr;
>> +       struct hob_res_desc *res_desc;
>>
>> -       hob.raw = gd->arch.hob_list;
>> -       while (!end_of_hob(hob)) {
>> -               if (get_hob_type(hob) == HOB_TYPE_RES_DESC) {
>> -                       if (hob.res_desc->type == RES_SYS_MEM ||
>> -                           hob.res_desc->type == RES_MEM_RESERVED) {
>> -                               ram_size += hob.res_desc->len;
>> +       hdr = gd->arch.hob_list;
>> +       while (!end_of_hob(hdr)) {
>> +               if (get_hob_type(hdr) == HOB_TYPE_RES_DESC) {
>> +                       res_desc = (struct hob_res_desc *)hdr;
>> +                       if (res_desc->type == RES_SYS_MEM ||
>> +                           res_desc->type == RES_MEM_RESERVED) {
>> +                               ram_size += res_desc->len;
>>                         }
>>                 }
>> -               hob.raw = get_next_hob(hob);
>> +               hdr = get_next_hob(hdr);
>>         }
>>
>>         gd->ram_size = ram_size;
>> @@ -55,22 +57,23 @@ ulong board_get_usable_ram_top(ulong total_size)
>>  unsigned install_e820_map(unsigned max_entries, struct e820entry *entries)
>>  {
>>         unsigned num_entries = 0;
>> +       const struct hob_header *hdr;
>> +       struct hob_res_desc *res_desc;
>>
>> -       union hob_pointers hob;
>> +       hdr = gd->arch.hob_list;
>>
>> -       hob.raw = gd->arch.hob_list;
>> +       while (!end_of_hob(hdr)) {
>> +               if (get_hob_type(hdr) == HOB_TYPE_RES_DESC) {
>> +                       res_desc = (struct hob_res_desc *)hdr;
>> +                       entries[num_entries].addr = res_desc->phys_start;
>> +                       entries[num_entries].size = res_desc->len;
>>
>> -       while (!end_of_hob(hob)) {
>> -               if (get_hob_type(hob) == HOB_TYPE_RES_DESC) {
>> -                       entries[num_entries].addr = hob.res_desc->phys_start;
>> -                       entries[num_entries].size = hob.res_desc->len;
>> -
>> -                       if (hob.res_desc->type == RES_SYS_MEM)
>> +                       if (res_desc->type == RES_SYS_MEM)
>>                                 entries[num_entries].type = E820_RAM;
>> -                       else if (hob.res_desc->type == RES_MEM_RESERVED)
>> +                       else if (res_desc->type == RES_MEM_RESERVED)
>>                                 entries[num_entries].type = E820_RESERVED;
>>                 }
>> -               hob.raw = get_next_hob(hob);
>> +               hdr = get_next_hob(hdr);
>>                 num_entries++;
>>         }
>>
>> 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 380b64e..5110361 100644
>> --- a/arch/x86/include/asm/arch-queensbay/fsp/fsp_hob.h
>> +++ b/arch/x86/include/asm/arch-queensbay/fsp/fsp_hob.h
>> @@ -182,15 +182,6 @@ struct hob_guid {
>>         /* GUID specific data goes here */
>>  };
>>
>> -/* Union of all the possible HOB Types */
>> -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;
>> -};
>> -
>>  /**
>>   * get_hob_type() - return the type of a HOB
>>   *
>> @@ -201,9 +192,9 @@ union hob_pointers {
>>   *
>>   * @return: HOB type.
>>   */
>> -static inline u16 get_hob_type(union hob_pointers hob)
>> +static inline u16 get_hob_type(const struct hob_header *hdr)
>>  {
>> -       return hob.hdr->type;
>> +       return hdr->type;
>>  }
>
> Drop this function?

Agreed.

>>
>>  /**
>> @@ -216,9 +207,9 @@ static inline u16 get_hob_type(union hob_pointers hob)
>>   *
>>   * @return: HOB length.
>>   */
>> -static inline u16 get_hob_length(union hob_pointers hob)
>> +static inline u16 get_hob_length(const struct hob_header *hdr)
>>  {
>> -       return hob.hdr->len;
>> +       return hdr->len;
>>  }
>
> Drop this function?

Agreed.

[snip]

Regards,
Bin


More information about the U-Boot mailing list