Re: [PATCH v12 1/1] serial: core: Start managing serial controllers to enable runtime PM

[Tony Lindgren <tony@atomide.com>]

* Maximilian Luz <luzmaximilian@gmail.com> [231003 22:09]:
> On 10/3/23 14:21, Tony Lindgren wrote:
> > * Tony Lindgren <tony@atomide.com> [231003 12:15]:
> > > Hi,
> > > 
> > > * Maximilian Luz <luzmaximilian@gmail.com> [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