From mboxrd@z Thu Jan 1 00:00:00 1970 From: Evan Green Subject: Re: [PATCH v1 2/2] interconnect: qcom: Add tagging and wake/sleep support for sdm845 Date: Fri, 8 Mar 2019 10:35:29 -0800 Message-ID: References: <20190208172152.1807-1-georgi.djakov@linaro.org> <20190208172152.1807-3-georgi.djakov@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Return-path: In-Reply-To: <20190208172152.1807-3-georgi.djakov@linaro.org> Sender: linux-kernel-owner@vger.kernel.org To: Georgi Djakov Cc: linux-pm@vger.kernel.org, daidavid1@codeaurora.org, Vincent Guittot , Bjorn Andersson , amit.kucheria@linaro.org, Doug Anderson , Sean Sweeney , Michael Turquette , Alexandre Bailon , Thierry Reding , ksitaraman@nvidia.com, sanjayc@nvidia.com, henryc.chen@mediatek.com, LKML , linux-arm-kernel@lists.infradead.org, linux-arm-msm List-Id: linux-arm-msm@vger.kernel.org On Fri, Feb 8, 2019 at 9:22 AM Georgi Djakov wrote: > > From: David Dai > > Add support for wake and sleep commands by using a tag to indicate > whether or not the aggregate and set requests are active only or > dual context for a particular path. > > Signed-off-by: David Dai > Signed-off-by: Georgi Djakov > --- > drivers/interconnect/qcom/sdm845.c | 101 +++++++++++++++++++++-------- > 1 file changed, 75 insertions(+), 26 deletions(-) > > diff --git a/drivers/interconnect/qcom/sdm845.c b/drivers/interconnect/qcom/sdm845.c > index fb526004c82e..13499f681160 100644 > --- a/drivers/interconnect/qcom/sdm845.c > +++ b/drivers/interconnect/qcom/sdm845.c > @@ -65,6 +65,12 @@ struct bcm_db { > #define SDM845_MAX_BCMS 30 > #define SDM845_MAX_BCM_PER_NODE 2 > #define SDM845_MAX_VCD 10 > +#define SDM845_MAX_CTX 2 > +#define SDM845_EE_STATE 2 > +#define EE_STATE_WAKE 0 > +#define EE_STATE_SLEEP 1 > +#define AO_CTX 0 > +#define DUAL_CTX 1 > I get really lost with these two sets of numbers here. I think this needs some explanation in the comments. From staring at this what I've understood so far is: 1) Clients pass in a tag that buckets their requests into either AO or DUAL within the node. (Although, the requests all seem to get aggregated together no matter what the tag is, more on that later). 2) During icc_set(), we go through the many-to-many mapping of BCMs to nodes. For each BCM, we aggregate all the nodes it has, bucketed again by AO or DUAL. 3) The actual votes sent to RPMh are in terms of WAKE and SLEEP. 4) The mapping from AO/DUAL to WAKE/SLEEP for a particular BCM is: WAKE = AO+DUAL, SLEEP=DUAL. 5) qcom_icc_set() sends EE_STATE_WAKE stuff to RPMH_ACTIVE_ONLY_STATE. 6) If there's any difference between SLEEP and WAKE, then the EE_STATE_WAKE commands are gathered together and sent to RPMH_WAKE_ONLY_STATE, and all the EE_STATE_SLEEP commands are sent to RPMH_SLEEP_STATE. So ultimately the muxing ends up like RPMH_ACTIVE_ONLY_STATE <-- EE_STATE_WAKE <-- AO+DUAL RPMH_SLEEP_STATE <-- EE_STATE_SLEEP <-- DUAL RPMH_WAKE_ONLY_STATE <-- EE_STATE_WAKE <-- AO+DUAL Why do we need this complicated muxing to happen? Is it because we're trying to avoid requiring drivers to specify three different paths in the simple case, when all they care about is "use this bandwidth all the time"? What I think would make more sense would be to use the "tag" as a bitfield instead. So you'd have #define QCOM_ICC_TAG_ACTIVE_ONLY 0x00000001 #define QCOM_ICC_TAG_SLEEP 0x00000002 #define QCOM_ICC_TAG_WAKE 0x00000004 #define QCOM_ICC_TAG_ALL_TIMES (QCOM_ICC_TAG_ACTIVE_ONLY | QCOM_ICC_TAG_SLEEP | QCOM_ICC_TAG_WAKE) Drivers that don't care about sleep/wake sets would just not set a tag, or set a tag of QCOM_ICC_TAG_ALL_TIMES. Then in qcom_icc_aggregate(), you aggregate the same values up to three times, one for each bit they have set. Finally in qcom_icc_set, you can pass the votes directly down in their buckets, without doing a weird mapping from AO/DUAL to SLEEP/WAKE/ACTIVE_ONLY. The sticky part about this is that now rather than one set of sum/peak values, there are now three independent ones, corresponding to each of the tag bits. It seems like the interconnect core wouldn't want to mandate whether providers use the tag as a bitfield or a value. So the provider would need to keep not only the official set of aggregates that are returned back to the core (probably ACTIVE_ONLY, or maybe a max of the three), but also the two shadow copies internally for SLEEP and WAKE. If you organized the tag that way, the only extra thing you'd need is a callback to the provider just before the core starts aggregation, so that the provider can reset the shadow buckets to 0 the same way the core resets the main sum/peak to 0 before looping.With this, we've plumbed out the full abilities of RPMh to the client drivers, but without complicating the simple case of "set this bandwidth all the time". What do you think? > /** > * struct qcom_icc_node - Qualcomm specific interconnect nodes > @@ -86,8 +92,8 @@ struct qcom_icc_node { > u16 num_links; > u16 channels; > u16 buswidth; > - u64 sum_avg; > - u64 max_peak; > + u64 sum_avg[SDM845_MAX_CTX]; > + u64 max_peak[SDM845_MAX_CTX]; > struct qcom_icc_bcm *bcms[SDM845_MAX_BCM_PER_NODE]; > size_t num_bcms; > }; > @@ -112,8 +118,8 @@ struct qcom_icc_bcm { > const char *name; > u32 type; > u32 addr; > - u64 vote_x; > - u64 vote_y; > + u64 vote_x[SDM845_EE_STATE]; > + u64 vote_y[SDM845_EE_STATE]; > bool dirty; > bool keepalive; > struct bcm_db aux_data; > @@ -555,7 +561,7 @@ inline void tcs_cmd_gen(struct tcs_cmd *cmd, u64 vote_x, u64 vote_y, > cmd->wait = true; > } > > -static void tcs_list_gen(struct list_head *bcm_list, > +static void tcs_list_gen(struct list_head *bcm_list, int ee_state, > struct tcs_cmd tcs_list[SDM845_MAX_VCD], > int n[SDM845_MAX_VCD]) > { > @@ -573,8 +579,8 @@ static void tcs_list_gen(struct list_head *bcm_list, > commit = true; > cur_vcd_size = 0; > } > - tcs_cmd_gen(&tcs_list[idx], bcm->vote_x, bcm->vote_y, > - bcm->addr, commit); > + tcs_cmd_gen(&tcs_list[idx], bcm->vote_x[ee_state], > + bcm->vote_y[ee_state], bcm->addr, commit); > idx++; > n[batch]++; > /* > @@ -595,32 +601,42 @@ static void tcs_list_gen(struct list_head *bcm_list, > > static void bcm_aggregate(struct qcom_icc_bcm *bcm) > { > - size_t i; > - u64 agg_avg = 0; > - u64 agg_peak = 0; > + size_t i, ctx; > + u64 agg_avg[SDM845_MAX_CTX] = {0}; > + u64 agg_peak[SDM845_MAX_CTX] = {0}; > u64 temp; > > - for (i = 0; i < bcm->num_nodes; i++) { > - temp = bcm->nodes[i]->sum_avg * bcm->aux_data.width; > - do_div(temp, bcm->nodes[i]->buswidth * bcm->nodes[i]->channels); > - agg_avg = max(agg_avg, temp); > + for (ctx = 0; ctx < SDM845_MAX_CTX; ctx++) { > + for (i = 0; i < bcm->num_nodes; i++) { > + temp = bcm->nodes[i]->sum_avg[ctx] * bcm->aux_data.width; > + do_div(temp, bcm->nodes[i]->buswidth * bcm->nodes[i]->channels); > + agg_avg[ctx] = max(agg_avg[ctx], temp); > > - temp = bcm->nodes[i]->max_peak * bcm->aux_data.width; > - do_div(temp, bcm->nodes[i]->buswidth); > - agg_peak = max(agg_peak, temp); > + temp = bcm->nodes[i]->max_peak[ctx] * bcm->aux_data.width; > + do_div(temp, bcm->nodes[i]->buswidth); > + agg_peak[ctx] = max(agg_peak[ctx], temp); > + } > } > > - temp = agg_avg * 1000ULL; > + temp = agg_avg[AO_CTX] + agg_avg[DUAL_CTX] * 1000ULL; > + do_div(temp, bcm->aux_data.unit); > + bcm->vote_x[EE_STATE_WAKE] = temp; > + > + temp = max(agg_peak[AO_CTX], agg_peak[DUAL_CTX]) * 1000ULL; > + do_div(temp, bcm->aux_data.unit); > + bcm->vote_y[EE_STATE_WAKE] = temp; > + > + temp = agg_avg[DUAL_CTX] * 1000ULL; > do_div(temp, bcm->aux_data.unit); > - bcm->vote_x = temp; > + bcm->vote_x[EE_STATE_SLEEP] = temp; > > - temp = agg_peak * 1000ULL; > + temp = agg_peak[DUAL_CTX] * 1000ULL; > do_div(temp, bcm->aux_data.unit); > - bcm->vote_y = temp; > + bcm->vote_y[EE_STATE_SLEEP] = temp; > > if (bcm->keepalive && bcm->vote_x == 0 && bcm->vote_y == 0) { > - bcm->vote_x = 1; > - bcm->vote_y = 1; > + bcm->vote_x[EE_STATE_WAKE] = 1; > + bcm->vote_y[EE_STATE_WAKE] = 1; > } > > bcm->dirty = false; > @@ -631,14 +647,16 @@ static int qcom_icc_aggregate(struct icc_node *node, u32 tag, u32 avg_bw, > { > size_t i; > struct qcom_icc_node *qn; > + u32 ctx = 0; > > qn = node->data; > + ctx = (!!tag) ? AO_CTX : DUAL_CTX; > > *agg_avg += avg_bw; > *agg_peak = max_t(u32, *agg_peak, peak_bw); > > - qn->sum_avg = *agg_avg; > - qn->max_peak = *agg_peak; > + qn->sum_avg[ctx] = *agg_avg; > + qn->max_peak[ctx] = *agg_peak; I'm confused that you seem to aggregate agg_avg and agg_peak regardless of what the tag is, but then save them into buckets as if they had been kept separate. Doesn't this just end up as a mish-mash of all tag aggregates, but whose final value is dependent on the order of requests? (Said another way, what gets saved into each [ctx] bucket is the the aggregate of all requests regardless of tag, but only up until the last request of that tag type was seen.) From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.0 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1D996C43381 for ; Fri, 8 Mar 2019 18:36:24 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id DE73B20684 for ; Fri, 8 Mar 2019 18:36:23 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="PBIWMdYD"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="Bt8QeK6j" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org DE73B20684 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=chromium.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:To:Subject:Message-ID:Date:From: In-Reply-To:References:MIME-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=e9vG+84LiI0zjKS88fD/FP8OWaf0XP9fFCzZEdE2NYo=; b=PBIWMdYDxfwXOr jl/0sfyYEPqMVriaM4MXtEgMn01hE00TyzsEq8/TqJSD4LnvwR7xzom6P6uy0vA6l+hNo++D+09YA OF+L/QndOSLIXL51nM5lP4d8Uj7JAwttkWOaS1/yqk9bh3u/ouxp36rIyACqLDM5ZA+uvq1pral6X 94+PDP/1a3EdOczPcY7svLBhMhLYjLPZZT2ZD0q52KNEX4+Km4cz1nFhhmb6TOFxeDbVnx3nfLHRe SaKITnjgWbWhuW24odaQ0RYOwfVboXav96+5GwZGgXcMK6epJwZ/mk9Lo6cjCHpknGrAiePBSWNrp jQxDPVDaZY1cPx/Wl88A==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.90_1 #2 (Red Hat Linux)) id 1h2KLu-0001R9-4b; Fri, 08 Mar 2019 18:36:14 +0000 Received: from mail-lf1-x141.google.com ([2a00:1450:4864:20::141]) by bombadil.infradead.org with esmtps (Exim 4.90_1 #2 (Red Hat Linux)) id 1h2KLq-0001Pa-Q8 for linux-arm-kernel@lists.infradead.org; Fri, 08 Mar 2019 18:36:12 +0000 Received: by mail-lf1-x141.google.com with SMTP id i13so3288858lfg.10 for ; Fri, 08 Mar 2019 10:36:09 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=9AX8IcMRfKsa3xPGnO2FE2GyE4Gfzqkce/slKXcRL/k=; b=Bt8QeK6jCxaNg6lMN/af4aCbeQ/lYuHnuCTHTbYYixBzOBD4AycuJslj3lKXglhyDa +9RxipOMOpoZsA9hg8CJEFTtXaJEYOA5OGyMK+wwgyUw61ZDbFkQ+TIysD+XoQfjQyNH yHmzUkbq6dKFF7mBPpwOp3P8q1yFETBxXtfSE= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=9AX8IcMRfKsa3xPGnO2FE2GyE4Gfzqkce/slKXcRL/k=; b=QwasLdj/m4LdFoGiHinNCcn89XBA6OEWxsINYlAGarnuUJcrYRxfOlJHZpYGbYIe4l 8+7t0jcHHWVe/eN4461MZ/b9Yx4Cnjqx6JA0JaipYFB+G6jtVrmkSKML+OM5NTpk1U7j InrLv93R/5Ho298YM6RKEzNUbR6JZgniapHsh9GMXfm4IoOK/S7jz/S4c0KJn92DMU5a JvVTzXGIT45QE4Q4A9y65SpF3/sK6r4BpmTBiS3DMN6IIswK7YeFCAQTOiyhN/+LowSI 0zHmXyWeXifKIGc5YQm48glhjWwWzMNe4wHxUficDxyy96SZEyjyAgG9wyC7E1HWujzE vLMw== X-Gm-Message-State: APjAAAWxzCeqsgJygOkL+4DIXwGGznMOFBOGRzw4PJr/07EyEP6Ast7b iG2Hf69twXzL2CEeEt1XlFCSKtGck+E= X-Google-Smtp-Source: APXvYqyFk3mJvvmPPWcMc5GcKigGuEePCaI5zSsSyxc6jryjKZItPF0wMckbAY6i2wu/QZAxtIA/Lw== X-Received: by 2002:a19:5503:: with SMTP id n3mr11798268lfe.35.1552070167252; Fri, 08 Mar 2019 10:36:07 -0800 (PST) Received: from mail-lf1-f49.google.com (mail-lf1-f49.google.com. [209.85.167.49]) by smtp.gmail.com with ESMTPSA id c2sm1701874ljj.45.2019.03.08.10.36.05 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 08 Mar 2019 10:36:06 -0800 (PST) Received: by mail-lf1-f49.google.com with SMTP id z23so14965030lfe.0 for ; Fri, 08 Mar 2019 10:36:05 -0800 (PST) X-Received: by 2002:ac2:5228:: with SMTP id i8mr10887858lfl.162.1552070165462; Fri, 08 Mar 2019 10:36:05 -0800 (PST) MIME-Version: 1.0 References: <20190208172152.1807-1-georgi.djakov@linaro.org> <20190208172152.1807-3-georgi.djakov@linaro.org> In-Reply-To: <20190208172152.1807-3-georgi.djakov@linaro.org> From: Evan Green Date: Fri, 8 Mar 2019 10:35:29 -0800 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v1 2/2] interconnect: qcom: Add tagging and wake/sleep support for sdm845 To: Georgi Djakov X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190308_103610_873939_5A186D6A X-CRM114-Status: GOOD ( 25.53 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Vincent Guittot , sanjayc@nvidia.com, linux-pm@vger.kernel.org, Sean Sweeney , LKML , Michael Turquette , daidavid1@codeaurora.org, Doug Anderson , amit.kucheria@linaro.org, Bjorn Andersson , Thierry Reding , henryc.chen@mediatek.com, Alexandre Bailon , linux-arm-msm , ksitaraman@nvidia.com, linux-arm-kernel@lists.infradead.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Fri, Feb 8, 2019 at 9:22 AM Georgi Djakov wrote: > > From: David Dai > > Add support for wake and sleep commands by using a tag to indicate > whether or not the aggregate and set requests are active only or > dual context for a particular path. > > Signed-off-by: David Dai > Signed-off-by: Georgi Djakov > --- > drivers/interconnect/qcom/sdm845.c | 101 +++++++++++++++++++++-------- > 1 file changed, 75 insertions(+), 26 deletions(-) > > diff --git a/drivers/interconnect/qcom/sdm845.c b/drivers/interconnect/qcom/sdm845.c > index fb526004c82e..13499f681160 100644 > --- a/drivers/interconnect/qcom/sdm845.c > +++ b/drivers/interconnect/qcom/sdm845.c > @@ -65,6 +65,12 @@ struct bcm_db { > #define SDM845_MAX_BCMS 30 > #define SDM845_MAX_BCM_PER_NODE 2 > #define SDM845_MAX_VCD 10 > +#define SDM845_MAX_CTX 2 > +#define SDM845_EE_STATE 2 > +#define EE_STATE_WAKE 0 > +#define EE_STATE_SLEEP 1 > +#define AO_CTX 0 > +#define DUAL_CTX 1 > I get really lost with these two sets of numbers here. I think this needs some explanation in the comments. From staring at this what I've understood so far is: 1) Clients pass in a tag that buckets their requests into either AO or DUAL within the node. (Although, the requests all seem to get aggregated together no matter what the tag is, more on that later). 2) During icc_set(), we go through the many-to-many mapping of BCMs to nodes. For each BCM, we aggregate all the nodes it has, bucketed again by AO or DUAL. 3) The actual votes sent to RPMh are in terms of WAKE and SLEEP. 4) The mapping from AO/DUAL to WAKE/SLEEP for a particular BCM is: WAKE = AO+DUAL, SLEEP=DUAL. 5) qcom_icc_set() sends EE_STATE_WAKE stuff to RPMH_ACTIVE_ONLY_STATE. 6) If there's any difference between SLEEP and WAKE, then the EE_STATE_WAKE commands are gathered together and sent to RPMH_WAKE_ONLY_STATE, and all the EE_STATE_SLEEP commands are sent to RPMH_SLEEP_STATE. So ultimately the muxing ends up like RPMH_ACTIVE_ONLY_STATE <-- EE_STATE_WAKE <-- AO+DUAL RPMH_SLEEP_STATE <-- EE_STATE_SLEEP <-- DUAL RPMH_WAKE_ONLY_STATE <-- EE_STATE_WAKE <-- AO+DUAL Why do we need this complicated muxing to happen? Is it because we're trying to avoid requiring drivers to specify three different paths in the simple case, when all they care about is "use this bandwidth all the time"? What I think would make more sense would be to use the "tag" as a bitfield instead. So you'd have #define QCOM_ICC_TAG_ACTIVE_ONLY 0x00000001 #define QCOM_ICC_TAG_SLEEP 0x00000002 #define QCOM_ICC_TAG_WAKE 0x00000004 #define QCOM_ICC_TAG_ALL_TIMES (QCOM_ICC_TAG_ACTIVE_ONLY | QCOM_ICC_TAG_SLEEP | QCOM_ICC_TAG_WAKE) Drivers that don't care about sleep/wake sets would just not set a tag, or set a tag of QCOM_ICC_TAG_ALL_TIMES. Then in qcom_icc_aggregate(), you aggregate the same values up to three times, one for each bit they have set. Finally in qcom_icc_set, you can pass the votes directly down in their buckets, without doing a weird mapping from AO/DUAL to SLEEP/WAKE/ACTIVE_ONLY. The sticky part about this is that now rather than one set of sum/peak values, there are now three independent ones, corresponding to each of the tag bits. It seems like the interconnect core wouldn't want to mandate whether providers use the tag as a bitfield or a value. So the provider would need to keep not only the official set of aggregates that are returned back to the core (probably ACTIVE_ONLY, or maybe a max of the three), but also the two shadow copies internally for SLEEP and WAKE. If you organized the tag that way, the only extra thing you'd need is a callback to the provider just before the core starts aggregation, so that the provider can reset the shadow buckets to 0 the same way the core resets the main sum/peak to 0 before looping.With this, we've plumbed out the full abilities of RPMh to the client drivers, but without complicating the simple case of "set this bandwidth all the time". What do you think? > /** > * struct qcom_icc_node - Qualcomm specific interconnect nodes > @@ -86,8 +92,8 @@ struct qcom_icc_node { > u16 num_links; > u16 channels; > u16 buswidth; > - u64 sum_avg; > - u64 max_peak; > + u64 sum_avg[SDM845_MAX_CTX]; > + u64 max_peak[SDM845_MAX_CTX]; > struct qcom_icc_bcm *bcms[SDM845_MAX_BCM_PER_NODE]; > size_t num_bcms; > }; > @@ -112,8 +118,8 @@ struct qcom_icc_bcm { > const char *name; > u32 type; > u32 addr; > - u64 vote_x; > - u64 vote_y; > + u64 vote_x[SDM845_EE_STATE]; > + u64 vote_y[SDM845_EE_STATE]; > bool dirty; > bool keepalive; > struct bcm_db aux_data; > @@ -555,7 +561,7 @@ inline void tcs_cmd_gen(struct tcs_cmd *cmd, u64 vote_x, u64 vote_y, > cmd->wait = true; > } > > -static void tcs_list_gen(struct list_head *bcm_list, > +static void tcs_list_gen(struct list_head *bcm_list, int ee_state, > struct tcs_cmd tcs_list[SDM845_MAX_VCD], > int n[SDM845_MAX_VCD]) > { > @@ -573,8 +579,8 @@ static void tcs_list_gen(struct list_head *bcm_list, > commit = true; > cur_vcd_size = 0; > } > - tcs_cmd_gen(&tcs_list[idx], bcm->vote_x, bcm->vote_y, > - bcm->addr, commit); > + tcs_cmd_gen(&tcs_list[idx], bcm->vote_x[ee_state], > + bcm->vote_y[ee_state], bcm->addr, commit); > idx++; > n[batch]++; > /* > @@ -595,32 +601,42 @@ static void tcs_list_gen(struct list_head *bcm_list, > > static void bcm_aggregate(struct qcom_icc_bcm *bcm) > { > - size_t i; > - u64 agg_avg = 0; > - u64 agg_peak = 0; > + size_t i, ctx; > + u64 agg_avg[SDM845_MAX_CTX] = {0}; > + u64 agg_peak[SDM845_MAX_CTX] = {0}; > u64 temp; > > - for (i = 0; i < bcm->num_nodes; i++) { > - temp = bcm->nodes[i]->sum_avg * bcm->aux_data.width; > - do_div(temp, bcm->nodes[i]->buswidth * bcm->nodes[i]->channels); > - agg_avg = max(agg_avg, temp); > + for (ctx = 0; ctx < SDM845_MAX_CTX; ctx++) { > + for (i = 0; i < bcm->num_nodes; i++) { > + temp = bcm->nodes[i]->sum_avg[ctx] * bcm->aux_data.width; > + do_div(temp, bcm->nodes[i]->buswidth * bcm->nodes[i]->channels); > + agg_avg[ctx] = max(agg_avg[ctx], temp); > > - temp = bcm->nodes[i]->max_peak * bcm->aux_data.width; > - do_div(temp, bcm->nodes[i]->buswidth); > - agg_peak = max(agg_peak, temp); > + temp = bcm->nodes[i]->max_peak[ctx] * bcm->aux_data.width; > + do_div(temp, bcm->nodes[i]->buswidth); > + agg_peak[ctx] = max(agg_peak[ctx], temp); > + } > } > > - temp = agg_avg * 1000ULL; > + temp = agg_avg[AO_CTX] + agg_avg[DUAL_CTX] * 1000ULL; > + do_div(temp, bcm->aux_data.unit); > + bcm->vote_x[EE_STATE_WAKE] = temp; > + > + temp = max(agg_peak[AO_CTX], agg_peak[DUAL_CTX]) * 1000ULL; > + do_div(temp, bcm->aux_data.unit); > + bcm->vote_y[EE_STATE_WAKE] = temp; > + > + temp = agg_avg[DUAL_CTX] * 1000ULL; > do_div(temp, bcm->aux_data.unit); > - bcm->vote_x = temp; > + bcm->vote_x[EE_STATE_SLEEP] = temp; > > - temp = agg_peak * 1000ULL; > + temp = agg_peak[DUAL_CTX] * 1000ULL; > do_div(temp, bcm->aux_data.unit); > - bcm->vote_y = temp; > + bcm->vote_y[EE_STATE_SLEEP] = temp; > > if (bcm->keepalive && bcm->vote_x == 0 && bcm->vote_y == 0) { > - bcm->vote_x = 1; > - bcm->vote_y = 1; > + bcm->vote_x[EE_STATE_WAKE] = 1; > + bcm->vote_y[EE_STATE_WAKE] = 1; > } > > bcm->dirty = false; > @@ -631,14 +647,16 @@ static int qcom_icc_aggregate(struct icc_node *node, u32 tag, u32 avg_bw, > { > size_t i; > struct qcom_icc_node *qn; > + u32 ctx = 0; > > qn = node->data; > + ctx = (!!tag) ? AO_CTX : DUAL_CTX; > > *agg_avg += avg_bw; > *agg_peak = max_t(u32, *agg_peak, peak_bw); > > - qn->sum_avg = *agg_avg; > - qn->max_peak = *agg_peak; > + qn->sum_avg[ctx] = *agg_avg; > + qn->max_peak[ctx] = *agg_peak; I'm confused that you seem to aggregate agg_avg and agg_peak regardless of what the tag is, but then save them into buckets as if they had been kept separate. Doesn't this just end up as a mish-mash of all tag aggregates, but whose final value is dependent on the order of requests? (Said another way, what gets saved into each [ctx] bucket is the the aggregate of all requests regardless of tag, but only up until the last request of that tag type was seen.) _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel