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 do this, let's set up a 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: 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 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/Makefile | 2 +-
drivers/tty/serial/serial_core.c | 189 ++++++++++++++++++++++++++--
drivers/tty/serial/serial_ctrl.c | 61 +++++++++
drivers/tty/serial/serial_ctrl.h | 22 ++++
drivers/tty/serial/serial_port.c | 109 ++++++++++++++++
include/linux/serial_core.h | 4 +-
7 files changed, 379 insertions(+), 9 deletions(-)
create mode 100644 drivers/tty/serial/serial_ctrl.c
create mode 100644 drivers/tty/serial/serial_ctrl.h
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
@@ -996,6 +996,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/Makefile b/drivers/tty/serial/Makefile
--- a/drivers/tty/serial/Makefile
+++ b/drivers/tty/serial/Makefile
@@ -3,7 +3,7 @@
# Makefile for the kernel serial device drivers.
#
-obj-$(CONFIG_SERIAL_CORE) += serial_core.o
+obj-$(CONFIG_SERIAL_CORE) += serial_core.o serial_ctrl.o serial_port.o
obj-$(CONFIG_SERIAL_EARLYCON) += earlycon.o
obj-$(CONFIG_SERIAL_EARLYCON_ARM_SEMIHOST) += earlycon-arm-semihost.o
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
@@ -16,6 +16,8 @@
#include <linux/console.h>
#include <linux/gpio/consumer.h>
#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
#include <linux/proc_fs.h>
#include <linux/seq_file.h>
#include <linux/device.h>
@@ -30,6 +32,8 @@
#include <linux/irq.h>
#include <linux/uaccess.h>
+#include "serial_ctrl.h"
+
/*
* This is used to lock changes in serial line configuration.
*/
@@ -136,9 +140,31 @@ static void __uart_start(struct tty_struct *tty)
{
struct uart_state *state = tty->driver_data;
struct uart_port *port = state->uart_port;
+ struct device *port_dev;
+ int err;
+
+ if (!port || uart_tx_stopped(port))
+ return;
- if (port && !uart_tx_stopped(port))
+ port_dev = port->port_dev;
+
+ err = pm_runtime_get(port_dev);
+ if (err < 0) {
+ /* Something went wrong, attempt to start TX anyways */
port->ops->start_tx(port);
+ pm_runtime_put_noidle(port_dev);
+ return;
+ }
+
+ /*
+ * Start TX if enabled, and kick runtime PM. Otherwise we must
+ * wait for a retry. See also serial_port.c for runtime PM
+ * autosuspend timeout.
+ */
+ if (pm_runtime_active(port_dev))
+ port->ops->start_tx(port);
+ pm_runtime_mark_last_busy(port_dev);
+ pm_runtime_put_autosuspend(port_dev);
}
static void uart_start(struct tty_struct *tty)
@@ -3035,7 +3061,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.
*
@@ -3045,7 +3071,7 @@ static const struct attribute_group tty_dev_attr_group = {
* 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.
*/
-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;
@@ -3135,10 +3161,9 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
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
*
@@ -3147,7 +3172,8 @@ 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.
*/
-int uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
+static int 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;
@@ -3204,6 +3230,8 @@ int 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;
+ uport->ctrl_id = -ENODEV;
mutex_lock(&port->mutex);
WARN_ON(atomic_dec_return(&state->refcount) < 0);
@@ -3215,7 +3243,6 @@ int uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
return ret;
}
-EXPORT_SYMBOL(uart_remove_one_port);
/**
* uart_match_port - are the two ports equivalent?
@@ -3250,6 +3277,154 @@ bool uart_match_port(const struct uart_port *port1,
}
EXPORT_SYMBOL(uart_match_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 device *serial_core_ctrl_find(struct uart_driver *drv,
+ struct device *phys_dev,
+ int ctrl_id)
+{
+ struct uart_state *state;
+ int i;
+
+ if (ctrl_id < 0)
+ return NULL;
+
+ 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 state->uart_port->port_dev->parent;
+ }
+
+ return NULL;
+}
+
+static struct platform_device *serial_core_device_add(struct uart_port *port,
+ const char *name,
+ struct device *parent_dev,
+ void *pdata,
+ int pdata_size)
+{
+ struct platform_device_info pinfo = {};
+
+ pinfo.name = name;
+ pinfo.id = PLATFORM_DEVID_AUTO;
+ pinfo.parent = parent_dev;
+ pinfo.data = pdata;
+ pinfo.size_data = pdata_size;
+
+ return platform_device_register_full(&pinfo);
+}
+
+static struct device *serial_core_ctrl_device_add(struct uart_port *port)
+{
+ struct platform_device *pdev;
+
+ pdev = serial_core_device_add(port, "serial-ctrl", port->dev, NULL, 0);
+ if (IS_ERR(pdev))
+ return NULL;
+
+ return &pdev->dev;
+}
+
+static int serial_core_port_device_add(struct device *ctrl_dev, struct uart_port *port)
+{
+ struct serial_port_platdata pdata = {};
+ struct platform_device *pdev;
+
+ pdata.port = port;
+
+ pdev = serial_core_device_add(port, "serial-port", ctrl_dev, &pdata,
+ sizeof(pdata));
+ if (IS_ERR(pdev))
+ return PTR_ERR(pdev);
+
+ port->port_dev = &pdev->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 platform_device *ctrl_pdev = NULL;
+ struct device *ctrl_dev;
+ int ret;
+
+ mutex_lock(&port_mutex);
+
+ /* Inititalize a serial core controller device if needed */
+ ctrl_dev = serial_core_ctrl_find(drv, port->dev, port->ctrl_id);
+ if (!ctrl_dev) {
+ ctrl_dev = serial_core_ctrl_device_add(port);
+ if (!ctrl_dev) {
+ ret = -ENODEV;
+ goto err_unlock;
+ }
+ ctrl_pdev = to_platform_device(ctrl_dev);
+ }
+
+ /* Initialize a serial core port device */
+ ret = serial_core_port_device_add(ctrl_dev, port);
+ if (ret)
+ goto err_unregister_ctrl_dev;
+
+ mutex_unlock(&port_mutex);
+
+ ret = serial_core_add_one_port(drv, port);
+ if (ret)
+ goto err_unregister_port_dev;
+
+ return 0;
+
+err_unregister_port_dev:
+ mutex_lock(&port_mutex);
+ platform_device_unregister(to_platform_device(port->port_dev));
+
+err_unregister_ctrl_dev:
+ platform_device_unregister(ctrl_pdev);
+
+err_unlock:
+ mutex_unlock(&port_mutex);
+
+ return ret;
+}
+EXPORT_SYMBOL_NS(serial_core_register_port, SERIAL_CORE);
+
+/*
+ * 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 device *port_dev = port->port_dev;
+ struct device *ctrl_dev = port_dev->parent;
+ int ctrl_id = port->ctrl_id;
+
+ serial_core_remove_one_port(drv, port);
+
+ mutex_lock(&port_mutex);
+
+ platform_device_unregister(to_platform_device(port_dev));
+
+ /* Drop the serial core controller device if no ports are using it */
+ if (!serial_core_ctrl_find(drv, phys_dev, ctrl_id))
+ platform_device_unregister(to_platform_device(ctrl_dev));
+
+ mutex_unlock(&port_mutex);
+}
+EXPORT_SYMBOL_NS(serial_core_unregister_port, SERIAL_CORE);
+
/**
* 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,61 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+/*
+ * Serial core controller driver
+ *
+ * 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/module.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/serial_core.h>
+
+#include "serial_ctrl.h"
+
+static int serial_ctrl_probe(struct platform_device *pdev)
+{
+ pm_runtime_enable(&pdev->dev);
+
+ return 0;
+}
+
+static int serial_ctrl_remove(struct platform_device *pdev)
+{
+ pm_runtime_disable(&pdev->dev);
+
+ return 0;
+}
+
+static struct platform_driver serial_ctrl_driver = {
+ .driver = {
+ .name = "serial-ctrl",
+ .suppress_bind_attrs = true,
+ },
+ .probe = serial_ctrl_probe,
+ .remove = serial_ctrl_remove,
+};
+module_platform_driver(serial_ctrl_driver);
+
+/*
+ * 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);
+}
+EXPORT_SYMBOL_NS(serial_ctrl_register_port, SERIAL_CORE);
+
+void serial_ctrl_unregister_port(struct uart_driver *drv, struct uart_port *port)
+{
+ serial_core_unregister_port(drv, port);
+}
+EXPORT_SYMBOL_NS(serial_ctrl_unregister_port, SERIAL_CORE);
+
+MODULE_AUTHOR("Tony Lindgren <[email protected]>");
+MODULE_DESCRIPTION("Serial core controller driver");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(SERIAL_CORE);
diff --git a/drivers/tty/serial/serial_ctrl.h b/drivers/tty/serial/serial_ctrl.h
new file mode 100644
--- /dev/null
+++ b/drivers/tty/serial/serial_ctrl.h
@@ -0,0 +1,22 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+/* Serial core controller data. Serial port device drivers do not need this. */
+
+struct uart_driver;
+struct uart_port;
+
+/**
+ * struct serial_port_platdata - Serial port platform data
+ * @state: serial port state
+ *
+ * Used by serial_core and serial_port only. Allocated on uart_add_one_port(),
+ * and freed on uart_remove_one_port().
+ */
+struct serial_port_platdata {
+ struct uart_port *port;
+};
+
+extern int serial_ctrl_register_port(struct uart_driver *drv, struct uart_port *port);
+extern void serial_ctrl_unregister_port(struct uart_driver *drv, struct uart_port *port);
+extern int serial_core_register_port(struct uart_driver *drv, struct uart_port *port);
+extern void serial_core_unregister_port(struct uart_driver *drv, struct uart_port *port);
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,109 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+/*
+ * Serial core port device driver
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/serial_core.h>
+
+#include "serial_ctrl.h"
+
+#define SERIAL_PORT_AUTOSUSPEND_DELAY_MS 500
+
+struct serial_port_data {
+ struct uart_port *port;
+};
+
+/* 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_data *ddata = dev_get_drvdata(dev);
+ struct uart_port *port = ddata->port;
+ unsigned long flags;
+
+ /* 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);
+ 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 platform_device *pdev)
+{
+ struct serial_port_platdata *pd = dev_get_platdata(&pdev->dev);
+ struct device *dev = &pdev->dev;
+ struct serial_port_data *ddata;
+
+ ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
+ if (!ddata)
+ return -ENOMEM;
+
+ ddata->port = pd->port;
+ platform_set_drvdata(pdev, ddata);
+
+ 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 platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+
+ pm_runtime_dont_use_autosuspend(dev);
+ pm_runtime_disable(dev);
+
+ return 0;
+}
+
+static struct platform_driver serial_port_driver = {
+ .driver = {
+ .name = "serial-port",
+ .suppress_bind_attrs = true,
+ .pm = pm_ptr(&serial_port_pm),
+ },
+ .probe = serial_port_probe,
+ .remove = serial_port_remove,
+};
+module_platform_driver(serial_port_driver);
+
+/*
+ * 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);
+
+int uart_remove_one_port(struct uart_driver *drv, struct uart_port *port)
+{
+ serial_ctrl_unregister_port(drv, port);
+
+ return 0;
+}
+EXPORT_SYMBOL(uart_remove_one_port);
+
+MODULE_AUTHOR("Tony Lindgren <[email protected]>");
+MODULE_DESCRIPTION("Serial controller port driver");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(SERIAL_CORE);
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
@@ -458,6 +458,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 +564,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 device *port_dev; /* serial core port device */
unsigned long sysrq; /* sysrq timeout */
unsigned int sysrq_ch; /* char for sysrq */
--
2.39.0
On Mon, Jan 16, 2023 at 09:59:58AM +0200, 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 do this, let's set up a 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.
Looking better, but why is this new device a platform device? That
feels odd, you should never have a platform device hanging off of a
non-platform device, right?
What does the sysfs tree look like now with this patch applied?
thanks,
greg k-h
* Greg Kroah-Hartman <[email protected]> [230131 10:10]:
> On Mon, Jan 16, 2023 at 09:59:58AM +0200, 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 do this, let's set up a 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.
>
> Looking better, but why is this new device a platform device? That
> feels odd, you should never have a platform device hanging off of a
> non-platform device, right?
No special need for it to be a platform device. It just is easy to set
up, and for my test case the serial port physical device is also a
platform device.
What's your preference here?
> What does the sysfs tree look like now with this patch applied?
Below are two examples of what's in sysfs.
Regards,
Tony
8< -----------------
On arm64 with two physical 8250 devices I have:
# ls -l /sys/devices/platform/serial8250/
total 0
lrwxrwxrwx 1 root root 0 Jan 1 1970 driver -> ../../../bus/platform/drivers/serial8250
-rw-r--r-- 1 root root 4096 Jan 1 1970 driver_override
-r--r--r-- 1 root root 4096 Jan 1 1970 modalias
drwxr-xr-x 2 root root 0 Jan 1 1970 power
drwxr-xr-x 5 root root 0 Jan 1 1970 serial-ctrl.0.auto
lrwxrwxrwx 1 root root 0 Jan 1 1970 subsystem -> ../../../bus/platform
drwxr-xr-x 4 root root 0 Jan 1 1970 tty
-rw-r--r-- 1 root root 4096 Jan 1 1970 uevent
# ls -l /sys/devices/platform/serial8250/serial-ctrl*
total 0
lrwxrwxrwx 1 root root 0 Jan 1 1970 driver -> ../../../../bus/platform/drivers/serial-ctrl
-rw-r--r-- 1 root root 4096 Jan 1 1970 driver_override
-r--r--r-- 1 root root 4096 Jan 1 1970 modalias
drwxr-xr-x 2 root root 0 Jan 1 1970 power
drwxr-xr-x 3 root root 0 Jan 1 1970 serial-port.1.auto
drwxr-xr-x 3 root root 0 Jan 1 1970 serial-port.4.auto
lrwxrwxrwx 1 root root 0 Jan 1 1970 subsystem -> ../../../../bus/platform
-rw-r--r-- 1 root root 4096 Jan 1 1970 uevent
On x86_64 qemu with one port I have:
# ls -l /sys/devices/pnp0/00:04/
total 0
lrwxrwxrwx 1 root root 0 Feb 1 06:13 driver -> ../../../bus/pnp/drivers/serial
lrwxrwxrwx 1 root root 0 Feb 1 06:13 firmware_node -> ../../LNXSYSTM:00/LNXSYBUS:00/PNP0A03:00/device:01/PNP0501:00
-r--r--r-- 1 root root 4096 Feb 1 06:13 id
-r--r--r-- 1 root root 4096 Feb 1 06:13 options
-rw-r--r-- 1 root root 4096 Feb 1 06:13 resources
drwxr-xr-x 3 root root 0 Feb 1 06:12 serial-ctrl.0.auto
lrwxrwxrwx 1 root root 0 Feb 1 06:13 subsystem -> ../../../bus/pnp
drwxr-xr-x 3 root root 0 Feb 1 06:12 tty
-rw-r--r-- 1 root root 4096 Feb 1 06:12 uevent
# ls -l /sys/devices/pnp0/00:04/serial-ctrl.0.auto/
total 0
lrwxrwxrwx 1 root root 0 Feb 1 06:13 driver -> ../../../../bus/platform/drivers/serial-ctrl
-rw-r--r-- 1 root root 4096 Feb 1 06:13 driver_override
-r--r--r-- 1 root root 4096 Feb 1 06:13 modalias
drwxr-xr-x 2 root root 0 Feb 1 06:12 serial-port.1.auto
lrwxrwxrwx 1 root root 0 Feb 1 06:13 subsystem -> ../../../../bus/platform
-rw-r--r-- 1 root root 4096 Feb 1 06:12 uevent
On Mon, Jan 16, 2023 at 09:59:58AM +0200, 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 do this, let's set up a 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.
A side note. Perhaps it makes sense to also clean up documentation somehow
related to this change. For example, I found that
Documentation/firmware-guide/acpi/enumeration.rst has this:
"Note that standard UARTs are not busses so there is no struct uart_device,
although some of them may be represented by struct serdev_device."
--
With Best Regards,
Andy Shevchenko
* Andy Shevchenko <[email protected]> [230302 16:07]:
> On Mon, Jan 16, 2023 at 09:59:58AM +0200, 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 do this, let's set up a 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.
>
> A side note. Perhaps it makes sense to also clean up documentation somehow
> related to this change. For example, I found that
> Documentation/firmware-guide/acpi/enumeration.rst has this:
>
> "Note that standard UARTs are not busses so there is no struct uart_device,
> although some of them may be represented by struct serdev_device."
OK good point, will update that for the next version.
FYI, I replaced the serial core platform bus with just struct device and
bus, need to clean-up a bit before posting though.
Regards,
Tony
On Wed, Feb 01, 2023 at 08:38:42AM +0200, Tony Lindgren wrote:
> * Greg Kroah-Hartman <[email protected]> [230131 10:10]:
> > On Mon, Jan 16, 2023 at 09:59:58AM +0200, 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 do this, let's set up a 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.
> >
> > Looking better, but why is this new device a platform device? That
> > feels odd, you should never have a platform device hanging off of a
> > non-platform device, right?
>
> No special need for it to be a platform device. It just is easy to set
> up, and for my test case the serial port physical device is also a
> platform device.
>
> What's your preference here?
Never make up a "fake" platform device please. Only use them for real
platform devices. Use a virtual device if you want a virtual one.
thanks,
greg k-h