From 112eb85c5a23a0c4a3f14d7099f18fd3128996ec Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 7 Jan 2024 17:14:54 -0700 Subject: video: Allow querying the font size All the font size to be queried using the 'font size' command. Signed-off-by: Simon Glass --- cmd/font.c | 17 +++++++++-------- doc/usage/cmd/font.rst | 6 ++++-- test/cmd/font.c | 9 +++++++++ 3 files changed, 22 insertions(+), 10 deletions(-) diff --git a/cmd/font.c b/cmd/font.c index eb13fb12f79..36e41203654 100644 --- a/cmd/font.c +++ b/cmd/font.c @@ -55,9 +55,6 @@ static int do_font_size(struct cmd_tbl *cmdtp, int flag, int argc, uint size; int ret; - if (argc != 2) - return CMD_RET_USAGE; - if (uclass_first_device_err(UCLASS_VIDEO_CONSOLE, &dev)) return CMD_RET_FAILURE; ret = vidconsole_get_font_size(dev, &font_name, &size); @@ -66,12 +63,16 @@ static int do_font_size(struct cmd_tbl *cmdtp, int flag, int argc, return CMD_RET_FAILURE; } - size = dectoul(argv[1], NULL); + if (argc < 2) { + printf("%d\n", size); + } else { + size = dectoul(argv[1], NULL); - ret = vidconsole_select_font(dev, font_name, size); - if (ret) { - printf("Failed (error %d)\n", ret); - return CMD_RET_FAILURE; + ret = vidconsole_select_font(dev, font_name, size); + if (ret) { + printf("Failed (error %d)\n", ret); + return CMD_RET_FAILURE; + } } return 0; diff --git a/doc/usage/cmd/font.rst b/doc/usage/cmd/font.rst index a8782546333..44a04f5d075 100644 --- a/doc/usage/cmd/font.rst +++ b/doc/usage/cmd/font.rst @@ -13,7 +13,7 @@ Synopsis font list font select [] - font size + font size [] Description ----------- @@ -34,7 +34,7 @@ This selects a new font and optionally changes the size. font size ~~~~~~~~~ -This changes the font size only. +This changes the font size only. With no argument it shows the current size. Examples -------- @@ -44,6 +44,8 @@ Examples => font list nimbus_sans_l_regular cantoraone_regular + => font size + 30 => font size 40 => font select cantoraone_regular 20 => diff --git a/test/cmd/font.c b/test/cmd/font.c index 25d365dedd2..2aea7b68c47 100644 --- a/test/cmd/font.c +++ b/test/cmd/font.c @@ -58,10 +58,19 @@ static int font_test_base(struct unit_test_state *uts) ut_assertok(vidconsole_get_font_size(dev, &name, &size)); ut_asserteq_str("cantoraone_regular", name); ut_asserteq(40, size); + ut_assertok(ut_check_console_end(uts)); + + ut_assertok(run_command("font size", 0)); + ut_assert_nextline("40"); + ut_assertok(ut_check_console_end(uts)); ut_assertok(run_command("font size 30", 0)); ut_assert_console_end(); + ut_assertok(run_command("font size", 0)); + ut_assert_nextline("30"); + ut_assertok(ut_check_console_end(uts)); + ut_assertok(vidconsole_get_font_size(dev, &name, &size)); ut_asserteq_str("cantoraone_regular", name); ut_asserteq(30, size); -- cgit v1.3.1 From f0feda90826556d2841ff4168f7d21902d992d70 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 7 Jan 2024 17:14:55 -0700 Subject: coreboot: Switch to a monospaced font The default font is proportional, with different character widths. Select a monospace font for coreboot so that the 'dm tree' output lines up correctly. Update the coreboot tests to match. Signed-off-by: Simon Glass --- configs/coreboot64_defconfig | 3 +++ configs/coreboot_defconfig | 3 +++ test/cmd/font.c | 12 +++++++++--- 3 files changed, 15 insertions(+), 3 deletions(-) diff --git a/configs/coreboot64_defconfig b/configs/coreboot64_defconfig index da42ad0a20a..553f1dc83f0 100644 --- a/configs/coreboot64_defconfig +++ b/configs/coreboot64_defconfig @@ -61,6 +61,9 @@ CONFIG_SOUND=y CONFIG_SOUND_I8254=y CONFIG_VIDEO_COPY=y CONFIG_CONSOLE_TRUETYPE=y +CONFIG_CONSOLE_TRUETYPE_SIZE=20 +# CONFIG_CONSOLE_TRUETYPE_NIMBUS is not set +CONFIG_CONSOLE_TRUETYPE_ANKACODER=y CONFIG_CONSOLE_SCROLL_LINES=5 CONFIG_SPL_ACPI=y CONFIG_CMD_DHRYSTONE=y diff --git a/configs/coreboot_defconfig b/configs/coreboot_defconfig index 0b103efe16d..e68a3aed659 100644 --- a/configs/coreboot_defconfig +++ b/configs/coreboot_defconfig @@ -55,6 +55,9 @@ CONFIG_SOUND=y CONFIG_SOUND_I8254=y CONFIG_VIDEO_COPY=y CONFIG_CONSOLE_TRUETYPE=y +CONFIG_CONSOLE_TRUETYPE_SIZE=20 +# CONFIG_CONSOLE_TRUETYPE_NIMBUS is not set +CONFIG_CONSOLE_TRUETYPE_ANKACODER=y CONFIG_CONSOLE_SCROLL_LINES=5 CONFIG_CMD_DHRYSTONE=y # CONFIG_GZIP is not set diff --git a/test/cmd/font.c b/test/cmd/font.c index 2aea7b68c47..3335dd65bea 100644 --- a/test/cmd/font.c +++ b/test/cmd/font.c @@ -27,14 +27,20 @@ static int font_test_base(struct unit_test_state *uts) ut_assertok(uclass_first_device_err(UCLASS_VIDEO_CONSOLE, &dev)); ut_assertok(run_command("font list", 0)); - ut_assert_nextline("nimbus_sans_l_regular"); + if (IS_ENABLED(CONFIG_CONSOLE_TRUETYPE_NIMBUS)) + ut_assert_nextline("nimbus_sans_l_regular"); + if (IS_ENABLED(CONFIG_CONSOLE_TRUETYPE_ANKACODER)) + ut_assert_nextline("ankacoder_c75_r"); if (IS_ENABLED(CONFIG_CONSOLE_TRUETYPE_CANTORAONE)) ut_assert_nextline("cantoraone_regular"); ut_assert_console_end(); ut_assertok(vidconsole_get_font_size(dev, &name, &size)); - ut_asserteq_str("nimbus_sans_l_regular", name); - ut_asserteq(18, size); + if (IS_ENABLED(CONFIG_CONSOLE_TRUETYPE_ANKACODER)) + ut_asserteq_str("ankacoder_c75_r", name); + else + ut_asserteq_str("nimbus_sans_l_regular", name); + ut_asserteq(CONFIG_CONSOLE_TRUETYPE_SIZE, size); if (!IS_ENABLED(CONFIG_CONSOLE_TRUETYPE_CANTORAONE)) return 0; -- cgit v1.3.1 From b84d2f203edb102568cd63630394559d62d51737 Mon Sep 17 00:00:00 2001 From: Tom Rini Date: Mon, 22 Jan 2024 09:35:12 -0500 Subject: x86: Make default_print_cpuinfo be a weak alias for print_cpuinfo While a few SoCs have a unique print_cpuinfo function, a number of them just use default_print_cpuinfo. Make default_print_cpuinfo have a weak alias to provie print_cpuinfo. Signed-off-by: Tom Rini --- arch/x86/cpu/coreboot/coreboot.c | 5 ----- arch/x86/cpu/cpu.c | 2 ++ arch/x86/cpu/efi/app.c | 5 ----- arch/x86/cpu/efi/payload.c | 5 ----- arch/x86/cpu/slimbootloader/slimbootloader.c | 5 ----- arch/x86/cpu/tangier/tangier.c | 5 ----- 6 files changed, 2 insertions(+), 25 deletions(-) diff --git a/arch/x86/cpu/coreboot/coreboot.c b/arch/x86/cpu/coreboot/coreboot.c index c3d7442b4a8..ef1a3f42d2e 100644 --- a/arch/x86/cpu/coreboot/coreboot.c +++ b/arch/x86/cpu/coreboot/coreboot.c @@ -43,11 +43,6 @@ int checkcpu(void) return 0; } -int print_cpuinfo(void) -{ - return default_print_cpuinfo(); -} - static void board_final_init(void) { /* diff --git a/arch/x86/cpu/cpu.c b/arch/x86/cpu/cpu.c index ea11b09eacc..f8549e32092 100644 --- a/arch/x86/cpu/cpu.c +++ b/arch/x86/cpu/cpu.c @@ -163,6 +163,8 @@ char *cpu_get_name(char *name) return ptr; } +int print_cpuinfo(void) __attribute__((weak, alias("default_print_cpuinfo"))); + int default_print_cpuinfo(void) { printf("CPU: %s, vendor %s, device %xh\n", diff --git a/arch/x86/cpu/efi/app.c b/arch/x86/cpu/efi/app.c index 218a68c4642..84fe50e2f2f 100644 --- a/arch/x86/cpu/efi/app.c +++ b/arch/x86/cpu/efi/app.c @@ -19,11 +19,6 @@ int checkcpu(void) return 0; } -int print_cpuinfo(void) -{ - return default_print_cpuinfo(); -} - void board_final_init(void) { } diff --git a/arch/x86/cpu/efi/payload.c b/arch/x86/cpu/efi/payload.c index 642a87a37d8..6845ce72ff9 100644 --- a/arch/x86/cpu/efi/payload.c +++ b/arch/x86/cpu/efi/payload.c @@ -144,11 +144,6 @@ int checkcpu(void) return 0; } -int print_cpuinfo(void) -{ - return default_print_cpuinfo(); -} - /* Find any available tables and copy them to a safe place */ int reserve_arch(void) { diff --git a/arch/x86/cpu/slimbootloader/slimbootloader.c b/arch/x86/cpu/slimbootloader/slimbootloader.c index 142c9341cf8..8a5c78595aa 100644 --- a/arch/x86/cpu/slimbootloader/slimbootloader.c +++ b/arch/x86/cpu/slimbootloader/slimbootloader.c @@ -54,8 +54,3 @@ int checkcpu(void) { return 0; } - -int print_cpuinfo(void) -{ - return default_print_cpuinfo(); -} diff --git a/arch/x86/cpu/tangier/tangier.c b/arch/x86/cpu/tangier/tangier.c index 8a8f7d27a9d..b005bc7d9a0 100644 --- a/arch/x86/cpu/tangier/tangier.c +++ b/arch/x86/cpu/tangier/tangier.c @@ -19,8 +19,3 @@ int checkcpu(void) { return 0; } - -int print_cpuinfo(void) -{ - return default_print_cpuinfo(); -} -- cgit v1.3.1 From 9fb768c27c353048dc1e40d71153acdc2bbf1a1a Mon Sep 17 00:00:00 2001 From: Dmitrii Merkurev Date: Wed, 6 Mar 2024 18:59:18 +0000 Subject: virtio: blk: introduce virtio-block erase support Co-developed-by: Cody Schuffelen Signed-off-by: Cody Schuffelen Signed-off-by: Dmitrii Merkurev Cc: Tuomas Tynkkynen Cc: Simon Glass Cc: Mattijs Korpershoek Cc: Ying-Chun Liu (PaulLiu) Tested-by: Mattijs Korpershoek # sandbox --- drivers/virtio/virtio_blk.c | 91 ++++++++++++++++++++++++++++++++++++++------- drivers/virtio/virtio_blk.h | 47 +++++++++++++++++++++++ 2 files changed, 124 insertions(+), 14 deletions(-) diff --git a/drivers/virtio/virtio_blk.c b/drivers/virtio/virtio_blk.c index 3404f61eba5..2f999fc8bbe 100644 --- a/drivers/virtio/virtio_blk.c +++ b/drivers/virtio/virtio_blk.c @@ -18,30 +18,82 @@ struct virtio_blk_priv { struct virtqueue *vq; }; +static const u32 feature[] = { + VIRTIO_BLK_F_WRITE_ZEROES +}; + +static void virtio_blk_init_header_sg(struct udevice *dev, u64 sector, u32 type, + struct virtio_blk_outhdr *out_hdr, struct virtio_sg *sg) +{ + const bool sector_is_needed = type == VIRTIO_BLK_T_IN || + type == VIRTIO_BLK_T_OUT; + + out_hdr->type = cpu_to_virtio32(dev, type); + out_hdr->sector = cpu_to_virtio64(dev, sector_is_needed ? sector : 0); + + sg->addr = out_hdr; + sg->length = sizeof(*out_hdr); +} + +static void virtio_blk_init_write_zeroes_sg(struct udevice *dev, u64 sector, lbaint_t blkcnt, + struct virtio_blk_discard_write_zeroes *wz, + struct virtio_sg *sg) +{ + wz->sector = cpu_to_virtio64(dev, sector); + wz->num_sectors = cpu_to_virtio32(dev, blkcnt); + wz->flags = cpu_to_virtio32(dev, 0); + + sg->addr = wz; + sg->length = sizeof(*wz); +} + +static void virtio_blk_init_status_sg(u8 *status, struct virtio_sg *sg) +{ + sg->addr = status; + sg->length = sizeof(*status); +} + +static void virtio_blk_init_data_sg(void *buffer, lbaint_t blkcnt, struct virtio_sg *sg) +{ + sg->addr = buffer; + sg->length = blkcnt * 512; +} + static ulong virtio_blk_do_req(struct udevice *dev, u64 sector, lbaint_t blkcnt, void *buffer, u32 type) { struct virtio_blk_priv *priv = dev_get_priv(dev); + struct virtio_blk_outhdr out_hdr; + struct virtio_blk_discard_write_zeroes wz_hdr; unsigned int num_out = 0, num_in = 0; + struct virtio_sg hdr_sg, wz_sg, data_sg, status_sg; struct virtio_sg *sgs[3]; u8 status; int ret; - struct virtio_blk_outhdr out_hdr = { - .type = cpu_to_virtio32(dev, type), - .sector = cpu_to_virtio64(dev, sector), - }; - struct virtio_sg hdr_sg = { &out_hdr, sizeof(out_hdr) }; - struct virtio_sg data_sg = { buffer, blkcnt * 512 }; - struct virtio_sg status_sg = { &status, sizeof(status) }; - + virtio_blk_init_header_sg(dev, sector, type, &out_hdr, &hdr_sg); sgs[num_out++] = &hdr_sg; - if (type & VIRTIO_BLK_T_OUT) - sgs[num_out++] = &data_sg; - else - sgs[num_out + num_in++] = &data_sg; - + switch (type) { + case VIRTIO_BLK_T_IN: + case VIRTIO_BLK_T_OUT: + virtio_blk_init_data_sg(buffer, blkcnt, &data_sg); + if (type & VIRTIO_BLK_T_OUT) + sgs[num_out++] = &data_sg; + else + sgs[num_out + num_in++] = &data_sg; + break; + + case VIRTIO_BLK_T_WRITE_ZEROES: + virtio_blk_init_write_zeroes_sg(dev, sector, blkcnt, &wz_hdr, &wz_sg); + sgs[num_out++] = &wz_sg; + break; + + default: + return -EINVAL; + } + + virtio_blk_init_status_sg(&status, &status_sg); sgs[num_out + num_in++] = &status_sg; log_debug("dev=%s, active=%d, priv=%p, priv->vq=%p\n", dev->name, device_active(dev), priv, priv->vq); @@ -75,6 +127,15 @@ static ulong virtio_blk_write(struct udevice *dev, lbaint_t start, VIRTIO_BLK_T_OUT); } +static ulong virtio_blk_erase(struct udevice *dev, lbaint_t start, + lbaint_t blkcnt) +{ + if (!virtio_has_feature(dev, VIRTIO_BLK_F_WRITE_ZEROES)) + return -EOPNOTSUPP; + + return virtio_blk_do_req(dev, start, blkcnt, NULL, VIRTIO_BLK_T_WRITE_ZEROES); +} + static int virtio_blk_bind(struct udevice *dev) { struct virtio_dev_priv *uc_priv = dev_get_uclass_priv(dev->parent); @@ -104,7 +165,8 @@ static int virtio_blk_bind(struct udevice *dev) desc->bdev = dev; /* Indicate what driver features we support */ - virtio_driver_features_init(uc_priv, NULL, 0, NULL, 0); + virtio_driver_features_init(uc_priv, feature, ARRAY_SIZE(feature), + NULL, 0); return 0; } @@ -131,6 +193,7 @@ static int virtio_blk_probe(struct udevice *dev) static const struct blk_ops virtio_blk_ops = { .read = virtio_blk_read, .write = virtio_blk_write, + .erase = virtio_blk_erase, }; U_BOOT_DRIVER(virtio_blk) = { diff --git a/drivers/virtio/virtio_blk.h b/drivers/virtio/virtio_blk.h index 8d8e02fa2ea..b37ba264df4 100644 --- a/drivers/virtio/virtio_blk.h +++ b/drivers/virtio/virtio_blk.h @@ -17,6 +17,8 @@ #define VIRTIO_BLK_F_BLK_SIZE 6 /* Block size of disk is available */ #define VIRTIO_BLK_F_TOPOLOGY 10 /* Topology information is available */ #define VIRTIO_BLK_F_MQ 12 /* Support more than one vq */ +#define VIRTIO_BLK_F_DISCARD 13 /* Discard is supported */ +#define VIRTIO_BLK_F_WRITE_ZEROES 14 /* Write zeroes is supported */ /* Legacy feature bits */ #ifndef VIRTIO_BLK_NO_LEGACY @@ -65,6 +67,39 @@ struct __packed virtio_blk_config { /* number of vqs, only available when VIRTIO_BLK_F_MQ is set */ __u16 num_queues; + + /* the next 3 entries are guarded by VIRTIO_BLK_F_DISCARD */ + /* + * The maximum discard sectors (in 512-byte sectors) for + * one segment. + */ + __u32 max_discard_sectors; + /* + * The maximum number of discard segments in a + * discard command. + */ + __u32 max_discard_seg; + /* Discard commands must be aligned to this number of sectors. */ + __u32 discard_sector_alignment; + + /* the next 3 entries are guarded by VIRTIO_BLK_F_WRITE_ZEROES */ + /* + * The maximum number of write zeroes sectors (in 512-byte sectors) in + * one segment. + */ + __u32 max_write_zeroes_sectors; + /* + * The maximum number of segments in a write zeroes + * command. + */ + __u32 max_write_zeroes_seg; + /* + * Set if a VIRTIO_BLK_T_WRITE_ZEROES request may result in the + * deallocation of one or more of the sectors. + */ + __u8 write_zeroes_may_unmap; + + __u8 unused1[3]; }; /* @@ -93,6 +128,9 @@ struct __packed virtio_blk_config { /* Get device ID command */ #define VIRTIO_BLK_T_GET_ID 8 +/* Write zeroes command */ +#define VIRTIO_BLK_T_WRITE_ZEROES 13 + #ifndef VIRTIO_BLK_NO_LEGACY /* Barrier before this op */ #define VIRTIO_BLK_T_BARRIER 0x80000000 @@ -112,6 +150,15 @@ struct virtio_blk_outhdr { __virtio64 sector; }; +struct virtio_blk_discard_write_zeroes { + /* discard/write zeroes start sector */ + __virtio64 sector; + /* number of discard/write zeroes sectors */ + __virtio32 num_sectors; + /* flags for this range */ + __virtio32 flags; +}; + #ifndef VIRTIO_BLK_NO_LEGACY struct virtio_scsi_inhdr { __virtio32 errors; -- cgit v1.3.1 From 6ab545ba21668132d9dad27026c91d298b6e5a77 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Tue, 27 Aug 2024 19:44:24 -0600 Subject: x86: Show the CPU vendor in bdinfo Refactor the cpu code and use it to show the CPU vendor, e.g. AuthenticAMD or GenuineIntel Signed-off-by: Simon Glass --- arch/x86/cpu/i386/cpu.c | 94 +++++++++++++++++++++++---------------- arch/x86/include/asm/u-boot-x86.h | 8 ++++ arch/x86/lib/bdinfo.c | 7 ++- 3 files changed, 69 insertions(+), 40 deletions(-) diff --git a/arch/x86/cpu/i386/cpu.c b/arch/x86/cpu/i386/cpu.c index d837fb97982..532f690c874 100644 --- a/arch/x86/cpu/i386/cpu.c +++ b/arch/x86/cpu/i386/cpu.c @@ -263,6 +263,49 @@ static int build_vendor_name(char *vendor_name) } #endif +int x86_cpu_vendor_info(char *name) +{ + uint cpu_device; + + cpu_device = 0; + + /* gcc 7.3 does not want to drop x86_vendors, so use #ifdef */ +#ifndef CONFIG_TPL_BUILD + *name = '\0'; /* Unset */ + + /* Find the id and vendor_name */ + if (!has_cpuid()) { + /* Its a 486 if we can modify the AC flag */ + if (flag_is_changeable_p(X86_EFLAGS_AC)) + cpu_device = 0x00000400; /* 486 */ + else + cpu_device = 0x00000300; /* 386 */ + if (cpu_device == 0x00000400 && test_cyrix_52div()) { + /* If we ever care we can enable cpuid here */ + memcpy(name, "CyrixInstead", 13); + + /* Detect NexGen with old hypercode */ + } else if (deep_magic_nexgen_probe()) { + memcpy(name, "NexGenDriven", 13); + } + } else { + int cpuid_level; + + cpuid_level = build_vendor_name(name); + name[12] = '\0'; + + /* Intel-defined flags: level 0x00000001 */ + if (cpuid_level >= 0x00000001) + cpu_device = cpuid_eax(0x00000001); + else + /* Have CPUID level 0 only unheard of */ + cpu_device = 0x00000400; + } +#endif /* CONFIG_TPL_BUILD */ + + return cpu_device; +} + static void identify_cpu(struct cpu_device_id *cpu) { cpu->device = 0; /* fix gcc 4.4.4 warning */ @@ -289,46 +332,19 @@ static void identify_cpu(struct cpu_device_id *cpu) return; } -/* gcc 7.3 does not want to drop x86_vendors, so use #ifdef */ #ifndef CONFIG_TPL_BUILD - char vendor_name[16]; - int i; - - vendor_name[0] = '\0'; /* Unset */ - - /* Find the id and vendor_name */ - if (!has_cpuid()) { - /* Its a 486 if we can modify the AC flag */ - if (flag_is_changeable_p(X86_EFLAGS_AC)) - cpu->device = 0x00000400; /* 486 */ - else - cpu->device = 0x00000300; /* 386 */ - if ((cpu->device == 0x00000400) && test_cyrix_52div()) { - memcpy(vendor_name, "CyrixInstead", 13); - /* If we ever care we can enable cpuid here */ - } - /* Detect NexGen with old hypercode */ - else if (deep_magic_nexgen_probe()) - memcpy(vendor_name, "NexGenDriven", 13); - } else { - int cpuid_level; - - cpuid_level = build_vendor_name(vendor_name); - vendor_name[12] = '\0'; - - /* Intel-defined flags: level 0x00000001 */ - if (cpuid_level >= 0x00000001) { - cpu->device = cpuid_eax(0x00000001); - } else { - /* Have CPUID level 0 only unheard of */ - cpu->device = 0x00000400; - } - } - cpu->vendor = X86_VENDOR_UNKNOWN; - for (i = 0; i < ARRAY_SIZE(x86_vendors); i++) { - if (memcmp(vendor_name, x86_vendors[i].name, 12) == 0) { - cpu->vendor = x86_vendors[i].vendor; - break; + { + char vendor_name[16]; + int i; + + cpu->device = x86_cpu_vendor_info(vendor_name); + + cpu->vendor = X86_VENDOR_UNKNOWN; + for (i = 0; i < ARRAY_SIZE(x86_vendors); i++) { + if (memcmp(vendor_name, x86_vendors[i].name, 12) == 0) { + cpu->vendor = x86_vendors[i].vendor; + break; + } } } #endif diff --git a/arch/x86/include/asm/u-boot-x86.h b/arch/x86/include/asm/u-boot-x86.h index 3acc58ad74b..5cc8f63334e 100644 --- a/arch/x86/include/asm/u-boot-x86.h +++ b/arch/x86/include/asm/u-boot-x86.h @@ -51,6 +51,14 @@ int x86_cpu_init_tpl(void); */ void cpu_reinit_fpu(void); +/** + * x86_cpu_vendor_info() - Get the CPU-vendor name and device number + * + * @name: 13-byte area to hold the returned string + * Return: CPU device number read from cpuid + */ +int x86_cpu_vendor_info(char *name); + int cpu_init_f(void); void setup_gdt(struct global_data *id, u64 *gdt_addr); /* diff --git a/arch/x86/lib/bdinfo.c b/arch/x86/lib/bdinfo.c index 165e8ab944f..2a78f578dee 100644 --- a/arch/x86/lib/bdinfo.c +++ b/arch/x86/lib/bdinfo.c @@ -19,7 +19,12 @@ void arch_print_bdinfo(void) bdinfo_print_num_l("clock_rate", gd->arch.clock_rate); bdinfo_print_num_l("tsc_base", gd->arch.tsc_base); bdinfo_print_num_l("vendor", gd->arch.x86_vendor); - bdinfo_print_str(" name", cpu_vendor_name(gd->arch.x86_vendor)); + if (!IS_ENABLED(CONFIG_X86_64)) { + char vendor_name[16]; + + x86_cpu_vendor_info(vendor_name); + bdinfo_print_str(" name", vendor_name); + } bdinfo_print_num_l("model", gd->arch.x86_model); bdinfo_print_num_l("phys_addr in bits", cpu_phys_address_size()); bdinfo_print_num_l("table start", gd->arch.table_start); -- cgit v1.3.1 From 3b2e4f542e3ea5d116d5830f4eef9be97d872312 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Tue, 27 Aug 2024 19:44:25 -0600 Subject: x86: Ensure the CPU identity exists for timer init When bootstage is used the timer can be inited before the CPU identity is set up, resulting in the checks for the vendor not working. Add a special call to work around this. Signed-off-by: Simon Glass --- arch/x86/cpu/i386/cpu.c | 5 +++++ arch/x86/cpu/x86_64/cpu.c | 6 ++++++ arch/x86/include/asm/u-boot-x86.h | 9 +++++++++ drivers/timer/tsc_timer.c | 4 ++++ 4 files changed, 24 insertions(+) diff --git a/arch/x86/cpu/i386/cpu.c b/arch/x86/cpu/i386/cpu.c index 532f690c874..a51a24498a7 100644 --- a/arch/x86/cpu/i386/cpu.c +++ b/arch/x86/cpu/i386/cpu.c @@ -501,6 +501,11 @@ int x86_cpu_reinit_f(void) return 0; } +void x86_get_identity_for_timer(void) +{ + setup_identity(); +} + void x86_enable_caches(void) { unsigned long cr0; diff --git a/arch/x86/cpu/x86_64/cpu.c b/arch/x86/cpu/x86_64/cpu.c index 80eab710315..71bc07f872a 100644 --- a/arch/x86/cpu/x86_64/cpu.c +++ b/arch/x86/cpu/x86_64/cpu.c @@ -75,3 +75,9 @@ void board_debug_uart_init(void) /* this was already done in SPL */ } #endif + +void x86_get_identity_for_timer(void) +{ + /* set the vendor to Intel so that native_calibrate_tsc() works */ + gd->arch.x86_vendor = X86_VENDOR_INTEL; +} diff --git a/arch/x86/include/asm/u-boot-x86.h b/arch/x86/include/asm/u-boot-x86.h index 5cc8f63334e..64139ead190 100644 --- a/arch/x86/include/asm/u-boot-x86.h +++ b/arch/x86/include/asm/u-boot-x86.h @@ -43,6 +43,15 @@ int x86_cpu_reinit_f(void); */ int x86_cpu_init_tpl(void); +/** + * x86_get_identity_for_timer() - Set up CPU identity for use by the early timer + * + * The timer can be needed early in board_f if bootstage is enabled. This + * function can be called from the TSC timer to make sure that the CPU-identity + * info has been set up + */ +void x86_get_identity_for_timer(void); + /** * cpu_reinit_fpu() - Reinit the FPU if something is wrong with it * diff --git a/drivers/timer/tsc_timer.c b/drivers/timer/tsc_timer.c index 80c084f380d..d11227cf440 100644 --- a/drivers/timer/tsc_timer.c +++ b/drivers/timer/tsc_timer.c @@ -403,6 +403,10 @@ static void tsc_timer_ensure_setup(bool early) if (!gd->arch.clock_rate) { unsigned long fast_calibrate; + /* deal with this being called before x86_cpu_init_f() */ + if (!gd->arch.x86_vendor) + x86_get_identity_for_timer(); + /** * There is no obvious way to obtain this information from EFI * boot services. This value was measured on a Framework Laptop -- cgit v1.3.1 From 7c0f70b65b7c6ab0c09f87932615c65142542ed5 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Tue, 27 Aug 2024 19:44:26 -0600 Subject: x86: Avoid timer-clock overflow When the clock speed is above about 4GHz, e.g. on modern PC hardware, the timer overflows, resulting in a much lower frequency than expected. Deal with this by capping the clock speed. It would be possible to move to a 64-bit value for the clock, but that is a pain to deal with. A better approach might be to express the clock in MHz but that is left for later consideration. Signed-off-by: Simon Glass --- drivers/timer/tsc_timer.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/timer/tsc_timer.c b/drivers/timer/tsc_timer.c index d11227cf440..2f2c2f27b7f 100644 --- a/drivers/timer/tsc_timer.c +++ b/drivers/timer/tsc_timer.c @@ -442,6 +442,7 @@ static void tsc_timer_ensure_setup(bool early) return; done: + fast_calibrate = min(fast_calibrate, 4000UL); if (!gd->arch.clock_rate) gd->arch.clock_rate = fast_calibrate * 1000000; } -- cgit v1.3.1 From ecf31113f17817f5ab9263937270255b6d05cdf9 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Tue, 27 Aug 2024 19:44:27 -0600 Subject: x86: Sync up tsc_timer with Linux Since we are using the code from Linux, update it to the newer version in v6.11 Signed-off-by: Simon Glass --- drivers/timer/tsc_timer.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/drivers/timer/tsc_timer.c b/drivers/timer/tsc_timer.c index 2f2c2f27b7f..dd16ab320b2 100644 --- a/drivers/timer/tsc_timer.c +++ b/drivers/timer/tsc_timer.c @@ -83,7 +83,7 @@ static unsigned long cpu_mhz_from_cpuid(void) if (cpuid_eax(0) < 0x16) return 0; - return cpuid_eax(0x16); + return cpuid_eax(0x15); } /* @@ -299,10 +299,19 @@ static unsigned long __maybe_unused quick_pit_calibrate(void) if (!pit_expect_msb(0xff-i, &delta, &d2)) break; + delta -= tsc; + + /* + * Extrapolate the error and fail fast if the error will + * never be below 500 ppm. + */ + if (i == 1 && + d1 + d2 >= (delta * MAX_QUICK_PIT_ITERATIONS) >> 11) + return 0; + /* * Iterate until the error is less than 500 ppm */ - delta -= tsc; if (d1+d2 >= delta >> 11) continue; -- cgit v1.3.1 From 557767f80294054932c7453be0e268ad39643fdc Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Tue, 27 Aug 2024 19:44:28 -0600 Subject: x86: Add a cpuid command It is useful to obtain the results of cpuid queries, so add a command for this. Signed-off-by: Simon Glass --- cmd/x86/Makefile | 2 +- cmd/x86/cpuid.c | 37 +++++++++++++++++++++++++++ doc/usage/cmd/cpuid.rst | 68 +++++++++++++++++++++++++++++++++++++++++++++++++ doc/usage/index.rst | 1 + test/cmd/Makefile | 1 + test/cmd/cpuid.c | 22 ++++++++++++++++ 6 files changed, 130 insertions(+), 1 deletion(-) create mode 100644 cmd/x86/cpuid.c create mode 100644 doc/usage/cmd/cpuid.rst create mode 100644 test/cmd/cpuid.c diff --git a/cmd/x86/Makefile b/cmd/x86/Makefile index b1f39d3bfde..1648907ac2d 100644 --- a/cmd/x86/Makefile +++ b/cmd/x86/Makefile @@ -1,7 +1,7 @@ # SPDX-License-Identifier: GPL-2.0+ obj-$(CONFIG_CMD_CBSYSINFO) += cbsysinfo.o -obj-y += mtrr.o +obj-y += cpuid.o mtrr.o obj-$(CONFIG_CMD_EXCEPTION) += exception.o obj-$(CONFIG_USE_HOB) += hob.o obj-$(CONFIG_HAVE_FSP) += fsp.o diff --git a/cmd/x86/cpuid.c b/cmd/x86/cpuid.c new file mode 100644 index 00000000000..222754b5d15 --- /dev/null +++ b/cmd/x86/cpuid.c @@ -0,0 +1,37 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * The 'cpuid' command provides access to the CPU's cpuid information + * + * Copyright 2024 Google, LLC + * Written by Simon Glass + */ + +#include +#include +#include + +static int do_cpuid(struct cmd_tbl *cmdtp, int flag, int argc, + char *const argv[]) +{ + struct cpuid_result res; + ulong op; + + if (argc < 2) + return CMD_RET_USAGE; + + op = hextoul(argv[1], NULL); + res = cpuid(op); + printf("eax %08x\n", res.eax); + printf("ebx %08x\n", res.ebx); + printf("ecx %08x\n", res.ecx); + printf("edx %08x\n", res.edx); + + return 0; +} + +U_BOOT_LONGHELP(cpuid, "Show CPU Identification information"); + +U_BOOT_CMD( + cpuid, 2, 1, do_cpuid, + "cpuid ", cpuid_help_text +); diff --git a/doc/usage/cmd/cpuid.rst b/doc/usage/cmd/cpuid.rst new file mode 100644 index 00000000000..cccf9262ed4 --- /dev/null +++ b/doc/usage/cmd/cpuid.rst @@ -0,0 +1,68 @@ +.. SPDX-License-Identifier: GPL-2.0+ + +.. index:: + single: cpuid (command) + +cpuid command +============= + +Synopsis +-------- + +:: + + cpuid + +Description +----------- + +The cpuid command requests CPU-identification information on x86 CPUs. The +operation selects what information is returned. Up to four 32-bit registers +can be update (eax-edx) depending on the operation. + +Configuration +------------- + +The cpuid command is only available on x86. + +Return value +------------ + +The return value $? is 0 (true). + +Example +------- + +:: + + => cpuid 1 + eax 00060fb1 + ebx 00040800 + ecx 80002001 + edx 178bfbfd + +This shows checking for 64-bit 'long' mode:: + + => cpuid 80000000 + eax 8000000a + ebx 68747541 + ecx 444d4163 + edx 69746e65 + => cpuid 80000001 + eax 00060fb1 + ebx 00000000 + ecx 00000007 + edx 2193fbfd # Bit 29 is set in edx, so long mode is available + +On a 32-bit-only CPU:: + + => cpuid 80000000 + eax 80000004 + ebx 756e6547 + ecx 6c65746e + edx 49656e69 + => cpuid 80000001 + eax 00000663 + ebx 00000000 + ecx 00000000 + edx 00000000 # Bit 29 is not set in edx, so long mode is not available diff --git a/doc/usage/index.rst b/doc/usage/index.rst index fcce125a611..d2b187b0118 100644 --- a/doc/usage/index.rst +++ b/doc/usage/index.rst @@ -52,6 +52,7 @@ Shell commands cmd/conitrace cmd/cp cmd/cpu + cmd/cpuid cmd/cyclic cmd/dm cmd/ebtupdate diff --git a/test/cmd/Makefile b/test/cmd/Makefile index dbee9b26405..302c748389b 100644 --- a/test/cmd/Makefile +++ b/test/cmd/Makefile @@ -12,6 +12,7 @@ ifdef CONFIG_CONSOLE_RECORD obj-$(CONFIG_CMD_PAUSE) += test_pause.o endif obj-y += exit.o mem.o +obj-$(CONFIG_X86) += cpuid.o obj-$(CONFIG_CMD_ADDRMAP) += addrmap.o obj-$(CONFIG_CMD_BDI) += bdinfo.o obj-$(CONFIG_CMD_FDT) += fdt.o diff --git a/test/cmd/cpuid.c b/test/cmd/cpuid.c new file mode 100644 index 00000000000..e07f5fd4696 --- /dev/null +++ b/test/cmd/cpuid.c @@ -0,0 +1,22 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Tests for cpuid command + * + * Copyright 2024 Google LLC + * Written by Simon Glass + */ + +#include +#include + +static int cmd_test_cpuid(struct unit_test_state *uts) +{ + ut_assertok(run_commandf("cpuid 1")); + ut_assert_nextline("eax 00060fb1"); + ut_assert_nextline("ebx 00000800"); + ut_assert_nextline("ecx 80002001"); + ut_assert_nextline("edx 078bfbfd"); + + return 0; +} +CMD_TEST(cmd_test_cpuid, UTF_CONSOLE); -- cgit v1.3.1 From c4e582654a94fc085196464ebe409ce7f89739f6 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Tue, 27 Aug 2024 19:44:29 -0600 Subject: x86: Add msr command It is useful to obtain the results of MSR queries as well as to update MSR registers, so add a command these tasks. Signed-off-by: Simon Glass --- cmd/x86/Makefile | 2 +- cmd/x86/msr.c | 52 +++++++++++++++++++++++++++++++++++++++++++ doc/usage/cmd/msr.rst | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++ doc/usage/index.rst | 1 + test/cmd/Makefile | 2 +- test/cmd/msr.c | 38 ++++++++++++++++++++++++++++++++ 6 files changed, 154 insertions(+), 2 deletions(-) create mode 100644 cmd/x86/msr.c create mode 100644 doc/usage/cmd/msr.rst create mode 100644 test/cmd/msr.c diff --git a/cmd/x86/Makefile b/cmd/x86/Makefile index 1648907ac2d..925215235d3 100644 --- a/cmd/x86/Makefile +++ b/cmd/x86/Makefile @@ -1,7 +1,7 @@ # SPDX-License-Identifier: GPL-2.0+ obj-$(CONFIG_CMD_CBSYSINFO) += cbsysinfo.o -obj-y += cpuid.o mtrr.o +obj-y += cpuid.o msr.o mtrr.o obj-$(CONFIG_CMD_EXCEPTION) += exception.o obj-$(CONFIG_USE_HOB) += hob.o obj-$(CONFIG_HAVE_FSP) += fsp.o diff --git a/cmd/x86/msr.c b/cmd/x86/msr.c new file mode 100644 index 00000000000..3cb70d1f89a --- /dev/null +++ b/cmd/x86/msr.c @@ -0,0 +1,52 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * The 'cpuid' command provides access to the CPU's cpuid information + * + * Copyright 2024 Google, LLC + * Written by Simon Glass + */ + +#include +#include +#include + +static int do_read(struct cmd_tbl *cmdtp, int flag, int argc, + char *const argv[]) +{ + struct msr_t msr; + ulong op; + + if (argc < 2) + return CMD_RET_USAGE; + + op = hextoul(argv[1], NULL); + msr = msr_read(op); + printf("%08x %08x\n", msr.hi, msr.lo); + + return 0; +} + +static int do_write(struct cmd_tbl *cmdtp, int flag, int argc, + char *const argv[]) +{ + struct msr_t msr; + ulong op; + + if (argc < 4) + return CMD_RET_USAGE; + + op = hextoul(argv[1], NULL); + msr.hi = hextoul(argv[2], NULL); + msr.lo = hextoul(argv[3], NULL); + msr_write(op, msr); + + return 0; +} + +U_BOOT_LONGHELP(msr, + "read - read a machine-status register (MSR) as \n" + "write - write an MSR"); + +U_BOOT_CMD_WITH_SUBCMDS(msr, "Machine Status Registers", msr_help_text, + U_BOOT_CMD_MKENT(read, CONFIG_SYS_MAXARGS, 1, do_read, "", ""), + U_BOOT_CMD_MKENT(write, CONFIG_SYS_MAXARGS, 1, do_write, "", "")); diff --git a/doc/usage/cmd/msr.rst b/doc/usage/cmd/msr.rst new file mode 100644 index 00000000000..04ee52cc1c7 --- /dev/null +++ b/doc/usage/cmd/msr.rst @@ -0,0 +1,61 @@ +.. SPDX-License-Identifier: GPL-2.0+ + +.. index:: + single: msr (command) + +msr command +=========== + +Synopsis +-------- + +:: + + msr read + msr write + +Description +----------- + +The msr command reads and writes machine-status registers (MSRs) on x86 CPUs. +The information is a 64-bit value split into two parts, for the top 32 +bits and for the bottom 32 bits. + +The operation selects what information is read or written. + +msr read +~~~~~~~~ + +This reads an MSR and displays the value obtained. + +msr write +~~~~~~~~~ + +This writes a value to an MSR. + +Configuration +------------- + +The msr command is only available on x86. + +Return value +------------ + +The return value $? is 0 (true). + +Example +------- + +This shows reading msr 0x194 which is MSR_FLEX_RATIO on Intel CPUs:: + + => msr read 194 + 00000000 00011200 # Bits 16 (flex ratio enable) and 20 (lock) are set + +This shows adjusting the energy-performance bias on an Intel CPU:: + + => msr read 1b0 + 00000000 00000006 # 6 means 'normal' + + => msr write 1b0 0 f # change to power-save + => msr read 1b0 + 00000000 0000000f diff --git a/doc/usage/index.rst b/doc/usage/index.rst index d2b187b0118..b84d8ee909f 100644 --- a/doc/usage/index.rst +++ b/doc/usage/index.rst @@ -87,6 +87,7 @@ Shell commands cmd/mbr cmd/md cmd/mmc + cmd/msr cmd/mtest cmd/mtrr cmd/panic diff --git a/test/cmd/Makefile b/test/cmd/Makefile index 302c748389b..40808350962 100644 --- a/test/cmd/Makefile +++ b/test/cmd/Makefile @@ -12,7 +12,7 @@ ifdef CONFIG_CONSOLE_RECORD obj-$(CONFIG_CMD_PAUSE) += test_pause.o endif obj-y += exit.o mem.o -obj-$(CONFIG_X86) += cpuid.o +obj-$(CONFIG_X86) += cpuid.o msr.o obj-$(CONFIG_CMD_ADDRMAP) += addrmap.o obj-$(CONFIG_CMD_BDI) += bdinfo.o obj-$(CONFIG_CMD_FDT) += fdt.o diff --git a/test/cmd/msr.c b/test/cmd/msr.c new file mode 100644 index 00000000000..e9a152ee5bf --- /dev/null +++ b/test/cmd/msr.c @@ -0,0 +1,38 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Tests for msr command + * + * Copyright 2024 Google LLC + * Written by Simon Glass + */ + +#include +#include + +static int cmd_test_msr(struct unit_test_state *uts) +{ + ut_assertok(run_commandf("msr read 200")); + ut_assert_nextline("00000000 ffe00006"); + ut_assert_console_end(); + + /* change the first variable msr and see it reflected in the mtrr cmd */ + ut_assertok(run_commandf("mtrr")); + ut_assert_nextline("CPU 65537:"); + ut_assert_nextlinen("Reg"); + ut_assert_nextlinen("0 Y Back 00000000ffe00000"); + ut_assertok(console_record_reset_enable()); + + /* change the type from 6 to 5 */ + ut_assertok(run_commandf("msr write 200 0 ffe00005")); + ut_assert_console_end(); + + /* Now it shows 'Protect' */ + ut_assertok(run_commandf("mtrr")); + ut_assert_nextline("CPU 65537:"); + ut_assert_nextlinen("Reg"); + ut_assert_nextlinen("0 Y Protect 00000000ffe00000"); + ut_assertok(console_record_reset_enable()); + + return 0; +} +CMD_TEST(cmd_test_msr, UTF_CONSOLE); -- cgit v1.3.1 From 734b0cbbca62a70d5a7c40d17fc45eb6cf1298ea Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Wed, 28 Aug 2024 14:13:53 -0600 Subject: x86: coreboot: Update the timestamp code to use sysinfo Rather than using a special variable, get the timestamp info from the coreboot sysinfo struct. Return a proper error as well. Signed-off-by: Simon Glass --- arch/x86/cpu/coreboot/timestamp.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/arch/x86/cpu/coreboot/timestamp.c b/arch/x86/cpu/coreboot/timestamp.c index ec4003c4e77..681191d85bb 100644 --- a/arch/x86/cpu/coreboot/timestamp.c +++ b/arch/x86/cpu/coreboot/timestamp.c @@ -6,13 +6,12 @@ */ #include +#include #include #include #include #include -static struct timestamp_table *ts_table __section(".data"); - void timestamp_init(void) { timestamp_add_now(TS_U_BOOT_INITTED); @@ -20,6 +19,8 @@ void timestamp_init(void) void timestamp_add(enum timestamp_id id, uint64_t ts_time) { + const struct sysinfo_t *info = cb_get_sysinfo(); + struct timestamp_table *ts_table = info->tstamp_table; struct timestamp_entry *tse; if (!ts_table || (ts_table->num_entries == ts_table->max_entries)) @@ -37,13 +38,15 @@ void timestamp_add_now(enum timestamp_id id) int timestamp_add_to_bootstage(void) { + const struct sysinfo_t *info = cb_get_sysinfo(); + const struct timestamp_table *ts_table = info->tstamp_table; uint i; if (!ts_table) - return -1; + return -ENOENT; for (i = 0; i < ts_table->num_entries; i++) { - struct timestamp_entry *tse = &ts_table->entries[i]; + const struct timestamp_entry *tse = &ts_table->entries[i]; const char *name = NULL; switch (tse->entry_id) { -- cgit v1.3.1 From e1c0dd369d759e1dc23e2cd7ab9fca2095c9b714 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Wed, 28 Aug 2024 14:13:54 -0600 Subject: x86: coreboot: Add timestamps from coreboot to bootstage Receiving timestamps from coreboot was unceremoniously dropped some time ago. Add it back. Signed-off-by: Simon Glass Fixes: 8ad01ce36f7 ("x86: Remove x86 specific GD flags as they are...") --- arch/x86/cpu/coreboot/coreboot.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/arch/x86/cpu/coreboot/coreboot.c b/arch/x86/cpu/coreboot/coreboot.c index ef1a3f42d2e..b15715c935d 100644 --- a/arch/x86/cpu/coreboot/coreboot.c +++ b/arch/x86/cpu/coreboot/coreboot.c @@ -77,6 +77,8 @@ static void board_final_init(void) static int last_stage_init(void) { + timestamp_add_to_bootstage(); + if (IS_ENABLED(CONFIG_XPL_BUILD)) return 0; -- cgit v1.3.1 From a768742d65c7d96ab0a8737fed7c1360f84706dc Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Wed, 28 Aug 2024 14:13:55 -0600 Subject: x86: coreboot: Enable receiving timestamps from coreboot Bring this information into bootstage. Signed-off-by: Simon Glass --- configs/coreboot_defconfig | 2 ++ 1 file changed, 2 insertions(+) diff --git a/configs/coreboot_defconfig b/configs/coreboot_defconfig index e68a3aed659..881bf8dd180 100644 --- a/configs/coreboot_defconfig +++ b/configs/coreboot_defconfig @@ -11,6 +11,7 @@ CONFIG_SYS_MONITOR_BASE=0x01110000 CONFIG_FIT=y CONFIG_FIT_SIGNATURE=y CONFIG_BOOTSTD_FULL=y +CONFIG_BOOTSTAGE=y CONFIG_SHOW_BOOT_PROGRESS=y CONFIG_USE_BOOTARGS=y CONFIG_BOOTARGS="root=/dev/sdb3 init=/sbin/init rootwait ro" @@ -30,6 +31,7 @@ CONFIG_CMD_USB=y CONFIG_BOOTP_BOOTFILESIZE=y CONFIG_CMD_TIME=y CONFIG_CMD_SOUND=y +CONFIG_CMD_BOOTSTAGE=y CONFIG_CMD_EXT4_WRITE=y CONFIG_MAC_PARTITION=y CONFIG_ENV_OVERWRITE=y -- cgit v1.3.1 From 8c3e44d307e34ea8efe524e3b5c0bac79f22f280 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 1 Sep 2024 16:27:27 -0600 Subject: blk: Make functions available unconditionally Some boards still don't enable BLK but we want to be able to at least compile the code which relies on this. For example, bootstd includes calls to blk_...() functions, albeit with a check for BLK so that the code is eliminated by the compiler. Reduce the scope of the BLK #ifdef to help with this. Signed-off-by: Simon Glass --- include/blk.h | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/include/blk.h b/include/blk.h index 1fc9a5b8471..eef6c8629e8 100644 --- a/include/blk.h +++ b/include/blk.h @@ -197,7 +197,6 @@ static inline void blkcache_free(void) {} #endif -#if CONFIG_IS_ENABLED(BLK) struct udevice; /* Operations on block devices */ @@ -278,6 +277,8 @@ struct blk_ops { #endif /* CONFIG_BOUNCE_BUFFER */ }; +#if CONFIG_IS_ENABLED(BLK) + /* * These functions should take struct udevice instead of struct blk_desc, * but this is convenient for migration to driver model. Add a 'd' prefix @@ -291,6 +292,8 @@ unsigned long blk_dwrite(struct blk_desc *block_dev, lbaint_t start, unsigned long blk_derase(struct blk_desc *block_dev, lbaint_t start, lbaint_t blkcnt); +#endif /* BLK */ + /** * blk_read() - Read from a block device * @@ -528,8 +531,10 @@ struct blk_desc *blk_get_by_device(struct udevice *dev); */ int blk_get_desc(enum uclass_id uclass_id, int devnum, struct blk_desc **descp); -#else +#if !CONFIG_IS_ENABLED(BLK) + #include + /* * These functions should take struct udevice instead of struct blk_desc, * but this is convenient for migration to driver model. Add a 'd' prefix -- cgit v1.3.1 From aa0b3e6ef405c01063298cf3c990ae490a673e5e Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sun, 1 Sep 2024 16:27:28 -0600 Subject: bootstd: Avoid calling unavailable block functions When BLK is not enabled but BOOTSTD is, some features of standard boot become unavailable. Add a check for this in the only site that is currently apparent. Signed-off-by: Simon Glass --- boot/bootdev-uclass.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/boot/bootdev-uclass.c b/boot/bootdev-uclass.c index 807f8dfb064..64ec4fde493 100644 --- a/boot/bootdev-uclass.c +++ b/boot/bootdev-uclass.c @@ -434,6 +434,9 @@ int bootdev_find_by_label(const char *label, struct udevice **devp, struct uclass *uc; enum uclass_id id; + if (!CONFIG_IS_ENABLED(BLK)) + return -ENOSYS; + ret = label_to_uclass(label, &seq, &method_flags); if (ret < 0) return log_msg_ret("uc", ret); -- cgit v1.3.1 From 7c4e5223cbce11b21848db75092c7c172649c7d4 Mon Sep 17 00:00:00 2001 From: Alexander Kochetkov Date: Mon, 16 Sep 2024 11:24:44 +0300 Subject: binman: fix passing loadables to mkimage on first run FIT use mkimage from BuildSectionData() to build FIT entry contents. BuildSectionData() get called several times during building FIT image. Currently when fit inserts loadables, it use self._loadables property that contain loadables computed during previuos BuildSectionData() invocation. So for the first run it use empty list and pass no loadables to mkimage. That makes problem for adding signature to FIT image because mkimage fails to add signature and aborts building FIT if no loadables provided. The patch fixes described behaviour in a way that BuildSectionData() uses recently calculated loadables value, not previosly calculated. Signed-off-by: Alexander Kochetkov Reviewed-by: Simon Glass --- tools/binman/etype/fit.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py index 0abe1c78c43..732510e2a05 100644 --- a/tools/binman/etype/fit.py +++ b/tools/binman/etype/fit.py @@ -637,8 +637,8 @@ class Entry_fit(Entry_section): """ val = fdt_util.GetStringList(node, 'fit,firmware') if val is None: - return None, self._loadables - valid_entries = list(self._loadables) + return None, loadables + valid_entries = list(loadables) for name, entry in self.GetEntries().items(): missing = [] entry.CheckMissing(missing) @@ -653,7 +653,7 @@ class Entry_fit(Entry_section): firmware = name elif name not in result: result.append(name) - for name in self._loadables: + for name in loadables: if name != firmware and name not in result: result.append(name) return firmware, result -- cgit v1.3.1 From 9e81f13dbb41843abb25e6c4b20961f092630f13 Mon Sep 17 00:00:00 2001 From: Alexander Kochetkov Date: Mon, 16 Sep 2024 11:24:45 +0300 Subject: image-host: fix 'unknown error' error message Fix error message like this: Can't add verification data for node 'fdt-1' () We get unknown error because we decode error as fdt error but actually it is system error. Signed-off-by: Alexander Kochetkov Reviewed-by: Simon Glass --- tools/image-host.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/image-host.c b/tools/image-host.c index 49ce7436bb9..5e01b853c50 100644 --- a/tools/image-host.c +++ b/tools/image-host.c @@ -1333,7 +1333,7 @@ int fit_add_verification_data(const char *keydir, const char *keyfile, if (ret) { fprintf(stderr, "Can't add verification data for node '%s' (%s)\n", fdt_get_name(fit, noffset, NULL), - fdt_strerror(ret)); + strerror(-ret)); return ret; } } -- cgit v1.3.1 From 133c000ca334743fda3a4d77f7638da1a807e40e Mon Sep 17 00:00:00 2001 From: Alexander Kochetkov Date: Mon, 16 Sep 2024 11:24:46 +0300 Subject: binman: implement signing FIT images during image build The patch implement new property 'fit,sign' that can be declared at the top-level 'fit' node. If that option is declared, fit tryies to detect private keys directory among binman include directories. That directory than passed to mkimage using '-k' flag and that enable signing of FIT. Signed-off-by: Alexander Kochetkov Reviewed-by: Simon Glass Renumbered files, moved new tests to end: Signed-off-by: Simon Glass --- tools/binman/btool/mkimage.py | 5 +- tools/binman/entries.rst | 7 +++ tools/binman/etype/fit.py | 51 +++++++++++++++++ tools/binman/ftest.py | 95 ++++++++++++++++++++++++++++++++ tools/binman/test/340_fit_signature.dts | 98 +++++++++++++++++++++++++++++++++ tools/binman/test/340_rsa2048.key | 28 ++++++++++ tools/binman/test/341_fit_signature.dts | 98 +++++++++++++++++++++++++++++++++ tools/binman/test/342_fit_signature.dts | 61 ++++++++++++++++++++ 8 files changed, 442 insertions(+), 1 deletion(-) create mode 100644 tools/binman/test/340_fit_signature.dts create mode 100644 tools/binman/test/340_rsa2048.key create mode 100644 tools/binman/test/341_fit_signature.dts create mode 100644 tools/binman/test/342_fit_signature.dts diff --git a/tools/binman/btool/mkimage.py b/tools/binman/btool/mkimage.py index 39a4c8c1432..78d3301bc10 100644 --- a/tools/binman/btool/mkimage.py +++ b/tools/binman/btool/mkimage.py @@ -22,7 +22,7 @@ class Bintoolmkimage(bintool.Bintool): # pylint: disable=R0913 def run(self, reset_timestamp=False, output_fname=None, external=False, - pad=None, align=None): + pad=None, align=None, priv_keys_dir=None): """Run mkimage Args: @@ -34,6 +34,7 @@ class Bintoolmkimage(bintool.Bintool): other things to be easily added later, if required, such as signatures align: Bytes to use for alignment of the FIT and its external data + priv_keys_dir: Path to directory containing private keys version: True to get the mkimage version """ args = [] @@ -45,6 +46,8 @@ class Bintoolmkimage(bintool.Bintool): args += ['-B', f'{align:x}'] if reset_timestamp: args.append('-t') + if priv_keys_dir: + args += ['-k', f'{priv_keys_dir}'] if output_fname: args += ['-F', output_fname] return self.run_cmd(*args) diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst index 3006c5914d6..e918162fb48 100644 --- a/tools/binman/entries.rst +++ b/tools/binman/entries.rst @@ -864,6 +864,13 @@ The top-level 'fit' node supports the following special properties: fit,fdt-list-dir = "arch/arm/dts + fit,sign + Enable signing FIT images via mkimage as described in + verified-boot.rst. If the property is found, the private keys path is + detected among binman include directories and passed to mkimage via + -k flag. All the keys required for signing FIT must be available at + time of signing and must be located in single include directory. + Substitutions ~~~~~~~~~~~~~ diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py index 732510e2a05..b5afbda41b5 100644 --- a/tools/binman/etype/fit.py +++ b/tools/binman/etype/fit.py @@ -9,6 +9,7 @@ import glob import os import libfdt +import os from binman.entry import Entry, EntryArg from binman.etype.section import Entry_section @@ -101,6 +102,14 @@ class Entry_fit(Entry_section): In this case the input directories are ignored and all devicetree files must be in that directory. + fit,sign + Enable signing FIT images via mkimage as described in + verified-boot.rst. If the property is found, the private keys path + is detected among binman include directories and passed to mkimage + via -k flag. All the keys required for signing FIT must be + available at time of signing and must be located in single include + directory. + Substitutions ~~~~~~~~~~~~~ @@ -426,6 +435,7 @@ class Entry_fit(Entry_section): self._remove_props = props.split() self.mkimage = None self.fdtgrep = None + self._fit_sign = None def ReadNode(self): super().ReadNode() @@ -508,6 +518,45 @@ class Entry_fit(Entry_section): # are removed from self._entries later. self._priv_entries = dict(self._entries) + def _get_priv_keys_dir(self, data): + """Detect private keys path among binman include directories + + Args: + data: FIT image in binary format + + Returns: + str: Single path containing all private keys found or None + + Raises: + ValueError: Filename 'rsa2048.key' not found in input path + ValueError: Multiple key paths found + """ + def _find_keys_dir(node): + for subnode in node.subnodes: + if subnode.name.startswith('signature'): + if subnode.props.get('key-name-hint') is None: + continue + hint = subnode.props['key-name-hint'].value + name = tools.get_input_filename(f"{hint}.key") + path = os.path.dirname(name) + if path not in paths: + paths.append(path) + else: + _find_keys_dir(subnode) + return None + + fdt = Fdt.FromData(data) + fdt.Scan() + + paths = [] + + _find_keys_dir(fdt.GetRoot()) + + if len(paths) > 1: + self.Raise("multiple key paths found (%s)" % ",".join(paths)) + + return paths[0] if len(paths) else None + def BuildSectionData(self, required): """Build FIT entry contents @@ -538,6 +587,8 @@ class Entry_fit(Entry_section): align = self._fit_props.get('fit,align') if align is not None: args.update({'align': fdt_util.fdt32_to_cpu(align.value)}) + if self._fit_props.get('fit,sign') is not None: + args.update({'priv_keys_dir': self._get_priv_keys_dir(data)}) if self.mkimage.run(reset_timestamp=True, output_fname=output_fname, **args) is None: if not self.GetAllowMissing(): diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index e3f231e4bcc..156567ace77 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -7804,6 +7804,101 @@ fdt fdtmap Extract the devicetree blob from the fdtmap """Test that binman can produce an iMX8 image""" self._DoTestFile('339_nxp_imx8.dts') + def testFitSignSimple(self): + """Test that image with FIT and signature nodes can be signed""" + if not elf.ELF_TOOLS: + self.skipTest('Python elftools not available') + entry_args = { + 'of-list': 'test-fdt1', + 'default-dt': 'test-fdt1', + 'atf-bl31-path': 'bl31.elf', + } + data = tools.read_file(self.TestFile("340_rsa2048.key")) + self._MakeInputFile("keys/rsa2048.key", data) + + test_subdir = os.path.join(self._indir, TEST_FDT_SUBDIR) + keys_subdir = os.path.join(self._indir, "keys") + data = self._DoReadFileDtb( + '340_fit_signature.dts', + entry_args=entry_args, + extra_indirs=[test_subdir, keys_subdir])[0] + + dtb = fdt.Fdt.FromData(data) + dtb.Scan() + + conf = dtb.GetNode('/configurations/conf-uboot-1') + self.assertIsNotNone(conf) + signature = conf.FindNode('signature') + self.assertIsNotNone(signature) + self.assertIsNotNone(signature.props.get('value')) + + images = dtb.GetNode('/images') + self.assertIsNotNone(images) + for subnode in images.subnodes: + signature = subnode.FindNode('signature') + self.assertIsNotNone(signature) + self.assertIsNotNone(signature.props.get('value')) + + def testFitSignKeyNotFound(self): + """Test that missing keys raise an error""" + if not elf.ELF_TOOLS: + self.skipTest('Python elftools not available') + entry_args = { + 'of-list': 'test-fdt1', + 'default-dt': 'test-fdt1', + 'atf-bl31-path': 'bl31.elf', + } + test_subdir = os.path.join(self._indir, TEST_FDT_SUBDIR) + with self.assertRaises(ValueError) as e: + self._DoReadFileDtb( + '340_fit_signature.dts', + entry_args=entry_args, + extra_indirs=[test_subdir])[0] + self.assertIn( + 'Filename \'rsa2048.key\' not found in input path', + str(e.exception)) + + def testFitSignMultipleKeyPaths(self): + """Test that keys found in multiple paths raise an error""" + if not elf.ELF_TOOLS: + self.skipTest('Python elftools not available') + entry_args = { + 'of-list': 'test-fdt1', + 'default-dt': 'test-fdt1', + 'atf-bl31-path': 'bl31.elf', + } + data = tools.read_file(self.TestFile("340_rsa2048.key")) + self._MakeInputFile("keys1/rsa2048.key", data) + data = tools.read_file(self.TestFile("340_rsa2048.key")) + self._MakeInputFile("keys2/conf-rsa2048.key", data) + + test_subdir = os.path.join(self._indir, TEST_FDT_SUBDIR) + keys_subdir1 = os.path.join(self._indir, "keys1") + keys_subdir2 = os.path.join(self._indir, "keys2") + with self.assertRaises(ValueError) as e: + self._DoReadFileDtb( + '341_fit_signature.dts', + entry_args=entry_args, + extra_indirs=[test_subdir, keys_subdir1, keys_subdir2])[0] + self.assertIn( + 'Node \'/binman/fit\': multiple key paths found', + str(e.exception)) + + def testFitSignNoSingatureNodes(self): + """Test that fit,sign doens't raise error if no signature nodes found""" + if not elf.ELF_TOOLS: + self.skipTest('Python elftools not available') + entry_args = { + 'of-list': 'test-fdt1', + 'default-dt': 'test-fdt1', + 'atf-bl31-path': 'bl31.elf', + } + test_subdir = os.path.join(self._indir, TEST_FDT_SUBDIR) + self._DoReadFileDtb( + '342_fit_signature.dts', + entry_args=entry_args, + extra_indirs=[test_subdir])[0] + if __name__ == "__main__": unittest.main() diff --git a/tools/binman/test/340_fit_signature.dts b/tools/binman/test/340_fit_signature.dts new file mode 100644 index 00000000000..9dce62e52de --- /dev/null +++ b/tools/binman/test/340_fit_signature.dts @@ -0,0 +1,98 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + fit { + description = "test desc"; + #address-cells = <1>; + fit,fdt-list = "of-list"; + fit,sign; + + images { + u-boot { + description = "test u-boot"; + type = "standalone"; + arch = "arm64"; + os = "u-boot"; + compression = "none"; + load = <0x00000000>; + entry = <0x00000000>; + + u-boot-nodtb { + }; + + hash { + algo = "sha256"; + }; + + signature { + algo = "sha256,rsa2048"; + key-name-hint = "rsa2048"; + }; + }; + @atf-SEQ { + fit,operation = "split-elf"; + description = "test tf-a"; + type = "firmware"; + arch = "arm64"; + os = "arm-trusted-firmware"; + compression = "none"; + fit,load; + fit,entry; + fit,data; + + atf-bl31 { + }; + + hash { + algo = "sha256"; + }; + + signature { + algo = "sha256,rsa2048"; + key-name-hint = "rsa2048"; + }; + }; + @fdt-SEQ { + description = "test fdt"; + type = "flat_dt"; + compression = "none"; + + hash { + algo = "sha256"; + }; + + signature { + algo = "sha256,rsa2048"; + key-name-hint = "rsa2048"; + }; + }; + }; + + configurations { + default = "@conf-uboot-DEFAULT-SEQ"; + @conf-uboot-SEQ { + description = "uboot config"; + fdt = "fdt-SEQ"; + fit,firmware = "u-boot"; + fit,loadables; + + hash { + algo = "sha256"; + }; + + signature { + algo = "sha256,rsa2048"; + key-name-hint = "rsa2048"; + sign-images = "firmware", "loadables", "fdt"; + }; + }; + }; + }; + }; +}; diff --git a/tools/binman/test/340_rsa2048.key b/tools/binman/test/340_rsa2048.key new file mode 100644 index 00000000000..e74b20cf392 --- /dev/null +++ b/tools/binman/test/340_rsa2048.key @@ -0,0 +1,28 @@ +-----BEGIN PRIVATE KEY----- +MIIEvwIBADANBgkqhkiG9w0BAQEFAASCBKkwggSlAgEAAoIBAQDVUiT2JAF8Ajcx +3XTB5qdGxuPMVFcXKJH+4L66oSt4YUBGi1bClo80U2azu08BTzk2Jzv6hez/mvzL +hBvL3WnPwMl5vdOxb1kvUQyKLSw2bkM8VB0X1jGsKsKjzArg/aI8RknfiaSc5jua +2lqwUFwv2RMF8jvIMN/1GnTLdECeMFVgVFSFkzIocISAHGPoGUOxTf8xK7o0x4RX +NzB+95RtIqTQ5Az/KPVCOcQR5ETrUBXHF1I0rYjJjHHO4dUxxfDqFabt60EzQ/R2 +oZu58C4y0TrRI98g4hVPBYapildWjaNQm1Exa4ZaSDVl01OXsFW9Dm80PqfW4tTH +Cm4nuCq5AgMBAAECggEBAIoG5b2SHJfFwzrzpQmVmeTU6i6a3+MvMBAwEZkmkb8J +hhJfNFsiGjTsRgbDiuI5BbbBejCmmWvmN+3jZCzr7fwsLPEl36TufFF+atO5WOM7 +Qyv07QIwaOGSpXBgpSVhV6kSfdgy8p1G54hSAt4UkSGwnnt5ei8VWMP6Q1oltW3k +f9DQ/ar4UEVa4jlJU3xqchcUTiKBKSH6pMC/Fqlq8x5JTLmk1Yb6C2UNcgJYez1u +sHkdCA0FG3rFPrpFoQ1LUjMj1uEYNAxM3jOxE7Uvmk4yo9WpQDY7cRb2+Th9YY8a +IKQ2s81Yg2TmkGzr8f5nrZz3WbAmQhQgsKbwlo6snjUCgYEA7kBOt0JlU7bJTfOr +9s51g2VUfIH9lDS2Eh8MY+Bt6Y0Kdw/UK4HR8ZlN/nn0bHuHkc12K8lXEsQpgIEW +DaqHytZJHqFs2egzKu/IvQYZ2WXEMj47LZQxEDHO9gtjE+5qCW9yJGqxW9BJKPVD +F4spus4NqC+yD5OHM+6ESUtL/wMCgYEA5TZj6OHmECeh3efrwHqjDcjrqQbOTozU +KPCNCY3Pv4Cg4xas/L93TE2CY6HJTr6mwEMUM+M4Ujjj15VCmSDQ/YYcGau1jo+f +XdphOEENrPwoe9ATWIyBpT/wDrEz3L6JbE9dWMYY8vKYESt3qhVqDlbpmnYl8Jm+ +O3r5Cy2NlJMCgYEAyqzsCZuy5QcesnByvm8dqpxdxdkzJYu9wyakfKZj+gUgfO57 +OFOkjFk07yFB27MuPctCFredmfpDr+ygHRoPkG7AHw2Fss2EEaeP5bU18ilPQMqN +vxVMs5EblVVUgJUVoVcsC2yz2f4S7oPOAk5BPoehOIzydauznWrvIAas7I8CgYBr +CFHxLoNq6cbZQ3JACERZrIf2/vmZjoOHtoR1gKYRK7R1NmKDB7lihRMtCSBix/4/ +61Lkw+bJ5kzmn4lgzgUpTdWTWy5FquVlQxOA3EfRjlItNsXB5KKpksi7Y53vJ34u +eIUDbkW6NPQzmFOhtaw3k/gzq5Yd2v0M82iWAqiJRwKBgQCl2+e2cjISK31QhKTC +puhwQ0/YuC3zlwMXQgB3nPw8b9RlaDTMrRBCIUFIrrX11tHswGWpyVsxW2AvZ3Zm +jsWpwGkUdpRdXJBhSaisV/PA+x3kYhpibzEI8FrzhU69zNROCb8CTkN4WcdBdq6J +PUh/jRtKoE79qrlnIlNvFoz2gQ== +-----END PRIVATE KEY----- diff --git a/tools/binman/test/341_fit_signature.dts b/tools/binman/test/341_fit_signature.dts new file mode 100644 index 00000000000..77bec8df1e5 --- /dev/null +++ b/tools/binman/test/341_fit_signature.dts @@ -0,0 +1,98 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + fit { + description = "test desc"; + #address-cells = <1>; + fit,fdt-list = "of-list"; + fit,sign; + + images { + u-boot { + description = "test u-boot"; + type = "standalone"; + arch = "arm64"; + os = "u-boot"; + compression = "none"; + load = <0x00000000>; + entry = <0x00000000>; + + u-boot-nodtb { + }; + + hash { + algo = "sha256"; + }; + + signature { + algo = "sha256,rsa2048"; + key-name-hint = "rsa2048"; + }; + }; + @atf-SEQ { + fit,operation = "split-elf"; + description = "test tf-a"; + type = "firmware"; + arch = "arm64"; + os = "arm-trusted-firmware"; + compression = "none"; + fit,load; + fit,entry; + fit,data; + + atf-bl31 { + }; + + hash { + algo = "sha256"; + }; + + signature { + algo = "sha256,rsa2048"; + key-name-hint = "rsa2048"; + }; + }; + @fdt-SEQ { + description = "test fdt"; + type = "flat_dt"; + compression = "none"; + + hash { + algo = "sha256"; + }; + + signature { + algo = "sha256,rsa2048"; + key-name-hint = "rsa2048"; + }; + }; + }; + + configurations { + default = "@conf-uboot-DEFAULT-SEQ"; + @conf-uboot-SEQ { + description = "uboot config"; + fdt = "fdt-SEQ"; + fit,firmware = "u-boot"; + fit,loadables; + + hash { + algo = "sha256"; + }; + + signature { + algo = "sha256,rsa2048"; + key-name-hint = "conf-rsa2048"; + sign-images = "firmware", "loadables", "fdt"; + }; + }; + }; + }; + }; +}; diff --git a/tools/binman/test/342_fit_signature.dts b/tools/binman/test/342_fit_signature.dts new file mode 100644 index 00000000000..267105d0f68 --- /dev/null +++ b/tools/binman/test/342_fit_signature.dts @@ -0,0 +1,61 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + fit { + description = "test desc"; + #address-cells = <1>; + fit,fdt-list = "of-list"; + fit,sign; + + images { + u-boot { + description = "test u-boot"; + type = "standalone"; + arch = "arm64"; + os = "u-boot"; + compression = "none"; + load = <0x00000000>; + entry = <0x00000000>; + + u-boot-nodtb { + }; + }; + @atf-SEQ { + fit,operation = "split-elf"; + description = "test tf-a"; + type = "firmware"; + arch = "arm64"; + os = "arm-trusted-firmware"; + compression = "none"; + fit,load; + fit,entry; + fit,data; + + atf-bl31 { + }; + }; + @fdt-SEQ { + description = "test fdt"; + type = "flat_dt"; + compression = "none"; + }; + }; + + configurations { + default = "@conf-uboot-DEFAULT-SEQ"; + @conf-uboot-SEQ { + description = "uboot config"; + fdt = "fdt-SEQ"; + fit,firmware = "u-boot"; + fit,loadables; + }; + }; + }; + }; +}; -- cgit v1.3.1 From 02eb9f166573009f576133a15d6323ff4ab479b1 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Sat, 21 Sep 2024 19:57:57 +0200 Subject: buildman: Correct some warnings about regex strings With Python 3.12 some warnings have cropped up. Fix them. Signed-off-by: Simon Glass --- tools/buildman/builder.py | 6 +++--- tools/buildman/toolchain.py | 8 ++++---- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/tools/buildman/builder.py b/tools/buildman/builder.py index 4090d328b30..cbf1345281b 100644 --- a/tools/buildman/builder.py +++ b/tools/buildman/builder.py @@ -34,7 +34,7 @@ from u_boot_pylib.terminal import tprint # Error in reading or end of file. # << # which indicates that BREAK_ME has an empty default -RE_NO_DEFAULT = re.compile(b'\((\w+)\) \[] \(NEW\)') +RE_NO_DEFAULT = re.compile(br'\((\w+)\) \[] \(NEW\)') # Symbol types which appear in the bloat feature (-B). Others are silently # dropped when reading in the 'nm' output @@ -374,9 +374,9 @@ class Builder: self._re_function = re.compile('(.*): In function.*') self._re_files = re.compile('In file included from.*') - self._re_warning = re.compile('(.*):(\d*):(\d*): warning: .*') + self._re_warning = re.compile(r'(.*):(\d*):(\d*): warning: .*') self._re_dtb_warning = re.compile('(.*): Warning .*') - self._re_note = re.compile('(.*):(\d*):(\d*): note: this is the location of the previous.*') + self._re_note = re.compile(r'(.*):(\d*):(\d*): note: this is the location of the previous.*') self._re_migration_warning = re.compile(r'^={21} WARNING ={22}\n.*\n=+\n', re.MULTILINE | re.DOTALL) diff --git a/tools/buildman/toolchain.py b/tools/buildman/toolchain.py index a7d7883b851..0c8a4fa16eb 100644 --- a/tools/buildman/toolchain.py +++ b/tools/buildman/toolchain.py @@ -440,12 +440,12 @@ class Toolchains: This converts ${blah} within the string to the value of blah. This function works recursively. + Resolved string + Args: var_dict: Dictionary containing variables and their values args: String containing make arguments Returns: - Resolved string - >>> bsettings.setup(None) >>> tcs = Toolchains() >>> tcs.Add('fred', False) @@ -456,7 +456,7 @@ class Toolchains: >>> tcs.ResolveReferences(var_dict, 'this=${oblique}_set${first}nd') 'this=OBLIQUE_setfi2ndrstnd' """ - re_var = re.compile('(\$\{[-_a-z0-9A-Z]{1,}\})') + re_var = re.compile(r'(\$\{[-_a-z0-9A-Z]{1,}\})') while True: m = re_var.search(args) @@ -495,7 +495,7 @@ class Toolchains: self._make_flags['target'] = brd.target arg_str = self.ResolveReferences(self._make_flags, self._make_flags.get(brd.target, '')) - args = re.findall("(?:\".*?\"|\S)+", arg_str) + args = re.findall(r"(?:\".*?\"|\S)+", arg_str) i = 0 while i < len(args): args[i] = args[i].replace('"', '') -- cgit v1.3.1 From 58918fa4f06c9bc529755efe133f514435d94467 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Wed, 25 Sep 2024 12:44:53 +0200 Subject: sandbox_spl: Restrict use of UPL when not enabled With sandbox_spl we want to use the file-based boot in CI, so that this flow is tested. The recent UPL change enabled booting via that method, thus overriding the file-based boot. Correct this by using UPL only when the --upl flag is given. Signed-off-by: Simon Glass Fixes: 91fde8e1764 ("sandbox: Add an SPL loader for UPL") --- arch/sandbox/cpu/spl.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/arch/sandbox/cpu/spl.c b/arch/sandbox/cpu/spl.c index c50df5f9179..7056cfd0180 100644 --- a/arch/sandbox/cpu/spl.c +++ b/arch/sandbox/cpu/spl.c @@ -55,9 +55,10 @@ void board_init_f(ulong flag) void board_boot_order(u32 *spl_boot_list) { + struct sandbox_state *state = state_get_current(); + spl_boot_list[0] = BOOT_DEVICE_VBE; - spl_boot_list[1] = BOOT_DEVICE_UPL; - spl_boot_list[2] = BOOT_DEVICE_BOARD; + spl_boot_list[1] = state->upl ? BOOT_DEVICE_UPL : BOOT_DEVICE_BOARD; } static int spl_board_load_file(struct spl_image_info *spl_image, -- cgit v1.3.1 From c95a3c889079c83f4a47c2a09794879378047592 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 30 Sep 2024 12:51:36 -0600 Subject: u_boot_pylib: Support running coverage on selected functions At present run_test_coverage() assumes you want code coverage for the entire code base. This is the normal situation, but sometimes it is useful to see the coverage provided by just a single test. Add support for this. Signed-off-by: Simon Glass --- tools/u_boot_pylib/test_util.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/tools/u_boot_pylib/test_util.py b/tools/u_boot_pylib/test_util.py index 857ce58c98c..dd671965263 100644 --- a/tools/u_boot_pylib/test_util.py +++ b/tools/u_boot_pylib/test_util.py @@ -23,8 +23,9 @@ except: use_concurrent = False -def run_test_coverage(prog, filter_fname, exclude_list, build_dir, required=None, - extra_args=None, single_thread='-P1'): +def run_test_coverage(prog, filter_fname, exclude_list, build_dir, + required=None, extra_args=None, single_thread='-P1', + args=None): """Run tests and check that we get 100% coverage Args: @@ -42,6 +43,7 @@ def run_test_coverage(prog, filter_fname, exclude_list, build_dir, required=None single_thread (str): Argument string to make the tests run single-threaded. This is necessary to get proper coverage results. The default is '-P0' + args (list of str): List of tests to run, or None to run all Raises: ValueError if the code coverage is not 100% @@ -66,9 +68,10 @@ def run_test_coverage(prog, filter_fname, exclude_list, build_dir, required=None 'coverage') cmd = ('%s%s run ' - '--omit "%s" %s %s %s %s' % (prefix, covtool, ','.join(glob_list), - prog, extra_args or '', test_cmd, - single_thread or '-P1')) + '--omit "%s" %s %s %s %s %s' % (prefix, covtool, ','.join(glob_list), + prog, extra_args or '', test_cmd, + single_thread or '-P1', + ' '.join(args) if args else '')) os.system(cmd) stdout = command.output(covtool, 'report') lines = stdout.splitlines() -- cgit v1.3.1 From 3473e1779cccf486df7cae43b79fedd837703756 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 30 Sep 2024 12:51:37 -0600 Subject: binman: Support getting test-coverage on just one test Pass the arguments through to test_util so that a single test can be used. Update the docs and add some missing backquotes in the same section. Signed-off-by: Simon Glass --- tools/binman/binman.rst | 5 ++++- tools/binman/main.py | 6 +++--- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/tools/binman/binman.rst b/tools/binman/binman.rst index f9a3a42183b..c25914312a8 100644 --- a/tools/binman/binman.rst +++ b/tools/binman/binman.rst @@ -2099,12 +2099,15 @@ Code coverage ------------- Binman is a critical tool and is designed to be very testable. Entry -implementations target 100% test coverage. Run 'binman test -T' to check this. +implementations target 100% test coverage. Run ``binman test -T`` to check this. To enable Python test coverage on Debian-type distributions (e.g. Ubuntu):: $ sudo apt-get install python-coverage python3-coverage python-pytest +You can also check the coverage provided by a single test, e.g.:: + + binman test -T testSimple Exit status ----------- diff --git a/tools/binman/main.py b/tools/binman/main.py index dc817ddcd42..619840e7d55 100755 --- a/tools/binman/main.py +++ b/tools/binman/main.py @@ -85,7 +85,7 @@ def RunTests(debug, verbosity, processes, test_preserve_dirs, args, toolpath): return (0 if result.wasSuccessful() else 1) -def RunTestCoverage(toolpath, build_dir): +def RunTestCoverage(toolpath, build_dir, args): """Run the tests and check that we get 100% coverage""" glob_list = control.GetEntryModules(False) all_set = set([os.path.splitext(os.path.basename(item))[0] @@ -97,7 +97,7 @@ def RunTestCoverage(toolpath, build_dir): test_util.run_test_coverage('tools/binman/binman', None, ['*test*', '*main.py', 'tools/patman/*', 'tools/dtoc/*', 'tools/u_boot_pylib/*'], - build_dir, all_set, extra_args or None) + build_dir, all_set, extra_args or None, args=args) def RunBinman(args): """Main entry point to binman once arguments are parsed @@ -117,7 +117,7 @@ def RunBinman(args): if args.cmd == 'test': if args.test_coverage: - RunTestCoverage(args.toolpath, args.build_dir) + RunTestCoverage(args.toolpath, args.build_dir, args.tests) else: ret_code = RunTests(args.debug, args.verbosity, args.processes, args.test_preserve_dirs, args.tests, -- cgit v1.3.1 From 7f4e1ea00bc417d99fa5a87091932280de34cab4 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 30 Sep 2024 12:51:38 -0600 Subject: binman: Add a tutorial on resolving test-coverage bugs Provide a short description of how tests work, why they are so critical and how to resolve gaps in Binman's test coverage. Signed-off-by: Simon Glass Acked-by: Heinrich Schuchardt Reviewed-by: Mattijs Korpershoek --- MAINTAINERS | 1 + doc/develop/binman_tests.rst | 734 +++++++++++++++++++++++++++++++++++++++++++ doc/develop/index.rst | 1 + tools/binman/binman.rst | 5 + 4 files changed, 741 insertions(+) create mode 100644 doc/develop/binman_tests.rst diff --git a/MAINTAINERS b/MAINTAINERS index eef63530f68..573403734b2 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -916,6 +916,7 @@ BINMAN M: Simon Glass M: Alper Nebi Yasak S: Maintained +F: doc/develop/binman_tests.rst F: tools/binman/ BLKMAP diff --git a/doc/develop/binman_tests.rst b/doc/develop/binman_tests.rst new file mode 100644 index 00000000000..a632694a6fe --- /dev/null +++ b/doc/develop/binman_tests.rst @@ -0,0 +1,734 @@ +.. SPDX-License-Identifier: GPL-2.0+ + +.. toctree:: + :maxdepth: 1 + +Binman Tests +============ + +.. contents:: + :depth: 2 + :local: + +There is some material on writing tests in the main Binman documentation +(see :doc:`package/index`). This short guide is separate so people don't +feel they have to read as much. + +Code and output is mostly included verbatim, which makes the doc longer, but +avoids its becoming confusing when the output or referenced code changes in the +future. + +Purpose +------- + +The main purpose of tests in Binman is to make sure that Binman actually does +what it is supposed to. Various people contribute code, refactoring is done +over time, but U-Boot users (developers, SoC vendors, board vendors) rely on +Binman producing images which function correctly. Without tests, a one-line +change could unintentionally break a corner-case and the problem might not be +noticed for months. Debugging an image-generation problem with a board you +don't have can be very hard. + +A secondary purpose is productivity. U-Boot contributors are busy and often +have too much on their plate. Trying to figure out why their patch broke +some other vendor's workflow can be very time-consuming and frustrating. By +building in tests from the start, this is largely avoided. If your change has +full test coverage and doesn't break any test, all is well and no one can +complain. + +A lessor purpose is to document what Binman actually does. If a test covers a +feature, it works. If there is no test coverage, no one can say for sure +whether it works in all expected situations, certainly not wihout manual +effort. + +In fact, strictly speaking it isn't completely clear what 'works' even means in +the case where these is no test to cover the code. We are often left guessing +as to what the documentation means, what was actually intended, etc. + +Finally, code-coverage helps to remove 'zombie code', copied from elsewhere +because it looks reasonable, but not actually needed. The same situation arises +in silicon-chip design, where a part of the chip is not validated. If it isn't +validated, it can be assumed not to work, either now or later, so it is best to +remove that logic to avoid it causing problems. + +Setting up +---------- + +Binman tests use various utility programs. Most of these are documented in +:doc:`../build/gcc`. But some are SoC-specific. To fetch these, tell Binman to +fetch or build any missing tools: + +.. code-block:: bash + + $ binman tool -f missing + +When this completes successfully, you can list the tools. You should see +something like this: + +.. code-block:: bash + + $ binman tool -l + Name Version Description Path + --------------- ----------- ------------------------- ------------------------------ + bootgen ****** Bootg Xilinx Bootgen /home/sglass/.binman-tools/bootgen + bzip2 1.0.8 bzip2 compression /usr/bin/bzip2 + cbfstool unknown Manipulate CBFS files /home/sglass/bin/cbfstool + fdt_add_pubkey unknown Generate image for U-Boot /home/sglass/bin/fdt_add_pubkey + fdtgrep unknown Grep devicetree files /home/sglass/bin/fdtgrep + fiptool v2.11.0(rele Manipulate ATF FIP files /home/sglass/.binman-tools/fiptool + futility v0.0.1-9f2e9 Chromium OS firmware utili /home/sglass/.binman-tools/futility + gzip 1.12 gzip compression /usr/bin/gzip + ifwitool unknown Manipulate Intel IFWI file /home/sglass/.binman-tools/ifwitool + lz4 v1.9.4 lz4 compression /usr/bin/lz4 + lzma_alone 9.22 beta lzma_alone compression /usr/bin/lzma_alone + lzop v1.04 lzo compression /usr/bin/lzop + mkeficapsule 2024.10-rc5- mkeficapsule tool for gene /home/sglass/bin/mkeficapsule + mkimage 2024.10-rc5- Generate image for U-Boot /home/sglass/bin/mkimage + openssl 3.0.13 30 Ja openssl cryptography toolk /usr/bin/openssl + xz 5.4.5 xz compression /usr/bin/xz + zstd v1.5.5 zstd compression /usr/bin/zstd + +The tools are written to ``~/.binman-tools`` so add that to your ``PATH``. +It's fine to have some of the tools elsewhere (e.g. ``~/bin``) so long as they +are up-to-date. This allows you use the version of the tools intended for +running tests. + +Now you should be able to actually run the tests: + +.. code-block:: bash + + $ binman test + ======================== Running binman tests ======================== + ...................................................................... + ...................................................................... + ...................................................................... + ...................................................................... + ...................................................................... + ...................................................................... + ...................................................................... + ...................................................................... + ........ + ---------------------------------------------------------------------- + Ran 568 tests in 2.578s + + OK + +If this doesn't work, see if you can have some missing tools. Check that the +dependencies are all there as above. If it is very slow, try installing +concurrencytest so that the tests run in parallel. + +The next thing to set up is code coverage, using the -T flag: + +.. code-block:: bash + + $ binman test -T + ======================== Running binman tests ======================== + ...................................................................... + ...................................................................... + ...................................................................... + ...................................................................... + ...................................................................... + ...................................................................... + ...................................................................... + ...................................................................... + ........ + ---------------------------------------------------------------------- + Ran 568 tests in 17.367s + + OK + + 99% + Name Stmts Miss Cover + --------------------------------------------------------------------------- + tools/binman/__init__.py 0 0 100% + tools/binman/bintool.py 263 0 100% + tools/binman/btool/bootgen.py 21 0 100% + tools/binman/btool/btool_gzip.py 5 0 100% + tools/binman/btool/bzip2.py 5 0 100% + tools/binman/btool/cbfstool.py 24 0 100% + tools/binman/btool/cst.py 15 4 73% + tools/binman/btool/fdt_add_pubkey.py 21 0 100% + tools/binman/btool/fdtgrep.py 26 0 100% + tools/binman/btool/fiptool.py 19 0 100% + tools/binman/btool/futility.py 19 0 100% + tools/binman/btool/ifwitool.py 22 0 100% + tools/binman/btool/lz4.py 22 0 100% + tools/binman/btool/lzma_alone.py 34 0 100% + tools/binman/btool/lzop.py 5 0 100% + tools/binman/btool/mkeficapsule.py 27 0 100% + tools/binman/btool/mkimage.py 23 0 100% + tools/binman/btool/openssl.py 42 0 100% + tools/binman/btool/xz.py 5 0 100% + tools/binman/btool/zstd.py 5 0 100% + tools/binman/cbfs_util.py 376 0 100% + tools/binman/cmdline.py 90 0 100% + tools/binman/control.py 409 0 100% + tools/binman/elf.py 241 0 100% + tools/binman/entry.py 548 0 100% + tools/binman/etype/alternates_fdt.py 58 0 100% + tools/binman/etype/atf_bl31.py 5 0 100% + tools/binman/etype/atf_fip.py 67 0 100% + tools/binman/etype/blob.py 49 0 100% + tools/binman/etype/blob_dtb.py 46 0 100% + tools/binman/etype/blob_ext.py 9 0 100% + tools/binman/etype/blob_ext_list.py 32 0 100% + tools/binman/etype/blob_named_by_arg.py 9 0 100% + tools/binman/etype/blob_phase.py 22 0 100% + tools/binman/etype/cbfs.py 101 0 100% + tools/binman/etype/collection.py 30 0 100% + tools/binman/etype/cros_ec_rw.py 5 0 100% + tools/binman/etype/efi_capsule.py 59 0 100% + tools/binman/etype/efi_empty_capsule.py 33 0 100% + tools/binman/etype/encrypted.py 34 0 100% + tools/binman/etype/fdtmap.py 62 0 100% + tools/binman/etype/files.py 35 0 100% + tools/binman/etype/fill.py 13 0 100% + tools/binman/etype/fit.py 311 0 100% + tools/binman/etype/fmap.py 37 0 100% + tools/binman/etype/gbb.py 37 0 100% + tools/binman/etype/image_header.py 53 0 100% + tools/binman/etype/intel_cmc.py 4 0 100% + tools/binman/etype/intel_descriptor.py 39 0 100% + tools/binman/etype/intel_fit.py 12 0 100% + tools/binman/etype/intel_fit_ptr.py 17 0 100% + tools/binman/etype/intel_fsp.py 4 0 100% + tools/binman/etype/intel_fsp_m.py 4 0 100% + tools/binman/etype/intel_fsp_s.py 4 0 100% + tools/binman/etype/intel_fsp_t.py 4 0 100% + tools/binman/etype/intel_ifwi.py 67 0 100% + tools/binman/etype/intel_me.py 4 0 100% + tools/binman/etype/intel_mrc.py 6 0 100% + tools/binman/etype/intel_refcode.py 6 0 100% + tools/binman/etype/intel_vbt.py 4 0 100% + tools/binman/etype/intel_vga.py 4 0 100% + tools/binman/etype/mkimage.py 84 0 100% + tools/binman/etype/null.py 9 0 100% + tools/binman/etype/nxp_imx8mcst.py 78 59 24% + tools/binman/etype/nxp_imx8mimage.py 38 6 84% + tools/binman/etype/opensbi.py 5 0 100% + tools/binman/etype/powerpc_mpc85xx_bootpg_resetvec.py 6 0 100% + tools/binman/etype/pre_load.py 76 0 100% + tools/binman/etype/rockchip_tpl.py 5 0 100% + tools/binman/etype/scp.py 5 0 100% + tools/binman/etype/section.py 418 0 100% + tools/binman/etype/tee_os.py 31 0 100% + tools/binman/etype/text.py 21 0 100% + tools/binman/etype/ti_board_config.py 139 0 100% + tools/binman/etype/ti_dm.py 5 0 100% + tools/binman/etype/ti_secure.py 65 0 100% + tools/binman/etype/ti_secure_rom.py 117 0 100% + tools/binman/etype/u_boot.py 7 0 100% + tools/binman/etype/u_boot_dtb.py 9 0 100% + tools/binman/etype/u_boot_dtb_with_ucode.py 51 0 100% + tools/binman/etype/u_boot_elf.py 19 0 100% + tools/binman/etype/u_boot_env.py 27 0 100% + tools/binman/etype/u_boot_expanded.py 4 0 100% + tools/binman/etype/u_boot_img.py 7 0 100% + tools/binman/etype/u_boot_nodtb.py 7 0 100% + tools/binman/etype/u_boot_spl.py 8 0 100% + tools/binman/etype/u_boot_spl_bss_pad.py 14 0 100% + tools/binman/etype/u_boot_spl_dtb.py 9 0 100% + tools/binman/etype/u_boot_spl_elf.py 8 0 100% + tools/binman/etype/u_boot_spl_expanded.py 12 0 100% + tools/binman/etype/u_boot_spl_nodtb.py 8 0 100% + tools/binman/etype/u_boot_spl_pubkey_dtb.py 32 0 100% + tools/binman/etype/u_boot_spl_with_ucode_ptr.py 8 0 100% + tools/binman/etype/u_boot_tpl.py 8 0 100% + tools/binman/etype/u_boot_tpl_bss_pad.py 14 0 100% + tools/binman/etype/u_boot_tpl_dtb.py 9 0 100% + tools/binman/etype/u_boot_tpl_dtb_with_ucode.py 8 0 100% + tools/binman/etype/u_boot_tpl_elf.py 8 0 100% + tools/binman/etype/u_boot_tpl_expanded.py 12 0 100% + tools/binman/etype/u_boot_tpl_nodtb.py 8 0 100% + tools/binman/etype/u_boot_tpl_with_ucode_ptr.py 12 0 100% + tools/binman/etype/u_boot_ucode.py 33 0 100% + tools/binman/etype/u_boot_vpl.py 8 0 100% + tools/binman/etype/u_boot_vpl_bss_pad.py 14 0 100% + tools/binman/etype/u_boot_vpl_dtb.py 9 0 100% + tools/binman/etype/u_boot_vpl_elf.py 8 0 100% + tools/binman/etype/u_boot_vpl_expanded.py 12 0 100% + tools/binman/etype/u_boot_vpl_nodtb.py 8 0 100% + tools/binman/etype/u_boot_with_ucode_ptr.py 42 0 100% + tools/binman/etype/vblock.py 38 0 100% + tools/binman/etype/x86_reset16.py 7 0 100% + tools/binman/etype/x86_reset16_spl.py 7 0 100% + tools/binman/etype/x86_reset16_tpl.py 7 0 100% + tools/binman/etype/x86_start16.py 7 0 100% + tools/binman/etype/x86_start16_spl.py 7 0 100% + tools/binman/etype/x86_start16_tpl.py 7 0 100% + tools/binman/etype/x509_cert.py 71 0 100% + tools/binman/etype/xilinx_bootgen.py 72 0 100% + tools/binman/fip_util.py 202 0 100% + tools/binman/fmap_util.py 49 0 100% + tools/binman/image.py 181 0 100% + tools/binman/state.py 201 0 100% + --------------------------------------------------------------------------- + TOTAL 5954 69 99% + + To get a report in 'htmlcov/index.html', type: python3-coverage html + Coverage error: 99%, but should be 100% + ValueError: Test coverage failure + +Unfortunately the run failed. As it suggests, create a report: + +.. code-block:: bash + + $ python3-coverage html + Wrote HTML report to htmlcov/index.html + +If you open that file in the browser, you can see which files are not reaching +100% and click on them. Here is ``nxp_imx8mimage.py``, for example: + +.. code-block:: python + + 43 # Generate mkimage configuration file similar to imx8mimage.cfg + 44 # and pass it to mkimage to generate SPL image for us here. + 45 cfg_fname = tools.get_output_filename('nxp.imx8mimage.cfg.%s' % uniq) + 46 with open(cfg_fname, 'w') as outf: + 47 print('ROM_VERSION v%d' % self.rom_version, file=outf) + 48 print('BOOT_FROM %s' % self.boot_from, file=outf) + 49 print('LOADER %s %#x' % (input_fname, self.loader_address), file=outf) + 50 + 51 output_fname = tools.get_output_filename(f'cfg-out.{uniq}') + 52 args = ['-d', input_fname, '-n', cfg_fname, '-T', 'imx8mimage', + 53 output_fname] + 54 if self.mkimage.run_cmd(*args) is not None: + 55 return tools.read_file(output_fname) + 56 else: + 57 # Bintool is missing; just use the input data as the output + 58 x self.record_missing_bintool(self.mkimage) + 59 x return data + 60 + 61 def SetImagePos(self, image_pos): + 62 # Customized SoC specific SetImagePos which skips the mkimage etype + 63 # implementation and removes the 0x48 offset introduced there. That + 64 # offset is only used for uImage/fitImage, which is not the case in + 65 # here. + 66 upto = 0x00 + 67 for entry in super().GetEntries().values(): + 68 x entry.SetOffsetSize(upto, None) + 69 + 70 # Give up if any entries lack a size + 71 x if entry.size is None: + 72 x return + 73 x upto += entry.size + 74 + 75 Entry_section.SetImagePos(self, image_pos) + +Most of the file is covered, but the lines marked with ``x`` indicate missing +coverage. The will show up red in your browser. + +What is a test? +--------------- + +A test is a function in ``ftest.py`` which uses an image description in +``tools/binman/test`` to perform some operations and exercise the code. Some +tests are just a few lines; some are more complicated. + +Here is a simple test: + +.. code-block:: python + + def testSimple(self): + """Test a simple binman with a single file""" + data = self._DoReadFile('005_simple.dts') + self.assertEqual(U_BOOT_DATA, data) + +This test tells Binman to build an image using the description. Then it checks +that the resulting image looks correct. The image description is: + +.. code-block:: devicetree + + /dts-v1/; + + / { + #address-cells = <1>; + #size-cells = <1>; + + binman { + u-boot { + }; + }; + }; + +As you will know from the Binman documentation, this says that there is +one image and it contains the U-Boot binary. So this test builds an image +consisting of a U-Boot binary, then checks that it does indeed have just a +U-Boot binary in it. + +Test data +--------- + +Using real binaries (like ``u-boot.bin``) to test Binman would be quite tedious. +Every output file would be large and it would be hard to tell by looking at the +output (e.g. with a hex dump) if a particular entry contains ``u-boot.bin`` or +``u-boot-spl.bin`` or something else. + +Binman gets around this by using simple placeholders. Here is the placeholder +for u-boot.bin: + +.. code-block:: python + + U_BOOT_DATA = b'1234' + +This is just bytes. So the test above checks that the output image contains +these four bytes. This makes verification fast for Binman and very easy for +humans. + +Even the devicetree is a placeholder: + +.. code-block:: python + + U_BOOT_DTB_DATA = b'udtb' + +But for some tests you need to use the real devicetree. In that case you can +use ``_DoReadFileRealDtb()``. See ``testUpdateFdtAll()`` for an example of how +to check the devicetree updated by Binman. + +Test structure +-------------- + +Each test is designed to test just one thing. Binman tests are named according +to what they are testing. Individually they don't do very much, but as a whole +they test every line of code in Binman. + +So ``testSimple()`` is designed to check that Binman can build the +simplest-possible image that isn't completely empty. + +Another type of test is one which checks error-handling, for example: + +.. code-block:: python + + def testFillNoSize(self): + """Test for an fill entry type with no size""" + with self.assertRaises(ValueError) as e: + self._DoReadFile('070_fill_no_size.dts') + self.assertIn("'fill' entry is missing properties: size", + str(e.exception)) + +This test deliberately tries to provoke an error. The image description is: + +.. code-block:: devicetree + + // SPDX-License-Identifier: GPL-2.0+ + /dts-v1/; + + / { + #address-cells = <1>; + #size-cells = <1>; + + binman { + size = <16>; + fill { + fill-byte = [ff]; + }; + }; + }; + +You can see that there is no size for the 'fill' entry, so we would expect +Binman to complain. The test checks that it actually does. It also checks the +error message produced by Binman. Sometimes you need to add several tests, each +with their own broken image description, in order to check all the error cases. + +Sometimes you need to capture the console output of Binman, to check it is +correct. You can to this with ``test_util.capture_sys_output()``, for example: + +.. code-block:: python + + with test_util.capture_sys_output() as (_, stderr): + self._DoTestFile('071_gbb.dts', force_missing_bintools='futility', + entry_args=entry_args) + err = stderr.getvalue() + self.assertRegex(err, "Image 'image'.*missing bintools.*: futility") + +The test collects the output and checks it with a regular expression. If you +need to see the test output (e.g. to debug it), you will have to remove that +capture line. + +How to add a new test +--------------------- + +This section explains the process of writing a new test. It uses an example to +help with this, but your code will be different. + +Generally you are adding a test because you are adding a new entry type +('etype'). So start by creating the shortest and simplest image-description you +can, which contains the new etype. Put it in a numbered file in +``tool/binman/test`` so that it comes last. All the numbers are unique and there +are no gaps. + +Example from ``tools/binman/test/339_nxp_imx8.dts``: + +.. code-block:: devicetree + + // SPDX-License-Identifier: GPL-2.0+ + + /dts-v1/; + + / { + #address-cells = <1>; + #size-cells = <1>; + + binman { + nxp-imx8mimage { + args; /* TODO: Needed by mkimage etype superclass */ + nxp,boot-from = "sd"; + nxp,rom-version = <1>; + nxp,loader-address = <0x10>; + }; + }; + }; + +Note that you should use tabs in the file, not spaces. You can see that this has +been cut down to the bare minimum, just enough to include the etype and the +arguments it needs. This is of course not a real image. It will not boot on +anything. But that's fine; we are just trying to test this one etype. Try not +to add any other sections and etypes unless they are absolutely essential for +your test to work. This helps others too: they don't need to understand the full +complexity of your etype just to read your test. + +Then create your test by adding a new function at the end of ``ftest.py``: + +.. code-block:: python + + def testNxpImx8Image(self): + """Test that binman can produce an iMX8 image""" + self._DoTestFile('339_nxp_imx8.dts') + +This uses the test file that you created. It doesn't check anything, it just +runs the image description through binman. + +Let's run it: + +.. code-block:: bash + + $ binman test testNxpImx8Image + ======================== Running binman tests ======================== + . + ---------------------------------------------------------------------- + Ran 1 test in 0.242s + + OK + +So the test passes. It doesn't really do a lot, but it does exercise the etype. +The next step is to update it to actually check the output: + +.. code-block:: python + + def testNxpImx8Image(self): + """Test that binman can produce an iMX8 image""" + data = self._DoReadFile('339_nxp_imx8.dts') + print('data', len(data)) + +The ``_DoReadFile()`` function is documented in the code. It returns the image +contents as the first part of a tuple. + +Running this we see: + +.. code-block:: bash + + data 2200 + +So it is producing a little over 8K of data. Your etype will be different, but +in any case you can add Python code to check that this data is actually correct, +based on your knowledge of your etype. Note that you should not be checking +whether the external tools (called 'bintools' in Binman) are actually working, +since presumably they have their own tests. You just need to check that the +image seems reasonable, e.g. is not empty, contains the expected sections, etc. + +When your etype does use a bintool, it also needs tests, but generally it will +be tested by virtue of the etype test. This is because your etype must call the +bintool to create the image. Sometimes you might need to add a test for a +bintool error-condition, though. + +Finishing code coverage +----------------------- + +The objective is to have test-coverage for every line of code that you add to +Binman. So how can you tell? First, get a coverage report as described above. +Look through the output for any files which are not at 100%. Add more test cases +(image descriptions and new functions in ``ftest.py``) until you have covered +each line. + +In the above example, here are some possible steps: + +#. The first red bit is where the ``mkimage`` call returns None. This can be + traced to ``Bintoolmkimage.mkimage()`` which calls + ``Bintool.run_cmd_result()`` and ``None`` means that ``mkimage`` is missing. + So the etype has code to handle that case, but it is never used. You can + look for other examples of ``self.mkimage`` returning ``None`` - e.g. + ``Entry_mkimage.BuildSectionData()`` does this. The clue for finding this is + that the ``nxp-imx8mimage`` etype is based on ``Entry_mkimage``: + + .. code-block:: python + + class Entry_nxp_imx8mimage(Entry_mkimage): + + It must be tested somewhere...in this case ``testMkimage()`` doesn't do it, + but ``testMkimageMissing()`` immediately below that does. So you can create a + similar test, e.g.: + + .. code-block:: python + + def testNxpImx8ImageMkimageMissing(self): + """Test that binman can produce an iMX8 image""" + with test_util.capture_sys_output() as (_, stderr): + self._DoTestFile('339_nxp_imx8.dts', + force_missing_bintools='mkimage') + err = stderr.getvalue() + self.assertRegex(err, "Image 'image'.*missing bintools.*: mkimage") + + Note that this uses exactly the same image description as the first test. + It just checks what happens when the tool is missing. Checking the coverage + again, you will see that the first red bit has gone: + + .. code-block:: bash + + $ binman test -T + $ python3-coverage html + +#. The second red bit is for ``SetImagePos()``. You can see that it is iterating + through the sub-entries inside the ``nxp-imx8mimage`` entry. In the case of + the 339 file, there are no such entries, so this code inside the for() loop + isn't used: + + .. code-block:: python + + def SetImagePos(self, image_pos): + # Customized SoC specific SetImagePos which skips the mkimage etype + # implementation and removes the 0x48 offset introduced there. That + # offset is only used for uImage/fitImage, which is not the case in + # here. + upto = 0x00 + for entry in super().GetEntries().values(): + entry.SetOffsetSize(upto, None) + + # Give up if any entries lack a size + if entry.size is None: + return + upto += entry.size + + Entry_section.SetImagePos(self, image_pos) + + The solution is to add an entry, e.g. in ``340_nxp_imx8_non_empty.dts``: + + .. code-block:: devicetree + + // SPDX-License-Identifier: GPL-2.0+ + + /dts-v1/; + + / { + #address-cells = <1>; + #size-cells = <1>; + + binman { + nxp-imx8mimage { + args; /* TODO: Needed by mkimage etype superclass */ + nxp,boot-from = "sd"; + nxp,rom-version = <1>; + nxp,loader-address = <0x10>; + + u-boot { + }; + }; + }; + }; + + Now write a little test to use it: + + .. code-block:: python + + def testNxpImx8ImageNonEmpty(self): + """Test that binman can produce an iMX8 image with something in it""" + data = self._DoReadFile('340_nxp_imx8_non_empty.dts') + # check data here + + With that, the second red bit goes away, because the for() loop is now used. + +#. There is one more red bit left, the ``return`` in ``SetImagePos()``. The + above effort got the for() loop to be executed, but it doesn't cover the + ``return``. It might have been copied from some other etype, e.g. the mkimage + one. See ``Entry_mkimage.SetImagePos()`` which contains the code: + + .. code-block:: python + + for entry in self.GetEntries().values(): + entry.SetOffsetSize(upto, None) + + # Give up if any entries lack a size + if entry.size is None: + return + upto += entry.size + + But which test covers that code for mkimage? By figuring that out, you could + use a similar technique. One way to find out is to delete the two lines in + ``Entry_mkimage`` which check for entry.size being None and returning, then + see what breaks with ``binman test``: + + .. code-block:: bash + + ERROR: binman.ftest.TestFunctional.testMkimageCollection (subunit.RemotedTestCase) + binman.ftest.TestFunctional.testMkimageCollection + ---------------------------------------------------------------------- + testtools.testresult.real._StringException: Traceback (most recent call last): + TypeError: unsupported operand type(s) for +=: 'int' and 'NoneType' + + ====================================================================== + ERROR: binman.ftest.TestFunctional.testMkimageImage (subunit.RemotedTestCase) + binman.ftest.TestFunctional.testMkimageImage + ---------------------------------------------------------------------- + testtools.testresult.real._StringException: Traceback (most recent call last): + TypeError: unsupported operand type(s) for +=: 'int' and 'NoneType' + + ====================================================================== + ERROR: binman.ftest.TestFunctional.testMkimageSpecial (subunit.RemotedTestCase) + binman.ftest.TestFunctional.testMkimageSpecial + ---------------------------------------------------------------------- + testtools.testresult.real._StringException: Traceback (most recent call last): + TypeError: unsupported operand type(s) for +=: 'int' and 'NoneType' + + We can verify that you got the right test, by putting the lines back in and + getting coverage for just that test: + + .. code-block:: bash + + binman test -T testMkimageCollection + python3-coverage html + + You will see a lot of red since we are seeing test coverage just for one + test, but if you look in ``mkimage.py`` at ``SetImagePos()`` you will see + that the ``return`` is covered (i.e. it is marked green). + + Looking at the ``.dts`` files for each of these tests, none jumps out as + being relevant to our case. It seems that this code just isn't needed, so the + best solution is to delete those two lines from the function: + + .. code-block:: python + + def SetImagePos(self, image_pos): + # Customized SoC specific SetImagePos which skips the mkimage etype + # implementation and removes the 0x48 offset introduced there. That + # offset is only used for uImage/fitImage, which is not the case in + # here. + upto = 0x00 + for entry in super().GetEntries().values(): + entry.SetOffsetSize(upto, None) + upto += entry.size + + Entry_section.SetImagePos(self, image_pos) + +We should check the updated code on a real build, to make sure it really +isn't needed, of course. + +Now, the test coverage is complete! + +If we later discover a case where those lines are needed, we can add the +lines back, along with a test for this case. + +Getting help +------------ + +If you are stuck and cannot work out how to add test coverage for your entry +type, ask on the U-Boot mailing list, cc ``Simon Glass `` or +on irc ``sjg1`` diff --git a/doc/develop/index.rst b/doc/develop/index.rst index c23192c2770..30f7fdb8847 100644 --- a/doc/develop/index.rst +++ b/doc/develop/index.rst @@ -83,6 +83,7 @@ Testing py_testing tests_writing tests_sandbox + binman_tests Refactoring ----------- diff --git a/tools/binman/binman.rst b/tools/binman/binman.rst index c25914312a8..381e55686f9 100644 --- a/tools/binman/binman.rst +++ b/tools/binman/binman.rst @@ -2194,6 +2194,11 @@ Use '-P 1' to disable this. It is automatically disabled when code coverage is being used (-T) since they are incompatible. +Writing tests +------------- + +See :doc:`../binman_tests`. + Debugging tests --------------- -- cgit v1.3.1 From 82c6ebae42e3462b8baf3e2a0e017f9ca18e81ac Mon Sep 17 00:00:00 2001 From: Marek Vasut Date: Sat, 5 Oct 2024 16:33:37 +0200 Subject: sandbox: dts: Add missing sandbox, emul phandle to sandbox-pmic This phandle was missing in the sandbox DT, add it, otherwise sandbox-i2c driver cannot look up the emulator via i2c_emul_find(). This fixes the following i2c_emul_find() error: " $ ./u-boot -Dc "" ... i2c_emul_find() No emulators for device 'sandbox_pmic' sandbox_pmic_write() write error to device: 0000000018c568d0 register: 0x0! out_set_value() PMIC write failed: -5 i2c_emul_find() No emulators for device 'sandbox_pmic' sandbox_pmic_write() write error to device: 0000000018c568d0 register: 0x0! out_set_value() PMIC write failed: -5 ... " Signed-off-by: Marek Vasut Reviewed-by: Heiko Schocher Reviewed-by: Simon Glass --- arch/sandbox/dts/sandbox_pmic.dtsi | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/sandbox/dts/sandbox_pmic.dtsi b/arch/sandbox/dts/sandbox_pmic.dtsi index 565c382ed45..ff2cb42844c 100644 --- a/arch/sandbox/dts/sandbox_pmic.dtsi +++ b/arch/sandbox/dts/sandbox_pmic.dtsi @@ -10,6 +10,7 @@ &sandbox_pmic { compatible = "sandbox,pmic"; + sandbox,emul = <&emul_pmic0>; buck1 { regulator-name = "SUPPLY_1.2V"; -- cgit v1.3.1 From 52c19173dfa162868be792d61a2e48e4bb6e0112 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 14 Oct 2024 16:31:53 -0600 Subject: video: Add a dark-grey console colour This is useful for highlighting something with a black background, as is needed with cedit when using a white-on-black console. Add this as a new colour. Signed-off-by: Simon Glass --- boot/scene.c | 2 +- drivers/video/video-uclass.c | 3 +++ include/video.h | 1 + 3 files changed, 5 insertions(+), 1 deletion(-) diff --git a/boot/scene.c b/boot/scene.c index 270c9c67233..0135287cfcb 100644 --- a/boot/scene.c +++ b/boot/scene.c @@ -339,7 +339,7 @@ static void scene_render_background(struct scene_obj *obj, bool box_only) /* draw a background for the object */ if (CONFIG_IS_ENABLED(SYS_WHITE_ON_BLACK)) { - fore = VID_BLACK; + fore = VID_DARK_GREY; back = VID_WHITE; } else { fore = VID_LIGHT_GRAY; diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c index 9823673f817..a5b3e898066 100644 --- a/drivers/video/video-uclass.c +++ b/drivers/video/video-uclass.c @@ -294,6 +294,9 @@ static const struct vid_rgb colours[VID_COLOUR_COUNT] = { { 0xff, 0x00, 0xff }, /* bright magenta */ { 0x00, 0xff, 0xff }, /* bright cyan */ { 0xff, 0xff, 0xff }, /* white */ + + /* an extra one for menus */ + { 0x40, 0x40, 0x40 }, /* dark gray */ }; u32 video_index_to_colour(struct video_priv *priv, enum colour_idx idx) diff --git a/include/video.h b/include/video.h index 606c8a37fb8..fd19723b1d2 100644 --- a/include/video.h +++ b/include/video.h @@ -181,6 +181,7 @@ enum colour_idx { VID_LIGHT_MAGENTA, VID_LIGHT_CYAN, VID_WHITE, + VID_DARK_GREY, VID_COLOUR_COUNT }; -- cgit v1.3.1 From 18a85cf7b5da3b8fabfed809cc972f0b58279620 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 14 Oct 2024 16:31:54 -0600 Subject: video: Avoid starting a new line to close to the bottom When starting a new text line, an assumption is made that the current vertical position is a multiple of the character height. When this is not true, characters can be written after the end of the framebuffer. This can causes crashes and strange errors from QEMU. Adjust the scrolling check when processing a newline character, to avoid any problems. Add some comments to make things a little clearer. Signed-off-by: Simon Glass --- drivers/video/vidconsole-uclass.c | 4 +++- include/video.h | 3 ++- include/video_console.h | 8 ++++++++ 3 files changed, 13 insertions(+), 2 deletions(-) diff --git a/drivers/video/vidconsole-uclass.c b/drivers/video/vidconsole-uclass.c index 80e7adf6a1a..ebe96bf0c2f 100644 --- a/drivers/video/vidconsole-uclass.c +++ b/drivers/video/vidconsole-uclass.c @@ -94,7 +94,9 @@ static void vidconsole_newline(struct udevice *dev) priv->ycur += priv->y_charsize; /* Check if we need to scroll the terminal */ - if ((priv->ycur + priv->y_charsize) / priv->y_charsize > priv->rows) { + if (vid_priv->rot % 2 ? + priv->ycur + priv->x_charsize > vid_priv->xsize : + priv->ycur + priv->y_charsize > vid_priv->ysize) { vidconsole_move_rows(dev, 0, rows, priv->rows - rows); for (i = 0; i < rows; i++) vidconsole_set_row(dev, priv->rows - i - 1, diff --git a/include/video.h b/include/video.h index fd19723b1d2..c7afe22d823 100644 --- a/include/video.h +++ b/include/video.h @@ -78,7 +78,8 @@ enum video_format { * * @xsize: Number of pixel columns (e.g. 1366) * @ysize: Number of pixels rows (e.g.. 768) - * @rot: Display rotation (0=none, 1=90 degrees clockwise, etc.) + * @rot: Display rotation (0=none, 1=90 degrees clockwise, etc.). THis + * does not affect @xsize and @ysize * @bpix: Encoded bits per pixel (enum video_log2_bpp) * @format: Pixel format (enum video_format) * @vidconsole_drv_name: Driver to use for the text console, NULL to diff --git a/include/video_console.h b/include/video_console.h index 8b5928dc5eb..723d2315606 100644 --- a/include/video_console.h +++ b/include/video_console.h @@ -27,6 +27,14 @@ enum { * Drivers must set up @rows, @cols, @x_charsize, @y_charsize in their probe() * method. Drivers may set up @xstart_frac if desired. * + * Note that these values relate to the rotated console, so that an 80x25 + * console which is rotated 90 degrees will have rows=80 and cols=25 + * + * The xcur_frac and ycur values refer to the unrotated coordinates, that is + * xcur_frac always advances with each character, even if its limit might be + * vid_priv->ysize instead of vid_priv->xsize if the console is rotated 90 or + * 270 degrees. + * * @sdev: stdio device, acting as an output sink * @xcur_frac: Current X position, in fractional units (VID_TO_POS(x)) * @ycur: Current Y position in pixels (0=top) -- cgit v1.3.1 From 54eca1d39bc980a7e99af53a5b32443d5774b1a0 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 14 Oct 2024 16:31:55 -0600 Subject: expo: Place menu items to the right of all labels At present a fixed position is used for menu items, 200 pixels to the right of the left side of the labels. This means that a menu item with a very long label may overlap the items. It seems better to calculate the maximum label width and then place the items to the right of all of them. To implement this, add a new struct to containing arrangement information. Calculate it before doing the actual arrangement. Add a new style item which sets the amount of space from the right side of the labels to left side of the items. Signed-off-by: Simon Glass --- boot/cedit.c | 13 ++++++++++--- boot/expo.c | 2 ++ boot/scene.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++++-- boot/scene_internal.h | 20 ++++++++++++++++++-- boot/scene_menu.c | 9 ++++++--- boot/scene_textline.c | 3 ++- doc/develop/expo.rst | 4 ++++ include/expo.h | 12 ++++++++++++ 8 files changed, 104 insertions(+), 11 deletions(-) diff --git a/boot/cedit.c b/boot/cedit.c index c29a2be14ce..5758cc5a0d6 100644 --- a/boot/cedit.c +++ b/boot/cedit.c @@ -51,10 +51,11 @@ struct cedit_iter_priv { int cedit_arange(struct expo *exp, struct video_priv *vpriv, uint scene_id) { + struct expo_arrange_info arr; struct scene_obj_txt *txt; struct scene_obj *obj; struct scene *scn; - int y; + int y, ret; scn = expo_lookup_scene_id(exp, scene_id); if (!scn) @@ -68,6 +69,11 @@ int cedit_arange(struct expo *exp, struct video_priv *vpriv, uint scene_id) if (txt) scene_obj_set_pos(scn, txt->obj.id, 200, 10); + memset(&arr, '\0', sizeof(arr)); + ret = scene_calc_arrange(scn, &arr); + if (ret < 0) + return log_msg_ret("arr", ret); + y = 100; list_for_each_entry(obj, &scn->obj_head, sibling) { switch (obj->type) { @@ -77,12 +83,13 @@ int cedit_arange(struct expo *exp, struct video_priv *vpriv, uint scene_id) break; case SCENEOBJT_MENU: scene_obj_set_pos(scn, obj->id, 50, y); - scene_menu_arrange(scn, (struct scene_obj_menu *)obj); + scene_menu_arrange(scn, &arr, + (struct scene_obj_menu *)obj); y += 50; break; case SCENEOBJT_TEXTLINE: scene_obj_set_pos(scn, obj->id, 50, y); - scene_textline_arrange(scn, + scene_textline_arrange(scn, &arr, (struct scene_obj_textline *)obj); y += 50; break; diff --git a/boot/expo.c b/boot/expo.c index ed01483f1d3..c7ce19e834b 100644 --- a/boot/expo.c +++ b/boot/expo.c @@ -258,6 +258,8 @@ int expo_apply_theme(struct expo *exp, ofnode node) ofnode_read_u32(node, "font-size", &theme->font_size); ofnode_read_u32(node, "menu-inset", &theme->menu_inset); ofnode_read_u32(node, "menuitem-gap-y", &theme->menuitem_gap_y); + ofnode_read_u32(node, "menu-title-margin-x", + &theme->menu_title_margin_x); list_for_each_entry(scn, &exp->scene_head, sibling) { ret = scene_apply_theme(scn, theme); diff --git a/boot/scene.c b/boot/scene.c index 0135287cfcb..a4836000b28 100644 --- a/boot/scene.c +++ b/boot/scene.c @@ -471,11 +471,59 @@ static int scene_obj_render(struct scene_obj *obj, bool text_mode) return 0; } +int scene_calc_arrange(struct scene *scn, struct expo_arrange_info *arr) +{ + struct scene_obj *obj; + + arr->label_width = 0; + list_for_each_entry(obj, &scn->obj_head, sibling) { + uint label_id = 0; + int width; + + switch (obj->type) { + case SCENEOBJT_NONE: + case SCENEOBJT_IMAGE: + case SCENEOBJT_TEXT: + break; + case SCENEOBJT_MENU: { + struct scene_obj_menu *menu; + + menu = (struct scene_obj_menu *)obj, + label_id = menu->title_id; + break; + } + case SCENEOBJT_TEXTLINE: { + struct scene_obj_textline *tline; + + tline = (struct scene_obj_textline *)obj, + label_id = tline->label_id; + break; + } + } + + if (label_id) { + int ret; + + ret = scene_obj_get_hw(scn, label_id, &width); + if (ret < 0) + return log_msg_ret("hei", ret); + arr->label_width = max(arr->label_width, width); + } + } + + return 0; +} + int scene_arrange(struct scene *scn) { + struct expo_arrange_info arr; struct scene_obj *obj; int ret; + ret = scene_calc_arrange(scn, &arr); + if (ret < 0) + return log_msg_ret("arr", ret); + list_for_each_entry(obj, &scn->obj_head, sibling) { switch (obj->type) { case SCENEOBJT_NONE: @@ -486,7 +534,7 @@ int scene_arrange(struct scene *scn) struct scene_obj_menu *menu; menu = (struct scene_obj_menu *)obj, - ret = scene_menu_arrange(scn, menu); + ret = scene_menu_arrange(scn, &arr, menu); if (ret) return log_msg_ret("arr", ret); break; @@ -495,7 +543,7 @@ int scene_arrange(struct scene *scn) struct scene_obj_textline *tline; tline = (struct scene_obj_textline *)obj, - ret = scene_textline_arrange(scn, tline); + ret = scene_textline_arrange(scn, &arr, tline); if (ret) return log_msg_ret("arr", ret); break; diff --git a/boot/scene_internal.h b/boot/scene_internal.h index e72202c9821..be25f6a8b96 100644 --- a/boot/scene_internal.h +++ b/boot/scene_internal.h @@ -96,10 +96,12 @@ int scene_calc_dims(struct scene *scn, bool do_menus); * if not already done * * @scn: Scene to update + * @arr: Arrangement information * @menu: Menu to process * Returns: 0 if OK, -ve on error */ -int scene_menu_arrange(struct scene *scn, struct scene_obj_menu *menu); +int scene_menu_arrange(struct scene *scn, struct expo_arrange_info *arr, + struct scene_obj_menu *menu); /** * scene_textline_arrange() - Set the position of things in a textline @@ -108,10 +110,12 @@ int scene_menu_arrange(struct scene *scn, struct scene_obj_menu *menu); * positioned correctly relative to the textline. * * @scn: Scene to update + * @arr: Arrangement information * @tline: textline to process * Returns: 0 if OK, -ve on error */ -int scene_textline_arrange(struct scene *scn, struct scene_obj_textline *tline); +int scene_textline_arrange(struct scene *scn, struct expo_arrange_info *arr, + struct scene_obj_textline *tline); /** * scene_apply_theme() - Apply a theme to a scene @@ -358,4 +362,16 @@ int scene_textline_open(struct scene *scn, struct scene_obj_textline *tline); */ int scene_textline_close(struct scene *scn, struct scene_obj_textline *tline); +/** + * scene_calc_arrange() - Calculate sizes needed to arrange a scene + * + * Checks the size of some objects and stores this info to help with a later + * scene arrangement + * + * @scn: Scene to check + * @arr: Place to put scene-arrangement info + * Returns: 0 if OK, -ve on error + */ +int scene_calc_arrange(struct scene *scn, struct expo_arrange_info *arr); + #endif /* __SCENE_INTERNAL_H */ diff --git a/boot/scene_menu.c b/boot/scene_menu.c index 80bd7457cb1..c331f6670cc 100644 --- a/boot/scene_menu.c +++ b/boot/scene_menu.c @@ -168,7 +168,8 @@ int scene_menu_calc_dims(struct scene_obj_menu *menu) return 0; } -int scene_menu_arrange(struct scene *scn, struct scene_obj_menu *menu) +int scene_menu_arrange(struct scene *scn, struct expo_arrange_info *arr, + struct scene_obj_menu *menu) { const bool open = menu->obj.flags & SCENEOF_OPEN; struct expo *exp = scn->expo; @@ -182,16 +183,18 @@ int scene_menu_arrange(struct scene *scn, struct scene_obj_menu *menu) x = menu->obj.dim.x; y = menu->obj.dim.y; if (menu->title_id) { + int width; + ret = scene_obj_set_pos(scn, menu->title_id, menu->obj.dim.x, y); if (ret < 0) return log_msg_ret("tit", ret); - ret = scene_obj_get_hw(scn, menu->title_id, NULL); + ret = scene_obj_get_hw(scn, menu->title_id, &width); if (ret < 0) return log_msg_ret("hei", ret); if (stack) - x += 200; + x += arr->label_width + theme->menu_title_margin_x; else y += ret * 2; } diff --git a/boot/scene_textline.c b/boot/scene_textline.c index bba8663b98d..6adef7cc173 100644 --- a/boot/scene_textline.c +++ b/boot/scene_textline.c @@ -87,7 +87,8 @@ int scene_textline_calc_dims(struct scene_obj_textline *tline) return 0; } -int scene_textline_arrange(struct scene *scn, struct scene_obj_textline *tline) +int scene_textline_arrange(struct scene *scn, struct expo_arrange_info *arr, + struct scene_obj_textline *tline) { const bool open = tline->obj.flags & SCENEOF_OPEN; bool point; diff --git a/doc/develop/expo.rst b/doc/develop/expo.rst index c87b6ec8128..f7b636e5fc6 100644 --- a/doc/develop/expo.rst +++ b/doc/develop/expo.rst @@ -176,6 +176,10 @@ menu-inset menuitem-gap-y Number of pixels between menu items +menu-title-margin-x + Number of pixels between right side of menu title to the left size of the + menu labels + Pop-up mode ----------- diff --git a/include/expo.h b/include/expo.h index c235fa2709d..50e11cd118c 100644 --- a/include/expo.h +++ b/include/expo.h @@ -59,11 +59,14 @@ struct expo_action { * @font_size: Default font size for all text * @menu_inset: Inset width (on each side and top/bottom) for menu items * @menuitem_gap_y: Gap between menu items in pixels + * @menu_title_margin_x: Gap between right side of menu title and left size of + * menu label */ struct expo_theme { u32 font_size; u32 menu_inset; u32 menuitem_gap_y; + u32 menu_title_margin_x; }; /** @@ -341,6 +344,15 @@ struct scene_obj_textline { uint pos; }; +/** + * struct expo_arrange_info - Information used when arranging a scene + * + * @label_width: Maximum width of labels in scene + */ +struct expo_arrange_info { + int label_width; +}; + /** * expo_new() - create a new expo * -- cgit v1.3.1 From 89f4f33c447e4c17e43372e3e91525187c43f54e Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 14 Oct 2024 16:31:56 -0600 Subject: expo: Set the initial next_id to 1 If expo_set_dynamic_start() is never called, the first scene created will have an ID of 0, which is invalid. Correct this by setting a default value. Add a test to check this. Signed-off-by: Simon Glass --- boot/expo.c | 1 + test/boot/expo.c | 23 +++++++++++++++++++++-- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/boot/expo.c b/boot/expo.c index c7ce19e834b..700786ec0cc 100644 --- a/boot/expo.c +++ b/boot/expo.c @@ -29,6 +29,7 @@ int expo_new(const char *name, void *priv, struct expo **expp) exp->priv = priv; INIT_LIST_HEAD(&exp->scene_head); INIT_LIST_HEAD(&exp->str_head); + exp->next_id = 1; *expp = exp; diff --git a/test/boot/expo.c b/test/boot/expo.c index 9b4aa803eb1..f6a6b4fd2cb 100644 --- a/test/boot/expo.c +++ b/test/boot/expo.c @@ -91,7 +91,7 @@ static int expo_base(struct unit_test_state *uts) *name = '\0'; ut_assertnonnull(exp); ut_asserteq(0, exp->scene_id); - ut_asserteq(0, exp->next_id); + ut_asserteq(1, exp->next_id); /* Make sure the name was allocated */ ut_assertnonnull(exp->name); @@ -130,7 +130,7 @@ static int expo_scene(struct unit_test_state *uts) ut_assertok(expo_new(EXPO_NAME, NULL, &exp)); scn = NULL; - ut_asserteq(0, exp->next_id); + ut_asserteq(1, exp->next_id); strcpy(name, SCENE_NAME1); id = scene_new(exp, name, SCENE1, &scn); *name = '\0'; @@ -167,6 +167,25 @@ static int expo_scene(struct unit_test_state *uts) } BOOTSTD_TEST(expo_scene, UTF_DM | UTF_SCAN_FDT); +/* Check creating a scene with no ID */ +static int expo_scene_no_id(struct unit_test_state *uts) +{ + struct scene *scn; + struct expo *exp; + char name[100]; + int id; + + ut_assertok(expo_new(EXPO_NAME, NULL, &exp)); + ut_asserteq(1, exp->next_id); + + strcpy(name, SCENE_NAME1); + id = scene_new(exp, SCENE_NAME1, 0, &scn); + ut_asserteq(1, scn->id); + + return 0; +} +BOOTSTD_TEST(expo_scene_no_id, UTF_DM | UTF_SCAN_FDT); + /* Check creating a scene with objects */ static int expo_object(struct unit_test_state *uts) { -- cgit v1.3.1 From d8ff97ce91529263c9d82a4bc00e133822673ab0 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 14 Oct 2024 16:31:57 -0600 Subject: expo: Use standard numbering for save and discard Set aside some expo IDs for 'save' and 'discard' buttons. This avoids needing to store the IDs for these. Adjust the documentation and expo tool for the new EXPOID_BASE_ID value. Ignore these objects when saving and loading the cedit, since they do not contain real data. Adjust 'cedit run' to return failure when the user exits the expo without saving. Update the test for this change as well. Signed-off-by: Simon Glass --- boot/cedit.c | 24 +++++++++++++++++++++--- boot/expo.c | 2 +- doc/develop/cedit.rst | 7 ++++++- doc/develop/expo.rst | 12 ++++++++---- include/expo.h | 20 ++++++++++++++++++++ include/test/cedit-test.h | 30 +++++++++++++++--------------- test/boot/cedit.c | 14 ++++++++------ test/boot/expo.c | 8 ++++---- test/boot/files/expo_ids.h | 3 +-- tools/expo.py | 33 ++++++++++++++++++++++++++++----- 10 files changed, 112 insertions(+), 41 deletions(-) diff --git a/boot/cedit.c b/boot/cedit.c index 5758cc5a0d6..cd935d4beba 100644 --- a/boot/cedit.c +++ b/boot/cedit.c @@ -154,7 +154,7 @@ int cedit_run(struct expo *exp) struct video_priv *vid_priv; uint scene_id; struct scene *scn; - bool done; + bool done, save; int ret; cli_ch_init(cch); @@ -164,6 +164,7 @@ int cedit_run(struct expo *exp) scene_id = ret; done = false; + save = false; do { struct expo_action act; int ichar, key; @@ -208,6 +209,15 @@ int cedit_run(struct expo *exp) case EXPOACT_OPEN: scene_set_open(scn, act.select.id, true); cedit_arange(exp, vid_priv, scene_id); + switch (scn->highlight_id) { + case EXPOID_SAVE: + done = true; + save = true; + break; + case EXPOID_DISCARD: + done = true; + break; + } break; case EXPOACT_CLOSE: scene_set_open(scn, act.select.id, false); @@ -229,6 +239,8 @@ int cedit_run(struct expo *exp) if (ret) return log_msg_ret("end", ret); + if (!save) + return -EACCES; return 0; } @@ -477,6 +489,9 @@ static int h_write_settings_env(struct scene_obj *obj, void *vpriv) const char *str; int val, ret; + if (obj->id < EXPOID_BASE_ID) + return 0; + snprintf(var, sizeof(var), "c.%s", obj->name); switch (obj->type) { @@ -549,6 +564,9 @@ static int h_read_settings_env(struct scene_obj *obj, void *vpriv) char var[60]; int val; + if (obj->id < EXPOID_BASE_ID) + return 0; + snprintf(var, sizeof(var), "c.%s", obj->name); switch (obj->type) { @@ -644,7 +662,7 @@ static int h_write_settings_cmos(struct scene_obj *obj, void *vpriv) int val, ret; uint i, seq; - if (obj->type != SCENEOBJT_MENU) + if (obj->type != SCENEOBJT_MENU || obj->id < EXPOID_BASE_ID) return 0; menu = (struct scene_obj_menu *)obj; @@ -734,7 +752,7 @@ static int h_read_settings_cmos(struct scene_obj *obj, void *vpriv) int val, ret; uint i; - if (obj->type != SCENEOBJT_MENU) + if (obj->type != SCENEOBJT_MENU || obj->id < EXPOID_BASE_ID) return 0; menu = (struct scene_obj_menu *)obj; diff --git a/boot/expo.c b/boot/expo.c index 700786ec0cc..786f665f53c 100644 --- a/boot/expo.c +++ b/boot/expo.c @@ -29,7 +29,7 @@ int expo_new(const char *name, void *priv, struct expo **expp) exp->priv = priv; INIT_LIST_HEAD(&exp->scene_head); INIT_LIST_HEAD(&exp->str_head); - exp->next_id = 1; + exp->next_id = EXPOID_BASE_ID; *expp = exp; diff --git a/doc/develop/cedit.rst b/doc/develop/cedit.rst index 82305b921f0..310be889240 100644 --- a/doc/develop/cedit.rst +++ b/doc/develop/cedit.rst @@ -94,7 +94,7 @@ them. Expo supports doing this with an enum, where every ID is listed in the enum:: enum { - ZERO, + ID_PROMPT = EXPOID_BASE_ID, ID_PROMPT, @@ -130,6 +130,11 @@ that means that something is wrong with your syntax, or perhaps you have an ID in the `.dts` file that is not mentioned in your enum. Check both files and try again. +Note that the first ID in your file must be no less that `EXPOID_BASE_ID` since +IDs before that are reserved. The `expo.py` tool automatically obtains this +value from the `expo.h` header file, but you must set the first ID to this +enum value. + Use the command interface ------------------------- diff --git a/doc/develop/expo.rst b/doc/develop/expo.rst index f7b636e5fc6..d8115c463c1 100644 --- a/doc/develop/expo.rst +++ b/doc/develop/expo.rst @@ -88,8 +88,13 @@ or even the IDs of objects. Programmatic creation of many items in a loop can be handled by allocating space in the enum for a maximum number of items, then adding the loop count to the enum values to obtain unique IDs. -Where dynamic IDs are need, use expo_set_dynamic_start() to set the start value, -so that they are allocated above the starting (enum) IDs. +Some standard IDs are reserved for certain purposes. These are defined by +`enum expo_id_t` and start at 1. `EXPOID_BASE_ID` defines the first ID which +can be used for an expo. + +An ID of 0 is invalid. If this is specified in an expo call then a valid +'dynamic IDs is allocated. Use expo_set_dynamic_start() to set the start +value, so that they are allocated above the starting (enum) IDs. All text strings are stored in a structure attached to the expo, referenced by a text ID. This makes it easier at some point to implement multiple languages or @@ -417,8 +422,7 @@ strings are provided inline in the nodes where they are used. /* this comment is parsed by the expo.py tool to insert the values below enum { - ZERO, - ID_PROMPT, + ID_PROMPT = EXPOID_BASE_ID, ID_SCENE1, ID_SCENE1_TITLE, diff --git a/include/expo.h b/include/expo.h index 50e11cd118c..d6e2ccee41b 100644 --- a/include/expo.h +++ b/include/expo.h @@ -15,6 +15,26 @@ struct udevice; #include +/** + * enum expo_id_t - standard expo IDs + * + * These are assumed to be in use at all times. Expos should use IDs starting + * from EXPOID_BASE_ID, + * + * @EXPOID_NONE: Not used, invalid ID 0 + * @EXPOID_SAVE: User has requested that the expo data be saved + * @EXPOID_DISCARD: User has requested that the expo data be discarded + * @EXPOID_BASE_ID: First ID which can be used for expo objects + */ +enum expo_id_t { + EXPOID_NONE, + + EXPOID_SAVE, + EXPOID_DISCARD, + + EXPOID_BASE_ID = 5, +}; + /** * enum expoact_type - types of actions reported by the expo * diff --git a/include/test/cedit-test.h b/include/test/cedit-test.h index 475ecc9c2dc..0d38a953415 100644 --- a/include/test/cedit-test.h +++ b/include/test/cedit-test.h @@ -9,24 +9,24 @@ #ifndef __cedit_test_h #define __cedit_test_h -#define ID_PROMPT 1 -#define ID_SCENE1 2 -#define ID_SCENE1_TITLE 3 +#define ID_PROMPT 5 +#define ID_SCENE1 6 +#define ID_SCENE1_TITLE 7 -#define ID_CPU_SPEED 4 -#define ID_CPU_SPEED_TITLE 5 -#define ID_CPU_SPEED_1 6 -#define ID_CPU_SPEED_2 7 -#define ID_CPU_SPEED_3 8 +#define ID_CPU_SPEED 8 +#define ID_CPU_SPEED_TITLE 9 +#define ID_CPU_SPEED_1 10 +#define ID_CPU_SPEED_2 11 +#define ID_CPU_SPEED_3 12 -#define ID_POWER_LOSS 9 -#define ID_AC_OFF 10 -#define ID_AC_ON 11 -#define ID_AC_MEMORY 12 +#define ID_POWER_LOSS 13 +#define ID_AC_OFF 14 +#define ID_AC_ON 15 +#define ID_AC_MEMORY 16 -#define ID_MACHINE_NAME 13 -#define ID_MACHINE_NAME_EDIT 14 +#define ID_MACHINE_NAME 17 +#define ID_MACHINE_NAME_EDIT 18 -#define ID_DYNAMIC_START 15 +#define ID_DYNAMIC_START 19 #endif diff --git a/test/boot/cedit.c b/test/boot/cedit.c index 1f7af8e5d79..0f2c2e73dd3 100644 --- a/test/boot/cedit.c +++ b/test/boot/cedit.c @@ -31,9 +31,11 @@ static int cedit_base(struct unit_test_state *uts) * ^N Move down to second item * ^M Select item * \e Quit + * + * cedit_run() returns -EACCESS so this command returns CMD_RET_FAILURE */ console_in_puts("\x0e\x0d\x0e\x0d\e"); - ut_assertok(run_command("cedit run", 0)); + ut_asserteq(1, run_command("cedit run", 0)); exp = cur_exp; scn = expo_lookup_scene_id(exp, exp->scene_id); @@ -147,14 +149,14 @@ static int cedit_env(struct unit_test_state *uts) strcpy(str, "my-machine"); ut_assertok(run_command("cedit write_env -v", 0)); - ut_assert_nextlinen("c.cpu-speed=7"); + ut_assert_nextlinen("c.cpu-speed=11"); ut_assert_nextlinen("c.cpu-speed-str=2.5 GHz"); - ut_assert_nextlinen("c.power-loss=10"); + ut_assert_nextlinen("c.power-loss=14"); ut_assert_nextlinen("c.power-loss-str=Always Off"); ut_assert_nextlinen("c.machine-name=my-machine"); ut_assert_console_end(); - ut_asserteq(7, env_get_ulong("c.cpu-speed", 10, 0)); + ut_asserteq(11, env_get_ulong("c.cpu-speed", 10, 0)); ut_asserteq_str("2.5 GHz", env_get("c.cpu-speed-str")); ut_asserteq_str("my-machine", env_get("c.machine-name")); @@ -163,8 +165,8 @@ static int cedit_env(struct unit_test_state *uts) *str = '\0'; ut_assertok(run_command("cedit read_env -v", 0)); - ut_assert_nextlinen("c.cpu-speed=7"); - ut_assert_nextlinen("c.power-loss=10"); + ut_assert_nextlinen("c.cpu-speed=11"); + ut_assert_nextlinen("c.power-loss=14"); ut_assert_nextlinen("c.machine-name=my-machine"); ut_assert_console_end(); diff --git a/test/boot/expo.c b/test/boot/expo.c index f6a6b4fd2cb..b0bf2988bf0 100644 --- a/test/boot/expo.c +++ b/test/boot/expo.c @@ -91,7 +91,7 @@ static int expo_base(struct unit_test_state *uts) *name = '\0'; ut_assertnonnull(exp); ut_asserteq(0, exp->scene_id); - ut_asserteq(1, exp->next_id); + ut_asserteq(EXPOID_BASE_ID, exp->next_id); /* Make sure the name was allocated */ ut_assertnonnull(exp->name); @@ -130,7 +130,7 @@ static int expo_scene(struct unit_test_state *uts) ut_assertok(expo_new(EXPO_NAME, NULL, &exp)); scn = NULL; - ut_asserteq(1, exp->next_id); + ut_asserteq(EXPOID_BASE_ID, exp->next_id); strcpy(name, SCENE_NAME1); id = scene_new(exp, name, SCENE1, &scn); *name = '\0'; @@ -176,11 +176,11 @@ static int expo_scene_no_id(struct unit_test_state *uts) int id; ut_assertok(expo_new(EXPO_NAME, NULL, &exp)); - ut_asserteq(1, exp->next_id); + ut_asserteq(EXPOID_BASE_ID, exp->next_id); strcpy(name, SCENE_NAME1); id = scene_new(exp, SCENE_NAME1, 0, &scn); - ut_asserteq(1, scn->id); + ut_asserteq(EXPOID_BASE_ID, scn->id); return 0; } diff --git a/test/boot/files/expo_ids.h b/test/boot/files/expo_ids.h index a86e0d06f6b..ffb511364b1 100644 --- a/test/boot/files/expo_ids.h +++ b/test/boot/files/expo_ids.h @@ -4,8 +4,7 @@ */ enum { - ZERO, - ID_PROMPT, + ID_PROMPT = EXPOID_BASE_ID, ID_SCENE1, ID_SCENE1_TITLE, diff --git a/tools/expo.py b/tools/expo.py index ea80c70f04e..44995f28a38 100755 --- a/tools/expo.py +++ b/tools/expo.py @@ -20,17 +20,22 @@ from u_boot_pylib import tools # Parse: # SCENE1 = 7, +# or SCENE1 = EXPOID_BASE_ID, # or SCENE2, -RE_ENUM = re.compile(r'(\S*)(\s*= (\d))?,') +RE_ENUM = re.compile(r'(\S*)(\s*= ([0-9A-Z_]+))?,') # Parse #define "string" RE_DEF = re.compile(r'#define (\S*)\s*"(.*)"') -def calc_ids(fname): +# Parse EXPOID_BASE_ID = 5, +RE_BASE_ID = re.compile(r'\s*EXPOID_BASE_ID\s*= (\d+),') + +def calc_ids(fname, base_id): """Figure out the value of the enums in a C file Args: fname (str): Filename to parse + base_id (int): Base ID (value of EXPOID_BASE_ID) Returns: OrderedDict(): @@ -55,8 +60,12 @@ def calc_ids(fname): if not line or line.startswith('/*'): continue m_enum = RE_ENUM.match(line) - if m_enum.group(3): - cur_id = int(m_enum.group(3)) + enum_name = m_enum.group(3) + if enum_name: + if enum_name == 'EXPOID_BASE_ID': + cur_id = base_id + else: + cur_id = int(enum_name) vals[m_enum.group(1)] = cur_id cur_id += 1 else: @@ -67,10 +76,24 @@ def calc_ids(fname): return vals +def find_base_id(): + fname = 'include/expo.h' + base_id = None + with open(fname, 'r', encoding='utf-8') as inf: + for line in inf.readlines(): + m_base_id = RE_BASE_ID.match(line) + if m_base_id: + base_id = int(m_base_id.group(1)) + if base_id is None: + raise ValueError('EXPOID_BASE_ID not found in expo.h') + #print(f'EXPOID_BASE_ID={base_id}') + return base_id + def run_expo(args): """Run the expo program""" + base_id = find_base_id() fname = args.enum_fname or args.layout - ids = calc_ids(fname) + ids = calc_ids(fname, base_id) if not ids: print(f"Warning: No enum ID values found in file '{fname}'") -- cgit v1.3.1 From 012e1e86523e3591ed5a986f894e207e1bcb32f1 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 14 Oct 2024 16:31:58 -0600 Subject: expo: Allow menu items to have values At present menu items are stored according to their sequence number in the menu. In some cases we may want to have holes in that sequence, or not use a sequence at all. Add a new 'value' property for menu items. This will be used for reading and writing, if present. If there is no 'value' property, then the normal sequence number will be used instead. Signed-off-by: Simon Glass --- arch/sandbox/dts/cedit.dtsi | 3 +++ boot/expo_build.c | 16 ++++++++++++---- boot/scene_internal.h | 10 ++++++++++ boot/scene_menu.c | 17 +++++++++++++++++ doc/develop/expo.rst | 10 ++++++++++ include/expo.h | 2 ++ test/boot/expo.c | 1 + test/boot/files/expo_layout.dts | 3 +++ 8 files changed, 58 insertions(+), 4 deletions(-) diff --git a/arch/sandbox/dts/cedit.dtsi b/arch/sandbox/dts/cedit.dtsi index 9bd84e62936..facd7a49bef 100644 --- a/arch/sandbox/dts/cedit.dtsi +++ b/arch/sandbox/dts/cedit.dtsi @@ -39,6 +39,9 @@ /* IDs for the menu items */ item-id = ; + + /* values for the menu items */ + item-value = <0 3 6>; }; power-loss { diff --git a/boot/expo_build.c b/boot/expo_build.c index a4df798adeb..fece3ea67f9 100644 --- a/boot/expo_build.c +++ b/boot/expo_build.c @@ -227,10 +227,10 @@ static void list_strings(struct build_info *info) static int menu_build(struct build_info *info, ofnode node, struct scene *scn, uint id, struct scene_obj **objp) { + const u32 *item_ids, *item_values; struct scene_obj_menu *menu; + int ret, size, i, num_items; uint title_id, menu_id; - const u32 *item_ids; - int ret, size, i; const char *name; name = ofnode_get_name(node); @@ -254,9 +254,15 @@ static int menu_build(struct build_info *info, ofnode node, struct scene *scn, return log_msg_ret("itm", -EINVAL); if (!size || size % sizeof(u32)) return log_msg_ret("isz", -EINVAL); - size /= sizeof(u32); + num_items = size / sizeof(u32); - for (i = 0; i < size; i++) { + item_values = ofnode_read_prop(node, "item-value", &size); + if (item_values) { + if (size != num_items * sizeof(u32)) + return log_msg_ret("vsz", -EINVAL); + } + + for (i = 0; i < num_items; i++) { struct scene_menitem *item; uint label, key, desc; @@ -280,6 +286,8 @@ static int menu_build(struct build_info *info, ofnode node, struct scene *scn, desc, 0, 0, &item); if (ret < 0) return log_msg_ret("mi", ret); + if (item_values) + item->value = fdt32_to_cpu(item_values[i]); } *objp = &menu->obj; diff --git a/boot/scene_internal.h b/boot/scene_internal.h index be25f6a8b96..ec9008ea593 100644 --- a/boot/scene_internal.h +++ b/boot/scene_internal.h @@ -281,6 +281,16 @@ struct scene_menitem *scene_menuitem_find(const struct scene_obj_menu *menu, struct scene_menitem *scene_menuitem_find_seq(const struct scene_obj_menu *menu, uint seq); +/** + * scene_menuitem_find_val() - Find the menu item with a given value + * + * @menu: Menu to check + * @find_val: Value to look for + * Return: menu item if found, else NULL + */ +struct scene_menitem *scene_menuitem_find_val(const struct scene_obj_menu *menu, + int val); + /** * scene_bbox_union() - update bouding box with the demensions of an object * diff --git a/boot/scene_menu.c b/boot/scene_menu.c index c331f6670cc..04ff1590bc1 100644 --- a/boot/scene_menu.c +++ b/boot/scene_menu.c @@ -61,6 +61,22 @@ struct scene_menitem *scene_menuitem_find_seq(const struct scene_obj_menu *menu, return NULL; } +struct scene_menitem *scene_menuitem_find_val(const struct scene_obj_menu *menu, + int val) +{ + struct scene_menitem *item; + uint i; + + i = 0; + list_for_each_entry(item, &menu->item_head, sibling) { + if (item->value == val) + return item; + i++; + } + + return NULL; +} + /** * update_pointers() - Update the pointer object and handle highlights * @@ -416,6 +432,7 @@ int scene_menuitem(struct scene *scn, uint menu_id, const char *name, uint id, item->desc_id = desc_id; item->preview_id = preview_id; item->flags = flags; + item->value = INT_MAX; list_add_tail(&item->sibling, &menu->item_head); if (itemp) diff --git a/doc/develop/expo.rst b/doc/develop/expo.rst index d8115c463c1..cc7c36173db 100644 --- a/doc/develop/expo.rst +++ b/doc/develop/expo.rst @@ -361,6 +361,13 @@ item-id Specifies the ID for each menu item. These are used for checking which item has been selected. +item-value + type: u32 list, optional + + Specifies the value for each menu item. These are used for saving and + loading. If this is omitted the value is its position in the menu (0..n-1). + Valid values are positive and negative integers INT_MIN...(INT_MAX - 1). + item-label / item-label-id type: string list / u32 list, required @@ -474,6 +481,9 @@ strings are provided inline in the nodes where they are used. /* IDs for the menu items */ item-id = ; + + /* values for the menu items */ + item-value = <(-1) 3 6>; }; power-loss { diff --git a/include/expo.h b/include/expo.h index d6e2ccee41b..acff98ea65b 100644 --- a/include/expo.h +++ b/include/expo.h @@ -330,6 +330,7 @@ enum scene_menuitem_flags_t { * @desc_id: ID of text object to use as the description text * @preview_id: ID of the preview object, or 0 if none * @flags: Flags for this item + * @value: Value for this item, or INT_MAX to use sequence * @sibling: Node to link this item to its siblings */ struct scene_menitem { @@ -340,6 +341,7 @@ struct scene_menitem { uint desc_id; uint preview_id; uint flags; + int value; struct list_head sibling; }; diff --git a/test/boot/expo.c b/test/boot/expo.c index b0bf2988bf0..1c2e746decc 100644 --- a/test/boot/expo.c +++ b/test/boot/expo.c @@ -717,6 +717,7 @@ static int expo_test_build(struct unit_test_state *uts) ut_asserteq(0, item->desc_id); ut_asserteq(0, item->preview_id); ut_asserteq(0, item->flags); + ut_asserteq(0, item->value); txt = scene_obj_find(scn, item->label_id, SCENEOBJT_NONE); ut_asserteq_str("2 GHz", expo_get_str(exp, txt->str_id)); diff --git a/test/boot/files/expo_layout.dts b/test/boot/files/expo_layout.dts index bed552288f4..ebe5adb27bb 100644 --- a/test/boot/files/expo_layout.dts +++ b/test/boot/files/expo_layout.dts @@ -39,6 +39,9 @@ item-id = ; + /* values for the menu items */ + item-value = <(-1) 3 6>; + start-bit = <0x400>; bit-length = <2>; }; -- cgit v1.3.1 From f698786cff8ae111a12395750def54ecb470d56e Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 14 Oct 2024 16:31:59 -0600 Subject: expo: Add a little more cedit CMOS logging Add some more logging in the CMOS read/write code. Tidy up a few comments while we are here. Signed-off-by: Simon Glass --- boot/cedit.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/boot/cedit.c b/boot/cedit.c index cd935d4beba..9d48d3ed50b 100644 --- a/boot/cedit.c +++ b/boot/cedit.c @@ -584,7 +584,7 @@ static int h_read_settings_env(struct scene_obj *obj, void *vpriv) /* * note that no validation is done here, to make sure the ID is - * valid * and actually points to a menu item + * valid and actually points to a menu item */ menu->cur_item_id = val; break; @@ -718,6 +718,7 @@ int cedit_write_settings_cmos(struct expo *exp, struct udevice *dev, } /* write the data to the RTC */ + log_debug("Writing CMOS\n"); first = CMOS_MAX_BYTES; last = -1; for (i = 0, count = 0; i < CMOS_MAX_BYTES; i++) { @@ -785,6 +786,7 @@ static int h_read_settings_cmos(struct scene_obj *obj, void *vpriv) } /* update the current item */ + log_debug("look for menuitem value %d in menu %d\n", val, menu->obj.id); mi = scene_menuitem_find_seq(menu, val); if (!mi) return log_msg_ret("seq", -ENOENT); @@ -819,7 +821,7 @@ int cedit_read_settings_cmos(struct expo *exp, struct udevice *dev, goto done; } - /* read the data to the RTC */ + /* indicate what bytes were read from the RTC */ first = CMOS_MAX_BYTES; last = -1; for (i = 0, count = 0; i < CMOS_MAX_BYTES; i++) { -- cgit v1.3.1 From 55a9de574ce07a507e094cc2ca6b3e9312f1acb8 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 14 Oct 2024 16:32:00 -0600 Subject: expo: Support menu-item values in cedit Update the cedit read/write functions to support menu items with values. Signed-off-by: Simon Glass --- boot/cedit.c | 148 ++++++++++++++++++++++++++-------------- boot/scene_menu.c | 2 +- doc/usage/cmd/cedit.rst | 15 +++- test/boot/cedit.c | 8 ++- test/boot/files/expo_layout.dts | 4 +- 5 files changed, 117 insertions(+), 60 deletions(-) diff --git a/boot/cedit.c b/boot/cedit.c index 9d48d3ed50b..d12892fbc4a 100644 --- a/boot/cedit.c +++ b/boot/cedit.c @@ -293,11 +293,49 @@ static int get_cur_menuitem_text(const struct scene_obj_menu *menu, return 0; } +/** + * get_cur_menuitem_val() - Get the value of a menu's current item + * + * Obtains the value of the current item in the menu. If no value, then + * enumerates the items of a menu (0, 1, 2) and returns the sequence number of + * the currently selected item. If the first item is selected, this returns 0; + * if the second, 1; etc. + * + * @menu: Menu to check + * @valp: Returns current-item value / sequence number + * Return: 0 on success, else -ve error value + */ +static int get_cur_menuitem_val(const struct scene_obj_menu *menu, int *valp) +{ + const struct scene_menitem *mi; + int seq; + + seq = 0; + list_for_each_entry(mi, &menu->item_head, sibling) { + if (mi->id == menu->cur_item_id) { + *valp = mi->value == INT_MAX ? seq : mi->value; + return 0; + } + seq++; + } + + return log_msg_ret("nf", -ENOENT); +} + +/** + * write_dt_string() - Write a string to the devicetree, expanding if needed + * + * If this fails, it tries again after expanding the devicetree a little + * + * @buf: Buffer containing the devicetree + * @name: Property name to use + * @str: String value + * Return: 0 if OK, -EFAULT if something went horribly wrong + */ static int write_dt_string(struct abuf *buf, const char *name, const char *str) { int ret, i; - /* write the text of the current item */ ret = -EAGAIN; for (i = 0; ret && i < 2; i++) { ret = fdt_property_string(abuf_data(buf), name, str); @@ -315,6 +353,38 @@ static int write_dt_string(struct abuf *buf, const char *name, const char *str) return 0; } +/** + * write_dt_u32() - Write an int to the devicetree, expanding if needed + * + * If this fails, it tries again after expanding the devicetree a little + * + * @buf: Buffer containing the devicetree + * @name: Property name to use + * @lva: Integer value + * Return: 0 if OK, -EFAULT if something went horribly wrong + */ +static int write_dt_u32(struct abuf *buf, const char *name, uint val) +{ + int ret, i; + + /* write the text of the current item */ + ret = -EAGAIN; + for (i = 0; ret && i < 2; i++) { + ret = fdt_property_u32(abuf_data(buf), name, val); + if (!i) { + ret = check_space(ret, buf); + if (ret) + return log_msg_ret("rs2", -ENOMEM); + } + } + + /* this should not happen */ + if (ret) + return log_msg_ret("str", -EFAULT); + + return 0; +} + static int h_write_settings(struct scene_obj *obj, void *vpriv) { struct cedit_iter_priv *priv = vpriv; @@ -339,23 +409,21 @@ static int h_write_settings(struct scene_obj *obj, void *vpriv) const struct scene_obj_menu *menu; const char *str; char name[80]; - int i; + int val; /* write the ID of the current item */ menu = (struct scene_obj_menu *)obj; - ret = -EAGAIN; - for (i = 0; ret && i < 2; i++) { - ret = fdt_property_u32(abuf_data(buf), obj->name, - menu->cur_item_id); - if (!i) { - ret = check_space(ret, buf); - if (ret) - return log_msg_ret("res", -ENOMEM); - } - } - /* this should not happen */ + ret = write_dt_u32(buf, obj->name, menu->cur_item_id); if (ret) - return log_msg_ret("wrt", -EFAULT); + return log_msg_ret("wrt", ret); + + snprintf(name, sizeof(name), "%s-value", obj->name); + ret = get_cur_menuitem_val(menu, &val); + if (ret < 0) + return log_msg_ret("cur", ret); + ret = write_dt_u32(buf, name, val); + if (ret) + return log_msg_ret("wr2", ret); ret = get_cur_menuitem_text(menu, &str); if (ret) @@ -521,6 +589,14 @@ static int h_write_settings_env(struct scene_obj *obj, void *vpriv) ret = env_set(name, str); if (ret) return log_msg_ret("st2", ret); + + ret = get_cur_menuitem_val(menu, &val); + if (ret < 0) + return log_msg_ret("cur", ret); + snprintf(name, sizeof(name), "c.%s-value", obj->name); + if (priv->verbose) + printf("%s=%d\n", name, val); + break; case SCENEOBJT_TEXTLINE: { const struct scene_obj_textline *tline; @@ -624,43 +700,12 @@ int cedit_read_settings_env(struct expo *exp, bool verbose) return 0; } -/** - * get_cur_menuitem_seq() - Get the sequence number of a menu's current item - * - * Enumerates the items of a menu (0, 1, 2) and returns the sequence number of - * the currently selected item. If the first item is selected, this returns 0; - * if the second, 1; etc. - * - * @menu: Menu to check - * Return: Sequence number on success, else -ve error value - */ -static int get_cur_menuitem_seq(const struct scene_obj_menu *menu) -{ - const struct scene_menitem *mi; - int seq, found; - - seq = 0; - found = -1; - list_for_each_entry(mi, &menu->item_head, sibling) { - if (mi->id == menu->cur_item_id) { - found = seq; - break; - } - seq++; - } - - if (found == -1) - return log_msg_ret("nf", -ENOENT); - - return found; -} - static int h_write_settings_cmos(struct scene_obj *obj, void *vpriv) { const struct scene_obj_menu *menu; struct cedit_iter_priv *priv = vpriv; int val, ret; - uint i, seq; + uint i; if (obj->type != SCENEOBJT_MENU || obj->id < EXPOID_BASE_ID) return 0; @@ -668,11 +713,10 @@ static int h_write_settings_cmos(struct scene_obj *obj, void *vpriv) menu = (struct scene_obj_menu *)obj; val = menu->cur_item_id; - ret = get_cur_menuitem_seq(menu); + ret = get_cur_menuitem_val(menu, &val); if (ret < 0) return log_msg_ret("cur", ret); - seq = ret; - log_debug("%s: seq=%d\n", menu->obj.name, seq); + log_debug("%s: val=%d\n", menu->obj.name, val); /* figure out where to place this item */ if (!obj->bit_length) @@ -680,11 +724,11 @@ static int h_write_settings_cmos(struct scene_obj *obj, void *vpriv) if (obj->start_bit + obj->bit_length > CMOS_MAX_BITS) return log_msg_ret("bit", -E2BIG); - for (i = 0; i < obj->bit_length; i++, seq >>= 1) { + for (i = 0; i < obj->bit_length; i++, val >>= 1) { uint bitnum = obj->start_bit + i; priv->mask[CMOS_BYTE(bitnum)] |= 1 << CMOS_BIT(bitnum); - if (seq & 1) + if (val & 1) priv->value[CMOS_BYTE(bitnum)] |= BIT(CMOS_BIT(bitnum)); log_debug("bit %x %x %x\n", bitnum, priv->mask[CMOS_BYTE(bitnum)], @@ -787,7 +831,7 @@ static int h_read_settings_cmos(struct scene_obj *obj, void *vpriv) /* update the current item */ log_debug("look for menuitem value %d in menu %d\n", val, menu->obj.id); - mi = scene_menuitem_find_seq(menu, val); + mi = scene_menuitem_find_val(menu, val); if (!mi) return log_msg_ret("seq", -ENOENT); diff --git a/boot/scene_menu.c b/boot/scene_menu.c index 04ff1590bc1..17150af145d 100644 --- a/boot/scene_menu.c +++ b/boot/scene_menu.c @@ -69,7 +69,7 @@ struct scene_menitem *scene_menuitem_find_val(const struct scene_obj_menu *menu, i = 0; list_for_each_entry(item, &menu->item_head, sibling) { - if (item->value == val) + if (item->value == INT_MAX ? val == i : item->value == val) return item; i++; } diff --git a/doc/usage/cmd/cedit.rst b/doc/usage/cmd/cedit.rst index 5670805a00e..f29f1b3f388 100644 --- a/doc/usage/cmd/cedit.rst +++ b/doc/usage/cmd/cedit.rst @@ -107,8 +107,10 @@ That results in:: / { cedit-values { cpu-speed = <0x00000006>; + cpu-speed-value = <0x00000003>; cpu-speed-str = "2 GHz"; power-loss = <0x0000000a>; + power-loss-value = <0x00000000>; power-loss-str = "Always Off"; }; } @@ -118,16 +120,23 @@ That results in:: This shows settings being stored in the environment:: => cedit write_env -v - c.cpu-speed=7 + c.cpu-speed=11 c.cpu-speed-str=2.5 GHz - c.power-loss=12 - c.power-loss-str=Memory + c.cpu-speed-value=3 + c.power-loss=14 + c.power-loss-str=Always Off + c.power-loss-value=0 + c.machine-name=my-machine + c.cpu-speed=11 + c.power-loss=14 + c.machine-name=my-machine => print ... c.cpu-speed=6 c.cpu-speed-str=2 GHz c.power-loss=10 c.power-loss-str=Always Off + c.machine-name=my-machine ... => cedit read_env -v diff --git a/test/boot/cedit.c b/test/boot/cedit.c index 0f2c2e73dd3..4d1b99bc2ea 100644 --- a/test/boot/cedit.c +++ b/test/boot/cedit.c @@ -96,14 +96,16 @@ static int cedit_fdt(struct unit_test_state *uts) ut_asserteq(ID_CPU_SPEED_2, ofnode_read_u32_default(node, "cpu-speed", 0)); + ut_asserteq(3, + ofnode_read_u32_default(node, "cpu-speed-value", 0)); ut_asserteq_str("2.5 GHz", ofnode_read_string(node, "cpu-speed-str")); ut_asserteq_str("my-machine", ofnode_read_string(node, "machine-name")); - /* There should only be 5 properties */ + /* There should only be 7 properties */ for (i = 0, ofnode_first_property(node, &prop); ofprop_valid(&prop); i++, ofnode_next_property(&prop)) ; - ut_asserteq(5, i); + ut_asserteq(7, i); ut_assert_console_end(); @@ -151,8 +153,10 @@ static int cedit_env(struct unit_test_state *uts) ut_assertok(run_command("cedit write_env -v", 0)); ut_assert_nextlinen("c.cpu-speed=11"); ut_assert_nextlinen("c.cpu-speed-str=2.5 GHz"); + ut_assert_nextlinen("c.cpu-speed-value=3"); ut_assert_nextlinen("c.power-loss=14"); ut_assert_nextlinen("c.power-loss-str=Always Off"); + ut_assert_nextlinen("c.power-loss-value=0"); ut_assert_nextlinen("c.machine-name=my-machine"); ut_assert_console_end(); diff --git a/test/boot/files/expo_layout.dts b/test/boot/files/expo_layout.dts index ebe5adb27bb..9bc1e4950b9 100644 --- a/test/boot/files/expo_layout.dts +++ b/test/boot/files/expo_layout.dts @@ -40,10 +40,10 @@ ID_CPU_SPEED_3>; /* values for the menu items */ - item-value = <(-1) 3 6>; + item-value = <0 3 6>; start-bit = <0x400>; - bit-length = <2>; + bit-length = <3>; }; power-loss { -- cgit v1.3.1 From fde707501f614ad57dbbbc6269f1b99b86875c39 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 14 Oct 2024 16:32:01 -0600 Subject: expo: Drop unneceesary calls to expo_str() The scene_txt_str() function calls expo_str() so there is no need to call it beforehand. Drop this unnecessary code. Signed-off-by: Simon Glass --- boot/expo_build.c | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/boot/expo_build.c b/boot/expo_build.c index fece3ea67f9..930cc40ec07 100644 --- a/boot/expo_build.c +++ b/boot/expo_build.c @@ -46,7 +46,6 @@ int add_txt_str(struct build_info *info, ofnode node, struct scene *scn, const char *find_name, uint obj_id) { const char *text; - uint str_id; int ret; info->err_prop = find_name; @@ -67,12 +66,7 @@ int add_txt_str(struct build_info *info, ofnode node, struct scene *scn, return log_msg_ret("id", -EINVAL); } - ret = expo_str(scn->expo, find_name, 0, text); - if (ret < 0) - return log_msg_ret("add", ret); - str_id = ret; - - ret = scene_txt_str(scn, find_name, obj_id, str_id, text, NULL); + ret = scene_txt_str(scn, find_name, obj_id, 0, text, NULL); if (ret < 0) return log_msg_ret("add", ret); @@ -94,7 +88,6 @@ int add_txt_str_list(struct build_info *info, ofnode node, struct scene *scn, const char *find_name, int index, uint obj_id) { const char *text; - uint str_id; int ret; ret = ofnode_read_string_index(node, find_name, index, &text); @@ -114,12 +107,7 @@ int add_txt_str_list(struct build_info *info, ofnode node, struct scene *scn, return log_msg_ret("id", -EINVAL); } - ret = expo_str(scn->expo, find_name, 0, text); - if (ret < 0) - return log_msg_ret("add", ret); - str_id = ret; - - ret = scene_txt_str(scn, find_name, obj_id, str_id, text, NULL); + ret = scene_txt_str(scn, find_name, obj_id, 0, text, NULL); if (ret < 0) return log_msg_ret("add", ret); -- cgit v1.3.1 From 96cbafb1d2902cfbf8a7776f258e97b4394d00fe Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 14 Oct 2024 16:32:02 -0600 Subject: expo: Drop scene_title_set() This function is really just an assignment, so serves no useful purpose. Drop it. Signed-off-by: Simon Glass --- boot/expo_build.c | 4 ++-- boot/scene.c | 7 ------- include/expo.h | 9 --------- test/boot/expo.c | 2 +- 4 files changed, 3 insertions(+), 19 deletions(-) diff --git a/boot/expo_build.c b/boot/expo_build.c index 930cc40ec07..d97347e1725 100644 --- a/boot/expo_build.c +++ b/boot/expo_build.c @@ -404,7 +404,7 @@ static int scene_build(struct build_info *info, ofnode scn_node, if (ret < 0) return log_msg_ret("tit", ret); title_id = ret; - scene_title_set(scn, title_id); + scn->title_id = title_id; ret = add_txt_str(info, scn_node, scn, "prompt", 0); if (ret < 0) @@ -420,7 +420,7 @@ static int scene_build(struct build_info *info, ofnode scn_node, return 0; } -int build_it(struct build_info *info, ofnode root, struct expo **expp) +static int build_it(struct build_info *info, ofnode root, struct expo **expp) { ofnode scenes, node; struct expo *exp; diff --git a/boot/scene.c b/boot/scene.c index a4836000b28..3290a40222a 100644 --- a/boot/scene.c +++ b/boot/scene.c @@ -70,13 +70,6 @@ void scene_destroy(struct scene *scn) free(scn); } -int scene_title_set(struct scene *scn, uint id) -{ - scn->title_id = id; - - return 0; -} - int scene_obj_count(struct scene *scn) { return list_count_nodes(&scn->obj_head); diff --git a/include/expo.h b/include/expo.h index acff98ea65b..8cb37260db5 100644 --- a/include/expo.h +++ b/include/expo.h @@ -540,15 +540,6 @@ void scene_set_highlight_id(struct scene *scn, uint id); */ int scene_set_open(struct scene *scn, uint id, bool open); -/** - * scene_title_set() - set the scene title - * - * @scn: Scene to update - * @title_id: Title ID to set - * Returns: 0 if OK - */ -int scene_title_set(struct scene *scn, uint title_id); - /** * scene_obj_count() - Count the number of objects in a scene * diff --git a/test/boot/expo.c b/test/boot/expo.c index 1c2e746decc..db14ff86f8b 100644 --- a/test/boot/expo.c +++ b/test/boot/expo.c @@ -151,7 +151,7 @@ static int expo_scene(struct unit_test_state *uts) scn = NULL; id = scene_new(exp, SCENE_NAME2, 0, &scn); ut_assertnonnull(scn); - ut_assertok(scene_title_set(scn, title_id)); + scn->title_id = title_id; ut_asserteq(STR_SCENE_TITLE + 1, id); ut_asserteq(STR_SCENE_TITLE + 2, exp->next_id); ut_asserteq_ptr(exp, scn->expo); -- cgit v1.3.1 From 790687c9883f220bdb52f1675c2684e7ec42370d Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 14 Oct 2024 16:32:03 -0600 Subject: expo: Add forward declaration for udevice to cedit Some files may include this header file without first including dm.h so add a forward declaration. Signed-off-by: Simon Glass --- include/cedit.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/cedit.h b/include/cedit.h index a31b4245247..856509f0c7f 100644 --- a/include/cedit.h +++ b/include/cedit.h @@ -14,6 +14,7 @@ struct abuf; struct expo; struct scene; +struct udevice; struct video_priv; struct udevice; -- cgit v1.3.1 From 1903aa72ec443faa28a931da090ef379955d7e3f Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 14 Oct 2024 16:32:05 -0600 Subject: docker: Update coreboot Update to a newer version which supports settings in CMOS RAM and linear framebuffer. Signed-off-by: Simon Glass Reviewed-by: Tom Rini --- tools/docker/Dockerfile | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tools/docker/Dockerfile b/tools/docker/Dockerfile index c401170b1e1..967ac89fbde 100644 --- a/tools/docker/Dockerfile +++ b/tools/docker/Dockerfile @@ -244,12 +244,15 @@ RUN mkdir /tmp/trace && \ rm -rf /tmp/trace # Build coreboot -RUN wget -O - https://coreboot.org/releases/coreboot-4.22.01.tar.xz | tar -C /tmp -xJ && \ - cd /tmp/coreboot-4.22.01 && \ +RUN wget -O - https://coreboot.org/releases/coreboot-24.08.tar.xz | tar -C /tmp -xJ && \ + cd /tmp/coreboot-24.08 && \ make crossgcc-i386 CPUS=$(nproc) && \ make -C payloads/coreinfo olddefconfig && \ make -C payloads/coreinfo && \ make olddefconfig && \ + echo CONFIG_GENERIC_LINEAR_FRAMEBUFFER=y | tee -a .config && \ + echo CONFIG_USE_OPTION_TABLE=y | tee -a .config && \ + make olddefconfig && \ make -j $(nproc) && \ sudo mkdir /opt/coreboot && \ sudo cp build/coreboot.rom build/cbfstool /opt/coreboot/ -- cgit v1.3.1 From 424d537e70a056103608085e24cec56b77767b22 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 14 Oct 2024 16:32:06 -0600 Subject: doc: Update coreboot's CI information This is in the Dockerfile now, so update this section of the docs. Signed-off-by: Simon Glass --- doc/board/coreboot/coreboot.rst | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/doc/board/coreboot/coreboot.rst b/doc/board/coreboot/coreboot.rst index 7154f59c374..a177265c16e 100644 --- a/doc/board/coreboot/coreboot.rst +++ b/doc/board/coreboot/coreboot.rst @@ -181,16 +181,4 @@ coreboot in CI CI runs tests using a pre-built coreboot image. This ensures that U-Boot can boot as a coreboot payload, based on a known-good build of coreboot. -To update the `coreboot.rom` file which is used: - -#. Build coreboot with `CONFIG_GENERIC_LINEAR_FRAMEBUFFER=y`. If using - `make menuconfig`, this is under - `Devices->Display->Framebuffer mode->Linear "high resolution" framebuffer`. - -#. Compress the resulting `coreboot.rom`:: - - xz -c /path/to/coreboot/build/coreboot.rom > coreboot.rom.xz - -#. Upload the file to Google drive - -#. Send a patch to change the file ID used by wget in the CI yaml files. +To update the `coreboot.rom` file which is used, see ``tools/Dockerfile`` -- cgit v1.3.1 From abb778038101c5d150c14027e39f7009afc30208 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 14 Oct 2024 16:32:12 -0600 Subject: x86: Enable RTC command by default The real-time clock is needed for most X86 systems and it is useful to be able to read from it. Enable the rtc command by default. Signed-off-by: Simon Glass --- cmd/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/cmd/Kconfig b/cmd/Kconfig index bff22b94de2..a4ca61c37dd 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -2291,6 +2291,7 @@ config CMD_DATE config CMD_RTC bool "rtc" depends on DM_RTC + default y if X86 help Enable the 'rtc' command for low-level access to RTC devices. -- cgit v1.3.1 From c222dda2da1283a859533c0cacca975f86524dc6 Mon Sep 17 00:00:00 2001 From: Andy Shevchenko Date: Sat, 5 Oct 2024 22:11:57 +0300 Subject: x86: cpu: Use default print_cpuinfo() for all MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Most of the copies of the print_cpuinfo() call the default method. Remove all of those in order to have only the default one when no `cpu` command is compiled. This also helps avoiding compiler warning, e.g.: arch/x86/cpu/tangier/tangier.c:23:5: warning: no previous prototype for ‘print_cpuinfo’ [-Wmissing-prototypes] Signed-off-by: Andy Shevchenko Reviewed-by: Simon Glass --- arch/x86/cpu/broadwell/cpu.c | 12 ------------ arch/x86/cpu/coreboot/coreboot.c | 5 ----- arch/x86/cpu/cpu.c | 8 +++++--- arch/x86/cpu/ivybridge/cpu.c | 14 -------------- arch/x86/cpu/qemu/qemu.c | 6 ------ arch/x86/cpu/quark/quark.c | 6 ------ arch/x86/cpu/x86_64/misc.c | 5 ----- arch/x86/include/asm/u-boot-x86.h | 1 - arch/x86/lib/fsp/fsp_common.c | 6 ------ 9 files changed, 5 insertions(+), 58 deletions(-) diff --git a/arch/x86/cpu/broadwell/cpu.c b/arch/x86/cpu/broadwell/cpu.c index 87463748c4d..8127d31e57e 100644 --- a/arch/x86/cpu/broadwell/cpu.c +++ b/arch/x86/cpu/broadwell/cpu.c @@ -88,18 +88,6 @@ int checkcpu(void) return 0; } -int print_cpuinfo(void) -{ - char processor_name[CPU_MAX_NAME_LEN]; - const char *name; - - /* Print processor name */ - name = cpu_get_name(processor_name); - printf("CPU: %s\n", name); - - return 0; -} - void board_debug_uart_init(void) { /* com1 / com2 decode range */ diff --git a/arch/x86/cpu/coreboot/coreboot.c b/arch/x86/cpu/coreboot/coreboot.c index b15715c935d..fa7430b436f 100644 --- a/arch/x86/cpu/coreboot/coreboot.c +++ b/arch/x86/cpu/coreboot/coreboot.c @@ -38,11 +38,6 @@ int arch_cpu_init(void) return 0; } -int checkcpu(void) -{ - return 0; -} - static void board_final_init(void) { /* diff --git a/arch/x86/cpu/cpu.c b/arch/x86/cpu/cpu.c index f8549e32092..40dd5ecb107 100644 --- a/arch/x86/cpu/cpu.c +++ b/arch/x86/cpu/cpu.c @@ -163,10 +163,11 @@ char *cpu_get_name(char *name) return ptr; } -int print_cpuinfo(void) __attribute__((weak, alias("default_print_cpuinfo"))); - -int default_print_cpuinfo(void) +#if !CONFIG_IS_ENABLED(CPU) +int print_cpuinfo(void) { + post_code(POST_CPU_INFO); + printf("CPU: %s, vendor %s, device %xh\n", cpu_has_64bit() ? "x86_64" : "x86", cpu_vendor_name(gd->arch.x86_vendor), gd->arch.x86_device); @@ -178,6 +179,7 @@ int default_print_cpuinfo(void) return 0; } +#endif #if CONFIG_IS_ENABLED(SHOW_BOOT_PROGRESS) void show_boot_progress(int val) diff --git a/arch/x86/cpu/ivybridge/cpu.c b/arch/x86/cpu/ivybridge/cpu.c index 05691a38d2e..d299068a879 100644 --- a/arch/x86/cpu/ivybridge/cpu.c +++ b/arch/x86/cpu/ivybridge/cpu.c @@ -182,20 +182,6 @@ int checkcpu(void) return 0; } -int print_cpuinfo(void) -{ - char processor_name[CPU_MAX_NAME_LEN]; - const char *name; - - /* Print processor name */ - name = cpu_get_name(processor_name); - printf("CPU: %s\n", name); - - post_code(POST_CPU_INFO); - - return 0; -} - void board_debug_uart_init(void) { /* This enables the debug UART */ diff --git a/arch/x86/cpu/qemu/qemu.c b/arch/x86/cpu/qemu/qemu.c index 262584d01f0..563f63e2bc8 100644 --- a/arch/x86/cpu/qemu/qemu.c +++ b/arch/x86/cpu/qemu/qemu.c @@ -109,12 +109,6 @@ int checkcpu(void) { return 0; } - -int print_cpuinfo(void) -{ - post_code(POST_CPU_INFO); - return default_print_cpuinfo(); -} #endif int arch_early_init_r(void) diff --git a/arch/x86/cpu/quark/quark.c b/arch/x86/cpu/quark/quark.c index fdf92b2c0c3..07504faaffb 100644 --- a/arch/x86/cpu/quark/quark.c +++ b/arch/x86/cpu/quark/quark.c @@ -266,12 +266,6 @@ int checkcpu(void) return 0; } -int print_cpuinfo(void) -{ - post_code(POST_CPU_INFO); - return default_print_cpuinfo(); -} - static void quark_pcie_init(void) { u32 val; diff --git a/arch/x86/cpu/x86_64/misc.c b/arch/x86/cpu/x86_64/misc.c index 294511e6eba..fc449ca4ed6 100644 --- a/arch/x86/cpu/x86_64/misc.c +++ b/arch/x86/cpu/x86_64/misc.c @@ -32,9 +32,4 @@ int checkcpu(void) { return 0; } - -int print_cpuinfo(void) -{ - return 0; -} #endif diff --git a/arch/x86/include/asm/u-boot-x86.h b/arch/x86/include/asm/u-boot-x86.h index 64139ead190..ed2f6aa3893 100644 --- a/arch/x86/include/asm/u-boot-x86.h +++ b/arch/x86/include/asm/u-boot-x86.h @@ -95,7 +95,6 @@ void x86_enable_caches(void); void x86_disable_caches(void); int x86_init_cache(void); phys_addr_t board_get_usable_ram_top(phys_size_t total_size); -int default_print_cpuinfo(void); /* Set up a UART which can be used with printch(), printhex8(), etc. */ int setup_internal_uart(int enable); diff --git a/arch/x86/lib/fsp/fsp_common.c b/arch/x86/lib/fsp/fsp_common.c index c47e6ca4738..7e4c1476634 100644 --- a/arch/x86/lib/fsp/fsp_common.c +++ b/arch/x86/lib/fsp/fsp_common.c @@ -26,12 +26,6 @@ int checkcpu(void) return 0; } -int print_cpuinfo(void) -{ - post_code(POST_CPU_INFO); - return default_print_cpuinfo(); -} - int fsp_init_phase_pci(void) { u32 status; -- cgit v1.3.1 From 2fb5cc2ad8155f9dc32e454bf01f051dc98b5684 Mon Sep 17 00:00:00 2001 From: Andy Shevchenko Date: Sat, 5 Oct 2024 22:11:58 +0300 Subject: x86: cpu: Mark a few functions static MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Some functions are not used anywhere except the same file where they are defined. Mark them static. This helps avoiding the compiler warnings: arch/x86/cpu/cpu.c:343:6: warning: no previous prototype for ‘detect_coreboot_table_at’ [-Wmissing-prototypes] arch/x86/cpu/mtrr.c:90:6: warning: no previous prototype for ‘mtrr_write_all’ [-Wmissing-prototypes] arch/x86/cpu/i386/interrupt.c:240:6: warning: no previous prototype for ‘__do_irq’ [-Wmissing-prototypes] Signed-off-by: Andy Shevchenko Reviewed-by: Simon Glass --- arch/x86/cpu/cpu.c | 2 +- arch/x86/cpu/i386/interrupt.c | 2 +- arch/x86/cpu/mtrr.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/arch/x86/cpu/cpu.c b/arch/x86/cpu/cpu.c index 40dd5ecb107..a8b21406ac0 100644 --- a/arch/x86/cpu/cpu.c +++ b/arch/x86/cpu/cpu.c @@ -340,7 +340,7 @@ int reserve_arch(void) } #endif -long detect_coreboot_table_at(ulong start, ulong size) +static long detect_coreboot_table_at(ulong start, ulong size) { u32 *ptr, *end; diff --git a/arch/x86/cpu/i386/interrupt.c b/arch/x86/cpu/i386/interrupt.c index b3f4214acdb..6f78b072cde 100644 --- a/arch/x86/cpu/i386/interrupt.c +++ b/arch/x86/cpu/i386/interrupt.c @@ -237,7 +237,7 @@ void *x86_get_idt(void) return &idt_ptr; } -void __do_irq(int irq) +static void __do_irq(int irq) { printf("Unhandled IRQ : %d\n", irq); } diff --git a/arch/x86/cpu/mtrr.c b/arch/x86/cpu/mtrr.c index 50cba5fb88d..07ea89162de 100644 --- a/arch/x86/cpu/mtrr.c +++ b/arch/x86/cpu/mtrr.c @@ -87,7 +87,7 @@ void mtrr_read_all(struct mtrr_info *info) } } -void mtrr_write_all(struct mtrr_info *info) +static void mtrr_write_all(struct mtrr_info *info) { int reg_count = mtrr_get_var_count(); struct mtrr_state state; -- cgit v1.3.1 From c1614a4ef676d55e2ec0a55b45391730ce22cdd7 Mon Sep 17 00:00:00 2001 From: Andy Shevchenko Date: Sat, 5 Oct 2024 22:11:59 +0300 Subject: x86: cpu: Add a few prototypes to the header file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The compiler is not happy to have no prototypes for the functions that are not defined static. Add them. This helps avoiding the compiler warnings: arch/x86/cpu/cpu.c:197:13: warning: no previous prototype for ‘board_final_init’ [-Wmissing-prototypes] arch/x86/cpu/cpu.c:205:13: warning: no previous prototype for ‘board_final_cleanup’ [-Wmissing-prototypes] arch/x86/cpu/cpu.c:307:5: warning: no previous prototype for ‘reserve_arch’ [-Wmissing-prototypes] Signed-off-by: Andy Shevchenko Reviewed-by: Simon Glass --- arch/x86/include/asm/cpu.h | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/arch/x86/include/asm/cpu.h b/arch/x86/include/asm/cpu.h index 87e0c6f12b6..8c1ef4c8cc1 100644 --- a/arch/x86/include/asm/cpu.h +++ b/arch/x86/include/asm/cpu.h @@ -293,4 +293,11 @@ u32 cpu_get_stepping(void); */ int cpu_phys_address_size(void); +void board_final_init(void); +void board_final_cleanup(void); + +#ifndef CONFIG_EFI_STUB +int reserve_arch(void); +#endif + #endif -- cgit v1.3.1 From 44917d586657eeae0401bc29af80011a264002e7 Mon Sep 17 00:00:00 2001 From: Andy Shevchenko Date: Sat, 5 Oct 2024 22:12:00 +0300 Subject: x86: cpu: Add missing header inclusion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Without asm/cpu_x86.h inclusion a compiler is not happy: arch/x86/cpu/cpu_x86.c:14:5: warning: no previous prototype for ‘cpu_x86_bind’ [-Wmissing-prototypes] arch/x86/cpu/cpu_x86.c:29:5: warning: no previous prototype for ‘cpu_x86_get_vendor’ [-Wmissing-prototypes] arch/x86/cpu/cpu_x86.c:41:5: warning: no previous prototype for ‘cpu_x86_get_desc’ [-Wmissing-prototypes] arch/x86/cpu/cpu_x86.c:55:5: warning: no previous prototype for ‘cpu_x86_get_count’ [-Wmissing-prototypes] Add missing header inclusion. Signed-off-by: Andy Shevchenko Reviewed-by: Simon Glass --- arch/x86/cpu/cpu_x86.c | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/x86/cpu/cpu_x86.c b/arch/x86/cpu/cpu_x86.c index 6c53f0ea821..6c32ae499df 100644 --- a/arch/x86/cpu/cpu_x86.c +++ b/arch/x86/cpu/cpu_x86.c @@ -7,6 +7,7 @@ #include #include #include +#include #include DECLARE_GLOBAL_DATA_PTR; -- cgit v1.3.1