From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 49B1EC433F5 for ; Wed, 6 Oct 2021 13:29:36 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 2F92461075 for ; Wed, 6 Oct 2021 13:29:36 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231665AbhJFNb1 (ORCPT ); Wed, 6 Oct 2021 09:31:27 -0400 Received: from smtp-relay-internal-1.canonical.com ([185.125.188.123]:52432 "EHLO smtp-relay-internal-1.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231356AbhJFNb0 (ORCPT ); Wed, 6 Oct 2021 09:31:26 -0400 Received: from mail-lf1-f70.google.com (mail-lf1-f70.google.com [209.85.167.70]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by smtp-relay-internal-1.canonical.com (Postfix) with ESMTPS id E4CDF3FFEF for ; Wed, 6 Oct 2021 13:29:33 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=canonical.com; s=20210705; t=1633526973; bh=Ld6+ZKe0AociGcEEMM9rX6l+gjcfCFCnMNQZNPjE3U4=; h=Subject:To:Cc:References:From:Message-ID:Date:MIME-Version: In-Reply-To:Content-Type; b=ZIRsLvjpyeo3FSwc74TFp+XwgygyWYshy22wr8XWRbNtgRt72OVO1qwVcu3c1L4gy Q23kddGwYqZOBZHB128xLlzRSO5lUvcgWCdHafEnEQAV3V8AlMbpPsXMrecqnbvhmQ pVdyQZe+nUVg/OhSj0y59C/p5xQ5FbumouDiueI9XEKGyWYYGku37sLyncjasJjZYO TU83Dxu4BzyofJ4OfNGPJaV0NVYlA1Bh73sU3jLPkFDlhlVE/xzeSd44QzTDRi3x7V oDzJr63HUsdMohFQ+mR8wjHJ1pkX+ep4uObNuqSsioBFHy5c6l18z+wejF847PkMHI WwvoFdxhsXfbg== Received: by mail-lf1-f70.google.com with SMTP id f32-20020a0565123b2000b003fd19ba9acaso2004219lfv.10 for ; Wed, 06 Oct 2021 06:29:33 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=Ld6+ZKe0AociGcEEMM9rX6l+gjcfCFCnMNQZNPjE3U4=; b=DtvDG0R2/Cj0c00hqbZvrw5rCZsY8fuHRj0l+sXJWA3PLq/r2tEMhVUGFPXv5acSYT Jnva1FqL1O7VzCCwCvXEGsZ4OFGHHGSLv6JXV1V7L1Ybmk61X6hQM0ao3SVMdZROMA4i A4cZcFuSKbZ7zSPCWhyB/MCDtltP8xBqXPiKm1w0fZMtebkuwblDAr4/yAt5M6BQc2P3 SUVEjVHF1bYfbpRN8fDPVd15uJ7lCxtvt2i5viD+idTj8hxDDG6vKhnPss0JTFaQdd9/ 9irB/CPAqn01CFzeLHnRr7/EE2K7T8dDyHNNWVtohAodSMWU8BZ5CkPmf6J1SS45+03H +g/g== X-Gm-Message-State: AOAM530JgVHo2+xqm5VRJsPb+Z1lgn0soY2t+cCo+SBDQBrPg42/pTSW GcS9MicpVjFbhxZJ4lPk755Caub3vsVOwREU2eL1TSHCTv3smWbJXOPsUnhv6QeD5H40XU7m9Di y4qwGF30jvX1xqLvGeCnLd5uRiFCu/EpYYM9kDYr56f9+fKGN X-Received: by 2002:a19:9102:: with SMTP id t2mr4263979lfd.431.1633526973112; Wed, 06 Oct 2021 06:29:33 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwXKn/6Bfm2mm70Ig+2BseuKQIkDzaugyqTqGjoigj0kYItm2g3eUz6fl7aOdDI4FnW9IgceA== X-Received: by 2002:a19:9102:: with SMTP id t2mr4263947lfd.431.1633526972903; Wed, 06 Oct 2021 06:29:32 -0700 (PDT) Received: from [192.168.0.20] (78-11-189-27.static.ip.netia.com.pl. [78.11.189.27]) by smtp.gmail.com with ESMTPSA id a23sm1715544ljb.107.2021.10.06.06.29.31 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 06 Oct 2021 06:29:32 -0700 (PDT) Subject: Re: [PATCH 6/7] tty: serial: samsung_tty: Support runtime PM To: "Rafael J. Wysocki" Cc: Hector Martin , Linux ARM , Marc Zyngier , Rob Herring , Arnd Bergmann , Linus Walleij , Alyssa Rosenzweig , Greg Kroah-Hartman , Mark Kettenis , Philipp Zabel , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , Linux PM , Linux Kernel Mailing List , Linux Samsung SoC , linux-serial@vger.kernel.org References: <20211005155923.173399-1-marcan@marcan.st> <20211005155923.173399-7-marcan@marcan.st> <77ae3bb1-6da5-3ec6-de33-5e5f661b6145@canonical.com> From: Krzysztof Kozlowski Message-ID: Date: Wed, 6 Oct 2021 15:29:31 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.13.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-samsung-soc@vger.kernel.org On 06/10/2021 15:25, Rafael J. Wysocki wrote: > On Wed, Oct 6, 2021 at 9:44 AM Krzysztof Kozlowski > wrote: >> >> On 05/10/2021 17:59, Hector Martin wrote: >>> This allows idle UART devices to be suspended using the standard >>> runtime-PM framework. The logic is modeled after stm32-usart. >>> >>> Signed-off-by: Hector Martin >>> --- >>> drivers/tty/serial/samsung_tty.c | 88 ++++++++++++++++++++------------ >>> 1 file changed, 54 insertions(+), 34 deletions(-) >>> >>> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c >>> index e2f49863e9c2..d68e3341adc6 100644 >>> --- a/drivers/tty/serial/samsung_tty.c >>> +++ b/drivers/tty/serial/samsung_tty.c >>> @@ -40,6 +40,7 @@ >>> #include >>> #include >>> #include >>> +#include >>> #include >>> >>> /* UART name and device definitions */ >>> @@ -1381,31 +1382,49 @@ static void exynos_usi_init(struct uart_port *port) >>> >>> /* power power management control */ >>> >>> -static void s3c24xx_serial_pm(struct uart_port *port, unsigned int level, >>> - unsigned int old) >>> +static int __maybe_unused s3c24xx_serial_runtime_suspend(struct device *dev) >>> { >>> + struct uart_port *port = dev_get_drvdata(dev); >>> struct s3c24xx_uart_port *ourport = to_ourport(port); >>> int timeout = 10000; >>> >>> - ourport->pm_level = level; >>> + while (--timeout && !s3c24xx_serial_txempty_nofifo(port)) >>> + udelay(100); >>> >>> - switch (level) { >>> - case 3: >>> - while (--timeout && !s3c24xx_serial_txempty_nofifo(port)) >>> - udelay(100); >>> + if (!IS_ERR(ourport->baudclk)) >>> + clk_disable_unprepare(ourport->baudclk); >>> >>> - if (!IS_ERR(ourport->baudclk)) >>> - clk_disable_unprepare(ourport->baudclk); >>> + clk_disable_unprepare(ourport->clk); >>> + return 0; >>> +}; >>> >>> - clk_disable_unprepare(ourport->clk); >>> - break; >>> +static int __maybe_unused s3c24xx_serial_runtime_resume(struct device *dev) >>> +{ >>> + struct uart_port *port = dev_get_drvdata(dev); >>> + struct s3c24xx_uart_port *ourport = to_ourport(port); >>> >>> - case 0: >>> - clk_prepare_enable(ourport->clk); >>> + clk_prepare_enable(ourport->clk); >>> >>> - if (!IS_ERR(ourport->baudclk)) >>> - clk_prepare_enable(ourport->baudclk); >>> + if (!IS_ERR(ourport->baudclk)) >>> + clk_prepare_enable(ourport->baudclk); >>> + return 0; >>> +}; >>> + >>> +static void s3c24xx_serial_pm(struct uart_port *port, unsigned int level, >>> + unsigned int old) >>> +{ >>> + struct s3c24xx_uart_port *ourport = to_ourport(port); >>> + >>> + ourport->pm_level = level; >>> >>> + switch (level) { >>> + case UART_PM_STATE_OFF: >>> + pm_runtime_mark_last_busy(port->dev); >>> + pm_runtime_put_sync(port->dev); >>> + break; >>> + >>> + case UART_PM_STATE_ON: >>> + pm_runtime_get_sync(port->dev); >>> exynos_usi_init(port); >>> break; >>> default: >>> @@ -2282,18 +2301,15 @@ static int s3c24xx_serial_probe(struct platform_device *pdev) >>> } >>> } >>> >>> + pm_runtime_get_noresume(&pdev->dev); >>> + pm_runtime_set_active(&pdev->dev); >>> + pm_runtime_enable(&pdev->dev); >>> + >> >> You need to cleanup in error paths (put/disable). >> >>> dev_dbg(&pdev->dev, "%s: adding port\n", __func__); >>> uart_add_one_port(&s3c24xx_uart_drv, &ourport->port); >>> platform_set_drvdata(pdev, &ourport->port); >>> >>> - /* >>> - * Deactivate the clock enabled in s3c24xx_serial_init_port here, >>> - * so that a potential re-enablement through the pm-callback overlaps >>> - * and keeps the clock enabled in this case. >>> - */ >>> - clk_disable_unprepare(ourport->clk); >>> - if (!IS_ERR(ourport->baudclk)) >>> - clk_disable_unprepare(ourport->baudclk); >>> + pm_runtime_put_sync(&pdev->dev); >>> >>> ret = s3c24xx_serial_cpufreq_register(ourport); >>> if (ret < 0) >>> @@ -2309,8 +2325,14 @@ static int s3c24xx_serial_remove(struct platform_device *dev) >>> struct uart_port *port = s3c24xx_dev_to_port(&dev->dev); >>> >>> if (port) { >>> + pm_runtime_get_sync(&dev->dev); >> >> 1. You need to check return status. > > Why? What can be done differently if the resume fails? The answer is connected with the point (2) below. If there were for example register access here, maybe it should be simply skipped to avoid imprecise abort... > >> 2. Why do you need to resume the device here? > > This appears to be to prevent the device from suspending after the given point. Makes sense. > >>> + >>> s3c24xx_serial_cpufreq_deregister(to_ourport(port)); >>> uart_remove_one_port(&s3c24xx_uart_drv, port); >>> + >>> + pm_runtime_disable(&dev->dev); >> >> Why disabling it only if port!=NULL? Can remove() be called if >> platform_set_drvdata() was not? >> >>> + pm_runtime_set_suspended(&dev->dev); >>> + pm_runtime_put_noidle(&dev->dev); >>> } >>> >>> uart_unregister_driver(&s3c24xx_uart_drv); >>> @@ -2319,8 +2341,8 @@ static int s3c24xx_serial_remove(struct platform_device *dev) >>> } >>> >> >> Best regards, >> Krzysztof Best regards, Krzysztof