Attention is currently required from: Paul Menzel.
Damien Zammit has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/62809 )
Change subject: mb/hp/z220_series: Add Z220 CMT Workstation variant
......................................................................
Patch Set 2:
(2 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/62809/comment/4a6a137c_6d8f313b
PS2, Line 13:
> 1. Testing details like payload and OS should be documented. […]
1. It's a new variant. It boots to linux with the code my friend gave me, but we need to verify this commit also boots.
2. Do you mean the differences between the two boards?
File src/mainboard/hp/z220_series/Kconfig:
https://review.coreboot.org/c/coreboot/+/62809/comment/ca17661a_5d2eedad
PS2, Line 48: default "z220_cmt_workstation" if BOARD_HP_Z220_CMT_WORKSTATION
> Sort all the entries?
All 2 entries? Not worth pushing again IMHO. But if you force me to redo the commit message, I might slip this in to reorder two lines of code.
--
To view, visit https://review.coreboot.org/c/coreboot/+/62809
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I2b298921e6f509440ec7b049e086c0878f708bd3
Gerrit-Change-Number: 62809
Gerrit-PatchSet: 2
Gerrit-Owner: Damien Zammit
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Comment-Date: Wed, 16 Mar 2022 08:26:55 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-MessageType: comment
Attention is currently required from: Julius Werner, Yu-Ping Wu.
Xixi Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/62850 )
Change subject: libpayload: Add struct name "channel" for external access
......................................................................
Patch Set 2:
(1 comment)
File src/commonlib/bsd/include/commonlib/bsd/mem_chip_info.h:
https://review.coreboot.org/c/coreboot/+/62850/comment/5b5f0655_ac783235
PS2, Line 18: channel
> mem_chip_channel
Maybe the name "channel" is shorter? And the structure is in "mem_chip_info", i think we can ignore the prefix "mem_chip_"?
--
To view, visit https://review.coreboot.org/c/coreboot/+/62850
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I8dcd3b52f33f80afb7885ffdcad826d86b54b543
Gerrit-Change-Number: 62850
Gerrit-PatchSet: 2
Gerrit-Owner: Xixi Chen <xixi.chen(a)mediatek.corp-partner.google.com>
Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Ravi kumar <rbokka(a)codeaurora.org>
Gerrit-CC: Rex-BC Chen <rex-bc.chen(a)mediatek.com>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Comment-Date: Wed, 16 Mar 2022 08:16:13 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-MessageType: comment
Attention is currently required from: Hung-Te Lin, Xi Chen, Paul Menzel, Yu-Ping Wu.
Hello Xi Chen, Hung-Te Lin, build bot (Jenkins), Yu-Ping Wu,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/61334
to look at the new patch set (#21).
Change subject: soc/mediatek: Pass dram info to cbmem
......................................................................
soc/mediatek: Pass dram info to cbmem
Pass dram_info struct from coreboot to depthcharge using payload.
BUG=b:206014043
TEST=Build pass on Kingler
Signed-off-by: Xi Chen <xixi.chen(a)mediatek.corp-partner.google.com>
Change-Id: I195187c0c757a43bb6d2c57c8f303249f2a7995a
---
M src/soc/mediatek/common/memory.c
1 file changed, 48 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/61334/21
--
To view, visit https://review.coreboot.org/c/coreboot/+/61334
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I195187c0c757a43bb6d2c57c8f303249f2a7995a
Gerrit-Change-Number: 61334
Gerrit-PatchSet: 21
Gerrit-Owner: Xixi Chen <xixi.chen(a)mediatek.corp-partner.google.com>
Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org>
Gerrit-Reviewer: Xi Chen <xixi.chen(a)mediatek.com>
Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.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: Xi Chen <xixi.chen(a)mediatek.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Shelley Chen, Rex-BC Chen, Paul Menzel, Angel Pons, Yu-Ping Wu, Jianjun Wang.
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/62359 )
Change subject: soc/mediatek: PCI: Assert PERST# at bootblock stage
......................................................................
Patch Set 17:
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/62359/comment/56b5ab30_77ed2028
PS9, Line 14: 100ms
> we just need to use 8 bytes to store this information (uint64_t)
I tend to agree with the proposal - create something like WATCHDOG_TOMBSTONE in SRAM and simply use it as a timestamp for pre-init, since it's within coreboot stages (not some info that will be read by depthcharge) and also much easier to implement and maintain.
--
To view, visit https://review.coreboot.org/c/coreboot/+/62359
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Id5b9369e6f8599f93415588ea585c952a41c5e7d
Gerrit-Change-Number: 62359
Gerrit-PatchSet: 17
Gerrit-Owner: Jianjun Wang <jianjun.wang(a)mediatek.corp-partner.google.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: Shelley Chen <shchen(a)google.com>
Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Rex-BC Chen <rex-bc.chen(a)mediatek.com>
Gerrit-Attention: Shelley Chen <shchen(a)google.com>
Gerrit-Attention: Rex-BC Chen <rex-bc.chen(a)mediatek.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Attention: Jianjun Wang <jianjun.wang(a)mediatek.corp-partner.google.com>
Gerrit-Comment-Date: Wed, 16 Mar 2022 08:09:06 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Hung-Te Lin <hungte(a)chromium.org>
Comment-In-Reply-To: Rex-BC Chen <rex-bc.chen(a)mediatek.com>
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Comment-In-Reply-To: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-MessageType: comment
Attention is currently required from: Julius Werner, Xixi Chen.
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/62850 )
Change subject: libpayload: Add struct name "channel" for external access
......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS2:
oh, I see - you are trying to create a pointer to the channel structure; and yes having a struct name will be much easier.
--
To view, visit https://review.coreboot.org/c/coreboot/+/62850
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I8dcd3b52f33f80afb7885ffdcad826d86b54b543
Gerrit-Change-Number: 62850
Gerrit-PatchSet: 2
Gerrit-Owner: Xixi Chen <xixi.chen(a)mediatek.corp-partner.google.com>
Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Ravi kumar <rbokka(a)codeaurora.org>
Gerrit-CC: Rex-BC Chen <rex-bc.chen(a)mediatek.com>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Attention: Xixi Chen <xixi.chen(a)mediatek.corp-partner.google.com>
Gerrit-Comment-Date: Wed, 16 Mar 2022 08:04:55 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Julius Werner, Xixi Chen.
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/62850 )
Change subject: libpayload: Add struct name "channel" for external access
......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS2:
Is that true? I thought struct fields are accessed by variable name instead of type name, and channel is the variable name.
Example:
```
struct {
int a;
struct {
int b;
} c[0];
} d;
int main() {
d.a = 1;
d.c[0].b = 0;
return 0;
}
```
I can successfully compile this program.
--
To view, visit https://review.coreboot.org/c/coreboot/+/62850
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I8dcd3b52f33f80afb7885ffdcad826d86b54b543
Gerrit-Change-Number: 62850
Gerrit-PatchSet: 2
Gerrit-Owner: Xixi Chen <xixi.chen(a)mediatek.corp-partner.google.com>
Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Ravi kumar <rbokka(a)codeaurora.org>
Gerrit-CC: Rex-BC Chen <rex-bc.chen(a)mediatek.com>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Attention: Xixi Chen <xixi.chen(a)mediatek.corp-partner.google.com>
Gerrit-Comment-Date: Wed, 16 Mar 2022 08:02:55 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Hung-Te Lin, Xi Chen, Paul Menzel, Yu-Ping Wu.
Xixi Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/61334 )
Change subject: soc/mediatek: Pass dram info to cbmem
......................................................................
Patch Set 20:
(8 comments)
File src/soc/mediatek/common/memory.c:
https://review.coreboot.org/c/coreboot/+/61334/comment/27767be5_c19e16cc
PS18, Line 107:
> only one space
Done
https://review.coreboot.org/c/coreboot/+/61334/comment/0f32c484_45b95104
PS18, Line 106: struct mem_chip *p = &curr_dram_info;
: const struct ddr_base_info *blob = curr_ddr_info;
> move these to the parameters and pass them from the caller? […]
Done
https://review.coreboot.org/c/coreboot/+/61334/comment/11144760_2b7fda6a
PS18, Line 107: blob
> probably not 'blob'. […]
Done
https://review.coreboot.org/c/coreboot/+/61334/comment/a32eef8d_978d5a19
PS18, Line 110: MEM_CHIP_LPDDR4
> MEM_CHIP_LPDDR4X (and remove comment) […]
Done
https://review.coreboot.org/c/coreboot/+/61334/comment/77f98cc4_9e260e51
PS18, Line 112: unsigned int i = 0
> move to beginning of the function and declare only one time
Done
https://review.coreboot.org/c/coreboot/+/61334/comment/4f5a32d6_b45ac8d0
PS18, Line 128: /* Add cbmem */
> the cbmem_add is clear enough, no need to have this comment
Done
https://review.coreboot.org/c/coreboot/+/61334/comment/8575e02d_e71b21d0
PS18, Line 130: + sizeof(curr_dram_info.channel) * CHANNEL_MAX)
> so we allocated more but won't copy the rest of them? (the memcpy only copies sizeof(curr_dram_info)
Yes, right, missing the channel info.
File src/soc/mediatek/mt8186/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/61334/comment/a2fe2b54_5dc32b16
PS18, Line 74: CPPFLAGS_common += -Isrc/commonlib/bsd/include/commonlib/bsd
> Why do we need this? Can't we just include <commonlib/bsd/mem_chip_info.h> in memory. […]
Ack
--
To view, visit https://review.coreboot.org/c/coreboot/+/61334
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I195187c0c757a43bb6d2c57c8f303249f2a7995a
Gerrit-Change-Number: 61334
Gerrit-PatchSet: 20
Gerrit-Owner: Xixi Chen <xixi.chen(a)mediatek.corp-partner.google.com>
Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org>
Gerrit-Reviewer: Xi Chen <xixi.chen(a)mediatek.com>
Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.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: Xi Chen <xixi.chen(a)mediatek.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Comment-Date: Wed, 16 Mar 2022 08:01:39 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Hung-Te Lin <hungte(a)chromium.org>
Comment-In-Reply-To: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-MessageType: comment
Attention is currently required from: Yu-Ping Wu.
Hello Hung-Te Lin, Yu-Ping Wu,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/62065
to look at the new patch set (#13).
Change subject: src/mediatek/mt8186: Get dram size from CBMEM_ID_MEMINFO
......................................................................
src/mediatek/mt8186: Get dram size from CBMEM_ID_MEMINFO
Originally, dram size is set to 4GB by default. To support different
dram size, should update from CBMEM.
BUG=b:206014043
TEST=Build pass on Kingler
Signed-off-by: Xi Chen <xixi.chen(a)mediatek.corp-partner.google.com>
Change-Id: I017e9d1a2d6e26f1fc21b67b5962dfb5c6ade8a5
---
M src/soc/mediatek/mt8186/emi.c
1 file changed, 18 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/62065/13
--
To view, visit https://review.coreboot.org/c/coreboot/+/62065
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I017e9d1a2d6e26f1fc21b67b5962dfb5c6ade8a5
Gerrit-Change-Number: 62065
Gerrit-PatchSet: 13
Gerrit-Owner: Xixi Chen <xixi.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-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Rex-BC Chen <rex-bc.chen(a)mediatek.com>
Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Xi Chen, Paul Menzel.
Hello Xi Chen, Hung-Te Lin, build bot (Jenkins), Yu-Ping Wu,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/61334
to look at the new patch set (#20).
Change subject: soc/mediatek: Pass dram info to cbmem
......................................................................
soc/mediatek: Pass dram info to cbmem
Pass dram_info struct from coreboot to depthcharge using payload.
BUG=b:206014043
TEST=Build pass on Kingler
Signed-off-by: Xi Chen <xixi.chen(a)mediatek.corp-partner.google.com>
Change-Id: I195187c0c757a43bb6d2c57c8f303249f2a7995a
---
M src/soc/mediatek/common/memory.c
M src/soc/mediatek/mt8192/Makefile.inc
M src/soc/mediatek/mt8195/Makefile.inc
3 files changed, 50 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/61334/20
--
To view, visit https://review.coreboot.org/c/coreboot/+/61334
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I195187c0c757a43bb6d2c57c8f303249f2a7995a
Gerrit-Change-Number: 61334
Gerrit-PatchSet: 20
Gerrit-Owner: Xixi Chen <xixi.chen(a)mediatek.corp-partner.google.com>
Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org>
Gerrit-Reviewer: Xi Chen <xixi.chen(a)mediatek.com>
Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Rex-BC Chen <rex-bc.chen(a)mediatek.com>
Gerrit-Attention: Xi Chen <xixi.chen(a)mediatek.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-MessageType: newpatchset