Change in coreboot[master]: soc/mediatek/mt8195: Add APU device apc driver

[Yu-Ping Wu (Code Review)]

Attention is currently required from: Hung-Te Lin, Rex-BC Chen, [e-mail].
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58969 )

Change subject: soc/mediatek/mt8195: Add APU device apc driver
......................................................................

Patch Set 2:

(19 comments)

File src/soc/mediatek/mt8195/apusys_devapc.c:

https://review.coreboot.org/c/coreboot/+/58969/comment/5920c684_bab534e4 
PS2, Line 58: APUSYS_NOC_DAPC_AO
Lower case name.

https://review.coreboot.org/c/coreboot/+/58969/comment/43af17bd_d8c02738 
PS2, Line 78: APUSYS_AO_Devices
Lower case name.

https://review.coreboot.org/c/coreboot/+/58969/comment/7eedbd80_f7e4f820 
PS2, Line 166: 
_Static_assert(ARRAY_SIZE(APUSYS_AO_Devices) == 71);

https://review.coreboot.org/c/coreboot/+/58969/comment/a670c2e6_f878d921 
PS2, Line 179: 0x%x
%#x

Same for all occurrences in this file.

https://review.coreboot.org/c/coreboot/+/58969/comment/994dac5a_45e8e017 
PS2, Line 181: ret = APUSYS_APC_ERR_PERMISSION_NOT_SUPPORTED;
             : 		goto exit;
return APUSYS_APC_ERR_PERMISSION_NOT_SUPPORTED;

https://review.coreboot.org/c/coreboot/+/58969/comment/f7358998_ef338d60 
PS2, Line 191: if (slave < APUSYS_NOC_DAPC_AO_SLAVE_NUM &&
             : 	    domain_id < APUSYS_NOC_DAPC_AO_DOM_NUM) {
             : 		base = (u32 *)((size_t)APUSYS_NOC_DAPC_AO_BASE +
             : 			       domain_id * 0x40 + apc_register_index * 4);
             : 		apuapc_writel(apuapc_readl(base) & clr_bit, base);
             : 		apuapc_writel(apuapc_readl(base) | set_bit, base);
             : 		ret = APUSYS_APC_OK;
             : 	} else {
             : 		printk(BIOS_DEBUG, "[NOC_DAPC] %s: %s, %s:0x%x, %s:0x%x\n",
             : 		       __func__, "out of boundary", "slave", slave, "domain_id",
             : 		       domain_id);
             : 		ret = APUSYS_APC_ERR_OUT_OF_BOUNDARY;
             : 	}
Rewrite as

if (slave >= APUSYS_NOC_DAPC_AO_SLAVE_NUM) {
     printk(BIOS_ERR, "[NOC_DAPC] %s: slave out of boundary: %#x", __func__, slave);
     return APUSYS_APC_ERR_OUT_OF_BOUNDARY;
 }

if (domain_id >= ...) {
     ...
 }

base = ...;
 ...;
 return APUSYS_APC_OK;

https://review.coreboot.org/c/coreboot/+/58969/comment/911e8f13_47c5c2c1 
PS2, Line 205: exit:
             : 	return ret;
Remove

https://review.coreboot.org/c/coreboot/+/58969/comment/8e8a6404_77c2b16e 
PS2, Line 217: <=
Why is this "<=" instead of "<"? Should we define reg_num as

DIV_ROUND_UP(APUSYS_NOC_DAPC_AO_SLAVE_NUM,
              APUSYS_NOC_DAPC_AO_SLAVE_NUM_IN_1_DOM)

https://review.coreboot.org/c/coreboot/+/58969/comment/c5f5ef78_8cccc5bd 
PS2, Line 231:  = NULL
No need to initialize.

https://review.coreboot.org/c/coreboot/+/58969/comment/70d8c67e_72260ba8 
PS2, Line 239: ret = APUSYS_APC_ERR_PERMISSION_NOT_SUPPORTED;
             : 		goto exit;
return APUSYS_APC_ERR_PERMISSION_NOT_SUPPORTED

https://review.coreboot.org/c/coreboot/+/58969/comment/90e84300_045098e3 
PS2, Line 249: if (slave < APUSYS_APC_SYS0_AO_SLAVE_NUM &&
             : 	    domain_id < APUSYS_APC_SYS0_AO_DOM_NUM) {
             : 		base = (u32 *)((size_t)APUSYS_APC_AO_BASE + domain_id * 0x40 +
             : 			       apc_register_index * 4);
             : 		apuapc_writel(apuapc_readl(base) & clr_bit, base);
             : 		apuapc_writel(apuapc_readl(base) | set_bit, base);
             : 		ret = APUSYS_APC_OK;
             : 	} else {
             : 		printk(BIOS_DEBUG, "[APUAPC] %s: %s, %s:0x%x, %s:0x%x\n",
             : 		       __func__, "out of boundary", "slave", slave, "domain_id",
             : 		       domain_id);
             : 		ret = APUSYS_APC_ERR_OUT_OF_BOUNDARY;
             : 	}
             : 
             : exit:
             : 	return ret;
Same as set_slave_noc_dapc(). Please rewrite.

https://review.coreboot.org/c/coreboot/+/58969/comment/fd0426bc_00557430 
PS2, Line 275: <=
Same.

https://review.coreboot.org/c/coreboot/+/58969/comment/6fd9db52_baa7ecda 
PS2, Line 377: set_bit
Write the value inline:

BIT(APUSYS_APC_SYS0_LOCK_BIT_APU_SCTRL_REVISER) | 
 BIT(APUSYS_APC_SYS0_LOCK_BIT_DEVAPC_AO_WRAPPER)

File src/soc/mediatek/mt8195/include/soc/apusys_devapc_def.h:

https://review.coreboot.org/c/coreboot/+/58969/comment/21319240_9a454922 
PS2, Line 10: APUSYS_APC_ERR_STATUS
Lower case name.

https://review.coreboot.org/c/coreboot/+/58969/comment/08094c62_0dce2396 
PS2, Line 13: 0x1000
Why use 0x1000 instead of 1 for errors? Are these values copied from somewhere?

https://review.coreboot.org/c/coreboot/+/58969/comment/c8066729_0bed65e3 
PS2, Line 23: APUSYS_APC_PERM_TYPE
Lower case name.

https://review.coreboot.org/c/coreboot/+/58969/comment/c7a4b166_158419e4 
PS2, Line 31: APUSYS_APC_DOMAIN_ID
Lower case name.

https://review.coreboot.org/c/coreboot/+/58969/comment/899ee89d_5c5b578b 
PS2, Line 50: APC_DOM_16
Lower case name.

https://review.coreboot.org/c/coreboot/+/58969/comment/7b641ad4_8023a3b2 
PS2, Line 51: unsigned char d0_permission;
            : 	unsigned char d1_permission;
            : 	unsigned char d2_permission;
            : 	unsigned char d3_permission;
            : 	unsigned char d4_permission;
            : 	unsigned char d5_permission;
            : 	unsigned char d6_permission;
            : 	unsigned char d7_permission;
            : 	unsigned char d8_permission;
            : 	unsigned char d9_permission;
            : 	unsigned char d10_permission;
            : 	unsigned char d11_permission;
            : 	unsigned char d12_permission;
            : 	unsigned char d13_permission;
            : 	unsigned char d14_permission;
            : 	unsigned char d15_permission;
Why not use an array?

unsigned char d_permission[16];