Change in ...coreboot[master]: mb/google/hatch: Change the DEVSLP reset config to PLTRST
Rizwan Qureshi has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32136
Change subject: mb/google/hatch: Change the DEVSLP reset config to PLTRST ......................................................................
mb/google/hatch: Change the DEVSLP reset config to PLTRST
In S3 the PCH is driving the DEVSLP signal low, assuming that the SATA device is already powered off. However on hatch the SATA power is still enabled. And since DEVSLP is low, this causes the SATA device to not enter low power state. The fix here is to set the pad cofnig to be reset on PLTRST assertion which will cause the pin to be high impedance state anf will be pulled up by the SATA device.
BUG=b:126611255 BRANCH=None TEST=Make sure that S3 and S0ix is working fine on hatch. And also make sure that DEVSLP is pulled high in S3.
Change-Id: Ifb6a71a72244522c8dd8d48e9b9f8dc6feef8981 Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com --- M src/mainboard/google/hatch/variants/baseboard/gpio.c 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/32136/1
diff --git a/src/mainboard/google/hatch/variants/baseboard/gpio.c b/src/mainboard/google/hatch/variants/baseboard/gpio.c index b974e49..2912875 100644 --- a/src/mainboard/google/hatch/variants/baseboard/gpio.c +++ b/src/mainboard/google/hatch/variants/baseboard/gpio.c @@ -241,7 +241,7 @@ /* E4 : M2_SSD_PE_WAKE_ODL */ PAD_CFG_GPI(GPP_E4, NONE, DEEP), /* E5 : SATA_DEVSLP1 */ - PAD_CFG_NF(GPP_E5, NONE, DEEP, NF1), + PAD_CFG_NF(GPP_E5, NONE, PLTRST, NF1), /* E6 : M2_SSD_RST_L */ PAD_NC(GPP_E6, NONE), /* E7 : GPP_E7 ==> NC */
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32136 )
Change subject: mb/google/hatch: Change the DEVSLP reset config to PLTRST ......................................................................
Patch Set 1: Code-Review+2
(2 comments)
https://review.coreboot.org/#/c/32136/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32136/1//COMMIT_MSG@12 PS1, Line 12: cofnig config
https://review.coreboot.org/#/c/32136/1//COMMIT_MSG@13 PS1, Line 13: anf and
Hello Subrata Banik, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32136
to look at the new patch set (#2).
Change subject: mb/google/hatch: Change the DEVSLP reset config to PLTRST ......................................................................
mb/google/hatch: Change the DEVSLP reset config to PLTRST
In S3 the PCH is driving the DEVSLP signal low, assuming that the SATA device is already powered off. However on hatch the SATA power is still enabled. And since DEVSLP is low, this causes the SATA device to not enter low power state. The fix here is to set the pad config to be reset on PLTRST assertion which will cause the pin to be high impedance state and will be pulled up by the SATA device.
BUG=b:126611255 BRANCH=None TEST=Make sure that S3 and S0ix is working fine on hatch. And also make sure that DEVSLP is pulled high in S3.
Change-Id: Ifb6a71a72244522c8dd8d48e9b9f8dc6feef8981 Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com --- M src/mainboard/google/hatch/variants/baseboard/gpio.c 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/32136/2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32136 )
Change subject: mb/google/hatch: Change the DEVSLP reset config to PLTRST ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/#/c/32136/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32136/2//COMMIT_MSG@10 PS2, Line 10: However on hatch the SATA power is still enabled. Why? Hardware design, driver issue?
https://review.coreboot.org/#/c/32136/2//COMMIT_MSG@11 PS2, Line 11: And And fits on the previous line.
Hello Subrata Banik, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32136
to look at the new patch set (#3).
Change subject: mb/google/hatch: Change the DEVSLP reset config to PLTRST ......................................................................
mb/google/hatch: Change the DEVSLP reset config to PLTRST
In S3 the PCH is driving the DEVSLP signal low, assuming that the SATA device is already powered off. However on hatch the SATA power is still enabled. And, since DEVSLP is low, this causes the SATA device to not enter low power state. The fix here is to set the pad config to be reset on PLTRST assertion which will cause the pin to be high impedance state and will be pulled up by the SATA device.
BUG=b:126611255 BRANCH=None TEST=Make sure that S3 and S0ix is working fine on hatch. And also make sure that DEVSLP is pulled high in S3.
Change-Id: Ifb6a71a72244522c8dd8d48e9b9f8dc6feef8981 Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com --- M src/mainboard/google/hatch/variants/baseboard/gpio.c 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/36/32136/3
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32136 )
Change subject: mb/google/hatch: Change the DEVSLP reset config to PLTRST ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/#/c/32136/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32136/2//COMMIT_MSG@10 PS2, Line 10: However on hatch the SATA power is still enabled.
Why? Hardware design, driver issue?
The expectation is that the PCH leaves the DEVSLP signal in high Z state so that it can be pulled high. But since the pad_rst_config is being preserved across deep sleep the signal is not in high Z. This can be fixed in 2 ways,
1. Add a load switch to power the SATA device and turn it off when going to sleep. 2. Make the pin to be in High Z state when PLTRST is asserted(which is this change).
Hatch is primarily designed for S0ix in mind (in which DEVSLP is well managed) and not S3. So it is not really an issue but a design choice.
https://review.coreboot.org/#/c/32136/2//COMMIT_MSG@11 PS2, Line 11: And
And fits on the previous line.
Done
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32136 )
Change subject: mb/google/hatch: Change the DEVSLP reset config to PLTRST ......................................................................
mb/google/hatch: Change the DEVSLP reset config to PLTRST
In S3 the PCH is driving the DEVSLP signal low, assuming that the SATA device is already powered off. However on hatch the SATA power is still enabled. And, since DEVSLP is low, this causes the SATA device to not enter low power state. The fix here is to set the pad config to be reset on PLTRST assertion which will cause the pin to be high impedance state and will be pulled up by the SATA device.
BUG=b:126611255 BRANCH=None TEST=Make sure that S3 and S0ix is working fine on hatch. And also make sure that DEVSLP is pulled high in S3.
Change-Id: Ifb6a71a72244522c8dd8d48e9b9f8dc6feef8981 Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/32136 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com --- M src/mainboard/google/hatch/variants/baseboard/gpio.c 1 file changed, 1 insertion(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/mainboard/google/hatch/variants/baseboard/gpio.c b/src/mainboard/google/hatch/variants/baseboard/gpio.c index 4d1c87e..0391dfe 100644 --- a/src/mainboard/google/hatch/variants/baseboard/gpio.c +++ b/src/mainboard/google/hatch/variants/baseboard/gpio.c @@ -241,7 +241,7 @@ /* E4 : M2_SSD_PE_WAKE_ODL */ PAD_CFG_GPI(GPP_E4, NONE, DEEP), /* E5 : SATA_DEVSLP1 */ - PAD_CFG_NF(GPP_E5, NONE, DEEP, NF1), + PAD_CFG_NF(GPP_E5, NONE, PLTRST, NF1), /* E6 : M2_SSD_RST_L */ PAD_NC(GPP_E6, NONE), /* E7 : GPP_E7 ==> NC */
-
Furquan Shaikh (Code Review) -
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)
-
Rizwan Qureshi (Code Review)