[U-Boot] [PATCH 0/8] x86: fsp: Move platform-specific config to chipset directory
All FSP spec v1.0 complaint FSP binary uses struct fspinit_rtbuf as defined by the 1.0 spec, however there are FSPs that do not follow 1.0 spec, like Intel FSP for 3rd generation Intel Core and Intel Celeron processors with mobile Intel HM76 and QM77 chipsets platform (formerly Chief River Platform: Ivy Bridge and Panther Point). Although Intel website says this FSP conforms to FSP v1.0 specification which defines the UPD usage, it is not really the case. This might possible due to that FSP predates the 1.0 spec. Also future FSP binary that is complaint to v1.1 spec defines an optional paltform-specific runtime data in the struct fspinit_rtbuf.
Besides this fspinit_rtbuf, the IvyBridge FSP does not support UPD either, so that current codes in arch/x86/lib/fsp/fsp_support.c won't work for that FSP. We need some flexibility, hence move those platform-specific config to chipset directory.
Right now we still use hardcoded 'const struct' table (in flash) to pass the Azalia (Intel HD Audio) verb table to FSP, because different FSP may use different verb table format (at least for Queensbay and BayTrail), we cannot handle this in the common FSP support codes. But with this series, we are now able to get Azalia verb table from device tree, just like what we did for overriding UPD data configuration. This can be done in future patch set.
Bin Meng (8): x86: fsp: Simplify fsp_continue() x86: fsp: Avoid cast stack_top in struct shared_data x86: fsp: Add boot_mode as a member of struct shared_data x86: fsp: Rename shared_data to fsp_config_data x86: fsp: Rename update_fsp_upd() and change its signature x86: fsp: Move struct fspinit_rtbuf definition to chipset header x86: fsp: Move VPD/UPD verification to update_fsp_configs() x86: queensbay: Remove invalid comments in update_fsp_upd()
arch/x86/cpu/baytrail/fsp_configs.c | 27 ++++++++- arch/x86/cpu/queensbay/fsp_configs.c | 30 ++++++++-- .../include/asm/arch-baytrail/fsp/fsp_configs.h | 21 +++++++ .../include/asm/arch-queensbay/fsp/fsp_configs.h | 21 +++++++ arch/x86/include/asm/fsp/fsp_api.h | 2 +- arch/x86/include/asm/fsp/fsp_platform.h | 15 ----- arch/x86/include/asm/fsp/fsp_support.h | 20 +++---- arch/x86/lib/fsp/fsp_support.c | 64 +++++----------------- 8 files changed, 112 insertions(+), 88 deletions(-) create mode 100644 arch/x86/include/asm/arch-baytrail/fsp/fsp_configs.h create mode 100644 arch/x86/include/asm/arch-queensbay/fsp/fsp_configs.h delete mode 100644 arch/x86/include/asm/fsp/fsp_platform.h
There is no need to pass shared_data to fsp_continue() so we can remove unnecessary codes that simplifies the function a lot.
Signed-off-by: Bin Meng bmeng.cn@gmail.com ---
arch/x86/include/asm/fsp/fsp_support.h | 4 +--- arch/x86/lib/fsp/fsp_support.c | 29 ++++++----------------------- 2 files changed, 7 insertions(+), 26 deletions(-)
diff --git a/arch/x86/include/asm/fsp/fsp_support.h b/arch/x86/include/asm/fsp/fsp_support.h index 7317dda..f30d7b4 100644 --- a/arch/x86/include/asm/fsp/fsp_support.h +++ b/arch/x86/include/asm/fsp/fsp_support.h @@ -49,14 +49,12 @@ void fsp_init_done(void *hob_list); /** * FSP Continuation function * - * @shared_data: Shared data base before stack migration * @status: Always 0 * @hob_list: HOB list pointer * * @retval: Never returns */ -void fsp_continue(struct shared_data *shared_data, u32 status, - void *hob_list); +void fsp_continue(u32 status, void *hob_list);
/** * Find FSP header offset in FSP image diff --git a/arch/x86/lib/fsp/fsp_support.c b/arch/x86/lib/fsp/fsp_support.c index 1d48ff4..0408b5d 100644 --- a/arch/x86/lib/fsp/fsp_support.c +++ b/arch/x86/lib/fsp/fsp_support.c @@ -87,28 +87,12 @@ struct fsp_header *__attribute__((optimize("O0"))) find_fsp_header(void) return (struct fsp_header *)fsp; }
-void fsp_continue(struct shared_data *shared_data, u32 status, void *hob_list) +void fsp_continue(u32 status, void *hob_list) { - u32 stack_len; - u32 stack_base; - u32 stack_top; - post_code(POST_MRC);
assert(status == 0);
- /* Get the migrated stack in normal memory */ - 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 *)(stack_base + - ((u32)shared_data - *(u32 *)stack_top)); - /* The boot loader main function entry */ fsp_init_done(hob_list); } @@ -176,19 +160,18 @@ void fsp_init(u32 stack_top, u32 boot_mode, void *nvs_buf) setup_fsp_gdt();
/* - * Use ASM code to ensure the register value in EAX & ECX - * will be passed into BlContinuationFunc + * Use ASM code to ensure the register value in EAX & EDX + * will be passed into fsp_continue */ asm volatile ( "pushl %0;" "call *%%eax;" ".global asm_continuation;" "asm_continuation:;" - "movl %%ebx, %%eax;" /* shared_data */ - "movl 4(%%esp), %%edx;" /* status */ - "movl 8(%%esp), %%ecx;" /* hob_list */ + "movl 4(%%esp), %%eax;" /* status */ + "movl 8(%%esp), %%edx;" /* hob_list */ "jmp fsp_continue;" - : : "m"(params_ptr), "a"(init), "b"(&shared_data) + : : "m"(params_ptr), "a"(init) );
/*
Hi Bin,
On 2 December 2015 at 01:58, Bin Meng bmeng.cn@gmail.com wrote:
There is no need to pass shared_data to fsp_continue() so we can remove unnecessary codes that simplifies the function a lot.
Is this because shared_data is effectively useful for a bootloader due to the stack being changed?
Signed-off-by: Bin Meng bmeng.cn@gmail.com
arch/x86/include/asm/fsp/fsp_support.h | 4 +--- arch/x86/lib/fsp/fsp_support.c | 29 ++++++----------------------- 2 files changed, 7 insertions(+), 26 deletions(-)
[snip]
Regards, Simon
Hi Simon,
On Thu, Dec 3, 2015 at 5:05 AM, Simon Glass sjg@chromium.org wrote:
Hi Bin,
On 2 December 2015 at 01:58, Bin Meng bmeng.cn@gmail.com wrote:
There is no need to pass shared_data to fsp_continue() so we can remove unnecessary codes that simplifies the function a lot.
Is this because shared_data is effectively useful for a bootloader due to the stack being changed?
The removed codes were originally from Queensbay FSP package. Looks Intel's intention was to help stack migration, but I doubt they ever worked. We don't support stack migration after fsp_init() hence these codes are never needed.
Signed-off-by: Bin Meng bmeng.cn@gmail.com
arch/x86/include/asm/fsp/fsp_support.h | 4 +--- arch/x86/lib/fsp/fsp_support.c | 29 ++++++----------------------- 2 files changed, 7 insertions(+), 26 deletions(-)
[snip]
Regards, Simon
Regards, Bin
Hi Bin,
On 2 December 2015 at 22:11, Bin Meng bmeng.cn@gmail.com wrote:
Hi Simon,
On Thu, Dec 3, 2015 at 5:05 AM, Simon Glass sjg@chromium.org wrote:
Hi Bin,
On 2 December 2015 at 01:58, Bin Meng bmeng.cn@gmail.com wrote:
There is no need to pass shared_data to fsp_continue() so we can remove unnecessary codes that simplifies the function a lot.
Is this because shared_data is effectively useful for a bootloader due to the stack being changed?
The removed codes were originally from Queensbay FSP package. Looks Intel's intention was to help stack migration, but I doubt they ever worked. We don't support stack migration after fsp_init() hence these codes are never needed.
Eek. Nasty unimplemented specs!
Acked-by: Simon Glass sjg@chromium.org
Signed-off-by: Bin Meng bmeng.cn@gmail.com
arch/x86/include/asm/fsp/fsp_support.h | 4 +--- arch/x86/lib/fsp/fsp_support.c | 29 ++++++----------------------- 2 files changed, 7 insertions(+), 26 deletions(-)
[snip]
Regards, Simon
Regards, Bin
Declare stack_top as u32 in struct shared_data and struct common_buf so that we can avoid casting in fsp_init().
Signed-off-by: Bin Meng bmeng.cn@gmail.com ---
arch/x86/include/asm/fsp/fsp_api.h | 2 +- arch/x86/include/asm/fsp/fsp_support.h | 2 +- arch/x86/lib/fsp/fsp_support.c | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/x86/include/asm/fsp/fsp_api.h b/arch/x86/include/asm/fsp/fsp_api.h index 2d34d13..db83e35 100644 --- a/arch/x86/include/asm/fsp/fsp_api.h +++ b/arch/x86/include/asm/fsp/fsp_api.h @@ -30,7 +30,7 @@ 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. */ - u32 *stack_top; + u32 stack_top; u32 boot_mode; /* Current system boot mode */ void *upd_data; /* User platform configuraiton data region */ u32 reserved[7]; /* Reserved */ diff --git a/arch/x86/include/asm/fsp/fsp_support.h b/arch/x86/include/asm/fsp/fsp_support.h index f30d7b4..685778e 100644 --- a/arch/x86/include/asm/fsp/fsp_support.h +++ b/arch/x86/include/asm/fsp/fsp_support.h @@ -20,7 +20,7 @@
struct shared_data { struct fsp_header *fsp_hdr; - u32 *stack_top; + u32 stack_top; struct upd_region fsp_upd; };
diff --git a/arch/x86/lib/fsp/fsp_support.c b/arch/x86/lib/fsp/fsp_support.c index 0408b5d..df62ba8 100644 --- a/arch/x86/lib/fsp/fsp_support.c +++ b/arch/x86/lib/fsp/fsp_support.c @@ -122,7 +122,7 @@ void fsp_init(u32 stack_top, u32 boot_mode, void *nvs_buf) 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.stack_top = stack_top - 32; rt_buf.common.boot_mode = boot_mode; rt_buf.common.upd_data = fsp_upd;
@@ -152,7 +152,7 @@ void fsp_init(u32 stack_top, u32 boot_mode, void *nvs_buf) params_ptr = ¶ms;
shared_data.fsp_hdr = fsp_hdr; - shared_data.stack_top = (u32 *)stack_top; + shared_data.stack_top = stack_top;
post_code(POST_PRE_MRC);
On 2 December 2015 at 01:58, Bin Meng bmeng.cn@gmail.com wrote:
Declare stack_top as u32 in struct shared_data and struct common_buf so that we can avoid casting in fsp_init().
Signed-off-by: Bin Meng bmeng.cn@gmail.com
arch/x86/include/asm/fsp/fsp_api.h | 2 +- arch/x86/include/asm/fsp/fsp_support.h | 2 +- arch/x86/lib/fsp/fsp_support.c | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-)
Acked-by: Simon Glass sjg@chromium.org
Save boot_mode in struct shared_data for future refactoring
Signed-off-by: Bin Meng bmeng.cn@gmail.com ---
arch/x86/include/asm/fsp/fsp_support.h | 1 + arch/x86/lib/fsp/fsp_support.c | 1 + 2 files changed, 2 insertions(+)
diff --git a/arch/x86/include/asm/fsp/fsp_support.h b/arch/x86/include/asm/fsp/fsp_support.h index 685778e..18e2d21 100644 --- a/arch/x86/include/asm/fsp/fsp_support.h +++ b/arch/x86/include/asm/fsp/fsp_support.h @@ -21,6 +21,7 @@ struct shared_data { struct fsp_header *fsp_hdr; u32 stack_top; + u32 boot_mode; struct upd_region fsp_upd; };
diff --git a/arch/x86/lib/fsp/fsp_support.c b/arch/x86/lib/fsp/fsp_support.c index df62ba8..083d855 100644 --- a/arch/x86/lib/fsp/fsp_support.c +++ b/arch/x86/lib/fsp/fsp_support.c @@ -153,6 +153,7 @@ void fsp_init(u32 stack_top, u32 boot_mode, void *nvs_buf)
shared_data.fsp_hdr = fsp_hdr; shared_data.stack_top = stack_top; + shared_data.boot_mode = boot_mode;
post_code(POST_PRE_MRC);
On 2 December 2015 at 01:58, Bin Meng bmeng.cn@gmail.com wrote:
Save boot_mode in struct shared_data for future refactoring
Signed-off-by: Bin Meng bmeng.cn@gmail.com
arch/x86/include/asm/fsp/fsp_support.h | 1 + arch/x86/lib/fsp/fsp_support.c | 1 + 2 files changed, 2 insertions(+)
Acked-by: Simon Glass sjg@chromium.org
FSP has several config data like UPD, HDA verb table which can be overridden or provided by bootloader. Currently in U-Boot only UPD is handled via struct shared_data. To accommodate any platform, we rename shared_data to fsp_config_data and move the definition from common place fsp_support.h to platform-specific place fsp_configs.h.
Signed-off-by: Bin Meng bmeng.cn@gmail.com ---
arch/x86/include/asm/arch-baytrail/fsp/fsp_configs.h | 17 +++++++++++++++++ arch/x86/include/asm/arch-queensbay/fsp/fsp_configs.h | 17 +++++++++++++++++ arch/x86/include/asm/fsp/fsp_support.h | 8 +------- arch/x86/lib/fsp/fsp_support.c | 10 +++++----- 4 files changed, 40 insertions(+), 12 deletions(-) create mode 100644 arch/x86/include/asm/arch-baytrail/fsp/fsp_configs.h create mode 100644 arch/x86/include/asm/arch-queensbay/fsp/fsp_configs.h
diff --git a/arch/x86/include/asm/arch-baytrail/fsp/fsp_configs.h b/arch/x86/include/asm/arch-baytrail/fsp/fsp_configs.h new file mode 100644 index 0000000..2397553 --- /dev/null +++ b/arch/x86/include/asm/arch-baytrail/fsp/fsp_configs.h @@ -0,0 +1,17 @@ +/* + * Copyright (C) 2015, Bin Meng bmeng.cn@gmail.com + * + * SPDX-License-Identifier: Intel + */ + +#ifndef __FSP_CONFIGS_H__ +#define __FSP_CONFIGS_H__ + +struct fsp_config_data { + struct fsp_header *fsp_hdr; + u32 stack_top; + u32 boot_mode; + struct upd_region fsp_upd; +}; + +#endif /* __FSP_CONFIGS_H__ */ diff --git a/arch/x86/include/asm/arch-queensbay/fsp/fsp_configs.h b/arch/x86/include/asm/arch-queensbay/fsp/fsp_configs.h new file mode 100644 index 0000000..2397553 --- /dev/null +++ b/arch/x86/include/asm/arch-queensbay/fsp/fsp_configs.h @@ -0,0 +1,17 @@ +/* + * Copyright (C) 2015, Bin Meng bmeng.cn@gmail.com + * + * SPDX-License-Identifier: Intel + */ + +#ifndef __FSP_CONFIGS_H__ +#define __FSP_CONFIGS_H__ + +struct fsp_config_data { + struct fsp_header *fsp_hdr; + u32 stack_top; + u32 boot_mode; + struct upd_region fsp_upd; +}; + +#endif /* __FSP_CONFIGS_H__ */ diff --git a/arch/x86/include/asm/fsp/fsp_support.h b/arch/x86/include/asm/fsp/fsp_support.h index 18e2d21..39b2864 100644 --- a/arch/x86/include/asm/fsp/fsp_support.h +++ b/arch/x86/include/asm/fsp/fsp_support.h @@ -17,13 +17,7 @@ #include "fsp_infoheader.h" #include "fsp_bootmode.h" #include <asm/arch/fsp/fsp_vpd.h> - -struct shared_data { - struct fsp_header *fsp_hdr; - u32 stack_top; - u32 boot_mode; - struct upd_region fsp_upd; -}; +#include <asm/arch/fsp/fsp_configs.h>
#define FSP_LOWMEM_BASE 0x100000UL #define FSP_HIGHMEM_BASE 0x100000000ULL diff --git a/arch/x86/lib/fsp/fsp_support.c b/arch/x86/lib/fsp/fsp_support.c index 083d855..9da720b 100644 --- a/arch/x86/lib/fsp/fsp_support.c +++ b/arch/x86/lib/fsp/fsp_support.c @@ -99,7 +99,7 @@ void fsp_continue(u32 status, void *hob_list)
void fsp_init(u32 stack_top, u32 boot_mode, void *nvs_buf) { - struct shared_data shared_data; + struct fsp_config_data config_data; fsp_init_f init; struct fsp_init_params params; struct fspinit_rtbuf rt_buf; @@ -118,7 +118,7 @@ void fsp_init(u32 stack_top, u32 boot_mode, void *nvs_buf) panic("Invalid FSP header"); }
- fsp_upd = &shared_data.fsp_upd; + fsp_upd = &config_data.fsp_upd; memset(&rt_buf, 0, sizeof(struct fspinit_rtbuf));
/* Reserve a gap in stack top */ @@ -151,9 +151,9 @@ void fsp_init(u32 stack_top, u32 boot_mode, void *nvs_buf) init = (fsp_init_f)(fsp_hdr->img_base + fsp_hdr->fsp_init); params_ptr = ¶ms;
- shared_data.fsp_hdr = fsp_hdr; - shared_data.stack_top = stack_top; - shared_data.boot_mode = boot_mode; + config_data.fsp_hdr = fsp_hdr; + config_data.stack_top = stack_top; + config_data.boot_mode = boot_mode;
post_code(POST_PRE_MRC);
Hi Bin,
On 2 December 2015 at 01:59, Bin Meng bmeng.cn@gmail.com wrote:
FSP has several config data like UPD, HDA verb table which can be overridden or provided by bootloader. Currently in U-Boot only UPD is handled via struct shared_data. To accommodate any platform, we rename shared_data to fsp_config_data and move the definition from common place fsp_support.h to platform-specific place fsp_configs.h.
Signed-off-by: Bin Meng bmeng.cn@gmail.com
arch/x86/include/asm/arch-baytrail/fsp/fsp_configs.h | 17 +++++++++++++++++ arch/x86/include/asm/arch-queensbay/fsp/fsp_configs.h | 17 +++++++++++++++++ arch/x86/include/asm/fsp/fsp_support.h | 8 +------- arch/x86/lib/fsp/fsp_support.c | 10 +++++----- 4 files changed, 40 insertions(+), 12 deletions(-) create mode 100644 arch/x86/include/asm/arch-baytrail/fsp/fsp_configs.h create mode 100644 arch/x86/include/asm/arch-queensbay/fsp/fsp_configs.h
diff --git a/arch/x86/include/asm/arch-baytrail/fsp/fsp_configs.h b/arch/x86/include/asm/arch-baytrail/fsp/fsp_configs.h new file mode 100644 index 0000000..2397553 --- /dev/null +++ b/arch/x86/include/asm/arch-baytrail/fsp/fsp_configs.h @@ -0,0 +1,17 @@ +/*
- Copyright (C) 2015, Bin Meng bmeng.cn@gmail.com
- SPDX-License-Identifier: Intel
- */
+#ifndef __FSP_CONFIGS_H__ +#define __FSP_CONFIGS_H__
Does this justify its own header file? I suppose it does...we have too many fsp header files and I never know which file contains a particular struct definition.
+struct fsp_config_data {
struct fsp_header *fsp_hdr;u32 stack_top;u32 boot_mode;struct upd_region fsp_upd;
These are common - should we have a common struct as the first member?
I'm just struggling to understand the benefit of duplicating this common thing into separate files.
+};
+#endif /* __FSP_CONFIGS_H__ */ diff --git a/arch/x86/include/asm/arch-queensbay/fsp/fsp_configs.h b/arch/x86/include/asm/arch-queensbay/fsp/fsp_configs.h new file mode 100644 index 0000000..2397553 --- /dev/null +++ b/arch/x86/include/asm/arch-queensbay/fsp/fsp_configs.h @@ -0,0 +1,17 @@ +/*
- Copyright (C) 2015, Bin Meng bmeng.cn@gmail.com
- SPDX-License-Identifier: Intel
- */
+#ifndef __FSP_CONFIGS_H__ +#define __FSP_CONFIGS_H__
+struct fsp_config_data {
struct fsp_header *fsp_hdr;u32 stack_top;u32 boot_mode;struct upd_region fsp_upd;+};
+#endif /* __FSP_CONFIGS_H__ */ diff --git a/arch/x86/include/asm/fsp/fsp_support.h b/arch/x86/include/asm/fsp/fsp_support.h index 18e2d21..39b2864 100644 --- a/arch/x86/include/asm/fsp/fsp_support.h +++ b/arch/x86/include/asm/fsp/fsp_support.h @@ -17,13 +17,7 @@ #include "fsp_infoheader.h" #include "fsp_bootmode.h" #include <asm/arch/fsp/fsp_vpd.h>
-struct shared_data {
struct fsp_header *fsp_hdr;u32 stack_top;u32 boot_mode;struct upd_region fsp_upd;-}; +#include <asm/arch/fsp/fsp_configs.h>
#define FSP_LOWMEM_BASE 0x100000UL #define FSP_HIGHMEM_BASE 0x100000000ULL diff --git a/arch/x86/lib/fsp/fsp_support.c b/arch/x86/lib/fsp/fsp_support.c index 083d855..9da720b 100644 --- a/arch/x86/lib/fsp/fsp_support.c +++ b/arch/x86/lib/fsp/fsp_support.c @@ -99,7 +99,7 @@ void fsp_continue(u32 status, void *hob_list)
void fsp_init(u32 stack_top, u32 boot_mode, void *nvs_buf) {
struct shared_data shared_data;
struct fsp_config_data config_data; fsp_init_f init; struct fsp_init_params params; struct fspinit_rtbuf rt_buf;@@ -118,7 +118,7 @@ void fsp_init(u32 stack_top, u32 boot_mode, void *nvs_buf) panic("Invalid FSP header"); }
fsp_upd = &shared_data.fsp_upd;
fsp_upd = &config_data.fsp_upd; memset(&rt_buf, 0, sizeof(struct fspinit_rtbuf)); /* Reserve a gap in stack top */@@ -151,9 +151,9 @@ void fsp_init(u32 stack_top, u32 boot_mode, void *nvs_buf) init = (fsp_init_f)(fsp_hdr->img_base + fsp_hdr->fsp_init); params_ptr = ¶ms;
shared_data.fsp_hdr = fsp_hdr;shared_data.stack_top = stack_top;shared_data.boot_mode = boot_mode;
config_data.fsp_hdr = fsp_hdr;config_data.stack_top = stack_top;config_data.boot_mode = boot_mode; post_code(POST_PRE_MRC);-- 1.8.2.1
Regards, Simon
Hi Simon,
On Thu, Dec 3, 2015 at 5:05 AM, Simon Glass sjg@chromium.org wrote:
Hi Bin,
On 2 December 2015 at 01:59, Bin Meng bmeng.cn@gmail.com wrote:
FSP has several config data like UPD, HDA verb table which can be overridden or provided by bootloader. Currently in U-Boot only UPD is handled via struct shared_data. To accommodate any platform, we rename shared_data to fsp_config_data and move the definition from common place fsp_support.h to platform-specific place fsp_configs.h.
Signed-off-by: Bin Meng bmeng.cn@gmail.com
arch/x86/include/asm/arch-baytrail/fsp/fsp_configs.h | 17 +++++++++++++++++ arch/x86/include/asm/arch-queensbay/fsp/fsp_configs.h | 17 +++++++++++++++++ arch/x86/include/asm/fsp/fsp_support.h | 8 +------- arch/x86/lib/fsp/fsp_support.c | 10 +++++----- 4 files changed, 40 insertions(+), 12 deletions(-) create mode 100644 arch/x86/include/asm/arch-baytrail/fsp/fsp_configs.h create mode 100644 arch/x86/include/asm/arch-queensbay/fsp/fsp_configs.h
diff --git a/arch/x86/include/asm/arch-baytrail/fsp/fsp_configs.h b/arch/x86/include/asm/arch-baytrail/fsp/fsp_configs.h new file mode 100644 index 0000000..2397553 --- /dev/null +++ b/arch/x86/include/asm/arch-baytrail/fsp/fsp_configs.h @@ -0,0 +1,17 @@ +/*
- Copyright (C) 2015, Bin Meng bmeng.cn@gmail.com
- SPDX-License-Identifier: Intel
- */
+#ifndef __FSP_CONFIGS_H__ +#define __FSP_CONFIGS_H__
Does this justify its own header file? I suppose it does...we have too many fsp header files and I never know which file contains a particular struct definition.
+struct fsp_config_data {
struct fsp_header *fsp_hdr;u32 stack_top;u32 boot_mode;struct upd_region fsp_upd;These are common - should we have a common struct as the first member?
We certainly can create a common struct for the first 3 members (fsp_hdr, stack_top, boot_mode). Another way is to change fsp_update_configs() (in patch#7 of this series) signature to:
void update_fsp_configs(struct fsp_config_data *config, struct fspinit_rtbuf *rt_buf, struct fsp_header *fsp_hdr, u32 stack_top, u32 boot_mode);
This way we avoid saving these 3 parameters into struct fsp_config_data.
I'm just struggling to understand the benefit of duplicating this common thing into separate files.
[snip]
Regards, Bin
Hi Bin,
On 2 December 2015 at 22:18, Bin Meng bmeng.cn@gmail.com wrote:
Hi Simon,
On Thu, Dec 3, 2015 at 5:05 AM, Simon Glass sjg@chromium.org wrote:
Hi Bin,
On 2 December 2015 at 01:59, Bin Meng bmeng.cn@gmail.com wrote:
FSP has several config data like UPD, HDA verb table which can be overridden or provided by bootloader. Currently in U-Boot only UPD is handled via struct shared_data. To accommodate any platform, we rename shared_data to fsp_config_data and move the definition from common place fsp_support.h to platform-specific place fsp_configs.h.
Signed-off-by: Bin Meng bmeng.cn@gmail.com
arch/x86/include/asm/arch-baytrail/fsp/fsp_configs.h | 17 +++++++++++++++++ arch/x86/include/asm/arch-queensbay/fsp/fsp_configs.h | 17 +++++++++++++++++ arch/x86/include/asm/fsp/fsp_support.h | 8 +------- arch/x86/lib/fsp/fsp_support.c | 10 +++++----- 4 files changed, 40 insertions(+), 12 deletions(-) create mode 100644 arch/x86/include/asm/arch-baytrail/fsp/fsp_configs.h create mode 100644 arch/x86/include/asm/arch-queensbay/fsp/fsp_configs.h
diff --git a/arch/x86/include/asm/arch-baytrail/fsp/fsp_configs.h b/arch/x86/include/asm/arch-baytrail/fsp/fsp_configs.h new file mode 100644 index 0000000..2397553 --- /dev/null +++ b/arch/x86/include/asm/arch-baytrail/fsp/fsp_configs.h @@ -0,0 +1,17 @@ +/*
- Copyright (C) 2015, Bin Meng bmeng.cn@gmail.com
- SPDX-License-Identifier: Intel
- */
+#ifndef __FSP_CONFIGS_H__ +#define __FSP_CONFIGS_H__
Does this justify its own header file? I suppose it does...we have too many fsp header files and I never know which file contains a particular struct definition.
+struct fsp_config_data {
struct fsp_header *fsp_hdr;u32 stack_top;u32 boot_mode;struct upd_region fsp_upd;These are common - should we have a common struct as the first member?
We certainly can create a common struct for the first 3 members (fsp_hdr, stack_top, boot_mode). Another way is to change fsp_update_configs() (in patch#7 of this series) signature to:
void update_fsp_configs(struct fsp_config_data *config, struct fspinit_rtbuf *rt_buf, struct fsp_header *fsp_hdr, u32 stack_top, u32 boot_mode);
This way we avoid saving these 3 parameters into struct fsp_config_data.
That's a lot of parameters. I think a common struct seems better for now. We may see a different approach when newer FSPs turn up.
I'm just struggling to understand the benefit of duplicating this common thing into separate files.
[snip]
Regards, Bin
Regards, Simon
To support platform-specific configurations (might not always be UPD on some platform), use a better name update_fsp_configs() and accepct struct fsp_config_data as its parameter so that platform codes can handle whatever configuration data for that FSP.
Signed-off-by: Bin Meng bmeng.cn@gmail.com ---
arch/x86/cpu/baytrail/fsp_configs.c | 5 +++-- arch/x86/cpu/queensbay/fsp_configs.c | 2 +- arch/x86/include/asm/fsp/fsp_support.h | 6 +++--- arch/x86/lib/fsp/fsp_support.c | 12 ++++++------ 4 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/arch/x86/cpu/baytrail/fsp_configs.c b/arch/x86/cpu/baytrail/fsp_configs.c index a72d615..9810921 100644 --- a/arch/x86/cpu/baytrail/fsp_configs.c +++ b/arch/x86/cpu/baytrail/fsp_configs.c @@ -121,12 +121,13 @@ const struct pch_azalia_config azalia_config = { };
/** - * Override the FSP's UPD. + * Override the FSP's configuration data. * If the device tree does not specify an integer setting, use the default * provided in Intel's Baytrail_FSP_Gold4.tgz release FSP/BayleyBayFsp.bsf file. */ -void update_fsp_upd(struct upd_region *fsp_upd) +void update_fsp_configs(struct fsp_config_data *config) { + struct upd_region *fsp_upd = &config->fsp_upd; struct memory_down_data *mem; const void *blob = gd->fdt_blob; int node; diff --git a/arch/x86/cpu/queensbay/fsp_configs.c b/arch/x86/cpu/queensbay/fsp_configs.c index 78bc966..f84ae30 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/fsp/fsp_support.h>
-void update_fsp_upd(struct upd_region *fsp_upd) +void update_fsp_configs(struct fsp_config_data *config) { /* Override any UPD setting if required */
diff --git a/arch/x86/include/asm/fsp/fsp_support.h b/arch/x86/include/asm/fsp/fsp_support.h index 39b2864..67741cc 100644 --- a/arch/x86/include/asm/fsp/fsp_support.h +++ b/arch/x86/include/asm/fsp/fsp_support.h @@ -192,13 +192,13 @@ void *fsp_get_nvs_data(const void *hob_list, u32 *len); void *fsp_get_bootloader_tmp_mem(const void *hob_list, u32 *len);
/** - * This function overrides the default configurations in the UPD data region. + * This function overrides the default configurations of FSP. * - * @fsp_upd: A pointer to the upd_region data strcture + * @config: A pointer to the FSP configuration data structure * * @return: None */ -void update_fsp_upd(struct upd_region *fsp_upd); +void update_fsp_configs(struct fsp_config_data *config);
/** * fsp_init_phase_pci() - Tell the FSP that we have completed PCI init diff --git a/arch/x86/lib/fsp/fsp_support.c b/arch/x86/lib/fsp/fsp_support.c index 9da720b..faca93e 100644 --- a/arch/x86/lib/fsp/fsp_support.c +++ b/arch/x86/lib/fsp/fsp_support.c @@ -118,6 +118,10 @@ void fsp_init(u32 stack_top, u32 boot_mode, void *nvs_buf) panic("Invalid FSP header"); }
+ config_data.fsp_hdr = fsp_hdr; + config_data.stack_top = stack_top; + config_data.boot_mode = boot_mode; + fsp_upd = &config_data.fsp_upd; memset(&rt_buf, 0, sizeof(struct fspinit_rtbuf));
@@ -140,8 +144,8 @@ void fsp_init(u32 stack_top, u32 boot_mode, void *nvs_buf) /* Verify the UPD data region is valid */ assert(fsp_upd->terminator == UPD_TERMINATOR);
- /* Override any UPD setting if required */ - update_fsp_upd(fsp_upd); + /* Override any configuration if required */ + update_fsp_configs(&config_data);
memset(¶ms, 0, sizeof(struct fsp_init_params)); params.nvs_buf = nvs_buf; @@ -151,10 +155,6 @@ void fsp_init(u32 stack_top, u32 boot_mode, void *nvs_buf) init = (fsp_init_f)(fsp_hdr->img_base + fsp_hdr->fsp_init); params_ptr = ¶ms;
- config_data.fsp_hdr = fsp_hdr; - config_data.stack_top = stack_top; - config_data.boot_mode = boot_mode; - post_code(POST_PRE_MRC);
/* Load GDT for FSP */
On 2 December 2015 at 01:59, Bin Meng bmeng.cn@gmail.com wrote:
To support platform-specific configurations (might not always be UPD on some platform), use a better name update_fsp_configs() and accepct struct fsp_config_data as its parameter so that platform codes can handle whatever configuration data for that FSP.
Signed-off-by: Bin Meng bmeng.cn@gmail.com
arch/x86/cpu/baytrail/fsp_configs.c | 5 +++-- arch/x86/cpu/queensbay/fsp_configs.c | 2 +- arch/x86/include/asm/fsp/fsp_support.h | 6 +++--- arch/x86/lib/fsp/fsp_support.c | 12 ++++++------ 4 files changed, 13 insertions(+), 12 deletions(-)
Acked-by: Simon Glass sjg@chromium.org
All FSP spec v1.0 complaint FSP binary uses struct fspinit_rtbuf as defined by the 1.0 spec, however there are FSPs that does not follow 1.0 spec (possible due to that FSP predates the 1.0 spec), and future FSP binary that is complaint to v1.1 spec defines an optional paltform-specific runtime data in the struct fspinit_rtbuf. Hence move the definition to chipset header.
Signed-off-by: Bin Meng bmeng.cn@gmail.com ---
arch/x86/include/asm/arch-baytrail/fsp/fsp_configs.h | 4 ++++ arch/x86/include/asm/arch-queensbay/fsp/fsp_configs.h | 4 ++++ arch/x86/include/asm/fsp/fsp_platform.h | 15 --------------- arch/x86/include/asm/fsp/fsp_support.h | 1 - 4 files changed, 8 insertions(+), 16 deletions(-) delete mode 100644 arch/x86/include/asm/fsp/fsp_platform.h
diff --git a/arch/x86/include/asm/arch-baytrail/fsp/fsp_configs.h b/arch/x86/include/asm/arch-baytrail/fsp/fsp_configs.h index 2397553..f1da205 100644 --- a/arch/x86/include/asm/arch-baytrail/fsp/fsp_configs.h +++ b/arch/x86/include/asm/arch-baytrail/fsp/fsp_configs.h @@ -14,4 +14,8 @@ struct fsp_config_data { struct upd_region fsp_upd; };
+struct fspinit_rtbuf { + struct common_buf common; /* FSP common runtime data structure */ +}; + #endif /* __FSP_CONFIGS_H__ */ diff --git a/arch/x86/include/asm/arch-queensbay/fsp/fsp_configs.h b/arch/x86/include/asm/arch-queensbay/fsp/fsp_configs.h index 2397553..f1da205 100644 --- a/arch/x86/include/asm/arch-queensbay/fsp/fsp_configs.h +++ b/arch/x86/include/asm/arch-queensbay/fsp/fsp_configs.h @@ -14,4 +14,8 @@ struct fsp_config_data { struct upd_region fsp_upd; };
+struct fspinit_rtbuf { + struct common_buf common; /* FSP common runtime data structure */ +}; + #endif /* __FSP_CONFIGS_H__ */ diff --git a/arch/x86/include/asm/fsp/fsp_platform.h b/arch/x86/include/asm/fsp/fsp_platform.h deleted file mode 100644 index 61286ce..0000000 --- a/arch/x86/include/asm/fsp/fsp_platform.h +++ /dev/null @@ -1,15 +0,0 @@ -/* - * Copyright (C) 2013, Intel Corporation - * Copyright (C) 2014, Bin Meng bmeng.cn@gmail.com - * - * SPDX-License-Identifier: Intel - */ - -#ifndef __FSP_PLATFORM_H__ -#define __FSP_PLATFORM_H__ - -struct fspinit_rtbuf { - struct common_buf common; /* FSP common runtime data structure */ -}; - -#endif diff --git a/arch/x86/include/asm/fsp/fsp_support.h b/arch/x86/include/asm/fsp/fsp_support.h index 67741cc..e65a130 100644 --- a/arch/x86/include/asm/fsp/fsp_support.h +++ b/arch/x86/include/asm/fsp/fsp_support.h @@ -13,7 +13,6 @@ #include "fsp_ffs.h" #include "fsp_api.h" #include "fsp_hob.h" -#include "fsp_platform.h" #include "fsp_infoheader.h" #include "fsp_bootmode.h" #include <asm/arch/fsp/fsp_vpd.h>
On 2 December 2015 at 01:59, Bin Meng bmeng.cn@gmail.com wrote:
All FSP spec v1.0 complaint FSP binary uses struct fspinit_rtbuf as defined by the 1.0 spec, however there are FSPs that does not follow 1.0 spec (possible due to that FSP predates the 1.0 spec), and future FSP binary that is complaint to v1.1 spec defines an optional paltform-specific runtime data in the struct fspinit_rtbuf. Hence move the definition to chipset header.
Signed-off-by: Bin Meng bmeng.cn@gmail.com
arch/x86/include/asm/arch-baytrail/fsp/fsp_configs.h | 4 ++++ arch/x86/include/asm/arch-queensbay/fsp/fsp_configs.h | 4 ++++ arch/x86/include/asm/fsp/fsp_platform.h | 15 --------------- arch/x86/include/asm/fsp/fsp_support.h | 1 - 4 files changed, 8 insertions(+), 16 deletions(-) delete mode 100644 arch/x86/include/asm/fsp/fsp_platform.h
Acked-by: Simon Glass sjg@chromium.org
Since VPD/UPD may not exist on every FSP, move the codes that verifies VPD/UPD to chipset-specific update_fsp_configs(). This also updates update_fsp_configs() signature to accpect FSP runtime buffer pointer as its second parameter.
Signed-off-by: Bin Meng bmeng.cn@gmail.com ---
arch/x86/cpu/baytrail/fsp_configs.c | 24 +++++++++++++++++++++++- arch/x86/cpu/queensbay/fsp_configs.c | 26 +++++++++++++++++++++++++- arch/x86/include/asm/fsp/fsp_support.h | 4 +++- arch/x86/lib/fsp/fsp_support.c | 24 +----------------------- 4 files changed, 52 insertions(+), 26 deletions(-)
diff --git a/arch/x86/cpu/baytrail/fsp_configs.c b/arch/x86/cpu/baytrail/fsp_configs.c index 9810921..3a03392 100644 --- a/arch/x86/cpu/baytrail/fsp_configs.c +++ b/arch/x86/cpu/baytrail/fsp_configs.c @@ -125,13 +125,35 @@ const struct pch_azalia_config azalia_config = { * If the device tree does not specify an integer setting, use the default * provided in Intel's Baytrail_FSP_Gold4.tgz release FSP/BayleyBayFsp.bsf file. */ -void update_fsp_configs(struct fsp_config_data *config) +void update_fsp_configs(struct fsp_config_data *config, + struct fspinit_rtbuf *rt_buf) { + struct fsp_header *fsp_hdr = config->fsp_hdr; + struct vpd_region *fsp_vpd; struct upd_region *fsp_upd = &config->fsp_upd; struct memory_down_data *mem; const void *blob = gd->fdt_blob; int node;
+ /* Initialize runtime buffer for fsp_init() */ + rt_buf->common.stack_top = config->stack_top - 32; + rt_buf->common.boot_mode = config->boot_mode; + rt_buf->common.upd_data = &config->fsp_upd; + + /* Get VPD region start */ + fsp_vpd = (struct vpd_region *)(fsp_hdr->img_base + + fsp_hdr->cfg_region_off); + + /* Verify the VPD data region is valid */ + assert(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)); + + /* Verify the UPD data region is valid */ + assert(fsp_upd->terminator == UPD_TERMINATOR); + fsp_upd->azalia_config_ptr = (uint32_t)&azalia_config;
node = fdtdec_next_compatible(blob, 0, COMPAT_INTEL_BAYTRAIL_FSP); diff --git a/arch/x86/cpu/queensbay/fsp_configs.c b/arch/x86/cpu/queensbay/fsp_configs.c index f84ae30..96ec116 100644 --- a/arch/x86/cpu/queensbay/fsp_configs.c +++ b/arch/x86/cpu/queensbay/fsp_configs.c @@ -8,8 +8,32 @@ #include <common.h> #include <asm/fsp/fsp_support.h>
-void update_fsp_configs(struct fsp_config_data *config) +void update_fsp_configs(struct fsp_config_data *config, + struct fspinit_rtbuf *rt_buf) { + struct fsp_header *fsp_hdr = config->fsp_hdr; + struct vpd_region *fsp_vpd; + struct upd_region *fsp_upd = &config->fsp_upd; + + /* Initialize runtime buffer for fsp_init() */ + rt_buf->common.stack_top = config->stack_top - 32; + rt_buf->common.boot_mode = config->boot_mode; + rt_buf->common.upd_data = &config->fsp_upd; + + /* Get VPD region start */ + fsp_vpd = (struct vpd_region *)(fsp_hdr->img_base + + fsp_hdr->cfg_region_off); + + /* Verify the VPD data region is valid */ + assert(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)); + + /* Verify the UPD data region is valid */ + assert(fsp_upd->terminator == UPD_TERMINATOR); + /* Override any UPD setting if required */
/* Uncomment the line below to enable DEBUG message */ diff --git a/arch/x86/include/asm/fsp/fsp_support.h b/arch/x86/include/asm/fsp/fsp_support.h index e65a130..61d811f 100644 --- a/arch/x86/include/asm/fsp/fsp_support.h +++ b/arch/x86/include/asm/fsp/fsp_support.h @@ -194,10 +194,12 @@ void *fsp_get_bootloader_tmp_mem(const void *hob_list, u32 *len); * This function overrides the default configurations of FSP. * * @config: A pointer to the FSP configuration data structure + * @rt_buf: A pointer to the FSP runtime buffer data structure * * @return: None */ -void update_fsp_configs(struct fsp_config_data *config); +void update_fsp_configs(struct fsp_config_data *config, + struct fspinit_rtbuf *rt_buf);
/** * fsp_init_phase_pci() - Tell the FSP that we have completed PCI init diff --git a/arch/x86/lib/fsp/fsp_support.c b/arch/x86/lib/fsp/fsp_support.c index faca93e..c386b75 100644 --- a/arch/x86/lib/fsp/fsp_support.c +++ b/arch/x86/lib/fsp/fsp_support.c @@ -103,10 +103,8 @@ void fsp_init(u32 stack_top, u32 boot_mode, void *nvs_buf) fsp_init_f init; 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;
#ifdef CONFIG_DEBUG_UART setup_early_uart(); @@ -122,30 +120,10 @@ void fsp_init(u32 stack_top, u32 boot_mode, void *nvs_buf) config_data.stack_top = stack_top; config_data.boot_mode = boot_mode;
- fsp_upd = &config_data.fsp_upd; memset(&rt_buf, 0, sizeof(struct fspinit_rtbuf));
- /* Reserve a gap in stack top */ - rt_buf.common.stack_top = stack_top - 32; - rt_buf.common.boot_mode = boot_mode; - rt_buf.common.upd_data = fsp_upd; - - /* Get VPD region start */ - fsp_vpd = (struct vpd_region *)(fsp_hdr->img_base + - fsp_hdr->cfg_region_off); - - /* Verify the VPD data region is valid */ - assert(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)); - - /* Verify the UPD data region is valid */ - assert(fsp_upd->terminator == UPD_TERMINATOR); - /* Override any configuration if required */ - update_fsp_configs(&config_data); + update_fsp_configs(&config_data, &rt_buf);
memset(¶ms, 0, sizeof(struct fsp_init_params)); params.nvs_buf = nvs_buf;
Hi Bin,
On 2 December 2015 at 01:59, Bin Meng bmeng.cn@gmail.com wrote:
Since VPD/UPD may not exist on every FSP, move the codes that verifies VPD/UPD to chipset-specific update_fsp_configs(). This also updates update_fsp_configs() signature to accpect FSP runtime buffer pointer as its second parameter.
Signed-off-by: Bin Meng bmeng.cn@gmail.com
arch/x86/cpu/baytrail/fsp_configs.c | 24 +++++++++++++++++++++++- arch/x86/cpu/queensbay/fsp_configs.c | 26 +++++++++++++++++++++++++- arch/x86/include/asm/fsp/fsp_support.h | 4 +++- arch/x86/lib/fsp/fsp_support.c | 24 +----------------------- 4 files changed, 52 insertions(+), 26 deletions(-)
diff --git a/arch/x86/cpu/baytrail/fsp_configs.c b/arch/x86/cpu/baytrail/fsp_configs.c index 9810921..3a03392 100644 --- a/arch/x86/cpu/baytrail/fsp_configs.c +++ b/arch/x86/cpu/baytrail/fsp_configs.c @@ -125,13 +125,35 @@ const struct pch_azalia_config azalia_config = {
- If the device tree does not specify an integer setting, use the default
- provided in Intel's Baytrail_FSP_Gold4.tgz release FSP/BayleyBayFsp.bsf file.
*/ -void update_fsp_configs(struct fsp_config_data *config) +void update_fsp_configs(struct fsp_config_data *config,
struct fspinit_rtbuf *rt_buf){
struct fsp_header *fsp_hdr = config->fsp_hdr;struct vpd_region *fsp_vpd; struct upd_region *fsp_upd = &config->fsp_upd; struct memory_down_data *mem; const void *blob = gd->fdt_blob; int node;/* Initialize runtime buffer for fsp_init() */rt_buf->common.stack_top = config->stack_top - 32;rt_buf->common.boot_mode = config->boot_mode;rt_buf->common.upd_data = &config->fsp_upd;/* Get VPD region start */fsp_vpd = (struct vpd_region *)(fsp_hdr->img_base +fsp_hdr->cfg_region_off);/* Verify the VPD data region is valid */assert(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));/* Verify the UPD data region is valid */assert(fsp_upd->terminator == UPD_TERMINATOR);
Maybe rather than duplicating this code, it can go in a common function that is called for these two boards?
fsp_upd->azalia_config_ptr = (uint32_t)&azalia_config; node = fdtdec_next_compatible(blob, 0, COMPAT_INTEL_BAYTRAIL_FSP);diff --git a/arch/x86/cpu/queensbay/fsp_configs.c b/arch/x86/cpu/queensbay/fsp_configs.c index f84ae30..96ec116 100644 --- a/arch/x86/cpu/queensbay/fsp_configs.c +++ b/arch/x86/cpu/queensbay/fsp_configs.c @@ -8,8 +8,32 @@ #include <common.h> #include <asm/fsp/fsp_support.h>
-void update_fsp_configs(struct fsp_config_data *config) +void update_fsp_configs(struct fsp_config_data *config,
struct fspinit_rtbuf *rt_buf){
struct fsp_header *fsp_hdr = config->fsp_hdr;struct vpd_region *fsp_vpd;struct upd_region *fsp_upd = &config->fsp_upd;/* Initialize runtime buffer for fsp_init() */rt_buf->common.stack_top = config->stack_top - 32;rt_buf->common.boot_mode = config->boot_mode;rt_buf->common.upd_data = &config->fsp_upd;/* Get VPD region start */fsp_vpd = (struct vpd_region *)(fsp_hdr->img_base +fsp_hdr->cfg_region_off);/* Verify the VPD data region is valid */assert(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));/* Verify the UPD data region is valid */assert(fsp_upd->terminator == UPD_TERMINATOR);/* Override any UPD setting if required */ /* Uncomment the line below to enable DEBUG message */diff --git a/arch/x86/include/asm/fsp/fsp_support.h b/arch/x86/include/asm/fsp/fsp_support.h index e65a130..61d811f 100644 --- a/arch/x86/include/asm/fsp/fsp_support.h +++ b/arch/x86/include/asm/fsp/fsp_support.h @@ -194,10 +194,12 @@ void *fsp_get_bootloader_tmp_mem(const void *hob_list, u32 *len);
- This function overrides the default configurations of FSP.
- @config: A pointer to the FSP configuration data structure
*/
- @rt_buf: A pointer to the FSP runtime buffer data structure
- @return: None
-void update_fsp_configs(struct fsp_config_data *config); +void update_fsp_configs(struct fsp_config_data *config,
struct fspinit_rtbuf *rt_buf);/**
- fsp_init_phase_pci() - Tell the FSP that we have completed PCI init
diff --git a/arch/x86/lib/fsp/fsp_support.c b/arch/x86/lib/fsp/fsp_support.c index faca93e..c386b75 100644 --- a/arch/x86/lib/fsp/fsp_support.c +++ b/arch/x86/lib/fsp/fsp_support.c @@ -103,10 +103,8 @@ void fsp_init(u32 stack_top, u32 boot_mode, void *nvs_buf) fsp_init_f init; 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;#ifdef CONFIG_DEBUG_UART setup_early_uart(); @@ -122,30 +120,10 @@ void fsp_init(u32 stack_top, u32 boot_mode, void *nvs_buf) config_data.stack_top = stack_top; config_data.boot_mode = boot_mode;
fsp_upd = &config_data.fsp_upd; memset(&rt_buf, 0, sizeof(struct fspinit_rtbuf));/* Reserve a gap in stack top */rt_buf.common.stack_top = stack_top - 32;rt_buf.common.boot_mode = boot_mode;rt_buf.common.upd_data = fsp_upd;/* Get VPD region start */fsp_vpd = (struct vpd_region *)(fsp_hdr->img_base +fsp_hdr->cfg_region_off);/* Verify the VPD data region is valid */assert(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));/* Verify the UPD data region is valid */assert(fsp_upd->terminator == UPD_TERMINATOR);/* Override any configuration if required */update_fsp_configs(&config_data);
update_fsp_configs(&config_data, &rt_buf); memset(¶ms, 0, sizeof(struct fsp_init_params)); params.nvs_buf = nvs_buf;-- 1.8.2.1
Regards, Simon
Hi Simon,
On Thu, Dec 3, 2015 at 5:05 AM, Simon Glass sjg@chromium.org wrote:
Hi Bin,
On 2 December 2015 at 01:59, Bin Meng bmeng.cn@gmail.com wrote:
Since VPD/UPD may not exist on every FSP, move the codes that verifies VPD/UPD to chipset-specific update_fsp_configs(). This also updates update_fsp_configs() signature to accpect FSP runtime buffer pointer as its second parameter.
Signed-off-by: Bin Meng bmeng.cn@gmail.com
arch/x86/cpu/baytrail/fsp_configs.c | 24 +++++++++++++++++++++++- arch/x86/cpu/queensbay/fsp_configs.c | 26 +++++++++++++++++++++++++- arch/x86/include/asm/fsp/fsp_support.h | 4 +++- arch/x86/lib/fsp/fsp_support.c | 24 +----------------------- 4 files changed, 52 insertions(+), 26 deletions(-)
diff --git a/arch/x86/cpu/baytrail/fsp_configs.c b/arch/x86/cpu/baytrail/fsp_configs.c index 9810921..3a03392 100644 --- a/arch/x86/cpu/baytrail/fsp_configs.c +++ b/arch/x86/cpu/baytrail/fsp_configs.c @@ -125,13 +125,35 @@ const struct pch_azalia_config azalia_config = {
- If the device tree does not specify an integer setting, use the default
- provided in Intel's Baytrail_FSP_Gold4.tgz release FSP/BayleyBayFsp.bsf file.
*/ -void update_fsp_configs(struct fsp_config_data *config) +void update_fsp_configs(struct fsp_config_data *config,
struct fspinit_rtbuf *rt_buf){
struct fsp_header *fsp_hdr = config->fsp_hdr;struct vpd_region *fsp_vpd; struct upd_region *fsp_upd = &config->fsp_upd; struct memory_down_data *mem; const void *blob = gd->fdt_blob; int node;/* Initialize runtime buffer for fsp_init() */rt_buf->common.stack_top = config->stack_top - 32;rt_buf->common.boot_mode = config->boot_mode;rt_buf->common.upd_data = &config->fsp_upd;/* Get VPD region start */fsp_vpd = (struct vpd_region *)(fsp_hdr->img_base +fsp_hdr->cfg_region_off);/* Verify the VPD data region is valid */assert(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));/* Verify the UPD data region is valid */assert(fsp_upd->terminator == UPD_TERMINATOR);Maybe rather than duplicating this code, it can go in a common function that is called for these two boards?
Yes, we can create a common function to do these, but that requires we create a Kconfig option something like CONFIG_FSP_USE_UPD or CONFIG_FSP_NO_UPD to wrap these codes in arch/x86/lib/fsp_support.c. I don't want to create a Kconfig option so chose to duplicate the codes. What do you think?
[snip]
Regards, Bin
Hi Bin,
On 2 December 2015 at 22:22, Bin Meng bmeng.cn@gmail.com wrote:
Hi Simon,
On Thu, Dec 3, 2015 at 5:05 AM, Simon Glass sjg@chromium.org wrote:
Hi Bin,
On 2 December 2015 at 01:59, Bin Meng bmeng.cn@gmail.com wrote:
Since VPD/UPD may not exist on every FSP, move the codes that verifies VPD/UPD to chipset-specific update_fsp_configs(). This also updates update_fsp_configs() signature to accpect FSP runtime buffer pointer as its second parameter.
Signed-off-by: Bin Meng bmeng.cn@gmail.com
arch/x86/cpu/baytrail/fsp_configs.c | 24 +++++++++++++++++++++++- arch/x86/cpu/queensbay/fsp_configs.c | 26 +++++++++++++++++++++++++- arch/x86/include/asm/fsp/fsp_support.h | 4 +++- arch/x86/lib/fsp/fsp_support.c | 24 +----------------------- 4 files changed, 52 insertions(+), 26 deletions(-)
diff --git a/arch/x86/cpu/baytrail/fsp_configs.c b/arch/x86/cpu/baytrail/fsp_configs.c index 9810921..3a03392 100644 --- a/arch/x86/cpu/baytrail/fsp_configs.c +++ b/arch/x86/cpu/baytrail/fsp_configs.c @@ -125,13 +125,35 @@ const struct pch_azalia_config azalia_config = {
- If the device tree does not specify an integer setting, use the default
- provided in Intel's Baytrail_FSP_Gold4.tgz release FSP/BayleyBayFsp.bsf file.
*/ -void update_fsp_configs(struct fsp_config_data *config) +void update_fsp_configs(struct fsp_config_data *config,
struct fspinit_rtbuf *rt_buf){
struct fsp_header *fsp_hdr = config->fsp_hdr;struct vpd_region *fsp_vpd; struct upd_region *fsp_upd = &config->fsp_upd; struct memory_down_data *mem; const void *blob = gd->fdt_blob; int node;/* Initialize runtime buffer for fsp_init() */rt_buf->common.stack_top = config->stack_top - 32;rt_buf->common.boot_mode = config->boot_mode;rt_buf->common.upd_data = &config->fsp_upd;/* Get VPD region start */fsp_vpd = (struct vpd_region *)(fsp_hdr->img_base +fsp_hdr->cfg_region_off);/* Verify the VPD data region is valid */assert(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));/* Verify the UPD data region is valid */assert(fsp_upd->terminator == UPD_TERMINATOR);Maybe rather than duplicating this code, it can go in a common function that is called for these two boards?
Yes, we can create a common function to do these, but that requires we create a Kconfig option something like CONFIG_FSP_USE_UPD or CONFIG_FSP_NO_UPD to wrap these codes in arch/x86/lib/fsp_support.c. I don't want to create a Kconfig option so chose to duplicate the codes. What do you think?
Well I dislike duplicated code more, so I think CONFIG_FSP_USE_UPD might be best.
[snip]
Regards, Bin
Regards, Simon
Those comments in update_fsp_upd() are not correct. Remove them.
Signed-off-by: Bin Meng bmeng.cn@gmail.com
---
arch/x86/cpu/queensbay/fsp_configs.c | 6 ------ 1 file changed, 6 deletions(-)
diff --git a/arch/x86/cpu/queensbay/fsp_configs.c b/arch/x86/cpu/queensbay/fsp_configs.c index 96ec116..faa79d1 100644 --- a/arch/x86/cpu/queensbay/fsp_configs.c +++ b/arch/x86/cpu/queensbay/fsp_configs.c @@ -35,10 +35,4 @@ void update_fsp_configs(struct fsp_config_data *config, assert(fsp_upd->terminator == UPD_TERMINATOR);
/* Override any UPD setting if required */ - - /* Uncomment the line below to enable DEBUG message */ - /* fsp_upd->serial_dbgport_type = 1; */ - - /* Examples on how to initialize the pointers in UPD region */ - /* fsp_upd->pcd_example = (EXAMPLE_DATA *)&example; */ }
On 2 December 2015 at 01:59, Bin Meng bmeng.cn@gmail.com wrote:
Those comments in update_fsp_upd() are not correct. Remove them.
Signed-off-by: Bin Meng bmeng.cn@gmail.com
arch/x86/cpu/queensbay/fsp_configs.c | 6 ------ 1 file changed, 6 deletions(-)
Acked-by: Simon Glass sjg@chromium.org
participants (2)
-
Bin Meng -
Simon Glass