Re: [PATCH v8 09/10] dt-bindings: interconnect: Add interconnect-tags bindings

From: Sibi Sankar
Date: Tue May 26 2020 - 13:08:45 EST




On 5/21/20 12:43 AM, Saravana Kannan wrote:
On Wed, May 20, 2020 at 11:51 AM Sibi Sankar <sibis@xxxxxxxxxxxxxx> wrote:

On 2020-05-20 01:27, Saravana Kannan wrote:
On Tue, May 19, 2020 at 11:58 AM Rob Herring <robh@xxxxxxxxxx> wrote:

On Tue, May 12, 2020 at 03:53:26PM +0300, Georgi Djakov wrote:
From: Sibi Sankar <sibis@xxxxxxxxxxxxxx>

Add interconnect-tags bindings to enable passing of optional
tag information to the interconnect framework.

Signed-off-by: Sibi Sankar <sibis@xxxxxxxxxxxxxx>
Signed-off-by: Georgi Djakov <georgi.djakov@xxxxxxxxxx>
---
v8:
* New patch, picked from here:
https://lore.kernel.org/r/20200504202243.5476-10-sibis@xxxxxxxxxxxxxx

.../devicetree/bindings/interconnect/interconnect.txt | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/interconnect/interconnect.txt b/Documentation/devicetree/bindings/interconnect/interconnect.txt
index 6f5d23a605b7..c1a226a934e5 100644
--- a/Documentation/devicetree/bindings/interconnect/interconnect.txt
+++ b/Documentation/devicetree/bindings/interconnect/interconnect.txt
@@ -55,6 +55,11 @@ interconnect-names : List of interconnect path name strings sorted in the same
* dma-mem: Path from the device to the main memory of
the system

+interconnect-tags : List of interconnect path tags sorted in the same order as the
+ interconnects property. Consumers can append a specific tag to
+ the path and pass this information to the interconnect framework
+ to do aggregation based on the attached tag.

Why isn't this information in the 'interconnect' arg cells?

We have 'interconnect-names' because strings don't mix with cells. An
expanding list of 'interconnect-.*' is not a good pattern IMO.

Rob,
Currently the interconnect paths
assume a default tag and only few
icc paths require tags that differ
from the default ones. Encoding the
tags in the interconnect arg cells
would force all paths to specify
the tags. I guess that's okay.

I think that's the right thing. Those cells are meant to be "args" to
the provider.

diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi

index ea4764f06a901..b34f024d4ab63 100644

--- a/arch/arm64/boot/dts/qcom/sc7180.dtsi

+++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi

@@ -132,9 +132,8 @@ &LITTLE_CPU_SLEEP_1

capacity-dmips-mhz = <1024>;

dynamic-power-coefficient = <100>;

operating-points-v2 = <&cpu0_opp_table>;

- interconnects = <&gem_noc MASTER_APPSS_PROC &mc_virt SLAVE_EBI1>,

+ interconnects = <&gem_noc MASTER_APPSS_PROC 3 &mc_virt SLAVE_EBI1 3>,

<&osm_l3 MASTER_OSM_L3_APPS &osm_l3 SLAVE_OSM_L3>;

next-level-cache = <&L2_0>;

#cooling-cells = <2>;

qcom,freq-domain = <&cpufreq_hw 0>;



....

mc_virt: interconnect@1638000 {

compatible = "qcom,sc7180-mc-virt";

reg = <0 0x01638000 0 0x1000>;

- #interconnect-cells = <1>;

+ #interconnect-cells = <2>;

qcom,bcm-voters = <&apps_bcm_voter>;

};

....



@@ -2216,14 +2208,14 @@ system-cache-controller@9200000 {

gem_noc: interconnect@9680000 {

compatible = "qcom,sc7180-gem-noc";

reg = <0 0x09680000 0 0x3e200>;

- #interconnect-cells = <1>;

+ #interconnect-cells = <2>;

qcom,bcm-voters = <&apps_bcm_voter>;

};

....



diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c

index 294e9c58565bb..6a7a785bd90b9 100644

--- a/drivers/interconnect/core.c

+++ b/drivers/interconnect/core.c

@@ -340,7 +340,7 @@ static struct icc_node *of_icc_get_from_provider(struct of_phandle_args *spec)

struct icc_node *node = ERR_PTR(-EPROBE_DEFER);

struct icc_provider *provider;



- if (!spec || spec->args_count != 1)

+ if (!spec || spec->args_count < 1)

return ERR_PTR(-EINVAL);



mutex_lock(&icc_lock);

@@ -469,6 +469,9 @@ struct icc_path *of_icc_get_by_index(struct device *dev, int idx)

return ERR_PTR(-ENOMEM);

}



+ if (src_args.args_count == 2)

+ icc_set_tag(path, src_args.args[1]);

diff: https://paste.ubuntu.com/p/sRRYhxQjsV/

Saravana/Georgi,
A few concerns here, I feel tag info as the second arg to the provider
may not be true for all socs. Does introducing soc specific of_icc_get
functions make sense?



Also, is there an example for interconnect-tags that I missed? Is it a
list of strings, numbers, etc?

Saravana,
https://patchwork.kernel.org/patch/11527589/
^^ is an example of interconnect-tag useage.

If we actually merge interconnect-tags, I think the doc should be
updated. Instead of having to grep around.

-Saravana


--
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc, is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project