Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41030 )
Change subject: soc/intel/gpio: set default value for RXEVCFG in native mode ......................................................................
Patch Set 1:
Thank you for your comment. So, I would like to summarize and note the main things in our discussion:
1) I understand your reluctance to add this patch and reluctance to make any changes to the macros, as these changes affect a large number of motherboards in the project. However, I would like to clean up these macros so that this doesn’t confuse developers who will deal with GPIO in the future.
If this patch is added, then I will make the appropriate changes in the intelp2m utility (CB:35643 , convert the raw value of the DW0/1 registers to PAD_CFG_* macros) to finally complete this work. Thus, we get a good tool that will allow you to generate GPIO macros (using inteltool) for motherboard configuration. At the moment, intelp2m generates an extended macro PAD_CFG_NF_BUF_TRIG to match the configuration from the inteltool dump (although I now understand that it would be correct to add the PAD_CFG_NF_BUF macro instead).
We found that the trig parameter doesn’t affect the pad in native function mode, and besides, I already tested several motherboards with trig = OFF in NF mode (using the same macro PAD_CFG_NF_BUF_TRIG): - asrock h110m dvs (sky/kaby lake) - kontron mal10 (apollo lake) - tioga pass (skx) - cedar island (cpx)
It works without problems.
Based on the foregoing, I don’t see any special obstacles not to add this patch. If you believe the documentation and my tests, the changes will not have an effect, but they will save me from constant confusion with these macros (sometimes it’s easier for the developer to add raw values of the DW0 and DW1 registers to gpio.h, which affects the readability of the code).
2) In accordance with the documentation:
for Sunrise Point PAD_CFG_DW0 - Default: 4400xx00h (same as for the 300th series);
for Lewisburg PAD_CFG_DW0 - Default: 44000700h;
for Apollo Lake SoC PAD_CFG_DW0 - Default: 44000300h.
So the macro for NF should look like this:
/ * Native function configuration * / #define PAD_CFG_NF(pad, pull, rst, func) \ _PAD_CFG_STRUCT(pad, \ PAD_RESET(rst) | PAD_TRIG(OFF) | PAD_FUNC(func) | PAD_BUF(TX_RX_DISABLE), \ PAD_PULL(pull) | PAD_IOSSTATE(TxLASTRxE))
3) The trig and bufdis parameters do not affect the pad in NF mode. However, in the header files for fsp we see a strange comment: "When in native mode setting Direction (except Inversion) ...". In FSP, the parameters from GPIO_DIRECTION specify bufdis for the pad.
If I recall, at the University they explained us that the inverted pads are pads with an active "0" low state (eg reset signal output for LPC or eSPI bus), which are indicated by a circle on the diagram or contain the suffix “_N” in the name. Let's look at the pinout for the C624 chipset:
----| GPP_A17 ----| GPP_A16_CLKOUT_LPC2 --o| GPP_A15_SUSACK_N (Inversion) --o| GPP_A14_ESPI_RESET_N (Inversion) ----| GPP_A13_SUSWARN_N_SUSPWRDNACK
The bufdis parameter should be used for GPP_A15_SUSACK_N and GPP_A14_ESPI_RESET_N pins.
@Lance, do you agree with me?
4) This will not be a serious argument, but I would like to emphasize that firmware from Intel, as well as from AMI, sets the bufdis parameter for all pads for the NF pad.
I would like to hear a clear verdict of the court - guilty (we add the patch) or not (use PAD_CFG_NF_BUF_TRIG, as before) :)
I would like to put an end to the issue with GPIO and do more useful work.
Also, please pay attention to the patch for GPI https://review.coreboot.org/c/coreboot/+/41031 Thanks again to everyone for the review.