Shelley Chen has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34484 )
Change subject: mb/google/hatch: Initialize SSD GPIOs in bootblock ......................................................................
mb/google/hatch: Initialize SSD GPIOs in bootblock
Moving these to bootblock as we are seeing some instances where devices are rebooting into the broken screen with the 0x5a error (no storage device detected). This needed to be done for KBL platforms and never got transferred to hatch.
BUG=b:137681648 BRANCH=None TEST=Run faft_bios
Change-Id: I8cf09c26d77d890f5d0490709504e9edf485a93f Signed-off-by: Shelley Chen shchen@google.com --- M src/mainboard/google/hatch/variants/baseboard/gpio.c 1 file changed, 6 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/34484/1
diff --git a/src/mainboard/google/hatch/variants/baseboard/gpio.c b/src/mainboard/google/hatch/variants/baseboard/gpio.c index c202f5c..e6cae15 100644 --- a/src/mainboard/google/hatch/variants/baseboard/gpio.c +++ b/src/mainboard/google/hatch/variants/baseboard/gpio.c @@ -439,6 +439,12 @@ PAD_CFG_GPI_APIC(GPP_C21, NONE, PLTRST, LEVEL, INVERT), /* C23 : WLAN_PE_RST# */ PAD_CFG_GPO(GPP_C23, 1, DEEP), + /* E1 : M2_SSD_PEDET */ + PAD_CFG_NF(GPP_E1, NONE, DEEP, NF1), + /* E4 : M2_SSD_PE_WAKE_ODL */ + PAD_CFG_GPI(GPP_E4, NONE, DEEP), + /* E5 : SATA_DEVSLP1 */ + PAD_CFG_NF(GPP_E5, NONE, PLTRST, NF1), /* F2 : MEM_CH_SEL */ PAD_CFG_GPI(GPP_F2, NONE, PLTRST), /* F11 : PCH_MEM_STRAP2 */
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34484 )
Change subject: mb/google/hatch: Initialize SSD GPIOs in bootblock ......................................................................
Patch Set 1:
(4 comments)
https://review.coreboot.org/c/coreboot/+/34484/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34484/1//COMMIT_MSG@7 PS1, Line 7: GPIOs Did you identify which of these are really required to fix the 0x5a issue?
https://review.coreboot.org/c/coreboot/+/34484/1//COMMIT_MSG@16 PS1, Line 16: Run faft_bios Is the 0x5a issue resolved with this for Hatch?
https://review.coreboot.org/c/coreboot/+/34484/1/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/34484/1/src/mainboard/google/hatch/... PS1, Line 445: GPP_E4 Is this really required? It seems to be a wake pin?
https://review.coreboot.org/c/coreboot/+/34484/1/src/mainboard/google/hatch/... PS1, Line 447: GPP_E5 Does this matter for NVMe? It seems to be a SATA related pin. It might be required for SATA disks though.
Shelley Chen has uploaded a new patch set (#2). ( https://review.coreboot.org/c/coreboot/+/34484 )
Change subject: mb/google/hatch: Initialize SSD GPIOs in bootblock ......................................................................
mb/google/hatch: Initialize SSD GPIOs in bootblock
Moving these to bootblock as we are seeing some instances where devices are rebooting into the broken screen with the 0x5a error (no bootable storage device in system). This needed to be done for KBL platforms and never got transferred to hatch.
BUG=b:137681648 BRANCH=None TEST=Run faft_bios
Change-Id: I8cf09c26d77d890f5d0490709504e9edf485a93f Signed-off-by: Shelley Chen shchen@google.com --- M src/mainboard/google/hatch/variants/baseboard/gpio.c 1 file changed, 6 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/34484/2
Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34484 )
Change subject: mb/google/hatch: Initialize SSD GPIOs in bootblock ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34484/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34484/1//COMMIT_MSG@16 PS1, Line 16: Run faft_bios
Is the 0x5a issue resolved with this for Hatch?
Not completely resolved. But I do believe that these are required. I think that there are still other issues.
https://review.coreboot.org/c/coreboot/+/34484/1/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/34484/1/src/mainboard/google/hatch/... PS1, Line 445: GPP_E4
Is this really required? It seems to be a wake pin?
I pulled the SSD pins over together, so I'm not sure if this one needs to be initialized in bootblock. I can take it out for now.
https://review.coreboot.org/c/coreboot/+/34484/1/src/mainboard/google/hatch/... PS1, Line 447: GPP_E5
Does this matter for NVMe? It seems to be a SATA related pin. […]
This fix is not specific for NVMes. Do you want me to separate the NVMe related GPIOs and the SATA specific ones? This was supposed to be relevant for all storage devices.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34484 )
Change subject: mb/google/hatch: Initialize SSD GPIOs in bootblock ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34484/1/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/34484/1/src/mainboard/google/hatch/... PS1, Line 447: GPP_E5
This fix is not specific for NVMes. […]
No, I am just trying to understand what the root cause of the issue is and why a GPIO needs to be initialized early on. This affects more than just Hatch and we should document the behavior correctly so that future devices can benefit from it too.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34484 )
Change subject: mb/google/hatch: Initialize SSD GPIOs in bootblock ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34484/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34484/1//COMMIT_MSG@16 PS1, Line 16: Run faft_bios
Not completely resolved. But I do believe that these are required. […]
Let's wait to root cause the issue to submit this change to ToT. It would be good to identify the actual behavior so that it helps more than just Hatch.
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34484
to look at the new patch set (#3).
Change subject: mb/google/hatch: Initialize SSD GPIOs in bootblock ......................................................................
mb/google/hatch: Initialize SSD GPIOs in bootblock
Moving these to bootblock as we are seeing some instances where devices are rebooting into the broken screen with the 0x5a error (no bootable storage device in system). This needed to be done for KBL platforms and never got transferred to hatch.
BUG=b:137681648 BRANCH=None TEST=Run faft_bios
Change-Id: I8cf09c26d77d890f5d0490709504e9edf485a93f Signed-off-by: Shelley Chen shchen@google.com --- M src/mainboard/google/hatch/variants/baseboard/gpio.c 1 file changed, 4 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/34484/3
Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34484 )
Change subject: mb/google/hatch: Initialize SSD GPIOs in bootblock ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34484/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34484/1//COMMIT_MSG@16 PS1, Line 16: Run faft_bios
Let's wait to root cause the issue to submit this change to ToT. […]
I thought that it would be useful for partners to run the tests with the early SSD GPIO inits to see if they are still hitting the 0x5a error with other storage devices.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34484 )
Change subject: mb/google/hatch: Initialize SSD GPIOs in bootblock ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34484/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34484/1//COMMIT_MSG@16 PS1, Line 16: Run faft_bios
I thought that it would be useful for partners to run the tests with the early SSD GPIO inits to see […]
I think we should do two things here:
1. Identify a reliable test case for reproducing the issue and ask partners to run those tests 2. Point them to this CL to see if the issue is fixed with these changes.
Feel free to raise individual bugs for each device to follow-up on the tests you want partners to run.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34484 )
Change subject: mb/google/hatch: Initialize SSD GPIOs in bootblock ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34484/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34484/3//COMMIT_MSG@10 PS3, Line 10: broken recovery?
https://review.coreboot.org/c/coreboot/+/34484/3//COMMIT_MSG@11 PS3, Line 11: KBL : platforms Please reference the commit.
https://review.coreboot.org/c/coreboot/+/34484/3//COMMIT_MSG@16 PS3, Line 16: faft_bios What is that?
Hello Paul Fagerburg, Tim Wawrzynczak, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34484
to look at the new patch set (#4).
Change subject: mb/google/hatch: Initialize SSD GPIOs in bootblock ......................................................................
mb/google/hatch: Initialize SSD GPIOs in bootblock
Moving these to bootblock as we are seeing some instances where devices are rebooting into the recovery broken screen with the 0x5a error (no bootable storage device in system). This needed to be done for KBL platforms and never got transferred to hatch.
Please reference https://review.coreboot.org/c/coreboot/+/23647
BUG=b:137681648 BRANCH=None TEST=Run autotest faft_bios and faft_ec suites
Change-Id: I8cf09c26d77d890f5d0490709504e9edf485a93f Signed-off-by: Shelley Chen shchen@google.com --- M src/mainboard/google/hatch/variants/baseboard/gpio.c 1 file changed, 4 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/34484/4
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34484 )
Change subject: mb/google/hatch: Initialize SSD GPIOs in bootblock ......................................................................
Patch Set 4: Code-Review+1
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34484 )
Change subject: mb/google/hatch: Initialize SSD GPIOs in bootblock ......................................................................
Patch Set 4: Code-Review+2
Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34484 )
Change subject: mb/google/hatch: Initialize SSD GPIOs in bootblock ......................................................................
Patch Set 4:
(7 comments)
Resolving comments for submission. We decided to submit these GPIOs as they are needed for now and debug further. The 0x5a issue still exists on certain SSDs.
https://review.coreboot.org/c/coreboot/+/34484/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34484/1//COMMIT_MSG@7 PS1, Line 7: GPIOs
Did you identify which of these are really required to fix the 0x5a issue?
Done
https://review.coreboot.org/c/coreboot/+/34484/1//COMMIT_MSG@16 PS1, Line 16: Run faft_bios
I think we should do two things here: […]
Done
https://review.coreboot.org/c/coreboot/+/34484/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34484/3//COMMIT_MSG@10 PS3, Line 10: broken
recovery?
Done
https://review.coreboot.org/c/coreboot/+/34484/3//COMMIT_MSG@11 PS3, Line 11: KBL : platforms
Please reference the commit.
Done
https://review.coreboot.org/c/coreboot/+/34484/3//COMMIT_MSG@16 PS3, Line 16: faft_bios
What is that?
Done
https://review.coreboot.org/c/coreboot/+/34484/1/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/34484/1/src/mainboard/google/hatch/... PS1, Line 445: GPP_E4
I pulled the SSD pins over together, so I'm not sure if this one needs to be initialized in bootbloc […]
Done
https://review.coreboot.org/c/coreboot/+/34484/1/src/mainboard/google/hatch/... PS1, Line 447: GPP_E5
No, I am just trying to understand what the root cause of the issue is and why a GPIO needs to be in […]
Done
Shelley Chen has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34484 )
Change subject: mb/google/hatch: Initialize SSD GPIOs in bootblock ......................................................................
mb/google/hatch: Initialize SSD GPIOs in bootblock
Moving these to bootblock as we are seeing some instances where devices are rebooting into the recovery broken screen with the 0x5a error (no bootable storage device in system). This needed to be done for KBL platforms and never got transferred to hatch.
Please reference https://review.coreboot.org/c/coreboot/+/23647
BUG=b:137681648 BRANCH=None TEST=Run autotest faft_bios and faft_ec suites
Change-Id: I8cf09c26d77d890f5d0490709504e9edf485a93f Signed-off-by: Shelley Chen shchen@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/34484 Reviewed-by: Paul Fagerburg pfagerburg@chromium.org Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/mainboard/google/hatch/variants/baseboard/gpio.c 1 file changed, 4 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Tim Wawrzynczak: Looks good to me, approved Paul Fagerburg: Looks good to me, but someone else must approve
diff --git a/src/mainboard/google/hatch/variants/baseboard/gpio.c b/src/mainboard/google/hatch/variants/baseboard/gpio.c index 8a0c948..5666d1c 100644 --- a/src/mainboard/google/hatch/variants/baseboard/gpio.c +++ b/src/mainboard/google/hatch/variants/baseboard/gpio.c @@ -446,6 +446,10 @@ PAD_CFG_GPI_APIC(GPP_C21, NONE, PLTRST, LEVEL, INVERT), /* C23 : WLAN_PE_RST# */ PAD_CFG_GPO(GPP_C23, 1, DEEP), + /* E1 : M2_SSD_PEDET */ + PAD_CFG_NF(GPP_E1, NONE, DEEP, NF1), + /* E5 : SATA_DEVSLP1 */ + PAD_CFG_NF(GPP_E5, NONE, PLTRST, NF1), /* F2 : MEM_CH_SEL */ PAD_CFG_GPI(GPP_F2, NONE, PLTRST), /* F11 : PCH_MEM_STRAP2 */
-
Furquan Shaikh (Code Review) -
Paul Fagerburg (Code Review)
-
Paul Menzel (Code Review)
-
Shelley Chen (Code Review)
-
Tim Wawrzynczak (Code Review)