hsin-hsiung wang has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32057
Change subject: mediatek/mt8183: modify vsim2 calibration ......................................................................
mediatek/mt8183: modify vsim2 calibration
This patch modifies vsim2 calibration to meet 2.7V
BUG=b:126139364 BRANCH=none TEST=measure vsim2 voltage with multimeter
Change-Id: I4dffdde89cbde91286d92e6c2b445f0b3d0ad2fe Signed-off-by: Hsin-Hsiung Wang hsin-hsiung.wang@mediatek.com --- M src/soc/mediatek/mt8183/include/soc/mt6358.h M src/soc/mediatek/mt8183/mt6358.c 2 files changed, 33 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/32057/1
diff --git a/src/soc/mediatek/mt8183/include/soc/mt6358.h b/src/soc/mediatek/mt8183/include/soc/mt6358.h index 02937ba..157f6be 100644 --- a/src/soc/mediatek/mt8183/include/soc/mt6358.h +++ b/src/soc/mediatek/mt8183/include/soc/mt6358.h @@ -27,6 +27,21 @@ PMIC_CPSDSA4 = 0x0a2e, PMIC_VDRAM1_VOSEL_SLEEP = 0x160a, PMIC_SMPS_ANA_CON0 = 0x1808, + PMIC_VSIM2_ANA_CON0 = 0x1c34, +}; + +enum { + VSIM2_CALI_POS_0_MV = 0, + VSIM2_CALI_POS_10_MV, + VSIM2_CALI_POS_20_MV, + VSIM2_CALI_POS_30_MV, + VSIM2_CALI_POS_40_MV, + VSIM2_CALI_POS_50_MV, + VSIM2_CALI_POS_60_MV, + VSIM2_CALI_POS_70_MV, + VSIM2_CALI_POS_80_MV, + VSIM2_CALI_POS_90_MV, + VSIM2_CALI_POS_100_MV, };
struct pmic_setting { diff --git a/src/soc/mediatek/mt8183/mt6358.c b/src/soc/mediatek/mt8183/mt6358.c index 3dc9fe2..bcddb783 100644 --- a/src/soc/mediatek/mt8183/mt6358.c +++ b/src/soc/mediatek/mt8183/mt6358.c @@ -769,6 +769,23 @@ lp_setting[i].mask, lp_setting[i].shift); }
+static void pmic_set_vsim2_cali(void) +{ + u16 vsim2_cali; + + /* [11:8]=0x8, RG_VSIM2_VOSEL = 2.7V */ + pwrap_write_field(PMIC_VSIM2_ANA_CON0, 0x8, 0xF, 8); + + /* [3:0], RG_VSIM2_VOCAL */ + vsim2_cali = pwrap_read_field(PMIC_VSIM2_ANA_CON0, 0xF, 0); + if (vsim2_cali > VSIM2_CALI_POS_60_MV) + vsim2_cali -= VSIM2_CALI_POS_60_MV; + else + vsim2_cali = VSIM2_CALI_POS_0_MV; + + pwrap_write_field(PMIC_VSIM2_ANA_CON0, vsim2_cali, 0xF, 0); +} + void mt6358_init(void) { if (pwrap_init()) @@ -780,4 +797,5 @@ wk_sleep_voltage_by_ddr(); wk_power_down_seq(); mt6358_lp_setting(); + pmic_set_vsim2_cali(); }
Hello Julius Werner,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32057
to look at the new patch set (#2).
Change subject: mediatek/mt8183: modify vsim2 calibration ......................................................................
mediatek/mt8183: modify vsim2 calibration
This patch modifies vsim2 calibration to meet 2.7V
BUG=b:126139364 BRANCH=none TEST=measure vsim2 voltage with multimeter
Change-Id: I4dffdde89cbde91286d92e6c2b445f0b3d0ad2fe Signed-off-by: Hsin-Hsiung Wang hsin-hsiung.wang@mediatek.com --- M src/soc/mediatek/mt8183/include/soc/mt6358.h M src/soc/mediatek/mt8183/mt6358.c 2 files changed, 33 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/32057/2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32057 )
Change subject: mediatek/mt8183: modify vsim2 calibration ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/#/c/32057/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32057/1//COMMIT_MSG@7 PS1, Line 7: mediatek/mt8183: modify vsim2 calibration Please be more specific in the summary. Maybe:
Calibrate vsim2 to 2.7 V
https://review.coreboot.org/#/c/32057/1//COMMIT_MSG@9 PS1, Line 9: This patch modifies vsim2 calibration to meet 2.7V 1. Please add a dot/period at the end of sentences. 2. Please add the motivation, why 2.7 V are required.
https://review.coreboot.org/#/c/32057/1/src/soc/mediatek/mt8183/include/soc/... File src/soc/mediatek/mt8183/include/soc/mt6358.h:
https://review.coreboot.org/#/c/32057/1/src/soc/mediatek/mt8183/include/soc/... PS1, Line 30: PMIC_VSIM2_ANA_CON0 = 0x1c34, Use spaces for alignment as above.
Hello 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/+/32057
to look at the new patch set (#3).
Change subject: mediatek/mt8183: modify vsim2 calibration ......................................................................
mediatek/mt8183: modify vsim2 calibration
This patch modifies vsim2 calibration to meet 2.7V
BUG=b:126139364 BRANCH=none TEST=measure vsim2 voltage with multimeter
Change-Id: I4dffdde89cbde91286d92e6c2b445f0b3d0ad2fe Signed-off-by: Hsin-Hsiung Wang hsin-hsiung.wang@mediatek.com --- M src/soc/mediatek/mt8183/include/soc/mt6358.h M src/soc/mediatek/mt8183/mt6358.c 2 files changed, 33 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/32057/3
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32057 )
Change subject: mediatek/mt8183: modify vsim2 calibration ......................................................................
Patch Set 3:
Please also amend the commit message.
You-Cheng Syu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32057 )
Change subject: mediatek/mt8183: modify vsim2 calibration ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/32057/3/src/soc/mediatek/mt8183/mt6358.c File src/soc/mediatek/mt8183/mt6358.c:
https://review.coreboot.org/#/c/32057/3/src/soc/mediatek/mt8183/mt6358.c@782 PS3, Line 782: VSIM2_CALI_POS_60_MV It's weird to me that you define a lot of enum for different values and perform arithmetic computation on them... What about rename 'vsim2_cali' to 'vsim2_cali_10mv' (or any better name) and remove those enums? And line 781-784 looks equivalent to 'vsim2_cali = MAX(0, vsim2_cali - 6);'
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32057 )
Change subject: mediatek/mt8183: modify vsim2 calibration ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/32057/3/src/soc/mediatek/mt8183/mt6358.c File src/soc/mediatek/mt8183/mt6358.c:
https://review.coreboot.org/#/c/32057/3/src/soc/mediatek/mt8183/mt6358.c@782 PS3, Line 782: VSIM2_CALI_POS_60_MV
And line 781-784 looks equivalent to 'vsim2_cali = MAX(0, vsim2_cali - 6);'
Just wanna caution about doing math like that on unsigned types. This would work here, but only due to weird quirks in the integer promotion rules. If vsim2_cali was a u32 it would blow up in your face. Better to just always use 'int' for those kinds of things to be safe.
Hello 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/+/32057
to look at the new patch set (#4).
Change subject: mediatek/mt8183: modify vsim2 calibration ......................................................................
mediatek/mt8183: modify vsim2 calibration
The default voltage of vsim2 is set to 2.76V for sim card usage. However, this power may be used for other usage, so we set it to 2.7V with modifying calibration value.
BUG=b:126139364 BRANCH=none TEST=measure vsim2 voltage with multimeter
Change-Id: I4dffdde89cbde91286d92e6c2b445f0b3d0ad2fe Signed-off-by: Hsin-Hsiung Wang hsin-hsiung.wang@mediatek.com --- M src/soc/mediatek/mt8183/include/soc/mt6358.h M src/soc/mediatek/mt8183/mt6358.c 2 files changed, 16 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/32057/4
hsin-hsiung wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32057 )
Change subject: mediatek/mt8183: modify vsim2 calibration ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/32057/3/src/soc/mediatek/mt8183/mt6358.c File src/soc/mediatek/mt8183/mt6358.c:
https://review.coreboot.org/#/c/32057/3/src/soc/mediatek/mt8183/mt6358.c@782 PS3, Line 782: VSIM2_CALI_POS_60_MV
And line 781-784 looks equivalent to 'vsim2_cali = MAX(0, vsim2_cali - 6);' […]
Done
Hello Julius Werner, You-Cheng Syu, Tristan Hsieh, Hung-Te Lin, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32057
to look at the new patch set (#5).
Change subject: mediatek/mt8183: Calibrate vsim2 to 2.7 V ......................................................................
mediatek/mt8183: Calibrate vsim2 to 2.7 V
The default voltage of vsim2 is set to 2.76V for sim card usage. However, this power may be used for other usage, so we set it to 2.7V with modifying calibration value.
BUG=b:126139364 BRANCH=none TEST=measure vsim2 voltage with multimeter
Change-Id: I4dffdde89cbde91286d92e6c2b445f0b3d0ad2fe Signed-off-by: Hsin-Hsiung Wang hsin-hsiung.wang@mediatek.com --- M src/soc/mediatek/mt8183/include/soc/mt6358.h M src/soc/mediatek/mt8183/mt6358.c 2 files changed, 16 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/32057/5
hsin-hsiung wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32057 )
Change subject: mediatek/mt8183: Calibrate vsim2 to 2.7 V ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/#/c/32057/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32057/1//COMMIT_MSG@7 PS1, Line 7: mediatek/mt8183: modify vsim2 calibration
Please be more specific in the summary. Maybe: […]
Done
https://review.coreboot.org/#/c/32057/1//COMMIT_MSG@9 PS1, Line 9: This patch modifies vsim2 calibration to meet 2.7V
- Please add a dot/period at the end of sentences. […]
Done
https://review.coreboot.org/#/c/32057/1/src/soc/mediatek/mt8183/include/soc/... File src/soc/mediatek/mt8183/include/soc/mt6358.h:
https://review.coreboot.org/#/c/32057/1/src/soc/mediatek/mt8183/include/soc/... PS1, Line 30: PMIC_VSIM2_ANA_CON0 = 0x1c34,
Use spaces for alignment as above.
Done
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32057 )
Change subject: mediatek/mt8183: Calibrate vsim2 to 2.7 V ......................................................................
Patch Set 5: Code-Review+1
@Julius are you ok with the final version?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32057 )
Change subject: mediatek/mt8183: Calibrate vsim2 to 2.7 V ......................................................................
Patch Set 5: Code-Review+2
(1 comment)
I don't really understand what this does. Code-wise it seems fine.
https://review.coreboot.org/#/c/32057/5/src/soc/mediatek/mt8183/mt6358.c File src/soc/mediatek/mt8183/mt6358.c:
https://review.coreboot.org/#/c/32057/5/src/soc/mediatek/mt8183/mt6358.c@786 PS5, Line 786: vsim2_cali_0mv = (vsim2_cali_0mv > 6) ? (vsim2_cali_0mv - 6) : 0; I assume we've somehow made sure that this cannot run twice and subtract the value twice? Does this code guarantee that the PMIC will be reset and all registers return to power-on values on every boot? (Please don't just base this of Kukui-specific board design... this code should do the right thing for all possible MT8183 boards.)
Hello Julius Werner, You-Cheng Syu, Tristan Hsieh, Hung-Te Lin, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32057
to look at the new patch set (#6).
Change subject: mediatek/mt8183: Calibrate vsim2 to 2.7 V ......................................................................
mediatek/mt8183: Calibrate vsim2 to 2.7 V
The default voltage of vsim2 is set to 2.76V for sim card usage. However, this power may be used for other usage, so we set it to 2.7V with modifying calibration value.
BUG=b:126139364 BRANCH=none TEST=measure vsim2 voltage with multimeter
Change-Id: I4dffdde89cbde91286d92e6c2b445f0b3d0ad2fe Signed-off-by: Hsin-Hsiung Wang hsin-hsiung.wang@mediatek.com --- M src/mainboard/google/kukui/romstage.c M src/soc/mediatek/mt8183/include/soc/mt6358.h M src/soc/mediatek/mt8183/mt6358.c 3 files changed, 17 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/32057/6
hsin-hsiung wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32057 )
Change subject: mediatek/mt8183: Calibrate vsim2 to 2.7 V ......................................................................
Patch Set 7: Code-Review+1
(1 comment)
https://review.coreboot.org/#/c/32057/5/src/soc/mediatek/mt8183/mt6358.c File src/soc/mediatek/mt8183/mt6358.c:
https://review.coreboot.org/#/c/32057/5/src/soc/mediatek/mt8183/mt6358.c@786 PS5, Line 786: vsim2_cali_0mv = (vsim2_cali_0mv > 6) ? (vsim2_cali_0mv - 6) : 0;
I assume we've somehow made sure that this cannot run twice and subtract the value twice? Does this […]
Hi Julius, I think you're right. The patch is base on kukui board design, so I move the vsim2 calibration to platform initialization. For your question, PMIC will be reset to default voltage on every boot.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32057 )
Change subject: mediatek/mt8183: Calibrate vsim2 to 2.7 V ......................................................................
Patch Set 7: Code-Review+2
(1 comment)
https://review.coreboot.org/#/c/32057/7/src/soc/mediatek/mt8183/include/soc/... File src/soc/mediatek/mt8183/include/soc/mt6358.h:
https://review.coreboot.org/#/c/32057/7/src/soc/mediatek/mt8183/include/soc/... PS7, Line 42: void pmic_set_vsim2_cali(void); I still have no idea what this does and which boards would need it vs which don't. An explaining comment that details what this does and what in the board design makes it necessary would be great.
Hello Julius Werner, You-Cheng Syu, Tristan Hsieh, Hung-Te Lin, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32057
to look at the new patch set (#8).
Change subject: mediatek/mt8183: Calibrate vsim2 to 2.7 V ......................................................................
mediatek/mt8183: Calibrate vsim2 to 2.7 V
The default voltage of vsim2 is set to 2.76V for sim card usage. However, vsim2 is used for the tx_ovdd power of display port IT6505 on the kukui board design which needs 2.7V. So we set it to 2.7V with modifying calibration value.
BUG=b:126139364 BRANCH=none TEST=measure vsim2 voltage with multimeter
Change-Id: I4dffdde89cbde91286d92e6c2b445f0b3d0ad2fe Signed-off-by: Hsin-Hsiung Wang hsin-hsiung.wang@mediatek.com --- M src/mainboard/google/kukui/romstage.c M src/soc/mediatek/mt8183/include/soc/mt6358.h M src/soc/mediatek/mt8183/mt6358.c 3 files changed, 17 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/32057/8
hsin-hsiung wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32057 )
Change subject: mediatek/mt8183: Calibrate vsim2 to 2.7 V ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/#/c/32057/7/src/soc/mediatek/mt8183/include/soc/... File src/soc/mediatek/mt8183/include/soc/mt6358.h:
https://review.coreboot.org/#/c/32057/7/src/soc/mediatek/mt8183/include/soc/... PS7, Line 42: void pmic_set_vsim2_cali(void);
I still have no idea what this does and which boards would need it vs which don't. […]
Hi Julius, I update the comment message. Are there any other things that need to be clarified?
Hello Julius Werner, You-Cheng Syu, Tristan Hsieh, Hung-Te Lin, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32057
to look at the new patch set (#9).
Change subject: mediatek/mt8183: Calibrate vsim2 to 2.7 V ......................................................................
mediatek/mt8183: Calibrate vsim2 to 2.7 V
The default voltage of vsim2 is set to 2.76V for sim card usage. In general, 2.76V of vsim2 is composed of 2.7V main voltage and 0.06 calibration voltage. However, vsim2 is used for the tx_ovdd power of display port IT6505 on the kukui board design which needs 2.7V. So we set it to 2.7V with modifying calibration value.
BUG=b:126139364 BRANCH=none TEST=measure vsim2 voltage with multimeter
Change-Id: I4dffdde89cbde91286d92e6c2b445f0b3d0ad2fe Signed-off-by: Hsin-Hsiung Wang hsin-hsiung.wang@mediatek.com --- M src/mainboard/google/kukui/romstage.c M src/soc/mediatek/mt8183/include/soc/mt6358.h M src/soc/mediatek/mt8183/mt6358.c 3 files changed, 17 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/32057/9
Hello Julius Werner, You-Cheng Syu, Tristan Hsieh, Hung-Te Lin, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32057
to look at the new patch set (#10).
Change subject: mediatek/mt8183: Calibrate vsim2 to 2.7 V ......................................................................
mediatek/mt8183: Calibrate vsim2 to 2.7 V
The default voltage of vsim2 is set to 2.76V for sim card usage. In general, 2.76V of vsim2 is composed of 2.7V main voltage and 0.06V calibration voltage. However, vsim2 is used for the tx_ovdd power of display port IT6505 on the kukui board design which needs 2.7V. So we set it to 2.7V with modifying calibration value.
BUG=b:126139364 BRANCH=none TEST=measure vsim2 voltage with multimeter
Change-Id: I4dffdde89cbde91286d92e6c2b445f0b3d0ad2fe Signed-off-by: Hsin-Hsiung Wang hsin-hsiung.wang@mediatek.com --- M src/mainboard/google/kukui/romstage.c M src/soc/mediatek/mt8183/include/soc/mt6358.h M src/soc/mediatek/mt8183/mt6358.c 3 files changed, 17 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/32057/10
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32057 )
Change subject: mediatek/mt8183: Calibrate vsim2 to 2.7 V ......................................................................
Patch Set 10:
(3 comments)
https://review.coreboot.org/#/c/32057/7/src/mainboard/google/kukui/romstage.... File src/mainboard/google/kukui/romstage.c:
https://review.coreboot.org/#/c/32057/7/src/mainboard/google/kukui/romstage.... PS7, Line 32: pmic_set_vsim2_cali(); There should be a short code comment here why this is necessary, e.g.
/* Adjust VSIM2 down to 2.7V because it is shared with IT6505. */
https://review.coreboot.org/#/c/32057/7/src/soc/mediatek/mt8183/include/soc/... File src/soc/mediatek/mt8183/include/soc/mt6358.h:
https://review.coreboot.org/#/c/32057/7/src/soc/mediatek/mt8183/include/soc/... PS7, Line 42: void pmic_set_vsim2_cali(void);
Hi Julius, […]
I guess the hardcoding of 0.06 still feels a bit weird? Maybe this should be pmic_adjust_vsim2_cali(int millivolts), and then you can call it with -60 for Kukui. Or maybe pmic_set_vsim2(2700).
https://review.coreboot.org/#/c/32057/10/src/soc/mediatek/mt8183/mt6358.c File src/soc/mediatek/mt8183/mt6358.c:
https://review.coreboot.org/#/c/32057/10/src/soc/mediatek/mt8183/mt6358.c@74... PS10, Line 745: vsim2_cali_0mv = (vsim2_cali_0mv > 6) ? (vsim2_cali_0mv - 6) : 0; nit: I think writing this as
vsim2_cali_0mv = MIN(vsim2_cali_0mv - 6, 0);
would be more readable.
Hello Julius Werner, You-Cheng Syu, Tristan Hsieh, Hung-Te Lin, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32057
to look at the new patch set (#11).
Change subject: mediatek/mt8183: Calibrate vsim2 to 2.7 V ......................................................................
mediatek/mt8183: Calibrate vsim2 to 2.7 V
The default voltage of vsim2 is set to 2.76V for sim card usage. In general, 2.76V of vsim2 is composed of 2.7V main voltage and 0.06V calibration voltage. However, vsim2 is used for the tx_ovdd power of display port IT6505 on the kukui board design which needs 2.7V. So we set it to 2.7V with modifying calibration value.
BUG=b:126139364 BRANCH=none TEST=measure vsim2 voltage with multimeter
Change-Id: I4dffdde89cbde91286d92e6c2b445f0b3d0ad2fe Signed-off-by: Hsin-Hsiung Wang hsin-hsiung.wang@mediatek.com --- M src/mainboard/google/kukui/romstage.c M src/soc/mediatek/mt8183/include/soc/mt6358.h M src/soc/mediatek/mt8183/mt6358.c 3 files changed, 74 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/32057/11
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32057 )
Change subject: mediatek/mt8183: Calibrate vsim2 to 2.7 V ......................................................................
Patch Set 11:
(16 comments)
https://review.coreboot.org/#/c/32057/11/src/soc/mediatek/mt8183/mt6358.c File src/soc/mediatek/mt8183/mt6358.c:
https://review.coreboot.org/#/c/32057/11/src/soc/mediatek/mt8183/mt6358.c@73... PS11, Line 732: {0x14A6, 0x10, 0x7f, 0}, please, no spaces at the start of a line
https://review.coreboot.org/#/c/32057/11/src/soc/mediatek/mt8183/mt6358.c@73... PS11, Line 734: {0x14A6, 0x10, 0x7f, 8}, please, no spaces at the start of a line
https://review.coreboot.org/#/c/32057/11/src/soc/mediatek/mt8183/mt6358.c@73... PS11, Line 737: {0x14A4, 0x1, 0x3, 0}, please, no spaces at the start of a line
https://review.coreboot.org/#/c/32057/11/src/soc/mediatek/mt8183/mt6358.c@73... PS11, Line 739: {0x1BC6, 0x38, 0x7f, 0}, please, no spaces at the start of a line
https://review.coreboot.org/#/c/32057/11/src/soc/mediatek/mt8183/mt6358.c@74... PS11, Line 741: {0x1BC6, 0x38, 0x7f, 8}, please, no spaces at the start of a line
https://review.coreboot.org/#/c/32057/11/src/soc/mediatek/mt8183/mt6358.c@74... PS11, Line 744: {0x1BC4, 0x1, 0x3, 0}, please, no spaces at the start of a line
https://review.coreboot.org/#/c/32057/11/src/soc/mediatek/mt8183/mt6358.c@74... PS11, Line 746: {0x134, 0x1, 0x1, 4}, please, no spaces at the start of a line
https://review.coreboot.org/#/c/32057/11/src/soc/mediatek/mt8183/mt6358.c@85... PS11, Line 850: printk(BIOS_ERR, "[%s] Bingo 0x%x = 0x%x\n", __func__, 0x1426, pwrap_read_field(0x1426, 0xFFFF, 0)); line over 80 characters
https://review.coreboot.org/#/c/32057/11/src/soc/mediatek/mt8183/mt6358.c@85... PS11, Line 851: printk(BIOS_ERR, "[%s] Bingo 0x%x = 0x%x\n", __func__, 0x141e, pwrap_read_field(0x141e, 0xFFFF, 0)); line over 80 characters
https://review.coreboot.org/#/c/32057/11/src/soc/mediatek/mt8183/mt6358.c@85... PS11, Line 855: printk(BIOS_ERR, "[%s] SCP Bingo 0x%x = 0x%x\n", __func__, 0x1426, pwrap_read_field(0x1426, 0xFFFF, 0)); line over 80 characters
https://review.coreboot.org/#/c/32057/11/src/soc/mediatek/mt8183/mt6358.c@85... PS11, Line 856: printk(BIOS_ERR, "[%s] SCP Bingo 0x%x = 0x%x\n", __func__, 0x141e, pwrap_read_field(0x141e, 0xFFFF, 0)); line over 80 characters
https://review.coreboot.org/#/c/32057/11/src/soc/mediatek/mt8183/mt6358.c@85... PS11, Line 857: printk(BIOS_ERR, "[%s] SCP Bingo 0x%x = 0x%x\n", __func__, 0x14a4, pwrap_read_field(0x14a4, 0xFFFF, 0)); line over 80 characters
https://review.coreboot.org/#/c/32057/11/src/soc/mediatek/mt8183/mt6358.c@85... PS11, Line 858: printk(BIOS_ERR, "[%s] SCP Bingo 0x%x = 0x%x\n", __func__, 0x14a6, pwrap_read_field(0x14a6, 0xFFFF, 0)); line over 80 characters
https://review.coreboot.org/#/c/32057/11/src/soc/mediatek/mt8183/mt6358.c@85... PS11, Line 859: printk(BIOS_ERR, "[%s] SCP Bingo 0x%x = 0x%x\n", __func__, 0x1bc4, pwrap_read_field(0x1bc4, 0xFFFF, 0)); line over 80 characters
https://review.coreboot.org/#/c/32057/11/src/soc/mediatek/mt8183/mt6358.c@86... PS11, Line 860: printk(BIOS_ERR, "[%s] SCP Bingo 0x%x = 0x%x\n", __func__, 0x1bc6, pwrap_read_field(0x1bc6, 0xFFFF, 0)); line over 80 characters
https://review.coreboot.org/#/c/32057/11/src/soc/mediatek/mt8183/mt6358.c@86... PS11, Line 861: printk(BIOS_ERR, "[%s] SCP Bingo 0x%x = 0x%x\n", __func__, 0x134, pwrap_read_field(0x134, 0xFFFF, 0)); line over 80 characters
Hello Julius Werner, You-Cheng Syu, Tristan Hsieh, Hung-Te Lin, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32057
to look at the new patch set (#12).
Change subject: mediatek/mt8183: Calibrate vsim2 to 2.7 V ......................................................................
mediatek/mt8183: Calibrate vsim2 to 2.7 V
The default voltage of vsim2 is set to 2.76V for sim card usage. In general, 2.76V of vsim2 is composed of 2.7V main voltage and 0.06V calibration voltage. However, vsim2 is used for the tx_ovdd power of display port IT6505 on the kukui board design which needs 2.7V. So we set it to 2.7V with modifying calibration value.
BUG=b:126139364 BRANCH=none TEST=measure vsim2 voltage with multimeter
Change-Id: I4dffdde89cbde91286d92e6c2b445f0b3d0ad2fe Signed-off-by: Hsin-Hsiung Wang hsin-hsiung.wang@mediatek.com --- M src/mainboard/google/kukui/romstage.c M src/soc/mediatek/mt8183/include/soc/mt6358.h M src/soc/mediatek/mt8183/mt6358.c 3 files changed, 45 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/32057/12
Hello Julius Werner, You-Cheng Syu, Tristan Hsieh, Hung-Te Lin, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32057
to look at the new patch set (#13).
Change subject: mediatek/mt8183: Calibrate vsim2 to 2.7 V ......................................................................
mediatek/mt8183: Calibrate vsim2 to 2.7 V
The default voltage of vsim2 is set to 2.76V for sim card usage. In general, 2.76V of vsim2 is composed of 2.7V main voltage and 0.06V calibration voltage. However, vsim2 is used for the tx_ovdd power of display port IT6505 on the kukui board design which needs 2.7V. So we set it to 2.7V with modifying calibration value.
BUG=b:126139364 BRANCH=none TEST=measure vsim2 voltage with multimeter
Change-Id: I4dffdde89cbde91286d92e6c2b445f0b3d0ad2fe Signed-off-by: Hsin-Hsiung Wang hsin-hsiung.wang@mediatek.com --- M src/mainboard/google/kukui/romstage.c M src/soc/mediatek/mt8183/include/soc/mt6358.h M src/soc/mediatek/mt8183/mt6358.c 3 files changed, 45 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/32057/13
Hello Julius Werner, You-Cheng Syu, Tristan Hsieh, Hung-Te Lin, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32057
to look at the new patch set (#14).
Change subject: mediatek/mt8183: Calibrate vsim2 to 2.7 V ......................................................................
mediatek/mt8183: Calibrate vsim2 to 2.7 V
The default voltage of vsim2 is set to 2.76V for sim card usage. In general, 2.76V of vsim2 is composed of 2.7V main voltage and 0.06V calibration voltage. However, vsim2 is used for the tx_ovdd power of display port IT6505 on the kukui board design which needs 2.7V. So we set it to 2.7V with modifying calibration value.
BUG=b:126139364 BRANCH=none TEST=measure vsim2 voltage with multimeter
Change-Id: I4dffdde89cbde91286d92e6c2b445f0b3d0ad2fe Signed-off-by: Hsin-Hsiung Wang hsin-hsiung.wang@mediatek.com --- M src/mainboard/google/kukui/romstage.c M src/soc/mediatek/mt8183/include/soc/mt6358.h M src/soc/mediatek/mt8183/mt6358.c 3 files changed, 45 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/32057/14
hsin-hsiung wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32057 )
Change subject: mediatek/mt8183: Calibrate vsim2 to 2.7 V ......................................................................
Patch Set 14: Code-Review+1
(1 comment)
https://review.coreboot.org/#/c/32057/10/src/soc/mediatek/mt8183/mt6358.c File src/soc/mediatek/mt8183/mt6358.c:
https://review.coreboot.org/#/c/32057/10/src/soc/mediatek/mt8183/mt6358.c@74... PS10, Line 745: vsim2_cali_0mv = (vsim2_cali_0mv > 6) ? (vsim2_cali_0mv - 6) : 0;
nit: I think writing this as […]
Hi Julius, After checking the calibration register with our chip designer, I have a misunderstanding about it. Please help review the patch again, thanks.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32057 )
Change subject: mediatek/mt8183: Calibrate vsim2 to 2.7 V ......................................................................
Patch Set 14:
(3 comments)
Hmm... well... I'm still somewhat confused. Your patch changed behavior now. Previously you were reading the current value and subtracting 6, now you're just overwriting the value. I thought you were doing that because you didn't know what the existing calibration value was (e.g. it might be different per board and initialized from fuses or something), so you could only adjust the existing one.
But if you are sure that setting it directly is okay then this approach is fine.
https://review.coreboot.org/#/c/32057/14/src/soc/mediatek/mt8183/mt6358.c File src/soc/mediatek/mt8183/mt6358.c:
https://review.coreboot.org/#/c/32057/14/src/soc/mediatek/mt8183/mt6358.c@73... PS14, Line 736: unsigned int vsim2_mv, unsigned int cali_mv If these voltages really just get added like that, does it make sense to have two separate parameters? Why not just have one millivolts parameter and then do something like
assert(millivolts % 10 == 0); int cali_mv = millivolts % 100; switch (millivolts - cali_mv) { ... }
https://review.coreboot.org/#/c/32057/14/src/soc/mediatek/mt8183/mt6358.c@76... PS14, Line 762: return; This should die() or assert()
https://review.coreboot.org/#/c/32057/14/src/soc/mediatek/mt8183/mt6358.c@76... PS14, Line 768: if (cali_mv > 100) You should assert() that the value is in range instead
Hello Julius Werner, You-Cheng Syu, Tristan Hsieh, Hung-Te Lin, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32057
to look at the new patch set (#15).
Change subject: mediatek/mt8183: Calibrate vsim2 to 2.7 V ......................................................................
mediatek/mt8183: Calibrate vsim2 to 2.7 V
The default voltage of vsim2 is set to 2.76V for sim card usage. In general, 2.76V of vsim2 is composed of 2.7V main voltage and 0.06V calibration voltage. However, vsim2 is used for the tx_ovdd power of display port IT6505 on the kukui board design which needs 2.7V. So we set it to 2.7V with modifying calibration value.
BUG=b:126139364 BRANCH=none TEST=measure vsim2 voltage with multimeter
Change-Id: I4dffdde89cbde91286d92e6c2b445f0b3d0ad2fe Signed-off-by: Hsin-Hsiung Wang hsin-hsiung.wang@mediatek.com --- M src/mainboard/google/kukui/romstage.c M src/soc/mediatek/mt8183/include/soc/mt6358.h M src/soc/mediatek/mt8183/mt6358.c 3 files changed, 45 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/32057/15
hsin-hsiung wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32057 )
Change subject: mediatek/mt8183: Calibrate vsim2 to 2.7 V ......................................................................
Patch Set 15:
Patch Set 14:
(3 comments)
Hmm... well... I'm still somewhat confused. Your patch changed behavior now. Previously you were reading the current value and subtracting 6, now you're just overwriting the value. I thought you were doing that because you didn't know what the existing calibration value was (e.g. it might be different per board and initialized from fuses or something), so you could only adjust the existing one.
But if you are sure that setting it directly is okay then this approach is fine.
Hi, Julius I have a misunderstanding about the calibration of pmic mt6358 before and it is correct now. I update the patch for your comments, please let me know if there is any problem. Many thanks for your reviewing.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32057 )
Change subject: mediatek/mt8183: Calibrate vsim2 to 2.7 V ......................................................................
Patch Set 15: Code-Review+2
hsin-hsiung wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32057 )
Change subject: mediatek/mt8183: Calibrate vsim2 to 2.7 V ......................................................................
Patch Set 15: Code-Review+1
Julius Werner has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32057 )
Change subject: mediatek/mt8183: Calibrate vsim2 to 2.7 V ......................................................................
mediatek/mt8183: Calibrate vsim2 to 2.7 V
The default voltage of vsim2 is set to 2.76V for sim card usage. In general, 2.76V of vsim2 is composed of 2.7V main voltage and 0.06V calibration voltage. However, vsim2 is used for the tx_ovdd power of display port IT6505 on the kukui board design which needs 2.7V. So we set it to 2.7V with modifying calibration value.
BUG=b:126139364 BRANCH=none TEST=measure vsim2 voltage with multimeter
Change-Id: I4dffdde89cbde91286d92e6c2b445f0b3d0ad2fe Signed-off-by: Hsin-Hsiung Wang hsin-hsiung.wang@mediatek.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/32057 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Julius Werner jwerner@chromium.org --- M src/mainboard/google/kukui/romstage.c M src/soc/mediatek/mt8183/include/soc/mt6358.h M src/soc/mediatek/mt8183/mt6358.c 3 files changed, 45 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved hsin-hsiung wang: Looks good to me, but someone else must approve
diff --git a/src/mainboard/google/kukui/romstage.c b/src/mainboard/google/kukui/romstage.c index baaca43..1465243 100644 --- a/src/mainboard/google/kukui/romstage.c +++ b/src/mainboard/google/kukui/romstage.c @@ -29,6 +29,8 @@ mainboard_early_init();
mt6358_init(); + /* Adjust VSIM2 down to 2.7V because it is shared with IT6505. */ + pmic_set_vsim2_cali(2700); mt_pll_raise_ca53_freq(1989 * MHz); rtc_boot(); mt_mem_init(get_sdram_config()); diff --git a/src/soc/mediatek/mt8183/include/soc/mt6358.h b/src/soc/mediatek/mt8183/include/soc/mt6358.h index 02937ba..277ee9a 100644 --- a/src/soc/mediatek/mt8183/include/soc/mt6358.h +++ b/src/soc/mediatek/mt8183/include/soc/mt6358.h @@ -27,6 +27,7 @@ PMIC_CPSDSA4 = 0x0a2e, PMIC_VDRAM1_VOSEL_SLEEP = 0x160a, PMIC_SMPS_ANA_CON0 = 0x1808, + PMIC_VSIM2_ANA_CON0 = 0x1e30, };
struct pmic_setting { @@ -38,5 +39,6 @@
void mt6358_init(void); void pmic_set_power_hold(bool enable); +void pmic_set_vsim2_cali(unsigned int vsim2_mv);
#endif /* __SOC_MEDIATEK_MT6358_H__ */ diff --git a/src/soc/mediatek/mt8183/mt6358.c b/src/soc/mediatek/mt8183/mt6358.c index 7054243..ea4274f 100644 --- a/src/soc/mediatek/mt8183/mt6358.c +++ b/src/soc/mediatek/mt8183/mt6358.c @@ -13,6 +13,7 @@ * GNU General Public License for more details. */
+#include <assert.h> #include <console/console.h> #include <soc/pmic_wrap.h> #include <soc/mt6358.h> @@ -733,6 +734,46 @@ pwrap_write_field(PMIC_PWRHOLD, (enable) ? 1 : 0, 0x1, 0); }
+void pmic_set_vsim2_cali(unsigned int vsim2_mv) +{ + u16 vsim2_reg, cali_mv; + + cali_mv = vsim2_mv % 100; + assert(cali_mv % 10 == 0); + + switch (vsim2_mv - cali_mv) { + case 1700: + vsim2_reg = 0x3; + break; + + case 1800: + vsim2_reg = 0x4; + break; + + case 2700: + vsim2_reg = 0x8; + break; + + case 3000: + vsim2_reg = 0xb; + break; + + case 3100: + vsim2_reg = 0xc; + break; + + default: + assert(0); + return; + }; + + /* [11:8]=0x8, RG_VSIM2_VOSEL */ + pwrap_write_field(PMIC_VSIM2_ANA_CON0, vsim2_reg, 0xF, 8); + + /* [3:0], RG_VSIM2_VOCAL */ + pwrap_write_field(PMIC_VSIM2_ANA_CON0, cali_mv / 10, 0xF, 0); +} + static void pmic_wdt_set(void) { /* [5]=1, RG_WDTRSTB_DEB */
-
build bot (Jenkins) (Code Review) -
hsin-hsiung wang (Code Review)
-
Hung-Te Lin (Code Review)
-
Julius Werner (Code Review)
-
Paul Menzel (Code Review)
-
You-Cheng Syu (Code Review)