Lance Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38387 )
Change subject: soc/intel/{apl,cnl,icl,skl,tgl}: Update SA bit fields as per EDS ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/38387/2/src/soc/intel/cannonlake/in... File src/soc/intel/cannonlake/include/soc/systemagent.h:
https://review.coreboot.org/c/coreboot/+/38387/2/src/soc/intel/cannonlake/in... PS2, Line 56: true Using something like sa_mmio_descriptor_64 to move the "true" into header file?
https://review.coreboot.org/c/coreboot/+/38387/2/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent_early.c:
https://review.coreboot.org/c/coreboot/+/38387/2/src/soc/intel/common/block/... PS2, Line 94: if (is64bit) Without that condition? Writing zero shall be no harm?
https://review.coreboot.org/c/coreboot/+/38387/2/src/soc/intel/common/block/... PS2, Line 80: /* Check if PCI BAR already enabled */ : if (is64bit) { : base = pci_read_config32(SA_DEV_ROOT, index + 4); : base <<= 32; : } : base |= pci_read_config32(SA_DEV_ROOT, index); : : /* If enabled don't program it. */ : if (base & 0x1) : return; : : base = fixed_set_resources[i].base; : : pci_write_config32(SA_DEV_ROOT, index, base | 1); : if (is64bit) : pci_write_config32(SA_DEV_ROOT, index + 4, base >> 32); : } My personal opinion will have two separated body, one for 32 bit and another for 64 bit will be more easier for reading. But that's just my 2 cents.