[coreboot-gerrit] Change in ...coreboot[master]: drivers/generic/bayhub: Add reset capability

[Richard Spiegel (Code Review)]

Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30319 )

Change subject: drivers/generic/bayhub: Add reset capability
......................................................................


Patch Set 2:

(4 comments)

https://review.coreboot.org/#/c/30319/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/30319/2//COMMIT_MSG@10 
PS2, Line 10: set bit 15 of subsystem ID
> interesting choice
Martin's suggestion.


https://review.coreboot.org/#/c/30319/2/src/drivers/generic/bayhub/bh720.c 
File src/drivers/generic/bayhub/bh720.c:

https://review.coreboot.org/#/c/30319/2/src/drivers/generic/bayhub/bh720.c@61 
PS2, Line 61: 		pci_or_config16(dev, PCI_SUBSYSTEM_ID, BH720_EARLY_RESET);
> Move into the `if` below so you don't have to clear it in the `else`?
I'm not clearing in the else... the 2 possible messages are:
BayHub BH720: Early reset success
BayHub BH720: Early reset failed
Saving space on the debug version, where speed does not matter. On release version, the print becomes a no op, so it does not matter either.


https://review.coreboot.org/#/c/30319/2/src/drivers/generic/bayhub/bh720.c@63 
PS2, Line 63: 			     BH720_SOFTWARE_RESET);
> I guess this shortcut is fine for the given device (you now exactly that it's a 32-bit memory resour […]
That's how it's done with depthcharge. I copied the variables (with a small change in name) and the code from there.


https://review.coreboot.org/#/c/30319/2/src/drivers/generic/bayhub/bh720.c@91 
PS2, Line 91: 	.init			= bh720_init,
> This is rather late in ramstage. Might still save enough time, though.
I know. I would have preferred romstage. However, the reset is also not done early in depthcharge (about 1/3 into it), so maybe there will be enough time.



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

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ic3878ee782c8da1a28c6d669dd7eceda7c8cf4e5
Gerrit-Change-Number: 30319
Gerrit-PatchSet: 2
Gerrit-Owner: Richard Spiegel <richard.spiegel at silverbackltd.com>
Gerrit-Reviewer: Daniel Kurtz <djkurtz at google.com>
Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd at gmail.com>
Gerrit-Reviewer: Martin Roth <martinroth at google.com>
Gerrit-Reviewer: Nico Huber <nico.h at gmx.de>
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-Comment-Date: Thu, 20 Dec 2018 16:07:03 +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/20181220/5c89d969/attachment.html>