Subrata Banik 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:
(12 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.
Ack
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. […]
Furquan, was thinking if we can make ASL2.0 syntax conversion for entire file at one shot after this common CL goes in. what is your thought ?
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. […]
same as above ?
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?
Ack
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. […]
Ack
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. […]
Ack
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. […]
Understood and tried the same implementation but i'm expected to see an error like this on CML-Hatch
coreboot toolchain vc3c9afb 2019-02-03 Compiler aborting due to parser-detected syntax error(s) dsdt.asl 140: , (15 - 1), Error 6126 - ^ syntax error, unexpected PARSEOP_OPEN_PAREN, expecting PARSEOP_INTEGER
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. […]
Ack
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.
coreboot toolchain vc3c9afb 2019-02-03 Compiler aborting due to parser-detected syntax error(s) dsdt.asl 144: , (26 - 2 - 1), Error 6126 - ^ syntax error, unexpected PARSEOP_OPEN_PAREN, expecting PARSEOP_INTEGER
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 differ […]
Ack
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. […]
Ack
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?
Ack