diff mbox series

[v3,5/6] arm64: dts: qcom: sdm845: Add OPP table support to UFSHC

Message ID 20230731163357.49045-6-manivannan.sadhasivam@linaro.org (mailing list archive)
State Changes Requested
Headers show
Series UFS: Add OPP support | expand

Commit Message

Manivannan Sadhasivam July 31, 2023, 4:33 p.m. UTC 
From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

UFS host controller, when scaling gears, should choose appropriate
performance state of RPMh power domain controller along with clock
frequency. So let's add the OPP table support to specify both clock
frequency and RPMh performance states replacing the old "freq-table-hz"
property.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
[mani: Splitted pd change and used rpmhpd_opp_low_svs]
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 arch/arm64/boot/dts/qcom/sdm845.dtsi | 42 +++++++++++++++++++++-------
 1 file changed, 32 insertions(+), 10 deletions(-)

Comments

Dmitry Baryshkov Sept. 11, 2023, 1:15 p.m. UTC | #1 
On 31/07/2023 19:33, Manivannan Sadhasivam wrote:
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> 
> UFS host controller, when scaling gears, should choose appropriate
> performance state of RPMh power domain controller along with clock
> frequency. So let's add the OPP table support to specify both clock
> frequency and RPMh performance states replacing the old "freq-table-hz"
> property.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> [mani: Splitted pd change and used rpmhpd_opp_low_svs]
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>   arch/arm64/boot/dts/qcom/sdm845.dtsi | 42 +++++++++++++++++++++-------
>   1 file changed, 32 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index 055ca80c0075..2ea6eb44953e 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -2605,22 +2605,44 @@ ufs_mem_hc: ufshc@1d84000 {
>   				<&gcc GCC_UFS_PHY_RX_SYMBOL_0_CLK>,
>   				<&gcc GCC_UFS_PHY_RX_SYMBOL_1_CLK>,
>   				<&gcc GCC_UFS_PHY_ICE_CORE_CLK>;
> -			freq-table-hz =
> -				<50000000 200000000>,
> -				<0 0>,
> -				<0 0>,
> -				<37500000 150000000>,
> -				<0 0>,
> -				<0 0>,
> -				<0 0>,
> -				<0 0>,
> -				<75000000 300000000>;
> +
> +			operating-points-v2 = <&ufs_opp_table>;
>   
>   			interconnects = <&aggre1_noc MASTER_UFS_MEM 0 &mem_noc SLAVE_EBI1 0>,
>   					<&gladiator_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_UFS_MEM_CFG 0>;
>   			interconnect-names = "ufs-ddr", "cpu-ufs";
>   
>   			status = "disabled";
> +
> +			ufs_opp_table: opp-table {
> +				compatible = "operating-points-v2";
> +
> +				opp-50000000 {
> +					opp-hz = /bits/ 64 <50000000>,
> +						 /bits/ 64 <0>,
> +						 /bits/ 64 <0>,
> +						 /bits/ 64 <37500000>,
> +						 /bits/ 64 <0>,
> +						 /bits/ 64 <0>,
> +						 /bits/ 64 <0>,
> +						 /bits/ 64 <0>,
> +						 /bits/ 64 <75000000>;
> +					required-opps = <&rpmhpd_opp_low_svs>;
> +				};

I'd say, I'm still slightly unhappy about the 0 clock rates here.
We need only three clocks here: core, core_clk_unipro and optional 
ice_core_clk. Can we modify ufshcd_parse_operating_points() to pass only 
these two or three clock names to devm_pm_opp_set_config() ? The OPP 
core doesn't need to know about all the rest of the clocks.

> +
> +				opp-200000000 {
> +					opp-hz = /bits/ 64 <200000000>,
> +						 /bits/ 64 <0>,
> +						 /bits/ 64 <0>,
> +						 /bits/ 64 <150000000>,
> +						 /bits/ 64 <0>,
> +						 /bits/ 64 <0>,
> +						 /bits/ 64 <0>,
> +						 /bits/ 64 <0>,
> +						 /bits/ 64 <300000000>;
> +					required-opps = <&rpmhpd_opp_nom>;
> +				};
> +			};
>   		};
>   
>   		ufs_mem_phy: phy@1d87000 {
Manivannan Sadhasivam Sept. 12, 2023, 6:59 a.m. UTC | #2 
On Mon, Sep 11, 2023 at 04:15:10PM +0300, Dmitry Baryshkov wrote:
> On 31/07/2023 19:33, Manivannan Sadhasivam wrote:
> > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > 
> > UFS host controller, when scaling gears, should choose appropriate
> > performance state of RPMh power domain controller along with clock
> > frequency. So let's add the OPP table support to specify both clock
> > frequency and RPMh performance states replacing the old "freq-table-hz"
> > property.
> > 
> > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > [mani: Splitted pd change and used rpmhpd_opp_low_svs]
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >   arch/arm64/boot/dts/qcom/sdm845.dtsi | 42 +++++++++++++++++++++-------
> >   1 file changed, 32 insertions(+), 10 deletions(-)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > index 055ca80c0075..2ea6eb44953e 100644
> > --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > @@ -2605,22 +2605,44 @@ ufs_mem_hc: ufshc@1d84000 {
> >   				<&gcc GCC_UFS_PHY_RX_SYMBOL_0_CLK>,
> >   				<&gcc GCC_UFS_PHY_RX_SYMBOL_1_CLK>,
> >   				<&gcc GCC_UFS_PHY_ICE_CORE_CLK>;
> > -			freq-table-hz =
> > -				<50000000 200000000>,
> > -				<0 0>,
> > -				<0 0>,
> > -				<37500000 150000000>,
> > -				<0 0>,
> > -				<0 0>,
> > -				<0 0>,
> > -				<0 0>,
> > -				<75000000 300000000>;
> > +
> > +			operating-points-v2 = <&ufs_opp_table>;
> >   			interconnects = <&aggre1_noc MASTER_UFS_MEM 0 &mem_noc SLAVE_EBI1 0>,
> >   					<&gladiator_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_UFS_MEM_CFG 0>;
> >   			interconnect-names = "ufs-ddr", "cpu-ufs";
> >   			status = "disabled";
> > +
> > +			ufs_opp_table: opp-table {
> > +				compatible = "operating-points-v2";
> > +
> > +				opp-50000000 {
> > +					opp-hz = /bits/ 64 <50000000>,
> > +						 /bits/ 64 <0>,
> > +						 /bits/ 64 <0>,
> > +						 /bits/ 64 <37500000>,
> > +						 /bits/ 64 <0>,
> > +						 /bits/ 64 <0>,
> > +						 /bits/ 64 <0>,
> > +						 /bits/ 64 <0>,
> > +						 /bits/ 64 <75000000>;
> > +					required-opps = <&rpmhpd_opp_low_svs>;
> > +				};
> 
> I'd say, I'm still slightly unhappy about the 0 clock rates here.

Neither do I. But it is the only viable option I could found.

> We need only three clocks here: core, core_clk_unipro and optional
> ice_core_clk. Can we modify ufshcd_parse_operating_points() to pass only
> these two or three clock names to devm_pm_opp_set_config() ? The OPP core
> doesn't need to know about all the rest of the clocks.
> 

We need to enable/disable all of the clocks, but only need to control the rate
for these 3 clocks. So we cannot just use 3 clocks.

If the OPP table has only 3 entries (omitting the gate-only clocks), then we
need some hack in the driver to match the rates against the clock entries. Doing
so will result in hardcoding the clock info in the driver which I do not want to
do.

If we have something like "opp-hz-names" to relate the rates to clock-names, it
might do the job. But it needs some input from Viresh.

- Mani

> > +
> > +				opp-200000000 {
> > +					opp-hz = /bits/ 64 <200000000>,
> > +						 /bits/ 64 <0>,
> > +						 /bits/ 64 <0>,
> > +						 /bits/ 64 <150000000>,
> > +						 /bits/ 64 <0>,
> > +						 /bits/ 64 <0>,
> > +						 /bits/ 64 <0>,
> > +						 /bits/ 64 <0>,
> > +						 /bits/ 64 <300000000>;
> > +					required-opps = <&rpmhpd_opp_nom>;
> > +				};
> > +			};
> >   		};
> >   		ufs_mem_phy: phy@1d87000 {
> 
> -- 
> With best wishes
> Dmitry
>
Viresh Kumar Sept. 27, 2023, 11:15 a.m. UTC | #3 
On 12-09-23, 12:29, Manivannan Sadhasivam wrote:
> On Mon, Sep 11, 2023 at 04:15:10PM +0300, Dmitry Baryshkov wrote:
> > I'd say, I'm still slightly unhappy about the 0 clock rates here.
> 
> Neither do I. But it is the only viable option I could found.
> 
> > We need only three clocks here: core, core_clk_unipro and optional
> > ice_core_clk. Can we modify ufshcd_parse_operating_points() to pass only
> > these two or three clock names to devm_pm_opp_set_config() ? The OPP core
> > doesn't need to know about all the rest of the clocks.
> > 
> 
> We need to enable/disable all of the clocks, but only need to control the rate
> for these 3 clocks. So we cannot just use 3 clocks.
> 
> If the OPP table has only 3 entries (omitting the gate-only clocks), then we
> need some hack in the driver to match the rates against the clock entries. Doing
> so will result in hardcoding the clock info in the driver which I do not want to
> do.
> 
> If we have something like "opp-hz-names" to relate the rates to clock-names, it
> might do the job. But it needs some input from Viresh.

I have already given an option earlier about this [1]. You can change the order
of clks in the "clock-names" field, so that the first three are the one with
valid frequencies. You shouldn't need much of the hacks after that I guess.

Or maybe I missed something else now, talked about this a long time ago :)
Manivannan Sadhasivam Sept. 27, 2023, 12:39 p.m. UTC | #4 
On Wed, Sep 27, 2023 at 04:45:46PM +0530, Viresh Kumar wrote:
> On 12-09-23, 12:29, Manivannan Sadhasivam wrote:
> > On Mon, Sep 11, 2023 at 04:15:10PM +0300, Dmitry Baryshkov wrote:
> > > I'd say, I'm still slightly unhappy about the 0 clock rates here.
> > 
> > Neither do I. But it is the only viable option I could found.
> > 
> > > We need only three clocks here: core, core_clk_unipro and optional
> > > ice_core_clk. Can we modify ufshcd_parse_operating_points() to pass only
> > > these two or three clock names to devm_pm_opp_set_config() ? The OPP core
> > > doesn't need to know about all the rest of the clocks.
> > > 
> > 
> > We need to enable/disable all of the clocks, but only need to control the rate
> > for these 3 clocks. So we cannot just use 3 clocks.
> > 
> > If the OPP table has only 3 entries (omitting the gate-only clocks), then we
> > need some hack in the driver to match the rates against the clock entries. Doing
> > so will result in hardcoding the clock info in the driver which I do not want to
> > do.
> > 
> > If we have something like "opp-hz-names" to relate the rates to clock-names, it
> > might do the job. But it needs some input from Viresh.
> 
> I have already given an option earlier about this [1]. You can change the order
> of clks in the "clock-names" field, so that the first three are the one with
> valid frequencies. You shouldn't need much of the hacks after that I guess.
> 

I do not remember all the details on the top of my head, but I did investigate on
that and concluded that it won't fit.

- Mani

> Or maybe I missed something else now, talked about this a long time ago :)
> 
> -- 
> viresh
> [1] https://lore.kernel.org/all/20230713040918.jnf5oqiwymrdnrmq@vireshk-i7/
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index 055ca80c0075..2ea6eb44953e 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -2605,22 +2605,44 @@  ufs_mem_hc: ufshc@1d84000 {
 				<&gcc GCC_UFS_PHY_RX_SYMBOL_0_CLK>,
 				<&gcc GCC_UFS_PHY_RX_SYMBOL_1_CLK>,
 				<&gcc GCC_UFS_PHY_ICE_CORE_CLK>;
-			freq-table-hz =
-				<50000000 200000000>,
-				<0 0>,
-				<0 0>,
-				<37500000 150000000>,
-				<0 0>,
-				<0 0>,
-				<0 0>,
-				<0 0>,
-				<75000000 300000000>;
+
+			operating-points-v2 = <&ufs_opp_table>;
 
 			interconnects = <&aggre1_noc MASTER_UFS_MEM 0 &mem_noc SLAVE_EBI1 0>,
 					<&gladiator_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_UFS_MEM_CFG 0>;
 			interconnect-names = "ufs-ddr", "cpu-ufs";
 
 			status = "disabled";
+
+			ufs_opp_table: opp-table {
+				compatible = "operating-points-v2";
+
+				opp-50000000 {
+					opp-hz = /bits/ 64 <50000000>,
+						 /bits/ 64 <0>,
+						 /bits/ 64 <0>,
+						 /bits/ 64 <37500000>,
+						 /bits/ 64 <0>,
+						 /bits/ 64 <0>,
+						 /bits/ 64 <0>,
+						 /bits/ 64 <0>,
+						 /bits/ 64 <75000000>;
+					required-opps = <&rpmhpd_opp_low_svs>;
+				};
+
+				opp-200000000 {
+					opp-hz = /bits/ 64 <200000000>,
+						 /bits/ 64 <0>,
+						 /bits/ 64 <0>,
+						 /bits/ 64 <150000000>,
+						 /bits/ 64 <0>,
+						 /bits/ 64 <0>,
+						 /bits/ 64 <0>,
+						 /bits/ 64 <0>,
+						 /bits/ 64 <300000000>;
+					required-opps = <&rpmhpd_opp_nom>;
+				};
+			};
 		};
 
 		ufs_mem_phy: phy@1d87000 {