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.