Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38456 )
Change subject: soc/intel/common/systemagent: Make common northbridge.asl ......................................................................
Patch Set 8: Code-Review+1
(5 comments)
It looks much better now, thanks!
https://review.coreboot.org/c/coreboot/+/38456/8/src/soc/intel/common/block/... File src/soc/intel/common/block/acpi/acpi/northbridge.asl:
https://review.coreboot.org/c/coreboot/+/38456/8/src/soc/intel/common/block/... PS8, Line 284: _BAS Both _BAS and _LEN get updated now, so this comment should be updated.
Also, I really like this approach. Even though I know very little ASL, it's very easy to understand :D
https://review.coreboot.org/c/coreboot/+/38456/8/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/Kconfig:
https://review.coreboot.org/c/coreboot/+/38456/8/src/soc/intel/common/block/... PS8, Line 12: default 14 I understand that, as most platforms use the same value, one would want to make it the default setting. But what happens if, when adding a new platform, somebody forgets to specify the correct value? Everything builds, but the used value can be incorrect.
IMHO, it would be a good idea to put invalid defaults here, so that each SoC has to define the correct value. That way, the build process can be aborted if an invalid configuration is detected. For example, zero: it is not a valid value, and is a common "default" value.
To implement this check, I would add to `soc/intel/common/block/systemagent/Makefile.inc`:
ifeq ($(CONFIG_SA_MCHBAR_GRANULARITY),0) $(error SA_MCHBAR_GRANULARITY value is invalid. SoC must provide this) endif
(this also applies to the other fields below)
https://review.coreboot.org/c/coreboot/+/38456/8/src/soc/intel/common/block/... PS8, Line 14: reserve reserve*d*
https://review.coreboot.org/c/coreboot/+/38456/8/src/soc/intel/common/block/... PS8, Line 32: reserve reserve*d*
https://review.coreboot.org/c/coreboot/+/38456/4/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent_early.c:
https://review.coreboot.org/c/coreboot/+/38456/4/src/soc/intel/common/block/... PS4, Line 42: switch (CONFIG_SA_PCIEX_LENGTH) {
Ack
Sorry if my previous message was confusing. I meant to say that, since all the values here are known at compile-time, the switch() statement could be avoided. In any case, this belongs to a separate change.