Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37781 )
Change subject: soc/intel/tigerlake: Update ACPI files ......................................................................
Patch Set 18:
(26 comments)
https://review.coreboot.org/c/coreboot/+/37781/18/src/soc/intel/tigerlake/ac... File src/soc/intel/tigerlake/acpi/pch_clock_ctl.asl:
PS18: I think the file should be named camera_clock_ctl.asl since it is dealing with camera clocks specifically?
https://review.coreboot.org/c/coreboot/+/37781/18/src/soc/intel/tigerlake/ac... PS18, Line 23: PID_ISCLK Don't you need to include some header file to provide the definition of this?
https://review.coreboot.org/c/coreboot/+/37781/18/src/soc/intel/tigerlake/ac... PS18, Line 23: ( space before (
https://review.coreboot.org/c/coreboot/+/37781/18/src/soc/intel/tigerlake/ac... PS18, Line 24: ICLK,AnyAcc,Lock,Preserve) space after ,
https://review.coreboot.org/c/coreboot/+/37781/18/src/soc/intel/tigerlake/ac... PS18, Line 24: ( space before (
https://review.coreboot.org/c/coreboot/+/37781/18/src/soc/intel/tigerlake/ac... PS18, Line 40: NCLK Who calls this method?
https://review.coreboot.org/c/coreboot/+/37781/18/src/soc/intel/tigerlake/ac... PS18, Line 40: ( space before (
https://review.coreboot.org/c/coreboot/+/37781/18/src/soc/intel/tigerlake/ac... PS18, Line 51: ( space before (
https://review.coreboot.org/c/coreboot/+/37781/18/src/soc/intel/tigerlake/ac... PS18, Line 65: ToInteger(Arg0) ToInteger is already done before call to CLKC. Is this required again?
https://review.coreboot.org/c/coreboot/+/37781/18/src/soc/intel/tigerlake/ac... PS18, Line 69: << spaces around <<
https://review.coreboot.org/c/coreboot/+/37781/18/src/soc/intel/tigerlake/ac... PS18, Line 69: ( space before (
https://review.coreboot.org/c/coreboot/+/37781/18/src/soc/intel/tigerlake/ac... PS18, Line 73: << space around <<
https://review.coreboot.org/c/coreboot/+/37781/18/src/soc/intel/tigerlake/ac... PS18, Line 73: ( space before (
https://review.coreboot.org/c/coreboot/+/37781/18/src/soc/intel/tigerlake/ac... PS18, Line 99: ( space before (
https://review.coreboot.org/c/coreboot/+/37781/18/src/soc/intel/tigerlake/ac... PS18, Line 101: ( space before (
https://review.coreboot.org/c/coreboot/+/37781/18/src/soc/intel/tigerlake/ac... PS18, Line 101: ( space before (
https://review.coreboot.org/c/coreboot/+/37781/18/src/soc/intel/tigerlake/ac... PS18, Line 137: MCCT What calls this method? Is there a kernel driver? Can you please provide pointers?
https://review.coreboot.org/c/coreboot/+/37781/18/src/soc/intel/tigerlake/ac... PS18, Line 137: ( space before (
https://review.coreboot.org/c/coreboot/+/37781/18/src/soc/intel/tigerlake/ac... PS18, Line 139: CLKC The order looks incorrect to me for enabling. Shouldn't it be: 1. Set frequency 2. Enable clock
and for disabling it should be 1. Disable clock 2. Set frequency should not be required.
https://review.coreboot.org/c/coreboot/+/37781/18/src/soc/intel/tigerlake/ac... File src/soc/intel/tigerlake/acpi/platform.asl:
https://review.coreboot.org/c/coreboot/+/37781/18/src/soc/intel/tigerlake/ac... PS18, Line 23: OperationRegion (APMP, SystemIO, 0xb2, 2) : Field (APMP, ByteAcc, NoLock, Preserve) : { : APMC, 8, /* APM command */ : APMS, 8 /* APM status */ : } Where and how are these used? I don't see any of the previous Intel SoCs adding these. Why is this required on tigerlake?
https://review.coreboot.org/c/coreboot/+/37781/18/src/soc/intel/tigerlake/ac... File src/soc/intel/tigerlake/acpi/pmc.asl:
https://review.coreboot.org/c/coreboot/+/37781/18/src/soc/intel/tigerlake/ac... PS18, Line 25: INT34D2 I remember this device was added on APL for PMC telemetry. But, we never added it on big-core platforms, mostly because the PMC registers were not the same on APL and big core. Is that not the case for TGL? Is the corresponding kernel driver the right one to use for TGL PMC?
https://review.coreboot.org/c/coreboot/+/37781/18/src/soc/intel/tigerlake/ac... PS18, Line 30: 0x10000 Use PCH_PWRM_BASE_SIZE
https://review.coreboot.org/c/coreboot/+/37781/18/src/soc/intel/tigerlake/ac... PS18, Line 31: , space after ,
https://review.coreboot.org/c/coreboot/+/37781/18/src/soc/intel/tigerlake/ac... PS18, Line 34: 0x12 This should use PMC_IRQ : https://review.coreboot.org/c/coreboot/+/38258/5/src/soc/intel/tigerlake/inc...
https://review.coreboot.org/c/coreboot/+/37781/18/src/soc/intel/tigerlake/ac... File src/soc/intel/tigerlake/acpi/sleepstates.asl:
PS18: This file is not required anymore: https://review.coreboot.org/c/coreboot/+/36463
https://review.coreboot.org/c/coreboot/+/37781/18/src/soc/intel/tigerlake/ac... File src/soc/intel/tigerlake/acpi/southbridge.asl:
https://review.coreboot.org/c/coreboot/+/37781/18/src/soc/intel/tigerlake/ac... PS18, Line 23: extra blank line not required.