Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45408 )
Change subject: mb/ocp/deltalake: Override uart base address via VPD variable ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45408/3/src/mainboard/ocp/deltalake... File src/mainboard/ocp/deltalake/uart.c:
https://review.coreboot.org/c/coreboot/+/45408/3/src/mainboard/ocp/deltalake... PS3, Line 8: int hex2int(const char *hexstr) src/lib/hexstrtobin.c
https://review.coreboot.org/c/coreboot/+/45408/3/src/mainboard/ocp/deltalake... PS3, Line 35: if (vpd_gets(UART_IOPORT, port, VPD_LEN, VPD_RW_THEN_RO)) This sounds like a fundamentally bad idea. VPD was not meant to be used for things like this. We only recently added support to access VPD in pre-RAM stages at all and that was meant to be used only in exceptional cases. The UART is needed *everywhere*, so you're gonna pull the whole VPD code into every stage from the bootblock forward and you're gonna parse the VPD header again and again in every stage. Worse, you're depending the UART on this pretty complex piece of code (which also has a big slew of dependencies) so if anything goes wrong in there you're not gonna get any output at all.
Kconfig is really the appropriate way to set this. If you need it to be modifiable without rebuilding the whole image, you could also just put it in a CBFS file (e.g. fallback/uart_addr or whatever). Or maybe you can use that CMOS option table thing for this. Any of those solutions would be much better suited than VPD.