[PATCH v2 0/3] Support for GPU ACD feature on Adreno X1-85

[PATCH v2 0/3] Support for GPU ACD feature on Adreno X1-85

[Akhil P Oommen]

This series adds support for ACD feature for Adreno GPU which helps to
lower the power consumption on GX rail and also sometimes is a requirement
to enable higher GPU frequencies. At high level, following are the
sequences required for ACD feature:
	1. Identify the ACD level data for each regulator corner
	2. Send a message to AOSS to switch voltage plan
	3. Send a table with ACD level information to GMU during every
	gpu wake up

For (1), it is better to keep ACD level data in devicetree because this
value depends on the process node, voltage margins etc which are
chipset specific. For instance, same GPU HW IP on a different chipset
would have a different set of values. So, a new schema which extends
opp-v2 is created to add a new property called "qcom,opp-acd-level".

ACD support is dynamically detected based on the presence of
"qcom,opp-acd-level" property in GPU's opp table. Also, qmp node should be
present under GMU node in devicetree for communication with AOSS.

The devicetree patch in this series adds the acd-level data for X1-85
GPU present in Snapdragon X1 Elite chipset.

This series is rebased on top of drm-msm/msm-next.

---
Changes in v2:
- Removed RFC tag for the series
- Improve documentation for the new dt bindings (Krzysztof)
- Add fallback compatible string for opp-table (Krzysztof)
- Link to v1: https://lore.kernel.org/r/20241012-gpu-acd-v1-0-1e5e91aa95b6@quicinc.com

---
Akhil P Oommen (3):
      drm/msm/adreno: Add support for ACD
      dt-bindings: opp: Add v2-qcom-adreno vendor bindings
      arm64: dts: qcom: x1e80100: Add ACD levels for GPU

 .../bindings/opp/opp-v2-qcom-adreno.yaml           | 96 ++++++++++++++++++++++
 arch/arm64/boot/dts/qcom/x1e80100.dtsi             | 11 ++-
 drivers/gpu/drm/msm/adreno/a6xx_gmu.c              | 81 ++++++++++++++----
 drivers/gpu/drm/msm/adreno/a6xx_gmu.h              |  1 +
 drivers/gpu/drm/msm/adreno/a6xx_hfi.c              | 36 ++++++++
 drivers/gpu/drm/msm/adreno/a6xx_hfi.h              | 21 +++++
 6 files changed, 230 insertions(+), 16 deletions(-)
---
base-commit: a20a91fb1bfac5d05ec5bcf9afe0c9363f6c8c93
change-id: 20240724-gpu-acd-6c1dc5dcf516

Best regards,
-- 
Akhil P Oommen <quic_akhilpo@quicinc.com>

[PATCH v2 1/3] drm/msm/adreno: Add support for ACD

[Akhil P Oommen]

ACD a.k.a Adaptive Clock Distribution is a feature which helps to reduce
the power consumption. In some chipsets, it is also a requirement to
support higher GPU frequencies. This patch adds support for GPU ACD by
sending necessary data to GMU and AOSS. The feature support for the
chipset is detected based on devicetree data.

Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
---
 drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 81 ++++++++++++++++++++++++++++-------
 drivers/gpu/drm/msm/adreno/a6xx_gmu.h |  1 +
 drivers/gpu/drm/msm/adreno/a6xx_hfi.c | 36 ++++++++++++++++
 drivers/gpu/drm/msm/adreno/a6xx_hfi.h | 21 +++++++++
 4 files changed, 124 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index 37927bdd6fbe..09fb3f397dbb 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -1021,14 +1021,6 @@ int a6xx_gmu_resume(struct a6xx_gpu *a6xx_gpu)
 
 	gmu->hung = false;
 
-	/* Notify AOSS about the ACD state (unimplemented for now => disable it) */
-	if (!IS_ERR(gmu->qmp)) {
-		ret = qmp_send(gmu->qmp, "{class: gpu, res: acd, val: %d}",
-			       0 /* Hardcode ACD to be disabled for now */);
-		if (ret)
-			dev_err(gmu->dev, "failed to send GPU ACD state\n");
-	}
-
 	/* Turn on the resources */
 	pm_runtime_get_sync(gmu->dev);
 
@@ -1476,6 +1468,64 @@ static int a6xx_gmu_pwrlevels_probe(struct a6xx_gmu *gmu)
 	return a6xx_gmu_rpmh_votes_init(gmu);
 }
 
+static int a6xx_gmu_acd_probe(struct a6xx_gmu *gmu)
+{
+	struct a6xx_gpu *a6xx_gpu = container_of(gmu, struct a6xx_gpu, gmu);
+	struct a6xx_hfi_acd_table *cmd = &gmu->acd_table;
+	struct adreno_gpu *adreno_gpu = &a6xx_gpu->base;
+	struct msm_gpu *gpu = &adreno_gpu->base;
+	int ret, i, cmd_idx = 0;
+
+	cmd->version = 1;
+	cmd->stride = 1;
+	cmd->enable_by_level = 0;
+
+	/* Skip freq = 0 and parse acd-level for rest of the OPPs */
+	for (i = 1; i < gmu->nr_gpu_freqs; i++) {
+		struct dev_pm_opp *opp;
+		struct device_node *np;
+		unsigned long freq;
+		u32 val;
+
+		freq = gmu->gpu_freqs[i];
+		opp = dev_pm_opp_find_freq_exact(&gpu->pdev->dev, freq, true);
+		np = dev_pm_opp_get_of_node(opp);
+
+		ret = of_property_read_u32(np, "qcom,opp-acd-level", &val);
+		of_node_put(np);
+		dev_pm_opp_put(opp);
+		if (ret == -EINVAL)
+			continue;
+		else if (ret) {
+			DRM_DEV_ERROR(gmu->dev, "Unable to read acd level for freq %lu\n", freq);
+			return ret;
+		}
+
+		cmd->enable_by_level |= BIT(i);
+		cmd->data[cmd_idx++] = val;
+	}
+
+	cmd->num_levels = cmd_idx;
+
+	/* We are done here if ACD is not required for any of the OPPs */
+	if (!cmd->enable_by_level)
+		return 0;
+
+	/* Initialize qmp node to talk to AOSS */
+	gmu->qmp = qmp_get(gmu->dev);
+	if (IS_ERR(gmu->qmp)) {
+		cmd->enable_by_level = 0;
+		return dev_err_probe(gmu->dev, PTR_ERR(gmu->qmp), "Failed to initialize qmp\n");
+	}
+
+	/* Notify AOSS about the ACD state */
+	ret = qmp_send(gmu->qmp, "{class: gpu, res: acd, val: %d}", 1);
+	if (ret)
+		DRM_DEV_ERROR(gmu->dev, "failed to send GPU ACD state\n");
+
+	return 0;
+}
+
 static int a6xx_gmu_clocks_probe(struct a6xx_gmu *gmu)
 {
 	int ret = devm_clk_bulk_get_all(gmu->dev, &gmu->clocks);
@@ -1792,12 +1842,6 @@ int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct device_node *node)
 		goto detach_cxpd;
 	}
 
-	gmu->qmp = qmp_get(gmu->dev);
-	if (IS_ERR(gmu->qmp) && adreno_is_a7xx(adreno_gpu)) {
-		ret = PTR_ERR(gmu->qmp);
-		goto remove_device_link;
-	}
-
 	init_completion(&gmu->pd_gate);
 	complete_all(&gmu->pd_gate);
 	gmu->pd_nb.notifier_call = cxpd_notifier_cb;
@@ -1811,6 +1855,10 @@ int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct device_node *node)
 	/* Get the power levels for the GMU and GPU */
 	a6xx_gmu_pwrlevels_probe(gmu);
 
+	ret = a6xx_gmu_acd_probe(gmu);
+	if (ret)
+		goto detach_gxpd;
+
 	/* Set up the HFI queues */
 	a6xx_hfi_init(gmu);
 
@@ -1821,7 +1869,10 @@ int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct device_node *node)
 
 	return 0;
 
-remove_device_link:
+detach_gxpd:
+	if (!IS_ERR_OR_NULL(gmu->gxpd))
+		dev_pm_domain_detach(gmu->gxpd, false);
+
 	device_link_del(link);
 
 detach_cxpd:
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
index 94b6c5cab6f4..2690511149ed 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
@@ -81,6 +81,7 @@ struct a6xx_gmu {
 	int nr_gpu_freqs;
 	unsigned long gpu_freqs[16];
 	u32 gx_arc_votes[16];
+	struct a6xx_hfi_acd_table acd_table;
 
 	int nr_gmu_freqs;
 	unsigned long gmu_freqs[4];
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_hfi.c b/drivers/gpu/drm/msm/adreno/a6xx_hfi.c
index cdb3f6e74d3e..af94e339188b 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_hfi.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_hfi.c
@@ -659,6 +659,38 @@ static int a6xx_hfi_send_bw_table(struct a6xx_gmu *gmu)
 		NULL, 0);
 }
 
+#define HFI_FEATURE_ACD 12
+
+static int a6xx_hfi_enable_acd(struct a6xx_gmu *gmu)
+{
+	struct a6xx_hfi_acd_table *acd_table = &gmu->acd_table;
+	struct a6xx_hfi_msg_feature_ctrl msg = {
+		.feature = HFI_FEATURE_ACD,
+		.enable = 1,
+		.data = 0,
+	};
+	int ret;
+
+	if (!acd_table->enable_by_level)
+		return 0;
+
+	/* Enable ACD feature at GMU */
+	ret = a6xx_hfi_send_msg(gmu, HFI_H2F_FEATURE_CTRL, &msg, sizeof(msg), NULL, 0);
+	if (ret) {
+		DRM_DEV_ERROR(gmu->dev, "Unable to enable ACD (%d)\n", ret);
+		return ret;
+	}
+
+	/* Send ACD table to GMU */
+	ret = a6xx_hfi_send_msg(gmu, HFI_H2F_MSG_ACD, &msg, sizeof(msg), NULL, 0);
+	if (ret) {
+		DRM_DEV_ERROR(gmu->dev, "Unable to ACD table (%d)\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
 static int a6xx_hfi_send_test(struct a6xx_gmu *gmu)
 {
 	struct a6xx_hfi_msg_test msg = { 0 };
@@ -756,6 +788,10 @@ int a6xx_hfi_start(struct a6xx_gmu *gmu, int boot_state)
 	if (ret)
 		return ret;
 
+	ret = a6xx_hfi_enable_acd(gmu);
+	if (ret)
+		return ret;
+
 	ret = a6xx_hfi_send_core_fw_start(gmu);
 	if (ret)
 		return ret;
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_hfi.h b/drivers/gpu/drm/msm/adreno/a6xx_hfi.h
index 528110169398..51864c8ad0e6 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_hfi.h
+++ b/drivers/gpu/drm/msm/adreno/a6xx_hfi.h
@@ -151,12 +151,33 @@ struct a6xx_hfi_msg_test {
 	u32 header;
 };
 
+#define HFI_H2F_MSG_ACD 7
+#define MAX_ACD_STRIDE 2
+
+struct a6xx_hfi_acd_table {
+	u32 header;
+	u32 version;
+	u32 enable_by_level;
+	u32 stride;
+	u32 num_levels;
+	u32 data[16 * MAX_ACD_STRIDE];
+};
+
 #define HFI_H2F_MSG_START 10
 
 struct a6xx_hfi_msg_start {
 	u32 header;
 };
 
+#define HFI_H2F_FEATURE_CTRL 11
+
+struct a6xx_hfi_msg_feature_ctrl {
+	u32 header;
+	u32 feature;
+	u32 enable;
+	u32 data;
+};
+
 #define HFI_H2F_MSG_CORE_FW_START 14
 
 struct a6xx_hfi_msg_core_fw_start {

-- 
2.45.2

[PATCH v2 2/3] dt-bindings: opp: Add v2-qcom-adreno vendor bindings

[Akhil P Oommen]

Add a new schema which extends opp-v2 to support a new vendor specific
property required for Adreno GPUs found in Qualcomm's SoCs. The new
property called "qcom,opp-acd-level" carries a u32 value recommended
for each opp needs to be shared to GMU during runtime.

Cc: Rob Clark <robdclark@gmail.com>
Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
---
 .../bindings/opp/opp-v2-qcom-adreno.yaml           | 96 ++++++++++++++++++++++
 1 file changed, 96 insertions(+)

diff --git a/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml
new file mode 100644
index 000000000000..6d50c0405ef8
--- /dev/null
+++ b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml
@@ -0,0 +1,96 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/opp/opp-v2-qcom-adreno.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Adreno compatible OPP supply
+
+description:
+  Adreno GPUs present in Qualcomm's Snapdragon chipsets uses an OPP specific
+  ACD related information tailored for the specific chipset. This binding
+  provides the information needed to describe such a hardware value.
+
+maintainers:
+  - Rob Clark <robdclark@gmail.com>
+
+allOf:
+  - $ref: opp-v2-base.yaml#
+
+properties:
+  compatible:
+    items:
+      - const: operating-points-v2-adreno
+      - const: operating-points-v2
+
+patternProperties:
+  '^opp-?[0-9]+$':
+    type: object
+    additionalProperties: false
+
+    properties:
+      opp-hz: true
+
+      opp-level: true
+
+      opp-peak-kBps: true
+
+      opp-supported-hw: true
+
+      qcom,opp-acd-level:
+        description: |
+          A positive value representing the ACD (Adaptive Clock Distribution,
+          a fancy name for clk throttling during voltage droop) level associated
+          with this OPP node. This value is shared to a co-processor inside GPU
+          (called Graphics Management Unit a.k.a GMU) during wake up. It may not
+          be present for some OPPs and GMU will disable ACD while transitioning
+          to that OPP. This value encodes a voltage threshold and few other knobs
+          which are identified by characterization of the SoC. So, it doesn't have
+          any unit.
+        $ref: /schemas/types.yaml#/definitions/uint32
+
+    required:
+      - opp-hz
+      - opp-level
+
+required:
+  - compatible
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/power/qcom-rpmpd.h>
+
+    gpu_opp_table: opp-table {
+        compatible = "operating-points-v2-adreno", "operating-points-v2";
+
+        opp-687000000 {
+                opp-hz = /bits/ 64 <687000000>;
+                opp-level = <RPMH_REGULATOR_LEVEL_SVS_L1>;
+                opp-peak-kBps = <8171875>;
+                qcom,opp-acd-level = <0x882e5ffd>;
+        };
+
+        opp-550000000 {
+                opp-hz = /bits/ 64 <550000000>;
+                opp-level = <RPMH_REGULATOR_LEVEL_SVS>;
+                opp-peak-kBps = <6074219>;
+                qcom,opp-acd-level = <0xc0285ffd>;
+        };
+
+        opp-390000000 {
+                opp-hz = /bits/ 64 <390000000>;
+                opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
+                opp-peak-kBps = <3000000>;
+                qcom,opp-acd-level = <0xc0285ffd>;
+        };
+
+        opp-300000000 {
+                opp-hz = /bits/ 64 <300000000>;
+                opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS_D1>;
+                opp-peak-kBps = <2136719>;
+                /* Intentionally left out qcom,opp-acd-level property here */
+        };
+
+    };

-- 
2.45.2

[PATCH v2 3/3] arm64: dts: qcom: x1e80100: Add ACD levels for GPU

[Akhil P Oommen]

Update GPU node to include acd level values.

Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
---
 arch/arm64/boot/dts/qcom/x1e80100.dtsi | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
index a36076e3c56b..81ce8bccc7a5 100644
--- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
+++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
@@ -3323,60 +3323,69 @@ zap-shader {
 			};
 
 			gpu_opp_table: opp-table {
-				compatible = "operating-points-v2";
+				compatible = "operating-points-v2-adreno", "operating-points-v2";
 
 				opp-1100000000 {
 					opp-hz = /bits/ 64 <1100000000>;
 					opp-level = <RPMH_REGULATOR_LEVEL_TURBO_L1>;
 					opp-peak-kBps = <16500000>;
+					qcom,opp-acd-level = <0xa82a5ffd>;
 				};
 
 				opp-1000000000 {
 					opp-hz = /bits/ 64 <1000000000>;
 					opp-level = <RPMH_REGULATOR_LEVEL_TURBO>;
 					opp-peak-kBps = <14398438>;
+					qcom,opp-acd-level = <0xa82b5ffd>;
 				};
 
 				opp-925000000 {
 					opp-hz = /bits/ 64 <925000000>;
 					opp-level = <RPMH_REGULATOR_LEVEL_NOM_L1>;
 					opp-peak-kBps = <14398438>;
+					qcom,opp-acd-level = <0xa82b5ffd>;
 				};
 
 				opp-800000000 {
 					opp-hz = /bits/ 64 <800000000>;
 					opp-level = <RPMH_REGULATOR_LEVEL_NOM>;
 					opp-peak-kBps = <12449219>;
+					qcom,opp-acd-level = <0xa82c5ffd>;
 				};
 
 				opp-744000000 {
 					opp-hz = /bits/ 64 <744000000>;
 					opp-level = <RPMH_REGULATOR_LEVEL_SVS_L2>;
 					opp-peak-kBps = <10687500>;
+					qcom,opp-acd-level = <0x882e5ffd>;
 				};
 
 				opp-687000000 {
 					opp-hz = /bits/ 64 <687000000>;
 					opp-level = <RPMH_REGULATOR_LEVEL_SVS_L1>;
 					opp-peak-kBps = <8171875>;
+					qcom,opp-acd-level = <0x882e5ffd>;
 				};
 
 				opp-550000000 {
 					opp-hz = /bits/ 64 <550000000>;
 					opp-level = <RPMH_REGULATOR_LEVEL_SVS>;
 					opp-peak-kBps = <6074219>;
+					qcom,opp-acd-level = <0xc0285ffd>;
 				};
 
 				opp-390000000 {
 					opp-hz = /bits/ 64 <390000000>;
 					opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
 					opp-peak-kBps = <3000000>;
+					qcom,opp-acd-level = <0xc0285ffd>;
 				};
 
 				opp-300000000 {
 					opp-hz = /bits/ 64 <300000000>;
 					opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS_D1>;
 					opp-peak-kBps = <2136719>;
+					qcom,opp-acd-level = <0xc02b5ffd>;
 				};
 			};
 		};

-- 
2.45.2

Re: [PATCH v2 2/3] dt-bindings: opp: Add v2-qcom-adreno vendor bindings

[Krzysztof Kozlowski]

On Mon, Oct 21, 2024 at 05:23:43PM +0530, Akhil P Oommen wrote:
> Add a new schema which extends opp-v2 to support a new vendor specific
> property required for Adreno GPUs found in Qualcomm's SoCs. The new
> property called "qcom,opp-acd-level" carries a u32 value recommended
> for each opp needs to be shared to GMU during runtime.
> 
> Cc: Rob Clark <robdclark@gmail.com>
> Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
> ---
>  .../bindings/opp/opp-v2-qcom-adreno.yaml           | 96 ++++++++++++++++++++++
>  1 file changed, 96 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml
> new file mode 100644
> index 000000000000..6d50c0405ef8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml
> @@ -0,0 +1,96 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/opp/opp-v2-qcom-adreno.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Adreno compatible OPP supply
> +
> +description:
> +  Adreno GPUs present in Qualcomm's Snapdragon chipsets uses an OPP specific
> +  ACD related information tailored for the specific chipset. This binding
> +  provides the information needed to describe such a hardware value.
> +
> +maintainers:
> +  - Rob Clark <robdclark@gmail.com>
> +
> +allOf:
> +  - $ref: opp-v2-base.yaml#
> +
> +properties:
> +  compatible:
> +    items:
> +      - const: operating-points-v2-adreno
> +      - const: operating-points-v2
> +
> +patternProperties:
> +  '^opp-?[0-9]+$':

'-' should not be optional. opp1 is not expected name.

> +    type: object
> +    additionalProperties: false
> +
> +    properties:
> +      opp-hz: true
> +
> +      opp-level: true
> +
> +      opp-peak-kBps: true
> +
> +      opp-supported-hw: true
> +
> +      qcom,opp-acd-level:
> +        description: |
> +          A positive value representing the ACD (Adaptive Clock Distribution,
> +          a fancy name for clk throttling during voltage droop) level associated
> +          with this OPP node. This value is shared to a co-processor inside GPU
> +          (called Graphics Management Unit a.k.a GMU) during wake up. It may not
> +          be present for some OPPs and GMU will disable ACD while transitioning
> +          to that OPP. This value encodes a voltage threshold and few other knobs
> +          which are identified by characterization of the SoC. So, it doesn't have
> +          any unit.

Thanks for explanation and other updates. I am still not happy with this
property. I do not see reason why DT should encode magic values in a
quite generic piece of code. This creates poor ABI, difficult to
maintain or understand.


> +        $ref: /schemas/types.yaml#/definitions/uint32
> +
> +    required:
> +      - opp-hz
> +      - opp-level
> +
> +required:
> +  - compatible
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/power/qcom-rpmpd.h>
> +
> +    gpu_opp_table: opp-table {
> +        compatible = "operating-points-v2-adreno", "operating-points-v2";
> +
> +        opp-687000000 {
> +                opp-hz = /bits/ 64 <687000000>;

Messed indentation.

Best regards,
Krzysztof

Re: [PATCH v2 1/3] drm/msm/adreno: Add support for ACD

[Bryan O'Donoghue]

On 21/10/2024 12:53, Akhil P Oommen wrote:
> ACD a.k.a Adaptive Clock Distribution is a feature which helps to reduce
> the power consumption. In some chipsets, it is also a requirement to
> support higher GPU frequencies. This patch adds support for GPU ACD by
> sending necessary data to GMU and AOSS. The feature support for the
> chipset is detected based on devicetree data.
> 
> Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
> ---
>   drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 81 ++++++++++++++++++++++++++++-------
>   drivers/gpu/drm/msm/adreno/a6xx_gmu.h |  1 +
>   drivers/gpu/drm/msm/adreno/a6xx_hfi.c | 36 ++++++++++++++++
>   drivers/gpu/drm/msm/adreno/a6xx_hfi.h | 21 +++++++++
>   4 files changed, 124 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> index 37927bdd6fbe..09fb3f397dbb 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> @@ -1021,14 +1021,6 @@ int a6xx_gmu_resume(struct a6xx_gpu *a6xx_gpu)
>   
>   	gmu->hung = false;
>   
> -	/* Notify AOSS about the ACD state (unimplemented for now => disable it) */
> -	if (!IS_ERR(gmu->qmp)) {
> -		ret = qmp_send(gmu->qmp, "{class: gpu, res: acd, val: %d}",
> -			       0 /* Hardcode ACD to be disabled for now */);
> -		if (ret)
> -			dev_err(gmu->dev, "failed to send GPU ACD state\n");
> -	}
> -
>   	/* Turn on the resources */
>   	pm_runtime_get_sync(gmu->dev);
>   
> @@ -1476,6 +1468,64 @@ static int a6xx_gmu_pwrlevels_probe(struct a6xx_gmu *gmu)
>   	return a6xx_gmu_rpmh_votes_init(gmu);
>   }
>   
> +static int a6xx_gmu_acd_probe(struct a6xx_gmu *gmu)
> +{
> +	struct a6xx_gpu *a6xx_gpu = container_of(gmu, struct a6xx_gpu, gmu);
> +	struct a6xx_hfi_acd_table *cmd = &gmu->acd_table;
> +	struct adreno_gpu *adreno_gpu = &a6xx_gpu->base;
> +	struct msm_gpu *gpu = &adreno_gpu->base;
> +	int ret, i, cmd_idx = 0;
> +
> +	cmd->version = 1;
> +	cmd->stride = 1;
> +	cmd->enable_by_level = 0;
> +
> +	/* Skip freq = 0 and parse acd-level for rest of the OPPs */
> +	for (i = 1; i < gmu->nr_gpu_freqs; i++) {
> +		struct dev_pm_opp *opp;
> +		struct device_node *np;
> +		unsigned long freq;
> +		u32 val;
> +
> +		freq = gmu->gpu_freqs[i];
> +		opp = dev_pm_opp_find_freq_exact(&gpu->pdev->dev, freq, true);
> +		np = dev_pm_opp_get_of_node(opp);
> +
> +		ret = of_property_read_u32(np, "qcom,opp-acd-level", &val);
> +		of_node_put(np);
> +		dev_pm_opp_put(opp);
> +		if (ret == -EINVAL)
> +			continue;
> +		else if (ret) {
> +			DRM_DEV_ERROR(gmu->dev, "Unable to read acd level for freq %lu\n", freq);
> +			return ret;
> +		}
> +
> +		cmd->enable_by_level |= BIT(i);
> +		cmd->data[cmd_idx++] = val;

How do you know that cmd_idx is always < sizeof(cmd->data); ?

> +	}
> +
> +	cmd->num_levels = cmd_idx;
> +
> +	/* We are done here if ACD is not required for any of the OPPs */
> +	if (!cmd->enable_by_level)
> +		return 0;
> +
> +	/* Initialize qmp node to talk to AOSS */
> +	gmu->qmp = qmp_get(gmu->dev);
> +	if (IS_ERR(gmu->qmp)) {
> +		cmd->enable_by_level = 0;
> +		return dev_err_probe(gmu->dev, PTR_ERR(gmu->qmp), "Failed to initialize qmp\n");
> +	}
> +
> +	/* Notify AOSS about the ACD state */
> +	ret = qmp_send(gmu->qmp, "{class: gpu, res: acd, val: %d}", 1);
> +	if (ret)
> +		DRM_DEV_ERROR(gmu->dev, "failed to send GPU ACD state\n");
> +
> +	return 0;

Shouldn't the ret from gmp_send() get propogated in the return of this 
function ?

i.e. how can your probe be successful if the notification failed ?

---
bod

Re: [PATCH v2 2/3] dt-bindings: opp: Add v2-qcom-adreno vendor bindings

[Akhil P Oommen]

On 10/22/2024 11:19 AM, Krzysztof Kozlowski wrote:
> On Mon, Oct 21, 2024 at 05:23:43PM +0530, Akhil P Oommen wrote:
>> Add a new schema which extends opp-v2 to support a new vendor specific
>> property required for Adreno GPUs found in Qualcomm's SoCs. The new
>> property called "qcom,opp-acd-level" carries a u32 value recommended
>> for each opp needs to be shared to GMU during runtime.
>>
>> Cc: Rob Clark <robdclark@gmail.com>
>> Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
>> ---
>>  .../bindings/opp/opp-v2-qcom-adreno.yaml           | 96 ++++++++++++++++++++++
>>  1 file changed, 96 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml
>> new file mode 100644
>> index 000000000000..6d50c0405ef8
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml
>> @@ -0,0 +1,96 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/opp/opp-v2-qcom-adreno.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm Adreno compatible OPP supply
>> +
>> +description:
>> +  Adreno GPUs present in Qualcomm's Snapdragon chipsets uses an OPP specific
>> +  ACD related information tailored for the specific chipset. This binding
>> +  provides the information needed to describe such a hardware value.
>> +
>> +maintainers:
>> +  - Rob Clark <robdclark@gmail.com>
>> +
>> +allOf:
>> +  - $ref: opp-v2-base.yaml#
>> +
>> +properties:
>> +  compatible:
>> +    items:
>> +      - const: operating-points-v2-adreno
>> +      - const: operating-points-v2
>> +
>> +patternProperties:
>> +  '^opp-?[0-9]+$':
> 
> '-' should not be optional. opp1 is not expected name.

Agree. Will change this to '^opp-[0-9]+$'

> 
>> +    type: object
>> +    additionalProperties: false
>> +
>> +    properties:
>> +      opp-hz: true
>> +
>> +      opp-level: true
>> +
>> +      opp-peak-kBps: true
>> +
>> +      opp-supported-hw: true
>> +
>> +      qcom,opp-acd-level:
>> +        description: |
>> +          A positive value representing the ACD (Adaptive Clock Distribution,
>> +          a fancy name for clk throttling during voltage droop) level associated
>> +          with this OPP node. This value is shared to a co-processor inside GPU
>> +          (called Graphics Management Unit a.k.a GMU) during wake up. It may not
>> +          be present for some OPPs and GMU will disable ACD while transitioning
>> +          to that OPP. This value encodes a voltage threshold and few other knobs
>> +          which are identified by characterization of the SoC. So, it doesn't have
>> +          any unit.
> 
> Thanks for explanation and other updates. I am still not happy with this
> property. I do not see reason why DT should encode magic values in a
> quite generic piece of code. This creates poor ABI, difficult to
> maintain or understand.
> 

Configuring GPU ACD block with its respective value is a requirement for each OPP.
So OPP node seems like the natural place for this data.

If it helps to resolve your concerns, I can elaborate the documentation with
details on the GMU HFI interface where this value should be passed on to the
hardware. Also replace "few other knobs" with "Delay cycles & Calibration margin"
in the above doc.
 
> 
>> +        $ref: /schemas/types.yaml#/definitions/uint32
>> +
>> +    required:
>> +      - opp-hz
>> +      - opp-level
>> +
>> +required:
>> +  - compatible
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/power/qcom-rpmpd.h>
>> +
>> +    gpu_opp_table: opp-table {
>> +        compatible = "operating-points-v2-adreno", "operating-points-v2";
>> +
>> +        opp-687000000 {
>> +                opp-hz = /bits/ 64 <687000000>;
> 
> Messed indentation.

It seems my text editor got confused here. Will fix.

Thanks,
-Akhil

> 
> Best regards,
> Krzysztof
> 
> 

Re: [PATCH v2 2/3] dt-bindings: opp: Add v2-qcom-adreno vendor bindings

[Dmitry Baryshkov]

On Thu, Oct 24, 2024 at 12:56:58AM +0530, Akhil P Oommen wrote:
> On 10/22/2024 11:19 AM, Krzysztof Kozlowski wrote:
> > On Mon, Oct 21, 2024 at 05:23:43PM +0530, Akhil P Oommen wrote:
> >> Add a new schema which extends opp-v2 to support a new vendor specific
> >> property required for Adreno GPUs found in Qualcomm's SoCs. The new
> >> property called "qcom,opp-acd-level" carries a u32 value recommended
> >> for each opp needs to be shared to GMU during runtime.
> >>
> >> Cc: Rob Clark <robdclark@gmail.com>
> >> Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
> >> ---
> >>  .../bindings/opp/opp-v2-qcom-adreno.yaml           | 96 ++++++++++++++++++++++
> >>  1 file changed, 96 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml
> >> new file mode 100644
> >> index 000000000000..6d50c0405ef8
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml
> >> @@ -0,0 +1,96 @@
> >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/opp/opp-v2-qcom-adreno.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: Qualcomm Adreno compatible OPP supply
> >> +
> >> +description:
> >> +  Adreno GPUs present in Qualcomm's Snapdragon chipsets uses an OPP specific
> >> +  ACD related information tailored for the specific chipset. This binding
> >> +  provides the information needed to describe such a hardware value.
> >> +
> >> +maintainers:
> >> +  - Rob Clark <robdclark@gmail.com>
> >> +
> >> +allOf:
> >> +  - $ref: opp-v2-base.yaml#
> >> +
> >> +properties:
> >> +  compatible:
> >> +    items:
> >> +      - const: operating-points-v2-adreno
> >> +      - const: operating-points-v2
> >> +
> >> +patternProperties:
> >> +  '^opp-?[0-9]+$':
> > 
> > '-' should not be optional. opp1 is not expected name.
> 
> Agree. Will change this to '^opp-[0-9]+$'
> 
> > 
> >> +    type: object
> >> +    additionalProperties: false
> >> +
> >> +    properties:
> >> +      opp-hz: true
> >> +
> >> +      opp-level: true
> >> +
> >> +      opp-peak-kBps: true
> >> +
> >> +      opp-supported-hw: true
> >> +
> >> +      qcom,opp-acd-level:
> >> +        description: |
> >> +          A positive value representing the ACD (Adaptive Clock Distribution,
> >> +          a fancy name for clk throttling during voltage droop) level associated
> >> +          with this OPP node. This value is shared to a co-processor inside GPU
> >> +          (called Graphics Management Unit a.k.a GMU) during wake up. It may not
> >> +          be present for some OPPs and GMU will disable ACD while transitioning
> >> +          to that OPP. This value encodes a voltage threshold and few other knobs
> >> +          which are identified by characterization of the SoC. So, it doesn't have
> >> +          any unit.
> > 
> > Thanks for explanation and other updates. I am still not happy with this
> > property. I do not see reason why DT should encode magic values in a
> > quite generic piece of code. This creates poor ABI, difficult to
> > maintain or understand.
> > 
> 
> Configuring GPU ACD block with its respective value is a requirement for each OPP.
> So OPP node seems like the natural place for this data.
> 
> If it helps to resolve your concerns, I can elaborate the documentation with
> details on the GMU HFI interface where this value should be passed on to the
> hardware. Also replace "few other knobs" with "Delay cycles & Calibration margin"
> in the above doc.

Usually the preference for DT is to specify data in a sensible way
rather than just the values being programmed to the register. Is it
possible to implement this approach for ACD values?

>  
> > 

-- 
With best wishes
Dmitry

Re: [PATCH v2 2/3] dt-bindings: opp: Add v2-qcom-adreno vendor bindings

[Akhil P Oommen]

On 10/25/2024 11:58 AM, Dmitry Baryshkov wrote:
> On Thu, Oct 24, 2024 at 12:56:58AM +0530, Akhil P Oommen wrote:
>> On 10/22/2024 11:19 AM, Krzysztof Kozlowski wrote:
>>> On Mon, Oct 21, 2024 at 05:23:43PM +0530, Akhil P Oommen wrote:
>>>> Add a new schema which extends opp-v2 to support a new vendor specific
>>>> property required for Adreno GPUs found in Qualcomm's SoCs. The new
>>>> property called "qcom,opp-acd-level" carries a u32 value recommended
>>>> for each opp needs to be shared to GMU during runtime.
>>>>
>>>> Cc: Rob Clark <robdclark@gmail.com>
>>>> Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
>>>> ---
>>>>  .../bindings/opp/opp-v2-qcom-adreno.yaml           | 96 ++++++++++++++++++++++
>>>>  1 file changed, 96 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml
>>>> new file mode 100644
>>>> index 000000000000..6d50c0405ef8
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml
>>>> @@ -0,0 +1,96 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/opp/opp-v2-qcom-adreno.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Qualcomm Adreno compatible OPP supply
>>>> +
>>>> +description:
>>>> +  Adreno GPUs present in Qualcomm's Snapdragon chipsets uses an OPP specific
>>>> +  ACD related information tailored for the specific chipset. This binding
>>>> +  provides the information needed to describe such a hardware value.
>>>> +
>>>> +maintainers:
>>>> +  - Rob Clark <robdclark@gmail.com>
>>>> +
>>>> +allOf:
>>>> +  - $ref: opp-v2-base.yaml#
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    items:
>>>> +      - const: operating-points-v2-adreno
>>>> +      - const: operating-points-v2
>>>> +
>>>> +patternProperties:
>>>> +  '^opp-?[0-9]+$':
>>>
>>> '-' should not be optional. opp1 is not expected name.
>>
>> Agree. Will change this to '^opp-[0-9]+$'
>>
>>>
>>>> +    type: object
>>>> +    additionalProperties: false
>>>> +
>>>> +    properties:
>>>> +      opp-hz: true
>>>> +
>>>> +      opp-level: true
>>>> +
>>>> +      opp-peak-kBps: true
>>>> +
>>>> +      opp-supported-hw: true
>>>> +
>>>> +      qcom,opp-acd-level:
>>>> +        description: |
>>>> +          A positive value representing the ACD (Adaptive Clock Distribution,
>>>> +          a fancy name for clk throttling during voltage droop) level associated
>>>> +          with this OPP node. This value is shared to a co-processor inside GPU
>>>> +          (called Graphics Management Unit a.k.a GMU) during wake up. It may not
>>>> +          be present for some OPPs and GMU will disable ACD while transitioning
>>>> +          to that OPP. This value encodes a voltage threshold and few other knobs
>>>> +          which are identified by characterization of the SoC. So, it doesn't have
>>>> +          any unit.
>>>
>>> Thanks for explanation and other updates. I am still not happy with this
>>> property. I do not see reason why DT should encode magic values in a
>>> quite generic piece of code. This creates poor ABI, difficult to
>>> maintain or understand.
>>>
>>
>> Configuring GPU ACD block with its respective value is a requirement for each OPP.
>> So OPP node seems like the natural place for this data.
>>
>> If it helps to resolve your concerns, I can elaborate the documentation with
>> details on the GMU HFI interface where this value should be passed on to the
>> hardware. Also replace "few other knobs" with "Delay cycles & Calibration margin"
>> in the above doc.
> 
> Usually the preference for DT is to specify data in a sensible way
> rather than just the values being programmed to the register. Is it
> possible to implement this approach for ACD values?

I am still checking about this. Will get back.

-Akhil

> 
>>  
>>>
> 

Re: [PATCH v2 2/3] dt-bindings: opp: Add v2-qcom-adreno vendor bindings

[Akhil P Oommen]

On 11/1/2024 9:54 PM, Akhil P Oommen wrote:
> On 10/25/2024 11:58 AM, Dmitry Baryshkov wrote:
>> On Thu, Oct 24, 2024 at 12:56:58AM +0530, Akhil P Oommen wrote:
>>> On 10/22/2024 11:19 AM, Krzysztof Kozlowski wrote:
>>>> On Mon, Oct 21, 2024 at 05:23:43PM +0530, Akhil P Oommen wrote:
>>>>> Add a new schema which extends opp-v2 to support a new vendor specific
>>>>> property required for Adreno GPUs found in Qualcomm's SoCs. The new
>>>>> property called "qcom,opp-acd-level" carries a u32 value recommended
>>>>> for each opp needs to be shared to GMU during runtime.
>>>>>
>>>>> Cc: Rob Clark <robdclark@gmail.com>
>>>>> Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
>>>>> ---
>>>>>  .../bindings/opp/opp-v2-qcom-adreno.yaml           | 96 ++++++++++++++++++++++
>>>>>  1 file changed, 96 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..6d50c0405ef8
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml
>>>>> @@ -0,0 +1,96 @@
>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: http://devicetree.org/schemas/opp/opp-v2-qcom-adreno.yaml#
>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>> +
>>>>> +title: Qualcomm Adreno compatible OPP supply
>>>>> +
>>>>> +description:
>>>>> +  Adreno GPUs present in Qualcomm's Snapdragon chipsets uses an OPP specific
>>>>> +  ACD related information tailored for the specific chipset. This binding
>>>>> +  provides the information needed to describe such a hardware value.
>>>>> +
>>>>> +maintainers:
>>>>> +  - Rob Clark <robdclark@gmail.com>
>>>>> +
>>>>> +allOf:
>>>>> +  - $ref: opp-v2-base.yaml#
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    items:
>>>>> +      - const: operating-points-v2-adreno
>>>>> +      - const: operating-points-v2
>>>>> +
>>>>> +patternProperties:
>>>>> +  '^opp-?[0-9]+$':
>>>>
>>>> '-' should not be optional. opp1 is not expected name.
>>>
>>> Agree. Will change this to '^opp-[0-9]+$'
>>>
>>>>
>>>>> +    type: object
>>>>> +    additionalProperties: false
>>>>> +
>>>>> +    properties:
>>>>> +      opp-hz: true
>>>>> +
>>>>> +      opp-level: true
>>>>> +
>>>>> +      opp-peak-kBps: true
>>>>> +
>>>>> +      opp-supported-hw: true
>>>>> +
>>>>> +      qcom,opp-acd-level:
>>>>> +        description: |
>>>>> +          A positive value representing the ACD (Adaptive Clock Distribution,
>>>>> +          a fancy name for clk throttling during voltage droop) level associated
>>>>> +          with this OPP node. This value is shared to a co-processor inside GPU
>>>>> +          (called Graphics Management Unit a.k.a GMU) during wake up. It may not
>>>>> +          be present for some OPPs and GMU will disable ACD while transitioning
>>>>> +          to that OPP. This value encodes a voltage threshold and few other knobs
>>>>> +          which are identified by characterization of the SoC. So, it doesn't have
>>>>> +          any unit.
>>>>
>>>> Thanks for explanation and other updates. I am still not happy with this
>>>> property. I do not see reason why DT should encode magic values in a
>>>> quite generic piece of code. This creates poor ABI, difficult to
>>>> maintain or understand.
>>>>
>>>
>>> Configuring GPU ACD block with its respective value is a requirement for each OPP.
>>> So OPP node seems like the natural place for this data.
>>>
>>> If it helps to resolve your concerns, I can elaborate the documentation with
>>> details on the GMU HFI interface where this value should be passed on to the
>>> hardware. Also replace "few other knobs" with "Delay cycles & Calibration margin"
>>> in the above doc.
>>
>> Usually the preference for DT is to specify data in a sensible way
>> rather than just the values being programmed to the register. Is it
>> possible to implement this approach for ACD values?

Krzysztof/Dmitry,

BIT(0)-BIT(15) are static configurations which doesn't change between
OPPs. We can move it to driver.

BIT(16)-BIT(31) indicates a threshold margin which triggers ACD. We can
keep this in the devicetree. And the driver can construct the final
value from both data and send it to GMU.

If this is acceptable, I will send the v3 revision.

-Akhil.

> 
> I am still checking about this. Will get back.
> 
> -Akhil
> 
>>
>>>  
>>>>
>>
> 

Re: [PATCH v2 2/3] dt-bindings: opp: Add v2-qcom-adreno vendor bindings

[Dmitry Baryshkov]

Hello Akhil,

On Thu, 14 Nov 2024 at 20:50, Akhil P Oommen <quic_akhilpo@quicinc.com> wrote:
>
> On 11/1/2024 9:54 PM, Akhil P Oommen wrote:
> > On 10/25/2024 11:58 AM, Dmitry Baryshkov wrote:
> >> On Thu, Oct 24, 2024 at 12:56:58AM +0530, Akhil P Oommen wrote:
> >>> On 10/22/2024 11:19 AM, Krzysztof Kozlowski wrote:
> >>>> On Mon, Oct 21, 2024 at 05:23:43PM +0530, Akhil P Oommen wrote:
> >>>>> Add a new schema which extends opp-v2 to support a new vendor specific
> >>>>> property required for Adreno GPUs found in Qualcomm's SoCs. The new
> >>>>> property called "qcom,opp-acd-level" carries a u32 value recommended
> >>>>> for each opp needs to be shared to GMU during runtime.
> >>>>>
> >>>>> Cc: Rob Clark <robdclark@gmail.com>
> >>>>> Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
> >>>>> ---
> >>>>>  .../bindings/opp/opp-v2-qcom-adreno.yaml           | 96 ++++++++++++++++++++++
> >>>>>  1 file changed, 96 insertions(+)
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml
> >>>>> new file mode 100644
> >>>>> index 000000000000..6d50c0405ef8
> >>>>> --- /dev/null
> >>>>> +++ b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml
> >>>>> @@ -0,0 +1,96 @@
> >>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>>>> +%YAML 1.2
> >>>>> +---
> >>>>> +$id: http://devicetree.org/schemas/opp/opp-v2-qcom-adreno.yaml#
> >>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>>> +
> >>>>> +title: Qualcomm Adreno compatible OPP supply
> >>>>> +
> >>>>> +description:
> >>>>> +  Adreno GPUs present in Qualcomm's Snapdragon chipsets uses an OPP specific
> >>>>> +  ACD related information tailored for the specific chipset. This binding
> >>>>> +  provides the information needed to describe such a hardware value.
> >>>>> +
> >>>>> +maintainers:
> >>>>> +  - Rob Clark <robdclark@gmail.com>
> >>>>> +
> >>>>> +allOf:
> >>>>> +  - $ref: opp-v2-base.yaml#
> >>>>> +
> >>>>> +properties:
> >>>>> +  compatible:
> >>>>> +    items:
> >>>>> +      - const: operating-points-v2-adreno
> >>>>> +      - const: operating-points-v2
> >>>>> +
> >>>>> +patternProperties:
> >>>>> +  '^opp-?[0-9]+$':
> >>>>
> >>>> '-' should not be optional. opp1 is not expected name.
> >>>
> >>> Agree. Will change this to '^opp-[0-9]+$'
> >>>
> >>>>
> >>>>> +    type: object
> >>>>> +    additionalProperties: false
> >>>>> +
> >>>>> +    properties:
> >>>>> +      opp-hz: true
> >>>>> +
> >>>>> +      opp-level: true
> >>>>> +
> >>>>> +      opp-peak-kBps: true
> >>>>> +
> >>>>> +      opp-supported-hw: true
> >>>>> +
> >>>>> +      qcom,opp-acd-level:
> >>>>> +        description: |
> >>>>> +          A positive value representing the ACD (Adaptive Clock Distribution,
> >>>>> +          a fancy name for clk throttling during voltage droop) level associated
> >>>>> +          with this OPP node. This value is shared to a co-processor inside GPU
> >>>>> +          (called Graphics Management Unit a.k.a GMU) during wake up. It may not
> >>>>> +          be present for some OPPs and GMU will disable ACD while transitioning
> >>>>> +          to that OPP. This value encodes a voltage threshold and few other knobs
> >>>>> +          which are identified by characterization of the SoC. So, it doesn't have
> >>>>> +          any unit.
> >>>>
> >>>> Thanks for explanation and other updates. I am still not happy with this
> >>>> property. I do not see reason why DT should encode magic values in a
> >>>> quite generic piece of code. This creates poor ABI, difficult to
> >>>> maintain or understand.
> >>>>
> >>>
> >>> Configuring GPU ACD block with its respective value is a requirement for each OPP.
> >>> So OPP node seems like the natural place for this data.
> >>>
> >>> If it helps to resolve your concerns, I can elaborate the documentation with
> >>> details on the GMU HFI interface where this value should be passed on to the
> >>> hardware. Also replace "few other knobs" with "Delay cycles & Calibration margin"
> >>> in the above doc.
> >>
> >> Usually the preference for DT is to specify data in a sensible way
> >> rather than just the values being programmed to the register. Is it
> >> possible to implement this approach for ACD values?
>
> Krzysztof/Dmitry,
>
> BIT(0)-BIT(15) are static configurations which doesn't change between
> OPPs. We can move it to driver.
>
> BIT(16)-BIT(31) indicates a threshold margin which triggers ACD. We can
> keep this in the devicetree. And the driver can construct the final
> value from both data and send it to GMU.
>
> If this is acceptable, I will send the v3 revision.

Can the upper bitfield have a sensible representation in DT (like uV
or something similar)?

>
> -Akhil.
>
> >
> > I am still checking about this. Will get back.
> >
> > -Akhil
> >
> >>
> >>>
> >>>>
> >>
> >
>


-- 
With best wishes
Dmitry

Re: [PATCH v2 2/3] dt-bindings: opp: Add v2-qcom-adreno vendor bindings

[Akhil P Oommen]

On 11/15/2024 3:54 AM, Dmitry Baryshkov wrote:
> Hello Akhil,
> 
> On Thu, 14 Nov 2024 at 20:50, Akhil P Oommen <quic_akhilpo@quicinc.com> wrote:
>>
>> On 11/1/2024 9:54 PM, Akhil P Oommen wrote:
>>> On 10/25/2024 11:58 AM, Dmitry Baryshkov wrote:
>>>> On Thu, Oct 24, 2024 at 12:56:58AM +0530, Akhil P Oommen wrote:
>>>>> On 10/22/2024 11:19 AM, Krzysztof Kozlowski wrote:
>>>>>> On Mon, Oct 21, 2024 at 05:23:43PM +0530, Akhil P Oommen wrote:
>>>>>>> Add a new schema which extends opp-v2 to support a new vendor specific
>>>>>>> property required for Adreno GPUs found in Qualcomm's SoCs. The new
>>>>>>> property called "qcom,opp-acd-level" carries a u32 value recommended
>>>>>>> for each opp needs to be shared to GMU during runtime.
>>>>>>>
>>>>>>> Cc: Rob Clark <robdclark@gmail.com>
>>>>>>> Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
>>>>>>> ---
>>>>>>>  .../bindings/opp/opp-v2-qcom-adreno.yaml           | 96 ++++++++++++++++++++++
>>>>>>>  1 file changed, 96 insertions(+)
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml
>>>>>>> new file mode 100644
>>>>>>> index 000000000000..6d50c0405ef8
>>>>>>> --- /dev/null
>>>>>>> +++ b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml
>>>>>>> @@ -0,0 +1,96 @@
>>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>>>> +%YAML 1.2
>>>>>>> +---
>>>>>>> +$id: http://devicetree.org/schemas/opp/opp-v2-qcom-adreno.yaml#
>>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>>> +
>>>>>>> +title: Qualcomm Adreno compatible OPP supply
>>>>>>> +
>>>>>>> +description:
>>>>>>> +  Adreno GPUs present in Qualcomm's Snapdragon chipsets uses an OPP specific
>>>>>>> +  ACD related information tailored for the specific chipset. This binding
>>>>>>> +  provides the information needed to describe such a hardware value.
>>>>>>> +
>>>>>>> +maintainers:
>>>>>>> +  - Rob Clark <robdclark@gmail.com>
>>>>>>> +
>>>>>>> +allOf:
>>>>>>> +  - $ref: opp-v2-base.yaml#
>>>>>>> +
>>>>>>> +properties:
>>>>>>> +  compatible:
>>>>>>> +    items:
>>>>>>> +      - const: operating-points-v2-adreno
>>>>>>> +      - const: operating-points-v2
>>>>>>> +
>>>>>>> +patternProperties:
>>>>>>> +  '^opp-?[0-9]+$':
>>>>>>
>>>>>> '-' should not be optional. opp1 is not expected name.
>>>>>
>>>>> Agree. Will change this to '^opp-[0-9]+$'
>>>>>
>>>>>>
>>>>>>> +    type: object
>>>>>>> +    additionalProperties: false
>>>>>>> +
>>>>>>> +    properties:
>>>>>>> +      opp-hz: true
>>>>>>> +
>>>>>>> +      opp-level: true
>>>>>>> +
>>>>>>> +      opp-peak-kBps: true
>>>>>>> +
>>>>>>> +      opp-supported-hw: true
>>>>>>> +
>>>>>>> +      qcom,opp-acd-level:
>>>>>>> +        description: |
>>>>>>> +          A positive value representing the ACD (Adaptive Clock Distribution,
>>>>>>> +          a fancy name for clk throttling during voltage droop) level associated
>>>>>>> +          with this OPP node. This value is shared to a co-processor inside GPU
>>>>>>> +          (called Graphics Management Unit a.k.a GMU) during wake up. It may not
>>>>>>> +          be present for some OPPs and GMU will disable ACD while transitioning
>>>>>>> +          to that OPP. This value encodes a voltage threshold and few other knobs
>>>>>>> +          which are identified by characterization of the SoC. So, it doesn't have
>>>>>>> +          any unit.
>>>>>>
>>>>>> Thanks for explanation and other updates. I am still not happy with this
>>>>>> property. I do not see reason why DT should encode magic values in a
>>>>>> quite generic piece of code. This creates poor ABI, difficult to
>>>>>> maintain or understand.
>>>>>>
>>>>>
>>>>> Configuring GPU ACD block with its respective value is a requirement for each OPP.
>>>>> So OPP node seems like the natural place for this data.
>>>>>
>>>>> If it helps to resolve your concerns, I can elaborate the documentation with
>>>>> details on the GMU HFI interface where this value should be passed on to the
>>>>> hardware. Also replace "few other knobs" with "Delay cycles & Calibration margin"
>>>>> in the above doc.
>>>>
>>>> Usually the preference for DT is to specify data in a sensible way
>>>> rather than just the values being programmed to the register. Is it
>>>> possible to implement this approach for ACD values?
>>
>> Krzysztof/Dmitry,
>>
>> BIT(0)-BIT(15) are static configurations which doesn't change between
>> OPPs. We can move it to driver.
>>
>> BIT(16)-BIT(31) indicates a threshold margin which triggers ACD. We can
>> keep this in the devicetree. And the driver can construct the final
>> value from both data and send it to GMU.
>>
>> If this is acceptable, I will send the v3 revision.
> 
> Can the upper bitfield have a sensible representation in DT (like uV
> or something similar)?

Closest approximation is quantized voltage steps. So, unit-less.
Converting it to the exact voltage requires identifying the pmic voltage
steps and other stuffs which are outside of my expertise.

It is convenient if we can abstract it as an integer which correlates
with the voltage margin that should be maintained for each regulator corner.

-Akhil.

> 
>>
>> -Akhil.
>>
>>>
>>> I am still checking about this. Will get back.
>>>
>>> -Akhil
>>>
>>>>
>>>>>
>>>>>>
>>>>
>>>
>>
> 
> 

Re: [PATCH v2 2/3] dt-bindings: opp: Add v2-qcom-adreno vendor bindings

[Dmitry Baryshkov]

On Fri, 15 Nov 2024 at 19:54, Akhil P Oommen <quic_akhilpo@quicinc.com> wrote:
>
> On 11/15/2024 3:54 AM, Dmitry Baryshkov wrote:
> > Hello Akhil,
> >
> > On Thu, 14 Nov 2024 at 20:50, Akhil P Oommen <quic_akhilpo@quicinc.com> wrote:
> >>
> >> On 11/1/2024 9:54 PM, Akhil P Oommen wrote:
> >>> On 10/25/2024 11:58 AM, Dmitry Baryshkov wrote:
> >>>> On Thu, Oct 24, 2024 at 12:56:58AM +0530, Akhil P Oommen wrote:
> >>>>> On 10/22/2024 11:19 AM, Krzysztof Kozlowski wrote:
> >>>>>> On Mon, Oct 21, 2024 at 05:23:43PM +0530, Akhil P Oommen wrote:
> >>>>>>> Add a new schema which extends opp-v2 to support a new vendor specific
> >>>>>>> property required for Adreno GPUs found in Qualcomm's SoCs. The new
> >>>>>>> property called "qcom,opp-acd-level" carries a u32 value recommended
> >>>>>>> for each opp needs to be shared to GMU during runtime.
> >>>>>>>
> >>>>>>> Cc: Rob Clark <robdclark@gmail.com>
> >>>>>>> Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
> >>>>>>> ---
> >>>>>>>  .../bindings/opp/opp-v2-qcom-adreno.yaml           | 96 ++++++++++++++++++++++
> >>>>>>>  1 file changed, 96 insertions(+)
> >>>>>>>
> >>>>>>> diff --git a/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml
> >>>>>>> new file mode 100644
> >>>>>>> index 000000000000..6d50c0405ef8
> >>>>>>> --- /dev/null
> >>>>>>> +++ b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml
> >>>>>>> @@ -0,0 +1,96 @@
> >>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>>>>>> +%YAML 1.2
> >>>>>>> +---
> >>>>>>> +$id: http://devicetree.org/schemas/opp/opp-v2-qcom-adreno.yaml#
> >>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>>>>> +
> >>>>>>> +title: Qualcomm Adreno compatible OPP supply
> >>>>>>> +
> >>>>>>> +description:
> >>>>>>> +  Adreno GPUs present in Qualcomm's Snapdragon chipsets uses an OPP specific
> >>>>>>> +  ACD related information tailored for the specific chipset. This binding
> >>>>>>> +  provides the information needed to describe such a hardware value.
> >>>>>>> +
> >>>>>>> +maintainers:
> >>>>>>> +  - Rob Clark <robdclark@gmail.com>
> >>>>>>> +
> >>>>>>> +allOf:
> >>>>>>> +  - $ref: opp-v2-base.yaml#
> >>>>>>> +
> >>>>>>> +properties:
> >>>>>>> +  compatible:
> >>>>>>> +    items:
> >>>>>>> +      - const: operating-points-v2-adreno
> >>>>>>> +      - const: operating-points-v2
> >>>>>>> +
> >>>>>>> +patternProperties:
> >>>>>>> +  '^opp-?[0-9]+$':
> >>>>>>
> >>>>>> '-' should not be optional. opp1 is not expected name.
> >>>>>
> >>>>> Agree. Will change this to '^opp-[0-9]+$'
> >>>>>
> >>>>>>
> >>>>>>> +    type: object
> >>>>>>> +    additionalProperties: false
> >>>>>>> +
> >>>>>>> +    properties:
> >>>>>>> +      opp-hz: true
> >>>>>>> +
> >>>>>>> +      opp-level: true
> >>>>>>> +
> >>>>>>> +      opp-peak-kBps: true
> >>>>>>> +
> >>>>>>> +      opp-supported-hw: true
> >>>>>>> +
> >>>>>>> +      qcom,opp-acd-level:
> >>>>>>> +        description: |
> >>>>>>> +          A positive value representing the ACD (Adaptive Clock Distribution,
> >>>>>>> +          a fancy name for clk throttling during voltage droop) level associated
> >>>>>>> +          with this OPP node. This value is shared to a co-processor inside GPU
> >>>>>>> +          (called Graphics Management Unit a.k.a GMU) during wake up. It may not
> >>>>>>> +          be present for some OPPs and GMU will disable ACD while transitioning
> >>>>>>> +          to that OPP. This value encodes a voltage threshold and few other knobs
> >>>>>>> +          which are identified by characterization of the SoC. So, it doesn't have
> >>>>>>> +          any unit.
> >>>>>>
> >>>>>> Thanks for explanation and other updates. I am still not happy with this
> >>>>>> property. I do not see reason why DT should encode magic values in a
> >>>>>> quite generic piece of code. This creates poor ABI, difficult to
> >>>>>> maintain or understand.
> >>>>>>
> >>>>>
> >>>>> Configuring GPU ACD block with its respective value is a requirement for each OPP.
> >>>>> So OPP node seems like the natural place for this data.
> >>>>>
> >>>>> If it helps to resolve your concerns, I can elaborate the documentation with
> >>>>> details on the GMU HFI interface where this value should be passed on to the
> >>>>> hardware. Also replace "few other knobs" with "Delay cycles & Calibration margin"
> >>>>> in the above doc.
> >>>>
> >>>> Usually the preference for DT is to specify data in a sensible way
> >>>> rather than just the values being programmed to the register. Is it
> >>>> possible to implement this approach for ACD values?
> >>
> >> Krzysztof/Dmitry,
> >>
> >> BIT(0)-BIT(15) are static configurations which doesn't change between
> >> OPPs. We can move it to driver.
> >>
> >> BIT(16)-BIT(31) indicates a threshold margin which triggers ACD. We can
> >> keep this in the devicetree. And the driver can construct the final
> >> value from both data and send it to GMU.
> >>
> >> If this is acceptable, I will send the v3 revision.
> >
> > Can the upper bitfield have a sensible representation in DT (like uV
> > or something similar)?
>
> Closest approximation is quantized voltage steps. So, unit-less.
> Converting it to the exact voltage requires identifying the pmic voltage
> steps and other stuffs which are outside of my expertise.
>
> It is convenient if we can abstract it as an integer which correlates
> with the voltage margin that should be maintained for each regulator corner.

I'd say, this is up to the DT maintainers then.

>
> -Akhil.
>
> >
> >>
> >> -Akhil.
> >>
> >>>
> >>> I am still checking about this. Will get back.
> >>>
> >>> -Akhil
> >>>
> >>>>
> >>>>>
> >>>>>>
> >>>>
> >>>
> >>
> >
> >
>


-- 
With best wishes
Dmitry

Re: [PATCH v2 2/3] dt-bindings: opp: Add v2-qcom-adreno vendor bindings

[Akhil P Oommen]

On 11/16/2024 1:17 AM, Dmitry Baryshkov wrote:
> On Fri, 15 Nov 2024 at 19:54, Akhil P Oommen <quic_akhilpo@quicinc.com> wrote:
>>
>> On 11/15/2024 3:54 AM, Dmitry Baryshkov wrote:
>>> Hello Akhil,
>>>
>>> On Thu, 14 Nov 2024 at 20:50, Akhil P Oommen <quic_akhilpo@quicinc.com> wrote:
>>>>
>>>> On 11/1/2024 9:54 PM, Akhil P Oommen wrote:
>>>>> On 10/25/2024 11:58 AM, Dmitry Baryshkov wrote:
>>>>>> On Thu, Oct 24, 2024 at 12:56:58AM +0530, Akhil P Oommen wrote:
>>>>>>> On 10/22/2024 11:19 AM, Krzysztof Kozlowski wrote:
>>>>>>>> On Mon, Oct 21, 2024 at 05:23:43PM +0530, Akhil P Oommen wrote:
>>>>>>>>> Add a new schema which extends opp-v2 to support a new vendor specific
>>>>>>>>> property required for Adreno GPUs found in Qualcomm's SoCs. The new
>>>>>>>>> property called "qcom,opp-acd-level" carries a u32 value recommended
>>>>>>>>> for each opp needs to be shared to GMU during runtime.
>>>>>>>>>
>>>>>>>>> Cc: Rob Clark <robdclark@gmail.com>
>>>>>>>>> Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
>>>>>>>>> ---
>>>>>>>>>  .../bindings/opp/opp-v2-qcom-adreno.yaml           | 96 ++++++++++++++++++++++
>>>>>>>>>  1 file changed, 96 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml
>>>>>>>>> new file mode 100644
>>>>>>>>> index 000000000000..6d50c0405ef8
>>>>>>>>> --- /dev/null
>>>>>>>>> +++ b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml
>>>>>>>>> @@ -0,0 +1,96 @@
>>>>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>>>>>> +%YAML 1.2
>>>>>>>>> +---
>>>>>>>>> +$id: http://devicetree.org/schemas/opp/opp-v2-qcom-adreno.yaml#
>>>>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>>>>> +
>>>>>>>>> +title: Qualcomm Adreno compatible OPP supply
>>>>>>>>> +
>>>>>>>>> +description:
>>>>>>>>> +  Adreno GPUs present in Qualcomm's Snapdragon chipsets uses an OPP specific
>>>>>>>>> +  ACD related information tailored for the specific chipset. This binding
>>>>>>>>> +  provides the information needed to describe such a hardware value.
>>>>>>>>> +
>>>>>>>>> +maintainers:
>>>>>>>>> +  - Rob Clark <robdclark@gmail.com>
>>>>>>>>> +
>>>>>>>>> +allOf:
>>>>>>>>> +  - $ref: opp-v2-base.yaml#
>>>>>>>>> +
>>>>>>>>> +properties:
>>>>>>>>> +  compatible:
>>>>>>>>> +    items:
>>>>>>>>> +      - const: operating-points-v2-adreno
>>>>>>>>> +      - const: operating-points-v2
>>>>>>>>> +
>>>>>>>>> +patternProperties:
>>>>>>>>> +  '^opp-?[0-9]+$':
>>>>>>>>
>>>>>>>> '-' should not be optional. opp1 is not expected name.
>>>>>>>
>>>>>>> Agree. Will change this to '^opp-[0-9]+$'
>>>>>>>
>>>>>>>>
>>>>>>>>> +    type: object
>>>>>>>>> +    additionalProperties: false
>>>>>>>>> +
>>>>>>>>> +    properties:
>>>>>>>>> +      opp-hz: true
>>>>>>>>> +
>>>>>>>>> +      opp-level: true
>>>>>>>>> +
>>>>>>>>> +      opp-peak-kBps: true
>>>>>>>>> +
>>>>>>>>> +      opp-supported-hw: true
>>>>>>>>> +
>>>>>>>>> +      qcom,opp-acd-level:
>>>>>>>>> +        description: |
>>>>>>>>> +          A positive value representing the ACD (Adaptive Clock Distribution,
>>>>>>>>> +          a fancy name for clk throttling during voltage droop) level associated
>>>>>>>>> +          with this OPP node. This value is shared to a co-processor inside GPU
>>>>>>>>> +          (called Graphics Management Unit a.k.a GMU) during wake up. It may not
>>>>>>>>> +          be present for some OPPs and GMU will disable ACD while transitioning
>>>>>>>>> +          to that OPP. This value encodes a voltage threshold and few other knobs
>>>>>>>>> +          which are identified by characterization of the SoC. So, it doesn't have
>>>>>>>>> +          any unit.
>>>>>>>>
>>>>>>>> Thanks for explanation and other updates. I am still not happy with this
>>>>>>>> property. I do not see reason why DT should encode magic values in a
>>>>>>>> quite generic piece of code. This creates poor ABI, difficult to
>>>>>>>> maintain or understand.
>>>>>>>>
>>>>>>>
>>>>>>> Configuring GPU ACD block with its respective value is a requirement for each OPP.
>>>>>>> So OPP node seems like the natural place for this data.
>>>>>>>
>>>>>>> If it helps to resolve your concerns, I can elaborate the documentation with
>>>>>>> details on the GMU HFI interface where this value should be passed on to the
>>>>>>> hardware. Also replace "few other knobs" with "Delay cycles & Calibration margin"
>>>>>>> in the above doc.
>>>>>>
>>>>>> Usually the preference for DT is to specify data in a sensible way
>>>>>> rather than just the values being programmed to the register. Is it
>>>>>> possible to implement this approach for ACD values?
>>>>
>>>> Krzysztof/Dmitry,
>>>>
>>>> BIT(0)-BIT(15) are static configurations which doesn't change between
>>>> OPPs. We can move it to driver.
>>>>
>>>> BIT(16)-BIT(31) indicates a threshold margin which triggers ACD. We can
>>>> keep this in the devicetree. And the driver can construct the final
>>>> value from both data and send it to GMU.
>>>>
>>>> If this is acceptable, I will send the v3 revision.
>>>
>>> Can the upper bitfield have a sensible representation in DT (like uV
>>> or something similar)?
>>
>> Closest approximation is quantized voltage steps. So, unit-less.
>> Converting it to the exact voltage requires identifying the pmic voltage
>> steps and other stuffs which are outside of my expertise.
>>
>> It is convenient if we can abstract it as an integer which correlates
>> with the voltage margin that should be maintained for each regulator corner.

Krzysztof,

Could you please confirm if this approach would be acceptable?

To reiterate, move the lower 16 bits which is same across OPPs to the
driver. Abstract the higher 16 bits as number of quantized voltage
margin when ACD mitigation gets triggered.

-Akhil.

> 
> I'd say, this is up to the DT maintainers then.
> 
>>
>> -Akhil.
>>
>>>
>>>>
>>>> -Akhil.
>>>>
>>>>>
>>>>> I am still checking about this. Will get back.
>>>>>
>>>>> -Akhil
>>>>>
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>>
>>
> 
> 

Re: [PATCH v2 2/3] dt-bindings: opp: Add v2-qcom-adreno vendor bindings

[Konrad Dybcio]

On 4.12.2024 7:18 PM, Akhil P Oommen wrote:
> On 11/16/2024 1:17 AM, Dmitry Baryshkov wrote:
>> On Fri, 15 Nov 2024 at 19:54, Akhil P Oommen <quic_akhilpo@quicinc.com> wrote:
>>>
>>> On 11/15/2024 3:54 AM, Dmitry Baryshkov wrote:
>>>> Hello Akhil,
>>>>
>>>> On Thu, 14 Nov 2024 at 20:50, Akhil P Oommen <quic_akhilpo@quicinc.com> wrote:
>>>>>
>>>>> On 11/1/2024 9:54 PM, Akhil P Oommen wrote:
>>>>>> On 10/25/2024 11:58 AM, Dmitry Baryshkov wrote:
>>>>>>> On Thu, Oct 24, 2024 at 12:56:58AM +0530, Akhil P Oommen wrote:
>>>>>>>> On 10/22/2024 11:19 AM, Krzysztof Kozlowski wrote:
>>>>>>>>> On Mon, Oct 21, 2024 at 05:23:43PM +0530, Akhil P Oommen wrote:
>>>>>>>>>> Add a new schema which extends opp-v2 to support a new vendor specific
>>>>>>>>>> property required for Adreno GPUs found in Qualcomm's SoCs. The new
>>>>>>>>>> property called "qcom,opp-acd-level" carries a u32 value recommended
>>>>>>>>>> for each opp needs to be shared to GMU during runtime.
>>>>>>>>>>
>>>>>>>>>> Cc: Rob Clark <robdclark@gmail.com>
>>>>>>>>>> Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
>>>>>>>>>> ---
>>>>>>>>>>  .../bindings/opp/opp-v2-qcom-adreno.yaml           | 96 ++++++++++++++++++++++
>>>>>>>>>>  1 file changed, 96 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml
>>>>>>>>>> new file mode 100644
>>>>>>>>>> index 000000000000..6d50c0405ef8
>>>>>>>>>> --- /dev/null
>>>>>>>>>> +++ b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml
>>>>>>>>>> @@ -0,0 +1,96 @@
>>>>>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>>>>>>> +%YAML 1.2
>>>>>>>>>> +---
>>>>>>>>>> +$id: http://devicetree.org/schemas/opp/opp-v2-qcom-adreno.yaml#
>>>>>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>>>>>> +
>>>>>>>>>> +title: Qualcomm Adreno compatible OPP supply
>>>>>>>>>> +
>>>>>>>>>> +description:
>>>>>>>>>> +  Adreno GPUs present in Qualcomm's Snapdragon chipsets uses an OPP specific
>>>>>>>>>> +  ACD related information tailored for the specific chipset. This binding
>>>>>>>>>> +  provides the information needed to describe such a hardware value.
>>>>>>>>>> +
>>>>>>>>>> +maintainers:
>>>>>>>>>> +  - Rob Clark <robdclark@gmail.com>
>>>>>>>>>> +
>>>>>>>>>> +allOf:
>>>>>>>>>> +  - $ref: opp-v2-base.yaml#
>>>>>>>>>> +
>>>>>>>>>> +properties:
>>>>>>>>>> +  compatible:
>>>>>>>>>> +    items:
>>>>>>>>>> +      - const: operating-points-v2-adreno
>>>>>>>>>> +      - const: operating-points-v2
>>>>>>>>>> +
>>>>>>>>>> +patternProperties:
>>>>>>>>>> +  '^opp-?[0-9]+$':
>>>>>>>>>
>>>>>>>>> '-' should not be optional. opp1 is not expected name.
>>>>>>>>
>>>>>>>> Agree. Will change this to '^opp-[0-9]+$'
>>>>>>>>
>>>>>>>>>
>>>>>>>>>> +    type: object
>>>>>>>>>> +    additionalProperties: false
>>>>>>>>>> +
>>>>>>>>>> +    properties:
>>>>>>>>>> +      opp-hz: true
>>>>>>>>>> +
>>>>>>>>>> +      opp-level: true
>>>>>>>>>> +
>>>>>>>>>> +      opp-peak-kBps: true
>>>>>>>>>> +
>>>>>>>>>> +      opp-supported-hw: true
>>>>>>>>>> +
>>>>>>>>>> +      qcom,opp-acd-level:
>>>>>>>>>> +        description: |
>>>>>>>>>> +          A positive value representing the ACD (Adaptive Clock Distribution,
>>>>>>>>>> +          a fancy name for clk throttling during voltage droop) level associated
>>>>>>>>>> +          with this OPP node. This value is shared to a co-processor inside GPU
>>>>>>>>>> +          (called Graphics Management Unit a.k.a GMU) during wake up. It may not
>>>>>>>>>> +          be present for some OPPs and GMU will disable ACD while transitioning
>>>>>>>>>> +          to that OPP. This value encodes a voltage threshold and few other knobs
>>>>>>>>>> +          which are identified by characterization of the SoC. So, it doesn't have
>>>>>>>>>> +          any unit.
>>>>>>>>>
>>>>>>>>> Thanks for explanation and other updates. I am still not happy with this
>>>>>>>>> property. I do not see reason why DT should encode magic values in a
>>>>>>>>> quite generic piece of code. This creates poor ABI, difficult to
>>>>>>>>> maintain or understand.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Configuring GPU ACD block with its respective value is a requirement for each OPP.
>>>>>>>> So OPP node seems like the natural place for this data.
>>>>>>>>
>>>>>>>> If it helps to resolve your concerns, I can elaborate the documentation with
>>>>>>>> details on the GMU HFI interface where this value should be passed on to the
>>>>>>>> hardware. Also replace "few other knobs" with "Delay cycles & Calibration margin"
>>>>>>>> in the above doc.
>>>>>>>
>>>>>>> Usually the preference for DT is to specify data in a sensible way
>>>>>>> rather than just the values being programmed to the register. Is it
>>>>>>> possible to implement this approach for ACD values?
>>>>>
>>>>> Krzysztof/Dmitry,
>>>>>
>>>>> BIT(0)-BIT(15) are static configurations which doesn't change between
>>>>> OPPs. We can move it to driver.
>>>>>
>>>>> BIT(16)-BIT(31) indicates a threshold margin which triggers ACD. We can
>>>>> keep this in the devicetree. And the driver can construct the final
>>>>> value from both data and send it to GMU.
>>>>>
>>>>> If this is acceptable, I will send the v3 revision.
>>>>
>>>> Can the upper bitfield have a sensible representation in DT (like uV
>>>> or something similar)?
>>>
>>> Closest approximation is quantized voltage steps. So, unit-less.
>>> Converting it to the exact voltage requires identifying the pmic voltage
>>> steps and other stuffs which are outside of my expertise.
>>>
>>> It is convenient if we can abstract it as an integer which correlates
>>> with the voltage margin that should be maintained for each regulator corner.
> 
> Krzysztof,
> 
> Could you please confirm if this approach would be acceptable?
> 
> To reiterate, move the lower 16 bits which is same across OPPs to the
> driver. Abstract the higher 16 bits as number of quantized voltage
> margin when ACD mitigation gets triggered.

I know I'm not Krzysztof, but given this is ultimately a magic value
passed to the firmware, I'm a bit lukewarm on decomposing it and would
rather see the entire 32b passed in a property, so that if a future
target needs a different constant in the lower word, we don't have to
pull our hair out again, trying to add more spaghetti logic to account
for that.

Konrad

Re: [PATCH v2 2/3] dt-bindings: opp: Add v2-qcom-adreno vendor bindings

[Dmitry Baryshkov]

On Mon, Dec 23, 2024 at 12:31:27PM +0100, Konrad Dybcio wrote:
> On 4.12.2024 7:18 PM, Akhil P Oommen wrote:
> > On 11/16/2024 1:17 AM, Dmitry Baryshkov wrote:
> >> On Fri, 15 Nov 2024 at 19:54, Akhil P Oommen <quic_akhilpo@quicinc.com> wrote:
> >>>
> >>> On 11/15/2024 3:54 AM, Dmitry Baryshkov wrote:
> >>>> Hello Akhil,
> >>>>
> >>>> On Thu, 14 Nov 2024 at 20:50, Akhil P Oommen <quic_akhilpo@quicinc.com> wrote:
> >>>>>
> >>>>> On 11/1/2024 9:54 PM, Akhil P Oommen wrote:
> >>>>>> On 10/25/2024 11:58 AM, Dmitry Baryshkov wrote:
> >>>>>>> On Thu, Oct 24, 2024 at 12:56:58AM +0530, Akhil P Oommen wrote:
> >>>>>>>> On 10/22/2024 11:19 AM, Krzysztof Kozlowski wrote:
> >>>>>>>>> On Mon, Oct 21, 2024 at 05:23:43PM +0530, Akhil P Oommen wrote:
> >>>>>>>>>> Add a new schema which extends opp-v2 to support a new vendor specific
> >>>>>>>>>> property required for Adreno GPUs found in Qualcomm's SoCs. The new
> >>>>>>>>>> property called "qcom,opp-acd-level" carries a u32 value recommended
> >>>>>>>>>> for each opp needs to be shared to GMU during runtime.
> >>>>>>>>>>
> >>>>>>>>>> Cc: Rob Clark <robdclark@gmail.com>
> >>>>>>>>>> Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
> >>>>>>>>>> ---
> >>>>>>>>>>  .../bindings/opp/opp-v2-qcom-adreno.yaml           | 96 ++++++++++++++++++++++
> >>>>>>>>>>  1 file changed, 96 insertions(+)
> >>>>>>>>>>
> >>>>>>>>>> diff --git a/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml
> >>>>>>>>>> new file mode 100644
> >>>>>>>>>> index 000000000000..6d50c0405ef8
> >>>>>>>>>> --- /dev/null
> >>>>>>>>>> +++ b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml
> >>>>>>>>>> @@ -0,0 +1,96 @@
> >>>>>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>>>>>>>>> +%YAML 1.2
> >>>>>>>>>> +---
> >>>>>>>>>> +$id: http://devicetree.org/schemas/opp/opp-v2-qcom-adreno.yaml#
> >>>>>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>>>>>>>> +
> >>>>>>>>>> +title: Qualcomm Adreno compatible OPP supply
> >>>>>>>>>> +
> >>>>>>>>>> +description:
> >>>>>>>>>> +  Adreno GPUs present in Qualcomm's Snapdragon chipsets uses an OPP specific
> >>>>>>>>>> +  ACD related information tailored for the specific chipset. This binding
> >>>>>>>>>> +  provides the information needed to describe such a hardware value.
> >>>>>>>>>> +
> >>>>>>>>>> +maintainers:
> >>>>>>>>>> +  - Rob Clark <robdclark@gmail.com>
> >>>>>>>>>> +
> >>>>>>>>>> +allOf:
> >>>>>>>>>> +  - $ref: opp-v2-base.yaml#
> >>>>>>>>>> +
> >>>>>>>>>> +properties:
> >>>>>>>>>> +  compatible:
> >>>>>>>>>> +    items:
> >>>>>>>>>> +      - const: operating-points-v2-adreno
> >>>>>>>>>> +      - const: operating-points-v2
> >>>>>>>>>> +
> >>>>>>>>>> +patternProperties:
> >>>>>>>>>> +  '^opp-?[0-9]+$':
> >>>>>>>>>
> >>>>>>>>> '-' should not be optional. opp1 is not expected name.
> >>>>>>>>
> >>>>>>>> Agree. Will change this to '^opp-[0-9]+$'
> >>>>>>>>
> >>>>>>>>>
> >>>>>>>>>> +    type: object
> >>>>>>>>>> +    additionalProperties: false
> >>>>>>>>>> +
> >>>>>>>>>> +    properties:
> >>>>>>>>>> +      opp-hz: true
> >>>>>>>>>> +
> >>>>>>>>>> +      opp-level: true
> >>>>>>>>>> +
> >>>>>>>>>> +      opp-peak-kBps: true
> >>>>>>>>>> +
> >>>>>>>>>> +      opp-supported-hw: true
> >>>>>>>>>> +
> >>>>>>>>>> +      qcom,opp-acd-level:
> >>>>>>>>>> +        description: |
> >>>>>>>>>> +          A positive value representing the ACD (Adaptive Clock Distribution,
> >>>>>>>>>> +          a fancy name for clk throttling during voltage droop) level associated
> >>>>>>>>>> +          with this OPP node. This value is shared to a co-processor inside GPU
> >>>>>>>>>> +          (called Graphics Management Unit a.k.a GMU) during wake up. It may not
> >>>>>>>>>> +          be present for some OPPs and GMU will disable ACD while transitioning
> >>>>>>>>>> +          to that OPP. This value encodes a voltage threshold and few other knobs
> >>>>>>>>>> +          which are identified by characterization of the SoC. So, it doesn't have
> >>>>>>>>>> +          any unit.
> >>>>>>>>>
> >>>>>>>>> Thanks for explanation and other updates. I am still not happy with this
> >>>>>>>>> property. I do not see reason why DT should encode magic values in a
> >>>>>>>>> quite generic piece of code. This creates poor ABI, difficult to
> >>>>>>>>> maintain or understand.
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> Configuring GPU ACD block with its respective value is a requirement for each OPP.
> >>>>>>>> So OPP node seems like the natural place for this data.
> >>>>>>>>
> >>>>>>>> If it helps to resolve your concerns, I can elaborate the documentation with
> >>>>>>>> details on the GMU HFI interface where this value should be passed on to the
> >>>>>>>> hardware. Also replace "few other knobs" with "Delay cycles & Calibration margin"
> >>>>>>>> in the above doc.
> >>>>>>>
> >>>>>>> Usually the preference for DT is to specify data in a sensible way
> >>>>>>> rather than just the values being programmed to the register. Is it
> >>>>>>> possible to implement this approach for ACD values?
> >>>>>
> >>>>> Krzysztof/Dmitry,
> >>>>>
> >>>>> BIT(0)-BIT(15) are static configurations which doesn't change between
> >>>>> OPPs. We can move it to driver.
> >>>>>
> >>>>> BIT(16)-BIT(31) indicates a threshold margin which triggers ACD. We can
> >>>>> keep this in the devicetree. And the driver can construct the final
> >>>>> value from both data and send it to GMU.
> >>>>>
> >>>>> If this is acceptable, I will send the v3 revision.
> >>>>
> >>>> Can the upper bitfield have a sensible representation in DT (like uV
> >>>> or something similar)?
> >>>
> >>> Closest approximation is quantized voltage steps. So, unit-less.
> >>> Converting it to the exact voltage requires identifying the pmic voltage
> >>> steps and other stuffs which are outside of my expertise.
> >>>
> >>> It is convenient if we can abstract it as an integer which correlates
> >>> with the voltage margin that should be maintained for each regulator corner.
> > 
> > Krzysztof,
> > 
> > Could you please confirm if this approach would be acceptable?
> > 
> > To reiterate, move the lower 16 bits which is same across OPPs to the
> > driver. Abstract the higher 16 bits as number of quantized voltage
> > margin when ACD mitigation gets triggered.
> 
> I know I'm not Krzysztof, but given this is ultimately a magic value
> passed to the firmware, I'm a bit lukewarm on decomposing it and would
> rather see the entire 32b passed in a property, so that if a future
> target needs a different constant in the lower word, we don't have to
> pull our hair out again, trying to add more spaghetti logic to account
> for that.

Also obviously being non-Krzysztof, if we don't have a semantic value
for the upper half I'm fine with having the magic value as a single
instance instead of spreading it between two places.

-- 
With best wishes
Dmitry

Re: [PATCH v2 2/3] dt-bindings: opp: Add v2-qcom-adreno vendor bindings

[Akhil P Oommen]

On 12/23/2024 5:24 PM, Dmitry Baryshkov wrote:
> On Mon, Dec 23, 2024 at 12:31:27PM +0100, Konrad Dybcio wrote:
>> On 4.12.2024 7:18 PM, Akhil P Oommen wrote:
>>> On 11/16/2024 1:17 AM, Dmitry Baryshkov wrote:
>>>> On Fri, 15 Nov 2024 at 19:54, Akhil P Oommen <quic_akhilpo@quicinc.com> wrote:
>>>>>
>>>>> On 11/15/2024 3:54 AM, Dmitry Baryshkov wrote:
>>>>>> Hello Akhil,
>>>>>>
>>>>>> On Thu, 14 Nov 2024 at 20:50, Akhil P Oommen <quic_akhilpo@quicinc.com> wrote:
>>>>>>>
>>>>>>> On 11/1/2024 9:54 PM, Akhil P Oommen wrote:
>>>>>>>> On 10/25/2024 11:58 AM, Dmitry Baryshkov wrote:
>>>>>>>>> On Thu, Oct 24, 2024 at 12:56:58AM +0530, Akhil P Oommen wrote:
>>>>>>>>>> On 10/22/2024 11:19 AM, Krzysztof Kozlowski wrote:
>>>>>>>>>>> On Mon, Oct 21, 2024 at 05:23:43PM +0530, Akhil P Oommen wrote:
>>>>>>>>>>>> Add a new schema which extends opp-v2 to support a new vendor specific
>>>>>>>>>>>> property required for Adreno GPUs found in Qualcomm's SoCs. The new
>>>>>>>>>>>> property called "qcom,opp-acd-level" carries a u32 value recommended
>>>>>>>>>>>> for each opp needs to be shared to GMU during runtime.
>>>>>>>>>>>>
>>>>>>>>>>>> Cc: Rob Clark <robdclark@gmail.com>
>>>>>>>>>>>> Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
>>>>>>>>>>>> ---
>>>>>>>>>>>>  .../bindings/opp/opp-v2-qcom-adreno.yaml           | 96 ++++++++++++++++++++++
>>>>>>>>>>>>  1 file changed, 96 insertions(+)
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml
>>>>>>>>>>>> new file mode 100644
>>>>>>>>>>>> index 000000000000..6d50c0405ef8
>>>>>>>>>>>> --- /dev/null
>>>>>>>>>>>> +++ b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml
>>>>>>>>>>>> @@ -0,0 +1,96 @@
>>>>>>>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>>>>>>>>> +%YAML 1.2
>>>>>>>>>>>> +---
>>>>>>>>>>>> +$id: http://devicetree.org/schemas/opp/opp-v2-qcom-adreno.yaml#
>>>>>>>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>>>>>>>> +
>>>>>>>>>>>> +title: Qualcomm Adreno compatible OPP supply
>>>>>>>>>>>> +
>>>>>>>>>>>> +description:
>>>>>>>>>>>> +  Adreno GPUs present in Qualcomm's Snapdragon chipsets uses an OPP specific
>>>>>>>>>>>> +  ACD related information tailored for the specific chipset. This binding
>>>>>>>>>>>> +  provides the information needed to describe such a hardware value.
>>>>>>>>>>>> +
>>>>>>>>>>>> +maintainers:
>>>>>>>>>>>> +  - Rob Clark <robdclark@gmail.com>
>>>>>>>>>>>> +
>>>>>>>>>>>> +allOf:
>>>>>>>>>>>> +  - $ref: opp-v2-base.yaml#
>>>>>>>>>>>> +
>>>>>>>>>>>> +properties:
>>>>>>>>>>>> +  compatible:
>>>>>>>>>>>> +    items:
>>>>>>>>>>>> +      - const: operating-points-v2-adreno
>>>>>>>>>>>> +      - const: operating-points-v2
>>>>>>>>>>>> +
>>>>>>>>>>>> +patternProperties:
>>>>>>>>>>>> +  '^opp-?[0-9]+$':
>>>>>>>>>>>
>>>>>>>>>>> '-' should not be optional. opp1 is not expected name.
>>>>>>>>>>
>>>>>>>>>> Agree. Will change this to '^opp-[0-9]+$'
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> +    type: object
>>>>>>>>>>>> +    additionalProperties: false
>>>>>>>>>>>> +
>>>>>>>>>>>> +    properties:
>>>>>>>>>>>> +      opp-hz: true
>>>>>>>>>>>> +
>>>>>>>>>>>> +      opp-level: true
>>>>>>>>>>>> +
>>>>>>>>>>>> +      opp-peak-kBps: true
>>>>>>>>>>>> +
>>>>>>>>>>>> +      opp-supported-hw: true
>>>>>>>>>>>> +
>>>>>>>>>>>> +      qcom,opp-acd-level:
>>>>>>>>>>>> +        description: |
>>>>>>>>>>>> +          A positive value representing the ACD (Adaptive Clock Distribution,
>>>>>>>>>>>> +          a fancy name for clk throttling during voltage droop) level associated
>>>>>>>>>>>> +          with this OPP node. This value is shared to a co-processor inside GPU
>>>>>>>>>>>> +          (called Graphics Management Unit a.k.a GMU) during wake up. It may not
>>>>>>>>>>>> +          be present for some OPPs and GMU will disable ACD while transitioning
>>>>>>>>>>>> +          to that OPP. This value encodes a voltage threshold and few other knobs
>>>>>>>>>>>> +          which are identified by characterization of the SoC. So, it doesn't have
>>>>>>>>>>>> +          any unit.
>>>>>>>>>>>
>>>>>>>>>>> Thanks for explanation and other updates. I am still not happy with this
>>>>>>>>>>> property. I do not see reason why DT should encode magic values in a
>>>>>>>>>>> quite generic piece of code. This creates poor ABI, difficult to
>>>>>>>>>>> maintain or understand.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Configuring GPU ACD block with its respective value is a requirement for each OPP.
>>>>>>>>>> So OPP node seems like the natural place for this data.
>>>>>>>>>>
>>>>>>>>>> If it helps to resolve your concerns, I can elaborate the documentation with
>>>>>>>>>> details on the GMU HFI interface where this value should be passed on to the
>>>>>>>>>> hardware. Also replace "few other knobs" with "Delay cycles & Calibration margin"
>>>>>>>>>> in the above doc.
>>>>>>>>>
>>>>>>>>> Usually the preference for DT is to specify data in a sensible way
>>>>>>>>> rather than just the values being programmed to the register. Is it
>>>>>>>>> possible to implement this approach for ACD values?
>>>>>>>
>>>>>>> Krzysztof/Dmitry,
>>>>>>>
>>>>>>> BIT(0)-BIT(15) are static configurations which doesn't change between
>>>>>>> OPPs. We can move it to driver.
>>>>>>>
>>>>>>> BIT(16)-BIT(31) indicates a threshold margin which triggers ACD. We can
>>>>>>> keep this in the devicetree. And the driver can construct the final
>>>>>>> value from both data and send it to GMU.
>>>>>>>
>>>>>>> If this is acceptable, I will send the v3 revision.
>>>>>>
>>>>>> Can the upper bitfield have a sensible representation in DT (like uV
>>>>>> or something similar)?
>>>>>
>>>>> Closest approximation is quantized voltage steps. So, unit-less.
>>>>> Converting it to the exact voltage requires identifying the pmic voltage
>>>>> steps and other stuffs which are outside of my expertise.
>>>>>
>>>>> It is convenient if we can abstract it as an integer which correlates
>>>>> with the voltage margin that should be maintained for each regulator corner.
>>>
>>> Krzysztof,
>>>
>>> Could you please confirm if this approach would be acceptable?
>>>
>>> To reiterate, move the lower 16 bits which is same across OPPs to the
>>> driver. Abstract the higher 16 bits as number of quantized voltage
>>> margin when ACD mitigation gets triggered.
>>
>> I know I'm not Krzysztof, but given this is ultimately a magic value
>> passed to the firmware, I'm a bit lukewarm on decomposing it and would
>> rather see the entire 32b passed in a property, so that if a future
>> target needs a different constant in the lower word, we don't have to
>> pull our hair out again, trying to add more spaghetti logic to account
>> for that.
> 
> Also obviously being non-Krzysztof, if we don't have a semantic value
> for the upper half I'm fine with having the magic value as a single
> instance instead of spreading it between two places.

If there is a general consensus, I will send out another revision with
some minor updates.

-Akhil.

> 

Re: [PATCH v2 2/3] dt-bindings: opp: Add v2-qcom-adreno vendor bindings

[Krzysztof Kozlowski]

On 23/12/2024 22:31, Akhil P Oommen wrote:
> On 12/23/2024 5:24 PM, Dmitry Baryshkov wrote:
>> On Mon, Dec 23, 2024 at 12:31:27PM +0100, Konrad Dybcio wrote:
>>> On 4.12.2024 7:18 PM, Akhil P Oommen wrote:
>>>> On 11/16/2024 1:17 AM, Dmitry Baryshkov wrote:
>>>>> On Fri, 15 Nov 2024 at 19:54, Akhil P Oommen <quic_akhilpo@quicinc.com> wrote:
>>>>>>
>>>>>> On 11/15/2024 3:54 AM, Dmitry Baryshkov wrote:
>>>>>>> Hello Akhil,
>>>>>>>
>>>>>>> On Thu, 14 Nov 2024 at 20:50, Akhil P Oommen <quic_akhilpo@quicinc.com> wrote:
>>>>>>>>
>>>>>>>> On 11/1/2024 9:54 PM, Akhil P Oommen wrote:
>>>>>>>>> On 10/25/2024 11:58 AM, Dmitry Baryshkov wrote:
>>>>>>>>>> On Thu, Oct 24, 2024 at 12:56:58AM +0530, Akhil P Oommen wrote:
>>>>>>>>>>> On 10/22/2024 11:19 AM, Krzysztof Kozlowski wrote:
>>>>>>>>>>>> On Mon, Oct 21, 2024 at 05:23:43PM +0530, Akhil P Oommen wrote:
>>>>>>>>>>>>> Add a new schema which extends opp-v2 to support a new vendor specific
>>>>>>>>>>>>> property required for Adreno GPUs found in Qualcomm's SoCs. The new
>>>>>>>>>>>>> property called "qcom,opp-acd-level" carries a u32 value recommended
>>>>>>>>>>>>> for each opp needs to be shared to GMU during runtime.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Cc: Rob Clark <robdclark@gmail.com>
>>>>>>>>>>>>> Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>>  .../bindings/opp/opp-v2-qcom-adreno.yaml           | 96 ++++++++++++++++++++++
>>>>>>>>>>>>>  1 file changed, 96 insertions(+)
>>>>>>>>>>>>>
>>>>>>>>>>>>> diff --git a/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml
>>>>>>>>>>>>> new file mode 100644
>>>>>>>>>>>>> index 000000000000..6d50c0405ef8
>>>>>>>>>>>>> --- /dev/null
>>>>>>>>>>>>> +++ b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml
>>>>>>>>>>>>> @@ -0,0 +1,96 @@
>>>>>>>>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>>>>>>>>>> +%YAML 1.2
>>>>>>>>>>>>> +---
>>>>>>>>>>>>> +$id: http://devicetree.org/schemas/opp/opp-v2-qcom-adreno.yaml#
>>>>>>>>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +title: Qualcomm Adreno compatible OPP supply
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +description:
>>>>>>>>>>>>> +  Adreno GPUs present in Qualcomm's Snapdragon chipsets uses an OPP specific
>>>>>>>>>>>>> +  ACD related information tailored for the specific chipset. This binding
>>>>>>>>>>>>> +  provides the information needed to describe such a hardware value.
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +maintainers:
>>>>>>>>>>>>> +  - Rob Clark <robdclark@gmail.com>
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +allOf:
>>>>>>>>>>>>> +  - $ref: opp-v2-base.yaml#
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +properties:
>>>>>>>>>>>>> +  compatible:
>>>>>>>>>>>>> +    items:
>>>>>>>>>>>>> +      - const: operating-points-v2-adreno
>>>>>>>>>>>>> +      - const: operating-points-v2
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +patternProperties:
>>>>>>>>>>>>> +  '^opp-?[0-9]+$':
>>>>>>>>>>>>
>>>>>>>>>>>> '-' should not be optional. opp1 is not expected name.
>>>>>>>>>>>
>>>>>>>>>>> Agree. Will change this to '^opp-[0-9]+$'
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>> +    type: object
>>>>>>>>>>>>> +    additionalProperties: false
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +    properties:
>>>>>>>>>>>>> +      opp-hz: true
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +      opp-level: true
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +      opp-peak-kBps: true
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +      opp-supported-hw: true
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +      qcom,opp-acd-level:
>>>>>>>>>>>>> +        description: |
>>>>>>>>>>>>> +          A positive value representing the ACD (Adaptive Clock Distribution,
>>>>>>>>>>>>> +          a fancy name for clk throttling during voltage droop) level associated
>>>>>>>>>>>>> +          with this OPP node. This value is shared to a co-processor inside GPU
>>>>>>>>>>>>> +          (called Graphics Management Unit a.k.a GMU) during wake up. It may not
>>>>>>>>>>>>> +          be present for some OPPs and GMU will disable ACD while transitioning
>>>>>>>>>>>>> +          to that OPP. This value encodes a voltage threshold and few other knobs
>>>>>>>>>>>>> +          which are identified by characterization of the SoC. So, it doesn't have
>>>>>>>>>>>>> +          any unit.
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks for explanation and other updates. I am still not happy with this
>>>>>>>>>>>> property. I do not see reason why DT should encode magic values in a
>>>>>>>>>>>> quite generic piece of code. This creates poor ABI, difficult to
>>>>>>>>>>>> maintain or understand.
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Configuring GPU ACD block with its respective value is a requirement for each OPP.
>>>>>>>>>>> So OPP node seems like the natural place for this data.
>>>>>>>>>>>
>>>>>>>>>>> If it helps to resolve your concerns, I can elaborate the documentation with
>>>>>>>>>>> details on the GMU HFI interface where this value should be passed on to the
>>>>>>>>>>> hardware. Also replace "few other knobs" with "Delay cycles & Calibration margin"
>>>>>>>>>>> in the above doc.
>>>>>>>>>>
>>>>>>>>>> Usually the preference for DT is to specify data in a sensible way
>>>>>>>>>> rather than just the values being programmed to the register. Is it
>>>>>>>>>> possible to implement this approach for ACD values?
>>>>>>>>
>>>>>>>> Krzysztof/Dmitry,
>>>>>>>>
>>>>>>>> BIT(0)-BIT(15) are static configurations which doesn't change between
>>>>>>>> OPPs. We can move it to driver.
>>>>>>>>
>>>>>>>> BIT(16)-BIT(31) indicates a threshold margin which triggers ACD. We can
>>>>>>>> keep this in the devicetree. And the driver can construct the final
>>>>>>>> value from both data and send it to GMU.
>>>>>>>>
>>>>>>>> If this is acceptable, I will send the v3 revision.
>>>>>>>
>>>>>>> Can the upper bitfield have a sensible representation in DT (like uV
>>>>>>> or something similar)?
>>>>>>
>>>>>> Closest approximation is quantized voltage steps. So, unit-less.
>>>>>> Converting it to the exact voltage requires identifying the pmic voltage
>>>>>> steps and other stuffs which are outside of my expertise.
>>>>>>
>>>>>> It is convenient if we can abstract it as an integer which correlates
>>>>>> with the voltage margin that should be maintained for each regulator corner.
>>>>
>>>> Krzysztof,
>>>>
>>>> Could you please confirm if this approach would be acceptable?
>>>>
>>>> To reiterate, move the lower 16 bits which is same across OPPs to the
>>>> driver. Abstract the higher 16 bits as number of quantized voltage
>>>> margin when ACD mitigation gets triggered.
>>>
>>> I know I'm not Krzysztof, but given this is ultimately a magic value
>>> passed to the firmware, I'm a bit lukewarm on decomposing it and would
>>> rather see the entire 32b passed in a property, so that if a future
>>> target needs a different constant in the lower word, we don't have to
>>> pull our hair out again, trying to add more spaghetti logic to account
>>> for that.

I can also imagine future SoC not respecting existing interface and
switching to something new, duplicating the effort. All this is "driven
by downstream" approach... but sure, let's go with existing approach.



Best regards,
Krzysztof

Re: [PATCH v2 2/3] dt-bindings: opp: Add v2-qcom-adreno vendor bindings

[Konrad Dybcio]

On 24.12.2024 9:51 AM, Krzysztof Kozlowski wrote:
> On 23/12/2024 22:31, Akhil P Oommen wrote:
>> On 12/23/2024 5:24 PM, Dmitry Baryshkov wrote:
>>> On Mon, Dec 23, 2024 at 12:31:27PM +0100, Konrad Dybcio wrote:
>>>> On 4.12.2024 7:18 PM, Akhil P Oommen wrote:
>>>>> On 11/16/2024 1:17 AM, Dmitry Baryshkov wrote:
>>>>>> On Fri, 15 Nov 2024 at 19:54, Akhil P Oommen <quic_akhilpo@quicinc.com> wrote:
>>>>>>>
>>>>>>> On 11/15/2024 3:54 AM, Dmitry Baryshkov wrote:
>>>>>>>> Hello Akhil,
>>>>>>>>
>>>>>>>> On Thu, 14 Nov 2024 at 20:50, Akhil P Oommen <quic_akhilpo@quicinc.com> wrote:
>>>>>>>>>
>>>>>>>>> On 11/1/2024 9:54 PM, Akhil P Oommen wrote:
>>>>>>>>>> On 10/25/2024 11:58 AM, Dmitry Baryshkov wrote:
>>>>>>>>>>> On Thu, Oct 24, 2024 at 12:56:58AM +0530, Akhil P Oommen wrote:
>>>>>>>>>>>> On 10/22/2024 11:19 AM, Krzysztof Kozlowski wrote:
>>>>>>>>>>>>> On Mon, Oct 21, 2024 at 05:23:43PM +0530, Akhil P Oommen wrote:
>>>>>>>>>>>>>> Add a new schema which extends opp-v2 to support a new vendor specific
>>>>>>>>>>>>>> property required for Adreno GPUs found in Qualcomm's SoCs. The new
>>>>>>>>>>>>>> property called "qcom,opp-acd-level" carries a u32 value recommended
>>>>>>>>>>>>>> for each opp needs to be shared to GMU during runtime.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Cc: Rob Clark <robdclark@gmail.com>
>>>>>>>>>>>>>> Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>  .../bindings/opp/opp-v2-qcom-adreno.yaml           | 96 ++++++++++++++++++++++
>>>>>>>>>>>>>>  1 file changed, 96 insertions(+)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> diff --git a/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml
>>>>>>>>>>>>>> new file mode 100644
>>>>>>>>>>>>>> index 000000000000..6d50c0405ef8
>>>>>>>>>>>>>> --- /dev/null
>>>>>>>>>>>>>> +++ b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml
>>>>>>>>>>>>>> @@ -0,0 +1,96 @@
>>>>>>>>>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>>>>>>>>>>> +%YAML 1.2
>>>>>>>>>>>>>> +---
>>>>>>>>>>>>>> +$id: http://devicetree.org/schemas/opp/opp-v2-qcom-adreno.yaml#
>>>>>>>>>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> +title: Qualcomm Adreno compatible OPP supply
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> +description:
>>>>>>>>>>>>>> +  Adreno GPUs present in Qualcomm's Snapdragon chipsets uses an OPP specific
>>>>>>>>>>>>>> +  ACD related information tailored for the specific chipset. This binding
>>>>>>>>>>>>>> +  provides the information needed to describe such a hardware value.
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> +maintainers:
>>>>>>>>>>>>>> +  - Rob Clark <robdclark@gmail.com>
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> +allOf:
>>>>>>>>>>>>>> +  - $ref: opp-v2-base.yaml#
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> +properties:
>>>>>>>>>>>>>> +  compatible:
>>>>>>>>>>>>>> +    items:
>>>>>>>>>>>>>> +      - const: operating-points-v2-adreno
>>>>>>>>>>>>>> +      - const: operating-points-v2
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> +patternProperties:
>>>>>>>>>>>>>> +  '^opp-?[0-9]+$':
>>>>>>>>>>>>>
>>>>>>>>>>>>> '-' should not be optional. opp1 is not expected name.
>>>>>>>>>>>>
>>>>>>>>>>>> Agree. Will change this to '^opp-[0-9]+$'
>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>> +    type: object
>>>>>>>>>>>>>> +    additionalProperties: false
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> +    properties:
>>>>>>>>>>>>>> +      opp-hz: true
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> +      opp-level: true
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> +      opp-peak-kBps: true
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> +      opp-supported-hw: true
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> +      qcom,opp-acd-level:
>>>>>>>>>>>>>> +        description: |
>>>>>>>>>>>>>> +          A positive value representing the ACD (Adaptive Clock Distribution,
>>>>>>>>>>>>>> +          a fancy name for clk throttling during voltage droop) level associated
>>>>>>>>>>>>>> +          with this OPP node. This value is shared to a co-processor inside GPU
>>>>>>>>>>>>>> +          (called Graphics Management Unit a.k.a GMU) during wake up. It may not
>>>>>>>>>>>>>> +          be present for some OPPs and GMU will disable ACD while transitioning
>>>>>>>>>>>>>> +          to that OPP. This value encodes a voltage threshold and few other knobs
>>>>>>>>>>>>>> +          which are identified by characterization of the SoC. So, it doesn't have
>>>>>>>>>>>>>> +          any unit.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks for explanation and other updates. I am still not happy with this
>>>>>>>>>>>>> property. I do not see reason why DT should encode magic values in a
>>>>>>>>>>>>> quite generic piece of code. This creates poor ABI, difficult to
>>>>>>>>>>>>> maintain or understand.
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Configuring GPU ACD block with its respective value is a requirement for each OPP.
>>>>>>>>>>>> So OPP node seems like the natural place for this data.
>>>>>>>>>>>>
>>>>>>>>>>>> If it helps to resolve your concerns, I can elaborate the documentation with
>>>>>>>>>>>> details on the GMU HFI interface where this value should be passed on to the
>>>>>>>>>>>> hardware. Also replace "few other knobs" with "Delay cycles & Calibration margin"
>>>>>>>>>>>> in the above doc.
>>>>>>>>>>>
>>>>>>>>>>> Usually the preference for DT is to specify data in a sensible way
>>>>>>>>>>> rather than just the values being programmed to the register. Is it
>>>>>>>>>>> possible to implement this approach for ACD values?
>>>>>>>>>
>>>>>>>>> Krzysztof/Dmitry,
>>>>>>>>>
>>>>>>>>> BIT(0)-BIT(15) are static configurations which doesn't change between
>>>>>>>>> OPPs. We can move it to driver.
>>>>>>>>>
>>>>>>>>> BIT(16)-BIT(31) indicates a threshold margin which triggers ACD. We can
>>>>>>>>> keep this in the devicetree. And the driver can construct the final
>>>>>>>>> value from both data and send it to GMU.
>>>>>>>>>
>>>>>>>>> If this is acceptable, I will send the v3 revision.
>>>>>>>>
>>>>>>>> Can the upper bitfield have a sensible representation in DT (like uV
>>>>>>>> or something similar)?
>>>>>>>
>>>>>>> Closest approximation is quantized voltage steps. So, unit-less.
>>>>>>> Converting it to the exact voltage requires identifying the pmic voltage
>>>>>>> steps and other stuffs which are outside of my expertise.
>>>>>>>
>>>>>>> It is convenient if we can abstract it as an integer which correlates
>>>>>>> with the voltage margin that should be maintained for each regulator corner.
>>>>>
>>>>> Krzysztof,
>>>>>
>>>>> Could you please confirm if this approach would be acceptable?
>>>>>
>>>>> To reiterate, move the lower 16 bits which is same across OPPs to the
>>>>> driver. Abstract the higher 16 bits as number of quantized voltage
>>>>> margin when ACD mitigation gets triggered.
>>>>
>>>> I know I'm not Krzysztof, but given this is ultimately a magic value
>>>> passed to the firmware, I'm a bit lukewarm on decomposing it and would
>>>> rather see the entire 32b passed in a property, so that if a future
>>>> target needs a different constant in the lower word, we don't have to
>>>> pull our hair out again, trying to add more spaghetti logic to account
>>>> for that.
> 
> I can also imagine future SoC not respecting existing interface and
> switching to something new, duplicating the effort. All this is "driven
> by downstream" approach... but sure, let's go with existing approach.

Yeah I'd much rather have the firmware contain borderline-blob data,
but I guess the sw architecture accounts for easier DVFS table
replacement instead

Konrad