Il 06/06/23 11:17, Chen-Yu Tsai ha scritto:
The 8250_mtk driver's runtime PM support has some issues:
- The bus clock is enabled (through runtime PM callback) later than a
register write
- runtime PM resume callback directly called in probe, but no
pm_runtime_set_active() call is present
- UART PM function calls the callbacks directly, _and_ calls runtime
PM API
- runtime PM callbacks try to do reference counting, adding yet another
count between runtime PM and clocks
This fragile setup worked in a way, but broke recently with runtime PM
support added to the serial core. The system would hang when the UART
console was probed and brought up.
Tony provided some potential fixes [1][2], though they were still a bit
complicated. The 8250_dw driver, which the 8250_mtk driver might have
been based on, has a similar structure but simpler runtime PM usage.
Simplify clock sequencing and runtime PM support in the 8250_mtk driver.
Specifically, the clock is acquired enabled and assumed to be active,
unless toggled through runtime PM suspend/resume. Reference counting is
removed and left to the runtime PM core. The serial pm function now
only calls the runtime PM API.
[1] https://lore.kernel.org/linux-serial/20230602092701.GP14287@xxxxxxxxxxx/
[2] https://lore.kernel.org/linux-serial/20230605061511.GW14287@xxxxxxxxxxx/
Fixes: 84a9582fd203 ("serial: core: Start managing serial controllers to enable runtime PM")
Suggested-by: Tony Lindgren <tony@xxxxxxxxxxx>
Signed-off-by: Chen-Yu Tsai <wenst@xxxxxxxxxxxx>
You're both cleaning this up and solving a critical issue and I completely agree
about doing that.
I can imagine what actually fixes the driver, but still, is it possible to split
this commit in two?
One that solves the issue, one that performs the much needed cleanups.
If it's not possible, then we can leave this commit as it is... and if the problem
about splitting is the Fixes tag... well, we don't forcefully need it: after all,
issues started arising after runtime PM support for 8250 landed and before that the
driver technically worked, even though it was fragile.
Thanks,
Angelo
---
drivers/tty/serial/8250/8250_mtk.c | 50 ++++++------------------------
1 file changed, 10 insertions(+), 40 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c
index aa8e98164d68..74da5676ce67 100644
--- a/drivers/tty/serial/8250/8250_mtk.c
+++ b/drivers/tty/serial/8250/8250_mtk.c
@@ -431,12 +431,7 @@ static int __maybe_unused mtk8250_runtime_suspend(struct device *dev)
while
(serial_in(up, MTK_UART_DEBUG0));
- if (data->clk_count == 0U) {
- dev_dbg(dev, "%s clock count is 0\n", __func__);
- } else {
- clk_disable_unprepare(data->bus_clk);
- data->clk_count--;
- }
+ clk_disable_unprepare(data->bus_clk);
return 0;
}
@@ -444,19 +439,8 @@ static int __maybe_unused mtk8250_runtime_suspend(struct device *dev)
static int __maybe_unused mtk8250_runtime_resume(struct device *dev)
{
struct mtk8250_data *data = dev_get_drvdata(dev);
- int err;
- if (data->clk_count > 0U) {
- dev_dbg(dev, "%s clock count is %d\n", __func__,
- data->clk_count);
- } else {
- err = clk_prepare_enable(data->bus_clk);
- if (err) {
- dev_warn(dev, "Can't enable bus clock\n");
- return err;
- }
- data->clk_count++;
- }
+ clk_prepare_enable(data->bus_clk);
return 0;
}
@@ -465,14 +449,12 @@ static void
mtk8250_do_pm(struct uart_port *port, unsigned int state, unsigned int old)
{
if (!state)
- if (!mtk8250_runtime_resume(port->dev))
- pm_runtime_get_sync(port->dev);
+ pm_runtime_get_sync(port->dev);
serial8250_do_pm(port, state, old);
if (state)
- if (!pm_runtime_put_sync_suspend(port->dev))
- mtk8250_runtime_suspend(port->dev);
+ pm_runtime_put_sync_suspend(port->dev);
}
#ifdef CONFIG_SERIAL_8250_DMA
@@ -504,7 +486,7 @@ static int mtk8250_probe_of(struct platform_device *pdev, struct uart_port *p,
return 0;
}
- data->bus_clk = devm_clk_get(&pdev->dev, "bus");
+ data->bus_clk = devm_clk_get_enabled(&pdev->dev, "bus");
if (IS_ERR(data->bus_clk))
return PTR_ERR(data->bus_clk);
@@ -587,25 +569,16 @@ static int mtk8250_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, data);
- pm_runtime_enable(&pdev->dev);
- err = mtk8250_runtime_resume(&pdev->dev);
- if (err)
- goto err_pm_disable;
-
data->line = serial8250_register_8250_port(&uart);
- if (data->line < 0) {
- err = data->line;
- goto err_pm_disable;
- }
+ if (data->line < 0)
+ return data->line;
data->rx_wakeup_irq = platform_get_irq_optional(pdev, 1);
- return 0;
-
-err_pm_disable:
- pm_runtime_disable(&pdev->dev);
+ pm_runtime_set_active(&pdev->dev);
+ pm_runtime_enable(&pdev->dev);
- return err;
+ return 0;
}
static int mtk8250_remove(struct platform_device *pdev)
@@ -619,9 +592,6 @@ static int mtk8250_remove(struct platform_device *pdev)
pm_runtime_disable(&pdev->dev);
pm_runtime_put_noidle(&pdev->dev);
- if (!pm_runtime_status_suspended(&pdev->dev))
- mtk8250_runtime_suspend(&pdev->dev);
-
return 0;
}
