Message-ID: <155189520594.20095.18225586788918924296@swboyd.mtv.corp.google.com>
Date:   Wed, 06 Mar 2019 10:00:05 -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>,
        Jeffrey Hugo <jhugo@...eaurora.org>,
        Chen-Yu Tsai <wens@...e.org>
Subject: Re: [PATCH v2 6/8] clk: Allow parents to be specified without string names

Quoting Jerome Brunet (2019-03-02 10:40:42)
> On Tue, 2019-02-26 at 14:34 -0800, Stephen Boyd wrote:
> > The common clk framework is lacking in ability to describe the clk
> > topology without specifying strings for every possible parent-child
> > link. There are a few drawbacks to the current approach:
> > 
> >  1) String comparisons are used for everything, including describing
> >  topologies that are 'local' to a single clock controller.
> > 
> >  2) clk providers (e.g. i2c clk drivers) need to create globally unique
> >  clk names to avoid collisions in the clk namespace, leading to awkward
> >  name generation code in various clk drivers.
> > 
> >  3) DT bindings may not fully describe the clk topology and linkages
> >  between clk controllers because drivers can easily rely on globally unique
> >  strings to describe connections between clks.
> > 
> > This leads to confusing DT bindings, complicated clk name generation
> > code, and inefficient string comparisons during clk registration just so
> > that the clk framework can detect the topology of the clk tree.
> > Furthermore, some drivers call clk_get() and then __clk_get_name() to
> > extract the globally unique clk name just so they can specify the parent
> > of the clk they're registering. We have of_clk_parent_fill() but that
> > mostly only works for single clks registered from a DT node, which isn't
> > the norm. Let's simplify this all by introducing two new ways of
> > specifying clk parents.
> > 
> > The first method is an array of pointers to clk_hw structures
> > corresponding to the parents at that index. This works for clks that are
> > registered when we have access to all the clk_hw pointers for the
> > parents.
> > 
> > The second method is a mix of clk_hw pointers and strings of local and
> > global parent clk names. If the .name member of the map is set we'll
> > look for that clk by performing a DT based lookup of the device the clk
> > is registered with and the .name specified in the map. If that fails,
> > we'll fallback to the .fallback member and perform a global clk name
> > lookup like we've always done before.
> 
> Nitpick: I think you forgot to update the commit message when renaming the
> struct members

Fixed. Thanks!

> 
> > 
> > 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.
> > 
> 
> Being able to specify parents from DT is great addition !!! Thx !

Woohooo!

> 
> Overall, it looks good but with such big patch on framework is not easy get a
> clear idea. I'll try to give it a spin next week.

Ok. Please be aware that I found one bug already but there are probably
more lurking. I've also pushed the branch out to clk.git on kernel.org
if you're interested in looking at the mass conversions going on.

https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-parent-rewrite

Powered by blists - more mailing lists