huayang duan has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38474 )
Change subject: soc/mediatek/mt8183: Fix typo error of DRAMC setting ......................................................................
soc/mediatek/mt8183: Fix typo error of DRAMC setting
1. The ac timing of 2400Mbps should use diff params with 1600Mbps. 2. Fix the typo error of save shufffle function for DVFS.
BRANCH=kukui BUG=none TEST=emerge-kukui coreboot
Change-Id: I5edac32938def50836f386426e7deb652b80d42d Signed-off-by: Huayang Duan huayang.duan@mediatek.com --- M src/soc/mediatek/mt8183/emi.c 1 file changed, 3 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/38474/1
diff --git a/src/soc/mediatek/mt8183/emi.c b/src/soc/mediatek/mt8183/emi.c index cf104f8..3f157e3 100644 --- a/src/soc/mediatek/mt8183/emi.c +++ b/src/soc/mediatek/mt8183/emi.c @@ -325,7 +325,7 @@ { struct optimize_ac_time rf_cab_opt[LP4X_DDRFREQ_MAX] = { [LP4X_DDR1600] = {.rfc = 44, .rfc_05t = 0, .tx_ref_cnt = 62}, - [LP4X_DDR2400] = {.rfc = 44, .rfc_05t = 0, .tx_ref_cnt = 62}, + [LP4X_DDR2400] = {.rfc = 72, .rfc_05t = 0, .tx_ref_cnt = 91}, [LP4X_DDR3200] = {.rfc = 100, .rfc_05t = 0, .tx_ref_cnt = 119}, [LP4X_DDR3600] = {.rfc = 118, .rfc_05t = 1, .tx_ref_cnt = 138}, }; @@ -456,9 +456,9 @@ value = read32(src_addr) & 0x7f;
if (dst_shuffle == DRAM_DFS_SHUFFLE_2) - clrsetbits32(dst_addr, 0x7f << 0x8, value << 0x8); + clrsetbits_le32(dst_addr, 0x7f << 8, value << 8); else if (dst_shuffle == DRAM_DFS_SHUFFLE_3) - clrsetbits32(dst_addr, 0x7f << 0x16, value << 0x16); + clrsetbits_le32(dst_addr, 0x7f << 16, value << 16);
/* DRAMC-exception-2 */ src_addr = (u8 *)&ch[chn].ao.dvfsdll;
Hello Julius Werner,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38474
to look at the new patch set (#2).
Change subject: soc/mediatek/mt8183: Fix typo error of DRAMC setting ......................................................................
soc/mediatek/mt8183: Fix typo error of DRAMC setting
1. The ac timing of 2400Mbps should use diff params with 1600Mbps. 2. Fix the typo error of save shufffle function for DVFS.
BRANCH=kukui BUG=none TEST=emerge-kukui coreboot
Change-Id: I5edac32938def50836f386426e7deb652b80d42d Signed-off-by: Huayang Duan huayang.duan@mediatek.com --- M src/soc/mediatek/mt8183/emi.c 1 file changed, 3 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/38474/2
Hello Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38474
to look at the new patch set (#3).
Change subject: soc/mediatek/mt8183: Fix typo error of DRAMC setting ......................................................................
soc/mediatek/mt8183: Fix typo error of DRAMC setting
1. The ac timing of 2400Mbps should use diff params with 1600Mbps. 2. Fix the typo error of save shufffle function for DVFS.
BRANCH=kukui BUG=none TEST=emerge-kukui coreboot
Change-Id: I5edac32938def50836f386426e7deb652b80d42d Signed-off-by: Huayang Duan huayang.duan@mediatek.com --- M src/soc/mediatek/mt8183/emi.c 1 file changed, 3 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/38474/3
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38474 )
Change subject: soc/mediatek/mt8183: Fix typo error of DRAMC setting ......................................................................
Patch Set 3:
(2 comments)
Two commits would be nice.
https://review.coreboot.org/c/coreboot/+/38474/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38474/3//COMMIT_MSG@10 PS3, Line 10: shufffle shuffle
https://review.coreboot.org/c/coreboot/+/38474/3//COMMIT_MSG@11 PS3, Line 11: Why not two commits?
Idwer Vollering has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38474 )
Change subject: soc/mediatek/mt8183: Fix typo error of DRAMC setting ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38474/3/src/soc/mediatek/mt8183/emi... File src/soc/mediatek/mt8183/emi.c:
https://review.coreboot.org/c/coreboot/+/38474/3/src/soc/mediatek/mt8183/emi... PS3, Line 461: clrsetbits32(dst_addr, 0x7f << 16, value << 16); 0x16 converts to 22, not 16. Is this intentional?
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38474 )
Change subject: soc/mediatek/mt8183: Fix typo error of DRAMC setting ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38474/3/src/soc/mediatek/mt8183/emi... File src/soc/mediatek/mt8183/emi.c:
https://review.coreboot.org/c/coreboot/+/38474/3/src/soc/mediatek/mt8183/emi... PS3, Line 461: clrsetbits32(dst_addr, 0x7f << 16, value << 16);
0x16 converts to 22, not 16. […]
That’s the second item of the commit message.
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38474 )
Change subject: soc/mediatek/mt8183: Fix typo error of DRAMC setting ......................................................................
Patch Set 3:
@huayang, do we have to wipe out existing saved calibration cache after applying this patch?
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38474 )
Change subject: soc/mediatek/mt8183: Fix typo error of DRAMC setting ......................................................................
Patch Set 3:
and like the other comments said, you should split this into two patches.
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38474 )
Change subject: soc/mediatek/mt8183: Fix typo error of DRAMC setting ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38474/3/src/soc/mediatek/mt8183/emi... File src/soc/mediatek/mt8183/emi.c:
https://review.coreboot.org/c/coreboot/+/38474/3/src/soc/mediatek/mt8183/emi... PS3, Line 328: [LP4X_DDR2400] = {.rfc = 72, .rfc_05t = 0, .tx_ref_cnt = 91}, Agree with Paul Menzel. This is clearly not a typo.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38474 )
Change subject: soc/mediatek/mt8183: Fix typo error of DRAMC setting ......................................................................
Patch Set 3: Code-Review+1
(4 comments)
https://review.coreboot.org/c/coreboot/+/38474/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38474/3//COMMIT_MSG@7 PS3, Line 7: soc/mediatek/mt8183: Fix typo error of DRAMC setting As the errors are not typos, maybe change the commit summary:
soc/mediatek/mt8183: Fix programming errors of DRAMC settings
https://review.coreboot.org/c/coreboot/+/38474/3//COMMIT_MSG@11 PS3, Line 11:
Why not two commits?
I'm not sure if it's worth the hassle with such tiny changes.
https://review.coreboot.org/c/coreboot/+/38474/3/src/soc/mediatek/mt8183/emi... File src/soc/mediatek/mt8183/emi.c:
https://review.coreboot.org/c/coreboot/+/38474/3/src/soc/mediatek/mt8183/emi... PS3, Line 328: [LP4X_DDR2400] = {.rfc = 72, .rfc_05t = 0, .tx_ref_cnt = 91},
Agree with Paul Menzel. This is clearly not a typo.
I guess the error was a "copy-pasta" (copied and pasted, but did not change the values)
https://review.coreboot.org/c/coreboot/+/38474/3/src/soc/mediatek/mt8183/emi... PS3, Line 461: clrsetbits32(dst_addr, 0x7f << 16, value << 16);
That’s the second item of the commit message.
It is intentional. Notice the code below that does the same operation with other values.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38474 )
Change subject: soc/mediatek/mt8183: Fix typo error of DRAMC setting ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38474/3/src/soc/mediatek/mt8183/emi... File src/soc/mediatek/mt8183/emi.c:
https://review.coreboot.org/c/coreboot/+/38474/3/src/soc/mediatek/mt8183/emi... PS3, Line 328: [LP4X_DDR2400] = {.rfc = 72, .rfc_05t = 0, .tx_ref_cnt = 91},
I guess the error was a "copy-pasta" (copied and pasted, but did not change the values)
In any case, the corrected values make more sense, as they correspond to the midpoint between DDR1600 and DDR3200.
Duan huayang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38474 )
Change subject: soc/mediatek/mt8183: Fix typo error of DRAMC setting ......................................................................
Patch Set 3:
Patch Set 3:
@huayang, do we have to wipe out existing saved calibration cache after applying this patch?
no need clear the saved calibration cache
Duan huayang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38474 )
Change subject: soc/mediatek/mt8183: Fix typo error of DRAMC setting ......................................................................
Patch Set 3:
(5 comments)
https://review.coreboot.org/c/coreboot/+/38474/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38474/3//COMMIT_MSG@7 PS3, Line 7: soc/mediatek/mt8183: Fix typo error of DRAMC setting
As the errors are not typos, maybe change the commit summary: […]
Done
https://review.coreboot.org/c/coreboot/+/38474/3//COMMIT_MSG@10 PS3, Line 10: shufffle
shuffle
Done
https://review.coreboot.org/c/coreboot/+/38474/3//COMMIT_MSG@11 PS3, Line 11:
I'm not sure if it's worth the hassle with such tiny changes.
Ack
https://review.coreboot.org/c/coreboot/+/38474/3/src/soc/mediatek/mt8183/emi... File src/soc/mediatek/mt8183/emi.c:
https://review.coreboot.org/c/coreboot/+/38474/3/src/soc/mediatek/mt8183/emi... PS3, Line 328: [LP4X_DDR2400] = {.rfc = 72, .rfc_05t = 0, .tx_ref_cnt = 91},
In any case, the corrected values make more sense, as they correspond to the midpoint between DDR160 […]
Ack
https://review.coreboot.org/c/coreboot/+/38474/3/src/soc/mediatek/mt8183/emi... PS3, Line 461: clrsetbits32(dst_addr, 0x7f << 16, value << 16);
It is intentional. Notice the code below that does the same operation with other values.
Ack
Hello Yu-Ping Wu, Duan huayang, Angel Pons, Julius Werner, You-Cheng Syu, Hung-Te Lin, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38474
to look at the new patch set (#4).
Change subject: soc/mediatek/mt8183: Fix programming error of DRAMC setting ......................................................................
soc/mediatek/mt8183: Fix programming error of DRAMC setting
1. The ac timing of 2400Mbps should use diff params with 1600Mbps. 2. Fix the typo error of save shuffle function for DVFS.
BRANCH=kukui BUG=none TEST=emerge-kukui coreboot
Change-Id: I5edac32938def50836f386426e7deb652b80d42d Signed-off-by: Huayang Duan huayang.duan@mediatek.com --- M src/soc/mediatek/mt8183/emi.c 1 file changed, 3 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/74/38474/4
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38474 )
Change subject: soc/mediatek/mt8183: Fix programming error of DRAMC setting ......................................................................
Patch Set 5: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/38474/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38474/5//COMMIT_MSG@14 PS5, Line 14: TEST=emerge-kukui coreboot Did you check if kukui can boot?
Duan huayang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38474 )
Change subject: soc/mediatek/mt8183: Fix programming error of DRAMC setting ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38474/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38474/5//COMMIT_MSG@14 PS5, Line 14: TEST=emerge-kukui coreboot
Did you check if kukui can boot?
yes,can bootup success.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38474 )
Change subject: soc/mediatek/mt8183: Fix programming error of DRAMC setting ......................................................................
Patch Set 5: Code-Review+2
To me, it looks good.
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38474 )
Change subject: soc/mediatek/mt8183: Fix programming error of DRAMC setting ......................................................................
Patch Set 5: Code-Review+2
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38474 )
Change subject: soc/mediatek/mt8183: Fix programming error of DRAMC setting ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38474/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38474/5//COMMIT_MSG@14 PS5, Line 14: TEST=emerge-kukui coreboot
yes,can bootup success.
Ack
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38474 )
Change subject: soc/mediatek/mt8183: Fix programming error of DRAMC setting ......................................................................
soc/mediatek/mt8183: Fix programming error of DRAMC setting
1. The ac timing of 2400Mbps should use diff params with 1600Mbps. 2. Fix the typo error of save shuffle function for DVFS.
BRANCH=kukui BUG=none TEST=emerge-kukui coreboot
Change-Id: I5edac32938def50836f386426e7deb652b80d42d Signed-off-by: Huayang Duan huayang.duan@mediatek.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/38474 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Yu-Ping Wu yupingso@google.com Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Hung-Te Lin hungte@chromium.org --- M src/soc/mediatek/mt8183/emi.c 1 file changed, 3 insertions(+), 3 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved Hung-Te Lin: Looks good to me, approved Yu-Ping Wu: Looks good to me, but someone else must approve
diff --git a/src/soc/mediatek/mt8183/emi.c b/src/soc/mediatek/mt8183/emi.c index cf104f8..07d1cc8 100644 --- a/src/soc/mediatek/mt8183/emi.c +++ b/src/soc/mediatek/mt8183/emi.c @@ -325,7 +325,7 @@ { struct optimize_ac_time rf_cab_opt[LP4X_DDRFREQ_MAX] = { [LP4X_DDR1600] = {.rfc = 44, .rfc_05t = 0, .tx_ref_cnt = 62}, - [LP4X_DDR2400] = {.rfc = 44, .rfc_05t = 0, .tx_ref_cnt = 62}, + [LP4X_DDR2400] = {.rfc = 72, .rfc_05t = 0, .tx_ref_cnt = 91}, [LP4X_DDR3200] = {.rfc = 100, .rfc_05t = 0, .tx_ref_cnt = 119}, [LP4X_DDR3600] = {.rfc = 118, .rfc_05t = 1, .tx_ref_cnt = 138}, }; @@ -456,9 +456,9 @@ value = read32(src_addr) & 0x7f;
if (dst_shuffle == DRAM_DFS_SHUFFLE_2) - clrsetbits32(dst_addr, 0x7f << 0x8, value << 0x8); + clrsetbits32(dst_addr, 0x7f << 8, value << 8); else if (dst_shuffle == DRAM_DFS_SHUFFLE_3) - clrsetbits32(dst_addr, 0x7f << 0x16, value << 0x16); + clrsetbits32(dst_addr, 0x7f << 16, value << 16);
/* DRAMC-exception-2 */ src_addr = (u8 *)&ch[chn].ao.dvfsdll;
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38474 )
Change subject: soc/mediatek/mt8183: Fix programming error of DRAMC setting ......................................................................
Patch Set 6:
Automatic boot test returned (PASS/FAIL/TOTAL): 3/0/3 Emulation targets: EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/863 EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/862 EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/861
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Angel Pons (Code Review)
-
Duan huayang (Code Review)
-
huayang duan (Code Review)
-
Hung-Te Lin (Code Review)
-
Idwer Vollering (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)
-
Yu-Ping Wu (Code Review)