Change in coreboot[master]: [TEST-ONLY] Distinguish SKU1 and 2 for eMMC and SSD respectively
Peichao Li has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35204 )
Change subject: [TEST-ONLY] Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
[TEST-ONLY] Distinguish SKU1 and 2 for eMMC and SSD respectively
1. SKU1 for eMMC 2. SKU2 for SSD
BUG=b:132918661 TEST=Verify SSD is disabled when SKU ID = 2/4/21/22
Signed-off-by: Peichao Wang peichao.wang@bitland.corp-partner.google.com Change-Id: I827e6f1420801d43e0eb4708b8b8ad1692ef7e9f --- M src/mainboard/google/hatch/variants/akemi/Makefile.inc M src/mainboard/google/hatch/variants/akemi/gpio.c A src/mainboard/google/hatch/variants/akemi/variant.c 3 files changed, 206 insertions(+), 24 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/35204/1
diff --git a/src/mainboard/google/hatch/variants/akemi/Makefile.inc b/src/mainboard/google/hatch/variants/akemi/Makefile.inc index 2f590bf..6cabe91 100644 --- a/src/mainboard/google/hatch/variants/akemi/Makefile.inc +++ b/src/mainboard/google/hatch/variants/akemi/Makefile.inc @@ -21,3 +21,4 @@
bootblock-y += gpio.c ramstage-y += gpio.c +ramstage-y += variant.c diff --git a/src/mainboard/google/hatch/variants/akemi/gpio.c b/src/mainboard/google/hatch/variants/akemi/gpio.c index 5a58839..4fa443f 100644 --- a/src/mainboard/google/hatch/variants/akemi/gpio.c +++ b/src/mainboard/google/hatch/variants/akemi/gpio.c @@ -19,6 +19,148 @@ #include <commonlib/helpers.h> #include <console/console.h>
+static const struct pad_config ssd_sku_gpio_table[] = { +/* A0 : NC */ + PAD_NC(GPP_A0, NONE), + /* A6 : NC */ + PAD_NC(GPP_A6, NONE), + /* A8 : NC */ + PAD_NC(GPP_A8, NONE), + /* A10 : NC */ + PAD_NC(GPP_A10, NONE), + /* A11 : NC */ + PAD_NC(GPP_A11, NONE), + /* A12 : NC */ + PAD_NC(GPP_A12, NONE), + /* A18 : NC */ + PAD_NC(GPP_A18, NONE), + /* A19 : NC */ + PAD_NC(GPP_A19, NONE), + /* A22 : NC */ + PAD_NC(GPP_A22, NONE), + /* A23 : NC */ + PAD_NC(GPP_A23, NONE), + /* B20 : NC */ + PAD_NC(GPP_B20, NONE), + /* B21 : NC */ + PAD_NC(GPP_B21, NONE), + /* B22 : NC */ + PAD_NC(GPP_B22, NONE), + /* C11 : NC */ + PAD_NC(GPP_C11, NONE), + /* C15 : NC */ + PAD_NC(GPP_C15, NONE), + /* F1 : NC */ + PAD_NC(GPP_F1, NONE), + /* F3 : MEM_STRAP_3 */ + PAD_CFG_GPI(GPP_F3, NONE, PLTRST), + /* F10 : MEM_STRAP_2 */ + PAD_CFG_GPI(GPP_F10, NONE, PLTRST), + /* F11 : NC */ + PAD_NC(GPP_F11, NONE), + /* F12 : NC */ + PAD_NC(GPP_F12, NONE), + /* F13 : NC */ + PAD_NC(GPP_F13, NONE), + /* F14 : NC */ + PAD_NC(GPP_F14, NONE), + /* F15 : NC */ + PAD_NC(GPP_F15, NONE), + /* F16 : NC */ + PAD_NC(GPP_F16, NONE), + /* F17 : NC */ + PAD_NC(GPP_F17, NONE), + /* F18 : NC */ + PAD_NC(GPP_F18, NONE), + /* F19 : NC */ + PAD_NC(GPP_F19, NONE), + /* F20 : NC */ + PAD_NC(GPP_F20, NONE), + /* F21 : NC */ + PAD_NC(GPP_F21, NONE), + /* F22 : NC */ + PAD_NC(GPP_F22, NONE), + /* H6 : NC */ + PAD_NC(GPP_H6, NONE), + /* H7 : NC */ + PAD_NC(GPP_H7, NONE), + /* H19 : MEM_STRAP_0 */ + PAD_CFG_GPI(GPP_H19, NONE, PLTRST), + /* H22 : MEM_STRAP_1 */ + PAD_CFG_GPI(GPP_H22, NONE, PLTRST), +}; + +static const struct pad_config emmc_sku_gpio_table[] = { + /* A0 : NC */ + PAD_NC(GPP_A0, NONE), + /* A6 : NC */ + PAD_NC(GPP_A6, NONE), + /* A8 : NC */ + PAD_NC(GPP_A8, NONE), + /* A10 : NC */ + PAD_NC(GPP_A10, NONE), + /* A11 : NC */ + PAD_NC(GPP_A11, NONE), + /* A12 : NC */ + PAD_NC(GPP_A12, NONE), + /* A18 : NC */ + PAD_NC(GPP_A18, NONE), + /* A19 : NC */ + PAD_NC(GPP_A19, NONE), + /* A22 : NC */ + PAD_NC(GPP_A22, NONE), + /* A23 : NC */ + PAD_NC(GPP_A23, NONE), + /* B20 : NC */ + PAD_NC(GPP_B20, NONE), + /* B21 : NC */ + PAD_NC(GPP_B21, NONE), + /* B22 : NC */ + PAD_NC(GPP_B22, NONE), + /* C11 : NC */ + PAD_NC(GPP_C11, NONE), + /* C15 : NC */ + PAD_NC(GPP_C15, NONE), + /* F1 : NC */ + PAD_NC(GPP_F1, NONE), + /* F3 : MEM_STRAP_3 */ + PAD_CFG_GPI(GPP_F3, NONE, PLTRST), + /* F10 : MEM_STRAP_2 */ + PAD_CFG_GPI(GPP_F10, NONE, PLTRST), + /* F11 : EMMC_CMD ==> EMMC_CMD */ + PAD_CFG_NF(GPP_F11,UP_20K, DEEP, NF1), + /* F12 : EMMC_DATA0 ==> EMMC_DAT0 */ + PAD_CFG_NF(GPP_F12,UP_20K, DEEP, NF1), + /* F13 : EMMC_DATA1 ==> EMMC_DAT1 */ + PAD_CFG_NF(GPP_F13,UP_20K, DEEP, NF1), + /* F14 : EMMC_DATA2 ==> EMMC_DAT2 */ + PAD_CFG_NF(GPP_F14,UP_20K, DEEP, NF1), + /* F15 : EMMC_DATA3 ==> EMMC_DAT3 */ + PAD_CFG_NF(GPP_F15,UP_20K, DEEP, NF1), + /* F16 : EMMC_DATA4 ==> EMMC_DAT4 */ + PAD_CFG_NF(GPP_F16,UP_20K, DEEP, NF1), + /* F17 : EMMC_DATA5 ==> EMMC_DAT5 */ + PAD_CFG_NF(GPP_F17,UP_20K, DEEP, NF1), + /* F18 : EMMC_DATA6 ==> EMMC_DAT6 */ + PAD_CFG_NF(GPP_F18,UP_20K, DEEP, NF1), + /* F19 : EMMC_DATA7 ==> EMMC_DAT7 */ + PAD_CFG_NF(GPP_F19,UP_20K, DEEP, NF1), + /* F20 : EMMC_RCLK ==> EMMC_RCLK */ + PAD_CFG_NF(GPP_F20, DN_20K, DEEP, NF1), + /* F21 : EMMC_CLK ==> EMMC_CLK */ + PAD_CFG_NF(GPP_F21, DN_20K, DEEP, NF1), + /* F22 : EMMC_RESET# ==> EMMC_RST_L */ + PAD_CFG_NF(GPP_F22, NONE, DEEP, NF1), + /* H6 : NC */ + PAD_NC(GPP_H6, NONE), + /* H7 : NC */ + PAD_NC(GPP_H7, NONE), + /* H19 : MEM_STRAP_0 */ + PAD_CFG_GPI(GPP_H19, NONE, PLTRST), + /* H22 : MEM_STRAP_1 */ + PAD_CFG_GPI(GPP_H22, NONE, PLTRST), +}; + static const struct pad_config gpio_table[] = { /* A0 : NC */ PAD_NC(GPP_A0, NONE), @@ -92,6 +234,17 @@
const struct pad_config *override_gpio_table(size_t *num) { + uint32_t sku_id = get_board_sku(); + /* For SSD SKU */ + if (sku_id == 2) { + *num = ARRAY_SIZE(ssd_sku_gpio_table); + return ssd_sku_gpio_table; + } + /* For eMMC SKU */ + if (sku_id == 1) { + *num = ARRAY_SIZE(emmc_sku_gpio_table); + return emmc_sku_gpio_table; + } *num = ARRAY_SIZE(gpio_table); return gpio_table; } @@ -128,30 +281,6 @@ PAD_CFG_NF(GPP_E5, NONE, PLTRST, NF1), /* F2 : MEM_CH_SEL */ PAD_CFG_GPI(GPP_F2, NONE, PLTRST), - /* F11 : EMMC_CMD ==> EMMC_CMD */ - PAD_CFG_NF(GPP_F11, NONE, DEEP, NF1), - /* F12 : EMMC_DATA0 ==> EMMC_DAT0 */ - PAD_CFG_NF(GPP_F12, NONE, DEEP, NF1), - /* F13 : EMMC_DATA1 ==> EMMC_DAT1 */ - PAD_CFG_NF(GPP_F13, NONE, DEEP, NF1), - /* F14 : EMMC_DATA2 ==> EMMC_DAT2 */ - PAD_CFG_NF(GPP_F14, NONE, DEEP, NF1), - /* F15 : EMMC_DATA3 ==> EMMC_DAT3 */ - PAD_CFG_NF(GPP_F15, NONE, DEEP, NF1), - /* F16 : EMMC_DATA4 ==> EMMC_DAT4 */ - PAD_CFG_NF(GPP_F16, NONE, DEEP, NF1), - /* F17 : EMMC_DATA5 ==> EMMC_DAT5 */ - PAD_CFG_NF(GPP_F17, NONE, DEEP, NF1), - /* F18 : EMMC_DATA6 ==> EMMC_DAT6 */ - PAD_CFG_NF(GPP_F18, NONE, DEEP, NF1), - /* F19 : EMMC_DATA7 ==> EMMC_DAT7 */ - PAD_CFG_NF(GPP_F19, NONE, DEEP, NF1), - /* F20 : EMMC_RCLK ==> EMMC_RCLK */ - PAD_CFG_NF(GPP_F20, NONE, DEEP, NF1), - /* F21 : EMMC_CLK ==> EMMC_CLK */ - PAD_CFG_NF(GPP_F21, NONE, DEEP, NF1), - /* F22 : EMMC_RESET# ==> EMMC_RST_L */ - PAD_CFG_NF(GPP_F22, NONE, DEEP, NF1), };
const struct pad_config *variant_early_gpio_table(size_t *num) diff --git a/src/mainboard/google/hatch/variants/akemi/variant.c b/src/mainboard/google/hatch/variants/akemi/variant.c new file mode 100644 index 0000000..1c4e420 --- /dev/null +++ b/src/mainboard/google/hatch/variants/akemi/variant.c @@ -0,0 +1,52 @@ +/* + * This file is part of the coreboot project. + * + * Copyright 2019 Google LLC + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <baseboard/variants.h> +#include <chip.h> +#include <soc/pci_devs.h> +#include <ec/google/chromeec/ec.h> +void variant_devtree_update(void) +{ + uint32_t sku_id; + struct device *emmc_host; + + struct device *ssd_host; + config_t *cfg = config_of_path(SA_DEVFN_ROOT); + emmc_host = pcidev_path_on_root(PCH_DEVFN_EMMC); + ssd_host = pcidev_path_on_root(PCH_DEVFN_SATA); + + + /* SKU ID 2 doesn't have a eMMC device, hence disable it. */ + sku_id = get_board_sku(); + if (sku_id == 2) { + if (emmc_host == NULL) + return; + emmc_host->enabled = 0; + cfg->ScsEmmcHs400Enabled = 0; + } + + /* SKU ID 1 doesn't have a SSD device, hence disable it. */ + + if (sku_id == 1) { + if (ssd_host == NULL) + return; + ssd_host->enabled = 0; + cfg->SataSalpSupport = 0; + cfg->SataMode = 0; + cfg->SataPortsEnable[1] = 0; + cfg->SataPortsDevSlp[1] = 0; + cfg->satapwroptimize = 0; + } +}
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35204 )
Change subject: [TEST-ONLY] Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
Patch Set 1:
(9 comments)
https://review.coreboot.org/c/coreboot/+/35204/1/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/akemi/gpio.c:
https://review.coreboot.org/c/coreboot/+/35204/1/src/mainboard/google/hatch/... PS1, Line 131: PAD_CFG_NF(GPP_F11,UP_20K, DEEP, NF1), space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/35204/1/src/mainboard/google/hatch/... PS1, Line 133: PAD_CFG_NF(GPP_F12,UP_20K, DEEP, NF1), space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/35204/1/src/mainboard/google/hatch/... PS1, Line 135: PAD_CFG_NF(GPP_F13,UP_20K, DEEP, NF1), space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/35204/1/src/mainboard/google/hatch/... PS1, Line 137: PAD_CFG_NF(GPP_F14,UP_20K, DEEP, NF1), space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/35204/1/src/mainboard/google/hatch/... PS1, Line 139: PAD_CFG_NF(GPP_F15,UP_20K, DEEP, NF1), space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/35204/1/src/mainboard/google/hatch/... PS1, Line 141: PAD_CFG_NF(GPP_F16,UP_20K, DEEP, NF1), space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/35204/1/src/mainboard/google/hatch/... PS1, Line 143: PAD_CFG_NF(GPP_F17,UP_20K, DEEP, NF1), space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/35204/1/src/mainboard/google/hatch/... PS1, Line 145: PAD_CFG_NF(GPP_F18,UP_20K, DEEP, NF1), space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/35204/1/src/mainboard/google/hatch/... PS1, Line 147: PAD_CFG_NF(GPP_F19,UP_20K, DEEP, NF1), space required after that ',' (ctx:VxV)
Peichao Li has removed Patrick Georgi from this change. ( https://review.coreboot.org/c/coreboot/+/35204 )
Change subject: [TEST-ONLY] Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
Removed reviewer Patrick Georgi.
Peichao Li has removed Martin Roth from this change. ( https://review.coreboot.org/c/coreboot/+/35204 )
Change subject: [TEST-ONLY] Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
Removed reviewer Martin Roth.
Peichao Li has uploaded a new patch set (#2). ( https://review.coreboot.org/c/coreboot/+/35204 )
Change subject: [TEST-ONLY] Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
[TEST-ONLY] Distinguish SKU1 and 2 for eMMC and SSD respectively
1. SKU1 for eMMC 2. SKU2 for SSD
BUG=b:132918661 TEST=Verify SSD is disabled when SKU ID = 2/4/21/22
Signed-off-by: Peichao Wang peichao.wang@bitland.corp-partner.google.com Change-Id: I827e6f1420801d43e0eb4708b8b8ad1692ef7e9f --- M src/mainboard/google/hatch/variants/akemi/Makefile.inc M src/mainboard/google/hatch/variants/akemi/gpio.c A src/mainboard/google/hatch/variants/akemi/variant.c 3 files changed, 206 insertions(+), 24 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/35204/2
Peichao Li has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35204 )
Change subject: [TEST-ONLY] Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
Patch Set 2:
(9 comments)
https://review.coreboot.org/c/coreboot/+/35204/1/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/akemi/gpio.c:
https://review.coreboot.org/c/coreboot/+/35204/1/src/mainboard/google/hatch/... PS1, Line 131: PAD_CFG_NF(GPP_F11,UP_20K, DEEP, NF1),
space required after that ',' (ctx:VxV)
Done
https://review.coreboot.org/c/coreboot/+/35204/1/src/mainboard/google/hatch/... PS1, Line 133: PAD_CFG_NF(GPP_F12,UP_20K, DEEP, NF1),
space required after that ',' (ctx:VxV)
Done
https://review.coreboot.org/c/coreboot/+/35204/1/src/mainboard/google/hatch/... PS1, Line 135: PAD_CFG_NF(GPP_F13,UP_20K, DEEP, NF1),
space required after that ',' (ctx:VxV)
Done
https://review.coreboot.org/c/coreboot/+/35204/1/src/mainboard/google/hatch/... PS1, Line 137: PAD_CFG_NF(GPP_F14,UP_20K, DEEP, NF1),
space required after that ',' (ctx:VxV)
Done
https://review.coreboot.org/c/coreboot/+/35204/1/src/mainboard/google/hatch/... PS1, Line 139: PAD_CFG_NF(GPP_F15,UP_20K, DEEP, NF1),
space required after that ',' (ctx:VxV)
Done
https://review.coreboot.org/c/coreboot/+/35204/1/src/mainboard/google/hatch/... PS1, Line 141: PAD_CFG_NF(GPP_F16,UP_20K, DEEP, NF1),
space required after that ',' (ctx:VxV)
Done
https://review.coreboot.org/c/coreboot/+/35204/1/src/mainboard/google/hatch/... PS1, Line 143: PAD_CFG_NF(GPP_F17,UP_20K, DEEP, NF1),
space required after that ',' (ctx:VxV)
Done
https://review.coreboot.org/c/coreboot/+/35204/1/src/mainboard/google/hatch/... PS1, Line 145: PAD_CFG_NF(GPP_F18,UP_20K, DEEP, NF1),
space required after that ',' (ctx:VxV)
Done
https://review.coreboot.org/c/coreboot/+/35204/1/src/mainboard/google/hatch/... PS1, Line 147: PAD_CFG_NF(GPP_F19,UP_20K, DEEP, NF1),
space required after that ',' (ctx:VxV)
Done
Hello build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35204
to look at the new patch set (#3).
Change subject: [TEST-ONLY] Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
[TEST-ONLY] Distinguish SKU1 and 2 for eMMC and SSD respectively
1. SKU1 for eMMC 2. SKU2 for SSD
BUG=NONE TEST=Verify SSD and eMMC SKUs will able to bring up respectively
Signed-off-by: Peichao Wang peichao.wang@bitland.corp-partner.google.com Change-Id: I827e6f1420801d43e0eb4708b8b8ad1692ef7e9f --- M src/mainboard/google/hatch/variants/akemi/Makefile.inc M src/mainboard/google/hatch/variants/akemi/gpio.c A src/mainboard/google/hatch/variants/akemi/variant.c 3 files changed, 206 insertions(+), 24 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/35204/3
Peichao Li has removed Patrick Georgi from this change. ( https://review.coreboot.org/c/coreboot/+/35204 )
Change subject: [TEST-ONLY] Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
Removed reviewer Patrick Georgi.
Peichao Li has removed Martin Roth from this change. ( https://review.coreboot.org/c/coreboot/+/35204 )
Change subject: [TEST-ONLY] Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
Removed reviewer Martin Roth.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35204 )
Change subject: [TEST-ONLY] Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
Patch Set 3:
(5 comments)
I haven't double checked this against the schematic, but overall LGTM
https://review.coreboot.org/c/coreboot/+/35204/3/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/akemi/gpio.c:
https://review.coreboot.org/c/coreboot/+/35204/3/src/mainboard/google/hatch/... PS3, Line 23: * nit: indent this
https://review.coreboot.org/c/coreboot/+/35204/3/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/akemi/variant.c:
https://review.coreboot.org/c/coreboot/+/35204/3/src/mainboard/google/hatch/... PS3, Line 20: void variant_devtree_update(void) Please put a newline between #includes and anything else.
https://review.coreboot.org/c/coreboot/+/35204/3/src/mainboard/google/hatch/... PS3, Line 24: Remove this empty line
https://review.coreboot.org/c/coreboot/+/35204/3/src/mainboard/google/hatch/... PS3, Line 29: Remove extra line
https://review.coreboot.org/c/coreboot/+/35204/3/src/mainboard/google/hatch/... PS3, Line 41: Remove empty line
Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35204 )
Change subject: [TEST-ONLY] Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
Patch Set 3:
May I consult what is bad thing for enabling eMMC and SSD controller always? Thanks.
Peichao Li has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35204 )
Change subject: [TEST-ONLY] Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
Patch Set 3:
Patch Set 3:
May I consult what is bad thing for enabling eMMC and SSD controller always? Thanks.
Dear Marco, if enable both MDIO and PCI-E controller, DUT may encounter interrupt storm since have relevant GPIOs(MDIO or PCI-E) not be closed.
Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35204 )
Change subject: [TEST-ONLY] Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
Patch Set 3:
Patch Set 3:
Patch Set 3:
May I consult what is bad thing for enabling eMMC and SSD controller always? Thanks.
Dear Marco, if enable both MDIO and PCI-E controller, DUT may encounter interrupt storm since have relevant GPIOs(MDIO or PCI-E) not be closed.
Then for the discussion of SKU ID 255 to support both storage, we will have this interrupt storm issue? Where is the interrupt storm came from? Sorry, if you discussed it then please just point me to the issue, thanks.
Hello Tim Wawrzynczak, Shelley Chen, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35204
to look at the new patch set (#4).
Change subject: [TEST-ONLY] Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
[TEST-ONLY] Distinguish SKU1 and 2 for eMMC and SSD respectively
1. SKU1 for eMMC 2. SKU2 for SSD
BUG=b:132918661 TEST=Verify SSD is disabled when SKU ID = 2/4/21/22
Signed-off-by: Peichao Wang peichao.wang@bitland.corp-partner.google.com Change-Id: I827e6f1420801d43e0eb4708b8b8ad1692ef7e9f --- M src/mainboard/google/hatch/variants/akemi/Makefile.inc M src/mainboard/google/hatch/variants/akemi/gpio.c A src/mainboard/google/hatch/variants/akemi/variant.c 3 files changed, 204 insertions(+), 24 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/35204/4
Peichao Li has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35204 )
Change subject: [TEST-ONLY] Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
Patch Set 4:
(5 comments)
https://review.coreboot.org/c/coreboot/+/35204/3/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/akemi/gpio.c:
https://review.coreboot.org/c/coreboot/+/35204/3/src/mainboard/google/hatch/... PS3, Line 23: *
nit: indent this
Done
https://review.coreboot.org/c/coreboot/+/35204/3/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/akemi/variant.c:
https://review.coreboot.org/c/coreboot/+/35204/3/src/mainboard/google/hatch/... PS3, Line 20: void variant_devtree_update(void)
Please put a newline between #includes and anything else.
Done
https://review.coreboot.org/c/coreboot/+/35204/3/src/mainboard/google/hatch/... PS3, Line 24:
Remove this empty line
Done
https://review.coreboot.org/c/coreboot/+/35204/3/src/mainboard/google/hatch/... PS3, Line 29:
Remove extra line
Done
https://review.coreboot.org/c/coreboot/+/35204/3/src/mainboard/google/hatch/... PS3, Line 41:
Remove empty line
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35204 )
Change subject: [TEST-ONLY] Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
Patch Set 4:
Patch Set 3:
Patch Set 3:
Patch Set 3:
May I consult what is bad thing for enabling eMMC and SSD controller always? Thanks.
Dear Marco, if enable both MDIO and PCI-E controller, DUT may encounter interrupt storm since have relevant GPIOs(MDIO or PCI-E) not be closed.
Then for the discussion of SKU ID 255 to support both storage, we will have this interrupt storm issue? Where is the interrupt storm came from? Sorry, if you discussed it then please just point me to the issue, thanks.
I am actually interested in knowing the answer to this. What is the interrupt storm that you are referring to here. I was under the impression that the controllers are being disabled for power or S0ix related reasons. I would really like to see an analysis of why GPIOs/controllers need to be configured differently between eMMC and SSD.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35204 )
Change subject: [TEST-ONLY] Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
Patch Set 4:
Patch Set 4:
Patch Set 3:
Patch Set 3:
Patch Set 3:
May I consult what is bad thing for enabling eMMC and SSD controller always? Thanks.
Dear Marco, if enable both MDIO and PCI-E controller, DUT may encounter interrupt storm since have relevant GPIOs(MDIO or PCI-E) not be closed.
Then for the discussion of SKU ID 255 to support both storage, we will have this interrupt storm issue? Where is the interrupt storm came from? Sorry, if you discussed it then please just point me to the issue, thanks.
I am actually interested in knowing the answer to this. What is the interrupt storm that you are referring to here. I was under the impression that the controllers are being disabled for power or S0ix related reasons. I would really like to see an analysis of why GPIOs/controllers need to be configured differently between eMMC and SSD.
I thought it was power as well. If the controllers are disabled, why are the interrupts enabled?
Peichao Li has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35204 )
Change subject: [TEST-ONLY] Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
Patch Set 4:
Patch Set 4:
Patch Set 4:
Patch Set 3:
Patch Set 3:
Patch Set 3:
May I consult what is bad thing for enabling eMMC and SSD controller always? Thanks.
Dear Marco, if enable both MDIO and PCI-E controller, DUT may encounter interrupt storm since have relevant GPIOs(MDIO or PCI-E) not be closed.
Then for the discussion of SKU ID 255 to support both storage, we will have this interrupt storm issue? Where is the interrupt storm came from? Sorry, if you discussed it then please just point me to the issue, thanks.
I am actually interested in knowing the answer to this. What is the interrupt storm that you are referring to here. I was under the impression that the controllers are being disabled for power or S0ix related reasons. I would really like to see an analysis of why GPIOs/controllers need to be configured differently between eMMC and SSD.
I thought it was power as well. If the controllers are disabled, why are the interrupts enabled?
Dear Marco Furquan Tim,
I need clarify interrupt is one of side effects. As I know 1. correct SKU ID must point to correct GPIO configuration, if not, will lead to side effects include interrupt storm. 2. Why will impact S0ix if don't close these relevant GPIOs. As I know if no device attach to the platform, relevant device can't respond system request when enter S0ix, so block system enter S0ix. for example PCI-E device need enable ASPM(L1.1 or L1.2) and if no relevant PCI-E device on platform, which will block system enter S0ix. 3. as I know if controller are disabled, need close relevant GPIOs(generally configure IO is GPI and Hi-Z), If not, these GPIOs may be impacted by external electromagnetic environment.
Peichao Li has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35204 )
Change subject: [TEST-ONLY] Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
Patch Set 4:
Patch Set 4:
Patch Set 4:
Patch Set 4:
Patch Set 3:
Patch Set 3:
Patch Set 3:
May I consult what is bad thing for enabling eMMC and SSD controller always? Thanks.
Dear Marco, if enable both MDIO and PCI-E controller, DUT may encounter interrupt storm since have relevant GPIOs(MDIO or PCI-E) not be closed.
Then for the discussion of SKU ID 255 to support both storage, we will have this interrupt storm issue? Where is the interrupt storm came from? Sorry, if you discussed it then please just point me to the issue, thanks.
I am actually interested in knowing the answer to this. What is the interrupt storm that you are referring to here. I was under the impression that the controllers are being disabled for power or S0ix related reasons. I would really like to see an analysis of why GPIOs/controllers need to be configured differently between eMMC and SSD.
I thought it was power as well. If the controllers are disabled, why are the interrupts enabled?
Dear Marco Furquan Tim,
I need clarify interrupt is one of side effects. As I know 1. correct SKU ID must point to correct GPIO configuration, if not, will lead to side effects include interrupt storm. 2. Why will impact S0ix if don't close these relevant GPIOs. As I know if no device attach to the platform, relevant device can't respond system request when enter S0ix, so block system enter S0ix. for example PCI-E device need enable ASPM(L1.1 or L1.2) and if no relevant PCI-E device on platform, which will block system enter S0ix. 3. as I know if controller are disabled, need close relevant GPIOs(generally configure IO is GPI and Hi-Z), If not, these GPIOs may be impacted by external electromagnetic environment.
Dear Sirs,
So if I am wrong, please correct me. Thanks you very much!
Best regards
Peichao Li has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35204 )
Change subject: [TEST-ONLY] Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
Patch Set 4:
Patch Set 4:
Patch Set 4:
Patch Set 4:
Patch Set 4:
Patch Set 3:
Patch Set 3:
> Patch Set 3: > > May I consult what is bad thing for enabling eMMC and SSD controller always? Thanks.
Dear Marco, if enable both MDIO and PCI-E controller, DUT may encounter interrupt storm since have relevant GPIOs(MDIO or PCI-E) not be closed.
Then for the discussion of SKU ID 255 to support both storage, we will have this interrupt storm issue? Where is the interrupt storm came from? Sorry, if you discussed it then please just point me to the issue, thanks.
I am actually interested in knowing the answer to this. What is the interrupt storm that you are referring to here. I was under the impression that the controllers are being disabled for power or S0ix related reasons. I would really like to see an analysis of why GPIOs/controllers need to be configured differently between eMMC and SSD.
I thought it was power as well. If the controllers are disabled, why are the interrupts enabled?
Dear Marco Furquan Tim,
I need clarify interrupt is one of side effects. As I know 1. correct SKU ID must point to correct GPIO configuration, if not, will lead to side effects include interrupt storm. 2. Why will impact S0ix if don't close these relevant GPIOs. As I know if no device attach to the platform, relevant device can't respond system request when enter S0ix, so block system enter S0ix. for example PCI-E device need enable ASPM(L1.1 or L1.2) and if no relevant PCI-E device on platform, which will block system enter S0ix. 3. as I know if controller are disabled, need close relevant GPIOs(generally configure IO is GPI and Hi-Z), If not, these GPIOs may be impacted by external electromagnetic environment.
Dear Sirs,
So if I am wrong, please correct me. Thanks you very much!
Best regards
sorry correct: I need clarify interrupt is one of side effects. the interrupt actually point to interrupt storm.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35204 )
Change subject: [TEST-ONLY] Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
Patch Set 4:
Patch Set 4:
Patch Set 4:
Patch Set 4:
Patch Set 4:
Patch Set 4:
Patch Set 3:
> Patch Set 3: > > > Patch Set 3: > > > > May I consult what is bad thing for enabling eMMC and SSD controller always? Thanks. > > Dear Marco, if enable both MDIO and PCI-E controller, DUT may encounter interrupt storm since have relevant GPIOs(MDIO or PCI-E) not be closed.
Then for the discussion of SKU ID 255 to support both storage, we will have this interrupt storm issue? Where is the interrupt storm came from? Sorry, if you discussed it then please just point me to the issue, thanks.
I am actually interested in knowing the answer to this. What is the interrupt storm that you are referring to here. I was under the impression that the controllers are being disabled for power or S0ix related reasons. I would really like to see an analysis of why GPIOs/controllers need to be configured differently between eMMC and SSD.
I thought it was power as well. If the controllers are disabled, why are the interrupts enabled?
Dear Marco Furquan Tim,
I need clarify interrupt is one of side effects. As I know 1. correct SKU ID must point to correct GPIO configuration, if not, will lead to side effects include interrupt storm. 2. Why will impact S0ix if don't close these relevant GPIOs. As I know if no device attach to the platform, relevant device can't respond system request when enter S0ix, so block system enter S0ix. for example PCI-E device need enable ASPM(L1.1 or L1.2) and if no relevant PCI-E device on platform, which will block system enter S0ix. 3. as I know if controller are disabled, need close relevant GPIOs(generally configure IO is GPI and Hi-Z), If not, these GPIOs may be impacted by external electromagnetic environment.
Dear Sirs,
So if I am wrong, please correct me. Thanks you very much!
Best regards
sorry correct: I need clarify interrupt is one of side effects. the interrupt actually point to interrupt storm.
#2 and #2 I can understand. But I don't understand #1. Would that be a problem with SKU255 as well?
Peichao Li has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35204 )
Change subject: [TEST-ONLY] Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
Patch Set 4:
Patch Set 4:
Patch Set 4:
Patch Set 4:
Patch Set 4:
Patch Set 4:
Patch Set 4:
> Patch Set 3: > > > Patch Set 3: > > > > > Patch Set 3: > > > > > > May I consult what is bad thing for enabling eMMC and SSD controller always? Thanks. > > > > Dear Marco, if enable both MDIO and PCI-E controller, DUT may encounter interrupt storm since have relevant GPIOs(MDIO or PCI-E) not be closed. > > Then for the discussion of SKU ID 255 to support both storage, we will have this interrupt storm issue? Where is the interrupt storm came from? Sorry, if you discussed it then please just point me to the issue, thanks.
I am actually interested in knowing the answer to this. What is the interrupt storm that you are referring to here. I was under the impression that the controllers are being disabled for power or S0ix related reasons. I would really like to see an analysis of why GPIOs/controllers need to be configured differently between eMMC and SSD.
I thought it was power as well. If the controllers are disabled, why are the interrupts enabled?
Dear Marco Furquan Tim,
I need clarify interrupt is one of side effects. As I know 1. correct SKU ID must point to correct GPIO configuration, if not, will lead to side effects include interrupt storm. 2. Why will impact S0ix if don't close these relevant GPIOs. As I know if no device attach to the platform, relevant device can't respond system request when enter S0ix, so block system enter S0ix. for example PCI-E device need enable ASPM(L1.1 or L1.2) and if no relevant PCI-E device on platform, which will block system enter S0ix. 3. as I know if controller are disabled, need close relevant GPIOs(generally configure IO is GPI and Hi-Z), If not, these GPIOs may be impacted by external electromagnetic environment.
Dear Sirs,
So if I am wrong, please correct me. Thanks you very much!
Best regards
sorry correct: I need clarify interrupt is one of side effects. the interrupt actually point to interrupt storm.
#2 and #2 I can understand. But I don't understand #1. Would that be a problem with SKU255 as well?
SKU 255 for eMMC/SSD if fine, I am no concern in factory flow. I mean we also need assign different sku for different configuration DUTs when factory flow finish.
Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35204 )
Change subject: [TEST-ONLY] Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
Patch Set 4:
Patch Set 4: I need clarify interrupt is one of side effects. As I know 1. correct SKU ID must point to correct GPIO configuration, if not, will lead to side effects include interrupt storm. 2. Why will impact S0ix if don't close these relevant GPIOs. As I know if no device attach to the platform, relevant device can't respond system request when enter S0ix, so block system enter S0ix. for example PCI-E device need enable ASPM(L1.1 or L1.2) and if no relevant PCI-E device on platform, which will block system enter S0ix. 3. as I know if controller are disabled, need close relevant GPIOs(generally configure IO is GPI and Hi-Z), If not, these GPIOs may be impacted by external electromagnetic environment.
Whether eMMC or SSD in M/B will be locked down during SMT by BOM therefore could we take care of that for non-mounting one (either eMMC or SSD) we can fine-tune pins for N/C, PU or PD to avoid any leakage or interrupt storm? As a result, at least we can support 255?
Peichao Li has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35204 )
Change subject: [TEST-ONLY] Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
Patch Set 4:
Patch Set 4:
Patch Set 4: I need clarify interrupt is one of side effects. As I know 1. correct SKU ID must point to correct GPIO configuration, if not, will lead to side effects include interrupt storm. 2. Why will impact S0ix if don't close these relevant GPIOs. As I know if no device attach to the platform, relevant device can't respond system request when enter S0ix, so block system enter S0ix. for example PCI-E device need enable ASPM(L1.1 or L1.2) and if no relevant PCI-E device on platform, which will block system enter S0ix. 3. as I know if controller are disabled, need close relevant GPIOs(generally configure IO is GPI and Hi-Z), If not, these GPIOs may be impacted by external electromagnetic environment.
Whether eMMC or SSD in M/B will be locked down during SMT by BOM therefore could we take care of that for non-mounting one (either eMMC or SSD) we can fine-tune pins for N/C, PU or PD to avoid any leakage or interrupt storm? As a result, at least we can support 255?
Dear Marco, sorry feedback to you later. I confirmed with our factory team and PM, Proto board just power on, BTW don't do factory flow. So I think SKU255 for Proto build that is fine.
Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35204 )
Change subject: [TEST-ONLY] Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
Patch Set 4:
Patch Set 4:
Whether eMMC or SSD in M/B will be locked down during SMT by BOM therefore could we take care of that for non-mounting one (either eMMC or SSD) we can fine-tune pins for N/C, PU or PD to avoid any leakage or interrupt storm? As a result, at least we can support 255?
Dear Marco, sorry feedback to you later. I confirmed with our factory team and PM, Proto board just power on, BTW don't do factory flow. So I think SKU255 for Proto build that is fine.
1. Even for proto stage, the SKU ID 255 still needs to be ensure that board can be power on generally; otherwise everyone got this board needs to re-program SKU ID by servo console?
2. Considering to EVT, we still need to evaluate to boot up by 255.
Peichao Li has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35204 )
Change subject: [TEST-ONLY] Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
Patch Set 4:
Patch Set 4:
Patch Set 4:
Whether eMMC or SSD in M/B will be locked down during SMT by BOM therefore could we take care of that for non-mounting one (either eMMC or SSD) we can fine-tune pins for N/C, PU or PD to avoid any leakage or interrupt storm? As a result, at least we can support 255?
Dear Marco, sorry feedback to you later. I confirmed with our factory team and PM, Proto board just power on, BTW don't do factory flow. So I think SKU255 for Proto build that is fine.
Even for proto stage, the SKU ID 255 still needs to be ensure that board can be power on generally; otherwise everyone got this board needs to re-program SKU ID by servo console?
Considering to EVT, we still need to evaluate to boot up by 255.
1. Even for proto stage, the SKU ID 255 still needs to be ensure that board can be power on generally; otherwise everyone got this board needs to re-program SKU ID by servo console? [Peichao:] your mean, DUT will bring up whatever the SKU ID is 1 or 2?
2. Considering to EVT, we still need to evaluate to boot up by 255. [Peichao:] Sure, this CL has included this logic, we can bring up normally from eMMC or SSD when SKU ID is 255
Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35204 )
Change subject: [TEST-ONLY] Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
Patch Set 4:
Patch Set 4:
- Even for proto stage, the SKU ID 255 still needs to be ensure that board can be power on generally; otherwise everyone got this board needs to re-program SKU ID by servo console?
[Peichao:] your mean, DUT will bring up whatever the SKU ID is 1 or 2?
DUT should be able to boot up from user side who gets this proto board with eMMC or SSD mounted.
- Considering to EVT, we still need to evaluate to boot up by 255.
[Peichao:] Sure, this CL has included this logic, we can bring up normally from eMMC or SSD when SKU ID is 255
If SKU ID is 255, the gpio.c doesn't set eMMC pins correct at least?
Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35204 )
Change subject: [TEST-ONLY] Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
Patch Set 4:
Patch Set 4: DUT should be able to boot up from user side who gets this proto board with eMMC or SSD mounted.
- Considering to EVT, we still need to evaluate to boot up by 255.
[Peichao:] Sure, this CL has included this logic, we can bring up normally from eMMC or SSD when SKU ID is 255
If SKU ID is 255, the gpio.c doesn't set eMMC pins correct at least?
Has the offline chat with paichao about: 1. Although the eMMC GPIO NF setting is removed from early_gpio_table but it is originally in the gpio_table already so 255 can be with eMMC config from gpio_table.
Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35204 )
Change subject: [TEST-ONLY] Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
Patch Set 4: Code-Review+1
Peichao Li has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35204 )
Change subject: [TEST-ONLY] Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
Patch Set 4:
Patch Set 4:
Patch Set 4: DUT should be able to boot up from user side who gets this proto board with eMMC or SSD mounted.
- Considering to EVT, we still need to evaluate to boot up by 255.
[Peichao:] Sure, this CL has included this logic, we can bring up normally from eMMC or SSD when SKU ID is 255
If SKU ID is 255, the gpio.c doesn't set eMMC pins correct at least?
Has the offline chat with paichao about:
- Although the eMMC GPIO NF setting is removed from early_gpio_table but it is originally in the gpio_table already so 255 can be with eMMC config from gpio_table.
Dear Marco, as we discussion, CBI pre-flash keep apply SKU ID 255 for Proto build, we will do CBI update and verification when Proto board back to Shenzhen office, we also update status to Google team. Thanks a lot!
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35204 )
Change subject: [TEST-ONLY] Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35204/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35204/4//COMMIT_MSG@12 PS4, Line 12: 132918661 This doesn't look like the right bug#.
Hello Marco Chen, Philip Chen, Tim Wawrzynczak, Shelley Chen, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35204
to look at the new patch set (#5).
Change subject: [TEST-ONLY] Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
[TEST-ONLY] Distinguish SKU1 and 2 for eMMC and SSD respectively
1. SKU1 for eMMC 2. SKU2 for SSD
BUG=None TEST=Verify SSD is disabled when SKU ID = 2/4/21/22
Signed-off-by: Peichao Wang peichao.wang@bitland.corp-partner.google.com Change-Id: I827e6f1420801d43e0eb4708b8b8ad1692ef7e9f --- M src/mainboard/google/hatch/variants/akemi/Makefile.inc M src/mainboard/google/hatch/variants/akemi/gpio.c A src/mainboard/google/hatch/variants/akemi/variant.c 3 files changed, 204 insertions(+), 24 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/35204/5
Peichao Li has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35204 )
Change subject: [TEST-ONLY] Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35204/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35204/4//COMMIT_MSG@12 PS4, Line 12: 132918661
This doesn't look like the right bug#.
At this time, there is no bug, so remove it and this cl is a feature request, I will create a new ticket when EVT stage.
Hello Marco Chen, Philip Chen, Tim Wawrzynczak, Shelley Chen, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35204
to look at the new patch set (#6).
Change subject: mb/google/hatch: Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
mb/google/hatch: Distinguish SKU1 and 2 for eMMC and SSD respectively
1. SKU1 for eMMC 2. SKU2 for SSD
BUG=b:140008849, b:140573677 TEST=Verify SSD is disabled when SKU ID = 2/4/21/22
Signed-off-by: Peichao Wang peichao.wang@bitland.corp-partner.google.com Change-Id: I827e6f1420801d43e0eb4708b8b8ad1692ef7e9f --- M src/mainboard/google/hatch/variants/akemi/Makefile.inc M src/mainboard/google/hatch/variants/akemi/gpio.c A src/mainboard/google/hatch/variants/akemi/variant.c 3 files changed, 204 insertions(+), 24 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/35204/6
Peichao Li has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35204 )
Change subject: mb/google/hatch: Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
Patch Set 6: Code-Review+1
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35204 )
Change subject: mb/google/hatch: Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
Patch Set 6: Code-Review+2
Peichao Li has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35204 )
Change subject: mb/google/hatch: Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
Patch Set 6: Code-Review+2
Martin Roth has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35204 )
Change subject: mb/google/hatch: Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
mb/google/hatch: Distinguish SKU1 and 2 for eMMC and SSD respectively
1. SKU1 for eMMC 2. SKU2 for SSD
BUG=b:140008849, b:140573677 TEST=Verify SSD is disabled when SKU ID = 2/4/21/22
Signed-off-by: Peichao Wang peichao.wang@bitland.corp-partner.google.com Change-Id: I827e6f1420801d43e0eb4708b8b8ad1692ef7e9f Reviewed-on: https://review.coreboot.org/c/coreboot/+/35204 Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org Reviewed-by: Marco Chen marcochen@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/mainboard/google/hatch/variants/akemi/Makefile.inc M src/mainboard/google/hatch/variants/akemi/gpio.c A src/mainboard/google/hatch/variants/akemi/variant.c 3 files changed, 204 insertions(+), 24 deletions(-)
Approvals: build bot (Jenkins): Verified Marco Chen: Looks good to me, but someone else must approve Peichao Li: Looks good to me, approved Tim Wawrzynczak: Looks good to me, approved
diff --git a/src/mainboard/google/hatch/variants/akemi/Makefile.inc b/src/mainboard/google/hatch/variants/akemi/Makefile.inc index 2f590bf..6cabe91 100644 --- a/src/mainboard/google/hatch/variants/akemi/Makefile.inc +++ b/src/mainboard/google/hatch/variants/akemi/Makefile.inc @@ -21,3 +21,4 @@
bootblock-y += gpio.c ramstage-y += gpio.c +ramstage-y += variant.c diff --git a/src/mainboard/google/hatch/variants/akemi/gpio.c b/src/mainboard/google/hatch/variants/akemi/gpio.c index 5a58839..27b49e5 100644 --- a/src/mainboard/google/hatch/variants/akemi/gpio.c +++ b/src/mainboard/google/hatch/variants/akemi/gpio.c @@ -19,6 +19,148 @@ #include <commonlib/helpers.h> #include <console/console.h>
+static const struct pad_config ssd_sku_gpio_table[] = { + /* A0 : NC */ + PAD_NC(GPP_A0, NONE), + /* A6 : NC */ + PAD_NC(GPP_A6, NONE), + /* A8 : NC */ + PAD_NC(GPP_A8, NONE), + /* A10 : NC */ + PAD_NC(GPP_A10, NONE), + /* A11 : NC */ + PAD_NC(GPP_A11, NONE), + /* A12 : NC */ + PAD_NC(GPP_A12, NONE), + /* A18 : NC */ + PAD_NC(GPP_A18, NONE), + /* A19 : NC */ + PAD_NC(GPP_A19, NONE), + /* A22 : NC */ + PAD_NC(GPP_A22, NONE), + /* A23 : NC */ + PAD_NC(GPP_A23, NONE), + /* B20 : NC */ + PAD_NC(GPP_B20, NONE), + /* B21 : NC */ + PAD_NC(GPP_B21, NONE), + /* B22 : NC */ + PAD_NC(GPP_B22, NONE), + /* C11 : NC */ + PAD_NC(GPP_C11, NONE), + /* C15 : NC */ + PAD_NC(GPP_C15, NONE), + /* F1 : NC */ + PAD_NC(GPP_F1, NONE), + /* F3 : MEM_STRAP_3 */ + PAD_CFG_GPI(GPP_F3, NONE, PLTRST), + /* F10 : MEM_STRAP_2 */ + PAD_CFG_GPI(GPP_F10, NONE, PLTRST), + /* F11 : NC */ + PAD_NC(GPP_F11, NONE), + /* F12 : NC */ + PAD_NC(GPP_F12, NONE), + /* F13 : NC */ + PAD_NC(GPP_F13, NONE), + /* F14 : NC */ + PAD_NC(GPP_F14, NONE), + /* F15 : NC */ + PAD_NC(GPP_F15, NONE), + /* F16 : NC */ + PAD_NC(GPP_F16, NONE), + /* F17 : NC */ + PAD_NC(GPP_F17, NONE), + /* F18 : NC */ + PAD_NC(GPP_F18, NONE), + /* F19 : NC */ + PAD_NC(GPP_F19, NONE), + /* F20 : NC */ + PAD_NC(GPP_F20, NONE), + /* F21 : NC */ + PAD_NC(GPP_F21, NONE), + /* F22 : NC */ + PAD_NC(GPP_F22, NONE), + /* H6 : NC */ + PAD_NC(GPP_H6, NONE), + /* H7 : NC */ + PAD_NC(GPP_H7, NONE), + /* H19 : MEM_STRAP_0 */ + PAD_CFG_GPI(GPP_H19, NONE, PLTRST), + /* H22 : MEM_STRAP_1 */ + PAD_CFG_GPI(GPP_H22, NONE, PLTRST), +}; + +static const struct pad_config emmc_sku_gpio_table[] = { + /* A0 : NC */ + PAD_NC(GPP_A0, NONE), + /* A6 : NC */ + PAD_NC(GPP_A6, NONE), + /* A8 : NC */ + PAD_NC(GPP_A8, NONE), + /* A10 : NC */ + PAD_NC(GPP_A10, NONE), + /* A11 : NC */ + PAD_NC(GPP_A11, NONE), + /* A12 : NC */ + PAD_NC(GPP_A12, NONE), + /* A18 : NC */ + PAD_NC(GPP_A18, NONE), + /* A19 : NC */ + PAD_NC(GPP_A19, NONE), + /* A22 : NC */ + PAD_NC(GPP_A22, NONE), + /* A23 : NC */ + PAD_NC(GPP_A23, NONE), + /* B20 : NC */ + PAD_NC(GPP_B20, NONE), + /* B21 : NC */ + PAD_NC(GPP_B21, NONE), + /* B22 : NC */ + PAD_NC(GPP_B22, NONE), + /* C11 : NC */ + PAD_NC(GPP_C11, NONE), + /* C15 : NC */ + PAD_NC(GPP_C15, NONE), + /* F1 : NC */ + PAD_NC(GPP_F1, NONE), + /* F3 : MEM_STRAP_3 */ + PAD_CFG_GPI(GPP_F3, NONE, PLTRST), + /* F10 : MEM_STRAP_2 */ + PAD_CFG_GPI(GPP_F10, NONE, PLTRST), + /* F11 : EMMC_CMD ==> EMMC_CMD */ + PAD_CFG_NF(GPP_F11, NONE, DEEP, NF1), + /* F12 : EMMC_DATA0 ==> EMMC_DAT0 */ + PAD_CFG_NF(GPP_F12, NONE, DEEP, NF1), + /* F13 : EMMC_DATA1 ==> EMMC_DAT1 */ + PAD_CFG_NF(GPP_F13, NONE, DEEP, NF1), + /* F14 : EMMC_DATA2 ==> EMMC_DAT2 */ + PAD_CFG_NF(GPP_F14, NONE, DEEP, NF1), + /* F15 : EMMC_DATA3 ==> EMMC_DAT3 */ + PAD_CFG_NF(GPP_F15, NONE, DEEP, NF1), + /* F16 : EMMC_DATA4 ==> EMMC_DAT4 */ + PAD_CFG_NF(GPP_F16, NONE, DEEP, NF1), + /* F17 : EMMC_DATA5 ==> EMMC_DAT5 */ + PAD_CFG_NF(GPP_F17, NONE, DEEP, NF1), + /* F18 : EMMC_DATA6 ==> EMMC_DAT6 */ + PAD_CFG_NF(GPP_F18, NONE, DEEP, NF1), + /* F19 : EMMC_DATA7 ==> EMMC_DAT7 */ + PAD_CFG_NF(GPP_F19, NONE, DEEP, NF1), + /* F20 : EMMC_RCLK ==> EMMC_RCLK */ + PAD_CFG_NF(GPP_F20, NONE, DEEP, NF1), + /* F21 : EMMC_CLK ==> EMMC_CLK */ + PAD_CFG_NF(GPP_F21, NONE, DEEP, NF1), + /* F22 : EMMC_RESET# ==> EMMC_RST_L */ + PAD_CFG_NF(GPP_F22, NONE, DEEP, NF1), + /* H6 : NC */ + PAD_NC(GPP_H6, NONE), + /* H7 : NC */ + PAD_NC(GPP_H7, NONE), + /* H19 : MEM_STRAP_0 */ + PAD_CFG_GPI(GPP_H19, NONE, PLTRST), + /* H22 : MEM_STRAP_1 */ + PAD_CFG_GPI(GPP_H22, NONE, PLTRST), +}; + static const struct pad_config gpio_table[] = { /* A0 : NC */ PAD_NC(GPP_A0, NONE), @@ -92,6 +234,17 @@
const struct pad_config *override_gpio_table(size_t *num) { + uint32_t sku_id = get_board_sku(); + /* For SSD SKU */ + if (sku_id == 2) { + *num = ARRAY_SIZE(ssd_sku_gpio_table); + return ssd_sku_gpio_table; + } + /* For eMMC SKU */ + if (sku_id == 1) { + *num = ARRAY_SIZE(emmc_sku_gpio_table); + return emmc_sku_gpio_table; + } *num = ARRAY_SIZE(gpio_table); return gpio_table; } @@ -128,30 +281,6 @@ PAD_CFG_NF(GPP_E5, NONE, PLTRST, NF1), /* F2 : MEM_CH_SEL */ PAD_CFG_GPI(GPP_F2, NONE, PLTRST), - /* F11 : EMMC_CMD ==> EMMC_CMD */ - PAD_CFG_NF(GPP_F11, NONE, DEEP, NF1), - /* F12 : EMMC_DATA0 ==> EMMC_DAT0 */ - PAD_CFG_NF(GPP_F12, NONE, DEEP, NF1), - /* F13 : EMMC_DATA1 ==> EMMC_DAT1 */ - PAD_CFG_NF(GPP_F13, NONE, DEEP, NF1), - /* F14 : EMMC_DATA2 ==> EMMC_DAT2 */ - PAD_CFG_NF(GPP_F14, NONE, DEEP, NF1), - /* F15 : EMMC_DATA3 ==> EMMC_DAT3 */ - PAD_CFG_NF(GPP_F15, NONE, DEEP, NF1), - /* F16 : EMMC_DATA4 ==> EMMC_DAT4 */ - PAD_CFG_NF(GPP_F16, NONE, DEEP, NF1), - /* F17 : EMMC_DATA5 ==> EMMC_DAT5 */ - PAD_CFG_NF(GPP_F17, NONE, DEEP, NF1), - /* F18 : EMMC_DATA6 ==> EMMC_DAT6 */ - PAD_CFG_NF(GPP_F18, NONE, DEEP, NF1), - /* F19 : EMMC_DATA7 ==> EMMC_DAT7 */ - PAD_CFG_NF(GPP_F19, NONE, DEEP, NF1), - /* F20 : EMMC_RCLK ==> EMMC_RCLK */ - PAD_CFG_NF(GPP_F20, NONE, DEEP, NF1), - /* F21 : EMMC_CLK ==> EMMC_CLK */ - PAD_CFG_NF(GPP_F21, NONE, DEEP, NF1), - /* F22 : EMMC_RESET# ==> EMMC_RST_L */ - PAD_CFG_NF(GPP_F22, NONE, DEEP, NF1), };
const struct pad_config *variant_early_gpio_table(size_t *num) diff --git a/src/mainboard/google/hatch/variants/akemi/variant.c b/src/mainboard/google/hatch/variants/akemi/variant.c new file mode 100644 index 0000000..0717e81 --- /dev/null +++ b/src/mainboard/google/hatch/variants/akemi/variant.c @@ -0,0 +1,50 @@ +/* + * This file is part of the coreboot project. + * + * Copyright 2019 Google LLC + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <baseboard/variants.h> +#include <chip.h> +#include <soc/pci_devs.h> +#include <ec/google/chromeec/ec.h> + +void variant_devtree_update(void) +{ + uint32_t sku_id; + struct device *emmc_host; + struct device *ssd_host; + config_t *cfg = config_of_path(SA_DEVFN_ROOT); + emmc_host = pcidev_path_on_root(PCH_DEVFN_EMMC); + ssd_host = pcidev_path_on_root(PCH_DEVFN_SATA); + + /* SKU ID 2 doesn't have a eMMC device, hence disable it. */ + sku_id = get_board_sku(); + if (sku_id == 2) { + if (emmc_host == NULL) + return; + emmc_host->enabled = 0; + cfg->ScsEmmcHs400Enabled = 0; + } + + /* SKU ID 1 doesn't have a SSD device, hence disable it. */ + if (sku_id == 1) { + if (ssd_host == NULL) + return; + ssd_host->enabled = 0; + cfg->SataSalpSupport = 0; + cfg->SataMode = 0; + cfg->SataPortsEnable[1] = 0; + cfg->SataPortsDevSlp[1] = 0; + cfg->satapwroptimize = 0; + } +}
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35204 )
Change subject: mb/google/hatch: Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35204/7/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/akemi/gpio.c:
https://review.coreboot.org/c/coreboot/+/35204/7/src/mainboard/google/hatch/... PS7, Line 238: /* For SSD SKU */ What if SKU ID is 255? Don't you need to configure both SSD and eMMC GPIOs?
Peichao Li has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35204 )
Change subject: mb/google/hatch: Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35204/7/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/akemi/gpio.c:
https://review.coreboot.org/c/coreboot/+/35204/7/src/mainboard/google/hatch/... PS7, Line 238: /* For SSD SKU */
What if SKU ID is 255? Don't you need to configure both SSD and eMMC GPIOs?
Dear Furquan, SKU ID 255 is default, DUT could bring up from eMMC and SSD, I have do the experiment for configuring SKU ID 1, 2 and 255 in the CBI on my side DUT, it could bring up normally form eMMC and SSD.
Peichao Li has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35204 )
Change subject: mb/google/hatch: Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
Patch Set 7:
Dear Furquan,
Default eMMC and PCI host controller is enable, so don't need configure them when sku id is 255. Because in the overridetree.cb under the Akmei, node pci 1a.0 point to eMMC and it is enable, In addition devicetree.cb under the baseboard, the PCI host controller default is enable. Please kindly review.
Thanks and best regards
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35204 )
Change subject: mb/google/hatch: Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
Patch Set 7:
Patch Set 7:
Dear Furquan,
Default eMMC and PCI host controller is enable, so don't need configure them when sku id is 255. Because in the overridetree.cb under the Akmei, node pci 1a.0 point to eMMC and it is enable, In addition devicetree.cb under the baseboard, the PCI host controller default is enable. Please kindly review.
Thanks and best regards
PCI devices are enabled, but what about the GPIO configurations? Don't you need to ensure that the GPIOs are configured correctly too?
Peichao Li has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35204 )
Change subject: mb/google/hatch: Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
Patch Set 7:
Patch Set 7:
Patch Set 7:
Dear Furquan,
Default eMMC and PCI host controller is enable, so don't need configure them when sku id is 255. Because in the overridetree.cb under the Akmei, node pci 1a.0 point to eMMC and it is enable, In addition devicetree.cb under the baseboard, the PCI host controller default is enable. Please kindly review.
Thanks and best regards
PCI devices are enabled, but what about the GPIO configurations? Don't you need to ensure that the GPIOs are configured correctly too?
Dear Furquan,
You could take look https://review.coreboot.org/c/coreboot/+/35204/7/src/mainboard/google/hatch/... about default gpio configuration.
Thanks and best regards
Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35204 )
Change subject: mb/google/hatch: Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
Patch Set 7:
Hi Furquan, we have the similar discussion there - https://review.coreboot.org/c/coreboot/+/35204/7#message-f04d3f9880853748220... .
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35204 )
Change subject: mb/google/hatch: Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
Patch Set 7:
Patch Set 7:
Hi Furquan, we have the similar discussion there - https://review.coreboot.org/c/coreboot/+/35204/7#message-f04d3f9880853748220... .
Ah thanks for the pointer Marco.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35204 )
Change subject: mb/google/hatch: Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35204/7/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/akemi/gpio.c:
https://review.coreboot.org/c/coreboot/+/35204/7/src/mainboard/google/hatch/... PS7, Line 238: /* For SSD SKU */
Dear Furquan, SKU ID 255 is default, DUT could bring up from eMMC and SSD, I have do the experiment […]
Looks like emmc_sku_gpio_table and gpio_table are basically copies of each other. Why not just have one? Or are you expecting changes in future which will go in gpio_table but not in emmc_sku_gpio_table or the other way round?
Peichao Li has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35204 )
Change subject: mb/google/hatch: Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35204/7/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/akemi/gpio.c:
https://review.coreboot.org/c/coreboot/+/35204/7/src/mainboard/google/hatch/... PS7, Line 238: /* For SSD SKU */
Looks like emmc_sku_gpio_table and gpio_table are basically copies of each other. […]
Catch your point, I think your mention is make sense, let me merge emmc_sku_gpio_table and gpio_table to one gpio_table, Do you think. Thanks a lot!
Peichao Li has created a revert of this change. ( https://review.coreboot.org/c/coreboot/+/35204 )
Change subject: mb/google/hatch: Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
Peichao Li has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35204 )
Change subject: mb/google/hatch: Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35204/7/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/akemi/gpio.c:
https://review.coreboot.org/c/coreboot/+/35204/7/src/mainboard/google/hatch/... PS7, Line 238: /* For SSD SKU */
Catch your point, I think your mention is make sense, let me merge emmc_sku_gpio_table and gpio_tabl […]
I have uprev the new CL for emmc table and default gpio table combination, please kindly check: https://review.coreboot.org/c/coreboot/+/35344
Philip Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35204 )
Change subject: mb/google/hatch: Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
Patch Set 7:
The CL is not merged yet, so there is no point to revert. If you don't need this CL anymore, please abandon it. Thanks.
Philip Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35204 )
Change subject: mb/google/hatch: Distinguish SKU1 and 2 for eMMC and SSD respectively ......................................................................
Patch Set 7:
Patch Set 7:
The CL is not merged yet, so there is no point to revert. If you don't need this CL anymore, please abandon it. Thanks.
Ah, sorry, I saw it was really merged way before those comments were made. So yes, please revert it.
-
build bot (Jenkins) (Code Review) -
Furquan Shaikh (Code Review)
-
Marco Chen (Code Review)
-
Martin Roth (Code Review)
-
Peichao Li (Code Review)
-
Philip Chen (Code Review)
-
Tim Wawrzynczak (Code Review)