Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34337 )
Change subject: soc/intel/common: add advanced macros for gpio config ......................................................................
Patch Set 2:
(6 comments)
https://review.coreboot.org/c/coreboot/+/34337/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34337/2//COMMIT_MSG@19 PS2, Line 19: trig
Thanks for the review. […]
Ah okay. That makes sense for NF. What about GPO? Is a trig type set for GPO as well anytime?
https://review.coreboot.org/c/coreboot/+/34337/2/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/gpio_defs.h:
https://review.coreboot.org/c/coreboot/+/34337/2/src/soc/intel/common/block/... PS2, Line 136: You can move your macros up here:
#define PAD_CFG0_BUF_TX_DISABLE PAD_CFG0_TX_DISABLE #define PAD_CFG0_BUF_RX_DISABLE PAD_CFG0_RX_DISABLE #define PAD_CFG0_BUF_TX_RX_DISABLE (PAD_CFG0_TX_DISABLE | PAD_CFG0_RX_DISABLE) #define PAD_CFG0_BUF_NO_DISABLE (0)
#define PAD_BUF(value) PAD_CFG0_BUF_##value
https://review.coreboot.org/c/coreboot/+/34337/2/src/soc/intel/common/block/... PS2, Line 182: And then add your NF macro here:
#define PAD_CFG_NF_BUF_TRIG(pad, pull, rst, bufdis, trig, func)
I know its kind of a bigger name, but just trying to make its intent clear.
https://review.coreboot.org/c/coreboot/+/34337/2/src/soc/intel/common/block/... PS2, Line 421: trig
In only one case 0h = Level is used: […]
I think it makes sense to have a macro for this case.
https://review.coreboot.org/c/coreboot/+/34337/2/src/soc/intel/common/block/... PS2, Line 427: trig
I don’t sure about trig for a GPO. […]
In my opinion, we should just update PAD_CFG_GPO to set trig to DISABLE. Since Rx Trig should not matter for a GPO.
https://review.coreboot.org/c/coreboot/+/34337/2/src/soc/intel/common/block/... PS2, Line 427: pull
I repeated the inteltool dump: […]
I honestly think that this is incorrect. If the pad is being actively driven why is an internal 20K_PD required?