Change in coreboot[master]: mb/google/octopus/variants/garg: support LTE power sequence.
Hello Marco Chen,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/34188
to review the following change.
Change subject: mb/google/octopus/variants/garg: support LTE power sequence. ......................................................................
mb/google/octopus/variants/garg: support LTE power sequence.
BUG=b:137033609 BRANCH=octopus TEST=build image and verify on the DUT with LTE DB.
Change-Id: I7bf6fee087c885c22363b44aa98aa61f91be90b4 Signed-off-by: Marco Chen marcochen@google.com --- M src/include/gpio.h M src/lib/gpio.c M src/mainboard/google/octopus/bootblock.c M src/mainboard/google/octopus/smihandler.c M src/mainboard/google/octopus/variants/baseboard/gpio.c M src/mainboard/google/octopus/variants/baseboard/include/baseboard/gpio.h M src/mainboard/google/octopus/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/octopus/variants/garg/gpio.c M src/soc/intel/common/block/gpio/gpio.c 9 files changed, 111 insertions(+), 17 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/34188/1
diff --git a/src/include/gpio.h b/src/include/gpio.h index 0a37ee7..a605f17 100644 --- a/src/include/gpio.h +++ b/src/include/gpio.h @@ -24,6 +24,7 @@
/* The following functions must be implemented by SoC/board code. */ int gpio_get(gpio_t gpio); +int gpio_get_output(gpio_t gpio); void gpio_set(gpio_t gpio, int value); void gpio_input_pulldown(gpio_t gpio); void gpio_input_pullup(gpio_t gpio); diff --git a/src/lib/gpio.c b/src/lib/gpio.c index 8ea3b5e..9e22b63 100644 --- a/src/lib/gpio.c +++ b/src/lib/gpio.c @@ -190,3 +190,9 @@ { return 0; } + +/* Default handler returns 0 because type of gpio_t is unknown */ +__weak int gpio_get_output(gpio_t gpio) +{ + return 0; +} diff --git a/src/mainboard/google/octopus/bootblock.c b/src/mainboard/google/octopus/bootblock.c index 0c239db..4da3e94 100644 --- a/src/mainboard/google/octopus/bootblock.c +++ b/src/mainboard/google/octopus/bootblock.c @@ -21,8 +21,8 @@
void bootblock_mainboard_init(void) { - const struct pad_config *pads; - size_t num; + const struct pad_config *pads, *override_pads; + size_t num, override_num;
lpc_configure_pads();
@@ -34,5 +34,7 @@ mainboard_ec_init();
pads = variant_early_gpio_table(&num); - gpio_configure_pads(pads, num); + override_pads = variant_early_override_gpio_table(&override_num); + gpio_configure_pads_with_override(pads, num, + override_pads, override_num); } diff --git a/src/mainboard/google/octopus/smihandler.c b/src/mainboard/google/octopus/smihandler.c index 27928ee..a1686e2 100644 --- a/src/mainboard/google/octopus/smihandler.c +++ b/src/mainboard/google/octopus/smihandler.c @@ -16,15 +16,48 @@ #include <arch/acpi.h> #include <baseboard/variants.h> #include <cpu/x86/smm.h> +#include <delay.h> #include <ec/google/chromeec/ec.h> #include <ec/google/chromeec/smm.h> #include <elog.h> +#include <gpio.h> #include <intelblocks/smihandler.h> #include <soc/pm.h> #include <soc/gpio.h> #include <variant/ec.h> #include <variant/gpio.h>
+struct pad_config_with_delay { + const struct pad_config config; + unsigned int delay_msecs; +}; + +static void power_off_lte_module_if_exist(u8 slp_typ) +{ + if (slp_typ != ACPI_S5 || !gpio_get_output(GPIO_LTE_RESET)) + return; + + const struct pad_config_with_delay lte_power_off_gpios[] = { + { + PAD_CFG_GPO(GPIO_161, 0, DEEP), /* AVS_I2S1_MCLK -- PLT_RST_LTE_L */ + 30, + }, + { + PAD_CFG_GPO(GPIO_117, 0, DEEP), /* PCIE_WAKE1_B -- FULL_CARD_POWER_OFF */ + 100 + }, + { + PAD_CFG_GPO(GPIO_67, 0, DEEP), /* UART2-CTS_B -- EN_PP3300_DX_LTE_SOC */ + 0 + } + }; + + for (int i = 0; i < ARRAY_SIZE(lte_power_off_gpios); i++) { + gpio_configure_pads(<e_power_off_gpios[i].config, 1); + mdelay(lte_power_off_gpios[i].delay_msecs); + } +} + void mainboard_smi_gpi_handler(const struct gpi_status *sts) { if (gpi_status_get(sts, EC_SMI_GPI)) @@ -39,6 +72,8 @@ pads = variant_sleep_gpio_table(&num, slp_typ); gpio_configure_pads(pads, num);
+ power_off_lte_module_if_exist(slp_typ); + chromeec_smi_sleep(slp_typ, MAINBOARD_EC_S3_WAKE_EVENTS, MAINBOARD_EC_S5_WAKE_EVENTS); } diff --git a/src/mainboard/google/octopus/variants/baseboard/gpio.c b/src/mainboard/google/octopus/variants/baseboard/gpio.c index febf779..05b2c64 100644 --- a/src/mainboard/google/octopus/variants/baseboard/gpio.c +++ b/src/mainboard/google/octopus/variants/baseboard/gpio.c @@ -94,7 +94,6 @@ PAD_CFG_NF_IOSSTATE_IOSTERM(GPIO_64, UP_20K, DEEP, NF1, HIZCRx1, DISPUPD), /* LPSS_UART2_RXD */ PAD_CFG_NF_IOSSTATE_IOSTERM(GPIO_65, UP_20K, DEEP, NF1, TxLASTRxE, DISPUPD), /* LPSS_UART2_TXD */ PAD_NC(GPIO_66, UP_20K), /* UART2-RTS_B -- unused */ - PAD_CFG_GPO_IOSSTATE_IOSTERM(GPIO_67, 0, DEEP, NONE, TxLASTRxE, DISPUPD), /* UART2-CTS_B -- EN_PP3300_DX_LTE_SOC */ PAD_CFG_GPI(GPIO_68, NONE, DEEP), /* DRAM_ID0 */ PAD_CFG_GPI(GPIO_69, NONE, DEEP), /* DRAM_ID1 */ PAD_CFG_GPI(GPIO_70, NONE, DEEP), /* DRAM_ID2 */ @@ -165,7 +164,6 @@
/* PCIE_WAKE[0:3]_B */ PAD_CFG_GPO_IOSSTATE_IOSTERM(GPIO_116, 1, DEEP, NONE, Tx1RxDCRx1, DISPUPD), /* PCIE_WAKE0_B -- WIFI_DISABLE_L */ - PAD_CFG_GPI_SCI_LOW(GPIO_117, NONE, DEEP, EDGE_SINGLE),/* PCIE_WAKE1_B -- LTE_WAKE_L */ PAD_NC(GPIO_118, UP_20K),/* PCIE_WAKE2_B -- unused */ PAD_CFG_GPI_SCI_LOW(GPIO_119, NONE, DEEP, EDGE_SINGLE),/* PCIE_WAKE3_B */
@@ -299,6 +297,12 @@ return NULL; }
+const struct pad_config *__weak variant_early_override_gpio_table(size_t *num) +{ + *num = 0; + return NULL; +} + /* GPIOs needed prior to ramstage. */ static const struct pad_config early_gpio_table[] = { PAD_CFG_GPI(GPIO_190, NONE, DEEP), /* PCH_WP_OD */ @@ -325,6 +329,14 @@ */ PAD_CFG_NF_IOSSTATE_IOSTERM(GPIO_151, UP_20K, DEEP, NF2, HIZCRx1, ENPU), /* ESPI_IO1 */ + + PAD_CFG_GPO_IOSSTATE_IOSTERM(GPIO_67, 0, DEEP, NONE, TxLASTRxE, DISPUPD), /* UART2-CTS_B -- EN_PP3300_DX_LTE_SOC */ + PAD_CFG_GPI_SCI_LOW(GPIO_117, NONE, DEEP, EDGE_SINGLE),/* PCIE_WAKE1_B -- LTE_WAKE_L */ + /* GPIO_161 is in early_gpio_table and gpio_table because LTE SKU needs + * to override this pin to output low then high respectively in two + * stages. + */ + PAD_CFG_GPO_IOSSTATE_IOSTERM(GPIO_161, 1, DEEP, UP_20K, Tx1RxDCRx0, DISPUPD), /* AVS_I2S1_MCLK -- LTE_OFF_ODL */ };
const struct pad_config *__weak diff --git a/src/mainboard/google/octopus/variants/baseboard/include/baseboard/gpio.h b/src/mainboard/google/octopus/variants/baseboard/include/baseboard/gpio.h index b408403..04ccaa7 100644 --- a/src/mainboard/google/octopus/variants/baseboard/include/baseboard/gpio.h +++ b/src/mainboard/google/octopus/variants/baseboard/include/baseboard/gpio.h @@ -33,6 +33,9 @@
#define GPIO_PCH_WP GPIO_190
+/* LTE reset GPIO */ +#define GPIO_LTE_RESET TGPIO_161 + /* Memory SKU GPIOs. */ #define MEM_CONFIG0 GPIO_68 #define MEM_CONFIG1 GPIO_69 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 5374ace..33a8f52 100644 --- a/src/mainboard/google/octopus/variants/baseboard/include/baseboard/variants.h +++ b/src/mainboard/google/octopus/variants/baseboard/include/baseboard/variants.h @@ -26,6 +26,7 @@ const struct pad_config *variant_base_gpio_table(size_t *num); const struct pad_config *variant_override_gpio_table(size_t *num); const struct pad_config *variant_early_gpio_table(size_t *num); +const struct pad_config *variant_early_override_gpio_table(size_t *num); const struct pad_config *variant_sleep_gpio_table(size_t *num, int slp_typ);
/* Baseboard default swizzle. Can be reused if swizzle is same. */ diff --git a/src/mainboard/google/octopus/variants/garg/gpio.c b/src/mainboard/google/octopus/variants/garg/gpio.c index 90601ce..c589829 100644 --- a/src/mainboard/google/octopus/variants/garg/gpio.c +++ b/src/mainboard/google/octopus/variants/garg/gpio.c @@ -19,6 +19,12 @@ #include <gpio.h> #include <soc/gpio.h>
+#define DEFAULT_OVERRIDE_GPIOS() PAD_NC(GPIO_104, UP_20K), \ + /* EN_PP3300_TOUCHSCREEN */ \ + PAD_CFG_GPO_IOSSTATE_IOSTERM(GPIO_146, 0, DEEP, NONE, Tx0RxDCRx0, \ + DISPUPD), \ + PAD_NC(GPIO_213, DN_20K), \ + enum { SKU_1_2A2C = 1, SKU_9_HDMI = 9, @@ -26,17 +32,11 @@ };
static const struct pad_config default_override_table[] = { - PAD_NC(GPIO_104, UP_20K), - - /* EN_PP3300_TOUCHSCREEN */ - PAD_CFG_GPO_IOSSTATE_IOSTERM(GPIO_146, 0, DEEP, NONE, Tx0RxDCRx0, - DISPUPD), - - PAD_NC(GPIO_213, DN_20K), + DEFAULT_OVERRIDE_GPIOS() };
static const struct pad_config hdmi_override_table[] = { - PAD_NC(GPIO_104, UP_20K), + DEFAULT_OVERRIDE_GPIOS()
/* HV_DDI1_DDC_SDA */ PAD_CFG_NF_IOSSTATE_IOSTERM(GPIO_126, NONE, DEEP, NF1, HIZCRx1, @@ -44,12 +44,13 @@ /* HV_DDI1_DDC_SCL */ PAD_CFG_NF_IOSSTATE_IOSTERM(GPIO_127, NONE, DEEP, NF1, HIZCRx1, DISPUPD), +};
- /* EN_PP3300_TOUCHSCREEN */ - PAD_CFG_GPO_IOSSTATE_IOSTERM(GPIO_146, 0, DEEP, NONE, Tx0RxDCRx0, - DISPUPD), +static const struct pad_config lte_override_table[] = { + DEFAULT_OVERRIDE_GPIOS()
- PAD_NC(GPIO_213, DN_20K), + /* AVS_I2S1_MCLK -- PLT_RST_LTE_L */ + PAD_CFG_GPO(GPIO_161, 1, DEEP), };
const struct pad_config *variant_override_gpio_table(size_t *num) @@ -61,8 +62,29 @@ case SKU_9_HDMI: *num = ARRAY_SIZE(hdmi_override_table); return hdmi_override_table; + case SKU_17_LTE: + *num = ARRAY_SIZE(lte_override_table); + return lte_override_table; default: *num = ARRAY_SIZE(default_override_table); return default_override_table; } } + +static const struct pad_config lte_early_override_table[] = { + /* UART2-CTS_B -- EN_PP3300_DX_LTE_SOC */ + PAD_CFG_GPO(GPIO_67, 1, PWROK), + + /* PCIE_WAKE1_B -- FULL_CARD_POWER_OFF */ + PAD_CFG_GPO(GPIO_117, 1, PWROK), + + /* AVS_I2S1_MCLK -- PLT_RST_LTE_L */ + PAD_CFG_GPO(GPIO_161, 0, DEEP), +}; + +const struct pad_config *variant_early_override_gpio_table(size_t *num) +{ + *num = ARRAY_SIZE(lte_early_override_table); + + return lte_early_override_table; +} diff --git a/src/soc/intel/common/block/gpio/gpio.c b/src/soc/intel/common/block/gpio/gpio.c index 7ebf05a..a53e16a 100644 --- a/src/soc/intel/common/block/gpio/gpio.c +++ b/src/soc/intel/common/block/gpio/gpio.c @@ -402,6 +402,18 @@ return !!(reg & PAD_CFG0_RX_STATE); }
+int gpio_get_output(gpio_t gpio_num) +{ + const struct pad_community *comm = gpio_get_community(gpio_num); + uint16_t config_offset; + uint32_t reg; + + config_offset = pad_config_offset(comm, gpio_num); + reg = pcr_read32(comm->port, config_offset); + + return !!(reg & PAD_CFG0_TX_STATE); +} + void gpio_set(gpio_t gpio_num, int value) { const struct pad_community *comm = gpio_get_community(gpio_num);
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34188 )
Change subject: mb/google/octopus/variants/garg: support LTE power sequence. ......................................................................
Patch Set 1:
(4 comments)
https://review.coreboot.org/c/coreboot/+/34188/1/src/mainboard/google/octopu... File src/mainboard/google/octopus/smihandler.c:
https://review.coreboot.org/c/coreboot/+/34188/1/src/mainboard/google/octopu... PS1, Line 46: PAD_CFG_GPO(GPIO_117, 0, DEEP), /* PCIE_WAKE1_B -- FULL_CARD_POWER_OFF */ line over 96 characters
https://review.coreboot.org/c/coreboot/+/34188/1/src/mainboard/google/octopu... File src/mainboard/google/octopus/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/34188/1/src/mainboard/google/octopu... PS1, Line 333: PAD_CFG_GPO_IOSSTATE_IOSTERM(GPIO_67, 0, DEEP, NONE, TxLASTRxE, DISPUPD), /* UART2-CTS_B -- EN_PP3300_DX_LTE_SOC */ line over 96 characters
https://review.coreboot.org/c/coreboot/+/34188/1/src/mainboard/google/octopu... PS1, Line 339: PAD_CFG_GPO_IOSSTATE_IOSTERM(GPIO_161, 1, DEEP, UP_20K, Tx1RxDCRx0, DISPUPD), /* AVS_I2S1_MCLK -- LTE_OFF_ODL */ line over 96 characters
https://review.coreboot.org/c/coreboot/+/34188/1/src/mainboard/google/octopu... File src/mainboard/google/octopus/variants/garg/gpio.c:
https://review.coreboot.org/c/coreboot/+/34188/1/src/mainboard/google/octopu... PS1, Line 22: #define DEFAULT_OVERRIDE_GPIOS() PAD_NC(GPIO_104, UP_20K), \ Macros with complex values should be enclosed in parentheses
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34188 )
Change subject: mb/google/octopus/variants/garg: support LTE power sequence. ......................................................................
Patch Set 1:
(5 comments)
https://review.coreboot.org/c/coreboot/+/34188/1/src/include/gpio.h File src/include/gpio.h:
https://review.coreboot.org/c/coreboot/+/34188/1/src/include/gpio.h@27 PS1, Line 27: gpio_get_output Can you please push these as 3 separate changes: 1. Changes to common code for gpio.c/h 2. Changes to SoC files 3. Changes to mainboard files
https://review.coreboot.org/c/coreboot/+/34188/1/src/mainboard/google/octopu... File src/mainboard/google/octopus/smihandler.c:
https://review.coreboot.org/c/coreboot/+/34188/1/src/mainboard/google/octopu... PS1, Line 37: !gpio_get_output(GPIO_LTE_RESET) Is it really necessary to check this? I am guessing that the GPIOs which are used for LTE remain not connected on other variants? In that case it is probably just fine to configure these always irrespective of LTE present or not? If the GPIOs are used on some variants for other purpose, you can potentially do this in the variant that is actually using it?
https://review.coreboot.org/c/coreboot/+/34188/1/src/mainboard/google/octopu... PS1, Line 56: lte_power_off_gpios Since all these GPIOs are outputs and are being set to 0, you can simply have a struct with GPIO# and delay and then use: gpio_output(GPIO_xyz, 0);
https://review.coreboot.org/c/coreboot/+/34188/1/src/mainboard/google/octopu... File src/mainboard/google/octopus/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/34188/1/src/mainboard/google/octopu... PS1, Line 333: PAD_CFG_GPO_IOSSTATE_IOSTERM Is there no power on sequencing requirements?
https://review.coreboot.org/c/coreboot/+/34188/1/src/mainboard/google/octopu... File src/mainboard/google/octopus/variants/baseboard/include/baseboard/gpio.h:
https://review.coreboot.org/c/coreboot/+/34188/1/src/mainboard/google/octopu... PS1, Line 37: TGPIO_161 GPIO_161?
Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34188 )
Change subject: mb/google/octopus/variants/garg: support LTE power sequence. ......................................................................
Patch Set 1:
(4 comments)
https://review.coreboot.org/c/coreboot/+/34188/1/src/mainboard/google/octopu... File src/mainboard/google/octopus/smihandler.c:
https://review.coreboot.org/c/coreboot/+/34188/1/src/mainboard/google/octopu... PS1, Line 37: !gpio_get_output(GPIO_LTE_RESET)
Is it really necessary to check this? I am guessing that the GPIOs which are used for LTE remain not […]
The intention is to reduce the effect of delaying to other variants and will check with HW Eng as well.
https://review.coreboot.org/c/coreboot/+/34188/1/src/mainboard/google/octopu... PS1, Line 56: lte_power_off_gpios
Since all these GPIOs are outputs and are being set to 0, you can simply have a struct with GPIO# an […]
Copy that and will try.
https://review.coreboot.org/c/coreboot/+/34188/1/src/mainboard/google/octopu... File src/mainboard/google/octopus/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/34188/1/src/mainboard/google/octopu... PS1, Line 333: PAD_CFG_GPO_IOSSTATE_IOSTERM
Is there no power on sequencing requirements?
Regarding power on sequence, the only requirement is that there should be a gap (larger then 10ms) between 67/117 (3.3V/CARD_POWER) and 161(RESET). Therefore we enable 67/117 in bootblock then enable 161 in ramstage which are override in garg variants.
https://review.coreboot.org/c/coreboot/+/34188/1/src/mainboard/google/octopu... File src/mainboard/google/octopus/variants/baseboard/include/baseboard/gpio.h:
https://review.coreboot.org/c/coreboot/+/34188/1/src/mainboard/google/octopu... PS1, Line 37: TGPIO_161
GPIO_161?
Ack and found that build will get failed when I move this CL into chromium repo for testing. Thanks.
Hello Patrick Rudolph, Karthik Ramasubramanian, Justin TerAvest, Marco Chen, Kevin Chiu, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34188
to look at the new patch set (#2).
Change subject: mb/google/octopus/variants/garg: support LTE power sequence. ......................................................................
mb/google/octopus/variants/garg: support LTE power sequence.
BUG=b:137033609 BRANCH=octopus TEST=build image and verify on the DUT with LTE DB.
Change-Id: I7bf6fee087c885c22363b44aa98aa61f91be90b4 Signed-off-by: Marco Chen marcochen@google.com --- M src/include/gpio.h M src/lib/gpio.c M src/mainboard/google/octopus/bootblock.c M src/mainboard/google/octopus/smihandler.c M src/mainboard/google/octopus/variants/baseboard/gpio.c M src/mainboard/google/octopus/variants/baseboard/include/baseboard/gpio.h M src/mainboard/google/octopus/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/octopus/variants/garg/gpio.c M src/soc/intel/common/block/gpio/gpio.c 9 files changed, 111 insertions(+), 17 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/34188/2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34188 )
Change subject: mb/google/octopus/variants/garg: support LTE power sequence. ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/34188/2/src/mainboard/google/octopu... File src/mainboard/google/octopus/smihandler.c:
https://review.coreboot.org/c/coreboot/+/34188/2/src/mainboard/google/octopu... PS2, Line 46: PAD_CFG_GPO(GPIO_117, 0, DEEP), /* PCIE_WAKE1_B -- FULL_CARD_POWER_OFF */ line over 96 characters
https://review.coreboot.org/c/coreboot/+/34188/2/src/mainboard/google/octopu... File src/mainboard/google/octopus/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/34188/2/src/mainboard/google/octopu... PS2, Line 333: PAD_CFG_GPO_IOSSTATE_IOSTERM(GPIO_67, 0, DEEP, NONE, TxLASTRxE, DISPUPD), /* UART2-CTS_B -- EN_PP3300_DX_LTE_SOC */ line over 96 characters
https://review.coreboot.org/c/coreboot/+/34188/2/src/mainboard/google/octopu... PS2, Line 339: PAD_CFG_GPO_IOSSTATE_IOSTERM(GPIO_161, 1, DEEP, UP_20K, Tx1RxDCRx0, DISPUPD), /* AVS_I2S1_MCLK -- LTE_OFF_ODL */ line over 96 characters
https://review.coreboot.org/c/coreboot/+/34188/2/src/mainboard/google/octopu... File src/mainboard/google/octopus/variants/garg/gpio.c:
https://review.coreboot.org/c/coreboot/+/34188/2/src/mainboard/google/octopu... PS2, Line 22: #define DEFAULT_OVERRIDE_GPIOS() PAD_NC(GPIO_104, UP_20K), \ Macros with complex values should be enclosed in parentheses
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34188 )
Change subject: mb/google/octopus/variants/garg: support LTE power sequence. ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/c/coreboot/+/34188/3/src/mainboard/google/octopu... File src/mainboard/google/octopus/smihandler.c:
https://review.coreboot.org/c/coreboot/+/34188/3/src/mainboard/google/octopu... PS3, Line 46: PAD_CFG_GPO(GPIO_117, 0, DEEP), /* PCIE_WAKE1_B -- FULL_CARD_POWER_OFF */ line over 96 characters
https://review.coreboot.org/c/coreboot/+/34188/3/src/mainboard/google/octopu... File src/mainboard/google/octopus/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/34188/3/src/mainboard/google/octopu... PS3, Line 333: PAD_CFG_GPO_IOSSTATE_IOSTERM(GPIO_67, 0, DEEP, NONE, TxLASTRxE, DISPUPD), /* UART2-CTS_B -- EN_PP3300_DX_LTE_SOC */ line over 96 characters
https://review.coreboot.org/c/coreboot/+/34188/3/src/mainboard/google/octopu... PS3, Line 339: PAD_CFG_GPO_IOSSTATE_IOSTERM(GPIO_161, 1, DEEP, UP_20K, Tx1RxDCRx0, DISPUPD), /* AVS_I2S1_MCLK -- LTE_OFF_ODL */ line over 96 characters
https://review.coreboot.org/c/coreboot/+/34188/3/src/mainboard/google/octopu... File src/mainboard/google/octopus/variants/garg/gpio.c:
https://review.coreboot.org/c/coreboot/+/34188/3/src/mainboard/google/octopu... PS3, Line 22: #define DEFAULT_OVERRIDE_GPIOS() PAD_NC(GPIO_104, UP_20K), \ Macros with complex values should be enclosed in parentheses
Hello Patrick Rudolph, Karthik Ramasubramanian, Justin TerAvest, Marco Chen, Kevin Chiu, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34188
to look at the new patch set (#4).
Change subject: mb/google/octopus/variants/garg: support LTE power sequence. ......................................................................
mb/google/octopus/variants/garg: support LTE power sequence.
BUG=b:137033609 BRANCH=octopus TEST=build image and verify on the DUT with LTE DB.
Change-Id: I7bf6fee087c885c22363b44aa98aa61f91be90b4 Signed-off-by: Marco Chen marcochen@google.com --- M src/mainboard/google/octopus/bootblock.c M src/mainboard/google/octopus/smihandler.c M src/mainboard/google/octopus/variants/baseboard/gpio.c M src/mainboard/google/octopus/variants/baseboard/include/baseboard/gpio.h M src/mainboard/google/octopus/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/octopus/variants/garg/gpio.c 6 files changed, 93 insertions(+), 18 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/34188/4
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34188 )
Change subject: mb/google/octopus/variants/garg: support LTE power sequence. ......................................................................
Patch Set 4:
(5 comments)
https://review.coreboot.org/c/coreboot/+/34188/4/src/mainboard/google/octopu... File src/mainboard/google/octopus/smihandler.c:
https://review.coreboot.org/c/coreboot/+/34188/4/src/mainboard/google/octopu... PS4, Line 46: PAD_CFG_GPO(GPIO_117, 0, DEEP), /* PCIE_WAKE1_B -- FULL_CARD_POWER_OFF */ line over 96 characters
https://review.coreboot.org/c/coreboot/+/34188/4/src/mainboard/google/octopu... File src/mainboard/google/octopus/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/34188/4/src/mainboard/google/octopu... PS4, Line 229: PAD_CFG_GPO_IOSSTATE_IOSTERM(GPIO_161, 0, DEEP, NONE, TxLASTRxE, DISPUPD), /* AVS_I2S1_MCLK -- LTE_OFF_ODL */ line over 96 characters
https://review.coreboot.org/c/coreboot/+/34188/4/src/mainboard/google/octopu... PS4, Line 333: PAD_CFG_GPO_IOSSTATE_IOSTERM(GPIO_67, 0, DEEP, NONE, TxLASTRxE, DISPUPD), /* UART2-CTS_B -- EN_PP3300_DX_LTE_SOC */ line over 96 characters
https://review.coreboot.org/c/coreboot/+/34188/4/src/mainboard/google/octopu... PS4, Line 339: PAD_CFG_GPO_IOSSTATE_IOSTERM(GPIO_161, 0, DEEP, NONE, TxLASTRxE, DISPUPD), /* AVS_I2S1_MCLK -- LTE_OFF_ODL */ line over 96 characters
https://review.coreboot.org/c/coreboot/+/34188/4/src/mainboard/google/octopu... File src/mainboard/google/octopus/variants/garg/gpio.c:
https://review.coreboot.org/c/coreboot/+/34188/4/src/mainboard/google/octopu... PS4, Line 22: #define DEFAULT_OVERRIDE_GPIOS() PAD_NC(GPIO_104, UP_20K), \ Macros with complex values should be enclosed in parentheses
Hello Patrick Rudolph, Karthik Ramasubramanian, Justin TerAvest, Marco Chen, Kevin Chiu, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34188
to look at the new patch set (#5).
Change subject: mb/google/octopus/variants/garg: support LTE power sequence. ......................................................................
mb/google/octopus/variants/garg: support LTE power sequence.
BUG=b:137033609 BRANCH=octopus TEST=build image and verify on the DUT with LTE DB.
Change-Id: I7bf6fee087c885c22363b44aa98aa61f91be90b4 Signed-off-by: Marco Chen marcochen@google.com --- M src/mainboard/google/octopus/bootblock.c M src/mainboard/google/octopus/smihandler.c M src/mainboard/google/octopus/variants/baseboard/gpio.c M src/mainboard/google/octopus/variants/baseboard/include/baseboard/gpio.h M src/mainboard/google/octopus/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/octopus/variants/garg/gpio.c 6 files changed, 93 insertions(+), 18 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/34188/5
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34188 )
Change subject: mb/google/octopus/variants/garg: support LTE power sequence. ......................................................................
Patch Set 5:
(4 comments)
https://review.coreboot.org/c/coreboot/+/34188/5/src/mainboard/google/octopu... File src/mainboard/google/octopus/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/34188/5/src/mainboard/google/octopu... PS5, Line 229: PAD_CFG_GPO_IOSSTATE_IOSTERM(GPIO_161, 0, DEEP, NONE, TxLASTRxE, DISPUPD), /* AVS_I2S1_MCLK -- LTE_OFF_ODL */ line over 96 characters
https://review.coreboot.org/c/coreboot/+/34188/5/src/mainboard/google/octopu... PS5, Line 333: PAD_CFG_GPO_IOSSTATE_IOSTERM(GPIO_67, 0, DEEP, NONE, TxLASTRxE, DISPUPD), /* UART2-CTS_B -- EN_PP3300_DX_LTE_SOC */ line over 96 characters
https://review.coreboot.org/c/coreboot/+/34188/5/src/mainboard/google/octopu... PS5, Line 339: PAD_CFG_GPO_IOSSTATE_IOSTERM(GPIO_161, 0, DEEP, NONE, TxLASTRxE, DISPUPD), /* AVS_I2S1_MCLK -- LTE_OFF_ODL */ line over 96 characters
https://review.coreboot.org/c/coreboot/+/34188/5/src/mainboard/google/octopu... File src/mainboard/google/octopus/variants/garg/gpio.c:
https://review.coreboot.org/c/coreboot/+/34188/5/src/mainboard/google/octopu... PS5, Line 22: #define DEFAULT_OVERRIDE_GPIOS() PAD_NC(GPIO_104, UP_20K), \ Macros with complex values should be enclosed in parentheses
Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34188 )
Change subject: mb/google/octopus/variants/garg: support LTE power sequence. ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34188/1/src/mainboard/google/octopu... File src/mainboard/google/octopus/smihandler.c:
https://review.coreboot.org/c/coreboot/+/34188/1/src/mainboard/google/octopu... PS1, Line 37: !gpio_get_output(GPIO_LTE_RESET)
The intention is to reduce the effect of delaying to other variants and will check with HW Eng as we […]
Checked with kuten@ and the comment is keeping to enable LTE power should be fine for all variants. But personally would like to prevent delays here from non-LTE SKUs.
https://review.coreboot.org/c/coreboot/+/34188/1/src/mainboard/google/octopu... PS1, Line 56: lte_power_off_gpios
Copy that and will try.
Done.
https://review.coreboot.org/c/coreboot/+/34188/5/src/mainboard/google/octopu... File src/mainboard/google/octopus/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/34188/5/src/mainboard/google/octopu... PS5, Line 229: PAD_CFG_GPO_IOSSTATE_IOSTERM(GPIO_161, 0, DEEP, NONE, TxLASTRxE, DISPUPD), /* AVS_I2S1_MCLK -- LTE_OFF_ODL */
line over 96 characters
change to output low for non-LTE SKUs so will output high only in LTE SKU.
Hello Patrick Rudolph, Karthik Ramasubramanian, Justin TerAvest, Marco Chen, Kevin Chiu, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34188
to look at the new patch set (#6).
Change subject: mb/google/octopus/variants/garg: support LTE power sequence. ......................................................................
mb/google/octopus/variants/garg: support LTE power sequence.
BUG=b:137033609 BRANCH=octopus TEST=build image and verify on the DUT with LTE DB.
Change-Id: I7bf6fee087c885c22363b44aa98aa61f91be90b4 Signed-off-by: Marco Chen marcochen@google.com --- M src/mainboard/google/octopus/bootblock.c M src/mainboard/google/octopus/smihandler.c M src/mainboard/google/octopus/variants/baseboard/gpio.c M src/mainboard/google/octopus/variants/baseboard/include/baseboard/gpio.h M src/mainboard/google/octopus/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/octopus/variants/garg/gpio.c 6 files changed, 93 insertions(+), 18 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/34188/6
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34188 )
Change subject: mb/google/octopus/variants/garg: support LTE power sequence. ......................................................................
Patch Set 6:
(4 comments)
https://review.coreboot.org/c/coreboot/+/34188/6/src/mainboard/google/octopu... File src/mainboard/google/octopus/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/34188/6/src/mainboard/google/octopu... PS6, Line 229: PAD_CFG_GPO_IOSSTATE_IOSTERM(GPIO_161, 0, DEEP, NONE, TxLASTRxE, DISPUPD), /* AVS_I2S1_MCLK -- LTE_OFF_ODL */ line over 96 characters
https://review.coreboot.org/c/coreboot/+/34188/6/src/mainboard/google/octopu... PS6, Line 333: PAD_CFG_GPO_IOSSTATE_IOSTERM(GPIO_67, 0, DEEP, NONE, TxLASTRxE, DISPUPD), /* UART2-CTS_B -- EN_PP3300_DX_LTE_SOC */ line over 96 characters
https://review.coreboot.org/c/coreboot/+/34188/6/src/mainboard/google/octopu... PS6, Line 339: PAD_CFG_GPO_IOSSTATE_IOSTERM(GPIO_161, 0, DEEP, NONE, TxLASTRxE, DISPUPD), /* AVS_I2S1_MCLK -- LTE_OFF_ODL */ line over 96 characters
https://review.coreboot.org/c/coreboot/+/34188/6/src/mainboard/google/octopu... File src/mainboard/google/octopus/variants/garg/gpio.c:
https://review.coreboot.org/c/coreboot/+/34188/6/src/mainboard/google/octopu... PS6, Line 22: #define DEFAULT_OVERRIDE_GPIOS() PAD_NC(GPIO_104, UP_20K), \ Macros with complex values should be enclosed in parentheses
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34188 )
Change subject: mb/google/octopus/variants/garg: support LTE power sequence. ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34188/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34188/6//COMMIT_MSG@7 PS6, Line 7: . No period at the end of commit summaries, please.
Kevin Chiu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34188 )
Change subject: mb/google/octopus/variants/garg: support LTE power sequence. ......................................................................
Patch Set 6: Code-Review+1
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34188 )
Change subject: mb/google/octopus/variants/garg: support LTE power sequence. ......................................................................
Patch Set 6:
(5 comments)
https://review.coreboot.org/c/coreboot/+/34188/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34188/6//COMMIT_MSG@8 PS6, Line 8: Can you please add some description about the power sequence requirements and timing here?
https://review.coreboot.org/c/coreboot/+/34188/6/src/mainboard/google/octopu... File src/mainboard/google/octopus/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/34188/6/src/mainboard/google/octopu... PS6, Line 97: I think we should not remove the configuration for GPIO_67, GPIO_117 for the benefit of non-LTE skus. Otherwise atleast on Garg, these gpios will be left driving 1. Rather we should override the configuration in Garg to continue driving 1 for LTE skus.
https://review.coreboot.org/c/coreboot/+/34188/6/src/mainboard/google/octopu... File src/mainboard/google/octopus/variants/garg/gpio.c:
https://review.coreboot.org/c/coreboot/+/34188/6/src/mainboard/google/octopu... PS6, Line 22: ) Not sure if we need paranthesis i.e if we need this to look like a function.
https://review.coreboot.org/c/coreboot/+/34188/6/src/mainboard/google/octopu... PS6, Line 26: , \ You don't need this in the last line of a macro. Also use the comma at the place where you are referring to the macro
https://review.coreboot.org/c/coreboot/+/34188/6/src/mainboard/google/octopu... PS6, Line 22: #define DEFAULT_OVERRIDE_GPIOS() PAD_NC(GPIO_104, UP_20K), \ : /* EN_PP3300_TOUCHSCREEN */ \ : PAD_CFG_GPO_IOSSTATE_IOSTERM(GPIO_146, 0, DEEP, NONE, Tx0RxDCRx0, \ : DISPUPD), \ : PAD_NC(GPIO_213, DN_20K), \ : Prefer introducing this macro as a separate CL and place this CL on top of that CL.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34188 )
Change subject: mb/google/octopus/variants/garg: support LTE power sequence. ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34188/6/src/mainboard/google/octopu... File src/mainboard/google/octopus/smihandler.c:
https://review.coreboot.org/c/coreboot/+/34188/6/src/mainboard/google/octopu... PS6, Line 37: gpio_get_output Just curious: can we not use sku id to determine if it is a LTE SKU and then do the power off sequence? That way we don't have to introduce this new function?
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34188 )
Change subject: mb/google/octopus/variants/garg: support LTE power sequence. ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34188/6/src/mainboard/google/octopu... File src/mainboard/google/octopus/smihandler.c:
https://review.coreboot.org/c/coreboot/+/34188/6/src/mainboard/google/octopu... PS6, Line 37: gpio_get_output
Just curious: can we not use sku id to determine if it is a LTE SKU and then do the power off sequen […]
Just realized that this is in the mainboard and checking for sku will not work.
Another option is to introduce a octopus wide flag which indicates the presence or absence of lte module and then set it at the variants which have the lte module. Then use the state of that flag here to power off lte modules.
Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34188 )
Change subject: mb/google/octopus/variants/garg: support LTE power sequence. ......................................................................
Patch Set 6:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34188/6/src/mainboard/google/octopu... File src/mainboard/google/octopus/smihandler.c:
https://review.coreboot.org/c/coreboot/+/34188/6/src/mainboard/google/octopu... PS6, Line 37: gpio_get_output
Just realized that this is in the mainboard and checking for sku will not work. […]
Like codes for CNVi module, we check some values in runtime to determine whether it exists. Here, I think RESET pin is the hint for us to judge or is there any issue here?
https://review.coreboot.org/c/coreboot/+/34188/6/src/mainboard/google/octopu... File src/mainboard/google/octopus/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/34188/6/src/mainboard/google/octopu... PS6, Line 97:
I think we should not remove the configuration for GPIO_67, GPIO_117 for the benefit of non-LTE skus […]
The previous comment mentioned about after confirming with HW eng - kuten@, these pins can be remained to 1 no matter LTE or non-LTE skus in garg. As a result, the CL didn't drive them to 0 in ram stage when in non-LTE skus.
Otherwise, if we want to play it safe then I can add this logic. thought?
https://review.coreboot.org/c/coreboot/+/34188/6/src/mainboard/google/octopu... File src/mainboard/google/octopus/variants/garg/gpio.c:
https://review.coreboot.org/c/coreboot/+/34188/6/src/mainboard/google/octopu... PS6, Line 82: PAD_CFG_GPO(GPIO_161, 0, DEEP), By the way, the reason for setting 67/117 to PWROK and 161 to DEEP is for power sequence of warm boot, where we need RESET (161) to toggle but keep others the same (hight). Will add a comment for whole power sequence requirement we consider to.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34188 )
Change subject: mb/google/octopus/variants/garg: support LTE power sequence. ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34188/1/src/mainboard/google/octopu... File src/mainboard/google/octopus/smihandler.c:
https://review.coreboot.org/c/coreboot/+/34188/1/src/mainboard/google/octopu... PS1, Line 37: !gpio_get_output(GPIO_LTE_RESET)
The intention is to reduce the effect of delaying to other variants and will check with HW Eng as well.
What do you mean? How will it reduce delay?
Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34188 )
Change subject: mb/google/octopus/variants/garg: support LTE power sequence. ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34188/1/src/mainboard/google/octopu... File src/mainboard/google/octopus/smihandler.c:
https://review.coreboot.org/c/coreboot/+/34188/1/src/mainboard/google/octopu... PS1, Line 37: !gpio_get_output(GPIO_LTE_RESET)
The intention is to reduce the effect of delaying to other variants and will check with HW Eng as […]
Without the checking, more then 130ms of delay will be applied to any SKUs during shutdown no matter there is a LTE or not.
With this simple approach (not heavy), we can limit the 130ms delay for LTE sku only and necessary for power down sequence.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34188 )
Change subject: mb/google/octopus/variants/garg: support LTE power sequence. ......................................................................
Patch Set 6:
(5 comments)
https://review.coreboot.org/c/coreboot/+/34188/1/src/mainboard/google/octopu... File src/mainboard/google/octopus/smihandler.c:
https://review.coreboot.org/c/coreboot/+/34188/1/src/mainboard/google/octopu... PS1, Line 37: !gpio_get_output(GPIO_LTE_RESET)
Without the checking, more then 130ms of delay will be applied to any SKUs during shutdown no matter […]
Ideally it should not matter in case of shutdown. But, I think this could be called from variant specific code so that we don't have to rely on the state of a GPIO to decide LTE presence. I think it is very brittle. In case any future variants decide to use that GPIO for a different purpose things would be broken.
https://review.coreboot.org/c/coreboot/+/34188/6/src/mainboard/google/octopu... File src/mainboard/google/octopus/smihandler.c:
https://review.coreboot.org/c/coreboot/+/34188/6/src/mainboard/google/octopu... PS6, Line 37: gpio_get_output
Like codes for CNVi module, we check some values in runtime to determine whether it exists. […]
In general, I don't think it is a good idea to rely on the state of a GPIO to decide presence of the module.
https://review.coreboot.org/c/coreboot/+/34188/6/src/mainboard/google/octopu... PS6, Line 75: power_off_lte_module_if_exist I think it would be better to add variant_smi_sleep() which calls power_off_lte_module(). That way you don't have to rely on the state of a GPIO and can use SKU ID just like initialization to decide if the GPIOs need to be toggled for LTE power off.
https://review.coreboot.org/c/coreboot/+/34188/6/src/mainboard/google/octopu... File src/mainboard/google/octopus/variants/garg/gpio.c:
https://review.coreboot.org/c/coreboot/+/34188/6/src/mainboard/google/octopu... PS6, Line 22: )
Not sure if we need paranthesis i.e if we need this to look like a function.
Just a personal opinion: I am generally scared using such complicated macros :). Since it is just 3 GPIOs probably okay to just duplicate them.
https://review.coreboot.org/c/coreboot/+/34188/6/src/mainboard/google/octopu... PS6, Line 82: PAD_CFG_GPO(GPIO_161, 0, DEEP),
By the way, the reason for setting 67/117 to PWROK and 161 to DEEP is for power sequence of warm boo […]
What system power states are you expecting RESET to toggle but power to remain enabled?
Hello Patrick Rudolph, Karthik Ramasubramanian, Justin TerAvest, Marco Chen, Kevin Chiu, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34188
to look at the new patch set (#7).
Change subject: mb/google/octopus/variants/garg: support LTE power sequence ......................................................................
mb/google/octopus/variants/garg: support LTE power sequence
GPIOs related to power sequnce are GPIO_67 - EN_PP3300 GPIO_117 - FULL_CARD_POWER_ON_OFF GPIO_161 - PLT_RST_LTE_L 1. Power on: GPIO_67 -> 0ms -> GPIO_117 -> 30ms -> GPIO_161 2. Power off: GPIO_161 -> 30ms -> GPIO_117 -> 100ms -> GPIO_67 3. Power reset: - keep GPIO_67 and GPIO_117 high and - pull down GPIO_161 for 30ms then release it.
BUG=b:137033609 BRANCH=octopus TEST=build image and verify on the DUT with LTE DB.
Change-Id: I7bf6fee087c885c22363b44aa98aa61f91be90b4 Signed-off-by: Marco Chen marcochen@google.com --- M src/mainboard/google/octopus/bootblock.c M src/mainboard/google/octopus/smihandler.c M src/mainboard/google/octopus/variants/baseboard/gpio.c M src/mainboard/google/octopus/variants/baseboard/include/baseboard/gpio.h M src/mainboard/google/octopus/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/octopus/variants/garg/gpio.c M src/mainboard/google/octopus/variants/garg/variant.c 7 files changed, 117 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/34188/7
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34188 )
Change subject: mb/google/octopus/variants/garg: support LTE power sequence ......................................................................
Patch Set 7:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34188/7/src/mainboard/google/octopu... File src/mainboard/google/octopus/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/34188/7/src/mainboard/google/octopu... PS7, Line 229: PAD_CFG_GPO_IOSSTATE_IOSTERM(GPIO_161, 0, DEEP, NONE, TxLASTRxE, DISPUPD), /* AVS_I2S1_MCLK -- LTE_OFF_ODL */ line over 96 characters
https://review.coreboot.org/c/coreboot/+/34188/7/src/mainboard/google/octopu... PS7, Line 333: PAD_CFG_GPO_IOSSTATE_IOSTERM(GPIO_67, 0, DEEP, NONE, TxLASTRxE, DISPUPD), /* UART2-CTS_B -- EN_PP3300_DX_LTE_SOC */ line over 96 characters
https://review.coreboot.org/c/coreboot/+/34188/7/src/mainboard/google/octopu... PS7, Line 339: PAD_CFG_GPO_IOSSTATE_IOSTERM(GPIO_161, 0, DEEP, NONE, TxLASTRxE, DISPUPD), /* AVS_I2S1_MCLK -- LTE_OFF_ODL */ line over 96 characters
Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34188 )
Change subject: mb/google/octopus/variants/garg: support LTE power sequence ......................................................................
Patch Set 7:
(6 comments)
https://review.coreboot.org/c/coreboot/+/34188/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34188/6//COMMIT_MSG@7 PS6, Line 7: .
No period at the end of commit summaries, please.
Done
https://review.coreboot.org/c/coreboot/+/34188/6//COMMIT_MSG@8 PS6, Line 8:
Can you please add some description about the power sequence requirements and timing here?
Done
https://review.coreboot.org/c/coreboot/+/34188/1/src/mainboard/google/octopu... File src/mainboard/google/octopus/smihandler.c:
https://review.coreboot.org/c/coreboot/+/34188/1/src/mainboard/google/octopu... PS1, Line 37: !gpio_get_output(GPIO_LTE_RESET)
Ideally it should not matter in case of shutdown. […]
Ack
https://review.coreboot.org/c/coreboot/+/34188/6/src/mainboard/google/octopu... File src/mainboard/google/octopus/smihandler.c:
https://review.coreboot.org/c/coreboot/+/34188/6/src/mainboard/google/octopu... PS6, Line 75: power_off_lte_module_if_exist
I think it would be better to add variant_smi_sleep() which calls power_off_lte_module(). […]
Done
https://review.coreboot.org/c/coreboot/+/34188/6/src/mainboard/google/octopu... File src/mainboard/google/octopus/variants/garg/gpio.c:
https://review.coreboot.org/c/coreboot/+/34188/6/src/mainboard/google/octopu... PS6, Line 22: )
Just a personal opinion: I am generally scared using such complicated macros :). […]
Ack
https://review.coreboot.org/c/coreboot/+/34188/6/src/mainboard/google/octopu... PS6, Line 82: PAD_CFG_GPO(GPIO_161, 0, DEEP),
What system power states are you expecting RESET to toggle but power to remain enabled?
Warm boot of SoC and follow Figure 3-8 in L850-GL datasheet.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34188 )
Change subject: mb/google/octopus/variants/garg: support LTE power sequence ......................................................................
Patch Set 7:
(2 comments)
Now that we are no longer using gpio_get_output function, we can abandon those CLs and remove the dependencies for this CL.
https://review.coreboot.org/c/coreboot/+/34188/6/src/mainboard/google/octopu... File src/mainboard/google/octopus/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/34188/6/src/mainboard/google/octopu... PS6, Line 97:
The previous comment mentioned about […]
Personal Opinion: Safe to put it back in the default state for non-LTE SKUs
https://review.coreboot.org/c/coreboot/+/34188/7/src/mainboard/google/octopu... File src/mainboard/google/octopus/variants/garg/variant.c:
https://review.coreboot.org/c/coreboot/+/34188/7/src/mainboard/google/octopu... PS7, Line 79: power_off_lte_module_if_exist Fix the indentation issue. May need an additional tab to match alignment with return statement.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34188 )
Change subject: mb/google/octopus/variants/garg: support LTE power sequence ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34188/7/src/mainboard/google/octopu... File src/mainboard/google/octopus/variants/baseboard/include/baseboard/gpio.h:
https://review.coreboot.org/c/coreboot/+/34188/7/src/mainboard/google/octopu... PS7, Line 36: /* LTE reset GPIO */ : #define GPIO_LTE_RESET GPIO_161 : Not required anymore.
https://review.coreboot.org/c/coreboot/+/34188/7/src/mainboard/google/octopu... File src/mainboard/google/octopus/variants/garg/variant.c:
https://review.coreboot.org/c/coreboot/+/34188/7/src/mainboard/google/octopu... PS7, Line 37: if (slp_typ != ACPI_S5) : return; nit: If you do this before calling get_board_sku(), one transaction to the EC can be saved in non-S5 cases.
Hello Patrick Rudolph, Karthik Ramasubramanian, Justin TerAvest, Marco Chen, Kevin Chiu, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34188
to look at the new patch set (#8).
Change subject: mb/google/octopus/variants/garg: support LTE power sequence ......................................................................
mb/google/octopus/variants/garg: support LTE power sequence
GPIOs related to power sequnce are GPIO_67 - EN_PP3300 GPIO_117 - FULL_CARD_POWER_ON_OFF GPIO_161 - PLT_RST_LTE_L 1. Power on: GPIO_67 -> 0ms -> GPIO_117 -> 30ms -> GPIO_161 2. Power off: GPIO_161 -> 30ms -> GPIO_117 -> 100ms -> GPIO_67 3. Power reset: - keep GPIO_67 and GPIO_117 high and - pull down GPIO_161 for 30ms then release it.
BUG=b:137033609 BRANCH=octopus TEST=build image and verify on the DUT with LTE DB.
Change-Id: I7bf6fee087c885c22363b44aa98aa61f91be90b4 Signed-off-by: Marco Chen marcochen@google.com --- M src/mainboard/google/octopus/bootblock.c M src/mainboard/google/octopus/smihandler.c M src/mainboard/google/octopus/variants/baseboard/gpio.c M src/mainboard/google/octopus/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/octopus/variants/garg/gpio.c M src/mainboard/google/octopus/variants/garg/variant.c 6 files changed, 128 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/34188/8
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34188 )
Change subject: mb/google/octopus/variants/garg: support LTE power sequence ......................................................................
Patch Set 8:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34188/8/src/mainboard/google/octopu... File src/mainboard/google/octopus/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/34188/8/src/mainboard/google/octopu... PS8, Line 231: PAD_CFG_GPO_IOSSTATE_IOSTERM(GPIO_161, 0, DEEP, NONE, TxLASTRxE, DISPUPD), /* AVS_I2S1_MCLK -- LTE_OFF_ODL */ line over 96 characters
https://review.coreboot.org/c/coreboot/+/34188/8/src/mainboard/google/octopu... PS8, Line 335: PAD_CFG_GPO_IOSSTATE_IOSTERM(GPIO_67, 0, DEEP, NONE, TxLASTRxE, DISPUPD), /* UART2-CTS_B -- EN_PP3300_DX_LTE_SOC */ line over 96 characters
https://review.coreboot.org/c/coreboot/+/34188/8/src/mainboard/google/octopu... PS8, Line 341: PAD_CFG_GPO_IOSSTATE_IOSTERM(GPIO_161, 0, DEEP, NONE, TxLASTRxE, DISPUPD), /* AVS_I2S1_MCLK -- LTE_OFF_ODL */ line over 96 characters
Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34188 )
Change subject: mb/google/octopus/variants/garg: support LTE power sequence ......................................................................
Patch Set 8:
(4 comments)
https://review.coreboot.org/c/coreboot/+/34188/6/src/mainboard/google/octopu... File src/mainboard/google/octopus/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/34188/6/src/mainboard/google/octopu... PS6, Line 97:
Personal Opinion: Safe to put it back in the default state for non-LTE SKUs
One reviewer asked to evaluate whether all SKUs of garg can stay in the state of LTE SKUs and one reviewer suggest to play it safe.
My previous comment is also to play it safe if codes is manageable so I choose to follow this way.
https://review.coreboot.org/c/coreboot/+/34188/7/src/mainboard/google/octopu... File src/mainboard/google/octopus/variants/baseboard/include/baseboard/gpio.h:
https://review.coreboot.org/c/coreboot/+/34188/7/src/mainboard/google/octopu... PS7, Line 36: /* LTE reset GPIO */ : #define GPIO_LTE_RESET GPIO_161 :
Not required anymore.
Done
https://review.coreboot.org/c/coreboot/+/34188/7/src/mainboard/google/octopu... File src/mainboard/google/octopus/variants/garg/variant.c:
https://review.coreboot.org/c/coreboot/+/34188/7/src/mainboard/google/octopu... PS7, Line 37: if (slp_typ != ACPI_S5) : return;
nit: If you do this before calling get_board_sku(), one transaction to the EC can be saved in non-S5 […]
Yes and also add the comment to remind the reason in variant_smi_sleep().
https://review.coreboot.org/c/coreboot/+/34188/7/src/mainboard/google/octopu... PS7, Line 79: power_off_lte_module_if_exist
Fix the indentation issue. May need an additional tab to match alignment with return statement.
Done
Hello Patrick Rudolph, Karthik Ramasubramanian, Justin TerAvest, Marco Chen, Kevin Chiu, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34188
to look at the new patch set (#9).
Change subject: mb/google/octopus/variants/garg: support LTE power sequence ......................................................................
mb/google/octopus/variants/garg: support LTE power sequence
GPIOs related to power sequnce are GPIO_67 - EN_PP3300 GPIO_117 - FULL_CARD_POWER_ON_OFF GPIO_161 - PLT_RST_LTE_L 1. Power on: GPIO_67 -> 0ms -> GPIO_117 -> 30ms -> GPIO_161 2. Power off: GPIO_161 -> 30ms -> GPIO_117 -> 100ms -> GPIO_67 3. Power reset: - keep GPIO_67 and GPIO_117 high and - pull down GPIO_161 for 30ms then release it.
BUG=b:137033609 BRANCH=octopus TEST=build image and verify on the DUT with LTE DB.
Change-Id: I7bf6fee087c885c22363b44aa98aa61f91be90b4 Signed-off-by: Marco Chen marcochen@google.com --- M src/mainboard/google/octopus/bootblock.c M src/mainboard/google/octopus/smihandler.c M src/mainboard/google/octopus/variants/baseboard/gpio.c M src/mainboard/google/octopus/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/octopus/variants/garg/gpio.c M src/mainboard/google/octopus/variants/garg/variant.c 6 files changed, 122 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/34188/9
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34188 )
Change subject: mb/google/octopus/variants/garg: support LTE power sequence ......................................................................
Patch Set 9:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34188/9/src/mainboard/google/octopu... File src/mainboard/google/octopus/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/34188/9/src/mainboard/google/octopu... PS9, Line 231: PAD_CFG_GPO_IOSSTATE_IOSTERM(GPIO_161, 1, DEEP, NONE, TxLASTRxE, DISPUPD), /* AVS_I2S1_MCLK -- LTE_OFF_ODL */ line over 96 characters
https://review.coreboot.org/c/coreboot/+/34188/9/src/mainboard/google/octopu... PS9, Line 335: PAD_CFG_GPO_IOSSTATE_IOSTERM(GPIO_67, 0, DEEP, NONE, TxLASTRxE, DISPUPD), /* UART2-CTS_B -- EN_PP3300_DX_LTE_SOC */ line over 96 characters
https://review.coreboot.org/c/coreboot/+/34188/9/src/mainboard/google/octopu... PS9, Line 341: PAD_CFG_GPO_IOSSTATE_IOSTERM(GPIO_161, 0, DEEP, NONE, TxLASTRxE, DISPUPD), /* AVS_I2S1_MCLK -- LTE_OFF_ODL */ line over 96 characters
Hello Patrick Rudolph, Karthik Ramasubramanian, Justin TerAvest, Marco Chen, Kevin Chiu, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34188
to look at the new patch set (#10).
Change subject: mb/google/octopus/variants/garg: support LTE power sequence ......................................................................
mb/google/octopus/variants/garg: support LTE power sequence
GPIOs related to power sequnce are GPIO_67 - EN_PP3300 GPIO_117 - FULL_CARD_POWER_ON_OFF GPIO_161 - PLT_RST_LTE_L 1. Power on: GPIO_67 -> 0ms -> GPIO_117 -> 30ms -> GPIO_161 2. Power off: GPIO_161 -> 30ms -> GPIO_117 -> 100ms -> GPIO_67 3. Power reset: - keep GPIO_67 and GPIO_117 high and - pull down GPIO_161 for 30ms then release it.
BUG=b:137033609 BRANCH=octopus TEST=build image and verify on the DUT with LTE DB.
Change-Id: I7bf6fee087c885c22363b44aa98aa61f91be90b4 Signed-off-by: Marco Chen marcochen@google.com --- M src/mainboard/google/octopus/bootblock.c M src/mainboard/google/octopus/smihandler.c M src/mainboard/google/octopus/variants/baseboard/gpio.c M src/mainboard/google/octopus/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/octopus/variants/garg/gpio.c M src/mainboard/google/octopus/variants/garg/variant.c 6 files changed, 125 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/34188/10
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34188 )
Change subject: mb/google/octopus/variants/garg: support LTE power sequence ......................................................................
Patch Set 10:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34188/10/src/mainboard/google/octop... File src/mainboard/google/octopus/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/34188/10/src/mainboard/google/octop... PS10, Line 339: PAD_CFG_GPO_IOSSTATE_IOSTERM(GPIO_67, 0, DEEP, NONE, TxLASTRxE, DISPUPD), /* UART2-CTS_B -- EN_PP3300_DX_LTE_SOC */ line over 96 characters
https://review.coreboot.org/c/coreboot/+/34188/10/src/mainboard/google/octop... PS10, Line 345: PAD_CFG_GPO_IOSSTATE_IOSTERM(GPIO_161, 0, DEEP, UP_20K, Tx1RxDCRx0, DISPUPD), /* AVS_I2S1_MCLK -- LTE_OFF_ODL */ line over 96 characters
Hello Patrick Rudolph, Karthik Ramasubramanian, Justin TerAvest, Marco Chen, Kevin Chiu, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34188
to look at the new patch set (#11).
Change subject: mb/google/octopus/variants/garg: support LTE power sequence ......................................................................
mb/google/octopus/variants/garg: support LTE power sequence
GPIOs related to power sequnce are GPIO_67 - EN_PP3300 GPIO_117 - FULL_CARD_POWER_ON_OFF GPIO_161 - PLT_RST_LTE_L 1. Power on: GPIO_67 -> 0ms -> GPIO_117 -> 30ms -> GPIO_161 2. Power off: GPIO_161 -> 30ms -> GPIO_117 -> 100ms -> GPIO_67 3. Power reset: - keep GPIO_67 and GPIO_117 high and - pull down GPIO_161 for 30ms then release it.
BUG=b:137033609 BRANCH=octopus TEST=build image and verify on the DUT with LTE DB.
Change-Id: I7bf6fee087c885c22363b44aa98aa61f91be90b4 Signed-off-by: Marco Chen marcochen@google.com --- M src/mainboard/google/octopus/bootblock.c M src/mainboard/google/octopus/smihandler.c M src/mainboard/google/octopus/variants/baseboard/gpio.c M src/mainboard/google/octopus/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/octopus/variants/garg/gpio.c M src/mainboard/google/octopus/variants/garg/variant.c 6 files changed, 126 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/34188/11
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34188 )
Change subject: mb/google/octopus/variants/garg: support LTE power sequence ......................................................................
Patch Set 11:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34188/11/src/mainboard/google/octop... File src/mainboard/google/octopus/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/34188/11/src/mainboard/google/octop... PS11, Line 340: PAD_CFG_GPO_IOSSTATE_IOSTERM(GPIO_67, 0, DEEP, NONE, TxLASTRxE, DISPUPD), /* UART2-CTS_B -- EN_PP3300_DX_LTE_SOC */ line over 96 characters
https://review.coreboot.org/c/coreboot/+/34188/11/src/mainboard/google/octop... PS11, Line 346: PAD_CFG_GPO_IOSSTATE_IOSTERM(GPIO_161, 0, DEEP, UP_20K, Tx1RxDCRx0, DISPUPD), /* AVS_I2S1_MCLK -- LTE_OFF_ODL */ line over 96 characters
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34188 )
Change subject: mb/google/octopus/variants/garg: support LTE power sequence ......................................................................
Patch Set 11:
(4 comments)
https://review.coreboot.org/c/coreboot/+/34188/11//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34188/11//COMMIT_MSG@8 PS11, Line 8: Can you please also add details that this CL adds 2 new variant APIs: variant_smi_sleep variant_early_override_gpio_table
https://review.coreboot.org/c/coreboot/+/34188/11/src/mainboard/google/octop... File src/mainboard/google/octopus/smihandler.c:
https://review.coreboot.org/c/coreboot/+/34188/11/src/mainboard/google/octop... PS11, Line 22: #include <gpio.h> Not required?
https://review.coreboot.org/c/coreboot/+/34188/11/src/mainboard/google/octop... File src/mainboard/google/octopus/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/34188/11/src/mainboard/google/octop... PS11, Line 346: UP_20K Why is PU required here when this pad is being actively driven?
https://review.coreboot.org/c/coreboot/+/34188/11/src/mainboard/google/octop... File src/mainboard/google/octopus/variants/garg/gpio.c:
https://review.coreboot.org/c/coreboot/+/34188/11/src/mainboard/google/octop... PS11, Line 110: lte_early_override_table I believe you already confirmed that it is okay to configure the GPIOs this way on non-LTE SKUs?
Hello Patrick Rudolph, Karthik Ramasubramanian, Justin TerAvest, Marco Chen, Kevin Chiu, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34188
to look at the new patch set (#12).
Change subject: mb/google/octopus/variants/garg: support LTE power sequence ......................................................................
mb/google/octopus/variants/garg: support LTE power sequence
GPIOs related to power sequnce are GPIO_67 - EN_PP3300 GPIO_117 - FULL_CARD_POWER_ON_OFF GPIO_161 - PLT_RST_LTE_L 1. Power on: GPIO_67 -> 0ms -> GPIO_117 -> 30ms -> GPIO_161 2. Power off: GPIO_161 -> 30ms -> GPIO_117 -> 100ms -> GPIO_67 3. Power reset: - keep GPIO_67 and GPIO_117 high and - pull down GPIO_161 for 30ms then release it.
BUG=b:137033609 BRANCH=octopus TEST=build image and verify on the DUT with LTE DB.
Change-Id: I7bf6fee087c885c22363b44aa98aa61f91be90b4 Signed-off-by: Marco Chen marcochen@google.com --- M src/mainboard/google/octopus/variants/baseboard/gpio.c M src/mainboard/google/octopus/variants/garg/gpio.c M src/mainboard/google/octopus/variants/garg/variant.c 3 files changed, 103 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/34188/12
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34188 )
Change subject: mb/google/octopus/variants/garg: support LTE power sequence ......................................................................
Patch Set 12:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34188/12/src/mainboard/google/octop... File src/mainboard/google/octopus/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/34188/12/src/mainboard/google/octop... PS12, Line 340: PAD_CFG_GPO_IOSSTATE_IOSTERM(GPIO_67, 0, DEEP, NONE, TxLASTRxE, DISPUPD), /* UART2-CTS_B -- EN_PP3300_DX_LTE_SOC */ line over 96 characters
https://review.coreboot.org/c/coreboot/+/34188/12/src/mainboard/google/octop... PS12, Line 346: PAD_CFG_GPO_IOSSTATE_IOSTERM(GPIO_161, 1, DEEP, UP_20K, Tx1RxDCRx0, DISPUPD), /* AVS_I2S1_MCLK -- LTE_OFF_ODL */ line over 96 characters
Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34188 )
Change subject: mb/google/octopus/variants/garg: support LTE power sequence ......................................................................
Patch Set 12:
(4 comments)
https://review.coreboot.org/c/coreboot/+/34188/11//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34188/11//COMMIT_MSG@8 PS11, Line 8:
Can you please also add details that this CL adds 2 new variant APIs: […]
Referring to other practice, I split CLs for adding each variant APIs above.
https://review.coreboot.org/c/coreboot/+/34188/11/src/mainboard/google/octop... File src/mainboard/google/octopus/smihandler.c:
https://review.coreboot.org/c/coreboot/+/34188/11/src/mainboard/google/octop... PS11, Line 22: #include <gpio.h>
Not required?
Done
https://review.coreboot.org/c/coreboot/+/34188/11/src/mainboard/google/octop... File src/mainboard/google/octopus/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/34188/11/src/mainboard/google/octop... PS11, Line 346: UP_20K
Why is PU required here when this pad is being actively driven?
Just copy the original GPIO configuration from line 231 in gpio_table as the defautl stage.
https://review.coreboot.org/c/coreboot/+/34188/11/src/mainboard/google/octop... File src/mainboard/google/octopus/variants/garg/gpio.c:
https://review.coreboot.org/c/coreboot/+/34188/11/src/mainboard/google/octop... PS11, Line 110: lte_early_override_table
I believe you already confirmed that it is okay to configure the GPIOs this way on non-LTE SKUs?
Yes, as my previous comment to Karthik. I confirmed with kuten@ about it is fine to configure GPIOs for LTE SKU in non-LTE SKUs.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34188 )
Change subject: mb/google/octopus/variants/garg: support LTE power sequence ......................................................................
Patch Set 12:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34188/11//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34188/11//COMMIT_MSG@8 PS11, Line 8:
Referring to other practice, I split CLs for adding each variant APIs above.
That's perfect :). Thanks for doing that.
https://review.coreboot.org/c/coreboot/+/34188/11/src/mainboard/google/octop... File src/mainboard/google/octopus/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/34188/11/src/mainboard/google/octop... PS11, Line 346: UP_20K
Just copy the original GPIO configuration from line 231 in gpio_table as the defautl stage.
Ah I see. I think we should clean up the UP_20K from here and gpio_table as a follow up.
https://review.coreboot.org/c/coreboot/+/34188/11/src/mainboard/google/octop... File src/mainboard/google/octopus/variants/garg/gpio.c:
https://review.coreboot.org/c/coreboot/+/34188/11/src/mainboard/google/octop... PS11, Line 110: lte_early_override_table
Yes, as my previous comment to Karthik. […]
Thanks for confirming.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34188 )
Change subject: mb/google/octopus/variants/garg: support LTE power sequence ......................................................................
Patch Set 12: Code-Review+1
LGTM. Let's wait for Karthik to approve the change.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34188 )
Change subject: mb/google/octopus/variants/garg: support LTE power sequence ......................................................................
Patch Set 12: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/34188/12/src/mainboard/google/octop... File src/mainboard/google/octopus/variants/garg/variant.c:
https://review.coreboot.org/c/coreboot/+/34188/12/src/mainboard/google/octop... PS12, Line 35: _if_exist nit: _if_exist can be removed since we are sure at this point that LTE module exists
Hello Patrick Rudolph, Karthik Ramasubramanian, Justin TerAvest, Marco Chen, Kevin Chiu, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34188
to look at the new patch set (#13).
Change subject: mb/google/octopus/variants/garg: support LTE power sequence ......................................................................
mb/google/octopus/variants/garg: support LTE power sequence
GPIOs related to power sequnce are GPIO_67 - EN_PP3300 GPIO_117 - FULL_CARD_POWER_ON_OFF GPIO_161 - PLT_RST_LTE_L 1. Power on: GPIO_67 -> 0ms -> GPIO_117 -> 30ms -> GPIO_161 2. Power off: GPIO_161 -> 30ms -> GPIO_117 -> 100ms -> GPIO_67 3. Power reset: - keep GPIO_67 and GPIO_117 high and - pull down GPIO_161 for 30ms then release it.
BUG=b:137033609 BRANCH=octopus TEST=build image and verify on the DUT with LTE DB.
Change-Id: I7bf6fee087c885c22363b44aa98aa61f91be90b4 Signed-off-by: Marco Chen marcochen@google.com --- M src/mainboard/google/octopus/variants/baseboard/gpio.c M src/mainboard/google/octopus/variants/garg/gpio.c M src/mainboard/google/octopus/variants/garg/variant.c 3 files changed, 103 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/34188/13
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34188 )
Change subject: mb/google/octopus/variants/garg: support LTE power sequence ......................................................................
Patch Set 13:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34188/13/src/mainboard/google/octop... File src/mainboard/google/octopus/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/34188/13/src/mainboard/google/octop... PS13, Line 340: PAD_CFG_GPO_IOSSTATE_IOSTERM(GPIO_67, 0, DEEP, NONE, TxLASTRxE, DISPUPD), /* UART2-CTS_B -- EN_PP3300_DX_LTE_SOC */ line over 96 characters
https://review.coreboot.org/c/coreboot/+/34188/13/src/mainboard/google/octop... PS13, Line 346: PAD_CFG_GPO_IOSSTATE_IOSTERM(GPIO_161, 1, DEEP, UP_20K, Tx1RxDCRx0, DISPUPD), /* AVS_I2S1_MCLK -- LTE_OFF_ODL */ line over 96 characters
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34188 )
Change subject: mb/google/octopus/variants/garg: support LTE power sequence ......................................................................
Patch Set 13: Code-Review+2
Martin Roth has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34188 )
Change subject: mb/google/octopus/variants/garg: support LTE power sequence ......................................................................
mb/google/octopus/variants/garg: support LTE power sequence
GPIOs related to power sequnce are GPIO_67 - EN_PP3300 GPIO_117 - FULL_CARD_POWER_ON_OFF GPIO_161 - PLT_RST_LTE_L 1. Power on: GPIO_67 -> 0ms -> GPIO_117 -> 30ms -> GPIO_161 2. Power off: GPIO_161 -> 30ms -> GPIO_117 -> 100ms -> GPIO_67 3. Power reset: - keep GPIO_67 and GPIO_117 high and - pull down GPIO_161 for 30ms then release it.
BUG=b:137033609 BRANCH=octopus TEST=build image and verify on the DUT with LTE DB.
Change-Id: I7bf6fee087c885c22363b44aa98aa61f91be90b4 Signed-off-by: Marco Chen marcochen@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/34188 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Karthik Ramasubramanian kramasub@google.com --- M src/mainboard/google/octopus/variants/baseboard/gpio.c M src/mainboard/google/octopus/variants/garg/gpio.c M src/mainboard/google/octopus/variants/garg/variant.c 3 files changed, 103 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Karthik Ramasubramanian: Looks good to me, approved
diff --git a/src/mainboard/google/octopus/variants/baseboard/gpio.c b/src/mainboard/google/octopus/variants/baseboard/gpio.c index 82f4ee1..b3145d1 100644 --- a/src/mainboard/google/octopus/variants/baseboard/gpio.c +++ b/src/mainboard/google/octopus/variants/baseboard/gpio.c @@ -331,6 +331,19 @@ */ PAD_CFG_NF_IOSSTATE_IOSTERM(GPIO_151, UP_20K, DEEP, NF2, HIZCRx1, ENPU), /* ESPI_IO1 */ + + /* GPIO_67 and GPIO_117 are in early_gpio_table and gpio_table. For variants + * having LTE SKUs, these two GPIOs would be overridden to output high first + * in the bootblock then be set to default state in gpio_table for non-LTE + * SKUs and keep to output high for LTE SKUs in ramstage. + */ + PAD_CFG_GPO_IOSSTATE_IOSTERM(GPIO_67, 0, DEEP, NONE, TxLASTRxE, DISPUPD), /* UART2-CTS_B -- EN_PP3300_DX_LTE_SOC */ + PAD_CFG_GPI_SCI_LOW(GPIO_117, NONE, DEEP, EDGE_SINGLE),/* PCIE_WAKE1_B -- LTE_WAKE_L */ + /* GPIO_161 is in early_gpio_table and gpio_table because LTE SKU needs + * to override this pin to output low then high respectively in two + * stages. + */ + PAD_CFG_GPO_IOSSTATE_IOSTERM(GPIO_161, 1, DEEP, UP_20K, Tx1RxDCRx0, DISPUPD), /* AVS_I2S1_MCLK -- LTE_OFF_ODL */ };
const struct pad_config *__weak diff --git a/src/mainboard/google/octopus/variants/garg/gpio.c b/src/mainboard/google/octopus/variants/garg/gpio.c index 90601ce..36fde90 100644 --- a/src/mainboard/google/octopus/variants/garg/gpio.c +++ b/src/mainboard/google/octopus/variants/garg/gpio.c @@ -50,6 +50,28 @@ DISPUPD),
PAD_NC(GPIO_213, DN_20K), + +}; + +static const struct pad_config lte_override_table[] = { + /* Default override table. */ + PAD_NC(GPIO_104, UP_20K), + + /* EN_PP3300_TOUCHSCREEN */ + PAD_CFG_GPO_IOSSTATE_IOSTERM(GPIO_146, 0, DEEP, NONE, Tx0RxDCRx0, + DISPUPD), + + PAD_NC(GPIO_213, DN_20K), + + /* Be specific to LTE SKU */ + /* UART2-CTS_B -- EN_PP3300_DX_LTE_SOC */ + PAD_CFG_GPO(GPIO_67, 1, PWROK), + + /* PCIE_WAKE1_B -- FULL_CARD_POWER_OFF */ + PAD_CFG_GPO(GPIO_117, 1, PWROK), + + /* AVS_I2S1_MCLK -- PLT_RST_LTE_L */ + PAD_CFG_GPO(GPIO_161, 1, DEEP), };
const struct pad_config *variant_override_gpio_table(size_t *num) @@ -61,8 +83,29 @@ case SKU_9_HDMI: *num = ARRAY_SIZE(hdmi_override_table); return hdmi_override_table; + case SKU_17_LTE: + *num = ARRAY_SIZE(lte_override_table); + return lte_override_table; default: *num = ARRAY_SIZE(default_override_table); return default_override_table; } } + +static const struct pad_config lte_early_override_table[] = { + /* UART2-CTS_B -- EN_PP3300_DX_LTE_SOC */ + PAD_CFG_GPO(GPIO_67, 1, PWROK), + + /* PCIE_WAKE1_B -- FULL_CARD_POWER_OFF */ + PAD_CFG_GPO(GPIO_117, 1, PWROK), + + /* AVS_I2S1_MCLK -- PLT_RST_LTE_L */ + PAD_CFG_GPO(GPIO_161, 0, DEEP), +}; + +const struct pad_config *variant_early_override_gpio_table(size_t *num) +{ + *num = ARRAY_SIZE(lte_early_override_table); + + return lte_early_override_table; +} diff --git a/src/mainboard/google/octopus/variants/garg/variant.c b/src/mainboard/google/octopus/variants/garg/variant.c index a3efe3f..2754e64 100644 --- a/src/mainboard/google/octopus/variants/garg/variant.c +++ b/src/mainboard/google/octopus/variants/garg/variant.c @@ -13,10 +13,13 @@ * GNU General Public License for more details. */
+#include <arch/acpi.h> #include <boardid.h> #include <ec/google/chromeec/ec.h> #include <drivers/intel/gma/opregion.h> #include <baseboard/variants.h> +#include <delay.h> +#include <gpio.h>
enum { SKU_1_2A2C = 1, @@ -24,6 +27,34 @@ SKU_17_LTE = 17, };
+struct gpio_with_delay { + gpio_t gpio; + unsigned int delay_msecs; +}; + +static void power_off_lte_module(u8 slp_typ) +{ + const struct gpio_with_delay lte_power_off_gpios[] = { + { + GPIO_161, /* AVS_I2S1_MCLK -- PLT_RST_LTE_L */ + 30, + }, + { + GPIO_117, /* PCIE_WAKE1_B -- FULL_CARD_POWER_OFF */ + 100 + }, + { + GPIO_67, /* UART2-CTS_B -- EN_PP3300_DX_LTE_SOC */ + 0 + } + }; + + for (int i = 0; i < ARRAY_SIZE(lte_power_off_gpios); i++) { + gpio_output(lte_power_off_gpios[i].gpio, 0); + mdelay(lte_power_off_gpios[i].delay_msecs); + } +} + const char *mainboard_vbt_filename(void) { uint32_t sku_id; @@ -37,3 +68,19 @@ return "vbt.bin"; } } + +void variant_smi_sleep(u8 slp_typ) +{ + /* Currently use cases here all target to S5 therefore we do early return + * here for saving one transaction to the EC for getting SKU ID. */ + if (slp_typ != ACPI_S5) + return; + + switch (get_board_sku()) { + case SKU_17_LTE: + power_off_lte_module(slp_typ); + return; + default: + return; + } +}
-
Angel Pons (Code Review) -
build bot (Jenkins) (Code Review)
-
Furquan Shaikh (Code Review)
-
Karthik Ramasubramanian (Code Review)
-
Kevin Chiu (Code Review)
-
Marco Chen (Code Review)
-
Martin Roth (Code Review)