[coreboot-gerrit] [L] Change in coreboot[main]: soc/mediatek/mt8196: Add srclken_rc drivers

[ot_song fan (Code Review)]

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

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

Change subject: soc/mediatek/mt8196: Add srclken_rc drivers
......................................................................

Patch Set 3:

(27 comments)

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

https://review.coreboot.org/c/coreboot/+/85842/comment/33f0a92e_8ad19bae?usp... :
PS1, Line 6: /***********************************************************************************
           : ** Definitions
           : ************************************************************************************/
> remove

Done

https://review.coreboot.org/c/coreboot/+/85842/comment/2151fe81_2d94c064?usp... :
PS1, Line 16:  = 5
> Remove these.

Done

https://review.coreboot.org/c/coreboot/+/85842/comment/6062890a_2bcb69aa?usp... :
PS1, Line 126: _M
> What do `_M` and `_B` mean here? If that means MASK, and we remove `_M` and `_MSK_B`?

DCXO_FPM_CTRL_M means dcxo_fpm_ctrl_mode.
SRCVOLTEN_FPM_MSK_B is for the HW function of srcvolten_to_fpm mask bit, just register name.

https://review.coreboot.org/c/coreboot/+/85842/comment/c4aa7b3f_3365b90b?usp... :
PS1, Line 184: struct mtk_rc_status_regs {
> Merge this into the `mtk_rc_regs` struct. Remove `RC_STATE_BASE`.

Done

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

https://review.coreboot.org/c/coreboot/+/85842/comment/80343621_f091c430?usp... :
PS1, Line 32: ~=
> spaces around `~=`

Done

https://review.coreboot.org/c/coreboot/+/85842/comment/3577b79b_a8833b47?usp... :
PS1, Line 135:
> Remove extra blank line.

Done

https://review.coreboot.org/c/coreboot/+/85842/comment/17b1b694_51e4a75c?usp... :
PS1, Line 167: #define CHN_MASK			(BIT(CHN_DCXO_L) | BIT(CHN_DCXO_H))
             : #define CHN_NOT_SUPPORT(i)		(BIT(i) & CHN_MASK)
> For `CHN_NOT_SUPPORT`, `INIT_SUBSYS_FPM2LPM` and `INIT_SUBSYS_TO_HW`, can we change them to an confi […]

Done

https://review.coreboot.org/c/coreboot/+/85842/comment/554b6665_250a94a5?usp... :
PS1, Line 182: *
> space

Done

https://review.coreboot.org/c/coreboot/+/85842/comment/0d8dd9d7_9995c0c0?usp... :
PS1, Line 203: XO_DEFAULT_STABLE_TIME, REQ_ACK_IMD_EN
> `XO_DEFAULT_STABLE_TIME, REQ_ACK_IMD_EN` can be moved to the `SUB_CTRL_CON_UNUSE` macro definition.

Done

https://review.coreboot.org/c/coreboot/+/85842/comment/eee71e64_86f1eaaa?usp... :
PS1, Line 225: RG
> What does RG mean?

RG means register.

https://review.coreboot.org/c/coreboot/+/85842/comment/04a38988_06ab6292?usp... :
PS1, Line 265: code flow ASSERT already
> What does this mean?

This code flow will call ASSERT(). If ASSERT is ready for checking that chn is correct, it is not necessary to print RC_Mxx_REQ_STA_0.

https://review.coreboot.org/c/coreboot/+/85842/comment/9541f3d0_4861bab5?usp... :
PS1, Line 273: ASSERT
> assert […]

Done

https://review.coreboot.org/c/coreboot/+/85842/comment/e3cc2b22_d271dd7c?usp... :
PS1, Line 276: INIT_MODE
> Can we split `INIT_MODE` to a separate function `rc_ctrl_mode_init`? Then `INIT_MODE` enum value can […]

Done

https://review.coreboot.org/c/coreboot/+/85842/comment/5ce7f4dc_bac08897?usp... :
PS1, Line 278: rc_ctrl[id]
> Use a local variable (const pointer) for `rc_ctrl[id]`.

Done

https://review.coreboot.org/c/coreboot/+/85842/comment/61c32d97_0eea0b57?usp... :
PS1, Line 312: else
             : 		return;
>

Done

https://review.coreboot.org/c/coreboot/+/85842/comment/8fea1719_561e209c?usp... :
PS1, Line 315: 	rc_ctrl[id].sw_fpm = mode;
> Can we change `rc_ctrl` const and NOT modify it here? In line #511, we can still get the current fpm […]

Done

https://review.coreboot.org/c/coreboot/+/85842/comment/3fdd641a_aa365e02?usp... :
PS1, Line 346: rc_ctrl_mode_switch_init
> Adjust the order of these 3 functions: rc_ctrl_mode_switch_init -> rc_init_subsys_lpm -> rc_init_sub […]

Done

https://review.coreboot.org/c/coreboot/+/85842/comment/7ae11179_46cde0a6?usp... :
PS1, Line 357: unsigned int state
> `u32 mask, u32 value` […]

Done

https://review.coreboot.org/c/coreboot/+/85842/comment/5a51c2ba_70416e13?usp... :
PS1, Line 357: unsigned int chn
> `enum chn_id id`

Done

https://review.coreboot.org/c/coreboot/+/85842/comment/007df647_672509c2?usp... :
PS1, Line 360: INFO
> ERROR

Done

https://review.coreboot.org/c/coreboot/+/85842/comment/0625c1b4_f6435e34?usp... :
PS1, Line 378: e
> E

Done

https://review.coreboot.org/c/coreboot/+/85842/comment/37648e42_4feaf470?usp... :
PS1, Line 401: e
> E

Done

https://review.coreboot.org/c/coreboot/+/85842/comment/4abba025_3bf450cd?usp... :
PS1, Line 414: _BF_VALUE
> `_BF_VALUE` isn't part of the API, so it should never be used. Please use `WRITE32_BITFIELDS`.

Done

https://review.coreboot.org/c/coreboot/+/85842/comment/65e89c36_de5c6e87?usp... :
PS1, Line 477: u
> U

Done

https://review.coreboot.org/c/coreboot/+/85842/comment/d0c4b5c0_db155c44?usp... :
PS1, Line 514: ret
> Should return -1 at the end of function if one of the channel fails. […]

Done

https://review.coreboot.org/c/coreboot/+/85842/comment/5fd64450_3b0d854d?usp... :
PS1, Line 515: 		ASSERT(!ret);
> We shouldn't use assertions for run-time errors. […]

Done

https://review.coreboot.org/c/coreboot/+/85842/comment/d55ee210_03d480ba?usp... :
PS1, Line 537: w
> W

Done