Attention is currently required from: Bora Guvendik, Cliff Huang, Hannah Williams, Jérémy Compostella, Pranava Y N, Wonkyu Kim.
Subrata Banik has posted comments on this change by Cliff Huang. ( https://review.coreboot.org/c/coreboot/+/84901?usp=email )
Change subject: soc/intel/common/gpio: add function to lock GPIO configuration ......................................................................
Patch Set 3:
(1 comment)
File src/soc/intel/common/block/gpio/gpio.c:
https://review.coreboot.org/c/coreboot/+/84901/comment/c6f1cbc8_7b9be86e?usp... : PS3, Line 485: /* Clear lock for the exception PADs */
I think it's implementation way difference. We can lock each pin mux table by using existing _LOCK macro but it may be work if we want to lock some of pin's configuration. Basic assumption for this change(having exception table for lock) is for locking most of pin configuration for system safety. And it'll require less change compared to changing entire pin configuration in gpio.c.
unless you populate a mainboard table the one you called as exception table, I'm unable to understand how this is not any more duplication between using LOCK macro in gpio.c vs adding one more table in mainboard for locking the PAD configuration.
Eventually both should do the same. Also, please don't take the ability for changing the PAD configuration if not required to lock, we are using feature like FW shell or UEFI shell to alter the GPIO configuration to debug some critical issue w/o need to flash the entire AP FW> Therefore, I don't see a need to lock every GPIOs, rather we should only lock the required GPIOs like wake capable and IRQ ones (rests are harmless).