coreboot-gerrit April 2022

coreboot-gerrit@coreboot.org

1 participants
5879 discussions

Change in coreboot[master]: sc7280: Add PCIe host controller driver
by Ravi Kumar Bokka (Code Review) 07 Apr '22

07 Apr '22

Attention is currently required from: Ravi Kumar Bokka, Furquan Shaikh, Julius Werner, Prasad Malisetty, mturney mturney. Hello Shelley Chen, build bot (Jenkins), Paul Menzel, Julius Werner, mturney mturney, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/53902 to look at the new patch set (#82). Change subject: sc7280: Add PCIe host controller driver ...................................................................... sc7280: Add PCIe host controller driver Add support for PCIe host driver to configure in RC (root complex) mode. Host driver will power on the PHY, enable the PCIe link and start the bus enumeration. BUG=b:182963902,b:216686574,b:181098581 TEST=Validated on qualcomm sc7280 development board Change-Id: Iccf60aa56541f5230fa9c3f821d7709615c36631 Signed-off-by: Prasad Malisetty <quic_pmaliset(a)quicinc.com> --- M src/mainboard/google/herobrine/mainboard.c M src/soc/qualcomm/sc7280/Makefile.inc M src/soc/qualcomm/sc7280/include/soc/addressmap.h A src/soc/qualcomm/sc7280/include/soc/pcie.h A src/soc/qualcomm/sc7280/include/soc/qcom_qmp_phy.h A src/soc/qualcomm/sc7280/pcie_host.c 6 files changed, 1,594 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/53902/82 -- To view, visit https://review.coreboot.org/c/coreboot/+/53902 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iccf60aa56541f5230fa9c3f821d7709615c36631 Gerrit-Change-Number: 53902 Gerrit-PatchSet: 82 Gerrit-Owner: Ravi Kumar Bokka <rbokka(a)codeaurora.org> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: Shelley Chen <shchen(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Reviewer: mturney mturney <mturney(a)codeaurora.org> Gerrit-CC: Furquan Shaikh <furquan.m.shaikh(a)gmail.com> Gerrit-CC: Prasad Malisetty <pmaliset(a)qualcomm.corp-partner.google.com> Gerrit-CC: Ravi Kumar Bokka <c_rbokka(a)qualcomm.corp-partner.google.com> Gerrit-Attention: Ravi Kumar Bokka <rbokka(a)codeaurora.org> Gerrit-Attention: Furquan Shaikh <furquan.m.shaikh(a)gmail.com> Gerrit-Attention: Ravi Kumar Bokka <c_rbokka(a)qualcomm.corp-partner.google.com> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Prasad Malisetty <pmaliset(a)qualcomm.corp-partner.google.com> Gerrit-Attention: mturney mturney <mturney(a)codeaurora.org> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: soc/qualcomm/common: Update helper function to know size of memchipinfo
by Ravi Kumar Bokka (Code Review) 07 Apr '22

07 Apr '22

Attention is currently required from: Paul Menzel, Julius Werner, Yu-Ping Wu. Hello Shelley Chen, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/63026 to look at the new patch set (#7). Change subject: soc/qualcomm/common: Update helper function to know size of memchipinfo ...................................................................... soc/qualcomm/common: Update helper function to know size of memchipinfo Added mem_chip_info_size() function with CB:63407 as size of mem_chip_info strcuture is using in multiple places. This fix bugs introduced with CB:59195 and commit sha: 4573ca42e675fb8b61bcf05591d8d4d05f93944d BUG=b:182963902,b:177917361 TEST=Validated on qualcomm sc7280 development board Signed-off-by: Ravi Kumar Bokka <rbokka(a)codeaurora.org> Change-Id: I0d59669adaf287d0eb7b58ccb0fe3f98e3d23281 --- M src/soc/qualcomm/common/qclib.c 1 file changed, 17 insertions(+), 9 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/63026/7 -- To view, visit https://review.coreboot.org/c/coreboot/+/63026 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0d59669adaf287d0eb7b58ccb0fe3f98e3d23281 Gerrit-Change-Number: 63026 Gerrit-PatchSet: 7 Gerrit-Owner: Ravi Kumar Bokka <rbokka(a)codeaurora.org> Gerrit-Reviewer: Shelley Chen <shchen(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Julius Werner <jwerner(a)chromium.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: soc/qualcomm/common: Fix bugs introduced with CB:59195
by Ravi Kumar Bokka (Code Review) 07 Apr '22

07 Apr '22

Attention is currently required from: Paul Menzel, Julius Werner, Yu-Ping Wu. Ravi Kumar Bokka has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63026 ) Change subject: soc/qualcomm/common: Fix bugs introduced with CB:59195 ...................................................................... Patch Set 6: (3 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/63026/comment/b73d29fe_76225429 PS5, Line 7: soc/qualcomm/common: update size of memchipinfo structure > That is not what this patch does. […] hi julius updated commit message with details Commit Message: https://review.coreboot.org/c/coreboot/+/63026/comment/953b8682_111e50ba PS6, Line 7: soc/qualcomm/common: Fix bugs introduced with CB:59195 > Please improve the summary, describing the fix for the bug. Ack https://review.coreboot.org/c/coreboot/+/63026/comment/ef06c197_401b006a PS6, Line 8: > Please summarize the problem, and the fix. […] hi paul, i have updated detail commit description. -- To view, visit https://review.coreboot.org/c/coreboot/+/63026 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I0d59669adaf287d0eb7b58ccb0fe3f98e3d23281 Gerrit-Change-Number: 63026 Gerrit-PatchSet: 6 Gerrit-Owner: Ravi Kumar Bokka <rbokka(a)codeaurora.org> Gerrit-Reviewer: Shelley Chen <shchen(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Julius Werner <jwerner(a)chromium.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Comment-Date: Thu, 07 Apr 2022 06:00:10 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org> Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: libpayload/pci: Add pci_map_bus interface for MediaTek platform
by Paul Menzel (Code Review) 07 Apr '22

07 Apr '22

Attention is currently required from: Hung-Te Lin, Angel Pons, Jianjun Wang. Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56794 ) Change subject: libpayload/pci: Add pci_map_bus interface for MediaTek platform ...................................................................... Patch Set 56: (2 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/56794/comment/56e10d5e_2b1fc1ef PS56, Line 9: Add 'pci_map_bus' interface for MediaTek platform. Also mention the Kconfig option? File payloads/libpayload/drivers/pcie_mediatek.c: https://review.coreboot.org/c/coreboot/+/56794/comment/db9c9224_71dce8c0 PS56, Line 20: val); Fits on line above? -- To view, visit https://review.coreboot.org/c/coreboot/+/56794 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I9ea7d111fed6b816fa2352fe93c268116519a577 Gerrit-Change-Number: 56794 Gerrit-PatchSet: 56 Gerrit-Owner: Jianjun Wang <jianjun.wang(a)mediatek.com> Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com> 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: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Rex-BC Chen <rex-bc.chen(a)mediatek.com> Gerrit-Attention: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Jianjun Wang <jianjun.wang(a)mediatek.com> Gerrit-Comment-Date: Thu, 07 Apr 2022 05:59:48 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: amdfwtool: remove duplicates
by Paul Menzel (Code Review) 07 Apr '22

07 Apr '22

Attention is currently required from: Bao Zheng, Kangheui Won, Angel Pons, Felix Held. Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59581 ) Change subject: amdfwtool: remove duplicates ...................................................................... Patch Set 1: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/59581/comment/d693a0de_e631cd5b PS1, Line 7: amdfwtool: remove duplicates What kind of duplicates? -- To view, visit https://review.coreboot.org/c/coreboot/+/59581 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3b86e24ba1c2769985e2864528e4dfcbf3cccc4b Gerrit-Change-Number: 59581 Gerrit-PatchSet: 1 Gerrit-Owner: Kangheui Won <khwon(a)chromium.org> Gerrit-Reviewer: Bao Zheng <fishbaozi(a)gmail.com> Gerrit-Reviewer: Bao Zheng <zheng.bao(a)amd.corp-partner.google.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> 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-CC: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Bao Zheng <fishbaozi(a)gmail.com> Gerrit-Attention: Bao Zheng <zheng.bao(a)amd.corp-partner.google.com> Gerrit-Attention: Kangheui Won <khwon(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Thu, 07 Apr 2022 05:57:51 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mb/google/dedede/var/beadrix: Update PCIe and SATA pins for low power...
by Paul Menzel (Code Review) 07 Apr '22

07 Apr '22

Attention is currently required from: Henry Sun, Teddy Shih, Aseda Aboagye, Simon Yang. Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/62776 ) Change subject: mb/google/dedede/var/beadrix: Update PCIe and SATA pins for low power consumption ...................................................................... Patch Set 4: Code-Review+1 (1 comment) Patchset: PS4: Great work. -- To view, visit https://review.coreboot.org/c/coreboot/+/62776 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I79ec524c5ce8f2a79da4aeba084786fb9dac17af Gerrit-Change-Number: 62776 Gerrit-PatchSet: 4 Gerrit-Owner: Teddy Shih <teddyshih(a)ami.corp-partner.google.com> Gerrit-Reviewer: Aseda Aboagye <aaboagye(a)google.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Henry Sun <henrysun(a)google.com> Gerrit-Reviewer: Ivan Chen <yulunchen(a)google.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: Simon Yang <simon1.yang(a)intel.corp-partner.google.com> Gerrit-Reviewer: Super Ni <super.ni(a)intel.corp-partner.google.com> Gerrit-Reviewer: Teddy Shih <teddyshih(a)ami.corp-partner.google.com> Gerrit-Reviewer: Teddy Shih <teddyshihau(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Jack Cheng <jack.cheng(a)ecs.corp-partner.google.com> Gerrit-CC: Raymond Chung <raymondchung(a)ami.corp-partner.google.com> Gerrit-Attention: Henry Sun <henrysun(a)google.com> Gerrit-Attention: Teddy Shih <teddyshihau(a)gmail.com> Gerrit-Attention: Aseda Aboagye <aaboagye(a)google.com> Gerrit-Attention: Simon Yang <simon1.yang(a)intel.corp-partner.google.com> Gerrit-Attention: Teddy Shih <teddyshih(a)ami.corp-partner.google.com> Gerrit-Comment-Date: Thu, 07 Apr 2022 05:56:59 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mb/qemu-armv7: Initialize cbmem
by build bot (Jenkins) (Code Review) 07 Apr '22

07 Apr '22

Attention is currently required from: Arthur Heymans. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63421 ) Change subject: mb/qemu-armv7: Initialize cbmem ...................................................................... Patch Set 2: Verified-1 -- To view, visit https://review.coreboot.org/c/coreboot/+/63421 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I607205a0d44c71eb26031ced7a8af303efacd6f2 Gerrit-Change-Number: 63421 Gerrit-PatchSet: 2 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Comment-Date: Thu, 07 Apr 2022 05:56:11 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: sc7280: Add PCIe host controller driver
by Paul Menzel (Code Review) 07 Apr '22

07 Apr '22

Attention is currently required from: Ravi Kumar Bokka, Furquan Shaikh, Julius Werner, Prasad Malisetty, mturney mturney. Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/53902 ) Change subject: sc7280: Add PCIe host controller driver ...................................................................... Patch Set 81: Code-Review-1 (68 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/53902/comment/ca5d15a5_40df4a6e PS81, Line 7: sc7280 soc/qualcomm/sc7280 https://review.coreboot.org/c/coreboot/+/53902/comment/20f416dc_faa8e4bc PS81, Line 9: Add support for PCIe host driver to configure : in RC (root complex) mode. Host driver will power on : the PHY, enable the PCIe link and start the bus enumeration. Please reflow for 72 characters per line. https://review.coreboot.org/c/coreboot/+/53902/comment/887c6ae0_270edede PS81, Line 12: Please mention the source for your implementation (datasheet name, revision, section), describe the implementation, and discuss why it’s implemented that way and not more of coreboot’s “PCIe framework” is used. Currently, it looks like a lot of duplication, and maintenance is going to be hard. Also, is there already another Qualcomm PCIe implementation in U-Boot or the Linux kernel for example? https://review.coreboot.org/c/coreboot/+/53902/comment/a640ee65_eec69613 PS81, Line 14: TEST=Validated on qualcomm sc7280 development board With what PCIe card? File src/mainboard/google/herobrine/mainboard.c: https://review.coreboot.org/c/coreboot/+/53902/comment/c5564a89_7007267a PS81, Line 37: /* Assert NVMe External LDO's GPIO */ Why is here something NVMe specific? https://review.coreboot.org/c/coreboot/+/53902/comment/f86ab05c_3fb8258b PS81, Line 43: pcie1 What does the index 1 mean? https://review.coreboot.org/c/coreboot/+/53902/comment/a041697e_932b59f8 PS81, Line 43: /*Initialize Missing space. https://review.coreboot.org/c/coreboot/+/53902/comment/27a4ae07_c1973efc PS81, Line 124: RC root complex https://review.coreboot.org/c/coreboot/+/53902/comment/00308ee1_a06ffd5b PS81, Line 125: setup_pcie Why not: setup_pcie_in_rootcomplex_mode() or something similar? Then you also can get rid of the comment. File src/soc/qualcomm/sc7280/include/soc/addressmap.h: https://review.coreboot.org/c/coreboot/+/53902/comment/804a83a3_54b226fd PS81, Line 91: definitions */ One space too much. https://review.coreboot.org/c/coreboot/+/53902/comment/cc993651_4441b855 PS81, Line 100: /*PHY Missing space. File src/soc/qualcomm/sc7280/include/soc/pcie.h: https://review.coreboot.org/c/coreboot/+/53902/comment/8e6f31d7_55a304b1 PS81, Line 2: #include <types.h> Please add a blank line above. https://review.coreboot.org/c/coreboot/+/53902/comment/83b26c2e_ba10cd59 PS81, Line 17: Tab for alignment? https://review.coreboot.org/c/coreboot/+/53902/comment/5abddc40_bfc02cb3 PS81, Line 70: *< Remove? https://review.coreboot.org/c/coreboot/+/53902/comment/83709bea_e8fc51b1 PS81, Line 115: From 4.80 core version What “core version”? https://review.coreboot.org/c/coreboot/+/53902/comment/027c8499_e9a72621 PS81, Line 152: */ Remove one space. https://review.coreboot.org/c/coreboot/+/53902/comment/39456142_5713a341 PS81, Line 173: Start on the bus Sounds strange. https://review.coreboot.org/c/coreboot/+/53902/comment/88444b2a_a18697c7 PS81, Line 196: )<<8 Please add spaces around the operator. https://review.coreboot.org/c/coreboot/+/53902/comment/0052dead_7dacd196 PS81, Line 215: } Should the {} align? https://review.coreboot.org/c/coreboot/+/53902/comment/21a66ff4_2a6eb524 PS81, Line 217: /* PCIe setup */ The comment is redundant. Either remove or elaborate. File src/soc/qualcomm/sc7280/pcie_host.c: https://review.coreboot.org/c/coreboot/+/53902/comment/7a89f649_a93483be PS81, Line 32: *@lanes Missing space. https://review.coreboot.org/c/coreboot/+/53902/comment/c3c70274_95cae586 PS81, Line 73: int unsigned int (also below) https://review.coreboot.org/c/coreboot/+/53902/comment/793770ac_f4b1b159 PS81, Line 97: int Can be unsigned? https://review.coreboot.org/c/coreboot/+/53902/comment/f1e217ee_448b0cbd PS81, Line 231: .parf = (void *) PCIE1_PCIE_PARF, : .dbi_base = (void *) PCIE1_GEN3X2_PCIE_DBI, : .elbi = (void *) PCIE1_GEN3X2_PCIE_ELBI, : .atu_base = (void *) PCIE1_GEN3X2_DWC_PCIE_DM_IATU, : .cfg_base = (void *) PCIE1_GEN3x2_CONF, : .pcie1_bcr = (void *) PCIE1_BCR, : .qmp_phy_bcr = (void *) GCC_PCIE_1_PHY_BCR, : .lanes = PCIE1_3x2_NUM_LANES, : .cfg_size = PCIE1_GEN3x2_CONF_SIZE, : .perst = GPIO(2), : : /* Store the IO and MEM windows settings for future use by the ATU */ : .io.phys_start = PCIE1_IO_ADDR, /* IO base */ : .io.bus_start = PCIE1_IO_ADDR, /* IO_bus_addr */ : .io.size = PCIE1_IO_SIZE, /* IO size */ : : .mem.phys_start = PCIE1_MEM_ADDR, /* MEM base */ : .mem.bus_start = PCIE1_MEM_ADDR, /* MEM_bus_addr */ : .mem.size = PCIE1_MEM_SIZE, Please use either tabs or spaces to align the equal sign (=). https://review.coreboot.org/c/coreboot/+/53902/comment/2c6d69c5_780aadc3 PS81, Line 284: old old_value? https://review.coreboot.org/c/coreboot/+/53902/comment/a2355ee2_0893ff9e PS81, Line 284: uint32_t offset_t? https://review.coreboot.org/c/coreboot/+/53902/comment/3f71098f_12a1da3c PS81, Line 285: uint8_t size); Fits on line above. https://review.coreboot.org/c/coreboot/+/53902/comment/4ce1a639_9bfe749b PS81, Line 285: uint8_t size_t https://review.coreboot.org/c/coreboot/+/53902/comment/5bed1301_ff722a8e PS81, Line 287: int qcom_pcie_conv_32_to_size(int value, int offset, int size); Can all arguments be unsigned? Even use `offset_t` and `size_t`? https://review.coreboot.org/c/coreboot/+/53902/comment/d9d3726a_9b769f2c PS81, Line 290: *Convtert Missing space, and typo: Convert. https://review.coreboot.org/c/coreboot/+/53902/comment/91ac82f9_e7b251a9 PS81, Line 291: * Remove blank line. https://review.coreboot.org/c/coreboot/+/53902/comment/9bb97800_9811f759 PS81, Line 306: *Convtert 8/16 bit data into 32 bit size for compatibility. : * Ditto. https://review.coreboot.org/c/coreboot/+/53902/comment/42187186_314ab382 PS81, Line 310: uint8_t size) Fits on line above. https://review.coreboot.org/c/coreboot/+/53902/comment/3dea82a3_fbb9a464 PS81, Line 336: en Please write it out. https://review.coreboot.org/c/coreboot/+/53902/comment/80070c66_1a031600 PS81, Line 336: _enable Remove that from the function name, as you pass a parameter? https://review.coreboot.org/c/coreboot/+/53902/comment/8da359d5_6bb22aa0 PS81, Line 351: uint32_t index Can `unsigned int` be used? https://review.coreboot.org/c/coreboot/+/53902/comment/5dc68acb_c3be38dc PS81, Line 372: retries Make it `unsigned int`. https://review.coreboot.org/c/coreboot/+/53902/comment/35f7a4cd_4f9633ca PS81, Line 388: /* Add a blank line above. https://review.coreboot.org/c/coreboot/+/53902/comment/2f22140f_291b29f8 PS81, Line 388: /* : * Make sure ATU enable takes effect before any subsequent config : * and I/O accesses. : */ Maybe use concise multi-line comment style. https://review.coreboot.org/c/coreboot/+/53902/comment/29f307de_ce001b9a PS81, Line 399: printk(BIOS_EMERG, "outbound iATU is not being enabled\n"); Please extend: > Outbound iATU could not be enabled after %u … attempts https://review.coreboot.org/c/coreboot/+/53902/comment/5c89868a_647095eb PS81, Line 407: GPIO_16MA, GPIO_OUTPUT); Fits on line above. https://review.coreboot.org/c/coreboot/+/53902/comment/e85c4c1b_7876d2b7 PS81, Line 415: mdelay(100); Please add a comment explaining these long delays. Referencing the PCIe specification is a good idea. https://review.coreboot.org/c/coreboot/+/53902/comment/b68734f1_1fc53558 PS81, Line 426: mdelay(50); 150 ms delays need to be well explained in coreboot. Please extend the comment. https://review.coreboot.org/c/coreboot/+/53902/comment/9f4814bf_1eee4229 PS81, Line 427: printk(BIOS_NOTICE, "PCIe PERST desserted\n"); Also info? https://review.coreboot.org/c/coreboot/+/53902/comment/1bda0cf3_f8f463f4 PS81, Line 452: DISABLE Please use standard types. https://review.coreboot.org/c/coreboot/+/53902/comment/89c7a325_045e697e PS81, Line 453: printk(BIOS_INFO, "Link speed configured in Gen %d\n", cap_speed); Please give an example message. https://review.coreboot.org/c/coreboot/+/53902/comment/3f1808d2_35c549d4 PS81, Line 459: 1 (true) The type is bool. https://review.coreboot.org/c/coreboot/+/53902/comment/d202657a_ebed8008 PS81, Line 465: register*/ Missing space. https://review.coreboot.org/c/coreboot/+/53902/comment/bfe123d6_5a8a60b1 PS81, Line 476: int bool? https://review.coreboot.org/c/coreboot/+/53902/comment/7ccacf7f_a60a626c PS81, Line 478: int unsigned int https://review.coreboot.org/c/coreboot/+/53902/comment/839c277e_19d8bcb7 PS81, Line 486: printk(BIOS_EMERG, "link never came up\n"); Mention PCIe in the message, and the number of tries. https://review.coreboot.org/c/coreboot/+/53902/comment/74901646_f0750dec PS81, Line 495: printk(BIOS_EMERG, "PCIe Link is already up\n"); Wrong log level? https://review.coreboot.org/c/coreboot/+/53902/comment/2b269d71_7cd9346a PS81, Line 496: 0 true https://review.coreboot.org/c/coreboot/+/53902/comment/14499d9f_e0db054d PS81, Line 509: printk(BIOS_EMERG, "Link up\n"); Wrong log levle, please elaborate. https://review.coreboot.org/c/coreboot/+/53902/comment/2820fc86_8bb8bc9c PS81, Line 510: 0 true https://review.coreboot.org/c/coreboot/+/53902/comment/04fb8247_f5760793 PS81, Line 512: /* : * Link can be established in Gen 1. still need to wait : */ Please add a blank line above, and use a single line comment. https://review.coreboot.org/c/coreboot/+/53902/comment/49bb7dc7_386b644f PS81, Line 515: udelay(100); Why the delay? https://review.coreboot.org/c/coreboot/+/53902/comment/4692e84b_ac0d61e6 PS81, Line 516: 1 false https://review.coreboot.org/c/coreboot/+/53902/comment/ce4628dd_c2db4232 PS81, Line 521: cfg_size unsigned int https://review.coreboot.org/c/coreboot/+/53902/comment/ba83770f_f72456df PS81, Line 528: /* Accessing root port configuration space. */ Dots can be remove from short comments. https://review.coreboot.org/c/coreboot/+/53902/comment/461b477f_d4200775 PS81, Line 535: 4 We have the macro, no need to mention the value? https://review.coreboot.org/c/coreboot/+/53902/comment/9a8a5dd6_09181c14 PS81, Line 534: if (pcierc->current_bus == 1) : /* For local bus, change TLP Type field to 4. */ : atu_type = PCIE_ATU_TYPE_CFG0; : else : /* Otherwise, change TLP Type field to 5. */ : atu_type = PCIE_ATU_TYPE_CFG1; Use ternary operator, and only one comment line. https://review.coreboot.org/c/coreboot/+/53902/comment/4a5c906b_cc702f8a PS81, Line 550: int Always returns 0? https://review.coreboot.org/c/coreboot/+/53902/comment/c8eb6146_b7fda661 PS81, Line 573: int Ditto. https://review.coreboot.org/c/coreboot/+/53902/comment/4379f22c_12610574 PS81, Line 601: bar unsigned int https://review.coreboot.org/c/coreboot/+/53902/comment/d505ea66_12f57346 PS81, Line 606: I stop reviewing here. The code needs to be improved, and the types and style issues fixed first before showing it to reviewers. https://review.coreboot.org/c/coreboot/+/53902/comment/a3d7d9d5_a1da0375 PS81, Line 842: int unsigned int https://review.coreboot.org/c/coreboot/+/53902/comment/cd0f784b_0c24384e PS81, Line 862: long unsigned long? -- To view, visit https://review.coreboot.org/c/coreboot/+/53902 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iccf60aa56541f5230fa9c3f821d7709615c36631 Gerrit-Change-Number: 53902 Gerrit-PatchSet: 81 Gerrit-Owner: Ravi Kumar Bokka <rbokka(a)codeaurora.org> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: Shelley Chen <shchen(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Reviewer: mturney mturney <mturney(a)codeaurora.org> Gerrit-CC: Furquan Shaikh <furquan.m.shaikh(a)gmail.com> Gerrit-CC: Prasad Malisetty <pmaliset(a)qualcomm.corp-partner.google.com> Gerrit-CC: Ravi Kumar Bokka <c_rbokka(a)qualcomm.corp-partner.google.com> Gerrit-Attention: Ravi Kumar Bokka <rbokka(a)codeaurora.org> Gerrit-Attention: Furquan Shaikh <furquan.m.shaikh(a)gmail.com> Gerrit-Attention: Ravi Kumar Bokka <c_rbokka(a)qualcomm.corp-partner.google.com> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Prasad Malisetty <pmaliset(a)qualcomm.corp-partner.google.com> Gerrit-Attention: mturney mturney <mturney(a)codeaurora.org> Gerrit-Comment-Date: Thu, 07 Apr 2022 05:53:00 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: link Agesa into bootblock
by build bot (Jenkins) (Code Review) 07 Apr '22

07 Apr '22

Attention is currently required from: Arthur Heymans, Kyösti Mälkki. build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55070 ) Change subject: link Agesa into bootblock ...................................................................... Patch Set 14: (1 comment) File src/drivers/amd/agesa/bootblock.c: Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-145653): https://review.coreboot.org/c/coreboot/+/55070/comment/7ad3e19d_abf74556 PS14, Line 47: void (*ap_romstage_entry)(void) = get_ap_entry_ptr(); function definition argument 'void' should also have an identifier name -- To view, visit https://review.coreboot.org/c/coreboot/+/55070 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic4c71b9c9a245e07d713839fb3628cbfc0dc3457 Gerrit-Change-Number: 55070 Gerrit-PatchSet: 14 Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-CC: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Comment-Date: Thu, 07 Apr 2022 05:52:53 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: [TEST] mb/intel/adlrvp_n: Enable Type-c Super Speed
by Usha P (Code Review) 07 Apr '22

07 Apr '22

Attention is currently required from: V Sowmya, Madhusudanarao Amara, Maulik V Vaghela. Usha P has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/62916 ) Change subject: [TEST] mb/intel/adlrvp_n: Enable Type-c Super Speed ...................................................................... Removed Code-Review+2 by Usha P <usha.p(a)intel.com> -- To view, visit https://review.coreboot.org/c/coreboot/+/62916 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I10d507fb004f6d7b9f9fe586b946f2da152a5ff5 Gerrit-Change-Number: 62916 Gerrit-PatchSet: 1 Gerrit-Owner: Usha P <usha.p(a)intel.com> Gerrit-Reviewer: Madhusudanarao Amara <madhusudanarao.amara(a)intel.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela(a)intel.com> Gerrit-Reviewer: Usha P <usha.p(a)intel.com> Gerrit-Reviewer: V Sowmya <v.sowmya(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: V Sowmya <v.sowmya(a)intel.com> Gerrit-Attention: Madhusudanarao Amara <madhusudanarao.amara(a)intel.com> Gerrit-Attention: Maulik V Vaghela <maulik.v.vaghela(a)intel.com> Gerrit-MessageType: deleteVote

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit April 2022