Change in coreboot[master]: soc/intel/denverton_ns/bootblock: Fix 32-bit RW PCI_BASE_ADDRESS_0 re...
HAOUAS Elyes has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41609 )
Change subject: soc/intel/denverton_ns/bootblock: Fix 32-bit RW PCI_BASE_ADDRESS_0 register ......................................................................
soc/intel/denverton_ns/bootblock: Fix 32-bit RW PCI_BASE_ADDRESS_0 register
Change-Id: I2b65b2aee672808932e32438410a4ebe8fa37606 Signed-off-by: Elyes HAOUAS ehaouas@noos.fr --- M src/soc/intel/denverton_ns/bootblock/uart.c 1 file changed, 1 insertion(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/09/41609/1
diff --git a/src/soc/intel/denverton_ns/bootblock/uart.c b/src/soc/intel/denverton_ns/bootblock/uart.c index 7514fee..78018ca 100644 --- a/src/soc/intel/denverton_ns/bootblock/uart.c +++ b/src/soc/intel/denverton_ns/bootblock/uart.c @@ -17,15 +17,13 @@ static void pci_early_hsuart_device_probe(u8 bus, u8 dev, u8 func, u32 mmio_base) { - register uint16_t reg16; pci_devfn_t uart_dev = PCI_DEV(bus, dev, func);
/* We're using MMIO for HSUARTs. This section is needed for logging * from FSP only */ /* Decode IOBASE at IOBA (BAR0). */ - reg16 = pci_read_config16(uart_dev, PCI_BASE_ADDRESS_0) | mmio_base; - pci_write_config16(uart_dev, PCI_BASE_ADDRESS_0, reg16); + pci_or_config32(uart_dev, PCI_BASE_ADDRESS_0, mmio_base);
#if (CONFIG(NON_LEGACY_UART_MODE)) /* Decode MMIO at MEMBA (BAR1) */
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41609 )
Change subject: soc/intel/denverton_ns/bootblock: Fix 32-bit RW PCI_BASE_ADDRESS_0 register ......................................................................
Patch Set 1:
(4 comments)
https://review.coreboot.org/c/coreboot/+/41609/1/src/soc/intel/denverton_ns/... File src/soc/intel/denverton_ns/bootblock/uart.c:
https://review.coreboot.org/c/coreboot/+/41609/1/src/soc/intel/denverton_ns/... PS1, Line 27: mmio_base Why is this called `mmio` if we configure I/O ports?
https://review.coreboot.org/c/coreboot/+/41609/1/src/soc/intel/denverton_ns/... PS1, Line 28: pci_write_config16(uart_dev, PCI_BASE_ADDRESS_0, reg16); Why is this a read-modify-write?
https://review.coreboot.org/c/coreboot/+/41609/1/src/soc/intel/denverton_ns/... PS1, Line 37: allow to initiate : * a transaction as a master Why?
https://review.coreboot.org/c/coreboot/+/41609/1/src/soc/intel/denverton_ns/... PS1, Line 169: early_config_gpio(); Why do we unconditionally change pad configurations?
David Guckian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41609 )
Change subject: soc/intel/denverton_ns/bootblock: Fix 32-bit RW PCI_BASE_ADDRESS_0 register ......................................................................
Patch Set 1: Code-Review+1
(3 comments)
https://review.coreboot.org/c/coreboot/+/41609/1/src/soc/intel/denverton_ns/... File src/soc/intel/denverton_ns/bootblock/uart.c:
https://review.coreboot.org/c/coreboot/+/41609/1/src/soc/intel/denverton_ns/... PS1, Line 27: mmio_base
Why is this called `mmio` if we configure I/O ports?
I agree its a poor choice of name used here.
https://review.coreboot.org/c/coreboot/+/41609/1/src/soc/intel/denverton_ns/... PS1, Line 28: pci_write_config16(uart_dev, PCI_BASE_ADDRESS_0, reg16);
Why is this a read-modify-write?
I don't know the reason why this was done, possibly following an example from another SOC.
https://review.coreboot.org/c/coreboot/+/41609/1/src/soc/intel/denverton_ns/... PS1, Line 169: early_config_gpio();
Why do we unconditionally change pad configurations?
I don't know the history for doing this here. I imagine its being done as a precaution in case the GPIOs were set differently in softstraps.
HAOUAS Elyes has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/41609 )
Change subject: soc/intel/denverton_ns/bootblock: Fix 32-bit RW PCI_BASE_ADDRESS_0 register ......................................................................
Abandoned
-
David Guckian (Code Review) -
HAOUAS Elyes (Code Review)
-
Nico Huber (Code Review)