| Message ID | 1420296026-8764-4-git-send-email-bmeng.cn@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Simon Glass |
| Headers | show |
Hi Bin, On 3 January 2015 at 07:40, Bin Meng <bmeng.cn@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@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? > > diff --git a/board/coreboot/coreboot/coreboot.c b/board/coreboot/coreboot/coreboot.c > index 154faf6..e076ea6 100644 > --- a/board/coreboot/coreboot/coreboot.c > +++ b/board/coreboot/coreboot/coreboot.c > @@ -10,8 +10,10 @@ > > int arch_early_init_r(void) > { > +#ifdef CONFIG_CROS_EC > if (cros_ec_board_init()) > return -1; > +#endif > > return 0; > } > diff --git a/include/configs/coreboot.h b/include/configs/coreboot.h > index 990a2d1..e5fe5e0 100644 > --- a/include/configs/coreboot.h > +++ b/include/configs/coreboot.h > @@ -67,9 +67,11 @@ > > #define CONFIG_BOOTDELAY 2 > > -#define CONFIG_CROS_EC > +#undef CONFIG_CROS_EC > +#ifdef CONFIG_CROS_EC > #define CONFIG_CROS_EC_LPC > #define CONFIG_CMD_CROS_EC > +#endif > #define CONFIG_ARCH_EARLY_INIT_R > > #endif /* __CONFIG_H */ > -- > 1.8.2.1 > Regards, Simon
Hi Simon, On Sun, Jan 4, 2015 at 10:33 AM, Simon Glass <sjg@chromium.org> wrote: > Hi Bin, > > On 3 January 2015 at 07:40, Bin Meng <bmeng.cn@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@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? [snip] Regards, Bin
Hi Bin, On 3 January 2015 at 19:58, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Simon, > > On Sun, Jan 4, 2015 at 10:33 AM, Simon Glass <sjg@chromium.org> wrote: >> Hi Bin, >> >> On 3 January 2015 at 07:40, Bin Meng <bmeng.cn@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@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? Regards, Simon
Hi Simon, On Sun, Jan 4, 2015 at 11:01 AM, Simon Glass <sjg@chromium.org> wrote: > Hi Bin, > > On 3 January 2015 at 19:58, Bin Meng <bmeng.cn@gmail.com> wrote: >> Hi Simon, >> >> On Sun, Jan 4, 2015 at 10:33 AM, Simon Glass <sjg@chromium.org> wrote: >>> Hi Bin, >>> >>> On 3 January 2015 at 07:40, Bin Meng <bmeng.cn@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@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. Regards, Bin
Hi Bin, On 3 January 2015 at 20:12, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Simon, > > On Sun, Jan 4, 2015 at 11:01 AM, Simon Glass <sjg@chromium.org> wrote: >> Hi Bin, >> >> On 3 January 2015 at 19:58, Bin Meng <bmeng.cn@gmail.com> wrote: >>> Hi Simon, >>> >>> On Sun, Jan 4, 2015 at 10:33 AM, Simon Glass <sjg@chromium.org> wrote: >>>> Hi Bin, >>>> >>>> On 3 January 2015 at 07:40, Bin Meng <bmeng.cn@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@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 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. 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. Regards, Simon
Hi Simon, On Sun, Jan 4, 2015 at 11:19 AM, Simon Glass <sjg@chromium.org> wrote: > Hi Bin, > > On 3 January 2015 at 20:12, Bin Meng <bmeng.cn@gmail.com> wrote: >> Hi Simon, >> >> On Sun, Jan 4, 2015 at 11:01 AM, Simon Glass <sjg@chromium.org> wrote: >>> Hi Bin, >>> >>> On 3 January 2015 at 19:58, Bin Meng <bmeng.cn@gmail.com> wrote: >>>> Hi Simon, >>>> >>>> On Sun, Jan 4, 2015 at 10:33 AM, Simon Glass <sjg@chromium.org> wrote: >>>>> Hi Bin, >>>>> >>>>> On 3 January 2015 at 07:40, Bin Meng <bmeng.cn@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@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
Hi Bin, On 3 January 2015 at 20:41, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Simon, > > On Sun, Jan 4, 2015 at 11:19 AM, Simon Glass <sjg@chromium.org> wrote: >> Hi Bin, >> >> On 3 January 2015 at 20:12, Bin Meng <bmeng.cn@gmail.com> wrote: >>> Hi Simon, >>> >>> On Sun, Jan 4, 2015 at 11:01 AM, Simon Glass <sjg@chromium.org> wrote: >>>> Hi Bin, >>>> >>>> On 3 January 2015 at 19:58, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>> Hi Simon, >>>>> >>>>> On Sun, Jan 4, 2015 at 10:33 AM, Simon Glass <sjg@chromium.org> wrote: >>>>>> Hi Bin, >>>>>> >>>>>> On 3 January 2015 at 07:40, Bin Meng <bmeng.cn@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@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
diff --git a/board/coreboot/coreboot/coreboot.c b/board/coreboot/coreboot/coreboot.c index 154faf6..e076ea6 100644 --- a/board/coreboot/coreboot/coreboot.c +++ b/board/coreboot/coreboot/coreboot.c @@ -10,8 +10,10 @@ int arch_early_init_r(void) { +#ifdef CONFIG_CROS_EC if (cros_ec_board_init()) return -1; +#endif return 0; } diff --git a/include/configs/coreboot.h b/include/configs/coreboot.h index 990a2d1..e5fe5e0 100644 --- a/include/configs/coreboot.h +++ b/include/configs/coreboot.h @@ -67,9 +67,11 @@ #define CONFIG_BOOTDELAY 2 -#define CONFIG_CROS_EC +#undef CONFIG_CROS_EC +#ifdef CONFIG_CROS_EC #define CONFIG_CROS_EC_LPC #define CONFIG_CMD_CROS_EC +#endif #define CONFIG_ARCH_EARLY_INIT_R #endif /* __CONFIG_H */
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@gmail.com> --- board/coreboot/coreboot/coreboot.c | 2 ++ include/configs/coreboot.h | 4 +++- 2 files changed, 5 insertions(+), 1 deletion(-)