We want to enable runtime PM for serial port device drivers in a generic
way. To do this, we want to have the serial core layer manage the
registered physical serial controller devices.
To manage serial controllers, let's set up a struct bus and struct device
for the serial core controller as suggested by Greg and Jiri. The serial
core controller devices are children of the physical serial port device.
The serial core controller device is needed to support multiple different
kind of ports connected to single physical serial port device.
Let's also set up a struct device for the serial core port. The serial
core port instances are children of the serial core controller device.
With the serial core port device we can now flush pending TX on the
runtime PM resume as suggested by Johan.
Suggested-by: Andy Shevchenko <[email protected]>
Suggested-by: Greg Kroah-Hartman <[email protected]>
Suggested-by: Jiri Slaby <[email protected]>
Suggested-by: Johan Hovold <[email protected]>
Signed-off-by: Tony Lindgren <[email protected]>
---
Changes since v11:
- Reworked code to add struct serial_ctrl_device and serial_port_device
wrappers around struct device for type checking as suggested by Greg
Changes since v10:
- Added missing handling for unknown type in serial_base_device_add()
as noted by kernel test robot
- Use y instead of objs fro serial_base in Makefile as noted by Andy
- Improve serial_port pm_ops alignment as noted by Andy
Changes since v9:
- Built in serial_base and core related components into serial_base.ko as
suggested by Greg. We now have module_init only in serial_base.c that
calls serial_base_ctrl_init() and serial_base_port_init(). I renamed
serial_bus.c to serial_base_bus.c to build serial_base.ko. Note that
if we wanted to build these into serial_core.ko, renaming serial_core.c
would be needed which is not necessarily nice for folks that may have
pending patches
- Dropped string comparison for ctrl and port, and switched to using
struct device_type as suggested by Greg
- Dropped port->state checks in serial_base_get_port() as noted by
Greg
- Dropped EXPORT_SYMBOL_NS(), these are no longer needed with components
built into serial_base.ko. I also noticed that we have some dependency
loops if components are not built into serial_base.ko. And we would
have hard time later on moving port specific functions to serial_port.c
for example
- Dropped checks for negative ctrl_id in serial_core_ctrl_find() as
suggested by Greg
- Stopped resetting ctrl_id in serial_core_remove_one_port(), instead
let's properly init it in serial8250_init_port(). The ctrl_id is
optionally passed to uart_add_one_port() and zero otherwise
- Moved port_mutex and UPF_DEAD handling from serial_core_add_one_port()
to serial_core_register_port() to simplify things a bit
- Updated license and copyright as suggested by Greg
- Dropped Andy's reviewed-by, things still changed quite a bit
Changes since v8:
- Drop unnecessary free for name noticed by Andy, the name is freed
on put_device()
- Cosmetic fix for comments in serial_port.c noted by Andy
- Spelling fix for word uninitialized in serial_base_get_port()
Changes since v7:
- Add release() put_device() to serial_base.c as noted by Andy
- Make struct serial_base_device private to serial_base.c by adding
serial_base_get_port()
- Add more comments to __uart_start()
- Coding style improvments for serial_base.c from Andy
Changes since v6:
- Fix up a memory leak and a bunch of issues in serial_base.c as noted
by Andy
- Replace bool with new_ctrl_dev for freeing the added device on
error path
- Drop unused pm ops for serial_ctrl.c as noted by kernel test robot
- Drop documentation updates for acpi devices for now to avoid a merge
conflict and make testing easier between -rc2 and Linux next
Changes since v5:
- Replace platform bus and device with bus_add() and device_add(),
Greg did not like platform bus and device here. This also gets
rid of the need for platform data with struct serial_base_device,
see new file serial_base.c
- Update documentation to drop reference to struct uart_device as
suggested by Andy
Changes since v4:
- Fix issue noted by Ilpo by calling serial_core_add_one_port() after
the devices are created
Changes since v3:
- Simplify things by adding a serial core control device as the child of
the physical serial port as suggested by Jiri
- Drop the tinkering of the physical serial port device for runtime PM.
Serial core just needs to manage port->port_dev with the addition of
the serial core control device and the device hierarchy will keep the
pysical serial port device enabled as needed
- Simplify patch description with all the runtime PM tinkering gone
- Coding style improvments as noted by Andy
- Post as a single RFC patch as we're close to the merge window
Changes since v2:
- Make each serial port a proper device as suggested by Greg. This is
a separate patch that flushes the TX on runtime PM resume
Changes since v1:
- Use kref as suggested by Andy
- Fix memory leak on error as noted by Andy
- Use use unsigned char for supports_autosuspend as suggested by Andy
- Coding style improvments as suggested by Andy
---
drivers/tty/serial/8250/8250_core.c | 1 +
drivers/tty/serial/8250/8250_port.c | 1 +
drivers/tty/serial/Makefile | 3 +-
drivers/tty/serial/serial_base.h | 46 ++++++
drivers/tty/serial/serial_base_bus.c | 200 +++++++++++++++++++++++++++
drivers/tty/serial/serial_core.c | 192 ++++++++++++++++++++++---
drivers/tty/serial/serial_ctrl.c | 68 +++++++++
drivers/tty/serial/serial_port.c | 105 ++++++++++++++
include/linux/serial_core.h | 5 +-
9 files changed, 598 insertions(+), 23 deletions(-)
create mode 100644 drivers/tty/serial/serial_base.h
create mode 100644 drivers/tty/serial/serial_base_bus.c
create mode 100644 drivers/tty/serial/serial_ctrl.c
create mode 100644 drivers/tty/serial/serial_port.c
diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -1039,6 +1039,7 @@ int serial8250_register_8250_port(const struct uart_8250_port *up)
if (uart->port.dev)
uart_remove_one_port(&serial8250_reg, &uart->port);
+ uart->port.ctrl_id = up->port.ctrl_id;
uart->port.iobase = up->port.iobase;
uart->port.membase = up->port.membase;
uart->port.irq = up->port.irq;
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -3214,6 +3214,7 @@ void serial8250_init_port(struct uart_8250_port *up)
struct uart_port *port = &up->port;
spin_lock_init(&port->lock);
+ port->ctrl_id = 0;
port->ops = &serial8250_pops;
port->has_sysrq = IS_ENABLED(CONFIG_SERIAL_8250_CONSOLE);
diff --git a/drivers/tty/serial/Makefile b/drivers/tty/serial/Makefile
--- a/drivers/tty/serial/Makefile
+++ b/drivers/tty/serial/Makefile
@@ -3,7 +3,8 @@
# Makefile for the kernel serial device drivers.
#
-obj-$(CONFIG_SERIAL_CORE) += serial_core.o
+obj-$(CONFIG_SERIAL_CORE) += serial_base.o
+serial_base-y := serial_core.o serial_base_bus.o serial_ctrl.o serial_port.o
obj-$(CONFIG_SERIAL_EARLYCON) += earlycon.o
obj-$(CONFIG_SERIAL_EARLYCON_SEMIHOST) += earlycon-semihost.o
diff --git a/drivers/tty/serial/serial_base.h b/drivers/tty/serial/serial_base.h
new file mode 100644
--- /dev/null
+++ b/drivers/tty/serial/serial_base.h
@@ -0,0 +1,46 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Serial core related functions, serial port device drivers do not need this.
+ *
+ * Copyright (C) 2023 Texas Instruments Incorporated - https://www.ti.com/
+ * Author: Tony Lindgren <[email protected]>
+ */
+
+#define to_serial_base_ctrl_device(d) container_of((d), struct serial_ctrl_device, dev)
+#define to_serial_base_port_device(d) container_of((d), struct serial_port_device, dev)
+
+struct uart_driver;
+struct uart_port;
+struct device_driver;
+struct device;
+
+struct serial_ctrl_device {
+ struct device dev;
+};
+
+struct serial_port_device {
+ struct device dev;
+ struct uart_port *port;
+};
+
+int serial_base_ctrl_init(void);
+void serial_base_ctrl_exit(void);
+
+int serial_base_port_init(void);
+void serial_base_port_exit(void);
+
+int serial_base_driver_register(struct device_driver *driver);
+void serial_base_driver_unregister(struct device_driver *driver);
+
+struct serial_ctrl_device *serial_base_ctrl_add(struct uart_port *port,
+ struct device *parent);
+struct serial_port_device *serial_base_port_add(struct uart_port *port,
+ struct serial_ctrl_device *parent);
+void serial_base_ctrl_device_remove(struct serial_ctrl_device *ctrl_dev);
+void serial_base_port_device_remove(struct serial_port_device *port_dev);
+
+int serial_ctrl_register_port(struct uart_driver *drv, struct uart_port *port);
+void serial_ctrl_unregister_port(struct uart_driver *drv, struct uart_port *port);
+
+int serial_core_register_port(struct uart_driver *drv, struct uart_port *port);
+void serial_core_unregister_port(struct uart_driver *drv, struct uart_port *port);
diff --git a/drivers/tty/serial/serial_base_bus.c b/drivers/tty/serial/serial_base_bus.c
new file mode 100644
--- /dev/null
+++ b/drivers/tty/serial/serial_base_bus.c
@@ -0,0 +1,200 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Serial base bus layer for controllers
+ *
+ * Copyright (C) 2023 Texas Instruments Incorporated - https://www.ti.com/
+ * Author: Tony Lindgren <[email protected]>
+ *
+ * The serial core bus manages the serial core controller instances.
+ */
+
+#include <linux/container_of.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/serial_core.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+
+#include "serial_base.h"
+
+static int serial_base_match(struct device *dev, struct device_driver *drv)
+{
+ int len = strlen(drv->name);
+
+ return !strncmp(dev_name(dev), drv->name, len);
+}
+
+static struct bus_type serial_base_bus_type = {
+ .name = "serial-base",
+ .match = serial_base_match,
+};
+
+int serial_base_driver_register(struct device_driver *driver)
+{
+ driver->bus = &serial_base_bus_type;
+
+ return driver_register(driver);
+}
+
+void serial_base_driver_unregister(struct device_driver *driver)
+{
+ driver_unregister(driver);
+}
+
+static int serial_base_device_init(struct uart_port *port,
+ struct device *dev,
+ struct device *parent_dev,
+ const struct device_type *type,
+ void (*release)(struct device *dev),
+ int id)
+{
+ device_initialize(dev);
+ dev->type = type;
+ dev->parent = parent_dev;
+ dev->bus = &serial_base_bus_type;
+ dev->release = release;
+
+ return dev_set_name(dev, "%s.%s.%d", type->name, dev_name(port->dev), id);
+}
+
+static const struct device_type serial_ctrl_type = {
+ .name = "ctrl",
+};
+
+static void serial_base_ctrl_release(struct device *dev)
+{
+ struct serial_ctrl_device *ctrl_dev = to_serial_base_ctrl_device(dev);
+
+ kfree(ctrl_dev);
+}
+
+void serial_base_ctrl_device_remove(struct serial_ctrl_device *ctrl_dev)
+{
+ if (!ctrl_dev)
+ return;
+
+ device_del(&ctrl_dev->dev);
+}
+
+struct serial_ctrl_device *serial_base_ctrl_add(struct uart_port *port,
+ struct device *parent)
+{
+ struct serial_ctrl_device *ctrl_dev;
+ int err;
+
+ ctrl_dev = kzalloc(sizeof(*ctrl_dev), GFP_KERNEL);
+ if (!ctrl_dev)
+ return ERR_PTR(-ENOMEM);
+
+ err = serial_base_device_init(port, &ctrl_dev->dev,
+ parent, &serial_ctrl_type,
+ serial_base_ctrl_release,
+ port->ctrl_id);
+ if (err)
+ goto err_free_ctrl_dev;
+
+ err = device_add(&ctrl_dev->dev);
+ if (err)
+ goto err_put_device;
+
+ return ctrl_dev;
+
+err_put_device:
+ put_device(&ctrl_dev->dev);
+err_free_ctrl_dev:
+ kfree(ctrl_dev);
+
+ return ERR_PTR(err);
+}
+
+static const struct device_type serial_port_type = {
+ .name = "port",
+};
+
+static void serial_base_port_release(struct device *dev)
+{
+ struct serial_port_device *port_dev = to_serial_base_port_device(dev);
+
+ kfree(port_dev);
+}
+
+struct serial_port_device *serial_base_port_add(struct uart_port *port,
+ struct serial_ctrl_device *ctrl_dev)
+{
+ struct serial_port_device *port_dev;
+ int err;
+
+ port_dev = kzalloc(sizeof(*port_dev), GFP_KERNEL);
+ if (!port_dev)
+ return ERR_PTR(-ENOMEM);
+
+ err = serial_base_device_init(port, &port_dev->dev,
+ &ctrl_dev->dev, &serial_port_type,
+ serial_base_port_release,
+ port->line);
+ if (err)
+ goto err_free_port_dev;
+
+ port_dev->port = port;
+
+ err = device_add(&port_dev->dev);
+ if (err)
+ goto err_put_device;
+
+ return port_dev;
+
+err_put_device:
+ put_device(&port_dev->dev);
+err_free_port_dev:
+ kfree(port_dev);
+
+ return ERR_PTR(err);
+}
+
+void serial_base_port_device_remove(struct serial_port_device *port_dev)
+{
+ if (!port_dev)
+ return;
+
+ device_del(&port_dev->dev);
+}
+
+static int serial_base_init(void)
+{
+ int ret;
+
+ ret = bus_register(&serial_base_bus_type);
+ if (ret)
+ return ret;
+
+ ret = serial_base_ctrl_init();
+ if (ret)
+ goto err_bus_unregister;
+
+ ret = serial_base_port_init();
+ if (ret)
+ goto err_ctrl_exit;
+
+ return 0;
+
+err_ctrl_exit:
+ serial_base_ctrl_exit();
+
+err_bus_unregister:
+ bus_unregister(&serial_base_bus_type);
+
+ return ret;
+}
+module_init(serial_base_init);
+
+static void serial_base_exit(void)
+{
+ serial_base_port_exit();
+ serial_base_ctrl_exit();
+ bus_unregister(&serial_base_bus_type);
+}
+module_exit(serial_base_exit);
+
+MODULE_AUTHOR("Tony Lindgren <[email protected]>");
+MODULE_DESCRIPTION("Serial core bus");
+MODULE_LICENSE("GPL");
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -17,6 +17,7 @@
#include <linux/gpio/consumer.h>
#include <linux/kernel.h>
#include <linux/of.h>
+#include <linux/pm_runtime.h>
#include <linux/proc_fs.h>
#include <linux/seq_file.h>
#include <linux/device.h>
@@ -31,6 +32,8 @@
#include <linux/irq.h>
#include <linux/uaccess.h>
+#include "serial_base.h"
+
/*
* This is used to lock changes in serial line configuration.
*/
@@ -134,9 +137,30 @@ static void __uart_start(struct tty_struct *tty)
{
struct uart_state *state = tty->driver_data;
struct uart_port *port = state->uart_port;
+ struct serial_port_device *port_dev;
+ int err;
- if (port && !(port->flags & UPF_DEAD) && !uart_tx_stopped(port))
+ if (!port || port->flags & UPF_DEAD || uart_tx_stopped(port))
+ return;
+
+ port_dev = port->port_dev;
+
+ /* Increment the runtime PM usage count for the active check below */
+ err = pm_runtime_get(&port_dev->dev);
+ if (err < 0) {
+ pm_runtime_put_noidle(&port_dev->dev);
+ return;
+ }
+
+ /*
+ * Start TX if enabled, and kick runtime PM. If the device is not
+ * enabled, serial_port_runtime_resume() calls start_tx() again
+ * after enabling the device.
+ */
+ if (pm_runtime_active(&port_dev->dev))
port->ops->start_tx(port);
+ pm_runtime_mark_last_busy(&port_dev->dev);
+ pm_runtime_put_autosuspend(&port_dev->dev);
}
static void uart_start(struct tty_struct *tty)
@@ -3042,7 +3066,7 @@ static const struct attribute_group tty_dev_attr_group = {
};
/**
- * uart_add_one_port - attach a driver-defined port structure
+ * serial_core_add_one_port - attach a driver-defined port structure
* @drv: pointer to the uart low level driver structure for this port
* @uport: uart port structure to use for this port.
*
@@ -3051,8 +3075,9 @@ static const struct attribute_group tty_dev_attr_group = {
* This allows the driver @drv to register its own uart_port structure with the
* core driver. The main purpose is to allow the low level uart drivers to
* expand uart_port, rather than having yet more levels of structures.
+ * Caller must hold port_mutex.
*/
-int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
+static int serial_core_add_one_port(struct uart_driver *drv, struct uart_port *uport)
{
struct uart_state *state;
struct tty_port *port;
@@ -3066,7 +3091,6 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
state = drv->state + uport->line;
port = &state->port;
- mutex_lock(&port_mutex);
mutex_lock(&port->mutex);
if (state->uart_port) {
ret = -EINVAL;
@@ -3131,21 +3155,14 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
uport->line);
}
- /*
- * Ensure UPF_DEAD is not set.
- */
- uport->flags &= ~UPF_DEAD;
-
out:
mutex_unlock(&port->mutex);
- mutex_unlock(&port_mutex);
return ret;
}
-EXPORT_SYMBOL(uart_add_one_port);
/**
- * uart_remove_one_port - detach a driver defined port structure
+ * serial_core_remove_one_port - detach a driver defined port structure
* @drv: pointer to the uart low level driver structure for this port
* @uport: uart port structure for this port
*
@@ -3153,20 +3170,16 @@ EXPORT_SYMBOL(uart_add_one_port);
*
* This unhooks (and hangs up) the specified port structure from the core
* driver. No further calls will be made to the low-level code for this port.
+ * Caller must hold port_mutex.
*/
-void uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
+static void serial_core_remove_one_port(struct uart_driver *drv,
+ struct uart_port *uport)
{
struct uart_state *state = drv->state + uport->line;
struct tty_port *port = &state->port;
struct uart_port *uart_port;
struct tty_struct *tty;
- mutex_lock(&port_mutex);
-
- /*
- * Mark the port "dead" - this prevents any opens from
- * succeeding while we shut down the port.
- */
mutex_lock(&port->mutex);
uart_port = uart_port_check(state);
if (uart_port != uport)
@@ -3177,7 +3190,6 @@ void uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
mutex_unlock(&port->mutex);
goto out;
}
- uport->flags |= UPF_DEAD;
mutex_unlock(&port->mutex);
/*
@@ -3209,6 +3221,7 @@ void uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
* Indicate that there isn't a port here anymore.
*/
uport->type = PORT_UNKNOWN;
+ uport->port_dev = NULL;
mutex_lock(&port->mutex);
WARN_ON(atomic_dec_return(&state->refcount) < 0);
@@ -3218,7 +3231,6 @@ void uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
out:
mutex_unlock(&port_mutex);
}
-EXPORT_SYMBOL(uart_remove_one_port);
/**
* uart_match_port - are the two ports equivalent?
@@ -3253,6 +3265,144 @@ bool uart_match_port(const struct uart_port *port1,
}
EXPORT_SYMBOL(uart_match_port);
+static struct serial_ctrl_device *
+serial_core_get_ctrl_dev(struct serial_port_device *port_dev)
+{
+ struct device *dev = &port_dev->dev;
+
+ return to_serial_base_ctrl_device(dev->parent);
+}
+
+/*
+ * Find a registered serial core controller device if one exists. Returns
+ * the first device matching the ctrl_id. Caller must hold port_mutex.
+ */
+static struct serial_ctrl_device *serial_core_ctrl_find(struct uart_driver *drv,
+ struct device *phys_dev,
+ int ctrl_id)
+{
+ struct uart_state *state;
+ int i;
+
+ lockdep_assert_held(&port_mutex);
+
+ for (i = 0; i < drv->nr; i++) {
+ state = drv->state + i;
+ if (!state->uart_port || !state->uart_port->port_dev)
+ continue;
+
+ if (state->uart_port->dev == phys_dev &&
+ state->uart_port->ctrl_id == ctrl_id)
+ return serial_core_get_ctrl_dev(state->uart_port->port_dev);
+ }
+
+ return NULL;
+}
+
+static struct serial_ctrl_device *serial_core_ctrl_device_add(struct uart_port *port)
+{
+ return serial_base_ctrl_add(port, port->dev);
+}
+
+static int serial_core_port_device_add(struct serial_ctrl_device *ctrl_dev,
+ struct uart_port *port)
+{
+ struct serial_port_device *port_dev;
+
+ port_dev = serial_base_port_add(port, ctrl_dev);
+ if (IS_ERR(port_dev))
+ return PTR_ERR(port_dev);
+
+ port->port_dev = port_dev;
+
+ return 0;
+}
+
+/*
+ * Initialize a serial core port device, and a controller device if needed.
+ */
+int serial_core_register_port(struct uart_driver *drv, struct uart_port *port)
+{
+ struct serial_ctrl_device *ctrl_dev, *new_ctrl_dev = NULL;
+ int ret;
+
+ mutex_lock(&port_mutex);
+
+ /*
+ * Prevent serial_port_runtime_resume() from trying to use the port
+ * until serial_core_add_one_port() has completed
+ */
+ port->flags |= UPF_DEAD;
+
+ /* Inititalize a serial core controller device if needed */
+ ctrl_dev = serial_core_ctrl_find(drv, port->dev, port->ctrl_id);
+ if (!ctrl_dev) {
+ new_ctrl_dev = serial_core_ctrl_device_add(port);
+ if (!new_ctrl_dev) {
+ ret = -ENODEV;
+ goto err_unlock;
+ }
+ ctrl_dev = new_ctrl_dev;
+ }
+
+ /*
+ * Initialize a serial core port device. Tag the port dead to prevent
+ * serial_port_runtime_resume() trying to do anything until port has
+ * been registered. It gets cleared by serial_core_add_one_port().
+ */
+ ret = serial_core_port_device_add(ctrl_dev, port);
+ if (ret)
+ goto err_unregister_ctrl_dev;
+
+ ret = serial_core_add_one_port(drv, port);
+ if (ret)
+ goto err_unregister_port_dev;
+
+ port->flags &= ~UPF_DEAD;
+
+ mutex_unlock(&port_mutex);
+
+ return 0;
+
+err_unregister_port_dev:
+ serial_base_port_device_remove(port->port_dev);
+
+err_unregister_ctrl_dev:
+ serial_base_ctrl_device_remove(new_ctrl_dev);
+
+err_unlock:
+ mutex_unlock(&port_mutex);
+
+ return ret;
+}
+
+/*
+ * Removes a serial core port device, and the related serial core controller
+ * device if the last instance.
+ */
+void serial_core_unregister_port(struct uart_driver *drv, struct uart_port *port)
+{
+ struct device *phys_dev = port->dev;
+ struct serial_port_device *port_dev = port->port_dev;
+ struct serial_ctrl_device *ctrl_dev = serial_core_get_ctrl_dev(port_dev);
+ int ctrl_id = port->ctrl_id;
+
+ mutex_lock(&port_mutex);
+
+ port->flags |= UPF_DEAD;
+
+ serial_core_remove_one_port(drv, port);
+
+ /* Note that struct uart_port *port is no longer valid at this point */
+ serial_base_port_device_remove(port_dev);
+
+ /* Drop the serial core controller device if no ports are using it */
+ if (!serial_core_ctrl_find(drv, phys_dev, ctrl_id))
+ serial_base_ctrl_device_remove(ctrl_dev);
+
+ mutex_unlock(&port_mutex);
+}
+
/**
* uart_handle_dcd_change - handle a change of carrier detect state
* @uport: uart_port structure for the open port
diff --git a/drivers/tty/serial/serial_ctrl.c b/drivers/tty/serial/serial_ctrl.c
new file mode 100644
--- /dev/null
+++ b/drivers/tty/serial/serial_ctrl.c
@@ -0,0 +1,68 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Serial core controller driver
+ *
+ * Copyright (C) 2023 Texas Instruments Incorporated - https://www.ti.com/
+ * Author: Tony Lindgren <[email protected]>
+ *
+ * This driver manages the serial core controller struct device instances.
+ * The serial core controller devices are children of the physical serial
+ * port device.
+ */
+
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/pm_runtime.h>
+#include <linux/serial_core.h>
+#include <linux/spinlock.h>
+
+#include "serial_base.h"
+
+static int serial_ctrl_probe(struct device *dev)
+{
+ pm_runtime_enable(dev);
+
+ return 0;
+}
+
+static int serial_ctrl_remove(struct device *dev)
+{
+ pm_runtime_disable(dev);
+
+ return 0;
+}
+
+/*
+ * Serial core controller device init functions. Note that the physical
+ * serial port device driver may not have completed probe at this point.
+ */
+int serial_ctrl_register_port(struct uart_driver *drv, struct uart_port *port)
+{
+ return serial_core_register_port(drv, port);
+}
+
+void serial_ctrl_unregister_port(struct uart_driver *drv, struct uart_port *port)
+{
+ serial_core_unregister_port(drv, port);
+}
+
+static struct device_driver serial_ctrl_driver = {
+ .name = "ctrl",
+ .suppress_bind_attrs = true,
+ .probe = serial_ctrl_probe,
+ .remove = serial_ctrl_remove,
+};
+
+int serial_base_ctrl_init(void)
+{
+ return serial_base_driver_register(&serial_ctrl_driver);
+}
+
+void serial_base_ctrl_exit(void)
+{
+ serial_base_driver_unregister(&serial_ctrl_driver);
+}
+
+MODULE_AUTHOR("Tony Lindgren <[email protected]>");
+MODULE_DESCRIPTION("Serial core controller driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/tty/serial/serial_port.c b/drivers/tty/serial/serial_port.c
new file mode 100644
--- /dev/null
+++ b/drivers/tty/serial/serial_port.c
@@ -0,0 +1,105 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Serial core port device driver
+ *
+ * Copyright (C) 2023 Texas Instruments Incorporated - https://www.ti.com/
+ * Author: Tony Lindgren <[email protected]>
+ */
+
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/pm_runtime.h>
+#include <linux/serial_core.h>
+#include <linux/spinlock.h>
+
+#include "serial_base.h"
+
+#define SERIAL_PORT_AUTOSUSPEND_DELAY_MS 500
+
+/* Only considers pending TX for now. Caller must take care of locking */
+static int __serial_port_busy(struct uart_port *port)
+{
+ return !uart_tx_stopped(port) &&
+ uart_circ_chars_pending(&port->state->xmit);
+}
+
+static int serial_port_runtime_resume(struct device *dev)
+{
+ struct serial_port_device *port_dev = to_serial_base_port_device(dev);
+ struct uart_port *port;
+ unsigned long flags;
+
+ port = port_dev->port;
+
+ if (port->flags & UPF_DEAD)
+ goto out;
+
+ /* Flush any pending TX for the port */
+ spin_lock_irqsave(&port->lock, flags);
+ if (__serial_port_busy(port))
+ port->ops->start_tx(port);
+ spin_unlock_irqrestore(&port->lock, flags);
+
+out:
+ pm_runtime_mark_last_busy(dev);
+
+ return 0;
+}
+
+static DEFINE_RUNTIME_DEV_PM_OPS(serial_port_pm,
+ NULL, serial_port_runtime_resume, NULL);
+
+static int serial_port_probe(struct device *dev)
+{
+ pm_runtime_enable(dev);
+ pm_runtime_set_autosuspend_delay(dev, SERIAL_PORT_AUTOSUSPEND_DELAY_MS);
+ pm_runtime_use_autosuspend(dev);
+
+ return 0;
+}
+
+static int serial_port_remove(struct device *dev)
+{
+ pm_runtime_dont_use_autosuspend(dev);
+ pm_runtime_disable(dev);
+
+ return 0;
+}
+
+/*
+ * Serial core port device init functions. Note that the physical serial
+ * port device driver may not have completed probe at this point.
+ */
+int uart_add_one_port(struct uart_driver *drv, struct uart_port *port)
+{
+ return serial_ctrl_register_port(drv, port);
+}
+EXPORT_SYMBOL(uart_add_one_port);
+
+void uart_remove_one_port(struct uart_driver *drv, struct uart_port *port)
+{
+ serial_ctrl_unregister_port(drv, port);
+}
+EXPORT_SYMBOL(uart_remove_one_port);
+
+static struct device_driver serial_port_driver = {
+ .name = "port",
+ .suppress_bind_attrs = true,
+ .probe = serial_port_probe,
+ .remove = serial_port_remove,
+ .pm = pm_ptr(&serial_port_pm),
+};
+
+int serial_base_port_init(void)
+{
+ return serial_base_driver_register(&serial_port_driver);
+}
+
+void serial_base_port_exit(void)
+{
+ serial_base_driver_unregister(&serial_port_driver);
+}
+
+MODULE_AUTHOR("Tony Lindgren <[email protected]>");
+MODULE_DESCRIPTION("Serial controller port driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -28,6 +28,7 @@
struct uart_port;
struct serial_struct;
+struct serial_port_device;
struct device;
struct gpio_desc;
@@ -458,6 +459,7 @@ struct uart_port {
struct serial_rs485 *rs485);
int (*iso7816_config)(struct uart_port *,
struct serial_iso7816 *iso7816);
+ int ctrl_id; /* optional serial core controller id */
unsigned int irq; /* irq number */
unsigned long irqflags; /* irq flags */
unsigned int uartclk; /* base uart clock */
@@ -563,7 +565,8 @@ struct uart_port {
unsigned int minor;
resource_size_t mapbase; /* for ioremap */
resource_size_t mapsize;
- struct device *dev; /* parent device */
+ struct device *dev; /* serial port physical parent device */
+ struct serial_port_device *port_dev; /* serial core port device */
unsigned long sysrq; /* sysrq timeout */
unsigned int sysrq_ch; /* char for sysrq */
--
2.40.1
On Thu, May 25, 2023 at 02:30:30PM +0300, Tony Lindgren wrote:
> We want to enable runtime PM for serial port device drivers in a generic
> way. To do this, we want to have the serial core layer manage the
> registered physical serial controller devices.
>
> To manage serial controllers, let's set up a struct bus and struct device
> for the serial core controller as suggested by Greg and Jiri. The serial
> core controller devices are children of the physical serial port device.
> The serial core controller device is needed to support multiple different
> kind of ports connected to single physical serial port device.
>
> Let's also set up a struct device for the serial core port. The serial
> core port instances are children of the serial core controller device.
>
> With the serial core port device we can now flush pending TX on the
> runtime PM resume as suggested by Johan.
Cool development, thank you!
Reviewed-by: Andy Shevchenko <[email protected]>
> Suggested-by: Andy Shevchenko <[email protected]>
> Suggested-by: Greg Kroah-Hartman <[email protected]>
> Suggested-by: Jiri Slaby <[email protected]>
> Suggested-by: Johan Hovold <[email protected]>
> Signed-off-by: Tony Lindgren <[email protected]>
> ---
> Changes since v11:
> - Reworked code to add struct serial_ctrl_device and serial_port_device
> wrappers around struct device for type checking as suggested by Greg
>
> Changes since v10:
>
> - Added missing handling for unknown type in serial_base_device_add()
> as noted by kernel test robot
>
> - Use y instead of objs fro serial_base in Makefile as noted by Andy
>
> - Improve serial_port pm_ops alignment as noted by Andy
>
> Changes since v9:
>
> - Built in serial_base and core related components into serial_base.ko as
> suggested by Greg. We now have module_init only in serial_base.c that
> calls serial_base_ctrl_init() and serial_base_port_init(). I renamed
> serial_bus.c to serial_base_bus.c to build serial_base.ko. Note that
> if we wanted to build these into serial_core.ko, renaming serial_core.c
> would be needed which is not necessarily nice for folks that may have
> pending patches
>
> - Dropped string comparison for ctrl and port, and switched to using
> struct device_type as suggested by Greg
>
> - Dropped port->state checks in serial_base_get_port() as noted by
> Greg
>
> - Dropped EXPORT_SYMBOL_NS(), these are no longer needed with components
> built into serial_base.ko. I also noticed that we have some dependency
> loops if components are not built into serial_base.ko. And we would
> have hard time later on moving port specific functions to serial_port.c
> for example
>
> - Dropped checks for negative ctrl_id in serial_core_ctrl_find() as
> suggested by Greg
>
> - Stopped resetting ctrl_id in serial_core_remove_one_port(), instead
> let's properly init it in serial8250_init_port(). The ctrl_id is
> optionally passed to uart_add_one_port() and zero otherwise
>
> - Moved port_mutex and UPF_DEAD handling from serial_core_add_one_port()
> to serial_core_register_port() to simplify things a bit
>
> - Updated license and copyright as suggested by Greg
>
> - Dropped Andy's reviewed-by, things still changed quite a bit
>
> Changes since v8:
>
> - Drop unnecessary free for name noticed by Andy, the name is freed
> on put_device()
>
> - Cosmetic fix for comments in serial_port.c noted by Andy
>
> - Spelling fix for word uninitialized in serial_base_get_port()
>
> Changes since v7:
>
> - Add release() put_device() to serial_base.c as noted by Andy
>
> - Make struct serial_base_device private to serial_base.c by adding
> serial_base_get_port()
>
> - Add more comments to __uart_start()
>
> - Coding style improvments for serial_base.c from Andy
>
> Changes since v6:
>
> - Fix up a memory leak and a bunch of issues in serial_base.c as noted
> by Andy
>
> - Replace bool with new_ctrl_dev for freeing the added device on
> error path
>
> - Drop unused pm ops for serial_ctrl.c as noted by kernel test robot
>
> - Drop documentation updates for acpi devices for now to avoid a merge
> conflict and make testing easier between -rc2 and Linux next
>
> Changes since v5:
>
> - Replace platform bus and device with bus_add() and device_add(),
> Greg did not like platform bus and device here. This also gets
> rid of the need for platform data with struct serial_base_device,
> see new file serial_base.c
>
> - Update documentation to drop reference to struct uart_device as
> suggested by Andy
>
> Changes since v4:
>
> - Fix issue noted by Ilpo by calling serial_core_add_one_port() after
> the devices are created
>
> Changes since v3:
>
> - Simplify things by adding a serial core control device as the child of
> the physical serial port as suggested by Jiri
>
> - Drop the tinkering of the physical serial port device for runtime PM.
> Serial core just needs to manage port->port_dev with the addition of
> the serial core control device and the device hierarchy will keep the
> pysical serial port device enabled as needed
>
> - Simplify patch description with all the runtime PM tinkering gone
>
> - Coding style improvments as noted by Andy
>
> - Post as a single RFC patch as we're close to the merge window
>
> Changes since v2:
>
> - Make each serial port a proper device as suggested by Greg. This is
> a separate patch that flushes the TX on runtime PM resume
>
> Changes since v1:
>
> - Use kref as suggested by Andy
>
> - Fix memory leak on error as noted by Andy
>
> - Use use unsigned char for supports_autosuspend as suggested by Andy
>
> - Coding style improvments as suggested by Andy
> ---
> drivers/tty/serial/8250/8250_core.c | 1 +
> drivers/tty/serial/8250/8250_port.c | 1 +
> drivers/tty/serial/Makefile | 3 +-
> drivers/tty/serial/serial_base.h | 46 ++++++
> drivers/tty/serial/serial_base_bus.c | 200 +++++++++++++++++++++++++++
> drivers/tty/serial/serial_core.c | 192 ++++++++++++++++++++++---
> drivers/tty/serial/serial_ctrl.c | 68 +++++++++
> drivers/tty/serial/serial_port.c | 105 ++++++++++++++
> include/linux/serial_core.h | 5 +-
> 9 files changed, 598 insertions(+), 23 deletions(-)
> create mode 100644 drivers/tty/serial/serial_base.h
> create mode 100644 drivers/tty/serial/serial_base_bus.c
> create mode 100644 drivers/tty/serial/serial_ctrl.c
> create mode 100644 drivers/tty/serial/serial_port.c
>
> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -1039,6 +1039,7 @@ int serial8250_register_8250_port(const struct uart_8250_port *up)
> if (uart->port.dev)
> uart_remove_one_port(&serial8250_reg, &uart->port);
>
> + uart->port.ctrl_id = up->port.ctrl_id;
> uart->port.iobase = up->port.iobase;
> uart->port.membase = up->port.membase;
> uart->port.irq = up->port.irq;
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -3214,6 +3214,7 @@ void serial8250_init_port(struct uart_8250_port *up)
> struct uart_port *port = &up->port;
>
> spin_lock_init(&port->lock);
> + port->ctrl_id = 0;
> port->ops = &serial8250_pops;
> port->has_sysrq = IS_ENABLED(CONFIG_SERIAL_8250_CONSOLE);
>
> diff --git a/drivers/tty/serial/Makefile b/drivers/tty/serial/Makefile
> --- a/drivers/tty/serial/Makefile
> +++ b/drivers/tty/serial/Makefile
> @@ -3,7 +3,8 @@
> # Makefile for the kernel serial device drivers.
> #
>
> -obj-$(CONFIG_SERIAL_CORE) += serial_core.o
> +obj-$(CONFIG_SERIAL_CORE) += serial_base.o
> +serial_base-y := serial_core.o serial_base_bus.o serial_ctrl.o serial_port.o
>
> obj-$(CONFIG_SERIAL_EARLYCON) += earlycon.o
> obj-$(CONFIG_SERIAL_EARLYCON_SEMIHOST) += earlycon-semihost.o
> diff --git a/drivers/tty/serial/serial_base.h b/drivers/tty/serial/serial_base.h
> new file mode 100644
> --- /dev/null
> +++ b/drivers/tty/serial/serial_base.h
> @@ -0,0 +1,46 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Serial core related functions, serial port device drivers do not need this.
> + *
> + * Copyright (C) 2023 Texas Instruments Incorporated - https://www.ti.com/
> + * Author: Tony Lindgren <[email protected]>
> + */
> +
> +#define to_serial_base_ctrl_device(d) container_of((d), struct serial_ctrl_device, dev)
> +#define to_serial_base_port_device(d) container_of((d), struct serial_port_device, dev)
> +
> +struct uart_driver;
> +struct uart_port;
> +struct device_driver;
> +struct device;
> +
> +struct serial_ctrl_device {
> + struct device dev;
> +};
> +
> +struct serial_port_device {
> + struct device dev;
> + struct uart_port *port;
> +};
> +
> +int serial_base_ctrl_init(void);
> +void serial_base_ctrl_exit(void);
> +
> +int serial_base_port_init(void);
> +void serial_base_port_exit(void);
> +
> +int serial_base_driver_register(struct device_driver *driver);
> +void serial_base_driver_unregister(struct device_driver *driver);
> +
> +struct serial_ctrl_device *serial_base_ctrl_add(struct uart_port *port,
> + struct device *parent);
> +struct serial_port_device *serial_base_port_add(struct uart_port *port,
> + struct serial_ctrl_device *parent);
> +void serial_base_ctrl_device_remove(struct serial_ctrl_device *ctrl_dev);
> +void serial_base_port_device_remove(struct serial_port_device *port_dev);
> +
> +int serial_ctrl_register_port(struct uart_driver *drv, struct uart_port *port);
> +void serial_ctrl_unregister_port(struct uart_driver *drv, struct uart_port *port);
> +
> +int serial_core_register_port(struct uart_driver *drv, struct uart_port *port);
> +void serial_core_unregister_port(struct uart_driver *drv, struct uart_port *port);
> diff --git a/drivers/tty/serial/serial_base_bus.c b/drivers/tty/serial/serial_base_bus.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/tty/serial/serial_base_bus.c
> @@ -0,0 +1,200 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Serial base bus layer for controllers
> + *
> + * Copyright (C) 2023 Texas Instruments Incorporated - https://www.ti.com/
> + * Author: Tony Lindgren <[email protected]>
> + *
> + * The serial core bus manages the serial core controller instances.
> + */
> +
> +#include <linux/container_of.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/serial_core.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +
> +#include "serial_base.h"
> +
> +static int serial_base_match(struct device *dev, struct device_driver *drv)
> +{
> + int len = strlen(drv->name);
> +
> + return !strncmp(dev_name(dev), drv->name, len);
> +}
> +
> +static struct bus_type serial_base_bus_type = {
> + .name = "serial-base",
> + .match = serial_base_match,
> +};
> +
> +int serial_base_driver_register(struct device_driver *driver)
> +{
> + driver->bus = &serial_base_bus_type;
> +
> + return driver_register(driver);
> +}
> +
> +void serial_base_driver_unregister(struct device_driver *driver)
> +{
> + driver_unregister(driver);
> +}
> +
> +static int serial_base_device_init(struct uart_port *port,
> + struct device *dev,
> + struct device *parent_dev,
> + const struct device_type *type,
> + void (*release)(struct device *dev),
> + int id)
> +{
> + device_initialize(dev);
> + dev->type = type;
> + dev->parent = parent_dev;
> + dev->bus = &serial_base_bus_type;
> + dev->release = release;
> +
> + return dev_set_name(dev, "%s.%s.%d", type->name, dev_name(port->dev), id);
> +}
> +
> +static const struct device_type serial_ctrl_type = {
> + .name = "ctrl",
> +};
> +
> +static void serial_base_ctrl_release(struct device *dev)
> +{
> + struct serial_ctrl_device *ctrl_dev = to_serial_base_ctrl_device(dev);
> +
> + kfree(ctrl_dev);
> +}
> +
> +void serial_base_ctrl_device_remove(struct serial_ctrl_device *ctrl_dev)
> +{
> + if (!ctrl_dev)
> + return;
> +
> + device_del(&ctrl_dev->dev);
> +}
> +
> +struct serial_ctrl_device *serial_base_ctrl_add(struct uart_port *port,
> + struct device *parent)
> +{
> + struct serial_ctrl_device *ctrl_dev;
> + int err;
> +
> + ctrl_dev = kzalloc(sizeof(*ctrl_dev), GFP_KERNEL);
> + if (!ctrl_dev)
> + return ERR_PTR(-ENOMEM);
> +
> + err = serial_base_device_init(port, &ctrl_dev->dev,
> + parent, &serial_ctrl_type,
> + serial_base_ctrl_release,
> + port->ctrl_id);
> + if (err)
> + goto err_free_ctrl_dev;
> +
> + err = device_add(&ctrl_dev->dev);
> + if (err)
> + goto err_put_device;
> +
> + return ctrl_dev;
> +
> +err_put_device:
> + put_device(&ctrl_dev->dev);
> +err_free_ctrl_dev:
> + kfree(ctrl_dev);
> +
> + return ERR_PTR(err);
> +}
> +
> +static const struct device_type serial_port_type = {
> + .name = "port",
> +};
> +
> +static void serial_base_port_release(struct device *dev)
> +{
> + struct serial_port_device *port_dev = to_serial_base_port_device(dev);
> +
> + kfree(port_dev);
> +}
> +
> +struct serial_port_device *serial_base_port_add(struct uart_port *port,
> + struct serial_ctrl_device *ctrl_dev)
> +{
> + struct serial_port_device *port_dev;
> + int err;
> +
> + port_dev = kzalloc(sizeof(*port_dev), GFP_KERNEL);
> + if (!port_dev)
> + return ERR_PTR(-ENOMEM);
> +
> + err = serial_base_device_init(port, &port_dev->dev,
> + &ctrl_dev->dev, &serial_port_type,
> + serial_base_port_release,
> + port->line);
> + if (err)
> + goto err_free_port_dev;
> +
> + port_dev->port = port;
> +
> + err = device_add(&port_dev->dev);
> + if (err)
> + goto err_put_device;
> +
> + return port_dev;
> +
> +err_put_device:
> + put_device(&port_dev->dev);
> +err_free_port_dev:
> + kfree(port_dev);
> +
> + return ERR_PTR(err);
> +}
> +
> +void serial_base_port_device_remove(struct serial_port_device *port_dev)
> +{
> + if (!port_dev)
> + return;
> +
> + device_del(&port_dev->dev);
> +}
> +
> +static int serial_base_init(void)
> +{
> + int ret;
> +
> + ret = bus_register(&serial_base_bus_type);
> + if (ret)
> + return ret;
> +
> + ret = serial_base_ctrl_init();
> + if (ret)
> + goto err_bus_unregister;
> +
> + ret = serial_base_port_init();
> + if (ret)
> + goto err_ctrl_exit;
> +
> + return 0;
> +
> +err_ctrl_exit:
> + serial_base_ctrl_exit();
> +
> +err_bus_unregister:
> + bus_unregister(&serial_base_bus_type);
> +
> + return ret;
> +}
> +module_init(serial_base_init);
> +
> +static void serial_base_exit(void)
> +{
> + serial_base_port_exit();
> + serial_base_ctrl_exit();
> + bus_unregister(&serial_base_bus_type);
> +}
> +module_exit(serial_base_exit);
> +
> +MODULE_AUTHOR("Tony Lindgren <[email protected]>");
> +MODULE_DESCRIPTION("Serial core bus");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -17,6 +17,7 @@
> #include <linux/gpio/consumer.h>
> #include <linux/kernel.h>
> #include <linux/of.h>
> +#include <linux/pm_runtime.h>
> #include <linux/proc_fs.h>
> #include <linux/seq_file.h>
> #include <linux/device.h>
> @@ -31,6 +32,8 @@
> #include <linux/irq.h>
> #include <linux/uaccess.h>
>
> +#include "serial_base.h"
> +
> /*
> * This is used to lock changes in serial line configuration.
> */
> @@ -134,9 +137,30 @@ static void __uart_start(struct tty_struct *tty)
> {
> struct uart_state *state = tty->driver_data;
> struct uart_port *port = state->uart_port;
> + struct serial_port_device *port_dev;
> + int err;
>
> - if (port && !(port->flags & UPF_DEAD) && !uart_tx_stopped(port))
> + if (!port || port->flags & UPF_DEAD || uart_tx_stopped(port))
> + return;
> +
> + port_dev = port->port_dev;
> +
> + /* Increment the runtime PM usage count for the active check below */
> + err = pm_runtime_get(&port_dev->dev);
> + if (err < 0) {
> + pm_runtime_put_noidle(&port_dev->dev);
> + return;
> + }
> +
> + /*
> + * Start TX if enabled, and kick runtime PM. If the device is not
> + * enabled, serial_port_runtime_resume() calls start_tx() again
> + * after enabling the device.
> + */
> + if (pm_runtime_active(&port_dev->dev))
> port->ops->start_tx(port);
> + pm_runtime_mark_last_busy(&port_dev->dev);
> + pm_runtime_put_autosuspend(&port_dev->dev);
> }
>
> static void uart_start(struct tty_struct *tty)
> @@ -3042,7 +3066,7 @@ static const struct attribute_group tty_dev_attr_group = {
> };
>
> /**
> - * uart_add_one_port - attach a driver-defined port structure
> + * serial_core_add_one_port - attach a driver-defined port structure
> * @drv: pointer to the uart low level driver structure for this port
> * @uport: uart port structure to use for this port.
> *
> @@ -3051,8 +3075,9 @@ static const struct attribute_group tty_dev_attr_group = {
> * This allows the driver @drv to register its own uart_port structure with the
> * core driver. The main purpose is to allow the low level uart drivers to
> * expand uart_port, rather than having yet more levels of structures.
> + * Caller must hold port_mutex.
> */
> -int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
> +static int serial_core_add_one_port(struct uart_driver *drv, struct uart_port *uport)
> {
> struct uart_state *state;
> struct tty_port *port;
> @@ -3066,7 +3091,6 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
> state = drv->state + uport->line;
> port = &state->port;
>
> - mutex_lock(&port_mutex);
> mutex_lock(&port->mutex);
> if (state->uart_port) {
> ret = -EINVAL;
> @@ -3131,21 +3155,14 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
> uport->line);
> }
>
> - /*
> - * Ensure UPF_DEAD is not set.
> - */
> - uport->flags &= ~UPF_DEAD;
> -
> out:
> mutex_unlock(&port->mutex);
> - mutex_unlock(&port_mutex);
>
> return ret;
> }
> -EXPORT_SYMBOL(uart_add_one_port);
>
> /**
> - * uart_remove_one_port - detach a driver defined port structure
> + * serial_core_remove_one_port - detach a driver defined port structure
> * @drv: pointer to the uart low level driver structure for this port
> * @uport: uart port structure for this port
> *
> @@ -3153,20 +3170,16 @@ EXPORT_SYMBOL(uart_add_one_port);
> *
> * This unhooks (and hangs up) the specified port structure from the core
> * driver. No further calls will be made to the low-level code for this port.
> + * Caller must hold port_mutex.
> */
> -void uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
> +static void serial_core_remove_one_port(struct uart_driver *drv,
> + struct uart_port *uport)
> {
> struct uart_state *state = drv->state + uport->line;
> struct tty_port *port = &state->port;
> struct uart_port *uart_port;
> struct tty_struct *tty;
>
> - mutex_lock(&port_mutex);
> -
> - /*
> - * Mark the port "dead" - this prevents any opens from
> - * succeeding while we shut down the port.
> - */
> mutex_lock(&port->mutex);
> uart_port = uart_port_check(state);
> if (uart_port != uport)
> @@ -3177,7 +3190,6 @@ void uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
> mutex_unlock(&port->mutex);
> goto out;
> }
> - uport->flags |= UPF_DEAD;
> mutex_unlock(&port->mutex);
>
> /*
> @@ -3209,6 +3221,7 @@ void uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
> * Indicate that there isn't a port here anymore.
> */
> uport->type = PORT_UNKNOWN;
> + uport->port_dev = NULL;
>
> mutex_lock(&port->mutex);
> WARN_ON(atomic_dec_return(&state->refcount) < 0);
> @@ -3218,7 +3231,6 @@ void uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
> out:
> mutex_unlock(&port_mutex);
> }
> -EXPORT_SYMBOL(uart_remove_one_port);
>
> /**
> * uart_match_port - are the two ports equivalent?
> @@ -3253,6 +3265,144 @@ bool uart_match_port(const struct uart_port *port1,
> }
> EXPORT_SYMBOL(uart_match_port);
>
> +static struct serial_ctrl_device *
> +serial_core_get_ctrl_dev(struct serial_port_device *port_dev)
> +{
> + struct device *dev = &port_dev->dev;
> +
> + return to_serial_base_ctrl_device(dev->parent);
> +}
> +
> +/*
> + * Find a registered serial core controller device if one exists. Returns
> + * the first device matching the ctrl_id. Caller must hold port_mutex.
> + */
> +static struct serial_ctrl_device *serial_core_ctrl_find(struct uart_driver *drv,
> + struct device *phys_dev,
> + int ctrl_id)
> +{
> + struct uart_state *state;
> + int i;
> +
> + lockdep_assert_held(&port_mutex);
> +
> + for (i = 0; i < drv->nr; i++) {
> + state = drv->state + i;
> + if (!state->uart_port || !state->uart_port->port_dev)
> + continue;
> +
> + if (state->uart_port->dev == phys_dev &&
> + state->uart_port->ctrl_id == ctrl_id)
> + return serial_core_get_ctrl_dev(state->uart_port->port_dev);
> + }
> +
> + return NULL;
> +}
> +
> +static struct serial_ctrl_device *serial_core_ctrl_device_add(struct uart_port *port)
> +{
> + return serial_base_ctrl_add(port, port->dev);
> +}
> +
> +static int serial_core_port_device_add(struct serial_ctrl_device *ctrl_dev,
> + struct uart_port *port)
> +{
> + struct serial_port_device *port_dev;
> +
> + port_dev = serial_base_port_add(port, ctrl_dev);
> + if (IS_ERR(port_dev))
> + return PTR_ERR(port_dev);
> +
> + port->port_dev = port_dev;
> +
> + return 0;
> +}
> +
> +/*
> + * Initialize a serial core port device, and a controller device if needed.
> + */
> +int serial_core_register_port(struct uart_driver *drv, struct uart_port *port)
> +{
> + struct serial_ctrl_device *ctrl_dev, *new_ctrl_dev = NULL;
> + int ret;
> +
> + mutex_lock(&port_mutex);
> +
> + /*
> + * Prevent serial_port_runtime_resume() from trying to use the port
> + * until serial_core_add_one_port() has completed
> + */
> + port->flags |= UPF_DEAD;
> +
> + /* Inititalize a serial core controller device if needed */
> + ctrl_dev = serial_core_ctrl_find(drv, port->dev, port->ctrl_id);
> + if (!ctrl_dev) {
> + new_ctrl_dev = serial_core_ctrl_device_add(port);
> + if (!new_ctrl_dev) {
> + ret = -ENODEV;
> + goto err_unlock;
> + }
> + ctrl_dev = new_ctrl_dev;
> + }
> +
> + /*
> + * Initialize a serial core port device. Tag the port dead to prevent
> + * serial_port_runtime_resume() trying to do anything until port has
> + * been registered. It gets cleared by serial_core_add_one_port().
> + */
> + ret = serial_core_port_device_add(ctrl_dev, port);
> + if (ret)
> + goto err_unregister_ctrl_dev;
> +
> + ret = serial_core_add_one_port(drv, port);
> + if (ret)
> + goto err_unregister_port_dev;
> +
> + port->flags &= ~UPF_DEAD;
> +
> + mutex_unlock(&port_mutex);
> +
> + return 0;
> +
> +err_unregister_port_dev:
> + serial_base_port_device_remove(port->port_dev);
> +
> +err_unregister_ctrl_dev:
> + serial_base_ctrl_device_remove(new_ctrl_dev);
> +
> +err_unlock:
> + mutex_unlock(&port_mutex);
> +
> + return ret;
> +}
> +
> +/*
> + * Removes a serial core port device, and the related serial core controller
> + * device if the last instance.
> + */
> +void serial_core_unregister_port(struct uart_driver *drv, struct uart_port *port)
> +{
> + struct device *phys_dev = port->dev;
> + struct serial_port_device *port_dev = port->port_dev;
> + struct serial_ctrl_device *ctrl_dev = serial_core_get_ctrl_dev(port_dev);
> + int ctrl_id = port->ctrl_id;
> +
> + mutex_lock(&port_mutex);
> +
> + port->flags |= UPF_DEAD;
> +
> + serial_core_remove_one_port(drv, port);
> +
> + /* Note that struct uart_port *port is no longer valid at this point */
> + serial_base_port_device_remove(port_dev);
> +
> + /* Drop the serial core controller device if no ports are using it */
> + if (!serial_core_ctrl_find(drv, phys_dev, ctrl_id))
> + serial_base_ctrl_device_remove(ctrl_dev);
> +
> + mutex_unlock(&port_mutex);
> +}
> +
> /**
> * uart_handle_dcd_change - handle a change of carrier detect state
> * @uport: uart_port structure for the open port
> diff --git a/drivers/tty/serial/serial_ctrl.c b/drivers/tty/serial/serial_ctrl.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/tty/serial/serial_ctrl.c
> @@ -0,0 +1,68 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Serial core controller driver
> + *
> + * Copyright (C) 2023 Texas Instruments Incorporated - https://www.ti.com/
> + * Author: Tony Lindgren <[email protected]>
> + *
> + * This driver manages the serial core controller struct device instances.
> + * The serial core controller devices are children of the physical serial
> + * port device.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/serial_core.h>
> +#include <linux/spinlock.h>
> +
> +#include "serial_base.h"
> +
> +static int serial_ctrl_probe(struct device *dev)
> +{
> + pm_runtime_enable(dev);
> +
> + return 0;
> +}
> +
> +static int serial_ctrl_remove(struct device *dev)
> +{
> + pm_runtime_disable(dev);
> +
> + return 0;
> +}
> +
> +/*
> + * Serial core controller device init functions. Note that the physical
> + * serial port device driver may not have completed probe at this point.
> + */
> +int serial_ctrl_register_port(struct uart_driver *drv, struct uart_port *port)
> +{
> + return serial_core_register_port(drv, port);
> +}
> +
> +void serial_ctrl_unregister_port(struct uart_driver *drv, struct uart_port *port)
> +{
> + serial_core_unregister_port(drv, port);
> +}
> +
> +static struct device_driver serial_ctrl_driver = {
> + .name = "ctrl",
> + .suppress_bind_attrs = true,
> + .probe = serial_ctrl_probe,
> + .remove = serial_ctrl_remove,
> +};
> +
> +int serial_base_ctrl_init(void)
> +{
> + return serial_base_driver_register(&serial_ctrl_driver);
> +}
> +
> +void serial_base_ctrl_exit(void)
> +{
> + serial_base_driver_unregister(&serial_ctrl_driver);
> +}
> +
> +MODULE_AUTHOR("Tony Lindgren <[email protected]>");
> +MODULE_DESCRIPTION("Serial core controller driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/tty/serial/serial_port.c b/drivers/tty/serial/serial_port.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/tty/serial/serial_port.c
> @@ -0,0 +1,105 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Serial core port device driver
> + *
> + * Copyright (C) 2023 Texas Instruments Incorporated - https://www.ti.com/
> + * Author: Tony Lindgren <[email protected]>
> + */
> +
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/serial_core.h>
> +#include <linux/spinlock.h>
> +
> +#include "serial_base.h"
> +
> +#define SERIAL_PORT_AUTOSUSPEND_DELAY_MS 500
> +
> +/* Only considers pending TX for now. Caller must take care of locking */
> +static int __serial_port_busy(struct uart_port *port)
> +{
> + return !uart_tx_stopped(port) &&
> + uart_circ_chars_pending(&port->state->xmit);
> +}
> +
> +static int serial_port_runtime_resume(struct device *dev)
> +{
> + struct serial_port_device *port_dev = to_serial_base_port_device(dev);
> + struct uart_port *port;
> + unsigned long flags;
> +
> + port = port_dev->port;
> +
> + if (port->flags & UPF_DEAD)
> + goto out;
> +
> + /* Flush any pending TX for the port */
> + spin_lock_irqsave(&port->lock, flags);
> + if (__serial_port_busy(port))
> + port->ops->start_tx(port);
> + spin_unlock_irqrestore(&port->lock, flags);
> +
> +out:
> + pm_runtime_mark_last_busy(dev);
> +
> + return 0;
> +}
> +
> +static DEFINE_RUNTIME_DEV_PM_OPS(serial_port_pm,
> + NULL, serial_port_runtime_resume, NULL);
> +
> +static int serial_port_probe(struct device *dev)
> +{
> + pm_runtime_enable(dev);
> + pm_runtime_set_autosuspend_delay(dev, SERIAL_PORT_AUTOSUSPEND_DELAY_MS);
> + pm_runtime_use_autosuspend(dev);
> +
> + return 0;
> +}
> +
> +static int serial_port_remove(struct device *dev)
> +{
> + pm_runtime_dont_use_autosuspend(dev);
> + pm_runtime_disable(dev);
> +
> + return 0;
> +}
> +
> +/*
> + * Serial core port device init functions. Note that the physical serial
> + * port device driver may not have completed probe at this point.
> + */
> +int uart_add_one_port(struct uart_driver *drv, struct uart_port *port)
> +{
> + return serial_ctrl_register_port(drv, port);
> +}
> +EXPORT_SYMBOL(uart_add_one_port);
> +
> +void uart_remove_one_port(struct uart_driver *drv, struct uart_port *port)
> +{
> + serial_ctrl_unregister_port(drv, port);
> +}
> +EXPORT_SYMBOL(uart_remove_one_port);
> +
> +static struct device_driver serial_port_driver = {
> + .name = "port",
> + .suppress_bind_attrs = true,
> + .probe = serial_port_probe,
> + .remove = serial_port_remove,
> + .pm = pm_ptr(&serial_port_pm),
> +};
> +
> +int serial_base_port_init(void)
> +{
> + return serial_base_driver_register(&serial_port_driver);
> +}
> +
> +void serial_base_port_exit(void)
> +{
> + serial_base_driver_unregister(&serial_port_driver);
> +}
> +
> +MODULE_AUTHOR("Tony Lindgren <[email protected]>");
> +MODULE_DESCRIPTION("Serial controller port driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -28,6 +28,7 @@
>
> struct uart_port;
> struct serial_struct;
> +struct serial_port_device;
> struct device;
> struct gpio_desc;
>
> @@ -458,6 +459,7 @@ struct uart_port {
> struct serial_rs485 *rs485);
> int (*iso7816_config)(struct uart_port *,
> struct serial_iso7816 *iso7816);
> + int ctrl_id; /* optional serial core controller id */
> unsigned int irq; /* irq number */
> unsigned long irqflags; /* irq flags */
> unsigned int uartclk; /* base uart clock */
> @@ -563,7 +565,8 @@ struct uart_port {
> unsigned int minor;
> resource_size_t mapbase; /* for ioremap */
> resource_size_t mapsize;
> - struct device *dev; /* parent device */
> + struct device *dev; /* serial port physical parent device */
> + struct serial_port_device *port_dev; /* serial core port device */
>
> unsigned long sysrq; /* sysrq timeout */
> unsigned int sysrq_ch; /* char for sysrq */
> --
> 2.40.1
--
With Best Regards,
Andy Shevchenko
On Thu, May 25, 2023 at 02:30:30PM +0300, Tony Lindgren wrote:
> We want to enable runtime PM for serial port device drivers in a generic
> way. To do this, we want to have the serial core layer manage the
> registered physical serial controller devices.
>
> To manage serial controllers, let's set up a struct bus and struct device
> for the serial core controller as suggested by Greg and Jiri. The serial
> core controller devices are children of the physical serial port device.
> The serial core controller device is needed to support multiple different
> kind of ports connected to single physical serial port device.
>
> Let's also set up a struct device for the serial core port. The serial
> core port instances are children of the serial core controller device.
>
> With the serial core port device we can now flush pending TX on the
> runtime PM resume as suggested by Johan.
>
> Suggested-by: Andy Shevchenko <[email protected]>
> Suggested-by: Greg Kroah-Hartman <[email protected]>
> Suggested-by: Jiri Slaby <[email protected]>
> Suggested-by: Johan Hovold <[email protected]>
> Signed-off-by: Tony Lindgren <[email protected]>
> ---
Thanks for sticking with this, looks good now so I've queued it up in my
tree.
greg k-h
Hi,
This has arrived in linux-next and causes boot warnings, see below.
On 25/05/2023 12:30, Tony Lindgren wrote:
> We want to enable runtime PM for serial port device drivers in a generic
> way. To do this, we want to have the serial core layer manage the
> registered physical serial controller devices.
>
> To manage serial controllers, let's set up a struct bus and struct device
> for the serial core controller as suggested by Greg and Jiri. The serial
> core controller devices are children of the physical serial port device.
> The serial core controller device is needed to support multiple different
> kind of ports connected to single physical serial port device.
>
> Let's also set up a struct device for the serial core port. The serial
> core port instances are children of the serial core controller device.
>
> With the serial core port device we can now flush pending TX on the
> runtime PM resume as suggested by Johan.
>
> Suggested-by: Andy Shevchenko <[email protected]>
> Suggested-by: Greg Kroah-Hartman <[email protected]>
> Suggested-by: Jiri Slaby <[email protected]>
> Suggested-by: Johan Hovold <[email protected]>
> Signed-off-by: Tony Lindgren <[email protected]>
...
>
> /**
> - * uart_remove_one_port - detach a driver defined port structure
> + * serial_core_remove_one_port - detach a driver defined port structure
> * @drv: pointer to the uart low level driver structure for this port
> * @uport: uart port structure for this port
> *
> @@ -3153,20 +3170,16 @@ EXPORT_SYMBOL(uart_add_one_port);
> *
> * This unhooks (and hangs up) the specified port structure from the core
> * driver. No further calls will be made to the low-level code for this port.
> + * Caller must hold port_mutex.
> */
> -void uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
> +static void serial_core_remove_one_port(struct uart_driver *drv,
> + struct uart_port *uport)
> {
> struct uart_state *state = drv->state + uport->line;
> struct tty_port *port = &state->port;
> struct uart_port *uart_port;
> struct tty_struct *tty;
>
> - mutex_lock(&port_mutex);
serial_core_remove_one_port() no longer takes port_mutex (caller must
hold it)...
> -
> - /*
> - * Mark the port "dead" - this prevents any opens from
> - * succeeding while we shut down the port.
> - */
> mutex_lock(&port->mutex);
> uart_port = uart_port_check(state);
> if (uart_port != uport)
> @@ -3177,7 +3190,6 @@ void uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
> mutex_unlock(&port->mutex);
> goto out;
> }
> - uport->flags |= UPF_DEAD;
> mutex_unlock(&port->mutex);
>
> /*
> @@ -3209,6 +3221,7 @@ void uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
> * Indicate that there isn't a port here anymore.
> */
> uport->type = PORT_UNKNOWN;
> + uport->port_dev = NULL;
>
> mutex_lock(&port->mutex);
> WARN_ON(atomic_dec_return(&state->refcount) < 0);
> @@ -3218,7 +3231,6 @@ void uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
> out:
> mutex_unlock(&port_mutex);
... but it still drops it at the end of the function.
> }
> -EXPORT_SYMBOL(uart_remove_one_port);
...
> +/*
> + * Find a registered serial core controller device if one exists. Returns
> + * the first device matching the ctrl_id. Caller must hold port_mutex.
> + */
> +static struct serial_ctrl_device *serial_core_ctrl_find(struct uart_driver *drv,
> + struct device *phys_dev,
> + int ctrl_id)
> +{
> + struct uart_state *state;
> + int i;
> +
> + lockdep_assert_held(&port_mutex);
This function must be called with port_mutex held, but...
> +
> + for (i = 0; i < drv->nr; i++) {
> + state = drv->state + i;
> + if (!state->uart_port || !state->uart_port->port_dev)
> + continue;
> +
> + if (state->uart_port->dev == phys_dev &&
> + state->uart_port->ctrl_id == ctrl_id)
> + return serial_core_get_ctrl_dev(state->uart_port->port_dev);
> + }
> +
> + return NULL;
> +}
...
> +/*
> + * Removes a serial core port device, and the related serial core controller
> + * device if the last instance.
> + */
> +void serial_core_unregister_port(struct uart_driver *drv, struct uart_port *port)
> +{
> + struct device *phys_dev = port->dev;
> + struct serial_port_device *port_dev = port->port_dev;
> + struct serial_ctrl_device *ctrl_dev = serial_core_get_ctrl_dev(port_dev);
> + int ctrl_id = port->ctrl_id;
> +
> + mutex_lock(&port_mutex);
We take port_mutex here...
> +
> + port->flags |= UPF_DEAD;
> +
> + serial_core_remove_one_port(drv, port);
> +
> + /* Note that struct uart_port *port is no longer valid at this point */
> + serial_base_port_device_remove(port_dev);
serial_base_port_device_remove() then drops it...
> +
> + /* Drop the serial core controller device if no ports are using it */
> + if (!serial_core_ctrl_find(drv, phys_dev, ctrl_id))
serial_core_ctrl_find() complains that it's not held.
> + serial_base_ctrl_device_remove(ctrl_dev);
> +
> + mutex_unlock(&port_mutex);
And we attempt to unlock it when it's not held.
I haven't studied this change in detail, but I assume the bug is that
serial_base_port_device_remove() shouldn't be dropping port_mutex. The
below hack gets my board booting again.
Thanks,
Steve
Hack fix:
----8<----
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 29bd5ede0b25..044e4853341a 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -3234,8 +3234,7 @@ static void serial_core_remove_one_port(struct uart_driver *drv,
wait_event(state->remove_wait, !atomic_read(&state->refcount));
state->uart_port = NULL;
mutex_unlock(&port->mutex);
-out:
- mutex_unlock(&port_mutex);
+out:;
}
/**
Hi,
* Steven Price <[email protected]> [230601 10:04]:
> I haven't studied this change in detail, but I assume the bug is that
> serial_base_port_device_remove() shouldn't be dropping port_mutex. The
> below hack gets my board booting again.
You're right. I wonder how I managed to miss that.. Care to post a proper
fix for this or do you want me to post it?
> Thanks,
>
> Steve
>
> Hack fix:
> ----8<----
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 29bd5ede0b25..044e4853341a 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -3234,8 +3234,7 @@ static void serial_core_remove_one_port(struct uart_driver *drv,
> wait_event(state->remove_wait, !atomic_read(&state->refcount));
> state->uart_port = NULL;
> mutex_unlock(&port->mutex);
> -out:
> - mutex_unlock(&port_mutex);
> +out:;
> }
Seems you can remove out here and just do a return earlier instead of goto.
Regards,
Tony
* Steven Price <[email protected]> [230601 10:53]:
> On 01/06/2023 11:44, Tony Lindgren wrote:
> > Hi,
> >
> > * Steven Price <[email protected]> [230601 10:04]:
> >> I haven't studied this change in detail, but I assume the bug is that
> >> serial_base_port_device_remove() shouldn't be dropping port_mutex. The
> >> below hack gets my board booting again.
> >
> > You're right. I wonder how I managed to miss that.. Care to post a proper
> > fix for this or do you want me to post it?
>
> I'll post a proper fix shortly. Thanks for the confirmation of the fix.
OK great thanks!
> > Seems you can remove out here and just do a return earlier instead of goto.
>
> Yes, this was just the smallest change. I'll do it properly with an
> early return in the proper patch.
OK
Regards,
Tony
Hi Tony,
On 25.05.2023 13:30, Tony Lindgren wrote:
> We want to enable runtime PM for serial port device drivers in a generic
> way. To do this, we want to have the serial core layer manage the
> registered physical serial controller devices.
>
> To manage serial controllers, let's set up a struct bus and struct device
> for the serial core controller as suggested by Greg and Jiri. The serial
> core controller devices are children of the physical serial port device.
> The serial core controller device is needed to support multiple different
> kind of ports connected to single physical serial port device.
>
> Let's also set up a struct device for the serial core port. The serial
> core port instances are children of the serial core controller device.
>
> With the serial core port device we can now flush pending TX on the
> runtime PM resume as suggested by Johan.
>
> Suggested-by: Andy Shevchenko <[email protected]>
> Suggested-by: Greg Kroah-Hartman <[email protected]>
> Suggested-by: Jiri Slaby <[email protected]>
> Suggested-by: Johan Hovold <[email protected]>
> Signed-off-by: Tony Lindgren <[email protected]>
This patch landed in today's linux next-20230601 as commit 84a9582fd203
("serial: core: Start managing serial controllers to enable runtime
PM"). Unfortunately it breaks booting some of my test boards. This can
be easily reproduced with QEMU and ARM64 virt machine. The last message
I see in the log is:
[ 3.084743] Run /sbin/init as init process
I've tried a hack posted here by Steven Price, but unfortunately it
doesn't fix my issue. Reverting $subject on top of next-20230601 fixes
the boot.
Here is my qemu test command (nothing really special...):
qemu-system-aarch64 -kernel Image -append "console=ttyAMA0
no_console_suspend root=/dev/vda rootwait ip=::::target::off" -M virt
-cpu cortex-a57 -smp 2 -m 1024 -device
virtio-blk-device,drive=virtio-blk0 -device
virtio-blk-device,drive=virtio-blk1 -drive
file=qemu-virt-rootfs.raw,id=virtio-blk1,if=none,format=raw -drive
file=initrd,id=virtio-blk0,if=none,format=raw -netdev user,id=user
-device virtio-net-device,netdev=user -display none
> ...
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
On 01/06/2023 11:44, Tony Lindgren wrote:
> Hi,
>
> * Steven Price <[email protected]> [230601 10:04]:
>> I haven't studied this change in detail, but I assume the bug is that
>> serial_base_port_device_remove() shouldn't be dropping port_mutex. The
>> below hack gets my board booting again.
>
> You're right. I wonder how I managed to miss that.. Care to post a proper
> fix for this or do you want me to post it?
I'll post a proper fix shortly. Thanks for the confirmation of the fix.
>> Thanks,
>>
>> Steve
>>
>> Hack fix:
>> ----8<----
>> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
>> index 29bd5ede0b25..044e4853341a 100644
>> --- a/drivers/tty/serial/serial_core.c
>> +++ b/drivers/tty/serial/serial_core.c
>> @@ -3234,8 +3234,7 @@ static void serial_core_remove_one_port(struct uart_driver *drv,
>> wait_event(state->remove_wait, !atomic_read(&state->refcount));
>> state->uart_port = NULL;
>> mutex_unlock(&port->mutex);
>> -out:
>> - mutex_unlock(&port_mutex);
>> +out:;
>> }
>
> Seems you can remove out here and just do a return earlier instead of goto.
Yes, this was just the smallest change. I'll do it properly with an
early return in the proper patch.
Thanks,
Steve
> Regards,
>
> Tony
Hi,
* Marek Szyprowski <[email protected]> [230601 11:00]:
> Hi Tony,
>
> On 25.05.2023 13:30, Tony Lindgren wrote:
> > We want to enable runtime PM for serial port device drivers in a generic
> > way. To do this, we want to have the serial core layer manage the
> > registered physical serial controller devices.
> >
> > To manage serial controllers, let's set up a struct bus and struct device
> > for the serial core controller as suggested by Greg and Jiri. The serial
> > core controller devices are children of the physical serial port device.
> > The serial core controller device is needed to support multiple different
> > kind of ports connected to single physical serial port device.
> >
> > Let's also set up a struct device for the serial core port. The serial
> > core port instances are children of the serial core controller device.
> >
> > With the serial core port device we can now flush pending TX on the
> > runtime PM resume as suggested by Johan.
> >
> > Suggested-by: Andy Shevchenko <[email protected]>
> > Suggested-by: Greg Kroah-Hartman <[email protected]>
> > Suggested-by: Jiri Slaby <[email protected]>
> > Suggested-by: Johan Hovold <[email protected]>
> > Signed-off-by: Tony Lindgren <[email protected]>
>
> This patch landed in today's linux next-20230601 as commit 84a9582fd203
> ("serial: core: Start managing serial controllers to enable runtime
> PM"). Unfortunately it breaks booting some of my test boards. This can
> be easily reproduced with QEMU and ARM64 virt machine. The last message
> I see in the log is:
>
> [ 3.084743] Run /sbin/init as init process
OK thanks for the report. I wonder if this issue is specific to ttyAM
serial port devices somehow?
> I've tried a hack posted here by Steven Price, but unfortunately it
> doesn't fix my issue. Reverting $subject on top of next-20230601 fixes
> the boot.
OK
> Here is my qemu test command (nothing really special...):
>
> qemu-system-aarch64 -kernel Image -append "console=ttyAMA0
> no_console_suspend root=/dev/vda rootwait ip=::::target::off" -M virt
> -cpu cortex-a57 -smp 2 -m 1024 -device
> virtio-blk-device,drive=virtio-blk0 -device
> virtio-blk-device,drive=virtio-blk1 -drive
> file=qemu-virt-rootfs.raw,id=virtio-blk1,if=none,format=raw -drive
> file=initrd,id=virtio-blk0,if=none,format=raw -netdev user,id=user
> -device virtio-net-device,netdev=user -display none
OK thanks I'll try to reproduce it.
Regards,
Tony
* Tony Lindgren <[email protected]> [230601 11:12]:
> * Marek Szyprowski <[email protected]> [230601 11:00]:
> > This patch landed in today's linux next-20230601 as commit 84a9582fd203
> > ("serial: core: Start managing serial controllers to enable runtime
> > PM"). Unfortunately it breaks booting some of my test boards. This can
> > be easily reproduced with QEMU and ARM64 virt machine. The last message
> > I see in the log is:
> >
> > [ 3.084743] Run /sbin/init as init process
>
> OK thanks for the report. I wonder if this issue is specific to ttyAM
> serial port devices somehow?
Looks like the problem happens with serial port drivers that use
arch_initcall():
$ git grep arch_initcall drivers/tty/serial/
drivers/tty/serial/amba-pl011.c:arch_initcall(pl011_init);
drivers/tty/serial/mps2-uart.c:arch_initcall(mps2_uart_init);
drivers/tty/serial/mvebu-uart.c:arch_initcall(mvebu_uart_init);
drivers/tty/serial/pic32_uart.c:arch_initcall(pic32_uart_init);
drivers/tty/serial/serial_base_bus.c:arch_initcall(serial_base_init);
drivers/tty/serial/xilinx_uartps.c:arch_initcall(cdns_uart_init);
We have serial_base_bus use module_init() so the serial core controller
and port device associated with the physical serial port are not probed.
The patch below should fix the problem you're seeing, care to test and
if it works I'll post a proper fix?
Note that if we ever have cases where uart_add_one_port() gets called
even earlier, we should just call serial_base_init() directly when
adding the first port.
Regards,
Tony
8< ------------------
diff --git a/drivers/tty/serial/serial_base_bus.c b/drivers/tty/serial/serial_base_bus.c
--- a/drivers/tty/serial/serial_base_bus.c
+++ b/drivers/tty/serial/serial_base_bus.c
@@ -186,7 +186,7 @@ static int serial_base_init(void)
return ret;
}
-module_init(serial_base_init);
+arch_initcall(serial_base_init);
static void serial_base_exit(void)
{
--
2.40.1
* Marek Szyprowski <[email protected]> [230601 14:16]:
> On 01.06.2023 15:20, Tony Lindgren wrote:
> > * Tony Lindgren <[email protected]> [230601 11:12]:
> >> * Marek Szyprowski <[email protected]> [230601 11:00]:
> >>> This patch landed in today's linux next-20230601 as commit 84a9582fd203
> >>> ("serial: core: Start managing serial controllers to enable runtime
> >>> PM"). Unfortunately it breaks booting some of my test boards. This can
> >>> be easily reproduced with QEMU and ARM64 virt machine. The last message
> >>> I see in the log is:
> >>>
> >>> [ 3.084743] Run /sbin/init as init process
> >> OK thanks for the report. I wonder if this issue is specific to ttyAM
> >> serial port devices somehow?
> > Looks like the problem happens with serial port drivers that use
> > arch_initcall():
> >
> > $ git grep arch_initcall drivers/tty/serial/
> > drivers/tty/serial/amba-pl011.c:arch_initcall(pl011_init);
> > drivers/tty/serial/mps2-uart.c:arch_initcall(mps2_uart_init);
> > drivers/tty/serial/mvebu-uart.c:arch_initcall(mvebu_uart_init);
> > drivers/tty/serial/pic32_uart.c:arch_initcall(pic32_uart_init);
> > drivers/tty/serial/serial_base_bus.c:arch_initcall(serial_base_init);
> > drivers/tty/serial/xilinx_uartps.c:arch_initcall(cdns_uart_init);
> >
> > We have serial_base_bus use module_init() so the serial core controller
> > and port device associated with the physical serial port are not probed.
> >
> > The patch below should fix the problem you're seeing, care to test and
> > if it works I'll post a proper fix?
>
> Right, this fixes my issue. Feel free to add:
>
> Reported-by: Marek Szyprowski <[email protected]>
OK great just posted it with your Reported-by before seeing this, I added
returning an error too so maybe reply to the posted patch with your
Tested-by assuming it's still OK for you.
For reference if other folks are hitting this, the fix is at [0] below.
Regards,
Tony
[0] https://lore.kernel.org/linux-serial/[email protected]/T/#u
On 01.06.2023 15:20, Tony Lindgren wrote:
> * Tony Lindgren <[email protected]> [230601 11:12]:
>> * Marek Szyprowski <[email protected]> [230601 11:00]:
>>> This patch landed in today's linux next-20230601 as commit 84a9582fd203
>>> ("serial: core: Start managing serial controllers to enable runtime
>>> PM"). Unfortunately it breaks booting some of my test boards. This can
>>> be easily reproduced with QEMU and ARM64 virt machine. The last message
>>> I see in the log is:
>>>
>>> [ 3.084743] Run /sbin/init as init process
>> OK thanks for the report. I wonder if this issue is specific to ttyAM
>> serial port devices somehow?
> Looks like the problem happens with serial port drivers that use
> arch_initcall():
>
> $ git grep arch_initcall drivers/tty/serial/
> drivers/tty/serial/amba-pl011.c:arch_initcall(pl011_init);
> drivers/tty/serial/mps2-uart.c:arch_initcall(mps2_uart_init);
> drivers/tty/serial/mvebu-uart.c:arch_initcall(mvebu_uart_init);
> drivers/tty/serial/pic32_uart.c:arch_initcall(pic32_uart_init);
> drivers/tty/serial/serial_base_bus.c:arch_initcall(serial_base_init);
> drivers/tty/serial/xilinx_uartps.c:arch_initcall(cdns_uart_init);
>
> We have serial_base_bus use module_init() so the serial core controller
> and port device associated with the physical serial port are not probed.
>
> The patch below should fix the problem you're seeing, care to test and
> if it works I'll post a proper fix?
Right, this fixes my issue. Feel free to add:
Reported-by: Marek Szyprowski <[email protected]>
Tested-by: Marek Szyprowski <[email protected]>
> Note that if we ever have cases where uart_add_one_port() gets called
> even earlier, we should just call serial_base_init() directly when
> adding the first port.
>
> Regards,
>
> Tony
>
> 8< ------------------
> diff --git a/drivers/tty/serial/serial_base_bus.c b/drivers/tty/serial/serial_base_bus.c
> --- a/drivers/tty/serial/serial_base_bus.c
> +++ b/drivers/tty/serial/serial_base_bus.c
> @@ -186,7 +186,7 @@ static int serial_base_init(void)
>
> return ret;
> }
> -module_init(serial_base_init);
> +arch_initcall(serial_base_init);
>
> static void serial_base_exit(void)
> {
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
On Thu, May 25, 2023 at 02:30:30PM +0300, Tony Lindgren wrote:
> We want to enable runtime PM for serial port device drivers in a generic
> way. To do this, we want to have the serial core layer manage the
> registered physical serial controller devices.
>
> To manage serial controllers, let's set up a struct bus and struct device
> for the serial core controller as suggested by Greg and Jiri. The serial
> core controller devices are children of the physical serial port device.
> The serial core controller device is needed to support multiple different
> kind of ports connected to single physical serial port device.
>
> Let's also set up a struct device for the serial core port. The serial
> core port instances are children of the serial core controller device.
>
> With the serial core port device we can now flush pending TX on the
> runtime PM resume as suggested by Johan.
>
> Suggested-by: Andy Shevchenko <[email protected]>
> Suggested-by: Greg Kroah-Hartman <[email protected]>
> Suggested-by: Jiri Slaby <[email protected]>
> Suggested-by: Johan Hovold <[email protected]>
> Signed-off-by: Tony Lindgren <[email protected]>
This patch, in linux-next since 20230601, unfortunately breaks MediaTek
based Chromebooks. The kernel hangs during the probe of the serial ports,
which use the 8250_mtk driver. This happens even with the subsequent
fixes in next-20230602 and on the mailing list:
serial: core: Fix probing serial_base_bus devices
serial: core: Don't drop port_mutex in serial_core_remove_one_port
serial: core: Fix error handling for serial_core_ctrl_device_add()
Without the fixes, the kernel gives "WARNING: bad unlock balance detected!"
With the fixes, it just silently hangs. The last messages seen on the
(serial) console are:
Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
printk: console [ttyS0] disabled
mt6577-uart 11002000.serial: using DT '/soc/serial@11002000' for 'rs485-term' GPIO lookup
of_get_named_gpiod_flags: can't parse 'rs485-term-gpios' property of node '/soc/serial@11002000[0]'
of_get_named_gpiod_flags: can't parse 'rs485-term-gpio' property of node '/soc/serial@11002000[0]'
mt6577-uart 11002000.serial: using lookup tables for GPIO lookup
mt6577-uart 11002000.serial: No GPIO consumer rs485-term found
mt6577-uart 11002000.serial: using DT '/soc/serial@11002000' for 'rs485-rx-during-tx' GPIO lookup
of_get_named_gpiod_flags: can't parse 'rs485-rx-during-tx-gpios' property of node '/soc/serial@11002000[0]'
of_get_named_gpiod_flags: can't parse 'rs485-rx-during-tx-gpio' property of node '/soc/serial@11002000[0]'
mt6577-uart 11002000.serial: using lookup tables for GPIO lookup
mt6577-uart 11002000.serial: No GPIO consumer rs485-rx-during-tx found
What can we do to help resolve this?
Thanks
ChenYu
> ---
> Changes since v11:
> - Reworked code to add struct serial_ctrl_device and serial_port_device
> wrappers around struct device for type checking as suggested by Greg
>
> Changes since v10:
>
> - Added missing handling for unknown type in serial_base_device_add()
> as noted by kernel test robot
>
> - Use y instead of objs fro serial_base in Makefile as noted by Andy
>
> - Improve serial_port pm_ops alignment as noted by Andy
>
> Changes since v9:
>
> - Built in serial_base and core related components into serial_base.ko as
> suggested by Greg. We now have module_init only in serial_base.c that
> calls serial_base_ctrl_init() and serial_base_port_init(). I renamed
> serial_bus.c to serial_base_bus.c to build serial_base.ko. Note that
> if we wanted to build these into serial_core.ko, renaming serial_core.c
> would be needed which is not necessarily nice for folks that may have
> pending patches
>
> - Dropped string comparison for ctrl and port, and switched to using
> struct device_type as suggested by Greg
>
> - Dropped port->state checks in serial_base_get_port() as noted by
> Greg
>
> - Dropped EXPORT_SYMBOL_NS(), these are no longer needed with components
> built into serial_base.ko. I also noticed that we have some dependency
> loops if components are not built into serial_base.ko. And we would
> have hard time later on moving port specific functions to serial_port.c
> for example
>
> - Dropped checks for negative ctrl_id in serial_core_ctrl_find() as
> suggested by Greg
>
> - Stopped resetting ctrl_id in serial_core_remove_one_port(), instead
> let's properly init it in serial8250_init_port(). The ctrl_id is
> optionally passed to uart_add_one_port() and zero otherwise
>
> - Moved port_mutex and UPF_DEAD handling from serial_core_add_one_port()
> to serial_core_register_port() to simplify things a bit
>
> - Updated license and copyright as suggested by Greg
>
> - Dropped Andy's reviewed-by, things still changed quite a bit
>
> Changes since v8:
>
> - Drop unnecessary free for name noticed by Andy, the name is freed
> on put_device()
>
> - Cosmetic fix for comments in serial_port.c noted by Andy
>
> - Spelling fix for word uninitialized in serial_base_get_port()
>
> Changes since v7:
>
> - Add release() put_device() to serial_base.c as noted by Andy
>
> - Make struct serial_base_device private to serial_base.c by adding
> serial_base_get_port()
>
> - Add more comments to __uart_start()
>
> - Coding style improvments for serial_base.c from Andy
>
> Changes since v6:
>
> - Fix up a memory leak and a bunch of issues in serial_base.c as noted
> by Andy
>
> - Replace bool with new_ctrl_dev for freeing the added device on
> error path
>
> - Drop unused pm ops for serial_ctrl.c as noted by kernel test robot
>
> - Drop documentation updates for acpi devices for now to avoid a merge
> conflict and make testing easier between -rc2 and Linux next
>
> Changes since v5:
>
> - Replace platform bus and device with bus_add() and device_add(),
> Greg did not like platform bus and device here. This also gets
> rid of the need for platform data with struct serial_base_device,
> see new file serial_base.c
>
> - Update documentation to drop reference to struct uart_device as
> suggested by Andy
>
> Changes since v4:
>
> - Fix issue noted by Ilpo by calling serial_core_add_one_port() after
> the devices are created
>
> Changes since v3:
>
> - Simplify things by adding a serial core control device as the child of
> the physical serial port as suggested by Jiri
>
> - Drop the tinkering of the physical serial port device for runtime PM.
> Serial core just needs to manage port->port_dev with the addition of
> the serial core control device and the device hierarchy will keep the
> pysical serial port device enabled as needed
>
> - Simplify patch description with all the runtime PM tinkering gone
>
> - Coding style improvments as noted by Andy
>
> - Post as a single RFC patch as we're close to the merge window
>
> Changes since v2:
>
> - Make each serial port a proper device as suggested by Greg. This is
> a separate patch that flushes the TX on runtime PM resume
>
> Changes since v1:
>
> - Use kref as suggested by Andy
>
> - Fix memory leak on error as noted by Andy
>
> - Use use unsigned char for supports_autosuspend as suggested by Andy
>
> - Coding style improvments as suggested by Andy
> ---
> drivers/tty/serial/8250/8250_core.c | 1 +
> drivers/tty/serial/8250/8250_port.c | 1 +
> drivers/tty/serial/Makefile | 3 +-
> drivers/tty/serial/serial_base.h | 46 ++++++
> drivers/tty/serial/serial_base_bus.c | 200 +++++++++++++++++++++++++++
> drivers/tty/serial/serial_core.c | 192 ++++++++++++++++++++++---
> drivers/tty/serial/serial_ctrl.c | 68 +++++++++
> drivers/tty/serial/serial_port.c | 105 ++++++++++++++
> include/linux/serial_core.h | 5 +-
> 9 files changed, 598 insertions(+), 23 deletions(-)
> create mode 100644 drivers/tty/serial/serial_base.h
> create mode 100644 drivers/tty/serial/serial_base_bus.c
> create mode 100644 drivers/tty/serial/serial_ctrl.c
> create mode 100644 drivers/tty/serial/serial_port.c
>
> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -1039,6 +1039,7 @@ int serial8250_register_8250_port(const struct uart_8250_port *up)
> if (uart->port.dev)
> uart_remove_one_port(&serial8250_reg, &uart->port);
>
> + uart->port.ctrl_id = up->port.ctrl_id;
> uart->port.iobase = up->port.iobase;
> uart->port.membase = up->port.membase;
> uart->port.irq = up->port.irq;
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -3214,6 +3214,7 @@ void serial8250_init_port(struct uart_8250_port *up)
> struct uart_port *port = &up->port;
>
> spin_lock_init(&port->lock);
> + port->ctrl_id = 0;
> port->ops = &serial8250_pops;
> port->has_sysrq = IS_ENABLED(CONFIG_SERIAL_8250_CONSOLE);
>
> diff --git a/drivers/tty/serial/Makefile b/drivers/tty/serial/Makefile
> --- a/drivers/tty/serial/Makefile
> +++ b/drivers/tty/serial/Makefile
> @@ -3,7 +3,8 @@
> # Makefile for the kernel serial device drivers.
> #
>
> -obj-$(CONFIG_SERIAL_CORE) += serial_core.o
> +obj-$(CONFIG_SERIAL_CORE) += serial_base.o
> +serial_base-y := serial_core.o serial_base_bus.o serial_ctrl.o serial_port.o
>
> obj-$(CONFIG_SERIAL_EARLYCON) += earlycon.o
> obj-$(CONFIG_SERIAL_EARLYCON_SEMIHOST) += earlycon-semihost.o
> diff --git a/drivers/tty/serial/serial_base.h b/drivers/tty/serial/serial_base.h
> new file mode 100644
> --- /dev/null
> +++ b/drivers/tty/serial/serial_base.h
> @@ -0,0 +1,46 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Serial core related functions, serial port device drivers do not need this.
> + *
> + * Copyright (C) 2023 Texas Instruments Incorporated - https://www.ti.com/
> + * Author: Tony Lindgren <[email protected]>
> + */
> +
> +#define to_serial_base_ctrl_device(d) container_of((d), struct serial_ctrl_device, dev)
> +#define to_serial_base_port_device(d) container_of((d), struct serial_port_device, dev)
> +
> +struct uart_driver;
> +struct uart_port;
> +struct device_driver;
> +struct device;
> +
> +struct serial_ctrl_device {
> + struct device dev;
> +};
> +
> +struct serial_port_device {
> + struct device dev;
> + struct uart_port *port;
> +};
> +
> +int serial_base_ctrl_init(void);
> +void serial_base_ctrl_exit(void);
> +
> +int serial_base_port_init(void);
> +void serial_base_port_exit(void);
> +
> +int serial_base_driver_register(struct device_driver *driver);
> +void serial_base_driver_unregister(struct device_driver *driver);
> +
> +struct serial_ctrl_device *serial_base_ctrl_add(struct uart_port *port,
> + struct device *parent);
> +struct serial_port_device *serial_base_port_add(struct uart_port *port,
> + struct serial_ctrl_device *parent);
> +void serial_base_ctrl_device_remove(struct serial_ctrl_device *ctrl_dev);
> +void serial_base_port_device_remove(struct serial_port_device *port_dev);
> +
> +int serial_ctrl_register_port(struct uart_driver *drv, struct uart_port *port);
> +void serial_ctrl_unregister_port(struct uart_driver *drv, struct uart_port *port);
> +
> +int serial_core_register_port(struct uart_driver *drv, struct uart_port *port);
> +void serial_core_unregister_port(struct uart_driver *drv, struct uart_port *port);
> diff --git a/drivers/tty/serial/serial_base_bus.c b/drivers/tty/serial/serial_base_bus.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/tty/serial/serial_base_bus.c
> @@ -0,0 +1,200 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Serial base bus layer for controllers
> + *
> + * Copyright (C) 2023 Texas Instruments Incorporated - https://www.ti.com/
> + * Author: Tony Lindgren <[email protected]>
> + *
> + * The serial core bus manages the serial core controller instances.
> + */
> +
> +#include <linux/container_of.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/serial_core.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +
> +#include "serial_base.h"
> +
> +static int serial_base_match(struct device *dev, struct device_driver *drv)
> +{
> + int len = strlen(drv->name);
> +
> + return !strncmp(dev_name(dev), drv->name, len);
> +}
> +
> +static struct bus_type serial_base_bus_type = {
> + .name = "serial-base",
> + .match = serial_base_match,
> +};
> +
> +int serial_base_driver_register(struct device_driver *driver)
> +{
> + driver->bus = &serial_base_bus_type;
> +
> + return driver_register(driver);
> +}
> +
> +void serial_base_driver_unregister(struct device_driver *driver)
> +{
> + driver_unregister(driver);
> +}
> +
> +static int serial_base_device_init(struct uart_port *port,
> + struct device *dev,
> + struct device *parent_dev,
> + const struct device_type *type,
> + void (*release)(struct device *dev),
> + int id)
> +{
> + device_initialize(dev);
> + dev->type = type;
> + dev->parent = parent_dev;
> + dev->bus = &serial_base_bus_type;
> + dev->release = release;
> +
> + return dev_set_name(dev, "%s.%s.%d", type->name, dev_name(port->dev), id);
> +}
> +
> +static const struct device_type serial_ctrl_type = {
> + .name = "ctrl",
> +};
> +
> +static void serial_base_ctrl_release(struct device *dev)
> +{
> + struct serial_ctrl_device *ctrl_dev = to_serial_base_ctrl_device(dev);
> +
> + kfree(ctrl_dev);
> +}
> +
> +void serial_base_ctrl_device_remove(struct serial_ctrl_device *ctrl_dev)
> +{
> + if (!ctrl_dev)
> + return;
> +
> + device_del(&ctrl_dev->dev);
> +}
> +
> +struct serial_ctrl_device *serial_base_ctrl_add(struct uart_port *port,
> + struct device *parent)
> +{
> + struct serial_ctrl_device *ctrl_dev;
> + int err;
> +
> + ctrl_dev = kzalloc(sizeof(*ctrl_dev), GFP_KERNEL);
> + if (!ctrl_dev)
> + return ERR_PTR(-ENOMEM);
> +
> + err = serial_base_device_init(port, &ctrl_dev->dev,
> + parent, &serial_ctrl_type,
> + serial_base_ctrl_release,
> + port->ctrl_id);
> + if (err)
> + goto err_free_ctrl_dev;
> +
> + err = device_add(&ctrl_dev->dev);
> + if (err)
> + goto err_put_device;
> +
> + return ctrl_dev;
> +
> +err_put_device:
> + put_device(&ctrl_dev->dev);
> +err_free_ctrl_dev:
> + kfree(ctrl_dev);
> +
> + return ERR_PTR(err);
> +}
> +
> +static const struct device_type serial_port_type = {
> + .name = "port",
> +};
> +
> +static void serial_base_port_release(struct device *dev)
> +{
> + struct serial_port_device *port_dev = to_serial_base_port_device(dev);
> +
> + kfree(port_dev);
> +}
> +
> +struct serial_port_device *serial_base_port_add(struct uart_port *port,
> + struct serial_ctrl_device *ctrl_dev)
> +{
> + struct serial_port_device *port_dev;
> + int err;
> +
> + port_dev = kzalloc(sizeof(*port_dev), GFP_KERNEL);
> + if (!port_dev)
> + return ERR_PTR(-ENOMEM);
> +
> + err = serial_base_device_init(port, &port_dev->dev,
> + &ctrl_dev->dev, &serial_port_type,
> + serial_base_port_release,
> + port->line);
> + if (err)
> + goto err_free_port_dev;
> +
> + port_dev->port = port;
> +
> + err = device_add(&port_dev->dev);
> + if (err)
> + goto err_put_device;
> +
> + return port_dev;
> +
> +err_put_device:
> + put_device(&port_dev->dev);
> +err_free_port_dev:
> + kfree(port_dev);
> +
> + return ERR_PTR(err);
> +}
> +
> +void serial_base_port_device_remove(struct serial_port_device *port_dev)
> +{
> + if (!port_dev)
> + return;
> +
> + device_del(&port_dev->dev);
> +}
> +
> +static int serial_base_init(void)
> +{
> + int ret;
> +
> + ret = bus_register(&serial_base_bus_type);
> + if (ret)
> + return ret;
> +
> + ret = serial_base_ctrl_init();
> + if (ret)
> + goto err_bus_unregister;
> +
> + ret = serial_base_port_init();
> + if (ret)
> + goto err_ctrl_exit;
> +
> + return 0;
> +
> +err_ctrl_exit:
> + serial_base_ctrl_exit();
> +
> +err_bus_unregister:
> + bus_unregister(&serial_base_bus_type);
> +
> + return ret;
> +}
> +module_init(serial_base_init);
> +
> +static void serial_base_exit(void)
> +{
> + serial_base_port_exit();
> + serial_base_ctrl_exit();
> + bus_unregister(&serial_base_bus_type);
> +}
> +module_exit(serial_base_exit);
> +
> +MODULE_AUTHOR("Tony Lindgren <[email protected]>");
> +MODULE_DESCRIPTION("Serial core bus");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -17,6 +17,7 @@
> #include <linux/gpio/consumer.h>
> #include <linux/kernel.h>
> #include <linux/of.h>
> +#include <linux/pm_runtime.h>
> #include <linux/proc_fs.h>
> #include <linux/seq_file.h>
> #include <linux/device.h>
> @@ -31,6 +32,8 @@
> #include <linux/irq.h>
> #include <linux/uaccess.h>
>
> +#include "serial_base.h"
> +
> /*
> * This is used to lock changes in serial line configuration.
> */
> @@ -134,9 +137,30 @@ static void __uart_start(struct tty_struct *tty)
> {
> struct uart_state *state = tty->driver_data;
> struct uart_port *port = state->uart_port;
> + struct serial_port_device *port_dev;
> + int err;
>
> - if (port && !(port->flags & UPF_DEAD) && !uart_tx_stopped(port))
> + if (!port || port->flags & UPF_DEAD || uart_tx_stopped(port))
> + return;
> +
> + port_dev = port->port_dev;
> +
> + /* Increment the runtime PM usage count for the active check below */
> + err = pm_runtime_get(&port_dev->dev);
> + if (err < 0) {
> + pm_runtime_put_noidle(&port_dev->dev);
> + return;
> + }
> +
> + /*
> + * Start TX if enabled, and kick runtime PM. If the device is not
> + * enabled, serial_port_runtime_resume() calls start_tx() again
> + * after enabling the device.
> + */
> + if (pm_runtime_active(&port_dev->dev))
> port->ops->start_tx(port);
> + pm_runtime_mark_last_busy(&port_dev->dev);
> + pm_runtime_put_autosuspend(&port_dev->dev);
> }
>
> static void uart_start(struct tty_struct *tty)
> @@ -3042,7 +3066,7 @@ static const struct attribute_group tty_dev_attr_group = {
> };
>
> /**
> - * uart_add_one_port - attach a driver-defined port structure
> + * serial_core_add_one_port - attach a driver-defined port structure
> * @drv: pointer to the uart low level driver structure for this port
> * @uport: uart port structure to use for this port.
> *
> @@ -3051,8 +3075,9 @@ static const struct attribute_group tty_dev_attr_group = {
> * This allows the driver @drv to register its own uart_port structure with the
> * core driver. The main purpose is to allow the low level uart drivers to
> * expand uart_port, rather than having yet more levels of structures.
> + * Caller must hold port_mutex.
> */
> -int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
> +static int serial_core_add_one_port(struct uart_driver *drv, struct uart_port *uport)
> {
> struct uart_state *state;
> struct tty_port *port;
> @@ -3066,7 +3091,6 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
> state = drv->state + uport->line;
> port = &state->port;
>
> - mutex_lock(&port_mutex);
> mutex_lock(&port->mutex);
> if (state->uart_port) {
> ret = -EINVAL;
> @@ -3131,21 +3155,14 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
> uport->line);
> }
>
> - /*
> - * Ensure UPF_DEAD is not set.
> - */
> - uport->flags &= ~UPF_DEAD;
> -
> out:
> mutex_unlock(&port->mutex);
> - mutex_unlock(&port_mutex);
>
> return ret;
> }
> -EXPORT_SYMBOL(uart_add_one_port);
>
> /**
> - * uart_remove_one_port - detach a driver defined port structure
> + * serial_core_remove_one_port - detach a driver defined port structure
> * @drv: pointer to the uart low level driver structure for this port
> * @uport: uart port structure for this port
> *
> @@ -3153,20 +3170,16 @@ EXPORT_SYMBOL(uart_add_one_port);
> *
> * This unhooks (and hangs up) the specified port structure from the core
> * driver. No further calls will be made to the low-level code for this port.
> + * Caller must hold port_mutex.
> */
> -void uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
> +static void serial_core_remove_one_port(struct uart_driver *drv,
> + struct uart_port *uport)
> {
> struct uart_state *state = drv->state + uport->line;
> struct tty_port *port = &state->port;
> struct uart_port *uart_port;
> struct tty_struct *tty;
>
> - mutex_lock(&port_mutex);
> -
> - /*
> - * Mark the port "dead" - this prevents any opens from
> - * succeeding while we shut down the port.
> - */
> mutex_lock(&port->mutex);
> uart_port = uart_port_check(state);
> if (uart_port != uport)
> @@ -3177,7 +3190,6 @@ void uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
> mutex_unlock(&port->mutex);
> goto out;
> }
> - uport->flags |= UPF_DEAD;
> mutex_unlock(&port->mutex);
>
> /*
> @@ -3209,6 +3221,7 @@ void uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
> * Indicate that there isn't a port here anymore.
> */
> uport->type = PORT_UNKNOWN;
> + uport->port_dev = NULL;
>
> mutex_lock(&port->mutex);
> WARN_ON(atomic_dec_return(&state->refcount) < 0);
> @@ -3218,7 +3231,6 @@ void uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
> out:
> mutex_unlock(&port_mutex);
> }
> -EXPORT_SYMBOL(uart_remove_one_port);
>
> /**
> * uart_match_port - are the two ports equivalent?
> @@ -3253,6 +3265,144 @@ bool uart_match_port(const struct uart_port *port1,
> }
> EXPORT_SYMBOL(uart_match_port);
>
> +static struct serial_ctrl_device *
> +serial_core_get_ctrl_dev(struct serial_port_device *port_dev)
> +{
> + struct device *dev = &port_dev->dev;
> +
> + return to_serial_base_ctrl_device(dev->parent);
> +}
> +
> +/*
> + * Find a registered serial core controller device if one exists. Returns
> + * the first device matching the ctrl_id. Caller must hold port_mutex.
> + */
> +static struct serial_ctrl_device *serial_core_ctrl_find(struct uart_driver *drv,
> + struct device *phys_dev,
> + int ctrl_id)
> +{
> + struct uart_state *state;
> + int i;
> +
> + lockdep_assert_held(&port_mutex);
> +
> + for (i = 0; i < drv->nr; i++) {
> + state = drv->state + i;
> + if (!state->uart_port || !state->uart_port->port_dev)
> + continue;
> +
> + if (state->uart_port->dev == phys_dev &&
> + state->uart_port->ctrl_id == ctrl_id)
> + return serial_core_get_ctrl_dev(state->uart_port->port_dev);
> + }
> +
> + return NULL;
> +}
> +
> +static struct serial_ctrl_device *serial_core_ctrl_device_add(struct uart_port *port)
> +{
> + return serial_base_ctrl_add(port, port->dev);
> +}
> +
> +static int serial_core_port_device_add(struct serial_ctrl_device *ctrl_dev,
> + struct uart_port *port)
> +{
> + struct serial_port_device *port_dev;
> +
> + port_dev = serial_base_port_add(port, ctrl_dev);
> + if (IS_ERR(port_dev))
> + return PTR_ERR(port_dev);
> +
> + port->port_dev = port_dev;
> +
> + return 0;
> +}
> +
> +/*
> + * Initialize a serial core port device, and a controller device if needed.
> + */
> +int serial_core_register_port(struct uart_driver *drv, struct uart_port *port)
> +{
> + struct serial_ctrl_device *ctrl_dev, *new_ctrl_dev = NULL;
> + int ret;
> +
> + mutex_lock(&port_mutex);
> +
> + /*
> + * Prevent serial_port_runtime_resume() from trying to use the port
> + * until serial_core_add_one_port() has completed
> + */
> + port->flags |= UPF_DEAD;
> +
> + /* Inititalize a serial core controller device if needed */
> + ctrl_dev = serial_core_ctrl_find(drv, port->dev, port->ctrl_id);
> + if (!ctrl_dev) {
> + new_ctrl_dev = serial_core_ctrl_device_add(port);
> + if (!new_ctrl_dev) {
> + ret = -ENODEV;
> + goto err_unlock;
> + }
> + ctrl_dev = new_ctrl_dev;
> + }
> +
> + /*
> + * Initialize a serial core port device. Tag the port dead to prevent
> + * serial_port_runtime_resume() trying to do anything until port has
> + * been registered. It gets cleared by serial_core_add_one_port().
> + */
> + ret = serial_core_port_device_add(ctrl_dev, port);
> + if (ret)
> + goto err_unregister_ctrl_dev;
> +
> + ret = serial_core_add_one_port(drv, port);
> + if (ret)
> + goto err_unregister_port_dev;
> +
> + port->flags &= ~UPF_DEAD;
> +
> + mutex_unlock(&port_mutex);
> +
> + return 0;
> +
> +err_unregister_port_dev:
> + serial_base_port_device_remove(port->port_dev);
> +
> +err_unregister_ctrl_dev:
> + serial_base_ctrl_device_remove(new_ctrl_dev);
> +
> +err_unlock:
> + mutex_unlock(&port_mutex);
> +
> + return ret;
> +}
> +
> +/*
> + * Removes a serial core port device, and the related serial core controller
> + * device if the last instance.
> + */
> +void serial_core_unregister_port(struct uart_driver *drv, struct uart_port *port)
> +{
> + struct device *phys_dev = port->dev;
> + struct serial_port_device *port_dev = port->port_dev;
> + struct serial_ctrl_device *ctrl_dev = serial_core_get_ctrl_dev(port_dev);
> + int ctrl_id = port->ctrl_id;
> +
> + mutex_lock(&port_mutex);
> +
> + port->flags |= UPF_DEAD;
> +
> + serial_core_remove_one_port(drv, port);
> +
> + /* Note that struct uart_port *port is no longer valid at this point */
> + serial_base_port_device_remove(port_dev);
> +
> + /* Drop the serial core controller device if no ports are using it */
> + if (!serial_core_ctrl_find(drv, phys_dev, ctrl_id))
> + serial_base_ctrl_device_remove(ctrl_dev);
> +
> + mutex_unlock(&port_mutex);
> +}
> +
> /**
> * uart_handle_dcd_change - handle a change of carrier detect state
> * @uport: uart_port structure for the open port
> diff --git a/drivers/tty/serial/serial_ctrl.c b/drivers/tty/serial/serial_ctrl.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/tty/serial/serial_ctrl.c
> @@ -0,0 +1,68 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Serial core controller driver
> + *
> + * Copyright (C) 2023 Texas Instruments Incorporated - https://www.ti.com/
> + * Author: Tony Lindgren <[email protected]>
> + *
> + * This driver manages the serial core controller struct device instances.
> + * The serial core controller devices are children of the physical serial
> + * port device.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/serial_core.h>
> +#include <linux/spinlock.h>
> +
> +#include "serial_base.h"
> +
> +static int serial_ctrl_probe(struct device *dev)
> +{
> + pm_runtime_enable(dev);
> +
> + return 0;
> +}
> +
> +static int serial_ctrl_remove(struct device *dev)
> +{
> + pm_runtime_disable(dev);
> +
> + return 0;
> +}
> +
> +/*
> + * Serial core controller device init functions. Note that the physical
> + * serial port device driver may not have completed probe at this point.
> + */
> +int serial_ctrl_register_port(struct uart_driver *drv, struct uart_port *port)
> +{
> + return serial_core_register_port(drv, port);
> +}
> +
> +void serial_ctrl_unregister_port(struct uart_driver *drv, struct uart_port *port)
> +{
> + serial_core_unregister_port(drv, port);
> +}
> +
> +static struct device_driver serial_ctrl_driver = {
> + .name = "ctrl",
> + .suppress_bind_attrs = true,
> + .probe = serial_ctrl_probe,
> + .remove = serial_ctrl_remove,
> +};
> +
> +int serial_base_ctrl_init(void)
> +{
> + return serial_base_driver_register(&serial_ctrl_driver);
> +}
> +
> +void serial_base_ctrl_exit(void)
> +{
> + serial_base_driver_unregister(&serial_ctrl_driver);
> +}
> +
> +MODULE_AUTHOR("Tony Lindgren <[email protected]>");
> +MODULE_DESCRIPTION("Serial core controller driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/tty/serial/serial_port.c b/drivers/tty/serial/serial_port.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/tty/serial/serial_port.c
> @@ -0,0 +1,105 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Serial core port device driver
> + *
> + * Copyright (C) 2023 Texas Instruments Incorporated - https://www.ti.com/
> + * Author: Tony Lindgren <[email protected]>
> + */
> +
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/serial_core.h>
> +#include <linux/spinlock.h>
> +
> +#include "serial_base.h"
> +
> +#define SERIAL_PORT_AUTOSUSPEND_DELAY_MS 500
> +
> +/* Only considers pending TX for now. Caller must take care of locking */
> +static int __serial_port_busy(struct uart_port *port)
> +{
> + return !uart_tx_stopped(port) &&
> + uart_circ_chars_pending(&port->state->xmit);
> +}
> +
> +static int serial_port_runtime_resume(struct device *dev)
> +{
> + struct serial_port_device *port_dev = to_serial_base_port_device(dev);
> + struct uart_port *port;
> + unsigned long flags;
> +
> + port = port_dev->port;
> +
> + if (port->flags & UPF_DEAD)
> + goto out;
> +
> + /* Flush any pending TX for the port */
> + spin_lock_irqsave(&port->lock, flags);
> + if (__serial_port_busy(port))
> + port->ops->start_tx(port);
> + spin_unlock_irqrestore(&port->lock, flags);
> +
> +out:
> + pm_runtime_mark_last_busy(dev);
> +
> + return 0;
> +}
> +
> +static DEFINE_RUNTIME_DEV_PM_OPS(serial_port_pm,
> + NULL, serial_port_runtime_resume, NULL);
> +
> +static int serial_port_probe(struct device *dev)
> +{
> + pm_runtime_enable(dev);
> + pm_runtime_set_autosuspend_delay(dev, SERIAL_PORT_AUTOSUSPEND_DELAY_MS);
> + pm_runtime_use_autosuspend(dev);
> +
> + return 0;
> +}
> +
> +static int serial_port_remove(struct device *dev)
> +{
> + pm_runtime_dont_use_autosuspend(dev);
> + pm_runtime_disable(dev);
> +
> + return 0;
> +}
> +
> +/*
> + * Serial core port device init functions. Note that the physical serial
> + * port device driver may not have completed probe at this point.
> + */
> +int uart_add_one_port(struct uart_driver *drv, struct uart_port *port)
> +{
> + return serial_ctrl_register_port(drv, port);
> +}
> +EXPORT_SYMBOL(uart_add_one_port);
> +
> +void uart_remove_one_port(struct uart_driver *drv, struct uart_port *port)
> +{
> + serial_ctrl_unregister_port(drv, port);
> +}
> +EXPORT_SYMBOL(uart_remove_one_port);
> +
> +static struct device_driver serial_port_driver = {
> + .name = "port",
> + .suppress_bind_attrs = true,
> + .probe = serial_port_probe,
> + .remove = serial_port_remove,
> + .pm = pm_ptr(&serial_port_pm),
> +};
> +
> +int serial_base_port_init(void)
> +{
> + return serial_base_driver_register(&serial_port_driver);
> +}
> +
> +void serial_base_port_exit(void)
> +{
> + serial_base_driver_unregister(&serial_port_driver);
> +}
> +
> +MODULE_AUTHOR("Tony Lindgren <[email protected]>");
> +MODULE_DESCRIPTION("Serial controller port driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -28,6 +28,7 @@
>
> struct uart_port;
> struct serial_struct;
> +struct serial_port_device;
> struct device;
> struct gpio_desc;
>
> @@ -458,6 +459,7 @@ struct uart_port {
> struct serial_rs485 *rs485);
> int (*iso7816_config)(struct uart_port *,
> struct serial_iso7816 *iso7816);
> + int ctrl_id; /* optional serial core controller id */
> unsigned int irq; /* irq number */
> unsigned long irqflags; /* irq flags */
> unsigned int uartclk; /* base uart clock */
> @@ -563,7 +565,8 @@ struct uart_port {
> unsigned int minor;
> resource_size_t mapbase; /* for ioremap */
> resource_size_t mapsize;
> - struct device *dev; /* parent device */
> + struct device *dev; /* serial port physical parent device */
> + struct serial_port_device *port_dev; /* serial core port device */
>
> unsigned long sysrq; /* sysrq timeout */
> unsigned int sysrq_ch; /* char for sysrq */
> --
> 2.40.1
>
Hi,
* Chen-Yu Tsai <[email protected]> [230602 08:33]:
> This patch, in linux-next since 20230601, unfortunately breaks MediaTek
> based Chromebooks. The kernel hangs during the probe of the serial ports,
> which use the 8250_mtk driver. This happens even with the subsequent
> fixes in next-20230602 and on the mailing list:
>
> serial: core: Fix probing serial_base_bus devices
> serial: core: Don't drop port_mutex in serial_core_remove_one_port
> serial: core: Fix error handling for serial_core_ctrl_device_add()
OK thanks for reporting it.
> Without the fixes, the kernel gives "WARNING: bad unlock balance detected!"
> With the fixes, it just silently hangs. The last messages seen on the
> (serial) console are:
>
> Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
> printk: console [ttyS0] disabled
> mt6577-uart 11002000.serial: using DT '/soc/serial@11002000' for 'rs485-term' GPIO lookup
> of_get_named_gpiod_flags: can't parse 'rs485-term-gpios' property of node '/soc/serial@11002000[0]'
> of_get_named_gpiod_flags: can't parse 'rs485-term-gpio' property of node '/soc/serial@11002000[0]'
> mt6577-uart 11002000.serial: using lookup tables for GPIO lookup
> mt6577-uart 11002000.serial: No GPIO consumer rs485-term found
> mt6577-uart 11002000.serial: using DT '/soc/serial@11002000' for 'rs485-rx-during-tx' GPIO lookup
> of_get_named_gpiod_flags: can't parse 'rs485-rx-during-tx-gpios' property of node '/soc/serial@11002000[0]'
> of_get_named_gpiod_flags: can't parse 'rs485-rx-during-tx-gpio' property of node '/soc/serial@11002000[0]'
> mt6577-uart 11002000.serial: using lookup tables for GPIO lookup
> mt6577-uart 11002000.serial: No GPIO consumer rs485-rx-during-tx found
>
> What can we do to help resolve this?
There may be something blocking serial_ctrl and serial_port from
probing. That was the issue with the arch_initcall() using drivers.
Not sure yet what the issue here might be, but the 8250_mtk should be
fairly similar use case to the 8250_omap driver that I've tested with.
But unfortunately I don't think I have any 8250_mtk using devices to
test with.
The following hack should allow you to maybe see more info on what goes
wrong and allows adding some debug printk to serial_base_match() for
example to see if that gets called for mt6577-uart.
Hmm maybe early_mtk8250_setup() somehow triggers the issue? Not sure why
early_serial8250_setup() would cause issues here though.
Regards,
Tony
8< -----------------
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -144,7 +144,7 @@ static void __uart_start(struct tty_struct *tty)
return;
port_dev = port->port_dev;
-
+#if 0
/* Increment the runtime PM usage count for the active check below */
err = pm_runtime_get(&port_dev->dev);
if (err < 0) {
@@ -161,6 +161,9 @@ static void __uart_start(struct tty_struct *tty)
port->ops->start_tx(port);
pm_runtime_mark_last_busy(&port_dev->dev);
pm_runtime_put_autosuspend(&port_dev->dev);
+#else
+ port->ops->start_tx(port);
+#endif
}
static void uart_start(struct tty_struct *tty)
--
2.41.0
On 2023-06-02, Chen-Yu Tsai <[email protected]> wrote:
> This patch, in linux-next since 20230601, unfortunately breaks
> MediaTek based Chromebooks. The kernel hangs during the probe of the
> serial ports, which use the 8250_mtk driver.
Are you sure it is this patch? Have you bisected it?
Unfortunately next-20230601 also brought in a series that added
spinlocking to the 8250 driver. That may be the issue here instead.
For 8250 bug reports we really need to bisection.
John Ogness
Hi,
* John Ogness <[email protected]> [230602 10:13]:
> Unfortunately next-20230601 also brought in a series that added
> spinlocking to the 8250 driver. That may be the issue here instead.
I think you're off the hook here with the spinlocking changes :)
My guess right now is that 8250_mtk does not want runtime PM resume called
on probe for some reason, and assumes it won't happen until until in
mtk8250_do_pm()?
Looking at the probe, the driver does pm_runtime_enable(), but then calls
mtk8250_runtime_resume() directly. Not sure what the intention here is.
Maybe adding pm_runtime_set_active() in probe might provide more clues.
When we add the new serial_ctrl and serial_port devices, their runtime PM
functions propagate to the parent 8250_mtk device. And then something goes
wrong, well this is my guess on what's going on..
To me it seems the 8250_mtk should just do pm_runtime_resume_and_get()
instead of mtk8250_runtime_resume(), then do pm_runtime_put() at the end
of the probe?
I don't think 8250_mtk needs to do register access before and after the
serial port registration, but if it does, then adding custom read/write
functions can be done that do not rely on initialized port like
serial_out().
Looking at the kernelci.org test boot results for Linux next [0], seems
this issue is somehow 8250_mtk specific. I don't think the rk3399 boot
issue is serial port related.
Regards,
Tony
[0] https://linux.kernelci.org/test/job/next/branch/master/kernel/next-20230602/plan/baseline/
* Tony Lindgren <[email protected]> [230603 05:41]:
> I don't think 8250_mtk needs to do register access before and after the
> serial port registration, but if it does, then adding custom read/write
> functions can be done that do not rely on initialized port like
> serial_out().
Oh but mtk8250_runtime_suspend() calls serial_in(up, MTK_UART_DEBUG0), so
yeah if that gets called before registration is complete it causes a NULL
pointer exception. If the serial_ctrl and serial_port devices do runtime
suspend before port registration completes, things will fail.
Sounds like doing pm_runtime_resume_and_get() in mtk8250_probe() might
fix the issue. Still seems that adding a custom read function for
mtk8250_runtime_suspend() to use instead of calling serial_in() should
not be needed.
An experimental untested patch below, maye it helps?
Regards,
Tony
8< ------
diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c
--- a/drivers/tty/serial/8250/8250_mtk.c
+++ b/drivers/tty/serial/8250/8250_mtk.c
@@ -588,20 +588,24 @@ static int mtk8250_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, data);
pm_runtime_enable(&pdev->dev);
- err = mtk8250_runtime_resume(&pdev->dev);
+ err = pm_runtime_resume_and_get(&pdev->dev);
if (err)
goto err_pm_disable;
data->line = serial8250_register_8250_port(&uart);
if (data->line < 0) {
err = data->line;
- goto err_pm_disable;
+ goto err_pm_put;
}
data->rx_wakeup_irq = platform_get_irq_optional(pdev, 1);
+ pm_runtime_put_sync(&pdev->dev);
+
return 0;
+err_pm_put:
+ pm_runtime_put_sync(&pdev->dev);
err_pm_disable:
pm_runtime_disable(&pdev->dev);
--
2.41.0
Hi,
On Sat, Jun 03, 2023 at 08:41:39AM +0300, Tony Lindgren wrote:
> Looking at the kernelci.org test boot results for Linux next [0], seems
> this issue is somehow 8250_mtk specific. I don't think the rk3399 boot
> issue is serial port related.
The rk3399-gru-kevin board is broken because of a change from me
renaming CONFIG_MFD_RK808 to CONFIG_MFD_RK8XX and forgetting to
update the defconfig :( This means the board is missing its PMIC
driver. It should be fixed once the defconfig update is queued:
https://lore.kernel.org/all/[email protected]/
Unfortuantely nobody seems to feel responsible for the generic arm
defconfig files :(
-- Sebastian
Hi,
* Sebastian Reichel <[email protected]> [230603 21:57]:
> On Sat, Jun 03, 2023 at 08:41:39AM +0300, Tony Lindgren wrote:
> > Looking at the kernelci.org test boot results for Linux next [0], seems
> > this issue is somehow 8250_mtk specific. I don't think the rk3399 boot
> > issue is serial port related.
>
> The rk3399-gru-kevin board is broken because of a change from me
> renaming CONFIG_MFD_RK808 to CONFIG_MFD_RK8XX and forgetting to
> update the defconfig :( This means the board is missing its PMIC
> driver. It should be fixed once the defconfig update is queued:
>
> https://lore.kernel.org/all/[email protected]/
OK thanks for the info.
> Unfortuantely nobody seems to feel responsible for the generic arm
> defconfig files :(
You could put together a git branch with the defconfig changes and
send a pull request to the SoC maintainers if it does not get picked
up before that.
Regards,
Tony
On Fri, Jun 2, 2023 at 6:13 PM John Ogness <[email protected]> wrote:
>
> On 2023-06-02, Chen-Yu Tsai <[email protected]> wrote:
> > This patch, in linux-next since 20230601, unfortunately breaks
> > MediaTek based Chromebooks. The kernel hangs during the probe of the
> > serial ports, which use the 8250_mtk driver.
>
> Are you sure it is this patch? Have you bisected it?
>
> Unfortunately next-20230601 also brought in a series that added
> spinlocking to the 8250 driver. That may be the issue here instead.
>
> For 8250 bug reports we really need to bisection.
As Tony mentioned, you're off the hook for it.
I should've been more clear. After reverting the top three patches in
drivers/tty/serial from next-20230602, the system booted correctly again:
539914240a01 serial: core: Fix probing serial_base_bus devices
d0a396083e91 serial: core: Don't drop port_mutex in
serial_core_remove_one_port
84a9582fd203 serial: core: Start managing serial controllers to
enable runtime PM
ChenYu
* Tony Lindgren <[email protected]> [230603 06:35]:
> * Tony Lindgren <[email protected]> [230603 05:41]:
> > I don't think 8250_mtk needs to do register access before and after the
> > serial port registration, but if it does, then adding custom read/write
> > functions can be done that do not rely on initialized port like
> > serial_out().
>
> Oh but mtk8250_runtime_suspend() calls serial_in(up, MTK_UART_DEBUG0), so
> yeah if that gets called before registration is complete it causes a NULL
> pointer exception. If the serial_ctrl and serial_port devices do runtime
> suspend before port registration completes, things will fail.
>
> Sounds like doing pm_runtime_resume_and_get() in mtk8250_probe() might
> fix the issue. Still seems that adding a custom read function for
> mtk8250_runtime_suspend() to use instead of calling serial_in() should
> not be needed.
Looking at this again, if serial8250_register_8250_port() fails, then
mtk8250_runtime_suspend() would again try to access uninitialized port.
Here's a better untested version of the patch to try.
Regards,
Tony
8< ---------------------------
diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c
--- a/drivers/tty/serial/8250/8250_mtk.c
+++ b/drivers/tty/serial/8250/8250_mtk.c
@@ -57,6 +57,8 @@
#define MTK_UART_XON1 40 /* I/O: Xon character 1 */
#define MTK_UART_XOFF1 42 /* I/O: Xoff character 1 */
+#define MTK_UART_REGSHIFT 2
+
#ifdef CONFIG_SERIAL_8250_DMA
enum dma_rx_status {
DMA_RX_START = 0,
@@ -69,6 +71,7 @@ struct mtk8250_data {
int line;
unsigned int rx_pos;
unsigned int clk_count;
+ void __iomem *membase;
struct clk *uart_clk;
struct clk *bus_clk;
struct uart_8250_dma *dma;
@@ -187,6 +190,17 @@ static void mtk8250_dma_enable(struct uart_8250_port *up)
}
#endif
+/* Read and write for register access before and after port registration */
+static u32 __maybe_unused mtk8250_read(struct mtk8250_data *data, u32 reg)
+{
+ return readl(data->membase + (reg << MTK_UART_REGSHIFT));
+}
+
+static void mtk8250_write(struct mtk8250_data *data, u32 reg, u32 val)
+{
+ writel(val, data->membase + (reg << MTK_UART_REGSHIFT));
+}
+
static int mtk8250_startup(struct uart_port *port)
{
#ifdef CONFIG_SERIAL_8250_DMA
@@ -425,11 +439,10 @@ mtk8250_set_termios(struct uart_port *port, struct ktermios *termios,
static int __maybe_unused mtk8250_runtime_suspend(struct device *dev)
{
struct mtk8250_data *data = dev_get_drvdata(dev);
- struct uart_8250_port *up = serial8250_get_port(data->line);
/* wait until UART in idle status */
while
- (serial_in(up, MTK_UART_DEBUG0));
+ (mtk8250_read(data, MTK_UART_DEBUG0));
if (data->clk_count == 0U) {
dev_dbg(dev, "%s clock count is 0\n", __func__);
@@ -553,6 +566,7 @@ static int mtk8250_probe(struct platform_device *pdev)
if (!data)
return -ENOMEM;
+ data->membase = uart.port.membase;
data->clk_count = 0;
if (pdev->dev.of_node) {
@@ -570,7 +584,7 @@ static int mtk8250_probe(struct platform_device *pdev)
uart.port.flags = UPF_BOOT_AUTOCONF | UPF_FIXED_PORT;
uart.port.dev = &pdev->dev;
uart.port.iotype = UPIO_MEM32;
- uart.port.regshift = 2;
+ uart.port.regshift = MTK_UART_REGSHIFT;
uart.port.private_data = data;
uart.port.shutdown = mtk8250_shutdown;
uart.port.startup = mtk8250_startup;
@@ -581,27 +595,30 @@ static int mtk8250_probe(struct platform_device *pdev)
uart.dma = data->dma;
#endif
- /* Disable Rate Fix function */
- writel(0x0, uart.port.membase +
- (MTK_UART_RATE_FIX << uart.port.regshift));
-
platform_set_drvdata(pdev, data);
pm_runtime_enable(&pdev->dev);
- err = mtk8250_runtime_resume(&pdev->dev);
+ err = pm_runtime_resume_and_get(&pdev->dev);
if (err)
goto err_pm_disable;
+ /* Disable Rate Fix function */
+ mtk8250_write(data, 0, MTK_UART_RATE_FIX);
+
data->line = serial8250_register_8250_port(&uart);
if (data->line < 0) {
err = data->line;
- goto err_pm_disable;
+ goto err_pm_put;
}
data->rx_wakeup_irq = platform_get_irq_optional(pdev, 1);
+ pm_runtime_put_sync(&pdev->dev);
+
return 0;
+err_pm_put:
+ pm_runtime_put_sync(&pdev->dev);
err_pm_disable:
pm_runtime_disable(&pdev->dev);
@@ -694,7 +711,7 @@ static int __init early_mtk8250_setup(struct earlycon_device *device,
return -ENODEV;
device->port.iotype = UPIO_MEM32;
- device->port.regshift = 2;
+ device->port.regshift = MTK_UART_REGSHIFT;
return early_serial8250_setup(device, NULL);
}
On Mon, Jun 05, 2023 at 09:15:11AM +0300, Tony Lindgren wrote:
> * Tony Lindgren <[email protected]> [230603 06:35]:
...
> /* wait until UART in idle status */
> while
> - (serial_in(up, MTK_UART_DEBUG0));
> + (mtk8250_read(data, MTK_UART_DEBUG0));
In case you go with this, make it a single line.
--
With Best Regards,
Andy Shevchenko
Hi,
On Mon, Jun 5, 2023 at 2:15 PM Tony Lindgren <[email protected]> wrote:
>
> * Tony Lindgren <[email protected]> [230603 06:35]:
> > * Tony Lindgren <[email protected]> [230603 05:41]:
> > > I don't think 8250_mtk needs to do register access before and after the
> > > serial port registration, but if it does, then adding custom read/write
> > > functions can be done that do not rely on initialized port like
> > > serial_out().
> >
> > Oh but mtk8250_runtime_suspend() calls serial_in(up, MTK_UART_DEBUG0), so
> > yeah if that gets called before registration is complete it causes a NULL
> > pointer exception. If the serial_ctrl and serial_port devices do runtime
> > suspend before port registration completes, things will fail.
> >
> > Sounds like doing pm_runtime_resume_and_get() in mtk8250_probe() might
> > fix the issue. Still seems that adding a custom read function for
> > mtk8250_runtime_suspend() to use instead of calling serial_in() should
> > not be needed.
>
> Looking at this again, if serial8250_register_8250_port() fails, then
> mtk8250_runtime_suspend() would again try to access uninitialized port.
>
> Here's a better untested version of the patch to try.
>
> Regards,
>
> Tony
>
> 8< ---------------------------
> diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c
> --- a/drivers/tty/serial/8250/8250_mtk.c
> +++ b/drivers/tty/serial/8250/8250_mtk.c
> @@ -57,6 +57,8 @@
> #define MTK_UART_XON1 40 /* I/O: Xon character 1 */
> #define MTK_UART_XOFF1 42 /* I/O: Xoff character 1 */
>
> +#define MTK_UART_REGSHIFT 2
> +
> #ifdef CONFIG_SERIAL_8250_DMA
> enum dma_rx_status {
> DMA_RX_START = 0,
> @@ -69,6 +71,7 @@ struct mtk8250_data {
> int line;
> unsigned int rx_pos;
> unsigned int clk_count;
> + void __iomem *membase;
> struct clk *uart_clk;
> struct clk *bus_clk;
> struct uart_8250_dma *dma;
> @@ -187,6 +190,17 @@ static void mtk8250_dma_enable(struct uart_8250_port *up)
> }
> #endif
>
> +/* Read and write for register access before and after port registration */
> +static u32 __maybe_unused mtk8250_read(struct mtk8250_data *data, u32 reg)
> +{
> + return readl(data->membase + (reg << MTK_UART_REGSHIFT));
> +}
> +
> +static void mtk8250_write(struct mtk8250_data *data, u32 reg, u32 val)
> +{
> + writel(val, data->membase + (reg << MTK_UART_REGSHIFT));
> +}
> +
> static int mtk8250_startup(struct uart_port *port)
> {
> #ifdef CONFIG_SERIAL_8250_DMA
> @@ -425,11 +439,10 @@ mtk8250_set_termios(struct uart_port *port, struct ktermios *termios,
> static int __maybe_unused mtk8250_runtime_suspend(struct device *dev)
> {
> struct mtk8250_data *data = dev_get_drvdata(dev);
> - struct uart_8250_port *up = serial8250_get_port(data->line);
>
> /* wait until UART in idle status */
> while
> - (serial_in(up, MTK_UART_DEBUG0));
> + (mtk8250_read(data, MTK_UART_DEBUG0));
I believe it still gets stuck here sometimes.
With your earlier patch, it could get through registering the port, and
the console would show
11002000.serial: ttyS0 at MMIO 0x11002000 (irq = 240, base_baud =
1625000) is a ST16650V2
for the console UART.
Angelo mentioned that we should be using SLEEP_REQ/SLEEP_ACK registers
in the MTK UART hardware.
I tried reworking it into your patch here, but it causes issues with the
UART-based Bluetooth on one of my devices. After the UART runtime suspends
and resumes, something is off and causes the transfers during Bluetooth
init to become corrupt.
I'll try some more stuff, but the existing code seems timing dependent.
If I add too many printk statements to the runtime suspend/resume
callbacks, things seem to work. One time I even ended up with broken
UARTs but otherwise booted up the system.
ChenYu
>
> if (data->clk_count == 0U) {
> dev_dbg(dev, "%s clock count is 0\n", __func__);
> @@ -553,6 +566,7 @@ static int mtk8250_probe(struct platform_device *pdev)
> if (!data)
> return -ENOMEM;
>
> + data->membase = uart.port.membase;
> data->clk_count = 0;
>
> if (pdev->dev.of_node) {
> @@ -570,7 +584,7 @@ static int mtk8250_probe(struct platform_device *pdev)
> uart.port.flags = UPF_BOOT_AUTOCONF | UPF_FIXED_PORT;
> uart.port.dev = &pdev->dev;
> uart.port.iotype = UPIO_MEM32;
> - uart.port.regshift = 2;
> + uart.port.regshift = MTK_UART_REGSHIFT;
> uart.port.private_data = data;
> uart.port.shutdown = mtk8250_shutdown;
> uart.port.startup = mtk8250_startup;
> @@ -581,27 +595,30 @@ static int mtk8250_probe(struct platform_device *pdev)
> uart.dma = data->dma;
> #endif
>
> - /* Disable Rate Fix function */
> - writel(0x0, uart.port.membase +
> - (MTK_UART_RATE_FIX << uart.port.regshift));
> -
> platform_set_drvdata(pdev, data);
>
> pm_runtime_enable(&pdev->dev);
> - err = mtk8250_runtime_resume(&pdev->dev);
> + err = pm_runtime_resume_and_get(&pdev->dev);
> if (err)
> goto err_pm_disable;
>
> + /* Disable Rate Fix function */
> + mtk8250_write(data, 0, MTK_UART_RATE_FIX);
> +
> data->line = serial8250_register_8250_port(&uart);
> if (data->line < 0) {
> err = data->line;
> - goto err_pm_disable;
> + goto err_pm_put;
> }
>
> data->rx_wakeup_irq = platform_get_irq_optional(pdev, 1);
>
> + pm_runtime_put_sync(&pdev->dev);
> +
> return 0;
>
> +err_pm_put:
> + pm_runtime_put_sync(&pdev->dev);
> err_pm_disable:
> pm_runtime_disable(&pdev->dev);
>
> @@ -694,7 +711,7 @@ static int __init early_mtk8250_setup(struct earlycon_device *device,
> return -ENODEV;
>
> device->port.iotype = UPIO_MEM32;
> - device->port.regshift = 2;
> + device->port.regshift = MTK_UART_REGSHIFT;
>
> return early_serial8250_setup(device, NULL);
> }
* Chen-Yu Tsai <[email protected]> [230605 11:34]:
> On Mon, Jun 5, 2023 at 2:15 PM Tony Lindgren <[email protected]> wrote:
> > diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c
> > --- a/drivers/tty/serial/8250/8250_mtk.c
> > +++ b/drivers/tty/serial/8250/8250_mtk.c
> > @@ -425,11 +439,10 @@ mtk8250_set_termios(struct uart_port *port, struct ktermios *termios,
> > static int __maybe_unused mtk8250_runtime_suspend(struct device *dev)
> > {
> > struct mtk8250_data *data = dev_get_drvdata(dev);
> > - struct uart_8250_port *up = serial8250_get_port(data->line);
> >
> > /* wait until UART in idle status */
> > while
> > - (serial_in(up, MTK_UART_DEBUG0));
> > + (mtk8250_read(data, MTK_UART_DEBUG0));
>
> I believe it still gets stuck here sometimes.
Hmm so maybe you need to mtk8250_write(data, 0, MTK_UART_RATE_FIX) in
probe before pm_runtime_resume_and_get() that enables the baud clock?
That's something I changed, so maybe it messes up things.
Looking at the 8250_mtk git log, it's runtime PM functions seem to only
currently manage the baud clock so register access should be doable
without runtime PM resume?
> With your earlier patch, it could get through registering the port, and
> the console would show
>
> 11002000.serial: ttyS0 at MMIO 0x11002000 (irq = 240, base_baud =
> 1625000) is a ST16650V2
>
> for the console UART.
OK
> Angelo mentioned that we should be using SLEEP_REQ/SLEEP_ACK registers
> in the MTK UART hardware.
>
> I tried reworking it into your patch here, but it causes issues with the
> UART-based Bluetooth on one of my devices. After the UART runtime suspends
> and resumes, something is off and causes the transfers during Bluetooth
> init to become corrupt.
>
> I'll try some more stuff, but the existing code seems timing dependent.
> If I add too many printk statements to the runtime suspend/resume
> callbacks, things seem to work. One time I even ended up with broken
> UARTs but otherwise booted up the system.
Well another thing that now changes is that we now runtime suspend the
port at the end of the probe. What the 8250_mtk probe was doing earlier
it was leaving the port baud clock enabled, but runtime PM disabled
until mtk8250_do_pm() I guess.
Regards,
Tony
* Andy Shevchenko <[email protected]> [230605 11:28]:
> On Mon, Jun 05, 2023 at 09:15:11AM +0300, Tony Lindgren wrote:
> > * Tony Lindgren <[email protected]> [230603 06:35]:
>
> ...
>
> > /* wait until UART in idle status */
> > while
> > - (serial_in(up, MTK_UART_DEBUG0));
> > + (mtk8250_read(data, MTK_UART_DEBUG0));
>
> In case you go with this, make it a single line.
OK makes sense thanks.
Tony
On Mon, Jun 5, 2023 at 8:24 PM Tony Lindgren <[email protected]> wrote:
>
> * Chen-Yu Tsai <[email protected]> [230605 11:34]:
> > On Mon, Jun 5, 2023 at 2:15 PM Tony Lindgren <[email protected]> wrote:
> > > diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c
> > > --- a/drivers/tty/serial/8250/8250_mtk.c
> > > +++ b/drivers/tty/serial/8250/8250_mtk.c
> > > @@ -425,11 +439,10 @@ mtk8250_set_termios(struct uart_port *port, struct ktermios *termios,
> > > static int __maybe_unused mtk8250_runtime_suspend(struct device *dev)
> > > {
> > > struct mtk8250_data *data = dev_get_drvdata(dev);
> > > - struct uart_8250_port *up = serial8250_get_port(data->line);
> > >
> > > /* wait until UART in idle status */
> > > while
> > > - (serial_in(up, MTK_UART_DEBUG0));
> > > + (mtk8250_read(data, MTK_UART_DEBUG0));
> >
> > I believe it still gets stuck here sometimes.
>
> Hmm so maybe you need to mtk8250_write(data, 0, MTK_UART_RATE_FIX) in
> probe before pm_runtime_resume_and_get() that enables the baud clock?
> That's something I changed, so maybe it messes up things.
I think it has something to do with the do_pm() function calling
the callbacks directly, then also calling runtime PM.
> Looking at the 8250_mtk git log, it's runtime PM functions seem to only
> currently manage the baud clock so register access should be doable
> without runtime PM resume?
Actually it only manages the bus clock. The baud clock is simply the system
XTAL which is not gateble.
> > With your earlier patch, it could get through registering the port, and
> > the console would show
> >
> > 11002000.serial: ttyS0 at MMIO 0x11002000 (irq = 240, base_baud =
> > 1625000) is a ST16650V2
> >
> > for the console UART.
>
> OK
>
> > Angelo mentioned that we should be using SLEEP_REQ/SLEEP_ACK registers
> > in the MTK UART hardware.
> >
> > I tried reworking it into your patch here, but it causes issues with the
> > UART-based Bluetooth on one of my devices. After the UART runtime suspends
> > and resumes, something is off and causes the transfers during Bluetooth
> > init to become corrupt.
> >
> > I'll try some more stuff, but the existing code seems timing dependent.
> > If I add too many printk statements to the runtime suspend/resume
> > callbacks, things seem to work. One time I even ended up with broken
> > UARTs but otherwise booted up the system.
>
> Well another thing that now changes is that we now runtime suspend the
> port at the end of the probe. What the 8250_mtk probe was doing earlier
> it was leaving the port baud clock enabled, but runtime PM disabled
> until mtk8250_do_pm() I guess.
I guess that's the biggest difference? Since the *bus* clock gets disabled,
any access will hang. Is it enough to just support runtime PM? Or do I have
to also have UART_CAP_RPM?
ChenYu
* Chen-Yu Tsai <[email protected]> [230605 13:01]:
> On Mon, Jun 5, 2023 at 8:24 PM Tony Lindgren <[email protected]> wrote:
> >
> > * Chen-Yu Tsai <[email protected]> [230605 11:34]:
> > > On Mon, Jun 5, 2023 at 2:15 PM Tony Lindgren <[email protected]> wrote:
> > > > diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c
> > > > --- a/drivers/tty/serial/8250/8250_mtk.c
> > > > +++ b/drivers/tty/serial/8250/8250_mtk.c
> > > > @@ -425,11 +439,10 @@ mtk8250_set_termios(struct uart_port *port, struct ktermios *termios,
> > > > static int __maybe_unused mtk8250_runtime_suspend(struct device *dev)
> > > > {
> > > > struct mtk8250_data *data = dev_get_drvdata(dev);
> > > > - struct uart_8250_port *up = serial8250_get_port(data->line);
> > > >
> > > > /* wait until UART in idle status */
> > > > while
> > > > - (serial_in(up, MTK_UART_DEBUG0));
> > > > + (mtk8250_read(data, MTK_UART_DEBUG0));
> > >
> > > I believe it still gets stuck here sometimes.
> >
> > Hmm so maybe you need to mtk8250_write(data, 0, MTK_UART_RATE_FIX) in
> > probe before pm_runtime_resume_and_get() that enables the baud clock?
> > That's something I changed, so maybe it messes up things.
>
> I think it has something to do with the do_pm() function calling
> the callbacks directly, then also calling runtime PM.
Yeah I'm not following really what's going on there.. So then I guess the
call for mtk8250_write(data, 0, MTK_UART_RATE_FIX) should be after the
pm_runtime_resume_and_get() call.
> > Looking at the 8250_mtk git log, it's runtime PM functions seem to only
> > currently manage the baud clock so register access should be doable
> > without runtime PM resume?
>
> Actually it only manages the bus clock. The baud clock is simply the system
> XTAL which is not gateble.
OK
> > > With your earlier patch, it could get through registering the port, and
> > > the console would show
> > >
> > > 11002000.serial: ttyS0 at MMIO 0x11002000 (irq = 240, base_baud =
> > > 1625000) is a ST16650V2
> > >
> > > for the console UART.
> >
> > OK
> >
> > > Angelo mentioned that we should be using SLEEP_REQ/SLEEP_ACK registers
> > > in the MTK UART hardware.
> > >
> > > I tried reworking it into your patch here, but it causes issues with the
> > > UART-based Bluetooth on one of my devices. After the UART runtime suspends
> > > and resumes, something is off and causes the transfers during Bluetooth
> > > init to become corrupt.
> > >
> > > I'll try some more stuff, but the existing code seems timing dependent.
> > > If I add too many printk statements to the runtime suspend/resume
> > > callbacks, things seem to work. One time I even ended up with broken
> > > UARTs but otherwise booted up the system.
> >
> > Well another thing that now changes is that we now runtime suspend the
> > port at the end of the probe. What the 8250_mtk probe was doing earlier
> > it was leaving the port baud clock enabled, but runtime PM disabled
> > until mtk8250_do_pm() I guess.
>
> I guess that's the biggest difference? Since the *bus* clock gets disabled,
> any access will hang. Is it enough to just support runtime PM? Or do I have
> to also have UART_CAP_RPM?
Maybe try changing pm_runtime_put_sync() at the end of the probe to just
pm_runtime_put_noidle()? Then the driver should be back to where it was
with clocks enabled but runtime PM suspended.
I don't think you need UART_CAP_RPM right now unless 8250_mtk adds support
for autosuspend. That stuff will get replaced by the serial_core generic
PM patch from Andy. I think in it's current form 8250_mtk just gets enabled
when the port is opened, and disabled when the port is closed. And gets
disabled for system suspend.
Regards,
Tony
On Mon, Jun 5, 2023 at 9:18 PM Tony Lindgren <[email protected]> wrote:
>
> * Chen-Yu Tsai <[email protected]> [230605 13:01]:
> > On Mon, Jun 5, 2023 at 8:24 PM Tony Lindgren <[email protected]> wrote:
> > >
> > > * Chen-Yu Tsai <[email protected]> [230605 11:34]:
> > > > On Mon, Jun 5, 2023 at 2:15 PM Tony Lindgren <[email protected]> wrote:
> > > > > diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c
> > > > > --- a/drivers/tty/serial/8250/8250_mtk.c
> > > > > +++ b/drivers/tty/serial/8250/8250_mtk.c
> > > > > @@ -425,11 +439,10 @@ mtk8250_set_termios(struct uart_port *port, struct ktermios *termios,
> > > > > static int __maybe_unused mtk8250_runtime_suspend(struct device *dev)
> > > > > {
> > > > > struct mtk8250_data *data = dev_get_drvdata(dev);
> > > > > - struct uart_8250_port *up = serial8250_get_port(data->line);
> > > > >
> > > > > /* wait until UART in idle status */
> > > > > while
> > > > > - (serial_in(up, MTK_UART_DEBUG0));
> > > > > + (mtk8250_read(data, MTK_UART_DEBUG0));
> > > >
> > > > I believe it still gets stuck here sometimes.
> > >
> > > Hmm so maybe you need to mtk8250_write(data, 0, MTK_UART_RATE_FIX) in
> > > probe before pm_runtime_resume_and_get() that enables the baud clock?
> > > That's something I changed, so maybe it messes up things.
> >
> > I think it has something to do with the do_pm() function calling
> > the callbacks directly, then also calling runtime PM.
>
> Yeah I'm not following really what's going on there.. So then I guess the
> call for mtk8250_write(data, 0, MTK_UART_RATE_FIX) should be after the
> pm_runtime_resume_and_get() call.
>
> > > Looking at the 8250_mtk git log, it's runtime PM functions seem to only
> > > currently manage the baud clock so register access should be doable
> > > without runtime PM resume?
> >
> > Actually it only manages the bus clock. The baud clock is simply the system
> > XTAL which is not gateble.
>
> OK
>
> > > > With your earlier patch, it could get through registering the port, and
> > > > the console would show
> > > >
> > > > 11002000.serial: ttyS0 at MMIO 0x11002000 (irq = 240, base_baud =
> > > > 1625000) is a ST16650V2
> > > >
> > > > for the console UART.
> > >
> > > OK
> > >
> > > > Angelo mentioned that we should be using SLEEP_REQ/SLEEP_ACK registers
> > > > in the MTK UART hardware.
> > > >
> > > > I tried reworking it into your patch here, but it causes issues with the
> > > > UART-based Bluetooth on one of my devices. After the UART runtime suspends
> > > > and resumes, something is off and causes the transfers during Bluetooth
> > > > init to become corrupt.
> > > >
> > > > I'll try some more stuff, but the existing code seems timing dependent.
> > > > If I add too many printk statements to the runtime suspend/resume
> > > > callbacks, things seem to work. One time I even ended up with broken
> > > > UARTs but otherwise booted up the system.
> > >
> > > Well another thing that now changes is that we now runtime suspend the
> > > port at the end of the probe. What the 8250_mtk probe was doing earlier
> > > it was leaving the port baud clock enabled, but runtime PM disabled
> > > until mtk8250_do_pm() I guess.
> >
> > I guess that's the biggest difference? Since the *bus* clock gets disabled,
> > any access will hang. Is it enough to just support runtime PM? Or do I have
> > to also have UART_CAP_RPM?
>
> Maybe try changing pm_runtime_put_sync() at the end of the probe to just
> pm_runtime_put_noidle()? Then the driver should be back to where it was
> with clocks enabled but runtime PM suspended.
>
> I don't think you need UART_CAP_RPM right now unless 8250_mtk adds support
> for autosuspend. That stuff will get replaced by the serial_core generic
> PM patch from Andy. I think in it's current form 8250_mtk just gets enabled
> when the port is opened, and disabled when the port is closed. And gets
> disabled for system suspend.
I ended up following 8250_dw's design, which seemed less convoluted.
The original code was waaay too convoluted.
BTW, the Bluetooth breakage seems like a different problem. It works
on v6.4-rc5, but breaks somewhere between that and next, before the
runtime PM series. This particular device has a Qualcomm WiFi/BT chip
with the Bluetooth part going through UART. The btqca reports a bunch
of frame reassembly errors during and after initialization:
Bluetooth: hci0: setting up ROME/QCA6390
Bluetooth: hci0: Frame reassembly failed (-84)
Bluetooth: hci0: QCA Product ID :0x00000008
Bluetooth: hci0: QCA SOC Version :0x00000044
Bluetooth: hci0: QCA ROM Version :0x00000302
Bluetooth: hci0: QCA Patch Version:0x00000111
Bluetooth: hci0: QCA controller version 0x00440302
Bluetooth: hci0: QCA Downloading qca/rampatch_00440302.bin
Bluetooth: hci0: Frame reassembly failed (-84)
Bluetooth: hci0: QCA Downloading qca/nvm_00440302_i2s.bin
Bluetooth: hci0: QCA setup on UART is completed
Bluetooth: hci0: Opcode 0x1002 failed: -110
Bluetooth: hci0: command 0x1002 tx timeout
Bluetooth: hci0: crash the soc to collect controller dump
Bluetooth: hci0: QCA collecting dump of size:196608
Bluetooth: hci0: Frame reassembly failed (-84)
...
Bluetooth: hci0: Frame reassembly failed (-84)
Bluetooth: Received HCI_IBS_WAKE_ACK in tx state 0
Bluetooth: hci0: Frame reassembly failed (-84)
...
Bluetooth: hci0: Frame reassembly failed (-84)
Bluetooth: hci0: Frame reassembly failed (-90)
Bluetooth: hci0: Frame reassembly failed (-84)
...
Bluetooth: hci0: Frame reassembly failed (-84)
Bluetooth: hci0: Injecting HCI hardware error event
However on a different device that has a Realtek WiFi/BT chip,
it doesn't seem to run into errors.
Just putting it out there in case anyone else runs into it.
Thank you for your help on this.
ChenYu
* Chen-Yu Tsai <[email protected]> [230606 09:17]:
> I ended up following 8250_dw's design, which seemed less convoluted.
> The original code was waaay too convoluted.
OK that looks good to me thanks. Good to hear you got it sorted out.
The 8250_dw style runtime PM is a good solution for simple cases. Where
it won't work are SoCs where runtime PM calls need to propagate up the
bus hierarchy. For example, 8250_omap needs runtime PM calls for the
interconnect and power domain to get register access working.
> BTW, the Bluetooth breakage seems like a different problem.
OK seems like we're good to go then :)
Regards,
Tony
On Tue, Jun 6, 2023 at 8:21 PM Tony Lindgren <[email protected]> wrote:
>
> * Chen-Yu Tsai <[email protected]> [230606 09:17]:
> > I ended up following 8250_dw's design, which seemed less convoluted.
> > The original code was waaay too convoluted.
>
> OK that looks good to me thanks. Good to hear you got it sorted out.
>
> The 8250_dw style runtime PM is a good solution for simple cases. Where
> it won't work are SoCs where runtime PM calls need to propagate up the
> bus hierarchy. For example, 8250_omap needs runtime PM calls for the
> interconnect and power domain to get register access working.
Good to know. On MediaTek platforms I don't think there are any power
domains covering the basic peripherals. (Or it's hidden from the kernel.)
> > BTW, the Bluetooth breakage seems like a different problem.
>
> OK seems like we're good to go then :)
Yup. After a bit more testing, it seems the Bluetooth problem is more like
an undervolt issue. If I have WiFi and BT probe at the same time, Bluetooth
fails. If they probe separately, everything works fine.
ChenYu
Il 07/06/23 06:46, Chen-Yu Tsai ha scritto:
> On Tue, Jun 6, 2023 at 8:21 PM Tony Lindgren <[email protected]> wrote:
>>
>> * Chen-Yu Tsai <[email protected]> [230606 09:17]:
>>> I ended up following 8250_dw's design, which seemed less convoluted.
>>> The original code was waaay too convoluted.
>>
>> OK that looks good to me thanks. Good to hear you got it sorted out.
>>
>> The 8250_dw style runtime PM is a good solution for simple cases. Where
>> it won't work are SoCs where runtime PM calls need to propagate up the
>> bus hierarchy. For example, 8250_omap needs runtime PM calls for the
>> interconnect and power domain to get register access working.
>
> Good to know. On MediaTek platforms I don't think there are any power
> domains covering the basic peripherals. (Or it's hidden from the kernel.)
>
On (relatively) new SoCs, basic peripherals are always powered, you're correct.
Cheers,
Angelo
>>> BTW, the Bluetooth breakage seems like a different problem.
>>
>> OK seems like we're good to go then :)
>
> Yup. After a bit more testing, it seems the Bluetooth problem is more like
> an undervolt issue. If I have WiFi and BT probe at the same time, Bluetooth
> fails. If they probe separately, everything works fine.
>
> ChenYu
On Wed, Jun 07, 2023 at 12:46:51PM +0800, Chen-Yu Tsai wrote:
> On Tue, Jun 6, 2023 at 8:21 PM Tony Lindgren <[email protected]> wrote:
...
> After a bit more testing, it seems the Bluetooth problem is more like
> an undervolt issue.
Undercurrent I believe.
Just my pedantic 2 cents and electronics engineer by education :-)
> If I have WiFi and BT probe at the same time, Bluetooth
> fails. If they probe separately, everything works fine.
--
With Best Regards,
Andy Shevchenko
On 5/25/23 13:30, Tony Lindgren wrote:
> We want to enable runtime PM for serial port device drivers in a generic
> way. To do this, we want to have the serial core layer manage the
> registered physical serial controller devices.
>
> To manage serial controllers, let's set up a struct bus and struct device
> for the serial core controller as suggested by Greg and Jiri. The serial
> core controller devices are children of the physical serial port device.
> The serial core controller device is needed to support multiple different
> kind of ports connected to single physical serial port device.
>
> Let's also set up a struct device for the serial core port. The serial
> core port instances are children of the serial core controller device.
>
> With the serial core port device we can now flush pending TX on the
> runtime PM resume as suggested by Johan.
Hi,
Unfortunately, this patch (now commit 84a9582fd203 with v6.5) breaks suspend on
a bunch of Microsoft Surface devices (confirmed via git bisect).
The root cause is that when we enter system suspend with the serial port in
runtime suspend, all transfers will be paused until the serial port is
runtime-resumed, which will only happen after complete() is called, so
basically after we are done resuming the system. In short: This patch
essentially blocks all serial communication in system suspend transitions.
The affected devices have an EC (the Surface Aggregator Module) which needs
some communication when entering system suspend. In particular, we need to tell
it to stop sending us events, turn off the keyboard backlight, and transition
it to a lower power mode. With this patch, all of these operations now time
out, preventing us from entering suspend.
A bad workaround is to disable runtime PM, e.g. via
echo on > /sys/bus/serial-base/devices/dw-apb-uart.4:0/dw-apb-uart.4:0.0/power/control
or the diff attached below, but obviously that's not a great solution and can
be broken quite easily from userspace in the same way (and without users really
actively doing so through tools like TLP).
Any ideas on how this can be fixed without reverting?
See also https://github.com/linux-surface/linux-surface/issues/1258.
Regards,
Max
---
drivers/tty/serial/serial_port.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/tty/serial/serial_port.c b/drivers/tty/serial/serial_port.c
index 862423237007..6ceacea5e790 100644
--- a/drivers/tty/serial/serial_port.c
+++ b/drivers/tty/serial/serial_port.c
@@ -55,6 +55,8 @@ static int serial_port_probe(struct device *dev)
pm_runtime_set_autosuspend_delay(dev, SERIAL_PORT_AUTOSUSPEND_DELAY_MS);
pm_runtime_use_autosuspend(dev);
+ pm_runtime_forbid(dev);
+
return 0;
}
--
2.42.0
Hi,
* Maximilian Luz <[email protected]> [231003 11:57]:
> A bad workaround is to disable runtime PM, e.g. via
>
> echo on > /sys/bus/serial-base/devices/dw-apb-uart.4:0/dw-apb-uart.4:0.0/power/control
If the touchscreen controller driver(s) are using serdev they are
children of the dw-apb-uart.4:0.0 and can use runtime PM calls to
block the parent device from idling as necessary. The hierarchy
unless changed using ignore_children.
Then when the children are done, seem like dw-apb-uart driver should
use force_suspend and force_resume calls in the system suspend path.
Do you have some mainline kernel test case available or is this
still out of tree code?
Regards,
Tony
* Tony Lindgren <[email protected]> [231003 12:15]:
> Hi,
>
> * Maximilian Luz <[email protected]> [231003 11:57]:
> > A bad workaround is to disable runtime PM, e.g. via
> >
> > echo on > /sys/bus/serial-base/devices/dw-apb-uart.4:0/dw-apb-uart.4:0.0/power/control
>
> If the touchscreen controller driver(s) are using serdev they are
> children of the dw-apb-uart.4:0.0 and can use runtime PM calls to
> block the parent device from idling as necessary. The hierarchy
> unless changed using ignore_children.
Sorry about all the typos, I meant "the hierarchy works unless changed"
above. The rest of the typos are easier to decipher probably :)
Regards,
Tony
On 10/3/23 14:21, Tony Lindgren wrote:
> * Tony Lindgren <[email protected]> [231003 12:15]:
>> Hi,
>>
>> * Maximilian Luz <[email protected]> [231003 11:57]:
>>> A bad workaround is to disable runtime PM, e.g. via
>>>
>>> echo on > /sys/bus/serial-base/devices/dw-apb-uart.4:0/dw-apb-uart.4:0.0/power/control
>>
>> If the touchscreen controller driver(s) are using serdev they are
>> children of the dw-apb-uart.4:0.0 and can use runtime PM calls to
>> block the parent device from idling as necessary. The hierarchy
>> unless changed using ignore_children.
>
> Sorry about all the typos, I meant "the hierarchy works unless changed"
> above. The rest of the typos are easier to decipher probably :)
Unfortunately that doesn't quite line up with what I can see on v6.5.5. The
serdev controller seems to be a child of dw-apb-uart.4, a platform device. The
serial-base and serdev devices are siblings. According to sysfs:
/sys/bus/platform/devices/dw-apb-uart.4
├── driver -> ../../../../bus/platform/drivers/dw-apb-uart
├── subsystem -> ../../../../bus/platform
│
├── dw-apb-uart.4:0
│ ├── driver -> ../../../../../bus/serial-base/drivers/ctrl
│ ├── subsystem -> ../../../../../bus/serial-base
│ │
│ └── dw-apb-uart.4:0.0
│ ├── driver -> ../../../../../../bus/serial-base/drivers/port
│ └── subsystem -> ../../../../../../bus/serial-base
│
└── serial0
├── subsystem -> ../../../../../bus/serial
│
└── serial0-0
├── driver -> ../../../../../../bus/serial/drivers/surface_serial_hub
└── subsystem -> ../../../../../../bus/serial
Runtime suspend on serial0-0 is disabled/not set up at all. So I assume that if
it were a descendent of dw-apb-uart.4:0.0, things should have worked
out-of-the-box.
Regards,
Max
* Maximilian Luz <[email protected]> [231003 22:09]:
> On 10/3/23 14:21, Tony Lindgren wrote:
> > * Tony Lindgren <[email protected]> [231003 12:15]:
> > > Hi,
> > >
> > > * Maximilian Luz <[email protected]> [231003 11:57]:
> > > > A bad workaround is to disable runtime PM, e.g. via
> > > >
> > > > echo on > /sys/bus/serial-base/devices/dw-apb-uart.4:0/dw-apb-uart.4:0.0/power/control
> > >
> > > If the touchscreen controller driver(s) are using serdev they are
> > > children of the dw-apb-uart.4:0.0 and can use runtime PM calls to
> > > block the parent device from idling as necessary. The hierarchy
> > > unless changed using ignore_children.
> >
> > Sorry about all the typos, I meant "the hierarchy works unless changed"
> > above. The rest of the typos are easier to decipher probably :)
>
> Unfortunately that doesn't quite line up with what I can see on v6.5.5. The
> serdev controller seems to be a child of dw-apb-uart.4, a platform device. The
> serial-base and serdev devices are siblings. According to sysfs:
>
> /sys/bus/platform/devices/dw-apb-uart.4
> ├── driver -> ../../../../bus/platform/drivers/dw-apb-uart
> ├── subsystem -> ../../../../bus/platform
> │
> ├── dw-apb-uart.4:0
> │ ├── driver -> ../../../../../bus/serial-base/drivers/ctrl
> │ ├── subsystem -> ../../../../../bus/serial-base
> │ │
> │ └── dw-apb-uart.4:0.0
> │ ├── driver -> ../../../../../../bus/serial-base/drivers/port
> │ └── subsystem -> ../../../../../../bus/serial-base
> │
> └── serial0
> ├── subsystem -> ../../../../../bus/serial
> │
> └── serial0-0
> ├── driver -> ../../../../../../bus/serial/drivers/surface_serial_hub
> └── subsystem -> ../../../../../../bus/serial
The hierachy above is correct. Looks like I pasted the wrong device above,
I meant dw-apb-uart.4, sorry about the extra confusion added. Eventually
the serdev device could be a child of dw-apb-uart.4:0.0 at some point as
it's specific to a serial port instance, but for now that should not be
needed.
If serial0-0 is runtime PM active, then dw-apb-uart.4 is runtime PM active
also unless ingore_children is set.
> Runtime suspend on serial0-0 is disabled/not set up at all. So I assume that if
> it were a descendent of dw-apb-uart.4:0.0, things should have worked
> out-of-the-box.
Hmm yes so maybe the issue is not with surface_serial_hub, but with serial
port device being nable to resume after __device_suspend_late() has
disabled runtime PM like you've been saying.
If the issue is with the serial port not being able to runtime resume, then
the patch below should help. Care to give it a try?
Regards,
Tony
8< ------------------
diff --git a/drivers/tty/serial/serial_port.c b/drivers/tty/serial/serial_port.c
--- a/drivers/tty/serial/serial_port.c
+++ b/drivers/tty/serial/serial_port.c
@@ -46,8 +46,27 @@ static int serial_port_runtime_resume(struct device *dev)
return 0;
}
-static DEFINE_RUNTIME_DEV_PM_OPS(serial_port_pm,
- NULL, serial_port_runtime_resume, NULL);
+/*
+ * Allow serdev devices to talk to hardware during system suspend.
+ * Assumes the serial port hardware controller device driver calls
+ * pm_runtime_force_suspend() and pm_runtime_force_resume() for
+ * system suspend as needed.
+ */
+static int serial_port_prepare(struct device *dev)
+{
+ return pm_runtime_resume_and_get(dev);
+}
+
+static void serial_port_complete(struct device *dev)
+{
+ pm_runtime_put_sync(dev);
+}
+
+static const struct dev_pm_ops __maybe_unused serial_port_pm = {
+ SET_RUNTIME_PM_OPS(NULL, serial_port_runtime_resume, NULL)
+ .prepare = serial_port_prepare,
+ .complete = serial_port_complete,
+};
static int serial_port_probe(struct device *dev)
{
--
2.42.0
On Wed, Oct 04, 2023 at 09:17:08AM +0300, Tony Lindgren wrote:
> * Maximilian Luz <[email protected]> [231003 22:09]:
> > Unfortunately that doesn't quite line up with what I can see on v6.5.5. The
> > serdev controller seems to be a child of dw-apb-uart.4, a platform device. The
> > serial-base and serdev devices are siblings. According to sysfs:
> >
> > /sys/bus/platform/devices/dw-apb-uart.4
> > ├── driver -> ../../../../bus/platform/drivers/dw-apb-uart
> > ├── subsystem -> ../../../../bus/platform
> > │
> > ├── dw-apb-uart.4:0
> > │ ├── driver -> ../../../../../bus/serial-base/drivers/ctrl
> > │ ├── subsystem -> ../../../../../bus/serial-base
> > │ │
> > │ └── dw-apb-uart.4:0.0
> > │ ├── driver -> ../../../../../../bus/serial-base/drivers/port
> > │ └── subsystem -> ../../../../../../bus/serial-base
> > │
> > └── serial0
> > ├── subsystem -> ../../../../../bus/serial
> > │
> > └── serial0-0
> > ├── driver -> ../../../../../../bus/serial/drivers/surface_serial_hub
> > └── subsystem -> ../../../../../../bus/serial
>
> The hierachy above is correct. Looks like I pasted the wrong device above,
> I meant dw-apb-uart.4, sorry about the extra confusion added. Eventually
> the serdev device could be a child of dw-apb-uart.4:0.0 at some point as
> it's specific to a serial port instance, but for now that should not be
> needed.
>
> If serial0-0 is runtime PM active, then dw-apb-uart.4 is runtime PM active
> also unless ingore_children is set.
>
> > Runtime suspend on serial0-0 is disabled/not set up at all. So I assume that if
> > it were a descendent of dw-apb-uart.4:0.0, things should have worked
> > out-of-the-box.
>
> Hmm yes so maybe the issue is not with surface_serial_hub, but with serial
> port device being nable to resume after __device_suspend_late() has
> disabled runtime PM like you've been saying.
>
> If the issue is with the serial port not being able to runtime resume, then
> the patch below should help. Care to give it a try?
> 8< ------------------
> diff --git a/drivers/tty/serial/serial_port.c b/drivers/tty/serial/serial_port.c
> --- a/drivers/tty/serial/serial_port.c
> +++ b/drivers/tty/serial/serial_port.c
> @@ -46,8 +46,27 @@ static int serial_port_runtime_resume(struct device *dev)
> return 0;
> }
>
> -static DEFINE_RUNTIME_DEV_PM_OPS(serial_port_pm,
> - NULL, serial_port_runtime_resume, NULL);
> +/*
> + * Allow serdev devices to talk to hardware during system suspend.
> + * Assumes the serial port hardware controller device driver calls
> + * pm_runtime_force_suspend() and pm_runtime_force_resume() for
> + * system suspend as needed.
> + */
> +static int serial_port_prepare(struct device *dev)
> +{
> + return pm_runtime_resume_and_get(dev);
> +}
> +
> +static void serial_port_complete(struct device *dev)
> +{
> + pm_runtime_put_sync(dev);
> +}
> +
> +static const struct dev_pm_ops __maybe_unused serial_port_pm = {
> + SET_RUNTIME_PM_OPS(NULL, serial_port_runtime_resume, NULL)
> + .prepare = serial_port_prepare,
> + .complete = serial_port_complete,
> +};
>
> static int serial_port_probe(struct device *dev)
> {
Just a drive-by comment: The above looks like a too big of a hammer and
the wrong place to fix this.
The serdev runtime PM implementation is supposed to just work for serdev
drivers that do not want to use it, and otherwise those drivers manage
the runtime PM state of the serdev (serial) controller directly (e.g.
see c3bf40ce2c20 ("serdev: add controller runtime PM support")).
Without having time to look at this regression (or the rework) in
detail, it seems like the serial core rework has broken the serdev
runtime PM implementation if the serial controller is now suspended
without the serdev driver having asked for it.
The pm_runtime_get_sync() in serdev_device_open() is supposed to prevent
that from happening by default and if that now longer works, then that
needs to be fixed.
Johan
On 10/4/23 08:17, Tony Lindgren wrote:
> * Maximilian Luz <[email protected]> [231003 22:09]:
>> On 10/3/23 14:21, Tony Lindgren wrote:
>>> * Tony Lindgren <[email protected]> [231003 12:15]:
>>>> Hi,
>>>>
>>>> * Maximilian Luz <[email protected]> [231003 11:57]:
>>>>> A bad workaround is to disable runtime PM, e.g. via
>>>>>
>>>>> echo on > /sys/bus/serial-base/devices/dw-apb-uart.4:0/dw-apb-uart.4:0.0/power/control
>>>>
>>>> If the touchscreen controller driver(s) are using serdev they are
>>>> children of the dw-apb-uart.4:0.0 and can use runtime PM calls to
>>>> block the parent device from idling as necessary. The hierarchy
>>>> unless changed using ignore_children.
>>>
>>> Sorry about all the typos, I meant "the hierarchy works unless changed"
>>> above. The rest of the typos are easier to decipher probably :)
>>
>> Unfortunately that doesn't quite line up with what I can see on v6.5.5. The
>> serdev controller seems to be a child of dw-apb-uart.4, a platform device. The
>> serial-base and serdev devices are siblings. According to sysfs:
>>
>> /sys/bus/platform/devices/dw-apb-uart.4
>> ├── driver -> ../../../../bus/platform/drivers/dw-apb-uart
>> ├── subsystem -> ../../../../bus/platform
>> │
>> ├── dw-apb-uart.4:0
>> │ ├── driver -> ../../../../../bus/serial-base/drivers/ctrl
>> │ ├── subsystem -> ../../../../../bus/serial-base
>> │ │
>> │ └── dw-apb-uart.4:0.0
>> │ ├── driver -> ../../../../../../bus/serial-base/drivers/port
>> │ └── subsystem -> ../../../../../../bus/serial-base
>> │
>> └── serial0
>> ├── subsystem -> ../../../../../bus/serial
>> │
>> └── serial0-0
>> ├── driver -> ../../../../../../bus/serial/drivers/surface_serial_hub
>> └── subsystem -> ../../../../../../bus/serial
>
> The hierachy above is correct. Looks like I pasted the wrong device above,
> I meant dw-apb-uart.4, sorry about the extra confusion added. Eventually
> the serdev device could be a child of dw-apb-uart.4:0.0 at some point as
> it's specific to a serial port instance, but for now that should not be
> needed.
>
> If serial0-0 is runtime PM active, then dw-apb-uart.4 is runtime PM active
> also unless ingore_children is set.
>
>> Runtime suspend on serial0-0 is disabled/not set up at all. So I assume that if
>> it were a descendent of dw-apb-uart.4:0.0, things should have worked
>> out-of-the-box.
>
> Hmm yes so maybe the issue is not with surface_serial_hub, but with serial
> port device being nable to resume after __device_suspend_late() has
> disabled runtime PM like you've been saying.
>
> If the issue is with the serial port not being able to runtime resume, then
> the patch below should help. Care to give it a try?
Your patch does indeed make it work. So that's at least a better workaround
that we can carry in our downstream for now. Thanks!
Regards,
Max
> 8< ------------------
> diff --git a/drivers/tty/serial/serial_port.c b/drivers/tty/serial/serial_port.c
> --- a/drivers/tty/serial/serial_port.c
> +++ b/drivers/tty/serial/serial_port.c
> @@ -46,8 +46,27 @@ static int serial_port_runtime_resume(struct device *dev)
> return 0;
> }
>
> -static DEFINE_RUNTIME_DEV_PM_OPS(serial_port_pm,
> - NULL, serial_port_runtime_resume, NULL);
> +/*
> + * Allow serdev devices to talk to hardware during system suspend.
> + * Assumes the serial port hardware controller device driver calls
> + * pm_runtime_force_suspend() and pm_runtime_force_resume() for
> + * system suspend as needed.
> + */
> +static int serial_port_prepare(struct device *dev)
> +{
> + return pm_runtime_resume_and_get(dev);
> +}
> +
> +static void serial_port_complete(struct device *dev)
> +{
> + pm_runtime_put_sync(dev);
> +}
> +
> +static const struct dev_pm_ops __maybe_unused serial_port_pm = {
> + SET_RUNTIME_PM_OPS(NULL, serial_port_runtime_resume, NULL)
> + .prepare = serial_port_prepare,
> + .complete = serial_port_complete,
> +};
>
> static int serial_port_probe(struct device *dev)
> {
* Johan Hovold <[email protected]> [231004 07:14]:
> The pm_runtime_get_sync() in serdev_device_open() is supposed to prevent
> that from happening by default and if that now longer works, then that
> needs to be fixed.
No changes there, that all should work just as before.
What is broken is that the new serial port device can autosuspend while
the serdev device is active. This prevents serial tx in the suspend
path.
The serial port device and serdev device are siblings of the physical
serial port controller device as seen in the hierarcy printed out by
Maximilian.
Regards,
Tony
On Wed, Oct 04, 2023 at 12:03:20PM +0300, Tony Lindgren wrote:
> * Johan Hovold <[email protected]> [231004 07:14]:
> > The pm_runtime_get_sync() in serdev_device_open() is supposed to prevent
> > that from happening by default and if that now longer works, then that
> > needs to be fixed.
>
> No changes there, that all should work just as before.
Well, it clearly does not work as before.
> What is broken is that the new serial port device can autosuspend while
> the serdev device is active. This prevents serial tx in the suspend
> path.
>
> The serial port device and serdev device are siblings of the physical
> serial port controller device as seen in the hierarcy printed out by
> Maximilian.
Yeah, and that's precisely the broken part. Keeping the serdev
controller active is supposed to keep the serial controller active. Your
serial core rework appears to have broken just that.
The new "devices" that you've added (I have still not tried to
understand why that was even needed, it looks overly complicated) must
not change that.
If the serdev controller is active, tx should just work (as it did
before).
Johan
* Johan Hovold <[email protected]> [231004 09:14]:
> On Wed, Oct 04, 2023 at 12:03:20PM +0300, Tony Lindgren wrote:
> > The serial port device and serdev device are siblings of the physical
> > serial port controller device as seen in the hierarcy printed out by
> > Maximilian.
>
> Yeah, and that's precisely the broken part. Keeping the serdev
> controller active is supposed to keep the serial controller active. Your
> serial core rework appears to have broken just that.
Hmm OK good point, tx can currently have an extra delay if a serdev
device is active, and the serial port controller device is not active.
So we can check for active port->dev instead of &port_dev->dev though
to know when when start_tx() is safe to do as below.
Thanks.
Tony
8< -----------------
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 6207f0051f23d..defecc5b04422 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -156,7 +156,7 @@ static void __uart_start(struct uart_state *state)
* enabled, serial_port_runtime_resume() calls start_tx() again
* after enabling the device.
*/
- if (pm_runtime_active(&port_dev->dev))
+ if (!pm_runtime_enabled(port->dev) || pm_runtime_active(port->dev))
port->ops->start_tx(port);
pm_runtime_mark_last_busy(&port_dev->dev);
pm_runtime_put_autosuspend(&port_dev->dev);
On 10/4/23 12:01, Tony Lindgren wrote:
> * Johan Hovold <[email protected]> [231004 09:14]:
>> On Wed, Oct 04, 2023 at 12:03:20PM +0300, Tony Lindgren wrote:
>>> The serial port device and serdev device are siblings of the physical
>>> serial port controller device as seen in the hierarcy printed out by
>>> Maximilian.
>>
>> Yeah, and that's precisely the broken part. Keeping the serdev
>> controller active is supposed to keep the serial controller active. Your
>> serial core rework appears to have broken just that.
>
> Hmm OK good point, tx can currently have an extra delay if a serdev
> device is active, and the serial port controller device is not active.
>
> So we can check for active port->dev instead of &port_dev->dev though
> to know when when start_tx() is safe to do as below.
I can confirm that this works as well.
Thanks,
Max
> 8< -----------------
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 6207f0051f23d..defecc5b04422 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -156,7 +156,7 @@ static void __uart_start(struct uart_state *state)
> * enabled, serial_port_runtime_resume() calls start_tx() again
> * after enabling the device.
> */
> - if (pm_runtime_active(&port_dev->dev))
> + if (!pm_runtime_enabled(port->dev) || pm_runtime_active(port->dev))
> port->ops->start_tx(port);
> pm_runtime_mark_last_busy(&port_dev->dev);
> pm_runtime_put_autosuspend(&port_dev->dev);
>