diff mbox series

[15/15] arm64: dts: qcom: sm6115: Add VDD_CX to GPU_CCC

Message ID	20230717-topic-branch_aon_cleanup-v1-15-27784d27a4f4@linaro.org
State	New
Headers	Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; From: Konrad Dybcio <konrad.dybcio@linaro.org> Date: Mon, 17 Jul 2023 17:19:22 +0200 Subject: [PATCH 15/15] arm64: dts: qcom: sm6115: Add VDD_CX to GPU_CCC MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <20230717-topic-branch_aon_cleanup-v1-15-27784d27a4f4@linaro.org> References: <20230717-topic-branch_aon_cleanup-v1-0-27784d27a4f4@linaro.org> In-Reply-To: <20230717-topic-branch_aon_cleanup-v1-0-27784d27a4f4@linaro.org> To: Bjorn Andersson <andersson@kernel.org>, Andy Gross <agross@kernel.org>, Michael Turquette <mturquette@baylibre.com>, Stephen Boyd <sboyd@kernel.org>, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org> Cc: Marijn Suijten <marijn.suijten@somainline.org>, linux-arm-msm@vger.kernel.org, linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, Konrad Dybcio <konrad.dybcio@linaro.org> Precedence: bulk
Series	Unregister critical branch clocks + some RPM \| [00/15] Unregister critical branch clocks + some RPM [01/15] clk: qcom: branch: Add a helper for setting the enable bit [02/15] clk: qcom: Use qcom_branch_set_clk_en() [03/15] clk: qcom: gcc-sm6375: Unregister critical clocks [04/15] clk: qcom: gcc-sm6375: Add runtime PM [05/15] clk: qcom: gpucc-sm6375: Unregister critical clocks [06/15] clk: qcom: gpucc-sm6115: Unregister critical clocks [07/15] clk: qcom: gpucc-sm6115: Add runtime PM [08/15] clk: qcom: gcc-sm6115: Unregister critical clocks [09/15] clk: qcom: gcc-sm6115: Add runtime PM [10/15] clk: qcom: gcc-qcm2290: Unregister critical clocks [11/15] clk: qcom: gcc-qcm2290: Add runtime PM [12/15] arm64: dts: qcom: sm6375: Add VDD_CX to GCC [13/15] arm64: dts: qcom: qcm2290: Add VDD_CX to GCC [14/15] arm64: dts: qcom: sm6115: Add VDD_CX to GCC [15/15] arm64: dts: qcom: sm6115: Add VDD_CX to GPU_CCC

Commit Message

Konrad Dybcio July 17, 2023, 3:19 p.m. UTC

  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(+)

Comments

Stephan Gerhold July 17, 2023, 4:28 p.m. UTC | #1

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.

Thanks,
Stephan

Konrad Dybcio July 17, 2023, 4:50 p.m. UTC | #2

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

Stephan Gerhold July 17, 2023, 4:56 p.m. UTC | #3

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

Konrad Dybcio July 17, 2023, 5:11 p.m. UTC | #4

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.

Konrad

Stephan Gerhold July 17, 2023, 5:23 p.m. UTC | #5

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?

Thanks,
Stephan

Konrad Dybcio July 17, 2023, 7:18 p.m. UTC | #6

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

Bjorn Andersson July 18, 2023, 4:25 a.m. UTC | #7

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

Stephan Gerhold July 18, 2023, 11:56 a.m. UTC | #8

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

Konrad Dybcio July 18, 2023, 12:21 p.m. UTC | #9

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

Konrad Dybcio July 18, 2023, 12:47 p.m. UTC | #10

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

Dmitry Baryshkov July 18, 2023, 1:08 p.m. UTC | #11

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.

Konrad Dybcio July 18, 2023, 1:36 p.m. UTC | #12

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

Bjorn Andersson July 18, 2023, 3:06 p.m. UTC | #13

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

diff mbox series

Patch

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>;
 			#clock-cells = <1>;
 			#reset-cells = <1>;
 			#power-domain-cells = <1>;