[coreboot-gerrit] Change in ...coreboot[master]: drivers: Add eMMC reset driver

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30225 )

Change subject: drivers: Add eMMC reset driver
......................................................................


Patch Set 1:

(7 comments)

How (if) does this relate to `Work in progress: Early eMMC phase 1`?

https://review.coreboot.org/#/c/30225/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/30225/1//COMMIT_MSG@14 
PS1, Line 14: second time). This might save up to 100 milliseconds (exact value TBD).
I would focus on the `exact value`. There was this weird eMMC on Intel hardware (that's why there is commonlib/storage) that needed much more care (continuous polling, iirc).


https://review.coreboot.org/#/c/30225/1/src/drivers/emmc/emmc_ram.c 
File src/drivers/emmc/emmc_ram.c:

https://review.coreboot.org/#/c/30225/1/src/drivers/emmc/emmc_ram.c@28 
PS1, Line 28: 		return EMMC_STATUS_CBMEM_FAILED;
what about `pinfo->identifier`?


https://review.coreboot.org/#/c/30225/1/src/drivers/emmc/emmc_ram.c@30 
PS1, Line 30: 		return EMMC_STATUS_INVALID_ADDR;
Please move parameter checks to the top, it can be decided immediately.


https://review.coreboot.org/#/c/30225/1/src/drivers/emmc/emmc_ram.c@34 
PS1, Line 34: 	byte = *reset;
use read8() / write8()


https://review.coreboot.org/#/c/30225/1/src/drivers/emmc/emmc_rom.c 
File src/drivers/emmc/emmc_rom.c:

https://review.coreboot.org/#/c/30225/1/src/drivers/emmc/emmc_rom.c@40 
PS1, Line 40: 	byte = 0; /* preclear */
sorry, you might have already noticed, I'm allergic to comments (especially those that cover broken code up).

what is this line supposed to do in the long run?


https://review.coreboot.org/#/c/30225/1/src/include/device/emmc.h 
File src/include/device/emmc.h:

https://review.coreboot.org/#/c/30225/1/src/include/device/emmc.h@23 
PS1, Line 23: #define EMMC_RESET_COMPLETED		4
Given that the consumer has to expect outdated information, what information does this actually carry?


https://review.coreboot.org/#/c/30225/1/src/include/device/emmc.h@51 
PS1, Line 51:  */
I still don't get it. How is this different from the CBMEM id? It's hard to see what you are up to as you didn't use it in your own code.



-- 
To view, visit https://review.coreboot.org/c/coreboot/+/30225
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I4bb55fde23e3b70da1cca76e5672e880317b8bd2
Gerrit-Change-Number: 30225
Gerrit-PatchSet: 1
Gerrit-Owner: Richard Spiegel <richard.spiegel at silverbackltd.com>
Gerrit-Reviewer: Daniel Kurtz <djkurtz at google.com>
Gerrit-Reviewer: Martin Roth <martinroth at google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi at google.com>
Gerrit-Reviewer: Raul Rangel <rrangel at chromium.org>
Gerrit-Reviewer: Richard Spiegel <richard.spiegel at silverbackltd.com>
Gerrit-Reviewer: Simon Glass <sjg at chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-CC: Nico Huber <nico.h at gmx.de>
Gerrit-Comment-Date: Sat, 15 Dec 2018 10:59:43 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot-gerrit/attachments/20181215/2e3fc814/attachment.html>