Change in coreboot[master]: src/mb/google/volteer: Add camera ACPI configuration
Daniel Kang has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/39684 ) Change subject: src/mb/google/volteer: Add camera ACPI configuration ...................................................................... src/mb/google/volteer: Add camera ACPI configuration Add camera ACPI configuration for Ripto/Volteer BUG=None BRANCH=None TEST=Build and boot Ripto or Volteer. Start camera app and able to capture images. Signed-off-by: Daniel Kang <daniel.h.kang@intel.com> Change-Id: I2b47ccd989192273a29f09bf097e12e357929334 --- A src/mainboard/google/volteer/acpi/mipi_camera.asl M src/mainboard/google/volteer/dsdt.asl 2 files changed, 616 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/39684/1 diff --git a/src/mainboard/google/volteer/acpi/mipi_camera.asl b/src/mainboard/google/volteer/acpi/mipi_camera.asl new file mode 100644 index 0000000..1b12360 --- /dev/null +++ b/src/mainboard/google/volteer/acpi/mipi_camera.asl @@ -0,0 +1,613 @@ +/* + * 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 4533f89..d259926 100644 --- a/src/mainboard/google/volteer/dsdt.asl +++ b/src/mainboard/google/volteer/dsdt.asl @@ -47,4 +47,7 @@ } #include <southbridge/intel/common/acpi/sleepstates.asl> + /* Camera */ + #include <soc/intel/tigerlake/acpi/ipu.asl> + #include "acpi/mipi_camera.asl" } -- To view, visit https://review.coreboot.org/c/coreboot/+/39684 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2b47ccd989192273a29f09bf097e12e357929334 Gerrit-Change-Number: 39684 Gerrit-PatchSet: 1 Gerrit-Owner: Daniel Kang <daniel.h.kang@intel.com> Gerrit-MessageType: newchange
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39684 ) Change subject: src/mb/google/volteer: Add camera ACPI configuration ...................................................................... Patch Set 1: (13 comments) https://review.coreboot.org/c/coreboot/+/39684/1/src/mainboard/google/voltee... File src/mainboard/google/volteer/acpi/mipi_camera.asl: https://review.coreboot.org/c/coreboot/+/39684/1/src/mainboard/google/voltee... PS1, Line 187: #endif trailing whitespace https://review.coreboot.org/c/coreboot/+/39684/1/src/mainboard/google/voltee... PS1, Line 206: #endif trailing whitespace https://review.coreboot.org/c/coreboot/+/39684/1/src/mainboard/google/voltee... PS1, Line 414: ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device Properties for _DSD */, trailing whitespace https://review.coreboot.org/c/coreboot/+/39684/1/src/mainboard/google/voltee... PS1, Line 419: "size", trailing whitespace https://review.coreboot.org/c/coreboot/+/39684/1/src/mainboard/google/voltee... PS1, Line 421: }, trailing whitespace https://review.coreboot.org/c/coreboot/+/39684/1/src/mainboard/google/voltee... PS1, Line 424: "pagesize", trailing whitespace https://review.coreboot.org/c/coreboot/+/39684/1/src/mainboard/google/voltee... PS1, Line 426: }, trailing whitespace https://review.coreboot.org/c/coreboot/+/39684/1/src/mainboard/google/voltee... PS1, Line 429: "read-only", trailing whitespace https://review.coreboot.org/c/coreboot/+/39684/1/src/mainboard/google/voltee... PS1, Line 431: }, trailing whitespace https://review.coreboot.org/c/coreboot/+/39684/1/src/mainboard/google/voltee... PS1, Line 434: "address-width", trailing whitespace https://review.coreboot.org/c/coreboot/+/39684/1/src/mainboard/google/voltee... PS1, Line 436: }, trailing whitespace https://review.coreboot.org/c/coreboot/+/39684/1/src/mainboard/google/voltee... PS1, Line 439: "compatible", trailing whitespace https://review.coreboot.org/c/coreboot/+/39684/1/src/mainboard/google/voltee... PS1, Line 464: trailing whitespace -- To view, visit https://review.coreboot.org/c/coreboot/+/39684 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2b47ccd989192273a29f09bf097e12e357929334 Gerrit-Change-Number: 39684 Gerrit-PatchSet: 1 Gerrit-Owner: Daniel Kang <daniel.h.kang@intel.com> Gerrit-CC: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Thu, 19 Mar 2020 23:53:59 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
Hello build bot (Jenkins), Wonkyu Kim, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/39684 to look at the new patch set (#2). Change subject: src/mb/google/volteer: Add camera ACPI configuration ...................................................................... src/mb/google/volteer: Add camera ACPI configuration Add camera ACPI configuration for Ripto/Volteer BUG=None BRANCH=None TEST=Build and boot Ripto or Volteer. Start camera app and able to capture images. Signed-off-by: Daniel Kang <daniel.h.kang@intel.com> Change-Id: I2b47ccd989192273a29f09bf097e12e357929334 --- A src/mainboard/google/volteer/acpi/mipi_camera.asl M src/mainboard/google/volteer/dsdt.asl 2 files changed, 616 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/39684/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/39684 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2b47ccd989192273a29f09bf097e12e357929334 Gerrit-Change-Number: 39684 Gerrit-PatchSet: 2 Gerrit-Owner: Daniel Kang <daniel.h.kang@intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-MessageType: newpatchset
Daniel Kang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39684 ) Change subject: src/mb/google/volteer: Add camera ACPI configuration ...................................................................... Patch Set 2: (13 comments) https://review.coreboot.org/c/coreboot/+/39684/1/src/mainboard/google/voltee... File src/mainboard/google/volteer/acpi/mipi_camera.asl: https://review.coreboot.org/c/coreboot/+/39684/1/src/mainboard/google/voltee... PS1, Line 187: #endif
trailing whitespace Done
https://review.coreboot.org/c/coreboot/+/39684/1/src/mainboard/google/voltee... PS1, Line 206: #endif
trailing whitespace Done
https://review.coreboot.org/c/coreboot/+/39684/1/src/mainboard/google/voltee... PS1, Line 414: ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device Properties for _DSD */,
trailing whitespace Done
https://review.coreboot.org/c/coreboot/+/39684/1/src/mainboard/google/voltee... PS1, Line 419: "size",
trailing whitespace Done
https://review.coreboot.org/c/coreboot/+/39684/1/src/mainboard/google/voltee... PS1, Line 421: },
trailing whitespace Done
https://review.coreboot.org/c/coreboot/+/39684/1/src/mainboard/google/voltee... PS1, Line 424: "pagesize",
trailing whitespace Done
https://review.coreboot.org/c/coreboot/+/39684/1/src/mainboard/google/voltee... PS1, Line 426: },
trailing whitespace Done
https://review.coreboot.org/c/coreboot/+/39684/1/src/mainboard/google/voltee... PS1, Line 429: "read-only",
trailing whitespace Done
https://review.coreboot.org/c/coreboot/+/39684/1/src/mainboard/google/voltee... PS1, Line 431: },
trailing whitespace Done
https://review.coreboot.org/c/coreboot/+/39684/1/src/mainboard/google/voltee... PS1, Line 434: "address-width",
trailing whitespace Done
https://review.coreboot.org/c/coreboot/+/39684/1/src/mainboard/google/voltee... PS1, Line 436: },
trailing whitespace Done
https://review.coreboot.org/c/coreboot/+/39684/1/src/mainboard/google/voltee... PS1, Line 439: "compatible",
trailing whitespace Done
https://review.coreboot.org/c/coreboot/+/39684/1/src/mainboard/google/voltee... PS1, Line 464:
trailing whitespace Done
-- To view, visit https://review.coreboot.org/c/coreboot/+/39684 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2b47ccd989192273a29f09bf097e12e357929334 Gerrit-Change-Number: 39684 Gerrit-PatchSet: 2 Gerrit-Owner: Daniel Kang <daniel.h.kang@intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Thu, 19 Mar 2020 23:59:22 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-MessageType: comment
Daniel Kang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39684 ) Change subject: src/mb/google/volteer: Add camera ACPI configuration ...................................................................... Patch Set 2: Code-Review+1 -- To view, visit https://review.coreboot.org/c/coreboot/+/39684 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2b47ccd989192273a29f09bf097e12e357929334 Gerrit-Change-Number: 39684 Gerrit-PatchSet: 2 Gerrit-Owner: Daniel Kang <daniel.h.kang@intel.com> Gerrit-Reviewer: Daniel Kang <daniel.h.kang@intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Fri, 20 Mar 2020 20:54:17 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Hello build bot (Jenkins), Wonkyu Kim, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/39684 to look at the new patch set (#3). Change subject: src/mb/google/volteer: Add camera ACPI configuration ...................................................................... src/mb/google/volteer: Add camera ACPI configuration Add camera ACPI configuration for Ripto/Volteer BUG=None BRANCH=None TEST=Build and boot Ripto or Volteer. Start camera app and able to capture images. Signed-off-by: Daniel Kang <daniel.h.kang@intel.com> Change-Id: I2b47ccd989192273a29f09bf097e12e357929334 --- A src/mainboard/google/volteer/acpi/mipi_camera.asl M src/mainboard/google/volteer/dsdt.asl 2 files changed, 616 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/39684/3 -- To view, visit https://review.coreboot.org/c/coreboot/+/39684 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2b47ccd989192273a29f09bf097e12e357929334 Gerrit-Change-Number: 39684 Gerrit-PatchSet: 3 Gerrit-Owner: Daniel Kang <daniel.h.kang@intel.com> Gerrit-Reviewer: Daniel Kang <daniel.h.kang@intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-MessageType: newpatchset
Daniel Kang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39684 ) Change subject: src/mb/google/volteer: Add camera ACPI configuration ...................................................................... Patch Set 3: Code-Review+1 -- To view, visit https://review.coreboot.org/c/coreboot/+/39684 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2b47ccd989192273a29f09bf097e12e357929334 Gerrit-Change-Number: 39684 Gerrit-PatchSet: 3 Gerrit-Owner: Daniel Kang <daniel.h.kang@intel.com> Gerrit-Reviewer: Daniel Kang <daniel.h.kang@intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Fri, 20 Mar 2020 22:43:08 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39684 ) Change subject: src/mb/google/volteer: Add camera ACPI configuration ...................................................................... Patch Set 3: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/39684 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2b47ccd989192273a29f09bf097e12e357929334 Gerrit-Change-Number: 39684 Gerrit-PatchSet: 3 Gerrit-Owner: Daniel Kang <daniel.h.kang@intel.com> Gerrit-Reviewer: Daniel Kang <daniel.h.kang@intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Sat, 21 Mar 2020 00:08:49 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39684 ) Change subject: src/mb/google/volteer: Add camera ACPI configuration ...................................................................... src/mb/google/volteer: Add camera ACPI configuration Add camera ACPI configuration for Ripto/Volteer BUG=None BRANCH=None TEST=Build and boot Ripto or Volteer. Start camera app and able to capture images. Signed-off-by: Daniel Kang <daniel.h.kang@intel.com> Change-Id: I2b47ccd989192273a29f09bf097e12e357929334 Reviewed-on: https://review.coreboot.org/c/coreboot/+/39684 Tested-by: build bot (Jenkins) <no-reply@coreboot.org> Reviewed-by: Wonkyu Kim <wonkyu.kim@intel.com> --- A src/mainboard/google/volteer/acpi/mipi_camera.asl M src/mainboard/google/volteer/dsdt.asl 2 files changed, 616 insertions(+), 0 deletions(-) Approvals: build bot (Jenkins): Verified Wonkyu Kim: Looks good to me, approved Daniel Kang: Looks good to me, but someone else must approve diff --git a/src/mainboard/google/volteer/acpi/mipi_camera.asl b/src/mainboard/google/volteer/acpi/mipi_camera.asl new file mode 100644 index 0000000..7c2ca61 --- /dev/null +++ b/src/mainboard/google/volteer/acpi/mipi_camera.asl @@ -0,0 +1,613 @@ +/* + * 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 4533f89..d259926 100644 --- a/src/mainboard/google/volteer/dsdt.asl +++ b/src/mainboard/google/volteer/dsdt.asl @@ -47,4 +47,7 @@ } #include <southbridge/intel/common/acpi/sleepstates.asl> + /* Camera */ + #include <soc/intel/tigerlake/acpi/ipu.asl> + #include "acpi/mipi_camera.asl" } -- To view, visit https://review.coreboot.org/c/coreboot/+/39684 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2b47ccd989192273a29f09bf097e12e357929334 Gerrit-Change-Number: 39684 Gerrit-PatchSet: 4 Gerrit-Owner: Daniel Kang <daniel.h.kang@intel.com> Gerrit-Reviewer: Daniel Kang <daniel.h.kang@intel.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: merged
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39684 ) Change subject: src/mb/google/volteer: Add camera ACPI configuration ...................................................................... Patch Set 4: We want the camera configuration to be done as part of SSDT rather than the static asl file being added here. I am creating a revert for this since there is ongoing effort to add SSDT changes: b/146512346 -- To view, visit https://review.coreboot.org/c/coreboot/+/39684 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2b47ccd989192273a29f09bf097e12e357929334 Gerrit-Change-Number: 39684 Gerrit-PatchSet: 4 Gerrit-Owner: Daniel Kang <daniel.h.kang@intel.com> Gerrit-Reviewer: Daniel Kang <daniel.h.kang@intel.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Furquan Shaikh <furquan@google.com> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Mon, 23 Mar 2020 22:39:18 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment
Furquan Shaikh has created a revert of this change. ( https://review.coreboot.org/c/coreboot/+/39684 ) Change subject: src/mb/google/volteer: Add camera ACPI configuration ...................................................................... -- To view, visit https://review.coreboot.org/c/coreboot/+/39684 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2b47ccd989192273a29f09bf097e12e357929334 Gerrit-Change-Number: 39684 Gerrit-PatchSet: 4 Gerrit-Owner: Daniel Kang <daniel.h.kang@intel.com> Gerrit-Reviewer: Daniel Kang <daniel.h.kang@intel.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Furquan Shaikh <furquan@google.com> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: revert
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39684 ) Change subject: src/mb/google/volteer: Add camera ACPI configuration ...................................................................... Patch Set 4: (6 comments) Posting some comments for problems that I see with current implementation. I haven't gone through the entire change in detail, but we need to move to SSDT ASAP. https://review.coreboot.org/c/coreboot/+/39684/4/src/mainboard/google/voltee... File src/mainboard/google/volteer/acpi/mipi_camera.asl: https://review.coreboot.org/c/coreboot/+/39684/4/src/mainboard/google/voltee... PS4, Line 166: If ((STA == Zero)) Why is this check required? Does the _ON routine get called multiple times? https://review.coreboot.org/c/coreboot/+/39684/4/src/mainboard/google/voltee... PS4, Line 172: #if CONFIG(BOARD_GOOGLE_VOLTEER) This check should not be here. It is not at all scalable. We cannot keep adding configs for each variant in this file. https://review.coreboot.org/c/coreboot/+/39684/4/src/mainboard/google/voltee... PS4, Line 180: Sleep(2) /* reset pulse width */ Does this match the datasheet? https://review.coreboot.org/c/coreboot/+/39684/4/src/mainboard/google/voltee... PS4, Line 195: If ((STA == One)) Same question as above. Do you see _OFF getting called multiple times? https://review.coreboot.org/c/coreboot/+/39684/4/src/mainboard/google/voltee... PS4, Line 198: Sleep(1) /* t0+t1 */ Why is this sleep required? https://review.coreboot.org/c/coreboot/+/39684/4/src/mainboard/google/voltee... File src/mainboard/google/volteer/dsdt.asl: https://review.coreboot.org/c/coreboot/+/39684/4/src/mainboard/google/voltee... PS4, Line 50: /* Camera */ : #include <soc/intel/tigerlake/acpi/ipu.asl> : #include "acpi/mipi_camera.asl" This is not correct. You cannot just unconditionally add it here. There can be different variants of volteer which either do not use MIPI camera or use a very different configuration than volteer. This is not going to work. -- To view, visit https://review.coreboot.org/c/coreboot/+/39684 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2b47ccd989192273a29f09bf097e12e357929334 Gerrit-Change-Number: 39684 Gerrit-PatchSet: 4 Gerrit-Owner: Daniel Kang <daniel.h.kang@intel.com> Gerrit-Reviewer: Daniel Kang <daniel.h.kang@intel.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Furquan Shaikh <furquan@google.com> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Tue, 31 Mar 2020 17:07:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
Daniel Kang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39684 ) Change subject: src/mb/google/volteer: Add camera ACPI configuration ...................................................................... Patch Set 4: (6 comments) https://review.coreboot.org/c/coreboot/+/39684/4/src/mainboard/google/voltee... File src/mainboard/google/volteer/acpi/mipi_camera.asl: https://review.coreboot.org/c/coreboot/+/39684/4/src/mainboard/google/voltee... PS4, Line 166: If ((STA == Zero))
Why is this check required? Does the _ON routine get called multiple times? I thought this would be needed in case the two cameras share the same power rail. But now I don't think this is needed. Will remove this.
https://review.coreboot.org/c/coreboot/+/39684/4/src/mainboard/google/voltee... PS4, Line 172: #if CONFIG(BOARD_GOOGLE_VOLTEER)
This check should not be here. It is not at all scalable. […] This will be handled in SSDT properly.
https://review.coreboot.org/c/coreboot/+/39684/4/src/mainboard/google/voltee... PS4, Line 180: Sleep(2) /* reset pulse width */
Does this match the datasheet? In chapter 2.7 of the data sheet, "The reset pulse width should be greater than or equal to 2ms." I added this delay to meet this.
https://review.coreboot.org/c/coreboot/+/39684/4/src/mainboard/google/voltee... PS4, Line 195: If ((STA == One))
Same question as above. […] I thought this would be needed in case the two cameras share the same power rail. But now I don't think this is needed. Will remove this.
https://review.coreboot.org/c/coreboot/+/39684/4/src/mainboard/google/voltee... PS4, Line 198: Sleep(1) /* t0+t1 */
Why is this sleep required? The min values of t1 is 512 XVCLK cycles, so this is less than 1ms but added just 1.
https://review.coreboot.org/c/coreboot/+/39684/4/src/mainboard/google/voltee... File src/mainboard/google/volteer/dsdt.asl: https://review.coreboot.org/c/coreboot/+/39684/4/src/mainboard/google/voltee... PS4, Line 50: /* Camera */ : #include <soc/intel/tigerlake/acpi/ipu.asl> : #include "acpi/mipi_camera.asl"
This is not correct. You cannot just unconditionally add it here. […] This will be handled in SSDT properly.
-- To view, visit https://review.coreboot.org/c/coreboot/+/39684 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2b47ccd989192273a29f09bf097e12e357929334 Gerrit-Change-Number: 39684 Gerrit-PatchSet: 4 Gerrit-Owner: Daniel Kang <daniel.h.kang@intel.com> Gerrit-Reviewer: Daniel Kang <daniel.h.kang@intel.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Furquan Shaikh <furquan@google.com> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Wed, 01 Apr 2020 19:36:29 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Furquan Shaikh <furquan@google.com> Gerrit-MessageType: comment
participants (5)
-
build bot (Jenkins) (Code Review) -
Daniel Kang (Code Review)
-
Furquan Shaikh (Code Review)
-
Patrick Georgi (Code Review)
-
Wonkyu Kim (Code Review)