Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42685 )
Change subject: soc/amd/common: Drop ACPIMMIO bank for SMBus device PCI config ......................................................................
Patch Set 6:
(3 comments)
https://review.coreboot.org/c/coreboot/+/42685/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42685/6//COMMIT_MSG@7 PS6, Line 7: Drop ACPIMMIO ban
Why do you want to drop it?
Why have it if PCI config write does the same thing, with less confusion? If PSP did map this particular ACPIMMIO bank this could have had some use.
https://review.coreboot.org/c/coreboot/+/42685/6//COMMIT_MSG@10 PS6, Line 10: PCI config write
Sure: D14F0x0FC
No PCI config access from psp-verstage?
https://review.coreboot.org/c/coreboot/+/42685/6/src/soc/amd/picasso/uart.c File src/soc/amd/picasso/uart.c:
https://review.coreboot.org/c/coreboot/+/42685/6/src/soc/amd/picasso/uart.c@... PS6, Line 60: Help
0=use 48MHz as UART baud rate clock. 1=enable UART legacy mode to use 1.843MHz as baud rate clock. […]
If psp verstage is the first code we can execute, we will want to be able to configure the UART and baudrate. As long as there are no other relevant UART configuration bits inside D14F0, sm_pci_read/write() can be dropped..
But if there _are_ relevant UART enable bits there... PSP kernel should really provide either PCI configuration access or a service call to map this SM_PCI bank.