Joel Kitching has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31943
Change subject: vboot: deprecate physical dev switch ......................................................................
vboot: deprecate physical dev switch
Currently only two devices make use of physical dev switch: stumpy, lumpy
Deprecate this switch. If these devices are flashed to ToT, they may still make use of virtual dev switch, activated via recovery screen.
BUG=b:124141368, b:124192753, chromium:942901 TEST=util/lint/checkpatch.pl -g origin/master..HEAD TEST=util/abuild/abuild -B -e -y -c 50 -p none -x TEST=make clean && make test-abuild BRANCH=none
Change-Id: I87ec0db6148c1727b95475d94e3e3f6e7ec83193 Signed-off-by: Joel Kitching kitching@google.com --- M src/include/bootmode.h M src/mainboard/intel/glkrvp/chromeos.c M src/mainboard/samsung/lumpy/Kconfig M src/mainboard/samsung/lumpy/chromeos.c M src/mainboard/samsung/stumpy/Kconfig M src/mainboard/samsung/stumpy/chromeos.c M src/security/vboot/Kconfig M src/security/vboot/vboot_handoff.c M src/security/vboot/vboot_logic.c 9 files changed, 1 insertion(+), 57 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/31943/1
diff --git a/src/include/bootmode.h b/src/include/bootmode.h index 5650b3d..33148dc 100644 --- a/src/include/bootmode.h +++ b/src/include/bootmode.h @@ -19,7 +19,6 @@ /* functions implemented per mainboard: */ void init_bootmode_straps(void); int get_write_protect_state(void); -int get_developer_mode_switch(void); int get_recovery_mode_switch(void); int get_recovery_mode_retrain_switch(void); int clear_recovery_mode_switch(void); diff --git a/src/mainboard/intel/glkrvp/chromeos.c b/src/mainboard/intel/glkrvp/chromeos.c index 1da0d29..07d92e9 100644 --- a/src/mainboard/intel/glkrvp/chromeos.c +++ b/src/mainboard/intel/glkrvp/chromeos.c @@ -26,7 +26,6 @@ struct lb_gpio chromeos_gpios[] = { {-1, ACTIVE_HIGH, get_write_protect_state(), "write protect"}, {-1, ACTIVE_HIGH, get_recovery_mode_switch(), "recovery"}, - {-1, ACTIVE_HIGH, get_developer_mode_switch(), "developer"}, {-1, ACTIVE_HIGH, get_lid_switch(), "lid"}, {-1, ACTIVE_HIGH, 0, "power"}, {-1, ACTIVE_HIGH, gfx_get_init_done(), "oprom"}, @@ -35,12 +34,6 @@ lb_add_gpios(gpios, chromeos_gpios, ARRAY_SIZE(chromeos_gpios)); }
-int get_developer_mode_switch(void) -{ - /* No physical developer mode switch. It's virtual. */ - return 0; -} - int get_write_protect_state(void) { return 0; diff --git a/src/mainboard/samsung/lumpy/Kconfig b/src/mainboard/samsung/lumpy/Kconfig index 68aeabd..37c967f 100644 --- a/src/mainboard/samsung/lumpy/Kconfig +++ b/src/mainboard/samsung/lumpy/Kconfig @@ -25,7 +25,6 @@ select INTEL_INT15
config VBOOT - select VBOOT_PHYSICAL_DEV_SWITCH select VBOOT_PHYSICAL_REC_SWITCH select VBOOT_VBNV_CMOS
diff --git a/src/mainboard/samsung/lumpy/chromeos.c b/src/mainboard/samsung/lumpy/chromeos.c index baa4094..ba00245 100644 --- a/src/mainboard/samsung/lumpy/chromeos.c +++ b/src/mainboard/samsung/lumpy/chromeos.c @@ -60,12 +60,6 @@ gpios->gpios[1].value = !get_recovery_mode_switch(); strncpy((char *)gpios->gpios[1].name,"recovery", GPIO_MAX_NAME_LENGTH);
- /* Developer: GPIO17 = KBC3_DVP_MODE */ - gpios->gpios[2].port = GPIO_DEV_MODE; - gpios->gpios[2].polarity = ACTIVE_HIGH; - gpios->gpios[2].value = get_developer_mode_switch(); - strncpy((char *)gpios->gpios[2].name,"developer", GPIO_MAX_NAME_LENGTH); - gpios->gpios[3].port = 100; gpios->gpios[3].polarity = ACTIVE_HIGH; gpios->gpios[3].value = lid & 1; @@ -95,16 +89,6 @@ return (pci_read_config32(dev, SATA_SP) >> FLAG_SPI_WP) & 1; }
-int get_developer_mode_switch(void) -{ -#ifdef __SIMPLE_DEVICE__ - pci_devfn_t dev = PCI_DEV(0, 0x1f, 2); -#else - struct device *dev = pcidev_on_root(0x1f, 2); -#endif - return (pci_read_config32(dev, SATA_SP) >> FLAG_DEV_MODE) & 1; -} - int get_recovery_mode_switch(void) { #ifdef __SIMPLE_DEVICE__ diff --git a/src/mainboard/samsung/stumpy/Kconfig b/src/mainboard/samsung/stumpy/Kconfig index 36e28ff..d17dc68 100644 --- a/src/mainboard/samsung/stumpy/Kconfig +++ b/src/mainboard/samsung/stumpy/Kconfig @@ -21,7 +21,6 @@ select INTEL_INT15
config VBOOT - select VBOOT_PHYSICAL_DEV_SWITCH select VBOOT_PHYSICAL_REC_SWITCH select VBOOT_VBNV_CMOS
diff --git a/src/mainboard/samsung/stumpy/chromeos.c b/src/mainboard/samsung/stumpy/chromeos.c index 85f9038..b3c66c5 100644 --- a/src/mainboard/samsung/stumpy/chromeos.c +++ b/src/mainboard/samsung/stumpy/chromeos.c @@ -56,12 +56,6 @@ gpios->gpios[1].value = !get_recovery_mode_switch(); strncpy((char *)gpios->gpios[1].name,"recovery", GPIO_MAX_NAME_LENGTH);
- /* Developer: GPIO17 = KBC3_DVP_MODE */ - gpios->gpios[2].port = GPIO_DEV_MODE; - gpios->gpios[2].polarity = ACTIVE_HIGH; - gpios->gpios[2].value = get_developer_mode_switch(); - strncpy((char *)gpios->gpios[2].name,"developer", GPIO_MAX_NAME_LENGTH); - /* Hard code the lid switch GPIO to open. */ gpios->gpios[3].port = 100; gpios->gpios[3].polarity = ACTIVE_HIGH; @@ -92,16 +86,6 @@ return (pci_read_config32(dev, SATA_SP) >> FLAG_SPI_WP) & 1; }
-int get_developer_mode_switch(void) -{ -#ifdef __SIMPLE_DEVICE__ - pci_devfn_t dev = PCI_DEV(0, 0x1f, 2); -#else - struct device *dev = pcidev_on_root(0x1f, 2); -#endif - return (pci_read_config32(dev, SATA_SP) >> FLAG_DEV_MODE) & 1; -} - int get_recovery_mode_switch(void) { #ifdef __SIMPLE_DEVICE__ diff --git a/src/security/vboot/Kconfig b/src/security/vboot/Kconfig index ef5455f..8a8f8dc 100644 --- a/src/security/vboot/Kconfig +++ b/src/security/vboot/Kconfig @@ -176,15 +176,6 @@ bool default n
-config VBOOT_PHYSICAL_DEV_SWITCH - bool - default n - help - Whether this platform has a physical developer switch. Note that this - disables virtual dev switch functionality (through secdata). Operation - where both a physical pin and the virtual switch get sampled is not - supported by coreboot. - config VBOOT_PHYSICAL_REC_SWITCH bool default n diff --git a/src/security/vboot/vboot_handoff.c b/src/security/vboot/vboot_handoff.c index c916b6d..6f5b0a3 100644 --- a/src/security/vboot/vboot_handoff.c +++ b/src/security/vboot/vboot_handoff.c @@ -79,8 +79,7 @@ vb_sd->flags |= VBSD_LF_DEV_SWITCH_ON; } /* TODO: Set these in depthcharge */ - if (!CONFIG(VBOOT_PHYSICAL_DEV_SWITCH)) - vb_sd->flags |= VBSD_HONOR_VIRT_DEV_SWITCH; + vb_sd->flags |= VBSD_HONOR_VIRT_DEV_SWITCH; if (!CONFIG(VBOOT_PHYSICAL_REC_SWITCH)) vb_sd->flags |= VBSD_BOOT_REC_SWITCH_VIRTUAL; if (CONFIG(VBOOT_OPROM_MATTERS)) { diff --git a/src/security/vboot/vboot_logic.c b/src/security/vboot/vboot_logic.c index 4aab795..a074539 100644 --- a/src/security/vboot/vboot_logic.c +++ b/src/security/vboot/vboot_logic.c @@ -324,10 +324,6 @@ die("Initializing measured boot mode failed!"); }
- if (CONFIG(VBOOT_PHYSICAL_DEV_SWITCH) && - get_developer_mode_switch()) - ctx.flags |= VB2_CONTEXT_FORCE_DEVELOPER_MODE; - if (get_recovery_mode_switch()) { ctx.flags |= VB2_CONTEXT_FORCE_RECOVERY_MODE; if (CONFIG(VBOOT_DISABLE_DEV_ON_RECOVERY))
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31943 )
Change subject: vboot: deprecate physical dev switch ......................................................................
Patch Set 1:
(2 comments)
I don't know the details, I thought virtual dev switch depended on chrome-ec.
https://review.coreboot.org/#/c/31943/1/src/mainboard/samsung/stumpy/chromeo... File src/mainboard/samsung/stumpy/chromeos.c:
https://review.coreboot.org/#/c/31943/1/src/mainboard/samsung/stumpy/chromeo... PS1, Line 132: Do you want to keep this?
https://review.coreboot.org/#/c/31943/1/src/mainboard/samsung/stumpy/chromeo... PS1, Line 139: This probably keeps switch advertised in ACPI ?
Hello Aaron Durbin, Julius Werner, Hung-Te Lin, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31943
to look at the new patch set (#2).
Change subject: vboot: deprecate physical dev switch ......................................................................
vboot: deprecate physical dev switch
Currently only two devices make use of physical dev switch: stumpy, lumpy
Deprecate this switch. If these devices are flashed to ToT, they may still make use of virtual dev switch, activated via recovery screen.
BUG=b:124141368, b:124192753, chromium:942901 TEST=util/lint/checkpatch.pl -g origin/master..HEAD TEST=util/abuild/abuild -B -e -y -c 50 -p none -x TEST=make clean && make test-abuild BRANCH=none
Change-Id: I87ec0db6148c1727b95475d94e3e3f6e7ec83193 Signed-off-by: Joel Kitching kitching@google.com --- M src/include/bootmode.h M src/mainboard/google/beltino/chromeos.c M src/mainboard/google/jecht/chromeos.c M src/mainboard/intel/glkrvp/chromeos.c M src/mainboard/samsung/lumpy/Kconfig M src/mainboard/samsung/lumpy/chromeos.c M src/mainboard/samsung/stumpy/Kconfig M src/mainboard/samsung/stumpy/chromeos.c M src/security/vboot/Kconfig M src/security/vboot/vboot_handoff.c M src/security/vboot/vboot_logic.c 11 files changed, 1 insertion(+), 71 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/31943/2
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31943 )
Change subject: vboot: deprecate physical dev switch ......................................................................
Patch Set 2: Code-Review+1
(2 comments)
Patch Set 1:
(2 comments)
I don't know the details, I thought virtual dev switch depended on chrome-ec.
Thanks for pointing out that GPIO code.
Virtual dev switch depends on being able to enter recovery mode. The process for entering recovery mode depends on the particular device -- for most, a keyboard combination is used (Esc+Refresh+Power) which indeed depends on Chrome EC. For others, a physical recovery mode button is used, which does not require Chrome EC.
https://review.coreboot.org/#/c/31943/1/src/mainboard/samsung/stumpy/chromeo... File src/mainboard/samsung/stumpy/chromeos.c:
https://review.coreboot.org/#/c/31943/1/src/mainboard/samsung/stumpy/chromeo... PS1, Line 132:
Do you want to keep this?
Done
https://review.coreboot.org/#/c/31943/1/src/mainboard/samsung/stumpy/chromeo... PS1, Line 139:
This probably keeps switch advertised in ACPI ?
Done
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31943 )
Change subject: vboot: deprecate physical dev switch ......................................................................
Patch Set 2: Code-Review+1
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31943 )
Change subject: vboot: deprecate physical dev switch ......................................................................
Patch Set 2:
Adding Patrick for help pulling patch downstream.
Simon Glass has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31943 )
Change subject: vboot: deprecate physical dev switch ......................................................................
Patch Set 2: Code-Review+1
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31943 )
Change subject: vboot: deprecate physical dev switch ......................................................................
Patch Set 3:
(3 comments)
I don't know the details, I thought virtual dev switch depended on chrome-ec.
Thanks for pointing out that GPIO code.
Virtual dev switch depends on being able to enter recovery mode. The process for entering recovery mode depends on the particular device -- for most, a keyboard combination is used (Esc+Refresh+Power) which indeed depends on Chrome EC. For others, a physical recovery mode button is used, which does not require Chrome EC.
So in short: no, it doesn't and never did depend on the EC (virtual *recovery* switch does, but that's something else). It does depend on the TPM, but all Chromebooks (even the ones that still used physical dev switches) have one, so I hope (although we're not gonna dig out the 7 year old hardware to test it) that the virtual switch would just work on a Lumpy after this patch.
https://review.coreboot.org/#/c/31943/3/src/mainboard/samsung/lumpy/chromeos... File src/mainboard/samsung/lumpy/chromeos.c:
https://review.coreboot.org/#/c/31943/3/src/mainboard/samsung/lumpy/chromeos... PS3, Line 61: 3 Uhh... fix GPIO_COUNT and remove the gap please. Otherwise there'll be uninitialized garbage in the table (or possibly zeroes, not quite sure, but neither of that is good).
In fact, it would be nice to update this to use the newer, more compact (and more fool-proof) lb_add_gpios() notation, if you want to go that extra mile.
https://review.coreboot.org/#/c/31943/3/src/mainboard/samsung/lumpy/chromeos... PS3, Line 119: static const struct cros_gpio cros_gpios[] = { Do we wanna get rid of the CROS_GPIO_DEV_AL/AH macros as well while we're at it? And maybe remove or at least rename the CROS_GPIO_DEV enum value.
https://review.coreboot.org/#/c/31943/3/src/mainboard/samsung/stumpy/chromeo... File src/mainboard/samsung/stumpy/chromeos.c:
https://review.coreboot.org/#/c/31943/3/src/mainboard/samsung/stumpy/chromeo... PS3, Line 58: gpios->gpios[3].port = 100; Same here.
Hello Kyösti Mälkki, Aaron Durbin, Simon Glass, Julius Werner, Hung-Te Lin, build bot (Jenkins), Simon Glass, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31943
to look at the new patch set (#5).
Change subject: vboot: deprecate physical dev switch ......................................................................
vboot: deprecate physical dev switch
Currently only two devices make use of physical dev switch: stumpy, lumpy
Deprecate this switch. If these devices are flashed to ToT, they may still make use of virtual dev switch, activated via recovery screen.
BUG=b:124141368, b:124192753, chromium:942901 TEST=util/lint/checkpatch.pl -g origin/master..HEAD TEST=util/abuild/abuild -B -e -y -c 50 -p none -x TEST=make clean && make test-abuild BRANCH=none
Change-Id: I87ec0db6148c1727b95475d94e3e3f6e7ec83193 Signed-off-by: Joel Kitching kitching@google.com --- M src/include/bootmode.h M src/mainboard/google/beltino/chromeos.c M src/mainboard/google/jecht/chromeos.c M src/mainboard/intel/glkrvp/chromeos.c M src/mainboard/samsung/lumpy/Kconfig M src/mainboard/samsung/lumpy/chromeos.c M src/mainboard/samsung/stumpy/Kconfig M src/mainboard/samsung/stumpy/chromeos.c M src/security/vboot/Kconfig M src/security/vboot/vboot_handoff.c M src/security/vboot/vboot_logic.c 11 files changed, 26 insertions(+), 96 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/31943/5
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31943 )
Change subject: vboot: deprecate physical dev switch ......................................................................
Patch Set 5:
(6 comments)
https://review.coreboot.org/#/c/31943/5/src/mainboard/samsung/lumpy/chromeos... File src/mainboard/samsung/lumpy/chromeos.c:
https://review.coreboot.org/#/c/31943/5/src/mainboard/samsung/lumpy/chromeos... PS5, Line 64: strncpy((char *)gpios->gpios[2].name,"lid", GPIO_MAX_NAME_LENGTH); space required after that ',' (ctx:VxV)
https://review.coreboot.org/#/c/31943/5/src/mainboard/samsung/lumpy/chromeos... PS5, Line 70: strncpy((char *)gpios->gpios[3].name,"power", GPIO_MAX_NAME_LENGTH); space required after that ',' (ctx:VxV)
https://review.coreboot.org/#/c/31943/5/src/mainboard/samsung/lumpy/chromeos... PS5, Line 76: strncpy((char *)gpios->gpios[4].name,"oprom", GPIO_MAX_NAME_LENGTH); space required after that ',' (ctx:VxV)
https://review.coreboot.org/#/c/31943/5/src/mainboard/samsung/stumpy/chromeo... File src/mainboard/samsung/stumpy/chromeos.c:
https://review.coreboot.org/#/c/31943/5/src/mainboard/samsung/stumpy/chromeo... PS5, Line 61: strncpy((char *)gpios->gpios[2].name,"lid", GPIO_MAX_NAME_LENGTH); space required after that ',' (ctx:VxV)
https://review.coreboot.org/#/c/31943/5/src/mainboard/samsung/stumpy/chromeo... PS5, Line 67: strncpy((char *)gpios->gpios[3].name,"power", GPIO_MAX_NAME_LENGTH); space required after that ',' (ctx:VxV)
https://review.coreboot.org/#/c/31943/5/src/mainboard/samsung/stumpy/chromeo... PS5, Line 73: strncpy((char *)gpios->gpios[4].name,"oprom", GPIO_MAX_NAME_LENGTH); space required after that ',' (ctx:VxV)
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31943 )
Change subject: vboot: deprecate physical dev switch ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/31943/5/src/mainboard/google/beltino/chromeo... File src/mainboard/google/beltino/chromeos.c:
https://review.coreboot.org/#/c/31943/5/src/mainboard/google/beltino/chromeo... PS5, Line 31: #ifndef __PRE_RAM__ This changed in master, might require manual rebase.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31943 )
Change subject: vboot: deprecate physical dev switch ......................................................................
Patch Set 5:
Also that jenkins failure will get fixed with a rebase to master.
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31943 )
Change subject: vboot: deprecate physical dev switch ......................................................................
Patch Set 6:
(4 comments)
Patch Set 5:
Also that jenkins failure will get fixed with a rebase to master.
Right, thanks!
https://review.coreboot.org/#/c/31943/5/src/mainboard/google/beltino/chromeo... File src/mainboard/google/beltino/chromeos.c:
https://review.coreboot.org/#/c/31943/5/src/mainboard/google/beltino/chromeo... PS5, Line 31: #ifndef __PRE_RAM__
This changed in master, might require manual rebase.
Thanks for the reminder!
https://review.coreboot.org/#/c/31943/3/src/mainboard/samsung/lumpy/chromeos... File src/mainboard/samsung/lumpy/chromeos.c:
https://review.coreboot.org/#/c/31943/3/src/mainboard/samsung/lumpy/chromeos... PS3, Line 61: 3
Uhh... fix GPIO_COUNT and remove the gap please. […]
Done
https://review.coreboot.org/#/c/31943/3/src/mainboard/samsung/lumpy/chromeos... PS3, Line 119: static const struct cros_gpio cros_gpios[] = {
Do we wanna get rid of the CROS_GPIO_DEV_AL/AH macros as well while we're at it? And maybe remove or […]
Let's deal with the ACPI stuff separately. I wrote a bunch about it here: https://bugs.chromium.org/p/chromium/issues/detail?id=942901#c13
https://review.coreboot.org/#/c/31943/3/src/mainboard/samsung/stumpy/chromeo... File src/mainboard/samsung/stumpy/chromeos.c:
https://review.coreboot.org/#/c/31943/3/src/mainboard/samsung/stumpy/chromeo... PS3, Line 58: gpios->gpios[3].port = 100;
Same here.
Done
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31943 )
Change subject: vboot: deprecate physical dev switch ......................................................................
Patch Set 6:
(6 comments)
https://review.coreboot.org/#/c/31943/6/src/mainboard/samsung/lumpy/chromeos... File src/mainboard/samsung/lumpy/chromeos.c:
https://review.coreboot.org/#/c/31943/6/src/mainboard/samsung/lumpy/chromeos... PS6, Line 64: strncpy((char *)gpios->gpios[2].name,"lid", GPIO_MAX_NAME_LENGTH); space required after that ',' (ctx:VxV)
https://review.coreboot.org/#/c/31943/6/src/mainboard/samsung/lumpy/chromeos... PS6, Line 70: strncpy((char *)gpios->gpios[3].name,"power", GPIO_MAX_NAME_LENGTH); space required after that ',' (ctx:VxV)
https://review.coreboot.org/#/c/31943/6/src/mainboard/samsung/lumpy/chromeos... PS6, Line 76: strncpy((char *)gpios->gpios[4].name,"oprom", GPIO_MAX_NAME_LENGTH); space required after that ',' (ctx:VxV)
https://review.coreboot.org/#/c/31943/6/src/mainboard/samsung/stumpy/chromeo... File src/mainboard/samsung/stumpy/chromeos.c:
https://review.coreboot.org/#/c/31943/6/src/mainboard/samsung/stumpy/chromeo... PS6, Line 61: strncpy((char *)gpios->gpios[2].name,"lid", GPIO_MAX_NAME_LENGTH); space required after that ',' (ctx:VxV)
https://review.coreboot.org/#/c/31943/6/src/mainboard/samsung/stumpy/chromeo... PS6, Line 67: strncpy((char *)gpios->gpios[3].name,"power", GPIO_MAX_NAME_LENGTH); space required after that ',' (ctx:VxV)
https://review.coreboot.org/#/c/31943/6/src/mainboard/samsung/stumpy/chromeo... PS6, Line 73: strncpy((char *)gpios->gpios[4].name,"oprom", GPIO_MAX_NAME_LENGTH); space required after that ',' (ctx:VxV)
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31943 )
Change subject: vboot: deprecate physical dev switch ......................................................................
Patch Set 6: Code-Review+1
intel/emeraldlake2/chromeos.c:87 is one more phys dev switch I believe
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31943 )
Change subject: vboot: deprecate physical dev switch ......................................................................
Patch Set 6: Code-Review+2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31943 )
Change subject: vboot: deprecate physical dev switch ......................................................................
Patch Set 7:
(6 comments)
https://review.coreboot.org/#/c/31943/7/src/mainboard/samsung/lumpy/chromeos... File src/mainboard/samsung/lumpy/chromeos.c:
https://review.coreboot.org/#/c/31943/7/src/mainboard/samsung/lumpy/chromeos... PS7, Line 64: strncpy((char *)gpios->gpios[2].name,"lid", GPIO_MAX_NAME_LENGTH); space required after that ',' (ctx:VxV)
https://review.coreboot.org/#/c/31943/7/src/mainboard/samsung/lumpy/chromeos... PS7, Line 70: strncpy((char *)gpios->gpios[3].name,"power", GPIO_MAX_NAME_LENGTH); space required after that ',' (ctx:VxV)
https://review.coreboot.org/#/c/31943/7/src/mainboard/samsung/lumpy/chromeos... PS7, Line 76: strncpy((char *)gpios->gpios[4].name,"oprom", GPIO_MAX_NAME_LENGTH); space required after that ',' (ctx:VxV)
https://review.coreboot.org/#/c/31943/7/src/mainboard/samsung/stumpy/chromeo... File src/mainboard/samsung/stumpy/chromeos.c:
https://review.coreboot.org/#/c/31943/7/src/mainboard/samsung/stumpy/chromeo... PS7, Line 61: strncpy((char *)gpios->gpios[2].name,"lid", GPIO_MAX_NAME_LENGTH); space required after that ',' (ctx:VxV)
https://review.coreboot.org/#/c/31943/7/src/mainboard/samsung/stumpy/chromeo... PS7, Line 67: strncpy((char *)gpios->gpios[3].name,"power", GPIO_MAX_NAME_LENGTH); space required after that ',' (ctx:VxV)
https://review.coreboot.org/#/c/31943/7/src/mainboard/samsung/stumpy/chromeo... PS7, Line 73: strncpy((char *)gpios->gpios[4].name,"oprom", GPIO_MAX_NAME_LENGTH); space required after that ',' (ctx:VxV)
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31943 )
Change subject: vboot: deprecate physical dev switch ......................................................................
Patch Set 7: Code-Review+1
Patch Set 6: Code-Review+1
intel/emeraldlake2/chromeos.c:87 is one more phys dev switch I believe
Let's deal with the ACPI stuff separately. I wrote a bunch about it here: https://bugs.chromium.org/p/chromium/issues/detail?id=942901#c13
Simon Glass has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31943 )
Change subject: vboot: deprecate physical dev switch ......................................................................
Patch Set 7: Code-Review+1
Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31943 )
Change subject: vboot: deprecate physical dev switch ......................................................................
Patch Set 7:
There are still some Chromium CLs blocked on this one being merged; can we submit this?
Julius Werner has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31943 )
Change subject: vboot: deprecate physical dev switch ......................................................................
vboot: deprecate physical dev switch
Currently only two devices make use of physical dev switch: stumpy, lumpy
Deprecate this switch. If these devices are flashed to ToT, they may still make use of virtual dev switch, activated via recovery screen.
BUG=b:124141368, b:124192753, chromium:942901 TEST=util/lint/checkpatch.pl -g origin/master..HEAD TEST=util/abuild/abuild -B -e -y -c 50 -p none -x TEST=make clean && make test-abuild BRANCH=none
Change-Id: I87ec0db6148c1727b95475d94e3e3f6e7ec83193 Signed-off-by: Joel Kitching kitching@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/31943 Reviewed-by: Simon Glass sjg@chromium.org Reviewed-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-by: Julius Werner jwerner@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/include/bootmode.h M src/mainboard/google/beltino/chromeos.c M src/mainboard/google/jecht/chromeos.c M src/mainboard/intel/glkrvp/chromeos.c M src/mainboard/samsung/lumpy/Kconfig M src/mainboard/samsung/lumpy/chromeos.c M src/mainboard/samsung/stumpy/Kconfig M src/mainboard/samsung/stumpy/chromeos.c M src/security/vboot/Kconfig M src/security/vboot/vboot_handoff.c M src/security/vboot/vboot_logic.c 11 files changed, 26 insertions(+), 96 deletions(-)
Approvals: build bot (Jenkins): Verified Kyösti Mälkki: Looks good to me, but someone else must approve Julius Werner: Looks good to me, approved Simon Glass: Looks good to me, but someone else must approve Joel Kitching: Looks good to me, but someone else must approve
diff --git a/src/include/bootmode.h b/src/include/bootmode.h index 5650b3d..33148dc 100644 --- a/src/include/bootmode.h +++ b/src/include/bootmode.h @@ -19,7 +19,6 @@ /* functions implemented per mainboard: */ void init_bootmode_straps(void); int get_write_protect_state(void); -int get_developer_mode_switch(void); int get_recovery_mode_switch(void); int get_recovery_mode_retrain_switch(void); int clear_recovery_mode_switch(void); diff --git a/src/mainboard/google/beltino/chromeos.c b/src/mainboard/google/beltino/chromeos.c index dfa93a2..1a1d0a2 100644 --- a/src/mainboard/google/beltino/chromeos.c +++ b/src/mainboard/google/beltino/chromeos.c @@ -26,7 +26,6 @@
#define FLAG_SPI_WP 0 #define FLAG_REC_MODE 1 -#define FLAG_DEV_MODE 2
#if ENV_RAMSTAGE #include <boot/coreboot_tables.h> diff --git a/src/mainboard/google/jecht/chromeos.c b/src/mainboard/google/jecht/chromeos.c index 548470e..c0b14f3 100644 --- a/src/mainboard/google/jecht/chromeos.c +++ b/src/mainboard/google/jecht/chromeos.c @@ -27,7 +27,6 @@
#define FLAG_SPI_WP 0 #define FLAG_REC_MODE 1 -#define FLAG_DEV_MODE 2
#if ENV_RAMSTAGE #include <boot/coreboot_tables.h> diff --git a/src/mainboard/intel/glkrvp/chromeos.c b/src/mainboard/intel/glkrvp/chromeos.c index 1da0d29..07d92e9 100644 --- a/src/mainboard/intel/glkrvp/chromeos.c +++ b/src/mainboard/intel/glkrvp/chromeos.c @@ -26,7 +26,6 @@ struct lb_gpio chromeos_gpios[] = { {-1, ACTIVE_HIGH, get_write_protect_state(), "write protect"}, {-1, ACTIVE_HIGH, get_recovery_mode_switch(), "recovery"}, - {-1, ACTIVE_HIGH, get_developer_mode_switch(), "developer"}, {-1, ACTIVE_HIGH, get_lid_switch(), "lid"}, {-1, ACTIVE_HIGH, 0, "power"}, {-1, ACTIVE_HIGH, gfx_get_init_done(), "oprom"}, @@ -35,12 +34,6 @@ lb_add_gpios(gpios, chromeos_gpios, ARRAY_SIZE(chromeos_gpios)); }
-int get_developer_mode_switch(void) -{ - /* No physical developer mode switch. It's virtual. */ - return 0; -} - int get_write_protect_state(void) { return 0; diff --git a/src/mainboard/samsung/lumpy/Kconfig b/src/mainboard/samsung/lumpy/Kconfig index 68aeabd..37c967f 100644 --- a/src/mainboard/samsung/lumpy/Kconfig +++ b/src/mainboard/samsung/lumpy/Kconfig @@ -25,7 +25,6 @@ select INTEL_INT15
config VBOOT - select VBOOT_PHYSICAL_DEV_SWITCH select VBOOT_PHYSICAL_REC_SWITCH select VBOOT_VBNV_CMOS
diff --git a/src/mainboard/samsung/lumpy/chromeos.c b/src/mainboard/samsung/lumpy/chromeos.c index 0dba27a..5d688fe 100644 --- a/src/mainboard/samsung/lumpy/chromeos.c +++ b/src/mainboard/samsung/lumpy/chromeos.c @@ -25,18 +25,16 @@
#define GPIO_SPI_WP 24 #define GPIO_REC_MODE 42 -#define GPIO_DEV_MODE 17
#define FLAG_SPI_WP 0 #define FLAG_REC_MODE 1 -#define FLAG_DEV_MODE 2
#if ENV_RAMSTAGE #include <boot/coreboot_tables.h> #include "ec.h" #include <ec/smsc/mec1308/ec.h>
-#define GPIO_COUNT 6 +#define GPIO_COUNT 5
void fill_lb_gpios(struct lb_gpios *gpios) { @@ -60,28 +58,22 @@ gpios->gpios[1].value = !get_recovery_mode_switch(); strncpy((char *)gpios->gpios[1].name,"recovery", GPIO_MAX_NAME_LENGTH);
- /* Developer: GPIO17 = KBC3_DVP_MODE */ - gpios->gpios[2].port = GPIO_DEV_MODE; + gpios->gpios[2].port = 100; gpios->gpios[2].polarity = ACTIVE_HIGH; - gpios->gpios[2].value = get_developer_mode_switch(); - strncpy((char *)gpios->gpios[2].name,"developer", GPIO_MAX_NAME_LENGTH); - - gpios->gpios[3].port = 100; - gpios->gpios[3].polarity = ACTIVE_HIGH; - gpios->gpios[3].value = lid & 1; - strncpy((char *)gpios->gpios[3].name,"lid", GPIO_MAX_NAME_LENGTH); + gpios->gpios[2].value = lid & 1; + strncpy((char *)gpios->gpios[2].name,"lid", GPIO_MAX_NAME_LENGTH);
/* Power Button */ - gpios->gpios[4].port = 101; - gpios->gpios[4].polarity = ACTIVE_LOW; - gpios->gpios[4].value = (gen_pmcon_1 >> 9) & 1; - strncpy((char *)gpios->gpios[4].name,"power", GPIO_MAX_NAME_LENGTH); + gpios->gpios[3].port = 101; + gpios->gpios[3].polarity = ACTIVE_LOW; + gpios->gpios[3].value = (gen_pmcon_1 >> 9) & 1; + strncpy((char *)gpios->gpios[3].name,"power", GPIO_MAX_NAME_LENGTH);
/* Did we load the VGA Option ROM? */ - gpios->gpios[5].port = -1; /* Indicate that this is a pseudo GPIO */ - gpios->gpios[5].polarity = ACTIVE_HIGH; - gpios->gpios[5].value = gfx_get_init_done(); - strncpy((char *)gpios->gpios[5].name,"oprom", GPIO_MAX_NAME_LENGTH); + gpios->gpios[4].port = -1; /* Indicate that this is a pseudo GPIO */ + gpios->gpios[4].polarity = ACTIVE_HIGH; + gpios->gpios[4].value = gfx_get_init_done(); + strncpy((char *)gpios->gpios[4].name,"oprom", GPIO_MAX_NAME_LENGTH); } #endif
@@ -95,16 +87,6 @@ return (pci_read_config32(dev, SATA_SP) >> FLAG_SPI_WP) & 1; }
-int get_developer_mode_switch(void) -{ -#ifdef __SIMPLE_DEVICE__ - pci_devfn_t dev = PCI_DEV(0, 0x1f, 2); -#else - struct device *dev = pcidev_on_root(0x1f, 2); -#endif - return (pci_read_config32(dev, SATA_SP) >> FLAG_DEV_MODE) & 1; -} - int get_recovery_mode_switch(void) { #ifdef __SIMPLE_DEVICE__ @@ -130,16 +112,12 @@ /* Recovery: GPIO42 = CHP3_REC_MODE#, active low */ if (!get_gpio(GPIO_REC_MODE)) flags |= (1 << FLAG_REC_MODE); - /* Developer: GPIO17 = KBC3_DVP_MODE, active high */ - if (get_gpio(GPIO_DEV_MODE)) - flags |= (1 << FLAG_DEV_MODE);
pci_write_config32(dev, SATA_SP, flags); }
static const struct cros_gpio cros_gpios[] = { CROS_GPIO_REC_AL(GPIO_REC_MODE, CROS_GPIO_DEVICE_NAME), - CROS_GPIO_DEV_AH(GPIO_DEV_MODE, CROS_GPIO_DEVICE_NAME), CROS_GPIO_WP_AH(GPIO_SPI_WP, CROS_GPIO_DEVICE_NAME), };
diff --git a/src/mainboard/samsung/stumpy/Kconfig b/src/mainboard/samsung/stumpy/Kconfig index 36e28ff..d17dc68 100644 --- a/src/mainboard/samsung/stumpy/Kconfig +++ b/src/mainboard/samsung/stumpy/Kconfig @@ -21,7 +21,6 @@ select INTEL_INT15
config VBOOT - select VBOOT_PHYSICAL_DEV_SWITCH select VBOOT_PHYSICAL_REC_SWITCH select VBOOT_VBNV_CMOS
diff --git a/src/mainboard/samsung/stumpy/chromeos.c b/src/mainboard/samsung/stumpy/chromeos.c index d055444..295c31f 100644 --- a/src/mainboard/samsung/stumpy/chromeos.c +++ b/src/mainboard/samsung/stumpy/chromeos.c @@ -24,16 +24,14 @@
#define GPIO_SPI_WP 68 #define GPIO_REC_MODE 42 -#define GPIO_DEV_MODE 17
#define FLAG_SPI_WP 0 #define FLAG_REC_MODE 1 -#define FLAG_DEV_MODE 2
#if ENV_RAMSTAGE #include <boot/coreboot_tables.h>
-#define GPIO_COUNT 6 +#define GPIO_COUNT 5
void fill_lb_gpios(struct lb_gpios *gpios) { @@ -56,29 +54,23 @@ gpios->gpios[1].value = !get_recovery_mode_switch(); strncpy((char *)gpios->gpios[1].name,"recovery", GPIO_MAX_NAME_LENGTH);
- /* Developer: GPIO17 = KBC3_DVP_MODE */ - gpios->gpios[2].port = GPIO_DEV_MODE; - gpios->gpios[2].polarity = ACTIVE_HIGH; - gpios->gpios[2].value = get_developer_mode_switch(); - strncpy((char *)gpios->gpios[2].name,"developer", GPIO_MAX_NAME_LENGTH); - /* Hard code the lid switch GPIO to open. */ - gpios->gpios[3].port = 100; - gpios->gpios[3].polarity = ACTIVE_HIGH; - gpios->gpios[3].value = 1; - strncpy((char *)gpios->gpios[3].name,"lid", GPIO_MAX_NAME_LENGTH); + gpios->gpios[2].port = 100; + gpios->gpios[2].polarity = ACTIVE_HIGH; + gpios->gpios[2].value = 1; + strncpy((char *)gpios->gpios[2].name,"lid", GPIO_MAX_NAME_LENGTH);
/* Power Button */ - gpios->gpios[4].port = 101; - gpios->gpios[4].polarity = ACTIVE_LOW; - gpios->gpios[4].value = (gen_pmcon_1 >> 9) & 1; - strncpy((char *)gpios->gpios[4].name,"power", GPIO_MAX_NAME_LENGTH); + gpios->gpios[3].port = 101; + gpios->gpios[3].polarity = ACTIVE_LOW; + gpios->gpios[3].value = (gen_pmcon_1 >> 9) & 1; + strncpy((char *)gpios->gpios[3].name,"power", GPIO_MAX_NAME_LENGTH);
/* Did we load the VGA Option ROM? */ - gpios->gpios[5].port = -1; /* Indicate that this is a pseudo GPIO */ - gpios->gpios[5].polarity = ACTIVE_HIGH; - gpios->gpios[5].value = gfx_get_init_done(); - strncpy((char *)gpios->gpios[5].name,"oprom", GPIO_MAX_NAME_LENGTH); + gpios->gpios[4].port = -1; /* Indicate that this is a pseudo GPIO */ + gpios->gpios[4].polarity = ACTIVE_HIGH; + gpios->gpios[4].value = gfx_get_init_done(); + strncpy((char *)gpios->gpios[4].name,"oprom", GPIO_MAX_NAME_LENGTH); } #endif
@@ -92,16 +84,6 @@ return (pci_read_config32(dev, SATA_SP) >> FLAG_SPI_WP) & 1; }
-int get_developer_mode_switch(void) -{ -#ifdef __SIMPLE_DEVICE__ - pci_devfn_t dev = PCI_DEV(0, 0x1f, 2); -#else - struct device *dev = pcidev_on_root(0x1f, 2); -#endif - return (pci_read_config32(dev, SATA_SP) >> FLAG_DEV_MODE) & 1; -} - int get_recovery_mode_switch(void) { #ifdef __SIMPLE_DEVICE__ @@ -127,16 +109,12 @@ /* Recovery: GPIO42 = CHP3_REC_MODE#, active low */ if (!get_gpio(GPIO_REC_MODE)) flags |= (1 << FLAG_REC_MODE); - /* Developer: GPIO17 = KBC3_DVP_MODE, active high */ - if (get_gpio(GPIO_DEV_MODE)) - flags |= (1 << FLAG_DEV_MODE);
pci_write_config32(dev, SATA_SP, flags); }
static const struct cros_gpio cros_gpios[] = { CROS_GPIO_REC_AL(GPIO_REC_MODE, CROS_GPIO_DEVICE_NAME), - CROS_GPIO_DEV_AH(GPIO_DEV_MODE, CROS_GPIO_DEVICE_NAME), CROS_GPIO_WP_AH(GPIO_SPI_WP, CROS_GPIO_DEVICE_NAME), };
diff --git a/src/security/vboot/Kconfig b/src/security/vboot/Kconfig index 42e7b19..ca25423 100644 --- a/src/security/vboot/Kconfig +++ b/src/security/vboot/Kconfig @@ -191,15 +191,6 @@ bool default n
-config VBOOT_PHYSICAL_DEV_SWITCH - bool - default n - help - Whether this platform has a physical developer switch. Note that this - disables virtual dev switch functionality (through secdata). Operation - where both a physical pin and the virtual switch get sampled is not - supported by coreboot. - config VBOOT_PHYSICAL_REC_SWITCH bool default n diff --git a/src/security/vboot/vboot_handoff.c b/src/security/vboot/vboot_handoff.c index cbfedf5..11831d5 100644 --- a/src/security/vboot/vboot_handoff.c +++ b/src/security/vboot/vboot_handoff.c @@ -79,8 +79,7 @@ vb_sd->flags |= VBSD_LF_DEV_SWITCH_ON; } /* TODO: Set these in depthcharge */ - if (!CONFIG(VBOOT_PHYSICAL_DEV_SWITCH)) - vb_sd->flags |= VBSD_HONOR_VIRT_DEV_SWITCH; + vb_sd->flags |= VBSD_HONOR_VIRT_DEV_SWITCH; if (!CONFIG(VBOOT_PHYSICAL_REC_SWITCH)) vb_sd->flags |= VBSD_BOOT_REC_SWITCH_VIRTUAL; if (CONFIG(VBOOT_OPROM_MATTERS)) { diff --git a/src/security/vboot/vboot_logic.c b/src/security/vboot/vboot_logic.c index 0b5763b..ac3d3c3 100644 --- a/src/security/vboot/vboot_logic.c +++ b/src/security/vboot/vboot_logic.c @@ -324,10 +324,6 @@ die("Initializing measured boot mode failed!"); }
- if (CONFIG(VBOOT_PHYSICAL_DEV_SWITCH) && - get_developer_mode_switch()) - ctx.flags |= VB2_CONTEXT_FORCE_DEVELOPER_MODE; - if (get_recovery_mode_switch()) { ctx.flags |= VB2_CONTEXT_FORCE_RECOVERY_MODE; if (CONFIG(VBOOT_DISABLE_DEV_ON_RECOVERY))
-
build bot (Jenkins) (Code Review) -
Joel Kitching (Code Review)
-
Julius Werner (Code Review)
-
Kyösti Mälkki (Code Review)
-
Simon Glass (Code Review)