[PATCH v3 0/3] serial: Support a disabled serial port
This series provides a way to tell a serial UART that it can't actually work, at runtime. The main motivation is to deal with a coreboot feature where it does not provide UART details in the sysinfo structure unless the UART is also enabled in coreboot.
Attempts to introduce a way to enable a silent UART in coreboot have lead to a large amount of discussion but no result, sadly.
This series reworks a patch sent last year, which was not quite ready to be applied. A coreboot user (on irc) hit this problem, so it needs to be resolved.
Link: https://patchwork.ozlabs.org/project/uboot/patch/20231101180447.99361-1-sjg@...
Changes in v3: - Put the feature behind a Kconfig - Move the feature to the serial uclass, so any serial driver can use it
Changes in v2: - Drop RFC tag since there were no comments
Simon Glass (3): serial: Allow a serial port to be silent disabled serial: ns16550: Avoid probing hardware when disabled x86: coreboot: Make use of disabled console
arch/x86/cpu/coreboot/Kconfig | 1 + drivers/serial/Kconfig | 9 +++++++++ drivers/serial/ns16550.c | 3 ++- drivers/serial/serial-uclass.c | 19 ++++++++++++++++++- drivers/serial/serial_coreboot.c | 2 ++ include/serial.h | 25 +++++++++++++++++++++++++ 6 files changed, 57 insertions(+), 2 deletions(-)
Provide a way to disable a serial port at runtime if it is known that it will not work.
This is useful when booting from coreboot, if it does not provide info about the serial port in its sysinfo tables. U-Boot then hangs since if does not have the UART harware-address needed to write to the UART.
The feature is optional, since it increases code-size slightly.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
drivers/serial/Kconfig | 9 +++++++++ drivers/serial/serial-uclass.c | 19 ++++++++++++++++++- include/serial.h | 25 +++++++++++++++++++++++++ 3 files changed, 52 insertions(+), 1 deletion(-)
diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig index 8b27ad9a77e..d6541b289a0 100644 --- a/drivers/serial/Kconfig +++ b/drivers/serial/Kconfig @@ -99,6 +99,15 @@ config VPL_SERIAL_PRESENT This option enables the full UART in TPL, so if is it disabled, the full UART driver will be omitted, thus saving space.
+config SERIAL_CAN_DISABLE + bool "Serial ports can be silently disabled" + help + Enable this to allow serial ports to be disabled at runtime. This is + used when there is a UART driver available, but it cannot operate, + perhaps because it does not have the required hardware information. + An example is when coreboot boots into U-Boot but does not provide the + serial information in its sysinfo tables + config CONS_INDEX int "UART used for console" depends on SPECIFY_CONSOLE_INDEX diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c index a08678dde4e..7f1914e21ed 100644 --- a/drivers/serial/serial-uclass.c +++ b/drivers/serial/serial-uclass.c @@ -242,11 +242,20 @@ static void _serial_flush(struct udevice *dev) ; }
+bool serial_is_disabled_(struct udevice *dev) +{ + struct serial_dev_plat *uplat = dev_get_uclass_plat(dev); + + return uplat->disable; +} + static void _serial_putc(struct udevice *dev, char ch) { struct dm_serial_ops *ops = serial_get_ops(dev); int err;
+ if (serial_is_disabled(dev)) + return; if (ch == '\n') _serial_putc(dev, '\r');
@@ -262,6 +271,8 @@ static int __serial_puts(struct udevice *dev, const char *str, size_t len) { struct dm_serial_ops *ops = serial_get_ops(dev);
+ if (serial_is_disabled(dev)) + return 0; do { ssize_t written = ops->puts(dev, str, len);
@@ -306,6 +317,8 @@ static int __serial_getc(struct udevice *dev) struct dm_serial_ops *ops = serial_get_ops(dev); int err;
+ if (serial_is_disabled(dev)) + return 0; do { err = ops->getc(dev); if (err == -EAGAIN) @@ -319,6 +332,9 @@ static int __serial_tstc(struct udevice *dev) { struct dm_serial_ops *ops = serial_get_ops(dev);
+ if (serial_is_disabled(dev)) + return 0; + if (ops->pending) return ops->pending(dev, true);
@@ -568,7 +584,7 @@ static int serial_post_probe(struct udevice *dev) int ret;
/* Set the baud rate */ - if (ops->setbrg) { + if (!serial_is_disabled(dev) && ops->setbrg) { ret = ops->setbrg(dev, gd->baudrate); if (ret) return ret; @@ -611,6 +627,7 @@ UCLASS_DRIVER(serial) = { .flags = DM_UC_FLAG_SEQ_ALIAS, .post_probe = serial_post_probe, .pre_remove = serial_pre_remove, + .per_device_plat_auto = sizeof(struct serial_dev_plat), .per_device_auto = sizeof(struct serial_dev_priv), }; #endif diff --git a/include/serial.h b/include/serial.h index e5f6d984d28..f62dc6e9ba4 100644 --- a/include/serial.h +++ b/include/serial.h @@ -287,6 +287,19 @@ struct dm_serial_ops { int (*getinfo)(struct udevice *dev, struct serial_device_info *info); };
+/** + * struct serial_dev_plat - plat data used by the uclass + * + * @disable: true to disable probing and using this device. This is used when + * there is a UART driver available, but it cannot operate, perhaps because it + * does not have the required hardware information. An example is when coreboot + * boots into U-Boot but does not provide the serial information in its sysinfo + * tables + */ +struct serial_dev_plat { + bool disable; +}; + /** * struct serial_dev_priv - information about a device used by the uclass * @@ -309,6 +322,18 @@ struct serial_dev_priv { /* Access the serial operations for a device */ #define serial_get_ops(dev) ((struct dm_serial_ops *)(dev)->driver->ops)
+bool serial_is_disabled_(struct udevice *dev); + +/* Provide a way to silently disable the port */ +static inline bool serial_is_disabled(struct udevice *dev) +{ +#ifdef CONFIG_SERIAL_CAN_DISABLE + return serial_is_disabled_(dev); +#else + return false; +#endif +} + /** * serial_getconfig() - Get the uart configuration * (parity, 5/6/7/8 bits word length, stop bits)
When a serial port is marked as disabled, don't try to probe it, since it won't work and will probably hang.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
drivers/serial/ns16550.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c index 3f6860f3916..6576be4b58e 100644 --- a/drivers/serial/ns16550.c +++ b/drivers/serial/ns16550.c @@ -533,7 +533,8 @@ int ns16550_serial_probe(struct udevice *dev) reset_deassert_bulk(&reset_bulk);
com_port->plat = dev_get_plat(dev); - ns16550_init(com_port, -1); + if (!serial_is_disabled(dev)) + ns16550_init(com_port, -1);
return 0; }
U-Boot normally requires a UART. When booting from coreboot it is sometimes just not available, e.g. when no sysinfo or DBG2 information is provided.
In this case we need to continue running, since the display can be used. Use the 'disable' flag for this case.
This allows U-Boot to start up and operation from the display, instead of hanging on start-up.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v3: - Put the feature behind a Kconfig - Move the feature to the serial uclass, so any serial driver can use it
Changes in v2: - Drop RFC tag since there were no comments
arch/x86/cpu/coreboot/Kconfig | 1 + drivers/serial/serial_coreboot.c | 2 ++ 2 files changed, 3 insertions(+)
diff --git a/arch/x86/cpu/coreboot/Kconfig b/arch/x86/cpu/coreboot/Kconfig index 085302c0482..d850be9dc73 100644 --- a/arch/x86/cpu/coreboot/Kconfig +++ b/arch/x86/cpu/coreboot/Kconfig @@ -29,5 +29,6 @@ config SYS_COREBOOT select BINMAN if X86_64 select SYSINFO imply SYSINFO_EXTRA + select SERIAL_CAN_DISABLE
endif diff --git a/drivers/serial/serial_coreboot.c b/drivers/serial/serial_coreboot.c index b1f69f6998c..a53c37a8756 100644 --- a/drivers/serial/serial_coreboot.c +++ b/drivers/serial/serial_coreboot.c @@ -95,6 +95,7 @@ static int read_dbg2(struct ns16550_plat *plat)
static int coreboot_of_to_plat(struct udevice *dev) { + struct serial_dev_plat *uplat = dev_get_uclass_plat(dev); struct ns16550_plat *plat = dev_get_plat(dev); struct cb_serial *cb_info = lib_sysinfo.serial; int ret = -ENOENT; @@ -119,6 +120,7 @@ static int coreboot_of_to_plat(struct udevice *dev) * there is no UART, which may panic. So stay silent and * pray that the video console will work. */ + uplat->disable = true; log_debug("Cannot detect UART\n"); }
Hi Simon,
On 20/11/2024 16:35, Simon Glass wrote:
This series provides a way to tell a serial UART that it can't actually work, at runtime. The main motivation is to deal with a coreboot feature where it does not provide UART details in the sysinfo structure unless the UART is also enabled in coreboot.
Why is the UART driver probed if coreboot doesn't provide the necessary info for it?
Couldn't you disable CONFIG_REQUIRE_SERIAL_CONSOLE and skip probing it?
Kind regards,
Attempts to introduce a way to enable a silent UART in coreboot have lead to a large amount of discussion but no result, sadly.
This series reworks a patch sent last year, which was not quite ready to be applied. A coreboot user (on irc) hit this problem, so it needs to be resolved.
Link: https://patchwork.ozlabs.org/project/uboot/patch/20231101180447.99361-1-sjg@...
Changes in v3:
- Put the feature behind a Kconfig
- Move the feature to the serial uclass, so any serial driver can use it
Changes in v2:
- Drop RFC tag since there were no comments
Simon Glass (3): serial: Allow a serial port to be silent disabled serial: ns16550: Avoid probing hardware when disabled x86: coreboot: Make use of disabled console
arch/x86/cpu/coreboot/Kconfig | 1 + drivers/serial/Kconfig | 9 +++++++++ drivers/serial/ns16550.c | 3 ++- drivers/serial/serial-uclass.c | 19 ++++++++++++++++++- drivers/serial/serial_coreboot.c | 2 ++ include/serial.h | 25 +++++++++++++++++++++++++ 6 files changed, 57 insertions(+), 2 deletions(-)
Hi Caleb,
On Wed, 20 Nov 2024 at 08:56, Caleb Connolly caleb.connolly@linaro.org wrote:
Hi Simon,
On 20/11/2024 16:35, Simon Glass wrote:
This series provides a way to tell a serial UART that it can't actually work, at runtime. The main motivation is to deal with a coreboot feature where it does not provide UART details in the sysinfo structure unless the UART is also enabled in coreboot.
Why is the UART driver probed if coreboot doesn't provide the necessary info for it?
U-Boot doesn't do well without some sort of serial device. That could perhaps be improved. But the point here is that we need a single U-Boot build which can start from coreboot, regardless of what coreboot decides to do.
Couldn't you disable CONFIG_REQUIRE_SERIAL_CONSOLE and skip probing it?
No, because I want it to work if the serial info is available.
[..]
Regards, SImon
On 20/11/2024 17:03, Simon Glass wrote:
Hi Caleb,
On Wed, 20 Nov 2024 at 08:56, Caleb Connolly caleb.connolly@linaro.org wrote:
Hi Simon,
On 20/11/2024 16:35, Simon Glass wrote:
This series provides a way to tell a serial UART that it can't actually work, at runtime. The main motivation is to deal with a coreboot feature where it does not provide UART details in the sysinfo structure unless the UART is also enabled in coreboot.
Why is the UART driver probed if coreboot doesn't provide the necessary info for it?
U-Boot doesn't do well without some sort of serial device. That could perhaps be improved. But the point here is that we need a single U-Boot build which can start from coreboot, regardless of what coreboot decides to do.
Right, I'm suggesting that at runtime you notice that coreboot didn't provide a serial port definition (or gave you a bogus address or something) and skip probing the serial port in that case.
Couldn't you disable CONFIG_REQUIRE_SERIAL_CONSOLE and skip probing it?
No, because I want it to work if the serial info is available.
This option doesn't disable the serial port, it just makes U-Boot not panic if it can't find one. For example I enable this when building U-Boot for the upstream supported Qualcomm phones since many don't enable the serial port in their DTS.
[..]
Regards, SImon
On Wed, Nov 20, 2024 at 05:37:35PM +0100, Caleb Connolly wrote:
On 20/11/2024 17:03, Simon Glass wrote:
Hi Caleb,
On Wed, 20 Nov 2024 at 08:56, Caleb Connolly caleb.connolly@linaro.org wrote:
Hi Simon,
On 20/11/2024 16:35, Simon Glass wrote:
This series provides a way to tell a serial UART that it can't actually work, at runtime. The main motivation is to deal with a coreboot feature where it does not provide UART details in the sysinfo structure unless the UART is also enabled in coreboot.
Why is the UART driver probed if coreboot doesn't provide the necessary info for it?
U-Boot doesn't do well without some sort of serial device. That could perhaps be improved. But the point here is that we need a single U-Boot build which can start from coreboot, regardless of what coreboot decides to do.
Right, I'm suggesting that at runtime you notice that coreboot didn't provide a serial port definition (or gave you a bogus address or something) and skip probing the serial port in that case.
Couldn't you disable CONFIG_REQUIRE_SERIAL_CONSOLE and skip probing it?
No, because I want it to work if the serial info is available.
This option doesn't disable the serial port, it just makes U-Boot not panic if it can't find one. For example I enable this when building U-Boot for the upstream supported Qualcomm phones since many don't enable the serial port in their DTS.
Yes, this feels like an oddly coreboot specific solution to a generic problem that we should be able to handle already I thought? Like a Pi can have or not have a serial port enabled and still have video and we use that. We might well need something specific to know the port is disabled since I gather we aren't getting a devicetree that says status = disabled in it. But this ought to be a common enough general case?
participants (3)
-
Caleb Connolly -
Simon Glass -
Tom Rini