Attention is currently required from: Hung-Te Lin, Paul Menzel, Wenbin Mei, Yu-Ping Wu. Angel Pons 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 5: Code-Review+1
(21 comments)
File src/soc/mediatek/common/include/soc/msdc.h:
https://review.coreboot.org/c/coreboot/+/51966/comment/bbdaaf07_8b7b5d57 PS5, Line 25: Please avoid mixing tabs and spaces to align the values. Either tabs or spaces, but not both
https://review.coreboot.org/c/coreboot/+/51966/comment/111b8148_3764204f PS5, Line 66: /* MSDC_CFG mask */ I usually place bitfield definitions right after the corresponding register offset define, but I like this approach too. 😊
(No action needed here)
https://review.coreboot.org/c/coreboot/+/51966/comment/c41cbccb_bb3fe60e PS5, Line 84: #define MSDC_FIFOCS_CLR (0x1 << 31) /* RW */ The `MSDC_FIFOCS_TXCNT` mask already contains bit 31 (`MSDC_FIFOCS_CLR`), is this intentional?
https://review.coreboot.org/c/coreboot/+/51966/comment/a5861fb0_48a9bc99 PS5, Line 96: #define SDC_CMD_WR BIT(13) nit: For consistency with the other macros, I wouldn't use `BIT(x)` here:
#define SDC_CMD_STOP (1 << 14) #define SDC_CMD_WR (1 << 13)
https://review.coreboot.org/c/coreboot/+/51966/comment/936fcf82_e3bb33aa PS5, Line 102: #define SDC_CMD_CMD_S 0 If I understand correctly, the `_S` suffix means shift, and the `_M` suffix means mask. I would define the mask using the corresponding shift:
#define SDC_CMD_CMD_S 0 #define SDC_CMD_CMD_M (0x3f << SDC_CMD_CMD_S) #define SDC_CMD_RSPTYP_S 7 #define SDC_CMD_RSPTYP_M (0x7 << SDC_CMD_RSPTYP_S) #define SDC_CMD_DTYPE_S 11 #define SDC_CMD_DTYPE_M (0x3 << SDC_CMD_DTYPE_S) #define SDC_CMD_WR (1 << 13) #define SDC_CMD_STOP (1 << 14) #define SDC_CMD_BLK_LEN_S 16 #define SDC_CMD_BLK_LEN_M (0xfff << SDC_CMD_BLK_LEN_S)
I've also inverted the ordering, for consistency with the macros for the other registers
https://review.coreboot.org/c/coreboot/+/51966/comment/e243cd30_a070328c 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:
#define CMD_TIMEOUT_MS (5 * 100) /* 500ms */ #define MSDC_TIMEOUT_US (1000 * 1000) /* 1s */
And I think we have macros somewhere in commonlib to convert between time units, which could also be used
https://review.coreboot.org/c/coreboot/+/51966/comment/8fa0ac00_0f11fdc9 PS5, Line 167: int nit: bool?
File src/soc/mediatek/common/msdc.c:
https://review.coreboot.org/c/coreboot/+/51966/comment/9d6d02a7_4005963d PS5, Line 12: int unsigned int
https://review.coreboot.org/c/coreboot/+/51966/comment/bc89fb5b_6951eabc 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.h
https://review.coreboot.org/c/coreboot/+/51966/comment/2d694490_4321c38a PS5, Line 22: write32(reg, tv); This could be replaced with:
clrsetbits32(reg, field, val << __ffs(field));
https://review.coreboot.org/c/coreboot/+/51966/comment/98726d1b_9ce1fbaf PS5, Line 33: int u32?
https://review.coreboot.org/c/coreboot/+/51966/comment/6d4f3f4b_98176036 PS5, Line 45: return MSDC_SUCCESS; I'd suggest using the timer API (timer.h):
#include <timer.h>
static int msdc_poll_timeout(void *addr, u32 mask) { struct stopwatch timer; stopwatch_init_usecs_expire(&timer, MSDC_TIMEOUT); u32 reg; do { reg = read32(addr); if (stopwatch_expired(&timer)) return -MSDC_NOT_READY; udelay(1); } while (reg & mask); return MSDC_SUCCESS; }
https://review.coreboot.org/c/coreboot/+/51966/comment/0fb843d2_5ee3a230 PS5, Line 57: while (!(reg & mask)) { nit: I would use a do-while loop like on `msdc_poll_timeout`
https://review.coreboot.org/c/coreboot/+/51966/comment/d6348365_36bc9d13 PS5, Line 61: *status = reg; Missing a null pointer check?
if (status) *status = reg;
https://review.coreboot.org/c/coreboot/+/51966/comment/6fa05d7c_d44fcdbc PS5, Line 72: static void msdc_reset_hw(struct msdc_ctrlr *host) nit: should this function return an error if reset and FIFO clear times out?
https://review.coreboot.org/c/coreboot/+/51966/comment/634b3bb4_3aed38d0 PS5, Line 162: resp = 0x1; nit: you could return the value directly
https://review.coreboot.org/c/coreboot/+/51966/comment/93b48d88_4a79e3d6 PS5, Line 243: ((dtype << SDC_CMD_DTYPE_S) & SDC_CMD_DTYPE_M); I'd suggest splitting this in multiple statements:
rawcmd |= (opcode << SDC_CMD_CMD_S) & SDC_CMD_CMD_M; rawcmd |= (resp_type << SDC_CMD_RSPTYP_S) & SDC_CMD_RSPTYP_M; rawcmd |= (blocksize << SDC_CMD_BLK_LEN_S) & SDC_CMD_BLK_LEN_M; rawcmd |= (dtype << SDC_CMD_DTYPE_S) & SDC_CMD_DTYPE_M;
https://review.coreboot.org/c/coreboot/+/51966/comment/6a81c517_9f55ec35 PS5, Line 269: if (cmd->cmdidx != MMC_CMD_AUTO_TUNING_SEQUENCE) nit: I'd add braces here. Even though they're not strictly required because there's only one statement, the comment spans multiple lines
https://review.coreboot.org/c/coreboot/+/51966/comment/79b248f7_65993a91 PS5, Line 328: return cmd_ret; nit: Just return `msdc_start_command(host, cmd, data)` directly?
https://review.coreboot.org/c/coreboot/+/51966/comment/090691d0_ccd74b1c PS5, Line 454: sizeof(int) nit: sizeof(status) ?
https://review.coreboot.org/c/coreboot/+/51966/comment/7206d7f3_58bc1e87 PS5, Line 477: memset(&media, 0, sizeof(media)); You can zero-initialize media without memset:
struct storage_media media = { 0 };