[coreboot-gerrit] [M] Change in coreboot[main]: soc/mediatek/mt8196: Initialize MCUPM

[Jarried Lin (Code Review)]

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

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

Change subject: soc/mediatek/mt8196: Initialize MCUPM
......................................................................

Patch Set 3:

(14 comments)

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

https://review.coreboot.org/c/coreboot/+/85888/comment/b9b9e21f_ea077392?usp... :
PS2, Line 42: 1b
> What does this mean?

Done

https://review.coreboot.org/c/coreboot/+/85888/comment/986ddc02_dcc92cd4?usp... :
PS2, Line 70: REPLACE_15
> Please define a meaningful name. […]

Done

https://review.coreboot.org/c/coreboot/+/85888/comment/4574c315_221df424?usp... :
PS2, Line 84: SIZE
> Is this supposed to be `NUM`? From the comment, this is the number of slots, not the size of a slot.

Done

https://review.coreboot.org/c/coreboot/+/85888/comment/afd41235_aece24fb?usp... :
PS2, Line 93: 0x1000BC7C
> What does this stand for?

Done

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

https://review.coreboot.org/c/coreboot/+/85888/comment/62ae361f_d53a9d4f?usp... :
PS1, Line 9: #include <stdlib.h>
> `#include <console/console. […]

Done

https://review.coreboot.org/c/coreboot/+/85888/comment/e346dca3_1b59fdbf?usp... :
PS1, Line 39: 	/* set CPUEB_STATE_VALID = 1 */
            : 	setbits32p(SPM_MCUPM_SPMC_CON, CPUEB_STATE_VALID);
            : 
            : 	/* polling CPUEB_STATE_FINISH_ACK = 1 */
            : 	if (!retry(POLLING_ACK_TIME,
            : 		   (read32p(SPM_MCUPM_SPMC_CON) & CPUEB_STATE_FINISH_ACK) ==
            : 		   CPUEB_STATE_FINISH_ACK, udelay(1))) {
            : 		printk(BIOS_WARNING,
            : 		       "[EB_SPMC] Polling ACK timeout, %#x\n",
            : 		       read32p(SPM_MCUPM_SPMC_CON));
            : 		return -1;
            : 	}
            : 
            : 	/* set CPUEB_STATE_VALID = 0 */
            : 	clrbits32p(SPM_MCUPM_SPMC_CON, CPUEB_STATE_VALID);
            : 
            : 	/* polling CPUEB_STATE_FINISH_ACK = 0 */
            : 	if (!retry(POLLING_ACK_TIME,
            : 		   (read32p(SPM_MCUPM_SPMC_CON) & CPUEB_STATE_FINISH_ACK) !=
            : 		   CPUEB_STATE_FINISH_ACK, udelay(1))) {
            : 		printk(BIOS_WARNING,
            : 		       "[EB_SPMC] Polling ACK timeout, %#x\n",
            : 		       read32p(SPM_MCUPM_SPMC_CON));
            : 		return -1;
            : 	}
> How about […]

Done

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

https://review.coreboot.org/c/coreboot/+/85888/comment/243f71f1_bd09745f?usp... :
PS2, Line 16: Polling
> Poll

Done

https://review.coreboot.org/c/coreboot/+/85888/comment/a388b897_87df8b4c?usp... :
PS2, Line 20: WARNING
> Should this be `ERR`?

Done

https://review.coreboot.org/c/coreboot/+/85888/comment/d79eb647_30635d5e?usp... :
PS2, Line 46: WARNING
> ERR? Same below.

Done

https://review.coreboot.org/c/coreboot/+/85888/comment/a73266cb_c8727081?usp... :
PS2, Line 65: eb_sleep_protect
> return eb_sleep_protect();

Done

https://review.coreboot.org/c/coreboot/+/85888/comment/555bcc64_7706e476?usp... :
PS2, Line 81: %s:
> Let's use `[EB_SPMC]` for consistency.

Done

https://review.coreboot.org/c/coreboot/+/85888/comment/3fcbf8fe_24161611?usp... :
PS2, Line 85: %s
> remove

Done

https://review.coreboot.org/c/coreboot/+/85888/comment/eb0c0eb7_99b2ae8c?usp... :
PS2, Line 124: %s()
> `%s:`

Done

https://review.coreboot.org/c/coreboot/+/85888/comment/3ff97bef_691e3ba1?usp... :
PS2, Line 127: %s()
> `%s:`

Done