| Message ID | 20230930102218.229613-3-robimarko@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | viresh kumar |
| Headers | show |
| Series | [v5,1/4] cpufreq: qcom-nvmem: add support for IPQ8074 | expand |
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
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().
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 --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 },