Rob Barnes has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/43176 )
Change subject: mb/google/zork: Add VBOOT_VBNV_CMOS flag ......................................................................
mb/google/zork: Add VBOOT_VBNV_CMOS flag
Add VBOOT_VBNV_CMOS flag for zork boards.
BUG=b:160297933 TEST=Enter and leave developer mode on Ezkinil
Change-Id: I19ec139c8f098da630eeab199fec55297e3d977c Signed-off-by: Rob Barnes robbarnes@google.com --- M src/mainboard/google/zork/Kconfig 1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/43176/1
diff --git a/src/mainboard/google/zork/Kconfig b/src/mainboard/google/zork/Kconfig index e24f78a..16fa6e8 100644 --- a/src/mainboard/google/zork/Kconfig +++ b/src/mainboard/google/zork/Kconfig @@ -102,6 +102,7 @@ select VBOOT_LID_SWITCH select VBOOT_STARTS_IN_BOOTBLOCK select VBOOT_SEPARATE_VERSTAGE + select VBOOT_VBNV_CMOS
config VBOOT_VBNV_OFFSET hex
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43176 )
Change subject: mb/google/zork: Add VBOOT_VBNV_CMOS flag ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43176/1/src/mainboard/google/zork/K... File src/mainboard/google/zork/Kconfig:
https://review.coreboot.org/c/coreboot/+/43176/1/src/mainboard/google/zork/K... PS1, Line 105: select VBOOT_VBNV_CMOS VBNV_FLASH should be selected first and foremost.
Hello build bot (Jenkins), Martin Roth, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43176
to look at the new patch set (#2).
Change subject: mb/google/zork: Add VBOOT_VBNV_CMOS flag ......................................................................
mb/google/zork: Add VBOOT_VBNV_CMOS flag
Add VBOOT_VBNV_CMOS flag for zork boards.
BUG=b:160297933 TEST=Enter and leave developer mode on Ezkinil
Change-Id: I19ec139c8f098da630eeab199fec55297e3d977c Signed-off-by: Rob Barnes robbarnes@google.com --- M src/mainboard/google/zork/Kconfig 1 file changed, 2 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/43176/2
Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43176 )
Change subject: mb/google/zork: Add VBOOT_VBNV_CMOS flag ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43176/1/src/mainboard/google/zork/K... File src/mainboard/google/zork/Kconfig:
https://review.coreboot.org/c/coreboot/+/43176/1/src/mainboard/google/zork/K... PS1, Line 105: select VBOOT_VBNV_CMOS
VBNV_FLASH should be selected first and foremost.
I propose we unblock with CMOS here and track switching to VBNV_FLASH with b/160682256.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43176 )
Change subject: mb/google/zork: Add VBOOT_VBNV_CMOS flag ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43176/2/src/mainboard/google/zork/K... File src/mainboard/google/zork/Kconfig:
https://review.coreboot.org/c/coreboot/+/43176/2/src/mainboard/google/zork/K... PS2, Line 106: select VBOOT_VBNV_CMOS_BACKUP_TO_FLASH Did you check that VBOOT_VBNV_OFFSET does not collide with existing uses in the cmos? There are some cmos bugs in buganizer. It looks to be 0x2a below, but we're really adding 14 to the offset for 16 bytes.
So we're really using 0x38 -> 0x48.
Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43176 )
Change subject: mb/google/zork: Add VBOOT_VBNV_CMOS flag ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43176/2/src/mainboard/google/zork/K... File src/mainboard/google/zork/Kconfig:
https://review.coreboot.org/c/coreboot/+/43176/2/src/mainboard/google/zork/K... PS2, Line 106: select VBOOT_VBNV_CMOS_BACKUP_TO_FLASH
Did you check that VBOOT_VBNV_OFFSET does not collide with existing uses in the cmos? There are some […]
I think you are referring to b/148164303? I don't see any conflicts there. I resummarized your comment there too.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43176 )
Change subject: mb/google/zork: Add VBOOT_VBNV_CMOS flag ......................................................................
Patch Set 2: Code-Review+2
Hello build bot (Jenkins), Martin Roth, Marshall Dawson, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43176
to look at the new patch set (#3).
Change subject: mb/google/zork: Add VBOOT_VBNV_CMOS flag ......................................................................
mb/google/zork: Add VBOOT_VBNV_CMOS flag
Add VBOOT_VBNV_CMOS and VBOOT_VBNV_CMOS_BACKUP_TO_FLASH flag for zork boards.
BUG=b:160297933 TEST=Enter and leave developer mode on Ezkinil
Change-Id: I19ec139c8f098da630eeab199fec55297e3d977c Signed-off-by: Rob Barnes robbarnes@google.com --- M src/mainboard/google/zork/Kconfig 1 file changed, 2 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/43176/3
Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43176 )
Change subject: mb/google/zork: Add VBOOT_VBNV_CMOS flag ......................................................................
Patch Set 3:
(1 comment)
Lost +2 due to rebase conflict.
https://review.coreboot.org/c/coreboot/+/43176/1/src/mainboard/google/zork/K... File src/mainboard/google/zork/Kconfig:
https://review.coreboot.org/c/coreboot/+/43176/1/src/mainboard/google/zork/K... PS1, Line 105: select VBOOT_VBNV_CMOS
I propose we unblock with CMOS here and track switching to VBNV_FLASH with b/160682256.
Resolving, reopen if you disagree.
Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43176 )
Change subject: mb/google/zork: Add VBOOT_VBNV_CMOS flag ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43176/2/src/mainboard/google/zork/K... File src/mainboard/google/zork/Kconfig:
https://review.coreboot.org/c/coreboot/+/43176/2/src/mainboard/google/zork/K... PS2, Line 106: select VBOOT_VBNV_CMOS_BACKUP_TO_FLASH
I think you are referring to b/148164303? I don't see any conflicts there. […]
Resolving.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43176 )
Change subject: mb/google/zork: Add VBOOT_VBNV_CMOS flag ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43176/3/src/mainboard/google/zork/K... File src/mainboard/google/zork/Kconfig:
https://review.coreboot.org/c/coreboot/+/43176/3/src/mainboard/google/zork/K... PS3, Line 103: select VBOOT_VBNV_CMOS These aren't needed in this file. They get selected in the SOC kconfig file.
https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/src...
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43176 )
Change subject: mb/google/zork: Add VBOOT_VBNV_CMOS flag ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43176/3/src/mainboard/google/zork/K... File src/mainboard/google/zork/Kconfig:
https://review.coreboot.org/c/coreboot/+/43176/3/src/mainboard/google/zork/K... PS3, Line 103: select VBOOT_VBNV_CMOS
These aren't needed in this file. They get selected in the SOC kconfig file. […]
Martin, why was the backup to flash made conditional? We need to make that work no matter what.
Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43176 )
Change subject: mb/google/zork: Add VBOOT_VBNV_CMOS flag ......................................................................
Patch Set 3:
(1 comment)
Abandoning because https://review.coreboot.org/c/coreboot/+/41816 merged.
https://review.coreboot.org/c/coreboot/+/43176/3/src/mainboard/google/zork/K... File src/mainboard/google/zork/Kconfig:
https://review.coreboot.org/c/coreboot/+/43176/3/src/mainboard/google/zork/K... PS3, Line 103: select VBOOT_VBNV_CMOS
Martin, why was the backup to flash made conditional? We need to make that work no matter what.
Indeed it is. You merged it today :) https://review.coreboot.org/c/coreboot/+/41816. I'll abandon this change.
Rob Barnes has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/43176 )
Change subject: mb/google/zork: Add VBOOT_VBNV_CMOS flag ......................................................................
Abandoned
https://review.coreboot.org/c/coreboot/+/41816
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43176 )
Change subject: mb/google/zork: Add VBOOT_VBNV_CMOS flag ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43176/3/src/mainboard/google/zork/K... File src/mainboard/google/zork/Kconfig:
https://review.coreboot.org/c/coreboot/+/43176/3/src/mainboard/google/zork/K... PS3, Line 103: select VBOOT_VBNV_CMOS
Indeed it is. You merged it today :) https://review.coreboot.org/c/coreboot/+/41816. […]
Fair enough, it's been so long since I uploaded it, I guess I just thought the code was already merged.
As far as why it's conditional, it's just not in place yet. I agree that we need to get it working, and that will be one of the next pieces I work on.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43176 )
Change subject: mb/google/zork: Add VBOOT_VBNV_CMOS flag ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43176/3/src/mainboard/google/zork/K... File src/mainboard/google/zork/Kconfig:
https://review.coreboot.org/c/coreboot/+/43176/3/src/mainboard/google/zork/K... PS3, Line 103: select VBOOT_VBNV_CMOS
Fair enough, it's been so long since I uploaded it, I guess I just thought the code was already merg […]
Sounds good. Let me know if you need any help.
-
Aaron Durbin (Code Review) -
Martin Roth (Code Review)
-
Rob Barnes (Code Review)