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@xxxxxxxxxxx> wrote:
Hi,
I'll comment this patch fully, just a remark down below about this
mail-thread.
From: Shivnandan Kumar <quic_kshivnan@xxxxxxxxxxx>
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@xxxxxxxxxxx>
Co-developed-by: Ramakrishna Gottimukkula <quic_rgottimu@xxxxxxxxxxx>
Signed-off-by: Ramakrishna Gottimukkula <quic_rgottimu@xxxxxxxxxxx>
Co-developed-by: Amir Vajid <avajid@xxxxxxxxxxx>
Signed-off-by: Amir Vajid <avajid@xxxxxxxxxxx>
Co-developed-by: Sibi Sankar <quic_sibis@xxxxxxxxxxx>
Signed-off-by: Sibi Sankar <quic_sibis@xxxxxxxxxxx>
---
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
