Attention is currently required from: Furquan Shaikh. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49213 )
Change subject: soc/intel/common/uart: Use simple(_s_) variants of PCI functions ......................................................................
Patch Set 2: Code-Review+1
(3 comments)
File src/soc/intel/common/block/uart/uart.c:
https://review.coreboot.org/c/coreboot/+/49213/comment/0ad8f091_3af2c3db PS2, Line 61: devfn Hmmm... I feel this `devfn` can confuse people (at least it confuses me). I don't like the `pci_devfn_t` type name at all. We use it for PCI_DEV(b, d, f) values, whereas devfn's are plain ints. Ugh, no good solution.
https://review.coreboot.org/c/coreboot/+/49213/comment/b6b609fd_52ff0310 PS2, Line 84: pci_devfn_t devfn These should be renamed to `dev` to avoid confusion with actual devfn-only values elsewhere.
https://review.coreboot.org/c/coreboot/+/49213/comment/fe30aa2d_9de73bcf PS2, Line 215: if (uart_controller_needs_init(dev)) { : uintptr_t base; : pci_devfn_t devfn = PCI_BDF(dev); : : base = pci_s_read_config32(devfn, PCI_BASE_ADDRESS_0) & ~0xFFF; : if (base) : uart_lpss_init(devfn, base); : } This function is ramstage-only. Wouldn't it be enough to simply add PCI_BDF in the `uart_lpss_init` function call?
if (uart_controller_needs_init(dev)) { uintptr_t base;
base = pci_read_config32(dev, PCI_BASE_ADDRESS_0) & ~0xFFF;
if (base) uart_lpss_init(PCI_BDF(dev), base); }
(Also, ramstage functions should be moved to a ramstage-only file, but that's for another patch)