Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45654 )
Change subject: soc/intel: Make GPIO ASL helper function unified across SoC ......................................................................
Patch Set 1: Code-Review+1
(5 comments)
The changes look good, but they could be split up to ease review and verification through reproducible builds.
I should probably write a tutorial about reproducible builds. For now, this long comment will have to do:
=== What are reproducible builds? ===
Reproducible builds are coreboot images stripped of things like version, timestamps... This allows binaries to be easily compared with e.g. `diff`. To build reproducible binaries, one can run `make BUILD_TIMELESS=1 -j$(nproc)`. Abuild also has a `--timeless` option to build reproducible images.
=== Why are reproducible builds awesome? ===
Reproducible builds provide a mechanism to verify commits that do not intend to change the behavior of the code. This is extremely useful to verify code rewrites, factoring out common code and cosmetic cleanups. When a commit does not change the resulting coreboot image, boot-testing it isn't necessary: if the board was working before the commit, it will work after the commit.
I usually try to change a small thing and check if the coreboot image has changed. A one-liner I use is `make BUILD_TIMELESS=1 -j$(nproc) && diff -s -q before.rom build/coreboot.rom`. Before changing anything, I build a timeless coreboot image and rename it to `before.rom`. Checking every few changes makes it easy to spot where reproducibility broke.
=== Which kinds of changes can be verified with reproducible builds? ===
For example, cleaning up cosmetics, moving things between headers, renaming macros, replacing bare register offsets with macros, rewriting gpio arrays with macros, replacing raw Azalia verbs with macros... Folding mainboards into variants and conversions to overridetrees should also be reproducible, but I have little experience with them.
One thing to consider is that, by default, the .config file is included in coreboot images. This is controllable by Kconfig. I generally disable this, especially when the changes I want to test are expected to result in different config files. For example, when folding multiple mainboards together using the variants mechanism, it is possible to obtain an identical binary even when the config has changed.
Converting files to ASL 2.0 syntax is usually reproducible, but there can be some edge cases where IASL realizes it can optimize something differently.
There can be corner cases where something isn't reproducible for some reason. I isolate the problematic cases and handle them in a separate commit, either before or after the reproducible commit.
Moving function definitions between compilation units is usually not reproducible. If I have to factor out such cases, I usually resort to temporarily including a .c file from other .c files, then fix it in a follow-up. The first commit has a big diffstat but is reproducible, whereas the second one changes the binary but is tiny.
I've refactored Sandy Bridge native raminit several times with the help of timeless builds. I think the craziest (ab)use of timeless builds I've done is when I introduced an API to program sequences of DRAM commands into the IOSAV registers.
Here's the chronologically-sorted list of commits: CB:40972 CB:40973 CB:40976 CB:40971 CB:40977 CB:40978 CB:40982 CB:40983 CB:40981 CB:40980 CB:40984 CB:41003
https://review.coreboot.org/c/coreboot/+/45654/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45654/1//COMMIT_MSG@9 PS1, Line 9: List When you have a list of changes in a commit message, you should be suspicious: Shouldn't this be a list of commits instead, each doing one thing? đ
https://review.coreboot.org/c/coreboot/+/45654/1/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/gpio_op.asl:
PS1: I think the conversion to ASL 2.0 syntax can be done in its own commit. It should result in a reproducible build (using BUILD_TIMELESS=1), too.
And please, one commit per platform. It may seem like a waste of commits, but commits are fungible material (like soldering tin or thermal paste). Plus, I (and possibly others) can +2 small, localized commits much more easily than large commits touching multiple platforms. đ
https://review.coreboot.org/c/coreboot/+/45654/1/src/soc/intel/cannonlake/in... File src/soc/intel/cannonlake/include/soc/gpio_common.h:
PS1: Dropping this in favor of common definitions is something best done in its own commit. It should even be a reproducible change.
https://review.coreboot.org/c/coreboot/+/45654/1/src/soc/intel/jasperlake/ac... File src/soc/intel/jasperlake/acpi/gpio_op.asl:
PS1: Same here, the conversion to ASL 2.0 syntax would be best done in a separate commit.
https://review.coreboot.org/c/coreboot/+/45654/1/src/soc/intel/skylake/acpi/... File src/soc/intel/skylake/acpi/gpio.asl:
https://review.coreboot.org/c/coreboot/+/45654/1/src/soc/intel/skylake/acpi/... PS1, Line 120: Local0 = PAD_CFG0_RX_STATE & (VAL0 >> PAD_CFG0_RX_STATE_BIT) Same here, the conversion to ASL 2.0 syntax would be best done in a separate commit.