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

[Yu-Ping Wu (Code Review)]

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

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

(28 comments)

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

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

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

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

https://review.coreboot.org/c/coreboot/+/85842/comment/eb439e7e_552fdaf7?usp... :
PS1, Line 202: 		
Remove 2 tabs

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

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

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

https://review.coreboot.org/c/coreboot/+/85842/comment/366496ff_73764c68?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 configuration array?

```
static const struct {
    bool disabled;
    bool lpm;
    bool hw_mode;
} rc_config[MAX_CHN_NUM] = {
    [CHN_SUSPEND] = ...,
    ...
    [CHN_GPS] = { .lpm = true, .hw_mode = true },
    ...
    [CHN_DCXO_L] = { .disabled = true },
    ...
};
```

Alternatively, we can change them to 3 functions: `chn_supported()`, `chn_is_lpm()`, `chn_is_hw_mode()`.

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

https://review.coreboot.org/c/coreboot/+/85842/comment/0844bc36_2697c832?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.

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

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

https://review.coreboot.org/c/coreboot/+/85842/comment/e662b25b_3472fd44?usp... :
PS1, Line 273: ASSERT
assert

Same below.

https://review.coreboot.org/c/coreboot/+/85842/comment/d598e4fa_bdb64af2?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 also be removed.

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

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

```
assert(mode == SW_FPM_HIGH || mode == SW_FPM_LOW);

int fpm = SW_FPM_HIGH ? 1 : 0;
SET32_BITFIELDS(&rc_regs->rc_mxx_srclken_cfg[id], SW_SRCLKEN_FPM, fpm);
```

https://review.coreboot.org/c/coreboot/+/85842/comment/c194ee59_ef16db2b?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 value by:

```
unsigned int fpm = rc_ctrl[ch].sw_fpm;
if (rc_config[ch].lpm)
    fpm = SW_FPM_LOW;
```

https://review.coreboot.org/c/coreboot/+/85842/comment/216d58b2_f7bce42b?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_subsys_hw_mode.

https://review.coreboot.org/c/coreboot/+/85842/comment/9f5ed530_e5fa57e2?usp... :
PS1, Line 357: unsigned int state
`u32 mask, u32 value`

Pass `mask` (0xA) from the caller.

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

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

https://review.coreboot.org/c/coreboot/+/85842/comment/62a784c7_8a4ca796?usp... :
PS1, Line 378: e
E

https://review.coreboot.org/c/coreboot/+/85842/comment/42dee60b_072b1dc7?usp... :
PS1, Line 385: dsb
> coreboot read/write APIs already include `dmb()`. […]

I have the same question.

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

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

https://review.coreboot.org/c/coreboot/+/85842/comment/1325ab0a_dd6f3563?usp... :
PS1, Line 477: u
U

https://review.coreboot.org/c/coreboot/+/85842/comment/cc48333e_bd1caf13?usp... :
PS1, Line 514: ret
Should return -1 at the end of function if one of the channel fails. Right now we store the return value of the *last* channel, which is wrong.

https://review.coreboot.org/c/coreboot/+/85842/comment/50f91f98_46dcac2f?usp... :
PS1, Line 515: 		ASSERT(!ret);
We shouldn't use assertions for run-time errors. When this fails, should we continue to boot or not?

1. If so, then return -1 here.
2. If not, call `die("...")`.

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