Change in coreboot[master]: payloads/tianocore: Rework Makefile

[Martin Roth (Code Review)]

Attention is currently required from: Sean Rhodes, Andy Pont, Stefan Reinauer.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/61550 )

Change subject: payloads/tianocore: Rework Makefile
......................................................................


Patch Set 8:

(6 comments)

File payloads/external/tianocore/Kconfig:

https://review.coreboot.org/c/coreboot/+/61550/comment/f490f9d2_4c2a5df2 
PS8, Line 9: config TIANOCORE_REPOSITORY
           : 	string "URL to git repository for edk2"
           : 	default "https://github.com/tianocore/edk2" if TIANOCORE_UPSTREAM
           : 	default "https://github.com/mrchromebox/edk2"
I think this should default to "https://review.coreboot.org/edk2", which should in theory be a superset of the mrchromebox repo.


https://review.coreboot.org/c/coreboot/+/61550/comment/9c8ea935_2c60fd8f 
PS8, Line 14: TIANOCORE_TAG
since this doesn't need to be a tag, maybe TIANOCORE_TAG_OR_REV ?


https://review.coreboot.org/c/coreboot/+/61550/comment/d8119693_a6b23b06 
PS8, Line 82: Disabling this option, which will reserve memory above 4G,
I think this could be phrased better - Though it's probable that nobody will misunderstand, It seems to be saying that disabling the option will enable the memory above 4G.

maybe "Disabling memory above 4G is useful..."


https://review.coreboot.org/c/coreboot/+/61550/comment/4976f313_48eb7d36 
PS8, Line 128: without an internal
with an external display?


https://review.coreboot.org/c/coreboot/+/61550/comment/cafd8529_a9edcc83 
PS8, Line 136: by ~220KB.
This will probably change in the future, so maybe say something like "as of Jan 2022, this increases..."


File payloads/external/tianocore/Makefile:

https://review.coreboot.org/c/coreboot/+/61550/comment/f86e5aee_ebe8a030 
PS8, Line 26: -D A
Nit: I think we usually prefer to have -D options without the space. I don't think it's particularly important, and I see that it wasn't that way in the old makefile, but I thought I'd mention it.



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

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ic52e0afa7744f4a902274c41aed59ca23fd9f5fc
Gerrit-Change-Number: 61550
Gerrit-PatchSet: 8
Gerrit-Owner: Sean Rhodes <[e-mail]>
Gerrit-Reviewer: Andy Pont <[e-mail]>
Gerrit-Reviewer: Martin Roth <[e-mail]>
Gerrit-Reviewer: Matt DeVillier <[e-mail]>
Gerrit-Reviewer: Stefan Reinauer <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Attention: Sean Rhodes <[e-mail]>
Gerrit-Attention: Andy Pont <[e-mail]>
Gerrit-Attention: Stefan Reinauer <[e-mail]>
Gerrit-Comment-Date: Thu, 03 Feb 2022 16:37:56 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment