Messages in this thread | | | Date | Wed, 4 Oct 2023 09:39:17 +0200 | Subject | Re: [PATCH v12 1/1] serial: core: Start managing serial controllers to enable runtime PM | From | Maximilian Luz <> |
| |
On 10/4/23 08:17, Tony Lindgren wrote: > * 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?
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) > {
| |