coreboot-gerrit December 2019

coreboot-gerrit@coreboot.org

1 participants
1761 discussions

Change in coreboot[master]: mainboard/variant/puff: set PL values for puff
by Kangheui Won (Code Review) 24 Dec '19

24 Dec '19

Kangheui Won has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37651 ) Change subject: mainboard/variant/puff: set PL values for puff ...................................................................... Patch Set 2: (3 comments) https://review.coreboot.org/c/coreboot/+/37651/1/src/mainboard/google/hatch… File src/mainboard/google/hatch/variants/puff/Makefile.inc: https://review.coreboot.org/c/coreboot/+/37651/1/src/mainboard/google/hatch… PS1, Line 16: variant.c > mainboard. […] Ack https://review.coreboot.org/c/coreboot/+/37651/1/src/mainboard/google/hatch… File src/mainboard/google/hatch/variants/puff/variant.c: https://review.coreboot.org/c/coreboot/+/37651/1/src/mainboard/google/hatch… PS1, Line 1: /* > call this file mainboard. […] Ack https://review.coreboot.org/c/coreboot/+/37651/1/src/mainboard/google/hatch… PS1, Line 66: pl4 = 0; > lets not block on this. Ack -- To view, visit https://review.coreboot.org/c/coreboot/+/37651 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I041ebaa33bf2582386198290e625099ba8e2f3c9 Gerrit-Change-Number: 37651 Gerrit-PatchSet: 2 Gerrit-Owner: Kangheui Won <khwon(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Kangheui Won <khwon(a)chromium.org> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Shelley Chen <shchen(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Tue, 24 Dec 2019 10:17:48 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/tigerlake: Update GPIO config
by Furquan Shaikh (Code Review) 24 Dec '19

24 Dec '19

1 0

Change in coreboot[master]: mainboard/variant/puff: set PL values for puff
by Kangheui Won (Code Review) 24 Dec '19

24 Dec '19

Hello Edward O'Callaghan, Shelley Chen, build bot (Jenkins), Patrick Georgi, Martin Roth, Furquan Shaikh, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/37651 to look at the new patch set (#2). Change subject: mainboard/variant/puff: set PL values for puff ...................................................................... mainboard/variant/puff: set PL values for puff To be safe for now, don't differentiate between SKUs and use lower values to ensure board won't be browned out. BUG=b:143246320 TEST=none BRANCH=none Change-Id: I041ebaa33bf2582386198290e625099ba8e2f3c9 Signed-off-by: Kangheui Won <khwon(a)chromium.org> --- M src/mainboard/google/hatch/ramstage.c M src/mainboard/google/hatch/variants/baseboard/include/baseboard/variants.h M src/mainboard/google/hatch/variants/puff/Makefile.inc A src/mainboard/google/hatch/variants/puff/mainboard.c 4 files changed, 100 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/37651/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/37651 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I041ebaa33bf2582386198290e625099ba8e2f3c9 Gerrit-Change-Number: 37651 Gerrit-PatchSet: 2 Gerrit-Owner: Kangheui Won <khwon(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Kangheui Won <khwon(a)chromium.org> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Shelley Chen <shchen(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: soc/intel/tigerlake: Update GPIO config
by Furquan Shaikh (Code Review) 24 Dec '19

24 Dec '19

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37427 ) Change subject: soc/intel/tigerlake: Update GPIO config ...................................................................... Patch Set 12: (30 comments) https://review.coreboot.org/c/coreboot/+/37427/12//COMMIT_MSG Commit Message: https://review.coreboot.org/c/coreboot/+/37427/12//COMMIT_MSG@10 PS12, Line 10: GPIO's GPIOs https://review.coreboot.org/c/coreboot/+/37427/12//COMMIT_MSG@13 PS12, Line 13: I am looking at this as reference: https://github.com/torvalds/linux/tree/master/drivers/pinctrl/intel/pinctrl… Please let me know if this is not the right kernel driver to review your code. https://review.coreboot.org/c/coreboot/+/37427/12//COMMIT_MSG@14 PS12, Line 14: none b:144680462 https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/a… File src/soc/intel/tigerlake/acpi/gpio.asl: https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/a… PS12, Line 78: LAnd (LGreaterEqual (Arg0, GPP_B0), LLessEqual (Arg0, ESPI_CLK_LOOPBK)) It would be better to stick to ASL2.0 syntax as it was before. Just update the offsets that you want to change. https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/a… PS12, Line 101: * space before * https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/a… PS12, Line 101: 05 "5" to keep the naming consistent. https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/a… PS12, Line 140: And (GPIOTXSTATE_MASK, VAL0, Local0) Can you please use ASL2.0 syntax in the entire file? https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/a… PS12, Line 159: extra blank lines https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/g… File src/soc/intel/tigerlake/gpio.c: https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/g… PS12, Line 20: * Chapter number: 27 Was the chapter number dropped intentionally? https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/g… PS12, Line 87: [COMM_0] Why were these dropped? Because of this, now you would be setting the properties of community 4 to [COMM_3] which is not correct. https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/g… PS12, Line 94: gpi_int_sts_reg_0 = GPI_INT_STS_0, : .gpi_int_en_reg_0 = GPI_INT_EN_0, Why were these fields dropped? https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/g… PS12, Line 44: INTEL_GPP_BASE Why do we need INTEL_GPP_BASE for TGL? I think it should be perfectly fine to use INTEL_GPP instead of INTEL_GPP_BASE. https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/g… PS12, Line 45: INTEL_GPP(GPP_B0, GSPI0_CLK_LOOPBK, GSPI1_CLK_LOOPBK) Can't these be clubbed with the above entry? INTEL_GPP(GPP_B0, GPP_B0, GSPI1_CLK_LOOPBK) https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/g… PS12, Line 51: INTEL_GPP_BASE Same question about using INTEL_GPP instead of INTEL_GPP_BASE for all entries here. https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/g… PS12, Line 93: S,D,H,U,VGPIO nit: space after comma https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/g… PS12, Line 169: GPP_B_DW0_1 Why are all the entries here mapping to DW0_1 only? What happens if a mapping for DW2 is required? https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/i… File src/soc/intel/tigerlake/include/soc/gpio_defs.h: https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/i… PS12, Line 268: #define GPI_INT_STS_0 0x100 : #define GPI_INT_EN_0 0x110 Why were these dropped? https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/i… PS12, Line 119: * space before * https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/i… PS12, Line 130: Group D This is not GPP Group D. https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/i… PS12, Line 255: iIRQ extra i https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/i… File src/soc/intel/tigerlake/include/soc/gpio_soc_defs.h: https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/i… PS12, Line 26: * DW0,1 and DW2. Based on this the devicetree should assign them. : * Example of devicetree: : * register "gpe0_dw0" = "GPP_C_DW0_1" # GPP_C : * register "gpe0_dw1" = "GPP_D_DW0_1" # GPP_D : * register "gpe0_dw2" = "GPP_E_DW2" # GPP_E Do we really need to do that? Is it possible for us to have a mapping scheme such that the devicetree user doesn't have to care about GPP_C being different in DW0 v/s DW1 v/s DW2. But, behind the scenes SoC code can take care of mapping GPP_C to its right value depending upon whether it is used in DW0 or DW1 or DW2. That way even the mapping of PMC to GPIO GPE can be simplified. https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/i… PS12, Line 89: GSPI0_CLK_LOOPBK So, there is no GPP# for this pad? e.g. GPP_B24 or something similar? https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/i… PS12, Line 135: ESPI_CLK_LOOPBK No GPP#? https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/i… PS12, Line 225: GPP_VGPIO0 Can we use the same names as used by the kernel driver? https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/i… PS12, Line 256: #define GPD0 171 This community is not exposed or used by the kernel driver. If GPIO #s are assigned consecutively here after COM1, wouldn't it mess up the mapping in kernel v/s coreboot? I think in the past platforms, this was handled by assigning numbers to GPD group towards the end. https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/i… PS12, Line 369: TSSTB TRSTB? https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/i… PS12, Line 374: #define GPP_MLK 277 I don't see this in the kernel driver. https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/i… PS12, Line 401: 5 How is this 5? There is GPIOCOM0 - GPIOCOM5 which is a total of 6? Is this because GPIOCOM3 is missing? https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/i… PS12, Line 407: pin mux setting What exactly does that mean? Is it doing anything more than setting DW0 and DW1? https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/i… File src/soc/intel/tigerlake/include/soc/pmc.h: https://review.coreboot.org/c/coreboot/+/37427/12/src/soc/intel/tigerlake/i… PS12, Line 118: #define PMC_GPP_B 0x0 : #define PMC_GPP_T 0x1 : #define PMC_GPP_A 0x2 : #define PMC_GPP_R 0x3 : #define PMC_GPD 0x4 : #define PMC_GPP_S 0x5 : #define PMC_GPP_H 0x6 : #define PMC_GPP_D 0x7 : #define PMC_GPP_U 0x8 : #define PMC_GPP_F 0xA : #define PMC_GPP_C 0xB : #define PMC_GPP_E 0xC As per EDS version 1, this is not correct. I see PMC GPP values being different based on DWx. -- To view, visit https://review.coreboot.org/c/coreboot/+/37427 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I585100375feee39b5a9105bdf6d9f5ca3a5bb2fa Gerrit-Change-Number: 37427 Gerrit-PatchSet: 12 Gerrit-Owner: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org> Gerrit-Reviewer: Francois Toguo Fotso <francois.toguo.fotso(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Jamie Ryu <jamie.m.ryu(a)intel.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Raj Astekar <raj.astekar(a)intel.com> Gerrit-Reviewer: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Reviewer: Shaunak Saha <shaunak.saha(a)intel.com> Gerrit-Reviewer: Srinidhi N Kaushik <srinidhi.n.kaushik(a)intel.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Venkata Krishna Nimmagadda Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Tue, 24 Dec 2019 10:09:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mainboard/variant/puff: set initial PL values for puff
by Edward O'Callaghan (Code Review) 24 Dec '19

24 Dec '19

Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37651 ) Change subject: mainboard/variant/puff: set initial PL values for puff ...................................................................... Patch Set 1: (2 comments) > Patch Set 1: > > (1 comment) https://review.coreboot.org/c/coreboot/+/37651/1/src/mainboard/google/hatch… File src/mainboard/google/hatch/variants/puff/variant.c: https://review.coreboot.org/c/coreboot/+/37651/1/src/mainboard/google/hatch… PS1, Line 66: pl4 = 0; > Seems like you could invert the if stmt and avoid a else branch then have the sane safe defaults set […] lets not block on this. https://review.coreboot.org/c/coreboot/+/37651/1/src/mainboard/google/hatch… PS1, Line 89: config_t *conf = config_of_soc(); > Is it identical? I thought about it but I couldn't be sure, so used config_of_soc(). It should be the root 0,0. I don't mind this change being a follow up. -- To view, visit https://review.coreboot.org/c/coreboot/+/37651 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I041ebaa33bf2582386198290e625099ba8e2f3c9 Gerrit-Change-Number: 37651 Gerrit-PatchSet: 1 Gerrit-Owner: Kangheui Won <khwon(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Kangheui Won <khwon(a)chromium.org> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Shelley Chen <shchen(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Tue, 24 Dec 2019 10:01:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Kangheui Won <khwon(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mainboard/variant/puff: set initial PL values for puff
by Kangheui Won (Code Review) 24 Dec '19

24 Dec '19

Kangheui Won has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37651 ) Change subject: mainboard/variant/puff: set initial PL values for puff ...................................................................... Patch Set 1: (1 comment) https://review.coreboot.org/c/coreboot/+/37651/1/src/mainboard/google/hatch… File src/mainboard/google/hatch/variants/puff/variant.c: https://review.coreboot.org/c/coreboot/+/37651/1/src/mainboard/google/hatch… PS1, Line 89: config_t *conf = config_of_soc(); > can this be avoided with `config_t * cfg = dev->chip_info;` ? that way we actually use the argument […] Is it identical? I thought about it but I couldn't be sure, so used config_of_soc(). -- To view, visit https://review.coreboot.org/c/coreboot/+/37651 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I041ebaa33bf2582386198290e625099ba8e2f3c9 Gerrit-Change-Number: 37651 Gerrit-PatchSet: 1 Gerrit-Owner: Kangheui Won <khwon(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Kangheui Won <khwon(a)chromium.org> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Shelley Chen <shchen(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Tue, 24 Dec 2019 09:57:09 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mb/emulation/qemu-q35,qemu-i440fx: Add x86_64 support
by Arthur Heymans (Code Review) 24 Dec '19

24 Dec '19

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29667 ) Change subject: mb/emulation/qemu-q35,qemu-i440fx: Add x86_64 support ...................................................................... Patch Set 38: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/29667 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: If2f02a95b2f91ab51043d4e81054354f4a6eb5d5 Gerrit-Change-Number: 29667 Gerrit-PatchSet: 38 Gerrit-Owner: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Reviewer: ron minnich <rminnich(a)gmail.com> Gerrit-CC: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Tue, 24 Dec 2019 09:22:57 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: util/pgtblgen: Fix qemu on KVM
by Arthur Heymans (Code Review) 24 Dec '19

24 Dec '19

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36778 ) Change subject: util/pgtblgen: Fix qemu on KVM ...................................................................... Patch Set 5: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/36778 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I4beae8deec6bf34f9762e7b54c5da4e5b63f6d24 Gerrit-Change-Number: 36778 Gerrit-PatchSet: 5 Gerrit-Owner: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Tue, 24 Dec 2019 09:21:31 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: arch/x86/boot: Jump to payload in protected mode
by Arthur Heymans (Code Review) 24 Dec '19

24 Dec '19

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30118 ) Change subject: arch/x86/boot: Jump to payload in protected mode ...................................................................... Patch Set 29: (4 comments) https://review.coreboot.org/c/coreboot/+/30118/29//COMMIT_MSG Commit Message: https://review.coreboot.org/c/coreboot/+/30118/29//COMMIT_MSG@9 PS29, Line 9: On ARCH_RAMSTAGE_X86_64 jump to the payload in protected mode. I think this should be documented somewhere? https://review.coreboot.org/c/coreboot/+/30118/29/src/arch/x86/c_exit.S File src/arch/x86/c_exit.S: https://review.coreboot.org/c/coreboot/+/30118/29/src/arch/x86/c_exit.S@25 PS29, Line 25: .section .bss, "aw", @nobits What is this doing here? https://review.coreboot.org/c/coreboot/+/30118/29/src/arch/x86/c_exit.S@27 PS29, Line 27: .section ".text.SetCodeSelector32", "ax", @progbits Is just specifying the text section not enough? https://review.coreboot.org/c/coreboot/+/30118/29/src/include/program_loadi… File src/include/program_loading.h: https://review.coreboot.org/c/coreboot/+/30118/29/src/include/program_loadi… PS29, Line 201: uint32_t This cuts off a part of prog_entry_arg which returns a pointer to void. -- To view, visit https://review.coreboot.org/c/coreboot/+/30118 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I6552ac30f1b6205e08e16d251328e01ce3fbfd14 Gerrit-Change-Number: 30118 Gerrit-PatchSet: 29 Gerrit-Owner: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Krystian Hebel <krystian.hebel(a)3mdeb.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Tue, 24 Dec 2019 09:18:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mainboard/variant/puff: set initial PL values for puff
by Edward O'Callaghan (Code Review) 24 Dec '19

24 Dec '19

Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37651 ) Change subject: mainboard/variant/puff: set initial PL values for puff ...................................................................... Patch Set 1: (3 comments) https://review.coreboot.org/c/coreboot/+/37651/1/src/mainboard/google/hatch… File src/mainboard/google/hatch/variants/puff/Makefile.inc: https://review.coreboot.org/c/coreboot/+/37651/1/src/mainboard/google/hatch… PS1, Line 16: variant.c mainboard.c https://review.coreboot.org/c/coreboot/+/37651/1/src/mainboard/google/hatch… File src/mainboard/google/hatch/variants/puff/variant.c: https://review.coreboot.org/c/coreboot/+/37651/1/src/mainboard/google/hatch… PS1, Line 1: /* call this file mainboard.c https://review.coreboot.org/c/coreboot/+/37651/1/src/mainboard/google/hatch… PS1, Line 89: config_t *conf = config_of_soc(); can this be avoided with `config_t * cfg = dev->chip_info;` ? that way we actually use the argument and don't rely on global state. -- To view, visit https://review.coreboot.org/c/coreboot/+/37651 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I041ebaa33bf2582386198290e625099ba8e2f3c9 Gerrit-Change-Number: 37651 Gerrit-PatchSet: 1 Gerrit-Owner: Kangheui Won <khwon(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Kangheui Won <khwon(a)chromium.org> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Shelley Chen <shchen(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Tue, 24 Dec 2019 09:17:57 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit December 2019