Change in coreboot[master]: soc/intel/gpio: set default value for RXEVCFG in native mode
Maxim Polyakov has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41030 )
Change subject: soc/intel/gpio: set default value for RXEVCFG in native mode ......................................................................
soc/intel/gpio: set default value for RXEVCFG in native mode
According to the documentation (Intel document #549921), RX Level/Edge Configuration (RXEVCFG) settings are not applicable in the native mode and BIOS does not need to configure them. Therefore, there is no need to change the default value for this field in the PAD_CFG_DW0 register. Set the RX Level/Edge to disable (2h = Drive '0') for all PAD_CFG_NF* macros.
Change-Id: Ia667ae4bac4198fe269091050bf067c0a488289a Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M src/soc/intel/common/block/include/intelblocks/gpio_defs.h 1 file changed, 22 insertions(+), 16 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/41030/1
diff --git a/src/soc/intel/common/block/include/intelblocks/gpio_defs.h b/src/soc/intel/common/block/include/intelblocks/gpio_defs.h index 6eaaf0c..4bb318b 100644 --- a/src/soc/intel/common/block/include/intelblocks/gpio_defs.h +++ b/src/soc/intel/common/block/include/intelblocks/gpio_defs.h @@ -190,9 +190,10 @@ #endif
/* Native function configuration */ -#define PAD_CFG_NF(pad, pull, rst, func) \ - _PAD_CFG_STRUCT(pad, PAD_RESET(rst) | PAD_FUNC(func), PAD_PULL(pull) | \ - PAD_IOSSTATE(TxLASTRxE)) +#define PAD_CFG_NF(pad, pull, rst, func) \ + _PAD_CFG_STRUCT(pad, \ + PAD_RESET(rst) | PAD_CFG0_TRIG_OFF | PAD_FUNC(func), \ + PAD_PULL(pull) | PAD_IOSSTATE(TxLASTRxE))
/* * Set native function with RX Level/Edge configuration and disable @@ -207,31 +208,36 @@ /* Native 1.8V tolerant pad, only applies to some pads like I2C/I2S Not applicable to all SOCs. Refer EDS */ -#define PAD_CFG_NF_1V8(pad, pull, rst, func) \ - _PAD_CFG_STRUCT(pad, PAD_RESET(rst) | PAD_FUNC(func), PAD_PULL(pull) |\ - PAD_IOSSTATE(TxLASTRxE) | PAD_CFG1_TOL_1V8) +#define PAD_CFG_NF_1V8(pad, pull, rst, func) \ + _PAD_CFG_STRUCT(pad, \ + PAD_RESET(rst) | PAD_CFG0_TRIG_OFF | PAD_FUNC(func), \ + PAD_PULL(pull) | PAD_IOSSTATE(TxLASTRxE) | PAD_CFG1_TOL_1V8) #endif
/* Native function configuration for standby state */ -#define PAD_CFG_NF_IOSSTATE(pad, pull, rst, func, iosstate) \ - _PAD_CFG_STRUCT(pad, PAD_RESET(rst) | PAD_FUNC(func), PAD_PULL(pull) | \ - PAD_IOSSTATE(iosstate)) +#define PAD_CFG_NF_IOSSTATE(pad, pull, rst, func, iosstate) \ + _PAD_CFG_STRUCT(pad, \ + PAD_RESET(rst) | PAD_CFG0_TRIG_OFF | PAD_FUNC(func), \ + PAD_PULL(pull) | PAD_IOSSTATE(iosstate))
/* Native function configuration for standby state, also configuring iostandby as masked */ -#define PAD_CFG_NF_IOSTANDBY_IGNORE(pad, pull, rst, func) \ - _PAD_CFG_STRUCT(pad, PAD_RESET(rst) | PAD_FUNC(func), PAD_PULL(pull) | \ - PAD_IOSSTATE(IGNORE)) +#define PAD_CFG_NF_IOSTANDBY_IGNORE(pad, pull, rst, func) \ + _PAD_CFG_STRUCT(pad, \ + PAD_RESET(rst) | PAD_CFG0_TRIG_OFF | PAD_FUNC(func), \ + PAD_PULL(pull) | PAD_IOSSTATE(IGNORE))
/* Native function configuration for standby state, also configuring iosstate and iosterm */ -#define PAD_CFG_NF_IOSSTATE_IOSTERM(pad, pull, rst, func, iosstate, iosterm) \ - _PAD_CFG_STRUCT(pad, PAD_RESET(rst) | PAD_FUNC(func), PAD_PULL(pull) | \ - PAD_IOSSTATE(iosstate) | PAD_IOSTERM(iosterm)) +#define PAD_CFG_NF_IOSSTATE_IOSTERM(pad, pull, rst, func, iosstate, iosterm) \ + _PAD_CFG_STRUCT(pad, \ + PAD_RESET(rst) | PAD_CFG0_TRIG_OFF | PAD_FUNC(func), \ + PAD_PULL(pull) | PAD_IOSSTATE(iosstate) | PAD_IOSTERM(iosterm))
/* Configure native function, iosstate, iosterm and disable input/output buffer */ #define PAD_CFG_NF_BUF_IOSSTATE_IOSTERM(pad, pull, rst, func, bufdis, iosstate, iosterm) \ - _PAD_CFG_STRUCT(pad, PAD_RESET(rst) | PAD_BUF(bufdis) | PAD_FUNC(func), \ + _PAD_CFG_STRUCT(pad, \ + PAD_RESET(rst) | PAD_CFG0_TRIG_OFF | PAD_BUF(bufdis) | PAD_FUNC(func), \ PAD_PULL(pull) | PAD_IOSSTATE(iosstate) | PAD_IOSTERM(iosterm))
/* General purpose output, no pullup/down. */
Paul Menzel 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: Code-Review+1
Michael Niewöhner 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:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41030/1/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/gpio_defs.h:
https://review.coreboot.org/c/coreboot/+/41030/1/src/soc/intel/common/block/... PS1, Line 202: #define PAD_CFG_NF_BUF_TRIG(pad, pull, rst, func, bufdis, trig) \ bufdis, trig are not applicable to native function, too. So the whole macro can be dropped, if I am right
(I sent you an email itr)
Michael Niewöhner 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:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41030/1/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/gpio_defs.h:
https://review.coreboot.org/c/coreboot/+/41030/1/src/soc/intel/common/block/... PS1, Line 202: #define PAD_CFG_NF_BUF_TRIG(pad, pull, rst, func, bufdis, trig) \
bufdis, trig are not applicable to native function, too. […]
""" I have been working an a lenovo board port, where dumped some GPIO values that actually match the macro PAD_CFG_NF_BUF_TRIG(..., NONE, DEEP, NF1, TX_RX_DISABLE, OFF).
I was talking to Angel Pons and Nico Huber on IRC since the datasheets tell nothing about the direction setting in case of native functions. We finally found a comment in 3rdparty/fsp/KabylakeFspBinPkg/Include/GpioConfig.h, stating that multiple parameters do not apply to native function pads.
Do you have any contradictory information on that? If not, we should drop that macro again and when converting from DW values to macros simply set leave non- applicable ones 0.
What do you think? You can talk to me on IRC (c0d3z3r0) or just reply by mail. Looking forward to hear from you! """
Michael Niewöhner 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:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41030/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41030/1//COMMIT_MSG@9 PS1, Line 9: RX Level/Edge : Configuration (RXEVCFG) settings are not applicable in the native mode and interrupt (GPIROUT*), direction (GPIOTXDIS, GPIORXDIS), tx state (GPIOTXSTATE), host sw ownership (GPIO_HOSTSW_OWN), output state lock (?)
Lance Zhao 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:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41030/1/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/gpio_defs.h:
https://review.coreboot.org/c/coreboot/+/41030/1/src/soc/intel/common/block/... PS1, Line 202: #define PAD_CFG_NF_BUF_TRIG(pad, pull, rst, func, bufdis, trig) \
""" […]
Though I will prefer to skip programming and leave those two register fields as default. But looks like "2" is the default anyway for trig, so that shall be fine. For native mode, the inputs can still get manipulated by Rx registers, so far I have never see any native mode pin will need to touch trig register yet.
Michael Niewöhner 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:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41030/1/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/gpio_defs.h:
https://review.coreboot.org/c/coreboot/+/41030/1/src/soc/intel/common/block/... PS1, Line 202: #define PAD_CFG_NF_BUF_TRIG(pad, pull, rst, func, bufdis, trig) \
For native mode, the inputs can still get manipulated by Rx registers
What do you mean here exactly?
Lance Zhao 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:
Patch Set 1:
(1 comment)
Even a GPIO in native mode, some RX configuration bits can still take effect. But that will not affect this commit changes.
Aaron Durbin 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:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41030/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41030/1//COMMIT_MSG@9 PS1, Line 9: (Intel document #549921 what chipset is this? The code you are updating is being used on many chipsets. Does it really matter if we're writing a value and then new values are programmed once native mode is enabled?
I have had experiences where some native mode functions didn't manipulate the pad registers while others have. It's a mixed bag.
Lance Zhao 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:
Patch Set 1:
(1 comment)
That's skylake/kabylake PCH BIOS write guide. What I try to say is, GPIO as native mode for output, native mode take full control. But GPIO pin as native mode for input, the following register can still take effect. RXRAW,RXTXEnCfg,DebEn,PreGfRXSel,RXINV,RxPadStSel.
So the original of the commit is actual right, bufdis/trig will not be effected by native mode anyway.
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:
Patch Set 1:
Patch Set 1:
(1 comment)
That's skylake/kabylake PCH BIOS write guide. What I try to say is, GPIO as native mode for output, native mode take full control. But GPIO pin as native mode for input, the following register can still take effect. RXRAW,RXTXEnCfg,DebEn,PreGfRXSel,RXINV,RxPadStSel.
So the original of the commit is actual right, bufdis/trig will not be effected by native mode anyway.
I agree with Lance about setting trig for NF. However, I'm not sure about bifdis.
From 3rdparty/fsp/KabylakeFspBinPkg/Include/GpioConfig.h:
"When in native mode setting Direction (except Inversion)..." I think this means that for inverted pads the direction should be set. I'm right?
Jonathan also set bufdis settings (GpioDirInOut/GpioDirIn/GpioDirOut) for NF in the Tioga Pass: https://review.coreboot.org/c/coreboot/+/39453/42/src/mainboard/ocp/tiogapas...
Aaron Durbin 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:
Patch Set 1:
Patch Set 1:
Patch Set 1:
(1 comment)
That's skylake/kabylake PCH BIOS write guide. What I try to say is, GPIO as native mode for output, native mode take full control. But GPIO pin as native mode for input, the following register can still take effect. RXRAW,RXTXEnCfg,DebEn,PreGfRXSel,RXINV,RxPadStSel.
So the original of the commit is actual right, bufdis/trig will not be effected by native mode anyway.
I agree with Lance about setting trig for NF. However, I'm not sure about bifdis.
From 3rdparty/fsp/KabylakeFspBinPkg/Include/GpioConfig.h:
"When in native mode setting Direction (except Inversion)..." I think this means that for inverted pads the direction should be set. I'm right?
Jonathan also set bufdis settings (GpioDirInOut/GpioDirIn/GpioDirOut) for NF in the Tioga Pass: https://review.coreboot.org/c/coreboot/+/39453/42/src/mainboard/ocp/tiogapas...
This is my point. We can't make a sweeping change that would apply to all potential chipsets w/o knowing the detailed behavior on all those chipsets. As I noted I've had specific experience about NF not setting the correct pad configuration as well as others do. It's chipset implementation dependent.
Michael Niewöhner 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:
(1 comment)
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
(1 comment)
That's skylake/kabylake PCH BIOS write guide. What I try to say is, GPIO as native mode for output, native mode take full control. But GPIO pin as native mode for input, the following register can still take effect. RXRAW,RXTXEnCfg,DebEn,PreGfRXSel,RXINV,RxPadStSel.
So the original of the commit is actual right, bufdis/trig will not be effected by native mode anyway.
I agree with Lance about setting trig for NF. However, I'm not sure about bifdis.
From 3rdparty/fsp/KabylakeFspBinPkg/Include/GpioConfig.h:
"When in native mode setting Direction (except Inversion)..." I think this means that for inverted pads the direction should be set. I'm right?
Jonathan also set bufdis settings (GpioDirInOut/GpioDirIn/GpioDirOut) for NF in the Tioga Pass: https://review.coreboot.org/c/coreboot/+/39453/42/src/mainboard/ocp/tiogapas...
This is my point. We can't make a sweeping change that would apply to all potential chipsets w/o knowing the detailed behavior on all those chipsets. As I noted I've had specific experience about NF not setting the correct pad configuration as well as others do. It's chipset implementation dependent.
https://review.coreboot.org/c/coreboot/+/41030/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41030/1//COMMIT_MSG@9 PS1, Line 9: (Intel document #549921
what chipset is this? The code you are updating is being used on many chipsets. […]
Aaron, on what chipset did that happen and which functions didn't do it right?
Aaron Durbin 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:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41030/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41030/1//COMMIT_MSG@9 PS1, Line 9: (Intel document #549921
Aaron, on what chipset did that happen and which functions didn't do it right?
I want to say apollolake, skylake, or glk. I did a brief search and found apollolake had an issue on LPC block: https://review.coreboot.org/c/coreboot/+/16426 That was involving the termination pulls. I recall an EMMC issue as well, however I didn't hvae time to dig that one up.
I'm not sure we want to figure out the whole list of things that are or are not updated when in NF mode because if we write a value that is overwritten by the IP block that implements once we put the pad in NF mode then there's no regression. However, if we have instances of different behavior from experience why would we risk changing current behavior?
Michael Niewöhner 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:
Patch Set 1:
Patch Set 1:
Patch Set 1:
(1 comment)
That's skylake/kabylake PCH BIOS write guide. What I try to say is, GPIO as native mode for output, native mode take full control. But GPIO pin as native mode for input, the following register can still take effect. RXRAW,RXTXEnCfg,DebEn,PreGfRXSel,RXINV,RxPadStSel.
So the original of the commit is actual right, bufdis/trig will not be effected by native mode anyway.
I agree with Lance about setting trig for NF. However, I'm not sure about bifdis.
From 3rdparty/fsp/KabylakeFspBinPkg/Include/GpioConfig.h:
"When in native mode setting Direction (except Inversion)..." I think this means that for inverted pads the direction should be set. I'm right?
Jonathan also set bufdis settings (GpioDirInOut/GpioDirIn/GpioDirOut) for NF in the Tioga Pass: https://review.coreboot.org/c/coreboot/+/39453/42/src/mainboard/ocp/tiogapas...
Copied from your/my mail:
However, I see here - "except Inversion".
I think this means that for inverted pads the direction should be set. I'm right? What do you think about this?
That's simply misleading because in FSP direction and inversion ("GpioDirInInv") gets set via the same parameter, so they mean inversion is valid all other directions settings are not.
Michael Niewöhner 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:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41030/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41030/1//COMMIT_MSG@9 PS1, Line 9: (Intel document #549921
I want to say apollolake, skylake, or glk. […]
ouch... just curious, does the IP block override the value just internally or does the register change than? so do we see what if it gets changed?
Aaron Durbin 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:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41030/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41030/1//COMMIT_MSG@9 PS1, Line 9: (Intel document #549921
ouch... […]
I suspect it's manipulating the register values we can see and not directly driving those settings with another set of registers. No confirmation, but we could empirically try settings and see what happens. However it's an exercise that needs to be done on all chipsets.
Lance Zhao 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:
Patch Set 1:
Patch Set 1:
Patch Set 1:
(1 comment)
That's skylake/kabylake PCH BIOS write guide. What I try to say is, GPIO as native mode for output, native mode take full control. But GPIO pin as native mode for input, the following register can still take effect. RXRAW,RXTXEnCfg,DebEn,PreGfRXSel,RXINV,RxPadStSel.
So the original of the commit is actual right, bufdis/trig will not be effected by native mode anyway.
I agree with Lance about setting trig for NF. However, I'm not sure about bifdis.
From 3rdparty/fsp/KabylakeFspBinPkg/Include/GpioConfig.h:
"When in native mode setting Direction (except Inversion)..." I think this means that for inverted pads the direction should be set. I'm right?
Jonathan also set bufdis settings (GpioDirInOut/GpioDirIn/GpioDirOut) for NF in the Tioga Pass: https://review.coreboot.org/c/coreboot/+/39453/42/src/mainboard/ocp/tiogapas...
GPIO A5 used as LFRAME# which is output, so as long as gpio pin had been used as native mode output, native mode take whole control. So the bufdis there doesn't count.
Nico Huber 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:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41030/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41030/1//COMMIT_MSG@9 PS1, Line 9: (Intel document #549921
I want to say apollolake, skylake, or glk. I did a brief search and found apollolake had an issue on LPC block: https://review.coreboot.org/c/coreboot/+/16426 That was involving the termination pulls. I recall an EMMC issue as well, however I didn't hvae time to dig that one up.
Termination in the case of native functions is badly documented. There's a little word in the register description:
1111: (optional) Native controller selected by Pad Mode controls the Termination
AFAICT, this "optional" means that only some NFs support this. And I can't find more documentation which these are.
Apart from this, everything seems to be documented. Most descriptions of register fields mention if they are meaningful in NF or GPIO mode. And I see no clue that any fields would be automatically changed by the hardware, only that the hardware will ignore certain fields in one mode or another.
Anyway, I don't like this change either. We shouldn't add complexity for cosmetic reasons.
Michael Niewöhner 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:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41030/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41030/1//COMMIT_MSG@9 PS1, Line 9: (Intel document #549921
Anyway, I don't like this change either. We shouldn't add complexity for
cosmetic reasons.
iow we shouldn't set bits that don't do anything?
@Aaron are you aware of any other issues not involving termination but rxevcfg, interrupt, direction, ownership? if not, we really should drop bits being set for them in nf, too
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.
Nico Huber 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:
It seems to me that we discuss 2 questions here:
a) Should we set fields that are not supposed to affect hardware (in a given mode) to their default values or clear them to 0? b) Should we allow mainboard ports to set such fields to arbitrary value?
I'm not sure if I got it right: Do you suggest to favor default values in a) to get rid of b)?
But b) was never necessary.
- 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.
I think the risk to break a lot of boards is secondary. If it does, we can revert things and learned something. However, there is another risk, that we make maintenance harder in the future. That should be avoided.
You assume to know what confuses developers. Personally, I would be much confused if we'd set values to don't-care fields. Also, if we have to decide between the convenience of coreboot-native developers and those that retro fit coreboot, I think we should favor the former (even though, I'm only doing retro-fit ports myself).
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.
What did you test? if the board boots (unrelated) or that expected interrupts still trigger?
- 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.
This is only a single value per platform? There are more. So far I've only spotted read-only registers that don't set RXEVCFG. But are there others? Who's going to review all that.
If you want other people to review such error-prone long lists of default values just for a cosmetic change and your own convenience, please start a fund to pay them.
There is an alternative to reviewing hard-coded defaults: We could mask the don't-care fields in coreboot. But that would increase the complexity in an area that is already hard to handle (with the current macro approach).
- 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.
So far, I was convinced that "Inversion" there refers to the RXINV field which is applicable to NF modes.
- 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.
Um, yes. People have started coreboot to differ from those.
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) :)
PAD_CFG_NF_BUF_TRIG seems dangerous for future maintenance. It adds information to the mainboard ports that is supposed to make no difference for coreboot. But people might doubt this later, and then are afraid to change the code. Please remove it, before it spreads further (some boards already adopted it).
I would like to put an end to the issue with GPIO and do more useful work.
The best way to get it done, IMHO, is by reasoning about each pad and test the result. Copying / generating these configurations just because the list is long creates confusion and weird ideas about cosmetics.
As I have suggested before, you can mask the bits in a dump that aren't supposed to affect the given mode of a pad. Then the settings of coreboot and your reference firmware should match without adding a burden to coreboot.
Michael Niewöhner 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:
Patch Set 1:
It seems to me that we discuss 2 questions here:
a) Should we set fields that are not supposed to affect hardware (in a given mode) to their default values or clear them to 0? b) Should we allow mainboard ports to set such fields to arbitrary value?
I'm not sure if I got it right: Do you suggest to favor default values in a) to get rid of b)?
But b) was never necessary.
- 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.
I think the risk to break a lot of boards is secondary. If it does, we can revert things and learned something. However, there is another risk, that we make maintenance harder in the future. That should be avoided.
You assume to know what confuses developers. Personally, I would be much confused if we'd set values to don't-care fields. Also, if we have to decide between the convenience of coreboot-native developers and those that retro fit coreboot, I think we should favor the former (even though, I'm only doing retro-fit ports myself).
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.
What did you test? if the board boots (unrelated) or that expected interrupts still trigger?
- 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.
This is only a single value per platform? There are more. So far I've only spotted read-only registers that don't set RXEVCFG. But are there others? Who's going to review all that.
If you want other people to review such error-prone long lists of default values just for a cosmetic change and your own convenience, please start a fund to pay them.
There is an alternative to reviewing hard-coded defaults: We could mask the don't-care fields in coreboot. But that would increase the complexity in an area that is already hard to handle (with the current macro approach).
- 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.
So far, I was convinced that "Inversion" there refers to the RXINV field which is applicable to NF modes.
- 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.
Um, yes. People have started coreboot to differ from those.
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) :)
PAD_CFG_NF_BUF_TRIG seems dangerous for future maintenance. It adds information to the mainboard ports that is supposed to make no difference for coreboot. But people might doubt this later, and then are afraid to change the code. Please remove it, before it spreads further (some boards already adopted it).
I would like to put an end to the issue with GPIO and do more useful work.
The best way to get it done, IMHO, is by reasoning about each pad and test the result. Copying / generating these configurations just because the list is long creates confusion and weird ideas about cosmetics.
As I have suggested before, you can mask the bits in a dump that aren't supposed to affect the given mode of a pad. Then the settings of coreboot and your reference firmware should match without adding a burden to coreboot.
So, to summarize that in short: - do not add PAD_CFG0_TRIG_OFF (since we should not care about don't-care fields) - drop PAD_CFG_NF_BUF_TRIG which is useless anyway
@Nico is that right?
Michael Niewöhner 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:
@Maxim that should be done for intelp2m
As I have suggested before, you can mask the bits in a dump that aren't supposed to affect the given mode of a pad. Then the settings of coreboot and your reference firmware should match without adding a burden to coreboot. So, to summarize that in short:
Michael Niewöhner 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: Code-Review-1
-1 see last two comments
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:
Patch Set 1:
Patch Set 1:
It seems to me that we discuss 2 questions here:
a) Should we set fields that are not supposed to affect hardware (in a given mode) to their default values or clear them to 0? b) Should we allow mainboard ports to set such fields to arbitrary value?
I'm not sure if I got it right: Do you suggest to favor default values in a) to get rid of b)?
But b) was never necessary.
- 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.
I think the risk to break a lot of boards is secondary. If it does, we can revert things and learned something. However, there is another risk, that we make maintenance harder in the future. That should be avoided.
You assume to know what confuses developers. Personally, I would be much confused if we'd set values to don't-care fields. Also, if we have to decide between the convenience of coreboot-native developers and those that retro fit coreboot, I think we should favor the former (even though, I'm only doing retro-fit ports myself).
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.
What did you test? if the board boots (unrelated) or that expected interrupts still trigger?
- 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.
This is only a single value per platform? There are more. So far I've only spotted read-only registers that don't set RXEVCFG. But are there others? Who's going to review all that.
If you want other people to review such error-prone long lists of default values just for a cosmetic change and your own convenience, please start a fund to pay them.
There is an alternative to reviewing hard-coded defaults: We could mask the don't-care fields in coreboot. But that would increase the complexity in an area that is already hard to handle (with the current macro approach).
- 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.
So far, I was convinced that "Inversion" there refers to the RXINV field which is applicable to NF modes.
- 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.
Um, yes. People have started coreboot to differ from those.
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) :)
PAD_CFG_NF_BUF_TRIG seems dangerous for future maintenance. It adds information to the mainboard ports that is supposed to make no difference for coreboot. But people might doubt this later, and then are afraid to change the code. Please remove it, before it spreads further (some boards already adopted it).
I would like to put an end to the issue with GPIO and do more useful work.
The best way to get it done, IMHO, is by reasoning about each pad and test the result. Copying / generating these configurations just because the list is long creates confusion and weird ideas about cosmetics.
As I have suggested before, you can mask the bits in a dump that aren't supposed to affect the given mode of a pad. Then the settings of coreboot and your reference firmware should match without adding a burden to coreboot.
So, to summarize that in short:
- do not add PAD_CFG0_TRIG_OFF (since we should not care about don't-care fields)
- drop PAD_CFG_NF_BUF_TRIG which is useless anyway
@Nico is that right?
I'm sorry for not writing to you for a long time
do not add PAD_CFG0_TRIG_OFF (since we should not care about don't-care fields)
yes
drop PAD_CFG_NF_BUF_TRIG which is useless anyway
yes
intelp2m takes this into account. Thanks
Maxim Polyakov has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/41030 )
Change subject: soc/intel/gpio: set default value for RXEVCFG in native mode ......................................................................
Abandoned
I took into account all comments in the work on GPIO macros. These changes are unnecessary. Thanks.
-
Aaron Durbin (Code Review) -
Lance Zhao (Code Review)
-
Maxim Polyakov (Code Review)
-
Michael Niewöhner (Code Review)
-
Nico Huber (Code Review)
-
Paul Menzel (Code Review)