Re: [PATCH v4 0/6] Add MSM8939 SoC support with two devices

[Konrad Dybcio <konrad.dybcio@linaro.org>]

On 23.01.2023 13:49, Stephan Gerhold wrote:
> 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
Wouldn't it make wake-on-wifi-with-cpus-off possible?
(obviously given the wlan chip supports it and can ping
the cpu etc etc)

Konrad

 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