[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 3:

(5 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).
> We are using a bayhub eMMC which will not accept any command until a software reset (bit 0 of regist […]
You can't pass the exact situation through cbmem unless the reset has actually finished. If it hasn't, it might still finish in the meantime until the payload driver takes over. That's why I think that checking it at the end of ramstage is futile and unnecessary. You need the code in the payload (to wait for a reset to finish) anyway.


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 */
> I just used byte above with bit 0 set. […]
You mean the assignment in the next line might fail? wouldn't that violate C?


https://review.coreboot.org/#/c/30225/1/src/drivers/emmc/emmc_rom.c@43 
PS1, Line 43: 		pinfo->status = EMMC_CONFIRMED_RESET_ACCEPTED;
Can you point me to documentation / a datasheet where this behaviour
is specified? What particularly concerns me is for how long the
EMMC_RESET_ALL bit is considered to be set at minimum? Or will it
always be set on the first read after setting it (no matter how much
time passed until the read)?


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
> If cbmem for reset status is not found or is status is EMMC_SEND_RESET, something went wrong and pay […]
AFAICS, all you need to do in coreboot is to set EMMC_RESET_ALL and notify
the payload that you did. Everything else can be handled in the payload:

    if (!coreboot_send_reset)
            set EMMC_RESET_ALL;
    while (still EMMC_RESET_ALL)
            wait;

All your other states seem to be artificial software additions without any
meaning for the hardware.


https://review.coreboot.org/#/c/30225/1/src/include/device/emmc.h@51 
PS1, Line 51:  */
> What I'm saying is that other code can use the same cbmem id. […]
Why should other code use the same cbmem id? It seems to me you are extending
the concept of cbmem id without a reason.



-- 
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: 3
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: Tue, 18 Dec 2018 16:22:22 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Richard Spiegel <richard.spiegel at silverbackltd.com>
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/20181218/45a664df/attachment.html>