Re: [PATCH v4] serial: 8250-mtk: modify mtk uart power and clock management

[Nicolas Boichat]

On Wed, Feb 26, 2020 at 4:54 PM Changqi Hu <changqi.hu@xxxxxxxxxxxx> wrote:
>
> MTK uart design no need to control uart clock,
> so we just control bus clock in runtime function.
> Add uart clock used count to avoid repeatedly switching the clock.

This patch does a lot more than that:
 - Adds a busy loop in mtk8250_runtime_suspend
 - Changes how you do pm_runtime stuff.

These probably need to be split to different patches, and can you
please describe why you are making those changes in the commit
message?

> Signed-off-by: Changqi Hu <changqi.hu@xxxxxxxxxxxx>
> ---
>
> Changes in v4:
>  Modify commit-message
>
> Changes in v3:
>  Merge patch v1 and v2 together.
>
> Changes in v2:
>  Enable uart bus clock when probe and resume base on v1 patch,
>  but miss v1 patch itself.
>
>  drivers/tty/serial/8250/8250_mtk.c | 50 ++++++++++++++++++++++++--------------
>  1 file changed, 32 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c
> index 4d067f5..f839380 100644
> --- a/drivers/tty/serial/8250/8250_mtk.c
> +++ b/drivers/tty/serial/8250/8250_mtk.c
> @@ -32,6 +32,7 @@
>  #define MTK_UART_RXTRI_AD      0x14    /* RX Trigger address */
>  #define MTK_UART_FRACDIV_L     0x15    /* Fractional divider LSB address */
>  #define MTK_UART_FRACDIV_M     0x16    /* Fractional divider MSB address */
> +#define MTK_UART_DEBUG0        0x18
>  #define MTK_UART_IER_XOFFI     0x20    /* Enable XOFF character interrupt */
>  #define MTK_UART_IER_RTSI      0x40    /* Enable RTS Modem status interrupt */
>  #define MTK_UART_IER_CTSI      0x80    /* Enable CTS Modem status interrupt */
> @@ -388,9 +389,18 @@ mtk8250_set_termios(struct uart_port *port, struct ktermios *termios,
>  static int __maybe_unused mtk8250_runtime_suspend(struct device *dev)
>  {
>         struct mtk8250_data *data = dev_get_drvdata(dev);
> +       struct uart_8250_port *up = serial8250_get_port(data->line);
>
> -       clk_disable_unprepare(data->uart_clk);
> -       clk_disable_unprepare(data->bus_clk);
> +       /* wait until UART in idle status */
> +       while
> +               (serial_in(up, MTK_UART_DEBUG0));

No timeout?

> +
> +       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--;
> +       }

The clock core already does reference counting for you, so I don't
think you need this.
https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk.c#L1004

>
>         return 0;
>  }
> @@ -400,16 +410,16 @@ static int __maybe_unused mtk8250_runtime_resume(struct device *dev)
>         struct mtk8250_data *data = dev_get_drvdata(dev);
>         int err;
>
> -       err = clk_prepare_enable(data->uart_clk);
> -       if (err) {
> -               dev_warn(dev, "Can't enable clock\n");
> -               return err;
> -       }
> -
> -       err = clk_prepare_enable(data->bus_clk);
> -       if (err) {
> -               dev_warn(dev, "Can't enable bus clock\n");
> -               return 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++;
>         }
>
>         return 0;
> @@ -419,12 +429,14 @@ static void
>  mtk8250_do_pm(struct uart_port *port, unsigned int state, unsigned int old)
>  {
>         if (!state)
> -               pm_runtime_get_sync(port->dev);
> +               if (!mtk8250_runtime_resume(port->dev))
> +                       pm_runtime_get_sync(port->dev);
>
>         serial8250_do_pm(port, state, old);
>
>         if (state)
> -               pm_runtime_put_sync_suspend(port->dev);
> +               if (!pm_runtime_put_sync_suspend(port->dev))
> +                       mtk8250_runtime_suspend(port->dev);
>  }
>
>  #ifdef CONFIG_SERIAL_8250_DMA
> @@ -501,6 +513,8 @@ static int mtk8250_probe(struct platform_device *pdev)
>         if (!data)
>                 return -ENOMEM;
>
> +       data->clk_count = 0;
> +
>         if (pdev->dev.of_node) {
>                 err = mtk8250_probe_of(pdev, &uart.port, data);
>                 if (err)
> @@ -533,6 +547,7 @@ 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)
>                 return err;
> @@ -541,9 +556,6 @@ static int mtk8250_probe(struct platform_device *pdev)
>         if (data->line < 0)
>                 return data->line;
>
> -       pm_runtime_set_active(&pdev->dev);
> -       pm_runtime_enable(&pdev->dev);
> -
>         data->rx_wakeup_irq = platform_get_irq_optional(pdev, 1);
>
>         return 0;
> @@ -556,11 +568,13 @@ static int mtk8250_remove(struct platform_device *pdev)
>         pm_runtime_get_sync(&pdev->dev);
>
>         serial8250_unregister_port(data->line);
> -       mtk8250_runtime_suspend(&pdev->dev);
>
>         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;
>  }
>
> --
> 2.6.4