Message ID | 20230123023127.1186619-1-bryan.odonoghue@linaro.org |
---|---|
Headers | show |
Series | Add MSM8939 SoC support with two devices | expand |
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 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 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