Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45571 )
Change subject: soc/intel/alderlake: Add GPIOs for Alder Lake SOC ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45571/2/src/soc/intel/alderlake/inc... File src/soc/intel/alderlake/include/soc/gpio_soc_defs.h:
https://review.coreboot.org/c/coreboot/+/45571/2/src/soc/intel/alderlake/inc... PS2, Line 309:
Angel, please check this https://github.com/coreboot/coreboot/blob/master/src/soc/intel/tigerlake/rom...
This looks like good progress, thanks for sharing. However as usual, FSP UPDs are not acurately documented so we have to guess what it does specifically. There's a hint in the header file it says:
2 - skips GpioSetNativePadByFunction and GpioSetPadMode
But we set it to 1. That at least implies that FSP doesn't skip everything in the current configuration. I wonder if this was another accident, as the commit that added it seemed to assume that is does skip everything.
I'm had refer about SoC default GPIO :) not sure if i have done in mistake conveying the same where FSP also don't touch those GPIOs. ideally no one program those PADs, one could argue, what happen if i want to touch from coreboot, its okay to do i believe as long as its part of EDS, which is not the case, so if like to do randomly knowing there is GPIO COMM 3 and let configure those to NF without knowing the PIN number and description is only my concern.
I completely agree to these concerns. It is always a PITA when one tries to review a change and the necessarry information is missing in the EDS. So please, please, please share your concerns with the people who decide what goes into the EDS and what doesn't. It often seems that it only contains information for OS developers and 80% of the information that would be only for "BIOS" developers is missing. Hiding information from the EDS slows development significantly down.