From: Amit Kucheria <amit.kucheria@linaro.org>
To: Loic Poulain <loic.poulain@linaro.org>
Cc: Stephen Boyd <sboyd@kernel.org>,
Bjorn Andersson <bjorn.andersson@linaro.org>,
Michael Turquette <mturquette@baylibre.com>,
linux-arm-msm <linux-arm-msm@vger.kernel.org>,
linux-clk <linux-clk@vger.kernel.org>
Subject: Re: [PATCH v3 4/4] arch: arm64: dts: msm8996: Add opp and thermal
Date: Thu, 2 Jul 2020 19:12:27 +0530 [thread overview]
Message-ID: <CAP245DXEvD=ZX507FENHCmBhW+ecOOS3Q=GT+N01Fa-FHhL_jQ@mail.gmail.com> (raw)
In-Reply-To: <CAMZdPi-oJTH4tdvvi5PUHRHmP=2=PHpL4YV3nhO3fOLV=882rw@mail.gmail.com>
On Thu, Jul 2, 2020 at 5:39 PM Loic Poulain <loic.poulain@linaro.org> wrote:
>
>
>
> On Thu, 2 Jul 2020 at 12:02, Amit Kucheria <amit.kucheria@linaro.org> wrote:
>>
>> On Thu, Jul 2, 2020 at 3:04 PM Loic Poulain <loic.poulain@linaro.org> wrote:
>> >
>> > };
>> >
>> > @@ -424,7 +449,7 @@
>> > bits = <1 4>;
>> > };
>> >
>> > - gpu_speed_bin: gpu_speed_bin@133 {
>> > + speedbin_efuse: speedbin@133 {
>> > reg = <0x133 0x1>;
>> > bits = <5 3>;
>> > };
>> > @@ -642,7 +667,7 @@
>> > power-domains = <&mmcc GPU_GDSC>;
>> > iommus = <&adreno_smmu 0>;
>> >
>> > - nvmem-cells = <&gpu_speed_bin>;
>> > + nvmem-cells = <&speedbin_efuse>;
>> > nvmem-cell-names = "speed_bin";
>>
>> This bit changing the name of the node should be a separate patch, IMO.
>
>
> Well, I think it makes sense to change the name here since the new introduced
> cpu opp tables make use of the speedbin value as well, keeping gpu name would
> be misleading.
That is the point I'm making. This rename should be done earlier in
your series as a bug fix since obviously the node name is not just
related to gpu.
Then in this patch you just the corrected name.
>>
>> > qcom,gpu-quirk-two-pass-use-wfi;
>> > @@ -1703,8 +1728,9 @@
>> > };
>> > };
>> > };
>> > +
>> > kryocc: clock-controller@6400000 {
>> > - compatible = "qcom,apcc-msm8996";
>> > + compatible = "qcom,msm8996-apcc";
>> > reg = <0x06400000 0x90000>;
>> > #clock-cells = <1>;
>> > };
>> > @@ -2172,6 +2198,229 @@
>> > sound: sound {
>> > };
>> >
>> > + cluster0_opp: opp_table0 {
>> > + compatible = "operating-points-v2-qcom-cpu";
>> > + nvmem-cells = <&speedbin_efuse>;
>> > + opp-shared;
>> > +
>> > + /* Nominal fmax for now */
>> > +
>> > + opp-307200000 {
>> > + opp-hz = /bits/ 64 < 307200000 >;
>> > + opp-supported-hw = <0x77>;
>> > + clock-latency-ns = <200000>;
>> > + };
>> > + opp-422400000 {
>> > + opp-hz = /bits/ 64 < 422400000 >;
>> > + opp-supported-hw = <0x77>;
>> > + clock-latency-ns = <200000>;
>> > + };
>> > + opp-480000000 {
>> > + opp-hz = /bits/ 64 < 480000000 >;
>> > + opp-supported-hw = <0x77>;
>> > + clock-latency-ns = <200000>;
>> > + };
>> > + opp-556800000 {
>> > + opp-hz = /bits/ 64 < 556800000 >;
>> > + opp-supported-hw = <0x77>;
>> > + clock-latency-ns = <200000>;
>> > + };
>> > + opp-652800000 {
>> > + opp-hz = /bits/ 64 < 652800000 >;
>> > + opp-supported-hw = <0x77>;
>> > + clock-latency-ns = <200000>;
>> > + };
>> > + opp-729600000 {
>> > + opp-hz = /bits/ 64 < 729600000 >;
>> > + opp-supported-hw = <0x77>;
>> > + clock-latency-ns = <200000>;
>> > + };
>> > + opp-844800000 {
>> > + opp-hz = /bits/ 64 < 844800000 >;
>> > + opp-supported-hw = <0x77>;
>> > + clock-latency-ns = <200000>;
>> > + };
>> > + opp-960000000 {
>> > + opp-hz = /bits/ 64 < 960000000 >;
>> > + opp-supported-hw = <0x77>;
>> > + clock-latency-ns = <200000>;
>> > + };
>> > + opp-1036800000 {
>> > + opp-hz = /bits/ 64 < 1036800000 >;
>> > + opp-supported-hw = <0x77>;
>> > + clock-latency-ns = <200000>;
>> > + };
>> > + opp-1113600000 {
>> > + opp-hz = /bits/ 64 < 1113600000 >;
>> > + opp-supported-hw = <0x77>;
>> > + clock-latency-ns = <200000>;
>> > + };
>> > + opp-1190400000 {
>> > + opp-hz = /bits/ 64 < 1190400000 >;
>> > + opp-supported-hw = <0x77>;
>> > + clock-latency-ns = <200000>;
>> > + };
>> > + opp-1228800000 {
>> > + opp-hz = /bits/ 64 < 1228800000 >;
>> > + opp-supported-hw = <0x77>;
>> > + clock-latency-ns = <200000>;
>> > + };
>> > + opp-1324800000 {
>> > + opp-hz = /bits/ 64 < 1324800000 >;
>> > + opp-supported-hw = <0x77>;
>> > + clock-latency-ns = <200000>;
>> > + };
>> > + opp-1401600000 {
>> > + opp-hz = /bits/ 64 < 1401600000 >;
>> > + opp-supported-hw = <0x77>;
>> > + clock-latency-ns = <200000>;
>> > + };
>> > + opp-1478400000 {
>> > + opp-hz = /bits/ 64 < 1478400000 >;
>> > + opp-supported-hw = <0x77>;
>> > + clock-latency-ns = <200000>;
>> > + };
>> > + opp-1593600000 {
>> > + opp-hz = /bits/ 64 < 1593600000 >;
>> > + opp-supported-hw = <0x77>;
>> > + clock-latency-ns = <200000>;
>> > + };
>> > + };
>> > +
>> > + cluster1_opp: opp_table1 {
>> > + compatible = "operating-points-v2-qcom-cpu";
>> > + nvmem-cells = <&speedbin_efuse>;
>> > + opp-shared;
>> > +
>> > + /* Nominal fmax for now */
>> > +
>> > + opp-307200000 {
>> > + opp-hz = /bits/ 64 < 307200000 >;
>> > + opp-supported-hw = <0x77>;
>> > + clock-latency-ns = <200000>;
>> > + };
>> > + opp-403200000 {
>> > + opp-hz = /bits/ 64 < 403200000 >;
>> > + opp-supported-hw = <0x77>;
>> > + clock-latency-ns = <200000>;
>> > + };
>> > + opp-480000000 {
>> > + opp-hz = /bits/ 64 < 480000000 >;
>> > + opp-supported-hw = <0x77>;
>> > + clock-latency-ns = <200000>;
>> > + };
>> > + opp-556800000 {
>> > + opp-hz = /bits/ 64 < 556800000 >;
>> > + opp-supported-hw = <0x77>;
>> > + clock-latency-ns = <200000>;
>> > + };
>> > + opp-652800000 {
>> > + opp-hz = /bits/ 64 < 652800000 >;
>> > + opp-supported-hw = <0x77>;
>> > + clock-latency-ns = <200000>;
>> > + };
>> > + opp-729600000 {
>> > + opp-hz = /bits/ 64 < 729600000 >;
>> > + opp-supported-hw = <0x77>;
>> > + clock-latency-ns = <200000>;
>> > + };
>> > + opp-806400000 {
>> > + opp-hz = /bits/ 64 < 806400000 >;
>> > + opp-supported-hw = <0x77>;
>> > + clock-latency-ns = <200000>;
>> > + };
>> > + opp-883200000 {
>> > + opp-hz = /bits/ 64 < 883200000 >;
>> > + opp-supported-hw = <0x77>;
>> > + clock-latency-ns = <200000>;
>> > + };
>> > + opp-940800000 {
>> > + opp-hz = /bits/ 64 < 940800000 >;
>> > + opp-supported-hw = <0x77>;
>> > + clock-latency-ns = <200000>;
>> > + };
>> > + opp-1036800000 {
>> > + opp-hz = /bits/ 64 < 1036800000 >;
>> > + opp-supported-hw = <0x77>;
>> > + clock-latency-ns = <200000>;
>> > + };
>> > + opp-1113600000 {
>> > + opp-hz = /bits/ 64 < 1113600000 >;
>> > + opp-supported-hw = <0x77>;
>> > + clock-latency-ns = <200000>;
>> > + };
>> > + opp-1190400000 {
>> > + opp-hz = /bits/ 64 < 1190400000 >;
>> > + opp-supported-hw = <0x77>;
>> > + clock-latency-ns = <200000>;
>> > + };
>> > + opp-1248000000 {
>> > + opp-hz = /bits/ 64 < 1248000000 >;
>> > + opp-supported-hw = <0x77>;
>> > + clock-latency-ns = <200000>;
>> > + };
>> > + opp-1324800000 {
>> > + opp-hz = /bits/ 64 < 1324800000 >;
>> > + opp-supported-hw = <0x77>;
>> > + clock-latency-ns = <200000>;
>> > + };
>> > + opp-1401600000 {
>> > + opp-hz = /bits/ 64 < 1401600000 >;
>> > + opp-supported-hw = <0x77>;
>> > + clock-latency-ns = <200000>;
>> > + };
>> > + opp-1478400000 {
>> > + opp-hz = /bits/ 64 < 1478400000 >;
>> > + opp-supported-hw = <0x77>;
>> > + clock-latency-ns = <200000>;
>> > + };
>> > + opp-1555200000 {
>> > + opp-hz = /bits/ 64 < 1555200000 >;
>> > + opp-supported-hw = <0x77>;
>> > + clock-latency-ns = <200000>;
>> > + };
>> > + opp-1632000000 {
>> > + opp-hz = /bits/ 64 < 1632000000 >;
>> > + opp-supported-hw = <0x77>;
>> > + clock-latency-ns = <200000>;
>> > + };
>> > + opp-1708800000 {
>> > + opp-hz = /bits/ 64 < 1708800000 >;
>> > + opp-supported-hw = <0x77>;
>> > + clock-latency-ns = <200000>;
>> > + };
>> > + opp-1785600000 {
>> > + opp-hz = /bits/ 64 < 1785600000 >;
>> > + opp-supported-hw = <0x77>;
>> > + clock-latency-ns = <200000>;
>> > + };
>> > + opp-1824000000 {
>> > + opp-hz = /bits/ 64 < 1824000000 >;
>> > + opp-supported-hw = <0x77>;
>> > + clock-latency-ns = <200000>;
>> > + };
>> > + opp-1920000000 {
>> > + opp-hz = /bits/ 64 < 1920000000 >;
>> > + opp-supported-hw = <0x77>;
>> > + clock-latency-ns = <200000>;
>> > + };
>> > + opp-1996800000 {
>> > + opp-hz = /bits/ 64 < 1996800000 >;
>> > + opp-supported-hw = <0x77>;
>> > + clock-latency-ns = <200000>;
>> > + };
>> > + opp-2073600000 {
>> > + opp-hz = /bits/ 64 < 2073600000 >;
>> > + opp-supported-hw = <0x77>;
>> > + clock-latency-ns = <200000>;
>> > + };
>> > + opp-2150400000 {
>> > + opp-hz = /bits/ 64 < 2150400000 >;
>> > + opp-supported-hw = <0x77>;
>> > + clock-latency-ns = <200000>;
>> > + };
>> > + };
>> > +
>> > thermal-zones {
>> > cpu0-thermal {
>> > polling-delay-passive = <250>;
>> > @@ -2180,18 +2429,33 @@
>> > thermal-sensors = <&tsens0 3>;
>> >
>> > trips {
>> > - cpu0_alert0: trip-point@0 {
>> > + cpu_alert0: cpu_alert0 {
>>
>> Please use the node name pattern we're using in some of the other dtsi
>> filenames for consistency. See sdm845.dtsi for an example.
>>
>> This also fixes dtsi warnings about address units, I suspect. So the
>> name change may be a separate patch just to fix the warnings, if you
>> want.
>
>
> Ok, rebasing on master now to get your changes.
>
>>
>>
>> > temperature = <75000>;
>> > hysteresis = <2000>;
>> > + type = "active";
>>
>> We only need a passive trip type for cpufreq driven cooling. active
>> trip types are for driving fans and such.
>
>
> Right, will change that.
>
> Loic
prev parent reply other threads:[~2020-07-02 13:42 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-02 9:39 [PATCH v3 0/4] msm8996 CPU scaling suppor Loic Poulain
2020-07-02 9:39 ` [PATCH v3 1/4] soc: qcom: Separate kryo l2 accessors from PMU driver Loic Poulain
2020-07-02 9:39 ` [PATCH v3 2/4] clk: qcom: Add CPU clock driver for msm8996 Loic Poulain
2020-07-02 9:39 ` [PATCH v3 3/4] dt-bindings: clk: qcom: Add bindings for CPU clock " Loic Poulain
2020-07-02 9:39 ` [PATCH v3 4/4] arch: arm64: dts: msm8996: Add opp and thermal Loic Poulain
2020-07-02 10:01 ` Amit Kucheria
2020-07-02 10:31 ` Amit Kucheria
[not found] ` <CAMZdPi-oJTH4tdvvi5PUHRHmP=2=PHpL4YV3nhO3fOLV=882rw@mail.gmail.com>
2020-07-02 13:42 ` Amit Kucheria [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='CAP245DXEvD=ZX507FENHCmBhW+ecOOS3Q=GT+N01Fa-FHhL_jQ@mail.gmail.com' \
--to=amit.kucheria@linaro.org \
--cc=bjorn.andersson@linaro.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-clk@vger.kernel.org \
--cc=loic.poulain@linaro.org \
--cc=mturquette@baylibre.com \
--cc=sboyd@kernel.org \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).