Anil Kumar K has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/39868 )
Change subject: mb/google/deltaur: Return SKU ID info ......................................................................
mb/google/deltaur: Return SKU ID info
For Deltaur and Deltan variants return proper SKU ID based on EC firmware type and sensor detect GPIO value
BUG=b:152544516 TEST=make build successful
Change-Id: I20a497739e5062400b093648c3a634203dec6105 Signed-off-by: Anil Kumar anil.kumar.k@intel.com --- M src/mainboard/google/deltaur/Makefile.inc A src/mainboard/google/deltaur/sku.c M src/mainboard/google/deltaur/variants/deltan/include/variant/gpio.h A src/mainboard/google/deltaur/variants/deltan/include/variant/variant.h M src/mainboard/google/deltaur/variants/deltaur/include/variant/gpio.h A src/mainboard/google/deltaur/variants/deltaur/include/variant/variant.h 6 files changed, 131 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/39868/1
diff --git a/src/mainboard/google/deltaur/Makefile.inc b/src/mainboard/google/deltaur/Makefile.inc index a913c75..d005788 100644 --- a/src/mainboard/google/deltaur/Makefile.inc +++ b/src/mainboard/google/deltaur/Makefile.inc @@ -14,6 +14,7 @@ ramstage-$(CONFIG_CHROMEOS) += chromeos.c ramstage-y += ec.c ramstage-y += mainboard.c +ramstage-y += sku.c
verstage-$(CONFIG_CHROMEOS) += chromeos.c verstage-y += ec.c diff --git a/src/mainboard/google/deltaur/sku.c b/src/mainboard/google/deltaur/sku.c new file mode 100644 index 0000000..736a145 --- /dev/null +++ b/src/mainboard/google/deltaur/sku.c @@ -0,0 +1,34 @@ +/* + * This file is part of the coreboot project. + * + * Copyright 2019 Google LLC + * + * 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 <boardid.h> +#include <smbios.h> +#include <ec/google/wilco/commands.h> +#include <variant/variant.h> + +static const uint32_t get_sku_index(void) +{ + return ((!has_360_sensor_board()) | (wilco_ec_signed_fw() << 1)); +} + +uint32_t sku_id(void) +{ + return skus[get_sku_index()].id; +} + +const char *smbios_system_sku(void) +{ + return skus[get_sku_index()].name; +} diff --git a/src/mainboard/google/deltaur/variants/deltan/include/variant/gpio.h b/src/mainboard/google/deltaur/variants/deltan/include/variant/gpio.h index a1e3789..f763bac 100644 --- a/src/mainboard/google/deltaur/variants/deltan/include/variant/gpio.h +++ b/src/mainboard/google/deltaur/variants/deltan/include/variant/gpio.h @@ -8,7 +8,9 @@ #define VARIANT_GPIO_H
#include <baseboard/gpio.h> - /* Copied from baseboard and may need to change for the new variant. */
+/* Sensor detection pin */ +#define SENSOR_DET_360 GPP_C10 + #endif diff --git a/src/mainboard/google/deltaur/variants/deltan/include/variant/variant.h b/src/mainboard/google/deltaur/variants/deltan/include/variant/variant.h new file mode 100644 index 0000000..b57a49f --- /dev/null +++ b/src/mainboard/google/deltaur/variants/deltan/include/variant/variant.h @@ -0,0 +1,45 @@ +/* + * This file is part of the coreboot project. + * + * Copyright 2018 Google LLC + * + * 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. + */ + +#ifndef VARIANT_H +#define VARIANT_H + +#include <variant/gpio.h> +#include <gpio.h> +/* SKU ID structure */ +typedef struct { + int id; + const char *name; +} sku_info; + +const static sku_info skus[] = { + /* Deltan 360 - invalid configuration */ + { .id = -1, .name = "sku_invalid" }, + /* Deltan */ + { .id = 1, .name = "sku1" }, + /* Deltan 360 signed - invalid configuration */ + { .id = -1, .name = "sku_invalid" }, + /* Deltan signed */ + { .id = 2, .name = "sku2" }, +}; + + +/* Check if the device has a 360 sensor board present */ +static inline int has_360_sensor_board(void) +{ + return gpio_get(SENSOR_DET_360) == 0; +} + +#endif diff --git a/src/mainboard/google/deltaur/variants/deltaur/include/variant/gpio.h b/src/mainboard/google/deltaur/variants/deltaur/include/variant/gpio.h index a1e3789..acede5d 100644 --- a/src/mainboard/google/deltaur/variants/deltaur/include/variant/gpio.h +++ b/src/mainboard/google/deltaur/variants/deltaur/include/variant/gpio.h @@ -8,7 +8,10 @@ #define VARIANT_GPIO_H
#include <baseboard/gpio.h> - +#include <soc/gpio.h> /* Copied from baseboard and may need to change for the new variant. */
+/* Sensor detection pin */ +#define SENSOR_DET_360 GPP_C10 + #endif diff --git a/src/mainboard/google/deltaur/variants/deltaur/include/variant/variant.h b/src/mainboard/google/deltaur/variants/deltaur/include/variant/variant.h new file mode 100644 index 0000000..4b2dca2 --- /dev/null +++ b/src/mainboard/google/deltaur/variants/deltaur/include/variant/variant.h @@ -0,0 +1,44 @@ +/* + * This file is part of the coreboot project. + * + * Copyright 2018 Google LLC + * + * 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. + */ + +#ifndef VARIANT_H +#define VARIANT_H + +#include <variant/gpio.h> + +/* SKU ID structure */ +typedef struct { + int id; + const char *name; +} sku_info; + +const static sku_info skus[] = { + /* Deltaur 360 */ + { .id = 3, .name = "sku3" }, + /* Deltaur */ + { .id = 4, .name = "sku4" }, + /* Deltaur 360 signed */ + { .id = 5, .name = "sku5" }, + /* Deltaur signed */ + { .id = 6, .name = "sku6" }, +}; + +/* Check if the device has a 360 sensor board present */ +static inline int has_360_sensor_board(void) +{ + return gpio_get(SENSOR_DET_360) == 0; +} + +#endif
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39868 )
Change subject: mb/google/deltaur: Return SKU ID info ......................................................................
Patch Set 1:
(36 comments)
https://review.coreboot.org/c/coreboot/+/39868/1/src/mainboard/google/deltau... File src/mainboard/google/deltaur/variants/deltan/include/variant/variant.h:
https://review.coreboot.org/c/coreboot/+/39868/1/src/mainboard/google/deltau... PS1, Line 23: int id; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39868/1/src/mainboard/google/deltau... PS1, Line 23: int id; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/39868/1/src/mainboard/google/deltau... PS1, Line 24: const char *name; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39868/1/src/mainboard/google/deltau... PS1, Line 24: const char *name; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/39868/1/src/mainboard/google/deltau... PS1, Line 28: /* Deltan 360 - invalid configuration */ code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39868/1/src/mainboard/google/deltau... PS1, Line 29: { .id = -1, .name = "sku_invalid" }, code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39868/1/src/mainboard/google/deltau... PS1, Line 29: { .id = -1, .name = "sku_invalid" }, please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/39868/1/src/mainboard/google/deltau... PS1, Line 30: /* Deltan */ code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39868/1/src/mainboard/google/deltau... PS1, Line 31: { .id = 1, .name = "sku1" }, code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39868/1/src/mainboard/google/deltau... PS1, Line 31: { .id = 1, .name = "sku1" }, please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/39868/1/src/mainboard/google/deltau... PS1, Line 32: /* Deltan 360 signed - invalid configuration */ code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39868/1/src/mainboard/google/deltau... PS1, Line 33: { .id = -1, .name = "sku_invalid" }, code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39868/1/src/mainboard/google/deltau... PS1, Line 33: { .id = -1, .name = "sku_invalid" }, please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/39868/1/src/mainboard/google/deltau... PS1, Line 34: /* Deltan signed */ code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39868/1/src/mainboard/google/deltau... PS1, Line 35: { .id = 2, .name = "sku2" }, code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39868/1/src/mainboard/google/deltau... PS1, Line 35: { .id = 2, .name = "sku2" }, please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/39868/1/src/mainboard/google/deltau... PS1, Line 42: return gpio_get(SENSOR_DET_360) == 0; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39868/1/src/mainboard/google/deltau... PS1, Line 42: return gpio_get(SENSOR_DET_360) == 0; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/39868/1/src/mainboard/google/deltau... File src/mainboard/google/deltaur/variants/deltaur/include/variant/variant.h:
https://review.coreboot.org/c/coreboot/+/39868/1/src/mainboard/google/deltau... PS1, Line 23: int id; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39868/1/src/mainboard/google/deltau... PS1, Line 23: int id; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/39868/1/src/mainboard/google/deltau... PS1, Line 24: const char *name; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39868/1/src/mainboard/google/deltau... PS1, Line 24: const char *name; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/39868/1/src/mainboard/google/deltau... PS1, Line 28: /* Deltaur 360 */ code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39868/1/src/mainboard/google/deltau... PS1, Line 29: { .id = 3, .name = "sku3" }, code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39868/1/src/mainboard/google/deltau... PS1, Line 29: { .id = 3, .name = "sku3" }, please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/39868/1/src/mainboard/google/deltau... PS1, Line 30: /* Deltaur */ code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39868/1/src/mainboard/google/deltau... PS1, Line 31: { .id = 4, .name = "sku4" }, code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39868/1/src/mainboard/google/deltau... PS1, Line 31: { .id = 4, .name = "sku4" }, please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/39868/1/src/mainboard/google/deltau... PS1, Line 32: /* Deltaur 360 signed */ code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39868/1/src/mainboard/google/deltau... PS1, Line 33: { .id = 5, .name = "sku5" }, code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39868/1/src/mainboard/google/deltau... PS1, Line 33: { .id = 5, .name = "sku5" }, please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/39868/1/src/mainboard/google/deltau... PS1, Line 34: /* Deltaur signed */ code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39868/1/src/mainboard/google/deltau... PS1, Line 35: { .id = 6, .name = "sku6" }, code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39868/1/src/mainboard/google/deltau... PS1, Line 35: { .id = 6, .name = "sku6" }, please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/39868/1/src/mainboard/google/deltau... PS1, Line 41: return gpio_get(SENSOR_DET_360) == 0; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39868/1/src/mainboard/google/deltau... PS1, Line 41: return gpio_get(SENSOR_DET_360) == 0; please, no spaces at the start of a line
Hello build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39868
to look at the new patch set (#2).
Change subject: mb/google/deltaur: Return SKU ID info ......................................................................
mb/google/deltaur: Return SKU ID info
For Deltaur and Deltan variants return proper SKU ID based on EC firmware type and sensor detect GPIO value
BUG=b:152544516 TEST=make build successful
Change-Id: I20a497739e5062400b093648c3a634203dec6105 Signed-off-by: Anil Kumar anil.kumar.k@intel.com --- M src/mainboard/google/deltaur/Makefile.inc A src/mainboard/google/deltaur/sku.c M src/mainboard/google/deltaur/variants/deltan/include/variant/gpio.h A src/mainboard/google/deltaur/variants/deltan/include/variant/variant.h M src/mainboard/google/deltaur/variants/deltaur/include/variant/gpio.h A src/mainboard/google/deltaur/variants/deltaur/include/variant/variant.h 6 files changed, 130 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/39868/2
Anil Kumar K has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39868 )
Change subject: mb/google/deltaur: Return SKU ID info ......................................................................
Patch Set 2: Code-Review+1
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39868 )
Change subject: mb/google/deltaur: Return SKU ID info ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39868/2/src/mainboard/google/deltau... File src/mainboard/google/deltaur/sku.c:
https://review.coreboot.org/c/coreboot/+/39868/2/src/mainboard/google/deltau... PS2, Line 21: static const uint32_t get_sku_index(void) You need weak this function. We should add it in different variants. Only Deltaur will has detect pin. Deltan just need signed and unsigned.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39868 )
Change subject: mb/google/deltaur: Return SKU ID info ......................................................................
Patch Set 2:
(5 comments)
https://review.coreboot.org/c/coreboot/+/39868/2/src/mainboard/google/deltau... File src/mainboard/google/deltaur/sku.c:
https://review.coreboot.org/c/coreboot/+/39868/2/src/mainboard/google/deltau... PS2, Line 21: static const uint32_t get_sku_index(void)
You need weak this function. We should add it in different variants. […]
I saw you separate in gpio.h.. It should be fine as well.
https://review.coreboot.org/c/coreboot/+/39868/2/src/mainboard/google/deltau... File src/mainboard/google/deltaur/variants/deltan/include/variant/gpio.h:
https://review.coreboot.org/c/coreboot/+/39868/2/src/mainboard/google/deltau... PS2, Line 14: #define SENSOR_DET_360 GPP_C10 remove this, just return 0.
https://review.coreboot.org/c/coreboot/+/39868/2/src/mainboard/google/deltau... File src/mainboard/google/deltaur/variants/deltan/include/variant/variant.h:
https://review.coreboot.org/c/coreboot/+/39868/2/src/mainboard/google/deltau... PS2, Line 41: return gpio_get(SENSOR_DET_360) == 0; Deltan just return 0 here.
https://review.coreboot.org/c/coreboot/+/39868/2/src/mainboard/google/deltau... File src/mainboard/google/deltaur/variants/deltaur/include/variant/gpio.h:
https://review.coreboot.org/c/coreboot/+/39868/2/src/mainboard/google/deltau... PS2, Line 15: #define SENSOR_DET_360 GPP_C10 TBD. Please add TODO here.
https://review.coreboot.org/c/coreboot/+/39868/2/src/mainboard/google/deltau... File src/mainboard/google/deltaur/variants/deltaur/include/variant/variant.h:
https://review.coreboot.org/c/coreboot/+/39868/2/src/mainboard/google/deltau... PS2, Line 29: { .id = 3, .name = "sku3" }, Please change to 1,2,3,4. We will separate two different products in model.yaml.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39868 )
Change subject: mb/google/deltaur: Return SKU ID info ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39868/2/src/mainboard/google/deltau... File src/mainboard/google/deltaur/variants/deltan/include/variant/variant.h:
https://review.coreboot.org/c/coreboot/+/39868/2/src/mainboard/google/deltau... PS2, Line 41: return gpio_get(SENSOR_DET_360) == 0;
Deltan just return 0 here.
Correct myself, return 1. But you can set this function in baseboard as weak function and default return 1.
Hello Bora Guvendik, build bot (Jenkins), Patrick Georgi, Martin Roth, Selma Bensaid, Tim Wawrzynczak, Bernardo Perez Priego, EricR Lai,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39868
to look at the new patch set (#3).
Change subject: mb/google/deltaur: Return SKU ID info ......................................................................
mb/google/deltaur: Return SKU ID info
For Deltaur and Deltan variants return proper SKU ID based on EC firmware type and sensor detect GPIO value
BUG=b:152544516 TEST=make build successful
Change-Id: I20a497739e5062400b093648c3a634203dec6105 Signed-off-by: Anil Kumar anil.kumar.k@intel.com --- M src/mainboard/google/deltaur/Makefile.inc A src/mainboard/google/deltaur/sku.c M src/mainboard/google/deltaur/variants/baseboard/gpio.c M src/mainboard/google/deltaur/variants/baseboard/include/baseboard/variants.h A src/mainboard/google/deltaur/variants/deltan/include/variant/variant.h M src/mainboard/google/deltaur/variants/deltaur/gpio.c M src/mainboard/google/deltaur/variants/deltaur/include/variant/gpio.h A src/mainboard/google/deltaur/variants/deltaur/include/variant/variant.h 8 files changed, 103 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/39868/3
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39868 )
Change subject: mb/google/deltaur: Return SKU ID info ......................................................................
Patch Set 3:
(5 comments)
https://review.coreboot.org/c/coreboot/+/39868/3/src/mainboard/google/deltau... File src/mainboard/google/deltaur/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/39868/3/src/mainboard/google/deltau... PS3, Line 51: return 0; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39868/3/src/mainboard/google/deltau... PS3, Line 51: return 0; please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/39868/3/src/mainboard/google/deltau... File src/mainboard/google/deltaur/variants/baseboard/include/baseboard/variants.h:
https://review.coreboot.org/c/coreboot/+/39868/3/src/mainboard/google/deltau... PS3, Line 25: int __weak has_360_sensor_board(void); Using weak declarations can have unintended link defects
https://review.coreboot.org/c/coreboot/+/39868/3/src/mainboard/google/deltau... File src/mainboard/google/deltaur/variants/deltaur/gpio.c:
https://review.coreboot.org/c/coreboot/+/39868/3/src/mainboard/google/deltau... PS3, Line 38: return gpio_get(SENSOR_DET_360) == 0; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/39868/3/src/mainboard/google/deltau... PS3, Line 38: return gpio_get(SENSOR_DET_360) == 0; please, no spaces at the start of a line
Hello Bora Guvendik, build bot (Jenkins), Patrick Georgi, Martin Roth, Selma Bensaid, Tim Wawrzynczak, Bernardo Perez Priego, EricR Lai,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39868
to look at the new patch set (#4).
Change subject: mb/google/deltaur: Return SKU ID info ......................................................................
mb/google/deltaur: Return SKU ID info
For Deltaur and Deltan variants return proper SKU ID based on EC firmware type and sensor detect GPIO value
BUG=b:152544516 TEST=make build successful
Change-Id: I20a497739e5062400b093648c3a634203dec6105 Signed-off-by: Anil Kumar anil.kumar.k@intel.com --- M src/mainboard/google/deltaur/Makefile.inc A src/mainboard/google/deltaur/sku.c M src/mainboard/google/deltaur/variants/baseboard/gpio.c M src/mainboard/google/deltaur/variants/baseboard/include/baseboard/variants.h A src/mainboard/google/deltaur/variants/deltan/include/variant/variant.h M src/mainboard/google/deltaur/variants/deltaur/gpio.c M src/mainboard/google/deltaur/variants/deltaur/include/variant/gpio.h A src/mainboard/google/deltaur/variants/deltaur/include/variant/variant.h 8 files changed, 103 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/39868/4
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39868 )
Change subject: mb/google/deltaur: Return SKU ID info ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39868/4/src/mainboard/google/deltau... File src/mainboard/google/deltaur/variants/baseboard/include/baseboard/variants.h:
https://review.coreboot.org/c/coreboot/+/39868/4/src/mainboard/google/deltau... PS4, Line 25: int __weak has_360_sensor_board(void); Using weak declarations can have unintended link defects
Anil Kumar K has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39868 )
Change subject: mb/google/deltaur: Return SKU ID info ......................................................................
Patch Set 4:
(5 comments)
https://review.coreboot.org/c/coreboot/+/39868/2/src/mainboard/google/deltau... File src/mainboard/google/deltaur/sku.c:
https://review.coreboot.org/c/coreboot/+/39868/2/src/mainboard/google/deltau... PS2, Line 21: static const uint32_t get_sku_index(void)
I saw you separate in gpio.h.. It should be fine as well.
Done
https://review.coreboot.org/c/coreboot/+/39868/2/src/mainboard/google/deltau... File src/mainboard/google/deltaur/variants/deltan/include/variant/gpio.h:
https://review.coreboot.org/c/coreboot/+/39868/2/src/mainboard/google/deltau... PS2, Line 14: #define SENSOR_DET_360 GPP_C10
remove this, just return 0.
Done
https://review.coreboot.org/c/coreboot/+/39868/2/src/mainboard/google/deltau... File src/mainboard/google/deltaur/variants/deltan/include/variant/variant.h:
https://review.coreboot.org/c/coreboot/+/39868/2/src/mainboard/google/deltau... PS2, Line 41: return gpio_get(SENSOR_DET_360) == 0;
Correct myself, return 1. […]
Done
https://review.coreboot.org/c/coreboot/+/39868/2/src/mainboard/google/deltau... File src/mainboard/google/deltaur/variants/deltaur/include/variant/gpio.h:
https://review.coreboot.org/c/coreboot/+/39868/2/src/mainboard/google/deltau... PS2, Line 15: #define SENSOR_DET_360 GPP_C10
TBD. Please add TODO here.
Done
https://review.coreboot.org/c/coreboot/+/39868/2/src/mainboard/google/deltau... File src/mainboard/google/deltaur/variants/deltaur/include/variant/variant.h:
https://review.coreboot.org/c/coreboot/+/39868/2/src/mainboard/google/deltau... PS2, Line 29: { .id = 3, .name = "sku3" },
Please change to 1,2,3,4. We will separate two different products in model.yaml.
Done
Anil Kumar K has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39868 )
Change subject: mb/google/deltaur: Return SKU ID info ......................................................................
Patch Set 4: Code-Review+1
Patch Set 2:
(1 comment)
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39868 )
Change subject: mb/google/deltaur: Return SKU ID info ......................................................................
Patch Set 4: Code-Review+1
(3 comments)
https://review.coreboot.org/c/coreboot/+/39868/4/src/mainboard/google/deltau... File src/mainboard/google/deltaur/sku.c:
https://review.coreboot.org/c/coreboot/+/39868/4/src/mainboard/google/deltau... PS4, Line 11: #include <baseboard/variants.h> nits: add new line here. nits: alphabet order for the includes.
https://review.coreboot.org/c/coreboot/+/39868/4/src/mainboard/google/deltau... File src/mainboard/google/deltaur/variants/deltan/include/variant/variant.h:
https://review.coreboot.org/c/coreboot/+/39868/4/src/mainboard/google/deltau... PS4, Line 11: #include <gpio.h> same comment as previous one.
https://review.coreboot.org/c/coreboot/+/39868/4/src/mainboard/google/deltau... File src/mainboard/google/deltaur/variants/deltaur/gpio.c:
https://review.coreboot.org/c/coreboot/+/39868/4/src/mainboard/google/deltau... PS4, Line 11: #include <gpio.h> nit: alphabet order ,thx.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39868 )
Change subject: mb/google/deltaur: Return SKU ID info ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/c/coreboot/+/39868/4/src/mainboard/google/deltau... File src/mainboard/google/deltaur/sku.c:
https://review.coreboot.org/c/coreboot/+/39868/4/src/mainboard/google/deltau... PS4, Line 17: sku_id Maybe board_sku_id?
https://review.coreboot.org/c/coreboot/+/39868/4/src/mainboard/google/deltau... File src/mainboard/google/deltaur/variants/baseboard/include/baseboard/variants.h:
https://review.coreboot.org/c/coreboot/+/39868/4/src/mainboard/google/deltau... PS4, Line 25: __weak buildbot is right, please no __weak in front of a declaration.
https://review.coreboot.org/c/coreboot/+/39868/4/src/mainboard/google/deltau... File src/mainboard/google/deltaur/variants/deltan/include/variant/variant.h:
https://review.coreboot.org/c/coreboot/+/39868/4/src/mainboard/google/deltau... PS4, Line 18: const static sku_info skus[] = { : /* Deltan 360 - invalid configuration */ : { .id = -1, .name = "sku_invalid" }, : /* Deltan */ : { .id = 1, .name = "sku1" }, : /* Deltan 360 signed - invalid configuration */ : { .id = -1, .name = "sku_invalid" }, : /* Deltan signed */ : { .id = 2, .name = "sku2" }, : }; Variable declarations rarely belong in header files.
Let's make a sku.c under each variant instead (deltaur/variants/deltaur/sku.c, and deltaur/variants/deltan/sku.c). variants.h can then have the prototypes for the functions in sku.c
Hello Bora Guvendik, build bot (Jenkins), Patrick Georgi, Martin Roth, Selma Bensaid, Tim Wawrzynczak, Kevin Chowski, Bernardo Perez Priego, EricR Lai,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39868
to look at the new patch set (#5).
Change subject: mb/google/deltaur: Return SKU ID info ......................................................................
mb/google/deltaur: Return SKU ID info
For Deltaur and Deltan variants return proper SKU ID based on EC firmware type and sensor detect GPIO value
BUG=b:152544516 TEST=make build successful
Change-Id: I20a497739e5062400b093648c3a634203dec6105 Signed-off-by: Anil Kumar anil.kumar.k@intel.com --- M src/mainboard/google/deltaur/variants/baseboard/gpio.c M src/mainboard/google/deltaur/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/deltaur/variants/deltan/Makefile.inc A src/mainboard/google/deltaur/variants/deltan/sku.c M src/mainboard/google/deltaur/variants/deltaur/Makefile.inc M src/mainboard/google/deltaur/variants/deltaur/gpio.c A src/mainboard/google/deltaur/variants/deltaur/include/variant/variants.h A src/mainboard/google/deltaur/variants/deltaur/sku.c 8 files changed, 115 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/39868/5
Anil Kumar K has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39868 )
Change subject: mb/google/deltaur: Return SKU ID info ......................................................................
Patch Set 5:
(4 comments)
https://review.coreboot.org/c/coreboot/+/39868/4/src/mainboard/google/deltau... File src/mainboard/google/deltaur/sku.c:
https://review.coreboot.org/c/coreboot/+/39868/4/src/mainboard/google/deltau... PS4, Line 11: #include <baseboard/variants.h>
nits: add new line here. […]
Done
https://review.coreboot.org/c/coreboot/+/39868/4/src/mainboard/google/deltau... PS4, Line 17: sku_id
Maybe board_sku_id?
this is predefined in boardid.h
https://review.coreboot.org/c/coreboot/+/39868/4/src/mainboard/google/deltau... File src/mainboard/google/deltaur/variants/baseboard/include/baseboard/variants.h:
https://review.coreboot.org/c/coreboot/+/39868/4/src/mainboard/google/deltau... PS4, Line 25: __weak
buildbot is right, please no __weak in front of a declaration.
Done
https://review.coreboot.org/c/coreboot/+/39868/4/src/mainboard/google/deltau... File src/mainboard/google/deltaur/variants/deltan/include/variant/variant.h:
https://review.coreboot.org/c/coreboot/+/39868/4/src/mainboard/google/deltau... PS4, Line 18: const static sku_info skus[] = { : /* Deltan 360 - invalid configuration */ : { .id = -1, .name = "sku_invalid" }, : /* Deltan */ : { .id = 1, .name = "sku1" }, : /* Deltan 360 signed - invalid configuration */ : { .id = -1, .name = "sku_invalid" }, : /* Deltan signed */ : { .id = 2, .name = "sku2" }, : };
Variable declarations rarely belong in header files. […]
Done
Anil Kumar K has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39868 )
Change subject: mb/google/deltaur: Return SKU ID info ......................................................................
Patch Set 5: Code-Review+1
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39868 )
Change subject: mb/google/deltaur: Return SKU ID info ......................................................................
Patch Set 5:
This conflicts with https://review.coreboot.org/c/coreboot/+/39848, you should rebase on top of that first.
Hello Bora Guvendik, build bot (Jenkins), Patrick Georgi, Martin Roth, Selma Bensaid, Tim Wawrzynczak, Kevin Chowski, Bernardo Perez Priego, EricR Lai,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39868
to look at the new patch set (#6).
Change subject: mb/google/deltaur: Return SKU ID info ......................................................................
mb/google/deltaur: Return SKU ID info
For Deltaur and Deltan variants return proper SKU ID based on EC firmware type and sensor detect GPIO value
BUG=b:152544516 TEST=make build successful
Change-Id: I20a497739e5062400b093648c3a634203dec6105 Signed-off-by: Anil Kumar anil.kumar.k@intel.com --- M src/mainboard/google/deltaur/variants/baseboard/gpio.c M src/mainboard/google/deltaur/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/deltaur/variants/deltan/Makefile.inc A src/mainboard/google/deltaur/variants/deltan/sku.c M src/mainboard/google/deltaur/variants/deltaur/Makefile.inc M src/mainboard/google/deltaur/variants/deltaur/gpio.c A src/mainboard/google/deltaur/variants/deltaur/include/variant/variants.h A src/mainboard/google/deltaur/variants/deltaur/sku.c 8 files changed, 115 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/39868/6
Anil Kumar K has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39868 )
Change subject: mb/google/deltaur: Return SKU ID info ......................................................................
Patch Set 6: Code-Review+1
Varun Joshi has uploaded a new patch set (#8) to the change originally created by Anil Kumar K. ( https://review.coreboot.org/c/coreboot/+/39868 )
Change subject: mb/google/deltaur: Return SKU ID info ......................................................................
mb/google/deltaur: Return SKU ID info
For Deltaur and Deltan variants return proper SKU ID based on EC firmware type and sensor detect GPIO value
BUG=b:152544516 TEST=make build successful
Change-Id: I20a497739e5062400b093648c3a634203dec6105 Signed-off-by: Anil Kumar anil.kumar.k@intel.com --- M src/mainboard/google/deltaur/variants/baseboard/gpio.c M src/mainboard/google/deltaur/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/deltaur/variants/deltan/Makefile.inc A src/mainboard/google/deltaur/variants/deltan/sku.c M src/mainboard/google/deltaur/variants/deltaur/Makefile.inc M src/mainboard/google/deltaur/variants/deltaur/gpio.c A src/mainboard/google/deltaur/variants/deltaur/include/variant/variants.h A src/mainboard/google/deltaur/variants/deltaur/sku.c 8 files changed, 115 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/39868/8
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39868 )
Change subject: mb/google/deltaur: Return SKU ID info ......................................................................
Patch Set 8:
(3 comments)
https://review.coreboot.org/c/coreboot/+/39868/8/src/mainboard/google/deltau... File src/mainboard/google/deltaur/variants/deltan/sku.c:
https://review.coreboot.org/c/coreboot/+/39868/8/src/mainboard/google/deltau... PS8, Line 18: const static sku_info skus[] = { : /* Deltan 360 - invalid configuration */ : { .id = -1, .name = "sku_invalid" }, : /* Deltan */ : { .id = 1, .name = "sku1" }, : /* Deltan 360 signed - invalid configuration */ : { .id = -1, .name = "sku_invalid" }, : /* Deltan signed */ : { .id = 2, .name = "sku2" }, : }; : : static const uint32_t get_sku_index(void) : { : return ((!has_360_sensor_board()) | (wilco_ec_signed_fw() << 1)); : } Since Deltan does not have the 360 sensor board, how about rewriting it without that? Something like:
skus[] = { { .id = 1, .name = "sku1" }, { .id = 2, .name = "sku2" }, };
get_sku_index(void) { return wilco_ec_signed_fw(); }
?
https://review.coreboot.org/c/coreboot/+/39868/8/src/mainboard/google/deltau... File src/mainboard/google/deltaur/variants/deltaur/include/variant/variants.h:
https://review.coreboot.org/c/coreboot/+/39868/8/src/mainboard/google/deltau... PS8, Line 10: : I created a partner bug for this. Can you add b/153027724 in the comment?
https://review.coreboot.org/c/coreboot/+/39868/8/src/mainboard/google/deltau... File src/mainboard/google/deltaur/variants/deltaur/sku.c:
https://review.coreboot.org/c/coreboot/+/39868/8/src/mainboard/google/deltau... PS8, Line 31: ! !! would be a little more correct here.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39868 )
Change subject: mb/google/deltaur: Return SKU ID info ......................................................................
Patch Set 9:
Patch Set 8:
(3 comments)
My thought is we can make get_sku_index, sku_id, smbios_system_sku in baseboard, since they are the same. And put "sku_info skus" in variant. This should be consistent and clear.
Anil Kumar K has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39868 )
Change subject: mb/google/deltaur: Return SKU ID info ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39868/8/src/mainboard/google/deltau... File src/mainboard/google/deltaur/variants/deltaur/sku.c:
https://review.coreboot.org/c/coreboot/+/39868/8/src/mainboard/google/deltau... PS8, Line 31: !
!! would be a little more correct here.
the has_360_sensor_board returns 1 for 360 config and 0 for clamshell. Therefor i guess it should be !has_360_sensor_board. That way we have the following the result of the following expression would be
(!has_360_sensor_board()) | (wilco_ec_signed_fw() << 1)
0 -> 360 1 -> 180 2 -> 360 signed 3 -> 180 signed
This is as per https://partnerissuetracker.corp.google.com/u/1/issues/152333562.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39868 )
Change subject: mb/google/deltaur: Return SKU ID info ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39868/8/src/mainboard/google/deltau... File src/mainboard/google/deltaur/variants/deltaur/sku.c:
https://review.coreboot.org/c/coreboot/+/39868/8/src/mainboard/google/deltau... PS8, Line 31: !
the has_360_sensor_board returns 1 for 360 config and 0 for clamshell. […]
Ack
Anil Kumar K has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39868 )
Change subject: mb/google/deltaur: Return SKU ID info ......................................................................
Patch Set 9:
(1 comment)
Patch Set 9:
Patch Set 8:
(3 comments)
My thought is we can make get_sku_index, sku_id, smbios_system_sku in baseboard, since they are the same. And put "sku_info skus" in variant. This should be consistent and clear.
If i take care of Tim's last comment the get_sku_index would be different for each variant. So this would stay in variants. Also the sku_info struct is part of sku.c now and not exposed through a header file . Keeping these in mind shall we have these functions inside variants, instead of mainboard.c file ? Thoughts ?
https://review.coreboot.org/c/coreboot/+/39868/8/src/mainboard/google/deltau... File src/mainboard/google/deltaur/variants/deltaur/include/variant/variants.h:
https://review.coreboot.org/c/coreboot/+/39868/8/src/mainboard/google/deltau... PS8, Line 10: :
I created a partner bug for this. […]
will do
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39868 )
Change subject: mb/google/deltaur: Return SKU ID info ......................................................................
Patch Set 9:
My first thought is like Tim's comment. But I think it's fine to keep the sku_id struct consistent.
Anil Kumar K has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39868 )
Change subject: mb/google/deltaur: Return SKU ID info ......................................................................
Patch Set 9:
Patch Set 9:
My first thought is like Tim's comment. But I think it's fine to keep the sku_id struct consistent.
So we are inclining more towards patchset4 implementation. Have one sku.c in the root directory and implement the sku_info structure definitions inside variants.h . Have same implementation for sku_id, smbios_system_sku, get_sku_index functions in sku.c . I second this approach and think it would be more consistent.
Tim , if u are ok with this we will move back to patchset 4 implementation ? Suggestions ?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39868 )
Change subject: mb/google/deltaur: Return SKU ID info ......................................................................
Patch Set 9:
(1 comment)
Sure, let's go with that.
https://review.coreboot.org/c/coreboot/+/39868/4/src/mainboard/google/deltau... File src/mainboard/google/deltaur/sku.c:
https://review.coreboot.org/c/coreboot/+/39868/4/src/mainboard/google/deltau... PS4, Line 17: sku_id
this is predefined in boardid. […]
Ack
Hello Bora Guvendik, build bot (Jenkins), Patrick Georgi, Martin Roth, Selma Bensaid, Tim Wawrzynczak, Kevin Chowski, Bernardo Perez Priego, Varun Joshi, EricR Lai,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39868
to look at the new patch set (#10).
Change subject: mb/google/deltaur: Return SKU ID info ......................................................................
mb/google/deltaur: Return SKU ID info
For Deltaur and Deltan variants return proper SKU ID based on EC firmware type and sensor detect GPIO value
BUG=b:152544516 TEST=make build successful
Change-Id: I20a497739e5062400b093648c3a634203dec6105 Signed-off-by: Anil Kumar anil.kumar.k@intel.com --- M src/mainboard/google/deltaur/Makefile.inc A src/mainboard/google/deltaur/sku.c M src/mainboard/google/deltaur/variants/baseboard/gpio.c M src/mainboard/google/deltaur/variants/baseboard/include/baseboard/variants.h A src/mainboard/google/deltaur/variants/deltan/include/variant/variant.h M src/mainboard/google/deltaur/variants/deltaur/gpio.c M src/mainboard/google/deltaur/variants/deltaur/include/variant/gpio.h A src/mainboard/google/deltaur/variants/deltaur/include/variant/variant.h 8 files changed, 105 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/39868/10
Anil Kumar K has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39868 )
Change subject: mb/google/deltaur: Return SKU ID info ......................................................................
Patch Set 10: Code-Review+1
(3 comments)
https://review.coreboot.org/c/coreboot/+/39868/4/src/mainboard/google/deltau... File src/mainboard/google/deltaur/variants/deltan/include/variant/variant.h:
https://review.coreboot.org/c/coreboot/+/39868/4/src/mainboard/google/deltau... PS4, Line 11: #include <gpio.h>
same comment as previous one.
Done
https://review.coreboot.org/c/coreboot/+/39868/4/src/mainboard/google/deltau... File src/mainboard/google/deltaur/variants/deltaur/gpio.c:
https://review.coreboot.org/c/coreboot/+/39868/4/src/mainboard/google/deltau... PS4, Line 11: #include <gpio.h>
nit: alphabet order ,thx.
Done
https://review.coreboot.org/c/coreboot/+/39868/8/src/mainboard/google/deltau... File src/mainboard/google/deltaur/variants/deltaur/include/variant/variants.h:
https://review.coreboot.org/c/coreboot/+/39868/8/src/mainboard/google/deltau... PS8, Line 10: :
will do
Done
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39868 )
Change subject: mb/google/deltaur: Return SKU ID info ......................................................................
Patch Set 10:
(3 comments)
Or we can use as default then
const static sku_info skus[] = { /* Deltan 360 - invalid configuration */ { .id = -1, .name = "sku_invalid" }, /* Deltan */ { .id = 1, .name = "sku1" }, /* Deltan 360 signed - invalid configuration */ { .id = -1, .name = "sku_invalid" }, /* Deltan signed */ { .id = 2, .name = "sku2" }, };
https://review.coreboot.org/c/coreboot/+/39868/10/src/mainboard/google/delta... File src/mainboard/google/deltaur/sku.c:
https://review.coreboot.org/c/coreboot/+/39868/10/src/mainboard/google/delta... PS10, Line 18: uint32_t sku_id(void) maybe const
https://review.coreboot.org/c/coreboot/+/39868/10/src/mainboard/google/delta... File src/mainboard/google/deltaur/variants/deltan/include/variant/variant.h:
https://review.coreboot.org/c/coreboot/+/39868/10/src/mainboard/google/delta... PS10, Line 13: /* SKU ID structure */ move this to baseboard.
https://review.coreboot.org/c/coreboot/+/39868/10/src/mainboard/google/delta... File src/mainboard/google/deltaur/variants/deltaur/include/variant/variant.h:
https://review.coreboot.org/c/coreboot/+/39868/10/src/mainboard/google/delta... PS10, Line 12: /* SKU ID structure */ same.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39868 )
Change subject: mb/google/deltaur: Return SKU ID info ......................................................................
Patch Set 10:
From error log, you need to rebase this.
Hello Bora Guvendik, build bot (Jenkins), Patrick Georgi, Martin Roth, Selma Bensaid, Tim Wawrzynczak, Kevin Chowski, Anil Kumar K, Bernardo Perez Priego, Varun Joshi, EricR Lai,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39868
to look at the new patch set (#11).
Change subject: mb/google/deltaur: Return SKU ID info ......................................................................
mb/google/deltaur: Return SKU ID info
For Deltaur and Deltan variants return proper SKU ID based on EC firmware type and sensor detect GPIO value
BUG=b:152544516 TEST=make build successful
Change-Id: I20a497739e5062400b093648c3a634203dec6105 Signed-off-by: Anil Kumar anil.kumar.k@intel.com --- M src/mainboard/google/deltaur/variants/baseboard/Makefile.inc M src/mainboard/google/deltaur/variants/baseboard/gpio.c M src/mainboard/google/deltaur/variants/baseboard/include/baseboard/variants.h A src/mainboard/google/deltaur/variants/baseboard/sku.c A src/mainboard/google/deltaur/variants/deltan/include/variant/variant.h M src/mainboard/google/deltaur/variants/deltaur/gpio.c M src/mainboard/google/deltaur/variants/deltaur/include/variant/gpio.h A src/mainboard/google/deltaur/variants/deltaur/include/variant/variant.h 8 files changed, 100 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/39868/11
Hello Bora Guvendik, build bot (Jenkins), Patrick Georgi, Martin Roth, Selma Bensaid, Tim Wawrzynczak, Kevin Chowski, Anil Kumar K, Bernardo Perez Priego, Varun Joshi, EricR Lai,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39868
to look at the new patch set (#12).
Change subject: mb/google/deltaur: Return SKU ID info ......................................................................
mb/google/deltaur: Return SKU ID info
For Deltaur and Deltan variants return proper SKU ID based on EC firmware type and sensor detect GPIO value
BUG=b:152544516 TEST=make build successful
Change-Id: I20a497739e5062400b093648c3a634203dec6105 Signed-off-by: Anil Kumar anil.kumar.k@intel.com --- M src/mainboard/google/deltaur/variants/baseboard/Makefile.inc M src/mainboard/google/deltaur/variants/baseboard/gpio.c M src/mainboard/google/deltaur/variants/baseboard/include/baseboard/variants.h A src/mainboard/google/deltaur/variants/baseboard/sku.c A src/mainboard/google/deltaur/variants/deltan/include/variant/variant.h M src/mainboard/google/deltaur/variants/deltaur/gpio.c A src/mainboard/google/deltaur/variants/deltaur/include/variant/variant.h 7 files changed, 100 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/39868/12
Anil Kumar K has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39868 )
Change subject: mb/google/deltaur: Return SKU ID info ......................................................................
Patch Set 12: Code-Review+1
Hello Bora Guvendik, build bot (Jenkins), Patrick Georgi, Martin Roth, Selma Bensaid, Tim Wawrzynczak, Kevin Chowski, Anil Kumar K, Bernardo Perez Priego, Varun Joshi, EricR Lai,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39868
to look at the new patch set (#13).
Change subject: mb/google/deltaur: Return SKU ID info ......................................................................
mb/google/deltaur: Return SKU ID info
For Deltaur and Deltan variants return proper SKU ID based on EC firmware type and sensor detect GPIO value
BUG=b:152544516 TEST=make build successful
Change-Id: I20a497739e5062400b093648c3a634203dec6105 Signed-off-by: Anil Kumar anil.kumar.k@intel.com --- M src/mainboard/google/deltaur/variants/baseboard/Makefile.inc M src/mainboard/google/deltaur/variants/baseboard/gpio.c M src/mainboard/google/deltaur/variants/baseboard/include/baseboard/variants.h A src/mainboard/google/deltaur/variants/baseboard/sku.c A src/mainboard/google/deltaur/variants/deltan/include/variant/variant.h M src/mainboard/google/deltaur/variants/deltaur/gpio.c M src/mainboard/google/deltaur/variants/deltaur/include/variant/gpio.h A src/mainboard/google/deltaur/variants/deltaur/include/variant/variant.h 8 files changed, 100 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/39868/13
Hello Bora Guvendik, build bot (Jenkins), Patrick Georgi, Martin Roth, Selma Bensaid, Tim Wawrzynczak, Kevin Chowski, Anil Kumar K, Bernardo Perez Priego, Varun Joshi, EricR Lai,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39868
to look at the new patch set (#14).
Change subject: mb/google/deltaur: Return SKU ID info ......................................................................
mb/google/deltaur: Return SKU ID info
For Deltaur and Deltan variants return proper SKU ID based on EC firmware type and sensor detect GPIO value
BUG=b:152544516 TEST=make build successful
Change-Id: I20a497739e5062400b093648c3a634203dec6105 Signed-off-by: Anil Kumar anil.kumar.k@intel.com --- M src/mainboard/google/deltaur/variants/baseboard/Makefile.inc M src/mainboard/google/deltaur/variants/baseboard/gpio.c M src/mainboard/google/deltaur/variants/baseboard/include/baseboard/variants.h A src/mainboard/google/deltaur/variants/baseboard/sku.c A src/mainboard/google/deltaur/variants/deltan/include/variant/variant.h M src/mainboard/google/deltaur/variants/deltaur/gpio.c A src/mainboard/google/deltaur/variants/deltaur/include/variant/variant.h 7 files changed, 100 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/39868/14
Anil Kumar K has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39868 )
Change subject: mb/google/deltaur: Return SKU ID info ......................................................................
Patch Set 14: Code-Review+1
Hello Bora Guvendik, build bot (Jenkins), Patrick Georgi, Martin Roth, Selma Bensaid, Tim Wawrzynczak, Kevin Chowski, Anil Kumar K, Bernardo Perez Priego, Varun Joshi, EricR Lai,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39868
to look at the new patch set (#15).
Change subject: mb/google/deltaur: Return SKU ID info ......................................................................
mb/google/deltaur: Return SKU ID info
For Deltaur and Deltan variants return proper SKU ID based on EC firmware type and sensor detect GPIO value
BUG=b:152544516 TEST=make build successful for deltan
Change-Id: I20a497739e5062400b093648c3a634203dec6105 Signed-off-by: Anil Kumar anil.kumar.k@intel.com --- M src/mainboard/google/deltaur/variants/baseboard/Makefile.inc M src/mainboard/google/deltaur/variants/baseboard/gpio.c M src/mainboard/google/deltaur/variants/baseboard/include/baseboard/variants.h A src/mainboard/google/deltaur/variants/baseboard/sku.c A src/mainboard/google/deltaur/variants/deltan/include/variant/variant.h M src/mainboard/google/deltaur/variants/deltaur/gpio.c A src/mainboard/google/deltaur/variants/deltaur/include/variant/variant.h 7 files changed, 100 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/39868/15
Anil Kumar K has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39868 )
Change subject: mb/google/deltaur: Return SKU ID info ......................................................................
Patch Set 15:
(3 comments)
https://review.coreboot.org/c/coreboot/+/39868/10/src/mainboard/google/delta... File src/mainboard/google/deltaur/sku.c:
https://review.coreboot.org/c/coreboot/+/39868/10/src/mainboard/google/delta... PS10, Line 18: uint32_t sku_id(void)
maybe const
Done
https://review.coreboot.org/c/coreboot/+/39868/10/src/mainboard/google/delta... File src/mainboard/google/deltaur/variants/deltan/include/variant/variant.h:
https://review.coreboot.org/c/coreboot/+/39868/10/src/mainboard/google/delta... PS10, Line 13: /* SKU ID structure */
move this to baseboard.
Done
https://review.coreboot.org/c/coreboot/+/39868/10/src/mainboard/google/delta... File src/mainboard/google/deltaur/variants/deltaur/include/variant/variant.h:
https://review.coreboot.org/c/coreboot/+/39868/10/src/mainboard/google/delta... PS10, Line 12: /* SKU ID structure */
same.
Done
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39868 )
Change subject: mb/google/deltaur: Return SKU ID info ......................................................................
Patch Set 15: Code-Review+2
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39868 )
Change subject: mb/google/deltaur: Return SKU ID info ......................................................................
Patch Set 15: Code-Review+2
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39868 )
Change subject: mb/google/deltaur: Return SKU ID info ......................................................................
Patch Set 16:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39868/8/src/mainboard/google/deltau... File src/mainboard/google/deltaur/variants/deltan/sku.c:
https://review.coreboot.org/c/coreboot/+/39868/8/src/mainboard/google/deltau... PS8, Line 18: const static sku_info skus[] = { : /* Deltan 360 - invalid configuration */ : { .id = -1, .name = "sku_invalid" }, : /* Deltan */ : { .id = 1, .name = "sku1" }, : /* Deltan 360 signed - invalid configuration */ : { .id = -1, .name = "sku_invalid" }, : /* Deltan signed */ : { .id = 2, .name = "sku2" }, : }; : : static const uint32_t get_sku_index(void) : { : return ((!has_360_sensor_board()) | (wilco_ec_signed_fw() << 1)); : }
Since Deltan does not have the 360 sensor board, how about rewriting it without that? Something like […]
Ack
Martin Roth has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39868 )
Change subject: mb/google/deltaur: Return SKU ID info ......................................................................
mb/google/deltaur: Return SKU ID info
For Deltaur and Deltan variants return proper SKU ID based on EC firmware type and sensor detect GPIO value
BUG=b:152544516 TEST=make build successful for deltan
Change-Id: I20a497739e5062400b093648c3a634203dec6105 Signed-off-by: Anil Kumar anil.kumar.k@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/39868 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org Reviewed-by: EricR Lai ericr_lai@compal.corp-partner.google.com --- M src/mainboard/google/deltaur/variants/baseboard/Makefile.inc M src/mainboard/google/deltaur/variants/baseboard/gpio.c M src/mainboard/google/deltaur/variants/baseboard/include/baseboard/variants.h A src/mainboard/google/deltaur/variants/baseboard/sku.c A src/mainboard/google/deltaur/variants/deltan/include/variant/variant.h M src/mainboard/google/deltaur/variants/deltaur/gpio.c A src/mainboard/google/deltaur/variants/deltaur/include/variant/variant.h 7 files changed, 100 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified EricR Lai: Looks good to me, approved Tim Wawrzynczak: Looks good to me, approved Anil Kumar K: Looks good to me, but someone else must approve
diff --git a/src/mainboard/google/deltaur/variants/baseboard/Makefile.inc b/src/mainboard/google/deltaur/variants/baseboard/Makefile.inc index 937cb46..277b75a 100644 --- a/src/mainboard/google/deltaur/variants/baseboard/Makefile.inc +++ b/src/mainboard/google/deltaur/variants/baseboard/Makefile.inc @@ -7,5 +7,6 @@ bootblock-y += gpio.c
ramstage-y += gpio.c +ramstage-y += sku.c
verstage-y += gpio.c diff --git a/src/mainboard/google/deltaur/variants/baseboard/gpio.c b/src/mainboard/google/deltaur/variants/baseboard/gpio.c index a67dd8cb..46a5cdd 100644 --- a/src/mainboard/google/deltaur/variants/baseboard/gpio.c +++ b/src/mainboard/google/deltaur/variants/baseboard/gpio.c @@ -468,3 +468,8 @@ *num = ARRAY_SIZE(early_gpio_table); return early_gpio_table; } + +int __weak has_360_sensor_board(void) +{ + return 0; +} diff --git a/src/mainboard/google/deltaur/variants/baseboard/include/baseboard/variants.h b/src/mainboard/google/deltaur/variants/baseboard/include/baseboard/variants.h index a1f1b22..332a2c6 100644 --- a/src/mainboard/google/deltaur/variants/baseboard/include/baseboard/variants.h +++ b/src/mainboard/google/deltaur/variants/baseboard/include/baseboard/variants.h @@ -25,4 +25,13 @@ const struct lpddr4x_cfg *variant_memory_params(void); void variant_memory_init(FSP_M_CONFIG *mem_cfg);
+/* SKU ID structure */ +typedef struct { + int id; + const char *name; +} sku_info; + +/* Check if the device has a 360 sensor board present */ +int has_360_sensor_board(void); + #endif /* __BASEBOARD_VARIANTS_H__ */ diff --git a/src/mainboard/google/deltaur/variants/baseboard/sku.c b/src/mainboard/google/deltaur/variants/baseboard/sku.c new file mode 100644 index 0000000..8465e64 --- /dev/null +++ b/src/mainboard/google/deltaur/variants/baseboard/sku.c @@ -0,0 +1,26 @@ +/* + * This file is part of the coreboot project. + * + * SPDX-License-Identifier: GPL-2.0-or-later + */ + +#include <baseboard/variants.h> +#include <boardid.h> +#include <ec/google/wilco/commands.h> +#include <smbios.h> +#include <variant/variant.h> + +static const uint32_t get_sku_index(void) +{ + return ((!has_360_sensor_board()) | (wilco_ec_signed_fw() << 1)); +} + +const uint32_t sku_id(void) +{ + return skus[get_sku_index()].id; +} + +const char *smbios_system_sku(void) +{ + return skus[get_sku_index()].name; +} diff --git a/src/mainboard/google/deltaur/variants/deltan/include/variant/variant.h b/src/mainboard/google/deltaur/variants/deltan/include/variant/variant.h new file mode 100644 index 0000000..be4c970 --- /dev/null +++ b/src/mainboard/google/deltaur/variants/deltan/include/variant/variant.h @@ -0,0 +1,25 @@ +/* + * This file is part of the coreboot project. + * + * SPDX-License-Identifier: GPL-2.0-or-later + */ + +#ifndef VARIANT_H +#define VARIANT_H + +#include <baseboard/variants.h> +#include <gpio.h> +#include <variant/gpio.h> + +const static sku_info skus[] = { + /* Deltan 360 - invalid configuration */ + { .id = -1, .name = "sku_invalid" }, + /* Deltan */ + { .id = 1, .name = "sku1" }, + /* Deltan 360 signed - invalid configuration */ + { .id = -1, .name = "sku_invalid" }, + /* Deltan signed */ + { .id = 2, .name = "sku2" }, +}; + +#endif diff --git a/src/mainboard/google/deltaur/variants/deltaur/gpio.c b/src/mainboard/google/deltaur/variants/deltaur/gpio.c index 30315bb..02cb127 100644 --- a/src/mainboard/google/deltaur/variants/deltaur/gpio.c +++ b/src/mainboard/google/deltaur/variants/deltaur/gpio.c @@ -7,6 +7,8 @@ #include <baseboard/gpio.h> #include <baseboard/variants.h> #include <commonlib/helpers.h> +#include <gpio.h> +#include <variant/variant.h>
/* Pad configuration in ramstage */ static const struct pad_config gpio_table[] = { @@ -29,3 +31,9 @@ *num = ARRAY_SIZE(early_gpio_table); return early_gpio_table; } + +/* Check if the device has a 360 sensor board present */ +int has_360_sensor_board(void) +{ + return gpio_get(SENSOR_DET_360) == 0; +} diff --git a/src/mainboard/google/deltaur/variants/deltaur/include/variant/variant.h b/src/mainboard/google/deltaur/variants/deltaur/include/variant/variant.h new file mode 100644 index 0000000..8a5e3a8 --- /dev/null +++ b/src/mainboard/google/deltaur/variants/deltaur/include/variant/variant.h @@ -0,0 +1,26 @@ +/* + * This file is part of the coreboot project. + * + * SPDX-License-Identifier: GPL-2.0-or-later + */ + +#ifndef VARIANT_H +#define VARIANT_H + +#include <baseboard/variants.h> + +/* TODO b/153027724: Sensor detection pin */ +#define SENSOR_DET_360 GPP_C10 + +const static sku_info skus[] = { + /* Deltaur 360 */ + { .id = 1, .name = "sku1" }, + /* Deltaur */ + { .id = 2, .name = "sku2" }, + /* Deltaur 360 signed */ + { .id = 3, .name = "sku3" }, + /* Deltaur signed */ + { .id = 4, .name = "sku4" }, +}; + +#endif
-
Anil Kumar K (Code Review) -
build bot (Jenkins) (Code Review)
-
EricR Lai (Code Review)
-
Martin Roth (Code Review)
-
Tim Wawrzynczak (Code Review)
-
Varun Joshi (Code Review)