coreboot-gerrit June 2020

coreboot-gerrit@coreboot.org

1 participants
2825 discussions

Change in coreboot[master]: [WIP] mb/intel/cannonlake_rvp,coffeelake_rvp: Add LPC_TPM
by Kyösti Mälkki (Code Review) 10 Jun '20

10 Jun '20

Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35103 ) Change subject: [WIP] mb/intel/cannonlake_rvp,coffeelake_rvp: Add LPC_TPM ...................................................................... [WIP] mb/intel/cannonlake_rvp,coffeelake_rvp: Add LPC_TPM Board devicetrees requests for drivers7pc80/tpm but that was not included for the build. Change-Id: Ia959ae9072e3fc709b779b1fc778eb82e10c2ee3 Signed-off-by: Kyösti Mälkki <kyosti.malkki(a)gmail.com> --- M src/mainboard/intel/cannonlake_rvp/Kconfig M src/mainboard/intel/coffeelake_rvp/Kconfig 2 files changed, 2 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/35103/1 diff --git a/src/mainboard/intel/cannonlake_rvp/Kconfig b/src/mainboard/intel/cannonlake_rvp/Kconfig index d86e485..6d686c3 100644 --- a/src/mainboard/intel/cannonlake_rvp/Kconfig +++ b/src/mainboard/intel/cannonlake_rvp/Kconfig @@ -13,6 +13,7 @@ select SOC_INTEL_CANNONLAKE select MAINBOARD_USES_IFD_EC_REGION select INTEL_LPSS_UART_FOR_CONSOLE + select MAINBOARD_HAS_LPC_TPM config MAINBOARD_DIR string diff --git a/src/mainboard/intel/coffeelake_rvp/Kconfig b/src/mainboard/intel/coffeelake_rvp/Kconfig index 13e55b3..8bd2881 100644 --- a/src/mainboard/intel/coffeelake_rvp/Kconfig +++ b/src/mainboard/intel/coffeelake_rvp/Kconfig @@ -16,6 +16,7 @@ select SOC_INTEL_COMMON_BLOCK_HDA if BOARD_INTEL_WHISKEYLAKE_RVP || BOARD_INTEL_COMETLAKE_RVP select MAINBOARD_USES_IFD_EC_REGION select MAINBOARD_USES_IFD_GBE_REGION if BOARD_INTEL_COFFEELAKE_RVP11 || BOARD_INTEL_COFFEELAKE_RVP8 + select MAINBOARD_HAS_LPC_TPM config MAINBOARD_DIR string -- To view, visit https://review.coreboot.org/c/coreboot/+/35103 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ia959ae9072e3fc709b779b1fc778eb82e10c2ee3 Gerrit-Change-Number: 35103 Gerrit-PatchSet: 1 Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-MessageType: newchange

8 21

Change in coreboot[master]: drivers/intel/mipi_camera: Generate SSDT for camera
by Paul Menzel (Code Review) 10 Jun '20

10 Jun '20

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41607 ) Change subject: drivers/intel/mipi_camera: Generate SSDT for camera ...................................................................... Patch Set 17: Code-Review+1 (6 comments) https://review.coreboot.org/c/coreboot/+/41607/17//COMMIT_MSG Commit Message: https://review.coreboot.org/c/coreboot/+/41607/17//COMMIT_MSG@21 PS17, Line 21: in to into https://review.coreboot.org/c/coreboot/+/41607/17/src/drivers/intel/mipi_ca… File src/drivers/intel/mipi_camera/chip.h: https://review.coreboot.org/c/coreboot/+/41607/17/src/drivers/intel/mipi_ca… PS17, Line 14: {.type = IMGCLK, .index = (ind), .action = (act), .delay_ms = (delay) } Please add a space after { in the beginning. https://review.coreboot.org/c/coreboot/+/41607/17/src/drivers/intel/mipi_ca… PS17, Line 17: {.type = GPIO, .index = (ind), .action = (act), .delay_ms = (delay) } Ditto. https://review.coreboot.org/c/coreboot/+/41607/17/src/drivers/intel/mipi_ca… PS17, Line 125: * this is in unit of usec */ The coding style prefer no asterisk for concise multi-line comments. https://doc.coreboot.org/coding_style.html#commenting https://review.coreboot.org/c/coreboot/+/41607/17/src/drivers/intel/mipi_ca… PS17, Line 174: uint32_t nvm_readonly; That sounds like, it should be a boolean? https://review.coreboot.org/c/coreboot/+/41607/12/src/drivers/intel/mipi_ca… File src/drivers/intel/mipi_camera/chip.h: https://review.coreboot.org/c/coreboot/+/41607/12/src/drivers/intel/mipi_ca… PS12, Line 167: uint32_t nvm_size; > Any specific reason to change them to native. […] Both works, but If there is not a explicit length requirement, the reader would think, that there is. The compiler and architecture use minimum length anyway [1]. (I know that coreboot code is small, so it doesn’t make a difference.) [1]: http://notabs.org/coding/smallIntsBigPenalty.htm -- To view, visit https://review.coreboot.org/c/coreboot/+/41607 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I15979f345fb823df2560db269e902a1ea650b69e Gerrit-Change-Number: 41607 Gerrit-PatchSet: 17 Gerrit-Owner: Sugnan Prabhu S <sugnan.prabhu.s(a)intel.com> Gerrit-Reviewer: Daniel Kang <daniel.h.kang(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Matt Delco <delco(a)chromium.org> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)google.com> Gerrit-Reviewer: Varshit B Pandya <varshit.b.pandya(a)intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-CC: Kiran2 Kumar <kiran2.kumar(a)intel.corp-partner.google.com> Gerrit-Comment-Date: Wed, 10 Jun 2020 11:46:10 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Sugnan Prabhu S <sugnan.prabhu.s(a)intel.com> Comment-In-Reply-To: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: acpi: Add support for calling method with parameter
by Paul Menzel (Code Review) 10 Jun '20

10 Jun '20

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41620 ) Change subject: acpi: Add support for calling method with parameter ...................................................................... Patch Set 13: (2 comments) https://review.coreboot.org/c/coreboot/+/41620/11/src/acpi/acpigen.c File src/acpi/acpigen.c: https://review.coreboot.org/c/coreboot/+/41620/11/src/acpi/acpigen.c@1970 PS11, Line 1970: int > only if params_len is unsigned, otherwise you've got signed/unsigned comparisons True. Good catch. https://review.coreboot.org/c/coreboot/+/41620/13/src/acpi/acpigen.c File src/acpi/acpigen.c: https://review.coreboot.org/c/coreboot/+/41620/13/src/acpi/acpigen.c@1957 PS13, Line 1957: unsigned int Thank you for the update. Maybe even `size_t` then? I’ll resolve my comment right away, as you can decide. -- To view, visit https://review.coreboot.org/c/coreboot/+/41620 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ibcb67bd756dc8ae1dfd2d40020c273325583df0f Gerrit-Change-Number: 41620 Gerrit-PatchSet: 13 Gerrit-Owner: Sugnan Prabhu S <sugnan.prabhu.s(a)intel.com> Gerrit-Reviewer: Daniel Kang <daniel.h.kang(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)google.com> Gerrit-Reviewer: Varshit B Pandya <varshit.b.pandya(a)intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Kiran2 Kumar <kiran2.kumar(a)intel.corp-partner.google.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Wed, 10 Jun 2020 11:39:14 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Comment-In-Reply-To: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: drivers/intel/mipi_camera: Support adding camera power resource
by Sugnan Prabhu S (Code Review) 10 Jun '20

10 Jun '20

Sugnan Prabhu S has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41624 ) Change subject: drivers/intel/mipi_camera: Support adding camera power resource ...................................................................... Patch Set 14: (17 comments) https://review.coreboot.org/c/coreboot/+/41624/12//COMMIT_MSG Commit Message: https://review.coreboot.org/c/coreboot/+/41624/12//COMMIT_MSG@7 PS12, Line 7: drivers/intel/mipi_camera: Support for adding camera power resource > Please make it a statement by adding a verb (in imperative mood): […] Done https://review.coreboot.org/c/coreboot/+/41624/12/src/drivers/intel/mipi_ca… File src/drivers/intel/mipi_camera/camera.c: https://review.coreboot.org/c/coreboot/+/41624/12/src/drivers/intel/mipi_ca… PS12, Line 428: int > unsigned int Done https://review.coreboot.org/c/coreboot/+/41624/12/src/drivers/intel/mipi_ca… PS12, Line 437: method_params[0] = config->clk_panel.clks[index].clknum; : method_params[1] = config->clk_panel.clks[index].freq; : acpigen_call_method("MCON", method_params, 2); > This is why I don't particularly like acpigen_call_method(), because I don't find this any easier to […] I will consider moving acpigen_call_method to separate patch and use acpigen helper directly here. https://review.coreboot.org/c/coreboot/+/41624/12/src/drivers/intel/mipi_ca… PS12, Line 445: printk(BIOS_ERR, "Unsupported clock action: %x\n", > Please add the consequence for the user. […] Done https://review.coreboot.org/c/coreboot/+/41624/12/src/drivers/intel/mipi_ca… PS12, Line 458: printk(BIOS_ERR, "Unsupported GPIO action: %x\n", > Please add the consequence for the user. […] Done https://review.coreboot.org/c/coreboot/+/41624/12/src/drivers/intel/mipi_ca… PS12, Line 464: printk(BIOS_ERR, "Unsupported power operation: %x\n", seq->ops[i].type); > Please add the consequence for the user. […] Done https://review.coreboot.org/c/coreboot/+/41624/12/src/drivers/intel/mipi_ca… PS12, Line 512: (config->pr0 > This should check for again NULL and supply a default. […] we are already checking for 0 in if condition line 511. If config->pr0 is not set in devicetree it will have default value 0 it will never be NULL. JSL is always using different custom name for CAM0 and CAM1 https://review.coreboot.org/c/coreboot/+/41624/12/src/drivers/intel/mipi_ca… PS12, Line 537: acpigen_write_store_op_to_namestr(0, "STA"); This implementation is based on some of the older platforms PowerResource. Checking internally if this is required. https://github.com/coreboot/coreboot/blob/master/src/mainboard/google/poppy… https://github.com/coreboot/coreboot/blob/master/src/mainboard/google/poppy… https://review.coreboot.org/c/coreboot/+/41624/12/src/drivers/intel/mipi_ca… PS12, Line 599: acpigen_write_name("_DEP"); : acpigen_write_package(1); : acpigen_emit_namestring(config->dep); : acpigen_pop_len(); > I still don't understand this. The ACPI spec says this about _DEP: […] I think this can be removed. I will test once and remove it from SSDT generation patch. https://review.coreboot.org/c/coreboot/+/41624/12/src/drivers/intel/mipi_ca… File src/drivers/intel/mipi_camera/chip.h: https://review.coreboot.org/c/coreboot/+/41624/12/src/drivers/intel/mipi_ca… PS12, Line 69: clk_ct > there could be a better name here, I'm not sure what the 'ct' refers to. […] Done https://review.coreboot.org/c/coreboot/+/41624/12/src/drivers/intel/mipi_ca… PS12, Line 81: 2 > symbolic constant please Done https://review.coreboot.org/c/coreboot/+/41624/12/src/drivers/intel/mipi_ca… PS12, Line 84: gp_ctrl_panel > gpio_... […] Done https://review.coreboot.org/c/coreboot/+/41624/12/src/drivers/intel/mipi_ca… PS12, Line 85: 4 > symbolic constant please Done https://review.coreboot.org/c/coreboot/+/41624/12/src/drivers/intel/mipi_ca… PS12, Line 85: struct gp_ct gps[4]; > is the struct for gp_ct necessary? could just have uint8_t gpios[4]. struct gp_ct can be extended to include other properties related to GPIO in the future. https://review.coreboot.org/c/coreboot/+/41624/12/src/drivers/intel/mipi_ca… PS12, Line 90: index > what is the index for? Doesn't their relative order in operation_seq define the order they're applie […] This is index to items in clock_ctrl_panel or gp_ctrl_panel https://review.coreboot.org/c/coreboot/+/41624/12/src/drivers/intel/mipi_ca… PS12, Line 91: enum action_type action; > Action type is not a good name? Why can’t a bool be used, and named `enable`? There are plan to extend the operations types, so we might add more actions other than ENABLE/DISABLE. eg, we might add READ and WRITE incase if we start handling register operations. https://review.coreboot.org/c/coreboot/+/41624/12/src/drivers/intel/mipi_ca… PS12, Line 97: delay_ms > should specify (in a comment or in the name) if it is pre- or post-op delay Current we do not need and pre or post delay as there is a delay already part of the operation_type. Removing it form this structure. -- To view, visit https://review.coreboot.org/c/coreboot/+/41624 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I31e198b50acf2c64035aff9cb054fbe3602dd83e Gerrit-Change-Number: 41624 Gerrit-PatchSet: 14 Gerrit-Owner: Sugnan Prabhu S <sugnan.prabhu.s(a)intel.com> Gerrit-Reviewer: Daniel Kang <daniel.h.kang(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)google.com> Gerrit-Reviewer: Varshit B Pandya <varshit.b.pandya(a)intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Kiran2 Kumar <kiran2.kumar(a)intel.corp-partner.google.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Wed, 10 Jun 2020 10:54:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Paul Menzel <paulepanter(a)users.sourceforge.net> Comment-In-Reply-To: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mb/intel/jasperlake_rvp: camera SSDT changes for JSLRVP
by Sugnan Prabhu S (Code Review) 10 Jun '20

10 Jun '20

Sugnan Prabhu S has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41758 ) Change subject: mb/intel/jasperlake_rvp: camera SSDT changes for JSLRVP ...................................................................... Patch Set 6: (2 comments) https://review.coreboot.org/c/coreboot/+/41758/5/src/mainboard/intel/jasper… File src/mainboard/intel/jasperlake_rvp/variants/jslrvp/devicetree.cb: https://review.coreboot.org/c/coreboot/+/41758/5/src/mainboard/intel/jasper… PS5, Line 320: register "on_seq.ops[0].type" = "IMGCLK" : register "on_seq.ops[0].index" = "0" #clk enable : register "on_seq.ops[0].action" = "ENABLE" > suggestion: making a macro to build these structs could make this easier to read. […] Done https://review.coreboot.org/c/coreboot/+/41758/5/src/mainboard/intel/jasper… PS5, Line 446: register "cio2_lanes_used[0]" = "2" : register "cio2_lanes_used[1]" = "2" > could be register "cio2_lanes_used" = "{2, 2}" Done -- To view, visit https://review.coreboot.org/c/coreboot/+/41758 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib439572bc1d15ef02c86c7bfa88af6b16eb06f97 Gerrit-Change-Number: 41758 Gerrit-PatchSet: 6 Gerrit-Owner: Sugnan Prabhu S <sugnan.prabhu.s(a)intel.com> Gerrit-Reviewer: Daniel Kang <daniel.h.kang(a)intel.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Varshit B Pandya <varshit.b.pandya(a)intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-CC: Kiran2 Kumar <kiran2.kumar(a)intel.corp-partner.google.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-CC: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Comment-Date: Wed, 10 Jun 2020 10:53:50 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mb/intel/jasperlake_rvp: camera SSDT changes for JSLRVP
by Sugnan Prabhu S (Code Review) 10 Jun '20

10 Jun '20

Hello Varshit B Pandya, Daniel Kang, Wonkyu Kim, Rizwan Qureshi, Patrick Rudolph, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/41758 to look at the new patch set (#6). Change subject: mb/intel/jasperlake_rvp: camera SSDT changes for JSLRVP ...................................................................... mb/intel/jasperlake_rvp: camera SSDT changes for JSLRVP Change-Id: Ib439572bc1d15ef02c86c7bfa88af6b16eb06f97 Signed-off-by: Sugnan Prabhu S <sugnan.prabhu.s(a)intel.com> --- M src/drivers/intel/mipi_camera/Kconfig M src/mainboard/intel/jasperlake_rvp/dsdt.asl M src/mainboard/intel/jasperlake_rvp/variants/jslrvp/devicetree.cb 3 files changed, 118 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/41758/6 -- To view, visit https://review.coreboot.org/c/coreboot/+/41758 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib439572bc1d15ef02c86c7bfa88af6b16eb06f97 Gerrit-Change-Number: 41758 Gerrit-PatchSet: 6 Gerrit-Owner: Sugnan Prabhu S <sugnan.prabhu.s(a)intel.com> Gerrit-Reviewer: Daniel Kang <daniel.h.kang(a)intel.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Varshit B Pandya <varshit.b.pandya(a)intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-CC: Kiran2 Kumar <kiran2.kumar(a)intel.corp-partner.google.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-CC: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: drivers/intel/mipi_camera: Generate SSDT for camera
by Sugnan Prabhu S (Code Review) 10 Jun '20

10 Jun '20

Hello Varshit B Pandya, build bot (Jenkins), Matt Delco, Daniel Kang, Furquan Shaikh, Wonkyu Kim, Tim Wawrzynczak, Rizwan Qureshi, Tim Wawrzynczak, Patrick Rudolph, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/41607 to look at the new patch set (#17). Change subject: drivers/intel/mipi_camera: Generate SSDT for camera ...................................................................... drivers/intel/mipi_camera: Generate SSDT for camera Some boards & variants are using raw ASL files to define the ACPI devices and related properties required by Linux MIPI camera drivers. The mipi_camera driver can provide a SSDB property, but it's missing some properties (like _DEP, _DSD, PRT0 and EP00 etc.) needed for Linux drivers. This change updates the mipi_camera driver so it can provide what's required by Linux and expands the support for the IMGU and CIO2 devices. BUG=None BRANCH=None TEST=Verified that the ACPI state generated by the driver (with appropriate devicetree changes) is extremely comparible to what's generated by the ASL files (with minor differences like some scope were split in to multiple scopes unlike handwritten ASL). Change-Id: I15979f345fb823df2560db269e902a1ea650b69e Signed-off-by: Matt Delco <delco(a)chromium.org> Signed-off-by: Sugnan Prabhu S <sugnan.prabhu.s(a)intel.com> --- M src/drivers/intel/mipi_camera/camera.c M src/drivers/intel/mipi_camera/chip.h 2 files changed, 668 insertions(+), 28 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/41607/17 -- To view, visit https://review.coreboot.org/c/coreboot/+/41607 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I15979f345fb823df2560db269e902a1ea650b69e Gerrit-Change-Number: 41607 Gerrit-PatchSet: 17 Gerrit-Owner: Sugnan Prabhu S <sugnan.prabhu.s(a)intel.com> Gerrit-Reviewer: Daniel Kang <daniel.h.kang(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Matt Delco <delco(a)chromium.org> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)google.com> Gerrit-Reviewer: Varshit B Pandya <varshit.b.pandya(a)intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-CC: Kiran2 Kumar <kiran2.kumar(a)intel.corp-partner.google.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: drivers/intel/mipi_camera: Support adding camera power resource
by Sugnan Prabhu S (Code Review) 10 Jun '20

10 Jun '20

Hello Varshit B Pandya, build bot (Jenkins), Daniel Kang, Furquan Shaikh, Wonkyu Kim, Tim Wawrzynczak, Rizwan Qureshi, Tim Wawrzynczak, Patrick Rudolph, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/41624 to look at the new patch set (#13). Change subject: drivers/intel/mipi_camera: Support adding camera power resource ...................................................................... drivers/intel/mipi_camera: Support adding camera power resource This change adds support function to parse entries in the devicetree to generate PowerResource entries for the MIPI camera. Change-Id: I31e198b50acf2c64035aff9cb054fbe3602dd83e Signed-off-by: Sugnan Prabhu S <sugnan.prabhu.s(a)intel.com> --- M src/drivers/intel/mipi_camera/camera.c M src/drivers/intel/mipi_camera/chip.h 2 files changed, 157 insertions(+), 3 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/24/41624/13 -- To view, visit https://review.coreboot.org/c/coreboot/+/41624 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I31e198b50acf2c64035aff9cb054fbe3602dd83e Gerrit-Change-Number: 41624 Gerrit-PatchSet: 13 Gerrit-Owner: Sugnan Prabhu S <sugnan.prabhu.s(a)intel.com> Gerrit-Reviewer: Daniel Kang <daniel.h.kang(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)google.com> Gerrit-Reviewer: Varshit B Pandya <varshit.b.pandya(a)intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Kiran2 Kumar <kiran2.kumar(a)intel.corp-partner.google.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: acpi: Add support for calling method with parameter
by Sugnan Prabhu S (Code Review) 10 Jun '20

10 Jun '20

Hello Varshit B Pandya, build bot (Jenkins), Daniel Kang, Furquan Shaikh, Wonkyu Kim, Tim Wawrzynczak, Rizwan Qureshi, Tim Wawrzynczak, Patrick Rudolph, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/41620 to look at the new patch set (#12). Change subject: acpi: Add support for calling method with parameter ...................................................................... acpi: Add support for calling method with parameter This change adds support function for calling method with parameters. Change-Id: Ibcb67bd756dc8ae1dfd2d40020c273325583df0f Signed-off-by: Sugnan Prabhu S <sugnan.prabhu.s(a)intel.com> --- M src/acpi/acpigen.c M src/include/acpi/acpigen.h 2 files changed, 11 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/41620/12 -- To view, visit https://review.coreboot.org/c/coreboot/+/41620 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ibcb67bd756dc8ae1dfd2d40020c273325583df0f Gerrit-Change-Number: 41620 Gerrit-PatchSet: 12 Gerrit-Owner: Sugnan Prabhu S <sugnan.prabhu.s(a)intel.com> Gerrit-Reviewer: Daniel Kang <daniel.h.kang(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)google.com> Gerrit-Reviewer: Varshit B Pandya <varshit.b.pandya(a)intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Kiran2 Kumar <kiran2.kumar(a)intel.corp-partner.google.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: [NOT FOR MERGE] crossgcc: back port GNAT EH handler_v1 patch
by Iru Cai (vimacs) (Code Review) 10 Jun '20

10 Jun '20

Hello Iru Cai, I'd like you to do a code review. Please visit https://review.coreboot.org/c/coreboot/+/42158 to review the following change. Change subject: [NOT FOR MERGE] crossgcc: back port GNAT EH handler_v1 patch ...................................................................... [NOT FOR MERGE] crossgcc: back port GNAT EH handler_v1 patch GCC 10 uses a new exception handler ABI, so older GNAT cannot be built with GCC 10. Backport GCC commit 5d733372faa9 to make GNAT able to be built. Change-Id: I757158056bf4698d3c68715e026c226615bc70a1 Signed-off-by: Iru Cai <mytbk920423(a)gmail.com> --- A util/crossgcc/patches/gcc-8.3.0_gnat_eh.patch 1 file changed, 270 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/42158/1 diff --git a/util/crossgcc/patches/gcc-8.3.0_gnat_eh.patch b/util/crossgcc/patches/gcc-8.3.0_gnat_eh.patch new file mode 100644 index 0000000..151a3e6 --- /dev/null +++ b/util/crossgcc/patches/gcc-8.3.0_gnat_eh.patch @@ -0,0 +1,270 @@ +commit 5d733372faa97c1c3943a20a252d000db37c738b +Author: Alexandre Oliva <oliva(a)adacore.com> +Date: Fri Aug 2 18:46:51 2019 +0000 + + rework Ada EH Machine_Occurrence deallocation + + Introduce exception handler ABI #1 to ensure single release, no access + after release of reraised Machine_Occurrences, and no failure to + re-reraise a Machine_Occurrence. + + Unlike Ada exceptions, foreign exceptions do not get a new + Machine_Occurrence upon reraise, but each handler would delete the + exception upon completion, normal or exceptional, save for the case of + a 'raise;' statement within the handler, that avoided the delete by + clearing the exception pointer that the cleanup would use to release + it. The cleared exception pointer might then be used by a subsequent + reraise within the same handler. Get_Current_Excep.all would also + expose the Machine_Occurrence to reuse by Reraise_Occurrence, even for + native exceptions. + + Under ABI #1, Begin_Handler_v1 claims responsibility for releasing an + exception by saving its cleanup and setting it to Claimed_Cleanup. + End_Handler_v1 restores the cleanup and runs it, as long as it isn't + still Claimed_Cleanup (which indicates an enclosing handler has + already claimed responsibility for releasing it), and as long as the + same exception is not being propagated up (the next handler of the + propagating exception will then claim responsibility for releasing + it), so reraise no longer needs to clear the exception pointer, and it + can just propagate the exception, just like Reraise_Occurrence. + + ABI #1 is fully interoperable with ABI #0, i.e., exception handlers + that call the #0 primitives can be linked together with ones that call + the #1 primitives, and they will not misbehave. When a #1 handler + claims responsibility for releasing an exception, even #0 reraises + dynamically nested within it will refrain from releasing it. However, + when a #0 handler is a handler of a foreign exception that would have + been responsible for releasing it with #1, a Reraise_Occurrence of + that foreign or other Machine_Occurrence-carrying exception may still + cause the exception to be released multiple times, and to be used + after it is first released, even if other handlers of the foreign + exception use #1. + + + for gcc/ada/ChangeLog + + * libgnat/a-exexpr.adb (Begin_Handler_v1, End_Handler_v1): New. + (Claimed_Cleanup): New. + (Begin_Handler, End_Handler): Document. + * gcc-interface/trans.c (gigi): Switch to exception handler + ABI #1. + (Exception_Handler_to_gnu_gcc): Save the original cleanup + returned by begin handler, pass it to end handler, and use + EH_ELSE_EXPR to pass a propagating exception to end handler. + (gnat_to_gnu): Leave the exception pointer alone for reraise. + (add_cleanup): Handle EH_ELSE_EXPR, require it by itself. + + From-SVN: r274029 + +diff --git a/gcc/ada/libgnat/a-exexpr.adb b/gcc/ada/libgnat/a-exexpr.adb +index b1aa1c6e6ba..5e72fd6e3f2 100644 +--- a/gcc/ada/libgnat/a-exexpr.adb ++++ b/gcc/ada/libgnat/a-exexpr.adb +@@ -197,15 +197,75 @@ package body Exception_Propagation is + -- whose machine occurrence is Mo. The message is empty, the backtrace + -- is empty too and the exception identity is Foreign_Exception. + +- -- Hooks called when entering/leaving an exception handler for a given +- -- occurrence, aimed at handling the stack of active occurrences. The +- -- calls are generated by gigi in tree_transform/N_Exception_Handler. ++ -- Hooks called when entering/leaving an exception handler for a ++ -- given occurrence. The calls are generated by gigi in ++ -- Exception_Handler_to_gnu_gcc. ++ ++ -- Begin_Handler_v1, called when entering an exception handler, ++ -- claims responsibility for the handler to release the ++ -- GCC_Exception occurrence. End_Handler_v1, called when ++ -- leaving the handler, releases the occurrence, unless the ++ -- occurrence is propagating further up, or the handler is ++ -- dynamically nested in the context of another handler that ++ -- claimed responsibility for releasing that occurrence. ++ ++ -- Responsibility is claimed by changing the Cleanup field to ++ -- Claimed_Cleanup, which enables claimed exceptions to be ++ -- recognized, and avoids accidental releases even by foreign ++ -- handlers. ++ ++ function Begin_Handler_v1 ++ (GCC_Exception : not null GCC_Exception_Access) ++ return System.Address; ++ pragma Export (C, Begin_Handler_v1, "__gnat_begin_handler_v1"); ++ -- Called when entering an exception handler. Claim ++ -- responsibility for releasing GCC_Exception, by setting the ++ -- cleanup/release function to Claimed_Cleanup, and return the ++ -- address of the previous cleanup/release function. ++ ++ procedure End_Handler_v1 ++ (GCC_Exception : not null GCC_Exception_Access; ++ Saved_Cleanup : System.Address; ++ Propagating_Exception : GCC_Exception_Access); ++ pragma Export (C, End_Handler_v1, "__gnat_end_handler_v1"); ++ -- Called when leaving an exception handler. Restore the ++ -- Saved_Cleanup in the GCC_Exception occurrence, and then release ++ -- it, unless it remains claimed by an enclosing handler, or ++ -- GCC_Exception and Propagating_Exception are the same ++ -- occurrence. Propagating_Exception could be either an ++ -- occurrence (re)raised within the handler of GCC_Exception, when ++ -- we're executing as an exceptional cleanup, or null, if we're ++ -- completing the handler of GCC_Exception normally. ++ ++ procedure Claimed_Cleanup ++ (Reason : Unwind_Reason_Code; ++ GCC_Exception : not null GCC_Exception_Access); ++ pragma Export (C, Claimed_Cleanup, "__gnat_claimed_cleanup"); ++ -- A do-nothing placeholder installed as GCC_Exception.Cleanup ++ -- while handling GCC_Exception, to claim responsibility for ++ -- releasing it, and to stop it from being accidentally released. ++ ++ -- The following are version 0 implementations of the version 1 ++ -- hooks above. They remain in place for compatibility with the ++ -- output of compilers that still use version 0, such as those ++ -- used during bootstrap. They are interoperable with the v1 ++ -- hooks, except that the older versions may malfunction when ++ -- handling foreign exceptions passed to Reraise_Occurrence. + + procedure Begin_Handler (GCC_Exception : not null GCC_Exception_Access); + pragma Export (C, Begin_Handler, "__gnat_begin_handler"); ++ -- Called when entering an exception handler translated by an old ++ -- compiler. It does nothing. + + procedure End_Handler (GCC_Exception : GCC_Exception_Access); + pragma Export (C, End_Handler, "__gnat_end_handler"); ++ -- Called when leaving an exception handler translated by an old ++ -- compiler. It releases GCC_Exception, unless it is null. It is ++ -- only ever null when the handler has a 'raise;' translated by a ++ -- v0-using compiler. The artificial handler variable passed to ++ -- End_Handler was set to null to tell End_Handler to refrain from ++ -- releasing the reraised exception. In v1 safer ways are used to ++ -- accomplish that. + + -------------------------------------------------------------------- + -- Accessors to Basic Components of a GNAT Exception Data Pointer -- +@@ -352,6 +412,128 @@ package body Exception_Propagation is + end if; + end Setup_Current_Excep; + ++ ---------------------- ++ -- Begin_Handler_v1 -- ++ ---------------------- ++ ++ function Begin_Handler_v1 ++ (GCC_Exception : not null GCC_Exception_Access) ++ return System.Address is ++ Saved_Cleanup : constant System.Address := GCC_Exception.Cleanup; ++ begin ++ -- Claim responsibility for releasing this exception, and stop ++ -- others from releasing it. ++ GCC_Exception.Cleanup := Claimed_Cleanup'Address; ++ return Saved_Cleanup; ++ end Begin_Handler_v1; ++ ++ -------------------- ++ -- End_Handler_v1 -- ++ -------------------- ++ ++ procedure End_Handler_v1 ++ (GCC_Exception : not null GCC_Exception_Access; ++ Saved_Cleanup : System.Address; ++ Propagating_Exception : GCC_Exception_Access) is ++ begin ++ GCC_Exception.Cleanup := Saved_Cleanup; ++ -- Restore the Saved_Cleanup, so that it is either used to ++ -- release GCC_Exception below, or transferred to the next ++ -- handler of the Propagating_Exception occurrence. The ++ -- following test ensures that an occurrence is only released ++ -- once, even after reraises. ++ -- ++ -- The idea is that the GCC_Exception is not to be released ++ -- unless it had an unclaimed Cleanup when the handler started ++ -- (see Begin_Handler_v1 above), but if we propagate across its ++ -- handler a reraise of the same exception, we transfer to the ++ -- Propagating_Exception the responsibility for running the ++ -- Saved_Cleanup when its handler completes. ++ -- ++ -- This ownership transfer mechanism ensures safety, as in ++ -- single release and no dangling pointers, because there is no ++ -- way to hold on to the Machine_Occurrence of an ++ -- Exception_Occurrence: the only situations in which another ++ -- Exception_Occurrence gets the same Machine_Occurrence are ++ -- through Reraise_Occurrence, and plain reraise, and so we ++ -- have the following possibilities: ++ -- ++ -- - Reraise_Occurrence is handled within the running handler, ++ -- and so when completing the dynamically nested handler, we ++ -- must NOT release the exception. A Claimed_Cleanup upon ++ -- entry of the nested handler, installed when entering the ++ -- enclosing handler, ensures the exception will not be ++ -- released by the nested handler, but rather by the enclosing ++ -- handler. ++ -- ++ -- - Reraise_Occurrence/reraise escapes the running handler, ++ -- and we run as an exceptional cleanup for GCC_Exception. The ++ -- Saved_Cleanup was reinstalled, but since we're propagating ++ -- the same machine occurrence, we do not release it. Instead, ++ -- we transfer responsibility for releasing it to the eventual ++ -- handler of the propagating exception. ++ -- ++ -- - An unrelated exception propagates through the running ++ -- handler. We restored GCC_Exception.Saved_Cleanup above. ++ -- Since we're propagating a different exception, we proceed to ++ -- release GCC_Exception, unless Saved_Cleanup was ++ -- Claimed_Cleanup, because then we know we're not in the ++ -- outermost handler for GCC_Exception. ++ -- ++ -- - The handler completes normally, so it reinstalls the ++ -- Saved_Cleanup and runs it, unless it was Claimed_Cleanup. ++ -- If Saved_Cleanup is null, Unwind_DeleteException (currently) ++ -- has no effect, so we could skip it, but if it is ever ++ -- changed to do more in this case, we're ready for that, ++ -- calling it exactly once. ++ if Saved_Cleanup /= Claimed_Cleanup'Address ++ and then ++ Propagating_Exception /= GCC_Exception ++ then ++ declare ++ Current : constant EOA := Get_Current_Excep.all; ++ Cur_Occ : constant GCC_Exception_Access ++ := To_GCC_Exception (Current.Machine_Occurrence); ++ begin ++ -- If we are releasing the Machine_Occurrence of the current ++ -- exception, reset the access to it, so that it is no ++ -- longer accessible. ++ if Cur_Occ = GCC_Exception then ++ Current.Machine_Occurrence := System.Null_Address; ++ end if; ++ end; ++ Unwind_DeleteException (GCC_Exception); ++ end if; ++ end End_Handler_v1; ++ ++ --------------------- ++ -- Claimed_Cleanup -- ++ --------------------- ++ ++ procedure Claimed_Cleanup ++ (Reason : Unwind_Reason_Code; ++ GCC_Exception : not null GCC_Exception_Access) is ++ pragma Unreferenced (Reason); ++ pragma Unreferenced (GCC_Exception); ++ begin ++ -- This procedure should never run. If it does, it's either a ++ -- version 0 handler or a foreign handler, attempting to ++ -- release an exception while a version 1 handler that claimed ++ -- responsibility for releasing the exception remains still ++ -- active. This placeholder stops GCC_Exception from being ++ -- released by them. ++ ++ -- We could get away with just Null_Address instead, with ++ -- nearly the same effect, but with this placeholder we can ++ -- detect and report unexpected releases, and we can tell apart ++ -- a GCC_Exception without a Cleanup, from one with another ++ -- active handler, so as to still call Unwind_DeleteException ++ -- exactly once: currently, Unwind_DeleteException does nothing ++ -- when the Cleanup is null, but should it ever be changed to ++ -- do more, we'll still be safe. ++ null; ++ end Claimed_Cleanup; ++ + ------------------- + -- Begin_Handler -- + ------------------- -- To view, visit https://review.coreboot.org/c/coreboot/+/42158 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I757158056bf4698d3c68715e026c226615bc70a1 Gerrit-Change-Number: 42158 Gerrit-PatchSet: 1 Gerrit-Owner: Iru Cai (vimacs) <mytbk920423(a)gmail.com> Gerrit-Reviewer: Iru Cai <mytbk920423(a)gmail.com> Gerrit-MessageType: newchange

4 9

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit June 2020