[U-Boot] [PATCH 00/16] RFC: Board init using driver model
At present we have a lot of ad-hoc init functions related to boards, for example board_early_init_f(), board_misc_init_f() and dram_init().
There are used in different ways by different boards as useful hooks to do the required init and sequence it correctly. Some functions are always enabled but have a __weak default. Some are controlled by the existence of a CONFIG.
There are two main init sequences: board_init_f() (f for running from read-only flash) which runs before relocation and board_init_r() (r for relocated) which runs afterwards.
One problem with the current sequence is that it has a lot of arch-specific #ifdefs around various functions. There are also #ifdefs for various features. There has been quite a bit of discussion about how to tidy this up and at least one RFC series[1].
Now that we have driver model we can use this to deal with the init sequences. This approach has several advantages:
- We have a path to remove the #ifdefs - It is easy for multiple parts of the code to implement the same hook - We can track what is called and what is not - We don't need weak functions - We can eventually adjust the sequence to improve naming or to add new init phases - It provides a model for how we might deal with ft_board_setup() and friends
This series starts the process of replacing the pre-relocation init sequence with a driver-model solution. It defines a uclass, adds tests and converts sandbox and a few x86 boards over to use this new setup.
This series is not ready for use yet as the rest of the init sequence hooks need to be converted. But there is enough here to show the idea.
Comments welcome.
[1] https://lists.denx.de/pipermail/u-boot/2011-August/098718.html
Simon Glass (16): x86: Drop leading spaces in cpu_x86_get_desc() x86: Display the SPL banner only once x86: config: Enable dhrystone command for link dm: board: Add a uclass for init functions dm: board: Add a command to view phase info dm: board: Adjust pre-relocation init hooks dm: board: Support printing CPU info using the uclass dm: sandbox: Convert to using driver-model baord init dm: board: Add tests for the board uclass dm: board: Add documentation dm: x86: board: Enable CONFIG_BOARD dm: x86: board: Add a BOARD_F_RESERVE_ARCH driver dm: x86: board: ivybridge: Add support for CONFIG_BOARD dm: x86: board: ivybridge: Switch to use CONFIG_BOARD dm: x86: board: ivybridge: Remove old board init code dm: board: Add information about the new board init to the README
README | 3 + arch/Kconfig | 5 + arch/sandbox/dts/test.dts | 12 ++ arch/sandbox/include/asm/state.h | 3 + arch/sandbox/include/asm/test.h | 9 ++ arch/x86/cpu/coreboot/coreboot.c | 2 + arch/x86/cpu/cpu.c | 41 ++++- arch/x86/cpu/cpu_x86.c | 6 +- arch/x86/cpu/ivybridge/cpu.c | 57 +++++-- arch/x86/cpu/ivybridge/sdram.c | 6 +- arch/x86/cpu/ivybridge/sdram_nop.c | 36 ++++- arch/x86/cpu/x86_64/cpu.c | 2 + arch/x86/include/asm/arch-ivybridge/sandybridge.h | 7 + arch/x86/lib/spl.c | 11 +- board/google/chromebook_link/link.c | 5 - board/google/chromebox_panther/panther.c | 5 - board/sandbox/sandbox.c | 44 +++++- cmd/Kconfig | 8 + cmd/Makefile | 1 + cmd/board.c | 58 +++++++ common/Kconfig | 31 ++++ common/board_f.c | 58 ++++++- common/init/Makefile | 1 + common/init/board-uclass.c | 108 +++++++++++++ configs/chromebook_link64_defconfig | 2 + configs/chromebook_link_defconfig | 2 + configs/chromebox_panther_defconfig | 1 + doc/driver-model/board-info.txt | 181 ++++++++++++++++++++++ drivers/cpu/cpu-uclass.c | 19 +++ drivers/misc/Makefile | 1 + drivers/misc/board_sandbox.c | 44 ++++++ include/asm-generic/global_data.h | 5 + include/board.h | 170 ++++++++++++++++++++ include/cpu.h | 5 + include/dm/uclass-id.h | 1 + test/dm/Makefile | 1 + test/dm/board.c | 74 +++++++++ 37 files changed, 980 insertions(+), 45 deletions(-) create mode 100644 cmd/board.c create mode 100644 common/init/board-uclass.c create mode 100644 doc/driver-model/board-info.txt create mode 100644 drivers/misc/board_sandbox.c create mode 100644 include/board.h create mode 100644 test/dm/board.c
The Intel CPU name can have leading spaces. Remove them since they are not useful.
Signed-off-by: Simon Glass sjg@chromium.org ---
arch/x86/cpu/cpu_x86.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/arch/x86/cpu/cpu_x86.c b/arch/x86/cpu/cpu_x86.c index 8be14b5929..b465b14a94 100644 --- a/arch/x86/cpu/cpu_x86.c +++ b/arch/x86/cpu/cpu_x86.c @@ -41,10 +41,14 @@ int cpu_x86_get_vendor(struct udevice *dev, char *buf, int size)
int cpu_x86_get_desc(struct udevice *dev, char *buf, int size) { + char *ptr; + if (size < CPU_MAX_NAME_LEN) return -ENOSPC;
- cpu_get_name(buf); + ptr = cpu_get_name(buf); + if (ptr != buf) + strcpy(buf, ptr);
return 0; }
On Mon, Mar 20, 2017 at 2:59 AM, Simon Glass sjg@chromium.org wrote:
The Intel CPU name can have leading spaces. Remove them since they are not useful.
Signed-off-by: Simon Glass sjg@chromium.org
arch/x86/cpu/cpu_x86.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
Reviewed-by: Bin Meng bmeng.cn@gmail.com
On Wed, Apr 12, 2017 at 11:09 AM, Bin Meng bmeng.cn@gmail.com wrote:
On Mon, Mar 20, 2017 at 2:59 AM, Simon Glass sjg@chromium.org wrote:
The Intel CPU name can have leading spaces. Remove them since they are not useful.
Signed-off-by: Simon Glass sjg@chromium.org
arch/x86/cpu/cpu_x86.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
Reviewed-by: Bin Meng bmeng.cn@gmail.com
applied to u-boot-x86, thanks!
At present on a cold reboot we must reset the CPU to get it to full speed. With 64-bit U-Boot this happens in SPL. At present we print the banner before doing this, the end result being that we print the banner twice. Print the banner a little later (after the CPU is ready) to avoid this.
Signed-off-by: Simon Glass sjg@chromium.org ---
arch/x86/lib/spl.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/arch/x86/lib/spl.c b/arch/x86/lib/spl.c index ed2d40b552..fa93d64a7a 100644 --- a/arch/x86/lib/spl.c +++ b/arch/x86/lib/spl.c @@ -37,8 +37,6 @@ static int x86_spl_init(void) debug("%s: spl_init() failed\n", __func__); return ret; } - preloader_console_init(); - ret = arch_cpu_init(); if (ret) { debug("%s: arch_cpu_init() failed\n", __func__); @@ -49,6 +47,7 @@ static int x86_spl_init(void) debug("%s: arch_cpu_init_dm() failed\n", __func__); return ret; } + preloader_console_init(); ret = print_cpuinfo(); if (ret) { debug("%s: print_cpuinfo() failed\n", __func__);
Hi Simon,
On Mon, Mar 20, 2017 at 2:59 AM, Simon Glass sjg@chromium.org wrote:
At present on a cold reboot we must reset the CPU to get it to full speed. With 64-bit U-Boot this happens in SPL. At present we print the banner before doing this, the end result being that we print the banner twice. Print the banner a little later (after the CPU is ready) to avoid this.
Signed-off-by: Simon Glass sjg@chromium.org
arch/x86/lib/spl.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
Looks no difference when testing this on QEMU 64-bit. Am I missing anything?
Regards, Bin
Hi Bin,
On 11 April 2017 at 21:09, Bin Meng bmeng.cn@gmail.com wrote:
Hi Simon,
On Mon, Mar 20, 2017 at 2:59 AM, Simon Glass sjg@chromium.org wrote:
At present on a cold reboot we must reset the CPU to get it to full speed. With 64-bit U-Boot this happens in SPL. At present we print the banner before doing this, the end result being that we print the banner twice. Print the banner a little later (after the CPU is ready) to avoid this.
Signed-off-by: Simon Glass sjg@chromium.org
arch/x86/lib/spl.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
Looks no difference when testing this on QEMU 64-bit. Am I missing anything?
I suspect that QEMU doesn't have this problem, but on chromebook_link we have to reset to change the CPU speed.
Regards, Simon
On Wed, Apr 12, 2017 at 11:29 AM, Simon Glass sjg@chromium.org wrote:
Hi Bin,
On 11 April 2017 at 21:09, Bin Meng bmeng.cn@gmail.com wrote:
Hi Simon,
On Mon, Mar 20, 2017 at 2:59 AM, Simon Glass sjg@chromium.org wrote:
At present on a cold reboot we must reset the CPU to get it to full speed. With 64-bit U-Boot this happens in SPL. At present we print the banner before doing this, the end result being that we print the banner twice. Print the banner a little later (after the CPU is ready) to avoid this.
Signed-off-by: Simon Glass sjg@chromium.org
arch/x86/lib/spl.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
Looks no difference when testing this on QEMU 64-bit. Am I missing anything?
I suspect that QEMU doesn't have this problem, but on chromebook_link we have to reset to change the CPU speed.
That makes sense.
Reviewed-by: Bin Meng bmeng.cn@gmail.com Tested on QEMU Tested-by: Bin Meng bmeng.cn@gmail.com
On Tue, Apr 18, 2017 at 3:30 PM, Bin Meng bmeng.cn@gmail.com wrote:
On Wed, Apr 12, 2017 at 11:29 AM, Simon Glass sjg@chromium.org wrote:
Hi Bin,
On 11 April 2017 at 21:09, Bin Meng bmeng.cn@gmail.com wrote:
Hi Simon,
On Mon, Mar 20, 2017 at 2:59 AM, Simon Glass sjg@chromium.org wrote:
At present on a cold reboot we must reset the CPU to get it to full speed. With 64-bit U-Boot this happens in SPL. At present we print the banner before doing this, the end result being that we print the banner twice. Print the banner a little later (after the CPU is ready) to avoid this.
Signed-off-by: Simon Glass sjg@chromium.org
arch/x86/lib/spl.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
Looks no difference when testing this on QEMU 64-bit. Am I missing anything?
I suspect that QEMU doesn't have this problem, but on chromebook_link we have to reset to change the CPU speed.
That makes sense.
Reviewed-by: Bin Meng bmeng.cn@gmail.com Tested on QEMU Tested-by: Bin Meng bmeng.cn@gmail.com
applied to u-boot-x86, thanks!
Enable this command so we can get an approximate performance measurement.
Signed-off-by: Simon Glass sjg@chromium.org ---
configs/chromebook_link64_defconfig | 1 + configs/chromebook_link_defconfig | 1 + 2 files changed, 2 insertions(+)
diff --git a/configs/chromebook_link64_defconfig b/configs/chromebook_link64_defconfig index 3ab34cd72b..749cfd43b0 100644 --- a/configs/chromebook_link64_defconfig +++ b/configs/chromebook_link64_defconfig @@ -86,4 +86,5 @@ CONFIG_FRAMEBUFFER_VESA_MODE_11A=y CONFIG_VIDEO_IVYBRIDGE_IGD=y CONFIG_CONSOLE_SCROLL_LINES=5 CONFIG_USE_PRIVATE_LIBGCC=y +CONFIG_CMD_DHRYSTONE=y CONFIG_TPM=y diff --git a/configs/chromebook_link_defconfig b/configs/chromebook_link_defconfig index 85b7d5fcd9..5ebb556f90 100644 --- a/configs/chromebook_link_defconfig +++ b/configs/chromebook_link_defconfig @@ -69,4 +69,5 @@ CONFIG_FRAMEBUFFER_VESA_MODE_11A=y CONFIG_VIDEO_IVYBRIDGE_IGD=y CONFIG_CONSOLE_SCROLL_LINES=5 CONFIG_USE_PRIVATE_LIBGCC=y +CONFIG_CMD_DHRYSTONE=y CONFIG_TPM=y
On Mon, Mar 20, 2017 at 2:59 AM, Simon Glass sjg@chromium.org wrote:
Enable this command so we can get an approximate performance measurement.
Signed-off-by: Simon Glass sjg@chromium.org
configs/chromebook_link64_defconfig | 1 + configs/chromebook_link_defconfig | 1 + 2 files changed, 2 insertions(+)
Reviewed-by: Bin Meng bmeng.cn@gmail.com
On Wed, Apr 12, 2017 at 11:09 AM, Bin Meng bmeng.cn@gmail.com wrote:
On Mon, Mar 20, 2017 at 2:59 AM, Simon Glass sjg@chromium.org wrote:
Enable this command so we can get an approximate performance measurement.
Signed-off-by: Simon Glass sjg@chromium.org
configs/chromebook_link64_defconfig | 1 + configs/chromebook_link_defconfig | 1 + 2 files changed, 2 insertions(+)
Reviewed-by: Bin Meng bmeng.cn@gmail.com
applied to u-boot-x86, thanks!
Add a uclass to handle board init. This allows drivers to be provided to perform the various phases of init. Functions are provided to call all devices that can handle a particular phase.
Signed-off-by: Simon Glass sjg@chromium.org ---
common/Kconfig | 31 +++++++ common/init/Makefile | 1 + common/init/board-uclass.c | 108 ++++++++++++++++++++++++ include/asm-generic/global_data.h | 5 ++ include/board.h | 170 ++++++++++++++++++++++++++++++++++++++ include/dm/uclass-id.h | 1 + 6 files changed, 316 insertions(+) create mode 100644 common/init/board-uclass.c create mode 100644 include/board.h
diff --git a/common/Kconfig b/common/Kconfig index 8f73c8f757..7d2bd15371 100644 --- a/common/Kconfig +++ b/common/Kconfig @@ -395,6 +395,36 @@ config DISPLAY_BOARDINFO
menu "Start-up hooks"
+config BOARD + bool "Support using driver model for board start-up hooks, etc." + help + This adds support for the board uclass and associated functions. With + this you can create and use BOARD drivers. However unless + BOARD_ENABLE is also set, the existing init sequence will continue + to be used. This is designed to ease migration of boards, since + support for both methods can be provided during the transition + period. + +config SPL_BOARD + bool "Support using driver model for board start-up hooks, etc. in SPL" + help + This adds support for the board uclass and associated functions in + SPL. With this you can create and use BOARD drivers. However unless + BOARD_ENABLE is also set, the existing init sequence will continue + to be used. This is designed to ease migration of boards, since + support for both methods can be provided during the transition + period. + +config BOARD_ENABLE + bool "Use driver model instead of ad-hoc board init functions" + depends on BOARD + help + This replaces the ad-hoc start-up functions like board_early_init_f() + with a driver-model-based interface. With this enabled, boards + provide one or more drivers which implement these phases of init as + well as access to the board decription. Existing init functions are + no-longer called. + config ARCH_EARLY_INIT_R bool "Call arch-specific init soon after relocation" default y if X86 @@ -414,6 +444,7 @@ config ARCH_MISC_INIT
config BOARD_EARLY_INIT_F bool "Call board-specific init before relocation" + depends on !BOARD_ENABLE default y if X86 help Some boards need to perform initialisation as soon as possible diff --git a/common/init/Makefile b/common/init/Makefile index 4902635f53..923ce8e139 100644 --- a/common/init/Makefile +++ b/common/init/Makefile @@ -5,3 +5,4 @@ #
obj-y += board_init.o +obj-$(CONFIG_$(SPL_)BOARD) += board-uclass.o diff --git a/common/init/board-uclass.c b/common/init/board-uclass.c new file mode 100644 index 0000000000..2ca65f44da --- /dev/null +++ b/common/init/board-uclass.c @@ -0,0 +1,108 @@ +/* + * Board driver interface + * + * Copyright (c) 2017 Google, Inc + * Written by Simon Glass sjg@chromium.org + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +#include <common.h> +#include <board.h> + +DECLARE_GLOBAL_DATA_PTR; + +int board_phase(struct udevice *dev, enum board_phase_t phase) +{ + struct board_uc_priv *priv = dev_get_uclass_priv(dev); + struct board_ops *ops = board_get_ops(dev); + + if (!ops->phase) + return -ENOSYS; + if (!priv->phase_mask && phase == BOARD_PHASE_FIRST) { + printf("Device '%s' supports no phases\n", dev->name); + return -EINVAL; + } + if (!(priv->phase_mask & board_phase_mask(phase))) + return -ENOSYS; + + return ops->phase(dev, phase); +} + +int board_walk_phase_count(enum board_phase_t phase, bool verbose) +{ + struct udevice *dev; + int count = 0; + int ret; + + for (ret = uclass_first_device(UCLASS_BOARD, &dev); + dev; + uclass_next_device(&dev)) { + ret = board_phase(dev, phase); + if (ret == 0) { + count++; + } else if (ret == BOARD_PHASE_CLAIMED) { + count++; + debug("Phase %d claimed by '%s'\n", phase, dev->name); + break; + } else if (ret != -ENOSYS) { + gd->phase_count[phase] += count; + return ret; + } + } + + if (!count) { + if (verbose) + printf("Unable to find driver for phase %d\n", phase); + return -ENOSYS; + } + gd->phase_count[phase] += count; + + return count; +} + +int board_walk_phase(enum board_phase_t phase) +{ + int ret; + + ret = board_walk_phase_count(phase, true); + if (ret < 0) + return ret; + + return 0; +} + +int board_walk_opt_phase(enum board_phase_t phase) +{ + int ret; + + ret = board_walk_phase_count(phase, false); + if (ret < 0 && ret != -ENOSYS) + return ret; + + return 0; +} + +int board_support_phase(struct udevice *dev, enum board_phase_t phase) +{ + struct board_uc_priv *priv = dev_get_uclass_priv(dev); + + priv->phase_mask |= board_phase_mask(phase); + + return 0; +} + +int board_support_phase_mask(struct udevice *dev, ulong phase_mask) +{ + struct board_uc_priv *priv = dev_get_uclass_priv(dev); + + priv->phase_mask = phase_mask; + + return 0; +} + +UCLASS_DRIVER(board) = { + .id = UCLASS_BOARD, + .name = "board", + .per_device_auto_alloc_size = sizeof(struct board_uc_priv), +}; diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h index 5b356dd231..fea1e916ed 100644 --- a/include/asm-generic/global_data.h +++ b/include/asm-generic/global_data.h @@ -21,6 +21,7 @@ */
#ifndef __ASSEMBLY__ +#include <board.h> #include <membuff.h> #include <linux/list.h>
@@ -107,6 +108,10 @@ typedef struct global_data { ulong video_top; /* Top of video frame buffer area */ ulong video_bottom; /* Bottom of video frame buffer area */ #endif +#ifdef CONFIG_BOARD + /* number of drivers which handled each phase */ + uint8_t phase_count[BOARD_PHASE_COUNT]; +#endif } gd_t; #endif
diff --git a/include/board.h b/include/board.h new file mode 100644 index 0000000000..0975f5ac12 --- /dev/null +++ b/include/board.h @@ -0,0 +1,170 @@ +/* + * Board driver interface + * + * Copyright (c) 2017 Google, Inc + * Written by Simon Glass sjg@chromium.org + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +#ifndef __BOARD_H +#define __BOARD_H + +/* Include dm.h here because board.h comes before dm.h in include order */ +#include <dm.h> + +/* + * This file provides access to board drivers, which are responsible for + * initing the board as well as (in future) querying its state. + */ + +/* Phases of init - please update phase_name also */ +enum board_phase_t { + /* + * Pre-relocation phases. + * TODO(sjg@chromium.org): At present these are named the same as the + * functions they replace to avoid confusion. However this naming is + * not very consistent. At some point once enough boards uses this + * interface, we could rename some of these. + * + * TODO(sjg@chromium.org): arch_cpu_init() and mach_cpu_init() are + * called before driver model is ready so we cannot yet add them to + * this interface. Hopefully this can be adjusted later: + * BOARD_F_ARCH_CPU_INIT, + * BOARD_F_MACH_CPU_INIT, + */ + BOARD_PHASE_FIRST, + BOARD_F_ARCH_CPU_INIT_DM = BOARD_PHASE_FIRST, + BOARD_F_EARLY_INIT_F, + BOARD_F_CHECKCPU, + BOARD_F_MISC_INIT_F, + BOARD_F_DRAM_INIT, + BOARD_F_RESERVE_ARCH, + + /* + * Post-relocation phases go here: + * + * BOARD_R_... + */ + + BOARD_PHASE_TEST, /* For sandbox testing */ + BOARD_PHASE_COUNT, + BOARD_PHASE_INVALID, /* For sandbox testing */ +}; + +static inline ulong board_phase_mask(enum board_phase_t phase) +{ + return 1UL << (ulong)phase; +} + +/* + * Return this from phase() to indicate that no more devices should handle this + * phase + */ +#define BOARD_PHASE_CLAIMED EUSERS + +/* Operations for the board driver */ +struct board_ops { + /** + * phase() - Execute a phase of board init + * + * @dev: Device to use + * @phase: Phase to execute + * @return 0 if done, -ENOSYS if not supported (which is often fine), + BOARD_PHASE_CLAIMED if this was handled and that processing + of this phase should stop (i.e. do not send it to other + devices), other error if something went wrong + */ + int (*phase)(struct udevice *dev, enum board_phase_t phase); + + /** + * get_desc() - Get a description string for a board + * + * @dev: Device to check (UCLASS_BOARD) + * @buf: Buffer to place string + * @size: Size of string space + * @return 0 if OK, -ENOSPC if buffer is too small, other -ve on error + */ + int (*get_desc)(struct udevice *dev, char *buf, int size); +}; + +#define board_get_ops(dev) ((struct board_ops *)(dev)->driver->ops) + +/* Private uclass information about each device */ +struct board_uc_priv { + ulong phase_mask; /* Mask of phases supported by this device */ +}; + +/** + * board_phase() - Execute a phase of board init on a device + * + * @phase: Phase to execute + * @return 0 if done, -ENOSYS if not supported by this device, other error if + * something went wrong + */ +int board_phase(struct udevice *dev, enum board_phase_t phase); + +/** + * board_walk_phase() - Execute a phase of board init + * + * This works through the available board devices asking each one to perform + * the requested init phase. The process continues until there are no more + * board devices. + * + * If no board device provides the phase, this function returns -ENOSYS. + * + * @phase: Phase to execute + * @return 0 if done, -ENOSYS if not supported, other error if something went + * wrong + */ +int board_walk_phase(enum board_phase_t phase); + +/** + * board_opt_walk_phase() - Execute an optional phase of board init + * + * This works through the available board devices asking each one to perform + * the requested init phase. The process continues until there are no more + * board devices. + * + * If no board device provides the phase, this function returns 0. + * + * @phase: Phase to execute + * @return 0 if done, other error if something went wrong + */ +int board_walk_opt_phase(enum board_phase_t phase); + +/** + * board_walk_phase_count() - Execute an optional phase of board init + * + * This works through the available board devices asking each one to perform + * the requested init phase. The process continues until there are no more + * board devices. + * + * If no board provides the phase, this function returns -ENOSYS. + * + * @phase: Phase to execute + * @return number of devices which handled this phase if done, -ve error if + * something went wrong + */ +int board_walk_phase_count(enum board_phase_t phase, bool verbose); + +/** + * board_support_phase() - Mark a board device as supporting the given phase + * + * @dev: Board device + * @phase: Phase to execute + * @return 0 + */ +int board_support_phase(struct udevice *dev, enum board_phase_t phase); + +/** + * board_support_phase_mask() - Mark a board device as supporting given phases + * + * @dev: Board device + * @phase_mask: Mask of phases to execute, built up by ORing board_phase_mask() + * values together + * @return 0 + */ +int board_support_phase_mask(struct udevice *dev, ulong phase_mask); + +#endif diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h index 8c92d0b030..166194fead 100644 --- a/include/dm/uclass-id.h +++ b/include/dm/uclass-id.h @@ -27,6 +27,7 @@ enum uclass_id { /* U-Boot uclasses start here - in alphabetical order */ UCLASS_ADC, /* Analog-to-digital converter */ UCLASS_AHCI, /* SATA disk controller */ + UCLASS_BOARD, /* Board-specific driver */ UCLASS_BLK, /* Block device */ UCLASS_CLK, /* Clock source, e.g. used by peripherals */ UCLASS_CPU, /* CPU, typically part of an SoC */
On 03/19/17 20:59, Simon Glass wrote:
Add a uclass to handle board init. This allows drivers to be provided to perform the various phases of init. Functions are provided to call all devices that can handle a particular phase.
Signed-off-by: Simon Glass sjg@chromium.org
common/Kconfig | 31 +++++++ common/init/Makefile | 1 + common/init/board-uclass.c | 108 ++++++++++++++++++++++++ include/asm-generic/global_data.h | 5 ++ include/board.h | 170 ++++++++++++++++++++++++++++++++++++++ include/dm/uclass-id.h | 1 + 6 files changed, 316 insertions(+) create mode 100644 common/init/board-uclass.c create mode 100644 include/board.h
[...]
diff --git a/include/board.h b/include/board.h new file mode 100644 index 0000000000..0975f5ac12 --- /dev/null +++ b/include/board.h
[...]
+/* Operations for the board driver */ +struct board_ops {
- /**
- phase() - Execute a phase of board init
* @dev: Device to use
- @phase: Phase to execute
- @return 0 if done, -ENOSYS if not supported (which is often fine),
BOARD_PHASE_CLAIMED if this was handled and that processingof this phase should stop (i.e. do not send it to otherdevices), other error if something went wrong- */
- int (*phase)(struct udevice *dev, enum board_phase_t phase);
That looks a bit tiny interface. This will force all the boards to define switch to figure out what the current phase is... This might cause a problem (probably #ifdefs) in the board code as some code will be available in SPL and some not.
I would prefer a wider interface instead of a single entry point to any kind of flexibility to the board driver.
- /**
* get_desc() - Get a description string for a board** @dev: Device to check (UCLASS_BOARD)* @buf: Buffer to place string* @size: Size of string space* @return 0 if OK, -ENOSPC if buffer is too small, other -ve on error*/- int (*get_desc)(struct udevice *dev, char *buf, int size);
+};
+#define board_get_ops(dev) ((struct board_ops *)(dev)->driver->ops)
[...]
Hi Igor,
On 20 March 2017 at 01:54, Igor Grinberg grinberg@compulab.co.il wrote:
On 03/19/17 20:59, Simon Glass wrote:
Add a uclass to handle board init. This allows drivers to be provided to perform the various phases of init. Functions are provided to call all devices that can handle a particular phase.
Signed-off-by: Simon Glass sjg@chromium.org
common/Kconfig | 31 +++++++ common/init/Makefile | 1 + common/init/board-uclass.c | 108 ++++++++++++++++++++++++ include/asm-generic/global_data.h | 5 ++ include/board.h | 170 ++++++++++++++++++++++++++++++++++++++ include/dm/uclass-id.h | 1 + 6 files changed, 316 insertions(+) create mode 100644 common/init/board-uclass.c create mode 100644 include/board.h
[...]
diff --git a/include/board.h b/include/board.h new file mode 100644 index 0000000000..0975f5ac12 --- /dev/null +++ b/include/board.h
[...]
+/* Operations for the board driver */ +struct board_ops {
/*** phase() - Execute a phase of board init** @dev: Device to use* @phase: Phase to execute* @return 0 if done, -ENOSYS if not supported (which is often fine),BOARD_PHASE_CLAIMED if this was handled and that processingof this phase should stop (i.e. do not send it to otherdevices), other error if something went wrong*/int (*phase)(struct udevice *dev, enum board_phase_t phase);That looks a bit tiny interface. This will force all the boards to define switch to figure out what the current phase is... This might cause a problem (probably #ifdefs) in the board code as some code will be available in SPL and some not.
I would prefer a wider interface instead of a single entry point to any kind of flexibility to the board driver.
Yes that certainly seems nicer.
But it means we might have ~20 or so methods in the interface. Do you think most of them would be used? Also I am thinking a linker list idea might help with code size, and would need some sort of 'phase' parameter I think.
/*** get_desc() - Get a description string for a board** @dev: Device to check (UCLASS_BOARD)* @buf: Buffer to place string* @size: Size of string space* @return 0 if OK, -ENOSPC if buffer is too small, other -ve on error*/int (*get_desc)(struct udevice *dev, char *buf, int size);+};
+#define board_get_ops(dev) ((struct board_ops *)(dev)->driver->ops)
[...]
-- Regards, Igor.
Regards, Simon
Add a 'board phases' command ('boa' or 'boa p' for short) which shows which board phases have been run and how many devices provided an implementation of each phase. Sample output:
=> board phases 1 arch_cpu_init_dm 0 board_early_init_f 1 checkcpu 0 misc_init_f 1 dram_init 1 reserve_arch
Signed-off-by: Simon Glass sjg@chromium.org ---
cmd/Kconfig | 8 ++++++++ cmd/Makefile | 1 + cmd/board.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 67 insertions(+) create mode 100644 cmd/board.c
diff --git a/cmd/Kconfig b/cmd/Kconfig index 25e3b783a8..9b93f213b7 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -126,6 +126,14 @@ config CMD_BDI help Print board info
+config CMD_BOARD + bool "board" + default y if BOARD + help + Provides access to board-specific drivers. This includes information + about which board init was completed as well as the board + description. + config CMD_CONFIG bool "config" select BUILD_BIN2C diff --git a/cmd/Makefile b/cmd/Makefile index f13bb8c11e..4b1d183514 100644 --- a/cmd/Makefile +++ b/cmd/Makefile @@ -16,6 +16,7 @@ obj-y += version.o obj-$(CONFIG_CMD_AES) += aes.o obj-$(CONFIG_CMD_AMBAPP) += ambapp.o obj-$(CONFIG_CMD_ARMFLASH) += armflash.o +obj-$(CONFIG_CMD_BOARD) += board.o obj-$(CONFIG_SOURCE) += source.o obj-$(CONFIG_CMD_SOURCE) += source.o obj-$(CONFIG_CMD_BDI) += bdinfo.o diff --git a/cmd/board.c b/cmd/board.c new file mode 100644 index 0000000000..3ee91ca25d --- /dev/null +++ b/cmd/board.c @@ -0,0 +1,58 @@ +/* + * Board driver commands + * + * Copyright (c) 2017 Google, Inc + * Written by Simon Glass sjg@chromium.org + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +#include <common.h> +#include <board.h> +#include <command.h> + +DECLARE_GLOBAL_DATA_PTR; + +static const char * const phase_name[] = { + [BOARD_F_ARCH_CPU_INIT_DM] = "arch_cpu_init_dm", + [BOARD_F_EARLY_INIT_F] = "board_early_init_f", + [BOARD_F_CHECKCPU] = "checkcpu", + [BOARD_F_MISC_INIT_F] = "misc_init_f", + [BOARD_F_DRAM_INIT] = "dram_init", + [BOARD_F_RESERVE_ARCH] = "reserve_arch", + [BOARD_PHASE_TEST] = "test", + [BOARD_PHASE_INVALID] = "invalid", + +}; + +static void board_list_phases(void) +{ + enum board_phase_t phase; + + for (phase = BOARD_PHASE_FIRST; phase < BOARD_PHASE_TEST; phase++) + printf("%3d %s\n", gd->phase_count[phase], phase_name[phase]); +} + +static int do_board(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) +{ + int cmd = 'p'; + + if (argc > 1) + cmd = argv[1][0]; + + switch (cmd) { + case 'p': /* phases */ + board_list_phases(); + break; + default: + return CMD_RET_FAILURE; + } + + return 0; +} + +U_BOOT_CMD( + board, 2, 0, do_board, + "Access board information", + "phases\t- Show information about completed board init phases" +);
When CONFIG_BOARD_ENABLED is enabled, replace the existing ad-hoc hooks with ones based on driver model. These call devices to handle each phase of init.
Signed-off-by: Simon Glass sjg@chromium.org ---
common/board_f.c | 47 ++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 42 insertions(+), 5 deletions(-)
diff --git a/common/board_f.c b/common/board_f.c index 7d1ede0404..df9a64a20f 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -11,7 +11,8 @@ */
#include <common.h> -#include <linux/compiler.h> +#include <board.h> +#include <cpu.h> #include <version.h> #include <console.h> #include <environment.h> @@ -819,6 +820,40 @@ static int initf_dm(void) return 0; }
+#ifdef CONFIG_BOARD_ENABLE + +int arch_cpu_init_dm(void) +{ + return board_walk_opt_phase(BOARD_F_ARCH_CPU_INIT_DM); +} + +int board_early_init_f(void) +{ + return board_walk_opt_phase(BOARD_F_EARLY_INIT_F); +} + +int checkcpu(void) +{ + return board_walk_opt_phase(BOARD_F_CHECKCPU); +} + +int misc_init_f(void) +{ + return board_walk_opt_phase(BOARD_F_MISC_INIT_F); +} + +int dram_init(void) +{ + return board_walk_phase(BOARD_F_DRAM_INIT); +} + +int reserve_arch(void) +{ + return board_walk_opt_phase(BOARD_F_RESERVE_ARCH); +} + +#else + /* Architecture-specific memory reservation */ __weak int reserve_arch(void) { @@ -829,6 +864,7 @@ __weak int arch_cpu_init_dm(void) { return 0; } +#endif /* !CONFIG_BOARD_ENABLE_ENABLED */
static const init_fnc_t init_sequence_f[] = { #ifdef CONFIG_SANDBOX @@ -851,7 +887,7 @@ static const init_fnc_t init_sequence_f[] = { initf_dm, arch_cpu_init_dm, mark_bootstage, /* need timer, go after init dm */ -#if defined(CONFIG_BOARD_EARLY_INIT_F) +#if defined(CONFIG_BOARD_EARLY_INIT_F) || defined(CONFIG_BOARD_ENABLE) board_early_init_f, #endif /* TODO: can any of this go into arch_cpu_init()? */ @@ -898,7 +934,8 @@ static const init_fnc_t init_sequence_f[] = { #if defined(CONFIG_MPC83xx) prt_83xx_rsr, #endif -#if defined(CONFIG_PPC) || defined(CONFIG_M68K) || defined(CONFIG_SH) +#if defined(CONFIG_PPC) || defined(CONFIG_M68K) || defined(CONFIG_SH) || \ + defined(CONFIG_BOARD_ENABLE) checkcpu, #endif #if defined(CONFIG_DISPLAY_CPUINFO) @@ -908,7 +945,7 @@ static const init_fnc_t init_sequence_f[] = { show_board_info, #endif INIT_FUNC_WATCHDOG_INIT -#if defined(CONFIG_MISC_INIT_F) +#if defined(CONFIG_MISC_INIT_F) || defined(CONFIG_BOARD_ENABLE) misc_init_f, #endif INIT_FUNC_WATCHDOG_RESET @@ -922,7 +959,7 @@ static const init_fnc_t init_sequence_f[] = { /* TODO: unify all these dram functions? */ #if defined(CONFIG_ARM) || defined(CONFIG_X86) || defined(CONFIG_NDS32) || \ defined(CONFIG_MICROBLAZE) || defined(CONFIG_AVR32) || \ - defined(CONFIG_SH) + defined(CONFIG_SH) || defined(CONFIG_SANDBOX) dram_init, /* configure available RAM banks */ #endif #if defined(CONFIG_MIPS) || defined(CONFIG_PPC) || defined(CONFIG_M68K)
With driver model we can obtain CPU information by calling the CPU uclass. This avoids ad-hoc code for each board. Change the code to do this when CONFIG_BOARD is enabled.
Signed-off-by: Simon Glass sjg@chromium.org ---
common/board_f.c | 11 ++++++++++- drivers/cpu/cpu-uclass.c | 19 +++++++++++++++++++ include/cpu.h | 5 +++++ 3 files changed, 34 insertions(+), 1 deletion(-)
diff --git a/common/board_f.c b/common/board_f.c index df9a64a20f..4d9d2f30c7 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -852,6 +852,15 @@ int reserve_arch(void) return board_walk_opt_phase(BOARD_F_RESERVE_ARCH); }
+int print_cpuinfo(void) +{ +#ifdef CONFIG_CPU + return cpu_print_info(); +#else + return 0; +#endif +} + #else
/* Architecture-specific memory reservation */ @@ -938,7 +947,7 @@ static const init_fnc_t init_sequence_f[] = { defined(CONFIG_BOARD_ENABLE) checkcpu, #endif -#if defined(CONFIG_DISPLAY_CPUINFO) +#if defined(CONFIG_DISPLAY_CPUINFO) || defined(CONFIG_BOARD_ENABLED) print_cpuinfo, /* display cpu info (and speed) */ #endif #if defined(CONFIG_DISPLAY_BOARDINFO) diff --git a/drivers/cpu/cpu-uclass.c b/drivers/cpu/cpu-uclass.c index c57ac16b3a..2a69ea3af3 100644 --- a/drivers/cpu/cpu-uclass.c +++ b/drivers/cpu/cpu-uclass.c @@ -54,6 +54,25 @@ int cpu_get_vendor(struct udevice *dev, char *buf, int size) return ops->get_vendor(dev, buf, size); }
+int cpu_print_info(void) +{ + struct udevice *dev; + char name[60]; + int ret; + + ret = uclass_first_device(UCLASS_CPU, &dev); + if (ret) + return ret; + if (!dev) + return 0; + ret = cpu_get_desc(dev, name, sizeof(name)); + if (ret) + return ret; + printf("CPU: %s\n", name); + + return 0; +} + U_BOOT_DRIVER(cpu_bus) = { .name = "cpu_bus", .id = UCLASS_SIMPLE_BUS, diff --git a/include/cpu.h b/include/cpu.h index 954257715a..2d5c373343 100644 --- a/include/cpu.h +++ b/include/cpu.h @@ -124,4 +124,9 @@ int cpu_get_count(struct udevice *dev); */ int cpu_get_vendor(struct udevice *dev, char *buf, int size);
+/** + * cpu_print_info() - Print information about the first CPU + */ +int cpu_print_info(void); + #endif
Adjust the existing hooks to use a driver instead.
Signed-off-by: Simon Glass sjg@chromium.org ---
arch/Kconfig | 3 +++ board/sandbox/sandbox.c | 44 ++++++++++++++++++++++++++++++++++++++------ 2 files changed, 41 insertions(+), 6 deletions(-)
diff --git a/arch/Kconfig b/arch/Kconfig index 76c690f667..f07070db18 100644 --- a/arch/Kconfig +++ b/arch/Kconfig @@ -64,6 +64,9 @@ config SANDBOX bool "Sandbox" select BOARD_LATE_INIT select SUPPORT_OF_CONTROL + select BOARD + select SPL_BOARD + select BOARD_ENABLE select DM select DM_KEYBOARD select DM_SPI_FLASH diff --git a/board/sandbox/sandbox.c b/board/sandbox/sandbox.c index b41e9decb3..1eea20cbcb 100644 --- a/board/sandbox/sandbox.c +++ b/board/sandbox/sandbox.c @@ -7,6 +7,7 @@ #include <cros_ec.h> #include <dm.h> #include <os.h> +#include <asm/state.h> #include <asm/test.h> #include <asm/u-boot-sandbox.h>
@@ -41,12 +42,6 @@ unsigned long timer_read_counter(void) } #endif
-int dram_init(void) -{ - gd->ram_size = CONFIG_SYS_SDRAM_SIZE; - return 0; -} - #ifdef CONFIG_BOARD_LATE_INIT int board_late_init(void) { @@ -63,3 +58,40 @@ int board_late_init(void) return 0; } #endif + +static int sandbox_phase(struct udevice *dev, enum board_phase_t phase) +{ + struct sandbox_state *state = state_get_current(); + + switch (phase) { + case BOARD_F_DRAM_INIT: + gd->ram_size = state->ram_size; + return 0; + default: + return -ENOSYS; + } + + return 0; +} + +static int sandbox_board_probe(struct udevice *dev) +{ + return board_support_phase(dev, BOARD_F_DRAM_INIT); +} + +static const struct board_ops sandbox_board_ops = { + .phase = sandbox_phase, +}; + +/* Name this starting with underscore so it will be called last */ +U_BOOT_DRIVER(_sandbox_board_drv) = { + .name = "sandbox_board", + .id = UCLASS_BOARD, + .ops = &sandbox_board_ops, + .probe = sandbox_board_probe, + .flags = DM_FLAG_PRE_RELOC, +}; + +U_BOOT_DEVICE(sandbox_board) = { + .name = "sandbox_board", +};
Add some tests which define some devices and check the operation of the various init functions.
Signed-off-by: Simon Glass sjg@chromium.org ---
arch/sandbox/dts/test.dts | 12 +++++++ arch/sandbox/include/asm/state.h | 3 ++ arch/sandbox/include/asm/test.h | 9 +++++ drivers/misc/Makefile | 1 + drivers/misc/board_sandbox.c | 44 ++++++++++++++++++++++++ test/dm/Makefile | 1 + test/dm/board.c | 74 ++++++++++++++++++++++++++++++++++++++++ 7 files changed, 144 insertions(+) create mode 100644 drivers/misc/board_sandbox.c create mode 100644 test/dm/board.c
diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts index fff175d1b7..084c3dff63 100644 --- a/arch/sandbox/dts/test.dts +++ b/arch/sandbox/dts/test.dts @@ -52,6 +52,18 @@ reg = <2 1>; };
+ board-test0 { + compatible = "sandbox,board-test0"; + }; + + board-test1 { + compatible = "sandbox,board-test1"; + }; + + board-test2 { + compatible = "sandbox,board-test2"; + }; + b-test { reg = <3 1>; compatible = "denx,u-boot-fdt-test"; diff --git a/arch/sandbox/include/asm/state.h b/arch/sandbox/include/asm/state.h index 149f28d873..d531531d72 100644 --- a/arch/sandbox/include/asm/state.h +++ b/arch/sandbox/include/asm/state.h @@ -9,6 +9,7 @@ #include <config.h> #include <sysreset.h> #include <stdbool.h> +#include <asm/test.h> #include <linux/stringify.h>
/** @@ -65,6 +66,8 @@ struct sandbox_state { enum state_terminal_raw term_raw; /* Terminal raw/cooked */ bool skip_delays; /* Ignore any time delays (for test) */ bool show_test_output; /* Don't suppress stdout in tests */ + /* Return values for board_sandbox */ + int board_sandbox_ret[BOARD_TEST_COUNT];
/* Pointer to information for each SPI bus/cs */ struct sandbox_spi_info spi[CONFIG_SANDBOX_SPI_MAX_BUS] diff --git a/arch/sandbox/include/asm/test.h b/arch/sandbox/include/asm/test.h index 451a78e590..12b3b9bc1e 100644 --- a/arch/sandbox/include/asm/test.h +++ b/arch/sandbox/include/asm/test.h @@ -79,4 +79,13 @@ long sandbox_i2c_rtc_get_set_base_time(struct udevice *dev, long base_time);
int sandbox_usb_keyb_add_string(struct udevice *dev, const char *str);
+/* For testing the BOARD uclass */ +enum { + BOARD_TEST0, + BOARD_TEST1, + BOARD_TEST2, + + BOARD_TEST_COUNT, +}; + #endif diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile index e3151ea097..88015cc8af 100644 --- a/drivers/misc/Makefile +++ b/drivers/misc/Makefile @@ -8,6 +8,7 @@ obj-$(CONFIG_MISC) += misc-uclass.o obj-$(CONFIG_ALI152X) += ali512x.o obj-$(CONFIG_ALTERA_SYSID) += altera_sysid.o +obj-$(CONFIG_SANDBOX) += board_sandbox.o obj-$(CONFIG_DS4510) += ds4510.o obj-$(CONFIG_CBMEM_CONSOLE) += cbmem_console.o ifndef CONFIG_SPL_BUILD diff --git a/drivers/misc/board_sandbox.c b/drivers/misc/board_sandbox.c new file mode 100644 index 0000000000..9ccdbfbbe8 --- /dev/null +++ b/drivers/misc/board_sandbox.c @@ -0,0 +1,44 @@ +/* + * Copyright (c) 2017 Google, Inc + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +#include <common.h> +#include <board.h> +#include <dm.h> +#include <asm/state.h> +#include <asm/test.h> + +static int board_sandbox_phase(struct udevice *dev, enum board_phase_t phase) +{ + struct sandbox_state *state = state_get_current(); + int id = dev_get_driver_data(dev); + + return state->board_sandbox_ret[id]; +} + +static int board_sandbox_probe(struct udevice *dev) +{ + return board_support_phase(dev, BOARD_PHASE_TEST); +} + +static const struct board_ops board_sandbox_ops = { + .phase = board_sandbox_phase, +}; + + +static const struct udevice_id board_sandbox_ids[] = { + { .compatible = "sandbox,board-test0", BOARD_TEST0 }, + { .compatible = "sandbox,board-test1", BOARD_TEST1 }, + { .compatible = "sandbox,board-test2", BOARD_TEST2 }, + { } +}; + +U_BOOT_DRIVER(board_sandbox_drv) = { + .name = "board_sandbox", + .id = UCLASS_BOARD, + .ops = &board_sandbox_ops, + .of_match = board_sandbox_ids, + .probe = board_sandbox_probe, +}; diff --git a/test/dm/Makefile b/test/dm/Makefile index 1885e17c38..c84a966708 100644 --- a/test/dm/Makefile +++ b/test/dm/Makefile @@ -15,6 +15,7 @@ obj-$(CONFIG_UT_DM) += test-uclass.o # subsystem you must add sandbox tests here. obj-$(CONFIG_UT_DM) += core.o ifneq ($(CONFIG_SANDBOX),) +obj-$(CONFIG_BOARD) += board.o obj-$(CONFIG_BLK) += blk.o obj-$(CONFIG_CLK) += clk.o obj-$(CONFIG_DM_ETH) += eth.o diff --git a/test/dm/board.c b/test/dm/board.c new file mode 100644 index 0000000000..986746632b --- /dev/null +++ b/test/dm/board.c @@ -0,0 +1,74 @@ +/* + * Copyright (C) 2015 Google, Inc + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +#include <common.h> +#include <asm/state.h> +#include <asm/test.h> +#include <dm/test.h> +#include <test/ut.h> + +DECLARE_GLOBAL_DATA_PTR; + +/* Test invoking a board phase with three active devices */ +static int dm_test_board(struct unit_test_state *uts) +{ + struct sandbox_state *state = state_get_current(); + + /* We should start with a count of 0 for our test phase */ + ut_asserteq(0, gd->phase_count[BOARD_PHASE_TEST]); + + /* Check that we can detect there being no driver */ + ut_asserteq(-ENOSYS, board_walk_phase_count(BOARD_PHASE_INVALID, + false)); + ut_asserteq(0, board_walk_opt_phase(BOARD_PHASE_INVALID)); + ut_asserteq(-ENOSYS, board_walk_phase(BOARD_PHASE_INVALID)); + + /* If no devices respond, we should get no action */ + state->board_sandbox_ret[BOARD_TEST0] = -ENOSYS; + state->board_sandbox_ret[BOARD_TEST1] = -ENOSYS; + state->board_sandbox_ret[BOARD_TEST2] = -ENOSYS; + ut_asserteq(-ENOSYS, board_walk_phase_count(BOARD_PHASE_TEST, false)); + ut_asserteq(0, board_walk_opt_phase(BOARD_PHASE_TEST)); + ut_asserteq(0, gd->phase_count[BOARD_PHASE_TEST]); + + /* Enable the first device */ + state->board_sandbox_ret[BOARD_TEST0] = 0; + ut_asserteq(1, board_walk_phase_count(BOARD_PHASE_TEST, false)); + ut_asserteq(1, gd->phase_count[BOARD_PHASE_TEST]); + + /* Enable the second device too */ + state->board_sandbox_ret[BOARD_TEST1] = 0; + ut_asserteq(2, board_walk_phase_count(BOARD_PHASE_TEST, false)); + ut_asserteq(3, gd->phase_count[BOARD_PHASE_TEST]); + + /* Enable all three devices */ + state->board_sandbox_ret[BOARD_TEST2] = 0; + ut_asserteq(3, board_walk_phase_count(BOARD_PHASE_TEST, false)); + ut_asserteq(6, gd->phase_count[BOARD_PHASE_TEST]); + + /* + * Check that the first device can claim the phase and lock out the + * other devices. + */ + state->board_sandbox_ret[BOARD_TEST0] = BOARD_PHASE_CLAIMED; + ut_asserteq(1, board_walk_phase_count(BOARD_PHASE_TEST, false)); + ut_asserteq(0, board_walk_phase(BOARD_PHASE_TEST)); + ut_asserteq(0, board_walk_opt_phase(BOARD_PHASE_TEST)); + ut_asserteq(9, gd->phase_count[BOARD_PHASE_TEST]); + + /* Any error should be reported, but previous devices should still get + * to process the phase. + */ + state->board_sandbox_ret[BOARD_TEST0] = 0; + state->board_sandbox_ret[BOARD_TEST1] = -ENOENT; + ut_asserteq(-ENOENT, board_walk_phase_count(BOARD_PHASE_TEST, false)); + ut_asserteq(-ENOENT, board_walk_phase(BOARD_PHASE_TEST)); + ut_asserteq(-ENOENT, board_walk_opt_phase(BOARD_PHASE_TEST)); + ut_asserteq(12, gd->phase_count[BOARD_PHASE_TEST]); + + return 0; +} +DM_TEST(dm_test_board, DM_TESTF_SCAN_FDT);
Add some documentation for the new board init system. This describes how it works and how to migrate to it.
Signed-off-by: Simon Glass sjg@chromium.org ---
doc/driver-model/board-info.txt | 181 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 181 insertions(+) create mode 100644 doc/driver-model/board-info.txt
diff --git a/doc/driver-model/board-info.txt b/doc/driver-model/board-info.txt new file mode 100644 index 0000000000..86db096e28 --- /dev/null +++ b/doc/driver-model/board-info.txt @@ -0,0 +1,181 @@ +How to handle board init with Driver Model +========================================== + +Motivation +---------- + +At present U-Boot has a lot of ad-hoc init functions related to boards, for +board_early_init_f, board_misc_init_f() and dram_init(). + +There are used in different ways by different boards as useful hooks to +do the required init and sequence it correctly. Some functions are always +enabled but have a __weak default. Some are controlled by the existence +of a CONFIG. + +There are two main init sequences: board_init_f() (f for running from +read-only flash) which runs before relocation and board_init_r() (r for +relocated) which runs afterwards. + +One problem with the current sequence is that it has a lot of +arch-specific #ifdefs around various functions. There are also #ifdefs +for various features. With a driver-model based approach it should be +possible to remove these over time since the board-specific code can move into +drivers and does not need to be called from the init sequence. + + +Design +------ + +A new uclass (UCLASS_BOARD) is defined. The uclass has one important method: +phase(). This is called to handle a particular phase of board init. Phases are +defined by enum board_phase_t. For example, the existing board_early_init_f() +function is replaced by the BOARD_F_EARLY_INIT_F phase. + +When the init sequence wants to initiate the BOARD_F_EARLY_INIT_F phase it +calls the phase() method of all the devices that implement that phase. It +knows which ones implement it because they call board_support_phase() or +board_support_phase_mask() in their probe method to register which phases they +support. + +Multiple devices can implement the same phase. The init sequence calls these +devices one by one. It is also possible for a device to 'claim' a phase, such +that no further devices are called. The ordering of devices is as per the +usual driver-model approach: the same order as the device tree nodes, or +ordered by device name in the case of U_BOOT_DEVICE() declarations. + +With this approach a call to board_early_init_f() is replaced with a call to +board_walk_opt_phase() which handles the phase but does not complain if no +device handles it. For a mandatory phase, board_walk_phase() can be used. + +After starting up you can use the 'board phases' command to see what phases +were executed: + + => board phases + 1 arch_cpu_init_dm + 0 board_early_init_f + 1 checkcpu + 0 misc_init_f + 1 dram_init + 1 reserve_arch + +This shows that four phases were executed and each had a single device which +handled that phase. + + +Example +------- + +Below is an example with sandbox: + + +static int sandbox_phase(struct udevice *dev, enum board_phase_t phase) +{ + struct sandbox_state *state = state_get_current(); + + switch (phase) { + case BOARD_F_DRAM_INIT: + gd->ram_size = state->ram_size; + return 0; + default: + return -ENOSYS; + } + + return 0; +} + +static int sandbox_board_probe(struct udevice *dev) +{ + return board_support_phase(dev, BOARD_F_DRAM_INIT); +} + +static const struct board_ops sandbox_board_ops = { + .phase = sandbox_phase, +}; + +/* Name this starting with underscore so it will be called last */ +U_BOOT_DRIVER(_sandbox_board_drv) = { + .name = "sandbox_board", + .id = UCLASS_BOARD, + .ops = &sandbox_board_ops, + .probe = sandbox_board_probe, + .flags = DM_FLAG_PRE_RELOC, +}; + +U_BOOT_DEVICE(sandbox_board) = { + .name = "sandbox_board", +}; + + +This is a normal driver which supports the phase() method. The U_BOOT_DEVICE() +declaration instantiates the device. It supports only one phase: +BOARD_F_DRAM_INIT. + +If you look at common/board_f.c you will see how dram_init() uses this: + +int dram_init(void) +{ + return board_walk_phase(BOARD_F_DRAM_INIT); +} + +That call will end up calling: + + sandbox_phase(dev, BOARD_F_DRAM_INIT) + +There is quite a bit of boilerplate with the driver declaration. It would be +quite easy to replace most of it with a macro, like: + +U_BOOT_BOARD_DRV(_sandbox_board, _sandbox_board_drv); + +but for now this is not attempted, to keep everything in the open. + + +Porting suggestions +------------------- + +You may find that you have all of your init handlers in a single file in your +board directory. If so you can simply add a driver similar to the above that +handles all the phases you use, and make your phase() method call the existing +functions in that file. You should rename the functions to avoid confusion +with the legacy method names and make them static since they will not be called +from outside your file. + +If you have handlers in multiple files you can either: + +- Add a separate driver in each file +- Use a single driver and have it call out to functions in other files for some + of the phases. + +Probably the first approach is better. + +You can perform the work in three patches: + +- one to add the board drivers and support +- one to switch on CONFIG_BOARD_ENABLE +- one to remove the now-unused legacy code + +This allows you to bisect easily if you find problems later. + + +Required work +------------- + +Before this feature can be applied to mainline it must support all phases. If +we do not do thing up front then adding a new method may require porting all +boards over to use that method. There is only a single control on whether or +not to use this features (CONFIG_BOARD_ENABLE) so it is all or nothing. + +The post-relocation init phases therefore need to be added, and a review of +pre-relocation phases needs to be completed to find anything that is missing. + + +Future work +----------- + +Apart from the required work, once all boards are ported to this framework we +should be able to clean up the init sequences to remove all the #ifdefs. + + +-- +Simon Glass +sjg@chromium.org +20-Mar-17
Enable this option so we can use board drivers.
Signed-off-by: Simon Glass sjg@chromium.org ---
arch/Kconfig | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/arch/Kconfig b/arch/Kconfig index f07070db18..2133a88c99 100644 --- a/arch/Kconfig +++ b/arch/Kconfig @@ -89,6 +89,8 @@ config X86 select CREATE_ARCH_SYMLINK select HAVE_PRIVATE_LIBGCC select SUPPORT_OF_CONTROL + select BOARD + select SPL_BOARD if SPL select DM select DM_KEYBOARD select DM_SERIAL
Add support for reserving initial memory.
Signed-off-by: Simon Glass sjg@chromium.org ---
arch/x86/cpu/cpu.c | 41 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-)
diff --git a/arch/x86/cpu/cpu.c b/arch/x86/cpu/cpu.c index 8fa6953588..893bec5c5c 100644 --- a/arch/x86/cpu/cpu.c +++ b/arch/x86/cpu/cpu.c @@ -254,7 +254,7 @@ int cpu_init_r(void) }
#ifndef CONFIG_EFI_STUB -int reserve_arch(void) +static int cpu_x86_reserve_arch(void) { #ifdef CONFIG_ENABLE_MRC_CACHE mrccache_reserve(); @@ -266,4 +266,43 @@ int reserve_arch(void)
return 0; } + +#ifndef CONFIG_BOARD_ENABLE +int reserve_arch(void) +{ + return cpu_x86_reserve_arch(); +} +#endif + +#endif /* !CONFIG_EFI_STUB */ + +static int cpu_x86_phase(struct udevice *dev, enum board_phase_t phase) +{ +#ifndef CONFIG_EFI_STUB + return cpu_x86_reserve_arch(); +#else + return 0; #endif +} + +static int cpu_x86_board_probe(struct udevice *dev) +{ + return board_support_phase(dev, BOARD_F_RESERVE_ARCH); +} + +static const struct board_ops cpu_x86_board_ops = { + .phase = cpu_x86_phase, +}; + +/* Name this starting with underscore so it will be called last */ +U_BOOT_DRIVER(_cpu_x86_board_drv) = { + .name = "cpu_x86_board", + .id = UCLASS_BOARD, + .ops = &cpu_x86_board_ops, + .probe = cpu_x86_board_probe, + .flags = DM_FLAG_PRE_RELOC, +}; + +U_BOOT_DEVICE(cpu_x86_board) = { + .name = "cpu_x86_board", +};
Add hooks for ivybridge boards so they can use CONFIG_BOARD for init.
Signed-off-by: Simon Glass sjg@chromium.org ---
arch/x86/cpu/coreboot/coreboot.c | 2 + arch/x86/cpu/ivybridge/cpu.c | 71 +++++++++++++++++++---- arch/x86/cpu/ivybridge/sdram.c | 13 ++++- arch/x86/cpu/ivybridge/sdram_nop.c | 43 +++++++++++++- arch/x86/cpu/x86_64/cpu.c | 2 + arch/x86/include/asm/arch-ivybridge/sandybridge.h | 7 +++ arch/x86/lib/spl.c | 13 +++++ board/google/chromebook_link/link.c | 2 + board/google/chromebox_panther/panther.c | 2 + 9 files changed, 140 insertions(+), 15 deletions(-)
diff --git a/arch/x86/cpu/coreboot/coreboot.c b/arch/x86/cpu/coreboot/coreboot.c index 1b042037bb..609bfcb2da 100644 --- a/arch/x86/cpu/coreboot/coreboot.c +++ b/arch/x86/cpu/coreboot/coreboot.c @@ -29,6 +29,7 @@ int arch_cpu_init(void) return x86_cpu_init_f(); }
+#ifndef CONFIG_BOARD_ENABLE int board_early_init_f(void) { return 0; @@ -38,6 +39,7 @@ int print_cpuinfo(void) { return default_print_cpuinfo(); } +#endif
static void board_final_cleanup(void) { diff --git a/arch/x86/cpu/ivybridge/cpu.c b/arch/x86/cpu/ivybridge/cpu.c index c4aca08f0d..96e69ef792 100644 --- a/arch/x86/cpu/ivybridge/cpu.c +++ b/arch/x86/cpu/ivybridge/cpu.c @@ -12,6 +12,7 @@ */
#include <common.h> +#include <board.h> #include <dm.h> #include <errno.h> #include <fdtdec.h> @@ -50,10 +51,10 @@ int arch_cpu_init(void) return x86_cpu_init_f(); }
-int arch_cpu_init_dm(void) +static int do_arch_cpu_init_dm(void) { struct pci_controller *hose; - struct udevice *bus, *dev; + struct udevice *bus, *lpc_dev; int ret;
post_code(0x70); @@ -67,7 +68,7 @@ int arch_cpu_init_dm(void) /* TODO(sjg@chromium.org): Get rid of gd->hose */ gd->hose = hose;
- ret = uclass_first_device_err(UCLASS_LPC, &dev); + ret = uclass_first_device_err(UCLASS_LPC, &lpc_dev); if (ret) return ret;
@@ -83,6 +84,13 @@ int arch_cpu_init_dm(void) return 0; }
+#ifndef CONFIG_BOARD_ENABLE +int arch_cpu_init_dm(void) +{ + return do_arch_cpu_init_dm(); +} +#endif + #define PCH_EHCI0_TEMP_BAR0 0xe8000000 #define PCH_EHCI1_TEMP_BAR0 0xe8000400 #define PCH_XHCI_TEMP_BAR0 0xe8001000 @@ -125,12 +133,10 @@ static void enable_usb_bar(struct udevice *bus) pci_bus_write_config(bus, usb3, PCI_COMMAND, cmd, PCI_SIZE_32); }
-int print_cpuinfo(void) +static int ivybridge_checkcpu(void) { enum pei_boot_mode_t boot_mode = PEI_BOOT_NONE; - char processor_name[CPU_MAX_NAME_LEN]; struct udevice *dev, *lpc; - const char *name; uint32_t pm1_cnt; uint16_t pm1_sts; int ret; @@ -181,19 +187,62 @@ int print_cpuinfo(void) }
gd->arch.pei_boot_mode = boot_mode; - - /* Print processor name */ - name = cpu_get_name(processor_name); - printf("CPU: %s\n", name); - post_code(POST_CPU_INFO);
return 0; }
+#ifndef CONFIG_BOARD_ENABLE +int print_cpuinfo(void) +{ + return ivybridge_checkcpu(); +} +#endif + void board_debug_uart_init(void) { /* This enables the debug UART */ pci_x86_write_config(NULL, PCH_LPC_DEV, LPC_EN, COMA_LPC_EN, PCI_SIZE_16); } + +static int cpu_x86_ivybridge_phase(struct udevice *dev, + enum board_phase_t phase) +{ + switch (phase) { + case BOARD_F_ARCH_CPU_INIT_DM: + return do_arch_cpu_init_dm(); + case BOARD_F_CHECKCPU: + return ivybridge_checkcpu(); + case BOARD_F_DRAM_INIT: + return ivybridge_dram_init(); + default: + return -ENOSYS; + } + + return 0; +} + +static int cpu_x86_ivybridge_board_probe(struct udevice *dev) +{ + return board_support_phase_mask(dev, + board_phase_mask(BOARD_F_ARCH_CPU_INIT_DM) | + board_phase_mask(BOARD_F_CHECKCPU) | + board_phase_mask(BOARD_F_DRAM_INIT)); +} + +static const struct board_ops cpu_x86_ivybridge_board_ops = { + .phase = cpu_x86_ivybridge_phase, +}; + +U_BOOT_DRIVER(cpu_x86_ivybridge_board_drv) = { + .name = "cpu_x86_ivybridge_board", + .id = UCLASS_BOARD, + .ops = &cpu_x86_ivybridge_board_ops, + .probe = cpu_x86_ivybridge_board_probe, + .flags = DM_FLAG_PRE_RELOC, +}; + +U_BOOT_DEVICE(cpu_x86_ivybridge_board) = { + .name = "cpu_x86_ivybridge_board", +}; diff --git a/arch/x86/cpu/ivybridge/sdram.c b/arch/x86/cpu/ivybridge/sdram.c index 201368c9c7..2484ed4859 100644 --- a/arch/x86/cpu/ivybridge/sdram.c +++ b/arch/x86/cpu/ivybridge/sdram.c @@ -401,7 +401,7 @@ static void rcba_config(void) setbits_le32(RCB_REG(FD), PCH_DISABLE_ALWAYS); }
-int dram_init(void) +int ivybridge_dram_init(void) { struct pei_data _pei_data __aligned(8) = { .pei_version = PEI_VERSION, @@ -541,8 +541,8 @@ int dram_init(void) /* S3 resume: don't save scrambler seed or MRC data */ if (pei_data->boot_mode != PEI_BOOT_RESUME) { /* - * This will be copied to SDRAM in reserve_arch(), then written - * to SPI flash in mrccache_save() + * This will be copied to SDRAM in the BOARD_F_RESERVE_ARCH + * call, then written to SPI flash in mrccache_save() */ gd->arch.mrc_output = (char *)pei_data->mrc_output; gd->arch.mrc_output_len = pei_data->mrc_output_len; @@ -559,3 +559,10 @@ int dram_init(void)
return 0; } + +#ifndef CONFIG_BOARD_ENABLE +int dram_init(void) +{ + return ivybridge_dram_init(); +} +#endif diff --git a/arch/x86/cpu/ivybridge/sdram_nop.c b/arch/x86/cpu/ivybridge/sdram_nop.c index bd1189e447..641d099bbf 100644 --- a/arch/x86/cpu/ivybridge/sdram_nop.c +++ b/arch/x86/cpu/ivybridge/sdram_nop.c @@ -5,10 +5,11 @@ */
#include <common.h> +#include <asm/arch/sandybridge.h>
DECLARE_GLOBAL_DATA_PTR;
-int dram_init(void) +int nop_dram_init(void) { gd->ram_size = 1ULL << 31; gd->bd->bi_dram[0].start = 0; @@ -16,3 +17,43 @@ int dram_init(void)
return 0; } + +#ifndef CONFIG_BOARD_ENABLE +int dram_init(void) +{ + return nop_dram_init(); +} +#endif + +static int cpu_x86_nop_phase(struct udevice *dev, enum board_phase_t phase) +{ + switch (phase) { + case BOARD_F_DRAM_INIT: + return nop_dram_init(); + default: + return -ENOSYS; + } + + return 0; +} + +static int cpu_x86_nop_board_probe(struct udevice *dev) +{ + return board_support_phase(dev, BOARD_F_DRAM_INIT); +} + +static const struct board_ops cpu_x86_nop_board_ops = { + .phase = cpu_x86_nop_phase, +}; + +U_BOOT_DRIVER(cpu_x86_nop_board_drv) = { + .name = "cpu_x86_nop_board", + .id = UCLASS_BOARD, + .ops = &cpu_x86_nop_board_ops, + .probe = cpu_x86_nop_board_probe, + .flags = DM_FLAG_PRE_RELOC, +}; + +U_BOOT_DEVICE(cpu_x86_nop_board) = { + .name = "cpu_x86_nop_board", +}; diff --git a/arch/x86/cpu/x86_64/cpu.c b/arch/x86/cpu/x86_64/cpu.c index db171f750d..ede46f7957 100644 --- a/arch/x86/cpu/x86_64/cpu.c +++ b/arch/x86/cpu/x86_64/cpu.c @@ -67,7 +67,9 @@ int misc_init_r(void) return 0; }
+#ifndef CONFIG_BOARD_ENABLE int print_cpuinfo(void) { return 0; } +#endif diff --git a/arch/x86/include/asm/arch-ivybridge/sandybridge.h b/arch/x86/include/asm/arch-ivybridge/sandybridge.h index 8e0f668f0b..52f4c17620 100644 --- a/arch/x86/include/asm/arch-ivybridge/sandybridge.h +++ b/arch/x86/include/asm/arch-ivybridge/sandybridge.h @@ -113,4 +113,11 @@ */ int bridge_silicon_revision(struct udevice *dev);
+/** + * ivybridge_dram_init() - Set up SDRAM + * + * @return 0 if OK, -ve on error + */ +int ivybridge_dram_init(void); + #endif diff --git a/arch/x86/lib/spl.c b/arch/x86/lib/spl.c index fa93d64a7a..a4c1a3ac35 100644 --- a/arch/x86/lib/spl.c +++ b/arch/x86/lib/spl.c @@ -48,6 +48,18 @@ static int x86_spl_init(void) return ret; } preloader_console_init(); +#ifdef CONFIG_BOARD_ENABLE + ret = board_walk_phase(BOARD_F_CHECKCPU); + if (ret) { + debug("%s: BOARD_F_CHECKCPU failed\n", __func__); + return ret; + } + ret = board_walk_phase(BOARD_F_DRAM_INIT); + if (ret) { + debug("%s: BOARD_F_DRAM_INIT failed\n", __func__); + return ret; + } +#else ret = print_cpuinfo(); if (ret) { debug("%s: print_cpuinfo() failed\n", __func__); @@ -58,6 +70,7 @@ static int x86_spl_init(void) debug("%s: dram_init() failed\n", __func__); return ret; } +#endif memset(&__bss_start, 0, (ulong)&__bss_end - (ulong)&__bss_start);
/* TODO(sjg@chromium.org): Consider calling cpu_init_r() here */ diff --git a/board/google/chromebook_link/link.c b/board/google/chromebook_link/link.c index 42615e1e23..99b1c91edc 100644 --- a/board/google/chromebook_link/link.c +++ b/board/google/chromebook_link/link.c @@ -17,7 +17,9 @@ int arch_early_init_r(void) return 0; }
+#ifndef CONFIG_BOARD_ENABLE int board_early_init_f(void) { return 0; } +#endif diff --git a/board/google/chromebox_panther/panther.c b/board/google/chromebox_panther/panther.c index e3baf88783..151cdd719d 100644 --- a/board/google/chromebox_panther/panther.c +++ b/board/google/chromebox_panther/panther.c @@ -12,7 +12,9 @@ int arch_early_init_r(void) return 0; }
+#ifndef CONFIG_BOARD_ENABLE int board_early_init_f(void) { return 0; } +#endif
Enable CONFIG_BOARD_ENABLE so that the new board init is used.
Signed-off-by: Simon Glass sjg@chromium.org ---
configs/chromebook_link64_defconfig | 1 + configs/chromebook_link_defconfig | 1 + configs/chromebox_panther_defconfig | 1 + 3 files changed, 3 insertions(+)
diff --git a/configs/chromebook_link64_defconfig b/configs/chromebook_link64_defconfig index 749cfd43b0..074b4a6a1a 100644 --- a/configs/chromebook_link64_defconfig +++ b/configs/chromebook_link64_defconfig @@ -19,6 +19,7 @@ CONFIG_SPL_LOAD_FIT=y CONFIG_BOOTSTAGE=y CONFIG_BOOTSTAGE_REPORT=y CONFIG_SYS_CONSOLE_INFO_QUIET=y +CONFIG_BOARD_ENABLE=y CONFIG_SPL_SYS_MALLOC_SIMPLE=y CONFIG_SPL_CPU_SUPPORT=y CONFIG_SPL_I2C_SUPPORT=y diff --git a/configs/chromebook_link_defconfig b/configs/chromebook_link_defconfig index 5ebb556f90..7588bc8f8e 100644 --- a/configs/chromebook_link_defconfig +++ b/configs/chromebook_link_defconfig @@ -11,6 +11,7 @@ CONFIG_FIT=y CONFIG_BOOTSTAGE=y CONFIG_BOOTSTAGE_REPORT=y CONFIG_SYS_CONSOLE_INFO_QUIET=y +CONFIG_BOARD_ENABLE=y CONFIG_HUSH_PARSER=y CONFIG_CMD_CPU=y # CONFIG_CMD_IMLS is not set diff --git a/configs/chromebox_panther_defconfig b/configs/chromebox_panther_defconfig index 0d65a90eba..fac227cb2a 100644 --- a/configs/chromebox_panther_defconfig +++ b/configs/chromebox_panther_defconfig @@ -9,6 +9,7 @@ CONFIG_FIT=y CONFIG_BOOTSTAGE=y CONFIG_BOOTSTAGE_REPORT=y CONFIG_SYS_CONSOLE_INFO_QUIET=y +CONFIG_BOARD_ENABLE=y CONFIG_HUSH_PARSER=y # CONFIG_CMD_IMLS is not set # CONFIG_CMD_FLASH is not set
Remove the legacy board init code since it is no-longer used.
Signed-off-by: Simon Glass sjg@chromium.org ---
arch/x86/cpu/ivybridge/cpu.c | 16 +--------------- arch/x86/cpu/ivybridge/sdram.c | 7 ------- arch/x86/cpu/ivybridge/sdram_nop.c | 7 ------- arch/x86/lib/spl.c | 13 ------------- board/google/chromebook_link/link.c | 7 ------- board/google/chromebox_panther/panther.c | 7 ------- 6 files changed, 1 insertion(+), 56 deletions(-)
diff --git a/arch/x86/cpu/ivybridge/cpu.c b/arch/x86/cpu/ivybridge/cpu.c index 96e69ef792..e7e6c3168a 100644 --- a/arch/x86/cpu/ivybridge/cpu.c +++ b/arch/x86/cpu/ivybridge/cpu.c @@ -84,13 +84,6 @@ static int do_arch_cpu_init_dm(void) return 0; }
-#ifndef CONFIG_BOARD_ENABLE -int arch_cpu_init_dm(void) -{ - return do_arch_cpu_init_dm(); -} -#endif - #define PCH_EHCI0_TEMP_BAR0 0xe8000000 #define PCH_EHCI1_TEMP_BAR0 0xe8000400 #define PCH_XHCI_TEMP_BAR0 0xe8001000 @@ -133,7 +126,7 @@ static void enable_usb_bar(struct udevice *bus) pci_bus_write_config(bus, usb3, PCI_COMMAND, cmd, PCI_SIZE_32); }
-static int ivybridge_checkcpu(void) +int ivybridge_checkcpu(void) { enum pei_boot_mode_t boot_mode = PEI_BOOT_NONE; struct udevice *dev, *lpc; @@ -192,13 +185,6 @@ static int ivybridge_checkcpu(void) return 0; }
-#ifndef CONFIG_BOARD_ENABLE -int print_cpuinfo(void) -{ - return ivybridge_checkcpu(); -} -#endif - void board_debug_uart_init(void) { /* This enables the debug UART */ diff --git a/arch/x86/cpu/ivybridge/sdram.c b/arch/x86/cpu/ivybridge/sdram.c index 2484ed4859..eb4d04f8da 100644 --- a/arch/x86/cpu/ivybridge/sdram.c +++ b/arch/x86/cpu/ivybridge/sdram.c @@ -559,10 +559,3 @@ int ivybridge_dram_init(void)
return 0; } - -#ifndef CONFIG_BOARD_ENABLE -int dram_init(void) -{ - return ivybridge_dram_init(); -} -#endif diff --git a/arch/x86/cpu/ivybridge/sdram_nop.c b/arch/x86/cpu/ivybridge/sdram_nop.c index 641d099bbf..6bf5366410 100644 --- a/arch/x86/cpu/ivybridge/sdram_nop.c +++ b/arch/x86/cpu/ivybridge/sdram_nop.c @@ -18,13 +18,6 @@ int nop_dram_init(void) return 0; }
-#ifndef CONFIG_BOARD_ENABLE -int dram_init(void) -{ - return nop_dram_init(); -} -#endif - static int cpu_x86_nop_phase(struct udevice *dev, enum board_phase_t phase) { switch (phase) { diff --git a/arch/x86/lib/spl.c b/arch/x86/lib/spl.c index a4c1a3ac35..0d40a6f41e 100644 --- a/arch/x86/lib/spl.c +++ b/arch/x86/lib/spl.c @@ -48,7 +48,6 @@ static int x86_spl_init(void) return ret; } preloader_console_init(); -#ifdef CONFIG_BOARD_ENABLE ret = board_walk_phase(BOARD_F_CHECKCPU); if (ret) { debug("%s: BOARD_F_CHECKCPU failed\n", __func__); @@ -59,18 +58,6 @@ static int x86_spl_init(void) debug("%s: BOARD_F_DRAM_INIT failed\n", __func__); return ret; } -#else - ret = print_cpuinfo(); - if (ret) { - debug("%s: print_cpuinfo() failed\n", __func__); - return ret; - } - ret = dram_init(); - if (ret) { - debug("%s: dram_init() failed\n", __func__); - return ret; - } -#endif memset(&__bss_start, 0, (ulong)&__bss_end - (ulong)&__bss_start);
/* TODO(sjg@chromium.org): Consider calling cpu_init_r() here */ diff --git a/board/google/chromebook_link/link.c b/board/google/chromebook_link/link.c index 99b1c91edc..64e7c1a08d 100644 --- a/board/google/chromebook_link/link.c +++ b/board/google/chromebook_link/link.c @@ -16,10 +16,3 @@ int arch_early_init_r(void) { return 0; } - -#ifndef CONFIG_BOARD_ENABLE -int board_early_init_f(void) -{ - return 0; -} -#endif diff --git a/board/google/chromebox_panther/panther.c b/board/google/chromebox_panther/panther.c index 151cdd719d..ed60e44264 100644 --- a/board/google/chromebox_panther/panther.c +++ b/board/google/chromebox_panther/panther.c @@ -11,10 +11,3 @@ int arch_early_init_r(void) { return 0; } - -#ifndef CONFIG_BOARD_ENABLE -int board_early_init_f(void) -{ - return 0; -} -#endif
Add a brief note pointing to the documentation.
Signed-off-by: Simon Glass sjg@chromium.org ---
README | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/README b/README index aa907ced8a..891da97838 100644 --- a/README +++ b/README @@ -222,6 +222,9 @@ See board/sandbox/README.sandbox for more details. Board Initialisation Flow: --------------------------
+Note: An effort to organise and perhaps rationalise the many init hooks in the +init sequence has started. See doc/driver-model/board-info.txt for details. + This is the intended start-up flow for boards. This should apply for both SPL and U-Boot proper (i.e. they both follow the same rules).
On Sun, Mar 19, 2017 at 12:59:19PM -0600, Simon Glass wrote:
At present we have a lot of ad-hoc init functions related to boards, for example board_early_init_f(), board_misc_init_f() and dram_init().
There are used in different ways by different boards as useful hooks to do the required init and sequence it correctly. Some functions are always enabled but have a __weak default. Some are controlled by the existence of a CONFIG.
There are two main init sequences: board_init_f() (f for running from read-only flash) which runs before relocation and board_init_r() (r for relocated) which runs afterwards.
One problem with the current sequence is that it has a lot of arch-specific #ifdefs around various functions. There are also #ifdefs for various features. There has been quite a bit of discussion about how to tidy this up and at least one RFC series[1].
Now that we have driver model we can use this to deal with the init sequences. This approach has several advantages:
- We have a path to remove the #ifdefs
- It is easy for multiple parts of the code to implement the same hook
- We can track what is called and what is not
- We don't need weak functions
- We can eventually adjust the sequence to improve naming or to add new
init phases
- It provides a model for how we might deal with ft_board_setup() and
friends
This series starts the process of replacing the pre-relocation init sequence with a driver-model solution. It defines a uclass, adds tests and converts sandbox and a few x86 boards over to use this new setup.
This series is not ready for use yet as the rest of the init sequence hooks need to be converted. But there is enough here to show the idea.
Comments welcome.
[1] https://lists.denx.de/pipermail/u-boot/2011-August/098718.html
How does this look, size wise? With all of these conversions and clean-ups, we really need to be size concious as well as it all keeps adding up. Thanks!
Hi Tom,
On 19 March 2017 at 18:47, Tom Rini trini@konsulko.com wrote:
On Sun, Mar 19, 2017 at 12:59:19PM -0600, Simon Glass wrote:
At present we have a lot of ad-hoc init functions related to boards, for example board_early_init_f(), board_misc_init_f() and dram_init().
There are used in different ways by different boards as useful hooks to do the required init and sequence it correctly. Some functions are always enabled but have a __weak default. Some are controlled by the existence of a CONFIG.
There are two main init sequences: board_init_f() (f for running from read-only flash) which runs before relocation and board_init_r() (r for relocated) which runs afterwards.
One problem with the current sequence is that it has a lot of arch-specific #ifdefs around various functions. There are also #ifdefs for various features. There has been quite a bit of discussion about how to tidy this up and at least one RFC series[1].
Now that we have driver model we can use this to deal with the init sequences. This approach has several advantages:
- We have a path to remove the #ifdefs
- It is easy for multiple parts of the code to implement the same hook
- We can track what is called and what is not
- We don't need weak functions
- We can eventually adjust the sequence to improve naming or to add new
init phases
- It provides a model for how we might deal with ft_board_setup() and
friends
This series starts the process of replacing the pre-relocation init sequence with a driver-model solution. It defines a uclass, adds tests and converts sandbox and a few x86 boards over to use this new setup.
This series is not ready for use yet as the rest of the init sequence hooks need to be converted. But there is enough here to show the idea.
Comments welcome.
[1] https://lists.denx.de/pipermail/u-boot/2011-August/098718.html
How does this look, size wise? With all of these conversions and clean-ups, we really need to be size concious as well as it all keeps adding up. Thanks!
It include size a bit - e.g. x86 808 bytes of text, although that does include a few extra features.
11: dm: board: Add documentation 16: dm: x86: board: ivybridge: Remove old board init code x86: (for 1/1 boards) all +4331.0 bss +2944.0 data +252.0 rodata +327.0 text +808.0 chromebook_link: all +4331 bss +2944 data +252 rodata +327 text +808 u-boot: add: 22/0, grow: 1/-4 bytes: 3012/-2183 (829) function old new delta ivybridge_dram_init - 1956 +1956 cpu_x86_ivybridge_phase - 151 +151 board_walk_phase_count - 145 +145 ivybridge_checkcpu - 96 +96 board_phase - 95 +95 do_board - 77 +77 cpu_print_info - 76 +76 _u_boot_list_2_uclass_2_board - 72 +72 _u_boot_list_2_driver_2_cpu_x86_ivybridge_board_drv - 68 +68 _u_boot_list_2_driver_2__cpu_x86_board_drv - 68 +68 board_walk_phase - 28 +28 board_walk_opt_phase - 28 +28 _u_boot_list_2_cmd_2_board - 28 +28 board_support_phase - 27 +27 board_support_phase_mask - 20 +20 cpu_x86_phase - 14 +14 misc_init_f - 10 +10 cpu_x86_ivybridge_board_probe - 10 +10 cpu_x86_board_probe - 10 +10 checkcpu - 10 +10 _u_boot_list_2_driver_info_2_cpu_x86_ivybridge_board - 8 +8 _u_boot_list_2_driver_info_2_cpu_x86_board - 8 +8 board_early_init_f 3 10 +7 reserve_arch 14 10 -4 arch_cpu_init_dm 120 7 -113 print_cpuinfo 125 5 -120 dram_init 1956 10 -1946 (no errors to report)
I think I can use a linker-list approach to reduce the overhead. But I still think the driver has value as it provides a means of adding hooks to do board-specific things from drivers, something that we keep running into. Also the idea of a board 'driver' seems conceptually useful.
So one approach would be to have:
1. A linker-list-based board hook setup, where you can declare, for example:
static int ivybridge_dram_init(void) { ... } U_BOOT_BOARD_HOOK(ivybridge_dram_init, BOARD_F_DRAM_INIT);
This should be pretty cheap, perhaps <200 bytes with some care.
2. An optional BOARD uclass which can be used for more involved situations, but with a higher code size penalty.
Regards, Simon
On Wed, Mar 22, 2017 at 07:05:38AM -0600, Simon Glass wrote:
Hi Tom,
On 19 March 2017 at 18:47, Tom Rini trini@konsulko.com wrote:
On Sun, Mar 19, 2017 at 12:59:19PM -0600, Simon Glass wrote:
At present we have a lot of ad-hoc init functions related to boards, for example board_early_init_f(), board_misc_init_f() and dram_init().
There are used in different ways by different boards as useful hooks to do the required init and sequence it correctly. Some functions are always enabled but have a __weak default. Some are controlled by the existence of a CONFIG.
There are two main init sequences: board_init_f() (f for running from read-only flash) which runs before relocation and board_init_r() (r for relocated) which runs afterwards.
One problem with the current sequence is that it has a lot of arch-specific #ifdefs around various functions. There are also #ifdefs for various features. There has been quite a bit of discussion about how to tidy this up and at least one RFC series[1].
Now that we have driver model we can use this to deal with the init sequences. This approach has several advantages:
- We have a path to remove the #ifdefs
- It is easy for multiple parts of the code to implement the same hook
- We can track what is called and what is not
- We don't need weak functions
- We can eventually adjust the sequence to improve naming or to add new
init phases
- It provides a model for how we might deal with ft_board_setup() and
friends
This series starts the process of replacing the pre-relocation init sequence with a driver-model solution. It defines a uclass, adds tests and converts sandbox and a few x86 boards over to use this new setup.
This series is not ready for use yet as the rest of the init sequence hooks need to be converted. But there is enough here to show the idea.
Comments welcome.
[1] https://lists.denx.de/pipermail/u-boot/2011-August/098718.html
How does this look, size wise? With all of these conversions and clean-ups, we really need to be size concious as well as it all keeps adding up. Thanks!
It include size a bit - e.g. x86 808 bytes of text, although that does include a few extra features.
How about if we don't include some of the extra debug/demo type features (which are useful at times, certainly) ? We keep adding things that add a few bytes here and a few bytes there and it all adds up.
[snip]
I think I can use a linker-list approach to reduce the overhead. But I still think the driver has value as it provides a means of adding hooks to do board-specific things from drivers, something that we keep running into. Also the idea of a board 'driver' seems conceptually useful.
So one approach would be to have:
- A linker-list-based board hook setup, where you can declare, for example:
static int ivybridge_dram_init(void) { ... } U_BOOT_BOARD_HOOK(ivybridge_dram_init, BOARD_F_DRAM_INIT);
This should be pretty cheap, perhaps <200 bytes with some care.
- An optional BOARD uclass which can be used for more involved
situations, but with a higher code size penalty.
OK. But I also recall that we had talked about trying to condense and re-factor things. My worry about an approach like this is it allows for us to more easily get back into the bad habits of having each architecture solve similar problems with different solutions.
Hi Tom,
On 22 March 2017 at 08:37, Tom Rini trini@konsulko.com wrote:
On Wed, Mar 22, 2017 at 07:05:38AM -0600, Simon Glass wrote:
Hi Tom,
On 19 March 2017 at 18:47, Tom Rini trini@konsulko.com wrote:
On Sun, Mar 19, 2017 at 12:59:19PM -0600, Simon Glass wrote:
At present we have a lot of ad-hoc init functions related to boards, for example board_early_init_f(), board_misc_init_f() and dram_init().
There are used in different ways by different boards as useful hooks to do the required init and sequence it correctly. Some functions are always enabled but have a __weak default. Some are controlled by the existence of a CONFIG.
There are two main init sequences: board_init_f() (f for running from read-only flash) which runs before relocation and board_init_r() (r for relocated) which runs afterwards.
One problem with the current sequence is that it has a lot of arch-specific #ifdefs around various functions. There are also #ifdefs for various features. There has been quite a bit of discussion about how to tidy this up and at least one RFC series[1].
Now that we have driver model we can use this to deal with the init sequences. This approach has several advantages:
- We have a path to remove the #ifdefs
- It is easy for multiple parts of the code to implement the same hook
- We can track what is called and what is not
- We don't need weak functions
- We can eventually adjust the sequence to improve naming or to add new
init phases
- It provides a model for how we might deal with ft_board_setup() and
friends
This series starts the process of replacing the pre-relocation init sequence with a driver-model solution. It defines a uclass, adds tests and converts sandbox and a few x86 boards over to use this new setup.
This series is not ready for use yet as the rest of the init sequence hooks need to be converted. But there is enough here to show the idea.
Comments welcome.
[1] https://lists.denx.de/pipermail/u-boot/2011-August/098718.html
How does this look, size wise? With all of these conversions and clean-ups, we really need to be size concious as well as it all keeps adding up. Thanks!
It include size a bit - e.g. x86 808 bytes of text, although that does include a few extra features.
How about if we don't include some of the extra debug/demo type features (which are useful at times, certainly) ? We keep adding things that add a few bytes here and a few bytes there and it all adds up.
Yes it's very important that the base version doesn't increase size, or at least only minimally. I should have examined that more closely in the RFC - my intent was really to get comments on the approach,
[snip]
I think I can use a linker-list approach to reduce the overhead. But I still think the driver has value as it provides a means of adding hooks to do board-specific things from drivers, something that we keep running into. Also the idea of a board 'driver' seems conceptually useful.
So one approach would be to have:
- A linker-list-based board hook setup, where you can declare, for example:
static int ivybridge_dram_init(void) { ... } U_BOOT_BOARD_HOOK(ivybridge_dram_init, BOARD_F_DRAM_INIT);
This should be pretty cheap, perhaps <200 bytes with some care.
- An optional BOARD uclass which can be used for more involved
situations, but with a higher code size penalty.
OK. But I also recall that we had talked about trying to condense and re-factor things. My worry about an approach like this is it allows for us to more easily get back into the bad habits of having each architecture solve similar problems with different solutions.
Yes that's true and I've been pushing back on this for a while. For example there is quite a bit of pressure to add board-specific init code to drivers with driver model. So far I think we have been able to avoid this using device tree and other drivers. For example if MMC needs a clock we can find the required clock by phandle and call the clock driver.
So are you thinking we should limit this to just a simple hook approach for now, and then consider the board uclass down the track?
Regards, Simon
On Wed, Mar 22, 2017 at 08:43:54AM -0600, Simon Glass wrote:
Hi Tom,
On 22 March 2017 at 08:37, Tom Rini trini@konsulko.com wrote:
On Wed, Mar 22, 2017 at 07:05:38AM -0600, Simon Glass wrote:
Hi Tom,
On 19 March 2017 at 18:47, Tom Rini trini@konsulko.com wrote:
On Sun, Mar 19, 2017 at 12:59:19PM -0600, Simon Glass wrote:
At present we have a lot of ad-hoc init functions related to boards, for example board_early_init_f(), board_misc_init_f() and dram_init().
There are used in different ways by different boards as useful hooks to do the required init and sequence it correctly. Some functions are always enabled but have a __weak default. Some are controlled by the existence of a CONFIG.
There are two main init sequences: board_init_f() (f for running from read-only flash) which runs before relocation and board_init_r() (r for relocated) which runs afterwards.
One problem with the current sequence is that it has a lot of arch-specific #ifdefs around various functions. There are also #ifdefs for various features. There has been quite a bit of discussion about how to tidy this up and at least one RFC series[1].
Now that we have driver model we can use this to deal with the init sequences. This approach has several advantages:
- We have a path to remove the #ifdefs
- It is easy for multiple parts of the code to implement the same hook
- We can track what is called and what is not
- We don't need weak functions
- We can eventually adjust the sequence to improve naming or to add new
init phases
- It provides a model for how we might deal with ft_board_setup() and
friends
This series starts the process of replacing the pre-relocation init sequence with a driver-model solution. It defines a uclass, adds tests and converts sandbox and a few x86 boards over to use this new setup.
This series is not ready for use yet as the rest of the init sequence hooks need to be converted. But there is enough here to show the idea.
Comments welcome.
[1] https://lists.denx.de/pipermail/u-boot/2011-August/098718.html
How does this look, size wise? With all of these conversions and clean-ups, we really need to be size concious as well as it all keeps adding up. Thanks!
It include size a bit - e.g. x86 808 bytes of text, although that does include a few extra features.
How about if we don't include some of the extra debug/demo type features (which are useful at times, certainly) ? We keep adding things that add a few bytes here and a few bytes there and it all adds up.
Yes it's very important that the base version doesn't increase size, or at least only minimally. I should have examined that more closely in the RFC - my intent was really to get comments on the approach,
[snip]
I think I can use a linker-list approach to reduce the overhead. But I still think the driver has value as it provides a means of adding hooks to do board-specific things from drivers, something that we keep running into. Also the idea of a board 'driver' seems conceptually useful.
So one approach would be to have:
- A linker-list-based board hook setup, where you can declare, for example:
static int ivybridge_dram_init(void) { ... } U_BOOT_BOARD_HOOK(ivybridge_dram_init, BOARD_F_DRAM_INIT);
This should be pretty cheap, perhaps <200 bytes with some care.
- An optional BOARD uclass which can be used for more involved
situations, but with a higher code size penalty.
OK. But I also recall that we had talked about trying to condense and re-factor things. My worry about an approach like this is it allows for us to more easily get back into the bad habits of having each architecture solve similar problems with different solutions.
Yes that's true and I've been pushing back on this for a while. For example there is quite a bit of pressure to add board-specific init code to drivers with driver model. So far I think we have been able to avoid this using device tree and other drivers. For example if MMC needs a clock we can find the required clock by phandle and call the clock driver.
So are you thinking we should limit this to just a simple hook approach for now, and then consider the board uclass down the track?
Looking over init_sequence_[rf], I can certainly see the case for "ug, this is ugly and we need to make it better" (and I now wonder if we don't have a lot more places where we need INIT_FUNC_WATCHDOG_RESET, anyhow...). But for the moment we seem to be able to resist adding more calls here. And I would like to see if we can rework / reduce the current table before we try and simplify and clean-up the mechanism that we use to handle them.
Hi Tom,
On 22 March 2017 at 09:13, Tom Rini trini@konsulko.com wrote:
On Wed, Mar 22, 2017 at 08:43:54AM -0600, Simon Glass wrote:
Hi Tom,
On 22 March 2017 at 08:37, Tom Rini trini@konsulko.com wrote:
On Wed, Mar 22, 2017 at 07:05:38AM -0600, Simon Glass wrote:
Hi Tom,
On 19 March 2017 at 18:47, Tom Rini trini@konsulko.com wrote:
On Sun, Mar 19, 2017 at 12:59:19PM -0600, Simon Glass wrote:
At present we have a lot of ad-hoc init functions related to boards, for example board_early_init_f(), board_misc_init_f() and dram_init().
There are used in different ways by different boards as useful hooks to do the required init and sequence it correctly. Some functions are always enabled but have a __weak default. Some are controlled by the existence of a CONFIG.
There are two main init sequences: board_init_f() (f for running from read-only flash) which runs before relocation and board_init_r() (r for relocated) which runs afterwards.
One problem with the current sequence is that it has a lot of arch-specific #ifdefs around various functions. There are also #ifdefs for various features. There has been quite a bit of discussion about how to tidy this up and at least one RFC series[1].
Now that we have driver model we can use this to deal with the init sequences. This approach has several advantages:
- We have a path to remove the #ifdefs
- It is easy for multiple parts of the code to implement the same hook
- We can track what is called and what is not
- We don't need weak functions
- We can eventually adjust the sequence to improve naming or to add new
init phases
- It provides a model for how we might deal with ft_board_setup() and
friends
This series starts the process of replacing the pre-relocation init sequence with a driver-model solution. It defines a uclass, adds tests and converts sandbox and a few x86 boards over to use this new setup.
This series is not ready for use yet as the rest of the init sequence hooks need to be converted. But there is enough here to show the idea.
Comments welcome.
[1] https://lists.denx.de/pipermail/u-boot/2011-August/098718.html
How does this look, size wise? With all of these conversions and clean-ups, we really need to be size concious as well as it all keeps adding up. Thanks!
It include size a bit - e.g. x86 808 bytes of text, although that does include a few extra features.
How about if we don't include some of the extra debug/demo type features (which are useful at times, certainly) ? We keep adding things that add a few bytes here and a few bytes there and it all adds up.
Yes it's very important that the base version doesn't increase size, or at least only minimally. I should have examined that more closely in the RFC - my intent was really to get comments on the approach,
[snip]
I think I can use a linker-list approach to reduce the overhead. But I still think the driver has value as it provides a means of adding hooks to do board-specific things from drivers, something that we keep running into. Also the idea of a board 'driver' seems conceptually useful.
So one approach would be to have:
- A linker-list-based board hook setup, where you can declare, for example:
static int ivybridge_dram_init(void) { ... } U_BOOT_BOARD_HOOK(ivybridge_dram_init, BOARD_F_DRAM_INIT);
This should be pretty cheap, perhaps <200 bytes with some care.
- An optional BOARD uclass which can be used for more involved
situations, but with a higher code size penalty.
OK. But I also recall that we had talked about trying to condense and re-factor things. My worry about an approach like this is it allows for us to more easily get back into the bad habits of having each architecture solve similar problems with different solutions.
Yes that's true and I've been pushing back on this for a while. For example there is quite a bit of pressure to add board-specific init code to drivers with driver model. So far I think we have been able to avoid this using device tree and other drivers. For example if MMC needs a clock we can find the required clock by phandle and call the clock driver.
So are you thinking we should limit this to just a simple hook approach for now, and then consider the board uclass down the track?
Looking over init_sequence_[rf], I can certainly see the case for "ug, this is ugly and we need to make it better" (and I now wonder if we don't have a lot more places where we need INIT_FUNC_WATCHDOG_RESET, anyhow...). But for the moment we seem to be able to resist adding more calls here. And I would like to see if we can rework / reduce the current table before we try and simplify and clean-up the mechanism that we use to handle them.
I agree, and I have some concern that making it easier to extend the init sequence might end up with less consistency between archs as to the sequence we go through during init.
For now I've done two series to tidy up board_f. There is more to do though. We can park this series until we get a bit closer (it might be quite a while).
Regards, Simon
Hi Simon,
On Sat, Apr 1, 2017 at 12:21 PM, Simon Glass sjg@chromium.org wrote:
Hi Tom,
On 22 March 2017 at 09:13, Tom Rini trini@konsulko.com wrote:
On Wed, Mar 22, 2017 at 08:43:54AM -0600, Simon Glass wrote:
Hi Tom,
On 22 March 2017 at 08:37, Tom Rini trini@konsulko.com wrote:
On Wed, Mar 22, 2017 at 07:05:38AM -0600, Simon Glass wrote:
Hi Tom,
On 19 March 2017 at 18:47, Tom Rini trini@konsulko.com wrote:
On Sun, Mar 19, 2017 at 12:59:19PM -0600, Simon Glass wrote: > At present we have a lot of ad-hoc init functions related to boards, for > example board_early_init_f(), board_misc_init_f() and dram_init(). > > There are used in different ways by different boards as useful hooks to > do the required init and sequence it correctly. Some functions are always > enabled but have a __weak default. Some are controlled by the existence > of a CONFIG. > > There are two main init sequences: board_init_f() (f for running from > read-only flash) which runs before relocation and board_init_r() (r for > relocated) which runs afterwards. > > One problem with the current sequence is that it has a lot of > arch-specific #ifdefs around various functions. There are also #ifdefs > for various features. There has been quite a bit of discussion about how > to tidy this up and at least one RFC series[1]. > > Now that we have driver model we can use this to deal with the init > sequences. This approach has several advantages: > > - We have a path to remove the #ifdefs > - It is easy for multiple parts of the code to implement the same hook > - We can track what is called and what is not > - We don't need weak functions > - We can eventually adjust the sequence to improve naming or to add new > init phases > - It provides a model for how we might deal with ft_board_setup() and > friends > > This series starts the process of replacing the pre-relocation init > sequence with a driver-model solution. It defines a uclass, adds tests > and converts sandbox and a few x86 boards over to use this new setup. > > This series is not ready for use yet as the rest of the init sequence > hooks need to be converted. But there is enough here to show the idea. > > Comments welcome. > > [1] https://lists.denx.de/pipermail/u-boot/2011-August/098718.html
How does this look, size wise? With all of these conversions and clean-ups, we really need to be size concious as well as it all keeps adding up. Thanks!
It include size a bit - e.g. x86 808 bytes of text, although that does include a few extra features.
How about if we don't include some of the extra debug/demo type features (which are useful at times, certainly) ? We keep adding things that add a few bytes here and a few bytes there and it all adds up.
Yes it's very important that the base version doesn't increase size, or at least only minimally. I should have examined that more closely in the RFC - my intent was really to get comments on the approach,
[snip]
I think I can use a linker-list approach to reduce the overhead. But I still think the driver has value as it provides a means of adding hooks to do board-specific things from drivers, something that we keep running into. Also the idea of a board 'driver' seems conceptually useful.
So one approach would be to have:
- A linker-list-based board hook setup, where you can declare, for example:
static int ivybridge_dram_init(void) { ... } U_BOOT_BOARD_HOOK(ivybridge_dram_init, BOARD_F_DRAM_INIT);
This should be pretty cheap, perhaps <200 bytes with some care.
- An optional BOARD uclass which can be used for more involved
situations, but with a higher code size penalty.
OK. But I also recall that we had talked about trying to condense and re-factor things. My worry about an approach like this is it allows for us to more easily get back into the bad habits of having each architecture solve similar problems with different solutions.
Yes that's true and I've been pushing back on this for a while. For example there is quite a bit of pressure to add board-specific init code to drivers with driver model. So far I think we have been able to avoid this using device tree and other drivers. For example if MMC needs a clock we can find the required clock by phandle and call the clock driver.
So are you thinking we should limit this to just a simple hook approach for now, and then consider the board uclass down the track?
Looking over init_sequence_[rf], I can certainly see the case for "ug, this is ugly and we need to make it better" (and I now wonder if we don't have a lot more places where we need INIT_FUNC_WATCHDOG_RESET, anyhow...). But for the moment we seem to be able to resist adding more calls here. And I would like to see if we can rework / reduce the current table before we try and simplify and clean-up the mechanism that we use to handle them.
I agree, and I have some concern that making it easier to extend the init sequence might end up with less consistency between archs as to the sequence we go through during init.
For now I've done two series to tidy up board_f. There is more to do though. We can park this series until we get a bit closer (it might be quite a while).
I did not track this series. What's our next step regarding to this series? I see some of them are x86-specific which I can apply, no?
Regards, Bin
Hi Bin,
On 11 April 2017 at 20:39, Bin Meng bmeng.cn@gmail.com wrote:
Hi Simon,
On Sat, Apr 1, 2017 at 12:21 PM, Simon Glass sjg@chromium.org wrote:
Hi Tom,
On 22 March 2017 at 09:13, Tom Rini trini@konsulko.com wrote:
On Wed, Mar 22, 2017 at 08:43:54AM -0600, Simon Glass wrote:
Hi Tom,
On 22 March 2017 at 08:37, Tom Rini trini@konsulko.com wrote:
On Wed, Mar 22, 2017 at 07:05:38AM -0600, Simon Glass wrote:
Hi Tom,
On 19 March 2017 at 18:47, Tom Rini trini@konsulko.com wrote: > On Sun, Mar 19, 2017 at 12:59:19PM -0600, Simon Glass wrote: >> At present we have a lot of ad-hoc init functions related to boards, for >> example board_early_init_f(), board_misc_init_f() and dram_init(). >> >> There are used in different ways by different boards as useful hooks to >> do the required init and sequence it correctly. Some functions are always >> enabled but have a __weak default. Some are controlled by the existence >> of a CONFIG. >> >> There are two main init sequences: board_init_f() (f for running from >> read-only flash) which runs before relocation and board_init_r() (r for >> relocated) which runs afterwards. >> >> One problem with the current sequence is that it has a lot of >> arch-specific #ifdefs around various functions. There are also #ifdefs >> for various features. There has been quite a bit of discussion about how >> to tidy this up and at least one RFC series[1]. >> >> Now that we have driver model we can use this to deal with the init >> sequences. This approach has several advantages: >> >> - We have a path to remove the #ifdefs >> - It is easy for multiple parts of the code to implement the same hook >> - We can track what is called and what is not >> - We don't need weak functions >> - We can eventually adjust the sequence to improve naming or to add new >> init phases >> - It provides a model for how we might deal with ft_board_setup() and >> friends >> >> This series starts the process of replacing the pre-relocation init >> sequence with a driver-model solution. It defines a uclass, adds tests >> and converts sandbox and a few x86 boards over to use this new setup. >> >> This series is not ready for use yet as the rest of the init sequence >> hooks need to be converted. But there is enough here to show the idea. >> >> Comments welcome. >> >> [1] https://lists.denx.de/pipermail/u-boot/2011-August/098718.html > > How does this look, size wise? With all of these conversions and > clean-ups, we really need to be size concious as well as it all keeps > adding up. Thanks!
It include size a bit - e.g. x86 808 bytes of text, although that does include a few extra features.
How about if we don't include some of the extra debug/demo type features (which are useful at times, certainly) ? We keep adding things that add a few bytes here and a few bytes there and it all adds up.
Yes it's very important that the base version doesn't increase size, or at least only minimally. I should have examined that more closely in the RFC - my intent was really to get comments on the approach,
[snip]
I think I can use a linker-list approach to reduce the overhead. But I still think the driver has value as it provides a means of adding hooks to do board-specific things from drivers, something that we keep running into. Also the idea of a board 'driver' seems conceptually useful.
So one approach would be to have:
- A linker-list-based board hook setup, where you can declare, for example:
static int ivybridge_dram_init(void) { ... } U_BOOT_BOARD_HOOK(ivybridge_dram_init, BOARD_F_DRAM_INIT);
This should be pretty cheap, perhaps <200 bytes with some care.
- An optional BOARD uclass which can be used for more involved
situations, but with a higher code size penalty.
OK. But I also recall that we had talked about trying to condense and re-factor things. My worry about an approach like this is it allows for us to more easily get back into the bad habits of having each architecture solve similar problems with different solutions.
Yes that's true and I've been pushing back on this for a while. For example there is quite a bit of pressure to add board-specific init code to drivers with driver model. So far I think we have been able to avoid this using device tree and other drivers. For example if MMC needs a clock we can find the required clock by phandle and call the clock driver.
So are you thinking we should limit this to just a simple hook approach for now, and then consider the board uclass down the track?
Looking over init_sequence_[rf], I can certainly see the case for "ug, this is ugly and we need to make it better" (and I now wonder if we don't have a lot more places where we need INIT_FUNC_WATCHDOG_RESET, anyhow...). But for the moment we seem to be able to resist adding more calls here. And I would like to see if we can rework / reduce the current table before we try and simplify and clean-up the mechanism that we use to handle them.
I agree, and I have some concern that making it easier to extend the init sequence might end up with less consistency between archs as to the sequence we go through during init.
For now I've done two series to tidy up board_f. There is more to do though. We can park this series until we get a bit closer (it might be quite a while).
I did not track this series. What's our next step regarding to this series? I see some of them are x86-specific which I can apply, no?
I think it is on hold for now. But yes you can apply the x86 patches.
I may come back to this later after the init sequence is tidied more, hopefully later in the year.
Regards, Simon
Hi Simon,
On 03/19/17 20:59, Simon Glass wrote:
At present we have a lot of ad-hoc init functions related to boards, for example board_early_init_f(), board_misc_init_f() and dram_init().
There are used in different ways by different boards as useful hooks to do the required init and sequence it correctly. Some functions are always enabled but have a __weak default. Some are controlled by the existence of a CONFIG.
There are two main init sequences: board_init_f() (f for running from read-only flash) which runs before relocation and board_init_r() (r for relocated) which runs afterwards.
One problem with the current sequence is that it has a lot of arch-specific #ifdefs around various functions. There are also #ifdefs for various features. There has been quite a bit of discussion about how to tidy this up and at least one RFC series[1].
Now that we have driver model we can use this to deal with the init sequences. This approach has several advantages:
- We have a path to remove the #ifdefs
- It is easy for multiple parts of the code to implement the same hook
- We can track what is called and what is not
- We don't need weak functions
- We can eventually adjust the sequence to improve naming or to add new
init phases
- It provides a model for how we might deal with ft_board_setup() and
friends
I haven't got this one yet from the patchset. May be I need to look closer...
This series starts the process of replacing the pre-relocation init sequence with a driver-model solution. It defines a uclass, adds tests and converts sandbox and a few x86 boards over to use this new setup.
This series is not ready for use yet as the rest of the init sequence hooks need to be converted. But there is enough here to show the idea.
Comments welcome.
Overall, it sounds like a great idea!
Hi Igor,
On 20 March 2017 at 01:30, Igor Grinberg grinberg@compulab.co.il wrote:
Hi Simon,
On 03/19/17 20:59, Simon Glass wrote:
At present we have a lot of ad-hoc init functions related to boards, for example board_early_init_f(), board_misc_init_f() and dram_init().
There are used in different ways by different boards as useful hooks to do the required init and sequence it correctly. Some functions are always enabled but have a __weak default. Some are controlled by the existence of a CONFIG.
There are two main init sequences: board_init_f() (f for running from read-only flash) which runs before relocation and board_init_r() (r for relocated) which runs afterwards.
One problem with the current sequence is that it has a lot of arch-specific #ifdefs around various functions. There are also #ifdefs for various features. There has been quite a bit of discussion about how to tidy this up and at least one RFC series[1].
Now that we have driver model we can use this to deal with the init sequences. This approach has several advantages:
- We have a path to remove the #ifdefs
- It is easy for multiple parts of the code to implement the same hook
- We can track what is called and what is not
- We don't need weak functions
- We can eventually adjust the sequence to improve naming or to add new
init phases
- It provides a model for how we might deal with ft_board_setup() and
friends
I haven't got this one yet from the patchset. May be I need to look closer...
It is not included in this patch set. I just mean that we could do a similar scheme with those functions.
This series starts the process of replacing the pre-relocation init sequence with a driver-model solution. It defines a uclass, adds tests and converts sandbox and a few x86 boards over to use this new setup.
This series is not ready for use yet as the rest of the init sequence hooks need to be converted. But there is enough here to show the idea.
Comments welcome.
Overall, it sounds like a great idea!
I'll do another spin and see if I can tighten it up a little.
-- Regards, Igor.
Regards, Simon
On 03/19/2017 11:59 AM, Simon Glass wrote:
At present we have a lot of ad-hoc init functions related to boards, for example board_early_init_f(), board_misc_init_f() and dram_init().
There are used in different ways by different boards as useful hooks to do the required init and sequence it correctly. Some functions are always enabled but have a __weak default. Some are controlled by the existence of a CONFIG.
There are two main init sequences: board_init_f() (f for running from read-only flash) which runs before relocation and board_init_r() (r for relocated) which runs afterwards.
One problem with the current sequence is that it has a lot of arch-specific #ifdefs around various functions. There are also #ifdefs for various features. There has been quite a bit of discussion about how to tidy this up and at least one RFC series[1].
Now that we have driver model we can use this to deal with the init sequences. This approach has several advantages:
- We have a path to remove the #ifdefs
- It is easy for multiple parts of the code to implement the same hook
- We can track what is called and what is not
- We don't need weak functions
- We can eventually adjust the sequence to improve naming or to add new
init phases
- It provides a model for how we might deal with ft_board_setup() and
friends
This series starts the process of replacing the pre-relocation init sequence with a driver-model solution.
I like the idea to be able to use DM earlier.
York
Hi York,
On 22 March 2017 at 10:00, york sun york.sun@nxp.com wrote:
On 03/19/2017 11:59 AM, Simon Glass wrote:
At present we have a lot of ad-hoc init functions related to boards, for example board_early_init_f(), board_misc_init_f() and dram_init().
There are used in different ways by different boards as useful hooks to do the required init and sequence it correctly. Some functions are always enabled but have a __weak default. Some are controlled by the existence of a CONFIG.
There are two main init sequences: board_init_f() (f for running from read-only flash) which runs before relocation and board_init_r() (r for relocated) which runs afterwards.
One problem with the current sequence is that it has a lot of arch-specific #ifdefs around various functions. There are also #ifdefs for various features. There has been quite a bit of discussion about how to tidy this up and at least one RFC series[1].
Now that we have driver model we can use this to deal with the init sequences. This approach has several advantages:
- We have a path to remove the #ifdefs
- It is easy for multiple parts of the code to implement the same hook
- We can track what is called and what is not
- We don't need weak functions
- We can eventually adjust the sequence to improve naming or to add new
init phases
- It provides a model for how we might deal with ft_board_setup() and
friends
This series starts the process of replacing the pre-relocation init sequence with a driver-model solution.
I like the idea to be able to use DM earlier.
Well actually my series does not allow that. Here is the start of the init sequence with u-boot-dm/rmb-working applied:
static const init_fnc_t init_sequence_f[] = { setup_mon_len, #ifdef CONFIG_OF_CONTROL fdtdec_setup, #endif #ifdef CONFIG_TRACE trace_early_init, #endif initf_malloc, initf_console_record, #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,
Of these: - Setting up device tree must happen before DM - trace could be moved later, but then you lose tracing of DM init - malloc() is needed - recording console could move, but as it is it records DM init console debug output, which could be useful - arch_fsp_init() and arch_cpu_init() and mach_cpu_init() could perhaps move later depending on how we define them
Anyway I'm interested in your suggestions.
Regards, Simon
participants (5)
-
Bin Meng -
Igor Grinberg -
Simon Glass -
Tom Rini -
york sun