Change in coreboot[master]: mb/google/hatch/var/kindred: Config GPIOs and disable eMMC for SSD SKUs
David Wu has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34788 )
Change subject: mb/google/hatch/var/kindred: Config GPIOs and disable eMMC for SSD SKUs ......................................................................
mb/google/hatch/var/kindred: Config GPIOs and disable eMMC for SSD SKUs
1. Config GPIOs for SSD SKUs 2. Disable eMMC controller for new SKU ID 23 and 24 3. Disable HS400 mode
BUG=b:132918661 TEST=Verify eMMC is disabled when SKU ID = 1/3/23/24
Change-Id: I0d893f0f7339e7b1a1e6b56d1598c0a361c8d604 Signed-off-by: David Wu david_wu@quanta.corp-partner.google.com --- M src/mainboard/google/hatch/ramstage.c M src/mainboard/google/hatch/variants/baseboard/gpio.c M src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/hatch/variants/kindred/gpio.c M src/mainboard/google/hatch/variants/kindred/variant.c 5 files changed, 108 insertions(+), 27 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/34788/1
diff --git a/src/mainboard/google/hatch/ramstage.c b/src/mainboard/google/hatch/ramstage.c index 04e1bc1..2e94b0f 100644 --- a/src/mainboard/google/hatch/ramstage.c +++ b/src/mainboard/google/hatch/ramstage.c @@ -25,17 +25,21 @@ { const struct pad_config *base_table; const struct pad_config *override_table; + const struct pad_config *override_sku_table; size_t base_gpios; size_t override_gpios; + size_t override_sku_gpios;
variant_devtree_update(); base_table = base_gpio_table(&base_gpios); override_table = override_gpio_table(&override_gpios); + override_sku_table = variant_sku_gpio_table(&override_sku_gpios);
gpio_configure_pads_with_override(base_table, base_gpios, override_table, override_gpios); + gpio_configure_pads(override_sku_table, override_sku_gpios); }
void __weak variant_devtree_update(void) diff --git a/src/mainboard/google/hatch/variants/baseboard/gpio.c b/src/mainboard/google/hatch/variants/baseboard/gpio.c index 38d44f3..943f6c1 100644 --- a/src/mainboard/google/hatch/variants/baseboard/gpio.c +++ b/src/mainboard/google/hatch/variants/baseboard/gpio.c @@ -491,3 +491,10 @@ *num = 0; return NULL; } + +/* override specific gpio by sku id */ +const struct pad_config *__weak variant_sku_gpio_table(size_t *num) +{ + *num = 0; + return NULL; +} diff --git a/src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h b/src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h index 71a2362..a99ef57 100644 --- a/src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h +++ b/src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h @@ -43,6 +43,9 @@ /* Return ChromeOS gpio table and fill in number of entries. */ const struct cros_gpio *variant_cros_gpios(size_t *num);
+/* Config gpio by different sku id */ +const struct pad_config *variant_sku_gpio_table(size_t *num); + /* Return board SKU */ uint32_t get_board_sku(void);
diff --git a/src/mainboard/google/hatch/variants/kindred/gpio.c b/src/mainboard/google/hatch/variants/kindred/gpio.c index f6aeb69..86db690 100644 --- a/src/mainboard/google/hatch/variants/kindred/gpio.c +++ b/src/mainboard/google/hatch/variants/kindred/gpio.c @@ -23,30 +23,6 @@ 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), /* H19 : MEM_STRAP_0 */ PAD_CFG_GPI(GPP_H19, NONE, PLTRST), /* H22 : MEM_STRAP_1 */ @@ -76,3 +52,91 @@ *num = ARRAY_SIZE(early_gpio_table); return early_gpio_table; } + +/* Config GPIOs for SSD SKUs */ +static const struct pad_config ssd_sku_gpio_table[] = { + /* F11 : EMMC_CMD */ + PAD_NC(GPP_F11, NONE), + /* F12 : EMMC_DATA0 */ + PAD_NC(GPP_F12, NONE), + /* F13 : EMMC_DATA1 */ + PAD_NC(GPP_F13, NONE), + /* F14 : EMMC_DATA2 */ + PAD_NC(GPP_F14, NONE), + /* F15 : EMMC_DATA3 */ + PAD_NC(GPP_F15, NONE), + /* F16 : EMMC_DATA4 */ + PAD_NC(GPP_F16, NONE), + /* F17 : EMMC_DATA5 */ + PAD_NC(GPP_F17, NONE), + /* F18 : EMMC_DATA6 */ + PAD_NC(GPP_F18, NONE), + /* F19 : EMMC_DATA7 */ + PAD_NC(GPP_F19, NONE), + /* F20 : EMMC_RCLK */ + PAD_NC(GPP_F20, NONE), + /* F21 : EMMC_CLK */ + PAD_NC(GPP_F21, NONE), + /* F22 : EMMC_RESET# */ + PAD_NC(GPP_F22, NONE), +}; + +static const struct pad_config default_sku_gpio_table[] = { + /* 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_sku_gpio_table(size_t *num) +{ + uint32_t sku_id = get_board_sku(); + const struct pad_config *board_gpio_tables; + switch (sku_id) { + case 1: + *num = ARRAY_SIZE(ssd_sku_gpio_table); + board_gpio_tables = ssd_sku_gpio_table; + break; + case 2: + case 3: + *num = ARRAY_SIZE(ssd_sku_gpio_table); + board_gpio_tables = ssd_sku_gpio_table; + break; + case 4: + case 21: + case 22: + case 23: + *num = ARRAY_SIZE(ssd_sku_gpio_table); + board_gpio_tables = ssd_sku_gpio_table; + break; + case 24: + *num = ARRAY_SIZE(ssd_sku_gpio_table); + board_gpio_tables = ssd_sku_gpio_table; + break; + default: + *num = ARRAY_SIZE(default_sku_gpio_table); + board_gpio_tables = default_sku_gpio_table; + break; + } + return board_gpio_tables; +} diff --git a/src/mainboard/google/hatch/variants/kindred/variant.c b/src/mainboard/google/hatch/variants/kindred/variant.c index 14b26ed..5405947 100644 --- a/src/mainboard/google/hatch/variants/kindred/variant.c +++ b/src/mainboard/google/hatch/variants/kindred/variant.c @@ -14,6 +14,7 @@ */
#include <baseboard/variants.h> +#include <chip.h> #include <soc/pci_devs.h> #include <ec/google/chromeec/ec.h>
@@ -21,14 +22,16 @@ { uint32_t sku_id; struct device *emmc_host; - + config_t *cfg = config_of_path(SA_DEVFN_ROOT); emmc_host = pcidev_path_on_root(PCH_DEVFN_EMMC);
if (emmc_host == NULL) return;
- /* SKU ID 1, 3 doesn't have a eMMC device, hence disable it. */ + /* SKU ID 1/3/23/24 doesn't have a eMMC device, hence disable it. */ sku_id = get_board_sku(); - if (sku_id == 1 || sku_id == 3) + if (sku_id == 1 || sku_id == 3 || sku_id == 23 || sku_id == 24) { emmc_host->enabled = 0; + cfg->ScsEmmcHs400Enabled = 0; + } }
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34788 )
Change subject: mb/google/hatch/var/kindred: Config GPIOs and disable eMMC for SSD SKUs ......................................................................
Patch Set 1:
Please rebase on top of https://review.coreboot.org/c/coreboot/+/34782
Hello Paul Fagerburg, Tim Wawrzynczak, Philip Chen, Shelley Chen, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34788
to look at the new patch set (#2).
Change subject: mb/google/hatch/var/kindred: Config GPIOs and disable eMMC for SSD SKUs ......................................................................
mb/google/hatch/var/kindred: Config GPIOs and disable eMMC for SSD SKUs
1. Config GPIOs for SSD SKUs 2. Disable eMMC controller for new SKU ID 23 and 24 3. Disable HS400 mode
BUG=b:132918661 TEST=Verify eMMC is disabled when SKU ID = 1/3/23/24
Change-Id: I0d893f0f7339e7b1a1e6b56d1598c0a361c8d604 Signed-off-by: David Wu david_wu@quanta.corp-partner.google.com --- M src/mainboard/google/hatch/ramstage.c M src/mainboard/google/hatch/variants/baseboard/gpio.c M src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/hatch/variants/kindred/gpio.c M src/mainboard/google/hatch/variants/kindred/variant.c 5 files changed, 108 insertions(+), 27 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/34788/2
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34788 )
Change subject: mb/google/hatch/var/kindred: Config GPIOs and disable eMMC for SSD SKUs ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34788/2/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/kindred/gpio.c:
https://review.coreboot.org/c/coreboot/+/34788/2/src/mainboard/google/hatch/... PS2, Line 137: switch (sku_id) { All of the cases (except for default) have the same body - they all use ssd_sku_gpio_table. In https://review.coreboot.org/c/coreboot/+/34789, the comments in variant.c say "SKU ID 2/4/21/22 doesn't have a SSD device, hence disable it." So why are the SSD GPIOs being used for those SKUs? If the SKUs are being grouped this way because there will be additional statements in the bodies in the future, you should note that in some comments. Otherwise, all of the case labels with the same body should be together with a single body.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34788 )
Change subject: mb/google/hatch/var/kindred: Config GPIOs and disable eMMC for SSD SKUs ......................................................................
Patch Set 2:
(1 comment)
Please split this up in separate commits.
https://review.coreboot.org/c/coreboot/+/34788/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34788/2//COMMIT_MSG@7 PS2, Line 7: Config Configure
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34788 )
Change subject: mb/google/hatch/var/kindred: Config GPIOs and disable eMMC for SSD SKUs ......................................................................
Patch Set 2: Code-Review-1
I don't see the need to introduce yet another way to configure GPIOs. This can all be done from Kindred's override_gpio_table(). If you get the SKU ID inside that function, you can return the table that is appropriate for that SKU.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34788 )
Change subject: mb/google/hatch/var/kindred: Config GPIOs and disable eMMC for SSD SKUs ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34788/2/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/kindred/gpio.c:
https://review.coreboot.org/c/coreboot/+/34788/2/src/mainboard/google/hatch/... PS2, Line 137: switch (sku_id) {
All of the cases (except for default) have the same body - they all use ssd_sku_gpio_table. […]
Agreed, for this commit, please keep all identical cases together; it's also appreciated to add a nice big comment as to why all the case statements are falling through together.
Hello Paul Fagerburg, Tim Wawrzynczak, Philip Chen, Tim Wawrzynczak, Shelley Chen, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34788
to look at the new patch set (#3).
Change subject: mb/google/hatch/var/kindred: Disable eMMC for new SKU ID 23 and 24 ......................................................................
mb/google/hatch/var/kindred: Disable eMMC for new SKU ID 23 and 24
1. Disable eMMC controller for new SKU ID 23 and 24 2. Disable HS400 mode
BUG=b:132918661 TEST=Verify eMMC is disabled when SKU ID = 1/3/23/24
Change-Id: I0d893f0f7339e7b1a1e6b56d1598c0a361c8d604 Signed-off-by: David Wu david_wu@quanta.corp-partner.google.com --- M src/mainboard/google/hatch/variants/kindred/variant.c 1 file changed, 6 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/34788/3
David Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34788 )
Change subject: mb/google/hatch/var/kindred: Disable eMMC for new SKU ID 23 and 24 ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34788/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34788/2//COMMIT_MSG@7 PS2, Line 7: Config
Configure
Done
https://review.coreboot.org/c/coreboot/+/34788/2/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/kindred/gpio.c:
https://review.coreboot.org/c/coreboot/+/34788/2/src/mainboard/google/hatch/... PS2, Line 137: switch (sku_id) {
Agreed, for this commit, please keep all identical cases together; it's also appreciated to add a ni […]
Move GPIO configure to https://review.coreboot.org/c/coreboot/+/34851/, please help to review. Thanks.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34788 )
Change subject: mb/google/hatch/var/kindred: Disable eMMC for new SKU ID 23 and 24 ......................................................................
Patch Set 3: Code-Review+2
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34788 )
Change subject: mb/google/hatch/var/kindred: Disable eMMC for new SKU ID 23 and 24 ......................................................................
Patch Set 3: Code-Review+2
David Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34788 )
Change subject: mb/google/hatch/var/kindred: Disable eMMC for new SKU ID 23 and 24 ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34788/2/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/kindred/gpio.c:
https://review.coreboot.org/c/coreboot/+/34788/2/src/mainboard/google/hatch/... PS2, Line 137: switch (sku_id) {
Move GPIO configure to https://review.coreboot.org/c/coreboot/+/34851/, please help to review. […]
Done
Furquan Shaikh has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34788 )
Change subject: mb/google/hatch/var/kindred: Disable eMMC for new SKU ID 23 and 24 ......................................................................
mb/google/hatch/var/kindred: Disable eMMC for new SKU ID 23 and 24
1. Disable eMMC controller for new SKU ID 23 and 24 2. Disable HS400 mode
BUG=b:132918661 TEST=Verify eMMC is disabled when SKU ID = 1/3/23/24
Change-Id: I0d893f0f7339e7b1a1e6b56d1598c0a361c8d604 Signed-off-by: David Wu david_wu@quanta.corp-partner.google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/34788 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org Reviewed-by: Paul Fagerburg pfagerburg@chromium.org --- M src/mainboard/google/hatch/variants/kindred/variant.c 1 file changed, 6 insertions(+), 3 deletions(-)
Approvals: build bot (Jenkins): Verified Tim Wawrzynczak: Looks good to me, approved Paul Fagerburg: Looks good to me, approved
diff --git a/src/mainboard/google/hatch/variants/kindred/variant.c b/src/mainboard/google/hatch/variants/kindred/variant.c index 14b26ed..5405947 100644 --- a/src/mainboard/google/hatch/variants/kindred/variant.c +++ b/src/mainboard/google/hatch/variants/kindred/variant.c @@ -14,6 +14,7 @@ */
#include <baseboard/variants.h> +#include <chip.h> #include <soc/pci_devs.h> #include <ec/google/chromeec/ec.h>
@@ -21,14 +22,16 @@ { uint32_t sku_id; struct device *emmc_host; - + config_t *cfg = config_of_path(SA_DEVFN_ROOT); emmc_host = pcidev_path_on_root(PCH_DEVFN_EMMC);
if (emmc_host == NULL) return;
- /* SKU ID 1, 3 doesn't have a eMMC device, hence disable it. */ + /* SKU ID 1/3/23/24 doesn't have a eMMC device, hence disable it. */ sku_id = get_board_sku(); - if (sku_id == 1 || sku_id == 3) + if (sku_id == 1 || sku_id == 3 || sku_id == 23 || sku_id == 24) { emmc_host->enabled = 0; + cfg->ScsEmmcHs400Enabled = 0; + } }
-
David Wu (Code Review) -
Furquan Shaikh (Code Review)
-
Paul Fagerburg (Code Review)
-
Paul Menzel (Code Review)
-
Tim Wawrzynczak (Code Review)