Change in coreboot[master]: soc/intel/common/block/uart: Update the UART PCI device reference
Aamir Bohra has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34582 )
Change subject: soc/intel/common/block/uart: Update the UART PCI device reference ......................................................................
soc/intel/common/block/uart: Update the UART PCI device reference
This implementation revises the UART PCI device reference in common UART driver. The SOC functions have been aligned to provide the UART PCI device refernce using pcidev_path_on_root.
Change-Id: Ie0fe5991f3b0b9c596c3de9472e98e4091d7dd87 Signed-off-by: Aamir Bohra aamir.bohra@intel.com --- M src/soc/intel/apollolake/uart.c M src/soc/intel/cannonlake/uart.c M src/soc/intel/common/block/uart/uart.c M src/soc/intel/icelake/uart.c M src/soc/intel/skylake/uart.c 5 files changed, 19 insertions(+), 16 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/34582/1
diff --git a/src/soc/intel/apollolake/uart.c b/src/soc/intel/apollolake/uart.c index 27be4e6..de07a80 100644 --- a/src/soc/intel/apollolake/uart.c +++ b/src/soc/intel/apollolake/uart.c @@ -82,13 +82,13 @@ */ switch (uart_console) { case 0: - return (struct device *)PCH_DEV_UART0; + return (struct device *)pcidev_path_on_root(PCH_DEVFN_UART0); case 1: - return (struct device *)PCH_DEV_UART1; + return (struct device *)pcidev_path_on_root(PCH_DEVFN_UART1); case 2: - return (struct device *)PCH_DEV_UART2; + return (struct device *)pcidev_path_on_root(PCH_DEVFN_UART2); case 3: - return (struct device *)PCH_DEV_UART3; + return (struct device *)pcidev_path_on_root(PCH_DEVFN_UART3); default: printk(BIOS_ERR, "Invalid UART console index\n"); return NULL; diff --git a/src/soc/intel/cannonlake/uart.c b/src/soc/intel/cannonlake/uart.c index 7174a9a..1909996 100644 --- a/src/soc/intel/cannonlake/uart.c +++ b/src/soc/intel/cannonlake/uart.c @@ -58,11 +58,11 @@ */ switch (uart_console) { case 0: - return (struct device *)PCH_DEV_UART0; + return (struct device *)pcidev_path_on_root(PCH_DEVFN_UART0); case 1: - return (struct device *)PCH_DEV_UART1; + return (struct device *)pcidev_path_on_root(PCH_DEVFN_UART1); case 2: - return (struct device *)PCH_DEV_UART2; + return (struct device *)pcidev_path_on_root(PCH_DEVFN_UART2); default: printk(BIOS_ERR, "Invalid UART console index\n"); return NULL; diff --git a/src/soc/intel/common/block/uart/uart.c b/src/soc/intel/common/block/uart/uart.c index 82e5df4..6a4765d 100644 --- a/src/soc/intel/common/block/uart/uart.c +++ b/src/soc/intel/common/block/uart/uart.c @@ -71,7 +71,8 @@ void uart_common_init(struct device *device, uintptr_t baseaddr) { #if defined(__SIMPLE_DEVICE__) - pci_devfn_t dev = (pci_devfn_t)(uintptr_t)device; + unsigned int devfn = device->path.pci.devfn; + pci_devfn_t dev = PCI_DEV(0, PCI_SLOT(devfn), PCI_FUNC(devfn)); #else struct device *dev = device; #endif @@ -108,11 +109,13 @@ bool uart_is_controller_initialized(void) { uintptr_t base; + struct device *dev_uart = uart_get_device();
#if defined(__SIMPLE_DEVICE__) - pci_devfn_t dev = (pci_devfn_t)(uintptr_t)uart_get_device(); + unsigned int devfn = dev_uart->path.pci.devfn; + pci_devfn_t dev = PCI_DEV(0, PCI_SLOT(devfn), PCI_FUNC(devfn)); #else - struct device *dev = uart_get_device(); + struct device *dev = dev_uart; #endif if (!dev) return false; diff --git a/src/soc/intel/icelake/uart.c b/src/soc/intel/icelake/uart.c index 7174a9a..1909996 100644 --- a/src/soc/intel/icelake/uart.c +++ b/src/soc/intel/icelake/uart.c @@ -58,11 +58,11 @@ */ switch (uart_console) { case 0: - return (struct device *)PCH_DEV_UART0; + return (struct device *)pcidev_path_on_root(PCH_DEVFN_UART0); case 1: - return (struct device *)PCH_DEV_UART1; + return (struct device *)pcidev_path_on_root(PCH_DEVFN_UART1); case 2: - return (struct device *)PCH_DEV_UART2; + return (struct device *)pcidev_path_on_root(PCH_DEVFN_UART2); default: printk(BIOS_ERR, "Invalid UART console index\n"); return NULL; diff --git a/src/soc/intel/skylake/uart.c b/src/soc/intel/skylake/uart.c index 8b7c99e..ee181dc 100644 --- a/src/soc/intel/skylake/uart.c +++ b/src/soc/intel/skylake/uart.c @@ -59,11 +59,11 @@ */ switch (uart_console) { case 0: - return (struct device *)PCH_DEV_UART0; + return (struct device *)pcidev_path_on_root(PCH_DEVFN_UART0); case 1: - return (struct device *)PCH_DEV_UART1; + return (struct device *)pcidev_path_on_root(PCH_DEVFN_UART1); case 2: - return (struct device *)PCH_DEV_UART2; + return (struct device *)pcidev_path_on_root(PCH_DEVFN_UART2); default: printk(BIOS_ERR, "Invalid UART console index\n"); return NULL;
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34582 )
Change subject: soc/intel/common/block/uart: Update the UART PCI device reference ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34582/1/src/soc/intel/cannonlake/ua... File src/soc/intel/cannonlake/uart.c:
https://review.coreboot.org/c/coreboot/+/34582/1/src/soc/intel/cannonlake/ua... PS1, Line 61: return (struct device *)pcidev_path_on_root(PCH_DEVFN_UART0); why do you need a cast here? seems fishy.
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34582 )
Change subject: soc/intel/common/block/uart: Update the UART PCI device reference ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34582/1/src/soc/intel/cannonlake/ua... File src/soc/intel/cannonlake/uart.c:
https://review.coreboot.org/c/coreboot/+/34582/1/src/soc/intel/cannonlake/ua... PS1, Line 61: return (struct device *)pcidev_path_on_root(PCH_DEVFN_UART0);
why do you need a cast here? seems fishy.
That is to only drop the const qualifier from pcidev_path_on_root return data. Or else the function return type needs to be revised to const struct device * and then the entire dependent functions(consumers) also needs to be align to revised return type.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34582 )
Change subject: soc/intel/common/block/uart: Update the UART PCI device reference ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34582/1/src/soc/intel/cannonlake/ua... File src/soc/intel/cannonlake/uart.c:
https://review.coreboot.org/c/coreboot/+/34582/1/src/soc/intel/cannonlake/ua... PS1, Line 53: struct device *soc_uart_console_to_device(int uart_console) Use DEVTREE_CONST here
https://review.coreboot.org/c/coreboot/+/34582/1/src/soc/intel/common/block/... File src/soc/intel/common/block/uart/uart.c:
https://review.coreboot.org/c/coreboot/+/34582/1/src/soc/intel/common/block/... PS1, Line 75: pci_devfn_t dev = PCI_DEV(0, PCI_SLOT(devfn), PCI_FUNC(devfn)); Should be: dev = PCI_BDF(device);
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34582 )
Change subject: soc/intel/common/block/uart: Update the UART PCI device reference ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34582/1/src/soc/intel/cannonlake/ua... File src/soc/intel/cannonlake/uart.c:
https://review.coreboot.org/c/coreboot/+/34582/1/src/soc/intel/cannonlake/ua... PS1, Line 53: struct device *soc_uart_console_to_device(int uart_console)
Use DEVTREE_CONST here
The type cast below was to drop the const qualifier. If I add the const qualifier here, then the related chain of functions also needs to be updated to align to updated return type. i.e uart_get_device and its consumers, including the ramstage common acpi driver. Plus, I don't see the functions manipulating the value returned by soc_uart_console_to_device and merely consuming it for the device reference. Please let me know if you see a risk.
https://review.coreboot.org/c/coreboot/+/34582/1/src/soc/intel/common/block/... File src/soc/intel/common/block/uart/uart.c:
https://review.coreboot.org/c/coreboot/+/34582/1/src/soc/intel/common/block/... PS1, Line 75: pci_devfn_t dev = PCI_DEV(0, PCI_SLOT(devfn), PCI_FUNC(devfn));
Should be: […]
I tried replacing it with PCI_BDF, seems the pcidev_assert is not available in simple device environment. guess will have to stick with PCI_DEV, thoughts?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34582 )
Change subject: soc/intel/common/block/uart: Update the UART PCI device reference ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34582/1/src/soc/intel/cannonlake/ua... File src/soc/intel/cannonlake/uart.c:
https://review.coreboot.org/c/coreboot/+/34582/1/src/soc/intel/cannonlake/ua... PS1, Line 53: struct device *soc_uart_console_to_device(int uart_console)
Plus, I don't see the functions manipulating the value returned by soc_uart_console_to_device and merely consuming it for the device reference.
If that's the case, please use `const` in the whole chain.
Generally, I think it's sometimes ok to cast a `const` away. But only if you use it to call very local functions that are unlikely to change ever.
If you cast a `const` away and pass that pointer through an API to other compilation units, all hope is lost (read as: highest possible risk).
https://review.coreboot.org/c/coreboot/+/34582/1/src/soc/intel/common/block/... File src/soc/intel/common/block/uart/uart.c:
https://review.coreboot.org/c/coreboot/+/34582/1/src/soc/intel/common/block/... PS1, Line 146: uart_get_device Missing NULL check.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34582 )
Change subject: soc/intel/common/block/uart: Update the UART PCI device reference ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34582/1/src/soc/intel/common/block/... File src/soc/intel/common/block/uart/uart.c:
https://review.coreboot.org/c/coreboot/+/34582/1/src/soc/intel/common/block/... PS1, Line 112: uart_get_device Missing NULL check.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34582 )
Change subject: soc/intel/common/block/uart: Update the UART PCI device reference ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34582/1/src/soc/intel/common/block/... File src/soc/intel/common/block/uart/uart.c:
https://review.coreboot.org/c/coreboot/+/34582/1/src/soc/intel/common/block/... PS1, Line 75: pci_devfn_t dev = PCI_DEV(0, PCI_SLOT(devfn), PCI_FUNC(devfn));
I tried replacing it with PCI_BDF, seems the pcidev_assert is not available in simple device enviro […]
I think we can safely draw pcidev_die(), pcidev_bdf() and pcidev_assert() out of the #if in `device/pci_ops.h` (in a preceding change). Kyösti, what do you think?
Oh, the implementation of pcidev_die() would have to move / be copied, too.
Hello Patrick Rudolph, Paul Fagerburg, Subrata Banik, Tim Wawrzynczak, build bot (Jenkins), Nico Huber, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34582
to look at the new patch set (#2).
Change subject: soc/intel/common/block/uart: Update the UART PCI device reference ......................................................................
soc/intel/common/block/uart: Update the UART PCI device reference
This implementation revises the UART PCI device reference in common UART driver. The SOC functions have been aligned to provide the UART PCI device refernce using pcidev_path_on_root.
Change-Id: Ie0fe5991f3b0b9c596c3de9472e98e4091d7dd87 Signed-off-by: Aamir Bohra aamir.bohra@intel.com --- M src/soc/intel/apollolake/uart.c M src/soc/intel/cannonlake/uart.c M src/soc/intel/common/block/acpi/acpi.c M src/soc/intel/common/block/include/intelblocks/uart.h M src/soc/intel/common/block/uart/uart.c M src/soc/intel/icelake/uart.c M src/soc/intel/skylake/acpi.c M src/soc/intel/skylake/uart.c 8 files changed, 49 insertions(+), 34 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/34582/2
Hello Patrick Rudolph, Paul Fagerburg, Subrata Banik, Tim Wawrzynczak, build bot (Jenkins), Nico Huber, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34582
to look at the new patch set (#3).
Change subject: soc/intel/common/block/uart: Update the UART PCI device reference ......................................................................
soc/intel/common/block/uart: Update the UART PCI device reference
This implementation revises the UART PCI device reference in common UART driver. The SOC functions have been aligned to provide the UART PCI device refernce using pcidev_path_on_root.
The uart_get_device() return type is changed, and files in which it gets used are updated.
Change-Id: Ie0fe5991f3b0b9c596c3de9472e98e4091d7dd87 Signed-off-by: Aamir Bohra aamir.bohra@intel.com --- M src/soc/intel/apollolake/uart.c M src/soc/intel/cannonlake/uart.c M src/soc/intel/common/block/acpi/acpi.c M src/soc/intel/common/block/include/intelblocks/uart.h M src/soc/intel/common/block/uart/uart.c M src/soc/intel/icelake/uart.c M src/soc/intel/skylake/acpi.c M src/soc/intel/skylake/uart.c 8 files changed, 48 insertions(+), 33 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/34582/3
Hello Patrick Rudolph, Paul Fagerburg, Subrata Banik, Tim Wawrzynczak, build bot (Jenkins), Nico Huber, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34582
to look at the new patch set (#4).
Change subject: soc/intel/common/block/uart: Update the UART PCI device reference ......................................................................
soc/intel/common/block/uart: Update the UART PCI device reference
This implementation revises the UART PCI device reference in common UART driver. The SOC functions have been aligned to provide the UART PCI device refernce using pcidev_path_on_root.
The uart_get_device() return type is changed, and files in which it gets used are updated.
Change-Id: Ie0fe5991f3b0b9c596c3de9472e98e4091d7dd87 Signed-off-by: Aamir Bohra aamir.bohra@intel.com --- M src/soc/intel/apollolake/uart.c M src/soc/intel/cannonlake/uart.c M src/soc/intel/common/block/acpi/acpi.c M src/soc/intel/common/block/include/intelblocks/uart.h M src/soc/intel/common/block/uart/uart.c M src/soc/intel/icelake/uart.c M src/soc/intel/skylake/acpi.c M src/soc/intel/skylake/uart.c 8 files changed, 50 insertions(+), 35 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/34582/4
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34582 )
Change subject: soc/intel/common/block/uart: Update the UART PCI device reference ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34582/1/src/soc/intel/cannonlake/ua... File src/soc/intel/cannonlake/uart.c:
https://review.coreboot.org/c/coreboot/+/34582/1/src/soc/intel/cannonlake/ua... PS1, Line 53: struct device *soc_uart_console_to_device(int uart_console)
Plus, I don't see the functions manipulating the value returned by soc_uart_console_to_device and […]
Ok I have the common UART functions to return and accept const data. For ACPI common driver , I had to type cast , if I did not , find_resource function also needed change and corresponding calls needed to be updated(>240 references).
https://review.coreboot.org/c/coreboot/+/34582/1/src/soc/intel/common/block/... File src/soc/intel/common/block/uart/uart.c:
https://review.coreboot.org/c/coreboot/+/34582/1/src/soc/intel/common/block/... PS1, Line 112: uart_get_device
Missing NULL check.
Done
https://review.coreboot.org/c/coreboot/+/34582/1/src/soc/intel/common/block/... PS1, Line 146: uart_get_device
Missing NULL check.
Done
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34582 )
Change subject: soc/intel/common/block/uart: Update the UART PCI device reference ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34582/1/src/soc/intel/common/block/... File src/soc/intel/common/block/uart/uart.c:
https://review.coreboot.org/c/coreboot/+/34582/1/src/soc/intel/common/block/... PS1, Line 75: pci_devfn_t dev = PCI_DEV(0, PCI_SLOT(devfn), PCI_FUNC(devfn));
I think we can safely draw pcidev_die(), pcidev_bdf() and pcidev_assert() out […]
CB:34614
I have some bigger plans to remove __SIMPLE_DEVICE__ but soc/intel/*/pci_devs.h made it into slow manual process.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34582 )
Change subject: soc/intel/common/block/uart: Update the UART PCI device reference ......................................................................
Patch Set 4:
(2 comments)
I don't understand how you decided when to use `const` and when `DEVTREE_CONST`. If you'd use the latter for uart_get_ device(), too, the acpi code wouldn't make you any trouble.
https://review.coreboot.org/c/coreboot/+/34582/1/src/soc/intel/cannonlake/ua... File src/soc/intel/cannonlake/uart.c:
https://review.coreboot.org/c/coreboot/+/34582/1/src/soc/intel/cannonlake/ua... PS1, Line 53: struct device *soc_uart_console_to_device(int uart_console)
Ok I have the common UART functions to return and accept const data. […]
See CB:34613. No update of callers necessary, an implicit conversion to `const` is always allowed.
https://review.coreboot.org/c/coreboot/+/34582/4/src/soc/intel/skylake/acpi.... File src/soc/intel/skylake/acpi.c:
https://review.coreboot.org/c/coreboot/+/34582/4/src/soc/intel/skylake/acpi.... PS4, Line 663: uart_get_device That we need uart_get_device() to get a valid pointer shows that this is the wrong place for this code. If it were in the uart driver, we'd only have to check if the passed `device` is the debug UART and don't add the table otherwise.
For another day.
Hello Patrick Rudolph, Paul Fagerburg, Subrata Banik, Tim Wawrzynczak, build bot (Jenkins), Nico Huber, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34582
to look at the new patch set (#5).
Change subject: soc/intel/common/block/uart: Update the UART PCI device reference ......................................................................
soc/intel/common/block/uart: Update the UART PCI device reference
This implementation revises the UART PCI device reference in common UART driver. The SOC functions have been aligned to provide the UART PCI device refernce using pcidev_path_on_root.
The uart_get_device() return type is changed, and files in which it gets used are updated.
Change-Id: Ie0fe5991f3b0b9c596c3de9472e98e4091d7dd87 Signed-off-by: Aamir Bohra aamir.bohra@intel.com --- M src/soc/intel/apollolake/uart.c M src/soc/intel/cannonlake/uart.c M src/soc/intel/common/block/include/intelblocks/uart.h M src/soc/intel/common/block/uart/uart.c M src/soc/intel/icelake/uart.c M src/soc/intel/skylake/uart.c 6 files changed, 38 insertions(+), 29 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/34582/5
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34582 )
Change subject: soc/intel/common/block/uart: Update the UART PCI device reference ......................................................................
Patch Set 5:
Patch Set 4:
(2 comments)
I don't understand how you decided when to use `const` and when `DEVTREE_CONST`. If you'd use the latter for uart_get_ device(), too, the acpi code wouldn't make you any trouble.
I used the DEVTREE_CONST were I wanted to signify the devicetree reference and then switched to const in the common driver
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34582 )
Change subject: soc/intel/common/block/uart: Update the UART PCI device reference ......................................................................
Patch Set 5:
(1 comment)
Patch Set 4:
(2 comments)
I don't understand how you decided when to use `const` and when `DEVTREE_CONST`. If you'd use the latter for uart_get_ device(), too, the acpi code wouldn't make you any trouble.
https://review.coreboot.org/c/coreboot/+/34582/1/src/soc/intel/cannonlake/ua... File src/soc/intel/cannonlake/uart.c:
https://review.coreboot.org/c/coreboot/+/34582/1/src/soc/intel/cannonlake/ua... PS1, Line 53: struct device *soc_uart_console_to_device(int uart_console)
See CB:34613. No update of callers necessary, an implicit conversion to `const` […]
Ok, I uploaded a CL to constify the device arguments in acpi APIs https://review.coreboot.org/c/coreboot/+/34625/1
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34582 )
Change subject: soc/intel/common/block/uart: Update the UART PCI device reference ......................................................................
Patch Set 5: Code-Review+2
(3 comments)
With `const` on all paths now, we could also turn the remaining `DEVTREE_CONST` into `const`. But I don't want to be picky, this is good work.
https://review.coreboot.org/c/coreboot/+/34582/5/src/soc/intel/common/block/... File src/soc/intel/common/block/uart/uart.c:
https://review.coreboot.org/c/coreboot/+/34582/5/src/soc/intel/common/block/... PS5, Line 76: if (!dev) : return; Nit: As `device` is already checked by the caller, and the `pci_devfn_t` shouldn't be 0 anyway, this could be dropped now.
https://review.coreboot.org/c/coreboot/+/34582/5/src/soc/intel/common/block/... PS5, Line 80: pci_write_config32(dev, PCI_BASE_ADDRESS_0, baseaddr); Question for Kyösti, actually: If we'd use the simple version here (pci_s_write_config32()), we wouldn't need the #if above. Would such usage be encouraged? Do we want to keep the simple versions in the long run?
https://review.coreboot.org/c/coreboot/+/34582/5/src/soc/intel/common/block/... PS5, Line 147: return; Nit: This looks odd with the empty line above but none below.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34582 )
Change subject: soc/intel/common/block/uart: Update the UART PCI device reference ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34582/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34582/5//COMMIT_MSG@11 PS5, Line 11: refernce reference
Hello Patrick Rudolph, Paul Fagerburg, Subrata Banik, Tim Wawrzynczak, build bot (Jenkins), Nico Huber, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34582
to look at the new patch set (#6).
Change subject: soc/intel/common/block/uart: Update the UART PCI device reference ......................................................................
soc/intel/common/block/uart: Update the UART PCI device reference
This implementation revises the UART PCI device reference in common UART driver. The SOC functions have been aligned to provide the UART PCI device reference using pcidev_path_on_root.
The uart_get_device() return type is changed, and files in which it gets used are updated.
Change-Id: Ie0fe5991f3b0b9c596c3de9472e98e4091d7dd87 Signed-off-by: Aamir Bohra aamir.bohra@intel.com --- M src/soc/intel/apollolake/uart.c M src/soc/intel/cannonlake/uart.c M src/soc/intel/common/block/include/intelblocks/uart.h M src/soc/intel/common/block/uart/uart.c M src/soc/intel/icelake/uart.c M src/soc/intel/skylake/uart.c 6 files changed, 38 insertions(+), 31 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/34582/6
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34582 )
Change subject: soc/intel/common/block/uart: Update the UART PCI device reference ......................................................................
Patch Set 6:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34582/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34582/5//COMMIT_MSG@11 PS5, Line 11: refernce
reference
Done
https://review.coreboot.org/c/coreboot/+/34582/5/src/soc/intel/common/block/... File src/soc/intel/common/block/uart/uart.c:
https://review.coreboot.org/c/coreboot/+/34582/5/src/soc/intel/common/block/... PS5, Line 76: if (!dev) : return;
Nit: As `device` is already checked by the caller, and the `pci_devfn_t` […]
Done
https://review.coreboot.org/c/coreboot/+/34582/5/src/soc/intel/common/block/... PS5, Line 147: return;
Nit: This looks odd with the empty line above but none below.
Done. Added a blank line below
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34582 )
Change subject: soc/intel/common/block/uart: Update the UART PCI device reference ......................................................................
Patch Set 6: Code-Review+1
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34582 )
Change subject: soc/intel/common/block/uart: Update the UART PCI device reference ......................................................................
Patch Set 6: Code-Review+2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34582 )
Change subject: soc/intel/common/block/uart: Update the UART PCI device reference ......................................................................
Patch Set 6: Code-Review+2
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34582 )
Change subject: soc/intel/common/block/uart: Update the UART PCI device reference ......................................................................
Patch Set 6:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34582/1/src/soc/intel/cannonlake/ua... File src/soc/intel/cannonlake/uart.c:
https://review.coreboot.org/c/coreboot/+/34582/1/src/soc/intel/cannonlake/ua... PS1, Line 53: struct device *soc_uart_console_to_device(int uart_console)
Ok, I uploaded a CL to constify the device arguments in acpi APIs […]
Done
https://review.coreboot.org/c/coreboot/+/34582/1/src/soc/intel/cannonlake/ua... PS1, Line 61: return (struct device *)pcidev_path_on_root(PCH_DEVFN_UART0);
That is to only drop the const qualifier from pcidev_path_on_root return data. […]
Done
https://review.coreboot.org/c/coreboot/+/34582/1/src/soc/intel/common/block/... File src/soc/intel/common/block/uart/uart.c:
https://review.coreboot.org/c/coreboot/+/34582/1/src/soc/intel/common/block/... PS1, Line 75: pci_devfn_t dev = PCI_DEV(0, PCI_SLOT(devfn), PCI_FUNC(devfn));
CB:34614 […]
Done
Martin Roth has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34582 )
Change subject: soc/intel/common/block/uart: Update the UART PCI device reference ......................................................................
soc/intel/common/block/uart: Update the UART PCI device reference
This implementation revises the UART PCI device reference in common UART driver. The SOC functions have been aligned to provide the UART PCI device reference using pcidev_path_on_root.
The uart_get_device() return type is changed, and files in which it gets used are updated.
Change-Id: Ie0fe5991f3b0b9c596c3de9472e98e4091d7dd87 Signed-off-by: Aamir Bohra aamir.bohra@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/34582 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Paul Fagerburg pfagerburg@chromium.org Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org Reviewed-by: Nico Huber nico.h@gmx.de --- M src/soc/intel/apollolake/uart.c M src/soc/intel/cannonlake/uart.c M src/soc/intel/common/block/include/intelblocks/uart.h M src/soc/intel/common/block/uart/uart.c M src/soc/intel/icelake/uart.c M src/soc/intel/skylake/uart.c 6 files changed, 38 insertions(+), 31 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Tim Wawrzynczak: Looks good to me, approved Paul Fagerburg: Looks good to me, but someone else must approve
diff --git a/src/soc/intel/apollolake/uart.c b/src/soc/intel/apollolake/uart.c index 27be4e6..f8c4aaf 100644 --- a/src/soc/intel/apollolake/uart.c +++ b/src/soc/intel/apollolake/uart.c @@ -74,7 +74,7 @@
const int uart_max_index = ARRAY_SIZE(uart_gpio_pads);
-struct device *soc_uart_console_to_device(int uart_console) +DEVTREE_CONST struct device *soc_uart_console_to_device(int uart_console) { /* * if index is valid, this function will return corresponding structure @@ -82,13 +82,13 @@ */ switch (uart_console) { case 0: - return (struct device *)PCH_DEV_UART0; + return pcidev_path_on_root(PCH_DEVFN_UART0); case 1: - return (struct device *)PCH_DEV_UART1; + return pcidev_path_on_root(PCH_DEVFN_UART1); case 2: - return (struct device *)PCH_DEV_UART2; + return pcidev_path_on_root(PCH_DEVFN_UART2); case 3: - return (struct device *)PCH_DEV_UART3; + return pcidev_path_on_root(PCH_DEVFN_UART3); default: printk(BIOS_ERR, "Invalid UART console index\n"); return NULL; diff --git a/src/soc/intel/cannonlake/uart.c b/src/soc/intel/cannonlake/uart.c index 7174a9a..ae19acc 100644 --- a/src/soc/intel/cannonlake/uart.c +++ b/src/soc/intel/cannonlake/uart.c @@ -50,7 +50,7 @@
const int uart_max_index = ARRAY_SIZE(uart_gpio_pads);
-struct device *soc_uart_console_to_device(int uart_console) +DEVTREE_CONST struct device *soc_uart_console_to_device(int uart_console) { /* * if index is valid, this function will return corresponding structure @@ -58,11 +58,11 @@ */ switch (uart_console) { case 0: - return (struct device *)PCH_DEV_UART0; + return pcidev_path_on_root(PCH_DEVFN_UART0); case 1: - return (struct device *)PCH_DEV_UART1; + return pcidev_path_on_root(PCH_DEVFN_UART1); case 2: - return (struct device *)PCH_DEV_UART2; + return pcidev_path_on_root(PCH_DEVFN_UART2); default: printk(BIOS_ERR, "Invalid UART console index\n"); return NULL; diff --git a/src/soc/intel/common/block/include/intelblocks/uart.h b/src/soc/intel/common/block/include/intelblocks/uart.h index 55f259d..1b62421 100644 --- a/src/soc/intel/common/block/include/intelblocks/uart.h +++ b/src/soc/intel/common/block/include/intelblocks/uart.h @@ -40,7 +40,7 @@ * Common routine to initialize UART controller PCI config space, take it out of * reset and configure M/N dividers. */ -void uart_common_init(struct device *dev, uintptr_t baseaddr); +void uart_common_init(const struct device *dev, uintptr_t baseaddr);
/* * Check if UART debug controller is initialized @@ -72,7 +72,7 @@ * Pointer to device structure = If device has a UART debug controller. * NULL = otherwise */ -struct device *uart_get_device(void); +const struct device *uart_get_device(void);
/**************************** SoC callbacks ***********************************/
@@ -89,6 +89,6 @@ * Pointer to device structure = If device has a UART debug controller. * NULL = otherwise */ -struct device *soc_uart_console_to_device(int uart_console); +DEVTREE_CONST struct device *soc_uart_console_to_device(int uart_console);
#endif /* SOC_INTEL_COMMON_BLOCK_UART_H */ diff --git a/src/soc/intel/common/block/uart/uart.c b/src/soc/intel/common/block/uart/uart.c index 9d820ff..f556aed 100644 --- a/src/soc/intel/common/block/uart/uart.c +++ b/src/soc/intel/common/block/uart/uart.c @@ -65,15 +65,13 @@ return UART_CONSOLE_INVALID_INDEX; }
-void uart_common_init(struct device *device, uintptr_t baseaddr) +void uart_common_init(const struct device *device, uintptr_t baseaddr) { #if defined(__SIMPLE_DEVICE__) - pci_devfn_t dev = (pci_devfn_t)(uintptr_t)device; + pci_devfn_t dev = PCI_BDF(device); #else - struct device *dev = device; + const struct device *dev = device; #endif - if (!dev) - return;
/* Set UART base address */ pci_write_config32(dev, PCI_BASE_ADDRESS_0, baseaddr); @@ -84,7 +82,7 @@ uart_lpss_init(baseaddr); }
-struct device *uart_get_device(void) +const struct device *uart_get_device(void) { /* * This function will get called even if INTEL_LPSS_UART_FOR_CONSOLE @@ -105,14 +103,16 @@ bool uart_is_controller_initialized(void) { uintptr_t base; + const struct device *dev_uart = uart_get_device(); + + if (!dev_uart) + return false;
#if defined(__SIMPLE_DEVICE__) - pci_devfn_t dev = (pci_devfn_t)(uintptr_t)uart_get_device(); + pci_devfn_t dev = PCI_BDF(dev_uart); #else - struct device *dev = uart_get_device(); + const struct device *dev = dev_uart; #endif - if (!dev) - return false;
base = pci_read_config32(dev, PCI_BASE_ADDRESS_0) & ~0xFFF; if (!base) @@ -136,8 +136,15 @@
void uart_bootblock_init(void) { + const struct device *dev_uart; + + dev_uart = uart_get_device(); + + if (!dev_uart) + return; + /* Program UART BAR0, command, reset and clock register */ - uart_common_init(uart_get_device(), CONFIG_CONSOLE_UART_BASE_ADDRESS); + uart_common_init(dev_uart, CONFIG_CONSOLE_UART_BASE_ADDRESS);
/* Configure the 2 pads per UART. */ uart_configure_gpio_pads(); diff --git a/src/soc/intel/icelake/uart.c b/src/soc/intel/icelake/uart.c index 7174a9a..ae19acc 100644 --- a/src/soc/intel/icelake/uart.c +++ b/src/soc/intel/icelake/uart.c @@ -50,7 +50,7 @@
const int uart_max_index = ARRAY_SIZE(uart_gpio_pads);
-struct device *soc_uart_console_to_device(int uart_console) +DEVTREE_CONST struct device *soc_uart_console_to_device(int uart_console) { /* * if index is valid, this function will return corresponding structure @@ -58,11 +58,11 @@ */ switch (uart_console) { case 0: - return (struct device *)PCH_DEV_UART0; + return pcidev_path_on_root(PCH_DEVFN_UART0); case 1: - return (struct device *)PCH_DEV_UART1; + return pcidev_path_on_root(PCH_DEVFN_UART1); case 2: - return (struct device *)PCH_DEV_UART2; + return pcidev_path_on_root(PCH_DEVFN_UART2); default: printk(BIOS_ERR, "Invalid UART console index\n"); return NULL; diff --git a/src/soc/intel/skylake/uart.c b/src/soc/intel/skylake/uart.c index 8b7c99e..18fcf1b 100644 --- a/src/soc/intel/skylake/uart.c +++ b/src/soc/intel/skylake/uart.c @@ -51,7 +51,7 @@
const int uart_max_index = ARRAY_SIZE(uart_gpio_pads);
-struct device *soc_uart_console_to_device(int uart_console) +DEVTREE_CONST struct device *soc_uart_console_to_device(int uart_console) { /* * if index is valid, this function will return corresponding structure @@ -59,11 +59,11 @@ */ switch (uart_console) { case 0: - return (struct device *)PCH_DEV_UART0; + return pcidev_path_on_root(PCH_DEVFN_UART0); case 1: - return (struct device *)PCH_DEV_UART1; + return pcidev_path_on_root(PCH_DEVFN_UART1); case 2: - return (struct device *)PCH_DEV_UART2; + return pcidev_path_on_root(PCH_DEVFN_UART2); default: printk(BIOS_ERR, "Invalid UART console index\n"); return NULL;
-
Aamir Bohra (Code Review) -
Kyösti Mälkki (Code Review)
-
Martin Roth (Code Review)
-
Nico Huber (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Fagerburg (Code Review)
-
Paul Menzel (Code Review)
-
Tim Wawrzynczak (Code Review)