Re: [PATCH v2 6/8] clk: Allow parents to be specified without string names

[Stephen Boyd]

Quoting Jerome Brunet (2019-03-15 03:01:53)
> On Tue, 2019-02-26 at 14:34 -0800, Stephen Boyd wrote:
> > ---
> >  drivers/clk/clk.c            | 260 ++++++++++++++++++++++++++---------
> >  include/linux/clk-provider.h |  19 +++
> >  2 files changed, 217 insertions(+), 62 deletions(-)
> 
> Sorry for the delay.
> 
> With the fix you sent to Jeffrey
> Tested by porting the aoclk controller of Amlogic g12a SoC.
> This allowed to test
>  * hws only table
>  * parent_data with a mix of hw pointers and fw_name (with different input
> controllers and also an input that is optional and never provided)
> 
> Tested-by: Jerome Brunet <jbrunet@xxxxxxxxxxxx>
> 
> With the small comment below
> 
> Reviewed-by Jerome Brunet <jbrunet@xxxxxxxxxxxx>
> 

Awesome. Thanks! I need to Cc Rob H to hopefully get an ack on the
concept of relying on DT so I'll resend this series again next week. It
would also be nice if I can throw in a couple more patches to let
drivers specify a DT node when registering a clk if they don't have a
struct device on hand and let drivers lookup with clk_lookups somehow.

> 
> > 
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 937b8d092d17..3d01e8c56400 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -39,6 +39,13 @@ static LIST_HEAD(clk_notifier_list);
> >  
> 
> [...]
> 
> >  
> > +static int clk_cpy_name(const char *dst, const char *src, bool must_exist)
> > +{
> > +     if (!src) {
> > +             if (must_exist)
> > +                     return -EINVAL;
> > +             return 0;
> > +     }
> > +
> > +     dst = kstrdup_const(src, GFP_KERNEL);
> > +     if (!dst)
> > +             return -ENOMEM;
> > +
> > +     return 0;
> > +}
> > +
> > +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;
> > +     const 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);
> > +                     ret = clk_cpy_name(parent->name, parent_names[i],
> > +                                        true);
> > +             } else if (parent_data) {
> 
> While testing, I mistakenly left both parent_names and parent_data. I was
> surprised that parent_data did not take precedence of parent_names.
> 
> Maybe it should ? (... but I understand we are not supposed to provide both)

I don't think we can. We have a problem where drivers don't initialize
the init structure properly, opting to just throw it on the stack and
leave junk in there that they overwrite. We'd have to go through all the
init structures and initialize them. I suppose we could make a macro for
that:

	DECLARE_CLK_INIT_DATA(init);

or something like that that does this. We could bury a magic number in
there under some debug option too so that we can make sure drivers are
doing this properly. Otherwise we're left to doing these weird tricks
like I've done here.

Regardless. I'll have to add a comment to this fact in the code. Thanks.

> 
> > +                     parent->hw = parent_data[i].hw;
> > +                     ret = clk_cpy_name(parent->fw_name,
> > +                                        parent_data[i].fw_name, false);
> > +                     if (!ret)
> > +                             ret = clk_cpy_name(parent->name,
> > +                                                parent_data[i].name,
> > +                                                false);
> > +             } else if (parent_hws) {
> > +                     parent->hw = parent_hws[i];
> > +             } else {
> 
> Maybe there should also some kinda of check to verify if more than one option
> (among hws, parent_data and parent_names) was provided and throw a warn ?
> 
> Could be useful with drivers move away from parent_names ?

Same thing. It would be nice but we're sort of unable to do so unless we
do what I suggest above. Should we do it?