Change in coreboot[master]: mb/google/octopus/variants/bobba: fix LTE power sequence

[Sheng-Liang Pan (Code Review)]

Sheng-Liang Pan has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37649 )

Change subject: mb/google/octopus/variants/bobba: fix LTE power sequence
......................................................................

mb/google/octopus/variants/bobba: fix LTE power sequence

fix Power_off section power sequence.

BUG=b:144327240
BRANCH=octopus
TEST=build image and verify on the DUT with LTE DB.

Change-Id: I287ba1cb092a95b3a9dd1f960a3b84fd85b9b221
Signed-off-by: Pan Sheng-Liang 
---
M src/mainboard/google/octopus/variants/bobba/Makefile.inc
M src/mainboard/google/octopus/variants/bobba/variant.c
2 files changed, 7 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/37649/1

diff --git a/src/mainboard/google/octopus/variants/bobba/Makefile.inc b/src/mainboard/google/octopus/variants/bobba/Makefile.inc
index ba865e9..7ee7e70 100644
--- a/src/mainboard/google/octopus/variants/bobba/Makefile.inc
+++ b/src/mainboard/google/octopus/variants/bobba/Makefile.inc
@@ -2,3 +2,5 @@

ramstage-y += gpio.c
 ramstage-y += variant.c
+
+smm-y += variant.c
diff --git a/src/mainboard/google/octopus/variants/bobba/variant.c b/src/mainboard/google/octopus/variants/bobba/variant.c
index 1f6e80d..fafaf03 100644
--- a/src/mainboard/google/octopus/variants/bobba/variant.c
+++ b/src/mainboard/google/octopus/variants/bobba/variant.c
@@ -19,6 +19,7 @@
 #include <baseboard/variants.h>
 #include <delay.h>
 #include <gpio.h>
+#include <ec/google/chromeec/ec.h>

enum {
    SKU_37_DROID = 37, /* LTE */
@@ -74,7 +75,10 @@
    if (slp_typ != ACPI_S5)
    	return;

-	switch (get_board_sku()) {
+	static uint32_t sku_id = 255;
+	google_chromeec_cbi_get_sku_id(&sku_id);
+
+	switch (sku_id) {
    case SKU_37_DROID:
    case SKU_38_DROID:
    case SKU_39_DROID:

[Sheng-Liang Pan (Code Review)]

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

Change subject: mb/google/octopus/variants/bobba: fix LTE power sequence
......................................................................

Patch Set 1: Code-Review+1

[Justin TerAvest (Code Review)]

Justin TerAvest has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37649 )

Change subject: mb/google/octopus/variants/bobba: fix LTE power sequence
......................................................................

Patch Set 1: Code-Review+2

[Paul Menzel (Code Review)]

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

Change subject: mb/google/octopus/variants/bobba: fix LTE power sequence
......................................................................

Patch Set 1: Code-Review-1

(1 comment)

https://review.coreboot.org/c/coreboot/+/37649/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/37649/1//COMMIT_MSG@10 
PS1, Line 10: 
Please describe the problem and fix in more detail. Why is `get_board_sku()` not enough?

[Sheng-Liang Pan (Code Review)]

Hello Marco Chen, Henry Sun, Justin TerAvest, Paul Menzel, build bot (Jenkins), Patrick Georgi, Martin Roth,

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

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

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

Change subject: mb/google/octopus/variants/bobba: fix LTE power sequence
......................................................................

mb/google/octopus/variants/bobba: fix LTE power sequence

fix Power_off section power sequence.
power_off_lte_module() should run in smm stage, add it in smm stage.

BUG=b:144327240
BRANCH=octopus
TEST=build image and verify on the DUT with LTE DB.

Change-Id: I287ba1cb092a95b3a9dd1f960a3b84fd85b9b221
Signed-off-by: Pan Sheng-Liang 
---
M src/mainboard/google/octopus/variants/bobba/Makefile.inc
M src/mainboard/google/octopus/variants/bobba/variant.c
2 files changed, 7 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/37649/2

[Sheng-Liang Pan (Code Review)]

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

Change subject: mb/google/octopus/variants/bobba: fix LTE power sequence
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/37649/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/37649/1//COMMIT_MSG@10 
PS1, Line 10:
> Please describe the problem and fix in more detail. […]

Here the problem is function power_off_lte_module() can't called in smm stage, so add variant.c to smm stage.

get_board_sku() was declared in mainboard.c which also not execute in smm mode, it's reason to use google_chromeec_cbi_get_sku_id().

[Karthik Ramasubramanian (Code Review)]

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

Change subject: mb/google/octopus/variants/bobba: fix LTE power sequence
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/37649/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/37649/1//COMMIT_MSG@10 
PS1, Line 10:
> Here the problem is function power_off_lte_module() can't called in smm stage, so add variant. […]

Can get_board_sku() be put into a separate file - eg. board_info.c or mainboard_misc.c. Then that file can be compiled in ramstage and smm stages.

That way we don't have to re-implement it.

Also you can mention the problem details you shared in the response in the commit summary.

[Sheng-Liang Pan (Code Review)]

Hello Marco Chen, Henry Sun, Justin TerAvest, Paul Menzel, build bot (Jenkins), Patrick Georgi, Martin Roth,

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

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

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

Change subject: mb/google/octopus/variants/bobba: fix LTE power sequence
......................................................................

mb/google/octopus/variants/bobba: fix LTE power sequence

fix Power_off section power sequence.
power_off_lte_module() should run in smm stage, add variant.c in smm stage.
and get_board_sku() in mainboard.c is complied in ramstage,
so use google_chromeec_cbi_get_sku_id() to get skuid.

BUG=b:144327240
BRANCH=octopus
TEST=build image and verify on the DUT with LTE DB.

Change-Id: I287ba1cb092a95b3a9dd1f960a3b84fd85b9b221
Signed-off-by: Pan Sheng-Liang 
---
M src/mainboard/google/octopus/variants/bobba/Makefile.inc
M src/mainboard/google/octopus/variants/bobba/variant.c
2 files changed, 7 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/37649/3

[Sheng-Liang Pan (Code Review)]

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

Change subject: mb/google/octopus/variants/bobba: fix LTE power sequence
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/37649/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/37649/1//COMMIT_MSG@10 
PS1, Line 10:
> Can get_board_sku() be put into a separate file - eg. board_info.c or mainboard_misc.c. […]

if use google_chromeec_cbi_get_sku_id() is much simple to replaced get_board_sku() rather then create a new file to let it added in smm mode?
I will describe detail in commit, thanks.

[Marco Chen (Code Review)]

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

Change subject: mb/google/octopus/variants/bobba: fix LTE power sequence
......................................................................

Patch Set 3:

Hi Sheng-Liang, are you able to address the comment from Karthik? I think one benefit to use  get_board_sku() is that we don't need to communicate to EC again if we did it once. Thanks.

Change in coreboot[master]: mb/google/octopus/variants/bobba: fix LTE power sequence and move get...

[Sheng-Liang Pan (Code Review)]

Hello Marco Chen, Henry Sun, Justin TerAvest, Paul Menzel, build bot (Jenkins), Patrick Georgi, Martin Roth,

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

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

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

Change subject: mb/google/octopus/variants/bobba: fix LTE power sequence and move get_board_sku to smm stage.
......................................................................

mb/google/octopus/variants/bobba: fix LTE power sequence and move
get_board_sku to smm stage.

fix Power_off section power sequence.
power_off_lte_module() should run in smm stage, add variant.c in smm stage.
also move get_board_sku() to mainboard_misc.c so that we can use it in smm stage
and ramstage.

BUG=b:144327240
BRANCH=octopus
TEST=build image and verify on the DUT with LTE DB.

Change-Id: I287ba1cb092a95b3a9dd1f960a3b84fd85b9b221
Signed-off-by: Pan Sheng-Liang 
---
M src/mainboard/google/octopus/Makefile.inc
M src/mainboard/google/octopus/mainboard.c
A src/mainboard/google/octopus/mainboard_misc.c
M src/mainboard/google/octopus/variants/bobba/Makefile.inc
4 files changed, 58 insertions(+), 32 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/37649/4

Change in coreboot[master]: mb/google/octopus/variants/bobba: fix LTE power sequence and move get...

[Sheng-Liang Pan (Code Review)]

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

Change subject: mb/google/octopus/variants/bobba: fix LTE power sequence and move get_board_sku to smm stage.
......................................................................

Patch Set 4: Code-Review+1
> Patch Set 3:
>
> Hi Sheng-Liang, are you able to address the comment from Karthik? I think one benefit to use  get_board_sku() is that we don't need to communicate to EC again if we did it once. Thanks.

upload patch set 4 for review. Please help check. Thanks

Change in coreboot[master]: mb/google/octopus/variants/bobba: fix LTE power sequence and move get...

[Marco Chen (Code Review)]

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

Change subject: mb/google/octopus/variants/bobba: fix LTE power sequence and move get_board_sku to smm stage.
......................................................................

Patch Set 4: Code-Review+2

Looks good to me, thanks Sheng-Liang. Great!!!

Change in coreboot[master]: mb/google/octopus/variants/bobba: fix LTE power sequence and move get...

[Ren Kuo (Code Review)]

Ren Kuo has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37649 )

Change subject: mb/google/octopus/variants/bobba: fix LTE power sequence and move get_board_sku to smm stage.
......................................................................

Patch Set 4: Code-Review+2

Change in coreboot[master]: mb/google/octopus/variants/bobba: fix LTE power sequence and move get...

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37649 )

Change subject: mb/google/octopus/variants/bobba: fix LTE power sequence and move get_board_sku to smm stage.
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/37649/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/37649/1//COMMIT_MSG@10 
PS1, Line 10:
> if use google_chromeec_cbi_get_sku_id() is much simple to replaced get_board_sku() rather then creat […]

Done

Change in coreboot[master]: mb/google/octopus/variants/bobba: fix LTE power sequence and move get...

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/37649 )

Change subject: mb/google/octopus/variants/bobba: fix LTE power sequence and move get_board_sku to smm stage.
......................................................................

mb/google/octopus/variants/bobba: fix LTE power sequence and move
get_board_sku to smm stage.

fix Power_off section power sequence.
power_off_lte_module() should run in smm stage, add variant.c in smm stage.
also move get_board_sku() to mainboard_misc.c so that we can use it in smm stage
and ramstage.

BUG=b:144327240
BRANCH=octopus
TEST=build image and verify on the DUT with LTE DB.

Change-Id: I287ba1cb092a95b3a9dd1f960a3b84fd85b9b221
Signed-off-by: Pan Sheng-Liang 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/37649
Reviewed-by: Marco Chen 
Reviewed-by: Ren Kuo 
Tested-by: build bot (Jenkins) 
---
M src/mainboard/google/octopus/Makefile.inc
M src/mainboard/google/octopus/mainboard.c
A src/mainboard/google/octopus/mainboard_misc.c
M src/mainboard/google/octopus/variants/bobba/Makefile.inc
4 files changed, 58 insertions(+), 32 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Ren Kuo: Looks good to me, approved
  Sheng-Liang Pan: Looks good to me, but someone else must approve
  Marco Chen: Looks good to me, approved

diff --git a/src/mainboard/google/octopus/Makefile.inc b/src/mainboard/google/octopus/Makefile.inc
index aa05524..d36d5f7 100644
--- a/src/mainboard/google/octopus/Makefile.inc
+++ b/src/mainboard/google/octopus/Makefile.inc
@@ -5,10 +5,12 @@

ramstage-$(CONFIG_CHROMEOS) += chromeos.c
 ramstage-y += ec.c
+ramstage-y += mainboard_misc.c
 ramstage-y += mainboard.c

verstage-$(CONFIG_CHROMEOS) += chromeos.c
 smm-y += smihandler.c
+smm-y += mainboard_misc.c

subdirs-y += variants/baseboard
 CPPFLAGS_common += -I$(src)/mainboard/$(MAINBOARDDIR)/variants/baseboard/include
diff --git a/src/mainboard/google/octopus/mainboard.c b/src/mainboard/google/octopus/mainboard.c
index 3312d4d..0ab4693 100644
--- a/src/mainboard/google/octopus/mainboard.c
+++ b/src/mainboard/google/octopus/mainboard.c
@@ -117,38 +117,6 @@
    .enable_dev = mainboard_enable,
 };

-#define SKU_UNKNOWN		0xFFFFFFFF
-#define SKU_MAX			255
-
-uint32_t 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 *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 > 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;
-}
-
 void __weak variant_update_devtree(struct device *dev)
 {
    /* Place holder for common updates. */
diff --git a/src/mainboard/google/octopus/mainboard_misc.c b/src/mainboard/google/octopus/mainboard_misc.c
new file mode 100644
index 0000000..3672f66
--- /dev/null
+++ b/src/mainboard/google/octopus/mainboard_misc.c
@@ -0,0 +1,54 @@
+/*
+ * This file is part of the coreboot project.
+ *
+ * Copyright 2019 Intel Corp.
+ *
+ * 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 <baseboard/variants.h>
+#include <boardid.h>
+#include <console/console.h>
+#include <ec/google/chromeec/ec.h>
+#include <ec/ec.h>
+#include <smbios.h>
+#include <string.h>
+
+#define SKU_UNKNOWN		0xFFFFFFFF
+#define SKU_MAX			255
+
+uint32_t 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 *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 > 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;
+}
diff --git a/src/mainboard/google/octopus/variants/bobba/Makefile.inc b/src/mainboard/google/octopus/variants/bobba/Makefile.inc
index ba865e9..7ee7e70 100644
--- a/src/mainboard/google/octopus/variants/bobba/Makefile.inc
+++ b/src/mainboard/google/octopus/variants/bobba/Makefile.inc
@@ -2,3 +2,5 @@

ramstage-y += gpio.c
 ramstage-y += variant.c
+
+smm-y += variant.c