[PATCH v3 0/7] ufs: set power domain performance state when scaling gears

[PATCH v3 0/7] ufs: set power domain performance state when scaling gears

[Krzysztof Kozlowski]

Hi,

Changes since v2
================
1. PM: Split PM OPP patch into two - getting clocks and rates. (Viresh)
2. PM: Do not set clock rates from PM OPPs but rely on set_opp helper. (Viresh)
3. PM: Use clk bulk operations in PM OPP for getting/releasing the clocks. (Bjorn)
4. UFS: Rework clock scalling to be called in the same place as old method, so
   pre/post changes notification will work. (Mani)
5. UFS: Bail out if both freq-table-hz and operating-points are provided. (Mani)
6. Add review tags.

Changes since v1
================
1. Patch #1 qcom,gcc-sdm845: fix typo (Stephen).
2. Patch #2 ufs dt-bindings: not adding Rob's review because patch
   changed significantly.
3. PM: add new code for handling multiple clocks.
4. UFS: deprecate freq-table-hz property and use PM opps instead.

Dependencies
============
The UFS patch depends on PM OPP patches adding multiple clocks/rates support.

Best regards,
Krzysztof

Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Krzysztof Kozlowski (7):
  dt-bindings: clock: qcom,gcc-sdm845: add parent power domain
  dt-bindings: opp: accept array of frequencies
  dt-bindings: ufs: common: add OPP table
  arm64: dts: qcom: sdm845: control RPMHPD performance states with UFS
  PM: opp: allow control of multiple clocks
  PM: opp: parse multiple frequencies in each OPP
  ufs: use PM OPP when scaling gears

 .../bindings/clock/qcom,gcc-sdm845.yaml       |   3 +
 .../devicetree/bindings/opp/opp-v2-base.yaml  |  10 +
 .../devicetree/bindings/ufs/ufs-common.yaml   |  34 ++-
 arch/arm64/boot/dts/qcom/sdm845.dtsi          |  43 +++-
 drivers/opp/core.c                            | 207 +++++++++++++-----
 drivers/opp/of.c                              |  47 ++++
 drivers/opp/opp.h                             |   9 +-
 drivers/opp/ti-opp-supply.c                   |   6 +-
 drivers/scsi/ufs/ufshcd-pltfrm.c              |  73 ++++++
 drivers/scsi/ufs/ufshcd.c                     | 150 ++++++++++---
 drivers/scsi/ufs/ufshcd.h                     |   6 +
 include/linux/pm_opp.h                        |  32 ++-
 12 files changed, 518 insertions(+), 102 deletions(-)

-- 
2.32.0

[PATCH v3 1/7] dt-bindings: clock: qcom,gcc-sdm845: add parent power domain

[Krzysztof Kozlowski]

Allow Qualcomm GCC to register its parent power domain (e.g. RPMHPD) to
properly pass performance state from children.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Acked-by: Rob Herring <robh@kernel.org>

---

Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 Documentation/devicetree/bindings/clock/qcom,gcc-sdm845.yaml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/qcom,gcc-sdm845.yaml b/Documentation/devicetree/bindings/clock/qcom,gcc-sdm845.yaml
index d902f137ab17..daf7906ebc40 100644
--- a/Documentation/devicetree/bindings/clock/qcom,gcc-sdm845.yaml
+++ b/Documentation/devicetree/bindings/clock/qcom,gcc-sdm845.yaml
@@ -43,6 +43,9 @@ properties:
   '#reset-cells':
     const: 1
 
+  power-domains:
+    maxItems: 1
+
   '#power-domain-cells':
     const: 1
 
-- 
2.32.0

[PATCH v3 2/7] dt-bindings: opp: accept array of frequencies

[Krzysztof Kozlowski]

Devices might need to control several clocks when scaling the frequency
and voltage.  Allow passing array of clock frequencies, similarly to the
voltages.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Acked-by: Rob Herring <robh@kernel.org>

---

Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 Documentation/devicetree/bindings/opp/opp-v2-base.yaml | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml
index 76c8acd981b3..66d0ec763f0b 100644
--- a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml
+++ b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml
@@ -50,6 +50,16 @@ patternProperties:
           property to uniquely identify the OPP nodes exists. Devices like power
           domains must have another (implementation dependent) property.
 
+          Entries for multiple clocks shall be provided in the same field, as
+          array of frequencies.  The OPP binding doesn't provide any provisions
+          to relate the values to their clocks or the order in which the clocks
+          need to be configured and that is left for the implementation
+          specific binding.
+        minItems: 1
+        maxItems: 16
+        items:
+          maxItems: 1
+
       opp-microvolt:
         description: |
           Voltage for the OPP
-- 
2.32.0

[PATCH v3 3/7] dt-bindings: ufs: common: add OPP table

[Krzysztof Kozlowski]

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>

---

Cc: 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.32.0

[PATCH v3 4/7] arm64: dts: qcom: sdm845: control RPMHPD performance states with UFS

[Krzysztof Kozlowski]

UFS, when scaling gears, should choose appropriate performance state of
RPMHPD power domain controller.  Since UFS belongs to UFS_PHY_GDSC power
domain, add necessary parent power domain to GCC.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

---

Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 arch/arm64/boot/dts/qcom/sdm845.dtsi | 43 +++++++++++++++++++++-------
 1 file changed, 33 insertions(+), 10 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index 692cf4be4eef..befcdd04d832 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -1078,6 +1078,7 @@ gcc: clock-controller@100000 {
 			#clock-cells = <1>;
 			#reset-cells = <1>;
 			#power-domain-cells = <1>;
+			power-domains = <&rpmhpd SDM845_CX>;
 		};
 
 		qfprom@784000 {
@@ -2326,18 +2327,40 @@ 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>,
-				<0 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
+						 0
+						 0
+						 37500000
+						 0
+						 0
+						 0
+						 0
+						 // FIXME: value 0 copied from freq-table-hz
+						 0>;
+					required-opps = <&rpmhpd_opp_svs>;
+				};
+
+				opp-200000000 {
+					opp-hz = /bits/ 64 <200000000
+						 0
+						 0
+						 150000000
+						 0
+						 0
+						 0
+						 0
+						 300000000>;
+					required-opps = <&rpmhpd_opp_nom>;
+				};
+			};
 		};
 
 		ufs_mem_phy: phy@1d87000 {
-- 
2.32.0

[PATCH v3 5/7] PM: opp: allow control of multiple clocks

[Krzysztof Kozlowski]

Devices might need to control several clocks when scaling the frequency
and voltage.  Example is the Universal Flash Storage (UFS) which scales
several independent clocks with change of performance levels.

Add parsing of multiple clocks and clock names, similarly to parsing
multiple regulators, and store them for custom set_opp.  The custom
set_opp helper is a requirement in such case, otherwise OPP framework
will refuse to scale multiple clocks.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

---

Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/opp/core.c          | 187 +++++++++++++++++++++++++++---------
 drivers/opp/opp.h           |   7 +-
 drivers/opp/ti-opp-supply.c |   6 +-
 include/linux/pm_opp.h      |  29 +++++-
 4 files changed, 177 insertions(+), 52 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 84063eaebb91..aac3c6d89ae0 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -66,6 +66,18 @@ static struct opp_table *_find_opp_table_unlocked(struct device *dev)
 	return ERR_PTR(-ENODEV);
 }
 
+static void _put_clocks(struct opp_table *opp_table)
+{
+	if (!opp_table->clks)
+		return;
+
+	clk_bulk_put(opp_table->clk_count, opp_table->clks);
+
+	kfree(opp_table->clks);
+	opp_table->clks = NULL;
+	opp_table->clk_count = -1;
+};
+
 /**
  * _find_opp_table() - find opp_table struct using device pointer
  * @dev:	device pointer used to lookup OPP table
@@ -874,11 +886,23 @@ static int _set_opp_voltage(struct device *dev, struct regulator *reg,
 	return ret;
 }
 
-static inline int _generic_set_opp_clk_only(struct device *dev, struct clk *clk,
+static inline int _generic_set_opp_clk_only(struct device *dev,
+					    struct opp_table *opp_table,
 					    unsigned long freq)
 {
+	struct clk *clk;
 	int ret;
 
+	/* This function only supports single clock per device */
+	if (WARN_ON(opp_table->clk_count > 1)) {
+		dev_err(dev, "multiple clocks are not supported\n");
+		return -EINVAL;
+	}
+
+	if (!opp_table->clks)
+		return 0;
+	clk = opp_table->clks[0].clk;
+
 	/* We may reach here for devices which don't change frequency */
 	if (IS_ERR(clk))
 		return 0;
@@ -903,8 +927,9 @@ static int _generic_set_opp_regulator(struct opp_table *opp_table,
 	int ret;
 
 	/* This function only supports single regulator per device */
-	if (WARN_ON(opp_table->regulator_count > 1)) {
-		dev_err(dev, "multiple regulators are not supported\n");
+	if (WARN_ON(opp_table->regulator_count > 1) ||
+	    WARN_ON(opp_table->clk_count > 1)) {
+		dev_err(dev, "multiple clocks/regulators are not supported\n");
 		return -EINVAL;
 	}
 
@@ -916,7 +941,7 @@ static int _generic_set_opp_regulator(struct opp_table *opp_table,
 	}
 
 	/* Change frequency */
-	ret = _generic_set_opp_clk_only(dev, opp_table->clk, freq);
+	ret = _generic_set_opp_clk_only(dev, opp_table, freq);
 	if (ret)
 		goto restore_voltage;
 
@@ -940,7 +965,7 @@ static int _generic_set_opp_regulator(struct opp_table *opp_table,
 	return 0;
 
 restore_freq:
-	if (_generic_set_opp_clk_only(dev, opp_table->clk, old_opp->rate))
+	if (_generic_set_opp_clk_only(dev, opp_table, old_opp->rate))
 		dev_err(dev, "%s: failed to restore old-freq (%lu Hz)\n",
 			__func__, old_opp->rate);
 restore_voltage:
@@ -987,8 +1012,8 @@ static int _set_opp_custom(const struct opp_table *opp_table,
 	int size;
 
 	/*
-	 * We support this only if dev_pm_opp_set_regulators() was called
-	 * earlier.
+	 * We support this only if dev_pm_opp_set_regulators() or
+	 * dev_pm_opp_set_clknames() were called earlier.
 	 */
 	if (opp_table->sod_supplies) {
 		size = sizeof(*old_opp->supplies) * opp_table->regulator_count;
@@ -999,8 +1024,13 @@ static int _set_opp_custom(const struct opp_table *opp_table,
 		data->regulator_count = 0;
 	}
 
+	if (opp_table->clks)
+		data->clk_count = opp_table->clk_count;
+	else
+		data->clk_count = 0;
+
 	data->regulators = opp_table->regulators;
-	data->clk = opp_table->clk;
+	data->clks = opp_table->clks;
 	data->dev = dev;
 	data->old_opp.rate = old_opp->rate;
 	data->new_opp.rate = freq;
@@ -1089,8 +1119,8 @@ static void _find_current_opp(struct device *dev, struct opp_table *opp_table)
 	struct dev_pm_opp *opp = ERR_PTR(-ENODEV);
 	unsigned long freq;
 
-	if (!IS_ERR(opp_table->clk)) {
-		freq = clk_get_rate(opp_table->clk);
+	if (opp_table->clks && !IS_ERR(opp_table->clks[0].clk)) {
+		freq = clk_get_rate(opp_table->clks[0].clk);
 		opp = _find_freq_ceil(opp_table, &freq);
 	}
 
@@ -1190,7 +1220,7 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
 						 scaling_down);
 	} else {
 		/* Only frequency scaling */
-		ret = _generic_set_opp_clk_only(dev, opp_table->clk, freq);
+		ret = _generic_set_opp_clk_only(dev, opp_table, freq);
 	}
 
 	if (ret)
@@ -1255,11 +1285,12 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 		 * equivalent to a clk_set_rate()
 		 */
 		if (!_get_opp_count(opp_table)) {
-			ret = _generic_set_opp_clk_only(dev, opp_table->clk, target_freq);
+			ret = _generic_set_opp_clk_only(dev, opp_table, target_freq);
 			goto put_opp_table;
 		}
 
-		freq = clk_round_rate(opp_table->clk, target_freq);
+		if (opp_table->clks)
+			freq = clk_round_rate(opp_table->clks[0].clk, target_freq);
 		if ((long)freq <= 0)
 			freq = target_freq;
 
@@ -1366,7 +1397,8 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index)
 	INIT_LIST_HEAD(&opp_table->dev_list);
 	INIT_LIST_HEAD(&opp_table->lazy);
 
-	/* Mark regulator count uninitialized */
+	/* Mark regulator/clk count uninitialized */
+	opp_table->clk_count = -1;
 	opp_table->regulator_count = -1;
 
 	opp_dev = _add_opp_dev(dev, opp_table);
@@ -1415,21 +1447,29 @@ static struct opp_table *_update_opp_table_clk(struct device *dev,
 	 * Return early if we don't need to get clk or we have already tried it
 	 * earlier.
 	 */
-	if (!getclk || IS_ERR(opp_table) || opp_table->clk)
+	if (!getclk || IS_ERR(opp_table) || opp_table->clks)
 		return opp_table;
 
-	/* Find clk for the device */
-	opp_table->clk = clk_get(dev, NULL);
+	opp_table->clks = kcalloc(1, sizeof(*opp_table->clks), GFP_KERNEL);
+	if (!opp_table->clks)
+		return ERR_PTR(-ENOMEM);
 
-	ret = PTR_ERR_OR_ZERO(opp_table->clk);
-	if (!ret)
+	/* Find clk for the device */
+	ret = clk_bulk_get(dev, 1, opp_table->clks);
+	if (!ret) {
+		opp_table->clk_count = 1;
 		return opp_table;
+	}
 
 	if (ret == -ENOENT) {
+		opp_table->clk_count = 0;
 		dev_dbg(dev, "%s: Couldn't find clock: %d\n", __func__, ret);
 		return opp_table;
 	}
 
+	kfree(opp_table->clks);
+	opp_table->clks = NULL;
+	opp_table->clk_count = -1;
 	dev_pm_opp_put_opp_table(opp_table);
 	dev_err_probe(dev, ret, "Couldn't find clock\n");
 
@@ -1528,9 +1568,7 @@ static void _opp_table_kref_release(struct kref *kref)
 
 	_of_clear_opp_table(opp_table);
 
-	/* Release clk */
-	if (!IS_ERR(opp_table->clk))
-		clk_put(opp_table->clk);
+	_put_clocks(opp_table);
 
 	if (opp_table->paths) {
 		for (i = 0; i < opp_table->path_count; i++)
@@ -2263,9 +2301,50 @@ EXPORT_SYMBOL_GPL(devm_pm_opp_set_regulators);
  * This must be called before any OPPs are initialized for the device.
  */
 struct opp_table *dev_pm_opp_set_clkname(struct device *dev, const char *name)
+{
+	return dev_pm_opp_set_clknames(dev, &name, 1);
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_set_clkname);
+
+/**
+ * dev_pm_opp_put_clkname() - Releases resources blocked for clk.
+ * @opp_table: OPP table returned from dev_pm_opp_set_clkname().
+ */
+void dev_pm_opp_put_clkname(struct opp_table *opp_table)
+{
+	return dev_pm_opp_put_clknames(opp_table);
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_put_clkname);
+
+/**
+ * devm_pm_opp_set_clkname() - Set clk name for the device
+ * @dev: Device for which clk name is being set.
+ * @name: Clk name.
+ *
+ * This is a resource-managed variant of dev_pm_opp_set_clkname().
+ *
+ * Return: 0 on success and errorno otherwise.
+ */
+int devm_pm_opp_set_clkname(struct device *dev, const char *name)
+{
+	return devm_pm_opp_set_clknames(dev, &name, 1);
+}
+EXPORT_SYMBOL_GPL(devm_pm_opp_set_clkname);
+
+/**
+ * dev_pm_opp_set_clknames() - Set clk names for the device
+ * @dev: Device for which clock names are being set.
+ * @names: Array of pointers to the names of the clocks.
+ * @count: Number of clocks.
+ *
+ * See: dev_pm_opp_set_clkname()
+ */
+struct opp_table *dev_pm_opp_set_clknames(struct device *dev,
+					  const char * const names[],
+					  unsigned int count)
 {
 	struct opp_table *opp_table;
-	int ret;
+	int ret, i;
 
 	opp_table = _add_opp_table(dev, false);
 	if (IS_ERR(opp_table))
@@ -2278,70 +2357,83 @@ struct opp_table *dev_pm_opp_set_clkname(struct device *dev, const char *name)
 	}
 
 	/* clk shouldn't be initialized at this point */
-	if (WARN_ON(opp_table->clk)) {
+	if (WARN_ON(opp_table->clks)) {
 		ret = -EBUSY;
 		goto err;
 	}
 
-	/* Find clk for the device */
-	opp_table->clk = clk_get(dev, name);
-	if (IS_ERR(opp_table->clk)) {
-		ret = dev_err_probe(dev, PTR_ERR(opp_table->clk),
-				    "%s: Couldn't find clock\n", __func__);
+	opp_table->clks = kcalloc(count, sizeof(*opp_table->clks), GFP_KERNEL);
+	if (!opp_table->clks) {
+		ret = -ENOMEM;
 		goto err;
 	}
 
+	for (i = 0; i < count; i++)
+		opp_table->clks[i].id = names[i];
+
+	ret = clk_bulk_get(dev, count, opp_table->clks);
+	if (ret)
+		goto free_clks;
+
+	opp_table->clk_count = count;
+
 	return opp_table;
 
+free_clks:
+	kfree(opp_table->clks);
+	opp_table->clks = NULL;
+	opp_table->clk_count = -1;
 err:
 	dev_pm_opp_put_opp_table(opp_table);
 
 	return ERR_PTR(ret);
 }
-EXPORT_SYMBOL_GPL(dev_pm_opp_set_clkname);
+EXPORT_SYMBOL_GPL(dev_pm_opp_set_clknames);
 
 /**
- * dev_pm_opp_put_clkname() - Releases resources blocked for clk.
- * @opp_table: OPP table returned from dev_pm_opp_set_clkname().
+ * dev_pm_opp_put_clknames() - Releases resources blocked for clk.
+ * @opp_table: OPP table returned from dev_pm_opp_set_clknames().
  */
-void dev_pm_opp_put_clkname(struct opp_table *opp_table)
+void dev_pm_opp_put_clknames(struct opp_table *opp_table)
 {
 	if (unlikely(!opp_table))
 		return;
 
-	clk_put(opp_table->clk);
-	opp_table->clk = ERR_PTR(-EINVAL);
+	_put_clocks(opp_table);
 
 	dev_pm_opp_put_opp_table(opp_table);
 }
-EXPORT_SYMBOL_GPL(dev_pm_opp_put_clkname);
+EXPORT_SYMBOL_GPL(dev_pm_opp_put_clknames);
 
-static void devm_pm_opp_clkname_release(void *data)
+static void devm_pm_opp_clknames_release(void *data)
 {
-	dev_pm_opp_put_clkname(data);
+	dev_pm_opp_put_clknames(data);
 }
 
 /**
- * devm_pm_opp_set_clkname() - Set clk name for the device
- * @dev: Device for which clk name is being set.
- * @name: Clk name.
+ * devm_pm_opp_set_clknames() - Set clock names for the device
+ * @dev: Device for which clock names are being set.
+ * @names: Array of pointers to the names of the clocks.
+ * @count: Number of clocks.
  *
- * This is a resource-managed variant of dev_pm_opp_set_clkname().
+ * This is a resource-managed variant of dev_pm_opp_set_clknames().
  *
  * Return: 0 on success and errorno otherwise.
  */
-int devm_pm_opp_set_clkname(struct device *dev, const char *name)
+int devm_pm_opp_set_clknames(struct device *dev,
+			     const char * const names[],
+			     unsigned int count)
 {
 	struct opp_table *opp_table;
 
-	opp_table = dev_pm_opp_set_clkname(dev, name);
+	opp_table = dev_pm_opp_set_clknames(dev, names, count);
 	if (IS_ERR(opp_table))
 		return PTR_ERR(opp_table);
 
-	return devm_add_action_or_reset(dev, devm_pm_opp_clkname_release,
+	return devm_add_action_or_reset(dev, devm_pm_opp_clknames_release,
 					opp_table);
 }
-EXPORT_SYMBOL_GPL(devm_pm_opp_set_clkname);
+EXPORT_SYMBOL_GPL(devm_pm_opp_set_clknames);
 
 /**
  * dev_pm_opp_register_set_opp_helper() - Register custom set OPP helper
@@ -2756,7 +2848,8 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
 	if (IS_ERR(opp_table))
 		return PTR_ERR(opp_table);
 
-	/* Fix regulator count for dynamic OPPs */
+	/* Fix regulator/clk count for dynamic OPPs */
+	opp_table->clk_count = 1;
 	opp_table->regulator_count = 1;
 
 	ret = _opp_add_v1(opp_table, dev, freq, u_volt, true);
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index 45e3a55239a1..59aac08baf82 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -149,7 +149,9 @@ enum opp_table_access {
  * @supported_hw: Array of version number to support.
  * @supported_hw_count: Number of elements in supported_hw array.
  * @prop_name: A name to postfix to many DT properties, while parsing them.
- * @clk: Device's clock handle
+ * @clks: Device clocks handles (clk bulk API).
+ * @clk_count: Number of clocks. Its value can be -1 (uninitialized), 0 (no
+ * clock handle provided)  or > 0 (has clock handles).
  * @regulators: Supply regulators
  * @regulator_count: Number of power supply regulators. Its value can be -1
  * (uninitialized), 0 (no opp-microvolt property) or > 0 (has opp-microvolt
@@ -200,7 +202,8 @@ struct opp_table {
 	unsigned int *supported_hw;
 	unsigned int supported_hw_count;
 	const char *prop_name;
-	struct clk *clk;
+	struct clk_bulk_data *clks;
+	int clk_count;
 	struct regulator **regulators;
 	int regulator_count;
 	struct icc_path **paths;
diff --git a/drivers/opp/ti-opp-supply.c b/drivers/opp/ti-opp-supply.c
index bd4771f388ab..ebb6531f39c4 100644
--- a/drivers/opp/ti-opp-supply.c
+++ b/drivers/opp/ti-opp-supply.c
@@ -281,12 +281,16 @@ static int ti_opp_supply_set_opp(struct dev_pm_set_opp_data *data)
 	struct dev_pm_opp_supply *new_supply_vbb = &data->new_opp.supplies[1];
 	struct device *dev = data->dev;
 	unsigned long old_freq = data->old_opp.rate, freq = data->new_opp.rate;
-	struct clk *clk = data->clk;
 	struct regulator *vdd_reg = data->regulators[0];
 	struct regulator *vbb_reg = data->regulators[1];
+	struct clk *clk;
 	int vdd_uv;
 	int ret;
 
+	if (WARN_ON_ONCE(!data->clk_count))
+		return -EINVAL;
+
+	clk = data->clks[0].clk;
 	vdd_uv = _get_optimal_vdd_voltage(dev, &opp_data,
 					  new_supply_vdd->u_volt);
 
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 6708b4ec244d..5f17f10fcc3f 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -75,7 +75,8 @@ struct dev_pm_opp_info {
  * @new_opp:	New OPP info
  * @regulators:	Array of regulator pointers
  * @regulator_count: Number of regulators
- * @clk:	Pointer to clk
+ * @clks:	Device clocks handles (clk bulk API)
+ * @clk_count:	Number of clocks
  * @dev:	Pointer to the struct device
  *
  * This structure contains all information required for setting an OPP.
@@ -86,7 +87,8 @@ struct dev_pm_set_opp_data {
 
 	struct regulator **regulators;
 	unsigned int regulator_count;
-	struct clk *clk;
+	struct clk_bulk_data *clks;
+	unsigned int clk_count;
 	struct device *dev;
 };
 
@@ -165,6 +167,13 @@ int devm_pm_opp_set_regulators(struct device *dev, const char * const names[], u
 struct opp_table *dev_pm_opp_set_clkname(struct device *dev, const char *name);
 void dev_pm_opp_put_clkname(struct opp_table *opp_table);
 int devm_pm_opp_set_clkname(struct device *dev, const char *name);
+struct opp_table *dev_pm_opp_set_clknames(struct device *dev,
+					  const char * const names[],
+					  unsigned int count);
+void dev_pm_opp_put_clknames(struct opp_table *opp_table);
+int devm_pm_opp_set_clknames(struct device *dev,
+			     const char * const names[],
+			     unsigned int count);
 struct opp_table *dev_pm_opp_register_set_opp_helper(struct device *dev, int (*set_opp)(struct dev_pm_set_opp_data *data));
 void dev_pm_opp_unregister_set_opp_helper(struct opp_table *opp_table);
 int devm_pm_opp_register_set_opp_helper(struct device *dev, int (*set_opp)(struct dev_pm_set_opp_data *data));
@@ -405,6 +414,22 @@ static inline int devm_pm_opp_set_clkname(struct device *dev, const char *name)
 	return -EOPNOTSUPP;
 }
 
+static inline struct opp_table *dev_pm_opp_set_clknames(struct device *dev,
+							const char * const names[],
+							unsigned int count)
+{
+	return ERR_PTR(-EOPNOTSUPP);
+}
+
+static inline void dev_pm_opp_put_clknames(struct opp_table *opp_table) {}
+
+static inline int devm_pm_opp_set_clknames(struct device *dev,
+					   const char * const names[],
+					   unsigned int count)
+{
+	return -EOPNOTSUPP;
+}
+
 static inline struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, const char * const *names, struct device ***virt_devs)
 {
 	return ERR_PTR(-EOPNOTSUPP);
-- 
2.32.0

[PATCH v3 6/7] PM: opp: parse multiple frequencies in each OPP

[Krzysztof Kozlowski]

Devices might need to control several clocks when scaling the frequency
and voltage, with each clock having different frequency.  Parse the
'opp-hz' Devicetree property as an array and store all frequencies for
the custom set_opp in a new field 'rates'.

Since the PM OPP framework operates on only one clock (multiple clocks
are for custom set_opp helpers), these frequencies are actually not used
internally and still the core relies on the original field 'rate'.

All other PM OPP functions, like finding floor/ceil PM OPPs, operate
still on first rate given in the 'opp-hz' frequency.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

---

Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/opp/core.c     | 20 +++++++++++++++---
 drivers/opp/of.c       | 47 ++++++++++++++++++++++++++++++++++++++++++
 drivers/opp/opp.h      |  2 ++
 include/linux/pm_opp.h |  3 +++
 4 files changed, 69 insertions(+), 3 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index aac3c6d89ae0..563e962c3411 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1033,7 +1033,9 @@ static int _set_opp_custom(const struct opp_table *opp_table,
 	data->clks = opp_table->clks;
 	data->dev = dev;
 	data->old_opp.rate = old_opp->rate;
+	data->old_opp.rates = old_opp->rates;
 	data->new_opp.rate = freq;
+	data->new_opp.rates = opp->rates;
 
 	return opp_table->set_opp(data);
 }
@@ -1307,6 +1309,11 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 				__func__, freq, ret);
 			goto put_opp_table;
 		}
+		/*
+		 * opp->rates are used for scaling clocks, so be sure accurate
+		 * 'freq' is used, instead what was defined via e.g. Devicetree.
+		 */
+		opp->rates[0] = freq;
 	}
 
 	ret = _set_opp(dev, opp_table, opp, freq);
@@ -1761,23 +1768,29 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_remove_all_dynamic);
 struct dev_pm_opp *_opp_allocate(struct opp_table *table)
 {
 	struct dev_pm_opp *opp;
-	int supply_count, supply_size, icc_size;
+	int rate_count, rate_size, supply_count, supply_size, icc_size;
 
 	/* Allocate space for at least one supply */
 	supply_count = table->regulator_count > 0 ? table->regulator_count : 1;
 	supply_size = sizeof(*opp->supplies) * supply_count;
+	/* Allocate space for at least one rate */
+	rate_count = table->clk_count > 0 ? table->clk_count : 1;
+	rate_size = sizeof(*opp->rates) * rate_count;
 	icc_size = sizeof(*opp->bandwidth) * table->path_count;
 
 	/* allocate new OPP node and supplies structures */
-	opp = kzalloc(sizeof(*opp) + supply_size + icc_size, GFP_KERNEL);
+	opp = kzalloc(sizeof(*opp) + rate_size + supply_size + icc_size,
+		      GFP_KERNEL);
 
 	if (!opp)
 		return NULL;
 
 	/* Put the supplies at the end of the OPP structure as an empty array */
 	opp->supplies = (struct dev_pm_opp_supply *)(opp + 1);
+	opp->rates = (unsigned long *)(opp->supplies + supply_count);
 	if (icc_size)
-		opp->bandwidth = (struct dev_pm_opp_icc_bw *)(opp->supplies + supply_count);
+		opp->bandwidth = (struct dev_pm_opp_icc_bw *)(opp->rates + rate_count);
+
 	INIT_LIST_HEAD(&opp->node);
 
 	return opp;
@@ -1957,6 +1970,7 @@ int _opp_add_v1(struct opp_table *opp_table, struct device *dev,
 
 	/* populate the opp table */
 	new_opp->rate = freq;
+	new_opp->rates[0] = freq;
 	tol = u_volt * opp_table->voltage_tolerance_v1 / 100;
 	new_opp->supplies[0].u_volt = u_volt;
 	new_opp->supplies[0].u_volt_min = u_volt - tol;
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index 30394929d700..b226d2c7a84b 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -767,6 +767,46 @@ void dev_pm_opp_of_remove_table(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_of_remove_table);
 
+static int _read_clocks(struct dev_pm_opp *opp, struct opp_table *opp_table,
+			struct device_node *np)
+{
+	int i, count, ret;
+	u64 *freq;
+
+	count = of_property_count_u64_elems(np, "opp-hz");
+	if (count < 0) {
+		pr_err("%s: Invalid %s property (%d)\n",
+			__func__, of_node_full_name(np), count);
+		return count;
+	}
+
+	if (count != opp_table->clk_count) {
+		pr_err("%s: number of rates %d does not match number of clocks %d in %pOF\n",
+		       __func__, count, opp_table->clk_count, np);
+		return -EINVAL;
+	}
+
+	freq = kmalloc_array(count, sizeof(*freq), GFP_KERNEL);
+	if (!freq)
+		return -ENOMEM;
+
+	ret = of_property_read_u64_array(np, "opp-hz", freq, count);
+	if (ret) {
+		pr_err("%s: error parsing %s: %d\n", __func__,
+		       of_node_full_name(np), ret);
+		ret = -EINVAL;
+		goto free_freq;
+	}
+
+	for (i = 0; i < count; i++)
+		opp->rates[i] = freq[i];
+
+free_freq:
+	kfree(freq);
+
+	return ret;
+}
+
 static int _read_bw(struct dev_pm_opp *new_opp, struct opp_table *table,
 		    struct device_node *np, bool peak)
 {
@@ -827,6 +867,13 @@ static int _read_opp_key(struct dev_pm_opp *new_opp, struct opp_table *table,
 	}
 	*rate_not_available = !!ret;
 
+	if (!ret) {
+		ret = _read_clocks(new_opp, table, np);
+		/* The properties were found but we failed to parse them */
+		if (ret && ret != -ENODEV)
+			return ret;
+	}
+
 	/*
 	 * Bandwidth consists of peak and average (optional) values:
 	 * opp-peak-kBps = <path1_value path2_value>;
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index 59aac08baf82..217d4376af9b 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -61,6 +61,7 @@ extern struct list_head opp_tables, lazy_opp_tables;
  * @rate:	Frequency in hertz
  * @level:	Performance level
  * @supplies:	Power supplies voltage/current values
+ * @rates:	Frequency rates for the clocks (equal to @rate for one clock)
  * @bandwidth:	Interconnect bandwidth values
  * @clock_latency_ns: Latency (in nanoseconds) of switching to this OPP's
  *		frequency from any other OPP's frequency.
@@ -85,6 +86,7 @@ struct dev_pm_opp {
 	unsigned int level;
 
 	struct dev_pm_opp_supply *supplies;
+	unsigned long *rates;
 	struct dev_pm_opp_icc_bw *bandwidth;
 
 	unsigned long clock_latency_ns;
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 5f17f10fcc3f..5fd3895af605 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -60,12 +60,15 @@ struct dev_pm_opp_icc_bw {
 /**
  * struct dev_pm_opp_info - OPP freq/voltage/current values
  * @rate:	Target clk rate in hz
+ * @rates:	Array of clock frequencies for all clocks (equal to @rate for
+ *		one clock)
  * @supplies:	Array of voltage/current values for all power supplies
  *
  * This structure stores the freq/voltage/current values for a single OPP.
  */
 struct dev_pm_opp_info {
 	unsigned long rate;
+	unsigned long *rates;
 	struct dev_pm_opp_supply *supplies;
 };
 
-- 
2.32.0

[PATCH v3 7/7] ufs: use PM OPP when scaling gears

[Krzysztof Kozlowski]

Scaling gears requires not only scaling clocks, but also voltage levels,
e.g. via performance states.

Use the provided OPP table, to set proper OPP frequency which through
required-opps will trigger performance state change.  This deprecates
the old freq-table-hz Devicetree property and old clock scaling method
in favor of PM core code.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

---

Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/scsi/ufs/ufshcd-pltfrm.c |  73 +++++++++++++++
 drivers/scsi/ufs/ufshcd.c        | 150 ++++++++++++++++++++++++-------
 drivers/scsi/ufs/ufshcd.h        |   6 ++
 3 files changed, 195 insertions(+), 34 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c
index 3ab555f6e66e..a603ca8e383b 100644
--- a/drivers/scsi/ufs/ufshcd-pltfrm.c
+++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
@@ -10,6 +10,7 @@
 
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/pm_opp.h>
 #include <linux/pm_runtime.h>
 #include <linux/of.h>
 
@@ -108,6 +109,72 @@ static int ufshcd_parse_clock_info(struct ufs_hba *hba)
 	return ret;
 }
 
+static int ufshcd_parse_operating_points(struct ufs_hba *hba)
+{
+	struct device *dev = hba->dev;
+	struct device_node *np = dev->of_node;
+	struct ufs_clk_info *clki;
+	const char *names[16];
+	int cnt, i, ret;
+
+	if (!of_find_property(dev->of_node, "operating-points-v2", NULL))
+		return 0;
+
+	cnt = of_property_count_strings(np, "clock-names");
+	if (cnt <= 0) {
+		dev_warn(dev, "%s: Missing clock-names\n",
+			 __func__);
+		return -EINVAL;
+	}
+
+	if (cnt > ARRAY_SIZE(names)) {
+		dev_info(dev, "%s: Too many clock-names\n",  __func__);
+		return -EINVAL;
+	}
+
+	if (of_find_property(np, "freq-table-hz", NULL)) {
+		dev_info(dev, "%s: operating-points and freq-table-hz are incompatible\n",
+			 __func__);
+		return -EINVAL;
+	}
+
+	for (i = 0; i < cnt; i++) {
+		ret = of_property_read_string_index(np, "clock-names", i,
+						    &names[i]);
+		if (ret)
+			return ret;
+
+		clki = devm_kzalloc(dev, sizeof(*clki), GFP_KERNEL);
+		if (!clki)
+			return -ENOMEM;
+
+		clki->name = devm_kstrdup(dev, names[i], GFP_KERNEL);
+		if (!clki->name)
+			return -ENOMEM;
+
+		if (!strcmp(names[i], "ref_clk"))
+			clki->keep_link_active = true;
+
+		list_add_tail(&clki->list, &hba->clk_list_head);
+	}
+
+	ret = devm_pm_opp_set_clknames(dev, names, i);
+	if (ret)
+		return ret;
+
+	ret = devm_pm_opp_register_set_opp_helper(dev, ufshcd_set_opp);
+	if (ret)
+		return ret;
+
+	ret = devm_pm_opp_of_add_table(dev);
+	if (ret)
+		return ret;
+
+	hba->use_pm_opp = true;
+
+	return 0;
+}
+
 #define MAX_PROP_SIZE 32
 static int ufshcd_populate_vreg(struct device *dev, const char *name,
 		struct ufs_vreg **out_vreg)
@@ -363,6 +430,12 @@ int ufshcd_pltfrm_init(struct platform_device *pdev,
 		goto dealloc_host;
 	}
 
+	err = ufshcd_parse_operating_points(hba);
+	if (err) {
+		dev_err(dev, "%s: OPP parse failed %d\n", __func__, err);
+		goto dealloc_host;
+	}
+
 	ufshcd_init_lanes_per_dir(hba);
 
 	err = ufshcd_init(hba, mmio_base, irq);
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index ea43aa2d26e1..9f07c4fd0087 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/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 <scsi/scsi_cmnd.h>
 #include <scsi/scsi_dbg.h>
@@ -259,7 +260,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, bool scale_up,
+			     unsigned long freq);
 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);
@@ -962,7 +964,7 @@ static bool ufshcd_is_unipro_pa_params_tuning_req(struct ufs_hba *hba)
 }
 
 /**
- * ufshcd_set_clk_freq - set UFS controller clock frequencies
+ * ufshcd_set_clk_freq - set UFS controller clock frequencies (directly)
  * @hba: per adapter instance
  * @scale_up: If True, set max possible frequency othewise set low frequency
  *
@@ -1024,15 +1026,48 @@ static int ufshcd_set_clk_freq(struct ufs_hba *hba, bool scale_up)
 	return ret;
 }
 
+/**
+ * ufshcd_set_opp - scale UFS controller clocks, called via PM OPP
+ * @data: PM OPP data
+ *
+ * Returns 0 for success, non-zero error value for errors.
+ */
+int ufshcd_set_opp(struct dev_pm_set_opp_data *data)
+{
+	struct ufs_hba *hba = dev_get_drvdata(data->dev);
+	int i, ret;
+
+	if (!data->clk_count)
+		return 0;
+
+	for (i = 0; i < data->clk_count; i++) {
+		if (!data->new_opp.rates[i])
+			continue;
+
+		ret = clk_set_rate(data->clks[i].clk, data->new_opp.rates[i]);
+		if (ret) {
+			dev_err(hba->dev, "%s: %pC clk set rate(%luHz) failed, %d\n",
+				__func__, data->clks[i].clk,
+				data->new_opp.rates[i], ret);
+			break;
+		}
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(ufshcd_set_opp);
+
 /**
  * ufshcd_scale_clks - scale up or scale down UFS controller clocks
  * @hba: per adapter instance
  * @scale_up: True if scaling up and false if scaling down
+ * @freq: Target frequency
  *
  * 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, bool scale_up,
+			     unsigned long freq)
 {
 	int ret = 0;
 	ktime_t start = ktime_get();
@@ -1041,13 +1076,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 = dev_pm_opp_set_rate(hba->dev, 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)
+			dev_pm_opp_set_rate(hba->dev,
+					    hba->devfreq->previous_freq);
+		else
+			ufshcd_set_clk_freq(hba, !scale_up);
+	}
 
 out:
 	trace_ufshcd_profile_clk_scaling(dev_name(hba->dev),
@@ -1059,11 +1102,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: Target frequency
  * @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,
+					       unsigned long freq,
 					       bool scale_up)
 {
 	struct ufs_clk_info *clki;
@@ -1072,6 +1117,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) {
@@ -1251,13 +1299,15 @@ static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, bool writelock)
 /**
  * ufshcd_devfreq_scale - scale up/down UFS clocks and gear
  * @hba: per adapter instance
+ * @freq: Target frequency
  * @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;
 	bool is_writelock = true;
@@ -1273,7 +1323,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, scale_up, freq);
 	if (ret) {
 		if (!scale_up)
 			ufshcd_scale_gear(hba, true);
@@ -1284,7 +1334,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, false,
+					  hba->devfreq->previous_freq);
 			goto out_unprepare;
 		}
 	}
@@ -1347,9 +1398,20 @@ 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;
+
+		opp = devfreq_recommended_opp(dev, freq, flags);
+		if (IS_ERR(opp))
+			return PTR_ERR(opp);
+		dev_pm_opp_put(opp);
+	} else {
+		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);
@@ -1365,11 +1427,11 @@ static int ufshcd_devfreq_target(struct device *dev,
 	}
 
 	/* Decide based on the rounded-off frequency and update */
-	scale_up = *freq == clki->max_freq;
-	if (!scale_up)
+	scale_up = (*freq > hba->clk_scaling.target_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 */
@@ -1377,7 +1439,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"),
@@ -1397,8 +1461,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))
@@ -1411,13 +1473,20 @@ 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);
+		/*
+		 * 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 (scaling->is_busy_started)
 		scaling->tot_busy_t += ktime_us_delta(curr_t,
 				scaling->busy_start_t);
@@ -1450,9 +1519,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);
@@ -1464,8 +1535,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;
 	}
 
@@ -1477,7 +1550,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;
@@ -1485,9 +1557,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)
@@ -1571,8 +1647,14 @@ static ssize_t ufshcd_clkscale_enable_store(struct device *dev,
 	if (value) {
 		ufshcd_resume_clkscaling(hba);
 	} else {
+		struct dev_pm_opp *opp;
+		unsigned long freq = ULONG_MAX;
+
+		opp = dev_pm_opp_find_freq_floor(dev, &freq);
+		dev_pm_opp_put(opp);
+
 		ufshcd_suspend_clkscaling(hba);
-		err = ufshcd_devfreq_scale(hba, true);
+		err = ufshcd_devfreq_scale(hba, freq, true);
 		if (err)
 			dev_err(hba->dev, "%s: failed to scale clocks up %d\n",
 					__func__, err);
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 0b8d0192f069..6962d36e6c65 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -400,6 +400,7 @@ struct ufs_saved_pwr_info {
  * @is_initialized: Indicates whether clock scaling is initialized or not
  * @is_busy_started: tracks if busy period has started or not
  * @is_suspended: tracks if devfreq is suspended or not
+ * @target_freq: frequency requested by devfreq framework
  */
 struct ufs_clk_scaling {
 	int active_reqs;
@@ -417,6 +418,7 @@ struct ufs_clk_scaling {
 	bool is_initialized;
 	bool is_busy_started;
 	bool is_suspended;
+	unsigned long target_freq;
 };
 
 #define UFS_EVENT_HIST_LENGTH 8
@@ -771,6 +773,8 @@ struct ufs_hba_monitor {
  * @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: whether OPP table is provided and scaling gears should trigger
+ *              setting OPP
  * @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.
@@ -906,6 +910,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;
@@ -1037,6 +1042,7 @@ void ufshcd_remove(struct ufs_hba *);
 int ufshcd_uic_hibern8_enter(struct ufs_hba *hba);
 int ufshcd_uic_hibern8_exit(struct ufs_hba *hba);
 void ufshcd_delay_us(unsigned long us, unsigned long tolerance);
+int ufshcd_set_opp(struct dev_pm_set_opp_data *data);
 void ufshcd_parse_dev_ref_clk_freq(struct ufs_hba *hba, struct clk *refclk);
 void ufshcd_update_evt_hist(struct ufs_hba *hba, u32 id, u32 val);
 void ufshcd_hba_stop(struct ufs_hba *hba);
-- 
2.32.0

Re: [PATCH v3 1/7] dt-bindings: clock: qcom,gcc-sdm845: add parent power domain

[Manivannan Sadhasivam]

On Fri, May 13, 2022 at 08:13:41AM +0200, Krzysztof Kozlowski wrote:
> Allow Qualcomm GCC to register its parent power domain (e.g. RPMHPD) to
> properly pass performance state from children.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thanks,
Mani

> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Acked-by: Rob Herring <robh@kernel.org>
> 
> ---
> 
> Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  Documentation/devicetree/bindings/clock/qcom,gcc-sdm845.yaml | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/clock/qcom,gcc-sdm845.yaml b/Documentation/devicetree/bindings/clock/qcom,gcc-sdm845.yaml
> index d902f137ab17..daf7906ebc40 100644
> --- a/Documentation/devicetree/bindings/clock/qcom,gcc-sdm845.yaml
> +++ b/Documentation/devicetree/bindings/clock/qcom,gcc-sdm845.yaml
> @@ -43,6 +43,9 @@ properties:
>    '#reset-cells':
>      const: 1
>  
> +  power-domains:
> +    maxItems: 1
> +
>    '#power-domain-cells':
>      const: 1
>  
> -- 
> 2.32.0
> 

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

Re: [PATCH v3 2/7] dt-bindings: opp: accept array of frequencies

[Manivannan Sadhasivam]

On Fri, May 13, 2022 at 08:13:42AM +0200, Krzysztof Kozlowski wrote:
> Devices might need to control several clocks when scaling the frequency
> and voltage.  Allow passing array of clock frequencies, similarly to the
> voltages.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thanks,
Mani

> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Acked-by: Rob Herring <robh@kernel.org>
> 
> ---
> 
> Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  Documentation/devicetree/bindings/opp/opp-v2-base.yaml | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml
> index 76c8acd981b3..66d0ec763f0b 100644
> --- a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml
> +++ b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml
> @@ -50,6 +50,16 @@ patternProperties:
>            property to uniquely identify the OPP nodes exists. Devices like power
>            domains must have another (implementation dependent) property.
>  
> +          Entries for multiple clocks shall be provided in the same field, as
> +          array of frequencies.  The OPP binding doesn't provide any provisions
> +          to relate the values to their clocks or the order in which the clocks
> +          need to be configured and that is left for the implementation
> +          specific binding.
> +        minItems: 1
> +        maxItems: 16
> +        items:
> +          maxItems: 1
> +
>        opp-microvolt:
>          description: |
>            Voltage for the OPP
> -- 
> 2.32.0
> 

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

Re: [PATCH v3 3/7] dt-bindings: ufs: common: add OPP table

[Manivannan Sadhasivam]

On Fri, May 13, 2022 at 08:13:43AM +0200, Krzysztof Kozlowski wrote:
> 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>
> 
> ---
> 
> Cc: 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.

This description mentions only the clocks and not voltages. But in theory, the
OPP table can contain other parameters like current, bandwidth, etc,... So to
avoid confusion, I'd suggest to get rid of the description.

> +
> +  opp-table: true
>  
>    interrupts:
>      maxItems: 1
> @@ -75,8 +88,23 @@ properties:
>  
>  dependencies:
>    freq-table-hz: [ 'clocks' ]
> +  operating-points-v2: [ 'clocks', 'clock-names' ]

What about voltage regulators if relevant opp property is present?

Thanks,
Mani

>  
>  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.32.0
> 

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

Re: [PATCH v3 7/7] ufs: use PM OPP when scaling gears

[Manivannan Sadhasivam]

On Fri, May 13, 2022 at 08:13:47AM +0200, Krzysztof Kozlowski wrote:
> Scaling gears requires not only scaling clocks, but also voltage levels,
> e.g. via performance states.
> 
> Use the provided OPP table, to set proper OPP frequency which through
> required-opps will trigger performance state change.  This deprecates
> the old freq-table-hz Devicetree property and old clock scaling method
> in favor of PM core code.
> 

To be clear, you are not changing the voltages (UFS supplies) through OPP. But
rather handle only clks and leave the power domain handling to parent OPP
device.

> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> 
> ---
> 
> Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/scsi/ufs/ufshcd-pltfrm.c |  73 +++++++++++++++
>  drivers/scsi/ufs/ufshcd.c        | 150 ++++++++++++++++++++++++-------
>  drivers/scsi/ufs/ufshcd.h        |   6 ++
>  3 files changed, 195 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c
> index 3ab555f6e66e..a603ca8e383b 100644
> --- a/drivers/scsi/ufs/ufshcd-pltfrm.c
> +++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
> @@ -10,6 +10,7 @@
>  
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm_opp.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/of.h>
>  
> @@ -108,6 +109,72 @@ static int ufshcd_parse_clock_info(struct ufs_hba *hba)
>  	return ret;
>  }
>  
> +static int ufshcd_parse_operating_points(struct ufs_hba *hba)
> +{
> +	struct device *dev = hba->dev;
> +	struct device_node *np = dev->of_node;
> +	struct ufs_clk_info *clki;
> +	const char *names[16];
> +	int cnt, i, ret;
> +
> +	if (!of_find_property(dev->of_node, "operating-points-v2", NULL))
> +		return 0;
> +
> +	cnt = of_property_count_strings(np, "clock-names");
> +	if (cnt <= 0) {
> +		dev_warn(dev, "%s: Missing clock-names\n",
> +			 __func__);

This is a hard error, right? So why not dev_err()?

> +		return -EINVAL;
> +	}
> +
> +	if (cnt > ARRAY_SIZE(names)) {
> +		dev_info(dev, "%s: Too many clock-names\n",  __func__);

dev_err()?

> +		return -EINVAL;
> +	}
> +
> +	if (of_find_property(np, "freq-table-hz", NULL)) {
> +		dev_info(dev, "%s: operating-points and freq-table-hz are incompatible\n",
> +			 __func__);

dev_err()?

> +		return -EINVAL;
> +	}
> +
> +	for (i = 0; i < cnt; i++) {
> +		ret = of_property_read_string_index(np, "clock-names", i,
> +						    &names[i]);
> +		if (ret)
> +			return ret;
> +
> +		clki = devm_kzalloc(dev, sizeof(*clki), GFP_KERNEL);
> +		if (!clki)
> +			return -ENOMEM;
> +
> +		clki->name = devm_kstrdup(dev, names[i], GFP_KERNEL);
> +		if (!clki->name)
> +			return -ENOMEM;
> +
> +		if (!strcmp(names[i], "ref_clk"))
> +			clki->keep_link_active = true;
> +
> +		list_add_tail(&clki->list, &hba->clk_list_head);
> +	}
> +
> +	ret = devm_pm_opp_set_clknames(dev, names, i);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_pm_opp_register_set_opp_helper(dev, ufshcd_set_opp);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_pm_opp_of_add_table(dev);
> +	if (ret)
> +		return ret;
> +
> +	hba->use_pm_opp = true;
> +

Since you are only handling the clks in UFS driver's OPP implementation, it
warrants atleast a comment. Otherwise, someone will add voltage to the OPP
table and complain that it is not getting changed. Eventhough the UFS driver
won't allow doing it, it is safer to mention it explicitly.

Also I'm worried about the implementation specific to Qcom platforms. Like we
rely on RPMHPD to handle the power domains, but that may not be true for other
platforms. I know that we cannot support all possible implementations but
atleast we should document this limitation.

Rest looks fine to me. I'll take one more look after testing this series on
SM8450.

Thanks,
Mani

> +	return 0;
> +}
> +
>  #define MAX_PROP_SIZE 32
>  static int ufshcd_populate_vreg(struct device *dev, const char *name,
>  		struct ufs_vreg **out_vreg)
> @@ -363,6 +430,12 @@ int ufshcd_pltfrm_init(struct platform_device *pdev,
>  		goto dealloc_host;
>  	}
>  
> +	err = ufshcd_parse_operating_points(hba);
> +	if (err) {
> +		dev_err(dev, "%s: OPP parse failed %d\n", __func__, err);
> +		goto dealloc_host;
> +	}
> +
>  	ufshcd_init_lanes_per_dir(hba);
>  
>  	err = ufshcd_init(hba, mmio_base, irq);
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index ea43aa2d26e1..9f07c4fd0087 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/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 <scsi/scsi_cmnd.h>
>  #include <scsi/scsi_dbg.h>
> @@ -259,7 +260,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, bool scale_up,
> +			     unsigned long freq);
>  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);
> @@ -962,7 +964,7 @@ static bool ufshcd_is_unipro_pa_params_tuning_req(struct ufs_hba *hba)
>  }
>  
>  /**
> - * ufshcd_set_clk_freq - set UFS controller clock frequencies
> + * ufshcd_set_clk_freq - set UFS controller clock frequencies (directly)
>   * @hba: per adapter instance
>   * @scale_up: If True, set max possible frequency othewise set low frequency
>   *
> @@ -1024,15 +1026,48 @@ static int ufshcd_set_clk_freq(struct ufs_hba *hba, bool scale_up)
>  	return ret;
>  }
>  
> +/**
> + * ufshcd_set_opp - scale UFS controller clocks, called via PM OPP
> + * @data: PM OPP data
> + *
> + * Returns 0 for success, non-zero error value for errors.
> + */
> +int ufshcd_set_opp(struct dev_pm_set_opp_data *data)
> +{
> +	struct ufs_hba *hba = dev_get_drvdata(data->dev);
> +	int i, ret;
> +
> +	if (!data->clk_count)
> +		return 0;
> +
> +	for (i = 0; i < data->clk_count; i++) {
> +		if (!data->new_opp.rates[i])
> +			continue;
> +
> +		ret = clk_set_rate(data->clks[i].clk, data->new_opp.rates[i]);
> +		if (ret) {
> +			dev_err(hba->dev, "%s: %pC clk set rate(%luHz) failed, %d\n",
> +				__func__, data->clks[i].clk,
> +				data->new_opp.rates[i], ret);
> +			break;
> +		}
> +	}
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(ufshcd_set_opp);
> +
>  /**
>   * ufshcd_scale_clks - scale up or scale down UFS controller clocks
>   * @hba: per adapter instance
>   * @scale_up: True if scaling up and false if scaling down
> + * @freq: Target frequency
>   *
>   * 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, bool scale_up,
> +			     unsigned long freq)
>  {
>  	int ret = 0;
>  	ktime_t start = ktime_get();
> @@ -1041,13 +1076,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 = dev_pm_opp_set_rate(hba->dev, 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)
> +			dev_pm_opp_set_rate(hba->dev,
> +					    hba->devfreq->previous_freq);
> +		else
> +			ufshcd_set_clk_freq(hba, !scale_up);
> +	}
>  
>  out:
>  	trace_ufshcd_profile_clk_scaling(dev_name(hba->dev),
> @@ -1059,11 +1102,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: Target frequency
>   * @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,
> +					       unsigned long freq,
>  					       bool scale_up)
>  {
>  	struct ufs_clk_info *clki;
> @@ -1072,6 +1117,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) {
> @@ -1251,13 +1299,15 @@ static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, bool writelock)
>  /**
>   * ufshcd_devfreq_scale - scale up/down UFS clocks and gear
>   * @hba: per adapter instance
> + * @freq: Target frequency
>   * @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;
>  	bool is_writelock = true;
> @@ -1273,7 +1323,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, scale_up, freq);
>  	if (ret) {
>  		if (!scale_up)
>  			ufshcd_scale_gear(hba, true);
> @@ -1284,7 +1334,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, false,
> +					  hba->devfreq->previous_freq);
>  			goto out_unprepare;
>  		}
>  	}
> @@ -1347,9 +1398,20 @@ 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;
> +
> +		opp = devfreq_recommended_opp(dev, freq, flags);
> +		if (IS_ERR(opp))
> +			return PTR_ERR(opp);
> +		dev_pm_opp_put(opp);
> +	} else {
> +		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);
> @@ -1365,11 +1427,11 @@ static int ufshcd_devfreq_target(struct device *dev,
>  	}
>  
>  	/* Decide based on the rounded-off frequency and update */
> -	scale_up = *freq == clki->max_freq;
> -	if (!scale_up)
> +	scale_up = (*freq > hba->clk_scaling.target_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 */
> @@ -1377,7 +1439,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"),
> @@ -1397,8 +1461,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))
> @@ -1411,13 +1473,20 @@ 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);
> +		/*
> +		 * 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 (scaling->is_busy_started)
>  		scaling->tot_busy_t += ktime_us_delta(curr_t,
>  				scaling->busy_start_t);
> @@ -1450,9 +1519,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);
> @@ -1464,8 +1535,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;
>  	}
>  
> @@ -1477,7 +1550,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;
> @@ -1485,9 +1557,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)
> @@ -1571,8 +1647,14 @@ static ssize_t ufshcd_clkscale_enable_store(struct device *dev,
>  	if (value) {
>  		ufshcd_resume_clkscaling(hba);
>  	} else {
> +		struct dev_pm_opp *opp;
> +		unsigned long freq = ULONG_MAX;
> +
> +		opp = dev_pm_opp_find_freq_floor(dev, &freq);
> +		dev_pm_opp_put(opp);
> +
>  		ufshcd_suspend_clkscaling(hba);
> -		err = ufshcd_devfreq_scale(hba, true);
> +		err = ufshcd_devfreq_scale(hba, freq, true);
>  		if (err)
>  			dev_err(hba->dev, "%s: failed to scale clocks up %d\n",
>  					__func__, err);
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 0b8d0192f069..6962d36e6c65 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -400,6 +400,7 @@ struct ufs_saved_pwr_info {
>   * @is_initialized: Indicates whether clock scaling is initialized or not
>   * @is_busy_started: tracks if busy period has started or not
>   * @is_suspended: tracks if devfreq is suspended or not
> + * @target_freq: frequency requested by devfreq framework
>   */
>  struct ufs_clk_scaling {
>  	int active_reqs;
> @@ -417,6 +418,7 @@ struct ufs_clk_scaling {
>  	bool is_initialized;
>  	bool is_busy_started;
>  	bool is_suspended;
> +	unsigned long target_freq;
>  };
>  
>  #define UFS_EVENT_HIST_LENGTH 8
> @@ -771,6 +773,8 @@ struct ufs_hba_monitor {
>   * @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: whether OPP table is provided and scaling gears should trigger
> + *              setting OPP
>   * @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.
> @@ -906,6 +910,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;
> @@ -1037,6 +1042,7 @@ void ufshcd_remove(struct ufs_hba *);
>  int ufshcd_uic_hibern8_enter(struct ufs_hba *hba);
>  int ufshcd_uic_hibern8_exit(struct ufs_hba *hba);
>  void ufshcd_delay_us(unsigned long us, unsigned long tolerance);
> +int ufshcd_set_opp(struct dev_pm_set_opp_data *data);
>  void ufshcd_parse_dev_ref_clk_freq(struct ufs_hba *hba, struct clk *refclk);
>  void ufshcd_update_evt_hist(struct ufs_hba *hba, u32 id, u32 val);
>  void ufshcd_hba_stop(struct ufs_hba *hba);
> -- 
> 2.32.0
> 

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

Re: [PATCH v3 3/7] dt-bindings: ufs: common: add OPP table

[Manivannan Sadhasivam]

On Fri, May 13, 2022 at 11:10:20PM +0530, Manivannan Sadhasivam wrote:
> On Fri, May 13, 2022 at 08:13:43AM +0200, Krzysztof Kozlowski wrote:
> > 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>
> > 
> > ---
> > 
> > Cc: 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.
> 
> This description mentions only the clocks and not voltages. But in theory, the
> OPP table can contain other parameters like current, bandwidth, etc,... So to
> avoid confusion, I'd suggest to get rid of the description.
> 
> > +
> > +  opp-table: true
> >  
> >    interrupts:
> >      maxItems: 1
> > @@ -75,8 +88,23 @@ properties:
> >  
> >  dependencies:
> >    freq-table-hz: [ 'clocks' ]
> > +  operating-points-v2: [ 'clocks', 'clock-names' ]
> 
> What about voltage regulators if relevant opp property is present?
> 

Current UFS driver model won't allow us to change both voltage supplies and clks
using OPP implementation. So please ignore my above comment.

Thanks,
Mani

> Thanks,
> Mani
> 
> >  
> >  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.32.0
> > 
> 
> -- 
> மணிவண்ணன் சதாசிவம்

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

Re: [PATCH v3 7/7] ufs: use PM OPP when scaling gears

[Krzysztof Kozlowski]

On 13/05/2022 20:25, Manivannan Sadhasivam wrote:
> On Fri, May 13, 2022 at 08:13:47AM +0200, Krzysztof Kozlowski wrote:
>> Scaling gears requires not only scaling clocks, but also voltage levels,
>> e.g. via performance states.
>>
>> Use the provided OPP table, to set proper OPP frequency which through
>> required-opps will trigger performance state change.  This deprecates
>> the old freq-table-hz Devicetree property and old clock scaling method
>> in favor of PM core code.
>>
> 
> To be clear, you are not changing the voltages (UFS supplies) through OPP. But
> rather handle only clks and leave the power domain handling to parent OPP
> device.

Correct, the patchset itself does not introduce itself regulator
control. For Qualcomm (and maybe others) these will be scaled via OPP
performance states.

> 
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>>
>> ---
>>
>> Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>> ---
>>  drivers/scsi/ufs/ufshcd-pltfrm.c |  73 +++++++++++++++
>>  drivers/scsi/ufs/ufshcd.c        | 150 ++++++++++++++++++++++++-------
>>  drivers/scsi/ufs/ufshcd.h        |   6 ++
>>  3 files changed, 195 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c
>> index 3ab555f6e66e..a603ca8e383b 100644
>> --- a/drivers/scsi/ufs/ufshcd-pltfrm.c
>> +++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
>> @@ -10,6 +10,7 @@
>>  
>>  #include <linux/module.h>
>>  #include <linux/platform_device.h>
>> +#include <linux/pm_opp.h>
>>  #include <linux/pm_runtime.h>
>>  #include <linux/of.h>
>>  
>> @@ -108,6 +109,72 @@ static int ufshcd_parse_clock_info(struct ufs_hba *hba)
>>  	return ret;
>>  }
>>  
>> +static int ufshcd_parse_operating_points(struct ufs_hba *hba)
>> +{
>> +	struct device *dev = hba->dev;
>> +	struct device_node *np = dev->of_node;
>> +	struct ufs_clk_info *clki;
>> +	const char *names[16];
>> +	int cnt, i, ret;
>> +
>> +	if (!of_find_property(dev->of_node, "operating-points-v2", NULL))
>> +		return 0;
>> +
>> +	cnt = of_property_count_strings(np, "clock-names");
>> +	if (cnt <= 0) {
>> +		dev_warn(dev, "%s: Missing clock-names\n",
>> +			 __func__);
> 
> This is a hard error, right? So why not dev_err()?

Good point, but actually this (and following cases) should be return 0,
because clocks/freq-table/opp-points are not required properties. The
original code (parsing it for freq-table-hz) also does not treat it as
error.

> 
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (cnt > ARRAY_SIZE(names)) {
>> +		dev_info(dev, "%s: Too many clock-names\n",  __func__);
> 
> dev_err()?
> 
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (of_find_property(np, "freq-table-hz", NULL)) {
>> +		dev_info(dev, "%s: operating-points and freq-table-hz are incompatible\n",
>> +			 __func__);
> 
> dev_err()?
> 
>> +		return -EINVAL;
>> +	}
>> +
>> +	for (i = 0; i < cnt; i++) {
>> +		ret = of_property_read_string_index(np, "clock-names", i,
>> +						    &names[i]);
>> +		if (ret)
>> +			return ret;
>> +
>> +		clki = devm_kzalloc(dev, sizeof(*clki), GFP_KERNEL);
>> +		if (!clki)
>> +			return -ENOMEM;
>> +
>> +		clki->name = devm_kstrdup(dev, names[i], GFP_KERNEL);
>> +		if (!clki->name)
>> +			return -ENOMEM;
>> +
>> +		if (!strcmp(names[i], "ref_clk"))
>> +			clki->keep_link_active = true;
>> +
>> +		list_add_tail(&clki->list, &hba->clk_list_head);
>> +	}
>> +
>> +	ret = devm_pm_opp_set_clknames(dev, names, i);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = devm_pm_opp_register_set_opp_helper(dev, ufshcd_set_opp);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = devm_pm_opp_of_add_table(dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	hba->use_pm_opp = true;
>> +
> 
> Since you are only handling the clks in UFS driver's OPP implementation, it
> warrants atleast a comment. Otherwise, someone will add voltage to the OPP
> table and complain that it is not getting changed. Eventhough the UFS driver
> won't allow doing it, it is safer to mention it explicitly.

Sure.

> 
> Also I'm worried about the implementation specific to Qcom platforms. Like we
> rely on RPMHPD to handle the power domains, but that may not be true for other
> platforms. I know that we cannot support all possible implementations but
> atleast we should document this limitation.
> 
> Rest looks fine to me. I'll take one more look after testing this series on
> SM8450.

Using OPPs is quite generic, so other platform could implement also
regulator scaling. The changes are indeed targetting Qcom platforms, but
they are not restricting any other usage.

Best regards,
Krzysztof

Re: [PATCH v3 4/7] arm64: dts: qcom: sdm845: control RPMHPD performance states with UFS

[Viresh Kumar]

On 13-05-22, 08:13, Krzysztof Kozlowski wrote:
> +			ufs_opp_table: opp-table {
> +				compatible = "operating-points-v2";
> +
> +				opp-50000000 {
> +					opp-hz = /bits/ 64 <50000000
> +						 0
> +						 0
> +						 37500000
> +						 0
> +						 0
> +						 0
> +						 0
> +						 // FIXME: value 0 copied from freq-table-hz
> +						 0>;

One general comment, I think this should follow how we specify
multiple voltages or other fields and so each frequency should be part
of a different < > braces. Like: opp-hz = /bits/ 64 <5000000>, <0>, ....

Whatever is there between < > seems to be connected, like
min/max/target for voltage.

The code will process both in a similar way though eventually.

-- 
viresh

Re: [PATCH v3 4/7] arm64: dts: qcom: sdm845: control RPMHPD performance states with UFS

[Krzysztof Kozlowski]

On 25/05/2022 09:16, Viresh Kumar wrote:
> On 13-05-22, 08:13, Krzysztof Kozlowski wrote:
>> +			ufs_opp_table: opp-table {
>> +				compatible = "operating-points-v2";
>> +
>> +				opp-50000000 {
>> +					opp-hz = /bits/ 64 <50000000
>> +						 0
>> +						 0
>> +						 37500000
>> +						 0
>> +						 0
>> +						 0
>> +						 0
>> +						 // FIXME: value 0 copied from freq-table-hz
>> +						 0>;
> 
> One general comment, I think this should follow how we specify
> multiple voltages or other fields and so each frequency should be part
> of a different < > braces. Like: opp-hz = /bits/ 64 <5000000>, <0>, ....
> 
> Whatever is there between < > seems to be connected, like
> min/max/target for voltage.
> 
> The code will process both in a similar way though eventually.

OK, I can change to such format.

Best regards,
Krzysztof

Re: (subset) [PATCH v3 1/7] dt-bindings: clock: qcom,gcc-sdm845: add parent power domain

[Bjorn Andersson]

On Fri, 13 May 2022 08:13:41 +0200, Krzysztof Kozlowski wrote:
> Allow Qualcomm GCC to register its parent power domain (e.g. RPMHPD) to
> properly pass performance state from children.
> 
> 

Applied, thanks!

[1/7] dt-bindings: clock: qcom,gcc-sdm845: add parent power domain
      commit: d62cac46b0184b8730c68b01359a33769fee821b

Best regards,
-- 
Bjorn Andersson <bjorn.andersson@linaro.org>

Re: [PATCH v3 2/7] dt-bindings: opp: accept array of frequencies

[Viresh Kumar]

On 13-05-22, 08:13, Krzysztof Kozlowski wrote:
> Devices might need to control several clocks when scaling the frequency
> and voltage.  Allow passing array of clock frequencies, similarly to the
> voltages.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Acked-by: Rob Herring <robh@kernel.org>
> 
> ---
> 
> Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  Documentation/devicetree/bindings/opp/opp-v2-base.yaml | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml
> index 76c8acd981b3..66d0ec763f0b 100644
> --- a/Documentation/devicetree/bindings/opp/opp-v2-base.yaml
> +++ b/Documentation/devicetree/bindings/opp/opp-v2-base.yaml
> @@ -50,6 +50,16 @@ patternProperties:
>            property to uniquely identify the OPP nodes exists. Devices like power
>            domains must have another (implementation dependent) property.
>  
> +          Entries for multiple clocks shall be provided in the same field, as
> +          array of frequencies.  The OPP binding doesn't provide any provisions
> +          to relate the values to their clocks or the order in which the clocks
> +          need to be configured and that is left for the implementation
> +          specific binding.
> +        minItems: 1
> +        maxItems: 16
> +        items:
> +          maxItems: 1
> +
>        opp-microvolt:
>          description: |
>            Voltage for the OPP

Applied. Thanks.

-- 
viresh