[PATCH v2 14/32] bootm: Adjust position of unmap_sysmem() in boot_get_kernel()
These unmaps should happen regardless of the return value. Move them
before the 'return' statement.
Signed-off-by: Simon Glass
---
Changes in v2:
- Add new patch to adjust position of unmap_sysmem() in boot_get_kernel()
boot/bootm.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/boot/bootm.c b/boot/bootm.c
index 1f3a01994cbe..6ed60bf05084 100644
--- a/boot/bootm.c
+++ b/boot/bootm.c
@@ -222,12 +222,12 @@ static int boot_get_kernel(const char *cmd_name, const
char *addr_fit,
printf("## Booting Android Image at 0x%08lx ...\n", img_addr);
ret = android_image_get_kernel(boot_img, vendor_boot_img,
images->verify, os_data, os_len);
- if (ret)
- return ret;
if (IS_ENABLED(CONFIG_CMD_ABOOTIMG)) {
unmap_sysmem(vendor_boot_img);
unmap_sysmem(boot_img);
}
+ if (ret)
+ return ret;
break;
}
#endif
--
2.43.0.rc0.421.g78406f8d94-goog
[PATCH v2 13/32] bootm: Adjust boot_get_kernel() to return an error
This function obtains lots of error codes and then throws them away.
Update it to return the error, moving the image pointer to an
argument.
Signed-off-by: Simon Glass
Reviewed-by: Tom Rini
---
Changes in v2:
- Use the command table to provide the command name, instead of "bootm"
boot/bootm.c | 35 +--
1 file changed, 21 insertions(+), 14 deletions(-)
diff --git a/boot/bootm.c b/boot/bootm.c
index ea1575bd07eb..1f3a01994cbe 100644
--- a/boot/bootm.c
+++ b/boot/bootm.c
@@ -110,6 +110,7 @@ static struct legacy_img_hdr *image_get_kernel(ulong
img_addr, int verify)
* @os_len: pointer to a ulong variable, will hold os data length
* address and length, otherwise NULL
* pointer to image header if valid image was found, plus kernel start
+ * @kernp: image header if valid image was found, otherwise NULL
*
* boot_get_kernel() tries to find a kernel image, verifies its integrity
* and locates kernel data.
@@ -118,9 +119,9 @@ static struct legacy_img_hdr *image_get_kernel(ulong
img_addr, int verify)
* pointer to image header if valid image was found, plus kernel start
* address and length, otherwise NULL
*/
-static const void *boot_get_kernel(const char *cmd_name, const char *addr_fit,
- struct bootm_headers *images,
- ulong *os_data, ulong *os_len)
+static int boot_get_kernel(const char *cmd_name, const char *addr_fit,
+ struct bootm_headers *images, ulong *os_data,
+ ulong *os_len, const void **kernp)
{
#if CONFIG_IS_ENABLED(LEGACY_IMAGE_FORMAT)
struct legacy_img_hdr *hdr;
@@ -154,7 +155,7 @@ static const void *boot_get_kernel(const char *cmd_name,
const char *addr_fit,
img_addr);
hdr = image_get_kernel(img_addr, images->verify);
if (!hdr)
- return NULL;
+ return -EINVAL;
bootstage_mark(BOOTSTAGE_ID_CHECK_IMAGETYPE);
/* get os_data and os_len */
@@ -175,7 +176,7 @@ static const void *boot_get_kernel(const char *cmd_name,
const char *addr_fit,
printf("Wrong Image Type for %s command\n",
cmd_name);
bootstage_error(BOOTSTAGE_ID_CHECK_IMAGETYPE);
- return NULL;
+ return -EPROTOTYPE;
}
/*
@@ -200,7 +201,7 @@ static const void *boot_get_kernel(const char *cmd_name,
const char *addr_fit,
BOOTSTAGE_ID_FIT_KERNEL_START,
FIT_LOAD_IGNORED, os_data, os_len);
if (os_noffset < 0)
- return NULL;
+ return -ENOENT;
images->fit_hdr_os = map_sysmem(img_addr, 0);
images->fit_uname_os = fit_uname_kernel;
@@ -209,7 +210,9 @@ static const void *boot_get_kernel(const char *cmd_name,
const char *addr_fit,
break;
#endif
#ifdef CONFIG_ANDROID_BOOT_IMAGE
- case IMAGE_FORMAT_ANDROID:
+ case IMAGE_FORMAT_ANDROID: {
+ int ret;
+
boot_img = buf;
vendor_boot_img = NULL;
if (IS_ENABLED(CONFIG_CMD_ABOOTIMG)) {
@@ -217,25 +220,28 @@ static const void *boot_get_kernel(const char *cmd_name,
const char *addr_fit,
vendor_boot_img =
map_sysmem(get_avendor_bootimg_addr(), 0);
}
printf("## Booting Android Image at 0x%08lx ...\n", img_addr);
- if (android_image_get_kernel(boot_img, vendor_boot_img,
images->verify,
-os_data, os_len))
- return NULL;
+ ret = android_image_get_kernel(boot_img, vendor_boot_img,
+ images->verify, os_data, os_len);
+ if (ret)
+ return ret;
if (IS_ENABLED(CONFIG_CMD_ABOOTIMG)) {
unmap_sysmem(vendor_boot_img);
unmap_sysmem(boot_img);
}
break;
+ }
#endif
default:
printf("Wrong Image Format for %s command\n", cmd_name);
bootstage_error(BOOTSTAGE_ID_FIT_KERNEL_INFO);
- return NULL;
+ return -EBADF;
}
debug(" kernel data at 0x%08lx, len = 0x%08lx (%ld)\n",
*os_data, *os_len, *os_len);
+ *kernp = buf;
- return buf;
+ return 0;
}
#ifdef CONFIG_LMB
@@ -316,8 +322,9 @@ static int bootm_find_os(struct cmd_tbl *cmdtp, int flag,
int argc,
int ret;
/* get kernel image header, start address and length */
- os_hdr = boot_get_kernel(cmdtp->name, argv[0], ,
-
[PATCH v2 12/32] image: Document error codes from fit_image_load()
Put a list of these in the function documentation so it is easier to decode what went wrong. Signed-off-by: Simon Glass Reviewed-by: Tom Rini --- (no changes since v1) include/image.h | 8 +++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/include/image.h b/include/image.h index 798c5f9c16e4..d37e44721672 100644 --- a/include/image.h +++ b/include/image.h @@ -728,7 +728,13 @@ int boot_get_fdt_fit(struct bootm_headers *images, ulong addr, * @param load_op Decribes what to do with the load address * @param datapReturns address of loaded image * @param lenp Returns length of loaded image - * Return: node offset of image, or -ve error code on error + * Return: node offset of image, or -ve error code on error: + * -ENOEXEC - unsupported architecture + * -ENOENT - could not find image / subimage + * -EACCES - hash, signature or decryptions failure + * -EBADF - invalid OS or image type, or cannot get image load-address + * -EXDEV - memory overwritten / overlap + * -NOEXEC - image decompression error, or invalid FDT */ int fit_image_load(struct bootm_headers *images, ulong addr, const char **fit_unamep, const char **fit_uname_configp, -- 2.43.0.rc0.421.g78406f8d94-goog
[PATCH v2 11/32] bootm: Reduce arguments to boot_get_kernel()
This function only uses one argument and just needs to know the name of
the command which called it. Adjust the function to use only what it
needs. This will make it easier to call from a non-command context.
Tidy up the function comment while we are here.
Signed-off-by: Simon Glass
---
Changes in v2:
- Use the command table to provide the command name, instead of "bootm"
boot/bootm.c | 22 +-
1 file changed, 13 insertions(+), 9 deletions(-)
diff --git a/boot/bootm.c b/boot/bootm.c
index e323c8b758e9..ea1575bd07eb 100644
--- a/boot/bootm.c
+++ b/boot/bootm.c
@@ -102,9 +102,14 @@ static struct legacy_img_hdr *image_get_kernel(ulong
img_addr, int verify)
#endif
/**
- * boot_get_kernel - find kernel image
+ * boot_get_kernel() - find kernel image
+ *
+ * @cmd_name: Name of the command calling this function, e.g. "bootm"
+ * @addr_fit: first argument to bootm: address, fit configuration, etc.
* @os_data: pointer to a ulong variable, will hold os data start address
* @os_len: pointer to a ulong variable, will hold os data length
+ * address and length, otherwise NULL
+ * pointer to image header if valid image was found, plus kernel start
*
* boot_get_kernel() tries to find a kernel image, verifies its integrity
* and locates kernel data.
@@ -113,8 +118,8 @@ static struct legacy_img_hdr *image_get_kernel(ulong
img_addr, int verify)
* pointer to image header if valid image was found, plus kernel start
* address and length, otherwise NULL
*/
-static const void *boot_get_kernel(struct cmd_tbl *cmdtp, int flag, int argc,
- char *const argv[], struct bootm_headers
*images,
+static const void *boot_get_kernel(const char *cmd_name, const char *addr_fit,
+ struct bootm_headers *images,
ulong *os_data, ulong *os_len)
{
#if CONFIG_IS_ENABLED(LEGACY_IMAGE_FORMAT)
@@ -131,8 +136,7 @@ static const void *boot_get_kernel(struct cmd_tbl *cmdtp,
int flag, int argc,
const void *boot_img;
const void *vendor_boot_img;
#endif
- img_addr = genimg_get_kernel_addr_fit(argc < 1 ? NULL : argv[0],
- _uname_config,
+ img_addr = genimg_get_kernel_addr_fit(addr_fit, _uname_config,
_uname_kernel);
if (IS_ENABLED(CONFIG_CMD_BOOTM_PRE_LOAD))
@@ -169,7 +173,7 @@ static const void *boot_get_kernel(struct cmd_tbl *cmdtp,
int flag, int argc,
break;
default:
printf("Wrong Image Type for %s command\n",
- cmdtp->name);
+ cmd_name);
bootstage_error(BOOTSTAGE_ID_CHECK_IMAGETYPE);
return NULL;
}
@@ -223,7 +227,7 @@ static const void *boot_get_kernel(struct cmd_tbl *cmdtp,
int flag, int argc,
break;
#endif
default:
- printf("Wrong Image Format for %s command\n", cmdtp->name);
+ printf("Wrong Image Format for %s command\n", cmd_name);
bootstage_error(BOOTSTAGE_ID_FIT_KERNEL_INFO);
return NULL;
}
@@ -312,8 +316,8 @@ static int bootm_find_os(struct cmd_tbl *cmdtp, int flag,
int argc,
int ret;
/* get kernel image header, start address and length */
- os_hdr = boot_get_kernel(cmdtp, flag, argc, argv,
- , _start, _len);
+ os_hdr = boot_get_kernel(cmdtp->name, argv[0], ,
+_start, _len);
if (images.os.image_len == 0) {
puts("ERROR: can't get kernel image!\n");
return 1;
--
2.43.0.rc0.421.g78406f8d94-goog
[PATCH v2 10/32] image: Tidy up genimg_get_kernel_addr_fit()
This function does not modify its first argument, so mark it const. Also
move the comments to the header file and expand them to provide more
useful information.
Signed-off-by: Simon Glass
Reviewed-by: Tom Rini
---
(no changes since v1)
boot/bootm.c | 3 +--
boot/image-board.c | 17 +
include/image.h| 29 ++---
3 files changed, 28 insertions(+), 21 deletions(-)
diff --git a/boot/bootm.c b/boot/bootm.c
index 598f880d86dd..e323c8b758e9 100644
--- a/boot/bootm.c
+++ b/boot/bootm.c
@@ -122,8 +122,7 @@ static const void *boot_get_kernel(struct cmd_tbl *cmdtp,
int flag, int argc,
#endif
ulong img_addr;
const void *buf;
- const char *fit_uname_config = NULL;
- const char *fit_uname_kernel = NULL;
+ const char *fit_uname_config = NULL, *fit_uname_kernel = NULL;
#if CONFIG_IS_ENABLED(FIT)
int os_noffset;
#endif
diff --git a/boot/image-board.c b/boot/image-board.c
index d500da1b4b91..062c76badecc 100644
--- a/boot/image-board.c
+++ b/boot/image-board.c
@@ -198,22 +198,7 @@ void memmove_wd(void *to, void *from, size_t len, ulong
chunksz)
}
}
-/**
- * genimg_get_kernel_addr_fit - get the real kernel address and return 2
- * FIT strings
- * @img_addr: a string might contain real image address
- * @fit_uname_config: double pointer to a char, will hold pointer to a
- *configuration unit name
- * @fit_uname_kernel: double pointer to a char, will hold pointer to a subimage
- *name
- *
- * genimg_get_kernel_addr_fit get the real kernel start address from a string
- * which is normally the first argv of bootm/bootz
- *
- * returns:
- * kernel start address
- */
-ulong genimg_get_kernel_addr_fit(char * const img_addr,
+ulong genimg_get_kernel_addr_fit(const char *const img_addr,
const char **fit_uname_config,
const char **fit_uname_kernel)
{
diff --git a/include/image.h b/include/image.h
index 2e3cf839ee36..798c5f9c16e4 100644
--- a/include/image.h
+++ b/include/image.h
@@ -612,9 +612,32 @@ int boot_get_setup(struct bootm_headers *images, uint8_t
arch, ulong *setup_star
#define IMAGE_FORMAT_FIT 0x02/* new, libfdt based format */
#define IMAGE_FORMAT_ANDROID 0x03/* Android boot image */
-ulong genimg_get_kernel_addr_fit(char * const img_addr,
-const char **fit_uname_config,
-const char **fit_uname_kernel);
+/**
+ * genimg_get_kernel_addr_fit() - Parse FIT specifier
+ *
+ * Get the real kernel start address from a string which is normally the first
+ * argv of bootm/bootz
+ *
+ * These cases are dealt with, based on the value of @img_addr:
+ *NULL: Returns image_load_addr, does not set last two args
+ *"": Returns address
+ *
+ * For FIT:
+ *"[]#": Returns address (or image_load_addr),
+ * sets fit_uname_config to config name
+ *"[]:": Returns address (or image_load_addr) and sets
+ * fit_uname_kernel to the subimage name
+ *
+ * @img_addr: a string might contain real image address (or NULL)
+ * @fit_uname_config: Returns configuration unit name
+ * @fit_uname_kernel: Returns subimage name
+ *
+ * Returns: kernel start address
+ */
+ulong genimg_get_kernel_addr_fit(const char *const img_addr,
+const char **fit_uname_config,
+const char **fit_uname_kernel);
+
ulong genimg_get_kernel_addr(char * const img_addr);
int genimg_get_format(const void *img_addr);
int genimg_has_config(struct bootm_headers *images);
--
2.43.0.rc0.421.g78406f8d94-goog
[PATCH v2 09/32] bootm: Move boot_get_kernel() higher in the file
Move this code and image_get_kernel() higher in the file to avoid the
need for a forward declaration.
No attempt is made to remove #ifdefs or adjust the code in any other
way.
Signed-off-by: Simon Glass
Reviewed-by: Tom Rini
---
(no changes since v1)
boot/bootm.c | 376 +--
1 file changed, 186 insertions(+), 190 deletions(-)
diff --git a/boot/bootm.c b/boot/bootm.c
index 1482a2cc783f..598f880d86dd 100644
--- a/boot/bootm.c
+++ b/boot/bootm.c
@@ -44,12 +44,195 @@ DECLARE_GLOBAL_DATA_PTR;
struct bootm_headers images; /* pointers to os/initrd/fdt images */
+__weak void board_quiesce_devices(void)
+{
+}
+
+#if CONFIG_IS_ENABLED(LEGACY_IMAGE_FORMAT)
+/**
+ * image_get_kernel - verify legacy format kernel image
+ * @img_addr: in RAM address of the legacy format image to be verified
+ * @verify: data CRC verification flag
+ *
+ * image_get_kernel() verifies legacy image integrity and returns pointer to
+ * legacy image header if image verification was completed successfully.
+ *
+ * returns:
+ * pointer to a legacy image header if valid image was found
+ * otherwise return NULL
+ */
+static struct legacy_img_hdr *image_get_kernel(ulong img_addr, int verify)
+{
+ struct legacy_img_hdr *hdr = (struct legacy_img_hdr *)img_addr;
+
+ if (!image_check_magic(hdr)) {
+ puts("Bad Magic Number\n");
+ bootstage_error(BOOTSTAGE_ID_CHECK_MAGIC);
+ return NULL;
+ }
+ bootstage_mark(BOOTSTAGE_ID_CHECK_HEADER);
+
+ if (!image_check_hcrc(hdr)) {
+ puts("Bad Header Checksum\n");
+ bootstage_error(BOOTSTAGE_ID_CHECK_HEADER);
+ return NULL;
+ }
+
+ bootstage_mark(BOOTSTAGE_ID_CHECK_CHECKSUM);
+ image_print_contents(hdr);
+
+ if (verify) {
+ puts(" Verifying Checksum ... ");
+ if (!image_check_dcrc(hdr)) {
+ printf("Bad Data CRC\n");
+ bootstage_error(BOOTSTAGE_ID_CHECK_CHECKSUM);
+ return NULL;
+ }
+ puts("OK\n");
+ }
+ bootstage_mark(BOOTSTAGE_ID_CHECK_ARCH);
+
+ if (!image_check_target_arch(hdr)) {
+ printf("Unsupported Architecture 0x%x\n", image_get_arch(hdr));
+ bootstage_error(BOOTSTAGE_ID_CHECK_ARCH);
+ return NULL;
+ }
+ return hdr;
+}
+#endif
+
+/**
+ * boot_get_kernel - find kernel image
+ * @os_data: pointer to a ulong variable, will hold os data start address
+ * @os_len: pointer to a ulong variable, will hold os data length
+ *
+ * boot_get_kernel() tries to find a kernel image, verifies its integrity
+ * and locates kernel data.
+ *
+ * returns:
+ * pointer to image header if valid image was found, plus kernel start
+ * address and length, otherwise NULL
+ */
static const void *boot_get_kernel(struct cmd_tbl *cmdtp, int flag, int argc,
char *const argv[], struct bootm_headers
*images,
- ulong *os_data, ulong *os_len);
-
-__weak void board_quiesce_devices(void)
+ ulong *os_data, ulong *os_len)
{
+#if CONFIG_IS_ENABLED(LEGACY_IMAGE_FORMAT)
+ struct legacy_img_hdr *hdr;
+#endif
+ ulong img_addr;
+ const void *buf;
+ const char *fit_uname_config = NULL;
+ const char *fit_uname_kernel = NULL;
+#if CONFIG_IS_ENABLED(FIT)
+ int os_noffset;
+#endif
+
+#ifdef CONFIG_ANDROID_BOOT_IMAGE
+ const void *boot_img;
+ const void *vendor_boot_img;
+#endif
+ img_addr = genimg_get_kernel_addr_fit(argc < 1 ? NULL : argv[0],
+ _uname_config,
+ _uname_kernel);
+
+ if (IS_ENABLED(CONFIG_CMD_BOOTM_PRE_LOAD))
+ img_addr += image_load_offset;
+
+ bootstage_mark(BOOTSTAGE_ID_CHECK_MAGIC);
+
+ /* check image type, for FIT images get FIT kernel node */
+ *os_data = *os_len = 0;
+ buf = map_sysmem(img_addr, 0);
+ switch (genimg_get_format(buf)) {
+#if CONFIG_IS_ENABLED(LEGACY_IMAGE_FORMAT)
+ case IMAGE_FORMAT_LEGACY:
+ printf("## Booting kernel from Legacy Image at %08lx ...\n",
+ img_addr);
+ hdr = image_get_kernel(img_addr, images->verify);
+ if (!hdr)
+ return NULL;
+ bootstage_mark(BOOTSTAGE_ID_CHECK_IMAGETYPE);
+
+ /* get os_data and os_len */
+ switch (image_get_type(hdr)) {
+ case IH_TYPE_KERNEL:
+ case IH_TYPE_KERNEL_NOLOAD:
+ *os_data = image_get_data(hdr);
+ *os_len = image_get_data_size(hdr);
+ break;
+
[PATCH v2 08/32] bootm: Simplify arguments for bootm_pre_load()
Move the argument decoding to the caller, to avoid needing to pass the
command-line arguments.
Add a function comment while we are here.
Signed-off-by: Simon Glass
Reviewed-by: Tom Rini
---
(no changes since v1)
boot/bootm.c | 23 ---
1 file changed, 16 insertions(+), 7 deletions(-)
diff --git a/boot/bootm.c b/boot/bootm.c
index fda97706fc26..1482a2cc783f 100644
--- a/boot/bootm.c
+++ b/boot/bootm.c
@@ -82,22 +82,31 @@ static int bootm_start(void)
return 0;
}
-static ulong bootm_data_addr(int argc, char *const argv[])
+static ulong bootm_data_addr(const char *addr_str)
{
ulong addr;
- if (argc > 0)
- addr = simple_strtoul(argv[0], NULL, 16);
+ if (addr_str)
+ addr = hextoul(addr_str, NULL);
else
addr = image_load_addr;
return addr;
}
-static int bootm_pre_load(struct cmd_tbl *cmdtp, int flag, int argc,
- char *const argv[])
+/**
+ * bootm_pre_load() - Handle the pre-load processing
+ *
+ * This can be used to do a full signature check of the image, for example.
+ * It calls image_pre_load() with the data address of the image to check.
+ *
+ * @addr_str: String containing load address in hex, or NULL to use
+ * image_load_addr
+ * Return: 0 if OK, CMD_RET_FAILURE on failure
+ */
+static int bootm_pre_load(const char *addr_str)
{
- ulong data_addr = bootm_data_addr(argc, argv);
+ ulong data_addr = bootm_data_addr(addr_str);
int ret = 0;
if (IS_ENABLED(CONFIG_CMD_BOOTM_PRE_LOAD))
@@ -785,7 +794,7 @@ int do_bootm_states(struct cmd_tbl *cmdtp, int flag, int
argc,
ret = bootm_start();
if (!ret && (states & BOOTM_STATE_PRE_LOAD))
- ret = bootm_pre_load(cmdtp, flag, argc, argv);
+ ret = bootm_pre_load(argv[0]);
if (!ret && (states & BOOTM_STATE_FINDOS))
ret = bootm_find_os(cmdtp, flag, argc, argv);
--
2.43.0.rc0.421.g78406f8d94-goog
[PATCH v2 07/32] bootm: Drop arguments from bootm_start()
This function does not use its arguments. Drop them.
Signed-off-by: Simon Glass
Reviewed-by: Tom Rini
---
(no changes since v1)
boot/bootm.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/boot/bootm.c b/boot/bootm.c
index cb61485c226c..fda97706fc26 100644
--- a/boot/bootm.c
+++ b/boot/bootm.c
@@ -69,8 +69,7 @@ static void boot_start_lmb(struct bootm_headers *images)
static inline void boot_start_lmb(struct bootm_headers *images) { }
#endif
-static int bootm_start(struct cmd_tbl *cmdtp, int flag, int argc,
- char *const argv[])
+static int bootm_start(void)
{
memset((void *), 0, sizeof(images));
images.verify = env_get_yesno("verify");
@@ -783,7 +782,7 @@ int do_bootm_states(struct cmd_tbl *cmdtp, int flag, int
argc,
* any error.
*/
if (states & BOOTM_STATE_START)
- ret = bootm_start(cmdtp, flag, argc, argv);
+ ret = bootm_start();
if (!ret && (states & BOOTM_STATE_PRE_LOAD))
ret = bootm_pre_load(cmdtp, flag, argc, argv);
--
2.43.0.rc0.421.g78406f8d94-goog
[PATCH v2 06/32] bootstd: Introduce programmable boot
At present bootstd requires CONFIG_CMDLINE to operate. Add a new
'programmable' boot which can be used when no command line is available.
For now it does almost nothing, since most bootmeths require the
command line.
Signed-off-by: Simon Glass
---
(no changes since v1)
boot/Kconfig | 11 ++
boot/Makefile | 2 ++
boot/prog_boot.c | 51 +++
common/main.c | 9 +
include/bootstd.h | 9 +
5 files changed, 82 insertions(+)
create mode 100644 boot/prog_boot.c
diff --git a/boot/Kconfig b/boot/Kconfig
index ef71883a5026..586d7a69fab3 100644
--- a/boot/Kconfig
+++ b/boot/Kconfig
@@ -459,6 +459,17 @@ config BOOTSTD_BOOTCOMMAND
standard boot does not support all of the features of distro boot
yet.
+config BOOTSTD_PROG
+ bool "Use programmatic boot"
+ depends on !CMDLINE
+ default y
+ help
+ Enable this to provide a board_run_command() function which can boot
+ a systen without using commands.
+
+ Note: This currently has many limitations and is not a useful booting
+ solution. Future work will eventually make this a viable option.
+
config BOOTMETH_GLOBAL
bool
help
diff --git a/boot/Makefile b/boot/Makefile
index 3fd048bb41ab..de0eafed14b1 100644
--- a/boot/Makefile
+++ b/boot/Makefile
@@ -25,6 +25,8 @@ obj-$(CONFIG_$(SPL_TPL_)BOOTSTD) += bootflow.o
obj-$(CONFIG_$(SPL_TPL_)BOOTSTD) += bootmeth-uclass.o
obj-$(CONFIG_$(SPL_TPL_)BOOTSTD) += bootstd-uclass.o
+obj-$(CONFIG_$(SPL_TPL_)BOOTSTD_PROG) += prog_boot.o
+
obj-$(CONFIG_$(SPL_TPL_)BOOTMETH_EXTLINUX) += bootmeth_extlinux.o
obj-$(CONFIG_$(SPL_TPL_)BOOTMETH_EXTLINUX_PXE) += bootmeth_pxe.o
obj-$(CONFIG_$(SPL_TPL_)BOOTMETH_EFILOADER) += bootmeth_efi.o
diff --git a/boot/prog_boot.c b/boot/prog_boot.c
new file mode 100644
index ..045554b93db0
--- /dev/null
+++ b/boot/prog_boot.c
@@ -0,0 +1,51 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2021 Google LLC
+ * Written by Simon Glass
+ */
+
+#define LOG_CATEGORY UCLASS_BOOTSTD
+
+#include
+#include
+#include
+#include
+
+/*
+ * show_bootmeths() - List available bootmeths
+ *
+ * We could refactor this to use do_bootmeth_list() if more detail (or
ordering)
+ * are needed
+ */
+static void show_bootmeths(void)
+{
+ struct udevice *dev;
+ struct uclass *uc;
+
+ printf("Bootmeths: ");
+ uclass_id_foreach_dev(UCLASS_BOOTMETH, dev, uc)
+ printf(" %s", dev->name);
+ printf("\n");
+}
+
+int bootstd_prog_boot(void)
+{
+ struct bootflow_iter iter;
+ struct bootflow bflow;
+ int ret, flags, i;
+
+ printf("Programmatic boot starting\n");
+ show_bootmeths();
+ flags = BOOTFLOWIF_HUNT | BOOTFLOWIF_SHOW | BOOTFLOWIF_SKIP_GLOBAL;
+
+ bootstd_clear_glob();
+ for (i = 0, ret = bootflow_scan_first(NULL, NULL, , flags, );
+i < 1000 && ret != -ENODEV;
+i++, ret = bootflow_scan_next(, )) {
+ if (!bflow.err)
+ bootflow_run_boot(, );
+ bootflow_free();
+ }
+
+ return -EFAULT;
+}
diff --git a/common/main.c b/common/main.c
index 7c70de2e59a8..2668f3bb1637 100644
--- a/common/main.c
+++ b/common/main.c
@@ -9,6 +9,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -67,6 +68,14 @@ void main_loop(void)
autoboot_command(s);
+ if (IS_ENABLED(CONFIG_BOOTSTD_PROG)) {
+ int ret;
+
+ ret = bootstd_prog_boot();
+ printf("Standard boot failed (err=%dE)\n", ret);
+ }
+
cli_loop();
+
panic("No CLI available");
}
diff --git a/include/bootstd.h b/include/bootstd.h
index 7802564bcc63..99ce7b64e7c5 100644
--- a/include/bootstd.h
+++ b/include/bootstd.h
@@ -94,4 +94,13 @@ int bootstd_get_priv(struct bootstd_priv **stdp);
*/
void bootstd_clear_glob(void);
+/**
+ * bootstd_prog_boot() - Run standard boot in a fully programmatic mode
+ *
+ * Attempts to boot without making any use of U-Boot commands
+ *
+ * Returns: -ve error value (does not return except on failure to boot)
+ */
+int bootstd_prog_boot(void);
+
#endif
--
2.43.0.rc0.421.g78406f8d94-goog
[PATCH v2 05/32] bootstd: Add missing header file from bootdev.h
Add a dm/uclass-id.h to the bootdev header file, since it uses enum uclass_id Signed-off-by: Simon Glass --- (no changes since v1) include/bootdev.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/bootdev.h b/include/bootdev.h index b079a91b5b7f..c1362673d418 100644 --- a/include/bootdev.h +++ b/include/bootdev.h @@ -7,6 +7,7 @@ #ifndef __bootdev_h #define __bootdev_h +#include #include struct bootflow; -- 2.43.0.rc0.421.g78406f8d94-goog
[PATCH v2 04/32] treewide: Tidy up semicolon after command macros
The U_BOOT_CMD_COMPLETE() macro has a semicolon at the end, perhaps inadvertently. Some code has taken advantage of this. Tidy this up by dropping the semicolon from the macro and adding it to macro invocations as required. Signed-off-by: Simon Glass Reviewed-by: Tom Rini --- (no changes since v1) board/freescale/common/vid.c | 2 +- board/xilinx/common/fru.c| 2 +- board/xilinx/versal/cmds.c | 2 +- board/xilinx/zynqmp/cmds.c | 2 +- cmd/btrfs.c | 2 +- cmd/eeprom.c | 2 +- cmd/ext2.c | 4 ++-- cmd/fs.c | 8 cmd/pinmux.c | 2 +- cmd/qfw.c| 2 +- include/command.h| 2 +- 11 files changed, 15 insertions(+), 15 deletions(-) diff --git a/board/freescale/common/vid.c b/board/freescale/common/vid.c index 5ec3f2a76b19..fc5d400cfe18 100644 --- a/board/freescale/common/vid.c +++ b/board/freescale/common/vid.c @@ -793,4 +793,4 @@ U_BOOT_CMD( vdd_read, 1, 0, do_vdd_read, "read VDD", " - Read the voltage specified in mV" -) +); diff --git a/board/xilinx/common/fru.c b/board/xilinx/common/fru.c index c916c3d6b4c8..12b21317496a 100644 --- a/board/xilinx/common/fru.c +++ b/board/xilinx/common/fru.c @@ -85,4 +85,4 @@ U_BOOT_CMD( fru, 8, 1, do_fru, "FRU table info", fru_help_text -) +); diff --git a/board/xilinx/versal/cmds.c b/board/xilinx/versal/cmds.c index 9cc2cdcebf1c..2a74e49aedec 100644 --- a/board/xilinx/versal/cmds.c +++ b/board/xilinx/versal/cmds.c @@ -98,4 +98,4 @@ U_BOOT_LONGHELP(versal, U_BOOT_CMD(versal, 4, 1, do_versal, "versal sub-system", versal_help_text -) +); diff --git a/board/xilinx/zynqmp/cmds.c b/board/xilinx/zynqmp/cmds.c index f1f3eff501e1..9524688f27d9 100644 --- a/board/xilinx/zynqmp/cmds.c +++ b/board/xilinx/zynqmp/cmds.c @@ -427,4 +427,4 @@ U_BOOT_CMD( zynqmp, 9, 1, do_zynqmp, "ZynqMP sub-system", zynqmp_help_text -) +); diff --git a/cmd/btrfs.c b/cmd/btrfs.c index 98daea99e9ed..2843835d08b8 100644 --- a/cmd/btrfs.c +++ b/cmd/btrfs.c @@ -24,4 +24,4 @@ U_BOOT_CMD(btrsubvol, 3, 1, do_btrsubvol, "list subvolumes of a BTRFS filesystem", " \n" " - List subvolumes of a BTRFS filesystem." -) +); diff --git a/cmd/eeprom.c b/cmd/eeprom.c index 0b6ca8c505fb..322765ad02a0 100644 --- a/cmd/eeprom.c +++ b/cmd/eeprom.c @@ -435,4 +435,4 @@ U_BOOT_CMD( "The values which can be provided with the -l option are:\n" CONFIG_EEPROM_LAYOUT_HELP_STRING"\n" #endif -) +); diff --git a/cmd/ext2.c b/cmd/ext2.c index 57a99516a6ac..a0ce0cf5796b 100644 --- a/cmd/ext2.c +++ b/cmd/ext2.c @@ -42,7 +42,7 @@ U_BOOT_CMD( "list files in a directory (default /)", " [directory]\n" "- list files from 'dev' on 'interface' in a 'directory'" -) +); U_BOOT_CMD( ext2load, 6, 0, do_ext2load, @@ -50,4 +50,4 @@ U_BOOT_CMD( " [ [addr [filename [bytes [pos]\n" "- load binary file 'filename' from 'dev' on 'interface'\n" " to address 'addr' from ext2 filesystem." -) +); diff --git a/cmd/fs.c b/cmd/fs.c index 6044f73af5b4..46cb43dcdb5b 100644 --- a/cmd/fs.c +++ b/cmd/fs.c @@ -39,7 +39,7 @@ U_BOOT_CMD( " If 'bytes' is 0 or omitted, the file is read until the end.\n" " 'pos' gives the file byte position to start reading from.\n" " If 'pos' is 0 or omitted, the file is read from the start." -) +); static int do_save_wrapper(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) @@ -56,7 +56,7 @@ U_BOOT_CMD( " 'bytes' gives the size to save in bytes and is mandatory.\n" " 'pos' gives the file byte position to start writing to.\n" " If 'pos' is 0 or omitted, the file is written from the start." -) +); static int do_ls_wrapper(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) @@ -70,7 +70,7 @@ U_BOOT_CMD( " [ [directory]]\n" "- List files in directory 'directory' of partition 'part' on\n" " device type 'interface' instance 'dev'." -) +); static int do_ln_wrapper(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) @@ -84,7 +84,7 @@ U_BOOT_CMD( " target linkname\n" "- create a symbolic link to 'target' with the name 'linkname' on\n" " device type 'interface' instance 'dev'." -) +); static int do_fstype_wrapper(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) diff --git a/cmd/pinmux.c b/cmd/pinmux
[PATCH v2 03/32] mmc: env: Unify the U_BOOT_ENV_LOCATION conditions
The declaration of U_BOOT_ENV_LOCATION() uses a different #ifdef
condition from the code it calls. Use the same condition to avoid a
build warning if CONFIG_CMD_SAVEENV is disabled.
Signed-off-by: Simon Glass
---
(no changes since v1)
env/mmc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/env/mmc.c b/env/mmc.c
index cb14bbb58f13..da84cddd74f0 100644
--- a/env/mmc.c
+++ b/env/mmc.c
@@ -495,7 +495,7 @@ U_BOOT_ENV_LOCATION(mmc) = {
.location = ENVL_MMC,
ENV_NAME("MMC")
.load = env_mmc_load,
-#ifndef CONFIG_SPL_BUILD
+#if defined(CONFIG_CMD_SAVEENV) && !defined(CONFIG_SPL_BUILD)
.save = env_save_ptr(env_mmc_save),
.erase = ENV_ERASE_PTR(env_mmc_erase)
#endif
--
2.43.0.rc0.421.g78406f8d94-goog
[PATCH v2 02/32] README: Correct docs for CONFIG_SPL_BUILD
This option is defined in both SPL and TPL builds, so correct the docs related to this. Also point to spl_phase() which is normally a better option. Mention VPL as well. Signed-off-by: Simon Glass Reported-by: Heinrich Schuchardt --- (no changes since v1) README | 26 ++ 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/README b/README index 60c6b8a19db1..f26c34a5e200 100644 --- a/README +++ b/README @@ -1545,16 +1545,26 @@ Low Level (hardware related) configuration options: globally (CONFIG_CMD_MEMORY). - CONFIG_SPL_BUILD - Set when the currently-running compilation is for an artifact - that will end up in the SPL (as opposed to the TPL or U-Boot - proper). Code that needs stage-specific behavior should check - this. + Set when the currently running compilation is for an artifact + that will end up in one of the 'xPL' builds, i.e. SPL, TPL or + VPL. Code that needs phase-specific behaviour can check this, + or (where possible) use spl_phase() instead. + + Note that CONFIG_SPL_BUILD *is* always defined when either + of CONFIG_TPL_BUILD / CONFIG_VPL_BUILD is defined. This can be + counter-intuitive and should perhaps be changed. - CONFIG_TPL_BUILD - Set when the currently-running compilation is for an artifact - that will end up in the TPL (as opposed to the SPL or U-Boot - proper). Code that needs stage-specific behavior should check - this. + Set when the currently running compilation is for an artifact + that will end up in the TPL build (as opposed to SPL, VPL or + U-Boot proper). Code that needs phase-specific behaviour can + check this, or (where possible) use spl_phase() instead. + +- CONFIG_VPL_BUILD + Set when the currently running compilation is for an artifact + that will end up in the VPL build (as opposed to the SPL, TPL + or U-Boot proper). Code that needs phase-specific behaviour can + check this, or (where possible) use spl_phase() instead. - CONFIG_ARCH_MAP_SYSMEM Generally U-Boot (and in particular the md command) uses -- 2.43.0.rc0.421.g78406f8d94-goog
[PATCH v2 01/32] arm: x86: Drop discarding of command linker-lists
Since we can now cleanly disable CMDLINE when needed, drop the rules
which discard the command code. It will not be built in the first
place.
Signed-off-by: Simon Glass
Reviewed-by: Tom Rini
---
(no changes since v1)
arch/arm/cpu/u-boot.lds | 3 ---
arch/x86/cpu/u-boot-64.lds | 4
arch/x86/cpu/u-boot-spl.lds | 4
arch/x86/cpu/u-boot.lds | 4
4 files changed, 15 deletions(-)
diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
index fc4f63d83489..7724c9332c3b 100644
--- a/arch/arm/cpu/u-boot.lds
+++ b/arch/arm/cpu/u-boot.lds
@@ -14,9 +14,6 @@ OUTPUT_ARCH(arm)
ENTRY(_start)
SECTIONS
{
-#ifndef CONFIG_CMDLINE
- /DISCARD/ : { *(__u_boot_list_2_cmd_*) }
-#endif
#if defined(CONFIG_ARMV7_SECURE_BASE) && defined(CONFIG_ARMV7_NONSEC)
/*
* If CONFIG_ARMV7_SECURE_BASE is true, secure code will not
diff --git a/arch/x86/cpu/u-boot-64.lds b/arch/x86/cpu/u-boot-64.lds
index d0398ff00d71..00a6d8691211 100644
--- a/arch/x86/cpu/u-boot-64.lds
+++ b/arch/x86/cpu/u-boot-64.lds
@@ -11,10 +11,6 @@ ENTRY(_start)
SECTIONS
{
-#ifndef CONFIG_CMDLINE
- /DISCARD/ : { *(__u_boot_list_2_cmd_*) }
-#endif
-
#ifdef CONFIG_TEXT_BASE
. = CONFIG_TEXT_BASE; /* Location of bootcode in flash */
#endif
diff --git a/arch/x86/cpu/u-boot-spl.lds b/arch/x86/cpu/u-boot-spl.lds
index a0a2a06a18cd..50b4b1608552 100644
--- a/arch/x86/cpu/u-boot-spl.lds
+++ b/arch/x86/cpu/u-boot-spl.lds
@@ -11,10 +11,6 @@ ENTRY(_start)
SECTIONS
{
-#ifndef CONFIG_CMDLINE
- /DISCARD/ : { *(__u_boot_list_2_cmd_*) }
-#endif
-
. = IMAGE_TEXT_BASE;/* Location of bootcode in flash */
__text_start = .;
.text : {
diff --git a/arch/x86/cpu/u-boot.lds b/arch/x86/cpu/u-boot.lds
index a31f4220a000..c418ff44aa08 100644
--- a/arch/x86/cpu/u-boot.lds
+++ b/arch/x86/cpu/u-boot.lds
@@ -11,10 +11,6 @@ ENTRY(_start)
SECTIONS
{
-#ifndef CONFIG_CMDLINE
- /DISCARD/ : { *(__u_boot_list_2_cmd_*) }
-#endif
-
. = CONFIG_TEXT_BASE; /* Location of bootcode in flash */
__text_start = .;
--
2.43.0.rc0.421.g78406f8d94-goog
Re: [PATCH] bootstd: Avoid freeing a non-allocated buffer
Hi Heinrich, On Wed, 15 Nov 2023 at 19:18, Heinrich Schuchardt wrote: > > On 11/16/23 02:35, Simon Glass wrote: > > EFI applications can be very large and thus used to cause boot failures > > when malloc() space was exhausted. > > > > A recent changed fixed this by using the kernel_addr_r environment var > > as the address of the buffer. However, it still frees the buffer when > > the bootflow is discarded. > > > > Fix this by introducing a flag to indicate whether the buffer was > > allocated, or not. > > > > Note that kernel_addr_r is not the last word here. It might be better > > to use lmb to place images. But there is a lot of refactoring to do > > We need a discussion about memory management. We currently have: > > * malloc > * EFI - AllocatePages(), AllocatePool() > * lmb > * preassigned addresses like $kernel_addr_r, $fdt_addr_r Yes indeed. There have been discussions already, but we need to do this on the ML. > > EFI manages memory on page level. It needs to provide a complete memory > map to the OS with flags for different types of memory. Currently it is > not aware of where files are loaded and might allocate those regions for > its own usage. > > lmb does not track any allocations but tries to recreate a memory map on > the fly whenever a file is loaded. > > It would be preferable to allocate memory for files and require to > explicitly unload a file before reusing the same memory area. > > We should start a separate thread on this topic. Indeed. If you start it, I will reply with some thoughts. The nice thing is that with bootstd we can start to tidy all this up and make sure it has tests. > > > before we can remove the environment variables. The distro scripts rely > > on them so it is safe for bootstd to do so too. > > > > Fixes: 6a8c2f9781c bootstd: Avoid allocating memory for the EFI file > > > > Signed-off-by: Simon Glass > > Reported by: Simon Glass > > Reported by: Shantur Rathore > > Reviewed-by: Heinrich Schuchardt > [..] Regards, Simon
Re: [PATCH v4 05/12] usb: Avoid unbinding devices in use by bootflows
Hi Heinrich, On Wed, 15 Nov 2023 at 19:06, Heinrich Schuchardt wrote: > > On 11/16/23 02:42, Simon Glass wrote: > > Hi Heinrich, > > > > On Wed, 15 Nov 2023 at 18:34, Heinrich Schuchardt > > wrote: > >> > >> On 11/15/23 17:23, Heinrich Schuchardt wrote: > >>> On 11/15/23 16:50, Simon Glass wrote: > >>>> Hi Heinrich, > >>>> > >>>> On Sun, 12 Nov 2023 at 16:35, Heinrich Schuchardt > >>>> wrote: > >>>>> > >>>>> > >>>>> > >>>>> Am 12. November 2023 22:20:50 MEZ schrieb Simon Glass > >>>>> : > >>>>>> Hi Heinrich, > >>>>>> > >>>>>> On Sun, 12 Nov 2023 at 13:32, Heinrich Schuchardt > >>>>>> wrote: > >>>>>>> > >>>>>>> > >>>>>>> > >>>>>>> Am 12. November 2023 21:02:42 MEZ schrieb Simon Glass > >>>>>>> : > >>>>>>>> When a USB device is unbound, it causes any bootflows attached to > >>>>>>>> it to > >>>>>>>> be removed, via a call to bootdev_clear_bootflows() from > >>>>>>>> bootdev_pre_unbind(). This obviously makes it impossible to boot the > >>>>>>>> bootflow. > >>>>>>>> > >>>>>>>> However, when booting a bootflow that relies on USB, usb_stop() is > >>>>>>>> called, which unbinds the device. For EFI, this happens in > >>>>>>>> efi_exit_boot_services() which means that the bootflow disappears > >>>>>>>> before it is finished with. > >>>>>>> > >>>>>>> After ExitBootServices() no driver of U-Boot can be used as the > >>>>>>> operating system is in charge. > >>>>>>> > >>>>>>> Any bootflow inside U-Boot is terminated by definition when > >>>>>>> reaching ExitBootServices. > >>>>>>> > >>>>>>>> > >>>>>>>> There is no need to unbind all the USB devices just to quiesce them. > >>>>>>>> Add a new usb_pause() call which removes them but leaves them bound. > >>>>>>> > >>>>>>> As U-Boot must not access any device after ExitBootServices() it is > >>>>>>> unclear to me what you want to achieve. > >>>>>> > >>>>>> I can't remember exactly what is needed from the bootflow, but > >>>>>> something is. Perhaps the kernel, or the cmdline, or fdt? It would > >>>>>> have been better if I had mentioned this explicitly, But then this > >>>>>> patch has been sitting around for ages... > >>>>>> > >>>>>> In any case, the intent of exit-boot-services is not to free all the > >>>>>> memory used, since some of it may have been used to hold data that the > >>>>>> app continues to use. > >>>>> > >>>>> The EFI application reads the memory map and receives an ID which it > >>>>> passes to ExitBootServiced() after this point any memory not marked > >>>>> as EFI runtime can be used by the EFI app. This includes the memory > >>>>> that harbors the U-Boot USB drivers. Therefore no drivers can be used > >>>>> here. > >>>>> > >>>>> Starting the EFI app via StartImage() must terminate any running > >>>>> U-Boot "boot flow". > >>>>> > >>>>> After ExitBootServices() the EFI application cannot return to U-Boot > >>>>> except for SetVirtualAdressMspsetting which relocates the EFI runtime. > >>>>> > >>>>> Bootflows and U-Boot drivers have no meaning after ExitBootServices(). > >>>>> > >>>>> > >>>>> > >>>>>> > >>>>>> Also there is U-Boot code between when the devices are unbound and > >>>>>> when U-Boot actually exits back to the app. > >>>>>> > >>>>>> This hang was 100% repeatable on brya (an x86 Chromebook) and it took > >>>>>> quite a while to find. > >>>>> > >>>>> We need a better understanding of the problem
Re: [PATCH 27/29] bootm: Adjust the parameters of bootm_find_images()
Hi Tom,
On Wed, 15 Nov 2023 at 19:07, Tom Rini wrote:
>
> On Wed, Nov 15, 2023 at 06:56:33PM -0700, Simon Glass wrote:
> > Hi Tom,
> >
> > On Wed, 15 Nov 2023 at 18:47, Tom Rini wrote:
> > >
> > > On Wed, Nov 15, 2023 at 06:42:19PM -0700, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Wed, 15 Nov 2023 at 15:38, Tom Rini wrote:
> > > > >
> > > > > On Sat, Nov 11, 2023 at 05:09:12PM -0700, Simon Glass wrote:
> > > > >
> > > > > > Rather than passing it all the command-line args, pass in the pieces
> > > > > > that it needs. These are the image address, the ramdisk address/name
> > > > > > and the FDT address/name.
> > > > > >
> > > > > > Ultimately this will allow usage of this function without being
> > > > > > called
> > > > > > from the command line.
> > > > >
> > > > > OK, so this goal is good.
> > > > >
> > > > > [snip]
> > > > > > + return bootm_find_images(img_addr, argc > 1 ? argv[1]
> > > > > > : NULL,
> > > > > > + argc > 2 ? argv[2] : NULL,
> > > > > > 0, 0);
> > > > >
> > > > > That we repeat this much harder to read test/if/else three times now
> > > > > is
> > > > > less good. Can we find some way to hide the complexity here in the
> > > > > case
> > > > > where it's coming from a command and so we have argc/argv[] ?
> > > >
> > > > I can't really think of one. Ultimately this is coming from the fact
> > > > that the booti and bootz commands directly call bootm_find_images(). I
> > > > haven't got far enough to know whether that will still be true in the
> > > > end, but I hope not.
> > > >
> > > > IMO the correct place for the logic above is in the command-processing
> > > > code, where it decides which arg means what.
> > > >
> > > > I could imagine something like:
> > > >
> > > > static const char *cmd_get_arg(int argc, char *const argv[], int argnum)
> > > > {
> > > >return argc > argnum ? argv[argnum] : NULL;
> > > > }
> > > >
> > > > but I'm not sure that is an improvement.
> > >
> > > I was thinking about this more after I sent this. And I think we might
> > > indeed want an inline / macro to handle this case more generally as I
> > > suspect we'll have similar cases where we need argN-or-NULL as we
> > > refactor other areas of code to split "here is the command" from "here
> > > is the library functionality" of it. Then we'll have:
> > > ulong foo = cmd_arg_one(argc, argv);
> > > ulong bar = cmd_arg_two(argc, argv);
> > >
> > > librarycall(foo, bar, ...);
> >
> > OK I can try something. But note that the one, two terminology becomes
> > confusing. Is the first argument zero or one? Also we often drop an
> > argument in a subcommand to allow things to start from 0 again.
> >
> > It might be better to use first and second, instead?
>
> Yes, please come up with a better naming scheme, I'm terrible about
> stuff like that.
Actually my naming has problems too...hopefully this has been solved
properly elsewhere and someone will reply to my patch.
Regards,
Simon
Re: [PATCH 27/29] bootm: Adjust the parameters of bootm_find_images()
Hi Tom,
On Wed, 15 Nov 2023 at 18:47, Tom Rini wrote:
>
> On Wed, Nov 15, 2023 at 06:42:19PM -0700, Simon Glass wrote:
> > Hi Tom,
> >
> > On Wed, 15 Nov 2023 at 15:38, Tom Rini wrote:
> > >
> > > On Sat, Nov 11, 2023 at 05:09:12PM -0700, Simon Glass wrote:
> > >
> > > > Rather than passing it all the command-line args, pass in the pieces
> > > > that it needs. These are the image address, the ramdisk address/name
> > > > and the FDT address/name.
> > > >
> > > > Ultimately this will allow usage of this function without being called
> > > > from the command line.
> > >
> > > OK, so this goal is good.
> > >
> > > [snip]
> > > > + return bootm_find_images(img_addr, argc > 1 ? argv[1] :
> > > > NULL,
> > > > + argc > 2 ? argv[2] : NULL, 0, 0);
> > >
> > > That we repeat this much harder to read test/if/else three times now is
> > > less good. Can we find some way to hide the complexity here in the case
> > > where it's coming from a command and so we have argc/argv[] ?
> >
> > I can't really think of one. Ultimately this is coming from the fact
> > that the booti and bootz commands directly call bootm_find_images(). I
> > haven't got far enough to know whether that will still be true in the
> > end, but I hope not.
> >
> > IMO the correct place for the logic above is in the command-processing
> > code, where it decides which arg means what.
> >
> > I could imagine something like:
> >
> > static const char *cmd_get_arg(int argc, char *const argv[], int argnum)
> > {
> >return argc > argnum ? argv[argnum] : NULL;
> > }
> >
> > but I'm not sure that is an improvement.
>
> I was thinking about this more after I sent this. And I think we might
> indeed want an inline / macro to handle this case more generally as I
> suspect we'll have similar cases where we need argN-or-NULL as we
> refactor other areas of code to split "here is the command" from "here
> is the library functionality" of it. Then we'll have:
> ulong foo = cmd_arg_one(argc, argv);
> ulong bar = cmd_arg_two(argc, argv);
>
> librarycall(foo, bar, ...);
OK I can try something. But note that the one, two terminology becomes
confusing. Is the first argument zero or one? Also we often drop an
argument in a subcommand to allow things to start from 0 again.
It might be better to use first and second, instead?
Regards,
Simon
Re: [PATCH 27/29] bootm: Adjust the parameters of bootm_find_images()
Hi Tom,
On Wed, 15 Nov 2023 at 15:38, Tom Rini wrote:
>
> On Sat, Nov 11, 2023 at 05:09:12PM -0700, Simon Glass wrote:
>
> > Rather than passing it all the command-line args, pass in the pieces
> > that it needs. These are the image address, the ramdisk address/name
> > and the FDT address/name.
> >
> > Ultimately this will allow usage of this function without being called
> > from the command line.
>
> OK, so this goal is good.
>
> [snip]
> > + return bootm_find_images(img_addr, argc > 1 ? argv[1] : NULL,
> > + argc > 2 ? argv[2] : NULL, 0, 0);
>
> That we repeat this much harder to read test/if/else three times now is
> less good. Can we find some way to hide the complexity here in the case
> where it's coming from a command and so we have argc/argv[] ?
I can't really think of one. Ultimately this is coming from the fact
that the booti and bootz commands directly call bootm_find_images(). I
haven't got far enough to know whether that will still be true in the
end, but I hope not.
IMO the correct place for the logic above is in the command-processing
code, where it decides which arg means what.
I could imagine something like:
static const char *cmd_get_arg(int argc, char *const argv[], int argnum)
{
return argc > argnum ? argv[argnum] : NULL;
}
but I'm not sure that is an improvement.
I have got a little further (15 more patches) and have added a 'struct
bootm_info' to hold the state of the boot, including the arguments for
the FIT address, ramdisk and fdt. But even then, the args will need to
be set up by the individual commands based on their syntax.
Regards,
Simon
Re: [BUG] usage of $kernel_addr_r in distro_efi_boot()
Hi Heinrich, On Wed, 15 Nov 2023 at 18:07, Heinrich Schuchardt wrote: > > If $kernel_addr_r cannot be read, distro_efi_boot() falls back to > address 0. This will lead to a segmentation fault on many systems. > > distro_efi_read_bootflow_net() has a fallback to CONFIG_SYS_LOAD_ADDR > and may have loaded to that address. > > Shouldn't we be consistent in what we program? > > Please, use a field in the bflow variable to indicate if and where an > EFI binary has been loaded. Can you check this series? https://patchwork.ozlabs.org/project/uboot/list/?series=382070 Regards, Simon
Re: Bug in distro_efi_boot()
Hi Heinrich, On Wed, 15 Nov 2023 at 17:54, Heinrich Schuchardt wrote: > > Hello Simon, > > In function distro_efi_boot() the bootefi command is called. The second > argument should only be passed if a device-tree has actually been > loaded. But this is not what the code does: > > For network boot methods the value of $fdt_addr_r is passed as second > parameter though possibly no file has been loaded there. We should not > pass a second parameter to bootefi in this case if no device-tree was > loaded. The bootefi command will fall back to the U-Boot device-tree in > this case. > > For other boot methods gd->fdt_blob is passed as second parameter. We > should not pass a second parameter in this case either. OK, thanks for the bug report. Can you send a patch? Regards, Simon
Re: [PATCH v4 05/12] usb: Avoid unbinding devices in use by bootflows
Hi Heinrich, On Wed, 15 Nov 2023 at 18:34, Heinrich Schuchardt wrote: > > On 11/15/23 17:23, Heinrich Schuchardt wrote: > > On 11/15/23 16:50, Simon Glass wrote: > >> Hi Heinrich, > >> > >> On Sun, 12 Nov 2023 at 16:35, Heinrich Schuchardt > >> wrote: > >>> > >>> > >>> > >>> Am 12. November 2023 22:20:50 MEZ schrieb Simon Glass > >>> : > >>>> Hi Heinrich, > >>>> > >>>> On Sun, 12 Nov 2023 at 13:32, Heinrich Schuchardt > >>>> wrote: > >>>>> > >>>>> > >>>>> > >>>>> Am 12. November 2023 21:02:42 MEZ schrieb Simon Glass > >>>>> : > >>>>>> When a USB device is unbound, it causes any bootflows attached to > >>>>>> it to > >>>>>> be removed, via a call to bootdev_clear_bootflows() from > >>>>>> bootdev_pre_unbind(). This obviously makes it impossible to boot the > >>>>>> bootflow. > >>>>>> > >>>>>> However, when booting a bootflow that relies on USB, usb_stop() is > >>>>>> called, which unbinds the device. For EFI, this happens in > >>>>>> efi_exit_boot_services() which means that the bootflow disappears > >>>>>> before it is finished with. > >>>>> > >>>>> After ExitBootServices() no driver of U-Boot can be used as the > >>>>> operating system is in charge. > >>>>> > >>>>> Any bootflow inside U-Boot is terminated by definition when > >>>>> reaching ExitBootServices. > >>>>> > >>>>>> > >>>>>> There is no need to unbind all the USB devices just to quiesce them. > >>>>>> Add a new usb_pause() call which removes them but leaves them bound. > >>>>> > >>>>> As U-Boot must not access any device after ExitBootServices() it is > >>>>> unclear to me what you want to achieve. > >>>> > >>>> I can't remember exactly what is needed from the bootflow, but > >>>> something is. Perhaps the kernel, or the cmdline, or fdt? It would > >>>> have been better if I had mentioned this explicitly, But then this > >>>> patch has been sitting around for ages... > >>>> > >>>> In any case, the intent of exit-boot-services is not to free all the > >>>> memory used, since some of it may have been used to hold data that the > >>>> app continues to use. > >>> > >>> The EFI application reads the memory map and receives an ID which it > >>> passes to ExitBootServiced() after this point any memory not marked > >>> as EFI runtime can be used by the EFI app. This includes the memory > >>> that harbors the U-Boot USB drivers. Therefore no drivers can be used > >>> here. > >>> > >>> Starting the EFI app via StartImage() must terminate any running > >>> U-Boot "boot flow". > >>> > >>> After ExitBootServices() the EFI application cannot return to U-Boot > >>> except for SetVirtualAdressMspsetting which relocates the EFI runtime. > >>> > >>> Bootflows and U-Boot drivers have no meaning after ExitBootServices(). > >>> > >>> > >>> > >>>> > >>>> Also there is U-Boot code between when the devices are unbound and > >>>> when U-Boot actually exits back to the app. > >>>> > >>>> This hang was 100% repeatable on brya (an x86 Chromebook) and it took > >>>> quite a while to find. > >>> > >>> We need a better understanding of the problem that you experience to > >>> find an adequate solution. Why does removing all devices lead to > >>> hanging the system? > >> > >> Are you able to test this? With your better knowledge of EFI you might > >> be able to figure out something else that is going on. But in my case > >> it causes some memory to be freed (perhaps the memory holding the EFI > >> app?), which is then overwritten by something else being malloc()'d, > > > > The memory for the EFI app is not assigned via malloc(). It is allocated > > by AllocatedPages(). > > > > Reading "some memory freed" does not give me confidence that the problem > > is sufficiently analyzed. > > > >&
Re: bug - bootflow: grub efi crashes when bootflow selected explicitly
Hi Heinrich, On Wed, 15 Nov 2023 at 18:25, Heinrich Schuchardt wrote: > > On 11/15/23 23:46, Heinrich Schuchardt wrote: > > > > > > Am 15. November 2023 23:15:46 MEZ schrieb Simon Glass : > >> Hi Shantur, > >> > >> On Wed, 15 Nov 2023 at 15:13, Shantur Rathore wrote: > >>> > >>> Hi Simon, > >>> > >>> I have figured out the cause of the crash. > >>> It happens here - > >>> https://github.com/u-boot/u-boot/blob/master/boot/bootflow.c#L470 > >>> while doing - free(bflow->buf) > > Unfortunately the description of the field bflow->buf is deceptively wrong: > > @buf: Bootflow file contents (allocated) > > The EFI bootflow never allocates this buffer but uses the address > $kernel_addr_r without allocation. > > We must not call free on an address that we never allocated via malloc(). > > Doesn't this also explain the error you experienced before writing > > [PATCH v4 05/12] usb: Avoid unbinding devices in use by bootflows > https://lore.kernel.org/u-boot/cahc5_t3v23k_xbws5o-g9iqfoq7fhpkscf89xdaaago+bu8...@mail.gmail.com/T/#m992e20fb25fe0f2f0047e901a76e78628e59da7a Yes that is indeed the bug report from Shantur. I just sent a patch. I still would like the USB patch to go in though, as it is wrong to unbind devices before boot. We have a special device_remove() flag for handling this and it should be used with all devices, including USB. [..] Regards, Simon
Re: bug - bootflow: grub efi crashes when bootflow selected explicitly
Hi Heinrich, On Wed, 15 Nov 2023 at 15:47, Heinrich Schuchardt wrote: > > > > Am 15. November 2023 23:15:46 MEZ schrieb Simon Glass : > >Hi Shantur, > > > >On Wed, 15 Nov 2023 at 15:13, Shantur Rathore wrote: > >> > >> Hi Simon, > >> > >> I have figured out the cause of the crash. > >> It happens here - > >> https://github.com/u-boot/u-boot/blob/master/boot/bootflow.c#L470 > >> while doing - free(bflow->buf) > >> > >> As I understand it, > >> - Just before starting kernel EFI binary calls usb-uclass->usb_stop() > >> - This starts removing all devices in my case ( 6x usb_hub, 2x > >> partition, 1x blk, 2x bootdev, 1x usb_maas_storage) > >> - While removing bootdev, it ends up calling bootdev-uclass -> > >> bootdev_pre_unbind -> bootdev_clear_bootflows which calls > >> bootflow->bootflow_remove and bootflow_free > >> > >> I don't know why this buf cannot be freed, is it because the EFI > >> binary in the buffer is still being used ( efi/boot/bootaa64.efi ) ? > > EFI binaries should never be started from memory allocated with malloc. > efi_load_image() should be invoked which allocates EFI memory via > efi_allocate_pages(). The handle created has to be passed to > efi_start_image(). This is a mess at present, as you are probably aware. EFI has hack to look at loaded images and try to patch up its tables. With bootstd we will be able to do this properly. But note that memory allocation in U-Boot is done via malloc() and lmb. We don't call efi_allocate_pages() except in the EFI code. > > >> But commenting this line out fixes the crash and linux boots happily. > >> > >> Fixing this properly is above my pay grade as of now. > > > >Great, thank you! I will send a patch. > > Free() typically crashes in U-Boot when freeing the same memory twice. Indeed. In this case it was not even allocated. Regards, Simon
[PATCH] bootstd: Avoid freeing a non-allocated buffer
EFI applications can be very large and thus used to cause boot failures
when malloc() space was exhausted.
A recent changed fixed this by using the kernel_addr_r environment var
as the address of the buffer. However, it still frees the buffer when
the bootflow is discarded.
Fix this by introducing a flag to indicate whether the buffer was
allocated, or not.
Note that kernel_addr_r is not the last word here. It might be better
to use lmb to place images. But there is a lot of refactoring to do
before we can remove the environment variables. The distro scripts rely
on them so it is safe for bootstd to do so too.
Fixes: 6a8c2f9781c bootstd: Avoid allocating memory for the EFI file
Signed-off-by: Simon Glass
Reported by: Simon Glass
Reported by: Shantur Rathore
---
boot/bootflow.c | 3 ++-
boot/bootmeth_efi.c | 1 +
include/bootflow.h | 5 -
3 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/boot/bootflow.c b/boot/bootflow.c
index 6922e7e0c4e7..1ea2966ae9a5 100644
--- a/boot/bootflow.c
+++ b/boot/bootflow.c
@@ -467,7 +467,8 @@ void bootflow_free(struct bootflow *bflow)
free(bflow->name);
free(bflow->subdir);
free(bflow->fname);
- free(bflow->buf);
+ if (!(bflow->flags & BOOTFLOWF_STATIC_BUF))
+ free(bflow->buf);
free(bflow->os_name);
free(bflow->fdt_fname);
free(bflow->bootmeth_priv);
diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c
index ae936c8daa18..9ba7734911e1 100644
--- a/boot/bootmeth_efi.c
+++ b/boot/bootmeth_efi.c
@@ -160,6 +160,7 @@ static int efiload_read_file(struct bootflow *bflow, ulong
addr)
if (ret)
return log_msg_ret("read", ret);
bflow->buf = map_sysmem(addr, bflow->size);
+ bflow->flags |= BOOTFLOWF_STATIC_BUF;
set_efi_bootdev(desc, bflow);
diff --git a/include/bootflow.h b/include/bootflow.h
index 44d3741eacae..fede8f22a2b8 100644
--- a/include/bootflow.h
+++ b/include/bootflow.h
@@ -43,9 +43,12 @@ enum bootflow_state_t {
* and it is using the prior-stage FDT, which is the U-Boot control FDT.
* This is only possible with the EFI bootmeth (distro-efi) and only when
* CONFIG_OF_HAS_PRIOR_STAGE is enabled
+ * @BOOTFLOWF_STATIC_BUF: Indicates that @bflow->buf is statically set, rather
+ * than being allocated by malloc().
*/
enum bootflow_flags_t {
BOOTFLOWF_USE_PRIOR_FDT = 1 << 0,
+ BOOTFLOWF_STATIC_BUF= 1 << 1,
};
/**
@@ -72,7 +75,7 @@ enum bootflow_flags_t {
* @fname: Filename of bootflow file (allocated)
* @logo: Logo to display for this bootflow (BMP format)
* @logo_size: Size of the logo in bytes
- * @buf: Bootflow file contents (allocated)
+ * @buf: Bootflow file contents (allocated unless @flags &
BOOTFLOWF_STATIC_BUF)
* @size: Size of bootflow file in bytes
* @err: Error number received (0 if OK)
* @os_name: Name of the OS / distro being booted, or NULL if not known
--
2.43.0.rc0.421.g78406f8d94-goog
Re: bug - bootflow: grub efi crashes when bootflow selected explicitly
Hi Shantur, On Wed, 15 Nov 2023 at 15:13, Shantur Rathore wrote: > > Hi Simon, > > I have figured out the cause of the crash. > It happens here - > https://github.com/u-boot/u-boot/blob/master/boot/bootflow.c#L470 > while doing - free(bflow->buf) > > As I understand it, > - Just before starting kernel EFI binary calls usb-uclass->usb_stop() > - This starts removing all devices in my case ( 6x usb_hub, 2x > partition, 1x blk, 2x bootdev, 1x usb_maas_storage) > - While removing bootdev, it ends up calling bootdev-uclass -> > bootdev_pre_unbind -> bootdev_clear_bootflows which calls > bootflow->bootflow_remove and bootflow_free > > I don't know why this buf cannot be freed, is it because the EFI > binary in the buffer is still being used ( efi/boot/bootaa64.efi ) ? > But commenting this line out fixes the crash and linux boots happily. > > Fixing this properly is above my pay grade as of now. Great, thank you! I will send a patch. Regards, Simon > > Kind regards, > Shantur > > On Wed, Nov 15, 2023 at 3:58 PM Simon Glass wrote: > > > > Hi Shantur, > > > > On Wed, 15 Nov 2023 at 08:23, Shantur Rathore wrote: > > > > > > Hi Simon, > > > > > > > > > > > Is this the blue port on top of the USB-C connector? > > > > > > > Yes, that's correct. > > > For my drive I needed - > > > https://patchwork.ozlabs.org/project/uboot/patch/20231110141311.512334-...@shantur.com/ > > > > > > > > > > > Which version of Armbian / download link? > > > > > > > https://redirect.armbian.com/rockpro64/Bookworm_current > > > > > > > Yes it is scanning first, before reading the efi app, etc. > > > > > > > > I have the same hardware so may be able to dig into this. > > > > > > Sorry, I meant to ask if anything specific to USB. > > > I see it loads efi from the network or disk and calls bootefi with the > > > loaded address. > > > I don't know deep boot / efi stuff, just trying to compare how it's > > > loading efi differently than fatload in this case. > > > > There is nothing special about USB in boootstd, so far as I know. > > > > But until we figure out this bug, it is hard to be sure. > > > > Regards, > > Simon
Re: bug - bootflow: grub efi crashes when bootflow selected explicitly
Hi Shantur, On Wed, 15 Nov 2023 at 08:23, Shantur Rathore wrote: > > Hi Simon, > > > > > Is this the blue port on top of the USB-C connector? > > > Yes, that's correct. > For my drive I needed - > https://patchwork.ozlabs.org/project/uboot/patch/20231110141311.512334-...@shantur.com/ > > > > > Which version of Armbian / download link? > > > https://redirect.armbian.com/rockpro64/Bookworm_current > > > Yes it is scanning first, before reading the efi app, etc. > > > > I have the same hardware so may be able to dig into this. > > Sorry, I meant to ask if anything specific to USB. > I see it loads efi from the network or disk and calls bootefi with the > loaded address. > I don't know deep boot / efi stuff, just trying to compare how it's > loading efi differently than fatload in this case. There is nothing special about USB in boootstd, so far as I know. But until we figure out this bug, it is hard to be sure. Regards, Simon
Re: [PATCH v4 05/12] usb: Avoid unbinding devices in use by bootflows
Hi Heinrich, On Sun, 12 Nov 2023 at 16:35, Heinrich Schuchardt wrote: > > > > Am 12. November 2023 22:20:50 MEZ schrieb Simon Glass : > >Hi Heinrich, > > > >On Sun, 12 Nov 2023 at 13:32, Heinrich Schuchardt wrote: > >> > >> > >> > >> Am 12. November 2023 21:02:42 MEZ schrieb Simon Glass : > >> >When a USB device is unbound, it causes any bootflows attached to it to > >> >be removed, via a call to bootdev_clear_bootflows() from > >> >bootdev_pre_unbind(). This obviously makes it impossible to boot the > >> >bootflow. > >> > > >> >However, when booting a bootflow that relies on USB, usb_stop() is > >> >called, which unbinds the device. For EFI, this happens in > >> >efi_exit_boot_services() which means that the bootflow disappears > >> >before it is finished with. > >> > >> After ExitBootServices() no driver of U-Boot can be used as the operating > >> system is in charge. > >> > >> Any bootflow inside U-Boot is terminated by definition when reaching > >> ExitBootServices. > >> > >> > > >> >There is no need to unbind all the USB devices just to quiesce them. > >> >Add a new usb_pause() call which removes them but leaves them bound. > >> > >> As U-Boot must not access any device after ExitBootServices() it is > >> unclear to me what you want to achieve. > > > >I can't remember exactly what is needed from the bootflow, but > >something is. Perhaps the kernel, or the cmdline, or fdt? It would > >have been better if I had mentioned this explicitly, But then this > >patch has been sitting around for ages... > > > >In any case, the intent of exit-boot-services is not to free all the > >memory used, since some of it may have been used to hold data that the > >app continues to use. > > The EFI application reads the memory map and receives an ID which it passes > to ExitBootServiced() after this point any memory not marked as EFI runtime > can be used by the EFI app. This includes the memory that harbors the U-Boot > USB drivers. Therefore no drivers can be used here. > > Starting the EFI app via StartImage() must terminate any running U-Boot > "boot flow". > > After ExitBootServices() the EFI application cannot return to U-Boot except > for SetVirtualAdressMspsetting which relocates the EFI runtime. > > Bootflows and U-Boot drivers have no meaning after ExitBootServices(). > > > > > > >Also there is U-Boot code between when the devices are unbound and > >when U-Boot actually exits back to the app. > > > >This hang was 100% repeatable on brya (an x86 Chromebook) and it took > >quite a while to find. > > We need a better understanding of the problem that you experience to find an > adequate solution. Why does removing all devices lead to hanging the system? Are you able to test this? With your better knowledge of EFI you might be able to figure out something else that is going on. But in my case it causes some memory to be freed (perhaps the memory holding the EFI app?), which is then overwritten by something else being malloc()'d, so the boot hangs. It is hard to see what is going on after the app starts. This patch was sent almost two months ago and fixes a real bug. The first few versions attracted no comment now it is being blocked because you don't understand how it fixes anything. I can get the hardware up again and try this but it will take a while. Even without the hang, this still fixes a bug. We should be using device_remove() to quiesce devices. There is no need to unbind them. BTW another patch that suffered a similar fate was [1]. I just applied it based on a review from Bin. [..] Regards, Simon [1] https://patchwork.ozlabs.org/project/uboot/patch/20231001191444.v3.1.I9f7f373d00947c704aeae0088dfedd8df07fab60@changeid/
Re: bug - bootflow: grub efi crashes when bootflow selected explicitly
Hi Shantur,
On Wed, 15 Nov 2023 at 07:50, Shantur Rathore wrote:
>
> Hi Simon,
>
> I did some testing on the USB 3.0 port and it seems like bootflow
> causes something to happen to the USB.
> It seems to be only reproducible on the USB3.0 port.
>
> I am using an AMicro AM8180 NVME to USB adapter on the USB3.0 port
> of RockPro64.
Is this the blue port on top of the USB-C connector?
>
> Booting the Armbian installation using
Which version of Armbian / download link?
>
> => fatload usb 0:1 ${kernel_addr_r} EFI/boot/bootaa64.efi
> 979672 bytes read in 6 ms (155.7 MiB/s)
> => bootefi ${kernel_addr_r}
>
> works 100%, no issues with USB whatsoever.
>
> Booting same with
>
> => setenv boot_targets usb
> => setenv bootmeths efi
> => bootflow scan -lb
> Scanning for bootflows in all bootdevs
> Seq Method State UclassPart Name Filename
> --- --- --
>
> Scanning bootdev 'usb_mass_storage.lun0.bootdev':
> 0 efi ready usb_mass_1 usb_mass_storage.lun0.boo
> efi/boot/bootaa64.efi
> ** Booting bootflow 'usb_mass_storage.lun0.bootdev.part_1' with efi
>
> And I see issues with USB device not communicating properly like
>
> [ 50.182144] sd 0:0:0:0: [sda] tag#29 uas_eh_abort_handler 0 uas-tag
> 24 inflight: CMD OUT
> [ 50.182957] sd 0:0:0:0: [sda] tag#29 CDB: opcode=0x2a 2a 00 03 4e
> e9 78 00 00 08 00
> [ 55.270116] xhci-hcd xhci-hcd.2.auto: xHCI host not responding to
> stop endpoint command
> [ 55.284476] xhci-hcd xhci-hcd.2.auto: xHCI host controller not
> responding, assume dead
> [ 55.285494] xhci-hcd xhci-hcd.2.auto: HC died; cleaning up
>
> This is 100% reproducible.
>
> Is bootflow doing something different to USB than normal fatload +
> bootefi, I see internally it uses bootefi as well.
Yes it is scanning first, before reading the efi app, etc.
I have the same hardware so may be able to dig into this.
Regards,
Simon
Re: bug - bootflow: grub efi crashes when bootflow selected explicitly
Hi Shantur, On Wed, 15 Nov 2023 at 07:12, Shantur Rathore wrote: > > HI Simon, > > Thanks for the speedy reply. > I can confirm patch [2] fixes the problem - at least on the USB2.0 > port on RockPro64. > I see some issue with the USB disk being unable to provide its size on > the USB3.0 port but that might not be linked to bootflow. > > PS - Also tested with > https://patchwork.ozlabs.org/project/uboot/list/?series=382070 OK, good. Can you please reply on the patch(es) with a Tested-by tag? Regards, SImon > > Regards, > Shantur > > On Wed, Nov 15, 2023 at 1:12 PM Simon Glass wrote: > > > > +Heinrich Schuchardt > > > > Hi Shantur, > > > > On Wed, 15 Nov 2023 at 05:59, Shantur Rathore wrote: > > > > > > Hi all, > > > > > > I am facing an issue with bootflow where grub efi crashes only when > > > bootflow is selected manually. > > > > > > Board - RockPro64 > > > OS - Armbian Bookworm CLI > > > U-boot : 2024.01-rc2 > > > Built and installed in SPI > > > Boot Device = USB drive connected to USB 2.0 port > > > > > > Success scenario : > > > > > > => setenv boot_targets usb > > > => setenv bootmeths efi > > > => bootflow scan -lb > > > > > > Grub loads and Linux boots > > > Full Log - https://pastebin.com/KUFtUcha > > > > > > Failure scenario > > > Full Log : https://pastebin.com/GHyG2NDz > > > > > > > setenv boot_targets usb > > > => setenv bootmeths efi > > > => bootflow scan > > > => bootflow list > > > Showing all bootflows > > > Seq Method State Uclass Part Name Filename > > > --- --- -- > > > > > > 0 efi ready usb_mass_ 1 usb_mass_storage.lun0.boo efi/boot/bootaa64.efi > > > --- --- -- > > > > > > (1 bootflow, 1 valid) > > > => bootflow select 0 > > > => bootflow boot > > > ** Booting bootflow 'usb_mass_storage.lun0.bootdev.part_1' with efi > > > > > > Loading Linux 6.1.50-current-rockchip64 ... > > > Loading initial ramdisk ... > > > EFI stub: Booting Linux Kernel... > > > EFI stub: Using DTB from configuration table > > > EFI stub: Exiting boot services... > > > "Synchronous Abort" handler, esr 0x9644, far 0x300905a65 > > > elr: 0022d634 lr : 00208e14 (reloc) > > > elr: f3f47634 lr : f3f22e14 > > > x0 : 000300905a4d x1 : > > > x2 : x3 : 0207fff0 > > > x4 : f3fd6470 x5 : 0207fff0 > > > x6 : 0004 x7 : f1f9fad0 > > > x8 : 7f7f7f7f7f7f7f7f x9 : d02c > > > x10: f1efee8c x11: d020 > > > x12: f1efef88 x13: ad40 > > > x14: f1efef2c x15: f1eff057 > > > x16: f3f21fc0 x17: d017a1d0 > > > x18: f1f11d70 x19: f1f21eb0 > > > x20: x21: f1f27b80 > > > x22: x23: > > > x24: x25: f3fc9ed7 > > > x26: b0c36000 x27: f02cd000 > > > x28: f03a210c x29: f1efee20 > > > > > > Code: f9400860 eb06001f 540004e0 f9400c66 (f9000c06) > > > UEFI image [0xf0da4000:0xf0dbbfff] '/efi\boot\fbaa64.efi' > > > UEFI image [0xf0386000:0xf07adfff] > > > '/\EFI\debian\grubaa64.efi' > > > UEFI image [0xaf4e:0xb11d] > > > Resetting CPU ... > > > > > > resetting ... > > > > > > I am unable to figure out what is the difference between the 2 ways of > > > booting. > > > > > > Thanks for your help. > > > > Thank you for the detailed report. Can you try series [1] in > > particular patch [2] > > > > It has been sitting around for quite a while, unfortunately. > > > > Regards, > > Simon > > > > [1] https://patchwork.ozlabs.org/project/uboot/list/?series=381851 > > [2] > > https://patchwork.ozlabs.org/project/uboot/patch/20231112130245.v4.5.If206027372f73ce32480223e5626f4b944e281b7@changeid/
Re: u-boot fails to build on sparc64 due redefinition of 'struct termio'
Hi Adrian,
On Wed, 15 Nov 2023 at 00:27, John Paul Adrian Glaubitz
wrote:
>
> Hello!
>
> On Linux sparc64, building u-boot fails with [1]:
>
> cc -o tools/mkenvimage tools/mkenvimage.o tools/os_support.o
> tools/lib/crc32.o
> In file included from /<>/tools/termios_linux.h:33,
> from /<>/tools/kwboot.c:153:
> /usr/include/sparc64-linux-gnu/asm/termbits.h:14:8: error: redefinition of
> 'struct termio'
>14 | struct termio {
> |^~
> In file included from /usr/include/sparc64-linux-gnu/sys/ioctl.h:29,
> from /<>/tools/termios_linux.h:30:
> /usr/include/sparc64-linux-gnu/bits/ioctl-types.h:36:8: note: originally
> defined here
>36 | struct termio
> |^~
>
> Reading through tools/termios_linux.h, it seems like the problem is that
>
> and are included at the same time. Not sure whether this is
> allowed
> according to the glibc documentation.
Can you send a patch to fix it?
(BTW gmail tells me you are John when I start the email, but I noticed
it this time and changed it)
Regards,
Simon
>
> Full build log available in [1].
>
> Adrian
>
> > [1]
> > https://buildd.debian.org/status/fetch.php?pkg=u-boot=sparc64=2023.07%2
> > Bdfsg-1=1699723544=0
>
> --
> .''`. John Paul Adrian Glaubitz
> : :' : Debian Developer
> `. `' Physicist
> `-GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
Re: [PATCH v5 11/20] efi: x86: Correct the condition for installing ACPI tables
Hi Heinrich,
On Wed, 15 Nov 2023 at 04:09, Heinrich Schuchardt wrote:
>
> On 9/20/23 05:00, Simon Glass wrote:
> > It is not always the case that U-Boot builds the ACPI tables itself. For
> > example, when booting from coreboot, the ACPI tables are built by
> > coreboot.
> >
> > Correct the Makefile condition so that U-Boot can pass on tables built
> > by a previous firmware stage.
> >
> > Tidy up the installation-condition code while we are here.
> >
> > Signed-off-by: Simon Glass
> > Reviewed-by: Ilias Apalodimas
> > Reviewed-by: Bin Meng
> > ---
> >
> > (no changes since v2)
> >
> > Changes in v2:
> > - Add new patch
> >
> > lib/efi_loader/Makefile| 2 +-
> > lib/efi_loader/efi_setup.c | 10 +-
> > 2 files changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
> > index 1a8c8d7cab5c..0eb748ff1a59 100644
> > --- a/lib/efi_loader/Makefile
> > +++ b/lib/efi_loader/Makefile
> > @@ -78,7 +78,7 @@ obj-$(CONFIG_EFI_ESRT) += efi_esrt.o
> > obj-$(CONFIG_VIDEO) += efi_gop.o
> > obj-$(CONFIG_BLK) += efi_disk.o
> > obj-$(CONFIG_NETDEVICES) += efi_net.o
> > -obj-$(CONFIG_GENERATE_ACPI_TABLE) += efi_acpi.o
> > +obj-$(CONFIG_ACPI) += efi_acpi.o
>
> This change merged as
> 53e8e6f98679 ("efi: x86: Correct the condition for installing ACPI tables")
> looks wrong:
>
> For QEMU with CONFIG_ACPI=y and CONFIG_GENERATE_ACPI=n I get
>
> EFI using ACPI tables at 0
> Error: Cannot initialize UEFI sub-system, r = 14
>
> Device-trees are only suppressed in the bootefi command if
> CONFIG_GENERATE_ACPI=y.
>
> Currently CONFIG_ACPI=y only indicates that ACPI libraries are built. It
> does not indicate that ACPI tables exist on the platform.
Similarly, neither does GENERATE_ACPI
Perhaps we should also check for tables being present?
if (IS_ENABLED(CONFIG_ACPI) && gd_acpi_start()) {
?
>
> Best regards
>
> Heinrich
>
>
> > obj-$(CONFIG_GENERATE_SMBIOS_TABLE) += efi_smbios.o
> > obj-$(CONFIG_EFI_RNG_PROTOCOL) += efi_rng.o
> > obj-$(CONFIG_EFI_TCG2_PROTOCOL) += efi_tcg2.o
> > diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
> > index 58d4e1340233..ad719afd6328 100644
> > --- a/lib/efi_loader/efi_setup.c
> > +++ b/lib/efi_loader/efi_setup.c
> > @@ -321,11 +321,11 @@ efi_status_t efi_init_obj_list(void)
> > if (ret != EFI_SUCCESS)
> > goto out;
> > #endif
> > -#ifdef CONFIG_GENERATE_ACPI_TABLE
> > - ret = efi_acpi_register();
> > - if (ret != EFI_SUCCESS)
> > - goto out;
> > -#endif
> > + if (IS_ENABLED(CONFIG_ACPI)) {
> > + ret = efi_acpi_register();
> > + if (ret != EFI_SUCCESS)
> > + goto out;
> > + }
> > #ifdef CONFIG_GENERATE_SMBIOS_TABLE
> > ret = efi_smbios_register();
> > if (ret != EFI_SUCCESS)
>
Regards,
Simon
Re: [PATCH 00/34] x86: expo: Add support for editing coreboot CMOS RAM settings
Hi Bin, On Sun, 1 Oct 2023 at 19:16, Simon Glass wrote: > > U-Boot provides support for editing settings with an 'expo', as well as > reading and writing settings to CMOS RAM. > > This series integrates expo functionality with coreboot, using the > sysinfo table to get a list of settings, creating an expo with them and > allowing them to be edited. > > A new CI test provides coverage for some coreboot-related commands. For > this to work, a number of minor tweaks are made to existing tests, so > that they pass on coreboot (x86) or at least are skipped when they > cannot pass. Likely most of these fixes will apply to other boards too. > > It includes several other fixes and improvements: > - new -m flag for 'bootflow scan' so it can display a menu automatically > - Fix for video-scrolling crash with Truetype > - menu items can now have individual integer values > - menu items are lined up according to the width of all menu labels > > > Simon Glass (34): > test: Run tests that don't need devices > test: Add a new suite for commands > test: Add helper to skip to partial console line > test: Make UT_LIB_ASN1 depend on sandbox > test: Run bootstd tests only on sandbox > test: Handle use of stack pointer in bdinfo > test: bdinfo: Add missing asserts > test: fdt: Add a special case for real boards > test: font: Add dependencies on fonts > test: event: Only run test_event_probe() on sandbox > test: lmb: Move tests into the lib suite > test: print: Skip test on x86 > video: Add a function to clear the display > sandbox: Add a dummy booti command > bootstd: Add a menu option to bootflow scan > video: Add a dark-grey console colour > video: Avoid starting a new line to close to the bottom > expo: Place menu items to the right of all labels > expo: Set the initial next_id to 1 > expo: Use standard numbering for save and discard > expo: Allow menu items to have values > expo: Add a little more cedit CMOS logging > expo: Support menu-item values in cedit > expo: Drop unneceesary calls to expo_str() > expo: Drop scene_title_set() > expo: Add forward declaration for udevice to cedit > x86: coreboot: Enable unit tests > x86: CI: Update coreboot > x86: coreboot: Add a test for cbsysinfo command > x86: coreboot: Show the option table > x86: coreboot: Enable support for the configuration editor > x86: coreboot: Add a command to check and update CMOS RAM > x86: coreboot: Allow building an expo for editing CMOS config > x86: Enable RTC command by default > > .azure-pipelines.yml | 2 +- > .gitlab-ci.yml| 2 +- > arch/sandbox/dts/cedit.dtsi | 3 + > arch/sandbox/lib/bootm.c | 7 + > arch/x86/dts/coreboot.dts | 7 + > boot/Makefile | 4 + > boot/cedit.c | 191 +++ > boot/expo.c | 3 + > boot/expo_build.c | 36 ++--- > boot/expo_build_cb.c | 244 ++ > boot/scene.c | 61 ++-- > boot/scene_internal.h | 30 +++- > boot/scene_menu.c | 26 +++- > boot/scene_textline.c | 3 +- > cmd/Kconfig | 14 +- > cmd/bootflow.c| 27 +++- > cmd/booti.c | 2 +- > cmd/cedit.c | 28 > cmd/cls.c | 25 +-- > cmd/x86/Makefile | 1 + > cmd/x86/cbcmos.c | 141 + > cmd/x86/cbsysinfo.c | 73 - > common/console.c | 31 > configs/coreboot64_defconfig | 2 + > configs/coreboot_defconfig| 6 + > configs/tools-only_defconfig | 3 +- > doc/board/coreboot/coreboot.rst | 6 + > doc/develop/cedit.rst | 9 +- > doc/develop/expo.rst | 26 +++- > doc/usage/cmd/bootflow.rst| 5 + > doc/usage/cmd/cbcmos.rst | 45 ++ > doc/usage/cmd/cbsysinfo.rst | 99 > doc/usage/cmd/cedit.rst | 91 ++- > doc/usage/index.rst | 1 + > drivers/video/vidconsole-uclass.c | 4 +- > drivers/video/video-uclass.c | 3 + > include/cedit.h | 1 + > include/console.h | 10 ++ > include/expo.h| 51 +-- > include/test/cedit-test.h | 30 ++-- > include/test/cmd.h| 15 ++ > include/test/suites.h | 1 + > include/test/ut.h | 30 > include/video.h | 4 +- > include/
Please pull u-boot-dm
Hi Tom, https://source.denx.de/u-boot/custodians/u-boot-dm/-/pipelines/18586 https://dev.azure.com/simon0972/u-boot/_build/results?buildId=55=results The following changes since commit 92b27528d777ce85362af45e7d2974a6c856219b: Merge branch 'master' of https://source.denx.de/u-boot/custodians/u-boot-sunxi (2023-11-12 16:36:52 -0500) are available in the Git repository at: git://git.denx.de/u-boot-dm.git tags/dm-pull-15nov23 for you to fetch changes up to 0d4d9f94c77f78cddc372c307465fc92413e: bootstage: Correct exhasuted typo (2023-11-14 20:04:01 -0700) patman correct import of u_boot_pylib correct long-standing EFI framebuffer bug minor test refactor Heinrich Schuchardt (1): dm: Do not enable debug messages by default Sean Anderson (1): sandbox: Close file after mmaping it Simon Glass (10): efi: Correct handling of frame buffer bootstd: Refactor mmc prep to allow a different scan bootstd: Add a return code to bootflow menu expo: Correct background colour patman: Split out arg parsing into its own file patman: Move the main program into a function patman: Correct easy pylint warnings in __main__ patman: Avoid using func_test at top level patman: Correct Python 3.6 behaviour bootstage: Correct exhasuted typo arch/sandbox/cpu/os.c | 15 ++-- boot/expo.c| 4 +- cmd/bootflow.c | 53 --- common/bootstage.c | 2 +- doc/usage/cmd/bootflow.rst | 67 ++ drivers/core/Kconfig | 1 - include/dm/util.h | 4 +- include/video.h| 9 +- lib/efi_loader/efi_gop.c | 12 +-- test/boot/bootflow.c | 64 +++-- tools/patman/__main__.py | 254 tools/patman/cmdline.py| 147 ++ 12 files changed, 413 insertions(+), 219 deletions(-) create mode 100644 tools/patman/cmdline.py Regards, Simon
Re: [PATCH v3 01/12] efi: Correct handling of frame buffer
On Mon, Oct 2, 2023 at 10:23 AM Simon Glass wrote:
>
> The efi_gop driver uses private fields from the video uclass to obtain a
> pointer to the frame buffer. Use the platform data instead.
>
> Check the VIDEO_COPY setting to determine which frame buffer to use. Once
> the next stage is running (and making use of U-Boot's EFI boot services)
> U-Boot does not handle copying from priv->fb to the hardware framebuffer,
> so we must allow EFI to write directly to the hardware framebuffer.
>
> We could provide a function to read this, but it seems better to just
> document how it works. The original change ignored an explicit comment
> in the video.h file ("Things that are private to the uclass: don't use
> these in the driver") which is why this was missed when the VIDEO_COPY
> feature was added.
>
> Signed-off-by: Simon Glass
> Fixes: 8f661a5b662 ("efi_loader: gop: Expose fb when 32bpp")
> ---
>
> (no changes since v2)
>
> Changes in v2:
> - Rebase to -next
> - Add some more comments to the header file
> - Add fixes tag
>
> include/video.h | 9 ++---
> lib/efi_loader/efi_gop.c | 12 +++-
> 2 files changed, 13 insertions(+), 8 deletions(-)
>
Reviewed-by: Bin Meng
Applied to u-boot-dm, thanks!
Re: [PATCH v3 02/12] bootstd: Refactor mmc prep to allow a different scan
Adjust scan_mmc4_bootdev() and related function so that the caller can do its own 'bootflow scan' command. This allows it to change the flags if needed. Signed-off-by: Simon Glass --- Changes in v3: - Add new patch to refactor mmc prep to allow a different scan test/boot/bootflow.c | 49 1 file changed, 40 insertions(+), 9 deletions(-) Applied to u-boot-dm, thanks!
Re: [PATCH v3 03/12] bootstd: Add a return code to bootflow menu
Return an error when the user does not select an OS, so we know whether to boot or not. Move calling of bootflow_menu_run() into a separate function so we can call it from other places. Expand the test to cover these cases. Add some documentation also, while we are here. Signed-off-by: Simon Glass --- Changes in v3: - Add missing word 'function' in the commit message Changes in v2: - Add new patch to add a return code to bootflow menu cmd/bootflow.c | 53 +++--- doc/usage/cmd/bootflow.rst | 67 ++ test/boot/bootflow.c | 15 + 3 files changed, 123 insertions(+), 12 deletions(-) Applied to u-boot-dm, thanks!
Re: [PATCH v3 06/12] expo: Correct background colour
On Mon, Oct 2, 2023 at 9:30 AM Simon Glass wrote: > > Use the correct background colour when using white-on-black. > > Signed-off-by: Simon Glass > --- > > (no changes since v1) > > boot/expo.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > Reviewed-by: Bin Meng Applied to u-boot-dm, thanks!
Re: [PATCH 1/5] patman: Split out arg parsing into its own file
Move this code into a separate cmdline module, as is done with the other tools. Use the same HAS_TESTS check as buildman Signed-off-by: Simon Glass --- tools/patman/__main__.py | 116 +- tools/patman/cmdline.py | 147 +++ 2 files changed, 150 insertions(+), 113 deletions(-) create mode 100644 tools/patman/cmdline.py Applied to u-boot-dm, thanks!
Re: [PATCH 2/5] patman: Move the main program into a function
Add a new run_patman() function to hold the main logic. Signed-off-by: Simon Glass --- tools/patman/__main__.py | 127 +-- 1 file changed, 67 insertions(+), 60 deletions(-) Applied to u-boot-dm, thanks!
Re: [PATCH 3/5] patman: Correct easy pylint warnings in __main__
Tidy up the code a little to reduce the number of pylint warnings. Signed-off-by: Simon Glass --- tools/patman/__main__.py | 32 1 file changed, 16 insertions(+), 16 deletions(-) Applied to u-boot-dm, thanks!
Re: [PATCH 4/5] patman: Avoid using func_test at top level
Import this only when it is needed, since it is not present when installed via 'pip install'. Signed-off-by: Simon Glass Fixes: https://source.denx.de/u-boot/u-boot/-/issues/26 --- tools/patman/__main__.py | 1 - 1 file changed, 1 deletion(-) Applied to u-boot-dm, thanks!
Re: [PATCH 5/5] patman: Correct Python 3.6 behaviour
The importlib_resources import is not actually used. Fix this so that patman can run on Python 3.6 to some extent, once 'pip3 install importlib-resources' has been run. Signed-off-by: Simon Glass --- tools/patman/__main__.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) Applied to u-boot-dm, thanks!
Re: [PATCH 1/1] dm: Do not enable debug messages by default
On Sat, 4 Nov 2023 at 18:40, Heinrich Schuchardt wrote: > > CONFIG_DM_WARN has a text indicating that these messages should only > provided when debugging. This implies that the setting must be default no. > > We should still create debug messages. > > Reported-by: Andre Przywara > Signed-off-by: Heinrich Schuchardt > --- > drivers/core/Kconfig | 1 - > include/dm/util.h| 4 +--- > 2 files changed, 1 insertion(+), 4 deletions(-) Reviewed-by: Simon Glass Applied to u-boot-dm, thanks!
Re: [PATCH] sandbox: Close file after mmaping it
On Sat, 4 Nov 2023 at 19:57, Sean Anderson wrote:
>
> After opening pathname, we must close ifd once we are done with it.
>
> Fixes: b9274095c2c ("sandbox: Add a way to map a file into memory")
> Signed-off-by: Sean Anderson
> ---
>
> arch/sandbox/cpu/os.c | 15 ++-
> 1 file changed, 10 insertions(+), 5 deletions(-)
Reviewed-by: Simon Glass
Applied to u-boot-dm, thanks!
Re: [PATCH] bootstage: Correct exhasuted typo
On Tue, 7 Nov 2023 at 02:18, Patrick Delaunay wrote: > > From: Simon Glass > > Correct this typo in the warning message shown when no more bootstage > records can be added. > > Signed-off-by: Simon Glass > Signed-off-by: Patrick Delaunay > --- > I just cherry-pick the Simon's patch in the branch fix-bs-working > of the dm custodian git but it is not yet present in patchwork. > > https://source.denx.de/u-boot/custodians/u-boot-dm/-/commit/8810b8dd3d233978c15203b23420fa819ab1e791 Reviewed-by: Simon Glass Thank you > > Regards > > Patrick > > common/bootstage.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > Applied to u-boot-dm, thanks!
Re: bug - bootflow: grub efi crashes when bootflow selected explicitly
+Heinrich Schuchardt Hi Shantur, On Wed, 15 Nov 2023 at 05:59, Shantur Rathore wrote: > > Hi all, > > I am facing an issue with bootflow where grub efi crashes only when > bootflow is selected manually. > > Board - RockPro64 > OS - Armbian Bookworm CLI > U-boot : 2024.01-rc2 > Built and installed in SPI > Boot Device = USB drive connected to USB 2.0 port > > Success scenario : > > => setenv boot_targets usb > => setenv bootmeths efi > => bootflow scan -lb > > Grub loads and Linux boots > Full Log - https://pastebin.com/KUFtUcha > > Failure scenario > Full Log : https://pastebin.com/GHyG2NDz > > > setenv boot_targets usb > => setenv bootmeths efi > => bootflow scan > => bootflow list > Showing all bootflows > Seq Method State Uclass Part Name Filename > --- --- -- > 0 efi ready usb_mass_ 1 usb_mass_storage.lun0.boo efi/boot/bootaa64.efi > --- --- -- > (1 bootflow, 1 valid) > => bootflow select 0 > => bootflow boot > ** Booting bootflow 'usb_mass_storage.lun0.bootdev.part_1' with efi > > Loading Linux 6.1.50-current-rockchip64 ... > Loading initial ramdisk ... > EFI stub: Booting Linux Kernel... > EFI stub: Using DTB from configuration table > EFI stub: Exiting boot services... > "Synchronous Abort" handler, esr 0x9644, far 0x300905a65 > elr: 0022d634 lr : 00208e14 (reloc) > elr: f3f47634 lr : f3f22e14 > x0 : 000300905a4d x1 : > x2 : x3 : 0207fff0 > x4 : f3fd6470 x5 : 0207fff0 > x6 : 0004 x7 : f1f9fad0 > x8 : 7f7f7f7f7f7f7f7f x9 : d02c > x10: f1efee8c x11: d020 > x12: f1efef88 x13: ad40 > x14: f1efef2c x15: f1eff057 > x16: f3f21fc0 x17: d017a1d0 > x18: f1f11d70 x19: f1f21eb0 > x20: x21: f1f27b80 > x22: x23: > x24: x25: f3fc9ed7 > x26: b0c36000 x27: f02cd000 > x28: f03a210c x29: f1efee20 > > Code: f9400860 eb06001f 540004e0 f9400c66 (f9000c06) > UEFI image [0xf0da4000:0xf0dbbfff] '/efi\boot\fbaa64.efi' > UEFI image [0xf0386000:0xf07adfff] '/\EFI\debian\grubaa64.efi' > UEFI image [0xaf4e:0xb11d] > Resetting CPU ... > > resetting ... > > I am unable to figure out what is the difference between the 2 ways of > booting. > > Thanks for your help. Thank you for the detailed report. Can you try series [1] in particular patch [2] It has been sitting around for quite a while, unfortunately. Regards, Simon [1] https://patchwork.ozlabs.org/project/uboot/list/?series=381851 [2] https://patchwork.ozlabs.org/project/uboot/patch/20231112130245.v4.5.If206027372f73ce32480223e5626f4b944e281b7@changeid/
Re: Booting an x86-64 BIOS Machine
Hi Desone, On Tue, 14 Nov 2023 at 17:43, Desone Burns wrote: > > Hello, > > I have an application in which I need to boot an x86-64 machine with a > legacy BIOS. I've read about the U-Boot executable, the SPL, and the > TPL. I'm just having a bit of trouble figuring out which pieces are > supposed to go where. Looking to Grub, I see there exists a minimal > "boot.img" file, which gets installed into the MBR of the drive and > subsequently bootstraps the rest of the sequence. My impression was > that either the TPL or the SPL would play that same role, but even > removing basically all options, the binary produced is still too large > to fit in this 512 Byte area. Syslinux also works on my device, and > the "mbr.bin" and "gptmbr.bin" files play the same role for that > bootloader. How can I make optimizations to generate an image small > enough to fit into the MBR of my device to allow BIOS to load the rest > of the bootloader? Normally U-Boot would go in the SPI flash. If you are wanting to put U-Boot on a disk, then I suspect you need some code in the MBR to load it into RAM. Something like [1] perhaps? For that case, I doubt you need TPL and SPL...we use those on newer x86 machines, e.g. Apollo Lake - see [2] Regards, Simon [1] https://thestarman.pcministry.com/asm/mbr/STDMBR.htm [2] https://docs.u-boot.org/en/latest/board/google/chromebook_coral.html?highlight=coral
Re: [PATCH] riscv: binman: fix the load field format
Hi Randolph,
On Mon, 13 Nov 2023 at 23:26, Randolph Lin wrote:
>
> Hi Simon,
> Thanks a lot.
> On Fri, Nov 10, 2023 at 04:50:24AM -0700, Simon Glass wrote:
> > Hi Randolph,
> >
> > On Wed, Nov 8, 2023, 20:15 Randolph wrote:
> > >
> > > The #address-cells is now equal to 2. The format of the load field for
> > > the Linux kernel doesn't match.
> > >
> > > Signed-off-by: Randolph
> > > ---
> > > arch/riscv/dts/binman.dtsi | 3 ++-
> > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/arch/riscv/dts/binman.dtsi b/arch/riscv/dts/binman.dtsi
> > > index 6b4eb8dc7b..5117d7c8c9 100644
> > > --- a/arch/riscv/dts/binman.dtsi
> > > +++ b/arch/riscv/dts/binman.dtsi
> > > @@ -50,7 +50,8 @@
> > > os = "Linux";
> > > arch = "riscv";
> > > compression = "none";
> > > - load = ;
> > > + load =
> > > > > +
> > > U64_TO_U32_L(CONFIG_TEXT_BASE)>;
> >
> I just see the #address-cells changed from 1 to 2 in commit id: 5a348ccf0257.
> In my last commit for binman.dtsi (commit id: d311df8b3169), it is based on
> the value of #address-cells being 1. That's the reason for my patch
> submission.
> > Does this work?
> >
> > load = /bits 64/
> >
> Yes, it works. We use this way for the DDR memory start address above 4G
> platform. We find the example for 64bit address in the document
> tools/binman/binman.rst and use it.
> What is the method that we should continue to use in the binman.dtsi?
> 1. #address-cells = 2
> 2. append /bits/64
I believe these go together. Using (1) means you have a 2-cell value,
which is 64 bits since each cell is 32-bits.
Or perhaps I misunderstand what you are asking?
> > >
> > > linux_blob: blob-ext {
> > > filename = "Image";
> > > --
> > > 2.34.1
> > >
> >
Regards,
Simon
Re: [PATCH v4 09/12] x86: Enable SSE in 64-bit mode
Hi, On Tue, 14 Nov 2023 at 17:44, Bin Meng wrote: > > Hi Tom, > > On Wed, Nov 15, 2023 at 12:22 AM Tom Rini wrote: > > > > On Tue, Nov 14, 2023 at 09:49:08AM +0800, Bin Meng wrote: > > > Hi Tom, > > > > > > On Tue, Nov 14, 2023 at 7:52 AM Tom Rini wrote: > > > > > > > > On Tue, Nov 14, 2023 at 07:46:36AM +0800, Bin Meng wrote: > > > > > Hi Tom, > > > > > > > > > > On Tue, Nov 14, 2023 at 6:59 AM Tom Rini wrote: > > > > > > > > > > > > On Mon, Nov 13, 2023 at 03:28:13PM -0700, Simon Glass wrote: > > > > > > > Hi Bin, > > > > > > > > > > > > > > On Mon, 13 Nov 2023 at 15:08, Bin Meng wrote: > > > > > > > > > > > > > > > > Hi Simon, > > > > > > > > > > > > > > > > On Mon, Nov 13, 2023 at 4:03 AM Simon Glass > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > This is needed to support Truetype fonts. In any case, the > > > > > > > > > compiler > > > > > > > > > expects SSE to be available in 64-bit mode. Provide an option > > > > > > > > > to enable > > > > > > > > > SSE so that hardware floating-point arithmetic works. > > > > > > > > > > > > > > > > > > Signed-off-by: Simon Glass > > > > > > > > > Suggested-by: Bin Meng > > > > > > > > > --- > > > > > > > > > > > > > > > > > > Changes in v4: > > > > > > > > > - Use a Kconfig option > > > > > > > > > > > > > > > > > > arch/x86/Kconfig | 8 > > > > > > > > > arch/x86/config.mk| 4 > > > > > > > > > arch/x86/cpu/x86_64/cpu.c | 12 > > > > > > > > > drivers/video/Kconfig | 1 + > > > > > > > > > 4 files changed, 25 insertions(+) > > > > > > > > > > > > > > > > > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > > > > > > > > > index 99e59d94c606..6b532d712ee8 100644 > > > > > > > > > --- a/arch/x86/Kconfig > > > > > > > > > +++ b/arch/x86/Kconfig > > > > > > > > > @@ -723,6 +723,14 @@ config ROM_TABLE_SIZE > > > > > > > > > hex > > > > > > > > > default 0x1 > > > > > > > > > > > > > > > > > > +config X86_HARDFP > > > > > > > > > + bool "Support hardware floating point" > > > > > > > > > + help > > > > > > > > > + U-Boot generally does not make use of floating > > > > > > > > > point. Where this is > > > > > > > > > + needed, it can be enabled using this option. This > > > > > > > > > adjusts the > > > > > > > > > + start-up code for 64-bit mode and changes the > > > > > > > > > compiler options for > > > > > > > > > + 64-bit to enable SSE. > > > > > > > > > > > > > > > > As discussed in another thread, this option should be made > > > > > > > > global to > > > > > > > > all architectures and by default no. > > > > > > > > > > > > > > > > > + > > > > > > > > > config HAVE_ITSS > > > > > > > > > bool "Enable ITSS" > > > > > > > > > help > > > > > > > > > diff --git a/arch/x86/config.mk b/arch/x86/config.mk > > > > > > > > > index 26ec1af2f0b0..2e3a7119e798 100644 > > > > > > > > > --- a/arch/x86/config.mk > > > > > > > > > +++ b/arch/x86/config.mk > > > > > > > > > @@ -27,9 +27,13 @@ ifeq ($(IS_32BIT),y) > > > > > > > > > PLATFORM_CPPFLAGS += -march=i386 -m32 > > > > > > > > > else > > > > > > > > > PLATFORM_C
Re: [PATCH v3 2/5] firmware: scmi: support protocols on sandbox only if enabled
On Mon, 13 Nov 2023 at 19:14, AKASHI Takahiro wrote: > > This change will be useful when we manually test SCMI on sandbox > by enabling/disabling a specific SCMI protocol. > > Signed-off-by: AKASHI Takahiro > --- > v9 > * use CONFIG_IS_ENABLED() rather than IS_ENABLED() > * remove goto by introducing a not_supported() function > --- > drivers/firmware/scmi/sandbox-scmi_agent.c | 30 ++-- > drivers/firmware/scmi/sandbox-scmi_devices.c | 78 > 2 files changed, 71 insertions(+), 37 deletions(-) Reviewed-by: Simon Glass
Re: [PATCH] spl: fix TPL_SYS_MALLOC_F description
On Tue, 14 Nov 2023 at 04:30, John Keeping wrote: > > This config option enables the malloc() pool in TPL not the SPL. Fix > the description to accurately reflect this. > > Fixes: fd8497dae54 (spl: Create proper symbols for enabling the malloc() pool) > Signed-off-by: John Keeping > --- > Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Simon Glass
Re: [PATCH v3 1/5] test: dm: skip scmi tests against disabled protocols
On Mon, 13 Nov 2023 at 19:14, AKASHI Takahiro wrote: > > This is a precautionary change to make scmi tests workable whether or not > a specific protocol be enabled. If a given protocol is not configured, > we skip the test by returning -EAGAIN. > > Signed-off-by: AKASHI Takahiro > --- > v9 > * return -EAGAIN if we want to skip a test > * use CONFIG_IS_ENABLED() rather than IS_ENABLED() > --- > test/dm/scmi.c | 12 > 1 file changed, 12 insertions(+) Reviewed-by: Simon Glass
Re: [PATCH 5/5] test: dm: add scmi command test
Hi,
On Mon, 13 Nov 2023 at 18:41, AKASHI Takahiro
wrote:
>
> On Mon, Nov 13, 2023 at 11:01:17AM -0700, Simon Glass wrote:
> > Hi,
> >
> > On Sun, 12 Nov 2023 at 18:46, AKASHI Takahiro
> > wrote:
> > >
> > > Hi Tom,
> > >
> > > On Fri, Nov 10, 2023 at 01:21:37PM -0500, Tom Rini wrote:
> > > > On Wed, Oct 25, 2023 at 02:14:27PM +0900, AKASHI Takahiro wrote:
> > > >
> > > > > In this test, "scmi" command is tested against different sub-commands.
> > > > > Please note that scmi command is for debug purpose and is not intended
> > > > > in production system.
> > > > >
> > > > > Signed-off-by: AKASHI Takahiro
> > > > > Reviewed-by: Simon Glass
> > > > > Reviewed-by: Etienne Carriere
> > > >
> > > > The test part of this still fails:
> > > > https://source.denx.de/u-boot/u-boot/-/jobs/732077
> > > >
> > > > I don't know why more output wasn't captured, when I run it locally
> > > > instead I get:
> > > > == FAILURES
> > > > ===
> > > > ___ test_ut[ut_dm_dm_test_scmi_cmd]
> > > > ___
> > > > test/py/u_boot_spawn.py:195: in expect
> > > > c = os.read(self.fd, 1024).decode(errors='replace')
> > > > E OSError: [Errno 5] Input/output error
> > > >
> > > > During handling of the above exception, another exception occurred:
> > > > test/py/tests/test_ut.py:502: in test_ut
> > > > output = u_boot_console.run_command('ut ' + ut_subtest)
> > > > test/py/u_boot_console_base.py:266: in run_command
> > > > m = self.p.expect([self.prompt_compiled] + self.bad_patterns)
> > > > test/py/u_boot_spawn.py:204: in expect
> > > > raise ValueError('U-Boot exited with %s' % info)
> > > > E ValueError: U-Boot exited with signal 11 (SIGSEGV)
> > >
> > >
> > > The command uses global variables which hold pointers to 'struct udevice'
> > > which are to be shared between the main and the sub-commands.
> > > Since pytest framework executes ut tests twice, once with a (normal?)
> > > device
> > > tree and once with a flat tree, udevices will be *voided* between
> > > two executions.
> >
> > Are you able to put the var in the uclass-priv data instead? The state
> > should be cleared before running each DM test.
>
> Well, I don't think we need such a trick.
> As you can see, we may simply fetch/find necessary udevices
> every time the command is called.
> It is enough given that the command is mainly for debug purpose.
OK. Let's see how it goes. When you mention global vars I get a bit
nervous, but we do have these in some places.
Regards,
Simon
>
> -Takahiro Akashi
>
>
> > Regards,
> > Simon
> >
> >
> > >
> > > I will fix it in v2.
> > >
> > > Thanks,
> > > -Takahiro Akashi
> > >
> > >
> > > > Captured stdout call
> > > > -
> > > > => ut dm dm_test_scmi_cmd
> > > > Test: dm_test_scmi_cmd: scmi.c
> > > > SCMI device: scmi
> > > > protocol version: 0x2
> > > > # of agents: 2
> > > > 0: platform
> > > > > 1: OSPM
> > > > # of protocols: 4
> > > > Power domain management
> > > > Clock management
> > > > Reset domain management
> > > > Voltage domain management
> > > > vendor: U-Boot
> > > > sub vendor: Sandbox
> > > > impl version: 0x1
> > > > Denying access to device:0 failed (-13)
> > > > Denying access to protocol:0x14 on device:0 failed (-13)
> > > > Reset failed (-13)
> > > > Test: dm_test_scmi_cmd: scmi.c (flat tree)
> > > > SCMI device: Q
> > > > === short test summary info
> > > > ===
> > > > FAILED test/py/tests/test_ut.py::test_ut[ut_dm_dm_test_scmi_cmd] -
> > > > ValueError: U-Boot exited...
> > > >
> > > > --
> > > > Tom
> > >
> > >
Re: [PATCH 01/13] sandbox: move asm-generic include to the end of file
On Tue, 14 Nov 2023 at 04:03, Igor Prusov wrote: > > Generic version of io.h should be included at the end of > architecture-specific ones to make sure that arch implementations are > used and to avoid redefinitions. > > Signed-off-by: Igor Prusov > --- > > arch/sandbox/include/asm/io.h | 28 ++-- > 1 file changed, 14 insertions(+), 14 deletions(-) Reviewed-by: Simon Glass
Re: [PATCH v4 09/12] x86: Enable SSE in 64-bit mode
Hi Bin,
On Mon, 13 Nov 2023 at 15:08, Bin Meng wrote:
>
> Hi Simon,
>
> On Mon, Nov 13, 2023 at 4:03 AM Simon Glass wrote:
> >
> > This is needed to support Truetype fonts. In any case, the compiler
> > expects SSE to be available in 64-bit mode. Provide an option to enable
> > SSE so that hardware floating-point arithmetic works.
> >
> > Signed-off-by: Simon Glass
> > Suggested-by: Bin Meng
> > ---
> >
> > Changes in v4:
> > - Use a Kconfig option
> >
> > arch/x86/Kconfig | 8
> > arch/x86/config.mk| 4
> > arch/x86/cpu/x86_64/cpu.c | 12
> > drivers/video/Kconfig | 1 +
> > 4 files changed, 25 insertions(+)
> >
> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > index 99e59d94c606..6b532d712ee8 100644
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -723,6 +723,14 @@ config ROM_TABLE_SIZE
> > hex
> > default 0x1
> >
> > +config X86_HARDFP
> > + bool "Support hardware floating point"
> > + help
> > + U-Boot generally does not make use of floating point. Where this
> > is
> > + needed, it can be enabled using this option. This adjusts the
> > + start-up code for 64-bit mode and changes the compiler options for
> > + 64-bit to enable SSE.
>
> As discussed in another thread, this option should be made global to
> all architectures and by default no.
>
> > +
> > config HAVE_ITSS
> > bool "Enable ITSS"
> > help
> > diff --git a/arch/x86/config.mk b/arch/x86/config.mk
> > index 26ec1af2f0b0..2e3a7119e798 100644
> > --- a/arch/x86/config.mk
> > +++ b/arch/x86/config.mk
> > @@ -27,9 +27,13 @@ ifeq ($(IS_32BIT),y)
> > PLATFORM_CPPFLAGS += -march=i386 -m32
> > else
> > PLATFORM_CPPFLAGS += $(if $(CONFIG_SPL_BUILD),,-fpic) -fno-common
> > -march=core2 -m64
> > +
> > +ifndef CONFIG_X86_HARDFP
> > PLATFORM_CPPFLAGS += -mno-mmx -mno-sse
> > endif
> >
> > +endif # IS_32BIT
> > +
> > PLATFORM_RELFLAGS += -fdata-sections -ffunction-sections
> > -fvisibility=hidden
> >
> > KBUILD_LDFLAGS += -Bsymbolic -Bsymbolic-functions
> > diff --git a/arch/x86/cpu/x86_64/cpu.c b/arch/x86/cpu/x86_64/cpu.c
> > index 2647bff891f8..5ea746ecce4d 100644
> > --- a/arch/x86/cpu/x86_64/cpu.c
> > +++ b/arch/x86/cpu/x86_64/cpu.c
> > @@ -10,6 +10,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > DECLARE_GLOBAL_DATA_PTR;
> >
> > @@ -39,11 +40,22 @@ int x86_mp_init(void)
> > return 0;
> > }
> >
> > +/* enable SSE features for hardware floating point */
> > +static void setup_sse_features(void)
> > +{
> > + asm ("mov %%cr4, %%rax\n" \
> > + "or %0, %%rax\n" \
> > + "mov %%rax, %%cr4\n" \
> > + : : "i" (X86_CR4_OSFXSR | X86_CR4_OSXMMEXCPT) : "eax");
> > +}
> > +
> > int x86_cpu_reinit_f(void)
> > {
> > /* set the vendor to Intel so that native_calibrate_tsc() works */
> > gd->arch.x86_vendor = X86_VENDOR_INTEL;
> > gd->arch.has_mtrr = true;
> > + if (IS_ENABLED(CONFIG_X86_HARDFP))
> > + setup_sse_features();
> >
> > return 0;
> > }
> > diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> > index 6f319ba0d544..39c82521be16 100644
> > --- a/drivers/video/Kconfig
> > +++ b/drivers/video/Kconfig
> > @@ -180,6 +180,7 @@ config CONSOLE_ROTATION
> >
> > config CONSOLE_TRUETYPE
> > bool "Support a console that uses TrueType fonts"
> > + select X86_HARDFP if X86
>
> This should be "depends on HARDFP", indicating that the TrueType
> library is using hardware fp itself, and user has to explicitly turn
> the hardware fp Kconfig option on.
So you mean 'depends on HARDFP if X86' ? After all, this is only for
X86 - other archs can use softfp which is already enabled, as I
understand it.
>
> "Select" does not work for architectures that does not have the
> "enabling hardware fp" logic in place.
>
> > help
> > TrueTrype fonts can provide outline-drawing capability rather than
> > needing to provide a bitmap for each font and size that is needed.
> > --
I still don't think we are on the same page here. I would prefer to
just enable the options without any option. I really don't want to get
into RISC-V stuff - that is a separate concern.
>From my POV it seems that x86 is special in that:
- it uses hardfp
- hardfp is always available in any CPU with 64-bit support (I think?)
So please can you be a bit more specific here?
Regards,
Simon
Re: U-Booters at LPC
Hi Sean, On Mon, 13 Nov 2023 at 11:15, Sean Anderson wrote: > > Hi All, > > I'm at LPC this week, and I'd love to chat with anyone else who's there > in person. That would be good, but sadly I am not :-( Regards, Simon
Re: [PATCH 1/1] efi_loader: improve efi_var_from_file() description
On Mon, 13 Nov 2023 at 07:50, Heinrich Schuchardt wrote: > > It is unclear to developers why efi_var_from_file() returns EFI_SUCCESS if > file ubootefi.var is missing or corrupted. Improve the description. > > Reported-by: Weizhao Ouyang > Signed-off-by: Heinrich Schuchardt > --- > lib/efi_loader/efi_var_file.c | 7 +-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/lib/efi_loader/efi_var_file.c b/lib/efi_loader/efi_var_file.c > index 62e071bd83..dbf76f93de 100644 > --- a/lib/efi_loader/efi_var_file.c > +++ b/lib/efi_loader/efi_var_file.c > @@ -204,8 +204,11 @@ efi_status_t efi_var_restore(struct efi_var_file *buf, > bool safe) > * File ubootefi.var is read from the EFI system partitions and the variables > * stored in the file are created. > * > - * In case the file does not exist yet or a variable cannot be set > EFI_SUCCESS > - * is returned. > + * On first boot the file ubootefi.var does not exist yet. This is if why we s/if// Reviewed-by: Simon Glass > + * must return EFI_SUCCESS in this case. > + * > + * If the variable file is corrupted, e.g. incorrect CRC32, we do not want to > + * stop the boot process. We deliberately return EFI_SUCCESS in this case, > too. Does this mean that the file will be erased? Doesn't that mean that all the settings are lost? > * > * Return: status code > */ > -- > 2.40.1 > Regards, Simon
Re: [PATCH 5/5] test: dm: add scmi command test
Hi,
On Sun, 12 Nov 2023 at 18:46, AKASHI Takahiro
wrote:
>
> Hi Tom,
>
> On Fri, Nov 10, 2023 at 01:21:37PM -0500, Tom Rini wrote:
> > On Wed, Oct 25, 2023 at 02:14:27PM +0900, AKASHI Takahiro wrote:
> >
> > > In this test, "scmi" command is tested against different sub-commands.
> > > Please note that scmi command is for debug purpose and is not intended
> > > in production system.
> > >
> > > Signed-off-by: AKASHI Takahiro
> > > Reviewed-by: Simon Glass
> > > Reviewed-by: Etienne Carriere
> >
> > The test part of this still fails:
> > https://source.denx.de/u-boot/u-boot/-/jobs/732077
> >
> > I don't know why more output wasn't captured, when I run it locally
> > instead I get:
> > == FAILURES
> > ===
> > ___ test_ut[ut_dm_dm_test_scmi_cmd]
> > ___
> > test/py/u_boot_spawn.py:195: in expect
> > c = os.read(self.fd, 1024).decode(errors='replace')
> > E OSError: [Errno 5] Input/output error
> >
> > During handling of the above exception, another exception occurred:
> > test/py/tests/test_ut.py:502: in test_ut
> > output = u_boot_console.run_command('ut ' + ut_subtest)
> > test/py/u_boot_console_base.py:266: in run_command
> > m = self.p.expect([self.prompt_compiled] + self.bad_patterns)
> > test/py/u_boot_spawn.py:204: in expect
> > raise ValueError('U-Boot exited with %s' % info)
> > E ValueError: U-Boot exited with signal 11 (SIGSEGV)
>
>
> The command uses global variables which hold pointers to 'struct udevice'
> which are to be shared between the main and the sub-commands.
> Since pytest framework executes ut tests twice, once with a (normal?) device
> tree and once with a flat tree, udevices will be *voided* between
> two executions.
Are you able to put the var in the uclass-priv data instead? The state
should be cleared before running each DM test.
Regards,
Simon
>
> I will fix it in v2.
>
> Thanks,
> -Takahiro Akashi
>
>
> > Captured stdout call
> > -
> > => ut dm dm_test_scmi_cmd
> > Test: dm_test_scmi_cmd: scmi.c
> > SCMI device: scmi
> > protocol version: 0x2
> > # of agents: 2
> > 0: platform
> > > 1: OSPM
> > # of protocols: 4
> > Power domain management
> > Clock management
> > Reset domain management
> > Voltage domain management
> > vendor: U-Boot
> > sub vendor: Sandbox
> > impl version: 0x1
> > Denying access to device:0 failed (-13)
> > Denying access to protocol:0x14 on device:0 failed (-13)
> > Reset failed (-13)
> > Test: dm_test_scmi_cmd: scmi.c (flat tree)
> > SCMI device: Q
> > === short test summary info
> > ===
> > FAILED test/py/tests/test_ut.py::test_ut[ut_dm_dm_test_scmi_cmd] -
> > ValueError: U-Boot exited...
> >
> > --
> > Tom
>
>
Re: [PATCH v2 2/5] firmware: scmi: support protocols on sandbox only if enabled
Hi AKASHI,
On Sun, 12 Nov 2023 at 18:49, AKASHI Takahiro
wrote:
>
> This change will be useful when we manually test SCMI on sandbox
> by enabling/disabling a specific SCMI protocol.
>
> Signed-off-by: AKASHI Takahiro
> ---
> drivers/firmware/scmi/sandbox-scmi_agent.c | 27 ++-
> drivers/firmware/scmi/sandbox-scmi_devices.c | 78
> 2 files changed, 72 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/firmware/scmi/sandbox-scmi_agent.c
> b/drivers/firmware/scmi/sandbox-scmi_agent.c
> index d13180962662..1fc9a0f4ea7e 100644
> --- a/drivers/firmware/scmi/sandbox-scmi_agent.c
> +++ b/drivers/firmware/scmi/sandbox-scmi_agent.c
> @@ -66,10 +66,18 @@ struct scmi_channel {
> };
>
> static u8 protocols[] = {
> +#if IS_ENABLED(CONFIG_SCMI_POWER_DOMAIN)
> SCMI_PROTOCOL_ID_POWER_DOMAIN,
Is this better? Perhaps not!
CONFIG_IS_ENABLED(SCMI_POWER_DOMAIN, (SCMI_PROTOCOL_ID_POWER_DOMAIN,))
> +#endif
> +#if IS_ENABLED(CONFIG_CLK_SCMI)
> SCMI_PROTOCOL_ID_CLOCK,
> +#endif
> +#if IS_ENABLED(CONFIG_RESET_SCMI)
> SCMI_PROTOCOL_ID_RESET_DOMAIN,
> +#endif
> +#if IS_ENABLED(CONFIG_DM_REGULATOR_SCMI)
> SCMI_PROTOCOL_ID_VOLTAGE_DOMAIN,
> +#endif
> };
>
> #define NUM_PROTOCOLS ARRAY_SIZE(protocols)
> @@ -1160,6 +1168,9 @@ static int sandbox_scmi_test_process_msg(struct udevice
> *dev,
> }
> break;
> case SCMI_PROTOCOL_ID_POWER_DOMAIN:
> + if (!IS_ENABLED(CONFIG_SCMI_POWER_DOMAIN))
> + goto not_supported;
> +
> switch (msg->message_id) {
> case SCMI_PROTOCOL_VERSION:
> return sandbox_scmi_pwd_protocol_version(dev, msg);
> @@ -1180,6 +1191,9 @@ static int sandbox_scmi_test_process_msg(struct udevice
> *dev,
> }
> break;
> case SCMI_PROTOCOL_ID_CLOCK:
> + if (!IS_ENABLED(CONFIG_CLK_SCMI))
> + goto not_supported;
How about putting this all in a function and avoiding the goto?
> +
> switch (msg->message_id) {
> case SCMI_PROTOCOL_ATTRIBUTES:
> return sandbox_scmi_clock_protocol_attribs(dev, msg);
> @@ -1196,6 +1210,9 @@ static int sandbox_scmi_test_process_msg(struct udevice
> *dev,
> }
> break;
> case SCMI_PROTOCOL_ID_RESET_DOMAIN:
> + if (!IS_ENABLED(CONFIG_RESET_SCMI))
> + goto not_supported;
> +
> switch (msg->message_id) {
> case SCMI_RESET_DOMAIN_ATTRIBUTES:
> return sandbox_scmi_rd_attribs(dev, msg);
> @@ -1206,6 +1223,9 @@ static int sandbox_scmi_test_process_msg(struct udevice
> *dev,
> }
> break;
> case SCMI_PROTOCOL_ID_VOLTAGE_DOMAIN:
> + if (!IS_ENABLED(CONFIG_DM_REGULATOR_SCMI))
> + goto not_supported;
> +
> switch (msg->message_id) {
> case SCMI_VOLTAGE_DOMAIN_ATTRIBUTES:
> return sandbox_scmi_voltd_attribs(dev, msg);
> @@ -1224,8 +1244,7 @@ static int sandbox_scmi_test_process_msg(struct udevice
> *dev,
> case SCMI_PROTOCOL_ID_SYSTEM:
> case SCMI_PROTOCOL_ID_PERF:
> case SCMI_PROTOCOL_ID_SENSOR:
> - *(u32 *)msg->out_msg = SCMI_NOT_SUPPORTED;
> - return 0;
> + goto not_supported;
> default:
> break;
> }
> @@ -1239,6 +1258,10 @@ static int sandbox_scmi_test_process_msg(struct
> udevice *dev,
> /* Intentionnaly report unhandled IDs through the SCMI return code */
> *(u32 *)msg->out_msg = SCMI_PROTOCOL_ERROR;
> return 0;
> +
> +not_supported:
> + *(u32 *)msg->out_msg = SCMI_NOT_SUPPORTED;
> + return 0;
> }
>
> static int sandbox_scmi_test_remove(struct udevice *dev)
> diff --git a/drivers/firmware/scmi/sandbox-scmi_devices.c
> b/drivers/firmware/scmi/sandbox-scmi_devices.c
> index facb5b06ffb5..0519cf889aa9 100644
> --- a/drivers/firmware/scmi/sandbox-scmi_devices.c
> +++ b/drivers/firmware/scmi/sandbox-scmi_devices.c
> @@ -62,12 +62,13 @@ static int sandbox_scmi_devices_remove(struct udevice
> *dev)
> if (!devices)
> return 0;
>
> - for (n = 0; n < SCMI_TEST_DEVICES_RD_COUNT; n++) {
> - int ret2 = reset_free(devices->reset + n);
> + if (IS_ENABLED(CONFIG_RESET_SCMI))
> + for (n = 0; n < SCMI_TEST_DEVICES_RD_COUNT; n++) {
> + int ret2 = reset_free(devices->reset + n);
>
> - if (ret2 && !ret)
> - ret = ret2;
> - }
> + if (ret2 && !ret)
> + ret = ret2;
> + }
>
> return ret;
> }
> @@ -89,39 +90,53 @@ static int sandbox_scmi_devices_probe(struct udevice *dev)
> .regul_count =
Re: [PATCH 2/4] serial: s5p: Use livetree API to get "id" property
Hi Sam,
On Fri, 10 Nov 2023 at 11:29, Sam Protsenko wrote:
>
> Hi Simon,
>
> On Tue, Nov 7, 2023 at 10:26 PM Simon Glass wrote:
> >
> > Hi Sam,
> >
> > On Tue, 7 Nov 2023 at 12:06, Sam Protsenko
> > wrote:
> > >
> > > Use dev_read_u8_default() instead of fdtdec_get_int() to read the "id"
> > > property from device tree, as suggested in [1]. dev_* API is already
> > > used in this driver, so there is no reason to stick to fdtdec_* API.
> > > This also fixes checkpatch warning:
> > >
> > > WARNING: Use the livetree API (dev_read_...)
> > >
> > > [1] doc/develop/driver-model/livetree.rst
> > >
> > > Signed-off-by: Sam Protsenko
> > > ---
> > > drivers/serial/serial_s5p.c | 5 +
> > > 1 file changed, 1 insertion(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/serial/serial_s5p.c b/drivers/serial/serial_s5p.c
> > > index 177215535676..c57bdd059ea6 100644
> > > --- a/drivers/serial/serial_s5p.c
> > > +++ b/drivers/serial/serial_s5p.c
> > > @@ -20,8 +20,6 @@
> > > #include
> > > #include
> > >
> > > -DECLARE_GLOBAL_DATA_PTR;
> > > -
> > > enum {
> > > PORT_S5P = 0,
> > > PORT_S5L
> > > @@ -220,8 +218,7 @@ static int s5p_serial_of_to_plat(struct udevice *dev)
> > >
> > > plat->reg = (struct s5p_uart *)addr;
> > > plat->reg_width = dev_read_u32_default(dev, "reg-io-width", 1);
> > > - plat->port_id = fdtdec_get_int(gd->fdt_blob, dev_of_offset(dev),
> > > - "id", dev_seq(dev));
> > > + plat->port_id = dev_read_u8_default(dev, "id", dev_seq(dev));
> > >
> > > if (port_type == PORT_S5L) {
> > > plat->rx_fifo_count_shift = S5L_RX_FIFO_COUNT_SHIFT;
> > > --
> > > 2.39.2
> > >
> >
> > Really this property should not be needed anymore. Can you figure out
> > how to drop it?
> >
>
> The 'port_id' seems to be needed for ARCH_EXYNOS4 boards. Because
> Exynos4 doesn't have proper DM clocks, it uses 'id' property to get
> corresponding UART clock frequency from its mach code.
>
> Here is what's happening in the serial driver in case of Exynos4:
>
> 1. get_uart_clk(port_id) is called
> 2. which in turn calls exynos4_get_uart_clk(port_id)
> 3. which uses "port_id" value to read corresponding bits of of
> CLK_SRC_PERIL0 register
> 4. those bits are used to get corresponding PLL clock's frequency
> 5. which is in turn used to calculate UART clock rate
> 6. calculated rate is returned by get_uart_clk() to serial driver
>
> So I don't see any *easy* way we can get rid of that id property.
>
> The proper way of doing that would require converting Exynos4 clock
> code to CCF (enabling CONFIG_CLK_EXYNOS). Which of course also means
> implementing clocks in dts, akin to kernel's exynos4.dtsi. Then it'll
> be possible to get rid of 'id' property.
That sounds good!
>
> Maybe I'm missing something, please let me know.
An easy way in the meantime would be to look at the compatible / reg
property, e.g. here is a sketch:
static int get_id(ofnode node)
{
ulong addr = (ulong)plat->reg;
if (ofnode_device_is_compatible(node, "samsung,exynos4210-uart")) {
return (addr >> 16) & 0xf;
...
reg = <0x1380 0x3c>;
id = <0>;
};
serail_1: serial@1381 {
compatible = "samsung,exynos4210-uart";
reg = <0x1381 0x3c>;
id = <1>;
};
serial_2: serial@1382 {
compatible = "samsung,exynos4210-uart";
reg = <0x1382 0x3c>;
id = <2>;
};
serial_3: serial@1383 {
compatible = "samsung,exynos4210-uart";
reg = <0x1383 0x3c>;
id = <3>;
};
serial_4: serial@1384 {
compatible = "samsung,exynos4210-uart";
reg = <0x1384 0x3c>;
id = <4>;
Regards,
Simon
Re: [PATCH v2 1/5] test: dm: skip scmi tests against disabled protocols
Hi AKASHI, On Sun, 12 Nov 2023 at 18:49, AKASHI Takahiro wrote: > > This is a precautionary change to make scmi tests workable whether or not > a specific protocol be enabled. > > Signed-off-by: AKASHI Takahiro > --- > test/dm/scmi.c | 12 > 1 file changed, 12 insertions(+) > > diff --git a/test/dm/scmi.c b/test/dm/scmi.c > index da45314f2e4c..2f63f2da16fb 100644 > --- a/test/dm/scmi.c > +++ b/test/dm/scmi.c > @@ -217,6 +217,9 @@ static int dm_test_scmi_power_domains(struct > unit_test_state *uts) > u8 *name; > int ret; > > + if (!IS_ENABLED(CONFIG_SCMI_POWER_DOMAIN)) > + return 0; -EAGAIN to skip a test Please update a comment if this needs to be documented better > + > /* preparation */ > ut_assertok(load_sandbox_scmi_test_devices(uts, , )); > ut_assertnonnull(agent); > @@ -317,6 +320,9 @@ static int dm_test_scmi_clocks(struct unit_test_state > *uts) > int ret_dev; > int ret; > > + if (!IS_ENABLED(CONFIG_CLK_SCMI)) > + return 0; > + > ret = load_sandbox_scmi_test_devices(uts, , ); > if (ret) > return ret; > @@ -382,6 +388,9 @@ static int dm_test_scmi_resets(struct unit_test_state > *uts) > struct udevice *agent_dev, *reset_dev, *dev = NULL; > int ret; > > + if (!IS_ENABLED(CONFIG_RESET_SCMI)) > + return 0; > + > ret = load_sandbox_scmi_test_devices(uts, , ); > if (ret) > return ret; > @@ -418,6 +427,9 @@ static int dm_test_scmi_voltage_domains(struct > unit_test_state *uts) > struct udevice *dev; > struct udevice *regul0_dev; > > + if (!IS_ENABLED(CONFIG_DM_REGULATOR_SCMI)) > + return 0; > + > ut_assertok(load_sandbox_scmi_test_devices(uts, , )); > > scmi_devices = sandbox_scmi_devices_ctx(dev); > -- > 2.34.1 > Regards, Simon
Re: [PATCH v3 09/12] x86: Enable SSE in 64-bit mode
Hi Tom, On Mon, 13 Nov 2023 at 07:06, Tom Rini wrote: > > On Mon, Nov 13, 2023 at 09:01:02PM +0800, Bin Meng wrote: > > Hi Simon, > > > > On Mon, Nov 13, 2023 at 4:02 AM Simon Glass wrote: > > > > > > Hi Bin, > > > > > > On Mon, 6 Nov 2023 at 08:36, Tom Rini wrote: > > > > > > > > On Mon, Nov 06, 2023 at 06:26:15PM +0800, Bin Meng wrote: > > > > > + Tom, > > > > > > > > > > Hi Simon, > > > > > > > > > > On Mon, Nov 6, 2023 at 12:29 AM Simon Glass wrote: > > > > > > > > > > > > Hi Bin, > > > > > > > > > > > > On Sun, 5 Nov 2023 at 14:05, Bin Meng wrote: > > > > > > > > > > > > > > Hi Simon, > > > > > > > > > > > > > > On Mon, Oct 2, 2023 at 9:15 AM Simon Glass > > > > > > > wrote: > > > > > > > > > > > > > > > > This is needed to support Truetype fonts. In any case, the > > > > > > > > compiler > > > > > > > > expects SSE to be available in 64-bit mode. Enable it. > > > > > > > > > > > > > > > > Signed-off-by: Simon Glass > > > > > > > > Suggested-by: Bin Meng > > > > > > > > --- > > > > > > > > > > > > > > > > (no changes since v1) > > > > > > > > > > > > > > > > arch/x86/config.mk| 1 - > > > > > > > > arch/x86/cpu/x86_64/cpu.c | 11 +++ > > > > > > > > 2 files changed, 11 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > > > > > > > > > I didn't suggest we enable SSE for x86. This is the wrong > > > > > > > approach. > > > > > > > > > > > > > > We should rewrite the Truetype support codes to avoid using > > > > > > > float/double types. > > > > > > > > > > > > > > This way the Truetype codes can be used on any other architectures > > > > > > > without the need for the compiler to emit explicit floating > > > > > > > instructions. > > > > > > > > > > > > I am not aware of any such library. At present, enabling truetype on > > > > > > coreboot64 causes a hang. > > > > > > > > > > > > > > > > If that's the case, we will have to either: > > > > > > > > > > - Switch all U-Boot builds' to use software float (e.g. -msoft-float) > > > > > which unfortunately depends on the compiler runtime intrinsics. > > > > > - Introduce a Kconfig option for hard float enabling and let each > > > > > architecture to decide whether it implements it or not, and update > > > > > Truetype to depend on the hard float. > > > > > > > > We generally do -msoft-float already, so introducing that for x86, and > > > > some Kconfig logic to ensure that no one else steps on this particular > > > > bug sounds reasonable. > > > > > > Yes soft float seems to be not-much-used on x86. For 64-bit chips the > > > compiler seems to assume that hardfp is available. > > > > We have compiler flags to ensure the compiler does not generate SSE > > instructions. Yes, I know SSE is in almost every x86 processor we see > > nowadays. > > > > > > > > So perhaps the best thing is to introduce a HARDFP option to x86 only. > > > > This option should be global as some other arches also don't have > > hardware fp, like RISC-V whose fp extension is optional. > > RISC-V should take the ARM approach (and what I was suggesting for x86) > and enforce soft-float for everyone. But see my comment above...I'm just not sure softfp is widely used on x86. It also is a bit silly, since 64-bit CPUs have hardfp and it is trivial to enable (in fact we have to disable it if we don;t want it). Bin, please let me know what you think. Regards, Simon
Re: [PATCH v3 09/12] x86: Enable SSE in 64-bit mode
Hi Bin, On Mon, 13 Nov 2023 at 06:01, Bin Meng wrote: > > Hi Simon, > > On Mon, Nov 13, 2023 at 4:02 AM Simon Glass wrote: > > > > Hi Bin, > > > > On Mon, 6 Nov 2023 at 08:36, Tom Rini wrote: > > > > > > On Mon, Nov 06, 2023 at 06:26:15PM +0800, Bin Meng wrote: > > > > + Tom, > > > > > > > > Hi Simon, > > > > > > > > On Mon, Nov 6, 2023 at 12:29 AM Simon Glass wrote: > > > > > > > > > > Hi Bin, > > > > > > > > > > On Sun, 5 Nov 2023 at 14:05, Bin Meng wrote: > > > > > > > > > > > > Hi Simon, > > > > > > > > > > > > On Mon, Oct 2, 2023 at 9:15 AM Simon Glass > > > > > > wrote: > > > > > > > > > > > > > > This is needed to support Truetype fonts. In any case, the > > > > > > > compiler > > > > > > > expects SSE to be available in 64-bit mode. Enable it. > > > > > > > > > > > > > > Signed-off-by: Simon Glass > > > > > > > Suggested-by: Bin Meng > > > > > > > --- > > > > > > > > > > > > > > (no changes since v1) > > > > > > > > > > > > > > arch/x86/config.mk| 1 - > > > > > > > arch/x86/cpu/x86_64/cpu.c | 11 +++ > > > > > > > 2 files changed, 11 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > > > > > > I didn't suggest we enable SSE for x86. This is the wrong approach. > > > > > > > > > > > > We should rewrite the Truetype support codes to avoid using > > > > > > float/double types. > > > > > > > > > > > > This way the Truetype codes can be used on any other architectures > > > > > > without the need for the compiler to emit explicit floating > > > > > > instructions. > > > > > > > > > > I am not aware of any such library. At present, enabling truetype on > > > > > coreboot64 causes a hang. > > > > > > > > > > > > > If that's the case, we will have to either: > > > > > > > > - Switch all U-Boot builds' to use software float (e.g. -msoft-float) > > > > which unfortunately depends on the compiler runtime intrinsics. > > > > - Introduce a Kconfig option for hard float enabling and let each > > > > architecture to decide whether it implements it or not, and update > > > > Truetype to depend on the hard float. > > > > > > We generally do -msoft-float already, so introducing that for x86, and > > > some Kconfig logic to ensure that no one else steps on this particular > > > bug sounds reasonable. > > > > Yes soft float seems to be not-much-used on x86. For 64-bit chips the > > compiler seems to assume that hardfp is available. > > We have compiler flags to ensure the compiler does not generate SSE > instructions. Yes, I know SSE is in almost every x86 processor we see > nowadays. > > > > > So perhaps the best thing is to introduce a HARDFP option to x86 only. > > This option should be global as some other arches also don't have > hardware fp, like RISC-V whose fp extension is optional. Here is the patch I have: https://patchwork.ozlabs.org/project/uboot/patch/20231112200255.172351-5-...@chromium.org/ So perhaps move it to arch/Kconfig , rename it to HARDFP and update the help to be more generic? Regards, Simon
Re: [PATCH v2 1/1] acpi: cannot have RSDT above 4 GiB
On Sun, 12 Nov 2023 at 16:54, Heinrich Schuchardt wrote: > > The field RsdtAddress has only 32 bit. The RSDT table cannot be located > beyond 4 GiB. > > Signed-off-by: Heinrich Schuchardt > --- > v2: > Avoid superfluous 0 assignment. RSDP is already zeroed out. > Use constants form linux/sizes.h > --- > lib/acpi/base.c | 23 --- > 1 file changed, 16 insertions(+), 7 deletions(-) Reviewed-by: Simon Glass
[PATCH 8/9] sysinfo: Allow displaying more info on startup
At present only the model name is shown on start. Some boards want to
display more information. Add some more options to allow display of the
manufacturer as well as the version and date of any prior-stage
firmware.
This is useful for coreboot, at least. If other boards have more
information to display, it is easy to add it, now.
Signed-off-by: Simon Glass
---
common/board_info.c | 72 +
drivers/sysinfo/Kconfig | 7
include/sysinfo.h | 3 ++
3 files changed, 61 insertions(+), 21 deletions(-)
diff --git a/common/board_info.c b/common/board_info.c
index a62c04794b90..f4c385add90c 100644
--- a/common/board_info.c
+++ b/common/board_info.c
@@ -15,35 +15,65 @@ int __weak checkboard(void)
return 0;
}
+static const struct to_show {
+ const char *name;
+ enum sysinfo_id id;
+} to_show[] = {
+ { "Manufacturer", SYSINFO_ID_BOARD_MANUFACTURER},
+ { "Prior-stage version", SYSINFO_ID_PRIOR_STAGE_VERSION },
+ { "Prior-stage date", SYSINFO_ID_PRIOR_STAGE_DATE },
+ { /* sentinel */ }
+};
+
+static int try_sysinfo(void)
+{
+ struct udevice *dev;
+ char str[80];
+ int ret;
+
+ /* This might provide more detail */
+ ret = sysinfo_get();
+ if (ret)
+ return ret;
+
+ ret = sysinfo_detect(dev);
+ if (ret)
+ return ret;
+
+ ret = sysinfo_get_str(dev, SYSINFO_ID_BOARD_MODEL, sizeof(str), str);
+ if (ret)
+ return ret;
+ printf("Model: %s\n", str);
+
+ if (IS_ENABLED(CONFIG_SYSINFO_EXTRA)) {
+ const struct to_show *item;
+
+ for (item = to_show; item->id; item++) {
+ ret = sysinfo_get_str(dev, item->id, sizeof(str), str);
+ if (!ret)
+ printf("%s: %s\n", item->name, str);
+ }
+ }
+
+ return 0;
+}
+
int show_board_info(void)
{
if (IS_ENABLED(CONFIG_OF_CONTROL)) {
- struct udevice *dev;
- const char *model;
- char str[80];
int ret = -ENOSYS;
- if (IS_ENABLED(CONFIG_SYSINFO)) {
- /* This might provide more detail */
- ret = sysinfo_get();
- if (!ret) {
- ret = sysinfo_detect(dev);
- if (!ret) {
- ret = sysinfo_get_str(dev,
- SYSINFO_ID_BOARD_MODEL,
- sizeof(str), str);
- }
- }
- }
+ if (IS_ENABLED(CONFIG_SYSINFO))
+ ret = try_sysinfo();
/* Fail back to the main 'model' if available */
- if (ret)
- model = fdt_getprop(gd->fdt_blob, 0, "model", NULL);
- else
- model = str;
+ if (ret) {
+ const char *model;
- if (model)
- printf("Model: %s\n", model);
+ model = fdt_getprop(gd->fdt_blob, 0, "model", NULL);
+ if (model)
+ printf("Model: %s\n", model);
+ }
}
return checkboard();
diff --git a/drivers/sysinfo/Kconfig b/drivers/sysinfo/Kconfig
index e35f7cb17914..2030e4babc9f 100644
--- a/drivers/sysinfo/Kconfig
+++ b/drivers/sysinfo/Kconfig
@@ -8,6 +8,13 @@ menuconfig SYSINFO
if SYSINFO
+config SYSINFO_EXTRA
+ bool "Show extra information on startup"
+ help
+ Enable this to see extra information on startup. Normally only the
+ model is shown, but with this option the vendor and any prior-stage
+ firmware's version and date are shown as well.
+
config SPL_SYSINFO
depends on SPL_DM
bool "Enable board driver support in SPL"
diff --git a/include/sysinfo.h b/include/sysinfo.h
index b140d742e93d..f2c1aa29d18e 100644
--- a/include/sysinfo.h
+++ b/include/sysinfo.h
@@ -46,6 +46,9 @@ enum sysinfo_id {
/* For show_board_info() */
SYSINFO_ID_BOARD_MODEL,
+ SYSINFO_ID_BOARD_MANUFACTURER,
+ SYSINFO_ID_PRIOR_STAGE_VERSION,
+ SYSINFO_ID_PRIOR_STAGE_DATE,
/* First value available for downstream/board used */
SYSINFO_ID_USER = 0x1000,
--
2.42.0.869.gea05f2083d-goog
[PATCH 9/9] x86: coreboot: Add a sysinfo driver
Create a sysinfo driver to avoid needing a custom checkboard()
function. With this the following information is printed when booting
from coreboot under QEMU:
Model: Standard PC (i440FX + PIIX, 1996)
Manufacturer: QEMU
Prior-stage version: 4.21-885-g2a87ef1eca56
Prior-stage date: 11/11/2023
Signed-off-by: Simon Glass
---
arch/x86/cpu/coreboot/Kconfig | 2 +
arch/x86/dts/coreboot.dts | 4 ++
board/coreboot/coreboot/Makefile | 1 +
board/coreboot/coreboot/coreboot.c | 44 ---
board/coreboot/coreboot/sysinfo.c | 89 ++
5 files changed, 96 insertions(+), 44 deletions(-)
create mode 100644 board/coreboot/coreboot/sysinfo.c
diff --git a/arch/x86/cpu/coreboot/Kconfig b/arch/x86/cpu/coreboot/Kconfig
index 178f8ad18162..085302c04829 100644
--- a/arch/x86/cpu/coreboot/Kconfig
+++ b/arch/x86/cpu/coreboot/Kconfig
@@ -27,5 +27,7 @@ config SYS_COREBOOT
imply X86_TSC_READ_BASE
imply USE_PREBOOT
select BINMAN if X86_64
+ select SYSINFO
+ imply SYSINFO_EXTRA
endif
diff --git a/arch/x86/dts/coreboot.dts b/arch/x86/dts/coreboot.dts
index 0eb31cae42c1..dfce7c2d5919 100644
--- a/arch/x86/dts/coreboot.dts
+++ b/arch/x86/dts/coreboot.dts
@@ -45,4 +45,8 @@
bootph-some-ram;
compatible = "coreboot-fb";
};
+
+ sysinfo {
+ compatible = "coreboot,sysinfo";
+ };
};
diff --git a/board/coreboot/coreboot/Makefile b/board/coreboot/coreboot/Makefile
index d292b7032c23..75bfbd189437 100644
--- a/board/coreboot/coreboot/Makefile
+++ b/board/coreboot/coreboot/Makefile
@@ -11,3 +11,4 @@
# Daniel Engström, Omicron Ceti AB, dan...@omicron.se.
obj-y += coreboot.o
+obj-$(CONFIG_$(SPL_TPL_)SMBIOS_PARSER) += sysinfo.o
diff --git a/board/coreboot/coreboot/coreboot.c
b/board/coreboot/coreboot/coreboot.c
index db855c11ae65..e58dce37477f 100644
--- a/board/coreboot/coreboot/coreboot.c
+++ b/board/coreboot/coreboot/coreboot.c
@@ -23,50 +23,6 @@ int board_early_init_r(void)
return 0;
}
-#ifdef CONFIG_SMBIOS_PARSER
-int show_board_info(void)
-{
- const struct smbios_entry *smbios =
smbios_entry(lib_sysinfo.smbios_start, lib_sysinfo.smbios_size);
-
- if (!smbios)
- goto fallback;
-
- const struct smbios_header *bios = smbios_header(smbios,
SMBIOS_BIOS_INFORMATION);
- const struct smbios_header *system = smbios_header(smbios,
SMBIOS_SYSTEM_INFORMATION);
- const struct smbios_type0 *t0 = (struct smbios_type0 *)bios;
- const struct smbios_type1 *t1 = (struct smbios_type1 *)system;
-
- if (!t0 || !t1)
- goto fallback;
-
- const char *bios_ver = smbios_string(bios, t0->bios_ver);
- const char *bios_date = smbios_string(bios, t0->bios_release_date);
- const char *model = smbios_string(system, t1->product_name);
- const char *manufacturer = smbios_string(system, t1->manufacturer);
-
- if (!model || !manufacturer || !bios_ver)
- goto fallback;
-
- printf("Vendor: %s\n", manufacturer);
- printf("Model: %s\n", model);
- printf("BIOS Version: %s\n", bios_ver);
- if (bios_date)
- printf("BIOS date: %s\n", bios_date);
-
- return 0;
-
-fallback:
- if (IS_ENABLED(CONFIG_OF_CONTROL)) {
- model = fdt_getprop(gd->fdt_blob, 0, "model", NULL);
-
- if (model)
- printf("Model: %s\n", model);
- }
-
- return checkboard();
-}
-#endif
-
static struct splash_location coreboot_splash_locations[] = {
{
.name = "virtio_fs",
diff --git a/board/coreboot/coreboot/sysinfo.c
b/board/coreboot/coreboot/sysinfo.c
new file mode 100644
index ..e0bdc7a5a88e
--- /dev/null
+++ b/board/coreboot/coreboot/sysinfo.c
@@ -0,0 +1,89 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * coreboot sysinfo driver
+ *
+ * Copyright 2023 Google LLC
+ * Written by Simon Glass
+ */
+
+#include
+#include
+#include
+#include
+
+struct cb_sysinfo_priv {
+ const struct smbios_header *bios;
+ const struct smbios_header *system;
+ const struct smbios_type0 *t0;
+ const struct smbios_type1 *t1;
+};
+
+static int cb_get_str(struct udevice *dev, int id, size_t size, char *val)
+{
+ struct cb_sysinfo_priv *priv = dev_get_priv(dev);
+ const char *str = NULL;
+
+ switch (id) {
+ case SYSINFO_ID_BOARD_MODEL:
+ if (priv->t1)
+ str = smbios_string(priv->system,
+ priv->t1->product_name);
+ break;
+ case SYSINFO_ID_BOARD_MANUFACTURER:
+ if (priv->t1)
+ str = smbios_string(priv->system,
+ priv->t1->manufacture
[PATCH 4/9] solidrun: Use checkboard() instead of show_board_info()
Boards can use a sysinfo driver if a particular model name is needed.
Update this board to use checkboard() directly, rather than having a
weak function laid on top of a weak function.
Signed-off-by: Simon Glass
---
board/solidrun/mx6cuboxi/mx6cuboxi.c | 7 +--
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/board/solidrun/mx6cuboxi/mx6cuboxi.c
b/board/solidrun/mx6cuboxi/mx6cuboxi.c
index e119330bc0c1..8edabf4404c2 100644
--- a/board/solidrun/mx6cuboxi/mx6cuboxi.c
+++ b/board/solidrun/mx6cuboxi/mx6cuboxi.c
@@ -381,6 +381,7 @@ static bool has_emmc(void)
return (mmc_get_op_cond(mmc, true) < 0) ? 0 : 1;
}
+/* Override the default implementation, DT model is not accurate */
int checkboard(void)
{
request_detect_gpios();
@@ -496,12 +497,6 @@ int ft_board_setup(void *fdt, struct bd_info *bd)
}
#endif
-/* Override the default implementation, DT model is not accurate */
-int show_board_info(void)
-{
- return checkboard();
-}
-
int board_late_init(void)
{
#ifdef CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG
--
2.42.0.869.gea05f2083d-goog
[PATCH 5/9] toradex: Use checkboard() instead of show_board_info()
Boards can use a sysinfo driver if a particular model name is needed.
Update this board to use checkboard() directly, rather than having a
weak function laid on top of a weak function.
Make all the checkboard() functions call the new tdx_checkboard() so
that the same information is displayed.
Signed-off-by: Simon Glass
---
board/toradex/apalis-imx8/apalis-imx8.c | 2 +-
board/toradex/apalis-tk1/apalis-tk1.c | 2 +-
board/toradex/apalis_imx6/apalis_imx6.c | 3 ++-
board/toradex/apalis_t30/apalis_t30.c | 2 +-
board/toradex/colibri-imx6ull/colibri-imx6ull.c | 2 +-
board/toradex/colibri-imx8x/colibri-imx8x.c | 2 +-
board/toradex/colibri_imx6/colibri_imx6.c | 3 ++-
board/toradex/colibri_imx7/colibri_imx7.c | 2 +-
board/toradex/colibri_t20/colibri_t20.c | 2 +-
board/toradex/colibri_t30/colibri_t30.c | 2 +-
board/toradex/colibri_vf/colibri_vf.c | 2 +-
board/toradex/common/tdx-common.c | 2 +-
board/toradex/common/tdx-common.h | 1 +
13 files changed, 15 insertions(+), 12 deletions(-)
diff --git a/board/toradex/apalis-imx8/apalis-imx8.c
b/board/toradex/apalis-imx8/apalis-imx8.c
index e2bbaba8b8c1..b351ce64abfc 100644
--- a/board/toradex/apalis-imx8/apalis-imx8.c
+++ b/board/toradex/apalis-imx8/apalis-imx8.c
@@ -215,7 +215,7 @@ int checkboard(void)
build_info();
print_bootinfo();
- return 0;
+ return tdx_checkboard();
}
static enum pcb_rev_t get_pcb_revision(void)
diff --git a/board/toradex/apalis-tk1/apalis-tk1.c
b/board/toradex/apalis-tk1/apalis-tk1.c
index 851343159186..79a1c92da0a0 100644
--- a/board/toradex/apalis-tk1/apalis-tk1.c
+++ b/board/toradex/apalis-tk1/apalis-tk1.c
@@ -95,7 +95,7 @@ int checkboard(void)
{
puts("Model: Toradex Apalis TK1 2GB\n");
- return 0;
+ return tdx_checkboard();
}
#if defined(CONFIG_OF_LIBFDT) && defined(CONFIG_OF_BOARD_SETUP)
diff --git a/board/toradex/apalis_imx6/apalis_imx6.c
b/board/toradex/apalis_imx6/apalis_imx6.c
index fa6b7226fedf..dc0e09991dde 100644
--- a/board/toradex/apalis_imx6/apalis_imx6.c
+++ b/board/toradex/apalis_imx6/apalis_imx6.c
@@ -730,7 +730,8 @@ int checkboard(void)
is_cpu_type(MXC_CPU_MX6D) ? "Dual" : "Quad",
(gd->ram_size == 0x8000) ? "2GB" :
(gd->ram_size == 0x4000) ? "1GB" : "512MB", it);
- return 0;
+
+ return tdx_checkboard();
}
#if defined(CONFIG_OF_LIBFDT) && defined(CONFIG_OF_BOARD_SETUP)
diff --git a/board/toradex/apalis_t30/apalis_t30.c
b/board/toradex/apalis_t30/apalis_t30.c
index ef71270d9f2d..b9a2af33f19f 100644
--- a/board/toradex/apalis_t30/apalis_t30.c
+++ b/board/toradex/apalis_t30/apalis_t30.c
@@ -50,7 +50,7 @@ int checkboard(void)
printf("Model: Toradex Apalis T30 %dGB\n",
(gd->ram_size == 0x4000) ? 1 : 2);
- return 0;
+ return tdx_checkboard();
}
#if defined(CONFIG_OF_LIBFDT) && defined(CONFIG_OF_BOARD_SETUP)
diff --git a/board/toradex/colibri-imx6ull/colibri-imx6ull.c
b/board/toradex/colibri-imx6ull/colibri-imx6ull.c
index 48fdb1e09712..6c8eeff38fa5 100644
--- a/board/toradex/colibri-imx6ull/colibri-imx6ull.c
+++ b/board/toradex/colibri-imx6ull/colibri-imx6ull.c
@@ -206,7 +206,7 @@ int checkboard(void)
{
printf("Model: Toradex Colibri iMX6ULL\n");
- return 0;
+ return tdx_checkboard();
}
#if defined(CONFIG_OF_LIBFDT) && defined(CONFIG_OF_BOARD_SETUP)
diff --git a/board/toradex/colibri-imx8x/colibri-imx8x.c
b/board/toradex/colibri-imx8x/colibri-imx8x.c
index 6c0b09787c8b..d8cc72f323c5 100644
--- a/board/toradex/colibri-imx8x/colibri-imx8x.c
+++ b/board/toradex/colibri-imx8x/colibri-imx8x.c
@@ -121,7 +121,7 @@ int checkboard(void)
build_info();
print_bootinfo();
- return 0;
+ return tdx_checkboard();
}
static void select_dt_from_module_version(void)
diff --git a/board/toradex/colibri_imx6/colibri_imx6.c
b/board/toradex/colibri_imx6/colibri_imx6.c
index e6c9b10570d1..7635c5811d26 100644
--- a/board/toradex/colibri_imx6/colibri_imx6.c
+++ b/board/toradex/colibri_imx6/colibri_imx6.c
@@ -649,7 +649,8 @@ int checkboard(void)
printf("Model: Toradex Colibri iMX6 %s %sMB%s\n",
is_cpu_type(MXC_CPU_MX6DL) ? "DualLite" : "Solo",
(gd->ram_size == 0x2000) ? "512" : "256", it);
- return 0;
+
+ return tdx_checkboard();
}
#if defined(CONFIG_OF_LIBFDT) && defined(CONFIG_OF_BOARD_SETUP)
diff --git a/board/toradex/colibri_imx7/colibri_imx7.c
b/board/toradex/colibri_imx7/colibri_imx7.c
index f0356af0082b..c3478b15111b 100644
--- a/board/toradex/colibri_imx7/colibri_imx7.c
+++ b/board/toradex/colibri_imx7/colibri_imx7.c
@@ -279,7 +279,7 @@ int checkboard
[PATCH 7/9] Revert "generic-board: make show_board_info a weak function"
We already have:
- a sysinfo driver-interface which can obtain the model
- a weak function called checkboard() for showing board info
The current implementation has a weak function on top of a weak
function. Now that all boards have been updated to use checkboard()
instead, drop the __weak on show_board_info()
This reverts commit f7637cc01414b9c87b6b0f861f34d83c19bfaaaf.
Signed-off-by: Simon Glass
---
common/board_info.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/board_info.c b/common/board_info.c
index 3185793da4a7..a62c04794b90 100644
--- a/common/board_info.c
+++ b/common/board_info.c
@@ -15,7 +15,7 @@ int __weak checkboard(void)
return 0;
}
-int __weak show_board_info(void)
+int show_board_info(void)
{
if (IS_ENABLED(CONFIG_OF_CONTROL)) {
struct udevice *dev;
--
2.42.0.869.gea05f2083d-goog
[PATCH 6/9] udoo: Use checkboard() instead of show_board_info()
Boards can use a sysinfo driver if a particular model name is needed.
Update this board to use checkboard() directly, rather than having a
weak function laid on top of a weak function.
Signed-off-by: Simon Glass
---
board/udoo/neo/neo.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/board/udoo/neo/neo.c b/board/udoo/neo/neo.c
index 730e266469b2..d99d93b44ae5 100644
--- a/board/udoo/neo/neo.c
+++ b/board/udoo/neo/neo.c
@@ -212,7 +212,7 @@ static char *board_string(int type)
}
/* Override the default implementation, DT model is not accurate */
-int show_board_info(void)
+int checkboard(void)
{
int *board_type = (int *)OCRAM_START;
--
2.42.0.869.gea05f2083d-goog
[PATCH 3/9] turris: Use checkboard() instead of show_board_info()
Boards can use a sysinfo driver if a particular model name is needed.
Update this board to use checkboard() directly, rather than having a
weak function laid on top of a weak function.
Signed-off-by: Simon Glass
---
board/CZ.NIC/turris_mox/turris_mox.c | 2 +-
board/CZ.NIC/turris_omnia/turris_omnia.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/board/CZ.NIC/turris_mox/turris_mox.c
b/board/CZ.NIC/turris_mox/turris_mox.c
index 63b869921943..3489bdd74bda 100644
--- a/board/CZ.NIC/turris_mox/turris_mox.c
+++ b/board/CZ.NIC/turris_mox/turris_mox.c
@@ -562,7 +562,7 @@ static void handle_reset_button(void)
}
}
-int show_board_info(void)
+int checkboard(void)
{
int i, ret, board_version, ram_size, is_sd;
const char *pub_key, *model;
diff --git a/board/CZ.NIC/turris_omnia/turris_omnia.c
b/board/CZ.NIC/turris_omnia/turris_omnia.c
index 19c5043fcbaa..adeb69a205be 100644
--- a/board/CZ.NIC/turris_omnia/turris_omnia.c
+++ b/board/CZ.NIC/turris_omnia/turris_omnia.c
@@ -962,7 +962,7 @@ int board_late_init(void)
return 0;
}
-int show_board_info(void)
+int checkboard(void)
{
char serial[17];
int err;
--
2.42.0.869.gea05f2083d-goog
[PATCH 2/9] meson: Use checkboard() instead of show_board_info()
Boards can use a sysinfo driver if a particular model name is needed.
Update this board to use checkboard() directly, rather than having a
weak function laid on top of a weak function.
Signed-off-by: Simon Glass
---
arch/arm/mach-meson/board-info.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/mach-meson/board-info.c b/arch/arm/mach-meson/board-info.c
index 2421acd817e4..95a29da07229 100644
--- a/arch/arm/mach-meson/board-info.c
+++ b/arch/arm/mach-meson/board-info.c
@@ -168,7 +168,7 @@ static unsigned int get_socinfo(void)
return socinfo;
}
-int show_board_info(void)
+int checkboard(void)
{
unsigned int socinfo;
--
2.42.0.869.gea05f2083d-goog
[PATCH 0/9] sysinfo: Expand sysinfo with some more banner information
The show_board_info() function was adjusted to weak so that it could be entirely replaced with a board-specific implementation. The intended way for boards to provide their own information is via a sysinfo driver. But currently there is no way to show anything other than the model name. This series adds support for showing a few more items, in a way that is easy for boards to extend. Since there is already a weak checkboard() function, we don't need to have two levels of weak function here. So this series drops the weak attribute from show_board_info() Existing boards will see a slight change in output, in that the model name will appear first, before any custom output. If that is a problem, then the solution is to implement a sysinfo driver for the board. Simon Glass (9): board: Move show_board_info() comment to header file meson: Use checkboard() instead of show_board_info() turris: Use checkboard() instead of show_board_info() solidrun: Use checkboard() instead of show_board_info() toradex: Use checkboard() instead of show_board_info() udoo: Use checkboard() instead of show_board_info() Revert "generic-board: make show_board_info a weak function" sysinfo: Allow displaying more info on startup x86: coreboot: Add a sysinfo driver arch/arm/mach-meson/board-info.c | 2 +- arch/x86/cpu/coreboot/Kconfig | 2 + arch/x86/dts/coreboot.dts | 4 + board/CZ.NIC/turris_mox/turris_mox.c | 2 +- board/CZ.NIC/turris_omnia/turris_omnia.c | 2 +- board/coreboot/coreboot/Makefile | 1 + board/coreboot/coreboot/coreboot.c| 44 - board/coreboot/coreboot/sysinfo.c | 89 +++ board/solidrun/mx6cuboxi/mx6cuboxi.c | 7 +- board/toradex/apalis-imx8/apalis-imx8.c | 2 +- board/toradex/apalis-tk1/apalis-tk1.c | 2 +- board/toradex/apalis_imx6/apalis_imx6.c | 3 +- board/toradex/apalis_t30/apalis_t30.c | 2 +- .../toradex/colibri-imx6ull/colibri-imx6ull.c | 2 +- board/toradex/colibri-imx8x/colibri-imx8x.c | 2 +- board/toradex/colibri_imx6/colibri_imx6.c | 3 +- board/toradex/colibri_imx7/colibri_imx7.c | 2 +- board/toradex/colibri_t20/colibri_t20.c | 2 +- board/toradex/colibri_t30/colibri_t30.c | 2 +- board/toradex/colibri_vf/colibri_vf.c | 2 +- board/toradex/common/tdx-common.c | 2 +- board/toradex/common/tdx-common.h | 1 + board/udoo/neo/neo.c | 2 +- common/board_info.c | 80 +++-- drivers/sysinfo/Kconfig | 7 ++ include/init.h| 11 +++ include/sysinfo.h | 3 + 27 files changed, 189 insertions(+), 94 deletions(-) create mode 100644 board/coreboot/coreboot/sysinfo.c -- 2.42.0.869.gea05f2083d-goog
[PATCH 1/9] board: Move show_board_info() comment to header file
Move this comment to its prototype and tidy it up a bit.
Signed-off-by: Simon Glass
---
common/board_info.c | 6 --
include/init.h | 11 +++
2 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/common/board_info.c b/common/board_info.c
index e0f2d9392204..3185793da4a7 100644
--- a/common/board_info.c
+++ b/common/board_info.c
@@ -15,12 +15,6 @@ int __weak checkboard(void)
return 0;
}
-/*
- * Check sysinfo for board information. Failing that if the root node of the
DTB
- * has a "model" property, show it.
- *
- * Then call checkboard().
- */
int __weak show_board_info(void)
{
if (IS_ENABLED(CONFIG_OF_CONTROL)) {
diff --git a/include/init.h b/include/init.h
index d57a24fd00dd..9a1951d10a01 100644
--- a/include/init.h
+++ b/include/init.h
@@ -292,6 +292,17 @@ int misc_init_r(void);
/* common/board_info.c */
int checkboard(void);
+
+/**
+ * show_board_info() - Show board information
+ *
+ * Check sysinfo for board information. Failing that if the root node of the
DTB
+ * has a "model" property, show it.
+ *
+ * Then call checkboard().
+ *
+ * Return 0 if OK, -ve on error
+ */
int show_board_info(void);
/**
--
2.42.0.869.gea05f2083d-goog
Re: [PATCH 1/1] acpi: cannot have RSDT above 4 GiB
Hi Heinrich,
On Sun, 12 Nov 2023 at 13:16, Heinrich Schuchardt
wrote:
>
>
>
> Simon Glass schrieb am So., 12. Nov. 2023, 21:03:
>>
>> Hi Heinrich,
>>
>> On Sat, 11 Nov 2023 at 07:28, Heinrich Schuchardt
>> wrote:
>> >
>> > The field RsdtAddress has only 32 bit. The RSDT table cannot be located
>> > beyond 4 GiB.
>> >
>> > Signed-off-by: Heinrich Schuchardt
>> > ---
>> > lib/acpi/base.c | 26 +++---
>> > 1 file changed, 19 insertions(+), 7 deletions(-)
>>
>> Reviewed-by: Simon Glass
>>
>> nits / question below
>>
>> >
>> > diff --git a/lib/acpi/base.c b/lib/acpi/base.c
>> > index 2057bd2bef..128a76ad39 100644
>> > --- a/lib/acpi/base.c
>> > +++ b/lib/acpi/base.c
>> > @@ -21,10 +21,17 @@ void acpi_write_rsdp(struct acpi_rsdp *rsdp, struct
>> > acpi_rsdt *rsdt,
>> > memcpy(rsdp->signature, RSDP_SIG, 8);
>> > memcpy(rsdp->oem_id, OEM_ID, 6);
>> >
>> > - rsdp->length = sizeof(struct acpi_rsdp);
>> > - rsdp->rsdt_address = map_to_sysmem(rsdt);
>> > + if (rsdt)
>> > + rsdp->rsdt_address = map_to_sysmem(rsdt);
>> > + else
>> > + rsdp->rsdt_address = 0;
>>
>> There is a memset() at the top so this line (and the one below) should
>> not be needed.
>>
>> > +
>> > + if (xsdt)
>> > + rsdp->xsdt_address = map_to_sysmem(xsdt);
>> > + else
>> > + rsdp->xsdt_address = 0;
>> >
>> > - rsdp->xsdt_address = map_to_sysmem(xsdt);
>> > + rsdp->length = sizeof(struct acpi_rsdp);
>> > rsdp->revision = ACPI_RSDP_REV_ACPI_2_0;
>> >
>> > /* Calculate checksums */
>> > @@ -68,11 +75,15 @@ static void acpi_write_xsdt(struct acpi_xsdt *xsdt)
>> > static int acpi_write_base(struct acpi_ctx *ctx,
>> >const struct acpi_writer *entry)
>> > {
>> > - /* We need at least an RSDP and an RSDT Table */
>> > + /* We need at least an RSDP and an XSDT Table */
>> > ctx->rsdp = ctx->current;
>> > acpi_inc_align(ctx, sizeof(struct acpi_rsdp));
>> > - ctx->rsdt = ctx->current;
>> > - acpi_inc_align(ctx, sizeof(struct acpi_rsdt));
>> > + if (map_to_sysmem(ctx->current) < UINT_MAX - 0x1) {
>>
>> Won't that do something different on 64-bit machines? It seems that
>> you want to check against SZ_4G ?
>
>
> For gcc int is 32bit on 64bit systems, so is UINT_MAX. Using SZ_4G could make
> this code clearer.
Oh I didn't know that...I just assumed that uint would be 64-bit on a
64-machine machine, since it is the natural reg size! Yes, that is
very very confusing. I vaguely recall some compiler setting for that
and that Linux did it a particular way.
>
> Best regards
>
> Heinrich
>
>
>>
>> > + ctx->rsdt = ctx->current;
>> > + acpi_inc_align(ctx, sizeof(struct acpi_rsdt));
>> > + } else {
>> > + ctx->rsdt = 0;
>> > + }
>> > ctx->xsdt = ctx->current;
>> > acpi_inc_align(ctx, sizeof(struct acpi_xsdt));
>> >
>> > @@ -80,7 +91,8 @@ static int acpi_write_base(struct acpi_ctx *ctx,
>> > memset(ctx->base, '\0', ctx->current - ctx->base);
>> >
>> > acpi_write_rsdp(ctx->rsdp, ctx->rsdt, ctx->xsdt);
>> > - acpi_write_rsdt(ctx->rsdt);
>> > + if (ctx->rsdt)
>> > + acpi_write_rsdt(ctx->rsdt);
>> > acpi_write_xsdt(ctx->xsdt);
>> >
>> > return 0;
>> > --
>> > 2.40.1
>> >
>>
>> Regards,
>> Simon
Re: [PATCH v4 05/12] usb: Avoid unbinding devices in use by bootflows
Hi Heinrich,
On Sun, 12 Nov 2023 at 13:32, Heinrich Schuchardt wrote:
>
>
>
> Am 12. November 2023 21:02:42 MEZ schrieb Simon Glass :
> >When a USB device is unbound, it causes any bootflows attached to it to
> >be removed, via a call to bootdev_clear_bootflows() from
> >bootdev_pre_unbind(). This obviously makes it impossible to boot the
> >bootflow.
> >
> >However, when booting a bootflow that relies on USB, usb_stop() is
> >called, which unbinds the device. For EFI, this happens in
> >efi_exit_boot_services() which means that the bootflow disappears
> >before it is finished with.
>
> After ExitBootServices() no driver of U-Boot can be used as the operating
> system is in charge.
>
> Any bootflow inside U-Boot is terminated by definition when reaching
> ExitBootServices.
>
> >
> >There is no need to unbind all the USB devices just to quiesce them.
> >Add a new usb_pause() call which removes them but leaves them bound.
>
> As U-Boot must not access any device after ExitBootServices() it is unclear
> to me what you want to achieve.
I can't remember exactly what is needed from the bootflow, but
something is. Perhaps the kernel, or the cmdline, or fdt? It would
have been better if I had mentioned this explicitly, But then this
patch has been sitting around for ages...
In any case, the intent of exit-boot-services is not to free all the
memory used, since some of it may have been used to hold data that the
app continues to use.
Also there is U-Boot code between when the devices are unbound and
when U-Boot actually exits back to the app.
This hang was 100% repeatable on brya (an x86 Chromebook) and it took
quite a while to find.
Regards,
Simon
>
> Best regards
>
> Heinrich
>
> >
> >This resolves a hang on x86 when booting a distro from USB. This was
> >found using a device with 4 bootflows, the last of which was USB.
> >
> >
> >Signed-off-by: Simon Glass
> >---
> >
> >Changes in v4:
> >- Don't rename the legacy-USB functions
> >- Add a bit more detail to the comment
> >
> >Changes in v2:
> >- Add new patch to avoid unbinding devices in use by bootflows
> >
> > boot/bootm.c | 2 +-
> > common/usb.c | 5 +
> > drivers/usb/host/usb-uclass.c | 14 --
> > include/usb.h | 21 -
> > 4 files changed, 38 insertions(+), 4 deletions(-)
> >
> >diff --git a/boot/bootm.c b/boot/bootm.c
> >index cb61485c226c..d9c3fa8dad99 100644
> >--- a/boot/bootm.c
> >+++ b/boot/bootm.c
> >@@ -502,7 +502,7 @@ ulong bootm_disable_interrupts(void)
> >* updated every 1 ms within the HCCA structure in SDRAM! For more
> >* details see the OpenHCI specification.
> >*/
> >- usb_stop();
> >+ usb_pause();
> > #endif
> > return iflag;
> > }
> >diff --git a/common/usb.c b/common/usb.c
> >index 836506dcd9e9..a486d1c87d4d 100644
> >--- a/common/usb.c
> >+++ b/common/usb.c
> >@@ -144,6 +144,11 @@ int usb_stop(void)
> > return 0;
> > }
> >
> >+int usb_pause(void)
> >+{
> >+ return usb_stop();
> >+}
> >+
> > /**
> > * Detect if a USB device has been plugged or unplugged.
> > */
> >diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c
> >index a1cd0ad2d669..c26c65d7986b 100644
> >--- a/drivers/usb/host/usb-uclass.c
> >+++ b/drivers/usb/host/usb-uclass.c
> >@@ -173,7 +173,7 @@ int usb_get_max_xfer_size(struct usb_device *udev,
> >size_t *size)
> > return ops->get_max_xfer_size(bus, size);
> > }
> >
> >-int usb_stop(void)
> >+static int usb_finish(bool unbind_all)
> > {
> > struct udevice *bus;
> > struct udevice *rh;
> >@@ -195,7 +195,7 @@ int usb_stop(void)
> >
> > /* Locate root hub device */
> > device_find_first_child(bus, );
> >- if (rh) {
> >+ if (rh && unbind_all) {
> > /*
> >* All USB devices are children of root hub.
> >* Unbinding root hub will unbind all of its children.
> >@@ -222,6 +222,16 @@ int usb_stop(void)
> > return err;
> > }
> >
> >+int usb_stop(void)
> >+{
> >+ return usb_finish(true);
> >+}
> >+
> >+int usb_pause(void)
> >+{
> >+ return usb_finish(false);
> >+}
> >+
> > static v
[PATCH v2 3/3] efi: Avoid using dm_scan_other()
This function is defined by bootstd so using it precludes using that
feature. Use the board_early_init_r() feature instead.
This requires moving quite a lot of code into the board directory, butt
this is the normal place for code called by board_early_init_r()
Signed-off-by: Simon Glass
---
Changes in v2:
- Drop duplicate acpi_xsdt patch
- Put the board_early_init_r code into board/
board/efi/efi-x86_app/Makefile | 5 +
board/efi/efi-x86_app/efi_app.c | 205
configs/efi-x86_app64_defconfig | 1 +
lib/efi/efi_app.c | 187 -
4 files changed, 211 insertions(+), 187 deletions(-)
create mode 100644 board/efi/efi-x86_app/Makefile
create mode 100644 board/efi/efi-x86_app/efi_app.c
diff --git a/board/efi/efi-x86_app/Makefile b/board/efi/efi-x86_app/Makefile
new file mode 100644
index ..682f8754b44d
--- /dev/null
+++ b/board/efi/efi-x86_app/Makefile
@@ -0,0 +1,5 @@
+# SPDX-License-Identifier: GPL-2.0+
+#
+# Copyright 2023 Google LLC
+
+obj-y += efi_app.o
diff --git a/board/efi/efi-x86_app/efi_app.c b/board/efi/efi-x86_app/efi_app.c
new file mode 100644
index ..c5e4192fe06c
--- /dev/null
+++ b/board/efi/efi-x86_app/efi_app.c
@@ -0,0 +1,205 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * EFI-app board implementation
+ *
+ * Copyright 2023 Google LLC
+ * Written by Simon Glass
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+DECLARE_GLOBAL_DATA_PTR;
+
+/**
+ * efi_bind_block() - bind a new block device to an EFI device
+ *
+ * Binds a new top-level EFI_MEDIA device as well as a child block device so
+ * that the block device can be accessed in U-Boot.
+ *
+ * The device can then be accessed using 'part list efi 0', 'fat ls efi 0:1',
+ * for example, just like any other interface type.
+ *
+ * @handle: handle of the controller on which this driver is installed
+ * @blkio: block io protocol proxied by this driver
+ * @device_path: EFI device path structure for this
+ * @len: Length of @device_path in bytes
+ * @devp: Returns the bound device
+ * Return: 0 if OK, -ve on error
+ */
+int efi_bind_block(efi_handle_t handle, struct efi_block_io *blkio,
+ struct efi_device_path *device_path, int len,
+ struct udevice **devp)
+{
+ struct efi_media_plat plat;
+ struct udevice *dev;
+ char name[18];
+ int ret;
+
+ plat.handle = handle;
+ plat.blkio = blkio;
+ plat.device_path = malloc(device_path->length);
+ if (!plat.device_path)
+ return log_msg_ret("path", -ENOMEM);
+ memcpy(plat.device_path, device_path, device_path->length);
+ ret = device_bind(dm_root(), DM_DRIVER_GET(efi_media), "efi_media",
+ , ofnode_null(), );
+ if (ret)
+ return log_msg_ret("bind", ret);
+
+ snprintf(name, sizeof(name), "efi_media_%x", dev_seq(dev));
+ device_set_name(dev, name);
+ *devp = dev;
+
+ return 0;
+}
+
+/**
+ * devpath_is_partition() - Figure out if a device path is a partition
+ *
+ * Checks if a device path refers to a partition on some media device. This
+ * works by checking for a valid partition number in a hard-driver media device
+ * as the final component of the device path.
+ *
+ * @path: device path
+ * Return: true if a partition, false if not
+ * (e.g. it might be media which contains partitions)
+ */
+static bool devpath_is_partition(const struct efi_device_path *path)
+{
+ const struct efi_device_path *p;
+ bool was_part = false;
+
+ for (p = path; p->type != DEVICE_PATH_TYPE_END;
+p = (void *)p + p->length) {
+ was_part = false;
+ if (p->type == DEVICE_PATH_TYPE_MEDIA_DEVICE &&
+ p->sub_type == DEVICE_PATH_SUB_TYPE_HARD_DRIVE_PATH) {
+ struct efi_device_path_hard_drive_path *hd =
+ (void *)path;
+
+ if (hd->partition_number)
+ was_part = true;
+ }
+ }
+
+ return was_part;
+}
+
+/**
+ * setup_block() - Find all block devices and setup EFI devices for them
+ *
+ * Partitions are ignored, since U-Boot has partition handling. Errors with
+ * particular devices produce a warning but execution continues to try to
+ * find others.
+ *
+ * Return: 0 if found, -ENOSYS if there is no boot-services table, -ENOTSUPP
+ * if a required protocol is not supported
+ */
+static int setup_block(void)
+{
+ efi_guid_t efi_blkio_guid = EFI_BLOCK_IO_PROTOCOL_GUID;
+ efi_guid_t efi_devpath_guid = EFI_DEVICE_PATH_PROTOCOL_GUID;
+ efi_guid_t efi_pathutil_guid = EFI_DEVICE_PATH_UTILITIES_PROTOCOL_GUID;
+ efi_guid_t efi_pathtext_guid = EFI_DEVICE_PATH_TO_TEXT_PROTOCOL_GUID;
+
[PATCH v2 2/3] efi: Correct display of table GUIDs
The printf() %pU option decodes GUIDs so it is not necessary to do this
first. Drop the incorrect code.
Signed-off-by: Simon Glass
---
(no changes since v1)
cmd/efi_common.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/cmd/efi_common.c b/cmd/efi_common.c
index f4056096cd3f..1aa2351fcdfd 100644
--- a/cmd/efi_common.c
+++ b/cmd/efi_common.c
@@ -17,10 +17,8 @@ void efi_show_tables(struct efi_system_table *systab)
for (i = 0; i < systab->nr_tables; i++) {
struct efi_configuration_table *tab = >tables[i];
- char guid_str[37];
- uuid_bin_to_str(tab->guid.b, guid_str, 1);
- printf("%p %pUl %s\n", tab->table, guid_str,
+ printf("%p %pUl %s\n", tab->table, tab->guid.b,
uuid_guid_get_str(tab->guid.b) ?: "(unknown)");
}
}
--
2.42.0.869.gea05f2083d-goog
[PATCH v2 1/3] efi: Collect the ACPI tables in the app
Locate these so that they can be displayed using the 'acpi' command.
Signed-off-by: Simon Glass
---
(no changes since v1)
lib/efi/efi_app.c | 24 +---
1 file changed, 21 insertions(+), 3 deletions(-)
diff --git a/lib/efi/efi_app.c b/lib/efi/efi_app.c
index 2209410f35b5..c5eb816655ea 100644
--- a/lib/efi/efi_app.c
+++ b/lib/efi/efi_app.c
@@ -12,18 +12,21 @@
#include
#include
#include
+#include
+#include
#include
#include
#include
+#include
+#include
#include
#include
#include
-#include
-#include
-#include
+#include
#include
#include
#include
+#include
DECLARE_GLOBAL_DATA_PTR;
@@ -320,6 +323,19 @@ int dm_scan_other(bool pre_reloc_only)
return 0;
}
+static void scan_tables(struct efi_system_table *sys_table)
+{
+ efi_guid_t acpi = EFI_ACPI_TABLE_GUID;
+ uint i;
+
+ for (i = 0; i < sys_table->nr_tables; i++) {
+ struct efi_configuration_table *tab = _table->tables[i];
+
+ if (!memcmp(>guid, , sizeof(efi_guid_t)))
+ gd_set_acpi_start(map_to_sysmem(tab->table));
+ }
+}
+
/**
* efi_main() - Start an EFI image
*
@@ -354,6 +370,8 @@ efi_status_t EFIAPI efi_main(efi_handle_t image,
return ret;
}
+ scan_tables(priv->sys_table);
+
/*
* We could store the EFI memory map here, but it changes all the time,
* so this is only useful for debugging.
--
2.42.0.869.gea05f2083d-goog
[PATCH v2 0/3] efi: Minor improvements for the EFI app
This series collects a few fixes and improvements useful when booting U-Boot as an EFI app. Changes in v2: - Drop duplicate acpi_xsdt patch - Put the board_early_init_r code into board/ Simon Glass (3): efi: Collect the ACPI tables in the app efi: Correct display of table GUIDs efi: Avoid using dm_scan_other() board/efi/efi-x86_app/Makefile | 5 + board/efi/efi-x86_app/efi_app.c | 205 cmd/efi_common.c| 4 +- configs/efi-x86_app64_defconfig | 1 + lib/efi/efi_app.c | 199 +++ 5 files changed, 227 insertions(+), 187 deletions(-) create mode 100644 board/efi/efi-x86_app/Makefile create mode 100644 board/efi/efi-x86_app/efi_app.c -- 2.42.0.869.gea05f2083d-goog
Re: [PATCH v3 07/12] video: Correct setting of cursor position
Hi Anatolij,
On Sun, 1 Oct 2023 at 19:15, Simon Glass wrote:
>
> The ANSI codes are not correctly handled at present, in that the
> requested X position is added to the current one.
>
> Correct this and also call vidconsole_entry_start() to start a new text
> line.
>
> Signed-off-by: Simon Glass
> ---
>
> (no changes since v1)
>
> drivers/video/vidconsole-uclass.c | 15 ---
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/video/vidconsole-uclass.c
> b/drivers/video/vidconsole-uclass.c
> index 22d55df71f63..a312a1985242 100644
> --- a/drivers/video/vidconsole-uclass.c
> +++ b/drivers/video/vidconsole-uclass.c
> @@ -125,6 +125,7 @@ void vidconsole_set_cursor_pos(struct udevice *dev, int
> x, int y)
> priv->xcur_frac = VID_TO_POS(x);
> priv->xstart_frac = priv->xcur_frac;
> priv->ycur = y;
> + vidconsole_entry_start(dev);
> }
>
> /**
> @@ -134,8 +135,10 @@ void vidconsole_set_cursor_pos(struct udevice *dev, int
> x, int y)
> * @row: new row
> * @col: new column
> */
> -static void set_cursor_position(struct vidconsole_priv *priv, int row, int
> col)
> +static void set_cursor_position(struct udevice *dev, int row, int col)
> {
> + struct vidconsole_priv *priv = dev_get_uclass_priv(dev);
> +
> /*
> * Ensure we stay in the bounds of the screen.
> */
> @@ -144,9 +147,7 @@ static void set_cursor_position(struct vidconsole_priv
> *priv, int row, int col)
> if (col >= priv->cols)
> col = priv->cols - 1;
>
> - priv->ycur = row * priv->y_charsize;
> - priv->xcur_frac = priv->xstart_frac +
> - VID_TO_POS(col * priv->x_charsize);
> + vidconsole_position_cursor(dev, col, row);
> }
>
> /**
> @@ -193,7 +194,7 @@ static void vidconsole_escape_char(struct udevice *dev,
> char ch)
> int row = priv->row_saved;
> int col = priv->col_saved;
>
> - set_cursor_position(priv, row, col);
> + set_cursor_position(dev, row, col);
> priv->escape = 0;
> return;
> }
> @@ -255,7 +256,7 @@ static void vidconsole_escape_char(struct udevice *dev,
> char ch)
> if (row < 0)
> row = 0;
> /* Right and bottom overflows are handled in the callee. */
> - set_cursor_position(priv, row, col);
> + set_cursor_position(dev, row, col);
> break;
> }
> case 'H':
> @@ -279,7 +280,7 @@ static void vidconsole_escape_char(struct udevice *dev,
> char ch)
> if (col)
> --col;
>
> - set_cursor_position(priv, row, col);
> + set_cursor_position(dev, row, col);
>
> break;
> }
> --
> 2.42.0.582.g8ccd20d70d-goog
>
Any thoughts on this patch, please?
Regards,
Simon
Re: [PATCH v2 4/6] video: Skip framebuffer reservation if already reserved
Hi Devarsh,
On Fri, 10 Nov 2023 at 08:29, Devarsh Thakkar wrote:
>
> Skip framebufer reservation if it was already reserved
> from previous stage and whose information was passed
> using a bloblist.
>
> Signed-off-by: Devarsh Thakkar
> Reviewed-by: Simon Glass
> ---
> V2:
> - Add debug prints
> - Fix commenting style
> V3:
> - Fix commenting style
> ---
> drivers/video/video-uclass.c | 13 +
> 1 file changed, 13 insertions(+)
>
> diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c
> index f743ed74c8..f619b5ae56 100644
> --- a/drivers/video/video-uclass.c
> +++ b/drivers/video/video-uclass.c
> @@ -123,6 +123,19 @@ int video_reserve(ulong *addrp)
> struct udevice *dev;
> ulong size;
>
> + if (IS_ENABLED(CONFIG_SPL_VIDEO_HANDOFF) && spl_phase() > PHASE_SPL) {
> + /*
> +* Skip allocation if already received a bloblist which
> +* filled below fields
> +*/
> + if (gd->fb_base && gd->video_top && gd->video_bottom) {
Can you drop that line? It should be an error to not have found the
handoff info.
> + debug("Found pre-reserved video memory from %lx to
> %lx\n",
> + gd->video_bottom, gd->video_top);
> + debug("Skipping video frame buffer allocation\n");
> + return 0;
> + }
> + }
> +
> gd->video_top = *addrp;
> for (uclass_find_first_device(UCLASS_VIDEO, );
> dev;
> --
> 2.34.1
>
Regards,
Simon
Re: [PATCH] configs: rockpro64: Enable SPI command and full BOOTSTD
Hi Jonas, On Sun, 12 Nov 2023 at 07:53, Jonas Karlman wrote: > > Hi Simon, > > On 2023-11-12 15:22, Simon Glass wrote: > > Hi Jonas, > > > > On Sun, 12 Nov 2023 at 05:50, Jonas Karlman wrote: > >> > >> Hi Shantur, > >> > >> On 2023-11-12 13:34, Shantur Rathore wrote: > >>> Hi Jonas, > >>> > >>>> The CMD_SPI is not used to interact with the SPI flash. > >>>> > >>>> CMD_SF is already enabled and you can use "sf probe" and any other sf > >>>> related action to interact with the SPI flash on this board. > >>>> > >>> > >>> You are right, this is not needed, thanks for correcting me. > >>> I will update my patch. > >>> > >>>> What is the reasoning behind enabling full bootstd commands? Especially > >>>> why just this board need it enabled by default. > >>>> > >>>> Standard boot is already fully functional, it just does not have full > >>>> features commands enabled by default. > >>> > >>> By default standard boot only allows you to boot from the first > >>> available boot device. > >>> This board supports SDCard, NVME (via PCIe port), USB and network boot > >>> and with BOOTSTD_FULL we can choose what to exactly boot. > >>> The boot_targets environment variable only allows you to choose which > >>> device to boot, not which boot method / flow to use. > >>> We have ample space in SPI Flash, so why not let it have the full > >>> potential of U-Boot by default. > >> > >> You can control boot targets using the boot_targets env var and boot > >> methods using the bootmeths env var. > >> > >> https://docs.u-boot.org/en/latest/develop/bootstd.html#controlling-ordering > >> > >> For normal generic use the full bootstd commands should not be needed, > >> do you have special scripting requirements that require access to full > >> bootstd commands? > >> > >> All rockchip boards use standard boot, this only enables full commands > >> for one particular board, why is this board special? Or is there merit > >> to imply enable of full commands for all rockchip boards? > > > > I suppose we do this in a lot of cases, where we enable commands and > > other features on various boards. So I don't have any objection to > > this. Certainly it is a pain to use bootstd for development and > > debugging of booting if you cannot use the full command set. > > I have no particular objection either, I just want to understand the > need, and if this is something we should apply to more boards. > > For RK35xx family of devices I have tried to ensure all boards have > similar commands and features enabled, at least for the features the > board support. So my thought now is if we should imply this Kconfig > option for ROCKCHIP_RK3568 and ROCKCHIP_RK3588. Yes, I understand...I happen to use rockpro64 for things so often find myself enabling various options. I enabled bootstage a while back to help with boot timing. > > Use of different features is also rather annoying for a distro that > wants to support a broad range of devices of a soc family, and U-Boot > for boards of the same soc family behave differently. Indeed. We need the basics supported on all boards and distros need to be careful not to use commands that are not widely enabled. Regards, Simon > > Regards, > Jonas > > > > > Reviewed-by: Simon Glass > > > > Regards, > > Simon > > > >> > >> Regards, > >> Jonas > >> > >>> > >>> Kind regards, > >>> Shantur > >>> > >>> On Sun, Nov 12, 2023 at 10:39 AM Jonas Karlman wrote: > >>>> > >>>> On 2023-11-11 01:13, Shantur Rathore wrote: > >>>>> RockPro64 has a 16MB onboard SPI chip and current u-boot takes > >>>>> around 2MB, we can enable more features. > >>>>> Updating config to enable SPI commands and full BootSTD support. > >>>>> --- > >>>>> configs/rockpro64-rk3399_defconfig | 2 ++ > >>>>> 1 file changed, 2 insertions(+) > >>>>> > >>>>> diff --git a/configs/rockpro64-rk3399_defconfig > >>>>> b/configs/rockpro64-rk3399_defconfig > >>>>> index 4cd6b76665..cad0b8a5d7 100644 > >>>>> --- a/configs/rockpro64-rk3399_defconfig > >>>>> +++ b/configs/rockpro64-rk3399_defco
Re: [PATCH v2 2/6] board: ti: am62x: evm: Remove video_setup from spl_board_init
On Fri, 10 Nov 2023 at 08:29, Devarsh Thakkar wrote: > > Remove video_setup from evm_init sequence since video memory > is getting called at an earlier place to make sure > video memory is reserved at the end of RAM. > > Suggested-by: Simon Glass > Signed-off-by: Devarsh Thakkar > --- > V2: No change > V3: No change > --- > board/ti/am62x/evm.c | 18 -- > 1 file changed, 18 deletions(-) Reviewed-by: Simon Glass
Re: [PATCH v2 5/6] video: Fill video handoff in video post probe
Hi Devarsh,
On Fri, 10 Nov 2023 at 08:29, Devarsh Thakkar wrote:
>
> Fill video handoff fields in video_post_probe
> as at this point we have full framebuffer-related
> information.
>
> Also fill all the fields available in video hand-off
> struct as those were missing earlier and U-boot
U-Boot
> framework expects them to be filled for some of the
> functionalities.
Can you wrap your commit messages to closer to 72 chars?
>
> Reported-by: Simon Glass
> Signed-off-by: Devarsh Thakkar
> ---
> V2:
> - No change
>
> V3:
> - Fix commit message per review comment
> - Add a note explaining assumption of single framebuffer
> ---
> drivers/video/video-uclass.c | 29 +++--
> 1 file changed, 19 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c
> index f619b5ae56..edc3376b46 100644
> --- a/drivers/video/video-uclass.c
> +++ b/drivers/video/video-uclass.c
> @@ -154,16 +154,6 @@ int video_reserve(ulong *addrp)
> debug("Video frame buffers from %lx to %lx\n", gd->video_bottom,
> gd->video_top);
>
> - if (spl_phase() == PHASE_SPL && CONFIG_IS_ENABLED(VIDEO_HANDOFF)) {
> - struct video_handoff *ho;
> -
> - ho = bloblist_add(BLOBLISTT_U_BOOT_VIDEO, sizeof(*ho), 0);
> - if (!ho)
> - return log_msg_ret("blf", -ENOENT);
> - ho->fb = *addrp;
> - ho->size = size;
> - }
> -
> return 0;
> }
>
> @@ -559,6 +549,25 @@ static int video_post_probe(struct udevice *dev)
>
> priv->fb_size = priv->line_length * priv->ysize;
>
> + /*
> +* Set up video handoff fields for passing video blob to next stage
> +* NOTE:
> +* This assumes that reserved video memory only uses a single
> framebuffer
> +*/
> + if (spl_phase() == PHASE_SPL && CONFIG_IS_ENABLED(BLOBLIST)) {
> + struct video_handoff *ho;
> +
> + ho = bloblist_add(BLOBLISTT_U_BOOT_VIDEO, sizeof(*ho), 0);
> + if (!ho)
> + return log_msg_ret("blf", -ENOENT);
> + ho->fb = gd->video_bottom;
> + ho->size = gd->video_top - gd->video_bottom;
should be plat->base and plat->size
> + ho->xsize = priv->xsize;
> + ho->ysize = priv->ysize;
> + ho->line_length = priv->line_length;
> + ho->bpix = priv->bpix;
> + }
> +
> if (IS_ENABLED(CONFIG_VIDEO_COPY) && plat->copy_base)
> priv->copy_fb = map_sysmem(plat->copy_base, plat->size);
>
> --
> 2.34.1
>
Regards,
Simon
Re: [PATCH v4 4/5] dm: core: Modify default for OFNODE_MULTI_TREE
Hi Sean, On Fri, 3 Nov 2023 at 13:38, Simon Glass wrote: > > On Fri, 3 Nov 2023 at 12:38, wrote: > > > > From: Sean Edmond > > > > There is a preference to use the "ofnode" API for FDT fixups > > moving forward. The FDT fixup will usually be for the kernel FDT. To > > fixup the kernel FDT with the ofnode API, it's required to set the > > OFNODE_MULTI_TREE option. > > > > To ensure existing users of kaslr fdt fixup are not impacted, Let's modify > > the default value for OFNODE_MULTI_TREE to ensure it's always set if > > !OF_LIVE. This will cause a 1007 byte increase in the code size. > > > > Signed-off-by: Sean Edmond > > --- > > drivers/core/Kconfig | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > Reviewed-by: Simon Glass This actually disables the option for sandbox. You may as well enabled it by default always, since if OF_LIVE is active we are already adding a lot of code. That explains the cedit error that Tom mentioned. Regards, Simon
Re: [PATCH v2 1/2] acpi: fix struct acpi_xsdt
On Sun, 12 Nov 2023 at 00:03, Heinrich Schuchardt wrote: > > The size of the ACPI table header is not a multiple of 8. We have to mark > struct acpi_xsdt as packed to correctly access field Entry. > > Add a unit test for the offsets of field Entry in the RSDT and XSDT tables. > > Signed-off-by: Heinrich Schuchardt > --- > v2: > add unit test > --- > include/acpi/acpi_table.h | 2 +- > test/dm/acpi.c| 10 ++ > 2 files changed, 11 insertions(+), 1 deletion(-) Reviewed-by: Simon Glass
[PATCH v3 2/2] blk: Drop reference to DM_SPL
The intent here is to only allow SPL_LEGACY_BLK if !SPL_DM - i.e. that when driver model is enabled in SPL, legacy block cannot be used. However this combination is used by about 240 boards, so we cannot disallow it, at least not yet. So just drop the condition. Signed-off-by: Simon Glass --- (no changes since v1) drivers/block/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig index b897cf1a3d1b..6ad18889f61e 100644 --- a/drivers/block/Kconfig +++ b/drivers/block/Kconfig @@ -13,7 +13,7 @@ config BLK config SPL_LEGACY_BLOCK bool # "Enable Legacy Block Device" - depends on SPL && !DM_SPL + depends on SPL default y if SPL_MMC || SPL_USB_STORAGE || SCSI || NVME || IDE default y if SPL_AHCI_PCI help -- 2.42.0.869.gea05f2083d-goog
[PATCH v3 1/2] dm: core: Correct reference to DM_SPL in SPL_DM_STATS
This does not existing anymore. Update SPL_DM_STATS to use the correct Kconfig option, which is SPL_DM Signed-off-by: Simon Glass --- Changes in v3: - Split the patch into two Changes in v2: - Add new patch to correct references to DM_SPL drivers/core/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/core/Kconfig b/drivers/core/Kconfig index fe5c41d57ecd..209d90e01fa5 100644 --- a/drivers/core/Kconfig +++ b/drivers/core/Kconfig @@ -88,7 +88,7 @@ config DM_STATS config SPL_DM_STATS bool "Collect and show driver model stats in SPL" - depends on DM_SPL + depends on SPL_DM help Enable this to collect and display memory statistics about driver model. This can help to figure out where all the memory is going and -- 2.42.0.869.gea05f2083d-goog
Re: [PATCH v2 6/6] doc: spl: Add info regarding memory reservation and missing Kconfigs
Hi Devarsh, On Fri, 10 Nov 2023 at 08:29, Devarsh Thakkar wrote: > > Add information regarding memory reservation scheme in SPL > and details regarding scheme which need to be followed while reserving > those areas which need to be preserved across bootstages. > > Also add missing CONFIG_SPL Kconfigs and new ones which were added > recently. > > Signed-off-by: Devarsh Thakkar > --- > V1->V3 : No change > --- > doc/develop/spl.rst | 22 ++ > 1 file changed, 22 insertions(+) > > diff --git a/doc/develop/spl.rst b/doc/develop/spl.rst > index 76e87f07c7..fc570589eb 100644 > --- a/doc/develop/spl.rst > +++ b/doc/develop/spl.rst > @@ -65,6 +65,15 @@ CONFIG_SPL_NAND_LOAD (drivers/mtd/nand/raw/nand_spl_load.o) > CONFIG_SPL_SPI_LOAD (drivers/mtd/spi/spi_spl_load.o) > CONFIG_SPL_RAM_DEVICE (common/spl/spl.c) > CONFIG_SPL_WATCHDOG (drivers/watchdog/libwatchdog.o) > +CONFIG_SPL_SYSCON (drivers/core/syscon-uclass.o) > +CONFIG_SPL_GZIP (lib/gzip.o) > +CONFIG_SPL_VIDEO (drivers/video/video-uclass.o > drivers/video/vidconsole-uclass.o) > +CONFIG_SPL_SPLASH_SCREEN (common/splash.o) > +CONFIG_SPL_SPLASH_SOURCE (common/splash_source.o) > +CONFIG_SPL_GPIO (drivers/gpio) > +CONFIG_SPL_DM_GPIO (drivers/gpio/gpio-uclass.o) > +CONFIG_SPL_BMP (drivers/video/bmp.o) > +CONFIG_SPL_BLOBLIST (common/bloblist.o) Did you intend to add the above? If so, please put it in its own patch. > > Adding SPL-specific code > > @@ -164,3 +173,16 @@ cflow will spit out a number of warnings as it does not > parse > the config files and picks functions based on #ifdef. Parsing the '.i' > files instead introduces another set of headaches. These warnings are > not usually important to understanding the flow, however. > + > + > +Reserving memory in SPL > +--- > + > +If memory need to be reserved in RAM during SPL stage so that area won't get > touched > +by SPL and/or u-boot, it need to be reserved starting from end of RAM. U-Boot (please avoid using any other spelling in docs and comments; please fix globally) > + > +Also the regions which are to be preserved across further stages of boot > need to be reserved first starting from > +framebuffer memory which must be reserved from end of RAM for which helper > function spl_reserve_video_from_ram_top is provided. Worth mentioning that framebuffer being reserved first means it is at the top of the reservation area with everything else reserved below it? > + > +The corresponding information of reservation for those regions can be passed > to further stages of boot using a bloblist. > +For e.g. the information for framebuffer area reserved by SPL can be passed > onto u-boot using BLOBLISTT_U_BOOT_VIDEO. Can you please wrap to 80 cols? > -- > 2.34.1 > Regards, Simon
Re: [PATCH v4 3/5] cmd: kaslrseed: Use common API to fixup FDT
Hi Sean,
On Fri, 3 Nov 2023 at 12:38, wrote:
>
> From: Sean Edmond
>
> Use the newly introduced common API fdt_fixup_kaslr_seed() in the
> kaslrseed command.
>
> Signed-off-by: Sean Edmond
> ---
> cmd/kaslrseed.c | 22 --
> 1 file changed, 8 insertions(+), 14 deletions(-)
>
> diff --git a/cmd/kaslrseed.c b/cmd/kaslrseed.c
> index 9acb8e16386..6f423702e7c 100644
> --- a/cmd/kaslrseed.c
> +++ b/cmd/kaslrseed.c
> @@ -19,7 +19,7 @@ static int do_kaslr_seed(struct cmd_tbl *cmdtp, int flag,
> int argc, char *const
> size_t n = 0x8;
> struct udevice *dev;
> u64 *buf;
> - int nodeoffset;
> + ofnode root;
> int ret = CMD_RET_SUCCESS;
>
> if (uclass_get_device(UCLASS_RNG, 0, ) || !dev) {
> @@ -45,21 +45,15 @@ static int do_kaslr_seed(struct cmd_tbl *cmdtp, int flag,
> int argc, char *const
> return CMD_RET_FAILURE;
> }
>
> - ret = fdt_check_header(working_fdt);
> - if (ret < 0) {
> - printf("fdt_chosen: %s\n", fdt_strerror(ret));
> - return CMD_RET_FAILURE;
> - }
> -
> - nodeoffset = fdt_find_or_add_subnode(working_fdt, 0, "chosen");
> - if (nodeoffset < 0) {
> - printf("Reading chosen node failed\n");
> - return CMD_RET_FAILURE;
> + ret = root_ofnode_from_fdt(working_fdt, );
> + if (ret) {
> + printf("ERROR: Unable to get root ofnode\n");
> + goto CMD_RET_FAILURE;
> }
>
> - ret = fdt_setprop(working_fdt, nodeoffset, "kaslr-seed", buf,
> sizeof(buf));
It looks like this buf needs a cast to u8 *
> - if (ret < 0) {
> - printf("Unable to set kaslr-seed on chosen node: %s\n",
> fdt_strerror(ret));
> + ret = fdt_fixup_kaslr_seed(root, buf, sizeof(buf));
> + if (ret) {
> + printf("ERROR: failed to add kaslr-seed to fdt\n");
> return CMD_RET_FAILURE;
> }
>
> --
> 2.42.0
>
Regards,
Simon
Re: [PATCH 1/4] acpi: Use __packed with struct acpi_xsdt
Hi Heinrich,
On Sun, 12 Nov 2023 at 09:46, Heinrich Schuchardt
wrote:
>
> On 11/12/23 16:58, Simon Glass wrote:
> > Since struct acpi_table_header is not a multiple of 64 bits, use the
> > __packed option for struct acpi_xsdt
> >
> > This ensures that the entry[] array starts on the correct boundary.
> >
>
> Typically we keep the original signed-off-by when reposting patches.
>
> Thanks for picking this change up.
>
> Signed-off-by: Heinrich Schuchardt
>
> As in your review you suggested to add a unit test I have done so in my v2:
>
> [PATCH v2 1/2] acpi: fix struct acpi_xsdt
> https://patchwork.ozlabs.org/project/uboot/patch/20231112070316.17982-2-heinrich.schucha...@canonical.com/
Oh yes, this is a duplicate patch to yours...I had it hanging around
for a while. So we can drop it.
>
> > Signed-off-by: Simon Glass
> > ---
> >
> > include/acpi/acpi_table.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/acpi/acpi_table.h b/include/acpi/acpi_table.h
> > index 1f85de091d39..59ab79ed17c2 100644
> > --- a/include/acpi/acpi_table.h
> > +++ b/include/acpi/acpi_table.h
> > @@ -80,7 +80,7 @@ struct acpi_rsdt {
> > };
> >
> > /* XSDT (Extended System Description Table) */
> > -struct acpi_xsdt {
> > +struct __packed acpi_xsdt {
> > struct acpi_table_header header;
> > u64 entry[MAX_ACPI_TABLES];
> > };
>
Regards,
Simon
Re: [PATCH 3/4] efi: Correct display of table GUIDs
Hi Heinrich,
On Sun, 12 Nov 2023 at 09:58, Heinrich Schuchardt wrote:
>
> On 11/12/23 16:58, Simon Glass wrote:
> > The printf() %pU option decodes GUIDs so it is not necessary to do this
> > first. Drop the incorrect code.
> >
> > Signed-off-by: Simon Glass
> > ---
> >
> > cmd/efi_common.c | 4 +---
> > 1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/cmd/efi_common.c b/cmd/efi_common.c
> > index f4056096cd3f..1aa2351fcdfd 100644
> > --- a/cmd/efi_common.c
> > +++ b/cmd/efi_common.c
> > @@ -17,10 +17,8 @@ void efi_show_tables(struct efi_system_table *systab)
> >
> > for (i = 0; i < systab->nr_tables; i++) {
> > struct efi_configuration_table *tab = >tables[i];
> > - char guid_str[37];
> >
> > - uuid_bin_to_str(tab->guid.b, guid_str, 1);
> > - printf("%p %pUl %s\n", tab->table, guid_str,
> > + printf("%p %pUl %s\n", tab->table, tab->guid.b,
> > uuid_guid_get_str(tab->guid.b) ?: "(unknown)");
>
> Please, observe, that we have printf("%pUs", ) for printing the
> text representation of a GUID. If the text representation is unknown, it
> will print the numeric representation.
Yes, I understand but this is the output I want:
=> efi tables
bfbd7690 ee4e5898-3914-4259-9d6e-dc7bd79403cf EFI_LZMA_COMPRESSED
bff27c40 05ad34ba-6f02-4214-952e-4da0398e2bb9 EFI_DXE_SERVICES
bfbd4010 7739f24c-93d7-11d4-9a3a-0090273fc14d EFI_HOB_LIST
bff283c0 4c19049f-4137-4dd3-9c10-8b97a83ffdfa EFI_MEMORY_TYPE
bff2892c 49152e77-1ada-4764-b7a2-7afefed95e8b (unknown)
bfcb6210 060cc026-4c0d-4dda-8f41-595fef00a502 EFI_MEM_STATUS_CODE_REC
bfc8 eb9d2d31-2d88-11d3-9a16-0090273fc14d SMBIOS table
bfe67000 eb9d2d30-2d88-11d3-9a16-0090273fc14d EFI_GUID_EFI_ACPI1
bfe67014 8868e871-e4f1-11d3-bc22-0080c73c8881 ACPI table
be9b9010 dcfa911d-26eb-469f-a220-38b7dc461220 (unknown)
(so I want to show the GUID in all cases, but also indicate which ones
are unknown)
Regards,
Simon
Re: [PATCH v4 1/5] fdt: common API to populate kaslr seed
Hi Sean,
On Fri, 3 Nov 2023 at 12:39, wrote:
>
> From: Dhananjay Phadke
>
> fdt_fixup_kaslr_seed() will update given ofnode with random seed value.
> Source for random seed can be TPM or RNG driver in u-boot or sec
> firmware (ARM).
>
> Signed-off-by: Dhananjay Phadke
> Signed-off-by: Sean Edmond
> ---
> arch/arm/cpu/armv8/sec_firmware.c | 39 +++
> boot/fdt_support.c| 19 +++
> drivers/core/ofnode.c | 17 ++
> include/dm/ofnode.h | 12 ++
> include/fdt_support.h | 9 +++
> 5 files changed, 71 insertions(+), 25 deletions(-)
>
> diff --git a/arch/arm/cpu/armv8/sec_firmware.c
> b/arch/arm/cpu/armv8/sec_firmware.c
> index c0e8726346f..5f04cd8aecd 100644
> --- a/arch/arm/cpu/armv8/sec_firmware.c
> +++ b/arch/arm/cpu/armv8/sec_firmware.c
> @@ -411,46 +411,35 @@ int sec_firmware_init(const void *sec_firmware_img,
> /*
> * fdt_fix_kaslr - Add kalsr-seed node in Device tree
> * @fdt: Device tree
> - * @eret: 0 in case of error, 1 for success
> + * @eret: 0 for success
> */
> int fdt_fixup_kaslr(void *fdt)
> {
> - int nodeoffset;
> - int err, ret = 0;
> - u8 rand[8];
> + int ret = 0;
>
> #if defined(CONFIG_ARMV8_SEC_FIRMWARE_SUPPORT)
> + u8 rand[8];
> + ofnode root;
> +
> /* Check if random seed generation is supported */
> if (sec_firmware_support_hwrng() == false) {
> printf("WARNING: SEC firmware not running, no kaslr-seed\n");
> - return 0;
> + return -EOPNOTSUPP;
> }
>
> - err = sec_firmware_get_random(rand, 8);
> - if (err < 0) {
> + ret = sec_firmware_get_random(rand, 8);
> + if (ret < 0) {
> printf("WARNING: No random number to set kaslr-seed\n");
> - return 0;
> + return ret;
> }
>
> - err = fdt_check_header(fdt);
> - if (err < 0) {
> - printf("fdt_chosen: %s\n", fdt_strerror(err));
> - return 0;
> + ret = root_ofnode_from_fdt(fdt, );
> + if (ret < 0) {
> + printf("WARNING: Unable to get root ofnode\n");
> + return ret;
> }
>
> - /* find or create "/chosen" node. */
> - nodeoffset = fdt_find_or_add_subnode(fdt, 0, "chosen");
> - if (nodeoffset < 0)
> - return 0;
> -
> - err = fdt_setprop(fdt, nodeoffset, "kaslr-seed", rand,
> - sizeof(rand));
> - if (err < 0) {
> - printf("WARNING: can't set kaslr-seed %s.\n",
> - fdt_strerror(err));
> - return 0;
> - }
> - ret = 1;
> + ret = fdt_fixup_kaslr_seed(root, rand, sizeof(rand));
> #endif
>
> return ret;
> diff --git a/boot/fdt_support.c b/boot/fdt_support.c
> index 5e49078f8c3..52be4375b46 100644
> --- a/boot/fdt_support.c
> +++ b/boot/fdt_support.c
> @@ -631,6 +631,25 @@ void fdt_fixup_ethernet(void *fdt)
> }
> }
>
> +int fdt_fixup_kaslr_seed(ofnode node, const u8 *seed, int len)
> +{
> + ofnode chosen;
> + int ret;
> +
> + /* find or create "/chosen" node. */
> + ret = ofnode_add_subnode(node, "chosen", );
> + if (ret && ret != -EEXIST)
> + return -ENOENT;
> +
> + ret = ofnode_write_prop(chosen, "kaslr-seed", seed, len, true);
> + if (ret) {
> + printf("WARNING: can't set kaslr-seed\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> int fdt_record_loadable(void *blob, u32 index, const char *name,
> uintptr_t load_addr, u32 size, uintptr_t entry_point,
> const char *type, const char *os, const char *arch)
> diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c
> index 29a42945102..55291f0202b 100644
> --- a/drivers/core/ofnode.c
> +++ b/drivers/core/ofnode.c
> @@ -966,6 +966,23 @@ ofnode oftree_path(oftree tree, const char *path)
> }
> }
>
> +int root_ofnode_from_fdt(void *fdt, ofnode *root_node)
> +{
> + oftree tree;
> + /* If OFNODE_MULTI_TREE is not set, and if fdt is not the control FDT,
> +* oftree_from_fdt() will return NULL
> +*/
> + tree = oftree_from_fdt(fdt);
> +
> + if (!oftree_valid(tree)) {
> + printf("Cannot create oftree\n");
> + return -EINVAL;
> + }
> + *root_node = oftree_root(tree);
> +
> + return 0;
> +}
> +
> const void *ofnode_read_chosen_prop(const char *propname, int *sizep)
> {
> ofnode chosen_node;
> diff --git a/include/dm/ofnode.h b/include/dm/ofnode.h
> index 19e97a90327..5759cac5b30 100644
> --- a/include/dm/ofnode.h
> +++ b/include/dm/ofnode.h
> @@ -936,6 +936,18 @@ ofnode oftree_path(oftree tree, const char *path);
> */
> ofnode oftree_root(oftree tree);
>
> +/**
> + *
Re: [PATCH v1 1/2] common: board_f: change calculation of gd->mon_len to fix s5p4418 reloc
On Sun, 12 Nov 2023 at 07:40, Stefan Bosch wrote:
>
> ARM and MICROBLAZE: Change calculation of monitor length (gd->mon_len)
> to fix relocation at boards with s5p4418-SoC. At s5p4418, _start is
> after the header (NSIH) therefore the monitor length has to be
> calculated using __image_copy_start instead of _start in order the hole
whole
> monitor code is relocated.
>
> Signed-off-by: Stefan Bosch Reviewed-by: Simon Glass
>
> ---
>
> common/board_f.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Simon Glass
>
> diff --git a/common/board_f.c b/common/board_f.c
> index d4d7d01f8f..d2e4d9eae2 100644
> --- a/common/board_f.c
> +++ b/common/board_f.c
> @@ -283,7 +283,7 @@ static int init_func_i2c(void)
> static int setup_mon_len(void)
> {
> #if defined(__ARM__) || defined(__MICROBLAZE__)
> - gd->mon_len = (ulong)__bss_end - (ulong)_start;
> + gd->mon_len = (ulong)__bss_end - (ulong)__image_copy_start;
> #elif defined(CONFIG_SANDBOX) && !defined(__riscv)
> gd->mon_len = (ulong)_end - (ulong)_init;
> #elif defined(CONFIG_SANDBOX)
> --
> 2.17.1
>
Re: [PATCH 1/1] acpi: cannot have RSDT above 4 GiB
Hi Heinrich,
On Sat, 11 Nov 2023 at 07:28, Heinrich Schuchardt
wrote:
>
> The field RsdtAddress has only 32 bit. The RSDT table cannot be located
> beyond 4 GiB.
>
> Signed-off-by: Heinrich Schuchardt
> ---
> lib/acpi/base.c | 26 +++---
> 1 file changed, 19 insertions(+), 7 deletions(-)
Reviewed-by: Simon Glass
nits / question below
>
> diff --git a/lib/acpi/base.c b/lib/acpi/base.c
> index 2057bd2bef..128a76ad39 100644
> --- a/lib/acpi/base.c
> +++ b/lib/acpi/base.c
> @@ -21,10 +21,17 @@ void acpi_write_rsdp(struct acpi_rsdp *rsdp, struct
> acpi_rsdt *rsdt,
> memcpy(rsdp->signature, RSDP_SIG, 8);
> memcpy(rsdp->oem_id, OEM_ID, 6);
>
> - rsdp->length = sizeof(struct acpi_rsdp);
> - rsdp->rsdt_address = map_to_sysmem(rsdt);
> + if (rsdt)
> + rsdp->rsdt_address = map_to_sysmem(rsdt);
> + else
> + rsdp->rsdt_address = 0;
There is a memset() at the top so this line (and the one below) should
not be needed.
> +
> + if (xsdt)
> + rsdp->xsdt_address = map_to_sysmem(xsdt);
> + else
> + rsdp->xsdt_address = 0;
>
> - rsdp->xsdt_address = map_to_sysmem(xsdt);
> + rsdp->length = sizeof(struct acpi_rsdp);
> rsdp->revision = ACPI_RSDP_REV_ACPI_2_0;
>
> /* Calculate checksums */
> @@ -68,11 +75,15 @@ static void acpi_write_xsdt(struct acpi_xsdt *xsdt)
> static int acpi_write_base(struct acpi_ctx *ctx,
>const struct acpi_writer *entry)
> {
> - /* We need at least an RSDP and an RSDT Table */
> + /* We need at least an RSDP and an XSDT Table */
> ctx->rsdp = ctx->current;
> acpi_inc_align(ctx, sizeof(struct acpi_rsdp));
> - ctx->rsdt = ctx->current;
> - acpi_inc_align(ctx, sizeof(struct acpi_rsdt));
> + if (map_to_sysmem(ctx->current) < UINT_MAX - 0x1) {
Won't that do something different on 64-bit machines? It seems that
you want to check against SZ_4G ?
> + ctx->rsdt = ctx->current;
> + acpi_inc_align(ctx, sizeof(struct acpi_rsdt));
> + } else {
> + ctx->rsdt = 0;
> + }
> ctx->xsdt = ctx->current;
> acpi_inc_align(ctx, sizeof(struct acpi_xsdt));
>
> @@ -80,7 +91,8 @@ static int acpi_write_base(struct acpi_ctx *ctx,
> memset(ctx->base, '\0', ctx->current - ctx->base);
>
> acpi_write_rsdp(ctx->rsdp, ctx->rsdt, ctx->xsdt);
> - acpi_write_rsdt(ctx->rsdt);
> + if (ctx->rsdt)
> + acpi_write_rsdt(ctx->rsdt);
> acpi_write_xsdt(ctx->xsdt);
>
> return 0;
> --
> 2.40.1
>
Regards,
Simon
Re: [PATCH v2 1/6] arm: mach-k3: common: Reserve video memory from end of the RAM
Hi Devarsh,
On Fri, 10 Nov 2023 at 08:29, Devarsh Thakkar wrote:
>
> Add function spl_reserve_video which is a wrapper
> around video_reserve to setup video memory and update
> the relocation address pointer.
>
> Setup video memory before page table reservation so that
> framebuffer memory gets reserved from the end of RAM.
>
> This is as per the new policy being discussed for passing
> blobs where each of the reserved areas for bloblists
> to be passed need to be reserved at the end of RAM.
>
> This is done to enable the next stage to directly skip
> the pre-reserved area from previous stage right from the end of RAM
> without having to make any gaps/holes to accommodate those
> regions which was the case before as previous stage
> reserved region not from the end of RAM.
>
> Suggested-by: Simon Glass
> Signed-off-by: Devarsh Thakkar
> ---
> V2:
> - Make a generic function "spl_reserve_video" under
> common/spl which can be re-used by other platforms too
> for reserving video memory from spl.
>
> V3:
> - Change spl_reserve_video to spl_reserve_video_from_ram_top
> which enforce framebuffer reservation from end of RAM
> - Use gd->ram_top instead of local ram_top and update
> gd->reloc_addr after each reservation
> - Print error message on framebuffer reservation
> ---
> arch/arm/mach-k3/common.c | 17 -
Please split that off into its own patch, so that the generic code
stands on its own.
> common/spl/spl.c | 27 +++
> include/spl.h | 6 ++
> 3 files changed, 45 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c
> index c3006ba387..6590045140 100644
> --- a/arch/arm/mach-k3/common.c
> +++ b/arch/arm/mach-k3/common.c
> @@ -525,19 +525,26 @@ void remove_fwl_configs(struct fwl_data *fwl_data,
> size_t fwl_data_size)
> void spl_enable_dcache(void)
> {
> #if !(defined(CONFIG_SYS_ICACHE_OFF) && defined(CONFIG_SYS_DCACHE_OFF))
> - phys_addr_t ram_top = CFG_SYS_SDRAM_BASE;
> + gd->ram_top = CFG_SYS_SDRAM_BASE;
> + int ret = 0;
>
> dram_init();
>
> /* reserve TLB table */
> gd->arch.tlb_size = PGTABLE_SIZE;
>
> - ram_top += get_effective_memsize();
> + gd->ram_top += get_effective_memsize();
> /* keep ram_top in the 32-bit address space */
> - if (ram_top >= 0x1)
> - ram_top = (phys_addr_t) 0x1;
> + if (gd->ram_top >= 0x1)
> + gd->ram_top = (phys_addr_t)0x1;
>
> - gd->arch.tlb_addr = ram_top - gd->arch.tlb_size;
> + gd->relocaddr = gd->ram_top;
> +
> + ret = spl_reserve_video_from_ram_top();
> + if (ret)
> + pr_err("ERROR: Failed to framebuffer memory in SPL\n");
> +
> + gd->arch.tlb_addr = gd->relocaddr - gd->arch.tlb_size;
> gd->arch.tlb_addr &= ~(0x1 - 1);
> debug("TLB table from %08lx to %08lx\n", gd->arch.tlb_addr,
> gd->arch.tlb_addr + gd->arch.tlb_size);
> diff --git a/common/spl/spl.c b/common/spl/spl.c
> index 732d90d39e..452bf303de 100644
> --- a/common/spl/spl.c
> +++ b/common/spl/spl.c
> @@ -41,6 +41,7 @@
> #include
> #include
> #include
> +#include
>
> DECLARE_GLOBAL_DATA_PTR;
> DECLARE_BINMAN_MAGIC_SYM;
> @@ -151,6 +152,32 @@ void spl_fixup_fdt(void *fdt_blob)
> #endif
> }
>
> +/*
> + * Reserve video memory for SPL splash screen from
> + * end of RAM
> + *
> + * RETURN
> + * 0 : On success
> + * Non-zero : On failure
> + */
Drop that comment as you have one in the header
> +int spl_reserve_video_from_ram_top(void)
> +{
> + if (CONFIG_IS_ENABLED(VIDEO)) {
> + ulong addr;
> + int ret;
> +
> + addr = gd->ram_top;
> + ret = video_reserve();
> + if (ret)
> + return ret;
> + debug("Reserving %luk for video at: %08lx\n",
> + ((unsigned long)gd->relocaddr - addr) >> 10, addr);
> + gd->relocaddr = addr;
> + }
> +
> + return 0;
> +}
> +
> ulong spl_get_image_pos(void)
> {
> if (!CONFIG_IS_ENABLED(BINMAN_UBOOT_SYMBOLS))
> diff --git a/include/spl.h b/include/spl.h
> index 0d49e4a454..39f2a7581d 100644
> --- a/include/spl.h
> +++ b/include/spl.h
> @@ -825,6 +825,12 @@ int spl_usb_load(struct spl_image_info *spl_image,
>
> int spl_ymodem_load_image(struct spl_image_info *spl_image,
> struct spl_boot_device *bootdev);
> +/**
> + * spl_reserve_video_from_ram_top() - Reserve framebuffer memory from end of
> RAM
> + *
This needs more detail
> + * Return: 0 on success, otherwise error code
> + */
> +int spl_reserve_video_from_ram_top(void);
>
> /**
> * spl_invoke_atf - boot using an ARM trusted firmware image
> --
> 2.34.1
>
Regards,
Simon
Re: [PATCH v2 2/2] cmd: acpi: fix acpi list command
On Sun, 12 Nov 2023 at 00:03, Heinrich Schuchardt wrote: > > ACPI tables may comprise either RSDT, XSDT, or both. The current code fails > to check the presence of the RSDT table before accessing it. This leads to > an exception if the RSDT table is not provided. > > The XSDT table takes precedence over the RSDT table. > > Addresses in the XSDT table are 64-bit. Adjust the output accordingly. > > As the RSDT table has to be ignored if the XSDT command is present there is > no need to compare the tables in a display command. Anyway the > specification does not require that the sequence of addresses in the RSDT > and XSDT table are the same. > > The FACS table header does not provide revision information. Correct the > description of dump_hdr(). > > Adjust the ACPI test to match the changed output format of the 'acpi list' > command. > > Signed-off-by: Heinrich Schuchardt > --- > v2: > add unit test for offset of field Entry in RSDT, XSDT > merge patch 2 and 3 > remove leading zeros from address output > --- > cmd/acpi.c | 48 +--- > test/dm/acpi.c | 18 +++++----- > 2 files changed, 38 insertions(+), 28 deletions(-) Reviewed-by: Simon Glass
Re: [PATCH 4/4] efi: Avoid using dm_scan_other()
Hi Tom, Heinrich,
On Sun, 12 Nov 2023 at 12:20, Heinrich Schuchardt wrote:
>
>
>
> Am 12. November 2023 19:03:57 MEZ schrieb Tom Rini :
> >On Sun, Nov 12, 2023 at 08:58:05AM -0700, Simon Glass wrote:
> >> This function is defined by bootstd so using it precludes using that
> >> feature. Use the board_early_init_r() feature instead.
> >>
> >> Signed-off-by: Simon Glass
> >> ---
> >>
> >> configs/efi-x86_app64_defconfig | 1 +
> >> lib/efi/efi_app.c | 5 ++---
> >> 2 files changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/configs/efi-x86_app64_defconfig
> >> b/configs/efi-x86_app64_defconfig
> >> index d6b6c3d82995..e6a62b30dd09 100644
> >> --- a/configs/efi-x86_app64_defconfig
> >> +++ b/configs/efi-x86_app64_defconfig
> >> @@ -17,6 +17,7 @@ CONFIG_USE_BOOTCOMMAND=y
> >> CONFIG_BOOTCOMMAND="ext2load scsi 0:3 0100 /boot/vmlinuz; zboot
> >> 0100"
> >> CONFIG_SYS_CONSOLE_INFO_QUIET=y
> >> CONFIG_DISPLAY_BOARDINFO_LATE=y
> >> +CONFIG_BOARD_EARLY_INIT_R=y
> >> CONFIG_HUSH_PARSER=y
> >> CONFIG_SYS_PBSIZE=532
> >> CONFIG_CMD_BOOTZ=y
> >> diff --git a/lib/efi/efi_app.c b/lib/efi/efi_app.c
> >> index c5eb816655ea..1bced775a4d0 100644
> >> --- a/lib/efi/efi_app.c
> >> +++ b/lib/efi/efi_app.c
> >> @@ -302,15 +302,14 @@ static int setup_block(void)
> >> }
> >>
> >> /**
> >> - * dm_scan_other() - Scan for UEFI devices that should be available to
> >> U-Boot
> >> + * board_early_init_r() - Scan for UEFI devices that should be available
> >> *
> >> * This sets up block devices within U-Boot for those found in UEFI. With
> >> this,
> >> * U-Boot can access those devices
> >> *
> >> - * @pre_reloc_only: true to only bind pre-relocation devices (ignored)
> >> * Returns: 0 on success, -ve on error
> >> */
> >> -int dm_scan_other(bool pre_reloc_only)
> >> +int board_early_init_r(void)
> >> {
> >> if (gd->flags & GD_FLG_RELOC) {
> >> int ret;
> >
> >Should this file really be board/efi/efi-x86_app/something.c ? We don't
OK will fix.
>
> The EFI app should not be x86 specific.
Sure, but so far we don't have an ARM version.
>
> Best regards
>
> Heinrich
>
>
> >define this function outside of the board directory today. Or should we
> >just move the function as the rest of the code is used in other EFI
> >applications we build (or no, the hello world one is too simple to need
> >this) ? If all of that sounds too wrong-direction, can we use event
> >lists or something?
> >
Regards,
Simon
