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?