From mboxrd@z Thu Jan 1 00:00:00 1970 From: Georgi Djakov Subject: Re: [RFC v0 1/2] interconnect: Add generic interconnect controller API Date: Tue, 14 Mar 2017 17:39:13 +0200 Message-ID: References: <20170301182235.19154-1-georgi.djakov@linaro.org> <20170301182235.19154-2-georgi.djakov@linaro.org> <58B8CFDA.3080303@codeaurora.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <58B8CFDA.3080303@codeaurora.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Saravana Kannan Cc: linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org, vincent.guittot@linaro.org, linux-pm@vger.kernel.org, seansw@qti.qualcomm.com, gregkh@linuxfoundation.org, mturquette@baylibre.com, rjw@rjwysocki.net, linux-kernel@vger.kernel.org, davidai@quicinc.com, robh+dt@kernel.org, khilman@baylibre.com, andy.gross@linaro.org, sboyd@codeaurora.org, linux-arm-kernel@lists.infradead.org List-Id: linux-arm-msm@vger.kernel.org On 03/03/2017 04:07 AM, Saravana Kannan wrote: > On 03/01/2017 10:22 AM, Georgi Djakov wrote: >> This patch introduce a new API to get the requirement and configure the >> interconnect buses across the entire chipset to fit with the current >> demand. >> [..] >> +int interconnect_set(struct interconnect_path *path, u32 bandwidth) >> +{ >> + struct interconnect_node *node; >> + >> + list_for_each_entry(node, &path->node_list, search_list) { >> + if (node->icp->ops->set) >> + node->icp->ops->set(node, bandwidth); > > This ops needs to take a "source" and "dest" input and you'll need to > pass in the prev/next nodes of each "node" in this list. This is needed > so that each interconnect know the local source/destination and can make > the aggregation decisions correctly based on the internal implementation > of the interconnect. For the first and last nodes in the list, the > source and destination nodes can be NULL, respectively. I agree. Updated. > >> + } >> + >> + return 0; >> +} >> + [..] >> +void interconnect_put(struct interconnect_path *path) >> +{ >> + struct interconnect_node *node; >> + struct icn_qos *req; >> + struct hlist_node *tmp; >> + >> + if (IS_ERR(path)) >> + return; >> + >> + list_for_each_entry(node, &path->node_list, search_list) { >> + hlist_for_each_entry_safe(req, tmp, &node->qos_list, node) { >> + if (req->path == path) { >> + hlist_del(&req->node); >> + kfree(req); >> + } > > Should we go through and remove any bandwidth votes that were made on > this path before we free it? > Yes, thanks! We should remove the constraints from the path, then update the nodes and after that free the memory. >> + } >> + } >> + >> + kfree(path); >> +} >> +EXPORT_SYMBOL_GPL(interconnect_put); >> + >> +int interconnect_add_provider(struct icp *icp) >> +{ >> + struct interconnect_node *node; >> + >> + WARN(!icp->ops->xlate, "%s: .xlate is not implemented\n", __func__); >> + WARN(!icp->ops->set, "%s: .set is not implemented\n", __func__); >> + >> + mutex_lock(&interconnect_provider_list_mutex); >> + list_add(&icp->icp_list, &interconnect_provider_list); >> + mutex_unlock(&interconnect_provider_list_mutex); >> + >> + list_for_each_entry(node, &icp->nodes, icn_list) { >> + INIT_HLIST_HEAD(&node->qos_list); >> + } >> + >> + dev_info(icp->dev, "added interconnect provider %s\n", icp->name); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(interconnect_add_provider); >> + >> +int interconnect_del_provider(struct icp *icp) >> +{ >> + mutex_lock(&interconnect_provider_list_mutex); >> + of_node_put(icp->of_node); >> + list_del(&icp->icp_list); > > If there's a path with an active vote, we should probably return a > -EBUSY to prevent deleting a provider that's actively used? > Thanks, sounds good. Will do. BR, Georgi From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752331AbdCNPsM (ORCPT ); Tue, 14 Mar 2017 11:48:12 -0400 Received: from mail-wr0-f176.google.com ([209.85.128.176]:32962 "EHLO mail-wr0-f176.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752525AbdCNPjU (ORCPT ); Tue, 14 Mar 2017 11:39:20 -0400 Subject: Re: [RFC v0 1/2] interconnect: Add generic interconnect controller API To: Saravana Kannan References: <20170301182235.19154-1-georgi.djakov@linaro.org> <20170301182235.19154-2-georgi.djakov@linaro.org> <58B8CFDA.3080303@codeaurora.org> Cc: linux-pm@vger.kernel.org, rjw@rjwysocki.net, robh+dt@kernel.org, gregkh@linuxfoundation.org, khilman@baylibre.com, mturquette@baylibre.com, vincent.guittot@linaro.org, sboyd@codeaurora.org, andy.gross@linaro.org, seansw@qti.qualcomm.com, davidai@quicinc.com, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-arm-msm@vger.kernel.org From: Georgi Djakov Message-ID: Date: Tue, 14 Mar 2017 17:39:13 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: <58B8CFDA.3080303@codeaurora.org> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 03/03/2017 04:07 AM, Saravana Kannan wrote: > On 03/01/2017 10:22 AM, Georgi Djakov wrote: >> This patch introduce a new API to get the requirement and configure the >> interconnect buses across the entire chipset to fit with the current >> demand. >> [..] >> +int interconnect_set(struct interconnect_path *path, u32 bandwidth) >> +{ >> + struct interconnect_node *node; >> + >> + list_for_each_entry(node, &path->node_list, search_list) { >> + if (node->icp->ops->set) >> + node->icp->ops->set(node, bandwidth); > > This ops needs to take a "source" and "dest" input and you'll need to > pass in the prev/next nodes of each "node" in this list. This is needed > so that each interconnect know the local source/destination and can make > the aggregation decisions correctly based on the internal implementation > of the interconnect. For the first and last nodes in the list, the > source and destination nodes can be NULL, respectively. I agree. Updated. > >> + } >> + >> + return 0; >> +} >> + [..] >> +void interconnect_put(struct interconnect_path *path) >> +{ >> + struct interconnect_node *node; >> + struct icn_qos *req; >> + struct hlist_node *tmp; >> + >> + if (IS_ERR(path)) >> + return; >> + >> + list_for_each_entry(node, &path->node_list, search_list) { >> + hlist_for_each_entry_safe(req, tmp, &node->qos_list, node) { >> + if (req->path == path) { >> + hlist_del(&req->node); >> + kfree(req); >> + } > > Should we go through and remove any bandwidth votes that were made on > this path before we free it? > Yes, thanks! We should remove the constraints from the path, then update the nodes and after that free the memory. >> + } >> + } >> + >> + kfree(path); >> +} >> +EXPORT_SYMBOL_GPL(interconnect_put); >> + >> +int interconnect_add_provider(struct icp *icp) >> +{ >> + struct interconnect_node *node; >> + >> + WARN(!icp->ops->xlate, "%s: .xlate is not implemented\n", __func__); >> + WARN(!icp->ops->set, "%s: .set is not implemented\n", __func__); >> + >> + mutex_lock(&interconnect_provider_list_mutex); >> + list_add(&icp->icp_list, &interconnect_provider_list); >> + mutex_unlock(&interconnect_provider_list_mutex); >> + >> + list_for_each_entry(node, &icp->nodes, icn_list) { >> + INIT_HLIST_HEAD(&node->qos_list); >> + } >> + >> + dev_info(icp->dev, "added interconnect provider %s\n", icp->name); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(interconnect_add_provider); >> + >> +int interconnect_del_provider(struct icp *icp) >> +{ >> + mutex_lock(&interconnect_provider_list_mutex); >> + of_node_put(icp->of_node); >> + list_del(&icp->icp_list); > > If there's a path with an active vote, we should probably return a > -EBUSY to prevent deleting a provider that's actively used? > Thanks, sounds good. Will do. BR, Georgi From mboxrd@z Thu Jan 1 00:00:00 1970 From: georgi.djakov@linaro.org (Georgi Djakov) Date: Tue, 14 Mar 2017 17:39:13 +0200 Subject: [RFC v0 1/2] interconnect: Add generic interconnect controller API In-Reply-To: <58B8CFDA.3080303@codeaurora.org> References: <20170301182235.19154-1-georgi.djakov@linaro.org> <20170301182235.19154-2-georgi.djakov@linaro.org> <58B8CFDA.3080303@codeaurora.org> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 03/03/2017 04:07 AM, Saravana Kannan wrote: > On 03/01/2017 10:22 AM, Georgi Djakov wrote: >> This patch introduce a new API to get the requirement and configure the >> interconnect buses across the entire chipset to fit with the current >> demand. >> [..] >> +int interconnect_set(struct interconnect_path *path, u32 bandwidth) >> +{ >> + struct interconnect_node *node; >> + >> + list_for_each_entry(node, &path->node_list, search_list) { >> + if (node->icp->ops->set) >> + node->icp->ops->set(node, bandwidth); > > This ops needs to take a "source" and "dest" input and you'll need to > pass in the prev/next nodes of each "node" in this list. This is needed > so that each interconnect know the local source/destination and can make > the aggregation decisions correctly based on the internal implementation > of the interconnect. For the first and last nodes in the list, the > source and destination nodes can be NULL, respectively. I agree. Updated. > >> + } >> + >> + return 0; >> +} >> + [..] >> +void interconnect_put(struct interconnect_path *path) >> +{ >> + struct interconnect_node *node; >> + struct icn_qos *req; >> + struct hlist_node *tmp; >> + >> + if (IS_ERR(path)) >> + return; >> + >> + list_for_each_entry(node, &path->node_list, search_list) { >> + hlist_for_each_entry_safe(req, tmp, &node->qos_list, node) { >> + if (req->path == path) { >> + hlist_del(&req->node); >> + kfree(req); >> + } > > Should we go through and remove any bandwidth votes that were made on > this path before we free it? > Yes, thanks! We should remove the constraints from the path, then update the nodes and after that free the memory. >> + } >> + } >> + >> + kfree(path); >> +} >> +EXPORT_SYMBOL_GPL(interconnect_put); >> + >> +int interconnect_add_provider(struct icp *icp) >> +{ >> + struct interconnect_node *node; >> + >> + WARN(!icp->ops->xlate, "%s: .xlate is not implemented\n", __func__); >> + WARN(!icp->ops->set, "%s: .set is not implemented\n", __func__); >> + >> + mutex_lock(&interconnect_provider_list_mutex); >> + list_add(&icp->icp_list, &interconnect_provider_list); >> + mutex_unlock(&interconnect_provider_list_mutex); >> + >> + list_for_each_entry(node, &icp->nodes, icn_list) { >> + INIT_HLIST_HEAD(&node->qos_list); >> + } >> + >> + dev_info(icp->dev, "added interconnect provider %s\n", icp->name); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(interconnect_add_provider); >> + >> +int interconnect_del_provider(struct icp *icp) >> +{ >> + mutex_lock(&interconnect_provider_list_mutex); >> + of_node_put(icp->of_node); >> + list_del(&icp->icp_list); > > If there's a path with an active vote, we should probably return a > -EBUSY to prevent deleting a provider that's actively used? > Thanks, sounds good. Will do. BR, Georgi