[RFC PATCH v2 0/3] Add support for reading D1 efuse speed bin

[RFC PATCH v2 0/3] Add support for reading D1 efuse speed bin

[Brandon Cheo Fusi]

Hi everyone,

This series is an attempt to get feedback on decoding D1 efuse speed bins
in the Sun50i H6 cpufreq driver, and turning the result into a meaningful
value that selects voltage ranges in an OPP table.

I want to make sure I get this right before sending in a v3 of the D1
cpufreq support series here

https://lore.kernel.org/linux-sunxi/20231218110543.64044-1-fusibrandon13@gmail.com/T/#t

which is currently stuck at

https://lore.kernel.org/linux-sunxi/aad8302d-a015-44ee-ad11-1a4c6e00074c@sholland.org/

Changes in v2:
- Make speed bin decoding generic in one patch and add D1 support in a
  separate patch
- Fix OPP voltage ranges to avoid stability issues

Brandon Cheo Fusi (3):
  cpufreq: sun50i: Refactor speed bin decoding
  cpufreq: sun50i: Add support for D1's speed bin decoding
  riscv: dts: allwinner: Fill in OPPs

 arch/riscv/boot/dts/allwinner/sun20i-d1s.dtsi | 19 +++-
 drivers/cpufreq/sun50i-cpufreq-nvmem.c        | 89 +++++++++++++++----
 2 files changed, 89 insertions(+), 19 deletions(-)

-- 
2.30.2

[RFC PATCH v2 0/3] Add support for reading D1 efuse speed bin

[Brandon Cheo Fusi]

Hi everyone,

This series is an attempt to get feedback on decoding D1 efuse speed bins
in the Sun50i H6 cpufreq driver, and turning the result into a meaningful
value that selects voltage ranges in an OPP table.

I want to make sure I get this right before sending in a v3 of the D1
cpufreq support series here

https://lore.kernel.org/linux-sunxi/20231218110543.64044-1-fusibrandon13@gmail.com/T/#t

which is currently stuck at

https://lore.kernel.org/linux-sunxi/aad8302d-a015-44ee-ad11-1a4c6e00074c@sholland.org/

Changes in v2:
- Make speed bin decoding generic in one patch and add D1 support in a
  separate patch
- Fix OPP voltage ranges to avoid stability issues

Brandon Cheo Fusi (3):
  cpufreq: sun50i: Refactor speed bin decoding
  cpufreq: sun50i: Add support for D1's speed bin decoding
  riscv: dts: allwinner: Fill in OPPs

 arch/riscv/boot/dts/allwinner/sun20i-d1s.dtsi | 19 +++-
 drivers/cpufreq/sun50i-cpufreq-nvmem.c        | 89 +++++++++++++++----
 2 files changed, 89 insertions(+), 19 deletions(-)

-- 
2.30.2


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[RFC PATCH v2 0/3] Add support for reading D1 efuse speed bin

[Brandon Cheo Fusi]

Hi everyone,

This series is an attempt to get feedback on decoding D1 efuse speed bins
in the Sun50i H6 cpufreq driver, and turning the result into a meaningful
value that selects voltage ranges in an OPP table.

I want to make sure I get this right before sending in a v3 of the D1
cpufreq support series here

https://lore.kernel.org/linux-sunxi/20231218110543.64044-1-fusibrandon13@gmail.com/T/#t

which is currently stuck at

https://lore.kernel.org/linux-sunxi/aad8302d-a015-44ee-ad11-1a4c6e00074c@sholland.org/

Changes in v2:
- Make speed bin decoding generic in one patch and add D1 support in a
  separate patch
- Fix OPP voltage ranges to avoid stability issues

Brandon Cheo Fusi (3):
  cpufreq: sun50i: Refactor speed bin decoding
  cpufreq: sun50i: Add support for D1's speed bin decoding
  riscv: dts: allwinner: Fill in OPPs

 arch/riscv/boot/dts/allwinner/sun20i-d1s.dtsi | 19 +++-
 drivers/cpufreq/sun50i-cpufreq-nvmem.c        | 89 +++++++++++++++----
 2 files changed, 89 insertions(+), 19 deletions(-)

-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC PATCH v2 1/3] cpufreq: sun50i: Refactor speed bin decoding

[Brandon Cheo Fusi]

Make converting the speed bin value into a speed grade generic
and determined by a platform specific callback.

Signed-off-by: Brandon Cheo Fusi <fusibrandon13@gmail.com>
---
 drivers/cpufreq/sun50i-cpufreq-nvmem.c | 55 ++++++++++++++++++--------
 1 file changed, 39 insertions(+), 16 deletions(-)

diff --git a/drivers/cpufreq/sun50i-cpufreq-nvmem.c b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
index 32a9c88f8..fc509fc49 100644
--- a/drivers/cpufreq/sun50i-cpufreq-nvmem.c
+++ b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
@@ -25,6 +25,38 @@
 
 static struct platform_device *cpufreq_dt_pdev, *sun50i_cpufreq_pdev;
 
+struct sunxi_cpufreq_data {
+	u32 (*efuse_xlate)(u32 *speedbin, size_t len);
+};
+
+static u32 sun50i_efuse_xlate(u32 *speedbin, size_t len)
+{
+	u32 efuse_value = 0;
+
+	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)
+		return efuse_value - 1;
+	else
+		return 0;
+}
+
+struct sunxi_cpufreq_data sun50i_cpufreq_data = {
+	.efuse_xlate = sun50i_efuse_xlate,
+};
+
+static const struct of_device_id cpu_opp_match_list[] = {
+	{ .compatible = "allwinner,sun50i-h6-operating-points",
+	  .data = &sun50i_cpufreq_data,
+	},
+	{}
+};
+
 /**
  * sun50i_cpufreq_get_efuse() - Determine speed grade from efuse value
  * @versions: Set to the value parsed from efuse
@@ -36,9 +68,10 @@ static int sun50i_cpufreq_get_efuse(u32 *versions)
 	struct nvmem_cell *speedbin_nvmem;
 	struct device_node *np;
 	struct device *cpu_dev;
-	u32 *speedbin, efuse_value;
+	const struct of_device_id *match;
+	const struct sunxi_cpufreq_data *opp_data;
+	u32 *speedbin;
 	size_t len;
-	int ret;
 
 	cpu_dev = get_cpu_device(0);
 	if (!cpu_dev)
@@ -48,12 +81,12 @@ 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) {
+	match = of_match_node(cpu_opp_match_list, np);
+	if (!match) {
 		of_node_put(np);
 		return -ENOENT;
 	}
+	opp_data = match->data;
 
 	speedbin_nvmem = of_nvmem_cell_get(np, NULL);
 	of_node_put(np);
@@ -66,17 +99,7 @@ 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;
+	*versions = opp_data->efuse_xlate(speedbin, len);
 
 	kfree(speedbin);
 	return 0;
-- 
2.30.2

[RFC PATCH v2 1/3] cpufreq: sun50i: Refactor speed bin decoding

[Brandon Cheo Fusi]

Make converting the speed bin value into a speed grade generic
and determined by a platform specific callback.

Signed-off-by: Brandon Cheo Fusi <fusibrandon13@gmail.com>
---
 drivers/cpufreq/sun50i-cpufreq-nvmem.c | 55 ++++++++++++++++++--------
 1 file changed, 39 insertions(+), 16 deletions(-)

diff --git a/drivers/cpufreq/sun50i-cpufreq-nvmem.c b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
index 32a9c88f8..fc509fc49 100644
--- a/drivers/cpufreq/sun50i-cpufreq-nvmem.c
+++ b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
@@ -25,6 +25,38 @@
 
 static struct platform_device *cpufreq_dt_pdev, *sun50i_cpufreq_pdev;
 
+struct sunxi_cpufreq_data {
+	u32 (*efuse_xlate)(u32 *speedbin, size_t len);
+};
+
+static u32 sun50i_efuse_xlate(u32 *speedbin, size_t len)
+{
+	u32 efuse_value = 0;
+
+	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)
+		return efuse_value - 1;
+	else
+		return 0;
+}
+
+struct sunxi_cpufreq_data sun50i_cpufreq_data = {
+	.efuse_xlate = sun50i_efuse_xlate,
+};
+
+static const struct of_device_id cpu_opp_match_list[] = {
+	{ .compatible = "allwinner,sun50i-h6-operating-points",
+	  .data = &sun50i_cpufreq_data,
+	},
+	{}
+};
+
 /**
  * sun50i_cpufreq_get_efuse() - Determine speed grade from efuse value
  * @versions: Set to the value parsed from efuse
@@ -36,9 +68,10 @@ static int sun50i_cpufreq_get_efuse(u32 *versions)
 	struct nvmem_cell *speedbin_nvmem;
 	struct device_node *np;
 	struct device *cpu_dev;
-	u32 *speedbin, efuse_value;
+	const struct of_device_id *match;
+	const struct sunxi_cpufreq_data *opp_data;
+	u32 *speedbin;
 	size_t len;
-	int ret;
 
 	cpu_dev = get_cpu_device(0);
 	if (!cpu_dev)
@@ -48,12 +81,12 @@ 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) {
+	match = of_match_node(cpu_opp_match_list, np);
+	if (!match) {
 		of_node_put(np);
 		return -ENOENT;
 	}
+	opp_data = match->data;
 
 	speedbin_nvmem = of_nvmem_cell_get(np, NULL);
 	of_node_put(np);
@@ -66,17 +99,7 @@ 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;
+	*versions = opp_data->efuse_xlate(speedbin, len);
 
 	kfree(speedbin);
 	return 0;
-- 
2.30.2


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[RFC PATCH v2 1/3] cpufreq: sun50i: Refactor speed bin decoding

[Brandon Cheo Fusi]

Make converting the speed bin value into a speed grade generic
and determined by a platform specific callback.

Signed-off-by: Brandon Cheo Fusi <fusibrandon13@gmail.com>
---
 drivers/cpufreq/sun50i-cpufreq-nvmem.c | 55 ++++++++++++++++++--------
 1 file changed, 39 insertions(+), 16 deletions(-)

diff --git a/drivers/cpufreq/sun50i-cpufreq-nvmem.c b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
index 32a9c88f8..fc509fc49 100644
--- a/drivers/cpufreq/sun50i-cpufreq-nvmem.c
+++ b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
@@ -25,6 +25,38 @@
 
 static struct platform_device *cpufreq_dt_pdev, *sun50i_cpufreq_pdev;
 
+struct sunxi_cpufreq_data {
+	u32 (*efuse_xlate)(u32 *speedbin, size_t len);
+};
+
+static u32 sun50i_efuse_xlate(u32 *speedbin, size_t len)
+{
+	u32 efuse_value = 0;
+
+	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)
+		return efuse_value - 1;
+	else
+		return 0;
+}
+
+struct sunxi_cpufreq_data sun50i_cpufreq_data = {
+	.efuse_xlate = sun50i_efuse_xlate,
+};
+
+static const struct of_device_id cpu_opp_match_list[] = {
+	{ .compatible = "allwinner,sun50i-h6-operating-points",
+	  .data = &sun50i_cpufreq_data,
+	},
+	{}
+};
+
 /**
  * sun50i_cpufreq_get_efuse() - Determine speed grade from efuse value
  * @versions: Set to the value parsed from efuse
@@ -36,9 +68,10 @@ static int sun50i_cpufreq_get_efuse(u32 *versions)
 	struct nvmem_cell *speedbin_nvmem;
 	struct device_node *np;
 	struct device *cpu_dev;
-	u32 *speedbin, efuse_value;
+	const struct of_device_id *match;
+	const struct sunxi_cpufreq_data *opp_data;
+	u32 *speedbin;
 	size_t len;
-	int ret;
 
 	cpu_dev = get_cpu_device(0);
 	if (!cpu_dev)
@@ -48,12 +81,12 @@ 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) {
+	match = of_match_node(cpu_opp_match_list, np);
+	if (!match) {
 		of_node_put(np);
 		return -ENOENT;
 	}
+	opp_data = match->data;
 
 	speedbin_nvmem = of_nvmem_cell_get(np, NULL);
 	of_node_put(np);
@@ -66,17 +99,7 @@ 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;
+	*versions = opp_data->efuse_xlate(speedbin, len);
 
 	kfree(speedbin);
 	return 0;
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC PATCH v2 2/3] cpufreq: sun50i: Add support for D1's speed bin decoding

[Brandon Cheo Fusi]

Adds support for decoding the efuse value read from D1 efuse speed
bins, and factors out equivalent code for sun50i.

The algorithm is gotten from

https://github.com/Tina-Linux/linux-5.4/blob/master/drivers/cpufreq/sun50i-cpufreq-nvmem.c#L293-L338

and maps an efuse value to either 0 or 1, with 1 meaning stable at
a lower supply voltage for the same clock frequency.

Signed-off-by: Brandon Cheo Fusi <fusibrandon13@gmail.com>
---
 drivers/cpufreq/sun50i-cpufreq-nvmem.c | 34 ++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/cpufreq/sun50i-cpufreq-nvmem.c b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
index fc509fc49..b1cb95308 100644
--- a/drivers/cpufreq/sun50i-cpufreq-nvmem.c
+++ b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
@@ -29,6 +29,33 @@ struct sunxi_cpufreq_data {
 	u32 (*efuse_xlate)(u32 *speedbin, size_t len);
 };
 
+static u32 sun20i_efuse_xlate(u32 *speedbin, size_t len)
+{
+	u32 ret, efuse_value = 0;
+	int i;
+
+	for (i = 0; i < len; i++)
+		efuse_value |= ((u32)speedbin[i] << (i * 8));
+
+	switch (efuse_value) {
+	case 0x5e00:
+		/* QFN package */
+		ret = 0;
+		break;
+	case 0x5c00:
+	case 0x7400:
+		/* QFN package */
+		ret = 1;
+		break;
+	case 0x5000:
+	default:
+		/* BGA package */
+		ret = 0;
+	}
+
+	return ret;
+}
+
 static u32 sun50i_efuse_xlate(u32 *speedbin, size_t len)
 {
 	u32 efuse_value = 0;
@@ -46,6 +73,10 @@ static u32 sun50i_efuse_xlate(u32 *speedbin, size_t len)
 		return 0;
 }
 
+struct sunxi_cpufreq_data sun20i_cpufreq_data = {
+	.efuse_xlate = sun20i_efuse_xlate,
+};
+
 struct sunxi_cpufreq_data sun50i_cpufreq_data = {
 	.efuse_xlate = sun50i_efuse_xlate,
 };
@@ -54,6 +85,9 @@ static const struct of_device_id cpu_opp_match_list[] = {
 	{ .compatible = "allwinner,sun50i-h6-operating-points",
 	  .data = &sun50i_cpufreq_data,
 	},
+	{ .compatible = "allwinner,sun20i-d1-operating-points",
+	  .data = &sun20i_cpufreq_data,
+	},
 	{}
 };
 
-- 
2.30.2

[RFC PATCH v2 2/3] cpufreq: sun50i: Add support for D1's speed bin decoding

[Brandon Cheo Fusi]

Adds support for decoding the efuse value read from D1 efuse speed
bins, and factors out equivalent code for sun50i.

The algorithm is gotten from

https://github.com/Tina-Linux/linux-5.4/blob/master/drivers/cpufreq/sun50i-cpufreq-nvmem.c#L293-L338

and maps an efuse value to either 0 or 1, with 1 meaning stable at
a lower supply voltage for the same clock frequency.

Signed-off-by: Brandon Cheo Fusi <fusibrandon13@gmail.com>
---
 drivers/cpufreq/sun50i-cpufreq-nvmem.c | 34 ++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/cpufreq/sun50i-cpufreq-nvmem.c b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
index fc509fc49..b1cb95308 100644
--- a/drivers/cpufreq/sun50i-cpufreq-nvmem.c
+++ b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
@@ -29,6 +29,33 @@ struct sunxi_cpufreq_data {
 	u32 (*efuse_xlate)(u32 *speedbin, size_t len);
 };
 
+static u32 sun20i_efuse_xlate(u32 *speedbin, size_t len)
+{
+	u32 ret, efuse_value = 0;
+	int i;
+
+	for (i = 0; i < len; i++)
+		efuse_value |= ((u32)speedbin[i] << (i * 8));
+
+	switch (efuse_value) {
+	case 0x5e00:
+		/* QFN package */
+		ret = 0;
+		break;
+	case 0x5c00:
+	case 0x7400:
+		/* QFN package */
+		ret = 1;
+		break;
+	case 0x5000:
+	default:
+		/* BGA package */
+		ret = 0;
+	}
+
+	return ret;
+}
+
 static u32 sun50i_efuse_xlate(u32 *speedbin, size_t len)
 {
 	u32 efuse_value = 0;
@@ -46,6 +73,10 @@ static u32 sun50i_efuse_xlate(u32 *speedbin, size_t len)
 		return 0;
 }
 
+struct sunxi_cpufreq_data sun20i_cpufreq_data = {
+	.efuse_xlate = sun20i_efuse_xlate,
+};
+
 struct sunxi_cpufreq_data sun50i_cpufreq_data = {
 	.efuse_xlate = sun50i_efuse_xlate,
 };
@@ -54,6 +85,9 @@ static const struct of_device_id cpu_opp_match_list[] = {
 	{ .compatible = "allwinner,sun50i-h6-operating-points",
 	  .data = &sun50i_cpufreq_data,
 	},
+	{ .compatible = "allwinner,sun20i-d1-operating-points",
+	  .data = &sun20i_cpufreq_data,
+	},
 	{}
 };
 
-- 
2.30.2


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[RFC PATCH v2 2/3] cpufreq: sun50i: Add support for D1's speed bin decoding

[Brandon Cheo Fusi]

Adds support for decoding the efuse value read from D1 efuse speed
bins, and factors out equivalent code for sun50i.

The algorithm is gotten from

https://github.com/Tina-Linux/linux-5.4/blob/master/drivers/cpufreq/sun50i-cpufreq-nvmem.c#L293-L338

and maps an efuse value to either 0 or 1, with 1 meaning stable at
a lower supply voltage for the same clock frequency.

Signed-off-by: Brandon Cheo Fusi <fusibrandon13@gmail.com>
---
 drivers/cpufreq/sun50i-cpufreq-nvmem.c | 34 ++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/cpufreq/sun50i-cpufreq-nvmem.c b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
index fc509fc49..b1cb95308 100644
--- a/drivers/cpufreq/sun50i-cpufreq-nvmem.c
+++ b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
@@ -29,6 +29,33 @@ struct sunxi_cpufreq_data {
 	u32 (*efuse_xlate)(u32 *speedbin, size_t len);
 };
 
+static u32 sun20i_efuse_xlate(u32 *speedbin, size_t len)
+{
+	u32 ret, efuse_value = 0;
+	int i;
+
+	for (i = 0; i < len; i++)
+		efuse_value |= ((u32)speedbin[i] << (i * 8));
+
+	switch (efuse_value) {
+	case 0x5e00:
+		/* QFN package */
+		ret = 0;
+		break;
+	case 0x5c00:
+	case 0x7400:
+		/* QFN package */
+		ret = 1;
+		break;
+	case 0x5000:
+	default:
+		/* BGA package */
+		ret = 0;
+	}
+
+	return ret;
+}
+
 static u32 sun50i_efuse_xlate(u32 *speedbin, size_t len)
 {
 	u32 efuse_value = 0;
@@ -46,6 +73,10 @@ static u32 sun50i_efuse_xlate(u32 *speedbin, size_t len)
 		return 0;
 }
 
+struct sunxi_cpufreq_data sun20i_cpufreq_data = {
+	.efuse_xlate = sun20i_efuse_xlate,
+};
+
 struct sunxi_cpufreq_data sun50i_cpufreq_data = {
 	.efuse_xlate = sun50i_efuse_xlate,
 };
@@ -54,6 +85,9 @@ static const struct of_device_id cpu_opp_match_list[] = {
 	{ .compatible = "allwinner,sun50i-h6-operating-points",
 	  .data = &sun50i_cpufreq_data,
 	},
+	{ .compatible = "allwinner,sun20i-d1-operating-points",
+	  .data = &sun20i_cpufreq_data,
+	},
 	{}
 };
 
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC PATCH v2 3/3] riscv: dts: allwinner: Fill in OPPs

[Brandon Cheo Fusi]

Specify two voltage ranges, in order of increasing stability,
for each OPP.

Link: https://github.com/Tina-Linux/linux-5.4/blob/master/arch/riscv/boot/dts/sunxi/sun20iw1p1.dtsi#L118-L133
Link: https://github.com/mangopi-sbc/tina-linux-5.4/blob/0d4903ebd9d2194ad914686d5b0fc1ddacf11a9d/arch/riscv/boot/dts/sunxi/sun20iw1p1.dtsi#L118-L182

Signed-off-by: Brandon Cheo Fusi <fusibrandon13@gmail.com>
---
 arch/riscv/boot/dts/allwinner/sun20i-d1s.dtsi | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/arch/riscv/boot/dts/allwinner/sun20i-d1s.dtsi b/arch/riscv/boot/dts/allwinner/sun20i-d1s.dtsi
index 64c3c2e6c..7e2e015e0 100644
--- a/arch/riscv/boot/dts/allwinner/sun20i-d1s.dtsi
+++ b/arch/riscv/boot/dts/allwinner/sun20i-d1s.dtsi
@@ -39,16 +39,23 @@ cpu0_intc: interrupt-controller {
 	};
 
 	opp_table_cpu: opp-table-cpu {
-		compatible = "operating-points-v2";
+		compatible = "allwinner,sun20i-d1-operating-points";
+		nvmem-cells = <&cpu_speed_grade>;
+		nvmem-cell-names = "speed";
+		opp-shared;
 
 		opp-408000000 {
 			opp-hz = /bits/ 64 <408000000>;
-			opp-microvolt = <900000 900000 1100000>;
+
+			opp-microvolt-speed0 = <950000 950000 1100000>;
+			opp-microvolt-speed1 = <900000 900000 1100000>;
 		};
 
 		opp-1080000000 {
 			opp-hz = /bits/ 64 <1008000000>;
-			opp-microvolt = <900000 900000 1100000>;
+
+			opp-microvolt-speed0 = <1100000>;
+			opp-microvolt-speed1 = <950000 950000 1100000>;
 		};
 	};
 
@@ -115,3 +122,9 @@ pmu {
 			<0x00000000 0x0000000f 0xffffffff 0xffffffff 0x00020000>;
 	};
 };
+
+&sid {
+	cpu_speed_grade: cpu-speed-grade@0 {
+		reg = <0x00 0x2>;
+	};
+};
-- 
2.30.2

[RFC PATCH v2 3/3] riscv: dts: allwinner: Fill in OPPs

[Brandon Cheo Fusi]

Specify two voltage ranges, in order of increasing stability,
for each OPP.

Link: https://github.com/Tina-Linux/linux-5.4/blob/master/arch/riscv/boot/dts/sunxi/sun20iw1p1.dtsi#L118-L133
Link: https://github.com/mangopi-sbc/tina-linux-5.4/blob/0d4903ebd9d2194ad914686d5b0fc1ddacf11a9d/arch/riscv/boot/dts/sunxi/sun20iw1p1.dtsi#L118-L182

Signed-off-by: Brandon Cheo Fusi <fusibrandon13@gmail.com>
---
 arch/riscv/boot/dts/allwinner/sun20i-d1s.dtsi | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/arch/riscv/boot/dts/allwinner/sun20i-d1s.dtsi b/arch/riscv/boot/dts/allwinner/sun20i-d1s.dtsi
index 64c3c2e6c..7e2e015e0 100644
--- a/arch/riscv/boot/dts/allwinner/sun20i-d1s.dtsi
+++ b/arch/riscv/boot/dts/allwinner/sun20i-d1s.dtsi
@@ -39,16 +39,23 @@ cpu0_intc: interrupt-controller {
 	};
 
 	opp_table_cpu: opp-table-cpu {
-		compatible = "operating-points-v2";
+		compatible = "allwinner,sun20i-d1-operating-points";
+		nvmem-cells = <&cpu_speed_grade>;
+		nvmem-cell-names = "speed";
+		opp-shared;
 
 		opp-408000000 {
 			opp-hz = /bits/ 64 <408000000>;
-			opp-microvolt = <900000 900000 1100000>;
+
+			opp-microvolt-speed0 = <950000 950000 1100000>;
+			opp-microvolt-speed1 = <900000 900000 1100000>;
 		};
 
 		opp-1080000000 {
 			opp-hz = /bits/ 64 <1008000000>;
-			opp-microvolt = <900000 900000 1100000>;
+
+			opp-microvolt-speed0 = <1100000>;
+			opp-microvolt-speed1 = <950000 950000 1100000>;
 		};
 	};
 
@@ -115,3 +122,9 @@ pmu {
 			<0x00000000 0x0000000f 0xffffffff 0xffffffff 0x00020000>;
 	};
 };
+
+&sid {
+	cpu_speed_grade: cpu-speed-grade@0 {
+		reg = <0x00 0x2>;
+	};
+};
-- 
2.30.2


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[RFC PATCH v2 3/3] riscv: dts: allwinner: Fill in OPPs

[Brandon Cheo Fusi]

Specify two voltage ranges, in order of increasing stability,
for each OPP.

Link: https://github.com/Tina-Linux/linux-5.4/blob/master/arch/riscv/boot/dts/sunxi/sun20iw1p1.dtsi#L118-L133
Link: https://github.com/mangopi-sbc/tina-linux-5.4/blob/0d4903ebd9d2194ad914686d5b0fc1ddacf11a9d/arch/riscv/boot/dts/sunxi/sun20iw1p1.dtsi#L118-L182

Signed-off-by: Brandon Cheo Fusi <fusibrandon13@gmail.com>
---
 arch/riscv/boot/dts/allwinner/sun20i-d1s.dtsi | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/arch/riscv/boot/dts/allwinner/sun20i-d1s.dtsi b/arch/riscv/boot/dts/allwinner/sun20i-d1s.dtsi
index 64c3c2e6c..7e2e015e0 100644
--- a/arch/riscv/boot/dts/allwinner/sun20i-d1s.dtsi
+++ b/arch/riscv/boot/dts/allwinner/sun20i-d1s.dtsi
@@ -39,16 +39,23 @@ cpu0_intc: interrupt-controller {
 	};
 
 	opp_table_cpu: opp-table-cpu {
-		compatible = "operating-points-v2";
+		compatible = "allwinner,sun20i-d1-operating-points";
+		nvmem-cells = <&cpu_speed_grade>;
+		nvmem-cell-names = "speed";
+		opp-shared;
 
 		opp-408000000 {
 			opp-hz = /bits/ 64 <408000000>;
-			opp-microvolt = <900000 900000 1100000>;
+
+			opp-microvolt-speed0 = <950000 950000 1100000>;
+			opp-microvolt-speed1 = <900000 900000 1100000>;
 		};
 
 		opp-1080000000 {
 			opp-hz = /bits/ 64 <1008000000>;
-			opp-microvolt = <900000 900000 1100000>;
+
+			opp-microvolt-speed0 = <1100000>;
+			opp-microvolt-speed1 = <950000 950000 1100000>;
 		};
 	};
 
@@ -115,3 +122,9 @@ pmu {
 			<0x00000000 0x0000000f 0xffffffff 0xffffffff 0x00020000>;
 	};
 };
+
+&sid {
+	cpu_speed_grade: cpu-speed-grade@0 {
+		reg = <0x00 0x2>;
+	};
+};
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v2 2/3] cpufreq: sun50i: Add support for D1's speed bin decoding

[Andre Przywara]

On Thu, 21 Dec 2023 11:10:12 +0100
Brandon Cheo Fusi <fusibrandon13@gmail.com> wrote:

Hi Brandon,

thanks for the quick turnaround, and for splitting this code up, that
makes reasoning about this much easier!

> Adds support for decoding the efuse value read from D1 efuse speed
> bins, and factors out equivalent code for sun50i.
> 
> The algorithm is gotten from
> 
> https://github.com/Tina-Linux/linux-5.4/blob/master/drivers/cpufreq/sun50i-cpufreq-nvmem.c#L293-L338
> 
> and maps an efuse value to either 0 or 1, with 1 meaning stable at
> a lower supply voltage for the same clock frequency.
> 
> Signed-off-by: Brandon Cheo Fusi <fusibrandon13@gmail.com>
> ---
>  drivers/cpufreq/sun50i-cpufreq-nvmem.c | 34 ++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/drivers/cpufreq/sun50i-cpufreq-nvmem.c b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> index fc509fc49..b1cb95308 100644
> --- a/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> +++ b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> @@ -29,6 +29,33 @@ struct sunxi_cpufreq_data {
>  	u32 (*efuse_xlate)(u32 *speedbin, size_t len);
>  };
>  
> +static u32 sun20i_efuse_xlate(u32 *speedbin, size_t len)

I feel like this prototype can be shortened to:

static u32 sun20i_efuse_xlate(u32 speedbin)

See below.

> +{
> +	u32 ret, efuse_value = 0;
> +	int i;
> +
> +	for (i = 0; i < len; i++)
> +		efuse_value |= ((u32)speedbin[i] << (i * 8));

The cast is not needed. Looking deeper into the original code you linked
to, cell_value[] there is an array of u8, so they assemble a little endian
32-bit integer from *up to* four 8-bit values read from the nvmem.

So I think this code here is wrong, len is the size of the nvmem cells
holding the bin identifier, in *bytes*, so the idea here is to just read
the (lowest) 16 bits (in the D1 case, cf. "reg = <0x00 0x2>;" in the next
patch) from this nvmem cell. Here you are combining two 32-bit words into
efuse_value.

So I think this whole part above is actually not necessary: we are
expecting maximum 32 bits, and nvmem_cell_read() should take care of
masking off unrequested bits, so we get the correct value back already. So
can you try to remove the loop above, and use ...

> +
> +	switch (efuse_value) {

	switch (*speedbin & 0xffff) {

here instead? Or drop the pointer at all, and just use one u32 value, see
the above prototype.

Cheers,
Andre

P.S. This is just a "peephole review" of this patch, I haven't got around
to look at this whole scheme in whole yet, to see if we actually need this
or can simplify this or clean it up.


> +	case 0x5e00:
> +		/* QFN package */
> +		ret = 0;
> +		break;
> +	case 0x5c00:
> +	case 0x7400:
> +		/* QFN package */
> +		ret = 1;
> +		break;
> +	case 0x5000:
> +	default:
> +		/* BGA package */
> +		ret = 0;
> +	}
> +
> +	return ret;
> +}
> +
>  static u32 sun50i_efuse_xlate(u32 *speedbin, size_t len)
>  {
>  	u32 efuse_value = 0;
> @@ -46,6 +73,10 @@ static u32 sun50i_efuse_xlate(u32 *speedbin, size_t len)
>  		return 0;
>  }
>  
> +struct sunxi_cpufreq_data sun20i_cpufreq_data = {
> +	.efuse_xlate = sun20i_efuse_xlate,
> +};
> +
>  struct sunxi_cpufreq_data sun50i_cpufreq_data = {
>  	.efuse_xlate = sun50i_efuse_xlate,
>  };
> @@ -54,6 +85,9 @@ static const struct of_device_id cpu_opp_match_list[] = {
>  	{ .compatible = "allwinner,sun50i-h6-operating-points",
>  	  .data = &sun50i_cpufreq_data,
>  	},
> +	{ .compatible = "allwinner,sun20i-d1-operating-points",
> +	  .data = &sun20i_cpufreq_data,
> +	},
>  	{}
>  };
>  

Re: [RFC PATCH v2 2/3] cpufreq: sun50i: Add support for D1's speed bin decoding

[Andre Przywara]

On Thu, 21 Dec 2023 11:10:12 +0100
Brandon Cheo Fusi <fusibrandon13@gmail.com> wrote:

Hi Brandon,

thanks for the quick turnaround, and for splitting this code up, that
makes reasoning about this much easier!

> Adds support for decoding the efuse value read from D1 efuse speed
> bins, and factors out equivalent code for sun50i.
> 
> The algorithm is gotten from
> 
> https://github.com/Tina-Linux/linux-5.4/blob/master/drivers/cpufreq/sun50i-cpufreq-nvmem.c#L293-L338
> 
> and maps an efuse value to either 0 or 1, with 1 meaning stable at
> a lower supply voltage for the same clock frequency.
> 
> Signed-off-by: Brandon Cheo Fusi <fusibrandon13@gmail.com>
> ---
>  drivers/cpufreq/sun50i-cpufreq-nvmem.c | 34 ++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/drivers/cpufreq/sun50i-cpufreq-nvmem.c b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> index fc509fc49..b1cb95308 100644
> --- a/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> +++ b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> @@ -29,6 +29,33 @@ struct sunxi_cpufreq_data {
>  	u32 (*efuse_xlate)(u32 *speedbin, size_t len);
>  };
>  
> +static u32 sun20i_efuse_xlate(u32 *speedbin, size_t len)

I feel like this prototype can be shortened to:

static u32 sun20i_efuse_xlate(u32 speedbin)

See below.

> +{
> +	u32 ret, efuse_value = 0;
> +	int i;
> +
> +	for (i = 0; i < len; i++)
> +		efuse_value |= ((u32)speedbin[i] << (i * 8));

The cast is not needed. Looking deeper into the original code you linked
to, cell_value[] there is an array of u8, so they assemble a little endian
32-bit integer from *up to* four 8-bit values read from the nvmem.

So I think this code here is wrong, len is the size of the nvmem cells
holding the bin identifier, in *bytes*, so the idea here is to just read
the (lowest) 16 bits (in the D1 case, cf. "reg = <0x00 0x2>;" in the next
patch) from this nvmem cell. Here you are combining two 32-bit words into
efuse_value.

So I think this whole part above is actually not necessary: we are
expecting maximum 32 bits, and nvmem_cell_read() should take care of
masking off unrequested bits, so we get the correct value back already. So
can you try to remove the loop above, and use ...

> +
> +	switch (efuse_value) {

	switch (*speedbin & 0xffff) {

here instead? Or drop the pointer at all, and just use one u32 value, see
the above prototype.

Cheers,
Andre

P.S. This is just a "peephole review" of this patch, I haven't got around
to look at this whole scheme in whole yet, to see if we actually need this
or can simplify this or clean it up.


> +	case 0x5e00:
> +		/* QFN package */
> +		ret = 0;
> +		break;
> +	case 0x5c00:
> +	case 0x7400:
> +		/* QFN package */
> +		ret = 1;
> +		break;
> +	case 0x5000:
> +	default:
> +		/* BGA package */
> +		ret = 0;
> +	}
> +
> +	return ret;
> +}
> +
>  static u32 sun50i_efuse_xlate(u32 *speedbin, size_t len)
>  {
>  	u32 efuse_value = 0;
> @@ -46,6 +73,10 @@ static u32 sun50i_efuse_xlate(u32 *speedbin, size_t len)
>  		return 0;
>  }
>  
> +struct sunxi_cpufreq_data sun20i_cpufreq_data = {
> +	.efuse_xlate = sun20i_efuse_xlate,
> +};
> +
>  struct sunxi_cpufreq_data sun50i_cpufreq_data = {
>  	.efuse_xlate = sun50i_efuse_xlate,
>  };
> @@ -54,6 +85,9 @@ static const struct of_device_id cpu_opp_match_list[] = {
>  	{ .compatible = "allwinner,sun50i-h6-operating-points",
>  	  .data = &sun50i_cpufreq_data,
>  	},
> +	{ .compatible = "allwinner,sun20i-d1-operating-points",
> +	  .data = &sun20i_cpufreq_data,
> +	},
>  	{}
>  };
>  


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [RFC PATCH v2 2/3] cpufreq: sun50i: Add support for D1's speed bin decoding

[Andre Przywara]

On Thu, 21 Dec 2023 11:10:12 +0100
Brandon Cheo Fusi <fusibrandon13@gmail.com> wrote:

Hi Brandon,

thanks for the quick turnaround, and for splitting this code up, that
makes reasoning about this much easier!

> Adds support for decoding the efuse value read from D1 efuse speed
> bins, and factors out equivalent code for sun50i.
> 
> The algorithm is gotten from
> 
> https://github.com/Tina-Linux/linux-5.4/blob/master/drivers/cpufreq/sun50i-cpufreq-nvmem.c#L293-L338
> 
> and maps an efuse value to either 0 or 1, with 1 meaning stable at
> a lower supply voltage for the same clock frequency.
> 
> Signed-off-by: Brandon Cheo Fusi <fusibrandon13@gmail.com>
> ---
>  drivers/cpufreq/sun50i-cpufreq-nvmem.c | 34 ++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/drivers/cpufreq/sun50i-cpufreq-nvmem.c b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> index fc509fc49..b1cb95308 100644
> --- a/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> +++ b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> @@ -29,6 +29,33 @@ struct sunxi_cpufreq_data {
>  	u32 (*efuse_xlate)(u32 *speedbin, size_t len);
>  };
>  
> +static u32 sun20i_efuse_xlate(u32 *speedbin, size_t len)

I feel like this prototype can be shortened to:

static u32 sun20i_efuse_xlate(u32 speedbin)

See below.

> +{
> +	u32 ret, efuse_value = 0;
> +	int i;
> +
> +	for (i = 0; i < len; i++)
> +		efuse_value |= ((u32)speedbin[i] << (i * 8));

The cast is not needed. Looking deeper into the original code you linked
to, cell_value[] there is an array of u8, so they assemble a little endian
32-bit integer from *up to* four 8-bit values read from the nvmem.

So I think this code here is wrong, len is the size of the nvmem cells
holding the bin identifier, in *bytes*, so the idea here is to just read
the (lowest) 16 bits (in the D1 case, cf. "reg = <0x00 0x2>;" in the next
patch) from this nvmem cell. Here you are combining two 32-bit words into
efuse_value.

So I think this whole part above is actually not necessary: we are
expecting maximum 32 bits, and nvmem_cell_read() should take care of
masking off unrequested bits, so we get the correct value back already. So
can you try to remove the loop above, and use ...

> +
> +	switch (efuse_value) {

	switch (*speedbin & 0xffff) {

here instead? Or drop the pointer at all, and just use one u32 value, see
the above prototype.

Cheers,
Andre

P.S. This is just a "peephole review" of this patch, I haven't got around
to look at this whole scheme in whole yet, to see if we actually need this
or can simplify this or clean it up.


> +	case 0x5e00:
> +		/* QFN package */
> +		ret = 0;
> +		break;
> +	case 0x5c00:
> +	case 0x7400:
> +		/* QFN package */
> +		ret = 1;
> +		break;
> +	case 0x5000:
> +	default:
> +		/* BGA package */
> +		ret = 0;
> +	}
> +
> +	return ret;
> +}
> +
>  static u32 sun50i_efuse_xlate(u32 *speedbin, size_t len)
>  {
>  	u32 efuse_value = 0;
> @@ -46,6 +73,10 @@ static u32 sun50i_efuse_xlate(u32 *speedbin, size_t len)
>  		return 0;
>  }
>  
> +struct sunxi_cpufreq_data sun20i_cpufreq_data = {
> +	.efuse_xlate = sun20i_efuse_xlate,
> +};
> +
>  struct sunxi_cpufreq_data sun50i_cpufreq_data = {
>  	.efuse_xlate = sun50i_efuse_xlate,
>  };
> @@ -54,6 +85,9 @@ static const struct of_device_id cpu_opp_match_list[] = {
>  	{ .compatible = "allwinner,sun50i-h6-operating-points",
>  	  .data = &sun50i_cpufreq_data,
>  	},
> +	{ .compatible = "allwinner,sun20i-d1-operating-points",
> +	  .data = &sun20i_cpufreq_data,
> +	},
>  	{}
>  };
>  


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v2 2/3] cpufreq: sun50i: Add support for D1's speed bin decoding

[Brandon Cheo Fusi]

On Thu, Dec 21, 2023 at 1:50 PM Andre Przywara <andre.przywara@arm.com> wrote:
>
> On Thu, 21 Dec 2023 11:10:12 +0100
> Brandon Cheo Fusi <fusibrandon13@gmail.com> wrote:
>
> Hi Brandon,
>
> thanks for the quick turnaround, and for splitting this code up, that
> makes reasoning about this much easier!
>
> > Adds support for decoding the efuse value read from D1 efuse speed
> > bins, and factors out equivalent code for sun50i.
> >
> > The algorithm is gotten from
> >
> > https://github.com/Tina-Linux/linux-5.4/blob/master/drivers/cpufreq/sun50i-cpufreq-nvmem.c#L293-L338
> >
> > and maps an efuse value to either 0 or 1, with 1 meaning stable at
> > a lower supply voltage for the same clock frequency.
> >
> > Signed-off-by: Brandon Cheo Fusi <fusibrandon13@gmail.com>
> > ---
> >  drivers/cpufreq/sun50i-cpufreq-nvmem.c | 34 ++++++++++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> >
> > diff --git a/drivers/cpufreq/sun50i-cpufreq-nvmem.c b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> > index fc509fc49..b1cb95308 100644
> > --- a/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> > +++ b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> > @@ -29,6 +29,33 @@ struct sunxi_cpufreq_data {
> >       u32 (*efuse_xlate)(u32 *speedbin, size_t len);
> >  };
> >
> > +static u32 sun20i_efuse_xlate(u32 *speedbin, size_t len)
>
> I feel like this prototype can be shortened to:
>
> static u32 sun20i_efuse_xlate(u32 speedbin)
>
> See below.
>
> > +{
> > +     u32 ret, efuse_value = 0;
> > +     int i;
> > +
> > +     for (i = 0; i < len; i++)
> > +             efuse_value |= ((u32)speedbin[i] << (i * 8));
>
> The cast is not needed. Looking deeper into the original code you linked
> to, cell_value[] there is an array of u8, so they assemble a little endian
> 32-bit integer from *up to* four 8-bit values read from the nvmem.
>
> So I think this code here is wrong, len is the size of the nvmem cells
> holding the bin identifier, in *bytes*, so the idea here is to just read
> the (lowest) 16 bits (in the D1 case, cf. "reg = <0x00 0x2>;" in the next
> patch) from this nvmem cell. Here you are combining two 32-bit words into

This is true. Not sure though what the 'in the D1 case...' bit means.

> efuse_value.
>
> So I think this whole part above is actually not necessary: we are
> expecting maximum 32 bits, and nvmem_cell_read() should take care of
> masking off unrequested bits, so we get the correct value back already. So
> can you try to remove the loop above, and use ...
>
> > +
> > +     switch (efuse_value) {
>
>         switch (*speedbin & 0xffff) {
>

Shouldn't the bytes in *speedbin be reversed? 

> here instead? Or drop the pointer at all, and just use one u32 value, see
> the above prototype.
>

I was uncomfortable dropping the len parameter, because then each
platform's efuse_xlate would ignore the number of valid bytes actually
read.

> Cheers,
> Andre
>
> P.S. This is just a "peephole review" of this patch, I haven't got around
> to look at this whole scheme in whole yet, to see if we actually need this
> or can simplify this or clean it up.
>
>
> > +     case 0x5e00:
> > +             /* QFN package */
> > +             ret = 0;
> > +             break;
> > +     case 0x5c00:
> > +     case 0x7400:
> > +             /* QFN package */
> > +             ret = 1;
> > +             break;
> > +     case 0x5000:
> > +     default:
> > +             /* BGA package */
> > +             ret = 0;
> > +     }
> > +
> > +     return ret;
> > +}
> > +
> >  static u32 sun50i_efuse_xlate(u32 *speedbin, size_t len)
> >  {
> >       u32 efuse_value = 0;
> > @@ -46,6 +73,10 @@ static u32 sun50i_efuse_xlate(u32 *speedbin, size_t len)
> >               return 0;
> >  }
> >
> > +struct sunxi_cpufreq_data sun20i_cpufreq_data = {
> > +     .efuse_xlate = sun20i_efuse_xlate,
> > +};
> > +
> >  struct sunxi_cpufreq_data sun50i_cpufreq_data = {
> >       .efuse_xlate = sun50i_efuse_xlate,
> >  };
> > @@ -54,6 +85,9 @@ static const struct of_device_id cpu_opp_match_list[] = {
> >       { .compatible = "allwinner,sun50i-h6-operating-points",
> >         .data = &sun50i_cpufreq_data,
> >       },
> > +     { .compatible = "allwinner,sun20i-d1-operating-points",
> > +       .data = &sun20i_cpufreq_data,
> > +     },
> >       {}
> >  };
> >
>

Thank you for reviewing.
Brandon.

Re: [RFC PATCH v2 2/3] cpufreq: sun50i: Add support for D1's speed bin decoding

[Brandon Cheo Fusi]

On Thu, Dec 21, 2023 at 1:50 PM Andre Przywara <andre.przywara@arm.com> wrote:
>
> On Thu, 21 Dec 2023 11:10:12 +0100
> Brandon Cheo Fusi <fusibrandon13@gmail.com> wrote:
>
> Hi Brandon,
>
> thanks for the quick turnaround, and for splitting this code up, that
> makes reasoning about this much easier!
>
> > Adds support for decoding the efuse value read from D1 efuse speed
> > bins, and factors out equivalent code for sun50i.
> >
> > The algorithm is gotten from
> >
> > https://github.com/Tina-Linux/linux-5.4/blob/master/drivers/cpufreq/sun50i-cpufreq-nvmem.c#L293-L338
> >
> > and maps an efuse value to either 0 or 1, with 1 meaning stable at
> > a lower supply voltage for the same clock frequency.
> >
> > Signed-off-by: Brandon Cheo Fusi <fusibrandon13@gmail.com>
> > ---
> >  drivers/cpufreq/sun50i-cpufreq-nvmem.c | 34 ++++++++++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> >
> > diff --git a/drivers/cpufreq/sun50i-cpufreq-nvmem.c b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> > index fc509fc49..b1cb95308 100644
> > --- a/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> > +++ b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> > @@ -29,6 +29,33 @@ struct sunxi_cpufreq_data {
> >       u32 (*efuse_xlate)(u32 *speedbin, size_t len);
> >  };
> >
> > +static u32 sun20i_efuse_xlate(u32 *speedbin, size_t len)
>
> I feel like this prototype can be shortened to:
>
> static u32 sun20i_efuse_xlate(u32 speedbin)
>
> See below.
>
> > +{
> > +     u32 ret, efuse_value = 0;
> > +     int i;
> > +
> > +     for (i = 0; i < len; i++)
> > +             efuse_value |= ((u32)speedbin[i] << (i * 8));
>
> The cast is not needed. Looking deeper into the original code you linked
> to, cell_value[] there is an array of u8, so they assemble a little endian
> 32-bit integer from *up to* four 8-bit values read from the nvmem.
>
> So I think this code here is wrong, len is the size of the nvmem cells
> holding the bin identifier, in *bytes*, so the idea here is to just read
> the (lowest) 16 bits (in the D1 case, cf. "reg = <0x00 0x2>;" in the next
> patch) from this nvmem cell. Here you are combining two 32-bit words into

This is true. Not sure though what the 'in the D1 case...' bit means.

> efuse_value.
>
> So I think this whole part above is actually not necessary: we are
> expecting maximum 32 bits, and nvmem_cell_read() should take care of
> masking off unrequested bits, so we get the correct value back already. So
> can you try to remove the loop above, and use ...
>
> > +
> > +     switch (efuse_value) {
>
>         switch (*speedbin & 0xffff) {
>

Shouldn't the bytes in *speedbin be reversed? 

> here instead? Or drop the pointer at all, and just use one u32 value, see
> the above prototype.
>

I was uncomfortable dropping the len parameter, because then each
platform's efuse_xlate would ignore the number of valid bytes actually
read.

> Cheers,
> Andre
>
> P.S. This is just a "peephole review" of this patch, I haven't got around
> to look at this whole scheme in whole yet, to see if we actually need this
> or can simplify this or clean it up.
>
>
> > +     case 0x5e00:
> > +             /* QFN package */
> > +             ret = 0;
> > +             break;
> > +     case 0x5c00:
> > +     case 0x7400:
> > +             /* QFN package */
> > +             ret = 1;
> > +             break;
> > +     case 0x5000:
> > +     default:
> > +             /* BGA package */
> > +             ret = 0;
> > +     }
> > +
> > +     return ret;
> > +}
> > +
> >  static u32 sun50i_efuse_xlate(u32 *speedbin, size_t len)
> >  {
> >       u32 efuse_value = 0;
> > @@ -46,6 +73,10 @@ static u32 sun50i_efuse_xlate(u32 *speedbin, size_t len)
> >               return 0;
> >  }
> >
> > +struct sunxi_cpufreq_data sun20i_cpufreq_data = {
> > +     .efuse_xlate = sun20i_efuse_xlate,
> > +};
> > +
> >  struct sunxi_cpufreq_data sun50i_cpufreq_data = {
> >       .efuse_xlate = sun50i_efuse_xlate,
> >  };
> > @@ -54,6 +85,9 @@ static const struct of_device_id cpu_opp_match_list[] = {
> >       { .compatible = "allwinner,sun50i-h6-operating-points",
> >         .data = &sun50i_cpufreq_data,
> >       },
> > +     { .compatible = "allwinner,sun20i-d1-operating-points",
> > +       .data = &sun20i_cpufreq_data,
> > +     },
> >       {}
> >  };
> >
>

Thank you for reviewing.
Brandon.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [RFC PATCH v2 2/3] cpufreq: sun50i: Add support for D1's speed bin decoding

[Brandon Cheo Fusi]

On Thu, Dec 21, 2023 at 1:50 PM Andre Przywara <andre.przywara@arm.com> wrote:
>
> On Thu, 21 Dec 2023 11:10:12 +0100
> Brandon Cheo Fusi <fusibrandon13@gmail.com> wrote:
>
> Hi Brandon,
>
> thanks for the quick turnaround, and for splitting this code up, that
> makes reasoning about this much easier!
>
> > Adds support for decoding the efuse value read from D1 efuse speed
> > bins, and factors out equivalent code for sun50i.
> >
> > The algorithm is gotten from
> >
> > https://github.com/Tina-Linux/linux-5.4/blob/master/drivers/cpufreq/sun50i-cpufreq-nvmem.c#L293-L338
> >
> > and maps an efuse value to either 0 or 1, with 1 meaning stable at
> > a lower supply voltage for the same clock frequency.
> >
> > Signed-off-by: Brandon Cheo Fusi <fusibrandon13@gmail.com>
> > ---
> >  drivers/cpufreq/sun50i-cpufreq-nvmem.c | 34 ++++++++++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> >
> > diff --git a/drivers/cpufreq/sun50i-cpufreq-nvmem.c b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> > index fc509fc49..b1cb95308 100644
> > --- a/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> > +++ b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> > @@ -29,6 +29,33 @@ struct sunxi_cpufreq_data {
> >       u32 (*efuse_xlate)(u32 *speedbin, size_t len);
> >  };
> >
> > +static u32 sun20i_efuse_xlate(u32 *speedbin, size_t len)
>
> I feel like this prototype can be shortened to:
>
> static u32 sun20i_efuse_xlate(u32 speedbin)
>
> See below.
>
> > +{
> > +     u32 ret, efuse_value = 0;
> > +     int i;
> > +
> > +     for (i = 0; i < len; i++)
> > +             efuse_value |= ((u32)speedbin[i] << (i * 8));
>
> The cast is not needed. Looking deeper into the original code you linked
> to, cell_value[] there is an array of u8, so they assemble a little endian
> 32-bit integer from *up to* four 8-bit values read from the nvmem.
>
> So I think this code here is wrong, len is the size of the nvmem cells
> holding the bin identifier, in *bytes*, so the idea here is to just read
> the (lowest) 16 bits (in the D1 case, cf. "reg = <0x00 0x2>;" in the next
> patch) from this nvmem cell. Here you are combining two 32-bit words into

This is true. Not sure though what the 'in the D1 case...' bit means.

> efuse_value.
>
> So I think this whole part above is actually not necessary: we are
> expecting maximum 32 bits, and nvmem_cell_read() should take care of
> masking off unrequested bits, so we get the correct value back already. So
> can you try to remove the loop above, and use ...
>
> > +
> > +     switch (efuse_value) {
>
>         switch (*speedbin & 0xffff) {
>

Shouldn't the bytes in *speedbin be reversed? 

> here instead? Or drop the pointer at all, and just use one u32 value, see
> the above prototype.
>

I was uncomfortable dropping the len parameter, because then each
platform's efuse_xlate would ignore the number of valid bytes actually
read.

> Cheers,
> Andre
>
> P.S. This is just a "peephole review" of this patch, I haven't got around
> to look at this whole scheme in whole yet, to see if we actually need this
> or can simplify this or clean it up.
>
>
> > +     case 0x5e00:
> > +             /* QFN package */
> > +             ret = 0;
> > +             break;
> > +     case 0x5c00:
> > +     case 0x7400:
> > +             /* QFN package */
> > +             ret = 1;
> > +             break;
> > +     case 0x5000:
> > +     default:
> > +             /* BGA package */
> > +             ret = 0;
> > +     }
> > +
> > +     return ret;
> > +}
> > +
> >  static u32 sun50i_efuse_xlate(u32 *speedbin, size_t len)
> >  {
> >       u32 efuse_value = 0;
> > @@ -46,6 +73,10 @@ static u32 sun50i_efuse_xlate(u32 *speedbin, size_t len)
> >               return 0;
> >  }
> >
> > +struct sunxi_cpufreq_data sun20i_cpufreq_data = {
> > +     .efuse_xlate = sun20i_efuse_xlate,
> > +};
> > +
> >  struct sunxi_cpufreq_data sun50i_cpufreq_data = {
> >       .efuse_xlate = sun50i_efuse_xlate,
> >  };
> > @@ -54,6 +85,9 @@ static const struct of_device_id cpu_opp_match_list[] = {
> >       { .compatible = "allwinner,sun50i-h6-operating-points",
> >         .data = &sun50i_cpufreq_data,
> >       },
> > +     { .compatible = "allwinner,sun20i-d1-operating-points",
> > +       .data = &sun20i_cpufreq_data,
> > +     },
> >       {}
> >  };
> >
>

Thank you for reviewing.
Brandon.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH v2 2/3] cpufreq: sun50i: Add support for D1's speed bin decoding

[Andre Przywara]

On Thu, 21 Dec 2023 18:11:07 +0100
Brandon Cheo Fusi <fusibrandon13@gmail.com> wrote:

Hi Brandon,

> On Thu, Dec 21, 2023 at 1:50 PM Andre Przywara <andre.przywara@arm.com> wrote:
> >
> > On Thu, 21 Dec 2023 11:10:12 +0100
> > Brandon Cheo Fusi <fusibrandon13@gmail.com> wrote:
> >
> > Hi Brandon,
> >
> > thanks for the quick turnaround, and for splitting this code up, that
> > makes reasoning about this much easier!
> >  
> > > Adds support for decoding the efuse value read from D1 efuse speed
> > > bins, and factors out equivalent code for sun50i.
> > >
> > > The algorithm is gotten from
> > >
> > > https://github.com/Tina-Linux/linux-5.4/blob/master/drivers/cpufreq/sun50i-cpufreq-nvmem.c#L293-L338
> > >
> > > and maps an efuse value to either 0 or 1, with 1 meaning stable at
> > > a lower supply voltage for the same clock frequency.
> > >
> > > Signed-off-by: Brandon Cheo Fusi <fusibrandon13@gmail.com>
> > > ---
> > >  drivers/cpufreq/sun50i-cpufreq-nvmem.c | 34 ++++++++++++++++++++++++++
> > >  1 file changed, 34 insertions(+)
> > >
> > > diff --git a/drivers/cpufreq/sun50i-cpufreq-nvmem.c b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> > > index fc509fc49..b1cb95308 100644
> > > --- a/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> > > +++ b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> > > @@ -29,6 +29,33 @@ struct sunxi_cpufreq_data {
> > >       u32 (*efuse_xlate)(u32 *speedbin, size_t len);
> > >  };
> > >
> > > +static u32 sun20i_efuse_xlate(u32 *speedbin, size_t len)  
> >
> > I feel like this prototype can be shortened to:
> >
> > static u32 sun20i_efuse_xlate(u32 speedbin)
> >
> > See below.
> >  
> > > +{
> > > +     u32 ret, efuse_value = 0;
> > > +     int i;
> > > +
> > > +     for (i = 0; i < len; i++)
> > > +             efuse_value |= ((u32)speedbin[i] << (i * 8));  
> >
> > The cast is not needed. Looking deeper into the original code you linked
> > to, cell_value[] there is an array of u8, so they assemble a little endian
> > 32-bit integer from *up to* four 8-bit values read from the nvmem.
> >
> > So I think this code here is wrong, len is the size of the nvmem cells
> > holding the bin identifier, in *bytes*, so the idea here is to just read
> > the (lowest) 16 bits (in the D1 case, cf. "reg = <0x00 0x2>;" in the next
> > patch) from this nvmem cell. Here you are combining two 32-bit words into  
> 
> This is true. Not sure though what the 'in the D1 case...' bit means.

In the next patch you introduce the nvmem DT property, and set the length
part to "0x2". So for the D1 we will always read two bytes.

> > efuse_value.
> >
> > So I think this whole part above is actually not necessary: we are
> > expecting maximum 32 bits, and nvmem_cell_read() should take care of
> > masking off unrequested bits, so we get the correct value back already. So
> > can you try to remove the loop above, and use ...
> >  
> > > +
> > > +     switch (efuse_value) {  
> >
> >         switch (*speedbin & 0xffff) {
> >  
> 
> Shouldn't the bytes in *speedbin be reversed? 

I believe they are stored as a little endian 16-bit integer in the fuses.
I haven't tried a BE kernel, but I think the NVMEM framework takes care of
that.
If you dump the values as returned by nvmem_cell_read(), we would know for
sure.

> > here instead? Or drop the pointer at all, and just use one u32 value, see
> > the above prototype.
> >  
> 
> I was uncomfortable dropping the len parameter, because then each
> platform's efuse_xlate would ignore the number of valid bytes actually
> read.

Well, I am not sure either, but neither the H6, nor the H616 or the D1
apparently really need that: they all use either 4 or 2 bytes to encode
the speed bin. And since the routines are SoC specific anyway, and the
first 32-bit word of the buffer filled by nvmem_cell_read() should always
be valid (and be it 0), I think there is little need to check that.
I ported the H616 code over, and it looks somewhat similar to the D1 (with
different numbers, though): it's (ab)using some die revision code (the
first two bytes in the SID) to derive the speed bin. The H6 had a
dedicated bin fuse.

So iff we are going to see a SoC needing to check the length, we can always
introduce that later: it's just an internal function.
But for now I'd like to keep it simple.

Cheers,
Andre

> 
> > Cheers,
> > Andre
> >
> > P.S. This is just a "peephole review" of this patch, I haven't got around
> > to look at this whole scheme in whole yet, to see if we actually need this
> > or can simplify this or clean it up.
> >
> >  
> > > +     case 0x5e00:
> > > +             /* QFN package */
> > > +             ret = 0;
> > > +             break;
> > > +     case 0x5c00:
> > > +     case 0x7400:
> > > +             /* QFN package */
> > > +             ret = 1;
> > > +             break;
> > > +     case 0x5000:
> > > +     default:
> > > +             /* BGA package */
> > > +             ret = 0;
> > > +     }
> > > +
> > > +     return ret;
> > > +}
> > > +
> > >  static u32 sun50i_efuse_xlate(u32 *speedbin, size_t len)
> > >  {
> > >       u32 efuse_value = 0;
> > > @@ -46,6 +73,10 @@ static u32 sun50i_efuse_xlate(u32 *speedbin, size_t len)
> > >               return 0;
> > >  }
> > >
> > > +struct sunxi_cpufreq_data sun20i_cpufreq_data = {
> > > +     .efuse_xlate = sun20i_efuse_xlate,
> > > +};
> > > +
> > >  struct sunxi_cpufreq_data sun50i_cpufreq_data = {
> > >       .efuse_xlate = sun50i_efuse_xlate,
> > >  };
> > > @@ -54,6 +85,9 @@ static const struct of_device_id cpu_opp_match_list[] = {
> > >       { .compatible = "allwinner,sun50i-h6-operating-points",
> > >         .data = &sun50i_cpufreq_data,
> > >       },
> > > +     { .compatible = "allwinner,sun20i-d1-operating-points",
> > > +       .data = &sun20i_cpufreq_data,
> > > +     },
> > >       {}
> > >  };
> > >  
> >  
> 
> Thank you for reviewing.
> Brandon.
> 

Re: [RFC PATCH v2 2/3] cpufreq: sun50i: Add support for D1's speed bin decoding

[Andre Przywara]

On Thu, 21 Dec 2023 18:11:07 +0100
Brandon Cheo Fusi <fusibrandon13@gmail.com> wrote:

Hi Brandon,

> On Thu, Dec 21, 2023 at 1:50 PM Andre Przywara <andre.przywara@arm.com> wrote:
> >
> > On Thu, 21 Dec 2023 11:10:12 +0100
> > Brandon Cheo Fusi <fusibrandon13@gmail.com> wrote:
> >
> > Hi Brandon,
> >
> > thanks for the quick turnaround, and for splitting this code up, that
> > makes reasoning about this much easier!
> >  
> > > Adds support for decoding the efuse value read from D1 efuse speed
> > > bins, and factors out equivalent code for sun50i.
> > >
> > > The algorithm is gotten from
> > >
> > > https://github.com/Tina-Linux/linux-5.4/blob/master/drivers/cpufreq/sun50i-cpufreq-nvmem.c#L293-L338
> > >
> > > and maps an efuse value to either 0 or 1, with 1 meaning stable at
> > > a lower supply voltage for the same clock frequency.
> > >
> > > Signed-off-by: Brandon Cheo Fusi <fusibrandon13@gmail.com>
> > > ---
> > >  drivers/cpufreq/sun50i-cpufreq-nvmem.c | 34 ++++++++++++++++++++++++++
> > >  1 file changed, 34 insertions(+)
> > >
> > > diff --git a/drivers/cpufreq/sun50i-cpufreq-nvmem.c b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> > > index fc509fc49..b1cb95308 100644
> > > --- a/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> > > +++ b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> > > @@ -29,6 +29,33 @@ struct sunxi_cpufreq_data {
> > >       u32 (*efuse_xlate)(u32 *speedbin, size_t len);
> > >  };
> > >
> > > +static u32 sun20i_efuse_xlate(u32 *speedbin, size_t len)  
> >
> > I feel like this prototype can be shortened to:
> >
> > static u32 sun20i_efuse_xlate(u32 speedbin)
> >
> > See below.
> >  
> > > +{
> > > +     u32 ret, efuse_value = 0;
> > > +     int i;
> > > +
> > > +     for (i = 0; i < len; i++)
> > > +             efuse_value |= ((u32)speedbin[i] << (i * 8));  
> >
> > The cast is not needed. Looking deeper into the original code you linked
> > to, cell_value[] there is an array of u8, so they assemble a little endian
> > 32-bit integer from *up to* four 8-bit values read from the nvmem.
> >
> > So I think this code here is wrong, len is the size of the nvmem cells
> > holding the bin identifier, in *bytes*, so the idea here is to just read
> > the (lowest) 16 bits (in the D1 case, cf. "reg = <0x00 0x2>;" in the next
> > patch) from this nvmem cell. Here you are combining two 32-bit words into  
> 
> This is true. Not sure though what the 'in the D1 case...' bit means.

In the next patch you introduce the nvmem DT property, and set the length
part to "0x2". So for the D1 we will always read two bytes.

> > efuse_value.
> >
> > So I think this whole part above is actually not necessary: we are
> > expecting maximum 32 bits, and nvmem_cell_read() should take care of
> > masking off unrequested bits, so we get the correct value back already. So
> > can you try to remove the loop above, and use ...
> >  
> > > +
> > > +     switch (efuse_value) {  
> >
> >         switch (*speedbin & 0xffff) {
> >  
> 
> Shouldn't the bytes in *speedbin be reversed? 

I believe they are stored as a little endian 16-bit integer in the fuses.
I haven't tried a BE kernel, but I think the NVMEM framework takes care of
that.
If you dump the values as returned by nvmem_cell_read(), we would know for
sure.

> > here instead? Or drop the pointer at all, and just use one u32 value, see
> > the above prototype.
> >  
> 
> I was uncomfortable dropping the len parameter, because then each
> platform's efuse_xlate would ignore the number of valid bytes actually
> read.

Well, I am not sure either, but neither the H6, nor the H616 or the D1
apparently really need that: they all use either 4 or 2 bytes to encode
the speed bin. And since the routines are SoC specific anyway, and the
first 32-bit word of the buffer filled by nvmem_cell_read() should always
be valid (and be it 0), I think there is little need to check that.
I ported the H616 code over, and it looks somewhat similar to the D1 (with
different numbers, though): it's (ab)using some die revision code (the
first two bytes in the SID) to derive the speed bin. The H6 had a
dedicated bin fuse.

So iff we are going to see a SoC needing to check the length, we can always
introduce that later: it's just an internal function.
But for now I'd like to keep it simple.

Cheers,
Andre

> 
> > Cheers,
> > Andre
> >
> > P.S. This is just a "peephole review" of this patch, I haven't got around
> > to look at this whole scheme in whole yet, to see if we actually need this
> > or can simplify this or clean it up.
> >
> >  
> > > +     case 0x5e00:
> > > +             /* QFN package */
> > > +             ret = 0;
> > > +             break;
> > > +     case 0x5c00:
> > > +     case 0x7400:
> > > +             /* QFN package */
> > > +             ret = 1;
> > > +             break;
> > > +     case 0x5000:
> > > +     default:
> > > +             /* BGA package */
> > > +             ret = 0;
> > > +     }
> > > +
> > > +     return ret;
> > > +}
> > > +
> > >  static u32 sun50i_efuse_xlate(u32 *speedbin, size_t len)
> > >  {
> > >       u32 efuse_value = 0;
> > > @@ -46,6 +73,10 @@ static u32 sun50i_efuse_xlate(u32 *speedbin, size_t len)
> > >               return 0;
> > >  }
> > >
> > > +struct sunxi_cpufreq_data sun20i_cpufreq_data = {
> > > +     .efuse_xlate = sun20i_efuse_xlate,
> > > +};
> > > +
> > >  struct sunxi_cpufreq_data sun50i_cpufreq_data = {
> > >       .efuse_xlate = sun50i_efuse_xlate,
> > >  };
> > > @@ -54,6 +85,9 @@ static const struct of_device_id cpu_opp_match_list[] = {
> > >       { .compatible = "allwinner,sun50i-h6-operating-points",
> > >         .data = &sun50i_cpufreq_data,
> > >       },
> > > +     { .compatible = "allwinner,sun20i-d1-operating-points",
> > > +       .data = &sun20i_cpufreq_data,
> > > +     },
> > >       {}
> > >  };
> > >  
> >  
> 
> Thank you for reviewing.
> Brandon.
> 


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [RFC PATCH v2 2/3] cpufreq: sun50i: Add support for D1's speed bin decoding

[Andre Przywara]

On Thu, 21 Dec 2023 18:11:07 +0100
Brandon Cheo Fusi <fusibrandon13@gmail.com> wrote:

Hi Brandon,

> On Thu, Dec 21, 2023 at 1:50 PM Andre Przywara <andre.przywara@arm.com> wrote:
> >
> > On Thu, 21 Dec 2023 11:10:12 +0100
> > Brandon Cheo Fusi <fusibrandon13@gmail.com> wrote:
> >
> > Hi Brandon,
> >
> > thanks for the quick turnaround, and for splitting this code up, that
> > makes reasoning about this much easier!
> >  
> > > Adds support for decoding the efuse value read from D1 efuse speed
> > > bins, and factors out equivalent code for sun50i.
> > >
> > > The algorithm is gotten from
> > >
> > > https://github.com/Tina-Linux/linux-5.4/blob/master/drivers/cpufreq/sun50i-cpufreq-nvmem.c#L293-L338
> > >
> > > and maps an efuse value to either 0 or 1, with 1 meaning stable at
> > > a lower supply voltage for the same clock frequency.
> > >
> > > Signed-off-by: Brandon Cheo Fusi <fusibrandon13@gmail.com>
> > > ---
> > >  drivers/cpufreq/sun50i-cpufreq-nvmem.c | 34 ++++++++++++++++++++++++++
> > >  1 file changed, 34 insertions(+)
> > >
> > > diff --git a/drivers/cpufreq/sun50i-cpufreq-nvmem.c b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> > > index fc509fc49..b1cb95308 100644
> > > --- a/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> > > +++ b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> > > @@ -29,6 +29,33 @@ struct sunxi_cpufreq_data {
> > >       u32 (*efuse_xlate)(u32 *speedbin, size_t len);
> > >  };
> > >
> > > +static u32 sun20i_efuse_xlate(u32 *speedbin, size_t len)  
> >
> > I feel like this prototype can be shortened to:
> >
> > static u32 sun20i_efuse_xlate(u32 speedbin)
> >
> > See below.
> >  
> > > +{
> > > +     u32 ret, efuse_value = 0;
> > > +     int i;
> > > +
> > > +     for (i = 0; i < len; i++)
> > > +             efuse_value |= ((u32)speedbin[i] << (i * 8));  
> >
> > The cast is not needed. Looking deeper into the original code you linked
> > to, cell_value[] there is an array of u8, so they assemble a little endian
> > 32-bit integer from *up to* four 8-bit values read from the nvmem.
> >
> > So I think this code here is wrong, len is the size of the nvmem cells
> > holding the bin identifier, in *bytes*, so the idea here is to just read
> > the (lowest) 16 bits (in the D1 case, cf. "reg = <0x00 0x2>;" in the next
> > patch) from this nvmem cell. Here you are combining two 32-bit words into  
> 
> This is true. Not sure though what the 'in the D1 case...' bit means.

In the next patch you introduce the nvmem DT property, and set the length
part to "0x2". So for the D1 we will always read two bytes.

> > efuse_value.
> >
> > So I think this whole part above is actually not necessary: we are
> > expecting maximum 32 bits, and nvmem_cell_read() should take care of
> > masking off unrequested bits, so we get the correct value back already. So
> > can you try to remove the loop above, and use ...
> >  
> > > +
> > > +     switch (efuse_value) {  
> >
> >         switch (*speedbin & 0xffff) {
> >  
> 
> Shouldn't the bytes in *speedbin be reversed? 

I believe they are stored as a little endian 16-bit integer in the fuses.
I haven't tried a BE kernel, but I think the NVMEM framework takes care of
that.
If you dump the values as returned by nvmem_cell_read(), we would know for
sure.

> > here instead? Or drop the pointer at all, and just use one u32 value, see
> > the above prototype.
> >  
> 
> I was uncomfortable dropping the len parameter, because then each
> platform's efuse_xlate would ignore the number of valid bytes actually
> read.

Well, I am not sure either, but neither the H6, nor the H616 or the D1
apparently really need that: they all use either 4 or 2 bytes to encode
the speed bin. And since the routines are SoC specific anyway, and the
first 32-bit word of the buffer filled by nvmem_cell_read() should always
be valid (and be it 0), I think there is little need to check that.
I ported the H616 code over, and it looks somewhat similar to the D1 (with
different numbers, though): it's (ab)using some die revision code (the
first two bytes in the SID) to derive the speed bin. The H6 had a
dedicated bin fuse.

So iff we are going to see a SoC needing to check the length, we can always
introduce that later: it's just an internal function.
But for now I'd like to keep it simple.

Cheers,
Andre

> 
> > Cheers,
> > Andre
> >
> > P.S. This is just a "peephole review" of this patch, I haven't got around
> > to look at this whole scheme in whole yet, to see if we actually need this
> > or can simplify this or clean it up.
> >
> >  
> > > +     case 0x5e00:
> > > +             /* QFN package */
> > > +             ret = 0;
> > > +             break;
> > > +     case 0x5c00:
> > > +     case 0x7400:
> > > +             /* QFN package */
> > > +             ret = 1;
> > > +             break;
> > > +     case 0x5000:
> > > +     default:
> > > +             /* BGA package */
> > > +             ret = 0;
> > > +     }
> > > +
> > > +     return ret;
> > > +}
> > > +
> > >  static u32 sun50i_efuse_xlate(u32 *speedbin, size_t len)
> > >  {
> > >       u32 efuse_value = 0;
> > > @@ -46,6 +73,10 @@ static u32 sun50i_efuse_xlate(u32 *speedbin, size_t len)
> > >               return 0;
> > >  }
> > >
> > > +struct sunxi_cpufreq_data sun20i_cpufreq_data = {
> > > +     .efuse_xlate = sun20i_efuse_xlate,
> > > +};
> > > +
> > >  struct sunxi_cpufreq_data sun50i_cpufreq_data = {
> > >       .efuse_xlate = sun50i_efuse_xlate,
> > >  };
> > > @@ -54,6 +85,9 @@ static const struct of_device_id cpu_opp_match_list[] = {
> > >       { .compatible = "allwinner,sun50i-h6-operating-points",
> > >         .data = &sun50i_cpufreq_data,
> > >       },
> > > +     { .compatible = "allwinner,sun20i-d1-operating-points",
> > > +       .data = &sun20i_cpufreq_data,
> > > +     },
> > >       {}
> > >  };
> > >  
> >  
> 
> Thank you for reviewing.
> Brandon.
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel