Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29110 )
Change subject: mb/google/octopus: Check for invalid pin termination settings ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/#/c/29110/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/29110/3//COMMIT_MSG@7 PS3, Line 7: mb/google/octopus This is not correct.
https://review.coreboot.org/#/c/29110/3/src/soc/intel/common/block/include/i... File src/soc/intel/common/block/include/intelblocks/gpio_defs.h:
https://review.coreboot.org/#/c/29110/3/src/soc/intel/common/block/include/i... PS3, Line 140: : /* Active (S0) and Standby state terminations should not conflict. When : * a Pull Up is engaged in S0, a Pull down shouldn't be used in Standby. : * Similarly, when a Pull Down is engaged in S0, a Pull Up shouldn't be : * used in Standby. : */ : #define PAD_CFG_CHECK_ENPD(value) \ : !(((((value) & PAD_CFG1_IOSTERM_MASK) == PAD_CFG1_IOSTERM_ENPD) \ : && (((value) & PAD_CFG1_PULL_MASK) > PAD_CFG1_PULL_DN_20K)) || \ : ((((value) & PAD_CFG1_IOSTERM_MASK) == PAD_CFG1_IOSTERM_ENPD) && \ : (((value) & PAD_CFG1_PULL_MASK) == PAD_CFG1_PULL_NONE))) : : #define PAD_CFG_CHECK_ENPU(value) \ : !(((((value) & PAD_CFG1_IOSTERM_MASK) == PAD_CFG1_IOSTERM_ENPU) \ : && (((value) & PAD_CFG1_PULL_MASK) < PAD_CFG1_PULL_UP_1K)) || \ : ((((value) & PAD_CFG1_IOSTERM_MASK) == PAD_CFG1_IOSTERM_ENPU) && \ : (((value) & PAD_CFG1_PULL_MASK) == PAD_CFG1_PULL_NATIVE))) : : #if IS_ENABLED(CONFIG_SOC_INTEL_GLK) : /* For dual-voltage pins, only allow 20K PU or None termination setting */ : #define PAD_CFG_CHECK_VCCIO(pad, value) \ : !(((((pad) >= GPIO_98) && ((pad) <= GPIO_155)) \ : || (((pad) >= GPIO_176) && ((pad) <= GPIO_178))) && \ : ((((value) & PAD_CFG1_PULL_MASK) != PAD_CFG1_PULL_NONE) && \ : (((value) & PAD_CFG1_PULL_MASK) != PAD_CFG1_PULL_UP_20K))) : #else : #define PAD_CFG_CHECK_VCCIO(pad, value) 1 : #endif I think a better way to handle this would be to put all the comparison logic within some gpio*.h file in specific SoC(that needs it) and have a config that says this SoC needs dw1 pad_config valid check and then PAD_CFG_IS_VALID(...) would default to __config1 in this file if that config is not set, otherwise the SoC definition will take effect.
https://review.coreboot.org/#/c/29110/3/src/soc/intel/common/block/include/i... PS3, Line 170: 572688 I don't think this doc applies to all Intel SoCs. Can you instead add meaningful comment indicating what the constraints are? Also, do these constraints apply to all SoCs? If not, then it would be better to just guard this appropriately.
https://review.coreboot.org/#/c/29110/3/src/soc/intel/common/block/include/i... PS3, Line 183: 0xFFFFFFFFFF Can we make use of Static_assert in _PAD_CFG_IS_VALID instead of assigning this value to make the error messages more relevant?