| Message ID | 1449046744-12964-5-git-send-email-bmeng.cn@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Bin Meng |
| Headers | show |
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
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);
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