[PATCH 4/4] serialio: Support for pci serial ports
From f65c435b8e3caf7249a3fb25150e7055898ccc12 Mon Sep 17 00:00:00 2001 From: gaobin <gaobin@amazon.com> Date: Fri, 18 Oct 2019 23:00:21 -0700 Subject: [PATCH 4/4] serialio: Support for pci serial ports Some Intel PCHs integrate pci uarts which are used for serial port debugging. For compatibility purpose, BIOS implementation may assign 0x3f8 to the pci uart's io bar at PEI stage, later during DXE stage the pci uart's bar is re-assigned to a different address. As a result, we can't use the hard coded IO port 0x3f8 in SeaBIOS for debugging. Instead, we need read the port base address from the pci uart's BAR, either an IO BAR, or a 32bit memory BAR. This patch adds support for pci serial port debugging. Signed-off-by: gaobin <gaobin@amazon.com> --- src/Kconfig | 34 +++++++++++++++++++++++++++++++++- src/hw/serialio.c | 29 +++++++++++++++++++++++++++-- 2 files changed, 60 insertions(+), 3 deletions(-) diff --git a/src/Kconfig b/src/Kconfig index 6606ce4..6d9ce3b 100644 --- a/src/Kconfig +++ b/src/Kconfig @@ -550,8 +550,40 @@ menu "Debugging" default 0x3f8 help Base port for serial - generally 0x3f8, 0x2f8, 0x3e8, or 0x2e8. - config DEBUG_SERIAL_MMIO + config DEBUG_SERIAL_PCI_IO depends on DEBUG_LEVEL != 0 && !DEBUG_SERIAL + bool "Serial port debugging via PCI IO" + default n + help + Send debugging information to PCI serial port. + The base address of the uart is in the PCI device's IO bar. + config DEBUG_SERIAL_PCI_MEM32 + depends on DEBUG_LEVEL != 0 && !DEBUG_SERIAL && !DEBUG_SERIAL_PCI_IO + bool "Serial port debugging via PCI MEM32" + default n + help + Send debugging information to PCI serial port. + The base address of the uart is in the PCI device's MEM32 bar. + config DEBUG_SERIAL_PCI_BDF + depends on DEBUG_SERIAL_PCI_IO || DEBUG_SERIAL_PCI_MEM32 + hex "Serial port PCI bus/device/function" + range 0 ffff + help + 16bit hex of B:D:F of the PCI serial port. + Bit 15:8 - bus, bit 7:3 - device, bit 2:0 - function + E.g. for a PCI device: bus 0x0, dev 0x1a, func 0x1 you + should input 00d1. + config DEBUG_SERIAL_PCI_BAR + depends on DEBUG_SERIAL_PCI_IO || DEBUG_SERIAL_PCI_MEM32 + hex "Serial port PCI BAR index (0 - 5)" + range 0 5 + default 0 + help + The index of the BAR where the base address would be read + from for the PCI serial port. Only IO BAR and 32bit memory + BAR are supported. + config DEBUG_SERIAL_MMIO + depends on DEBUG_LEVEL != 0 && !DEBUG_SERIAL && !DEBUG_SERIAL_PCI_IO && !DEBUG_SERIAL_PCI_MEM32 bool "Serial port debugging via memory mapped IO" default n help diff --git a/src/hw/serialio.c b/src/hw/serialio.c index 3163344..9a8565c 100644 --- a/src/hw/serialio.c +++ b/src/hw/serialio.c @@ -9,6 +9,7 @@ #include "output.h" // dprintf #include "serialio.h" // serial_debug_preinit #include "x86.h" // outb +#include "pci.h" //pci_config_readw /**************************************************************** @@ -23,6 +24,15 @@ serial_debug_write(u8 offset, u8 val) { if (CONFIG_DEBUG_SERIAL) { outb(val, CONFIG_DEBUG_SERIAL_PORT + offset); + } else if (CONFIG_DEBUG_SERIAL_PCI_IO) { + u16 base = pci_config_readw(CONFIG_DEBUG_SERIAL_PCI_BDF, + 0x10 + CONFIG_DEBUG_SERIAL_PCI_BAR*4) & (~0x3); + outb(val, base + offset); + } else if (CONFIG_DEBUG_SERIAL_PCI_MEM32) { + ASSERT32FLAT(); + u32 base = pci_config_readl(CONFIG_DEBUG_SERIAL_PCI_BDF, + 0x10 + CONFIG_DEBUG_SERIAL_PCI_BAR*4) & (~0x7); + writeb((void*)base + 4*offset, val); } else if (CONFIG_DEBUG_SERIAL_MMIO) { ASSERT32FLAT(); writeb((void*)CONFIG_DEBUG_SERIAL_MEM_ADDRESS + 4*offset, val); @@ -35,6 +45,17 @@ serial_debug_read(u8 offset) { if (CONFIG_DEBUG_SERIAL) return inb(CONFIG_DEBUG_SERIAL_PORT + offset); + if (CONFIG_DEBUG_SERIAL_PCI_IO) { + u16 base = pci_config_readw(CONFIG_DEBUG_SERIAL_PCI_BDF, + 0x10 + CONFIG_DEBUG_SERIAL_PCI_BAR*4) & (~0x3); + return inb(base + offset); + } + if (CONFIG_DEBUG_SERIAL_PCI_MEM32) { + ASSERT32FLAT(); + u32 base = pci_config_readl(CONFIG_DEBUG_SERIAL_PCI_BDF, + 0x10 + CONFIG_DEBUG_SERIAL_PCI_BAR*4) & (~0x7); + return readb((void*)base + 4*offset); + } if (CONFIG_DEBUG_SERIAL_MMIO) { ASSERT32FLAT(); return readb((void*)CONFIG_DEBUG_SERIAL_MEM_ADDRESS + 4*offset); @@ -65,7 +86,9 @@ serial_debug_preinit(void) static void serial_debug(char c) { - if (!CONFIG_DEBUG_SERIAL && (!CONFIG_DEBUG_SERIAL_MMIO || MODESEGMENT)) + if (!CONFIG_DEBUG_SERIAL && !CONFIG_DEBUG_SERIAL_PCI_IO && + ((!CONFIG_DEBUG_SERIAL_MMIO && !CONFIG_DEBUG_SERIAL_PCI_MEM32) || + MODESEGMENT)) return; int timeout = DEBUG_TIMEOUT; while ((serial_debug_read(SEROFF_LSR) & 0x20) != 0x20) @@ -87,7 +110,9 @@ serial_debug_putc(char c) void serial_debug_flush(void) { - if (!CONFIG_DEBUG_SERIAL && (!CONFIG_DEBUG_SERIAL_MMIO || MODESEGMENT)) + if (!CONFIG_DEBUG_SERIAL && !CONFIG_DEBUG_SERIAL_PCI_IO && + ((!CONFIG_DEBUG_SERIAL_MMIO && !CONFIG_DEBUG_SERIAL_PCI_MEM32) || + MODESEGMENT)) return; int timeout = DEBUG_TIMEOUT; while ((serial_debug_read(SEROFF_LSR) & 0x60) != 0x60) -- 2.17.1
Dear gaobin, Thank you for your changes. Some nit picks: Please configure git with `git config --global user.name "…"` to use your real name. You can also use `git send-email` to send the changes. On 2019-11-26 04:27, Your Real Name wrote:
From f65c435b8e3caf7249a3fb25150e7055898ccc12 Mon Sep 17 00:00:00 2001 From: gaobin <gaobin@amazon.com> Date: Fri, 18 Oct 2019 23:00:21 -0700 Subject: [PATCH 4/4] serialio: Support for pci serial ports
Make it a statement.
serialio: Support PCI serial ports
serialio: Add support for PCI serial ports
Some Intel PCHs integrate pci uarts which are used for serial port debugging. For compatibility purpose, BIOS implementation may assign 0x3f8 to the pci uart's io bar at PEI stage, later during DXE stage the pci uart's bar is re-assigned to a different address. As a result, we can't use the hard coded IO port 0x3f8 in SeaBIOS for debugging. Instead, we need read the port base
need *to* read
address from the pci uart's BAR, either an IO BAR, or a 32bit memory BAR. This patch adds support for pci serial port debugging.
Please add a command how to test this with QEMU for example.
Signed-off-by: gaobin <gaobin@amazon.com> --- src/Kconfig | 34 +++++++++++++++++++++++++++++++++- src/hw/serialio.c | 29 +++++++++++++++++++++++++++-- 2 files changed, 60 insertions(+), 3 deletions(-)
diff --git a/src/Kconfig b/src/Kconfig index 6606ce4..6d9ce3b 100644 --- a/src/Kconfig +++ b/src/Kconfig @@ -550,8 +550,40 @@ menu "Debugging" default 0x3f8 help Base port for serial - generally 0x3f8, 0x2f8, 0x3e8, or 0x2e8. - config DEBUG_SERIAL_MMIO + config DEBUG_SERIAL_PCI_IO depends on DEBUG_LEVEL != 0 && !DEBUG_SERIAL + bool "Serial port debugging via PCI IO" + default n + help + Send debugging information to PCI serial port. + The base address of the uart is in the PCI device's IO bar.
Please spell it UART.
+ config DEBUG_SERIAL_PCI_MEM32 + depends on DEBUG_LEVEL != 0 && !DEBUG_SERIAL && !DEBUG_SERIAL_PCI_IO + bool "Serial port debugging via PCI MEM32" + default n + help + Send debugging information to PCI serial port. + The base address of the uart is in the PCI device's MEM32 bar. + config DEBUG_SERIAL_PCI_BDF + depends on DEBUG_SERIAL_PCI_IO || DEBUG_SERIAL_PCI_MEM32 + hex "Serial port PCI bus/device/function" + range 0 ffff + help + 16bit hex of B:D:F of the PCI serial port. + Bit 15:8 - bus, bit 7:3 - device, bit 2:0 - function + E.g. for a PCI device: bus 0x0, dev 0x1a, func 0x1 you + should input 00d1. + config DEBUG_SERIAL_PCI_BAR + depends on DEBUG_SERIAL_PCI_IO || DEBUG_SERIAL_PCI_MEM32 + hex "Serial port PCI BAR index (0 - 5)" + range 0 5 + default 0 + help + The index of the BAR where the base address would be read + from for the PCI serial port. Only IO BAR and 32bit memory + BAR are supported. + config DEBUG_SERIAL_MMIO + depends on DEBUG_LEVEL != 0 && !DEBUG_SERIAL && !DEBUG_SERIAL_PCI_IO && !DEBUG_SERIAL_PCI_MEM32 bool "Serial port debugging via memory mapped IO" default n help diff --git a/src/hw/serialio.c b/src/hw/serialio.c index 3163344..9a8565c 100644 --- a/src/hw/serialio.c +++ b/src/hw/serialio.c @@ -9,6 +9,7 @@ #include "output.h" // dprintf #include "serialio.h" // serial_debug_preinit #include "x86.h" // outb +#include "pci.h" //pci_config_readw
/**************************************************************** @@ -23,6 +24,15 @@ serial_debug_write(u8 offset, u8 val) { if (CONFIG_DEBUG_SERIAL) { outb(val, CONFIG_DEBUG_SERIAL_PORT + offset); + } else if (CONFIG_DEBUG_SERIAL_PCI_IO) { + u16 base = pci_config_readw(CONFIG_DEBUG_SERIAL_PCI_BDF, + 0x10 + CONFIG_DEBUG_SERIAL_PCI_BAR*4) & (~0x3); + outb(val, base + offset); + } else if (CONFIG_DEBUG_SERIAL_PCI_MEM32) { + ASSERT32FLAT(); + u32 base = pci_config_readl(CONFIG_DEBUG_SERIAL_PCI_BDF,
Please use spaces for indentation.
+ 0x10 + CONFIG_DEBUG_SERIAL_PCI_BAR*4) & (~0x7); + writeb((void*)base + 4*offset, val); } else if (CONFIG_DEBUG_SERIAL_MMIO) { ASSERT32FLAT(); writeb((void*)CONFIG_DEBUG_SERIAL_MEM_ADDRESS + 4*offset, val); @@ -35,6 +45,17 @@ serial_debug_read(u8 offset) { if (CONFIG_DEBUG_SERIAL) return inb(CONFIG_DEBUG_SERIAL_PORT + offset); + if (CONFIG_DEBUG_SERIAL_PCI_IO) { + u16 base = pci_config_readw(CONFIG_DEBUG_SERIAL_PCI_BDF, + 0x10 + CONFIG_DEBUG_SERIAL_PCI_BAR*4) & (~0x3); + return inb(base + offset);
Please fix the indentation/alignment to follow the coding style. Also below.
+ } + if (CONFIG_DEBUG_SERIAL_PCI_MEM32) { + ASSERT32FLAT(); + u32 base = pci_config_readl(CONFIG_DEBUG_SERIAL_PCI_BDF, + 0x10 + CONFIG_DEBUG_SERIAL_PCI_BAR*4) & (~0x7); + return readb((void*)base + 4*offset); + } if (CONFIG_DEBUG_SERIAL_MMIO) { ASSERT32FLAT(); return readb((void*)CONFIG_DEBUG_SERIAL_MEM_ADDRESS + 4*offset); @@ -65,7 +86,9 @@ serial_debug_preinit(void) static void serial_debug(char c) { - if (!CONFIG_DEBUG_SERIAL && (!CONFIG_DEBUG_SERIAL_MMIO || MODESEGMENT)) + if (!CONFIG_DEBUG_SERIAL && !CONFIG_DEBUG_SERIAL_PCI_IO && + ((!CONFIG_DEBUG_SERIAL_MMIO && !CONFIG_DEBUG_SERIAL_PCI_MEM32) || + MODESEGMENT)) return; int timeout = DEBUG_TIMEOUT; while ((serial_debug_read(SEROFF_LSR) & 0x20) != 0x20) @@ -87,7 +110,9 @@ serial_debug_putc(char c) void serial_debug_flush(void) { - if (!CONFIG_DEBUG_SERIAL && (!CONFIG_DEBUG_SERIAL_MMIO || MODESEGMENT)) + if (!CONFIG_DEBUG_SERIAL && !CONFIG_DEBUG_SERIAL_PCI_IO && + ((!CONFIG_DEBUG_SERIAL_MMIO && !CONFIG_DEBUG_SERIAL_PCI_MEM32) || + MODESEGMENT)) return; int timeout = DEBUG_TIMEOUT; while ((serial_debug_read(SEROFF_LSR) & 0x60) != 0x60)
Kind regards, Paul
On Tue, Nov 26, 2019 at 11:29:09AM +0100, Paul Menzel wrote:
Dear gaobin,
Thank you for your changes. Some nit picks:
Please configure git with `git config --global user.name "…"` to use your real name. You can also use `git send-email` to send the changes.
Thanks for your review. I'll fix them in the next revision.
On 2019-11-26 04:27, Your Real Name wrote:
From f65c435b8e3caf7249a3fb25150e7055898ccc12 Mon Sep 17 00:00:00 2001 From: gaobin <gaobin@amazon.com> Date: Fri, 18 Oct 2019 23:00:21 -0700 Subject: [PATCH 4/4] serialio: Support for pci serial ports
Make it a statement.
serialio: Support PCI serial ports
serialio: Add support for PCI serial ports
Some Intel PCHs integrate pci uarts which are used for serial port debugging. For compatibility purpose, BIOS implementation may assign 0x3f8 to the pci uart's io bar at PEI stage, later during DXE stage the pci uart's bar is re-assigned to a different address. As a result, we can't use the hard coded IO port 0x3f8 in SeaBIOS for debugging. Instead, we need read the port base
need *to* read
address from the pci uart's BAR, either an IO BAR, or a 32bit memory BAR. This patch adds support for pci serial port debugging.
Please add a command how to test this with QEMU for example.
Signed-off-by: gaobin <gaobin@amazon.com> --- src/Kconfig | 34 +++++++++++++++++++++++++++++++++- src/hw/serialio.c | 29 +++++++++++++++++++++++++++-- 2 files changed, 60 insertions(+), 3 deletions(-)
diff --git a/src/Kconfig b/src/Kconfig index 6606ce4..6d9ce3b 100644 --- a/src/Kconfig +++ b/src/Kconfig @@ -550,8 +550,40 @@ menu "Debugging" default 0x3f8 help Base port for serial - generally 0x3f8, 0x2f8, 0x3e8, or 0x2e8. - config DEBUG_SERIAL_MMIO + config DEBUG_SERIAL_PCI_IO depends on DEBUG_LEVEL != 0 && !DEBUG_SERIAL + bool "Serial port debugging via PCI IO" + default n + help + Send debugging information to PCI serial port. + The base address of the uart is in the PCI device's IO bar.
Please spell it UART.
+ config DEBUG_SERIAL_PCI_MEM32 + depends on DEBUG_LEVEL != 0 && !DEBUG_SERIAL && !DEBUG_SERIAL_PCI_IO + bool "Serial port debugging via PCI MEM32" + default n + help + Send debugging information to PCI serial port. + The base address of the uart is in the PCI device's MEM32 bar. + config DEBUG_SERIAL_PCI_BDF + depends on DEBUG_SERIAL_PCI_IO || DEBUG_SERIAL_PCI_MEM32 + hex "Serial port PCI bus/device/function" + range 0 ffff + help + 16bit hex of B:D:F of the PCI serial port. + Bit 15:8 - bus, bit 7:3 - device, bit 2:0 - function + E.g. for a PCI device: bus 0x0, dev 0x1a, func 0x1 you + should input 00d1. + config DEBUG_SERIAL_PCI_BAR + depends on DEBUG_SERIAL_PCI_IO || DEBUG_SERIAL_PCI_MEM32 + hex "Serial port PCI BAR index (0 - 5)" + range 0 5 + default 0 + help + The index of the BAR where the base address would be read + from for the PCI serial port. Only IO BAR and 32bit memory + BAR are supported. + config DEBUG_SERIAL_MMIO + depends on DEBUG_LEVEL != 0 && !DEBUG_SERIAL && !DEBUG_SERIAL_PCI_IO && !DEBUG_SERIAL_PCI_MEM32 bool "Serial port debugging via memory mapped IO" default n help diff --git a/src/hw/serialio.c b/src/hw/serialio.c index 3163344..9a8565c 100644 --- a/src/hw/serialio.c +++ b/src/hw/serialio.c @@ -9,6 +9,7 @@ #include "output.h" // dprintf #include "serialio.h" // serial_debug_preinit #include "x86.h" // outb +#include "pci.h" //pci_config_readw
/**************************************************************** @@ -23,6 +24,15 @@ serial_debug_write(u8 offset, u8 val) { if (CONFIG_DEBUG_SERIAL) { outb(val, CONFIG_DEBUG_SERIAL_PORT + offset); + } else if (CONFIG_DEBUG_SERIAL_PCI_IO) { + u16 base = pci_config_readw(CONFIG_DEBUG_SERIAL_PCI_BDF, + 0x10 + CONFIG_DEBUG_SERIAL_PCI_BAR*4) & (~0x3); + outb(val, base + offset); + } else if (CONFIG_DEBUG_SERIAL_PCI_MEM32) { + ASSERT32FLAT(); + u32 base = pci_config_readl(CONFIG_DEBUG_SERIAL_PCI_BDF,
Please use spaces for indentation.
+ 0x10 + CONFIG_DEBUG_SERIAL_PCI_BAR*4) & (~0x7); + writeb((void*)base + 4*offset, val); } else if (CONFIG_DEBUG_SERIAL_MMIO) { ASSERT32FLAT(); writeb((void*)CONFIG_DEBUG_SERIAL_MEM_ADDRESS + 4*offset, val); @@ -35,6 +45,17 @@ serial_debug_read(u8 offset) { if (CONFIG_DEBUG_SERIAL) return inb(CONFIG_DEBUG_SERIAL_PORT + offset); + if (CONFIG_DEBUG_SERIAL_PCI_IO) { + u16 base = pci_config_readw(CONFIG_DEBUG_SERIAL_PCI_BDF, + 0x10 + CONFIG_DEBUG_SERIAL_PCI_BAR*4) & (~0x3); + return inb(base + offset);
Please fix the indentation/alignment to follow the coding style. Also below.
+ } + if (CONFIG_DEBUG_SERIAL_PCI_MEM32) { + ASSERT32FLAT(); + u32 base = pci_config_readl(CONFIG_DEBUG_SERIAL_PCI_BDF, + 0x10 + CONFIG_DEBUG_SERIAL_PCI_BAR*4) & (~0x7); + return readb((void*)base + 4*offset); + } if (CONFIG_DEBUG_SERIAL_MMIO) { ASSERT32FLAT(); return readb((void*)CONFIG_DEBUG_SERIAL_MEM_ADDRESS + 4*offset); @@ -65,7 +86,9 @@ serial_debug_preinit(void) static void serial_debug(char c) { - if (!CONFIG_DEBUG_SERIAL && (!CONFIG_DEBUG_SERIAL_MMIO || MODESEGMENT)) + if (!CONFIG_DEBUG_SERIAL && !CONFIG_DEBUG_SERIAL_PCI_IO && + ((!CONFIG_DEBUG_SERIAL_MMIO && !CONFIG_DEBUG_SERIAL_PCI_MEM32) || + MODESEGMENT)) return; int timeout = DEBUG_TIMEOUT; while ((serial_debug_read(SEROFF_LSR) & 0x20) != 0x20) @@ -87,7 +110,9 @@ serial_debug_putc(char c) void serial_debug_flush(void) { - if (!CONFIG_DEBUG_SERIAL && (!CONFIG_DEBUG_SERIAL_MMIO || MODESEGMENT)) + if (!CONFIG_DEBUG_SERIAL && !CONFIG_DEBUG_SERIAL_PCI_IO && + ((!CONFIG_DEBUG_SERIAL_MMIO && !CONFIG_DEBUG_SERIAL_PCI_MEM32) || + MODESEGMENT)) return; int timeout = DEBUG_TIMEOUT; while ((serial_debug_read(SEROFF_LSR) & 0x60) != 0x60)
Kind regards,
Paul
Hi,
Some Intel PCHs integrate pci uarts which are used for serial port debugging. For compatibility purpose, BIOS implementation may assign 0x3f8 to the pci uart's io bar at PEI stage, later during DXE stage the pci uart's bar is re-assigned to a different address.
What firmware you are running on this hardware? Something tianocore based I guess? Is the code public?
As a result, we can't use the hard coded IO port 0x3f8 in SeaBIOS for debugging. Instead, we need read the port base address from the pci uart's BAR, either an IO BAR, or a 32bit memory BAR.
Ok, but ...
+ config DEBUG_SERIAL_PCI_BDF + depends on DEBUG_SERIAL_PCI_IO || DEBUG_SERIAL_PCI_MEM32 + hex "Serial port PCI bus/device/function"
... hardcoding the BDF instead isn't that great either. Can't we just find the device while scanning for pci devices? cheers, Gerd
On Wed, Nov 27, 2019 at 08:37:38AM +0100, Gerd Hoffmann wrote:
Hi,
Some Intel PCHs integrate pci uarts which are used for serial port debugging. For compatibility purpose, BIOS implementation may assign 0x3f8 to the pci uart's io bar at PEI stage, later during DXE stage the pci uart's bar is re-assigned to a different address.
What firmware you are running on this hardware? Something tianocore based I guess? Is the code public?
The firmware we're running on is Intel RC(Reference Code) under NDA so we can not share here.
As a result, we can't use the hard coded IO port 0x3f8 in SeaBIOS for debugging. Instead, we need read the port base address from the pci uart's BAR, either an IO BAR, or a 32bit memory BAR.
Ok, but ...
+ config DEBUG_SERIAL_PCI_BDF + depends on DEBUG_SERIAL_PCI_IO || DEBUG_SERIAL_PCI_MEM32 + hex "Serial port PCI bus/device/function"
... hardcoding the BDF instead isn't that great either. Can't we just find the device while scanning for pci devices?
There might be multiple pci uart devices. So hardcoding the BDF seems to be a only solution. Actually this solution is similar to the port based uart( hard coding the port) and mmio based uart(hard coding the mmio address). Here we hard code the pci BDF.
cheers, Gerd
As a result, we can't use the hard coded IO port 0x3f8 in SeaBIOS for debugging. Instead, we need read the port base address from the pci uart's BAR, either an IO BAR, or a 32bit memory BAR.
Ok, but ...
+ config DEBUG_SERIAL_PCI_BDF + depends on DEBUG_SERIAL_PCI_IO || DEBUG_SERIAL_PCI_MEM32 + hex "Serial port PCI bus/device/function"
... hardcoding the BDF instead isn't that great either. Can't we just find the device while scanning for pci devices?
There might be multiple pci uart devices.
Which I doubt is actually the case. My guess would be you have a virtual serial device emulated by ME or something similar for serial-over-lan console access. And there is exactly one of these per machine.
So hardcoding the BDF seems to be a only solution.
Why? Just use the first device you find by default. Offering the option to specify the preferred device in case multiple devices exist is fine, but I doubt this is needed.
Actually this solution is similar to the port based uart( hard coding the port) and mmio based uart(hard coding the mmio address). Here we hard code the pci BDF.
Well, there is no way to reliable discover classic ioport uarts which are located behind a isa/lpc bridge, so we have to hardcode the port. There is no other option. PCI devices are discoverable, so we can do better than hardcoding the bdf. And given that seabios scans the pci bus anyway I think we should do that. This also allows to specify the preferred device by other means than the bdf, you could use the PCI ID instead for example. cheers, Gerd
On Tue, Dec 03, 2019 at 11:27:39AM +0100, Gerd Hoffmann wrote:
As a result, we can't use the hard coded IO port 0x3f8 in SeaBIOS for debugging. Instead, we need read the port base address from the pci uart's BAR, either an IO BAR, or a 32bit memory BAR.
Ok, but ...
+ config DEBUG_SERIAL_PCI_BDF + depends on DEBUG_SERIAL_PCI_IO || DEBUG_SERIAL_PCI_MEM32 + hex "Serial port PCI bus/device/function"
... hardcoding the BDF instead isn't that great either. Can't we just find the device while scanning for pci devices?
There might be multiple pci uart devices.
Which I doubt is actually the case. My guess would be you have a virtual serial device emulated by ME or something similar for serial-over-lan console access. And there is exactly one of these per machine.
In my case they are 3 physical PCI HS-UARTs directly from the SoC(but not inside the ME).
So hardcoding the BDF seems to be a only solution.
Why? Just use the first device you find by default. Offering the option to specify the preferred device in case multiple devices exist is fine, but I doubt this is needed.
Actually this solution is similar to the port based uart( hard coding the port) and mmio based uart(hard coding the mmio address). Here we hard code the pci BDF.
Well, there is no way to reliable discover classic ioport uarts which are located behind a isa/lpc bridge, so we have to hardcode the port. There is no other option.
PCI devices are discoverable, so we can do better than hardcoding the bdf. And given that seabios scans the pci bus anyway I think we should do that. This also allows to specify the preferred device by other means than the bdf, you could use the PCI ID instead for example.
I'm ok to use Base-Class/Sub-Class = 0x07/0x00 to filter out the PCI serial devices and choose the first one as the debug port. However, debug port needs to be enabled early, before seabios scans PCI bus. I think we don't want to scan the pci bus by ourself in src/hw/serialio.c, but it's too late to wait for seabios to scan the bus for us. Please advice a feasible solution.
cheers, Gerd
Hi,
I'm ok to use Base-Class/Sub-Class = 0x07/0x00 to filter out the PCI serial devices and choose the first one as the debug port. However, debug port needs to be enabled early, before seabios scans PCI bus. I think we don't want to scan the pci bus by ourself in src/hw/serialio.c, but it's too late to wait for seabios to scan the bus for us. Please advice a feasible solution.
You've mentioned elsewhere in the thread that the serial port is mapped to the legacy 0x3f8 ioport initially. So can we just use that before the PCI scan happens? cheers, Gerd
On Thu, Dec 05, 2019 at 06:52:46AM +0100, Gerd Hoffmann wrote:
Hi,
I'm ok to use Base-Class/Sub-Class = 0x07/0x00 to filter out the PCI serial devices and choose the first one as the debug port. However, debug port needs to be enabled early, before seabios scans PCI bus. I think we don't want to scan the pci bus by ourself in src/hw/serialio.c, but it's too late to wait for seabios to scan the bus for us. Please advice a feasible solution.
You've mentioned elsewhere in the thread that the serial port is mapped to the legacy 0x3f8 ioport initially. So can we just use that before the PCI scan happens?
The transition from 0x3f8 to an other IO address happened in EDK2 based BIOS, during the transition from PEI to DXE where the PCI bus is scanned resources are allocated. The first time BIOS calls to SeaBIOS, the IO address has already been changed to a non-0x3f8 one.
cheers, Gerd
On Fri, Dec 06, 2019 at 08:05:22PM -0800, Your Real Name wrote:
On Thu, Dec 05, 2019 at 06:52:46AM +0100, Gerd Hoffmann wrote:
Hi,
I'm ok to use Base-Class/Sub-Class = 0x07/0x00 to filter out the PCI serial devices and choose the first one as the debug port. However, debug port needs to be enabled early, before seabios scans PCI bus. I think we don't want to scan the pci bus by ourself in src/hw/serialio.c, but it's too late to wait for seabios to scan the bus for us. Please advice a feasible solution.
You've mentioned elsewhere in the thread that the serial port is mapped to the legacy 0x3f8 ioport initially. So can we just use that before the PCI scan happens?
The transition from 0x3f8 to an other IO address happened in EDK2 based BIOS, during the transition from PEI to DXE where the PCI bus is scanned resources are allocated. The first time BIOS calls to SeaBIOS, the IO address has already been changed to a non-0x3f8 one.
So seabios will never see the iobase move. Reading the iobase from config space for each char printed should not be needed then. Also: Can't you just set CONFIG_DEBUG_SERIAL_PORT? Failing that: I'd suggest to create a variable for the serial port iobase, defaulting to CONFIG_DEBUG_SERIAL_PORT. Add a serial_debuf_pci_init(int bdf) helper function to read the iobase from pci config space and set that variable. That helper can then be hooked into pci scan, or called with a hardcoded bdf for early debug output, or both. cheers, Gerd
On Mon, Dec 09, 2019 at 01:18:52PM +0100, Gerd Hoffmann wrote:
On Fri, Dec 06, 2019 at 08:05:22PM -0800, Your Real Name wrote:
On Thu, Dec 05, 2019 at 06:52:46AM +0100, Gerd Hoffmann wrote:
Hi,
I'm ok to use Base-Class/Sub-Class = 0x07/0x00 to filter out the PCI serial devices and choose the first one as the debug port. However, debug port needs to be enabled early, before seabios scans PCI bus. I think we don't want to scan the pci bus by ourself in src/hw/serialio.c, but it's too late to wait for seabios to scan the bus for us. Please advice a feasible solution.
You've mentioned elsewhere in the thread that the serial port is mapped to the legacy 0x3f8 ioport initially. So can we just use that before the PCI scan happens?
The transition from 0x3f8 to an other IO address happened in EDK2 based BIOS, during the transition from PEI to DXE where the PCI bus is scanned resources are allocated. The first time BIOS calls to SeaBIOS, the IO address has already been changed to a non-0x3f8 one.
So seabios will never see the iobase move. Reading the iobase from config space for each char printed should not be needed then. Also: Can't you just set CONFIG_DEBUG_SERIAL_PORT?
I can but it's not convenient. First, I need to find that port number by diving into the code and also it's platform dependent. Second, it's variable on configuration(e.g. if some devices are enabled/disabled, the port number may also change).
Failing that: I'd suggest to create a variable for the serial port iobase, defaulting to CONFIG_DEBUG_SERIAL_PORT. Add a serial_debuf_pci_init(int bdf) helper function to read the iobase from pci config space and set that variable.
That helper can then be hooked into pci scan, or called with a hardcoded bdf for early debug output, or both.
Thank you for your suggestion. So where should the argument "bdf" come from for the helper? serial_debug_pci_init(int bdf) In the case of hooking into pci scan, get the bdf by filtering the first pci serial device(class code)? In the case of called with hardcoded bdf for early debug output, we still need a config option to hard code the "bdf", right?
cheers, Gerd
Hi,
Failing that: I'd suggest to create a variable for the serial port iobase, defaulting to CONFIG_DEBUG_SERIAL_PORT. Add a serial_debuf_pci_init(int bdf) helper function to read the iobase from pci config space and set that variable.
That helper can then be hooked into pci scan, or called with a hardcoded bdf for early debug output, or both.
Thank you for your suggestion. So where should the argument "bdf" come from for the helper? serial_debug_pci_init(int bdf) In the case of hooking into pci scan, get the bdf by filtering the first pci serial device(class code)? In the case of called with hardcoded bdf for early debug output, we still need a config option to hard code the "bdf", right?
Yes. The simplest way is probably to make serial_debug_pci_init() work only once, i.e. if the variable is already set to something != CONFIG_DEBUG_SERIAL_PORT just return without doing anything. That way the hardcoded bdf will be used if available, otherwise the first device (in pci scan order) will win. cheers, Gerd
participants (3)
-
Gerd Hoffmann -
Paul Menzel -
Your Real Name