Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/39018 )
Change subject: ec/google/chromeec: Introduce SKU_ID helpers ......................................................................
ec/google/chromeec: Introduce SKU_ID helpers
Change-Id: I805b25465a3b4ee3dc0cbda5feb9e9ea2493ff9e Signed-off-by: Edward O'Callaghan quasisec@google.com --- M src/ec/google/chromeec/Kconfig M src/ec/google/chromeec/ec.h M src/ec/google/chromeec/ec_boardid.c 3 files changed, 40 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/39018/1
diff --git a/src/ec/google/chromeec/Kconfig b/src/ec/google/chromeec/Kconfig index b33864f..5616d94 100644 --- a/src/ec/google/chromeec/Kconfig +++ b/src/ec/google/chromeec/Kconfig @@ -100,6 +100,11 @@ hex default 0x0
+# set to 0xff for LEGACY +config EC_GOOGLE_CHROMEEC_SKU_ID_MAX + hex + default 0x7fffffff + config EC_GOOGLE_CHROMEEC_BOARDNAME depends on EC_GOOGLE_CHROMEEC string "Chrome EC board name for EC" diff --git a/src/ec/google/chromeec/ec.h b/src/ec/google/chromeec/ec.h index 5ce375e..f5c2e1a 100644 --- a/src/ec/google/chromeec/ec.h +++ b/src/ec/google/chromeec/ec.h @@ -88,6 +88,9 @@ int google_chromeec_cbi_get_dram_part_num(char *buf, size_t bufsize); int google_chromeec_cbi_get_oem_name(char *buf, size_t bufsize);
+uint32_t google_chromeec_get_board_sku(void); +const char *google_chromeec_smbios_system_sku(void); + /* MEC uses 0x800/0x804 as register/index pair, thus an 8-byte resource. */ #define MEC_EMI_BASE 0x800 #define MEC_EMI_SIZE 8 diff --git a/src/ec/google/chromeec/ec_boardid.c b/src/ec/google/chromeec/ec_boardid.c index 1307ce1..aa191c3 100644 --- a/src/ec/google/chromeec/ec_boardid.c +++ b/src/ec/google/chromeec/ec_boardid.c @@ -27,3 +27,35 @@
return id; } + +#define SKU_UNKNOWN 0xFFFFFFFF + +uint32_t google_chromeec_get_board_sku(void) +{ + static uint32_t sku_id = SKU_UNKNOWN; + + if (sku_id != SKU_UNKNOWN) + return sku_id; + + if (google_chromeec_cbi_get_sku_id(&sku_id)) + sku_id = SKU_UNKNOWN; + + return sku_id; +} + +const char *google_chromeec_smbios_system_sku(void) +{ + static char sku_str[7]; /* sku{0..255} */ + uint32_t sku_id = get_board_sku(); + + if ((sku_id == SKU_UNKNOWN) || + (sku_id > CONFIG_EC_GOOGLE_CHROMEEC_SKU_MAX)) { + printk(BIOS_ERR, "%s: Unexpected SKU ID %u\n", + __func__, sku_id); + return ""; + } + + snprintf(sku_str, sizeof(sku_str), "sku%u", sku_id); + + return sku_str; +}
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39018 )
Change subject: ec/google/chromeec: Introduce SKU_ID helpers ......................................................................
Patch Set 2:
This change is ready for review.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39018 )
Change subject: ec/google/chromeec: Introduce SKU_ID helpers ......................................................................
Patch Set 3:
This change is ready for review.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39018 )
Change subject: ec/google/chromeec: Introduce SKU_ID helpers ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/39018/3/src/ec/google/chromeec/ec_b... File src/ec/google/chromeec/ec_boardid.c:
https://review.coreboot.org/c/coreboot/+/39018/3/src/ec/google/chromeec/ec_b... PS3, Line 48: google_chromeec_smbios_system_sku What do you think about adding a Kconfig EC_GOOGLE_CHROMEEC_SKUID which pulls in a file ec_skuid.c that provides implementation of google_chromeec_get_board_sku() as well as smbios_system_sku(). Then, we don't need mainboards to provide smbios_system_sku() just to call google_chromeec_smbios_system_sku(). And this Kconfig can be selected by most recent mainboards.
https://review.coreboot.org/c/coreboot/+/39018/3/src/ec/google/chromeec/ec_b... PS3, Line 50: 255 If SKU can be as big as 0x7FFFFFF why is the string only allowing upto 255?
https://review.coreboot.org/c/coreboot/+/39018/3/src/ec/google/chromeec/ec_b... PS3, Line 53: if ((sku_id == SKU_UNKNOWN) || : (sku_id > CONFIG_EC_GOOGLE_CHROMEEC_SKU_ID_MAX)) { : printk(BIOS_ERR, "%s: Unexpected SKU ID %u\n", : __func__, sku_id); : return ""; : } I am wondering.. is it bad if we just skip this check completely? i.e. just pass whatever we get from google_chromeec_get_board_sku() as sku%u in SMBIOS? Then we don't need another Kconfig for this.
Jett Rink has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39018 )
Change subject: ec/google/chromeec: Introduce SKU_ID helpers ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39018/3/src/ec/google/chromeec/ec_b... File src/ec/google/chromeec/ec_boardid.c:
https://review.coreboot.org/c/coreboot/+/39018/3/src/ec/google/chromeec/ec_b... PS3, Line 53: if ((sku_id == SKU_UNKNOWN) || : (sku_id > CONFIG_EC_GOOGLE_CHROMEEC_SKU_ID_MAX)) { : printk(BIOS_ERR, "%s: Unexpected SKU ID %u\n", : __func__, sku_id); : return ""; : }
I am wondering.. is it bad if we just skip this check completely? i.e. […]
+1 to just passing up the value to SMBIOS since the firmware should no longer care about this value. All decision should be based on the firmware configuration field instead now.
Jett Rink has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39018 )
Change subject: ec/google/chromeec: Introduce SKU_ID helpers ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39018/3/src/ec/google/chromeec/ec_b... File src/ec/google/chromeec/ec_boardid.c:
https://review.coreboot.org/c/coreboot/+/39018/3/src/ec/google/chromeec/ec_b... PS3, Line 50: sku_str Also in decimal form (%u) the value value could be
"sku4294967295" which needs 13 character plus a NULL (so 14 total)
https://review.coreboot.org/c/coreboot/+/39018/3/src/ec/google/chromeec/ec_b... PS3, Line 53: if ((sku_id == SKU_UNKNOWN) || : (sku_id > CONFIG_EC_GOOGLE_CHROMEEC_SKU_ID_MAX)) { : printk(BIOS_ERR, "%s: Unexpected SKU ID %u\n", : __func__, sku_id); : return ""; : }
+1 to just passing up the value to SMBIOS since the firmware should no longer care about this value. […]
Nevermind about the firmware configuration comment here...I still like just passing up the value verbatim though
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39018 )
Change subject: ec/google/chromeec: Introduce SKU_ID helpers ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/c/coreboot/+/39018/3/src/ec/google/chromeec/ec_b... File src/ec/google/chromeec/ec_boardid.c:
https://review.coreboot.org/c/coreboot/+/39018/3/src/ec/google/chromeec/ec_b... PS3, Line 48: google_chromeec_smbios_system_sku
What do you think about adding a Kconfig EC_GOOGLE_CHROMEEC_SKUID which pulls in a file ec_skuid. […]
The trouble is the weak symbol smbios_system_sku is general and not necessarily a Google thing. However I think I have a solution however I will push the prior of that because I want to unblock our factory image.
https://review.coreboot.org/c/coreboot/+/39018/3/src/ec/google/chromeec/ec_b... PS3, Line 50: sku_str
Also in decimal form (%u) the value value could be […]
Thanks for picking this up, silly copy-paste error.
https://review.coreboot.org/c/coreboot/+/39018/3/src/ec/google/chromeec/ec_b... PS3, Line 50: 255
If SKU can be as big as 0x7FFFFFF why is the string only allowing upto 255?
Done
https://review.coreboot.org/c/coreboot/+/39018/3/src/ec/google/chromeec/ec_b... PS3, Line 53: if ((sku_id == SKU_UNKNOWN) || : (sku_id > CONFIG_EC_GOOGLE_CHROMEEC_SKU_ID_MAX)) { : printk(BIOS_ERR, "%s: Unexpected SKU ID %u\n", : __func__, sku_id); : return ""; : }
Nevermind about the firmware configuration comment here... […]
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39018 )
Change subject: ec/google/chromeec: Introduce SKU_ID helpers ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39018/3/src/ec/google/chromeec/ec_b... File src/ec/google/chromeec/ec_boardid.c:
https://review.coreboot.org/c/coreboot/+/39018/3/src/ec/google/chromeec/ec_b... PS3, Line 48: google_chromeec_smbios_system_sku
The trouble is the weak symbol smbios_system_sku is general and not necessarily a Google thing.
I think that should be fine. ec_skuid.c gets included only when a Google board using Chrome EC actually selects it. So, providing a stronger definition of smbios_system_sku() here would impact only those boards.
Hello Aaron Durbin, Jett Rink, Sam McNally, Daniel Kurtz, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39018
to look at the new patch set (#4).
Change subject: ec/google/chromeec: Introduce SKU_ID helpers ......................................................................
ec/google/chromeec: Introduce SKU_ID helpers
The following introduces helpers that, by default, accommodate a larger SKU id space. The following is the rational for that:
Allow INT32_MAX SKU id encodings beyond UINT8_MAX. This allows for the SKU id to accommodate up to 4 bytes however we reserve the highest bit for SKU_UNKNOWN to be encoded.
However, the legacy UINT8_MAX encoding is supported by leveraging the Kconfig by overriding it with the legacy max of 0xff.
Follow ups migrate boards to this common framework.
V.2: Fixup array size && drop sku_id SKU_UNKNOWN check and pass whatever is set to userspace as firmware doesn't care about the value.
BUG=b:149348474 BRANCH=none TEST=tested on hatch.
Change-Id: I805b25465a3b4ee3dc0cbda5feb9e9ea2493ff9e Signed-off-by: Edward O'Callaghan quasisec@google.com --- M src/ec/google/chromeec/Kconfig M src/ec/google/chromeec/Makefile.inc M src/ec/google/chromeec/ec.h M src/ec/google/chromeec/ec_boardid.c A src/ec/google/chromeec/ec_skuid.c 5 files changed, 59 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/39018/4
Hello Aaron Durbin, Jett Rink, Sam McNally, Daniel Kurtz, build bot (Jenkins), Patrick Georgi, Martin Roth, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39018
to look at the new patch set (#5).
Change subject: ec/google/chromeec: Introduce SKU_ID helpers ......................................................................
ec/google/chromeec: Introduce SKU_ID helpers
The following introduces helpers that, by default, accommodate a larger SKU id space. The following is the rational for that:
Allow INT32_MAX SKU id encodings beyond UINT8_MAX. This allows for the SKU id to accommodate up to 4 bytes however we reserve the highest bit for SKU_UNKNOWN to be encoded.
However, the legacy UINT8_MAX encoding is supported by leveraging the Kconfig by overriding it with the legacy max of 0xff.
Follow ups migrate boards to this common framework.
V.2: Fixup array size && drop sku_id SKU_UNKNOWN check and pass whatever is set to userspace as firmware doesn't care about the value.
BUG=b:149348474 BRANCH=none TEST=tested on hatch.
Change-Id: I805b25465a3b4ee3dc0cbda5feb9e9ea2493ff9e Signed-off-by: Edward O'Callaghan quasisec@google.com --- M src/ec/google/chromeec/Kconfig M src/ec/google/chromeec/Makefile.inc M src/ec/google/chromeec/ec.h A src/ec/google/chromeec/ec_skuid.c 4 files changed, 57 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/39018/5
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39018 )
Change subject: ec/google/chromeec: Introduce SKU_ID helpers ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39018/4/src/ec/google/chromeec/Kcon... File src/ec/google/chromeec/Kconfig:
https://review.coreboot.org/c/coreboot/+/39018/4/src/ec/google/chromeec/Kcon... PS4, Line 103: # set to 0xff for LEGACY : config EC_GOOGLE_CHROMEEC_SKUID_MAX : hex : default 0x7fffffff I don't think this is required anymore?
https://review.coreboot.org/c/coreboot/+/39018/4/src/ec/google/chromeec/ec_b... File src/ec/google/chromeec/ec_boardid.c:
https://review.coreboot.org/c/coreboot/+/39018/4/src/ec/google/chromeec/ec_b... PS4, Line 18: #include <console/console.h> : #include <string.h> Are these still required?
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39018 )
Change subject: ec/google/chromeec: Introduce SKU_ID helpers ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39018/4/src/ec/google/chromeec/ec_b... File src/ec/google/chromeec/ec_boardid.c:
https://review.coreboot.org/c/coreboot/+/39018/4/src/ec/google/chromeec/ec_b... PS4, Line 18: #include <console/console.h> : #include <string.h>
Are these still required?
No, I noticed after push.
However is the general concept of the patchset fine now and if so I can dot all the i's?
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39018 )
Change subject: ec/google/chromeec: Introduce SKU_ID helpers ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39018/4/src/ec/google/chromeec/Kcon... File src/ec/google/chromeec/Kconfig:
https://review.coreboot.org/c/coreboot/+/39018/4/src/ec/google/chromeec/Kcon... PS4, Line 103: # set to 0xff for LEGACY : config EC_GOOGLE_CHROMEEC_SKUID_MAX : hex : default 0x7fffffff
I don't think this is required anymore?
It is required because boards need the ability to override to legacy
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39018 )
Change subject: ec/google/chromeec: Introduce SKU_ID helpers ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39018/4/src/ec/google/chromeec/Kcon... File src/ec/google/chromeec/Kconfig:
https://review.coreboot.org/c/coreboot/+/39018/4/src/ec/google/chromeec/Kcon... PS4, Line 103: # set to 0xff for LEGACY : config EC_GOOGLE_CHROMEEC_SKUID_MAX : hex : default 0x7fffffff
It is required because boards need the ability to override to legacy
But where and how is it actually used?
For legacy boards: EC will only pass values upto 0xff and those will get exposed to the OS in SMBIOS.
For new boards: EC will pass values upto 0x7fffffff and those will get exposed to the OS in SMBIOS.
Does coreboot really need to know whether that MAX value is 0xff or 0x7fffffff?
https://review.coreboot.org/c/coreboot/+/39018/4/src/ec/google/chromeec/ec_s... File src/ec/google/chromeec/ec_skuid.c:
https://review.coreboot.org/c/coreboot/+/39018/4/src/ec/google/chromeec/ec_s... PS4, Line 34: google_chromeec_smbios_system_sku If you rename this smbios_system_sku, then you can avoid this https://review.coreboot.org/c/coreboot/+/39019/8/src/mainboard/google/hatch/....
Also, it wouldn't impact any other board since EC_GOOGLE_CHROMEEC_SKUID will not be selected for any board other that Google boards using Chrome EC.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39018 )
Change subject: ec/google/chromeec: Introduce SKU_ID helpers ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39018/4/src/ec/google/chromeec/Kcon... File src/ec/google/chromeec/Kconfig:
https://review.coreboot.org/c/coreboot/+/39018/4/src/ec/google/chromeec/Kcon... PS4, Line 103: # set to 0xff for LEGACY : config EC_GOOGLE_CHROMEEC_SKUID_MAX : hex : default 0x7fffffff
But where and how is it actually used? […]
It was used in google_chromeec_cbi_get_fw_config() however I can drop that check as well and leave everything up to userspace. My intent wasn't to change the behavior to userspace originally.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39018 )
Change subject: ec/google/chromeec: Introduce SKU_ID helpers ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39018/4/src/ec/google/chromeec/Kcon... File src/ec/google/chromeec/Kconfig:
https://review.coreboot.org/c/coreboot/+/39018/4/src/ec/google/chromeec/Kcon... PS4, Line 103: # set to 0xff for LEGACY : config EC_GOOGLE_CHROMEEC_SKUID_MAX : hex : default 0x7fffffff
It was used in google_chromeec_cbi_get_fw_config() however I can drop that check as well and leave e […]
Nothing is using fw_config in userspace yet. So, that shouldn't be a problem.
Hello Aaron Durbin, Jett Rink, Sam McNally, Daniel Kurtz, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39018
to look at the new patch set (#6).
Change subject: ec/google/chromeec: Introduce SKU_ID helpers ......................................................................
ec/google/chromeec: Introduce SKU_ID helpers
The following introduces helpers that, by default, accommodate a larger SKU id space. The following is the rational for that:
Allow INT32_MAX SKU id encodings beyond UINT8_MAX. This allows for the SKU id to accommodate up to 4 bytes however we reserve the highest bit for SKU_UNKNOWN to be encoded.
However, the legacy UINT8_MAX encoding is supported by leveraging the Kconfig by overriding it with the legacy max of 0xff.
Follow ups migrate boards to this common framework.
V.2: Fixup array size && drop sku_id SKU_UNKNOWN check and pass whatever is set to userspace as firmware doesn't care about the value.
BUG=b:149348474 BRANCH=none TEST=tested on hatch.
Change-Id: I805b25465a3b4ee3dc0cbda5feb9e9ea2493ff9e Signed-off-by: Edward O'Callaghan quasisec@google.com --- M src/ec/google/chromeec/Kconfig M src/ec/google/chromeec/Makefile.inc M src/ec/google/chromeec/ec.h A src/ec/google/chromeec/ec_skuid.c 4 files changed, 53 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/39018/6
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39018 )
Change subject: ec/google/chromeec: Introduce SKU_ID helpers ......................................................................
Patch Set 6:
(2 comments)
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39018/4/src/ec/google/chromeec/Kcon... File src/ec/google/chromeec/Kconfig:
https://review.coreboot.org/c/coreboot/+/39018/4/src/ec/google/chromeec/Kcon... PS4, Line 103: # set to 0xff for LEGACY : config EC_GOOGLE_CHROMEEC_SKUID_MAX : hex : default 0x7fffffff
Nothing is using fw_config in userspace yet. So, that shouldn't be a problem.
Ack
https://review.coreboot.org/c/coreboot/+/39018/4/src/ec/google/chromeec/ec_s... File src/ec/google/chromeec/ec_skuid.c:
https://review.coreboot.org/c/coreboot/+/39018/4/src/ec/google/chromeec/ec_s... PS4, Line 34: google_chromeec_smbios_system_sku
If you rename this smbios_system_sku, then you can avoid this https://review.coreboot. […]
Yes, to get rid of the wrapping indirect, what I was saying is that the the function is named without the googleness in mind in the general code path of arch/x86/smbios.c and wanted to keep it so that it isn't over-fitted to our needs.
Do you mind if we can treat that as a separate bug and scope those changes to that? The original bug b/149348474 already has very large scope runaway. I think these changes already create 90% of the ground work to polishing that off any way.
Hello Aaron Durbin, Jett Rink, Sam McNally, Daniel Kurtz, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39018
to look at the new patch set (#7).
Change subject: ec/google/chromeec: Introduce SKU_ID helpers ......................................................................
ec/google/chromeec: Introduce SKU_ID helpers
The following introduces helpers that, by default, accommodate a larger SKU id space. The following is the rational for that:
Allow INT32_MAX SKU id encodings beyond UINT8_MAX. This allows for the SKU id to accommodate up to 4 bytes however we reserve the highest bit for SKU_UNKNOWN to be encoded.
However, the legacy UINT8_MAX encoding is supported by leveraging the Kconfig by overriding it with the legacy max of 0xff.
Follow ups migrate boards to this common framework.
V.2: Fixup array size && drop sku_id SKU_UNKNOWN check and pass whatever is set to userspace as firmware doesn't care about the value.
BUG=b:149348474 BRANCH=none TEST=tested on hatch.
Change-Id: I805b25465a3b4ee3dc0cbda5feb9e9ea2493ff9e Signed-off-by: Edward O'Callaghan quasisec@google.com --- M src/ec/google/chromeec/Kconfig M src/ec/google/chromeec/Makefile.inc M src/ec/google/chromeec/ec.h A src/ec/google/chromeec/ec_skuid.c 4 files changed, 53 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/39018/7
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39018 )
Change subject: ec/google/chromeec: Introduce SKU_ID helpers ......................................................................
Patch Set 7:
(4 comments)
https://review.coreboot.org/c/coreboot/+/39018/4/src/ec/google/chromeec/ec_s... File src/ec/google/chromeec/ec_skuid.c:
https://review.coreboot.org/c/coreboot/+/39018/4/src/ec/google/chromeec/ec_s... PS4, Line 34: google_chromeec_smbios_system_sku
Yes, to get rid of the wrapping indirect, what I was saying is that the the function is named without the googleness in mind in the general code path of arch/x86/smbios.c and wanted to keep it so that it isn't over-fitted to our needs.
Why is googleness required in the general code path? It can still make a call to smbios_system_sku() and instead of mainboard implementing it, implementation for Google boards with Chrome EC will be provided by this file.
Do you mind if we can treat that as a separate bug and scope those changes to that?
Sure, we can go ahead with this change, but to be very honest, I don't think we need to change anything except rename this function as smbios_system_sku(). Example: https://review.coreboot.org/cgit/coreboot.git/tree/src/ec/google/chromeec/ec.... Weak definition is provided in lib/coreboot_table.c, but its definition for Google boards is provided by ec_boardid.c.
In fact, that makes me think -- Why are we even adding google_chromeec_get_board_sku()? Shouldn't google_chromeec_get_board_sku() be just named as sku_id()?
Basically, I am thinking of this file as providing default implementations of common functions handling system SKU for Google boards using Chrome EC. These function names don't need to be Google specific. It is just the implementation that is provided. That will allow us to reduce redundant code across mainboards where they would simply end up calling this EC function.
https://review.coreboot.org/c/coreboot/+/39018/7/src/ec/google/chromeec/ec_s... File src/ec/google/chromeec/ec_skuid.c:
https://review.coreboot.org/c/coreboot/+/39018/7/src/ec/google/chromeec/ec_s... PS7, Line 2: * Use SPDX-License?
https://review.coreboot.org/c/coreboot/+/39018/7/src/ec/google/chromeec/ec_s... PS7, Line 4: Inc LLC
https://review.coreboot.org/c/coreboot/+/39018/7/src/ec/google/chromeec/ec_s... PS7, Line 25: static MAYBE_STATIC_NONZERO since this is added to early stages as well.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39018 )
Change subject: ec/google/chromeec: Introduce SKU_ID helpers ......................................................................
Patch Set 7:
(4 comments)
https://review.coreboot.org/c/coreboot/+/39018/4/src/ec/google/chromeec/ec_s... File src/ec/google/chromeec/ec_skuid.c:
https://review.coreboot.org/c/coreboot/+/39018/4/src/ec/google/chromeec/ec_s... PS4, Line 34: google_chromeec_smbios_system_sku
Yes, to get rid of the wrapping indirect, what I was saying is that the the function is named with […]
I agree, it is simple. I just rather unblock the immediate problem however I will most definitely follow up with cleanups. The current series is needed any way to have intermediate steps that are buildable for boards to be migrated as separate patches. A final patch is needed to collapse it all down to a rename, I will get to it ASAP.
https://review.coreboot.org/c/coreboot/+/39018/7/src/ec/google/chromeec/ec_s... File src/ec/google/chromeec/ec_skuid.c:
https://review.coreboot.org/c/coreboot/+/39018/7/src/ec/google/chromeec/ec_s... PS7, Line 2: *
Use SPDX-License?
Done
https://review.coreboot.org/c/coreboot/+/39018/7/src/ec/google/chromeec/ec_s... PS7, Line 4: Inc
LLC
Ack
https://review.coreboot.org/c/coreboot/+/39018/7/src/ec/google/chromeec/ec_s... PS7, Line 25: static
MAYBE_STATIC_NONZERO since this is added to early stages as well.
Done
Hello Aaron Durbin, Jett Rink, Sam McNally, Daniel Kurtz, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39018
to look at the new patch set (#8).
Change subject: ec/google/chromeec: Introduce SKU_ID helpers ......................................................................
ec/google/chromeec: Introduce SKU_ID helpers
The following introduces helpers that, by default, accommodate a larger SKU id space. The following is the rational for that:
Allow INT32_MAX SKU id encodings beyond UINT8_MAX. This allows for the SKU id to accommodate up to 4 bytes however we reserve the highest bit for SKU_UNKNOWN to be encoded.
However, the legacy UINT8_MAX encoding is supported by leveraging the Kconfig by overriding it with the legacy max of 0xff.
Follow ups migrate boards to this common framework.
V.2: Fixup array size && drop sku_id SKU_UNKNOWN check and pass whatever is set to userspace as firmware doesn't care about the value. V.3: Use SPDX-License header.
BUG=b:149348474 BRANCH=none TEST=tested on hatch.
Change-Id: I805b25465a3b4ee3dc0cbda5feb9e9ea2493ff9e Signed-off-by: Edward O'Callaghan quasisec@google.com --- M src/ec/google/chromeec/Kconfig M src/ec/google/chromeec/Makefile.inc M src/ec/google/chromeec/ec.h A src/ec/google/chromeec/ec_skuid.c 4 files changed, 47 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/39018/8
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39018 )
Change subject: ec/google/chromeec: Introduce SKU_ID helpers ......................................................................
Patch Set 8: Code-Review+2
Hello Aaron Durbin, Jett Rink, Sam McNally, Daniel Kurtz, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39018
to look at the new patch set (#9).
Change subject: ec/google/chromeec: Introduce SKU_ID helpers ......................................................................
ec/google/chromeec: Introduce SKU_ID helpers
The following introduces helpers that, by default, accommodate a larger SKU id space. The following is the rational for that:
Allow INT32_MAX SKU id encodings beyond UINT8_MAX. This allows for the SKU id to accommodate up to 4 bytes however we reserve the highest bit for SKU_UNKNOWN to be encoded.
However, the legacy UINT8_MAX encoding is supported by leveraging the Kconfig by overriding it with the legacy max of 0xff.
Follow ups migrate boards to this common framework.
V.2: Fixup array size && drop sku_id SKU_UNKNOWN check and pass whatever is set to userspace as firmware doesn't care about the value. V.3: Use SPDX-License header.
BUG=b:149348474 BRANCH=none TEST=tested on hatch.
Change-Id: I805b25465a3b4ee3dc0cbda5feb9e9ea2493ff9e Signed-off-by: Edward O'Callaghan quasisec@google.com --- M src/ec/google/chromeec/Kconfig M src/ec/google/chromeec/Makefile.inc M src/ec/google/chromeec/ec.h A src/ec/google/chromeec/ec_skuid.c 4 files changed, 48 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/39018/9
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39018 )
Change subject: ec/google/chromeec: Introduce SKU_ID helpers ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39018/9/src/ec/google/chromeec/Make... File src/ec/google/chromeec/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/39018/9/src/ec/google/chromeec/Make... PS9, Line 9: smm-$(EC_GOOGLE_CHROMEEC_SKUID) += ec_skuid.c Is smm really required?
Hello Aaron Durbin, Jett Rink, Sam McNally, Daniel Kurtz, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39018
to look at the new patch set (#10).
Change subject: ec/google/chromeec: Introduce SKU_ID helpers ......................................................................
ec/google/chromeec: Introduce SKU_ID helpers
The following introduces helpers that, by default, accommodate a larger SKU id space. The following is the rational for that:
Allow INT32_MAX SKU id encodings beyond UINT8_MAX. This allows for the SKU id to accommodate up to 4 bytes however we reserve the highest bit for SKU_UNKNOWN to be encoded.
However, the legacy UINT8_MAX encoding is supported by leveraging the Kconfig by overriding it with the legacy max of 0xff.
Follow ups migrate boards to this common framework.
V.2: Fixup array size && drop sku_id SKU_UNKNOWN check and pass whatever is set to userspace as firmware doesn't care about the value. V.3: Use SPDX-License header.
BUG=b:149348474 BRANCH=none TEST=tested on hatch.
Change-Id: I805b25465a3b4ee3dc0cbda5feb9e9ea2493ff9e Signed-off-by: Edward O'Callaghan quasisec@google.com --- M src/ec/google/chromeec/Kconfig M src/ec/google/chromeec/Makefile.inc M src/ec/google/chromeec/ec.h A src/ec/google/chromeec/ec_skuid.c 4 files changed, 47 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/39018/10
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39018 )
Change subject: ec/google/chromeec: Introduce SKU_ID helpers ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39018/9/src/ec/google/chromeec/Make... File src/ec/google/chromeec/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/39018/9/src/ec/google/chromeec/Make... PS9, Line 9: smm-$(EC_GOOGLE_CHROMEEC_SKUID) += ec_skuid.c
Is smm really required?
yep, I found the typo in the Makefile now, was missing the CONFIG_ prefix :p
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39018 )
Change subject: ec/google/chromeec: Introduce SKU_ID helpers ......................................................................
Patch Set 10: Code-Review+2
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39018 )
Change subject: ec/google/chromeec: Introduce SKU_ID helpers ......................................................................
Patch Set 10:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39018/3/src/ec/google/chromeec/ec_b... File src/ec/google/chromeec/ec_boardid.c:
https://review.coreboot.org/c/coreboot/+/39018/3/src/ec/google/chromeec/ec_b... PS3, Line 48: google_chromeec_smbios_system_sku
The trouble is the weak symbol smbios_system_sku is general and not necessarily a Google thing. […]
Ack
https://review.coreboot.org/c/coreboot/+/39018/4/src/ec/google/chromeec/ec_s... File src/ec/google/chromeec/ec_skuid.c:
https://review.coreboot.org/c/coreboot/+/39018/4/src/ec/google/chromeec/ec_s... PS4, Line 34: google_chromeec_smbios_system_sku
I agree, it is simple. […]
Ack
Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39018 )
Change subject: ec/google/chromeec: Introduce SKU_ID helpers ......................................................................
ec/google/chromeec: Introduce SKU_ID helpers
The following introduces helpers that, by default, accommodate a larger SKU id space. The following is the rational for that:
Allow INT32_MAX SKU id encodings beyond UINT8_MAX. This allows for the SKU id to accommodate up to 4 bytes however we reserve the highest bit for SKU_UNKNOWN to be encoded.
However, the legacy UINT8_MAX encoding is supported by leveraging the Kconfig by overriding it with the legacy max of 0xff.
Follow ups migrate boards to this common framework.
V.2: Fixup array size && drop sku_id SKU_UNKNOWN check and pass whatever is set to userspace as firmware doesn't care about the value. V.3: Use SPDX-License header.
BUG=b:149348474 BRANCH=none TEST=tested on hatch.
Change-Id: I805b25465a3b4ee3dc0cbda5feb9e9ea2493ff9e Signed-off-by: Edward O'Callaghan quasisec@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/39018 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com --- M src/ec/google/chromeec/Kconfig M src/ec/google/chromeec/Makefile.inc M src/ec/google/chromeec/ec.h A src/ec/google/chromeec/ec_skuid.c 4 files changed, 47 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/ec/google/chromeec/Kconfig b/src/ec/google/chromeec/Kconfig index b33864f..554677c 100644 --- a/src/ec/google/chromeec/Kconfig +++ b/src/ec/google/chromeec/Kconfig @@ -100,6 +100,11 @@ hex default 0x0
+config EC_GOOGLE_CHROMEEC_SKUID + def_bool n + help + Provides common routine for reporting the skuid to ChromeOS. + config EC_GOOGLE_CHROMEEC_BOARDNAME depends on EC_GOOGLE_CHROMEEC string "Chrome EC board name for EC" diff --git a/src/ec/google/chromeec/Makefile.inc b/src/ec/google/chromeec/Makefile.inc index 4994480..b57333e 100644 --- a/src/ec/google/chromeec/Makefile.inc +++ b/src/ec/google/chromeec/Makefile.inc @@ -6,6 +6,9 @@ ramstage-$(CONFIG_EC_GOOGLE_CHROMEEC_BOARDID) += ec_boardid.c smm-$(CONFIG_EC_GOOGLE_CHROMEEC_BOARDID) += ec_boardid.c
+romstage-$(CONFIG_EC_GOOGLE_CHROMEEC_SKUID) += ec_skuid.c +ramstage-$(CONFIG_EC_GOOGLE_CHROMEEC_SKUID) += ec_skuid.c + bootblock-y += ec.c bootblock-$(CONFIG_EC_GOOGLE_CHROMEEC_LPC) += ec_lpc.c ramstage-y += ec.c crosec_proto.c vstore.c diff --git a/src/ec/google/chromeec/ec.h b/src/ec/google/chromeec/ec.h index 7341636..f13b510 100644 --- a/src/ec/google/chromeec/ec.h +++ b/src/ec/google/chromeec/ec.h @@ -89,6 +89,9 @@ int google_chromeec_cbi_get_dram_part_num(char *buf, size_t bufsize); int google_chromeec_cbi_get_oem_name(char *buf, size_t bufsize);
+uint32_t google_chromeec_get_board_sku(void); +const char *google_chromeec_smbios_system_sku(void); + /* MEC uses 0x800/0x804 as register/index pair, thus an 8-byte resource. */ #define MEC_EMI_BASE 0x800 #define MEC_EMI_SIZE 8 diff --git a/src/ec/google/chromeec/ec_skuid.c b/src/ec/google/chromeec/ec_skuid.c new file mode 100644 index 0000000..f8fc203 --- /dev/null +++ b/src/ec/google/chromeec/ec_skuid.c @@ -0,0 +1,36 @@ +/* + * This file is part of the coreboot project. + * + * Copyright 2020 The coreboot project Authors. + * + * SPDX-License-Identifier: GPL-2.0-or-later + */ + +#include <stddef.h> +#include <boardid.h> +#include <ec/google/chromeec/ec.h> +#include <console/console.h> +#include <string.h> + +#define SKU_UNKNOWN 0xFFFFFFFF + +uint32_t google_chromeec_get_board_sku(void) +{ + MAYBE_STATIC_NONZERO uint32_t sku_id = SKU_UNKNOWN; + + if (sku_id != SKU_UNKNOWN) + return sku_id; + + if (google_chromeec_cbi_get_sku_id(&sku_id)) + sku_id = SKU_UNKNOWN; + + return sku_id; +} + +const char *google_chromeec_smbios_system_sku(void) +{ + static char sku_str[14]; /* sku{0..2147483647} */ + uint32_t sku_id = google_chromeec_get_board_sku(); + snprintf(sku_str, sizeof(sku_str), "sku%u", sku_id); + return sku_str; +}
-
Edward O'Callaghan (Code Review) -
Furquan Shaikh (Code Review)
-
Jett Rink (Code Review)