[coreboot-gerrit] Change in coreboot[master]: amd/pi/mainboards: Consolidate early duplicated code

[Marc Jones (Code Review)]

Marc Jones has posted comments on this change. ( https://review.coreboot.org/18434 )

Change subject: amd/pi/mainboards: Consolidate early duplicated code
......................................................................


Patch Set 3:

(10 comments)

https://review.coreboot.org/#/c/18434/3//COMMIT_MSG
Commit Message:

Line 16: APUs also get BIST bits masked off.
> BIST masking is separate change.
agree, need to check


https://review.coreboot.org/#/c/18434/3/src/cpu/amd/pi/car.c
File src/cpu/amd/pi/car.c:

Line 26: void cache_as_ram_main(unsigned long bist, unsigned long cpu_init_detectedx)
> This cpu_init_detectedx here is very obscure. Seemed like it's just LAPIC I
agree


Line 30: #if IS_ENABLED(CONFIG_COLLECT_TIMESTAMPS)
> Our definition timestamp_init() probably already has dummy static inline to
need to check


Line 31: 	/* Initialize timestamp book keeping only once. */
> How is this only once? BSP CPU passes this twice?
need to check


Line 37: 	/* Must come first to enable PCI MMCONF. */
> That's what I did for AGESA.
OK, i see you patch now (please provide a link when you have an example.)
I don't really agree with calling the C code from asm and that it is any more safe being in asm code vs c code. Now you have a comment in the asm code that it must be there because first. You just added another call layer that isn't needed IMO.


Line 88: }
> I strongly recommend passing some common struct pointer in all of the calls
I'll disagree again. I don't agree with adding parameters/variables for something that doesn't exist. Happy to add them when there is something to pass. You wouldn't just add a variable because you would probably need it. Besides, the compiler would yell at you for an unused variable.


https://review.coreboot.org/#/c/18434/3/src/mainboard/amd/bettong/romstage.c
File src/mainboard/amd/bettong/romstage.c:

Line 27: void cache_as_ram_stage_main(void)
> I suggest you to reuse the definitions I have added to cpu/amd/car.h with c
You put this comment in a bunch of places, so I responded to each. We can work towards combined, but I don't agree with adding parameters/variables for something that doesn't exist. Happy to add them when there is something to pass. You wouldn't just add a variable because you would probably need it. Besides, the compiler would yell at you for an unused variable.


https://review.coreboot.org/#/c/18434/3/src/mainboard/amd/lamar/romstage.c
File src/mainboard/amd/lamar/romstage.c:

Line 38: void cache_as_ram_stage_main(void)
> Having no input parameter forces use of CAR GLOBAL if you want to pass this
I don't agree with adding parameters/variables for something that doesn't exist. Happy to add them when there is something to pass. You wouldn't just add a variable because you would probably need it. Besides, the compliler would yell at you for an unused variable.


https://review.coreboot.org/#/c/18434/3/src/mainboard/amd/olivehillplus/early_mainboard.c
File src/mainboard/amd/olivehillplus/early_mainboard.c:

Line 39: 	 */
> If it has to do with LPC, probably has to be done before console_init() if 
agree


https://review.coreboot.org/#/c/18434/3/src/mainboard/pcengines/apu2/early_mainboard.c
File src/mainboard/pcengines/apu2/early_mainboard.c:

Line 4:  * Copyright (C) 2012 - 2017 Advanced Micro Devices, Inc.
> Not sure if it's okay to change copyright date when you juts move code from
new lines for car_mainboard_pre_console_init() I believe.


-- 
To view, visit https://review.coreboot.org/18434
To unsubscribe, visit https://review.coreboot.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id10580ecc439df10df26ca99881772f328e7bdd0
Gerrit-PatchSet: 3
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Marc Jones <marc at marcjonesconsulting.com>
Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki at gmail.com>
Gerrit-Reviewer: Marc Jones <marc at marcjonesconsulting.com>
Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd at gmail.com>
Gerrit-Reviewer: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: Philippe Mathieu-Daudé <philippe.mathieu.daude at gmail.com>
Gerrit-Reviewer: build bot (Jenkins)
Gerrit-HasComments: Yes