Furquan Shaikh 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 5:
(13 comments)
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/common/block/... File src/soc/intel/common/block/acpi/acpi/northbridge.asl:
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/common/block/... PS5, Line 39: 38:16 Shouldn't this be 38:15? At least that is what I see in KBL and CML EDS.
BTW, is this really correct for TGL? I see a different offset there. Can you please confirm?
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/common/block/... PS5, Line 43: PXSZ, 2, Is this correct for TGL?
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/common/block/... PS5, Line 45: 38:28 Actually, this is 38:26 on all. Sorry, I had read this incorrectly before.
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/common/block/... PS5, Line 223: 16 Can you please confirm if this is correct for TGL?
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/common/block/... PS5, Line 237: 28 Why does this not take PXSZ into consideration? Wouldn't the shift change because of that?
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/common/block/... PS5, Line 244: ShiftRight (0x10000000, _SB.PCI0.MCHC.PXSZ, Local0) Is this correct fot TGL?
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/common/block/... PS5, Line 268: 0x08000 Is this correct for TGL?
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/systemagent.h:
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/common/block/... PS5, Line 63: uintptr_t Shouldn't this be uint64_t then?
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent_early.c:
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/common/block/... PS5, Line 81: if (is64bit) { : base = pci_read_config32(SA_DEV_ROOT, index + 4); : base <<= 32; : } Why is this required? Check below only cares about whether the base is enabled which is bit 0.
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/common/block/... PS5, Line 93: pci_write_config32(SA_DEV_ROOT, index, base | 1); : if (is64bit) : pci_write_config32(SA_DEV_ROOT, index + 4, base >> 32); Do we really need the flag is64bit? Can this be done something like:
pci_write_config32(SA_DEV_ROOT, index, (base & 0xffffffff) | 1); if (base >> 32) pci_write_config32(SA_DEV_ROOT, index + 4, base >> 32);
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/common/block/... PS5, Line 115: write32((void *)(MCH_BASE_ADDRESS + index), base | 1); : if (fixed_set_resources[i].is_64_bit) : write32((void *)(MCH_BASE_ADDRESS + index + 4), base >> 32); Same comment as above.
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/skylake/acpi/... File src/soc/intel/skylake/acpi/systemagent.asl:
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/skylake/acpi/... PS5, Line 44: 38:28 38:26
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/skylake/acpi/... PS5, Line 247: 28 Same comment as the common northbridge.asl file