Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 13 Jan 2023 18:18:47 +0000
From:   Matthias Kaehlcke <mka@chromium.org>
To:     Rajendra Nayak <quic_rjendra@quicinc.com>,
        Georgi Djakov <djakov@kernel.org>
Cc:     krzysztof.kozlowski@linaro.org, agross@kernel.org,
        andersson@kernel.org, konrad.dybcio@somainline.org,
        robh+dt@kernel.org, linux-arm-msm@vger.kernel.org,
        linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
        linux-pm@vger.kernel.org, Douglas Anderson <dianders@chromium.org>,
        Stephen Boyd <swboyd@chromium.org>
Subject: Re: [PATCH v2 4/4] arm64: dts: qcom: sc7280: Add cpu and llcc BWMON
 (=> interconnect issue)
Message-ID: <Y8Ggh6RObbB1cxSS@google.com>
References: <20220902043511.17130-1-quic_rjendra@quicinc.com>
 <20220902043511.17130-5-quic_rjendra@quicinc.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20220902043511.17130-5-quic_rjendra@quicinc.com>
Precedence: bulk

Hi,

On Fri, Sep 02, 2022 at 10:05:11AM +0530, Rajendra Nayak wrote:
> Add cpu and llcc BWMON nodes and their corresponding
> OPP tables for sc7280 SoC.
> 
> Signed-off-by: Rajendra Nayak <quic_rjendra@quicinc.com>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

I found that with a v6.1 kernel AOSS on sc7280 doesn't reach it's low
power state during system. This can be observed on herobrine based
boards on which the AP_SUSPEND signal should transition to 1 during
system suspend. If it doesn't the Embedded Controller (EC) notices
it and wakes the system up again.

Bisection points to this patch, the issue only occurs when
CONFIG_QCOM_ICC_BWMON is *not* set. One might think the patch shouldn't
have any impact at all when the driver is not enabled, but it does.

Debugging shows that the issue is interconnect related. A bare platform
device is created for each bwmon devices, which results in the average
and peak bandwidth of the interconnect link to be set 'initially' to
INT_MAX. The driver is supposed to call icc_sync_state() during probe,
which would set the initially bandwidths to 0 and determine the actually
needed bandwidth. But since the driver isn't probed the initial
bandwidths stay at INT_MAX.

This isn't actually an issue with this patch, but how the interconnect
framework deals with devices that are registered on the bus, but aren't
probed (yet). Not sure how this would be best fixed. Georgi, do you have
any ideas?

Thanks

Matthias