Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <1619123448-10138-1-git-send-email-skomatineni@nvidia.com> <1619123448-10138-3-git-send-email-skomatineni@nvidia.com>
In-Reply-To: <1619123448-10138-3-git-send-email-skomatineni@nvidia.com>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Fri, 23 Apr 2021 14:22:55 +0200
Message-ID: <CAJZ5v0hb7AES4A2QHhtMo76GbbkH=FnyQb0oEpHOaWWirpJ80A@mail.gmail.com>
Subject: Re: [RFC PATCH 2/4] cpuidle: menu: add idle_time to cpuidle_state
To:     Sowjanya Komatineni <skomatineni@nvidia.com>
Cc:     Sudeep Holla <sudeep.holla@arm.com>,
        Souvik Chakravarty <souvik.chakravarty@arm.com>,
        Thierry Reding <thierry.reding@gmail.com>,
        Mark Rutland <mark.rutland@arm.com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
        Daniel Lezcano <daniel.lezcano@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        Jon Hunter <jonathanh@nvidia.com>, ksitaraman@nvidia.com,
        sanjayc@nvidia.com,
        Linux ARM <linux-arm-kernel@lists.infradead.org>,
        linux-tegra <linux-tegra@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Linux PM <linux-pm@vger.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Apr 22, 2021 at 10:31 PM Sowjanya Komatineni
<skomatineni@nvidia.com> wrote:
>
> Some platforms use separate CPU firmware running in background to
> handle state transitions which may need runtime idle time of the
> corresponding target state from the kernel.

How exactly does this work?

> This patch adds idle_time to cpuidle state to expose to cpuidle driver the
> idle time that the governor menu predicts based on next events and states
> target residency for selecting proper idle state.
>
> CPU idle driver passes this runtime state idle time to TF-A.
>
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> ---
>  drivers/cpuidle/governors/menu.c | 7 ++++++-
>  include/linux/cpuidle.h          | 1 +
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
> index c3aa8d6..0da5bc5 100644
> --- a/drivers/cpuidle/governors/menu.c
> +++ b/drivers/cpuidle/governors/menu.c
> @@ -382,8 +382,10 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
>                          * stuck in the shallow one for too long.
>                          */
>                         if (drv->states[idx].target_residency_ns < TICK_NSEC &&
> -                           s->target_residency_ns <= delta_tick)
> +                           s->target_residency_ns <= delta_tick) {
> +                               drv->states[idx].idle_time = delta_tick / NSEC_PER_USEC;
>                                 idx = i;
> +                       }
>
>                         return idx;
>                 }
> @@ -393,6 +395,7 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
>                 idx = i;
>         }
>
> +       drv->states[idx].idle_time = predicted_ns / NSEC_PER_USEC;
>         if (idx == -1)
>                 idx = 0; /* No states enabled. Must use 0. */
>
> @@ -419,6 +422,8 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
>                                 if (drv->states[i].target_residency_ns <= delta_tick)
>                                         break;
>                         }
> +
> +                       drv->states[idx].idle_time = delta_tick / NSEC_PER_USEC;
>                 }
>         }
>
> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
> index fce4762..12db2e9 100644
> --- a/include/linux/cpuidle.h
> +++ b/include/linux/cpuidle.h
> @@ -55,6 +55,7 @@ struct cpuidle_state {
>         unsigned int    exit_latency; /* in US */
>         int             power_usage; /* in mW */
>         unsigned int    target_residency; /* in US */
> +       unsigned int    idle_time; /* in US */

No way.

This structure holds idle state properties of and not some random data
passed between cpuidle components.  And it is not per-CPU, while the
governors work on the per-CPU basis.

state_usage might be more suitable, but that only if I'm convinced
that this is really necessary.

>
>         int (*enter)    (struct cpuidle_device *dev,
>                         struct cpuidle_driver *drv,
> --