[U-Boot] [PATCH 3/3] x86: coreboot: Wrap cros_ec initialization

[Bin Meng]

Hi Simon,

On Sun, Jan 4, 2015 at 11:19 AM, Simon Glass <sjg at chromium.org> wrote:
> Hi Bin,
>
> On 3 January 2015 at 20:12, Bin Meng <bmeng.cn at gmail.com> wrote:
>> Hi Simon,
>>
>> On Sun, Jan 4, 2015 at 11:01 AM, Simon Glass <sjg at chromium.org> wrote:
>>> Hi Bin,
>>>
>>> On 3 January 2015 at 19:58, Bin Meng <bmeng.cn at gmail.com> wrote:
>>>> Hi Simon,
>>>>
>>>> On Sun, Jan 4, 2015 at 10:33 AM, Simon Glass <sjg at chromium.org> wrote:
>>>>> Hi Bin,
>>>>>
>>>>> On 3 January 2015 at 07:40, Bin Meng <bmeng.cn at gmail.com> wrote:
>>>>>> cros_ec_board_init() should be called only when CONFIG_CROS_EC is
>>>>>> enabled. Also undef CONFIG_CROS_EC in the coreboot configuration.
>>>>>>
>>>>>> Signed-off-by: Bin Meng <bmeng.cn at gmail.com>
>>>>>>
>>>>>> ---
>>>>>>
>>>>>>  board/coreboot/coreboot/coreboot.c | 2 ++
>>>>>>  include/configs/coreboot.h         | 4 +++-
>>>>>>  2 files changed, 5 insertions(+), 1 deletion(-)
>>>>>
>>>>> Can we just remove the node in the device tree? The current 'coreboot'
>>>>> config is designed to run on link (Chromebook Pixel) so it does have
>>>>> an EC. Maybe we should have a separate device tree file for the qemu
>>>>> version?
>>>>>
>>>>
>>>> Looks that removing ec node from dts should work with current code
>>>> logic in cros_ec_init(). Yes, we can have a separate device tree file
>>>> for maybe a generic board (not naming it as qemu.dts), and make this
>>>> generic board dts file as the default dts for coreboot board? How
>>>> about the defines in coreboot.h? Should we make it undefined like I
>>>> did in this patch?
>>>
>>> That sounds good, but I would prefer to use the same board config file
>>> if possible, perhaps just changing the CONFIG_DEFAULT_DEVICE_TREE?
>>
>> Yes, just changing the CONFIG_DEFAULT_DEVICE_TREE to the same board
>> dts file, say for example, I can change CONFIG_DEFAULT_DEVICE_TREE to
>> use crownbay.dts to build a U-Boot to be loaded by coreboot. The two
>> question remain: which board dts file should be used as the default
>> one in coreboot-x86_defconfig file? And how about those CROS_EX
>> defines in coreboot.h? Right now we have SYS_CONFIG_NAME but it is not
>> a visible Kconfig option so we cannot change it. Ideally we should
>> just change CONFIG_DEFAULT_DEVICE_TREE and SYS_CONFIG_NAME to allow
>> the U-Boot for coreboot to run on different boards.
>
> At present it is link, which is not idea for qemu. We probably need an
> easier way to select from multiple device trees to use for a board
> config. But for now perhaps we should have:
>
> - coreboot-link
> - coreboot-generic (qemu)
> - chromebook_link
> - crownbay

I mean if we are building U-Boot for coreboot, we just need change two
things in Kconfig: CONFIG_DEFAULT_DEVICE_TREE and SYS_CONFIG_NAME. We
should use the same dts file (arch/x86/dts/xxx.dts) for the board we
want U-Boot to run on, as dts file is a file to describe the hardware
information on a board. Similarly, we should use the same board
configuration file (include/configs/xxx.h) as defined by
SYS_CONFIG_NAME. Note since U-Boot for coreboot does not need run from
reset vector, we should make CONFIG_X86_RESET_VECTOR a Kconfig option
too so that we can switch it on/off. This coreboot build process
should be properly documented in README.x86.

> The first two can be the same apart from the device tree. From
> experience once configs split it is really hard to get them joined, so
> we should keep an eye on this.

The question is: with above build procedure, which board should we put
in coreboot-x86_defconfig as the default dts file name shown in
Kconfig?

> If I understand you correctly, I'd prefer to keep the CROS_EC defines
> in coreboot.h, meaning that the function is available if enabled by
> device tree.

With above procedure, I think we need use link.h directly for
chromebook_link to run U-Boot with coreboot. The coreboot.h should be
removed.

Regards,
Bin