Change in coreboot[master]: src/mainboard/ocp/monolake: Create SMBIOS type 16 for Monolake palatfrom
Name of user not set #1002648 has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36764 )
Change subject: src/mainboard/ocp/monolake: Create SMBIOS type 16 for Monolake palatfrom ......................................................................
src/mainboard/ocp/monolake: Create SMBIOS type 16 for Monolake palatfrom
Create SMBIOS type 16 for Monolake palatfrom, but the ECC setting can`t be confirmed in FSP, so hardcoded the Memory Error Correction field.
Change-Id: I821f2c9d6833478d4a6676d93832529295cfb53b Signed-off-by: Morgan Jang Morgan_Jang@wiwynn.com --- M src/mainboard/ocp/monolake/mainboard.c 1 file changed, 136 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/36764/1
diff --git a/src/mainboard/ocp/monolake/mainboard.c b/src/mainboard/ocp/monolake/mainboard.c index dffd19f..7a4c022 100644 --- a/src/mainboard/ocp/monolake/mainboard.c +++ b/src/mainboard/ocp/monolake/mainboard.c @@ -95,3 +95,139 @@ dimm->dimm_num); t->bank_locator = smbios_add_string(t->eos, locator); } +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2007-2009 coresystems GmbH + * Copyright (C) 2011 Google Inc. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <device/device.h> +#include <pc80/mc146818rtc.h> +#include <cf9_reset.h> +#include <smbios.h> +#include <string.h> +#include <drivers/vpd/vpd.h> +#include <console/console.h> +#include <drivers/ipmi/ipmi_ops.h> +#include "ipmi.h" +/* VPD variable for enabling/disabling FRB2 timer. */ +#define FRB2_TIMER "FRB2_TIMER" +/* VPD variable for setting FRB2 timer countdown value. */ +#define FRB2_COUNTDOWN "FRB2_COUNTDOWN" +#define VPD_LEN 10 +/* Default countdown is 15 minutes. */ +#define DEFAULT_COUNTDOWN 9000 +#define MAX_IMC 1 +#define MAX_DIMM_SIZE 32 //in GB + +static void init_frb2_wdt(void) +{ + + char val[VPD_LEN]; + /* Enable FRB2 timer by default. */ + u8 enable = 1; + uint16_t countdown; + + if (vpd_get_bool(FRB2_TIMER, VPD_RW, &enable)) { + if (!enable) { + printk(BIOS_DEBUG, "Disable FRB2 timer\n"); + ipmi_stop_bmc_wdt(BMC_KCS_BASE); + } + } + if (enable) { + if (vpd_gets(FRB2_COUNTDOWN, val, VPD_LEN, VPD_RW)) { + countdown = (uint16_t)atol(val); + printk(BIOS_DEBUG, "FRB2 timer countdown set to: %d\n", + countdown); + } else { + printk(BIOS_DEBUG, "FRB2 timer use default value: %d\n", + DEFAULT_COUNTDOWN); + countdown = DEFAULT_COUNTDOWN; + } + ipmi_init_and_start_bmc_wdt(BMC_KCS_BASE, countdown, + TIMEOUT_HARD_RESET); + } +} + +#if CONFIG(GENERATE_SMBIOS_TABLES) +static int write_smbios_type16(struct device *dev, int *handle, unsigned long *current) +{ + struct smbios_type16 *t = (struct smbios_type16 *)*current; + u32 maximum_capacity; + int len = sizeof(struct smbios_type16); + + printk(BIOS_DEBUG, "Create SMBIOS type 16\n"); + + memset(t, 0, sizeof(struct smbios_type16)); + t->type = SMBIOS_PHYS_MEMORY_ARRAY; + t->location = MEMORY_ARRAY_LOCATION_SYSTEM_BOARD; + t->use = MEMORY_ARRAY_USE_SYSTEM; + t->memory_error_correction = MEMORY_ARRAY_ECC_NONE; //The ECC setting can`t be confirmed in FSP, so hardcode it. + t->memory_error_information_handle = 0xFFFE; + t->number_of_memory_devices = CONFIG_DIMM_MAX/MAX_IMC; + + maximum_capacity = (u32)(CONFIG_DIMM_MAX * MAX_DIMM_SIZE) << 20; + if (maximum_capacity >= 0x80000000) { + t->maximum_capacity = 0x80000000; + t->extended_maximum_capacity = maximum_capacity << 10; + } else { + t->maximum_capacity = (u32)maximum_capacity; + t->extended_maximum_capacity = 0; + } + + *current += len; + t->handle = *handle; + *handle += 1; + t->length = len - 2; + return len; +} +#endif + +/* + * mainboard_enable is executed as first thing after enumerate_buses(). + * This is the earliest point to add customization. + */ +static void mainboard_enable(struct device *dev) +{ + ipmi_oem_rsp_t rsp; + + init_frb2_wdt(); + if (is_ipmi_clear_cmos_set(&rsp)) { + /* TODO: Should also try to restore CMOS to cmos.default + * if USE_OPTION_TABLE is set */ + cmos_init(1); + clear_ipmi_flags(&rsp); + system_reset(); + } +#if CONFIG(GENERATE_SMBIOS_TABLES) + dev->ops->get_smbios_data = write_smbios_type16; +#endif +} + +struct chip_operations mainboard_ops = { + .enable_dev = mainboard_enable, +}; + +void smbios_fill_dimm_locator(const struct dimm_info *dimm, struct smbios_type17 *t) +{ + + char locator[64] = {0}; + + snprintf(locator, sizeof(locator), "DIMM_%c%u", 'A' + dimm->channel_num, + dimm->dimm_num); + t->device_locator = smbios_add_string(t->eos, locator); + + snprintf(locator, sizeof(locator), "_Node0_Channel%d_Dimm%d", dimm->channel_num, + dimm->dimm_num); + t->bank_locator = smbios_add_string(t->eos, locator); +}
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36764 )
Change subject: src/mainboard/ocp/monolake: Create SMBIOS type 16 for Monolake palatfrom ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36764/1/src/mainboard/ocp/monolake/... File src/mainboard/ocp/monolake/mainboard.c:
https://review.coreboot.org/c/coreboot/+/36764/1/src/mainboard/ocp/monolake/... PS1, Line 175: t->memory_error_correction = MEMORY_ARRAY_ECC_NONE; //The ECC setting can`t be confirmed in FSP, so hardcode it. line over 96 characters
Hello Jingle Hsu, Patrick Rudolph, Jonathan Zhang, Johnny Lin, David Hendricks, Philipp Deppenwiese, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36764
to look at the new patch set (#2).
Change subject: src/mainboard/ocp/monolake: Create SMBIOS type 16 for Monolake palatfrom ......................................................................
src/mainboard/ocp/monolake: Create SMBIOS type 16 for Monolake palatfrom
Create SMBIOS type 16 for Monolake palatfrom, but the ECC setting can`t be confirmed in FSP, so hardcoded the Memory Error Correction field.
Change-Id: I821f2c9d6833478d4a6676d93832529295cfb53b Signed-off-by: Morgan Jang Morgan_Jang@wiwynn.com --- M src/mainboard/ocp/monolake/mainboard.c 1 file changed, 137 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/36764/2
Hello Jingle Hsu, Patrick Rudolph, Jonathan Zhang, Johnny Lin, David Hendricks, Philipp Deppenwiese, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36764
to look at the new patch set (#3).
Change subject: src/mainboard/ocp/monolake: Create SMBIOS type 16 for Monolake platform ......................................................................
src/mainboard/ocp/monolake: Create SMBIOS type 16 for Monolake platform
Create SMBIOS type 16 for Monolake platform, but the memory ECC setting can`t be enabled by the FSP, so it`s hardcoded to MEMORY_ARRAY_ECC_NONE.
Change-Id: I821f2c9d6833478d4a6676d93832529295cfb53b Signed-off-by: Morgan Jang Morgan_Jang@wiwynn.com --- M src/mainboard/ocp/monolake/mainboard.c 1 file changed, 41 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/36764/3
David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36764 )
Change subject: src/mainboard/ocp/monolake: Create SMBIOS type 16 for Monolake platform ......................................................................
Patch Set 3: Code-Review-1
(4 comments)
Mostly looks good - The only major thing to change is to I think we need to set the array type to MEMORY_ARRAY_ECC_SINGLE_BIT
https://review.coreboot.org/c/coreboot/+/36764/3/src/mainboard/ocp/monolake/... File src/mainboard/ocp/monolake/mainboard.c:
https://review.coreboot.org/c/coreboot/+/36764/3/src/mainboard/ocp/monolake/... PS3, Line 34: //in GB Coding style in coreboot generally follows Linux, so we use /* */ for comments.
https://review.coreboot.org/c/coreboot/+/36764/3/src/mainboard/ocp/monolake/... PS3, Line 78: //The ECC setting can`t be confirmed in FSP, so hardcode it. comment style
https://review.coreboot.org/c/coreboot/+/36764/3/src/mainboard/ocp/monolake/... PS3, Line 79: MEMORY_ARRAY_ECC_NONE If we're hardcoding it, should it be hardcoded to MEMORY_ARRAY_ECC_SINGLE_BIT?
https://review.coreboot.org/c/coreboot/+/36764/3/src/mainboard/ocp/monolake/... PS3, Line 83: (u32)(CONFIG_DIMM_MAX * MAX_DIMM_SIZE) << 20; Suggestion: Since MAX_DIMM_SIZE is only used here, it might be more obvious to define it as: #define MAX_DIMM_SIZE_GB (32UL << 20)
That way you can avoid the casting here, and this becomes: maximum_capacity = CONFIG_DIMM_MAX * MAX_DIMM_SIZE
which is more obvious
Hello Jingle Hsu, Patrick Rudolph, Jonathan Zhang, Johnny Lin, David Hendricks, Philipp Deppenwiese, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36764
to look at the new patch set (#5).
Change subject: src/mainboard/ocp/monolake: Create SMBIOS type 16 for Monolake platform ......................................................................
src/mainboard/ocp/monolake: Create SMBIOS type 16 for Monolake platform
Create SMBIOS type 16 for Monolake platform, but the memory ECC setting can`t be enabled by the FSP, so it`s hardcoded to MEMORY_ARRAY_ECC_SINGLE_BIT.
Change-Id: I821f2c9d6833478d4a6676d93832529295cfb53b Signed-off-by: Morgan Jang Morgan_Jang@wiwynn.com --- M src/mainboard/ocp/monolake/mainboard.c 1 file changed, 41 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/36764/5
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36764 )
Change subject: src/mainboard/ocp/monolake: Create SMBIOS type 16 for Monolake platform ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36764/6/src/mainboard/ocp/monolake/... File src/mainboard/ocp/monolake/mainboard.c:
https://review.coreboot.org/c/coreboot/+/36764/6/src/mainboard/ocp/monolake/... PS6, Line 81: / Add spaces around operators.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36764 )
Change subject: src/mainboard/ocp/monolake: Create SMBIOS type 16 for Monolake platform ......................................................................
Patch Set 6:
(7 comments)
https://review.coreboot.org/c/coreboot/+/36764/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36764/6//COMMIT_MSG@7 PS6, Line 7: src/mainboard/ocp/monolake: Create SMBIOS type 16 for Monolake platform Please remove *src* from the prefix, and you can abbreviate *mainboard*.
mb/ocp/monolake: …
https://review.coreboot.org/c/coreboot/+/36764/6//COMMIT_MSG@9 PS6, Line 9: can`t Wrong apostrophe: can’t or can't
https://review.coreboot.org/c/coreboot/+/36764/6//COMMIT_MSG@10 PS6, Line 10: it`s Ditto.
https://review.coreboot.org/c/coreboot/+/36764/6/src/mainboard/ocp/monolake/... File src/mainboard/ocp/monolake/mainboard.c:
https://review.coreboot.org/c/coreboot/+/36764/6/src/mainboard/ocp/monolake/... PS6, Line 33: 1 One space?
https://review.coreboot.org/c/coreboot/+/36764/6/src/mainboard/ocp/monolake/... PS6, Line 34: #define MAX_DIMM_SIZE_GB (32UL << 20) We have a macro MiB: 32 * MiB.
https://review.coreboot.org/c/coreboot/+/36764/6/src/mainboard/ocp/monolake/... PS6, Line 72: printk(BIOS_INFO, "Create SMBIOS type 16\n"); Is it used like this in the other parts of the code?
Creating SMBIOS tables type 16 (note, ECC information is hard-coded) …
without line break and in the end
printk(BIOS_INFO, "done\n");
https://review.coreboot.org/c/coreboot/+/36764/6/src/mainboard/ocp/monolake/... PS6, Line 117: #if CONFIG(GENERATE_SMBIOS_TABLES) Can’t you use the C if statement and include the new function without getting an unused function error?
if (CONFIG(GENERATE_SMBIOS_TABLES)) dev->ops->get_smbios_data = write_smbios_type16;
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36764 )
Change subject: src/mainboard/ocp/monolake: Create SMBIOS type 16 for Monolake platform ......................................................................
Patch Set 6:
I'd prefer a common driver for type16 entries. In the last few weeks everybody seem to duplicate those table creation code in mainboard directory. In the end you only need the maximum slot count, DIMM size per slot, soldered memory size and ECC capability. Those can easily be set in the devicetree.
Morgan Jang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36764 )
Change subject: src/mainboard/ocp/monolake: Create SMBIOS type 16 for Monolake platform ......................................................................
Patch Set 7:
(5 comments)
Patch Set 6:
I'd prefer a common driver for type16 entries. In the last few weeks everybody seem to duplicate those table creation code in mainboard directory. In the end you only need the maximum slot count, DIMM size per slot, soldered memory size and ECC capability. Those can easily be set in the devicetree.
The memory related configurations are not defined in device so far, should I wait for the configurations ready and then write a common driver for SMBIOS type16 entries?
https://review.coreboot.org/c/coreboot/+/36764/1/src/mainboard/ocp/monolake/... File src/mainboard/ocp/monolake/mainboard.c:
https://review.coreboot.org/c/coreboot/+/36764/1/src/mainboard/ocp/monolake/... PS1, Line 175: t->memory_error_correction = MEMORY_ARRAY_ECC_NONE; //The ECC setting can`t be confirmed in FSP, so hardcode it.
line over 96 characters
Done
https://review.coreboot.org/c/coreboot/+/36764/3/src/mainboard/ocp/monolake/... File src/mainboard/ocp/monolake/mainboard.c:
https://review.coreboot.org/c/coreboot/+/36764/3/src/mainboard/ocp/monolake/... PS3, Line 34: //in GB
Coding style in coreboot generally follows Linux, so we use /* */ for comments.
Done
https://review.coreboot.org/c/coreboot/+/36764/3/src/mainboard/ocp/monolake/... PS3, Line 78: //The ECC setting can`t be confirmed in FSP, so hardcode it.
comment style
Done
https://review.coreboot.org/c/coreboot/+/36764/3/src/mainboard/ocp/monolake/... PS3, Line 79: MEMORY_ARRAY_ECC_NONE
If we're hardcoding it, should it be hardcoded to MEMORY_ARRAY_ECC_SINGLE_BIT?
We checked the ECC setting with the tool, and confirmed it`s ECC enabled, so hardcode it to MEMORY_ARRAY_ECC_SINGLE_BIT.
https://review.coreboot.org/c/coreboot/+/36764/3/src/mainboard/ocp/monolake/... PS3, Line 83: (u32)(CONFIG_DIMM_MAX * MAX_DIMM_SIZE) << 20;
Suggestion: […]
Done
Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36764 )
Change subject: src/mainboard/ocp/monolake: Create SMBIOS type 16 for Monolake platform ......................................................................
Patch Set 7:
Patch Set 7:
(5 comments)
Patch Set 6:
I'd prefer a common driver for type16 entries. In the last few weeks everybody seem to duplicate those table creation code in mainboard directory. In the end you only need the maximum slot count, DIMM size per slot, soldered memory size and ECC capability. Those can easily be set in the devicetree.
The memory related configurations are not defined in device so far, should I wait for the configurations ready and then write a common driver for SMBIOS type16 entries?
The master will not support Broadwell-DE anymore, I think this change can be cherry-picked to 4.11_branch and continue to hard-coded approach. The better way of setting those values in devicetree can be implemented in master branch in the future.
Morgan Jang has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/36764 )
Change subject: src/mainboard/ocp/monolake: Create SMBIOS type 16 for Monolake platform ......................................................................
Abandoned
Move this change to 4.11 branch.
-
Arthur Heymans (Code Review) -
build bot (Jenkins) (Code Review)
-
David Hendricks (Code Review)
-
Johnny Lin (Code Review)
-
Morgan Jang (Code Review)
-
Name of user not set (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)