[coreboot-gerrit] Change in ...coreboot[master]: mainboard/google/mistral: Add support for Mistral

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29949 )

Change subject: mainboard/google/mistral: Add support for Mistral
......................................................................


Patch Set 2:

(3 comments)

https://review.coreboot.org/#/c/29949/2/src/mainboard/google/mistral/chromeos.fmd 
File src/mainboard/google/mistral/chromeos.fmd:

https://review.coreboot.org/#/c/29949/2/src/mainboard/google/mistral/chromeos.fmd@38 
PS2, Line 38: 		RW_XBL_BUFFER_A at 0x1E8000 0x4000
Please justify what you need all the non-standard sections in here for. This looks like it was copied from a very early version of Cheza code, where we later found out that a lot of it is unnecessary (e.g. this XBL thing).

Note that this is the latest FMAP for Cheza (which will plan to use, except that some section may still shrink or grow a bit): https://review.coreboot.org/c/coreboot/+/25208/62/src/mainboard/google/cheza/chromeos.fmd . I expect that Mistral should look exactly the same, except maybe without an FSG region (that one depends on whether you have an LTE modem or not).


https://review.coreboot.org/#/c/29949/2/src/mainboard/google/mistral/memlayout.ld 
File src/mainboard/google/mistral/memlayout.ld:

https://review.coreboot.org/#/c/29949/2/src/mainboard/google/mistral/memlayout.ld@16 
PS2, Line 16:  
There shouldn't be a space here (guess that's wrong in Cheza, too).


https://review.coreboot.org/#/c/29949/2/src/mainboard/google/mistral/romstage.c 
File src/mainboard/google/mistral/romstage.c:

https://review.coreboot.org/#/c/29949/2/src/mainboard/google/mistral/romstage.c@22 
PS2, Line 22: #include <arch/stages.h>
In general, do not define functions until you need them (e.g. you don't need to define platform_romstage_main() yet... in fact, you don't need to add this whole file yet), and do not add headers unless some code that you're adding with this patch actually uses them.



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

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I7ecfad68bb50f42acf36f51bc3433add56597c3d
Gerrit-Change-Number: 29949
Gerrit-PatchSet: 2
Gerrit-Owner: nsekar at codeaurora.org
Gerrit-Reviewer: Julius Werner <jwerner at chromium.org>
Gerrit-Reviewer: Martin Roth <martinroth at google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi at google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-Reviewer: nsekar at codeaurora.org
Gerrit-Comment-Date: Thu, 06 Dec 2018 22:03:41 +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/20181206/8df3cad0/attachment.html>