Date:   Mon, 23 Jan 2023 13:49:02 +0100
From:   Stephan Gerhold <stephan@gerhold.net>
To:     Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Cc:     agross@kernel.org, andersson@kernel.org, konrad.dybcio@linaro.org,
        djakov@kernel.org, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, linux-arm-msm@vger.kernel.org,
        linux-pm@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, benl@squareup.com,
        shawn.guo@linaro.org, fabien.parent@linaro.org, leo.yan@linaro.org,
        dmitry.baryshkov@linaro.org
Subject: Re: [PATCH v4 0/6] Add MSM8939 SoC support with two devices
Message-ID: <Y86CPmgvAi+kChQI@gerhold.net>
References: <20230123023127.1186619-1-bryan.odonoghue@linaro.org>
 <42baa874-c926-9111-b0b3-2df2562d8de6@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <42baa874-c926-9111-b0b3-2df2562d8de6@linaro.org>
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Mon, Jan 23, 2023 at 11:08:28AM +0000, Bryan O'Donoghue wrote:
> V4:
> - Left _AO for wcnss as downstream reference uses this - Bjorn/Bryan

Downstream is just an implementation and contains plenty of misleading
or even wrong information. IMO Bjorn is right here that VDDMX_AO is not
a logical choice.

The _AO (active-only) suffix means that the votes are only applied when
the processor making the vote is "active", that is when the Linux CPUs
are not in deep cpuidle mode.

For WCNSS the goal is to keep the necessary power domains active while
WCNSS is booting up, until it is able to make its own votes (handover).
The WCNSS firmware might then vote for VDDMX_AO internally because VDDMX
is not needed when the WCNSS CPU is suspended.

However, I would expect that the meaning is totally different when the
same vote is made from Linux. When Linux votes for _AO the "active"
state likely refers to the Linux CPUs, instead of the WCNSS CPU when
made from the WCNSS firmware.

Why does it work in downstream then? I would just assume "side effects":
  - Something else votes for VDDMX without _AO while WCNSS is booting
  - The Linux CPUs don't go into deep cpuidle state during startup
    - In particular, note how downstream often has "lpm_levels.sleep_disabled=1"
      on the kernel command line. This disables all cpuidle states until
      late after boot-up when userspace changes this setting. Without
      cpuidle, VDDMX_AO is identical to VDDMX.

Please change it to VDDMX (without _AO). It will most likely not make
any difference, but IMO it is logcially more correct and less
confusing/misleading. :)

> - Leaves dummy power-domain reference in cpu defintion as this is a
>   required property and the dt checker complains - Stephan/Bryan

It's only required though because you forgot to drop the DT schema patch
(3/4) when I suggested half a year ago that you make the MSM8939
cpufreq-qcom-nvmem changes together with the CPR stack [1]. :/

Anyway, it looks like qcom-cpufreq-nvmem.yaml requiring "cpr" power
domain unconditionally is a mistake anyway for multiple platforms.
[2] was recently submitted to fix this so that patch should allow you to
drop the dummy nodes. :)

[1]: https://lore.kernel.org/linux-arm-msm/Ysf8VRaXdGg+8Ev3@gerhold.net/
[2]: https://lore.kernel.org/linux-arm-msm/20230122174548.13758-1-ansuelsmth@gmail.com/

> - Left MDSS interconnects. I don't see a bug to fix here - Stephan/Bryan

Fair enough, if you would like to keep it I will likely send a revert
for the MSM8939 icc_sync_state() though. Because clearly it breaks
setups without a display and I don't see how one would fix that from the
device tree.

Also: The undocumented "register-mem" interconnect is still there. :)

> - power-domain in MDSS - dropped its not longer required after
>   commit a6f033938beb ("dt-bindings: msm: dsi-controller-main: Fix
> power-domain constraint") - Stephan

Thanks!

> - Adds gcc dsi1pll and dsi1pllbyte to gcc clock list.
>   Reviewing the silicon documentation we see dsi0_phy_pll is used to clock
>   GCC_BYTE1_CFG_RCGR : SRC_SEL
>   Root Source Select
>   000 : cxo
>   001 : dsi0_phy_pll_out_byteclk
>   010 : GPLL0_OUT_AUX
>   011 : gnd
>   100 : gnd
>   101 : gnd
>   110 : gnd
>   111 : reserved - Stephan/Bryan
> 

I'm confused. Are you not contradicting yourself here? You say that
dsi0_phy_pll (dsi ZERO) is used to clock GCC_BYTE1_CFG_RCGR. Then why
do you add dsi1_phy_pll (dsi ONE) to the gcc clock list?

To me this looks like a confirmation of what downstream does, that both
DSI byte clocks are actually sourced from the dsi0_phy and the PLL of
dsi1_phy is not used.

Thanks,
Stephan