coreboot-gerrit May 2024

coreboot-gerrit@coreboot.org

1 participants
3266 discussions

[L] Change in coreboot[main]: mainboard/lenovo: Add ThinkCentre M710s (Skylake)
by Angel Pons (Code Review) 01 May '24

01 May '24

Attention is currently required from: Nicholas Sudsgaard. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80343?usp=email ) Change subject: mainboard/lenovo: Add ThinkCentre M710s (Skylake) ...................................................................... Patch Set 27: (2 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/80343/comment/15e4b8ba_0a68b740 : PS27, Line 34: - Power button LED Looks like this LED is connected to GPIO `GP10` on the Super I/O. But the input/output control register is only accessible through "Simple I/O" (`io 0x62 = 0x0a00`). You could try to toggle the GPIO output level through the control register using Simple I/O (as long as the base address has been programmed and LPC forwarding has been enabled), but I'm not even sure which register this would be. You could try comparing the Simple I/O address space between vendor firmware and coreboot by reading several bytes starting from the Simple I/O base address. Ooooor you could just toggle the polarity of GP10: `irq 0xb0 = 0x01` in the devicetree (if that works, you can try to do it earlier in C code). Also, if you want to make the power LED blink, you probably need to set bit 0 in GPIO LDN, offset 0xf9. https://i.imgur.com/UIWO3G6.png is a table from the IT8625E datasheet (which is close enough to your chip, at least in this regard), you could even change the blinking rate if you wish. This should work no matter if the polarity is inverted or not. File src/mainboard/lenovo/thinkcentre_m710s/devicetree.cb: https://review.coreboot.org/c/coreboot/+/80343/comment/de24d845_2cc5b738 : PS27, Line 12: register "PcieRpClkReqSupport[0]" = "true" : register "PcieRpClkReqNumber[0]" = "2" : register "PcieRpClkSrcNumber[0]" = "0" I think the FSP UPDs exist, but they may not be exposed as devicetree settings. -- To view, visit https://review.coreboot.org/c/coreboot/+/80343?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I551753aecfbd2c0ee57d85bb22cb943eb21af3cc Gerrit-Change-Number: 80343 Gerrit-PatchSet: 27 Gerrit-Owner: Nicholas Sudsgaard <devel+coreboot(a)nsudsgaard.com> Gerrit-Reviewer: Alexander Couzens <lynxis(a)fe80.eu> Gerrit-Reviewer: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-Reviewer: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: 9elements QA <hardwaretestrobot(a)gmail.com> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nicholas Sudsgaard <devel+coreboot(a)nsudsgaard.com> Gerrit-Comment-Date: Wed, 01 May 2024 16:07:16 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in coreboot[main]: Documentation/mainboard/lenovo: Add ThinkCentre M710s
by Angel Pons (Code Review) 01 May '24

01 May '24

Attention is currently required from: Nicholas Sudsgaard. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80411?usp=email ) Change subject: Documentation/mainboard/lenovo: Add ThinkCentre M710s ...................................................................... Patch Set 17: (19 comments) File Documentation/mainboard/lenovo/thinkcentre_m710s.md: https://review.coreboot.org/c/coreboot/+/80411/comment/a86c00b7_b89de405 : PS17, Line 5: that even dummies could follow through I would prefer to avoid such expressions. If someone struggles to follow the instructions, would they be worse than a dummy? Ouch, that's a bit insulting. Instead, please consider something along the lines of: > This guide is targeted at beginners with little to no prior coreboot experience I feel that "beginner" (a person who is starting to do something or learn something for the first time) is both accurate and respectful. https://review.coreboot.org/c/coreboot/+/80411/comment/6fe01ba1_1095f0f6 : PS17, Line 12: ```eval_rst : Start by following the :doc:`tutorial <../../tutorial/part1>` until step 3. : ``` Wouldn't it be nice to have a tutorial for "installing" coreboot onto any board, instead of having to write similar procedures for each and every mainboard? I would rather have a general guide that references mainboard-specific documentation where necessary. I'm not sure if docs support it, but it would be really cool if the installation guide's page had a mainboard selector drop-down, and choosing the mainboard in there automatically merged the mainboard-specific details (preparation steps, getting required blobs, where is the flash chip, how to flash it, where is the console) into the installation guide. https://review.coreboot.org/c/coreboot/+/80411/comment/cbedec3a_684f3f4a : PS17, Line 22: # Take 2 images of your original flash image : flashrom -p internal -r vendor.rom : flashrom -p internal -r vendor.rom.backup That won't work properly because the ME region is not readable by the host. https://review.coreboot.org/c/coreboot/+/80411/comment/9d833fba_75c63907 : PS17, Line 26: # Compare the images to see if there is any differences (should be none) : sha1sum vendor.rom vendor.rom.backup This is unnecessary. Just tell flashrom to verify the file against the flash chip contents: ``` flashrom -p internal -r vendor.rom flashrom -p internal -v vendor.rom ``` https://review.coreboot.org/c/coreboot/+/80411/comment/94bfac82_058acd2a : PS17, Line 16: ### Step 2 - Preserving the original flash image (IMPORTANT) : : Before proceeding, it is **VERY IMPORTANT** to keep the original (vendor) flash image. : Without this, it would be very difficult to recover from mistakes during : flashing (or if you decide coreboot is not for you). : : # Take 2 images of your original flash image : flashrom -p internal -r vendor.rom : flashrom -p internal -r vendor.rom.backup : : # Compare the images to see if there is any differences (should be none) : sha1sum vendor.rom vendor.rom.backup : : # Make the images immutable to prevent accidental modification/deletion : chattr +i vendor.rom vendor.rom.backup : # Check whether the 'i' flag is set : lsattr vendor.rom vendor.rom.backup : : # To be absolutely sure you can attempt to modifiy/delete the images : # You should get 'Operation not permitted' : echo "test" > vendor.rom : rm -f vendor.rom : : If you ever need to undo the installation, simply flash this image. : As long as you do this, flashing coreboot is not as scary as it sounds. :) This is not mainboard specific. It doesn't belong in here. https://review.coreboot.org/c/coreboot/+/80411/comment/7a07a3a2_14d0fae5 : PS17, Line 44: Let's also use a layout file to be sure that we only modify the BIOS region : of your flash chip. This makes things simpler and reduces and chance of messing : up your flash chip. Adding the `--ifd -i bios` arguments to flashrom achieves the same effect and is a lot less error-prone. https://review.coreboot.org/c/coreboot/+/80411/comment/143bf189_6cc9d00c : PS17, Line 53: 00000000:00000fff fd : 00200000:007fffff bios : 00003000:001fffff me : 00001000:00002fff gbe Please keep this information in this docs page. https://review.coreboot.org/c/coreboot/+/80411/comment/5c03bb92_68af350d : PS17, Line 64: coreboot provides an easy way of configuration by using a TUI menu. To open this : run the following command: : : make menuconfig : : While it should be quite intuitive, the navigation is written at the top if you : get stuck. : : Every mainboard is different and you need to instruct coreboot to build for : Lenovo ThinkCentre M710s specifically. : : Mainboard ---> : Mainboard vendor (Lenovo) ---> : Mainboard model (ThinkCentre M710s) ---> It would be much better to have generic "how to build a coreboot image" instructions instead of having each and every board repeat itself. Please remove. https://review.coreboot.org/c/coreboot/+/80411/comment/eff1cc58_44636add : PS17, Line 81: Now you can choose the payload that you are going to use: : : * SeaBIOS : * Tianocore's EDK 2 (choose this if unsure) : : It is highly recommend not choosing anything else, as they have not been tested : and are most likely not going to work. I wouldn't tell people to use any payloads, just document what has been tested on this board and is known to work, and what hasn't been tested. You already state that SeaBIOS and edk2 are known to work later on, so the only thing to add would be to mention that other payloads haven't been tested. The "are most likely not going to work" part is unjustified, and I would simply omit it. I'm pretty sure that GRUB as a payload would work fine, it's just not tested. TL;DR just state what works and what is untested. If people make bad decisions (e.g. end up with a non-booting machine) because they don't know what "untested" means, I'd say it's not our fault. https://review.coreboot.org/c/coreboot/+/80411/comment/f6b6dc0e_02486b80 : PS17, Line 98: Tianocore's EDK II payload (Official edk2 repository) ---> But why wasn't the temptation (what you wrote below) resisted here? MrChromebox's edk2 is the default because it's specifically tested with coreboot, whereas upstream edk2 is not. https://review.coreboot.org/c/coreboot/+/80411/comment/c0cfa8c0_b6703539 : PS17, Line 79: ### Step 2 - Choosing the payload : : Now you can choose the payload that you are going to use: : : * SeaBIOS : * Tianocore's EDK 2 (choose this if unsure) : : It is highly recommend not choosing anything else, as they have not been tested : and are most likely not going to work. : : #### SeaBIOS : : Payload ---> : Payload to add (SeaBIOS) ---> : : #### EDK 2 : : Payload ---> : Payload to add (edk2 payload) ---> : Tianocore's EDK II payload (Official edk2 repository) ---> : : While understandable, it is recommended to resist the temptation of doing any : additional configuration if this is your first time building coreboot. Not mainboard specific. https://review.coreboot.org/c/coreboot/+/80411/comment/08239d15_010c6bb4 : PS17, Line 103: ### Step 3 - Building coreboot : : Finally, save your changes and build coreboot (this should only take a couple of minutes). : : make -j$(nproc) Not mainboard specific. https://review.coreboot.org/c/coreboot/+/80411/comment/ac5e204e_672d30a6 : PS17, Line 133: **Make sure you attach the clip using the correct pin configurations** (page 6 of : the datasheet or the image above), as it may **damage your chip** if done incorrectly. : How to do this depends on your programmer of choice and refer to other : documentation which can be found on the internet. Not mainboard specific. https://review.coreboot.org/c/coreboot/+/80411/comment/7b8797b3_b1db4a82 : PS17, Line 138: While [not recommended](https://libreboot.org/docs/install/spi.html#do-not-use-ch341a) : the CH431A programmer seems to be very popular. However, it can be slightly : confusing at first. Here is the pinout and which half to use ([datasheet](https://www.alldatasheet.com/datasheet-pdf/pdf/1132609/ETC2/CH34…). : : ![](ch341a_pinout.jpg) Not mainboard specific. Also, if not recommended, why is this the only explanation provided? https://review.coreboot.org/c/coreboot/+/80411/comment/e8c8a803_4326c10c : PS17, Line 148: 'W25Q64BV/W25Q64CV/W25Q64FV' Earlier you stated that the chip is a `W25Q64JV`. Why use the wrong chip definition? https://review.coreboot.org/c/coreboot/+/80411/comment/a0b79d7a_6b964823 : PS17, Line 160: In the unfortunate case, where your machine does not seem to boot, first check : for any mistakes. What constitutes a mistake? https://review.coreboot.org/c/coreboot/+/80411/comment/0086af44_ca3f6b8b : PS17, Line 161: If you are confident that you did not make any mistakes : reach out to the :doc:`community <../../community/forums>` for some help. And if you made mistakes, what? I really don't like how this section reads. https://review.coreboot.org/c/coreboot/+/80411/comment/b019c678_b1563192 : PS17, Line 165: ## Updating : : Luckily, once you have coreboot, updating can be done internally using the following command: : : flashrom -p internal --fmap -i COREBOOT -w build/coreboot.rom Any reason to update coreboot using the fmap here? https://review.coreboot.org/c/coreboot/+/80411/comment/2ee3680b_12ca5cde : PS17, Line 200: * DRM issue when using EDK 2 and libgfxinit What does "DRM issue" mean? -- To view, visit https://review.coreboot.org/c/coreboot/+/80411?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I90311257a28bd463712c4d43f8b83baa745509cc Gerrit-Change-Number: 80411 Gerrit-PatchSet: 17 Gerrit-Owner: Nicholas Sudsgaard <devel+coreboot(a)nsudsgaard.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nicholas Sudsgaard <devel+coreboot(a)nsudsgaard.com> Gerrit-Comment-Date: Wed, 01 May 2024 15:33:34 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[L] Change in coreboot[main]: mb/dell: Add Latitude E6430 (Ivy Bridge)
by Nicholas Chin (Code Review) 01 May '24

01 May '24

Attention is currently required from: Martin L Roth, Paul Menzel. Nicholas Chin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/77444?usp=email ) Change subject: mb/dell: Add Latitude E6430 (Ivy Bridge) ...................................................................... Patch Set 10: (2 comments) File src/mainboard/dell/snb_ivb_latitude/gma-mainboard.ads: PS10: Double check if this is the same for all the Latitudes in the patch train https://review.coreboot.org/c/coreboot/+/77444/comment/6cfc753b_6e2d5434 : PS10, Line 14: DP2, -- dock DP : DP3, -- dock DP Add HDMI for dock DVI ports -- To view, visit https://review.coreboot.org/c/coreboot/+/77444?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I93c6622fc5da1d0d61a5b2c197ac7227d9525908 Gerrit-Change-Number: 77444 Gerrit-PatchSet: 10 Gerrit-Owner: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-CC: Iru Cai <mytbk920423(a)gmail.com> Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Comment-Date: Wed, 01 May 2024 15:13:03 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in coreboot[main]: util/crossgcc: Update LLVM from 17.0.6 to 18.1.4
by Elyes Haouas (Code Review) 01 May '24

01 May '24

Attention is currently required from: Arthur Heymans, Felix Singer, Martin L Roth, Zebreus. Elyes Haouas has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80314?usp=email ) Change subject: util/crossgcc: Update LLVM from 17.0.6 to 18.1.4 ...................................................................... Patch Set 16: Code-Review+1 (1 comment) Patchset: PS13: > We want to make sure that the updates are free of errors. So better earlier than later. yep, sure! this what I also try to do here #81419. note that w'll have llvm version 18.1.5 in soon -- To view, visit https://review.coreboot.org/c/coreboot/+/80314?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I03a44e0c23a925396f614f282882405dc886ba58 Gerrit-Change-Number: 80314 Gerrit-PatchSet: 16 Gerrit-Owner: Zebreus <lennarteichhorn(a)googlemail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-Reviewer: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Zebreus <lennarteichhorn(a)googlemail.com> Gerrit-Comment-Date: Wed, 01 May 2024 14:49:45 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Comment-In-Reply-To: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-MessageType: comment

1 0

[L] Change in coreboot[main]: mb/dell: Add Latitude E6430 (Ivy Bridge)
by Felix Singer (Code Review) 01 May '24

01 May '24

Attention is currently required from: Martin L Roth, Nicholas Chin, Paul Menzel. Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/77444?usp=email ) Change subject: mb/dell: Add Latitude E6430 (Ivy Bridge) ...................................................................... Patch Set 10: (4 comments) File src/mainboard/dell/snb_ivb_latitude/acpi_tables.c: https://review.coreboot.org/c/coreboot/+/77444/comment/50f89e28_3b049744 : PS10, Line 6: /* FIXME: check this function. */ can this be removed? File src/mainboard/dell/snb_ivb_latitude/devicetree.cb: PS10: Missing license header File src/mainboard/dell/snb_ivb_latitude/dsdt.asl: https://review.coreboot.org/c/coreboot/+/77444/comment/a2d80758_80033509 : PS10, Line 3: /* SPDX-License-Identifier: GPL-2.0-only */ Move license header before the others File src/mainboard/dell/snb_ivb_latitude/variants/e6430/overridetree.cb: PS10: Missing license header -- To view, visit https://review.coreboot.org/c/coreboot/+/77444?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I93c6622fc5da1d0d61a5b2c197ac7227d9525908 Gerrit-Change-Number: 77444 Gerrit-PatchSet: 10 Gerrit-Owner: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-CC: Iru Cai <mytbk920423(a)gmail.com> Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-Comment-Date: Wed, 01 May 2024 12:37:09 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[L] Change in coreboot[main]: mb/asrock/z97_extreme6: Add new mainboard
by Angel Pons (Code Review) 01 May '24

01 May '24

Attention is currently required from: Felix Singer, Martin L Roth, Nico Huber. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68188?usp=email ) Change subject: mb/asrock/z97_extreme6: Add new mainboard ...................................................................... Patch Set 8: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/68188/comment/3822b47f_85c1849b : PS8, Line 51: - Flashing with flashrom > The `board_info.txt` says supported. Do you mean something in particular like […] It's probably out of sync. When I wrote this, I meant that I never tried flashing internally, but I was sure it would work. I primarily test this board using an EM100 so I don't really try flashing internally. That being said, I just reflashed the GbE region to one of the chips and it went well, so I'd say it is working. Will move. -- To view, visit https://review.coreboot.org/c/coreboot/+/68188?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: If1d22547725e59f435de36b973e1bf4f334269a9 Gerrit-Change-Number: 68188 Gerrit-PatchSet: 8 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-CC: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-CC: Martin L Roth <gaumless(a)gmail.com> Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com> Gerrit-Comment-Date: Wed, 01 May 2024 11:33:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Gerrit-MessageType: comment

1 0

[L] Change in coreboot[main]: mb/asrock/z97_extreme6: Add new mainboard
by Nico Huber (Code Review) 01 May '24

01 May '24

Attention is currently required from: Angel Pons, Felix Singer, Martin L Roth. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68188?usp=email ) Change subject: mb/asrock/z97_extreme6: Add new mainboard ...................................................................... Patch Set 8: (10 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/68188/comment/401003ba_af64eadc : PS4, Line 59: - Acer B247Y board driving a FHD panel of a Samsung S24E650 monitor, : connected to the board's HDMI output says "Unsupported resolution" : after libgfxinit configured the iGPU outputs in linear framebuffer : mode. HDMI output works fine after Linux's i915 driver takes over. : Not sure if it's specific to the monitor: the HDMI cable is beaten : up, and it is hard to replace (need to relocate the logic board so : that the ports are accessible). > Worth checking if this problem occurs on other libgfxinit boards/platforms. […] I've also some special HDMI video grabber collecting dust that works with Linux HDMI but not libgfxinit HDMI. Funny thing, right now for the very first time I had this thought: It might be because we run HDMI in legacy DVI mode w/o audio channel (not sure, but this probably also implies a different transport w/o packets). Commit Message: https://review.coreboot.org/c/coreboot/+/68188/comment/e99df806_0aa7ad2a : PS8, Line 51: - Flashing with flashrom The `board_info.txt` says supported. Do you mean something in particular like flashing from the original firmware, or is this just out of sync? File src/mainboard/asrock/z97_extreme6/bootblock.c: https://review.coreboot.org/c/coreboot/+/68188/comment/37873ee8_f072c0e7 : PS8, Line 11: PP_OD_DEV Maybe not the best choice of names as there is also a parallel port (PP) device. Can't come up with something better and short, though, hence maybe the following: The other blocks below start with a comment, maybe the PP_OD_DEV one should too, e.g. `/* Select push-pull vs. open-drain output */` https://review.coreboot.org/c/coreboot/+/68188/comment/4a89075c_ac41bfaa : PS8, Line 18: pnp_write_config(GLOBAL_DEV, 0x1b, 0xf0); Bit 4 sets pin 95 to CIRRX? But CIR is disabled in the DT. Boardview says DEVSLP for SATAE_1. https://review.coreboot.org/c/coreboot/+/68188/comment/8968aec5_94142b1f : PS8, Line 18: pnp_write_config(GLOBAL_DEV, 0x1b, 0xf0); Meh, bit 5 is reserved in my ds. https://review.coreboot.org/c/coreboot/+/68188/comment/e14fc613_2baa623e : PS8, Line 18: pnp_write_config(GLOBAL_DEV, 0x1b, 0xf0); Bits 2..1 suggest pin 51 is MSDA, but WLAN1_ON/OFF in boardview. https://review.coreboot.org/c/coreboot/+/68188/comment/5f682d05_27617f26 : PS8, Line 19: pnp_write_config(GLOBAL_DEV, 0x1c, 0x10); Note to self, 0x1a (default 0x30) and 0x1c look reasonable. https://review.coreboot.org/c/coreboot/+/68188/comment/28497006_ad177341 : PS8, Line 33: pnp_write_config(ACPI_DEV, 0xe4, 0x70); This also seems to set "User defined mode for power loss last-state. (The last- state flag is located on “CRE6h, bit4.”)" is that intentional? File src/mainboard/asrock/z97_extreme6/devicetree.cb: https://review.coreboot.org/c/coreboot/+/68188/comment/d7dcf827_e07fd280 : PS8, Line 20: Bifurcable If the verb is 'bifurcate', this would be 'bifurcatable' right? https://review.coreboot.org/c/coreboot/+/68188/comment/d904bc51_7910926e : PS8, Line 27: c Not that it matters, but I guess 8 bytes would suffice. -- To view, visit https://review.coreboot.org/c/coreboot/+/68188?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: If1d22547725e59f435de36b973e1bf4f334269a9 Gerrit-Change-Number: 68188 Gerrit-PatchSet: 8 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-CC: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-CC: Martin L Roth <gaumless(a)gmail.com> Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Wed, 01 May 2024 10:44:39 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

[M] Change in coreboot[main]: spd.h: Move enum ddr4_module_type to ddr4.h
by Elyes Haouas (Code Review) 01 May '24

01 May '24

Attention is currently required from: Angel Pons, Elyes Haouas, Jakub Czapiga. Hello Angel Pons, Jakub Czapiga, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/82124?usp=email to look at the new patch set (#3). Change subject: spd.h: Move enum ddr4_module_type to ddr4.h ...................................................................... spd.h: Move enum ddr4_module_type to ddr4.h Move specific enum ddr4_module_type to <device/dram/ddr4.h>. Change-Id: I33afccb768eaee94cc1cd947eb0ddbe15c2831bd Signed-off-by: Elyes Haouas <ehaouas(a)noos.fr> --- M src/device/dram/ddr3.c M src/device/dram/ddr4.c M src/device/dram/spd.c M src/include/device/dram/ddr3.h M src/include/device/dram/ddr4.h M src/include/spd.h M src/northbridge/intel/haswell/native_raminit/raminit_native.h M src/northbridge/intel/sandybridge/raminit.c M src/soc/intel/baytrail/romstage/raminit.c M tests/lib/dimm_info_util-test.c 10 files changed, 31 insertions(+), 43 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/24/82124/3 -- To view, visit https://review.coreboot.org/c/coreboot/+/82124?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I33afccb768eaee94cc1cd947eb0ddbe15c2831bd Gerrit-Change-Number: 82124 Gerrit-PatchSet: 3 Gerrit-Owner: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Jakub Czapiga <czapiga(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Jakub Czapiga <czapiga(a)google.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-MessageType: newpatchset

1 0

[M] Change in coreboot[main]: spd.h: Move enum ddr4_module_type to ddr4.h
by Elyes Haouas (Code Review) 01 May '24

01 May '24

Attention is currently required from: Angel Pons, Elyes Haouas, Jakub Czapiga. Hello Angel Pons, Jakub Czapiga, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/82124?usp=email to look at the new patch set (#2). The following approvals got outdated and were removed: Verified-1 by build bot (Jenkins) Change subject: spd.h: Move enum ddr4_module_type to ddr4.h ...................................................................... spd.h: Move enum ddr4_module_type to ddr4.h Move specific enum ddr4_module_type to <device/dram/ddr4.h>. Change-Id: I33afccb768eaee94cc1cd947eb0ddbe15c2831bd Signed-off-by: Elyes Haouas <ehaouas(a)noos.fr> --- M src/device/dram/ddr3.c M src/device/dram/ddr4.c M src/device/dram/spd.c M src/include/device/dram/ddr3.h M src/include/device/dram/ddr4.h M src/include/spd.h M src/northbridge/intel/haswell/native_raminit/raminit_native.h M src/northbridge/intel/sandybridge/raminit.c M src/soc/intel/baytrail/romstage/raminit.c M tests/lib/dimm_info_util-test.c 10 files changed, 31 insertions(+), 43 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/24/82124/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/82124?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I33afccb768eaee94cc1cd947eb0ddbe15c2831bd Gerrit-Change-Number: 82124 Gerrit-PatchSet: 2 Gerrit-Owner: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Jakub Czapiga <czapiga(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Jakub Czapiga <czapiga(a)google.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-MessageType: newpatchset

1 0

[S] Change in coreboot[main]: mb/google/nissa: Create a riven variant
by Kapil Porwal (Code Review) 01 May '24

01 May '24

Attention is currently required from: David Wu, Dinesh Gehlot, Eric Lai, Morris Hsu, Nick Vaccaro, Ren Kuo, Subrata Banik, Tyler Wang. Kapil Porwal has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/82132?usp=email ) Change subject: mb/google/nissa: Create a riven variant ...................................................................... Patch Set 2: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/82132?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I1be2346d87c891cc0e5fbda094e1f6e0dd60df1b Gerrit-Change-Number: 82132 Gerrit-PatchSet: 2 Gerrit-Owner: David Wu <david_wu(a)quanta.corp-partner.google.com> Gerrit-Reviewer: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Reviewer: Eric Lai <ericllai(a)google.com> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Morris Hsu <morris-hsu(a)quanta.corp-partner.google.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org> Gerrit-Reviewer: Ren Kuo <ren.kuo(a)quanta.corp-partner.google.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Tyler Wang <tyler.wang(a)quanta.corp-partner.google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Eric Lai <ericllai(a)google.com> Gerrit-Attention: David Wu <david_wu(a)quanta.corp-partner.google.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Ren Kuo <ren.kuo(a)quanta.corp-partner.google.com> Gerrit-Attention: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Attention: Nick Vaccaro <nvaccaro(a)chromium.org> Gerrit-Attention: Tyler Wang <tyler.wang(a)quanta.corp-partner.google.com> Gerrit-Attention: Morris Hsu <morris-hsu(a)quanta.corp-partner.google.com> Gerrit-Comment-Date: Wed, 01 May 2024 07:54:45 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit May 2024