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