Message-ID: <154878820913.136743.9860407353339842831@swboyd.mtv.corp.google.com>
Date:   Tue, 29 Jan 2019 10:56:49 -0800
From:   Stephen Boyd <sboyd@...nel.org>
To:     Jerome Brunet <jbrunet@...libre.com>,
        Michael Turquette <mturquette@...libre.com>
Cc:     linux-kernel@...r.kernel.org, linux-clk@...r.kernel.org,
        Miquel Raynal <miquel.raynal@...tlin.com>,
        Russell King <linux@...linux.org.uk>
Subject: Re: [PATCH 7/9] clk: Allow parents to be specified without string names

Quoting Jerome Brunet (2019-01-29 02:12:00)
> On Mon, 2019-01-28 at 22:10 -0800, Stephen Boyd wrote:
> > 
> > Using either one of these new methods is entirely optional. Existing
> > drivers will continue to work, and they can migrate to this new approach
> > as they see fit. Eventually, we'll want to get rid of the 'parent_names'
> > array in struct clk_init_data and use one of these new methods instead.
> 
> This may indeed allow to remove a lot of annoying code.
> 
> However, does this remove the globally unique name string constraints ? Are we
> now allowed to have 2 instances of a driver registering a clock named "foo" ?

Yes it would be allowed to have multiple clocks with the same name but
I'm not trying to solve that exact problem right now. The framework
already complains if that's happening, so drivers need to generate
unique names regardless of this series.

> 
> If this is the case, I wonder:
> * How will it work with debugfs: clock names are used to create the
> directories in there, plus clk_summary will quickly get messy.

Agreed. We should probably prepend the device name to the front of the
clk now when creating in debugfs. I'll throw a patch for that into the
pile to get that problem out of the way.

I'm also beginning to think we should add a way to pass in the of_node
for a clk when it's registered.  Probably need to do that with the
initdata structure again. That way we can lookup a clk's parents with
the DT node if they don't call clk_register() with a device and also to
generate a better unique name for the clk for debug purposes.

> * How will it behave if 2 clock registers with "foo" and one clock register
> with the fallback parent "foo" ?

Sorry, I'm not following. The fallback is global so we'll be unable to
figure out which parent the clk is referring to. Maybe this is an
argument for keeping everything globally unique in the clk name space?

> 
> > 
> > Cc: Miquel Raynal <miquel.raynal@...tlin.com>
> > Cc: Jerome Brunet <jbrunet@...libre.com>
> > Cc: Russell King <linux@...linux.org.uk>
> > Cc: Michael Turquette <mturquette@...libre.com>
> > Signed-off-by: Stephen Boyd <sboyd@...nel.org>
> > ---
> > @@ -2365,8 +2432,11 @@ bool clk_has_parent(struct clk *clk, struct clk
> > *parent)
> >       if (core->parent == parent_core)
> >               return true;
> >  
> > -     return match_string(core->parent_names, core->num_parents,
> > -                         parent_core->name) >= 0;
> > +     for (i = 0; i < core->num_parents; i++)
> > +             if (!strcmp(core->parents[i].fallback, parent_core->name))
> > +                     return true;

This is also messy and not great BTW.

> > +
> > +     return false;
> >  }
> >  EXPORT_SYMBOL_GPL(clk_has_parent);
> >  
> > @@ -2949,9 +3019,9 @@ static int possible_parents_show(struct seq_file *s,
> > void *data)
> >       int i;
> >  
> >       for (i = 0; i < core->num_parents - 1; i++)
> > -             seq_printf(s, "%s ", core->parent_names[i]);
> > +             seq_printf(s, "%s ", core->parents[i].fallback);
> >  
> > -     seq_printf(s, "%s\n", core->parent_names[i]);
> > +     seq_printf(s, "%s\n", core->parents[i].fallback);
> 
> Wouldn't this show nothing if parent_data is used but fallback is not provided
> (like in your example when you provide the clk_hw pointer instead) or did I
> miss something ?

Yes, it will show nothing. Maybe we need to generate the debug name
somehow? That code sounds quite awful because we're going in reverse to
the device through a DT node pointer. Or add an indicator in the output
if the parent is a global name vs. a local name or a clk debugfs name?

   "global_name(g)"
   "local_name(l)"
   "debug_name"

Or if we can't figure anything out then perhaps just ignoring this
problem for now is fine. It is debugfs after all.

> 
> >  
> >       return 0;
> >  }
> > @@ -3360,6 +3424,74 @@ struct clk *clk_hw_create_clk(struct device *dev,
> > struct clk_hw *hw,
> >       return clk;
> >  }
> >  
> > +static int clk_core_populate_parent_map(struct clk_core *core)
> > +{
> > +     const struct clk_init_data *init = core->hw->init;
> > +     u8 num_parents = init->num_parents;
> > +     const char * const *parent_names = init->parent_names;
> > +     struct clk_hw **parent_hws = init->parent_hws;
> > +     const struct clk_parent_data *parent_data = init->parent_data;
> > +     int i, ret = 0;
> > +     struct clk_parent_map *parents, *parent;
> > +
> > +     if (!num_parents)
> > +             return 0;
> > +
> > +     /*
> > +      * Avoid unnecessary string look-ups of clk_core's possible parents by
> > +      * having a cache of names/clk_hw pointers to clk_core pointers.
> > +      */
> > +     parents = kcalloc(num_parents, sizeof(*parents), GFP_KERNEL);
> > +     core->parents = parents;
> > +     if (!parents)
> > +             return -ENOMEM;
> > +
> > +     /* Copy everything over because it might be __initdata */
> > +     for (i = 0, parent = parents; i < num_parents; i++, parent++) {
> > +             if (parent_names) {
> > +                     /* throw a WARN if any entries are NULL */
> > +                     WARN(!parent_names[i],
> > +                             "%s: invalid NULL in %s's .parent_names\n",
> > +                             __func__, core->name);
> > +                     parent->fallback = kstrdup_const(parent_names[i],
> > +                                                      GFP_KERNEL);
> > +                     if (!parent->fallback) {
> > +                             ret = -ENOMEM;
> > +                             while (--i >= 0)
> > +                                     kfree_const(parents[i].fallback);
> > +                     }
> > +             } else if (parent_data) {
> > +                     parent->hw = parent_data[i].hw;
> > +                     parent->name = parent_data[i].name;
> > +                     parent->fallback = parent_data[i].fallback;
> 
> I'm a bit confused by the comment at the top of the loop. Strings in
> parent_data are not copied over like we used to do with parent_names.
> 
> Is it allowed to have parent_data in __initdata ? It could be error prone if
> parent_names and parent_data behaved differently on this.

Good catch, thanks. Will fix.

> 
> > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> > index 8b84dee942bf..f513f4074a93 100644
> > --- a/include/linux/clk-provider.h
> > +++ b/include/linux/clk-provider.h
> > @@ -264,6 +264,18 @@ struct clk_ops {
> >       void            (*debug_init)(struct clk_hw *hw, struct dentry
> > *dentry);
> >  };
> >  
> > +/**
> > + * struct clk_parent_data - clk parent information
> > + * @hw: parent clk_hw pointer (used for clk providers with internal clks)
> > + * @name: parent name local to provider registering clk
> > + * @fallback: globally unique parent name (used as a fallback)
> > + */
> > +struct clk_parent_data {
> > +     struct clk_hw   *hw;
> > +     const char      *name;
> > +     const char      *fallback;
> 
> If I understand correctly, .name and .fallback will be ignored if hw is
> provided ? Maybe this should be documented somehow ?

Sure. I'll add some documentation to the long portion of the kernel-doc
here describing priority order.

> 
> > +};
> > +
> >  /**
> >   * struct clk_init_data - holds init data that's common to all clocks and
> > is
> >   * shared between the clock provider and the common clock framework.
> > @@ -277,7 +289,10 @@ struct clk_ops {
> >  struct clk_init_data {
> >       const char              *name;
> >       const struct clk_ops    *ops;
> > -     const char              * const *parent_names;
> > +     /* Only one of the following three should be assigned */
> > +     const char              * const *parent_names; /* If NULL (and
> > num_parents != 0) look at parent_data and parent_hws */
> > +     const struct clk_parent_data    *parent_data;
> > +     struct clk_hw           **parent_hws;
> 
> Isn't parent_hws redundant with parent_data ? you may pass the clk_hw pointer
> with both, right ?

Yeah it's redundant, but I thought that drivers may not want to waste
the extra space for pointers if they had all the pointers in hand for
the parents they care about, or if they had a single parent and just
wanted to point to that directly.

Another thought is to have a union on these three pointers and then a
flag indicating which method is used:

	union {
		const char              * const *parent_names;
		const struct clk_parent_data    *parent_data;
		struct clk_hw           **parent_hws;
	};

	#define CLK_PARENTS_POINTERS		BIT(3)
	#define CLK_PARENTS_DATA		BIT(8)

Powered by blists - more mailing lists