Change in coreboot[master]: soc/intel/common/uart: Use simple(_s_) variants of PCI functions
Furquan Shaikh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/49213 )
Change subject: soc/intel/common/uart: Use simple(_s_) variants of PCI functions ......................................................................
soc/intel/common/uart: Use simple(_s_) variants of PCI functions
This change updates various uart_* functions to use simple(_s_) variants of PCI functions. This is done for a few reasons:
* __SIMPLE_DEVICE__ check can be dropped since the same data type can be used in early stages and ramstage. * Removes the requirement on early stage to walk the device tree to get access to the device structure. This allows linker-based device tree optimizations for early stages.
As part of this change, uart_get_device() is refactored and a new function uart_console_get_devfn() is added which returns pci_devfn_t in MMCONF format. It is then used directly by the _s_ variants of PCI functions.
Change-Id: I344037828118572ae5eb27c82c496d5e7a508a53 Signed-off-by: Furquan Shaikh furquan@google.com --- M src/soc/intel/common/block/uart/uart.c 1 file changed, 37 insertions(+), 43 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/49213/1
diff --git a/src/soc/intel/common/block/uart/uart.c b/src/soc/intel/common/block/uart/uart.c index 1cfdb67..8a8582e 100644 --- a/src/soc/intel/common/block/uart/uart.c +++ b/src/soc/intel/common/block/uart/uart.c @@ -23,10 +23,10 @@ extern const struct uart_controller_config uart_ctrlr_config[]; extern const int uart_max_index;
-static void uart_lpss_init(const struct device *dev, uintptr_t baseaddr) +static void uart_lpss_init(pci_devfn_t devfn, uintptr_t baseaddr) { /* Ensure controller is in D0 state */ - lpss_set_power_state(PCI_BDF(dev), STATE_D0); + lpss_set_power_state(devfn, STATE_D0);
/* Take UART out of reset */ lpss_reset_release(baseaddr); @@ -58,60 +58,49 @@ return UART_CONSOLE_INVALID_INDEX; }
-static void uart_common_init(const struct device *device, uintptr_t baseaddr) +static pci_devfn_t uart_console_get_devfn(void) { -#if defined(__SIMPLE_DEVICE__) - pci_devfn_t dev = PCI_BDF(device); -#else - const struct device *dev = device; -#endif + int devfn; + int index;
- /* Set UART base address */ - pci_write_config32(dev, PCI_BASE_ADDRESS_0, baseaddr); - - /* Enable memory access and bus master */ - pci_write_config16(dev, PCI_COMMAND, UART_PCI_ENABLE); - - uart_lpss_init(device, baseaddr); -} - -const struct device *uart_get_device(void) -{ /* * This function will get called even if INTEL_LPSS_UART_FOR_CONSOLE * config option is not selected. * By default return NULL in this case to avoid compilation errors. */ if (!CONFIG(INTEL_LPSS_UART_FOR_CONSOLE)) + return 0; + + index = uart_get_valid_index(); + if (index == UART_CONSOLE_INVALID_INDEX) + return 0; + + devfn = uart_ctrlr_config[index].devfn; + return PCI_DEV(0, PCI_SLOT(devfn), PCI_FUNC(devfn)); +} + +const struct device *uart_get_device(void) +{ + pci_devfn_t devfn = uart_console_get_devfn(); + if (devfn == 0) return NULL;
- int console_index = uart_get_valid_index(); - - if (console_index != UART_CONSOLE_INVALID_INDEX) - return pcidev_path_on_root(uart_ctrlr_config[index].devfn); - else - return NULL; + return pcidev_path_on_root(PCI_DEV2DEVFN(devfn)); }
bool uart_is_controller_initialized(void) { uintptr_t base; - const struct device *dev_uart = uart_get_device(); + pci_devfn_t devfn = uart_console_get_devfn();
- if (!dev_uart) + if (devfn == 0) return false;
-#if defined(__SIMPLE_DEVICE__) - pci_devfn_t dev = PCI_BDF(dev_uart); -#else - const struct device *dev = dev_uart; -#endif - - base = pci_read_config32(dev, PCI_BASE_ADDRESS_0) & ~0xFFF; + base = pci_s_read_config32(devfn, PCI_BASE_ADDRESS_0) & ~0xFFF; if (!base) return false;
- if ((pci_read_config16(dev, PCI_COMMAND) & UART_PCI_ENABLE) + if ((pci_s_read_config16(devfn, PCI_COMMAND) & UART_PCI_ENABLE) != UART_PCI_ENABLE) return false;
@@ -129,15 +118,19 @@
void uart_bootblock_init(void) { - const struct device *dev_uart; + const uint32_t baseaddr = CONFIG_CONSOLE_UART_BASE_ADDRESS; + pci_devfn_t devfn = uart_console_get_devfn();
- dev_uart = uart_get_device(); - - if (!dev_uart) + if (devfn == 0) return;
- /* Program UART BAR0, command, reset and clock register */ - uart_common_init(dev_uart, CONFIG_CONSOLE_UART_BASE_ADDRESS); + /* Set UART base address */ + pci_s_write_config32(devfn, PCI_BASE_ADDRESS_0, baseaddr); + + /* Enable memory access and bus master */ + pci_s_write_config16(devfn, PCI_COMMAND, UART_PCI_ENABLE); + + uart_lpss_init(devfn, baseaddr);
/* Configure the 2 pads per UART. */ uart_configure_gpio_pads(); @@ -221,10 +214,11 @@
if (uart_controller_needs_init(dev)) { uintptr_t base; + pci_devfn_t devfn = PCI_BDF(dev);
- base = pci_read_config32(dev, PCI_BASE_ADDRESS_0) & ~0xFFF; + base = pci_s_read_config32(devfn, PCI_BASE_ADDRESS_0) & ~0xFFF; if (base) - uart_lpss_init(dev, base); + uart_lpss_init(devfn, base); } }
Kyösti Mälkki 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 1:
I'd rather take approach of dropping __SIMPLE_DEVICE__ entirely and always pass struct device *.
Furquan Shaikh 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 1:
Patch Set 1:
I'd rather take approach of dropping __SIMPLE_DEVICE__ entirely and always pass struct device *.
I was trying to avoid the usage of struct device * where possible so that it doesn't pull in the references to all other devices unnecessarily in early stages.
Hello build bot (Jenkins), Nico Huber, Tim Wawrzynczak, Angel Pons, Kyösti Mälkki, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/49213
to look at the new patch set (#2).
Change subject: soc/intel/common/uart: Use simple(_s_) variants of PCI functions ......................................................................
soc/intel/common/uart: Use simple(_s_) variants of PCI functions
This change updates various uart_* functions to use simple(_s_) variants of PCI functions. This is done for a few reasons:
* __SIMPLE_DEVICE__ check can be dropped since the same data type can be used in early stages and ramstage. * Removes the requirement on early stage to walk the device tree to get access to the device structure. This allows linker-based device tree optimizations for early stages.
As part of this change, uart_get_device() is refactored and a new function uart_console_get_devfn() is added which returns pci_devfn_t in MMCONF format. It is then used directly by the _s_ variants of PCI functions.
Change-Id: I344037828118572ae5eb27c82c496d5e7a508a53 Signed-off-by: Furquan Shaikh furquan@google.com --- M src/soc/intel/common/block/uart/uart.c 1 file changed, 37 insertions(+), 43 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/49213/2
Kyösti Mälkki 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 1:
Patch Set 1:
Patch Set 1:
I'd rather take approach of dropping __SIMPLE_DEVICE__ entirely and always pass struct device *.
I was trying to avoid the usage of struct device * where possible so that it doesn't pull in the references to all other devices unnecessarily in early stages.
If you skip walking the devicetree and use alias instead, you can also remove all the tree topology aka links for the early stages. I have such commit somewhere locally.
Furquan Shaikh 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:
Patch Set 1:
Patch Set 1:
Patch Set 1:
I'd rather take approach of dropping __SIMPLE_DEVICE__ entirely and always pass struct device *.
I was trying to avoid the usage of struct device * where possible so that it doesn't pull in the references to all other devices unnecessarily in early stages.
If you skip walking the devicetree and use alias instead, you can also remove all the tree topology aka links for the early stages. I have such commit somewhere locally.
That is where I am planning to head towards eventually. But, it requires adding aliases for the SoCs and updating all relevant common/ and soc/ specific code. Until I can get there I am taking smaller steps to clean up in little chunks to get some space freed up for mainboards that are hitting the limit for bootblock.
Kyösti Mälkki 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:
That is where I am planning to head towards eventually. But, it requires adding aliases for the SoCs and updating all relevant common/ and soc/ specific code. Until I can get there I am taking smaller steps to clean up in little chunks to get some space freed up for mainboards that are hitting the limit for bootblock.
What limit for bootblock? C_ENV_BOOTBLOCK_SIZE is artificial and could be dropped, CB:47600. For certain platforms reducing .text alone also does not help because increasing .bss can also move _start16bit too far away from .reset.
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)
Attention is currently required from: Angel Pons. Furquan Shaikh 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:
(3 comments)
Patchset:
PS2:
Patch Set 2:
That is where I am planning to head towards eventually. But, it requires adding aliases for the SoCs and updating all relevant common/ and soc/ specific code. Until I can get there I am taking smaller steps to clean up in little chunks to get some space freed up for mainboards that are hitting the limit for bootblock.
What limit for bootblock? C_ENV_BOOTBLOCK_SIZE is artificial and could be dropped, CB:47600. For certain platforms reducing .text alone also does not help because increasing .bss can also move _start16bit too far away from .reset.
C_ENV_BOOTBLOCK_SIZE is real for APL/GLK platforms. Those platforms actually cannot have a bootblock size greater than C_ENV_BOOTBLOCK_SIZE. This is because of the CSE/boot flow requirements for the platforms.
File src/soc/intel/common/block/uart/uart.c:
https://review.coreboot.org/c/coreboot/+/49213/comment/e4533d0a_28a779e9 PS2, Line 61: devfn
Hmmm... I feel this `devfn` can confuse people (at least it confuses me). […]
Yeah, it has been on my list for a long time now to clean up the whole devfn/PCI_DEV weirdness. Hopefully, we can get to it soon.
I kept this as devfn because there is already a uart_get_device() which returns `struct device *`. Let me see if I can come up with another name.
https://review.coreboot.org/c/coreboot/+/49213/comment/793e8536_3faa8d69 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. […]
Ack.
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:
(1 comment)
File src/soc/intel/common/block/uart/uart.c:
https://review.coreboot.org/c/coreboot/+/49213/comment/fb38f9de_2d831132 PS2, Line 61: devfn
Yeah, it has been on my list for a long time now to clean up the whole devfn/PCI_DEV weirdness. […]
`uart_console_get_pci_bdf` ?
Attention is currently required from: Furquan Shaikh. Kyösti Mälkki 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:
(1 comment)
Patchset:
PS2:
Patch Set 2: […]
Yeah. And I am really in favor of reducing the static devicetree footprint. Your longterm intentions just were not clear from commit message, but you are now describing this as a workaround and there are bigger plans (for better).
Hello build bot (Jenkins), Nico Huber, Tim Wawrzynczak, Angel Pons, Kyösti Mälkki, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/49213
to look at the new patch set (#3).
Change subject: soc/intel/common/uart: Use simple(_s_) variants of PCI functions ......................................................................
soc/intel/common/uart: Use simple(_s_) variants of PCI functions
This change updates various uart_* functions to use simple(_s_) variants of PCI functions. This is done for a few reasons:
* __SIMPLE_DEVICE__ check can be dropped since the same data type can be used in early stages and ramstage. * Removes the requirement on early stage to walk the device tree to get access to the device structure. This allows linker-based device tree optimizations for early stages.
As part of this change, uart_get_device() is refactored and a new function uart_console_get_devfn() is added which returns pci_devfn_t in MMCONF format. It is then used directly by the _s_ variants of PCI functions.
Change-Id: I344037828118572ae5eb27c82c496d5e7a508a53 Signed-off-by: Furquan Shaikh furquan@google.com --- M src/soc/intel/common/block/uart/uart.c 1 file changed, 35 insertions(+), 42 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/49213/3
Attention is currently required from: Angel Pons. Furquan Shaikh 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 3:
(3 comments)
File src/soc/intel/common/block/uart/uart.c:
https://review.coreboot.org/c/coreboot/+/49213/comment/b851e479_5355678c PS2, Line 61: devfn
`uart_console_get_pci_bdf` ?
Done
https://review.coreboot.org/c/coreboot/+/49213/comment/b9cf3bff_ed2ea0aa PS2, Line 84: pci_devfn_t devfn
These should be renamed to `dev` to avoid confusion with actual devfn-only values elsewhere.
Done
https://review.coreboot.org/c/coreboot/+/49213/comment/ef7f8e41_53031f4f 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); : }
Ack.
Done
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 3: Code-Review+1
(1 comment)
File src/soc/intel/common/block/uart/uart.c:
https://review.coreboot.org/c/coreboot/+/49213/comment/0bb6f0ee_177a6b4a PS3, Line 26: devfn nit: `dev` here too
Attention is currently required from: Furquan Shaikh. Tim Wawrzynczak 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 3: Code-Review+1
Attention is currently required from: Angel Pons. Furquan Shaikh 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 3:
(1 comment)
File src/soc/intel/common/block/uart/uart.c:
https://review.coreboot.org/c/coreboot/+/49213/comment/dcdba790_3ebfde70 PS3, Line 26: devfn
nit: `dev` here too
Done
Attention is currently required from: Angel Pons. Hello build bot (Jenkins), Nico Huber, Tim Wawrzynczak, Angel Pons, Kyösti Mälkki, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/49213
to look at the new patch set (#4).
Change subject: soc/intel/common/uart: Use simple(_s_) variants of PCI functions ......................................................................
soc/intel/common/uart: Use simple(_s_) variants of PCI functions
This change updates various uart_* functions to use simple(_s_) variants of PCI functions. This is done for a few reasons:
* __SIMPLE_DEVICE__ check can be dropped since the same data type can be used in early stages and ramstage. * Removes the requirement on early stage to walk the device tree to get access to the device structure. This allows linker-based device tree optimizations for early stages.
As part of this change, uart_get_device() is refactored and a new function uart_console_get_devfn() is added which returns pci_devfn_t in MMCONF format. It is then used directly by the _s_ variants of PCI functions.
Change-Id: I344037828118572ae5eb27c82c496d5e7a508a53 Signed-off-by: Furquan Shaikh furquan@google.com --- M src/soc/intel/common/block/uart/uart.c 1 file changed, 35 insertions(+), 42 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/49213/4
Attention is currently required from: Furquan Shaikh, Angel Pons. Nico Huber 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 4: Code-Review+1
(2 comments)
File src/soc/intel/common/block/uart/uart.c:
https://review.coreboot.org/c/coreboot/+/49213/comment/c2bc59dd_2fb312d8 PS4, Line 20: PCI_COMMAND_MASTER Unrelated, this looks quite odd (without DRAM, what would a bus-master request target?).
https://review.coreboot.org/c/coreboot/+/49213/comment/ea5c48b9_523976b1 PS4, Line 72: return 0; This is very tricky, because `0` is a valid BDF. Please use `PCI_DEV_INVALID`.
Attention is currently required from: Furquan Shaikh, Angel Pons. Hello build bot (Jenkins), Nico Huber, Tim Wawrzynczak, Angel Pons, Kyösti Mälkki, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/49213
to look at the new patch set (#5).
Change subject: soc/intel/common/uart: Use simple(_s_) variants of PCI functions ......................................................................
soc/intel/common/uart: Use simple(_s_) variants of PCI functions
This change updates various uart_* functions to use simple(_s_) variants of PCI functions. This is done for a few reasons:
* __SIMPLE_DEVICE__ check can be dropped since the same data type can be used in early stages and ramstage. * Removes the requirement on early stage to walk the device tree to get access to the device structure. This allows linker-based device tree optimizations for early stages.
As part of this change, uart_get_device() is refactored and a new function uart_console_get_devfn() is added which returns pci_devfn_t in MMCONF format. It is then used directly by the _s_ variants of PCI functions.
Change-Id: I344037828118572ae5eb27c82c496d5e7a508a53 Signed-off-by: Furquan Shaikh furquan@google.com --- M src/soc/intel/common/block/uart/uart.c 1 file changed, 35 insertions(+), 42 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/49213/5
Attention is currently required from: Nico Huber, Angel Pons. Furquan Shaikh 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 5:
(1 comment)
File src/soc/intel/common/block/uart/uart.c:
https://review.coreboot.org/c/coreboot/+/49213/comment/177f5641_d5a5b6ab PS4, Line 72: return 0;
This is very tricky, because `0` is a valid BDF. Please use `PCI_DEV_INVALID`.
Good point. Updated in latest patchset.
Attention is currently required from: Furquan Shaikh, Angel Pons. Nico Huber 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 5: Code-Review+2
(1 comment)
File src/soc/intel/common/block/uart/uart.c:
https://review.coreboot.org/c/coreboot/+/49213/comment/daa095c9_eb3b2d49 PS4, Line 72: return 0;
Good point. Updated in latest patchset.
thanks!
Attention is currently required from: Angel Pons. Furquan Shaikh 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 5:
(1 comment)
File src/soc/intel/common/block/uart/uart.c:
https://review.coreboot.org/c/coreboot/+/49213/comment/4435baa6_51dc1a32 PS4, Line 20: PCI_COMMAND_MASTER
Unrelated, this looks quite odd (without DRAM, what would a bus-master request […]
Yeah, need to trace the history on this one.
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 5: Code-Review+2
Attention is currently required from: Furquan Shaikh. Karthik Ramasubramanian 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 5: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/49213 )
Change subject: soc/intel/common/uart: Use simple(_s_) variants of PCI functions ......................................................................
soc/intel/common/uart: Use simple(_s_) variants of PCI functions
This change updates various uart_* functions to use simple(_s_) variants of PCI functions. This is done for a few reasons:
* __SIMPLE_DEVICE__ check can be dropped since the same data type can be used in early stages and ramstage. * Removes the requirement on early stage to walk the device tree to get access to the device structure. This allows linker-based device tree optimizations for early stages.
As part of this change, uart_get_device() is refactored and a new function uart_console_get_devfn() is added which returns pci_devfn_t in MMCONF format. It is then used directly by the _s_ variants of PCI functions.
Change-Id: I344037828118572ae5eb27c82c496d5e7a508a53 Signed-off-by: Furquan Shaikh furquan@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/49213 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Nico Huber nico.h@gmx.de Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Karthik Ramasubramanian kramasub@google.com --- M src/soc/intel/common/block/uart/uart.c 1 file changed, 35 insertions(+), 42 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Angel Pons: Looks good to me, approved Karthik Ramasubramanian: Looks good to me, approved
diff --git a/src/soc/intel/common/block/uart/uart.c b/src/soc/intel/common/block/uart/uart.c index 559ba6d..69b443c 100644 --- a/src/soc/intel/common/block/uart/uart.c +++ b/src/soc/intel/common/block/uart/uart.c @@ -23,10 +23,10 @@ extern const struct uart_controller_config uart_ctrlr_config[]; extern const int uart_max_index;
-static void uart_lpss_init(const struct device *dev, uintptr_t baseaddr) +static void uart_lpss_init(pci_devfn_t dev, uintptr_t baseaddr) { /* Ensure controller is in D0 state */ - lpss_set_power_state(PCI_BDF(dev), STATE_D0); + lpss_set_power_state(dev, STATE_D0);
/* Take UART out of reset */ lpss_reset_release(baseaddr); @@ -58,60 +58,49 @@ return UART_CONSOLE_INVALID_INDEX; }
-static void uart_common_init(const struct device *device, uintptr_t baseaddr) +static pci_devfn_t uart_console_get_pci_bdf(void) { -#if defined(__SIMPLE_DEVICE__) - pci_devfn_t dev = PCI_BDF(device); -#else - const struct device *dev = device; -#endif + int devfn; + int index;
- /* Set UART base address */ - pci_write_config32(dev, PCI_BASE_ADDRESS_0, baseaddr); - - /* Enable memory access and bus master */ - pci_write_config16(dev, PCI_COMMAND, UART_PCI_ENABLE); - - uart_lpss_init(device, baseaddr); -} - -const struct device *uart_get_device(void) -{ /* * This function will get called even if INTEL_LPSS_UART_FOR_CONSOLE * config option is not selected. * By default return NULL in this case to avoid compilation errors. */ if (!CONFIG(INTEL_LPSS_UART_FOR_CONSOLE)) + return PCI_DEV_INVALID; + + index = uart_get_valid_index(); + if (index == UART_CONSOLE_INVALID_INDEX) + return PCI_DEV_INVALID; + + devfn = uart_ctrlr_config[index].devfn; + return PCI_DEV(0, PCI_SLOT(devfn), PCI_FUNC(devfn)); +} + +const struct device *uart_get_device(void) +{ + pci_devfn_t dev = uart_console_get_pci_bdf(); + if (dev == PCI_DEV_INVALID) return NULL;
- int console_index = uart_get_valid_index(); - - if (console_index != UART_CONSOLE_INVALID_INDEX) - return pcidev_path_on_root(uart_ctrlr_config[console_index].devfn); - else - return NULL; + return pcidev_path_on_root(PCI_DEV2DEVFN(dev)); }
bool uart_is_controller_initialized(void) { uintptr_t base; - const struct device *dev_uart = uart_get_device(); + pci_devfn_t dev = uart_console_get_pci_bdf();
- if (!dev_uart) + if (dev == PCI_DEV_INVALID) return false;
-#if defined(__SIMPLE_DEVICE__) - pci_devfn_t dev = PCI_BDF(dev_uart); -#else - const struct device *dev = dev_uart; -#endif - - base = pci_read_config32(dev, PCI_BASE_ADDRESS_0) & ~0xFFF; + base = pci_s_read_config32(dev, PCI_BASE_ADDRESS_0) & ~0xFFF; if (!base) return false;
- if ((pci_read_config16(dev, PCI_COMMAND) & UART_PCI_ENABLE) + if ((pci_s_read_config16(dev, PCI_COMMAND) & UART_PCI_ENABLE) != UART_PCI_ENABLE) return false;
@@ -129,15 +118,19 @@
void uart_bootblock_init(void) { - const struct device *dev_uart; + const uint32_t baseaddr = CONFIG_CONSOLE_UART_BASE_ADDRESS; + pci_devfn_t dev = uart_console_get_pci_bdf();
- dev_uart = uart_get_device(); - - if (!dev_uart) + if (dev == PCI_DEV_INVALID) return;
- /* Program UART BAR0, command, reset and clock register */ - uart_common_init(dev_uart, CONFIG_CONSOLE_UART_BASE_ADDRESS); + /* Set UART base address */ + pci_s_write_config32(dev, PCI_BASE_ADDRESS_0, baseaddr); + + /* Enable memory access and bus master */ + pci_s_write_config16(dev, PCI_COMMAND, UART_PCI_ENABLE); + + uart_lpss_init(dev, baseaddr);
/* Configure the 2 pads per UART. */ uart_configure_gpio_pads(); @@ -224,7 +217,7 @@
base = pci_read_config32(dev, PCI_BASE_ADDRESS_0) & ~0xFFF; if (base) - uart_lpss_init(dev, base); + uart_lpss_init(PCI_BDF(dev), base); } }
-
Angel Pons (Code Review) -
Furquan Shaikh (Code Review)
-
Karthik Ramasubramanian (Code Review)
-
Kyösti Mälkki (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Tim Wawrzynczak (Code Review)