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. > { > - 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? > > /** > @@ -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? > > /** > @@ -227,13 +218,13 @@ static inline u16 get_hob_length(union hob_pointers hob) > * This macro returns a pointer to HOB that follows the HOB specified by hob > * in the HOB List. > * > - * @hob: A pointer to a HOB. > + * @hdr: A pointer to a HOB. > * > * @return: A pointer to the next HOB in the HOB list. > */ > -static inline void *get_next_hob(union hob_pointers hob) > +static inline const struct hob_header *get_next_hob(const struct hob_header > *hdr) > { > - return (void *)(*(u8 **)&(hob) + get_hob_length(hob)); > + return (const struct hob_header *)((u32)hdr + get_hob_length(hdr)); > } > > /** > @@ -243,14 +234,14 @@ static inline void *get_next_hob(union hob_pointers hob) > * 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. > + * @hdr: 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 hdr is the last HOB in the HOB list. > + * @retval false: The HOB specified by hdr is not the last HOB in the HOB > list. > */ > -static inline bool end_of_hob(union hob_pointers hob) > +static inline bool end_of_hob(const struct hob_header *hdr) > { > - return get_hob_type(hob) == HOB_TYPE_EOH; > + return get_hob_type(hdr) == HOB_TYPE_EOH; > } I suppose this function is useful. > > /** > @@ -260,13 +251,13 @@ static inline bool end_of_hob(union hob_pointers hob) > * This macro returns a pointer to the data buffer in a HOB specified by hob. > * hob is assumed to be a HOB of type HOB_TYPE_GUID_EXT. > * > - * @hob: A pointer to a HOB. > + * @hdr: A pointer to a HOB. > * > * @return: A pointer to the data buffer in a HOB. > */ > -static inline void *get_guid_hob_data(u8 *hob) > +static inline void *get_guid_hob_data(const struct hob_header *hdr) > { > - return (void *)(hob + sizeof(struct hob_guid)); > + return (void *)((u32)hdr + sizeof(struct hob_guid)); > } > > /** > @@ -276,14 +267,13 @@ static inline void *get_guid_hob_data(u8 *hob) > * This macro returns the size, in bytes, of the data buffer in a HOB > * specified by hob. hob is assumed to be a HOB of type HOB_TYPE_GUID_EXT. > * > - * @hob: A pointer to a HOB. > + * @hdr: A pointer to a HOB. > * > * @return: The size of the data buffer. > */ > -static inline u16 get_guid_hob_data_size(u8 *hob) > +static inline u16 get_guid_hob_data_size(const struct hob_header *hdr) > { > - union hob_pointers hob_p = *(union hob_pointers *)hob; > - return get_hob_length(hob_p) - sizeof(struct hob_guid); > + return get_hob_length(hdr) - sizeof(struct hob_guid); > } > > /* FSP specific GUID HOB definitions */ > diff --git a/arch/x86/include/asm/arch-queensbay/fsp/fsp_support.h > b/arch/x86/include/asm/arch-queensbay/fsp/fsp_support.h > index 3ae1b66..2a3e987 100644 > --- a/arch/x86/include/asm/arch-queensbay/fsp/fsp_support.h > +++ b/arch/x86/include/asm/arch-queensbay/fsp/fsp_support.h > @@ -145,7 +145,7 @@ u32 fsp_get_tseg_reserved_mem(const void *hob_list, u32 > *len); > * > * @retval: A HOB object with matching type; Otherwise NULL. > */ > -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); > > /** > * Returns the next instance of the matched GUID HOB from the starting HOB. > @@ -155,7 +155,8 @@ void *fsp_get_next_hob(u16 type, const void *hob_list); > * > * @retval: A HOB object with matching GUID; Otherwise 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); > > /** > * This function retrieves a GUID HOB data buffer and size. > diff --git a/arch/x86/lib/cmd_hob.c b/arch/x86/lib/cmd_hob.c > index b552fe6..8d1f038 100644 > --- a/arch/x86/lib/cmd_hob.c > +++ b/arch/x86/lib/cmd_hob.c > @@ -28,20 +28,20 @@ static char *hob_type[] = { > > int do_hob(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > { > - union hob_pointers hob; > + const struct hob_header *hdr; > u16 type; > char *desc; > int i = 0; > > - hob.raw = (u8 *)gd->arch.hob_list; > + hdr = gd->arch.hob_list; > > - printf("HOB list address: 0x%08x\n\n", (unsigned int)hob.raw); > + printf("HOB list address: 0x%08x\n\n", (unsigned int)hdr); > > printf("No. | Address | Type | Length in Bytes\n"); > printf("----|----------|---------------------|----------------\n"); > - while (!end_of_hob(hob)) { > - printf("%-3d | %08x | ", i, (unsigned int)hob.raw); > - type = get_hob_type(hob); > + while (!end_of_hob(hdr)) { > + printf("%-3d | %08x | ", i, (unsigned int)hdr); > + type = get_hob_type(hdr); > if (type == HOB_TYPE_UNUSED) > desc = "*Unused*"; > else if (type == HOB_TYPE_EOH) > @@ -50,8 +50,8 @@ int do_hob(cmd_tbl_t *cmdtp, int flag, int argc, char * > const argv[]) > desc = hob_type[type]; > else > desc = "*Invalid Type*"; > - printf("%-19s | %-15d\n", desc, get_hob_length(hob)); > - hob.raw = get_next_hob(hob); > + printf("%-19s | %-15d\n", desc, get_hob_length(hdr)); > + hdr = get_next_hob(hdr); > i++; > } > > -- > 1.8.2.1 > Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot