Attention is currently required from: Hung-Te Lin, Paul Menzel, Arthur Heymans, Wenbin Mei. Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/51966 )
Change subject: soc/mediatek: add new driver 'msdc' for eMMC ......................................................................
Patch Set 9:
(9 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/51966/comment/73450453_32022590 PS3, Line 10:
Done
Ack
Commit Message:
https://review.coreboot.org/c/coreboot/+/51966/comment/ff023d19_f1f5c742 PS9, Line 10: support supports
File src/soc/mediatek/common/include/soc/msdc.h:
https://review.coreboot.org/c/coreboot/+/51966/comment/8307cc53_67b2448d PS9, Line 136: S s
File src/soc/mediatek/common/include/soc/msdc.h:
https://review.coreboot.org/c/coreboot/+/51966/comment/76d44f76_945e3ce7 PS7, Line 6: #include <commonlib/sd_mmc_ctrlr.h> : #include <commonlib/storage.h> : #include <commonlib/storage/sd_mmc.h> : #include <console/console.h> : #include <device/mmio.h> : #include <lib.h> : #include <stdio.h> : #include <string.h>
Please only include what you need in the header and include the rest in the . […]
Done
File src/soc/mediatek/common/include/soc/msdc.h:
https://review.coreboot.org/c/coreboot/+/51966/comment/e4fdf025_e7079165 PS5, Line 96: #define SDC_CMD_WR BIT(13)
nit: For consistency with the other macros, I wouldn't use `BIT(x)` here: […]
Done
https://review.coreboot.org/c/coreboot/+/51966/comment/4f582843_4ec2b556 PS5, Line 143: #define MSDC_TIMEOUT (1000*1000) /* 1S */
I would add a `_MS` or `_US` suffix to indicate the units the value is expressed in: […]
Done
File src/soc/mediatek/common/msdc.c:
https://review.coreboot.org/c/coreboot/+/51966/comment/366d48d2_723cdd70 PS2, Line 349: host
I think it's a pretty bad idea to recast (struct sd_mmc_ctrlr *) to (struct msdc_ctrlr *). […]
Sorry, my fault. I thought host and ctrlr had the same type.
Done
File src/soc/mediatek/common/msdc.c:
https://review.coreboot.org/c/coreboot/+/51966/comment/497166ff_c06eafdc PS5, Line 269: if (cmd->cmdidx != MMC_CMD_AUTO_TUNING_SEQUENCE)
nit: I'd add braces here. […]
Done
File src/soc/mediatek/common/msdc.c:
https://review.coreboot.org/c/coreboot/+/51966/comment/0f15ae8d_e756b7c0 PS7, Line 380: (struct msdc_ctrlr *)ctrlr;
this should also be container_of
Done