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;
Message-ID: <38dc17a1-a39e-7c1f-3377-beca19a01df2@linaro.org>
Date:   Tue, 16 May 2023 02:30:18 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH] arm64: dts: qcom: sc7180-lite: Fix SDRAM freq for
 misidentified sc7180-lite boards
Content-Language: en-US
To:     Douglas Anderson <dianders@chromium.org>,
        Bjorn Andersson <andersson@kernel.org>
Cc:     Andy Gross <agross@kernel.org>, Conor Dooley <conor+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        Sibi Sankar <sibis@codeaurora.org>,
        cros-qcom-dts-watchers@chromium.org, devicetree@vger.kernel.org,
        linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20230515171929.1.Ic8dee2cb79ce39ffc04eab2a344dde47b2f9459f@changeid>
From:   Konrad Dybcio <konrad.dybcio@linaro.org>
In-Reply-To: <20230515171929.1.Ic8dee2cb79ce39ffc04eab2a344dde47b2f9459f@changeid>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 16.05.2023 02:19, Douglas Anderson wrote:
> In general, the three SKUs of sc7180 (lite, normal, and pro) are
> handled dynamically.
> 
> The cpufreq table in sc7180.dtsi includes the superset of all CPU
> frequencies. The "qcom-cpufreq-hw" driver in Linux shows that we can
> dynamically detect which frequencies are actually available on the
> currently running CPU and then we can just enable those ones.
> 
> The GPU is similarly dynamic. The nvmem has a fuse in it (see
> "gpu_speed_bin" in sc7180.dtsi) that the GPU driver can use to figure
> out which frequencies to enable.
> 
> There is one part, however, that is not so dynamic. The way SDRAM
> frequency works in sc7180 is that it's tied to cpufreq. At the busiest
> cpufreq operating points we'll pick the top supported SDRAM frequency.
> They ramp down together.
> 
> For the "pro" SKU of sc7180, we only enable one extra cpufreq step.
> That extra cpufreq step runs SDRAM at the same speed as the step
> below. Thus, for normal and pro things are OK. There is no sc7180-pro
> device tree snippet.
> 
> For the "lite" SKU if sc7180, however, things aren't so easy. The
> "lite" SKU drops 3 cpufreq entries but can still run SDRAM at max
> frequency. That messed things up with the whole scheme. This is why we
> added the "sc7180-lite" fragment in commit 8fd01e01fd6f ("arm64: dts:
> qcom: sc7180-lite: Tweak DDR/L3 scaling on SC7180-lite").
> 
> When the lite scheme came about, it was agreed that the WiFi SKUs of
> lazor would _always_ be "lite" and would, in fact, be the only "lite"
> devices. Unfortunately, this decision changed and folks didn't realize
> that it would be a problem. Specifically, some later lazor WiFi-only
> devices were built with "pro" CPUs.
> 
> Building WiFi-only lazor with "pro" CPUs isn't the end of the world.
> The SDRAM will ramp up a little sooner than it otherwise would, but
> aside from a small power hit things work OK. One problem, though, is
> that the SDRAM scaling becomes a bit quirky. Specifically, with the
> current tables we'll max out SDRAM frequency at 2.1GHz but then
> _lower_ it at 2.2GHz / 2.3GHz only to raise it back to max for 2.4GHz
> and 2.55GHz.
> 
> Let's at least fix this so that the SDRAM frequency doesn't go down in
> that quirky way. On true "lite" SKUs this change will be a no-op
> because the operating points we're touching are disabled. This change
> is only useful when a board that thinks it has a "lite" CPU actually
> has a "normal" or "pro" one stuffed.
> 
> Fixes: 8fd01e01fd6f ("arm64: dts: qcom: sc7180-lite: Tweak DDR/L3 scaling on SC7180-lite")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
I'd love it if you wrote a book detailing all the crazy stories like
this one, given it was probably not a one-off :P

Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
Cc: <stable@vger.kernel.org>

Konrad
> 
>  arch/arm64/boot/dts/qcom/sc7180-lite.dtsi | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-lite.dtsi b/arch/arm64/boot/dts/qcom/sc7180-lite.dtsi
> index d8ed1d7b4ec7..4b306a59d9be 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-lite.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180-lite.dtsi
> @@ -16,3 +16,11 @@ &cpu6_opp11 {
>  &cpu6_opp12 {
>  	opp-peak-kBps = <8532000 23347200>;
>  };
> +
> +&cpu6_opp13 {
> +	opp-peak-kBps = <8532000 23347200>;
> +};
> +
> +&cpu6_opp14 {
> +	opp-peak-kBps = <8532000 23347200>;
> +};