On Mon, 12 Feb 2024 at 12:24, Sibi Sankar <quic_sibis@xxxxxxxxxxx> wrote:
>
>
>
> On 1/18/24 02:11, Dmitry Baryshkov wrote:
> > On Wed, 17 Jan 2024 at 19:36, Sibi Sankar <quic_sibis@xxxxxxxxxxx> wrote:
> >>
> >> 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.
>
> >
> >> 3 files changed, 497 insertions(+)
> >> create mode 100644 drivers/soc/qcom/qcom_scmi_client.c
> >>
> >> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> >> index c6ca4de42586..1530558aebfb 100644
> >> --- a/drivers/soc/qcom/Kconfig
> >> +++ b/drivers/soc/qcom/Kconfig
> >> @@ -264,6 +264,16 @@ config QCOM_ICC_BWMON
> >> the fixed bandwidth votes from cpufreq (CPU nodes) thus achieve high
> >> memory throughput even with lower CPU frequencies.
> >>
> >> +config QCOM_SCMI_CLIENT
> >> + tristate "Qualcomm Technologies Inc. SCMI client driver"
> >> + depends on QCOM_SCMI_VENDOR_PROTOCOL || COMPILE_TEST
> >> + default n
> >> + help
> >> + SCMI client driver registers for SCMI QCOM vendor protocol.
> >> +
> >> + This driver interacts with the vendor protocol and passes on the required
> >> + tuneables to start various features running on the SCMI controller.
> >> +
> >> config QCOM_INLINE_CRYPTO_ENGINE
> >> tristate
> >> select QCOM_SCM
> >> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> >> index 05b3d54e8dc9..c2a51293c886 100644
> >> --- a/drivers/soc/qcom/Makefile
> >> +++ b/drivers/soc/qcom/Makefile
> >> @@ -32,5 +32,6 @@ obj-$(CONFIG_QCOM_APR) += apr.o
> >> obj-$(CONFIG_QCOM_LLCC) += llcc-qcom.o
> >> obj-$(CONFIG_QCOM_KRYO_L2_ACCESSORS) += kryo-l2-accessors.o
> >> obj-$(CONFIG_QCOM_ICC_BWMON) += icc-bwmon.o
> >> +obj-$(CONFIG_QCOM_SCMI_CLIENT) += qcom_scmi_client.o
> >> qcom_ice-objs += ice.o
> >> obj-$(CONFIG_QCOM_INLINE_CRYPTO_ENGINE) += qcom_ice.o
> >> diff --git a/drivers/soc/qcom/qcom_scmi_client.c b/drivers/soc/qcom/qcom_scmi_client.c
> >> new file mode 100644
> >> index 000000000000..418aa7900496
> >> --- /dev/null
> >> +++ b/drivers/soc/qcom/qcom_scmi_client.c
> >> @@ -0,0 +1,486 @@
> >> +// SPDX-License-Identifier: GPL-2.0-only
> >> +/*
> >> + * Copyright (c) 2024, The Linux Foundation. All rights reserved.
> >> + */
> >> +
> >> +#include <linux/cpu.h>
> >> +#include <linux/err.h>
> >> +#include <linux/errno.h>
> >> +#include <linux/init.h>
> >> +#include <linux/kernel.h>
> >> +#include <linux/module.h>
> >> +#include <linux/of.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/qcom_scmi_vendor.h>
> >> +#include <linux/scmi_protocol.h>
> >> +
> >> +#define MAX_MEMORY_TYPES 3
> >> +#define MEMLAT_ALGO_STR 0x74616C6D656D /* "memlat" */
> >> +#define INVALID_IDX 0xFF
> >> +#define MAX_NAME_LEN 20
> >> +#define MAX_MAP_ENTRIES 6
> >> +#define MAX_MONITOR_CNT 4
> >> +#define SCMI_VENDOR_MSG_START 3
> >> +#define SCMI_VENDOR_MSG_MODULE_START 16
> >> +
> >> +enum scmi_memlat_protocol_cmd {
> >> + MEMLAT_SET_LOG_LEVEL = SCMI_VENDOR_MSG_START,
> >> + MEMLAT_FLUSH_LOGBUF,
> >> + MEMLAT_SET_MEM_GROUP = SCMI_VENDOR_MSG_MODULE_START,
> >> + MEMLAT_SET_MONITOR,
> >> + MEMLAT_SET_COMMON_EV_MAP,
> >> + MEMLAT_SET_GRP_EV_MAP,
> >> + MEMLAT_ADAPTIVE_LOW_FREQ,
> >> + MEMLAT_ADAPTIVE_HIGH_FREQ,
> >> + MEMLAT_GET_ADAPTIVE_CUR_FREQ,
> >> + MEMLAT_IPM_CEIL,
> >> + MEMLAT_FE_STALL_FLOOR,
> >> + MEMLAT_BE_STALL_FLOOR,
> >> + MEMLAT_WB_PCT,
> >> + MEMLAT_IPM_FILTER,
> >> + MEMLAT_FREQ_SCALE_PCT,
> >> + MEMLAT_FREQ_SCALE_CEIL_MHZ,
> >> + MEMLAT_FREQ_SCALE_FLOOR_MHZ,
> >> + MEMLAT_SAMPLE_MS,
> >> + MEMLAT_MON_FREQ_MAP,
> >> + MEMLAT_SET_MIN_FREQ,
> >> + MEMLAT_SET_MAX_FREQ,
> >> + MEMLAT_GET_CUR_FREQ,
> >> + MEMLAT_START_TIMER,
> >> + MEMLAT_STOP_TIMER,
> >> + MEMLAT_GET_TIMESTAMP,
> >> + MEMLAT_MAX_MSG
> >> +};
> >> +
> >> +struct map_table {
> >> + u16 v1;
> >> + u16 v2;
> >> +};
> >> +
> >
> > Any documentation for these structures? It won't bite you, but it will
> > help reviewers and other developers.
>
> ack
>
> >
> >> +struct map_param_msg {
> >> + u32 hw_type;
> >> + u32 mon_idx;
> >> + u32 nr_rows;
> >> + struct map_table tbl[MAX_MAP_ENTRIES];
> >> +} __packed;
> >> +
> >> +struct node_msg {
> >> + u32 cpumask;
> >> + u32 hw_type;
> >> + u32 mon_type;
> >> + u32 mon_idx;
> >> + char mon_name[MAX_NAME_LEN];
> >> +};
> >> +
> >> +struct scalar_param_msg {
> >> + u32 hw_type;
> >> + u32 mon_idx;
> >> + u32 val;
> >> +};
> >> +
> >> +enum common_ev_idx {
> >> + INST_IDX,
> >> + CYC_IDX,
> >> + FE_STALL_IDX,
> >> + BE_STALL_IDX,
> >> + NUM_COMMON_EVS
> >> +};
> >> +
> >> +enum grp_ev_idx {
> >> + MISS_IDX,
> >> + WB_IDX,
> >> + ACC_IDX,
> >> + NUM_GRP_EVS
> >> +};
> >> +
> >> +#define EV_CPU_CYCLES 0
> >> +#define EV_INST_RETIRED 2
> >> +#define EV_L2_D_RFILL 5
> >> +
> >> +struct ev_map_msg {
> >> + u32 num_evs;
> >> + u32 hw_type;
> >> + u32 cid[NUM_COMMON_EVS];
> >> +};
> >> +
> >> +struct cpufreq_memfreq_map {
> >> + unsigned int cpufreq_mhz;
> >> + unsigned int memfreq_khz;
> >> +};
> >> +
> >> +struct scmi_monitor_info {
> >> + struct cpufreq_memfreq_map *freq_map;
> >> + char mon_name[MAX_NAME_LEN];
> >> + u32 mon_idx;
> >> + u32 mon_type;
> >> + u32 ipm_ceil;
> >> + u32 mask;
> >> + u32 freq_map_len;
> >> +};
> >> +
> >> +struct scmi_memory_info {
> >> + struct scmi_monitor_info *monitor[MAX_MONITOR_CNT];
> >> + u32 hw_type;
> >> + int monitor_cnt;
> >> + u32 min_freq;
> >> + u32 max_freq;
> >> +};
> >> +
> >> +struct scmi_memlat_info {
> >> + struct scmi_protocol_handle *ph;
> >> + const struct qcom_scmi_vendor_ops *ops;
> >> + struct scmi_memory_info *memory[MAX_MEMORY_TYPES];
> >> + int memory_cnt;
> >> +};
> >> +
> >> +static int get_mask(struct device_node *np, u32 *mask)
> >> +{
> >> + struct device_node *dev_phandle;
> >> + struct device *cpu_dev;
> >> + int cpu, i = 0;
> >> + int ret = -ENODEV;
> >> +
> >> + dev_phandle = of_parse_phandle(np, "qcom,cpulist", i++);
> >> + while (dev_phandle) {
> >> + for_each_possible_cpu(cpu) {
> >> + cpu_dev = get_cpu_device(cpu);
> >> + if (cpu_dev && cpu_dev->of_node == dev_phandle) {
> >> + *mask |= BIT(cpu);
> >> + ret = 0;
> >> + break;
> >> + }
> >> + }
> >
> > There is of_cpu_node_to_id(). No need to reinvent it.
>
> ack
>
> >
> >> + dev_phandle = of_parse_phandle(np, "qcom,cpulist", i++);
> >> + }
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +static struct cpufreq_memfreq_map *init_cpufreq_memfreq_map(struct device *dev,
> >> + struct device_node *of_node,
> >> + u32 *cnt)
> >> +{
> >> + int len, nf, i, j;
> >> + u32 data;
> >> + struct cpufreq_memfreq_map *tbl;
> >> + int ret;
> >> +
> >> + if (!of_find_property(of_node, "qcom,cpufreq-memfreq-tbl", &len))
> >> + return NULL;
> >> + len /= sizeof(data);
> >> +
> >> + if (len % 2 || len == 0)
> >> + return NULL;
> >> + nf = len / 2;
> >> +
> >> + tbl = devm_kzalloc(dev, (nf + 1) * sizeof(struct cpufreq_memfreq_map),
> >> + GFP_KERNEL);
> >> + if (!tbl)
> >> + return NULL;
> >> +
> >> + for (i = 0, j = 0; i < nf; i++, j += 2) {
> >> + ret = of_property_read_u32_index(of_node, "qcom,cpufreq-memfreq-tbl",
> >> + j, &data);
> >> + if (ret < 0)
> >> + return NULL;
> >> + tbl[i].cpufreq_mhz = data / 1000;
> >> +
> >> + ret = of_property_read_u32_index(of_node, "qcom,cpufreq-memfreq-tbl",
> >> + j + 1, &data);
> >> + if (ret < 0)
> >> + return NULL;
> >> +
> >> + tbl[i].memfreq_khz = data;
> >> + pr_debug("Entry%d CPU:%u, Mem:%u\n", i, tbl[i].cpufreq_mhz,
> >> + tbl[i].memfreq_khz);
> >> + }
> >> + *cnt = nf;
> >> + tbl[i].cpufreq_mhz = 0;
> >
> > This looks like a lame version of the OPP table.
>
> I didn't know listing multiple frequencies in a opp was allowed. We can
> probably get away with it here since we just parse the data here and not
> populate data in the opp core.
You are describing driver behaviour. DT describes hardware. So, the
proper way to describe this kind of data is the OPP table.
--
With best wishes
Dmitry
