From: Stephen Boyd <sboyd@kernel.org>
To: Jerome Brunet <jbrunet@baylibre.com>,
Michael Turquette <mturquette@baylibre.com>
Cc: linux-kernel@vger.kernel.org, linux-clk@vger.kernel.org,
Miquel Raynal <miquel.raynal@bootlin.com>,
Russell King <linux@armlinux.org.uk>
Subject: Re: [PATCH 2/9] clk: Introduce get_parent_hw clk op
Date: Tue, 29 Jan 2019 13:15:44 -0800 [thread overview]
Message-ID: <154879654428.136743.10048771201181501034@swboyd.mtv.corp.google.com> (raw)
In-Reply-To: <e303573aefeaf6a7e3a4b1d820e801354ff900b4.camel@baylibre.com>
Quoting Jerome Brunet (2019-01-29 01:34:38)
> On Mon, 2019-01-28 at 22:10 -0800, Stephen Boyd wrote:
> > ---
> > drivers/clk/clk.c | 117 ++++++++++++++++++++++++++---------
> > include/linux/clk-provider.h | 9 +++
> > 2 files changed, 96 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 01b36f0851bd..5d82cf25bb29 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -2242,14 +2242,84 @@ struct clk *clk_get_parent(struct clk *clk)
> > }
> > EXPORT_SYMBOL_GPL(clk_get_parent);
> >
> > -static struct clk_core *__clk_init_parent(struct clk_core *core)
> > +static struct clk_core *
> > +__clk_init_parent(struct clk_core *core, bool update_orphan)
> > {
> > u8 index = 0;
> > + struct clk_hw *parent_hw = NULL;
> >
> > - if (core->num_parents > 1 && core->ops->get_parent)
> > - index = core->ops->get_parent(core->hw);
> > + if (core->ops->get_parent_hw) {
> > + parent_hw = core->ops->get_parent_hw(core->hw);
> > + /*
> > + * The provider driver doesn't know what the parent is,
> > + * but it's at least something valid, so it's not an
> > + * orphan, just a clk with some unknown parent.
> > + */
>
> I suppose this is the answer the discussion we had last year. I'm not sure it
> answer the problem. In the case I presented, we have no idea wether the
> setting is valid or not.
>
> We can't assume it is `at least something valid`, the value in the mux is just
> something we can't map.
So if you can't map the value in the mux how is that valid? I would
think the mux knows what indexes it has strings for, and if the index
isn't in there it's invalid. Is that not the case here?
>
> Aslo, could you provide an example of what such callback would be, with clk-
> mux maybe ?
Sounds fair. I can convert the clk-mux API to this op. It may be that we
need to make clk_hw_get_parent_by_index() return an error pointer
instead of NULL if it can't find the clk so that we can move the error
codes through this new API.
>
> I don't get how a clock driver will keep track of the clk_hw pointers it is
> connected to. Is there an API for this ? clk-mux must access to clk_core to
> explore his own parent ... which already does not scale well, expect if we
> plan to expose clk_core at some point ?
No we don't want to expose clk_core to provider drivers. It is only for
the use of the clk framework and it's not exposed even as an opaque
pointer. We have that core member of clk_hw but that's just to traverse
from clk_hw to clk_core, and not for anything else.
>
> > + if (!parent_hw && update_orphan)
> > + core->orphan = false;
> > + } else {
> > + if (core->num_parents > 1 && core->ops->get_parent)
>
> I still get why, when num_parents == 1, it is OK to call get_parent_hw() and
> no get_parent(). It does not seems coherent.
I'd rather not change behavior of existing code in this patch, so I took
the route of adding another callback with semantics that we can define
now because there aren't any users. The difference between the two is
made intentionally.
>
> > + index = core->ops->get_parent(core->hw);
> > +
> > + parent_hw = clk_hw_get_parent_by_index(core->hw, index);
> > + }
> > +
> > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> > index 60c51871b04b..8b84dee942bf 100644
> > --- a/include/linux/clk-provider.h
> > +++ b/include/linux/clk-provider.h
> > @@ -155,6 +155,14 @@ struct clk_duty {
> > * multiple parents. It is optional (and unnecessary) for clocks
> > * with 0 or 1 parents.
> > *
> > + * @get_parent_hw: Queries the hardware to determine the parent of a
> > clock. The
> > + * return value is a clk_hw pointer corresponding to
> > + * the parent clock. In short, this function translates the
> > parent
> > + * value read from hardware into a pointer to the clk_hw for that
> > clk.
> > + * Currently only called when the clock is initialized by
> > + * __clk_init. This callback is mandatory for clocks with
> > + * multiple parents. It is optional for clocks with 0 or 1
> > parents.
> > + *
>
> The comments above could imply that get_parent() and get_parent_hw() are both
> mandatory if num_parent > 1. (I don't think so but) Is this your intent ?
It is not the intent. I'll update the docs. Thanks.
next prev parent reply other threads:[~2019-01-29 21:15 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-01-29 6:10 [PATCH 0/9] Rewrite clk parent handling Stephen Boyd
2019-01-29 6:10 ` [PATCH 1/9] clk: Combine __clk_get() and __clk_create_clk() Stephen Boyd
2019-01-29 6:10 ` [PATCH 2/9] clk: Introduce get_parent_hw clk op Stephen Boyd
2019-01-29 9:34 ` Jerome Brunet
2019-01-29 21:15 ` Stephen Boyd [this message]
2019-01-30 9:53 ` Jerome Brunet
2019-01-30 21:30 ` Stephen Boyd
2019-01-31 18:40 ` Jerome Brunet
2019-02-06 0:01 ` Stephen Boyd
2019-02-13 9:16 ` Jerome Brunet
2019-02-15 17:01 ` Stephen Boyd
2019-02-11 16:09 ` Jeffrey Hugo
2019-02-15 18:47 ` Stephen Boyd
2019-02-15 19:29 ` Jeffrey Hugo
2019-02-15 21:29 ` Stephen Boyd
2019-02-15 21:34 ` Jeffrey Hugo
2019-01-29 6:10 ` [PATCH 3/9] clk: core: clarify the check for runtime PM Stephen Boyd
2019-01-29 6:10 ` [PATCH 4/9] clk: Introduce of_clk_get_hw_from_clkspec() Stephen Boyd
2019-01-29 6:10 ` [PATCH 5/9] clk: Inform the core about consumer devices Stephen Boyd
2019-01-29 6:10 ` [PATCH 6/9] clk: Move of_clk_*() APIs into clk.c from clkdev.c Stephen Boyd
2019-01-29 6:10 ` [PATCH 7/9] clk: Allow parents to be specified without string names Stephen Boyd
2019-01-29 10:12 ` Jerome Brunet
2019-01-29 18:56 ` Stephen Boyd
2019-01-29 21:08 ` Jerome Brunet
2019-02-13 9:32 ` Jerome Brunet
2019-02-15 21:13 ` Stephen Boyd
2019-01-29 6:10 ` [PATCH 8/9] clk: qcom: gcc-sdm845: Migrate to DT parent mapping Stephen Boyd
2019-01-29 6:10 ` [PATCH 9/9] arm64: dts: qcom: Specify XO clk as input to GCC node Stephen Boyd
2019-01-29 10:12 ` [PATCH 0/9] Rewrite clk parent handling Miquel Raynal
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=154879654428.136743.10048771201181501034@swboyd.mtv.corp.google.com \
--to=sboyd@kernel.org \
--cc=jbrunet@baylibre.com \
--cc=linux-clk@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@armlinux.org.uk \
--cc=miquel.raynal@bootlin.com \
--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).