Message ID | 20230123023127.1186619-1-bryan.odonoghue@linaro.org (mailing list archive) |
---|---|
Headers | show |
Series | Add MSM8939 SoC support with two devices | expand |
On 23/01/2023 02:31, Bryan O'Donoghue wrote:
> - Leaves dummy power-domain reference in cpu defintion as this
is required by the yaml
On 23/01/2023 02:31, Bryan O'Donoghue wrote: > V4: > - Adds Krzysztof's RB to snoc-mm > - Re-orders alphabetically missed nodes in previous iteration - Bjorn > - Adds LK address/size cells comment - Bjorn > > - Left _AO for wcnss as downstream reference uses this - Bjorn/Bryan > - Uses qcom,ids.h and QCOM_ID_SOCNAME for qcom,msm-id - Bjorn > - Revises comment from "Regulator" to "Power supply" - Bjorn > - Leaves dummy power-domain reference in cpu defintion as this > > - Relabels "cpu" to "CPU" to be more consistent with other dtsi - Bryan > - Moves msm8939 gcc to its own yaml file to capture 8939 specific form - Bryan > > is a required property and the dt checker complains - Stephan/Bryan > - Removes CPR entries from qfprom - Stephan > - Left MDSS interconnects. I don't see a bug to fix here - Stephan/Bryan > - power-domain in MDSS - dropped its not longer required after > commit a6f033938beb ("dt-bindings: msm: dsi-controller-main: Fix power-domain constraint") - Stephan > - 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 > > - pm8916_l16 -> pm8916_l6 in dsi definition, typo - Konrad > - Moved regulator_set_load location - Konrad > > Previous:https://lore.kernel.org/lkml/20230118050948.bibhq26s6sgzullg@builder.lan/T/ > Bootable:https://git.linaro.org/people/bryan.odonoghue/kernel.git/log/?h=linux-next-23-01-23-msm8939-nocpr Let me translate my 2:30 am email to english, I meant to group this by reviewer V4: - Adds Krzysztof's RB to snoc-mm - Re-orders alphabetically missed nodes in previous iteration - Bjorn - Adds LK address/size cells comment - Bjorn - Left _AO for wcnss as downstream reference uses this - Bjorn/Bryan - Uses qcom,ids.h and QCOM_ID_SOCNAME for qcom,msm-id - Bjorn - Revises comment from "Regulator" to "Power supply" - Bjorn - Leaves dummy power-domain reference in cpu defintion as this is a required property and the dt checker complains - Stephan/Bryan - Relabels "cpu" to "CPU" to be more consistent with other dtsi - Bryan - Moves msm8939 gcc to its own yaml file to 8939 specifics -Bryan - Removes CPR entries from qfprom - Stephan - Left MDSS interconnects. I don't see a bug to fix here - Stephan/Bryan - power-domain in MDSS - dropped its not longer required after commit a6f033938beb ("dt-bindings: msm: dsi-controller-main: Fix power-domain constraint") - Stephan - 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 - pm8916_l16 -> pm8916_l6 in dsi definition, typo - Konrad - Moved regulator_set_load location - Konrad
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
On 23/01/2023 12:49, Stephan Gerhold wrote:
> Also: The undocumented "register-mem" interconnect is still there.
On 23/01/2023 12:49, Stephan Gerhold wrote: > 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]. :/ Didn't forget, tested that and as I recall there are side-effects removing 8939 from drivers/cpufreq/cpufreq-dt-platdev.c - not all processors were booted. > 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.
On Mon, Jan 23, 2023 at 01:23:22PM +0000, Bryan O'Donoghue wrote: > On 23/01/2023 12:49, Stephan Gerhold wrote: > > 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]. :/ > > Didn't forget, tested that and as I recall there are side-effects removing > 8939 from drivers/cpufreq/cpufreq-dt-platdev.c - not all processors were > booted. > The cpufreq-dt-platdev.c addition for MSM8939 does not exist upstream because you dropped it in your v3 back then. You just kept the DT schema part. I don't have that addition and have no problems with SMP boot so I would say it works fine without. > > 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.
On 23/01/2023 14:00, Stephan Gerhold wrote: > Unless this conclusion changes with your CPR patch set this means that > both the DTS and the DT schema will need changes anyway, because you > wouldn't need power-domain-names = "cpr", but rather > > power-domains = <&rpmpd MSM8939_VDDMX_AO>, <&vreg_dummy>; > power-domain-names = "mx", "cpr"; I have not been owning the CPR for 8939 so far but, this what we have in our 4.19 tree. CPU0: cpu@100 { device_type = "cpu"; compatible = "arm,cortex-a53", "arm,armv8"; reg = <0x100>; next-level-cache = <&L2_1>; enable-method = "qcom,kpss-acc-v2"; qcom,acc = <&acc0>; qcom,saw = <&saw0>; clocks = <&apcs1>; operating-points-v2 = <&cluster1_opp_table>; power-domains = <&cpr>; power-domain-names = "cpr"; #cooling-cells = <2>; capacity-dmips-mhz = <1024>; }; cpr: power-controller@b018000 { compatible = "qcom,msm8939-cpr", "qcom,cpr"; reg = <0x0b018000 0x1000>; interrupts = <0 15 IRQ_TYPE_EDGE_RISING>; clocks = <&rpmcc CXO_SMD_CXO_A_CLK>; clock-names = "ref"; power-domains = <&rpmpd MSM8939_VDDMX_AO>; #power-domain-cells = <0>; operating-points-v2 = <&cpr_opp_table>; }; So the CPR code not the CPU code owns VDDMX_AO. I'm not sure if there's a good reason why it has been done that way. Anyway, this feels like a bit of a departure from our core discussion. I will see if it is possible to drop the CPU power-domain entirely contingent on the patch you flagged. --- bod
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
On 23/01/2023 16:21, Konrad Dybcio wrote: >> 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 WOWLAN is done via SMD not by raising of an interrupt between WCNSS and APSS directly and we do hit VDD min with AO in 4.19. So, so counter-intuitively so long as the SMD interrupt is unmasked in suspend - not a specific WCNSS interrupt, we will wake on WLAN. Its a complete tangent but, the WCNSS firmware has an SMD RPC call called "wake-on-wlan" or somesuch which *would* wake the system via interrupt but, appears to never have been implemented... Anyway. --- bod
On 23/01/2023 12:49, Stephan Gerhold wrote: >> - 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? So my understanding of the clock tree here is that dsi0_phy_pll_out_byteclk is a legacy name. Its perfectly possible to have DSI0 and DSI0_PHY switched off and to have DSI1/DSI1_PHY operable. dsi0_phy_pll_out_byteclk is perhaps an unfortunate name and probably should have been renamed. > 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 A better name would have been dsiX_phy_pll_out_byteclk. --- bod
On 26.01.2023 16:29, Bryan O'Donoghue wrote: > On 23/01/2023 12:49, Stephan Gerhold wrote: >>> - 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? > > So my understanding of the clock tree here is that dsi0_phy_pll_out_byteclk is a legacy name. > > Its perfectly possible to have DSI0 and DSI0_PHY switched off and to have DSI1/DSI1_PHY operable. > > dsi0_phy_pll_out_byteclk is perhaps an unfortunate name and probably should have been renamed. > >> 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 > > A better name would have been dsiX_phy_pll_out_byteclk. I believe Stephan is just confused what the clock source of both pairs of GCC DSI clocks are, as you're suggesting that: phy_clock0 |_gcc_clock0 and phy_clock0 (yes, zero) |_gcc_clock1 whereas on most other SoCs the following is true: phy_clock0 |_gcc_clock0 phy_clock1 |_gcc_clock_1 Konrad > > --- > bod
On 26/01/2023 15:34, Konrad Dybcio wrote: >>> 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 >> A better name would have been dsiX_phy_pll_out_byteclk. > I believe Stephan is just confused what the clock source of both > pairs of GCC DSI clocks are, as you're suggesting that: > > phy_clock0 > |_gcc_clock0 > > and > > phy_clock0 (yes, zero) > |_gcc_clock1 > > whereas on most other SoCs the following is true: > > phy_clock0 > |_gcc_clock0 > > phy_clock1 > |_gcc_clock_1 > > Konrad The only input clock to GCC is XO or buffered CXO if routed through the PMIC. You can select via GCC::RCGR where dsiX_phy_pll_out_byteclk is *sourced* from XO, GPLL0_AUX or P_DSI0_PHYPLL_BYTE. So, obvs the byte clock can be any one of those input sources. But the question is, if you select dsi0_phy_pll_out_byteclk - what provides it ? Reviewing the LK bootloader for 3.18, it *looks* to me like the dsi0 pll is always switched on. The downstream kernel tree doesn't represent that. 0x01A9811C MDSS_DSI_0_CLK_CTRL Type: RW Reset State: 0x00000000 -> BIT(4) -> Turns on/off BYTECLK for the DSI. If set to 1, clock is ON. Hmm. I think actually it must be the case that DSI1 is a slave of DSI0. You can have both interfaces running or just DSI0 on its own. Hmm, I'll change it. --- bod
On 26/01/2023 16:32, Bryan O'Donoghue wrote: > The only input clock to GCC is XO or buffered CXO if routed through the > PMIC. > > You can select via GCC::RCGR where dsiX_phy_pll_out_byteclk is *sourced* > from XO, GPLL0_AUX or P_DSI0_PHYPLL_BYTE. > > So, obvs the byte clock can be any one of those input sources. > > But the question is, if you select dsi0_phy_pll_out_byteclk - what > provides it ? > > Reviewing the LK bootloader for 3.18, it *looks* to me like the dsi0 pll > is always switched on. The downstream kernel tree doesn't represent that. > > 0x01A9811C MDSS_DSI_0_CLK_CTRL > Type: RW > Reset State: 0x00000000 -> BIT(4) -> Turns on/off BYTECLK for the DSI. > If set to 1, clock is ON. > > Hmm. I think actually it must be the case that DSI1 is a slave of DSI0. * If and only if you set P_DSI0_PHYPLL_BYTE::SRC_SEL = 0x01, using SRC_SEL = 0 (XO) or SRC_SEL = 0x02 (GPLL0_AUX) should negate the dependency. I'll review downstream further - perhaps DSI1 in practice doesn't set P_DSI0_PHYPLL_BYTE as the source clock.. --- bod