Change in coreboot[master]: mb/google/octopus/variants/garg: update new LTE SKU

[Kevin Chiu (Code Review)]

Hello Kevin Chiu,

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

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

to review the following change.

Change subject: mb/google/octopus/variants/garg: update new LTE SKU
......................................................................

mb/google/octopus/variants/garg: update new LTE SKU

Add new SKU definition:
Garg360 (LTE DB, touch, no stylus, rear camera) SKU ID - 39

BUG=b:170708728
BRANCH=octopus
TEST=emerge-octopus coreboot chromeos-bootimage

Change-Id: Ifec4e1360bd1aff3825bc6413b0a2ccd8b822075
Signed-off-by: Kevin Chiu 
---
M src/mainboard/google/octopus/variants/garg/gpio.c
M src/mainboard/google/octopus/variants/garg/include/variant/sku.h
M src/mainboard/google/octopus/variants/garg/variant.c
3 files changed, 4 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/15/47015/1

diff --git a/src/mainboard/google/octopus/variants/garg/gpio.c b/src/mainboard/google/octopus/variants/garg/gpio.c
index 67e1056..7ae59f8 100644
--- a/src/mainboard/google/octopus/variants/garg/gpio.c
+++ b/src/mainboard/google/octopus/variants/garg/gpio.c
@@ -71,6 +71,7 @@
    	return hdmi_override_table;
    case SKU_17_LTE:
    case SKU_18_LTE_TS:
+	case SKU_39_2A2C_360_LTE_TS_STYLUES:
    	*num = ARRAY_SIZE(lte_override_table);
    	return lte_override_table;
    default:
diff --git a/src/mainboard/google/octopus/variants/garg/include/variant/sku.h b/src/mainboard/google/octopus/variants/garg/include/variant/sku.h
index 8fc63cc..b325801 100644
--- a/src/mainboard/google/octopus/variants/garg/include/variant/sku.h
+++ b/src/mainboard/google/octopus/variants/garg/include/variant/sku.h
@@ -12,6 +12,7 @@
    SKU_20_2A2C_TS = 20,
    SKU_37_2A2C_360 = 37,
    SKU_38_2A2C_360_TS_NO_STYLUES = 38,
+	SKU_39_2A2C_360_LTE_TS_STYLUES = 39,
    SKU_49_2A2C_TS = 49,
    SKU_50_HDMI = 50,
    SKU_51_2A2C = 51,
diff --git a/src/mainboard/google/octopus/variants/garg/variant.c b/src/mainboard/google/octopus/variants/garg/variant.c
index 0a6574d..a0ae71f 100644
--- a/src/mainboard/google/octopus/variants/garg/variant.c
+++ b/src/mainboard/google/octopus/variants/garg/variant.c
@@ -37,6 +37,7 @@
    switch (google_chromeec_get_board_sku()) {
    case SKU_17_LTE:
    case SKU_18_LTE_TS:
+	case SKU_39_2A2C_360_LTE_TS_STYLUES:
    	power_off_lte_module();
    	return;
    default:
@@ -54,6 +55,7 @@
    	switch (google_chromeec_get_board_sku()) {
    	case SKU_17_LTE:
    	case SKU_18_LTE_TS:
+		case SKU_39_2A2C_360_LTE_TS_STYLUES:
    		cfg->disable_xhci_lfps_pm = 1;
    		return;
    	default:

[Marco Chen (Code Review)]

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

Change subject: mb/google/octopus/variants/garg: update new LTE SKU
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47015/1/src/mainboard/google/octopu... 
File src/mainboard/google/octopus/variants/garg/variant.c:

https://review.coreboot.org/c/coreboot/+/47015/1/src/mainboard/google/octopu... 
PS1, Line 58: 		case SKU_39_2A2C_360_LTE_TS_STYLUES:
It would be a good time to introduce a new function of checking whether it is the SKU of LTE so two checks here and the one in gpio.c can all leverage this new function?

[Kevin Chiu (Code Review)]

Hello Kevin Chiu, build bot (Jenkins), Martin Roth, Furquan Shaikh, Henry Sun, Marco Chen, Keith Tzeng,

I'd like you to reexamine a change. Please visit

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

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

Change subject: mb/google/octopus/variants/garg: update new LTE SKU
......................................................................

mb/google/octopus/variants/garg: update new LTE SKU

Add new SKU definition:
Garg360 (LTE DB,1A2C,TS, no stylus, rear camera) SKU ID - 39

BUG=b:170708728
BRANCH=octopus
TEST=emerge-octopus coreboot chromeos-bootimage

Change-Id: Ifec4e1360bd1aff3825bc6413b0a2ccd8b822075
Signed-off-by: Kevin Chiu 
---
M src/mainboard/google/octopus/variants/garg/gpio.c
M src/mainboard/google/octopus/variants/garg/include/variant/sku.h
M src/mainboard/google/octopus/variants/garg/variant.c
3 files changed, 4 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/15/47015/2

[Kevin Chiu (Code Review)]

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

Change subject: mb/google/octopus/variants/garg: update new LTE SKU
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47015/1/src/mainboard/google/octopu... 
File src/mainboard/google/octopus/variants/garg/variant.c:

https://review.coreboot.org/c/coreboot/+/47015/1/src/mainboard/google/octopu... 
PS1, Line 58: 		case SKU_39_2A2C_360_LTE_TS_STYLUES:
> It would be a good time to introduce a new function of checking whether it is the SKU of LTE so two  […]

Hi Marco,
since mainboard_misc.c was removed and the sku id matters were moved to common ec_skuid.c.
I suppose it'll be good to keep current format or are you prefer to add mainboard_misc.c back?
thanks.

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47015 )

Change subject: mb/google/octopus/variants/garg: update new LTE SKU
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47015/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/47015/2//COMMIT_MSG@7 
PS2, Line 7: update
Add

Change in coreboot[master]: mb/google/octopus/variants/garg: Add new LTE SKU

[Kevin Chiu (Code Review)]

Hello Kevin Chiu, build bot (Jenkins), Martin Roth, Furquan Shaikh, Henry Sun, Justin TerAvest, Marco Chen, Keith Tzeng, Karthik Ramasubramanian,

I'd like you to reexamine a change. Please visit

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

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

Change subject: mb/google/octopus/variants/garg: Add new LTE SKU
......................................................................

mb/google/octopus/variants/garg: Add new LTE SKU

Add new SKU definition:
Garg360 (LTE DB,1A2C,TS, no stylus, rear camera) SKU ID - 39

BUG=b:170708728
BRANCH=octopus
TEST=emerge-octopus coreboot chromeos-bootimage

Change-Id: Ifec4e1360bd1aff3825bc6413b0a2ccd8b822075
Signed-off-by: Kevin Chiu 
---
M src/mainboard/google/octopus/variants/garg/gpio.c
M src/mainboard/google/octopus/variants/garg/include/variant/sku.h
M src/mainboard/google/octopus/variants/garg/variant.c
3 files changed, 4 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/15/47015/3

Change in coreboot[master]: mb/google/octopus/variants/garg: Add new LTE SKU

[Kevin Chiu (Code Review)]

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

Change subject: mb/google/octopus/variants/garg: Add new LTE SKU
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47015/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/47015/2//COMMIT_MSG@7 
PS2, Line 7: update
> Add

Done

Change in coreboot[master]: mb/google/octopus/variants/garg: Add new LTE SKU

[Marco Chen (Code Review)]

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

Change subject: mb/google/octopus/variants/garg: Add new LTE SKU
......................................................................

Patch Set 3: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/47015/1/src/mainboard/google/octopu... 
File src/mainboard/google/octopus/variants/garg/variant.c:

https://review.coreboot.org/c/coreboot/+/47015/1/src/mainboard/google/octopu... 
PS1, Line 58: 		case SKU_39_2A2C_360_LTE_TS_STYLUES:
> Hi Marco, […]

Sorry to not give the suggestion clearly. What I mean is to have a function inside variants/garg to judge whether the SKU of this DUT is LTE or not like [1].

Due to the urgent schedule and not critical of this improvement, I would +2 this time first.

[1] https://chromium.googlesource.com/chromiumos/third_party/coreboot/+/refs/hea...

Change in coreboot[master]: mb/google/octopus/variants/garg: Add new LTE SKU

[Kevin Chiu (Code Review)]

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

Change subject: mb/google/octopus/variants/garg: Add new LTE SKU
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47015/1/src/mainboard/google/octopu... 
File src/mainboard/google/octopus/variants/garg/variant.c:

https://review.coreboot.org/c/coreboot/+/47015/1/src/mainboard/google/octopu... 
PS1, Line 58: 		case SKU_39_2A2C_360_LTE_TS_STYLUES:
> Sorry to not give the suggestion clearly. […]

Thank you, Marco!
and could you please help merge this one? thank you!

Change in coreboot[master]: mb/google/octopus/variants/garg: Add new LTE SKU

[Kevin Chiu (Code Review)]

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

Change subject: mb/google/octopus/variants/garg: Add new LTE SKU
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47015/1/src/mainboard/google/octopu... 
File src/mainboard/google/octopus/variants/garg/variant.c:

https://review.coreboot.org/c/coreboot/+/47015/1/src/mainboard/google/octopu... 
PS1, Line 58: 		case SKU_39_2A2C_360_LTE_TS_STYLUES:
> Thank you, Marco! […]

Done

Change in coreboot[master]: mb/google/octopus/variants/garg: Add new LTE SKU

[Kevin Chiu (Code Review)]

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

Change subject: mb/google/octopus/variants/garg: Add new LTE SKU
......................................................................

Patch Set 3:

hi reviewers,
could you please help merge this patch if no further concern?
thank you!

Change in coreboot[master]: mb/google/octopus/variants/garg: Add new LTE SKU

[Furquan Shaikh (Code Review)]

Furquan Shaikh has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47015 )

Change subject: mb/google/octopus/variants/garg: Add new LTE SKU
......................................................................

mb/google/octopus/variants/garg: Add new LTE SKU

Add new SKU definition:
Garg360 (LTE DB,1A2C,TS, no stylus, rear camera) SKU ID - 39

BUG=b:170708728
BRANCH=octopus
TEST=emerge-octopus coreboot chromeos-bootimage

Change-Id: Ifec4e1360bd1aff3825bc6413b0a2ccd8b822075
Signed-off-by: Kevin Chiu 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/47015
Reviewed-by: Marco Chen 
Tested-by: build bot (Jenkins) 
---
M src/mainboard/google/octopus/variants/garg/gpio.c
M src/mainboard/google/octopus/variants/garg/include/variant/sku.h
M src/mainboard/google/octopus/variants/garg/variant.c
3 files changed, 4 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Marco Chen: Looks good to me, approved

diff --git a/src/mainboard/google/octopus/variants/garg/gpio.c b/src/mainboard/google/octopus/variants/garg/gpio.c
index 67e1056..d7affc7 100644
--- a/src/mainboard/google/octopus/variants/garg/gpio.c
+++ b/src/mainboard/google/octopus/variants/garg/gpio.c
@@ -71,6 +71,7 @@
    	return hdmi_override_table;
    case SKU_17_LTE:
    case SKU_18_LTE_TS:
+	case SKU_39_1A2C_360_LTE_TS_NO_STYLUES:
    	*num = ARRAY_SIZE(lte_override_table);
    	return lte_override_table;
    default:
diff --git a/src/mainboard/google/octopus/variants/garg/include/variant/sku.h b/src/mainboard/google/octopus/variants/garg/include/variant/sku.h
index 8fc63cc..96e9c53 100644
--- a/src/mainboard/google/octopus/variants/garg/include/variant/sku.h
+++ b/src/mainboard/google/octopus/variants/garg/include/variant/sku.h
@@ -12,6 +12,7 @@
    SKU_20_2A2C_TS = 20,
    SKU_37_2A2C_360 = 37,
    SKU_38_2A2C_360_TS_NO_STYLUES = 38,
+	SKU_39_1A2C_360_LTE_TS_NO_STYLUES = 39,
    SKU_49_2A2C_TS = 49,
    SKU_50_HDMI = 50,
    SKU_51_2A2C = 51,
diff --git a/src/mainboard/google/octopus/variants/garg/variant.c b/src/mainboard/google/octopus/variants/garg/variant.c
index 0a6574d..00e5d32 100644
--- a/src/mainboard/google/octopus/variants/garg/variant.c
+++ b/src/mainboard/google/octopus/variants/garg/variant.c
@@ -37,6 +37,7 @@
    switch (google_chromeec_get_board_sku()) {
    case SKU_17_LTE:
    case SKU_18_LTE_TS:
+	case SKU_39_1A2C_360_LTE_TS_NO_STYLUES:
    	power_off_lte_module();
    	return;
    default:
@@ -54,6 +55,7 @@
    	switch (google_chromeec_get_board_sku()) {
    	case SKU_17_LTE:
    	case SKU_18_LTE_TS:
+		case SKU_39_1A2C_360_LTE_TS_NO_STYLUES:
    		cfg->disable_xhci_lfps_pm = 1;
    		return;
    	default: