coreboot-gerrit November 2021

coreboot-gerrit@coreboot.org

1 participants
6394 discussions

Start a nNew thread

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

15 Nov '21

1 0

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

15 Nov '21

Attention is currently required from: Andy Pont, Paul Menzel, Angel Pons, Felix Held. 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 37: (9 comments) File src/ec/starlabs/merlin/Kconfig: https://review.coreboot.org/c/coreboot/+/58343/comment/7d9ad14a_e4ad93fb PS37, Line 22: bool "Adjustable Keyboard Backlight" > Is this meant to be user-configurable? Or is it mainboard-dependent? Mainboard dependent. Updated help text, is that alright? https://review.coreboot.org/c/coreboot/+/58343/comment/19dc22ec_d4fc4c1a PS37, Line 36: Use open-source ec aka Merlin > How about `Use open-source Merlin EC firmware` ? Done File src/ec/starlabs/merlin/Makefile.inc: https://review.coreboot.org/c/coreboot/+/58343/comment/9e00b0c8_ddf56400 PS37, Line 9: $(VARIANT_DIR) > Is this using `CONFIG_VARIANT_DIR` from mainboard settings? I'd much prefer to add more Kconfig opti […] Like so? https://review.coreboot.org/c/coreboot/+/58343/comment/9fb0831d_d8c09628 PS37, Line 27: Mk IV > Just this model? Done File src/ec/starlabs/merlin/acpi/battery.asl: https://review.coreboot.org/c/coreboot/+/58343/comment/50710efc_b6e90942 PS37, Line 27: 0x64 > 100 Done File src/ec/starlabs/merlin/ec.c: https://review.coreboot.org/c/coreboot/+/58343/comment/c84e6ea0_f7249094 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 > Done Just realised why I didn't before - APL/GLK/GLK-R don't have ECRAM_FAN_MODE defined so won't build with C. I could just define it as `0x00`? File src/ec/starlabs/merlin/ec.c: https://review.coreboot.org/c/coreboot/+/58343/comment/3c452a64_8ff74eaa PS37, Line 79: 5 > nit: specifying the size isn't necessary when using an array initializer Done https://review.coreboot.org/c/coreboot/+/58343/comment/ebae76fe_2980f284 PS37, Line 201: Off, Low, High > Missing `On` ? Done File src/ec/starlabs/merlin/variants/apl/ecdefs.h: https://review.coreboot.org/c/coreboot/+/58343/comment/4169051f_64a968d2 PS37, Line 4: ITE ITE > One is enough Done -- 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: 37 Gerrit-Owner: Sean Rhodes <admin(a)starlabs.systems> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Andy Pont <andy.pont(a)sdcsystems.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Andy Pont <andy.pont(a)sdcsystems.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Mon, 15 Nov 2021 13:54:29 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Sean Rhodes <admin(a)starlabs.systems> 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) 15 Nov '21

15 Nov '21

Attention is currently required from: Sean Rhodes, Andy Pont, Paul Menzel, Felix Held. Hello build bot (Jenkins), Patrick Georgi, Angel Pons, Felix Held, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/58343 to look at the new patch set (#38). 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 --- 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 32 files changed, 3,692 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/58343/38 -- 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: 38 Gerrit-Owner: Sean Rhodes <admin(a)starlabs.systems> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Andy Pont <andy.pont(a)sdcsystems.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Sean Rhodes <admin(a)starlabs.systems> Gerrit-Attention: Andy Pont <andy.pont(a)sdcsystems.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: soc/mediatek/mt8186: Add I2C driver support
by Yu-Ping Wu (Code Review) 15 Nov '21

15 Nov '21

Attention is currently required from: Hung-Te Lin, Rex-BC Chen. Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59296 ) Change subject: soc/mediatek/mt8186: Add I2C driver support ...................................................................... Patch Set 5: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/59296 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ia3800e3a30b0796a64213d3b1ab688580c6ddbca Gerrit-Change-Number: 59296 Gerrit-PatchSet: 5 Gerrit-Owner: Rex-BC Chen <rex-bc.chen(a)mediatek.corp-partner.google.com> Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Attention: Rex-BC Chen <rex-bc.chen(a)mediatek.corp-partner.google.com> Gerrit-Comment-Date: Mon, 15 Nov 2021 13:21:09 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: Documentation/drivers: Add notes on Realtek HW EQ
by Sean Rhodes (Code Review) 15 Nov '21

15 Nov '21

Attention is currently required from: Matt DeVillier, Angel Pons. Sean Rhodes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59263 ) Change subject: Documentation/drivers: Add notes on Realtek HW EQ ...................................................................... Patch Set 4: (8 comments) File Documentation/drivers/realtek.md: https://review.coreboot.org/c/coreboot/+/59263/comment/f4ec6252_37c1b9d8 PS4, Line 12: mamed > typo? ma*i*med Done https://review.coreboot.org/c/coreboot/+/59263/comment/63413ef2_ed6a01cb PS4, Line 4: The datasheet has extensive documentation on this, as after : it says that it supports it, it provides all the below information on : how to configure it: : : ``` : : ``` : As this information is extemely helpful, we wrote up the notes below on : how to configure it. > While I appreciate the sense of humour, I'm not sure if everyone would understand the sarcasm here. […] Done https://review.coreboot.org/c/coreboot/+/59263/comment/bf833514_7e0d27e3 PS4, Line 19: 53 > 0x53, actually. Same for all other values. Done https://review.coreboot.org/c/coreboot/+/59263/comment/93bb773c_d65d8f41 PS4, Line 28: 0x053404DA > nit: Hex values in coreboot are typically written in lowercase Done https://review.coreboot.org/c/coreboot/+/59263/comment/b50acd6a_6d29e372 PS4, Line 41: Butterworth HPF > Butterworth High-Pass Filter? Maybe link to Wikipedia? […] Done https://review.coreboot.org/c/coreboot/+/59263/comment/7a80982c_62ab740e PS4, Line 136: > trailing space Done https://review.coreboot.org/c/coreboot/+/59263/comment/6e59e3bf_641b8dd6 PS4, Line 136: /* : * EQ Mode = Boost Gain : * : * High Pass Filter 1: : * Band Width = 1000Hz : * Gain (dB) = 0dB : * : * High Pass Filter 2: : * Band Width = 1000Hz : * Gain (dB) = 0dB : * : * Band Pass Filter 1 : * Frequency = 3000Hz : * Band Width = 1000Hz : * Gain = 0dB : * : * Band Pass Filter 2 : * Frequency = 6000Hz : * Band Width = 1000Hz : * Gain = 0dB : * : * Band Pass Filter 3 : * Frequency = 9000Hz : * Band Width = 1000Hz : * Gain = 0dB : * : * Band Pass Filter 4 : * Frequency = 9000Hz : * Band Width = 1000Hz : * Gain = 0dB : * : * Low Pass Filter 1 : * Band Width = 1000Hz : * Gain (dB) = 0dB : * : * AGC Threshold = -6dB : * AGC Front Boost Gain = 6dB : * AGC Post Boost Gain = 6dB : * : * Power Output = 2.5W : */ > I'd appreciate if you could split this comment and interleave the parts with the verb list, so that […] Done https://review.coreboot.org/c/coreboot/+/59263/comment/2277bdc1_4f038fb3 PS4, Line 306: > Missing closing ``` ? Done -- To view, visit https://review.coreboot.org/c/coreboot/+/59263 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie13f62622a4cf2b14d36728a8a4a9e3ef24d6852 Gerrit-Change-Number: 59263 Gerrit-PatchSet: 4 Gerrit-Owner: Sean Rhodes <admin(a)starlabs.systems> Gerrit-Reviewer: Angel Pons <angel.pons(a)9elements.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Angel Pons <angel.pons(a)9elements.com> Gerrit-Comment-Date: Mon, 15 Nov 2021 13:18:08 +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]: Documentation/drivers: Add notes on Realtek HW EQ
by Sean Rhodes (Code Review) 15 Nov '21

15 Nov '21

Attention is currently required from: Sean Rhodes, Matt DeVillier, Angel Pons. Hello build bot (Jenkins), Matt DeVillier, Angel Pons, Angel Pons, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/59263 to look at the new patch set (#6). Change subject: Documentation/drivers: Add notes on Realtek HW EQ ...................................................................... Documentation/drivers: Add notes on Realtek HW EQ Signed-off-by: Sean Rhodes <sean(a)starlabs.systems> Change-Id: Ie13f62622a4cf2b14d36728a8a4a9e3ef24d6852 --- A Documentation/drivers/realtek.md 1 file changed, 267 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/59263/6 -- To view, visit https://review.coreboot.org/c/coreboot/+/59263 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie13f62622a4cf2b14d36728a8a4a9e3ef24d6852 Gerrit-Change-Number: 59263 Gerrit-PatchSet: 6 Gerrit-Owner: Sean Rhodes <admin(a)starlabs.systems> Gerrit-Reviewer: Angel Pons <angel.pons(a)9elements.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Sean Rhodes <admin(a)starlabs.systems> Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Attention: Angel Pons <angel.pons(a)9elements.com> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: cpu/intel/haswell: Remove the fake lapic
by Angel Pons (Code Review) 15 Nov '21

15 Nov '21

Attention is currently required from: Arthur Heymans, Raul Rangel, Tristan Corrick, Alexander Couzens, Patrick Rudolph. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58604 ) Change subject: cpu/intel/haswell: Remove the fake lapic ...................................................................... Patch Set 1: (2 comments) File src/cpu/intel/haswell/acpi.c: https://review.coreboot.org/c/coreboot/+/58604/comment/b68bc784_bfc92816 PS1, Line 175: s0ix_enable > Thanks for the explanation. […] Now that I think of it, a `config_of_cpu()` helper would be useful to have. File src/cpu/intel/haswell/haswell_init.c: https://review.coreboot.org/c/coreboot/+/58604/comment/1eefb1b1_7fa0ec0b PS1, Line 182: struct cpu_info *info = cpu_info(); : if (!info) : return; > It should never return a NULL pointer. Yes, I checked the code and saw that something would need to go very wrong for `cpu_info()` to return a null pointer. I was pointing out the inconsistent usage of null checks. So, this check can be dropped. -- To view, visit https://review.coreboot.org/c/coreboot/+/58604 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I69f9c1fe71a773e1fba0942d8a785b6ad26dd9a0 Gerrit-Change-Number: 58604 Gerrit-PatchSet: 1 Gerrit-Owner: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Reviewer: Alexander Couzens <lynxis(a)fe80.eu> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Tristan Corrick <tristan(a)corrick.kiwi> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Raul Rangel <rrangel(a)chromium.org> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Arthur Heymans <arthur.heymans(a)9elements.com> Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Tristan Corrick <tristan(a)corrick.kiwi> Gerrit-Attention: Alexander Couzens <lynxis(a)fe80.eu> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Mon, 15 Nov 2021 13:04:43 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Raul Rangel <rrangel(a)chromium.org> 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 Angel Pons (Code Review) 15 Nov '21

15 Nov '21

Attention is currently required from: Sean Rhodes, Andy Pont, Paul Menzel, Felix Held. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58343 ) Change subject: ec/starlabs: Add standardised ITE EC support ...................................................................... Patch Set 37: Code-Review+1 (15 comments) File src/ec/starlabs/merlin/Kconfig: https://review.coreboot.org/c/coreboot/+/58343/comment/00abd36a_50b3e6bf PS37, Line 22: bool "Adjustable Keyboard Backlight" Is this meant to be user-configurable? Or is it mainboard-dependent? https://review.coreboot.org/c/coreboot/+/58343/comment/d263a2d2_2568d646 PS37, Line 29: bool "Fan device" Same as above. https://review.coreboot.org/c/coreboot/+/58343/comment/78f96a89_834ae672 PS37, Line 36: Use open-source ec aka Merlin How about `Use open-source Merlin EC firmware` ? File src/ec/starlabs/merlin/Makefile.inc: https://review.coreboot.org/c/coreboot/+/58343/comment/a4ec9bf3_c103bbd3 PS37, Line 9: $(VARIANT_DIR) Is this using `CONFIG_VARIANT_DIR` from mainboard settings? I'd much prefer to add more Kconfig options so that mainboards can select the right EC type. You can also add a `EC_VARIANT_DIR` option to do this, which would do the same thing but also avoid having to treat Merlin separately (just prioritize its conditional default by putting it before the other defaults). https://review.coreboot.org/c/coreboot/+/58343/comment/58ad9fcd_928c8870 PS37, Line 27: Mk IV Just this model? File src/ec/starlabs/merlin/acpi/battery.asl: https://review.coreboot.org/c/coreboot/+/58343/comment/310a8527_9476b5a4 PS37, Line 27: 0x64 100 https://review.coreboot.org/c/coreboot/+/58343/comment/f0b876aa_fd296d9e PS37, Line 36: 0x01 I'd use decimal for the indices File src/ec/starlabs/merlin/acpi/cmos.asl: PS37: Would be really interesting to generate these definitions from cmos.layout but for now this is good enough. File src/ec/starlabs/merlin/acpi/hid.asl: https://review.coreboot.org/c/coreboot/+/58343/comment/8af4cf26_113a60c6 PS37, Line 14: Usually, ACPI will check if the OS is 0x07DD (2013 - Windows 8.1ish) And this is why Linux claims to be Windows-compatible in ACPI _OSI queries. No one should be using older Windows versions on these machines anyway, so I think the chosen approach is best. File src/ec/starlabs/merlin/ec.h: https://review.coreboot.org/c/coreboot/+/58343/comment/a01d1c51_8aab01b4 PS37, Line 12: binary blob `EC firmware`? File src/ec/starlabs/merlin/ec.c: https://review.coreboot.org/c/coreboot/+/58343/comment/c554a22b_ccddb9f1 PS37, Line 79: 5 nit: specifying the size isn't necessary when using an array initializer https://review.coreboot.org/c/coreboot/+/58343/comment/bc890c70_abeaae1b PS37, Line 201: Off, Low, High Missing `On` ? https://review.coreboot.org/c/coreboot/+/58343/comment/34e8f56d_e228c4f5 PS37, Line 212: if (CONFIG(EC_STARLABS_KBL_LEVELS)) : ec_write(ECRAM_KBL_BRIGHTNESS, : get_ec_value_from_option("kbl_brightness", : 2, : kbl_brightness, : ARRAY_SIZE(kbl_brightness))); : else : ec_write(ECRAM_KBL_BRIGHTNESS, : get_ec_value_from_option("kbl_brightness", : 0, : kbl_brightness, : ARRAY_SIZE(kbl_brightness))); I'd use two different tables, but I'm fine with the current approach. File src/ec/starlabs/merlin/variants/apl/ecdefs.h: https://review.coreboot.org/c/coreboot/+/58343/comment/3218ce84_36cb6fa2 PS37, Line 4: ITE ITE One is enough https://review.coreboot.org/c/coreboot/+/58343/comment/af056c5b_f2b0674b PS37, Line 7: _EC_STARLABS_ITE_CFG_H I'd avoid having the same guard name on multiple files so that there's a build-time error if conflicting headers are included (e.g. both apl and merlin headers are included). How about `EC_STARLABS_APL_EC_DEFS_H` ? Same logic for the other headers. -- 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: 37 Gerrit-Owner: Sean Rhodes <admin(a)starlabs.systems> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Patrick Georgi <patrick(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Andy Pont <andy.pont(a)sdcsystems.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Sean Rhodes <admin(a)starlabs.systems> Gerrit-Attention: Andy Pont <andy.pont(a)sdcsystems.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Mon, 15 Nov 2021 12:51:59 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mb/google/brya/var/vell: update gpio override
by build bot (Jenkins) (Code Review) 15 Nov '21

15 Nov '21

Attention is currently required from: Tim Wawrzynczak, Kevin Chiu. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59305 ) Change subject: mb/google/brya/var/vell: update gpio override ...................................................................... Patch Set 1: (3 comments) File src/mainboard/google/brya/variants/vell/gpio.c: Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-133252): https://review.coreboot.org/c/coreboot/+/59305/comment/5e9d7925_887ffea9 PS1, Line 55: /* D11 : ISH_SPI_MISO ==> USB_C0_LSX_SOC_TX */ code indent should use tabs where possible Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-133252): https://review.coreboot.org/c/coreboot/+/59305/comment/58fb9980_c2e18f54 PS1, Line 56: PAD_CFG_NF(GPP_D11, NONE, DEEP, NF4), code indent should use tabs where possible Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-133252): https://review.coreboot.org/c/coreboot/+/59305/comment/7d103f44_895eca81 PS1, Line 56: PAD_CFG_NF(GPP_D11, NONE, DEEP, NF4), please, no spaces at the start of a line -- To view, visit https://review.coreboot.org/c/coreboot/+/59305 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icc91866f7555c294af7eed9e5d1550e73d8059d0 Gerrit-Change-Number: 59305 Gerrit-PatchSet: 1 Gerrit-Owner: Kevin Chiu <kevin.chiu.17802(a)gmail.com> Gerrit-Reviewer: Kevin Chiu <Kevin.Chiu(a)quantatw.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Attention: Kevin Chiu <Kevin.Chiu(a)quantatw.com> Gerrit-Comment-Date: Mon, 15 Nov 2021 12:50:58 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mb/google/brya/var/vell: update gpio override
by Kevin Chiu (Code Review) 15 Nov '21

15 Nov '21

Attention is currently required from: Kevin Chiu. Hello Kevin Chiu, I'd like you to do a code review. Please visit https://review.coreboot.org/c/coreboot/+/59305 to review the following change. Change subject: mb/google/brya/var/vell: update gpio override ...................................................................... mb/google/brya/var/vell: update gpio override BUG=b:205908918 TEST=emerge-brya coreboot Change-Id: Icc91866f7555c294af7eed9e5d1550e73d8059d0 Signed-off-by: Kevin Chiu <Kevin.Chiu(a)quantatw.com> --- A src/mainboard/google/brya/variants/vell/Makefile.inc A src/mainboard/google/brya/variants/vell/gpio.c 2 files changed, 100 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/05/59305/1 diff --git a/src/mainboard/google/brya/variants/vell/Makefile.inc b/src/mainboard/google/brya/variants/vell/Makefile.inc new file mode 100644 index 0000000..009117a --- /dev/null +++ b/src/mainboard/google/brya/variants/vell/Makefile.inc @@ -0,0 +1,4 @@ +# SPDX-License-Identifier: GPL-2.0-only +bootblock-y += gpio.c +romstage-y += gpio.c +ramstage-y += gpio.c \ No newline at end of file diff --git a/src/mainboard/google/brya/variants/vell/gpio.c b/src/mainboard/google/brya/variants/vell/gpio.c new file mode 100644 index 0000000..a4d1d54 --- /dev/null +++ b/src/mainboard/google/brya/variants/vell/gpio.c @@ -0,0 +1,96 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ + +#include <baseboard/gpio.h> +#include <baseboard/variants.h> +#include <commonlib/helpers.h> +#include <soc/gpio.h> + +/* Pad configuration in ramstage */ +static const struct pad_config override_gpio_table[] = { + /* B15 : TIME_SYNC0 ==> NC */ + PAD_NC(GPP_B15, NONE), + + /* C3 : SML0CLK ==> NC */ + PAD_NC(GPP_C3, NONE), + + /* E3 : PROC_GP0 ==> MEM_STRAP_0 */ + PAD_CFG_GPI(GPP_E3, NONE, DEEP), + /* E5 : SATA_DEVSLP1 ==> MEM_CH_SEL */ + PAD_CFG_GPI(GPP_E5, NONE, DEEP), + /* E7 : PROC_GP1 ==> MEM_STRAP_3 */ + PAD_CFG_GPI(GPP_E7, NONE, DEEP), + + /* S4 : SNDW2_CLK ==> NC */ + PAD_NC(GPP_S4, NONE), + /* S5 : SNDW2_DATA ==> NC */ + PAD_NC(GPP_S5, NONE), + /* S6 : SNDW3_CLK ==> NC */ + PAD_NC(GPP_S6, NONE), + /* S7 : SNDW3_DATA ==> NC */ + PAD_NC(GPP_S7, NONE), + +}; + +/* Early pad configuration in bootblock */ +static const struct pad_config early_gpio_table[] = { + /* A13 : PMC_I2C_SCL ==> GSC_PCH_INT_ODL */ + PAD_CFG_GPI_APIC(GPP_A13, NONE, PLTRST, LEVEL, INVERT), + /* B4 : PROC_GP3 ==> SSD_PERST_L */ + PAD_CFG_GPO(GPP_B4, 0, DEEP), + /* B7 : ISH_12C1_SDA ==> PCH_I2C_TPM_SDA */ + PAD_CFG_NF(GPP_B7, NONE, DEEP, NF2), + /* B8 : ISH_12C1_SCL ==> PCH_I2C_TPM_SCL */ + PAD_CFG_NF(GPP_B8, NONE, DEEP, NF2), + /* + * D1 : ISH_GP1 ==> FP_RST_ODL + * FP_RST_ODL comes out of reset as hi-z and does not have an external pull-down. + * To ensure proper power sequencing for the FPMCU device, reset signal is driven low + * early on in bootblock, followed by enabling of power. Reset signal is deasserted + * later on in ramstage. Since reset signal is asserted in bootblock, it results in + * FPMCU not working after a S3 resume. This is a known issue. + */ + PAD_CFG_GPO(GPP_D1, 0, DEEP), + /* D2 : ISH_GP2 ==> EN_FP_PWR */ + PAD_CFG_GPO(GPP_D2, 1, DEEP), + /* D11 : ISH_SPI_MISO ==> USB_C0_LSX_SOC_TX */ + PAD_CFG_NF(GPP_D11, NONE, DEEP, NF4), + + /* E3 : PROC_GP0 ==> MEM_STRAP_0 */ + PAD_CFG_GPI(GPP_E3, NONE, DEEP), + /* E5 : SATA_DEVSLP1 ==> MEM_CH_SEL */ + PAD_CFG_GPI(GPP_E5, NONE, DEEP), + /* E7 : PROC_GP1 ==> MEM_STRAP_3 */ + PAD_CFG_GPI(GPP_E7, NONE, DEEP), + + /* E15 : RSVD_TP ==> PCH_WP_OD */ + PAD_CFG_GPI_GPIO_DRIVER(GPP_E15, NONE, DEEP), + /* E16 : RSVD_TP ==> WWAN_RST_L */ + PAD_CFG_GPO(GPP_E16, 0, DEEP), + /* F18 : THC1_SPI2_INT# ==> EC_IN_RW_OD */ + PAD_CFG_GPI(GPP_F18, NONE, DEEP), + /* H10 : UART0_RXD ==> UART_PCH_RX_DBG_TX */ + PAD_CFG_NF(GPP_H10, NONE, DEEP, NF2), + /* H11 : UART0_TXD ==> UART_PCH_TX_DBG_RX */ + PAD_CFG_NF(GPP_H11, NONE, DEEP, NF2), + /* B4 : PROC_GP3 ==> SSD_PERST_L */ + PAD_CFG_GPO(GPP_B4, 0, DEEP), +}; + +static const struct pad_config romstage_gpio_table[] = { + /* B4 : PROC_GP3 ==> SSD_PERST_L */ + PAD_CFG_GPO(GPP_B4, 1, DEEP), + /* F21 : EXT_PWR_GATE2# ==> WWAN_FCPO_L (set here for correct power sequencing) */ + PAD_CFG_GPO(GPP_F21, 1, DEEP), +}; + +const struct pad_config *variant_gpio_override_table(size_t *num) +{ + *num = ARRAY_SIZE(override_gpio_table); + return override_gpio_table; +} + +const struct pad_config *variant_early_gpio_table(size_t *num) +{ + *num = ARRAY_SIZE(early_gpio_table); + return early_gpio_table; +} -- To view, visit https://review.coreboot.org/c/coreboot/+/59305 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Icc91866f7555c294af7eed9e5d1550e73d8059d0 Gerrit-Change-Number: 59305 Gerrit-PatchSet: 1 Gerrit-Owner: Kevin Chiu <kevin.chiu.17802(a)gmail.com> Gerrit-Reviewer: Kevin Chiu <Kevin.Chiu(a)quantatw.com> Gerrit-Attention: Kevin Chiu <Kevin.Chiu(a)quantatw.com> Gerrit-MessageType: newchange

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit November 2021