Hello Douglas Anderson, Philip Chen, Bob Moragues,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/45306
to review the following change.
Change subject: trogdor: Move EN_PP3300_DX_EDP for Coachz ......................................................................
trogdor: Move EN_PP3300_DX_EDP for Coachz
This patch updates the display power enable GPIO which moved from 30 to 52 for Coachz. Veterans of this project know that there's no point trying to ask *why* this change was necessary -- the pins move in mysterious ways and all we can do is watch and wonder. Pin 30 is now used for a new camera reset GPIO... surely, there must have been some excellent reason why that pin couldn't just have become pin 52 instead.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I00ad6a6249df66006b4f2b953a0a2449bd478f6d --- M src/mainboard/google/trogdor/board.h 1 file changed, 2 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/45306/1
diff --git a/src/mainboard/google/trogdor/board.h b/src/mainboard/google/trogdor/board.h index 39661b5..4d38378 100644 --- a/src/mainboard/google/trogdor/board.h +++ b/src/mainboard/google/trogdor/board.h @@ -16,7 +16,8 @@ /* Display specific GPIOS */ #define GPIO_BACKLIGHT_ENABLE GPIO(12) #define GPIO_EDP_BRIDGE_ENABLE (CONFIG(TROGDOR_REV0) ? GPIO(14) : GPIO(104)) -#define GPIO_EN_PP3300_DX_EDP (CONFIG(TROGDOR_REV0) ? GPIO(106) : GPIO(30)) +#define GPIO_EN_PP3300_DX_EDP (CONFIG(TROGDOR_REV0) ? GPIO(106) : \ + (CONFIG(BOARD_GOOGLE_COACHZ) ? GPIO(52) : GPIO(30)))
void setup_chromeos_gpios(void);
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45306 )
Change subject: trogdor: Move EN_PP3300_DX_EDP for Coachz ......................................................................
Patch Set 1: Code-Review+1
Philip Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45306 )
Change subject: trogdor: Move EN_PP3300_DX_EDP for Coachz ......................................................................
Patch Set 1:
Do we still want to support TROGDOR_REV0?
Douglas Anderson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45306 )
Change subject: trogdor: Move EN_PP3300_DX_EDP for Coachz ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
Do we still want to support TROGDOR_REV0?
Unfortunately, I think the answer is yes for now. We're still low on hardware and I think people are still using old trogdor-rev0 boards out there. I actually still use mine, too, since it's hooked up to my PC at work and I haven't been able to go in and switch to newer hardware there.
https://review.coreboot.org/c/coreboot/+/45306/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45306/1//COMMIT_MSG@12 PS1, Line 12: Pin 30 is now : used for a new camera reset GPIO Apparently pin 30 was the MIPI camera on some reference board? Is it like the modem where the code that runs on the camera processor itself hardcodes its GPIOs and therefor is hard to change? ...or maybe someone didn't realize that it was just a GPIO. Doh.
Philip Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45306 )
Change subject: trogdor: Move EN_PP3300_DX_EDP for Coachz ......................................................................
Patch Set 1: Code-Review+2
Philip Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45306 )
Change subject: trogdor: Move EN_PP3300_DX_EDP for Coachz ......................................................................
Patch Set 1:
Patch Set 1: Code-Review+1
(1 comment)
Do we still want to support TROGDOR_REV0?
Unfortunately, I think the answer is yes for now. We're still low on hardware and I think people are still using old trogdor-rev0 boards out there. I actually still use mine, too, since it's hooked up to my PC at work and I haven't been able to go in and switch to newer hardware there.
Oh, ok, LGTM then.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45306 )
Change subject: trogdor: Move EN_PP3300_DX_EDP for Coachz ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45306/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45306/1//COMMIT_MSG@12 PS1, Line 12: Pin 30 is now : used for a new camera reset GPIO
Apparently pin 30 was the MIPI camera on some reference board? Is it like the modem where the code […]
We have Camera processors that have independent access to I/Os? Yikes...
Julius Werner has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45306 )
Change subject: trogdor: Move EN_PP3300_DX_EDP for Coachz ......................................................................
trogdor: Move EN_PP3300_DX_EDP for Coachz
This patch updates the display power enable GPIO which moved from 30 to 52 for Coachz. Veterans of this project know that there's no point trying to ask *why* this change was necessary -- the pins move in mysterious ways and all we can do is watch and wonder. Pin 30 is now used for a new camera reset GPIO... surely, there must have been some excellent reason why that pin couldn't just have become pin 52 instead.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I00ad6a6249df66006b4f2b953a0a2449bd478f6d Reviewed-on: https://review.coreboot.org/c/coreboot/+/45306 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Paul Menzel paulepanter@users.sourceforge.net Reviewed-by: Douglas Anderson dianders@chromium.org Reviewed-by: Philip Chen philipchen@google.com --- M src/mainboard/google/trogdor/board.h 1 file changed, 2 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Paul Menzel: Looks good to me, but someone else must approve Philip Chen: Looks good to me, approved Douglas Anderson: Looks good to me, but someone else must approve
diff --git a/src/mainboard/google/trogdor/board.h b/src/mainboard/google/trogdor/board.h index 39661b5..4d38378 100644 --- a/src/mainboard/google/trogdor/board.h +++ b/src/mainboard/google/trogdor/board.h @@ -16,7 +16,8 @@ /* Display specific GPIOS */ #define GPIO_BACKLIGHT_ENABLE GPIO(12) #define GPIO_EDP_BRIDGE_ENABLE (CONFIG(TROGDOR_REV0) ? GPIO(14) : GPIO(104)) -#define GPIO_EN_PP3300_DX_EDP (CONFIG(TROGDOR_REV0) ? GPIO(106) : GPIO(30)) +#define GPIO_EN_PP3300_DX_EDP (CONFIG(TROGDOR_REV0) ? GPIO(106) : \ + (CONFIG(BOARD_GOOGLE_COACHZ) ? GPIO(52) : GPIO(30)))
void setup_chromeos_gpios(void);
Douglas Anderson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45306 )
Change subject: trogdor: Move EN_PP3300_DX_EDP for Coachz ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45306/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45306/1//COMMIT_MSG@12 PS1, Line 12: Pin 30 is now : used for a new camera reset GPIO
We have Camera processors that have independent access to I/Os? Yikes...
It is only a guess. I know the modem processor was seen to mess with GPIOs, but the modem is also a more central part of the SoC. Hopefully the camera can't actually do this...
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45306 )
Change subject: trogdor: Move EN_PP3300_DX_EDP for Coachz ......................................................................
Patch Set 2:
Automatic boot test returned (PASS/FAIL/TOTAL): 8/1/9 "QEMU x86 q35/ich9" (x86_32) using payload TianoCore : SUCCESS : https://lava.9esec.io/r/20010 "QEMU x86 q35/ich9" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/20009 "QEMU x86 i440fx/piix4" (x86_64) using payload SeaBIOS : FAIL : https://lava.9esec.io/r/20008 "QEMU x86 i440fx/piix4" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/20007 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/20006 "HP Z220 SFF Workstation" (x86_32) using payload LinuxBoot_BusyBox_kexec : SUCCESS : https://lava.9esec.io/r/20014 "HP Z220 SFF Workstation" (x86_32) using payload LinuxBoot_BusyBox_kexec : SUCCESS : https://lava.9esec.io/r/20013 "HP Compaq 8200 Elite SFF PC" (x86_32) using payload TianoCore : SUCCESS : https://lava.9esec.io/r/20012 "HP Compaq 8200 Elite SFF PC" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/20011
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Douglas Anderson (Code Review)
-
Julius Werner (Code Review)
-
Paul Menzel (Code Review)
-
Philip Chen (Code Review)