Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38456 )
Change subject: soc/intel/common/systemagent: Make northbridge.asl common ......................................................................
Patch Set 14:
(3 comments)
https://review.coreboot.org/c/coreboot/+/38456/14/src/soc/intel/common/block... File src/soc/intel/common/block/systemagent/systemagent_early.c:
https://review.coreboot.org/c/coreboot/+/38456/14/src/soc/intel/common/block... PS14, Line 42: CONFIG_SA_PCIEX_LENGTH nit: You can just divide this by MiB and then you wouldn't need UL or ULL or typecase for the different cases.
https://review.coreboot.org/c/coreboot/+/38456/14/src/soc/intel/common/block... PS14, Line 43: CONFIG_SA_PCIEXBAR_LENGTH_BIT_WIDTH >= PCIEXBAR_LENGTH_512MB This is a weird check.
Currently, CONFIG_SA_PCIEXBAR_LENGTH_BIT_WIDTH can be 2 or 3 which indicates the number of bits that the field has within the register. That is being compared with PCIEXBAR_LENGTH_512MB which is the value of that field when length is 512MiB. I understand that PCIEXBAR_LENGTH_512MB happens to be 3, but what should be really checked here is that the width of the field is 3 so that it supports length values greater than 256MiB.
https://review.coreboot.org/c/coreboot/+/38456/14/src/soc/intel/common/block... PS14, Line 44: ULL This is unsigned long long and the typecast is for unsigned long?