Wonkyu Kim 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. […]
Ack
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?
pcr_ids.h is already included in southbridge.asl, additional include file is not required.
https://review.coreboot.org/c/coreboot/+/37781/18/src/soc/intel/tigerlake/ac... PS18, Line 23: (
space before (
Ack
https://review.coreboot.org/c/coreboot/+/37781/18/src/soc/intel/tigerlake/ac... PS18, Line 24: ICLK,AnyAcc,Lock,Preserve)
space after ,
Ack
https://review.coreboot.org/c/coreboot/+/37781/18/src/soc/intel/tigerlake/ac... PS18, Line 40: NCLK
Who calls this method?
Currently it's not called. Remove
https://review.coreboot.org/c/coreboot/+/37781/18/src/soc/intel/tigerlake/ac... PS18, Line 40: (
space before (
Ack
https://review.coreboot.org/c/coreboot/+/37781/18/src/soc/intel/tigerlake/ac... PS18, Line 51: (
space before (
Ack
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. […]
Ack
https://review.coreboot.org/c/coreboot/+/37781/18/src/soc/intel/tigerlake/ac... PS18, Line 69: (
space before (
Ack
https://review.coreboot.org/c/coreboot/+/37781/18/src/soc/intel/tigerlake/ac... PS18, Line 69: <<
spaces around <<
Ack
https://review.coreboot.org/c/coreboot/+/37781/18/src/soc/intel/tigerlake/ac... PS18, Line 73: (
space before (
Ack
https://review.coreboot.org/c/coreboot/+/37781/18/src/soc/intel/tigerlake/ac... PS18, Line 73: <<
space around <<
Ack
https://review.coreboot.org/c/coreboot/+/37781/18/src/soc/intel/tigerlake/ac... PS18, Line 99: (
space before (
Ack
https://review.coreboot.org/c/coreboot/+/37781/18/src/soc/intel/tigerlake/ac... PS18, Line 101: (
space before (
Ack
https://review.coreboot.org/c/coreboot/+/37781/18/src/soc/intel/tigerlake/ac... PS18, Line 101: (
space before (
Ack
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?
This is called by mipi_camera.asl https://review.coreboot.org/c/coreboot/+/37863/7/src/mainboard/intel/tglrvp/...
https://review.coreboot.org/c/coreboot/+/37781/18/src/soc/intel/tigerlake/ac... PS18, Line 137: (
space before (
Ack
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: […]
Ack
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. […]
It's not required for boot up. We'll revisit the change and add later if we need.
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. […]
It's not required for boot up. We'll revisit the change and add later if we need.
https://review.coreboot.org/c/coreboot/+/37781/18/src/soc/intel/tigerlake/ac... PS18, Line 30: 0x10000
Use PCH_PWRM_BASE_SIZE
Ack
https://review.coreboot.org/c/coreboot/+/37781/18/src/soc/intel/tigerlake/ac... PS18, Line 31: ,
space after ,
Ack
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. […]
Ack
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. […]
Ack
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 4: * Copyright (C) 2017-2019 Intel Corp. : * (Written by Bora Guvendik bora.guvendik@intel.com for Intel Corp.) Don't need to udpate
https://review.coreboot.org/c/coreboot/+/37781/18/src/soc/intel/tigerlake/ac... PS18, Line 23:
extra blank line not required.
Ack