Message-ID: <ZdTANrUZuN_UZW9j@pluto>
Date: Tue, 20 Feb 2024 15:07:34 +0000
From: Cristian Marussi <cristian.marussi@....com>
To: Sibi Sankar <quic_sibis@...cinc.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 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...

> > 
> > >   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. 

Thanks,
Cristian

Powered by blists - more mailing lists