This series fixes the missing opp vote removal when closing a serial
port, which, for example, can lead to wasted power when a UART connected
Bluetooth controller is not in use.
Included is also a clean up of a related debug printk.
Johan
Johan Hovold (2):
serial: qcom-geni: fix opp vote on shutdown
serial: qcom-geni: clean up clock-rate debug printk
drivers/tty/serial/qcom_geni_serial.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
--
2.41.0
The operating-performance-point vote needs to be dropped when shutting
down the port to avoid wasting power by keeping resources like power
domains in an unnecessarily high performance state (e.g. when a UART
connected Bluetooth controller is not in use).
Fixes: a5819b548af0 ("tty: serial: qcom_geni_serial: Use OPP API to set clk/perf state")
Cc: [email protected] # 5.9
Cc: Rajendra Nayak <[email protected]>
Cc: Matthias Kaehlcke <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/tty/serial/qcom_geni_serial.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index b825b05e6137..8be896dbaa88 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -126,6 +126,7 @@ struct qcom_geni_serial_port {
dma_addr_t rx_dma_addr;
bool setup;
unsigned int baud;
+ unsigned long clk_rate;
void *rx_buf;
u32 loopback;
bool brk;
@@ -1249,6 +1250,7 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
baud * sampling_rate, clk_rate, clk_div);
uport->uartclk = clk_rate;
+ port->clk_rate = clk_rate;
dev_pm_opp_set_rate(uport->dev, clk_rate);
ser_clk_cfg = SER_CLK_EN;
ser_clk_cfg |= clk_div << CLK_DIV_SHFT;
@@ -1513,10 +1515,13 @@ static void qcom_geni_serial_pm(struct uart_port *uport,
if (new_state == UART_PM_STATE_ON && old_state == UART_PM_STATE_OFF) {
geni_icc_enable(&port->se);
+ if (port->clk_rate)
+ dev_pm_opp_set_rate(uport->dev, port->clk_rate);
geni_se_resources_on(&port->se);
} else if (new_state == UART_PM_STATE_OFF &&
old_state == UART_PM_STATE_ON) {
geni_se_resources_off(&port->se);
+ dev_pm_opp_set_rate(uport->dev, 0);
geni_icc_disable(&port->se);
}
}
--
2.41.0
Make the clock-rate debug printk more readable by using an equal sign
instead of a dash as separator between names and values and adding some
spaces:
qcom_geni_serial 988000.serial: desired_rate = 1843200, clk_rate = 7372800, clk_div = 4
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/tty/serial/qcom_geni_serial.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 8be896dbaa88..753ac68d03a6 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -1246,7 +1246,7 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
goto out_restart_rx;
}
- dev_dbg(port->se.dev, "desired_rate-%u, clk_rate-%lu, clk_div-%u\n",
+ dev_dbg(port->se.dev, "desired_rate = %u, clk_rate = %lu, clk_div = %u\n",
baud * sampling_rate, clk_rate, clk_div);
uport->uartclk = clk_rate;
--
2.41.0
On 14.07.2023 15:02, Johan Hovold wrote:
> Make the clock-rate debug printk more readable by using an equal sign
> instead of a dash as separator between names and values and adding some
> spaces:
>
> qcom_geni_serial 988000.serial: desired_rate = 1843200, clk_rate = 7372800, clk_div = 4
>
> Signed-off-by: Johan Hovold <[email protected]>
> ---
Reviewed-by: Konrad Dybcio <[email protected]>
Konrad
> drivers/tty/serial/qcom_geni_serial.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 8be896dbaa88..753ac68d03a6 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -1246,7 +1246,7 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
> goto out_restart_rx;
> }
>
> - dev_dbg(port->se.dev, "desired_rate-%u, clk_rate-%lu, clk_div-%u\n",
> + dev_dbg(port->se.dev, "desired_rate = %u, clk_rate = %lu, clk_div = %u\n",
> baud * sampling_rate, clk_rate, clk_div);
>
> uport->uartclk = clk_rate;
On 14.07.2023 15:02, Johan Hovold wrote:
> The operating-performance-point vote needs to be dropped when shutting
> down the port to avoid wasting power by keeping resources like power
> domains in an unnecessarily high performance state (e.g. when a UART
> connected Bluetooth controller is not in use).
>
> Fixes: a5819b548af0 ("tty: serial: qcom_geni_serial: Use OPP API to set clk/perf state")
> Cc: [email protected] # 5.9
> Cc: Rajendra Nayak <[email protected]>
> Cc: Matthias Kaehlcke <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>
> ---
I don't know a whole lot about this subsystem, but the PM call has
a pointer to uport which already contains this clock rate.. Is it
zeroed out by the core before we reach it, which would prevent us
from reusing it?
Konrad
> drivers/tty/serial/qcom_geni_serial.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index b825b05e6137..8be896dbaa88 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -126,6 +126,7 @@ struct qcom_geni_serial_port {
> dma_addr_t rx_dma_addr;
> bool setup;
> unsigned int baud;
> + unsigned long clk_rate;
> void *rx_buf;
> u32 loopback;
> bool brk;
> @@ -1249,6 +1250,7 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
> baud * sampling_rate, clk_rate, clk_div);
>
> uport->uartclk = clk_rate;
> + port->clk_rate = clk_rate;
> dev_pm_opp_set_rate(uport->dev, clk_rate);
> ser_clk_cfg = SER_CLK_EN;
> ser_clk_cfg |= clk_div << CLK_DIV_SHFT;
> @@ -1513,10 +1515,13 @@ static void qcom_geni_serial_pm(struct uart_port *uport,
>
> if (new_state == UART_PM_STATE_ON && old_state == UART_PM_STATE_OFF) {
> geni_icc_enable(&port->se);
> + if (port->clk_rate)
> + dev_pm_opp_set_rate(uport->dev, port->clk_rate);
> geni_se_resources_on(&port->se);
> } else if (new_state == UART_PM_STATE_OFF &&
> old_state == UART_PM_STATE_ON) {
> geni_se_resources_off(&port->se);
> + dev_pm_opp_set_rate(uport->dev, 0);
> geni_icc_disable(&port->se);
> }
> }
On Fri, Jul 14, 2023 at 04:29:08PM +0200, Konrad Dybcio wrote:
> On 14.07.2023 15:02, Johan Hovold wrote:
> > The operating-performance-point vote needs to be dropped when shutting
> > down the port to avoid wasting power by keeping resources like power
> > domains in an unnecessarily high performance state (e.g. when a UART
> > connected Bluetooth controller is not in use).
> >
> > Fixes: a5819b548af0 ("tty: serial: qcom_geni_serial: Use OPP API to set clk/perf state")
> > Cc: [email protected] # 5.9
> > Cc: Rajendra Nayak <[email protected]>
> > Cc: Matthias Kaehlcke <[email protected]>
> > Signed-off-by: Johan Hovold <[email protected]>
> > ---
> I don't know a whole lot about this subsystem, but the PM call has
> a pointer to uport which already contains this clock rate.. Is it
> zeroed out by the core before we reach it, which would prevent us
> from reusing it?
No, but this driver has other issues and I couldn't be arsed fixing them
before addressing this bug.
Specifically that uartclk variable can currently be set by userspace...
I'll fix that up next week.
Johan
On 14.07.2023 17:08, Johan Hovold wrote:
> On Fri, Jul 14, 2023 at 04:29:08PM +0200, Konrad Dybcio wrote:
>> On 14.07.2023 15:02, Johan Hovold wrote:
>>> The operating-performance-point vote needs to be dropped when shutting
>>> down the port to avoid wasting power by keeping resources like power
>>> domains in an unnecessarily high performance state (e.g. when a UART
>>> connected Bluetooth controller is not in use).
>>>
>>> Fixes: a5819b548af0 ("tty: serial: qcom_geni_serial: Use OPP API to set clk/perf state")
>>> Cc: [email protected] # 5.9
>>> Cc: Rajendra Nayak <[email protected]>
>>> Cc: Matthias Kaehlcke <[email protected]>
>>> Signed-off-by: Johan Hovold <[email protected]>
>>> ---
>> I don't know a whole lot about this subsystem, but the PM call has
>> a pointer to uport which already contains this clock rate.. Is it
>> zeroed out by the core before we reach it, which would prevent us
>> from reusing it?
>
> No, but this driver has other issues and I couldn't be arsed fixing them
> before addressing this bug.
>
> Specifically that uartclk variable can currently be set by userspace...
>
> I'll fix that up next week.
OK sounds good
Acked-by: Konrad Dybcio <[email protected]>
Konrad
>
> Johan