Received-SPF: pass (google.com: domain of linux-kernel+bounces-61678-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240117173458.2312669-1-quic_sibis@quicinc.com>
 <20240117173458.2312669-5-quic_sibis@quicinc.com> <CAA8EJpr8qLZ8Y7PjU05ZoxSHWOf=q-KtM+s-tnR5X2t96rFWhw@mail.gmail.com>
 <0adaa065-3883-ebfe-8259-05ebdbd821eb@quicinc.com>
In-Reply-To: <0adaa065-3883-ebfe-8259-05ebdbd821eb@quicinc.com>
From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Date: Mon, 12 Feb 2024 15:22:49 +0200
Message-ID: <CAA8EJpqjqCVs6DEaUQowk2d3yS66cSC8vnVQzv+MxhvobeNvEg@mail.gmail.com>
Subject: Re: [RFC 4/7] soc: qcom: Utilize qcom scmi vendor protocol for bus dvfs
To: Sibi Sankar <quic_sibis@quicinc.com>
Cc: sudeep.holla@arm.com, cristian.marussi@arm.com, andersson@kernel.org, 
	konrad.dybcio@linaro.org, jassisinghbrar@gmail.com, robh+dt@kernel.org, 
	krzysztof.kozlowski+dt@linaro.org, linux-kernel@vger.kernel.org, 
	linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org, 
	quic_rgottimu@quicinc.com, quic_kshivnan@quicinc.com, conor+dt@kernel.org, 
	Amir Vajid <avajid@quicinc.com>
Content-Type: text/plain; charset="UTF-8"

On Mon, 12 Feb 2024 at 12:24, Sibi Sankar <quic_sibis@quicinc.com> wrote:
>
>
>
> On 1/18/24 02:11, Dmitry Baryshkov wrote:
> > On Wed, 17 Jan 2024 at 19:36, Sibi Sankar <quic_sibis@quicinc.com> wrote:
> >>
> >> From: Shivnandan Kumar <quic_kshivnan@quicinc.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@quicinc.com>
> >> Co-developed-by: Ramakrishna Gottimukkula <quic_rgottimu@quicinc.com>
> >> Signed-off-by: Ramakrishna Gottimukkula <quic_rgottimu@quicinc.com>
> >> Co-developed-by: Amir Vajid <avajid@quicinc.com>
> >> Signed-off-by: Amir Vajid <avajid@quicinc.com>
> >> Co-developed-by: Sibi Sankar <quic_sibis@quicinc.com>
> >> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.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.
>
> >
> >>   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