Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35296 )
Change subject: soc/amd/picasso: Refactor AOAC enabling ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35296/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35296/1//COMMIT_MSG@13 PS1, Line 13: Picasso's UARTs Keep in mind this is primarily affecting the copy of stoneyridge code. The patch that updates picasso for its UARTs comes later in https://review.coreboot.org/c/coreboot/+/33767/14 I chose to remove the calculation here so that the update looks cleaner. UART 0/1/2/3 are devices 11/12/16/26.
Why not simply say that UART enabling is now controlled by a config parameter?
If anything, an APU UART was being turned on incorrectly when it wasn't supposed to be. Do you want me to add a note that it corrects this condition?
https://review.coreboot.org/c/coreboot/+/35296/1/src/soc/amd/picasso/southbr... File src/soc/amd/picasso/southbridge.c:
https://review.coreboot.org/c/coreboot/+/35296/1/src/soc/amd/picasso/southbr... PS1, Line 52: */
Maybe add why? I had to look the code below to understand it had a control through config PICASSO_UA […]
The comment is really meant to a caution to anyone who may try to add their UART to the aoac_devs list without first seeing what the code does. Reading the source below, it's pretty evident IMO that the console UART is only turned on when CONFIG(PICASSO_UART) is enabled.