Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33759 )
Change subject: soc/amd/picasso: Create a hybrid romstage to begin in DRAM ......................................................................
Patch Set 13:
(5 comments)
Is the name hibrid_romstage going to be final? I remember some discussions about using some different name. If so, shouldn't you change file name and the commit message?>
I'm flexible on the name but I've heard no better suggestions. Although it uses nearly all the romstage build infrastructure, I think it's important to try to draw a distinction between this implementation and a traditional romstage. If you're thinking of what I'd coined "pspstage", if it comes about, that's a completely separate entity and 0% romstage, with the makefile calling a new create_class_compiler, pspstage += <name> for all source, new environment definitions, etc. A pspstage approach is my current long-term preference.
https://review.coreboot.org/c/coreboot/+/33759/13/src/soc/amd/picasso/early_... File src/soc/amd/picasso/early_stack.S:
https://review.coreboot.org/c/coreboot/+/33759/13/src/soc/amd/picasso/early_... PS13, Line 31:
Extra space.
It's not extra. Using a double space after a period, and between sentences, was always the norm for typewritten text.
https://review.coreboot.org/c/coreboot/+/33759/13/src/soc/amd/picasso/early_... PS13, Line 32: * may be enabled. This could be made more elegant for more
Current limit is 96 columns, please use full new size. Same in next comment.
Although the limit is 96, it's still not preferred. 96 should only be used when 80 affects readability.
https://review.coreboot.org/c/coreboot/+/33759/13/src/soc/amd/picasso/hybrid... File src/soc/amd/picasso/hybrid_romstage.c:
https://review.coreboot.org/c/coreboot/+/33759/13/src/soc/amd/picasso/hybrid... PS13, Line 4: * Copyright (C) 2019 Advanced Micro Devices, Inc.
In romstage.h you placed this line after previous copyright, here you placed it before. […]
This line wasn't added. The two copyrights used to be AMD 2015-2016 and Intel 2015. I can't look at this file and determine what Intel might wish to claim ownership of, but OTOH I'm not wishing to remove their name from the list. However, I was comfortable making the claim that 100% of the AMD work is relevant to 2019. So the change was to replace "2015-2016" with "2019".
I prefer keeping AMD's name first. If you really want a change, I'll put the copyright back to 2015 again.
https://review.coreboot.org/c/coreboot/+/33759/9/src/soc/amd/picasso/reset_v... File src/soc/amd/picasso/reset_vector.S:
https://review.coreboot.org/c/coreboot/+/33759/9/src/soc/amd/picasso/reset_v... PS9, Line 111: .byte 0x00, 0x9b, 0xaf, 0x00
So far I haven't needed it bigger than 64K but I specifically gave myself the option. […]
Done
https://review.coreboot.org/c/coreboot/+/33759/9/src/soc/amd/picasso/romstag... File src/soc/amd/picasso/romstage.c:
https://review.coreboot.org/c/coreboot/+/33759/9/src/soc/amd/picasso/romstag... PS9, Line 49: || (CONFIG_DCACHE_RAM_BASE >= CONFIG_ROMSTAGE_ADDR \
Comparisons should place the constant on the right side of the test
OBE
-
Marshall Dawson (Code Review)