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=-12.2 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 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 9C6B8C4320A for ; Wed, 18 Aug 2021 04:44:05 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 8069B61075 for ; Wed, 18 Aug 2021 04:44:05 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S237790AbhHREoi (ORCPT ); Wed, 18 Aug 2021 00:44:38 -0400 Received: from m43-7.mailgun.net ([69.72.43.7]:61456 "EHLO m43-7.mailgun.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237606AbhHREoh (ORCPT ); Wed, 18 Aug 2021 00:44:37 -0400 DKIM-Signature: a=rsa-sha256; v=1; c=relaxed/relaxed; d=mg.codeaurora.org; q=dns/txt; s=smtp; t=1629261844; h=Content-Transfer-Encoding: Content-Type: In-Reply-To: MIME-Version: Date: Message-ID: References: Cc: To: Subject: From: Sender; bh=5gfuhlHOrex3OrHOs8ZwU6QWvfo1GgFjx0nSGuiUXvg=; b=j3DuwpCc3UxxAxLzRMQ6lQfua71IsG+JgB3Z9AK6hmsqQpBw8W82W9EyaqmwJbizB8JgfGiN q7freGxLPBw/xcHlPPXRLkjuC2tjCf902r0JBkoTzL87mZ6JvpBriPx0S0m+lMWTWa8iHbFL XacG7O6xHf7B/BE8FaoN8JTxZ6A= X-Mailgun-Sending-Ip: 69.72.43.7 X-Mailgun-Sid: WyI1MzIzYiIsICJsaW51eC1hcm0tbXNtQHZnZXIua2VybmVsLm9yZyIsICJiZTllNGEiXQ== Received: from smtp.codeaurora.org (ec2-35-166-182-171.us-west-2.compute.amazonaws.com [35.166.182.171]) by smtp-out-n03.prod.us-east-1.postgun.com with SMTP id 611c90099507ca1a34a037a6 (version=TLS1.2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256); Wed, 18 Aug 2021 04:43:53 GMT Sender: mdtipton=codeaurora.org@mg.codeaurora.org Received: by smtp.codeaurora.org (Postfix, from userid 1001) id B32CCC4360D; Wed, 18 Aug 2021 04:43:52 +0000 (UTC) Received: from [192.168.1.159] (ip70-179-20-127.sd.sd.cox.net [70.179.20.127]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) (Authenticated sender: mdtipton) by smtp.codeaurora.org (Postfix) with ESMTPSA id 6ADF9C43460; Wed, 18 Aug 2021 04:43:51 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 smtp.codeaurora.org 6ADF9C43460 Authentication-Results: aws-us-west-2-caf-mail-1.web.codeaurora.org; dmarc=none (p=none dis=none) header.from=codeaurora.org Authentication-Results: aws-us-west-2-caf-mail-1.web.codeaurora.org; spf=fail smtp.mailfrom=codeaurora.org From: Mike Tipton Subject: Re: [PATCH v2 4/4] interconnect: qcom: icc-rpmh: Add BCMs to commit list in pre_aggregate To: Stephen Boyd , Alex Elder , djakov@kernel.org Cc: bjorn.andersson@linaro.org, agross@kernel.org, saravanak@google.com, okukatla@codeaurora.org, linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org References: <20210721175432.2119-1-mdtipton@codeaurora.org> <20210721175432.2119-5-mdtipton@codeaurora.org> <0e659a74-ba52-2262-f7ce-eddf4a655b63@linaro.org> Message-ID: <49eeb675-3c76-ef3e-708d-230ec980202f@codeaurora.org> Date: Tue, 17 Aug 2021 21:43:50 -0700 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-arm-msm@vger.kernel.org On 8/11/2021 11:13 AM, Stephen Boyd wrote: > Quoting Alex Elder (2021-08-11 09:01:27) >> On 8/10/21 6:31 PM, Stephen Boyd wrote: >>> Quoting Mike Tipton (2021-07-21 10:54:32) >>>> We're only adding BCMs to the commit list in aggregate(), but there are >>>> cases where pre_aggregate() is called without subsequently calling >>>> aggregate(). In particular, in icc_sync_state() when a node with initial >>>> BW has zero requests. Since BCMs aren't added to the commit list in >>>> these cases, we don't actually send the zero BW request to HW. So the >>>> resources remain on unnecessarily. >>>> >>>> Add BCMs to the commit list in pre_aggregate() instead, which is always >>>> called even when there are no requests. >>>> >>>> Fixes: 976daac4a1c5 ("interconnect: qcom: Consolidate interconnect RPMh support") >>>> Signed-off-by: Mike Tipton >>>> --- >>> >>> This patch breaks reboot for me on sc7180 Lazor >> >> If I am using the interface improperly or something in the >> IPA driver, please let me know. I actually plan to switch >> to using the bulk interfaces soon (FYI). >> > > I suspect I'm seeing a shutdown ordering issue, where we start dropping > interconnect requests in driver shutdown callbacks and then some bus > turns off and the CPU can't access a device. Maybe to fix this problem > (if reverting isn't an option) would be to add a shutdown hook to > rpmh-icc that effectively "props up" the bandwidth requests during > shutdown so that we don't have to think about finding the place that the > interconnect is turned off. We're shutting down/restarting anyway, so > there isn't much point in trying to be power efficient for the last few > moments of runtime. > I wouldn't have expected this change to impact reboot, since this change should only impact places where pre_aggregate() is called without subsequently calling aggregate(). I don't think there are currently any places that can happen other than icc_sync_state(). I suppose what could be happening is we're now disabling certain paths in icc_sync_state() and their associated drivers just aren't used or attempting accesses until they're being torn down in reboot. That doesn't seem particularly likely, but nothing else immediately comes to mind. We already mark paths critical for the CPU as "keepalive" such that they'll never turn off. This includes the CPU path to DDR and top-level CSRs. Basically just paths that can't actually be turned off while SW is running. That logic is unchanged in this patch. So we generally shouldn't need any shutdown-specific callbacks to place BW votes during this window. Client drivers should still ensure they're sequencing their shutdown logic such that any bus accesses happen before they remove their BW requests.