[PATCH v3 0/3] Add interconnect sync state support

[PATCH v3 0/3] Add interconnect sync state support

[Georgi Djakov]

Bootloaders often leave some system resources enabled such as clocks,
regulators, interconnects etc. We want to keep these resources enabled
until all their consumers are probed. These resources are often shared,
so we must wait for all the consumers to come up, before deciding
whether to turn them off or change the configuration. This patchset is
trying to solve the above problem just for the on-chip interconnects.

The problem is solved by allowing the providers to implement the get_bw()
function which should return the current average/peak bandwidth. These are
used as floor values, that are enforced during boot while the requests from
all consumers are being collected. Then the sync_state() callback is used
to signal that all consumers have been probed, meaning that the floor
bandwidth is not needed anymore and the framework is ready to re-aggregate
and process all requests. If get_bw() is not implemented, the framework
will use INT_MAX as default bandwidth value.

v3:
* Go back to introducing the get_bw() function as in v1. (Saravana)
* If querying the current bandwidth is not supported, max out the
  bandwidth. (Saravana)
* Use icc_sync_state also for sc7180.

v2: https://lore.kernel.org/r/20200722110139.24778-1-georgi.djakov@linaro.org/
* Support initial values for both average and peak bandwidth (Mike)
* Skip aggregating/setting for nodes that don't specify initial bw (Mike)
* Drop patch 2/4: Add get_bw() callback (Mike)
* Squash patches 3 and 4.

v1: https://lore.kernel.org/lkml/20200709110705.30359-1-georgi.djakov@linaro.org/

Georgi Djakov (3):
  interconnect: Add get_bw() callback
  interconnect: Add sync state support
  interconnect: qcom: Use icc_sync_state

 drivers/interconnect/core.c           | 67 +++++++++++++++++++++++++++
 drivers/interconnect/qcom/osm-l3.c    |  1 +
 drivers/interconnect/qcom/sc7180.c    |  1 +
 drivers/interconnect/qcom/sdm845.c    |  1 +
 include/linux/interconnect-provider.h |  7 +++
 5 files changed, 77 insertions(+)

[PATCH v3 1/3] interconnect: Add get_bw() callback

[Georgi Djakov]

The interconnect controller hardware may support querying the current
bandwidth settings, so add a callback for providers to implement this
functionality if supported.

Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
---
 include/linux/interconnect-provider.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/interconnect-provider.h b/include/linux/interconnect-provider.h
index 4735518de515..520f70fe5a31 100644
--- a/include/linux/interconnect-provider.h
+++ b/include/linux/interconnect-provider.h
@@ -38,6 +38,7 @@ struct icc_node *of_icc_xlate_onecell(struct of_phandle_args *spec,
  * @aggregate: pointer to device specific aggregate operation function
  * @pre_aggregate: pointer to device specific function that is called
  *		   before the aggregation begins (optional)
+ * @get_bw: pointer to device specific function to get current bandwidth
  * @xlate: provider-specific callback for mapping nodes from phandle arguments
  * @dev: the device this interconnect provider belongs to
  * @users: count of active users
@@ -51,6 +52,7 @@ struct icc_provider {
 	int (*aggregate)(struct icc_node *node, u32 tag, u32 avg_bw,
 			 u32 peak_bw, u32 *agg_avg, u32 *agg_peak);
 	void (*pre_aggregate)(struct icc_node *node);
+	int (*get_bw)(struct icc_node *node, u32 *avg, u32 *peak);
 	struct icc_node* (*xlate)(struct of_phandle_args *spec, void *data);
 	struct device		*dev;
 	int			users;

[PATCH v3 2/3] interconnect: Add sync state support

[Georgi Djakov]

The bootloaders often do some initial configuration of the interconnects
in the system and we want to keep this configuration until all consumers
have probed and expressed their bandwidth needs. This is because we don't
want to change the configuration by starting to disable unused paths until
every user had a chance to request the amount of bandwidth it needs.

To accomplish this we will implement an interconnect specific sync_state
callback which will synchronize (aggregate and set) the current bandwidth
settings when all consumers have been probed.

Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
---
 drivers/interconnect/core.c           | 67 +++++++++++++++++++++++++++
 include/linux/interconnect-provider.h |  5 ++
 2 files changed, 72 insertions(+)

diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
index befd111049c0..eef389e8e26f 100644
--- a/drivers/interconnect/core.c
+++ b/drivers/interconnect/core.c
@@ -26,6 +26,8 @@
 
 static DEFINE_IDR(icc_idr);
 static LIST_HEAD(icc_providers);
+static int providers_count;
+static bool synced_state;
 static DEFINE_MUTEX(icc_lock);
 static struct dentry *icc_debugfs_dir;
 
@@ -261,6 +263,12 @@ static int aggregate_requests(struct icc_node *node)
 		}
 		p->aggregate(node, r->tag, avg_bw, peak_bw,
 			     &node->avg_bw, &node->peak_bw);
+
+		/* during boot use the initial bandwidth as a floor value */
+		if (!synced_state) {
+			node->avg_bw = max(node->avg_bw, node->init_avg);
+			node->peak_bw = max(node->peak_bw, node->init_peak);
+		}
 	}
 
 	return 0;
@@ -925,6 +933,19 @@ void icc_node_add(struct icc_node *node, struct icc_provider *provider)
 	node->provider = provider;
 	list_add_tail(&node->node_list, &provider->nodes);
 
+	/* get the initial bandwidth values and sync them with hardware */
+	if (provider->get_bw) {
+		provider->get_bw(node, &node->init_avg, &node->init_peak);
+	} else {
+		node->init_avg = INT_MAX;
+		node->init_peak = INT_MAX;
+	}
+	node->avg_bw = node->init_avg;
+	node->peak_bw = node->init_peak;
+	provider->set(node, node);
+	node->avg_bw = 0;
+	node->peak_bw = 0;
+
 	mutex_unlock(&icc_lock);
 }
 EXPORT_SYMBOL_GPL(icc_node_add);
@@ -1020,8 +1041,54 @@ int icc_provider_del(struct icc_provider *provider)
 }
 EXPORT_SYMBOL_GPL(icc_provider_del);
 
+static int of_count_icc_providers(struct device_node *np)
+{
+	struct device_node *child;
+	int count = 0;
+
+	for_each_available_child_of_node(np, child) {
+		if (of_property_read_bool(child, "#interconnect-cells"))
+			count++;
+		count += of_count_icc_providers(child);
+	}
+	of_node_put(np);
+
+	return count;
+}
+
+void icc_sync_state(struct device *dev)
+{
+	struct icc_provider *p;
+	struct icc_node *n;
+	static int count;
+
+	count++;
+
+	if (count < providers_count)
+		return;
+
+	mutex_lock(&icc_lock);
+	synced_state = true;
+	list_for_each_entry(p, &icc_providers, provider_list) {
+		dev_dbg(p->dev, "interconnect provider is in synced state\n");
+		list_for_each_entry(n, &p->nodes, node_list) {
+			if (n->init_avg || n->init_peak) {
+				aggregate_requests(n);
+				p->set(n, n);
+			}
+		}
+	}
+	mutex_unlock(&icc_lock);
+}
+EXPORT_SYMBOL_GPL(icc_sync_state);
+
 static int __init icc_init(void)
 {
+	struct device_node *root = of_find_node_by_path("/");
+
+	providers_count = of_count_icc_providers(root);
+	of_node_put(root);
+
 	icc_debugfs_dir = debugfs_create_dir("interconnect", NULL);
 	debugfs_create_file("interconnect_summary", 0444,
 			    icc_debugfs_dir, NULL, &icc_summary_fops);
diff --git a/include/linux/interconnect-provider.h b/include/linux/interconnect-provider.h
index 520f70fe5a31..f713308b8a8f 100644
--- a/include/linux/interconnect-provider.h
+++ b/include/linux/interconnect-provider.h
@@ -75,6 +75,8 @@ struct icc_provider {
  * @req_list: a list of QoS constraint requests associated with this node
  * @avg_bw: aggregated value of average bandwidth requests from all consumers
  * @peak_bw: aggregated value of peak bandwidth requests from all consumers
+ * @init_avg: average bandwidth value that is read from the hardware during init
+ * @init_peak: peak bandwidth value that is read from the hardware during init
  * @data: pointer to private data
  */
 struct icc_node {
@@ -91,6 +93,8 @@ struct icc_node {
 	struct hlist_head	req_list;
 	u32			avg_bw;
 	u32			peak_bw;
+	u32			init_avg;
+	u32			init_peak;
 	void			*data;
 };
 
@@ -108,6 +112,7 @@ int icc_nodes_remove(struct icc_provider *provider);
 int icc_provider_add(struct icc_provider *provider);
 int icc_provider_del(struct icc_provider *provider);
 struct icc_node *of_icc_get_from_provider(struct of_phandle_args *spec);
+void icc_sync_state(struct device *dev);
 
 #else
 

[PATCH v3 3/3] interconnect: qcom: Use icc_sync_state

[Georgi Djakov]

Lowering the bandwidth on the bus might have negative consequences if
it's done before all consumers had a chance to cast their vote. Now by
default the framework sets the bandwidth to maximum during boot. We need
to use the icc_sync_state callback to notify the framework when all
consumers are probed and there is no need to keep the bandwidth set to
maximum anymore.

Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
---
 drivers/interconnect/qcom/osm-l3.c | 1 +
 drivers/interconnect/qcom/sc7180.c | 1 +
 drivers/interconnect/qcom/sdm845.c | 1 +
 3 files changed, 3 insertions(+)

diff --git a/drivers/interconnect/qcom/osm-l3.c b/drivers/interconnect/qcom/osm-l3.c
index 96fb9ff5ff2e..ae955f164442 100644
--- a/drivers/interconnect/qcom/osm-l3.c
+++ b/drivers/interconnect/qcom/osm-l3.c
@@ -268,6 +268,7 @@ static struct platform_driver osm_l3_driver = {
 	.driver = {
 		.name = "osm-l3",
 		.of_match_table = osm_l3_of_match,
+		.sync_state = icc_sync_state,
 	},
 };
 module_platform_driver(osm_l3_driver);
diff --git a/drivers/interconnect/qcom/sc7180.c b/drivers/interconnect/qcom/sc7180.c
index dcf493d07928..4c5d38649220 100644
--- a/drivers/interconnect/qcom/sc7180.c
+++ b/drivers/interconnect/qcom/sc7180.c
@@ -633,6 +633,7 @@ static struct platform_driver qnoc_driver = {
 	.driver = {
 		.name = "qnoc-sc7180",
 		.of_match_table = qnoc_of_match,
+		.sync_state = icc_sync_state,
 	},
 };
 module_platform_driver(qnoc_driver);
diff --git a/drivers/interconnect/qcom/sdm845.c b/drivers/interconnect/qcom/sdm845.c
index f6c7b969520d..6aa39aad2555 100644
--- a/drivers/interconnect/qcom/sdm845.c
+++ b/drivers/interconnect/qcom/sdm845.c
@@ -559,6 +559,7 @@ static struct platform_driver qnoc_driver = {
 	.driver = {
 		.name = "qnoc-sdm845",
 		.of_match_table = qnoc_of_match,
+		.sync_state = icc_sync_state,
 	},
 };
 module_platform_driver(qnoc_driver);

Re: [PATCH v3 0/3] Add interconnect sync state support

[Saravana Kannan]

Reviewed-by: Saravana Kannan <saravanak@google.com>

to all 3 patches in the series.

-Saravana

On Tue, Aug 25, 2020 at 10:01 AM Georgi Djakov <georgi.djakov@linaro.org> wrote:
>
> Bootloaders often leave some system resources enabled such as clocks,
> regulators, interconnects etc. We want to keep these resources enabled
> until all their consumers are probed. These resources are often shared,
> so we must wait for all the consumers to come up, before deciding
> whether to turn them off or change the configuration. This patchset is
> trying to solve the above problem just for the on-chip interconnects.
>
> The problem is solved by allowing the providers to implement the get_bw()
> function which should return the current average/peak bandwidth. These are
> used as floor values, that are enforced during boot while the requests from
> all consumers are being collected. Then the sync_state() callback is used
> to signal that all consumers have been probed, meaning that the floor
> bandwidth is not needed anymore and the framework is ready to re-aggregate
> and process all requests. If get_bw() is not implemented, the framework
> will use INT_MAX as default bandwidth value.
>
> v3:
> * Go back to introducing the get_bw() function as in v1. (Saravana)
> * If querying the current bandwidth is not supported, max out the
>   bandwidth. (Saravana)
> * Use icc_sync_state also for sc7180.
>
> v2: https://lore.kernel.org/r/20200722110139.24778-1-georgi.djakov@linaro.org/
> * Support initial values for both average and peak bandwidth (Mike)
> * Skip aggregating/setting for nodes that don't specify initial bw (Mike)
> * Drop patch 2/4: Add get_bw() callback (Mike)
> * Squash patches 3 and 4.
>
> v1: https://lore.kernel.org/lkml/20200709110705.30359-1-georgi.djakov@linaro.org/
>
> Georgi Djakov (3):
>   interconnect: Add get_bw() callback
>   interconnect: Add sync state support
>   interconnect: qcom: Use icc_sync_state
>
>  drivers/interconnect/core.c           | 67 +++++++++++++++++++++++++++
>  drivers/interconnect/qcom/osm-l3.c    |  1 +
>  drivers/interconnect/qcom/sc7180.c    |  1 +
>  drivers/interconnect/qcom/sdm845.c    |  1 +
>  include/linux/interconnect-provider.h |  7 +++
>  5 files changed, 77 insertions(+)
>