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: stable@vger.kernel.org # 5.9 Cc: Rajendra Nayak quic_rjendra@quicinc.com Cc: Matthias Kaehlcke mka@chromium.org Signed-off-by: Johan Hovold johan+linaro@kernel.org --- 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 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: stable@vger.kernel.org # 5.9 Cc: Rajendra Nayak quic_rjendra@quicinc.com Cc: Matthias Kaehlcke mka@chromium.org Signed-off-by: Johan Hovold johan+linaro@kernel.org
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)
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, port->clk_rate);
geni_icc_disable(&port->se); }dev_pm_opp_set_rate(uport->dev, 0);
}
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: stable@vger.kernel.org # 5.9 Cc: Rajendra Nayak quic_rjendra@quicinc.com Cc: Matthias Kaehlcke mka@chromium.org Signed-off-by: Johan Hovold johan+linaro@kernel.org
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: stable@vger.kernel.org # 5.9 Cc: Rajendra Nayak quic_rjendra@quicinc.com Cc: Matthias Kaehlcke mka@chromium.org Signed-off-by: Johan Hovold johan+linaro@kernel.org
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 konrad.dybcio@linaro.org
Konrad
Johan
linux-stable-mirror@lists.linaro.org