[U-Boot] [PATCH 4/8] x86: fsp: Rename shared_data to fsp_config_data

[Simon Glass]

Hi Bin,

On 2 December 2015 at 22:18, Bin Meng <bmeng.cn at gmail.com> wrote:
> Hi Simon,
>
> On Thu, Dec 3, 2015 at 5:05 AM, Simon Glass <sjg at chromium.org> wrote:
>> Hi Bin,
>>
>> On 2 December 2015 at 01:59, Bin Meng <bmeng.cn at 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 at 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 at 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