Date: Wed, 28 Feb 2024 23:01:50 +0530
From: Sibi Sankar <quic_sibis@...cinc.com>
To: Cristian Marussi <cristian.marussi@....com>
CC: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>, <sudeep.holla@....com>,
        <andersson@...nel.org>, <konrad.dybcio@...aro.org>,
        <jassisinghbrar@...il.com>, <robh+dt@...nel.org>,
        <krzysztof.kozlowski+dt@...aro.org>, <linux-kernel@...r.kernel.org>,
        <linux-arm-msm@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <quic_rgottimu@...cinc.com>, <quic_kshivnan@...cinc.com>,
        <conor+dt@...nel.org>, Amir Vajid <avajid@...cinc.com>
Subject: Re: [RFC 4/7] soc: qcom: Utilize qcom scmi vendor protocol for bus
 dvfs



On 2/20/24 20:37, Cristian Marussi wrote:
> On Mon, Feb 12, 2024 at 03:54:27PM +0530, Sibi Sankar wrote:
>>
>>
>> On 1/18/24 02:11, Dmitry Baryshkov wrote:
>>> On Wed, 17 Jan 2024 at 19:36, Sibi Sankar <quic_sibis@...cinc.com> wrote:
> 
> Hi,
> 
> I'll comment this patch fully, just a remark down below about this
> mail-thread.
> 
>>>>
>>>> From: Shivnandan Kumar <quic_kshivnan@...cinc.com>
>>>>
>>>> This patch introduces a client driver that interacts with the SCMI QCOM
>>>
>>> git grep This.patch Documentation/process/
>>>
>>>> vendor protocol and passes on the required tuneables to start various
>>>> features running on the SCMI controller.
>>>
>>> Is there any word about the 'memlat'? No. Unless one  reads into the
>>> patch, one can not come up with the idea of what is being introduced.
>>
>> ack, will fix it in the re-spin.
>>
>>>
>>>>
>>>> Signed-off-by: Shivnandan Kumar <quic_kshivnan@...cinc.com>
>>>> Co-developed-by: Ramakrishna Gottimukkula <quic_rgottimu@...cinc.com>
>>>> Signed-off-by: Ramakrishna Gottimukkula <quic_rgottimu@...cinc.com>
>>>> Co-developed-by: Amir Vajid <avajid@...cinc.com>
>>>> Signed-off-by: Amir Vajid <avajid@...cinc.com>
>>>> Co-developed-by: Sibi Sankar <quic_sibis@...cinc.com>
>>>> Signed-off-by: Sibi Sankar <quic_sibis@...cinc.com>
>>>> ---
>>>>    drivers/soc/qcom/Kconfig            |  10 +
>>>>    drivers/soc/qcom/Makefile           |   1 +
>>>>    drivers/soc/qcom/qcom_scmi_client.c | 486 ++++++++++++++++++++++++++++
>>>
>>> Should it go to drivers/firmware/arm_scmi instead? Or maybe to drivers/devfreq?
>>
>> I don't think it should go into arm_scmi unless Sudeep wants it there.
>> As for drivers/devfreq, I would have moved it there if this driver
>> benfitted being classified as a devfreq device. We can't use any of
>> the available governors on it and the tuneables appear way too custom.
>> These are the reasons why I placed it in drivers/soc/qcom instead.
>>
> 
> I think we used to host a couple of generic SCMI driver related to
> standard protocols but they have been moved out of driver/firmware/arm_scmi
> into the related subsystem...not sure if Sudeep thinks otherwise but I
> suppose we want to host only SCMI drivers that are clearly lacking a
> place where to stay...

I see and it makes sense, since it would go through an additional layer
of review from the corresponding sub-system maintainers and only after
sufficient push back it gets to land in driver/firmware/arm_scmi.

> 
>>>
>>>>    3 files changed, 497 insertions(+)
>>>>    create mode 100644 drivers/soc/qcom/qcom_scmi_client.c
>   
>   [snip]
> 
>>>> +static int configure_cpucp_mon(struct scmi_memlat_info *info, int memory_index, int monitor_index)
>>>> +{
>>>> +       const struct qcom_scmi_vendor_ops *ops = info->ops;
>>>> +       struct scmi_memory_info *memory = info->memory[memory_index];
>>>> +       struct scmi_monitor_info *monitor = memory->monitor[monitor_index];
>>>> +       struct scalar_param_msg scalar_msg;
>>>> +       struct map_param_msg map_msg;
>>>> +       struct node_msg msg;
>>>> +       int ret;
>>>> +       int i;
>>>> +
>>>> +       msg.cpumask = monitor->mask;
>>>> +       msg.hw_type = memory->hw_type;
>>>> +       msg.mon_type = monitor->mon_type;
>>>> +       msg.mon_idx = monitor->mon_idx;
>>>> +       strscpy(msg.mon_name, monitor->mon_name, sizeof(msg.mon_name));
>>>> +       ret = ops->set_param(info->ph, &msg, MEMLAT_ALGO_STR, MEMLAT_SET_MONITOR, sizeof(msg));
>>>> +       if (ret < 0) {
>>>> +               pr_err("Failed to configure monitor %s\n", monitor->mon_name);
>>>> +               return ret;
>>>> +       }
>>>> +
>>>> +       scalar_msg.hw_type = memory->hw_type;
>>>> +       scalar_msg.mon_idx = monitor->mon_idx;
>>>> +       scalar_msg.val = monitor->ipm_ceil;
>>>> +       ret = ops->set_param(info->ph, &scalar_msg, MEMLAT_ALGO_STR, MEMLAT_IPM_CEIL,
>>>> +                            sizeof(scalar_msg));
>>>> +       if (ret < 0) {
>>>> +               pr_err("Failed to set ipm ceil for %s\n", monitor->mon_name);
>>>> +               return ret;
>>>> +       }
>>>> +
>>>> +       map_msg.hw_type = memory->hw_type;
>>>> +       map_msg.mon_idx = monitor->mon_idx;
>>>> +       map_msg.nr_rows = monitor->freq_map_len;
>>>> +       for (i = 0; i < monitor->freq_map_len; i++) {
>>>> +               map_msg.tbl[i].v1 = monitor->freq_map[i].cpufreq_mhz;
>>>> +               map_msg.tbl[i].v2 = monitor->freq_map[i].memfreq_khz / 1000;
>>>> +       }
>>>
>>> So this table goes 1:1 to the firmware? Is it going to be the same for
>>> all versions of the SoC? If so, it might be better to turn it into the
>>> static data inside the driver. If it doesn't change, there is no need
>>> to put it to DT.
>>
>> The table does go directly to the firmware but obviously varies across
>> SoCs. Also since it's a SCMI client driver we don't have a way to
>> distinguish between SoCs based on compatibles. So it made more sense to
>> move it to the device tree instead.
>>
> 
> Well, the SCMI fw running the server DOES know where it is running right ?
> 
> So, if you have multiple fixed config tables to feed into the firmware
> that vary based on the SoC you are running on, you could add an SCMI command
> to your QCOM SCMI vendor protocol and expose a related operation in ops to get
> the actual SoC model, so that you can embed the tableS in the driver here (as
> suggested) and then choose at runtime which one to use based on the reported
> platform...this is clearly config stuff (sa said by others) so it just
> does not belong to DT descriptions.

I still think changing it to opp-tables from qcom,cpufreq-memfreq-tbl
like Konrad/Dmitry suggested is the the right way to go for the
following reason. I would expect the SCP FW running on the SoC to be
board invariant i.e. a SoC can support multiple variants of a memory
that we are trying to scale like ram (lpddr4/lpddr5) so the having them
in the device tree actually allows us to configure the supported ddr
frequencies by overriding those in the board files. Thus closer to the
actual representation. Also like I explained just getting the SoC info
won't be sufficient since the tables are expected to change across
boards when there is a change in type or supported frequencies.

-Sibi

> 
> Thanks,
> Cristian

Powered by blists - more mailing lists