On Tue, Jun 6, 2023 at 5:36 PM AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxx> wrote: > > 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. The pure fix would look like what Tony posted [1]. However it would add stuff that isn't strictly needed after the cleanup. Doing it in one patch results in less churn. Think of it another way: it's a nice cleanup that just so happens to fix a regression. As for the fixes tag, it's there so other people potentially doing backports of the 8250 runtime PM work can spot this followup fix. ChenYu [1] https://lore.kernel.org/linux-serial/20230605061511.GW14287@xxxxxxxxxxx/ > 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; > > } > > > >
