Attention is currently required from: Hung-Te Lin, Angel Pons, Wenbin Mei, Yu-Ping Wu. Wenbin Mei 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 11:
(6 comments)
File src/soc/mediatek/common/include/soc/msdc.h:
https://review.coreboot.org/c/coreboot/+/51966/comment/c8f467e0_479be9e5 PS10, Line 154: Io
IO
Ack
https://review.coreboot.org/c/coreboot/+/51966/comment/623c7d82_91f5a6cb PS10, Line 155: io
IO
Ack
File src/soc/mediatek/common/include/soc/msdc.h:
https://review.coreboot.org/c/coreboot/+/51966/comment/ced93769_de2a0bd4 PS9, Line 11:
Use tabs for alignment?
Done
File src/soc/mediatek/common/include/soc/msdc.h:
https://review.coreboot.org/c/coreboot/+/51966/comment/102e4774_bfb6198f PS5, Line 18: #define MSDC_CFG 0x0 : #define MSDC_IOCON 0x04 : #define MSDC_PS 0x08 : #define MSDC_INT 0x0c : #define MSDC_INTEN 0x10 : #define MSDC_FIFOCS 0x14 : #define MSDC_TXDATA 0x18 : #define MSDC_RXDATA 0x1c : #define SDC_CFG 0x30 : #define SDC_CMD 0x34 : #define SDC_ARG 0x38 : #define SDC_STS 0x3c : #define SDC_RESP0 0x40 : #define SDC_RESP1 0x44 : #define SDC_RESP2 0x48 : #define SDC_RESP3 0x4c : #define SDC_BLK_NUM 0x50 : #define SDC_ADV_CFG0 0x64 : #define EMMC_IOCON 0x7c : #define SDC_ACMD_RESP 0x80 : #define DMA_SA_H4BIT 0x8c : #define MSDC_DMA_SA 0x90 : #define MSDC_DMA_CTRL 0x98 : #define MSDC_DMA_CFG 0x9c : #define MSDC_PATCH_BIT 0xb0 : #define MSDC_PATCH_BIT1 0xb4 : #define MSDC_PATCH_BIT2 0xb8 : #define MSDC_PAD_TUNE 0xec : #define MSDC_PAD_TUNE0 0xf0 : #define PAD_DS_TUNE 0x188 : #define PAD_CMD_TUNE 0x18c : #define EMMC51_CFG0 0x204 : #define EMMC50_CFG0 0x208 : #define EMMC50_CFG1 0x20c : #define EMMC50_CFG3 0x220 : #define SDC_FIFO_CFG 0x228 :
Will you consider changing this to a struct? […]
If we do that, we need to reserve some memory. I think the current way is better.
https://review.coreboot.org/c/coreboot/+/51966/comment/66d0a29b_ce5da832 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?
MSDC_FIFOCS_TXCNT is from bit16 to bit23, not contains bit31.
File src/soc/mediatek/common/msdc.c:
https://review.coreboot.org/c/coreboot/+/51966/comment/3c55c897_aa513921 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?
Because we don't care the result, so don't need return an error.