EricR Lai has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32552
Change subject: mb/google/sarien: Fine tune SD card D3 cold timing ......................................................................
mb/google/sarien: Fine tune SD card D3 cold timing
A13 and A15 need to set low before H12 reset. Change the program sequence for fit HW requirement.
BUG=b:131876963 TEST=boot up and check SD card functional
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: I2f1752070f24833aaaab75dea8493caf2ed7f157 --- M src/mainboard/google/sarien/variants/sarien/gpio.c 1 file changed, 7 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/32552/1
diff --git a/src/mainboard/google/sarien/variants/sarien/gpio.c b/src/mainboard/google/sarien/variants/sarien/gpio.c index 53a937f..76dd605 100644 --- a/src/mainboard/google/sarien/variants/sarien/gpio.c +++ b/src/mainboard/google/sarien/variants/sarien/gpio.c @@ -31,9 +31,9 @@ /* CLKOUT_LPC1 */ PAD_NC(GPP_A10, NONE), /* PME# */ PAD_NC(GPP_A11, NONE), /* BM_BUSY# */ PAD_NC(GPP_A12, NONE), -/* SUSWARN# */ PAD_CFG_GPO(GPP_A13, 0, DEEP), /* Card reader D3 cold */ + /* ESPI_RESET# */ -/* SUSACK# */ PAD_CFG_GPO(GPP_A15, 0, DEEP), /* Card reader D3 cold */ + /* SD_1P8_SEL */ PAD_NC(GPP_A16, NONE), /* SD_PWR_EN# */ PAD_NC(GPP_A17, NONE), /* ISH_GP0 */ PAD_NC(GPP_A18, NONE), @@ -224,9 +224,12 @@
/* Early pad configuration in bootblock */ static const struct pad_config early_gpio_table[] = { -/* M2_SKT2_CFG0 */ PAD_CFG_GPO(GPP_H12, 0, DEEP), /* D3 cold RST */ +/* SUSWARN# */ PAD_CFG_GPO(GPP_A13, 0, DEEP), /* Card reader D3 cold */ +/* SUSACK# */ PAD_CFG_GPO(GPP_A15, 0, DEEP), /* Card reader D3 cold */ + /* UART2_RXD */ PAD_CFG_NF(GPP_C20, NONE, DEEP, NF1), /* SERVOTX_UART */ /* UART2_TXD */ PAD_CFG_NF(GPP_C21, NONE, DEEP, NF1), /* SERVORX_UART */ +/* M2_SKT2_CFG0 */ PAD_CFG_GPO(GPP_H12, 0, DEEP), /* D3 cold RST */ /* I2C4_SDA */ PAD_CFG_NF(GPP_H8, NONE, DEEP, NF1), /* I2C_SDA_H1 */ /* I2C4_SCL */ PAD_CFG_NF(GPP_H9, NONE, DEEP, NF1), /* I2C_SCL_H1 */ /* DMIC_DATA1 */ PAD_CFG_GPI_APIC(GPP_D18, NONE, PLTRST, @@ -237,8 +240,8 @@ /* SATALED# */ PAD_CFG_GPI(GPP_E8, NONE, DEEP), /* RECOVERY# */ /* DDPD_HPD2 */ PAD_CFG_GPI(GPP_E15, NONE, DEEP), /* H1_FLASH_WP */ /* SSD RESET need to stay low first */ -/* M2_SKT2_CFG0 */ PAD_CFG_GPO(GPP_H12, 1, DEEP), /* D3 cold RST */ /* PWRBTN# */ PAD_CFG_NF(GPD3, UP_20K, DEEP, NF1), /* SIO_PWRBTN# */ +/* M2_SKT2_CFG0 */ PAD_CFG_GPO(GPP_H12, 1, DEEP), /* D3 cold RST */ };
const struct pad_config *variant_gpio_table(size_t *num)
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32552 )
Change subject: mb/google/sarien: Fine tune SD card D3 cold timing ......................................................................
Patch Set 1:
please help merge this due to this impact factory process.
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32552 )
Change subject: mb/google/sarien: Fine tune SD card D3 cold timing ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/32552/1/src/mainboard/google/sarien/variants... File src/mainboard/google/sarien/variants/sarien/gpio.c:
https://review.coreboot.org/#/c/32552/1/src/mainboard/google/sarien/variants... PS1, Line 244: PAD_CFG_GPO(GPP_H12, 1, DEEP), It is odd to set this twice in the same table, the timing is not going to be very long and it could be broken if the table were reordered or something added or removed. Maybe put GPP_H12=1 into the regular "gpio_table" so it will get deasserted later in the boot.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32552 )
Change subject: mb/google/sarien: Fine tune SD card D3 cold timing ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/32552/1/src/mainboard/google/sarien/variants... File src/mainboard/google/sarien/variants/sarien/gpio.c:
https://review.coreboot.org/#/c/32552/1/src/mainboard/google/sarien/variants... PS1, Line 244: PAD_CFG_GPO(GPP_H12, 1, DEEP),
It is odd to set this twice in the same table, the timing is not going to be very long and it could […]
This is modify by Lance. I am not sure what the result if put after. I have tried that as well. Since factory is building, maybe we can try after? Currently order is 1ms, if I put into regular table it increased to 300ms or 200ms I forgot. We can ask Intel it okay or not.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32552 )
Change subject: mb/google/sarien: Fine tune SD card D3 cold timing ......................................................................
Patch Set 1:
Patch Set 1:
(1 comment)
You can refer this CL:https://review.coreboot.org/c/coreboot/+/32434 Maybe we can discuss this in mail thread? It likely decide by Tim or Roy.
Lijian Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32552 )
Change subject: mb/google/sarien: Fine tune SD card D3 cold timing ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/32552/1/src/mainboard/google/sarien/variants... File src/mainboard/google/sarien/variants/sarien/gpio.c:
https://review.coreboot.org/#/c/32552/1/src/mainboard/google/sarien/variants... PS1, Line 244: PAD_CFG_GPO(GPP_H12, 1, DEEP),
This is modify by Lance. I am not sure what the result if put after. I have tried that as well. […]
Yes the reason we put it twice is totally for timing purpose.+Roy here
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32552 )
Change subject: mb/google/sarien: Fine tune SD card D3 cold timing ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/32552/1/src/mainboard/google/sarien/variants... File src/mainboard/google/sarien/variants/sarien/gpio.c:
https://review.coreboot.org/#/c/32552/1/src/mainboard/google/sarien/variants... PS1, Line 244: PAD_CFG_GPO(GPP_H12, 1, DEEP),
Yes the reason we put it twice is totally for timing purpose. […]
It would still get deasserted if we put the second GPIO setup into the normal table, it would just happen later. Does the reset have to be deasserted right after the power comes up?
Roy Park has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32552 )
Change subject: mb/google/sarien: Fine tune SD card D3 cold timing ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
https://review.coreboot.org/#/c/32552/1/src/mainboard/google/sarien/variants... File src/mainboard/google/sarien/variants/sarien/gpio.c:
https://review.coreboot.org/#/c/32552/1/src/mainboard/google/sarien/variants... PS1, Line 244: PAD_CFG_GPO(GPP_H12, 1, DEEP),
It would still get deasserted if we put the second GPIO setup into the normal table, it would just h […]
First, the total time of RESET asserted was very short when we checked the asserted RESET time. and it could be not good enough time to be recognized as LOW.
Second,when RESET pin was deasserted in the normal table, the total time of the asserted RESET was took too much long time.
To avoid these two concerns, we deasserted RESET pin in this early table.
Lijian Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32552 )
Change subject: mb/google/sarien: Fine tune SD card D3 cold timing ......................................................................
Patch Set 1: Code-Review+2
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32552 )
Change subject: mb/google/sarien: Fine tune SD card D3 cold timing ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/32552/1/src/mainboard/google/sarien/variants... File src/mainboard/google/sarien/variants/sarien/gpio.c:
https://review.coreboot.org/#/c/32552/1/src/mainboard/google/sarien/variants... PS1, Line 244: PAD_CFG_GPO(GPP_H12, 1, DEEP),
First, the total time of RESET asserted was very short when we checked the asserted RESET time. […]
Please add a comment to this entry that it needs to stay at the end of the table to ensure the reset pulse.
Hello Roy Park, Roy Mingi Park, Duncan Laurie, Lijian Zhao, build bot (Jenkins), Furquan Shaikh, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32552
to look at the new patch set (#2).
Change subject: mb/google/sarien: Fine tune SD card D3 cold timing ......................................................................
mb/google/sarien: Fine tune SD card D3 cold timing
A13 and A15 need to set low before H12 reset. Change the program sequence for fit HW requirement.
BUG=b:131876963 TEST=boot up and check SD card functional
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: I2f1752070f24833aaaab75dea8493caf2ed7f157 --- A src/mainboard/google/sarien/variants/arcada/include/variant/acpi/mainboard.asl M src/mainboard/google/sarien/variants/sarien/gpio.c A src/mainboard/google/sarien/variants/sarien/include/variant/acpi/mainboard.asl 3 files changed, 63 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/32552/2
Hello Roy Park, Roy Mingi Park, Duncan Laurie, Lijian Zhao, build bot (Jenkins), Furquan Shaikh, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32552
to look at the new patch set (#3).
Change subject: mb/google/sarien: Fine tune SD card D3 cold timing ......................................................................
mb/google/sarien: Fine tune SD card D3 cold timing
A13 and A15 need to set low before H12 reset. Change the program sequence for fit HW requirement.
BUG=b:131876963 TEST=boot up and check SD card functional
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: I2f1752070f24833aaaab75dea8493caf2ed7f157 --- M src/mainboard/google/sarien/variants/sarien/gpio.c 1 file changed, 7 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/32552/3
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32552 )
Change subject: mb/google/sarien: Fine tune SD card D3 cold timing ......................................................................
Patch Set 3:
(1 comment)
I put the comment at the beginning.
https://review.coreboot.org/#/c/32552/1/src/mainboard/google/sarien/variants... File src/mainboard/google/sarien/variants/sarien/gpio.c:
https://review.coreboot.org/#/c/32552/1/src/mainboard/google/sarien/variants... PS1, Line 244: PAD_CFG_GPO(GPP_H12, 1, DEEP),
Please add a comment to this entry that it needs to stay at the end of the table to ensure the reset […]
Done
Lijian Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32552 )
Change subject: mb/google/sarien: Fine tune SD card D3 cold timing ......................................................................
Patch Set 3: Code-Review+2
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32552 )
Change subject: mb/google/sarien: Fine tune SD card D3 cold timing ......................................................................
Patch Set 3: Code-Review+2
Duncan Laurie has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32552 )
Change subject: mb/google/sarien: Fine tune SD card D3 cold timing ......................................................................
mb/google/sarien: Fine tune SD card D3 cold timing
A13 and A15 need to set low before H12 reset. Change the program sequence for fit HW requirement.
BUG=b:131876963 TEST=boot up and check SD card functional
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: I2f1752070f24833aaaab75dea8493caf2ed7f157 Reviewed-on: https://review.coreboot.org/c/coreboot/+/32552 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Lijian Zhao lijian.zhao@intel.com Reviewed-by: Duncan Laurie dlaurie@chromium.org --- M src/mainboard/google/sarien/variants/sarien/gpio.c 1 file changed, 7 insertions(+), 5 deletions(-)
Approvals: build bot (Jenkins): Verified Duncan Laurie: Looks good to me, approved Lijian Zhao: Looks good to me, approved
diff --git a/src/mainboard/google/sarien/variants/sarien/gpio.c b/src/mainboard/google/sarien/variants/sarien/gpio.c index 53a937f..ec3b44d 100644 --- a/src/mainboard/google/sarien/variants/sarien/gpio.c +++ b/src/mainboard/google/sarien/variants/sarien/gpio.c @@ -31,9 +31,9 @@ /* CLKOUT_LPC1 */ PAD_NC(GPP_A10, NONE), /* PME# */ PAD_NC(GPP_A11, NONE), /* BM_BUSY# */ PAD_NC(GPP_A12, NONE), -/* SUSWARN# */ PAD_CFG_GPO(GPP_A13, 0, DEEP), /* Card reader D3 cold */ + /* ESPI_RESET# */ -/* SUSACK# */ PAD_CFG_GPO(GPP_A15, 0, DEEP), /* Card reader D3 cold */ + /* SD_1P8_SEL */ PAD_NC(GPP_A16, NONE), /* SD_PWR_EN# */ PAD_NC(GPP_A17, NONE), /* ISH_GP0 */ PAD_NC(GPP_A18, NONE), @@ -224,9 +224,12 @@
/* Early pad configuration in bootblock */ static const struct pad_config early_gpio_table[] = { -/* M2_SKT2_CFG0 */ PAD_CFG_GPO(GPP_H12, 0, DEEP), /* D3 cold RST */ +/* SUSWARN# */ PAD_CFG_GPO(GPP_A13, 0, DEEP), /* Card reader D3 cold */ +/* SUSACK# */ PAD_CFG_GPO(GPP_A15, 0, DEEP), /* Card reader D3 cold */ /* UART2_RXD */ PAD_CFG_NF(GPP_C20, NONE, DEEP, NF1), /* SERVOTX_UART */ /* UART2_TXD */ PAD_CFG_NF(GPP_C21, NONE, DEEP, NF1), /* SERVORX_UART */ +/* SSD RESET pin will stay low first */ +/* M2_SKT2_CFG0 */ PAD_CFG_GPO(GPP_H12, 0, DEEP), /* D3 cold RST */ /* I2C4_SDA */ PAD_CFG_NF(GPP_H8, NONE, DEEP, NF1), /* I2C_SDA_H1 */ /* I2C4_SCL */ PAD_CFG_NF(GPP_H9, NONE, DEEP, NF1), /* I2C_SCL_H1 */ /* DMIC_DATA1 */ PAD_CFG_GPI_APIC(GPP_D18, NONE, PLTRST, @@ -236,9 +239,8 @@ /* CPU_GP0 */ PAD_CFG_GPI(GPP_E3, NONE, DEEP), /* MEM_INTERLEAVED */ /* SATALED# */ PAD_CFG_GPI(GPP_E8, NONE, DEEP), /* RECOVERY# */ /* DDPD_HPD2 */ PAD_CFG_GPI(GPP_E15, NONE, DEEP), /* H1_FLASH_WP */ -/* SSD RESET need to stay low first */ -/* M2_SKT2_CFG0 */ PAD_CFG_GPO(GPP_H12, 1, DEEP), /* D3 cold RST */ /* PWRBTN# */ PAD_CFG_NF(GPD3, UP_20K, DEEP, NF1), /* SIO_PWRBTN# */ +/* M2_SKT2_CFG0 */ PAD_CFG_GPO(GPP_H12, 1, DEEP), /* D3 cold RST */ };
const struct pad_config *variant_gpio_table(size_t *num)
-
Duncan Laurie (Code Review) -
EricR Lai (Code Review)
-
Lijian Zhao (Code Review)
-
Roy Park (Code Review)