Hi Amit,

On 11/02/2017 09:28 AM, Amit Kucheria wrote:
[..>> +Interconnect node is the software definition of the interconnect
hardware

quoted

+port. Each interconnect provider consists of multiple interconnect nodes,
+which are connected to other SoC components including other interconnect
+providers. The point on the diagram where the CPUs connects to the memory is
+called an interconnect node, which belongs to the Mem NoC interconnect provider.
+
+Interconnect endpoits are the first or the last element of the path. Every

s/endpoits/endpoints

Ok!

quoted

+endpoint is a node, but not every node is an endpoint.
+
+Interconnect path is everything between two endpoints including all the nodes
+that have to be traversed to reach from a source to destination node. It may
+include multiple master-slave pairs across several interconnect providers.
+
+Interconnect consumers are the entities which make use of the data paths exposed
+by the providers. The consumers send requests to providers requesting various
+throughput, latency and priority. Usually the consumers are device drivers, that
+send request based on their needs. An example for a consumer is a a video

typo: is a video>

Ok!

[..]

quoted

+/**
+ * interconnect_set() - set constraints on a path between two endpoints
+ * @path: reference to the path returned by interconnect_get()
+ * @creq: request from the consumer, containing its requirements
+ *
+ * This function is used by an interconnect consumer to express its own needs
+ * in term of bandwidth and QoS for a previously requested path between two
+ * endpoints. The requests are aggregated and each node is updated accordingly.
+ *
+ * Returns 0 on success, or an approproate error code otherwise.
+ */
+int interconnect_set(struct interconnect_path *path,
+                    struct interconnect_creq *creq)
+{
+       struct interconnect_node *next, *prev = NULL;
+       size_t i;
+       int ret = 0;
+
+       for (i = 0; i < path->num_nodes; i++, prev = next) {
+               next = path->reqs[i].node;
+
+               /*
+                * Both endpoints should be valid and master-slave pairs of

Losing the 'and' improves readability.

Ok!

quoted

+                * the same interconnect provider that will be configured.
+                */
+               if (!next || !prev)
+                       continue;
+               if (next->icp != prev->icp)
+                       continue;
+
+               mutex_lock(&next->icp->lock);
+
+               /* update the consumer request for this path */
+               path->reqs[i].avg_bw = creq->avg_bw;
+               path->reqs[i].peak_bw = creq->peak_bw;
+
+               /* aggregate all requests */
+               interconnect_aggregate_icn(next);
+               interconnect_aggregate_icp(next->icp);
+
+               if (next->icp->ops->set) {
+                       /* commit the aggregated constraints */
+                       ret = next->icp->ops->set(prev, next, &next->icp->creq);
+               }

A comment here on how the contraints are propagated along the path
would be good. At the moment, this seems to go to each provider along
the path, take the provider lock and set the new constraints, then
move on to the next provider. Is there any need to make the changes
along the entire path "atomic"?

Yes, the above is correct. There is no such need at least for the
hardware i am currently playing with. We can add support for that later
if needed.

I'm trying to understand what happens in the case where a new request
comes along while the previous path is still be traversed.

It just gets aggregated and set and this seems to not be an issue as the
paths are valid. Now I am trying to keep it simple and if anything needs
serialization we can add some locking later.

quoted

+               mutex_unlock(&next->icp->lock);
+               if (ret)
+                       goto out;
+       }
+
+out:
+       return ret;
+}
+
+/**
+ * interconnect_get() - return a handle for path between two endpoints

This is not used currently by the msm8916 platform driver? Is this
expected to be called by leaf device drivers or by higher layer bus
drivers? I suspect a mix of the two, but an example would be nice.

This is called by a consumer driver to express its for example bandwidth
needs between various endpoints. Will add some examples.

[..]

quoted

+/**
+ * struct icp - interconnect provider (controller) entity that might
+ * provide multiple interconnect controls
+ *
+ * @icp_list: list of the registered interconnect providers
+ * @nodes: internal list of the interconnect provider nodes
+ * @ops: pointer to device specific struct icp_ops
+ * @dev: the device this interconnect provider belongs to
+ * @lock: a lock to protect creq and users
+ * @creq: the actual state of constraints for this interconnect provider
+ * @users: count of active users
+ * @data: pointer to private data
+ */
+struct icp {
+       struct list_head        icp_list;
+       struct list_head        nodes;
+       const struct icp_ops    *ops;
+       struct device           *dev;
+       struct mutex            lock;
+       struct interconnect_creq creq;
+       int                     users;
+       void                    *data;
+};

Use interconnect_provider here instead of icp for the sake of
consistency. Same for icp_ops. Or replace everything with any of the
other suggested alternatives. My suggestion to the name pool is 'xcon'
where x == inter.

Yes i am working on better naming, thanks!

BR,
Georgi