[U-Boot] [PATCH 11/13] x86: braswell: Add FSP configuration

[Bin Meng]

Hi Simon,

On Wed, Sep 6, 2017 at 9:39 AM, Simon Glass <sjg at chromium.org> wrote:
> Hi Bin,
>
> On 26 August 2017 at 18:10, Bin Meng <bmeng.cn at gmail.com> wrote:
>> Hi Simon,
>>
>> On Sun, Aug 27, 2017 at 6:39 AM, Simon Glass <sjg at chromium.org> wrote:
>>> Hi Bin,
>>>
>>> On 26 August 2017 at 07:56, Bin Meng <bmeng.cn at gmail.com> wrote:
>>>> Hi Simon,
>>>>
>>>> On Sat, Aug 26, 2017 at 9:39 PM, Simon Glass <sjg at chromium.org> wrote:
>>>>> On 15 August 2017 at 23:42, Bin Meng <bmeng.cn at gmail.com> wrote:
>>>>>> Add FSP related configuration for Braswell.
>>>>>>
>>>>>> Signed-off-by: Bin Meng <bmeng.cn at gmail.com>
>>>>>> ---
>>>>>>
>>>>>>  arch/x86/cpu/braswell/Makefile                     |   2 +-
>>>>>>  arch/x86/cpu/braswell/fsp_configs.c                | 158 ++++++++++++++
>>>>>>  .../include/asm/arch-braswell/fsp/fsp_configs.h    |  89 ++++++++
>>>>>>  arch/x86/include/asm/arch-braswell/fsp/fsp_vpd.h   | 172 +++++++++++++++
>>>>>>  arch/x86/include/asm/arch-braswell/gpio.h          | 234 +++++++++++++++++++++
>>>>>>  5 files changed, 654 insertions(+), 1 deletion(-)
>>>>>>  create mode 100644 arch/x86/cpu/braswell/fsp_configs.c
>>>>>>  create mode 100644 arch/x86/include/asm/arch-braswell/fsp/fsp_configs.h
>>>>>>  create mode 100644 arch/x86/include/asm/arch-braswell/fsp/fsp_vpd.h
>>>>>>  create mode 100644 arch/x86/include/asm/arch-braswell/gpio.h
>>>>>>
>>>>>
>>>>> Reviewed-by: Simon Glass <sjg at chromium.org>
>>>>>
>>>>> Can this use drivers instead of manual device-tree access?
>>>>
>>>> Which part?
>>>
>>> Well you have intel,braswell-fsp for example. You could create a
>>> driver with the two compatible strings and have it read the platdata
>>> from the DT in the ofdata_to_platdata() method.
>>
>> I thought this before. We discussed the possibility of adding a new
>> FSP uclass long time ago. When I added the Braswell support, I wanted
>> to have a try since Braswell's FSP is v1.1 spec complaint and if we
>> have a uclass for FSP we can put the common stuff in the uclass
>> driver. But in the end I did not do it because:
>>
>> 1. FSP's initialization sequence is just a one time initialization and
>> we don't do anything after the initialization completes.
>
> That's not a very good reason though. There will be several drivers like that.
>
>> 2. Making a uclass for FSP means we have to delay fsp_init() to after
>> initf_dm().But after fsp_init(), we will return to board_init_f()
>> again and do the initialization for the second time. So all previous
>> platdata of FSP that is set up by DM gets lost during this process.
>
> Yes, although this is in the nature of the broken FSP API that we hope
> Intel will fix. As I understand it we already do the init twice, this
> is just a case of knowing what stage we are at.
>

But DM initialization is unnecessary to get FSP run. This will lead
longer boot time.

>> 3. There are some other architecture-dependent stuff in the
>> arch_fsp_init() that is not suitable to be put in a FSP driver.
>
> But I am suggesting having a driver specific to the arch, not a
> generic one, so this should not be a problem.

I mean there will be some non-FSP driver stuff (eg: MRC cache, ACPI
S3) in the FSP driver, which doesn't look very good IMO.

Regards,
Bin