diff mbox series

[v5,3/4] cpufreq: qcom-nvmem: add support for IPQ8064

Message ID 20230930102218.229613-3-robimarko@gmail.com
State New
Headers
Series [v5,1/4] cpufreq: qcom-nvmem: add support for IPQ8074 |

Commit Message

Robert Marko Sept. 30, 2023, 10:21 a.m. UTC 
  From: Christian Marangi <ansuelsmth@gmail.com>

IPQ8064 comes in 3 families:
* IPQ8062 up to 1.0GHz
* IPQ8064/IPQ8066/IPQ8068 up to 1.4GHz
* IPQ8065/IPQ8069 up to 1.7Ghz

So, in order to be able to support one OPP table, add support for
IPQ8064 family based of SMEM SoC ID-s and correctly set the version so
opp-supported-hw can be correctly used.

Bit are set with the following logic:
* IPQ8062 BIT 0
* IPQ8064/IPQ8066/IPQ8068 BIT 1
* IPQ8065/IPQ8069 BIT 2

speed is never fused, only pvs values are fused.

IPQ806x SoC doesn't have pvs_version so we drop and we use the new
pattern:
opp-microvolt-speed0-pvs<PSV_VALUE>

Example:
- for ipq8062 psv2
  opp-microvolt-speed0-pvs2 = < 925000 878750 971250>

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
Signed-off-by: Robert Marko <robimarko@gmail.com>
---
Changes in v4:
* Free speedbin in case of an error

Changes in v3:
* Use enum for SoC version
* Dont evaluate speed as its not fused, only pvs

Changes in v2:
* Include IPQ8064 support

 drivers/cpufreq/qcom-cpufreq-nvmem.c | 68 +++++++++++++++++++++++++++-
 1 file changed, 67 insertions(+), 1 deletion(-)

Comments

Konrad Dybcio Oct. 10, 2023, 1:39 p.m. UTC | #1 
On 9/30/23 12:21, Robert Marko wrote:
> From: Christian Marangi <ansuelsmth@gmail.com>
> 
> IPQ8064 comes in 3 families:
> * IPQ8062 up to 1.0GHz
> * IPQ8064/IPQ8066/IPQ8068 up to 1.4GHz
> * IPQ8065/IPQ8069 up to 1.7Ghz
> 
> So, in order to be able to support one OPP table, add support for
> IPQ8064 family based of SMEM SoC ID-s and correctly set the version so
> opp-supported-hw can be correctly used.
> 
> Bit are set with the following logic:
> * IPQ8062 BIT 0
> * IPQ8064/IPQ8066/IPQ8068 BIT 1
> * IPQ8065/IPQ8069 BIT 2
> 
> speed is never fused, only pvs values are fused.
> 
> IPQ806x SoC doesn't have pvs_version so we drop and we use the new
> pattern:
> opp-microvolt-speed0-pvs<PSV_VALUE>
> 
> Example:
> - for ipq8062 psv2
>    opp-microvolt-speed0-pvs2 = < 925000 878750 971250>
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> Signed-off-by: Robert Marko <robimarko@gmail.com>
> ---
[...]

> +{
> +	int speed = 0, pvs = 0, pvs_ver = 0;
> +	int msm_id, ret = 0;
> +	u8 *speedbin;
> +	size_t len;
> +
> +	speedbin = nvmem_cell_read(speedbin_nvmem, &len);
> +
> +	if (IS_ERR(speedbin))
The stray newline above this line triggers my OCD :D

> +		return PTR_ERR(speedbin);
> +
> +	if (len != 4) {
> +		dev_err(cpu_dev, "Unable to read nvmem data. Defaulting to 0!\n");
> +		kfree(speedbin);
> +		return -ENODEV;
> +	}
> +
> +	get_krait_bin_format_a(cpu_dev, &speed, &pvs, &pvs_ver, speedbin);
> +
> +	ret = qcom_smem_get_soc_id(&msm_id);
> +	if (ret)
> +		return ret;
speedbin leaks here

you can free it right after the get_krait.. call
> +
> +	switch (msm_id) {
> +	case QCOM_ID_IPQ8062:
> +		drv->versions = BIT(IPQ8062_VERSION);
> +		break;
> +	case QCOM_ID_IPQ8064:
> +	case QCOM_ID_IPQ8066:
> +	case QCOM_ID_IPQ8068:
> +		drv->versions = BIT(IPQ8064_VERSION);
> +		break;
> +	case QCOM_ID_IPQ8065:
> +	case QCOM_ID_IPQ8069:
> +		drv->versions = BIT(IPQ8065_VERSION);
> +		break;
> +	default:
> +		dev_err(cpu_dev,
> +			"SoC ID %u is not part of IPQ8064 family, limiting to 1.0GHz!\n",
> +			msm_id);
> +		drv->versions = BIT(IPQ8062_VERSION);
> +		break;
> +	}
> +
> +	/* IPQ8064 speed is never fused. Only pvs values are fused. */
> +	snprintf(*pvs_name, sizeof("speedXX-pvsXX"), "speed%d-pvs%d",
> +		 speed, pvs);
Then drop the format for `speed` and just throw in a zero!

[...]

> -	{ .compatible = "qcom,ipq8064", .data = &match_data_krait },
> +	{ .compatible = "qcom,ipq8064", .data = &match_data_ipq8064 },
This change demands a Fixes tag, because you're essentially saying "the 
support for this SoC was supposedly there, but it could have never 
worked and was broken all along".

Konrad
Christian Marangi Oct. 10, 2023, 2:08 p.m. UTC | #2 
On Tue, Oct 10, 2023 at 03:39:54PM +0200, Konrad Dybcio wrote:
> 
> 
> On 9/30/23 12:21, Robert Marko wrote:
> > From: Christian Marangi <ansuelsmth@gmail.com>
> > 
> > IPQ8064 comes in 3 families:
> > * IPQ8062 up to 1.0GHz
> > * IPQ8064/IPQ8066/IPQ8068 up to 1.4GHz
> > * IPQ8065/IPQ8069 up to 1.7Ghz
> > 
> > So, in order to be able to support one OPP table, add support for
> > IPQ8064 family based of SMEM SoC ID-s and correctly set the version so
> > opp-supported-hw can be correctly used.
> > 
> > Bit are set with the following logic:
> > * IPQ8062 BIT 0
> > * IPQ8064/IPQ8066/IPQ8068 BIT 1
> > * IPQ8065/IPQ8069 BIT 2
> > 
> > speed is never fused, only pvs values are fused.
> > 
> > IPQ806x SoC doesn't have pvs_version so we drop and we use the new
> > pattern:
> > opp-microvolt-speed0-pvs<PSV_VALUE>
> > 
> > Example:
> > - for ipq8062 psv2
> >    opp-microvolt-speed0-pvs2 = < 925000 878750 971250>
> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > Signed-off-by: Robert Marko <robimarko@gmail.com>
> > ---
> [...]
> 
> > +{
> > +	int speed = 0, pvs = 0, pvs_ver = 0;
> > +	int msm_id, ret = 0;
> > +	u8 *speedbin;
> > +	size_t len;
> > +
> > +	speedbin = nvmem_cell_read(speedbin_nvmem, &len);
> > +
> > +	if (IS_ERR(speedbin))
> The stray newline above this line triggers my OCD :D
> 
> > +		return PTR_ERR(speedbin);
> > +
> > +	if (len != 4) {
> > +		dev_err(cpu_dev, "Unable to read nvmem data. Defaulting to 0!\n");
> > +		kfree(speedbin);
> > +		return -ENODEV;
> > +	}
> > +
> > +	get_krait_bin_format_a(cpu_dev, &speed, &pvs, &pvs_ver, speedbin);
> > +
> > +	ret = qcom_smem_get_soc_id(&msm_id);
> > +	if (ret)
> > +		return ret;
> speedbin leaks here
> 
> you can free it right after the get_krait.. call
> > +
> > +	switch (msm_id) {
> > +	case QCOM_ID_IPQ8062:
> > +		drv->versions = BIT(IPQ8062_VERSION);
> > +		break;
> > +	case QCOM_ID_IPQ8064:
> > +	case QCOM_ID_IPQ8066:
> > +	case QCOM_ID_IPQ8068:
> > +		drv->versions = BIT(IPQ8064_VERSION);
> > +		break;
> > +	case QCOM_ID_IPQ8065:
> > +	case QCOM_ID_IPQ8069:
> > +		drv->versions = BIT(IPQ8065_VERSION);
> > +		break;
> > +	default:
> > +		dev_err(cpu_dev,
> > +			"SoC ID %u is not part of IPQ8064 family, limiting to 1.0GHz!\n",
> > +			msm_id);
> > +		drv->versions = BIT(IPQ8062_VERSION);
> > +		break;
> > +	}
> > +
> > +	/* IPQ8064 speed is never fused. Only pvs values are fused. */
> > +	snprintf(*pvs_name, sizeof("speedXX-pvsXX"), "speed%d-pvs%d",
> > +		 speed, pvs);
> Then drop the format for `speed` and just throw in a zero!
> 
> [...]
> 
> > -	{ .compatible = "qcom,ipq8064", .data = &match_data_krait },
> > +	{ .compatible = "qcom,ipq8064", .data = &match_data_ipq8064 },
> This change demands a Fixes tag, because you're essentially saying "the
> support for this SoC was supposedly there, but it could have never worked
> and was broken all along".
>

Mhhh actually no. We are just changing the opp binding and introducing
hardcoded versions. But the thing worked and actually it's what was used
before this change in openwrt. Also current ipq806x dtsi doesn't have
any opp definition so no regression there. (and also 99% downstream either
use openwrt or use qcom sdk where this implementation is not used at
all)

Given these thing should we still add a fixes tag referencing the commit
that introduced the compatible for qcom,ipq8064? It's quite problematic
as this depends on qcom_smem_get_soc_id().
Konrad Dybcio Oct. 10, 2023, 7:26 p.m. UTC | #3 
On 10/10/23 16:08, Christian Marangi wrote:
> On Tue, Oct 10, 2023 at 03:39:54PM +0200, Konrad Dybcio wrote:
>>
>>
>> On 9/30/23 12:21, Robert Marko wrote:
>>> From: Christian Marangi <ansuelsmth@gmail.com>
>>>
>>> IPQ8064 comes in 3 families:
>>> * IPQ8062 up to 1.0GHz
>>> * IPQ8064/IPQ8066/IPQ8068 up to 1.4GHz
>>> * IPQ8065/IPQ8069 up to 1.7Ghz
>>>
>>> So, in order to be able to support one OPP table, add support for
>>> IPQ8064 family based of SMEM SoC ID-s and correctly set the version so
>>> opp-supported-hw can be correctly used.
>>>
>>> Bit are set with the following logic:
>>> * IPQ8062 BIT 0
>>> * IPQ8064/IPQ8066/IPQ8068 BIT 1
>>> * IPQ8065/IPQ8069 BIT 2
>>>
>>> speed is never fused, only pvs values are fused.
>>>
>>> IPQ806x SoC doesn't have pvs_version so we drop and we use the new
>>> pattern:
>>> opp-microvolt-speed0-pvs<PSV_VALUE>
>>>
>>> Example:
>>> - for ipq8062 psv2
>>>     opp-microvolt-speed0-pvs2 = < 925000 878750 971250>
>>>
>>> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
>>> Signed-off-by: Robert Marko <robimarko@gmail.com>
>>> ---
>> [...]
>>
>>> +{
>>> +	int speed = 0, pvs = 0, pvs_ver = 0;
>>> +	int msm_id, ret = 0;
>>> +	u8 *speedbin;
>>> +	size_t len;
>>> +
>>> +	speedbin = nvmem_cell_read(speedbin_nvmem, &len);
>>> +
>>> +	if (IS_ERR(speedbin))
>> The stray newline above this line triggers my OCD :D
>>
>>> +		return PTR_ERR(speedbin);
>>> +
>>> +	if (len != 4) {
>>> +		dev_err(cpu_dev, "Unable to read nvmem data. Defaulting to 0!\n");
>>> +		kfree(speedbin);
>>> +		return -ENODEV;
>>> +	}
>>> +
>>> +	get_krait_bin_format_a(cpu_dev, &speed, &pvs, &pvs_ver, speedbin);
>>> +
>>> +	ret = qcom_smem_get_soc_id(&msm_id);
>>> +	if (ret)
>>> +		return ret;
>> speedbin leaks here
>>
>> you can free it right after the get_krait.. call
>>> +
>>> +	switch (msm_id) {
>>> +	case QCOM_ID_IPQ8062:
>>> +		drv->versions = BIT(IPQ8062_VERSION);
>>> +		break;
>>> +	case QCOM_ID_IPQ8064:
>>> +	case QCOM_ID_IPQ8066:
>>> +	case QCOM_ID_IPQ8068:
>>> +		drv->versions = BIT(IPQ8064_VERSION);
>>> +		break;
>>> +	case QCOM_ID_IPQ8065:
>>> +	case QCOM_ID_IPQ8069:
>>> +		drv->versions = BIT(IPQ8065_VERSION);
>>> +		break;
>>> +	default:
>>> +		dev_err(cpu_dev,
>>> +			"SoC ID %u is not part of IPQ8064 family, limiting to 1.0GHz!\n",
>>> +			msm_id);
>>> +		drv->versions = BIT(IPQ8062_VERSION);
>>> +		break;
>>> +	}
>>> +
>>> +	/* IPQ8064 speed is never fused. Only pvs values are fused. */
>>> +	snprintf(*pvs_name, sizeof("speedXX-pvsXX"), "speed%d-pvs%d",
>>> +		 speed, pvs);
>> Then drop the format for `speed` and just throw in a zero!
>>
>> [...]
>>
>>> -	{ .compatible = "qcom,ipq8064", .data = &match_data_krait },
>>> +	{ .compatible = "qcom,ipq8064", .data = &match_data_ipq8064 },
>> This change demands a Fixes tag, because you're essentially saying "the
>> support for this SoC was supposedly there, but it could have never worked
>> and was broken all along".
>>
> 
> Mhhh actually no. We are just changing the opp binding and introducing
> hardcoded versions. But the thing worked and actually it's what was used
> before this change in openwrt. Also current ipq806x dtsi doesn't have
> any opp definition so no regression there. (and also 99% downstream either
> use openwrt or use qcom sdk where this implementation is not used at
> all)
> 
> Given these thing should we still add a fixes tag referencing the commit
> that introduced the compatible for qcom,ipq8064? It's quite problematic
> as this depends on qcom_smem_get_soc_id().
Fixes only hints auto backports, you shouldn't be worried about putting 
fixes on commits that fix bugs.

I see this as a "didnt work" -> "works" commit, which in my eyes 
qualifies as a fix.

Konrad
diff mbox series

Patch

diff --git a/drivers/cpufreq/qcom-cpufreq-nvmem.c b/drivers/cpufreq/qcom-cpufreq-nvmem.c
index ba9e1d60e5b5..3d93b511db86 100644
--- a/drivers/cpufreq/qcom-cpufreq-nvmem.c
+++ b/drivers/cpufreq/qcom-cpufreq-nvmem.c
@@ -35,6 +35,12 @@  enum ipq8074_versions {
 	IPQ8074_ACORN_VERSION,
 };
 
+enum ipq806x_versions {
+	IPQ8062_VERSION = 0,
+	IPQ8064_VERSION,
+	IPQ8065_VERSION,
+};
+
 struct qcom_cpufreq_drv;
 
 struct qcom_cpufreq_match_data {
@@ -208,6 +214,62 @@  static int qcom_cpufreq_krait_name_version(struct device *cpu_dev,
 	return ret;
 }
 
+static int qcom_cpufreq_ipq8064_name_version(struct device *cpu_dev,
+					     struct nvmem_cell *speedbin_nvmem,
+					     char **pvs_name,
+					     struct qcom_cpufreq_drv *drv)
+{
+	int speed = 0, pvs = 0, pvs_ver = 0;
+	int msm_id, ret = 0;
+	u8 *speedbin;
+	size_t len;
+
+	speedbin = nvmem_cell_read(speedbin_nvmem, &len);
+
+	if (IS_ERR(speedbin))
+		return PTR_ERR(speedbin);
+
+	if (len != 4) {
+		dev_err(cpu_dev, "Unable to read nvmem data. Defaulting to 0!\n");
+		kfree(speedbin);
+		return -ENODEV;
+	}
+
+	get_krait_bin_format_a(cpu_dev, &speed, &pvs, &pvs_ver, speedbin);
+
+	ret = qcom_smem_get_soc_id(&msm_id);
+	if (ret)
+		return ret;
+
+	switch (msm_id) {
+	case QCOM_ID_IPQ8062:
+		drv->versions = BIT(IPQ8062_VERSION);
+		break;
+	case QCOM_ID_IPQ8064:
+	case QCOM_ID_IPQ8066:
+	case QCOM_ID_IPQ8068:
+		drv->versions = BIT(IPQ8064_VERSION);
+		break;
+	case QCOM_ID_IPQ8065:
+	case QCOM_ID_IPQ8069:
+		drv->versions = BIT(IPQ8065_VERSION);
+		break;
+	default:
+		dev_err(cpu_dev,
+			"SoC ID %u is not part of IPQ8064 family, limiting to 1.0GHz!\n",
+			msm_id);
+		drv->versions = BIT(IPQ8062_VERSION);
+		break;
+	}
+
+	/* IPQ8064 speed is never fused. Only pvs values are fused. */
+	snprintf(*pvs_name, sizeof("speedXX-pvsXX"), "speed%d-pvs%d",
+		 speed, pvs);
+
+	kfree(speedbin);
+	return ret;
+}
+
 static int qcom_cpufreq_ipq8074_name_version(struct device *cpu_dev,
 					     struct nvmem_cell *speedbin_nvmem,
 					     char **pvs_name,
@@ -257,6 +319,10 @@  static const struct qcom_cpufreq_match_data match_data_qcs404 = {
 	.genpd_names = qcs404_genpd_names,
 };
 
+static const struct qcom_cpufreq_match_data match_data_ipq8064 = {
+	.get_version = qcom_cpufreq_ipq8064_name_version,
+};
+
 static const struct qcom_cpufreq_match_data match_data_ipq8074 = {
 	.get_version = qcom_cpufreq_ipq8074_name_version,
 };
@@ -403,7 +469,7 @@  static const struct of_device_id qcom_cpufreq_match_list[] __initconst = {
 	{ .compatible = "qcom,apq8096", .data = &match_data_kryo },
 	{ .compatible = "qcom,msm8996", .data = &match_data_kryo },
 	{ .compatible = "qcom,qcs404", .data = &match_data_qcs404 },
-	{ .compatible = "qcom,ipq8064", .data = &match_data_krait },
+	{ .compatible = "qcom,ipq8064", .data = &match_data_ipq8064 },
 	{ .compatible = "qcom,ipq8074", .data = &match_data_ipq8074 },
 	{ .compatible = "qcom,apq8064", .data = &match_data_krait },
 	{ .compatible = "qcom,msm8974", .data = &match_data_krait },