Re: [RESEND PATCH v2 1/2] cpufreq: qcom-nvmem: add support for IPQ8074

[Konrad Dybcio]

On 1.06.2023 16:55, Robert Marko wrote:
> On Thu, 1 Jun 2023 at 16:49, Kathiravan T <quic_kathirav@xxxxxxxxxxx> 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@xxxxxxxxxxx>
>>>> 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@xxxxxxxxx>
>>>>>> ---
>>>>>> 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 },