Change in coreboot[master]: WIP: soc/mediatek/mt8192: Unlock key protection before initial setting

[hsin-hsiung wang (Code Review)]

hsin-hsiung wang has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/48954 )

Change subject: WIP: soc/mediatek/mt8192: Unlock key protection before initial setting
......................................................................

WIP: soc/mediatek/mt8192: Unlock key protection before initial setting

We need unlock key protection so that some initial settings could be effective.
After applying initial settings, we would enable the key protection again.

BUG=b:172636735
BRANCH=none
TEST=boot asurada correctly

Signed-off-by: Hsin-Hsiung Wang 
Change-Id: I593d4e02bf0b62ac297957caf4ae1c1837f1f38d
---
M src/soc/mediatek/mt8192/mt6359p.c
1 file changed, 33 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/48954/1

diff --git a/src/soc/mediatek/mt8192/mt6359p.c b/src/soc/mediatek/mt8192/mt6359p.c
index a8e93ca..e6e9d36 100644
--- a/src/soc/mediatek/mt8192/mt6359p.c
+++ b/src/soc/mediatek/mt8192/mt6359p.c
@@ -6,6 +6,25 @@
 #include <soc/pmif.h>
 #include <soc/mt6359p.h>

+static const struct pmic_setting lock_key_setting[] = {
+	/* MT6359_TOP_TMA_KEY[0x3A8], for dram */
+	{0x3A8, 0x9CA6, 0xFFFF, 0},
+	/* MT6359_SPISLV_KEY[0x44A], for spi */
+	{0x44A, 0xBADE, 0xFFFF, 0},
+	/* MT6359_CPSWKEY[0xA3A], for power-off seq. */
+	{0xA3A, 0x4729, 0xFFFF, 0},
+	/* MT6359_BM_WKEY0[0xC58], for fuel gauge adc */
+	{0xC58, 0x1605, 0xFFFF, 0},
+	/* MT6359_BM_WKEY1[0xC5A], for battery temp adc */
+	{0xC5A, 0x1706, 0xFFFF, 0},
+	/* MT6359_BM_WKEY2[0xC5C], for bif */
+	{0xC5C, 0x1807, 0xFFFF, 0},
+	/* MT6359_HK_TOP_WKEY[0xFB4], for auxadc */
+	{0xFB4, 0x6359, 0xFFFF, 0},
+	/* MT6359_BUCK_TOP_KEY_PROT[0x1432], for buck */
+	{0x1432, 0x5543, 0xFFFF, 0},
+};
+
 static const struct pmic_setting init_setting[] = {
    {0x20, 0xA, 0xA, 0},
    {0x24, 0x1F00, 0x1F00, 0},
@@ -306,6 +325,18 @@
    mt6359p_write_field(PMIC_TOP_RST_MISC_SET, 0x01, 0xFFFF, 0);
 }

+
+static void pmic_protect_key_set(bool lock)
+{
+	if (lock) {
+		for (int i = 0; i < ARRAY_SIZE(lock_key_setting); i++)
+			mt6359p_write(lock_key_setting[i].addr, 0);
+	} else {
+		for (int i = 0; i < ARRAY_SIZE(lock_key_setting); i++)
+			mt6359p_write(lock_key_setting[i].addr, lock_key_setting[i].val);
+	}
+}
+
 static void pmic_init_setting(void)
 {
    for (int i = 0; i < ARRAY_SIZE(init_setting); i++)
@@ -437,8 +468,10 @@
    init_pmif_arb();
    pmic_set_power_hold();
    pmic_wdt_set();
+	pmic_protect_key_set(false);
    pmic_init_setting();
    pmic_lp_setting();
+	pmic_protect_key_set(true);
    pmic_wk_vs2_voter_setting();
 }

[hsin-hsiung wang (Code Review)]

Hello Hung-Te Lin, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/48954

to look at the new patch set (#2).

Change subject: WIP: soc/mediatek/mt8192: Unlock key protection before initial setting
......................................................................

WIP: soc/mediatek/mt8192: Unlock key protection before initial setting

We need unlock key protection so that some initial settings could be effective.
After applying initial settings, we would enable the key protection again.

BUG=b:172636735
BRANCH=none
TEST=boot asurada correctly

Signed-off-by: Hsin-Hsiung Wang 
Change-Id: I593d4e02bf0b62ac297957caf4ae1c1837f1f38d
---
M src/soc/mediatek/mt8192/mt6359p.c
1 file changed, 32 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/48954/2

[hsin-hsiung wang (Code Review)]

Hello Hung-Te Lin, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/48954

to look at the new patch set (#3).

Change subject: WIP: soc/mediatek/mt8192: Unlock key protection before initial setting
......................................................................

WIP: soc/mediatek/mt8192: Unlock key protection before initial setting

We need unlock key protection so that some initial settings could be effective.
After applying initial settings, we would enable the key protection again.

BUG=b:172636735
BRANCH=none
TEST=boot asurada correctly

Signed-off-by: Hsin-Hsiung Wang 
Change-Id: I593d4e02bf0b62ac297957caf4ae1c1837f1f38d
---
M src/soc/mediatek/mt8192/mt6359p.c
1 file changed, 24 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/48954/3

[hsin-hsiung wang (Code Review)]

Hello Hung-Te Lin, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/48954

to look at the new patch set (#4).

Change subject: WIP: soc/mediatek/mt8192: Unlock key protection before initial setting
......................................................................

WIP: soc/mediatek/mt8192: Unlock key protection before initial setting

We need unlock key protection so that some initial settings could be effective.
After applying initial settings, we would enable the key protection again.

BUG=b:172636735
BRANCH=none
TEST=boot asurada correctly

Signed-off-by: Hsin-Hsiung Wang 
Change-Id: I593d4e02bf0b62ac297957caf4ae1c1837f1f38d
---
M src/soc/mediatek/mt8192/mt6359p.c
1 file changed, 24 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/48954/4

Change in coreboot[master]: WIP: soc/mediatek/mt8192: pmic: unlock key protection before initial ...

[hsin-hsiung wang (Code Review)]

Hello Hung-Te Lin, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/48954

to look at the new patch set (#5).

Change subject: WIP: soc/mediatek/mt8192: pmic: unlock key protection before initial setting
......................................................................

WIP: soc/mediatek/mt8192: pmic: unlock key protection before initial setting

We need unlock key protection so that some initial settings could be effective.
After applying initial settings, we would enable the key protection again.

BUG=b:172636735
BRANCH=none
TEST=boot asurada correctly

Signed-off-by: Hsin-Hsiung Wang 
Change-Id: I593d4e02bf0b62ac297957caf4ae1c1837f1f38d
---
M src/soc/mediatek/mt8192/mt6359p.c
1 file changed, 24 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/48954/5

Change in coreboot[master]: soc/mediatek/mt8192: pmic: unlock key protection before initial setting

[hsin-hsiung wang (Code Review)]

Hello Hung-Te Lin, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/48954

to look at the new patch set (#6).

Change subject: soc/mediatek/mt8192: pmic: unlock key protection before initial setting
......................................................................

soc/mediatek/mt8192: pmic: unlock key protection before initial setting

We need unlock key protection so that some initial settings could be effective.
After applying initial settings, we would enable the key protection again.

BUG=b:172636735
BRANCH=none
TEST=boot asurada correctly

Signed-off-by: Hsin-Hsiung Wang 
Change-Id: I593d4e02bf0b62ac297957caf4ae1c1837f1f38d
---
M src/soc/mediatek/mt8192/mt6359p.c
1 file changed, 24 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/48954/6

Change in coreboot[master]: soc/mediatek/mt8192: pmic: unlock key protection before initial setting

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48954 )

Change subject: soc/mediatek/mt8192: pmic: unlock key protection before initial setting
......................................................................

Patch Set 6:

(5 comments)

https://review.coreboot.org/c/coreboot/+/48954/6//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/48954/6//COMMIT_MSG@9 
PS6, Line 9: initial settings could be effective
Which ones?

https://review.coreboot.org/c/coreboot/+/48954/6//COMMIT_MSG@9 
PS6, Line 9: unlock key protection
1.  Does that mean *disable key protection*?
2.  What keys are protected?

https://review.coreboot.org/c/coreboot/+/48954/6//COMMIT_MSG@11 
PS6, Line 11: 
Please mention the datasheet name and revision.

https://review.coreboot.org/c/coreboot/+/48954/6/src/soc/mediatek/mt8192/mt6... 
File src/soc/mediatek/mt8192/mt6359p.c:

https://review.coreboot.org/c/coreboot/+/48954/6/src/soc/mediatek/mt8192/mt6... 
PS6, Line 321: set
I would stay with *setting*.

https://review.coreboot.org/c/coreboot/+/48954/6/src/soc/mediatek/mt8192/mt6... 
PS6, Line 329: 	}
I’d put the for loop outside, and in the body use the ternary operator:

for (int i = 0; i < ARRAY_SIZE(key_protect_setting); i++)
        mt6359p_write(key_protect_setting[i].addr, lock ? 0 : key_protect_setting[i].val);

Change in coreboot[master]: soc/mediatek/mt8192: pmic: unlock key protection before initial setting

[Hung-Te Lin (Code Review)]

Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48954 )

Change subject: soc/mediatek/mt8192: pmic: unlock key protection before initial setting
......................................................................

Patch Set 8: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/48954/8/src/soc/mediatek/mt8192/mt6... 
File src/soc/mediatek/mt8192/mt6359p.c:

https://review.coreboot.org/c/coreboot/+/48954/8/src/soc/mediatek/mt8192/mt6... 
PS8, Line 323: 	if (lock) {
             : 		for (int i = 0; i < ARRAY_SIZE(key_protect_setting); i++)
             : 			mt6359p_write(key_protect_setting[i].addr, 0);
             : 	} else {
             : 		for (int i = 0; i < ARRAY_SIZE(key_protect_setting); i++)
             : 			mt6359p_write(key_protect_setting[i].addr, key_protect_setting[i].val);
             : 	}
I'd rather do

for (int i ...
    mt6359p_write(...addr, lock ? key_protect_settings[i].val : 0);

Change in coreboot[master]: soc/mediatek/mt8192: pmic: unlock key protection before initial setting

[Yu-Ping Wu (Code Review)]

Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48954 )

Change subject: soc/mediatek/mt8192: pmic: unlock key protection before initial setting
......................................................................

Patch Set 11:

(2 comments)

https://review.coreboot.org/c/coreboot/+/48954/11//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/48954/11//COMMIT_MSG@9 
PS11, Line 9: We need unlock key protection so that some initial settings could be effective.
Line too long (> 72 chars).

https://review.coreboot.org/c/coreboot/+/48954/11/src/soc/mediatek/mt8192/mt... 
File src/soc/mediatek/mt8192/mt6359p.c:

https://review.coreboot.org/c/coreboot/+/48954/11/src/soc/mediatek/mt8192/mt... 
PS11, Line 323: {
No curly braces.

Change in coreboot[master]: soc/mediatek/mt8192: pmic: unlock key protection before initial setting

[Yidi Lin (Code Review)]

Attention is currently required from: Yidi Lin.
Yidi Lin has uploaded a new patch set (#19) to the change originally created by hsin-hsiung wang. ( https://review.coreboot.org/c/coreboot/+/48954 )

Change subject: soc/mediatek/mt8192: pmic: unlock key protection before initial setting
......................................................................

soc/mediatek/mt8192: pmic: unlock key protection before initial setting

We need to write some special values to key protection registers before
applying init_setting table and lp_setting table to PMIC. Otherwise,
those settings won't take effect.
After applying init_setting table and lp_setting table, we lock the
settings by writing zero to key protection registers.

Reference datasheet: MT6359_PMIC_Data_Sheet_V1.5.docx, RH-D-2018-0205.

BUG=b:172636735
BRANCH=none
TEST=boot asurada correctly

Signed-off-by: Hsin-Hsiung Wang 
Change-Id: I593d4e02bf0b62ac297957caf4ae1c1837f1f38d
---
M src/soc/mediatek/mt8192/mt6359p.c
1 file changed, 20 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/48954/19

Change in coreboot[master]: soc/mediatek/mt8192: pmic: unlock key protection before initial setting

[Yidi Lin (Code Review)]

Attention is currently required from: Hung-Te Lin, Paul Menzel, Yu-Ping Wu, hsin-hsiung wang.
Yidi Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48954 )

Change subject: soc/mediatek/mt8192: pmic: unlock key protection before initial setting
......................................................................

Patch Set 19:

(8 comments)

Commit Message:

https://review.coreboot.org/c/coreboot/+/48954/comment/ca2dd2ea_acf23824 
PS6, Line 9: initial settings could be effective
> Which ones?

Done

https://review.coreboot.org/c/coreboot/+/48954/comment/63d2f4a9_b55afb5c 
PS6, Line 9: unlock key protection
> Does that mean *disable key protection*? […]

Done

https://review.coreboot.org/c/coreboot/+/48954/comment/11d34690_1251d107 
PS6, Line 11:
> Please mention the datasheet name and revision.

Done

Commit Message:

https://review.coreboot.org/c/coreboot/+/48954/comment/a388f680_cc9c7bcf 
PS11, Line 9: We need unlock key protection so that some initial settings could be effective.
> Line too long (> 72 chars).

Done

File src/soc/mediatek/mt8192/mt6359p.c:

https://review.coreboot.org/c/coreboot/+/48954/comment/040a5950_70fae6e5 
PS6, Line 321: set
> I would stay with *setting*.

Done

https://review.coreboot.org/c/coreboot/+/48954/comment/20a9deae_dfb889cd 
PS6, Line 329: 	}
> I’d put the for loop outside, and in the body use the ternary operator: […]

Done

File src/soc/mediatek/mt8192/mt6359p.c:

https://review.coreboot.org/c/coreboot/+/48954/comment/b060c0df_b7a631c9 
PS8, Line 323: 	if (lock) {
             : 		for (int i = 0; i < ARRAY_SIZE(key_protect_setting); i++)
             : 			mt6359p_write(key_protect_setting[i].addr, 0);
             : 	} else {
             : 		for (int i = 0; i < ARRAY_SIZE(key_protect_setting); i++)
             : 			mt6359p_write(key_protect_setting[i].addr, key_protect_setting[i].val);
             : 	}
> I'd rather do […]

Done

File src/soc/mediatek/mt8192/mt6359p.c:

https://review.coreboot.org/c/coreboot/+/48954/comment/70d67f37_7de41972 
PS11, Line 323: {
> No curly braces.

Done

Change in coreboot[master]: soc/mediatek/mt8192: pmic: unlock key protection before initial setting

[Hung-Te Lin (Code Review)]

Attention is currently required from: Paul Menzel, Yidi Lin, Yu-Ping Wu, hsin-hsiung wang.
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48954 )

Change subject: soc/mediatek/mt8192: pmic: unlock key protection before initial setting
......................................................................

Patch Set 19: Code-Review+2

Change in coreboot[master]: soc/mediatek/mt8192: pmic: unlock key protection before initial setting

[Yu-Ping Wu (Code Review)]

Attention is currently required from: Paul Menzel, Yidi Lin, hsin-hsiung wang.
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48954 )

Change subject: soc/mediatek/mt8192: pmic: unlock key protection before initial setting
......................................................................

Patch Set 19: Code-Review+2

Change in coreboot[master]: soc/mediatek/mt8192: pmic: unlock key protection before initial setting

[Hung-Te Lin (Code Review)]

Hung-Te Lin has submitted this change. ( https://review.coreboot.org/c/coreboot/+/48954 )

Change subject: soc/mediatek/mt8192: pmic: unlock key protection before initial setting
......................................................................

soc/mediatek/mt8192: pmic: unlock key protection before initial setting

We need to write some special values to key protection registers before
applying init_setting table and lp_setting table to PMIC. Otherwise,
those settings won't take effect.
After applying init_setting table and lp_setting table, we lock the
settings by writing zero to key protection registers.

Reference datasheet: MT6359_PMIC_Data_Sheet_V1.5.docx, RH-D-2018-0205.

BUG=b:172636735
BRANCH=none
TEST=boot asurada correctly

Signed-off-by: Hsin-Hsiung Wang 
Change-Id: I593d4e02bf0b62ac297957caf4ae1c1837f1f38d
Reviewed-on: https://review.coreboot.org/c/coreboot/+/48954
Tested-by: build bot (Jenkins) 
Reviewed-by: Hung-Te Lin 
Reviewed-by: Yu-Ping Wu 
---
M src/soc/mediatek/mt8192/mt6359p.c
1 file changed, 20 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Hung-Te Lin: Looks good to me, approved
  Yu-Ping Wu: Looks good to me, approved

diff --git a/src/soc/mediatek/mt8192/mt6359p.c b/src/soc/mediatek/mt8192/mt6359p.c
index df8eb39..c93b66a 100644
--- a/src/soc/mediatek/mt8192/mt6359p.c
+++ b/src/soc/mediatek/mt8192/mt6359p.c
@@ -6,6 +6,17 @@
 #include <soc/pmif.h>
 #include <soc/mt6359p.h>

+static const struct pmic_setting key_protect_setting[] = {
+	{0x3A8, 0x9CA6, 0xFFFF, 0},
+	{0x44A, 0xBADE, 0xFFFF, 0},
+	{0xA3A, 0x4729, 0xFFFF, 0},
+	{0xC58, 0x1605, 0xFFFF, 0},
+	{0xC5A, 0x1706, 0xFFFF, 0},
+	{0xC5C, 0x1807, 0xFFFF, 0},
+	{0xFB4, 0x6359, 0xFFFF, 0},
+	{0x1432, 0x5543, 0xFFFF, 0},
+};
+
 static const struct pmic_setting init_setting[] = {
    {0x20, 0xA, 0xA, 0},
    {0x24, 0x1F00, 0x1F00, 0},
@@ -318,6 +329,13 @@
    mt6359p_write_field(PMIC_TOP_RST_MISC_SET, 0x01, 0xFFFF, 0);
 }

+static void pmic_protect_key_setting(bool lock)
+{
+	for (int i = 0; i < ARRAY_SIZE(key_protect_setting); i++)
+		mt6359p_write(key_protect_setting[i].addr,
+			      lock ? 0 : key_protect_setting[i].val);
+}
+
 static void pmic_init_setting(void)
 {
    for (int i = 0; i < ARRAY_SIZE(init_setting); i++)
@@ -449,8 +467,10 @@
    init_pmif_arb();
    pmic_set_power_hold();
    pmic_wdt_set();
+	pmic_protect_key_setting(false);
    pmic_init_setting();
    pmic_lp_setting();
+	pmic_protect_key_setting(true);
    pmic_wk_vs2_voter_setting();
 }