Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230731064746.2717684-1-peng.fan@oss.nxp.com> <20230731064746.2717684-5-peng.fan@oss.nxp.com>
In-Reply-To: <20230731064746.2717684-5-peng.fan@oss.nxp.com>
From:   Ulf Hansson <ulf.hansson@linaro.org>
Date:   Thu, 10 Aug 2023 16:05:20 +0200
Message-ID: <CAPDyKFqC2=4iv-NfdY1A0oB-1eORJaG9F=T+Q6xCdQX7RH6j+g@mail.gmail.com>
Subject: Re: [PATCH V3 4/8] genpd: imx: scu-pd: do not power off console if no_console_suspend
To:     "Peng Fan (OSS)" <peng.fan@oss.nxp.com>
Cc:     shawnguo@kernel.org, s.hauer@pengutronix.de, kernel@pengutronix.de,
        festevam@gmail.com, linux-imx@nxp.com,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux-pm@vger.kernel.org, Peng Fan <peng.fan@nxp.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, 31 Jul 2023 at 08:43, Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote:
>
> From: Peng Fan <peng.fan@nxp.com>
>
> Do not power off console if no_console_suspend

Perhaps extend this a bit to let the reader understand this is about
leaving the serial device's corresponding PM domain on.

>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>

A comment below - that doesn't need to stop this from being applied though.

> ---
>  drivers/genpd/imx/scu-pd.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/genpd/imx/scu-pd.c b/drivers/genpd/imx/scu-pd.c
> index 08583a10ac62..d69da79d3130 100644
> --- a/drivers/genpd/imx/scu-pd.c
> +++ b/drivers/genpd/imx/scu-pd.c
> @@ -52,6 +52,7 @@
>   */
>
>  #include <dt-bindings/firmware/imx/rsrc.h>
> +#include <linux/console.h>
>  #include <linux/firmware/imx/sci.h>
>  #include <linux/firmware/imx/svc/rm.h>
>  #include <linux/io.h>
> @@ -324,6 +325,10 @@ static int imx_sc_pd_power(struct generic_pm_domain *domain, bool power_on)
>         msg.resource = pd->rsrc;
>         msg.mode = power_on ? IMX_SC_PM_PW_MODE_ON : IMX_SC_PM_PW_MODE_LP;
>
> +       /* keep uart console power on for no_console_suspend */
> +       if (imx_con_rsrc == pd->rsrc && !console_suspend_enabled && !power_on)

As I indicated above, I don't mind this, but I also think this is a
rather generic problem that you are trying to solve here.

In principle, I think it should be the serial driver's responsibility
to check the console_suspend_enabled flag. Based upon that, it should
inform upper layers (genpd) that its device may need to stay powered
on during system suspend. Quite similar to how we deal with system
wakeups. To make this work we could do the following instead of
$subject patch.

1. The serial driver should call device_set_wakeup_path() (the name of
that function is a bit confusing in this regard, but let's discuss
that separately) in its ->suspend() callback.
2. Set the GENPD_FLAG_ACTIVE_WAKEUP (again the name is a bit confusing
in this regard) for the corresponding genpd provider.

In this way, genpd will keep the PM domain powered on during system suspend.

> +               return -EBUSY;
> +
>         ret = imx_scu_call_rpc(pm_ipc_handle, &msg, true);
>         if (ret)
>                 dev_err(&domain->dev, "failed to power %s resource %d ret %d\n",
> --
> 2.37.1
>

Kind regards
Uffe