Linux-Clk Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2] clk: Fix falling back to legacy parent string matching
@ 2019-08-13 21:41 Stephen Boyd
  2019-08-16  3:52 ` Taniya Das
  2019-08-16 17:28 ` Stephen Boyd
  0 siblings, 2 replies; 3+ messages in thread
From: Stephen Boyd @ 2019-08-13 21:41 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd
  Cc: linux-kernel, linux-clk, Taniya Das, Jerome Brunet, Chen-Yu Tsai

Calls to clk_core_get() will return ERR_PTR(-EINVAL) if we've started
migrating a clk driver to use the DT based style of specifying parents
but we haven't made any DT updates yet. This happens when we pass a
non-NULL value as the 'name' argument of of_parse_clkspec(). That
function returns -EINVAL in such a situation, instead of -ENOENT like we
expected. The return value comes back up to clk_core_fill_parent_index()
which proceeds to skip calling clk_core_lookup() because the error
pointer isn't equal to -ENOENT, it's -EINVAL.

Furthermore, we blindly overwrite the error pointer returned by
clk_core_get() with NULL when there isn't a legacy .name member
specified in the parent map. This isn't too bad right now because we
don't really care to differentiate NULL from an error, but in the future
we should only try to do a legacy lookup if we know we might find
something. This way DT lookups that fail don't try to lookup based on
strings when there isn't any string to match, hiding the error from DT
parsing.

Fix both these problems so that clk provider drivers can use the new
style of parent mapping without having to also update their DT at the
same time. This patch is based on an earlier patch from Taniya Das which
checked for -EINVAL in addition to -ENOENT return values from
clk_core_get().

Fixes: 601b6e93304a ("clk: Allow parents to be specified via clkspec index")
Cc: Taniya Das <tdas@codeaurora.org>
Cc: Jerome Brunet <jbrunet@baylibre.com>
Cc: Chen-Yu Tsai <wens@csie.org>
Reported-by: Taniya Das <tdas@codeaurora.org>
Signed-off-by: Stephen Boyd <sboyd@kernel.org>
---

Changes from v1:
 * Don't trust error values from of_clk_get_hw() anymore, instead 
   look for parsing errors and only proceed if the parsing doesn't
   fail.
 * Leave the -ENOENT check in place so that EPROBE_DEFER returned
   from clk_core_lookup() doesn't try to do a global string search
   if parsing succeeded.

 drivers/clk/clk.c | 46 ++++++++++++++++++++++++++++++++++------------
 1 file changed, 34 insertions(+), 12 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index c0990703ce54..8bce6bb4a965 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -324,6 +324,25 @@ static struct clk_core *clk_core_lookup(const char *name)
 	return NULL;
 }
 
+#ifdef CONFIG_OF
+static int of_parse_clkspec(const struct device_node *np, int index,
+			    const char *name, struct of_phandle_args *out_args);
+static struct clk_hw *
+of_clk_get_hw_from_clkspec(struct of_phandle_args *clkspec);
+#else
+static inline int of_parse_clkspec(const struct device_node *np, int index,
+				   const char *name,
+				   struct of_phandle_args *out_args)
+{
+	return -ENOENT;
+}
+static inline struct clk_hw *
+of_clk_get_hw_from_clkspec(struct of_phandle_args *clkspec)
+{
+	return ERR_PTR(-ENOENT);
+}
+#endif
+
 /**
  * clk_core_get - Find the clk_core parent of a clk
  * @core: clk to find parent of
@@ -355,8 +374,9 @@ static struct clk_core *clk_core_lookup(const char *name)
  *      };
  *
  * 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.
+ * exist in the provider or the name can't be found in the DT node or
+ * in a clkdev lookup. 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, u8 p_index)
@@ -367,17 +387,19 @@ static struct clk_core *clk_core_get(struct clk_core *core, u8 p_index)
 	struct device *dev = core->dev;
 	const char *dev_id = dev ? dev_name(dev) : NULL;
 	struct device_node *np = core->of_node;
+	struct of_phandle_args clkspec;
 
-	if (np && (name || index >= 0))
-		hw = of_clk_get_hw(np, index, name);
-
-	/*
-	 * If the DT search above couldn't find the provider or the provider
-	 * didn't know about this clk, fallback to looking up via clkdev based
-	 * clk_lookups
-	 */
-	if (PTR_ERR(hw) == -ENOENT && name)
+	if (np && (name || index >= 0) &&
+	    !of_parse_clkspec(np, index, name, &clkspec)) {
+		hw = of_clk_get_hw_from_clkspec(&clkspec);
+		of_node_put(clkspec.np);
+	} else if (name) {
+		/*
+		 * If the DT search above couldn't find the provider fallback to
+		 * looking up via clkdev based clk_lookups.
+		 */
 		hw = clk_find_hw(dev_id, name);
+	}
 
 	if (IS_ERR(hw))
 		return ERR_CAST(hw);
@@ -401,7 +423,7 @@ static void clk_core_fill_parent_index(struct clk_core *core, u8 index)
 			parent = ERR_PTR(-EPROBE_DEFER);
 	} else {
 		parent = clk_core_get(core, index);
-		if (IS_ERR(parent) && PTR_ERR(parent) == -ENOENT)
+		if (IS_ERR(parent) && PTR_ERR(parent) == -ENOENT && entry->name)
 			parent = clk_core_lookup(entry->name);
 	}
 
-- 
Sent by a computer through tubes


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] clk: Fix falling back to legacy parent string matching
  2019-08-13 21:41 [PATCH v2] clk: Fix falling back to legacy parent string matching Stephen Boyd
@ 2019-08-16  3:52 ` Taniya Das
  2019-08-16 17:28 ` Stephen Boyd
  1 sibling, 0 replies; 3+ messages in thread
From: Taniya Das @ 2019-08-16  3:52 UTC (permalink / raw)
  To: Stephen Boyd, Michael Turquette
  Cc: linux-kernel, linux-clk, Jerome Brunet, Chen-Yu Tsai

Hello Stephen,

Thanks for the patch, I have tested it on the device.

On 8/14/2019 3:11 AM, Stephen Boyd wrote:
> Calls to clk_core_get() will return ERR_PTR(-EINVAL) if we've started
> migrating a clk driver to use the DT based style of specifying parents
> but we haven't made any DT updates yet. This happens when we pass a
> non-NULL value as the 'name' argument of of_parse_clkspec(). That
> function returns -EINVAL in such a situation, instead of -ENOENT like we
> expected. The return value comes back up to clk_core_fill_parent_index()
> which proceeds to skip calling clk_core_lookup() because the error
> pointer isn't equal to -ENOENT, it's -EINVAL.
> 
> Furthermore, we blindly overwrite the error pointer returned by
> clk_core_get() with NULL when there isn't a legacy .name member
> specified in the parent map. This isn't too bad right now because we
> don't really care to differentiate NULL from an error, but in the future
> we should only try to do a legacy lookup if we know we might find
> something. This way DT lookups that fail don't try to lookup based on
> strings when there isn't any string to match, hiding the error from DT
> parsing.
> 
> Fix both these problems so that clk provider drivers can use the new
> style of parent mapping without having to also update their DT at the
> same time. This patch is based on an earlier patch from Taniya Das which
> checked for -EINVAL in addition to -ENOENT return values from
> clk_core_get().
> 
> Fixes: 601b6e93304a ("clk: Allow parents to be specified via clkspec index")
> Cc: Taniya Das <tdas@codeaurora.org>
> Cc: Jerome Brunet <jbrunet@baylibre.com>
> Cc: Chen-Yu Tsai <wens@csie.org>
> Reported-by: Taniya Das <tdas@codeaurora.org>
> Signed-off-by: Stephen Boyd <sboyd@kernel.org>
> ---
> 

Tested-by: Taniya Das <tdas@codeaurora.org>

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.

--

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] clk: Fix falling back to legacy parent string matching
  2019-08-13 21:41 [PATCH v2] clk: Fix falling back to legacy parent string matching Stephen Boyd
  2019-08-16  3:52 ` Taniya Das
@ 2019-08-16 17:28 ` Stephen Boyd
  1 sibling, 0 replies; 3+ messages in thread
From: Stephen Boyd @ 2019-08-16 17:28 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd
  Cc: linux-kernel, linux-clk, Taniya Das, Jerome Brunet, Chen-Yu Tsai

Quoting Stephen Boyd (2019-08-13 14:41:47)
> Calls to clk_core_get() will return ERR_PTR(-EINVAL) if we've started
> migrating a clk driver to use the DT based style of specifying parents
> but we haven't made any DT updates yet. This happens when we pass a
> non-NULL value as the 'name' argument of of_parse_clkspec(). That
> function returns -EINVAL in such a situation, instead of -ENOENT like we
> expected. The return value comes back up to clk_core_fill_parent_index()
> which proceeds to skip calling clk_core_lookup() because the error
> pointer isn't equal to -ENOENT, it's -EINVAL.
> 
> Furthermore, we blindly overwrite the error pointer returned by
> clk_core_get() with NULL when there isn't a legacy .name member
> specified in the parent map. This isn't too bad right now because we
> don't really care to differentiate NULL from an error, but in the future
> we should only try to do a legacy lookup if we know we might find
> something. This way DT lookups that fail don't try to lookup based on
> strings when there isn't any string to match, hiding the error from DT
> parsing.
> 
> Fix both these problems so that clk provider drivers can use the new
> style of parent mapping without having to also update their DT at the
> same time. This patch is based on an earlier patch from Taniya Das which
> checked for -EINVAL in addition to -ENOENT return values from
> clk_core_get().
> 
> Fixes: 601b6e93304a ("clk: Allow parents to be specified via clkspec index")
> Cc: Taniya Das <tdas@codeaurora.org>
> Cc: Jerome Brunet <jbrunet@baylibre.com>
> Cc: Chen-Yu Tsai <wens@csie.org>
> Reported-by: Taniya Das <tdas@codeaurora.org>
> Signed-off-by: Stephen Boyd <sboyd@kernel.org>
> ---

Applied to clk-fixes


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-13 21:41 [PATCH v2] clk: Fix falling back to legacy parent string matching Stephen Boyd
2019-08-16  3:52 ` Taniya Das
2019-08-16 17:28 ` Stephen Boyd

Linux-Clk Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-clk/0 linux-clk/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-clk linux-clk/ https://lore.kernel.org/linux-clk \
		linux-clk@vger.kernel.org linux-clk@archiver.kernel.org
	public-inbox-index linux-clk


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-clk


AGPL code for this site: git clone https://public-inbox.org/ public-inbox