Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40322 )
Change subject: [WIP] [UNTESTED] soc/amd/picasso: Clean up legacy UART config ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/c/coreboot/+/40322/4/src/soc/amd/picasso/include... File src/soc/amd/picasso/include/soc/southbridge.h:
https://review.coreboot.org/c/coreboot/+/40322/4/src/soc/amd/picasso/include... PS4, Line 225: #define FCH_LEGACY_UART_MAP_SHIFT 8
nit: If these are bitfield definitions, I'd indent them with 2 spaces as done in the rest of the fil […]
Done
https://review.coreboot.org/c/coreboot/+/40322/4/src/soc/amd/picasso/uart.c File src/soc/amd/picasso/uart.c:
https://review.coreboot.org/c/coreboot/+/40322/4/src/soc/amd/picasso/uart.c@... PS4, Line 63: leg
it's a warlking UART! 😄
yeah, that does sound a bit odd; changed it to uart_legacy_decode
https://review.coreboot.org/c/coreboot/+/40322/4/src/soc/amd/picasso/uart.c@... PS4, Line 82: const uint8_t range_idx[ARRAY_SIZE(uart_info)] = { : FCH_LEGACY_UART_RANGE_3F8, : FCH_LEGACY_UART_RANGE_2F8, : FCH_LEGACY_UART_RANGE_3E8, : FCH_LEGACY_UART_RANGE_2E8, : };
How about adding this information to uart_info instead?
I didn't add that to uart_info for two reasons: uart_info contains information that's hard-wired in the chip while the mapping of the SoC UARTs to the legacy UART I/O regions is configurable and this table maps the I/O region to the UART that a legacy UART with the same number would have. When PICASSO_UART_LEGACY isn't set, which is the more common case, this function will get optimized out and I'm not sure if tat part of the mapping table would also be optimized out if it was added to uart_info