Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Subject: Re: [PATCH] clk: Remove global clk traversal on fetch parent index
Cc:     linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org,
        Derek Basehore <dbasehore@chromium.org>
To:     Derek Basehore <dbasehore@chromium.org>, mturquette@baylibre.com
From:   Stephen Boyd <sboyd@kernel.org>
In-Reply-To: <20181221003100.157581-1-dbasehore@chromium.org>
User-Agent: alot/0.8
References: <20181221003100.157581-1-dbasehore@chromium.org>
Message-ID: <154542449501.13075.1518070014652024380@swboyd.mtv.corp.google.com>
Date:   Fri, 21 Dec 2018 12:34:55 -0800
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Quoting Derek Basehore (2018-12-20 16:31:00)
> It's not required to traverse the entire clk tree when the parents
> array contains a NULL value. You already have the parent clk_core
> pointer, so you can just compare the parent->name and parent_names[i]
> pointers.

Ok.

>=20
> In cases where clk names are never registered, this can be
> a substantial power improvement since a mux having an unregistered
> parent name will traverse the clk tree on every set_rate. This can
> happen hundreds of times a second on CPU clks.
>=20
> Change-Id: I85499d2e576249568ff508e424ca8d5009e6e2b1

This should go away.

> Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> ---
>  drivers/clk/clk.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>=20
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index af011974d4ec..57a95c713286 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1516,9 +1516,18 @@ static int clk_fetch_parent_index(struct clk_core =
*core,
>         if (!parent)
>                 return -EINVAL;
> =20
> -       for (i =3D 0; i < core->num_parents; i++)
> -               if (clk_core_get_parent_by_index(core, i) =3D=3D parent)
> +       for (i =3D 0; i < core->num_parents; i++) {
> +               if (core->parents[i] =3D=3D parent)
> +                       return i;
> +
> +               if (core->parents[i])
> +                       continue;
> +
> +               if (!strcmp(parent->name, core->parent_names[i])) {
> +                       core->parents[i] =3D parent;
>                         return i;
> +               }
> +       }
> =20

Wow, this looks really familiar. In fact, take a look at commit
da0f0b2c3ad2 ("clk: Correct lookup logic in clk_fetch_parent_index()")
and you'll see that it's almost the same code. The main difference is
that this patch's clk_fetch_parent_index() doesn't call __clk_lookup()
to get a pointer to the clk it already has. And then in commit
470b5e2f97cf ("clk: simplify clk_fetch_parent_index() function") we
consolidated all the logic to simplify things, but it seems that we
became confused that the result of !strcmp(parent, names[i]) is actually
only true when the strings are equal. That was an invalid
transformation. Ouch. But this is a good find!

So this patch is reverting commit 470b5e2f97cf and then optimizing it to
never call __clk_lookup() because it's useless. I'll need to add a
comment and expand on the commit text here so that it's more obvious
what's going on. Otherwise someone will come by and try to consolidate
again and we'll do this all over again.