Hi Bin, On 2 December 2015 at 22:18, Bin Meng <bmeng...@gmail.com> wrote: > Hi Simon, > > On Thu, Dec 3, 2015 at 5:05 AM, Simon Glass <s...@chromium.org> wrote: >> Hi Bin, >> >> On 2 December 2015 at 01:59, Bin Meng <bmeng...@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...@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...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot