[XL] Change in coreboot[main]: soc/mediatek/mt8196: Add RTC driver

[Shunxi Zhang (Code Review)]

Attention is currently required from: Hung-Te Lin, Jarried Lin, Paul Menzel, Yidi Lin, Yu-Ping Wu.

Shunxi Zhang has posted comments on this change by Jarried Lin. ( https://review.coreboot.org/c/coreboot/+/85978?usp=email )

Change subject: soc/mediatek/mt8196: Add RTC driver
......................................................................

Patch Set 1:

(58 comments)

Commit Message:

https://review.coreboot.org/c/coreboot/+/85978/comment/3075a534_fa0ca826?usp... :
PS1, Line 9: rtc
> RTC

will update

File src/soc/mediatek/common/include/soc/rtc_common.h:

https://review.coreboot.org/c/coreboot/+/85978/comment/2f478564_af11ea7e?usp... :
PS1, Line 107: /* The return result of the clock calibration
             :  * The result is relationship coefficient between 26M and 32K,
             :  * which is non-unit.
             :  */
> Okay, I think this explains nothing. […]

Done

File src/soc/mediatek/mt8196/include/soc/mt6685_hw.h:

PS1:
> Is this file mt8196-specific? I wonder if this could be shared for all SoCs.

The PMIC adopted by 8196 is 6685, 8bit design

File src/soc/mediatek/mt8196/include/soc/mt6685_rtc_hw.h:

PS1:
> Is this file mt8196-specific? I wonder if this could be shared for all SoCs. […]

will update is next version, as discussion in meeting,  first update a version for the previous 56 comment,  Regarding the problem of the header file RTC_COMMON.H, a single version will be modified separately

https://review.coreboot.org/c/coreboot/+/85978/comment/128821d3_3209977a?usp... :
PS1, Line 24:
> Remove this extra space. Same for other values below.

It will modify in next version

https://review.coreboot.org/c/coreboot/+/85978/comment/1890d76a_48cdb7cb?usp... :
PS1, Line 89: mask
> masks

It will modify in next version

File src/soc/mediatek/mt8196/include/soc/rtc.h:

https://review.coreboot.org/c/coreboot/+/85978/comment/ac345694_edd33cb1?usp... :
PS1, Line 18: 	RTC_BBPU_PWREN		= BIT(0),
> Many constants are duplicate in mt6685_rtc_hw.h. Could you explain the differences of these files: […]

Regarding the problem of the header file RTC_COMMON.H, a single version will be modified separately;

https://review.coreboot.org/c/coreboot/+/85978/comment/17758cb8_ba5c5b93?usp... :
PS1, Line 58: WATCH_INIT_EXPIRED_TIME
> Can we have a more descriptive name? `XXX_US`

Done

https://review.coreboot.org/c/coreboot/+/85978/comment/e6a31365_a039ba35?usp... :
PS1, Line 59: WAIT_TIME_US
> Same here. Wait for what? […]

Done

https://review.coreboot.org/c/coreboot/+/85978/comment/e57dc31a_aa0a2193?usp... :
PS1, Line 68: //void rtc_boot(void);
> Remove?

Done

File src/soc/mediatek/mt8196/mt6685_rtc.c:

https://review.coreboot.org/c/coreboot/+/85978/comment/60cc1359_dc672bdd?usp... :
PS1, Line 6: 
           : #include <commonlib/bsd/bcd.h>
           : #include <console/console.h>
           : #include <delay.h>
           : #include <soc/rtc.h>
           : #include <soc/mt6685.h>
           : #include <soc/mt6685_rtc.h>
           : #include <soc/mt6685_hw.h>
           : #include <soc/mt6685_rtc_hw.h>
           : #include <soc/mt6685.h>
           : #include <soc/pmif.h>
           : #include <soc/spmi.h>
           : #include <stdbool.h>
           : #include <timer.h>
> sort

Acknowledged

https://review.coreboot.org/c/coreboot/+/85978/comment/37d92655_b82c3ef3?usp... :
PS1, Line 21: u16
> u8

Acknowledged

https://review.coreboot.org/c/coreboot/+/85978/comment/6efc119e_03583820?usp... :
PS1, Line 21: config_interface_read
> Does it make more sense to rename it to `config_interface_read_byte` or `rtc_read_byte`?

Acknowledged

https://review.coreboot.org/c/coreboot/+/85978/comment/57c0a2ba_5b382e18?usp... :
PS1, Line 23: 0xff
> upper case

Acknowledged

https://review.coreboot.org/c/coreboot/+/85978/comment/29861fcf_62f83ad1?usp... :
PS1, Line 43: freq
> Can we name these 3 fields instead of using an array?

will update

https://review.coreboot.org/c/coreboot/+/85978/comment/d3354998_5b863140?usp... :
PS1, Line 68: 			     RG_BANK_FQMTR_RST_SHIFT);
> align […]

Acknowledged

https://review.coreboot.org/c/coreboot/+/85978/comment/eca8acea_27b50c29?usp... :
PS1, Line 79: RG_FQMTR_CLK_CK_PDN_SHIFT
> align and fix similar issues.

Acknowledged

https://review.coreboot.org/c/coreboot/+/85978/comment/27b07f66_52c0647c?usp... :
PS1, Line 86:
> single space

Acknowledged

https://review.coreboot.org/c/coreboot/+/85978/comment/863a23a7_9c98e3fd?usp... :
PS1, Line 101: !((rtc_read
> Align with `FQMTR_TIMEOUT_US`

Acknowledged

https://review.coreboot.org/c/coreboot/+/85978/comment/855d66fb_9e0693a1?usp... :
PS1, Line 106: 794
> 26M / 32K = 26 * 1024 / 32 = 832. […]

26000000/(1024*32) = 793.45

https://review.coreboot.org/c/coreboot/+/85978/comment/1d338148_ca82cdf3?usp... :
PS1, Line 106: k
> K

26000000/(1024*32) = 793.45 approximately 794

https://review.coreboot.org/c/coreboot/+/85978/comment/b07413f1_63b91f43?usp... :
PS1, Line 107: rtc_read(RG_FQMTR_DATA, &rdata);
             : 	ret = rdata;
> Remove `ret`, add `u16 fqmtr_data`, write `rtc_read(... […]

will update

https://review.coreboot.org/c/coreboot/+/85978/comment/c4c58c0c_5424fbeb?usp... :
PS1, Line 119: rdata
> fqmtr_data

Done

https://review.coreboot.org/c/coreboot/+/85978/comment/ccc23bb6_ede32721?usp... :
PS1, Line 129: four
> There are only 3: freq[0], freq[1], freq[2].

will update

https://review.coreboot.org/c/coreboot/+/85978/comment/ae1cb79b_38512482?usp... :
PS1, Line 139:
> remove blank line

Done

https://review.coreboot.org/c/coreboot/+/85978/comment/b1809fc5_e16da45b?usp... :
PS1, Line 148: freq[2]
> Does the `config_interface` call in line #144 affect the result here?

will update

https://review.coreboot.org/c/coreboot/+/85978/comment/e87fe047_899cfe98?usp... :
PS1, Line 157: 	    (rtc_measure_four_clock(&result), rtc_eosc_check_clock(&result)))) {
> align

will update

https://review.coreboot.org/c/coreboot/+/85978/comment/77099865_4d0add39?usp... :
PS1, Line 167: 	printk(BIOS_INFO, "EOSC cali val = %#x\n", val);
> Sounds more like a debug message?

Yes, it is debug msg

https://review.coreboot.org/c/coreboot/+/85978/comment/f91eef62_a9429ae1?usp... :
PS1, Line 173: rtc_clr_clrset_trigger
> rtc_clrset_trigger

Done

https://review.coreboot.org/c/coreboot/+/85978/comment/a22051a4_53320b75?usp... :
PS1, Line 173: u32
> u16

Done

https://review.coreboot.org/c/coreboot/+/85978/comment/ae04dc45_0c98db49?usp... :
PS1, Line 173: u32
> u16

Done

https://review.coreboot.org/c/coreboot/+/85978/comment/4a11e638_22647891?usp... :
PS1, Line 191: 			       ~(RTC_CON_LPSTA_RAW | RTC_CON_LPRST |
> align

Done

https://review.coreboot.org/c/coreboot/+/85978/comment/18e2ad67_370443be?usp... :
PS1, Line 207: 		  (RTC_BBPU_RESET_SPAR & (~RTC_BBPU_SPAR_SW)));
> align

Done

https://review.coreboot.org/c/coreboot/+/85978/comment/a1a88928_e659051b?usp... :
PS1, Line 221: 	printk(BIOS_INFO, "%s time out!\n", __func__);
> This should be.

will update

https://review.coreboot.org/c/coreboot/+/85978/comment/62897a7a_3ebd77c0?usp... :
PS1, Line 233: rtc_read(RTC_AL_SEC, &rdata);
             : 
             : 	rtc_write(RTC_AL_SEC, (rdata & (~RTC_LPD_OPT_MASK)) | RTC_LPD_OPT_EOSC_LPD);
             : 	if (!rtc_write_trigger())
> rtc_clrset_trigger ?

will update

https://review.coreboot.org/c/coreboot/+/85978/comment/746fe6bb_63a80e71?usp... :
PS1, Line 239: rtc_read(RTC_CON, &rdata);
             : 	con = rdata | RTC_XOSC32_LPEN;
             : 	con &= ~RTC_CON_LPRST;
             : 	rtc_write(RTC_CON, con);
             : 	if (!rtc_write_trigger())
> rtc_clrset_trigger ?

will update

https://review.coreboot.org/c/coreboot/+/85978/comment/71e9cdf6_c8acb49c?usp... :
PS1, Line 309: MT6685_SCK_TOP_CKPDN_CON0_L
> Use `SCK_TOP_CKPDN_CON0_L`, and remove <soc/mt6685_hw.h>.

Done

https://review.coreboot.org/c/coreboot/+/85978/comment/6c7913b3_d6e9b261?usp... :
PS1, Line 315: RTC_SEC_DSN_REV0, rtc_sec_dsn_rev0);
> move to above line

Done

https://review.coreboot.org/c/coreboot/+/85978/comment/0ce4f010_c45cfa66?usp... :
PS1, Line 341: rtc_write(RTC_POWERKEY1, RTC_POWERKEY1_KEY);
             : 	rtc_write(RTC_POWERKEY2, RTC_POWERKEY2_KEY);
             : 
             : 	if (!rtc_write_trigger())
> `rtc_powerkey_init`

Done

https://review.coreboot.org/c/coreboot/+/85978/comment/c80791ea_a1ebcd12?usp... :
PS1, Line 357: rtc_write(RTC_POWERKEY1, RTC_POWERKEY1_KEY);
             : 	rtc_write(RTC_POWERKEY2, RTC_POWERKEY2_KEY);
             : 	if (!rtc_write_trigger())
> rtc_powerkey_init

Done

https://review.coreboot.org/c/coreboot/+/85978/comment/cc5e8d97_fd9aebc8?usp... :
PS1, Line 374: rtc_frequency_meter_check()
> move to the second and align with RETRY_MAX_TIME

Done

https://review.coreboot.org/c/coreboot/+/85978/comment/feb088b5_b6483082?usp... :
PS1, Line 376: 3 times
> retry

Done

https://review.coreboot.org/c/coreboot/+/85978/comment/9d17b34d_b6f50808?usp... :
PS1, Line 376: INFO
> ERR

Done

https://review.coreboot.org/c/coreboot/+/85978/comment/32865d25_795cf7cd?usp... :
PS1, Line 404: | RTC_K_EOSC_RSV_7 | RTC_K_EOSC_RSV_6);
> move to above line

Done

https://review.coreboot.org/c/coreboot/+/85978/comment/f786519a_ea41a3b1?usp... :
PS1, Line 417:
> remove one blank line

Done

https://review.coreboot.org/c/coreboot/+/85978/comment/ac130090_b65d79dc?usp... :
PS1, Line 426:
> failed

Done

https://review.coreboot.org/c/coreboot/+/85978/comment/6ce6b6c7_57aa34a2?usp... :
PS1, Line 426: after writing BBPU
> The point of this extra message is to differentiate between different error logs (such as the one in […]

Done

https://review.coreboot.org/c/coreboot/+/85978/comment/55bc7394_6a4b2221?usp... :
PS1, Line 433: MT6351
> Is this comment correct ?

Done

https://review.coreboot.org/c/coreboot/+/85978/comment/f6d5769b_c14b70a6?usp... :
PS1, Line 439:
> failed

Done

https://review.coreboot.org/c/coreboot/+/85978/comment/0bc84c07_81c9217b?usp... :
PS1, Line 445:
> failed

Done

https://review.coreboot.org/c/coreboot/+/85978/comment/3da13183_d1f2d928?usp... :
PS1, Line 434: 	rtc_write(RTC_POWERKEY1, RTC_POWERKEY1_KEY);
             : 	rtc_write(RTC_POWERKEY2, RTC_POWERKEY2_KEY);
             : 
             : 	if (!rtc_write_trigger()) {
             : 		printk(BIOS_ERR,
             : 		       "%s rtc_write_trigger after writing POWERKEY\n", __func__);
             : 		return false;
             : 	}
             : 
             : 	if (!rtc_writeif_unlock()) {
             : 		printk(BIOS_ERR,
             : 		       "%s rtc_writeif_unlock after writing POWERKEY\n", __func__);
             : 		return false;
             : 	}
> What is the difference between line 406~412 and here ? (additional call to `rtc_write_trigger`)

One is before writing RTC PowerKey, and the other is after writing RTC PowerKey;
The difference between the first boot and not the first boot

https://review.coreboot.org/c/coreboot/+/85978/comment/cd7fee12_e72015c4?usp... :
PS1, Line 497: void rtc_boot(void)
> It looks like it’s used in other drivers too, but I find the name non-descriptive.

Done

https://review.coreboot.org/c/coreboot/+/85978/comment/575706c9_5242d54d?usp... :
PS1, Line 524: Check clock source are match with 32K exist
> `Check if clock sources match existing 32K` […]

The function name will update:
this function is to check the frequency is in the normal range

https://review.coreboot.org/c/coreboot/+/85978/comment/4aee3851_a5c21002?usp... :
PS1, Line 535: (rtc_con & RTC_CON_LPSTA_RAW) ?
             : 	       "with" : "without")
> Same line.

Done

https://review.coreboot.org/c/coreboot/+/85978/comment/3b852e88_25bc4899?usp... :
PS1, Line 583:
> `to` […]

Done

https://review.coreboot.org/c/coreboot/+/85978/comment/3395d894_8d419e11?usp... :
PS1, Line 585: RG_OCT1_RTC32K_1V8_0_MASK
> align

Done

https://review.coreboot.org/c/coreboot/+/85978/comment/be8b05ce_b9b0566f?usp... :
PS1, Line 595: Selects
> Select

Done

https://review.coreboot.org/c/coreboot/+/85978/comment/f4afde3c_3f192e41?usp... :
PS1, Line 600: Truning
> You mean `Turn`?

Done