Re: [PATCH 2/9] clk: Introduce get_parent_hw clk op

[Stephen Boyd]

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.