[PATCH 0/6] cpufreq for H616

[PATCH 0/6] cpufreq for H616

[Martin Botka]

Hello,

This patch series adds support for cpufreq on H616 SoC.

H616 is bit interesting. It has SoC versions that have different
frequencies and uV but some versions have the same version ID and
we have to check the SoC revision to differentiate between them.

This is done via SMCCC. Thus the exporting of the symbol.

Please note that this series depends on my THS series which
depends on my SID series.

I also have not enabled the cpufreq on any devices to minimize
the series dependencies and I did test it only on CB1 where
it works.

Cheers,
Martin

Signed-off-by: Martin Botka <martin.botka@somainline.org>
---
Martin Botka (6):
      firmware: smccc: Export revision soc_id function
      cpufreq: dt-platdev: Blocklist allwinner,h616 SoC
      dt-bindings: opp: Add compatible for H616
      cpufreq: sun50i: Add H616 support
      arm64: dts: allwinner: h616: Add CPU Operating Performance Points table
      arm64: dts: allwinner: h616: Add cooling cells

 .../opp/allwinner,sun50i-h6-operating-points.yaml  |   6 +-
 .../boot/dts/allwinner/sun50i-h616-cpu-opp.dtsi    | 129 ++++++++++++++++++
 arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi     |   8 ++
 drivers/cpufreq/cpufreq-dt-platdev.c               |   1 +
 drivers/cpufreq/sun50i-cpufreq-nvmem.c             | 149 +++++++++++++++++----
 drivers/firmware/smccc/smccc.c                     |   1 +
 6 files changed, 270 insertions(+), 24 deletions(-)
---
base-commit: a384547b9656aa2c98f643037b0e940476c41f51
change-id: 20230824-cpufreq-h616-0370df5aea34

Best regards,
-- 
Martin Botka <martin.botka@somainline.org>

[PATCH 1/6] firmware: smccc: Export revision soc_id function

[Martin Botka]

arm_smccc_get_soc_id_revision need to be exported so it can be used
by sun50i cpufreq driver.

Signed-off-by: Martin Botka <martin.botka@somainline.org>
---
 drivers/firmware/smccc/smccc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/firmware/smccc/smccc.c b/drivers/firmware/smccc/smccc.c
index db818f9dcb8e..d670635914ec 100644
--- a/drivers/firmware/smccc/smccc.c
+++ b/drivers/firmware/smccc/smccc.c
@@ -69,6 +69,7 @@ s32 arm_smccc_get_soc_id_revision(void)
 {
 	return smccc_soc_id_revision;
 }
+EXPORT_SYMBOL_GPL(arm_smccc_get_soc_id_revision);
 
 static int __init smccc_devices_init(void)
 {

-- 
2.42.0

[PATCH 2/6] cpufreq: dt-platdev: Blocklist allwinner,h616 SoC

[Martin Botka]

The AllWinner H616 uses H6 cpufreq driver.
Add it to blocklist so its not created twice

Signed-off-by: Martin Botka <martin.botka@somainline.org>
---
 drivers/cpufreq/cpufreq-dt-platdev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c b/drivers/cpufreq/cpufreq-dt-platdev.c
index e2b20080de3a..51818cef8979 100644
--- a/drivers/cpufreq/cpufreq-dt-platdev.c
+++ b/drivers/cpufreq/cpufreq-dt-platdev.c
@@ -104,6 +104,7 @@ static const struct of_device_id allowlist[] __initconst = {
  */
 static const struct of_device_id blocklist[] __initconst = {
 	{ .compatible = "allwinner,sun50i-h6", },
+	{ .compatible = "allwinner,sun50i-h616", },
 
 	{ .compatible = "apple,arm-platform", },
 

-- 
2.42.0

[PATCH 3/6] dt-bindings: opp: Add compatible for H616

[Martin Botka]

We need to add compatible for H616 to H6 cpufreq driver bindings.

Also enable opp_supported_hw property that will be needed for H616.

Signed-off-by: Martin Botka <martin.botka@somainline.org>
---
 .../bindings/opp/allwinner,sun50i-h6-operating-points.yaml          | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/opp/allwinner,sun50i-h6-operating-points.yaml b/Documentation/devicetree/bindings/opp/allwinner,sun50i-h6-operating-points.yaml
index 51f62c3ae194..2fa1199f2d23 100644
--- a/Documentation/devicetree/bindings/opp/allwinner,sun50i-h6-operating-points.yaml
+++ b/Documentation/devicetree/bindings/opp/allwinner,sun50i-h6-operating-points.yaml
@@ -23,7 +23,10 @@ allOf:
 
 properties:
   compatible:
-    const: allwinner,sun50i-h6-operating-points
+    contains:
+      enum:
+        - allwinner,sun50i-h6-operating-points
+        - allwinner,sun50i-h616-operating-points
 
   nvmem-cells:
     description: |
@@ -47,6 +50,7 @@ patternProperties:
     properties:
       opp-hz: true
       clock-latency-ns: true
+      opp-supported-hw: true
 
     patternProperties:
       "^opp-microvolt-speed[0-9]$": true

-- 
2.42.0

[PATCH 4/6] cpufreq: sun50i: Add H616 support

[Martin Botka]

AllWinner H616 SoC has few revisions that support different list
of uV and frequencies.

Some revisions have the same NVMEM value and thus we have to check
the SoC revision from SMCCC to differentiate between them.

Signed-off-by: Martin Botka <martin.botka@somainline.org>
---
 drivers/cpufreq/sun50i-cpufreq-nvmem.c | 149 ++++++++++++++++++++++++++++-----
 1 file changed, 126 insertions(+), 23 deletions(-)

diff --git a/drivers/cpufreq/sun50i-cpufreq-nvmem.c b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
index 4321d7bbe769..19c126fb081e 100644
--- a/drivers/cpufreq/sun50i-cpufreq-nvmem.c
+++ b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
@@ -10,6 +10,7 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
+#include <linux/arm-smccc.h>
 #include <linux/cpu.h>
 #include <linux/module.h>
 #include <linux/nvmem-consumer.h>
@@ -23,20 +24,94 @@
 #define NVMEM_MASK	0x7
 #define NVMEM_SHIFT	5
 
+struct sunxi_cpufreq_soc_data {
+	int (*efuse_xlate)(u32 *versions, u32 *efuse, char *name, size_t len);
+	u8 ver_freq_limit;
+};
+
 static struct platform_device *cpufreq_dt_pdev, *sun50i_cpufreq_pdev;
 
+static int sun50i_h616_efuse_xlate(u32 *versions, u32 *efuse, char *name, size_t len)
+{
+	int value = 0;
+	u32 speedgrade = 0;
+	u32 i;
+	int ver_bits = arm_smccc_get_soc_id_revision();
+
+	if (len > 4) {
+		pr_err("Invalid nvmem cell length\n");
+		return -EINVAL;
+	}
+
+	for (i = 0; i < len; i++)
+		speedgrade |= (efuse[i] << (i * 8));
+
+	switch (speedgrade) {
+	case 0x2000:
+		value = 0;
+		break;
+	case 0x2400:
+	case 0x7400:
+	case 0x2c00:
+	case 0x7c00:
+		if (ver_bits <= 1) {
+			/* ic version A/B */
+			value = 1;
+		} else {
+			/* ic version C and later version */
+			value = 2;
+		}
+		break;
+	case 0x5000:
+	case 0x5400:
+	case 0x6000:
+		value = 3;
+		break;
+	case 0x5c00:
+		value = 4;
+		break;
+	case 0x5d00:
+	default:
+		value = 0;
+	}
+	*versions = (1 << value);
+	snprintf(name, MAX_NAME_LEN, "speed%d", value);
+	return 0;
+}
+
+static int sun50i_h6_efuse_xlate(u32 *versions, u32 *efuse, char *name, size_t len)
+{
+	int efuse_value = (*efuse >> NVMEM_SHIFT) & NVMEM_MASK;
+
+	/*
+	 * We treat unexpected efuse values as if the SoC was from
+	 * the slowest bin. Expected efuse values are 1-3, slowest
+	 * to fastest.
+	 */
+	if (efuse_value >= 1 && efuse_value <= 3)
+		*versions = efuse_value - 1;
+	else
+		*versions = 0;
+
+	snprintf(name, MAX_NAME_LEN, "speed%d", *versions);
+	return 0;
+}
+
 /**
  * sun50i_cpufreq_get_efuse() - Determine speed grade from efuse value
+ * @soc_data: Struct containing soc specific data & functions
  * @versions: Set to the value parsed from efuse
+ * @name: Set to the name of speed
  *
  * Returns 0 if success.
  */
-static int sun50i_cpufreq_get_efuse(u32 *versions)
+static int sun50i_cpufreq_get_efuse(const struct sunxi_cpufreq_soc_data *soc_data,
+				    u32 *versions, char *name)
 {
 	struct nvmem_cell *speedbin_nvmem;
 	struct device_node *np;
 	struct device *cpu_dev;
-	u32 *speedbin, efuse_value;
+	u32 *speedbin;
 	size_t len;
 	int ret;
 
@@ -48,9 +123,9 @@ static int sun50i_cpufreq_get_efuse(u32 *versions)
 	if (!np)
 		return -ENOENT;
 
-	ret = of_device_is_compatible(np,
-				      "allwinner,sun50i-h6-operating-points");
-	if (!ret) {
+	if (of_device_is_compatible(np, "allwinner,sun50i-h6-operating-points")) {
+	} else if (of_device_is_compatible(np, "allwinner,sun50i-h616-operating-points")) {
+	} else {
 		of_node_put(np);
 		return -ENOENT;
 	}
@@ -66,17 +141,9 @@ static int sun50i_cpufreq_get_efuse(u32 *versions)
 	if (IS_ERR(speedbin))
 		return PTR_ERR(speedbin);
 
-	efuse_value = (*speedbin >> NVMEM_SHIFT) & NVMEM_MASK;
-
-	/*
-	 * We treat unexpected efuse values as if the SoC was from
-	 * the slowest bin. Expected efuse values are 1-3, slowest
-	 * to fastest.
-	 */
-	if (efuse_value >= 1 && efuse_value <= 3)
-		*versions = efuse_value - 1;
-	else
-		*versions = 0;
+	ret = soc_data->efuse_xlate(versions, speedbin, name, len);
+	if (ret)
+		return ret;
 
 	kfree(speedbin);
 	return 0;
@@ -84,25 +151,30 @@ static int sun50i_cpufreq_get_efuse(u32 *versions)
 
 static int sun50i_cpufreq_nvmem_probe(struct platform_device *pdev)
 {
+	const struct of_device_id *match;
+	const struct sunxi_cpufreq_soc_data *soc_data;
 	int *opp_tokens;
 	char name[MAX_NAME_LEN];
 	unsigned int cpu;
-	u32 speed = 0;
+	u32 version = 0;
 	int ret;
 
+	match = dev_get_platdata(&pdev->dev);
+	if (!match)
+		return -EINVAL;
+	soc_data = match->data;
+
 	opp_tokens = kcalloc(num_possible_cpus(), sizeof(*opp_tokens),
 			     GFP_KERNEL);
 	if (!opp_tokens)
 		return -ENOMEM;
 
-	ret = sun50i_cpufreq_get_efuse(&speed);
+	ret = sun50i_cpufreq_get_efuse(match->data, &version, name);
 	if (ret) {
 		kfree(opp_tokens);
 		return ret;
 	}
 
-	snprintf(name, MAX_NAME_LEN, "speed%d", speed);
-
 	for_each_possible_cpu(cpu) {
 		struct device *cpu_dev = get_cpu_device(cpu);
 
@@ -117,6 +189,16 @@ static int sun50i_cpufreq_nvmem_probe(struct platform_device *pdev)
 			pr_err("Failed to set prop name\n");
 			goto free_opp;
 		}
+
+		if (soc_data->ver_freq_limit) {
+			opp_tokens[cpu] = dev_pm_opp_set_supported_hw(cpu_dev,
+								  &version, 1);
+			if (opp_tokens[cpu] < 0) {
+				ret = opp_tokens[cpu];
+				pr_err("Failed to set hw\n");
+				goto free_opp;
+			}
+		}
 	}
 
 	cpufreq_dt_pdev = platform_device_register_simple("cpufreq-dt", -1,
@@ -132,6 +214,8 @@ static int sun50i_cpufreq_nvmem_probe(struct platform_device *pdev)
 free_opp:
 	for_each_possible_cpu(cpu)
 		dev_pm_opp_put_prop_name(opp_tokens[cpu]);
+		if (soc_data->ver_freq_limit)
+			dev_pm_opp_put_supported_hw(opp_tokens[cpu]);
 	kfree(opp_tokens);
 
 	return ret;
@@ -140,12 +224,21 @@ static int sun50i_cpufreq_nvmem_probe(struct platform_device *pdev)
 static int sun50i_cpufreq_nvmem_remove(struct platform_device *pdev)
 {
 	int *opp_tokens = platform_get_drvdata(pdev);
+	const struct of_device_id *match;
+	const struct sunxi_cpufreq_soc_data *soc_data;
 	unsigned int cpu;
 
+	match = dev_get_platdata(&pdev->dev);
+	if (!match)
+		return -EINVAL;
+	soc_data = match->data;
+
 	platform_device_unregister(cpufreq_dt_pdev);
 
 	for_each_possible_cpu(cpu)
 		dev_pm_opp_put_prop_name(opp_tokens[cpu]);
+		if (soc_data->ver_freq_limit)
+			dev_pm_opp_put_supported_hw(opp_tokens[cpu]);
 
 	kfree(opp_tokens);
 
@@ -160,8 +253,18 @@ static struct platform_driver sun50i_cpufreq_driver = {
 	},
 };
 
+static const struct sunxi_cpufreq_soc_data sun50i_h616_data = {
+	.efuse_xlate = sun50i_h616_efuse_xlate,
+	.ver_freq_limit = true,
+};
+
+static const struct sunxi_cpufreq_soc_data sun50i_h6_data = {
+	.efuse_xlate = sun50i_h6_efuse_xlate,
+};
+
 static const struct of_device_id sun50i_cpufreq_match_list[] = {
-	{ .compatible = "allwinner,sun50i-h6" },
+	{ .compatible = "allwinner,sun50i-h6", .data = &sun50i_h6_data },
+	{ .compatible = "allwinner,sun50i-h616", .data = &sun50i_h616_data },
 	{}
 };
 MODULE_DEVICE_TABLE(of, sun50i_cpufreq_match_list);
@@ -197,8 +300,8 @@ static int __init sun50i_cpufreq_init(void)
 		return ret;
 
 	sun50i_cpufreq_pdev =
-		platform_device_register_simple("sun50i-cpufreq-nvmem",
-						-1, NULL, 0);
+		platform_device_register_data(NULL, "sun50i-cpufreq-nvmem",
+						-1, match, sizeof(*match));
 	ret = PTR_ERR_OR_ZERO(sun50i_cpufreq_pdev);
 	if (ret == 0)
 		return 0;

-- 
2.42.0

[PATCH 5/6] arm64: dts: allwinner: h616: Add CPU Operating Performance Points table

[Martin Botka]

Add an Operating Performance Points table for the CPU cores to
enable Dynamic Voltage & Frequency Scaling on the H616.

Also add the needed cpu_speed_grade nvmem cell.

Signed-off-by: Martin Botka <martin.botka@somainline.org>
---
 .../boot/dts/allwinner/sun50i-h616-cpu-opp.dtsi    | 129 +++++++++++++++++++++
 arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi     |   4 +
 2 files changed, 133 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h616-cpu-opp.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h616-cpu-opp.dtsi
new file mode 100644
index 000000000000..4c7eaba511a9
--- /dev/null
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h616-cpu-opp.dtsi
@@ -0,0 +1,129 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+// Copyright (C) 2023 Martin Botka <martin@somainline.org>
+
+/ {
+	cpu_opp_table: cpu-opp-table {
+		compatible = "allwinner,sun50i-h616-operating-points";
+		nvmem-cells = <&cpu_speed_grade>;
+		opp-shared;
+
+		opp-480000000 {
+			opp-hz = /bits/ 64 <480000000>;
+			opp-microvolt-speed0 = <900000>;
+			opp-microvolt-speed1 = <900000>;
+			opp-microvolt-speed2 = <900000>;
+			opp-microvolt-speed3 = <900000>;
+			opp-microvolt-speed4 = <900000>;
+			clock-latency-ns = <244144>; /* 8 32k periods */
+			opp-supported-hw = <0x1f>;
+		};
+
+		opp-600000000 {
+			opp-hz = /bits/ 64 <600000000>;
+			opp-microvolt-speed1 = <900000>;
+			opp-microvolt-speed4 = <900000>;
+			clock-latency-ns = <244144>; /* 8 32k periods */
+			opp-supported-hw = <0x12>;
+		};
+
+		opp-720000000 {
+			opp-hz = /bits/ 64 <720000000>;
+			opp-microvolt-speed0 = <900000>;
+			opp-microvolt-speed2 = <900000>;
+			opp-microvolt-speed3 = <900000>;
+			clock-latency-ns = <244144>; /* 8 32k periods */
+			opp-supported-hw = <0xd>;
+		};
+
+		opp-792000000 {
+			opp-hz = /bits/ 64 <792000000>;
+			opp-microvolt-speed1 = <900000>;
+			opp-microvolt-speed4 = <940000>;
+			clock-latency-ns = <244144>; /* 8 32k periods */
+			opp-supported-hw = <0x12>;
+		};
+
+		opp-936000000 {
+			opp-hz = /bits/ 64 <936000000>;
+			opp-microvolt-speed0 = <900000>;
+			opp-microvolt-speed2 = <900000>;
+			opp-microvolt-speed3 = <900000>;
+			clock-latency-ns = <244144>; /* 8 32k periods */
+			opp-supported-hw = <0xd>;
+		};
+
+		opp-1008000000 {
+			opp-hz = /bits/ 64 <1008000000>;
+			opp-microvolt-speed0 = <950000>;
+			opp-microvolt-speed1 = <940000>;
+			opp-microvolt-speed2 = <950000>;
+			opp-microvolt-speed3 = <950000>;
+			opp-microvolt-speed4 = <1020000>;
+			clock-latency-ns = <244144>; /* 8 32k periods */
+			opp-supported-hw = <0x1f>;
+		};
+
+		opp-1104000000 {
+			opp-hz = /bits/ 64 <1104000000>;
+			opp-microvolt-speed0 = <1000000>;
+			opp-microvolt-speed2 = <1000000>;
+			opp-microvolt-speed3 = <1000000>;
+			clock-latency-ns = <244144>; /* 8 32k periods */
+			opp-supported-hw = <0xd>;
+		};
+
+		opp-1200000000 {
+			opp-hz = /bits/ 64 <1200000000>;
+			opp-microvolt-speed0 = <1050000>;
+			opp-microvolt-speed1 = <1020000>;
+			opp-microvolt-speed2 = <1050000>;
+			opp-microvolt-speed3 = <1050000>;
+			opp-microvolt-speed4 = <1100000>;
+			clock-latency-ns = <244144>; /* 8 32k periods */
+			opp-supported-hw = <0x1f>;
+		};
+
+		opp-1320000000 {
+			opp-hz = /bits/ 64 <1320000000>;
+			opp-microvolt-speed0 = <1100000>;
+			opp-microvolt-speed2 = <1100000>;
+			opp-microvolt-speed3 = <1100000>;
+			opp-microvolt-speed4 = <1100000>;
+			clock-latency-ns = <244144>; /* 8 32k periods */
+			opp-supported-hw = <0x1d>;
+		};
+
+		opp-1416000000 {
+			opp-hz = /bits/ 64 <1416000000>;
+			opp-microvolt-speed0 = <1100000>;
+			opp-microvolt-speed2 = <1100000>;
+			opp-microvolt-speed3 = <1100000>;
+			clock-latency-ns = <244144>; /* 8 32k periods */
+			opp-supported-hw = <0xd>;
+		};
+
+		opp-1512000000 {
+			opp-hz = /bits/ 64 <1512000000>;
+			opp-microvolt-speed1 = <1100000>;
+			opp-microvolt-speed3 = <1100000>;
+			clock-latency-ns = <244144>; /* 8 32k periods */
+			opp-supported-hw = <0xa>;
+		};
+	};
+};
+
+&cpu0 {
+	operating-points-v2 = <&cpu_opp_table>;
+};
+
+&cpu1 {
+	operating-points-v2 = <&cpu_opp_table>;
+};
+
+&cpu2 {
+	operating-points-v2 = <&cpu_opp_table>;
+};
+
+&cpu3 {
+	operating-points-v2 = <&cpu_opp_table>;
+};
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi
index 063db9634e5f..78e79c591dba 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi
@@ -143,6 +143,10 @@ sid: efuse@3006000 {
 			ths_calibration: thermal-sensor-calibration@14 {
 				reg = <0x14 0x8>;
 			};
+
+			cpu_speed_grade: cpu_speed_grade@0 {
+				reg = <0x0 2>;
+			};
 		};
 
 		watchdog: watchdog@30090a0 {

-- 
2.42.0

[PATCH 6/6] arm64: dts: allwinner: h616: Add cooling cells

[Martin Botka]

Add cooling cells so we enable passive cooling on CPU by regulating
CPU voltage and frequency

Signed-off-by: Martin Botka <martin.botka@somainline.org>
---
 arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi
index 78e79c591dba..7dc4c95ea280 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi
@@ -26,6 +26,7 @@ cpu0: cpu@0 {
 			reg = <0>;
 			enable-method = "psci";
 			clocks = <&ccu CLK_CPUX>;
+			#cooling-cells = <2>;
 		};
 
 		cpu1: cpu@1 {
@@ -34,6 +35,7 @@ cpu1: cpu@1 {
 			reg = <1>;
 			enable-method = "psci";
 			clocks = <&ccu CLK_CPUX>;
+			#cooling-cells = <2>;
 		};
 
 		cpu2: cpu@2 {
@@ -42,6 +44,7 @@ cpu2: cpu@2 {
 			reg = <2>;
 			enable-method = "psci";
 			clocks = <&ccu CLK_CPUX>;
+			#cooling-cells = <2>;
 		};
 
 		cpu3: cpu@3 {
@@ -50,6 +53,7 @@ cpu3: cpu@3 {
 			reg = <3>;
 			enable-method = "psci";
 			clocks = <&ccu CLK_CPUX>;
+			#cooling-cells = <2>;
 		};
 	};
 

-- 
2.42.0

Re: [PATCH 4/6] cpufreq: sun50i: Add H616 support

[Martin Botka]

On Mon, Sep 4 2023 at 05:57:04 PM +02:00:00, Martin Botka 
<martin.botka@somainline.org> wrote:
> AllWinner H616 SoC has few revisions that support different list
> of uV and frequencies.
> 
> Some revisions have the same NVMEM value and thus we have to check
> the SoC revision from SMCCC to differentiate between them.
> 
> Signed-off-by: Martin Botka <martin.botka@somainline.org>
> ---
>  drivers/cpufreq/sun50i-cpufreq-nvmem.c | 149 
> ++++++++++++++++++++++++++++-----
>  1 file changed, 126 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/cpufreq/sun50i-cpufreq-nvmem.c 
> b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> index 4321d7bbe769..19c126fb081e 100644
> --- a/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> +++ b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> @@ -10,6 +10,7 @@
> 
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> 
> +#include <linux/arm-smccc.h>
>  #include <linux/cpu.h>
>  #include <linux/module.h>
>  #include <linux/nvmem-consumer.h>
> @@ -23,20 +24,94 @@
>  #define NVMEM_MASK	0x7
>  #define NVMEM_SHIFT	5
> 
> +struct sunxi_cpufreq_soc_data {
> +	int (*efuse_xlate)(u32 *versions, u32 *efuse, char *name, size_t 
> len);
> +	u8 ver_freq_limit;
> +};
> +
>  static struct platform_device *cpufreq_dt_pdev, *sun50i_cpufreq_pdev;
> 
> +static int sun50i_h616_efuse_xlate(u32 *versions, u32 *efuse, char 
> *name, size_t len)
> +{
> +	int value = 0;
> +	u32 speedgrade = 0;
> +	u32 i;
> +	int ver_bits = arm_smccc_get_soc_id_revision();
> +
> +	if (len > 4) {
> +		pr_err("Invalid nvmem cell length\n");
> +		return -EINVAL;
> +	}
> +
> +	for (i = 0; i < len; i++)
> +		speedgrade |= (efuse[i] << (i * 8));
> +
> +	switch (speedgrade) {
> +	case 0x2000:
> +		value = 0;
> +		break;
> +	case 0x2400:
> +	case 0x7400:
> +	case 0x2c00:
> +	case 0x7c00:
> +		if (ver_bits <= 1) {
> +			/* ic version A/B */
> +			value = 1;
> +		} else {
> +			/* ic version C and later version */
> +			value = 2;
> +		}
> +		break;
> +	case 0x5000:
> +	case 0x5400:
> +	case 0x6000:
> +		value = 3;
> +		break;
> +	case 0x5c00:
> +		value = 4;
> +		break;
> +	case 0x5d00:
> +	default:
> +		value = 0;
> +	}
> +	*versions = (1 << value);
> +	snprintf(name, MAX_NAME_LEN, "speed%d", value);
> +	return 0;
> +}
> +
> +static int sun50i_h6_efuse_xlate(u32 *versions, u32 *efuse, char 
> *name, size_t len)
> +{
> +	int efuse_value = (*efuse >> NVMEM_SHIFT) & NVMEM_MASK;
> +
> +	/*
> +	 * We treat unexpected efuse values as if the SoC was from
> +	 * the slowest bin. Expected efuse values are 1-3, slowest
> +	 * to fastest.
> +	 */
> +	if (efuse_value >= 1 && efuse_value <= 3)
> +		*versions = efuse_value - 1;
> +	else
> +		*versions = 0;
> +
> +	snprintf(name, MAX_NAME_LEN, "speed%d", *versions);
> +	return 0;
> +}
> +
>  /**
>   * sun50i_cpufreq_get_efuse() - Determine speed grade from efuse 
> value
> + * @soc_data: Struct containing soc specific data & functions
>   * @versions: Set to the value parsed from efuse
> + * @name: Set to the name of speed
>   *
>   * Returns 0 if success.
>   */
> -static int sun50i_cpufreq_get_efuse(u32 *versions)
> +static int sun50i_cpufreq_get_efuse(const struct 
> sunxi_cpufreq_soc_data *soc_data,
> +				    u32 *versions, char *name)
>  {
>  	struct nvmem_cell *speedbin_nvmem;
>  	struct device_node *np;
>  	struct device *cpu_dev;
> -	u32 *speedbin, efuse_value;
> +	u32 *speedbin;
>  	size_t len;
>  	int ret;
> 
> @@ -48,9 +123,9 @@ static int sun50i_cpufreq_get_efuse(u32 *versions)
>  	if (!np)
>  		return -ENOENT;
> 
> -	ret = of_device_is_compatible(np,
> -				      "allwinner,sun50i-h6-operating-points");
> -	if (!ret) {
> +	if (of_device_is_compatible(np, 
> "allwinner,sun50i-h6-operating-points")) {
> +	} else if (of_device_is_compatible(np, 
> "allwinner,sun50i-h616-operating-points")) {
> +	} else {
>  		of_node_put(np);
>  		return -ENOENT;
>  	}
> @@ -66,17 +141,9 @@ static int sun50i_cpufreq_get_efuse(u32 *versions)
>  	if (IS_ERR(speedbin))
>  		return PTR_ERR(speedbin);
> 
> -	efuse_value = (*speedbin >> NVMEM_SHIFT) & NVMEM_MASK;
> -
> -	/*
> -	 * We treat unexpected efuse values as if the SoC was from
> -	 * the slowest bin. Expected efuse values are 1-3, slowest
> -	 * to fastest.
> -	 */
> -	if (efuse_value >= 1 && efuse_value <= 3)
> -		*versions = efuse_value - 1;
> -	else
> -		*versions = 0;
> +	ret = soc_data->efuse_xlate(versions, speedbin, name, len);
> +	if (ret)
> +		return ret;
> 
>  	kfree(speedbin);
>  	return 0;
> @@ -84,25 +151,30 @@ static int sun50i_cpufreq_get_efuse(u32 
> *versions)
> 
>  static int sun50i_cpufreq_nvmem_probe(struct platform_device *pdev)
>  {
> +	const struct of_device_id *match;
> +	const struct sunxi_cpufreq_soc_data *soc_data;
>  	int *opp_tokens;
>  	char name[MAX_NAME_LEN];
>  	unsigned int cpu;
> -	u32 speed = 0;
> +	u32 version = 0;
>  	int ret;
> 
> +	match = dev_get_platdata(&pdev->dev);
> +	if (!match)
> +		return -EINVAL;
> +	soc_data = match->data;
> +
>  	opp_tokens = kcalloc(num_possible_cpus(), sizeof(*opp_tokens),
>  			     GFP_KERNEL);
>  	if (!opp_tokens)
>  		return -ENOMEM;
> 
> -	ret = sun50i_cpufreq_get_efuse(&speed);
> +	ret = sun50i_cpufreq_get_efuse(match->data, &version, name);
>  	if (ret) {
>  		kfree(opp_tokens);
>  		return ret;
>  	}
> 
> -	snprintf(name, MAX_NAME_LEN, "speed%d", speed);
> -
>  	for_each_possible_cpu(cpu) {
>  		struct device *cpu_dev = get_cpu_device(cpu);
> 
> @@ -117,6 +189,16 @@ static int sun50i_cpufreq_nvmem_probe(struct 
> platform_device *pdev)
>  			pr_err("Failed to set prop name\n");
>  			goto free_opp;
>  		}
> +
> +		if (soc_data->ver_freq_limit) {
> +			opp_tokens[cpu] = dev_pm_opp_set_supported_hw(cpu_dev,
> +								  &version, 1);
> +			if (opp_tokens[cpu] < 0) {
> +				ret = opp_tokens[cpu];
> +				pr_err("Failed to set hw\n");
> +				goto free_opp;
> +			}
> +		}
>  	}
> 
>  	cpufreq_dt_pdev = platform_device_register_simple("cpufreq-dt", -1,
> @@ -132,6 +214,8 @@ static int sun50i_cpufreq_nvmem_probe(struct 
> platform_device *pdev)
>  free_opp:
>  	for_each_possible_cpu(cpu)
>  		dev_pm_opp_put_prop_name(opp_tokens[cpu]);
> +		if (soc_data->ver_freq_limit)
> +			dev_pm_opp_put_supported_hw(opp_tokens[cpu]);
>  	kfree(opp_tokens);
> 
>  	return ret;
> @@ -140,12 +224,21 @@ static int sun50i_cpufreq_nvmem_probe(struct 
> platform_device *pdev)
>  static int sun50i_cpufreq_nvmem_remove(struct platform_device *pdev)
>  {
>  	int *opp_tokens = platform_get_drvdata(pdev);
> +	const struct of_device_id *match;
> +	const struct sunxi_cpufreq_soc_data *soc_data;
>  	unsigned int cpu;
> 
> +	match = dev_get_platdata(&pdev->dev);
> +	if (!match)
> +		return -EINVAL;
> +	soc_data = match->data;
> +
>  	platform_device_unregister(cpufreq_dt_pdev);
> 
>  	for_each_possible_cpu(cpu)
>  		dev_pm_opp_put_prop_name(opp_tokens[cpu]);
> +		if (soc_data->ver_freq_limit)
> +			dev_pm_opp_put_supported_hw(opp_tokens[cpu]);
I completely overlooked this issue here. Clang didnt report a warning 
here. I will fix it in both cases in V2 :) Sorry.
> 
>  	kfree(opp_tokens);
> 
> @@ -160,8 +253,18 @@ static struct platform_driver 
> sun50i_cpufreq_driver = {
>  	},
>  };
> 
> +static const struct sunxi_cpufreq_soc_data sun50i_h616_data = {
> +	.efuse_xlate = sun50i_h616_efuse_xlate,
> +	.ver_freq_limit = true,
> +};
> +
> +static const struct sunxi_cpufreq_soc_data sun50i_h6_data = {
> +	.efuse_xlate = sun50i_h6_efuse_xlate,
> +};
> +
>  static const struct of_device_id sun50i_cpufreq_match_list[] = {
> -	{ .compatible = "allwinner,sun50i-h6" },
> +	{ .compatible = "allwinner,sun50i-h6", .data = &sun50i_h6_data },
> +	{ .compatible = "allwinner,sun50i-h616", .data = &sun50i_h616_data 
> },
>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, sun50i_cpufreq_match_list);
> @@ -197,8 +300,8 @@ static int __init sun50i_cpufreq_init(void)
>  		return ret;
> 
>  	sun50i_cpufreq_pdev =
> -		platform_device_register_simple("sun50i-cpufreq-nvmem",
> -						-1, NULL, 0);
> +		platform_device_register_data(NULL, "sun50i-cpufreq-nvmem",
> +						-1, match, sizeof(*match));
>  	ret = PTR_ERR_OR_ZERO(sun50i_cpufreq_pdev);
>  	if (ret == 0)
>  		return 0;
> 
> --
> 2.42.0
> 

Re: [PATCH 3/6] dt-bindings: opp: Add compatible for H616

[Krzysztof Kozlowski]

On 04/09/2023 17:57, Martin Botka wrote:
> We need to add compatible for H616 to H6 cpufreq driver bindings.

Please describe the hardware, not what is needed for drivers.

> 
> Also enable opp_supported_hw property that will be needed for H616.
> 
> Signed-off-by: Martin Botka <martin.botka@somainline.org>
> ---
>  .../bindings/opp/allwinner,sun50i-h6-operating-points.yaml          | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/opp/allwinner,sun50i-h6-operating-points.yaml b/Documentation/devicetree/bindings/opp/allwinner,sun50i-h6-operating-points.yaml
> index 51f62c3ae194..2fa1199f2d23 100644
> --- a/Documentation/devicetree/bindings/opp/allwinner,sun50i-h6-operating-points.yaml
> +++ b/Documentation/devicetree/bindings/opp/allwinner,sun50i-h6-operating-points.yaml
> @@ -23,7 +23,10 @@ allOf:
>  
>  properties:
>    compatible:
> -    const: allwinner,sun50i-h6-operating-points
> +    contains:

This does not look like part of allOf, so contains is no correct here.
This must be specific, so drop contains.

> +      enum:
> +        - allwinner,sun50i-h6-operating-points
> +        - allwinner,sun50i-h616-operating-points
>  
>    nvmem-cells:
>      description: |
> @@ -47,6 +50,7 @@ patternProperties:
>      properties:
>        opp-hz: true
>        clock-latency-ns: true
> +      opp-supported-hw: true

Why? It is already allowed. You should rather explain the values.

> 

Best regards,
Krzysztof

Re: [PATCH 3/6] dt-bindings: opp: Add compatible for H616

[Krzysztof Kozlowski]

On 04/09/2023 21:31, Krzysztof Kozlowski wrote:
> On 04/09/2023 17:57, Martin Botka wrote:
>> We need to add compatible for H616 to H6 cpufreq driver bindings.
> 
> Please describe the hardware, not what is needed for drivers.
> 
>>
>> Also enable opp_supported_hw property that will be needed for H616.
>>
>> Signed-off-by: Martin Botka <martin.botka@somainline.org>
>> ---
>>  .../bindings/opp/allwinner,sun50i-h6-operating-points.yaml          | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/opp/allwinner,sun50i-h6-operating-points.yaml b/Documentation/devicetree/bindings/opp/allwinner,sun50i-h6-operating-points.yaml
>> index 51f62c3ae194..2fa1199f2d23 100644
>> --- a/Documentation/devicetree/bindings/opp/allwinner,sun50i-h6-operating-points.yaml
>> +++ b/Documentation/devicetree/bindings/opp/allwinner,sun50i-h6-operating-points.yaml
>> @@ -23,7 +23,10 @@ allOf:
>>  
>>  properties:
>>    compatible:
>> -    const: allwinner,sun50i-h6-operating-points
>> +    contains:
> 
> This does not look like part of allOf, so contains is no correct here.
> This must be specific, so drop contains.

BTW, I also do no see it used by the driver at all.

Best regards,
Krzysztof

Re: [PATCH 5/6] arm64: dts: allwinner: h616: Add CPU Operating Performance Points table

[Krzysztof Kozlowski]

On 04/09/2023 17:57, Martin Botka wrote:
> Add an Operating Performance Points table for the CPU cores to
> enable Dynamic Voltage & Frequency Scaling on the H616.
> 

...

> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi
> index 063db9634e5f..78e79c591dba 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi
> @@ -143,6 +143,10 @@ sid: efuse@3006000 {
>  			ths_calibration: thermal-sensor-calibration@14 {
>  				reg = <0x14 0x8>;
>  			};
> +
> +			cpu_speed_grade: cpu_speed_grade@0 {

Underscores are no allowed in node names.

It does not look like you tested the DTS against bindings. Please run
`make dtbs_check W=1` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).

Best regards,
Krzysztof

Re: [PATCH 5/6] arm64: dts: allwinner: h616: Add CPU Operating Performance Points table

[Martin Botka]

On Mon, Sep 4 2023 at 09:33:33 PM +02:00:00, Krzysztof Kozlowski 
<krzysztof.kozlowski@linaro.org> wrote:
> On 04/09/2023 17:57, Martin Botka wrote:
>>  Add an Operating Performance Points table for the CPU cores to
>>  enable Dynamic Voltage & Frequency Scaling on the H616.
>> 
> 
> ...
> 
>>  diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi 
>> b/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi
>>  index 063db9634e5f..78e79c591dba 100644
>>  --- a/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi
>>  +++ b/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi
>>  @@ -143,6 +143,10 @@ sid: efuse@3006000 {
>>   			ths_calibration: thermal-sensor-calibration@14 {
>>   				reg = <0x14 0x8>;
>>   			};
>>  +
>>  +			cpu_speed_grade: cpu_speed_grade@0 {
> 
> Underscores are no allowed in node names.
> 
> It does not look like you tested the DTS against bindings. Please run
> `make dtbs_check W=1` (see
> Documentation/devicetree/bindings/writing-schema.rst or
> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
> for instructions).
I actually did. And did rerun it just now to check. No error or warning 
was reported by dtbs_check W=1 for this.
But I will correct it in V2 and look if my setup is doing something 
wrong that its not reporting these issues.
Cheers,
Martin
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 3/6] dt-bindings: opp: Add compatible for H616

[Martin Botka]

On Mon, Sep 4 2023 at 09:32:44 PM +02:00:00, Krzysztof Kozlowski 
<krzysztof.kozlowski@linaro.org> wrote:
> On 04/09/2023 21:31, Krzysztof Kozlowski wrote:
>>  On 04/09/2023 17:57, Martin Botka wrote:
>>>  We need to add compatible for H616 to H6 cpufreq driver bindings.
>> 
>>  Please describe the hardware, not what is needed for drivers.
>> 
>>> 
>>>  Also enable opp_supported_hw property that will be needed for H616.
>>> 
>>>  Signed-off-by: Martin Botka <martin.botka@somainline.org>
>>>  ---
>>>   .../bindings/opp/allwinner,sun50i-h6-operating-points.yaml        
>>>   | 6 +++++-
>>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>> 
>>>  diff --git 
>>> a/Documentation/devicetree/bindings/opp/allwinner,sun50i-h6-operating-points.yaml 
>>> b/Documentation/devicetree/bindings/opp/allwinner,sun50i-h6-operating-points.yaml
>>>  index 51f62c3ae194..2fa1199f2d23 100644
>>>  --- 
>>> a/Documentation/devicetree/bindings/opp/allwinner,sun50i-h6-operating-points.yaml
>>>  +++ 
>>> b/Documentation/devicetree/bindings/opp/allwinner,sun50i-h6-operating-points.yaml
>>>  @@ -23,7 +23,10 @@ allOf:
>>> 
>>>   properties:
>>>     compatible:
>>>  -    const: allwinner,sun50i-h6-operating-points
>>>  +    contains:
>> 
>>  This does not look like part of allOf, so contains is no correct 
>> here.
>>  This must be specific, so drop contains.
> 
> BTW, I also do no see it used by the driver at all.
Function sun50i_cpufreq_get_efuse uses it. It checks for H6 compatible 
and if that fails we check for H616 compatible.
Also im double checking for H6. Nice screwup on my side :)

Cheers,
Martin
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 3/6] dt-bindings: opp: Add compatible for H616

[Martin Botka]

On Mon, Sep 4 2023 at 09:31:34 PM +02:00:00, Krzysztof Kozlowski 
<krzysztof.kozlowski@linaro.org> wrote:
> On 04/09/2023 17:57, Martin Botka wrote:
>>  We need to add compatible for H616 to H6 cpufreq driver bindings.
> 
> Please describe the hardware, not what is needed for drivers.
Got it. Sorry.
> 
>> 
>>  Also enable opp_supported_hw property that will be needed for H616.
>> 
>>  Signed-off-by: Martin Botka <martin.botka@somainline.org>
>>  ---
>>   .../bindings/opp/allwinner,sun50i-h6-operating-points.yaml         
>>  | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>> 
>>  diff --git 
>> a/Documentation/devicetree/bindings/opp/allwinner,sun50i-h6-operating-points.yaml 
>> b/Documentation/devicetree/bindings/opp/allwinner,sun50i-h6-operating-points.yaml
>>  index 51f62c3ae194..2fa1199f2d23 100644
>>  --- 
>> a/Documentation/devicetree/bindings/opp/allwinner,sun50i-h6-operating-points.yaml
>>  +++ 
>> b/Documentation/devicetree/bindings/opp/allwinner,sun50i-h6-operating-points.yaml
>>  @@ -23,7 +23,10 @@ allOf:
>> 
>>   properties:
>>     compatible:
>>  -    const: allwinner,sun50i-h6-operating-points
>>  +    contains:
> 
> This does not look like part of allOf, so contains is no correct here.
> This must be specific, so drop contains.
ack.
> 
>>  +      enum:
>>  +        - allwinner,sun50i-h6-operating-points
>>  +        - allwinner,sun50i-h616-operating-points
>> 
>>     nvmem-cells:
>>       description: |
>>  @@ -47,6 +50,7 @@ patternProperties:
>>       properties:
>>         opp-hz: true
>>         clock-latency-ns: true
>>  +      opp-supported-hw: true
> 
> Why? It is already allowed. You should rather explain the values.
Yea this can be dropped. I forgot to remove it. My bad.
Also the values i think are very clear ? The values converted to binary 
represent which chip revision is allowed to use the specified frequency.
1 bit for each revision.
> 
>> 
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 3/6] dt-bindings: opp: Add compatible for H616

[Krzysztof Kozlowski]

On 04/09/2023 21:48, Martin Botka wrote:
> 
> 
> On Mon, Sep 4 2023 at 09:32:44 PM +02:00:00, Krzysztof Kozlowski 
> <krzysztof.kozlowski@linaro.org> wrote:
>> On 04/09/2023 21:31, Krzysztof Kozlowski wrote:
>>>  On 04/09/2023 17:57, Martin Botka wrote:
>>>>  We need to add compatible for H616 to H6 cpufreq driver bindings.
>>>
>>>  Please describe the hardware, not what is needed for drivers.
>>>
>>>>
>>>>  Also enable opp_supported_hw property that will be needed for H616.
>>>>
>>>>  Signed-off-by: Martin Botka <martin.botka@somainline.org>
>>>>  ---
>>>>   .../bindings/opp/allwinner,sun50i-h6-operating-points.yaml        
>>>>   | 6 +++++-
>>>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>>  diff --git 
>>>> a/Documentation/devicetree/bindings/opp/allwinner,sun50i-h6-operating-points.yaml 
>>>> b/Documentation/devicetree/bindings/opp/allwinner,sun50i-h6-operating-points.yaml
>>>>  index 51f62c3ae194..2fa1199f2d23 100644
>>>>  --- 
>>>> a/Documentation/devicetree/bindings/opp/allwinner,sun50i-h6-operating-points.yaml
>>>>  +++ 
>>>> b/Documentation/devicetree/bindings/opp/allwinner,sun50i-h6-operating-points.yaml
>>>>  @@ -23,7 +23,10 @@ allOf:
>>>>
>>>>   properties:
>>>>     compatible:
>>>>  -    const: allwinner,sun50i-h6-operating-points
>>>>  +    contains:
>>>
>>>  This does not look like part of allOf, so contains is no correct 
>>> here.
>>>  This must be specific, so drop contains.
>>
>> BTW, I also do no see it used by the driver at all.
> Function sun50i_cpufreq_get_efuse uses it. It checks for H6 compatible 
> and if that fails we check for H616 compatible.

Such code does no scale. It also does not look reasonable - you cannot
have different compatible there. Device binds to h6 or h616, so you
cannot have OPP table from other devices.

Best regards,
Krzysztof

Re: [PATCH 3/6] dt-bindings: opp: Add compatible for H616

[Martin Botka]

On Mon, Sep 4 2023 at 09:53:05 PM +02:00:00, Krzysztof Kozlowski 
<krzysztof.kozlowski@linaro.org> wrote:
> On 04/09/2023 21:48, Martin Botka wrote:
>> 
>> 
>>  On Mon, Sep 4 2023 at 09:32:44 PM +02:00:00, Krzysztof Kozlowski
>>  <krzysztof.kozlowski@linaro.org> wrote:
>>>  On 04/09/2023 21:31, Krzysztof Kozlowski wrote:
>>>>   On 04/09/2023 17:57, Martin Botka wrote:
>>>>>   We need to add compatible for H616 to H6 cpufreq driver 
>>>>> bindings.
>>>> 
>>>>   Please describe the hardware, not what is needed for drivers.
>>>> 
>>>>> 
>>>>>   Also enable opp_supported_hw property that will be needed for 
>>>>> H616.
>>>>> 
>>>>>   Signed-off-by: Martin Botka <martin.botka@somainline.org>
>>>>>   ---
>>>>>    .../bindings/opp/allwinner,sun50i-h6-operating-points.yaml
>>>>>    | 6 +++++-
>>>>>    1 file changed, 5 insertions(+), 1 deletion(-)
>>>>> 
>>>>>   diff --git
>>>>>  
>>>>> a/Documentation/devicetree/bindings/opp/allwinner,sun50i-h6-operating-points.yaml
>>>>>  
>>>>> b/Documentation/devicetree/bindings/opp/allwinner,sun50i-h6-operating-points.yaml
>>>>>   index 51f62c3ae194..2fa1199f2d23 100644
>>>>>   ---
>>>>>  
>>>>> a/Documentation/devicetree/bindings/opp/allwinner,sun50i-h6-operating-points.yaml
>>>>>   +++
>>>>>  
>>>>> b/Documentation/devicetree/bindings/opp/allwinner,sun50i-h6-operating-points.yaml
>>>>>   @@ -23,7 +23,10 @@ allOf:
>>>>> 
>>>>>    properties:
>>>>>      compatible:
>>>>>   -    const: allwinner,sun50i-h6-operating-points
>>>>>   +    contains:
>>>> 
>>>>   This does not look like part of allOf, so contains is no correct
>>>>  here.
>>>>   This must be specific, so drop contains.
>>> 
>>>  BTW, I also do no see it used by the driver at all.
>>  Function sun50i_cpufreq_get_efuse uses it. It checks for H6 
>> compatible
>>  and if that fails we check for H616 compatible.
> 
> Such code does no scale. It also does not look reasonable - you cannot
> have different compatible there. Device binds to h6 or h616, so you
> cannot have OPP table from other devices.
> 
Heya. I checked how qcom nvmem driver does it. And yea this indeed does 
not scale. matchlist should have SoC compatible and driver needs to 
have single compatible. Thus also dropping this patch :)

Will do in V2. Thanks Krzystof for pointing me to the right way of 
doing it :)

Cheers,
Martin
> Best regards,
> Krzysztof
> 

Re: [PATCH 2/6] cpufreq: dt-platdev: Blocklist allwinner,h616 SoC

[Andre Przywara]

On Mon, 04 Sep 2023 17:57:02 +0200
Martin Botka <martin.botka@somainline.org> wrote:

> The AllWinner H616 uses H6 cpufreq driver.
> Add it to blocklist so its not created twice

That looks alright, but I think needs to be squashed into the patch
that enables the H616 driver operation, to avoid regressions during
bisecting.

Cheers,
Andre

> 
> Signed-off-by: Martin Botka <martin.botka@somainline.org>
> ---
>  drivers/cpufreq/cpufreq-dt-platdev.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c b/drivers/cpufreq/cpufreq-dt-platdev.c
> index e2b20080de3a..51818cef8979 100644
> --- a/drivers/cpufreq/cpufreq-dt-platdev.c
> +++ b/drivers/cpufreq/cpufreq-dt-platdev.c
> @@ -104,6 +104,7 @@ static const struct of_device_id allowlist[] __initconst = {
>   */
>  static const struct of_device_id blocklist[] __initconst = {
>  	{ .compatible = "allwinner,sun50i-h6", },
> +	{ .compatible = "allwinner,sun50i-h616", },
>  
>  	{ .compatible = "apple,arm-platform", },
>  
> 

Re: [PATCH 4/6] cpufreq: sun50i: Add H616 support

[Andre Przywara]

On Mon, 04 Sep 2023 17:57:04 +0200
Martin Botka <martin.botka@somainline.org> wrote:

Hi,

> AllWinner H616 SoC has few revisions that support different list
> of uV and frequencies.
> 
> Some revisions have the same NVMEM value and thus we have to check
> the SoC revision from SMCCC to differentiate between them.

So this patch is a bit hard to read, as it combines two things: the
refactoring and the actual H616 bits. Can you please split this up,
with a first patch just introducing struct sunxi_cpufreq_soc_data and
moving the existing code into the separate xlate function, and all the
other required changes? Then having a second patch adding the H616
bits on top? This makes review easier, as the first patch should not
change any behaviour, and the second patch just focuses on the new H616
bits.

Cheers,
Andre

> 
> Signed-off-by: Martin Botka <martin.botka@somainline.org>
> ---
>  drivers/cpufreq/sun50i-cpufreq-nvmem.c | 149 ++++++++++++++++++++++++++++-----
>  1 file changed, 126 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/cpufreq/sun50i-cpufreq-nvmem.c b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> index 4321d7bbe769..19c126fb081e 100644
> --- a/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> +++ b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> @@ -10,6 +10,7 @@
>  
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>  
> +#include <linux/arm-smccc.h>
>  #include <linux/cpu.h>
>  #include <linux/module.h>
>  #include <linux/nvmem-consumer.h>
> @@ -23,20 +24,94 @@
>  #define NVMEM_MASK	0x7
>  #define NVMEM_SHIFT	5
>  
> +struct sunxi_cpufreq_soc_data {
> +	int (*efuse_xlate)(u32 *versions, u32 *efuse, char *name, size_t len);
> +	u8 ver_freq_limit;
> +};
> +
>  static struct platform_device *cpufreq_dt_pdev, *sun50i_cpufreq_pdev;
>  
> +static int sun50i_h616_efuse_xlate(u32 *versions, u32 *efuse, char *name, size_t len)
> +{
> +	int value = 0;
> +	u32 speedgrade = 0;
> +	u32 i;
> +	int ver_bits = arm_smccc_get_soc_id_revision();
> +
> +	if (len > 4) {
> +		pr_err("Invalid nvmem cell length\n");
> +		return -EINVAL;
> +	}
> +
> +	for (i = 0; i < len; i++)
> +		speedgrade |= (efuse[i] << (i * 8));
> +
> +	switch (speedgrade) {
> +	case 0x2000:
> +		value = 0;
> +		break;
> +	case 0x2400:
> +	case 0x7400:
> +	case 0x2c00:
> +	case 0x7c00:
> +		if (ver_bits <= 1) {
> +			/* ic version A/B */
> +			value = 1;
> +		} else {
> +			/* ic version C and later version */
> +			value = 2;
> +		}
> +		break;
> +	case 0x5000:
> +	case 0x5400:
> +	case 0x6000:
> +		value = 3;
> +		break;
> +	case 0x5c00:
> +		value = 4;
> +		break;
> +	case 0x5d00:
> +	default:
> +		value = 0;
> +	}
> +	*versions = (1 << value);
> +	snprintf(name, MAX_NAME_LEN, "speed%d", value);
> +	return 0;
> +}
> +
> +static int sun50i_h6_efuse_xlate(u32 *versions, u32 *efuse, char *name, size_t len)
> +{
> +	int efuse_value = (*efuse >> NVMEM_SHIFT) & NVMEM_MASK;
> +
> +	/*
> +	 * We treat unexpected efuse values as if the SoC was from
> +	 * the slowest bin. Expected efuse values are 1-3, slowest
> +	 * to fastest.
> +	 */
> +	if (efuse_value >= 1 && efuse_value <= 3)
> +		*versions = efuse_value - 1;
> +	else
> +		*versions = 0;
> +
> +	snprintf(name, MAX_NAME_LEN, "speed%d", *versions);
> +	return 0;
> +}
> +
>  /**
>   * sun50i_cpufreq_get_efuse() - Determine speed grade from efuse value
> + * @soc_data: Struct containing soc specific data & functions
>   * @versions: Set to the value parsed from efuse
> + * @name: Set to the name of speed
>   *
>   * Returns 0 if success.
>   */
> -static int sun50i_cpufreq_get_efuse(u32 *versions)
> +static int sun50i_cpufreq_get_efuse(const struct sunxi_cpufreq_soc_data *soc_data,
> +				    u32 *versions, char *name)
>  {
>  	struct nvmem_cell *speedbin_nvmem;
>  	struct device_node *np;
>  	struct device *cpu_dev;
> -	u32 *speedbin, efuse_value;
> +	u32 *speedbin;
>  	size_t len;
>  	int ret;
>  
> @@ -48,9 +123,9 @@ static int sun50i_cpufreq_get_efuse(u32 *versions)
>  	if (!np)
>  		return -ENOENT;
>  
> -	ret = of_device_is_compatible(np,
> -				      "allwinner,sun50i-h6-operating-points");
> -	if (!ret) {
> +	if (of_device_is_compatible(np, "allwinner,sun50i-h6-operating-points")) {
> +	} else if (of_device_is_compatible(np, "allwinner,sun50i-h616-operating-points")) {
> +	} else {
>  		of_node_put(np);
>  		return -ENOENT;
>  	}
> @@ -66,17 +141,9 @@ static int sun50i_cpufreq_get_efuse(u32 *versions)
>  	if (IS_ERR(speedbin))
>  		return PTR_ERR(speedbin);
>  
> -	efuse_value = (*speedbin >> NVMEM_SHIFT) & NVMEM_MASK;
> -
> -	/*
> -	 * We treat unexpected efuse values as if the SoC was from
> -	 * the slowest bin. Expected efuse values are 1-3, slowest
> -	 * to fastest.
> -	 */
> -	if (efuse_value >= 1 && efuse_value <= 3)
> -		*versions = efuse_value - 1;
> -	else
> -		*versions = 0;
> +	ret = soc_data->efuse_xlate(versions, speedbin, name, len);
> +	if (ret)
> +		return ret;
>  
>  	kfree(speedbin);
>  	return 0;
> @@ -84,25 +151,30 @@ static int sun50i_cpufreq_get_efuse(u32 *versions)
>  
>  static int sun50i_cpufreq_nvmem_probe(struct platform_device *pdev)
>  {
> +	const struct of_device_id *match;
> +	const struct sunxi_cpufreq_soc_data *soc_data;
>  	int *opp_tokens;
>  	char name[MAX_NAME_LEN];
>  	unsigned int cpu;
> -	u32 speed = 0;
> +	u32 version = 0;
>  	int ret;
>  
> +	match = dev_get_platdata(&pdev->dev);
> +	if (!match)
> +		return -EINVAL;
> +	soc_data = match->data;
> +
>  	opp_tokens = kcalloc(num_possible_cpus(), sizeof(*opp_tokens),
>  			     GFP_KERNEL);
>  	if (!opp_tokens)
>  		return -ENOMEM;
>  
> -	ret = sun50i_cpufreq_get_efuse(&speed);
> +	ret = sun50i_cpufreq_get_efuse(match->data, &version, name);
>  	if (ret) {
>  		kfree(opp_tokens);
>  		return ret;
>  	}
>  
> -	snprintf(name, MAX_NAME_LEN, "speed%d", speed);
> -
>  	for_each_possible_cpu(cpu) {
>  		struct device *cpu_dev = get_cpu_device(cpu);
>  
> @@ -117,6 +189,16 @@ static int sun50i_cpufreq_nvmem_probe(struct platform_device *pdev)
>  			pr_err("Failed to set prop name\n");
>  			goto free_opp;
>  		}
> +
> +		if (soc_data->ver_freq_limit) {
> +			opp_tokens[cpu] = dev_pm_opp_set_supported_hw(cpu_dev,
> +								  &version, 1);
> +			if (opp_tokens[cpu] < 0) {
> +				ret = opp_tokens[cpu];
> +				pr_err("Failed to set hw\n");
> +				goto free_opp;
> +			}
> +		}
>  	}
>  
>  	cpufreq_dt_pdev = platform_device_register_simple("cpufreq-dt", -1,
> @@ -132,6 +214,8 @@ static int sun50i_cpufreq_nvmem_probe(struct platform_device *pdev)
>  free_opp:
>  	for_each_possible_cpu(cpu)
>  		dev_pm_opp_put_prop_name(opp_tokens[cpu]);
> +		if (soc_data->ver_freq_limit)
> +			dev_pm_opp_put_supported_hw(opp_tokens[cpu]);
>  	kfree(opp_tokens);
>  
>  	return ret;
> @@ -140,12 +224,21 @@ static int sun50i_cpufreq_nvmem_probe(struct platform_device *pdev)
>  static int sun50i_cpufreq_nvmem_remove(struct platform_device *pdev)
>  {
>  	int *opp_tokens = platform_get_drvdata(pdev);
> +	const struct of_device_id *match;
> +	const struct sunxi_cpufreq_soc_data *soc_data;
>  	unsigned int cpu;
>  
> +	match = dev_get_platdata(&pdev->dev);
> +	if (!match)
> +		return -EINVAL;
> +	soc_data = match->data;
> +
>  	platform_device_unregister(cpufreq_dt_pdev);
>  
>  	for_each_possible_cpu(cpu)
>  		dev_pm_opp_put_prop_name(opp_tokens[cpu]);
> +		if (soc_data->ver_freq_limit)
> +			dev_pm_opp_put_supported_hw(opp_tokens[cpu]);
>  
>  	kfree(opp_tokens);
>  
> @@ -160,8 +253,18 @@ static struct platform_driver sun50i_cpufreq_driver = {
>  	},
>  };
>  
> +static const struct sunxi_cpufreq_soc_data sun50i_h616_data = {
> +	.efuse_xlate = sun50i_h616_efuse_xlate,
> +	.ver_freq_limit = true,
> +};
> +
> +static const struct sunxi_cpufreq_soc_data sun50i_h6_data = {
> +	.efuse_xlate = sun50i_h6_efuse_xlate,
> +};
> +
>  static const struct of_device_id sun50i_cpufreq_match_list[] = {
> -	{ .compatible = "allwinner,sun50i-h6" },
> +	{ .compatible = "allwinner,sun50i-h6", .data = &sun50i_h6_data },
> +	{ .compatible = "allwinner,sun50i-h616", .data = &sun50i_h616_data },
>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, sun50i_cpufreq_match_list);
> @@ -197,8 +300,8 @@ static int __init sun50i_cpufreq_init(void)
>  		return ret;
>  
>  	sun50i_cpufreq_pdev =
> -		platform_device_register_simple("sun50i-cpufreq-nvmem",
> -						-1, NULL, 0);
> +		platform_device_register_data(NULL, "sun50i-cpufreq-nvmem",
> +						-1, match, sizeof(*match));
>  	ret = PTR_ERR_OR_ZERO(sun50i_cpufreq_pdev);
>  	if (ret == 0)
>  		return 0;
> 

Re: [PATCH 4/6] cpufreq: sun50i: Add H616 support

[Martin Botka]

On Mon, Sep 4 2023 at 09:41:08 PM +01:00:00, Andre Przywara 
<andre.przywara@arm.com> wrote:
> On Mon, 04 Sep 2023 17:57:04 +0200
> Martin Botka <martin.botka@somainline.org> wrote:
> 
> Hi,
> 
>>  AllWinner H616 SoC has few revisions that support different list
>>  of uV and frequencies.
>> 
>>  Some revisions have the same NVMEM value and thus we have to check
>>  the SoC revision from SMCCC to differentiate between them.
> 
> So this patch is a bit hard to read, as it combines two things: the
> refactoring and the actual H616 bits. Can you please split this up,
> with a first patch just introducing struct sunxi_cpufreq_soc_data and
> moving the existing code into the separate xlate function, and all the
> other required changes? Then having a second patch adding the H616
> bits on top? This makes review easier, as the first patch should not
> change any behaviour, and the second patch just focuses on the new 
> H616
> bits.
Yea it is. I will split them up in V2.
> 
> Cheers,
> Andre
> 
>> 
>>  Signed-off-by: Martin Botka <martin.botka@somainline.org>
>>  ---
>>   drivers/cpufreq/sun50i-cpufreq-nvmem.c | 149 
>> ++++++++++++++++++++++++++++-----
>>   1 file changed, 126 insertions(+), 23 deletions(-)
>> 
>>  diff --git a/drivers/cpufreq/sun50i-cpufreq-nvmem.c 
>> b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
>>  index 4321d7bbe769..19c126fb081e 100644
>>  --- a/drivers/cpufreq/sun50i-cpufreq-nvmem.c
>>  +++ b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
>>  @@ -10,6 +10,7 @@
>> 
>>   #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> 
>>  +#include <linux/arm-smccc.h>
>>   #include <linux/cpu.h>
>>   #include <linux/module.h>
>>   #include <linux/nvmem-consumer.h>
>>  @@ -23,20 +24,94 @@
>>   #define NVMEM_MASK	0x7
>>   #define NVMEM_SHIFT	5
>> 
>>  +struct sunxi_cpufreq_soc_data {
>>  +	int (*efuse_xlate)(u32 *versions, u32 *efuse, char *name, size_t 
>> len);
>>  +	u8 ver_freq_limit;
>>  +};
>>  +
>>   static struct platform_device *cpufreq_dt_pdev, 
>> *sun50i_cpufreq_pdev;
>> 
>>  +static int sun50i_h616_efuse_xlate(u32 *versions, u32 *efuse, char 
>> *name, size_t len)
>>  +{
>>  +	int value = 0;
>>  +	u32 speedgrade = 0;
>>  +	u32 i;
>>  +	int ver_bits = arm_smccc_get_soc_id_revision();
>>  +
>>  +	if (len > 4) {
>>  +		pr_err("Invalid nvmem cell length\n");
>>  +		return -EINVAL;
>>  +	}
>>  +
>>  +	for (i = 0; i < len; i++)
>>  +		speedgrade |= (efuse[i] << (i * 8));
>>  +
>>  +	switch (speedgrade) {
>>  +	case 0x2000:
>>  +		value = 0;
>>  +		break;
>>  +	case 0x2400:
>>  +	case 0x7400:
>>  +	case 0x2c00:
>>  +	case 0x7c00:
>>  +		if (ver_bits <= 1) {
>>  +			/* ic version A/B */
>>  +			value = 1;
>>  +		} else {
>>  +			/* ic version C and later version */
>>  +			value = 2;
>>  +		}
>>  +		break;
>>  +	case 0x5000:
>>  +	case 0x5400:
>>  +	case 0x6000:
>>  +		value = 3;
>>  +		break;
>>  +	case 0x5c00:
>>  +		value = 4;
>>  +		break;
>>  +	case 0x5d00:
>>  +	default:
>>  +		value = 0;
>>  +	}
>>  +	*versions = (1 << value);
>>  +	snprintf(name, MAX_NAME_LEN, "speed%d", value);
>>  +	return 0;
>>  +}
>>  +
>>  +static int sun50i_h6_efuse_xlate(u32 *versions, u32 *efuse, char 
>> *name, size_t len)
>>  +{
>>  +	int efuse_value = (*efuse >> NVMEM_SHIFT) & NVMEM_MASK;
>>  +
>>  +	/*
>>  +	 * We treat unexpected efuse values as if the SoC was from
>>  +	 * the slowest bin. Expected efuse values are 1-3, slowest
>>  +	 * to fastest.
>>  +	 */
>>  +	if (efuse_value >= 1 && efuse_value <= 3)
>>  +		*versions = efuse_value - 1;
>>  +	else
>>  +		*versions = 0;
>>  +
>>  +	snprintf(name, MAX_NAME_LEN, "speed%d", *versions);
>>  +	return 0;
>>  +}
>>  +
>>   /**
>>    * sun50i_cpufreq_get_efuse() - Determine speed grade from efuse 
>> value
>>  + * @soc_data: Struct containing soc specific data & functions
>>    * @versions: Set to the value parsed from efuse
>>  + * @name: Set to the name of speed
>>    *
>>    * Returns 0 if success.
>>    */
>>  -static int sun50i_cpufreq_get_efuse(u32 *versions)
>>  +static int sun50i_cpufreq_get_efuse(const struct 
>> sunxi_cpufreq_soc_data *soc_data,
>>  +				    u32 *versions, char *name)
>>   {
>>   	struct nvmem_cell *speedbin_nvmem;
>>   	struct device_node *np;
>>   	struct device *cpu_dev;
>>  -	u32 *speedbin, efuse_value;
>>  +	u32 *speedbin;
>>   	size_t len;
>>   	int ret;
>> 
>>  @@ -48,9 +123,9 @@ static int sun50i_cpufreq_get_efuse(u32 
>> *versions)
>>   	if (!np)
>>   		return -ENOENT;
>> 
>>  -	ret = of_device_is_compatible(np,
>>  -				      "allwinner,sun50i-h6-operating-points");
>>  -	if (!ret) {
>>  +	if (of_device_is_compatible(np, 
>> "allwinner,sun50i-h6-operating-points")) {
>>  +	} else if (of_device_is_compatible(np, 
>> "allwinner,sun50i-h616-operating-points")) {
>>  +	} else {
>>   		of_node_put(np);
>>   		return -ENOENT;
>>   	}
>>  @@ -66,17 +141,9 @@ static int sun50i_cpufreq_get_efuse(u32 
>> *versions)
>>   	if (IS_ERR(speedbin))
>>   		return PTR_ERR(speedbin);
>> 
>>  -	efuse_value = (*speedbin >> NVMEM_SHIFT) & NVMEM_MASK;
>>  -
>>  -	/*
>>  -	 * We treat unexpected efuse values as if the SoC was from
>>  -	 * the slowest bin. Expected efuse values are 1-3, slowest
>>  -	 * to fastest.
>>  -	 */
>>  -	if (efuse_value >= 1 && efuse_value <= 3)
>>  -		*versions = efuse_value - 1;
>>  -	else
>>  -		*versions = 0;
>>  +	ret = soc_data->efuse_xlate(versions, speedbin, name, len);
>>  +	if (ret)
>>  +		return ret;
>> 
>>   	kfree(speedbin);
>>   	return 0;
>>  @@ -84,25 +151,30 @@ static int sun50i_cpufreq_get_efuse(u32 
>> *versions)
>> 
>>   static int sun50i_cpufreq_nvmem_probe(struct platform_device *pdev)
>>   {
>>  +	const struct of_device_id *match;
>>  +	const struct sunxi_cpufreq_soc_data *soc_data;
>>   	int *opp_tokens;
>>   	char name[MAX_NAME_LEN];
>>   	unsigned int cpu;
>>  -	u32 speed = 0;
>>  +	u32 version = 0;
>>   	int ret;
>> 
>>  +	match = dev_get_platdata(&pdev->dev);
>>  +	if (!match)
>>  +		return -EINVAL;
>>  +	soc_data = match->data;
>>  +
>>   	opp_tokens = kcalloc(num_possible_cpus(), sizeof(*opp_tokens),
>>   			     GFP_KERNEL);
>>   	if (!opp_tokens)
>>   		return -ENOMEM;
>> 
>>  -	ret = sun50i_cpufreq_get_efuse(&speed);
>>  +	ret = sun50i_cpufreq_get_efuse(match->data, &version, name);
>>   	if (ret) {
>>   		kfree(opp_tokens);
>>   		return ret;
>>   	}
>> 
>>  -	snprintf(name, MAX_NAME_LEN, "speed%d", speed);
>>  -
>>   	for_each_possible_cpu(cpu) {
>>   		struct device *cpu_dev = get_cpu_device(cpu);
>> 
>>  @@ -117,6 +189,16 @@ static int sun50i_cpufreq_nvmem_probe(struct 
>> platform_device *pdev)
>>   			pr_err("Failed to set prop name\n");
>>   			goto free_opp;
>>   		}
>>  +
>>  +		if (soc_data->ver_freq_limit) {
>>  +			opp_tokens[cpu] = dev_pm_opp_set_supported_hw(cpu_dev,
>>  +								  &version, 1);
>>  +			if (opp_tokens[cpu] < 0) {
>>  +				ret = opp_tokens[cpu];
>>  +				pr_err("Failed to set hw\n");
>>  +				goto free_opp;
>>  +			}
>>  +		}
>>   	}
>> 
>>   	cpufreq_dt_pdev = platform_device_register_simple("cpufreq-dt", 
>> -1,
>>  @@ -132,6 +214,8 @@ static int sun50i_cpufreq_nvmem_probe(struct 
>> platform_device *pdev)
>>   free_opp:
>>   	for_each_possible_cpu(cpu)
>>   		dev_pm_opp_put_prop_name(opp_tokens[cpu]);
>>  +		if (soc_data->ver_freq_limit)
>>  +			dev_pm_opp_put_supported_hw(opp_tokens[cpu]);
>>   	kfree(opp_tokens);
>> 
>>   	return ret;
>>  @@ -140,12 +224,21 @@ static int sun50i_cpufreq_nvmem_probe(struct 
>> platform_device *pdev)
>>   static int sun50i_cpufreq_nvmem_remove(struct platform_device 
>> *pdev)
>>   {
>>   	int *opp_tokens = platform_get_drvdata(pdev);
>>  +	const struct of_device_id *match;
>>  +	const struct sunxi_cpufreq_soc_data *soc_data;
>>   	unsigned int cpu;
>> 
>>  +	match = dev_get_platdata(&pdev->dev);
>>  +	if (!match)
>>  +		return -EINVAL;
>>  +	soc_data = match->data;
>>  +
>>   	platform_device_unregister(cpufreq_dt_pdev);
>> 
>>   	for_each_possible_cpu(cpu)
>>   		dev_pm_opp_put_prop_name(opp_tokens[cpu]);
>>  +		if (soc_data->ver_freq_limit)
>>  +			dev_pm_opp_put_supported_hw(opp_tokens[cpu]);
>> 
>>   	kfree(opp_tokens);
>> 
>>  @@ -160,8 +253,18 @@ static struct platform_driver 
>> sun50i_cpufreq_driver = {
>>   	},
>>   };
>> 
>>  +static const struct sunxi_cpufreq_soc_data sun50i_h616_data = {
>>  +	.efuse_xlate = sun50i_h616_efuse_xlate,
>>  +	.ver_freq_limit = true,
>>  +};
>>  +
>>  +static const struct sunxi_cpufreq_soc_data sun50i_h6_data = {
>>  +	.efuse_xlate = sun50i_h6_efuse_xlate,
>>  +};
>>  +
>>   static const struct of_device_id sun50i_cpufreq_match_list[] = {
>>  -	{ .compatible = "allwinner,sun50i-h6" },
>>  +	{ .compatible = "allwinner,sun50i-h6", .data = &sun50i_h6_data },
>>  +	{ .compatible = "allwinner,sun50i-h616", .data = 
>> &sun50i_h616_data },
>>   	{}
>>   };
>>   MODULE_DEVICE_TABLE(of, sun50i_cpufreq_match_list);
>>  @@ -197,8 +300,8 @@ static int __init sun50i_cpufreq_init(void)
>>   		return ret;
>> 
>>   	sun50i_cpufreq_pdev =
>>  -		platform_device_register_simple("sun50i-cpufreq-nvmem",
>>  -						-1, NULL, 0);
>>  +		platform_device_register_data(NULL, "sun50i-cpufreq-nvmem",
>>  +						-1, match, sizeof(*match));
>>   	ret = PTR_ERR_OR_ZERO(sun50i_cpufreq_pdev);
>>   	if (ret == 0)
>>   		return 0;
>> 
> 

Re: [PATCH 2/6] cpufreq: dt-platdev: Blocklist allwinner,h616 SoC

[Icenowy Zheng]

在 2023-09-04星期一的 21:40 +0100，Andre Przywara写道：
> On Mon, 04 Sep 2023 17:57:02 +0200
> Martin Botka <martin.botka@somainline.org> wrote:
> 
> > The AllWinner H616 uses H6 cpufreq driver.
> > Add it to blocklist so its not created twice
> 
> That looks alright, but I think needs to be squashed into the patch
> that enables the H616 driver operation, to avoid regressions during
> bisecting.

Well I think if it's before the H616 enablement, it could be just okay.

> 
> Cheers,
> Andre
> 
> > 
> > Signed-off-by: Martin Botka <martin.botka@somainline.org>
> > ---
> >  drivers/cpufreq/cpufreq-dt-platdev.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c
> > b/drivers/cpufreq/cpufreq-dt-platdev.c
> > index e2b20080de3a..51818cef8979 100644
> > --- a/drivers/cpufreq/cpufreq-dt-platdev.c
> > +++ b/drivers/cpufreq/cpufreq-dt-platdev.c
> > @@ -104,6 +104,7 @@ static const struct of_device_id allowlist[]
> > __initconst = {
> >   */
> >  static const struct of_device_id blocklist[] __initconst = {
> >         { .compatible = "allwinner,sun50i-h6", },
> > +       { .compatible = "allwinner,sun50i-h616", },
> >  
> >         { .compatible = "apple,arm-platform", },
> >  
> > 
> 
> 

Re: [PATCH 2/6] cpufreq: dt-platdev: Blocklist allwinner,h616 SoC

[Andre Przywara]

On Mon, 04 Sep 2023 17:57:02 +0200
Martin Botka <martin.botka@somainline.org> wrote:

> The AllWinner H616 uses H6 cpufreq driver.
> Add it to blocklist so its not created twice
> 
> Signed-off-by: Martin Botka <martin.botka@somainline.org>
> ---
>  drivers/cpufreq/cpufreq-dt-platdev.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c b/drivers/cpufreq/cpufreq-dt-platdev.c
> index e2b20080de3a..51818cef8979 100644
> --- a/drivers/cpufreq/cpufreq-dt-platdev.c
> +++ b/drivers/cpufreq/cpufreq-dt-platdev.c
> @@ -104,6 +104,7 @@ static const struct of_device_id allowlist[] __initconst = {
>   */
>  static const struct of_device_id blocklist[] __initconst = {
>  	{ .compatible = "allwinner,sun50i-h6", },
> +	{ .compatible = "allwinner,sun50i-h616", },

The OrangePi Zero3 DT uses the "allwinner,sun50i-h618" compatible string.
Definitely for the purpose of this patch the SoCs are the same, so just
add another line with that name, please.

Cheers,
Andre


>  
>  	{ .compatible = "apple,arm-platform", },
>  
> 

Re: [PATCH 4/6] cpufreq: sun50i: Add H616 support

[Andre Przywara]

On Mon, 04 Sep 2023 17:57:04 +0200
Martin Botka <martin.botka@somainline.org> wrote:

Hi Martin,

> AllWinner H616 SoC has few revisions that support different list
> of uV and frequencies.
> 
> Some revisions have the same NVMEM value and thus we have to check
> the SoC revision from SMCCC to differentiate between them.
> 
> Signed-off-by: Martin Botka <martin.botka@somainline.org>
> ---
>  drivers/cpufreq/sun50i-cpufreq-nvmem.c | 149 ++++++++++++++++++++++++++++-----
>  1 file changed, 126 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/cpufreq/sun50i-cpufreq-nvmem.c b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> index 4321d7bbe769..19c126fb081e 100644
> --- a/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> +++ b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> @@ -10,6 +10,7 @@
>  
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>  
> +#include <linux/arm-smccc.h>
>  #include <linux/cpu.h>
>  #include <linux/module.h>
>  #include <linux/nvmem-consumer.h>
> @@ -23,20 +24,94 @@
>  #define NVMEM_MASK	0x7
>  #define NVMEM_SHIFT	5
>  
> +struct sunxi_cpufreq_soc_data {
> +	int (*efuse_xlate)(u32 *versions, u32 *efuse, char *name, size_t len);
> +	u8 ver_freq_limit;
> +};
> +
>  static struct platform_device *cpufreq_dt_pdev, *sun50i_cpufreq_pdev;
>  
> +static int sun50i_h616_efuse_xlate(u32 *versions, u32 *efuse, char *name, size_t len)
> +{
> +	int value = 0;
> +	u32 speedgrade = 0;
> +	u32 i;
> +	int ver_bits = arm_smccc_get_soc_id_revision();
> +
> +	if (len > 4) {
> +		pr_err("Invalid nvmem cell length\n");
> +		return -EINVAL;
> +	}
> +
> +	for (i = 0; i < len; i++)
> +		speedgrade |= (efuse[i] << (i * 8));
> +
> +	switch (speedgrade) {
> +	case 0x2000:
> +		value = 0;
> +		break;
> +	case 0x2400:
> +	case 0x7400:
> +	case 0x2c00:
> +	case 0x7c00:
> +		if (ver_bits <= 1) {
> +			/* ic version A/B */
> +			value = 1;
> +		} else {
> +			/* ic version C and later version */
> +			value = 2;
> +		}
> +		break;
> +	case 0x5000:
> +	case 0x5400:
> +	case 0x6000:
> +		value = 3;
> +		break;
> +	case 0x5c00:
> +		value = 4;
> +		break;
> +	case 0x5d00:
> +	default:
> +		value = 0;
> +	}
> +	*versions = (1 << value);
> +	snprintf(name, MAX_NAME_LEN, "speed%d", value);
> +	return 0;
> +}
> +
> +static int sun50i_h6_efuse_xlate(u32 *versions, u32 *efuse, char *name, size_t len)
> +{
> +	int efuse_value = (*efuse >> NVMEM_SHIFT) & NVMEM_MASK;
> +
> +	/*
> +	 * We treat unexpected efuse values as if the SoC was from
> +	 * the slowest bin. Expected efuse values are 1-3, slowest
> +	 * to fastest.
> +	 */
> +	if (efuse_value >= 1 && efuse_value <= 3)
> +		*versions = efuse_value - 1;
> +	else
> +		*versions = 0;
> +
> +	snprintf(name, MAX_NAME_LEN, "speed%d", *versions);
> +	return 0;
> +}
> +
>  /**
>   * sun50i_cpufreq_get_efuse() - Determine speed grade from efuse value
> + * @soc_data: Struct containing soc specific data & functions
>   * @versions: Set to the value parsed from efuse
> + * @name: Set to the name of speed
>   *
>   * Returns 0 if success.
>   */
> -static int sun50i_cpufreq_get_efuse(u32 *versions)
> +static int sun50i_cpufreq_get_efuse(const struct sunxi_cpufreq_soc_data *soc_data,
> +				    u32 *versions, char *name)
>  {
>  	struct nvmem_cell *speedbin_nvmem;
>  	struct device_node *np;
>  	struct device *cpu_dev;
> -	u32 *speedbin, efuse_value;
> +	u32 *speedbin;
>  	size_t len;
>  	int ret;
>  
> @@ -48,9 +123,9 @@ static int sun50i_cpufreq_get_efuse(u32 *versions)
>  	if (!np)
>  		return -ENOENT;
>  
> -	ret = of_device_is_compatible(np,
> -				      "allwinner,sun50i-h6-operating-points");
> -	if (!ret) {
> +	if (of_device_is_compatible(np, "allwinner,sun50i-h6-operating-points")) {
> +	} else if (of_device_is_compatible(np, "allwinner,sun50i-h616-operating-points")) {
> +	} else {
>  		of_node_put(np);
>  		return -ENOENT;
>  	}
> @@ -66,17 +141,9 @@ static int sun50i_cpufreq_get_efuse(u32 *versions)
>  	if (IS_ERR(speedbin))
>  		return PTR_ERR(speedbin);
>  
> -	efuse_value = (*speedbin >> NVMEM_SHIFT) & NVMEM_MASK;
> -
> -	/*
> -	 * We treat unexpected efuse values as if the SoC was from
> -	 * the slowest bin. Expected efuse values are 1-3, slowest
> -	 * to fastest.
> -	 */
> -	if (efuse_value >= 1 && efuse_value <= 3)
> -		*versions = efuse_value - 1;
> -	else
> -		*versions = 0;
> +	ret = soc_data->efuse_xlate(versions, speedbin, name, len);
> +	if (ret)
> +		return ret;
>  
>  	kfree(speedbin);
>  	return 0;
> @@ -84,25 +151,30 @@ static int sun50i_cpufreq_get_efuse(u32 *versions)
>  
>  static int sun50i_cpufreq_nvmem_probe(struct platform_device *pdev)
>  {
> +	const struct of_device_id *match;
> +	const struct sunxi_cpufreq_soc_data *soc_data;
>  	int *opp_tokens;
>  	char name[MAX_NAME_LEN];
>  	unsigned int cpu;
> -	u32 speed = 0;
> +	u32 version = 0;
>  	int ret;
>  
> +	match = dev_get_platdata(&pdev->dev);
> +	if (!match)
> +		return -EINVAL;
> +	soc_data = match->data;
> +
>  	opp_tokens = kcalloc(num_possible_cpus(), sizeof(*opp_tokens),
>  			     GFP_KERNEL);
>  	if (!opp_tokens)
>  		return -ENOMEM;
>  
> -	ret = sun50i_cpufreq_get_efuse(&speed);
> +	ret = sun50i_cpufreq_get_efuse(match->data, &version, name);
>  	if (ret) {
>  		kfree(opp_tokens);
>  		return ret;
>  	}
>  
> -	snprintf(name, MAX_NAME_LEN, "speed%d", speed);
> -
>  	for_each_possible_cpu(cpu) {
>  		struct device *cpu_dev = get_cpu_device(cpu);
>  
> @@ -117,6 +189,16 @@ static int sun50i_cpufreq_nvmem_probe(struct platform_device *pdev)
>  			pr_err("Failed to set prop name\n");
>  			goto free_opp;
>  		}
> +
> +		if (soc_data->ver_freq_limit) {
> +			opp_tokens[cpu] = dev_pm_opp_set_supported_hw(cpu_dev,
> +								  &version, 1);

This will overwrite opp_tokens[cpu] from the dev_pm_opp_set_prop_name()
call above. As the DTs stand today, only one of them would actually
provide OPPs, but still they reserve data structures and return discrete
tokens, so all of them would need to be "put" at cleanup/removal.

One solution I found is to pull in the two wrappers of
dev_pm_opp_set_prop_name() and dev_pm_opp_set_supported_hw(), and combine them in one call.
The dev_pm_opp_config struct can have multiple fields set, and
dev_pm_opp_set_config() will try all of them.

> +			if (opp_tokens[cpu] < 0) {
> +				ret = opp_tokens[cpu];
> +				pr_err("Failed to set hw\n");
> +				goto free_opp;
> +			}
> +		}
>  	}
>  
>  	cpufreq_dt_pdev = platform_device_register_simple("cpufreq-dt", -1,
> @@ -132,6 +214,8 @@ static int sun50i_cpufreq_nvmem_probe(struct platform_device *pdev)
>  free_opp:
>  	for_each_possible_cpu(cpu)
>  		dev_pm_opp_put_prop_name(opp_tokens[cpu]);
> +		if (soc_data->ver_freq_limit)
> +			dev_pm_opp_put_supported_hw(opp_tokens[cpu]);

See above, that does not make sense, since you "put" the same token again
as in the dev_pm_opp_put_prop_name() call right before. Would be
automatically solved by using only one dev_pm_opp_set_config() above.

>  	kfree(opp_tokens);
>  
>  	return ret;
> @@ -140,12 +224,21 @@ static int sun50i_cpufreq_nvmem_probe(struct platform_device *pdev)
>  static int sun50i_cpufreq_nvmem_remove(struct platform_device *pdev)
>  {
>  	int *opp_tokens = platform_get_drvdata(pdev);
> +	const struct of_device_id *match;
> +	const struct sunxi_cpufreq_soc_data *soc_data;
>  	unsigned int cpu;
>  
> +	match = dev_get_platdata(&pdev->dev);
> +	if (!match)
> +		return -EINVAL;
> +	soc_data = match->data;
> +
>  	platform_device_unregister(cpufreq_dt_pdev);
>  
>  	for_each_possible_cpu(cpu)
>  		dev_pm_opp_put_prop_name(opp_tokens[cpu]);
> +		if (soc_data->ver_freq_limit)
> +			dev_pm_opp_put_supported_hw(opp_tokens[cpu]);
>  
>  	kfree(opp_tokens);
>  
> @@ -160,8 +253,18 @@ static struct platform_driver sun50i_cpufreq_driver = {
>  	},
>  };
>  
> +static const struct sunxi_cpufreq_soc_data sun50i_h616_data = {
> +	.efuse_xlate = sun50i_h616_efuse_xlate,
> +	.ver_freq_limit = true,

As hinted above, the framework allows for both to be tried, and will do
the right thing depending on what the DT provides, so there is no need for
this flag.

Cheers,
Andre

> +};
> +
> +static const struct sunxi_cpufreq_soc_data sun50i_h6_data = {
> +	.efuse_xlate = sun50i_h6_efuse_xlate,
> +};
> +
>  static const struct of_device_id sun50i_cpufreq_match_list[] = {
> -	{ .compatible = "allwinner,sun50i-h6" },
> +	{ .compatible = "allwinner,sun50i-h6", .data = &sun50i_h6_data },
> +	{ .compatible = "allwinner,sun50i-h616", .data = &sun50i_h616_data },
>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, sun50i_cpufreq_match_list);
> @@ -197,8 +300,8 @@ static int __init sun50i_cpufreq_init(void)
>  		return ret;
>  
>  	sun50i_cpufreq_pdev =
> -		platform_device_register_simple("sun50i-cpufreq-nvmem",
> -						-1, NULL, 0);
> +		platform_device_register_data(NULL, "sun50i-cpufreq-nvmem",
> +						-1, match, sizeof(*match));
>  	ret = PTR_ERR_OR_ZERO(sun50i_cpufreq_pdev);
>  	if (ret == 0)
>  		return 0;
>