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

[Yu-Ping Wu (Code Review)]

Attention is currently required from: Hung-Te Lin, Jarried Lin, Paul Menzel.

Yu-Ping Wu 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:

(33 comments)

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.

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.

One problem with this file is that, here contains many duplicate macros that are also defined in rtc_reg_common.h.

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

https://review.coreboot.org/c/coreboot/+/85978/comment/3ee672e6_07c41070?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:

- rtc.h
- mt6685_hw.h
- mt6685_rtc_hw.h
- mt6685_rtc.h

Which one is SoC-specific?

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

https://review.coreboot.org/c/coreboot/+/85978/comment/1df04c76_728bc829?usp... :
PS1, Line 21: u16
u8

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

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

https://review.coreboot.org/c/coreboot/+/85978/comment/09ee787f_1d7add2d?usp... :
PS1, Line 68: 			     RG_BANK_FQMTR_RST_SHIFT);
align

Same for other similar calls.

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

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

https://review.coreboot.org/c/coreboot/+/85978/comment/9078186d_69cafb45?usp... :
PS1, Line 106: 794
26M / 32K = 26 * 1024 / 32 = 832. How did you get the number 794?

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

https://review.coreboot.org/c/coreboot/+/85978/comment/874b3646_ff39d29d?usp... :
PS1, Line 107: rtc_read(RG_FQMTR_DATA, &rdata);
             : 	ret = rdata;
Remove `ret`, add `u16 fqmtr_data`, write `rtc_read(..., &fqmtr_data)` here, and return `fqmtr_data` at the end of function.

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

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

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

https://review.coreboot.org/c/coreboot/+/85978/comment/e8f0f579_a51a30ac?usp... :
PS1, Line 148: freq[2]
> What is the difference from freq[0] ?

Does the `config_interface` call in line #144 affect the result here?

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

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

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

https://review.coreboot.org/c/coreboot/+/85978/comment/21ba362c_d7060a46?usp... :
PS1, Line 221: 	printk(BIOS_INFO, "%s time out!\n", __func__);
> Is this an error?

This should be.

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

https://review.coreboot.org/c/coreboot/+/85978/comment/84568dc0_62044b90?usp... :
PS1, Line 376: INFO
ERR

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

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

https://review.coreboot.org/c/coreboot/+/85978/comment/de4bbd0e_0cf6f482?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 line #392 and this one). Please use different messages. For example this one could be `rtc_writeif_unlock failed after writing BBPU again`.

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

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

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

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

Is this comment for `rtc_measure_four_clock` or `rtc_eosc_check_clock`? `rtc_measure_four_clock` doesn't seem to check anything.

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

https://review.coreboot.org/c/coreboot/+/85978/comment/2ea7a30e_7f31aab8?usp... :
PS1, Line 583:  
`to`

Same for other similar comments below.

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

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