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?