* [RESEND PATCH v2 2/2] cpufreq: qcom-nvmem: add support for IPQ8064
2023-05-30 16:58 [RESEND PATCH v2 1/2] cpufreq: qcom-nvmem: add support for IPQ8074 Robert Marko
@ 2023-05-30 16:58 ` Robert Marko
2023-05-31 2:03 ` Dmitry Baryshkov
2023-05-31 8:40 ` Konrad Dybcio
2023-05-31 2:08 ` [RESEND PATCH v2 1/2] cpufreq: qcom-nvmem: add support for IPQ8074 Dmitry Baryshkov
2023-06-01 12:55 ` Kathiravan T
2 siblings, 2 replies; 19+ messages in thread
From: Robert Marko @ 2023-05-30 16:58 UTC (permalink / raw)
To: rafael, viresh.kumar, agross, andersson, konrad.dybcio, ilia.lin,
linux-pm, linux-kernel, linux-arm-msm
Cc: ansuelsmth, Robert Marko
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 share one OPP table, add support for
IPQ8064 family based of SMEM SoC ID-s as speedbin fuse is always 0 on
IPQ8064.
Bit are set with the following logic:
* IPQ8062 BIT 0
* IPQ8064/IPQ8066/IPQ8068 BIT 1
* IPQ8065/IPQ8069 BIT 2
speed is never fused, only psv values are fused.
Set speed to the versions to permit a unified opp table following
this named opp:
opp-microvolt-speed<SPEED_VALUE>-pvs<PSV_VALUE>-v0
Example:
- for ipq8062 psv2
opp-microvolt-speed0-pvs2-v0 = < 925000 878750 971250>
- for ipq8064 psv2
opp-microvolt-speed2-pvs2-v0 = <925000 878750 971250>;
- for ipq8065 psv2
opp-microvolt-speed4-pvs2-v0 = <950000 902500 997500>;
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
Signed-off-by: Robert Marko <robimarko@gmail.com>
---
drivers/cpufreq/qcom-cpufreq-nvmem.c | 73 +++++++++++++++++++++++++++-
1 file changed, 72 insertions(+), 1 deletion(-)
diff --git a/drivers/cpufreq/qcom-cpufreq-nvmem.c b/drivers/cpufreq/qcom-cpufreq-nvmem.c
index ce444b5962f2..c644138680ba 100644
--- a/drivers/cpufreq/qcom-cpufreq-nvmem.c
+++ b/drivers/cpufreq/qcom-cpufreq-nvmem.c
@@ -34,6 +34,10 @@
#define IPQ8074_HAWKEYE_VERSION BIT(0)
#define IPQ8074_ACORN_VERSION BIT(1)
+#define IPQ8062_VERSION BIT(0)
+#define IPQ8064_VERSION BIT(1)
+#define IPQ8065_VERSION BIT(2)
+
struct qcom_cpufreq_drv;
struct qcom_cpufreq_match_data {
@@ -207,6 +211,69 @@ 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);
+
+ switch (len) {
+ case 4:
+ get_krait_bin_format_a(cpu_dev, &speed, &pvs, &pvs_ver,
+ speedbin);
+ break;
+ default:
+ dev_err(cpu_dev, "Unable to read nvmem data. Defaulting to 0!\n");
+ ret = -ENODEV;
+ goto len_error;
+ }
+
+ ret = qcom_smem_get_soc_id(&msm_id);
+ if (ret)
+ return ret;
+
+ switch (msm_id) {
+ case QCOM_ID_IPQ8062:
+ drv->versions = IPQ8062_VERSION;
+ break;
+ case QCOM_ID_IPQ8064:
+ case QCOM_ID_IPQ8066:
+ case QCOM_ID_IPQ8068:
+ drv->versions = IPQ8064_VERSION;
+ break;
+ case QCOM_ID_IPQ8065:
+ case QCOM_ID_IPQ8069:
+ drv->versions = 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 = IPQ8062_VERSION;
+ break;
+ }
+
+ /*
+ * IPQ8064 speed is never fused. Only psv values are fused.
+ * Set speed to the versions to permit a unified opp table.
+ */
+ snprintf(*pvs_name, sizeof("speedXX-pvsXX-vXX"), "speed%d-pvs%d-v%d",
+ drv->versions, pvs, pvs_ver);
+
+len_error:
+ kfree(speedbin);
+ return ret;
+}
+
static int qcom_cpufreq_ipq8074_name_version(struct device *cpu_dev,
struct nvmem_cell *speedbin_nvmem,
char **pvs_name,
@@ -256,6 +323,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,
};
@@ -404,7 +475,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 },
--
2.40.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [RESEND PATCH v2 2/2] cpufreq: qcom-nvmem: add support for IPQ8064
2023-05-30 16:58 ` [RESEND PATCH v2 2/2] cpufreq: qcom-nvmem: add support for IPQ8064 Robert Marko
@ 2023-05-31 2:03 ` Dmitry Baryshkov
2023-05-31 1:36 ` Christian Marangi
2023-05-31 8:40 ` Konrad Dybcio
1 sibling, 1 reply; 19+ messages in thread
From: Dmitry Baryshkov @ 2023-05-31 2:03 UTC (permalink / raw)
To: Robert Marko, rafael, viresh.kumar, agross, andersson,
konrad.dybcio, ilia.lin, linux-pm, linux-kernel, linux-arm-msm
Cc: ansuelsmth
On 30/05/2023 19:58, 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 share one OPP table, add support for
> IPQ8064 family based of SMEM SoC ID-s as speedbin fuse is always 0 on
> IPQ8064.
>
> Bit are set with the following logic:
> * IPQ8062 BIT 0
> * IPQ8064/IPQ8066/IPQ8068 BIT 1
> * IPQ8065/IPQ8069 BIT 2
>
> speed is never fused, only psv values are fused.
> Set speed to the versions to permit a unified opp table following
> this named opp:
>
> opp-microvolt-speed<SPEED_VALUE>-pvs<PSV_VALUE>-v0
>
> Example:
> - for ipq8062 psv2
> opp-microvolt-speed0-pvs2-v0 = < 925000 878750 971250>
> - for ipq8064 psv2
> opp-microvolt-speed2-pvs2-v0 = <925000 878750 971250>;
> - for ipq8065 psv2
> opp-microvolt-speed4-pvs2-v0 = <950000 902500 997500>;
>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> Signed-off-by: Robert Marko <robimarko@gmail.com>
> ---
> drivers/cpufreq/qcom-cpufreq-nvmem.c | 73 +++++++++++++++++++++++++++-
> 1 file changed, 72 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/cpufreq/qcom-cpufreq-nvmem.c b/drivers/cpufreq/qcom-cpufreq-nvmem.c
> index ce444b5962f2..c644138680ba 100644
> --- a/drivers/cpufreq/qcom-cpufreq-nvmem.c
> +++ b/drivers/cpufreq/qcom-cpufreq-nvmem.c
> @@ -34,6 +34,10 @@
> #define IPQ8074_HAWKEYE_VERSION BIT(0)
> #define IPQ8074_ACORN_VERSION BIT(1)
>
> +#define IPQ8062_VERSION BIT(0)
> +#define IPQ8064_VERSION BIT(1)
> +#define IPQ8065_VERSION BIT(2)
I think it would be more logical to change these defines to consecutive
enum instead of BIT(n) values. Another (and better in my opinion) option
is to drop versions completely (and remove speedN from the opp names)
and to have per-SoC tables in per-SoC dtsi files. There are already
separate ipq8064.dtsi, ipq8062.dtsi and ipq8065.dtsi files. It makes
little sense to overcomplicate the OPP tables.
> +
> struct qcom_cpufreq_drv;
>
> struct qcom_cpufreq_match_data {
> @@ -207,6 +211,69 @@ 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);
> +
> + switch (len) {
> + case 4:
> + get_krait_bin_format_a(cpu_dev, &speed, &pvs, &pvs_ver,
> + speedbin);
> + break;
> + default:
> + dev_err(cpu_dev, "Unable to read nvmem data. Defaulting to 0!\n");
> + ret = -ENODEV;
> + goto len_error;
> + }
> +
> + ret = qcom_smem_get_soc_id(&msm_id);
> + if (ret)
> + return ret;
> +
> + switch (msm_id) {
> + case QCOM_ID_IPQ8062:
> + drv->versions = IPQ8062_VERSION;
> + break;
> + case QCOM_ID_IPQ8064:
> + case QCOM_ID_IPQ8066:
> + case QCOM_ID_IPQ8068:
> + drv->versions = IPQ8064_VERSION;
> + break;
> + case QCOM_ID_IPQ8065:
> + case QCOM_ID_IPQ8069:
> + drv->versions = 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 = IPQ8062_VERSION;
> + break;
> + }
> +
> + /*
> + * IPQ8064 speed is never fused. Only psv values are fused.
> + * Set speed to the versions to permit a unified opp table.
> + */
> + snprintf(*pvs_name, sizeof("speedXX-pvsXX-vXX"), "speed%d-pvs%d-v%d",
> + drv->versions, pvs, pvs_ver);
> +
> +len_error:
> + kfree(speedbin);
> + return ret;
> +}
> +
> static int qcom_cpufreq_ipq8074_name_version(struct device *cpu_dev,
> struct nvmem_cell *speedbin_nvmem,
> char **pvs_name,
> @@ -256,6 +323,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,
> };
> @@ -404,7 +475,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 },
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [RESEND PATCH v2 2/2] cpufreq: qcom-nvmem: add support for IPQ8064
2023-05-31 2:03 ` Dmitry Baryshkov
@ 2023-05-31 1:36 ` Christian Marangi
2023-06-01 15:07 ` Dmitry Baryshkov
0 siblings, 1 reply; 19+ messages in thread
From: Christian Marangi @ 2023-05-31 1:36 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Robert Marko, rafael, viresh.kumar, agross, andersson,
konrad.dybcio, ilia.lin, linux-pm, linux-kernel, linux-arm-msm
On Wed, May 31, 2023 at 05:03:01AM +0300, Dmitry Baryshkov wrote:
> On 30/05/2023 19:58, 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 share one OPP table, add support for
> > IPQ8064 family based of SMEM SoC ID-s as speedbin fuse is always 0 on
> > IPQ8064.
> >
> > Bit are set with the following logic:
> > * IPQ8062 BIT 0
> > * IPQ8064/IPQ8066/IPQ8068 BIT 1
> > * IPQ8065/IPQ8069 BIT 2
> >
> > speed is never fused, only psv values are fused.
> > Set speed to the versions to permit a unified opp table following
> > this named opp:
> >
> > opp-microvolt-speed<SPEED_VALUE>-pvs<PSV_VALUE>-v0
> >
> > Example:
> > - for ipq8062 psv2
> > opp-microvolt-speed0-pvs2-v0 = < 925000 878750 971250>
> > - for ipq8064 psv2
> > opp-microvolt-speed2-pvs2-v0 = <925000 878750 971250>;
> > - for ipq8065 psv2
> > opp-microvolt-speed4-pvs2-v0 = <950000 902500 997500>;
> >
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > Signed-off-by: Robert Marko <robimarko@gmail.com>
> > ---
> > drivers/cpufreq/qcom-cpufreq-nvmem.c | 73 +++++++++++++++++++++++++++-
> > 1 file changed, 72 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/cpufreq/qcom-cpufreq-nvmem.c b/drivers/cpufreq/qcom-cpufreq-nvmem.c
> > index ce444b5962f2..c644138680ba 100644
> > --- a/drivers/cpufreq/qcom-cpufreq-nvmem.c
> > +++ b/drivers/cpufreq/qcom-cpufreq-nvmem.c
> > @@ -34,6 +34,10 @@
> > #define IPQ8074_HAWKEYE_VERSION BIT(0)
> > #define IPQ8074_ACORN_VERSION BIT(1)
> > +#define IPQ8062_VERSION BIT(0)
> > +#define IPQ8064_VERSION BIT(1)
> > +#define IPQ8065_VERSION BIT(2)
>
> I think it would be more logical to change these defines to consecutive enum
> instead of BIT(n) values. Another (and better in my opinion) option is to
> drop versions completely (and remove speedN from the opp names) and to have
> per-SoC tables in per-SoC dtsi files. There are already separate
> ipq8064.dtsi, ipq8062.dtsi and ipq8065.dtsi files. It makes little sense to
> overcomplicate the OPP tables.
>
That is what was used downstream but it was also wrong and against the
normal implementation of this driver itself.
OPP have opp-supported-hw just for the task with the principle of
declaring a single table in dtsi and automatically select the right one.
Using the implementation downstream (opp table in each dtsi) is actually
worse as ipq8065 have 1.4ghz and not 1.2ghz and that can correctly be
handled with opp-supported-hw (and this change) or using delete-property
in dtsi (that I don't really like and it's ugly)
Also this implementation would match what is currently secribed for the
use of OPP in the documentation.
Hope you can understand the reason of this change, the intention is to
clear and trying to use standard OPP stuff instead of hacks in the DTS.
> > +
> > struct qcom_cpufreq_drv;
> > struct qcom_cpufreq_match_data {
> > @@ -207,6 +211,69 @@ 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);
> > +
> > + switch (len) {
> > + case 4:
> > + get_krait_bin_format_a(cpu_dev, &speed, &pvs, &pvs_ver,
> > + speedbin);
> > + break;
> > + default:
> > + dev_err(cpu_dev, "Unable to read nvmem data. Defaulting to 0!\n");
> > + ret = -ENODEV;
> > + goto len_error;
> > + }
> > +
> > + ret = qcom_smem_get_soc_id(&msm_id);
> > + if (ret)
> > + return ret;
> > +
> > + switch (msm_id) {
> > + case QCOM_ID_IPQ8062:
> > + drv->versions = IPQ8062_VERSION;
> > + break;
> > + case QCOM_ID_IPQ8064:
> > + case QCOM_ID_IPQ8066:
> > + case QCOM_ID_IPQ8068:
> > + drv->versions = IPQ8064_VERSION;
> > + break;
> > + case QCOM_ID_IPQ8065:
> > + case QCOM_ID_IPQ8069:
> > + drv->versions = 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 = IPQ8062_VERSION;
> > + break;
> > + }
> > +
> > + /*
> > + * IPQ8064 speed is never fused. Only psv values are fused.
> > + * Set speed to the versions to permit a unified opp table.
> > + */
> > + snprintf(*pvs_name, sizeof("speedXX-pvsXX-vXX"), "speed%d-pvs%d-v%d",
> > + drv->versions, pvs, pvs_ver);
> > +
> > +len_error:
> > + kfree(speedbin);
> > + return ret;
> > +}
> > +
> > static int qcom_cpufreq_ipq8074_name_version(struct device *cpu_dev,
> > struct nvmem_cell *speedbin_nvmem,
> > char **pvs_name,
> > @@ -256,6 +323,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,
> > };
> > @@ -404,7 +475,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 },
>
> --
> With best wishes
> Dmitry
>
--
Ansuel
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [RESEND PATCH v2 2/2] cpufreq: qcom-nvmem: add support for IPQ8064
2023-05-31 1:36 ` Christian Marangi
@ 2023-06-01 15:07 ` Dmitry Baryshkov
2023-06-09 14:20 ` Christian Marangi
0 siblings, 1 reply; 19+ messages in thread
From: Dmitry Baryshkov @ 2023-06-01 15:07 UTC (permalink / raw)
To: Christian Marangi
Cc: Robert Marko, rafael, viresh.kumar, agross, andersson,
konrad.dybcio, ilia.lin, linux-pm, linux-kernel, linux-arm-msm
On 31/05/2023 04:36, Christian Marangi wrote:
> On Wed, May 31, 2023 at 05:03:01AM +0300, Dmitry Baryshkov wrote:
>> On 30/05/2023 19:58, 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 share one OPP table, add support for
>>> IPQ8064 family based of SMEM SoC ID-s as speedbin fuse is always 0 on
>>> IPQ8064.
>>>
>>> Bit are set with the following logic:
>>> * IPQ8062 BIT 0
>>> * IPQ8064/IPQ8066/IPQ8068 BIT 1
>>> * IPQ8065/IPQ8069 BIT 2
>>>
>>> speed is never fused, only psv values are fused.
>>> Set speed to the versions to permit a unified opp table following
>>> this named opp:
>>>
>>> opp-microvolt-speed<SPEED_VALUE>-pvs<PSV_VALUE>-v0
>>>
>>> Example:
>>> - for ipq8062 psv2
>>> opp-microvolt-speed0-pvs2-v0 = < 925000 878750 971250>
>>> - for ipq8064 psv2
>>> opp-microvolt-speed2-pvs2-v0 = <925000 878750 971250>;
>>> - for ipq8065 psv2
>>> opp-microvolt-speed4-pvs2-v0 = <950000 902500 997500>;
>>>
>>> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
>>> Signed-off-by: Robert Marko <robimarko@gmail.com>
>>> ---
>>> drivers/cpufreq/qcom-cpufreq-nvmem.c | 73 +++++++++++++++++++++++++++-
>>> 1 file changed, 72 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/cpufreq/qcom-cpufreq-nvmem.c b/drivers/cpufreq/qcom-cpufreq-nvmem.c
>>> index ce444b5962f2..c644138680ba 100644
>>> --- a/drivers/cpufreq/qcom-cpufreq-nvmem.c
>>> +++ b/drivers/cpufreq/qcom-cpufreq-nvmem.c
>>> @@ -34,6 +34,10 @@
>>> #define IPQ8074_HAWKEYE_VERSION BIT(0)
>>> #define IPQ8074_ACORN_VERSION BIT(1)
>>> +#define IPQ8062_VERSION BIT(0)
>>> +#define IPQ8064_VERSION BIT(1)
>>> +#define IPQ8065_VERSION BIT(2)
>>
>> I think it would be more logical to change these defines to consecutive enum
>> instead of BIT(n) values. Another (and better in my opinion) option is to
>> drop versions completely (and remove speedN from the opp names) and to have
>> per-SoC tables in per-SoC dtsi files. There are already separate
>> ipq8064.dtsi, ipq8062.dtsi and ipq8065.dtsi files. It makes little sense to
>> overcomplicate the OPP tables.
>>
>
> That is what was used downstream but it was also wrong and against the
> normal implementation of this driver itself.
>
> OPP have opp-supported-hw just for the task with the principle of
> declaring a single table in dtsi and automatically select the right one.
>
> Using the implementation downstream (opp table in each dtsi) is actually
> worse as ipq8065 have 1.4ghz and not 1.2ghz and that can correctly be
> handled with opp-supported-hw (and this change) or using delete-property
> in dtsi (that I don't really like and it's ugly)
>
> Also this implementation would match what is currently secribed for the
> use of OPP in the documentation.
>
> Hope you can understand the reason of this change, the intention is to
> clear and trying to use standard OPP stuff instead of hacks in the DTS.
I'm fine with the opp-supported-hw part (I forgot that it is used by
default with the help of drv->versions). I do not like the idea of
encoding the same value into the -speedN part. If it is not needed, it's
better be dropped than using a semi-dummy value there.
So, I'd suggest to define an enum, use BIT(enum_value) for drv->versions
and drop the speed%d part.
Also, while we are at it, could you please define a schema for your opp
extensions? An example would make it easier to understand the bindings
(and will also provide a reference for possible other implementers).
>
>>> +
>>> struct qcom_cpufreq_drv;
>>> struct qcom_cpufreq_match_data {
>>> @@ -207,6 +211,69 @@ 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);
>>> +
>>> + switch (len) {
>>> + case 4:
>>> + get_krait_bin_format_a(cpu_dev, &speed, &pvs, &pvs_ver,
>>> + speedbin);
>>> + break;
>>> + default:
>>> + dev_err(cpu_dev, "Unable to read nvmem data. Defaulting to 0!\n");
>>> + ret = -ENODEV;
>>> + goto len_error;
>>> + }
>>> +
>>> + ret = qcom_smem_get_soc_id(&msm_id);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + switch (msm_id) {
>>> + case QCOM_ID_IPQ8062:
>>> + drv->versions = IPQ8062_VERSION;
>>> + break;
>>> + case QCOM_ID_IPQ8064:
>>> + case QCOM_ID_IPQ8066:
>>> + case QCOM_ID_IPQ8068:
>>> + drv->versions = IPQ8064_VERSION;
>>> + break;
>>> + case QCOM_ID_IPQ8065:
>>> + case QCOM_ID_IPQ8069:
>>> + drv->versions = 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 = IPQ8062_VERSION;
>>> + break;
>>> + }
>>> +
>>> + /*
>>> + * IPQ8064 speed is never fused. Only psv values are fused.
>>> + * Set speed to the versions to permit a unified opp table.
>>> + */
>>> + snprintf(*pvs_name, sizeof("speedXX-pvsXX-vXX"), "speed%d-pvs%d-v%d",
>>> + drv->versions, pvs, pvs_ver);
>>> +
>>> +len_error:
>>> + kfree(speedbin);
>>> + return ret;
>>> +}
>>> +
>>> static int qcom_cpufreq_ipq8074_name_version(struct device *cpu_dev,
>>> struct nvmem_cell *speedbin_nvmem,
>>> char **pvs_name,
>>> @@ -256,6 +323,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,
>>> };
>>> @@ -404,7 +475,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 },
>>
>> --
>> With best wishes
>> Dmitry
>>
>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [RESEND PATCH v2 2/2] cpufreq: qcom-nvmem: add support for IPQ8064
2023-06-01 15:07 ` Dmitry Baryshkov
@ 2023-06-09 14:20 ` Christian Marangi
2023-06-09 14:53 ` Dmitry Baryshkov
0 siblings, 1 reply; 19+ messages in thread
From: Christian Marangi @ 2023-06-09 14:20 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Robert Marko, rafael, viresh.kumar, agross, andersson,
konrad.dybcio, ilia.lin, linux-pm, linux-kernel, linux-arm-msm
On Thu, Jun 01, 2023 at 06:07:17PM +0300, Dmitry Baryshkov wrote:
> On 31/05/2023 04:36, Christian Marangi wrote:
> > On Wed, May 31, 2023 at 05:03:01AM +0300, Dmitry Baryshkov wrote:
> > > On 30/05/2023 19:58, 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 share one OPP table, add support for
> > > > IPQ8064 family based of SMEM SoC ID-s as speedbin fuse is always 0 on
> > > > IPQ8064.
> > > >
> > > > Bit are set with the following logic:
> > > > * IPQ8062 BIT 0
> > > > * IPQ8064/IPQ8066/IPQ8068 BIT 1
> > > > * IPQ8065/IPQ8069 BIT 2
> > > >
> > > > speed is never fused, only psv values are fused.
> > > > Set speed to the versions to permit a unified opp table following
> > > > this named opp:
> > > >
> > > > opp-microvolt-speed<SPEED_VALUE>-pvs<PSV_VALUE>-v0
> > > >
> > > > Example:
> > > > - for ipq8062 psv2
> > > > opp-microvolt-speed0-pvs2-v0 = < 925000 878750 971250>
> > > > - for ipq8064 psv2
> > > > opp-microvolt-speed2-pvs2-v0 = <925000 878750 971250>;
> > > > - for ipq8065 psv2
> > > > opp-microvolt-speed4-pvs2-v0 = <950000 902500 997500>;
> > > >
> > > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > > > Signed-off-by: Robert Marko <robimarko@gmail.com>
> > > > ---
> > > > drivers/cpufreq/qcom-cpufreq-nvmem.c | 73 +++++++++++++++++++++++++++-
> > > > 1 file changed, 72 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/cpufreq/qcom-cpufreq-nvmem.c b/drivers/cpufreq/qcom-cpufreq-nvmem.c
> > > > index ce444b5962f2..c644138680ba 100644
> > > > --- a/drivers/cpufreq/qcom-cpufreq-nvmem.c
> > > > +++ b/drivers/cpufreq/qcom-cpufreq-nvmem.c
> > > > @@ -34,6 +34,10 @@
> > > > #define IPQ8074_HAWKEYE_VERSION BIT(0)
> > > > #define IPQ8074_ACORN_VERSION BIT(1)
> > > > +#define IPQ8062_VERSION BIT(0)
> > > > +#define IPQ8064_VERSION BIT(1)
> > > > +#define IPQ8065_VERSION BIT(2)
> > >
> > > I think it would be more logical to change these defines to consecutive enum
> > > instead of BIT(n) values. Another (and better in my opinion) option is to
> > > drop versions completely (and remove speedN from the opp names) and to have
> > > per-SoC tables in per-SoC dtsi files. There are already separate
> > > ipq8064.dtsi, ipq8062.dtsi and ipq8065.dtsi files. It makes little sense to
> > > overcomplicate the OPP tables.
> > >
> >
> > That is what was used downstream but it was also wrong and against the
> > normal implementation of this driver itself.
> >
> > OPP have opp-supported-hw just for the task with the principle of
> > declaring a single table in dtsi and automatically select the right one.
> >
> > Using the implementation downstream (opp table in each dtsi) is actually
> > worse as ipq8065 have 1.4ghz and not 1.2ghz and that can correctly be
> > handled with opp-supported-hw (and this change) or using delete-property
> > in dtsi (that I don't really like and it's ugly)
> >
> > Also this implementation would match what is currently secribed for the
> > use of OPP in the documentation.
> >
> > Hope you can understand the reason of this change, the intention is to
> > clear and trying to use standard OPP stuff instead of hacks in the DTS.
>
> I'm fine with the opp-supported-hw part (I forgot that it is used by default
> with the help of drv->versions). I do not like the idea of encoding the same
> value into the -speedN part. If it is not needed, it's better be dropped
> than using a semi-dummy value there.
>
> So, I'd suggest to define an enum, use BIT(enum_value) for drv->versions and
> drop the speed%d part.
>
> Also, while we are at it, could you please define a schema for your opp
> extensions? An example would make it easier to understand the bindings (and
> will also provide a reference for possible other implementers).
>
Sorry for the delay in answering this.
The speed part is still needed... since the voltage for each voltage
change on the different SoC.
Let me give you an example for one freq.
opp-384000000 {
opp-hz = /bits/ 64 <384000000>;
opp-microvolt-speed0-pvs0-v0 = <1000000 950000 1050000>;
opp-microvolt-speed0-pvs1-v0 = <925000 878750 971250>;
opp-microvolt-speed0-pvs2-v0 = <875000 831250 918750>;
opp-microvolt-speed0-pvs3-v0 = <800000 760000 840000>;
opp-microvolt-speed2-pvs0-v0 = <1000000 950000 1050000>;
opp-microvolt-speed2-pvs1-v0 = <925000 878750 971250>;
opp-microvolt-speed2-pvs2-v0 = <875000 831250 918750>;
opp-microvolt-speed2-pvs3-v0 = <800000 760000 840000>;
opp-microvolt-speed4-pvs0-v0 = <975000 926250 1023750>;
opp-microvolt-speed4-pvs1-v0 = <950000 902500 997500>;
opp-microvolt-speed4-pvs2-v0 = <925000 878750 971250>;
opp-microvolt-speed4-pvs3-v0 = <900000 855000 945000>;
opp-microvolt-speed4-pvs4-v0 = <875000 831250 918750>;
opp-microvolt-speed4-pvs5-v0 = <825000 783750 866250>;
opp-microvolt-speed4-pvs6-v0 = <775000 736250 813750>;
opp-supported-hw = <0x7>;
clock-latency-ns = <100000>;
};
As you can see we use the speed value to match the different SoC and
apply the correct voltage.
Yes I will add the missing info in the schema.
> >
> > > > +
> > > > struct qcom_cpufreq_drv;
> > > > struct qcom_cpufreq_match_data {
> > > > @@ -207,6 +211,69 @@ 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);
> > > > +
> > > > + switch (len) {
> > > > + case 4:
> > > > + get_krait_bin_format_a(cpu_dev, &speed, &pvs, &pvs_ver,
> > > > + speedbin);
> > > > + break;
> > > > + default:
> > > > + dev_err(cpu_dev, "Unable to read nvmem data. Defaulting to 0!\n");
> > > > + ret = -ENODEV;
> > > > + goto len_error;
> > > > + }
> > > > +
> > > > + ret = qcom_smem_get_soc_id(&msm_id);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + switch (msm_id) {
> > > > + case QCOM_ID_IPQ8062:
> > > > + drv->versions = IPQ8062_VERSION;
> > > > + break;
> > > > + case QCOM_ID_IPQ8064:
> > > > + case QCOM_ID_IPQ8066:
> > > > + case QCOM_ID_IPQ8068:
> > > > + drv->versions = IPQ8064_VERSION;
> > > > + break;
> > > > + case QCOM_ID_IPQ8065:
> > > > + case QCOM_ID_IPQ8069:
> > > > + drv->versions = 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 = IPQ8062_VERSION;
> > > > + break;
> > > > + }
> > > > +
> > > > + /*
> > > > + * IPQ8064 speed is never fused. Only psv values are fused.
> > > > + * Set speed to the versions to permit a unified opp table.
> > > > + */
> > > > + snprintf(*pvs_name, sizeof("speedXX-pvsXX-vXX"), "speed%d-pvs%d-v%d",
> > > > + drv->versions, pvs, pvs_ver);
> > > > +
> > > > +len_error:
> > > > + kfree(speedbin);
> > > > + return ret;
> > > > +}
> > > > +
> > > > static int qcom_cpufreq_ipq8074_name_version(struct device *cpu_dev,
> > > > struct nvmem_cell *speedbin_nvmem,
> > > > char **pvs_name,
> > > > @@ -256,6 +323,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,
> > > > };
> > > > @@ -404,7 +475,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 },
> > >
> > > --
> > > With best wishes
> > > Dmitry
> > >
> >
>
> --
> With best wishes
> Dmitry
>
--
Ansuel
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [RESEND PATCH v2 2/2] cpufreq: qcom-nvmem: add support for IPQ8064
2023-06-09 14:20 ` Christian Marangi
@ 2023-06-09 14:53 ` Dmitry Baryshkov
2023-06-09 15:02 ` Christian Marangi
0 siblings, 1 reply; 19+ messages in thread
From: Dmitry Baryshkov @ 2023-06-09 14:53 UTC (permalink / raw)
To: Christian Marangi
Cc: Robert Marko, rafael, viresh.kumar, agross, andersson,
konrad.dybcio, ilia.lin, linux-pm, linux-kernel, linux-arm-msm
On Fri, 9 Jun 2023 at 17:21, Christian Marangi <ansuelsmth@gmail.com> wrote:
>
> On Thu, Jun 01, 2023 at 06:07:17PM +0300, Dmitry Baryshkov wrote:
> > On 31/05/2023 04:36, Christian Marangi wrote:
> > > On Wed, May 31, 2023 at 05:03:01AM +0300, Dmitry Baryshkov wrote:
> > > > On 30/05/2023 19:58, 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 share one OPP table, add support for
> > > > > IPQ8064 family based of SMEM SoC ID-s as speedbin fuse is always 0 on
> > > > > IPQ8064.
> > > > >
> > > > > Bit are set with the following logic:
> > > > > * IPQ8062 BIT 0
> > > > > * IPQ8064/IPQ8066/IPQ8068 BIT 1
> > > > > * IPQ8065/IPQ8069 BIT 2
> > > > >
> > > > > speed is never fused, only psv values are fused.
> > > > > Set speed to the versions to permit a unified opp table following
> > > > > this named opp:
> > > > >
> > > > > opp-microvolt-speed<SPEED_VALUE>-pvs<PSV_VALUE>-v0
> > > > >
> > > > > Example:
> > > > > - for ipq8062 psv2
> > > > > opp-microvolt-speed0-pvs2-v0 = < 925000 878750 971250>
> > > > > - for ipq8064 psv2
> > > > > opp-microvolt-speed2-pvs2-v0 = <925000 878750 971250>;
> > > > > - for ipq8065 psv2
> > > > > opp-microvolt-speed4-pvs2-v0 = <950000 902500 997500>;
> > > > >
> > > > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > > > > Signed-off-by: Robert Marko <robimarko@gmail.com>
> > > > > ---
> > > > > drivers/cpufreq/qcom-cpufreq-nvmem.c | 73 +++++++++++++++++++++++++++-
> > > > > 1 file changed, 72 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/cpufreq/qcom-cpufreq-nvmem.c b/drivers/cpufreq/qcom-cpufreq-nvmem.c
> > > > > index ce444b5962f2..c644138680ba 100644
> > > > > --- a/drivers/cpufreq/qcom-cpufreq-nvmem.c
> > > > > +++ b/drivers/cpufreq/qcom-cpufreq-nvmem.c
> > > > > @@ -34,6 +34,10 @@
> > > > > #define IPQ8074_HAWKEYE_VERSION BIT(0)
> > > > > #define IPQ8074_ACORN_VERSION BIT(1)
> > > > > +#define IPQ8062_VERSION BIT(0)
> > > > > +#define IPQ8064_VERSION BIT(1)
> > > > > +#define IPQ8065_VERSION BIT(2)
> > > >
> > > > I think it would be more logical to change these defines to consecutive enum
> > > > instead of BIT(n) values. Another (and better in my opinion) option is to
> > > > drop versions completely (and remove speedN from the opp names) and to have
> > > > per-SoC tables in per-SoC dtsi files. There are already separate
> > > > ipq8064.dtsi, ipq8062.dtsi and ipq8065.dtsi files. It makes little sense to
> > > > overcomplicate the OPP tables.
> > > >
> > >
> > > That is what was used downstream but it was also wrong and against the
> > > normal implementation of this driver itself.
> > >
> > > OPP have opp-supported-hw just for the task with the principle of
> > > declaring a single table in dtsi and automatically select the right one.
> > >
> > > Using the implementation downstream (opp table in each dtsi) is actually
> > > worse as ipq8065 have 1.4ghz and not 1.2ghz and that can correctly be
> > > handled with opp-supported-hw (and this change) or using delete-property
> > > in dtsi (that I don't really like and it's ugly)
> > >
> > > Also this implementation would match what is currently secribed for the
> > > use of OPP in the documentation.
> > >
> > > Hope you can understand the reason of this change, the intention is to
> > > clear and trying to use standard OPP stuff instead of hacks in the DTS.
> >
> > I'm fine with the opp-supported-hw part (I forgot that it is used by default
> > with the help of drv->versions). I do not like the idea of encoding the same
> > value into the -speedN part. If it is not needed, it's better be dropped
> > than using a semi-dummy value there.
> >
> > So, I'd suggest to define an enum, use BIT(enum_value) for drv->versions and
> > drop the speed%d part.
> >
> > Also, while we are at it, could you please define a schema for your opp
> > extensions? An example would make it easier to understand the bindings (and
> > will also provide a reference for possible other implementers).
> >
>
> Sorry for the delay in answering this.
>
> The speed part is still needed... since the voltage for each voltage
> change on the different SoC.
>
> Let me give you an example for one freq.
>
> opp-384000000 {
> opp-hz = /bits/ 64 <384000000>;
> opp-microvolt-speed0-pvs0-v0 = <1000000 950000 1050000>;
> opp-microvolt-speed0-pvs1-v0 = <925000 878750 971250>;
> opp-microvolt-speed0-pvs2-v0 = <875000 831250 918750>;
> opp-microvolt-speed0-pvs3-v0 = <800000 760000 840000>;
> opp-microvolt-speed2-pvs0-v0 = <1000000 950000 1050000>;
> opp-microvolt-speed2-pvs1-v0 = <925000 878750 971250>;
> opp-microvolt-speed2-pvs2-v0 = <875000 831250 918750>;
> opp-microvolt-speed2-pvs3-v0 = <800000 760000 840000>;
> opp-microvolt-speed4-pvs0-v0 = <975000 926250 1023750>;
> opp-microvolt-speed4-pvs1-v0 = <950000 902500 997500>;
> opp-microvolt-speed4-pvs2-v0 = <925000 878750 971250>;
> opp-microvolt-speed4-pvs3-v0 = <900000 855000 945000>;
> opp-microvolt-speed4-pvs4-v0 = <875000 831250 918750>;
> opp-microvolt-speed4-pvs5-v0 = <825000 783750 866250>;
> opp-microvolt-speed4-pvs6-v0 = <775000 736250 813750>;
> opp-supported-hw = <0x7>;
> clock-latency-ns = <100000>;
> };
What about (it will require changes to opp-v2-base.yaml):
opp-384000000-0 {
opp-hz = /bits/ 64 <384000000>;
opp-microvolt-pvs0-v0 = <1000000 950000 1050000>;
opp-microvolt-pvs1-v0 = <925000 878750 971250>;
opp-microvolt-pvs2-v0 = <875000 831250 918750>;
opp-microvolt-pvs3-v0 = <800000 760000 840000>;
opp-supported-hw = <0x1>;
clock-latency-ns = <100000>;
};
opp-384000000-1 {
opp-hz = /bits/ 64 <384000000>;
opp-microvolt-pvs0-v0 = <1000000 950000 1050000>;
opp-microvolt-pvs1-v0 = <925000 878750 971250>;
opp-microvolt-pvs2-v0 = <875000 831250 918750>;
opp-microvolt-pvs3-v0 = <800000 760000 840000>;
opp-supported-hw = <0x2>;
clock-latency-ns = <100000>;
};
opp-384000000-2 {
opp-hz = /bits/ 64 <384000000>;
opp-microvolt-pvs0-v0 = <975000 926250 1023750>;
opp-microvolt-pvs1-v0 = <950000 902500 997500>;
opp-microvolt-pvs2-v0 = <925000 878750 971250>;
opp-microvolt-pvs3-v0 = <900000 855000 945000>;
opp-microvolt-pvs4-v0 = <875000 831250 918750>;
opp-microvolt-pvs5-v0 = <825000 783750 866250>;
opp-microvolt-pvs6-v0 = <775000 736250 813750>;
opp-supported-hw = <0x4>;
clock-latency-ns = <100000>;
};
> As you can see we use the speed value to match the different SoC and
> apply the correct voltage.
>
> Yes I will add the missing info in the schema.
>
> > >
> > > > > +
> > > > > struct qcom_cpufreq_drv;
> > > > > struct qcom_cpufreq_match_data {
> > > > > @@ -207,6 +211,69 @@ 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);
> > > > > +
> > > > > + switch (len) {
> > > > > + case 4:
> > > > > + get_krait_bin_format_a(cpu_dev, &speed, &pvs, &pvs_ver,
> > > > > + speedbin);
> > > > > + break;
> > > > > + default:
> > > > > + dev_err(cpu_dev, "Unable to read nvmem data. Defaulting to 0!\n");
> > > > > + ret = -ENODEV;
> > > > > + goto len_error;
> > > > > + }
> > > > > +
> > > > > + ret = qcom_smem_get_soc_id(&msm_id);
> > > > > + if (ret)
> > > > > + return ret;
> > > > > +
> > > > > + switch (msm_id) {
> > > > > + case QCOM_ID_IPQ8062:
> > > > > + drv->versions = IPQ8062_VERSION;
> > > > > + break;
> > > > > + case QCOM_ID_IPQ8064:
> > > > > + case QCOM_ID_IPQ8066:
> > > > > + case QCOM_ID_IPQ8068:
> > > > > + drv->versions = IPQ8064_VERSION;
> > > > > + break;
> > > > > + case QCOM_ID_IPQ8065:
> > > > > + case QCOM_ID_IPQ8069:
> > > > > + drv->versions = 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 = IPQ8062_VERSION;
> > > > > + break;
> > > > > + }
> > > > > +
> > > > > + /*
> > > > > + * IPQ8064 speed is never fused. Only psv values are fused.
> > > > > + * Set speed to the versions to permit a unified opp table.
> > > > > + */
> > > > > + snprintf(*pvs_name, sizeof("speedXX-pvsXX-vXX"), "speed%d-pvs%d-v%d",
> > > > > + drv->versions, pvs, pvs_ver);
> > > > > +
> > > > > +len_error:
> > > > > + kfree(speedbin);
> > > > > + return ret;
> > > > > +}
> > > > > +
> > > > > static int qcom_cpufreq_ipq8074_name_version(struct device *cpu_dev,
> > > > > struct nvmem_cell *speedbin_nvmem,
> > > > > char **pvs_name,
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [RESEND PATCH v2 2/2] cpufreq: qcom-nvmem: add support for IPQ8064
2023-06-09 14:53 ` Dmitry Baryshkov
@ 2023-06-09 15:02 ` Christian Marangi
2023-06-09 16:17 ` Dmitry Baryshkov
0 siblings, 1 reply; 19+ messages in thread
From: Christian Marangi @ 2023-06-09 15:02 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Robert Marko, rafael, viresh.kumar, agross, andersson,
konrad.dybcio, ilia.lin, linux-pm, linux-kernel, linux-arm-msm
On Fri, Jun 09, 2023 at 05:53:40PM +0300, Dmitry Baryshkov wrote:
> On Fri, 9 Jun 2023 at 17:21, Christian Marangi <ansuelsmth@gmail.com> wrote:
> >
> > On Thu, Jun 01, 2023 at 06:07:17PM +0300, Dmitry Baryshkov wrote:
> > > On 31/05/2023 04:36, Christian Marangi wrote:
> > > > On Wed, May 31, 2023 at 05:03:01AM +0300, Dmitry Baryshkov wrote:
> > > > > On 30/05/2023 19:58, 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 share one OPP table, add support for
> > > > > > IPQ8064 family based of SMEM SoC ID-s as speedbin fuse is always 0 on
> > > > > > IPQ8064.
> > > > > >
> > > > > > Bit are set with the following logic:
> > > > > > * IPQ8062 BIT 0
> > > > > > * IPQ8064/IPQ8066/IPQ8068 BIT 1
> > > > > > * IPQ8065/IPQ8069 BIT 2
> > > > > >
> > > > > > speed is never fused, only psv values are fused.
> > > > > > Set speed to the versions to permit a unified opp table following
> > > > > > this named opp:
> > > > > >
> > > > > > opp-microvolt-speed<SPEED_VALUE>-pvs<PSV_VALUE>-v0
> > > > > >
> > > > > > Example:
> > > > > > - for ipq8062 psv2
> > > > > > opp-microvolt-speed0-pvs2-v0 = < 925000 878750 971250>
> > > > > > - for ipq8064 psv2
> > > > > > opp-microvolt-speed2-pvs2-v0 = <925000 878750 971250>;
> > > > > > - for ipq8065 psv2
> > > > > > opp-microvolt-speed4-pvs2-v0 = <950000 902500 997500>;
> > > > > >
> > > > > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > > > > > Signed-off-by: Robert Marko <robimarko@gmail.com>
> > > > > > ---
> > > > > > drivers/cpufreq/qcom-cpufreq-nvmem.c | 73 +++++++++++++++++++++++++++-
> > > > > > 1 file changed, 72 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/cpufreq/qcom-cpufreq-nvmem.c b/drivers/cpufreq/qcom-cpufreq-nvmem.c
> > > > > > index ce444b5962f2..c644138680ba 100644
> > > > > > --- a/drivers/cpufreq/qcom-cpufreq-nvmem.c
> > > > > > +++ b/drivers/cpufreq/qcom-cpufreq-nvmem.c
> > > > > > @@ -34,6 +34,10 @@
> > > > > > #define IPQ8074_HAWKEYE_VERSION BIT(0)
> > > > > > #define IPQ8074_ACORN_VERSION BIT(1)
> > > > > > +#define IPQ8062_VERSION BIT(0)
> > > > > > +#define IPQ8064_VERSION BIT(1)
> > > > > > +#define IPQ8065_VERSION BIT(2)
> > > > >
> > > > > I think it would be more logical to change these defines to consecutive enum
> > > > > instead of BIT(n) values. Another (and better in my opinion) option is to
> > > > > drop versions completely (and remove speedN from the opp names) and to have
> > > > > per-SoC tables in per-SoC dtsi files. There are already separate
> > > > > ipq8064.dtsi, ipq8062.dtsi and ipq8065.dtsi files. It makes little sense to
> > > > > overcomplicate the OPP tables.
> > > > >
> > > >
> > > > That is what was used downstream but it was also wrong and against the
> > > > normal implementation of this driver itself.
> > > >
> > > > OPP have opp-supported-hw just for the task with the principle of
> > > > declaring a single table in dtsi and automatically select the right one.
> > > >
> > > > Using the implementation downstream (opp table in each dtsi) is actually
> > > > worse as ipq8065 have 1.4ghz and not 1.2ghz and that can correctly be
> > > > handled with opp-supported-hw (and this change) or using delete-property
> > > > in dtsi (that I don't really like and it's ugly)
> > > >
> > > > Also this implementation would match what is currently secribed for the
> > > > use of OPP in the documentation.
> > > >
> > > > Hope you can understand the reason of this change, the intention is to
> > > > clear and trying to use standard OPP stuff instead of hacks in the DTS.
> > >
> > > I'm fine with the opp-supported-hw part (I forgot that it is used by default
> > > with the help of drv->versions). I do not like the idea of encoding the same
> > > value into the -speedN part. If it is not needed, it's better be dropped
> > > than using a semi-dummy value there.
> > >
> > > So, I'd suggest to define an enum, use BIT(enum_value) for drv->versions and
> > > drop the speed%d part.
> > >
> > > Also, while we are at it, could you please define a schema for your opp
> > > extensions? An example would make it easier to understand the bindings (and
> > > will also provide a reference for possible other implementers).
> > >
> >
> > Sorry for the delay in answering this.
> >
> > The speed part is still needed... since the voltage for each voltage
> > change on the different SoC.
> >
> > Let me give you an example for one freq.
> >
> > opp-384000000 {
> > opp-hz = /bits/ 64 <384000000>;
> > opp-microvolt-speed0-pvs0-v0 = <1000000 950000 1050000>;
> > opp-microvolt-speed0-pvs1-v0 = <925000 878750 971250>;
> > opp-microvolt-speed0-pvs2-v0 = <875000 831250 918750>;
> > opp-microvolt-speed0-pvs3-v0 = <800000 760000 840000>;
> > opp-microvolt-speed2-pvs0-v0 = <1000000 950000 1050000>;
> > opp-microvolt-speed2-pvs1-v0 = <925000 878750 971250>;
> > opp-microvolt-speed2-pvs2-v0 = <875000 831250 918750>;
> > opp-microvolt-speed2-pvs3-v0 = <800000 760000 840000>;
> > opp-microvolt-speed4-pvs0-v0 = <975000 926250 1023750>;
> > opp-microvolt-speed4-pvs1-v0 = <950000 902500 997500>;
> > opp-microvolt-speed4-pvs2-v0 = <925000 878750 971250>;
> > opp-microvolt-speed4-pvs3-v0 = <900000 855000 945000>;
> > opp-microvolt-speed4-pvs4-v0 = <875000 831250 918750>;
> > opp-microvolt-speed4-pvs5-v0 = <825000 783750 866250>;
> > opp-microvolt-speed4-pvs6-v0 = <775000 736250 813750>;
> > opp-supported-hw = <0x7>;
> > clock-latency-ns = <100000>;
> > };
>
> What about (it will require changes to opp-v2-base.yaml):
>
> opp-384000000-0 {
> opp-hz = /bits/ 64 <384000000>;
> opp-microvolt-pvs0-v0 = <1000000 950000 1050000>;
> opp-microvolt-pvs1-v0 = <925000 878750 971250>;
> opp-microvolt-pvs2-v0 = <875000 831250 918750>;
> opp-microvolt-pvs3-v0 = <800000 760000 840000>;
> opp-supported-hw = <0x1>;
> clock-latency-ns = <100000>;
> };
>
> opp-384000000-1 {
> opp-hz = /bits/ 64 <384000000>;
> opp-microvolt-pvs0-v0 = <1000000 950000 1050000>;
> opp-microvolt-pvs1-v0 = <925000 878750 971250>;
> opp-microvolt-pvs2-v0 = <875000 831250 918750>;
> opp-microvolt-pvs3-v0 = <800000 760000 840000>;
> opp-supported-hw = <0x2>;
> clock-latency-ns = <100000>;
> };
>
> opp-384000000-2 {
> opp-hz = /bits/ 64 <384000000>;
> opp-microvolt-pvs0-v0 = <975000 926250 1023750>;
> opp-microvolt-pvs1-v0 = <950000 902500 997500>;
> opp-microvolt-pvs2-v0 = <925000 878750 971250>;
> opp-microvolt-pvs3-v0 = <900000 855000 945000>;
> opp-microvolt-pvs4-v0 = <875000 831250 918750>;
> opp-microvolt-pvs5-v0 = <825000 783750 866250>;
> opp-microvolt-pvs6-v0 = <775000 736250 813750>;
> opp-supported-hw = <0x4>;
> clock-latency-ns = <100000>;
> };
>
>
Mhhhhhh is it really worth it? Would also require to modify the pattern
currently used in the qcom-cpufreq-nvmem that is speedXX-pvsXX-vXX.
ipq806x is really krait cpu not correctly fused. I expect downstream the
same pattern to be used widely with other krait based systems so the
idea was to not reinvent the wheel. (introducing a different pattern
also means additional condition in the schema)
If we really don't want to have big opp table in one dtsi I can consider
redefining the same opp table in the different dtsi. (the
opp-supported-hw was really needed for the 1.2ghz problem, if asked I
can still split the table in the related dtsi but I don't really like
it)
Anyway I'm open to both solution.
If it's ok to change the pattern I would change it directly to something
like opp-microvolt-pvs since even pvs version is always 0.
I'm honestly against the idea of defining multiple opp-table for the
same freq (and also change the base schema) since we can do the same
thing by just overwriting the table in the other dtsi.
> > As you can see we use the speed value to match the different SoC and
> > apply the correct voltage.
> >
> > Yes I will add the missing info in the schema.
> >
> > > >
> > > > > > +
> > > > > > struct qcom_cpufreq_drv;
> > > > > > struct qcom_cpufreq_match_data {
> > > > > > @@ -207,6 +211,69 @@ 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);
> > > > > > +
> > > > > > + switch (len) {
> > > > > > + case 4:
> > > > > > + get_krait_bin_format_a(cpu_dev, &speed, &pvs, &pvs_ver,
> > > > > > + speedbin);
> > > > > > + break;
> > > > > > + default:
> > > > > > + dev_err(cpu_dev, "Unable to read nvmem data. Defaulting to 0!\n");
> > > > > > + ret = -ENODEV;
> > > > > > + goto len_error;
> > > > > > + }
> > > > > > +
> > > > > > + ret = qcom_smem_get_soc_id(&msm_id);
> > > > > > + if (ret)
> > > > > > + return ret;
> > > > > > +
> > > > > > + switch (msm_id) {
> > > > > > + case QCOM_ID_IPQ8062:
> > > > > > + drv->versions = IPQ8062_VERSION;
> > > > > > + break;
> > > > > > + case QCOM_ID_IPQ8064:
> > > > > > + case QCOM_ID_IPQ8066:
> > > > > > + case QCOM_ID_IPQ8068:
> > > > > > + drv->versions = IPQ8064_VERSION;
> > > > > > + break;
> > > > > > + case QCOM_ID_IPQ8065:
> > > > > > + case QCOM_ID_IPQ8069:
> > > > > > + drv->versions = 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 = IPQ8062_VERSION;
> > > > > > + break;
> > > > > > + }
> > > > > > +
> > > > > > + /*
> > > > > > + * IPQ8064 speed is never fused. Only psv values are fused.
> > > > > > + * Set speed to the versions to permit a unified opp table.
> > > > > > + */
> > > > > > + snprintf(*pvs_name, sizeof("speedXX-pvsXX-vXX"), "speed%d-pvs%d-v%d",
> > > > > > + drv->versions, pvs, pvs_ver);
> > > > > > +
> > > > > > +len_error:
> > > > > > + kfree(speedbin);
> > > > > > + return ret;
> > > > > > +}
> > > > > > +
> > > > > > static int qcom_cpufreq_ipq8074_name_version(struct device *cpu_dev,
> > > > > > struct nvmem_cell *speedbin_nvmem,
> > > > > > char **pvs_name,
>
>
> --
> With best wishes
> Dmitry
--
Ansuel
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [RESEND PATCH v2 2/2] cpufreq: qcom-nvmem: add support for IPQ8064
2023-06-09 15:02 ` Christian Marangi
@ 2023-06-09 16:17 ` Dmitry Baryshkov
0 siblings, 0 replies; 19+ messages in thread
From: Dmitry Baryshkov @ 2023-06-09 16:17 UTC (permalink / raw)
To: Christian Marangi
Cc: Robert Marko, rafael, viresh.kumar, agross, andersson,
konrad.dybcio, ilia.lin, linux-pm, linux-kernel, linux-arm-msm
On Fri, 9 Jun 2023 at 18:02, Christian Marangi <ansuelsmth@gmail.com> wrote:
>
> On Fri, Jun 09, 2023 at 05:53:40PM +0300, Dmitry Baryshkov wrote:
> > On Fri, 9 Jun 2023 at 17:21, Christian Marangi <ansuelsmth@gmail.com> wrote:
> > >
> > > On Thu, Jun 01, 2023 at 06:07:17PM +0300, Dmitry Baryshkov wrote:
> > > > On 31/05/2023 04:36, Christian Marangi wrote:
> > > > > On Wed, May 31, 2023 at 05:03:01AM +0300, Dmitry Baryshkov wrote:
> > > > > > On 30/05/2023 19:58, 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 share one OPP table, add support for
> > > > > > > IPQ8064 family based of SMEM SoC ID-s as speedbin fuse is always 0 on
> > > > > > > IPQ8064.
> > > > > > >
> > > > > > > Bit are set with the following logic:
> > > > > > > * IPQ8062 BIT 0
> > > > > > > * IPQ8064/IPQ8066/IPQ8068 BIT 1
> > > > > > > * IPQ8065/IPQ8069 BIT 2
> > > > > > >
> > > > > > > speed is never fused, only psv values are fused.
> > > > > > > Set speed to the versions to permit a unified opp table following
> > > > > > > this named opp:
> > > > > > >
> > > > > > > opp-microvolt-speed<SPEED_VALUE>-pvs<PSV_VALUE>-v0
> > > > > > >
> > > > > > > Example:
> > > > > > > - for ipq8062 psv2
> > > > > > > opp-microvolt-speed0-pvs2-v0 = < 925000 878750 971250>
> > > > > > > - for ipq8064 psv2
> > > > > > > opp-microvolt-speed2-pvs2-v0 = <925000 878750 971250>;
> > > > > > > - for ipq8065 psv2
> > > > > > > opp-microvolt-speed4-pvs2-v0 = <950000 902500 997500>;
> > > > > > >
> > > > > > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > > > > > > Signed-off-by: Robert Marko <robimarko@gmail.com>
> > > > > > > ---
> > > > > > > drivers/cpufreq/qcom-cpufreq-nvmem.c | 73 +++++++++++++++++++++++++++-
> > > > > > > 1 file changed, 72 insertions(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/drivers/cpufreq/qcom-cpufreq-nvmem.c b/drivers/cpufreq/qcom-cpufreq-nvmem.c
> > > > > > > index ce444b5962f2..c644138680ba 100644
> > > > > > > --- a/drivers/cpufreq/qcom-cpufreq-nvmem.c
> > > > > > > +++ b/drivers/cpufreq/qcom-cpufreq-nvmem.c
> > > > > > > @@ -34,6 +34,10 @@
> > > > > > > #define IPQ8074_HAWKEYE_VERSION BIT(0)
> > > > > > > #define IPQ8074_ACORN_VERSION BIT(1)
> > > > > > > +#define IPQ8062_VERSION BIT(0)
> > > > > > > +#define IPQ8064_VERSION BIT(1)
> > > > > > > +#define IPQ8065_VERSION BIT(2)
> > > > > >
> > > > > > I think it would be more logical to change these defines to consecutive enum
> > > > > > instead of BIT(n) values. Another (and better in my opinion) option is to
> > > > > > drop versions completely (and remove speedN from the opp names) and to have
> > > > > > per-SoC tables in per-SoC dtsi files. There are already separate
> > > > > > ipq8064.dtsi, ipq8062.dtsi and ipq8065.dtsi files. It makes little sense to
> > > > > > overcomplicate the OPP tables.
> > > > > >
> > > > >
> > > > > That is what was used downstream but it was also wrong and against the
> > > > > normal implementation of this driver itself.
> > > > >
> > > > > OPP have opp-supported-hw just for the task with the principle of
> > > > > declaring a single table in dtsi and automatically select the right one.
> > > > >
> > > > > Using the implementation downstream (opp table in each dtsi) is actually
> > > > > worse as ipq8065 have 1.4ghz and not 1.2ghz and that can correctly be
> > > > > handled with opp-supported-hw (and this change) or using delete-property
> > > > > in dtsi (that I don't really like and it's ugly)
> > > > >
> > > > > Also this implementation would match what is currently secribed for the
> > > > > use of OPP in the documentation.
> > > > >
> > > > > Hope you can understand the reason of this change, the intention is to
> > > > > clear and trying to use standard OPP stuff instead of hacks in the DTS.
> > > >
> > > > I'm fine with the opp-supported-hw part (I forgot that it is used by default
> > > > with the help of drv->versions). I do not like the idea of encoding the same
> > > > value into the -speedN part. If it is not needed, it's better be dropped
> > > > than using a semi-dummy value there.
> > > >
> > > > So, I'd suggest to define an enum, use BIT(enum_value) for drv->versions and
> > > > drop the speed%d part.
> > > >
> > > > Also, while we are at it, could you please define a schema for your opp
> > > > extensions? An example would make it easier to understand the bindings (and
> > > > will also provide a reference for possible other implementers).
> > > >
> > >
> > > Sorry for the delay in answering this.
> > >
> > > The speed part is still needed... since the voltage for each voltage
> > > change on the different SoC.
> > >
> > > Let me give you an example for one freq.
> > >
> > > opp-384000000 {
> > > opp-hz = /bits/ 64 <384000000>;
> > > opp-microvolt-speed0-pvs0-v0 = <1000000 950000 1050000>;
> > > opp-microvolt-speed0-pvs1-v0 = <925000 878750 971250>;
> > > opp-microvolt-speed0-pvs2-v0 = <875000 831250 918750>;
> > > opp-microvolt-speed0-pvs3-v0 = <800000 760000 840000>;
> > > opp-microvolt-speed2-pvs0-v0 = <1000000 950000 1050000>;
> > > opp-microvolt-speed2-pvs1-v0 = <925000 878750 971250>;
> > > opp-microvolt-speed2-pvs2-v0 = <875000 831250 918750>;
> > > opp-microvolt-speed2-pvs3-v0 = <800000 760000 840000>;
> > > opp-microvolt-speed4-pvs0-v0 = <975000 926250 1023750>;
> > > opp-microvolt-speed4-pvs1-v0 = <950000 902500 997500>;
> > > opp-microvolt-speed4-pvs2-v0 = <925000 878750 971250>;
> > > opp-microvolt-speed4-pvs3-v0 = <900000 855000 945000>;
> > > opp-microvolt-speed4-pvs4-v0 = <875000 831250 918750>;
> > > opp-microvolt-speed4-pvs5-v0 = <825000 783750 866250>;
> > > opp-microvolt-speed4-pvs6-v0 = <775000 736250 813750>;
> > > opp-supported-hw = <0x7>;
> > > clock-latency-ns = <100000>;
> > > };
> >
> > What about (it will require changes to opp-v2-base.yaml):
> >
> > opp-384000000-0 {
> > opp-hz = /bits/ 64 <384000000>;
> > opp-microvolt-pvs0-v0 = <1000000 950000 1050000>;
> > opp-microvolt-pvs1-v0 = <925000 878750 971250>;
> > opp-microvolt-pvs2-v0 = <875000 831250 918750>;
> > opp-microvolt-pvs3-v0 = <800000 760000 840000>;
> > opp-supported-hw = <0x1>;
> > clock-latency-ns = <100000>;
> > };
> >
> > opp-384000000-1 {
> > opp-hz = /bits/ 64 <384000000>;
> > opp-microvolt-pvs0-v0 = <1000000 950000 1050000>;
> > opp-microvolt-pvs1-v0 = <925000 878750 971250>;
> > opp-microvolt-pvs2-v0 = <875000 831250 918750>;
> > opp-microvolt-pvs3-v0 = <800000 760000 840000>;
> > opp-supported-hw = <0x2>;
> > clock-latency-ns = <100000>;
> > };
> >
> > opp-384000000-2 {
> > opp-hz = /bits/ 64 <384000000>;
> > opp-microvolt-pvs0-v0 = <975000 926250 1023750>;
> > opp-microvolt-pvs1-v0 = <950000 902500 997500>;
> > opp-microvolt-pvs2-v0 = <925000 878750 971250>;
> > opp-microvolt-pvs3-v0 = <900000 855000 945000>;
> > opp-microvolt-pvs4-v0 = <875000 831250 918750>;
> > opp-microvolt-pvs5-v0 = <825000 783750 866250>;
> > opp-microvolt-pvs6-v0 = <775000 736250 813750>;
> > opp-supported-hw = <0x4>;
> > clock-latency-ns = <100000>;
> > };
> >
> >
>
> Mhhhhhh is it really worth it? Would also require to modify the pattern
> currently used in the qcom-cpufreq-nvmem that is speedXX-pvsXX-vXX.
>
> ipq806x is really krait cpu not correctly fused. I expect downstream the
> same pattern to be used widely with other krait based systems so the
> idea was to not reinvent the wheel. (introducing a different pattern
> also means additional condition in the schema)
>
> If we really don't want to have big opp table in one dtsi I can consider
> redefining the same opp table in the different dtsi. (the
> opp-supported-hw was really needed for the 1.2ghz problem, if asked I
> can still split the table in the related dtsi but I don't really like
> it)
>
> Anyway I'm open to both solution.
> If it's ok to change the pattern I would change it directly to something
> like opp-microvolt-pvs since even pvs version is always 0.
>
> I'm honestly against the idea of defining multiple opp-table for the
> same freq (and also change the base schema) since we can do the same
> thing by just overwriting the table in the other dtsi.
The experience with msm8996 / msm8996ro showed that sharing the OPP
table between different SoCs brings more trouble than benefits. I
think it is easier to split the opp tables into per-SoC dtsi (and you
already have separate dtsi files).
Speed should be left to the speed bins of the same SoC.
Regarding speed and pvs version. I checked other platforms,
pvs_version exists only for msm8974/apq8084, while 8064 Kraits have
just speed and pvs. I'd suggest to follow the hardware/fuses and drop
the -vN for 8064 generations of the Krait.
With the opp tables being split to several DT files, I have nothing
against speed0-, it would match the apq8064 (if we ever finish the
cpufreq there).
>
> > > As you can see we use the speed value to match the different SoC and
> > > apply the correct voltage.
> > >
> > > Yes I will add the missing info in the schema.
> > >
> > > > >
> > > > > > > +
> > > > > > > struct qcom_cpufreq_drv;
> > > > > > > struct qcom_cpufreq_match_data {
> > > > > > > @@ -207,6 +211,69 @@ 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);
> > > > > > > +
> > > > > > > + switch (len) {
> > > > > > > + case 4:
> > > > > > > + get_krait_bin_format_a(cpu_dev, &speed, &pvs, &pvs_ver,
> > > > > > > + speedbin);
I think it was mentioned before, let's hardcode format_a for 8064 and
format_b for 8974/8084.
> > > > > > > + break;
> > > > > > > + default:
> > > > > > > + dev_err(cpu_dev, "Unable to read nvmem data. Defaulting to 0!\n");
> > > > > > > + ret = -ENODEV;
> > > > > > > + goto len_error;
> > > > > > > + }
> > > > > > > +
> > > > > > > + ret = qcom_smem_get_soc_id(&msm_id);
> > > > > > > + if (ret)
> > > > > > > + return ret;
> > > > > > > +
> > > > > > > + switch (msm_id) {
> > > > > > > + case QCOM_ID_IPQ8062:
> > > > > > > + drv->versions = IPQ8062_VERSION;
> > > > > > > + break;
> > > > > > > + case QCOM_ID_IPQ8064:
> > > > > > > + case QCOM_ID_IPQ8066:
> > > > > > > + case QCOM_ID_IPQ8068:
> > > > > > > + drv->versions = IPQ8064_VERSION;
> > > > > > > + break;
> > > > > > > + case QCOM_ID_IPQ8065:
> > > > > > > + case QCOM_ID_IPQ8069:
> > > > > > > + drv->versions = 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 = IPQ8062_VERSION;
> > > > > > > + break;
> > > > > > > + }
> > > > > > > +
> > > > > > > + /*
> > > > > > > + * IPQ8064 speed is never fused. Only psv values are fused.
> > > > > > > + * Set speed to the versions to permit a unified opp table.
> > > > > > > + */
> > > > > > > + snprintf(*pvs_name, sizeof("speedXX-pvsXX-vXX"), "speed%d-pvs%d-v%d",
> > > > > > > + drv->versions, pvs, pvs_ver);
> > > > > > > +
> > > > > > > +len_error:
> > > > > > > + kfree(speedbin);
> > > > > > > + return ret;
> > > > > > > +}
> > > > > > > +
> > > > > > > static int qcom_cpufreq_ipq8074_name_version(struct device *cpu_dev,
> > > > > > > struct nvmem_cell *speedbin_nvmem,
> > > > > > > char **pvs_name,
> >
> >
> > --
> > With best wishes
> > Dmitry
>
> --
> Ansuel
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RESEND PATCH v2 2/2] cpufreq: qcom-nvmem: add support for IPQ8064
2023-05-30 16:58 ` [RESEND PATCH v2 2/2] cpufreq: qcom-nvmem: add support for IPQ8064 Robert Marko
2023-05-31 2:03 ` Dmitry Baryshkov
@ 2023-05-31 8:40 ` Konrad Dybcio
2023-05-31 1:40 ` Christian Marangi
1 sibling, 1 reply; 19+ messages in thread
From: Konrad Dybcio @ 2023-05-31 8:40 UTC (permalink / raw)
To: Robert Marko, rafael, viresh.kumar, agross, andersson, ilia.lin,
linux-pm, linux-kernel, linux-arm-msm
Cc: ansuelsmth
On 30.05.2023 18:58, 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 share one OPP table, add support for
> IPQ8064 family based of SMEM SoC ID-s as speedbin fuse is always 0 on
> IPQ8064.
>
> Bit are set with the following logic:
> * IPQ8062 BIT 0
> * IPQ8064/IPQ8066/IPQ8068 BIT 1
> * IPQ8065/IPQ8069 BIT 2
>
> speed is never fused, only psv values are fused.
> Set speed to the versions to permit a unified opp table following
> this named opp:
>
> opp-microvolt-speed<SPEED_VALUE>-pvs<PSV_VALUE>-v0
>
> Example:
> - for ipq8062 psv2
> opp-microvolt-speed0-pvs2-v0 = < 925000 878750 971250>
> - for ipq8064 psv2
> opp-microvolt-speed2-pvs2-v0 = <925000 878750 971250>;
> - for ipq8065 psv2
> opp-microvolt-speed4-pvs2-v0 = <950000 902500 997500>;
>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> Signed-off-by: Robert Marko <robimarko@gmail.com>
> ---
> drivers/cpufreq/qcom-cpufreq-nvmem.c | 73 +++++++++++++++++++++++++++-
> 1 file changed, 72 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/cpufreq/qcom-cpufreq-nvmem.c b/drivers/cpufreq/qcom-cpufreq-nvmem.c
> index ce444b5962f2..c644138680ba 100644
> --- a/drivers/cpufreq/qcom-cpufreq-nvmem.c
> +++ b/drivers/cpufreq/qcom-cpufreq-nvmem.c
> @@ -34,6 +34,10 @@
> #define IPQ8074_HAWKEYE_VERSION BIT(0)
> #define IPQ8074_ACORN_VERSION BIT(1)
>
> +#define IPQ8062_VERSION BIT(0)
> +#define IPQ8064_VERSION BIT(1)
> +#define IPQ8065_VERSION BIT(2)
> +
> struct qcom_cpufreq_drv;
>
> struct qcom_cpufreq_match_data {
> @@ -207,6 +211,69 @@ 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);
> +
> + switch (len) {
Do we expect more variety here? Otherwise a switch statement sounds a
bit too heavy for the job, imo.
> + case 4:
> + get_krait_bin_format_a(cpu_dev, &speed, &pvs, &pvs_ver,
> + speedbin);
> + break;
> + default:
> + dev_err(cpu_dev, "Unable to read nvmem data. Defaulting to 0!\n");
> + ret = -ENODEV;
> + goto len_error;
> + }
> +
> + ret = qcom_smem_get_soc_id(&msm_id);
> + if (ret)
> + return ret;
> +
> + switch (msm_id) {
> + case QCOM_ID_IPQ8062:
> + drv->versions = IPQ8062_VERSION;
> + break;
> + case QCOM_ID_IPQ8064:
> + case QCOM_ID_IPQ8066:
> + case QCOM_ID_IPQ8068:
> + drv->versions = IPQ8064_VERSION;
> + break;
> + case QCOM_ID_IPQ8065:
> + case QCOM_ID_IPQ8069:
> + drv->versions = 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 = IPQ8062_VERSION;
> + break;
> + }
> +
> + /*
> + * IPQ8064 speed is never fused. Only psv values are fused.
> + * Set speed to the versions to permit a unified opp table.
> + */
> + snprintf(*pvs_name, sizeof("speedXX-pvsXX-vXX"), "speed%d-pvs%d-v%d",
> + drv->versions, pvs, pvs_ver);
> +
> +len_error:
> + kfree(speedbin);
Perhaps we should switch to devres-managed nvmem soon..
Konrad
> + return ret;
> +}
> +
> static int qcom_cpufreq_ipq8074_name_version(struct device *cpu_dev,
> struct nvmem_cell *speedbin_nvmem,
> char **pvs_name,
> @@ -256,6 +323,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,
> };
> @@ -404,7 +475,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 },
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [RESEND PATCH v2 2/2] cpufreq: qcom-nvmem: add support for IPQ8064
2023-05-31 8:40 ` Konrad Dybcio
@ 2023-05-31 1:40 ` Christian Marangi
0 siblings, 0 replies; 19+ messages in thread
From: Christian Marangi @ 2023-05-31 1:40 UTC (permalink / raw)
To: Konrad Dybcio
Cc: Robert Marko, rafael, viresh.kumar, agross, andersson, ilia.lin,
linux-pm, linux-kernel, linux-arm-msm
On Wed, May 31, 2023 at 10:40:54AM +0200, Konrad Dybcio wrote:
>
>
> On 30.05.2023 18:58, 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 share one OPP table, add support for
> > IPQ8064 family based of SMEM SoC ID-s as speedbin fuse is always 0 on
> > IPQ8064.
> >
> > Bit are set with the following logic:
> > * IPQ8062 BIT 0
> > * IPQ8064/IPQ8066/IPQ8068 BIT 1
> > * IPQ8065/IPQ8069 BIT 2
> >
> > speed is never fused, only psv values are fused.
> > Set speed to the versions to permit a unified opp table following
> > this named opp:
> >
> > opp-microvolt-speed<SPEED_VALUE>-pvs<PSV_VALUE>-v0
> >
> > Example:
> > - for ipq8062 psv2
> > opp-microvolt-speed0-pvs2-v0 = < 925000 878750 971250>
> > - for ipq8064 psv2
> > opp-microvolt-speed2-pvs2-v0 = <925000 878750 971250>;
> > - for ipq8065 psv2
> > opp-microvolt-speed4-pvs2-v0 = <950000 902500 997500>;
> >
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > Signed-off-by: Robert Marko <robimarko@gmail.com>
> > ---
> > drivers/cpufreq/qcom-cpufreq-nvmem.c | 73 +++++++++++++++++++++++++++-
> > 1 file changed, 72 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/cpufreq/qcom-cpufreq-nvmem.c b/drivers/cpufreq/qcom-cpufreq-nvmem.c
> > index ce444b5962f2..c644138680ba 100644
> > --- a/drivers/cpufreq/qcom-cpufreq-nvmem.c
> > +++ b/drivers/cpufreq/qcom-cpufreq-nvmem.c
> > @@ -34,6 +34,10 @@
> > #define IPQ8074_HAWKEYE_VERSION BIT(0)
> > #define IPQ8074_ACORN_VERSION BIT(1)
> >
> > +#define IPQ8062_VERSION BIT(0)
> > +#define IPQ8064_VERSION BIT(1)
> > +#define IPQ8065_VERSION BIT(2)
> > +
> > struct qcom_cpufreq_drv;
> >
> > struct qcom_cpufreq_match_data {
> > @@ -207,6 +211,69 @@ 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);
> > +
> > + switch (len) {
> Do we expect more variety here? Otherwise a switch statement sounds a
> bit too heavy for the job, imo.
>
Well no, considering ipq8064 is effectively EOL i guess format 4 is the
only one present. But if you check the driver this is like a pattern so
the idea was too keep that. I can totally change that to a simple
if (len != 4) if we really want.
> > + case 4:
> > + get_krait_bin_format_a(cpu_dev, &speed, &pvs, &pvs_ver,
> > + speedbin);
> > + break;
> > + default:
> > + dev_err(cpu_dev, "Unable to read nvmem data. Defaulting to 0!\n");
> > + ret = -ENODEV;
> > + goto len_error;
> > + }
> > +
> > + ret = qcom_smem_get_soc_id(&msm_id);
> > + if (ret)
> > + return ret;
> > +
> > + switch (msm_id) {
> > + case QCOM_ID_IPQ8062:
> > + drv->versions = IPQ8062_VERSION;
> > + break;
> > + case QCOM_ID_IPQ8064:
> > + case QCOM_ID_IPQ8066:
> > + case QCOM_ID_IPQ8068:
> > + drv->versions = IPQ8064_VERSION;
> > + break;
> > + case QCOM_ID_IPQ8065:
> > + case QCOM_ID_IPQ8069:
> > + drv->versions = 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 = IPQ8062_VERSION;
> > + break;
> > + }
> > +
> > + /*
> > + * IPQ8064 speed is never fused. Only psv values are fused.
> > + * Set speed to the versions to permit a unified opp table.
> > + */
> > + snprintf(*pvs_name, sizeof("speedXX-pvsXX-vXX"), "speed%d-pvs%d-v%d",
> > + drv->versions, pvs, pvs_ver);
> > +
> > +len_error:
> > + kfree(speedbin);
> Perhaps we should switch to devres-managed nvmem soon..
>
devres nvmem would be very good, maybe an idea would be search for the
actualy use of nvmem_cell_read and see if it's worth to introduce these
new API.
> > + return ret;
> > +}
> > +
> > static int qcom_cpufreq_ipq8074_name_version(struct device *cpu_dev,
> > struct nvmem_cell *speedbin_nvmem,
> > char **pvs_name,
> > @@ -256,6 +323,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,
> > };
> > @@ -404,7 +475,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 },
--
Ansuel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RESEND PATCH v2 1/2] cpufreq: qcom-nvmem: add support for IPQ8074
2023-05-30 16:58 [RESEND PATCH v2 1/2] cpufreq: qcom-nvmem: add support for IPQ8074 Robert Marko
2023-05-30 16:58 ` [RESEND PATCH v2 2/2] cpufreq: qcom-nvmem: add support for IPQ8064 Robert Marko
@ 2023-05-31 2:08 ` Dmitry Baryshkov
2023-06-01 12:55 ` Kathiravan T
2 siblings, 0 replies; 19+ messages in thread
From: Dmitry Baryshkov @ 2023-05-31 2:08 UTC (permalink / raw)
To: Robert Marko, rafael, viresh.kumar, agross, andersson,
konrad.dybcio, ilia.lin, linux-pm, linux-kernel, linux-arm-msm
Cc: ansuelsmth
On 30/05/2023 19:58, Robert Marko wrote:
> IPQ8074 comes in 2 families:
> * IPQ8070A/IPQ8071A (Acorn) up to 1.4GHz
> * IPQ8072A/IPQ8074A/IPQ8076A/IPQ8078A (Hawkeye) up to 2.2GHz
>
> So, in order to be able to share one OPP table lets add support for IPQ8074
> family based of SMEM SoC ID-s as speedbin fuse is always 0 on IPQ8074.
>
> IPQ8074 compatible is blacklisted from DT platdev as the cpufreq device
> will get created by NVMEM CPUFreq driver.
>
> Signed-off-by: Robert Marko <robimarko@gmail.com>
> ---
> Changes in v2:
> * Print an error if SMEM ID is not part of the IPQ8074 family
> and restrict the speed to Acorn variant (1.4GHz)
My comments to the patch2 apply here too. Would it be possible to add
separate dtsi files targeting each of these families?
If not, please change IPQ8074_foo_VERSIONS macros to the enum instead of
BIT(n).
> ---
> drivers/cpufreq/cpufreq-dt-platdev.c | 1 +
> drivers/cpufreq/qcom-cpufreq-nvmem.c | 43 ++++++++++++++++++++++++++++
> 2 files changed, 44 insertions(+)
>
> diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c b/drivers/cpufreq/cpufreq-dt-platdev.c
> index ea86c9f3ed7a..78f6ff933f93 100644
> --- a/drivers/cpufreq/cpufreq-dt-platdev.c
> +++ b/drivers/cpufreq/cpufreq-dt-platdev.c
> @@ -170,6 +170,7 @@ static const struct of_device_id blocklist[] __initconst = {
> { .compatible = "ti,am62a7", },
>
> { .compatible = "qcom,ipq8064", },
> + { .compatible = "qcom,ipq8074", },
> { .compatible = "qcom,apq8064", },
> { .compatible = "qcom,msm8974", },
> { .compatible = "qcom,msm8960", },
> diff --git a/drivers/cpufreq/qcom-cpufreq-nvmem.c b/drivers/cpufreq/qcom-cpufreq-nvmem.c
> index a88b6fe5db50..ce444b5962f2 100644
> --- a/drivers/cpufreq/qcom-cpufreq-nvmem.c
> +++ b/drivers/cpufreq/qcom-cpufreq-nvmem.c
> @@ -31,6 +31,9 @@
>
> #include <dt-bindings/arm/qcom,ids.h>
>
> +#define IPQ8074_HAWKEYE_VERSION BIT(0)
> +#define IPQ8074_ACORN_VERSION BIT(1)
> +
> struct qcom_cpufreq_drv;
>
> struct qcom_cpufreq_match_data {
> @@ -204,6 +207,41 @@ static int qcom_cpufreq_krait_name_version(struct device *cpu_dev,
> return ret;
> }
>
> +static int qcom_cpufreq_ipq8074_name_version(struct device *cpu_dev,
> + struct nvmem_cell *speedbin_nvmem,
> + char **pvs_name,
> + struct qcom_cpufreq_drv *drv)
> +{
> + u32 msm_id;
> + int ret;
> + *pvs_name = NULL;
> +
> + ret = qcom_smem_get_soc_id(&msm_id);
> + if (ret)
> + return ret;
> +
> + switch (msm_id) {
> + case QCOM_ID_IPQ8070A:
> + case QCOM_ID_IPQ8071A:
> + drv->versions = IPQ8074_ACORN_VERSION;
> + break;
> + case QCOM_ID_IPQ8072A:
> + case QCOM_ID_IPQ8074A:
> + case QCOM_ID_IPQ8076A:
> + case QCOM_ID_IPQ8078A:
> + drv->versions = IPQ8074_HAWKEYE_VERSION;
> + break;
> + default:
> + dev_err(cpu_dev,
> + "SoC ID %u is not part of IPQ8074 family, limiting to 1.4GHz!\n",
> + msm_id);
> + drv->versions = IPQ8074_ACORN_VERSION;
> + break;
> + }
> +
> + return 0;
> +}
> +
> static const struct qcom_cpufreq_match_data match_data_kryo = {
> .get_version = qcom_cpufreq_kryo_name_version,
> };
> @@ -218,6 +256,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_ipq8074 = {
> + .get_version = qcom_cpufreq_ipq8074_name_version,
> +};
> +
> static int qcom_cpufreq_probe(struct platform_device *pdev)
> {
> struct qcom_cpufreq_drv *drv;
> @@ -363,6 +405,7 @@ static const struct of_device_id qcom_cpufreq_match_list[] __initconst = {
> { .compatible = "qcom,msm8996", .data = &match_data_kryo },
> { .compatible = "qcom,qcs404", .data = &match_data_qcs404 },
> { .compatible = "qcom,ipq8064", .data = &match_data_krait },
> + { .compatible = "qcom,ipq8074", .data = &match_data_ipq8074 },
> { .compatible = "qcom,apq8064", .data = &match_data_krait },
> { .compatible = "qcom,msm8974", .data = &match_data_krait },
> { .compatible = "qcom,msm8960", .data = &match_data_krait },
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [RESEND PATCH v2 1/2] cpufreq: qcom-nvmem: add support for IPQ8074
2023-05-30 16:58 [RESEND PATCH v2 1/2] cpufreq: qcom-nvmem: add support for IPQ8074 Robert Marko
2023-05-30 16:58 ` [RESEND PATCH v2 2/2] cpufreq: qcom-nvmem: add support for IPQ8064 Robert Marko
2023-05-31 2:08 ` [RESEND PATCH v2 1/2] cpufreq: qcom-nvmem: add support for IPQ8074 Dmitry Baryshkov
@ 2023-06-01 12:55 ` Kathiravan T
2023-06-01 13:08 ` Konrad Dybcio
2023-06-01 13:10 ` Robert Marko
2 siblings, 2 replies; 19+ messages in thread
From: Kathiravan T @ 2023-06-01 12:55 UTC (permalink / raw)
To: Robert Marko, rafael, viresh.kumar, agross, andersson,
konrad.dybcio, ilia.lin, linux-pm, linux-kernel, linux-arm-msm
Cc: ansuelsmth
On 5/30/2023 10:28 PM, Robert Marko wrote:
> IPQ8074 comes in 2 families:
> * IPQ8070A/IPQ8071A (Acorn) up to 1.4GHz
> * IPQ8072A/IPQ8074A/IPQ8076A/IPQ8078A (Hawkeye) up to 2.2GHz
>
> So, in order to be able to share one OPP table lets add support for IPQ8074
> family based of SMEM SoC ID-s as speedbin fuse is always 0 on IPQ8074.
>
> IPQ8074 compatible is blacklisted from DT platdev as the cpufreq device
> will get created by NVMEM CPUFreq driver.
>
> Signed-off-by: Robert Marko <robimarko@gmail.com>
> ---
> Changes in v2:
> * Print an error if SMEM ID is not part of the IPQ8074 family
> and restrict the speed to Acorn variant (1.4GHz)
> ---
> drivers/cpufreq/cpufreq-dt-platdev.c | 1 +
> drivers/cpufreq/qcom-cpufreq-nvmem.c | 43 ++++++++++++++++++++++++++++
> 2 files changed, 44 insertions(+)
>
> diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c b/drivers/cpufreq/cpufreq-dt-platdev.c
> index ea86c9f3ed7a..78f6ff933f93 100644
> --- a/drivers/cpufreq/cpufreq-dt-platdev.c
> +++ b/drivers/cpufreq/cpufreq-dt-platdev.c
> @@ -170,6 +170,7 @@ static const struct of_device_id blocklist[] __initconst = {
> { .compatible = "ti,am62a7", },
>
> { .compatible = "qcom,ipq8064", },
> + { .compatible = "qcom,ipq8074", },
> { .compatible = "qcom,apq8064", },
> { .compatible = "qcom,msm8974", },
> { .compatible = "qcom,msm8960", },
> diff --git a/drivers/cpufreq/qcom-cpufreq-nvmem.c b/drivers/cpufreq/qcom-cpufreq-nvmem.c
> index a88b6fe5db50..ce444b5962f2 100644
> --- a/drivers/cpufreq/qcom-cpufreq-nvmem.c
> +++ b/drivers/cpufreq/qcom-cpufreq-nvmem.c
> @@ -31,6 +31,9 @@
>
> #include <dt-bindings/arm/qcom,ids.h>
>
> +#define IPQ8074_HAWKEYE_VERSION BIT(0)
> +#define IPQ8074_ACORN_VERSION BIT(1)
> +
> struct qcom_cpufreq_drv;
>
> struct qcom_cpufreq_match_data {
> @@ -204,6 +207,41 @@ static int qcom_cpufreq_krait_name_version(struct device *cpu_dev,
> return ret;
> }
>
> +static int qcom_cpufreq_ipq8074_name_version(struct device *cpu_dev,
> + struct nvmem_cell *speedbin_nvmem,
> + char **pvs_name,
> + struct qcom_cpufreq_drv *drv)
Most of the IPQ SoCs also supports the fuse based frequency selection.
Can we rename the function name to generic so that all the IPQ chips can
use the same function?
> +{
> + u32 msm_id;
soc_id please...?
> + int ret;
> + *pvs_name = NULL;
> +
> + ret = qcom_smem_get_soc_id(&msm_id);
> + if (ret)
> + return ret;
> +
> + switch (msm_id) {
> + case QCOM_ID_IPQ8070A:
> + case QCOM_ID_IPQ8071A:
> + drv->versions = IPQ8074_ACORN_VERSION;
> + break;
> + case QCOM_ID_IPQ8072A:
> + case QCOM_ID_IPQ8074A:
> + case QCOM_ID_IPQ8076A:
> + case QCOM_ID_IPQ8078A:
> + drv->versions = IPQ8074_HAWKEYE_VERSION;
> + break;
> + default:
> + dev_err(cpu_dev,
> + "SoC ID %u is not part of IPQ8074 family, limiting to 1.4GHz!\n",
> + msm_id);
> + drv->versions = IPQ8074_ACORN_VERSION;
> + break;
> + }
> +
> + return 0;
> +}
> +
> static const struct qcom_cpufreq_match_data match_data_kryo = {
> .get_version = qcom_cpufreq_kryo_name_version,
> };
> @@ -218,6 +256,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_ipq8074 = {
> + .get_version = qcom_cpufreq_ipq8074_name_version,
> +};
> +
> static int qcom_cpufreq_probe(struct platform_device *pdev)
> {
> struct qcom_cpufreq_drv *drv;
> @@ -363,6 +405,7 @@ static const struct of_device_id qcom_cpufreq_match_list[] __initconst = {
> { .compatible = "qcom,msm8996", .data = &match_data_kryo },
> { .compatible = "qcom,qcs404", .data = &match_data_qcs404 },
> { .compatible = "qcom,ipq8064", .data = &match_data_krait },
> + { .compatible = "qcom,ipq8074", .data = &match_data_ipq8074 },
> { .compatible = "qcom,apq8064", .data = &match_data_krait },
> { .compatible = "qcom,msm8974", .data = &match_data_krait },
> { .compatible = "qcom,msm8960", .data = &match_data_krait },
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [RESEND PATCH v2 1/2] cpufreq: qcom-nvmem: add support for IPQ8074
2023-06-01 12:55 ` Kathiravan T
@ 2023-06-01 13:08 ` Konrad Dybcio
2023-06-01 13:10 ` Robert Marko
1 sibling, 0 replies; 19+ messages in thread
From: Konrad Dybcio @ 2023-06-01 13:08 UTC (permalink / raw)
To: Kathiravan T, Robert Marko, rafael, viresh.kumar, agross,
andersson, ilia.lin, linux-pm, linux-kernel, linux-arm-msm
Cc: ansuelsmth
On 1.06.2023 14:55, Kathiravan T wrote:
>
> On 5/30/2023 10:28 PM, Robert Marko wrote:
>> IPQ8074 comes in 2 families:
>> * IPQ8070A/IPQ8071A (Acorn) up to 1.4GHz
>> * IPQ8072A/IPQ8074A/IPQ8076A/IPQ8078A (Hawkeye) up to 2.2GHz
>>
>> So, in order to be able to share one OPP table lets add support for IPQ8074
>> family based of SMEM SoC ID-s as speedbin fuse is always 0 on IPQ8074.
>>
>> IPQ8074 compatible is blacklisted from DT platdev as the cpufreq device
>> will get created by NVMEM CPUFreq driver.
>>
>> Signed-off-by: Robert Marko <robimarko@gmail.com>
>> ---
>> Changes in v2:
>> * Print an error if SMEM ID is not part of the IPQ8074 family
>> and restrict the speed to Acorn variant (1.4GHz)
>> ---
>> drivers/cpufreq/cpufreq-dt-platdev.c | 1 +
>> drivers/cpufreq/qcom-cpufreq-nvmem.c | 43 ++++++++++++++++++++++++++++
>> 2 files changed, 44 insertions(+)
>>
>> diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c b/drivers/cpufreq/cpufreq-dt-platdev.c
>> index ea86c9f3ed7a..78f6ff933f93 100644
>> --- a/drivers/cpufreq/cpufreq-dt-platdev.c
>> +++ b/drivers/cpufreq/cpufreq-dt-platdev.c
>> @@ -170,6 +170,7 @@ static const struct of_device_id blocklist[] __initconst = {
>> { .compatible = "ti,am62a7", },
>> { .compatible = "qcom,ipq8064", },
>> + { .compatible = "qcom,ipq8074", },
>> { .compatible = "qcom,apq8064", },
>> { .compatible = "qcom,msm8974", },
>> { .compatible = "qcom,msm8960", },
>> diff --git a/drivers/cpufreq/qcom-cpufreq-nvmem.c b/drivers/cpufreq/qcom-cpufreq-nvmem.c
>> index a88b6fe5db50..ce444b5962f2 100644
>> --- a/drivers/cpufreq/qcom-cpufreq-nvmem.c
>> +++ b/drivers/cpufreq/qcom-cpufreq-nvmem.c
>> @@ -31,6 +31,9 @@
>> #include <dt-bindings/arm/qcom,ids.h>
>> +#define IPQ8074_HAWKEYE_VERSION BIT(0)
>> +#define IPQ8074_ACORN_VERSION BIT(1)
>> +
>> struct qcom_cpufreq_drv;
>> struct qcom_cpufreq_match_data {
>> @@ -204,6 +207,41 @@ static int qcom_cpufreq_krait_name_version(struct device *cpu_dev,
>> return ret;
>> }
>> +static int qcom_cpufreq_ipq8074_name_version(struct device *cpu_dev,
>> + struct nvmem_cell *speedbin_nvmem,
>> + char **pvs_name,
>> + struct qcom_cpufreq_drv *drv)
>
>
> Most of the IPQ SoCs also supports the fuse based frequency selection. Can we rename the function name to generic so that all the IPQ chips can use the same function?
This should stay SoC-specific as we can't really share the speedbin
bit vals reasonably, unless you do something like:
case QCOM_ID_FOO:
case QCOM_ID_BAR:
drv->versions = SPEED_BIN_0; // BIT(0)
break;
case QCOM_ID_BAZ:
drv->versions = SPEED_BIN_1; // BIT(1)
Konrad
>
>
>> +{
>> + u32 msm_id;
>
>
> soc_id please...?
>
>
>> + int ret;
>> + *pvs_name = NULL;
>> +
>> + ret = qcom_smem_get_soc_id(&msm_id);
>> + if (ret)
>> + return ret;
>> +
>> + switch (msm_id) {
>> + case QCOM_ID_IPQ8070A:
>> + case QCOM_ID_IPQ8071A:
>> + drv->versions = IPQ8074_ACORN_VERSION;
>> + break;
>> + case QCOM_ID_IPQ8072A:
>> + case QCOM_ID_IPQ8074A:
>> + case QCOM_ID_IPQ8076A:
>> + case QCOM_ID_IPQ8078A:
>> + drv->versions = IPQ8074_HAWKEYE_VERSION;
>> + break;
>> + default:
>> + dev_err(cpu_dev,
>> + "SoC ID %u is not part of IPQ8074 family, limiting to 1.4GHz!\n",
>> + msm_id);
>> + drv->versions = IPQ8074_ACORN_VERSION;
>> + break;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static const struct qcom_cpufreq_match_data match_data_kryo = {
>> .get_version = qcom_cpufreq_kryo_name_version,
>> };
>> @@ -218,6 +256,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_ipq8074 = {
>> + .get_version = qcom_cpufreq_ipq8074_name_version,
>> +};
>> +
>> static int qcom_cpufreq_probe(struct platform_device *pdev)
>> {
>> struct qcom_cpufreq_drv *drv;
>> @@ -363,6 +405,7 @@ static const struct of_device_id qcom_cpufreq_match_list[] __initconst = {
>> { .compatible = "qcom,msm8996", .data = &match_data_kryo },
>> { .compatible = "qcom,qcs404", .data = &match_data_qcs404 },
>> { .compatible = "qcom,ipq8064", .data = &match_data_krait },
>> + { .compatible = "qcom,ipq8074", .data = &match_data_ipq8074 },
>> { .compatible = "qcom,apq8064", .data = &match_data_krait },
>> { .compatible = "qcom,msm8974", .data = &match_data_krait },
>> { .compatible = "qcom,msm8960", .data = &match_data_krait },
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [RESEND PATCH v2 1/2] cpufreq: qcom-nvmem: add support for IPQ8074
2023-06-01 12:55 ` Kathiravan T
2023-06-01 13:08 ` Konrad Dybcio
@ 2023-06-01 13:10 ` Robert Marko
2023-06-01 13:24 ` Kathiravan T
1 sibling, 1 reply; 19+ messages in thread
From: Robert Marko @ 2023-06-01 13:10 UTC (permalink / raw)
To: Kathiravan T
Cc: rafael, viresh.kumar, agross, andersson, konrad.dybcio, ilia.lin,
linux-pm, linux-kernel, linux-arm-msm, ansuelsmth
On Thu, 1 Jun 2023 at 14:57, Kathiravan T <quic_kathirav@quicinc.com> wrote:
>
>
> On 5/30/2023 10:28 PM, Robert Marko wrote:
> > IPQ8074 comes in 2 families:
> > * IPQ8070A/IPQ8071A (Acorn) up to 1.4GHz
> > * IPQ8072A/IPQ8074A/IPQ8076A/IPQ8078A (Hawkeye) up to 2.2GHz
> >
> > So, in order to be able to share one OPP table lets add support for IPQ8074
> > family based of SMEM SoC ID-s as speedbin fuse is always 0 on IPQ8074.
> >
> > IPQ8074 compatible is blacklisted from DT platdev as the cpufreq device
> > will get created by NVMEM CPUFreq driver.
> >
> > Signed-off-by: Robert Marko <robimarko@gmail.com>
> > ---
> > Changes in v2:
> > * Print an error if SMEM ID is not part of the IPQ8074 family
> > and restrict the speed to Acorn variant (1.4GHz)
> > ---
> > drivers/cpufreq/cpufreq-dt-platdev.c | 1 +
> > drivers/cpufreq/qcom-cpufreq-nvmem.c | 43 ++++++++++++++++++++++++++++
> > 2 files changed, 44 insertions(+)
> >
> > diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c b/drivers/cpufreq/cpufreq-dt-platdev.c
> > index ea86c9f3ed7a..78f6ff933f93 100644
> > --- a/drivers/cpufreq/cpufreq-dt-platdev.c
> > +++ b/drivers/cpufreq/cpufreq-dt-platdev.c
> > @@ -170,6 +170,7 @@ static const struct of_device_id blocklist[] __initconst = {
> > { .compatible = "ti,am62a7", },
> >
> > { .compatible = "qcom,ipq8064", },
> > + { .compatible = "qcom,ipq8074", },
> > { .compatible = "qcom,apq8064", },
> > { .compatible = "qcom,msm8974", },
> > { .compatible = "qcom,msm8960", },
> > diff --git a/drivers/cpufreq/qcom-cpufreq-nvmem.c b/drivers/cpufreq/qcom-cpufreq-nvmem.c
> > index a88b6fe5db50..ce444b5962f2 100644
> > --- a/drivers/cpufreq/qcom-cpufreq-nvmem.c
> > +++ b/drivers/cpufreq/qcom-cpufreq-nvmem.c
> > @@ -31,6 +31,9 @@
> >
> > #include <dt-bindings/arm/qcom,ids.h>
> >
> > +#define IPQ8074_HAWKEYE_VERSION BIT(0)
> > +#define IPQ8074_ACORN_VERSION BIT(1)
> > +
> > struct qcom_cpufreq_drv;
> >
> > struct qcom_cpufreq_match_data {
> > @@ -204,6 +207,41 @@ static int qcom_cpufreq_krait_name_version(struct device *cpu_dev,
> > return ret;
> > }
> >
> > +static int qcom_cpufreq_ipq8074_name_version(struct device *cpu_dev,
> > + struct nvmem_cell *speedbin_nvmem,
> > + char **pvs_name,
> > + struct qcom_cpufreq_drv *drv)
>
>
> Most of the IPQ SoCs also supports the fuse based frequency selection.
> Can we rename the function name to generic so that all the IPQ chips can
> use the same function?
Well, the only speedbin fuse I was able to dig from downstream is the one from
CPR driver and that one is 0 on all devices so it's not helpful.
Do you maybe know if there is one in the IPQ8074 family?
Function is not supposed to be shared between SoC-s, so I dont see a point in it
having a generic name cause for example IPQ6018 has a working fuse and its logic
is completely different for setting the versioning than IPQ8074, I
dont think having a
catch-all would work here.
>
>
> > +{
> > + u32 msm_id;
>
>
> soc_id please...?
Sure, that is more suitable.
Regards,
Robert
>
>
> > + int ret;
> > + *pvs_name = NULL;
> > +
> > + ret = qcom_smem_get_soc_id(&msm_id);
> > + if (ret)
> > + return ret;
> > +
> > + switch (msm_id) {
> > + case QCOM_ID_IPQ8070A:
> > + case QCOM_ID_IPQ8071A:
> > + drv->versions = IPQ8074_ACORN_VERSION;
> > + break;
> > + case QCOM_ID_IPQ8072A:
> > + case QCOM_ID_IPQ8074A:
> > + case QCOM_ID_IPQ8076A:
> > + case QCOM_ID_IPQ8078A:
> > + drv->versions = IPQ8074_HAWKEYE_VERSION;
> > + break;
> > + default:
> > + dev_err(cpu_dev,
> > + "SoC ID %u is not part of IPQ8074 family, limiting to 1.4GHz!\n",
> > + msm_id);
> > + drv->versions = IPQ8074_ACORN_VERSION;
> > + break;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static const struct qcom_cpufreq_match_data match_data_kryo = {
> > .get_version = qcom_cpufreq_kryo_name_version,
> > };
> > @@ -218,6 +256,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_ipq8074 = {
> > + .get_version = qcom_cpufreq_ipq8074_name_version,
> > +};
> > +
> > static int qcom_cpufreq_probe(struct platform_device *pdev)
> > {
> > struct qcom_cpufreq_drv *drv;
> > @@ -363,6 +405,7 @@ static const struct of_device_id qcom_cpufreq_match_list[] __initconst = {
> > { .compatible = "qcom,msm8996", .data = &match_data_kryo },
> > { .compatible = "qcom,qcs404", .data = &match_data_qcs404 },
> > { .compatible = "qcom,ipq8064", .data = &match_data_krait },
> > + { .compatible = "qcom,ipq8074", .data = &match_data_ipq8074 },
> > { .compatible = "qcom,apq8064", .data = &match_data_krait },
> > { .compatible = "qcom,msm8974", .data = &match_data_krait },
> > { .compatible = "qcom,msm8960", .data = &match_data_krait },
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [RESEND PATCH v2 1/2] cpufreq: qcom-nvmem: add support for IPQ8074
2023-06-01 13:10 ` Robert Marko
@ 2023-06-01 13:24 ` Kathiravan T
2023-06-01 14:49 ` Kathiravan T
0 siblings, 1 reply; 19+ messages in thread
From: Kathiravan T @ 2023-06-01 13:24 UTC (permalink / raw)
To: Robert Marko
Cc: rafael, viresh.kumar, agross, andersson, konrad.dybcio, ilia.lin,
linux-pm, linux-kernel, linux-arm-msm, ansuelsmth
On 6/1/2023 6:40 PM, Robert Marko wrote:
> On Thu, 1 Jun 2023 at 14:57, Kathiravan T <quic_kathirav@quicinc.com> wrote:
>>
>> On 5/30/2023 10:28 PM, Robert Marko wrote:
>>> IPQ8074 comes in 2 families:
>>> * IPQ8070A/IPQ8071A (Acorn) up to 1.4GHz
>>> * IPQ8072A/IPQ8074A/IPQ8076A/IPQ8078A (Hawkeye) up to 2.2GHz
>>>
>>> So, in order to be able to share one OPP table lets add support for IPQ8074
>>> family based of SMEM SoC ID-s as speedbin fuse is always 0 on IPQ8074.
>>>
>>> IPQ8074 compatible is blacklisted from DT platdev as the cpufreq device
>>> will get created by NVMEM CPUFreq driver.
>>>
>>> Signed-off-by: Robert Marko <robimarko@gmail.com>
>>> ---
>>> Changes in v2:
>>> * Print an error if SMEM ID is not part of the IPQ8074 family
>>> and restrict the speed to Acorn variant (1.4GHz)
>>> ---
>>> drivers/cpufreq/cpufreq-dt-platdev.c | 1 +
>>> drivers/cpufreq/qcom-cpufreq-nvmem.c | 43 ++++++++++++++++++++++++++++
>>> 2 files changed, 44 insertions(+)
>>>
>>> diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c b/drivers/cpufreq/cpufreq-dt-platdev.c
>>> index ea86c9f3ed7a..78f6ff933f93 100644
>>> --- a/drivers/cpufreq/cpufreq-dt-platdev.c
>>> +++ b/drivers/cpufreq/cpufreq-dt-platdev.c
>>> @@ -170,6 +170,7 @@ static const struct of_device_id blocklist[] __initconst = {
>>> { .compatible = "ti,am62a7", },
>>>
>>> { .compatible = "qcom,ipq8064", },
>>> + { .compatible = "qcom,ipq8074", },
>>> { .compatible = "qcom,apq8064", },
>>> { .compatible = "qcom,msm8974", },
>>> { .compatible = "qcom,msm8960", },
>>> diff --git a/drivers/cpufreq/qcom-cpufreq-nvmem.c b/drivers/cpufreq/qcom-cpufreq-nvmem.c
>>> index a88b6fe5db50..ce444b5962f2 100644
>>> --- a/drivers/cpufreq/qcom-cpufreq-nvmem.c
>>> +++ b/drivers/cpufreq/qcom-cpufreq-nvmem.c
>>> @@ -31,6 +31,9 @@
>>>
>>> #include <dt-bindings/arm/qcom,ids.h>
>>>
>>> +#define IPQ8074_HAWKEYE_VERSION BIT(0)
>>> +#define IPQ8074_ACORN_VERSION BIT(1)
>>> +
>>> struct qcom_cpufreq_drv;
>>>
>>> struct qcom_cpufreq_match_data {
>>> @@ -204,6 +207,41 @@ static int qcom_cpufreq_krait_name_version(struct device *cpu_dev,
>>> return ret;
>>> }
>>>
>>> +static int qcom_cpufreq_ipq8074_name_version(struct device *cpu_dev,
>>> + struct nvmem_cell *speedbin_nvmem,
>>> + char **pvs_name,
>>> + struct qcom_cpufreq_drv *drv)
>>
>> Most of the IPQ SoCs also supports the fuse based frequency selection.
>> Can we rename the function name to generic so that all the IPQ chips can
>> use the same function?
> Well, the only speedbin fuse I was able to dig from downstream is the one from
> CPR driver and that one is 0 on all devices so it's not helpful.
> Do you maybe know if there is one in the IPQ8074 family?
Let me check on this and get back to you probably by tomorrow...
>
> Function is not supposed to be shared between SoC-s, so I dont see a point in it
> having a generic name cause for example IPQ6018 has a working fuse and its logic
> is completely different for setting the versioning than IPQ8074, I
> dont think having a
> catch-all would work here.
Makes sense, thanks Robert and Konrad.
>
>>
>>> +{
>>> + u32 msm_id;
>>
>> soc_id please...?
> Sure, that is more suitable.
>
> Regards,
> Robert
>>
>>> + int ret;
>>> + *pvs_name = NULL;
>>> +
>>> + ret = qcom_smem_get_soc_id(&msm_id);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + switch (msm_id) {
>>> + case QCOM_ID_IPQ8070A:
>>> + case QCOM_ID_IPQ8071A:
>>> + drv->versions = IPQ8074_ACORN_VERSION;
>>> + break;
>>> + case QCOM_ID_IPQ8072A:
>>> + case QCOM_ID_IPQ8074A:
>>> + case QCOM_ID_IPQ8076A:
>>> + case QCOM_ID_IPQ8078A:
>>> + drv->versions = IPQ8074_HAWKEYE_VERSION;
>>> + break;
>>> + default:
>>> + dev_err(cpu_dev,
>>> + "SoC ID %u is not part of IPQ8074 family, limiting to 1.4GHz!\n",
>>> + msm_id);
>>> + drv->versions = IPQ8074_ACORN_VERSION;
>>> + break;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> static const struct qcom_cpufreq_match_data match_data_kryo = {
>>> .get_version = qcom_cpufreq_kryo_name_version,
>>> };
>>> @@ -218,6 +256,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_ipq8074 = {
>>> + .get_version = qcom_cpufreq_ipq8074_name_version,
>>> +};
>>> +
>>> static int qcom_cpufreq_probe(struct platform_device *pdev)
>>> {
>>> struct qcom_cpufreq_drv *drv;
>>> @@ -363,6 +405,7 @@ static const struct of_device_id qcom_cpufreq_match_list[] __initconst = {
>>> { .compatible = "qcom,msm8996", .data = &match_data_kryo },
>>> { .compatible = "qcom,qcs404", .data = &match_data_qcs404 },
>>> { .compatible = "qcom,ipq8064", .data = &match_data_krait },
>>> + { .compatible = "qcom,ipq8074", .data = &match_data_ipq8074 },
>>> { .compatible = "qcom,apq8064", .data = &match_data_krait },
>>> { .compatible = "qcom,msm8974", .data = &match_data_krait },
>>> { .compatible = "qcom,msm8960", .data = &match_data_krait },
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [RESEND PATCH v2 1/2] cpufreq: qcom-nvmem: add support for IPQ8074
2023-06-01 13:24 ` Kathiravan T
@ 2023-06-01 14:49 ` Kathiravan T
2023-06-01 14:55 ` Robert Marko
0 siblings, 1 reply; 19+ messages in thread
From: Kathiravan T @ 2023-06-01 14:49 UTC (permalink / raw)
To: Robert Marko
Cc: rafael, viresh.kumar, agross, andersson, konrad.dybcio, ilia.lin,
linux-pm, linux-kernel, linux-arm-msm, ansuelsmth
On 6/1/2023 6:54 PM, Kathiravan T wrote:
>
> On 6/1/2023 6:40 PM, Robert Marko wrote:
>> On Thu, 1 Jun 2023 at 14:57, Kathiravan T <quic_kathirav@quicinc.com>
>> wrote:
>>>
>>> On 5/30/2023 10:28 PM, Robert Marko wrote:
>>>> IPQ8074 comes in 2 families:
>>>> * IPQ8070A/IPQ8071A (Acorn) up to 1.4GHz
>>>> * IPQ8072A/IPQ8074A/IPQ8076A/IPQ8078A (Hawkeye) up to 2.2GHz
>>>>
>>>> So, in order to be able to share one OPP table lets add support for
>>>> IPQ8074
>>>> family based of SMEM SoC ID-s as speedbin fuse is always 0 on IPQ8074.
>>>>
>>>> IPQ8074 compatible is blacklisted from DT platdev as the cpufreq
>>>> device
>>>> will get created by NVMEM CPUFreq driver.
>>>>
>>>> Signed-off-by: Robert Marko <robimarko@gmail.com>
>>>> ---
>>>> Changes in v2:
>>>> * Print an error if SMEM ID is not part of the IPQ8074 family
>>>> and restrict the speed to Acorn variant (1.4GHz)
>>>> ---
>>>> drivers/cpufreq/cpufreq-dt-platdev.c | 1 +
>>>> drivers/cpufreq/qcom-cpufreq-nvmem.c | 43
>>>> ++++++++++++++++++++++++++++
>>>> 2 files changed, 44 insertions(+)
>>>>
>>>> diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c
>>>> b/drivers/cpufreq/cpufreq-dt-platdev.c
>>>> index ea86c9f3ed7a..78f6ff933f93 100644
>>>> --- a/drivers/cpufreq/cpufreq-dt-platdev.c
>>>> +++ b/drivers/cpufreq/cpufreq-dt-platdev.c
>>>> @@ -170,6 +170,7 @@ static const struct of_device_id blocklist[]
>>>> __initconst = {
>>>> { .compatible = "ti,am62a7", },
>>>>
>>>> { .compatible = "qcom,ipq8064", },
>>>> + { .compatible = "qcom,ipq8074", },
>>>> { .compatible = "qcom,apq8064", },
>>>> { .compatible = "qcom,msm8974", },
>>>> { .compatible = "qcom,msm8960", },
>>>> diff --git a/drivers/cpufreq/qcom-cpufreq-nvmem.c
>>>> b/drivers/cpufreq/qcom-cpufreq-nvmem.c
>>>> index a88b6fe5db50..ce444b5962f2 100644
>>>> --- a/drivers/cpufreq/qcom-cpufreq-nvmem.c
>>>> +++ b/drivers/cpufreq/qcom-cpufreq-nvmem.c
>>>> @@ -31,6 +31,9 @@
>>>>
>>>> #include <dt-bindings/arm/qcom,ids.h>
>>>>
>>>> +#define IPQ8074_HAWKEYE_VERSION BIT(0)
>>>> +#define IPQ8074_ACORN_VERSION BIT(1)
>>>> +
>>>> struct qcom_cpufreq_drv;
>>>>
>>>> struct qcom_cpufreq_match_data {
>>>> @@ -204,6 +207,41 @@ static int
>>>> qcom_cpufreq_krait_name_version(struct device *cpu_dev,
>>>> return ret;
>>>> }
>>>>
>>>> +static int qcom_cpufreq_ipq8074_name_version(struct device *cpu_dev,
>>>> + struct nvmem_cell
>>>> *speedbin_nvmem,
>>>> + char **pvs_name,
>>>> + struct qcom_cpufreq_drv
>>>> *drv)
>>>
>>> Most of the IPQ SoCs also supports the fuse based frequency selection.
>>> Can we rename the function name to generic so that all the IPQ chips
>>> can
>>> use the same function?
>> Well, the only speedbin fuse I was able to dig from downstream is the
>> one from
>> CPR driver and that one is 0 on all devices so it's not helpful.
>> Do you maybe know if there is one in the IPQ8074 family?
>
>
> Let me check on this and get back to you probably by tomorrow...
Robert, checked with the team and IPQ807x doesn't use fuse to determine
the CPU freq limits. Current approach (SoC ID based) should be fine.
BTW, are the DTS changes already posted or yet to be posted?
>
>
>>
>> Function is not supposed to be shared between SoC-s, so I dont see a
>> point in it
>> having a generic name cause for example IPQ6018 has a working fuse
>> and its logic
>> is completely different for setting the versioning than IPQ8074, I
>> dont think having a
>> catch-all would work here.
>
>
> Makes sense, thanks Robert and Konrad.
>
>
>>
>>>
>>>> +{
>>>> + u32 msm_id;
>>>
>>> soc_id please...?
>> Sure, that is more suitable.
>>
>> Regards,
>> Robert
>>>
>>>> + int ret;
>>>> + *pvs_name = NULL;
>>>> +
>>>> + ret = qcom_smem_get_soc_id(&msm_id);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + switch (msm_id) {
>>>> + case QCOM_ID_IPQ8070A:
>>>> + case QCOM_ID_IPQ8071A:
>>>> + drv->versions = IPQ8074_ACORN_VERSION;
>>>> + break;
>>>> + case QCOM_ID_IPQ8072A:
>>>> + case QCOM_ID_IPQ8074A:
>>>> + case QCOM_ID_IPQ8076A:
>>>> + case QCOM_ID_IPQ8078A:
>>>> + drv->versions = IPQ8074_HAWKEYE_VERSION;
>>>> + break;
>>>> + default:
>>>> + dev_err(cpu_dev,
>>>> + "SoC ID %u is not part of IPQ8074 family,
>>>> limiting to 1.4GHz!\n",
>>>> + msm_id);
>>>> + drv->versions = IPQ8074_ACORN_VERSION;
>>>> + break;
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> static const struct qcom_cpufreq_match_data match_data_kryo = {
>>>> .get_version = qcom_cpufreq_kryo_name_version,
>>>> };
>>>> @@ -218,6 +256,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_ipq8074 = {
>>>> + .get_version = qcom_cpufreq_ipq8074_name_version,
>>>> +};
>>>> +
>>>> static int qcom_cpufreq_probe(struct platform_device *pdev)
>>>> {
>>>> struct qcom_cpufreq_drv *drv;
>>>> @@ -363,6 +405,7 @@ static const struct of_device_id
>>>> qcom_cpufreq_match_list[] __initconst = {
>>>> { .compatible = "qcom,msm8996", .data = &match_data_kryo },
>>>> { .compatible = "qcom,qcs404", .data = &match_data_qcs404 },
>>>> { .compatible = "qcom,ipq8064", .data = &match_data_krait },
>>>> + { .compatible = "qcom,ipq8074", .data = &match_data_ipq8074 },
>>>> { .compatible = "qcom,apq8064", .data = &match_data_krait },
>>>> { .compatible = "qcom,msm8974", .data = &match_data_krait },
>>>> { .compatible = "qcom,msm8960", .data = &match_data_krait },
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [RESEND PATCH v2 1/2] cpufreq: qcom-nvmem: add support for IPQ8074
2023-06-01 14:49 ` Kathiravan T
@ 2023-06-01 14:55 ` Robert Marko
2023-06-02 8:57 ` Konrad Dybcio
0 siblings, 1 reply; 19+ messages in thread
From: Robert Marko @ 2023-06-01 14:55 UTC (permalink / raw)
To: Kathiravan T
Cc: rafael, viresh.kumar, agross, andersson, konrad.dybcio, ilia.lin,
linux-pm, linux-kernel, linux-arm-msm, ansuelsmth
On Thu, 1 Jun 2023 at 16:49, Kathiravan T <quic_kathirav@quicinc.com> wrote:
>
>
> On 6/1/2023 6:54 PM, Kathiravan T wrote:
> >
> > On 6/1/2023 6:40 PM, Robert Marko wrote:
> >> On Thu, 1 Jun 2023 at 14:57, Kathiravan T <quic_kathirav@quicinc.com>
> >> wrote:
> >>>
> >>> On 5/30/2023 10:28 PM, Robert Marko wrote:
> >>>> IPQ8074 comes in 2 families:
> >>>> * IPQ8070A/IPQ8071A (Acorn) up to 1.4GHz
> >>>> * IPQ8072A/IPQ8074A/IPQ8076A/IPQ8078A (Hawkeye) up to 2.2GHz
> >>>>
> >>>> So, in order to be able to share one OPP table lets add support for
> >>>> IPQ8074
> >>>> family based of SMEM SoC ID-s as speedbin fuse is always 0 on IPQ8074.
> >>>>
> >>>> IPQ8074 compatible is blacklisted from DT platdev as the cpufreq
> >>>> device
> >>>> will get created by NVMEM CPUFreq driver.
> >>>>
> >>>> Signed-off-by: Robert Marko <robimarko@gmail.com>
> >>>> ---
> >>>> Changes in v2:
> >>>> * Print an error if SMEM ID is not part of the IPQ8074 family
> >>>> and restrict the speed to Acorn variant (1.4GHz)
> >>>> ---
> >>>> drivers/cpufreq/cpufreq-dt-platdev.c | 1 +
> >>>> drivers/cpufreq/qcom-cpufreq-nvmem.c | 43
> >>>> ++++++++++++++++++++++++++++
> >>>> 2 files changed, 44 insertions(+)
> >>>>
> >>>> diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c
> >>>> b/drivers/cpufreq/cpufreq-dt-platdev.c
> >>>> index ea86c9f3ed7a..78f6ff933f93 100644
> >>>> --- a/drivers/cpufreq/cpufreq-dt-platdev.c
> >>>> +++ b/drivers/cpufreq/cpufreq-dt-platdev.c
> >>>> @@ -170,6 +170,7 @@ static const struct of_device_id blocklist[]
> >>>> __initconst = {
> >>>> { .compatible = "ti,am62a7", },
> >>>>
> >>>> { .compatible = "qcom,ipq8064", },
> >>>> + { .compatible = "qcom,ipq8074", },
> >>>> { .compatible = "qcom,apq8064", },
> >>>> { .compatible = "qcom,msm8974", },
> >>>> { .compatible = "qcom,msm8960", },
> >>>> diff --git a/drivers/cpufreq/qcom-cpufreq-nvmem.c
> >>>> b/drivers/cpufreq/qcom-cpufreq-nvmem.c
> >>>> index a88b6fe5db50..ce444b5962f2 100644
> >>>> --- a/drivers/cpufreq/qcom-cpufreq-nvmem.c
> >>>> +++ b/drivers/cpufreq/qcom-cpufreq-nvmem.c
> >>>> @@ -31,6 +31,9 @@
> >>>>
> >>>> #include <dt-bindings/arm/qcom,ids.h>
> >>>>
> >>>> +#define IPQ8074_HAWKEYE_VERSION BIT(0)
> >>>> +#define IPQ8074_ACORN_VERSION BIT(1)
> >>>> +
> >>>> struct qcom_cpufreq_drv;
> >>>>
> >>>> struct qcom_cpufreq_match_data {
> >>>> @@ -204,6 +207,41 @@ static int
> >>>> qcom_cpufreq_krait_name_version(struct device *cpu_dev,
> >>>> return ret;
> >>>> }
> >>>>
> >>>> +static int qcom_cpufreq_ipq8074_name_version(struct device *cpu_dev,
> >>>> + struct nvmem_cell
> >>>> *speedbin_nvmem,
> >>>> + char **pvs_name,
> >>>> + struct qcom_cpufreq_drv
> >>>> *drv)
> >>>
> >>> Most of the IPQ SoCs also supports the fuse based frequency selection.
> >>> Can we rename the function name to generic so that all the IPQ chips
> >>> can
> >>> use the same function?
> >> Well, the only speedbin fuse I was able to dig from downstream is the
> >> one from
> >> CPR driver and that one is 0 on all devices so it's not helpful.
> >> Do you maybe know if there is one in the IPQ8074 family?
> >
> >
> > Let me check on this and get back to you probably by tomorrow...
>
>
> Robert, checked with the team and IPQ807x doesn't use fuse to determine
> the CPU freq limits. Current approach (SoC ID based) should be fine.
> BTW, are the DTS changes already posted or yet to be posted?
Thanks for checking,
DTS changes are not posted as CPR support is required in order for scaling to
properly work, otherwise, all I could do is try and guess some safe voltages.
There was an effort to get CPR upstreamed, but I think that stalled out for now.
Regards,
Robert
>
>
> >
> >
> >>
> >> Function is not supposed to be shared between SoC-s, so I dont see a
> >> point in it
> >> having a generic name cause for example IPQ6018 has a working fuse
> >> and its logic
> >> is completely different for setting the versioning than IPQ8074, I
> >> dont think having a
> >> catch-all would work here.
> >
> >
> > Makes sense, thanks Robert and Konrad.
> >
> >
> >>
> >>>
> >>>> +{
> >>>> + u32 msm_id;
> >>>
> >>> soc_id please...?
> >> Sure, that is more suitable.
> >>
> >> Regards,
> >> Robert
> >>>
> >>>> + int ret;
> >>>> + *pvs_name = NULL;
> >>>> +
> >>>> + ret = qcom_smem_get_soc_id(&msm_id);
> >>>> + if (ret)
> >>>> + return ret;
> >>>> +
> >>>> + switch (msm_id) {
> >>>> + case QCOM_ID_IPQ8070A:
> >>>> + case QCOM_ID_IPQ8071A:
> >>>> + drv->versions = IPQ8074_ACORN_VERSION;
> >>>> + break;
> >>>> + case QCOM_ID_IPQ8072A:
> >>>> + case QCOM_ID_IPQ8074A:
> >>>> + case QCOM_ID_IPQ8076A:
> >>>> + case QCOM_ID_IPQ8078A:
> >>>> + drv->versions = IPQ8074_HAWKEYE_VERSION;
> >>>> + break;
> >>>> + default:
> >>>> + dev_err(cpu_dev,
> >>>> + "SoC ID %u is not part of IPQ8074 family,
> >>>> limiting to 1.4GHz!\n",
> >>>> + msm_id);
> >>>> + drv->versions = IPQ8074_ACORN_VERSION;
> >>>> + break;
> >>>> + }
> >>>> +
> >>>> + return 0;
> >>>> +}
> >>>> +
> >>>> static const struct qcom_cpufreq_match_data match_data_kryo = {
> >>>> .get_version = qcom_cpufreq_kryo_name_version,
> >>>> };
> >>>> @@ -218,6 +256,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_ipq8074 = {
> >>>> + .get_version = qcom_cpufreq_ipq8074_name_version,
> >>>> +};
> >>>> +
> >>>> static int qcom_cpufreq_probe(struct platform_device *pdev)
> >>>> {
> >>>> struct qcom_cpufreq_drv *drv;
> >>>> @@ -363,6 +405,7 @@ static const struct of_device_id
> >>>> qcom_cpufreq_match_list[] __initconst = {
> >>>> { .compatible = "qcom,msm8996", .data = &match_data_kryo },
> >>>> { .compatible = "qcom,qcs404", .data = &match_data_qcs404 },
> >>>> { .compatible = "qcom,ipq8064", .data = &match_data_krait },
> >>>> + { .compatible = "qcom,ipq8074", .data = &match_data_ipq8074 },
> >>>> { .compatible = "qcom,apq8064", .data = &match_data_krait },
> >>>> { .compatible = "qcom,msm8974", .data = &match_data_krait },
> >>>> { .compatible = "qcom,msm8960", .data = &match_data_krait },
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [RESEND PATCH v2 1/2] cpufreq: qcom-nvmem: add support for IPQ8074
2023-06-01 14:55 ` Robert Marko
@ 2023-06-02 8:57 ` Konrad Dybcio
0 siblings, 0 replies; 19+ messages in thread
From: Konrad Dybcio @ 2023-06-02 8:57 UTC (permalink / raw)
To: Robert Marko, Kathiravan T
Cc: rafael, viresh.kumar, agross, andersson, ilia.lin, linux-pm,
linux-kernel, linux-arm-msm, ansuelsmth
On 1.06.2023 16:55, Robert Marko wrote:
> On Thu, 1 Jun 2023 at 16:49, Kathiravan T <quic_kathirav@quicinc.com> wrote:
>>
>>
>> On 6/1/2023 6:54 PM, Kathiravan T wrote:
>>>
>>> On 6/1/2023 6:40 PM, Robert Marko wrote:
>>>> On Thu, 1 Jun 2023 at 14:57, Kathiravan T <quic_kathirav@quicinc.com>
>>>> wrote:
>>>>>
>>>>> On 5/30/2023 10:28 PM, Robert Marko wrote:
>>>>>> IPQ8074 comes in 2 families:
>>>>>> * IPQ8070A/IPQ8071A (Acorn) up to 1.4GHz
>>>>>> * IPQ8072A/IPQ8074A/IPQ8076A/IPQ8078A (Hawkeye) up to 2.2GHz
>>>>>>
>>>>>> So, in order to be able to share one OPP table lets add support for
>>>>>> IPQ8074
>>>>>> family based of SMEM SoC ID-s as speedbin fuse is always 0 on IPQ8074.
>>>>>>
>>>>>> IPQ8074 compatible is blacklisted from DT platdev as the cpufreq
>>>>>> device
>>>>>> will get created by NVMEM CPUFreq driver.
>>>>>>
>>>>>> Signed-off-by: Robert Marko <robimarko@gmail.com>
>>>>>> ---
>>>>>> Changes in v2:
>>>>>> * Print an error if SMEM ID is not part of the IPQ8074 family
>>>>>> and restrict the speed to Acorn variant (1.4GHz)
>>>>>> ---
>>>>>> drivers/cpufreq/cpufreq-dt-platdev.c | 1 +
>>>>>> drivers/cpufreq/qcom-cpufreq-nvmem.c | 43
>>>>>> ++++++++++++++++++++++++++++
>>>>>> 2 files changed, 44 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c
>>>>>> b/drivers/cpufreq/cpufreq-dt-platdev.c
>>>>>> index ea86c9f3ed7a..78f6ff933f93 100644
>>>>>> --- a/drivers/cpufreq/cpufreq-dt-platdev.c
>>>>>> +++ b/drivers/cpufreq/cpufreq-dt-platdev.c
>>>>>> @@ -170,6 +170,7 @@ static const struct of_device_id blocklist[]
>>>>>> __initconst = {
>>>>>> { .compatible = "ti,am62a7", },
>>>>>>
>>>>>> { .compatible = "qcom,ipq8064", },
>>>>>> + { .compatible = "qcom,ipq8074", },
>>>>>> { .compatible = "qcom,apq8064", },
>>>>>> { .compatible = "qcom,msm8974", },
>>>>>> { .compatible = "qcom,msm8960", },
>>>>>> diff --git a/drivers/cpufreq/qcom-cpufreq-nvmem.c
>>>>>> b/drivers/cpufreq/qcom-cpufreq-nvmem.c
>>>>>> index a88b6fe5db50..ce444b5962f2 100644
>>>>>> --- a/drivers/cpufreq/qcom-cpufreq-nvmem.c
>>>>>> +++ b/drivers/cpufreq/qcom-cpufreq-nvmem.c
>>>>>> @@ -31,6 +31,9 @@
>>>>>>
>>>>>> #include <dt-bindings/arm/qcom,ids.h>
>>>>>>
>>>>>> +#define IPQ8074_HAWKEYE_VERSION BIT(0)
>>>>>> +#define IPQ8074_ACORN_VERSION BIT(1)
>>>>>> +
>>>>>> struct qcom_cpufreq_drv;
>>>>>>
>>>>>> struct qcom_cpufreq_match_data {
>>>>>> @@ -204,6 +207,41 @@ static int
>>>>>> qcom_cpufreq_krait_name_version(struct device *cpu_dev,
>>>>>> return ret;
>>>>>> }
>>>>>>
>>>>>> +static int qcom_cpufreq_ipq8074_name_version(struct device *cpu_dev,
>>>>>> + struct nvmem_cell
>>>>>> *speedbin_nvmem,
>>>>>> + char **pvs_name,
>>>>>> + struct qcom_cpufreq_drv
>>>>>> *drv)
>>>>>
>>>>> Most of the IPQ SoCs also supports the fuse based frequency selection.
>>>>> Can we rename the function name to generic so that all the IPQ chips
>>>>> can
>>>>> use the same function?
>>>> Well, the only speedbin fuse I was able to dig from downstream is the
>>>> one from
>>>> CPR driver and that one is 0 on all devices so it's not helpful.
>>>> Do you maybe know if there is one in the IPQ8074 family?
>>>
>>>
>>> Let me check on this and get back to you probably by tomorrow...
>>
>>
>> Robert, checked with the team and IPQ807x doesn't use fuse to determine
>> the CPU freq limits. Current approach (SoC ID based) should be fine.
>> BTW, are the DTS changes already posted or yet to be posted?
>
> Thanks for checking,
> DTS changes are not posted as CPR support is required in order for scaling to
> properly work, otherwise, all I could do is try and guess some safe voltages.
> There was an effort to get CPR upstreamed, but I think that stalled out for now.
As much as I don't like it, yes it's stalled.. I have to get some bigger
fish out of my queue first.
Konrad
>
> Regards,
> Robert
>>
>>
>>>
>>>
>>>>
>>>> Function is not supposed to be shared between SoC-s, so I dont see a
>>>> point in it
>>>> having a generic name cause for example IPQ6018 has a working fuse
>>>> and its logic
>>>> is completely different for setting the versioning than IPQ8074, I
>>>> dont think having a
>>>> catch-all would work here.
>>>
>>>
>>> Makes sense, thanks Robert and Konrad.
>>>
>>>
>>>>
>>>>>
>>>>>> +{
>>>>>> + u32 msm_id;
>>>>>
>>>>> soc_id please...?
>>>> Sure, that is more suitable.
>>>>
>>>> Regards,
>>>> Robert
>>>>>
>>>>>> + int ret;
>>>>>> + *pvs_name = NULL;
>>>>>> +
>>>>>> + ret = qcom_smem_get_soc_id(&msm_id);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> +
>>>>>> + switch (msm_id) {
>>>>>> + case QCOM_ID_IPQ8070A:
>>>>>> + case QCOM_ID_IPQ8071A:
>>>>>> + drv->versions = IPQ8074_ACORN_VERSION;
>>>>>> + break;
>>>>>> + case QCOM_ID_IPQ8072A:
>>>>>> + case QCOM_ID_IPQ8074A:
>>>>>> + case QCOM_ID_IPQ8076A:
>>>>>> + case QCOM_ID_IPQ8078A:
>>>>>> + drv->versions = IPQ8074_HAWKEYE_VERSION;
>>>>>> + break;
>>>>>> + default:
>>>>>> + dev_err(cpu_dev,
>>>>>> + "SoC ID %u is not part of IPQ8074 family,
>>>>>> limiting to 1.4GHz!\n",
>>>>>> + msm_id);
>>>>>> + drv->versions = IPQ8074_ACORN_VERSION;
>>>>>> + break;
>>>>>> + }
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> static const struct qcom_cpufreq_match_data match_data_kryo = {
>>>>>> .get_version = qcom_cpufreq_kryo_name_version,
>>>>>> };
>>>>>> @@ -218,6 +256,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_ipq8074 = {
>>>>>> + .get_version = qcom_cpufreq_ipq8074_name_version,
>>>>>> +};
>>>>>> +
>>>>>> static int qcom_cpufreq_probe(struct platform_device *pdev)
>>>>>> {
>>>>>> struct qcom_cpufreq_drv *drv;
>>>>>> @@ -363,6 +405,7 @@ static const struct of_device_id
>>>>>> qcom_cpufreq_match_list[] __initconst = {
>>>>>> { .compatible = "qcom,msm8996", .data = &match_data_kryo },
>>>>>> { .compatible = "qcom,qcs404", .data = &match_data_qcs404 },
>>>>>> { .compatible = "qcom,ipq8064", .data = &match_data_krait },
>>>>>> + { .compatible = "qcom,ipq8074", .data = &match_data_ipq8074 },
>>>>>> { .compatible = "qcom,apq8064", .data = &match_data_krait },
>>>>>> { .compatible = "qcom,msm8974", .data = &match_data_krait },
>>>>>> { .compatible = "qcom,msm8960", .data = &match_data_krait },
^ permalink raw reply [flat|nested] 19+ messages in thread