Change in coreboot[master]: soc/mediatek: add new driver 'msdc' for eMMC

[Yu-Ping Wu (Code Review)]

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