Change in coreboot[master]: Revert "src/mb/google/volteer: Add camera ACPI configuration"
Hello build bot (Jenkins), Daniel Kang, Patrick Georgi, Wonkyu Kim,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/39639
to review the following change.
Change subject: Revert "src/mb/google/volteer: Add camera ACPI configuration" ......................................................................
Revert "src/mb/google/volteer: Add camera ACPI configuration"
This reverts commit 140a4ae7bf2960ac7d095ba94847093f4755bf04.
Reason for revert: Need to move to SSDT for camera configuration.
Change-Id: I6c5cbb8ae057771191fb80ec1a1768e0e817b890 --- D src/mainboard/google/volteer/acpi/mipi_camera.asl M src/mainboard/google/volteer/dsdt.asl 2 files changed, 0 insertions(+), 616 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/39639/1
diff --git a/src/mainboard/google/volteer/acpi/mipi_camera.asl b/src/mainboard/google/volteer/acpi/mipi_camera.asl deleted file mode 100644 index 7c2ca61..0000000 --- a/src/mainboard/google/volteer/acpi/mipi_camera.asl +++ /dev/null @@ -1,613 +0,0 @@ -/* - * This file is part of the coreboot project. - * - * - * This program is free software; you can redistribute it and/or modify - * it under the terms of the GNU General Public License as published by - * the Free Software Foundation; version 2 of the License. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - */ - -Scope (_SB.PCI0.IPU0) -{ - Name (_DSD, Package (0x02) /* _DSD: Device-Specific Data */ - { - ToUUID ("dbb8e3e6-5886-4ba6-8795-1319f52a966b"), - Package (0x02) - { - Package (0x02) - { - "port0", - "PRT0" - }, - Package (0x02) - { - "port1", - "PRT1" - } - } - }) - - Name (PRT0, Package (0x04) - { - ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), - Package (0x01) - { - Package (0x02) - { - "port", - 5 - } - }, - ToUUID ("dbb8e3e6-5886-4ba6-8795-1319f52a966b"), - Package (0x01) - { - Package (0x02) - { - "endpoint0", - "EP00" - } - } - }) - - Name (PRT1, Package (0x04) - { - ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), - Package (0x01) - { - Package (0x02) - { - "port", - 1 - } - }, - - ToUUID ("dbb8e3e6-5886-4ba6-8795-1319f52a966b"), - Package (0x01) - { - Package (0x02) - { - "endpoint0", - "EP10" - } - } - }) -} - -Scope (_SB.PCI0.IPU0) -{ - Name (EP00, Package (0x02) - { - ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), - Package (0x04) - { - Package (0x02) - { - "endpoint", - Zero - }, - Package (0x02) - { - "clock-lanes", - Zero - }, - Package (0x02) - { - "data-lanes", - Package (0x04) - { - One, - 0x02, - 0x03, - 0x04 - } - }, - Package (0x02) - { - "remote-endpoint", - Package (0x03) - { - ^I2C3.CAM0, - Zero, - Zero - } - } - } - }) - Name (EP10, Package (0x02) - { - ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), - Package (0x04) - { - Package (0x02) - { - "endpoint", - Zero - }, - Package (0x02) - { - "clock-lanes", - Zero - }, - Package (0x02) - { - "data-lanes", - Package (0x04) - { - One, - 0x02 - } - }, - Package (0x02) - { - "remote-endpoint", - Package (0x03) - { - ^I2C2.CAM1, - Zero, - Zero - } - } - } - }) -} - -Scope (_SB.PCI0.I2C3) -{ - PowerResource (RCPR, 0x00, 0x0000) - { - Name (STA, Zero) - Method (_ON, 0, Serialized) /* Rear camera_ON_: Power On */ - { - If ((STA == Zero)) - { - /* Enable IMG_CLK */ - MCON(3,1) /* Clock 3, 19.2MHz */ - - /* Pull RST low */ -#if CONFIG(BOARD_GOOGLE_VOLTEER) - CTXS(GPP_F15) -#else - CTXS(GPP_D4) -#endif - - /* Pull PWREN high */ - STXS(GPP_H20) - Sleep(2) /* reset pulse width */ - - /* Pull RST high */ -#if CONFIG(BOARD_GOOGLE_VOLTEER) - STXS(GPP_F15) -#else - STXS(GPP_D4) -#endif - Sleep(1) /* t2 */ - - Store(1,STA) - } - } - Method (_OFF, 0, Serialized) /* Rear camera _OFF: Power Off */ - { - If ((STA == One)) - { - /* Disable IMG_CLK */ - Sleep(1) /* t0+t1 */ - MCOF(3) /* Clock 3 */ - - /* Pull RST low */ -#if CONFIG(BOARD_GOOGLE_VOLTEER) - CTXS(GPP_F15) -#else - CTXS(GPP_D4) -#endif - - /* Pull PWREN low */ - CTXS(GPP_H20) - - Store(0,STA) - } - } - Method (_STA, 0, NotSerialized) /* _STA: Status */ - { - Return (STA) - } - } - - Device (CAM0) - { - Name (_HID, "OVTI8856") /* _HID: Hardware ID */ - Name (_UID, Zero) /* _UID: Unique ID */ - Name (_DDN, "Ov 8856 Camera") /* _DDN: DOS Device Name */ - Method (_STA, 0, NotSerialized) /* _STA: Status */ - { - Return (0x0F) - } - Name (_CRS, ResourceTemplate () /* _CRS: Current Resource Settings */ - { - I2cSerialBus (0x0010, ControllerInitiated, 0x00061A80, - AddressingMode7Bit, "\_SB.PCI0.I2C3", - 0x00, ResourceConsumer, , - ) - }) - Name (_PR0, Package (0x01) /* _PR0: Power Resources for D0 */ - { - RCPR - }) - Name (_PR3, Package (0x01) /* _PR3: Power Resources for D3hot */ - { - RCPR - }) - Name (_DSD, Package (0x04) /* _DSD: Device-Specific Data */ - { - ToUUID ("dbb8e3e6-5886-4ba6-8795-1319f52a966b"), - Package (0x01) - { - Package (0x02) - { - "port0", - "PRT0" - } - }, - ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), - Package (0x02) - { - Package (0x02) - { - "clock-frequency", - 0x0124F800 - }, - Package (0x02) - { - "lens-focus", - Package (0x01) - { - VCM0 - } - } - } - }) - Name (PRT0, Package (0x04) - { - ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), - Package (0x01) - { - Package (0x02) - { - "port", - Zero - } - }, - ToUUID ("dbb8e3e6-5886-4ba6-8795-1319f52a966b"), - Package (0x01) - { - Package (0x02) - { - "endpoint0", - "EP00" - } - } - }) - Name (EP00, Package (0x02) - { - ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), - Package (0x05) - { - Package (0x02) - { - "endpoint", - Zero - }, - Package (0x02) - { - "clock-lanes", - Zero - }, - Package (0x02) - { - "data-lanes", - Package (0x04) - { - One, - 0x02, - 0x03, - 0x04 - } - }, - Package (0x02) - { - "link-frequencies", - Package (0x02) - { - 0x15752A00, - 0xABA9500 - } - }, - Package (0x02) - { - "remote-endpoint", - Package (0x03) - { - IPU0, - Zero, - Zero - } - } - } - }) - } - - Device (VCM0) - { - Name (_HID, "PRP0001") /* _HID: Hardware ID */ - Name (_UID, 0x03) /* _UID: Unique ID */ - Name (_DDN, "GT9769 VCM") /* _DDN: DOS Device Name */ - Method (_STA, 0, NotSerialized) /* _STA: Status */ - { - Return (0x0F) - } - Name (_CRS, ResourceTemplate () /* _CRS: Current Resource Settings */ - { - I2cSerialBus (0x000C, ControllerInitiated, 0x00061A80, - AddressingMode7Bit, "\_SB.PCI0.I2C3", - 0x00, ResourceConsumer, , - ) - }) - Name (_DEP, Package (0x01) /* _DEP: Dependencies */ - { - CAM0 - }) - Name (_PR0, Package (0x01) /* _PR0: Power Resources for D0 */ - { - RCPR - }) - Name (_PR3, Package (0x01) /* _PR3: Power Resources for D3hot */ - { - RCPR - }) - Name (_DSD, Package (0x02) /* _DSD: Device-Specific Data */ - { - ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), - Package (0x01) - { - Package (0x02) - { - "compatible", - "giantec,gt9769-vcm" - } - } - }) - } - Device (NVM0) - { - Name (_HID, "PRP0001") // _HID: Hardware ID - Name (_UID, 0x03) // _UID: Unique ID - Name (_DDN, "GT9769 EEPROM") // _DDN: DOS Device Name - Method (_STA, 0, NotSerialized) // _STA: Status - { - Return (0x0F) - } - Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings - { - I2cSerialBusV2 (0x0058, ControllerInitiated, 0x00061A80, - AddressingMode7Bit, "\_SB.PCI0.I2C3", - 0x00, ResourceConsumer, , Exclusive, - ) - }) - Name (_DEP, Package (0x01) // _DEP: Dependencies - { - CAM0 - }) - Name (_PR0, Package (0x01) // _PR0: Power Resources for D0 - { - RCPR - }) - Name (_PR3, Package (0x01) // _PR3: Power Resources for D3hot - { - RCPR - }) - Name (_DSD, Package (0x02) // _DSD: Device-Specific Data - { - ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device Properties for _DSD */, - Package (0x05) - { - Package (0x02) - { - "size", - 0x2800 - }, - Package (0x02) - { - "pagesize", - One - }, - Package (0x02) - { - "read-only", - One - }, - Package (0x02) - { - "address-width", - 0x0D - }, - Package (0x02) - { - "compatible", - "giantec,gt9769-eeprom" - } - } - }) - } -} - -Scope (_SB.PCI0.I2C2) -{ - PowerResource (FCPR, 0x00, 0x0000) - { - Name (STA, Zero) - Method (_ON, 0, Serialized) /* Front camera_ON_: Power On */ - { - If ((STA == Zero)) - { - /* Enable IMG_CLK */ - MCON(2,1) /* Clock 2, 19.2MHz */ - - /* Pull RST low */ - CTXS(GPP_D4) - - /* Pull SNRPWR_EN high */ - STXS(GPP_D18) - - /* Pull PWREN high */ - STXS(GPP_D17) - Sleep(10) /* t9 */ - - /* Pull RST high */ - STXS(GPP_D4) - Sleep(1) /* t2 */ - - Store(1,STA) - } - } - Method (_OFF, 0, Serialized) /* Front camera_OFF_: Power Off */ - { - If ((STA == One)) - { - /* Disable IMG_CLK */ - Sleep(1) /* t0+t1 */ - MCOF(2) /* Clock 2 */ - - /* Pull RST low */ - CTXS(GPP_D4) - - /* Pull PWREN low */ - CTXS(GPP_D17) - - /* Pull SNRPWR_EN low */ - CTXS(GPP_D18) - - Store(0,STA) - } - } - Method (_STA, 0, NotSerialized) /* _STA: Status */ - { - Return (STA) - } - } - - Device (CAM1) - { - Name (_HID, "OVTI2740") /* _HID: Hardware ID */ - Name (_UID, Zero) /* _UID: Unique ID */ - Name (_DDN, "Ov 2740 Camera") /* _DDN: DOS Device Name */ - Method (_STA, 0, NotSerialized) /* _STA: Status */ - { - Return (0x0F) - } - Name (_CRS, ResourceTemplate () /* _CRS: Current Resource Settings */ - { - I2cSerialBus (0x0010, ControllerInitiated, 0x00061A80, - AddressingMode7Bit, "\_SB.PCI0.I2C2", - 0x00, ResourceConsumer, , - ) - }) - Name (_PR0, Package (0x01) /* _PR0: Power Resources for D0 */ - { - FCPR - }) - Name (_PR3, Package (0x01) /* _PR3: Power Resources for D3hot */ - { - FCPR - }) - Name (_DSD, Package (0x04) /* _DSD: Device-Specific Data */ - { - ToUUID ("dbb8e3e6-5886-4ba6-8795-1319f52a966b"), - Package (0x01) - { - Package (0x02) - { - "port0", - "PRT0" - } - }, - ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), - Package (0x01) - { - Package (0x02) - { - "clock-frequency", - 0x0124F800 - } - } - }) - Name (PRT0, Package (0x04) - { - ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), - Package (0x01) - { - Package (0x02) - { - "port", - Zero - } - }, - ToUUID ("dbb8e3e6-5886-4ba6-8795-1319f52a966b"), - Package (0x01) - { - Package (0x02) - { - "endpoint0", - "EP00" - } - } - }) - Name (EP00, Package (0x02) - { - ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), - Package (0x05) - { - Package (0x02) - { - "endpoint", - Zero - }, - Package (0x02) - { - "clock-lanes", - Zero - }, - Package (0x02) - { - "data-lanes", - Package (0x02) - { - One, - 0x02 - } - }, - Package (0x02) - { - "link-frequencies", - Package (0x01) - { - 0xABA9500 - } - }, - Package (0x02) - { - "remote-endpoint", - Package (0x03) - { - IPU0, - One, - Zero - } - } - } - }) - } -} diff --git a/src/mainboard/google/volteer/dsdt.asl b/src/mainboard/google/volteer/dsdt.asl index d259926..4533f89 100644 --- a/src/mainboard/google/volteer/dsdt.asl +++ b/src/mainboard/google/volteer/dsdt.asl @@ -47,7 +47,4 @@ }
#include <southbridge/intel/common/acpi/sleepstates.asl> - /* Camera */ - #include <soc/intel/tigerlake/acpi/ipu.asl> - #include "acpi/mipi_camera.asl" }
Hello build bot (Jenkins), Daniel Kang, Patrick Georgi, Wonkyu Kim, Caveh Jalali, Rizwan Qureshi, Nick Vaccaro,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39639
to look at the new patch set (#2).
Change subject: Revert "src/mb/google/volteer: Add camera ACPI configuration" ......................................................................
Revert "src/mb/google/volteer: Add camera ACPI configuration"
This reverts commit 140a4ae7bf2960ac7d095ba94847093f4755bf04.
Reason for revert: Need to move to SSDT for camera configuration.
Signed-off-by: Furquan Shaikh furquan@google.com Change-Id: I6c5cbb8ae057771191fb80ec1a1768e0e817b890 --- D src/mainboard/google/volteer/acpi/mipi_camera.asl M src/mainboard/google/volteer/dsdt.asl 2 files changed, 0 insertions(+), 616 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/39639/2
Daniel H Kang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39639 )
Change subject: Revert "src/mb/google/volteer: Add camera ACPI configuration" ......................................................................
Patch Set 2: Code-Review-1
The patch was for temporary use until we completely move to SSDT. The temporal DSDT patch is needed for camera validation and power on works. I'll revert it once SSDT patch is merged.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39639 )
Change subject: Revert "src/mb/google/volteer: Add camera ACPI configuration" ......................................................................
Patch Set 2:
Patch Set 2: Code-Review-1
The patch was for temporary use until we completely move to SSDT. The temporal DSDT patch is needed for camera validation and power on works. I'll revert it once SSDT patch is merged.
1. It was already communicated that the static ASL change will not be merged upstream. It was supposed to be cherry-picked for any validation/testing.
2. In the future, can you please ensure that the CLs that you push for volteer/tgl have nvaccaro@ and caveh@ as reviewers.
3. mipi_camera.asl needs a number of clean ups.
Do you have an ETA on when SSDT changes would be ready?
Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39639 )
Change subject: Revert "src/mb/google/volteer: Add camera ACPI configuration" ......................................................................
Patch Set 2:
Patch Set 2:
Patch Set 2: Code-Review-1
The patch was for temporary use until we completely move to SSDT. The temporal DSDT patch is needed for camera validation and power on works. I'll revert it once SSDT patch is merged.
It was already communicated that the static ASL change will not be merged upstream. It was supposed to be cherry-picked for any validation/testing.
In the future, can you please ensure that the CLs that you push for volteer/tgl have nvaccaro@ and caveh@ as reviewers.
mipi_camera.asl needs a number of clean ups.
Do you have an ETA on when SSDT changes would be ready?
#1, We did not expect the patch for volteer was merged soon and it was prepared for cherry pick for proto2 PO as Nick's cheery pick patch will not work as it is. Daniel created patch based on last review feedback(clean up and update acpi based on Camera spec) which is same as TGL RVP patch. #2, We planed to add more reviewer soon but it's merged before adding. The patch for TGL RVP included Google engineers. #3, We're preparing ETA with JSL team as SSDT for Camera is common module change. We'll update soon.
I think we can remove this DSDT acpi change when we push SSDT change.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39639 )
Change subject: Revert "src/mb/google/volteer: Add camera ACPI configuration" ......................................................................
Patch Set 2:
#1, We did not expect the patch for volteer was merged soon and it was prepared for cherry pick for proto2 PO as Nick's cheery pick patch will not work as it is. Daniel created patch based on last review feedback(clean up and update acpi based on Camera spec) which is same as TGL RVP patch.
If you did not expect the patch to be merged, please do not mark +2 Code Review.
#2, We planed to add more reviewer soon but it's merged before adding. The patch for TGL RVP included Google engineers.
You should be adding volteer team members right away when you are reviewing the CL. Else, there are going to be misses and surprises.
#3, We're preparing ETA with JSL team as SSDT for Camera is common module change. We'll update soon.
Do you have an ETA?
I think we can remove this DSDT acpi change when we push SSDT change.
I don't think the DSDT is doing everything correctly. I have posted some comments on the original CL.
Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39639 )
Change subject: Revert "src/mb/google/volteer: Add camera ACPI configuration" ......................................................................
Patch Set 2:
Patch Set 2:
#1, We did not expect the patch for volteer was merged soon and it was prepared for cherry pick for proto2 PO as Nick's cheery pick patch will not work as it is. Daniel created patch based on last review feedback(clean up and update acpi based on Camera spec) which is same as TGL RVP patch.
If you did not expect the patch to be merged, please do not mark +2 Code Review.
==> Sure, we'll do.
#2, We planed to add more reviewer soon but it's merged before adding. The patch for TGL RVP included Google engineers.
You should be adding volteer team members right away when you are reviewing the CL. Else, there are going to be misses and surprises.
==> We'll do
#3, We're preparing ETA with JSL team as SSDT for Camera is common module change. We'll update soon.
Do you have an ETA?
==> Rizwan will provide design document and will also provide ETA based on design document review.
I think we can remove this DSDT acpi change when we push SSDT change.
I don't think the DSDT is doing everything correctly. I have posted some comments on the original CL.
==> We'll review the comments as SSDT consider review comments.
Furquan Shaikh has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/39639 )
Change subject: Revert "src/mb/google/volteer: Add camera ACPI configuration" ......................................................................
Abandoned
-
Daniel H Kang (Code Review) -
Furquan Shaikh (Code Review)
-
Wonkyu Kim (Code Review)