[PATCH 0/3] clk: keystone: a few TI sci-clk improvements

[PATCH 0/3] clk: keystone: a few TI sci-clk improvements

[Tero Kristo]

Hi,

Patch 1 & 2 introduce a new DT flag for parsing the clock data from devicetree
instead of querying everything from firmware. The purpose of this change
is to improve boot time, it can be seen to give ~2s improvement in certain
setups. If adding a new DT flag for this is not acceptable, a kernel command
line parameter could be introduced instead, or even a new Kconfig option.
I voted for the DT flag for now as that seems to be the most versatile
solution.

Patch 3 shortens the kernel registered clock names, currently for am65x
they are of the form:

interconnect@100000:interconnect@28380000:interconnect@42040000:dmsc:clocks:11:1
0

After this patch the same clock name is cut down to:

clocks:11:10

This makes the debugfs interface much more readable in addition to some
memory savings.

-Tero

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH 1/3] dt-bindings: clock: ti,sci-clk: Add support for parsing clock info from DT

[Tero Kristo]

By default, the available clock info is queried from firmware, which can
be quite a lengthy operation if there is a very large amount of clocks
available. Add option for parsing the used clocks from DT instead, and
only register these which can improve the boot time of the device quite
a lot.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 Documentation/devicetree/bindings/clock/ti,sci-clk.txt | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/ti,sci-clk.txt b/Documentation/devicetree/bindings/clock/ti,sci-clk.txt
index 4e59dc6..c757ae1 100644
--- a/Documentation/devicetree/bindings/clock/ti,sci-clk.txt
+++ b/Documentation/devicetree/bindings/clock/ti,sci-clk.txt
@@ -18,6 +18,13 @@ Required properties:
   and clocks IDs for 66AK2G SoC are documented at
   http://processors.wiki.ti.com/index.php/TISCI#66AK2G02_Data
 
+Optional properties:
+-------------------
+- ti,scan-clocks-from-dt: Scan clock tree info from DT. By default,
+  clocks are queried from firmware, which can be rather slow operation,
+  especially if there is a really large number of clocks available out
+  of which only a handful are ever used by kernel.
+
 Examples:
 --------
 
-- 
1.9.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH 2/3] clk: keystone: sci-clk: add support from parsing clock info from DT

[Tero Kristo]

Currently the sci-clk driver queries clock data from the firmware, which
can be pretty slow operation in certain cases. Add an option for the
driver to probe the available clocks from DT also, in this case the
number of clocks probed from firmware gets cut down drastically. To
enable the feature, a new flag ti,scan-clocks-from-dt can be added
under the clock provider node.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
Tested-by: Andreas Dannenberg <dannenberg@ti.com>
---
 drivers/clk/keystone/sci-clk.c | 188 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 159 insertions(+), 29 deletions(-)

diff --git a/drivers/clk/keystone/sci-clk.c b/drivers/clk/keystone/sci-clk.c
index 4cb70be..d1d6dc8 100644
--- a/drivers/clk/keystone/sci-clk.c
+++ b/drivers/clk/keystone/sci-clk.c
@@ -23,6 +23,7 @@
 #include <linux/slab.h>
 #include <linux/soc/ti/ti_sci_protocol.h>
 #include <linux/bsearch.h>
+#include <linux/list_sort.h>
 
 #define SCI_CLK_SSC_ENABLE		BIT(0)
 #define SCI_CLK_ALLOW_FREQ_CHANGE	BIT(1)
@@ -52,6 +53,7 @@ struct sci_clk_provider {
  * @num_parents: Number of parents for this clock
  * @provider:	 Master clock provider
  * @flags:	 Flags for the clock
+ * @node:	 Link for handling clocks probed via DT
  */
 struct sci_clk {
 	struct clk_hw hw;
@@ -60,6 +62,7 @@ struct sci_clk {
 	u8 num_parents;
 	struct sci_clk_provider *provider;
 	u8 flags;
+	struct list_head node;
 };
 
 #define to_sci_clk(_hw) container_of(_hw, struct sci_clk, hw)
@@ -353,6 +356,15 @@ static int _cmp_sci_clk(const void *a, const void *b)
 	return -1;
 }
 
+static int _cmp_sci_clk_list(void *priv, struct list_head *a,
+			     struct list_head *b)
+{
+	struct sci_clk *ca = container_of(a, struct sci_clk, node);
+	struct sci_clk *cb = container_of(b, struct sci_clk, node);
+
+	return _cmp_sci_clk(ca, &cb);
+}
+
 /**
  * sci_clk_get - Xlate function for getting clock handles
  * @clkspec: device tree clock specifier
@@ -404,22 +416,8 @@ static int ti_sci_init_clocks(struct sci_clk_provider *p)
 };
 MODULE_DEVICE_TABLE(of, ti_sci_clk_of_match);
 
-/**
- * ti_sci_clk_probe - Probe function for the TI SCI clock driver
- * @pdev: platform device pointer to be probed
- *
- * Probes the TI SCI clock device. Allocates a new clock provider
- * and registers this to the common clock framework. Also applies
- * any required flags to the identified clocks via clock lists
- * supplied from DT. Returns 0 for success, negative error value
- * for failure.
- */
-static int ti_sci_clk_probe(struct platform_device *pdev)
+static int ti_sci_scan_clocks_from_fw(struct sci_clk_provider *provider)
 {
-	struct device *dev = &pdev->dev;
-	struct device_node *np = dev->of_node;
-	struct sci_clk_provider *provider;
-	const struct ti_sci_handle *handle;
 	int ret;
 	int num_clks = 0;
 	struct sci_clk **clks = NULL;
@@ -430,18 +428,7 @@ static int ti_sci_clk_probe(struct platform_device *pdev)
 	int dev_id = 0;
 	u8 num_parents;
 	int gap_size = 0;
-
-	handle = devm_ti_sci_get_handle(dev);
-	if (IS_ERR(handle))
-		return PTR_ERR(handle);
-
-	provider = devm_kzalloc(dev, sizeof(*provider), GFP_KERNEL);
-	if (!provider)
-		return -ENOMEM;
-
-	provider->sci = handle;
-	provider->ops = &handle->ops.clk_ops;
-	provider->dev = dev;
+	struct device *dev = provider->dev;
 
 	while (1) {
 		ret = provider->ops->get_num_parents(provider->sci, dev_id,
@@ -502,13 +489,156 @@ static int ti_sci_clk_probe(struct platform_device *pdev)
 
 	devm_kfree(dev, clks);
 
+	return 0;
+}
+
+static int ti_sci_scan_clocks_from_dt(struct sci_clk_provider *provider)
+{
+	struct device *dev = provider->dev;
+	struct device_node *np = NULL;
+	int ret;
+	int index;
+	struct of_phandle_args args;
+	struct list_head clks;
+	struct sci_clk *sci_clk, *prev;
+	int num_clks = 0;
+	int num_parents;
+	int clk_id;
+
+	INIT_LIST_HEAD(&clks);
+
+	while (1) {
+		np = of_find_node_with_property(np, "clocks");
+		if (!np)
+			break;
+
+		index = 0;
+
+		do {
+			ret = of_parse_phandle_with_args(np, "clocks",
+							 "#clock-cells", index,
+							 &args);
+			if (ret)
+				break;
+
+			if (args.args_count == 2 && args.np == dev->of_node) {
+				sci_clk = devm_kzalloc(dev, sizeof(*sci_clk),
+						       GFP_KERNEL);
+				if (!sci_clk)
+					return -ENOMEM;
+
+				sci_clk->dev_id = args.args[0];
+				sci_clk->clk_id = args.args[1];
+				sci_clk->provider = provider;
+				provider->ops->
+					get_num_parents(provider->sci,
+							sci_clk->dev_id,
+							sci_clk->clk_id,
+							&sci_clk->num_parents);
+				list_add_tail(&sci_clk->node, &clks);
+
+				num_clks++;
+
+				num_parents = sci_clk->num_parents;
+				if (num_parents == 1)
+					num_parents = 0;
+
+				clk_id = args.args[1] + 1;
+
+				while (num_parents--) {
+					sci_clk = devm_kzalloc(dev,
+							       sizeof(*sci_clk),
+							       GFP_KERNEL);
+					if (!sci_clk)
+						return -ENOMEM;
+					sci_clk->dev_id = args.args[0];
+					sci_clk->clk_id = clk_id++;
+					sci_clk->provider = provider;
+					list_add_tail(&sci_clk->node, &clks);
+
+					num_clks++;
+				}
+			}
+
+			index++;
+		} while (args.np);
+	}
+
+	list_sort(NULL, &clks, _cmp_sci_clk_list);
+
+	provider->clocks = devm_kmalloc_array(dev, num_clks, sizeof(sci_clk),
+					      GFP_KERNEL);
+	if (!provider->clocks)
+		return -ENOMEM;
+
+	num_clks = 0;
+	prev = NULL;
+
+	list_for_each_entry(sci_clk, &clks, node) {
+		if (prev && prev->dev_id == sci_clk->dev_id &&
+		    prev->clk_id == sci_clk->clk_id)
+			continue;
+
+		provider->clocks[num_clks++] = sci_clk;
+		prev = sci_clk;
+	}
+
+	provider->num_clocks = num_clks;
+
+	return 0;
+}
+
+/**
+ * ti_sci_clk_probe - Probe function for the TI SCI clock driver
+ * @pdev: platform device pointer to be probed
+ *
+ * Probes the TI SCI clock device. Allocates a new clock provider
+ * and registers this to the common clock framework. Also applies
+ * any required flags to the identified clocks via clock lists
+ * supplied from DT. Returns 0 for success, negative error value
+ * for failure.
+ */
+static int ti_sci_clk_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct sci_clk_provider *provider;
+	const struct ti_sci_handle *handle;
+	int ret;
+
+	handle = devm_ti_sci_get_handle(dev);
+	if (IS_ERR(handle))
+		return PTR_ERR(handle);
+
+	provider = devm_kzalloc(dev, sizeof(*provider), GFP_KERNEL);
+	if (!provider)
+		return -ENOMEM;
+
+	provider->sci = handle;
+	provider->ops = &handle->ops.clk_ops;
+	provider->dev = dev;
+
+	if (of_property_read_bool(np, "ti,scan-clocks-from-dt")) {
+		ret = ti_sci_scan_clocks_from_dt(provider);
+		if (ret) {
+			dev_err(dev, "scan clocks from DT failed: %d\n", ret);
+			return ret;
+		}
+	} else {
+		ret = ti_sci_scan_clocks_from_fw(provider);
+		if (ret) {
+			dev_err(dev, "scan clocks from FW failed: %d\n", ret);
+			return ret;
+		}
+	}
+
 	ret = ti_sci_init_clocks(provider);
 	if (ret) {
-		pr_err("ti-sci-init-clocks failed.\n");
+		dev_err(dev, "init-clocks failed: %d\n", ret);
 		return ret;
 	}
 
-	return of_clk_add_hw_provider(np, sci_clk_get, provider);
+	return of_clk_add_hw_provider(dev->of_node, sci_clk_get, provider);
 }
 
 /**
-- 
1.9.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH 3/3] clk: keystone: sci-clk: use shorter names for clocks

[Tero Kristo]

Currently the full device name of the clock provider is used as the initial
part of the registered clock name. This is pretty long and completely
unnecessary info in generic case, so shorten it up. Now, we only contain
the short name of the clock provider node (pOFn.)

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 drivers/clk/keystone/sci-clk.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/keystone/sci-clk.c b/drivers/clk/keystone/sci-clk.c
index d1d6dc8..57b7d02 100644
--- a/drivers/clk/keystone/sci-clk.c
+++ b/drivers/clk/keystone/sci-clk.c
@@ -283,7 +283,7 @@ static int _sci_clk_build(struct sci_clk_provider *provider,
 	int i;
 	int ret = 0;
 
-	name = kasprintf(GFP_KERNEL, "%s:%d:%d", dev_name(provider->dev),
+	name = kasprintf(GFP_KERNEL, "%pOFn:%d:%d", provider->dev->of_node,
 			 sci_clk->dev_id, sci_clk->clk_id);
 
 	init.name = name;
@@ -309,8 +309,8 @@ static int _sci_clk_build(struct sci_clk_provider *provider,
 		for (i = 0; i < sci_clk->num_parents; i++) {
 			char *parent_name;
 
-			parent_name = kasprintf(GFP_KERNEL, "%s:%d:%d",
-						dev_name(provider->dev),
+			parent_name = kasprintf(GFP_KERNEL, "%pOFn:%d:%d",
+						provider->dev->of_node,
 						sci_clk->dev_id,
 						sci_clk->clk_id + 1 + i);
 			if (!parent_name) {
-- 
1.9.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH 2/3] clk: keystone: sci-clk: add support from parsing clock info from DT

[Rob Herring]

On Tue, Jan 08, 2019 at 03:30:22PM +0200, Tero Kristo wrote:
> Currently the sci-clk driver queries clock data from the firmware, which
> can be pretty slow operation in certain cases. Add an option for the
> driver to probe the available clocks from DT also, in this case the
> number of clocks probed from firmware gets cut down drastically. To
> enable the feature, a new flag ti,scan-clocks-from-dt can be added
> under the clock provider node.
> 
> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> Tested-by: Andreas Dannenberg <dannenberg@ti.com>
> ---
>  drivers/clk/keystone/sci-clk.c | 188 ++++++++++++++++++++++++++++++++++-------
>  1 file changed, 159 insertions(+), 29 deletions(-)


> +static int ti_sci_scan_clocks_from_dt(struct sci_clk_provider *provider)
> +{
> +	struct device *dev = provider->dev;
> +	struct device_node *np = NULL;
> +	int ret;
> +	int index;
> +	struct of_phandle_args args;
> +	struct list_head clks;
> +	struct sci_clk *sci_clk, *prev;
> +	int num_clks = 0;
> +	int num_parents;
> +	int clk_id;
> +
> +	INIT_LIST_HEAD(&clks);
> +
> +	while (1) {
> +		np = of_find_node_with_property(np, "clocks");
> +		if (!np)
> +			break;

for_each_node_with_property

> +
> +		index = 0;
> +
> +		do {
> +			ret = of_parse_phandle_with_args(np, "clocks",
> +							 "#clock-cells", index,
> +							 &args);
> +			if (ret)
> +				break;

of_for_each_phandle can be used here. And then of_phandle_iterator_args 
will get the args.

> +
> +			if (args.args_count == 2 && args.np == dev->of_node) {

dtc will warn if the # of cells are wrong, so that's probably redundant.

Invert the if and save a level of indentation.

> +				sci_clk = devm_kzalloc(dev, sizeof(*sci_clk),
> +						       GFP_KERNEL);
> +				if (!sci_clk)
> +					return -ENOMEM;
> +
> +				sci_clk->dev_id = args.args[0];
> +				sci_clk->clk_id = args.args[1];
> +				sci_clk->provider = provider;
> +				provider->ops->
> +					get_num_parents(provider->sci,
> +							sci_clk->dev_id,
> +							sci_clk->clk_id,
> +							&sci_clk->num_parents);
> +				list_add_tail(&sci_clk->node, &clks);
> +
> +				num_clks++;
> +
> +				num_parents = sci_clk->num_parents;
> +				if (num_parents == 1)
> +					num_parents = 0;
> +
> +				clk_id = args.args[1] + 1;
> +
> +				while (num_parents--) {
> +					sci_clk = devm_kzalloc(dev,
> +							       sizeof(*sci_clk),
> +							       GFP_KERNEL);
> +					if (!sci_clk)
> +						return -ENOMEM;
> +					sci_clk->dev_id = args.args[0];
> +					sci_clk->clk_id = clk_id++;
> +					sci_clk->provider = provider;
> +					list_add_tail(&sci_clk->node, &clks);
> +
> +					num_clks++;
> +				}
> +			}
> +
> +			index++;
> +		} while (args.np);
> +	}
> +
> +	list_sort(NULL, &clks, _cmp_sci_clk_list);
> +
> +	provider->clocks = devm_kmalloc_array(dev, num_clks, sizeof(sci_clk),
> +					      GFP_KERNEL);
> +	if (!provider->clocks)
> +		return -ENOMEM;
> +
> +	num_clks = 0;
> +	prev = NULL;
> +
> +	list_for_each_entry(sci_clk, &clks, node) {
> +		if (prev && prev->dev_id == sci_clk->dev_id &&
> +		    prev->clk_id == sci_clk->clk_id)
> +			continue;
> +
> +		provider->clocks[num_clks++] = sci_clk;
> +		prev = sci_clk;
> +	}
> +
> +	provider->num_clocks = num_clks;
> +
> +	return 0;
> +}

Re: [PATCH 1/3] dt-bindings: clock: ti,sci-clk: Add support for parsing clock info from DT

[Rob Herring]

On Tue, Jan 08, 2019 at 03:30:21PM +0200, Tero Kristo wrote:
> By default, the available clock info is queried from firmware, which can
> be quite a lengthy operation if there is a very large amount of clocks
> available. Add option for parsing the used clocks from DT instead, and
> only register these which can improve the boot time of the device quite
> a lot.
> 
> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> ---
>  Documentation/devicetree/bindings/clock/ti,sci-clk.txt | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/clock/ti,sci-clk.txt b/Documentation/devicetree/bindings/clock/ti,sci-clk.txt
> index 4e59dc6..c757ae1 100644
> --- a/Documentation/devicetree/bindings/clock/ti,sci-clk.txt
> +++ b/Documentation/devicetree/bindings/clock/ti,sci-clk.txt
> @@ -18,6 +18,13 @@ Required properties:
>    and clocks IDs for 66AK2G SoC are documented at
>    http://processors.wiki.ti.com/index.php/TISCI#66AK2G02_Data
>  
> +Optional properties:
> +-------------------
> +- ti,scan-clocks-from-dt: Scan clock tree info from DT. By default,
> +  clocks are queried from firmware, which can be rather slow operation,
> +  especially if there is a really large number of clocks available out
> +  of which only a handful are ever used by kernel.

At first, I thought this was an either/or thing. Use firmware or use DT, 
but it is really only get the clocks used in the DT from firmware.

Why wouldn't you just always do that? I can think of 3 cases: 
reparenting, debug and overlays. This breaks reparenting and overlays, 
right? Debug could be handled with some userspace trigger to get all the 
clocks.

Why scan any of the clocks up front? Why not just create the clocks on 
demand? If an unknown clock id is requested, then create the clock and 
query the firmware at that point. That would avoid the DT scan too. 
Maybe there's some issues in the clk framework preventing that, but 
that's not really a DT problem.

Rob

Re: [PATCH 1/3] dt-bindings: clock: ti,sci-clk: Add support for parsing clock info from DT

[Tero Kristo]

On 21/01/2019 23:04, Rob Herring wrote:
> On Tue, Jan 08, 2019 at 03:30:21PM +0200, Tero Kristo wrote:
>> By default, the available clock info is queried from firmware, which can
>> be quite a lengthy operation if there is a very large amount of clocks
>> available. Add option for parsing the used clocks from DT instead, and
>> only register these which can improve the boot time of the device quite
>> a lot.
>>
>> Signed-off-by: Tero Kristo <t-kristo@ti.com>
>> ---
>>   Documentation/devicetree/bindings/clock/ti,sci-clk.txt | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/clock/ti,sci-clk.txt b/Documentation/devicetree/bindings/clock/ti,sci-clk.txt
>> index 4e59dc6..c757ae1 100644
>> --- a/Documentation/devicetree/bindings/clock/ti,sci-clk.txt
>> +++ b/Documentation/devicetree/bindings/clock/ti,sci-clk.txt
>> @@ -18,6 +18,13 @@ Required properties:
>>     and clocks IDs for 66AK2G SoC are documented at
>>     http://processors.wiki.ti.com/index.php/TISCI#66AK2G02_Data
>>   
>> +Optional properties:
>> +-------------------
>> +- ti,scan-clocks-from-dt: Scan clock tree info from DT. By default,
>> +  clocks are queried from firmware, which can be rather slow operation,
>> +  especially if there is a really large number of clocks available out
>> +  of which only a handful are ever used by kernel.
> 
> At first, I thought this was an either/or thing. Use firmware or use DT,
> but it is really only get the clocks used in the DT from firmware.
> 
> Why wouldn't you just always do that? I can think of 3 cases:
> reparenting, debug and overlays. This breaks reparenting and overlays,
> right? Debug could be handled with some userspace trigger to get all the
> clocks.

Re-parenting this does not break, as the scan still checks every 
possible parent of a clock scanned. Overlays are broken for sure, as we 
don't know which overlays we would be applying, and what clocks would be 
in them. Debug is kind of broken as we only scan a small portion of the 
clocks.

> 
> Why scan any of the clocks up front? Why not just create the clocks on
> demand? If an unknown clock id is requested, then create the clock and
> query the firmware at that point. That would avoid the DT scan too.
> Maybe there's some issues in the clk framework preventing that, but
> that's not really a DT problem.

The very initial version I did a couple of years back, did scan the 
clocks based on need, and registered them dynamically. Stephen shot down 
this based on the assessment that there might be locking issues with the 
common clock framework with this approach leading into potential 
deadlock situations.

-Tero
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH 1/3] dt-bindings: clock: ti,sci-clk: Add support for parsing clock info from DT

[Tero Kristo]

On 22/01/2019 09:33, Tero Kristo wrote:
> On 21/01/2019 23:04, Rob Herring wrote:
>> On Tue, Jan 08, 2019 at 03:30:21PM +0200, Tero Kristo wrote:
>>> By default, the available clock info is queried from firmware, which can
>>> be quite a lengthy operation if there is a very large amount of clocks
>>> available. Add option for parsing the used clocks from DT instead, and
>>> only register these which can improve the boot time of the device quite
>>> a lot.
>>>
>>> Signed-off-by: Tero Kristo <t-kristo@ti.com>
>>> ---
>>>   Documentation/devicetree/bindings/clock/ti,sci-clk.txt | 7 +++++++
>>>   1 file changed, 7 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/clock/ti,sci-clk.txt 
>>> b/Documentation/devicetree/bindings/clock/ti,sci-clk.txt
>>> index 4e59dc6..c757ae1 100644
>>> --- a/Documentation/devicetree/bindings/clock/ti,sci-clk.txt
>>> +++ b/Documentation/devicetree/bindings/clock/ti,sci-clk.txt
>>> @@ -18,6 +18,13 @@ Required properties:
>>>     and clocks IDs for 66AK2G SoC are documented at
>>>     http://processors.wiki.ti.com/index.php/TISCI#66AK2G02_Data
>>> +Optional properties:
>>> +-------------------
>>> +- ti,scan-clocks-from-dt: Scan clock tree info from DT. By default,
>>> +  clocks are queried from firmware, which can be rather slow operation,
>>> +  especially if there is a really large number of clocks available out
>>> +  of which only a handful are ever used by kernel.
>>
>> At first, I thought this was an either/or thing. Use firmware or use DT,
>> but it is really only get the clocks used in the DT from firmware.
>>
>> Why wouldn't you just always do that? I can think of 3 cases:
>> reparenting, debug and overlays. This breaks reparenting and overlays,
>> right? Debug could be handled with some userspace trigger to get all the
>> clocks.
> 
> Re-parenting this does not break, as the scan still checks every 
> possible parent of a clock scanned. Overlays are broken for sure, as we 
> don't know which overlays we would be applying, and what clocks would be 
> in them. Debug is kind of broken as we only scan a small portion of the 
> clocks.
> 
>>
>> Why scan any of the clocks up front? Why not just create the clocks on
>> demand? If an unknown clock id is requested, then create the clock and
>> query the firmware at that point. That would avoid the DT scan too.
>> Maybe there's some issues in the clk framework preventing that, but
>> that's not really a DT problem.
> 
> The very initial version I did a couple of years back, did scan the 
> clocks based on need, and registered them dynamically. Stephen shot down 
> this based on the assessment that there might be locking issues with the 
> common clock framework with this approach leading into potential 
> deadlock situations.

So Rob, what is the final call on this binding? Ack/NAK? If NAK, shall I 
implement a kernel cmdline param to select the parsing method or what is 
preferred? Doing it build time with a simple Kconfig seems too limiting.

-Tero
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH 1/3] dt-bindings: clock: ti,sci-clk: Add support for parsing clock info from DT

[Stephen Boyd]

Quoting Tero Kristo (2019-02-05 00:25:40)
> On 22/01/2019 09:33, Tero Kristo wrote:
> > On 21/01/2019 23:04, Rob Herring wrote:
> >> At first, I thought this was an either/or thing. Use firmware or use DT,
> >> but it is really only get the clocks used in the DT from firmware.
> >>
> >> Why wouldn't you just always do that? I can think of 3 cases:
> >> reparenting, debug and overlays. This breaks reparenting and overlays,
> >> right? Debug could be handled with some userspace trigger to get all the
> >> clocks.
> > 
> > Re-parenting this does not break, as the scan still checks every 
> > possible parent of a clock scanned. Overlays are broken for sure, as we 
> > don't know which overlays we would be applying, and what clocks would be 
> > in them. Debug is kind of broken as we only scan a small portion of the 
> > clocks.
> > 
> >>
> >> Why scan any of the clocks up front? Why not just create the clocks on
> >> demand? If an unknown clock id is requested, then create the clock and
> >> query the firmware at that point. That would avoid the DT scan too.
> >> Maybe there's some issues in the clk framework preventing that, but
> >> that's not really a DT problem.
> > 
> > The very initial version I did a couple of years back, did scan the 
> > clocks based on need, and registered them dynamically. Stephen shot down 
> > this based on the assessment that there might be locking issues with the 
> > common clock framework with this approach leading into potential 
> > deadlock situations.

It's an interesting idea to limit the scope of clks that are registered
to only the leaf and whatever up to the root of the tree is involved in
the working set of the kernel.

> 
> So Rob, what is the final call on this binding? Ack/NAK? If NAK, shall I 
> implement a kernel cmdline param to select the parsing method or what is 
> preferred? Doing it build time with a simple Kconfig seems too limiting.
> 

Is the problem a performance problem where probing the firmware for all
the clks is costly and time intensive? So instead of doing that we're
describing some of the details in DT? Why can't we describe the clk tree
in C code with some data structure that indicates parent child linkages?
This is how every other SoC is doing this so far.

Re: [PATCH 1/3] dt-bindings: clock: ti,sci-clk: Add support for parsing clock info from DT

[Tero Kristo]

On 06/02/2019 19:47, Stephen Boyd wrote:
> Quoting Tero Kristo (2019-02-05 00:25:40)
>> On 22/01/2019 09:33, Tero Kristo wrote:
>>> On 21/01/2019 23:04, Rob Herring wrote:
>>>> At first, I thought this was an either/or thing. Use firmware or use DT,
>>>> but it is really only get the clocks used in the DT from firmware.
>>>>
>>>> Why wouldn't you just always do that? I can think of 3 cases:
>>>> reparenting, debug and overlays. This breaks reparenting and overlays,
>>>> right? Debug could be handled with some userspace trigger to get all the
>>>> clocks.
>>>
>>> Re-parenting this does not break, as the scan still checks every
>>> possible parent of a clock scanned. Overlays are broken for sure, as we
>>> don't know which overlays we would be applying, and what clocks would be
>>> in them. Debug is kind of broken as we only scan a small portion of the
>>> clocks.
>>>
>>>>
>>>> Why scan any of the clocks up front? Why not just create the clocks on
>>>> demand? If an unknown clock id is requested, then create the clock and
>>>> query the firmware at that point. That would avoid the DT scan too.
>>>> Maybe there's some issues in the clk framework preventing that, but
>>>> that's not really a DT problem.
>>>
>>> The very initial version I did a couple of years back, did scan the
>>> clocks based on need, and registered them dynamically. Stephen shot down
>>> this based on the assessment that there might be locking issues with the
>>> common clock framework with this approach leading into potential
>>> deadlock situations.
> 
> It's an interesting idea to limit the scope of clks that are registered
> to only the leaf and whatever up to the root of the tree is involved in
> the working set of the kernel.
> 
>>
>> So Rob, what is the final call on this binding? Ack/NAK? If NAK, shall I
>> implement a kernel cmdline param to select the parsing method or what is
>> preferred? Doing it build time with a simple Kconfig seems too limiting.
>>
> 
> Is the problem a performance problem where probing the firmware for all
> the clks is costly and time intensive?

Yes this is pretty expensive, as there can be quite a large amount of 
clocks on a SoC, and each clock must be probed separately.

> So instead of doing that we're
> describing some of the details in DT? Why can't we describe the clk tree
> in C code with some data structure that indicates parent child linkages?
> This is how every other SoC is doing this so far.

We can obviously do that also, however it is "neat" that we can probe 
the available clocks from the device, and don't need to hardcode 
anything kernel side... and neither maintain the clock data. If we want 
to go the hardcoded way, I can create tools to autogenerate the kernel 
side clock data from the clock dump of a running system though, and use 
the built-in clock data if it is available, retaining the existing probe 
method basically for new devices.

-Tero
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki