Attention is currently required from: Hung-Te Lin, 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 4:
(38 comments)
File src/commonlib/include/commonlib/sd_mmc_ctrlr.h:
https://review.coreboot.org/c/coreboot/+/51966/comment/47c62757_7ab5fe12 PS2, Line 15: MMC_CMD1_READY
I think you tried to use -1 as well, so let's add […]
Done
File src/mainboard/google/asurada/Kconfig:
https://review.coreboot.org/c/coreboot/+/51966/comment/5f94fa52_15018f11 PS3, Line 33: select COMMONLIB_STORAGE_MMC
Please do the board changes in a separate commit.
Done
File src/mainboard/google/asurada/mainboard.c:
https://review.coreboot.org/c/coreboot/+/51966/comment/0b47294e_b2ea2ddd PS2, Line 23: #include <commonlib/storage/sd_mmc.h> : #include <commonlib/sd_mmc_ctrlr.h> : #include <commonlib/storage.h> : #include <cbmem.h> : #include <soc/msdc.h>
Sort
Ack
https://review.coreboot.org/c/coreboot/+/51966/comment/77ba8534_8e14826c PS2, Line 33:
Align using tabs
Ack
https://review.coreboot.org/c/coreboot/+/51966/comment/2bedcb15_471850eb PS2, Line 242: i
I
Done
https://review.coreboot.org/c/coreboot/+/51966/comment/35e1b1f4_5d833dff PS2, Line 247: 400*1000
400 * 1000
Done
File src/soc/mediatek/common/include/soc/msdc.h:
https://review.coreboot.org/c/coreboot/+/51966/comment/ba6a3f7f_fb032be6 PS3, Line 2: #ifndef __MSDC_H_
Please add a blank line above.
Done
File src/soc/mediatek/common/include/soc/msdc.h:
https://review.coreboot.org/c/coreboot/+/51966/comment/b4887896_aa11dcee PS2, Line 167: min
MIN() is already defined in commonlib/bsd/include/commonlib/bsd/helpers. […]
Done
File src/soc/mediatek/common/msdc.c:
https://review.coreboot.org/c/coreboot/+/51966/comment/e854e075_298f20dd PS2, Line 24: *
Remove this
Done
https://review.coreboot.org/c/coreboot/+/51966/comment/b2a57634_15070b0d PS2, Line 25: msdc_poll_timeout -
No need for this
Done
https://review.coreboot.org/c/coreboot/+/51966/comment/b250fecc_3309c32c PS2, Line 31:
Extra blank line
Done
https://review.coreboot.org/c/coreboot/+/51966/comment/3578a001_4fa9b6bc PS2, Line 35: status
This is always NULL in all calls.
Done
https://review.coreboot.org/c/coreboot/+/51966/comment/d19b6325_e6301ef1 PS2, Line 42: timeout == 0
Use "!--timeout" as in msdc_wait_done().
Done
https://review.coreboot.org/c/coreboot/+/51966/comment/74cd12d2_74f9718b PS2, Line 43: 0x%x
%#x
Ack
https://review.coreboot.org/c/coreboot/+/51966/comment/2b8b58d5_d45fc9d1 PS2, Line 43: 0x%x
%#x
Ack
https://review.coreboot.org/c/coreboot/+/51966/comment/8d9ba35b_0b7f438d PS2, Line 58: /* This is a private function to wait for a bit mask in a given register */ : /* To avoid endless loops, a time-out is implemented here. */
Format: […]
Done
https://review.coreboot.org/c/coreboot/+/51966/comment/33f1a079_5b921e47 PS2, Line 65: if (reg & mask) : goto end;
This can be removed if we replace the do-while loop with while-do loop.
Done
https://review.coreboot.org/c/coreboot/+/51966/comment/d929b81d_d2668425 PS2, Line 77:
space
Done
https://review.coreboot.org/c/coreboot/+/51966/comment/622bf46a_a1468cc3 PS2, Line 120: MSDC_PATCH_BIT1_STOP_DLY
Align with "host->base". Same below.
Done
https://review.coreboot.org/c/coreboot/+/51966/comment/30aeb1db_a2889138 PS2, Line 129: MSDC_PB2_RESPWAIT, 3);
Fit in one line
Done
https://review.coreboot.org/c/coreboot/+/51966/comment/883fe18f_c0c57f94 PS2, Line 136: u
U
Done
https://review.coreboot.org/c/coreboot/+/51966/comment/eaef07a4_df9ffc66 PS2, Line 136: need
need to
Done
https://review.coreboot.org/c/coreboot/+/51966/comment/db9e49a1_0bdc5467 PS2, Line 138: MSDC_PATCH_BIT2_CFGRESP);
Fit in one line
Done
https://review.coreboot.org/c/coreboot/+/51966/comment/39e5ebae_3582cdbe PS2, Line 155: /*
Wrong format
Done
https://review.coreboot.org/c/coreboot/+/51966/comment/944e4e0f_05484b60 PS2, Line 156: it's must otherwise sdio cmd5 failed
Otherwise, sdio cmd5 will fail.
Done
https://review.coreboot.org/c/coreboot/+/51966/comment/f42931d9_37e74368 PS2, Line 183:
Indent
Ack
https://review.coreboot.org/c/coreboot/+/51966/comment/57366807_043fdf02 PS2, Line 211: 0x%x
%#x
Done
https://review.coreboot.org/c/coreboot/+/51966/comment/3fd3e164_b091d527 PS2, Line 219: 0x%x
%#x
Done
https://review.coreboot.org/c/coreboot/+/51966/comment/a28c7f2a_75d34171 PS2, Line 234: unsigned int
uint32_t
Done
https://review.coreboot.org/c/coreboot/+/51966/comment/5d307118_96fc95fe PS2, Line 318: (read32(host->base + MSDC_FIFOCS) & MSDC_FIFOCS_TXCNT) >> 16 || : read32(host->base + MSDC_FIFOCS) & MSDC_FIFOCS_RXCNT
Just […]
Done
https://review.coreboot.org/c/coreboot/+/51966/comment/b180f9a9_5b90cbd8 PS2, Line 359: div_width
Add 'const', and no need to assert(div_width > 0)
Done
https://review.coreboot.org/c/coreboot/+/51966/comment/e5e9412b_4ced392f PS2, Line 454: 0x%08x
%#010x
Done
https://review.coreboot.org/c/coreboot/+/51966/comment/b7b032d8_fca95986 PS2, Line 461: 0x%08x
%#010x
Done
File src/soc/mediatek/common/msdc.c:
https://review.coreboot.org/c/coreboot/+/51966/comment/d388e635_9935aa38 PS3, Line 465: printk(BIOS_INFO, "status = %d\n", status);
Info level messages should be understandable for users. Please rephrase or make it a debug message.
Done
File src/soc/mediatek/common/msdc.c:
https://review.coreboot.org/c/coreboot/+/51966/comment/6a86bf7f_3895b2eb PS4, Line 61: space
https://review.coreboot.org/c/coreboot/+/51966/comment/68ff974c_c4056e4e PS4, Line 133: MSDC_PAD_TUNE_CMD_SEL Move to the previous line
https://review.coreboot.org/c/coreboot/+/51966/comment/a72e5166_fee21d5b PS4, Line 459: "%s Align with BIOS_ERR
File src/soc/mediatek/mt8192/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/51966/comment/8b6206ea_a8e51daf PS2, Line 65: ramstage-y += ../common/msdc.c
Sort
Done
-
Yu-Ping Wu (Code Review)