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

[Simon Glass]

Hi Bin,

On 12 September 2017 at 09:20, Bin Meng <bmeng.cn at gmail.com> wrote:
> 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.

Looking at the init sequence in board_f(), we have fsp_init() quite early:

#if defined(CONFIG_HAVE_FSP)
arch_fsp_init,
#endif
arch_cpu_init, /* basic arch cpu dependent setup */
mach_cpu_init, /* SoC/machine dependent CPU setup */
initf_dm,
arch_cpu_init_dm,

I still feel that ultimately FSP should be a driver (and should happen
before arch_cpu_init_dm(), and that this should not affect boot time
(since we need to do DM init at some point), but I think this needs
more investigation. We would need to first empty out arch_cpu_init()
and march_cpu_init().

So for now, let's leave it as is.

>
>>> 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.

Well given the nature of FSP (whole platform init) I think this is
fine. The FSP driver can call out to common code or pull in other
drivers as needed.

Regards,
Simon