Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44023 )
Change subject: arch/x86/smbios.c: Split out weak functions ......................................................................
arch/x86/smbios.c: Split out weak functions
The `smbios.c` file is rather long. To improve navigability, place weak function definitions on a separate compilation unit.
Change-Id: Idd2a4ba52b6b23aad8fd63e66ffa747d49ea713d Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/arch/x86/Makefile.inc M src/arch/x86/smbios.c A src/arch/x86/smbios_weak.c 3 files changed, 140 insertions(+), 132 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/44023/1
diff --git a/src/arch/x86/Makefile.inc b/src/arch/x86/Makefile.inc index 00690ba0..f652ce1 100644 --- a/src/arch/x86/Makefile.inc +++ b/src/arch/x86/Makefile.inc @@ -255,6 +255,7 @@ ramstage-$(CONFIG_GENERATE_PIRQ_TABLE) += pirq_routing.c ramstage-y += rdrand.c ramstage-$(CONFIG_GENERATE_SMBIOS_TABLES) += smbios.c +ramstage-$(CONFIG_GENERATE_SMBIOS_TABLES) += smbios_weak.c ramstage-y += tables.c ramstage-$(CONFIG_COOP_MULTITASKING) += thread.c ramstage-$(CONFIG_COOP_MULTITASKING) += thread_switch.S diff --git a/src/arch/x86/smbios.c b/src/arch/x86/smbios.c index b572624..b2da33c 100644 --- a/src/arch/x86/smbios.c +++ b/src/arch/x86/smbios.c @@ -206,18 +206,6 @@ t->manufacturer = smbios_add_string(t->eos, string_buffer); } } -/* this function will fill the corresponding locator */ -__weak void smbios_fill_dimm_locator(const struct dimm_info *dimm, struct smbios_type17 *t) -{ - char locator[40]; - - snprintf(locator, sizeof(locator), "Channel-%d-DIMM-%d", - dimm->channel_num, dimm->dimm_num); - t->device_locator = smbios_add_string(t->eos, locator); - - snprintf(locator, sizeof(locator), "BANK %d", dimm->bank_locator); - t->bank_locator = smbios_add_string(t->eos, locator); -}
static void trim_trailing_whitespace(char *buffer, size_t buffer_size) { @@ -405,11 +393,6 @@ return coreboot_version; }
-__weak const char *smbios_mainboard_bios_version(void) -{ - return NULL; -} - static int smbios_write_type0(unsigned long *current, int handle) { struct smbios_type0 *t = (struct smbios_type0 *)*current; @@ -459,121 +442,6 @@ return len; }
-__weak const char *smbios_mainboard_serial_number(void) -{ - return CONFIG_MAINBOARD_SERIAL_NUMBER; -} - -__weak const char *smbios_mainboard_version(void) -{ - return CONFIG_MAINBOARD_VERSION; -} - -__weak const char *smbios_mainboard_manufacturer(void) -{ - return CONFIG_MAINBOARD_SMBIOS_MANUFACTURER; -} - -__weak const char *smbios_mainboard_product_name(void) -{ - return CONFIG_MAINBOARD_SMBIOS_PRODUCT_NAME; -} - -__weak const char *smbios_mainboard_asset_tag(void) -{ - return ""; -} - -__weak u8 smbios_mainboard_feature_flags(void) -{ - return 0; -} - -__weak const char *smbios_mainboard_location_in_chassis(void) -{ - return ""; -} - -__weak smbios_board_type smbios_mainboard_board_type(void) -{ - return SMBIOS_BOARD_TYPE_UNKNOWN; -} - -/* - * System Enclosure or Chassis Types as defined in SMBIOS specification. - * The default value is SMBIOS_ENCLOSURE_DESKTOP (0x03) but laptop, - * convertible, or tablet enclosure will be used if the appropriate - * system type is selected. - */ -__weak smbios_enclosure_type smbios_mainboard_enclosure_type(void) -{ - if (CONFIG(SYSTEM_TYPE_LAPTOP)) - return SMBIOS_ENCLOSURE_LAPTOP; - else if (CONFIG(SYSTEM_TYPE_TABLET)) - return SMBIOS_ENCLOSURE_TABLET; - else if (CONFIG(SYSTEM_TYPE_CONVERTIBLE)) - return SMBIOS_ENCLOSURE_CONVERTIBLE; - else if (CONFIG(SYSTEM_TYPE_DETACHABLE)) - return SMBIOS_ENCLOSURE_DETACHABLE; - else - return SMBIOS_ENCLOSURE_DESKTOP; -} - -__weak const char *smbios_system_serial_number(void) -{ - return smbios_mainboard_serial_number(); -} - -__weak const char *smbios_system_version(void) -{ - return smbios_mainboard_version(); -} - -__weak const char *smbios_system_manufacturer(void) -{ - return smbios_mainboard_manufacturer(); -} - -__weak const char *smbios_system_product_name(void) -{ - return smbios_mainboard_product_name(); -} - -__weak void smbios_system_set_uuid(u8 *uuid) -{ - /* leave all zero */ -} - -__weak unsigned int smbios_cpu_get_max_speed_mhz(void) -{ - return 0; /* Unknown */ -} - -__weak unsigned int smbios_cpu_get_current_speed_mhz(void) -{ - return 0; /* Unknown */ -} - -__weak const char *smbios_system_sku(void) -{ - return ""; -} - -__weak const char *smbios_chassis_version(void) -{ - return ""; -} - -__weak const char *smbios_chassis_serial_number(void) -{ - return ""; -} - -__weak const char *smbios_processor_serial_number(void) -{ - return ""; -} - static int get_socket_type(void) { if (CONFIG(CPU_INTEL_SLOT_1)) diff --git a/src/arch/x86/smbios_weak.c b/src/arch/x86/smbios_weak.c new file mode 100644 index 0000000..22ce7a5 --- /dev/null +++ b/src/arch/x86/smbios_weak.c @@ -0,0 +1,139 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <memory_info.h> +#include <smbios.h> +#include <stdint.h> +#include <string.h> + +/* this function will fill the corresponding locator */ +__weak void smbios_fill_dimm_locator(const struct dimm_info *dimm, struct smbios_type17 *t) +{ + char locator[40]; + + snprintf(locator, sizeof(locator), "Channel-%d-DIMM-%d", + dimm->channel_num, dimm->dimm_num); + t->device_locator = smbios_add_string(t->eos, locator); + + snprintf(locator, sizeof(locator), "BANK %d", dimm->bank_locator); + t->bank_locator = smbios_add_string(t->eos, locator); +} + +__weak const char *smbios_mainboard_bios_version(void) +{ + return NULL; +} + +__weak const char *smbios_mainboard_serial_number(void) +{ + return CONFIG_MAINBOARD_SERIAL_NUMBER; +} + +__weak const char *smbios_mainboard_version(void) +{ + return CONFIG_MAINBOARD_VERSION; +} + +__weak const char *smbios_mainboard_manufacturer(void) +{ + return CONFIG_MAINBOARD_SMBIOS_MANUFACTURER; +} + +__weak const char *smbios_mainboard_product_name(void) +{ + return CONFIG_MAINBOARD_SMBIOS_PRODUCT_NAME; +} + +__weak const char *smbios_mainboard_asset_tag(void) +{ + return ""; +} + +__weak u8 smbios_mainboard_feature_flags(void) +{ + return 0; +} + +__weak const char *smbios_mainboard_location_in_chassis(void) +{ + return ""; +} + +__weak smbios_board_type smbios_mainboard_board_type(void) +{ + return SMBIOS_BOARD_TYPE_UNKNOWN; +} + +/* + * System Enclosure or Chassis Types as defined in SMBIOS specification. + * The default value is SMBIOS_ENCLOSURE_DESKTOP (0x03) but laptop, + * convertible, or tablet enclosure will be used if the appropriate + * system type is selected. + */ +__weak smbios_enclosure_type smbios_mainboard_enclosure_type(void) +{ + if (CONFIG(SYSTEM_TYPE_LAPTOP)) + return SMBIOS_ENCLOSURE_LAPTOP; + else if (CONFIG(SYSTEM_TYPE_TABLET)) + return SMBIOS_ENCLOSURE_TABLET; + else if (CONFIG(SYSTEM_TYPE_CONVERTIBLE)) + return SMBIOS_ENCLOSURE_CONVERTIBLE; + else if (CONFIG(SYSTEM_TYPE_DETACHABLE)) + return SMBIOS_ENCLOSURE_DETACHABLE; + else + return SMBIOS_ENCLOSURE_DESKTOP; +} + +__weak const char *smbios_system_serial_number(void) +{ + return smbios_mainboard_serial_number(); +} + +__weak const char *smbios_system_version(void) +{ + return smbios_mainboard_version(); +} + +__weak const char *smbios_system_manufacturer(void) +{ + return smbios_mainboard_manufacturer(); +} + +__weak const char *smbios_system_product_name(void) +{ + return smbios_mainboard_product_name(); +} + +__weak void smbios_system_set_uuid(u8 *uuid) +{ + /* leave all zero */ +} + +__weak unsigned int smbios_cpu_get_max_speed_mhz(void) +{ + return 0; /* Unknown */ +} + +__weak unsigned int smbios_cpu_get_current_speed_mhz(void) +{ + return 0; /* Unknown */ +} + +__weak const char *smbios_system_sku(void) +{ + return ""; +} + +__weak const char *smbios_chassis_version(void) +{ + return ""; +} + +__weak const char *smbios_chassis_serial_number(void) +{ + return ""; +} + +__weak const char *smbios_processor_serial_number(void) +{ + return ""; +}
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44023 )
Change subject: arch/x86/smbios.c: Split out weak functions ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44023/1/src/arch/x86/smbios_weak.c File src/arch/x86/smbios_weak.c:
PS1: suggestion: the filename `weak` is a little difficult to understand at a glance, what about smbios_defaults.c ?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44023 )
Change subject: arch/x86/smbios.c: Split out weak functions ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44023/1/src/arch/x86/smbios_weak.c File src/arch/x86/smbios_weak.c:
PS1:
suggestion: the filename `weak` is a little difficult to understand at a glance, what about smbios_d […]
Sounds good.
Hello build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44023
to look at the new patch set (#2).
Change subject: arch/x86/smbios.c: Split out weak functions ......................................................................
arch/x86/smbios.c: Split out weak functions
The `smbios.c` file is rather long. To improve navigability, place weak function definitions on a separate compilation unit.
Change-Id: Idd2a4ba52b6b23aad8fd63e66ffa747d49ea713d Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/arch/x86/Makefile.inc M src/arch/x86/smbios.c A src/arch/x86/smbios_defaults.c 3 files changed, 140 insertions(+), 132 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/44023/2
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44023 )
Change subject: arch/x86/smbios.c: Split out weak functions ......................................................................
Patch Set 2: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44023 )
Change subject: arch/x86/smbios.c: Split out weak functions ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44023/1/src/arch/x86/smbios_weak.c File src/arch/x86/smbios_weak.c:
PS1:
Sounds good.
Done
Angel Pons has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44023 )
Change subject: arch/x86/smbios.c: Split out weak functions ......................................................................
arch/x86/smbios.c: Split out weak functions
The `smbios.c` file is rather long. To improve navigability, place weak function definitions on a separate compilation unit.
Change-Id: Idd2a4ba52b6b23aad8fd63e66ffa747d49ea713d Signed-off-by: Angel Pons th3fanbus@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/44023 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/arch/x86/Makefile.inc M src/arch/x86/smbios.c A src/arch/x86/smbios_defaults.c 3 files changed, 140 insertions(+), 132 deletions(-)
Approvals: build bot (Jenkins): Verified Tim Wawrzynczak: Looks good to me, approved
diff --git a/src/arch/x86/Makefile.inc b/src/arch/x86/Makefile.inc index 00690ba0..17ab3dc 100644 --- a/src/arch/x86/Makefile.inc +++ b/src/arch/x86/Makefile.inc @@ -255,6 +255,7 @@ ramstage-$(CONFIG_GENERATE_PIRQ_TABLE) += pirq_routing.c ramstage-y += rdrand.c ramstage-$(CONFIG_GENERATE_SMBIOS_TABLES) += smbios.c +ramstage-$(CONFIG_GENERATE_SMBIOS_TABLES) += smbios_defaults.c ramstage-y += tables.c ramstage-$(CONFIG_COOP_MULTITASKING) += thread.c ramstage-$(CONFIG_COOP_MULTITASKING) += thread_switch.S diff --git a/src/arch/x86/smbios.c b/src/arch/x86/smbios.c index 1b51d90..8c7f92c 100644 --- a/src/arch/x86/smbios.c +++ b/src/arch/x86/smbios.c @@ -205,18 +205,6 @@ t->manufacturer = smbios_add_string(t->eos, string_buffer); } } -/* this function will fill the corresponding locator */ -__weak void smbios_fill_dimm_locator(const struct dimm_info *dimm, struct smbios_type17 *t) -{ - char locator[40]; - - snprintf(locator, sizeof(locator), "Channel-%d-DIMM-%d", - dimm->channel_num, dimm->dimm_num); - t->device_locator = smbios_add_string(t->eos, locator); - - snprintf(locator, sizeof(locator), "BANK %d", dimm->bank_locator); - t->bank_locator = smbios_add_string(t->eos, locator); -}
static void trim_trailing_whitespace(char *buffer, size_t buffer_size) { @@ -404,11 +392,6 @@ return coreboot_version; }
-__weak const char *smbios_mainboard_bios_version(void) -{ - return NULL; -} - static int smbios_write_type0(unsigned long *current, int handle) { struct smbios_type0 *t = (struct smbios_type0 *)*current; @@ -458,121 +441,6 @@ return len; }
-__weak const char *smbios_mainboard_serial_number(void) -{ - return CONFIG_MAINBOARD_SERIAL_NUMBER; -} - -__weak const char *smbios_mainboard_version(void) -{ - return CONFIG_MAINBOARD_VERSION; -} - -__weak const char *smbios_mainboard_manufacturer(void) -{ - return CONFIG_MAINBOARD_SMBIOS_MANUFACTURER; -} - -__weak const char *smbios_mainboard_product_name(void) -{ - return CONFIG_MAINBOARD_SMBIOS_PRODUCT_NAME; -} - -__weak const char *smbios_mainboard_asset_tag(void) -{ - return ""; -} - -__weak u8 smbios_mainboard_feature_flags(void) -{ - return 0; -} - -__weak const char *smbios_mainboard_location_in_chassis(void) -{ - return ""; -} - -__weak smbios_board_type smbios_mainboard_board_type(void) -{ - return SMBIOS_BOARD_TYPE_UNKNOWN; -} - -/* - * System Enclosure or Chassis Types as defined in SMBIOS specification. - * The default value is SMBIOS_ENCLOSURE_DESKTOP (0x03) but laptop, - * convertible, or tablet enclosure will be used if the appropriate - * system type is selected. - */ -__weak smbios_enclosure_type smbios_mainboard_enclosure_type(void) -{ - if (CONFIG(SYSTEM_TYPE_LAPTOP)) - return SMBIOS_ENCLOSURE_LAPTOP; - else if (CONFIG(SYSTEM_TYPE_TABLET)) - return SMBIOS_ENCLOSURE_TABLET; - else if (CONFIG(SYSTEM_TYPE_CONVERTIBLE)) - return SMBIOS_ENCLOSURE_CONVERTIBLE; - else if (CONFIG(SYSTEM_TYPE_DETACHABLE)) - return SMBIOS_ENCLOSURE_DETACHABLE; - else - return SMBIOS_ENCLOSURE_DESKTOP; -} - -__weak const char *smbios_system_serial_number(void) -{ - return smbios_mainboard_serial_number(); -} - -__weak const char *smbios_system_version(void) -{ - return smbios_mainboard_version(); -} - -__weak const char *smbios_system_manufacturer(void) -{ - return smbios_mainboard_manufacturer(); -} - -__weak const char *smbios_system_product_name(void) -{ - return smbios_mainboard_product_name(); -} - -__weak void smbios_system_set_uuid(u8 *uuid) -{ - /* leave all zero */ -} - -__weak unsigned int smbios_cpu_get_max_speed_mhz(void) -{ - return 0; /* Unknown */ -} - -__weak unsigned int smbios_cpu_get_current_speed_mhz(void) -{ - return 0; /* Unknown */ -} - -__weak const char *smbios_system_sku(void) -{ - return ""; -} - -__weak const char *smbios_chassis_version(void) -{ - return ""; -} - -__weak const char *smbios_chassis_serial_number(void) -{ - return ""; -} - -__weak const char *smbios_processor_serial_number(void) -{ - return ""; -} - static int get_socket_type(void) { if (CONFIG(CPU_INTEL_SLOT_1)) diff --git a/src/arch/x86/smbios_defaults.c b/src/arch/x86/smbios_defaults.c new file mode 100644 index 0000000..22ce7a5 --- /dev/null +++ b/src/arch/x86/smbios_defaults.c @@ -0,0 +1,139 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <memory_info.h> +#include <smbios.h> +#include <stdint.h> +#include <string.h> + +/* this function will fill the corresponding locator */ +__weak void smbios_fill_dimm_locator(const struct dimm_info *dimm, struct smbios_type17 *t) +{ + char locator[40]; + + snprintf(locator, sizeof(locator), "Channel-%d-DIMM-%d", + dimm->channel_num, dimm->dimm_num); + t->device_locator = smbios_add_string(t->eos, locator); + + snprintf(locator, sizeof(locator), "BANK %d", dimm->bank_locator); + t->bank_locator = smbios_add_string(t->eos, locator); +} + +__weak const char *smbios_mainboard_bios_version(void) +{ + return NULL; +} + +__weak const char *smbios_mainboard_serial_number(void) +{ + return CONFIG_MAINBOARD_SERIAL_NUMBER; +} + +__weak const char *smbios_mainboard_version(void) +{ + return CONFIG_MAINBOARD_VERSION; +} + +__weak const char *smbios_mainboard_manufacturer(void) +{ + return CONFIG_MAINBOARD_SMBIOS_MANUFACTURER; +} + +__weak const char *smbios_mainboard_product_name(void) +{ + return CONFIG_MAINBOARD_SMBIOS_PRODUCT_NAME; +} + +__weak const char *smbios_mainboard_asset_tag(void) +{ + return ""; +} + +__weak u8 smbios_mainboard_feature_flags(void) +{ + return 0; +} + +__weak const char *smbios_mainboard_location_in_chassis(void) +{ + return ""; +} + +__weak smbios_board_type smbios_mainboard_board_type(void) +{ + return SMBIOS_BOARD_TYPE_UNKNOWN; +} + +/* + * System Enclosure or Chassis Types as defined in SMBIOS specification. + * The default value is SMBIOS_ENCLOSURE_DESKTOP (0x03) but laptop, + * convertible, or tablet enclosure will be used if the appropriate + * system type is selected. + */ +__weak smbios_enclosure_type smbios_mainboard_enclosure_type(void) +{ + if (CONFIG(SYSTEM_TYPE_LAPTOP)) + return SMBIOS_ENCLOSURE_LAPTOP; + else if (CONFIG(SYSTEM_TYPE_TABLET)) + return SMBIOS_ENCLOSURE_TABLET; + else if (CONFIG(SYSTEM_TYPE_CONVERTIBLE)) + return SMBIOS_ENCLOSURE_CONVERTIBLE; + else if (CONFIG(SYSTEM_TYPE_DETACHABLE)) + return SMBIOS_ENCLOSURE_DETACHABLE; + else + return SMBIOS_ENCLOSURE_DESKTOP; +} + +__weak const char *smbios_system_serial_number(void) +{ + return smbios_mainboard_serial_number(); +} + +__weak const char *smbios_system_version(void) +{ + return smbios_mainboard_version(); +} + +__weak const char *smbios_system_manufacturer(void) +{ + return smbios_mainboard_manufacturer(); +} + +__weak const char *smbios_system_product_name(void) +{ + return smbios_mainboard_product_name(); +} + +__weak void smbios_system_set_uuid(u8 *uuid) +{ + /* leave all zero */ +} + +__weak unsigned int smbios_cpu_get_max_speed_mhz(void) +{ + return 0; /* Unknown */ +} + +__weak unsigned int smbios_cpu_get_current_speed_mhz(void) +{ + return 0; /* Unknown */ +} + +__weak const char *smbios_system_sku(void) +{ + return ""; +} + +__weak const char *smbios_chassis_version(void) +{ + return ""; +} + +__weak const char *smbios_chassis_serial_number(void) +{ + return ""; +} + +__weak const char *smbios_processor_serial_number(void) +{ + return ""; +}
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44023 )
Change subject: arch/x86/smbios.c: Split out weak functions ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44023/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44023/3//COMMIT_MSG@9 PS3, Line 9: To improve navigability For my workflow it has the exact opposite effect. Also I have to grep now for functions that were in the same spot for a decade or so. I don't understand why we should allow such changes.
Angel Pons has created a revert of this change. ( https://review.coreboot.org/c/coreboot/+/44023 )
Change subject: arch/x86/smbios.c: Split out weak functions ......................................................................
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44023 )
Change subject: arch/x86/smbios.c: Split out weak functions ......................................................................
Patch Set 3:
Created a revert of this change as Ia09f25e64dfc46d230e7ec6faf335b1d04ee6f78
Sorry, I did not mean to provoke this. IMHO, a revert (after 2 months+) has about the same traits as the original patch: I couldn't decide for all affected developers if that makes it better.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44023 )
Change subject: arch/x86/smbios.c: Split out weak functions ......................................................................
Patch Set 3:
Patch Set 3:
Created a revert of this change as Ia09f25e64dfc46d230e7ec6faf335b1d04ee6f78
Sorry, I did not mean to provoke this. IMHO, a revert (after 2 months+) has about the same traits as the original patch: I couldn't decide for all affected developers if that makes it better.
For the record: I deleted the revert.
-
Angel Pons (Code Review) -
Nico Huber (Code Review)
-
Tim Wawrzynczak (Code Review)