All of lore.kernel.org
 help / color / mirror / Atom feed
From: Mike Tipton <mdtipton@codeaurora.org>
To: Stephen Boyd <swboyd@chromium.org>, Alex Elder <elder@linaro.org>,
	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
Subject: Re: [PATCH v2 4/4] interconnect: qcom: icc-rpmh: Add BCMs to commit list in pre_aggregate
Date: Tue, 17 Aug 2021 21:43:50 -0700	[thread overview]
Message-ID: <49eeb675-3c76-ef3e-708d-230ec980202f@codeaurora.org> (raw)
In-Reply-To: <CAE-0n53GfD-8d0NJ+Hv1wcx0DDACc5_gT3qV0NR-vLiZgtCKpg@mail.gmail.com>

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 <mdtipton@codeaurora.org>
>>>> ---
>>>
>>> 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.

      reply	other threads:[~2021-08-18  4:44 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-21 17:54 [PATCH v2 0/4] interconnect: Fix sync-state issues Mike Tipton
2021-07-21 17:54 ` [PATCH v2 1/4] interconnect: Zero initial BW after sync-state Mike Tipton
2021-07-21 17:54 ` [PATCH v2 2/4] interconnect: Always call pre_aggregate before aggregate Mike Tipton
2021-07-21 17:54 ` [PATCH v2 3/4] interconnect: qcom: icc-rpmh: Ensure floor BW is enforced for all nodes Mike Tipton
2021-07-21 17:54 ` [PATCH v2 4/4] interconnect: qcom: icc-rpmh: Add BCMs to commit list in pre_aggregate Mike Tipton
2021-08-10 23:31   ` Stephen Boyd
2021-08-11  0:18     ` Bjorn Andersson
2021-08-11  4:22       ` Stephen Boyd
2021-08-18  4:43       ` Mike Tipton
2021-08-11 16:01     ` Alex Elder
2021-08-11 18:13       ` Stephen Boyd
2021-08-18  4:43         ` Mike Tipton [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=49eeb675-3c76-ef3e-708d-230ec980202f@codeaurora.org \
    --to=mdtipton@codeaurora.org \
    --cc=agross@kernel.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=djakov@kernel.org \
    --cc=elder@linaro.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=okukatla@codeaurora.org \
    --cc=saravanak@google.com \
    --cc=swboyd@chromium.org \
    --subject='Re: [PATCH v2 4/4] interconnect: qcom: icc-rpmh: Add BCMs to commit list in pre_aggregate' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.