Jes Klinke has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/48223 )
Change subject: mb/google/volteer: ACPI nodes for volteer2_ti50 ......................................................................
mb/google/volteer: ACPI nodes for volteer2_ti50
Unique among the Volteer devices, the volteer2_ti50 variant connects to the TPM via I2C. This CL introduces the proper devicestree declarations for the Linux kernel to recognize that.
overridetree.cb is shared between "sub"-variants volteer2 and volteer2_ti50, so both will have two TPM nodes, the I2C being disabled by default. The odd _ti50 variant then has code in variant.c to enable the I2C node and disable the SPI node.
BUG=b:173461736 TEST=abuild -t GOOGLE_VOLTEER2{_TI50,} -c max -x
Change-Id: I5576a595bbabc34c62b768f8b3439e35ff6bcf7b Signed-off-by: Jes Bodi Klinke jbk@chromium.org --- M src/mainboard/google/volteer/Kconfig.name M src/mainboard/google/volteer/mainboard.c M src/mainboard/google/volteer/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/volteer/variants/volteer2/Makefile.inc M src/mainboard/google/volteer/variants/volteer2/overridetree.cb A src/mainboard/google/volteer/variants/volteer2/variant.c 6 files changed, 57 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/48223/1
diff --git a/src/mainboard/google/volteer/Kconfig.name b/src/mainboard/google/volteer/Kconfig.name index 7536dc5..43304cb 100644 --- a/src/mainboard/google/volteer/Kconfig.name +++ b/src/mainboard/google/volteer/Kconfig.name @@ -61,6 +61,7 @@ select VARIANT_HAS_MIPI_CAMERA select SOC_INTEL_CSE_LITE_SKU select DRIVERS_GENESYSLOGIC_GL9755 + select DRIVER_I2C_TPM_ACPI
# Reworked Volteer2 prototype, Haven chip replaced with Dauntless demo board config BOARD_GOOGLE_VOLTEER2_TI50 @@ -69,6 +70,7 @@ select VARIANT_HAS_MIPI_CAMERA select SOC_INTEL_CSE_LITE_SKU select DRIVERS_GENESYSLOGIC_GL9755 + select DRIVER_I2C_TPM_ACPI
config BOARD_GOOGLE_VOXEL bool "-> Voxel" diff --git a/src/mainboard/google/volteer/mainboard.c b/src/mainboard/google/volteer/mainboard.c index ea9e08f..1f20e18 100644 --- a/src/mainboard/google/volteer/mainboard.c +++ b/src/mainboard/google/volteer/mainboard.c @@ -23,8 +23,8 @@
static bool is_port1(struct device *dev) { - return dev->path.type == DEVICE_PATH_GENERIC && dev->path.generic.id == 1 && - dev->chip_ops == &drivers_intel_pmc_mux_conn_ops; + return dev->path.type == DEVICE_PATH_GENERIC && dev->path.generic.id == 1 + && dev->chip_ops == &drivers_intel_pmc_mux_conn_ops; }
static void typec_orientation_fixup(void) @@ -53,14 +53,15 @@ if (!conn) return;
- if (fw_config_probe(FW_CONFIG(DB_USB, USB4_GEN2)) || - fw_config_probe(FW_CONFIG(DB_USB, USB3_ACTIVE)) || - fw_config_probe(FW_CONFIG(DB_USB, USB4_GEN3)) || - fw_config_probe(FW_CONFIG(DB_USB, USB3_NO_A))) { + if (fw_config_probe(FW_CONFIG(DB_USB, USB4_GEN2)) + || fw_config_probe(FW_CONFIG(DB_USB, USB3_ACTIVE)) + || fw_config_probe(FW_CONFIG(DB_USB, USB4_GEN3)) + || fw_config_probe(FW_CONFIG(DB_USB, USB3_NO_A))) { struct drivers_intel_pmc_mux_conn_config *config = conn->chip_info;
if (config) { - printk(BIOS_INFO, "Configure Right Type-C port orientation for retimer\n"); + printk(BIOS_INFO, + "Configure Right Type-C port orientation for retimer\n"); config->sbu_orientation = TYPEC_ORIENTATION_NORMAL; } } @@ -70,6 +71,11 @@ { mainboard_ec_init(); typec_orientation_fixup(); + variant_devtree_update(); +} + +void __weak variant_devtree_update(void) +{ }
static void add_fw_config_oem_string(const struct fw_config *config, void *arg) diff --git a/src/mainboard/google/volteer/variants/baseboard/include/baseboard/variants.h b/src/mainboard/google/volteer/variants/baseboard/include/baseboard/variants.h index 8408198..4d5dc87 100644 --- a/src/mainboard/google/volteer/variants/baseboard/include/baseboard/variants.h +++ b/src/mainboard/google/volteer/variants/baseboard/include/baseboard/variants.h @@ -21,4 +21,7 @@ const struct ddr_memory_cfg *variant_memory_params(void); int variant_memory_sku(void);
+/* Modify devictree settings during ramstage. */ +void variant_devtree_update(void); + #endif /* __BASEBOARD_VARIANTS_H__ */ diff --git a/src/mainboard/google/volteer/variants/volteer2/Makefile.inc b/src/mainboard/google/volteer/variants/volteer2/Makefile.inc index 13269db..04af3ae 100644 --- a/src/mainboard/google/volteer/variants/volteer2/Makefile.inc +++ b/src/mainboard/google/volteer/variants/volteer2/Makefile.inc @@ -3,3 +3,4 @@ bootblock-y += gpio.c
ramstage-y += gpio.c +ramstage-y += variant.c diff --git a/src/mainboard/google/volteer/variants/volteer2/overridetree.cb b/src/mainboard/google/volteer/variants/volteer2/overridetree.cb index a1012a6..916777c 100644 --- a/src/mainboard/google/volteer/variants/volteer2/overridetree.cb +++ b/src/mainboard/google/volteer/variants/volteer2/overridetree.cb @@ -201,6 +201,11 @@ register "key.label" = ""pen_eject"" device generic 0 on end end + chip drivers/i2c/tpm + register "hid" = ""GOOG0005"" + register "irq" = "ACPI_IRQ_EDGE_LOW(GPP_C21_IRQ)" + device i2c 50 off end + end end device ref i2c2 on chip drivers/i2c/sx9310 diff --git a/src/mainboard/google/volteer/variants/volteer2/variant.c b/src/mainboard/google/volteer/variants/volteer2/variant.c new file mode 100644 index 0000000..057bb8a --- /dev/null +++ b/src/mainboard/google/volteer/variants/volteer2/variant.c @@ -0,0 +1,33 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <baseboard/variants.h> +#include <soc/pci_devs.h> +#include <acpi/acpi_device.h> + +extern struct chip_operations drivers_i2c_tpm_ops; + +static bool match_i2c_tpm(DEVTREE_CONST struct device *dev) +{ + return dev->chip_ops == &drivers_i2c_tpm_ops; +} + +/* + * This function runs only on the volteer_ti50 variant, which has the GSC on a + * reworked I2C bus. + */ +static void devtree_enable_i2c_tpm(void) +{ + struct device *spi_tpm = pcidev_path_on_root(PCH_DEVFN_GSPI0)->link_list->children; + struct device *i2c_tpm = dev_find_matching_device_on_bus( + pcidev_path_on_root(PCH_DEVFN_I2C1)->link_list, match_i2c_tpm); + if (!i2c_tpm || !spi_tpm) + return; + spi_tpm->enabled = 0; + i2c_tpm->enabled = 1; +} + +void variant_devtree_update(void) +{ + if (CONFIG(MAINBOARD_HAS_I2C_TPM_CR50)) + devtree_enable_i2c_tpm(); +}
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Nick Vaccaro,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48223
to look at the new patch set (#3).
Change subject: mb/google/volteer: ACPI nodes for volteer2_ti50 ......................................................................
mb/google/volteer: ACPI nodes for volteer2_ti50
Unique among the Volteer devices, the volteer2_ti50 variant connects to the TPM via I2C. This CL introduces the proper devicestree declarations for the Linux kernel to recognize that.
overridetree.cb is shared between "sub"-variants volteer2 and volteer2_ti50, so both will have two TPM nodes, the I2C being disabled by default. The odd _ti50 variant then has code in variant.c to enable the I2C node and disable the SPI node.
BUG=b:173461736 TEST=abuild -t GOOGLE_VOLTEER2{_TI50,} -c max -x
Change-Id: I5576a595bbabc34c62b768f8b3439e35ff6bcf7b Signed-off-by: Jes Bodi Klinke jbk@chromium.org --- M src/mainboard/google/volteer/Kconfig.name M src/mainboard/google/volteer/mainboard.c M src/mainboard/google/volteer/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/volteer/variants/volteer2/Makefile.inc M src/mainboard/google/volteer/variants/volteer2/overridetree.cb A src/mainboard/google/volteer/variants/volteer2/variant.c 6 files changed, 57 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/48223/3
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48223 )
Change subject: mb/google/volteer: ACPI nodes for volteer2_ti50 ......................................................................
Patch Set 3: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/48223/3/src/mainboard/google/voltee... File src/mainboard/google/volteer/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/48223/3/src/mainboard/google/voltee... PS3, Line 64: DRIVER_I2C_TPM_ACPI Doing this unconditionally in Kconfig for volteer would be fine too. It will get dropped if unused for the variant.
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48223 )
Change subject: mb/google/volteer: ACPI nodes for volteer2_ti50 ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/48223/3/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/volteer2/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/48223/3/src/mainboard/google/voltee... PS3, Line 205: GOOG0005 Is there any reason to use a new ID for Ti50? If the i2c kernel driver works as is that is fine, but do we expect to need any differences there in the long term?
Obviously this would mean adding a new ACPI ID to the list in the i2c driver (which is in the process of being upstreamed now) but we can do that pretty easily.
https://review.coreboot.org/c/coreboot/+/48223/3/src/mainboard/google/voltee... PS3, Line 207: off Do we want to allocate a bit in fw_config for this so you don't need the messy code to enable/disable entries?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48223 )
Change subject: mb/google/volteer: ACPI nodes for volteer2_ti50 ......................................................................
Patch Set 3: -Code-Review
(2 comments)
https://review.coreboot.org/c/coreboot/+/48223/3/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/volteer2/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/48223/3/src/mainboard/google/voltee... PS3, Line 205: GOOG0005
Is there any reason to use a new ID for Ti50? If the i2c kernel driver works as is that is fine, bu […]
Good point. I had missed that the HID used was different than other I2C cr50 devices.
https://review.coreboot.org/c/coreboot/+/48223/3/src/mainboard/google/voltee... PS3, Line 207: off
Do we want to allocate a bit in fw_config for this so you don't need the messy code to enable/disabl […]
I didn't suggest that because it takes away another bit from FW_CONFIG space for all variants.
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48223 )
Change subject: mb/google/volteer: ACPI nodes for volteer2_ti50 ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/48223/3/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/volteer2/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/48223/3/src/mainboard/google/voltee... PS3, Line 205: GOOG0005
Good point. I had missed that the HID used was different than other I2C cr50 devices.
Its actually the same as current H1 I2C, but we could make it different.
I don't know that we need it to be different, but there were some odd architectural limitations we found when I first did the H1 I2C drivers that we might be able to improve with Ti50. (maybe? I have not looked into it yet..)
https://review.coreboot.org/c/coreboot/+/48223/3/src/mainboard/google/voltee... PS3, Line 207: off
I didn't suggest that because it takes away another bit from FW_CONFIG space for all variants.
Ya we are down to ~15 free bits in the program fw_config, but we also have this unused 4-bit field for "thermal" which could be repurposed.
Since this is a non-shipping config it is probably fine as is..
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48223 )
Change subject: mb/google/volteer: ACPI nodes for volteer2_ti50 ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48223/3/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/volteer2/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/48223/3/src/mainboard/google/voltee... PS3, Line 205: GOOG0005
Its actually the same as current H1 I2C, but we could make it different. […]
Ah okay. Makes sense.
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48223 )
Change subject: mb/google/volteer: ACPI nodes for volteer2_ti50 ......................................................................
Patch Set 3: Code-Review+2
I don't want to hold this up on my HID question, especially if the drivers work as is today. We can always change the HID later if the kernel drivers end up needing it.
Jes Klinke has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48223 )
Change subject: mb/google/volteer: ACPI nodes for volteer2_ti50 ......................................................................
Patch Set 3:
(2 comments)
Thanks for the thorough review.
https://review.coreboot.org/c/coreboot/+/48223/3/src/mainboard/google/voltee... File src/mainboard/google/volteer/variants/volteer2/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/48223/3/src/mainboard/google/voltee... PS3, Line 205: GOOG0005
Ah okay. Makes sense.
I copied this ID from some other ChromeOS model, with Cr50.
Currently, the Ti50 firmware uses the same non-standard I2C protocol as cr50 does (at the time we added I2C support to cr50, there was a standard only for SPI).
Long term, we want to modify Ti50 to adhere to the subsequently published standard for I2C TPM communication. At that point, we likely want to change the ID here, so that the kernel can know to use a different protocol.
For now, I think we want to keep the Cr50 ID, we do not want to introduce a new ID, to be used only temporarily.
https://review.coreboot.org/c/coreboot/+/48223/3/src/mainboard/google/voltee... PS3, Line 207: off
Ya we are down to ~15 free bits in the program fw_config, but we also have this unused 4-bit field f […]
The messy code will run only for the volteer2_ti50 variant (we could further move it to a file with suffix _ti50, if we want), so I am not too worried that keeping it will get in the way of other development.
Jes Klinke has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48223 )
Change subject: mb/google/volteer: ACPI nodes for volteer2_ti50 ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48223/3/src/mainboard/google/voltee... File src/mainboard/google/volteer/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/48223/3/src/mainboard/google/voltee... PS3, Line 64: DRIVER_I2C_TPM_ACPI
Doing this unconditionally in Kconfig for volteer would be fine too. […]
I see. I think I still want to keep these in a part specific to volteer2, as is. Unless you feel that avoiding the conditional declarations are preferrable.
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/48223 )
Change subject: mb/google/volteer: ACPI nodes for volteer2_ti50 ......................................................................
mb/google/volteer: ACPI nodes for volteer2_ti50
Unique among the Volteer devices, the volteer2_ti50 variant connects to the TPM via I2C. This CL introduces the proper devicestree declarations for the Linux kernel to recognize that.
overridetree.cb is shared between "sub"-variants volteer2 and volteer2_ti50, so both will have two TPM nodes, the I2C being disabled by default. The odd _ti50 variant then has code in variant.c to enable the I2C node and disable the SPI node.
BUG=b:173461736 TEST=abuild -t GOOGLE_VOLTEER2{_TI50,} -c max -x
Change-Id: I5576a595bbabc34c62b768f8b3439e35ff6bcf7b Signed-off-by: Jes Bodi Klinke jbk@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/48223 Reviewed-by: Duncan Laurie dlaurie@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/mainboard/google/volteer/Kconfig.name M src/mainboard/google/volteer/mainboard.c M src/mainboard/google/volteer/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/volteer/variants/volteer2/Makefile.inc M src/mainboard/google/volteer/variants/volteer2/overridetree.cb A src/mainboard/google/volteer/variants/volteer2/variant.c 6 files changed, 57 insertions(+), 7 deletions(-)
Approvals: build bot (Jenkins): Verified Duncan Laurie: Looks good to me, approved
diff --git a/src/mainboard/google/volteer/Kconfig.name b/src/mainboard/google/volteer/Kconfig.name index 9b66662..8777994 100644 --- a/src/mainboard/google/volteer/Kconfig.name +++ b/src/mainboard/google/volteer/Kconfig.name @@ -61,6 +61,7 @@ select VARIANT_HAS_MIPI_CAMERA select SOC_INTEL_CSE_LITE_SKU select DRIVERS_GENESYSLOGIC_GL9755 + select DRIVER_I2C_TPM_ACPI
# Reworked Volteer2 prototype, Haven chip replaced with Dauntless demo board config BOARD_GOOGLE_VOLTEER2_TI50 @@ -69,6 +70,7 @@ select VARIANT_HAS_MIPI_CAMERA select SOC_INTEL_CSE_LITE_SKU select DRIVERS_GENESYSLOGIC_GL9755 + select DRIVER_I2C_TPM_ACPI
config BOARD_GOOGLE_VOXEL bool "-> Voxel" diff --git a/src/mainboard/google/volteer/mainboard.c b/src/mainboard/google/volteer/mainboard.c index ea9e08f..1f20e18 100644 --- a/src/mainboard/google/volteer/mainboard.c +++ b/src/mainboard/google/volteer/mainboard.c @@ -23,8 +23,8 @@
static bool is_port1(struct device *dev) { - return dev->path.type == DEVICE_PATH_GENERIC && dev->path.generic.id == 1 && - dev->chip_ops == &drivers_intel_pmc_mux_conn_ops; + return dev->path.type == DEVICE_PATH_GENERIC && dev->path.generic.id == 1 + && dev->chip_ops == &drivers_intel_pmc_mux_conn_ops; }
static void typec_orientation_fixup(void) @@ -53,14 +53,15 @@ if (!conn) return;
- if (fw_config_probe(FW_CONFIG(DB_USB, USB4_GEN2)) || - fw_config_probe(FW_CONFIG(DB_USB, USB3_ACTIVE)) || - fw_config_probe(FW_CONFIG(DB_USB, USB4_GEN3)) || - fw_config_probe(FW_CONFIG(DB_USB, USB3_NO_A))) { + if (fw_config_probe(FW_CONFIG(DB_USB, USB4_GEN2)) + || fw_config_probe(FW_CONFIG(DB_USB, USB3_ACTIVE)) + || fw_config_probe(FW_CONFIG(DB_USB, USB4_GEN3)) + || fw_config_probe(FW_CONFIG(DB_USB, USB3_NO_A))) { struct drivers_intel_pmc_mux_conn_config *config = conn->chip_info;
if (config) { - printk(BIOS_INFO, "Configure Right Type-C port orientation for retimer\n"); + printk(BIOS_INFO, + "Configure Right Type-C port orientation for retimer\n"); config->sbu_orientation = TYPEC_ORIENTATION_NORMAL; } } @@ -70,6 +71,11 @@ { mainboard_ec_init(); typec_orientation_fixup(); + variant_devtree_update(); +} + +void __weak variant_devtree_update(void) +{ }
static void add_fw_config_oem_string(const struct fw_config *config, void *arg) diff --git a/src/mainboard/google/volteer/variants/baseboard/include/baseboard/variants.h b/src/mainboard/google/volteer/variants/baseboard/include/baseboard/variants.h index 8408198..4d5dc87 100644 --- a/src/mainboard/google/volteer/variants/baseboard/include/baseboard/variants.h +++ b/src/mainboard/google/volteer/variants/baseboard/include/baseboard/variants.h @@ -21,4 +21,7 @@ const struct ddr_memory_cfg *variant_memory_params(void); int variant_memory_sku(void);
+/* Modify devictree settings during ramstage. */ +void variant_devtree_update(void); + #endif /* __BASEBOARD_VARIANTS_H__ */ diff --git a/src/mainboard/google/volteer/variants/volteer2/Makefile.inc b/src/mainboard/google/volteer/variants/volteer2/Makefile.inc index 13269db..04af3ae 100644 --- a/src/mainboard/google/volteer/variants/volteer2/Makefile.inc +++ b/src/mainboard/google/volteer/variants/volteer2/Makefile.inc @@ -3,3 +3,4 @@ bootblock-y += gpio.c
ramstage-y += gpio.c +ramstage-y += variant.c diff --git a/src/mainboard/google/volteer/variants/volteer2/overridetree.cb b/src/mainboard/google/volteer/variants/volteer2/overridetree.cb index a1012a6..916777c 100644 --- a/src/mainboard/google/volteer/variants/volteer2/overridetree.cb +++ b/src/mainboard/google/volteer/variants/volteer2/overridetree.cb @@ -201,6 +201,11 @@ register "key.label" = ""pen_eject"" device generic 0 on end end + chip drivers/i2c/tpm + register "hid" = ""GOOG0005"" + register "irq" = "ACPI_IRQ_EDGE_LOW(GPP_C21_IRQ)" + device i2c 50 off end + end end device ref i2c2 on chip drivers/i2c/sx9310 diff --git a/src/mainboard/google/volteer/variants/volteer2/variant.c b/src/mainboard/google/volteer/variants/volteer2/variant.c new file mode 100644 index 0000000..057bb8a --- /dev/null +++ b/src/mainboard/google/volteer/variants/volteer2/variant.c @@ -0,0 +1,33 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <baseboard/variants.h> +#include <soc/pci_devs.h> +#include <acpi/acpi_device.h> + +extern struct chip_operations drivers_i2c_tpm_ops; + +static bool match_i2c_tpm(DEVTREE_CONST struct device *dev) +{ + return dev->chip_ops == &drivers_i2c_tpm_ops; +} + +/* + * This function runs only on the volteer_ti50 variant, which has the GSC on a + * reworked I2C bus. + */ +static void devtree_enable_i2c_tpm(void) +{ + struct device *spi_tpm = pcidev_path_on_root(PCH_DEVFN_GSPI0)->link_list->children; + struct device *i2c_tpm = dev_find_matching_device_on_bus( + pcidev_path_on_root(PCH_DEVFN_I2C1)->link_list, match_i2c_tpm); + if (!i2c_tpm || !spi_tpm) + return; + spi_tpm->enabled = 0; + i2c_tpm->enabled = 1; +} + +void variant_devtree_update(void) +{ + if (CONFIG(MAINBOARD_HAS_I2C_TPM_CR50)) + devtree_enable_i2c_tpm(); +}
-
Duncan Laurie (Code Review) -
Furquan Shaikh (Code Review)
-
Jes Klinke (Code Review)
-
Patrick Georgi (Code Review)