Attention is currently required from: Dinesh Gehlot, Eric Lai, Jayvik Desai, Kapil Porwal, Nick Vaccaro, Paul Menzel, V Sowmya, Vidya Gopalakrishnan, Vinay Kumar.
Subrata Banik has posted comments on this change by Vidya Gopalakrishnan. ( https://review.coreboot.org/c/coreboot/+/85184?usp=email )
Change subject: mb/google/brya/var/trulo: Remove overriding of PL1 value to 20W
......................................................................
Patch Set 5:
(1 comment)
File src/mainboard/google/brya/variants/trulo/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/85184/comment/65e6a3f3_ea1cb6b4?us… :
PS4, Line 184: register "power_limits_config[ADL_N_041_6W_CORE]" = "{
: .tdp_pl1_override = 20,
: .tdp_pl2_override = 25,
: .tdp_pl4 = 78,
: }"
:
: register "power_limits_config[ADL_N_081_7W_CORE]" = "{
: .tdp_pl1_override = 20,
: .tdp_pl2_override = 25,
: .tdp_pl4 = 78,
: }"
:
: register "power_limits_config[ADL_N_081_15W_CORE]" = "{
: .tdp_pl1_override = 20,
: .tdp_pl2_override = 35,
: .tdp_pl4 = 83,
: }"
> > PL1/PL2 override hooks can still be used by ODMs by adding this block of code back. But overriding PL1/PL2 to a higher value as done here should not be a default guidance given from a reference design as using a higher PLx can have thermal impacts to silicon. This code block seems to be copied from some other production device variant that is also using a higher override for PLx values. Such overrides are not part of any other reference design variant.
>
> This code may coexist with the actual limit if there's a check to assert the build if the overridden value is larger than the limit. Ideally, a check like this should be included.
>
> There is no override required if overriden value is invalid or matches to the existing limit.
we have agreed to implement an assert logic if overriden value is higher than the limit.
--
To view, visit https://review.coreboot.org/c/coreboot/+/85184?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I798c4f10e10a579f470e00dbdb77a84619ad796a
Gerrit-Change-Number: 85184
Gerrit-PatchSet: 5
Gerrit-Owner: Vidya Gopalakrishnan <vidya.gopalakrishnan(a)intel.com>
Gerrit-Reviewer: Dinesh Gehlot <digehlot(a)google.com>
Gerrit-Reviewer: Eric Lai <ericllai(a)google.com>
Gerrit-Reviewer: Jayvik Desai <jayvik(a)google.com>
Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: V Sowmya <v.sowmya(a)intel.com>
Gerrit-Reviewer: Vinay Kumar <vinay.kumar(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: Jayvik Desai <jayvik(a)google.com>
Gerrit-Attention: Eric Lai <ericllai(a)google.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Dinesh Gehlot <digehlot(a)google.com>
Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Attention: Vidya Gopalakrishnan <vidya.gopalakrishnan(a)intel.com>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)chromium.org>
Gerrit-Attention: Vinay Kumar <vinay.kumar(a)intel.com>
Gerrit-Comment-Date: Fri, 22 Nov 2024 08:53:30 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Comment-In-Reply-To: Vidya Gopalakrishnan <vidya.gopalakrishnan(a)intel.com>
Attention is currently required from: Hung-Te Lin, Jarried Lin, Yidi Lin, Yu-Ping Wu.
Crystal Guo has posted comments on this change by Jarried Lin. ( https://review.coreboot.org/c/coreboot/+/85123?usp=email )
Change subject: soc/mediatek/common: Save dram_reserved_buffer info to sram
......................................................................
Patch Set 11:
(1 comment)
File src/soc/mediatek/common/memory.c:
https://review.coreboot.org/c/coreboot/+/85123/comment/c263fedb_d564a027?us… :
PS8, Line 359: reserved_buffer_ptr->dram_rank_num = curr_ddr_info->support_ranks;
> Thanks. […]
mt8196/emi.c has been removed in CL*85098...
Or create a new file to calculate the ranges?
--
To view, visit https://review.coreboot.org/c/coreboot/+/85123?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Ibc74103c3826a4de525e183e764aa1fd9ac2c131
Gerrit-Change-Number: 85123
Gerrit-PatchSet: 11
Gerrit-Owner: Jarried Lin <jarried.lin(a)mediatek.com>
Gerrit-Reviewer: Crystal Guo <crystal.guo(a)mediatek.corp-partner.google.com>
Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org>
Gerrit-Reviewer: Yidi Lin <yidilin(a)google.com>
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: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Attention: Yidi Lin <yidilin(a)google.com>
Gerrit-Attention: Jarried Lin <jarried.lin(a)mediatek.com>
Gerrit-Comment-Date: Fri, 22 Nov 2024 08:16:23 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Crystal Guo <crystal.guo(a)mediatek.corp-partner.google.com>
Comment-In-Reply-To: Yu-Ping Wu <yupingso(a)google.com>
Comment-In-Reply-To: Yidi Lin <yidilin(a)google.com>
Attention is currently required from: Crystal Guo, Hung-Te Lin, Jarried Lin, Yidi Lin.
Yu-Ping Wu has posted comments on this change by Jarried Lin. ( https://review.coreboot.org/c/coreboot/+/85123?usp=email )
Change subject: soc/mediatek/common: Save dram_reserved_buffer info to sram
......................................................................
Patch Set 11:
(1 comment)
File src/soc/mediatek/common/memory.c:
https://review.coreboot.org/c/coreboot/+/85123/comment/80ca3b76_419f40c8?us… :
PS8, Line 359: reserved_buffer_ptr->dram_rank_num = curr_ddr_info->support_ranks;
> Yes, it works, thanks […]
Thanks. Instead of calculating the ranges in `bootmem_platform_add_ranges`, please add an API in mt8196/emi.c.
--
To view, visit https://review.coreboot.org/c/coreboot/+/85123?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Ibc74103c3826a4de525e183e764aa1fd9ac2c131
Gerrit-Change-Number: 85123
Gerrit-PatchSet: 11
Gerrit-Owner: Jarried Lin <jarried.lin(a)mediatek.com>
Gerrit-Reviewer: Crystal Guo <crystal.guo(a)mediatek.corp-partner.google.com>
Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org>
Gerrit-Reviewer: Yidi Lin <yidilin(a)google.com>
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: Crystal Guo <crystal.guo(a)mediatek.corp-partner.google.com>
Gerrit-Attention: Yidi Lin <yidilin(a)google.com>
Gerrit-Attention: Jarried Lin <jarried.lin(a)mediatek.com>
Gerrit-Comment-Date: Fri, 22 Nov 2024 08:12:26 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Crystal Guo <crystal.guo(a)mediatek.corp-partner.google.com>
Comment-In-Reply-To: Yu-Ping Wu <yupingso(a)google.com>
Comment-In-Reply-To: Yidi Lin <yidilin(a)google.com>
Attention is currently required from: Hung-Te Lin, Jarried Lin, Paul Menzel.
Yu-Ping Wu has posted comments on this change by Jarried Lin. ( https://review.coreboot.org/c/coreboot/+/85188?usp=email )
Change subject: soc/mediatek/mt8196: Add MMinfra driver support
......................................................................
Patch Set 5:
(3 comments)
File src/soc/mediatek/mt8196/bootblock.c:
https://review.coreboot.org/c/coreboot/+/85188/comment/cc580518_48e6c4f9?us… :
PS5, Line 19: mm_pre_init
Why is this called `pre_init`? This only registers callbacks for 3 IDs. Looking at the code, I don't think we need to register them before `mt_pll_post_init`. The `disp_vcore_cb` callback in mtcmos.c is also registered in `mtcmos_post_init`, not in `pre_init` or `init` function.
I wonder if we can rename this function to `mminfra_post_init`, and move the 3 `mtcmos_ctrl` calls there:
```
void mminfra_post_init(void)
{
mtcmos_cb_register(...);
mtcmos_cb_register(...);
mtcmos_cb_register(...);
mtcmos_ctrl(MTCMOS_ID_MM_INFRA_AO, MTCMOS_POWER_ON);
mtcmos_ctrl(MTCMOS_ID_MM_INFRA0, MTCMOS_POWER_ON);
mtcmos_ctrl(MTCMOS_ID_MM_INFRA1, MTCMOS_POWER_ON);
}
```
Then the flow here would be:
```
mtcmos_init();
mt_pll_post_init();
mminfra_post_init();
mtcmos_post_init();
```
Does that make sense to you?
File src/soc/mediatek/mt8196/include/soc/mminfra.h:
https://review.coreboot.org/c/coreboot/+/85188/comment/5f27db01_8ff7fe78?us… :
PS5, Line 33: mm
mminfra
File src/soc/mediatek/mt8196/mminfra_pd.c:
PS5:
If there's no other `mminfra_*.c` files, can we rename this to `mminfra.c`?
--
To view, visit https://review.coreboot.org/c/coreboot/+/85188?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Ie86f141a0957fc60d4973875c0dbcbdb57be1f75
Gerrit-Change-Number: 85188
Gerrit-PatchSet: 5
Gerrit-Owner: Jarried Lin <jarried.lin(a)mediatek.com>
Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org>
Gerrit-Reviewer: Yidi Lin <yidilin(a)google.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-Attention: Hung-Te Lin <hungte(a)chromium.org>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Jarried Lin <jarried.lin(a)mediatek.com>
Gerrit-Comment-Date: Fri, 22 Nov 2024 07:58:44 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Attention is currently required from: Guangjie Song, Hung-Te Lin, Jarried Lin.
Yu-Ping Wu has posted comments on this change by Jarried Lin. ( https://review.coreboot.org/c/coreboot/+/84497?usp=email )
Change subject: soc/mediatek/mt8196: Add mtcmos init support
......................................................................
Patch Set 34:
(2 comments)
File src/soc/mediatek/mt8196/mtcmos.c:
https://review.coreboot.org/c/coreboot/+/84497/comment/d293607f_9a5c4b58?us… :
PS34, Line 390: if (!strcmp(item->name, cb->name)) {
Using a string as the unique identifier is inefficient. If we really worry about having register duplicate callbacks, please use numeric IDs. However I'm not sure if the check is needed here.
https://review.coreboot.org/c/coreboot/+/84497/comment/41651479_d3492d78?us… :
PS34, Line 836: mtcmos_ctrl(MTCMOS_ID_MM_INFRA_AO, MTCMOS_POWER_ON);
: mtcmos_ctrl(MTCMOS_ID_MM_INFRA0, MTCMOS_POWER_ON);
: mtcmos_ctrl(MTCMOS_ID_MM_INFRA1, MTCMOS_POWER_ON);
:
: mtcmos_ctrl(MTCMOS_ID_DISP_VCORE, MTCMOS_POWER_ON);
So, the whole callback code is only for these 4 `mtcmos_ctrl` calls? If there are no other calls that need callbacks, then we can remove the callback infrastructure altogether.
```
// spm_mtcmos.h
struct mtcmos_ops {
int (*pre_on)(void);
int (*pre_off)(void);
...
int (*post_on_sram)(void);
};
// mtcmos.c
void mtcmos_ctrl_ops(enum mtcmos_id id, enum mtcmos_state state,
const struct mtcmos_ops *ops)
{
...;
}
void mtcmos_ctrl(enum mtcmos_id id, enum mtcmos_state state)
{
mtcmos_ctrl_ops(id, state, NULL);
}
static struct mtcmos_ops disp_vcore_ops {
...;
};
void mtcmos_post_init(void)
{
...
mtcmos_ctrl_ops(MTCMOS_ID_DISP_VCORE, MTCMOS_POWER_ON, &disp_vcore_ops);
...
}
```
--
To view, visit https://review.coreboot.org/c/coreboot/+/84497?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I44f2bb10453377a8412e80ac0c100760ebfbaff9
Gerrit-Change-Number: 84497
Gerrit-PatchSet: 34
Gerrit-Owner: Jarried Lin <jarried.lin(a)mediatek.com>
Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org>
Gerrit-Reviewer: Yidi Lin <yidilin(a)google.com>
Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Guangjie Song <guangjie.song(a)mediatek.corp-partner.google.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Hung-Te Lin <hungte(a)chromium.org>
Gerrit-Attention: Guangjie Song <guangjie.song(a)mediatek.corp-partner.google.com>
Gerrit-Attention: Jarried Lin <jarried.lin(a)mediatek.com>
Gerrit-Comment-Date: Fri, 22 Nov 2024 07:56:56 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Attention is currently required from: Kenji Yu, Yu-Ping Wu.
Yidi Lin has posted comments on this change by Kenji Yu. ( https://review.coreboot.org/c/blobs/+/85247?usp=email )
Change subject: soc/mediatek/mt8196: Update SSPM firmware to v2.0
......................................................................
Patch Set 8:
(2 comments)
Commit Message:
https://review.coreboot.org/c/blobs/+/85247/comment/ab92af2d_6dc4925f?usp=e… :
PS8, Line 9: Update sspm.bin to fix suspend issue.
Can you elaborate the original suspend issue ?
https://review.coreboot.org/c/blobs/+/85247/comment/d730c1d1_9df31631?usp=e… :
PS8, Line 11: b:317009620
I don't think this is the correct bug ID.
--
To view, visit https://review.coreboot.org/c/blobs/+/85247?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: blobs
Gerrit-Branch: main
Gerrit-Change-Id: I2c5503e31c2d97bc23d39c7ae442a1849a0dec85
Gerrit-Change-Number: 85247
Gerrit-PatchSet: 8
Gerrit-Owner: Kenji Yu <kenji.yu(a)mediatek.corp-partner.google.com>
Gerrit-Reviewer: Jarried Lin <jarried.lin(a)mediatek.com>
Gerrit-Reviewer: Kenji Yu <kenji.yu(a)mediatek.corp-partner.google.com>
Gerrit-Reviewer: Yidi Lin <yidilin(a)google.com>
Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-CC: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Attention: Kenji Yu <kenji.yu(a)mediatek.corp-partner.google.com>
Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Comment-Date: Fri, 22 Nov 2024 07:28:41 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Attention is currently required from: Jarried Lin.
Kenji Yu has posted comments on this change by Kenji Yu. ( https://review.coreboot.org/c/blobs/+/85247?usp=email )
Change subject: soc/mediatek/mt8196: Update SSPM firmware to v2.0
......................................................................
Patch Set 8: Code-Review+1
--
To view, visit https://review.coreboot.org/c/blobs/+/85247?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: blobs
Gerrit-Branch: main
Gerrit-Change-Id: I2c5503e31c2d97bc23d39c7ae442a1849a0dec85
Gerrit-Change-Number: 85247
Gerrit-PatchSet: 8
Gerrit-Owner: Kenji Yu <kenji.yu(a)mediatek.corp-partner.google.com>
Gerrit-Reviewer: Jarried Lin <jarried.lin(a)mediatek.com>
Gerrit-Reviewer: Kenji Yu <kenji.yu(a)mediatek.corp-partner.google.com>
Gerrit-CC: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Attention: Jarried Lin <jarried.lin(a)mediatek.com>
Gerrit-Comment-Date: Fri, 22 Nov 2024 05:42:15 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes