Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <70de3314-766d-4c7f-5b1a-41740cfeac8c@quicinc.com>
Date:   Thu, 1 Jun 2023 18:54:23 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.0
Subject: Re: [RESEND PATCH v2 1/2] cpufreq: qcom-nvmem: add support for
 IPQ8074
Content-Language: en-US
To:     Robert Marko <robimarko@gmail.com>
CC:     <rafael@kernel.org>, <viresh.kumar@linaro.org>,
        <agross@kernel.org>, <andersson@kernel.org>,
        <konrad.dybcio@linaro.org>, <ilia.lin@kernel.org>,
        <linux-pm@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <linux-arm-msm@vger.kernel.org>, <ansuelsmth@gmail.com>
References: <20230530165807.642084-1-robimarko@gmail.com>
 <b565b9b5-d7c1-7779-532e-565c3f5c5cd3@quicinc.com>
 <CAOX2RU6ay_Bc0JYQ6rBcTRadm-71Jie5YH9B0J_1UywkcyqZ8g@mail.gmail.com>
From:   Kathiravan T <quic_kathirav@quicinc.com>
In-Reply-To: <CAOX2RU6ay_Bc0JYQ6rBcTRadm-71Jie5YH9B0J_1UywkcyqZ8g@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


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 },