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@xxxxxxxxxxx/ > [2]: https://lore.kernel.org/linux-arm-msm/20230122174548.13758-1-ansuelsmth@xxxxxxxxx/ > >> - 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
