Change in ...coreboot[master]: mediatek/mt8183: Enable RTC eosc calibration feature to save power

[Ran Bi (Code Review)]

Ran Bi has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32339

Change subject: mediatek/mt8183: Enable RTC eosc calibration feature to save power
......................................................................

mediatek/mt8183: Enable RTC eosc calibration feature to save power

When system shutdown, RTC enable eosc calibration feature to save
power. Then coreboot RTC driver need to call rtc_enable_dcxo function
at every boot up to switch RTC clock source to a more accurate one.

BUG=b:128467245
BRANCH=none
TEST=Boots correctly on Kukui

Change-Id: Iee21e7611df8959cbbc63b6e6655cfb462147748
Signed-off-by: Ran Bi 
---
M src/soc/mediatek/mt8183/rtc.c
1 file changed, 4 insertions(+), 6 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/32339/1

diff --git a/src/soc/mediatek/mt8183/rtc.c b/src/soc/mediatek/mt8183/rtc.c
index 62256eb..ba05f86 100644
--- a/src/soc/mediatek/mt8183/rtc.c
+++ b/src/soc/mediatek/mt8183/rtc.c
@@ -197,12 +197,6 @@
    	goto err;
    }

-	/* using dcxo 32K clock */
-	if (!rtc_enable_dcxo()) {
-		ret = -RTC_STATUS_OSC_SETTING_FAIL;
-		goto err;
-	}
-
    if (recover)
    	mdelay(20);

@@ -311,6 +305,10 @@
    pwrap_write_field(PMIC_RG_DCXO_CW02, 0xF, 0xF, 0);
    pwrap_write_field(PMIC_RG_SCK_TOP_CON0, 0x1, 0x1, 0);

+	/* using dcxo 32K clock */
+	if (!rtc_enable_dcxo())
+		rtc_info("rtc_enable_dcxo() fail\n");
+
    rtc_boot_common();
    rtc_bbpu_power_on();
 }

[Paul Menzel (Code Review)]

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

Change subject: mediatek/mt8183: Enable RTC eosc calibration feature to save power
......................................................................

Patch Set 1:

(5 comments)

https://review.coreboot.org/#/c/32339/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/32339/1//COMMIT_MSG@9 
PS1, Line 9: shutdown
shuts down

Do you mean a real shutdown? Why does it matter, how the RTC is configure there?

https://review.coreboot.org/#/c/32339/1//COMMIT_MSG@9 
PS1, Line 9: When system shutdown, RTC enable eosc calibration feature to save
           : power.
… This will reduce the RTC clock source accuracy. ?

https://review.coreboot.org/#/c/32339/1//COMMIT_MSG@11 
PS1, Line 11: boot up
boot

https://review.coreboot.org/#/c/32339/1//COMMIT_MSG@11 
PS1, Line 11: a more accurate one
Which one?

https://review.coreboot.org/#/c/32339/1//COMMIT_MSG@12 
PS1, Line 12: 
How much power is saved?

[Ran Bi (Code Review)]

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/+/32339

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

Change subject: mediatek/mt8183: Enable RTC eosc calibration feature to save power
......................................................................

mediatek/mt8183: Enable RTC eosc calibration feature to save power

When system shuts down, RTC enable eosc calibration feature to save
power. Then coreboot RTC driver need to call rtc_enable_dcxo function
at every boot to switch RTC clock source to dcxo.

BUG=b:128467245
BRANCH=none
TEST=Boots correctly on Kukui

Change-Id: Iee21e7611df8959cbbc63b6e6655cfb462147748
Signed-off-by: Ran Bi 
---
M src/soc/mediatek/mt8183/rtc.c
1 file changed, 4 insertions(+), 6 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/32339/2

[Ran Bi (Code Review)]

Ran Bi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32339 )

Change subject: mediatek/mt8183: Enable RTC eosc calibration feature to save power
......................................................................

Patch Set 2:

(5 comments)

https://review.coreboot.org/#/c/32339/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/32339/1//COMMIT_MSG@9 
PS1, Line 9: shutdown
> shuts down […]

In atf code, RTC will switch clock source from dcxo to eosc before real shutdown. So we need to switch it back at next boot.

https://review.coreboot.org/#/c/32339/1//COMMIT_MSG@9 
PS1, Line 9: When system shutdown, RTC enable eosc calibration feature to save
           : power.
> … This will reduce the RTC clock source accuracy. […]

At eosc calibration mode, RTC bias is within 63.4ppm which dcxo is within 2ppm.

https://review.coreboot.org/#/c/32339/1//COMMIT_MSG@11 
PS1, Line 11: a more accurate one
> Which one?

Done

https://review.coreboot.org/#/c/32339/1//COMMIT_MSG@11 
PS1, Line 11: boot up
> boot

Done

https://review.coreboot.org/#/c/32339/1//COMMIT_MSG@12 
PS1, Line 12:
> How much power is saved?

This eosc calibration mode saved about 200uA power.

[Julius Werner (Code Review)]

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

Change subject: mediatek/mt8183: Enable RTC eosc calibration feature to save power
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/32339/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/32339/1//COMMIT_MSG@9 
PS1, Line 9: shutdown
> In atf code, RTC will switch clock source from dcxo to eosc before real shutdown. […]

Is there a good reason why this is handled in coreboot/Arm TF respectively? Why can't you just do it in the kernel instead? Generally, functionality shouldn't be implemented in firmware unless there's a good reason why it can't be done by the OS, and having a slightly more accurate RTC for the one second it takes us to load the kernel doesn't really seem like a good enough reason.

[Ran Bi (Code Review)]

Ran Bi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32339 )

Change subject: mediatek/mt8183: Enable RTC eosc calibration feature to save power
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/32339/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/32339/1//COMMIT_MSG@9 
PS1, Line 9: shutdown
> Is there a good reason why this is handled in coreboot/Arm TF respectively? Why can't you just do it […]

Patch in coreboot reason:
EOSC calibration only exist when system shutdown, we should not use EOSC clock directly without calibration. So we want to switch the clock to DCXO earlier because watchdog, BT and other modules might use this clock before kernel RTC init.

Patch in ATF reason:
We implement psci system_off function in ATF which contains some PMIC settings and RTC alarm settings. So we suppose it's better to put RTC eosc calibration setting here.

[Julius Werner (Code Review)]

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

Change subject: mediatek/mt8183: Enable RTC eosc calibration feature to save power
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/32339/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/32339/1//COMMIT_MSG@9 
PS1, Line 9: shutdown
> EOSC calibration only exist when system shutdown, we should not use EOSC clock directly without calibration. So we want to switch the clock to DCXO earlier because watchdog, BT and other modules might use this clock before kernel RTC init.

Does BT mean Bluetooth? What does the PMIC RTC clock have to do with Bluetooth? And are we actually using the PMIC watchdog for anything? (I assume this is different from the AP watchdog?) We generally only want one hardware watchdog on the system, otherwise they'll just get in each other's way.
> We implement psci system_off function in ATF which contains some PMIC settings and RTC alarm settings.

Do you not have an RTC/PMIC driver in the kernel. Also, what RTC alarm stuff do you have in TF? That really sounds like something that should only be in the kernel.

[Ran Bi (Code Review)]

Ran Bi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32339 )

Change subject: mediatek/mt8183: Enable RTC eosc calibration feature to save power
......................................................................

Patch Set 2:

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

https://review.coreboot.org/#/c/32339/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/32339/1//COMMIT_MSG@9 
PS1, Line 9: shutdown
> Does BT mean Bluetooth? What does the PMIC RTC clock have to do with Bluetooth? And are we actually using the PMIC watchdog for anything? (I assume this is different from the AP watchdog?) We generally only want one hardware watchdog on the system, otherwise they'll just get in each other's way.

Yes, BT means Bluetooth. BT using 32K clock source which RTC output. AP watchdog/SPM also using 32K clock source which RTC output.
> Do you not have an RTC/PMIC driver in the kernel. Also, what RTC alarm stuff do you have in TF? That really sounds like something that should only be in the kernel.

ATF PSCI system_off function is the last step of system shutdown. We need to do some PMIC and RTC settings here then trigger PMIC register to disable power supply to SoC.

[You-Cheng Syu (Code Review)]

You-Cheng Syu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32339 )

Change subject: mediatek/mt8183: Enable RTC eosc calibration feature to save power
......................................................................

Patch Set 2:

ping

[Ran Bi (Code Review)]

Ran Bi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32339 )

Change subject: mediatek/mt8183: Enable RTC eosc calibration feature to save power
......................................................................

Patch Set 2:

I have a patch to fix some issues related to this patch. (1. RTC time count drift. 2. Sometimes cannot go into EOSC cali mode.) Can I just upload the patch to this patch? Or I need to upload a new patch?

[Ran Bi (Code Review)]

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/+/32339

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

Change subject: mediatek/mt8183: Enable RTC eosc calibration feature to save power
......................................................................

mediatek/mt8183: Enable RTC eosc calibration feature to save power

When system shuts down, RTC enable eosc calibration feature to save
power. Then coreboot RTC driver need to call rtc_enable_dcxo function
at every boot to switch RTC clock source to dcxo.

BUG=b:128467245
BRANCH=none
TEST=Boots correctly on Kukui

Change-Id: Iee21e7611df8959cbbc63b6e6655cfb462147748
Signed-off-by: Ran Bi 
---
M src/soc/mediatek/common/rtc.c
M src/soc/mediatek/mt8183/include/soc/rtc.h
M src/soc/mediatek/mt8183/rtc.c
3 files changed, 30 insertions(+), 23 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/32339/3

[Hung-Te Lin (Code Review)]

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

Change subject: mediatek/mt8183: Enable RTC eosc calibration feature to save power
......................................................................

Patch Set 3: Code-Review+1

Seems more clear now. @Julius?

[Paul Menzel (Code Review)]

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

Change subject: mediatek/mt8183: Enable RTC eosc calibration feature to save power
......................................................................

Patch Set 3:

(4 comments)

https://review.coreboot.org/#/c/32339/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/32339/3//COMMIT_MSG@10 
PS3, Line 10: need
needs

https://review.coreboot.org/#/c/32339/3//COMMIT_MSG@10 
PS3, Line 10: power. Then coreboot RTC driver need to call rtc_enable_dcxo function
How much power is saved?

https://review.coreboot.org/#/c/32339/3/src/soc/mediatek/mt8183/rtc.c 
File src/soc/mediatek/mt8183/rtc.c:

https://review.coreboot.org/#/c/32339/3/src/soc/mediatek/mt8183/rtc.c@307 
PS3, Line 307: using
Use

https://review.coreboot.org/#/c/32339/3/src/soc/mediatek/mt8183/rtc.c@309 
PS3, Line 309: fail
failed

[Ran Bi (Code Review)]

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/+/32339

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

Change subject: mediatek/mt8183: Enable RTC eosc calibration feature to save power
......................................................................

mediatek/mt8183: Enable RTC eosc calibration feature to save power

When system shuts down, RTC enable eosc calibration feature to save
power. Then coreboot RTC driver needs to call rtc_enable_dcxo function
at every boot to switch RTC clock source to dcxo.

BUG=b:128467245
BRANCH=none
TEST=Boots correctly on Kukui

Change-Id: Iee21e7611df8959cbbc63b6e6655cfb462147748
Signed-off-by: Ran Bi 
---
M src/soc/mediatek/common/rtc.c
M src/soc/mediatek/mt8183/include/soc/rtc.h
M src/soc/mediatek/mt8183/rtc.c
3 files changed, 33 insertions(+), 26 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/32339/4

[Ran Bi (Code Review)]

Ran Bi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32339 )

Change subject: mediatek/mt8183: Enable RTC eosc calibration feature to save power
......................................................................

Patch Set 4:

(4 comments)

https://review.coreboot.org/#/c/32339/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/32339/3//COMMIT_MSG@10 
PS3, Line 10: power. Then coreboot RTC driver need to call rtc_enable_dcxo function
> How much power is saved?

This eosc calibration mode saved about 200uA power.

https://review.coreboot.org/#/c/32339/3//COMMIT_MSG@10 
PS3, Line 10: need
> needs

Done

https://review.coreboot.org/#/c/32339/3/src/soc/mediatek/mt8183/rtc.c 
File src/soc/mediatek/mt8183/rtc.c:

https://review.coreboot.org/#/c/32339/3/src/soc/mediatek/mt8183/rtc.c@307 
PS3, Line 307: using
> Use

Done

https://review.coreboot.org/#/c/32339/3/src/soc/mediatek/mt8183/rtc.c@309 
PS3, Line 309: fail
> failed

Done

[Julius Werner (Code Review)]

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

Change subject: mediatek/mt8183: Enable RTC eosc calibration feature to save power
......................................................................

Patch Set 4: Code-Review+2

Change in coreboot[master]: mediatek/mt8183: Enable RTC eosc calibration feature to save power

[Martin Roth (Code Review)]

Martin Roth has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32339 )

Change subject: mediatek/mt8183: Enable RTC eosc calibration feature to save power
......................................................................

mediatek/mt8183: Enable RTC eosc calibration feature to save power

When system shuts down, RTC enable eosc calibration feature to save
power. Then coreboot RTC driver needs to call rtc_enable_dcxo function
at every boot to switch RTC clock source to dcxo.

BUG=b:128467245
BRANCH=none
TEST=Boots correctly on Kukui

Change-Id: Iee21e7611df8959cbbc63b6e6655cfb462147748
Signed-off-by: Ran Bi 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/32339
Tested-by: build bot (Jenkins) 
Reviewed-by: Julius Werner 
---
M src/soc/mediatek/common/rtc.c
M src/soc/mediatek/mt8183/include/soc/rtc.h
M src/soc/mediatek/mt8183/rtc.c
3 files changed, 33 insertions(+), 26 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Julius Werner: Looks good to me, approved

diff --git a/src/soc/mediatek/common/rtc.c b/src/soc/mediatek/common/rtc.c
index a9142b6..fe252b5 100644
--- a/src/soc/mediatek/common/rtc.c
+++ b/src/soc/mediatek/common/rtc.c
@@ -91,12 +91,15 @@
    u16 bbpu;

rtc_write(RTC_OSC32CON, RTC_OSC32CON_UNLOCK1);
-	udelay(200);
+	if (!rtc_busy_wait())
+		return 0;
    rtc_write(RTC_OSC32CON, RTC_OSC32CON_UNLOCK2);
-	udelay(200);
+	if (!rtc_busy_wait())
+		return 0;

rtc_write(RTC_OSC32CON, val);
-	udelay(200);
+	if (!rtc_busy_wait())
+		return 0;

rtc_read(RTC_BBPU, &bbpu);
    bbpu |= RTC_BBPU_KEY | RTC_BBPU_RELOAD;
diff --git a/src/soc/mediatek/mt8183/include/soc/rtc.h b/src/soc/mediatek/mt8183/include/soc/rtc.h
index 3d115fe..841a202 100644
--- a/src/soc/mediatek/mt8183/include/soc/rtc.h
+++ b/src/soc/mediatek/mt8183/include/soc/rtc.h
@@ -100,18 +100,25 @@
 };

enum {
-	RTC_EMBCK_SRC_SEL	= 1 << 8,
-	RTC_EMBCK_SEL_MODE	= 3 << 6,
-	RTC_XOSC32_ENB		= 1 << 5,
-	RTC_REG_XOSC32_ENB	= 1 << 15
+	RTC_XOSCCALI_MASK	= 0x1F << 0,
+	RTC_XOSC32_ENB		= 1U << 5,
+	RTC_EMB_HW_MODE		= 0U << 6,
+	RTC_EMB_K_EOSC32_MODE	= 1U << 6,
+	RTC_EMB_SW_DCXO_MODE	= 2U << 6,
+	RTC_EMB_SW_EOSC32_MODE	= 3U << 6,
+	RTC_EMBCK_SEL_MODE_MASK	= 3U << 6,
+	RTC_EMBCK_SRC_SEL	= 1U << 8,
+	RTC_EMBCK_SEL_OPTION	= 1U << 9,
+	RTC_GPS_CKOUT_EN	= 1U << 10,
+	RTC_REG_XOSC32_ENB	= 1U << 15
 };

enum {
-	RTC_LPD_OPT_XOSC_AND_EOSC_LPD	= 0 << 13,
-	RTC_LPD_OPT_EOSC_LPD		= 1 << 13,
-	RTC_LPD_OPT_XOSC_LPD		= 2 << 13,
-	RTC_LPD_OPT_F32K_CK_ALIVE	= 3 << 13,
-	RTC_LPD_OPT_MASK		= 3 << 13
+	RTC_LPD_OPT_XOSC_AND_EOSC_LPD	= 0U << 13,
+	RTC_LPD_OPT_EOSC_LPD		= 1U << 13,
+	RTC_LPD_OPT_XOSC_LPD		= 2U << 13,
+	RTC_LPD_OPT_F32K_CK_ALIVE	= 3U << 13,
+	RTC_LPD_OPT_MASK		= 3U << 13
 };

/* PMIC TOP Register Definition */
diff --git a/src/soc/mediatek/mt8183/rtc.c b/src/soc/mediatek/mt8183/rtc.c
index 62256eb..5879088 100644
--- a/src/soc/mediatek/mt8183/rtc.c
+++ b/src/soc/mediatek/mt8183/rtc.c
@@ -33,20 +33,19 @@

mdelay(1);
    if (!rtc_writeif_unlock()) { /* Unlock for reload */
-		rtc_info("rtc_writeif_unlock() fail\n");
+		rtc_info("rtc_writeif_unlock() failed\n");
    	return 0;
    }

rtc_read(RTC_OSC32CON, &osc32con);
-	osc32con &= ~RTC_EMBCK_SRC_SEL;
-	osc32con |= RTC_XOSC32_ENB | RTC_REG_XOSC32_ENB;
+	osc32con &= ~(RTC_EMBCK_SRC_SEL | RTC_EMBCK_SEL_MODE_MASK
+		      | RTC_GPS_CKOUT_EN);
+	osc32con |= RTC_XOSC32_ENB | RTC_REG_XOSC32_ENB
+		    | RTC_EMB_K_EOSC32_MODE | RTC_EMBCK_SEL_OPTION;
    if (!rtc_xosc_write(osc32con)) {
-		rtc_info("rtc_xosc_write() fail\n");
+		rtc_info("rtc_xosc_write() failed\n");
    	return 0;
    }
-	rtc_read(RTC_BBPU, &bbpu);
-	rtc_write(RTC_BBPU, bbpu | RTC_BBPU_KEY | RTC_BBPU_RELOAD);
-	rtc_write_trigger();

rtc_read(RTC_CON, &con);
    rtc_read(RTC_OSC32CON, &osc32con);
@@ -197,12 +196,6 @@
    	goto err;
    }

-	/* using dcxo 32K clock */
-	if (!rtc_enable_dcxo()) {
-		ret = -RTC_STATUS_OSC_SETTING_FAIL;
-		goto err;
-	}
-
    if (recover)
    	mdelay(20);

@@ -264,7 +257,7 @@
    u16 bbpu;

if (!rtc_writeif_unlock())
-		rtc_info("rtc_writeif_unlock() fail\n");
+		rtc_info("rtc_writeif_unlock() failed\n");
    /* pull PWRBB low */
    bbpu = RTC_BBPU_KEY | RTC_BBPU_RELOAD | RTC_BBPU_PWREN;
    rtc_write(RTC_BBPU, bbpu);
@@ -311,6 +304,10 @@
    pwrap_write_field(PMIC_RG_DCXO_CW02, 0xF, 0xF, 0);
    pwrap_write_field(PMIC_RG_SCK_TOP_CON0, 0x1, 0x1, 0);

+	/* use dcxo 32K clock */
+	if (!rtc_enable_dcxo())
+		rtc_info("rtc_enable_dcxo() failed\n");
+
    rtc_boot_common();
    rtc_bbpu_power_on();
 }