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

Mon Dec 17 15:56:42 CET 2018

Richard Spiegel 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)

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`. […]
We are using a bayhub eMMC which will not accept any command until a software reset (bit 0 of register offset 2fh) is set and cleared. It clears by itself once the reset is completed, and that's what takes a long time. We are reducing the total boot time by making it happen at coreboot time, so that when payload takes over, it's either completed or about to complete (exact situation passed through cbmem) and the payload (in this board depthcharge) don't need to send it again. It either continues after reset is completed (ideal situation) or just waits its completion (still saving time). The exact value depends on how long it takes between the fist command and payload execution.

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`?
See the comment where it's defined. It's a way for other eMMC that needs a different sequence to tell who they are. In theory, payload has to read the identifier before interpreting any byte(s) following it.

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.
will do.

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

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  […]
I just used byte above with bit 0 set. If reads fail for any reason and there's no actual value loaded into byte, bit 0 will be already set, thus mascaraing a failure situation. By clearing bit 0 first than checking if it's set, I guarantee a read was executed.

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 carr […]
If cbmem for reset status is not found or is status is EMMC_SEND_RESET, something went wrong and payload has to execute full reset.
if status = EMMC_CONFIRMED_RESET_ACCEPTED, then reset has not been checked for complition, and payload should act just as in the case of EMMC_RESET_STILL_ACTIVE.
If status = EMMC_RESET_STILL_ACTIVE, payload has to wait bit 0 of register 2fh to become clear before continuing code.
If status = EMMC_RESET_COMPLETED, payload continues from its code after the call to reset the eMMC.

https://review.coreboot.org/#/c/30225/1/src/include/device/emmc.h@51 
PS1, Line 51:  */
> I still don't get it. […]
What I'm saying is that other code can use the same cbmem id. As you mentioned, there's an Intel eMMC where apparently something else needs to be done (though apparently depthcharge knows nothing about it, as I copy depthcharge reset code for eMMC). So I'm creating an opportunity for payload to know it has something else happening. There's a single cbmem id (so payload can easely immediately identify if there was an early attempt to reset the eMMC, and then by checking this variable decide how it has to act.

-- 
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: Patrick Rudolph <siro at das-labor.org>
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: Mon, 17 Dec 2018 14:56:42 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h at gmx.de>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot-gerrit/attachments/20181217/1ba84d30/attachment.html>