Change in coreboot[master]: mediatek/mt8183: postpone dcxo low power mode setting

[Weiyi Lu (Code Review)]

Hello Weiyi Lu,

I'd like you to do a code review. Please visit

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

to review the following change.

Change subject: mediatek/mt8183: postpone dcxo low power mode setting
......................................................................

mediatek/mt8183: postpone dcxo low power mode setting

Consider the association between MD and DCXO, this patch is a fix for
eb5e47d("mediatek/mt8183: update dcxo output buffer setting") [1]
We should not disable XO_CEL and block the bblpm request when MD is still ON.

[1] https://review.coreboot.org/c/coreboot/+/32323

BRANCH=none
TEST=Boots correctly on Krane.

Change-Id: I047ebed615e874977ca211aafd52b5551c71b764
Signed-off-by: Weiyi Lu 
---
M src/soc/mediatek/mt8183/include/soc/rtc.h
M src/soc/mediatek/mt8183/rtc.c
M src/soc/mediatek/mt8183/soc.c
3 files changed, 14 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/34777/1

diff --git a/src/soc/mediatek/mt8183/include/soc/rtc.h b/src/soc/mediatek/mt8183/include/soc/rtc.h
index 5a61208..aa4cc43 100644
--- a/src/soc/mediatek/mt8183/include/soc/rtc.h
+++ b/src/soc/mediatek/mt8183/include/soc/rtc.h
@@ -147,6 +147,7 @@
 /* PMIC DCXO Register Definition */
 enum {
    PMIC_RG_DCXO_CW00 = 0x0788,
+	PMIC_RG_DCXO_CW00_CLR = 0x078C,
    PMIC_RG_DCXO_CW02 = 0x0790,
    PMIC_RG_DCXO_CW07 = 0x079A,
    PMIC_RG_DCXO_CW09 = 0x079E,
@@ -218,5 +219,6 @@
 void rtc_osc_init(void);
 int rtc_init(u8 recover);
 void rtc_boot(void);
+void dcxo_disable_unused(void);

#endif /* SOC_MEDIATEK_MT8183_RTC_H */
diff --git a/src/soc/mediatek/mt8183/rtc.c b/src/soc/mediatek/mt8183/rtc.c
index af30d1f..464fff3 100644
--- a/src/soc/mediatek/mt8183/rtc.c
+++ b/src/soc/mediatek/mt8183/rtc.c
@@ -411,10 +411,9 @@
    rtc_write(PMIC_RG_DCXO_CW16, 0x9855);

/* 26M enable control */
-	/* Enable clock buffer XO_SOC */
-	rtc_write(PMIC_RG_DCXO_CW00, 0x4005);
+	/* Enable clock buffer XO_SOC, XO_CEL */
+	rtc_write(PMIC_RG_DCXO_CW00, 0x4805);
    rtc_write(PMIC_RG_DCXO_CW11, 0x8000);
-	rtc_write(PMIC_RG_DCXO_CW23, 0x0053);

/* Load thermal coefficient */
    rtc_write(PMIC_RG_TOP_TMA_KEY, 0x9CA7);
@@ -432,6 +431,14 @@
    mdelay(5);
 }

+void dcxo_disable_unused(void)
+{
+	/* Disable clock buffer XO_CEL */
+	rtc_write(PMIC_RG_DCXO_CW00_CLR, 0x0800);
+	/* Mask bblpm */
+	rtc_write(PMIC_RG_DCXO_CW23, 0x0053);
+}
+
 /* the rtc boot flow entry */
 void rtc_boot(void)
 {
diff --git a/src/soc/mediatek/mt8183/soc.c b/src/soc/mediatek/mt8183/soc.c
index c9c2147..f190480 100644
--- a/src/soc/mediatek/mt8183/soc.c
+++ b/src/soc/mediatek/mt8183/soc.c
@@ -17,6 +17,7 @@
 #include <soc/emi.h>
 #include <soc/md_ctrl.h>
 #include <soc/mmu_operations.h>
+#include <soc/rtc.h>
 #include <soc/sspm.h>
 #include <symbols.h>

@@ -29,6 +30,7 @@
 {
    mtk_mmu_disable_l2c_sram();
    mtk_md_early_init();
+	dcxo_disable_unused();
    sspm_init();
 }

[Hung-Te Lin (Code Review)]

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

Change subject: mediatek/mt8183: postpone dcxo low power mode setting
......................................................................

Patch Set 1: Code-Review+1

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34777 )

Change subject: mediatek/mt8183: postpone dcxo low power mode setting
......................................................................

Patch Set 1: Code-Review+2

(2 comments)

https://review.coreboot.org/c/coreboot/+/34777/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/34777/1//COMMIT_MSG@11 
PS1, Line 11: We should not disable XO_CEL and block the bblpm request when MD is still ON.
nit: I have no idea what this means. What is "MD"? Please be a little more elaborate.

https://review.coreboot.org/c/coreboot/+/34777/1/src/soc/mediatek/mt8183/inc... 
File src/soc/mediatek/mt8183/include/soc/rtc.h:

https://review.coreboot.org/c/coreboot/+/34777/1/src/soc/mediatek/mt8183/inc... 
PS1, Line 222: void dcxo_disable_unused(void);
nit: external functions should be better namespaced, e.g. rtc_dcxo_disable_unused() or mt6358_dcxo_disable_unused(). (What does "unused" even mean here? Name could be more descriptive.)

[Weiyi Lu (Code Review)]

Hello Yu-Ping Wu, Julius Werner, You-Cheng Syu, Hung-Te Lin, Ben Ho, build bot (Jenkins), Weiyi Lu,

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

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

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

Change subject: mediatek/mt8183: postpone dcxo low power mode setting
......................................................................

mediatek/mt8183: postpone dcxo low power mode setting

Consider the association between modem[1] and DCXO, this patch is a fix for
eb5e47d("mediatek/mt8183: update dcxo output buffer setting") [2]
We should not disable XO_CEL and block the bblpm request when modem is still ON.
For power-saving, we still could disable unused XO_CEL and
mask request to disable unused power mode when modem is no longer be used.

[1] https://review.coreboot.org/c/coreboot/+/32666
[2] https://review.coreboot.org/c/coreboot/+/32323

BRANCH=none
TEST=Boots correctly on Krane.

Change-Id: I047ebed615e874977ca211aafd52b5551c71b764
Signed-off-by: Weiyi Lu 
---
M src/soc/mediatek/mt8183/include/soc/rtc.h
M src/soc/mediatek/mt8183/rtc.c
M src/soc/mediatek/mt8183/soc.c
3 files changed, 14 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/34777/2

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34777 )

Change subject: mediatek/mt8183: postpone dcxo low power mode setting
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34777/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/34777/2//COMMIT_MSG@13 
PS2, Line 13: mask request to disable unused power mode when modem is no longer be used.
So is the only reason why we need this that we're calling mt6358_init() before mtk_md_early_init()? Why don't we just move mtk_md_early_init() into romstage? It's just a 2-line function...

[Hung-Te Lin (Code Review)]

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

Change subject: mediatek/mt8183: postpone dcxo low power mode setting
......................................................................

Patch Set 2:

MTK is having internal discussion on comparing few alternatives. Let's wait for their conclusion.

[Yanjie Jiang (Code Review)]

Yanjie Jiang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34777 )

Change subject: mediatek/mt8183: postpone dcxo low power mode setting
......................................................................

Patch Set 2:
> Patch Set 2:
>
> (1 comment)

will not only 2-line at have modem version.
also will have a close modem MTCMOS flow here.

[Hung-Te Lin (Code Review)]

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

Change subject: mediatek/mt8183: postpone dcxo low power mode setting
......................................................................

Patch Set 2: Code-Review+2

Confirmed with MTK. There are other timing issues so moving mtk_md_early_init to romstage would fail. Let's keep current approach.

[Weiyi Lu (Code Review)]

Weiyi Lu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34777 )

Change subject: mediatek/mt8183: postpone dcxo low power mode setting
......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/c/coreboot/+/34777/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/34777/1//COMMIT_MSG@11 
PS1, Line 11: We should not disable XO_CEL and block the bblpm request when MD is still ON.
> nit: I have no idea what this means. What is "MD"? Please be a little more elaborate.

Done, modify it in commit message, MD => modem

https://review.coreboot.org/c/coreboot/+/34777/1/src/soc/mediatek/mt8183/inc... 
File src/soc/mediatek/mt8183/include/soc/rtc.h:

https://review.coreboot.org/c/coreboot/+/34777/1/src/soc/mediatek/mt8183/inc... 
PS1, Line 222: void dcxo_disable_unused(void);
> nit: external functions should be better namespaced, e.g. […]

Done, add more description about unused stuff in commit message

[Hung-Te Lin (Code Review)]

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

Change subject: mediatek/mt8183: postpone dcxo low power mode setting
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34777/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/34777/2//COMMIT_MSG@13 
PS2, Line 13: mask request to disable unused power mode when modem is no longer be used.
> So is the only reason why we need this that we're calling mt6358_init() before mtk_md_early_init()?  […]

Ack

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/34777 )

Change subject: mediatek/mt8183: postpone dcxo low power mode setting
......................................................................

mediatek/mt8183: postpone dcxo low power mode setting

Consider the association between modem[1] and DCXO, this patch is a fix for
eb5e47d("mediatek/mt8183: update dcxo output buffer setting") [2]
We should not disable XO_CEL and block the bblpm request when modem is still ON.
For power-saving, we still could disable unused XO_CEL and
mask request to disable unused power mode when modem is no longer be used.

[1] https://review.coreboot.org/c/coreboot/+/32666
[2] https://review.coreboot.org/c/coreboot/+/32323

BRANCH=none
TEST=Boots correctly on Krane.

Change-Id: I047ebed615e874977ca211aafd52b5551c71b764
Signed-off-by: Weiyi Lu 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/34777
Tested-by: build bot (Jenkins) 
Reviewed-by: Hung-Te Lin 
---
M src/soc/mediatek/mt8183/include/soc/rtc.h
M src/soc/mediatek/mt8183/rtc.c
M src/soc/mediatek/mt8183/soc.c
3 files changed, 14 insertions(+), 3 deletions(-)

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

diff --git a/src/soc/mediatek/mt8183/include/soc/rtc.h b/src/soc/mediatek/mt8183/include/soc/rtc.h
index 5a61208..bf120e9 100644
--- a/src/soc/mediatek/mt8183/include/soc/rtc.h
+++ b/src/soc/mediatek/mt8183/include/soc/rtc.h
@@ -147,6 +147,7 @@
 /* PMIC DCXO Register Definition */
 enum {
    PMIC_RG_DCXO_CW00 = 0x0788,
+	PMIC_RG_DCXO_CW00_CLR = 0x078C,
    PMIC_RG_DCXO_CW02 = 0x0790,
    PMIC_RG_DCXO_CW07 = 0x079A,
    PMIC_RG_DCXO_CW09 = 0x079E,
@@ -218,5 +219,6 @@
 void rtc_osc_init(void);
 int rtc_init(u8 recover);
 void rtc_boot(void);
+void mt6358_dcxo_disable_unused(void);

#endif /* SOC_MEDIATEK_MT8183_RTC_H */
diff --git a/src/soc/mediatek/mt8183/rtc.c b/src/soc/mediatek/mt8183/rtc.c
index f8d81f8..19b717c 100644
--- a/src/soc/mediatek/mt8183/rtc.c
+++ b/src/soc/mediatek/mt8183/rtc.c
@@ -411,10 +411,9 @@
    rtc_write(PMIC_RG_DCXO_CW16, 0x9855);

/* 26M enable control */
-	/* Enable clock buffer XO_SOC */
-	rtc_write(PMIC_RG_DCXO_CW00, 0x4005);
+	/* Enable clock buffer XO_SOC, XO_CEL */
+	rtc_write(PMIC_RG_DCXO_CW00, 0x4805);
    rtc_write(PMIC_RG_DCXO_CW11, 0x8000);
-	rtc_write(PMIC_RG_DCXO_CW23, 0x0053);

/* Load thermal coefficient */
    rtc_write(PMIC_RG_TOP_TMA_KEY, 0x9CA7);
@@ -432,6 +431,14 @@
    mdelay(5);
 }

+void mt6358_dcxo_disable_unused(void)
+{
+	/* Disable clock buffer XO_CEL */
+	rtc_write(PMIC_RG_DCXO_CW00_CLR, 0x0800);
+	/* Mask bblpm */
+	rtc_write(PMIC_RG_DCXO_CW23, 0x0053);
+}
+
 /* the rtc boot flow entry */
 void rtc_boot(void)
 {
diff --git a/src/soc/mediatek/mt8183/soc.c b/src/soc/mediatek/mt8183/soc.c
index c9c2147..21b2f81 100644
--- a/src/soc/mediatek/mt8183/soc.c
+++ b/src/soc/mediatek/mt8183/soc.c
@@ -17,6 +17,7 @@
 #include <soc/emi.h>
 #include <soc/md_ctrl.h>
 #include <soc/mmu_operations.h>
+#include <soc/rtc.h>
 #include <soc/sspm.h>
 #include <symbols.h>

@@ -29,6 +30,7 @@
 {
    mtk_mmu_disable_l2c_sram();
    mtk_md_early_init();
+	mt6358_dcxo_disable_unused();
    sspm_init();
 }