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

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
-- To view, visit https://review.coreboot.org/c/coreboot/+/51966 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I54a7749ed167c00cd631a76af7c67c654c7bc725 Gerrit-Change-Number: 51966 Gerrit-PatchSet: 4 Gerrit-Owner: Wenbin Mei <wenbin.mei@mediatek.corp-partner.google.com> Gerrit-Reviewer: Hung-Te Lin <hungte@chromium.org> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Xi Chen <xixi.chen@mediatek.com> Gerrit-Reviewer: Yidi Lin <yidi.lin@mediatek.com> Gerrit-Reviewer: Yu-Ping Wu <yupingso@google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Attention: Hung-Te Lin <hungte@chromium.org> Gerrit-Attention: Wenbin Mei <wenbin.mei@mediatek.corp-partner.google.com> Gerrit-Comment-Date: Thu, 01 Apr 2021 10:09:58 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Hung-Te Lin <hungte@chromium.org> Comment-In-Reply-To: Paul Menzel <paulepanter@users.sourceforge.net> Comment-In-Reply-To: Wenbin Mei <wenbin.mei@mediatek.corp-partner.google.com> Comment-In-Reply-To: Yu-Ping Wu <yupingso@google.com> Gerrit-MessageType: comment
participants (1)
-
Yu-Ping Wu (Code Review)