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

[Simon Glass]

Hi Bin,

On 3 January 2015 at 20:41, Bin Meng <bmeng.cn at gmail.com> wrote:
> 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.

OK, that sounds good.

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

I'd say link for now since it is the only real hardware tested. But
perhaps when qemu is better supported we could switch to that if you
prefer.

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

OK. As you say we need to move CONFIG_X86_RESET_VECTOR to Kconfig.

Regards,
Simon