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.
> I'm just struggling to understand the benefit of duplicating this
> common thing into separate files.
>
[snip]
Regards,
Bin
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
