[U-Boot] [PATCH] x86: Simplify the fsp hob access functions
Simon Glass
sjg at chromium.org
Wed Dec 31 00:02:53 CET 2014
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.
> {
> - 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
More information about the U-Boot
mailing list