Hi Simon,
On Wed, Dec 31, 2014 at 7:02 AM, Simon Glass <s...@chromium.org> wrote:
> Hi Bin,
>
> On 30 December 2014 at 01:02, Bin Meng <bmeng...@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...@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 <s...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
