| Message ID | 20210921103346.64824-5-tony@atomide.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Get rid of pm_runtime_irq_safe() for 8250_omap | expand |
On Tue, Sep 21, 2021 at 01:33:44PM +0300, Tony Lindgren wrote: > We can use the prep_tx() call to wake up an idle serial port. This allows > ust to remove the depedency to pm_runtime_irq_safe() for 8250_omap driver > in the following patches. > > Signed-off-by: Tony Lindgren <tony@atomide.com> > --- > drivers/tty/serial/8250/8250_port.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > 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 > @@ -1650,6 +1650,29 @@ static enum hrtimer_restart serial8250_em485_handle_start_tx(struct hrtimer *t) > return HRTIMER_NORESTART; > } > > +static int serial8250_prep_tx(struct uart_port *port) > +{ > + struct uart_8250_port *up = up_to_u8250p(port); > + struct device *dev = up->port.dev; > + int err; > + > + if (!(up->capabilities & UART_CAP_RPM)) > + return 0; > + > + if (!pm_runtime_suspended(dev)) { > + pm_runtime_mark_last_busy(dev); > + return 0; > + } > + > + err = pm_request_resume(dev); > + if (err < 0) { > + dev_warn(dev, "prep_tx wakeup failed: %d\n", err); > + return err; > + } How is this supposed to work without a runtime PM usage-counter increment? What's to prevent the port from suspending again while it's transmitting? > + > + return -EINPROGRESS; > +} Johan
* Johan Hovold <johan@kernel.org> [210923 12:50]: > On Tue, Sep 21, 2021 at 01:33:44PM +0300, Tony Lindgren wrote: > > +static int serial8250_prep_tx(struct uart_port *port) > > +{ > > + struct uart_8250_port *up = up_to_u8250p(port); > > + struct device *dev = up->port.dev; > > + int err; > > + > > + if (!(up->capabilities & UART_CAP_RPM)) > > + return 0; > > + > > + if (!pm_runtime_suspended(dev)) { > > + pm_runtime_mark_last_busy(dev); > > + return 0; > > + } > > + > > + err = pm_request_resume(dev); > > + if (err < 0) { > > + dev_warn(dev, "prep_tx wakeup failed: %d\n", err); > > + return err; > > + } > > How is this supposed to work without a runtime PM usage-counter > increment? What's to prevent the port from suspending again while it's > transmitting? Hmm yeah we should at pm_runtime_get() and pm_runtime_put() to write() unless serial8250_rpm_get() and serial8250_rpm_put() are doing it. Or pair prep with finish and deal with the usage count there. Regards, Tony
On Thu, Sep 23, 2021 at 06:05:36PM +0300, Tony Lindgren wrote: > * Johan Hovold <johan@kernel.org> [210923 12:50]: > > On Tue, Sep 21, 2021 at 01:33:44PM +0300, Tony Lindgren wrote: > > > +static int serial8250_prep_tx(struct uart_port *port) > > > +{ > > > + struct uart_8250_port *up = up_to_u8250p(port); > > > + struct device *dev = up->port.dev; > > > + int err; > > > + > > > + if (!(up->capabilities & UART_CAP_RPM)) > > > + return 0; > > > + > > > + if (!pm_runtime_suspended(dev)) { > > > + pm_runtime_mark_last_busy(dev); > > > + return 0; > > > + } > > > + > > > + err = pm_request_resume(dev); > > > + if (err < 0) { > > > + dev_warn(dev, "prep_tx wakeup failed: %d\n", err); > > > + return err; > > > + } > > > > How is this supposed to work without a runtime PM usage-counter > > increment? What's to prevent the port from suspending again while it's > > transmitting? > > Hmm yeah we should at pm_runtime_get() and pm_runtime_put() to write() > unless serial8250_rpm_get() and serial8250_rpm_put() are doing it. If you do the put after just buffering the data it doesn't really solve anything. > Or pair prep with finish and deal with the usage count there. Problem is where to call it from. How do you tell the device is done transmitting? And how should we deal with flow control? Etc. Johan
* Johan Hovold <johan@kernel.org> [210924 14:44]: > On Thu, Sep 23, 2021 at 06:05:36PM +0300, Tony Lindgren wrote: > > * Johan Hovold <johan@kernel.org> [210923 12:50]: > > > On Tue, Sep 21, 2021 at 01:33:44PM +0300, Tony Lindgren wrote: > > > > +static int serial8250_prep_tx(struct uart_port *port) > > > > +{ > > > > + struct uart_8250_port *up = up_to_u8250p(port); > > > > + struct device *dev = up->port.dev; > > > > + int err; > > > > + > > > > + if (!(up->capabilities & UART_CAP_RPM)) > > > > + return 0; > > > > + > > > > + if (!pm_runtime_suspended(dev)) { > > > > + pm_runtime_mark_last_busy(dev); > > > > + return 0; > > > > + } > > > > + > > > > + err = pm_request_resume(dev); > > > > + if (err < 0) { > > > > + dev_warn(dev, "prep_tx wakeup failed: %d\n", err); > > > > + return err; > > > > + } > > > > > > How is this supposed to work without a runtime PM usage-counter > > > increment? What's to prevent the port from suspending again while it's > > > transmitting? > > > > Hmm yeah we should at pm_runtime_get() and pm_runtime_put() to write() > > unless serial8250_rpm_get() and serial8250_rpm_put() are doing it. > > If you do the put after just buffering the data it doesn't really solve > anything. Right, sounds like we currently rely on the autosuspend_timeout there. > > Or pair prep with finish and deal with the usage count there. > > Problem is where to call it from. How do you tell the device is done > transmitting? And how should we deal with flow control? Etc. Maybe if the device driver needs to call uart_start() also from runtime PM idle function and if no data allow suspend. Then if there is more data, uart_write() calls uart_start() again, device wakes up and so on. Regards, Tony
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 @@ -1650,6 +1650,29 @@ static enum hrtimer_restart serial8250_em485_handle_start_tx(struct hrtimer *t) return HRTIMER_NORESTART; } +static int serial8250_prep_tx(struct uart_port *port) +{ + struct uart_8250_port *up = up_to_u8250p(port); + struct device *dev = up->port.dev; + int err; + + if (!(up->capabilities & UART_CAP_RPM)) + return 0; + + if (!pm_runtime_suspended(dev)) { + pm_runtime_mark_last_busy(dev); + return 0; + } + + err = pm_request_resume(dev); + if (err < 0) { + dev_warn(dev, "prep_tx wakeup failed: %d\n", err); + return err; + } + + return -EINPROGRESS; +} + static void serial8250_start_tx(struct uart_port *port) { struct uart_8250_port *up = up_to_u8250p(port); @@ -3227,6 +3250,7 @@ static const struct uart_ops serial8250_pops = { .set_mctrl = serial8250_set_mctrl, .get_mctrl = serial8250_get_mctrl, .stop_tx = serial8250_stop_tx, + .prep_tx = serial8250_prep_tx, .start_tx = serial8250_start_tx, .throttle = serial8250_throttle, .unthrottle = serial8250_unthrottle,
We can use the prep_tx() call to wake up an idle serial port. This allows ust to remove the depedency to pm_runtime_irq_safe() for 8250_omap driver in the following patches. Signed-off-by: Tony Lindgren <tony@atomide.com> --- drivers/tty/serial/8250/8250_port.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)