| Message ID | 20230717-topic-branch_aon_cleanup-v1-0-27784d27a4f4@linaro.org |
|---|---|
| Headers | show |
| Series | Unregister critical branch clocks + some RPM | expand |
On 17.07.2023 18:15, Stephan Gerhold wrote: > On Mon, Jul 17, 2023 at 05:19:12PM +0200, Konrad Dybcio wrote: >> Some clocks need to be always-on, but we don't really do anything >> with them, other than calling enable() once and telling Linux they're >> enabled. >> >> Unregister them to save a couple of bytes and, perhaps more >> importantly, allow for runtime suspend of the clock controller device, >> as CLK_IS_CRITICAL prevents the latter. >> >> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> >> --- > You seem to revert PATCH 03/15 here. > Argh, I had an iffy conflict resolution that I thought I had gotten rid of.. but looks like in the end my brain farted anyway.. Thanks for spotting that! Konrad
On 17.07.2023 18:28, Stephan Gerhold wrote: > On Mon, Jul 17, 2023 at 05:19:22PM +0200, Konrad Dybcio wrote: >> The GPU_CC block is powered by VDD_CX. Describe that. >> >> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> >> --- >> arch/arm64/boot/dts/qcom/sm6115.dtsi | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi >> index 29b5b388cd94..bfaaa1801a4d 100644 >> --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi >> +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi >> @@ -1430,6 +1430,8 @@ gpucc: clock-controller@5990000 { >> clocks = <&rpmcc RPM_SMD_XO_CLK_SRC>, >> <&gcc GCC_GPU_GPLL0_CLK_SRC>, >> <&gcc GCC_GPU_GPLL0_DIV_CLK_SRC>; >> + power-domains = <&rpmpd SM6115_VDDCX>; >> + required-opps = <&rpmpd_opp_low_svs>; > > Where is this required-opp coming from? The clocks in gpucc seem to have > different voltage requirements depending on the rates, but we usually > handle that in the OPP tables of the consumer. The only lower levels defined for this SoC are VDD_MIN and VDD_RET, but quite obviously the GPU won't work then Konrad > > Thanks, > Stephan
On Mon, Jul 17, 2023 at 06:50:18PM +0200, Konrad Dybcio wrote: > On 17.07.2023 18:28, Stephan Gerhold wrote: > > On Mon, Jul 17, 2023 at 05:19:22PM +0200, Konrad Dybcio wrote: > >> The GPU_CC block is powered by VDD_CX. Describe that. > >> > >> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > >> --- > >> arch/arm64/boot/dts/qcom/sm6115.dtsi | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi > >> index 29b5b388cd94..bfaaa1801a4d 100644 > >> --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi > >> +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi > >> @@ -1430,6 +1430,8 @@ gpucc: clock-controller@5990000 { > >> clocks = <&rpmcc RPM_SMD_XO_CLK_SRC>, > >> <&gcc GCC_GPU_GPLL0_CLK_SRC>, > >> <&gcc GCC_GPU_GPLL0_DIV_CLK_SRC>; > >> + power-domains = <&rpmpd SM6115_VDDCX>; > >> + required-opps = <&rpmpd_opp_low_svs>; > > > > Where is this required-opp coming from? The clocks in gpucc seem to have > > different voltage requirements depending on the rates, but we usually > > handle that in the OPP tables of the consumer. > The only lower levels defined for this SoC are VDD_MIN and VDD_RET, > but quite obviously the GPU won't work then > The levels needed for the GPU clocks to run should be in the GPU OPP table though, just like e.g. sdhc2_opp_table for the SDCC clocks. I still don't really understand why this is specified here. :) Stephan
On 17.07.2023 19:23, Stephan Gerhold wrote: > On Mon, Jul 17, 2023 at 07:11:33PM +0200, Konrad Dybcio wrote: >> On 17.07.2023 18:56, Stephan Gerhold wrote: >>> On Mon, Jul 17, 2023 at 06:50:18PM +0200, Konrad Dybcio wrote: >>>> On 17.07.2023 18:28, Stephan Gerhold wrote: >>>>> On Mon, Jul 17, 2023 at 05:19:22PM +0200, Konrad Dybcio wrote: >>>>>> The GPU_CC block is powered by VDD_CX. Describe that. >>>>>> >>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> >>>>>> --- >>>>>> arch/arm64/boot/dts/qcom/sm6115.dtsi | 2 ++ >>>>>> 1 file changed, 2 insertions(+) >>>>>> >>>>>> diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi >>>>>> index 29b5b388cd94..bfaaa1801a4d 100644 >>>>>> --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi >>>>>> +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi >>>>>> @@ -1430,6 +1430,8 @@ gpucc: clock-controller@5990000 { >>>>>> clocks = <&rpmcc RPM_SMD_XO_CLK_SRC>, >>>>>> <&gcc GCC_GPU_GPLL0_CLK_SRC>, >>>>>> <&gcc GCC_GPU_GPLL0_DIV_CLK_SRC>; >>>>>> + power-domains = <&rpmpd SM6115_VDDCX>; >>>>>> + required-opps = <&rpmpd_opp_low_svs>; >>>>> >>>>> Where is this required-opp coming from? The clocks in gpucc seem to have >>>>> different voltage requirements depending on the rates, but we usually >>>>> handle that in the OPP tables of the consumer. >>>> The only lower levels defined for this SoC are VDD_MIN and VDD_RET, >>>> but quite obviously the GPU won't work then >>>> >>> >>> The levels needed for the GPU clocks to run should be in the GPU OPP >>> table though, just like e.g. sdhc2_opp_table for the SDCC clocks. >>> >>> I still don't really understand why this is specified here. :) >> The GPU_CC block needs this rail to be at a certain power level for >> register access. This describes that requirement. >> > > Can you show where this is defined downstream? On a quick look I didn't > see something like that anywhere. Or is this from some secret > documentation? As far as downstream goes, you can notice that no branch's or RCG's vdd tables ever define a level lower than the one I mentioned. Konrad
On Mon, Jul 17, 2023 at 07:11:33PM +0200, Konrad Dybcio wrote: > On 17.07.2023 18:56, Stephan Gerhold wrote: > > On Mon, Jul 17, 2023 at 06:50:18PM +0200, Konrad Dybcio wrote: > >> On 17.07.2023 18:28, Stephan Gerhold wrote: > >>> On Mon, Jul 17, 2023 at 05:19:22PM +0200, Konrad Dybcio wrote: > >>>> The GPU_CC block is powered by VDD_CX. Describe that. > >>>> > >>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > >>>> --- > >>>> arch/arm64/boot/dts/qcom/sm6115.dtsi | 2 ++ > >>>> 1 file changed, 2 insertions(+) > >>>> > >>>> diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi > >>>> index 29b5b388cd94..bfaaa1801a4d 100644 > >>>> --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi > >>>> +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi > >>>> @@ -1430,6 +1430,8 @@ gpucc: clock-controller@5990000 { > >>>> clocks = <&rpmcc RPM_SMD_XO_CLK_SRC>, > >>>> <&gcc GCC_GPU_GPLL0_CLK_SRC>, > >>>> <&gcc GCC_GPU_GPLL0_DIV_CLK_SRC>; > >>>> + power-domains = <&rpmpd SM6115_VDDCX>; > >>>> + required-opps = <&rpmpd_opp_low_svs>; > >>> > >>> Where is this required-opp coming from? The clocks in gpucc seem to have > >>> different voltage requirements depending on the rates, but we usually > >>> handle that in the OPP tables of the consumer. > >> The only lower levels defined for this SoC are VDD_MIN and VDD_RET, > >> but quite obviously the GPU won't work then > >> > > > > The levels needed for the GPU clocks to run should be in the GPU OPP > > table though, just like e.g. sdhc2_opp_table for the SDCC clocks. > > > > I still don't really understand why this is specified here. :) > The GPU_CC block needs this rail to be at a certain power level for > register access. This describes that requirement. > And that is not the lowest level reported by command db? Please describe this part in the commit message as well. Thanks, Bjorn
On Mon, Jul 17, 2023 at 09:18:21PM +0200, Konrad Dybcio wrote: > On 17.07.2023 19:23, Stephan Gerhold wrote: > > On Mon, Jul 17, 2023 at 07:11:33PM +0200, Konrad Dybcio wrote: > >> On 17.07.2023 18:56, Stephan Gerhold wrote: > >>> On Mon, Jul 17, 2023 at 06:50:18PM +0200, Konrad Dybcio wrote: > >>>> On 17.07.2023 18:28, Stephan Gerhold wrote: > >>>>> On Mon, Jul 17, 2023 at 05:19:22PM +0200, Konrad Dybcio wrote: > >>>>>> The GPU_CC block is powered by VDD_CX. Describe that. > >>>>>> > >>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > >>>>>> --- > >>>>>> arch/arm64/boot/dts/qcom/sm6115.dtsi | 2 ++ > >>>>>> 1 file changed, 2 insertions(+) > >>>>>> > >>>>>> diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi > >>>>>> index 29b5b388cd94..bfaaa1801a4d 100644 > >>>>>> --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi > >>>>>> +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi > >>>>>> @@ -1430,6 +1430,8 @@ gpucc: clock-controller@5990000 { > >>>>>> clocks = <&rpmcc RPM_SMD_XO_CLK_SRC>, > >>>>>> <&gcc GCC_GPU_GPLL0_CLK_SRC>, > >>>>>> <&gcc GCC_GPU_GPLL0_DIV_CLK_SRC>; > >>>>>> + power-domains = <&rpmpd SM6115_VDDCX>; > >>>>>> + required-opps = <&rpmpd_opp_low_svs>; > >>>>> > >>>>> Where is this required-opp coming from? The clocks in gpucc seem to have > >>>>> different voltage requirements depending on the rates, but we usually > >>>>> handle that in the OPP tables of the consumer. > >>>> The only lower levels defined for this SoC are VDD_MIN and VDD_RET, > >>>> but quite obviously the GPU won't work then > >>>> > >>> > >>> The levels needed for the GPU clocks to run should be in the GPU OPP > >>> table though, just like e.g. sdhc2_opp_table for the SDCC clocks. > >>> > >>> I still don't really understand why this is specified here. :) > >> The GPU_CC block needs this rail to be at a certain power level for > >> register access. This describes that requirement. > >> > > > > Can you show where this is defined downstream? On a quick look I didn't > > see something like that anywhere. Or is this from some secret > > documentation? > As far as downstream goes, you can notice that no branch's or RCG's > vdd tables ever define a level lower than the one I mentioned. > As far as I can tell the vdd tables are only used when the clock is actually enabled though, not for writing to registers while they are disabled. Stephan
On 18.07.2023 06:25, Bjorn Andersson wrote: > On Mon, Jul 17, 2023 at 07:11:33PM +0200, Konrad Dybcio wrote: >> On 17.07.2023 18:56, Stephan Gerhold wrote: >>> On Mon, Jul 17, 2023 at 06:50:18PM +0200, Konrad Dybcio wrote: >>>> On 17.07.2023 18:28, Stephan Gerhold wrote: >>>>> On Mon, Jul 17, 2023 at 05:19:22PM +0200, Konrad Dybcio wrote: >>>>>> The GPU_CC block is powered by VDD_CX. Describe that. >>>>>> >>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> >>>>>> --- >>>>>> arch/arm64/boot/dts/qcom/sm6115.dtsi | 2 ++ >>>>>> 1 file changed, 2 insertions(+) >>>>>> >>>>>> diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi >>>>>> index 29b5b388cd94..bfaaa1801a4d 100644 >>>>>> --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi >>>>>> +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi >>>>>> @@ -1430,6 +1430,8 @@ gpucc: clock-controller@5990000 { >>>>>> clocks = <&rpmcc RPM_SMD_XO_CLK_SRC>, >>>>>> <&gcc GCC_GPU_GPLL0_CLK_SRC>, >>>>>> <&gcc GCC_GPU_GPLL0_DIV_CLK_SRC>; >>>>>> + power-domains = <&rpmpd SM6115_VDDCX>; >>>>>> + required-opps = <&rpmpd_opp_low_svs>; >>>>> >>>>> Where is this required-opp coming from? The clocks in gpucc seem to have >>>>> different voltage requirements depending on the rates, but we usually >>>>> handle that in the OPP tables of the consumer. >>>> The only lower levels defined for this SoC are VDD_MIN and VDD_RET, >>>> but quite obviously the GPU won't work then >>>> >>> >>> The levels needed for the GPU clocks to run should be in the GPU OPP >>> table though, just like e.g. sdhc2_opp_table for the SDCC clocks. >>> >>> I still don't really understand why this is specified here. :) >> The GPU_CC block needs this rail to be at a certain power level for >> register access. This describes that requirement. >> > > And that is not the lowest level reported by command db? > Please describe this part in the commit message as well. command-what? ;) RPM exports VDD_NONE (off), VDD_MIN (the lowest state before collapse) and then low_svs is usually the lowest "actually on" state for all consumers. Konrad
On 18.07.2023 13:56, Stephan Gerhold wrote: > On Mon, Jul 17, 2023 at 09:18:21PM +0200, Konrad Dybcio wrote: >> On 17.07.2023 19:23, Stephan Gerhold wrote: >>> On Mon, Jul 17, 2023 at 07:11:33PM +0200, Konrad Dybcio wrote: >>>> On 17.07.2023 18:56, Stephan Gerhold wrote: >>>>> On Mon, Jul 17, 2023 at 06:50:18PM +0200, Konrad Dybcio wrote: >>>>>> On 17.07.2023 18:28, Stephan Gerhold wrote: >>>>>>> On Mon, Jul 17, 2023 at 05:19:22PM +0200, Konrad Dybcio wrote: >>>>>>>> The GPU_CC block is powered by VDD_CX. Describe that. >>>>>>>> >>>>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> >>>>>>>> --- >>>>>>>> arch/arm64/boot/dts/qcom/sm6115.dtsi | 2 ++ >>>>>>>> 1 file changed, 2 insertions(+) >>>>>>>> >>>>>>>> diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi >>>>>>>> index 29b5b388cd94..bfaaa1801a4d 100644 >>>>>>>> --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi >>>>>>>> +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi >>>>>>>> @@ -1430,6 +1430,8 @@ gpucc: clock-controller@5990000 { >>>>>>>> clocks = <&rpmcc RPM_SMD_XO_CLK_SRC>, >>>>>>>> <&gcc GCC_GPU_GPLL0_CLK_SRC>, >>>>>>>> <&gcc GCC_GPU_GPLL0_DIV_CLK_SRC>; >>>>>>>> + power-domains = <&rpmpd SM6115_VDDCX>; >>>>>>>> + required-opps = <&rpmpd_opp_low_svs>; >>>>>>> >>>>>>> Where is this required-opp coming from? The clocks in gpucc seem to have >>>>>>> different voltage requirements depending on the rates, but we usually >>>>>>> handle that in the OPP tables of the consumer. >>>>>> The only lower levels defined for this SoC are VDD_MIN and VDD_RET, >>>>>> but quite obviously the GPU won't work then >>>>>> >>>>> >>>>> The levels needed for the GPU clocks to run should be in the GPU OPP >>>>> table though, just like e.g. sdhc2_opp_table for the SDCC clocks. >>>>> >>>>> I still don't really understand why this is specified here. :) >>>> The GPU_CC block needs this rail to be at a certain power level for >>>> register access. This describes that requirement. >>>> >>> >>> Can you show where this is defined downstream? On a quick look I didn't >>> see something like that anywhere. Or is this from some secret >>> documentation? >> As far as downstream goes, you can notice that no branch's or RCG's >> vdd tables ever define a level lower than the one I mentioned. >> > > As far as I can tell the vdd tables are only used when the clock is > actually enabled though, not for writing to registers while they are > disabled. Maybe, but you can also notice that even XO rates require at least SVS_LOW to function. Konrad
On Tue, 18 Jul 2023 at 15:48, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: > > On 18.07.2023 13:56, Stephan Gerhold wrote: > > On Mon, Jul 17, 2023 at 09:18:21PM +0200, Konrad Dybcio wrote: > >> On 17.07.2023 19:23, Stephan Gerhold wrote: > >>> On Mon, Jul 17, 2023 at 07:11:33PM +0200, Konrad Dybcio wrote: > >>>> On 17.07.2023 18:56, Stephan Gerhold wrote: > >>>>> On Mon, Jul 17, 2023 at 06:50:18PM +0200, Konrad Dybcio wrote: > >>>>>> On 17.07.2023 18:28, Stephan Gerhold wrote: > >>>>>>> On Mon, Jul 17, 2023 at 05:19:22PM +0200, Konrad Dybcio wrote: > >>>>>>>> The GPU_CC block is powered by VDD_CX. Describe that. > >>>>>>>> > >>>>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > >>>>>>>> --- > >>>>>>>> arch/arm64/boot/dts/qcom/sm6115.dtsi | 2 ++ > >>>>>>>> 1 file changed, 2 insertions(+) > >>>>>>>> > >>>>>>>> diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi > >>>>>>>> index 29b5b388cd94..bfaaa1801a4d 100644 > >>>>>>>> --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi > >>>>>>>> +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi > >>>>>>>> @@ -1430,6 +1430,8 @@ gpucc: clock-controller@5990000 { > >>>>>>>> clocks = <&rpmcc RPM_SMD_XO_CLK_SRC>, > >>>>>>>> <&gcc GCC_GPU_GPLL0_CLK_SRC>, > >>>>>>>> <&gcc GCC_GPU_GPLL0_DIV_CLK_SRC>; > >>>>>>>> + power-domains = <&rpmpd SM6115_VDDCX>; > >>>>>>>> + required-opps = <&rpmpd_opp_low_svs>; > >>>>>>> > >>>>>>> Where is this required-opp coming from? The clocks in gpucc seem to have > >>>>>>> different voltage requirements depending on the rates, but we usually > >>>>>>> handle that in the OPP tables of the consumer. > >>>>>> The only lower levels defined for this SoC are VDD_MIN and VDD_RET, > >>>>>> but quite obviously the GPU won't work then > >>>>>> > >>>>> > >>>>> The levels needed for the GPU clocks to run should be in the GPU OPP > >>>>> table though, just like e.g. sdhc2_opp_table for the SDCC clocks. > >>>>> > >>>>> I still don't really understand why this is specified here. :) > >>>> The GPU_CC block needs this rail to be at a certain power level for > >>>> register access. This describes that requirement. > >>>> > >>> > >>> Can you show where this is defined downstream? On a quick look I didn't > >>> see something like that anywhere. Or is this from some secret > >>> documentation? > >> As far as downstream goes, you can notice that no branch's or RCG's > >> vdd tables ever define a level lower than the one I mentioned. > >> > > > > As far as I can tell the vdd tables are only used when the clock is > > actually enabled though, not for writing to registers while they are > > disabled. > Maybe, but you can also notice that even XO rates require at least > SVS_LOW to function. But the vdd tables are related to clock rates, which, in the upstream design, should be voted by the consumers, not by the clock driver.
On Mon, Jul 17, 2023 at 05:19:08PM +0200, Konrad Dybcio wrote: > We harcode some clocks to be always-on, as they're essential to the typo: hardcode > functioning of the SoC / some peripherals. Add a helper to do so > to make the writes less magic. > > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> Looks good otherwise: Reviewed-by: Johan Hovold <johan+linaro@kernel.org>
On Mon, Jul 17, 2023 at 05:19:14PM +0200, Konrad Dybcio wrote: > The GPU_CC block on SM6115 is powered by the VDD_CX rail. We need to > ensure that it's enabled to prevent unwanted power collapse. This bit is not correct, the power domain would not have been disabled until you enable runtime PM as part of this very patch. I noticed similar claims have incorrectly been made in the past, for example, in the recent: 2a541abd9837 clk: qcom: gcc-sc8280xp: Add runtime PM and older: a91c483b42fa ("clk: qcom: videocc-sm8250: use runtime PM for the clock controller") Johan
On 18.07.2023 15:24, Johan Hovold wrote: > On Mon, Jul 17, 2023 at 05:19:14PM +0200, Konrad Dybcio wrote: >> The GPU_CC block on SM6115 is powered by the VDD_CX rail. We need to >> ensure that it's enabled to prevent unwanted power collapse. > > This bit is not correct, the power domain would not have been disabled > until you enable runtime PM as part of this very patch. Right, this was a bit of a thought-jump. The part that ensures there's any vote at all is actually the DT commit adding a reference to the genpd. Konrad
On 18.07.2023 15:08, Dmitry Baryshkov wrote: > On Tue, 18 Jul 2023 at 15:48, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: >> >> On 18.07.2023 13:56, Stephan Gerhold wrote: >>> On Mon, Jul 17, 2023 at 09:18:21PM +0200, Konrad Dybcio wrote: >>>> On 17.07.2023 19:23, Stephan Gerhold wrote: >>>>> On Mon, Jul 17, 2023 at 07:11:33PM +0200, Konrad Dybcio wrote: >>>>>> On 17.07.2023 18:56, Stephan Gerhold wrote: >>>>>>> On Mon, Jul 17, 2023 at 06:50:18PM +0200, Konrad Dybcio wrote: >>>>>>>> On 17.07.2023 18:28, Stephan Gerhold wrote: >>>>>>>>> On Mon, Jul 17, 2023 at 05:19:22PM +0200, Konrad Dybcio wrote: >>>>>>>>>> The GPU_CC block is powered by VDD_CX. Describe that. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> >>>>>>>>>> --- >>>>>>>>>> arch/arm64/boot/dts/qcom/sm6115.dtsi | 2 ++ >>>>>>>>>> 1 file changed, 2 insertions(+) >>>>>>>>>> >>>>>>>>>> diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi >>>>>>>>>> index 29b5b388cd94..bfaaa1801a4d 100644 >>>>>>>>>> --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi >>>>>>>>>> +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi >>>>>>>>>> @@ -1430,6 +1430,8 @@ gpucc: clock-controller@5990000 { >>>>>>>>>> clocks = <&rpmcc RPM_SMD_XO_CLK_SRC>, >>>>>>>>>> <&gcc GCC_GPU_GPLL0_CLK_SRC>, >>>>>>>>>> <&gcc GCC_GPU_GPLL0_DIV_CLK_SRC>; >>>>>>>>>> + power-domains = <&rpmpd SM6115_VDDCX>; >>>>>>>>>> + required-opps = <&rpmpd_opp_low_svs>; >>>>>>>>> >>>>>>>>> Where is this required-opp coming from? The clocks in gpucc seem to have >>>>>>>>> different voltage requirements depending on the rates, but we usually >>>>>>>>> handle that in the OPP tables of the consumer. >>>>>>>> The only lower levels defined for this SoC are VDD_MIN and VDD_RET, >>>>>>>> but quite obviously the GPU won't work then >>>>>>>> >>>>>>> >>>>>>> The levels needed for the GPU clocks to run should be in the GPU OPP >>>>>>> table though, just like e.g. sdhc2_opp_table for the SDCC clocks. >>>>>>> >>>>>>> I still don't really understand why this is specified here. :) >>>>>> The GPU_CC block needs this rail to be at a certain power level for >>>>>> register access. This describes that requirement. >>>>>> >>>>> >>>>> Can you show where this is defined downstream? On a quick look I didn't >>>>> see something like that anywhere. Or is this from some secret >>>>> documentation? >>>> As far as downstream goes, you can notice that no branch's or RCG's >>>> vdd tables ever define a level lower than the one I mentioned. >>>> >>> >>> As far as I can tell the vdd tables are only used when the clock is >>> actually enabled though, not for writing to registers while they are >>> disabled. >> Maybe, but you can also notice that even XO rates require at least >> SVS_LOW to function. > > But the vdd tables are related to clock rates, which, in the upstream > design, should be voted by the consumers, not by the clock driver. Not all of the clocks are associated with OPP tables upstream, and it would be nice if the clock controller block had power flowing to it in case one wanted to access a different clock. Konrad
On Tue, Jul 18, 2023 at 02:21:31PM +0200, Konrad Dybcio wrote: > On 18.07.2023 06:25, Bjorn Andersson wrote: > > On Mon, Jul 17, 2023 at 07:11:33PM +0200, Konrad Dybcio wrote: > >> On 17.07.2023 18:56, Stephan Gerhold wrote: > >>> On Mon, Jul 17, 2023 at 06:50:18PM +0200, Konrad Dybcio wrote: > >>>> On 17.07.2023 18:28, Stephan Gerhold wrote: > >>>>> On Mon, Jul 17, 2023 at 05:19:22PM +0200, Konrad Dybcio wrote: > >>>>>> The GPU_CC block is powered by VDD_CX. Describe that. > >>>>>> > >>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > >>>>>> --- > >>>>>> arch/arm64/boot/dts/qcom/sm6115.dtsi | 2 ++ > >>>>>> 1 file changed, 2 insertions(+) > >>>>>> > >>>>>> diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi > >>>>>> index 29b5b388cd94..bfaaa1801a4d 100644 > >>>>>> --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi > >>>>>> +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi > >>>>>> @@ -1430,6 +1430,8 @@ gpucc: clock-controller@5990000 { > >>>>>> clocks = <&rpmcc RPM_SMD_XO_CLK_SRC>, > >>>>>> <&gcc GCC_GPU_GPLL0_CLK_SRC>, > >>>>>> <&gcc GCC_GPU_GPLL0_DIV_CLK_SRC>; > >>>>>> + power-domains = <&rpmpd SM6115_VDDCX>; > >>>>>> + required-opps = <&rpmpd_opp_low_svs>; > >>>>> > >>>>> Where is this required-opp coming from? The clocks in gpucc seem to have > >>>>> different voltage requirements depending on the rates, but we usually > >>>>> handle that in the OPP tables of the consumer. > >>>> The only lower levels defined for this SoC are VDD_MIN and VDD_RET, > >>>> but quite obviously the GPU won't work then > >>>> > >>> > >>> The levels needed for the GPU clocks to run should be in the GPU OPP > >>> table though, just like e.g. sdhc2_opp_table for the SDCC clocks. > >>> > >>> I still don't really understand why this is specified here. :) > >> The GPU_CC block needs this rail to be at a certain power level for > >> register access. This describes that requirement. > >> > > > > And that is not the lowest level reported by command db? > > Please describe this part in the commit message as well. > command-what? ;) > Apparently doesn't matter that I read that line multiple times, my brain really wanted a 'h' in there. > RPM exports VDD_NONE (off), VDD_MIN (the lowest state before collapse) > and then low_svs is usually the lowest "actually on" state for all > consumers. > In rpmhpd I changed it such that the minimal enabled state would be !disabled (so that the automatic enablement during probe would be sufficient to access registers), but talking to Ulf this is provider-specific. So unless you can figure out a acceptable lowest non-disabled state this is what has to be done... PS. My ask for mentioning this in the commit message still stands. Regards, Bjorn
On 18.07.2023 15:28, Konrad Dybcio wrote: > On 18.07.2023 15:24, Johan Hovold wrote: >> On Mon, Jul 17, 2023 at 05:19:14PM +0200, Konrad Dybcio wrote: >>> The GPU_CC block on SM6115 is powered by the VDD_CX rail. We need to >>> ensure that it's enabled to prevent unwanted power collapse. >> >> This bit is not correct, the power domain would not have been disabled >> until you enable runtime PM as part of this very patch. > Right, this was a bit of a thought-jump. The part that ensures there's > any vote at all is actually the DT commit adding a reference to the > genpd. Well I read it again and I think my original intention was "it = the power domain" and not "it = runtime PM", it makes sense that way.. Anyway, I'll rephrase that to make it less ambiguous. Konrad
On Qualcomm SoCs, certain branch clocks either need to be always-on, or should be if you're interested in touching some part of the hardware. Using CLK_IS_CRITICAL for this purpose sounds like a genius idea, however that messes with the runtime pm handling - if a clock is marked as such, the clock controller device will never enter the "suspended" state, leaving the associated resources online, which in turn breaks SoC-wide suspend. This series aims to solve that on a couple SoCs that I could test the changes on and it sprinkles some runtime pm enablement atop these drivers. Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> --- Konrad Dybcio (15): clk: qcom: branch: Add a helper for setting the enable bit clk: qcom: Use qcom_branch_set_clk_en() clk: qcom: gcc-sm6375: Unregister critical clocks clk: qcom: gcc-sm6375: Add runtime PM clk: qcom: gpucc-sm6375: Unregister critical clocks clk: qcom: gpucc-sm6115: Unregister critical clocks clk: qcom: gpucc-sm6115: Add runtime PM clk: qcom: gcc-sm6115: Unregister critical clocks clk: qcom: gcc-sm6115: Add runtime PM clk: qcom: gcc-qcm2290: Unregister critical clocks clk: qcom: gcc-qcm2290: Add runtime PM arm64: dts: qcom: sm6375: Add VDD_CX to GCC arm64: dts: qcom: qcm2290: Add VDD_CX to GCC arm64: dts: qcom: sm6115: Add VDD_CX to GCC arm64: dts: qcom: sm6115: Add VDD_CX to GPU_CCC arch/arm64/boot/dts/qcom/qcm2290.dtsi | 1 + arch/arm64/boot/dts/qcom/sm6115.dtsi | 3 + arch/arm64/boot/dts/qcom/sm6375.dtsi | 1 + drivers/clk/qcom/clk-branch.h | 7 ++ drivers/clk/qcom/dispcc-qcm2290.c | 2 +- drivers/clk/qcom/dispcc-sc7280.c | 2 +- drivers/clk/qcom/dispcc-sc8280xp.c | 2 +- drivers/clk/qcom/dispcc-sm6115.c | 2 +- drivers/clk/qcom/dispcc-sm8250.c | 2 +- drivers/clk/qcom/dispcc-sm8450.c | 2 +- drivers/clk/qcom/dispcc-sm8550.c | 2 +- drivers/clk/qcom/gcc-qcm2290.c | 136 ++++++++--------------------- drivers/clk/qcom/gcc-sa8775p.c | 18 ++-- drivers/clk/qcom/gcc-sc7180.c | 16 ++-- drivers/clk/qcom/gcc-sc7280.c | 14 +-- drivers/clk/qcom/gcc-sc8180x.c | 20 ++--- drivers/clk/qcom/gcc-sc8280xp.c | 18 ++-- drivers/clk/qcom/gcc-sdx55.c | 2 +- drivers/clk/qcom/gcc-sdx65.c | 2 +- drivers/clk/qcom/gcc-sdx75.c | 4 +- drivers/clk/qcom/gcc-sm6115.c | 155 ++++++++-------------------------- drivers/clk/qcom/gcc-sm6375.c | 34 +++++--- drivers/clk/qcom/gcc-sm7150.c | 16 ++-- drivers/clk/qcom/gcc-sm8250.c | 12 +-- drivers/clk/qcom/gcc-sm8350.c | 14 +-- drivers/clk/qcom/gcc-sm8450.c | 14 +-- drivers/clk/qcom/gcc-sm8550.c | 14 +-- drivers/clk/qcom/gpucc-sc7280.c | 4 +- drivers/clk/qcom/gpucc-sc8280xp.c | 4 +- drivers/clk/qcom/gpucc-sm6115.c | 57 ++++++------- drivers/clk/qcom/gpucc-sm6375.c | 38 ++------- drivers/clk/qcom/gpucc-sm8550.c | 4 +- drivers/clk/qcom/lpasscorecc-sc7180.c | 2 +- drivers/clk/qcom/videocc-sm8250.c | 4 +- drivers/clk/qcom/videocc-sm8350.c | 4 +- drivers/clk/qcom/videocc-sm8450.c | 6 +- drivers/clk/qcom/videocc-sm8550.c | 6 +- 37 files changed, 245 insertions(+), 399 deletions(-) --- base-commit: 2205be537aeb1ca2ace998e2fefaa2be04e393e4 change-id: 20230717-topic-branch_aon_cleanup-6976c13fe71c Best regards,