Re: [PATCH 7/9] clk: Allow parents to be specified without string names

[Jerome Brunet <jbrunet@baylibre.com>]

On Tue, 2019-01-29 at 10:56 -0800, Stephen Boyd wrote:
> 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.

Ok. Got it

> 
> > 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.

If we are supposed to keep globally unique names, maybe we should be keep
things as they are for now.

> 
> 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.

I don't get it now, but I'm sure I'll understand better with the code later on
:)

> 
> > * 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.

This is what I was thinking as well. But since the global clock namespace is
not the point of this series, the question is off topic. Let's leave this foranother day 

>  Maybe this is an
> argument for keeping everything globally unique in the clk name space?

For now, sure.

> 
> > > Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> > > Cc: Jerome Brunet <jbrunet@baylibre.com>
> > > Cc: Russell King <linux@armlinux.org.uk>
> > > Cc: Michael Turquette <mturquette@baylibre.com>
> > > Signed-off-by: Stephen Boyd <sboyd@kernel.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 would be nice. that or dropping the possible parent entry.

> 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.

Humm, I don't know if there are any rules regarding debugfs but it bothers me
if we knowingly report wrong values through it. This may lead other developers
to waste their time on this 

If we can't report the actual parent_name, we should at very least warn about
it - display [FIXME] or [MISSING] for example.

> 
> > >  
> > >       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.

Fine. Makes sense.

> 
> 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)
> 

Whatever is simpler :) 
If keeping the current solution, we just need to document the priority order.