Change in ...coreboot[master]: mb/google/octopus: Override VBT selection for Garg

[Kevin Chiu (Code Review)]

Hello Kevin Chiu,

I'd like you to do a code review. Please visit

https://review.coreboot.org/c/coreboot/+/33395

to review the following change.

Change subject: mb/google/octopus: Override VBT selection for Garg
......................................................................

mb/google/octopus: Override VBT selection for Garg

Garg proto build has 3 SKUs:
 garg 2A2C DB: SKU ID - 1
 garg HDMI DB: SKU ID - 9
 garg LTE  DB: SKU ID - 17

For SKU#9, VBT will need to be overridden to enable DDI_C output to HDMI

BUG=b:134912735
BRANCH=octopus
TEST=emerge-octopus coreboot chromeos-bootimage
Change-Id: I6c0ec086496eaf217ea8e326f5084d886d0e698f
Signed-off-by: Kevin Chiu 
---
M src/mainboard/google/octopus/variants/garg/Makefile.inc
A src/mainboard/google/octopus/variants/garg/mainboard.c
2 files changed, 41 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/33395/1

diff --git a/src/mainboard/google/octopus/variants/garg/Makefile.inc b/src/mainboard/google/octopus/variants/garg/Makefile.inc
index 9fb63f5..5b657f4 100644
--- a/src/mainboard/google/octopus/variants/garg/Makefile.inc
+++ b/src/mainboard/google/octopus/variants/garg/Makefile.inc
@@ -1,3 +1,3 @@
 bootblock-y += gpio.c
-
+ramstage-y += mainboard.c
 ramstage-y += gpio.c
diff --git a/src/mainboard/google/octopus/variants/garg/mainboard.c b/src/mainboard/google/octopus/variants/garg/mainboard.c
new file mode 100644
index 0000000..73f0ae5
--- /dev/null
+++ b/src/mainboard/google/octopus/variants/garg/mainboard.c
@@ -0,0 +1,40 @@
+/*
+ * 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.
+ */
+
+#include <boardid.h>
+#include <ec/google/chromeec/ec.h>
+#include <drivers/intel/gma/opregion.h>
+
+enum {
+	SKU_1_2A2C = 1,
+	SKU_9_HDMI = 9,
+	SKU_17_LTE = 17,
+};
+
+const char *mainboard_vbt_filename(void)
+{
+	uint32_t sku_id;
+
+	if (google_chromeec_cbi_get_sku_id(&sku_id))
+		sku_id = -1;
+
+	switch (sku_id) {
+	case SKU_9_HDMI:
+		return "vbt-garg.bin";
+	default:
+		return "vbt.bin";
+		break;
+	}
+}

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33395 )

Change subject: mb/google/octopus: Override VBT selection for Garg
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/33395/1/src/mainboard/google/octopus/variant... 
File src/mainboard/google/octopus/variants/garg/mainboard.c:

https://review.coreboot.org/#/c/33395/1/src/mainboard/google/octopus/variant... 
PS1, Line 38: 		break;
break is not useful after a goto or return

[Kevin Chiu (Code Review)]

Kevin Chiu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33395 )

Change subject: mb/google/octopus: Override VBT selection for Garg
......................................................................

Patch Set 3:

This change is ready for review.

[Karthik Ramasubramanian (Code Review)]

Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33395 )

Change subject: mb/google/octopus: Override VBT selection for Garg
......................................................................

Patch Set 3:

(4 comments)

https://review.coreboot.org/#/c/33395/3/src/mainboard/google/octopus/variant... 
File src/mainboard/google/octopus/variants/garg/Makefile.inc:

https://review.coreboot.org/#/c/33395/3/src/mainboard/google/octopus/variant... 
PS3, Line 2: ramstage-y += mainboard.c
Move this line after ramstage-y += gpio.c

https://review.coreboot.org/#/c/33395/3/src/mainboard/google/octopus/variant... 
File src/mainboard/google/octopus/variants/garg/mainboard.c:

https://review.coreboot.org/#/c/33395/3/src/mainboard/google/octopus/variant... 
PS3, Line 1: /*
Rename this file to variant.c

https://review.coreboot.org/#/c/33395/3/src/mainboard/google/octopus/variant... 
PS3, Line 4: 2018
2019

https://review.coreboot.org/#/c/33395/3/src/mainboard/google/octopus/variant... 
PS3, Line 30: google_chromeec_cbi_get_sku_id
Can you please use get_board_sku from src/mainboard/google/octopus/mainboard.c. That way it can return the cached sku id information without checking with EC.

[Kevin Chiu (Code Review)]

Hello Sheng-Liang Pan, Karthik Ramasubramanian, Justin TerAvest, 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/+/33395

to look at the new patch set (#4).

Change subject: mb/google/octopus: Override VBT selection for Garg
......................................................................

mb/google/octopus: Override VBT selection for Garg

Garg proto build has 3 SKUs:
 garg 2A2C DB: SKU ID - 1
 garg HDMI DB: SKU ID - 9
 garg LTE  DB: SKU ID - 17

For SKU#9, VBT will need to be overridden to enable DDI_C output to HDMI

BUG=b:134912735
BRANCH=octopus
TEST=emerge-octopus coreboot chromeos-bootimage
Cq-Depend: chrome-internal:1380847

Change-Id: I6c0ec086496eaf217ea8e326f5084d886d0e698f
Signed-off-by: Kevin Chiu 
---
M src/mainboard/google/octopus/variants/garg/Makefile.inc
A src/mainboard/google/octopus/variants/garg/variant.c
2 files changed, 40 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/33395/4

[Kevin Chiu (Code Review)]

Kevin Chiu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33395 )

Change subject: mb/google/octopus: Override VBT selection for Garg
......................................................................

Patch Set 4:

(3 comments)

https://review.coreboot.org/#/c/33395/3/src/mainboard/google/octopus/variant... 
File src/mainboard/google/octopus/variants/garg/mainboard.c:

https://review.coreboot.org/#/c/33395/3/src/mainboard/google/octopus/variant... 
PS3, Line 1: /*
> Rename this file to variant. […]

Done

https://review.coreboot.org/#/c/33395/3/src/mainboard/google/octopus/variant... 
PS3, Line 4: 2018
> 2019

Done

https://review.coreboot.org/#/c/33395/3/src/mainboard/google/octopus/variant... 
PS3, Line 30: google_chromeec_cbi_get_sku_id
> Can you please use get_board_sku from src/mainboard/google/octopus/mainboard.c. […]

Hi Karthik,
it looks to me get_board_sku will do the same thing (invoke google_chromeec_cbi_get_sku_id as well).
besides get_board_sku is a static function, it won't be able to call from outer files.
thanks.

[Kevin Chiu (Code Review)]

Hello Sheng-Liang Pan, Karthik Ramasubramanian, Justin TerAvest, 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/+/33395

to look at the new patch set (#5).

Change subject: mb/google/octopus: Override VBT selection for Garg
......................................................................

mb/google/octopus: Override VBT selection for Garg

Garg proto build has 3 SKUs:
 garg 2A2C DB: SKU ID - 1
 garg HDMI DB: SKU ID - 9
 garg LTE  DB: SKU ID - 17

For SKU#9, VBT will need to be overridden to enable DDI_C output to HDMI

BUG=b:134912735
BRANCH=octopus
TEST=emerge-octopus coreboot chromeos-bootimage
Cq-Depend: chrome-internal:1380847

Change-Id: I6c0ec086496eaf217ea8e326f5084d886d0e698f
Signed-off-by: Kevin Chiu 
---
M src/mainboard/google/octopus/variants/garg/Makefile.inc
A src/mainboard/google/octopus/variants/garg/variant.c
2 files changed, 40 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/33395/5

[Marco Chen (Code Review)]

Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33395 )

Change subject: mb/google/octopus: Override VBT selection for Garg
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/#/c/33395/3/src/mainboard/google/octopus/variant... 
File src/mainboard/google/octopus/variants/garg/mainboard.c:

https://review.coreboot.org/#/c/33395/3/src/mainboard/google/octopus/variant... 
PS3, Line 30: google_chromeec_cbi_get_sku_id
> Hi Karthik, […]

This should be similar function like variant_board_sku which was in multiple platforms like nami and reef. And the header file associated to it is various.h in baseboard dir.

As a result, we can just rename it to variant_board_sku and remove the static limitation then all others can call it instead of google_chromeec_cbi_get_sku_id with additional check.

[Kevin Chiu (Code Review)]

Hello Sheng-Liang Pan, Karthik Ramasubramanian, Justin TerAvest, 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/+/33395

to look at the new patch set (#6).

Change subject: mb/google/octopus: Override VBT selection for Garg
......................................................................

mb/google/octopus: Override VBT selection for Garg

Garg proto build has 3 SKUs:
 garg 2A2C DB: SKU ID - 1
 garg HDMI DB: SKU ID - 9
 garg LTE  DB: SKU ID - 17

For SKU#9, VBT will need to be overridden to enable DDI_C output to HDMI

BUG=b:134912735
BRANCH=octopus
TEST=emerge-octopus coreboot chromeos-bootimage
Cq-Depend: chrome-internal:1380847

Change-Id: I6c0ec086496eaf217ea8e326f5084d886d0e698f
Signed-off-by: Kevin Chiu 
---
M src/mainboard/google/octopus/mainboard.c
M src/mainboard/google/octopus/variants/baseboard/include/baseboard/variants.h
M src/mainboard/google/octopus/variants/garg/Makefile.inc
A src/mainboard/google/octopus/variants/garg/variant.c
4 files changed, 44 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/33395/6

[Kevin Chiu (Code Review)]

Kevin Chiu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33395 )

Change subject: mb/google/octopus: Override VBT selection for Garg
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/#/c/33395/3/src/mainboard/google/octopus/variant... 
File src/mainboard/google/octopus/variants/garg/mainboard.c:

https://review.coreboot.org/#/c/33395/3/src/mainboard/google/octopus/variant... 
PS3, Line 30: google_chromeec_cbi_get_sku_id
> This should be similar function like variant_board_sku which was in multiple platforms like nami and […]

Done at patch#6

[Sheng-Liang Pan (Code Review)]

Sheng-Liang Pan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33395 )

Change subject: mb/google/octopus: Override VBT selection for Garg
......................................................................

Patch Set 6: Code-Review+1

[Marco Chen (Code Review)]

Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33395 )

Change subject: mb/google/octopus: Override VBT selection for Garg
......................................................................

Patch Set 6: Code-Review+1

[Karthik Ramasubramanian (Code Review)]

Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33395 )

Change subject: mb/google/octopus: Override VBT selection for Garg
......................................................................

Patch Set 6:

(2 comments)

https://review.coreboot.org/#/c/33395/6//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/33395/6//COMMIT_MSG@19 
PS6, Line 19: Cq-Depend: chrome-internal:1380847
I don't think Cq-Depend has any meaning in the upstream repository. Can someone please confirm if my understanding is correct? Also even if you are adding it, it should be grouped as part of Change-Id paragraph.

https://review.coreboot.org/#/c/33395/6/src/mainboard/google/octopus/mainboa... 
File src/mainboard/google/octopus/mainboard.c:

https://review.coreboot.org/#/c/33395/6/src/mainboard/google/octopus/mainboa... 
PS6, Line 121: uint32_t variant_board_sku(void)
Ideally we would want this i.e. exporting variant_board_sku function as a separate patch. That way we can keep VBT selection independent. Can you please split it up?

[Marco Chen (Code Review)]

Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33395 )

Change subject: mb/google/octopus: Override VBT selection for Garg
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/#/c/33395/6//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/33395/6//COMMIT_MSG@19 
PS6, Line 19: Cq-Depend: chrome-internal:1380847
> I don't think Cq-Depend has any meaning in the upstream repository. […]

I did see old syntax - CQ-DEPEND in the git log of coreboot in chromium side and it should still work after cherry-picking it to chromium side.

[Kevin Chiu (Code Review)]

Hello Sheng-Liang Pan, Karthik Ramasubramanian, Justin TerAvest, 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/+/33395

to look at the new patch set (#7).

Change subject: mb/google/octopus: Override VBT selection for Garg
......................................................................

mb/google/octopus: Override VBT selection for Garg

Garg proto build has 3 SKUs:
 garg 2A2C DB: SKU ID - 1
 garg HDMI DB: SKU ID - 9
 garg LTE  DB: SKU ID - 17

For SKU#9, VBT will need to be overridden to enable DDI_C output to HDMI

BUG=b:134912735
BRANCH=octopus
TEST=emerge-octopus coreboot chromeos-bootimage
Cq-Depend: chrome-internal:1380847

Change-Id: I6c0ec086496eaf217ea8e326f5084d886d0e698f
Signed-off-by: Kevin Chiu 
---
M src/mainboard/google/octopus/variants/garg/Makefile.inc
A src/mainboard/google/octopus/variants/garg/variant.c
2 files changed, 40 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/33395/7

[Kevin Chiu (Code Review)]

Kevin Chiu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33395 )

Change subject: mb/google/octopus: Override VBT selection for Garg
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/#/c/33395/6/src/mainboard/google/octopus/mainboa... 
File src/mainboard/google/octopus/mainboard.c:

https://review.coreboot.org/#/c/33395/6/src/mainboard/google/octopus/mainboa... 
PS6, Line 121: uint32_t variant_board_sku(void)
> Ideally we would want this i.e. exporting variant_board_sku function as a separate patch. […]

done at https://review.coreboot.org/c/coreboot/+/33448

[Karthik Ramasubramanian (Code Review)]

Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33395 )

Change subject: mb/google/octopus: Override VBT selection for Garg
......................................................................

Patch Set 7: Code-Review+2

[Marco Chen (Code Review)]

Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33395 )

Change subject: mb/google/octopus: Override VBT selection for Garg
......................................................................

Patch Set 7: Code-Review+1

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33395 )

Change subject: mb/google/octopus: Override VBT selection for Garg
......................................................................

Patch Set 7: Code-Review-1

see previous patch

[Kevin Chiu (Code Review)]

Hello Aaron Durbin, Sheng-Liang Pan, Karthik Ramasubramanian, Justin TerAvest, 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/+/33395

to look at the new patch set (#8).

Change subject: mb/google/octopus: Override VBT selection for Garg
......................................................................

mb/google/octopus: Override VBT selection for Garg

Garg proto build has 3 SKUs:
 garg 2A2C DB: SKU ID - 1
 garg HDMI DB: SKU ID - 9
 garg LTE  DB: SKU ID - 17

For SKU#9, VBT will need to be overridden to enable DDI_C output to HDMI

BUG=b:134912735
BRANCH=octopus
TEST=emerge-octopus coreboot chromeos-bootimage
Cq-Depend: chrome-internal:1380847

Change-Id: I6c0ec086496eaf217ea8e326f5084d886d0e698f
Signed-off-by: Kevin Chiu 
---
M src/mainboard/google/octopus/variants/garg/Makefile.inc
A src/mainboard/google/octopus/variants/garg/variant.c
2 files changed, 40 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/95/33395/8

[Kevin Chiu (Code Review)]

Kevin Chiu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33395 )

Change subject: mb/google/octopus: Override VBT selection for Garg
......................................................................

Patch Set 8:
> Patch Set 7: Code-Review-1
>
> see previous patch

Hi Aaron, restore to "get_board_sku" at patch#8. thank you.

[Karthik Ramasubramanian (Code Review)]

Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33395 )

Change subject: mb/google/octopus: Override VBT selection for Garg
......................................................................

Patch Set 8: Code-Review+2

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33395 )

Change subject: mb/google/octopus: Override VBT selection for Garg
......................................................................

mb/google/octopus: Override VBT selection for Garg

Garg proto build has 3 SKUs:
 garg 2A2C DB: SKU ID - 1
 garg HDMI DB: SKU ID - 9
 garg LTE  DB: SKU ID - 17

For SKU#9, VBT will need to be overridden to enable DDI_C output to HDMI

BUG=b:134912735
BRANCH=octopus
TEST=emerge-octopus coreboot chromeos-bootimage
Cq-Depend: chrome-internal:1380847

Change-Id: I6c0ec086496eaf217ea8e326f5084d886d0e698f
Signed-off-by: Kevin Chiu 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/33395
Tested-by: build bot (Jenkins) 
Reviewed-by: Karthik Ramasubramanian 
---
M src/mainboard/google/octopus/variants/garg/Makefile.inc
A src/mainboard/google/octopus/variants/garg/variant.c
2 files changed, 40 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Karthik Ramasubramanian: Looks good to me, approved

diff --git a/src/mainboard/google/octopus/variants/garg/Makefile.inc b/src/mainboard/google/octopus/variants/garg/Makefile.inc
index 9fb63f5..ba865e9 100644
--- a/src/mainboard/google/octopus/variants/garg/Makefile.inc
+++ b/src/mainboard/google/octopus/variants/garg/Makefile.inc
@@ -1,3 +1,4 @@
 bootblock-y += gpio.c

ramstage-y += gpio.c
+ramstage-y += variant.c
diff --git a/src/mainboard/google/octopus/variants/garg/variant.c b/src/mainboard/google/octopus/variants/garg/variant.c
new file mode 100644
index 0000000..a3efe3f
--- /dev/null
+++ b/src/mainboard/google/octopus/variants/garg/variant.c
@@ -0,0 +1,39 @@
+/*
+ * 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 <ec/google/chromeec/ec.h>
+#include <drivers/intel/gma/opregion.h>
+#include <baseboard/variants.h>
+
+enum {
+	SKU_1_2A2C = 1,
+	SKU_9_HDMI = 9,
+	SKU_17_LTE = 17,
+};
+
+const char *mainboard_vbt_filename(void)
+{
+	uint32_t sku_id;
+
+	sku_id = get_board_sku();
+
+	switch (sku_id) {
+	case SKU_9_HDMI:
+		return "vbt_garg_hdmi.bin";
+	default:
+		return "vbt.bin";
+	}
+}