Mailing List Archive

On Mon, Jun 5, 2023 at 8:24?PM Tony Lindgren <tony@atomide.com> wrote:
>
> * Chen-Yu Tsai <wenst@chromium.org> [230605 11:34]:
> > On Mon, Jun 5, 2023 at 2:15?PM Tony Lindgren <tony@atomide.com> 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 <wenst@chromium.org> [230605 13:01]:
> On Mon, Jun 5, 2023 at 8:24?PM Tony Lindgren <tony@atomide.com> wrote:
> >
> > * Chen-Yu Tsai <wenst@chromium.org> [230605 11:34]:
> > > On Mon, Jun 5, 2023 at 2:15?PM Tony Lindgren <tony@atomide.com> 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 <tony@atomide.com> wrote:
>
> * Chen-Yu Tsai <wenst@chromium.org> [230605 13:01]:
> > On Mon, Jun 5, 2023 at 8:24?PM Tony Lindgren <tony@atomide.com> wrote:
> > >
> > > * Chen-Yu Tsai <wenst@chromium.org> [230605 11:34]:
> > > > On Mon, Jun 5, 2023 at 2:15?PM Tony Lindgren <tony@atomide.com> 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 <wenst@chromium.org> [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 <tony@atomide.com> wrote:
>
> * Chen-Yu Tsai <wenst@chromium.org> [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 <tony@atomide.com> wrote:
>>
>> * Chen-Yu Tsai <wenst@chromium.org> [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 <tony@atomide.com> 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 <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.

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 <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 :)

Regards,

Tony

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

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 <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

On Wed, Oct 04, 2023 at 09:17:08AM +0300, Tony Lindgren wrote:
> * Maximilian Luz <luzmaximilian@gmail.com> [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 <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)
> {

* Johan Hovold <johan@kernel.org> [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 <johan@kernel.org> [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 <johan@kernel.org> [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 <johan@kernel.org> [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);
>