From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=3.0 tests=DKIM_INVALID,DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 481F8C4360F for ; Sat, 2 Mar 2019 21:25:14 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id D3CF120815 for ; Sat, 2 Mar 2019 21:25:13 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="key not found in DNS" (0-bit key) header.d=codeaurora.org header.i=@codeaurora.org header.b="kfBRtNHJ"; dkim=fail reason="key not found in DNS" (0-bit key) header.d=codeaurora.org header.i=@codeaurora.org header.b="EBgg3dPT" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726733AbfCBVZL (ORCPT ); Sat, 2 Mar 2019 16:25:11 -0500 Received: from smtp.codeaurora.org ([198.145.29.96]:40762 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726592AbfCBVZL (ORCPT ); Sat, 2 Mar 2019 16:25:11 -0500 Received: by smtp.codeaurora.org (Postfix, from userid 1000) id B010B60E41; Sat, 2 Mar 2019 21:25:09 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=codeaurora.org; s=default; t=1551561909; bh=d7vPR8n0NAAq2KHjK27tOyeYtrYZ4DZesXIJq1tx/wE=; h=Subject:To:Cc:References:From:Date:In-Reply-To:From; b=kfBRtNHJWZ0P/jbX0mIl2PNhpbaxywD8JoSUbh9GKQ3ThuedeM5taH/BEJsjAd8ar CzwGCYS7TwTQd/DahChl6sN+/ojx2irfMNSBSSBpletSmzVsEJFUcqkxJ5Hs+0vwj8 x9G/B8zWhQvccs5Yvdo57GKuZ+n4nV0TzhLbuaDo= Received: from [10.226.60.81] (i-global254.qualcomm.com [199.106.103.254]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) (Authenticated sender: jhugo@smtp.codeaurora.org) by smtp.codeaurora.org (Postfix) with ESMTPSA id A938760907; Sat, 2 Mar 2019 21:25:06 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=codeaurora.org; s=default; t=1551561907; bh=d7vPR8n0NAAq2KHjK27tOyeYtrYZ4DZesXIJq1tx/wE=; h=Subject:To:Cc:References:From:Date:In-Reply-To:From; b=EBgg3dPTqLSdDEyZMyNe7HlHGQGkcLUM1WEshVUqYVjsrFPc17htIWsJyWQIu5ie7 A39DLFZk1t1de5hfXPHAcakumk670iqQHoohi4UPv6B868NOuhsEz/g0bIsGpycmfC EA+7NkzI52tVDLgZaalT1KYeA+H9iiWK2X8M3z24= DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org A938760907 Authentication-Results: pdx-caf-mail.web.codeaurora.org; dmarc=none (p=none dis=none) header.from=codeaurora.org Authentication-Results: pdx-caf-mail.web.codeaurora.org; spf=none smtp.mailfrom=jhugo@codeaurora.org Subject: Re: [PATCH v2 6/8] clk: Allow parents to be specified without string names To: Stephen Boyd , Michael Turquette Cc: linux-kernel@vger.kernel.org, linux-clk@vger.kernel.org, Miquel Raynal , Jerome Brunet , Russell King , Chen-Yu Tsai References: <20190226223429.193873-1-sboyd@kernel.org> <20190226223429.193873-7-sboyd@kernel.org> From: Jeffrey Hugo Message-ID: Date: Sat, 2 Mar 2019 14:25:06 -0700 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.5.0 MIME-Version: 1.0 In-Reply-To: <20190226223429.193873-7-sboyd@kernel.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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? > Cc: Miquel Raynal > Cc: Jerome Brunet > Cc: Russell King > Cc: Michael Turquette > Cc: Jeffrey Hugo > Cc: Chen-Yu Tsai > Signed-off-by: Stephen Boyd > --- > drivers/clk/clk.c | 260 ++++++++++++++++++++++++++--------- > include/linux/clk-provider.h | 19 +++ > 2 files changed, 217 insertions(+), 62 deletions(-) > > 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); > > /*** private data structures ***/ > > +struct clk_parent_map { > + const struct clk_hw *hw; > + struct clk_core *core; > + const char *fw_name; > + const char *name; > +}; > + > struct clk_core { > const char *name; > const struct clk_ops *ops; > @@ -46,8 +53,7 @@ struct clk_core { > struct module *owner; > struct device *dev; > struct clk_core *parent; > - const char **parent_names; > - struct clk_core **parents; > + struct clk_parent_map *parents; > u8 num_parents; > u8 new_parent_index; > unsigned long rate; > @@ -316,17 +322,92 @@ static struct clk_core *clk_core_lookup(const char *name) > return NULL; > } > > +/** > + * clk_core_get - Find the parent of a clk using a clock specifier in DT > + * @core: clk to find parent of > + * @name: name to search for in 'clock-names' of device providing clk > + * > + * This is the preferred method for clk providers to find the parent of a > + * clk when that parent is external to the clk controller. The parent_names > + * array is indexed and treated as a local name matching a string in the device > + * node's 'clock-names' property. This allows clk providers to use their own > + * namespace instead of looking for a globally unique parent string. > + * > + * For example the following DT snippet would allow a clock registered by the > + * clock-controller@c001 that has a clk_init_data::parent_data array > + * with 'xtal' in the 'name' member to find the clock provided by the > + * clock-controller@f00abcd without needing to get the globally unique name of > + * the xtal clk. > + * > + * parent: clock-controller@f00abcd { > + * reg = <0xf00abcd 0xabcd>; > + * #clock-cells = <0>; > + * }; > + * > + * clock-controller@c001 { > + * reg = <0xc001 0xf00d>; > + * clocks = <&parent>; > + * clock-names = "xtal"; > + * #clock-cells = <1>; > + * }; > + * > + * Returns: -ENOENT when the provider can't be found or the clk doesn't > + * exist in the provider. -EINVAL when the name can't be found. NULL when the > + * provider knows about the clk but it isn't provided on this system. > + * A valid clk_core pointer when the clk can be found in the provider. > + */ > +static struct clk_core *clk_core_get(struct clk_core *core, const char *name) > +{ > + struct clk_hw *hw; > + struct device *dev = core->dev; > + > + if (!dev) > + return ERR_PTR(-ENOENT); > + > + /* TODO: Support clkdev clk_lookups */ > + hw = of_clk_get_hw(dev->of_node, -1, name); > + if (IS_ERR_OR_NULL(hw)) > + return ERR_CAST(hw); > + > + return hw->core; > +} > + > +static void clk_core_fill_parent_index(struct clk_core *core, u8 index) > +{ > + struct clk_parent_map *entry = &core->parents[index]; > + struct clk_core *parent = ERR_PTR(-ENOENT); > + > + if (entry->hw) { > + parent = entry->hw->core; > + /* > + * We have a direct reference but it isn't registered yet? Orphan it > + * and let clk_reparent() update the orphan status when the parent > + * is registered. > + */ > + if (!parent) > + parent = ERR_PTR(-EPROBE_DEFER); > + } else { > + if (entry->fw_name) > + parent = clk_core_get(core, entry->fw_name); > + if (IS_ERR(parent) && PTR_ERR(parent) == -ENOENT) > + parent = clk_core_lookup(entry->name); > + } > + > + /* Only cache it if it's not an error */ > + if (!IS_ERR(parent)) > + entry->core = parent; > +} > + > static struct clk_core *clk_core_get_parent_by_index(struct clk_core *core, > u8 index) > { > - if (!core || index >= core->num_parents) > + if (!core || index >= core->num_parents || !core->parents) > return NULL; > > - if (!core->parents[index]) > - core->parents[index] = > - clk_core_lookup(core->parent_names[index]); > + if (!core->parents[index].core) > + clk_core_fill_parent_index(core, index); > > - return core->parents[index]; > + return core->parents[index].core; > } > > struct clk_hw * > @@ -1516,15 +1597,15 @@ static int clk_fetch_parent_index(struct clk_core *core, > return -EINVAL; > > for (i = 0; i < core->num_parents; i++) { > - if (core->parents[i] == parent) > + if (core->parents[i].core == parent) > return i; > > - if (core->parents[i]) > + if (core->parents[i].core) > continue; > > /* Fallback to comparing globally unique names */ > - if (!strcmp(parent->name, core->parent_names[i])) { > - core->parents[i] = parent; > + if (!strcmp(parent->name, core->parents[i].name)) { > + core->parents[i].core = parent; > return i; > } > } > @@ -2290,6 +2371,7 @@ void clk_hw_reparent(struct clk_hw *hw, struct clk_hw *new_parent) > bool clk_has_parent(struct clk *clk, struct clk *parent) > { > struct clk_core *core, *parent_core; > + int i; > > /* NULL clocks should be nops, so return success if either is NULL. */ > if (!clk || !parent) > @@ -2302,8 +2384,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].name, parent_core->name)) > + return true; > + > + return false; > } > EXPORT_SYMBOL_GPL(clk_has_parent); > > @@ -2886,9 +2971,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].name); > > - seq_printf(s, "%s\n", core->parent_names[i]); > + seq_printf(s, "%s\n", core->parents[i].name); > > return 0; > } > @@ -3022,7 +3107,7 @@ static inline void clk_debug_unregister(struct clk_core *core) > */ > static int __clk_core_init(struct clk_core *core) > { > - int i, ret; > + int ret; > struct clk_core *orphan; > struct hlist_node *tmp2; > unsigned long rate; > @@ -3076,12 +3161,6 @@ static int __clk_core_init(struct clk_core *core) > goto out; > } > > - /* throw a WARN if any entries in parent_names are NULL */ > - for (i = 0; i < core->num_parents; i++) > - WARN(!core->parent_names[i], > - "%s: invalid NULL in %s's .parent_names\n", > - __func__, core->name); > - > core->parent = __clk_init_parent(core); > > /* > @@ -3310,6 +3389,96 @@ 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) > +{ > + 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) { > + 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 { > + ret = -EINVAL; > + WARN(1, "Must specify parents if num_parents > 0\n"); > + } > + > + if (ret) { > + do { > + kfree_const(parents[i].name); > + kfree_const(parents[i].fw_name); > + } while (--i >= 0); > + kfree(parents); > + > + return ret; > + } > + } > + > + return 0; > +} > + > +static void clk_core_free_parent_map(struct clk_core *core) > +{ > + int i = core->num_parents; > + > + if (!core->num_parents) > + return; > + > + while (--i >= 0) { > + kfree_const(core->parents[i].name); > + kfree_const(core->parents[i].fw_name); > + } > + > + kfree(core->parents); > +} > + > /** > * clk_register - allocate a new clock, register it and return an opaque cookie > * @dev: device that is registering this clock > @@ -3323,7 +3492,7 @@ struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw, > */ > struct clk *clk_register(struct device *dev, struct clk_hw *hw) > { > - int i, ret; > + int ret; > struct clk_core *core; > > core = kzalloc(sizeof(*core), GFP_KERNEL); > @@ -3356,33 +3525,9 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw) > core->max_rate = ULONG_MAX; > hw->core = core; > > - /* allocate local copy in case parent_names is __initdata */ > - core->parent_names = kcalloc(core->num_parents, sizeof(char *), > - GFP_KERNEL); > - > - if (!core->parent_names) { > - ret = -ENOMEM; > - goto fail_parent_names; > - } > - > - > - /* copy each string name in case parent_names is __initdata */ > - for (i = 0; i < core->num_parents; i++) { > - core->parent_names[i] = kstrdup_const(hw->init->parent_names[i], > - GFP_KERNEL); > - if (!core->parent_names[i]) { > - ret = -ENOMEM; > - goto fail_parent_names_copy; > - } > - } > - > - /* avoid unnecessary string look-ups of clk_core's possible parents. */ > - core->parents = kcalloc(core->num_parents, sizeof(*core->parents), > - GFP_KERNEL); > - if (!core->parents) { > - ret = -ENOMEM; > + ret = clk_core_populate_parent_map(core); > + if (ret) > goto fail_parents; > - }; > > INIT_HLIST_HEAD(&core->clks); > > @@ -3393,7 +3538,7 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw) > hw->clk = alloc_clk(core, NULL, NULL); > if (IS_ERR(hw->clk)) { > ret = PTR_ERR(hw->clk); > - goto fail_parents; > + goto fail_create_clk; > } > > clk_core_link_consumer(hw->core, hw->clk); > @@ -3409,13 +3554,9 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw) > free_clk(hw->clk); > hw->clk = NULL; > > +fail_create_clk: > + clk_core_free_parent_map(core); > fail_parents: > - kfree(core->parents); > -fail_parent_names_copy: > - while (--i >= 0) > - kfree_const(core->parent_names[i]); > - kfree(core->parent_names); > -fail_parent_names: > fail_ops: > kfree_const(core->name); > fail_name: > @@ -3445,15 +3586,10 @@ EXPORT_SYMBOL_GPL(clk_hw_register); > static void __clk_release(struct kref *ref) > { > struct clk_core *core = container_of(ref, struct clk_core, ref); > - int i = core->num_parents; > > lockdep_assert_held(&prepare_lock); > > - kfree(core->parents); > - while (--i >= 0) > - kfree_const(core->parent_names[i]); > - > - kfree(core->parent_names); > + clk_core_free_parent_map(core); > kfree_const(core->name); > kfree(core); > } > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h > index e443fa9fa859..f4de52c6764e 100644 > --- a/include/linux/clk-provider.h > +++ b/include/linux/clk-provider.h > @@ -250,6 +250,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) > + * @fw_name: parent name local to provider registering clk > + * @name: globally unique parent name (used as a fallback) > + */ > +struct clk_parent_data { > + const struct clk_hw *hw; > + const char *fw_name; > + const char *name; > +}; > + > /** > * 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. > @@ -257,13 +269,20 @@ struct clk_ops { > * @name: clock name > * @ops: operations this clock supports > * @parent_names: array of string names for all possible parents > + * @parent_data: array of parent data for all possible parents (when some > + * parents are external to the clk controller) > + * @parent_hws: array of pointers to all possible parents (when all parents > + * are internal to the clk controller) > * @num_parents: number of possible parents > * @flags: framework-level hints and quirks > */ > struct clk_init_data { > const char *name; > const struct clk_ops *ops; > + /* Only one of the following three should be assigned */ > const char * const *parent_names; > + const struct clk_parent_data *parent_data; > + const struct clk_hw **parent_hws; > u8 num_parents; > unsigned long flags; > }; > -- 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.