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:
(9 comments)
https://review.coreboot.org/c/coreboot/+/38456/14/src/soc/intel/common/block... File src/soc/intel/common/block/acpi/acpi/northbridge.asl:
https://review.coreboot.org/c/coreboot/+/38456/14/src/soc/intel/common/block... PS14, Line 200: #if !CONFIG(TPM_CR50) Commit message does not talk about this change.
https://review.coreboot.org/c/coreboot/+/38456/14/src/soc/intel/common/block... PS14, Line 241: ShiftLeft (1, Add (CONFIG_SA_MCHBAR_GRANULARITY, 1), Local0) Just use ASL 2.0 syntax?
https://review.coreboot.org/c/coreboot/+/38456/14/src/soc/intel/common/block... PS14, Line 248: ShiftLeft (_SB.PCI0.MCHC.MHBR, Add (CONFIG_SA_MCHBAR_GRANULARITY, 1), Local0) Use ASL 2.0 syntax?
https://review.coreboot.org/c/coreboot/+/38456/14/src/soc/intel/common/block... PS14, Line 269: If (LLess (_SB.PCI0.MCHC.PXSZ, 3)) { : ShiftRight (0x10000000, _SB.PCI0.MCHC.PXSZ, Local0) : } Else { : Store(0x10000000, Local0) : } Shouldn't this just return CONFIG_SA_PCIEX_LENGTH?
https://review.coreboot.org/c/coreboot/+/38456/2/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/Kconfig:
https://review.coreboot.org/c/coreboot/+/38456/2/src/soc/intel/common/block/... PS2, Line 6: SOC_INTEL_COMMON_BLOCK_SA_VERSION_2 To be very honest, I think it would be best if we can avoid this. It becomes really tricky managing versions for these IPs especially when they are not really tied to visible hardware version.
https://review.coreboot.org/c/coreboot/+/38456/2/src/soc/intel/common/block/... PS2, Line 18: SA_PCIEX_LENGTH Can we change this to SA_PCIEX_LENGTH_MIB and then use the values in MiB for different lengths.Then you won't need the cast in systemagent_early.c
https://review.coreboot.org/c/coreboot/+/38456/14/src/soc/intel/common/block... File src/soc/intel/common/block/systemagent/Kconfig:
https://review.coreboot.org/c/coreboot/+/38456/14/src/soc/intel/common/block... PS14, Line 14: This field corresponds to the reserved bits (bit 1 onwards) of MCHBAR register. Sorry, but I am really confused by this explanation. Does this mean number/count of reserved bits in MCHBAR register?
Wouldn't it be easier to have SA_MCHBAR_BIT_OFFSET indicating the bit offset where the MCHBAR field is supposed to start within the register?
Looking at northbridge.asl, I see that you are using this field to indicate number of reserved bits before you can get to MCHBAR. With SA_MCHBAR_BIT_OFFSET, I think you can still achieve the same by using SA_MCHBAR_BIT_OFFSET - 1. It just makes it easier to compare the Kconfigs with the EDS register.
https://review.coreboot.org/c/coreboot/+/38456/14/src/soc/intel/common/block... PS14, Line 28: This field corresponds to the length of PCIEXBAR region. This is not completely correct. This Kconfig basically indicates the "*width of the* field that represents the length of PCIEXBAR region". Length of PCIEXBAR region is provided by SA_PCIEX_LENGTH.
https://review.coreboot.org/c/coreboot/+/38456/14/src/soc/intel/common/block... PS14, Line 35: This field corresponds to the reserved bits (after pciex length) of PCIEXBAR register. Same comment as above about using BIT_OFFSET instead of GRANULARITY.