[PATCH 00/14] UFS: Add OPP and interconnect support

[PATCH 00/14] UFS: Add OPP and interconnect support

[Manivannan Sadhasivam]

Hi,

This series adds OPP (Operating Points) support to UFSHCD driver and
interconnect support to Qcom UFS driver.

Motivation behind adding OPP support is to scale both clocks as well as
regulators/performance state dynamically. Currently, UFSHCD just scales
clock frequency during runtime with the help of "freq-table-hz" property
defined in devicetree. With the addition of OPP tables in devicetree (as
done for Qcom SDM845 and SM8250 SoCs in this series) UFSHCD can now scale
both clocks and performance state of power domain which helps in power
saving.

For the addition of OPP support to UFSHCD, there are changes required to
the OPP framework and devfreq drivers which are also added in this series.

Finally, interconnect support is added to Qcom UFS driver for scaling the
interconnect path dynamically. This is required to avoid boot crash in
recent SoCs and also to save power during runtime. More information is
available in patch 13/13.

Credits
=======

This series is a continuation of previous work by Krzysztof Kozlowski [1]
and Brian Masney [2]. Ideally, this could've split into two series (OPP
and interconnect) but since there will be a dependency in the devicetree,
I decided to keep them in a single series.

Testing
=======

This series is tested on 96Boards RB3 (SDM845 SoC) and RB5 (SM8250 SoC)
development boards.

Merging Strategy
================

An immutable branch might be required between OPP and SCSI trees because of
the API dependency (devfreq too). And I leave it up to the maintainers to
decide.

Thanks,
Mani

[1] https://lore.kernel.org/all/20220513061347.46480-1-krzysztof.kozlowski@linaro.org/
[2] https://lore.kernel.org/all/20221117104957.254648-1-bmasney@redhat.com/

Krzysztof Kozlowski (2):
  dt-bindings: ufs: common: add OPP table
  arm64: dts: qcom: sdm845: Add OPP table support to UFSHC

Manivannan Sadhasivam (12):
  dt-bindings: opp: Increase maxItems for opp-hz property
  arm64: dts: qcom: sdm845: Add missing RPMh power domain to GCC
  arm64: dts: qcom: sdm845: Fix the min frequency of "ice_core_clk"
  arm64: dts: qcom: sm8250: Add OPP table support to UFSHC
  OPP: Introduce dev_pm_opp_find_freq_{ceil/floor}_indexed() APIs
  OPP: Introduce dev_pm_opp_get_freq_indexed() API
  PM / devfreq: Switch to dev_pm_opp_find_freq_{ceil/floor}_indexed()
    APIs
  scsi: ufs: core: Add OPP support for scaling clocks and regulators
  scsi: ufs: host: Add support for parsing OPP
  arm64: dts: qcom: sdm845: Add interconnect paths to UFSHC
  arm64: dts: qcom: sm8250: Add interconnect paths to UFSHC
  scsi: ufs: qcom: Add support for scaling interconnects

 .../devicetree/bindings/opp/opp-v2-base.yaml  |   2 +-
 .../devicetree/bindings/ufs/ufs-common.yaml   |  34 ++++-
 arch/arm64/boot/dts/qcom/sdm845.dtsi          |  47 ++++--
 arch/arm64/boot/dts/qcom/sm8250.dtsi          |  43 ++++--
 drivers/devfreq/devfreq.c                     |  14 +-
 drivers/opp/core.c                            |  76 ++++++++++
 drivers/ufs/core/ufshcd.c                     | 142 ++++++++++++++----
 drivers/ufs/host/ufs-qcom.c                   | 131 +++++++++++++++-
 drivers/ufs/host/ufs-qcom.h                   |   3 +
 drivers/ufs/host/ufshcd-pltfrm.c              | 116 ++++++++++++++
 include/linux/pm_opp.h                        |  26 ++++
 include/ufs/ufshcd.h                          |   4 +
 12 files changed, 574 insertions(+), 64 deletions(-)

-- 
2.25.1

[PATCH 01/14] dt-bindings: ufs: common: add OPP table

[Manivannan Sadhasivam]

From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Except scaling UFS and bus clocks, it's necessary to scale also the
voltages of regulators or power domain performance state levels.  Adding
Operating Performance Points table allows to adjust power domain
performance state, depending on the UFS clock speed.

OPPv2 deprecates previous property limited to clock scaling:
freq-table-hz.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 .../devicetree/bindings/ufs/ufs-common.yaml   | 34 +++++++++++++++++--
 1 file changed, 31 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/ufs/ufs-common.yaml b/Documentation/devicetree/bindings/ufs/ufs-common.yaml
index 47a4e9e1a775..d7d2c8a136bb 100644
--- a/Documentation/devicetree/bindings/ufs/ufs-common.yaml
+++ b/Documentation/devicetree/bindings/ufs/ufs-common.yaml
@@ -20,11 +20,24 @@ properties:
       items:
         - description: Minimum frequency for given clock in Hz
         - description: Maximum frequency for given clock in Hz
+    deprecated: true
     description: |
+      Preferred is operating-points-v2.
+
       Array of <min max> operating frequencies in Hz stored in the same order
-      as the clocks property. If this property is not defined or a value in the
-      array is "0" then it is assumed that the frequency is set by the parent
-      clock or a fixed rate clock source.
+      as the clocks property. If either this property or operating-points-v2 is
+      not defined or a value in the array is "0" then it is assumed that the
+      frequency is set by the parent clock or a fixed rate clock source.
+
+  operating-points-v2:
+    description:
+      Preferred over freq-table-hz.
+      If present, each OPP must contain array of frequencies stored in the same
+      order for each clock.  If clock frequency in the array is "0" then it is
+      assumed that the frequency is set by the parent clock or a fixed rate
+      clock source.
+
+  opp-table: true
 
   interrupts:
     maxItems: 1
@@ -75,8 +88,23 @@ properties:
 
 dependencies:
   freq-table-hz: [ 'clocks' ]
+  operating-points-v2: [ 'clocks', 'clock-names' ]
 
 required:
   - interrupts
 
+allOf:
+  - if:
+      required:
+        - freq-table-hz
+    then:
+      properties:
+        operating-points-v2: false
+  - if:
+      required:
+        - operating-points-v2
+    then:
+      properties:
+        freq-table-hz: false
+
 additionalProperties: true
-- 
2.25.1

[PATCH 02/14] dt-bindings: opp: Increase maxItems for opp-hz property

[Manivannan Sadhasivam]

Current limit of 16 will be exhausted by platforms specifying the frequency
for 9 clocks using opp-hz, like Qcom SDM845 SoC: 9 * 2 (64 bits) = 18

So let's increase the limit to 32 which should be enough for most platforms
(hopefully).

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 Documentation/devicetree/bindings/opp/opp-v2-base.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml
index 47e6f36b7637..e2f8f7af3cf4 100644
--- a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml
+++ b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml
@@ -56,7 +56,7 @@ patternProperties:
           need to be configured and that is left for the implementation
           specific binding.
         minItems: 1
-        maxItems: 16
+        maxItems: 32
         items:
           maxItems: 1
 
-- 
2.25.1

Re: [PATCH 02/14] dt-bindings: opp: Increase maxItems for opp-hz property

[Viresh Kumar]

On 12-07-23, 16:01, Manivannan Sadhasivam wrote:
> Current limit of 16 will be exhausted by platforms specifying the frequency
> for 9 clocks using opp-hz, like Qcom SDM845 SoC: 9 * 2 (64 bits) = 18

You missed mentioning why you are multiplying by 2 here (I suppose one
place for /bits/ 64 and one for <freq>.

Also full stop (.) is missing at the end.

> So let's increase the limit to 32 which should be enough for most platforms
> (hopefully).
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  Documentation/devicetree/bindings/opp/opp-v2-base.yaml | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml
> index 47e6f36b7637..e2f8f7af3cf4 100644
> --- a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml
> +++ b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml
> @@ -56,7 +56,7 @@ patternProperties:
>            need to be configured and that is left for the implementation
>            specific binding.
>          minItems: 1
> -        maxItems: 16
> +        maxItems: 32
>          items:
>            maxItems: 1
>  
> -- 
> 2.25.1

-- 
viresh

Re: [PATCH 02/14] dt-bindings: opp: Increase maxItems for opp-hz property

[Manivannan Sadhasivam]

On Wed, Jul 12, 2023 at 04:09:21PM +0530, Viresh Kumar wrote:
> On 12-07-23, 16:01, Manivannan Sadhasivam wrote:
> > Current limit of 16 will be exhausted by platforms specifying the frequency
> > for 9 clocks using opp-hz, like Qcom SDM845 SoC: 9 * 2 (64 bits) = 18
> 
> You missed mentioning why you are multiplying by 2 here (I suppose one
> place for /bits/ 64 and one for <freq>.
> 
> Also full stop (.) is missing at the end.
> 

Ok.

- Mani

> > So let's increase the limit to 32 which should be enough for most platforms
> > (hopefully).
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  Documentation/devicetree/bindings/opp/opp-v2-base.yaml | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml
> > index 47e6f36b7637..e2f8f7af3cf4 100644
> > --- a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml
> > +++ b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml
> > @@ -56,7 +56,7 @@ patternProperties:
> >            need to be configured and that is left for the implementation
> >            specific binding.
> >          minItems: 1
> > -        maxItems: 16
> > +        maxItems: 32
> >          items:
> >            maxItems: 1
> >  
> > -- 
> > 2.25.1
> 
> -- 
> viresh

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 02/14] dt-bindings: opp: Increase maxItems for opp-hz property

[Rob Herring]

On Wed, Jul 12, 2023 at 04:01:57PM +0530, Manivannan Sadhasivam wrote:
> Current limit of 16 will be exhausted by platforms specifying the frequency
> for 9 clocks using opp-hz, like Qcom SDM845 SoC: 9 * 2 (64 bits) = 18

Are you sure about that? opp-hz is defined as uint64-matrix, so 16 
64-bit entries should already be allowed. It doesn't matter how the .dts 
is encoded (it used to though).

> 
> So let's increase the limit to 32 which should be enough for most platforms
> (hopefully).
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  Documentation/devicetree/bindings/opp/opp-v2-base.yaml | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml
> index 47e6f36b7637..e2f8f7af3cf4 100644
> --- a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml
> +++ b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml
> @@ -56,7 +56,7 @@ patternProperties:
>            need to be configured and that is left for the implementation
>            specific binding.
>          minItems: 1
> -        maxItems: 16
> +        maxItems: 32
>          items:
>            maxItems: 1
>  
> -- 
> 2.25.1
> 

Re: [PATCH 02/14] dt-bindings: opp: Increase maxItems for opp-hz property

[Manivannan Sadhasivam]

On Fri, Jul 14, 2023 at 10:17:33AM -0600, Rob Herring wrote:
> On Wed, Jul 12, 2023 at 04:01:57PM +0530, Manivannan Sadhasivam wrote:
> > Current limit of 16 will be exhausted by platforms specifying the frequency
> > for 9 clocks using opp-hz, like Qcom SDM845 SoC: 9 * 2 (64 bits) = 18
> 
> Are you sure about that? opp-hz is defined as uint64-matrix, so 16 
> 64-bit entries should already be allowed. It doesn't matter how the .dts 
> is encoded (it used to though).
> 

Yes, 16 64bit entries are allowed, but SDM845 will add 18 entries (due to 9
clocks) and hence this patch.

Did I miss anything?

- Mani

> > 
> > So let's increase the limit to 32 which should be enough for most platforms
> > (hopefully).
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  Documentation/devicetree/bindings/opp/opp-v2-base.yaml | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml
> > index 47e6f36b7637..e2f8f7af3cf4 100644
> > --- a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml
> > +++ b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml
> > @@ -56,7 +56,7 @@ patternProperties:
> >            need to be configured and that is left for the implementation
> >            specific binding.
> >          minItems: 1
> > -        maxItems: 16
> > +        maxItems: 32
> >          items:
> >            maxItems: 1
> >  
> > -- 
> > 2.25.1
> > 

-- 
மணிவண்ணன் சதாசிவம்

[PATCH 03/14] arm64: dts: qcom: sdm845: Add missing RPMh power domain to GCC

[Manivannan Sadhasivam]

GCC and it's GDSCs are under the RPMh CX power domain. So let's add the
missing RPMh power domain to the GCC node.

Fixes: 6d4cf750d03a ("arm64: dts: sdm845: Add minimal dts/dtsi files for sdm845 SoC and MTP")
Co-developed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 arch/arm64/boot/dts/qcom/sdm845.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index 02a6ea0b8b2c..9ed74bf72d05 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -1207,6 +1207,7 @@ gcc: clock-controller@100000 {
 			#clock-cells = <1>;
 			#reset-cells = <1>;
 			#power-domain-cells = <1>;
+			power-domains = <&rpmhpd SDM845_CX>;
 		};
 
 		qfprom@784000 {
-- 
2.25.1

Re: [PATCH 03/14] arm64: dts: qcom: sdm845: Add missing RPMh power domain to GCC

[Konrad Dybcio]

On 12.07.2023 12:31, Manivannan Sadhasivam wrote:
> GCC and it's GDSCs are under the RPMh CX power domain. So let's add the
> missing RPMh power domain to the GCC node.
> 
> Fixes: 6d4cf750d03a ("arm64: dts: sdm845: Add minimal dts/dtsi files for sdm845 SoC and MTP")
> Co-developed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
The ratio of developers to lines of code changed is sus, but okay

Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad
>  arch/arm64/boot/dts/qcom/sdm845.dtsi | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index 02a6ea0b8b2c..9ed74bf72d05 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -1207,6 +1207,7 @@ gcc: clock-controller@100000 {
>  			#clock-cells = <1>;
>  			#reset-cells = <1>;
>  			#power-domain-cells = <1>;
> +			power-domains = <&rpmhpd SDM845_CX>;
>  		};
>  
>  		qfprom@784000 {

[PATCH 04/14] arm64: dts: qcom: sdm845: Fix the min frequency of "ice_core_clk"

[Manivannan Sadhasivam]

Minimum frequency of the "ice_core_clk" should be 75MHz as specified in the
downstream vendor devicetree. So fix it!

https://git.codelinaro.org/clo/la/kernel/msm-4.9/-/blob/LA.UM.7.3.r1-09300-sdm845.0/arch/arm64/boot/dts/qcom/sdm845.dtsi

Fixes: cc16687fbd74 ("arm64: dts: qcom: sdm845: add UFS controller")
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 arch/arm64/boot/dts/qcom/sdm845.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index 9ed74bf72d05..89520a9fe1e3 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -2614,7 +2614,7 @@ ufs_mem_hc: ufshc@1d84000 {
 				<0 0>,
 				<0 0>,
 				<0 0>,
-				<0 300000000>;
+				<75000000 300000000>;
 
 			status = "disabled";
 		};
-- 
2.25.1

Re: [PATCH 04/14] arm64: dts: qcom: sdm845: Fix the min frequency of "ice_core_clk"

[Konrad Dybcio]

On 12.07.2023 12:31, Manivannan Sadhasivam wrote:
> Minimum frequency of the "ice_core_clk" should be 75MHz as specified in the
> downstream vendor devicetree. So fix it!
> 
> https://git.codelinaro.org/clo/la/kernel/msm-4.9/-/blob/LA.UM.7.3.r1-09300-sdm845.0/arch/arm64/boot/dts/qcom/sdm845.dtsi
> 
> Fixes: cc16687fbd74 ("arm64: dts: qcom: sdm845: add UFS controller")
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
Would it make sense to move the ICE to the new bindings instead?

Can sdm845's ICE also work with the sdcard slot?

Konrad
>  arch/arm64/boot/dts/qcom/sdm845.dtsi | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index 9ed74bf72d05..89520a9fe1e3 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -2614,7 +2614,7 @@ ufs_mem_hc: ufshc@1d84000 {
>  				<0 0>,
>  				<0 0>,
>  				<0 0>,
> -				<0 300000000>;
> +				<75000000 300000000>;
>  
>  			status = "disabled";
>  		};

Re: [PATCH 04/14] arm64: dts: qcom: sdm845: Fix the min frequency of "ice_core_clk"

[Manivannan Sadhasivam]

On Wed, Jul 12, 2023 at 12:45:40PM +0200, Konrad Dybcio wrote:
> On 12.07.2023 12:31, Manivannan Sadhasivam wrote:
> > Minimum frequency of the "ice_core_clk" should be 75MHz as specified in the
> > downstream vendor devicetree. So fix it!
> > 
> > https://git.codelinaro.org/clo/la/kernel/msm-4.9/-/blob/LA.UM.7.3.r1-09300-sdm845.0/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > 
> > Fixes: cc16687fbd74 ("arm64: dts: qcom: sdm845: add UFS controller")
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> Would it make sense to move the ICE to the new bindings instead?
> 

That can be done later if required. Fixing the frequency just in this patch
allows it to get backported to stable.

> Can sdm845's ICE also work with the sdcard slot?
> 

I do not have any info on this.

- Mani

> Konrad
> >  arch/arm64/boot/dts/qcom/sdm845.dtsi | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > index 9ed74bf72d05..89520a9fe1e3 100644
> > --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > @@ -2614,7 +2614,7 @@ ufs_mem_hc: ufshc@1d84000 {
> >  				<0 0>,
> >  				<0 0>,
> >  				<0 0>,
> > -				<0 300000000>;
> > +				<75000000 300000000>;
> >  
> >  			status = "disabled";
> >  		};

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 04/14] arm64: dts: qcom: sdm845: Fix the min frequency of "ice_core_clk"

[Eric Biggers]

On Wed, Jul 12, 2023 at 04:01:59PM +0530, Manivannan Sadhasivam wrote:
> Minimum frequency of the "ice_core_clk" should be 75MHz as specified in the
> downstream vendor devicetree. So fix it!
> 
> https://git.codelinaro.org/clo/la/kernel/msm-4.9/-/blob/LA.UM.7.3.r1-09300-sdm845.0/arch/arm64/boot/dts/qcom/sdm845.dtsi
> 
> Fixes: cc16687fbd74 ("arm64: dts: qcom: sdm845: add UFS controller")
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/sdm845.dtsi | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index 9ed74bf72d05..89520a9fe1e3 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -2614,7 +2614,7 @@ ufs_mem_hc: ufshc@1d84000 {
>  				<0 0>,
>  				<0 0>,
>  				<0 0>,
> -				<0 300000000>;
> +				<75000000 300000000>;

Thanks for fixing this!  What was the visible effect of this bug, if any?
Do you know why the minimum has that particular value?

The Fixes tag is wrong; it should be:

Fixes: 433f9a57298f ("arm64: dts: sdm845: add Inline Crypto Engine registers and clock")

- Eric

Re: [PATCH 04/14] arm64: dts: qcom: sdm845: Fix the min frequency of "ice_core_clk"

[Manivannan Sadhasivam]

On Thu, Jul 13, 2023 at 12:30:03AM -0700, Eric Biggers wrote:
> On Wed, Jul 12, 2023 at 04:01:59PM +0530, Manivannan Sadhasivam wrote:
> > Minimum frequency of the "ice_core_clk" should be 75MHz as specified in the
> > downstream vendor devicetree. So fix it!
> > 
> > https://git.codelinaro.org/clo/la/kernel/msm-4.9/-/blob/LA.UM.7.3.r1-09300-sdm845.0/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > 
> > Fixes: cc16687fbd74 ("arm64: dts: qcom: sdm845: add UFS controller")
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  arch/arm64/boot/dts/qcom/sdm845.dtsi | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > index 9ed74bf72d05..89520a9fe1e3 100644
> > --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > @@ -2614,7 +2614,7 @@ ufs_mem_hc: ufshc@1d84000 {
> >  				<0 0>,
> >  				<0 0>,
> >  				<0 0>,
> > -				<0 300000000>;
> > +				<75000000 300000000>;
> 
> Thanks for fixing this!  What was the visible effect of this bug, if any?

No, this was just based on inspection.

> Do you know why the minimum has that particular value?
> 

Min and max frequencies come from the Qcom's internal documentation where they
have calculated the freq range based on RPMh performance states.

> The Fixes tag is wrong; it should be:
> 
> Fixes: 433f9a57298f ("arm64: dts: sdm845: add Inline Crypto Engine registers and clock")
> 

Yikes! Thanks for noticing, will fix it.

- Mani

> - Eric

-- 
மணிவண்ணன் சதாசிவம்

[PATCH 05/14] arm64: dts: qcom: sdm845: Add OPP table support to UFSHC

[Manivannan Sadhasivam]

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 | 41 +++++++++++++++++++++-------
 1 file changed, 31 insertions(+), 10 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index 89520a9fe1e3..e04a3cbb1017 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -2605,18 +2605,39 @@ 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>;
 			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 {
-- 
2.25.1

[PATCH 06/14] arm64: dts: qcom: sm8250: Add OPP table support to UFSHC

[Manivannan Sadhasivam]

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: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 arch/arm64/boot/dts/qcom/sm8250.dtsi | 39 +++++++++++++++++++++-------
 1 file changed, 30 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi
index 83ab6de459bc..72fd66db9c51 100644
--- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
@@ -2194,17 +2194,38 @@ ufs_mem_hc: ufshc@1d84000 {
 				<&gcc GCC_UFS_PHY_TX_SYMBOL_0_CLK>,
 				<&gcc GCC_UFS_PHY_RX_SYMBOL_0_CLK>,
 				<&gcc GCC_UFS_PHY_RX_SYMBOL_1_CLK>;
-			freq-table-hz =
-				<37500000 300000000>,
-				<0 0>,
-				<0 0>,
-				<37500000 300000000>,
-				<0 0>,
-				<0 0>,
-				<0 0>,
-				<0 0>;
+
+			operating-points-v2 = <&ufs_opp_table>;
 
 			status = "disabled";
+
+			ufs_opp_table: opp-table {
+				compatible = "operating-points-v2";
+
+				opp-37500000 {
+					opp-hz = /bits/ 64 <37500000>,
+						 /bits/ 64 <0>,
+						 /bits/ 64 <0>,
+						 /bits/ 64 <37500000>,
+						 /bits/ 64 <0>,
+						 /bits/ 64 <0>,
+						 /bits/ 64 <0>,
+						 /bits/ 64 <0>;
+					required-opps = <&rpmhpd_opp_low_svs>;
+				};
+
+				opp-300000000 {
+					opp-hz = /bits/ 64 <300000000>,
+						 /bits/ 64 <0>,
+						 /bits/ 64 <0>,
+						 /bits/ 64 <300000000>,
+						 /bits/ 64 <0>,
+						 /bits/ 64 <0>,
+						 /bits/ 64 <0>,
+						 /bits/ 64 <0>;
+					required-opps = <&rpmhpd_opp_nom>;
+				};
+			};
 		};
 
 		ufs_mem_phy: phy@1d87000 {
-- 
2.25.1

[PATCH 07/14] OPP: Introduce dev_pm_opp_find_freq_{ceil/floor}_indexed() APIs

[Manivannan Sadhasivam]

In the case of devices with multiple clocks, drivers need to specify the
clock index for the OPP framework to find the OPP corresponding to the
floor/ceil of the supplied frequency. So let's introduce the two new APIs
accepting the clock index as an argument.

These APIs use the exising _find_key_ceil() helper by supplying the clock
index to it.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/opp/core.c     | 54 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/pm_opp.h | 18 ++++++++++++++
 2 files changed, 72 insertions(+)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 3f46e499d615..a6d0b6b18e0e 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -658,6 +658,33 @@ struct dev_pm_opp *dev_pm_opp_find_freq_ceil(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_ceil);
 
+/**
+ * dev_pm_opp_find_freq_ceil_indexed() - Search for a rounded ceil freq for the
+ *					 clock corresponding to the index
+ * @dev:	Device for which we do this operation
+ * @freq:	Start frequency
+ * @index:	Clock index
+ *
+ * Search for the matching ceil *available* OPP for the clock corresponding to
+ * the specified index from a starting freq for a device.
+ *
+ * Return: matching *opp and refreshes *freq accordingly, else returns
+ * ERR_PTR in case of error and should be handled using IS_ERR. Error return
+ * values can be:
+ * EINVAL:	for bad pointer
+ * ERANGE:	no match found for search
+ * ENODEV:	if device not found in list of registered devices
+ *
+ * The callers are required to call dev_pm_opp_put() for the returned OPP after
+ * use.
+ */
+struct dev_pm_opp *dev_pm_opp_find_freq_ceil_indexed(struct device *dev,
+					     unsigned long *freq, u32 index)
+{
+	return _find_key_ceil(dev, freq, index, true, _read_freq, NULL);
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_ceil_indexed);
+
 /**
  * dev_pm_opp_find_freq_floor() - Search for a rounded floor freq
  * @dev:	device for which we do this operation
@@ -683,6 +710,33 @@ struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
 
+/**
+ * dev_pm_opp_find_freq_floor_indexed() - Search for a rounded floor freq for the
+ *					  clock corresponding to the index
+ * @dev:	Device for which we do this operation
+ * @freq:	Start frequency
+ * @index:	Clock index
+ *
+ * Search for the matching floor *available* OPP for the clock corresponding to
+ * the specified index from a starting freq for a device.
+ *
+ * Return: matching *opp and refreshes *freq accordingly, else returns
+ * ERR_PTR in case of error and should be handled using IS_ERR. Error return
+ * values can be:
+ * EINVAL:	for bad pointer
+ * ERANGE:	no match found for search
+ * ENODEV:	if device not found in list of registered devices
+ *
+ * The callers are required to call dev_pm_opp_put() for the returned OPP after
+ * use.
+ */
+struct dev_pm_opp *dev_pm_opp_find_freq_floor_indexed(struct device *dev,
+					      unsigned long *freq, u32 index)
+{
+	return _find_key_floor(dev, freq, index, true, _read_freq, NULL);
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor_indexed);
+
 /**
  * dev_pm_opp_find_level_exact() - search for an exact level
  * @dev:		device for which we do this operation
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index dc1fb5890792..991f54da79b5 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -124,6 +124,9 @@ struct dev_pm_opp *dev_pm_opp_find_freq_exact(struct device *dev,
 struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
 					      unsigned long *freq);
 
+struct dev_pm_opp *dev_pm_opp_find_freq_floor_indexed(struct device *dev,
+					      unsigned long *freq, u32 index);
+
 struct dev_pm_opp *dev_pm_opp_find_level_exact(struct device *dev,
 					       unsigned int level);
 struct dev_pm_opp *dev_pm_opp_find_level_ceil(struct device *dev,
@@ -132,6 +135,9 @@ struct dev_pm_opp *dev_pm_opp_find_level_ceil(struct device *dev,
 struct dev_pm_opp *dev_pm_opp_find_freq_ceil(struct device *dev,
 					     unsigned long *freq);
 
+struct dev_pm_opp *dev_pm_opp_find_freq_ceil_indexed(struct device *dev,
+					     unsigned long *freq, u32 index);
+
 struct dev_pm_opp *dev_pm_opp_find_bw_ceil(struct device *dev,
 					   unsigned int *bw, int index);
 
@@ -271,12 +277,24 @@ static inline struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
 	return ERR_PTR(-EOPNOTSUPP);
 }
 
+static inline struct dev_pm_opp *dev_pm_opp_find_freq_floor_indexed(struct device *dev,
+					unsigned long *freq, u32 index)
+{
+	return ERR_PTR(-EOPNOTSUPP);
+}
+
 static inline struct dev_pm_opp *dev_pm_opp_find_freq_ceil(struct device *dev,
 					unsigned long *freq)
 {
 	return ERR_PTR(-EOPNOTSUPP);
 }
 
+static inline struct dev_pm_opp *dev_pm_opp_find_freq_ceil_indexed(struct device *dev,
+					unsigned long *freq, u32 index)
+{
+	return ERR_PTR(-EOPNOTSUPP);
+}
+
 static inline struct dev_pm_opp *dev_pm_opp_find_bw_ceil(struct device *dev,
 					unsigned int *bw, int index)
 {
-- 
2.25.1

[PATCH 08/14] OPP: Introduce dev_pm_opp_get_freq_indexed() API

[Manivannan Sadhasivam]

In the case of devices with multiple clocks, drivers need to specify the
frequency index for the OPP framework to get the specific frequency within
the required OPP. So let's introduce the dev_pm_opp_get_freq_indexed() API
accepting the frequency index as an argument.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/opp/core.c     | 22 ++++++++++++++++++++++
 include/linux/pm_opp.h |  8 ++++++++
 2 files changed, 30 insertions(+)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index a6d0b6b18e0e..66dc0d0cfaed 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -197,6 +197,28 @@ unsigned long dev_pm_opp_get_freq(struct dev_pm_opp *opp)
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_get_freq);
 
+/**
+ * dev_pm_opp_get_freq_indexed() - Gets the frequency corresponding to an
+ *				   available opp with specified index
+ * @opp: opp for which frequency has to be returned for
+ * @index: index of the frequency within the required opp
+ *
+ * Return: frequency in hertz corresponding to the opp with specified index,
+ * else return 0
+ */
+unsigned long dev_pm_opp_get_freq_indexed(struct dev_pm_opp *opp, u32 index)
+{
+	struct opp_table *opp_table = opp->opp_table;
+
+	if (IS_ERR_OR_NULL(opp) || index >= opp_table->clk_count) {
+		pr_err("%s: Invalid parameters\n", __func__);
+		return 0;
+	}
+
+	return opp->rates[index];
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_get_freq_indexed);
+
 /**
  * dev_pm_opp_get_level() - Gets the level corresponding to an available opp
  * @opp:	opp for which level value has to be returned for
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 991f54da79b5..97eb6159fb93 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -105,6 +105,8 @@ unsigned long dev_pm_opp_get_power(struct dev_pm_opp *opp);
 
 unsigned long dev_pm_opp_get_freq(struct dev_pm_opp *opp);
 
+unsigned long dev_pm_opp_get_freq_indexed(struct dev_pm_opp *opp, u32 index);
+
 unsigned int dev_pm_opp_get_level(struct dev_pm_opp *opp);
 
 unsigned int dev_pm_opp_get_required_pstate(struct dev_pm_opp *opp,
@@ -211,6 +213,12 @@ static inline unsigned long dev_pm_opp_get_freq(struct dev_pm_opp *opp)
 	return 0;
 }
 
+static inline unsigned long dev_pm_opp_get_freq_indexed(struct dev_pm_opp *opp,
+						      u32 index)
+{
+	return 0;
+}
+
 static inline unsigned int dev_pm_opp_get_level(struct dev_pm_opp *opp)
 {
 	return 0;
-- 
2.25.1

[PATCH 09/14] PM / devfreq: Switch to dev_pm_opp_find_freq_{ceil/floor}_indexed() APIs

[Manivannan Sadhasivam]

Some devfreq consumers like UFS driver need to work with multiple clocks
through the OPP framework. For this reason, OPP framework exposes the
_indexed() APIs for finding the floor/ceil of the supplied frequency of
the indexed clock. So let's use them in the devfreq driver.

Currently, the clock index of 0 is used which works fine for multiple as
well as single clock.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/devfreq/devfreq.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index e36cbb920ec8..7686993d639f 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -88,7 +88,7 @@ static unsigned long find_available_min_freq(struct devfreq *devfreq)
 	struct dev_pm_opp *opp;
 	unsigned long min_freq = 0;
 
-	opp = dev_pm_opp_find_freq_ceil(devfreq->dev.parent, &min_freq);
+	opp = dev_pm_opp_find_freq_ceil_indexed(devfreq->dev.parent, &min_freq, 0);
 	if (IS_ERR(opp))
 		min_freq = 0;
 	else
@@ -102,7 +102,7 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq)
 	struct dev_pm_opp *opp;
 	unsigned long max_freq = ULONG_MAX;
 
-	opp = dev_pm_opp_find_freq_floor(devfreq->dev.parent, &max_freq);
+	opp = dev_pm_opp_find_freq_floor_indexed(devfreq->dev.parent, &max_freq, 0);
 	if (IS_ERR(opp))
 		max_freq = 0;
 	else
@@ -196,7 +196,7 @@ static int set_freq_table(struct devfreq *devfreq)
 		return -ENOMEM;
 
 	for (i = 0, freq = 0; i < devfreq->max_state; i++, freq++) {
-		opp = dev_pm_opp_find_freq_ceil(devfreq->dev.parent, &freq);
+		opp = dev_pm_opp_find_freq_ceil_indexed(devfreq->dev.parent, &freq, 0);
 		if (IS_ERR(opp)) {
 			devm_kfree(devfreq->dev.parent, devfreq->freq_table);
 			return PTR_ERR(opp);
@@ -2034,18 +2034,18 @@ struct dev_pm_opp *devfreq_recommended_opp(struct device *dev,
 
 	if (flags & DEVFREQ_FLAG_LEAST_UPPER_BOUND) {
 		/* The freq is an upper bound. opp should be lower */
-		opp = dev_pm_opp_find_freq_floor(dev, freq);
+		opp = dev_pm_opp_find_freq_floor_indexed(dev, freq, 0);
 
 		/* If not available, use the closest opp */
 		if (opp == ERR_PTR(-ERANGE))
-			opp = dev_pm_opp_find_freq_ceil(dev, freq);
+			opp = dev_pm_opp_find_freq_ceil_indexed(dev, freq, 0);
 	} else {
 		/* The freq is an lower bound. opp should be higher */
-		opp = dev_pm_opp_find_freq_ceil(dev, freq);
+		opp = dev_pm_opp_find_freq_ceil_indexed(dev, freq, 0);
 
 		/* If not available, use the closest opp */
 		if (opp == ERR_PTR(-ERANGE))
-			opp = dev_pm_opp_find_freq_floor(dev, freq);
+			opp = dev_pm_opp_find_freq_floor_indexed(dev, freq, 0);
 	}
 
 	return opp;
-- 
2.25.1

[PATCH 10/14] scsi: ufs: core: Add OPP support for scaling clocks and regulators

[Manivannan Sadhasivam]

UFS core is only scaling the clocks during devfreq scaling and
initialization. But for an optimum power saving, regulators should also
be scaled along with the clocks.

So let's use the OPP framework which supports scaling clocks, regulators,
and performance state using OPP table defined in devicetree. For
accomodating the OPP support, the existing APIs (ufshcd_scale_clks,
ufshcd_is_devfreq_scaling_required and ufshcd_devfreq_scale) are modified
to accept "freq" as an argument which in turn used by the OPP helpers.

The OPP support is added along with the old freq-table based clock scaling
so that the existing platforms work as expected.

Co-developed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/ufs/core/ufshcd.c | 142 +++++++++++++++++++++++++++++---------
 include/ufs/ufshcd.h      |   4 ++
 2 files changed, 113 insertions(+), 33 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 983fae84d9e8..119496bb11fb 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -20,6 +20,7 @@
 #include <linux/delay.h>
 #include <linux/interrupt.h>
 #include <linux/module.h>
+#include <linux/pm_opp.h>
 #include <linux/regulator/consumer.h>
 #include <linux/sched/clock.h>
 #include <scsi/scsi_cmnd.h>
@@ -276,7 +277,8 @@ static int ufshcd_host_reset_and_restore(struct ufs_hba *hba);
 static void ufshcd_resume_clkscaling(struct ufs_hba *hba);
 static void ufshcd_suspend_clkscaling(struct ufs_hba *hba);
 static void __ufshcd_suspend_clkscaling(struct ufs_hba *hba);
-static int ufshcd_scale_clks(struct ufs_hba *hba, bool scale_up);
+static int ufshcd_scale_clks(struct ufs_hba *hba, unsigned long freq,
+			     bool scale_up);
 static irqreturn_t ufshcd_intr(int irq, void *__hba);
 static int ufshcd_change_power_mode(struct ufs_hba *hba,
 			     struct ufs_pa_layer_attr *pwr_mode);
@@ -1087,15 +1089,31 @@ static int ufshcd_set_clk_freq(struct ufs_hba *hba, bool scale_up)
 	return ret;
 }
 
+static int ufshcd_opp_set_rate(struct ufs_hba *hba, unsigned long freq)
+{
+	struct dev_pm_opp *opp;
+
+	opp = dev_pm_opp_find_freq_floor_indexed(hba->dev,
+						 &freq, 0);
+	if (IS_ERR(opp))
+		return PTR_ERR(opp);
+
+	dev_pm_opp_put(opp);
+
+	return dev_pm_opp_set_opp(hba->dev, opp);
+}
+
 /**
  * ufshcd_scale_clks - scale up or scale down UFS controller clocks
  * @hba: per adapter instance
+ * @freq: frequency to scale
  * @scale_up: True if scaling up and false if scaling down
  *
  * Returns 0 if successful
  * Returns < 0 for any other errors
  */
-static int ufshcd_scale_clks(struct ufs_hba *hba, bool scale_up)
+static int ufshcd_scale_clks(struct ufs_hba *hba, unsigned long freq,
+			     bool scale_up)
 {
 	int ret = 0;
 	ktime_t start = ktime_get();
@@ -1104,13 +1122,21 @@ static int ufshcd_scale_clks(struct ufs_hba *hba, bool scale_up)
 	if (ret)
 		goto out;
 
-	ret = ufshcd_set_clk_freq(hba, scale_up);
+	if (hba->use_pm_opp)
+		ret = ufshcd_opp_set_rate(hba, freq);
+	else
+		ret = ufshcd_set_clk_freq(hba, scale_up);
 	if (ret)
 		goto out;
 
 	ret = ufshcd_vops_clk_scale_notify(hba, scale_up, POST_CHANGE);
-	if (ret)
-		ufshcd_set_clk_freq(hba, !scale_up);
+	if (ret) {
+		if (hba->use_pm_opp)
+			ufshcd_opp_set_rate(hba,
+					    hba->devfreq->previous_freq);
+		else
+			ufshcd_set_clk_freq(hba, !scale_up);
+	}
 
 out:
 	trace_ufshcd_profile_clk_scaling(dev_name(hba->dev),
@@ -1122,12 +1148,13 @@ static int ufshcd_scale_clks(struct ufs_hba *hba, bool scale_up)
 /**
  * ufshcd_is_devfreq_scaling_required - check if scaling is required or not
  * @hba: per adapter instance
+ * @freq: frequency to scale
  * @scale_up: True if scaling up and false if scaling down
  *
  * Returns true if scaling is required, false otherwise.
  */
 static bool ufshcd_is_devfreq_scaling_required(struct ufs_hba *hba,
-					       bool scale_up)
+					       unsigned long freq, bool scale_up)
 {
 	struct ufs_clk_info *clki;
 	struct list_head *head = &hba->clk_list_head;
@@ -1135,6 +1162,9 @@ static bool ufshcd_is_devfreq_scaling_required(struct ufs_hba *hba,
 	if (list_empty(head))
 		return false;
 
+	if (hba->use_pm_opp)
+		return freq != hba->clk_scaling.target_freq;
+
 	list_for_each_entry(clki, head, list) {
 		if (!IS_ERR_OR_NULL(clki->clk)) {
 			if (scale_up && clki->max_freq) {
@@ -1331,13 +1361,15 @@ static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, int err, bool sc
 /**
  * ufshcd_devfreq_scale - scale up/down UFS clocks and gear
  * @hba: per adapter instance
+ * @freq: frequency to scale
  * @scale_up: True for scaling up and false for scalin down
  *
  * Returns 0 for success,
  * Returns -EBUSY if scaling can't happen at this time
  * Returns non-zero for any other errors
  */
-static int ufshcd_devfreq_scale(struct ufs_hba *hba, bool scale_up)
+static int ufshcd_devfreq_scale(struct ufs_hba *hba, unsigned long freq,
+				bool scale_up)
 {
 	int ret = 0;
 
@@ -1352,7 +1384,7 @@ static int ufshcd_devfreq_scale(struct ufs_hba *hba, bool scale_up)
 			goto out_unprepare;
 	}
 
-	ret = ufshcd_scale_clks(hba, scale_up);
+	ret = ufshcd_scale_clks(hba, freq, scale_up);
 	if (ret) {
 		if (!scale_up)
 			ufshcd_scale_gear(hba, true);
@@ -1363,7 +1395,8 @@ static int ufshcd_devfreq_scale(struct ufs_hba *hba, bool scale_up)
 	if (scale_up) {
 		ret = ufshcd_scale_gear(hba, true);
 		if (ret) {
-			ufshcd_scale_clks(hba, false);
+			ufshcd_scale_clks(hba, hba->devfreq->previous_freq,
+					  false);
 			goto out_unprepare;
 		}
 	}
@@ -1421,9 +1454,22 @@ static int ufshcd_devfreq_target(struct device *dev,
 	if (!ufshcd_is_clkscaling_supported(hba))
 		return -EINVAL;
 
-	clki = list_first_entry(&hba->clk_list_head, struct ufs_clk_info, list);
-	/* Override with the closest supported frequency */
-	*freq = (unsigned long) clk_round_rate(clki->clk, *freq);
+	if (hba->use_pm_opp) {
+		struct dev_pm_opp *opp;
+
+		/* Get the recommended frequency from OPP framework */
+		opp = devfreq_recommended_opp(dev, freq, flags);
+		if (IS_ERR(opp))
+			return PTR_ERR(opp);
+
+		dev_pm_opp_put(opp);
+	} else {
+		/* Override with the closest supported frequency */
+		clki = list_first_entry(&hba->clk_list_head, struct ufs_clk_info,
+					list);
+		*freq =	(unsigned long) clk_round_rate(clki->clk, *freq);
+	}
+
 	spin_lock_irqsave(hba->host->host_lock, irq_flags);
 	if (ufshcd_eh_in_progress(hba)) {
 		spin_unlock_irqrestore(hba->host->host_lock, irq_flags);
@@ -1438,12 +1484,17 @@ static int ufshcd_devfreq_target(struct device *dev,
 		goto out;
 	}
 
-	/* Decide based on the rounded-off frequency and update */
-	scale_up = *freq == clki->max_freq;
-	if (!scale_up)
+	/* Decide based on the target or rounded-off frequency and update */
+	if (hba->use_pm_opp)
+		scale_up = *freq > hba->clk_scaling.target_freq;
+	else
+		scale_up = *freq == clki->max_freq;
+
+	if (!hba->use_pm_opp && !scale_up)
 		*freq = clki->min_freq;
+
 	/* Update the frequency */
-	if (!ufshcd_is_devfreq_scaling_required(hba, scale_up)) {
+	if (!ufshcd_is_devfreq_scaling_required(hba, *freq, scale_up)) {
 		spin_unlock_irqrestore(hba->host->host_lock, irq_flags);
 		ret = 0;
 		goto out; /* no state change required */
@@ -1451,7 +1502,9 @@ static int ufshcd_devfreq_target(struct device *dev,
 	spin_unlock_irqrestore(hba->host->host_lock, irq_flags);
 
 	start = ktime_get();
-	ret = ufshcd_devfreq_scale(hba, scale_up);
+	ret = ufshcd_devfreq_scale(hba, *freq, scale_up);
+	if (!ret)
+		hba->clk_scaling.target_freq = *freq;
 
 	trace_ufshcd_profile_clk_scaling(dev_name(hba->dev),
 		(scale_up ? "up" : "down"),
@@ -1471,8 +1524,6 @@ static int ufshcd_devfreq_get_dev_status(struct device *dev,
 	struct ufs_hba *hba = dev_get_drvdata(dev);
 	struct ufs_clk_scaling *scaling = &hba->clk_scaling;
 	unsigned long flags;
-	struct list_head *clk_list = &hba->clk_list_head;
-	struct ufs_clk_info *clki;
 	ktime_t curr_t;
 
 	if (!ufshcd_is_clkscaling_supported(hba))
@@ -1485,17 +1536,24 @@ static int ufshcd_devfreq_get_dev_status(struct device *dev,
 	if (!scaling->window_start_t)
 		goto start_window;
 
-	clki = list_first_entry(clk_list, struct ufs_clk_info, list);
 	/*
 	 * If current frequency is 0, then the ondemand governor considers
 	 * there's no initial frequency set. And it always requests to set
 	 * to max. frequency.
 	 */
-	stat->current_frequency = clki->curr_freq;
+	if (hba->use_pm_opp) {
+		stat->current_frequency = hba->clk_scaling.target_freq;
+	} else {
+		struct list_head *clk_list = &hba->clk_list_head;
+		struct ufs_clk_info *clki;
+
+		clki = list_first_entry(clk_list, struct ufs_clk_info, list);
+		stat->current_frequency = clki->curr_freq;
+	}
+
 	if (scaling->is_busy_started)
 		scaling->tot_busy_t += ktime_us_delta(curr_t,
 				scaling->busy_start_t);
-
 	stat->total_time = ktime_us_delta(curr_t, scaling->window_start_t);
 	stat->busy_time = scaling->tot_busy_t;
 start_window:
@@ -1524,9 +1582,11 @@ static int ufshcd_devfreq_init(struct ufs_hba *hba)
 	if (list_empty(clk_list))
 		return 0;
 
-	clki = list_first_entry(clk_list, struct ufs_clk_info, list);
-	dev_pm_opp_add(hba->dev, clki->min_freq, 0);
-	dev_pm_opp_add(hba->dev, clki->max_freq, 0);
+	if (!hba->use_pm_opp) {
+		clki = list_first_entry(clk_list, struct ufs_clk_info, list);
+		dev_pm_opp_add(hba->dev, clki->min_freq, 0);
+		dev_pm_opp_add(hba->dev, clki->max_freq, 0);
+	}
 
 	ufshcd_vops_config_scaling_param(hba, &hba->vps->devfreq_profile,
 					 &hba->vps->ondemand_data);
@@ -1538,8 +1598,10 @@ static int ufshcd_devfreq_init(struct ufs_hba *hba)
 		ret = PTR_ERR(devfreq);
 		dev_err(hba->dev, "Unable to register with devfreq %d\n", ret);
 
-		dev_pm_opp_remove(hba->dev, clki->min_freq);
-		dev_pm_opp_remove(hba->dev, clki->max_freq);
+		if (!hba->use_pm_opp) {
+			dev_pm_opp_remove(hba->dev, clki->min_freq);
+			dev_pm_opp_remove(hba->dev, clki->max_freq);
+		}
 		return ret;
 	}
 
@@ -1551,7 +1613,6 @@ static int ufshcd_devfreq_init(struct ufs_hba *hba)
 static void ufshcd_devfreq_remove(struct ufs_hba *hba)
 {
 	struct list_head *clk_list = &hba->clk_list_head;
-	struct ufs_clk_info *clki;
 
 	if (!hba->devfreq)
 		return;
@@ -1559,9 +1620,13 @@ static void ufshcd_devfreq_remove(struct ufs_hba *hba)
 	devfreq_remove_device(hba->devfreq);
 	hba->devfreq = NULL;
 
-	clki = list_first_entry(clk_list, struct ufs_clk_info, list);
-	dev_pm_opp_remove(hba->dev, clki->min_freq);
-	dev_pm_opp_remove(hba->dev, clki->max_freq);
+	if (!hba->use_pm_opp) {
+		struct ufs_clk_info *clki;
+
+		clki = list_first_entry(clk_list, struct ufs_clk_info, list);
+		dev_pm_opp_remove(hba->dev, clki->min_freq);
+		dev_pm_opp_remove(hba->dev, clki->max_freq);
+	}
 }
 
 static void __ufshcd_suspend_clkscaling(struct ufs_hba *hba)
@@ -1646,7 +1711,7 @@ static ssize_t ufshcd_clkscale_enable_store(struct device *dev,
 		ufshcd_resume_clkscaling(hba);
 	} else {
 		ufshcd_suspend_clkscaling(hba);
-		err = ufshcd_devfreq_scale(hba, true);
+		err = ufshcd_devfreq_scale(hba, ULONG_MAX, true);
 		if (err)
 			dev_err(hba->dev, "%s: failed to scale clocks up %d\n",
 					__func__, err);
@@ -7666,7 +7731,7 @@ static int ufshcd_host_reset_and_restore(struct ufs_hba *hba)
 	hba->silence_err_logs = false;
 
 	/* scale up clocks to max frequency before full reinitialization */
-	ufshcd_scale_clks(hba, true);
+	ufshcd_scale_clks(hba, ULONG_MAX, true);
 
 	err = ufshcd_hba_enable(hba);
 
@@ -9185,6 +9250,17 @@ static int ufshcd_init_clocks(struct ufs_hba *hba)
 		dev_dbg(dev, "%s: clk: %s, rate: %lu\n", __func__,
 				clki->name, clk_get_rate(clki->clk));
 	}
+
+	/* Set Max. frequency for all clocks */
+	if (hba->use_pm_opp) {
+		ret = ufshcd_opp_set_rate(hba, ULONG_MAX);
+		if (ret) {
+			dev_err(hba->dev, "%s: failed to set OPP: %d", __func__,
+				ret);
+			goto out;
+		}
+	}
+
 out:
 	return ret;
 }
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index 6dc11fa0ebb1..9f61b6d56d11 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -407,6 +407,7 @@ struct ufs_clk_gating {
  * @workq: workqueue to schedule devfreq suspend/resume work
  * @suspend_work: worker to suspend devfreq
  * @resume_work: worker to resume devfreq
+ * @target_freq: frequency requested by devfreq framework
  * @min_gear: lowest HS gear to scale down to
  * @is_enabled: tracks if scaling is currently enabled or not, controlled by
  *		clkscale_enable sysfs node
@@ -426,6 +427,7 @@ struct ufs_clk_scaling {
 	struct workqueue_struct *workq;
 	struct work_struct suspend_work;
 	struct work_struct resume_work;
+	unsigned long target_freq;
 	u32 min_gear;
 	bool is_enabled;
 	bool is_allowed;
@@ -870,6 +872,7 @@ enum ufshcd_mcq_opr {
  * @auto_bkops_enabled: to track whether bkops is enabled in device
  * @vreg_info: UFS device voltage regulator information
  * @clk_list_head: UFS host controller clocks list node head
+ * @use_pm_opp: Indicates whether OPP based scaling is used or not
  * @req_abort_count: number of times ufshcd_abort() has been called
  * @lanes_per_direction: number of lanes per data direction between the UFS
  *	controller and the UFS device.
@@ -1021,6 +1024,7 @@ struct ufs_hba {
 	bool auto_bkops_enabled;
 	struct ufs_vreg_info vreg_info;
 	struct list_head clk_list_head;
+	bool use_pm_opp;
 
 	/* Number of requests aborts */
 	int req_abort_count;
-- 
2.25.1

Re: [PATCH 10/14] scsi: ufs: core: Add OPP support for scaling clocks and regulators

[Viresh Kumar]

On 12-07-23, 16:02, Manivannan Sadhasivam wrote:
> +static int ufshcd_opp_set_rate(struct ufs_hba *hba, unsigned long freq)
> +{
> +	struct dev_pm_opp *opp;
> +
> +	opp = dev_pm_opp_find_freq_floor_indexed(hba->dev,
> +						 &freq, 0);
> +	if (IS_ERR(opp))
> +		return PTR_ERR(opp);
> +
> +	dev_pm_opp_put(opp);

You can't free the OPP while you are still using it :)

> +
> +	return dev_pm_opp_set_opp(hba->dev, opp);
> +}

-- 
viresh

Re: [PATCH 10/14] scsi: ufs: core: Add OPP support for scaling clocks and regulators

[Manivannan Sadhasivam]

On Thu, Jul 13, 2023 at 09:31:42AM +0530, Viresh Kumar wrote:
> On 12-07-23, 16:02, Manivannan Sadhasivam wrote:
> > +static int ufshcd_opp_set_rate(struct ufs_hba *hba, unsigned long freq)
> > +{
> > +	struct dev_pm_opp *opp;
> > +
> > +	opp = dev_pm_opp_find_freq_floor_indexed(hba->dev,
> > +						 &freq, 0);
> > +	if (IS_ERR(opp))
> > +		return PTR_ERR(opp);
> > +
> > +	dev_pm_opp_put(opp);
> 
> You can't free the OPP while you are still using it :)
> 

Ah... will fix it!

- Mani

> > +
> > +	return dev_pm_opp_set_opp(hba->dev, opp);
> > +}
> 
> -- 
> viresh

-- 
மணிவண்ணன் சதாசிவம்

[PATCH 10/13] scsi: ufs: host: Add support for parsing OPP

[Manivannan Sadhasivam]

OPP framework can be used to scale the clocks along with other entities
such as regulators, performance state etc... So let's add support for
parsing OPP from devicetree. OPP support in devicetree is added through
the "operating-points-v2" property which accepts the OPP table defining
clock frequency, regulator voltage, power domain performance state etc...

Since the UFS controller requires multiple clocks to be controlled for
proper working, devm_pm_opp_set_config() has been used which supports
scaling multiple clocks through custom ufshcd_opp_config_clks() callback.

It should be noted that the OPP support is not compatible with the old
"freq-table-hz" property. So only one can be used at a time even though
the UFS core supports both.

Co-developed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/ufs/host/ufshcd-pltfrm.c | 116 +++++++++++++++++++++++++++++++
 1 file changed, 116 insertions(+)

diff --git a/drivers/ufs/host/ufshcd-pltfrm.c b/drivers/ufs/host/ufshcd-pltfrm.c
index 0b7430033047..068c22378c88 100644
--- a/drivers/ufs/host/ufshcd-pltfrm.c
+++ b/drivers/ufs/host/ufshcd-pltfrm.c
@@ -8,8 +8,10 @@
  *	Vinayak Holikatti <h.vinayak@samsung.com>
  */
 
+#include <linux/clk.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/pm_opp.h>
 #include <linux/pm_runtime.h>
 #include <linux/of.h>
 
@@ -17,6 +19,8 @@
 #include "ufshcd-pltfrm.h"
 #include <ufs/unipro.h>
 
+#include <trace/events/ufs.h>
+
 #define UFSHCD_DEFAULT_LANES_PER_DIRECTION		2
 
 static int ufshcd_parse_clock_info(struct ufs_hba *hba)
@@ -205,6 +209,112 @@ static void ufshcd_init_lanes_per_dir(struct ufs_hba *hba)
 	}
 }
 
+static int ufshcd_opp_config_clks(struct device *dev, struct opp_table *opp_table,
+				  struct dev_pm_opp *opp, void *data,
+				  bool scaling_down)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+	struct list_head *head = &hba->clk_list_head;
+	struct ufs_clk_info *clki;
+	unsigned long freq;
+	u8 idx = 0;
+	int ret;
+
+	list_for_each_entry(clki, head, list) {
+		if (!IS_ERR_OR_NULL(clki->clk)) {
+			freq = dev_pm_opp_get_freq_indexed(opp, idx++);
+
+			/* Do not set rate for clocks having frequency as 0 */
+			if (!freq)
+				continue;
+
+			ret = clk_set_rate(clki->clk, freq);
+			if (ret) {
+				dev_err(dev, "%s: %s clk set rate(%ldHz) failed, %d\n",
+					__func__, clki->name, freq, ret);
+				return ret;
+			}
+
+			trace_ufshcd_clk_scaling(dev_name(dev),
+				(scaling_down ? "scaled down" : "scaled up"),
+				clki->name, hba->clk_scaling.target_freq, freq);
+		}
+	}
+
+	return 0;
+}
+
+static int ufshcd_parse_operating_points(struct ufs_hba *hba)
+{
+	struct device *dev = hba->dev;
+	struct device_node *np = dev->of_node;
+	struct dev_pm_opp_config config = {};
+	struct ufs_clk_info *clki;
+	const char **clk_names;
+	int cnt, i, ret;
+
+	if (!of_find_property(np, "operating-points-v2", NULL))
+		return 0;
+
+	if (of_find_property(np, "freq-table-hz", NULL)) {
+		dev_err(dev, "%s: operating-points and freq-table-hz are incompatible\n",
+			 __func__);
+		return -EINVAL;
+	}
+
+	cnt = of_property_count_strings(np, "clock-names");
+	if (cnt <= 0) {
+		dev_err(dev, "%s: Missing clock-names\n",  __func__);
+		return -ENODEV;
+	}
+
+	/* OPP expects clk_names to be NULL terminated */
+	clk_names = devm_kcalloc(dev, cnt + 1, sizeof(*clk_names), GFP_KERNEL);
+	if (!clk_names)
+		return -ENOMEM;
+
+	/*
+	 * We still need to get reference to all clocks as the UFS core uses
+	 * them separately.
+	 */
+	for (i = 0; i < cnt; i++) {
+		ret = of_property_read_string_index(np, "clock-names", i,
+						    &clk_names[i]);
+		if (ret)
+			return ret;
+
+		clki = devm_kzalloc(dev, sizeof(*clki), GFP_KERNEL);
+		if (!clki)
+			return -ENOMEM;
+
+		clki->name = devm_kstrdup(dev, clk_names[i], GFP_KERNEL);
+		if (!clki->name)
+			return -ENOMEM;
+
+		if (!strcmp(clk_names[i], "ref_clk"))
+			clki->keep_link_active = true;
+
+		list_add_tail(&clki->list, &hba->clk_list_head);
+	}
+
+	config.clk_names = clk_names,
+	config.config_clks = ufshcd_opp_config_clks;
+
+	ret = devm_pm_opp_set_config(dev, &config);
+	if (ret)
+		return ret;
+
+	ret = devm_pm_opp_of_add_table(dev);
+	if (ret) {
+		dev_err(dev, "Failed to add OPP table: %d\n", ret);
+		return ret;
+	}
+
+	hba->use_pm_opp = true;
+
+	return 0;
+}
+
 /**
  * ufshcd_get_pwr_dev_param - get finally agreed attributes for
  *                            power mode change
@@ -371,6 +481,12 @@ int ufshcd_pltfrm_init(struct platform_device *pdev,
 
 	ufshcd_init_lanes_per_dir(hba);
 
+	err = ufshcd_parse_operating_points(hba);
+	if (err) {
+		dev_err(dev, "%s: OPP parse failed %d\n", __func__, err);
+		goto dealloc_host;
+	}
+
 	err = ufshcd_init(hba, mmio_base, irq);
 	if (err) {
 		dev_err(dev, "Initialization failed\n");
-- 
2.25.1

[PATCH 11/13] arm64: dts: qcom: sdm845: Add interconnect paths to UFSHC

[Manivannan Sadhasivam]

UFS host controller requires interconnect path configuration for proper
working. So let's specify them for SDM845 SoC.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 arch/arm64/boot/dts/qcom/sdm845.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index e04a3cbb1017..2ea6eb44953e 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -2607,6 +2607,11 @@ ufs_mem_hc: ufshc@1d84000 {
 				<&gcc GCC_UFS_PHY_ICE_CORE_CLK>;
 
 			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 {
-- 
2.25.1

Re: [PATCH 11/13] arm64: dts: qcom: sdm845: Add interconnect paths to UFSHC

[Konrad Dybcio]

On 12.07.2023 12:32, Manivannan Sadhasivam wrote:
> UFS host controller requires interconnect path configuration for proper
> working. So let's specify them for SDM845 SoC.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad
>  arch/arm64/boot/dts/qcom/sdm845.dtsi | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index e04a3cbb1017..2ea6eb44953e 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -2607,6 +2607,11 @@ ufs_mem_hc: ufshc@1d84000 {
>  				<&gcc GCC_UFS_PHY_ICE_CORE_CLK>;
>  
>  			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 {

[PATCH 11/14] scsi: ufs: host: Add support for parsing OPP

[Manivannan Sadhasivam]

OPP framework can be used to scale the clocks along with other entities
such as regulators, performance state etc... So let's add support for
parsing OPP from devicetree. OPP support in devicetree is added through
the "operating-points-v2" property which accepts the OPP table defining
clock frequency, regulator voltage, power domain performance state etc...

Since the UFS controller requires multiple clocks to be controlled for
proper working, devm_pm_opp_set_config() has been used which supports
scaling multiple clocks through custom ufshcd_opp_config_clks() callback.

It should be noted that the OPP support is not compatible with the old
"freq-table-hz" property. So only one can be used at a time even though
the UFS core supports both.

Co-developed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/ufs/host/ufshcd-pltfrm.c | 116 +++++++++++++++++++++++++++++++
 1 file changed, 116 insertions(+)

diff --git a/drivers/ufs/host/ufshcd-pltfrm.c b/drivers/ufs/host/ufshcd-pltfrm.c
index 0b7430033047..068c22378c88 100644
--- a/drivers/ufs/host/ufshcd-pltfrm.c
+++ b/drivers/ufs/host/ufshcd-pltfrm.c
@@ -8,8 +8,10 @@
  *	Vinayak Holikatti <h.vinayak@samsung.com>
  */
 
+#include <linux/clk.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/pm_opp.h>
 #include <linux/pm_runtime.h>
 #include <linux/of.h>
 
@@ -17,6 +19,8 @@
 #include "ufshcd-pltfrm.h"
 #include <ufs/unipro.h>
 
+#include <trace/events/ufs.h>
+
 #define UFSHCD_DEFAULT_LANES_PER_DIRECTION		2
 
 static int ufshcd_parse_clock_info(struct ufs_hba *hba)
@@ -205,6 +209,112 @@ static void ufshcd_init_lanes_per_dir(struct ufs_hba *hba)
 	}
 }
 
+static int ufshcd_opp_config_clks(struct device *dev, struct opp_table *opp_table,
+				  struct dev_pm_opp *opp, void *data,
+				  bool scaling_down)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+	struct list_head *head = &hba->clk_list_head;
+	struct ufs_clk_info *clki;
+	unsigned long freq;
+	u8 idx = 0;
+	int ret;
+
+	list_for_each_entry(clki, head, list) {
+		if (!IS_ERR_OR_NULL(clki->clk)) {
+			freq = dev_pm_opp_get_freq_indexed(opp, idx++);
+
+			/* Do not set rate for clocks having frequency as 0 */
+			if (!freq)
+				continue;
+
+			ret = clk_set_rate(clki->clk, freq);
+			if (ret) {
+				dev_err(dev, "%s: %s clk set rate(%ldHz) failed, %d\n",
+					__func__, clki->name, freq, ret);
+				return ret;
+			}
+
+			trace_ufshcd_clk_scaling(dev_name(dev),
+				(scaling_down ? "scaled down" : "scaled up"),
+				clki->name, hba->clk_scaling.target_freq, freq);
+		}
+	}
+
+	return 0;
+}
+
+static int ufshcd_parse_operating_points(struct ufs_hba *hba)
+{
+	struct device *dev = hba->dev;
+	struct device_node *np = dev->of_node;
+	struct dev_pm_opp_config config = {};
+	struct ufs_clk_info *clki;
+	const char **clk_names;
+	int cnt, i, ret;
+
+	if (!of_find_property(np, "operating-points-v2", NULL))
+		return 0;
+
+	if (of_find_property(np, "freq-table-hz", NULL)) {
+		dev_err(dev, "%s: operating-points and freq-table-hz are incompatible\n",
+			 __func__);
+		return -EINVAL;
+	}
+
+	cnt = of_property_count_strings(np, "clock-names");
+	if (cnt <= 0) {
+		dev_err(dev, "%s: Missing clock-names\n",  __func__);
+		return -ENODEV;
+	}
+
+	/* OPP expects clk_names to be NULL terminated */
+	clk_names = devm_kcalloc(dev, cnt + 1, sizeof(*clk_names), GFP_KERNEL);
+	if (!clk_names)
+		return -ENOMEM;
+
+	/*
+	 * We still need to get reference to all clocks as the UFS core uses
+	 * them separately.
+	 */
+	for (i = 0; i < cnt; i++) {
+		ret = of_property_read_string_index(np, "clock-names", i,
+						    &clk_names[i]);
+		if (ret)
+			return ret;
+
+		clki = devm_kzalloc(dev, sizeof(*clki), GFP_KERNEL);
+		if (!clki)
+			return -ENOMEM;
+
+		clki->name = devm_kstrdup(dev, clk_names[i], GFP_KERNEL);
+		if (!clki->name)
+			return -ENOMEM;
+
+		if (!strcmp(clk_names[i], "ref_clk"))
+			clki->keep_link_active = true;
+
+		list_add_tail(&clki->list, &hba->clk_list_head);
+	}
+
+	config.clk_names = clk_names,
+	config.config_clks = ufshcd_opp_config_clks;
+
+	ret = devm_pm_opp_set_config(dev, &config);
+	if (ret)
+		return ret;
+
+	ret = devm_pm_opp_of_add_table(dev);
+	if (ret) {
+		dev_err(dev, "Failed to add OPP table: %d\n", ret);
+		return ret;
+	}
+
+	hba->use_pm_opp = true;
+
+	return 0;
+}
+
 /**
  * ufshcd_get_pwr_dev_param - get finally agreed attributes for
  *                            power mode change
@@ -371,6 +481,12 @@ int ufshcd_pltfrm_init(struct platform_device *pdev,
 
 	ufshcd_init_lanes_per_dir(hba);
 
+	err = ufshcd_parse_operating_points(hba);
+	if (err) {
+		dev_err(dev, "%s: OPP parse failed %d\n", __func__, err);
+		goto dealloc_host;
+	}
+
 	err = ufshcd_init(hba, mmio_base, irq);
 	if (err) {
 		dev_err(dev, "Initialization failed\n");
-- 
2.25.1

Re: [PATCH 11/14] scsi: ufs: host: Add support for parsing OPP

[Dmitry Baryshkov]

On 12/07/2023 13:32, Manivannan Sadhasivam wrote:
> OPP framework can be used to scale the clocks along with other entities
> such as regulators, performance state etc... So let's add support for
> parsing OPP from devicetree. OPP support in devicetree is added through
> the "operating-points-v2" property which accepts the OPP table defining
> clock frequency, regulator voltage, power domain performance state etc...
> 
> Since the UFS controller requires multiple clocks to be controlled for
> proper working, devm_pm_opp_set_config() has been used which supports
> scaling multiple clocks through custom ufshcd_opp_config_clks() callback.
> 
> It should be noted that the OPP support is not compatible with the old
> "freq-table-hz" property. So only one can be used at a time even though
> the UFS core supports both.
> 
> Co-developed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>   drivers/ufs/host/ufshcd-pltfrm.c | 116 +++++++++++++++++++++++++++++++
>   1 file changed, 116 insertions(+)
> 
> diff --git a/drivers/ufs/host/ufshcd-pltfrm.c b/drivers/ufs/host/ufshcd-pltfrm.c
> index 0b7430033047..068c22378c88 100644
> --- a/drivers/ufs/host/ufshcd-pltfrm.c
> +++ b/drivers/ufs/host/ufshcd-pltfrm.c
> @@ -8,8 +8,10 @@
>    *	Vinayak Holikatti <h.vinayak@samsung.com>
>    */
>   
> +#include <linux/clk.h>
>   #include <linux/module.h>
>   #include <linux/platform_device.h>
> +#include <linux/pm_opp.h>
>   #include <linux/pm_runtime.h>
>   #include <linux/of.h>
>   
> @@ -17,6 +19,8 @@
>   #include "ufshcd-pltfrm.h"
>   #include <ufs/unipro.h>
>   
> +#include <trace/events/ufs.h>
> +
>   #define UFSHCD_DEFAULT_LANES_PER_DIRECTION		2
>   
>   static int ufshcd_parse_clock_info(struct ufs_hba *hba)
> @@ -205,6 +209,112 @@ static void ufshcd_init_lanes_per_dir(struct ufs_hba *hba)
>   	}
>   }
>   
> +static int ufshcd_opp_config_clks(struct device *dev, struct opp_table *opp_table,
> +				  struct dev_pm_opp *opp, void *data,
> +				  bool scaling_down)
> +{
> +	struct ufs_hba *hba = dev_get_drvdata(dev);
> +	struct list_head *head = &hba->clk_list_head;
> +	struct ufs_clk_info *clki;
> +	unsigned long freq;
> +	u8 idx = 0;
> +	int ret;
> +
> +	list_for_each_entry(clki, head, list) {
> +		if (!IS_ERR_OR_NULL(clki->clk)) {
> +			freq = dev_pm_opp_get_freq_indexed(opp, idx++);
> +
> +			/* Do not set rate for clocks having frequency as 0 */
> +			if (!freq)
> +				continue;

Can we omit these clocks from the opp table? I don't think they serve 
any purpose.

Maybe it would even make sense to move this function to drivers/opp 
then, as it will be generic enough.

> +
> +			ret = clk_set_rate(clki->clk, freq);
> +			if (ret) {
> +				dev_err(dev, "%s: %s clk set rate(%ldHz) failed, %d\n",
> +					__func__, clki->name, freq, ret);
> +				return ret;
> +			}
> +
> +			trace_ufshcd_clk_scaling(dev_name(dev),
> +				(scaling_down ? "scaled down" : "scaled up"),
> +				clki->name, hba->clk_scaling.target_freq, freq);
> +		}
> +	}
> +
> +	return 0;
> +} > +
> +static int ufshcd_parse_operating_points(struct ufs_hba *hba)
> +{
> +	struct device *dev = hba->dev;
> +	struct device_node *np = dev->of_node;
> +	struct dev_pm_opp_config config = {};
> +	struct ufs_clk_info *clki;
> +	const char **clk_names;
> +	int cnt, i, ret;
> +
> +	if (!of_find_property(np, "operating-points-v2", NULL))
> +		return 0;
> +
> +	if (of_find_property(np, "freq-table-hz", NULL)) {
> +		dev_err(dev, "%s: operating-points and freq-table-hz are incompatible\n",
> +			 __func__);
> +		return -EINVAL;
> +	}
> +
> +	cnt = of_property_count_strings(np, "clock-names");
> +	if (cnt <= 0) {
> +		dev_err(dev, "%s: Missing clock-names\n",  __func__);
> +		return -ENODEV;
> +	}
> +
> +	/* OPP expects clk_names to be NULL terminated */
> +	clk_names = devm_kcalloc(dev, cnt + 1, sizeof(*clk_names), GFP_KERNEL);
> +	if (!clk_names)
> +		return -ENOMEM;
> +
> +	/*
> +	 * We still need to get reference to all clocks as the UFS core uses
> +	 * them separately.
> +	 */
> +	for (i = 0; i < cnt; i++) {
> +		ret = of_property_read_string_index(np, "clock-names", i,
> +						    &clk_names[i]);
> +		if (ret)
> +			return ret;
> +
> +		clki = devm_kzalloc(dev, sizeof(*clki), GFP_KERNEL);
> +		if (!clki)
> +			return -ENOMEM;
> +
> +		clki->name = devm_kstrdup(dev, clk_names[i], GFP_KERNEL);
> +		if (!clki->name)
> +			return -ENOMEM;
> +
> +		if (!strcmp(clk_names[i], "ref_clk"))
> +			clki->keep_link_active = true;
> +
> +		list_add_tail(&clki->list, &hba->clk_list_head);
> +	}
> +
> +	config.clk_names = clk_names,
> +	config.config_clks = ufshcd_opp_config_clks;
> +
> +	ret = devm_pm_opp_set_config(dev, &config);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_pm_opp_of_add_table(dev);
> +	if (ret) {
> +		dev_err(dev, "Failed to add OPP table: %d\n", ret);
> +		return ret;
> +	}
> +
> +	hba->use_pm_opp = true;
> +
> +	return 0;
> +}
> +
>   /**
>    * ufshcd_get_pwr_dev_param - get finally agreed attributes for
>    *                            power mode change
> @@ -371,6 +481,12 @@ int ufshcd_pltfrm_init(struct platform_device *pdev,
>   
>   	ufshcd_init_lanes_per_dir(hba);
>   
> +	err = ufshcd_parse_operating_points(hba);
> +	if (err) {
> +		dev_err(dev, "%s: OPP parse failed %d\n", __func__, err);
> +		goto dealloc_host;
> +	}
> +
>   	err = ufshcd_init(hba, mmio_base, irq);
>   	if (err) {
>   		dev_err(dev, "Initialization failed\n");

-- 
With best wishes
Dmitry

Re: [PATCH 11/14] scsi: ufs: host: Add support for parsing OPP

[Manivannan Sadhasivam]

On Wed, Jul 12, 2023 at 04:15:12PM +0300, Dmitry Baryshkov wrote:
> On 12/07/2023 13:32, Manivannan Sadhasivam wrote:
> > OPP framework can be used to scale the clocks along with other entities
> > such as regulators, performance state etc... So let's add support for
> > parsing OPP from devicetree. OPP support in devicetree is added through
> > the "operating-points-v2" property which accepts the OPP table defining
> > clock frequency, regulator voltage, power domain performance state etc...
> > 
> > Since the UFS controller requires multiple clocks to be controlled for
> > proper working, devm_pm_opp_set_config() has been used which supports
> > scaling multiple clocks through custom ufshcd_opp_config_clks() callback.
> > 
> > It should be noted that the OPP support is not compatible with the old
> > "freq-table-hz" property. So only one can be used at a time even though
> > the UFS core supports both.
> > 
> > Co-developed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >   drivers/ufs/host/ufshcd-pltfrm.c | 116 +++++++++++++++++++++++++++++++
> >   1 file changed, 116 insertions(+)
> > 
> > diff --git a/drivers/ufs/host/ufshcd-pltfrm.c b/drivers/ufs/host/ufshcd-pltfrm.c
> > index 0b7430033047..068c22378c88 100644
> > --- a/drivers/ufs/host/ufshcd-pltfrm.c
> > +++ b/drivers/ufs/host/ufshcd-pltfrm.c
> > @@ -8,8 +8,10 @@
> >    *	Vinayak Holikatti <h.vinayak@samsung.com>
> >    */
> > +#include <linux/clk.h>
> >   #include <linux/module.h>
> >   #include <linux/platform_device.h>
> > +#include <linux/pm_opp.h>
> >   #include <linux/pm_runtime.h>
> >   #include <linux/of.h>
> > @@ -17,6 +19,8 @@
> >   #include "ufshcd-pltfrm.h"
> >   #include <ufs/unipro.h>
> > +#include <trace/events/ufs.h>
> > +
> >   #define UFSHCD_DEFAULT_LANES_PER_DIRECTION		2
> >   static int ufshcd_parse_clock_info(struct ufs_hba *hba)
> > @@ -205,6 +209,112 @@ static void ufshcd_init_lanes_per_dir(struct ufs_hba *hba)
> >   	}
> >   }
> > +static int ufshcd_opp_config_clks(struct device *dev, struct opp_table *opp_table,
> > +				  struct dev_pm_opp *opp, void *data,
> > +				  bool scaling_down)
> > +{
> > +	struct ufs_hba *hba = dev_get_drvdata(dev);
> > +	struct list_head *head = &hba->clk_list_head;
> > +	struct ufs_clk_info *clki;
> > +	unsigned long freq;
> > +	u8 idx = 0;
> > +	int ret;
> > +
> > +	list_for_each_entry(clki, head, list) {
> > +		if (!IS_ERR_OR_NULL(clki->clk)) {
> > +			freq = dev_pm_opp_get_freq_indexed(opp, idx++);
> > +
> > +			/* Do not set rate for clocks having frequency as 0 */
> > +			if (!freq)
> > +				continue;
> 
> Can we omit these clocks from the opp table? I don't think they serve any
> purpose.
> 

No, we cannot. OPP requires the clocks and opp-hz to be of same length. And we
cannot omit those clocks as well since linux needs to gate control them.

> Maybe it would even make sense to move this function to drivers/opp then, as
> it will be generic enough.
> 

There is already a generic function available in OPP core. But we cannot use it
as we need to skip setting 0 freq and that's not applicable in OPP core as
discussed with Viresh offline.

- Mani

> > +
> > +			ret = clk_set_rate(clki->clk, freq);
> > +			if (ret) {
> > +				dev_err(dev, "%s: %s clk set rate(%ldHz) failed, %d\n",
> > +					__func__, clki->name, freq, ret);
> > +				return ret;
> > +			}
> > +
> > +			trace_ufshcd_clk_scaling(dev_name(dev),
> > +				(scaling_down ? "scaled down" : "scaled up"),
> > +				clki->name, hba->clk_scaling.target_freq, freq);
> > +		}
> > +	}
> > +
> > +	return 0;
> > +} > +
> > +static int ufshcd_parse_operating_points(struct ufs_hba *hba)
> > +{
> > +	struct device *dev = hba->dev;
> > +	struct device_node *np = dev->of_node;
> > +	struct dev_pm_opp_config config = {};
> > +	struct ufs_clk_info *clki;
> > +	const char **clk_names;
> > +	int cnt, i, ret;
> > +
> > +	if (!of_find_property(np, "operating-points-v2", NULL))
> > +		return 0;
> > +
> > +	if (of_find_property(np, "freq-table-hz", NULL)) {
> > +		dev_err(dev, "%s: operating-points and freq-table-hz are incompatible\n",
> > +			 __func__);
> > +		return -EINVAL;
> > +	}
> > +
> > +	cnt = of_property_count_strings(np, "clock-names");
> > +	if (cnt <= 0) {
> > +		dev_err(dev, "%s: Missing clock-names\n",  __func__);
> > +		return -ENODEV;
> > +	}
> > +
> > +	/* OPP expects clk_names to be NULL terminated */
> > +	clk_names = devm_kcalloc(dev, cnt + 1, sizeof(*clk_names), GFP_KERNEL);
> > +	if (!clk_names)
> > +		return -ENOMEM;
> > +
> > +	/*
> > +	 * We still need to get reference to all clocks as the UFS core uses
> > +	 * them separately.
> > +	 */
> > +	for (i = 0; i < cnt; i++) {
> > +		ret = of_property_read_string_index(np, "clock-names", i,
> > +						    &clk_names[i]);
> > +		if (ret)
> > +			return ret;
> > +
> > +		clki = devm_kzalloc(dev, sizeof(*clki), GFP_KERNEL);
> > +		if (!clki)
> > +			return -ENOMEM;
> > +
> > +		clki->name = devm_kstrdup(dev, clk_names[i], GFP_KERNEL);
> > +		if (!clki->name)
> > +			return -ENOMEM;
> > +
> > +		if (!strcmp(clk_names[i], "ref_clk"))
> > +			clki->keep_link_active = true;
> > +
> > +		list_add_tail(&clki->list, &hba->clk_list_head);
> > +	}
> > +
> > +	config.clk_names = clk_names,
> > +	config.config_clks = ufshcd_opp_config_clks;
> > +
> > +	ret = devm_pm_opp_set_config(dev, &config);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = devm_pm_opp_of_add_table(dev);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to add OPP table: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	hba->use_pm_opp = true;
> > +
> > +	return 0;
> > +}
> > +
> >   /**
> >    * ufshcd_get_pwr_dev_param - get finally agreed attributes for
> >    *                            power mode change
> > @@ -371,6 +481,12 @@ int ufshcd_pltfrm_init(struct platform_device *pdev,
> >   	ufshcd_init_lanes_per_dir(hba);
> > +	err = ufshcd_parse_operating_points(hba);
> > +	if (err) {
> > +		dev_err(dev, "%s: OPP parse failed %d\n", __func__, err);
> > +		goto dealloc_host;
> > +	}
> > +
> >   	err = ufshcd_init(hba, mmio_base, irq);
> >   	if (err) {
> >   		dev_err(dev, "Initialization failed\n");
> 
> -- 
> With best wishes
> Dmitry
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 11/14] scsi: ufs: host: Add support for parsing OPP

[Dmitry Baryshkov]

On Wed, 12 Jul 2023 at 19:34, Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> On Wed, Jul 12, 2023 at 04:15:12PM +0300, Dmitry Baryshkov wrote:
> > On 12/07/2023 13:32, Manivannan Sadhasivam wrote:
> > > OPP framework can be used to scale the clocks along with other entities
> > > such as regulators, performance state etc... So let's add support for
> > > parsing OPP from devicetree. OPP support in devicetree is added through
> > > the "operating-points-v2" property which accepts the OPP table defining
> > > clock frequency, regulator voltage, power domain performance state etc...
> > >
> > > Since the UFS controller requires multiple clocks to be controlled for
> > > proper working, devm_pm_opp_set_config() has been used which supports
> > > scaling multiple clocks through custom ufshcd_opp_config_clks() callback.
> > >
> > > It should be noted that the OPP support is not compatible with the old
> > > "freq-table-hz" property. So only one can be used at a time even though
> > > the UFS core supports both.
> > >
> > > Co-developed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > ---
> > >   drivers/ufs/host/ufshcd-pltfrm.c | 116 +++++++++++++++++++++++++++++++
> > >   1 file changed, 116 insertions(+)
> > >
> > > diff --git a/drivers/ufs/host/ufshcd-pltfrm.c b/drivers/ufs/host/ufshcd-pltfrm.c
> > > index 0b7430033047..068c22378c88 100644
> > > --- a/drivers/ufs/host/ufshcd-pltfrm.c
> > > +++ b/drivers/ufs/host/ufshcd-pltfrm.c
> > > @@ -8,8 +8,10 @@
> > >    *        Vinayak Holikatti <h.vinayak@samsung.com>
> > >    */
> > > +#include <linux/clk.h>
> > >   #include <linux/module.h>
> > >   #include <linux/platform_device.h>
> > > +#include <linux/pm_opp.h>
> > >   #include <linux/pm_runtime.h>
> > >   #include <linux/of.h>
> > > @@ -17,6 +19,8 @@
> > >   #include "ufshcd-pltfrm.h"
> > >   #include <ufs/unipro.h>
> > > +#include <trace/events/ufs.h>
> > > +
> > >   #define UFSHCD_DEFAULT_LANES_PER_DIRECTION                2
> > >   static int ufshcd_parse_clock_info(struct ufs_hba *hba)
> > > @@ -205,6 +209,112 @@ static void ufshcd_init_lanes_per_dir(struct ufs_hba *hba)
> > >     }
> > >   }
> > > +static int ufshcd_opp_config_clks(struct device *dev, struct opp_table *opp_table,
> > > +                             struct dev_pm_opp *opp, void *data,
> > > +                             bool scaling_down)
> > > +{
> > > +   struct ufs_hba *hba = dev_get_drvdata(dev);
> > > +   struct list_head *head = &hba->clk_list_head;
> > > +   struct ufs_clk_info *clki;
> > > +   unsigned long freq;
> > > +   u8 idx = 0;
> > > +   int ret;
> > > +
> > > +   list_for_each_entry(clki, head, list) {
> > > +           if (!IS_ERR_OR_NULL(clki->clk)) {
> > > +                   freq = dev_pm_opp_get_freq_indexed(opp, idx++);
> > > +
> > > +                   /* Do not set rate for clocks having frequency as 0 */
> > > +                   if (!freq)
> > > +                           continue;
> >
> > Can we omit these clocks from the opp table? I don't think they serve any
> > purpose.
> >
>
> No, we cannot. OPP requires the clocks and opp-hz to be of same length. And we
> cannot omit those clocks as well since linux needs to gate control them.

Hmm, I thought we push the list of "interesting" clocks through
config->clock_names.

>
> > Maybe it would even make sense to move this function to drivers/opp then, as
> > it will be generic enough.
> >
>
> There is already a generic function available in OPP core. But we cannot use it
> as we need to skip setting 0 freq and that's not applicable in OPP core as
> discussed with Viresh offline.

Ack.

>
> - Mani
>
> > > +
> > > +                   ret = clk_set_rate(clki->clk, freq);
> > > +                   if (ret) {
> > > +                           dev_err(dev, "%s: %s clk set rate(%ldHz) failed, %d\n",
> > > +                                   __func__, clki->name, freq, ret);
> > > +                           return ret;
> > > +                   }
> > > +
> > > +                   trace_ufshcd_clk_scaling(dev_name(dev),
> > > +                           (scaling_down ? "scaled down" : "scaled up"),
> > > +                           clki->name, hba->clk_scaling.target_freq, freq);
> > > +           }
> > > +   }
> > > +
> > > +   return 0;
> > > +} > +
> > > +static int ufshcd_parse_operating_points(struct ufs_hba *hba)
> > > +{
> > > +   struct device *dev = hba->dev;
> > > +   struct device_node *np = dev->of_node;
> > > +   struct dev_pm_opp_config config = {};
> > > +   struct ufs_clk_info *clki;
> > > +   const char **clk_names;
> > > +   int cnt, i, ret;
> > > +
> > > +   if (!of_find_property(np, "operating-points-v2", NULL))
> > > +           return 0;
> > > +
> > > +   if (of_find_property(np, "freq-table-hz", NULL)) {
> > > +           dev_err(dev, "%s: operating-points and freq-table-hz are incompatible\n",
> > > +                    __func__);
> > > +           return -EINVAL;
> > > +   }
> > > +
> > > +   cnt = of_property_count_strings(np, "clock-names");
> > > +   if (cnt <= 0) {
> > > +           dev_err(dev, "%s: Missing clock-names\n",  __func__);
> > > +           return -ENODEV;
> > > +   }
> > > +
> > > +   /* OPP expects clk_names to be NULL terminated */
> > > +   clk_names = devm_kcalloc(dev, cnt + 1, sizeof(*clk_names), GFP_KERNEL);
> > > +   if (!clk_names)
> > > +           return -ENOMEM;
> > > +
> > > +   /*
> > > +    * We still need to get reference to all clocks as the UFS core uses
> > > +    * them separately.
> > > +    */
> > > +   for (i = 0; i < cnt; i++) {
> > > +           ret = of_property_read_string_index(np, "clock-names", i,
> > > +                                               &clk_names[i]);
> > > +           if (ret)
> > > +                   return ret;
> > > +
> > > +           clki = devm_kzalloc(dev, sizeof(*clki), GFP_KERNEL);
> > > +           if (!clki)
> > > +                   return -ENOMEM;
> > > +
> > > +           clki->name = devm_kstrdup(dev, clk_names[i], GFP_KERNEL);
> > > +           if (!clki->name)
> > > +                   return -ENOMEM;
> > > +
> > > +           if (!strcmp(clk_names[i], "ref_clk"))
> > > +                   clki->keep_link_active = true;
> > > +
> > > +           list_add_tail(&clki->list, &hba->clk_list_head);
> > > +   }
> > > +
> > > +   config.clk_names = clk_names,
> > > +   config.config_clks = ufshcd_opp_config_clks;
> > > +
> > > +   ret = devm_pm_opp_set_config(dev, &config);
> > > +   if (ret)
> > > +           return ret;
> > > +
> > > +   ret = devm_pm_opp_of_add_table(dev);
> > > +   if (ret) {
> > > +           dev_err(dev, "Failed to add OPP table: %d\n", ret);
> > > +           return ret;
> > > +   }
> > > +
> > > +   hba->use_pm_opp = true;
> > > +
> > > +   return 0;
> > > +}
> > > +
> > >   /**
> > >    * ufshcd_get_pwr_dev_param - get finally agreed attributes for
> > >    *                            power mode change
> > > @@ -371,6 +481,12 @@ int ufshcd_pltfrm_init(struct platform_device *pdev,
> > >     ufshcd_init_lanes_per_dir(hba);
> > > +   err = ufshcd_parse_operating_points(hba);
> > > +   if (err) {
> > > +           dev_err(dev, "%s: OPP parse failed %d\n", __func__, err);
> > > +           goto dealloc_host;
> > > +   }
> > > +
> > >     err = ufshcd_init(hba, mmio_base, irq);
> > >     if (err) {
> > >             dev_err(dev, "Initialization failed\n");
> >
> > --
> > With best wishes
> > Dmitry
> >
>
> --
> மணிவண்ணன் சதாசிவம்



-- 
With best wishes
Dmitry

Re: [PATCH 11/14] scsi: ufs: host: Add support for parsing OPP

[Viresh Kumar]

On 12-07-23, 19:48, Dmitry Baryshkov wrote:
> On Wed, 12 Jul 2023 at 19:34, Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org> wrote:
> > On Wed, Jul 12, 2023 at 04:15:12PM +0300, Dmitry Baryshkov wrote:
> > > On 12/07/2023 13:32, Manivannan Sadhasivam wrote:

> > > > +static int ufshcd_opp_config_clks(struct device *dev, struct opp_table *opp_table,
> > > > +                             struct dev_pm_opp *opp, void *data,
> > > > +                             bool scaling_down)
> > > > +{
> > > > +   struct ufs_hba *hba = dev_get_drvdata(dev);
> > > > +   struct list_head *head = &hba->clk_list_head;
> > > > +   struct ufs_clk_info *clki;
> > > > +   unsigned long freq;
> > > > +   u8 idx = 0;
> > > > +   int ret;
> > > > +
> > > > +   list_for_each_entry(clki, head, list) {
> > > > +           if (!IS_ERR_OR_NULL(clki->clk)) {
> > > > +                   freq = dev_pm_opp_get_freq_indexed(opp, idx++);
> > > > +
> > > > +                   /* Do not set rate for clocks having frequency as 0 */
> > > > +                   if (!freq)
> > > > +                           continue;
> > >
> > > Can we omit these clocks from the opp table? I don't think they serve any
> > > purpose.
> > >
> >
> > No, we cannot. OPP requires the clocks and opp-hz to be of same length.

I am okay with having a patch for the OPP core to modify this
behavior, as I told privately earlier.

> > And we
> > cannot omit those clocks as well since linux needs to gate control them.
> 
> Hmm, I thought we push the list of "interesting" clocks through
> config->clock_names.

Yes, another way to solve this would be keep the interesting clocks in
the beginning in "clock-names" field and let the platform pass only
those to the OPP core.

> >
> > > Maybe it would even make sense to move this function to drivers/opp then, as
> > > it will be generic enough.
> > >
> >
> > There is already a generic function available in OPP core. But we cannot use it
> > as we need to skip setting 0 freq and that's not applicable in OPP core as
> > discussed with Viresh offline.
> 
> Ack.

I am okay with either of the solutions, it is for you guys to decide
what works better for your platform.

-- 
viresh

Re: [PATCH 11/14] scsi: ufs: host: Add support for parsing OPP

[Manivannan Sadhasivam]

On Thu, Jul 13, 2023 at 09:39:18AM +0530, Viresh Kumar wrote:
> On 12-07-23, 19:48, Dmitry Baryshkov wrote:
> > On Wed, 12 Jul 2023 at 19:34, Manivannan Sadhasivam
> > <manivannan.sadhasivam@linaro.org> wrote:
> > > On Wed, Jul 12, 2023 at 04:15:12PM +0300, Dmitry Baryshkov wrote:
> > > > On 12/07/2023 13:32, Manivannan Sadhasivam wrote:
> 
> > > > > +static int ufshcd_opp_config_clks(struct device *dev, struct opp_table *opp_table,
> > > > > +                             struct dev_pm_opp *opp, void *data,
> > > > > +                             bool scaling_down)
> > > > > +{
> > > > > +   struct ufs_hba *hba = dev_get_drvdata(dev);
> > > > > +   struct list_head *head = &hba->clk_list_head;
> > > > > +   struct ufs_clk_info *clki;
> > > > > +   unsigned long freq;
> > > > > +   u8 idx = 0;
> > > > > +   int ret;
> > > > > +
> > > > > +   list_for_each_entry(clki, head, list) {
> > > > > +           if (!IS_ERR_OR_NULL(clki->clk)) {
> > > > > +                   freq = dev_pm_opp_get_freq_indexed(opp, idx++);
> > > > > +
> > > > > +                   /* Do not set rate for clocks having frequency as 0 */
> > > > > +                   if (!freq)
> > > > > +                           continue;
> > > >
> > > > Can we omit these clocks from the opp table? I don't think they serve any
> > > > purpose.
> > > >
> > >
> > > No, we cannot. OPP requires the clocks and opp-hz to be of same length.
> 
> I am okay with having a patch for the OPP core to modify this
> behavior, as I told privately earlier.
> 
> > > And we
> > > cannot omit those clocks as well since linux needs to gate control them.
> > 
> > Hmm, I thought we push the list of "interesting" clocks through
> > config->clock_names.
> 
> Yes, another way to solve this would be keep the interesting clocks in
> the beginning in "clock-names" field and let the platform pass only
> those to the OPP core.
> 
> > >
> > > > Maybe it would even make sense to move this function to drivers/opp then, as
> > > > it will be generic enough.
> > > >
> > >
> > > There is already a generic function available in OPP core. But we cannot use it
> > > as we need to skip setting 0 freq and that's not applicable in OPP core as
> > > discussed with Viresh offline.
> > 
> > Ack.
> 
> I am okay with either of the solutions, it is for you guys to decide
> what works better for your platform.
> 

We can settle with this custom callback for now. If there are drivers in the
future trying to do the same (skipping 0 freq) then we can generalize.

- Mani

> -- 
> viresh

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 11/14] scsi: ufs: host: Add support for parsing OPP

[Viresh Kumar]

On 13-07-23, 10:35, Manivannan Sadhasivam wrote:
> We can settle with this custom callback for now. If there are drivers in the
> future trying to do the same (skipping 0 freq) then we can generalize.

Just for completeness, there isn't much to generalize here apart from
changing the DT order of clocks. Isn't it ?

The change require for the OPP core makes sense, I will probably just
push it anyway.

-- 
viresh

Re: [PATCH 11/14] scsi: ufs: host: Add support for parsing OPP

[Manivannan Sadhasivam]

On Thu, Jul 13, 2023 at 10:42:35AM +0530, Viresh Kumar wrote:
> On 13-07-23, 10:35, Manivannan Sadhasivam wrote:
> > We can settle with this custom callback for now. If there are drivers in the
> > future trying to do the same (skipping 0 freq) then we can generalize.
> 
> Just for completeness, there isn't much to generalize here apart from
> changing the DT order of clocks. Isn't it ?
> 

Even with changing the order, driver has to know the "interesting" clocks
beforehand. But that varies between platforms (this is a generic driver for
ufshc platforms).

And I do not know if clocks have any dependency between them, atleast not in
Qcom platforms. But not sure about others.

- Mani

> The change require for the OPP core makes sense, I will probably just
> push it anyway.
> 
> -- 
> viresh

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 11/14] scsi: ufs: host: Add support for parsing OPP

[Viresh Kumar]

Okay, sorry about missing one point first. I thought we are adding the
clk config callback (which neglects 0 frequencies) to a Qcom only
driver and so was okay-ish with that. But now that I realize that this
is a generic driver instead (my mistake here), I wonder if it is the
right thing to do anymore.

On 13-07-23, 10:58, Manivannan Sadhasivam wrote:
> On Thu, Jul 13, 2023 at 10:42:35AM +0530, Viresh Kumar wrote:
> > On 13-07-23, 10:35, Manivannan Sadhasivam wrote:
> > > We can settle with this custom callback for now. If there are drivers in the
> > > future trying to do the same (skipping 0 freq) then we can generalize.
> > 
> > Just for completeness, there isn't much to generalize here apart from
> > changing the DT order of clocks. Isn't it ?
> > 
> 
> Even with changing the order, driver has to know the "interesting" clocks
> beforehand. But that varies between platforms (this is a generic driver for
> ufshc platforms).
> 
> And I do not know if clocks have any dependency between them, atleast not in
> Qcom platforms. But not sure about others.

Maybe this requires some sort of callback, per-platform, which gets
you these details or the struct dev_pm_opp_config itself (so platforms
can choose the callback too, in case order is important).

> > The change require for the OPP core makes sense, I will probably just
> > push it anyway.

I tried to look at this code and I think it is doing the right thing
currently, i.e. it matches clk-count with the number of frequencies in
opp-hz, which should turn out to be the same in your case. So nothing
to change here I guess.

-- 
viresh

Re: [PATCH 11/14] scsi: ufs: host: Add support for parsing OPP

[Manivannan Sadhasivam]

On Thu, Jul 13, 2023 at 11:13:02AM +0530, Viresh Kumar wrote:
> Okay, sorry about missing one point first. I thought we are adding the
> clk config callback (which neglects 0 frequencies) to a Qcom only
> driver and so was okay-ish with that. But now that I realize that this
> is a generic driver instead (my mistake here), I wonder if it is the
> right thing to do anymore.
> 

That's the pre-opp behavior as well. Reason is, most of the platforms have only
gate clocks supplied to the ufs controller and cannot change the frequency. Only
Qcom requires changing the frequency of _some_ clocks, so that's why we have to
use this hack of skipping 0 freq clocks.

> On 13-07-23, 10:58, Manivannan Sadhasivam wrote:
> > On Thu, Jul 13, 2023 at 10:42:35AM +0530, Viresh Kumar wrote:
> > > On 13-07-23, 10:35, Manivannan Sadhasivam wrote:
> > > > We can settle with this custom callback for now. If there are drivers in the
> > > > future trying to do the same (skipping 0 freq) then we can generalize.
> > > 
> > > Just for completeness, there isn't much to generalize here apart from
> > > changing the DT order of clocks. Isn't it ?
> > > 
> > 
> > Even with changing the order, driver has to know the "interesting" clocks
> > beforehand. But that varies between platforms (this is a generic driver for
> > ufshc platforms).
> > 
> > And I do not know if clocks have any dependency between them, atleast not in
> > Qcom platforms. But not sure about others.
> 
> Maybe this requires some sort of callback, per-platform, which gets
> you these details or the struct dev_pm_opp_config itself (so platforms
> can choose the callback too, in case order is important).
> 

Yeah but that seems overkill since the current config_clks helper satisfies the
requirement.

- Mani

> > > The change require for the OPP core makes sense, I will probably just
> > > push it anyway.
> 
> I tried to look at this code and I think it is doing the right thing
> currently, i.e. it matches clk-count with the number of frequencies in
> opp-hz, which should turn out to be the same in your case. So nothing
> to change here I guess.
> 
> -- 
> viresh

-- 
மணிவண்ணன் சதாசிவம்

[PATCH 12/14] arm64: dts: qcom: sdm845: Add interconnect paths to UFSHC

[Manivannan Sadhasivam]

UFS host controller requires interconnect path configuration for proper
working. So let's specify them for SDM845 SoC.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 arch/arm64/boot/dts/qcom/sdm845.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index e04a3cbb1017..2ea6eb44953e 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -2607,6 +2607,11 @@ ufs_mem_hc: ufshc@1d84000 {
 				<&gcc GCC_UFS_PHY_ICE_CORE_CLK>;
 
 			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 {
-- 
2.25.1

Re: [PATCH 12/14] arm64: dts: qcom: sdm845: Add interconnect paths to UFSHC

[Konrad Dybcio]

On 12.07.2023 12:32, Manivannan Sadhasivam wrote:
> UFS host controller requires interconnect path configuration for proper
> working. So let's specify them for SDM845 SoC.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
Duplicate of patch 11

Konrad
>  arch/arm64/boot/dts/qcom/sdm845.dtsi | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index e04a3cbb1017..2ea6eb44953e 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -2607,6 +2607,11 @@ ufs_mem_hc: ufshc@1d84000 {
>  				<&gcc GCC_UFS_PHY_ICE_CORE_CLK>;
>  
>  			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 {

[PATCH 12/13] arm64: dts: qcom: sm8250: Add interconnect paths to UFSHC

[Manivannan Sadhasivam]

UFS host controller requires interconnect path configuration for proper
working. So let's specify them for SM8250 SoC.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 arch/arm64/boot/dts/qcom/sm8250.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi
index 72fd66db9c51..7a495ff7512f 100644
--- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
@@ -2197,6 +2197,10 @@ ufs_mem_hc: ufshc@1d84000 {
 
 			operating-points-v2 = <&ufs_opp_table>;
 
+			interconnects = <&aggre1_noc MASTER_UFS_MEM 0 &mc_virt SLAVE_EBI_CH0 0>,
+					<&gem_noc MASTER_AMPSS_M0 0 &config_noc SLAVE_UFS_MEM_CFG 0>;
+			interconnect-names = "ufs-ddr", "cpu-ufs";
+
 			status = "disabled";
 
 			ufs_opp_table: opp-table {
-- 
2.25.1

Re: [PATCH 12/13] arm64: dts: qcom: sm8250: Add interconnect paths to UFSHC

[Konrad Dybcio]

On 12.07.2023 12:32, Manivannan Sadhasivam wrote:
> UFS host controller requires interconnect path configuration for proper
> working. So let's specify them for SM8250 SoC.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad
>  arch/arm64/boot/dts/qcom/sm8250.dtsi | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi
> index 72fd66db9c51..7a495ff7512f 100644
> --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
> @@ -2197,6 +2197,10 @@ ufs_mem_hc: ufshc@1d84000 {
>  
>  			operating-points-v2 = <&ufs_opp_table>;
>  
> +			interconnects = <&aggre1_noc MASTER_UFS_MEM 0 &mc_virt SLAVE_EBI_CH0 0>,
> +					<&gem_noc MASTER_AMPSS_M0 0 &config_noc SLAVE_UFS_MEM_CFG 0>;
> +			interconnect-names = "ufs-ddr", "cpu-ufs";
> +
>  			status = "disabled";
>  
>  			ufs_opp_table: opp-table {

[PATCH 13/14] arm64: dts: qcom: sm8250: Add interconnect paths to UFSHC

[Manivannan Sadhasivam]

UFS host controller requires interconnect path configuration for proper
working. So let's specify them for SM8250 SoC.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 arch/arm64/boot/dts/qcom/sm8250.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi
index 72fd66db9c51..7a495ff7512f 100644
--- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
@@ -2197,6 +2197,10 @@ ufs_mem_hc: ufshc@1d84000 {
 
 			operating-points-v2 = <&ufs_opp_table>;
 
+			interconnects = <&aggre1_noc MASTER_UFS_MEM 0 &mc_virt SLAVE_EBI_CH0 0>,
+					<&gem_noc MASTER_AMPSS_M0 0 &config_noc SLAVE_UFS_MEM_CFG 0>;
+			interconnect-names = "ufs-ddr", "cpu-ufs";
+
 			status = "disabled";
 
 			ufs_opp_table: opp-table {
-- 
2.25.1

Re: [PATCH 13/14] arm64: dts: qcom: sm8250: Add interconnect paths to UFSHC

[Konrad Dybcio]

On 12.07.2023 12:32, Manivannan Sadhasivam wrote:
> UFS host controller requires interconnect path configuration for proper
> working. So let's specify them for SM8250 SoC.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
Duplicate

Konrad
>  arch/arm64/boot/dts/qcom/sm8250.dtsi | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi
> index 72fd66db9c51..7a495ff7512f 100644
> --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
> @@ -2197,6 +2197,10 @@ ufs_mem_hc: ufshc@1d84000 {
>  
>  			operating-points-v2 = <&ufs_opp_table>;
>  
> +			interconnects = <&aggre1_noc MASTER_UFS_MEM 0 &mc_virt SLAVE_EBI_CH0 0>,
> +					<&gem_noc MASTER_AMPSS_M0 0 &config_noc SLAVE_UFS_MEM_CFG 0>;
> +			interconnect-names = "ufs-ddr", "cpu-ufs";
> +
>  			status = "disabled";
>  
>  			ufs_opp_table: opp-table {

[PATCH 13/13] scsi: ufs: qcom: Add support for scaling interconnects

[Manivannan Sadhasivam]

Qcom SoCs require scaling the interconnect paths for proper working of the
peripherals connected through interconnects. Even for accessing the UFS
controller, someone should setup the interconnect paths. So far, the
bootloaders used to setup the interconnect paths before booting linux as
they need to access the UFS storage for things like fetching boot firmware.
But with the advent of multi boot options, bootloader nowadays like in
SA8540p SoC do not setup the interconnect paths at all.

So trying to configure UFS in the absence of the interconnect path
configuration, results in boot crash.

To fix this issue and also to dynamically scale the interconnects (UFS-DDR
and CPU-UFS), interconnect API support is added to the Qcom UFS driver.
With this support, the interconnect paths are scaled dynamically based on
the gear configuration.

During the early stage of ufs_qcom_init(), ufs_qcom_icc_init() will setup
the paths to max bandwidth to allow configuring the UFS registers. Touching
the registers without configuring the icc paths would result in a crash.
However, we don't really need to set max vote for the icc paths as any
minimal vote would suffice. But the max value would allow initialization to
be done faster. After init, the bandwidth will get updated using
ufs_qcom_icc_update_bw() based on the gear and lane configuration.

The bandwidth values defined in ufs_qcom_bw_table struct are taken from
Qcom downstream vendor devicetree source and are calculated as per the
UFS3.1 Spec, Section 6.4.1, HS Gear Rates. So it is fixed across platforms.

Cc: Brian Masney <bmasney@redhat.com>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/ufs/host/ufs-qcom.c | 131 +++++++++++++++++++++++++++++++++++-
 drivers/ufs/host/ufs-qcom.h |   3 +
 2 files changed, 133 insertions(+), 1 deletion(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 8d6fd4c3324f..8a3132d45a65 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -7,6 +7,7 @@
 #include <linux/time.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
+#include <linux/interconnect.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
@@ -46,6 +47,49 @@ enum {
 	TSTBUS_MAX,
 };
 
+#define QCOM_UFS_MAX_GEAR 4
+#define QCOM_UFS_MAX_LANE 2
+
+enum {
+	MODE_MIN,
+	MODE_PWM,
+	MODE_HS_RA,
+	MODE_HS_RB,
+	MODE_MAX,
+};
+
+struct __ufs_qcom_bw_table {
+	u32 bw1;
+	u32 bw2;
+} ufs_qcom_bw_table[MODE_MAX + 1][QCOM_UFS_MAX_GEAR + 1][QCOM_UFS_MAX_LANE + 1] = {
+	[MODE_MIN][0][0] = { 0,		0 },	/* Bandwidth values are in KB/s */
+	[MODE_PWM][1][1] = { 922,	1000 },
+	[MODE_PWM][2][1] = { 1844,	1000 },
+	[MODE_PWM][3][1] = { 3688,	1000 },
+	[MODE_PWM][4][1] = { 7376,	1000 },
+	[MODE_PWM][1][2] = { 1844,	1000 },
+	[MODE_PWM][2][2] = { 3688,	1000 },
+	[MODE_PWM][3][2] = { 7376,	1000 },
+	[MODE_PWM][4][2] = { 14752,	1000 },
+	[MODE_HS_RA][1][1] = { 127796,	1000 },
+	[MODE_HS_RA][2][1] = { 255591,	1000 },
+	[MODE_HS_RA][3][1] = { 1492582,	102400 },
+	[MODE_HS_RA][4][1] = { 2915200,	204800 },
+	[MODE_HS_RA][1][2] = { 255591,	1000 },
+	[MODE_HS_RA][2][2] = { 511181,	1000 },
+	[MODE_HS_RA][3][2] = { 1492582,	204800 },
+	[MODE_HS_RA][4][2] = { 2915200,	409600 },
+	[MODE_HS_RB][1][1] = { 149422,	1000 },
+	[MODE_HS_RB][2][1] = { 298189,	1000 },
+	[MODE_HS_RB][3][1] = { 1492582,	102400 },
+	[MODE_HS_RB][4][1] = { 2915200,	204800 },
+	[MODE_HS_RB][1][2] = { 298189,	1000 },
+	[MODE_HS_RB][2][2] = { 596378,	1000 },
+	[MODE_HS_RB][3][2] = { 1492582,	204800 },
+	[MODE_HS_RB][4][2] = { 2915200,	409600 },
+	[MODE_MAX][0][0] = { 7643136, 307200 },
+};
+
 static struct ufs_qcom_host *ufs_qcom_hosts[MAX_UFS_QCOM_HOSTS];
 
 static void ufs_qcom_get_default_testbus_cfg(struct ufs_qcom_host *host);
@@ -789,6 +833,51 @@ static void ufs_qcom_dev_ref_clk_ctrl(struct ufs_qcom_host *host, bool enable)
 	}
 }
 
+static int ufs_qcom_icc_set_bw(struct ufs_qcom_host *host, u32 bw1, u32 bw2)
+{
+	struct device *dev = host->hba->dev;
+	int ret;
+
+	ret = icc_set_bw(host->icc_ddr, 0, bw1);
+	if (ret < 0) {
+		dev_err(dev, "failed to set bandwidth request: %d\n", ret);
+		return ret;
+	}
+
+	ret = icc_set_bw(host->icc_cpu, 0, bw2);
+	if (ret < 0) {
+		dev_err(dev, "failed to set bandwidth request: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static struct __ufs_qcom_bw_table ufs_qcom_get_bw_table(struct ufs_qcom_host *host)
+{
+	struct ufs_pa_layer_attr *p = &host->dev_req_params;
+	int gear = max_t(u32, p->gear_rx, p->gear_tx);
+	int lane = max_t(u32, p->lane_rx, p->lane_tx);
+
+	if (ufshcd_is_hs_mode(p)) {
+		if (p->hs_rate == PA_HS_MODE_B)
+			return ufs_qcom_bw_table[MODE_HS_RB][gear][lane];
+		else
+			return ufs_qcom_bw_table[MODE_HS_RA][gear][lane];
+	} else {
+		return ufs_qcom_bw_table[MODE_PWM][gear][lane];
+	}
+}
+
+static int ufs_qcom_icc_update_bw(struct ufs_qcom_host *host)
+{
+	struct __ufs_qcom_bw_table bw_table;
+
+	bw_table = ufs_qcom_get_bw_table(host);
+
+	return ufs_qcom_icc_set_bw(host, bw_table.bw1, bw_table.bw2);
+}
+
 static int ufs_qcom_pwr_change_notify(struct ufs_hba *hba,
 				enum ufs_notify_change_status status,
 				struct ufs_pa_layer_attr *dev_max_params,
@@ -852,6 +941,8 @@ static int ufs_qcom_pwr_change_notify(struct ufs_hba *hba,
 		memcpy(&host->dev_req_params,
 				dev_req_params, sizeof(*dev_req_params));
 
+		ufs_qcom_icc_update_bw(host);
+
 		/* disable the device ref clock if entered PWM mode */
 		if (ufshcd_is_hs_mode(&hba->pwr_info) &&
 			!ufshcd_is_hs_mode(dev_req_params))
@@ -981,7 +1072,9 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
 
 	switch (status) {
 	case PRE_CHANGE:
-		if (!on) {
+		if (on) {
+			ufs_qcom_icc_update_bw(host);
+		} else {
 			if (!ufs_qcom_is_link_active(hba)) {
 				/* disable device ref_clk */
 				ufs_qcom_dev_ref_clk_ctrl(host, false);
@@ -993,6 +1086,9 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
 			/* enable the device ref clock for HS mode*/
 			if (ufshcd_is_hs_mode(&hba->pwr_info))
 				ufs_qcom_dev_ref_clk_ctrl(host, true);
+		} else {
+			ufs_qcom_icc_set_bw(host, ufs_qcom_bw_table[MODE_MIN][0][0].bw1,
+					    ufs_qcom_bw_table[MODE_MIN][0][0].bw2);
 		}
 		break;
 	}
@@ -1031,6 +1127,34 @@ static const struct reset_control_ops ufs_qcom_reset_ops = {
 	.deassert = ufs_qcom_reset_deassert,
 };
 
+static int ufs_qcom_icc_init(struct ufs_qcom_host *host)
+{
+	struct device *dev = host->hba->dev;
+	int ret;
+
+	host->icc_ddr = devm_of_icc_get(dev, "ufs-ddr");
+	if (IS_ERR(host->icc_ddr))
+		return dev_err_probe(dev, PTR_ERR(host->icc_ddr),
+				    "failed to acquire interconnect path\n");
+
+	host->icc_cpu = devm_of_icc_get(dev, "cpu-ufs");
+	if (IS_ERR(host->icc_cpu))
+		return dev_err_probe(dev, PTR_ERR(host->icc_cpu),
+				    "failed to acquire interconnect path\n");
+
+	/*
+	 * Set Maximum bandwidth vote before initializing the UFS controller and
+	 * device. Ideally, a minimal interconnect vote would suffice for the
+	 * initialization, but a max vote would allow faster initialization.
+	 */
+	ret = ufs_qcom_icc_set_bw(host, ufs_qcom_bw_table[MODE_MAX][0][0].bw1,
+				  ufs_qcom_bw_table[MODE_MAX][0][0].bw2);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "failed to set bandwidth request\n");
+
+	return 0;
+}
+
 /**
  * ufs_qcom_init - bind phy with controller
  * @hba: host controller instance
@@ -1085,6 +1209,10 @@ static int ufs_qcom_init(struct ufs_hba *hba)
 		}
 	}
 
+	err = ufs_qcom_icc_init(host);
+	if (err)
+		goto out_variant_clear;
+
 	host->device_reset = devm_gpiod_get_optional(dev, "reset",
 						     GPIOD_OUT_HIGH);
 	if (IS_ERR(host->device_reset)) {
@@ -1282,6 +1410,7 @@ static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba,
 				    dev_req_params->pwr_rx,
 				    dev_req_params->hs_rate,
 				    false);
+		ufs_qcom_icc_update_bw(host);
 		ufshcd_uic_hibern8_exit(hba);
 	}
 
diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
index 6289ad5a42d0..dc27395ecba1 100644
--- a/drivers/ufs/host/ufs-qcom.h
+++ b/drivers/ufs/host/ufs-qcom.h
@@ -206,6 +206,9 @@ struct ufs_qcom_host {
 	struct clk *tx_l1_sync_clk;
 	bool is_lane_clks_enabled;
 
+	struct icc_path *icc_ddr;
+	struct icc_path *icc_cpu;
+
 #ifdef CONFIG_SCSI_UFS_CRYPTO
 	struct qcom_ice *ice;
 #endif
-- 
2.25.1

[PATCH 14/14] scsi: ufs: qcom: Add support for scaling interconnects

[Manivannan Sadhasivam]

Qcom SoCs require scaling the interconnect paths for proper working of the
peripherals connected through interconnects. Even for accessing the UFS
controller, someone should setup the interconnect paths. So far, the
bootloaders used to setup the interconnect paths before booting linux as
they need to access the UFS storage for things like fetching boot firmware.
But with the advent of multi boot options, bootloader nowadays like in
SA8540p SoC do not setup the interconnect paths at all.

So trying to configure UFS in the absence of the interconnect path
configuration, results in boot crash.

To fix this issue and also to dynamically scale the interconnects (UFS-DDR
and CPU-UFS), interconnect API support is added to the Qcom UFS driver.
With this support, the interconnect paths are scaled dynamically based on
the gear configuration.

During the early stage of ufs_qcom_init(), ufs_qcom_icc_init() will setup
the paths to max bandwidth to allow configuring the UFS registers. Touching
the registers without configuring the icc paths would result in a crash.
However, we don't really need to set max vote for the icc paths as any
minimal vote would suffice. But the max value would allow initialization to
be done faster. After init, the bandwidth will get updated using
ufs_qcom_icc_update_bw() based on the gear and lane configuration.

The bandwidth values defined in ufs_qcom_bw_table struct are taken from
Qcom downstream vendor devicetree source and are calculated as per the
UFS3.1 Spec, Section 6.4.1, HS Gear Rates. So it is fixed across platforms.

Cc: Brian Masney <bmasney@redhat.com>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/ufs/host/ufs-qcom.c | 131 +++++++++++++++++++++++++++++++++++-
 drivers/ufs/host/ufs-qcom.h |   3 +
 2 files changed, 133 insertions(+), 1 deletion(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 8d6fd4c3324f..8a3132d45a65 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -7,6 +7,7 @@
 #include <linux/time.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
+#include <linux/interconnect.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
@@ -46,6 +47,49 @@ enum {
 	TSTBUS_MAX,
 };
 
+#define QCOM_UFS_MAX_GEAR 4
+#define QCOM_UFS_MAX_LANE 2
+
+enum {
+	MODE_MIN,
+	MODE_PWM,
+	MODE_HS_RA,
+	MODE_HS_RB,
+	MODE_MAX,
+};
+
+struct __ufs_qcom_bw_table {
+	u32 bw1;
+	u32 bw2;
+} ufs_qcom_bw_table[MODE_MAX + 1][QCOM_UFS_MAX_GEAR + 1][QCOM_UFS_MAX_LANE + 1] = {
+	[MODE_MIN][0][0] = { 0,		0 },	/* Bandwidth values are in KB/s */
+	[MODE_PWM][1][1] = { 922,	1000 },
+	[MODE_PWM][2][1] = { 1844,	1000 },
+	[MODE_PWM][3][1] = { 3688,	1000 },
+	[MODE_PWM][4][1] = { 7376,	1000 },
+	[MODE_PWM][1][2] = { 1844,	1000 },
+	[MODE_PWM][2][2] = { 3688,	1000 },
+	[MODE_PWM][3][2] = { 7376,	1000 },
+	[MODE_PWM][4][2] = { 14752,	1000 },
+	[MODE_HS_RA][1][1] = { 127796,	1000 },
+	[MODE_HS_RA][2][1] = { 255591,	1000 },
+	[MODE_HS_RA][3][1] = { 1492582,	102400 },
+	[MODE_HS_RA][4][1] = { 2915200,	204800 },
+	[MODE_HS_RA][1][2] = { 255591,	1000 },
+	[MODE_HS_RA][2][2] = { 511181,	1000 },
+	[MODE_HS_RA][3][2] = { 1492582,	204800 },
+	[MODE_HS_RA][4][2] = { 2915200,	409600 },
+	[MODE_HS_RB][1][1] = { 149422,	1000 },
+	[MODE_HS_RB][2][1] = { 298189,	1000 },
+	[MODE_HS_RB][3][1] = { 1492582,	102400 },
+	[MODE_HS_RB][4][1] = { 2915200,	204800 },
+	[MODE_HS_RB][1][2] = { 298189,	1000 },
+	[MODE_HS_RB][2][2] = { 596378,	1000 },
+	[MODE_HS_RB][3][2] = { 1492582,	204800 },
+	[MODE_HS_RB][4][2] = { 2915200,	409600 },
+	[MODE_MAX][0][0] = { 7643136, 307200 },
+};
+
 static struct ufs_qcom_host *ufs_qcom_hosts[MAX_UFS_QCOM_HOSTS];
 
 static void ufs_qcom_get_default_testbus_cfg(struct ufs_qcom_host *host);
@@ -789,6 +833,51 @@ static void ufs_qcom_dev_ref_clk_ctrl(struct ufs_qcom_host *host, bool enable)
 	}
 }
 
+static int ufs_qcom_icc_set_bw(struct ufs_qcom_host *host, u32 bw1, u32 bw2)
+{
+	struct device *dev = host->hba->dev;
+	int ret;
+
+	ret = icc_set_bw(host->icc_ddr, 0, bw1);
+	if (ret < 0) {
+		dev_err(dev, "failed to set bandwidth request: %d\n", ret);
+		return ret;
+	}
+
+	ret = icc_set_bw(host->icc_cpu, 0, bw2);
+	if (ret < 0) {
+		dev_err(dev, "failed to set bandwidth request: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static struct __ufs_qcom_bw_table ufs_qcom_get_bw_table(struct ufs_qcom_host *host)
+{
+	struct ufs_pa_layer_attr *p = &host->dev_req_params;
+	int gear = max_t(u32, p->gear_rx, p->gear_tx);
+	int lane = max_t(u32, p->lane_rx, p->lane_tx);
+
+	if (ufshcd_is_hs_mode(p)) {
+		if (p->hs_rate == PA_HS_MODE_B)
+			return ufs_qcom_bw_table[MODE_HS_RB][gear][lane];
+		else
+			return ufs_qcom_bw_table[MODE_HS_RA][gear][lane];
+	} else {
+		return ufs_qcom_bw_table[MODE_PWM][gear][lane];
+	}
+}
+
+static int ufs_qcom_icc_update_bw(struct ufs_qcom_host *host)
+{
+	struct __ufs_qcom_bw_table bw_table;
+
+	bw_table = ufs_qcom_get_bw_table(host);
+
+	return ufs_qcom_icc_set_bw(host, bw_table.bw1, bw_table.bw2);
+}
+
 static int ufs_qcom_pwr_change_notify(struct ufs_hba *hba,
 				enum ufs_notify_change_status status,
 				struct ufs_pa_layer_attr *dev_max_params,
@@ -852,6 +941,8 @@ static int ufs_qcom_pwr_change_notify(struct ufs_hba *hba,
 		memcpy(&host->dev_req_params,
 				dev_req_params, sizeof(*dev_req_params));
 
+		ufs_qcom_icc_update_bw(host);
+
 		/* disable the device ref clock if entered PWM mode */
 		if (ufshcd_is_hs_mode(&hba->pwr_info) &&
 			!ufshcd_is_hs_mode(dev_req_params))
@@ -981,7 +1072,9 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
 
 	switch (status) {
 	case PRE_CHANGE:
-		if (!on) {
+		if (on) {
+			ufs_qcom_icc_update_bw(host);
+		} else {
 			if (!ufs_qcom_is_link_active(hba)) {
 				/* disable device ref_clk */
 				ufs_qcom_dev_ref_clk_ctrl(host, false);
@@ -993,6 +1086,9 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
 			/* enable the device ref clock for HS mode*/
 			if (ufshcd_is_hs_mode(&hba->pwr_info))
 				ufs_qcom_dev_ref_clk_ctrl(host, true);
+		} else {
+			ufs_qcom_icc_set_bw(host, ufs_qcom_bw_table[MODE_MIN][0][0].bw1,
+					    ufs_qcom_bw_table[MODE_MIN][0][0].bw2);
 		}
 		break;
 	}
@@ -1031,6 +1127,34 @@ static const struct reset_control_ops ufs_qcom_reset_ops = {
 	.deassert = ufs_qcom_reset_deassert,
 };
 
+static int ufs_qcom_icc_init(struct ufs_qcom_host *host)
+{
+	struct device *dev = host->hba->dev;
+	int ret;
+
+	host->icc_ddr = devm_of_icc_get(dev, "ufs-ddr");
+	if (IS_ERR(host->icc_ddr))
+		return dev_err_probe(dev, PTR_ERR(host->icc_ddr),
+				    "failed to acquire interconnect path\n");
+
+	host->icc_cpu = devm_of_icc_get(dev, "cpu-ufs");
+	if (IS_ERR(host->icc_cpu))
+		return dev_err_probe(dev, PTR_ERR(host->icc_cpu),
+				    "failed to acquire interconnect path\n");
+
+	/*
+	 * Set Maximum bandwidth vote before initializing the UFS controller and
+	 * device. Ideally, a minimal interconnect vote would suffice for the
+	 * initialization, but a max vote would allow faster initialization.
+	 */
+	ret = ufs_qcom_icc_set_bw(host, ufs_qcom_bw_table[MODE_MAX][0][0].bw1,
+				  ufs_qcom_bw_table[MODE_MAX][0][0].bw2);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "failed to set bandwidth request\n");
+
+	return 0;
+}
+
 /**
  * ufs_qcom_init - bind phy with controller
  * @hba: host controller instance
@@ -1085,6 +1209,10 @@ static int ufs_qcom_init(struct ufs_hba *hba)
 		}
 	}
 
+	err = ufs_qcom_icc_init(host);
+	if (err)
+		goto out_variant_clear;
+
 	host->device_reset = devm_gpiod_get_optional(dev, "reset",
 						     GPIOD_OUT_HIGH);
 	if (IS_ERR(host->device_reset)) {
@@ -1282,6 +1410,7 @@ static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba,
 				    dev_req_params->pwr_rx,
 				    dev_req_params->hs_rate,
 				    false);
+		ufs_qcom_icc_update_bw(host);
 		ufshcd_uic_hibern8_exit(hba);
 	}
 
diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
index 6289ad5a42d0..dc27395ecba1 100644
--- a/drivers/ufs/host/ufs-qcom.h
+++ b/drivers/ufs/host/ufs-qcom.h
@@ -206,6 +206,9 @@ struct ufs_qcom_host {
 	struct clk *tx_l1_sync_clk;
 	bool is_lane_clks_enabled;
 
+	struct icc_path *icc_ddr;
+	struct icc_path *icc_cpu;
+
 #ifdef CONFIG_SCSI_UFS_CRYPTO
 	struct qcom_ice *ice;
 #endif
-- 
2.25.1

Re: [PATCH 14/14] scsi: ufs: qcom: Add support for scaling interconnects

[Dmitry Baryshkov]

On 12/07/2023 13:32, Manivannan Sadhasivam wrote:
> Qcom SoCs require scaling the interconnect paths for proper working of the
> peripherals connected through interconnects. Even for accessing the UFS
> controller, someone should setup the interconnect paths. So far, the
> bootloaders used to setup the interconnect paths before booting linux as
> they need to access the UFS storage for things like fetching boot firmware.
> But with the advent of multi boot options, bootloader nowadays like in
> SA8540p SoC do not setup the interconnect paths at all.
> 
> So trying to configure UFS in the absence of the interconnect path
> configuration, results in boot crash.
> 
> To fix this issue and also to dynamically scale the interconnects (UFS-DDR
> and CPU-UFS), interconnect API support is added to the Qcom UFS driver.
> With this support, the interconnect paths are scaled dynamically based on
> the gear configuration.
> 
> During the early stage of ufs_qcom_init(), ufs_qcom_icc_init() will setup
> the paths to max bandwidth to allow configuring the UFS registers. Touching
> the registers without configuring the icc paths would result in a crash.
> However, we don't really need to set max vote for the icc paths as any
> minimal vote would suffice. But the max value would allow initialization to
> be done faster. After init, the bandwidth will get updated using
> ufs_qcom_icc_update_bw() based on the gear and lane configuration.
> 
> The bandwidth values defined in ufs_qcom_bw_table struct are taken from
> Qcom downstream vendor devicetree source and are calculated as per the
> UFS3.1 Spec, Section 6.4.1, HS Gear Rates. So it is fixed across platforms.
> 
> Cc: Brian Masney <bmasney@redhat.com>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>   drivers/ufs/host/ufs-qcom.c | 131 +++++++++++++++++++++++++++++++++++-
>   drivers/ufs/host/ufs-qcom.h |   3 +
>   2 files changed, 133 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index 8d6fd4c3324f..8a3132d45a65 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -7,6 +7,7 @@
>   #include <linux/time.h>
>   #include <linux/clk.h>
>   #include <linux/delay.h>
> +#include <linux/interconnect.h>
>   #include <linux/module.h>
>   #include <linux/of.h>
>   #include <linux/platform_device.h>
> @@ -46,6 +47,49 @@ enum {
>   	TSTBUS_MAX,
>   };
>   
> +#define QCOM_UFS_MAX_GEAR 4
> +#define QCOM_UFS_MAX_LANE 2
> +
> +enum {
> +	MODE_MIN,
> +	MODE_PWM,
> +	MODE_HS_RA,
> +	MODE_HS_RB,
> +	MODE_MAX,

MODE_MIN and MODE_MAX seem to be unused

> +};
> +
> +struct __ufs_qcom_bw_table {
> +	u32 bw1;
> +	u32 bw2;

Please consider:

s/bw1/mem_bw/
s/bw2/cfg_bw/

> +} ufs_qcom_bw_table[MODE_MAX + 1][QCOM_UFS_MAX_GEAR + 1][QCOM_UFS_MAX_LANE + 1] = {

I'd say, these +1's are slightly confusing and unnecessary.

> +	[MODE_MIN][0][0] = { 0,		0 },	/* Bandwidth values are in KB/s */

I'd say, this becomes impossible to check. Can you please structure it?
Either by inclusion:

[MODE_PWM] = {
   [1] = {
      [1] = { .. },
      [2] = { .. },
   },
   // etc.
};

Also, do we have defines for gears? Can we use them instead of indices?

> +	[MODE_PWM][1][1] = { 922,	1000 },
> +	[MODE_PWM][2][1] = { 1844,	1000 },
> +	[MODE_PWM][3][1] = { 3688,	1000 },
> +	[MODE_PWM][4][1] = { 7376,	1000 },
> +	[MODE_PWM][1][2] = { 1844,	1000 },
> +	[MODE_PWM][2][2] = { 3688,	1000 },
> +	[MODE_PWM][3][2] = { 7376,	1000 },
> +	[MODE_PWM][4][2] = { 14752,	1000 },
> +	[MODE_HS_RA][1][1] = { 127796,	1000 },
> +	[MODE_HS_RA][2][1] = { 255591,	1000 },
> +	[MODE_HS_RA][3][1] = { 1492582,	102400 },
> +	[MODE_HS_RA][4][1] = { 2915200,	204800 },
> +	[MODE_HS_RA][1][2] = { 255591,	1000 },
> +	[MODE_HS_RA][2][2] = { 511181,	1000 },
> +	[MODE_HS_RA][3][2] = { 1492582,	204800 },
> +	[MODE_HS_RA][4][2] = { 2915200,	409600 },
> +	[MODE_HS_RB][1][1] = { 149422,	1000 },
> +	[MODE_HS_RB][2][1] = { 298189,	1000 },
> +	[MODE_HS_RB][3][1] = { 1492582,	102400 },
> +	[MODE_HS_RB][4][1] = { 2915200,	204800 },
> +	[MODE_HS_RB][1][2] = { 298189,	1000 },
> +	[MODE_HS_RB][2][2] = { 596378,	1000 },
> +	[MODE_HS_RB][3][2] = { 1492582,	204800 },
> +	[MODE_HS_RB][4][2] = { 2915200,	409600 },
> +	[MODE_MAX][0][0] = { 7643136, 307200 },
> +};
> +
>   static struct ufs_qcom_host *ufs_qcom_hosts[MAX_UFS_QCOM_HOSTS];
>   
>   static void ufs_qcom_get_default_testbus_cfg(struct ufs_qcom_host *host);
> @@ -789,6 +833,51 @@ static void ufs_qcom_dev_ref_clk_ctrl(struct ufs_qcom_host *host, bool enable)
>   	}
>   }
>   
> +static int ufs_qcom_icc_set_bw(struct ufs_qcom_host *host, u32 bw1, u32 bw2)
> +{
> +	struct device *dev = host->hba->dev;
> +	int ret;
> +
> +	ret = icc_set_bw(host->icc_ddr, 0, bw1);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to set bandwidth request: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = icc_set_bw(host->icc_cpu, 0, bw2);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to set bandwidth request: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct __ufs_qcom_bw_table ufs_qcom_get_bw_table(struct ufs_qcom_host *host)
> +{
> +	struct ufs_pa_layer_attr *p = &host->dev_req_params;
> +	int gear = max_t(u32, p->gear_rx, p->gear_tx);
> +	int lane = max_t(u32, p->lane_rx, p->lane_tx);
> +
> +	if (ufshcd_is_hs_mode(p)) {
> +		if (p->hs_rate == PA_HS_MODE_B)
> +			return ufs_qcom_bw_table[MODE_HS_RB][gear][lane];
> +		else
> +			return ufs_qcom_bw_table[MODE_HS_RA][gear][lane];
> +	} else {
> +		return ufs_qcom_bw_table[MODE_PWM][gear][lane];
> +	}
> +}
> +
> +static int ufs_qcom_icc_update_bw(struct ufs_qcom_host *host)
> +{
> +	struct __ufs_qcom_bw_table bw_table;
> +
> +	bw_table = ufs_qcom_get_bw_table(host);
> +
> +	return ufs_qcom_icc_set_bw(host, bw_table.bw1, bw_table.bw2);
> +}
> +
>   static int ufs_qcom_pwr_change_notify(struct ufs_hba *hba,
>   				enum ufs_notify_change_status status,
>   				struct ufs_pa_layer_attr *dev_max_params,
> @@ -852,6 +941,8 @@ static int ufs_qcom_pwr_change_notify(struct ufs_hba *hba,
>   		memcpy(&host->dev_req_params,
>   				dev_req_params, sizeof(*dev_req_params));
>   
> +		ufs_qcom_icc_update_bw(host);
> +
>   		/* disable the device ref clock if entered PWM mode */
>   		if (ufshcd_is_hs_mode(&hba->pwr_info) &&
>   			!ufshcd_is_hs_mode(dev_req_params))
> @@ -981,7 +1072,9 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
>   
>   	switch (status) {
>   	case PRE_CHANGE:
> -		if (!on) {
> +		if (on) {
> +			ufs_qcom_icc_update_bw(host);
> +		} else {
>   			if (!ufs_qcom_is_link_active(hba)) {
>   				/* disable device ref_clk */
>   				ufs_qcom_dev_ref_clk_ctrl(host, false);
> @@ -993,6 +1086,9 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
>   			/* enable the device ref clock for HS mode*/
>   			if (ufshcd_is_hs_mode(&hba->pwr_info))
>   				ufs_qcom_dev_ref_clk_ctrl(host, true);
> +		} else {
> +			ufs_qcom_icc_set_bw(host, ufs_qcom_bw_table[MODE_MIN][0][0].bw1,
> +					    ufs_qcom_bw_table[MODE_MIN][0][0].bw2);
>   		}
>   		break;
>   	}
> @@ -1031,6 +1127,34 @@ static const struct reset_control_ops ufs_qcom_reset_ops = {
>   	.deassert = ufs_qcom_reset_deassert,
>   };
>   
> +static int ufs_qcom_icc_init(struct ufs_qcom_host *host)
> +{
> +	struct device *dev = host->hba->dev;
> +	int ret;
> +
> +	host->icc_ddr = devm_of_icc_get(dev, "ufs-ddr");
> +	if (IS_ERR(host->icc_ddr))
> +		return dev_err_probe(dev, PTR_ERR(host->icc_ddr),
> +				    "failed to acquire interconnect path\n");
> +
> +	host->icc_cpu = devm_of_icc_get(dev, "cpu-ufs");
> +	if (IS_ERR(host->icc_cpu))
> +		return dev_err_probe(dev, PTR_ERR(host->icc_cpu),
> +				    "failed to acquire interconnect path\n");
> +
> +	/*
> +	 * Set Maximum bandwidth vote before initializing the UFS controller and
> +	 * device. Ideally, a minimal interconnect vote would suffice for the
> +	 * initialization, but a max vote would allow faster initialization.
> +	 */
> +	ret = ufs_qcom_icc_set_bw(host, ufs_qcom_bw_table[MODE_MAX][0][0].bw1,
> +				  ufs_qcom_bw_table[MODE_MAX][0][0].bw2);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "failed to set bandwidth request\n");
> +
> +	return 0;
> +}
> +
>   /**
>    * ufs_qcom_init - bind phy with controller
>    * @hba: host controller instance
> @@ -1085,6 +1209,10 @@ static int ufs_qcom_init(struct ufs_hba *hba)
>   		}
>   	}
>   
> +	err = ufs_qcom_icc_init(host);
> +	if (err)
> +		goto out_variant_clear;
> +
>   	host->device_reset = devm_gpiod_get_optional(dev, "reset",
>   						     GPIOD_OUT_HIGH);
>   	if (IS_ERR(host->device_reset)) {
> @@ -1282,6 +1410,7 @@ static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba,
>   				    dev_req_params->pwr_rx,
>   				    dev_req_params->hs_rate,
>   				    false);
> +		ufs_qcom_icc_update_bw(host);
>   		ufshcd_uic_hibern8_exit(hba);
>   	}
>   
> diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
> index 6289ad5a42d0..dc27395ecba1 100644
> --- a/drivers/ufs/host/ufs-qcom.h
> +++ b/drivers/ufs/host/ufs-qcom.h
> @@ -206,6 +206,9 @@ struct ufs_qcom_host {
>   	struct clk *tx_l1_sync_clk;
>   	bool is_lane_clks_enabled;
>   
> +	struct icc_path *icc_ddr;
> +	struct icc_path *icc_cpu;
> +
>   #ifdef CONFIG_SCSI_UFS_CRYPTO
>   	struct qcom_ice *ice;
>   #endif

-- 
With best wishes
Dmitry

Re: [PATCH 14/14] scsi: ufs: qcom: Add support for scaling interconnects

[Manivannan Sadhasivam]

On Wed, Jul 12, 2023 at 04:22:51PM +0300, Dmitry Baryshkov wrote:
> On 12/07/2023 13:32, Manivannan Sadhasivam wrote:
> > Qcom SoCs require scaling the interconnect paths for proper working of the
> > peripherals connected through interconnects. Even for accessing the UFS
> > controller, someone should setup the interconnect paths. So far, the
> > bootloaders used to setup the interconnect paths before booting linux as
> > they need to access the UFS storage for things like fetching boot firmware.
> > But with the advent of multi boot options, bootloader nowadays like in
> > SA8540p SoC do not setup the interconnect paths at all.
> > 
> > So trying to configure UFS in the absence of the interconnect path
> > configuration, results in boot crash.
> > 
> > To fix this issue and also to dynamically scale the interconnects (UFS-DDR
> > and CPU-UFS), interconnect API support is added to the Qcom UFS driver.
> > With this support, the interconnect paths are scaled dynamically based on
> > the gear configuration.
> > 
> > During the early stage of ufs_qcom_init(), ufs_qcom_icc_init() will setup
> > the paths to max bandwidth to allow configuring the UFS registers. Touching
> > the registers without configuring the icc paths would result in a crash.
> > However, we don't really need to set max vote for the icc paths as any
> > minimal vote would suffice. But the max value would allow initialization to
> > be done faster. After init, the bandwidth will get updated using
> > ufs_qcom_icc_update_bw() based on the gear and lane configuration.
> > 
> > The bandwidth values defined in ufs_qcom_bw_table struct are taken from
> > Qcom downstream vendor devicetree source and are calculated as per the
> > UFS3.1 Spec, Section 6.4.1, HS Gear Rates. So it is fixed across platforms.
> > 
> > Cc: Brian Masney <bmasney@redhat.com>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >   drivers/ufs/host/ufs-qcom.c | 131 +++++++++++++++++++++++++++++++++++-
> >   drivers/ufs/host/ufs-qcom.h |   3 +
> >   2 files changed, 133 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> > index 8d6fd4c3324f..8a3132d45a65 100644
> > --- a/drivers/ufs/host/ufs-qcom.c
> > +++ b/drivers/ufs/host/ufs-qcom.c
> > @@ -7,6 +7,7 @@
> >   #include <linux/time.h>
> >   #include <linux/clk.h>
> >   #include <linux/delay.h>
> > +#include <linux/interconnect.h>
> >   #include <linux/module.h>
> >   #include <linux/of.h>
> >   #include <linux/platform_device.h>
> > @@ -46,6 +47,49 @@ enum {
> >   	TSTBUS_MAX,
> >   };
> > +#define QCOM_UFS_MAX_GEAR 4
> > +#define QCOM_UFS_MAX_LANE 2
> > +
> > +enum {
> > +	MODE_MIN,
> > +	MODE_PWM,
> > +	MODE_HS_RA,
> > +	MODE_HS_RB,
> > +	MODE_MAX,
> 
> MODE_MIN and MODE_MAX seem to be unused
> 

No, they are used in the driver.

> > +};
> > +
> > +struct __ufs_qcom_bw_table {
> > +	u32 bw1;
> > +	u32 bw2;
> 
> Please consider:
> 
> s/bw1/mem_bw/
> s/bw2/cfg_bw/
> 

Ok.

> > +} ufs_qcom_bw_table[MODE_MAX + 1][QCOM_UFS_MAX_GEAR + 1][QCOM_UFS_MAX_LANE + 1] = {
> 
> I'd say, these +1's are slightly confusing and unnecessary.
> 

These max values itself are used in the table. Without adding 1 to them, we will
end up with out of bounds access.

> > +	[MODE_MIN][0][0] = { 0,		0 },	/* Bandwidth values are in KB/s */
> 
> I'd say, this becomes impossible to check. Can you please structure it?

What do you mean by "impossible to check"?

> Either by inclusion:
> 
> [MODE_PWM] = {
>   [1] = {
>      [1] = { .. },
>      [2] = { .. },
>   },
>   // etc.

Now, this seems confusion to me. The existing table looks fine IMO.

> };
> 
> Also, do we have defines for gears? Can we use them instead of indices?
> 

There are defines for the gears but not for lanes. So I ended up using numbers
for simplicity.

- Mani

> > +	[MODE_PWM][1][1] = { 922,	1000 },
> > +	[MODE_PWM][2][1] = { 1844,	1000 },
> > +	[MODE_PWM][3][1] = { 3688,	1000 },
> > +	[MODE_PWM][4][1] = { 7376,	1000 },
> > +	[MODE_PWM][1][2] = { 1844,	1000 },
> > +	[MODE_PWM][2][2] = { 3688,	1000 },
> > +	[MODE_PWM][3][2] = { 7376,	1000 },
> > +	[MODE_PWM][4][2] = { 14752,	1000 },
> > +	[MODE_HS_RA][1][1] = { 127796,	1000 },
> > +	[MODE_HS_RA][2][1] = { 255591,	1000 },
> > +	[MODE_HS_RA][3][1] = { 1492582,	102400 },
> > +	[MODE_HS_RA][4][1] = { 2915200,	204800 },
> > +	[MODE_HS_RA][1][2] = { 255591,	1000 },
> > +	[MODE_HS_RA][2][2] = { 511181,	1000 },
> > +	[MODE_HS_RA][3][2] = { 1492582,	204800 },
> > +	[MODE_HS_RA][4][2] = { 2915200,	409600 },
> > +	[MODE_HS_RB][1][1] = { 149422,	1000 },
> > +	[MODE_HS_RB][2][1] = { 298189,	1000 },
> > +	[MODE_HS_RB][3][1] = { 1492582,	102400 },
> > +	[MODE_HS_RB][4][1] = { 2915200,	204800 },
> > +	[MODE_HS_RB][1][2] = { 298189,	1000 },
> > +	[MODE_HS_RB][2][2] = { 596378,	1000 },
> > +	[MODE_HS_RB][3][2] = { 1492582,	204800 },
> > +	[MODE_HS_RB][4][2] = { 2915200,	409600 },
> > +	[MODE_MAX][0][0] = { 7643136, 307200 },
> > +};
> > +
> >   static struct ufs_qcom_host *ufs_qcom_hosts[MAX_UFS_QCOM_HOSTS];
> >   static void ufs_qcom_get_default_testbus_cfg(struct ufs_qcom_host *host);
> > @@ -789,6 +833,51 @@ static void ufs_qcom_dev_ref_clk_ctrl(struct ufs_qcom_host *host, bool enable)
> >   	}
> >   }
> > +static int ufs_qcom_icc_set_bw(struct ufs_qcom_host *host, u32 bw1, u32 bw2)
> > +{
> > +	struct device *dev = host->hba->dev;
> > +	int ret;
> > +
> > +	ret = icc_set_bw(host->icc_ddr, 0, bw1);
> > +	if (ret < 0) {
> > +		dev_err(dev, "failed to set bandwidth request: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = icc_set_bw(host->icc_cpu, 0, bw2);
> > +	if (ret < 0) {
> > +		dev_err(dev, "failed to set bandwidth request: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static struct __ufs_qcom_bw_table ufs_qcom_get_bw_table(struct ufs_qcom_host *host)
> > +{
> > +	struct ufs_pa_layer_attr *p = &host->dev_req_params;
> > +	int gear = max_t(u32, p->gear_rx, p->gear_tx);
> > +	int lane = max_t(u32, p->lane_rx, p->lane_tx);
> > +
> > +	if (ufshcd_is_hs_mode(p)) {
> > +		if (p->hs_rate == PA_HS_MODE_B)
> > +			return ufs_qcom_bw_table[MODE_HS_RB][gear][lane];
> > +		else
> > +			return ufs_qcom_bw_table[MODE_HS_RA][gear][lane];
> > +	} else {
> > +		return ufs_qcom_bw_table[MODE_PWM][gear][lane];
> > +	}
> > +}
> > +
> > +static int ufs_qcom_icc_update_bw(struct ufs_qcom_host *host)
> > +{
> > +	struct __ufs_qcom_bw_table bw_table;
> > +
> > +	bw_table = ufs_qcom_get_bw_table(host);
> > +
> > +	return ufs_qcom_icc_set_bw(host, bw_table.bw1, bw_table.bw2);
> > +}
> > +
> >   static int ufs_qcom_pwr_change_notify(struct ufs_hba *hba,
> >   				enum ufs_notify_change_status status,
> >   				struct ufs_pa_layer_attr *dev_max_params,
> > @@ -852,6 +941,8 @@ static int ufs_qcom_pwr_change_notify(struct ufs_hba *hba,
> >   		memcpy(&host->dev_req_params,
> >   				dev_req_params, sizeof(*dev_req_params));
> > +		ufs_qcom_icc_update_bw(host);
> > +
> >   		/* disable the device ref clock if entered PWM mode */
> >   		if (ufshcd_is_hs_mode(&hba->pwr_info) &&
> >   			!ufshcd_is_hs_mode(dev_req_params))
> > @@ -981,7 +1072,9 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
> >   	switch (status) {
> >   	case PRE_CHANGE:
> > -		if (!on) {
> > +		if (on) {
> > +			ufs_qcom_icc_update_bw(host);
> > +		} else {
> >   			if (!ufs_qcom_is_link_active(hba)) {
> >   				/* disable device ref_clk */
> >   				ufs_qcom_dev_ref_clk_ctrl(host, false);
> > @@ -993,6 +1086,9 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
> >   			/* enable the device ref clock for HS mode*/
> >   			if (ufshcd_is_hs_mode(&hba->pwr_info))
> >   				ufs_qcom_dev_ref_clk_ctrl(host, true);
> > +		} else {
> > +			ufs_qcom_icc_set_bw(host, ufs_qcom_bw_table[MODE_MIN][0][0].bw1,
> > +					    ufs_qcom_bw_table[MODE_MIN][0][0].bw2);
> >   		}
> >   		break;
> >   	}
> > @@ -1031,6 +1127,34 @@ static const struct reset_control_ops ufs_qcom_reset_ops = {
> >   	.deassert = ufs_qcom_reset_deassert,
> >   };
> > +static int ufs_qcom_icc_init(struct ufs_qcom_host *host)
> > +{
> > +	struct device *dev = host->hba->dev;
> > +	int ret;
> > +
> > +	host->icc_ddr = devm_of_icc_get(dev, "ufs-ddr");
> > +	if (IS_ERR(host->icc_ddr))
> > +		return dev_err_probe(dev, PTR_ERR(host->icc_ddr),
> > +				    "failed to acquire interconnect path\n");
> > +
> > +	host->icc_cpu = devm_of_icc_get(dev, "cpu-ufs");
> > +	if (IS_ERR(host->icc_cpu))
> > +		return dev_err_probe(dev, PTR_ERR(host->icc_cpu),
> > +				    "failed to acquire interconnect path\n");
> > +
> > +	/*
> > +	 * Set Maximum bandwidth vote before initializing the UFS controller and
> > +	 * device. Ideally, a minimal interconnect vote would suffice for the
> > +	 * initialization, but a max vote would allow faster initialization.
> > +	 */
> > +	ret = ufs_qcom_icc_set_bw(host, ufs_qcom_bw_table[MODE_MAX][0][0].bw1,
> > +				  ufs_qcom_bw_table[MODE_MAX][0][0].bw2);
> > +	if (ret < 0)
> > +		return dev_err_probe(dev, ret, "failed to set bandwidth request\n");
> > +
> > +	return 0;
> > +}
> > +
> >   /**
> >    * ufs_qcom_init - bind phy with controller
> >    * @hba: host controller instance
> > @@ -1085,6 +1209,10 @@ static int ufs_qcom_init(struct ufs_hba *hba)
> >   		}
> >   	}
> > +	err = ufs_qcom_icc_init(host);
> > +	if (err)
> > +		goto out_variant_clear;
> > +
> >   	host->device_reset = devm_gpiod_get_optional(dev, "reset",
> >   						     GPIOD_OUT_HIGH);
> >   	if (IS_ERR(host->device_reset)) {
> > @@ -1282,6 +1410,7 @@ static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba,
> >   				    dev_req_params->pwr_rx,
> >   				    dev_req_params->hs_rate,
> >   				    false);
> > +		ufs_qcom_icc_update_bw(host);
> >   		ufshcd_uic_hibern8_exit(hba);
> >   	}
> > diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
> > index 6289ad5a42d0..dc27395ecba1 100644
> > --- a/drivers/ufs/host/ufs-qcom.h
> > +++ b/drivers/ufs/host/ufs-qcom.h
> > @@ -206,6 +206,9 @@ struct ufs_qcom_host {
> >   	struct clk *tx_l1_sync_clk;
> >   	bool is_lane_clks_enabled;
> > +	struct icc_path *icc_ddr;
> > +	struct icc_path *icc_cpu;
> > +
> >   #ifdef CONFIG_SCSI_UFS_CRYPTO
> >   	struct qcom_ice *ice;
> >   #endif
> 
> -- 
> With best wishes
> Dmitry
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 14/14] scsi: ufs: qcom: Add support for scaling interconnects

[Dmitry Baryshkov]

On 12/07/2023 19:41, Manivannan Sadhasivam wrote:
> On Wed, Jul 12, 2023 at 04:22:51PM +0300, Dmitry Baryshkov wrote:
>> On 12/07/2023 13:32, Manivannan Sadhasivam wrote:
>>> Qcom SoCs require scaling the interconnect paths for proper working of the
>>> peripherals connected through interconnects. Even for accessing the UFS
>>> controller, someone should setup the interconnect paths. So far, the
>>> bootloaders used to setup the interconnect paths before booting linux as
>>> they need to access the UFS storage for things like fetching boot firmware.
>>> But with the advent of multi boot options, bootloader nowadays like in
>>> SA8540p SoC do not setup the interconnect paths at all.
>>>
>>> So trying to configure UFS in the absence of the interconnect path
>>> configuration, results in boot crash.
>>>
>>> To fix this issue and also to dynamically scale the interconnects (UFS-DDR
>>> and CPU-UFS), interconnect API support is added to the Qcom UFS driver.
>>> With this support, the interconnect paths are scaled dynamically based on
>>> the gear configuration.
>>>
>>> During the early stage of ufs_qcom_init(), ufs_qcom_icc_init() will setup
>>> the paths to max bandwidth to allow configuring the UFS registers. Touching
>>> the registers without configuring the icc paths would result in a crash.
>>> However, we don't really need to set max vote for the icc paths as any
>>> minimal vote would suffice. But the max value would allow initialization to
>>> be done faster. After init, the bandwidth will get updated using
>>> ufs_qcom_icc_update_bw() based on the gear and lane configuration.
>>>
>>> The bandwidth values defined in ufs_qcom_bw_table struct are taken from
>>> Qcom downstream vendor devicetree source and are calculated as per the
>>> UFS3.1 Spec, Section 6.4.1, HS Gear Rates. So it is fixed across platforms.
>>>
>>> Cc: Brian Masney <bmasney@redhat.com>
>>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>>> ---
>>>    drivers/ufs/host/ufs-qcom.c | 131 +++++++++++++++++++++++++++++++++++-
>>>    drivers/ufs/host/ufs-qcom.h |   3 +
>>>    2 files changed, 133 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
>>> index 8d6fd4c3324f..8a3132d45a65 100644
>>> --- a/drivers/ufs/host/ufs-qcom.c
>>> +++ b/drivers/ufs/host/ufs-qcom.c
>>> @@ -7,6 +7,7 @@
>>>    #include <linux/time.h>
>>>    #include <linux/clk.h>
>>>    #include <linux/delay.h>
>>> +#include <linux/interconnect.h>
>>>    #include <linux/module.h>
>>>    #include <linux/of.h>
>>>    #include <linux/platform_device.h>
>>> @@ -46,6 +47,49 @@ enum {
>>>    	TSTBUS_MAX,
>>>    };
>>> +#define QCOM_UFS_MAX_GEAR 4
>>> +#define QCOM_UFS_MAX_LANE 2
>>> +
>>> +enum {
>>> +	MODE_MIN,
>>> +	MODE_PWM,
>>> +	MODE_HS_RA,
>>> +	MODE_HS_RB,
>>> +	MODE_MAX,
>>
>> MODE_MIN and MODE_MAX seem to be unused
>>
> 
> No, they are used in the driver.

Ack, missed that. Will comment inline.

> 
>>> +};
>>> +
>>> +struct __ufs_qcom_bw_table {
>>> +	u32 bw1;
>>> +	u32 bw2;
>>
>> Please consider:
>>
>> s/bw1/mem_bw/
>> s/bw2/cfg_bw/
>>
> 
> Ok.
> 
>>> +} ufs_qcom_bw_table[MODE_MAX + 1][QCOM_UFS_MAX_GEAR + 1][QCOM_UFS_MAX_LANE + 1] = {
>>
>> I'd say, these +1's are slightly confusing and unnecessary.
>>
> 
> These max values itself are used in the table. Without adding 1 to them, we will
> end up with out of bounds access.



> 
>>> +	[MODE_MIN][0][0] = { 0,		0 },	/* Bandwidth values are in KB/s */
>>
>> I'd say, this becomes impossible to check. Can you please structure it?
> 
> What do you mean by "impossible to check"?
> 
>> Either by inclusion:
>>
>> [MODE_PWM] = {
>>    [1] = {
>>       [1] = { .. },
>>       [2] = { .. },
>>    },
>>    // etc.
> 
> Now, this seems confusion to me. The existing table looks fine IMO.

Fine with me then, your call!

> 
>> };
>>
>> Also, do we have defines for gears? Can we use them instead of indices?
>>
> 
> There are defines for the gears but not for lanes. So I ended up using numbers
> for simplicity.

My suggestion would be to use them for gears at least. Then it becomes 
cleaner (and maybe will solve some of my other comments).

> 
> - Mani
> 
>>> +	[MODE_PWM][1][1] = { 922,	1000 },
>>> +	[MODE_PWM][2][1] = { 1844,	1000 },
>>> +	[MODE_PWM][3][1] = { 3688,	1000 },
>>> +	[MODE_PWM][4][1] = { 7376,	1000 },
>>> +	[MODE_PWM][1][2] = { 1844,	1000 },
>>> +	[MODE_PWM][2][2] = { 3688,	1000 },
>>> +	[MODE_PWM][3][2] = { 7376,	1000 },
>>> +	[MODE_PWM][4][2] = { 14752,	1000 },
>>> +	[MODE_HS_RA][1][1] = { 127796,	1000 },
>>> +	[MODE_HS_RA][2][1] = { 255591,	1000 },
>>> +	[MODE_HS_RA][3][1] = { 1492582,	102400 },
>>> +	[MODE_HS_RA][4][1] = { 2915200,	204800 },
>>> +	[MODE_HS_RA][1][2] = { 255591,	1000 },
>>> +	[MODE_HS_RA][2][2] = { 511181,	1000 },
>>> +	[MODE_HS_RA][3][2] = { 1492582,	204800 },
>>> +	[MODE_HS_RA][4][2] = { 2915200,	409600 },
>>> +	[MODE_HS_RB][1][1] = { 149422,	1000 },
>>> +	[MODE_HS_RB][2][1] = { 298189,	1000 },
>>> +	[MODE_HS_RB][3][1] = { 1492582,	102400 },
>>> +	[MODE_HS_RB][4][1] = { 2915200,	204800 },
>>> +	[MODE_HS_RB][1][2] = { 298189,	1000 },
>>> +	[MODE_HS_RB][2][2] = { 596378,	1000 },
>>> +	[MODE_HS_RB][3][2] = { 1492582,	204800 },
>>> +	[MODE_HS_RB][4][2] = { 2915200,	409600 },
>>> +	[MODE_MAX][0][0] = { 7643136, 307200 },
>>> +};
>>> +
>>>    static struct ufs_qcom_host *ufs_qcom_hosts[MAX_UFS_QCOM_HOSTS];
>>>    static void ufs_qcom_get_default_testbus_cfg(struct ufs_qcom_host *host);
>>> @@ -789,6 +833,51 @@ static void ufs_qcom_dev_ref_clk_ctrl(struct ufs_qcom_host *host, bool enable)
>>>    	}
>>>    }
>>> +static int ufs_qcom_icc_set_bw(struct ufs_qcom_host *host, u32 bw1, u32 bw2)
>>> +{
>>> +	struct device *dev = host->hba->dev;
>>> +	int ret;
>>> +
>>> +	ret = icc_set_bw(host->icc_ddr, 0, bw1);
>>> +	if (ret < 0) {
>>> +		dev_err(dev, "failed to set bandwidth request: %d\n", ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	ret = icc_set_bw(host->icc_cpu, 0, bw2);
>>> +	if (ret < 0) {
>>> +		dev_err(dev, "failed to set bandwidth request: %d\n", ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static struct __ufs_qcom_bw_table ufs_qcom_get_bw_table(struct ufs_qcom_host *host)
>>> +{
>>> +	struct ufs_pa_layer_attr *p = &host->dev_req_params;
>>> +	int gear = max_t(u32, p->gear_rx, p->gear_tx);
>>> +	int lane = max_t(u32, p->lane_rx, p->lane_tx);
>>> +
>>> +	if (ufshcd_is_hs_mode(p)) {
>>> +		if (p->hs_rate == PA_HS_MODE_B)
>>> +			return ufs_qcom_bw_table[MODE_HS_RB][gear][lane];
>>> +		else
>>> +			return ufs_qcom_bw_table[MODE_HS_RA][gear][lane];
>>> +	} else {
>>> +		return ufs_qcom_bw_table[MODE_PWM][gear][lane];
>>> +	}
>>> +}
>>> +
>>> +static int ufs_qcom_icc_update_bw(struct ufs_qcom_host *host)
>>> +{
>>> +	struct __ufs_qcom_bw_table bw_table;
>>> +
>>> +	bw_table = ufs_qcom_get_bw_table(host);
>>> +
>>> +	return ufs_qcom_icc_set_bw(host, bw_table.bw1, bw_table.bw2);
>>> +}
>>> +
>>>    static int ufs_qcom_pwr_change_notify(struct ufs_hba *hba,
>>>    				enum ufs_notify_change_status status,
>>>    				struct ufs_pa_layer_attr *dev_max_params,
>>> @@ -852,6 +941,8 @@ static int ufs_qcom_pwr_change_notify(struct ufs_hba *hba,
>>>    		memcpy(&host->dev_req_params,
>>>    				dev_req_params, sizeof(*dev_req_params));
>>> +		ufs_qcom_icc_update_bw(host);
>>> +
>>>    		/* disable the device ref clock if entered PWM mode */
>>>    		if (ufshcd_is_hs_mode(&hba->pwr_info) &&
>>>    			!ufshcd_is_hs_mode(dev_req_params))
>>> @@ -981,7 +1072,9 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
>>>    	switch (status) {
>>>    	case PRE_CHANGE:
>>> -		if (!on) {
>>> +		if (on) {
>>> +			ufs_qcom_icc_update_bw(host);
>>> +		} else {
>>>    			if (!ufs_qcom_is_link_active(hba)) {
>>>    				/* disable device ref_clk */
>>>    				ufs_qcom_dev_ref_clk_ctrl(host, false);
>>> @@ -993,6 +1086,9 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
>>>    			/* enable the device ref clock for HS mode*/
>>>    			if (ufshcd_is_hs_mode(&hba->pwr_info))
>>>    				ufs_qcom_dev_ref_clk_ctrl(host, true);
>>> +		} else {
>>> +			ufs_qcom_icc_set_bw(host, ufs_qcom_bw_table[MODE_MIN][0][0].bw1,
>>> +					    ufs_qcom_bw_table[MODE_MIN][0][0].bw2);

With MODE_MIN values being initialised to 0, can we use the value 
directly instead? You are not defining the whole table for MODE_MIN anyway.

>>>    		}
>>>    		break;
>>>    	}
>>> @@ -1031,6 +1127,34 @@ static const struct reset_control_ops ufs_qcom_reset_ops = {
>>>    	.deassert = ufs_qcom_reset_deassert,
>>>    };
>>> +static int ufs_qcom_icc_init(struct ufs_qcom_host *host)
>>> +{
>>> +	struct device *dev = host->hba->dev;
>>> +	int ret;
>>> +
>>> +	host->icc_ddr = devm_of_icc_get(dev, "ufs-ddr");
>>> +	if (IS_ERR(host->icc_ddr))
>>> +		return dev_err_probe(dev, PTR_ERR(host->icc_ddr),
>>> +				    "failed to acquire interconnect path\n");
>>> +
>>> +	host->icc_cpu = devm_of_icc_get(dev, "cpu-ufs");
>>> +	if (IS_ERR(host->icc_cpu))
>>> +		return dev_err_probe(dev, PTR_ERR(host->icc_cpu),
>>> +				    "failed to acquire interconnect path\n");
>>> +
>>> +	/*
>>> +	 * Set Maximum bandwidth vote before initializing the UFS controller and
>>> +	 * device. Ideally, a minimal interconnect vote would suffice for the
>>> +	 * initialization, but a max vote would allow faster initialization.
>>> +	 */
>>> +	ret = ufs_qcom_icc_set_bw(host, ufs_qcom_bw_table[MODE_MAX][0][0].bw1,
>>> +				  ufs_qcom_bw_table[MODE_MAX][0][0].bw2);

If this is a static value, can we define it separately? You don't have a 
full gear/lanes enumeration anyway.

>>> +	if (ret < 0)
>>> +		return dev_err_probe(dev, ret, "failed to set bandwidth request\n");
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>    /**
>>>     * ufs_qcom_init - bind phy with controller
>>>     * @hba: host controller instance
>>> @@ -1085,6 +1209,10 @@ static int ufs_qcom_init(struct ufs_hba *hba)
>>>    		}
>>>    	}
>>> +	err = ufs_qcom_icc_init(host);
>>> +	if (err)
>>> +		goto out_variant_clear;
>>> +
>>>    	host->device_reset = devm_gpiod_get_optional(dev, "reset",
>>>    						     GPIOD_OUT_HIGH);
>>>    	if (IS_ERR(host->device_reset)) {
>>> @@ -1282,6 +1410,7 @@ static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba,
>>>    				    dev_req_params->pwr_rx,
>>>    				    dev_req_params->hs_rate,
>>>    				    false);
>>> +		ufs_qcom_icc_update_bw(host);
>>>    		ufshcd_uic_hibern8_exit(hba);
>>>    	}
>>> diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
>>> index 6289ad5a42d0..dc27395ecba1 100644
>>> --- a/drivers/ufs/host/ufs-qcom.h
>>> +++ b/drivers/ufs/host/ufs-qcom.h
>>> @@ -206,6 +206,9 @@ struct ufs_qcom_host {
>>>    	struct clk *tx_l1_sync_clk;
>>>    	bool is_lane_clks_enabled;
>>> +	struct icc_path *icc_ddr;
>>> +	struct icc_path *icc_cpu;
>>> +
>>>    #ifdef CONFIG_SCSI_UFS_CRYPTO
>>>    	struct qcom_ice *ice;
>>>    #endif
>>
>> -- 
>> With best wishes
>> Dmitry
>>
> 

-- 
With best wishes
Dmitry

Re: [PATCH 14/14] scsi: ufs: qcom: Add support for scaling interconnects

[Manivannan Sadhasivam]

On Wed, Jul 12, 2023 at 08:23:02PM +0300, Dmitry Baryshkov wrote:
> On 12/07/2023 19:41, Manivannan Sadhasivam wrote:
> > On Wed, Jul 12, 2023 at 04:22:51PM +0300, Dmitry Baryshkov wrote:
> > > On 12/07/2023 13:32, Manivannan Sadhasivam wrote:
> > > > Qcom SoCs require scaling the interconnect paths for proper working of the
> > > > peripherals connected through interconnects. Even for accessing the UFS
> > > > controller, someone should setup the interconnect paths. So far, the
> > > > bootloaders used to setup the interconnect paths before booting linux as
> > > > they need to access the UFS storage for things like fetching boot firmware.
> > > > But with the advent of multi boot options, bootloader nowadays like in
> > > > SA8540p SoC do not setup the interconnect paths at all.
> > > > 
> > > > So trying to configure UFS in the absence of the interconnect path
> > > > configuration, results in boot crash.
> > > > 
> > > > To fix this issue and also to dynamically scale the interconnects (UFS-DDR
> > > > and CPU-UFS), interconnect API support is added to the Qcom UFS driver.
> > > > With this support, the interconnect paths are scaled dynamically based on
> > > > the gear configuration.
> > > > 
> > > > During the early stage of ufs_qcom_init(), ufs_qcom_icc_init() will setup
> > > > the paths to max bandwidth to allow configuring the UFS registers. Touching
> > > > the registers without configuring the icc paths would result in a crash.
> > > > However, we don't really need to set max vote for the icc paths as any
> > > > minimal vote would suffice. But the max value would allow initialization to
> > > > be done faster. After init, the bandwidth will get updated using
> > > > ufs_qcom_icc_update_bw() based on the gear and lane configuration.
> > > > 
> > > > The bandwidth values defined in ufs_qcom_bw_table struct are taken from
> > > > Qcom downstream vendor devicetree source and are calculated as per the
> > > > UFS3.1 Spec, Section 6.4.1, HS Gear Rates. So it is fixed across platforms.
> > > > 
> > > > Cc: Brian Masney <bmasney@redhat.com>
> > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > ---
> > > >    drivers/ufs/host/ufs-qcom.c | 131 +++++++++++++++++++++++++++++++++++-
> > > >    drivers/ufs/host/ufs-qcom.h |   3 +
> > > >    2 files changed, 133 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> > > > index 8d6fd4c3324f..8a3132d45a65 100644
> > > > --- a/drivers/ufs/host/ufs-qcom.c
> > > > +++ b/drivers/ufs/host/ufs-qcom.c
> > > > @@ -7,6 +7,7 @@
> > > >    #include <linux/time.h>
> > > >    #include <linux/clk.h>
> > > >    #include <linux/delay.h>
> > > > +#include <linux/interconnect.h>
> > > >    #include <linux/module.h>
> > > >    #include <linux/of.h>
> > > >    #include <linux/platform_device.h>
> > > > @@ -46,6 +47,49 @@ enum {
> > > >    	TSTBUS_MAX,
> > > >    };
> > 

[...]

> > > };
> > > 
> > > Also, do we have defines for gears? Can we use them instead of indices?
> > > 
> > 
> > There are defines for the gears but not for lanes. So I ended up using numbers
> > for simplicity.
> 
> My suggestion would be to use them for gears at least. Then it becomes
> cleaner (and maybe will solve some of my other comments).
> 

I think it'd better to add enums for lanes as well (in unipro.h) and use both.

> > 
> > - Mani
> > 
> > > > +	[MODE_PWM][1][1] = { 922,	1000 },
> > > > +	[MODE_PWM][2][1] = { 1844,	1000 },
> > > > +	[MODE_PWM][3][1] = { 3688,	1000 },
> > > > +	[MODE_PWM][4][1] = { 7376,	1000 },
> > > > +	[MODE_PWM][1][2] = { 1844,	1000 },
> > > > +	[MODE_PWM][2][2] = { 3688,	1000 },
> > > > +	[MODE_PWM][3][2] = { 7376,	1000 },
> > > > +	[MODE_PWM][4][2] = { 14752,	1000 },
> > > > +	[MODE_HS_RA][1][1] = { 127796,	1000 },
> > > > +	[MODE_HS_RA][2][1] = { 255591,	1000 },
> > > > +	[MODE_HS_RA][3][1] = { 1492582,	102400 },
> > > > +	[MODE_HS_RA][4][1] = { 2915200,	204800 },
> > > > +	[MODE_HS_RA][1][2] = { 255591,	1000 },
> > > > +	[MODE_HS_RA][2][2] = { 511181,	1000 },
> > > > +	[MODE_HS_RA][3][2] = { 1492582,	204800 },
> > > > +	[MODE_HS_RA][4][2] = { 2915200,	409600 },
> > > > +	[MODE_HS_RB][1][1] = { 149422,	1000 },
> > > > +	[MODE_HS_RB][2][1] = { 298189,	1000 },
> > > > +	[MODE_HS_RB][3][1] = { 1492582,	102400 },
> > > > +	[MODE_HS_RB][4][1] = { 2915200,	204800 },
> > > > +	[MODE_HS_RB][1][2] = { 298189,	1000 },
> > > > +	[MODE_HS_RB][2][2] = { 596378,	1000 },
> > > > +	[MODE_HS_RB][3][2] = { 1492582,	204800 },
> > > > +	[MODE_HS_RB][4][2] = { 2915200,	409600 },
> > > > +	[MODE_MAX][0][0] = { 7643136, 307200 },
> > > > +};
> > > > +

[...]

> > > > +static int ufs_qcom_icc_update_bw(struct ufs_qcom_host *host)
> > > > +{
> > > > +	struct __ufs_qcom_bw_table bw_table;
> > > > +
> > > > +	bw_table = ufs_qcom_get_bw_table(host);
> > > > +
> > > > +	return ufs_qcom_icc_set_bw(host, bw_table.bw1, bw_table.bw2);
> > > > +}
> > > > +
> > > >    static int ufs_qcom_pwr_change_notify(struct ufs_hba *hba,
> > > >    				enum ufs_notify_change_status status,
> > > >    				struct ufs_pa_layer_attr *dev_max_params,
> > > > @@ -852,6 +941,8 @@ static int ufs_qcom_pwr_change_notify(struct ufs_hba *hba,
> > > >    		memcpy(&host->dev_req_params,
> > > >    				dev_req_params, sizeof(*dev_req_params));
> > > > +		ufs_qcom_icc_update_bw(host);
> > > > +
> > > >    		/* disable the device ref clock if entered PWM mode */
> > > >    		if (ufshcd_is_hs_mode(&hba->pwr_info) &&
> > > >    			!ufshcd_is_hs_mode(dev_req_params))
> > > > @@ -981,7 +1072,9 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
> > > >    	switch (status) {
> > > >    	case PRE_CHANGE:
> > > > -		if (!on) {
> > > > +		if (on) {
> > > > +			ufs_qcom_icc_update_bw(host);
> > > > +		} else {
> > > >    			if (!ufs_qcom_is_link_active(hba)) {
> > > >    				/* disable device ref_clk */
> > > >    				ufs_qcom_dev_ref_clk_ctrl(host, false);
> > > > @@ -993,6 +1086,9 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
> > > >    			/* enable the device ref clock for HS mode*/
> > > >    			if (ufshcd_is_hs_mode(&hba->pwr_info))
> > > >    				ufs_qcom_dev_ref_clk_ctrl(host, true);
> > > > +		} else {
> > > > +			ufs_qcom_icc_set_bw(host, ufs_qcom_bw_table[MODE_MIN][0][0].bw1,
> > > > +					    ufs_qcom_bw_table[MODE_MIN][0][0].bw2);
> 
> With MODE_MIN values being initialised to 0, can we use the value directly
> instead? You are not defining the whole table for MODE_MIN anyway.
> 

I initially thought about it, but having all the values in the table gives
better visibility IMO. Otherwise, one has to look into the actual call to
determine what is being set for min and max.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 00/14] UFS: Add OPP and interconnect support

[Manivannan Sadhasivam]

On Wed, Jul 12, 2023 at 04:01:55PM +0530, Manivannan Sadhasivam wrote:
> Hi,
> 
> This series adds OPP (Operating Points) support to UFSHCD driver and
> interconnect support to Qcom UFS driver.
> 

Missed to cc SCSI folks. Will be resending this series. Sorry for the noise.

- Mani

> Motivation behind adding OPP support is to scale both clocks as well as
> regulators/performance state dynamically. Currently, UFSHCD just scales
> clock frequency during runtime with the help of "freq-table-hz" property
> defined in devicetree. With the addition of OPP tables in devicetree (as
> done for Qcom SDM845 and SM8250 SoCs in this series) UFSHCD can now scale
> both clocks and performance state of power domain which helps in power
> saving.
> 
> For the addition of OPP support to UFSHCD, there are changes required to
> the OPP framework and devfreq drivers which are also added in this series.
> 
> Finally, interconnect support is added to Qcom UFS driver for scaling the
> interconnect path dynamically. This is required to avoid boot crash in
> recent SoCs and also to save power during runtime. More information is
> available in patch 13/13.
> 
> Credits
> =======
> 
> This series is a continuation of previous work by Krzysztof Kozlowski [1]
> and Brian Masney [2]. Ideally, this could've split into two series (OPP
> and interconnect) but since there will be a dependency in the devicetree,
> I decided to keep them in a single series.
> 
> Testing
> =======
> 
> This series is tested on 96Boards RB3 (SDM845 SoC) and RB5 (SM8250 SoC)
> development boards.
> 
> Merging Strategy
> ================
> 
> An immutable branch might be required between OPP and SCSI trees because of
> the API dependency (devfreq too). And I leave it up to the maintainers to
> decide.
> 
> Thanks,
> Mani
> 
> [1] https://lore.kernel.org/all/20220513061347.46480-1-krzysztof.kozlowski@linaro.org/
> [2] https://lore.kernel.org/all/20221117104957.254648-1-bmasney@redhat.com/
> 
> Krzysztof Kozlowski (2):
>   dt-bindings: ufs: common: add OPP table
>   arm64: dts: qcom: sdm845: Add OPP table support to UFSHC
> 
> Manivannan Sadhasivam (12):
>   dt-bindings: opp: Increase maxItems for opp-hz property
>   arm64: dts: qcom: sdm845: Add missing RPMh power domain to GCC
>   arm64: dts: qcom: sdm845: Fix the min frequency of "ice_core_clk"
>   arm64: dts: qcom: sm8250: Add OPP table support to UFSHC
>   OPP: Introduce dev_pm_opp_find_freq_{ceil/floor}_indexed() APIs
>   OPP: Introduce dev_pm_opp_get_freq_indexed() API
>   PM / devfreq: Switch to dev_pm_opp_find_freq_{ceil/floor}_indexed()
>     APIs
>   scsi: ufs: core: Add OPP support for scaling clocks and regulators
>   scsi: ufs: host: Add support for parsing OPP
>   arm64: dts: qcom: sdm845: Add interconnect paths to UFSHC
>   arm64: dts: qcom: sm8250: Add interconnect paths to UFSHC
>   scsi: ufs: qcom: Add support for scaling interconnects
> 
>  .../devicetree/bindings/opp/opp-v2-base.yaml  |   2 +-
>  .../devicetree/bindings/ufs/ufs-common.yaml   |  34 ++++-
>  arch/arm64/boot/dts/qcom/sdm845.dtsi          |  47 ++++--
>  arch/arm64/boot/dts/qcom/sm8250.dtsi          |  43 ++++--
>  drivers/devfreq/devfreq.c                     |  14 +-
>  drivers/opp/core.c                            |  76 ++++++++++
>  drivers/ufs/core/ufshcd.c                     | 142 ++++++++++++++----
>  drivers/ufs/host/ufs-qcom.c                   | 131 +++++++++++++++-
>  drivers/ufs/host/ufs-qcom.h                   |   3 +
>  drivers/ufs/host/ufshcd-pltfrm.c              | 116 ++++++++++++++
>  include/linux/pm_opp.h                        |  26 ++++
>  include/ufs/ufshcd.h                          |   4 +
>  12 files changed, 574 insertions(+), 64 deletions(-)
> 
> -- 
> 2.25.1
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 00/14] UFS: Add OPP and interconnect support

[Dmitry Baryshkov]

On 12/07/2023 13:40, Manivannan Sadhasivam wrote:
> On Wed, Jul 12, 2023 at 04:01:55PM +0530, Manivannan Sadhasivam wrote:
>> Hi,
>>
>> This series adds OPP (Operating Points) support to UFSHCD driver and
>> interconnect support to Qcom UFS driver.
>>
> 
> Missed to cc SCSI folks. Will be resending this series. Sorry for the noise.

I'd say, there is a need for the resend anyway, the series got duplicate 
patch indices.

> 
> - Mani
> 
>> Motivation behind adding OPP support is to scale both clocks as well as
>> regulators/performance state dynamically. Currently, UFSHCD just scales
>> clock frequency during runtime with the help of "freq-table-hz" property
>> defined in devicetree. With the addition of OPP tables in devicetree (as
>> done for Qcom SDM845 and SM8250 SoCs in this series) UFSHCD can now scale
>> both clocks and performance state of power domain which helps in power
>> saving.
>>
>> For the addition of OPP support to UFSHCD, there are changes required to
>> the OPP framework and devfreq drivers which are also added in this series.
>>
>> Finally, interconnect support is added to Qcom UFS driver for scaling the
>> interconnect path dynamically. This is required to avoid boot crash in
>> recent SoCs and also to save power during runtime. More information is
>> available in patch 13/13.
>>
>> Credits
>> =======
>>
>> This series is a continuation of previous work by Krzysztof Kozlowski [1]
>> and Brian Masney [2]. Ideally, this could've split into two series (OPP
>> and interconnect) but since there will be a dependency in the devicetree,
>> I decided to keep them in a single series.
>>
>> Testing
>> =======
>>
>> This series is tested on 96Boards RB3 (SDM845 SoC) and RB5 (SM8250 SoC)
>> development boards.
>>
>> Merging Strategy
>> ================
>>
>> An immutable branch might be required between OPP and SCSI trees because of
>> the API dependency (devfreq too). And I leave it up to the maintainers to
>> decide.
>>
>> Thanks,
>> Mani
>>
>> [1] https://lore.kernel.org/all/20220513061347.46480-1-krzysztof.kozlowski@linaro.org/
>> [2] https://lore.kernel.org/all/20221117104957.254648-1-bmasney@redhat.com/
>>
>> Krzysztof Kozlowski (2):
>>    dt-bindings: ufs: common: add OPP table
>>    arm64: dts: qcom: sdm845: Add OPP table support to UFSHC
>>
>> Manivannan Sadhasivam (12):
>>    dt-bindings: opp: Increase maxItems for opp-hz property
>>    arm64: dts: qcom: sdm845: Add missing RPMh power domain to GCC
>>    arm64: dts: qcom: sdm845: Fix the min frequency of "ice_core_clk"
>>    arm64: dts: qcom: sm8250: Add OPP table support to UFSHC
>>    OPP: Introduce dev_pm_opp_find_freq_{ceil/floor}_indexed() APIs
>>    OPP: Introduce dev_pm_opp_get_freq_indexed() API
>>    PM / devfreq: Switch to dev_pm_opp_find_freq_{ceil/floor}_indexed()
>>      APIs
>>    scsi: ufs: core: Add OPP support for scaling clocks and regulators
>>    scsi: ufs: host: Add support for parsing OPP
>>    arm64: dts: qcom: sdm845: Add interconnect paths to UFSHC
>>    arm64: dts: qcom: sm8250: Add interconnect paths to UFSHC
>>    scsi: ufs: qcom: Add support for scaling interconnects
>>
>>   .../devicetree/bindings/opp/opp-v2-base.yaml  |   2 +-
>>   .../devicetree/bindings/ufs/ufs-common.yaml   |  34 ++++-
>>   arch/arm64/boot/dts/qcom/sdm845.dtsi          |  47 ++++--
>>   arch/arm64/boot/dts/qcom/sm8250.dtsi          |  43 ++++--
>>   drivers/devfreq/devfreq.c                     |  14 +-
>>   drivers/opp/core.c                            |  76 ++++++++++
>>   drivers/ufs/core/ufshcd.c                     | 142 ++++++++++++++----
>>   drivers/ufs/host/ufs-qcom.c                   | 131 +++++++++++++++-
>>   drivers/ufs/host/ufs-qcom.h                   |   3 +
>>   drivers/ufs/host/ufshcd-pltfrm.c              | 116 ++++++++++++++
>>   include/linux/pm_opp.h                        |  26 ++++
>>   include/ufs/ufshcd.h                          |   4 +
>>   12 files changed, 574 insertions(+), 64 deletions(-)
>>
>> -- 
>> 2.25.1
>>
> 

-- 
With best wishes
Dmitry

Re: [PATCH 00/14] UFS: Add OPP and interconnect support

[Manivannan Sadhasivam]

On Wed, Jul 12, 2023 at 03:18:38PM +0300, Dmitry Baryshkov wrote:
> On 12/07/2023 13:40, Manivannan Sadhasivam wrote:
> > On Wed, Jul 12, 2023 at 04:01:55PM +0530, Manivannan Sadhasivam wrote:
> > > Hi,
> > > 
> > > This series adds OPP (Operating Points) support to UFSHCD driver and
> > > interconnect support to Qcom UFS driver.
> > > 
> > 
> > Missed to cc SCSI folks. Will be resending this series. Sorry for the noise.
> 
> I'd say, there is a need for the resend anyway, the series got duplicate
> patch indices.
> 

Yeah, my script screwed up. Will wait for some time to get comments before
resending though.

- Mani

> > 
> > - Mani
> > 
> > > Motivation behind adding OPP support is to scale both clocks as well as
> > > regulators/performance state dynamically. Currently, UFSHCD just scales
> > > clock frequency during runtime with the help of "freq-table-hz" property
> > > defined in devicetree. With the addition of OPP tables in devicetree (as
> > > done for Qcom SDM845 and SM8250 SoCs in this series) UFSHCD can now scale
> > > both clocks and performance state of power domain which helps in power
> > > saving.
> > > 
> > > For the addition of OPP support to UFSHCD, there are changes required to
> > > the OPP framework and devfreq drivers which are also added in this series.
> > > 
> > > Finally, interconnect support is added to Qcom UFS driver for scaling the
> > > interconnect path dynamically. This is required to avoid boot crash in
> > > recent SoCs and also to save power during runtime. More information is
> > > available in patch 13/13.
> > > 
> > > Credits
> > > =======
> > > 
> > > This series is a continuation of previous work by Krzysztof Kozlowski [1]
> > > and Brian Masney [2]. Ideally, this could've split into two series (OPP
> > > and interconnect) but since there will be a dependency in the devicetree,
> > > I decided to keep them in a single series.
> > > 
> > > Testing
> > > =======
> > > 
> > > This series is tested on 96Boards RB3 (SDM845 SoC) and RB5 (SM8250 SoC)
> > > development boards.
> > > 
> > > Merging Strategy
> > > ================
> > > 
> > > An immutable branch might be required between OPP and SCSI trees because of
> > > the API dependency (devfreq too). And I leave it up to the maintainers to
> > > decide.
> > > 
> > > Thanks,
> > > Mani
> > > 
> > > [1] https://lore.kernel.org/all/20220513061347.46480-1-krzysztof.kozlowski@linaro.org/
> > > [2] https://lore.kernel.org/all/20221117104957.254648-1-bmasney@redhat.com/
> > > 
> > > Krzysztof Kozlowski (2):
> > >    dt-bindings: ufs: common: add OPP table
> > >    arm64: dts: qcom: sdm845: Add OPP table support to UFSHC
> > > 
> > > Manivannan Sadhasivam (12):
> > >    dt-bindings: opp: Increase maxItems for opp-hz property
> > >    arm64: dts: qcom: sdm845: Add missing RPMh power domain to GCC
> > >    arm64: dts: qcom: sdm845: Fix the min frequency of "ice_core_clk"
> > >    arm64: dts: qcom: sm8250: Add OPP table support to UFSHC
> > >    OPP: Introduce dev_pm_opp_find_freq_{ceil/floor}_indexed() APIs
> > >    OPP: Introduce dev_pm_opp_get_freq_indexed() API
> > >    PM / devfreq: Switch to dev_pm_opp_find_freq_{ceil/floor}_indexed()
> > >      APIs
> > >    scsi: ufs: core: Add OPP support for scaling clocks and regulators
> > >    scsi: ufs: host: Add support for parsing OPP
> > >    arm64: dts: qcom: sdm845: Add interconnect paths to UFSHC
> > >    arm64: dts: qcom: sm8250: Add interconnect paths to UFSHC
> > >    scsi: ufs: qcom: Add support for scaling interconnects
> > > 
> > >   .../devicetree/bindings/opp/opp-v2-base.yaml  |   2 +-
> > >   .../devicetree/bindings/ufs/ufs-common.yaml   |  34 ++++-
> > >   arch/arm64/boot/dts/qcom/sdm845.dtsi          |  47 ++++--
> > >   arch/arm64/boot/dts/qcom/sm8250.dtsi          |  43 ++++--
> > >   drivers/devfreq/devfreq.c                     |  14 +-
> > >   drivers/opp/core.c                            |  76 ++++++++++
> > >   drivers/ufs/core/ufshcd.c                     | 142 ++++++++++++++----
> > >   drivers/ufs/host/ufs-qcom.c                   | 131 +++++++++++++++-
> > >   drivers/ufs/host/ufs-qcom.h                   |   3 +
> > >   drivers/ufs/host/ufshcd-pltfrm.c              | 116 ++++++++++++++
> > >   include/linux/pm_opp.h                        |  26 ++++
> > >   include/ufs/ufshcd.h                          |   4 +
> > >   12 files changed, 574 insertions(+), 64 deletions(-)
> > > 
> > > -- 
> > > 2.25.1
> > > 
> > 
> 
> -- 
> With best wishes
> Dmitry
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 00/14] UFS: Add OPP and interconnect support

[John Garry]

Did you bcc linux-scsi on this series?

I am just wondering why it came directly to my inbox and I am not the 
to: or cc: list. I also notice replies in the archives which I have not 
received...

Thanks,
John

Re: [PATCH 00/14] UFS: Add OPP and interconnect support

[Manivannan Sadhasivam]

On Wed, Jul 12, 2023 at 12:02:37PM +0100, John Garry wrote:
> Did you bcc linux-scsi on this series?
> 
> I am just wondering why it came directly to my inbox and I am not the to: or
> cc: list. I also notice replies in the archives which I have not received...
> 

I initially missed CCing scsi list and maintainers. So instead of resending the
series (since it has too many people CCed), I just bounced the patches to scsi
list and maintainers using Mutt.

From the next iteration, I will make sure to add everyone. Sorry for the
trouble.

- Mani

> Thanks,
> John

-- 
மணிவண்ணன் சதாசிவம்