Subrata Banik 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. […]
send you register details
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/common/block/... PS5, Line 43: PXSZ, 2,
Is this correct for TGL?
send you register details
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.
all PCIE BAR is 28:38 except APL/GLK
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?
this is correct as per my observation
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?
i don't understand this one Furquan. PCIE BAR is Bit 28 to 38 hence shifting by 28.
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?
this would be correct for TGL along as ONFIG_SA_PCIEX_LENGTH=0x10000000 in Kconfig
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/common/block/... PS5, Line 268: 0x08000
Is this correct for TGL?
this is mistake looks like from myside, this value should be 0x10000
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?
Done
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.
got it
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: […]
good idea
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.
Ack
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
its 28-38 as per register details
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/skylake/acpi/... PS5, Line 247: 28
Same comment as the common northbridge. […]
i have asked some question there