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

[Ccong Chen (Code Review)]

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

Ccong Chen 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 2:

(42 comments)

File src/soc/mediatek/common/Kconfig:

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

Done

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

https://review.coreboot.org/c/coreboot/+/85599/comment/ec1bc60a_6d18de24?usp... :
PS1, Line 59: #ifndef CONFIG_MEDIATEK_SPM_V2
> if (!CONFIG(MEDIATEK_SPM_V2)) […]

In 8196 SOC design, this part has been simplified, no need the part of setting

https://review.coreboot.org/c/coreboot/+/85599/comment/2e83276c_5c201551?usp... :
PS1, Line 71: 	/* In the new SOC design, this part has been simplified */
> Remove this comment (same below). […]

Done

https://review.coreboot.org/c/coreboot/+/85599/comment/87a8e788_e5da0653?usp... :
PS1, Line 101: CONFIG_MEDIATEK_SPM_V2
> if (!CONFIG(MEDIATEK_SPM_V2))

8196 have not the register setting

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

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

Done

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

https://review.coreboot.org/c/coreboot/+/85599/comment/b9efee56_fbbfdbda?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 […]

Done

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

https://review.coreboot.org/c/coreboot/+/85599/comment/5234fbb1_b1a64f02?usp... :
PS1, Line 508: u32 spm_power_on_val[4];
> Keep it unmodified.

Done

https://review.coreboot.org/c/coreboot/+/85599/comment/52ea792e_9151df29?usp... :
PS1, Line 535: u32 md32pcm_scu_ctrl[4];
> Keep it unmodified.

Done

https://review.coreboot.org/c/coreboot/+/85599/comment/c1978c0a_a279970a?usp... :
PS1, Line 562: u32 sw2spm_mailbox[4];
             : 	u32 spm2sw_mailbox[4];
             :
> Keep it unmodified.

Done

https://review.coreboot.org/c/coreboot/+/85599/comment/217ced40_ec0b3bea?usp... :
PS1, Line 575: 	u32 pcm_apwdt_latch[20];
> Keep it unmodified.

apwdt register address have been changed

https://review.coreboot.org/c/coreboot/+/85599/comment/c2ad26e5_5ed6606a?usp... :
PS1, Line 591: u32 spm_dpsw_con;
> Keep it unmodified.

spm_dpsw_con register address have been changed

https://review.coreboot.org/c/coreboot/+/85599/comment/f718cffb_fa313d79?usp... :
PS1, Line 645: 	struct {
             : 		u32 latch_l;
             : 		u32 latch_h;
             : 	} sys_timer_latch[16];
> Keep it unmodified.

Done

https://review.coreboot.org/c/coreboot/+/85599/comment/7777a265_89495105?usp... :
PS1, Line 654: u32 pcm_apwdt_latch_20[19];
             : 	u8  reserved18[4];
> Keep it unmodified.

The register address have been changed

https://review.coreboot.org/c/coreboot/+/85599/comment/2bc4c3d4_9c372456?usp... :
PS1, Line 660: u32 spm_sw_rsv[9];
> Keep it unmodified.

Done

https://review.coreboot.org/c/coreboot/+/85599/comment/06fbde08_68529c83?usp... :
PS1, Line 684: u32 spm_sema_m[8];
> Keep it unmodified.

Done

https://review.coreboot.org/c/coreboot/+/85599/comment/193e8568_2d2614eb?usp... :
PS1, Line 687: u32 spm2pmcu_mailbox[4];
             : 	u32 pmcu2spm_mailbox[4];
> Keep it unmodified.

Done

https://review.coreboot.org/c/coreboot/+/85599/comment/4320a50f_9aebf46b?usp... :
PS1, Line 704: u32 spm_src_mask[12];
             : 	u32 spm_req_sta[12];
> Keep it unmodified.

Done

https://review.coreboot.org/c/coreboot/+/85599/comment/b3fae9a8_141dbf2f?usp... :
PS1, Line 710: u32 spm_resource_ack_con[2];
             : 	u32 spm_resource_ack_mask[9];
> Keep it unmodified.

Done

https://review.coreboot.org/c/coreboot/+/85599/comment/34773737_525b264b?usp... :
PS1, Line 12: 0x10001060
> `(INFRACFG_AO_BASE + ... […]

Done

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

Done

https://review.coreboot.org/c/coreboot/+/85599/comment/dde1e689_d7752042?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.

Done

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

Done

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

Done

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

Done

https://review.coreboot.org/c/coreboot/+/85599/comment/c5f4e386_cb655e66?usp... :
PS1, Line 717: [1]
> remove `[1]`

Done

https://review.coreboot.org/c/coreboot/+/85599/comment/43dcd639_48ee9ec0?usp... :
PS1, Line 968: 	u32 spm_src_mask_17;
             : 	u32 spm_src_mask_18;
             : 	u32 spm_resource_ack_mask_13;
             : 	u32 spm_resource_ack_mask_14;
> The offsets are varied previous version. Please make sure the offsets are correct.

Yes, it's correct

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

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

Done

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

Done

https://review.coreboot.org/c/coreboot/+/85599/comment/e993c015_41857bc7?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.

Done

https://review.coreboot.org/c/coreboot/+/85599/comment/4f8aa46a_e255b7a9?usp... :
PS1, Line 731: (
> remove

Done

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

Done

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

Done

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

Done

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

Done

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

Done

https://review.coreboot.org/c/coreboot/+/85599/comment/353b16fb_93328b5f?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?

Done

https://review.coreboot.org/c/coreboot/+/85599/comment/480f63a2_775255b2?usp... :
PS1, Line 755: SPM_ACK_CHK_3_CON_HW_MODE_TRIG
> Align

Done

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

Done

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

Done

https://review.coreboot.org/c/coreboot/+/85599/comment/53270e4c_a9356088?usp... :
PS1, Line 794: (
> remove

Done

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

Done

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

align line742 ?