coreboot-gerrit October 2022

coreboot-gerrit@coreboot.org

1 participants
5881 discussions

[M] Change in coreboot[master]: mb/amd,google: move fch_irq_routing struct definition to soc/amd
by Felix Held (Code Review) 26 Oct '22

26 Oct '22

Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/68817 ) ( 1 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )Change subject: mb/amd,google: move fch_irq_routing struct definition to soc/amd ...................................................................... mb/amd,google: move fch_irq_routing struct definition to soc/amd Define the fch_irq_routing struct once in a common header file instead of in every mainboard's code. Signed-off-by: Felix Held <felix-coreboot(a)felixheld.de> Change-Id: I11d9000b6ed7529e4afd7f6e8a7332c390da6dab Reviewed-on: https://review.coreboot.org/c/coreboot/+/68817 Reviewed-by: Fred Reitberger <reitbergerfred(a)gmail.com> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M src/mainboard/amd/bilby/mainboard.c M src/mainboard/amd/birman/mainboard.c M src/mainboard/amd/chausie/mainboard.c M src/mainboard/amd/majolica/mainboard.c M src/mainboard/amd/mandolin/mainboard.c M src/mainboard/google/guybrush/mainboard.c M src/mainboard/google/skyrim/mainboard.c M src/mainboard/google/zork/mainboard.c M src/soc/amd/common/block/include/amdblocks/amd_pci_util.h 9 files changed, 30 insertions(+), 40 deletions(-) Approvals: build bot (Jenkins): Verified Fred Reitberger: Looks good to me, approved diff --git a/src/mainboard/amd/bilby/mainboard.c b/src/mainboard/amd/bilby/mainboard.c index f87046a..c402c29 100644 --- a/src/mainboard/amd/bilby/mainboard.c +++ b/src/mainboard/amd/bilby/mainboard.c @@ -33,11 +33,7 @@ _Static_assert(sizeof(fch_pic_routing) == sizeof(fch_apic_routing), "PIC and APIC FCH interrupt tables must be the same size"); -static const struct fch_irq_routing { - uint8_t intr_index; - uint8_t pic_irq_num; - uint8_t apic_irq_num; -} bilby_fch[] = { +static const struct fch_irq_routing bilby_fch[] = { { PIRQ_A, 8, 16 }, { PIRQ_B, 10, 17 }, { PIRQ_C, 11, 18 }, diff --git a/src/mainboard/amd/birman/mainboard.c b/src/mainboard/amd/birman/mainboard.c index ffb778c..34c3b4d 100644 --- a/src/mainboard/amd/birman/mainboard.c +++ b/src/mainboard/amd/birman/mainboard.c @@ -34,11 +34,7 @@ * 8: rtc0 <- soc/amd/common/acpi/lpc.asl * 9: acpi <- soc/amd/common/acpi/lpc.asl */ -static const struct fch_irq_routing { - uint8_t intr_index; - uint8_t pic_irq_num; - uint8_t apic_irq_num; -} birman_fch[] = { +static const struct fch_irq_routing birman_fch[] = { { PIRQ_A, 12, PIRQ_NC }, { PIRQ_B, 14, PIRQ_NC }, { PIRQ_C, 15, PIRQ_NC }, diff --git a/src/mainboard/amd/chausie/mainboard.c b/src/mainboard/amd/chausie/mainboard.c index 2966b55..9251ff2 100644 --- a/src/mainboard/amd/chausie/mainboard.c +++ b/src/mainboard/amd/chausie/mainboard.c @@ -32,11 +32,7 @@ * 8: rtc0 <- soc/amd/common/acpi/lpc.asl * 9: acpi <- soc/amd/common/acpi/lpc.asl */ -static const struct fch_irq_routing { - uint8_t intr_index; - uint8_t pic_irq_num; - uint8_t apic_irq_num; -} chausie_fch[] = { +static const struct fch_irq_routing chausie_fch[] = { { PIRQ_A, 12, PIRQ_NC }, { PIRQ_B, 14, PIRQ_NC }, { PIRQ_C, 15, PIRQ_NC }, diff --git a/src/mainboard/amd/majolica/mainboard.c b/src/mainboard/amd/majolica/mainboard.c index 199cf47..58f7546 100644 --- a/src/mainboard/amd/majolica/mainboard.c +++ b/src/mainboard/amd/majolica/mainboard.c @@ -31,11 +31,7 @@ * 8: rtc0 <- soc/amd/common/acpi/lpc.asl * 9: acpi <- soc/amd/common/acpi/lpc.asl */ -static const struct fch_irq_routing { - uint8_t intr_index; - uint8_t pic_irq_num; - uint8_t apic_irq_num; -} majolica_fch[] = { +static const struct fch_irq_routing majolica_fch[] = { { PIRQ_A, 12, PIRQ_NC }, { PIRQ_B, 14, PIRQ_NC }, { PIRQ_C, 15, PIRQ_NC }, diff --git a/src/mainboard/amd/mandolin/mainboard.c b/src/mainboard/amd/mandolin/mainboard.c index ccea7c7..0f87d9c 100644 --- a/src/mainboard/amd/mandolin/mainboard.c +++ b/src/mainboard/amd/mandolin/mainboard.c @@ -28,11 +28,7 @@ _Static_assert(sizeof(fch_pic_routing) == sizeof(fch_apic_routing), "PIC and APIC FCH interrupt tables must be the same size"); -static const struct fch_irq_routing { - uint8_t intr_index; - uint8_t pic_irq_num; - uint8_t apic_irq_num; -} mandolin_fch[] = { +static const struct fch_irq_routing mandolin_fch[] = { { PIRQ_A, 8, 16 }, { PIRQ_B, 10, 17 }, { PIRQ_C, 11, 18 }, diff --git a/src/mainboard/google/guybrush/mainboard.c b/src/mainboard/google/guybrush/mainboard.c index 9a1c24d..4bcb52e 100644 --- a/src/mainboard/google/guybrush/mainboard.c +++ b/src/mainboard/google/guybrush/mainboard.c @@ -46,11 +46,7 @@ * 8: rtc0 <- soc/amd/common/acpi/lpc.asl * 9: acpi <- soc/amd/common/acpi/lpc.asl */ -static const struct fch_irq_routing { - uint8_t intr_index; - uint8_t pic_irq_num; - uint8_t apic_irq_num; -} guybrush_fch[] = { +static const struct fch_irq_routing guybrush_fch[] = { { PIRQ_A, 12, PIRQ_NC }, { PIRQ_B, 14, PIRQ_NC }, { PIRQ_C, 15, PIRQ_NC }, diff --git a/src/mainboard/google/skyrim/mainboard.c b/src/mainboard/google/skyrim/mainboard.c index 6af647c..0b707d7 100644 --- a/src/mainboard/google/skyrim/mainboard.c +++ b/src/mainboard/google/skyrim/mainboard.c @@ -33,11 +33,7 @@ * 9: acpi <- soc/amd/common/acpi/lpc.asl */ -static const struct fch_irq_routing { - uint8_t intr_index; - uint8_t pic_irq_num; - uint8_t apic_irq_num; -} skyrim_fch[] = { +static const struct fch_irq_routing skyrim_fch[] = { { PIRQ_A, 12, PIRQ_NC }, { PIRQ_B, 14, PIRQ_NC }, { PIRQ_C, 15, PIRQ_NC }, diff --git a/src/mainboard/google/zork/mainboard.c b/src/mainboard/google/zork/mainboard.c index 82e5c6a..fedf1cc 100644 --- a/src/mainboard/google/zork/mainboard.c +++ b/src/mainboard/google/zork/mainboard.c @@ -56,11 +56,7 @@ * 9: acpi <- soc/amd/common/acpi/lpc.asl * 12: i8042 <- ec/google/chromeec/acpi/superio.asl */ -static const struct fch_irq_routing { - uint8_t intr_index; - uint8_t pic_irq_num; - uint8_t apic_irq_num; -} fch_pirq[] = { +static const struct fch_irq_routing fch_pirq[] = { { PIRQ_A, 6, PIRQ_NC }, { PIRQ_B, 13, PIRQ_NC }, { PIRQ_C, 14, PIRQ_NC }, diff --git a/src/soc/amd/common/block/include/amdblocks/amd_pci_util.h b/src/soc/amd/common/block/include/amdblocks/amd_pci_util.h index 0a4061f..6543853 100644 --- a/src/soc/amd/common/block/include/amdblocks/amd_pci_util.h +++ b/src/soc/amd/common/block/include/amdblocks/amd_pci_util.h @@ -11,6 +11,12 @@ #define PCI_INTR_INDEX 0xc00 #define PCI_INTR_DATA 0xc01 +struct fch_irq_routing { + uint8_t intr_index; + uint8_t pic_irq_num; + uint8_t apic_irq_num; +}; + struct pirq_struct { u8 devfn; u8 PIN[4]; /* PINA/B/C/D are index 0/1/2/3 */ -- To view, visit https://review.coreboot.org/c/coreboot/+/68817 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I11d9000b6ed7529e4afd7f6e8a7332c390da6dab Gerrit-Change-Number: 68817 Gerrit-PatchSet: 3 Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Jason Nien <jason.nien(a)amd.corp-partner.google.com> Gerrit-Reviewer: Martin Roth <martin.roth(a)amd.corp-partner.google.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[S] Change in coreboot[master]: mb/google/brya: Update Crota's ELAN touchscreen delay to 150 ms
by Rajat Jain (Code Review) 26 Oct '22

26 Oct '22

Attention is currently required from: Paz Zcharya, Subrata Banik, Terry Chen, Tim Wawrzynczak, Nick Vaccaro, Franklin Lin. Rajat Jain has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68868 ) Change subject: mb/google/brya: Update Crota's ELAN touchscreen delay to 150 ms ...................................................................... Patch Set 6: (1 comment) File src/mainboard/google/brya/variants/crota/overridetree.cb: https://review.coreboot.org/c/coreboot/+/68868/comment/ecb91022_474fbf37 PS6, Line 225: 150 > > Oh I think Subrata is saying why do we need 150ms on Crota? If so, yes I think I agree with Paz's […] This delay is "parallelized" with other initializatoin operations, and since they are in seconds, this delay does not matter. (Even if we had 0 delay, ther delays in other drivers take care of it at boot time). But then we introduce this unstated dependency on other drivers taking > x ms. That is problem 1. Other problem is that at resume time, things go a lot faster (our resume time expectation is within 500ms). There also we parallelize this delay, but we'd like to have a way to ensure what is the minimum delay before which the touschscreen can become funtional. -- To view, visit https://review.coreboot.org/c/coreboot/+/68868 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I17e1f7d419637f6dff4049484ce1836ad98017ea Gerrit-Change-Number: 68868 Gerrit-PatchSet: 6 Gerrit-Owner: Paz Zcharya <pazz(a)google.com> Gerrit-Reviewer: Eran Mitrani <mitrani(a)google.com> Gerrit-Reviewer: Franklin Lin <franklin_lin(a)wistron.corp-partner.google.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com> Gerrit-Reviewer: Terry Chen <terry_chen(a)wistron.corp-partner.google.com> Gerrit-Reviewer: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Rajat Jain <rajatja(a)google.com> Gerrit-Attention: Paz Zcharya <pazz(a)google.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Terry Chen <terry_chen(a)wistron.corp-partner.google.com> Gerrit-Attention: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Attention: Franklin Lin <franklin_lin(a)wistron.corp-partner.google.com> Gerrit-Comment-Date: Wed, 26 Oct 2022 17:32:48 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Paz Zcharya <pazz(a)google.com> Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Rajat Jain <rajatja(a)google.com> Gerrit-MessageType: comment

1 0

[S] Change in coreboot[master]: mb/google/brya: Update Crota's ELAN touchscreen delay to 150 ms
by Tarun Tuli (Code Review) 26 Oct '22

26 Oct '22

Attention is currently required from: Paz Zcharya, Subrata Banik, Terry Chen, Tim Wawrzynczak, Nick Vaccaro, Franklin Lin, Rajat Jain. Tarun Tuli has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68868 ) Change subject: mb/google/brya: Update Crota's ELAN touchscreen delay to 150 ms ...................................................................... Patch Set 6: Code-Review+2 (1 comment) File src/mainboard/google/brya/variants/crota/overridetree.cb: https://review.coreboot.org/c/coreboot/+/68868/comment/2060cc16_ff750705 PS6, Line 225: 150 > > Oh I think Subrata is saying why do we need 150ms on Crota? If so, yes I think I agree with Paz's […] I think we really need to ensure we are meeting the timings and not hope we inherently get enough. The spec specifies two timings, one before we can send for a i2c ack (20ms) and one for i2c hid/enumeration to start (150ms). We'd want to see which is happening first and how long that is after we de-assert reset. -- To view, visit https://review.coreboot.org/c/coreboot/+/68868 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I17e1f7d419637f6dff4049484ce1836ad98017ea Gerrit-Change-Number: 68868 Gerrit-PatchSet: 6 Gerrit-Owner: Paz Zcharya <pazz(a)google.com> Gerrit-Reviewer: Eran Mitrani <mitrani(a)google.com> Gerrit-Reviewer: Franklin Lin <franklin_lin(a)wistron.corp-partner.google.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com> Gerrit-Reviewer: Terry Chen <terry_chen(a)wistron.corp-partner.google.com> Gerrit-Reviewer: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Rajat Jain <rajatja(a)google.com> Gerrit-Attention: Paz Zcharya <pazz(a)google.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Terry Chen <terry_chen(a)wistron.corp-partner.google.com> Gerrit-Attention: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Attention: Franklin Lin <franklin_lin(a)wistron.corp-partner.google.com> Gerrit-Attention: Rajat Jain <rajatja(a)google.com> Gerrit-Comment-Date: Wed, 26 Oct 2022 17:30:03 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Paz Zcharya <pazz(a)google.com> Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Rajat Jain <rajatja(a)google.com> Gerrit-MessageType: comment

1 0

[S] Change in coreboot[master]: mb/google/brya: Update Crota's ELAN touchscreen delay to 150 ms
by Subrata Banik (Code Review) 26 Oct '22

26 Oct '22

Attention is currently required from: Paz Zcharya, Terry Chen, Tim Wawrzynczak, Nick Vaccaro, Franklin Lin, Rajat Jain. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68868 ) Change subject: mb/google/brya: Update Crota's ELAN touchscreen delay to 150 ms ...................................................................... Patch Set 6: (1 comment) File src/mainboard/google/brya/variants/crota/overridetree.cb: https://review.coreboot.org/c/coreboot/+/68868/comment/3d9bbc1b_3a5fbf4f PS6, Line 225: 150 > Oh I think Subrata is saying why do we need 150ms on Crota? If so, yes I think I agree with Paz's response above. Probably this delay doesn't matter at bootup given other delays, but will have an impact on resume time. exactly, IMO, those delays can be avoided (based on the understanding of the touch FW) as we don't need to explicitly give any static delay. The delays can be inheried between the operations. -- To view, visit https://review.coreboot.org/c/coreboot/+/68868 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I17e1f7d419637f6dff4049484ce1836ad98017ea Gerrit-Change-Number: 68868 Gerrit-PatchSet: 6 Gerrit-Owner: Paz Zcharya <pazz(a)google.com> Gerrit-Reviewer: Eran Mitrani <mitrani(a)google.com> Gerrit-Reviewer: Franklin Lin <franklin_lin(a)wistron.corp-partner.google.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com> Gerrit-Reviewer: Terry Chen <terry_chen(a)wistron.corp-partner.google.com> Gerrit-Reviewer: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Rajat Jain <rajatja(a)google.com> Gerrit-Attention: Paz Zcharya <pazz(a)google.com> Gerrit-Attention: Terry Chen <terry_chen(a)wistron.corp-partner.google.com> Gerrit-Attention: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Attention: Franklin Lin <franklin_lin(a)wistron.corp-partner.google.com> Gerrit-Attention: Rajat Jain <rajatja(a)google.com> Gerrit-Comment-Date: Wed, 26 Oct 2022 17:26:51 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Paz Zcharya <pazz(a)google.com> Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Rajat Jain <rajatja(a)google.com> Gerrit-MessageType: comment

1 0

[S] Change in coreboot[master]: mb/google/brya: Update Crota's ELAN touchscreen delay to 150 ms
by Rajat Jain (Code Review) 26 Oct '22

26 Oct '22

Attention is currently required from: Paz Zcharya, Subrata Banik, Terry Chen, Tim Wawrzynczak, Nick Vaccaro, Franklin Lin. Rajat Jain has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68868 ) Change subject: mb/google/brya: Update Crota's ELAN touchscreen delay to 150 ms ...................................................................... Patch Set 6: (1 comment) File src/mainboard/google/brya/variants/crota/overridetree.cb: https://review.coreboot.org/c/coreboot/+/68868/comment/f073649b_f9350d93 PS6, Line 225: 150 > It is my understanding that this CL ONLY affects Crota device. […] Oh I think Subrata is saying why do we need 150ms on Crota? If so, yes I think I agree with Paz's response above. Probably this delay doesn't matter at bootup given other delays, but will have an impact on resume time. -- To view, visit https://review.coreboot.org/c/coreboot/+/68868 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I17e1f7d419637f6dff4049484ce1836ad98017ea Gerrit-Change-Number: 68868 Gerrit-PatchSet: 6 Gerrit-Owner: Paz Zcharya <pazz(a)google.com> Gerrit-Reviewer: Eran Mitrani <mitrani(a)google.com> Gerrit-Reviewer: Franklin Lin <franklin_lin(a)wistron.corp-partner.google.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com> Gerrit-Reviewer: Terry Chen <terry_chen(a)wistron.corp-partner.google.com> Gerrit-Reviewer: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Rajat Jain <rajatja(a)google.com> Gerrit-Attention: Paz Zcharya <pazz(a)google.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Terry Chen <terry_chen(a)wistron.corp-partner.google.com> Gerrit-Attention: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Attention: Franklin Lin <franklin_lin(a)wistron.corp-partner.google.com> Gerrit-Comment-Date: Wed, 26 Oct 2022 17:23:23 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Paz Zcharya <pazz(a)google.com> Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Rajat Jain <rajatja(a)google.com> Gerrit-MessageType: comment

1 0

[S] Change in coreboot[master]: mb/google/brya: Update Crota's ELAN touchscreen delay to 150 ms
by Rajat Jain (Code Review) 26 Oct '22

26 Oct '22

Attention is currently required from: Paz Zcharya, Subrata Banik, Terry Chen, Tim Wawrzynczak, Nick Vaccaro, Franklin Lin. Rajat Jain has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68868 ) Change subject: mb/google/brya: Update Crota's ELAN touchscreen delay to 150 ms ...................................................................... Patch Set 6: (1 comment) File src/mainboard/google/brya/variants/crota/overridetree.cb: https://review.coreboot.org/c/coreboot/+/68868/comment/6efaead0_7fbd1d63 PS6, Line 225: 150 > Tarun pointed out that the delay requirement depends on the type of communication (ack vs. hid). […] It is my understanding that this CL ONLY affects Crota device. I'm confused as to why are we discussing Rex here? -- To view, visit https://review.coreboot.org/c/coreboot/+/68868 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I17e1f7d419637f6dff4049484ce1836ad98017ea Gerrit-Change-Number: 68868 Gerrit-PatchSet: 6 Gerrit-Owner: Paz Zcharya <pazz(a)google.com> Gerrit-Reviewer: Eran Mitrani <mitrani(a)google.com> Gerrit-Reviewer: Franklin Lin <franklin_lin(a)wistron.corp-partner.google.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com> Gerrit-Reviewer: Terry Chen <terry_chen(a)wistron.corp-partner.google.com> Gerrit-Reviewer: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Rajat Jain <rajatja(a)google.com> Gerrit-Attention: Paz Zcharya <pazz(a)google.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Terry Chen <terry_chen(a)wistron.corp-partner.google.com> Gerrit-Attention: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Attention: Franklin Lin <franklin_lin(a)wistron.corp-partner.google.com> Gerrit-Comment-Date: Wed, 26 Oct 2022 17:21:31 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Paz Zcharya <pazz(a)google.com> Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Gerrit-MessageType: comment

1 0

[S] Change in coreboot[master]: mb/google/skyrim: Enable CBFS Verification
by Raul Rangel (Code Review) 26 Oct '22

26 Oct '22

Raul Rangel has submitted this change. ( https://review.coreboot.org/c/coreboot/+/66948 ) Change subject: mb/google/skyrim: Enable CBFS Verification ...................................................................... mb/google/skyrim: Enable CBFS Verification Enable RO verification by GSC and CBFS verification. BUG=b:227809919 TEST=Build and boot to OS in Skyrim with CBFS verification enabled using x86 verstage and PSP verstage. Change-Id: Idd22a521a913705af0d2aca17acd1aa069a77f29 Signed-off-by: Karthikeyan Ramasubramanian <kramasub(a)google.com> Reviewed-on: https://review.coreboot.org/c/coreboot/+/66948 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Raul Rangel <rrangel(a)chromium.org> --- M src/mainboard/google/skyrim/Kconfig 1 file changed, 19 insertions(+), 5 deletions(-) Approvals: build bot (Jenkins): Verified Raul Rangel: Looks good to me, approved diff --git a/src/mainboard/google/skyrim/Kconfig b/src/mainboard/google/skyrim/Kconfig index cb1932f..e746af1 100644 --- a/src/mainboard/google/skyrim/Kconfig +++ b/src/mainboard/google/skyrim/Kconfig @@ -105,11 +105,6 @@ bool default y if VBOOT && !VBOOT_STARTS_BEFORE_BOOTBLOCK -# TODO: Remove once CBFS verification on AMD has been fixed. -config VBOOT_GSCVD - bool - default n - if !EM100 # EM100 defaults in soc/amd/common/blocks/spi/Kconfig config EFS_SPI_READ_MODE default 2 # Dual IO (1-1-2) -- To view, visit https://review.coreboot.org/c/coreboot/+/66948 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Idd22a521a913705af0d2aca17acd1aa069a77f29 Gerrit-Change-Number: 66948 Gerrit-PatchSet: 15 Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Jason Nien <jason.nien(a)amd.corp-partner.google.com> Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Reviewer: Martin Roth <martin.roth(a)amd.corp-partner.google.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[S] Change in coreboot[master]: cbfs_verification: Remove dependency on VBOOT_STARTS_BEFORE_BOOTBLOCK
by Raul Rangel (Code Review) 26 Oct '22

26 Oct '22

Raul Rangel has submitted this change. ( https://review.coreboot.org/c/coreboot/+/66947 ) Change subject: cbfs_verification: Remove dependency on VBOOT_STARTS_BEFORE_BOOTBLOCK ...................................................................... cbfs_verification: Remove dependency on VBOOT_STARTS_BEFORE_BOOTBLOCK CBFS verification on boards where VBOOT starts before bootblock eg. PSP verstage has been accommodated by keeping metadata hash outside the bootblock. Hence the dependency can be removed. BUG=b:227809919 TEST=Build and boot to OS in skyrim with CBFS verification enabled using both x86 verstage and PSP verstage. Change-Id: I0a3254728a51a8ee7d7782afcea15ea06d93da7d Signed-off-by: Karthikeyan Ramasubramanian <kramasub(a)google.com> Reviewed-on: https://review.coreboot.org/c/coreboot/+/66947 Reviewed-by: Julius Werner <jwerner(a)chromium.org> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Raul Rangel <rrangel(a)chromium.org> --- M src/lib/Kconfig.cbfs_verification 1 file changed, 22 insertions(+), 1 deletion(-) Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved Raul Rangel: Looks good to me, approved diff --git a/src/lib/Kconfig.cbfs_verification b/src/lib/Kconfig.cbfs_verification index 9a9ba31..9b053e1 100644 --- a/src/lib/Kconfig.cbfs_verification +++ b/src/lib/Kconfig.cbfs_verification @@ -6,7 +6,6 @@ config CBFS_VERIFICATION bool "Enable CBFS verification" - depends on !VBOOT_STARTS_BEFORE_BOOTBLOCK # this is gonna get tricky... select VBOOT_LIB help Say yes here to enable code that cryptographically verifies each CBFS -- To view, visit https://review.coreboot.org/c/coreboot/+/66947 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0a3254728a51a8ee7d7782afcea15ea06d93da7d Gerrit-Change-Number: 66947 Gerrit-PatchSet: 14 Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[M] Change in coreboot[master]: soc/amd/common/psp_verstage: Pass SRAM buffer to Crypto Engine
by Raul Rangel (Code Review) 26 Oct '22

26 Oct '22

Raul Rangel has submitted this change. ( https://review.coreboot.org/c/coreboot/+/68184 ) Change subject: soc/amd/common/psp_verstage: Pass SRAM buffer to Crypto Engine ...................................................................... soc/amd/common/psp_verstage: Pass SRAM buffer to Crypto Engine Crypto engine prefers the buffer from SRAM. CBFS verification may pass the mapped address of a CBFS file from SPI flash. This causes PSP crypto engine to return invalid address. Hence if the buffer is from SRAM, pass it directly to crypto engine. Else copy into a temporary buffer before passing it to crypto engine. BUG=b:b:227809919 TEST=Build and boot to OS in skyrim with CBFS verification enabled using both x86 verstage and PSP verstage. Change-Id: Ie9bc9e786f302e7938969c8093d5405b5a85b711 Signed-off-by: Karthikeyan Ramasubramanian <kramasub(a)google.com> Reviewed-on: https://review.coreboot.org/c/coreboot/+/68184 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Raul Rangel <rrangel(a)chromium.org> --- M src/soc/amd/common/psp_verstage/vboot_crypto.c 1 file changed, 60 insertions(+), 1 deletion(-) Approvals: build bot (Jenkins): Verified Raul Rangel: Looks good to me, approved diff --git a/src/soc/amd/common/psp_verstage/vboot_crypto.c b/src/soc/amd/common/psp_verstage/vboot_crypto.c index 81a6740..8c1f313 100644 --- a/src/soc/amd/common/psp_verstage/vboot_crypto.c +++ b/src/soc/amd/common/psp_verstage/vboot_crypto.c @@ -6,9 +6,11 @@ #include <commonlib/bsd/helpers.h> #include <console/console.h> #include "psp_verstage.h" +#include <soc/psp_verstage_addr.h> #include <stddef.h> #include <string.h> #include <swab.h> +#include <symbols.h> #include <vb2_api.h> static struct sha_generic_data sha_op; @@ -40,9 +42,10 @@ return VB2_SUCCESS; } -vb2_error_t vb2ex_hwcrypto_digest_extend(const uint8_t *buf, uint32_t size) +static vb2_error_t vb2ex_hwcrypto_digest_extend_psp_sram(const uint8_t *buf, uint32_t size) { uint32_t retval; + sha_op.Data = (uint8_t *)buf; if (!sha_op_size_remaining) { @@ -76,6 +79,39 @@ return VB2_SUCCESS; } + +vb2_error_t vb2ex_hwcrypto_digest_extend(const uint8_t *buf, uint32_t size) +{ + vb2_error_t retval; + uint32_t offset = 0, copy_size; + + /* + * Crypto engine prefers the buffer from SRAM. CBFS verification may pass the + * mapped address of SPI flash which makes crypto engine to return invalid address. + * Hence if the buffer is from SRAM, pass it to crypto engine. Else copy into a + * temporary buffer before passing it to crypto engine. + */ + if (buf >= _sram && (buf + size) < _esram) + return vb2ex_hwcrypto_digest_extend_psp_sram(buf, size); + + while (size) { + uint8_t block[CONFIG_VBOOT_HASH_BLOCK_SIZE]; + + copy_size = size < CONFIG_VBOOT_HASH_BLOCK_SIZE ? + size : CONFIG_VBOOT_HASH_BLOCK_SIZE; + memcpy(block, buf + offset, copy_size); + + retval = vb2ex_hwcrypto_digest_extend_psp_sram(block, copy_size); + if (retval != VB2_SUCCESS) + return retval; + + size -= copy_size; + offset += copy_size; + } + + return VB2_SUCCESS; +} + /* Copy the hash back to verstage */ vb2_error_t vb2ex_hwcrypto_digest_finalize(uint8_t *digest, uint32_t digest_size) { -- To view, visit https://review.coreboot.org/c/coreboot/+/68184 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie9bc9e786f302e7938969c8093d5405b5a85b711 Gerrit-Change-Number: 68184 Gerrit-PatchSet: 11 Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[M] Change in coreboot[master]: soc/amd/common/psp_verstage: Pass SRAM buffer to Crypto Engine
by Raul Rangel (Code Review) 26 Oct '22

26 Oct '22

Attention is currently required from: Jason Glenesk, Matt DeVillier, Fred Reitberger, Karthik Ramasubramanian, Felix Held. Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68184 ) Change subject: soc/amd/common/psp_verstage: Pass SRAM buffer to Crypto Engine ...................................................................... Patch Set 10: Code-Review+2 (1 comment) File src/soc/amd/common/psp_verstage/vboot_crypto.c: https://review.coreboot.org/c/coreboot/+/68184/comment/631f1fa8_c81fc36d PS10, Line 98: CONFIG_VBOOT_HASH_BLOCK_SIZE > My testing indicated it should not. […] Hrmm: https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/sr… I guess the stack is at the very end: https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/sr… so if it does overflow we will panic. -- To view, visit https://review.coreboot.org/c/coreboot/+/68184 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie9bc9e786f302e7938969c8093d5405b5a85b711 Gerrit-Change-Number: 68184 Gerrit-PatchSet: 10 Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Wed, 26 Oct 2022 17:18:59 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Raul Rangel <rrangel(a)chromium.org> Comment-In-Reply-To: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-MessageType: comment

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit October 2022