Skip to contentSkip to conversation

clk: Ensure new parent is looked up when changing parents

PATCH In review Charles Keepax 2 participants 8 messages Apr 30, 2019 – May 3, 2019

Subsystems (2)
lore
Share & tools
lore

Charles Keepax

author maintainer
#0 lore raw

PATCH 1/2 clk: Ensure new parent is looked up when changing parents

clk_core_fill_parent_index is called from clk_mux_determine_rate_flags and for the initial parent of the clock but seems to not get called on the path changing a clocks parent. This can cause a clock parent change to fail, fix this by adding a call in clk_fetch_parent_index.

Fixes fc0c209c147f ("clk: Allow parents to be specified without string names")
---
 drivers/clk/clk.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/clk/clk.c +3 -0
index ffd33b63c37eb..5aa180180ee95 100644
@@ -1601,6 +1601,9 @@ static int clk_fetch_parent_index(struct clk_core *core,
 		return -EINVAL;
 
 	for (i = 0; i < core->num_parents; i++) {
+		if (!core->parents[i].core)
+			clk_core_fill_parent_index(core, i);
+
 		if (core->parents[i].core == parent)
 			return i;
 
Signature · 1 line
2.11.0
author maintainer
#1 lore raw

PATCH 2/2 clk: lochnagar: Use new parent_data approach to register clock parents

Switch over to the more modern style of registering parents and simplify the code in the process.

---
 drivers/clk/clk-lochnagar.c | 192 ++++++++++++++++++--------------------------
 1 file changed, 76 insertions(+), 116 deletions(-)
drivers/clk/clk-lochnagar.c +76 -116
index ced832e82f46b..58576db6e31a3 100644
@@ -42,46 +42,45 @@ struct lochnagar_clk_priv {
 	struct regmap *regmap;
 	enum lochnagar_type type;
 
-	const char **parents;
-	unsigned int nparents;
-
 	struct lochnagar_clk lclks[LOCHNAGAR_NUM_CLOCKS];
 };
 
-static const char * const lochnagar1_clk_parents[] = {
-	"ln-none",
-	"ln-spdif-mclk",
-	"ln-psia1-mclk",
-	"ln-psia2-mclk",
-	"ln-cdc-clkout",
-	"ln-dsp-clkout",
-	"ln-pmic-32k",
-	"ln-gf-mclk1",
-	"ln-gf-mclk3",
-	"ln-gf-mclk2",
-	"ln-gf-mclk4",
+#define LN_PARENT(NAME) { .name = NAME, .fw_name = NAME }
+
+static const struct clk_parent_data lochnagar1_clk_parents[] = {
+	LN_PARENT("ln-none"),
+	LN_PARENT("ln-spdif-mclk"),
+	LN_PARENT("ln-psia1-mclk"),
+	LN_PARENT("ln-psia2-mclk"),
+	LN_PARENT("ln-cdc-clkout"),
+	LN_PARENT("ln-dsp-clkout"),
+	LN_PARENT("ln-pmic-32k"),
+	LN_PARENT("ln-gf-mclk1"),
+	LN_PARENT("ln-gf-mclk3"),
+	LN_PARENT("ln-gf-mclk2"),
+	LN_PARENT("ln-gf-mclk4"),
 };
 
-static const char * const lochnagar2_clk_parents[] = {
-	"ln-none",
-	"ln-cdc-clkout",
-	"ln-dsp-clkout",
-	"ln-pmic-32k",
-	"ln-spdif-mclk",
-	"ln-clk-12m",
-	"ln-clk-11m",
-	"ln-clk-24m",
-	"ln-clk-22m",
-	"ln-clk-8m",
-	"ln-usb-clk-24m",
-	"ln-gf-mclk1",
-	"ln-gf-mclk3",
-	"ln-gf-mclk2",
-	"ln-psia1-mclk",
-	"ln-psia2-mclk",
-	"ln-spdif-clkout",
-	"ln-adat-mclk",
-	"ln-usb-clk-12m",
+static const struct clk_parent_data lochnagar2_clk_parents[] = {
+	LN_PARENT("ln-none"),
+	LN_PARENT("ln-cdc-clkout"),
+	LN_PARENT("ln-dsp-clkout"),
+	LN_PARENT("ln-pmic-32k"),
+	LN_PARENT("ln-spdif-mclk"),
+	LN_PARENT("ln-clk-12m"),
+	LN_PARENT("ln-clk-11m"),
+	LN_PARENT("ln-clk-24m"),
+	LN_PARENT("ln-clk-22m"),
+	LN_PARENT("ln-clk-8m"),
+	LN_PARENT("ln-usb-clk-24m"),
+	LN_PARENT("ln-gf-mclk1"),
+	LN_PARENT("ln-gf-mclk3"),
+	LN_PARENT("ln-gf-mclk2"),
+	LN_PARENT("ln-psia1-mclk"),
+	LN_PARENT("ln-psia2-mclk"),
+	LN_PARENT("ln-spdif-clkout"),
+	LN_PARENT("ln-adat-mclk"),
+	LN_PARENT("ln-usb-clk-12m"),
 };
 
 #define LN1_CLK(ID, NAME, REG) \
@@ -183,7 +182,7 @@ static u8 lochnagar_clk_get_parent(struct clk_hw *hw)
 	if (ret < 0) {
 		dev_dbg(priv->dev, "Failed to read parent of %s: %d\n",
 			lclk->name, ret);
-		return priv->nparents;
+		return hw->init->num_parents;
 	}
 
 	val &= lclk->src_mask;
@@ -198,46 +197,6 @@ static const struct clk_ops lochnagar_clk_ops = {
 	.get_parent = lochnagar_clk_get_parent,
 };
 
-static int lochnagar_init_parents(struct lochnagar_clk_priv *priv)
-{
-	struct device_node *np = priv->dev->of_node;
-	int i, j;
-
-	switch (priv->type) {
-	case LOCHNAGAR1:
-		memcpy(priv->lclks, lochnagar1_clks, sizeof(lochnagar1_clks));
-
-		priv->nparents = ARRAY_SIZE(lochnagar1_clk_parents);
-		priv->parents = devm_kmemdup(priv->dev, lochnagar1_clk_parents,
-					     sizeof(lochnagar1_clk_parents),
-					     GFP_KERNEL);
-		break;
-	case LOCHNAGAR2:
-		memcpy(priv->lclks, lochnagar2_clks, sizeof(lochnagar2_clks));
-
-		priv->nparents = ARRAY_SIZE(lochnagar2_clk_parents);
-		priv->parents = devm_kmemdup(priv->dev, lochnagar2_clk_parents,
-					     sizeof(lochnagar2_clk_parents),
-					     GFP_KERNEL);
-		break;
-	default:
-		dev_err(priv->dev, "Unknown Lochnagar type: %d\n", priv->type);
-		return -EINVAL;
-	}
-
-	if (!priv->parents)
-		return -ENOMEM;
-
-	for (i = 0; i < priv->nparents; i++) {
-		j = of_property_match_string(np, "clock-names",
-					     priv->parents[i]);
-		if (j >= 0)
-			priv->parents[i] = of_clk_get_parent_name(np, j);
-	}
-
-	return 0;
-}
-
 static struct clk_hw *
 lochnagar_of_clk_hw_get(struct of_phandle_args *clkspec, void *data)
 {
@@ -252,16 +211,54 @@ lochnagar_of_clk_hw_get(struct of_phandle_args *clkspec, void *data)
 	return &priv->lclks[idx].hw;
 }
 
-static int lochnagar_init_clks(struct lochnagar_clk_priv *priv)
+static const struct of_device_id lochnagar_of_match[] = {
+	{ .compatible = "cirrus,lochnagar1-clk", .data = (void *)LOCHNAGAR1 },
+	{ .compatible = "cirrus,lochnagar2-clk", .data = (void *)LOCHNAGAR2 },
+	{}
+};
+MODULE_DEVICE_TABLE(of, lochnagar_of_match);
+
+static int lochnagar_clk_probe(struct platform_device *pdev)
 {
 	struct clk_init_data clk_init = {
 		.ops = &lochnagar_clk_ops,
-		.parent_names = priv->parents,
-		.num_parents = priv->nparents,
 	};
+	struct device *dev = &pdev->dev;
+	struct lochnagar_clk_priv *priv;
+	const struct of_device_id *of_id;
 	struct lochnagar_clk *lclk;
 	int ret, i;
 
+	of_id = of_match_device(lochnagar_of_match, dev);
+	if (!of_id)
+		return -EINVAL;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = dev;
+	priv->regmap = dev_get_regmap(dev->parent, NULL);
+	priv->type = (enum lochnagar_type)of_id->data;
+
+	switch (priv->type) {
+	case LOCHNAGAR1:
+		memcpy(priv->lclks, lochnagar1_clks, sizeof(lochnagar1_clks));
+
+		clk_init.parent_data = lochnagar1_clk_parents;
+		clk_init.num_parents = ARRAY_SIZE(lochnagar1_clk_parents);
+		break;
+	case LOCHNAGAR2:
+		memcpy(priv->lclks, lochnagar2_clks, sizeof(lochnagar2_clks));
+
+		clk_init.parent_data = lochnagar2_clk_parents;
+		clk_init.num_parents = ARRAY_SIZE(lochnagar2_clk_parents);
+		break;
+	default:
+		dev_err(priv->dev, "Unknown Lochnagar type: %d\n", priv->type);
+		return -EINVAL;
+	}
+
 	for (i = 0; i < ARRAY_SIZE(priv->lclks); i++) {
 		lclk = &priv->lclks[i];
 
@@ -289,43 +286,6 @@ static int lochnagar_init_clks(struct lochnagar_clk_priv *priv)
 	return ret;
 }
 
-static const struct of_device_id lochnagar_of_match[] = {
-	{ .compatible = "cirrus,lochnagar1-clk", .data = (void *)LOCHNAGAR1 },
-	{ .compatible = "cirrus,lochnagar2-clk", .data = (void *)LOCHNAGAR2 },
-	{}
-};
-MODULE_DEVICE_TABLE(of, lochnagar_of_match);
-
-static int lochnagar_clk_probe(struct platform_device *pdev)
-{
-	struct device *dev = &pdev->dev;
-	struct lochnagar_clk_priv *priv;
-	const struct of_device_id *of_id;
-	int ret;
-
-	of_id = of_match_device(lochnagar_of_match, dev);
-	if (!of_id)
-		return -EINVAL;
-
-	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
-	if (!priv)
-		return -ENOMEM;
-
-	priv->dev = dev;
-	priv->regmap = dev_get_regmap(dev->parent, NULL);
-	priv->type = (enum lochnagar_type)of_id->data;
-
-	ret = lochnagar_init_parents(priv);
-	if (ret)
-		return ret;
-
-	ret = lochnagar_init_clks(priv);
-	if (ret)
-		return ret;
-
-	return 0;
-}
-
 static struct platform_driver lochnagar_clk_driver = {
 	.driver = {
 		.name = "lochnagar-clk",
Signature · 1 line
2.11.0
maintainer
#2 lore raw

Re: [PATCH 2/2] clk: lochnagar: Use new parent_data approach to register clock parents

Quoting Charles Keepax (2019-04-30 07:44:12)

Quoted 17 lines
drivers/clk/clk-lochnagar.c +0 -6
index ced832e82f46b..58576db6e31a3 100644
@@ -42,46 +42,45 @@ struct lochnagar_clk_priv {
        struct regmap *regmap;
        enum lochnagar_type type;
 
-       const char **parents;
-       unsigned int nparents;
-
        struct lochnagar_clk lclks[LOCHNAGAR_NUM_CLOCKS];
 };
 
-static const char * const lochnagar1_clk_parents[] = {
-       "ln-none",
-       "ln-spdif-mclk",

drivers/clk/clk-lochnagar.c struct lochnagar_clk_priv { · line 48

Quoted 10 lines
drivers/clk/clk-lochnagar.c +1 -9
-       "ln-psia1-mclk",
-       "ln-psia2-mclk",
-       "ln-cdc-clkout",
-       "ln-dsp-clkout",
-       "ln-pmic-32k",
-       "ln-gf-mclk1",
-       "ln-gf-mclk3",
-       "ln-gf-mclk2",
-       "ln-gf-mclk4",
+#define LN_PARENT(NAME) { .name = NAME, .fw_name = NAME }

Is the name in DT really the same as the globally unique name of the clk? I wouldn't expect that to be the case.

Quoted 59 lines
drivers/clk/clk-lochnagar.c +33 -20
+
+static const struct clk_parent_data lochnagar1_clk_parents[] = {
+       LN_PARENT("ln-none"),
+       LN_PARENT("ln-spdif-mclk"),
+       LN_PARENT("ln-psia1-mclk"),
+       LN_PARENT("ln-psia2-mclk"),
+       LN_PARENT("ln-cdc-clkout"),
+       LN_PARENT("ln-dsp-clkout"),
+       LN_PARENT("ln-pmic-32k"),
+       LN_PARENT("ln-gf-mclk1"),
+       LN_PARENT("ln-gf-mclk3"),
+       LN_PARENT("ln-gf-mclk2"),
+       LN_PARENT("ln-gf-mclk4"),
 };
 
-static const char * const lochnagar2_clk_parents[] = {
-       "ln-none",
-       "ln-cdc-clkout",
-       "ln-dsp-clkout",
-       "ln-pmic-32k",
-       "ln-spdif-mclk",
-       "ln-clk-12m",
-       "ln-clk-11m",
-       "ln-clk-24m",
-       "ln-clk-22m",
-       "ln-clk-8m",
-       "ln-usb-clk-24m",
-       "ln-gf-mclk1",
-       "ln-gf-mclk3",
-       "ln-gf-mclk2",
-       "ln-psia1-mclk",
-       "ln-psia2-mclk",
-       "ln-spdif-clkout",
-       "ln-adat-mclk",
-       "ln-usb-clk-12m",
+static const struct clk_parent_data lochnagar2_clk_parents[] = {
+       LN_PARENT("ln-none"),
+       LN_PARENT("ln-cdc-clkout"),
+       LN_PARENT("ln-dsp-clkout"),
+       LN_PARENT("ln-pmic-32k"),
+       LN_PARENT("ln-spdif-mclk"),
+       LN_PARENT("ln-clk-12m"),
+       LN_PARENT("ln-clk-11m"),
+       LN_PARENT("ln-clk-24m"),
+       LN_PARENT("ln-clk-22m"),
+       LN_PARENT("ln-clk-8m"),
+       LN_PARENT("ln-usb-clk-24m"),
+       LN_PARENT("ln-gf-mclk1"),
+       LN_PARENT("ln-gf-mclk3"),
+       LN_PARENT("ln-gf-mclk2"),
+       LN_PARENT("ln-psia1-mclk"),
+       LN_PARENT("ln-psia2-mclk"),
+       LN_PARENT("ln-spdif-clkout"),
+       LN_PARENT("ln-adat-mclk"),
+       LN_PARENT("ln-usb-clk-12m"),
 };
 
 #define LN1_CLK(ID, NAME, REG) \

drivers/clk/clk-lochnagar.c lochnagar_of_clk_hw_get(struct of_phandle_args *clkspec, void *data) · line 215

Quoted 6 lines
drivers/clk/clk-lochnagar.c +2 -1
@@ -252,16 +211,54 @@ lochnagar_of_clk_hw_get(struct of_phandle_args *clkspec, void *data)
        return &priv->lclks[idx].hw;
 }
 
-static int lochnagar_init_clks(struct lochnagar_clk_priv *priv)
+static const struct of_device_id lochnagar_of_match[] = {
+       { .compatible = "cirrus,lochnagar1-clk", .data = (void *)LOCHNAGAR1 },

Maybe this enum should just be a structure of parents and the size of the parents?

Quoted 4 lines
drivers/clk/clk-lochnagar.c +4 -0
+       { .compatible = "cirrus,lochnagar2-clk", .data = (void *)LOCHNAGAR2 },
+       {}
+};
+MODULE_DEVICE_TABLE(of, lochnagar_of_match);
maintainer
#3 lore raw
In reply to Charles Keepax

Quoting Charles Keepax (2019-04-30 07:44:11)

Quoted 16 lines

clk_core_fill_parent_index is called from clk_mux_determine_rate_flags and for the initial parent of the clock but seems to not get called on the path changing a clocks parent. This can cause a clock parent change to fail, fix this by adding a call in clk_fetch_parent_index.

Fixes fc0c209c147f ("clk: Allow parents to be specified without string names")
---
 drivers/clk/clk.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/clk/clk.c +0 -0
index ffd33b63c37eb..5aa180180ee95 100644

drivers/clk/clk.c static int clk_fetch_parent_index(struct clk_core *core, · line 1606

Quoted 6 lines
drivers/clk/clk.c +3 -0
@@ -1601,6 +1601,9 @@ static int clk_fetch_parent_index(struct clk_core *core,
                return -EINVAL;
 
        for (i = 0; i < core->num_parents; i++) {
+               if (!core->parents[i].core)
+                       clk_core_fill_parent_index(core, i);
+

Hm... are you not specifying 'names' for the parent, so just clk_hw pointer? Maybe we need to compare clk_hw pointers with clk_hw pointers and then fill in the core pointer with what we have in hand. Pretty much at all costs we shouldn't call clk_core_fill_parent_index() here because drivers may fall into the trap of searching the entire clk tree for a pointer we already have.

drivers/clk/clk.c +5 -0
index 114f0bffd630..c4fa341330fa 100644
@@ -1619,6 +1619,11 @@ static int clk_fetch_parent_index(struct clk_core *core,
 		if (core->parents[i].core)
 			continue;
 
+		if (core->parents[i].hw == parent->hw) {
+			core->parents[i].core = parent;
+			return i;
+		}
+
 		/* Fallback to comparing globally unique names */
 		if (!strcmp(parent->name, core->parents[i].name)) {
 			core->parents[i].core = parent;
author maintainer
#4 lore raw
Quoted 19 lines · Stephen Boyd

Quoting Charles Keepax (2019-04-30 07:44:11)

Quoted 18 lines

clk_core_fill_parent_index is called from clk_mux_determine_rate_flags and for the initial parent of the clock but seems to not get called on the path changing a clocks parent. This can cause a clock parent change to fail, fix this by adding a call in clk_fetch_parent_index.

Fixes fc0c209c147f ("clk: Allow parents to be specified without string names")
---
 drivers/clk/clk.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/clk/clk.c +0 -0
index ffd33b63c37eb..5aa180180ee95 100644
@@ -1601,6 +1601,9 @@ static int clk_fetch_parent_index(struct clk_core *core,
                return -EINVAL;
 
Quoted 12 lines

drivers/clk/clk.c static int clk_fetch_parent_index(struct clk_core *core, · line 1606

Quoted 4 lines
drivers/clk/clk.c +3 -0
        for (i = 0; i < core->num_parents; i++) {
+               if (!core->parents[i].core)
+                       clk_core_fill_parent_index(core, i);
+

Hm... are you not specifying 'names' for the parent, so just clk_hw pointer? Maybe we need to compare clk_hw pointers with clk_hw pointers and then fill in the core pointer with what we have in hand. Pretty much at all costs we shouldn't call clk_core_fill_parent_index() here because drivers may fall into the trap of searching the entire clk tree for a pointer we already have.

Apologies perhaps I am misunderstanding how this new system works. In the event of the parent clocks being specified in DT, whilst going round this loop would you expect the clock to match on the core == parent check? Or on the fallback unique name check? My assumption was on the core == parent check, and if that is the case how would you expect the parents[i].core field to have been populated?

Thanks,
Charles

maintainer
#5 lore raw

Quoting Charles Keepax (2019-05-01 01:33:17)

Quoted 33 lines · Stephen Boyd
Quoted 19 lines

Quoting Charles Keepax (2019-04-30 07:44:11)

Quoted 18 lines

clk_core_fill_parent_index is called from clk_mux_determine_rate_flags and for the initial parent of the clock but seems to not get called on the path changing a clocks parent. This can cause a clock parent change to fail, fix this by adding a call in clk_fetch_parent_index.

Fixes fc0c209c147f ("clk: Allow parents to be specified without string names")
---
 drivers/clk/clk.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/clk/clk.c +0 -0
index ffd33b63c37eb..5aa180180ee95 100644
@@ -1601,6 +1601,9 @@ static int clk_fetch_parent_index(struct clk_core *core,
                return -EINVAL;
 
Quoted 12 lines

drivers/clk/clk.c static int clk_fetch_parent_index(struct clk_core *core, · line 1606

Quoted 4 lines
drivers/clk/clk.c +3 -0
        for (i = 0; i < core->num_parents; i++) {
+               if (!core->parents[i].core)
+                       clk_core_fill_parent_index(core, i);
+

Hm... are you not specifying 'names' for the parent, so just clk_hw pointer? Maybe we need to compare clk_hw pointers with clk_hw pointers and then fill in the core pointer with what we have in hand. Pretty much at all costs we shouldn't call clk_core_fill_parent_index() here because drivers may fall into the trap of searching the entire clk tree for a pointer we already have.

Quoted 8 lines

Apologies perhaps I am misunderstanding how this new system works. In the event of the parent clocks being specified in DT, whilst going round this loop would you expect the clock to match on the core == parent check? Or on the fallback unique name check? My assumption was on the core == parent check, and if that is the case how would you expect the parents[i].core field to have been populated?

I don't expect parents[i].core to be populated until we do the fallback string match in this function when the globally unique names match or if it's already populated by some other path calling clk_core_fill_parent_index(). The problem is we just fixed a long standing regression in this function with commit ede77858473a ("clk:
Remove global clk traversal on fetch parent index"). Calling clk_core_fill_parent_index() will bring that performance problem back, so we need to figure out how to find the index for a clk without doing the global search.

It seems that some clk providers specify parents that may never exist, so we'll possibly spend time looping through the parents each time doing a global string comparison on hundreds of clks. It would be best to avoid that, so we shouldn't really do any sort of caching here except for the one clk_core pointer we already have passed in.

So you're saying this happens in the clk_set_parent() path, where the parent passed into this function has never been cached before? Otherwise, it looks like the only other case where it might be a problem would be if a .round_rate() or .determine_rate() clk_op returns a parent that hasn't been cached yet, maybe via hard-coding of the parent hw pointer or just because they don't call clk_hw_get_parent_by_index(). I'd rather not cater to those corner cases by trying to optimize them, so perhaps we can use this patch and avoid the regression while supporting the use-cases we need?

----8<-----

drivers/clk/clk.c +22 -5
index dc05cb339761..7ad1751bf2b4 100644
@@ -327,8 +327,7 @@ static struct clk_core *clk_core_lookup(const char *name)
 /**
  * clk_core_get - Find the clk_core parent of a clk
  * @core: clk to find parent of
- * @name: name to search for (if string based)
- * @index: index to use for search (if DT index based)
+ * @p_index: parent index to search for
  *
  * 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
@@ -360,9 +359,10 @@ static struct clk_core *clk_core_lookup(const char *name)
  * 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,
-				     int index)
+static struct clk_core *clk_core_get(struct clk_core *core, u8 p_index)
 {
+	const char *name = core->parents[p_index].fw_name;
+	u8 index = core->parents[p_index].index;
 	struct clk_hw *hw = ERR_PTR(-ENOENT);
 	struct device *dev = core->dev;
 	const char *dev_id = dev ? dev_name(dev) : NULL;
@@ -400,7 +400,7 @@ static void clk_core_fill_parent_index(struct clk_core *core, u8 index)
 		if (!parent)
 			parent = ERR_PTR(-EPROBE_DEFER);
 	} else {
-		parent = clk_core_get(core, entry->fw_name, entry->index);
+		parent = clk_core_get(core, index);
 		if (IS_ERR(parent) && PTR_ERR(parent) == -ENOENT)
 			parent = clk_core_lookup(entry->name);
 	}
@@ -1612,12 +1612,29 @@ static int clk_fetch_parent_index(struct clk_core *core,
 		return -EINVAL;
 
 	for (i = 0; i < core->num_parents; i++) {
+		/* Found it first try! */
 		if (core->parents[i].core == parent)
 			return i;
 
+		/* Something else is here, so keep looking */
 		if (core->parents[i].core)
 			continue;
 
+		/* Maybe core hasn't been cached but the hw is all we know? */
+		if (core->parents[i].hw) {
+			if (core->parents[i].hw == parent->hw) {
+				core->parents[i].core = parent;
+				return i;
+			}
+
+			/* Didn't match, but we're expecting a clk_hw */
+			continue;
+		}
+
+		/* Maybe it hasn't been cached (clk_set_parent() path) */
+		if (parent == clk_core_get(core, i))
+			return i;
+
 		/* Fallback to comparing globally unique names */
 		if (!strcmp(parent->name, core->parents[i].name)) {
 			core->parents[i].core = parent;
author maintainer
#6 lore raw
Quoted 3 lines · Stephen Boyd

Quoting Charles Keepax (2019-05-01 01:33:17)

Quoted 2 lines · Stephen Boyd
Quoted 1 line

Quoting Charles Keepax (2019-04-30 07:44:11)

Thank you for the explanation think I am starting to get there.

Quoted 10 lines

I don't expect parents[i].core to be populated until we do the fallback string match in this function when the globally unique names match or if it's already populated by some other path calling clk_core_fill_parent_index(). The problem is we just fixed a long standing regression in this function with commit ede77858473a ("clk:
Remove global clk traversal on fetch parent index"). Calling clk_core_fill_parent_index() will bring that performance problem back, so we need to figure out how to find the index for a clk without doing the global search.

There is a slight error in the commit message there I think, the change doesn't affect clk_mux_determine_rate_flags which still calls clk_core_get_parent_by_index so will still do the full lookup. It looks like it affects clk_calc_new_rates instead.

Which I guess does raise the question would an optimisation on the determine_rate path help these power issues as well?

Quoted 8 lines

It seems that some clk providers specify parents that may never exist, so we'll possibly spend time looping through the parents each time doing a global string comparison on hundreds of clks. It would be best to avoid that, so we shouldn't really do any sort of caching here except for the one clk_core pointer we already have passed in.

So you're saying this happens in the clk_set_parent() path, where the parent passed into this function has never been cached before?

Yeah that seems to be what is happening in my case. As best I can figure out so far, this relates to our clocks not having any rate setting capabilities, as it looks like most of the cache population comes from those paths.

drivers/clk/clk.c static int clk_fetch_parent_index(struct clk_core *core, · line 1636

Quoted 4 lines
drivers/clk/clk.c +4 -0
+
+		/* Maybe it hasn't been cached (clk_set_parent() path) */
+		if (parent == clk_core_get(core, i))
+			return i;

This part does fix my issue. Is there a reason not to update core->parents[i].core on this path?

Thanks,
Charles

maintainer
#7 lore raw

Quoting Charles Keepax (2019-05-03 09:33:22)

Quoted 18 lines · Stephen Boyd
Quoted 3 lines

Quoting Charles Keepax (2019-05-01 01:33:17)

Quoted 2 lines · Stephen Boyd
Quoted 1 line

Quoting Charles Keepax (2019-04-30 07:44:11)

Thank you for the explanation think I am starting to get there.

Quoted 10 lines

I don't expect parents[i].core to be populated until we do the fallback string match in this function when the globally unique names match or if it's already populated by some other path calling clk_core_fill_parent_index(). The problem is we just fixed a long standing regression in this function with commit ede77858473a ("clk:
Remove global clk traversal on fetch parent index"). Calling clk_core_fill_parent_index() will bring that performance problem back, so we need to figure out how to find the index for a clk without doing the global search.

Quoted 7 lines

There is a slight error in the commit message there I think, the change doesn't affect clk_mux_determine_rate_flags which still calls clk_core_get_parent_by_index so will still do the full lookup. It looks like it affects clk_calc_new_rates instead.

Which I guess does raise the question would an optimisation on the determine_rate path help these power issues as well?

Yes I would think that determine_rate() is iterating over parents and trying to find them again and again even when there are holes and that's probably wasting time. I hope we can fill the parent cache of a clk with the error pointer indicating the clk will never appear (-ENOENT?) and then skip over it in this case.

Quoted 7 lines
Quoted 7 lines

It seems that some clk providers specify parents that may never exist, so we'll possibly spend time looping through the parents each time doing a global string comparison on hundreds of clks. It would be best to avoid that, so we shouldn't really do any sort of caching here except for the one clk_core pointer we already have passed in.

Quoted 7 lines
Quoted 2 lines

So you're saying this happens in the clk_set_parent() path, where the parent passed into this function has never been cached before?

Yeah that seems to be what is happening in my case. As best I can figure out so far, this relates to our clocks not having any rate setting capabilities, as it looks like most of the cache population comes from those paths.

Makes sense!

Quoted 9 lines

Review

Quoted 5 lines
(quoted diff) +4 -0
+
+             /* Maybe it hasn't been cached (clk_set_parent() path) */
+             if (parent == clk_core_get(core, i))
+                     return i;

This part does fix my issue. Is there a reason not to update core->parents[i].core on this path?

Nope. I though clk_core_get() did it already, but it doesn't. Thanks for catching it. Find the new patch attached:

---8<---
Subject: [PATCH] clk: Cache core in clk_fetch_parent_index() without names

If a clk has specified parents via clk_hw pointers it won't specify the globally unique names for the parents. Without the unique names, we can't fallback to comparing them against the name of the 'parent' pointer here. Therefore, do a pointer comparison against the clk_hw pointers too and cache the clk_core structure if they match. This fixes parent lookup code for clks that only specify clk_hw pointers and nothing else, like muxes that are purely inside a clk controller.

Similarly, if the parent pointer isn't cached after trying to match clk_core or clk_hw pointers, lookup the pointer from DT or via clkdev lookups instead of relying purely on the globally unique clk name match. This should allow us to move away from having to specify global names for clk parents entirely.

While we're in the area, add some comments so it's clearer what's going on. The if statements don't lend themselves to much clarity in their raw form.

Fixes fc0c209c147f ("clk: Allow parents to be specified without string names")
---
 drivers/clk/clk.c | 37 +++++++++++++++++++++++++++----------
 1 file changed, 27 insertions(+), 10 deletions(-)
drivers/clk/clk.c +27 -10
index dc05cb339761..dcb7e1cddd2d 100644
@@ -327,8 +327,7 @@ static struct clk_core *clk_core_lookup(const char *name)
 /**
  * clk_core_get - Find the clk_core parent of a clk
  * @core: clk to find parent of
- * @name: name to search for (if string based)
- * @index: index to use for search (if DT index based)
+ * @p_index: parent index to search for
  *
  * 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
@@ -360,9 +359,10 @@ static struct clk_core *clk_core_lookup(const char *name)
  * 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,
-				     int index)
+static struct clk_core *clk_core_get(struct clk_core *core, u8 p_index)
 {
+	const char *name = core->parents[p_index].fw_name;
+	int index = core->parents[p_index].index;
 	struct clk_hw *hw = ERR_PTR(-ENOENT);
 	struct device *dev = core->dev;
 	const char *dev_id = dev ? dev_name(dev) : NULL;
@@ -400,7 +400,7 @@ static void clk_core_fill_parent_index(struct clk_core *core, u8 index)
 		if (!parent)
 			parent = ERR_PTR(-EPROBE_DEFER);
 	} else {
-		parent = clk_core_get(core, entry->fw_name, entry->index);
+		parent = clk_core_get(core, index);
 		if (IS_ERR(parent) && PTR_ERR(parent) == -ENOENT)
 			parent = clk_core_lookup(entry->name);
 	}
@@ -1612,20 +1612,37 @@ static int clk_fetch_parent_index(struct clk_core *core,
 		return -EINVAL;
 
 	for (i = 0; i < core->num_parents; i++) {
+		/* Found it first try! */
 		if (core->parents[i].core == parent)
 			return i;
 
+		/* Something else is here, so keep looking */
 		if (core->parents[i].core)
 			continue;
 
-		/* Fallback to comparing globally unique names */
-		if (!strcmp(parent->name, core->parents[i].name)) {
-			core->parents[i].core = parent;
-			return i;
+		/* Maybe core hasn't been cached but the hw is all we know? */
+		if (core->parents[i].hw) {
+			if (core->parents[i].hw == parent->hw)
+				break;
+
+			/* Didn't match, but we're expecting a clk_hw */
+			continue;
 		}
+
+		/* Maybe it hasn't been cached (clk_set_parent() path) */
+		if (parent == clk_core_get(core, i))
+			break;
+
+		/* Fallback to comparing globally unique names */
+		if (!strcmp(parent->name, core->parents[i].name))
+			break;
 	}
 
-	return -EINVAL;
+	if (i == core->num_parents)
+		return -EINVAL;
+
+	core->parents[i].core = parent;
+	return i;
 }
 
 /*
Signature · 1 line
Sent by a computer through tubes