From: Stephen Boyd <sboyd@kernel.org>
To: Alexandre Mergnat <amergnat@baylibre.com>,
Jerome Brunet <jbrunet@baylibre.com>,
mturquette@baylibre.com
Cc: linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org,
baylibre-upstreaming@groups.io
Subject: Re: [PATCH] clk: fix clock global name usage.
Date: Fri, 24 May 2019 10:44:53 -0700 [thread overview]
Message-ID: <20190524174454.8043420879@mail.kernel.org> (raw)
In-Reply-To: <c89ecb6f328014ce22ae5d6c634e5337dbbf3ea2.camel@baylibre.com>
Quoting Jerome Brunet (2019-05-24 08:00:08)
> On Fri, 2019-05-24 at 07:33 -0700, Stephen Boyd wrote:
> > Do you set the index to 0 in this clk's parent_data? We purposefully set
> > the index to -1 in clk_core_populate_parent_map() so that the fw_name
> > can be NULL but the index can be something >= 0 and then we'll use that
> > to lookup the clk from DT. We need to support that combination.
> >
> > fw_name | index | DT lookup?
> > ----------+---------+------------
> > NULL | >= 0 | Y
> > NULL | -1 | N
> > non-NULL | -1 | ?
> > non-NULL | >= 0 | Y
> >
> > Maybe we should support the ? case, because right now it will fail to do
> > the DT lookup when the index is -1.
>
> Hi Stephen,
>
> We are trying to migrate all meson clocks to the new parent structure.
> There is a little quirk which forces us to continue to use legacy names
> for a couple of clocks.
>
> We heavily use static data which init everything to 0.
> Here is an example:
>
> static struct clk_regmap g12a_aoclk_cts_rtc_oscin = {
> [...]
> .hw.init = &(struct clk_init_data){
> .name = "g12a_ao_cts_rtc_oscin",
> .ops = &clk_regmap_mux_ops,
> - .parent_names = (const char *[]){ "g12a_ao_32k_by_oscin",
> - IN_PREFIX "ext_32k-0" },
> + .parent_data = (const struct clk_parent_data []) {
> + { .name = "g12a_ao_32k_by_oscin" },
> + { .fw_name = "ext-32k-0", },
> + },
> .num_parents = 2,
> .flags = CLK_SET_RATE_PARENT,
> },
> };
>
> With this, instead of taking name = "g12a_ao_32k_by_oscin" for entry #0
> it takes DT names at index 0 which is not what we intended.
>
> If I understand correctly we should put
> + { .name = "g12a_ao_32k_by_oscin", index = -1, },
>
> And would be alright ?
I don't understand why this wouldn't have a .fw_name or an .index >= 0,
or both. Is there some reason why that isn't happening?
>
> While I understand it, it is not very obvious or nice to look at.
> Plus it is a bit weird that this -1 is required for .name and not .hw.
Sure. It can be better documented. Sorry it's not super obvious. I added
this later in the series. We could have:
#define CLK_SKIP_FW_LOOKUP .index = -1
and then this would read as:
{ .name = "g12a_ao_32k_by_oscin", CLK_SKIP_FW_LOOKUP },
>
> Do you think we could come up with a priority order which makes the first
> example work ?
Maybe? I'm open to suggestions.
>
> Something like:
>
> if (hw) {
> /* use pointer */
> } else if (name) {
> /* use legacy global names */
I don't imagine we can get rid of legacy name for a long time, so this
can't be in this order. Otherwise we'll try to lookup the legacy name
before trying the DT lookup and suffer performance hits doing a big
global search while also skipping the DT lookup that we want drivers to
use if they're more modern.
> } else if (fw_name) {
> /* use DT names */
> } else if (index >= 0)
> /* use DT index */
> } else {
> return -EINVAL;
> }
>
> The last 2 clause could be removed if we make index an unsigned.
>
So just assign -1 to .index? I still think my patch may be needed if
somehow the index is assigned to something less than 0 and the .fw_name
is specified. I guess that's possible if the struct is on the stack, so
we'll probably have to allow this combination.
next prev parent reply other threads:[~2019-05-24 17:44 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-05-24 7:27 [PATCH] clk: fix clock global name usage Alexandre Mergnat
2019-05-24 14:33 ` Stephen Boyd
2019-05-24 15:00 ` Jerome Brunet
2019-05-24 17:44 ` Stephen Boyd [this message]
2019-05-24 18:12 ` Jerome Brunet
2019-06-06 22:54 ` Stephen Boyd
2019-06-10 9:37 ` Jerome Brunet
2019-06-11 6:46 ` wens Tsai
2019-06-12 23:00 ` Stephen Boyd
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190524174454.8043420879@mail.kernel.org \
--to=sboyd@kernel.org \
--cc=amergnat@baylibre.com \
--cc=baylibre-upstreaming@groups.io \
--cc=jbrunet@baylibre.com \
--cc=linux-clk@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mturquette@baylibre.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).