lkml.org 
[lkml]   [2019]   [Mar]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 6/8] clk: Allow parents to be specified without string names
From
Date
On 3/6/2019 10:48 AM, Stephen Boyd wrote:
> Quoting Jeffrey Hugo (2019-03-02 13:25:06)
>> On 2/26/2019 3:34 PM, 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.
>>>
>>> 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.
>>>
>>
>> I don't know exactly what regressed from V1, but this change breaks all
>> clock drivers as far as I can tell. All clocks from old and new (ie
>> 8998 mmcc rebased onto this) drivers end up as orphans.
>>
>> Is there some data I can provide to help you figure out the issue?
>>
>
> Can you try this patch? It fixes a pointer blunder that I'm sad about.

That did the trick. Everything seems to work again. I haven't
identified any additional issues.

>
> ----8<-----
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 37aea7884166..d12afd256dc5 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -3297,15 +3297,17 @@ struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw,
> return clk;
> }
>
> -static int clk_cpy_name(const char *dst, const char *src, bool must_exist)
> +static int clk_cpy_name(const char **dst_p, const char *src, bool must_exist)
> {
> + const char *dst;
> +
> if (!src) {
> if (must_exist)
> return -EINVAL;
> return 0;
> }
>
> - dst = kstrdup_const(src, GFP_KERNEL);
> + *dst_p = dst = kstrdup_const(src, GFP_KERNEL);
> if (!dst)
> return -ENOMEM;
>
> @@ -3341,14 +3343,14 @@ static int clk_core_populate_parent_map(struct clk_core *core)
> 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],
> + ret = clk_cpy_name(&parent->name, parent_names[i],
> true);
> } else if (parent_data) {
> parent->hw = parent_data[i].hw;
> - ret = clk_cpy_name(parent->fw_name,
> + ret = clk_cpy_name(&parent->fw_name,
> parent_data[i].fw_name, false);
> if (!ret)
> - ret = clk_cpy_name(parent->name,
> + ret = clk_cpy_name(&parent->name,
> parent_data[i].name,
> false);
> } else if (parent_hws) {
>


--
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

\
 
 \ /
  Last update: 2019-03-06 22:47    [W:0.061 / U:5.648 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site