coreboot-gerrit November 2021

coreboot-gerrit@coreboot.org

1 participants
6394 discussions

Change in coreboot[master]: mb/google/guybrush/var/nipperkin: Update SPKR GPIO configuration
by Rob Barnes (Code Review) 10 Nov '21

10 Nov '21

Attention is currently required from: Kevin Chiu, Bhanu Prakash Maiya, Eric Peers, Karthik Ramasubramanian. Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59051 ) Change subject: mb/google/guybrush/var/nipperkin: Update SPKR GPIO configuration ...................................................................... Patch Set 3: (3 comments) File src/mainboard/google/guybrush/variants/nipperkin/gpio.c: https://review.coreboot.org/c/coreboot/+/59051/comment/bf19d5b6_ed865fcc PS3, Line 39: /* EN_SPKR */ Should disconnect baseboard EN_SPKR / GPIO_31 https://review.coreboot.org/c/coreboot/+/59051/comment/4c683ad9_b6846e29 PS3, Line 40: PAD_GPO(GPIO_70, HIGH), Should sdmode_gpio in nipperkin/overridetree.cb be GPIO_70? https://review.coreboot.org/c/coreboot/+/59051/comment/eb74a8b6_60171b0f PS3, Line 61: PAD_NC(GPIO_18), GPIO_70 is disconnected in baseboard pcie_gpio_table. Should connect here. -- To view, visit https://review.coreboot.org/c/coreboot/+/59051 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3d82292b116f53d85d9518364ffd2169bd915a7e Gerrit-Change-Number: 59051 Gerrit-PatchSet: 3 Gerrit-Owner: Kevin Chiu <kevin.chiu.17802(a)gmail.com> Gerrit-Reviewer: Bhanu Prakash Maiya <bhanumaiya(a)google.com> Gerrit-Reviewer: Eric Peers <epeers(a)google.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Rob Barnes <robbarnes(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Kevin Chiu <kevin.chiu.17802(a)gmail.com> Gerrit-Attention: Bhanu Prakash Maiya <bhanumaiya(a)google.com> Gerrit-Attention: Eric Peers <epeers(a)google.com> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Comment-Date: Wed, 10 Nov 2021 16:06:22 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: ec/starlabs: Add standardised ITE EC support
by Sean Rhodes (Code Review) 10 Nov '21

10 Nov '21

Attention is currently required from: Paul Menzel, Angel Pons. Sean Rhodes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58343 ) Change subject: ec/starlabs: Add standardised ITE EC support ...................................................................... Patch Set 19: (25 comments) File src/ec/starlabs/merlin/acpi/battery.asl: https://review.coreboot.org/c/coreboot/+/58343/comment/a82dd54e_4714491d PS19, Line 41: 0x0a > 10 Done https://review.coreboot.org/c/coreboot/+/58343/comment/6073ae6e_d2fbfb65 PS19, Line 42: 0x64 > 100 Done https://review.coreboot.org/c/coreboot/+/58343/comment/68d826d7_00f1b4b8 PS19, Line 43: 0x64 > 100 Done File src/ec/starlabs/merlin/acpi/ec.asl: https://review.coreboot.org/c/coreboot/+/58343/comment/364f10ae_d6e5e192 PS19, Line 67: Embedded > nit: `Embedded Controller`? Done https://review.coreboot.org/c/coreboot/+/58343/comment/5f3154c2_f859e84b PS19, Line 104: // ECWT (Embedded Write Method) > What exactly does this method do? Is it used anywhere? I'll stop being a horder. https://review.coreboot.org/c/coreboot/+/58343/comment/fd3c6ff1_a8ccc6ad PS19, Line 135: Embedded > nit: `Embedded Controller`? Done https://review.coreboot.org/c/coreboot/+/58343/comment/08c8182e_fb731e80 PS19, Line 170: Method (ECMD, 0, Serialized) : { : Return (0x00) : } : : Method (ECM1, 0, Serialized) : { : Return (0x00) : } : : Method (ECNT, 0, Serialized) : { : Return (0x00) : } > Are these methods needed? Done File src/ec/starlabs/merlin/acpi/hid.asl: https://review.coreboot.org/c/coreboot/+/58343/comment/64ced04c_81b60e88 PS19, Line 109: 0xFA > 250 Done https://review.coreboot.org/c/coreboot/+/58343/comment/01b46492_837ffa0b PS19, Line 111: 0x04 > 4 Done https://review.coreboot.org/c/coreboot/+/58343/comment/cc6f8c86_dc57b959 PS19, Line 244: Its > It's Done https://review.coreboot.org/c/coreboot/+/58343/comment/1929550d_fff434bc PS19, Line 248: Its > It's Done https://review.coreboot.org/c/coreboot/+/58343/comment/ece19da1_087229ca PS19, Line 255: Its > It's Done File src/ec/starlabs/merlin/acpi/keyboard.asl: https://review.coreboot.org/c/coreboot/+/58343/comment/9e23e954_0f590802 PS19, Line 23: _Q80 > Copy-paste error Done File src/ec/starlabs/merlin/acpi/thermal.asl: https://review.coreboot.org/c/coreboot/+/58343/comment/697a4856_f63672d0 PS19, Line 5: Notify (\_TZ.TZ01, 0x80 > Syntax error. Looks like the file is unused, I'd remove it. Done File src/ec/starlabs/merlin/acpi/typec.asl: https://review.coreboot.org/c/coreboot/+/58343/comment/5c737e5d_cbae64f3 PS19, Line 5: 0x32 > 50 Done https://review.coreboot.org/c/coreboot/+/58343/comment/c34705bf_61787882 PS19, Line 35: 0x32 > 50 Done https://review.coreboot.org/c/coreboot/+/58343/comment/19d93dab_9229fc3e PS19, Line 60: 0x32 > 50 Done File src/ec/starlabs/merlin/ec.h: https://review.coreboot.org/c/coreboot/+/58343/comment/52bb404b_468e2e50 PS19, Line 41: /* Keyboard Backlight Timeout */ : #define SEC_30 0x00 : #define MIN_1 0x01 : #define MIN_3 0x02 : #define MIN_5 0x03 : #define NEVER 0x04 : : /* Fn Ctrl Swap */ : #define FN_CTRL 0x00 : #define CTRL_FN 0x01 : : /* Max Charge Setting */ : #define CHARGE_100 0x00 : #define CHARGE_80 0xbb : #define CHARGE_60 0xaa : : /* Fan Mode Setting */ : #define FAN_NORMAL 0x00 : #define FAN_AGGRESSIVE 0xbb : #define FAN_QUIET 0xaa : : /* Fn Lock State */ : #define UNLOCKED 0x00 : #define LOCKED 0x01 : : /* Trackpad State */ : #define TRACKPAD_ENABLED 0x00 : #define TRACKPAD_DISABLED 0x22 : : /* Keyboard Brightness Levels */ : #define KBL_ON 0xdd : #define KBL_OFF 0xcc : #define KBL_LOW 0xbb : #define KBL_HIGH 0xaa : : /* Keyboard Backlight State */ : #define KBL_DISABLED 0x00 : #define KBL_ENABLED 0xaa > I'd use actual enums for these instead of macros. Done File src/ec/starlabs/merlin/ec.c: https://review.coreboot.org/c/coreboot/+/58343/comment/4fca8885_2176c394 PS19, Line 68: 0 > I'd use a named value here Done https://review.coreboot.org/c/coreboot/+/58343/comment/3bbaf25b_3a48f59b PS19, Line 81: default: > nit: for completeness, I'd explicitly handle `SEC_30`: […] Done https://review.coreboot.org/c/coreboot/+/58343/comment/604b9652_b4d389fa PS19, Line 59: /* : * Keyboard Backlight Timeout : * : * Setting: kbl_timeout : * : * Values: 30 Seconds, 1 Minute, 3 Minutes, 5 Minutes, Never : * Default: 30 Seconds : * : */ : switch (get_uint_option("kbl_timeout", 0)) { : case MIN_1: : ec_write(ECRAM_KBL_TIMEOUT, MIN_1); : break; : case MIN_3: : ec_write(ECRAM_KBL_TIMEOUT, MIN_3); : break; : case MIN_5: : ec_write(ECRAM_KBL_TIMEOUT, MIN_5); : break; : case NEVER: : ec_write(ECRAM_KBL_TIMEOUT, NEVER); : break; : default: : ec_write(ECRAM_KBL_TIMEOUT, SEC_30); : break; : } > Idea: Encapsulate these into helper functions? Done https://review.coreboot.org/c/coreboot/+/58343/comment/0d573910_e47b6279 PS19, Line 113: switch (get_uint_option("max_charge", 0)) { : case CHARGE_80: : ec_write(ECRAM_MAX_CHARGE, CHARGE_80); : break; : case CHARGE_60: : ec_write(ECRAM_MAX_CHARGE, CHARGE_60); : break; : default: : ec_write(ECRAM_MAX_CHARGE, CHARGE_100); : break; : } > cmos.layout of CB:56088 won't work with this, the default case will always be taken. […] Done https://review.coreboot.org/c/coreboot/+/58343/comment/74c45d63_ee8feccf PS19, Line 134: #if CONFIG(EC_STARLABS_FAN) : switch (get_uint_option("fan_mode", 0)) { : case FAN_AGGRESSIVE: : ec_write(ECRAM_FAN_MODE, FAN_AGGRESSIVE); : break; : case FAN_QUIET: : ec_write(ECRAM_FAN_MODE, FAN_QUIET); : break; : default: : ec_write(ECRAM_FAN_MODE, FAN_NORMAL); : break; : } : #endif > Why not use a regular C check? Done https://review.coreboot.org/c/coreboot/+/58343/comment/eccf5575_d3daf6d7 PS19, Line 193: #if CONFIG(EC_STARLABS_KBL_LEVELS) : switch (get_uint_option("kbl_brightness", 0)) { : case KBL_OFF: : ec_write(ECRAM_KBL_BRIGHTNESS, KBL_OFF); : break; : case KBL_HIGH: : ec_write(ECRAM_KBL_BRIGHTNESS, KBL_HIGH); : break; : default: : ec_write(ECRAM_KBL_BRIGHTNESS, KBL_LOW); : break; : } : #else : switch (get_uint_option("kbl_brightness", 0)) { : case KBL_OFF: : ec_write(ECRAM_KBL_BRIGHTNESS, KBL_OFF); : break; : default: : ec_write(ECRAM_KBL_BRIGHTNESS, KBL_ON); : break; : } : #endif > Same as before Done https://review.coreboot.org/c/coreboot/+/58343/comment/4d45a1cf_503a3236 PS19, Line 251: ITE ITE > One "ITE" is enough? lol -- To view, visit https://review.coreboot.org/c/coreboot/+/58343 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I8023c26de23c874c84106fda96e64dcfa0c5ba32 Gerrit-Change-Number: 58343 Gerrit-PatchSet: 19 Gerrit-Owner: Sean Rhodes <admin(a)starlabs.systems> Gerrit-Reviewer: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Wed, 10 Nov 2021 15:59:59 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: ec/starlabs: Add standardised ITE EC support
by Sean Rhodes (Code Review) 10 Nov '21

10 Nov '21

Attention is currently required from: Sean Rhodes, Paul Menzel. Hello build bot (Jenkins), Patrick Georgi, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/58343 to look at the new patch set (#20). Change subject: ec/starlabs: Add standardised ITE EC support ...................................................................... ec/starlabs: Add standardised ITE EC support Add EC support that supports different Q Events and EC memory. Created from the ITE IT5570E and IT8987E datasheets, all using data port 0x4e. Tested with Ubuntu 20.04.3 and Windows 10 on: * StarBook Mk V (TGL + IT5570E): * ITE Firmware 1.00 * Merlin Firmware 1.00 * LabTop Mk IV (CML + IT8987E): * ITE Firmware 1.04 * LabTop Mk III (KBL + IT8987E): * ITE Firmware 3.12 Signed-off-by: Sean Rhodes <sean(a)starlabs.systems> Change-Id: I8023c26de23c874c84106fda96e64dcfa0c5ba32 --- M 3rdparty/blobs A src/ec/starlabs/merlin/Kconfig A src/ec/starlabs/merlin/Makefile.inc A src/ec/starlabs/merlin/acpi/ac.asl A src/ec/starlabs/merlin/acpi/battery.asl A src/ec/starlabs/merlin/acpi/cmos.asl A src/ec/starlabs/merlin/acpi/ec.asl A src/ec/starlabs/merlin/acpi/hid.asl A src/ec/starlabs/merlin/acpi/keyboard.asl A src/ec/starlabs/merlin/acpi/lid.asl A src/ec/starlabs/merlin/acpi/suspend.asl A src/ec/starlabs/merlin/acpi/typec.asl A src/ec/starlabs/merlin/acpi/ubtc.asl A src/ec/starlabs/merlin/ec.c A src/ec/starlabs/merlin/ec.h A src/ec/starlabs/merlin/variants/apl/ecdefs.h A src/ec/starlabs/merlin/variants/apl/emem.asl A src/ec/starlabs/merlin/variants/apl/events.asl A src/ec/starlabs/merlin/variants/cml/ecdefs.h A src/ec/starlabs/merlin/variants/cml/emem.asl A src/ec/starlabs/merlin/variants/cml/events.asl A src/ec/starlabs/merlin/variants/glk/ecdefs.h A src/ec/starlabs/merlin/variants/glk/emem.asl A src/ec/starlabs/merlin/variants/glk/events.asl A src/ec/starlabs/merlin/variants/kbl/ecdefs.h A src/ec/starlabs/merlin/variants/kbl/emem.asl A src/ec/starlabs/merlin/variants/kbl/events.asl A src/ec/starlabs/merlin/variants/merlin/ecdefs.h A src/ec/starlabs/merlin/variants/merlin/emem.asl A src/ec/starlabs/merlin/variants/merlin/events.asl A src/ec/starlabs/merlin/variants/tgl/ecdefs.h A src/ec/starlabs/merlin/variants/tgl/emem.asl A src/ec/starlabs/merlin/variants/tgl/events.asl 33 files changed, 3,627 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/58343/20 -- To view, visit https://review.coreboot.org/c/coreboot/+/58343 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I8023c26de23c874c84106fda96e64dcfa0c5ba32 Gerrit-Change-Number: 58343 Gerrit-PatchSet: 20 Gerrit-Owner: Sean Rhodes <admin(a)starlabs.systems> Gerrit-Reviewer: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Sean Rhodes <admin(a)starlabs.systems> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: ChromeOS: Fix <vc/google/chromeos/chromeos.h>
by Kyösti Mälkki (Code Review) 10 Nov '21

10 Nov '21

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58923 ) Change subject: ChromeOS: Fix <vc/google/chromeos/chromeos.h> ...................................................................... Patch Set 12: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/58923/comment/e3caa84d_e97c90cd PS12, Line 7: ChromeOS: Fix <vc/google/chromeos/chromeos.h> > What does "fix" mean? Here; one of spurious or missing include, for a lazy old dog too bothered to get interested about quick brown fox jumping all over. -- To view, visit https://review.coreboot.org/c/coreboot/+/58923 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ibbdd589119bbccd3516737c8ee9f90c4bef17c1e Gerrit-Change-Number: 58923 Gerrit-PatchSet: 12 Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Huang Jin <huang.jin(a)intel.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Lee Leahy <leroy.p.leahy(a)intel.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Comment-Date: Wed, 10 Nov 2021 15:57:28 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mb/google/guybrush/dewatt: update dewatt config
by Rob Barnes (Code Review) 10 Nov '21

10 Nov '21

Attention is currently required from: Chris Wang, Karthik Ramasubramanian. Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59092 ) Change subject: mb/google/guybrush/dewatt: update dewatt config ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/59092 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ide9e002390e59725dc0e45f83280db2a78270993 Gerrit-Change-Number: 59092 Gerrit-PatchSet: 1 Gerrit-Owner: Chris Wang <chris.wang(a)amd.corp-partner.google.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Rob Barnes <robbarnes(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Chris Wang <chris.wang(a)amd.corp-partner.google.com> Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Comment-Date: Wed, 10 Nov 2021 15:55:57 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: ChromeOS: Fix <vc/google/chromeos/chromeos.h>
by Angel Pons (Code Review) 10 Nov '21

10 Nov '21

Attention is currently required from: Kyösti Mälkki. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58923 ) Change subject: ChromeOS: Fix <vc/google/chromeos/chromeos.h> ...................................................................... Patch Set 12: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/58923/comment/c4ea46a8_b0eabeec PS12, Line 7: ChromeOS: Fix <vc/google/chromeos/chromeos.h> What does "fix" mean? -- To view, visit https://review.coreboot.org/c/coreboot/+/58923 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ibbdd589119bbccd3516737c8ee9f90c4bef17c1e Gerrit-Change-Number: 58923 Gerrit-PatchSet: 12 Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Huang Jin <huang.jin(a)intel.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Lee Leahy <leroy.p.leahy(a)intel.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Comment-Date: Wed, 10 Nov 2021 15:45:10 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: Documentation/coding_style.md: Avoid weakly linked symbols
by Arthur Heymans (Code Review) 10 Nov '21

10 Nov '21

Attention is currently required from: Paul Menzel, Julius Werner. Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58934 ) Change subject: Documentation/coding_style.md: Avoid weakly linked symbols ...................................................................... Patch Set 2: (1 comment) Patchset: PS2: > Has there been some recent discussion about this somewhere that I may have missed? There's not a lot of context on this CL (commit message?) and it seems to go against practice that has been established in many parts of coreboot for years. > > I don't really agree with this at least in the form provided here -- I think weak functions are fine in the right context and cluttering Kconfig with a billion PLATFORM_USES_HOOK_XYZ isn't fundamentally a better alternative. We can discuss which situations are and aren't a good fit for weak symbols and then try to agree on some guidelines based on that, but I don't think a blanket "they're bad" is the right approach. > > Personally, I think weak functions are a good solution whenever generic code has a hook that can optionally be implemented by more specific (e.g. SoC or mainboard) code. Good examples for this are things like bootblock_soc_init(), bootblock_mainboard_init(), bootblock_mainboard_early_init(), etc... most platforms implement most of these functions, but sometimes not all of them. Hiding each of them behind a Kconfig and splattering every soc/.../Kconfig file with `select HAS_BOOTBLOCK_SOC_INIT`, `select HAS_BOOTBLOCK_SOC_EARLY_INIT`, etc. would just create a lot more config boilerplate (making it harder to read the meaningful options in between) and I don't think it would really help anyone's understanding of the code. The other alternative would be to add empty stub functions to each individual mainboard/SoC file when they're not using those hooks, but that's just adding more boilerplate there and I don't see how it would really make anything better either... also, it is a really annoying maintenance burden because whenever you need to add a new hook somewhere, you would have to add empty stubs to a billion SoC/mainboard files. I think weak functions solve this problem well and without causing any real issues... I mean, it's a hook, either you do something there or you don't, when you forgot to implement it then I guess your platform didn't need it anyway and the default weak no-op function was the right choice for you. > So I agree that optional mainboard specific hooks are not too bad. SOC specific weak override, I already like a lot less. The problem is that those weak functions are often used initially just to get things compile tested. In the end a real implementation is needed but forgotten because jenkins does not complain. A good example of this can be found in: https://review.coreboot.org/c/coreboot/+/58649 and https://review.coreboot.org/c/coreboot/+/58662/2. https://review.coreboot.org/c/coreboot/+/58657 was also needed, which pulled in a lot of NOP code due to the existence of weak functions. > On the other hand, I do agree that weak functions can make things very confusing when the weak function actually implements meaningful behavior, and then overriding the function means you're missing that behavior. That's a terrible pattern that I think/hope we don't really use in coreboot anyway, and I think codifying that in the style guide would be fine. Weak functions should only be allowed when the weak implementation is a no-op or otherwise represents the most simple and "default" way the function could be implemented (and when that default implementation is generally "safe" from a security standpoint -- e.g. the weak implementation of vboot's get_recovery_mode_switch() is behind an extra Kconfig for a reason), but for those cases I think they can be an appropriate tool. I don't think there are many cases of weak functions implementing actual meaningful behavior. There are however quite a few 'placeholders' weak functions that should have a meaningful implementation, but don't. Those placeholder functions sometimes even print to the console that an implementation is needed. This really pushes a problem from compiletime to runtime, and I'd like to stop that practice. -- To view, visit https://review.coreboot.org/c/coreboot/+/58934 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: If70d38c5c646c1e8365bb16d3292cebb2787eba2 Gerrit-Change-Number: 58934 Gerrit-PatchSet: 2 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Hung-Te Lin <hungte(a)chromium.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Comment-Date: Wed, 10 Nov 2021 15:29:34 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: amdfwtool: Pack out-of-bounds check into a function and move
by Felix Held (Code Review) 10 Nov '21

10 Nov '21

Attention is currently required from: Bao Zheng, Martin Roth, Marshall Dawson, Zheng Bao. Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58764 ) Change subject: amdfwtool: Pack out-of-bounds check into a function and move ...................................................................... Patch Set 5: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/58764 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I71117d1c817c0b6ddaea4ea47aea91672cc6d55a Gerrit-Change-Number: 58764 Gerrit-PatchSet: 5 Gerrit-Owner: Bao Zheng <fishbaozi(a)gmail.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Zheng Bao Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Patrick Georgi <patrick(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Bao Zheng <fishbaozi(a)gmail.com> Gerrit-Attention: Martin Roth <martinroth(a)google.com> Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Attention: Zheng Bao Gerrit-Comment-Date: Wed, 10 Nov 2021 15:27:11 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/amd/cezanne: enable LPC decodes if platform uses LPC
by Jeremy Soller (Code Review) 10 Nov '21

10 Nov '21

Jeremy Soller has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/59103 ) Change subject: soc/amd/cezanne: enable LPC decodes if platform uses LPC ...................................................................... soc/amd/cezanne: enable LPC decodes if platform uses LPC Change-Id: I2473fe61b299d1c6221844cd744791b8012c5c67 --- M src/soc/amd/cezanne/early_fch.c 1 file changed, 9 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/59103/1 diff --git a/src/soc/amd/cezanne/early_fch.c b/src/soc/amd/cezanne/early_fch.c index f06d576..8cad78f 100644 --- a/src/soc/amd/cezanne/early_fch.c +++ b/src/soc/amd/cezanne/early_fch.c @@ -21,6 +21,12 @@ I2C_RESET_SCL_PIN(I2C3_SCL_PIN, GPIO_I2C3_SCL), }; +static void lpc_configure_decodes(void) +{ + if (CONFIG(POST_IO) && (CONFIG_POST_IO_PORT == 0x80)) + lpc_enable_port80(); +} + static void reset_i2c_peripherals(void) { const struct soc_amd_cezanne_config *cfg = config_of_soc(); @@ -41,6 +47,9 @@ /* Setup SPI base by calling lpc_early_init before setting up eSPI. */ lpc_early_init(); + if (!CONFIG(SOC_AMD_COMMON_BLOCK_USE_ESPI)) + lpc_configure_decodes(); + /* Setup eSPI to enable port80 routing if the board is using eSPI and the eSPI interface hasn't already been set up in verstage on PSP */ if (CONFIG(SOC_AMD_COMMON_BLOCK_USE_ESPI) && !CONFIG(VBOOT_STARTS_BEFORE_BOOTBLOCK)) -- To view, visit https://review.coreboot.org/c/coreboot/+/59103 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I2473fe61b299d1c6221844cd744791b8012c5c67 Gerrit-Change-Number: 59103 Gerrit-PatchSet: 1 Gerrit-Owner: Jeremy Soller <jeremy(a)system76.com> Gerrit-MessageType: newchange

1 0

Change in coreboot[master]: mb/google/brya/var/gimble: Improve USB2 eye diagram of DB Type-C port
by Zhuohao Lee (Code Review) 10 Nov '21

10 Nov '21

Attention is currently required from: YH Lin, Tim Wawrzynczak, Mark Hsieh, Felix Held, Zhuohao Lee. Zhuohao Lee has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59100 ) Change subject: mb/google/brya/var/gimble: Improve USB2 eye diagram of DB Type-C port ...................................................................... Patch Set 2: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/59100 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Idcf13ad072ae5d7a897f54adb19e6b2b068609dc Gerrit-Change-Number: 59100 Gerrit-PatchSet: 2 Gerrit-Owner: Mark Hsieh <mark_hsieh(a)wistron.corp-partner.google.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Mark Hsieh <mark_hsieh(a)wistron.corp-partner.google.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: YH Lin <yueherngl(a)google.com> Gerrit-Reviewer: Zhuohao Lee <zhuohao(a)chromium.org> Gerrit-Reviewer: Zhuohao Lee <zhuohao(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anfernee Chen <anfernee_chen(a)wistron.corp-partner.google.com> Gerrit-CC: Ariel Fang <ariel_fang(a)wistron.corp-partner.google.com> Gerrit-CC: Casper Chang <casper_chang(a)wistron.corp-partner.google.com> Gerrit-CC: Malik Hsu <malik_hsu(a)wistron.corp-partner.google.com> Gerrit-CC: Scott Chao <scott_chao(a)wistron.corp-partner.google.com> Gerrit-CC: Terry Chen <terry_chen(a)wistron.corp-partner.google.com> Gerrit-CC: Will Tsai <will_tsai(a)wistron.corp-partner.google.com> Gerrit-Attention: YH Lin <yueherngl(a)google.com> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Mark Hsieh <mark_hsieh(a)wistron.corp-partner.google.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Attention: Zhuohao Lee <zhuohao(a)chromium.org> Gerrit-Comment-Date: Wed, 10 Nov 2021 14:52:54 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit November 2021