[coreboot-gerrit] [XL] Change in coreboot[main]: soc/meidatek/mt8196: Add SPM loader

[Yu-Ping Wu (Code Review)]

Attention is currently required from: Ccong Chen, Hung-Te Lin, Wenzhen Yu.

Yu-Ping Wu has posted comments on this change by Ccong Chen. ( https://review.coreboot.org/c/coreboot/+/85599?usp=email )

Change subject: soc/meidatek/mt8196: Add SPM loader
......................................................................

Patch Set 1:

(27 comments)

File src/soc/mediatek/common/Kconfig:

https://review.coreboot.org/c/coreboot/+/85599/comment/e069fada_d0f5aafe?usp... :
PS1, Line 122: spm version2
SPM version 2

File src/soc/mediatek/common/spm.c:

https://review.coreboot.org/c/coreboot/+/85599/comment/45085fe6_47b49329?usp... :
PS1, Line 71: 	/* In the new SOC design, this part has been simplified */
Remove this comment (same below). The difference should be explained in the commit message and the Kconfig file instead.

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

https://review.coreboot.org/c/coreboot/+/85599/comment/0425c294_9b555ef1?usp... :
PS1, Line 146: SCP_BASE
Just use `SPM_BASE`. Also see CB:85521.

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

https://review.coreboot.org/c/coreboot/+/85599/comment/c238f838_ea691eb5?usp... :
PS1, Line 140: _0
Why `_0`? How many config registers do we have? If we have 8, please write `clk_scp_cfg[8]` here and remove `reserved3`.

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

https://review.coreboot.org/c/coreboot/+/85599/comment/d4de2bbe_a77322a9?usp... :
PS1, Line 12: 0x10001060
`(INFRACFG_AO_BASE + ...)`

https://review.coreboot.org/c/coreboot/+/85599/comment/a780e9c6_e3db69ea?usp... :
PS1, Line 13: 0x1c001108
Should we define a structure for `VLPCFG_BASE`?

https://review.coreboot.org/c/coreboot/+/85599/comment/3cea9617_7537c318?usp... :
PS1, Line 69: 0x003FFE20
Keep it lower case in this patch. We should change all of these to upper case in a separate patch.

https://review.coreboot.org/c/coreboot/+/85599/comment/ce5f0965_72678e4e?usp... :
PS1, Line 85: PCM_WDT_TIMEOUT
What's the unit?

https://review.coreboot.org/c/coreboot/+/85599/comment/7fdd53dd_e9950655?usp... :
PS1, Line 87: AP_WDT_TIMEOUT_SUSPEND
Append unit such as `_MS`.

https://review.coreboot.org/c/coreboot/+/85599/comment/ab3de88e_ae1242bd?usp... :
PS1, Line 201: 	u8 reg_apifr_mem_apsrc_req_mask_b;
Is the original register definition wrong? If that's the case, please split the "fixing" part into a separate patch.

https://review.coreboot.org/c/coreboot/+/85599/comment/499f236b_68660876?usp... :
PS1, Line 635: 	u32 reserved0[5];
Revert these unrelated changes.

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

https://review.coreboot.org/c/coreboot/+/85599/comment/2bda3f63_6c18d874?usp... :
PS1, Line 10:   
one space only

https://review.coreboot.org/c/coreboot/+/85599/comment/0c7b549b_727d22c1?usp... :
PS1, Line 17: 
remove extra blank lnie.

https://review.coreboot.org/c/coreboot/+/85599/comment/307d4001_5b343027?usp... :
PS1, Line 26: /* TODO: replace with common regs structure if they have been defined */
Remove this comment if common structures are not defined already.

https://review.coreboot.org/c/coreboot/+/85599/comment/2196756a_65fd484d?usp... :
PS1, Line 731: (
remove

https://review.coreboot.org/c/coreboot/+/85599/comment/bd073c0f_68823a6b?usp... :
PS1, Line 735: (
remove

https://review.coreboot.org/c/coreboot/+/85599/comment/300ff288_f86d6ccb?usp... :
PS1, Line 738: status
Remove if not used.

https://review.coreboot.org/c/coreboot/+/85599/comment/0c147d9e_15b4a986?usp... :
PS1, Line 738: en
Do you mean `enable`?

https://review.coreboot.org/c/coreboot/+/85599/comment/d9632b33_f0d305c8?usp... :
PS1, Line 738: int
bool

https://review.coreboot.org/c/coreboot/+/85599/comment/44dde7ad_1f52dc3c?usp... :
PS1, Line 740: unsigned int
u32

https://review.coreboot.org/c/coreboot/+/85599/comment/1a5615de_4cc60a9e?usp... :
PS1, Line 743: clrbits32(&mtk_spm->spm_ack_chk_con_3, SPM_ACK_CHK_3_CON_CLR_ALL);
             : 		setbits32(&mtk_spm->spm_ack_chk_con_3, SPM_ACK_CHK_3_CON_EN);
Can this be done in one `clrsetbits32` call?

https://review.coreboot.org/c/coreboot/+/85599/comment/9327a057_d59c8b23?usp... :
PS1, Line 755: SPM_ACK_CHK_3_CON_HW_MODE_TRIG
Align

https://review.coreboot.org/c/coreboot/+/85599/comment/59ca9edb_84575848?usp... :
PS1, Line 782: (
Space before `(`

https://review.coreboot.org/c/coreboot/+/85599/comment/bd8a5acc_c9a5d95b?usp... :
PS1, Line 791: i
I

https://review.coreboot.org/c/coreboot/+/85599/comment/82edbb2a_9e70f6dd?usp... :
PS1, Line 794: (
remove

https://review.coreboot.org/c/coreboot/+/85599/comment/245efb43_682b82c4?usp... :
PS1, Line 797: (
remove

https://review.coreboot.org/c/coreboot/+/85599/comment/725aac4c_a48cd688?usp... :
PS1, Line 861: 		SPM_REGWR_CFG_KEY | REG_SPM_APB_INTERNAL_EN_LSB |
Align