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