Hello Chris Wang,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/45082
to review the following change.
Change subject: mb/google/zork:Set HotplugEnhanced to NVME SSD ......................................................................
mb/google/zork:Set HotplugEnhanced to NVME SSD
Set the link hotplug type to hotplug Enhnced to workaround the Phison SSD training issue.
BUG=b:160949731 TEST=Build,Verified the Phison SSD is bootable. BRANCH=Zork
Signed-off-by: Chris Wang chris.wang@amd.corp-partner.google.com Change-Id: If98c9e577b7c67aaad3c3f2d05ed5e24fc855bc7 --- M src/mainboard/google/zork/variants/baseboard/fsps_baseboard_dalboz.c M src/mainboard/google/zork/variants/baseboard/fsps_baseboard_trembyle.c 2 files changed, 3 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/45082/1
diff --git a/src/mainboard/google/zork/variants/baseboard/fsps_baseboard_dalboz.c b/src/mainboard/google/zork/variants/baseboard/fsps_baseboard_dalboz.c index 4be866d..5dda6f9 100644 --- a/src/mainboard/google/zork/variants/baseboard/fsps_baseboard_dalboz.c +++ b/src/mainboard/google/zork/variants/baseboard/fsps_baseboard_dalboz.c @@ -22,6 +22,7 @@ .end_logical_lane = 5, .device_number = 1, .function_number = 7, + .link_hotplug = 3, // HotplugEnhanced .link_aspm = ASPM_L1, .link_aspm_L1_1 = true, .link_aspm_L1_2 = true, diff --git a/src/mainboard/google/zork/variants/baseboard/fsps_baseboard_trembyle.c b/src/mainboard/google/zork/variants/baseboard/fsps_baseboard_trembyle.c index 49d8ade..ca1a542 100644 --- a/src/mainboard/google/zork/variants/baseboard/fsps_baseboard_trembyle.c +++ b/src/mainboard/google/zork/variants/baseboard/fsps_baseboard_trembyle.c @@ -27,6 +27,7 @@ .end_logical_lane = 3, .device_number = 1, .function_number = 7, + .link_hotplug = 3, // HotplugEnhanced .link_aspm = ASPM_L1, .link_aspm_L1_1 = true, .link_aspm_L1_2 = true, @@ -72,6 +73,7 @@ .end_logical_lane = 1, .device_number = 1, .function_number = 7, + .link_hotplug = 3, // HotplugEnhanced .link_aspm = ASPM_L1, .link_aspm_L1_1 = true, .link_aspm_L1_2 = true,
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45082 )
Change subject: mb/google/zork:Set HotplugEnhanced to NVME SSD ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45082/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45082/1//COMMIT_MSG@9 PS1, Line 9: Set the link hotplug type to hotplug Enhnced to workaround the Phison : SSD training issue. It was not very clear to me from the bug why enabling of hotplug enhanced helps with the issue. Does disabling of PSPP have the same impact? Also, is it really correct to have the hotplug enhanced enabled for entire zork family? Have you ensured that there are no regressions on other devices?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45082 )
Change subject: mb/google/zork:Set HotplugEnhanced to NVME SSD ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45082/1/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/baseboard/fsps_baseboard_trembyle.c:
https://review.coreboot.org/c/coreboot/+/45082/1/src/mainboard/google/zork/v... PS1, Line 30: .link_hotplug = 3, // HotplugEnhanced Can we please introduce some macros that provide semantic meaning instead of open coded values?
https://review.coreboot.org/c/coreboot/+/45082/1/src/mainboard/google/zork/v... PS1, Line 76: .link_hotplug = 3, // HotplugEnhanced Why would this only need to be applied to nvme root port? Why wouldn't this be needed for all root ports?
Matt Papageorge has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45082 )
Change subject: mb/google/zork:Set HotplugEnhanced to NVME SSD ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45082/1/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/baseboard/fsps_baseboard_dalboz.c:
https://review.coreboot.org/c/coreboot/+/45082/1/src/mainboard/google/zork/v... PS1, Line 48: .port_present = true, SD Reader also causes failure. We should assume WLAN as well
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45082 )
Change subject: mb/google/zork:Set HotplugEnhanced to NVME SSD ......................................................................
Patch Set 1:
(4 comments)
https://review.coreboot.org/c/coreboot/+/45082/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45082/1//COMMIT_MSG@7 PS1, Line 7: mb/google/zork:Set Please add a space after the colon.
https://review.coreboot.org/c/coreboot/+/45082/1//COMMIT_MSG@9 PS1, Line 9: workaround work around
https://review.coreboot.org/c/coreboot/+/45082/1//COMMIT_MSG@9 PS1, Line 9: the Phison : SSD training issue. Please summarize the issue in the commit message.
https://review.coreboot.org/c/coreboot/+/45082/1//COMMIT_MSG@13 PS1, Line 13: TEST=Build,Verified the Phison SSD is bootable. Please add a space after the colon.
chris wang has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/45082 )
Change subject: mb/google/zork:Set HotplugEnhanced to NVME SSD ......................................................................
Abandoned
the fixed should belong to SMU. abandon this CL
-
Aaron Durbin (Code Review) -
chris wang (Code Review)
-
Furquan Shaikh (Code Review)
-
Matt Papageorge (Code Review)
-
Paul Menzel (Code Review)