Change in ...coreboot[master]: mb/google/octopus: rename get_board_sku and make it overridable
Kevin Chiu has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33448
Change subject: mb/google/octopus: rename get_board_sku and make it overridable ......................................................................
mb/google/octopus: rename get_board_sku and make it overridable
BUG=b:134912735 BRANCH=octopus TEST=emerge-octopus coreboot chromeos-bootimage Change-Id: I1fb7b5eeac48f2cd9c24fa1d3ac3fe4b390762d2 Signed-off-by: Kevin Chiu Kevin.Chiu@quantatw.com --- M src/mainboard/google/octopus/mainboard.c M src/mainboard/google/octopus/variants/baseboard/include/baseboard/variants.h 2 files changed, 4 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/33448/1
diff --git a/src/mainboard/google/octopus/mainboard.c b/src/mainboard/google/octopus/mainboard.c index 4316ffe..31a2fd1 100644 --- a/src/mainboard/google/octopus/mainboard.c +++ b/src/mainboard/google/octopus/mainboard.c @@ -118,7 +118,7 @@ #define SKU_UNKNOWN 0xFFFFFFFF #define SKU_MAX 255
-static uint32_t get_board_sku(void) +uint32_t __weak variant_board_sku(void) { static uint32_t sku_id = SKU_UNKNOWN;
@@ -134,7 +134,7 @@ const char *smbios_system_sku(void) { static char sku_str[7]; /* sku{0..255} */ - uint32_t sku_id = get_board_sku(); + uint32_t sku_id = variant_board_sku();
if ((sku_id == SKU_UNKNOWN) || (sku_id > SKU_MAX)) { printk(BIOS_ERR, "%s: Unexpected SKU ID %u\n", diff --git a/src/mainboard/google/octopus/variants/baseboard/include/baseboard/variants.h b/src/mainboard/google/octopus/variants/baseboard/include/baseboard/variants.h index 9638118..1df6dea 100644 --- a/src/mainboard/google/octopus/variants/baseboard/include/baseboard/variants.h +++ b/src/mainboard/google/octopus/variants/baseboard/include/baseboard/variants.h @@ -34,7 +34,8 @@ const struct lpddr4_cfg *variant_lpddr4_config(void); /* Return memory SKU for the board. */ size_t variant_memory_sku(void); - +/* Return board SKU */ +uint32_t variant_board_sku(void); /* Return ChromeOS gpio table and fill in number of entries. */ const struct cros_gpio *variant_cros_gpios(size_t *num);
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33448 )
Change subject: mb/google/octopus: rename get_board_sku and make it overridable ......................................................................
Patch Set 1: Code-Review+2
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33448 )
Change subject: mb/google/octopus: rename get_board_sku and make it overridable ......................................................................
Patch Set 1: Code-Review-1
(1 comment)
https://review.coreboot.org/#/c/33448/1/src/mainboard/google/octopus/mainboa... File src/mainboard/google/octopus/mainboard.c:
https://review.coreboot.org/#/c/33448/1/src/mainboard/google/octopus/mainboa... PS1, Line 121: __weak It is not clear to me why this is weak. All you need is this to be exposed globally (non static). A rename isn't needed either. Can you please explain? The follow patch just wants the sku id. Nothing more.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33448 )
Change subject: mb/google/octopus: rename get_board_sku and make it overridable ......................................................................
Patch Set 1: Code-Review-1
I would like to know why the function was made __weak as well.
Kevin Chiu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33448 )
Change subject: mb/google/octopus: rename get_board_sku and make it overridable ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/33448/1/src/mainboard/google/octopus/mainboa... File src/mainboard/google/octopus/mainboard.c:
https://review.coreboot.org/#/c/33448/1/src/mainboard/google/octopus/mainboa... PS1, Line 121: __weak
It is not clear to me why this is weak. All you need is this to be exposed globally (non static). […]
Hi Aaron, to have weak attribute was just in case it might need to override in variants. (ex reef case) that's why rename this function name as well because of it might be invoked by variants. thanks.
Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33448 )
Change subject: mb/google/octopus: rename get_board_sku and make it overridable ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/33448/1/src/mainboard/google/octopus/mainboa... File src/mainboard/google/octopus/mainboard.c:
https://review.coreboot.org/#/c/33448/1/src/mainboard/google/octopus/mainboa... PS1, Line 121: __weak
Hi Aaron, […]
I had suggested partners to refer how nami's implementation was and I support to leverage the same function name so partners later are easy to align and refer example codes.
Regarding weak, when I gave +1 I had the same concern with Kevin about allowing variants to override the function implementation even though Octopus would only have one impl. of managing SKU ID. But it would be good to have consistent.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33448 )
Change subject: mb/google/octopus: rename get_board_sku and make it overridable ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/33448/1/src/mainboard/google/octopus/mainboa... File src/mainboard/google/octopus/mainboard.c:
https://review.coreboot.org/#/c/33448/1/src/mainboard/google/octopus/mainboa... PS1, Line 121: __weak
I had suggested partners to refer how nami's implementation was and I support to leverage the same f […]
There is only one way to get the board sku on octopus platforms: cbi. There is no other way currently where a variant needs to change that behavior. The __weak was there for other platforms because there were multiple ways and workarounds required. For octopus designs there isn't a necessity for having multiple ways. The only change I see required is exposing it as global. I agree that consistency is good, but I think in this case the previous implementations needed overridable linkage because of transitions in design at the time and needing to support all those at once.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33448 )
Change subject: mb/google/octopus: rename get_board_sku and make it overridable ......................................................................
Patch Set 1: -Code-Review
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33448 )
Change subject: mb/google/octopus: rename get_board_sku and make it overridable ......................................................................
Patch Set 1:
Thanks Aaron for sharing more details.
Hi Kevin,
Can you please just make get_board_sku as a global function?
Hello Aaron Durbin, Sheng-Liang Pan, Karthik Ramasubramanian, Justin TerAvest, Angel Pons, Marco Chen, Kevin Chiu, build bot (Jenkins), Keith Tzeng,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33448
to look at the new patch set (#2).
Change subject: mb/google/octopus: expose get_board_sku as global ......................................................................
mb/google/octopus: expose get_board_sku as global
BUG=b:134912735 BRANCH=octopus TEST=emerge-octopus coreboot chromeos-bootimage Change-Id: I1fb7b5eeac48f2cd9c24fa1d3ac3fe4b390762d2 Signed-off-by: Kevin Chiu Kevin.Chiu@quantatw.com --- M src/mainboard/google/octopus/mainboard.c M src/mainboard/google/octopus/variants/baseboard/include/baseboard/variants.h 2 files changed, 3 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/33448/2
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33448 )
Change subject: mb/google/octopus: expose get_board_sku as global ......................................................................
Patch Set 2: Code-Review+2
Kevin Chiu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33448 )
Change subject: mb/google/octopus: expose get_board_sku as global ......................................................................
Patch Set 2:
(1 comment)
Patch Set 1:
Thanks Aaron for sharing more details.
Hi Kevin,
Can you please just make get_board_sku as a global function?
done at patch#2, thanks.
https://review.coreboot.org/#/c/33448/1/src/mainboard/google/octopus/mainboa... File src/mainboard/google/octopus/mainboard.c:
https://review.coreboot.org/#/c/33448/1/src/mainboard/google/octopus/mainboa... PS1, Line 121: __weak
There is only one way to get the board sku on octopus platforms: cbi. […]
Done, thank you!
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33448 )
Change subject: mb/google/octopus: expose get_board_sku as global ......................................................................
Patch Set 2: Code-Review+2
Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33448 )
Change subject: mb/google/octopus: expose get_board_sku as global ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/33448/1/src/mainboard/google/octopus/mainboa... File src/mainboard/google/octopus/mainboard.c:
https://review.coreboot.org/#/c/33448/1/src/mainboard/google/octopus/mainboa... PS1, Line 121: __weak
Done, thank you!
My last argument is to have common function in the same name like this get_board_sku / variant_board_sku and it would be good for partners to refer as well and be good once we have plan to come out a doc for topic like 'board customization tips in a baseboard of coreboot'.
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33448 )
Change subject: mb/google/octopus: expose get_board_sku as global ......................................................................
mb/google/octopus: expose get_board_sku as global
BUG=b:134912735 BRANCH=octopus TEST=emerge-octopus coreboot chromeos-bootimage Change-Id: I1fb7b5eeac48f2cd9c24fa1d3ac3fe4b390762d2 Signed-off-by: Kevin Chiu Kevin.Chiu@quantatw.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/33448 Reviewed-by: Aaron Durbin adurbin@chromium.org Reviewed-by: Karthik Ramasubramanian kramasub@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/mainboard/google/octopus/mainboard.c M src/mainboard/google/octopus/variants/baseboard/include/baseboard/variants.h 2 files changed, 3 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved Karthik Ramasubramanian: Looks good to me, approved
diff --git a/src/mainboard/google/octopus/mainboard.c b/src/mainboard/google/octopus/mainboard.c index 4316ffe..91cf1e4 100644 --- a/src/mainboard/google/octopus/mainboard.c +++ b/src/mainboard/google/octopus/mainboard.c @@ -118,7 +118,7 @@ #define SKU_UNKNOWN 0xFFFFFFFF #define SKU_MAX 255
-static uint32_t get_board_sku(void) +uint32_t get_board_sku(void) { static uint32_t sku_id = SKU_UNKNOWN;
diff --git a/src/mainboard/google/octopus/variants/baseboard/include/baseboard/variants.h b/src/mainboard/google/octopus/variants/baseboard/include/baseboard/variants.h index 9638118..5374ace 100644 --- a/src/mainboard/google/octopus/variants/baseboard/include/baseboard/variants.h +++ b/src/mainboard/google/octopus/variants/baseboard/include/baseboard/variants.h @@ -34,7 +34,8 @@ const struct lpddr4_cfg *variant_lpddr4_config(void); /* Return memory SKU for the board. */ size_t variant_memory_sku(void); - +/* Return board SKU */ +uint32_t get_board_sku(void); /* Return ChromeOS gpio table and fill in number of entries. */ const struct cros_gpio *variant_cros_gpios(size_t *num);
-
Aaron Durbin (Code Review) -
Angel Pons (Code Review)
-
Karthik Ramasubramanian (Code Review)
-
Kevin Chiu (Code Review)
-
Marco Chen (Code Review)
-
Patrick Georgi (Code Review)