Jes Klinke has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46437 )
Change subject: mb/google/volteer: New variant for Volteer reworked with Dauntless ......................................................................
Patch Set 10:
(3 comments)
https://review.coreboot.org/c/coreboot/+/46437/6/src/mainboard/google/voltee... File src/mainboard/google/volteer/Kconfig:
https://review.coreboot.org/c/coreboot/+/46437/6/src/mainboard/google/voltee... PS6, Line 140: volteer2_ti50
I am surprised though, that I do not see any similar declaration in the SPI section of the existi […]
Acknowledged, thanks. I will look at how to get the kernel to play along, once we get there.
https://review.coreboot.org/c/coreboot/+/46437/10/src/mainboard/google/volte... File src/mainboard/google/volteer/variants/volteer2/gpio.c:
https://review.coreboot.org/c/coreboot/+/46437/10/src/mainboard/google/volte... PS10, Line 249: UP_2K
Why UP_2K here and for SCL below?
That was how it was in the code that I copied from, "reef", I think: https://source.corp.google.com/chromeos_public/src/third_party/coreboot/src/...
I do not know if the reworked Volteer boards have external pullups, where reef did not. I do see on my scope that the slopes when returning to high level are not much steeper than what is necessary to support the current 400kHZ clock rate.
https://review.coreboot.org/c/coreboot/+/46437/10/src/mainboard/google/volte... File src/mainboard/google/volteer/variants/volteer2/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/46437/10/src/mainboard/google/volte... PS10, Line 23: register "common_soc_config.gspi[0].early_init" = "CONFIG(MAINBOARD_HAS_SPI_TPM_CR50)" : register "common_soc_config.i2c[1].early_init" = "CONFIG(MAINBOARD_HAS_I2C_TPM_CR50)"
Did these work correctly for overriding the early_init configs? I remember there was some expectatio […]
I had not. Looking at static.c after running util/abuild/abuild -t GOOGLE_VOLTEER2_TI50 -c max -x, it seems to use syntax that I am not familiar with:
struct soc_intel_tigerlake_config soc_intel_tigerlake_info_1 = { ..., .common_soc_config = { ...., .gspi[0] = { .speed_mhz = 1, .early_init = 1, }, ..., .i2c[1] = { .speed = I2C_SPEED_FAST, }, ..., }, .common_soc_config.gspi[0].early_init = CONFIG(MAINBOARD_HAS_SPI_TPM_CR50), .common_soc_config.i2c[1].early_init = CONFIG(MAINBOARD_HAS_I2C_TPM_CR50), ..., }
I do remember getting errors if I spelled i2c[1] wrong, so maybe it is valid syntax, I do not know. I have not tried running a Volteer with the code, as I do not have my coreboot repository linked to a ChromeOS chroot. I will try that to be sure.
If the current code does not work, then I would prefer not to copy all of the common_soc_config section from the shared devicetree.cb into this overridetree.cb. I feel that the risk of some future tweak to the common devicestree.cb would too easily be lost that way. Rather, unless I head objections, I would like to put the two conditional CONFIG() invocations into the common devicetree.cb.