Attention is currently required from: Angel Pons, Wenbin Mei, Yu-Ping Wu. Hung-Te Lin 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 6:
(28 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/51966/comment/ba4ae302_849bd7ad PS2, Line 7: mmc: Add MTK mmc driver support
Done
Ack
File src/mainboard/google/asurada/mainboard.c:
https://review.coreboot.org/c/coreboot/+/51966/comment/43687879_c30e8457 PS1, Line 233: struct msdc_ctrlr msdc_host;
Ack
Ack
https://review.coreboot.org/c/coreboot/+/51966/comment/47f3e0bd_105c101e PS1, Line 243: (void *)MSDC0_BASE, (void *)MSDC0_TOP_BASE
Done
Ack
https://review.coreboot.org/c/coreboot/+/51966/comment/5b4e2a96_e72d6d39 PS1, Line 247: 400*1000
Yes, 400KHz is init clock, and all eMMCs will support it.
Ack
File src/soc/mediatek/common/include/soc/msdc.h:
https://review.coreboot.org/c/coreboot/+/51966/comment/0f562ebf_60e0b6d2 PS6, Line 97: SDC_CMD_DTYPE_S And if you use bit fields...
DEFINE_BITFIELD(SDC_CMD_DTYPE, 12, 11) DEFINE_BIT(SDC_CMD_WD, 13) DEFINE_BIT(SDC_CMD_STOP, 14) DEFINE_BITFIELD(SDC_CMD_BLK_LEN, 27, 16) DEFINE_BITFIELD(SDC_CMD_CMD, 5, 0)
The numbers should align to the datasheet bit definition.
However, I also saw how these macros were used in prepare_cmd, so it's probably fine to keep how you implement them today.
https://review.coreboot.org/c/coreboot/+/51966/comment/0fc9f196_63a90fdb PS6, Line 142: 5*100 5 * 100
https://review.coreboot.org/c/coreboot/+/51966/comment/61ace8f3_a835748f PS6, Line 143: 1000*1000 1000 * 1000
File src/soc/mediatek/common/include/soc/msdc.h:
https://review.coreboot.org/c/coreboot/+/51966/comment/66e24d56_90219283 PS5, Line 25:
Please avoid mixing tabs and spaces to align the values. […]
Ack
https://review.coreboot.org/c/coreboot/+/51966/comment/e0992a87_3d4ae9dc PS5, Line 62: /*--------------------------------------------------------------------------*/ : /* Register Mask */ : /*--------------------------------------------------------------------------*/
will you consider redefining all the masks, using DEFINE_BITFIELDS ?
Ack
https://review.coreboot.org/c/coreboot/+/51966/comment/98e1d1e9_20a2358c PS5, Line 102: #define SDC_CMD_CMD_S 0
If I understand correctly, the `_S` suffix means shift, and the `_M` suffix means mask. […]
Ack
https://review.coreboot.org/c/coreboot/+/51966/comment/1feb3b23_ba0574b8 PS5, Line 167: int
nit: bool?
Ack
File src/soc/mediatek/common/msdc.c:
https://review.coreboot.org/c/coreboot/+/51966/comment/28b9a777_d165063a PS2, Line 70: udelay(1);
Ack
Ack
https://review.coreboot.org/c/coreboot/+/51966/comment/118a50e7_9a760c85 PS2, Line 72: MSDC_NOT_READY
Done
Ack
https://review.coreboot.org/c/coreboot/+/51966/comment/d99e8640_54fc1848 PS2, Line 349: host
We need "msdc_ctrlr *" pointer as first parameter for the "msdc_start_command" function, so it shoul […]
I think it's a pretty bad idea to recast (struct sd_mmc_ctrlr *) to (struct msdc_ctrlr *). That's simply making the implementation pretty confusing. Yes I know sdhci.c is doing this but let's try not making more bad examples.
Can you replace these with container_of macro?
Take a look at src/soc/ti/am335x/mmc.c to see how this can be properly done without casting.
https://review.coreboot.org/c/coreboot/+/51966/comment/ca877dd4_1172ebf0 PS2, Line 385: clrsetbits_le32
Yes
Ack
File src/soc/mediatek/common/msdc.c:
https://review.coreboot.org/c/coreboot/+/51966/comment/ec840ebb_ac9a4319 PS5, Line 12: int
unsigned int
Ack
https://review.coreboot.org/c/coreboot/+/51966/comment/4df83794_ac42194b PS5, Line 14: static inline u32 div_round_up(u32 n, u32 d) { return (n + d - 1) / d; }
There's a DIV_ROUND_UP macro in commonlib/bsd/helpers. […]
Ack
https://review.coreboot.org/c/coreboot/+/51966/comment/e90ee246_8a425669 PS5, Line 22: write32(reg, tv);
This could be replaced with: […]
Ack
https://review.coreboot.org/c/coreboot/+/51966/comment/71cad540_616c786a PS5, Line 33: int
u32?
Ack
https://review.coreboot.org/c/coreboot/+/51966/comment/0595b4df_d9ca934e PS5, Line 45: return MSDC_SUCCESS;
Also see the wait_us() macro.
Ack
https://review.coreboot.org/c/coreboot/+/51966/comment/7077d4f5_73d27aa2 PS5, Line 57: while (!(reg & mask)) {
nit: I would use a do-while loop like on `msdc_poll_timeout`
Ack
https://review.coreboot.org/c/coreboot/+/51966/comment/89fd6504_68376efd PS5, Line 61: *status = reg;
Missing a null pointer check? […]
Ack
https://review.coreboot.org/c/coreboot/+/51966/comment/14368656_fc61fdda PS5, Line 162: resp = 0x1;
nit: you could return the value directly
Ack
https://review.coreboot.org/c/coreboot/+/51966/comment/1240323b_b6379787 PS5, Line 243: ((dtype << SDC_CMD_DTYPE_S) & SDC_CMD_DTYPE_M);
I'd suggest splitting this in multiple statements: […]
Ack
https://review.coreboot.org/c/coreboot/+/51966/comment/b9192bd0_4ee4018a PS5, Line 328: return cmd_ret;
nit: Just return `msdc_start_command(host, cmd, data)` directly?
Ack
https://review.coreboot.org/c/coreboot/+/51966/comment/cb42cc71_08379c24 PS5, Line 454: sizeof(int)
nit: sizeof(status) ?
Ack
https://review.coreboot.org/c/coreboot/+/51966/comment/4efc1bcb_74a0001e PS5, Line 477: memset(&media, 0, sizeof(media));
You can zero-initialize media without memset: […]
Ack
File src/soc/mediatek/mt8192/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/51966/comment/038b2897_eddfe5b0 PS5, Line 55: ramstage-y += ../common/msdc.c
this change should go to next patch, when you modify 8192 config to add msdc.
Ack
-
Hung-Te Lin (Code Review)