Hello Maulik V Vaghela,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/32051
to review the following change.
Change subject: mb/google/hatch: Deassert EN_PP3300_WWAN during sleep ......................................................................
mb/google/hatch: Deassert EN_PP3300_WWAN during sleep
Deassert EN_PP3300_WWAN to turn the WWAN module completely off when entering S5. This is the same fix in commit eeb475c5c for coral board.
BUG=none BRANCH=none TEST=On hatch, Perform a quick system power cycle, verify that the modem is powered cycle and the SIM with PIN lock enabled requests unlocking.
Change-Id: I3ec8ccb7618189b9e8586f5571a68d3309597ee7 Signed-off-by: Maulik V Vaghela maulik.v.vaghela@intel.com --- M src/mainboard/google/hatch/smihandler.c M src/mainboard/google/hatch/variants/baseboard/Makefile.inc M src/mainboard/google/hatch/variants/baseboard/gpio.c M src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h 4 files changed, 33 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/32051/1
diff --git a/src/mainboard/google/hatch/smihandler.c b/src/mainboard/google/hatch/smihandler.c index af069e1..4a09a15 100644 --- a/src/mainboard/google/hatch/smihandler.c +++ b/src/mainboard/google/hatch/smihandler.c @@ -12,7 +12,8 @@ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the * GNU General Public License for more details. */ - +#include <baseboard/variants.h> +#include <console/console.h> #include <cpu/x86/smm.h> #include <ec/google/chromeec/ec.h> #include <ec/google/chromeec/smm.h> @@ -27,6 +28,12 @@
void mainboard_smi_sleep(u8 slp_typ) { + const struct pad_config *pads; + size_t num; + + pads = variant_sleep_gpio_table(slp_typ, &num); + gpio_configure_pads(pads, num); + chromeec_smi_sleep(slp_typ, MAINBOARD_EC_S3_WAKE_EVENTS, MAINBOARD_EC_S5_WAKE_EVENTS); } diff --git a/src/mainboard/google/hatch/variants/baseboard/Makefile.inc b/src/mainboard/google/hatch/variants/baseboard/Makefile.inc index 3c12e14..5d5695f 100644 --- a/src/mainboard/google/hatch/variants/baseboard/Makefile.inc +++ b/src/mainboard/google/hatch/variants/baseboard/Makefile.inc @@ -21,3 +21,5 @@ ramstage-y += gpio.c
verstage-y += gpio.c + +smm-y += gpio.c diff --git a/src/mainboard/google/hatch/variants/baseboard/gpio.c b/src/mainboard/google/hatch/variants/baseboard/gpio.c index 93e0af1..5713564 100644 --- a/src/mainboard/google/hatch/variants/baseboard/gpio.c +++ b/src/mainboard/google/hatch/variants/baseboard/gpio.c @@ -408,6 +408,28 @@ return gpio_table; }
+/* Default GPIO settings before entering sleep. */ +static const struct pad_config default_sleep_gpio_table[] = { +}; + +/* + * GPIO settings before entering S5, which are same as default_sleep_gpio_table + * but also turn off EN_PP3300_DX_LTE_SOC. + */ +static const struct pad_config s5_sleep_gpio_table[] = { + PAD_CFG_GPO(GPP_A18, 0, DEEP), /* EN_PP3300_WWAN */ +}; + +const struct pad_config *variant_sleep_gpio_table(u8 slp_typ, size_t *num) +{ + if (slp_typ == ACPI_S5) { + *num = ARRAY_SIZE(s5_sleep_gpio_table); + return s5_sleep_gpio_table; + } + *num = ARRAY_SIZE(default_sleep_gpio_table); + return default_sleep_gpio_table; +} + /* GPIOs needed prior to ramstage. */ static const struct pad_config early_gpio_table[] = { /* B15 : H1_SLAVE_SPI_CS_L */ diff --git a/src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h b/src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h index aa7c67d..c283f78 100644 --- a/src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h +++ b/src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h @@ -35,4 +35,5 @@ /* Return ChromeOS gpio table and fill in number of entries. */ const struct cros_gpio *variant_cros_gpios(size_t *num);
+const struct pad_config *variant_sleep_gpio_table(u8 slp_typ, size_t *num); #endif /* BASEBOARD_VARIANTS_H */
Hello Maulik V Vaghela, 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/+/32051
to look at the new patch set (#2).
Change subject: mb/google/hatch: Deassert EN_PP3300_WWAN during sleep ......................................................................
mb/google/hatch: Deassert EN_PP3300_WWAN during sleep
Deassert EN_PP3300_WWAN to turn the WWAN module completely off when entering S5. This is the same fix in commit eeb475c5c for coral board.
BUG=none BRANCH=none TEST=On hatch, Perform a quick system power cycle, verify that the modem is powered cycle and the SIM with PIN lock enabled requests unlocking.
Change-Id: I3ec8ccb7618189b9e8586f5571a68d3309597ee7 Signed-off-by: Maulik V Vaghela maulik.v.vaghela@intel.com --- M src/mainboard/google/hatch/smihandler.c M src/mainboard/google/hatch/variants/baseboard/Makefile.inc M src/mainboard/google/hatch/variants/baseboard/gpio.c M src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h 4 files changed, 32 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/32051/2
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32051 )
Change subject: mb/google/hatch: Deassert EN_PP3300_WWAN during sleep ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
https://review.coreboot.org/#/c/32051/2/src/mainboard/google/hatch/variants/... File src/mainboard/google/hatch/variants/baseboard/gpio.c:
https://review.coreboot.org/#/c/32051/2/src/mainboard/google/hatch/variants/... PS2, Line 420: remove tab ?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32051 )
Change subject: mb/google/hatch: Deassert EN_PP3300_WWAN during sleep ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/#/c/32051/2/src/mainboard/google/hatch/variants/... File src/mainboard/google/hatch/variants/baseboard/gpio.c:
https://review.coreboot.org/#/c/32051/2/src/mainboard/google/hatch/variants/... PS2, Line 412: default_sleep_gpio_table Do we need an empty table here? Probably okay to just pass back num as 0?
https://review.coreboot.org/#/c/32051/2/src/mainboard/google/hatch/variants/... PS2, Line 417: EN_PP3300_DX_LTE_SOC EN_PP3300_WWAN
https://review.coreboot.org/#/c/32051/2/src/mainboard/google/hatch/variants/... PS2, Line 423: variant_sleep_gpio_table __weak
https://review.coreboot.org/#/c/32051/2/src/mainboard/google/hatch/variants/... File src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h:
https://review.coreboot.org/#/c/32051/2/src/mainboard/google/hatch/variants/... PS2, Line 38: variant_sleep_gpio_table Put this with the other variant_*_gpio_table above?
Hello Subrata Banik, Maulik V Vaghela, 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/+/32051
to look at the new patch set (#3).
Change subject: mb/google/hatch: Deassert EN_PP3300_WWAN during sleep ......................................................................
mb/google/hatch: Deassert EN_PP3300_WWAN during sleep
Deassert EN_PP3300_WWAN to turn the WWAN module completely off when entering S5. This is the same fix in commit eeb475c5c for coral board.
BUG=none BRANCH=none TEST=On hatch, Perform a quick system power cycle, verify that the modem is powered cycle and the SIM with PIN lock enabled requests unlocking.
Change-Id: I3ec8ccb7618189b9e8586f5571a68d3309597ee7 Signed-off-by: Maulik V Vaghela maulik.v.vaghela@intel.com --- M src/mainboard/google/hatch/smihandler.c M src/mainboard/google/hatch/variants/baseboard/Makefile.inc M src/mainboard/google/hatch/variants/baseboard/gpio.c M src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h 4 files changed, 36 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/32051/3
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32051 )
Change subject: mb/google/hatch: Deassert EN_PP3300_WWAN during sleep ......................................................................
Patch Set 2:
(5 comments)
https://review.coreboot.org/#/c/32051/2/src/mainboard/google/hatch/variants/... File src/mainboard/google/hatch/variants/baseboard/gpio.c:
https://review.coreboot.org/#/c/32051/2/src/mainboard/google/hatch/variants/... PS2, Line 412: default_sleep_gpio_table
Do we need an empty table here? Probably okay to just pass back num as 0?
not necessary, but created as place holder anyway for future use. And the code in variant_sleep_gpio_table reads better with this defined.
https://review.coreboot.org/#/c/32051/2/src/mainboard/google/hatch/variants/... PS2, Line 417: EN_PP3300_DX_LTE_SOC
EN_PP3300_WWAN
Done
https://review.coreboot.org/#/c/32051/2/src/mainboard/google/hatch/variants/... PS2, Line 420:
remove tab ?
Done
https://review.coreboot.org/#/c/32051/2/src/mainboard/google/hatch/variants/... PS2, Line 423: variant_sleep_gpio_table
__weak
Done
https://review.coreboot.org/#/c/32051/2/src/mainboard/google/hatch/variants/... File src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h:
https://review.coreboot.org/#/c/32051/2/src/mainboard/google/hatch/variants/... PS2, Line 38: variant_sleep_gpio_table
Put this with the other variant_*_gpio_table above?
Done
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32051 )
Change subject: mb/google/hatch: Deassert EN_PP3300_WWAN during sleep ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/32051/3/src/mainboard/google/hatch/variants/... File src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h:
https://review.coreboot.org/#/c/32051/3/src/mainboard/google/hatch/variants/... PS3, Line 35: /* Return variant specifc gpio pads to be configured during sleep */ 'specifc' may be misspelled - perhaps 'specific'?
Hello Subrata Banik, Maulik V Vaghela, 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/+/32051
to look at the new patch set (#4).
Change subject: mb/google/hatch: Deassert EN_PP3300_WWAN during sleep ......................................................................
mb/google/hatch: Deassert EN_PP3300_WWAN during sleep
Deassert EN_PP3300_WWAN to turn the WWAN module completely off when entering S5. This is the same fix in commit eeb475c5c for coral board.
BUG=none BRANCH=none TEST=On hatch, Perform a quick system power cycle, verify that the modem is powered cycle and the SIM with PIN lock enabled requests unlocking.
Change-Id: I3ec8ccb7618189b9e8586f5571a68d3309597ee7 Signed-off-by: Maulik V Vaghela maulik.v.vaghela@intel.com --- M src/mainboard/google/hatch/smihandler.c M src/mainboard/google/hatch/variants/baseboard/Makefile.inc M src/mainboard/google/hatch/variants/baseboard/gpio.c M src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h 4 files changed, 36 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/32051/4
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32051 )
Change subject: mb/google/hatch: Deassert EN_PP3300_WWAN during sleep ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/32051/4/src/mainboard/google/hatch/variants/... File src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h:
https://review.coreboot.org/#/c/32051/4/src/mainboard/google/hatch/variants/... PS4, Line 35: /* Return variant specifc gpio pads to be configured during sleep */ 'specifc' may be misspelled - perhaps 'specific'?
Hello Subrata Banik, Maulik V Vaghela, 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/+/32051
to look at the new patch set (#5).
Change subject: mb/google/hatch: Deassert EN_PP3300_WWAN during sleep ......................................................................
mb/google/hatch: Deassert EN_PP3300_WWAN during sleep
Deassert EN_PP3300_WWAN to turn the WWAN module completely off when entering S5. This is the same fix in commit eeb475c5c for coral board.
BUG=none BRANCH=none TEST=On hatch, Perform a quick system power cycle, verify that the modem is powered cycle and the SIM with PIN lock enabled requests unlocking.
Change-Id: I3ec8ccb7618189b9e8586f5571a68d3309597ee7 Signed-off-by: Maulik V Vaghela maulik.v.vaghela@intel.com --- M src/mainboard/google/hatch/smihandler.c M src/mainboard/google/hatch/variants/baseboard/Makefile.inc M src/mainboard/google/hatch/variants/baseboard/gpio.c M src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h 4 files changed, 36 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/32051/5
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32051 )
Change subject: mb/google/hatch: Deassert EN_PP3300_WWAN during sleep ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/32051/3/src/mainboard/google/hatch/variants/... File src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h:
https://review.coreboot.org/#/c/32051/3/src/mainboard/google/hatch/variants/... PS3, Line 35: /* Return variant specifc gpio pads to be configured during sleep */
'specifc' may be misspelled - perhaps 'specific'?
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32051 )
Change subject: mb/google/hatch: Deassert EN_PP3300_WWAN during sleep ......................................................................
Patch Set 5: Code-Review+2
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32051 )
Change subject: mb/google/hatch: Deassert EN_PP3300_WWAN during sleep ......................................................................
mb/google/hatch: Deassert EN_PP3300_WWAN during sleep
Deassert EN_PP3300_WWAN to turn the WWAN module completely off when entering S5. This is the same fix in commit eeb475c5c for coral board.
BUG=none BRANCH=none TEST=On hatch, Perform a quick system power cycle, verify that the modem is powered cycle and the SIM with PIN lock enabled requests unlocking.
Change-Id: I3ec8ccb7618189b9e8586f5571a68d3309597ee7 Signed-off-by: Maulik V Vaghela maulik.v.vaghela@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/32051 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com --- M src/mainboard/google/hatch/smihandler.c M src/mainboard/google/hatch/variants/baseboard/Makefile.inc M src/mainboard/google/hatch/variants/baseboard/gpio.c M src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h 4 files changed, 36 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/mainboard/google/hatch/smihandler.c b/src/mainboard/google/hatch/smihandler.c index af069e1..68d562d 100644 --- a/src/mainboard/google/hatch/smihandler.c +++ b/src/mainboard/google/hatch/smihandler.c @@ -12,7 +12,7 @@ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the * GNU General Public License for more details. */ - +#include <baseboard/variants.h> #include <cpu/x86/smm.h> #include <ec/google/chromeec/ec.h> #include <ec/google/chromeec/smm.h> @@ -27,6 +27,12 @@
void mainboard_smi_sleep(u8 slp_typ) { + const struct pad_config *pads; + size_t num; + + pads = variant_sleep_gpio_table(slp_typ, &num); + gpio_configure_pads(pads, num); + chromeec_smi_sleep(slp_typ, MAINBOARD_EC_S3_WAKE_EVENTS, MAINBOARD_EC_S5_WAKE_EVENTS); } diff --git a/src/mainboard/google/hatch/variants/baseboard/Makefile.inc b/src/mainboard/google/hatch/variants/baseboard/Makefile.inc index 3c12e14..5d5695f 100644 --- a/src/mainboard/google/hatch/variants/baseboard/Makefile.inc +++ b/src/mainboard/google/hatch/variants/baseboard/Makefile.inc @@ -21,3 +21,5 @@ ramstage-y += gpio.c
verstage-y += gpio.c + +smm-y += gpio.c diff --git a/src/mainboard/google/hatch/variants/baseboard/gpio.c b/src/mainboard/google/hatch/variants/baseboard/gpio.c index 68bbaab..b974e49 100644 --- a/src/mainboard/google/hatch/variants/baseboard/gpio.c +++ b/src/mainboard/google/hatch/variants/baseboard/gpio.c @@ -411,6 +411,30 @@ return gpio_table; }
+/* Default GPIO settings before entering sleep. */ +static const struct pad_config default_sleep_gpio_table[] = { +}; + +/* + * GPIO settings before entering S5, which are same as + * default_sleep_gpio_table but also, + * turn off EN_PP3300_WWAN. + */ +static const struct pad_config s5_sleep_gpio_table[] = { + PAD_CFG_GPO(GPP_A18, 0, DEEP), /* EN_PP3300_WWAN */ +}; + +const struct pad_config * __weak +variant_sleep_gpio_table(u8 slp_typ, size_t *num) +{ + if (slp_typ == ACPI_S5) { + *num = ARRAY_SIZE(s5_sleep_gpio_table); + return s5_sleep_gpio_table; + } + *num = ARRAY_SIZE(default_sleep_gpio_table); + return default_sleep_gpio_table; +} + /* GPIOs needed prior to ramstage. */ static const struct pad_config early_gpio_table[] = { /* B15 : H1_SLAVE_SPI_CS_L */ diff --git a/src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h b/src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h index aa7c67d..fd39c45 100644 --- a/src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h +++ b/src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h @@ -32,6 +32,9 @@ /* Return board specific memory configuration */ void variant_memory_params(struct cnl_mb_cfg *bcfg);
+/* Return variant specific gpio pads to be configured during sleep */ +const struct pad_config *variant_sleep_gpio_table(u8 slp_typ, size_t *num); + /* Return ChromeOS gpio table and fill in number of entries. */ const struct cros_gpio *variant_cros_gpios(size_t *num);